| Message ID | 20230628141017.18937-1-stanislav.lisovskiy@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Don't preserve dpll_hw_state for slave crtc in Bigjoiner | expand |
On Wed, 28 Jun 2023, Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> wrote: > If we are using Bigjoiner dpll_hw_state is supposed to be exactly > same as for master crtc, so no need to save it's state for slave crtc. Could this help with [1]? BR, Jani. [1] https://gitlab.freedesktop.org/drm/intel/-/issues/8720 > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > --- > drivers/gpu/drm/i915/display/intel_display.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > index 43d6ba980780..c3e93bdde29d 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.c > +++ b/drivers/gpu/drm/i915/display/intel_display.c > @@ -4568,7 +4568,6 @@ copy_bigjoiner_crtc_state_modeset(struct intel_atomic_state *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);
On Wed, Jun 28, 2023 at 06:10:15PM +0300, Jani Nikula wrote: > On Wed, 28 Jun 2023, Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> wrote: > > If we are using Bigjoiner dpll_hw_state is supposed to be exactly > > same as for master crtc, so no need to save it's state for slave crtc. > > Could this help with [1]? > > BR, > Jani. > > [1] https://gitlab.freedesktop.org/drm/intel/-/issues/8720 I don't think that would help, here this is mostly a cosmetic fix to remove dmesg warn caused by slave crtc sw/hw state mismatch, because we don't calculate pll state for slave crtc in Bigjoiner, but still try to verify it against the values in the correspodent hw. Stan > > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_display.c | 1 - > > 1 file changed, 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > > index 43d6ba980780..c3e93bdde29d 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > @@ -4568,7 +4568,6 @@ copy_bigjoiner_crtc_state_modeset(struct intel_atomic_state *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); > > -- > Jani Nikula, Intel Open Source Graphics Center
On Thu, 29 Jun 2023, "Lisovskiy, Stanislav" <stanislav.lisovskiy@intel.com> wrote: > On Wed, Jun 28, 2023 at 06:10:15PM +0300, Jani Nikula wrote: >> On Wed, 28 Jun 2023, Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> wrote: >> > If we are using Bigjoiner dpll_hw_state is supposed to be exactly >> > same as for master crtc, so no need to save it's state for slave crtc. >> >> Could this help with [1]? >> >> BR, >> Jani. >> >> [1] https://gitlab.freedesktop.org/drm/intel/-/issues/8720 > > I don't think that would help, here this is mostly a cosmetic fix > to remove dmesg warn caused by slave crtc sw/hw state mismatch, because > we don't calculate pll state for slave crtc in Bigjoiner, but still try > to verify it against the values in the correspodent hw. I think that's one part of what's going in the bug, but I guess just for a different phy. BR, Jani. > > Stan > >> >> > >> > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> >> > --- >> > drivers/gpu/drm/i915/display/intel_display.c | 1 - >> > 1 file changed, 1 deletion(-) >> > >> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c >> > index 43d6ba980780..c3e93bdde29d 100644 >> > --- a/drivers/gpu/drm/i915/display/intel_display.c >> > +++ b/drivers/gpu/drm/i915/display/intel_display.c >> > @@ -4568,7 +4568,6 @@ copy_bigjoiner_crtc_state_modeset(struct intel_atomic_state *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); >> >> -- >> Jani Nikula, Intel Open Source Graphics Center
On Wed, Jun 28, 2023 at 05:10:17PM +0300, Stanislav Lisovskiy wrote: > If we are using Bigjoiner dpll_hw_state is supposed to be exactly > same as for master crtc, so no need to save it's state for slave crtc. Yeah, and the master has recalculated this already. I guess this used to make some sense in the times when we did the dpll calculations much later. So probably deserves a Fixes: 0ff0e219d9b8 ("drm/i915: Compute clocks earlier") Anyways, Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > --- > drivers/gpu/drm/i915/display/intel_display.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > index 43d6ba980780..c3e93bdde29d 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.c > +++ b/drivers/gpu/drm/i915/display/intel_display.c > @@ -4568,7 +4568,6 @@ copy_bigjoiner_crtc_state_modeset(struct intel_atomic_state *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; Seems like we should also remove it from the intel_crtc_prepare_cleared_state() but that one clearly needs much more thought due to the port_dpll[] stuff... > saved_state->crc_enabled = slave_crtc_state->crc_enabled; > > intel_crtc_free_hw_state(slave_crtc_state); > -- > 2.37.3
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c index 43d6ba980780..c3e93bdde29d 100644 --- a/drivers/gpu/drm/i915/display/intel_display.c +++ b/drivers/gpu/drm/i915/display/intel_display.c @@ -4568,7 +4568,6 @@ copy_bigjoiner_crtc_state_modeset(struct intel_atomic_state *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);
If we are using Bigjoiner dpll_hw_state is supposed to be exactly same as for master crtc, so no need to save it's state for slave crtc. Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> --- drivers/gpu/drm/i915/display/intel_display.c | 1 - 1 file changed, 1 deletion(-)