[Intel-gfx] [PATCH 02/12] drm/i915: set dev_priv->fbc.crtc before scheduling the enable work

[Chris Wilson]

On Fri, Nov 13, 2015 at 05:53:34PM -0200, Paulo Zanoni wrote:
> This thing where we need to get the crtc either from the work
> structure or the fbc structure itself is confusing and unnecessary.
> Set fbc.crtc right when scheduling the enable work so we can always
> use it.
> 
> The problem is not what gets passed and how to retrieve it. The
> problem is that when we're in the other parts of the code we always
> have to keep in mind that if FBC is already enabled we have to get the
> CRTC from place A, if FBC is scheduled we have to get the CRTC from
> place B, and if it's disabled there's no CRTC. Having a single place
> to retrieve the CRTC from allows us to treat the "is enabled" and "is
> scheduled" cases as the same case, reducing the mistake surface. I
> guess I should add this to the commit message.
> 
> Besides the immediate advantages, this is also going to make one of
> the next commits much simpler. And even later, when we introduce
> enable/disable + activate/deactivate, this will be even simpler as
> we'll set the CRTC at enable time. So all the
> activate/deactivate/update code can just look at the single CRTC
> variable regardless of the current state.
> 
> v2: Improve commit message (Chris).
> v3: Rebase after changing the patch order.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h  |  1 -
>  drivers/gpu/drm/i915/intel_fbc.c | 22 +++++++++-------------
>  2 files changed, 9 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 71bd1dc..317414b 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -920,7 +920,6 @@ struct i915_fbc {
>  
>  	struct intel_fbc_work {
>  		struct delayed_work work;
> -		struct intel_crtc *crtc;
>  		struct drm_framebuffer *fb;
>  	} *fbc_work;
>  
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> index 611672f..bc9cd33 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -334,14 +334,13 @@ bool intel_fbc_enabled(struct drm_i915_private *dev_priv)
>  	return dev_priv->fbc.enabled;
>  }
>  
> -static void intel_fbc_enable(struct intel_crtc *crtc,
> -			     const struct drm_framebuffer *fb)
> +static void intel_fbc_enable(const struct drm_framebuffer *fb)
>  {
> -	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
> +	struct drm_i915_private *dev_priv = fb->dev->dev_private;
> +	struct intel_crtc *crtc = dev_priv->fbc.crtc;

This is confusing me. I think of FBC in terms of the CRTC/pipe, and the
fb just describes the plane currently bound to the primary. You've
pushed everywhere else to also work on the CRTC, I think it would be
consistent here to pass crtc and then use fbc.fb_id =
crtc->primary->fb->id.

Have I missed something in the later patches that explains the choice
here?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre