drm/i915/dsc: Remove redundant checks in DSC disable

Commit Message

Kulkarni, Vandita June 3, 2021, 6:53 a.m. UTC

There can be a chance that pre os has enabled
DSC and driver's compute config would not need
dsc to be enabled, in such case if we check on
compute config's compression state to disable,
we might end up in state mismatch.

Signed-off-by: Vandita Kulkarni <[e-mail]>
---
 drivers/gpu/drm/i915/display/intel_vdsc.c | 4 ----
 1 file changed, 4 deletions(-)

Comments

Saarinen, Jani June 3, 2021, 7:37 a.m. UTC | #1

Hi, 
> -----Original Message-----
> From: Intel-gfx <[e-mail]> On Behalf Of Vandita
> Kulkarni
> Sent: torstai 3. kesäkuuta 2021 9.54
> To: [e-mail]
> Cc: Nikula, Jani <[e-mail]>
> Subject: [Intel-gfx] [PATCH] drm/i915/dsc: Remove redundant checks in DSC disable
> 
> There can be a chance that pre os has enabled DSC and driver's compute config
> would not need dsc to be enabled, in such case if we check on compute config's
> compression state to disable, we might end up in state mismatch.

I assume this fixes real gitlab issue too? 

> 
> Signed-off-by: Vandita Kulkarni <[e-mail]>
> ---
>  drivers/gpu/drm/i915/display/intel_vdsc.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_vdsc.c
> b/drivers/gpu/drm/i915/display/intel_vdsc.c
> index 19cd9531c115..b05a96011d93 100644
> --- a/drivers/gpu/drm/i915/display/intel_vdsc.c
> +++ b/drivers/gpu/drm/i915/display/intel_vdsc.c
> @@ -1161,10 +1161,6 @@ void intel_dsc_disable(const struct intel_crtc_state
> *old_crtc_state)
>  	struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->uapi.crtc);
>  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> 
> -	if (!(old_crtc_state->dsc.compression_enable &&
> -	      old_crtc_state->bigjoiner))
> -		return;
> -
>  	intel_de_write(dev_priv, dss_ctl1_reg(old_crtc_state), 0);
>  	intel_de_write(dev_priv, dss_ctl2_reg(old_crtc_state), 0);  }
> --
> 2.21.0.5.gaeb582a
> 
> _______________________________________________
> Intel-gfx mailing list
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Kulkarni, Vandita June 3, 2021, 8:47 a.m. UTC | #2

> -----Original Message-----
> From: Saarinen, Jani <[e-mail]>
> Sent: Thursday, June 3, 2021 1:07 PM
> To: Kulkarni, Vandita <[e-mail]>; intel-
> [e-mail]
> Cc: Nikula, Jani <[e-mail]>
> Subject: RE: [Intel-gfx] [PATCH] drm/i915/dsc: Remove redundant checks in
> DSC disable
> 
> Hi,
> > -----Original Message-----
> > From: Intel-gfx <[e-mail]> On Behalf Of
> > Vandita Kulkarni
> > Sent: torstai 3. kesäkuuta 2021 9.54
> > To: [e-mail]
> > Cc: Nikula, Jani <[e-mail]>
> > Subject: [Intel-gfx] [PATCH] drm/i915/dsc: Remove redundant checks in
> > DSC disable
> >
> > There can be a chance that pre os has enabled DSC and driver's compute
> > config would not need dsc to be enabled, in such case if we check on
> > compute config's compression state to disable, we might end up in state
> mismatch.
> 
> I assume this fixes real gitlab issue too?
Okay, will add the tag 
Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/3537

Thanks,
Vandita
> 
> >
> > Signed-off-by: Vandita Kulkarni <[e-mail]>
> > ---
> >  drivers/gpu/drm/i915/display/intel_vdsc.c | 4 ----
> >  1 file changed, 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_vdsc.c
> > b/drivers/gpu/drm/i915/display/intel_vdsc.c
> > index 19cd9531c115..b05a96011d93 100644
> > --- a/drivers/gpu/drm/i915/display/intel_vdsc.c
> > +++ b/drivers/gpu/drm/i915/display/intel_vdsc.c
> > @@ -1161,10 +1161,6 @@ void intel_dsc_disable(const struct
> > intel_crtc_state
> > *old_crtc_state)
> >  	struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->uapi.crtc);
> >  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> >
> > -	if (!(old_crtc_state->dsc.compression_enable &&
> > -	      old_crtc_state->bigjoiner))
> > -		return;
> > -
> >  	intel_de_write(dev_priv, dss_ctl1_reg(old_crtc_state), 0);
> >  	intel_de_write(dev_priv, dss_ctl2_reg(old_crtc_state), 0);  }
> > --
> > 2.21.0.5.gaeb582a
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > [e-mail]
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Jani Nikula June 3, 2021, 9:41 a.m. UTC | #3

