Re: [Intel-gfx] [PATCH 3/3] drm/i915: Mark FBC B gone if pipe B is gone
On Fri, 2022-09-23 at 10:46 +0300, Ville Syrjälä wrote:
> On Fri, Sep 23, 2022 at 09:24:28AM +0300, Luca Coelho wrote:
> > On Thu, 2022-09-22 at 14:57 +0300, Ville Syrjälä wrote:
> > > On Thu, Sep 22, 2022 at 02:37:35PM +0300, Luca Coelho wrote:
> > > > 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ä
> > > > > > 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ä
> > > > > > > > >
> > > > > > > > > If pipe B is fused off we also shouldn't have FBC B.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Ville Syrjälä
> > > > > > > > > ---
> > > > > > > > > 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...
> > >
> > > Hmm. I don't see any real problem with adding the FBC C+D
> > > enum values even if not used by any platform currently.
> > > Do you want to write that patch?
> >
> > Sure, I can do it. I guess it should be done _after_ your patch? Or
> > should I just add those definitions and you'll rebase your patch? And
> > there's a third option: I can add the definitions and replace your
> > patch with one that does this for all PIPEs at once...
> >
> > Which one do you prefer?
>
> I'm fine with just dropping my patch and you taking over the
> the wholew thing. Less stuff for me to do ;)
>
Okay, I sent it out: https://patchwork.freedesktop.org/patch/504761/
--
Cheers,
Luca.
Re: [Intel-gfx] [PATCH 3/3] drm/i915: Mark FBC B gone if pipe B is gone
On Fri, Sep 23, 2022 at 09:24:28AM +0300, Luca Coelho wrote:
> On Thu, 2022-09-22 at 14:57 +0300, Ville Syrjälä wrote:
> > On Thu, Sep 22, 2022 at 02:37:35PM +0300, Luca Coelho wrote:
> > > 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ä
> > > > > 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ä
> > > > > > > >
> > > > > > > > If pipe B is fused off we also shouldn't have FBC B.
> > > > > > > >
> > > > > > > > Signed-off-by: Ville Syrjälä
> > > > > > > > ---
> > > > > > > > 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...
> >
> > Hmm. I don't see any real problem with adding the FBC C+D
> > enum values even if not used by any platform currently.
> > Do you want to write that patch?
>
> Sure, I can do it. I guess it should be done _after_ your patch? Or
> should I just add those definitions and you'll rebase your patch? And
> there's a third option: I can add the definitions and replace your
> patch with one that does this for all PIPEs at once...
>
> Which one do you prefer?
I'm fine with just dropping my patch and you taking over the
the wholew thing. Less stuff for me to do ;)
--
Ville Syrjälä
Intel
Re: [Intel-gfx] [PATCH 3/3] drm/i915: Mark FBC B gone if pipe B is gone
On Thu, 2022-09-22 at 14:57 +0300, Ville Syrjälä wrote:
> On Thu, Sep 22, 2022 at 02:37:35PM +0300, Luca Coelho wrote:
> > 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ä
> > > > 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ä
> > > > > > >
> > > > > > > If pipe B is fused off we also shouldn't have FBC B.
> > > > > > >
> > > > > > > Signed-off-by: Ville Syrjälä
> > > > > > > ---
> > > > > > > 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...
>
> Hmm. I don't see any real problem with adding the FBC C+D
> enum values even if not used by any platform currently.
> Do you want to write that patch?
Sure, I can do it. I guess it should be done _after_ your patch? Or
should I just add those definitions and you'll rebase your patch? And
there's a third option: I can add the definitions and replace your
patch with one that does this for all PIPEs at once...
Which one do you prefer?
--
Cheers,
Luca.
Re: [Intel-gfx] [PATCH 3/3] drm/i915: Mark FBC B gone if pipe B is gone
On Thu, Sep 22, 2022 at 02:37:35PM +0300, Luca Coelho wrote:
> 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ä 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ä
> > > > > >
> > > > > > If pipe B is fused off we also shouldn't have FBC B.
> > > > > >
> > > > > > Signed-off-by: Ville Syrjälä
> > > > > > ---
> > > > > > 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...
Hmm. I don't see any real problem with adding the FBC C+D
enum values even if not used by any platform currently.
Do you want to write that patch?
--
Ville Syrjälä
Intel
Re: [Intel-gfx] [PATCH 3/3] drm/i915: Mark FBC B gone if pipe B is gone
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ä 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ä
> > > > >
> > > > > If pipe B is fused off we also shouldn't have FBC B.
> > > > >
> > > > > Signed-off-by: Ville Syrjälä
> > > > > ---
> > > > > 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.
Re: [Intel-gfx] [PATCH 3/3] drm/i915: Mark FBC B gone if pipe B is gone
On Thu, 22 Sep 2022, Ville Syrjälä wrote:
> On Thu, Sep 22, 2022 at 12:36:46PM +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ä 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ä
>> > >> >
>> > >> > If pipe B is fused off we also shouldn't have FBC B.
>> > >> >
>> > >> > Signed-off-by: Ville Syrjälä
>> > >> > ---
>> > >> > 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.
>
> Well, I guess I already did that partially in eg. skl_fbc_id_for_pipe()
> though that one does allow for a difference in bias at least.
Regardless, scratch what I just said, I don't like it either. For
whatever reason I momentarily thought fbc_mask was indexed using
pipes. *facepalm*.
Sorry for the noise.
BR,
Jani.
>
>> 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.
>>
>> >
>> > after all the fuse handling. Would also fix any misconfiguration in
>> > i915_pci.c.
>> >
>> >
>> > BR,
>> > Jani.
>> >
>> >
>> > --
>> > Jani Nikula, Intel Open Source Graphics Center
>>
>> --
>> Ville Syrjälä
>> Intel
--
Jani Nikula, Intel Open Source Graphics Center
Re: [Intel-gfx] [PATCH 3/3] drm/i915: Mark FBC B gone if pipe B is gone
On Thu, 2022-09-22 at 11:29 +0300, Ville Syrjälä 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ä
> > >
> > > If pipe B is fused off we also shouldn't have FBC B.
> > >
> > > Signed-off-by: Ville Syrjälä
> > > ---
> > > 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.
>
Okay, but AFAICT INTEL_FBC_B was defined but not really used directly
anywhere. So wouldn't it make sense to add this for pipes C and D as
well? Then, if you ever just need to check if the pipe supports FBC,
you could just check that bit, without making any other assumptions.
--
Cheers,
Luca.
Re: [Intel-gfx] [PATCH 3/3] drm/i915: Mark FBC B gone if pipe B is gone
On Thu, Sep 22, 2022 at 12:36:46PM +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ä 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ä
> > >> >
> > >> > If pipe B is fused off we also shouldn't have FBC B.
> > >> >
> > >> > Signed-off-by: Ville Syrjälä
> > >> > ---
> > >> > 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.
Well, I guess I already did that partially in eg. skl_fbc_id_for_pipe()
though that one does allow for a difference in bias at least.
> 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.
>
> >
> > after all the fuse handling. Would also fix any misconfiguration in
> > i915_pci.c.
> >
> >
> > BR,
> > Jani.
> >
> >
> > --
> > Jani Nikula, Intel Open Source Graphics Center
>
> --
> Ville Syrjälä
> Intel
--
Ville Syrjälä
Intel
Re: [Intel-gfx] [PATCH 3/3] drm/i915: Mark FBC B gone if pipe B is gone
On Thu, Sep 22, 2022 at 11:51:16AM +0300, Jani Nikula wrote:
> On Thu, 22 Sep 2022, Ville Syrjälä 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ä
> >> >
> >> > If pipe B is fused off we also shouldn't have FBC B.
> >> >
> >> > Signed-off-by: Ville Syrjälä
> >> > ---
> >> > 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.
>
> after all the fuse handling. Would also fix any misconfiguration in
> i915_pci.c.
>
>
> BR,
> Jani.
>
>
> --
> Jani Nikula, Intel Open Source Graphics Center
--
Ville Syrjälä
Intel
Re: [Intel-gfx] [PATCH 3/3] drm/i915: Mark FBC B gone if pipe B is gone
On Thu, 22 Sep 2022, Ville Syrjälä 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ä
>> >
>> > If pipe B is fused off we also shouldn't have FBC B.
>> >
>> > Signed-off-by: Ville Syrjälä
>> > ---
>> > 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;
after all the fuse handling. Would also fix any misconfiguration in
i915_pci.c.
BR,
Jani.
--
Jani Nikula, Intel Open Source Graphics Center
Re: [Intel-gfx] [PATCH 3/3] drm/i915: Mark FBC B gone if pipe B is gone
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ä
> >
> > If pipe B is fused off we also shouldn't have FBC B.
> >
> > Signed-off-by: Ville Syrjälä
> > ---
> > 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.
--
Ville Syrjälä
Intel
Re: [Intel-gfx] [PATCH 3/3] drm/i915: Mark FBC B gone if pipe B is gone
On Fri, 2022-09-16 at 19:52 +0300, Ville Syrjala wrote:
> From: Ville Syrjälä
>
> If pipe B is fused off we also shouldn't have FBC B.
>
> Signed-off-by: Ville Syrjälä
> ---
> 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.
But what about PIPE_C and PIPE_D? Wouldn't it make sense to do the same
thing for them as well?
--
Cheers,
Luca.
[Intel-gfx] [PATCH 3/3] drm/i915: Mark FBC B gone if pipe B is gone
From: Ville Syrjälä
If pipe B is fused off we also shouldn't have FBC B.
Signed-off-by: Ville Syrjälä
---
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);
--
2.35.1
