[PATCH 00/15] drm/edid: EDID override refactoring and fixes

[PATCH 00/15] drm/edid: EDID override refactoring and fixes

[Jani Nikula]

I set out to fix some issues in switching i915 over to use drm_edid more
widely, and stumbled onto issues with edid override usage. Here's some
resulting fixes, refactoring and cleanup.

BR,
Jani.

Jani Nikula (15):
  drm/i915/hdmi: do dual mode detect only if connected
  drm/i915/hdmi: stop using connector->override_edid
  drm/amd/display: stop using connector->override_edid
  drm/edid: debug log EDID override set/reset
  drm/edid: abstract debugfs override EDID show better
  drm/edid: rename drm_add_override_edid_modes() to
    drm_edid_override_connector_update()
  drm/edid: split drm_edid block count helper
  drm/edid: add function for checking drm_edid validity
  drm/edid: detach debugfs EDID override from EDID property update
  drm/edid/firmware: drop redundant connector_name variable/parameter
  drm/edid/firmware: rename drm_load_edid_firmware() to
    drm_edid_load_firmware()
  drm/edid: use struct drm_edid for override/firmware EDID
  drm/edid: move edid load declarations to internal header
  drm/edid/firmware: convert to drm device specific logging
  drm/edid: convert to device specific logging

 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |   3 -
 drivers/gpu/drm/drm_crtc_internal.h           |  15 +-
 drivers/gpu/drm/drm_debugfs.c                 |   8 +-
 drivers/gpu/drm/drm_edid.c                    | 259 ++++++++++--------
 drivers/gpu/drm/drm_edid_load.c               | 107 ++------
 drivers/gpu/drm/drm_probe_helper.c            |   2 +-
 drivers/gpu/drm/i915/display/intel_hdmi.c     |  21 +-
 include/drm/drm_connector.h                   |  11 +-
 include/drm/drm_edid.h                        |  10 +-
 9 files changed, 206 insertions(+), 230 deletions(-)

-- 
2.34.1

[PATCH 01/15] drm/i915/hdmi: do dual mode detect only if connected

[Jani Nikula]

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) {
 		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

Re: [PATCH 01/15] drm/i915/hdmi: do dual mode detect only if connected

[Ville Syrjälä]

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

-- 
Ville Syrjälä
Intel

Re: [PATCH 01/15] drm/i915/hdmi: do dual mode detect only if connected

[Ville Syrjälä]

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.

-- 
Ville Syrjälä
Intel

Re: [PATCH 01/15] drm/i915/hdmi: do dual mode detect only if connected

[Jani Nikula]

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/


-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [PATCH 01/15] drm/i915/hdmi: do dual mode detect only if connected

[Ville Syrjälä]

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.

-- 
Ville Syrjälä
Intel

Re: [Intel-gfx] [PATCH 01/15] drm/i915/hdmi: do dual mode detect only if connected

[Ville Syrjälä]

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.

-- 
Ville Syrjälä
Intel

Re: [Intel-gfx] [PATCH 01/15] drm/i915/hdmi: do dual mode detect only if connected

[Jani Nikula]

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.

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [Intel-gfx] [PATCH 01/15] drm/i915/hdmi: do dual mode detect only if connected

[Ville Syrjälä]

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>

-- 
Ville Syrjälä
Intel

[PATCH 02/15] drm/i915/hdmi: stop using connector->override_edid

[Jani Nikula]

The connector->override_edid flag is strictly for EDID override debugfs
management, and drivers have no business using it.

The check for override_edid was added in commit 301906290553 ("drm/i915:
Ignore TMDS clock limit for DP++ when EDID override is set") to
facilitate mode list cross-checking against modes in override EDID when
the connector in question isn't even connected. The dual mode detect
fallback would do VBT based limiting in this case.

Instead of override EDID, check for connector forcing in the fallback.

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 | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c
index a332eaac86cd..878a65c887f7 100644
--- a/drivers/gpu/drm/i915/display/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
@@ -2374,10 +2374,8 @@ intel_hdmi_dp_dual_mode_detect(struct drm_connector *connector)
 	 * 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 (!connector->override_edid &&
+		if (connector->force != DRM_FORCE_ON &&
+		    connector->force != DRM_FORCE_ON_DIGITAL &&
 		    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");
-- 
2.34.1

Re: [PATCH 02/15] drm/i915/hdmi: stop using connector->override_edid

[Ville Syrjälä]

On Tue, Oct 11, 2022 at 04:49:36PM +0300, Jani Nikula wrote:
> The connector->override_edid flag is strictly for EDID override debugfs
> management, and drivers have no business using it.
> 
> The check for override_edid was added in commit 301906290553 ("drm/i915:
> Ignore TMDS clock limit for DP++ when EDID override is set") to
> facilitate mode list cross-checking against modes in override EDID when
> the connector in question isn't even connected. The dual mode detect
> fallback would do VBT based limiting in this case.
> 
> Instead of override EDID, check for connector forcing in the fallback.
> 
> 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 | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c
> index a332eaac86cd..878a65c887f7 100644
> --- a/drivers/gpu/drm/i915/display/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
> @@ -2374,10 +2374,8 @@ intel_hdmi_dp_dual_mode_detect(struct drm_connector *connector)
>  	 * 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 (!connector->override_edid &&
> +		if (connector->force != DRM_FORCE_ON &&
> +		    connector->force != DRM_FORCE_ON_DIGITAL &&

I don't think we can get here with force==OFF, so could simply to just
if (!connector->force && ...

which might even be less confusing either way. At least I'm getting
confused thinking we'd want to assume the presence of the adaptor
with force==OFF.

>  		    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");
> -- 
> 2.34.1

-- 
Ville Syrjälä
Intel

[PATCH 03/15] drm/amd/display: stop using connector->override_edid

[Jani Nikula]

The connector->override_edid flag is strictly for EDID override debugfs
management, and drivers have no business using it.

Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: Christian König <christian.koenig@amd.com>
Cc: Xinhui Pan <Xinhui.Pan@amd.com>
Cc: amd-gfx@lists.freedesktop.org
Signed-off-by: Jani Nikula <jani.nikula@intel.com>

---

I really have no idea what the functional impact here is. I can only
guess that the intention is to abuse ->override_edid to block EDID
property updates. In any case, this use needs to go.

It also seems really curious we get here via connector .get_modes hook!
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 4c73727e0b7d..d96877196a7f 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -6109,7 +6109,6 @@ static void create_eml_sink(struct amdgpu_dm_connector *aconnector)
 				aconnector->base.name);
 
 		aconnector->base.force = DRM_FORCE_OFF;
-		aconnector->base.override_edid = false;
 		return;
 	}
 
@@ -6144,8 +6143,6 @@ static void handle_edid_mgmt(struct amdgpu_dm_connector *aconnector)
 		link->verified_link_cap.link_rate = LINK_RATE_HIGH2;
 	}
 
-
-	aconnector->base.override_edid = true;
 	create_eml_sink(aconnector);
 }
 
-- 
2.34.1

Re: [PATCH 03/15] drm/amd/display: stop using connector->override_edid

[Harry Wentland]

On 2022-10-11 09:49, Jani Nikula wrote:
> The connector->override_edid flag is strictly for EDID override debugfs
> management, and drivers have no business using it.
> 
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Xinhui Pan <Xinhui.Pan@amd.com>
> Cc: amd-gfx@lists.freedesktop.org
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> 
> ---
> 
> I really have no idea what the functional impact here is. I can only
> guess that the intention is to abuse ->override_edid to block EDID
> property updates. In any case, this use needs to go.
> 
> It also seems really curious we get here via connector .get_modes hook!
> ---
>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index 4c73727e0b7d..d96877196a7f 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -6109,7 +6109,6 @@ static void create_eml_sink(struct amdgpu_dm_connector *aconnector)
>  				aconnector->base.name);
>  
>  		aconnector->base.force = DRM_FORCE_OFF;
> -		aconnector->base.override_edid = false;

I'm not even sure the DRM_FORCE_OFF business is right.

Either way, I don't think amdgpu should be messing with
override_edid, so this is
Reviewed-by: Harry Wentland <harry.wentland@amd.com>

Harry

>  		return;
>  	}
>  
> @@ -6144,8 +6143,6 @@ static void handle_edid_mgmt(struct amdgpu_dm_connector *aconnector)
>  		link->verified_link_cap.link_rate = LINK_RATE_HIGH2;
>  	}
>  
> -
> -	aconnector->base.override_edid = true;
>  	create_eml_sink(aconnector);
>  }
>  

Re: [PATCH 03/15] drm/amd/display: stop using connector->override_edid

[Jani Nikula]

On Tue, 11 Oct 2022, Harry Wentland <harry.wentland@amd.com> wrote:
> On 2022-10-11 09:49, Jani Nikula wrote:
>> The connector->override_edid flag is strictly for EDID override debugfs
>> management, and drivers have no business using it.
>> 
>> Cc: Alex Deucher <alexander.deucher@amd.com>
>> Cc: Christian König <christian.koenig@amd.com>
>> Cc: Xinhui Pan <Xinhui.Pan@amd.com>
>> Cc: amd-gfx@lists.freedesktop.org
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> 
>> ---
>> 
>> I really have no idea what the functional impact here is. I can only
>> guess that the intention is to abuse ->override_edid to block EDID
>> property updates. In any case, this use needs to go.
>> 
>> It also seems really curious we get here via connector .get_modes hook!
>> ---
>>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 3 ---
>>  1 file changed, 3 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> index 4c73727e0b7d..d96877196a7f 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> @@ -6109,7 +6109,6 @@ static void create_eml_sink(struct amdgpu_dm_connector *aconnector)
>>  				aconnector->base.name);
>>  
>>  		aconnector->base.force = DRM_FORCE_OFF;
>> -		aconnector->base.override_edid = false;
>
> I'm not even sure the DRM_FORCE_OFF business is right.
>
> Either way, I don't think amdgpu should be messing with
> override_edid, so this is
> Reviewed-by: Harry Wentland <harry.wentland@amd.com>

Thanks for the swift review; is it okay to merge this via drm-misc-next
along with the rest (once they've been reviewed, of course)?

BR,
Jani.

>
> Harry
>
>>  		return;
>>  	}
>>  
>> @@ -6144,8 +6143,6 @@ static void handle_edid_mgmt(struct amdgpu_dm_connector *aconnector)
>>  		link->verified_link_cap.link_rate = LINK_RATE_HIGH2;
>>  	}
>>  
>> -
>> -	aconnector->base.override_edid = true;
>>  	create_eml_sink(aconnector);
>>  }
>>  
>

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [PATCH 03/15] drm/amd/display: stop using connector->override_edid

[Alex Deucher]

On Tue, Oct 11, 2022 at 10:12 AM Jani Nikula <jani.nikula@intel.com> wrote:
>
> On Tue, 11 Oct 2022, Harry Wentland <harry.wentland@amd.com> wrote:
> > On 2022-10-11 09:49, Jani Nikula wrote:
> >> The connector->override_edid flag is strictly for EDID override debugfs
> >> management, and drivers have no business using it.
> >>
> >> Cc: Alex Deucher <alexander.deucher@amd.com>
> >> Cc: Christian König <christian.koenig@amd.com>
> >> Cc: Xinhui Pan <Xinhui.Pan@amd.com>
> >> Cc: amd-gfx@lists.freedesktop.org
> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> >>
> >> ---
> >>
> >> I really have no idea what the functional impact here is. I can only
> >> guess that the intention is to abuse ->override_edid to block EDID
> >> property updates. In any case, this use needs to go.
> >>
> >> It also seems really curious we get here via connector .get_modes hook!
> >> ---
> >>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 3 ---
> >>  1 file changed, 3 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> >> index 4c73727e0b7d..d96877196a7f 100644
> >> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> >> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> >> @@ -6109,7 +6109,6 @@ static void create_eml_sink(struct amdgpu_dm_connector *aconnector)
> >>                              aconnector->base.name);
> >>
> >>              aconnector->base.force = DRM_FORCE_OFF;
> >> -            aconnector->base.override_edid = false;
> >
> > I'm not even sure the DRM_FORCE_OFF business is right.
> >
> > Either way, I don't think amdgpu should be messing with
> > override_edid, so this is
> > Reviewed-by: Harry Wentland <harry.wentland@amd.com>
>
> Thanks for the swift review; is it okay to merge this via drm-misc-next
> along with the rest (once they've been reviewed, of course)?

Sure.  No problem.

Alex

>
> BR,
> Jani.
>
> >
> > Harry
> >
> >>              return;
> >>      }
> >>
> >> @@ -6144,8 +6143,6 @@ static void handle_edid_mgmt(struct amdgpu_dm_connector *aconnector)
> >>              link->verified_link_cap.link_rate = LINK_RATE_HIGH2;
> >>      }
> >>
> >> -
> >> -    aconnector->base.override_edid = true;
> >>      create_eml_sink(aconnector);
> >>  }
> >>
> >
>
> --
> Jani Nikula, Intel Open Source Graphics Center

[PATCH 04/15] drm/edid: debug log EDID override set/reset

[Jani Nikula]

