Re: [Intel-gfx] [PATCH v2 04/10] drm/i915: Clean up the bigjoiner state copy logic

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

On Fri, Feb 04, 2022 at 12:52:01PM -0800, Navare, Manasi wrote:
> On Fri, Feb 04, 2022 at 09:20:49AM +0200, Ville Syrjala wrote:
<snip>
> >  static int
> > -copy_bigjoiner_crtc_state(struct intel_crtc_state *crtc_state,
> > -			  const struct intel_crtc_state *from_crtc_state)
> > +copy_bigjoiner_crtc_state_modeset(struct intel_atomic_state *state,
> > +				  struct intel_crtc *slave_crtc)
> >  {
> > +	struct intel_crtc_state *slave_crtc_state =
> > +		intel_atomic_get_new_crtc_state(state, slave_crtc);
> > +	struct intel_crtc *master_crtc = intel_master_crtc(slave_crtc_state);
> > +	const struct intel_crtc_state *master_crtc_state =
> > +		intel_atomic_get_new_crtc_state(state, master_crtc);
> >  	struct intel_crtc_state *saved_state;
> >  
> > -	saved_state = kmemdup(from_crtc_state, sizeof(*saved_state), GFP_KERNEL);
> > +	saved_state = kmemdup(master_crtc_state, sizeof(*saved_state), GFP_KERNEL);
> >  	if (!saved_state)
> >  		return -ENOMEM;
> >  
> > -	saved_state->uapi = crtc_state->uapi;
> > -	saved_state->scaler_state = crtc_state->scaler_state;
> > -	saved_state->shared_dpll = crtc_state->shared_dpll;
> > -	saved_state->dpll_hw_state = crtc_state->dpll_hw_state;
> > -	saved_state->crc_enabled = crtc_state->crc_enabled;
> > +	/* preserve some things from the slave's original crtc state */
> > +	saved_state->uapi = slave_crtc_state->uapi;
> > +	saved_state->scaler_state = slave_crtc_state->scaler_state;
> > +	saved_state->shared_dpll = slave_crtc_state->shared_dpll;
> > +	saved_state->dpll_hw_state = slave_crtc_state->dpll_hw_state;
> > +	saved_state->crc_enabled = slave_crtc_state->crc_enabled;
> 
> Slave crtc state here not set at all , so why do we preserve the things from slave's original crtc state and how we
> decide on what all to preserve?

It's the same junk as in intel_crtc_prepare_cleared_state(). There's
a comment there IIRC that explains some of the historical baggage here.
We should fix this mess, but it would require some actual thought.

-- 
Ville Syrjälä
Intel