| Message ID | 20210722232922.3796835-1-lucas.demarchi@intel.com |
|---|---|
| State | Accepted |
| Commit | b4bde5554f70fb04ff07989fdc1356ab84d6f482 |
| Headers | show |
| Series |
"drm/i915/display: split DISPLAY_VER 9 and 10 in intel_setup_outputs()"
( rev:
2
)
in
Intel GFX |
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c index c274bfb8e549..3f5383f3c744 100644 --- a/drivers/gpu/drm/i915/display/intel_display.c +++ b/drivers/gpu/drm/i915/display/intel_display.c @@ -11376,13 +11376,19 @@ 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) { + } else if (DISPLAY_VER(dev_priv) == 10) { 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 (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); } else if (HAS_DDI(dev_priv)) { u32 found;
On Thu, Jul 22, 2021 at 04:29:22PM -0700, Lucas De Marchi wrote: > Commit 5a9d38b20a5a ("drm/i915/display: hide workaround for broken vbt > in intel_bios.c") moved the workaround for broken or missing VBT to > intel_bios.c. However is_port_valid() only protects the handling of > different skus of the same display version. Since in > intel_setup_outputs() we share the code path with version 9, this would > also create port F for SKL/KBL, which does not exist. > > Missing VBT can be reproduced when starting a headless QEMU with no > opregion available. > > Avoid the issue by splitting versions 9 and 10 in intel_setup_outputs(), > which also makes it more clear what code path it's taking for each > version. Or we could just drop the PORT_F line. We've slowly been dropping bits and pieces of CNL support from the driver for a while now since all the hardware that came out had fused off graphics/display; that leaves GLK as the only real platform with display version 10, and it's already handled in its own condition branch above. Matt > > v2: move generic display version after Geminilake since that one has > a different set of outputs > > Fixes: 5a9d38b20a5a ("drm/i915/display: hide workaround for broken vbt in intel_bios.c") > Cc: Jani Nikula <jani.nikula@intel.com> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Reported-by: Christoph Hellwig <hch@infradead.org> > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > --- > drivers/gpu/drm/i915/display/intel_display.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > index c274bfb8e549..3f5383f3c744 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.c > +++ b/drivers/gpu/drm/i915/display/intel_display.c > @@ -11376,13 +11376,19 @@ 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) { > + } else if (DISPLAY_VER(dev_priv) == 10) { > 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 (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); > } else if (HAS_DDI(dev_priv)) { > u32 found; > > -- > 2.31.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, Jul 22, 2021 at 10:57:09PM -0700, Matt Roper wrote: >On Thu, Jul 22, 2021 at 04:29:22PM -0700, Lucas De Marchi wrote: >> Commit 5a9d38b20a5a ("drm/i915/display: hide workaround for broken vbt >> in intel_bios.c") moved the workaround for broken or missing VBT to >> intel_bios.c. However is_port_valid() only protects the handling of >> different skus of the same display version. Since in >> intel_setup_outputs() we share the code path with version 9, this would >> also create port F for SKL/KBL, which does not exist. >> >> Missing VBT can be reproduced when starting a headless QEMU with no >> opregion available. >> >> Avoid the issue by splitting versions 9 and 10 in intel_setup_outputs(), >> which also makes it more clear what code path it's taking for each >> version. > >Or we could just drop the PORT_F line. We've slowly been dropping bits >and pieces of CNL support from the driver for a while now since all the >hardware that came out had fused off graphics/display; that leaves GLK >as the only real platform with display version 10, and it's already >handled in its own condition branch above. no, that was my suggestion when I did this for the first time. Review from Ville last time was that we should either remove it completely or not at all, instead of dropping some pieces. At the time I started a series to remove it, but never completed. https://patchwork.freedesktop.org/patch/428168/?series=88988&rev=1#comment_768918 I will try to come back to that series again, but it's not something to go to -fixes, so I'd prefer to keep this patch. thanks Lucas De Marchi > > >Matt > >> >> v2: move generic display version after Geminilake since that one has >> a different set of outputs >> >> Fixes: 5a9d38b20a5a ("drm/i915/display: hide workaround for broken vbt in intel_bios.c") >> Cc: Jani Nikula <jani.nikula@intel.com> >> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> >> Reported-by: Christoph Hellwig <hch@infradead.org> >> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> >> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> >> --- >> drivers/gpu/drm/i915/display/intel_display.c | 8 +++++++- >> 1 file changed, 7 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c >> index c274bfb8e549..3f5383f3c744 100644 >> --- a/drivers/gpu/drm/i915/display/intel_display.c >> +++ b/drivers/gpu/drm/i915/display/intel_display.c >> @@ -11376,13 +11376,19 @@ 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) { >> + } else if (DISPLAY_VER(dev_priv) == 10) { >> 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 (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); >> } else if (HAS_DDI(dev_priv)) { >> u32 found; >> >> -- >> 2.31.1 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx > >-- >Matt Roper >Graphics Software Engineer >VTT-OSGC Platform Enablement >Intel Corporation >(916) 356-2795
On Fri, Jul 23, 2021 at 08:02:40AM -0700, Lucas De Marchi wrote: > On Thu, Jul 22, 2021 at 10:57:09PM -0700, Matt Roper wrote: > > On Thu, Jul 22, 2021 at 04:29:22PM -0700, Lucas De Marchi wrote: > > > Commit 5a9d38b20a5a ("drm/i915/display: hide workaround for broken vbt > > > in intel_bios.c") moved the workaround for broken or missing VBT to > > > intel_bios.c. However is_port_valid() only protects the handling of > > > different skus of the same display version. Since in > > > intel_setup_outputs() we share the code path with version 9, this would > > > also create port F for SKL/KBL, which does not exist. > > > > > > Missing VBT can be reproduced when starting a headless QEMU with no > > > opregion available. > > > > > > Avoid the issue by splitting versions 9 and 10 in intel_setup_outputs(), > > > which also makes it more clear what code path it's taking for each > > > version. > > > > Or we could just drop the PORT_F line. We've slowly been dropping bits > > and pieces of CNL support from the driver for a while now since all the > > hardware that came out had fused off graphics/display; that leaves GLK > > as the only real platform with display version 10, and it's already > > handled in its own condition branch above. > > no, that was my suggestion when I did this for the first time. Review > from Ville last time was that we should either remove it completely or > not at all, instead of dropping some pieces. At the time I started a Well, that ship has already sailed and we've already been slowly dropping pieces of CNL. Plus the entire platform will taint the kernel as 'unsupported preproduction hardware' if you try to load on it. But this patch is fine too. Reviewed-by: Matt Roper <matthew.d.roper@intel.com> > series to remove it, but never completed. > > https://patchwork.freedesktop.org/patch/428168/?series=88988&rev=1#comment_768918 > > I will try to come back to that series again, but it's not something to > go to -fixes, so I'd prefer to keep this patch. > > thanks > Lucas De Marchi > > > > > > > Matt > > > > > > > > v2: move generic display version after Geminilake since that one has > > > a different set of outputs > > > > > > Fixes: 5a9d38b20a5a ("drm/i915/display: hide workaround for broken vbt in intel_bios.c") > > > Cc: Jani Nikula <jani.nikula@intel.com> > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > Reported-by: Christoph Hellwig <hch@infradead.org> > > > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> > > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > --- > > > drivers/gpu/drm/i915/display/intel_display.c | 8 +++++++- > > > 1 file changed, 7 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > > > index c274bfb8e549..3f5383f3c744 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > > @@ -11376,13 +11376,19 @@ 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) { > > > + } else if (DISPLAY_VER(dev_priv) == 10) { > > > 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 (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); > > > } else if (HAS_DDI(dev_priv)) { > > > u32 found; > > > > > > -- > > > 2.31.1 > > > > > > _______________________________________________ > > > Intel-gfx mailing list > > > Intel-gfx@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > > Matt Roper > > Graphics Software Engineer > > VTT-OSGC Platform Enablement > > Intel Corporation > > (916) 356-2795
Commit 5a9d38b20a5a ("drm/i915/display: hide workaround for broken vbt in intel_bios.c") moved the workaround for broken or missing VBT to intel_bios.c. However is_port_valid() only protects the handling of different skus of the same display version. Since in intel_setup_outputs() we share the code path with version 9, this would also create port F for SKL/KBL, which does not exist. Missing VBT can be reproduced when starting a headless QEMU with no opregion available. Avoid the issue by splitting versions 9 and 10 in intel_setup_outputs(), which also makes it more clear what code path it's taking for each version. v2: move generic display version after Geminilake since that one has a different set of outputs Fixes: 5a9d38b20a5a ("drm/i915/display: hide workaround for broken vbt in intel_bios.c") Cc: Jani Nikula <jani.nikula@intel.com> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> Reported-by: Christoph Hellwig <hch@infradead.org> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> --- drivers/gpu/drm/i915/display/intel_display.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)