All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] Nightly color management fixes
@ 2016-07-15 10:59 Lionel Landwerlin
  2016-07-15 10:59 ` [PATCH] drm/i915: Set legacy properties when using legacy gamma set IOCTL Lionel Landwerlin
                   ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Lionel Landwerlin @ 2016-07-15 10:59 UTC (permalink / raw)
  To: intel-gfx

Nightly has gone through some atomic reverts, we just need a single
patch to fix the remaining color management issues.

Cheers,

Lionel

Bob Paauwe (1):
  drm/i915: Set legacy properties when using legacy gamma set IOCTL.

 drivers/gpu/drm/i915/intel_display.c | 39 +++++++++++++++++++++++++++++++++++-
 1 file changed, 38 insertions(+), 1 deletion(-)

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH] drm/i915: Set legacy properties when using legacy gamma set IOCTL.
  2016-07-15 10:59 [PATCH] Nightly color management fixes Lionel Landwerlin
@ 2016-07-15 10:59 ` Lionel Landwerlin
  2016-07-16  4:24   ` kbuild test robot
  2016-07-15 11:11 ` ✗ Ro.CI.BAT: failure for drm/i915: Set legacy properties when using legacy gamma set IOCTL. (rev3) Patchwork
  2016-07-15 13:59 ` [PATCH] drm/i915: Set legacy properties when using legacy gamma set IOCTL. (v2) Lionel Landwerlin
  2 siblings, 1 reply; 12+ messages in thread
From: Lionel Landwerlin @ 2016-07-15 10:59 UTC (permalink / raw)
  To: intel-gfx

From: Bob Paauwe <bob.j.paauwe@intel.com>

The i915 driver is now using atomic properties and atomic commit
to handle the legacy set gamma IOCTL. However, if the driver is
configured without atomic (nuclear_pageflip = false), it won't
update the legacy properties for degamma_lut, gamma_lut and ctm
leaving them out of sync with the atomic version of the properties.

Until the driver is full atomic, make sure we update the non-atomic
version of the properties.

v2: Update the comment with a FIXME.  (Daniel)

v3: Update arguments of the gamma_set vfunc (Lionel)

igt-testcase: kms_pipe_color / legacy-gamma-reset-pipeX
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: Bob Paauwe <bob.j.paauwe@intel.com>
Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 39 +++++++++++++++++++++++++++++++++++-
 1 file changed, 38 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 9337d3a..f18a346 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13924,8 +13924,45 @@ out:
 
 #undef for_each_intel_crtc_masked
 
+/*
+ * FIXME: Remove this once i915 is fully DRIVER_ATOMIC by calling
+ *        drm_atomic_helper_legacy_gamma_set() directly.
+ */
+static void intel_atomic_legacy_gamma_set(struct drm_crtc *crtc,
+					  u16 *red, u16 *green, u16 *blue,
+					  uint32_t size)
+{
+	struct drm_device *dev = crtc->dev;
+	struct drm_mode_config *config = &dev->mode_config;
+	struct drm_crtc_state *state;
+
+	drm_atomic_helper_legacy_gamma_set(crtc, red, green, blue, size);
+
+	/*
+	 * Make sure we update the legacy properties so this works when
+	 * atomic is not enabled.
+	 */
+
+	state = crtc->state;
+
+	drm_object_property_set_value(&crtc->base,
+				      config->degamma_lut_property,
+				      (state->degamma_lut) ?
+				      state->degamma_lut->base.id : 0);
+
+	drm_object_property_set_value(&crtc->base,
+				      config->ctm_property,
+				      (state->ctm) ?
+				      state->ctm->base.id : 0);
+
+	drm_object_property_set_value(&crtc->base,
+				      config->gamma_lut_property,
+				      (state->gamma_lut) ?
+				      state->gamma_lut->base.id : 0);
+}
+
 static const struct drm_crtc_funcs intel_crtc_funcs = {
-	.gamma_set = drm_atomic_helper_legacy_gamma_set,
+	.gamma_set = intel_atomic_legacy_gamma_set,
 	.set_config = drm_atomic_helper_set_config,
 	.set_property = drm_atomic_helper_crtc_set_property,
 	.destroy = intel_crtc_destroy,
-- 
2.8.1

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

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* ✗ Ro.CI.BAT: failure for drm/i915: Set legacy properties when using legacy gamma set IOCTL. (rev3)
  2016-07-15 10:59 [PATCH] Nightly color management fixes Lionel Landwerlin
  2016-07-15 10:59 ` [PATCH] drm/i915: Set legacy properties when using legacy gamma set IOCTL Lionel Landwerlin
@ 2016-07-15 11:11 ` Patchwork
  2016-07-15 13:59 ` [PATCH] drm/i915: Set legacy properties when using legacy gamma set IOCTL. (v2) Lionel Landwerlin
  2 siblings, 0 replies; 12+ messages in thread
From: Patchwork @ 2016-07-15 11:11 UTC (permalink / raw)
  To: Bob Paauwe; +Cc: intel-gfx

== Series Details ==

Series: drm/i915: Set legacy properties when using legacy gamma set IOCTL. (rev3)
URL   : https://patchwork.freedesktop.org/series/5466/
State : failure

== Summary ==

  LD      drivers/tty/vt/built-in.o
  LD      drivers/video/fbdev/core/built-in.o
  LD      drivers/usb/core/usbcore.o
  LD      drivers/usb/core/built-in.o
  LD      drivers/video/fbdev/built-in.o
  LD [M]  drivers/gpu/drm/drm.o
  LD      drivers/tty/serial/8250/built-in.o
  LD      drivers/video/built-in.o
  LD      drivers/tty/serial/built-in.o
  LD      drivers/tty/built-in.o
  LD      drivers/usb/storage/usb-storage.o
  LD      drivers/usb/host/xhci-hcd.o
  LD      drivers/usb/storage/built-in.o
  LD      drivers/usb/host/built-in.o
  LD      drivers/usb/built-in.o
drivers/gpu/drm/i915/intel_display.c:13965:15: error: initialization from incompatible pointer type [-Werror=incompatible-pointer-types]
  .gamma_set = intel_atomic_legacy_gamma_set,
               ^
drivers/gpu/drm/i915/intel_display.c:13965:15: note: (near initialization for ‘intel_crtc_funcs.gamma_set’)
cc1: some warnings being treated as errors
scripts/Makefile.build:289: recipe for target 'drivers/gpu/drm/i915/intel_display.o' failed
make[4]: *** [drivers/gpu/drm/i915/intel_display.o] Error 1
scripts/Makefile.build:440: recipe for target 'drivers/gpu/drm/i915' failed
make[3]: *** [drivers/gpu/drm/i915] Error 2
scripts/Makefile.build:440: recipe for target 'drivers/gpu/drm' failed
make[2]: *** [drivers/gpu/drm] Error 2
scripts/Makefile.build:440: recipe for target 'drivers/gpu' failed
make[1]: *** [drivers/gpu] Error 2
Makefile:987: recipe for target 'drivers' failed
make: *** [drivers] Error 2

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH] drm/i915: Set legacy properties when using legacy gamma set IOCTL. (v2)
  2016-07-15 10:59 [PATCH] Nightly color management fixes Lionel Landwerlin
  2016-07-15 10:59 ` [PATCH] drm/i915: Set legacy properties when using legacy gamma set IOCTL Lionel Landwerlin
  2016-07-15 11:11 ` ✗ Ro.CI.BAT: failure for drm/i915: Set legacy properties when using legacy gamma set IOCTL. (rev3) Patchwork
