On Thu, Jun 28, 2018 at 7:36 PM, Ville Syrjälä ville.syrjala@linux.intel.com wrote:
On Thu, Jun 28, 2018 at 07:05:10PM +0200, Daniel Vetter wrote:
On Thu, Jun 28, 2018 at 04:54:56PM +0300, Ville Syrjala wrote:
From: Ville Syrjälä ville.syrjala@linux.intel.com
All the plane->fb/old_fb/crtc dance of __setplane_internal() is pointless on atomic drivers. So let's just introduce a simpler version that skips all that.
Ideally we could also skip the __setplane_check() as drm_atomic_plane_check() already checks for everything, but the legacy cursor/"async" .update_plane() tricks bypass that so we still need to call __setplane_check(). Toss in a FIXME to remind someone to clean this up later.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com
drivers/gpu/drm/drm_plane.c | 68 +++++++++++++++++++++++++++++++++++++-------- 1 file changed, 57 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c index 5c97a0131484..58702340808c 100644 --- a/drivers/gpu/drm/drm_plane.c +++ b/drivers/gpu/drm/drm_plane.c @@ -649,6 +649,8 @@ static int __setplane_internal(struct drm_plane *plane, { int ret = 0;
- WARN_ON(plane->state);
- /* No fb means shut it down */ if (!fb) { plane->old_fb = plane->fb;
@@ -673,11 +675,9 @@ static int __setplane_internal(struct drm_plane *plane, crtc_x, crtc_y, crtc_w, crtc_h, src_x, src_y, src_w, src_h, ctx); if (!ret) {
if (!plane->state) {plane->crtc = crtc;plane->fb = fb;drm_framebuffer_get(plane->fb);}
plane->crtc = crtc;plane->fb = fb;drm_framebuffer_get(plane->fb);@@ -690,6 +690,41 @@ static int __setplane_internal(struct drm_plane *plane, return ret; }
+static int __setplane_atomic(struct drm_plane *plane,
struct drm_crtc *crtc,struct drm_framebuffer *fb,int32_t crtc_x, int32_t crtc_y,uint32_t crtc_w, uint32_t crtc_h,uint32_t src_x, uint32_t src_y,uint32_t src_w, uint32_t src_h,struct drm_modeset_acquire_ctx *ctx)+{
- int ret;
- WARN_ON(!plane->state);
- /* No fb means shut it down */
- if (!fb)
return plane->funcs->disable_plane(plane, ctx);- /*
- FIXME: This is redundant with drm_atomic_plane_check(),
- but the legacy cursor/"async" .update_plane() tricks
- don't call that so we still need this here. Should remove
- this when all .update_plane() implementations have been
- fixed to call drm_atomic_plane_check().
- */
- ret = __setplane_check(plane, crtc, fb,
crtc_x, crtc_y, crtc_w, crtc_h,src_x, src_y, src_w, src_h);- if (ret)
return ret;- return plane->funcs->update_plane(plane, crtc, fb,
crtc_x, crtc_y, crtc_w, crtc_h,src_x, src_y, src_w, src_h, ctx);+}
static int setplane_internal(struct drm_plane *plane, struct drm_crtc *crtc, struct drm_framebuffer *fb, @@ -707,9 +742,15 @@ static int setplane_internal(struct drm_plane *plane, ret = drm_modeset_lock_all_ctx(plane->dev, &ctx); if (ret) goto fail;
- ret = __setplane_internal(plane, crtc, fb,
crtc_x, crtc_y, crtc_w, crtc_h,src_x, src_y, src_w, src_h, &ctx);
- if (plane->state)
We're not 100% consistent, but I think this here will make life harder for drivers transitioning to atomic. Not many left, but still some.
Please use drm_drv_uses_atomic_modeset() instead and check instead for DRIVER_ATOMIC in your WARN_ON in all the places you're checking for ->state in this patch and the next one.
We're already using obj->state as the atomic indicator here, so this patch itself isn't changing anything.
Huh, must have missed that when reviewing your legacy state removal series.
I guess you're suggesting that we change all the ->state checks we already have to the uses_atomic() thing instead? And the aim is to allow drives to add obj->state before actually implementing atomic fully? To me that feels like asking for trouble, but who knows, maybe I'm wrong.
Yup that's actually required by the transitional helpers. You start building up the ->state stuff before it's all fully in place, since otherwise you have a bit a chicken&egg situation. Which would mean a full driver rewrite to get to atomic instead of more gradual transition.
The one exception is the various getfoo ioctls, where the ->state stuff should be accurate enough as soon as it's there. -Daniel
With that fixed, on the series:
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
ret = __setplane_atomic(plane, crtc, fb,crtc_x, crtc_y, crtc_w, crtc_h,src_x, src_y, src_w, src_h, &ctx);- else
ret = __setplane_internal(plane, crtc, fb,crtc_x, crtc_y, crtc_w, crtc_h,src_x, src_y, src_w, src_h, &ctx);fail: if (ret == -EDEADLK) { @@ -841,9 +882,14 @@ static int drm_mode_cursor_universal(struct drm_crtc *crtc, src_h = fb->height << 16; }
- ret = __setplane_internal(plane, crtc, fb,
crtc_x, crtc_y, crtc_w, crtc_h,0, 0, src_w, src_h, ctx);
if (plane->state)
ret = __setplane_atomic(plane, crtc, fb,crtc_x, crtc_y, crtc_w, crtc_h,0, 0, src_w, src_h, ctx);else
ret = __setplane_internal(plane, crtc, fb,crtc_x, crtc_y, crtc_w, crtc_h,0, 0, src_w, src_h, ctx);if (fb) drm_framebuffer_put(fb);
-- 2.16.4
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
-- Ville Syrjälä Intel