| Message ID | 20fb913a93c60fd24fc51b441ccea7bc75e82dd1.1665496046.git.jani.nikula@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/edid: EDID override refactoring and fixes | expand |
On Tue, Oct 11, 2022 at 04:49:35PM +0300, Jani Nikula wrote: > For normal connector detect, there's really no point in trying dual mode > detect if the connector is disconnected. We can simplify the detect > sequence by skipping it. Since intel_hdmi_dp_dual_mode_detect() is only > called when EDID is present, we can drop the has_edid parameter. > > The functional effect is speeding up disconnected connector detection > ever so slightly, and, combined with firmware EDID, also stop logging > about assuming dual mode adaptor. > > It's a bit subtle, but this will also skip dual mode detect if the > connector is force connected and a) there's no EDID of any kind, normal > or override/firmare or b) there's EDID but it does not indicate > digital. These are corner cases no matter what, and arguably forcing > should not be limited by dual mode detect. > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Signed-off-by: Jani Nikula <jani.nikula@intel.com> > --- > drivers/gpu/drm/i915/display/intel_hdmi.c | 17 +++++++---------- > 1 file changed, 7 insertions(+), 10 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c > index 93519fb23d9d..a332eaac86cd 100644 > --- a/drivers/gpu/drm/i915/display/intel_hdmi.c > +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c > @@ -2355,7 +2355,7 @@ intel_hdmi_unset_edid(struct drm_connector *connector) > } > > static void > -intel_hdmi_dp_dual_mode_detect(struct drm_connector *connector, bool has_edid) > +intel_hdmi_dp_dual_mode_detect(struct drm_connector *connector) > { > struct drm_i915_private *dev_priv = to_i915(connector->dev); > struct intel_hdmi *hdmi = intel_attached_hdmi(to_intel_connector(connector)); > @@ -2371,16 +2371,13 @@ intel_hdmi_dp_dual_mode_detect(struct drm_connector *connector, bool has_edid) > * CONFIG1 pin, but no such luck on our hardware. > * > * The only method left to us is to check the VBT to see > - * if the port is a dual mode capable DP port. But let's > - * only do that when we sucesfully read the EDID, to avoid > - * confusing log messages about DP dual mode adaptors when > - * there's nothing connected to the port. > + * if the port is a dual mode capable DP port. > */ > if (type == DRM_DP_DUAL_MODE_UNKNOWN) { > /* An overridden EDID imply that we want this port for testing. > * Make sure not to set limits for that port. > */ > - if (has_edid && !connector->override_edid && > + if (!connector->override_edid && > intel_bios_is_port_dp_dual_mode(dev_priv, port)) { > drm_dbg_kms(&dev_priv->drm, > "Assuming DP dual mode adaptor presence based on VBT\n"); > @@ -2435,18 +2432,18 @@ intel_hdmi_set_edid(struct drm_connector *connector) > intel_gmbus_force_bit(i2c, false); > } > > - intel_hdmi_dp_dual_mode_detect(connector, edid != NULL); > - > - intel_display_power_put(dev_priv, POWER_DOMAIN_GMBUS, wakeref); > - > to_intel_connector(connector)->detect_edid = edid; > if (edid && edid->input & DRM_EDID_INPUT_DIGITAL) { We didn't have this digital input thing before. What happens with HDMI->VGA dongles for example? Hmm. This whole thing might already be broken on those. I suspect I've only used my HDMI->VGA dongle on LSPCON machines, so never noticed this. Need to go plug that thing into a native HDMI port... > intel_hdmi->has_audio = drm_detect_monitor_audio(edid); > intel_hdmi->has_hdmi_sink = drm_detect_hdmi_monitor(edid); > > + intel_hdmi_dp_dual_mode_detect(connector); > + > connected = true; > } > > + intel_display_power_put(dev_priv, POWER_DOMAIN_GMBUS, wakeref); > + > cec_notifier_set_phys_addr_from_edid(intel_hdmi->cec_notifier, edid); > > return connected; > -- > 2.34.1
On Thu, Oct 13, 2022 at 09:41:21PM +0300, Ville Syrjälä wrote: > On Tue, Oct 11, 2022 at 04:49:35PM +0300, Jani Nikula wrote: > > For normal connector detect, there's really no point in trying dual mode > > detect if the connector is disconnected. We can simplify the detect > > sequence by skipping it. Since intel_hdmi_dp_dual_mode_detect() is only > > called when EDID is present, we can drop the has_edid parameter. > > > > The functional effect is speeding up disconnected connector detection > > ever so slightly, and, combined with firmware EDID, also stop logging > > about assuming dual mode adaptor. > > > > It's a bit subtle, but this will also skip dual mode detect if the > > connector is force connected and a) there's no EDID of any kind, normal > > or override/firmare or b) there's EDID but it does not indicate > > digital. These are corner cases no matter what, and arguably forcing > > should not be limited by dual mode detect. > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Signed-off-by: Jani Nikula <jani.nikula@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_hdmi.c | 17 +++++++---------- > > 1 file changed, 7 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c > > index 93519fb23d9d..a332eaac86cd 100644 > > --- a/drivers/gpu/drm/i915/display/intel_hdmi.c > > +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c > > @@ -2355,7 +2355,7 @@ intel_hdmi_unset_edid(struct drm_connector *connector) > > } > > > > static void > > -intel_hdmi_dp_dual_mode_detect(struct drm_connector *connector, bool has_edid) > > +intel_hdmi_dp_dual_mode_detect(struct drm_connector *connector) > > { > > struct drm_i915_private *dev_priv = to_i915(connector->dev); > > struct intel_hdmi *hdmi = intel_attached_hdmi(to_intel_connector(connector)); > > @@ -2371,16 +2371,13 @@ intel_hdmi_dp_dual_mode_detect(struct drm_connector *connector, bool has_edid) > > * CONFIG1 pin, but no such luck on our hardware. > > * > > * The only method left to us is to check the VBT to see > > - * if the port is a dual mode capable DP port. But let's > > - * only do that when we sucesfully read the EDID, to avoid > > - * confusing log messages about DP dual mode adaptors when > > - * there's nothing connected to the port. > > + * if the port is a dual mode capable DP port. > > */ > > if (type == DRM_DP_DUAL_MODE_UNKNOWN) { > > /* An overridden EDID imply that we want this port for testing. > > * Make sure not to set limits for that port. > > */ > > - if (has_edid && !connector->override_edid && > > + if (!connector->override_edid && > > intel_bios_is_port_dp_dual_mode(dev_priv, port)) { > > drm_dbg_kms(&dev_priv->drm, > > "Assuming DP dual mode adaptor presence based on VBT\n"); > > @@ -2435,18 +2432,18 @@ intel_hdmi_set_edid(struct drm_connector *connector) > > intel_gmbus_force_bit(i2c, false); > > } > > > > - intel_hdmi_dp_dual_mode_detect(connector, edid != NULL); > > - > > - intel_display_power_put(dev_priv, POWER_DOMAIN_GMBUS, wakeref); > > - > > to_intel_connector(connector)->detect_edid = edid; > > if (edid && edid->input & DRM_EDID_INPUT_DIGITAL) { > > We didn't have this digital input thing before. What happens with > HDMI->VGA dongles for example? > > Hmm. This whole thing might already be broken on those. I suspect > I've only used my HDMI->VGA dongle on LSPCON machines, so never > noticed this. Need to go plug that thing into a native HDMI port... Except I must have left it elsewhere since I can't find it here. So can't test right now unfortunately. I first thought this digital check thing might be due to the DVI-I shenanigans in intel_crt_detect_ddc(), but that was added for am unspecified gen2 machine in commit f5afcd3dd0dc ("drm/i915/crt: Check for a analog monitor in case of DVI-I") so not even relevant here. And I don't think I've ever seen a g4x+ machine with an actual DVI-I port. commit aa93d632c496 ("drm/i915: Require digital monitor on HDMI ports for detect") is where this check was added, but there is no actual justification for checking the digital thing vs. just making sure the edid read succeeded. So looks to me like this check can just be removed. And if we do come across some real DVI-I use cases we should probably check the VBT DDC pin assignments before we go assuming anything about the wiring.
On Thu, 13 Oct 2022, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Thu, Oct 13, 2022 at 09:41:21PM +0300, Ville Syrjälä wrote: >> On Tue, Oct 11, 2022 at 04:49:35PM +0300, Jani Nikula wrote: >> > For normal connector detect, there's really no point in trying dual mode >> > detect if the connector is disconnected. We can simplify the detect >> > sequence by skipping it. Since intel_hdmi_dp_dual_mode_detect() is only >> > called when EDID is present, we can drop the has_edid parameter. >> > >> > The functional effect is speeding up disconnected connector detection >> > ever so slightly, and, combined with firmware EDID, also stop logging >> > about assuming dual mode adaptor. >> > >> > It's a bit subtle, but this will also skip dual mode detect if the >> > connector is force connected and a) there's no EDID of any kind, normal >> > or override/firmare or b) there's EDID but it does not indicate >> > digital. These are corner cases no matter what, and arguably forcing >> > should not be limited by dual mode detect. >> > >> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> >> > Signed-off-by: Jani Nikula <jani.nikula@intel.com> >> > --- >> > drivers/gpu/drm/i915/display/intel_hdmi.c | 17 +++++++---------- >> > 1 file changed, 7 insertions(+), 10 deletions(-) >> > >> > diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c >> > index 93519fb23d9d..a332eaac86cd 100644 >> > --- a/drivers/gpu/drm/i915/display/intel_hdmi.c >> > +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c >> > @@ -2355,7 +2355,7 @@ intel_hdmi_unset_edid(struct drm_connector *connector) >> > } >> > >> > static void >> > -intel_hdmi_dp_dual_mode_detect(struct drm_connector *connector, bool has_edid) >> > +intel_hdmi_dp_dual_mode_detect(struct drm_connector *connector) >> > { >> > struct drm_i915_private *dev_priv = to_i915(connector->dev); >> > struct intel_hdmi *hdmi = intel_attached_hdmi(to_intel_connector(connector)); >> > @@ -2371,16 +2371,13 @@ intel_hdmi_dp_dual_mode_detect(struct drm_connector *connector, bool has_edid) >> > * CONFIG1 pin, but no such luck on our hardware. >> > * >> > * The only method left to us is to check the VBT to see >> > - * if the port is a dual mode capable DP port. But let's >> > - * only do that when we sucesfully read the EDID, to avoid >> > - * confusing log messages about DP dual mode adaptors when >> > - * there's nothing connected to the port. >> > + * if the port is a dual mode capable DP port. >> > */ >> > if (type == DRM_DP_DUAL_MODE_UNKNOWN) { >> > /* An overridden EDID imply that we want this port for testing. >> > * Make sure not to set limits for that port. >> > */ >> > - if (has_edid && !connector->override_edid && >> > + if (!connector->override_edid && >> > intel_bios_is_port_dp_dual_mode(dev_priv, port)) { >> > drm_dbg_kms(&dev_priv->drm, >> > "Assuming DP dual mode adaptor presence based on VBT\n"); >> > @@ -2435,18 +2432,18 @@ intel_hdmi_set_edid(struct drm_connector *connector) >> > intel_gmbus_force_bit(i2c, false); >> > } >> > >> > - intel_hdmi_dp_dual_mode_detect(connector, edid != NULL); >> > - >> > - intel_display_power_put(dev_priv, POWER_DOMAIN_GMBUS, wakeref); >> > - >> > to_intel_connector(connector)->detect_edid = edid; >> > if (edid && edid->input & DRM_EDID_INPUT_DIGITAL) { >> >> We didn't have this digital input thing before. What happens with >> HDMI->VGA dongles for example? >> >> Hmm. This whole thing might already be broken on those. I suspect >> I've only used my HDMI->VGA dongle on LSPCON machines, so never >> noticed this. Need to go plug that thing into a native HDMI port... > > Except I must have left it elsewhere since I can't find it here. > So can't test right now unfortunately. > > I first thought this digital check thing might be due to > the DVI-I shenanigans in intel_crt_detect_ddc(), but that > was added for am unspecified gen2 machine in commit f5afcd3dd0dc > ("drm/i915/crt: Check for a analog monitor in case of DVI-I") > so not even relevant here. And I don't think I've ever seen > a g4x+ machine with an actual DVI-I port. > > commit aa93d632c496 ("drm/i915: Require digital monitor > on HDMI ports for detect") is where this check was added, > but there is no actual justification for checking the > digital thing vs. just making sure the edid read succeeded. > > So looks to me like this check can just be removed. And > if we do come across some real DVI-I use cases we should > probably check the VBT DDC pin assignments before we go > assuming anything about the wiring. Are you saying remove the "edid->input & DRM_EDID_INPUT_DIGITAL" altogether? Or turn this into: if (edid) { if (edid->input & DRM_EDID_INPUT_DIGITAL) { intel_hdmi->has_audio = drm_detect_monitor_audio(edid); intel_hdmi->has_hdmi_sink = drm_detect_hdmi_monitor(edid); } connected = true; } Since e.g. DP wraps the audio/hdmi detect calls in digital check. OTOH I really want to get rid of the detect audio/hdmi calls [1]. Just a lot of old cruft and the rabbit hole gets deeper. :( BR, Jani. [1] https://patchwork.freedesktop.org/series/108024/
On Fri, Oct 14, 2022 at 11:14:43AM +0300, Jani Nikula wrote: > On Thu, 13 Oct 2022, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > > On Thu, Oct 13, 2022 at 09:41:21PM +0300, Ville Syrjälä wrote: > >> On Tue, Oct 11, 2022 at 04:49:35PM +0300, Jani Nikula wrote: > >> > For normal connector detect, there's really no point in trying dual mode > >> > detect if the connector is disconnected. We can simplify the detect > >> > sequence by skipping it. Since intel_hdmi_dp_dual_mode_detect() is only > >> > called when EDID is present, we can drop the has_edid parameter. > >> > > >> > The functional effect is speeding up disconnected connector detection > >> > ever so slightly, and, combined with firmware EDID, also stop logging > >> > about assuming dual mode adaptor. > >> > > >> > It's a bit subtle, but this will also skip dual mode detect if the > >> > connector is force connected and a) there's no EDID of any kind, normal > >> > or override/firmare or b) there's EDID but it does not indicate > >> > digital. These are corner cases no matter what, and arguably forcing > >> > should not be limited by dual mode detect. > >> > > >> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > >> > Signed-off-by: Jani Nikula <jani.nikula@intel.com> > >> > --- > >> > drivers/gpu/drm/i915/display/intel_hdmi.c | 17 +++++++---------- > >> > 1 file changed, 7 insertions(+), 10 deletions(-) > >> > > >> > diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c > >> > index 93519fb23d9d..a332eaac86cd 100644 > >> > --- a/drivers/gpu/drm/i915/display/intel_hdmi.c > >> > +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c > >> > @@ -2355,7 +2355,7 @@ intel_hdmi_unset_edid(struct drm_connector *connector) > >> > } > >> > > >> > static void > >> > -intel_hdmi_dp_dual_mode_detect(struct drm_connector *connector, bool has_edid) > >> > +intel_hdmi_dp_dual_mode_detect(struct drm_connector *connector) > >> > { > >> > struct drm_i915_private *dev_priv = to_i915(connector->dev); > >> > struct intel_hdmi *hdmi = intel_attached_hdmi(to_intel_connector(connector)); > >> > @@ -2371,16 +2371,13 @@ intel_hdmi_dp_dual_mode_detect(struct drm_connector *connector, bool has_edid) > >> > * CONFIG1 pin, but no such luck on our hardware. > >> > * > >> > * The only method left to us is to check the VBT to see > >> > - * if the port is a dual mode capable DP port. But let's > >> > - * only do that when we sucesfully read the EDID, to avoid > >> > - * confusing log messages about DP dual mode adaptors when > >> > - * there's nothing connected to the port. > >> > + * if the port is a dual mode capable DP port. > >> > */ > >> > if (type == DRM_DP_DUAL_MODE_UNKNOWN) { > >> > /* An overridden EDID imply that we want this port for testing. > >> > * Make sure not to set limits for that port. > >> > */ > >> > - if (has_edid && !connector->override_edid && > >> > + if (!connector->override_edid && > >> > intel_bios_is_port_dp_dual_mode(dev_priv, port)) { > >> > drm_dbg_kms(&dev_priv->drm, > >> > "Assuming DP dual mode adaptor presence based on VBT\n"); > >> > @@ -2435,18 +2432,18 @@ intel_hdmi_set_edid(struct drm_connector *connector) > >> > intel_gmbus_force_bit(i2c, false); > >> > } > >> > > >> > - intel_hdmi_dp_dual_mode_detect(connector, edid != NULL); > >> > - > >> > - intel_display_power_put(dev_priv, POWER_DOMAIN_GMBUS, wakeref); > >> > - > >> > to_intel_connector(connector)->detect_edid = edid; > >> > if (edid && edid->input & DRM_EDID_INPUT_DIGITAL) { > >> > >> We didn't have this digital input thing before. What happens with > >> HDMI->VGA dongles for example? > >> > >> Hmm. This whole thing might already be broken on those. I suspect > >> I've only used my HDMI->VGA dongle on LSPCON machines, so never > >> noticed this. Need to go plug that thing into a native HDMI port... > > > > Except I must have left it elsewhere since I can't find it here. > > So can't test right now unfortunately. > > > > I first thought this digital check thing might be due to > > the DVI-I shenanigans in intel_crt_detect_ddc(), but that > > was added for am unspecified gen2 machine in commit f5afcd3dd0dc > > ("drm/i915/crt: Check for a analog monitor in case of DVI-I") > > so not even relevant here. And I don't think I've ever seen > > a g4x+ machine with an actual DVI-I port. > > > > commit aa93d632c496 ("drm/i915: Require digital monitor > > on HDMI ports for detect") is where this check was added, > > but there is no actual justification for checking the > > digital thing vs. just making sure the edid read succeeded. > > > > So looks to me like this check can just be removed. And > > if we do come across some real DVI-I use cases we should > > probably check the VBT DDC pin assignments before we go > > assuming anything about the wiring. > > Are you saying remove the "edid->input & DRM_EDID_INPUT_DIGITAL" > altogether? Or turn this into: > > if (edid) { > if (edid->input & DRM_EDID_INPUT_DIGITAL) { > intel_hdmi->has_audio = drm_detect_monitor_audio(edid); > intel_hdmi->has_hdmi_sink = drm_detect_hdmi_monitor(edid); > } > connected = true; > } > > Since e.g. DP wraps the audio/hdmi detect calls in digital check. I'm thinking they should just all go. But I guess that's a separate topic for the most part.
On Thu, Oct 13, 2022 at 10:24:54PM +0300, Ville Syrjälä wrote: > On Thu, Oct 13, 2022 at 09:41:21PM +0300, Ville Syrjälä wrote: > > On Tue, Oct 11, 2022 at 04:49:35PM +0300, Jani Nikula wrote: > > > For normal connector detect, there's really no point in trying dual mode > > > detect if the connector is disconnected. We can simplify the detect > > > sequence by skipping it. Since intel_hdmi_dp_dual_mode_detect() is only > > > called when EDID is present, we can drop the has_edid parameter. > > > > > > The functional effect is speeding up disconnected connector detection > > > ever so slightly, and, combined with firmware EDID, also stop logging > > > about assuming dual mode adaptor. > > > > > > It's a bit subtle, but this will also skip dual mode detect if the > > > connector is force connected and a) there's no EDID of any kind, normal > > > or override/firmare or b) there's EDID but it does not indicate > > > digital. These are corner cases no matter what, and arguably forcing > > > should not be limited by dual mode detect. > > > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > Signed-off-by: Jani Nikula <jani.nikula@intel.com> > > > --- > > > drivers/gpu/drm/i915/display/intel_hdmi.c | 17 +++++++---------- > > > 1 file changed, 7 insertions(+), 10 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c > > > index 93519fb23d9d..a332eaac86cd 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_hdmi.c > > > +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c > > > @@ -2355,7 +2355,7 @@ intel_hdmi_unset_edid(struct drm_connector *connector) > > > } > > > > > > static void > > > -intel_hdmi_dp_dual_mode_detect(struct drm_connector *connector, bool has_edid) > > > +intel_hdmi_dp_dual_mode_detect(struct drm_connector *connector) > > > { > > > struct drm_i915_private *dev_priv = to_i915(connector->dev); > > > struct intel_hdmi *hdmi = intel_attached_hdmi(to_intel_connector(connector)); > > > @@ -2371,16 +2371,13 @@ intel_hdmi_dp_dual_mode_detect(struct drm_connector *connector, bool has_edid) > > > * CONFIG1 pin, but no such luck on our hardware. > > > * > > > * The only method left to us is to check the VBT to see > > > - * if the port is a dual mode capable DP port. But let's > > > - * only do that when we sucesfully read the EDID, to avoid > > > - * confusing log messages about DP dual mode adaptors when > > > - * there's nothing connected to the port. > > > + * if the port is a dual mode capable DP port. > > > */ > > > if (type == DRM_DP_DUAL_MODE_UNKNOWN) { > > > /* An overridden EDID imply that we want this port for testing. > > > * Make sure not to set limits for that port. > > > */ > > > - if (has_edid && !connector->override_edid && > > > + if (!connector->override_edid && > > > intel_bios_is_port_dp_dual_mode(dev_priv, port)) { > > > drm_dbg_kms(&dev_priv->drm, > > > "Assuming DP dual mode adaptor presence based on VBT\n"); > > > @@ -2435,18 +2432,18 @@ intel_hdmi_set_edid(struct drm_connector *connector) > > > intel_gmbus_force_bit(i2c, false); > > > } > > > > > > - intel_hdmi_dp_dual_mode_detect(connector, edid != NULL); > > > - > > > - intel_display_power_put(dev_priv, POWER_DOMAIN_GMBUS, wakeref); > > > - > > > to_intel_connector(connector)->detect_edid = edid; > > > if (edid && edid->input & DRM_EDID_INPUT_DIGITAL) { > > > > We didn't have this digital input thing before. What happens with > > HDMI->VGA dongles for example? > > > > Hmm. This whole thing might already be broken on those. I suspect > > I've only used my HDMI->VGA dongle on LSPCON machines, so never > > noticed this. Need to go plug that thing into a native HDMI port... > > Except I must have left it elsewhere since I can't find it here. > So can't test right now unfortunately. > > I first thought this digital check thing might be due to > the DVI-I shenanigans in intel_crt_detect_ddc(), but that > was added for am unspecified gen2 machine in commit f5afcd3dd0dc > ("drm/i915/crt: Check for a analog monitor in case of DVI-I") > so not even relevant here. And I don't think I've ever seen > a g4x+ machine with an actual DVI-I port. Good news everyone, I found such a board: Intel DG43GT. Well, I didn't physically find one but I found the manual online. And some coreboot repo even had the vbt handily available. And yes, it does show the same DDC pins being used for the DVI-D port and CRT port. So I guess given that these digital checks do make some sense.
On Wed, 19 Oct 2022, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Thu, Oct 13, 2022 at 10:24:54PM +0300, Ville Syrjälä wrote: >> On Thu, Oct 13, 2022 at 09:41:21PM +0300, Ville Syrjälä wrote: >> > On Tue, Oct 11, 2022 at 04:49:35PM +0300, Jani Nikula wrote: >> > > - intel_hdmi_dp_dual_mode_detect(connector, edid != NULL); >> > > - >> > > - intel_display_power_put(dev_priv, POWER_DOMAIN_GMBUS, wakeref); >> > > - >> > > to_intel_connector(connector)->detect_edid = edid; >> > > if (edid && edid->input & DRM_EDID_INPUT_DIGITAL) { >> > >> > We didn't have this digital input thing before. What happens with >> > HDMI->VGA dongles for example? >> > >> > Hmm. This whole thing might already be broken on those. I suspect >> > I've only used my HDMI->VGA dongle on LSPCON machines, so never >> > noticed this. Need to go plug that thing into a native HDMI port... >> >> Except I must have left it elsewhere since I can't find it here. >> So can't test right now unfortunately. >> >> I first thought this digital check thing might be due to >> the DVI-I shenanigans in intel_crt_detect_ddc(), but that >> was added for am unspecified gen2 machine in commit f5afcd3dd0dc >> ("drm/i915/crt: Check for a analog monitor in case of DVI-I") >> so not even relevant here. And I don't think I've ever seen >> a g4x+ machine with an actual DVI-I port. > > Good news everyone, I found such a board: Intel DG43GT. > Well, I didn't physically find one but I found the manual > online. And some coreboot repo even had the vbt handily > available. And yes, it does show the same DDC pins being > used for the DVI-D port and CRT port. So I guess given > that these digital checks do make some sense. So what's the conclusion wrt the patch at hand? BR, Jani.
On Thu, Oct 20, 2022 at 11:57:06AM +0300, Jani Nikula wrote: > On Wed, 19 Oct 2022, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > > On Thu, Oct 13, 2022 at 10:24:54PM +0300, Ville Syrjälä wrote: > >> On Thu, Oct 13, 2022 at 09:41:21PM +0300, Ville Syrjälä wrote: > >> > On Tue, Oct 11, 2022 at 04:49:35PM +0300, Jani Nikula wrote: > >> > > - intel_hdmi_dp_dual_mode_detect(connector, edid != NULL); > >> > > - > >> > > - intel_display_power_put(dev_priv, POWER_DOMAIN_GMBUS, wakeref); > >> > > - > >> > > to_intel_connector(connector)->detect_edid = edid; > >> > > if (edid && edid->input & DRM_EDID_INPUT_DIGITAL) { > >> > > >> > We didn't have this digital input thing before. What happens with > >> > HDMI->VGA dongles for example? > >> > > >> > Hmm. This whole thing might already be broken on those. I suspect > >> > I've only used my HDMI->VGA dongle on LSPCON machines, so never > >> > noticed this. Need to go plug that thing into a native HDMI port... > >> > >> Except I must have left it elsewhere since I can't find it here. > >> So can't test right now unfortunately. > >> > >> I first thought this digital check thing might be due to > >> the DVI-I shenanigans in intel_crt_detect_ddc(), but that > >> was added for am unspecified gen2 machine in commit f5afcd3dd0dc > >> ("drm/i915/crt: Check for a analog monitor in case of DVI-I") > >> so not even relevant here. And I don't think I've ever seen > >> a g4x+ machine with an actual DVI-I port. > > > > Good news everyone, I found such a board: Intel DG43GT. > > Well, I didn't physically find one but I found the manual > > online. And some coreboot repo even had the vbt handily > > available. And yes, it does show the same DDC pins being > > used for the DVI-D port and CRT port. So I guess given > > that these digital checks do make some sense. > > So what's the conclusion wrt the patch at hand? Seems fine as is for the moment. We'll know more once I locate my dongle again. Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c index 93519fb23d9d..a332eaac86cd 100644 --- a/drivers/gpu/drm/i915/display/intel_hdmi.c +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c @@ -2355,7 +2355,7 @@ intel_hdmi_unset_edid(struct drm_connector *connector) } static void -intel_hdmi_dp_dual_mode_detect(struct drm_connector *connector, bool has_edid) +intel_hdmi_dp_dual_mode_detect(struct drm_connector *connector) { struct drm_i915_private *dev_priv = to_i915(connector->dev); struct intel_hdmi *hdmi = intel_attached_hdmi(to_intel_connector(connector)); @@ -2371,16 +2371,13 @@ intel_hdmi_dp_dual_mode_detect(struct drm_connector *connector, bool has_edid) * CONFIG1 pin, but no such luck on our hardware. * * The only method left to us is to check the VBT to see - * if the port is a dual mode capable DP port. But let's - * only do that when we sucesfully read the EDID, to avoid - * confusing log messages about DP dual mode adaptors when - * there's nothing connected to the port. + * if the port is a dual mode capable DP port. */ if (type == DRM_DP_DUAL_MODE_UNKNOWN) { /* An overridden EDID imply that we want this port for testing. * Make sure not to set limits for that port. */ - if (has_edid && !connector->override_edid && + if (!connector->override_edid && intel_bios_is_port_dp_dual_mode(dev_priv, port)) { drm_dbg_kms(&dev_priv->drm, "Assuming DP dual mode adaptor presence based on VBT\n"); @@ -2435,18 +2432,18 @@ intel_hdmi_set_edid(struct drm_connector *connector) intel_gmbus_force_bit(i2c, false); } - intel_hdmi_dp_dual_mode_detect(connector, edid != NULL); - - intel_display_power_put(dev_priv, POWER_DOMAIN_GMBUS, wakeref); - to_intel_connector(connector)->detect_edid = edid; if (edid && edid->input & DRM_EDID_INPUT_DIGITAL) { intel_hdmi->has_audio = drm_detect_monitor_audio(edid); intel_hdmi->has_hdmi_sink = drm_detect_hdmi_monitor(edid); + intel_hdmi_dp_dual_mode_detect(connector); + connected = true; } + intel_display_power_put(dev_priv, POWER_DOMAIN_GMBUS, wakeref); + cec_notifier_set_phys_addr_from_edid(intel_hdmi->cec_notifier, edid); return connected;
For normal connector detect, there's really no point in trying dual mode detect if the connector is disconnected. We can simplify the detect sequence by skipping it. Since intel_hdmi_dp_dual_mode_detect() is only called when EDID is present, we can drop the has_edid parameter. The functional effect is speeding up disconnected connector detection ever so slightly, and, combined with firmware EDID, also stop logging about assuming dual mode adaptor. It's a bit subtle, but this will also skip dual mode detect if the connector is force connected and a) there's no EDID of any kind, normal or override/firmare or b) there's EDID but it does not indicate digital. These are corner cases no matter what, and arguably forcing should not be limited by dual mode detect. Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Signed-off-by: Jani Nikula <jani.nikula@intel.com> --- drivers/gpu/drm/i915/display/intel_hdmi.c | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-)