On Thu, May 21, 2015 at 05:04:15PM +0300, Ville Syrjälä wrote:
> On Wed, May 20, 2015 at 07:12:17PM -0700, Matt Roper wrote:
> > Various places in the atomic plane code obtain the CRTC via
> > plane_state->crtc. But plane_state->crtc is NULL when disabling the
> > plane, so the code will fall back to looking at the old CRTC value in
> > plane->crtc in that case. This isn't going to work when we start
> > supporting non-blocking flips (since the legacy plane->crtc pointer will
> > already be updated to its new value before the asynchronous workqueue
> > task runs the plane commit function). The code is also fragile in
> > general (we have to be careful when doing stuff like updating properties
> > on a disabled plane). Since Intel hardware has a fixed plane to pipe
> > mapping, let's just lookup the CRTC via that fixed mapping and avoid
> > future mistakes.
>
> Older gens have non-fixed mapping. So I think we should really be looking
> at the state (future state in check, current state in commit). Sadly last
> time I tried to do that the state was still all lies and resulted in
> explosions. Dunno if things are better now.
Yeah I think for planes that can switch around we'd need to look at
states. But that's only possible once all the reworking has settled down a
bit. Luckily we don't support any such planes (yet).
-Daniel
>
> >
> > Cc: Vivek Kasireddy <vivek.kasireddy@xxxxxxxxx>
> > Reported-by: Vivek Kasireddy <vivek.kasireddy@xxxxxxxxx>
> > Signed-off-by: Matt Roper <matthew.d.roper@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/intel_atomic_plane.c | 12 ++++++------
> > drivers/gpu/drm/i915/intel_display.c | 8 ++++----
> > drivers/gpu/drm/i915/intel_drv.h | 7 +++++++
> > drivers/gpu/drm/i915/intel_sprite.c | 4 ++--
> > 4 files changed, 19 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
> > index 714ee24..21c808d 100644
> > --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> > +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> > @@ -115,16 +115,16 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
> > struct intel_plane *intel_plane = to_intel_plane(plane);
> > struct intel_plane_state *intel_state = to_intel_plane_state(state);
> >
> > - crtc = crtc ? crtc : plane->crtc;
> > + crtc = intel_get_crtc_for_drm_plane(plane);
> > intel_crtc = to_intel_crtc(crtc);
> >
> > /*
> > - * Both crtc and plane->crtc could be NULL if we're updating a
> > - * property while the plane is disabled. We don't actually have
> > - * anything driver-specific we need to test in that case, so
> > - * just return success.
> > + * Both state->crtc and plane->state->crtc could be NULL if we're
> > + * updating a property while the plane is disabled. We don't actually
> > + * have anything driver-specific we need to test in that case, so just
> > + * return success.
> > */
> > - if (!crtc)
> > + if (!state->crtc && !plane->state->crtc)
> > return 0;
> >
> > crtc_state = intel_crtc_state_for_plane(intel_state);
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 1a7d2a9..f4398a7 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -13092,7 +13092,7 @@ intel_check_primary_plane(struct drm_plane *plane,
> > int min_scale = DRM_PLANE_HELPER_NO_SCALING;
> > int ret;
> >
> > - crtc = crtc ? crtc : plane->crtc;
> > + crtc = intel_get_crtc_for_drm_plane(plane);
> > intel_crtc = to_intel_crtc(crtc);
> > crtc_state = intel_crtc_state_for_plane(state);
> >
> > @@ -13174,7 +13174,7 @@ intel_commit_primary_plane(struct drm_plane *plane,
> > struct intel_crtc *intel_crtc;
> > struct drm_rect *src = &state->src;
> >
> > - crtc = crtc ? crtc : plane->crtc;
> > + crtc = intel_get_crtc_for_drm_plane(plane);
> > intel_crtc = to_intel_crtc(crtc);
> >
> > plane->fb = fb;
> > @@ -13409,7 +13409,7 @@ intel_check_cursor_plane(struct drm_plane *plane,
> > unsigned stride;
> > int ret;
> >
> > - crtc = crtc ? crtc : plane->crtc;
> > + crtc = intel_get_crtc_for_drm_plane(plane);
> > intel_crtc = to_intel_crtc(crtc);
> >
> > ret = drm_plane_helper_check_update(plane, crtc, fb,
> > @@ -13482,7 +13482,7 @@ intel_commit_cursor_plane(struct drm_plane *plane,
> > struct drm_i915_gem_object *obj = intel_fb_obj(state->base.fb);
> > uint32_t addr;
> >
> > - crtc = crtc ? crtc : plane->crtc;
> > + crtc = intel_get_crtc_for_drm_plane(plane);
> > intel_crtc = to_intel_crtc(crtc);
> >
> > plane->fb = state->base.fb;
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 1e22ffe..ea67093 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -803,6 +803,13 @@ intel_get_crtc_for_plane(struct drm_device *dev, int plane)
> > return dev_priv->plane_to_crtc_mapping[plane];
> > }
> >
> > +static inline struct drm_crtc *
> > +intel_get_crtc_for_drm_plane(struct drm_plane *plane)
> > +{
> > + struct drm_i915_private *dev_priv = to_i915(plane->dev);
> > + return dev_priv->pipe_to_crtc_mapping[to_intel_plane(plane)->pipe];
> > +}
> > +
> > struct intel_unpin_work {
> > struct work_struct work;
> > struct drm_crtc *crtc;
> > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> > index bfe9213..2164e63 100644
> > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > @@ -752,7 +752,7 @@ intel_check_sprite_plane(struct drm_plane *plane,
> > int pixel_size;
> > int ret;
> >
> > - intel_crtc = intel_crtc ? intel_crtc : to_intel_crtc(plane->crtc);
> > + intel_crtc = to_intel_crtc(intel_get_crtc_for_drm_plane(plane));
> > crtc_state = intel_crtc_state_for_plane(state);
> >
> > if (!fb) {
> > @@ -942,7 +942,7 @@ intel_commit_sprite_plane(struct drm_plane *plane,
> > unsigned int crtc_w, crtc_h;
> > uint32_t src_x, src_y, src_w, src_h;
> >
> > - crtc = crtc ? crtc : plane->crtc;
> > + crtc = intel_get_crtc_for_drm_plane(plane);
> > intel_crtc = to_intel_crtc(crtc);
> >
> > plane->fb = fb;
> > --
> > 1.8.5.1
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
