[PATCH] drm: atomic: fix legacy gamma set helper

[PATCH] drm: atomic: fix legacy gamma set helper

[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.

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)
-- 
2.8.0.rc3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm: atomic: fix legacy gamma set helper

[Bob Paauwe]

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)



-- 
--
Bob Paauwe                  
Bob.J.Paauwe@intel.com
IOTG / PED Software Organization
Intel Corp.  Folsom, CA
(916) 356-6193    

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: warning for drm: atomic: fix legacy gamma set helper

[Patchwork]

== Series Details ==

Series: drm: atomic: fix legacy gamma set helper
URL   : https://patchwork.freedesktop.org/series/5471/
State : warning

== Summary ==

Series 5471v1 drm: atomic: fix legacy gamma set helper
http://patchwork.freedesktop.org/api/1.0/series/5471/revisions/1/mbox/

Test gem_exec_basic:
        Subgroup basic-bsd:
                dmesg-warn -> PASS       (bsw-nuc-2)
Test gem_exec_suspend:
        Subgroup basic-s3:
                dmesg-warn -> PASS       (bsw-nuc-2)
Test gem_sync:
        Subgroup basic-bsd:
                dmesg-warn -> PASS       (bsw-nuc-2)
Test kms_flip:
        Subgroup basic-flip-vs-dpms:
                pass       -> DMESG-WARN (ilk-hp8440p) UNSTABLE
Test kms_force_connector_basic:
        Subgroup force-connector-state:
                pass       -> SKIP       (snb-x220t)
        Subgroup force-load-detect:
                pass       -> SKIP       (snb-x220t)
        Subgroup prune-stale-modes:
                pass       -> SKIP       (ivb-t430s)
Test pm_rpm:
        Subgroup basic-pci-d3-state:
                pass       -> DMESG-WARN (byt-nuc)
        Subgroup basic-rte:
                dmesg-warn -> PASS       (bsw-nuc-2)

bdw-nuci7        total:196  pass:184  dwarn:0   dfail:0   fail:0   skip:12 
bdw-ultra        total:196  pass:175  dwarn:0   dfail:0   fail:0   skip:21 
bsw-nuc-2        total:196  pass:159  dwarn:0   dfail:0   fail:0   skip:37 
byt-nuc          total:196  pass:160  dwarn:1   dfail:0   fail:0   skip:35 
hsw-brixbox      total:196  pass:174  dwarn:0   dfail:0   fail:0   skip:22 
ilk-hp8440p      total:196  pass:131  dwarn:1   dfail:0   fail:0   skip:64 
ivb-t430s        total:196  pass:170  dwarn:0   dfail:0   fail:0   skip:26 
skl-i7k-2        total:196  pass:173  dwarn:0   dfail:0   fail:0   skip:23 
snb-dellxps      total:196  pass:162  dwarn:0   dfail:0   fail:0   skip:34 
snb-x220t        total:196  pass:160  dwarn:0   dfail:0   fail:1   skip:35 

Results at /archive/results/CI_IGT_test/Patchwork_1851/

949884a57b51aa158e3ae9afe1f08130cdb7a3ef drm-intel-nightly: 2016y-04m-08d-10h-45m-28s UTC integration manifest
ff2301ce4c39a6e755adac9f45225f4e1648336f drm: atomic: fix legacy gamma set helper

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm: atomic: fix legacy gamma set helper

[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);
-- 
2.8.0.rc3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: atomic: fix legacy gamma set helper

[Maarten Lankhorst]

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

[PATCH] drm: atomic: fix legacy gamma set helper

[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)

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

Re: [PATCH] drm: atomic: fix legacy gamma set helper

[Bob Paauwe]

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);



-- 
--
Bob Paauwe                  
Bob.J.Paauwe@intel.com
IOTG / PED Software Organization
Intel Corp.  Folsom, CA
(916) 356-6193    

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm: atomic: fix legacy gamma set helper

[Daniel Vetter]

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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: atomic: fix legacy gamma set helper

[Lionel Landwerlin]

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: atomic: fix legacy gamma set helper

[Daniel Vetter]

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
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: atomic: fix legacy gamma set helper

[Lionel Landwerlin]

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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm: atomic: fix legacy gamma set helper

[Daniel Vetter]

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
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel