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
