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