It's useful debugging information to know if and when an override EDID
was set or reset.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/drm_edid.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 47465b9765f1..a863cffa2dc5 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -2194,6 +2194,9 @@ int drm_edid_override_set(struct drm_connector *connector, const void *edid,
 
 	connector->override_edid = false;
 
+	drm_dbg_kms(connector->dev, "[CONNECTOR:%d:%s] EDID override set\n",
+		    connector->base.id, connector->name);
+
 	ret = drm_connector_update_edid_property(connector, edid);
 	if (!ret)
 		connector->override_edid = true;
@@ -2206,6 +2209,9 @@ int drm_edid_override_reset(struct drm_connector *connector)
 {
 	connector->override_edid = false;
 
+	drm_dbg_kms(connector->dev, "[CONNECTOR:%d:%s] EDID override reset\n",
+		    connector->base.id, connector->name);
+
 	return drm_connector_update_edid_property(connector, NULL);
 }
 
-- 
2.34.1

Re: [PATCH 04/15] drm/edid: debug log EDID override set/reset

[Ville Syrjälä]

On Tue, Oct 11, 2022 at 04:49:38PM +0300, Jani Nikula wrote:
> It's useful debugging information to know if and when an override EDID
> was set or reset.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/drm_edid.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 47465b9765f1..a863cffa2dc5 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -2194,6 +2194,9 @@ int drm_edid_override_set(struct drm_connector *connector, const void *edid,
>  
>  	connector->override_edid = false;
>  
> +	drm_dbg_kms(connector->dev, "[CONNECTOR:%d:%s] EDID override set\n",
> +		    connector->base.id, connector->name);
> +
>  	ret = drm_connector_update_edid_property(connector, edid);
>  	if (!ret)
>  		connector->override_edid = true;
> @@ -2206,6 +2209,9 @@ int drm_edid_override_reset(struct drm_connector *connector)
>  {
>  	connector->override_edid = false;
>  
> +	drm_dbg_kms(connector->dev, "[CONNECTOR:%d:%s] EDID override reset\n",
> +		    connector->base.id, connector->name);
> +
>  	return drm_connector_update_edid_property(connector, NULL);
>  }
>  
> -- 
> 2.34.1

-- 
Ville Syrjälä
Intel

[PATCH 05/15] drm/edid: abstract debugfs override EDID show better

[Jani Nikula]

Add a function to dump the override EDID in debugfs. This hides the
override EDID management better in drm_edid.c.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/drm_crtc_internal.h |  4 +++-
 drivers/gpu/drm/drm_debugfs.c       |  8 +-------
 drivers/gpu/drm/drm_edid.c          | 11 +++++++++++
 3 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h
index 56041b604881..fb8a68d90940 100644
--- a/drivers/gpu/drm/drm_crtc_internal.h
+++ b/drivers/gpu/drm/drm_crtc_internal.h
@@ -56,9 +56,10 @@ struct drm_plane;
 struct drm_plane_state;
 struct drm_property;
 struct edid;
+struct fwnode_handle;
 struct kref;
+struct seq_file;
 struct work_struct;
-struct fwnode_handle;
 
 /* drm_crtc.c */
 int drm_mode_crtc_set_obj_prop(struct drm_mode_object *obj,
@@ -286,5 +287,6 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
 
 /* drm_edid.c */
 void drm_mode_fixup_1366x768(struct drm_display_mode *mode);
+int drm_edid_override_show(struct drm_connector *connector, struct seq_file *m);
 int drm_edid_override_set(struct drm_connector *connector, const void *edid, size_t size);
 int drm_edid_override_reset(struct drm_connector *connector);
diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
index 01ee3febb813..ee445f4605ba 100644
--- a/drivers/gpu/drm/drm_debugfs.c
+++ b/drivers/gpu/drm/drm_debugfs.c
@@ -328,13 +328,7 @@ static ssize_t connector_write(struct file *file, const char __user *ubuf,
 
 static int edid_show(struct seq_file *m, void *data)
 {
-	struct drm_connector *connector = m->private;
-	struct drm_property_blob *edid = connector->edid_blob_ptr;
-
-	if (connector->override_edid && edid)
-		seq_write(m, edid->data, edid->length);
-
-	return 0;
+	return drm_edid_override_show(m->private, m);
 }
 
 static int edid_open(struct inode *inode, struct file *file)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index a863cffa2dc5..1ada36e0d826 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -2183,6 +2183,17 @@ static struct edid *drm_get_override_edid(struct drm_connector *connector,
 	return IS_ERR(override) ? NULL : override;
 }
 
+/* For debugfs edid_override implementation */
+int drm_edid_override_show(struct drm_connector *connector, struct seq_file *m)
+{
+	struct drm_property_blob *edid = connector->edid_blob_ptr;
+
+	if (connector->override_edid && edid)
+		seq_write(m, edid->data, edid->length);
+
+	return 0;
+}
+
 /* For debugfs edid_override implementation */
 int drm_edid_override_set(struct drm_connector *connector, const void *edid,
 			  size_t size)
-- 
2.34.1

Re: [PATCH 05/15] drm/edid: abstract debugfs override EDID show better

[Ville Syrjälä]

On Tue, Oct 11, 2022 at 04:49:39PM +0300, Jani Nikula wrote:
> Add a function to dump the override EDID in debugfs. This hides the
> override EDID management better in drm_edid.c.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/drm_crtc_internal.h |  4 +++-
>  drivers/gpu/drm/drm_debugfs.c       |  8 +-------
>  drivers/gpu/drm/drm_edid.c          | 11 +++++++++++
>  3 files changed, 15 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h
> index 56041b604881..fb8a68d90940 100644
> --- a/drivers/gpu/drm/drm_crtc_internal.h
> +++ b/drivers/gpu/drm/drm_crtc_internal.h
> @@ -56,9 +56,10 @@ struct drm_plane;
>  struct drm_plane_state;
>  struct drm_property;
>  struct edid;
> +struct fwnode_handle;
>  struct kref;
> +struct seq_file;
>  struct work_struct;
> -struct fwnode_handle;
>  
>  /* drm_crtc.c */
>  int drm_mode_crtc_set_obj_prop(struct drm_mode_object *obj,
> @@ -286,5 +287,6 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
>  
>  /* drm_edid.c */
>  void drm_mode_fixup_1366x768(struct drm_display_mode *mode);
> +int drm_edid_override_show(struct drm_connector *connector, struct seq_file *m);
>  int drm_edid_override_set(struct drm_connector *connector, const void *edid, size_t size);
>  int drm_edid_override_reset(struct drm_connector *connector);
> diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
> index 01ee3febb813..ee445f4605ba 100644
> --- a/drivers/gpu/drm/drm_debugfs.c
> +++ b/drivers/gpu/drm/drm_debugfs.c
> @@ -328,13 +328,7 @@ static ssize_t connector_write(struct file *file, const char __user *ubuf,
>  
>  static int edid_show(struct seq_file *m, void *data)
>  {
> -	struct drm_connector *connector = m->private;
> -	struct drm_property_blob *edid = connector->edid_blob_ptr;
> -
> -	if (connector->override_edid && edid)
> -		seq_write(m, edid->data, edid->length);
> -
> -	return 0;
> +	return drm_edid_override_show(m->private, m);
>  }
>  
>  static int edid_open(struct inode *inode, struct file *file)
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index a863cffa2dc5..1ada36e0d826 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -2183,6 +2183,17 @@ static struct edid *drm_get_override_edid(struct drm_connector *connector,
>  	return IS_ERR(override) ? NULL : override;
>  }
>  
> +/* For debugfs edid_override implementation */
> +int drm_edid_override_show(struct drm_connector *connector, struct seq_file *m)
> +{
> +	struct drm_property_blob *edid = connector->edid_blob_ptr;
> +
> +	if (connector->override_edid && edid)
> +		seq_write(m, edid->data, edid->length);
> +
> +	return 0;
> +}
> +
>  /* For debugfs edid_override implementation */
>  int drm_edid_override_set(struct drm_connector *connector, const void *edid,
>  			  size_t size)
> -- 
> 2.34.1

-- 
Ville Syrjälä
Intel

[PATCH 06/15] drm/edid: rename drm_add_override_edid_modes() to drm_edid_override_connector_update()

[Jani Nikula]

Follow the naming of both EDID override functions as well as
drm_edid_connector_update(). This also matches better what the function
does; a combination of EDID property update and add modes. Indeed it
should later be converted to call drm_edid_connector_update().

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/drm_edid.c         | 6 +++---
 drivers/gpu/drm/drm_probe_helper.c | 2 +-
 include/drm/drm_edid.h             | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 1ada36e0d826..8baa46dc2537 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -2227,7 +2227,7 @@ int drm_edid_override_reset(struct drm_connector *connector)
 }
 
 /**
- * drm_add_override_edid_modes - add modes from override/firmware EDID
+ * drm_edid_override_connector_update - add modes from override/firmware EDID
  * @connector: connector we're probing
  *
  * Add modes from the override/firmware EDID, if available. Only to be used from
@@ -2237,7 +2237,7 @@ int drm_edid_override_reset(struct drm_connector *connector)
  *
  * Return: The number of modes added or 0 if we couldn't find any.
  */
-int drm_add_override_edid_modes(struct drm_connector *connector)
+int drm_edid_override_connector_update(struct drm_connector *connector)
 {
 	struct edid *override;
 	int num_modes = 0;
@@ -2254,7 +2254,7 @@ int drm_add_override_edid_modes(struct drm_connector *connector)
 
 	return num_modes;
 }
-EXPORT_SYMBOL(drm_add_override_edid_modes);
+EXPORT_SYMBOL(drm_edid_override_connector_update);
 
 typedef int read_block_fn(void *context, u8 *buf, unsigned int block, size_t len);
 
diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
index 69b0b2b9cc1c..2fc21df709bc 100644
--- a/drivers/gpu/drm/drm_probe_helper.c
+++ b/drivers/gpu/drm/drm_probe_helper.c
@@ -367,7 +367,7 @@ static int drm_helper_probe_get_modes(struct drm_connector *connector)
 	 * override/firmware EDID.
 	 */
 	if (count == 0 && connector->status == connector_status_connected)
-		count = drm_add_override_edid_modes(connector);
+		count = drm_edid_override_connector_update(connector);
 
 	return count;
 }
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index 429735b91f63..05380681a4fb 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -577,7 +577,7 @@ struct edid *drm_get_edid_switcheroo(struct drm_connector *connector,
 				     struct i2c_adapter *adapter);
 struct edid *drm_edid_duplicate(const struct edid *edid);
 int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid);
-int drm_add_override_edid_modes(struct drm_connector *connector);
+int drm_edid_override_connector_update(struct drm_connector *connector);
 
 u8 drm_match_cea_mode(const struct drm_display_mode *to_match);
 bool drm_detect_hdmi_monitor(const struct edid *edid);
-- 
2.34.1

Re: [PATCH 06/15] drm/edid: rename drm_add_override_edid_modes() to drm_edid_override_connector_update()

[Ville Syrjälä]

On Tue, Oct 11, 2022 at 04:49:40PM +0300, Jani Nikula wrote:
> Follow the naming of both EDID override functions as well as
> drm_edid_connector_update(). This also matches better what the function
> does; a combination of EDID property update and add modes. Indeed it
> should later be converted to call drm_edid_connector_update().

All the function names around that stuff still make my head spin,
but hopefully it'll clear up eventually when all the cruft has
disappeared.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/drm_edid.c         | 6 +++---
>  drivers/gpu/drm/drm_probe_helper.c | 2 +-
>  include/drm/drm_edid.h             | 2 +-
>  3 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 1ada36e0d826..8baa46dc2537 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -2227,7 +2227,7 @@ int drm_edid_override_reset(struct drm_connector *connector)
>  }
>  
>  /**
> - * drm_add_override_edid_modes - add modes from override/firmware EDID
> + * drm_edid_override_connector_update - add modes from override/firmware EDID
>   * @connector: connector we're probing
>   *
>   * Add modes from the override/firmware EDID, if available. Only to be used from
> @@ -2237,7 +2237,7 @@ int drm_edid_override_reset(struct drm_connector *connector)
>   *
>   * Return: The number of modes added or 0 if we couldn't find any.
>   */
> -int drm_add_override_edid_modes(struct drm_connector *connector)
> +int drm_edid_override_connector_update(struct drm_connector *connector)
>  {
>  	struct edid *override;
>  	int num_modes = 0;
> @@ -2254,7 +2254,7 @@ int drm_add_override_edid_modes(struct drm_connector *connector)
>  
>  	return num_modes;
>  }
> -EXPORT_SYMBOL(drm_add_override_edid_modes);
> +EXPORT_SYMBOL(drm_edid_override_connector_update);
>  
>  typedef int read_block_fn(void *context, u8 *buf, unsigned int block, size_t len);
>  
> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> index 69b0b2b9cc1c..2fc21df709bc 100644
> --- a/drivers/gpu/drm/drm_probe_helper.c
> +++ b/drivers/gpu/drm/drm_probe_helper.c
> @@ -367,7 +367,7 @@ static int drm_helper_probe_get_modes(struct drm_connector *connector)
>  	 * override/firmware EDID.
>  	 */
>  	if (count == 0 && connector->status == connector_status_connected)
> -		count = drm_add_override_edid_modes(connector);
> +		count = drm_edid_override_connector_update(connector);
>  
>  	return count;
>  }
> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> index 429735b91f63..05380681a4fb 100644
> --- a/include/drm/drm_edid.h
> +++ b/include/drm/drm_edid.h
> @@ -577,7 +577,7 @@ struct edid *drm_get_edid_switcheroo(struct drm_connector *connector,
>  				     struct i2c_adapter *adapter);
>  struct edid *drm_edid_duplicate(const struct edid *edid);
>  int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid);
> -int drm_add_override_edid_modes(struct drm_connector *connector);
> +int drm_edid_override_connector_update(struct drm_connector *connector);
>  
>  u8 drm_match_cea_mode(const struct drm_display_mode *to_match);
>  bool drm_detect_hdmi_monitor(const struct edid *edid);
> -- 
> 2.34.1

-- 
Ville Syrjälä
Intel

[PATCH 07/15] drm/edid: split drm_edid block count helper

[Jani Nikula]

Split the drm_edid block count helper to a base version that reports the
block count indicated by EDID contents, and another on top that limits
the block count based on size allocated for the EDID.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/drm_edid.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 8baa46dc2537..616c1cdc7507 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -1613,7 +1613,8 @@ static const void *edid_extension_block_data(const struct edid *edid, int index)
 	return edid_block_data(edid, index + 1);
 }
 
