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@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 _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
