drm/i915: Set legacy properties when using legacy gamma set IOCTL.

Submitted by Bob Paauwe on April 8, 2016, 4:26 p.m.

Details

Message ID 1460132774-7976-1-git-send-email-bob.j.paauwe@intel.com
State New
Headers show
Series "drm/i915: Set legacy properties when using legacy gamma set IOCTL. (v2)" ( rev: 1 ) in Intel GFX

Not browsing as part of any series.

Commit Message

Bob Paauwe April 8, 2016, 4:26 p.m.
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(-)

Patch hide | download patch | download mbox

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,

Comments

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