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. > >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 > > > >>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@xxxxxxxxxxxxxxx> > >>Cc: Bob Paauwe <bob.j.paauwe@xxxxxxxxx> > >>Cc: <dri-devel@xxxxxxxxxxxxxxxxxxxxx> > >>Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@xxxxxxxxx> > >>--- > >> 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@xxxxxxxxxxxxxxxxxxxxx > >>https://lists.freedesktop.org/mailman/listinfo/dri-devel > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
