Message-ID: <a1272944-c090-3670-bccc-339a853e4441@amd.com>
Date:   Mon, 27 Feb 2023 13:22:49 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.8.0
Subject: Re: [PATCH v3 1/2] drm/edid: parse DRM VESA dsc bpp target
Content-Language: en-US
To:     Jani Nikula <jani.nikula@intel.com>,
        Yaroslav Bolyukin <iam@lach.pw>, amd-gfx@lists.freedesktop.org,
        dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org
Cc:     Leo Li <sunpeng.li@amd.com>,
        Rodrigo Siqueira <Rodrigo.Siqueira@amd.com>,
        Alex Deucher <alexander.deucher@amd.com>,
        =?UTF-8?Q?Christian_K=c3=b6nig?= <christian.koenig@amd.com>,
        "Pan, Xinhui" <Xinhui.Pan@amd.com>,
        David Airlie <airlied@gmail.com>,
        Daniel Vetter <daniel@ffwll.ch>,
        Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
        Maxime Ripard <mripard@kernel.org>,
        Thomas Zimmermann <tzimmermann@suse.de>,
        "Liu, Wenjing" <Wenjing.Liu@amd.com>
References: <20230226141051.21767-1-iam@lach.pw>
 <20230226141051.21767-2-iam@lach.pw>
 <bed5e04a-a0e3-fb80-d75e-cdcd85efe7ab@amd.com> <87356r117t.fsf@intel.com>
