| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <87o6za3ujp.fsf@intel.com>
Date: Mon, 10 Feb 2025 13:31:38 +0200
From: Jani Nikula <jani.nikula@...ux.intel.com>
To: Egor Vorontsov <sdoregor@...re.me>, linux-kernel@...r.kernel.org
Cc: dri-devel@...ts.freedesktop.org, Maarten Lankhorst
<maarten.lankhorst@...ux.intel.com>, Maxime Ripard <mripard@...nel.org>,
Thomas Zimmermann <tzimmermann@...e.de>, Egor Vorontsov
<sdoregor@...re.me>
Subject: Re: [PATCH RESEND] drm/edid: Implement DisplayID Type IX & X timing
blocks parsing
On Sat, 08 Feb 2025, Egor Vorontsov <sdoregor@...re.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@...se.io>
> Signed-off-by: Egor Vorontsov <sdoregor@...re.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);
--
Jani Nikula, Intel
Powered by blists - more mailing lists