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

[chris at chris-wilson.co.uk]

On Wed, Oct 21, 2015 at 05:27:32PM +0000, Zanoni, Paulo R wrote:
> Em Ter, 2015-10-20 às 17:03 +0100, Chris Wilson escreveu:
> > On Tue, Oct 20, 2015 at 11:49:51AM -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.
> > 
> > Pardon? It was confusing to have the crtc passed along with the work
> > item as opposed to digging it out from an indirect link on the
> > dev_priv.
> > iirc work->crtc was part of the serialisation of the work item.
> > 
> > What I think you meant was that you were passing around the wrong
> > struct, e.g.
> > intel_fbc_enable() just wants the crtc, in intel_fbc_flip_prepare()
> > you
> > check for the work struct, and then you should just use the work
> > struct.
> 
> 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.

Sure that's also my argument why I don't like this particular patch as
is. :) The functions here had just one place to retreive everything they
needed, and now they need two.

I guess the way to make us both happy is to embed the work_struct inside
the fbc struct - and that way you can eliminate the divergence.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre