On Fri, Sep 24, 2021 at 01:59:35PM +0300, Jani Nikula wrote:
> On Thu, 23 Sep 2021, "Souza, Jose" <jose.so...@intel.com> wrote:
> > On Tue, 2021-09-21 at 03:23 +0300, Imre Deak wrote:
> >> Atm during driver loading and system resume TypeC ports are accessed
> >> before their HW/SW state is synced. Move the TypeC port sanitization to
> >> the encoder's sync_state hook to fix this.
> >>
> >> Fixes: f9e76a6e68d3 ("drm/i915: Add an encoder hook to sanitize its state
> >> during init/resume")
> >> Cc: José Roberto de Souza <jose.so...@intel.com>
> >> Cc: Ville Syrjälä <ville.syrj...@linux.intel.com>
> >> Signed-off-by: Imre Deak <imre.d...@intel.com>
> >> ---
> >> drivers/gpu/drm/i915/display/intel_ddi.c | 8 +++++++-
> >> drivers/gpu/drm/i915/display/intel_display.c | 20 +++++---------------
> >> 2 files changed, 12 insertions(+), 16 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c
> >> b/drivers/gpu/drm/i915/display/intel_ddi.c
> >> index bba0ab99836b1..c4ed4675f5791 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> >> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> >> @@ -3840,7 +3840,13 @@ void hsw_ddi_get_config(struct intel_encoder
> >> *encoder,
> >> static void intel_ddi_sync_state(struct intel_encoder *encoder,
> >> const struct intel_crtc_state *crtc_state)
> >> {
> >> - if (intel_crtc_has_dp_encoder(crtc_state))
> >> + struct drm_i915_private *i915 = to_i915(encoder->base.dev);
> >> + enum phy phy = intel_port_to_phy(i915, encoder->port);
> >> +
> >> + if (intel_phy_is_tc(i915, phy))
> >> + intel_tc_port_sanitize(enc_to_dig_port(encoder));
> >
> > Okay at this point we will not have any mst encoder, so the check is not
> > needed.
> >
> >> +
> >> + if (crtc_state && intel_crtc_has_dp_encoder(crtc_state))
> >> intel_dp_sync_state(encoder, crtc_state);
> >> }
> >>
> >> diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> >> b/drivers/gpu/drm/i915/display/intel_display.c
> >> index f6c0c595f6313..8547842935389 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> >> @@ -12194,18 +12194,16 @@ static void
> >> intel_modeset_readout_hw_state(struct drm_device *dev)
> >> readout_plane_state(dev_priv);
> >>
> >> for_each_intel_encoder(dev, encoder) {
> >> + struct intel_crtc_state *crtc_state = NULL;
> >> +
> >> pipe = 0;
> >>
> >> if (encoder->get_hw_state(encoder, &pipe)) {
> >> - struct intel_crtc_state *crtc_state;
> >> -
> >> crtc = intel_get_crtc_for_pipe(dev_priv, pipe);
> >> crtc_state = to_intel_crtc_state(crtc->base.state);
> >>
> >> encoder->base.crtc = &crtc->base;
> >> intel_encoder_get_config(encoder, crtc_state);
> >> - if (encoder->sync_state)
> >> - encoder->sync_state(encoder, crtc_state);
> >>
> >> /* read out to slave crtc as well for bigjoiner */
> >> if (crtc_state->bigjoiner) {
> >> @@ -12220,6 +12218,9 @@ static void intel_modeset_readout_hw_state(struct
> >> drm_device *dev)
> >> encoder->base.crtc = NULL;
> >> }
> >>
> >> + if (encoder->sync_state)
> >> + encoder->sync_state(encoder, crtc_state);
> >
> > Call sync_state() with a null crtc_state will cause a crash in
> > gen11_dsi_sync_state().
Yes, thanks for catching that.
> >
> > gen11_dsi_sync_state() is the only user of crtc_state but it only wants to
> > know what is the pipe, maybe to be safer change the argument to enum pipe?
>
> How would intel_ddi_sync_state() know when to call and not call
> intel_dp_sync_state() then? If the encoder's disabled, you shouldn't do
> that. That's the distinction NULL crtc_state gives (and obviously needs
> to be taken into account).
Yes, missed gen11_dsi_sync_state(), it should have the crtc_state != NULL check
I added to
intel_ddi_sync_state().
> BR,
> Jani.
>
>
> >
> >> +
> >> drm_dbg_kms(&dev_priv->drm,
> >> "[ENCODER:%d:%s] hw state readout: %s, pipe %c\n",
> >> encoder->base.base.id, encoder->base.name,
> >> @@ -12502,17 +12503,6 @@ intel_modeset_setup_hw_state(struct drm_device
> >> *dev,
> >> intel_modeset_readout_hw_state(dev);
> >>
> >> /* HW state is read out, now we need to sanitize this mess. */
> >> -
> >> - /* Sanitize the TypeC port mode upfront, encoders depend on this */
> >> - for_each_intel_encoder(dev, encoder) {
> >> - enum phy phy = intel_port_to_phy(dev_priv, encoder->port);
> >> -
> >> - /* We need to sanitize only the MST primary port. */
> >> - if (encoder->type != INTEL_OUTPUT_DP_MST &&
> >> - intel_phy_is_tc(dev_priv, phy))
> >> - intel_tc_port_sanitize(enc_to_dig_port(encoder));
> >> - }
> >> -
> >> get_encoder_power_domains(dev_priv);
> >>
> >> if (HAS_PCH_IBX(dev_priv))
> >
>
> --
> Jani Nikula, Intel Open Source Graphics Center
