Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues: - [Medium] The SDR plane color pipeline is never exposed to userspace because `intel_color_pipeline_plane_init()` unconditionally early-returns for non-HDR planes. - [Low] The fixed matrix support mask validation fails to check bits 32-63 on 32-bit architectures due to unsigned integer zero-extension. Pre-existing issues: - [High] If colorop initialization fails early, the cleanup path dereferences a NULL `colorop->dev` pointer and uninitialized list head, causing a kernel panic. -- --- Patch [8]: [v5 08/14] drm/i915/color: Add YUV range correction to SDR plane pipeline --- commit cf2ffd1a2b5d8ea9f91da1ef444dd3fa882dd254 Author: Chaitanya Kumar Borah <[email protected]> drm/i915/color: Add YUV range correction to SDR plane pipeline Insert the INTEL_PLANE_CB_YUV_RANGE_CORRECT color block into the SDR plane color pipeline. This enables YCbCr limited-to-full range expansion ahead of the CSC block that expects full range pixel data. With this, the pipeline can accept limited range framebuffers. > diff --git a/drivers/gpu/drm/i915/display/intel_color_pipeline.c > b/drivers/gpu/drm/i915/display/intel_color_pipeline.c > index 7d25f4fbfa9b7..7457fa51cb97a 100644 > --- a/drivers/gpu/drm/i915/display/intel_color_pipeline.c > +++ b/drivers/gpu/drm/i915/display/intel_color_pipeline.c > @@ -44,6 +44,7 @@ static const enum intel_color_block hdr_plane_pipeline[] = { > }; > > static const enum intel_color_block sdr_plane_pipeline[] = { > + INTEL_PLANE_CB_YUV_RANGE_CORRECT, > INTEL_PLANE_CB_CSC_FF, > }; [Severity: Medium] Will this newly added SDR plane pipeline actually be exposed to userspace? Looking at intel_color_pipeline_plane_init(), there appears to be an early return that completely prevents color pipelines from being initialized for SDR planes: drivers/gpu/drm/i915/display/intel_color_pipeline.c:intel_color_pipeline_plane_init() { ... /* Currently expose pipeline only for HDR planes */ if (!icl_is_hdr_plane(display, to_intel_plane(plane)->id)) return 0; ... } Because of this early return, doesn't the assignment in _intel_color_pipeline_plane_init() become dead code, leaving the new SDR pipeline features inaccessible? [Severity: High] This is a pre-existing issue, but could the error path in intel_color_pipeline_plane_add_colorop() cause a kernel panic? If drm_plane_colorop_init() fails early (e.g., if drm_mode_object_add() returns -ENOMEM), it returns before colorop->dev is set and before colorop->head is added to the list. When intel_color_pipeline_plane_add_colorop() jumps to the cleanup label: drivers/gpu/drm/i915/display/intel_color_pipeline.c:intel_color_pipeline_plane_add_colorop() { ... cleanup: intel_colorop_destroy(&colorop->base); return ERR_PTR(ret); } This calls drm_colorop_cleanup(), which unconditionally dereferences colorop->dev and calls list_del() on the zeroed list head: drivers/gpu/drm/drm_colorop.c:drm_colorop_cleanup() { struct drm_device *dev = colorop->dev; struct drm_mode_config *config = &dev->mode_config; list_del(&colorop->head); ... } Would this lead to a NULL pointer dereference for colorop->dev and a list corruption panic? [Severity: Low] Does the bitwise validation in drm_plane_colorop_fixed_matrix_init() correctly handle checking on 32-bit architectures? Looking at drivers/gpu/drm/drm_colorop.c:drm_plane_colorop_fixed_matrix_init(): if ((supported_fm & -BIT(DRM_COLOROP_FM_COUNT)) != 0) { The check evaluates -BIT(5). On 32-bit platforms, this becomes a 32-bit unsigned value (0xFFFFFFE0). When bitwise ANDed with the u64 supported_fm, C integer promotion zero-extends it to 0x00000000FFFFFFE0. Would this allow any invalid bits set in supported_fm between 32 and 63 to silently bypass the check? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=8
