Re: [Intel-gfx] [PATCH] drm/i915/mtl: Copy c10 phy pll sw state from master to slave for bigjoiner

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

On Fri, Apr 21, 2023 at 03:43:41PM +0300, Lisovskiy, Stanislav wrote:
> On Fri, Apr 21, 2023 at 03:19:42PM +0300, Ville Syrjälä wrote:
> > On Fri, Apr 21, 2023 at 02:37:11PM +0300, Lisovskiy, Stanislav wrote:
> > > On Fri, Apr 21, 2023 at 12:26:08PM +0300, Ville Syrjälä wrote:
> > > > On Fri, Apr 21, 2023 at 11:35:20AM +0300, Stanislav Lisovskiy wrote:
> > > > > We try to verify pll registers in sw state for slave crtc with the hw state.
> > > > > However in case of bigjoiner we don't calculate those at all, so this verification
> > > > > will then always fail.
> > > > > So we should either skip the verification for Bigjoiner slave crtc or copy sw state
> > > > > from master crtc.
> > > > > 
> > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/display/intel_display.c | 1 +
> > > > >  1 file changed, 1 insertion(+)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > > > > index bf391a6cd8d6..83c98791fea3 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > > > @@ -4556,6 +4556,7 @@ copy_bigjoiner_crtc_state_modeset(struct intel_atomic_state *state,
> > > > >  	drm_mode_copy(&slave_crtc_state->hw.adjusted_mode,
> > > > >  		      &master_crtc_state->hw.adjusted_mode);
> > > > >  	slave_crtc_state->hw.scaling_filter = master_crtc_state->hw.scaling_filter;
> > > > > +	slave_crtc_state->cx0pll_state = master_crtc_state->cx0pll_state;
> > > > 
> > > > Wrong place. Also we're already copying dpll_hw_state which is in the
> > > > same union, and on first blush looks bigger than this thing. So why is
> > > > that not working?
> > > 
> > > No we aren't copying it, we are "saving" it earlier, however it doesn't help at all:
> > > 
> > > /* 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;
> > 
> > Hmm. How is anything at all working if we keep this around
> > from the old state?
> > 
> > Looks like I probably broke this in
> > commit 0ff0e219d9b8 ("drm/i915: Compute clocks earlier")
> > and somehow no one has noticed.
> > 
> > The correct fix would seem to be to just nuke that
> > dpll_hw_state preservation above.
> 
> Need to ask for this machine, where this is reproducible and
> check if that helps..

We really need to get joiner tested by regular ci. With adl+
that shouldn't even require any special hw due to the uncompressed 
joiner. For older hw we need a DSC capable display for bigjoiner
(or faking the DSC support and ignoring the fact that you won't
actually see anything on the screen).

-- 
Ville Syrjälä
Intel