Re: [PATCH v2 2/3] drm/edid: parse panel type from DisplayID 2.x Display Parameters

["Chen, Chen-Yu" <Chen-Yu.Chen@amd.com>]

On 5/20/2026 9:30 PM, Jani Nikula wrote:
> On Wed, 20 May 2026, Chenyu Chen <chen-yu.chen@amd.com> wrote:
>> Parse the Display Parameters Data Block (tag 0x21) defined in
>> DisplayID v2.1a Section 4.2.6. Extract the Display Device Technology
>> field from payload byte 27 bits [6:4], which indicates whether the
>> panel uses LCD (001b) or OLED (010b) technology.
>>
>> Add a did_panel_type field to struct drm_display_info and populate it
>> during DisplayID iteration so downstream drivers can use it for
>> panel-type-dependent behavior. Add DRM_MODE_PANEL_TYPE_LCD to the
>> UAPI constants for use as an internal communication value between
>> DRM core and drivers.
>>
>> Assisted-by: Copilot:Claude-Opus-4.6
>> Signed-off-by: Chenyu Chen <chen-yu.chen@amd.com>
>> Reviewed-by: Mario Limonciello (AMD) <superm1@kernel.org>
>> ---
>>  drivers/gpu/drm/drm_displayid_internal.h | 25 ++++++++++++++
>>  drivers/gpu/drm/drm_edid.c               | 44 ++++++++++++++++++++++++
>>  include/drm/drm_connector.h              |  6 ++++
>>  include/uapi/drm/drm_mode.h              |  1 +
>>  4 files changed, 76 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_displayid_internal.h b/drivers/gpu/drm/drm_displayid_internal.h
>> index 5b1b32f73516..e0f7c54d2987 100644
>> --- a/drivers/gpu/drm/drm_displayid_internal.h
>> +++ b/drivers/gpu/drm/drm_displayid_internal.h
>> @@ -142,6 +142,31 @@ struct displayid_formula_timing_block {
>>  	struct displayid_formula_timings_9 timings[];
>>  } __packed;
>>  
>> +/*
>> + * DisplayID v2.x Display Parameters Data Block (tag 0x21).
>> + *
>> + * Per VESA DisplayID v2.1a, Section 4.2.6, Table 4-14:
>> + * Offset 0x1E (payload byte 27) contains Native Color Depth and
>> + * Display Device Technology fields.
>> + *   bits [2:0] = Native Color Depth
>> + *   bit  [3]   = RESERVED
>> + *   bits [6:4] = Display Device Technology
>> + *     000b = not specified, 001b = LCD, 010b = OLED, others reserved
>> + *   bit  [7]   = Display Device Theme Preference
>> + */
> 
> Please drop the comment...
> 
>> +#define DISPLAYID_DISPLAY_PARAMS_DEVICE_TECH	GENMASK(6, 4)
>> +
>> +struct displayid_display_params_block {
>> +	struct displayid_block base;
>> +	u8 payload[27];
>> +	u8 device_tech_byte; /* bits [6:4] = Display Device Technology */
>> +	u8 reserved;
> 
> ...and actually define the information here.
> 
> Sure, you're only interested in one thing, but don't leave the rest for
> someone who comes after you, since you clearly have access to the spec,
> and whoever reviews this also needs to have access to the spec.
> 

I will replace the block comment and the u8 payload[27] with fully named
fields per spec Table 4-7 (image size, pixel count, features, primary
colors, white point, luminance, color_depth_and_tech, gamma_eotf). 
I also add the named defines DISPLAYID_DEVICE_TECH_{UNSPECIFIED,LCD,OLED}
for the technology values.

>> +} __packed;
>> +
>> +#define DISPLAYID_DISPLAY_PARAMS_MIN_LEN	\
>> +	(sizeof(struct displayid_display_params_block) -	\
>> +	 sizeof(struct displayid_block))
> 
> Is this helpful? Do you suggest we should add the define for all of the
> blocks? What about when there's a minimum, and you can optionally have
> more?
> 

I think defining a macro for every block doesn’t make sense.
So I will remove the macro and use sizeof(*params) - sizeof(params->base)
inline instead.