-static int drm_edid_block_count(const struct drm_edid *drm_edid)
+/* EDID block count indicated in EDID, may exceed allocated size */
+static int __drm_edid_block_count(const struct drm_edid *drm_edid)
 {
 	int num_blocks;
 
@@ -1633,12 +1634,18 @@ static int drm_edid_block_count(const struct drm_edid *drm_edid)
 			num_blocks = eeodb;
 	}
 
-	/* Limit by allocated size */
-	num_blocks = min(num_blocks, (int)drm_edid->size / EDID_LENGTH);
-
 	return num_blocks;
 }
 
+/* EDID block count, limited by allocated size */
+static int drm_edid_block_count(const struct drm_edid *drm_edid)
+{
+	/* Limit by allocated size */
+	return min(__drm_edid_block_count(drm_edid),
+		   (int)drm_edid->size / EDID_LENGTH);
+}
+
+/* EDID extension block count, limited by allocated size */
 static int drm_edid_extension_block_count(const struct drm_edid *drm_edid)
 {
 	return drm_edid_block_count(drm_edid) - 1;
-- 
2.34.1

Re: [PATCH 07/15] drm/edid: split drm_edid block count helper

[Ville Syrjälä]

On Tue, Oct 11, 2022 at 04:49:41PM +0300, Jani Nikula wrote:
> Split the drm_edid block count helper to a base version that reports the
> block count indicated by EDID contents, and another on top that limits
> the block count based on size allocated for the EDID.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/drm_edid.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 8baa46dc2537..616c1cdc7507 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -1613,7 +1613,8 @@ static const void *edid_extension_block_data(const struct edid *edid, int index)
>  	return edid_block_data(edid, index + 1);
>  }
>  
> -static int drm_edid_block_count(const struct drm_edid *drm_edid)
> +/* EDID block count indicated in EDID, may exceed allocated size */
> +static int __drm_edid_block_count(const struct drm_edid *drm_edid)
>  {
>  	int num_blocks;
>  
> @@ -1633,12 +1634,18 @@ static int drm_edid_block_count(const struct drm_edid *drm_edid)
>  			num_blocks = eeodb;
>  	}
>  
> -	/* Limit by allocated size */
> -	num_blocks = min(num_blocks, (int)drm_edid->size / EDID_LENGTH);
> -
>  	return num_blocks;
>  }
>  
> +/* EDID block count, limited by allocated size */
> +static int drm_edid_block_count(const struct drm_edid *drm_edid)
> +{
> +	/* Limit by allocated size */
> +	return min(__drm_edid_block_count(drm_edid),
> +		   (int)drm_edid->size / EDID_LENGTH);
> +}
> +
> +/* EDID extension block count, limited by allocated size */
>  static int drm_edid_extension_block_count(const struct drm_edid *drm_edid)
>  {
>  	return drm_edid_block_count(drm_edid) - 1;
> -- 
> 2.34.1

-- 
Ville Syrjälä
Intel

[PATCH 08/15] drm/edid: add function for checking drm_edid validity

[Jani Nikula]

We've lacked a function for immutable validity check on drm_edid. Add
one.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/drm_edid.c | 30 ++++++++++++++++++++++++++++++
 include/drm/drm_edid.h     |  1 +
 2 files changed, 31 insertions(+)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 616c1cdc7507..c3cf942186b7 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -2040,6 +2040,36 @@ bool drm_edid_is_valid(struct edid *edid)
 }
 EXPORT_SYMBOL(drm_edid_is_valid);
 
