Re: [PATCH 2/4] drm/i915: Lookup CRTC for plane directly

[Matt Roper <matthew.d.roper@intel.com>]

On Thu, Apr 09, 2015 at 02:44:05PM +0200, Daniel Vetter wrote:
> On Wed, Apr 08, 2015 at 06:56:52PM -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.
> > 
> > Cc: Vivek Kasireddy <vivek.kasireddy@intel.com>
> > Reported-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> 
> Because I'm a lazy reviewer ... does cocci agree that you've spotted all
> the drm_plane->crtc references?
> -Daniel

According to

        @@ struct drm_plane *P; @@
        * P->crtc

We have four remaining uses of drm_plane->crtc in the driver.  Two of
them are just updating it to properly mirror the value of
plane->state->crtc (e.g., in the load detect code), so those are good.
The other two uses are in intel_plane_restore(), which is intentionally
looking at what's already been committed and isn't in danger of being
used in the non-blocking/async code flow.  So I think we're okay now.


Matt

> 
> > ---
> >  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 90c4a82..740d1a6 100644
> > --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> > +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> > @@ -116,19 +116,19 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
> >  	struct intel_plane_state *intel_state = to_intel_plane_state(state);
> >  	bool active;
> >  
> > -	crtc = crtc ? crtc : plane->crtc;
> > +	crtc = intel_get_crtc_for_drm_plane(plane);
> >  	intel_crtc = to_intel_crtc(crtc);
> >  
> >  	intel_crtc_state = intel_crtc_state_for_plane(intel_state);
> >  	active = intel_crtc_state->base.enable;
> >  
> >  	/*
> > -	 * 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;
> >  
> >  	/*
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 88b0f69..1cf91ad 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -12625,7 +12625,7 @@ intel_check_primary_plane(struct drm_plane *plane,
> >  	bool active;
> >  	int ret;
> >  
> > -	crtc = crtc ? crtc : plane->crtc;
> > +	crtc = intel_get_crtc_for_drm_plane(plane);
> >  	intel_crtc = to_intel_crtc(crtc);
> >  
> >  	intel_crtc_state = intel_crtc_state_for_plane(state);
> > @@ -12694,7 +12694,7 @@ intel_commit_primary_plane(struct drm_plane *plane,
> >  	struct drm_rect *src = &state->src;
> >  	bool active;
> >  
> > -	crtc = crtc ? crtc : plane->crtc;
> > +	crtc = intel_get_crtc_for_drm_plane(plane);
> >  	intel_crtc = to_intel_crtc(crtc);
> >  
> >  	intel_crtc_state = intel_crtc_state_for_plane(state);
> > @@ -12916,7 +12916,7 @@ intel_check_cursor_plane(struct drm_plane *plane,
> >  	bool active;
> >  	int ret;
> >  
> > -	crtc = crtc ? crtc : plane->crtc;
> > +	crtc = intel_get_crtc_for_drm_plane(plane);
> >  	intel_crtc = to_intel_crtc(crtc);
> >  
> >  	intel_crtc_state = intel_crtc_state_for_plane(state);
> > @@ -12977,7 +12977,7 @@ intel_commit_cursor_plane(struct drm_plane *plane,
> >  	uint32_t addr;
> >  	bool active;
> >  
> > -	crtc = crtc ? crtc : plane->crtc;
> > +	crtc = intel_get_crtc_for_drm_plane(plane);
> >  	intel_crtc = to_intel_crtc(crtc);
> >  
> >  	intel_crtc_state = intel_crtc_state_for_plane(state);
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 71e0152..d5ea24f 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -731,6 +731,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 2016e78..492abcd 100644
> > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > @@ -878,7 +878,7 @@ intel_check_sprite_plane(struct drm_plane *plane,
> >  	int pixel_size;
> >  	bool active;
> >  
> > -	intel_crtc = intel_crtc ? intel_crtc : to_intel_crtc(plane->crtc);
> > +	intel_crtc = to_intel_crtc(intel_get_crtc_for_drm_plane(plane));
> >  
> >  	intel_crtc_state = intel_crtc_state_for_plane(state);
> >  	active = intel_crtc_state->base.enable;
> > @@ -1075,7 +1075,7 @@ intel_commit_sprite_plane(struct drm_plane *plane,
> >  	uint32_t src_x, src_y, src_w, src_h;
> >  	bool active;
> >  
> > -	crtc = crtc ? crtc : plane->crtc;
> > +	crtc = intel_get_crtc_for_drm_plane(plane);
> >  	intel_crtc = to_intel_crtc(crtc);
> >  
> >  	intel_crtc_state = intel_crtc_state_for_plane(state);
> > -- 
> > 1.8.5.1
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx