Re: [Intel-gfx] [PATCH 02/11] drm/i915: Make the CRTC wrt. CSC state consistent during sanitize-disabling

[Imre Deak <imre.deak@intel.com>]

On Wed, Apr 26, 2023 at 08:48:57PM +0300, Ville Syrjälä wrote:
> On Wed, Apr 26, 2023 at 07:52:56PM +0300, Imre Deak wrote:
> > intel_crtc_free_hw_state() frees all the Intel specific CSC blobs in the
> > CRTC state, but the next memset() will only clear the corresponding
> > pointers for the ones stored in intel_crtc_state:hw. Clear the ones
> > stored in intel_crtc_state as well. Also sync the UAPI state with the HW
> > state after the HW state was reset. This will reset the uapi.active
> > flag as well, so no need to do this separately. Syncing the state will
> > create a new umode blob, so move deleting the blob after the sync call.
> > 
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_modeset_setup.c | 12 +++++++++---
> >  1 file changed, 9 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_modeset_setup.c b/drivers/gpu/drm/i915/display/intel_modeset_setup.c
> > index eefa4018dc0c2..57d087de654f8 100644
> > --- a/drivers/gpu/drm/i915/display/intel_modeset_setup.c
> > +++ b/drivers/gpu/drm/i915/display/intel_modeset_setup.c
> > @@ -30,6 +30,8 @@
> >  #include "intel_wm.h"
> >  #include "skl_watermark.h"
> >  
> > +static void intel_crtc_copy_hw_to_uapi_state(struct intel_crtc_state *crtc_state);
> > +
> >  static void intel_crtc_disable_noatomic(struct intel_crtc *crtc,
> >  					struct drm_modeset_acquire_ctx *ctx)
> >  {
> > @@ -88,13 +90,17 @@ static void intel_crtc_disable_noatomic(struct intel_crtc *crtc,
> >  	crtc->active = false;
> >  	crtc->base.enabled = false;
> >  
> > -	drm_WARN_ON(&i915->drm,
> > -		    drm_atomic_set_mode_for_crtc(&crtc_state->uapi, NULL) < 0);
> > -	crtc_state->uapi.active = false;
> >  	crtc_state->uapi.connector_mask = 0;
> >  	crtc_state->uapi.encoder_mask = 0;
> > +
> >  	intel_crtc_free_hw_state(crtc_state);
> >  	memset(&crtc_state->hw, 0, sizeof(crtc_state->hw));
> > +	crtc_state->pre_csc_lut = NULL;
> > +	crtc_state->post_csc_lut = NULL;
> > +	intel_crtc_copy_hw_to_uapi_state(crtc_state);
> > +
> > +	drm_WARN_ON(&i915->drm,
> > +		    drm_atomic_set_mode_for_crtc(&crtc_state->uapi, NULL) < 0);
> 
> Hmm. Is there a reason we can't just do the full destroy+reset
> thing that intel_modeset_readout_hw_state() does?

Yes, that's better and works. It revealed that the (global) shared_dpll
state needs to be also updated here.

I'd like to use this function to disable bigjoiner configs as well
(later in this patchset), where the slave CRTC state is used during the
master CRTC disabling (in the encoder post_disable hook); so is it ok to
first disable all related CRTCs and then reset the state for all (also
moving the fbc disabling and watermark updating before the state
update)?

Both of the above changes are at
https://github.com/ideak/linux/commit/fc9b011249112369
on top of this patchset.

Also is it ok to keep the full CRTC reset change as a separate patch
(for the purpose of stable backporting the rest).

> 
> >  
> >  	for_each_encoder_on_crtc(&i915->drm, &crtc->base, encoder)
> >  		encoder->base.crtc = NULL;
> > -- 
> > 2.37.2
> 
> -- 
> Ville Syrjälä
> Intel