[PATCH] drm: atomic: fix legacy gamma set helper
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 > Cc: Bob Paauwe > Cc: > Signed-off-by: Lionel Landwerlin > --- > 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
[PATCH] drm: atomic: fix legacy gamma set helper
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/ > >>> 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. > >> 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 Cc: Bob Paauwe Cc: Signed-off-by: Lionel Landwerlin --- 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
[PATCH] drm: atomic: fix legacy gamma set helper
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 > >>Cc: Bob Paauwe > >>Cc: > >>Signed-off-by: Lionel Landwerlin > >>--- > >> 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
[PATCH] drm: atomic: fix legacy gamma set helper
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? 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 > > 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 > Cc: Bob Paauwe > Cc: > Signed-off-by: Lionel Landwerlin > --- > 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
[PATCH] drm: atomic: fix legacy gamma set helper
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 > > 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. >> 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 >> Cc: Bob Paauwe >> Cc: >> Signed-off-by: Lionel Landwerlin >> --- >> 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
[PATCH] drm: atomic: fix legacy gamma set helper
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. 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 Cc: Bob Paauwe Cc: Signed-off-by: Lionel Landwerlin --- 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
[PATCH] drm: atomic: fix legacy gamma set helper
Op 11-04-16 om 12:37 schreef Lionel Landwerlin: > 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. > > v2: Update non atomic values only if commit succeeds (Bob Paauwe) > > Cc: Maarten Lankhorst > Cc: Bob Paauwe > Cc: > Signed-off-by: Lionel Landwerlin > --- > 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..65e55ce 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); You can't use crtc_state after drm_atomic_commit as it's freed, you need to use crtc->state.
[PATCH] drm: atomic: fix legacy gamma set helper
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. v2: Update non atomic values only if commit succeeds (Bob Paauwe) Cc: Maarten Lankhorst Cc: Bob Paauwe Cc: Signed-off-by: Lionel Landwerlin --- 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..65e55ce 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
[PATCH] drm: atomic: fix legacy gamma set helper
On Mon, 11 Apr 2016 14:43:39 +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. > > v2: Update non atomic values only if commit succeeds (Bob Paauwe) > > v3: Do not access crtc_state after commit, use crtc->state (Maarten > Lankhorst) > Reviewed-by: Bob Paauwe Tested-by; Bob Paauwe > Cc: Maarten Lankhorst > Cc: Bob Paauwe > Cc: > Signed-off-by: Lionel Landwerlin > --- > 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); -- -- Bob Paauwe Bob.J.Paauwe at intel.com IOTG / PED Software Organization Intel Corp. Folsom, CA (916) 356-6193
[PATCH] drm: atomic: fix legacy gamma set helper
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. Cc: Maarten Lankhorst Cc: Bob Paauwe Cc: Signed-off-by: Lionel Landwerlin --- drivers/gpu/drm/drm_atomic_helper.c | 6 ++ 1 file changed, 6 insertions(+) diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 7bf678e..4aacd44 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -2964,16 +2964,22 @@ retry: config->degamma_lut_property, 0); if (ret) goto fail; + drm_object_property_set_value(&crtc->base, + config->degamma_lut_property, 0); ret = drm_atomic_crtc_set_property(crtc, crtc_state, config->ctm_property, 0); if (ret) goto fail; + drm_object_property_set_value(&crtc->base, + config->ctm_property, 0); ret = drm_atomic_crtc_set_property(crtc, crtc_state, config->gamma_lut_property, blob->base.id); if (ret) goto fail; + drm_object_property_set_value(&crtc->base, + config->gamma_lut_property, blob->base.id); ret = drm_atomic_commit(state); if (ret) -- 2.8.0.rc3
[PATCH] drm: atomic: fix legacy gamma set helper
On Fri, 8 Apr 2016 18:17:51 +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. > > Cc: Maarten Lankhorst > Cc: Bob Paauwe > Cc: > Signed-off-by: Lionel Landwerlin > --- > drivers/gpu/drm/drm_atomic_helper.c | 6 ++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c > b/drivers/gpu/drm/drm_atomic_helper.c > index 7bf678e..4aacd44 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -2964,16 +2964,22 @@ retry: > config->degamma_lut_property, 0); > if (ret) > goto fail; > + drm_object_property_set_value(&crtc->base, > + config->degamma_lut_property, 0); > > ret = drm_atomic_crtc_set_property(crtc, crtc_state, > config->ctm_property, 0); > if (ret) > goto fail; > + drm_object_property_set_value(&crtc->base, > + config->ctm_property, 0); > > ret = drm_atomic_crtc_set_property(crtc, crtc_state, > config->gamma_lut_property, blob->base.id); > if (ret) > goto fail; > + drm_object_property_set_value(&crtc->base, > + config->gamma_lut_property, blob->base.id); > This is similar to what I originally did to fix this problem. But if the commit below fails, you've now changed the non atomic values, but the atomic values would be reverted back to the original state so they're out of sync again. So just moving the set_value calls to after the commit succeeds might be a better solution. > ret = drm_atomic_commit(state); > if (ret) -- -- Bob Paauwe Bob.J.Paauwe at intel.com IOTG / PED Software Organization Intel Corp. Folsom, CA (916) 356-6193
