[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 07:13:40PM -0200, Paulo Zanoni wrote:
> 2015-11-13 18:56 GMT-02:00 Chris Wilson <chris at chris-wilson.co.uk>:
> > 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?
> 
> You are right that this is confusing. This is basically an artifact
> from the previous FBC design that I overlooked. I guess during the
> conversion, my thinking was "well, the arguments are the stuff we
> store in the work struct, and since we don't store the CRTC anymore,
> let's remove it and keep passing the FB since it's still part of the
> work struct". Now that you bring this under a different perspective, I
> see the problem. We don't even need to pass these things as arguments
> since we do check that work->fb is equal to crtc->fb.
> 
> I just hope you let me address this with follow-up patches. Function
> intel_fbc_activate() was created because there were 2 callers: one for
> the case there the work_fn happens, another for the case where we
> failed to allocate the work_fn. Later in the series there's only one
> caller because there's no allocation anymore, so the best way to fix
> is probably to just move those 3 lines to the place of the only
> caller.
> 
> You also requested me to grab references of fbc.fb_id, and I have some
> evil plans to completely kill it, which would even reduce those 3
> lines further.

If you have it in hand, the rest seems fine, so
Reviewed-by: Chris Wilson <chris at chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre