| Message ID | 20211217155403.31477-3-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Extend parse_ddi_port() to all g4x+ platforms | expand |
On Fri, 17 Dec 2021, Ville Syrjala <ville.syrjala@linux.intel.com> wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > CHV is currently straddling the divide by using parse_ddi_ports() stuff > for aux_ch/ddc_pin but going through all old codepaths for the rest > (intel_bios_is_port_present(), intel_bios_is_port_edp(), > intel_bios_is_port_dp_dual_mode()). Let's switch over full and use > i915->vbt.ports[] for the rest of the stuff. Whoa, this is far more subtle than what the code looks! We stop checking for port A for CHV in intel_bios_is_port_present(), but it's a warn and I don't recall any bug reports, so probably fine. We could add a check in parse_ddi_port(), but meh. Ditto for intel_bios_is_port_dp_dual_mode(), except it doesn't have a warn. The eDP check in intel_bios_is_port_edp() becomes slightly more relaxed. Both the old and new check require these to be set: - DEVICE_TYPE_DISPLAYPORT_OUTPUT - DEVICE_TYPE_INTERNAL_CONNECTOR. The old code also required these to be unset: - DEVICE_TYPE_MIPI_OUTPUT - DEVICE_TYPE_COMPOSITE_OUTPUT - DEVICE_TYPE_DUAL_CHANNEL - DEVICE_TYPE_LVDS_SIGNALING - DEVICE_TYPE_TMDS_DVI_SIGNALING - DEVICE_TYPE_VIDEO_SIGNALING - DEVICE_TYPE_ANALOG_OUTPUT It's possible we've added these just as a sanity check for broken VBTs more than anything. I guess I'd see if actual problems arise. Bottom line, I think the functional changes matter only for VBTs with bogus data. Reviewed-by: Jani Nikula <jani.nikula@intel.com> > > dvo_port_to_port() doesn't know about DSI so we won't get into > any kind of "is port B HDMI or DSI or both?" conundrum, which > could otherwise happen on VLV/CHV due to DSI ports living in a > separate world from the other digital ports. > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/display/intel_bios.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c > index fce1ea7a6693..b7adea9827c3 100644 > --- a/drivers/gpu/drm/i915/display/intel_bios.c > +++ b/drivers/gpu/drm/i915/display/intel_bios.c > @@ -2075,14 +2075,14 @@ static void parse_ddi_port(struct drm_i915_private *i915, > > static bool has_ddi_port_info(struct drm_i915_private *i915) > { > - return HAS_DDI(i915); > + return HAS_DDI(i915) || IS_CHERRYVIEW(i915); > } > > static void parse_ddi_ports(struct drm_i915_private *i915) > { > struct intel_bios_encoder_data *devdata; > > - if (!HAS_DDI(i915) && !IS_CHERRYVIEW(i915)) > + if (!has_ddi_port_info(i915)) > return; > > if (i915->vbt.version < 155)
On Fri, 17 Dec 2021, Ville Syrjala <ville.syrjala@linux.intel.com> wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > CHV is currently straddling the divide by using parse_ddi_ports() stuff > for aux_ch/ddc_pin but going through all old codepaths for the rest > (intel_bios_is_port_present(), intel_bios_is_port_edp(), > intel_bios_is_port_dp_dual_mode()). Let's switch over full and use > i915->vbt.ports[] for the rest of the stuff. > > dvo_port_to_port() doesn't know about DSI so we won't get into > any kind of "is port B HDMI or DSI or both?" conundrum, which > could otherwise happen on VLV/CHV due to DSI ports living in a > separate world from the other digital ports. This is also very subtle! But it does somewhat nicely dodge the whole question, and moves things forward. > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/display/intel_bios.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c > index fce1ea7a6693..b7adea9827c3 100644 > --- a/drivers/gpu/drm/i915/display/intel_bios.c > +++ b/drivers/gpu/drm/i915/display/intel_bios.c > @@ -2075,14 +2075,14 @@ static void parse_ddi_port(struct drm_i915_private *i915, > > static bool has_ddi_port_info(struct drm_i915_private *i915) > { > - return HAS_DDI(i915); > + return HAS_DDI(i915) || IS_CHERRYVIEW(i915); > } > > static void parse_ddi_ports(struct drm_i915_private *i915) > { > struct intel_bios_encoder_data *devdata; > > - if (!HAS_DDI(i915) && !IS_CHERRYVIEW(i915)) > + if (!has_ddi_port_info(i915)) > return; > > if (i915->vbt.version < 155)
On Wed, Dec 22, 2021 at 11:05:50AM +0200, Jani Nikula wrote: > On Fri, 17 Dec 2021, Ville Syrjala <ville.syrjala@linux.intel.com> wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > CHV is currently straddling the divide by using parse_ddi_ports() stuff > > for aux_ch/ddc_pin but going through all old codepaths for the rest > > (intel_bios_is_port_present(), intel_bios_is_port_edp(), > > intel_bios_is_port_dp_dual_mode()). Let's switch over full and use > > i915->vbt.ports[] for the rest of the stuff. > > Whoa, this is far more subtle than what the code looks! > > We stop checking for port A for CHV in intel_bios_is_port_present(), but > it's a warn and I don't recall any bug reports, so probably fine. We > could add a check in parse_ddi_port(), but meh. > > Ditto for intel_bios_is_port_dp_dual_mode(), except it doesn't have a > warn. > > The eDP check in intel_bios_is_port_edp() becomes slightly more > relaxed. Both the old and new check require these to be set: > > - DEVICE_TYPE_DISPLAYPORT_OUTPUT > - DEVICE_TYPE_INTERNAL_CONNECTOR. > > The old code also required these to be unset: > > - DEVICE_TYPE_MIPI_OUTPUT > - DEVICE_TYPE_COMPOSITE_OUTPUT > - DEVICE_TYPE_DUAL_CHANNEL > - DEVICE_TYPE_LVDS_SIGNALING > - DEVICE_TYPE_TMDS_DVI_SIGNALING > - DEVICE_TYPE_VIDEO_SIGNALING > - DEVICE_TYPE_ANALOG_OUTPUT > > It's possible we've added these just as a sanity check for broken VBTs > more than anything. I guess I'd see if actual problems arise. > > Bottom line, I think the functional changes matter only for VBTs with > bogus data. Yeah, I should probably include a bit more of thse details in this commit message as well to make it easier to figure out what actually changed if we get a regressoion. > Reviewed-by: Jani Nikula <jani.nikula@intel.com> Thanks.
diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c index fce1ea7a6693..b7adea9827c3 100644 --- a/drivers/gpu/drm/i915/display/intel_bios.c +++ b/drivers/gpu/drm/i915/display/intel_bios.c @@ -2075,14 +2075,14 @@ static void parse_ddi_port(struct drm_i915_private *i915, static bool has_ddi_port_info(struct drm_i915_private *i915) { - return HAS_DDI(i915); + return HAS_DDI(i915) || IS_CHERRYVIEW(i915); } static void parse_ddi_ports(struct drm_i915_private *i915) { struct intel_bios_encoder_data *devdata; - if (!HAS_DDI(i915) && !IS_CHERRYVIEW(i915)) + if (!has_ddi_port_info(i915)) return; if (i915->vbt.version < 155)