@ 2016-07-15 13:59 ` Lionel Landwerlin
  2016-07-15 16:13   ` Bob Paauwe
  2016-07-18  7:50   ` Maarten Lankhorst
  2 siblings, 2 replies; 12+ messages in thread
From: Lionel Landwerlin @ 2016-07-15 13:59 UTC (permalink / raw)
  To: intel-gfx

From: Bob Paauwe <bob.j.paauwe@intel.com>

The i915 driver is now using atomic properties and atomic commit
to handle the legacy set gamma IOCTL. However, if the driver is
configured without atomic (nuclear_pageflip = false), it won't
update the legacy properties for degamma_lut, gamma_lut and ctm
leaving them out of sync with the atomic version of the properties.

Until the driver is full atomic, make sure we update the non-atomic
version of the properties.

v2: Update the comment with a FIXME.  (Daniel)

v3: Update arguments of the gamma_set vfunc (Lionel)

v4: Fixed vfunc prototype (Lionel)

igt-testcase: kms_pipe_color / legacy-gamma-reset-pipeX
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: Bob Paauwe <bob.j.paauwe@intel.com>
Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 44 +++++++++++++++++++++++++++++++++++-
 1 file changed, 43 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 9337d3a..fb7d8fc5 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13924,8 +13924,50 @@ out:
 
 #undef for_each_intel_crtc_masked
 
+/*
+ * FIXME: Remove this once i915 is fully DRIVER_ATOMIC by calling
+ *        drm_atomic_helper_legacy_gamma_set() directly.
+ */
+static int intel_atomic_legacy_gamma_set(struct drm_crtc *crtc,
+					 u16 *red, u16 *green, u16 *blue,
+					 uint32_t size)
+{
+	struct drm_device *dev = crtc->dev;
+	struct drm_mode_config *config = &dev->mode_config;
+	struct drm_crtc_state *state;
+	int ret;
+
+	ret = drm_atomic_helper_legacy_gamma_set(crtc, red, green, blue, size);
+	if (ret)
+		return ret;
+
+	/*
+	 * Make sure we update the legacy properties so this works when
+	 * atomic is not enabled.
+	 */
+
+	state = crtc->state;
+
+	drm_object_property_set_value(&crtc->base,
+				      config->degamma_lut_property,
+				      (state->degamma_lut) ?
+				      state->degamma_lut->base.id : 0);
+
+	drm_object_property_set_value(&crtc->base,
+				      config->ctm_property,
+				      (state->ctm) ?
+				      state->ctm->base.id : 0);
+
+	drm_object_property_set_value(&crtc->base,
+				      config->gamma_lut_property,
+				      (state->gamma_lut) ?
+				      state->gamma_lut->base.id : 0);
+
+	return 0;
+}
+
 static const struct drm_crtc_funcs intel_crtc_funcs = {
-	.gamma_set = drm_atomic_helper_legacy_gamma_set,
+	.gamma_set = intel_atomic_legacy_gamma_set,
 	.set_config = drm_atomic_helper_set_config,
 	.set_property = drm_atomic_helper_crtc_set_property,
 	.destroy = intel_crtc_destroy,
-- 
2.8.1

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

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] drm/i915: Set legacy properties when using legacy gamma set IOCTL. (v2)
  2016-07-15 13:59 ` [PATCH] drm/i915: Set legacy properties when using legacy gamma set IOCTL. (v2) Lionel Landwerlin
@ 2016-07-15 16:13   ` Bob Paauwe
  2016-07-18 14:27     ` Maarten Lankhorst
  2016-07-18  7:50   ` Maarten Lankhorst
  1 sibling, 1 reply; 12+ messages in thread
From: Bob Paauwe @ 2016-07-15 16:13 UTC (permalink / raw)
  To: Lionel Landwerlin; +Cc: intel-gfx

On Fri, 15 Jul 2016 14:59:02 +0100
Lionel Landwerlin <lionel.g.landwerlin@intel.com> wrote:

> From: Bob Paauwe <bob.j.paauwe@intel.com>
> 
> The i915 driver is now using atomic properties and atomic commit
> to handle the legacy set gamma IOCTL. However, if the driver is
> configured without atomic (nuclear_pageflip = false), it won't
> update the legacy properties for degamma_lut, gamma_lut and ctm
> leaving them out of sync with the atomic version of the properties.
> 
> Until the driver is full atomic, make sure we update the non-atomic
> version of the properties.
> 
> v2: Update the comment with a FIXME.  (Daniel)
> 
> v3: Update arguments of the gamma_set vfunc (Lionel)
> 
> v4: Fixed vfunc prototype (Lionel)

Lionel,

The changes looks correct to me, thanks!  Is it appropriate for me to
add a reviewed-by for those changes?  

Bob

> 
> igt-testcase: kms_pipe_color / legacy-gamma-reset-pipeX
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Signed-off-by: Bob Paauwe <bob.j.paauwe@intel.com>
> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 44 +++++++++++++++++++++++++++++++++++-
>  1 file changed, 43 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 9337d3a..fb7d8fc5 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13924,8 +13924,50 @@ out:
>  
>  #undef for_each_intel_crtc_masked
>  
> +/*
> + * FIXME: Remove this once i915 is fully DRIVER_ATOMIC by calling
> + *        drm_atomic_helper_legacy_gamma_set() directly.
> + */
> +static int intel_atomic_legacy_gamma_set(struct drm_crtc *crtc,
> +					 u16 *red, u16 *green, u16 *blue,
> +					 uint32_t size)
> +{
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_mode_config *config = &dev->mode_config;
> +	struct drm_crtc_state *state;
> +	int ret;
> +
> +	ret = drm_atomic_helper_legacy_gamma_set(crtc, red, green, blue, size);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Make sure we update the legacy properties so this works when
> +	 * atomic is not enabled.
> +	 */
> +
> +	state = crtc->state;
> +
> +	drm_object_property_set_value(&crtc->base,
> +				      config->degamma_lut_property,
> +				      (state->degamma_lut) ?
> +				      state->degamma_lut->base.id : 0);
> +
> +	drm_object_property_set_value(&crtc->base,
> +				      config->ctm_property,
> +				      (state->ctm) ?
> +				      state->ctm->base.id : 0);
> +
> +	drm_object_property_set_value(&crtc->base,
> +				      config->gamma_lut_property,
> +				      (state->gamma_lut) ?
> +				      state->gamma_lut->base.id : 0);
> +
> +	return 0;
> +}
> +
>  static const struct drm_crtc_funcs intel_crtc_funcs = {
> -	.gamma_set = drm_atomic_helper_legacy_gamma_set,
> +	.gamma_set = intel_atomic_legacy_gamma_set,
>  	.set_config = drm_atomic_helper_set_config,
>  	.set_property = drm_atomic_helper_crtc_set_property,
>  	.destroy = intel_crtc_destroy,



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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] drm/i915: Set legacy properties when using legacy gamma set IOCTL.
  2016-07-15 10:59 ` [PATCH] drm/i915: Set legacy properties when using legacy gamma set IOCTL Lionel Landwerlin