On Thu, 03 Jun 2021, "Kulkarni, Vandita" <[e-mail]> wrote:
>> -----Original Message-----
>> From: Saarinen, Jani <[e-mail]>
>> Sent: Thursday, June 3, 2021 1:07 PM
>> To: Kulkarni, Vandita <[e-mail]>; intel-
>> [e-mail]
>> Cc: Nikula, Jani <[e-mail]>
>> Subject: RE: [Intel-gfx] [PATCH] drm/i915/dsc: Remove redundant checks in
>> DSC disable
>> 
>> Hi,
>> > -----Original Message-----
>> > From: Intel-gfx <[e-mail]> On Behalf Of
>> > Vandita Kulkarni
>> > Sent: torstai 3. kesäkuuta 2021 9.54
>> > To: [e-mail]
>> > Cc: Nikula, Jani <[e-mail]>
>> > Subject: [Intel-gfx] [PATCH] drm/i915/dsc: Remove redundant checks in
>> > DSC disable
>> >
>> > There can be a chance that pre os has enabled DSC and driver's compute
>> > config would not need dsc to be enabled, in such case if we check on
>> > compute config's compression state to disable, we might end up in state
>> mismatch.
>> 
>> I assume this fixes real gitlab issue too?
> Okay, will add the tag 
> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/3537

See https://lore.kernel.org/r/[e-mail]

The problem is with ->bigjoiner, not the entire statement.


BR,
Jani.

>
> Thanks,
> Vandita
>> 
>> >
>> > Signed-off-by: Vandita Kulkarni <[e-mail]>
>> > ---
>> >  drivers/gpu/drm/i915/display/intel_vdsc.c | 4 ----
>> >  1 file changed, 4 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/display/intel_vdsc.c
>> > b/drivers/gpu/drm/i915/display/intel_vdsc.c
>> > index 19cd9531c115..b05a96011d93 100644
>> > --- a/drivers/gpu/drm/i915/display/intel_vdsc.c
>> > +++ b/drivers/gpu/drm/i915/display/intel_vdsc.c
>> > @@ -1161,10 +1161,6 @@ void intel_dsc_disable(const struct
>> > intel_crtc_state
>> > *old_crtc_state)
>> >  	struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->uapi.crtc);
>> >  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>> >
>> > -	if (!(old_crtc_state->dsc.compression_enable &&
>> > -	      old_crtc_state->bigjoiner))
>> > -		return;
>> > -
>> >  	intel_de_write(dev_priv, dss_ctl1_reg(old_crtc_state), 0);
>> >  	intel_de_write(dev_priv, dss_ctl2_reg(old_crtc_state), 0);  }
>> > --
>> > 2.21.0.5.gaeb582a
>> >
>> > _______________________________________________
>> > Intel-gfx mailing list
>> > [e-mail]
>> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Kulkarni, Vandita June 3, 2021, 11:24 a.m. UTC | #4

> -----Original Message-----
> From: Nikula, Jani <[e-mail]>
> Sent: Thursday, June 3, 2021 3:11 PM
> To: Kulkarni, Vandita <[e-mail]>; Saarinen, Jani
> <[e-mail]>; [e-mail]
> Cc: Manna, Animesh <[e-mail]>; Navare, Manasi D
> <[e-mail]>
> Subject: RE: [Intel-gfx] [PATCH] drm/i915/dsc: Remove redundant checks in
> DSC disable
> 
> On Thu, 03 Jun 2021, "Kulkarni, Vandita" <[e-mail]>
> wrote:
> >> -----Original Message-----
> >> From: Saarinen, Jani <[e-mail]>
> >> Sent: Thursday, June 3, 2021 1:07 PM
> >> To: Kulkarni, Vandita <[e-mail]>; intel-
> >> [e-mail]
> >> Cc: Nikula, Jani <[e-mail]>
> >> Subject: RE: [Intel-gfx] [PATCH] drm/i915/dsc: Remove redundant
> >> checks in DSC disable
> >>
> >> Hi,
> >> > -----Original Message-----
> >> > From: Intel-gfx <[e-mail]> On Behalf
> >> > Of Vandita Kulkarni
> >> > Sent: torstai 3. kesäkuuta 2021 9.54
> >> > To: [e-mail]
> >> > Cc: Nikula, Jani <[e-mail]>
> >> > Subject: [Intel-gfx] [PATCH] drm/i915/dsc: Remove redundant checks
> >> > in DSC disable
> >> >
> >> > There can be a chance that pre os has enabled DSC and driver's
> >> > compute config would not need dsc to be enabled, in such case if we
> >> > check on compute config's compression state to disable, we might
> >> > end up in state
> >> mismatch.
> >>
> >> I assume this fixes real gitlab issue too?
> > Okay, will add the tag
> > Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/3537
> 
> See https://lore.kernel.org/r/[e-mail]
> 
> The problem is with ->bigjoiner, not the entire statement.
Thanks for pointing this out, true that bigjoiner not being enabled will stop dsc disabling.
The bigjoiner check was making the entire condition check unnecessary.

Will update and refloat.

Thanks,
Vandita
> 
> 
> BR,
> Jani.
> 
> >
> > Thanks,
> > Vandita
> >>
> >> >
> >> > Signed-off-by: Vandita Kulkarni <[e-mail]>
> >> > ---
> >> >  drivers/gpu/drm/i915/display/intel_vdsc.c | 4 ----
> >> >  1 file changed, 4 deletions(-)
> >> >
> >> > diff --git a/drivers/gpu/drm/i915/display/intel_vdsc.c
> >> > b/drivers/gpu/drm/i915/display/intel_vdsc.c
> >> > index 19cd9531c115..b05a96011d93 100644
> >> > --- a/drivers/gpu/drm/i915/display/intel_vdsc.c
> >> > +++ b/drivers/gpu/drm/i915/display/intel_vdsc.c
> >> > @@ -1161,10 +1161,6 @@ void intel_dsc_disable(const struct
> >> > intel_crtc_state
> >> > *old_crtc_state)
> >> >  	struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->uapi.crtc);
> >> >  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> >> >
> >> > -	if (!(old_crtc_state->dsc.compression_enable &&
> >> > -	      old_crtc_state->bigjoiner))
> >> > -		return;
> >> > -
> >> >  	intel_de_write(dev_priv, dss_ctl1_reg(old_crtc_state), 0);
> >> >  	intel_de_write(dev_priv, dss_ctl2_reg(old_crtc_state), 0);  }
> >> > --
> >> > 2.21.0.5.gaeb582a
> >> >
> >> > _______________________________________________
> >> > Intel-gfx mailing list
> >> > [e-mail]
> >> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> --
> Jani Nikula, Intel Open Source Graphics Center

Manna, Animesh June 3, 2021, 1:53 p.m. UTC | #5

> -----Original Message-----
> From: Kulkarni, Vandita <[e-mail]>
> Sent: Thursday, June 3, 2021 4:55 PM
> To: Nikula, Jani <[e-mail]>; Saarinen, Jani
> <[e-mail]>; [e-mail]
> Cc: Manna, Animesh <[e-mail]>; Navare, Manasi D
> <[e-mail]>
> Subject: RE: [Intel-gfx] [PATCH] drm/i915/dsc: Remove redundant checks in DSC
> disable
> 
> > -----Original Message-----
> > From: Nikula, Jani <[e-mail]>
> > Sent: Thursday, June 3, 2021 3:11 PM
> > To: Kulkarni, Vandita <[e-mail]>; Saarinen, Jani
> > <[e-mail]>; [e-mail]
> > Cc: Manna, Animesh <[e-mail]>; Navare, Manasi D
> > <[e-mail]>
> > Subject: RE: [Intel-gfx] [PATCH] drm/i915/dsc: Remove redundant checks
> > in DSC disable
> >
> > On Thu, 03 Jun 2021, "Kulkarni, Vandita" <[e-mail]>
> > wrote:
> > >> -----Original Message-----
> > >> From: Saarinen, Jani <[e-mail]>
> > >> Sent: Thursday, June 3, 2021 1:07 PM
> > >> To: Kulkarni, Vandita <[e-mail]>; intel-
> > >> [e-mail]
> > >> Cc: Nikula, Jani <[e-mail]>
> > >> Subject: RE: [Intel-gfx] [PATCH] drm/i915/dsc: Remove redundant
> > >> checks in DSC disable
> > >>
> > >> Hi,
> > >> > -----Original Message-----
> > >> > From: Intel-gfx <[e-mail]> On
> > >> > Behalf Of Vandita Kulkarni
> > >> > Sent: torstai 3. kesäkuuta 2021 9.54
> > >> > To: [e-mail]
> > >> > Cc: Nikula, Jani <[e-mail]>
> > >> > Subject: [Intel-gfx] [PATCH] drm/i915/dsc: Remove redundant
> > >> > checks in DSC disable
> > >> >
> > >> > There can be a chance that pre os has enabled DSC and driver's
> > >> > compute config would not need dsc to be enabled, in such case if
> > >> > we check on compute config's compression state to disable, we
> > >> > might end up in state
> > >> mismatch.
> > >>
> > >> I assume this fixes real gitlab issue too?
> > > Okay, will add the tag
> > > Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/3537
> >
> > See https://lore.kernel.org/r/[e-mail]
> >
> > The problem is with ->bigjoiner, not the entire statement.
> Thanks for pointing this out, true that bigjoiner not being enabled will stop dsc
> disabling.
> The bigjoiner check was making the entire condition check unnecessary.
> 
> Will update and refloat.

Hi Jani/Vandita,

For uncompressed bigjoiner case if we want to use the same function to clear the dsc_ctrl1 register we may need to remove both the condition check.
As for uncompressed bigjoiner case, compression_enable Will be 0 and will block in clearing the dss_ctl1_reg.

Regards,
Animesh
> 
> Thanks,
> Vandita
> >
> >
> > BR,
> > Jani.
> >
> > >
> > > Thanks,
> > > Vandita
> > >>
> > >> >
> > >> > Signed-off-by: Vandita Kulkarni <[e-mail]>
> > >> > ---
> > >> >  drivers/gpu/drm/i915/display/intel_vdsc.c | 4 ----
> > >> >  1 file changed, 4 deletions(-)
> > >> >
> > >> > diff --git a/drivers/gpu/drm/i915/display/intel_vdsc.c
> > >> > b/drivers/gpu/drm/i915/display/intel_vdsc.c
> > >> > index 19cd9531c115..b05a96011d93 100644
> > >> > --- a/drivers/gpu/drm/i915/display/intel_vdsc.c
> > >> > +++ b/drivers/gpu/drm/i915/display/intel_vdsc.c
> > >> > @@ -1161,10 +1161,6 @@ void intel_dsc_disable(const struct
> > >> > intel_crtc_state
> > >> > *old_crtc_state)
> > >> >  	struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->uapi.crtc);
> > >> >  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > >> >
> > >> > -	if (!(old_crtc_state->dsc.compression_enable &&
> > >> > -	      old_crtc_state->bigjoiner))
> > >> > -		return;
> > >> > -
> > >> >  	intel_de_write(dev_priv, dss_ctl1_reg(old_crtc_state), 0);
> > >> >  	intel_de_write(dev_priv, dss_ctl2_reg(old_crtc_state), 0);  }
> > >> > --
> > >> > 2.21.0.5.gaeb582a
> > >> >
> > >> > _______________________________________________
> > >> > Intel-gfx mailing list
> > >> > [e-mail]
> > >> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> > --
> > Jani Nikula, Intel Open Source Graphics Center

Kulkarni, Vandita June 3, 2021, 3:37 p.m. UTC | #6

> -----Original Message-----
> From: Manna, Animesh <[e-mail]>
> Sent: Thursday, June 3, 2021 7:24 PM
> To: Kulkarni, Vandita <[e-mail]>; Nikula, Jani
> <[e-mail]>; Saarinen, Jani <[e-mail]>; intel-
> [e-mail]
> Cc: Navare, Manasi D <[e-mail]>
> Subject: RE: [Intel-gfx] [PATCH] drm/i915/dsc: Remove redundant checks in
> DSC disable
> 
> 
> 
> > -----Original Message-----
> > From: Kulkarni, Vandita <[e-mail]>
> > Sent: Thursday, June 3, 2021 4:55 PM
> > To: Nikula, Jani <[e-mail]>; Saarinen, Jani
> > <[e-mail]>; [e-mail]
> > Cc: Manna, Animesh <[e-mail]>; Navare, Manasi D
> > <[e-mail]>
> > Subject: RE: [Intel-gfx] [PATCH] drm/i915/dsc: Remove redundant checks
> > in DSC disable
> >
> > > -----Original Message-----
> > > From: Nikula, Jani <[e-mail]>
> > > Sent: Thursday, June 3, 2021 3:11 PM
> > > To: Kulkarni, Vandita <[e-mail]>; Saarinen, Jani
> > > <[e-mail]>; [e-mail]
> > > Cc: Manna, Animesh <[e-mail]>; Navare, Manasi D
> > > <[e-mail]>
> > > Subject: RE: [Intel-gfx] [PATCH] drm/i915/dsc: Remove redundant
> > > checks in DSC disable
> > >
> > > On Thu, 03 Jun 2021, "Kulkarni, Vandita"
> > > <[e-mail]>
> > > wrote:
> > > >> -----Original Message-----
> > > >> From: Saarinen, Jani <[e-mail]>
> > > >> Sent: Thursday, June 3, 2021 1:07 PM
> > > >> To: Kulkarni, Vandita <[e-mail]>; intel-
> > > >> [e-mail]
> > > >> Cc: Nikula, Jani <[e-mail]>
> > > >> Subject: RE: [Intel-gfx] [PATCH] drm/i915/dsc: Remove redundant
> > > >> checks in DSC disable
> > > >>
> > > >> Hi,
> > > >> > -----Original Message-----
> > > >> > From: Intel-gfx <[e-mail]> On
> > > >> > Behalf Of Vandita Kulkarni
> > > >> > Sent: torstai 3. kesäkuuta 2021 9.54
> > > >> > To: [e-mail]
> > > >> > Cc: Nikula, Jani <[e-mail]>
> > > >> > Subject: [Intel-gfx] [PATCH] drm/i915/dsc: Remove redundant
> > > >> > checks in DSC disable
> > > >> >
> > > >> > There can be a chance that pre os has enabled DSC and driver's
> > > >> > compute config would not need dsc to be enabled, in such case
> > > >> > if we check on compute config's compression state to disable,
> > > >> > we might end up in state
> > > >> mismatch.
> > > >>
> > > >> I assume this fixes real gitlab issue too?
> > > > Okay, will add the tag
> > > > Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/3537
> > >
> > > See https://lore.kernel.org/r/[e-mail]
> > >
> > > The problem is with ->bigjoiner, not the entire statement.
> > Thanks for pointing this out, true that bigjoiner not being enabled
> > will stop dsc disabling.
> > The bigjoiner check was making the entire condition check unnecessary.
> >
> > Will update and refloat.
> 
> Hi Jani/Vandita,
> 
> For uncompressed bigjoiner case if we want to use the same function to
> clear the dsc_ctrl1 register we may need to remove both the condition
> check.
> As for uncompressed bigjoiner case, compression_enable Will be 0 and will
> block in clearing the dss_ctl1_reg.

Yes, I was going through and found that bit 20 and 21 of dss_ctl1 are being used
for uncompressed joiner.
So when dsc is not enabled to avoid writing the register we could add
below code .

if (dsc)
	clear dss_ctl2
if ( bigjoiner | dsc)
	clear dss_ctl1;
return;

bigjoiner = 1 and dsc = 0  - uncompressed , I think there is no harm in clearing dsc bits again
bigjoiner = 1 and dsc = 1 - compressed - uncompressed bits are already 0
bigjoiner = 0 and dsc= 1 - just dsc  - clear dsc rest are 0s already
bigjoiner = 0 and dsc = 0  do nothing, return

If I have missed any corner case, please let me know.

Thanks,
Vandita
> 
> Regards,
> Animesh
> >
> > Thanks,
> > Vandita
> > >
> > >
> > > BR,
> > > Jani.
> > >
> > > >
> > > > Thanks,
> > > > Vandita
> > > >>
> > > >> >
> > > >> > Signed-off-by: Vandita Kulkarni <[e-mail]>
> > > >> > ---
> > > >> >  drivers/gpu/drm/i915/display/intel_vdsc.c | 4 ----
> > > >> >  1 file changed, 4 deletions(-)
> > > >> >
> > > >> > diff --git a/drivers/gpu/drm/i915/display/intel_vdsc.c
> > > >> > b/drivers/gpu/drm/i915/display/intel_vdsc.c
> > > >> > index 19cd9531c115..b05a96011d93 100644
> > > >> > --- a/drivers/gpu/drm/i915/display/intel_vdsc.c
> > > >> > +++ b/drivers/gpu/drm/i915/display/intel_vdsc.c
> > > >> > @@ -1161,10 +1161,6 @@ void intel_dsc_disable(const struct
> > > >> > intel_crtc_state
> > > >> > *old_crtc_state)
> > > >> >  	struct intel_crtc *crtc = to_intel_crtc(old_crtc_state-
> >uapi.crtc);
> > > >> >  	struct drm_i915_private *dev_priv = to_i915(crtc-
> >base.dev);
> > > >> >
> > > >> > -	if (!(old_crtc_state->dsc.compression_enable &&
> > > >> > -	      old_crtc_state->bigjoiner))
> > > >> > -		return;
> > > >> > -
> > > >> >  	intel_de_write(dev_priv, dss_ctl1_reg(old_crtc_state), 0);
> > > >> >  	intel_de_write(dev_priv, dss_ctl2_reg(old_crtc_state), 0);  }
> > > >> > --
> > > >> > 2.21.0.5.gaeb582a
> > > >> >
> > > >> > _______________________________________________
> > > >> > Intel-gfx mailing list
> > > >> > [e-mail]
> > > >> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > >
> > > --
> > > Jani Nikula, Intel Open Source Graphics Center

Navare, Manasi June 3, 2021, 6:45 p.m. UTC | #7

On Thu, Jun 03, 2021 at 08:37:22AM -0700, Kulkarni, Vandita wrote:
> > -----Original Message-----
> > From: Manna, Animesh <[e-mail]>
> > Sent: Thursday, June 3, 2021 7:24 PM
> > To: Kulkarni, Vandita <[e-mail]>; Nikula, Jani
> > <[e-mail]>; Saarinen, Jani <[e-mail]>; intel-
> > [e-mail]
> > Cc: Navare, Manasi D <[e-mail]>
> > Subject: RE: [Intel-gfx] [PATCH] drm/i915/dsc: Remove redundant checks in
> > DSC disable
> > 
> > 
> > 
> > > -----Original Message-----
> > > From: Kulkarni, Vandita <[e-mail]>
> > > Sent: Thursday, June 3, 2021 4:55 PM
> > > To: Nikula, Jani <[e-mail]>; Saarinen, Jani
> > > <[e-mail]>; [e-mail]
> > > Cc: Manna, Animesh <[e-mail]>; Navare, Manasi D
> > > <[e-mail]>
> > > Subject: RE: [Intel-gfx] [PATCH] drm/i915/dsc: Remove redundant checks
> > > in DSC disable
> > >
> > > > -----Original Message-----
> > > > From: Nikula, Jani <[e-mail]>
> > > > Sent: Thursday, June 3, 2021 3:11 PM
> > > > To: Kulkarni, Vandita <[e-mail]>; Saarinen, Jani
> > > > <[e-mail]>; [e-mail]
> > > > Cc: Manna, Animesh <[e-mail]>; Navare, Manasi D
> > > > <[e-mail]>
> > > > Subject: RE: [Intel-gfx] [PATCH] drm/i915/dsc: Remove redundant
> > > > checks in DSC disable
> > > >
> > > > On Thu, 03 Jun 2021, "Kulkarni, Vandita"
> > > > <[e-mail]>
> > > > wrote:
> > > > >> -----Original Message-----
> > > > >> From: Saarinen, Jani <[e-mail]>
> > > > >> Sent: Thursday, June 3, 2021 1:07 PM
> > > > >> To: Kulkarni, Vandita <[e-mail]>; intel-
> > > > >> [e-mail]
> > > > >> Cc: Nikula, Jani <[e-mail]>
> > > > >> Subject: RE: [Intel-gfx] [PATCH] drm/i915/dsc: Remove redundant
> > > > >> checks in DSC disable
> > > > >>
> > > > >> Hi,
> > > > >> > -----Original Message-----
> > > > >> > From: Intel-gfx <[e-mail]> On
> > > > >> > Behalf Of Vandita Kulkarni
> > > > >> > Sent: torstai 3. kesäkuuta 2021 9.54
> > > > >> > To: [e-mail]
> > > > >> > Cc: Nikula, Jani <[e-mail]>
> > > > >> > Subject: [Intel-gfx] [PATCH] drm/i915/dsc: Remove redundant
> > > > >> > checks in DSC disable
> > > > >> >
> > > > >> > There can be a chance that pre os has enabled DSC and driver's
> > > > >> > compute config would not need dsc to be enabled, in such case
> > > > >> > if we check on compute config's compression state to disable,
> > > > >> > we might end up in state
> > > > >> mismatch.
> > > > >>
> > > > >> I assume this fixes real gitlab issue too?
> > > > > Okay, will add the tag
> > > > > Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/3537
> > > >
> > > > See https://lore.kernel.org/r/[e-mail]
> > > >
> > > > The problem is with ->bigjoiner, not the entire statement.
> > > Thanks for pointing this out, true that bigjoiner not being enabled
> > > will stop dsc disabling.
> > > The bigjoiner check was making the entire condition check unnecessary.
> > >
> > > Will update and refloat.
> > 
> > Hi Jani/Vandita,
> > 
> > For uncompressed bigjoiner case if we want to use the same function to
> > clear the dsc_ctrl1 register we may need to remove both the condition
> > check.
> > As for uncompressed bigjoiner case, compression_enable Will be 0 and will
> > block in clearing the dss_ctl1_reg.
> 
> Yes, I was going through and found that bit 20 and 21 of dss_ctl1 are being used
> for uncompressed joiner.
> So when dsc is not enabled to avoid writing the register we could add
> below code .
> 
> if (dsc)
> 	clear dss_ctl2
> if ( bigjoiner | dsc)
> 	clear dss_ctl1;
> return;
> 
> bigjoiner = 1 and dsc = 0  - uncompressed , I think there is no harm in clearing dsc bits again
> bigjoiner = 1 and dsc = 1 - compressed - uncompressed bits are already 0
> bigjoiner = 0 and dsc= 1 - just dsc  - clear dsc rest are 0s already
> bigjoiner = 0 and dsc = 0  do nothing, return
> 
> If I have missed any corner case, please let me know.
> 
> Thanks,
> Vandita

I think in the original code the condition was just reversed, instead it should be  :

if !(dsc_en || bigjoiner_en) {
	write 0 to dss ctl 1
	write 0 to dss ctl 2
}

So here basically it meets all the conditions you mentioned Vandita:

- only when both dsc and bigjoiner are 0, it will do nothing
- In all other cases DSC + Bigjoiner : Clear all bits including uncompressed bits which shd be 0 already
- In dsc = 0, bigjoiner = 1 (uncompressed), it will clear both again which is okay since dsc bits are already 0

Does this make sense?

Regards
Manasi


> > 
> > Regards,
> > Animesh
> > >
> > > Thanks,
> > > Vandita
> > > >
> > > >
> > > > BR,
> > > > Jani.
> > > >
> > > > >
> > > > > Thanks,
> > > > > Vandita
> > > > >>
> > > > >> >
> > > > >> > Signed-off-by: Vandita Kulkarni <[e-mail]>
> > > > >> > ---
> > > > >> >  drivers/gpu/drm/i915/display/intel_vdsc.c | 4 ----
> > > > >> >  1 file changed, 4 deletions(-)
> > > > >> >
> > > > >> > diff --git a/drivers/gpu/drm/i915/display/intel_vdsc.c
> > > > >> > b/drivers/gpu/drm/i915/display/intel_vdsc.c
> > > > >> > index 19cd9531c115..b05a96011d93 100644
> > > > >> > --- a/drivers/gpu/drm/i915/display/intel_vdsc.c
> > > > >> > +++ b/drivers/gpu/drm/i915/display/intel_vdsc.c
> > > > >> > @@ -1161,10 +1161,6 @@ void intel_dsc_disable(const struct
> > > > >> > intel_crtc_state
> > > > >> > *old_crtc_state)
> > > > >> >  	struct intel_crtc *crtc = to_intel_crtc(old_crtc_state-
> > >uapi.crtc);
> > > > >> >  	struct drm_i915_private *dev_priv = to_i915(crtc-
> > >base.dev);
> > > > >> >
> > > > >> > -	if (!(old_crtc_state->dsc.compression_enable &&
> > > > >> > -	      old_crtc_state->bigjoiner))
> > > > >> > -		return;
> > > > >> > -
> > > > >> >  	intel_de_write(dev_priv, dss_ctl1_reg(old_crtc_state), 0);
> > > > >> >  	intel_de_write(dev_priv, dss_ctl2_reg(old_crtc_state), 0);  }
> > > > >> > --
> > > > >> > 2.21.0.5.gaeb582a
> > > > >> >
> > > > >> > _______________________________________________
> > > > >> > Intel-gfx mailing list
> > > > >> > [e-mail]
> > > > >> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > >
> > > > --
> > > > Jani Nikula, Intel Open Source Graphics Center

