| Message ID | 20230421083520.14486-1-stanislav.lisovskiy@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/mtl: Copy c10 phy pll sw state from master to slave for bigjoiner | expand |
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? > > copy_bigjoiner_crtc_state_nomodeset(state, slave_crtc); > > -- > 2.37.3
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; saved_state->crc_enabled = slave_crtc_state->crc_enabled; intel_crtc_free_hw_state(slave_crtc_state); memcpy(slave_crtc_state, saved_state, sizeof(*slave_crtc_state)); kfree(saved_state); /* Re-init hw state */ memset(&slave_crtc_state->hw, 0, sizeof(slave_crtc_state->hw)); slave_crtc_state->hw.enable = master_crtc_state->hw.enable; slave_crtc_state->hw.active = master_crtc_state->hw.active; because I guess it just didn't have that stuff initially. It starts to work without those verify WARN's only if I copy it from master_crtc_state. Stan > > > > > copy_bigjoiner_crtc_state_nomodeset(state, slave_crtc); > > > > -- > > 2.37.3 > > -- > Ville Syrjälä > Intel
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. > saved_state->crc_enabled = slave_crtc_state->crc_enabled; > > intel_crtc_free_hw_state(slave_crtc_state); > memcpy(slave_crtc_state, saved_state, sizeof(*slave_crtc_state)); > kfree(saved_state); > > /* Re-init hw state */ > memset(&slave_crtc_state->hw, 0, sizeof(slave_crtc_state->hw)); > slave_crtc_state->hw.enable = master_crtc_state->hw.enable; > slave_crtc_state->hw.active = master_crtc_state->hw.active; > > because I guess it just didn't have that stuff initially. > > It starts to work without those verify WARN's only if I copy it from master_crtc_state. > > Stan > > > > > > > > > copy_bigjoiner_crtc_state_nomodeset(state, slave_crtc); > > > > > > -- > > > 2.37.3 > > > > -- > > Ville Syrjälä > > Intel
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.. Stan > > > > saved_state->crc_enabled = slave_crtc_state->crc_enabled; > > > > intel_crtc_free_hw_state(slave_crtc_state); > > memcpy(slave_crtc_state, saved_state, sizeof(*slave_crtc_state)); > > kfree(saved_state); > > > > /* Re-init hw state */ > > memset(&slave_crtc_state->hw, 0, sizeof(slave_crtc_state->hw)); > > slave_crtc_state->hw.enable = master_crtc_state->hw.enable; > > slave_crtc_state->hw.active = master_crtc_state->hw.active; > > > > because I guess it just didn't have that stuff initially. > > > > It starts to work without those verify WARN's only if I copy it from master_crtc_state. > > > > Stan > > > > > > > > > > > > > copy_bigjoiner_crtc_state_nomodeset(state, slave_crtc); > > > > > > > > -- > > > > 2.37.3 > > > > > > -- > > > Ville Syrjälä > > > Intel > > -- > Ville Syrjälä > Intel
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).
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; copy_bigjoiner_crtc_state_nomodeset(state, slave_crtc);
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(+)