From:   Harry Wentland <harry.wentland@amd.com>
In-Reply-To: <87356r117t.fsf@intel.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?d3pucnk2cHdhaHhQYnI0MXhRWFk5N0JOd3p2U0VOaC80dThZZnd3RnVsNFdI?=
 =?utf-8?B?b3Nqc2NQZk5uczdrTS9nTmpZUDJ4bTlXSXFxSjBOWlJIQmp0SkdjaFRvcE1F?=
 =?utf-8?B?K2lyTFArbzUreGtrS1A3RWhhUVhUN1lTTWtGajV5bi9pMExuVEtFU3lmMWxY?=
 =?utf-8?B?RktyZ2JGUGxBSnlxWW1iS1RUcE56QTYwMW1YVEJlTGRZaVBtM0hWU2dRZ00w?=
 =?utf-8?B?MkhVc2g5TCs0d2VoR3IzTjdSdE1ObHVJZW12TkcvSGh4dkU4Mk1Kek1FVWxY?=
 =?utf-8?B?THdzV1JtOFRjVTV0T0N1eHN0dy9TcE02d1Jvc21uL2F0czRBMlMreENlNS96?=
 =?utf-8?B?US9wUnBsRnpvY2ZOcjI0T0VuS1Q5MTNpcmdvZUxVdjYyYWsvRXV2QnFMZEpx?=
 =?utf-8?B?UWZ3bU9TcXVoeGs5aUk3U3o1djZkWHpBaWQ4ZDlvT2dpdm9HTTVXN0lWRU1Q?=
 =?utf-8?B?M25VV0swUHpNWkNhRk5rVzlNaGxnMGFrcndCTUVvYTJrSnh4ZktNeXA0RW9h?=
 =?utf-8?B?RVlUd0NoQXRJdkZIbjAyc28rZkdsSkRqZ1JJTUdrSlMwWGFiSmNpVlA3c21T?=
 =?utf-8?B?aW1xRjB3dEttVzNjVkJONmE4OFNXL09vOHRieWVVSm1LeTFVUmhnN3FsZG5Y?=
 =?utf-8?B?UEMrWVBXNVozQWQ0c2QyZ2RRbXlxR2s2Q3BLb2pTUHcxc1ZjbkNiaUhIcVdE?=
 =?utf-8?B?WDE0dzd5eVU3Q09PbEZ5TTJqcksyQjNSNGh4RnNxcTc1NzZIK3k2Z0U0Y21y?=
 =?utf-8?B?Q0dSRE45b2dldFpuNEcvOWpUQTNFM2pydzArQ2kvVllKelhDd0wyTWNQaHFR?=
 =?utf-8?B?Y2o3MGVjc1hzUGRYRUZ5WUtnbHljZTlTL01nQy9QZ1BkMEJWYjEzZmtya01S?=
 =?utf-8?B?T3IySmVNNHFrUVBiMmlaTVR6NUFpdUYvcVNZQjBSRXEwdG1Da09LOXA2RTRu?=
 =?utf-8?B?bXppb3Q1NVovVE5RRzhsUVJva1QrbjAwVW5EVEtIOW9OU00vUDFoLzdwSVd1?=
 =?utf-8?B?U29NUEFhNStUMEN2OUZSQ0xHSFJzMUF5dWlEeDNIYXBjTDZ6L0JCZGJSWkwz?=
 =?utf-8?B?ZUpleHY2eEY2Q29CQ3Y5RWpqWWJZVlJLclhtZmcyT1EyOGptMmd0TGFkM0Iv?=
 =?utf-8?B?Q3FLa3YvTUJVRlZtbDVXMlBoYTd5QkY1WVczeXlOMzROemw4MDkxNnI5L0tz?=
 =?utf-8?B?MGhHbW5uMC94T3VNalRpb3dYbWlyckxBcWhuVVFDYVRuejdOdVI4YjB3akx2?=
 =?utf-8?B?MHZwNmI3TXJRd2xJK3hNcVNKMy9HbEVqSW9acVlleG1FaEs2K2tWdG1aa09B?=
 =?utf-8?B?b0g3UDNCOUJaTmJQb1dQcWcxVWdmVUNES0EvSUhPdjl2Q21oeXdLcWQvbjE0?=
 =?utf-8?B?KzArNjNPN1BHajN0RTZGMEJRYTZiRUFNU2JHMGJUT09JSm9XbW80bG04Q0ZH?=
 =?utf-8?B?QTlxbDhhN09yMVZRZEh2UUxMNE1RZ0VRUnozcG5Ka1lxTCs5QjFEcE1XSXR6?=
 =?utf-8?B?MksrRmN2enJQUDEyelZtSTUyZ2dLejlnbXU3dWg4YituZXRsZmt1cUE1TXZr?=
 =?utf-8?B?TlI1Wjk2SHJNYW8zU2Mza2RRQW85bEhzdWdKY29LT3l6WUUxZ1FQM0xNNkVy?=
 =?utf-8?B?WWIrak5tUkN2eFRYczFncElIRFlWUDFEdkdEeHdnQmVnK09kUktnV2I1dDdJ?=
 =?utf-8?B?Mi8yWmxEak0zUUpaTTBXeEJENFRrSzNYSG9EZTBwNlFCOTc0Y3dLRGFQRkR3?=
 =?utf-8?B?SHJHbldWQUFXN3Ayb3hYb284eGpKdVhXVUZPOCt1b2N5UGgwQUE3N200UG94?=
 =?utf-8?B?UE5mODNFaW82SFZkWFV2M2crcUI4VkdpQjlzbzhVbjNyRGdlRzhYckYxdktH?=
 =?utf-8?B?SFUxVW1VS0RJTE02Y3FGdEhjazhOYkk5MnErdFQwTDR1QTNuRy93M2NvQm5r?=
 =?utf-8?B?c003Tnp1RWpPWmlWYkhYUTZTZldNSkE2OEdJVlRvSmt1elVKU2I5WnBCc0dS?=
 =?utf-8?B?UFFWUTNySnlxOHNDRFR6enF6RG5aNlpQSDdiWFIrcEoxNDFxUUlqOVB0YmhW?=
 =?utf-8?B?eUtRRWJGVktzZVp6cUpwT2lHRHhLOFBURkJMV2N5bFh0NndHQ0ordFNjcDVq?=
 =?utf-8?Q?6qMhqzDxq+ELpx3Z8T8Nfkpq5?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 04a7cb4b-848d-4e29-df1e-08db18efa4eb