>> +
>>  #define DISPLAYID_VESA_MSO_OVERLAP	GENMASK(3, 0)
>>  #define DISPLAYID_VESA_MSO_MODE		GENMASK(6, 5)
>>  
>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>> index 04878478ab78..d1de1a398677 100644
>> --- a/drivers/gpu/drm/drm_edid.c
>> +++ b/drivers/gpu/drm/drm_edid.c
>> @@ -6713,6 +6713,8 @@ static void drm_reset_display_info(struct drm_connector *connector)
>>  
>>  	info->source_physical_address = CEC_PHYS_ADDR_INVALID;
>>  	memset(&info->amd_vsdb, 0, sizeof(info->amd_vsdb));
>> +
>> +	info->did_panel_type = DRM_MODE_PANEL_TYPE_UNKNOWN;
>>  }
>>  
>>  static void drm_displayid_process_section_header(struct drm_connector *connector,
>> @@ -6731,6 +6733,44 @@ static void drm_displayid_process_section_header(struct drm_connector *connector
>>  		info->non_desktop = true;
>>  }
>>  
>> +static void
>> +drm_displayid_parse_display_params(struct drm_connector *connector,
>> +				   const struct displayid_block *block)
>> +{
>> +	struct drm_display_info *info = &connector->display_info;
>> +	const struct displayid_display_params_block *params =
>> +		(const struct displayid_display_params_block *)block;
>> +
> 
> Superfluous blank line.
> 

Will fix it.

>> +	u8 tech;
>> +
>> +	if (block->num_bytes < DISPLAYID_DISPLAY_PARAMS_MIN_LEN) {
>> +		drm_dbg_kms(connector->dev,
>> +			    "[CONNECTOR:%d:%s] DisplayID Display Parameters block too short (%u < %zu)\n",
>> +			    connector->base.id, connector->name,
>> +			    block->num_bytes,
>> +			    DISPLAYID_DISPLAY_PARAMS_MIN_LEN);
>> +		return;
>> +	}
>> +
>> +	tech = FIELD_GET(DISPLAYID_DISPLAY_PARAMS_DEVICE_TECH,
>> +			 params->device_tech_byte);
>> +
>> +	drm_dbg_kms(connector->dev,
>> +		    "[CONNECTOR:%d:%s] DisplayID Display Parameters: device technology %u\n",
>> +		    connector->base.id, connector->name, tech);
> 
> It would be more useful to print LCD or OLED, not the number.
> 

I will change it to print "LCD", "OLED", or "unspecified".

>> +
>> +	switch (tech) {
>> +	case 1: /* LCD */
>> +		info->did_panel_type = DRM_MODE_PANEL_TYPE_LCD;
>> +		break;
>> +	case 2: /* OLED */
> 
> The comments above are useless.
> 

I will remove the inline comments and use the named defines instead of
magic numbers in the switch cases.

>> +		info->did_panel_type = DRM_MODE_PANEL_TYPE_OLED;
>> +		break;
>> +	default:
>> +		break;
>> +	}
>> +}
>> +
>>  static void update_displayid_info(struct drm_connector *connector,
>>  				  const struct drm_edid *drm_edid)
>>  {
>> @@ -6744,6 +6784,10 @@ static void update_displayid_info(struct drm_connector *connector,
>>  			drm_displayid_process_section_header(connector, &iter);
>>  			header_processed = true;
>>  		}
>> +
>> +		if (displayid_version(&iter) == DISPLAY_ID_STRUCTURE_VER_20 &&
>> +		    block->tag == DATA_BLOCK_2_DISPLAY_PARAMETERS)
>> +			drm_displayid_parse_display_params(connector, block);
>>  	}
>>  	displayid_iter_end(&iter);
>>  }
>> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
>> index c398dbc68bbc..b95aec34ddb7 100644
>> --- a/include/drm/drm_connector.h
>> +++ b/include/drm/drm_connector.h
>> @@ -899,6 +899,12 @@ struct drm_display_info {
>>  	 * @amd_vsdb: AMD-specific VSDB information.
>>  	 */
>>  	struct drm_amd_vsdb_info amd_vsdb;
>> +
>> +	/**
>> +	 * @did_panel_type: Panel type from DisplayID Display Parameters
>> +	 * Data Block (tag 0x21). Uses DRM_MODE_PANEL_TYPE_* constants.
>> +	 */
>> +	u8 did_panel_type;
> 
> What does the did_ prefix give us?
> 


Good point, it doesn't add much value.

The intent was to indicate that the value is derived from DisplayID,
but I agree that encoding the data source in the field name is not
particularly helpful.

I'll rename it to "panel_type" and treat it as a generic field,
currently populated from DisplayID if available.

>>  };
>>  
>>  int drm_display_info_set_bus_formats(struct drm_display_info *info,
>> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
>> index 3693d82b5279..d7ca1040b92e 100644
>> --- a/include/uapi/drm/drm_mode.h
>> +++ b/include/uapi/drm/drm_mode.h
>> @@ -169,6 +169,7 @@ extern "C" {
>>  /* Panel type property */
>>  #define DRM_MODE_PANEL_TYPE_UNKNOWN	0
>>  #define DRM_MODE_PANEL_TYPE_OLED	1
>> +#define DRM_MODE_PANEL_TYPE_LCD		2
> 
> This is a UABI header.
> 
> Should we add this to the panel type property too?
> 
> 
> BR,
> Jani.
> 
> 

Yes. I think we can expose the panel type too.

Regards,
Chenyu

>>  
>>  /*
>>   * DRM_MODE_ROTATE_<degrees>
>