From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: drm: ensure blend mode supported if alpha exposed Date: Tue, 05 May 2026 08:17:26 +1000 Message-ID: In-Reply-To: <20260504140630.68707-3-leandro.ribeiro@collabora.com> References: <20260504140630.68707-1-leandro.ribeiro@collabora.com> <20260504140630.68707-3-leandro.ribeiro@collabora.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **Verdict: Needs rework =E2=80=94 will break existing drivers.** The design intent is correct: if a plane exposes alpha (either through the = alpha property or alpha-containing pixel formats), userspace needs to know = the blend mode. Enforcing this at driver registration time is the right pla= ce. **Problem 1: Breaks existing in-tree drivers.** As noted above, at least 6 = drivers set `alpha_property` without `blend_mode_property`, and many more s= upport ARGB formats without blend mode. These drivers currently work fine. = After this patch, `drm_dev_register()` returns `-EINVAL` and the driver fai= ls to load entirely. The commit message acknowledges this ("Drivers that hi= t such error must be fixed") but does not include the fixes. This violates the kernel's regression policy =E2=80=94 you cannot merge a c= hange that breaks existing working drivers and expect the fixes to come lat= er. The fixes for all affected drivers must be part of this series (or merg= ed first). **Problem 2: The pixel format alpha check is extremely broad.** Nearly ever= y DRM driver supports ARGB8888 or similar formats. The `has_alpha` check in= `plane_require_blend_mode_for_alpha()` will catch a very large number of d= rivers that don't set up blend mode. This needs careful auditing of every D= RM driver in the tree. **Problem 3: Naming convention.** The split between `drm_mode_config_valida= te()` (WARN-based, non-fatal) and `drm_mode_config_enforce()` (error-return= , fatal) introduces a new pattern. It would be worth documenting why some c= hecks are hard errors and others are soft warnings, or alternatively, consi= der making this a WARN for now and hardening it later once all drivers are = fixed. **Minor issues:** - Typo in commit message: "relly" should be "rely", "don't" should be "does= n't", "count with" should be "count on". - Alignment uses spaces instead of tab at line 373 of the diff: ```c drm_err(dev, "[PLANE:%d:%s] pixel format with alpha exposed but blend mod= e not setup", plane->base.id, plane->name); ``` The `plane->base.id` line appears to use spaces for alignment =E2=80=94 sho= uld use tabs per kernel coding style (compare with the `alpha_property` err= or message a few lines above which uses the correct tab alignment). - The function `drm_mode_config_enforce` is not exported (`EXPORT_SYMBOL`) = which is fine since it's only called from `drm_drv.c`, but it also lacks a = kerneldoc comment. Since `drm_mode_config_validate()` also lacks one, this = is consistent, but worth noting. --- Generated by Claude Code Patch Reviewer