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