On Wed, Oct 23, 2024 at 05:40:56PM +0300, Imre Deak wrote:
> On Wed, Oct 23, 2024 at 03:16:11PM +0300, Ville Syrjälä wrote:
> > On Tue, Oct 22, 2024 at 12:46:23PM +0300, Imre Deak wrote:
> > > While updating the source OUI on the sink the driver should avoid
> > > writing the OUI if it's already up-to-date to prevent the sink from
> > > resetting itself in response to the update. On eDP - the only output
> > > type where the OUI was updated so far - the driver ensured this by
> > > comparing the current source OUI DPCD register values with the expected
> > > Intel OUI value, skipping the update in case of a match. On some non-eDP
> > > sinks - at least on Synaptics branch devices - this method doesn't work,
> > > since the source OUI DPCD registers read back as all 0, even after
> > > updating the registers.
> > >
> > > Handle the above kind of sinks by tracking when the OUI was updated and
> > > so should be valid, regardless of what the DPCD registers contain.
> > >
> > > eDP sinks reset the written source OUI value when the panel power is
> > > disabled, invalidate the OUI state accordingly.
> > >
> > > This is required by a follow-up patch updating the source OUI for
> > > non-eDP sink types as well.
> > >
> > > v2: Fix setting intel_dp::oui_valid=true, if the DPCD register contains
> > > already the expected value.
> > >
> > > Signed-off-by: Imre Deak <imre.deak@xxxxxxxxx>
> > > ---
> > > drivers/gpu/drm/i915/display/g4x_dp.c | 1 +
> > > drivers/gpu/drm/i915/display/intel_ddi.c | 1 +
> > > .../drm/i915/display/intel_display_types.h | 1 +
> > > drivers/gpu/drm/i915/display/intel_dp.c | 38 ++++++++++++-------
> > > drivers/gpu/drm/i915/display/intel_dp.h | 1 +
> > > drivers/gpu/drm/i915/display/intel_pps.c | 6 ++-
> > > 6 files changed, 34 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/g4x_dp.c b/drivers/gpu/drm/i915/display/g4x_dp.c
> > > index 440fb3002f286..596e926ef0894 100644
> > > --- a/drivers/gpu/drm/i915/display/g4x_dp.c
> > > +++ b/drivers/gpu/drm/i915/display/g4x_dp.c
> > > @@ -1251,6 +1251,7 @@ static void intel_dp_encoder_reset(struct drm_encoder *encoder)
> > > intel_dp->DP = intel_de_read(display, intel_dp->output_reg);
> > >
> > > intel_dp->reset_link_params = true;
> > > + intel_dp_invalidate_source_oui(intel_dp);
> > >
> > > if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > > vlv_pps_pipe_reset(intel_dp);
> > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> > > index fe1ded6707f90..465f245a53c48 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > > @@ -4392,6 +4392,7 @@ static void intel_ddi_encoder_reset(struct drm_encoder *encoder)
> > > struct intel_digital_port *dig_port = enc_to_dig_port(to_intel_encoder(encoder));
> > >
> > > intel_dp->reset_link_params = true;
> > > + intel_dp_invalidate_source_oui(intel_dp);
> > >
> > > intel_pps_encoder_reset(intel_dp);
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > index 2bb1fa64da2f1..9b46427806c6d 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > @@ -1765,6 +1765,7 @@ struct intel_dp {
> > >
> > > /* When we last wrote the OUI for eDP */
> > > unsigned long last_oui_write;
> > > + bool oui_valid;
> > >
> > > bool colorimetry_support;
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> > > index 96ee5de719a2c..a6f84b8f770d0 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > > @@ -3400,33 +3400,46 @@ void intel_dp_sink_disable_decompression(struct intel_atomic_state *state,
> > > }
> > >
> > > static void
> > > -intel_edp_init_source_oui(struct intel_dp *intel_dp, bool careful)
> > > +intel_dp_init_source_oui(struct intel_dp *intel_dp)
> > > {
> > > struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> > > u8 oui[] = { 0x00, 0xaa, 0x01 };
> > > u8 buf[3] = {};
> > >
> > > + if (!intel_dp_is_edp(intel_dp))
> > > + return;
> > > +
> > > + if (READ_ONCE(intel_dp->oui_valid))
> > > + return;
> > > +
> > > + WRITE_ONCE(intel_dp->oui_valid, true);
> >
> > Would seem nicer if this is set alognside the timestamp.
> > Are we actually always setting both?
>
> Yes. In case DPCD contained already the correct value, it's not known
> when it was written, so the code assumes it's written now.
>
> > In that case maybe extract both into a small helper?
>
> You mean setting both at the same place, but not sure how it would work:
> I set the flag before writing the register to account for the
> T1: oui_valid=true -> T2: HPD/oui_valid=false -> T1: write DPCD case,
> guaranteeing that oui_valid remains false. OTOH last_oui_write should be
> set after the DPCD write so that the wait duration can't get shorter
> than needed.
OK, a bit ugly but can't see a better non-racy way for it, so
Reviewed-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>
> > > +
> > > /*
> > > * During driver init, we want to be careful and avoid changing the source OUI if it's
> > > * already set to what we want, so as to avoid clearing any state by accident
> > > */
> > > - if (careful) {
> > > - if (drm_dp_dpcd_read(&intel_dp->aux, DP_SOURCE_OUI, buf, sizeof(buf)) < 0)
> > > - drm_err(&i915->drm, "Failed to read source OUI\n");
> > > + if (drm_dp_dpcd_read(&intel_dp->aux, DP_SOURCE_OUI, buf, sizeof(buf)) < 0)
> > > + drm_err(&i915->drm, "Failed to read source OUI\n");
> > >
> > > - if (memcmp(oui, buf, sizeof(oui)) == 0) {
> > > - /* Assume the OUI was written now. */
> > > - intel_dp->last_oui_write = jiffies;
> > > - return;
> > > - }
> > > + if (memcmp(oui, buf, sizeof(oui)) == 0) {
> > > + /* Assume the OUI was written now. */
> > > + intel_dp->last_oui_write = jiffies;
> > > + return;
> > > }
> > >
> > > - if (drm_dp_dpcd_write(&intel_dp->aux, DP_SOURCE_OUI, oui, sizeof(oui)) < 0)
> > > + if (drm_dp_dpcd_write(&intel_dp->aux, DP_SOURCE_OUI, oui, sizeof(oui)) < 0) {
> > > drm_err(&i915->drm, "Failed to write source OUI\n");
> > > + WRITE_ONCE(intel_dp->oui_valid, false);
> > > + }
> > >
> > > intel_dp->last_oui_write = jiffies;
> > > }
> > >
> > > +void intel_dp_invalidate_source_oui(struct intel_dp *intel_dp)
> > > +{
> > > + WRITE_ONCE(intel_dp->oui_valid, false);
> > > +}
> > > +
> > > void intel_dp_wait_source_oui(struct intel_dp *intel_dp)
> > > {
> > > struct intel_connector *connector = intel_dp->attached_connector;
> > > @@ -3462,8 +3475,7 @@ void intel_dp_set_power(struct intel_dp *intel_dp, u8 mode)
> > > lspcon_resume(dp_to_dig_port(intel_dp));
> > >
> > > /* Write the source OUI as early as possible */
> > > - if (intel_dp_is_edp(intel_dp))
> > > - intel_edp_init_source_oui(intel_dp, false);
> > > + intel_dp_init_source_oui(intel_dp);
> > >
> > > /*
> > > * When turning on, we need to retry for 1ms to give the sink
> > > @@ -4184,7 +4196,7 @@ intel_edp_init_dpcd(struct intel_dp *intel_dp, struct intel_connector *connector
> > > * If needed, program our source OUI so we can make various Intel-specific AUX services
> > > * available (such as HDR backlight controls)
> > > */
> > > - intel_edp_init_source_oui(intel_dp, true);
> > > + intel_dp_init_source_oui(intel_dp);
> > >
> > > return true;
> > > }
> > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.h b/drivers/gpu/drm/i915/display/intel_dp.h
> > > index 4efb9605a50e0..48f10876be656 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dp.h
> > > +++ b/drivers/gpu/drm/i915/display/intel_dp.h
> > > @@ -189,6 +189,7 @@ void intel_dp_check_frl_training(struct intel_dp *intel_dp);
> > > void intel_dp_pcon_dsc_configure(struct intel_dp *intel_dp,
> > > const struct intel_crtc_state *crtc_state);
> > >
> > > +void intel_dp_invalidate_source_oui(struct intel_dp *intel_dp);
> > > void intel_dp_wait_source_oui(struct intel_dp *intel_dp);
> > > int intel_dp_output_bpp(enum intel_output_format output_format, int bpp);
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_pps.c b/drivers/gpu/drm/i915/display/intel_pps.c
> > > index 64f1f7ea94993..3d1f9b17034d0 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_pps.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_pps.c
> > > @@ -861,8 +861,10 @@ static void intel_pps_vdd_off_sync_unlocked(struct intel_dp *intel_dp)
> > > intel_de_read(display, pp_stat_reg),
> > > intel_de_read(display, pp_ctrl_reg));
> > >
> > > - if ((pp & PANEL_POWER_ON) == 0)
> > > + if ((pp & PANEL_POWER_ON) == 0) {
> > > intel_dp->pps.panel_power_off_time = ktime_get_boottime();
> > > + intel_dp_invalidate_source_oui(intel_dp);
> >
> > Hmm. I was thinking the rsulting long HPD would be enough, but I
> > guess we do have at least one example of a machine with no HPD on
> > eDP :(
>
> Ok, didn't think of an HPD getting always generated (is that guaranteed
> vs. only an artifact?) Another reason for doing the above synchronously
> here would be to guarantee that a modeset disabling/reenabling the panel
> power will be guaranteed to see oui_valid=false during reenabling, so
> that reenabling will rewrite the OUI (the HPD might be handled at any
> point wrt. the reenabling).
>
> > > + }
> > >
> > > intel_display_power_put(dev_priv,
> > > intel_aux_power_domain(dig_port),
> > > @@ -1074,6 +1076,8 @@ void intel_pps_off_unlocked(struct intel_dp *intel_dp)
> > > wait_panel_off(intel_dp);
> > > intel_dp->pps.panel_power_off_time = ktime_get_boottime();
> > >
> > > + intel_dp_invalidate_source_oui(intel_dp);
> > > +
> > > /* We got a reference when we enabled the VDD. */
> > > intel_display_power_put(dev_priv,
> > > intel_aux_power_domain(dig_port),
> > > --
> > > 2.44.2
> >
> > --
> > Ville Syrjälä
> > Intel
--
Ville Syrjälä
Intel
---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki
Business Identity Code: 0357606 - 4
Domiciled in Helsinki
This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
