On 2025-10-16 18:36, Jani Nikula wrote:
> On Thu, 16 Oct 2025, Yaroslav Bolyukin <[email protected]> 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
>
> Thanks for the patch.
Thanks for the quick review! :D
> I think there's just too much going on in a single patch. Should
> probably be split to several patches:
>
> - rename drm_parse_vesa_mso_data() to drm_parse_vesa_specific_block()
>
> - handle DSC pass-through parts in the above, including the macros for
> parsing that (but nothing about timing here yet), and adding to
> display_info
>
> - note that the above would be needed to backport mso support for 7 byte
> vendor blocks to stable!
Sorry, can you elaborate? Right now stable kernel just ignores
everything going after 5th byte, so it "supports 7 byte blocks" by
ignoring them.
> - Add the detailed timing parsing in a separate patch
>
I'll split the patch as requested
>>
>> Signed-off-by: Yaroslav Bolyukin <[email protected]>
>> ---
>> drivers/gpu/drm/drm_displayid_internal.h | 8 ++++
>> drivers/gpu/drm/drm_edid.c | 61 ++++++++++++++++--------
>> include/drm/drm_connector.h | 6 +++
>> include/drm/drm_modes.h | 10 ++++
>> 4 files changed, 64 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_displayid_internal.h
b/drivers/gpu/drm/drm_displayid_internal.h
>> index 957dd0619f5c..d008a98994bb 100644
>> --- a/drivers/gpu/drm/drm_displayid_internal.h
>> +++ b/drivers/gpu/drm/drm_displayid_internal.h
>> @@ -97,6 +97,10 @@ struct displayid_header {
>> u8 ext_count;
>> } __packed;
>>
>> +#define DISPLAYID_BLOCK_REV GENMASK(2, 0)
>> +#define DISPLAYID_BLOCK_PASSTHROUGH_TIMINGS_SUPPORT BIT(3)
>> +#define DISPLAYID_BLOCK_DESCRIPTOR_PAYLOAD_BYTES GENMASK(6, 4)
>
> These two are related to the rev of struct
> displayid_detailed_timing_block only, and should probably be defined
> next to it.
BLOCK_REV is handled identically for all the displayid block types
afaik, and DISPLAYID_BLOCK_DESCRIPTOR_PAYLOAD_BYTES is unrelated to the
timings block, I didn't want to spread the masks around the file, but
will do if you think that's better.
>> +
>> struct displayid_block {
>> u8 tag;
>> u8 rev;
>> @@ -144,12 +148,16 @@ struct displayid_formula_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;
>>
>> /*
>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>> index e2e85345aa9a..6e42e55b41f9 100644
>> --- a/drivers/gpu/drm/drm_edid.c
>> +++ b/drivers/gpu/drm/drm_edid.c
>> @@ -6524,8 +6524,8 @@ static void drm_get_monitor_range(struct
drm_connector *connector,
>> info->monitor_range.min_vfreq,
info->monitor_range.max_vfreq);
>> }
>>
>> -static void drm_parse_vesa_mso_data(struct drm_connector *connector,
>> - const struct displayid_block *block)
>> +static void drm_parse_vesa_specific_block(struct drm_connector
*connector,
>> + const struct displayid_block *block)
>> {
>> struct displayid_vesa_vendor_specific_block *vesa =
>> (struct displayid_vesa_vendor_specific_block *)block;
>> @@ -6541,7 +6541,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);
>> @@ -6564,28 +6564,40 @@ 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;
>
> Nitpick, I kind of like having this in the else path below instead of
> first setting it to 0 and then setting it again to something else.
>>>
>> - 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;
>> + 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;
>> + }
>> }
>>
>> drm_dbg_kms(connector->dev,
>> "[CONNECTOR:%d:%s] MSO stream count %u, pixel overlap
%u\n",
>> connector->base.id, connector->name,
>> info->mso_stream_count, info->mso_pixel_overlap);
>
> Not sure we want to debug log this unless info->mso_stream_count !=
> 0. This is a rare feature.
>
> Side note, we seem to be lacking the check for
> data_structure_type. Probably my bad. I'm not asking you to fix it, but
> hey, if you're up for it, another patch is welcome! ;)
I see, MSO overlap/stream count shouldn't be parsed for eDP, I'll do it.
Is that what you meant by "note that the above would be needed to
backport mso support for 7 byte vendor blocks to stable!"?
>> +
>> + 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) << 4 |
>> + FIELD_GET(DISPLAYID_VESA_DSC_BPP_FRACT,
vesa->dsc_bpp_fract);
>> +
>> + drm_dbg_kms(connector->dev,
>> + "[CONNECTOR:%d:%s] DSC bits per pixel %u\n",
>> + connector->base.id, connector->name,
>> + info->dp_dsc_bpp);
>> }
>>
>> -static void drm_update_mso(struct drm_connector *connector,
>> - const struct drm_edid *drm_edid)
>> +static void drm_update_vesa_specific_block(struct drm_connector
*connector,
>> + const struct drm_edid *drm_edid)
>> {
>> const struct displayid_block *block;
>> struct displayid_iter iter;
>> @@ -6593,7 +6605,7 @@ static void drm_update_mso(struct
drm_connector *connector,
>> displayid_iter_edid_begin(drm_edid, &iter);
>> displayid_iter_for_each(block, &iter) {
>> if (block->tag == DATA_BLOCK_2_VENDOR_SPECIFIC)
>> - drm_parse_vesa_mso_data(connector, block);
>> + drm_parse_vesa_specific_block(connector, block);
>> }
>> displayid_iter_end(&iter);
>> }
>> @@ -6630,6 +6642,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;
>> @@ -6753,7 +6766,7 @@ static void update_display_info(struct
drm_connector *connector,
>> if (edid->features & DRM_EDID_FEATURE_RGB_YCRCB422)
>> info->color_formats |= DRM_COLOR_FORMAT_YCBCR422;
>>
>> - drm_update_mso(connector, drm_edid);
>> + drm_update_vesa_specific_block(connector, drm_edid);
>>
>> out:
>> if (drm_edid_has_internal_quirk(connector, EDID_QUIRK_NON_DESKTOP)) {
>> @@ -6784,7 +6797,8 @@ static void update_display_info(struct
drm_connector *connector,
>>
>> static struct drm_display_mode *drm_mode_displayid_detailed(struct
drm_device *dev,
>> const struct
displayid_detailed_timings_1 *timings,
>> - bool type_7)
>> + bool type_7,
>> + int rev)
>
> If we added struct displayid_detailed_timing_block *block parameter
> (between dev and timings), the function could figure it all out from
> there instead of having to pass several parameters. Dunno which is
> cleaner. It's also not neat to pass rev as int, when it's really data
> that has to be parsed.
I agree, just didn't like passing both the block and struct from the
block (timings param), but it should be fine, I'll redo it.
>> {
>> struct drm_display_mode *mode;
>> unsigned int pixel_clock = (timings->pixel_clock[0] |
>> @@ -6805,6 +6819,10 @@ static struct drm_display_mode
*drm_mode_displayid_detailed(struct drm_device *d
>> if (!mode)
>> return NULL;
>>
>> + if (type_7 && FIELD_GET(DISPLAYID_BLOCK_REV, rev) >= 1)
>> + mode->dsc_passthrough_timings_support =
>> + !!(rev & DISPLAYID_BLOCK_PASSTHROUGH_TIMINGS_SUPPORT);
>
> I wonder if it would make life easier all around if we just filled the
> dp_dsc_bpp in the mode itself, instead of having a flag and having to
> look it up separately?
They are stored in the separate blocks, and vesa vendor specific block
can be located after the timings blocks, meaning to do that we need to
iterate over all the mode blocks again and parse their timings support
flag from rev again to fill this data. I don't like this either, but
seems like this is the most logical implementation.
We also have max_dsc_bpp declared in display_mode, and it should be
related to this.
It also won't help with the fact that it is hard to handle mode flag for
the modes created at runtime (see AMDGPU patch). I believe there should
be a fancier way to do this, but this anin't it.
I still have troubles understanding why does this flag need to exist, as
far as I can see, every device with passthrough timings doesn't have
both modes using them and not using them, and the implementation doesn't
look good due to this fact.
