Re: [PATCH v2 1/3] drm/edid: extract section header processing into helper

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

On 5/20/2026 9:10 PM, Jani Nikula wrote:
> On Wed, 20 May 2026, Chenyu Chen <chen-yu.chen@amd.com> wrote:
>> Extract the DisplayID section header logging and non_desktop
>> detection from update_displayid_info() into a dedicated helper,
>> drm_displayid_process_section_header(). Remove the break so the
>> iterator walks through all data blocks, preparing for future
>> patches that will parse additional block types within the loop.
>>
>> The helper is called only once for the base section via a
>> header_processed flag. Since version and primary_use are only
>> captured from the base section, and extension sections carry a
>> primary use of zero per spec, the non_desktop logic is unaffected.
>>
>> No functional change.
>>
>> 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_edid.c | 37 +++++++++++++++++++++----------------
>>  1 file changed, 21 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>> index 8031f021d4d0..04878478ab78 100644
>> --- a/drivers/gpu/drm/drm_edid.c
>> +++ b/drivers/gpu/drm/drm_edid.c
>> @@ -6715,30 +6715,35 @@ static void drm_reset_display_info(struct drm_connector *connector)
>>  	memset(&info->amd_vsdb, 0, sizeof(info->amd_vsdb));
>>  }
>>  
>> +static void drm_displayid_process_section_header(struct drm_connector *connector,
>> +				  const struct displayid_iter *iter)
> 
> The name's a bit grandiose, yet loses the bit about "base section" which
> was the crucial part in the comment that gets removed below.
> 

I'll rename it to drm_displayid_process_base_section_header().

>> +{
>> +	struct drm_display_info *info = &connector->display_info;
>> +
>> +	drm_dbg_kms(connector->dev,
>> +			"[CONNECTOR:%d:%s] DisplayID extension version 0x%02x, primary use 0x%02x\n",
>> +			connector->base.id, connector->name,
>> +			displayid_version(iter),
>> +			displayid_primary_use(iter));
>> +	if (displayid_version(iter) == DISPLAY_ID_STRUCTURE_VER_20 &&
>> +		(displayid_primary_use(iter) == PRIMARY_USE_HEAD_MOUNTED_VR ||
>> +			displayid_primary_use(iter) == PRIMARY_USE_HEAD_MOUNTED_AR))
>> +		info->non_desktop = true;
>> +}
> 
> The indent is all wrong in this function.
> 

I will fix it.

>> +
>>  static void update_displayid_info(struct drm_connector *connector,
>>  				  const struct drm_edid *drm_edid)
>>  {
>> -	struct drm_display_info *info = &connector->display_info;
>>  	const struct displayid_block *block;
>>  	struct displayid_iter iter;
>> +	bool header_processed = false;
>>  
>>  	displayid_iter_edid_begin(drm_edid, &iter);
>>  	displayid_iter_for_each(block, &iter) {
>> -		drm_dbg_kms(connector->dev,
>> -			    "[CONNECTOR:%d:%s] DisplayID extension version 0x%02x, primary use 0x%02x\n",
>> -			    connector->base.id, connector->name,
>> -			    displayid_version(&iter),
>> -			    displayid_primary_use(&iter));
>> -		if (displayid_version(&iter) == DISPLAY_ID_STRUCTURE_VER_20 &&
>> -		    (displayid_primary_use(&iter) == PRIMARY_USE_HEAD_MOUNTED_VR ||
>> -		     displayid_primary_use(&iter) == PRIMARY_USE_HEAD_MOUNTED_AR))
>> -			info->non_desktop = true;
>> -
>> -		/*
>> -		 * We're only interested in the base section here, no need to
>> -		 * iterate further.
>> -		 */
>> -		break;
>> +		if (!header_processed) {
>> +			drm_displayid_process_section_header(connector, &iter);
>> +			header_processed = true;
> 
> Every DisplayID Section has a header. Every DisplayID Data Block within
> a DisplayID Section has a header. This is about handling the information
> in the Base Section header only. IMO header_processed is misleading.
> 
> BR,
> Jani.
> 

I will rename it to `bool base_section_header_processed`.

Regards,
Chenyu

>> +		}
>>  	}
>>  	displayid_iter_end(&iter);
>>  }
>