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@xxxxxxxxx>
> ---
> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
