Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues: - [High] The fixed-function CSC configuration (csc_ff_enable and csc_ff_type) and other color pipeline states (LUTs, CTMs) are lost on atomic commits that do not explicitly modify the colorop (e.g., page flips). -- --- Patch [5]: [CI v4 05/14] drm/i915/display: Track CSC mode in intel plane state --- commit d2e5633249bd5c0c74c6412da61ad42bd832980e Author: Chaitanya Kumar Borah <[email protected]> drm/i915/display: Track CSC mode in intel plane state This commit adds CSC mode to hw plane state to enable programming the fixed-function CSC block for SDR planes. It also tracks the bypass state explicitly as a boolean since it is managed separately from the FIXED_MATRIX enum value. > diff --git a/drivers/gpu/drm/i915/display/intel_plane.c > b/drivers/gpu/drm/i915/display/intel_plane.c > index a440f92ff00cd..217c7a84bbfa5 100644 > --- a/drivers/gpu/drm/i915/display/intel_plane.c > +++ b/drivers/gpu/drm/i915/display/intel_plane.c > @@ -430,11 +430,19 @@ intel_plane_color_copy_uapi_to_hw_state(struct > intel_atomic_state *state, > while (iter_colorop) { > for_each_new_colorop_in_state(&state->base, colorop, > new_colorop_state, i) { > if (new_colorop_state->colorop == iter_colorop) { > intel_colorop = to_intel_colorop(colorop); > + if (intel_colorop->id == INTEL_PLANE_CB_CSC_FF) > { > + plane_state->hw.csc_ff_enable = > + !new_colorop_state->bypass; [Severity: High] This is a pre-existing issue, but does this loop fail to restore the hardware state of colorops that are active but not modified in the current atomic commit? Since intel_plane_clear_hw_state() zeroes plane_state->hw before intel_plane_color_copy_uapi_to_hw_state() is called, what happens on a simple page flip that doesn't modify the color pipeline? It looks like for_each_new_colorop_in_state() will only yield colorops that were explicitly added to the atomic commit. Since a simple page flip does not modify the color pipeline, the colorops wouldn't be in the atomic state and the inner loop body wouldn't execute. Could this cause the hardware state for unmodified colorops (including the newly added csc_ff_enable and csc_ff_type) to remain zeroed out, resulting in the loss of color correction or incorrect gamma during video playback or normal desktop use? > + plane_state->hw.csc_ff_type = > + > new_colorop_state->fixed_matrix_type; > + } else { > + blob = new_colorop_state->bypass ? > + NULL : new_colorop_state->data; > + changed |= > intel_plane_colorop_replace_blob(plane_state, > > intel_colorop, > > blob); > + } > } > } > iter_colorop = iter_colorop->next; -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=5
