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 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 | 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)
On Fri, 8 Apr 2016 18:17:51 +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.
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 | 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)
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 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..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);
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 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..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.
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(+)
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, 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_co... 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_co... 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_co... 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? :
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_co... 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
dri-devel@lists.freedesktop.org
-
Bob Paauwe -
Daniel Vetter -
Lionel Landwerlin -
Maarten Lankhorst