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

["Zanoni, Paulo R" <paulo.r.zanoni@intel.com>]

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.

Anyway, it's worth mentioning that later on this series when we
introduce enable() and disable() we already set dev_priv->fbc.crtc at
enable(), so all the activate/deactivate/update code can just look at
that regardless of the current state. So this patch is just an
intermediate step to keep everything simple and working until the
enable/disable patch.

> -Chris
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx