Re: [Intel-gfx] [PATCH] drm/i915/dsc: Remove redundant checks in DSC disable

["Kulkarni, Vandita" <vandita.kulkarni@intel.com>]

> -----Original Message-----
> From: Navare, Manasi D <manasi.d.navare@intel.com>
> Sent: Friday, June 4, 2021 12:16 AM
> To: Kulkarni, Vandita <vandita.kulkarni@intel.com>
> Cc: Manna, Animesh <animesh.manna@intel.com>; Nikula, Jani
> <jani.nikula@intel.com>; Saarinen, Jani <jani.saarinen@intel.com>; intel-
> gfx@lists.freedesktop.org
> 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 <animesh.manna@intel.com>
> > > Sent: Thursday, June 3, 2021 7:24 PM
> > > To: Kulkarni, Vandita <vandita.kulkarni@intel.com>; Nikula, Jani
> > > <jani.nikula@intel.com>; Saarinen, Jani <jani.saarinen@intel.com>;
> > > intel- gfx@lists.freedesktop.org
> > > Cc: Navare, Manasi D <manasi.d.navare@intel.com>
> > > Subject: RE: [Intel-gfx] [PATCH] drm/i915/dsc: Remove redundant
> > > checks in DSC disable
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Kulkarni, Vandita <vandita.kulkarni@intel.com>
> > > > Sent: Thursday, June 3, 2021 4:55 PM
> > > > To: Nikula, Jani <jani.nikula@intel.com>; Saarinen, Jani
> > > > <jani.saarinen@intel.com>; intel-gfx@lists.freedesktop.org
> > > > Cc: Manna, Animesh <animesh.manna@intel.com>; Navare, Manasi D
> > > > <manasi.d.navare@intel.com>
> > > > Subject: RE: [Intel-gfx] [PATCH] drm/i915/dsc: Remove redundant
> > > > checks in DSC disable
> > > >
> > > > > -----Original Message-----
> > > > > From: Nikula, Jani <jani.nikula@intel.com>
> > > > > Sent: Thursday, June 3, 2021 3:11 PM
> > > > > To: Kulkarni, Vandita <vandita.kulkarni@intel.com>; Saarinen,
> > > > > Jani <jani.saarinen@intel.com>; intel-gfx@lists.freedesktop.org
> > > > > Cc: Manna, Animesh <animesh.manna@intel.com>; Navare, Manasi D
> > > > > <manasi.d.navare@intel.com>
> > > > > Subject: RE: [Intel-gfx] [PATCH] drm/i915/dsc: Remove redundant
> > > > > checks in DSC disable
> > > > >
> > > > > On Thu, 03 Jun 2021, "Kulkarni, Vandita"
> > > > > <vandita.kulkarni@intel.com>
> > > > > wrote:
> > > > > >> -----Original Message-----
> > > > > >> From: Saarinen, Jani <jani.saarinen@intel.com>
> > > > > >> Sent: Thursday, June 3, 2021 1:07 PM
> > > > > >> To: Kulkarni, Vandita <vandita.kulkarni@intel.com>; intel-
> > > > > >> gfx@lists.freedesktop.org
> > > > > >> Cc: Nikula, Jani <jani.nikula@intel.com>
> > > > > >> Subject: RE: [Intel-gfx] [PATCH] drm/i915/dsc: Remove
> > > > > >> redundant checks in DSC disable
> > > > > >>
> > > > > >> Hi,
> > > > > >> > -----Original Message-----
> > > > > >> > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org>
> > > > > >> > On Behalf Of Vandita Kulkarni
> > > > > >> > Sent: torstai 3. kesäkuuta 2021 9.54
> > > > > >> > To: intel-gfx@lists.freedesktop.org
> > > > > >> > Cc: Nikula, Jani <jani.nikula@intel.com>
> > > > > >> > 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/87fsxzp9qx.fsf@intel.com
> > > > >
> > > > > 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
> > > > > >> > <vandita.kulkarni@intel.com>
> > > > > >> > ---
> > > > > >> >  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
> > > > > >> > Intel-gfx@lists.freedesktop.org
> > > > > >> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > > >
> > > > > --
> > > > > Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx