Re: [PATCH i-g-t v2] tests/kms_color: Always use atomic_commit for setting color properties

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

On Wed, Feb 11, 2026 at 11:24:43AM +0000, Jason-JH Lin (林睿祥) wrote:
> On Wed, 2025-11-26 at 22:40 +0800, Jason-JH.Lin wrote:
> > > > > > > > > 
> > 
> > [snip]
> > 
> > > > > > > > > 
> > > > > > Looking into igt_pipe_commit(), it used
> > > > > > drmModeObjectSetProperty()
> > > > > > to
> > > > > > set property to DRM and it used igt_plane_commit() to
> > > > > > synchronize
> > > > > > all
> > > > > > plane states and only primary plane will call
> > > > > > drmModeSetCrtc().
> > > > > > 
> > > > > > It think the reason we didn't get the new gamma_lut in crtc
> > > > > > state
> > > > > > is
> > > > > > taht set_gamma() will only call igt_pipe_obj_prop_changed()
> > > > > > to
> > > > > > update
> > > > > > pipe_obj->changed flag, but not update plane->changed flag.
> > > > > > In igt_primary_plane_commit_legacy(), it'll return if
> > > > > > primary-
> > > > > > > changed
> > > > > > flag is not set.
> > > > > 
> > > > > The LUT properties are supposed to be on the CRTC. So the plane
> > > > > stuff should not matter, unless you driver is doing something
> > > > > completely non-stanadard.
> > > > > 
> > > > 
> > > > You're right. Plane stuff should not matter to CRTC.
> > > > 
> > > > So I means drmModeSetCrtc() won't be called if plane properties
> > > > is
> > > > not
> > > > set in igt_primary_plane_commit_legacy() currently.
> > > > 
> > > > I think this legacy flow logic:
> > > > {
> > > > 
> > > > if (!igt_plane_is_prop_changed(primary, IGT_PLANE_FB_ID)) &&
> > > >     !(primary->changed & IGT_PLANE_COORD_CHANGED_MASK) &&
> > > >     !igt_pipe_obj_is_prop_changed(primary->pipe,
> > > > IGT_CRTC_MODE_ID))
> > > >         return 0;
> > > > ...
> > > > 
> > > > drmModeSetCrtc()
> > > > 
> > > > ...
> > > > 
> > > > }
> > > > may block LUT properties being synchronized into crtc state when
> > > > we
> > > > only called set_gamma() and not updated any plane properties.
> > > 
> > > That doesn't affect CRTC properties apart from the mode.
> > > For all other CRTC properties drmModeObjectSetProperty()
> > > will be called regardless by igt_pipe_commit().
> > > 
> > 
> > Okay, I checked that drm_mode_obj_set_property_ioctl() called from
> > drmModeObjectSetProperty() will synchronize the CRTC properties, such
> > as, gamm_lut into the CRTC state of atomic_state.
> > 
> > Our drm driver has implemented mode_config.funcs->atomic_commit(), 
> > so maybe we have some bug in our drivers that cause the issue I
> > encountered, the original IGT code should work fine.
> > 
> > I'll check out this later and send the wrapper API if needed.
> > Thanks for the correction and guidance.
> > 
> 
> Finally, I found the issue is that color_mgmt_changed is not set when
> updating gamma_lut, causing gamma changes to not be applied to
> hardware.
> 
> Root cause analysis:
> - When updating GAMMA_LUT, color_mgmt_changed is set correctly.
> - But when a subsequent unrelated property (e.g. VRR_ENABLED) is
> committed, __drm_atomic_helper_crtc_duplicate_state() resets
> color_mgmt_changed=0.
> - This results in crtc->state having gamma_lut but
> color_mgmt_changed=0, so gamma is never applied to hardware.
> 
> DRM framework design:
> - color_mgmt_changed is a per-commit flag, reset on each new commit.
> - Only set if color management properties change.
> - In COMMIT_LEGACY mode (used by IGT), each property is committed
> separately.
> - Unrelated property commits overwrite color_mgmt_changed=1 from
> previous gamma_lut update.
> 
> Solution in MTK DRM driver:
> - Retain color_mgmt_changed if the old state has the flag and color
> management blobs have not changed, preventing it from being overwritten
> by unrelated property commits.

That shouldn't be needed because, as you noted, each property
is commited separately. So the commit resulting from the GAMMA_LUT
setproperty should have already updated the hardware before the
next setproperty commit happens.

-- 
Ville Syrjälä
Intel