Kulkarni, Vandita June 4, 2021, 7:31 a.m. UTC | #8

> -----Original Message-----
> From: Navare, Manasi D <[e-mail]>
> Sent: Friday, June 4, 2021 12:16 AM
> To: Kulkarni, Vandita <[e-mail]>
> Cc: Manna, Animesh <[e-mail]>; Nikula, Jani
> <[e-mail]>; Saarinen, Jani <[e-mail]>; intel-
> [e-mail]
> Subject: Re: [Intel-gfx] [PATCH] drm/i915/dsc: Remove redundant checks in
> DSC disable
> 
> On Thu, Jun 03, 2021 at 08:37:22AM -0700, Kulkarni, Vandita wrote:
> > > -----Original Message-----
> > > From: Manna, Animesh <[e-mail]>
> > > Sent: Thursday, June 3, 2021 7:24 PM
> > > To: Kulkarni, Vandita <[e-mail]>; Nikula, Jani
> > > <[e-mail]>; Saarinen, Jani <[e-mail]>;
> > > intel- [e-mail]
> > > Cc: Navare, Manasi D <[e-mail]>
> > > Subject: RE: [Intel-gfx] [PATCH] drm/i915/dsc: Remove redundant
> > > checks in DSC disable
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Kulkarni, Vandita <[e-mail]>
> > > > Sent: Thursday, June 3, 2021 4:55 PM
> > > > To: Nikula, Jani <[e-mail]>; Saarinen, Jani
> > > > <[e-mail]>; [e-mail]
> > > > Cc: Manna, Animesh <[e-mail]>; Navare, Manasi D
> > > > <[e-mail]>
> > > > Subject: RE: [Intel-gfx] [PATCH] drm/i915/dsc: Remove redundant
> > > > checks in DSC disable
> > > >
> > > > > -----Original Message-----
> > > > > From: Nikula, Jani <[e-mail]>
> > > > > Sent: Thursday, June 3, 2021 3:11 PM
> > > > > To: Kulkarni, Vandita <[e-mail]>; Saarinen,
> > > > > Jani <[e-mail]>; [e-mail]
> > > > > Cc: Manna, Animesh <[e-mail]>; Navare, Manasi D
> > > > > <[e-mail]>
> > > > > Subject: RE: [Intel-gfx] [PATCH] drm/i915/dsc: Remove redundant
> > > > > checks in DSC disable
> > > > >
> > > > > On Thu, 03 Jun 2021, "Kulkarni, Vandita"
> > > > > <[e-mail]>
> > > > > wrote:
> > > > > >> -----Original Message-----
> > > > > >> From: Saarinen, Jani <[e-mail]>
> > > > > >> Sent: Thursday, June 3, 2021 1:07 PM
> > > > > >> To: Kulkarni, Vandita <[e-mail]>; intel-
> > > > > >> [e-mail]
> > > > > >> Cc: Nikula, Jani <[e-mail]>
> > > > > >> Subject: RE: [Intel-gfx] [PATCH] drm/i915/dsc: Remove
> > > > > >> redundant checks in DSC disable
> > > > > >>
> > > > > >> Hi,
> > > > > >> > -----Original Message-----
> > > > > >> > From: Intel-gfx <[e-mail]>
> > > > > >> > On Behalf Of Vandita Kulkarni
> > > > > >> > Sent: torstai 3. kesäkuuta 2021 9.54
> > > > > >> > To: [e-mail]
> > > > > >> > Cc: Nikula, Jani <[e-mail]>
> > > > > >> > Subject: [Intel-gfx] [PATCH] drm/i915/dsc: Remove redundant
> > > > > >> > checks in DSC disable
> > > > > >> >
> > > > > >> > There can be a chance that pre os has enabled DSC and
> > > > > >> > driver's compute config would not need dsc to be enabled,
> > > > > >> > in such case if we check on compute config's compression
> > > > > >> > state to disable, we might end up in state
> > > > > >> mismatch.
> > > > > >>
> > > > > >> I assume this fixes real gitlab issue too?
> > > > > > Okay, will add the tag
> > > > > > Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/3537
> > > > >
> > > > > See https://lore.kernel.org/r/[e-mail]
> > > > >
> > > > > The problem is with ->bigjoiner, not the entire statement.
> > > > Thanks for pointing this out, true that bigjoiner not being
> > > > enabled will stop dsc disabling.
> > > > The bigjoiner check was making the entire condition check
> unnecessary.
> > > >
> > > > Will update and refloat.
> > >
> > > Hi Jani/Vandita,
> > >
> > > For uncompressed bigjoiner case if we want to use the same function
> > > to clear the dsc_ctrl1 register we may need to remove both the
> > > condition check.
> > > As for uncompressed bigjoiner case, compression_enable Will be 0 and
> > > will block in clearing the dss_ctl1_reg.
> >
> > Yes, I was going through and found that bit 20 and 21 of dss_ctl1 are
> > being used for uncompressed joiner.
> > So when dsc is not enabled to avoid writing the register we could add
> > below code .
> >
> > if (dsc)
> > 	clear dss_ctl2
> > if ( bigjoiner | dsc)
> > 	clear dss_ctl1;
> > return;
> >
> > bigjoiner = 1 and dsc = 0  - uncompressed , I think there is no harm
> > in clearing dsc bits again bigjoiner = 1 and dsc = 1 - compressed -
> > uncompressed bits are already 0 bigjoiner = 0 and dsc= 1 - just dsc  -
> > clear dsc rest are 0s already bigjoiner = 0 and dsc = 0  do nothing,
> > return
> >
> > If I have missed any corner case, please let me know.
> >
> > Thanks,
> > Vandita
> 
> I think in the original code the condition was just reversed, instead it should
> be  :
> 
> if !(dsc_en || bigjoiner_en) {
> 	write 0 to dss ctl 1
> 	write 0 to dss ctl 2
> }
It should be the other way.
If neither of them are enabled then we have to just return.

Since I see that dss_ctl2 also gets set in case of big joiner I shall move that under
the main check of bigjoiner OR dsc and clear both the regs.

Thanks,
Vandita
> 
> So here basically it meets all the conditions you mentioned Vandita:
> 
> - only when both dsc and bigjoiner are 0, it will do nothing
> - In all other cases DSC + Bigjoiner : Clear all bits including uncompressed bits
> which shd be 0 already
> - In dsc = 0, bigjoiner = 1 (uncompressed), it will clear both again which is okay
> since dsc bits are already 0
> 
> Does this make sense?
> 
> Regards
> Manasi
> 
> 
> > >
> > > Regards,
> > > Animesh
> > > >
> > > > Thanks,
> > > > Vandita
> > > > >
> > > > >
> > > > > BR,
> > > > > Jani.
> > > > >
> > > > > >
> > > > > > Thanks,
> > > > > > Vandita
> > > > > >>
> > > > > >> >
> > > > > >> > Signed-off-by: Vandita Kulkarni
> > > > > >> > <[e-mail]>
> > > > > >> > ---
> > > > > >> >  drivers/gpu/drm/i915/display/intel_vdsc.c | 4 ----
> > > > > >> >  1 file changed, 4 deletions(-)
> > > > > >> >
> > > > > >> > diff --git a/drivers/gpu/drm/i915/display/intel_vdsc.c
> > > > > >> > b/drivers/gpu/drm/i915/display/intel_vdsc.c
> > > > > >> > index 19cd9531c115..b05a96011d93 100644
> > > > > >> > --- a/drivers/gpu/drm/i915/display/intel_vdsc.c
> > > > > >> > +++ b/drivers/gpu/drm/i915/display/intel_vdsc.c
> > > > > >> > @@ -1161,10 +1161,6 @@ void intel_dsc_disable(const struct
> > > > > >> > intel_crtc_state
> > > > > >> > *old_crtc_state)
> > > > > >> >  	struct intel_crtc *crtc = to_intel_crtc(old_crtc_state-
> > > >uapi.crtc);
> > > > > >> >  	struct drm_i915_private *dev_priv = to_i915(crtc-
> > > >base.dev);
> > > > > >> >
> > > > > >> > -	if (!(old_crtc_state->dsc.compression_enable &&
> > > > > >> > -	      old_crtc_state->bigjoiner))
> > > > > >> > -		return;
> > > > > >> > -
> > > > > >> >  	intel_de_write(dev_priv, dss_ctl1_reg(old_crtc_state), 0);
> > > > > >> >  	intel_de_write(dev_priv, dss_ctl2_reg(old_crtc_state),
> > > > > >> > 0);  }
> > > > > >> > --
> > > > > >> > 2.21.0.5.gaeb582a
> > > > > >> >
> > > > > >> > _______________________________________________
> > > > > >> > Intel-gfx mailing list
> > > > > >> > [e-mail]
> > > > > >> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > > >
> > > > > --
> > > > > Jani Nikula, Intel Open Source Graphics Center

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_vdsc.c b/drivers/gpu/drm/i915/display/intel_vdsc.c
index 19cd9531c115..b05a96011d93 100644
--- a/drivers/gpu/drm/i915/display/intel_vdsc.c
+++ b/drivers/gpu/drm/i915/display/intel_vdsc.c
@@ -1161,10 +1161,6 @@  void intel_dsc_disable(const struct intel_crtc_state *old_crtc_state)
 	struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->uapi.crtc);
 	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
 
-	if (!(old_crtc_state->dsc.compression_enable &&
-	      old_crtc_state->bigjoiner))
-		return;
-
 	intel_de_write(dev_priv, dss_ctl1_reg(old_crtc_state), 0);
 	intel_de_write(dev_priv, dss_ctl2_reg(old_crtc_state), 0);
 }