Re: [PATCH v2 03/11] drm/edid: slightly restructure timing and non-timing descriptor structs

[Jani Nikula <jani.nikula@intel.com>]

On Mon, 28 Mar 2022, Jani Nikula <jani.nikula@intel.com> wrote:
> The pixel clock is conceptually part of the detailed timings, while it's
> just zero padding for display descriptors. Modify the structures to
> reflect this. Rename struct detailed_non_pixel to
> edid_display_descriptor to better reflect spec while at it. (Further
> struct renames are left for follow-up work.)
>
> Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

This one's missing:

Cc: Harry Wentland <harry.wentland@amd.com>
Cc: Leo Li <sunpeng.li@amd.com>
Cc: Rodrigo Siqueira <Rodrigo.Siqueira@amd.com>
Cc: amd-gfx@lists.freedesktop.org

> ---
>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  6 +++---
>  drivers/gpu/drm/drm_edid.c                        | 12 ++++++------
>  include/drm/drm_edid.h                            |  9 +++++----
>  3 files changed, 14 insertions(+), 13 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 b30656959fd8..e477f4b42b6b 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -11537,7 +11537,7 @@ void amdgpu_dm_update_freesync_caps(struct drm_connector *connector,
>  {
>  	int i = 0;
>  	struct detailed_timing *timing;
> -	struct detailed_non_pixel *data;
> +	struct edid_display_descriptor *data;
>  	struct detailed_data_monitor_range *range;
>  	struct amdgpu_dm_connector *amdgpu_dm_connector =
>  			to_amdgpu_dm_connector(connector);
> @@ -11592,7 +11592,7 @@ void amdgpu_dm_update_freesync_caps(struct drm_connector *connector,
>  			for (i = 0; i < 4; i++) {
>  
>  				timing	= &edid->detailed_timings[i];
> -				data	= &timing->data.other_data;
> +				data	= &timing->data.descriptor;
>  				range	= &data->data.range;
>  				/*
>  				 * Check if monitor has continuous frequency mode
> @@ -11629,7 +11629,7 @@ void amdgpu_dm_update_freesync_caps(struct drm_connector *connector,
>  		i = parse_hdmi_amd_vsdb(amdgpu_dm_connector, edid, &vsdb_info);
>  		if (i >= 0 && vsdb_info.freesync_supported) {
>  			timing  = &edid->detailed_timings[i];
> -			data    = &timing->data.other_data;
> +			data    = &timing->data.descriptor;
>  
>  			amdgpu_dm_connector->min_vfreq = vsdb_info.min_refresh_rate_hz;
>  			amdgpu_dm_connector->max_vfreq = vsdb_info.max_refresh_rate_hz;
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 13d05062d68c..ac80681d64f6 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -2742,7 +2742,7 @@ static struct drm_display_mode *drm_mode_detailed(struct drm_device *dev,
>  	if (quirks & EDID_QUIRK_135_CLOCK_TOO_HIGH)
>  		mode->clock = 1088 * 10;
>  	else
> -		mode->clock = le16_to_cpu(timing->pixel_clock) * 10;
> +		mode->clock = le16_to_cpu(pt->pixel_clock) * 10;
>  
>  	mode->hdisplay = hactive;
>  	mode->hsync_start = mode->hdisplay + hsync_offset;
> @@ -2984,7 +2984,7 @@ static void
>  do_inferred_modes(struct detailed_timing *timing, void *c)
>  {
>  	struct detailed_mode_closure *closure = c;
> -	struct detailed_non_pixel *data = &timing->data.other_data;
> +	struct edid_display_descriptor *data = &timing->data.descriptor;
>  	struct detailed_data_monitor_range *range = &data->data.range;
>  
>  	if (!is_display_descriptor((const u8 *)timing, EDID_DETAIL_MONITOR_RANGE))
> @@ -3117,7 +3117,7 @@ static void
>  do_standard_modes(struct detailed_timing *timing, void *c)
>  {
>  	struct detailed_mode_closure *closure = c;
> -	struct detailed_non_pixel *data = &timing->data.other_data;
> +	struct edid_display_descriptor *data = &timing->data.descriptor;
>  	struct drm_connector *connector = closure->connector;
>  	struct edid *edid = closure->edid;
>  	int i;
> @@ -3187,7 +3187,7 @@ static int drm_cvt_modes(struct drm_connector *connector,
>  	for (i = 0; i < 4; i++) {
>  		int width, height;
>  
> -		cvt = &(timing->data.other_data.data.cvt[i]);
> +		cvt = &(timing->data.descriptor.data.cvt[i]);
>  
>  		if (!memcmp(cvt->code, empty, 3))
>  			continue;
> @@ -4494,7 +4494,7 @@ monitor_name(struct detailed_timing *t, void *data)
>  	if (!is_display_descriptor((const u8 *)t, EDID_DETAIL_MONITOR_NAME))
>  		return;
>  
> -	*(u8 **)data = t->data.other_data.data.str.str;
> +	*(u8 **)data = t->data.descriptor.data.str.str;
>  }
>  
>  static int get_monitor_name(struct edid *edid, char name[13])
> @@ -5223,7 +5223,7 @@ void get_monitor_range(struct detailed_timing *timing,
>  		       void *info_monitor_range)
>  {
>  	struct drm_monitor_range_info *monitor_range = info_monitor_range;
> -	const struct detailed_non_pixel *data = &timing->data.other_data;
> +	const struct edid_display_descriptor *data = &timing->data.descriptor;
>  	const struct detailed_data_monitor_range *range = &data->data.range;
>  
>  	if (!is_display_descriptor((const u8 *)timing, EDID_DETAIL_MONITOR_RANGE))
> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> index 144c495b99c4..8e322ef173a8 100644
> --- a/include/drm/drm_edid.h
> +++ b/include/drm/drm_edid.h
> @@ -68,6 +68,7 @@ struct std_timing {
>  
>  /* If detailed data is pixel timing */
>  struct detailed_pixel_timing {
> +	__le16 pixel_clock; /* non-zero, need to multiply by 10 KHz */
>  	u8 hactive_lo;
>  	u8 hblank_lo;
>  	u8 hactive_hblank_hi;
> @@ -142,8 +143,9 @@ struct cvt_timing {
>  	u8 code[3];
>  } __attribute__((packed));
>  
> -struct detailed_non_pixel {
> -	u8 pad1;
> +struct edid_display_descriptor {
> +	u16 pad0; /* 0 for Display Descriptor */
> +	u8 pad1; /* 0 for Display Descriptor */
>  	u8 type; /* ff=serial, fe=string, fd=monitor range, fc=monitor name
>  		    fb=color point data, fa=standard timing data,
>  		    f9=undefined, f8=mfg. reserved */
> @@ -168,10 +170,9 @@ struct detailed_non_pixel {
>  #define EDID_DETAIL_MONITOR_SERIAL 0xff
>  
>  struct detailed_timing {
> -	__le16 pixel_clock; /* need to multiply by 10 KHz */
>  	union {
>  		struct detailed_pixel_timing pixel_data;
> -		struct detailed_non_pixel other_data;
> +		struct edid_display_descriptor descriptor;
>  	} data;
>  } __attribute__((packed));

-- 
Jani Nikula, Intel Open Source Graphics Center