| Message ID | 20210413060927.114342-4-lucas.demarchi@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Simplify intel_setup_outputs | expand |
On Mon, 12 Apr 2021, Lucas De Marchi <lucas.demarchi@intel.com> wrote: > Direction on gen9+ was to stop reading the straps and only rely on the > VBT for marking the port presence. This happened while dealing with > WaIgnoreDDIAStrap and instead of using it as a WA, it should now be the > normal flow. See commit 885d3e5b6f08 ("drm/i915/display: fix comment on > skl straps"). > > For gen 10 it's hard to say if this will work or not since I can't test > it, so leave it with the same behavior as before. > > For PCH_TGP we should still rely on the VBT to make ports E and F not > available. > > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> > Reviewed-by: Anusha Srivatsa <anusha.srivatsa@intel.com> I think I'd get an ack from Ville for this. He has a knack for this stuff. BR, Jani. > --- > drivers/gpu/drm/i915/display/intel_display.c | 36 ++++++-------------- > 1 file changed, 11 insertions(+), 25 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > index d62ce9c87748..5a03cbba0280 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.c > +++ b/drivers/gpu/drm/i915/display/intel_display.c > @@ -10883,34 +10883,25 @@ static void intel_setup_outputs(struct drm_i915_private *dev_priv) > intel_ddi_init(dev_priv, PORT_B); > intel_ddi_init(dev_priv, PORT_C); > vlv_dsi_init(dev_priv); > + } else if (DISPLAY_VER(dev_priv) == 9) { > + intel_ddi_init(dev_priv, PORT_A); > + intel_ddi_init(dev_priv, PORT_B); > + intel_ddi_init(dev_priv, PORT_C); > + intel_ddi_init(dev_priv, PORT_D); > + intel_ddi_init(dev_priv, PORT_E); > + intel_ddi_init(dev_priv, PORT_F); > } else if (HAS_DDI(dev_priv)) { > - int found; > + u32 found; > > if (intel_ddi_crt_present(dev_priv)) > intel_crt_init(dev_priv); > > - /* > - * Haswell uses DDI functions to detect digital outputs. > - * On SKL pre-D0 the strap isn't connected. Later SKUs may or > - * may not have it - it was supposed to be fixed by the same > - * time we stopped using straps. Assume it's there. > - */ > + /* Haswell uses DDI functions to detect digital outputs. */ > found = intel_de_read(dev_priv, DDI_BUF_CTL(PORT_A)) & DDI_INIT_DISPLAY_DETECTED; > - /* WaIgnoreDDIAStrap: skl */ > - if (found || IS_DISPLAY_VER(dev_priv, 9)) > + if (found) > intel_ddi_init(dev_priv, PORT_A); > > - /* DDI B, C, D, and F detection is indicated by the SFUSE_STRAP > - * register */ > - if (HAS_PCH_TGP(dev_priv)) { > - /* W/A due to lack of STRAP config on TGP PCH*/ > - found = (SFUSE_STRAP_DDIB_DETECTED | > - SFUSE_STRAP_DDIC_DETECTED | > - SFUSE_STRAP_DDID_DETECTED); > - } else { > - found = intel_de_read(dev_priv, SFUSE_STRAP); > - } > - > + found = intel_de_read(dev_priv, SFUSE_STRAP); > if (found & SFUSE_STRAP_DDIB_DETECTED) > intel_ddi_init(dev_priv, PORT_B); > if (found & SFUSE_STRAP_DDIC_DETECTED) > @@ -10919,11 +10910,6 @@ static void intel_setup_outputs(struct drm_i915_private *dev_priv) > intel_ddi_init(dev_priv, PORT_D); > if (found & SFUSE_STRAP_DDIF_DETECTED) > intel_ddi_init(dev_priv, PORT_F); > - /* > - * On SKL we don't have a way to detect DDI-E so we rely on VBT. > - */ > - if (IS_DISPLAY_VER(dev_priv, 9) > - intel_ddi_init(dev_priv, PORT_E); > } else if (HAS_PCH_SPLIT(dev_priv)) { > int found;
On Mon, Apr 12, 2021 at 11:09:27PM -0700, Lucas De Marchi wrote: > Direction on gen9+ was to stop reading the straps and only rely on the > VBT for marking the port presence. This happened while dealing with > WaIgnoreDDIAStrap and instead of using it as a WA, it should now be the > normal flow. See commit 885d3e5b6f08 ("drm/i915/display: fix comment on > skl straps"). > > For gen 10 it's hard to say if this will work or not since I can't test > it, so leave it with the same behavior as before. > > For PCH_TGP we should still rely on the VBT to make ports E and F not > available. > > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> > Reviewed-by: Anusha Srivatsa <anusha.srivatsa@intel.com> > --- > drivers/gpu/drm/i915/display/intel_display.c | 36 ++++++-------------- > 1 file changed, 11 insertions(+), 25 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > index d62ce9c87748..5a03cbba0280 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.c > +++ b/drivers/gpu/drm/i915/display/intel_display.c > @@ -10883,34 +10883,25 @@ static void intel_setup_outputs(struct drm_i915_private *dev_priv) > intel_ddi_init(dev_priv, PORT_B); > intel_ddi_init(dev_priv, PORT_C); > vlv_dsi_init(dev_priv); > + } else if (DISPLAY_VER(dev_priv) == 9) { Should be >=10 I presume? Or did we want ot handle cnl along with icl perhaps? Doesn't really matter I suppose, but it's surely going to consfuse the me the next time I read this. > + intel_ddi_init(dev_priv, PORT_A); > + intel_ddi_init(dev_priv, PORT_B); > + intel_ddi_init(dev_priv, PORT_C); > + intel_ddi_init(dev_priv, PORT_D); > + intel_ddi_init(dev_priv, PORT_E); > + intel_ddi_init(dev_priv, PORT_F); DDI F isn't a thing on skl/derivatives, so I'd probably skip it on those. Could just use IS_CNL_WITH_PORT_F() to match the looks of the icl stuff. > } else if (HAS_DDI(dev_priv)) { > - int found; > + u32 found; > > if (intel_ddi_crt_present(dev_priv)) > intel_crt_init(dev_priv); > > - /* > - * Haswell uses DDI functions to detect digital outputs. > - * On SKL pre-D0 the strap isn't connected. Later SKUs may or > - * may not have it - it was supposed to be fixed by the same > - * time we stopped using straps. Assume it's there. > - */ > + /* Haswell uses DDI functions to detect digital outputs. */ > found = intel_de_read(dev_priv, DDI_BUF_CTL(PORT_A)) & DDI_INIT_DISPLAY_DETECTED; > - /* WaIgnoreDDIAStrap: skl */ > - if (found || IS_DISPLAY_VER(dev_priv, 9)) > + if (found) > intel_ddi_init(dev_priv, PORT_A); > > - /* DDI B, C, D, and F detection is indicated by the SFUSE_STRAP > - * register */ > - if (HAS_PCH_TGP(dev_priv)) { > - /* W/A due to lack of STRAP config on TGP PCH*/ > - found = (SFUSE_STRAP_DDIB_DETECTED | > - SFUSE_STRAP_DDIC_DETECTED | > - SFUSE_STRAP_DDID_DETECTED); > - } else { > - found = intel_de_read(dev_priv, SFUSE_STRAP); > - } > - > + found = intel_de_read(dev_priv, SFUSE_STRAP); > if (found & SFUSE_STRAP_DDIB_DETECTED) > intel_ddi_init(dev_priv, PORT_B); > if (found & SFUSE_STRAP_DDIC_DETECTED) > @@ -10919,11 +10910,6 @@ static void intel_setup_outputs(struct drm_i915_private *dev_priv) > intel_ddi_init(dev_priv, PORT_D); > if (found & SFUSE_STRAP_DDIF_DETECTED) > intel_ddi_init(dev_priv, PORT_F); > - /* > - * On SKL we don't have a way to detect DDI-E so we rely on VBT. > - */ > - if (IS_DISPLAY_VER(dev_priv, 9) > - intel_ddi_init(dev_priv, PORT_E); > } else if (HAS_PCH_SPLIT(dev_priv)) { > int found; > > -- > 2.31.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, Apr 13, 2021 at 06:45:16PM +0300, Ville Syrjälä wrote: >On Mon, Apr 12, 2021 at 11:09:27PM -0700, Lucas De Marchi wrote: >> Direction on gen9+ was to stop reading the straps and only rely on the >> VBT for marking the port presence. This happened while dealing with >> WaIgnoreDDIAStrap and instead of using it as a WA, it should now be the >> normal flow. See commit 885d3e5b6f08 ("drm/i915/display: fix comment on >> skl straps"). >> >> For gen 10 it's hard to say if this will work or not since I can't test >> it, so leave it with the same behavior as before. >> >> For PCH_TGP we should still rely on the VBT to make ports E and F not >> available. >> >> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> >> Reviewed-by: Anusha Srivatsa <anusha.srivatsa@intel.com> >> --- >> drivers/gpu/drm/i915/display/intel_display.c | 36 ++++++-------------- >> 1 file changed, 11 insertions(+), 25 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c >> index d62ce9c87748..5a03cbba0280 100644 >> --- a/drivers/gpu/drm/i915/display/intel_display.c >> +++ b/drivers/gpu/drm/i915/display/intel_display.c >> @@ -10883,34 +10883,25 @@ static void intel_setup_outputs(struct drm_i915_private *dev_priv) >> intel_ddi_init(dev_priv, PORT_B); >> intel_ddi_init(dev_priv, PORT_C); >> vlv_dsi_init(dev_priv); >> + } else if (DISPLAY_VER(dev_priv) == 9) { > >Should be >=10 I presume? Or did we want ot handle cnl along with why >= 10? The only DISPLAY_VER() == 10 platforms out there are handled in the branch above. I can make it >= 9, but not >= 10. Intention was to handle skl/kbl here. >icl perhaps? Doesn't really matter I suppose, but it's surely >going to consfuse the me the next time I read this. > >> + intel_ddi_init(dev_priv, PORT_A); >> + intel_ddi_init(dev_priv, PORT_B); >> + intel_ddi_init(dev_priv, PORT_C); >> + intel_ddi_init(dev_priv, PORT_D); >> + intel_ddi_init(dev_priv, PORT_E); >> + intel_ddi_init(dev_priv, PORT_F); > >DDI F isn't a thing on skl/derivatives, so I'd probably skip it on >those. Could just use IS_CNL_WITH_PORT_F() to match the looks of >the icl stuff. I was actually looking at ICL and thinking "shouldn't this hack for broken VBT be hidden in intel_bios.c?" I think we should trust what we parse from VBT everywhere except of course in intel_bios.c where we fixup when the VBT is wrong. Thoughts? Thanks Lucas De Marchi
On Tue, Apr 13, 2021 at 10:22:24AM -0700, Lucas De Marchi wrote: > On Tue, Apr 13, 2021 at 06:45:16PM +0300, Ville Syrjälä wrote: > >On Mon, Apr 12, 2021 at 11:09:27PM -0700, Lucas De Marchi wrote: > >> Direction on gen9+ was to stop reading the straps and only rely on the > >> VBT for marking the port presence. This happened while dealing with > >> WaIgnoreDDIAStrap and instead of using it as a WA, it should now be the > >> normal flow. See commit 885d3e5b6f08 ("drm/i915/display: fix comment on > >> skl straps"). > >> > >> For gen 10 it's hard to say if this will work or not since I can't test > >> it, so leave it with the same behavior as before. > >> > >> For PCH_TGP we should still rely on the VBT to make ports E and F not > >> available. > >> > >> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> > >> Reviewed-by: Anusha Srivatsa <anusha.srivatsa@intel.com> > >> --- > >> drivers/gpu/drm/i915/display/intel_display.c | 36 ++++++-------------- > >> 1 file changed, 11 insertions(+), 25 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > >> index d62ce9c87748..5a03cbba0280 100644 > >> --- a/drivers/gpu/drm/i915/display/intel_display.c > >> +++ b/drivers/gpu/drm/i915/display/intel_display.c > >> @@ -10883,34 +10883,25 @@ static void intel_setup_outputs(struct drm_i915_private *dev_priv) > >> intel_ddi_init(dev_priv, PORT_B); > >> intel_ddi_init(dev_priv, PORT_C); > >> vlv_dsi_init(dev_priv); > >> + } else if (DISPLAY_VER(dev_priv) == 9) { > > > >Should be >=10 I presume? Or did we want ot handle cnl along with > > why >= 10? The only DISPLAY_VER() == 10 platforms out there are handled > in the branch above. I can make it >= 9, but not >= 10. Intention was to > handle skl/kbl here. Yeah, meant to write >=9. Cnl not really a thing, but I would get confused if we started skipping it in some places while still handling it in others. I guess we may want to consider just nuking cnl totally everywhere, but until that time I think we should keep things consistent. > > > >icl perhaps? Doesn't really matter I suppose, but it's surely > >going to consfuse the me the next time I read this. > > > >> + intel_ddi_init(dev_priv, PORT_A); > >> + intel_ddi_init(dev_priv, PORT_B); > >> + intel_ddi_init(dev_priv, PORT_C); > >> + intel_ddi_init(dev_priv, PORT_D); > >> + intel_ddi_init(dev_priv, PORT_E); > >> + intel_ddi_init(dev_priv, PORT_F); > > > >DDI F isn't a thing on skl/derivatives, so I'd probably skip it on > >those. Could just use IS_CNL_WITH_PORT_F() to match the looks of > >the icl stuff. > > I was actually looking at ICL and thinking "shouldn't this hack for > broken VBT be hidden in intel_bios.c?" I think we should trust what we > parse from VBT everywhere except of course in intel_bios.c where we > fixup when the VBT is wrong. Thoughts? I guess we could stuff it all in there somehow. Not sure. Maybe Jani has thoughts on this?
On Tue, Apr 13, 2021 at 08:39:07PM +0300, Ville Syrjälä wrote: >On Tue, Apr 13, 2021 at 10:22:24AM -0700, Lucas De Marchi wrote: >> On Tue, Apr 13, 2021 at 06:45:16PM +0300, Ville Syrjälä wrote: >> >On Mon, Apr 12, 2021 at 11:09:27PM -0700, Lucas De Marchi wrote: >> >> Direction on gen9+ was to stop reading the straps and only rely on the >> >> VBT for marking the port presence. This happened while dealing with >> >> WaIgnoreDDIAStrap and instead of using it as a WA, it should now be the >> >> normal flow. See commit 885d3e5b6f08 ("drm/i915/display: fix comment on >> >> skl straps"). >> >> >> >> For gen 10 it's hard to say if this will work or not since I can't test >> >> it, so leave it with the same behavior as before. >> >> >> >> For PCH_TGP we should still rely on the VBT to make ports E and F not >> >> available. >> >> >> >> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> >> >> Reviewed-by: Anusha Srivatsa <anusha.srivatsa@intel.com> >> >> --- >> >> drivers/gpu/drm/i915/display/intel_display.c | 36 ++++++-------------- >> >> 1 file changed, 11 insertions(+), 25 deletions(-) >> >> >> >> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c >> >> index d62ce9c87748..5a03cbba0280 100644 >> >> --- a/drivers/gpu/drm/i915/display/intel_display.c >> >> +++ b/drivers/gpu/drm/i915/display/intel_display.c >> >> @@ -10883,34 +10883,25 @@ static void intel_setup_outputs(struct drm_i915_private *dev_priv) >> >> intel_ddi_init(dev_priv, PORT_B); >> >> intel_ddi_init(dev_priv, PORT_C); >> >> vlv_dsi_init(dev_priv); >> >> + } else if (DISPLAY_VER(dev_priv) == 9) { >> > >> >Should be >=10 I presume? Or did we want ot handle cnl along with >> >> why >= 10? The only DISPLAY_VER() == 10 platforms out there are handled >> in the branch above. I can make it >= 9, but not >= 10. Intention was to >> handle skl/kbl here. > >Yeah, meant to write >=9. Cnl not really a thing, but I would get >confused if we started skipping it in some places while still >handling it in others. I guess we may want to consider just nuking >cnl totally everywhere, but until that time I think we should keep >things consistent. considering mesa already did that, then yes. And agreed about being consistent while that doesn't happen. thanks Lucas De Marchi > >> >> >> >icl perhaps? Doesn't really matter I suppose, but it's surely >> >going to consfuse the me the next time I read this. >> > >> >> + intel_ddi_init(dev_priv, PORT_A); >> >> + intel_ddi_init(dev_priv, PORT_B); >> >> + intel_ddi_init(dev_priv, PORT_C); >> >> + intel_ddi_init(dev_priv, PORT_D); >> >> + intel_ddi_init(dev_priv, PORT_E); >> >> + intel_ddi_init(dev_priv, PORT_F); >> > >> >DDI F isn't a thing on skl/derivatives, so I'd probably skip it on >> >those. Could just use IS_CNL_WITH_PORT_F() to match the looks of >> >the icl stuff. >> >> I was actually looking at ICL and thinking "shouldn't this hack for >> broken VBT be hidden in intel_bios.c?" I think we should trust what we >> parse from VBT everywhere except of course in intel_bios.c where we >> fixup when the VBT is wrong. Thoughts? > >I guess we could stuff it all in there somehow. Not sure. >Maybe Jani has thoughts on this? > >-- >Ville Syrjälä >Intel
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c index d62ce9c87748..5a03cbba0280 100644 --- a/drivers/gpu/drm/i915/display/intel_display.c +++ b/drivers/gpu/drm/i915/display/intel_display.c @@ -10883,34 +10883,25 @@ static void intel_setup_outputs(struct drm_i915_private *dev_priv) intel_ddi_init(dev_priv, PORT_B); intel_ddi_init(dev_priv, PORT_C); vlv_dsi_init(dev_priv); + } else if (DISPLAY_VER(dev_priv) == 9) { + intel_ddi_init(dev_priv, PORT_A); + intel_ddi_init(dev_priv, PORT_B); + intel_ddi_init(dev_priv, PORT_C); + intel_ddi_init(dev_priv, PORT_D); + intel_ddi_init(dev_priv, PORT_E); + intel_ddi_init(dev_priv, PORT_F); } else if (HAS_DDI(dev_priv)) { - int found; + u32 found; if (intel_ddi_crt_present(dev_priv)) intel_crt_init(dev_priv); - /* - * Haswell uses DDI functions to detect digital outputs. - * On SKL pre-D0 the strap isn't connected. Later SKUs may or - * may not have it - it was supposed to be fixed by the same - * time we stopped using straps. Assume it's there. - */ + /* Haswell uses DDI functions to detect digital outputs. */ found = intel_de_read(dev_priv, DDI_BUF_CTL(PORT_A)) & DDI_INIT_DISPLAY_DETECTED; - /* WaIgnoreDDIAStrap: skl */ - if (found || IS_DISPLAY_VER(dev_priv, 9)) + if (found) intel_ddi_init(dev_priv, PORT_A); - /* DDI B, C, D, and F detection is indicated by the SFUSE_STRAP - * register */ - if (HAS_PCH_TGP(dev_priv)) { - /* W/A due to lack of STRAP config on TGP PCH*/ - found = (SFUSE_STRAP_DDIB_DETECTED | - SFUSE_STRAP_DDIC_DETECTED | - SFUSE_STRAP_DDID_DETECTED); - } else { - found = intel_de_read(dev_priv, SFUSE_STRAP); - } - + found = intel_de_read(dev_priv, SFUSE_STRAP); if (found & SFUSE_STRAP_DDIB_DETECTED) intel_ddi_init(dev_priv, PORT_B); if (found & SFUSE_STRAP_DDIC_DETECTED) @@ -10919,11 +10910,6 @@ static void intel_setup_outputs(struct drm_i915_private *dev_priv) intel_ddi_init(dev_priv, PORT_D); if (found & SFUSE_STRAP_DDIF_DETECTED) intel_ddi_init(dev_priv, PORT_F); - /* - * On SKL we don't have a way to detect DDI-E so we rely on VBT. - */ - if (IS_DISPLAY_VER(dev_priv, 9) - intel_ddi_init(dev_priv, PORT_E); } else if (HAS_PCH_SPLIT(dev_priv)) { int found;
Direction on gen9+ was to stop reading the straps and only rely on the VBT for marking the port presence. This happened while dealing with WaIgnoreDDIAStrap and instead of using it as a WA, it should now be the normal flow. See commit 885d3e5b6f08 ("drm/i915/display: fix comment on skl straps"). For gen 10 it's hard to say if this will work or not since I can't test it, so leave it with the same behavior as before. For PCH_TGP we should still rely on the VBT to make ports E and F not available. Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> Reviewed-by: Anusha Srivatsa <anusha.srivatsa@intel.com> --- drivers/gpu/drm/i915/display/intel_display.c | 36 ++++++-------------- 1 file changed, 11 insertions(+), 25 deletions(-)