Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
New issues: - [High] The patch relies on core DRM functions and enum values that are not defined anywhere in the kernel tree. - [Medium] The newly added SDR plane color pipeline is entirely unreachable because the caller function restricts pipeline initialization exclusively to HDR planes. - [Medium] The fixed-function CSC colorop state is exposed to userspace, but the driver completely ignores its configuration when programming the hardware. Pre-existing issues: - [High] The error cleanup path for colorop initialization calls `drm_colorop_cleanup` on partially initialized objects, causing a NULL pointer dereference. - [High] Missing error handling for `kasprintf` memory allocation failure leads to a kernel panic. -- commit d4c7800e456ca3d700da2c92787c702c23a7fcd5 Author: Chaitanya Kumar Borah <[email protected]> Subject: drm/i915/color: Add CSC on SDR plane color pipeline This commit introduces a fixed-function CSC block to the SDR plane color pipeline as a DRM_COLOROP_FIXED_MATRIX color operation. It adds the new pipeline definition and initializes it for SDR planes during setup. > diff --git a/drivers/gpu/drm/i915/display/intel_color_pipeline.c > b/drivers/gpu/drm/i915/display/intel_color_pipeline.c > index 6cf8080ee8000..efd4375c43318 100644 > --- a/drivers/gpu/drm/i915/display/intel_color_pipeline.c > +++ b/drivers/gpu/drm/i915/display/intel_color_pipeline.c [ ... ] > @@ -43,6 +43,18 @@ static const enum intel_color_block hdr_plane_pipeline[] = > { > INTEL_PLANE_CB_POST_CSC_LUT, > }; > > +static const enum intel_color_block sdr_plane_yuv_pipeline[] = { > + INTEL_PLANE_CB_CSC_FF, > +}; > + > +static const u64 intel_plane_supported_csc_ff = > + BIT(DRM_COLOROP_FM_YCBCR601_FULL_RGB) | > + BIT(DRM_COLOROP_FM_YCBCR601_LIMITED_RGB) | > + BIT(DRM_COLOROP_FM_YCBCR709_FULL_RGB) | > + BIT(DRM_COLOROP_FM_YCBCR709_LIMITED_RGB) | > + BIT(DRM_COLOROP_FM_YCBCR2020_NC_FULL_RGB) | > + BIT(DRM_COLOROP_FM_YCBCR2020_NC_LIMITED_RGB); [Severity: High] Are these enum values (e.g., DRM_COLOROP_FM_YCBCR601_FULL_RGB) defined in the kernel tree? It appears they are not defined anywhere, which would cause a compilation failure. Is there a prerequisite patch missing from this series? [ ... ] > @@ -92,6 +104,12 @@ struct intel_colorop > *intel_color_pipeline_plane_add_colorop(struct drm_plane *p > > DRM_COLOROP_LUT1D_INTERPOLATION_LINEAR, > > DRM_COLOROP_FLAG_ALLOW_BYPASS); > break; > + case INTEL_PLANE_CB_CSC_FF: > + ret = drm_plane_colorop_fixed_matrix_init(dev, &colorop->base, > plane, > + &intel_colorop_funcs, > + > intel_plane_supported_csc_ff, > + > DRM_COLOROP_FLAG_ALLOW_BYPASS); [Severity: High] Is drm_plane_colorop_fixed_matrix_init() implemented in another patch? This core DRM function does not seem to be defined in the tree, which will cause a build failure. [Severity: High] This isn't a bug introduced by this patch, but can the error path for this initialization lead to a NULL pointer dereference? If drm_plane_colorop_fixed_matrix_init() fails early, it jumps to the cleanup label: if (ret) goto cleanup; ... cleanup: intel_colorop_destroy(&colorop->base); This calls drm_colorop_cleanup(), which dereferences colorop->dev: void drm_colorop_cleanup(struct drm_colorop *colorop) { struct drm_device *dev = colorop->dev; struct drm_mode_config *config = &dev->mode_config; Since colorop was just allocated with kzalloc and colorop->dev might not be set yet if initialization fails early, could dev be NULL here? > + break; > default: > drm_err(plane->dev, "Invalid colorop id [%d]", id); > ret = -EINVAL; > @@ -126,9 +144,12 @@ int _intel_color_pipeline_plane_init(struct drm_plane > *plane, struct drm_prop_en > if (plane_has_3dlut(display, pipe, plane)) { > pipeline = xe3plpd_primary_plane_pipeline; > pipeline_len = ARRAY_SIZE(xe3plpd_primary_plane_pipeline); > - } else { > + } else if (icl_is_hdr_plane(display, to_intel_plane(plane)->id)) { > pipeline = hdr_plane_pipeline; > pipeline_len = ARRAY_SIZE(hdr_plane_pipeline); > + } else { > + pipeline = sdr_plane_yuv_pipeline; > + pipeline_len = ARRAY_SIZE(sdr_plane_yuv_pipeline); > } [Severity: Medium] Will this newly added SDR pipeline logic ever be reached? The caller function intel_color_pipeline_plane_init() unconditionally restricts pipeline initialization to HDR planes with an early return: int 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; ret = _intel_color_pipeline_plane_init(plane, &pipelines[len], pipe); ... } Does this early return make the sdr_plane_yuv_pipeline dead code? [Severity: High] This isn't a bug introduced by this patch, but does the kasprintf() call slightly below this block in _intel_color_pipeline_plane_init() need a NULL check? list->name = kasprintf(GFP_KERNEL, "Color Pipeline %d", ...); return 0; If kasprintf() fails due to OOM and returns NULL, the function still returns 0 (success). Could this propagate a NULL pointer to drm_property_add_enum(), leading to a kernel panic when it tries to read the name length? [Severity: Medium] Once the fixed-function CSC colorop state (INTEL_PLANE_CB_CSC_FF) is set by userspace, does the driver correctly apply it to the hardware? Looking at intel_plane_color_copy_uapi_to_hw_state(), there appears to be no logic in intel_plane_colorop_replace_blob() to extract the new fixed matrix configuration. Furthermore, the hardware programming in glk_plane_color_ctl() continues to rely on the legacy hw.color_encoding field: static u32 glk_plane_color_ctl(const struct intel_plane_state *plane_state) { ... if (fb->format->is_yuv && !icl_is_hdr_plane(display, plane->id)) { switch (plane_state->hw.color_encoding) { case DRM_COLOR_YCBCR_BT709: ... Could this result in the hardware being programmed with the legacy state instead of the newly requested color pipeline configuration? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
