[Intel-gfx] [PATCH] drm: atomic: fix legacy gamma set helper

[Daniel Vetter]

On Mon, Apr 18, 2016 at 11:07:43AM +0100, Lionel Landwerlin wrote:
> On 12/04/16 17:07, Daniel Vetter wrote:
> >On Tue, Apr 12, 2016 at 01:51:02PM +0100, Lionel Landwerlin wrote:
> >>On 12/04/16 12:57, Daniel Vetter wrote:
> >>>On Mon, Apr 11, 2016 at 02:43:39PM +0100, Lionel Landwerlin wrote:
> >>>>Color management properties are a bit of an odd use case because
> >>>>they're not marked as atomic properties. Currently we're not updating
> >>>>the non atomic values so the drm_crtc_state is out of sync with the
> >>>>values stored in the crtc object.
> >>>tbh sounds like this is the bug here - why does the lookup of the correct
> >>>values stored in the crtc_state drm_atomic_crtc_get_property() not work?
> >>This is only a problem when the kernel space modifies property values.
> >>User space ioctls do the right thing and update both places :
> >>
> >>https://cgit.freedesktop.org/drm-intel/tree/drivers/gpu/drm/drm_crtc.c#n4916
> >Nah, the problem is that we check fro DRIVER_ATOMIC in
> >drm_atomic_get_property, and because i915 is still not fully atomic that
> >gives us the wrong answer.
> >
> >Can you pls double-check that enabling i915 atomic (i915.nuclear_flip=1)
> >also fixes the bug?
> >
> >i915 is the only driver still transitioning, I definitely don't want to
> >have hacks all over for it.
> 
> Sorry for the late answer, there is no issue if we're running with
> i915.nuclear_flip=1.
> Should we go with Bob's patch then? :
> 
> https://patchwork.freedesktop.org/patch/80112/

Needs a giant hulking FIXME: Rip out when i915 is DRIVER_ATOMIC, plus cc
Maarten so he doesn't forget. But yeah seems much better.

> 
> >
> >>>Duct-taping over this bug in every place we update these properties
> >>>(you're just patching one of many) won't scale.
> >>>
> >>>Also: Where's the igt for this failure?
> >>>-Daniel
> >>There is : https://cgit.freedesktop.org/xorg/app/intel-gpu-tools/tree/tests/kms_pipe_color.c#n554
> >>That's how we caught it.
> >Needs a Testcase: igt/kms_pipe_color/$name line in the commit message. How
> >a bug was discovered and tested is a crucial part of a complete commit
> >message.
> >-Daniel
> 
> Will do.

Thanks, Daniel

> 
> >
> >>
> >>>>v2: Update non atomic values only if commit succeeds (Bob Paauwe)
> >>>>
> >>>>v3: Do not access crtc_state after commit, use crtc->state (Maarten
> >>>>     Lankhorst)
> >>>>
> >>>>Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> >>>>Cc: Bob Paauwe <bob.j.paauwe at intel.com>
> >>>>Cc: <dri-devel at lists.freedesktop.org>
> >>>>Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin at intel.com>
> >>>>---
> >>>>  drivers/gpu/drm/drm_atomic_helper.c | 8 ++++++++
> >>>>  1 file changed, 8 insertions(+)
> >>>>
> >>>>diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> >>>>index 7bf678e..13b86cf 100644
> >>>>--- a/drivers/gpu/drm/drm_atomic_helper.c
> >>>>+++ b/drivers/gpu/drm/drm_atomic_helper.c
> >>>>@@ -2979,6 +2979,14 @@ retry:
> >>>>  	if (ret)
> >>>>  		goto fail;
> >>>>+	drm_object_property_set_value(&crtc->base,
> >>>>+			config->degamma_lut_property, 0);
> >>>>+	drm_object_property_set_value(&crtc->base,
> >>>>+			config->ctm_property, 0);
> >>>>+	drm_object_property_set_value(&crtc->base,
> >>>>+			config->gamma_lut_property,
> >>>>+			crtc->state->gamma_lut->base.id);
> >>>>+
> >>>>  	/* Driver takes ownership of state on successful commit. */
> >>>>  	drm_property_unreference_blob(blob);
> >>>>-- 
> >>>>2.8.0.rc3
> >>>>
> >>>>_______________________________________________
> >>>>dri-devel mailing list
> >>>>dri-devel at lists.freedesktop.org
> >>>>https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch