Re: [PATCH 1/3] drm/i915/display: Copy color pipeline from plane in the primary joiner pipe

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Apr 01, 2026 at 07:40:44PM +0530, Borah, Chaitanya Kumar wrote:
> 
> 
> On 4/1/2026 5:10 PM, Ville Syrjälä wrote:
> > On Wed, Apr 01, 2026 at 02:08:39PM +0530, Chaitanya Kumar Borah wrote:
> >> When copying plane color state in a joiner configuration, use the plane in
> >> the primary joiner pipe since it carries the pipeline number selected by
> >> the user-space.
> >>
> >> This assumes that all pipes in the joiner are symmetric in their plane
> >> color capabilities.
> >>
> >> Cc: stable@vger.kernel.org # v6.19+
> >> Fixes: a78f1b6baf4d ("drm/i915/color: Add framework to program CSC")
> >> Signed-off-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>
> >> ---
> >>   drivers/gpu/drm/i915/display/intel_plane.c | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/display/intel_plane.c b/drivers/gpu/drm/i915/display/intel_plane.c
> >> index 5390ceb21ca4..82f445c83158 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_plane.c
> >> +++ b/drivers/gpu/drm/i915/display/intel_plane.c
> >> @@ -373,7 +373,7 @@ intel_plane_color_copy_uapi_to_hw_state(struct intel_plane_state *plane_state,
> >>   	bool changed = false;
> >>   	int i = 0;
> >>   
> >> -	iter_colorop = plane_state->uapi.color_pipeline;
> >> +	iter_colorop = from_plane_state->uapi.color_pipeline;
> >>   
> >>   	while (iter_colorop) {
> >>   		for_each_new_colorop_in_state(state, colorop, new_colorop_state, i) {
> > 
> > Hmm. This whole colorop thing seems a bit weird. So each plane/crtc/etc
> > doesn't actually have its full state in its state, but rather it points
> > to some other colorop state somewhere?
> > 
> 
> Yes, colorops are unique in the DRM model. While they are DRM objects 
> with their own states, they contain information about a plane/crtc's 
> state. The plane/crtc property "COLOR PIPELINE" determines which chain 
> of colorops is currently active.
> 
> 
> > The mess here with the 'intel_atomic_state' here needs to get cleaned up.
> > At the very least we need to pass the full atomic state from the caller
> > instead of digging it out via the plane_state->uapi.state footgun.
> 
> To make the dependency on the intel_atomic_state explicit?
> I will make the change.

Seems we do need to pass NULL occasionally:
- intel_legacy_cursor_update() should be fine since the colorop
  uapi state doesn't change there so nothing to update
- intel_find_initial_plane_obj() really shouldn't use that
  function at all. It really should do the readout into the hw state
  and the do the opposite (hw->uapi) copy. But changing that would
  require real thought, so I guess passing NULL there is fine for
  now as well

> 
> > That thing should never be used, and ideally we'd just nuke it entirely.
> > 
> 
> You mean the back pointer to the atomic state in the plane state?

Yeah. That pointer is only valid under specific circumstances.
If you don't use it, then you don't have to wonder about its validity.

-- 
Ville Syrjälä
Intel