X-MS-Exchange-CrossTenant-AuthSource: CO6PR12MB5427.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 27 Feb 2023 18:22:54.8518
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: y67L9ThDSU4zcme1jdYK5oUZ3bZfWYhJYwvjkbcBjs0LMToy2wBQ+8+M9rg0gxrTe17nIsd8oZJPy/lW8H60Hg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR12MB8179
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 2/27/23 12:12, Jani Nikula wrote:
> On Mon, 27 Feb 2023, Harry Wentland <harry.wentland@amd.com> wrote:
>> On 2/26/23 09:10, Yaroslav Bolyukin wrote:
>>> As per DisplayID v2.0 Errata E9 spec "DSC pass-through timing support"
>>> VESA vendor-specific data block may contain target DSC bits per pixel
>>> fields
>>>
>>
>> According to the errata this should only apply to VII timings. The way
>> it is currently implemented will make it apply to everything which is
>> not what we want.
>>
>> Can we add this field to drm_mode_info instead of drm_display_info and
>> set it inside drm_mode_displayid_detailed when parsing a type_7 timing?
> 
> That's actually difficult to do nicely. I think the patch at hand is
> fine, and it's fine to add the information to drm_display_info. It's a
> dependency to parsing the modes.
> 
> How the info will actually be used is a different matter, and obviously
> needs to follow the spec. As it is, *this* patch doesn't say anything
> about that. But whether it's handled in VII timings parsing or
> elsewhere, I still think this part is fine.
> 

This patch itself is okay but without further work can't be used by
drivers since they don't currently have an indication whether a mode
is based on a VII timing.

Harry

> 
> BR,
> Jani.
> 
>>
>> Harry
>>
>>
>>> Signed-off-by: Yaroslav Bolyukin <iam@lach.pw>
>>> ---
>>>  drivers/gpu/drm/drm_edid.c  | 38 +++++++++++++++++++++++++------------
>>>  include/drm/drm_connector.h |  6 ++++++
>>>  include/drm/drm_displayid.h |  4 ++++
>>>  3 files changed, 36 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>>> index 3d0a4da661bc..aa88ac82cbe0 100644
>>> --- a/drivers/gpu/drm/drm_edid.c
>>> +++ b/drivers/gpu/drm/drm_edid.c
>>> @@ -6338,7 +6338,7 @@ static void drm_parse_vesa_mso_data(struct drm_connector *connector,
>>>  	if (oui(vesa->oui[0], vesa->oui[1], vesa->oui[2]) != VESA_IEEE_OUI)
>>>  		return;
>>>  
>>> -	if (sizeof(*vesa) != sizeof(*block) + block->num_bytes) {
>>> +	if (block->num_bytes < 5) {
>>>  		drm_dbg_kms(connector->dev,
>>>  			    "[CONNECTOR:%d:%s] Unexpected VESA vendor block size\n",
>>>  			    connector->base.id, connector->name);
>>> @@ -6361,24 +6361,37 @@ static void drm_parse_vesa_mso_data(struct drm_connector *connector,
>>>  		break;
>>>  	}
>>>  
>>> -	if (!info->mso_stream_count) {
>>> -		info->mso_pixel_overlap = 0;
>>> -		return;
>>> -	}
>>> +	info->mso_pixel_overlap = 0;
>>> +
>>> +	if (info->mso_stream_count) {
>>> +		info->mso_pixel_overlap = FIELD_GET(DISPLAYID_VESA_MSO_OVERLAP, vesa->mso);
>>> +
>>> +		if (info->mso_pixel_overlap > 8) {
>>> +			drm_dbg_kms(connector->dev,
>>> +				    "[CONNECTOR:%d:%s] Reserved MSO pixel overlap value %u\n",
>>> +				    connector->base.id, connector->name,
>>> +				    info->mso_pixel_overlap);
>>> +			info->mso_pixel_overlap = 8;
>>> +		}
>>>  
>>> -	info->mso_pixel_overlap = FIELD_GET(DISPLAYID_VESA_MSO_OVERLAP, vesa->mso);
>>> -	if (info->mso_pixel_overlap > 8) {
>>>  		drm_dbg_kms(connector->dev,
>>> -			    "[CONNECTOR:%d:%s] Reserved MSO pixel overlap value %u\n",
>>> +			    "[CONNECTOR:%d:%s] MSO stream count %u, pixel overlap %u\n",
>>>  			    connector->base.id, connector->name,
>>> -			    info->mso_pixel_overlap);
>>> -		info->mso_pixel_overlap = 8;
>>> +			    info->mso_stream_count, info->mso_pixel_overlap);
>>> +	}
>>> +
>>> +	if (block->num_bytes < 7) {
>>> +		/* DSC bpp is optional */
>>> +		return;
>>>  	}
>>>  
>>> +	info->dp_dsc_bpp = FIELD_GET(DISPLAYID_VESA_DSC_BPP_INT, vesa->dsc_bpp_int) * 16
>>> +		+ FIELD_GET(DISPLAYID_VESA_DSC_BPP_FRACT, vesa->dsc_bpp_fract);
>>> +
>>>  	drm_dbg_kms(connector->dev,
>>> -		    "[CONNECTOR:%d:%s] MSO stream count %u, pixel overlap %u\n",
>>> +		    "[CONNECTOR:%d:%s] DSC bits per pixel %u\n",
>>>  		    connector->base.id, connector->name,
>>> -		    info->mso_stream_count, info->mso_pixel_overlap);
>>> +		    info->dp_dsc_bpp);
>>>  }
>>>  
>>>  static void drm_update_mso(struct drm_connector *connector,
>>> @@ -6425,6 +6438,7 @@ static void drm_reset_display_info(struct drm_connector *connector)
>>>  	info->mso_stream_count = 0;
>>>  	info->mso_pixel_overlap = 0;
>>>  	info->max_dsc_bpp = 0;
>>> +	info->dp_dsc_bpp = 0;
>>>  
>>>  	kfree(info->vics);
>>>  	info->vics = NULL;
>>> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
>>> index 7b5048516185..1d01e0146a7f 100644
>>> --- a/include/drm/drm_connector.h
>>> +++ b/include/drm/drm_connector.h
>>> @@ -719,6 +719,12 @@ struct drm_display_info {
>>>  	 */
>>>  	u32 max_dsc_bpp;
>>>  
>>> +	/**
>>> +	 * @dp_dsc_bpp: DP Display-Stream-Compression (DSC) timing's target
>>> +	 * DST bits per pixel in 6.4 fixed point format. 0 means undefined
>>> +	 */
>>> +	u16 dp_dsc_bpp;
>>> +
>>>  	/**
>>>  	 * @vics: Array of vics_len VICs. Internal to EDID parsing.
>>>  	 */
>>> diff --git a/include/drm/drm_displayid.h b/include/drm/drm_displayid.h
>>> index 49649eb8447e..0fc3afbd1675 100644
>>> --- a/include/drm/drm_displayid.h
>>> +++ b/include/drm/drm_displayid.h
>>> @@ -131,12 +131,16 @@ struct displayid_detailed_timing_block {
>>>  
>>>  #define DISPLAYID_VESA_MSO_OVERLAP	GENMASK(3, 0)
>>>  #define DISPLAYID_VESA_MSO_MODE		GENMASK(6, 5)
>>> +#define DISPLAYID_VESA_DSC_BPP_INT	GENMASK(5, 0)
>>> +#define DISPLAYID_VESA_DSC_BPP_FRACT	GENMASK(3, 0)
>>>  
>>>  struct displayid_vesa_vendor_specific_block {
>>>  	struct displayid_block base;
>>>  	u8 oui[3];
>>>  	u8 data_structure_type;
>>>  	u8 mso;
>>> +	u8 dsc_bpp_int;
>>> +	u8 dsc_bpp_fract;
>>>  } __packed;
>>>  
>>>  /* DisplayID iteration */
>>
>