| Message ID | 20200204000242.30542-1-vivek.kasireddy@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/ehl: Check VBT before updating the transcoder for pipe | expand |
On Mon, 03 Feb 2020, Vivek Kasireddy <vivek.kasireddy@intel.com> wrote: > Since the pipe->transcoder mapping is not expected to change unless > there is either eDP or DSI connectors present, check the VBT to confirm > their presence in addition to checking TRANS_DDI_FUNC_CTL(transcoder). > This additional check is needed on platforms like Elkhart Lake because > we cannot just rely on GOP/Firmware programmed values in > TRANS_DDI_FUNC_CTL(transcoder) before updating the transcoder mapping. > > This patch is only relevant to EHL -- and a no-op on others -- > because some of the PHYs are shared between the different DDIs and > we rely on the VBT to present the most accurate information to the > driver. > > Cc: Matt Roper <matthew.d.roper@intel.com> > Cc: José Roberto de Souza <jose.souza@intel.com> > Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com> > --- > drivers/gpu/drm/i915/display/intel_display.c | 15 ++++++++++++++- > 1 file changed, 14 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > index c0e5002ce64c..4b38f293bd88 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.c > +++ b/drivers/gpu/drm/i915/display/intel_display.c > @@ -10805,6 +10805,18 @@ static void hsw_get_ddi_pll(struct drm_i915_private *dev_priv, enum port port, > pipe_config->shared_dpll = intel_get_shared_dpll_by_id(dev_priv, id); > } > > +static bool ehl_vbt_edp_dsi_present(struct drm_i915_private *dev_priv, > + enum transcoder transcoder) > +{ > + bool edp_present = intel_bios_is_port_present(dev_priv, PORT_A); > + bool dsi_present = intel_bios_is_dsi_present(dev_priv, NULL); > + > + if (IS_ELKHARTLAKE(dev_priv)) > + return transcoder == TRANSCODER_EDP ? edp_present : dsi_present; > + > + return true; > +} One of those things... this jumps out and immediately feels all wrong, just like ehl_vbt_ddi_d_present() feels all wrong in intel_combo_phy.c. But I don't know what would be the right thing to do without spending time that I don't have on this. BR, Jani. > + > static bool hsw_get_transcoder_state(struct intel_crtc *crtc, > struct intel_crtc_state *pipe_config, > u64 *power_domain_mask, > @@ -10844,7 +10856,8 @@ static bool hsw_get_transcoder_state(struct intel_crtc *crtc, > > tmp = intel_de_read(dev_priv, > TRANS_DDI_FUNC_CTL(panel_transcoder)); > - if (!(tmp & TRANS_DDI_FUNC_ENABLE)) > + if (!(tmp & TRANS_DDI_FUNC_ENABLE) || > + !ehl_vbt_edp_dsi_present(dev_priv, panel_transcoder)) > continue; > > /*
On Tue, 4 Feb 2020 12:50:25 +0200 Jani Nikula <jani.nikula@linux.intel.com> wrote: Hi Jani, > On Mon, 03 Feb 2020, Vivek Kasireddy <vivek.kasireddy@intel.com> > wrote: > > Since the pipe->transcoder mapping is not expected to change unless > > there is either eDP or DSI connectors present, check the VBT to > > confirm their presence in addition to checking > > TRANS_DDI_FUNC_CTL(transcoder). This additional check is needed on > > platforms like Elkhart Lake because we cannot just rely on > > GOP/Firmware programmed values in TRANS_DDI_FUNC_CTL(transcoder) > > before updating the transcoder mapping. > > > > This patch is only relevant to EHL -- and a no-op on others -- > > because some of the PHYs are shared between the different DDIs and > > we rely on the VBT to present the most accurate information to the > > driver. > > > > Cc: Matt Roper <matthew.d.roper@intel.com> > > Cc: José Roberto de Souza <jose.souza@intel.com> > > Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_display.c | 15 ++++++++++++++- > > 1 file changed, 14 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c > > b/drivers/gpu/drm/i915/display/intel_display.c index > > c0e5002ce64c..4b38f293bd88 100644 --- > > a/drivers/gpu/drm/i915/display/intel_display.c +++ > > b/drivers/gpu/drm/i915/display/intel_display.c @@ -10805,6 > > +10805,18 @@ static void hsw_get_ddi_pll(struct drm_i915_private > > *dev_priv, enum port port, pipe_config->shared_dpll = > > intel_get_shared_dpll_by_id(dev_priv, id); } > > +static bool ehl_vbt_edp_dsi_present(struct drm_i915_private > > *dev_priv, > > + enum transcoder transcoder) > > +{ > > + bool edp_present = intel_bios_is_port_present(dev_priv, > > PORT_A); > > + bool dsi_present = intel_bios_is_dsi_present(dev_priv, > > NULL); + > > + if (IS_ELKHARTLAKE(dev_priv)) > > + return transcoder == TRANSCODER_EDP ? edp_present > > : dsi_present; + > > + return true; > > +} > > One of those things... this jumps out and immediately feels all wrong, > just like ehl_vbt_ddi_d_present() feels all wrong in > intel_combo_phy.c. But I don't know what would be the right thing to > do without spending time that I don't have on this. Is there a particular approach you want me to take to address this issue? All I am trying to do is address the plausible scenario(s) where the GOP/firmware may program the hardware in a certain way that seems incorrect from what i915 does based on the info in the VBT. I noticed this issue on the EHL board I am working on; therefore, I limited the fix to EHL only. Thanks, Vivek > > BR, > Jani. > > > > > + > > static bool hsw_get_transcoder_state(struct intel_crtc *crtc, > > struct intel_crtc_state > > *pipe_config, u64 *power_domain_mask, > > @@ -10844,7 +10856,8 @@ static bool hsw_get_transcoder_state(struct > > intel_crtc *crtc, > > tmp = intel_de_read(dev_priv, > > TRANS_DDI_FUNC_CTL(panel_transcoder)); > > - if (!(tmp & TRANS_DDI_FUNC_ENABLE)) > > + if (!(tmp & TRANS_DDI_FUNC_ENABLE) || > > + !ehl_vbt_edp_dsi_present(dev_priv, > > panel_transcoder)) continue; > > > > /* >
I guess a better solution would be do the HW state sanitization during the HW readout(intel_modeset_setup_hw_state()) On Tue, 2020-02-04 at 14:44 -0800, Vivek Kasireddy wrote: > On Tue, 4 Feb 2020 12:50:25 +0200 > Jani Nikula <jani.nikula@linux.intel.com> wrote: > Hi Jani, > > > On Mon, 03 Feb 2020, Vivek Kasireddy <vivek.kasireddy@intel.com> > > wrote: > > > Since the pipe->transcoder mapping is not expected to change > > > unless > > > there is either eDP or DSI connectors present, check the VBT to > > > confirm their presence in addition to checking > > > TRANS_DDI_FUNC_CTL(transcoder). This additional check is needed > > > on > > > platforms like Elkhart Lake because we cannot just rely on > > > GOP/Firmware programmed values in TRANS_DDI_FUNC_CTL(transcoder) > > > before updating the transcoder mapping. > > > > > > This patch is only relevant to EHL -- and a no-op on others -- > > > because some of the PHYs are shared between the different DDIs > > > and > > > we rely on the VBT to present the most accurate information to > > > the > > > driver. > > > > > > Cc: Matt Roper <matthew.d.roper@intel.com> > > > Cc: José Roberto de Souza <jose.souza@intel.com> > > > Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com> > > > --- > > > drivers/gpu/drm/i915/display/intel_display.c | 15 > > > ++++++++++++++- > > > 1 file changed, 14 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c > > > b/drivers/gpu/drm/i915/display/intel_display.c index > > > c0e5002ce64c..4b38f293bd88 100644 --- > > > a/drivers/gpu/drm/i915/display/intel_display.c +++ > > > b/drivers/gpu/drm/i915/display/intel_display.c @@ -10805,6 > > > +10805,18 @@ static void hsw_get_ddi_pll(struct drm_i915_private > > > *dev_priv, enum port port, pipe_config->shared_dpll = > > > intel_get_shared_dpll_by_id(dev_priv, id); } > > > +static bool ehl_vbt_edp_dsi_present(struct drm_i915_private > > > *dev_priv, > > > + enum transcoder transcoder) > > > +{ > > > + bool edp_present = intel_bios_is_port_present(dev_priv, > > > PORT_A); > > > + bool dsi_present = intel_bios_is_dsi_present(dev_priv, > > > NULL); + > > > + if (IS_ELKHARTLAKE(dev_priv)) > > > + return transcoder == TRANSCODER_EDP ? edp_present > > > : dsi_present; + > > > + return true; > > > +} > > > > One of those things... this jumps out and immediately feels all > > wrong, > > just like ehl_vbt_ddi_d_present() feels all wrong in > > intel_combo_phy.c. But I don't know what would be the right thing > > to > > do without spending time that I don't have on this. > > Is there a particular approach you want me to take to address this > issue? All I am trying to do is address the plausible scenario(s) > where > the GOP/firmware may program the hardware in a certain way that seems > incorrect from what i915 does based on the info in the VBT. I > noticed > this issue on the EHL board I am working on; therefore, I limited the > fix to EHL only. > > Thanks, > Vivek > > > BR, > > Jani. > > > > > > > > > + > > > static bool hsw_get_transcoder_state(struct intel_crtc *crtc, > > > struct intel_crtc_state > > > *pipe_config, u64 *power_domain_mask, > > > @@ -10844,7 +10856,8 @@ static bool > > > hsw_get_transcoder_state(struct > > > intel_crtc *crtc, > > > tmp = intel_de_read(dev_priv, > > > TRANS_DDI_FUNC_CTL(panel_transcoder > > > )); > > > - if (!(tmp & TRANS_DDI_FUNC_ENABLE)) > > > + if (!(tmp & TRANS_DDI_FUNC_ENABLE) || > > > + !ehl_vbt_edp_dsi_present(dev_priv, > > > panel_transcoder)) continue; > > > > > > /* > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c index c0e5002ce64c..4b38f293bd88 100644 --- a/drivers/gpu/drm/i915/display/intel_display.c +++ b/drivers/gpu/drm/i915/display/intel_display.c @@ -10805,6 +10805,18 @@ static void hsw_get_ddi_pll(struct drm_i915_private *dev_priv, enum port port, pipe_config->shared_dpll = intel_get_shared_dpll_by_id(dev_priv, id); } +static bool ehl_vbt_edp_dsi_present(struct drm_i915_private *dev_priv, + enum transcoder transcoder) +{ + bool edp_present = intel_bios_is_port_present(dev_priv, PORT_A); + bool dsi_present = intel_bios_is_dsi_present(dev_priv, NULL); + + if (IS_ELKHARTLAKE(dev_priv)) + return transcoder == TRANSCODER_EDP ? edp_present : dsi_present; + + return true; +} + static bool hsw_get_transcoder_state(struct intel_crtc *crtc, struct intel_crtc_state *pipe_config, u64 *power_domain_mask, @@ -10844,7 +10856,8 @@ static bool hsw_get_transcoder_state(struct intel_crtc *crtc, tmp = intel_de_read(dev_priv, TRANS_DDI_FUNC_CTL(panel_transcoder)); - if (!(tmp & TRANS_DDI_FUNC_ENABLE)) + if (!(tmp & TRANS_DDI_FUNC_ENABLE) || + !ehl_vbt_edp_dsi_present(dev_priv, panel_transcoder)) continue; /*
Since the pipe->transcoder mapping is not expected to change unless there is either eDP or DSI connectors present, check the VBT to confirm their presence in addition to checking TRANS_DDI_FUNC_CTL(transcoder). This additional check is needed on platforms like Elkhart Lake because we cannot just rely on GOP/Firmware programmed values in TRANS_DDI_FUNC_CTL(transcoder) before updating the transcoder mapping. This patch is only relevant to EHL -- and a no-op on others -- because some of the PHYs are shared between the different DDIs and we rely on the VBT to present the most accurate information to the driver. Cc: Matt Roper <matthew.d.roper@intel.com> Cc: José Roberto de Souza <jose.souza@intel.com> Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com> --- drivers/gpu/drm/i915/display/intel_display.c | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-)