| Message ID | e60254ec3544d37367c9917bc0e1cd5fdb95248d.camel@sdore.me (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RESEND] drm/edid: Implement DisplayID Type IX & X timing blocks parsing | expand |
On Sat, 08 Feb 2025, Egor Vorontsov <sdoregor@sdore.me> wrote: > Some newer high refresh rate consumer monitors (including those by Samsung) > make use of DisplayID 2.1 timing blocks in their EDID data, notably for > their highest refresh rate modes. Such modes won't be available as of now. > > Implement partial support for such blocks in order to enable native > support of HRR modes of most such monitors for users without having to rely > on EDID patching/override (or need thereof). Thanks for the patch. It appears to do what's desired, but please find quite a bit of comments inline. > Link: https://gitlab.freedesktop.org/drm/misc/kernel/-/issues/55 I think "Closes:" is what we want. > Suggested-by: Maximilian Boße <max@bosse.io> > Signed-off-by: Egor Vorontsov <sdoregor@sdore.me> > > --- > > The formatting was taken from the neighboring code for consistency, > thus some warnings. Sometimes it's good to follow the surrounding code, but let's not duplicate existing mistakes. ;) > > [Resent due to some Spamhaus issues that are now resolved.] > > drivers/gpu/drm/drm_displayid_internal.h | 13 +++++ > drivers/gpu/drm/drm_edid.c | 61 ++++++++++++++++++++++++ > 2 files changed, 74 insertions(+) > > diff --git a/drivers/gpu/drm/drm_displayid_internal.h b/drivers/gpu/drm/drm_displayid_internal.h > index aee1b86a73c1..a75d0f637b72 100644 > --- a/drivers/gpu/drm/drm_displayid_internal.h > +++ b/drivers/gpu/drm/drm_displayid_internal.h > @@ -66,6 +66,7 @@ struct drm_edid; > #define DATA_BLOCK_2_STEREO_DISPLAY_INTERFACE 0x27 > #define DATA_BLOCK_2_TILED_DISPLAY_TOPOLOGY 0x28 > #define DATA_BLOCK_2_CONTAINER_ID 0x29 > +#define DATA_BLOCK_2_TYPE_10_FORMULA_TIMING 0x2a > #define DATA_BLOCK_2_VENDOR_SPECIFIC 0x7e > #define DATA_BLOCK_2_CTA_DISPLAY_ID 0x81 > > @@ -129,6 +130,18 @@ struct displayid_detailed_timing_block { > struct displayid_detailed_timings_1 timings[]; > }; > > +struct displayid_formula_timings_9 { > + u8 flags; > + u8 hactive[2]; > + u8 vactive[2]; Regardless of what was done in struct displayid_detailed_timings_1, I'd go for defining these as: __be16 hactive; __be16 vactive; and using be16_to_cpu() instead of hand-rolling it. > + u8 refresh; Nitpick, I'd go for vrefresh. > +} __packed; > + > +struct displayid_formula_timing_block { > + struct displayid_block base; > + struct displayid_formula_timings_9 timings[]; > +}; I know it's lacking in struct displayid_detailed_timing_block, but I'd add __packed here too. > + > #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 13bc4c290b17..8a4dec1d781c 100644 > --- a/drivers/gpu/drm/drm_edid.c > +++ b/drivers/gpu/drm/drm_edid.c > @@ -6833,6 +6833,64 @@ static int add_displayid_detailed_1_modes(struct drm_connector *connector, > return num_modes; > } > > +static struct drm_display_mode *drm_mode_displayid_formula(struct drm_device *dev, > + struct displayid_formula_timings_9 *timings, const > + bool type_10) > +{ > + struct drm_display_mode *mode; > + unsigned hactive = (timings->hactive[0] | timings->hactive[1] << 8) + 1; > + unsigned vactive = (timings->vactive[0] | timings->vactive[1] << 8) + 1; u16 hactive = be16_to_cpu(timings->hactive) + 1; etc. > + u8 rb = timings->flags & 0b111; I'd call this formula or algorithm instead of just rb. Please avoid 0b, it's very rarely used. Just 0x7. Or we could start defining macros for these in drm_displayid_internal.h... Please add a blank line between declarations and code here. > + /* TODO: support RB-v2 & RB-v3 */ > + if (rb > 1) > + return NULL; > + > + mode = drm_cvt_mode(dev, hactive, vactive, timings->refresh, rb == 1, false, false); Refresh rate is -1 in the data block, so this needs timings->refresh + 1. > + if (!mode) > + return NULL; > + > + /* TODO: interpret S3D flags */ More important is TODO for fractional refresh rate indicated by bit 4 of flags. > + > + mode->type = DRM_MODE_TYPE_DRIVER; > + > + if (timings->flags & 0x80) > + mode->type |= DRM_MODE_TYPE_PREFERRED; There's no such thing? > + drm_mode_set_name(mode); > + > + return mode; > +} > + > +static int add_displayid_formula_modes(struct drm_connector *connector, > + const struct displayid_block *block) > +{ > + struct displayid_formula_timing_block *fb = (struct displayid_formula_timing_block *)block; Please avoid fb as the name for this. Everyone's going to go all "framebuffer", wtf, backtrack, oh, "formula block". ;) This should also be a const pointer. (I know, the detailed modes one isn't. Let's not duplicate mistakes from there.) > + int num_timings; > + struct drm_display_mode *newmode; > + int num_modes = 0; > + bool type_10 = block->tag == DATA_BLOCK_2_TYPE_10_FORMULA_TIMING; > + u8 timing_size = 6 + ((fb->base.rev & 0x70) >> 4); I'd go for int timing_size. Blank line here. > + /* extended blocks are not supported yet */ > + if (timing_size != 6) > + return 0; > + > + if (block->num_bytes % timing_size) > + return 0; > + > + num_timings = block->num_bytes / timing_size; > + for (int i = 0; i < num_timings; i++) { > + struct displayid_formula_timings_9 *timings = \ const. Please don't use \ line continuations, it's not necessary. > + (struct displayid_formula_timings_9 *)&((u8 *)fb->timings)[i * timing_size]; Since we only support the one size for now, and that size is fixed in struct displayid_formula_timings_9, there's no need to do all this hackery. Just formula->timings[i]. Whoever fixes this for size 7 might have a better idea how to handle the size anyway. > + > + newmode = drm_mode_displayid_formula(connector->dev, timings, type_10); > + if (!newmode) > + continue; > + > + drm_mode_probed_add(connector, newmode); > + num_modes++; > + } > + return num_modes; > +} > + > static int add_displayid_detailed_modes(struct drm_connector *connector, > const struct drm_edid *drm_edid) > { > @@ -6845,6 +6903,9 @@ static int add_displayid_detailed_modes(struct drm_connector *connector, > if (block->tag == DATA_BLOCK_TYPE_1_DETAILED_TIMING || > block->tag == DATA_BLOCK_2_TYPE_7_DETAILED_TIMING) > num_modes += add_displayid_detailed_1_modes(connector, block); > + else if (block->tag == DATA_BLOCK_2_TYPE_9_FORMULA_TIMING || > + block->tag == DATA_BLOCK_2_TYPE_10_FORMULA_TIMING) > + num_modes += add_displayid_formula_modes(connector, block); > } > displayid_iter_end(&iter);
On Mon, 2025-02-10 at 13:31 +0200, Jani Nikula wrote: > I think "Closes:" is what we want. > Sometimes it's good to follow the surrounding code, but let's not > duplicate existing mistakes. ;) Ack! > Please avoid 0b, it's very rarely used. Just 0x7. Or we could start > defining macros for these in drm_displayid_internal.h... I guess I'll leave the macros to future implementation, as clearly the naming here is subject to seeing the actual DisplayID 2.1 specs PDF, which is non-public as of now. > Refresh rate is -1 in the data block, so this needs timings->refresh + 1. Ack, nice catch! Worked for me, might've not for someone else's case. > > + if (timings->flags & 0x80) > > + mode->type |= DRM_MODE_TYPE_PREFERRED; > There's no such thing? Oh, accidentally picked up from `drm_mode_displayid_detailed()'. Ack. > > + u8 timing_size = 6 + ((fb->base.rev & 0x70) >> 4); > I'd go for int timing_size. I'd actually vote for an unsigned int, as it's a length and can't possibly be negative. But sticking to the M.O. of the file here. Thank you for the review! Fixed per all other notes, as well. Will resubmit soon, Egor
diff --git a/drivers/gpu/drm/drm_displayid_internal.h b/drivers/gpu/drm/drm_displayid_internal.h index aee1b86a73c1..a75d0f637b72 100644 --- a/drivers/gpu/drm/drm_displayid_internal.h +++ b/drivers/gpu/drm/drm_displayid_internal.h @@ -66,6 +66,7 @@ struct drm_edid; #define DATA_BLOCK_2_STEREO_DISPLAY_INTERFACE 0x27 #define DATA_BLOCK_2_TILED_DISPLAY_TOPOLOGY 0x28 #define DATA_BLOCK_2_CONTAINER_ID 0x29 +#define DATA_BLOCK_2_TYPE_10_FORMULA_TIMING 0x2a #define DATA_BLOCK_2_VENDOR_SPECIFIC 0x7e #define DATA_BLOCK_2_CTA_DISPLAY_ID 0x81 @@ -129,6 +130,18 @@ struct displayid_detailed_timing_block { struct displayid_detailed_timings_1 timings[]; }; +struct displayid_formula_timings_9 { + u8 flags; + u8 hactive[2]; + u8 vactive[2]; + u8 refresh; +} __packed; + +struct displayid_formula_timing_block { + struct displayid_block base; + struct displayid_formula_timings_9 timings[]; +}; + #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 13bc4c290b17..8a4dec1d781c 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -6833,6 +6833,64 @@ static int add_displayid_detailed_1_modes(struct drm_connector *connector, return num_modes; } +static struct drm_display_mode *drm_mode_displayid_formula(struct drm_device *dev, + struct displayid_formula_timings_9 *timings, + bool type_10) +{ + struct drm_display_mode *mode; + unsigned hactive = (timings->hactive[0] | timings->hactive[1] << 8) + 1; + unsigned vactive = (timings->vactive[0] | timings->vactive[1] << 8) + 1; + u8 rb = timings->flags & 0b111; + /* TODO: support RB-v2 & RB-v3 */ + if (rb > 1) + return NULL; + + mode = drm_cvt_mode(dev, hactive, vactive, timings->refresh, rb == 1, false, false); + if (!mode) + return NULL; + + /* TODO: interpret S3D flags */ + + mode->type = DRM_MODE_TYPE_DRIVER; + + if (timings->flags & 0x80) + mode->type |= DRM_MODE_TYPE_PREFERRED; + drm_mode_set_name(mode); + + return mode; +} + +static int add_displayid_formula_modes(struct drm_connector *connector, + const struct displayid_block *block) +{ + struct displayid_formula_timing_block *fb = (struct displayid_formula_timing_block *)block; + int num_timings; + struct drm_display_mode *newmode; + int num_modes = 0; + bool type_10 = block->tag == DATA_BLOCK_2_TYPE_10_FORMULA_TIMING; + u8 timing_size = 6 + ((fb->base.rev & 0x70) >> 4); + /* extended blocks are not supported yet */ + if (timing_size != 6) + return 0; + + if (block->num_bytes % timing_size) + return 0; + + num_timings = block->num_bytes / timing_size; + for (int i = 0; i < num_timings; i++) { + struct displayid_formula_timings_9 *timings = \ + (struct displayid_formula_timings_9 *)&((u8 *)fb->timings)[i * timing_size]; + + newmode = drm_mode_displayid_formula(connector->dev, timings, type_10); + if (!newmode) + continue; + + drm_mode_probed_add(connector, newmode); + num_modes++; + } + return num_modes; +} + static int add_displayid_detailed_modes(struct drm_connector *connector, const struct drm_edid *drm_edid) { @@ -6845,6 +6903,9 @@ static int add_displayid_detailed_modes(struct drm_connector *connector, if (block->tag == DATA_BLOCK_TYPE_1_DETAILED_TIMING || block->tag == DATA_BLOCK_2_TYPE_7_DETAILED_TIMING) num_modes += add_displayid_detailed_1_modes(connector, block); + else if (block->tag == DATA_BLOCK_2_TYPE_9_FORMULA_TIMING || + block->tag == DATA_BLOCK_2_TYPE_10_FORMULA_TIMING) + num_modes += add_displayid_formula_modes(connector, block); } displayid_iter_end(&iter);
Some newer high refresh rate consumer monitors (including those by Samsung) make use of DisplayID 2.1 timing blocks in their EDID data, notably for their highest refresh rate modes. Such modes won't be available as of now. Implement partial support for such blocks in order to enable native support of HRR modes of most such monitors for users without having to rely on EDID patching/override (or need thereof). Link: https://gitlab.freedesktop.org/drm/misc/kernel/-/issues/55 Suggested-by: Maximilian Boße <max@bosse.io> Signed-off-by: Egor Vorontsov <sdoregor@sdore.me> --- The formatting was taken from the neighboring code for consistency, thus some warnings. [Resent due to some Spamhaus issues that are now resolved.] drivers/gpu/drm/drm_displayid_internal.h | 13 +++++ drivers/gpu/drm/drm_edid.c | 61 ++++++++++++++++++++++++ 2 files changed, 74 insertions(+)