Re: [Intel-gfx] [PATCH 3/3] drm/i915: Mark FBC B gone if pipe B is gone

[Luca Coelho <luca@coelho.fi>]

On Thu, 2022-09-22 at 12:36 +0300, Ville Syrjälä wrote:
> On Thu, Sep 22, 2022 at 11:51:16AM +0300, Jani Nikula wrote:
> > On Thu, 22 Sep 2022, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > > On Thu, Sep 22, 2022 at 11:18:55AM +0300, Luca Coelho wrote:
> > > > On Fri, 2022-09-16 at 19:52 +0300, Ville Syrjala wrote:
> > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > 
> > > > > If pipe B is fused off we also shouldn't have FBC B.
> > > > > 
> > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/intel_device_info.c | 1 +
> > > > >  1 file changed, 1 insertion(+)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c
> > > > > index 1434dc33cf49..fbefebc023f1 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_device_info.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_device_info.c
> > > > > @@ -394,6 +394,7 @@ void intel_device_info_runtime_init(struct drm_i915_private *dev_priv)
> > > > >  		if (dfsm & SKL_DFSM_PIPE_B_DISABLE) {
> > > > >  			runtime->pipe_mask &= ~BIT(PIPE_B);
> > > > >  			runtime->cpu_transcoder_mask &= ~BIT(TRANSCODER_B);
> > > > > +			runtime->fbc_mask &= ~BIT(INTEL_FBC_B);
> > > > >  		}
> > > > >  		if (dfsm & SKL_DFSM_PIPE_C_DISABLE) {
> > > > >  			runtime->pipe_mask &= ~BIT(PIPE_C);
> > > > 
> > > > I don't know (yet) what exactly this does, but it makes sense if you
> > > > think of consistency: we already do that for PIPE_A.
> > > 
> > > It's basically saying the entire pipe is fused off, so anything
> > > living inside that pipe should also be fused off.
> > > 
> > > > 
> > > > But what about PIPE_C and PIPE_D? Wouldn't it make sense to do the same
> > > > thing for them as well?
> > > 
> > > There is no FBC engine on those pipes (we don't even have
> > > the INTEL_FBC_C+ enum values defined), at least for now.
> > 
> > A future proof way would be to add
> > 
> > 	runtime->fbc_mask &= runtime->pipe_mask;
> 
> Dunno if I entirely like the extra assumption that the enums match.
> Also would need to make sure we don't accidentally screw up any
> old platforms where FBC is not tied to a specific pipe, but I
> guess we should never have pipe A fused off on those w/o
> the entire display engine fused off as well.

I must say I don't like the idea of making these assumptions across
different masks either.

I think that, since you are reading the DFSM register at runtime to
check whether those pipes are fused off, you should go all the way and
disable everything, including in the fbc_mask for all pipes.  Then you
don't need to make any assumptions about whether a pipe has FBC or not.

In short, I think you could add those INTEL_FBC_C+ definitions and
force-unset them here too...

But that's just my 2c.

--
Cheers,
Luca.