| Message ID | 1460382219-29428-1-git-send-email-lionel.g.landwerlin@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, 11 Apr 2016 14:43:39 +0100 Lionel Landwerlin <lionel.g.landwerlin@intel.com> 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 <bob.j.paauwe@intel.com> Tested-by; Bob Paauwe <bob.j.paauwe@intel.com> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Cc: Bob Paauwe <bob.j.paauwe@intel.com> > Cc: <dri-devel@lists.freedesktop.org> > Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@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);
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 <maarten.lankhorst@linux.intel.com> > Cc: Bob Paauwe <bob.j.paauwe@intel.com> > Cc: <dri-devel@lists.freedesktop.org> > Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@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@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
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 <maarten.lankhorst@linux.intel.com> >> Cc: Bob Paauwe <bob.j.paauwe@intel.com> >> Cc: <dri-devel@lists.freedesktop.org> >> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@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@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
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@linux.intel.com> > >>Cc: Bob Paauwe <bob.j.paauwe@intel.com> > >>Cc: <dri-devel@lists.freedesktop.org> > >>Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@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@lists.freedesktop.org > >>https://lists.freedesktop.org/mailman/listinfo/dri-devel >
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 <maarten.lankhorst@linux.intel.com> >>>> Cc: Bob Paauwe <bob.j.paauwe@intel.com> >>>> Cc: <dri-devel@lists.freedesktop.org> >>>> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@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@lists.freedesktop.org >>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
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@linux.intel.com> > >>>>Cc: Bob Paauwe <bob.j.paauwe@intel.com> > >>>>Cc: <dri-devel@lists.freedesktop.org> > >>>>Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@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@lists.freedesktop.org > >>>>https://lists.freedesktop.org/mailman/listinfo/dri-devel >
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);
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 <maarten.lankhorst@linux.intel.com> Cc: Bob Paauwe <bob.j.paauwe@intel.com> Cc: <dri-devel@lists.freedesktop.org> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com> --- drivers/gpu/drm/drm_atomic_helper.c | 8 ++++++++ 1 file changed, 8 insertions(+)