| Message ID | 20240206070937.197986-3-mika.kahola@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/display: Force full modeset for eDP | expand |
On Tue, 06 Feb 2024, Mika Kahola <mika.kahola@intel.com> wrote: > Force full modeset for eDP when booting up. GOP programs > PLL parameters and hence, we would be able to use fastset > for eDP. However, with fastset we are not setting PLL values > from the driver and rely that GOP and driver PLL values match. > We have discovered that with some of the panels this is not > true and hence we would need to program PLL values by the > driver. The patch suggests a workaround as enabling full > modeset when booting up. This way we force the driver to > write the PLL values to the hw. No. We want to avoid full modesets if possible, both in general and at probe. And when we do end up with modesets, the decision needs to be based on changes in the state that we can't write to the hardware without a modeset. We can't unconditionally force a modeset on eDP panels at probe. BR, Jani. > > Signed-off-by: Mika Kahola <mika.kahola@intel.com> > --- > drivers/gpu/drm/i915/display/intel_dp.c | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c > index ab415f41924d..9699ded1eb5f 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp.c > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > @@ -3319,6 +3319,7 @@ bool intel_dp_initial_fastset_check(struct intel_encoder *encoder, > * of crtc_state->dsc, we have no way to ensure reliable fastset. > * Remove once we have readout for DSC. > */ > + > if (crtc_state->dsc.compression_enable) { > drm_dbg_kms(&i915->drm, "[ENCODER:%d:%s] Forcing full modeset due to DSC being enabled\n", > encoder->base.base.id, encoder->base.name); > @@ -3326,6 +3327,18 @@ bool intel_dp_initial_fastset_check(struct intel_encoder *encoder, > fastset = false; > } > > + /* > + * FIXME hack to force full modeset for eDP as not always BIOS written PLL > + * values does not match with the ones defined in the driver code > + */ > + if (!crtc_state->uapi.mode_changed && > + intel_dp_is_edp(intel_dp)) { > + drm_dbg_kms(&i915->drm, "Forcing full modeset for eDP\n"); > + crtc_state->uapi.mode_changed = true; > + fastset = false; > + } > + > + > return fastset; > }
> -----Original Message----- > From: Jani Nikula <jani.nikula@linux.intel.com> > Sent: Friday, February 9, 2024 11:06 AM > To: Kahola, Mika <mika.kahola@intel.com>; intel-gfx@lists.freedesktop.org > Cc: Kahola, Mika <mika.kahola@intel.com> > Subject: Re: [PATCH 2/2] drm/i915/display: Force full modeset for eDP > > On Tue, 06 Feb 2024, Mika Kahola <mika.kahola@intel.com> wrote: > > Force full modeset for eDP when booting up. GOP programs PLL > > parameters and hence, we would be able to use fastset for eDP. > > However, with fastset we are not setting PLL values from the driver > > and rely that GOP and driver PLL values match. > > We have discovered that with some of the panels this is not true and > > hence we would need to program PLL values by the driver. The patch > > suggests a workaround as enabling full modeset when booting up. This > > way we force the driver to write the PLL values to the hw. > > No. We want to avoid full modesets if possible, both in general and at probe. > > And when we do end up with modesets, the decision needs to be based on changes in the state that we can't write to the > hardware without a modeset. > > We can't unconditionally force a modeset on eDP panels at probe. Thanks! Just wondering what the options here might be? With fastest we end up having a mismatch with one PLL value with a few panels. Should we try identify the panels and setup some sort of quirks for these problematic panels or what would be the best solution? -Mika- > > > BR, > Jani. > > > > > Signed-off-by: Mika Kahola <mika.kahola@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_dp.c | 13 +++++++++++++ > > 1 file changed, 13 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c > > b/drivers/gpu/drm/i915/display/intel_dp.c > > index ab415f41924d..9699ded1eb5f 100644 > > --- a/drivers/gpu/drm/i915/display/intel_dp.c > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > > @@ -3319,6 +3319,7 @@ bool intel_dp_initial_fastset_check(struct intel_encoder *encoder, > > * of crtc_state->dsc, we have no way to ensure reliable fastset. > > * Remove once we have readout for DSC. > > */ > > + > > if (crtc_state->dsc.compression_enable) { > > drm_dbg_kms(&i915->drm, "[ENCODER:%d:%s] Forcing full modeset due to DSC being enabled\n", > > encoder->base.base.id, encoder->base.name); @@ -3326,6 > > +3327,18 @@ bool intel_dp_initial_fastset_check(struct intel_encoder *encoder, > > fastset = false; > > } > > > > + /* > > + * FIXME hack to force full modeset for eDP as not always BIOS written PLL > > + * values does not match with the ones defined in the driver code > > + */ > > + if (!crtc_state->uapi.mode_changed && > > + intel_dp_is_edp(intel_dp)) { > > + drm_dbg_kms(&i915->drm, "Forcing full modeset for eDP\n"); > > + crtc_state->uapi.mode_changed = true; > > + fastset = false; > > + } > > + > > + > > return fastset; > > } > > -- > Jani Nikula, Intel
On Fri, Feb 09, 2024 at 11:55:58AM +0000, Kahola, Mika wrote: > > -----Original Message----- > > From: Jani Nikula <jani.nikula@linux.intel.com> > > Sent: Friday, February 9, 2024 11:06 AM > > To: Kahola, Mika <mika.kahola@intel.com>; intel-gfx@lists.freedesktop.org > > Cc: Kahola, Mika <mika.kahola@intel.com> > > Subject: Re: [PATCH 2/2] drm/i915/display: Force full modeset for eDP > > > > On Tue, 06 Feb 2024, Mika Kahola <mika.kahola@intel.com> wrote: > > > Force full modeset for eDP when booting up. GOP programs PLL > > > parameters and hence, we would be able to use fastset for eDP. > > > However, with fastset we are not setting PLL values from the driver > > > and rely that GOP and driver PLL values match. > > > We have discovered that with some of the panels this is not true and > > > hence we would need to program PLL values by the driver. The patch > > > suggests a workaround as enabling full modeset when booting up. This > > > way we force the driver to write the PLL values to the hw. > > > > No. We want to avoid full modesets if possible, both in general and at probe. > > > > And when we do end up with modesets, the decision needs to be based on changes in the state that we can't write to the > > hardware without a modeset. > > > > We can't unconditionally force a modeset on eDP panels at probe. > > Thanks! Just wondering what the options here might be? With fastest we end up having a mismatch with one PLL value with a few panels. You seem to be stuck in some infinite loop. If your PLL parameters are mismatching that should prevent the fastset, but then I guess you added some hack to allow the fastset despite the mismatch, and now you're trying to undo that hack by blindly forcing a full modeset? > > Should we try identify the panels and setup some sort of quirks for these problematic panels or what would be the best solution? > > -Mika- > > > > > > > BR, > > Jani. > > > > > > > > Signed-off-by: Mika Kahola <mika.kahola@intel.com> > > > --- > > > drivers/gpu/drm/i915/display/intel_dp.c | 13 +++++++++++++ > > > 1 file changed, 13 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c > > > b/drivers/gpu/drm/i915/display/intel_dp.c > > > index ab415f41924d..9699ded1eb5f 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_dp.c > > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > > > @@ -3319,6 +3319,7 @@ bool intel_dp_initial_fastset_check(struct intel_encoder *encoder, > > > * of crtc_state->dsc, we have no way to ensure reliable fastset. > > > * Remove once we have readout for DSC. > > > */ > > > + > > > if (crtc_state->dsc.compression_enable) { > > > drm_dbg_kms(&i915->drm, "[ENCODER:%d:%s] Forcing full modeset due to DSC being enabled\n", > > > encoder->base.base.id, encoder->base.name); @@ -3326,6 > > > +3327,18 @@ bool intel_dp_initial_fastset_check(struct intel_encoder *encoder, > > > fastset = false; > > > } > > > > > > + /* > > > + * FIXME hack to force full modeset for eDP as not always BIOS written PLL > > > + * values does not match with the ones defined in the driver code > > > + */ > > > + if (!crtc_state->uapi.mode_changed && > > > + intel_dp_is_edp(intel_dp)) { > > > + drm_dbg_kms(&i915->drm, "Forcing full modeset for eDP\n"); > > > + crtc_state->uapi.mode_changed = true; > > > + fastset = false; > > > + } > > > + > > > + > > > return fastset; > > > } > > > > -- > > Jani Nikula, Intel
> -----Original Message----- > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > Sent: Friday, February 9, 2024 2:07 PM > To: Kahola, Mika <mika.kahola@intel.com> > Cc: Jani Nikula <jani.nikula@linux.intel.com>; intel-gfx@lists.freedesktop.org > Subject: Re: [PATCH 2/2] drm/i915/display: Force full modeset for eDP > > On Fri, Feb 09, 2024 at 11:55:58AM +0000, Kahola, Mika wrote: > > > -----Original Message----- > > > From: Jani Nikula <jani.nikula@linux.intel.com> > > > Sent: Friday, February 9, 2024 11:06 AM > > > To: Kahola, Mika <mika.kahola@intel.com>; > > > intel-gfx@lists.freedesktop.org > > > Cc: Kahola, Mika <mika.kahola@intel.com> > > > Subject: Re: [PATCH 2/2] drm/i915/display: Force full modeset for > > > eDP > > > > > > On Tue, 06 Feb 2024, Mika Kahola <mika.kahola@intel.com> wrote: > > > > Force full modeset for eDP when booting up. GOP programs PLL > > > > parameters and hence, we would be able to use fastset for eDP. > > > > However, with fastset we are not setting PLL values from the > > > > driver and rely that GOP and driver PLL values match. > > > > We have discovered that with some of the panels this is not true > > > > and hence we would need to program PLL values by the driver. The > > > > patch suggests a workaround as enabling full modeset when booting > > > > up. This way we force the driver to write the PLL values to the hw. > > > > > > No. We want to avoid full modesets if possible, both in general and at probe. > > > > > > And when we do end up with modesets, the decision needs to be based > > > on changes in the state that we can't write to the hardware without a modeset. > > > > > > We can't unconditionally force a modeset on eDP panels at probe. > > > > Thanks! Just wondering what the options here might be? With fastest we end up having a mismatch with one PLL value with a > few panels. > > You seem to be stuck in some infinite loop. If your PLL parameters are mismatching that should prevent the fastset, but then I > guess you added some hack to allow the fastset despite the mismatch, and now you're trying to undo that hack by blindly forcing a > full modeset? That's right, I found myself to be between a rock and a hard place. I did discard the fastest but found out that we cannot do that. Here, another hack is introduced to force the full modeset to ensure that the driver programs these PLL values. As Jani already mentioned, this is a no go option as well. > > > > > Should we try identify the panels and setup some sort of quirks for these problematic panels or what would be the best solution? > > > > -Mika- > > > > > > > > > > > BR, > > > Jani. > > > > > > > > > > > Signed-off-by: Mika Kahola <mika.kahola@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/display/intel_dp.c | 13 +++++++++++++ > > > > 1 file changed, 13 insertions(+) > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c > > > > b/drivers/gpu/drm/i915/display/intel_dp.c > > > > index ab415f41924d..9699ded1eb5f 100644 > > > > --- a/drivers/gpu/drm/i915/display/intel_dp.c > > > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > > > > @@ -3319,6 +3319,7 @@ bool intel_dp_initial_fastset_check(struct intel_encoder *encoder, > > > > * of crtc_state->dsc, we have no way to ensure reliable fastset. > > > > * Remove once we have readout for DSC. > > > > */ > > > > + > > > > if (crtc_state->dsc.compression_enable) { > > > > drm_dbg_kms(&i915->drm, "[ENCODER:%d:%s] Forcing full modeset due to DSC being enabled\n", > > > > encoder->base.base.id, encoder->base.name); @@ -3326,6 > > > > +3327,18 @@ bool intel_dp_initial_fastset_check(struct > > > > +intel_encoder *encoder, > > > > fastset = false; > > > > } > > > > > > > > + /* > > > > + * FIXME hack to force full modeset for eDP as not always BIOS written PLL > > > > + * values does not match with the ones defined in the driver code > > > > + */ > > > > + if (!crtc_state->uapi.mode_changed && > > > > + intel_dp_is_edp(intel_dp)) { > > > > + drm_dbg_kms(&i915->drm, "Forcing full modeset for eDP\n"); > > > > + crtc_state->uapi.mode_changed = true; > > > > + fastset = false; > > > > + } > > > > + > > > > + > > > > return fastset; > > > > } > > > > > > -- > > > Jani Nikula, Intel > > -- > Ville Syrjälä > Intel
On Fri, Feb 09, 2024 at 12:13:02PM +0000, Kahola, Mika wrote: > > -----Original Message----- > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Sent: Friday, February 9, 2024 2:07 PM > > To: Kahola, Mika <mika.kahola@intel.com> > > Cc: Jani Nikula <jani.nikula@linux.intel.com>; intel-gfx@lists.freedesktop.org > > Subject: Re: [PATCH 2/2] drm/i915/display: Force full modeset for eDP > > > > On Fri, Feb 09, 2024 at 11:55:58AM +0000, Kahola, Mika wrote: > > > > -----Original Message----- > > > > From: Jani Nikula <jani.nikula@linux.intel.com> > > > > Sent: Friday, February 9, 2024 11:06 AM > > > > To: Kahola, Mika <mika.kahola@intel.com>; > > > > intel-gfx@lists.freedesktop.org > > > > Cc: Kahola, Mika <mika.kahola@intel.com> > > > > Subject: Re: [PATCH 2/2] drm/i915/display: Force full modeset for > > > > eDP > > > > > > > > On Tue, 06 Feb 2024, Mika Kahola <mika.kahola@intel.com> wrote: > > > > > Force full modeset for eDP when booting up. GOP programs PLL > > > > > parameters and hence, we would be able to use fastset for eDP. > > > > > However, with fastset we are not setting PLL values from the > > > > > driver and rely that GOP and driver PLL values match. > > > > > We have discovered that with some of the panels this is not true > > > > > and hence we would need to program PLL values by the driver. The > > > > > patch suggests a workaround as enabling full modeset when booting > > > > > up. This way we force the driver to write the PLL values to the hw. > > > > > > > > No. We want to avoid full modesets if possible, both in general and at probe. > > > > > > > > And when we do end up with modesets, the decision needs to be based > > > > on changes in the state that we can't write to the hardware without a modeset. > > > > > > > > We can't unconditionally force a modeset on eDP panels at probe. > > > > > > Thanks! Just wondering what the options here might be? With fastest we end up having a mismatch with one PLL value with a > > few panels. > > > > You seem to be stuck in some infinite loop. If your PLL parameters are mismatching that should prevent the fastset, but then I > > guess you added some hack to allow the fastset despite the mismatch, and now you're trying to undo that hack by blindly forcing a > > full modeset? > > That's right, I found myself to be between a rock and a hard place. I did discard the fastest but found out that we cannot do that. If you discarded it then why are you not already getting the full modeset you want? > Here, another hack is introduced to force the full modeset to ensure that the driver programs these PLL values. As Jani already mentioned, this is a no go option as well. > > > > > > > > > Should we try identify the panels and setup some sort of quirks for these problematic panels or what would be the best solution? > > > > > > -Mika- > > > > > > > > > > > > > > > BR, > > > > Jani. > > > > > > > > > > > > > > Signed-off-by: Mika Kahola <mika.kahola@intel.com> > > > > > --- > > > > > drivers/gpu/drm/i915/display/intel_dp.c | 13 +++++++++++++ > > > > > 1 file changed, 13 insertions(+) > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c > > > > > b/drivers/gpu/drm/i915/display/intel_dp.c > > > > > index ab415f41924d..9699ded1eb5f 100644 > > > > > --- a/drivers/gpu/drm/i915/display/intel_dp.c > > > > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > > > > > @@ -3319,6 +3319,7 @@ bool intel_dp_initial_fastset_check(struct intel_encoder *encoder, > > > > > * of crtc_state->dsc, we have no way to ensure reliable fastset. > > > > > * Remove once we have readout for DSC. > > > > > */ > > > > > + > > > > > if (crtc_state->dsc.compression_enable) { > > > > > drm_dbg_kms(&i915->drm, "[ENCODER:%d:%s] Forcing full modeset due to DSC being enabled\n", > > > > > encoder->base.base.id, encoder->base.name); @@ -3326,6 > > > > > +3327,18 @@ bool intel_dp_initial_fastset_check(struct > > > > > +intel_encoder *encoder, > > > > > fastset = false; > > > > > } > > > > > > > > > > + /* > > > > > + * FIXME hack to force full modeset for eDP as not always BIOS written PLL > > > > > + * values does not match with the ones defined in the driver code > > > > > + */ > > > > > + if (!crtc_state->uapi.mode_changed && > > > > > + intel_dp_is_edp(intel_dp)) { > > > > > + drm_dbg_kms(&i915->drm, "Forcing full modeset for eDP\n"); > > > > > + crtc_state->uapi.mode_changed = true; > > > > > + fastset = false; > > > > > + } > > > > > + > > > > > + > > > > > return fastset; > > > > > } > > > > > > > > -- > > > > Jani Nikula, Intel > > > > -- > > Ville Syrjälä > > Intel
> -----Original Message----- > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > Sent: Friday, February 9, 2024 2:19 PM > To: Kahola, Mika <mika.kahola@intel.com> > Cc: Jani Nikula <jani.nikula@linux.intel.com>; intel-gfx@lists.freedesktop.org > Subject: Re: [PATCH 2/2] drm/i915/display: Force full modeset for eDP > > On Fri, Feb 09, 2024 at 12:13:02PM +0000, Kahola, Mika wrote: > > > -----Original Message----- > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > Sent: Friday, February 9, 2024 2:07 PM > > > To: Kahola, Mika <mika.kahola@intel.com> > > > Cc: Jani Nikula <jani.nikula@linux.intel.com>; > > > intel-gfx@lists.freedesktop.org > > > Subject: Re: [PATCH 2/2] drm/i915/display: Force full modeset for > > > eDP > > > > > > On Fri, Feb 09, 2024 at 11:55:58AM +0000, Kahola, Mika wrote: > > > > > -----Original Message----- > > > > > From: Jani Nikula <jani.nikula@linux.intel.com> > > > > > Sent: Friday, February 9, 2024 11:06 AM > > > > > To: Kahola, Mika <mika.kahola@intel.com>; > > > > > intel-gfx@lists.freedesktop.org > > > > > Cc: Kahola, Mika <mika.kahola@intel.com> > > > > > Subject: Re: [PATCH 2/2] drm/i915/display: Force full modeset > > > > > for eDP > > > > > > > > > > On Tue, 06 Feb 2024, Mika Kahola <mika.kahola@intel.com> wrote: > > > > > > Force full modeset for eDP when booting up. GOP programs PLL > > > > > > parameters and hence, we would be able to use fastset for eDP. > > > > > > However, with fastset we are not setting PLL values from the > > > > > > driver and rely that GOP and driver PLL values match. > > > > > > We have discovered that with some of the panels this is not > > > > > > true and hence we would need to program PLL values by the > > > > > > driver. The patch suggests a workaround as enabling full > > > > > > modeset when booting up. This way we force the driver to write the PLL values to the hw. > > > > > > > > > > No. We want to avoid full modesets if possible, both in general and at probe. > > > > > > > > > > And when we do end up with modesets, the decision needs to be > > > > > based on changes in the state that we can't write to the hardware without a modeset. > > > > > > > > > > We can't unconditionally force a modeset on eDP panels at probe. > > > > > > > > Thanks! Just wondering what the options here might be? With > > > > fastest we end up having a mismatch with one PLL value with a > > > few panels. > > > > > > You seem to be stuck in some infinite loop. If your PLL parameters > > > are mismatching that should prevent the fastset, but then I guess > > > you added some hack to allow the fastset despite the mismatch, and now you're trying to undo that hack by blindly forcing a > full modeset? > > > > That's right, I found myself to be between a rock and a hard place. I did discard the fastest but found out that we cannot do that. > > If you discarded it then why are you not already getting the full modeset you want? > Poor choice of words, I guess. What I meant that I discarded the state verification in case of fastest. This way the mismatch is hidden under the carpet. > > Here, another hack is introduced to force the full modeset to ensure that the driver programs these PLL values. As Jani already > mentioned, this is a no go option as well. > > > > > > > > > > > > > Should we try identify the panels and setup some sort of quirks for these problematic panels or what would be the best > solution? > > > > > > > > -Mika- > > > > > > > > > > > > > > > > > > > BR, > > > > > Jani. > > > > > > > > > > > > > > > > > Signed-off-by: Mika Kahola <mika.kahola@intel.com> > > > > > > --- > > > > > > drivers/gpu/drm/i915/display/intel_dp.c | 13 +++++++++++++ > > > > > > 1 file changed, 13 insertions(+) > > > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c > > > > > > b/drivers/gpu/drm/i915/display/intel_dp.c > > > > > > index ab415f41924d..9699ded1eb5f 100644 > > > > > > --- a/drivers/gpu/drm/i915/display/intel_dp.c > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > > > > > > @@ -3319,6 +3319,7 @@ bool intel_dp_initial_fastset_check(struct intel_encoder *encoder, > > > > > > * of crtc_state->dsc, we have no way to ensure reliable fastset. > > > > > > * Remove once we have readout for DSC. > > > > > > */ > > > > > > + > > > > > > if (crtc_state->dsc.compression_enable) { > > > > > > drm_dbg_kms(&i915->drm, "[ENCODER:%d:%s] Forcing full modeset due to DSC being enabled\n", > > > > > > encoder->base.base.id, encoder->base.name); @@ -3326,6 > > > > > > +3327,18 @@ bool intel_dp_initial_fastset_check(struct > > > > > > +intel_encoder *encoder, > > > > > > fastset = false; > > > > > > } > > > > > > > > > > > > + /* > > > > > > + * FIXME hack to force full modeset for eDP as not always BIOS written PLL > > > > > > + * values does not match with the ones defined in the driver code > > > > > > + */ > > > > > > + if (!crtc_state->uapi.mode_changed && > > > > > > + intel_dp_is_edp(intel_dp)) { > > > > > > + drm_dbg_kms(&i915->drm, "Forcing full modeset for eDP\n"); > > > > > > + crtc_state->uapi.mode_changed = true; > > > > > > + fastset = false; > > > > > > + } > > > > > > + > > > > > > + > > > > > > return fastset; > > > > > > } > > > > > > > > > > -- > > > > > Jani Nikula, Intel > > > > > > -- > > > Ville Syrjälä > > > Intel > > -- > Ville Syrjälä > Intel
On Fri, Feb 09, 2024 at 12:33:20PM +0000, Kahola, Mika wrote: > > -----Original Message----- > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Sent: Friday, February 9, 2024 2:19 PM > > To: Kahola, Mika <mika.kahola@intel.com> > > Cc: Jani Nikula <jani.nikula@linux.intel.com>; intel-gfx@lists.freedesktop.org > > Subject: Re: [PATCH 2/2] drm/i915/display: Force full modeset for eDP > > > > On Fri, Feb 09, 2024 at 12:13:02PM +0000, Kahola, Mika wrote: > > > > -----Original Message----- > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Sent: Friday, February 9, 2024 2:07 PM > > > > To: Kahola, Mika <mika.kahola@intel.com> > > > > Cc: Jani Nikula <jani.nikula@linux.intel.com>; > > > > intel-gfx@lists.freedesktop.org > > > > Subject: Re: [PATCH 2/2] drm/i915/display: Force full modeset for > > > > eDP > > > > > > > > On Fri, Feb 09, 2024 at 11:55:58AM +0000, Kahola, Mika wrote: > > > > > > -----Original Message----- > > > > > > From: Jani Nikula <jani.nikula@linux.intel.com> > > > > > > Sent: Friday, February 9, 2024 11:06 AM > > > > > > To: Kahola, Mika <mika.kahola@intel.com>; > > > > > > intel-gfx@lists.freedesktop.org > > > > > > Cc: Kahola, Mika <mika.kahola@intel.com> > > > > > > Subject: Re: [PATCH 2/2] drm/i915/display: Force full modeset > > > > > > for eDP > > > > > > > > > > > > On Tue, 06 Feb 2024, Mika Kahola <mika.kahola@intel.com> wrote: > > > > > > > Force full modeset for eDP when booting up. GOP programs PLL > > > > > > > parameters and hence, we would be able to use fastset for eDP. > > > > > > > However, with fastset we are not setting PLL values from the > > > > > > > driver and rely that GOP and driver PLL values match. > > > > > > > We have discovered that with some of the panels this is not > > > > > > > true and hence we would need to program PLL values by the > > > > > > > driver. The patch suggests a workaround as enabling full > > > > > > > modeset when booting up. This way we force the driver to write the PLL values to the hw. > > > > > > > > > > > > No. We want to avoid full modesets if possible, both in general and at probe. > > > > > > > > > > > > And when we do end up with modesets, the decision needs to be > > > > > > based on changes in the state that we can't write to the hardware without a modeset. > > > > > > > > > > > > We can't unconditionally force a modeset on eDP panels at probe. > > > > > > > > > > Thanks! Just wondering what the options here might be? With > > > > > fastest we end up having a mismatch with one PLL value with a > > > > few panels. > > > > > > > > You seem to be stuck in some infinite loop. If your PLL parameters > > > > are mismatching that should prevent the fastset, but then I guess > > > > you added some hack to allow the fastset despite the mismatch, and now you're trying to undo that hack by blindly forcing a > > full modeset? > > > > > > That's right, I found myself to be between a rock and a hard place. I did discard the fastest but found out that we cannot do that. > > > > If you discarded it then why are you not already getting the full modeset you want? > > > Poor choice of words, I guess. What I meant that I discarded the state verification in case of fastest. This way the mismatch is hidden under the carpet. Right. So just add the state verification back and it'll just work (tm). > > > > > Here, another hack is introduced to force the full modeset to ensure that the driver programs these PLL values. As Jani already > > mentioned, this is a no go option as well. > > > > > > > > > > > > > > > > > Should we try identify the panels and setup some sort of quirks for these problematic panels or what would be the best > > solution? > > > > > > > > > > -Mika- > > > > > > > > > > > > > > > > > > > > > > > BR, > > > > > > Jani. > > > > > > > > > > > > > > > > > > > > Signed-off-by: Mika Kahola <mika.kahola@intel.com> > > > > > > > --- > > > > > > > drivers/gpu/drm/i915/display/intel_dp.c | 13 +++++++++++++ > > > > > > > 1 file changed, 13 insertions(+) > > > > > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c > > > > > > > b/drivers/gpu/drm/i915/display/intel_dp.c > > > > > > > index ab415f41924d..9699ded1eb5f 100644 > > > > > > > --- a/drivers/gpu/drm/i915/display/intel_dp.c > > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > > > > > > > @@ -3319,6 +3319,7 @@ bool intel_dp_initial_fastset_check(struct intel_encoder *encoder, > > > > > > > * of crtc_state->dsc, we have no way to ensure reliable fastset. > > > > > > > * Remove once we have readout for DSC. > > > > > > > */ > > > > > > > + > > > > > > > if (crtc_state->dsc.compression_enable) { > > > > > > > drm_dbg_kms(&i915->drm, "[ENCODER:%d:%s] Forcing full modeset due to DSC being enabled\n", > > > > > > > encoder->base.base.id, encoder->base.name); @@ -3326,6 > > > > > > > +3327,18 @@ bool intel_dp_initial_fastset_check(struct > > > > > > > +intel_encoder *encoder, > > > > > > > fastset = false; > > > > > > > } > > > > > > > > > > > > > > + /* > > > > > > > + * FIXME hack to force full modeset for eDP as not always BIOS written PLL > > > > > > > + * values does not match with the ones defined in the driver code > > > > > > > + */ > > > > > > > + if (!crtc_state->uapi.mode_changed && > > > > > > > + intel_dp_is_edp(intel_dp)) { > > > > > > > + drm_dbg_kms(&i915->drm, "Forcing full modeset for eDP\n"); > > > > > > > + crtc_state->uapi.mode_changed = true; > > > > > > > + fastset = false; > > > > > > > + } > > > > > > > + > > > > > > > + > > > > > > > return fastset; > > > > > > > } > > > > > > > > > > > > -- > > > > > > Jani Nikula, Intel > > > > > > > > -- > > > > Ville Syrjälä > > > > Intel > > > > -- > > Ville Syrjälä > > Intel
> -----Original Message----- > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > Sent: Friday, February 9, 2024 2:49 PM > To: Kahola, Mika <mika.kahola@intel.com> > Cc: Jani Nikula <jani.nikula@linux.intel.com>; intel-gfx@lists.freedesktop.org > Subject: Re: [PATCH 2/2] drm/i915/display: Force full modeset for eDP > > On Fri, Feb 09, 2024 at 12:33:20PM +0000, Kahola, Mika wrote: > > > -----Original Message----- > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > Sent: Friday, February 9, 2024 2:19 PM > > > To: Kahola, Mika <mika.kahola@intel.com> > > > Cc: Jani Nikula <jani.nikula@linux.intel.com>; > > > intel-gfx@lists.freedesktop.org > > > Subject: Re: [PATCH 2/2] drm/i915/display: Force full modeset for > > > eDP > > > > > > On Fri, Feb 09, 2024 at 12:13:02PM +0000, Kahola, Mika wrote: > > > > > -----Original Message----- > > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > Sent: Friday, February 9, 2024 2:07 PM > > > > > To: Kahola, Mika <mika.kahola@intel.com> > > > > > Cc: Jani Nikula <jani.nikula@linux.intel.com>; > > > > > intel-gfx@lists.freedesktop.org > > > > > Subject: Re: [PATCH 2/2] drm/i915/display: Force full modeset > > > > > for eDP > > > > > > > > > > On Fri, Feb 09, 2024 at 11:55:58AM +0000, Kahola, Mika wrote: > > > > > > > -----Original Message----- > > > > > > > From: Jani Nikula <jani.nikula@linux.intel.com> > > > > > > > Sent: Friday, February 9, 2024 11:06 AM > > > > > > > To: Kahola, Mika <mika.kahola@intel.com>; > > > > > > > intel-gfx@lists.freedesktop.org > > > > > > > Cc: Kahola, Mika <mika.kahola@intel.com> > > > > > > > Subject: Re: [PATCH 2/2] drm/i915/display: Force full > > > > > > > modeset for eDP > > > > > > > > > > > > > > On Tue, 06 Feb 2024, Mika Kahola <mika.kahola@intel.com> wrote: > > > > > > > > Force full modeset for eDP when booting up. GOP programs > > > > > > > > PLL parameters and hence, we would be able to use fastset for eDP. > > > > > > > > However, with fastset we are not setting PLL values from > > > > > > > > the driver and rely that GOP and driver PLL values match. > > > > > > > > We have discovered that with some of the panels this is > > > > > > > > not true and hence we would need to program PLL values by > > > > > > > > the driver. The patch suggests a workaround as enabling > > > > > > > > full modeset when booting up. This way we force the driver to write the PLL values to the hw. > > > > > > > > > > > > > > No. We want to avoid full modesets if possible, both in general and at probe. > > > > > > > > > > > > > > And when we do end up with modesets, the decision needs to > > > > > > > be based on changes in the state that we can't write to the hardware without a modeset. > > > > > > > > > > > > > > We can't unconditionally force a modeset on eDP panels at probe. > > > > > > > > > > > > Thanks! Just wondering what the options here might be? With > > > > > > fastest we end up having a mismatch with one PLL value with a > > > > > few panels. > > > > > > > > > > You seem to be stuck in some infinite loop. If your PLL > > > > > parameters are mismatching that should prevent the fastset, but > > > > > then I guess you added some hack to allow the fastset despite > > > > > the mismatch, and now you're trying to undo that hack by blindly > > > > > forcing a > > > full modeset? > > > > > > > > That's right, I found myself to be between a rock and a hard place. I did discard the fastest but found out that we cannot do > that. > > > > > > If you discarded it then why are you not already getting the full modeset you want? > > > > > Poor choice of words, I guess. What I meant that I discarded the state verification in case of fastest. This way the mismatch is > hidden under the carpet. > > Right. So just add the state verification back and it'll just work (tm). I wish it would be that simple. With fastest it seems that we are not programming PLL values but instead rely the ones GOP programs. When reading out these values from HW there is this one value that differs from the SW state defined by our driver. I was thinking about that we should read the state from HW and update our SW state based on that but then we would end up comparing HW state to HW state, which to me is not what state verification is all about. > > > > > > > > > Here, another hack is introduced to force the full modeset to > > > > ensure that the driver programs these PLL values. As Jani already > > > mentioned, this is a no go option as well. > > > > > > > > > > > > > > > > > > > > > Should we try identify the panels and setup some sort of > > > > > > quirks for these problematic panels or what would be the best > > > solution? > > > > > > > > > > > > -Mika- > > > > > > > > > > > > > > > > > > > > > > > > > > > BR, > > > > > > > Jani. > > > > > > > > > > > > > > > > > > > > > > > Signed-off-by: Mika Kahola <mika.kahola@intel.com> > > > > > > > > --- > > > > > > > > drivers/gpu/drm/i915/display/intel_dp.c | 13 > > > > > > > > +++++++++++++ > > > > > > > > 1 file changed, 13 insertions(+) > > > > > > > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c > > > > > > > > b/drivers/gpu/drm/i915/display/intel_dp.c > > > > > > > > index ab415f41924d..9699ded1eb5f 100644 > > > > > > > > --- a/drivers/gpu/drm/i915/display/intel_dp.c > > > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > > > > > > > > @@ -3319,6 +3319,7 @@ bool intel_dp_initial_fastset_check(struct intel_encoder *encoder, > > > > > > > > * of crtc_state->dsc, we have no way to ensure reliable fastset. > > > > > > > > * Remove once we have readout for DSC. > > > > > > > > */ > > > > > > > > + > > > > > > > > if (crtc_state->dsc.compression_enable) { > > > > > > > > drm_dbg_kms(&i915->drm, "[ENCODER:%d:%s] Forcing full modeset due to DSC being enabled\n", > > > > > > > > encoder->base.base.id, encoder->base.name); @@ > > > > > > > > -3326,6 > > > > > > > > +3327,18 @@ bool intel_dp_initial_fastset_check(struct > > > > > > > > +intel_encoder *encoder, > > > > > > > > fastset = false; > > > > > > > > } > > > > > > > > > > > > > > > > + /* > > > > > > > > + * FIXME hack to force full modeset for eDP as not always BIOS written PLL > > > > > > > > + * values does not match with the ones defined in the driver code > > > > > > > > + */ > > > > > > > > + if (!crtc_state->uapi.mode_changed && > > > > > > > > + intel_dp_is_edp(intel_dp)) { > > > > > > > > + drm_dbg_kms(&i915->drm, "Forcing full modeset for eDP\n"); > > > > > > > > + crtc_state->uapi.mode_changed = true; > > > > > > > > + fastset = false; > > > > > > > > + } > > > > > > > > + > > > > > > > > + > > > > > > > > return fastset; > > > > > > > > } > > > > > > > > > > > > > > -- > > > > > > > Jani Nikula, Intel > > > > > > > > > > -- > > > > > Ville Syrjälä > > > > > Intel > > > > > > -- > > > Ville Syrjälä > > > Intel > > -- > Ville Syrjälä > Intel
On Fri, Feb 09, 2024 at 01:17:27PM +0000, Kahola, Mika wrote: > > -----Original Message----- > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Sent: Friday, February 9, 2024 2:49 PM > > To: Kahola, Mika <mika.kahola@intel.com> > > Cc: Jani Nikula <jani.nikula@linux.intel.com>; intel-gfx@lists.freedesktop.org > > Subject: Re: [PATCH 2/2] drm/i915/display: Force full modeset for eDP > > > > On Fri, Feb 09, 2024 at 12:33:20PM +0000, Kahola, Mika wrote: > > > > -----Original Message----- > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Sent: Friday, February 9, 2024 2:19 PM > > > > To: Kahola, Mika <mika.kahola@intel.com> > > > > Cc: Jani Nikula <jani.nikula@linux.intel.com>; > > > > intel-gfx@lists.freedesktop.org > > > > Subject: Re: [PATCH 2/2] drm/i915/display: Force full modeset for > > > > eDP > > > > > > > > On Fri, Feb 09, 2024 at 12:13:02PM +0000, Kahola, Mika wrote: > > > > > > -----Original Message----- > > > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > Sent: Friday, February 9, 2024 2:07 PM > > > > > > To: Kahola, Mika <mika.kahola@intel.com> > > > > > > Cc: Jani Nikula <jani.nikula@linux.intel.com>; > > > > > > intel-gfx@lists.freedesktop.org > > > > > > Subject: Re: [PATCH 2/2] drm/i915/display: Force full modeset > > > > > > for eDP > > > > > > > > > > > > On Fri, Feb 09, 2024 at 11:55:58AM +0000, Kahola, Mika wrote: > > > > > > > > -----Original Message----- > > > > > > > > From: Jani Nikula <jani.nikula@linux.intel.com> > > > > > > > > Sent: Friday, February 9, 2024 11:06 AM > > > > > > > > To: Kahola, Mika <mika.kahola@intel.com>; > > > > > > > > intel-gfx@lists.freedesktop.org > > > > > > > > Cc: Kahola, Mika <mika.kahola@intel.com> > > > > > > > > Subject: Re: [PATCH 2/2] drm/i915/display: Force full > > > > > > > > modeset for eDP > > > > > > > > > > > > > > > > On Tue, 06 Feb 2024, Mika Kahola <mika.kahola@intel.com> wrote: > > > > > > > > > Force full modeset for eDP when booting up. GOP programs > > > > > > > > > PLL parameters and hence, we would be able to use fastset for eDP. > > > > > > > > > However, with fastset we are not setting PLL values from > > > > > > > > > the driver and rely that GOP and driver PLL values match. > > > > > > > > > We have discovered that with some of the panels this is > > > > > > > > > not true and hence we would need to program PLL values by > > > > > > > > > the driver. The patch suggests a workaround as enabling > > > > > > > > > full modeset when booting up. This way we force the driver to write the PLL values to the hw. > > > > > > > > > > > > > > > > No. We want to avoid full modesets if possible, both in general and at probe. > > > > > > > > > > > > > > > > And when we do end up with modesets, the decision needs to > > > > > > > > be based on changes in the state that we can't write to the hardware without a modeset. > > > > > > > > > > > > > > > > We can't unconditionally force a modeset on eDP panels at probe. > > > > > > > > > > > > > > Thanks! Just wondering what the options here might be? With > > > > > > > fastest we end up having a mismatch with one PLL value with a > > > > > > few panels. > > > > > > > > > > > > You seem to be stuck in some infinite loop. If your PLL > > > > > > parameters are mismatching that should prevent the fastset, but > > > > > > then I guess you added some hack to allow the fastset despite > > > > > > the mismatch, and now you're trying to undo that hack by blindly > > > > > > forcing a > > > > full modeset? > > > > > > > > > > That's right, I found myself to be between a rock and a hard place. I did discard the fastest but found out that we cannot do > > that. > > > > > > > > If you discarded it then why are you not already getting the full modeset you want? > > > > > > > Poor choice of words, I guess. What I meant that I discarded the state verification in case of fastest. This way the mismatch is > > hidden under the carpet. > > > > Right. So just add the state verification back and it'll just work (tm). > > I wish it would be that simple. With fastest it seems that we are not programming PLL values but instead rely the ones GOP programs. When reading out these values from HW there is this one value that differs from the SW state defined by our driver. I was thinking about that we should read the state from HW and update our SW state based on that but then we would end up comparing HW state to HW state, which to me is not what state verification is all about. The actual problem would appear to be that someone forgot to handle these snps/cx0 PLLs in pipe_config_compare(). Implementing these PLLs outside the dpll_mgr framework was another big mistake. And the ridonculous size of dpll_hw_state is another problem. We need to do some major refactoring to clean up this mess once and for all. Some kind of plan: 1. move all PLLs over to the dpll_mgr framework, starting with snps/cx0 2. move all PLL hw state verification into the dpll_mgr so it can be customized per-PLL type 3. Perhaps make dpll_hw_state opaque and split it up into per-PLL type variants. Need to think about memory allocation issues with this one... I think I'll see if I can quickly type up a start for 2...
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c index ab415f41924d..9699ded1eb5f 100644 --- a/drivers/gpu/drm/i915/display/intel_dp.c +++ b/drivers/gpu/drm/i915/display/intel_dp.c @@ -3319,6 +3319,7 @@ bool intel_dp_initial_fastset_check(struct intel_encoder *encoder, * of crtc_state->dsc, we have no way to ensure reliable fastset. * Remove once we have readout for DSC. */ + if (crtc_state->dsc.compression_enable) { drm_dbg_kms(&i915->drm, "[ENCODER:%d:%s] Forcing full modeset due to DSC being enabled\n", encoder->base.base.id, encoder->base.name); @@ -3326,6 +3327,18 @@ bool intel_dp_initial_fastset_check(struct intel_encoder *encoder, fastset = false; } + /* + * FIXME hack to force full modeset for eDP as not always BIOS written PLL + * values does not match with the ones defined in the driver code + */ + if (!crtc_state->uapi.mode_changed && + intel_dp_is_edp(intel_dp)) { + drm_dbg_kms(&i915->drm, "Forcing full modeset for eDP\n"); + crtc_state->uapi.mode_changed = true; + fastset = false; + } + + return fastset; }
Force full modeset for eDP when booting up. GOP programs PLL parameters and hence, we would be able to use fastset for eDP. However, with fastset we are not setting PLL values from the driver and rely that GOP and driver PLL values match. We have discovered that with some of the panels this is not true and hence we would need to program PLL values by the driver. The patch suggests a workaround as enabling full modeset when booting up. This way we force the driver to write the PLL values to the hw. Signed-off-by: Mika Kahola <mika.kahola@intel.com> --- drivers/gpu/drm/i915/display/intel_dp.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)