Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
New issues: - [Medium] Hardware state for YUV range correction is incorrectly restricted to SDR planes when a color pipeline is active, preventing HDR planes from bypassing range correction. - [Low] The new YUV range correction color block (INTEL_PLANE_CB_YUV_RANGE_CORRECT) is never added to any plane's color pipeline, making the feature unreachable. Pre-existing issues: - [High] Error paths in colorop initialization lead to a kernel panic because they unconditionally clean up partially initialized colorop objects. - [High] Missing `drm_mode_object_unregister()` in the colorop teardown sequence leaks modeset IDR identifiers, resulting in a Use-After-Free. -- --- Patch [7]: [CI v4 07/14] drm/i915/color: Add YCbCr limited-to-full range color block support --- commit 0f5fc4ef9d6c7498eb8e7e7dcd0a849594fe9377 Author: Chaitanya Kumar Borah <[email protected]> Subject: drm/i915/color: Add YCbCr limited-to-full range color block support This patch adds support for a color block that performs YCbCr limited-to-full range expansion in the plane color pipeline. It represents this new color block using the DRM_COLOROP_FIXED_MATRIX colorop. > diff --git a/drivers/gpu/drm/i915/display/intel_color_pipeline.c > b/drivers/gpu/drm/i915/display/intel_color_pipeline.c > --- a/drivers/gpu/drm/i915/display/intel_color_pipeline.c > +++ b/drivers/gpu/drm/i915/display/intel_color_pipeline.c > @@ -108,6 +108,12 @@ struct intel_colorop > *intel_color_pipeline_plane_add_colorop(struct drm_plane *p > > intel_plane_supported_csc_ff, > > DRM_COLOROP_FLAG_ALLOW_BYPASS); > break; > + case INTEL_PLANE_CB_YUV_RANGE_CORRECT: > + ret = drm_plane_colorop_fixed_matrix_init(dev, &colorop->base, > plane, > + &intel_colorop_funcs, > + > BIT(DRM_COLOROP_FM_YCBCR_LIMITED_FULL), > + > DRM_COLOROP_FLAG_ALLOW_BYPASS); > + break; [Severity: Low] Is this new color block missing from the pipeline definition arrays? It appears INTEL_PLANE_CB_YUV_RANGE_CORRECT is defined and its configuration is implemented, but it is never added to the static pipeline definition arrays like sdr_plane_pipeline. Without being appended to the arrays that dictate pipeline construction, does this feature remain uninstantiated and unreachable by userspace? [Severity: High] This isn't a bug introduced by this patch, but can the error path here lead to a kernel panic? If drm_plane_colorop_fixed_matrix_init() fails, colorop->dev remains uninitialized. The error path jumps to the cleanup label and calls intel_colorop_destroy(), which then calls drm_colorop_cleanup(): drm_colorop_cleanup() struct drm_device *dev = colorop->dev; struct drm_mode_config *config = &dev->mode_config; list_del(&colorop->head); Since dev is NULL, will this blindly dereference a zero-initialized structure and oops on the driver probe error path? [Severity: High] This is a pre-existing issue, but does the colorop teardown sequence leak modeset IDR identifiers and cause a Use-After-Free? When drm_plane_colorop_init() is called, it generates an ID for the colorop->base pointer via drm_mode_object_add(). During teardown, drm_colorop_cleanup() is called but it omits the drm_mode_object_unregister() call. Then, intel_colorop_destroy() frees the memory: intel_colorop_destroy() drm_colorop_cleanup(colorop); kfree(to_intel_colorop(colorop)); Does this leave a dangling pointer in the modeset IDR, where a subsequent userspace lookup of the object ID would fetch and use freed memory? [ ... ] > diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c > b/drivers/gpu/drm/i915/display/skl_universal_plane.c > --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c > +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c > @@ -1296,6 +1296,8 @@ static u32 glk_plane_color_ctl_input_csc(const struct > intel_plane_state *plane_s > } else if (!icl_is_hdr_plane(display, plane->id)) { > if (plane_state->hw.csc_ff_enable) > ctl |= > intel_csc_ff_type_to_csc_mode(plane_state->hw.csc_ff_type); > + if (plane_state->hw.yuv_range_correction_disable) > + ctl |= PLANE_COLOR_YUV_RANGE_CORRECTION_DISABLE; > } > > return ctl; [Severity: Medium] Does this restrict the YUV range correction bypass to SDR planes only? The new check for hw.yuv_range_correction_disable is placed strictly inside the else if (!icl_is_hdr_plane()) block. This seems to force range correction on for HDR planes when a color pipeline is active, bypassing the disable flag completely. Should this check be moved outside the SDR-only restriction to match the legacy path which correctly applies the bypass to both? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=7