@ 2016-07-16  4:24   ` kbuild test robot
  0 siblings, 0 replies; 12+ messages in thread
From: kbuild test robot @ 2016-07-16  4:24 UTC (permalink / raw)
  To: Lionel Landwerlin; +Cc: intel-gfx, kbuild-all

[-- Attachment #1: Type: text/plain, Size: 1662 bytes --]

Hi,

[auto build test ERROR on drm-intel/for-linux-next]
[also build test ERROR on v4.7-rc7 next-20160715]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Lionel-Landwerlin/drm-i915-Set-legacy-properties-when-using-legacy-gamma-set-IOCTL/20160715-203721
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
config: x86_64-rhel (attached as .config)
compiler: gcc-6 (Debian 6.1.1-1) 6.1.1 20160430
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

>> drivers/gpu/drm/i915/intel_display.c:13959:15: error: initialization from incompatible pointer type [-Werror=incompatible-pointer-types]
     .gamma_set = intel_atomic_legacy_gamma_set,
                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/gpu/drm/i915/intel_display.c:13959:15: note: (near initialization for 'intel_crtc_funcs.gamma_set')
   cc1: some warnings being treated as errors

vim +13959 drivers/gpu/drm/i915/intel_display.c

 13953					      config->gamma_lut_property,
 13954					      (state->gamma_lut) ?
 13955					      state->gamma_lut->base.id : 0);
 13956	}
 13957	
 13958	static const struct drm_crtc_funcs intel_crtc_funcs = {
 13959		.gamma_set = intel_atomic_legacy_gamma_set,
 13960		.set_config = drm_atomic_helper_set_config,
 13961		.set_property = drm_atomic_helper_crtc_set_property,
 13962		.destroy = intel_crtc_destroy,

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 37220 bytes --]

[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] drm/i915: Set legacy properties when using legacy gamma set IOCTL. (v2)
  2016-07-15 13:59 ` [PATCH] drm/i915: Set legacy properties when using legacy gamma set IOCTL. (v2) Lionel Landwerlin
  2016-07-15 16:13   ` Bob Paauwe
@ 2016-07-18  7:50   ` Maarten Lankhorst
  1 sibling, 0 replies; 12+ messages in thread
From: Maarten Lankhorst @ 2016-07-18  7:50 UTC (permalink / raw)
  To: Lionel Landwerlin, intel-gfx