+/**
+ * drm_edid_valid - sanity check EDID data
+ * @drm_edid: EDID data
+ *
+ * Sanity check an EDID. Cross check block count against allocated size and
+ * checksum the blocks.
+ *
+ * Return: True if the EDID data is valid, false otherwise.
+ */
+bool drm_edid_valid(const struct drm_edid *drm_edid)
+{
+	int i;
+
+	if (!drm_edid)
+		return false;
+
+	if (edid_size_by_blocks(__drm_edid_block_count(drm_edid)) != drm_edid->size)
+		return false;
+
+	for (i = 0; i < drm_edid_block_count(drm_edid); i++) {
+		const void *block = drm_edid_block_data(drm_edid, i);
+
+		if (!edid_block_valid(block, i == 0))
+			return false;
+	}
+
+	return true;
+}
+EXPORT_SYMBOL(drm_edid_valid);
+
 static struct edid *edid_filter_invalid_blocks(struct edid *edid,
 					       size_t *alloc_size)
 {
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index 05380681a4fb..a2e25e7e6ee5 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -606,6 +606,7 @@ drm_display_mode_from_cea_vic(struct drm_device *dev,
 const struct drm_edid *drm_edid_alloc(const void *edid, size_t size);
 const struct drm_edid *drm_edid_dup(const struct drm_edid *drm_edid);
 void drm_edid_free(const struct drm_edid *drm_edid);
+bool drm_edid_valid(const struct drm_edid *drm_edid);
 const struct edid *drm_edid_raw(const struct drm_edid *drm_edid);
 const struct drm_edid *drm_edid_read(struct drm_connector *connector);
 const struct drm_edid *drm_edid_read_ddc(struct drm_connector *connector,
-- 
2.34.1

Re: [PATCH 08/15] drm/edid: add function for checking drm_edid validity

[Ville Syrjälä]

On Tue, Oct 11, 2022 at 04:49:42PM +0300, Jani Nikula wrote:
> We've lacked a function for immutable validity check on drm_edid. Add
> one.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/drm_edid.c | 30 ++++++++++++++++++++++++++++++
>  include/drm/drm_edid.h     |  1 +
>  2 files changed, 31 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 616c1cdc7507..c3cf942186b7 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -2040,6 +2040,36 @@ bool drm_edid_is_valid(struct edid *edid)
>  }
>  EXPORT_SYMBOL(drm_edid_is_valid);
>  
> +/**
> + * drm_edid_valid - sanity check EDID data
> + * @drm_edid: EDID data
> + *
> + * Sanity check an EDID. Cross check block count against allocated size and
> + * checksum the blocks.
> + *
> + * Return: True if the EDID data is valid, false otherwise.
> + */
> +bool drm_edid_valid(const struct drm_edid *drm_edid)
> +{
> +	int i;
> +
> +	if (!drm_edid)
> +		return false;
> +
> +	if (edid_size_by_blocks(__drm_edid_block_count(drm_edid)) != drm_edid->size)
> +		return false;

I Was consfued about the HF-EEODB crap for a bit but 
__drm_edid_block_count() does include that.

So looks sane
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> +
> +	for (i = 0; i < drm_edid_block_count(drm_edid); i++) {
> +		const void *block = drm_edid_block_data(drm_edid, i);
> +
> +		if (!edid_block_valid(block, i == 0))
> +			return false;
> +	}
> +
> +	return true;
> +}
> +EXPORT_SYMBOL(drm_edid_valid);
> +
>  static struct edid *edid_filter_invalid_blocks(struct edid *edid,
>  					       size_t *alloc_size)
>  {
> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> index 05380681a4fb..a2e25e7e6ee5 100644
> --- a/include/drm/drm_edid.h
> +++ b/include/drm/drm_edid.h
> @@ -606,6 +606,7 @@ drm_display_mode_from_cea_vic(struct drm_device *dev,
>  const struct drm_edid *drm_edid_alloc(const void *edid, size_t size);
>  const struct drm_edid *drm_edid_dup(const struct drm_edid *drm_edid);
>  void drm_edid_free(const struct drm_edid *drm_edid);
> +bool drm_edid_valid(const struct drm_edid *drm_edid);
>  const struct edid *drm_edid_raw(const struct drm_edid *drm_edid);
>  const struct drm_edid *drm_edid_read(struct drm_connector *connector);
>  const struct drm_edid *drm_edid_read_ddc(struct drm_connector *connector,
> -- 
> 2.34.1

-- 
Ville Syrjälä
Intel

[PATCH 09/15] drm/edid: detach debugfs EDID override from EDID property update

[Jani Nikula]

Having the EDID override debugfs directly update the EDID property is
problematic. The update is partial only. The driver has no way of
knowing it's been updated. Mode list is not updated. It's an
inconsistent state.

Detach debugfs EDID override from the property update completely. Only
set and reset a separate override EDID copy from debugfs, and have it
take effect only at detect (via EDID read). The copy is at
connector->edid_override.

This also brings override EDID closer to firmware EDID in behaviour.

Add validation of the override EDID which we completely lacked.

Note that IGT already forces a detect whenever tests update the override
EDID.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/drm_edid.c  | 56 ++++++++++++-------------------------
 include/drm/drm_connector.h | 11 +++++---
 2 files changed, 25 insertions(+), 42 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index c3cf942186b7..0f2898badd51 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -2207,8 +2207,8 @@ static struct edid *drm_get_override_edid(struct drm_connector *connector,
 {
 	struct edid *override = NULL;
 
-	if (connector->override_edid)
-		override = drm_edid_duplicate(connector->edid_blob_ptr->data);
+	if (connector->edid_override)
+		override = drm_edid_duplicate(connector->edid_override->edid);
 
 	if (!override)
 		override = drm_load_edid_firmware(connector);
@@ -2223,10 +2223,10 @@ static struct edid *drm_get_override_edid(struct drm_connector *connector,
 /* For debugfs edid_override implementation */
 int drm_edid_override_show(struct drm_connector *connector, struct seq_file *m)
 {
-	struct drm_property_blob *edid = connector->edid_blob_ptr;
+	const struct drm_edid *drm_edid = connector->edid_override;
 
-	if (connector->override_edid && edid)
-		seq_write(m, edid->data, edid->length);
+	if (drm_edid)
+		seq_write(m, drm_edid->edid, drm_edid->size);
 
 	return 0;
 }
@@ -2235,32 +2235,33 @@ int drm_edid_override_show(struct drm_connector *connector, struct seq_file *m)
 int drm_edid_override_set(struct drm_connector *connector, const void *edid,
 			  size_t size)
 {
-	int ret;
+	drm_edid_free(connector->edid_override);
 
-	if (size < EDID_LENGTH || edid_size(edid) > size)
+	connector->edid_override = drm_edid_alloc(edid, size);
+	if (!drm_edid_valid(connector->edid_override)) {
+		drm_dbg_kms(connector->dev, "[CONNECTOR:%d:%s] EDID override invalid\n",
+			    connector->base.id, connector->name);
+		drm_edid_free(connector->edid_override);
+		connector->edid_override = NULL;
 		return -EINVAL;
-
-	connector->override_edid = false;
+	}
 
 	drm_dbg_kms(connector->dev, "[CONNECTOR:%d:%s] EDID override set\n",
 		    connector->base.id, connector->name);
 
-	ret = drm_connector_update_edid_property(connector, edid);
-	if (!ret)
-		connector->override_edid = true;
-
-	return ret;
+	return 0;
 }
 
 /* For debugfs edid_override implementation */
 int drm_edid_override_reset(struct drm_connector *connector)
 {
-	connector->override_edid = false;
-
 	drm_dbg_kms(connector->dev, "[CONNECTOR:%d:%s] EDID override reset\n",
 		    connector->base.id, connector->name);
 
-	return drm_connector_update_edid_property(connector, NULL);
+	drm_edid_free(connector->edid_override);
+	connector->edid_override = NULL;
+
+	return 0;
 }
 
 /**
@@ -6634,23 +6635,6 @@ int drm_edid_connector_update(struct drm_connector *connector,
 {
 	int count;
 
-	/*
-	 * FIXME: Reconcile the differences in override_edid handling between
-	 * this and drm_connector_update_edid_property().
-	 *
-	 * If override_edid is set, and the EDID passed in here originates from
-	 * drm_edid_read() and friends, it will be the override EDID, and there
-	 * are no issues. drm_connector_update_edid_property() ignoring requests
-	 * to set the EDID dates back to a time when override EDID was not
-	 * handled at the low level EDID read.
-	 *
-	 * The only way the EDID passed in here can be different from the
-	 * override EDID is when a driver passes in an EDID that does *not*
-	 * originate from drm_edid_read() and friends, or passes in a stale
-	 * cached version. This, in turn, is a question of when an override EDID
-	 * set via debugfs should take effect.
-	 */
-
 	count = _drm_edid_connector_update(connector, drm_edid);
 
 	_drm_update_tile_info(connector, drm_edid);
@@ -6665,10 +6649,6 @@ EXPORT_SYMBOL(drm_edid_connector_update);
 static int _drm_connector_update_edid_property(struct drm_connector *connector,
 					       const struct drm_edid *drm_edid)
 {
-	/* ignore requests to set edid when overridden */
-	if (connector->override_edid)
-		return 0;
-
 	/*
 	 * Set the display info, using edid if available, otherwise resetting
 	 * the values to defaults. This duplicates the work done in
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index b1b2df48d42c..09a7d7f23e4a 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -1550,12 +1550,15 @@ struct drm_connector {
 	struct drm_cmdline_mode cmdline_mode;
 	/** @force: a DRM_FORCE_<foo> state for forced mode sets */
 	enum drm_connector_force force;
+
 	/**
-	 * @override_edid: has the EDID been overwritten through debugfs for
-	 * testing? Do not modify outside of drm_edid_override_set() and
-	 * drm_edid_override_reset().
+	 * @edid_override: Override EDID set via debugfs.
+	 *
+	 * Do not modify or access outside of the drm_edid_override_* family of
+	 * functions.
 	 */
-	bool override_edid;
+	const struct drm_edid *edid_override;
+
 	/** @epoch_counter: used to detect any other changes in connector, besides status */
 	u64 epoch_counter;
 
-- 
2.34.1

Re: [PATCH 09/15] drm/edid: detach debugfs EDID override from EDID property update

[Ville Syrjälä]

On Tue, Oct 11, 2022 at 04:49:43PM +0300, Jani Nikula wrote:
> Having the EDID override debugfs directly update the EDID property is
> problematic. The update is partial only. The driver has no way of
> knowing it's been updated. Mode list is not updated. It's an
> inconsistent state.
> 
> Detach debugfs EDID override from the property update completely. Only
> set and reset a separate override EDID copy from debugfs, and have it
> take effect only at detect (via EDID read). The copy is at
> connector->edid_override.
> 
> This also brings override EDID closer to firmware EDID in behaviour.
> 
> Add validation of the override EDID which we completely lacked.
> 
> Note that IGT already forces a detect whenever tests update the override
> EDID.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/drm_edid.c  | 56 ++++++++++++-------------------------
>  include/drm/drm_connector.h | 11 +++++---
>  2 files changed, 25 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index c3cf942186b7..0f2898badd51 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -2207,8 +2207,8 @@ static struct edid *drm_get_override_edid(struct drm_connector *connector,
>  {
>  	struct edid *override = NULL;
>  
> -	if (connector->override_edid)
> -		override = drm_edid_duplicate(connector->edid_blob_ptr->data);
> +	if (connector->edid_override)
> +		override = drm_edid_duplicate(connector->edid_override->edid);
>  
>  	if (!override)
>  		override = drm_load_edid_firmware(connector);
> @@ -2223,10 +2223,10 @@ static struct edid *drm_get_override_edid(struct drm_connector *connector,
>  /* For debugfs edid_override implementation */
>  int drm_edid_override_show(struct drm_connector *connector, struct seq_file *m)
>  {
> -	struct drm_property_blob *edid = connector->edid_blob_ptr;
> +	const struct drm_edid *drm_edid = connector->edid_override;
>  
> -	if (connector->override_edid && edid)
> -		seq_write(m, edid->data, edid->length);
> +	if (drm_edid)
> +		seq_write(m, drm_edid->edid, drm_edid->size);
>  
>  	return 0;
>  }
> @@ -2235,32 +2235,33 @@ int drm_edid_override_show(struct drm_connector *connector, struct seq_file *m)
>  int drm_edid_override_set(struct drm_connector *connector, const void *edid,
>  			  size_t size)
>  {
> -	int ret;
> +	drm_edid_free(connector->edid_override);
>  
> -	if (size < EDID_LENGTH || edid_size(edid) > size)
> +	connector->edid_override = drm_edid_alloc(edid, size);
> +	if (!drm_edid_valid(connector->edid_override)) {
> +		drm_dbg_kms(connector->dev, "[CONNECTOR:%d:%s] EDID override invalid\n",
> +			    connector->base.id, connector->name);
> +		drm_edid_free(connector->edid_override);
> +		connector->edid_override = NULL;
>  		return -EINVAL;

Hmm. Should we perhaps invest in some locking around these parts?

> -
> -	connector->override_edid = false;
> +	}
>  
>  	drm_dbg_kms(connector->dev, "[CONNECTOR:%d:%s] EDID override set\n",
>  		    connector->base.id, connector->name);
>  
> -	ret = drm_connector_update_edid_property(connector, edid);
> -	if (!ret)
> -		connector->override_edid = true;
> -
> -	return ret;
> +	return 0;
>  }
>  
>  /* For debugfs edid_override implementation */
>  int drm_edid_override_reset(struct drm_connector *connector)
>  {
> -	connector->override_edid = false;
> -
>  	drm_dbg_kms(connector->dev, "[CONNECTOR:%d:%s] EDID override reset\n",
>  		    connector->base.id, connector->name);
>  
> -	return drm_connector_update_edid_property(connector, NULL);
> +	drm_edid_free(connector->edid_override);
> +	connector->edid_override = NULL;
> +
> +	return 0;
>  }
>  
>  /**
> @@ -6634,23 +6635,6 @@ int drm_edid_connector_update(struct drm_connector *connector,
>  {
>  	int count;
>  
> -	/*
> -	 * FIXME: Reconcile the differences in override_edid handling between
> -	 * this and drm_connector_update_edid_property().
> -	 *
> -	 * If override_edid is set, and the EDID passed in here originates from
> -	 * drm_edid_read() and friends, it will be the override EDID, and there
> -	 * are no issues. drm_connector_update_edid_property() ignoring requests
> -	 * to set the EDID dates back to a time when override EDID was not
> -	 * handled at the low level EDID read.
> -	 *
> -	 * The only way the EDID passed in here can be different from the
> -	 * override EDID is when a driver passes in an EDID that does *not*
> -	 * originate from drm_edid_read() and friends, or passes in a stale
> -	 * cached version. This, in turn, is a question of when an override EDID
> -	 * set via debugfs should take effect.
> -	 */
> -
>  	count = _drm_edid_connector_update(connector, drm_edid);
>  
>  	_drm_update_tile_info(connector, drm_edid);
> @@ -6665,10 +6649,6 @@ EXPORT_SYMBOL(drm_edid_connector_update);
>  static int _drm_connector_update_edid_property(struct drm_connector *connector,
>  					       const struct drm_edid *drm_edid)
>  {
> -	/* ignore requests to set edid when overridden */
> -	if (connector->override_edid)
> -		return 0;
> -
>  	/*
>  	 * Set the display info, using edid if available, otherwise resetting
>  	 * the values to defaults. This duplicates the work done in
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index b1b2df48d42c..09a7d7f23e4a 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -1550,12 +1550,15 @@ struct drm_connector {
>  	struct drm_cmdline_mode cmdline_mode;
>  	/** @force: a DRM_FORCE_<foo> state for forced mode sets */
>  	enum drm_connector_force force;
> +
>  	/**
> -	 * @override_edid: has the EDID been overwritten through debugfs for
> -	 * testing? Do not modify outside of drm_edid_override_set() and
> -	 * drm_edid_override_reset().
> +	 * @edid_override: Override EDID set via debugfs.
> +	 *
> +	 * Do not modify or access outside of the drm_edid_override_* family of
> +	 * functions.
>  	 */
> -	bool override_edid;
> +	const struct drm_edid *edid_override;
> +
>  	/** @epoch_counter: used to detect any other changes in connector, besides status */
>  	u64 epoch_counter;
>  
> -- 
> 2.34.1

-- 
Ville Syrjälä
Intel

[PATCH 10/15] drm/edid/firmware: drop redundant connector_name variable/parameter

[Jani Nikula]

Stop passing around something that's readily available in
connector->name.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/drm_edid_load.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid_load.c b/drivers/gpu/drm/drm_edid_load.c
index 37d8ba3ddb46..13cdbfb991eb 100644
--- a/drivers/gpu/drm/drm_edid_load.c
+++ b/drivers/gpu/drm/drm_edid_load.c
@@ -167,8 +167,7 @@ static int edid_size(const u8 *edid, int data_size)
 	return (edid[0x7e] + 1) * EDID_LENGTH;
 }
 
-static void *edid_load(struct drm_connector *connector, const char *name,
-			const char *connector_name)
+static void *edid_load(struct drm_connector *connector, const char *name)
 {
 	const struct firmware *fw = NULL;
 	const u8 *fwdata;
@@ -185,10 +184,10 @@ static void *edid_load(struct drm_connector *connector, const char *name,
 		struct platform_device *pdev;
 		int err;
 
-		pdev = platform_device_register_simple(connector_name, -1, NULL, 0);
+		pdev = platform_device_register_simple(connector->name, -1, NULL, 0);
 		if (IS_ERR(pdev)) {
 			DRM_ERROR("Failed to register EDID firmware platform device "
-				  "for connector \"%s\"\n", connector_name);
+				  "for connector \"%s\"\n", connector->name);
 			return ERR_CAST(pdev);
 		}
 
@@ -244,7 +243,7 @@ static void *edid_load(struct drm_connector *connector, const char *name,
 		edid[EDID_LENGTH-1] += edid[0x7e] - valid_extensions;
 		DRM_INFO("Found %d valid extensions instead of %d in EDID data "
 		    "\"%s\" for connector \"%s\"\n", valid_extensions,
-		    edid[0x7e], name, connector_name);
+		    edid[0x7e], name, connector->name);
 		edid[0x7e] = valid_extensions;
 
 		new_edid = krealloc(edid, (valid_extensions + 1) * EDID_LENGTH,
@@ -256,7 +255,7 @@ static void *edid_load(struct drm_connector *connector, const char *name,
 	DRM_INFO("Got %s EDID base block and %d extension%s from "
 	    "\"%s\" for connector \"%s\"\n", (builtin >= 0) ? "built-in" :
 	    "external", valid_extensions, valid_extensions == 1 ? "" : "s",
-	    name, connector_name);
+	    name, connector->name);
 
 out:
 	release_firmware(fw);
@@ -265,7 +264,6 @@ static void *edid_load(struct drm_connector *connector, const char *name,
 
 struct edid *drm_load_edid_firmware(struct drm_connector *connector)
 {
-	const char *connector_name = connector->name;
 	char *edidname, *last, *colon, *fwstr, *edidstr, *fallback = NULL;
 	struct edid *edid;
 
@@ -288,7 +286,7 @@ struct edid *drm_load_edid_firmware(struct drm_connector *connector)
 	while ((edidname = strsep(&edidstr, ","))) {
 		colon = strchr(edidname, ':');
 		if (colon != NULL) {
-			if (strncmp(connector_name, edidname, colon - edidname))
+			if (strncmp(connector->name, edidname, colon - edidname))
 				continue;
 			edidname = colon + 1;
 			break;
@@ -310,7 +308,7 @@ struct edid *drm_load_edid_firmware(struct drm_connector *connector)
 	if (*last == '\n')
 		*last = '\0';
 
-	edid = edid_load(connector, edidname, connector_name);
+	edid = edid_load(connector, edidname);
 	kfree(fwstr);
 
 	return edid;
-- 
2.34.1

Re: [PATCH 10/15] drm/edid/firmware: drop redundant connector_name variable/parameter

[Ville Syrjälä]

On Tue, Oct 11, 2022 at 04:49:44PM +0300, Jani Nikula wrote:
> Stop passing around something that's readily available in
> connector->name.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

Weird. Did we not have a connector in there at some point or something?
Shrug.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/drm_edid_load.c | 16 +++++++---------
>  1 file changed, 7 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid_load.c b/drivers/gpu/drm/drm_edid_load.c
> index 37d8ba3ddb46..13cdbfb991eb 100644
> --- a/drivers/gpu/drm/drm_edid_load.c
> +++ b/drivers/gpu/drm/drm_edid_load.c
> @@ -167,8 +167,7 @@ static int edid_size(const u8 *edid, int data_size)
>  	return (edid[0x7e] + 1) * EDID_LENGTH;
>  }
>  
> -static void *edid_load(struct drm_connector *connector, const char *name,
> -			const char *connector_name)
> +static void *edid_load(struct drm_connector *connector, const char *name)
>  {
>  	const struct firmware *fw = NULL;
>  	const u8 *fwdata;
> @@ -185,10 +184,10 @@ static void *edid_load(struct drm_connector *connector, const char *name,
>  		struct platform_device *pdev;
>  		int err;
>  
> -		pdev = platform_device_register_simple(connector_name, -1, NULL, 0);
> +		pdev = platform_device_register_simple(connector->name, -1, NULL, 0);
>  		if (IS_ERR(pdev)) {
>  			DRM_ERROR("Failed to register EDID firmware platform device "
> -				  "for connector \"%s\"\n", connector_name);
> +				  "for connector \"%s\"\n", connector->name);
>  			return ERR_CAST(pdev);
>  		}
>  
> @@ -244,7 +243,7 @@ static void *edid_load(struct drm_connector *connector, const char *name,
>  		edid[EDID_LENGTH-1] += edid[0x7e] - valid_extensions;
>  		DRM_INFO("Found %d valid extensions instead of %d in EDID data "
>  		    "\"%s\" for connector \"%s\"\n", valid_extensions,
> -		    edid[0x7e], name, connector_name);
> +		    edid[0x7e], name, connector->name);
>  		edid[0x7e] = valid_extensions;
>  
>  		new_edid = krealloc(edid, (valid_extensions + 1) * EDID_LENGTH,
> @@ -256,7 +255,7 @@ static void *edid_load(struct drm_connector *connector, const char *name,
>  	DRM_INFO("Got %s EDID base block and %d extension%s from "
>  	    "\"%s\" for connector \"%s\"\n", (builtin >= 0) ? "built-in" :
>  	    "external", valid_extensions, valid_extensions == 1 ? "" : "s",
> -	    name, connector_name);
> +	    name, connector->name);
>  
>  out:
>  	release_firmware(fw);
> @@ -265,7 +264,6 @@ static void *edid_load(struct drm_connector *connector, const char *name,
>  
>  struct edid *drm_load_edid_firmware(struct drm_connector *connector)
>  {
> -	const char *connector_name = connector->name;
>  	char *edidname, *last, *colon, *fwstr, *edidstr, *fallback = NULL;
>  	struct edid *edid;
>  
> @@ -288,7 +286,7 @@ struct edid *drm_load_edid_firmware(struct drm_connector *connector)
>  	while ((edidname = strsep(&edidstr, ","))) {
>  		colon = strchr(edidname, ':');
>  		if (colon != NULL) {
> -			if (strncmp(connector_name, edidname, colon - edidname))
> +			if (strncmp(connector->name, edidname, colon - edidname))
>  				continue;
>  			edidname = colon + 1;
>  			break;
> @@ -310,7 +308,7 @@ struct edid *drm_load_edid_firmware(struct drm_connector *connector)
>  	if (*last == '\n')
>  		*last = '\0';
>  
> -	edid = edid_load(connector, edidname, connector_name);
> +	edid = edid_load(connector, edidname);
>  	kfree(fwstr);
>  
>  	return edid;
> -- 
> 2.34.1

-- 
Ville Syrjälä
Intel

[PATCH 11/15] drm/edid/firmware: rename drm_load_edid_firmware() to drm_edid_load_firmware()

[Jani Nikula]

Follow the usual naming convention by file name.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/drm_edid.c      | 10 +++++-----
 drivers/gpu/drm/drm_edid_load.c |  2 +-
 include/drm/drm_edid.h          |  4 ++--
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 0f2898badd51..2c66a901474d 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -2211,7 +2211,7 @@ static struct edid *drm_get_override_edid(struct drm_connector *connector,
 		override = drm_edid_duplicate(connector->edid_override->edid);
 
 	if (!override)
-		override = drm_load_edid_firmware(connector);
+		override = drm_edid_load_firmware(connector);
 
 	/* FIXME: Get alloc size from deeper down the stack */
 	if (!IS_ERR_OR_NULL(override) && alloc_size)
@@ -2443,7 +2443,7 @@ static struct edid *_drm_do_get_edid(struct drm_connector *connector,
  * adapter and use drm_get_edid() instead of abusing this function.
  *
  * The EDID may be overridden using debugfs override_edid or firmware EDID
- * (drm_load_edid_firmware() and drm.edid_firmware parameter), in this priority
+ * (drm_edid_load_firmware() and drm.edid_firmware parameter), in this priority
  * order. Having either of them bypasses actual EDID reads.
  *
  * Return: Pointer to valid EDID or NULL if we couldn't find any.
@@ -2621,7 +2621,7 @@ EXPORT_SYMBOL(drm_get_edid);
  * this function.
  *
  * The EDID may be overridden using debugfs override_edid or firmware EDID
- * (drm_load_edid_firmware() and drm.edid_firmware parameter), in this priority
+ * (drm_edid_load_firmware() and drm.edid_firmware parameter), in this priority
  * order. Having either of them bypasses actual EDID reads.
  *
  * The returned pointer must be freed using drm_edid_free().
@@ -2659,7 +2659,7 @@ EXPORT_SYMBOL(drm_edid_read_custom);
  * Read EDID using the given I2C adapter.
  *
  * The EDID may be overridden using debugfs override_edid or firmware EDID
- * (drm_load_edid_firmware() and drm.edid_firmware parameter), in this priority
+ * (drm_edid_load_firmware() and drm.edid_firmware parameter), in this priority
  * order. Having either of them bypasses actual EDID reads.
  *
  * Prefer initializing connector->ddc with drm_connector_init_with_ddc() and
@@ -2695,7 +2695,7 @@ EXPORT_SYMBOL(drm_edid_read_ddc);
  * Read EDID using the connector's I2C adapter.
  *
  * The EDID may be overridden using debugfs override_edid or firmware EDID
- * (drm_load_edid_firmware() and drm.edid_firmware parameter), in this priority
+ * (drm_edid_load_firmware() and drm.edid_firmware parameter), in this priority
  * order. Having either of them bypasses actual EDID reads.
  *
  * The returned pointer must be freed using drm_edid_free().
diff --git a/drivers/gpu/drm/drm_edid_load.c b/drivers/gpu/drm/drm_edid_load.c
index 13cdbfb991eb..bc6b96abd1b3 100644
--- a/drivers/gpu/drm/drm_edid_load.c
+++ b/drivers/gpu/drm/drm_edid_load.c
@@ -262,7 +262,7 @@ static void *edid_load(struct drm_connector *connector, const char *name)
 	return edid;
 }
 
-struct edid *drm_load_edid_firmware(struct drm_connector *connector)
+struct edid *drm_edid_load_firmware(struct drm_connector *connector)
 {
 	char *edidname, *last, *colon, *fwstr, *edidstr, *fallback = NULL;
 	struct edid *edid;
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index a2e25e7e6ee5..dc7467d25cd8 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -388,12 +388,12 @@ int drm_av_sync_delay(struct drm_connector *connector,
 		      const struct drm_display_mode *mode);
 
 #ifdef CONFIG_DRM_LOAD_EDID_FIRMWARE
-struct edid *drm_load_edid_firmware(struct drm_connector *connector);
+struct edid *drm_edid_load_firmware(struct drm_connector *connector);
 int __drm_set_edid_firmware_path(const char *path);
 int __drm_get_edid_firmware_path(char *buf, size_t bufsize);
 #else
 static inline struct edid *
-drm_load_edid_firmware(struct drm_connector *connector)
+drm_edid_load_firmware(struct drm_connector *connector)
 {
 	return ERR_PTR(-ENOENT);
 }
-- 
2.34.1

Re: [PATCH 11/15] drm/edid/firmware: rename drm_load_edid_firmware() to drm_edid_load_firmware()

[Ville Syrjälä]

On Tue, Oct 11, 2022 at 04:49:45PM +0300, Jani Nikula wrote:
> Follow the usual naming convention by file name.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/drm_edid.c      | 10 +++++-----
>  drivers/gpu/drm/drm_edid_load.c |  2 +-
>  include/drm/drm_edid.h          |  4 ++--
>  3 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 0f2898badd51..2c66a901474d 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -2211,7 +2211,7 @@ static struct edid *drm_get_override_edid(struct drm_connector *connector,
>  		override = drm_edid_duplicate(connector->edid_override->edid);
>  
>  	if (!override)
> -		override = drm_load_edid_firmware(connector);
> +		override = drm_edid_load_firmware(connector);
>  
>  	/* FIXME: Get alloc size from deeper down the stack */
>  	if (!IS_ERR_OR_NULL(override) && alloc_size)
> @@ -2443,7 +2443,7 @@ static struct edid *_drm_do_get_edid(struct drm_connector *connector,
>   * adapter and use drm_get_edid() instead of abusing this function.
>   *
>   * The EDID may be overridden using debugfs override_edid or firmware EDID
> - * (drm_load_edid_firmware() and drm.edid_firmware parameter), in this priority
> + * (drm_edid_load_firmware() and drm.edid_firmware parameter), in this priority
>   * order. Having either of them bypasses actual EDID reads.
>   *
>   * Return: Pointer to valid EDID or NULL if we couldn't find any.
> @@ -2621,7 +2621,7 @@ EXPORT_SYMBOL(drm_get_edid);
>   * this function.
>   *
>   * The EDID may be overridden using debugfs override_edid or firmware EDID
> - * (drm_load_edid_firmware() and drm.edid_firmware parameter), in this priority
> + * (drm_edid_load_firmware() and drm.edid_firmware parameter), in this priority
>   * order. Having either of them bypasses actual EDID reads.
>   *
>   * The returned pointer must be freed using drm_edid_free().
> @@ -2659,7 +2659,7 @@ EXPORT_SYMBOL(drm_edid_read_custom);
>   * Read EDID using the given I2C adapter.
>   *
>   * The EDID may be overridden using debugfs override_edid or firmware EDID
> - * (drm_load_edid_firmware() and drm.edid_firmware parameter), in this priority
> + * (drm_edid_load_firmware() and drm.edid_firmware parameter), in this priority
>   * order. Having either of them bypasses actual EDID reads.
>   *
>   * Prefer initializing connector->ddc with drm_connector_init_with_ddc() and
> @@ -2695,7 +2695,7 @@ EXPORT_SYMBOL(drm_edid_read_ddc);
>   * Read EDID using the connector's I2C adapter.
>   *
>   * The EDID may be overridden using debugfs override_edid or firmware EDID
> - * (drm_load_edid_firmware() and drm.edid_firmware parameter), in this priority
> + * (drm_edid_load_firmware() and drm.edid_firmware parameter), in this priority
>   * order. Having either of them bypasses actual EDID reads.
>   *
>   * The returned pointer must be freed using drm_edid_free().
> diff --git a/drivers/gpu/drm/drm_edid_load.c b/drivers/gpu/drm/drm_edid_load.c
> index 13cdbfb991eb..bc6b96abd1b3 100644
> --- a/drivers/gpu/drm/drm_edid_load.c
> +++ b/drivers/gpu/drm/drm_edid_load.c
> @@ -262,7 +262,7 @@ static void *edid_load(struct drm_connector *connector, const char *name)
>  	return edid;
>  }
>  
> -struct edid *drm_load_edid_firmware(struct drm_connector *connector)
> +struct edid *drm_edid_load_firmware(struct drm_connector *connector)
>  {
>  	char *edidname, *last, *colon, *fwstr, *edidstr, *fallback = NULL;
>  	struct edid *edid;
> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> index a2e25e7e6ee5..dc7467d25cd8 100644
> --- a/include/drm/drm_edid.h
> +++ b/include/drm/drm_edid.h
> @@ -388,12 +388,12 @@ int drm_av_sync_delay(struct drm_connector *connector,
>  		      const struct drm_display_mode *mode);
>  
>  #ifdef CONFIG_DRM_LOAD_EDID_FIRMWARE
> -struct edid *drm_load_edid_firmware(struct drm_connector *connector);
> +struct edid *drm_edid_load_firmware(struct drm_connector *connector);
>  int __drm_set_edid_firmware_path(const char *path);
>  int __drm_get_edid_firmware_path(char *buf, size_t bufsize);
>  #else
>  static inline struct edid *
> -drm_load_edid_firmware(struct drm_connector *connector)
> +drm_edid_load_firmware(struct drm_connector *connector)
>  {
>  	return ERR_PTR(-ENOENT);
>  }
> -- 
> 2.34.1

-- 
Ville Syrjälä
Intel

[PATCH 12/15] drm/edid: use struct drm_edid for override/firmware EDID

[Jani Nikula]

There's a lot going on here, but the main thing is switching the
firmware EDID loader to use struct drm_edid. Unfortunately, it's
difficult to reasonably split to smaller pieces.

Convert the EDID loader to struct drm_edid. There's a functional change
in validation; it no longer tries to fix errors or filter invalid
blocks. It's stricter in this sense. Hopefully this will not be an
issue.

As a by-product, this change also allows HF-EEODB extended EDIDs to be
passed via override/firmware EDID.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/drm_edid.c      | 32 ++++++------
 drivers/gpu/drm/drm_edid_load.c | 86 +++++++--------------------------
 include/drm/drm_edid.h          |  4 +-
 3 files changed, 36 insertions(+), 86 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 2c66a901474d..935bdf4e6269 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -2202,21 +2202,16 @@ static void connector_bad_edid(struct drm_connector *connector,
 }
 
 /* Get override or firmware EDID */
-static struct edid *drm_get_override_edid(struct drm_connector *connector,
-					  size_t *alloc_size)
+static const struct drm_edid *drm_edid_override_get(struct drm_connector *connector)
 {
-	struct edid *override = NULL;
+	const struct drm_edid *override = NULL;
 
 	if (connector->edid_override)
-		override = drm_edid_duplicate(connector->edid_override->edid);
+		override = drm_edid_dup(connector->edid_override);
 
 	if (!override)
 		override = drm_edid_load_firmware(connector);
 
-	/* FIXME: Get alloc size from deeper down the stack */
-	if (!IS_ERR_OR_NULL(override) && alloc_size)
-		*alloc_size = edid_size(override);
-
 	return IS_ERR(override) ? NULL : override;
 }
 
@@ -2277,14 +2272,14 @@ int drm_edid_override_reset(struct drm_connector *connector)
  */
 int drm_edid_override_connector_update(struct drm_connector *connector)
 {
-	struct edid *override;
+	const struct drm_edid *override;
 	int num_modes = 0;
 
-	override = drm_get_override_edid(connector, NULL);
+	override = drm_edid_override_get(connector);
 	if (override) {
-		drm_connector_update_edid_property(connector, override);
-		num_modes = drm_add_edid_modes(connector, override);
-		kfree(override);
+		num_modes = drm_edid_connector_update(connector, override);
+
+		drm_edid_free(override);
 
 		DRM_DEBUG_KMS("[CONNECTOR:%d:%s] adding %d modes via fallback override/firmware EDID\n",
 			      connector->base.id, connector->name, num_modes);
@@ -2335,12 +2330,19 @@ static struct edid *_drm_do_get_edid(struct drm_connector *connector,
 {
 	enum edid_block_status status;
 	int i, num_blocks, invalid_blocks = 0;
+	const struct drm_edid *override;
 	struct edid *edid, *new;
 	size_t alloc_size = EDID_LENGTH;
 
-	edid = drm_get_override_edid(connector, &alloc_size);
-	if (edid)
+	override = drm_edid_override_get(connector);
+	if (override) {
+		alloc_size = override->size;
+		edid = kmemdup(override->edid, alloc_size, GFP_KERNEL);
+		drm_edid_free(override);
+		if (!edid)
+			return NULL;
 		goto ok;
+	}
 
 	edid = kmalloc(alloc_size, GFP_KERNEL);
 	if (!edid)
diff --git a/drivers/gpu/drm/drm_edid_load.c b/drivers/gpu/drm/drm_edid_load.c
index bc6b96abd1b3..248f0685c33e 100644
--- a/drivers/gpu/drm/drm_edid_load.c
+++ b/drivers/gpu/drm/drm_edid_load.c
@@ -159,22 +159,12 @@ static const u8 generic_edid[GENERIC_EDIDS][128] = {
 	},
 };
 
-static int edid_size(const u8 *edid, int data_size)
-{
-	if (data_size < EDID_LENGTH)
-		return 0;
-
-	return (edid[0x7e] + 1) * EDID_LENGTH;
-}
-
-static void *edid_load(struct drm_connector *connector, const char *name)
+static const struct drm_edid *edid_load(struct drm_connector *connector, const char *name)
 {
 	const struct firmware *fw = NULL;
 	const u8 *fwdata;
-	u8 *edid;
+	const struct drm_edid *drm_edid;
 	int fwsize, builtin;
-	int i, valid_extensions = 0;
-	bool print_bad_edid = !connector->bad_edid_counter || drm_debug_enabled(DRM_UT_KMS);
 
 	builtin = match_string(generic_edid_name, GENERIC_EDIDS, name);
 	if (builtin >= 0) {
@@ -203,69 +193,26 @@ static void *edid_load(struct drm_connector *connector, const char *name)
 		fwsize = fw->size;
 	}
 
-	if (edid_size(fwdata, fwsize) != fwsize) {
-		DRM_ERROR("Size of EDID firmware \"%s\" is invalid "
-			  "(expected %d, got %d\n", name,
-			  edid_size(fwdata, fwsize), (int)fwsize);
-		edid = ERR_PTR(-EINVAL);
-		goto out;
-	}
-
-	edid = kmemdup(fwdata, fwsize, GFP_KERNEL);
-	if (edid == NULL) {
-		edid = ERR_PTR(-ENOMEM);
-		goto out;
-	}
-
-	if (!drm_edid_block_valid(edid, 0, print_bad_edid,
-				  &connector->edid_corrupt)) {
-		connector->bad_edid_counter++;
-		DRM_ERROR("Base block of EDID firmware \"%s\" is invalid ",
-		    name);
-		kfree(edid);
-		edid = ERR_PTR(-EINVAL);
-		goto out;
-	}
-
-	for (i = 1; i <= edid[0x7e]; i++) {
-		if (i != valid_extensions + 1)
-			memcpy(edid + (valid_extensions + 1) * EDID_LENGTH,
-			    edid + i * EDID_LENGTH, EDID_LENGTH);
-		if (drm_edid_block_valid(edid + i * EDID_LENGTH, i,
-					 print_bad_edid,
-					 NULL))
-			valid_extensions++;
-	}
-
-	if (valid_extensions != edid[0x7e]) {
-		u8 *new_edid;
+	drm_dbg_kms(connector->dev, "[CONNECTOR:%d:%s] Loaded %s firmware EDID \"%s\"\n",
+		    connector->base.id, connector->name,
+		    builtin >= 0 ? "built-in" : "external", name);
 
-		edid[EDID_LENGTH-1] += edid[0x7e] - valid_extensions;
-		DRM_INFO("Found %d valid extensions instead of %d in EDID data "
-		    "\"%s\" for connector \"%s\"\n", valid_extensions,
-		    edid[0x7e], name, connector->name);
-		edid[0x7e] = valid_extensions;
-
-		new_edid = krealloc(edid, (valid_extensions + 1) * EDID_LENGTH,
-				    GFP_KERNEL);
-		if (new_edid)
-			edid = new_edid;
+	drm_edid = drm_edid_alloc(fwdata, fwsize);
+	if (!drm_edid_valid(drm_edid)) {
+		drm_err(connector->dev, "Invalid firmware EDID \"%s\"\n", name);
+		drm_edid_free(drm_edid);
+		drm_edid = ERR_PTR(-EINVAL);
 	}
 
-	DRM_INFO("Got %s EDID base block and %d extension%s from "
-	    "\"%s\" for connector \"%s\"\n", (builtin >= 0) ? "built-in" :
-	    "external", valid_extensions, valid_extensions == 1 ? "" : "s",
-	    name, connector->name);
-
-out:
 	release_firmware(fw);
-	return edid;
+
+	return drm_edid;
 }
 
-struct edid *drm_edid_load_firmware(struct drm_connector *connector)
+const struct drm_edid *drm_edid_load_firmware(struct drm_connector *connector)
 {
 	char *edidname, *last, *colon, *fwstr, *edidstr, *fallback = NULL;
-	struct edid *edid;
+	const struct drm_edid *drm_edid;
 
 	if (edid_firmware[0] == '\0')
 		return ERR_PTR(-ENOENT);
@@ -308,8 +255,9 @@ struct edid *drm_edid_load_firmware(struct drm_connector *connector)
 	if (*last == '\n')
 		*last = '\0';
 
-	edid = edid_load(connector, edidname);
+	drm_edid = edid_load(connector, edidname);
+
 	kfree(fwstr);
 
-	return edid;
+	return drm_edid;
 }
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index dc7467d25cd8..8138613f4e4e 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -388,11 +388,11 @@ int drm_av_sync_delay(struct drm_connector *connector,
 		      const struct drm_display_mode *mode);
 
 #ifdef CONFIG_DRM_LOAD_EDID_FIRMWARE
-struct edid *drm_edid_load_firmware(struct drm_connector *connector);
+const struct drm_edid *drm_edid_load_firmware(struct drm_connector *connector);
 int __drm_set_edid_firmware_path(const char *path);
 int __drm_get_edid_firmware_path(char *buf, size_t bufsize);
 #else
-static inline struct edid *
+static inline const struct drm_edid *
 drm_edid_load_firmware(struct drm_connector *connector)
 {
 	return ERR_PTR(-ENOENT);
-- 
2.34.1

Re: [PATCH 12/15] drm/edid: use struct drm_edid for override/firmware EDID

[Ville Syrjälä]

On Tue, Oct 11, 2022 at 04:49:46PM +0300, Jani Nikula wrote:
> There's a lot going on here, but the main thing is switching the
> firmware EDID loader to use struct drm_edid. Unfortunately, it's
> difficult to reasonably split to smaller pieces.
> 
> Convert the EDID loader to struct drm_edid. There's a functional change
> in validation; it no longer tries to fix errors or filter invalid
> blocks. It's stricter in this sense. Hopefully this will not be an
> issue.
> 
> As a by-product, this change also allows HF-EEODB extended EDIDs to be
> passed via override/firmware EDID.

Was pondering about that reading the earlier patch. Figured I'd keep
on reading, and voila here it is.

> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/drm_edid.c      | 32 ++++++------
>  drivers/gpu/drm/drm_edid_load.c | 86 +++++++--------------------------
>  include/drm/drm_edid.h          |  4 +-
>  3 files changed, 36 insertions(+), 86 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 2c66a901474d..935bdf4e6269 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -2202,21 +2202,16 @@ static void connector_bad_edid(struct drm_connector *connector,
>  }
>  
>  /* Get override or firmware EDID */
> -static struct edid *drm_get_override_edid(struct drm_connector *connector,
> -					  size_t *alloc_size)
> +static const struct drm_edid *drm_edid_override_get(struct drm_connector *connector)
>  {
> -	struct edid *override = NULL;
> +	const struct drm_edid *override = NULL;
>  
>  	if (connector->edid_override)
> -		override = drm_edid_duplicate(connector->edid_override->edid);
> +		override = drm_edid_dup(connector->edid_override);
>  
>  	if (!override)
>  		override = drm_edid_load_firmware(connector);
>  
> -	/* FIXME: Get alloc size from deeper down the stack */
> -	if (!IS_ERR_OR_NULL(override) && alloc_size)
> -		*alloc_size = edid_size(override);
> -
>  	return IS_ERR(override) ? NULL : override;
>  }
>  
> @@ -2277,14 +2272,14 @@ int drm_edid_override_reset(struct drm_connector *connector)
>   */
>  int drm_edid_override_connector_update(struct drm_connector *connector)
>  {
> -	struct edid *override;
> +	const struct drm_edid *override;
>  	int num_modes = 0;
>  
> -	override = drm_get_override_edid(connector, NULL);
> +	override = drm_edid_override_get(connector);
>  	if (override) {
> -		drm_connector_update_edid_property(connector, override);
> -		num_modes = drm_add_edid_modes(connector, override);
> -		kfree(override);
> +		num_modes = drm_edid_connector_update(connector, override);
> +
> +		drm_edid_free(override);
>  
>  		DRM_DEBUG_KMS("[CONNECTOR:%d:%s] adding %d modes via fallback override/firmware EDID\n",
>  			      connector->base.id, connector->name, num_modes);
> @@ -2335,12 +2330,19 @@ static struct edid *_drm_do_get_edid(struct drm_connector *connector,
>  {
>  	enum edid_block_status status;
>  	int i, num_blocks, invalid_blocks = 0;
> +	const struct drm_edid *override;
>  	struct edid *edid, *new;
>  	size_t alloc_size = EDID_LENGTH;
>  
> -	edid = drm_get_override_edid(connector, &alloc_size);
> -	if (edid)
> +	override = drm_edid_override_get(connector);
> +	if (override) {
> +		alloc_size = override->size;
> +		edid = kmemdup(override->edid, alloc_size, GFP_KERNEL);
> +		drm_edid_free(override);
> +		if (!edid)
> +			return NULL;
>  		goto ok;
> +	}
>  
>  	edid = kmalloc(alloc_size, GFP_KERNEL);
>  	if (!edid)
> diff --git a/drivers/gpu/drm/drm_edid_load.c b/drivers/gpu/drm/drm_edid_load.c
> index bc6b96abd1b3..248f0685c33e 100644
> --- a/drivers/gpu/drm/drm_edid_load.c
> +++ b/drivers/gpu/drm/drm_edid_load.c
> @@ -159,22 +159,12 @@ static const u8 generic_edid[GENERIC_EDIDS][128] = {
>  	},
>  };
>  
> -static int edid_size(const u8 *edid, int data_size)
> -{
> -	if (data_size < EDID_LENGTH)
> -		return 0;
> -
> -	return (edid[0x7e] + 1) * EDID_LENGTH;
> -}
> -
> -static void *edid_load(struct drm_connector *connector, const char *name)
> +static const struct drm_edid *edid_load(struct drm_connector *connector, const char *name)
>  {
>  	const struct firmware *fw = NULL;
>  	const u8 *fwdata;
> -	u8 *edid;
> +	const struct drm_edid *drm_edid;
>  	int fwsize, builtin;
> -	int i, valid_extensions = 0;
> -	bool print_bad_edid = !connector->bad_edid_counter || drm_debug_enabled(DRM_UT_KMS);
>  
>  	builtin = match_string(generic_edid_name, GENERIC_EDIDS, name);
>  	if (builtin >= 0) {
> @@ -203,69 +193,26 @@ static void *edid_load(struct drm_connector *connector, const char *name)
>  		fwsize = fw->size;
>  	}
>  
> -	if (edid_size(fwdata, fwsize) != fwsize) {
> -		DRM_ERROR("Size of EDID firmware \"%s\" is invalid "
> -			  "(expected %d, got %d\n", name,
> -			  edid_size(fwdata, fwsize), (int)fwsize);
> -		edid = ERR_PTR(-EINVAL);
> -		goto out;
> -	}
> -
> -	edid = kmemdup(fwdata, fwsize, GFP_KERNEL);
> -	if (edid == NULL) {
> -		edid = ERR_PTR(-ENOMEM);
> -		goto out;
> -	}
> -
> -	if (!drm_edid_block_valid(edid, 0, print_bad_edid,
> -				  &connector->edid_corrupt)) {
> -		connector->bad_edid_counter++;
> -		DRM_ERROR("Base block of EDID firmware \"%s\" is invalid ",
> -		    name);
> -		kfree(edid);
> -		edid = ERR_PTR(-EINVAL);
> -		goto out;
> -	}
> -
> -	for (i = 1; i <= edid[0x7e]; i++) {
> -		if (i != valid_extensions + 1)
> -			memcpy(edid + (valid_extensions + 1) * EDID_LENGTH,
> -			    edid + i * EDID_LENGTH, EDID_LENGTH);
> -		if (drm_edid_block_valid(edid + i * EDID_LENGTH, i,
> -					 print_bad_edid,
> -					 NULL))
> -			valid_extensions++;
> -	}
> -
> -	if (valid_extensions != edid[0x7e]) {
> -		u8 *new_edid;
> +	drm_dbg_kms(connector->dev, "[CONNECTOR:%d:%s] Loaded %s firmware EDID \"%s\"\n",
> +		    connector->base.id, connector->name,
> +		    builtin >= 0 ? "built-in" : "external", name);
>  
> -		edid[EDID_LENGTH-1] += edid[0x7e] - valid_extensions;
> -		DRM_INFO("Found %d valid extensions instead of %d in EDID data "
> -		    "\"%s\" for connector \"%s\"\n", valid_extensions,
> -		    edid[0x7e], name, connector->name);
> -		edid[0x7e] = valid_extensions;
> -
> -		new_edid = krealloc(edid, (valid_extensions + 1) * EDID_LENGTH,
> -				    GFP_KERNEL);
> -		if (new_edid)
> -			edid = new_edid;
> +	drm_edid = drm_edid_alloc(fwdata, fwsize);
> +	if (!drm_edid_valid(drm_edid)) {
> +		drm_err(connector->dev, "Invalid firmware EDID \"%s\"\n", name);
> +		drm_edid_free(drm_edid);
> +		drm_edid = ERR_PTR(-EINVAL);
>  	}

Lovely diffstat ratio there.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

>  
> -	DRM_INFO("Got %s EDID base block and %d extension%s from "
> -	    "\"%s\" for connector \"%s\"\n", (builtin >= 0) ? "built-in" :
> -	    "external", valid_extensions, valid_extensions == 1 ? "" : "s",
> -	    name, connector->name);
> -
> -out:
>  	release_firmware(fw);
> -	return edid;
> +
> +	return drm_edid;
>  }
>  
> -struct edid *drm_edid_load_firmware(struct drm_connector *connector)
> +const struct drm_edid *drm_edid_load_firmware(struct drm_connector *connector)
>  {
>  	char *edidname, *last, *colon, *fwstr, *edidstr, *fallback = NULL;
> -	struct edid *edid;
> +	const struct drm_edid *drm_edid;
>  
>  	if (edid_firmware[0] == '\0')
>  		return ERR_PTR(-ENOENT);
> @@ -308,8 +255,9 @@ struct edid *drm_edid_load_firmware(struct drm_connector *connector)
>  	if (*last == '\n')
>  		*last = '\0';
>  
> -	edid = edid_load(connector, edidname);
> +	drm_edid = edid_load(connector, edidname);
> +
>  	kfree(fwstr);
>  
> -	return edid;
> +	return drm_edid;
>  }
> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> index dc7467d25cd8..8138613f4e4e 100644
> --- a/include/drm/drm_edid.h
> +++ b/include/drm/drm_edid.h
> @@ -388,11 +388,11 @@ int drm_av_sync_delay(struct drm_connector *connector,
>  		      const struct drm_display_mode *mode);
>  
>  #ifdef CONFIG_DRM_LOAD_EDID_FIRMWARE
> -struct edid *drm_edid_load_firmware(struct drm_connector *connector);
> +const struct drm_edid *drm_edid_load_firmware(struct drm_connector *connector);
>  int __drm_set_edid_firmware_path(const char *path);
>  int __drm_get_edid_firmware_path(char *buf, size_t bufsize);
>  #else
> -static inline struct edid *
> +static inline const struct drm_edid *
>  drm_edid_load_firmware(struct drm_connector *connector)
>  {
>  	return ERR_PTR(-ENOENT);
> -- 
> 2.34.1

-- 
Ville Syrjälä
Intel

[PATCH 13/15] drm/edid: move edid load declarations to internal header

[Jani Nikula]

The EDID loader is internal to drm, not for drivers.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/drm_crtc_internal.h | 11 +++++++++++
 drivers/gpu/drm/drm_edid_load.c     |  5 +++--
 include/drm/drm_edid.h              |  7 -------
 3 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h
index fb8a68d90940..501a10edd0e1 100644
--- a/drivers/gpu/drm/drm_crtc_internal.h
+++ b/drivers/gpu/drm/drm_crtc_internal.h
@@ -290,3 +290,14 @@ void drm_mode_fixup_1366x768(struct drm_display_mode *mode);
 int drm_edid_override_show(struct drm_connector *connector, struct seq_file *m);
 int drm_edid_override_set(struct drm_connector *connector, const void *edid, size_t size);
 int drm_edid_override_reset(struct drm_connector *connector);
+
+/* drm_edid_load.c */
+#ifdef CONFIG_DRM_LOAD_EDID_FIRMWARE
+const struct drm_edid *drm_edid_load_firmware(struct drm_connector *connector);
+#else
+static inline const struct drm_edid *
+drm_edid_load_firmware(struct drm_connector *connector)
+{
+	return ERR_PTR(-ENOENT);
+}
+#endif
diff --git a/drivers/gpu/drm/drm_edid_load.c b/drivers/gpu/drm/drm_edid_load.c
index 248f0685c33e..882caaa6e663 100644
--- a/drivers/gpu/drm/drm_edid_load.c
+++ b/drivers/gpu/drm/drm_edid_load.c
@@ -11,12 +11,13 @@
 #include <linux/module.h>
 #include <linux/platform_device.h>
 
-#include <drm/drm_crtc.h>
-#include <drm/drm_crtc_helper.h>
+#include <drm/drm_connector.h>
 #include <drm/drm_drv.h>
 #include <drm/drm_edid.h>
 #include <drm/drm_print.h>
 
+#include "drm_crtc_internal.h"
+
 static char edid_firmware[PATH_MAX];
 module_param_string(edid_firmware, edid_firmware, sizeof(edid_firmware), 0644);
 MODULE_PARM_DESC(edid_firmware, "Do not probe monitor, use specified EDID blob "
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index 8138613f4e4e..372963600f1d 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -388,15 +388,8 @@ int drm_av_sync_delay(struct drm_connector *connector,
 		      const struct drm_display_mode *mode);
 
 #ifdef CONFIG_DRM_LOAD_EDID_FIRMWARE
-const struct drm_edid *drm_edid_load_firmware(struct drm_connector *connector);
 int __drm_set_edid_firmware_path(const char *path);
 int __drm_get_edid_firmware_path(char *buf, size_t bufsize);
-#else
-static inline const struct drm_edid *
-drm_edid_load_firmware(struct drm_connector *connector)
-{
-	return ERR_PTR(-ENOENT);
-}
 #endif
 
 bool drm_edid_are_equal(const struct edid *edid1, const struct edid *edid2);
-- 
2.34.1

Re: [PATCH 13/15] drm/edid: move edid load declarations to internal header

[Ville Syrjälä]

On Tue, Oct 11, 2022 at 04:49:47PM +0300, Jani Nikula wrote:
> The EDID loader is internal to drm, not for drivers.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/drm_crtc_internal.h | 11 +++++++++++
>  drivers/gpu/drm/drm_edid_load.c     |  5 +++--
>  include/drm/drm_edid.h              |  7 -------
>  3 files changed, 14 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h
> index fb8a68d90940..501a10edd0e1 100644
> --- a/drivers/gpu/drm/drm_crtc_internal.h
> +++ b/drivers/gpu/drm/drm_crtc_internal.h
> @@ -290,3 +290,14 @@ void drm_mode_fixup_1366x768(struct drm_display_mode *mode);
>  int drm_edid_override_show(struct drm_connector *connector, struct seq_file *m);
>  int drm_edid_override_set(struct drm_connector *connector, const void *edid, size_t size);
>  int drm_edid_override_reset(struct drm_connector *connector);
> +
> +/* drm_edid_load.c */
> +#ifdef CONFIG_DRM_LOAD_EDID_FIRMWARE
> +const struct drm_edid *drm_edid_load_firmware(struct drm_connector *connector);
> +#else
> +static inline const struct drm_edid *
> +drm_edid_load_firmware(struct drm_connector *connector)
> +{
> +	return ERR_PTR(-ENOENT);
> +}
> +#endif
> diff --git a/drivers/gpu/drm/drm_edid_load.c b/drivers/gpu/drm/drm_edid_load.c
> index 248f0685c33e..882caaa6e663 100644
> --- a/drivers/gpu/drm/drm_edid_load.c
> +++ b/drivers/gpu/drm/drm_edid_load.c
> @@ -11,12 +11,13 @@
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
>  
> -#include <drm/drm_crtc.h>
> -#include <drm/drm_crtc_helper.h>
> +#include <drm/drm_connector.h>
>  #include <drm/drm_drv.h>
>  #include <drm/drm_edid.h>
>  #include <drm/drm_print.h>
>  
> +#include "drm_crtc_internal.h"
> +
>  static char edid_firmware[PATH_MAX];
>  module_param_string(edid_firmware, edid_firmware, sizeof(edid_firmware), 0644);
>  MODULE_PARM_DESC(edid_firmware, "Do not probe monitor, use specified EDID blob "
> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> index 8138613f4e4e..372963600f1d 100644
> --- a/include/drm/drm_edid.h
> +++ b/include/drm/drm_edid.h
> @@ -388,15 +388,8 @@ int drm_av_sync_delay(struct drm_connector *connector,
>  		      const struct drm_display_mode *mode);
>  
>  #ifdef CONFIG_DRM_LOAD_EDID_FIRMWARE
> -const struct drm_edid *drm_edid_load_firmware(struct drm_connector *connector);
>  int __drm_set_edid_firmware_path(const char *path);
>  int __drm_get_edid_firmware_path(char *buf, size_t bufsize);
> -#else
> -static inline const struct drm_edid *
> -drm_edid_load_firmware(struct drm_connector *connector)
> -{
> -	return ERR_PTR(-ENOENT);
> -}
>  #endif
>  
>  bool drm_edid_are_equal(const struct edid *edid1, const struct edid *edid2);
> -- 
> 2.34.1

-- 
Ville Syrjälä
Intel

[PATCH 14/15] drm/edid/firmware: convert to drm device specific logging

[Jani Nikula]

Conform to device specific logging.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/drm_edid_load.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid_load.c b/drivers/gpu/drm/drm_edid_load.c
index 882caaa6e663..dd472c66cb3c 100644
--- a/drivers/gpu/drm/drm_edid_load.c
+++ b/drivers/gpu/drm/drm_edid_load.c
@@ -177,16 +177,18 @@ static const struct drm_edid *edid_load(struct drm_connector *connector, const c
 
 		pdev = platform_device_register_simple(connector->name, -1, NULL, 0);
 		if (IS_ERR(pdev)) {
-			DRM_ERROR("Failed to register EDID firmware platform device "
-				  "for connector \"%s\"\n", connector->name);
+			drm_err(connector->dev,
+				"Failed to register EDID firmware platform device for connector \"%s\"\n",
+				connector->name);
 			return ERR_CAST(pdev);
 		}
 
 		err = request_firmware(&fw, name, &pdev->dev);
 		platform_device_unregister(pdev);
 		if (err) {
-			DRM_ERROR("Requesting EDID firmware \"%s\" failed (err=%d)\n",
-				  name, err);
+			drm_err(connector->dev,
+				"Requesting EDID firmware \"%s\" failed (err=%d)\n",
+				name, err);
 			return ERR_PTR(err);
 		}
 
-- 
2.34.1

Re: [PATCH 14/15] drm/edid/firmware: convert to drm device specific logging

[Ville Syrjälä]

On Tue, Oct 11, 2022 at 04:49:48PM +0300, Jani Nikula wrote:
> Conform to device specific logging.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/drm_edid_load.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid_load.c b/drivers/gpu/drm/drm_edid_load.c
> index 882caaa6e663..dd472c66cb3c 100644
> --- a/drivers/gpu/drm/drm_edid_load.c
> +++ b/drivers/gpu/drm/drm_edid_load.c
> @@ -177,16 +177,18 @@ static const struct drm_edid *edid_load(struct drm_connector *connector, const c
>  
>  		pdev = platform_device_register_simple(connector->name, -1, NULL, 0);
>  		if (IS_ERR(pdev)) {
> -			DRM_ERROR("Failed to register EDID firmware platform device "
> -				  "for connector \"%s\"\n", connector->name);
> +			drm_err(connector->dev,
> +				"Failed to register EDID firmware platform device for connector \"%s\"\n",
> +				connector->name);

Can go for the full [CONNECTOR:...] thing?

>  			return ERR_CAST(pdev);
>  		}
>  
>  		err = request_firmware(&fw, name, &pdev->dev);
>  		platform_device_unregister(pdev);
>  		if (err) {
> -			DRM_ERROR("Requesting EDID firmware \"%s\" failed (err=%d)\n",
> -				  name, err);
> +			drm_err(connector->dev,
> +				"Requesting EDID firmware \"%s\" failed (err=%d)\n",
> +				name, err);
>  			return ERR_PTR(err);
>  		}
>  
> -- 
> 2.34.1

-- 
Ville Syrjälä
Intel

[PATCH 15/15] drm/edid: convert to device specific logging

[Jani Nikula]

Convert to drm_kms_dbg/drm_err where possible, and unify the rest of the
debugs to DRM_DEBUG_KMS.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/drm_edid.c | 105 ++++++++++++++++++-------------------
 1 file changed, 50 insertions(+), 55 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 935bdf4e6269..09b0b8337382 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -1979,7 +1979,7 @@ bool drm_edid_block_valid(u8 *_block, int block_num, bool print_bad_edid,
 
 	status = edid_block_check(block, is_base_block);
 	if (status == EDID_BLOCK_HEADER_REPAIR) {
-		DRM_DEBUG("Fixing EDID header, your hardware may be failing\n");
+		DRM_DEBUG_KMS("Fixing EDID header, your hardware may be failing\n");
 		edid_header_fix(block);
 
 		/* Retry with fixed header, update status if that worked. */
@@ -2281,8 +2281,9 @@ int drm_edid_override_connector_update(struct drm_connector *connector)
 
 		drm_edid_free(override);
 
-		DRM_DEBUG_KMS("[CONNECTOR:%d:%s] adding %d modes via fallback override/firmware EDID\n",
-			      connector->base.id, connector->name, num_modes);
+		drm_dbg_kms(connector->dev,
+			    "[CONNECTOR:%d:%s] adding %d modes via fallback override/firmware EDID\n",
+			    connector->base.id, connector->name, num_modes);
 	}
 
 	return num_modes;
@@ -3389,17 +3390,16 @@ static struct drm_display_mode *drm_mode_detailed(struct drm_device *dev,
 		return NULL;
 
 	if (pt->misc & DRM_EDID_PT_STEREO) {
-		DRM_DEBUG_KMS("stereo mode not supported\n");
+		drm_dbg_kms(dev, "Stereo mode not supported\n");
 		return NULL;
 	}
 	if (!(pt->misc & DRM_EDID_PT_SEPARATE_SYNC)) {
-		DRM_DEBUG_KMS("composite sync not supported\n");
+		drm_dbg_kms(dev, "Composite sync not supported\n");
 	}
 
 	/* it is incorrect if hsync/vsync width is zero */
 	if (!hsync_pulse_width || !vsync_pulse_width) {
-		DRM_DEBUG_KMS("Incorrect Detailed timing. "
-				"Wrong Hsync/Vsync pulse width\n");
+		drm_dbg_kms(dev, "Incorrect Detailed timing. Wrong Hsync/Vsync pulse width\n");
 		return NULL;
 	}
 
@@ -4643,7 +4643,7 @@ static int add_hdmi_mode(struct drm_connector *connector, u8 vic)
 	struct drm_display_mode *newmode;
 
 	if (!drm_valid_hdmi_vic(vic)) {
-		DRM_ERROR("Unknown HDMI VIC: %d\n", vic);
+		drm_err(connector->dev, "Unknown HDMI VIC: %d\n", vic);
 		return 0;
 	}
 
@@ -5288,8 +5288,8 @@ static void fixup_detailed_cea_mode_clock(struct drm_display_mode *mode)
 	if (mode->clock == clock)
 		return;
 
-	DRM_DEBUG("detailed mode matches %s VIC %d, adjusting clock %d -> %d\n",
-		  type, vic, mode->clock, clock);
+	DRM_DEBUG_KMS("detailed mode matches %s VIC %d, adjusting clock %d -> %d\n",
+		      type, vic, mode->clock, clock);
 	mode->clock = clock;
 }
 
@@ -5397,15 +5397,11 @@ drm_parse_hdmi_vsdb_audio(struct drm_connector *connector, const u8 *db)
 	if (len >= 12)
 		connector->audio_latency[1] = db[12];
 
-	DRM_DEBUG_KMS("HDMI: latency present %d %d, "
-		      "video latency %d %d, "
-		      "audio latency %d %d\n",
-		      connector->latency_present[0],
-		      connector->latency_present[1],
-		      connector->video_latency[0],
-		      connector->video_latency[1],
-		      connector->audio_latency[0],
-		      connector->audio_latency[1]);
+	drm_dbg_kms(connector->dev,
+		    "HDMI: latency present %d %d, video latency %d %d, audio latency %d %d\n",
+		    connector->latency_present[0], connector->latency_present[1],
+		    connector->video_latency[0], connector->video_latency[1],
+		    connector->audio_latency[0], connector->audio_latency[1]);
 }
 
 static void
@@ -5503,7 +5499,8 @@ static void drm_edid_to_eld(struct drm_connector *connector,
 		return;
 
 	mnl = get_monitor_name(drm_edid, &eld[DRM_ELD_MONITOR_NAME_STRING]);
-	DRM_DEBUG_KMS("ELD monitor %s\n", &eld[DRM_ELD_MONITOR_NAME_STRING]);
+	drm_dbg_kms(connector->dev, "ELD monitor %s\n",
+		    &eld[DRM_ELD_MONITOR_NAME_STRING]);
 
 	eld[DRM_ELD_CEA_EDID_VER_MNL] = info->cea_rev << DRM_ELD_CEA_EDID_VER_SHIFT;
 	eld[DRM_ELD_CEA_EDID_VER_MNL] |= mnl;
@@ -5557,8 +5554,8 @@ static void drm_edid_to_eld(struct drm_connector *connector,
 	eld[DRM_ELD_BASELINE_ELD_LEN] =
 		DIV_ROUND_UP(drm_eld_calc_baseline_block_size(eld), 4);
 
-	DRM_DEBUG_KMS("ELD size %d, SAD count %d\n",
-		      drm_eld_size(eld), total_sad_count);
+	drm_dbg_kms(connector->dev, "ELD size %d, SAD count %d\n",
+		    drm_eld_size(eld), total_sad_count);
 }
 
 static int _drm_edid_to_sad(const struct drm_edid *drm_edid,
@@ -5829,7 +5826,7 @@ static void drm_parse_vcdb(struct drm_connector *connector, const u8 *db)
 {
 	struct drm_display_info *info = &connector->display_info;
 
-	DRM_DEBUG_KMS("CEA VCDB 0x%02x\n", db[2]);
+	drm_dbg_kms(connector->dev, "CEA VCDB 0x%02x\n", db[2]);
 
 	if (db[2] & EDID_CEA_VCDB_QS)
 		info->rgb_quant_range_selectable = true;
@@ -6031,39 +6028,39 @@ static void drm_parse_hdmi_deep_color_info(struct drm_connector *connector,
 	if (hdmi[6] & DRM_EDID_HDMI_DC_30) {
 		dc_bpc = 10;
 		info->edid_hdmi_rgb444_dc_modes |= DRM_EDID_HDMI_DC_30;
-		DRM_DEBUG("%s: HDMI sink does deep color 30.\n",
-			  connector->name);
+		drm_dbg_kms(connector->dev, "%s: HDMI sink does deep color 30.\n",
+			    connector->name);
 	}
 
 	if (hdmi[6] & DRM_EDID_HDMI_DC_36) {
 		dc_bpc = 12;
 		info->edid_hdmi_rgb444_dc_modes |= DRM_EDID_HDMI_DC_36;
-		DRM_DEBUG("%s: HDMI sink does deep color 36.\n",
-			  connector->name);
+		drm_dbg_kms(connector->dev, "%s: HDMI sink does deep color 36.\n",
+			    connector->name);
 	}
 
 	if (hdmi[6] & DRM_EDID_HDMI_DC_48) {
 		dc_bpc = 16;
 		info->edid_hdmi_rgb444_dc_modes |= DRM_EDID_HDMI_DC_48;
-		DRM_DEBUG("%s: HDMI sink does deep color 48.\n",
-			  connector->name);
+		drm_dbg_kms(connector->dev, "%s: HDMI sink does deep color 48.\n",
+			    connector->name);
 	}
 
 	if (dc_bpc == 0) {
-		DRM_DEBUG("%s: No deep color support on this HDMI sink.\n",
-			  connector->name);
+		drm_dbg_kms(connector->dev, "%s: No deep color support on this HDMI sink.\n",
+			    connector->name);
 		return;
 	}
 
-	DRM_DEBUG("%s: Assigning HDMI sink color depth as %d bpc.\n",
-		  connector->name, dc_bpc);
+	drm_dbg_kms(connector->dev, "%s: Assigning HDMI sink color depth as %d bpc.\n",
+		    connector->name, dc_bpc);
 	info->bpc = dc_bpc;
 
 	/* YCRCB444 is optional according to spec. */
 	if (hdmi[6] & DRM_EDID_HDMI_DC_Y444) {
 		info->edid_hdmi_ycbcr444_dc_modes = info->edid_hdmi_rgb444_dc_modes;
-		DRM_DEBUG("%s: HDMI sink does YCRCB444 in deep color.\n",
-			  connector->name);
+		drm_dbg_kms(connector->dev, "%s: HDMI sink does YCRCB444 in deep color.\n",
+			    connector->name);
 	}
 
 	/*
@@ -6071,8 +6068,8 @@ static void drm_parse_hdmi_deep_color_info(struct drm_connector *connector,
 	 * then deep color 36 bit must be supported.
 	 */
 	if (!(hdmi[6] & DRM_EDID_HDMI_DC_36)) {
-		DRM_DEBUG("%s: HDMI sink should do DC_36, but does not!\n",
-			  connector->name);
+		drm_dbg_kms(connector->dev, "%s: HDMI sink should do DC_36, but does not!\n",
+			    connector->name);
 	}
 }
 
@@ -6089,10 +6086,8 @@ drm_parse_hdmi_vsdb_video(struct drm_connector *connector, const u8 *db)
 	if (len >= 7)
 		info->max_tmds_clock = db[7] * 5000;
 
-	DRM_DEBUG_KMS("HDMI: DVI dual %d, "
-		      "max TMDS clock %d kHz\n",
-		      info->dvi_dual,
-		      info->max_tmds_clock);
+	drm_dbg_kms(connector->dev, "HDMI: DVI dual %d, max TMDS clock %d kHz\n",
+		    info->dvi_dual, info->max_tmds_clock);
 
 	drm_parse_hdmi_deep_color_info(connector, db);
 }
@@ -6134,8 +6129,8 @@ static void drm_parse_cea_ext(struct drm_connector *connector,
 			info->cea_rev = edid_ext[1];
 
 		if (info->cea_rev != edid_ext[1])
-			DRM_DEBUG_KMS("CEA extension version mismatch %u != %u\n",
-				      info->cea_rev, edid_ext[1]);
+			drm_dbg_kms(connector->dev, "CEA extension version mismatch %u != %u\n",
+				    info->cea_rev, edid_ext[1]);
 
 		/* The existence of a CTA extension should imply RGB support */
 		info->color_formats = DRM_COLOR_FORMAT_RGB444;
@@ -6221,9 +6216,8 @@ static void drm_get_monitor_range(struct drm_connector *connector,
 
 	drm_for_each_detailed_block(drm_edid, get_monitor_range, &closure);
 
-	DRM_DEBUG_KMS("Supported Monitor Refresh rate range is %d Hz - %d Hz\n",
-		      info->monitor_range.min_vfreq,
-		      info->monitor_range.max_vfreq);
+	drm_dbg_kms(connector->dev, "Supported Monitor Refresh rate range is %d Hz - %d Hz\n",
+		    info->monitor_range.min_vfreq, info->monitor_range.max_vfreq);
 }
 
 static void drm_parse_vesa_mso_data(struct drm_connector *connector,
@@ -6357,8 +6351,8 @@ static u32 update_display_info(struct drm_connector *connector,
 	if (info->bpc == 0 && edid->revision == 3 &&
 	    edid->input & DRM_EDID_DIGITAL_DFP_1_X) {
 		info->bpc = 8;
-		DRM_DEBUG("%s: Assigning DFP sink color depth as %d bpc.\n",
-			  connector->name, info->bpc);
+		drm_dbg_kms(connector->dev, "%s: Assigning DFP sink color depth as %d bpc.\n",
+			    connector->name, info->bpc);
 	}
 
 	/* Only defined for 1.4 with digital displays */
@@ -6390,8 +6384,8 @@ static u32 update_display_info(struct drm_connector *connector,
 		break;
 	}
 
-	DRM_DEBUG("%s: Assigning EDID-1.4 digital sink color depth as %d bpc.\n",
-			  connector->name, info->bpc);
+	drm_dbg_kms(connector->dev, "%s: Assigning EDID-1.4 digital sink color depth as %d bpc.\n",
+		    connector->name, info->bpc);
 
 	if (edid->features & DRM_EDID_FEATURE_RGB_YCRCB444)
 		info->color_formats |= DRM_COLOR_FORMAT_YCBCR444;
@@ -7090,11 +7084,12 @@ static void drm_parse_tiled_block(struct drm_connector *connector,
 	connector->tile_h_size = w + 1;
 	connector->tile_v_size = h + 1;
 
-	DRM_DEBUG_KMS("tile cap 0x%x\n", tile->tile_cap);
-	DRM_DEBUG_KMS("tile_size %d x %d\n", w + 1, h + 1);
-	DRM_DEBUG_KMS("topo num tiles %dx%d, location %dx%d\n",
-		      num_h_tile + 1, num_v_tile + 1, tile_h_loc, tile_v_loc);
-	DRM_DEBUG_KMS("vend %c%c%c\n", tile->topology_id[0], tile->topology_id[1], tile->topology_id[2]);
+	drm_dbg_kms(connector->dev, "tile cap 0x%x\n", tile->tile_cap);
+	drm_dbg_kms(connector->dev, "tile_size %d x %d\n", w + 1, h + 1);
+	drm_dbg_kms(connector->dev, "topo num tiles %dx%d, location %dx%d\n",
+		    num_h_tile + 1, num_v_tile + 1, tile_h_loc, tile_v_loc);
+	drm_dbg_kms(connector->dev, "vend %c%c%c\n", tile->topology_id[0],
+		    tile->topology_id[1], tile->topology_id[2]);
 
 	tg = drm_mode_get_tile_group(connector->dev, tile->topology_id);
 	if (!tg)
-- 
2.34.1

Re: [PATCH 15/15] drm/edid: convert to device specific logging

[Ville Syrjälä]

On Tue, Oct 11, 2022 at 04:49:49PM +0300, Jani Nikula wrote:
> Convert to drm_kms_dbg/drm_err where possible, and unify the rest of the
> debugs to DRM_DEBUG_KMS.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/drm_edid.c | 105 ++++++++++++++++++-------------------
>  1 file changed, 50 insertions(+), 55 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 935bdf4e6269..09b0b8337382 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -1979,7 +1979,7 @@ bool drm_edid_block_valid(u8 *_block, int block_num, bool print_bad_edid,
>  
>  	status = edid_block_check(block, is_base_block);
>  	if (status == EDID_BLOCK_HEADER_REPAIR) {
> -		DRM_DEBUG("Fixing EDID header, your hardware may be failing\n");
> +		DRM_DEBUG_KMS("Fixing EDID header, your hardware may be failing\n");
>  		edid_header_fix(block);
>  
>  		/* Retry with fixed header, update status if that worked. */
> @@ -2281,8 +2281,9 @@ int drm_edid_override_connector_update(struct drm_connector *connector)
>  
>  		drm_edid_free(override);
>  
> -		DRM_DEBUG_KMS("[CONNECTOR:%d:%s] adding %d modes via fallback override/firmware EDID\n",
> -			      connector->base.id, connector->name, num_modes);
> +		drm_dbg_kms(connector->dev,
> +			    "[CONNECTOR:%d:%s] adding %d modes via fallback override/firmware EDID\n",
> +			    connector->base.id, connector->name, num_modes);
>  	}
>  
>  	return num_modes;
> @@ -3389,17 +3390,16 @@ static struct drm_display_mode *drm_mode_detailed(struct drm_device *dev,
>  		return NULL;
>  
>  	if (pt->misc & DRM_EDID_PT_STEREO) {
> -		DRM_DEBUG_KMS("stereo mode not supported\n");
> +		drm_dbg_kms(dev, "Stereo mode not supported\n");
>  		return NULL;
>  	}
>  	if (!(pt->misc & DRM_EDID_PT_SEPARATE_SYNC)) {
> -		DRM_DEBUG_KMS("composite sync not supported\n");
> +		drm_dbg_kms(dev, "Composite sync not supported\n");
>  	}
>  
>  	/* it is incorrect if hsync/vsync width is zero */
>  	if (!hsync_pulse_width || !vsync_pulse_width) {
> -		DRM_DEBUG_KMS("Incorrect Detailed timing. "
> -				"Wrong Hsync/Vsync pulse width\n");
> +		drm_dbg_kms(dev, "Incorrect Detailed timing. Wrong Hsync/Vsync pulse width\n");
>  		return NULL;
>  	}
>  
> @@ -4643,7 +4643,7 @@ static int add_hdmi_mode(struct drm_connector *connector, u8 vic)
>  	struct drm_display_mode *newmode;
>  
>  	if (!drm_valid_hdmi_vic(vic)) {
> -		DRM_ERROR("Unknown HDMI VIC: %d\n", vic);
> +		drm_err(connector->dev, "Unknown HDMI VIC: %d\n", vic);

Quit a lot of these could also do the full [CONNECTOR:...] 
thing it seems.

-- 
Ville Syrjälä
Intel