[PATCH] drm/edid: Always set RGB444

[PATCH] drm/edid: Always set RGB444

[Maxime Ripard]

In order to fill the drm_display_info structure each time an EDID is
read, the code currently will call drm_add_display_info with the parsed
EDID.

drm_add_display_info will then call drm_reset_display_info to reset all
the fields to 0, and then set them to the proper value depending on the
EDID.

In the color_formats case, we will thus report that we don't support any
color format, and then fill it back with RGB444 plus the additional
formats described in the EDID Feature Support byte.

However, since that byte only contains format-related bits since the 1.4
specification, this doesn't happen if the EDID is following an earlier
specification. In turn, it means that for one of these EDID, we end up
with color_formats set to 0.

The EDID 1.3 specification never really specifies what it means by RGB
exactly, but since both HDMI and DVI will use RGB444, it's fairly safe
to assume it's supposed to be RGB444.

Let's move the addition of RGB444 to color_formats earlier in
drm_add_display_info() so that it's always set for a digital display.

Fixes: da05a5a71ad8 ("drm: parse color format support for digital displays")
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Reported-by: Matthias Reichl <hias@horus.com>
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/drm_edid.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 12893e7be89b..f5f5de362ff2 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -5345,6 +5345,7 @@ u32 drm_add_display_info(struct drm_connector *connector, const struct edid *edi
 	if (!(edid->input & DRM_EDID_INPUT_DIGITAL))
 		return quirks;
 
+	info->color_formats |= DRM_COLOR_FORMAT_RGB444;
 	drm_parse_cea_ext(connector, edid);
 
 	/*
@@ -5393,7 +5394,6 @@ u32 drm_add_display_info(struct drm_connector *connector, const struct edid *edi
 	DRM_DEBUG("%s: Assigning EDID-1.4 digital sink color depth as %d bpc.\n",
 			  connector->name, info->bpc);
 
-	info->color_formats |= DRM_COLOR_FORMAT_RGB444;
 	if (edid->features & DRM_EDID_FEATURE_RGB_YCRCB444)
 		info->color_formats |= DRM_COLOR_FORMAT_YCRCB444;
 	if (edid->features & DRM_EDID_FEATURE_RGB_YCRCB422)
-- 
2.34.1

Re: [PATCH] drm/edid: Always set RGB444

[Ville Syrjälä]

On Thu, Feb 03, 2022 at 12:54:16PM +0100, Maxime Ripard wrote:
> In order to fill the drm_display_info structure each time an EDID is
> read, the code currently will call drm_add_display_info with the parsed
> EDID.
> 
> drm_add_display_info will then call drm_reset_display_info to reset all
> the fields to 0, and then set them to the proper value depending on the
> EDID.
> 
> In the color_formats case, we will thus report that we don't support any
> color format, and then fill it back with RGB444 plus the additional
> formats described in the EDID Feature Support byte.
> 
> However, since that byte only contains format-related bits since the 1.4
> specification, this doesn't happen if the EDID is following an earlier
> specification. In turn, it means that for one of these EDID, we end up
> with color_formats set to 0.
> 
> The EDID 1.3 specification never really specifies what it means by RGB
> exactly, but since both HDMI and DVI will use RGB444, it's fairly safe
> to assume it's supposed to be RGB444.
> 
> Let's move the addition of RGB444 to color_formats earlier in
> drm_add_display_info() so that it's always set for a digital display.
> 
> Fixes: da05a5a71ad8 ("drm: parse color format support for digital displays")
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Reported-by: Matthias Reichl <hias@horus.com>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>

Looks OK to me. I guess the DFP1.x == 0 case might allow some
other encoding to be used but the EDID spec makes not mention
how to figure that out.

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

> ---
>  drivers/gpu/drm/drm_edid.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 12893e7be89b..f5f5de362ff2 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -5345,6 +5345,7 @@ u32 drm_add_display_info(struct drm_connector *connector, const struct edid *edi
>  	if (!(edid->input & DRM_EDID_INPUT_DIGITAL))
>  		return quirks;
>  
> +	info->color_formats |= DRM_COLOR_FORMAT_RGB444;
>  	drm_parse_cea_ext(connector, edid);
>  
>  	/*
> @@ -5393,7 +5394,6 @@ u32 drm_add_display_info(struct drm_connector *connector, const struct edid *edi
>  	DRM_DEBUG("%s: Assigning EDID-1.4 digital sink color depth as %d bpc.\n",
>  			  connector->name, info->bpc);
>  
> -	info->color_formats |= DRM_COLOR_FORMAT_RGB444;
>  	if (edid->features & DRM_EDID_FEATURE_RGB_YCRCB444)
>  		info->color_formats |= DRM_COLOR_FORMAT_YCRCB444;
>  	if (edid->features & DRM_EDID_FEATURE_RGB_YCRCB422)
> -- 
> 2.34.1

-- 
Ville Syrjälä
Intel

Re: (subset) [PATCH] drm/edid: Always set RGB444

[Maxime Ripard]

On Thu, 3 Feb 2022 12:54:16 +0100, Maxime Ripard wrote:
> In order to fill the drm_display_info structure each time an EDID is
> read, the code currently will call drm_add_display_info with the parsed
> EDID.
> 
> drm_add_display_info will then call drm_reset_display_info to reset all
> the fields to 0, and then set them to the proper value depending on the
> EDID.
> 
> [...]

Applied to drm/drm-misc (drm-misc-fixes).

Thanks!
Maxime