Op 15-07-16 om 15:59 schreef Lionel Landwerlin:
> From: Bob Paauwe <bob.j.paauwe@intel.com>
>
> The i915 driver is now using atomic properties and atomic commit
> to handle the legacy set gamma IOCTL. However, if the driver is
> configured without atomic (nuclear_pageflip = false), it won't
> update the legacy properties for degamma_lut, gamma_lut and ctm
> leaving them out of sync with the atomic version of the properties.
>
> Until the driver is full atomic, make sure we update the non-atomic
> version of the properties.
>
> v2: Update the comment with a FIXME.  (Daniel)
>
> v3: Update arguments of the gamma_set vfunc (Lionel)
>
> v4: Fixed vfunc prototype (Lionel)
>
> igt-testcase: kms_pipe_color / legacy-gamma-reset-pipeX
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Signed-off-by: Bob Paauwe <bob.j.paauwe@intel.com>
> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>

Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] drm/i915: Set legacy properties when using legacy gamma set IOCTL. (v2)
  2016-07-15 16:13   ` Bob Paauwe
@ 2016-07-18 14:27     ` Maarten Lankhorst
  0 siblings, 0 replies; 12+ messages in thread
From: Maarten Lankhorst @ 2016-07-18 14:27 UTC (permalink / raw)
  To: Bob Paauwe, Lionel Landwerlin; +Cc: intel-gfx

Op 15-07-16 om 18:13 schreef Bob Paauwe:
> On Fri, 15 Jul 2016 14:59:02 +0100
> Lionel Landwerlin <lionel.g.landwerlin@intel.com> wrote:
>
>> From: Bob Paauwe <bob.j.paauwe@intel.com>
>>
>> The i915 driver is now using atomic properties and atomic commit
>> to handle the legacy set gamma IOCTL. However, if the driver is
>> configured without atomic (nuclear_pageflip = false), it won't
>> update the legacy properties for degamma_lut, gamma_lut and ctm
>> leaving them out of sync with the atomic version of the properties.
>>
>> Until the driver is full atomic, make sure we update the non-atomic
>> version of the properties.
>>
>> v2: Update the comment with a FIXME.  (Daniel)
>>
>> v3: Update arguments of the gamma_set vfunc (Lionel)
>>
>> v4: Fixed vfunc prototype (Lionel)
> Lionel,
>
> The changes looks correct to me, thanks!  Is it appropriate for me to
> add a reviewed-by for those changes?  
I think the signed-off-by is strong enough, patch applied, thanks.

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] drm/i915: Set legacy properties when using legacy gamma set IOCTL.
  2016-04-08 17:21 ` Lionel Landwerlin
@ 2016-04-08 17:58   ` Bob Paauwe
  0 siblings, 0 replies; 12+ messages in thread
From: Bob Paauwe @ 2016-04-08 17:58 UTC (permalink / raw)
  To: Lionel Landwerlin; +Cc: intel-gfx

On Fri, 8 Apr 2016 18:21:51 +0100
Lionel Landwerlin <llandwerlin@gmail.com> wrote:

> Hi Paul,
> 
> Unfortunate that we've been writing the same patch at the same time :(
> I think this we've got pretty much the same fix, but it probably needs 
> to be done at the DRM level, because this can impact other drivers too.
> 
> Cheers,
> 
> -
> Lionel

In this case, it's happening because the i915 driver is using the
atomic set property paths to implement the set gamma IOCTL. This is
fine if the i915 driver is fully using atomic (nuclear_pageflip = true)
because queries for the properties will go through the atomic path.
But if i915 is not using atomic, then queries for the properties will
not go through the atomic path and will get the incorrect or out of
sync legacy property values.

So this would only effect other drivers if they implemented the set
gamma IOCTL the same way and have two code paths for querying the
properties.

This patch is really a temporary fix until we move to
nuclear_pageflip=true as the default/only mode for the i915 driver.

Bob
> 
> On 08/04/16 17:26, Bob Paauwe wrote:
> > The i915 driver is now using atomic properties and atomic commit
> > to handle the legacy set gamma IOCTL. However, if the driver is
> > configured without atomic (nuclear_pageflip = false), it won't
> > update the legacy properties for degamma_lut, gamma_lut and ctm
> > leaving them out of sync with the atomic version of the properties.
> >
> > Until the driver is full atomic, make sure we update the non-atomic
> > version of the properties.
> >
> > igt-testcase: kms_pipe_color / legacy-gamma-reset-pipeX
> > Signed-off-by: Bob Paauwe <bob.j.paauwe@intel.com>
> > ---
> >   drivers/gpu/drm/i915/intel_display.c | 38 +++++++++++++++++++++++++++++++++++-
> >   1 file changed, 37 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 5155efb6..ff09a18 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -13885,8 +13885,44 @@ out:
> >   
> >   #undef for_each_intel_crtc_masked
> >   
> > +/*
> > + * TODO: Remove this once we have full atomic implmented.
> > + */
> > +static void intel_atomic_legacy_gamma_set(struct drm_crtc *crtc,
> > +					  u16 *red, u16 *green, u16 *blue,
> > +					  uint32_t start, uint32_t size)
> > +{
> > +	struct drm_device *dev = crtc->dev;
> > +	struct drm_mode_config *config = &dev->mode_config;
> > +	struct drm_crtc_state *state;
> > +
> > +	drm_atomic_helper_legacy_gamma_set(crtc, red, green, blue, start, size);
> > +
> > +	/*
> > +	 * Make sure we update the legacy properties so this works when
> > +	 * atomic is not enabled.
> > +	 */
> > +
> > +	state = crtc->state;
> > +
> > +	drm_object_property_set_value(&crtc->base,
> > +				      config->degamma_lut_property,
> > +				      (state->degamma_lut) ?
> > +				      state->degamma_lut->base.id : 0);
> > +
> > +	drm_object_property_set_value(&crtc->base,
> > +				      config->ctm_property,
> > +				      (state->ctm) ?
> > +				      state->ctm->base.id : 0);
> > +
> > +	drm_object_property_set_value(&crtc->base,
> > +				      config->gamma_lut_property,
> > +				      (state->gamma_lut) ?
> > +				      state->gamma_lut->base.id : 0);
> > +}
> > +
> >   static const struct drm_crtc_funcs intel_crtc_funcs = {
> > -	.gamma_set = drm_atomic_helper_legacy_gamma_set,
> > +	.gamma_set = intel_atomic_legacy_gamma_set,
> >   	.set_config = drm_atomic_helper_set_config,
> >   	.set_property = drm_atomic_helper_crtc_set_property,
> >   	.destroy = intel_crtc_destroy,  
> 



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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] drm/i915: Set legacy properties when using legacy gamma set IOCTL.
  2016-04-08 16:26 [PATCH] drm/i915: Set legacy properties when using legacy gamma set IOCTL Bob Paauwe
  2016-04-08 17:21 ` Lionel Landwerlin
@ 2016-04-08 17:21 ` Lionel Landwerlin
  1 sibling, 0 replies; 12+ messages in thread
From: Lionel Landwerlin @ 2016-04-08 17:21 UTC (permalink / raw)
  To: intel-gfx, bob.j.paauwe

Hi Paul,

Unfortunate that we've been writing the same patch at the same time :(
I think this we've got pretty much the same fix, but it probably needs 
to be done at the DRM level, because this can impact other drivers too.

Cheers,

-
Lionel

On 08/04/16 17:26, Bob Paauwe wrote:
> The i915 driver is now using atomic properties and atomic commit
> to handle the legacy set gamma IOCTL. However, if the driver is
> configured without atomic (nuclear_pageflip = false), it won't
> update the legacy properties for degamma_lut, gamma_lut and ctm
> leaving them out of sync with the atomic version of the properties.
>
> Until the driver is full atomic, make sure we update the non-atomic
> version of the properties.
>
> igt-testcase: kms_pipe_color / legacy-gamma-reset-pipeX
> Signed-off-by: Bob Paauwe <bob.j.paauwe@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_display.c | 38 +++++++++++++++++++++++++++++++++++-
>   1 file changed, 37 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 5155efb6..ff09a18 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13885,8 +13885,44 @@ out:
>   
>   #undef for_each_intel_crtc_masked
>   
> +/*
> + * TODO: Remove this once we have full atomic implmented.
> + */
> +static void intel_atomic_legacy_gamma_set(struct drm_crtc *crtc,
> +					  u16 *red, u16 *green, u16 *blue,
> +					  uint32_t start, uint32_t size)
> +{
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_mode_config *config = &dev->mode_config;
> +	struct drm_crtc_state *state;
> +
> +	drm_atomic_helper_legacy_gamma_set(crtc, red, green, blue, start, size);
> +
> +	/*
> +	 * Make sure we update the legacy properties so this works when
> +	 * atomic is not enabled.
> +	 */
> +
> +	state = crtc->state;
> +
> +	drm_object_property_set_value(&crtc->base,
> +				      config->degamma_lut_property,
> +				      (state->degamma_lut) ?
> +				      state->degamma_lut->base.id : 0);
> +
> +	drm_object_property_set_value(&crtc->base,
> +				      config->ctm_property,
> +				      (state->ctm) ?
> +				      state->ctm->base.id : 0);
> +
> +	drm_object_property_set_value(&crtc->base,
> +				      config->gamma_lut_property,
> +				      (state->gamma_lut) ?
> +				      state->gamma_lut->base.id : 0);
> +}
> +
>   static const struct drm_crtc_funcs intel_crtc_funcs = {
> -	.gamma_set = drm_atomic_helper_legacy_gamma_set,
> +	.gamma_set = intel_atomic_legacy_gamma_set,
>   	.set_config = drm_atomic_helper_set_config,
>   	.set_property = drm_atomic_helper_crtc_set_property,
>   	.destroy = intel_crtc_destroy,

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] drm/i915: Set legacy properties when using legacy gamma set IOCTL.
  2016-04-08 16:26 [PATCH] drm/i915: Set legacy properties when using legacy gamma set IOCTL Bob Paauwe
@ 2016-04-08 17:21 ` Lionel Landwerlin
  2016-04-08 17:58   ` Bob Paauwe
  2016-04-08 17:21 ` Lionel Landwerlin
  1 sibling, 1 reply; 12+ messages in thread
From: Lionel Landwerlin @ 2016-04-08 17:21 UTC (permalink / raw)
  To: intel-gfx, bob.j.paauwe

Hi Paul,

Unfortunate that we've been writing the same patch at the same time :(
I think this we've got pretty much the same fix, but it probably needs 
to be done at the DRM level, because this can impact other drivers too.

Cheers,

-
Lionel

On 08/04/16 17:26, Bob Paauwe wrote:
> The i915 driver is now using atomic properties and atomic commit
> to handle the legacy set gamma IOCTL. However, if the driver is
> configured without atomic (nuclear_pageflip = false), it won't
> update the legacy properties for degamma_lut, gamma_lut and ctm
> leaving them out of sync with the atomic version of the properties.
>
> Until the driver is full atomic, make sure we update the non-atomic
> version of the properties.
>
> igt-testcase: kms_pipe_color / legacy-gamma-reset-pipeX
> Signed-off-by: Bob Paauwe <bob.j.paauwe@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_display.c | 38 +++++++++++++++++++++++++++++++++++-
>   1 file changed, 37 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 5155efb6..ff09a18 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13885,8 +13885,44 @@ out:
>   
>   #undef for_each_intel_crtc_masked
>   
> +/*
> + * TODO: Remove this once we have full atomic implmented.
> + */
> +static void intel_atomic_legacy_gamma_set(struct drm_crtc *crtc,
> +					  u16 *red, u16 *green, u16 *blue,
> +					  uint32_t start, uint32_t size)
> +{
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_mode_config *config = &dev->mode_config;
> +	struct drm_crtc_state *state;
> +
> +	drm_atomic_helper_legacy_gamma_set(crtc, red, green, blue, start, size);
> +
> +	/*
> +	 * Make sure we update the legacy properties so this works when
> +	 * atomic is not enabled.
> +	 */
> +
> +	state = crtc->state;
> +
> +	drm_object_property_set_value(&crtc->base,
> +				      config->degamma_lut_property,
> +				      (state->degamma_lut) ?
> +				      state->degamma_lut->base.id : 0);
> +
> +	drm_object_property_set_value(&crtc->base,
> +				      config->ctm_property,
> +				      (state->ctm) ?
> +				      state->ctm->base.id : 0);
> +
> +	drm_object_property_set_value(&crtc->base,
> +				      config->gamma_lut_property,
> +				      (state->gamma_lut) ?
> +				      state->gamma_lut->base.id : 0);
> +}
> +
>   static const struct drm_crtc_funcs intel_crtc_funcs = {
> -	.gamma_set = drm_atomic_helper_legacy_gamma_set,
> +	.gamma_set = intel_atomic_legacy_gamma_set,
>   	.set_config = drm_atomic_helper_set_config,
>   	.set_property = drm_atomic_helper_crtc_set_property,
>   	.destroy = intel_crtc_destroy,

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH] drm/i915: Set legacy properties when using legacy gamma set IOCTL.
@ 2016-04-08 16:26 Bob Paauwe
  2016-04-08 17:21 ` Lionel Landwerlin
  2016-04-08 17:21 ` Lionel Landwerlin
  0 siblings, 2 replies; 12+ messages in thread
From: Bob Paauwe @ 2016-04-08 16:26 UTC (permalink / raw)
  To: intel-gfx

The i915 driver is now using atomic properties and atomic commit
to handle the legacy set gamma IOCTL. However, if the driver is
configured without atomic (nuclear_pageflip = false), it won't
update the legacy properties for degamma_lut, gamma_lut and ctm
leaving them out of sync with the atomic version of the properties.

Until the driver is full atomic, make sure we update the non-atomic
version of the properties.

igt-testcase: kms_pipe_color / legacy-gamma-reset-pipeX
Signed-off-by: Bob Paauwe <bob.j.paauwe@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 38 +++++++++++++++++++++++++++++++++++-
 1 file changed, 37 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 5155efb6..ff09a18 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13885,8 +13885,44 @@ out:
 
 #undef for_each_intel_crtc_masked
 
+/*
+ * TODO: Remove this once we have full atomic implmented.
+ */
+static void intel_atomic_legacy_gamma_set(struct drm_crtc *crtc,
+					  u16 *red, u16 *green, u16 *blue,
+					  uint32_t start, uint32_t size)
+{
+	struct drm_device *dev = crtc->dev;
+	struct drm_mode_config *config = &dev->mode_config;
+	struct drm_crtc_state *state;
+
+	drm_atomic_helper_legacy_gamma_set(crtc, red, green, blue, start, size);
+
+	/*
+	 * Make sure we update the legacy properties so this works when
+	 * atomic is not enabled.
+	 */
+
+	state = crtc->state;
+
+	drm_object_property_set_value(&crtc->base,
+				      config->degamma_lut_property,
+				      (state->degamma_lut) ?
+				      state->degamma_lut->base.id : 0);
+
+	drm_object_property_set_value(&crtc->base,
+				      config->ctm_property,
+				      (state->ctm) ?
+				      state->ctm->base.id : 0);
+
+	drm_object_property_set_value(&crtc->base,
+				      config->gamma_lut_property,
+				      (state->gamma_lut) ?
+				      state->gamma_lut->base.id : 0);
+}
+
 static const struct drm_crtc_funcs intel_crtc_funcs = {
-	.gamma_set = drm_atomic_helper_legacy_gamma_set,
+	.gamma_set = intel_atomic_legacy_gamma_set,
 	.set_config = drm_atomic_helper_set_config,
 	.set_property = drm_atomic_helper_crtc_set_property,
 	.destroy = intel_crtc_destroy,
-- 
2.5.5

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

^ permalink raw reply related	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2016-07-18 14:27 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-07-15 10:59 [PATCH] Nightly color management fixes Lionel Landwerlin
2016-07-15 10:59 ` [PATCH] drm/i915: Set legacy properties when using legacy gamma set IOCTL Lionel Landwerlin
2016-07-16  4:24   ` kbuild test robot
2016-07-15 11:11 ` ✗ Ro.CI.BAT: failure for drm/i915: Set legacy properties when using legacy gamma set IOCTL. (rev3) Patchwork
2016-07-15 13:59 ` [PATCH] drm/i915: Set legacy properties when using legacy gamma set IOCTL. (v2) Lionel Landwerlin
2016-07-15 16:13   ` Bob Paauwe
2016-07-18 14:27     ` Maarten Lankhorst
2016-07-18  7:50   ` Maarten Lankhorst
  -- strict thread matches above, loose matches on Subject: below --
2016-04-08 16:26 [PATCH] drm/i915: Set legacy properties when using legacy gamma set IOCTL Bob Paauwe
2016-04-08 17:21 ` Lionel Landwerlin
2016-04-08 17:58   ` Bob Paauwe
2016-04-08 17:21 ` Lionel Landwerlin

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.