Re: [Intel-gfx] [PATCH 02/10] drm/i915: Fix bigjoiner state copy fails

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Feb 03, 2022 at 02:13:41PM -0800, Navare, Manasi wrote:
> On Thu, Feb 03, 2022 at 08:38:15PM +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > We seem to be missing a few things from the bigjoiner state copy.
> > Namely hw.mode isn't getting copied (which probably causes PIPESRC
> > to be misconfigured), CTM/LUTs aren't getting copied (which could
> > cause the pipe to produced incorrect output), and we also forgot
> > to copy over the color_mgmt_changed flag so potentially we fail
> > to do the actual CTM/LUT programming (assuming we aren't doing
> > a full modeset or fastset). Fix it all.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c | 14 +++++++++++++-
> >  1 file changed, 13 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index 85dce8a093d4..2006eec6e166 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -5827,8 +5827,10 @@ intel_crtc_copy_uapi_to_hw_state_nomodeset(struct intel_atomic_state *state,
> >  	master_crtc_state = intel_atomic_get_new_crtc_state(state, master_crtc);
> >  
> >  	/* No need to copy state if the master state is unchanged */
> > -	if (master_crtc_state)
> > +	if (master_crtc_state) {
> > +		crtc_state->uapi.color_mgmt_changed = master_crtc_state->uapi.color_mgmt_changed;
> 
> Since in this function we are copying from uapi_to_hw_state, is this the right function to copy to uapi state ?

These *_changed flags aren't really state. They're just
ephemeral properties of the atomic transaction to indicate what
has changed since last time. I've occasionally pondered about
moving all them to  live as bitmasks in the top level 
drm_atomic_state instead to make that more clear, but haven't
actually gotten bored enough to do it. Also there are other things
hanging off the various state objects that arent really part of the
state proper either.

> 
> 
> >  		intel_crtc_copy_color_blobs(crtc_state, master_crtc_state);
> > +	}
> >  }
> >  
> >  static void
> > @@ -5890,13 +5892,23 @@ copy_bigjoiner_crtc_state(struct intel_crtc_state *crtc_state,
> >  	memset(&crtc_state->hw, 0, sizeof(saved_state->hw));
> >  	crtc_state->hw.enable = from_crtc_state->hw.enable;
> >  	crtc_state->hw.active = from_crtc_state->hw.active;
> > +	crtc_state->hw.mode = from_crtc_state->hw.mode;
> >  	crtc_state->hw.pipe_mode = from_crtc_state->hw.pipe_mode;
> >  	crtc_state->hw.adjusted_mode = from_crtc_state->hw.adjusted_mode;
> > +	crtc_state->hw.scaling_filter = from_crtc_state->hw.scaling_filter;
> > +
> > +	drm_property_replace_blob(&crtc_state->hw.degamma_lut,
> > +				  from_crtc_state->hw.degamma_lut);
> > +	drm_property_replace_blob(&crtc_state->hw.gamma_lut,
> > +				  from_crtc_state->hw.gamma_lut);
> > +	drm_property_replace_blob(&crtc_state->uapi.ctm,

Just noticed a copy pasta fail here...

> > +				  from_crtc_state->hw.ctm);
> >  
> >  	/* Some fixups */
> >  	crtc_state->uapi.mode_changed = from_crtc_state->uapi.mode_changed;
> >  	crtc_state->uapi.connectors_changed = from_crtc_state->uapi.connectors_changed;
> >  	crtc_state->uapi.active_changed = from_crtc_state->uapi.active_changed;
> > +	crtc_state->uapi.color_mgmt_changed = from_crtc_state->uapi.color_mgmt_changed;
> >  	crtc_state->nv12_planes = crtc_state->c8_planes = crtc_state->update_planes = 0;
> >  	crtc_state->bigjoiner_linked_crtc = to_intel_crtc(from_crtc_state->uapi.crtc);
> >  	crtc_state->bigjoiner_slave = true;
> 
> This looks good
> 
> Manasi
> 
> > -- 
> > 2.34.1
> > 

-- 
Ville Syrjälä
Intel