Re: [Intel-gfx] [PATCH 13/13] drm/i915: Disable pipe gamma when C8 pixel format is used
On Thu, Jan 17, 2019 at 11:13:58AM -0800, Matt Roper wrote:
> On Fri, Jan 11, 2019 at 07:08:23PM +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä
> >
> > Planes scanning out C8 will want to use the legacy lut as
> > their palette. That means the LUT content are unikely to
> > be useful for gamma correction on other planes. Thus we
> > should disable pipe gamma for all the other planes. And
> > we should reject any non legacy LUT configurations when
> > C8 planes are present.
> >
> > Fixes the appearance of the hw cursor when running
> > X -depth 8.
> >
> > Note that CHV with it's independent CGM degamma/gamma LUTs
> > could probably use the CGM for gamma correction even when
> > the legacy LUT is used for C8. But that would require a
> > new uapi for configuring the legacy LUT and CGM LUTs at
> > the same time. Totally not worth it.
> >
> > Signed-off-by: Ville Syrjälä
> > ---
> > drivers/gpu/drm/i915/intel_atomic_plane.c | 5 +
> > drivers/gpu/drm/i915/intel_color.c| 8 +++-
> > drivers/gpu/drm/i915/intel_drv.h | 1 +
> > 3 files changed, 13 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c
> > b/drivers/gpu/drm/i915/intel_atomic_plane.c
> > index 50be2c5dd76e..f311763867c4 100644
> > --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> > +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> > @@ -119,6 +119,7 @@ int intel_plane_atomic_check_with_state(const struct
> > intel_crtc_state *old_crtc_
> >
> > new_crtc_state->active_planes &= ~BIT(plane->id);
> > new_crtc_state->nv12_planes &= ~BIT(plane->id);
> > + new_crtc_state->c8_planes &= ~BIT(plane->id);
> > new_plane_state->base.visible = false;
> >
> > if (!new_plane_state->base.crtc && !old_plane_state->base.crtc)
> > @@ -136,6 +137,10 @@ int intel_plane_atomic_check_with_state(const struct
> > intel_crtc_state *old_crtc_
> > new_plane_state->base.fb->format->format == DRM_FORMAT_NV12)
> > new_crtc_state->nv12_planes |= BIT(plane->id);
> >
> > + if (new_plane_state->base.visible &&
> > + new_plane_state->base.fb->format->format == DRM_FORMAT_C8)
> > + new_crtc_state->c8_planes |= BIT(plane->id);
> > +
> > if (new_plane_state->base.visible || old_plane_state->base.visible)
> > new_crtc_state->update_planes |= BIT(plane->id);
> >
> > diff --git a/drivers/gpu/drm/i915/intel_color.c
> > b/drivers/gpu/drm/i915/intel_color.c
> > index 789b04bb51d2..c8d12653d77f 100644
> > --- a/drivers/gpu/drm/i915/intel_color.c
> > +++ b/drivers/gpu/drm/i915/intel_color.c
> > @@ -691,7 +691,13 @@ int intel_color_check(struct intel_crtc_state
> > *crtc_state)
> > degamma_length = INTEL_INFO(dev_priv)->color.degamma_lut_size;
> > gamma_length = INTEL_INFO(dev_priv)->color.gamma_lut_size;
> >
> > - crtc_state->gamma_enable = gamma_lut || degamma_lut;
> > + /* C8 needs the legacy LUT all to itself */
> > + if (crtc_state->c8_planes &&
> > + !crtc_state_is_legacy_gamma(crtc_state))
> > + return -EINVAL;
> > +
> > + crtc_state->gamma_enable = (gamma_lut || degamma_lut) &&
> > + !crtc_state->c8_planes;
>
> It's not really clear to me from the bspec...the legacy gamma will still
> get used as the c8 palette even when the pipe gamma enable bit is off
> for the plane?
Yes. At least it did on all the platforms I tried, which IIRC
were quite a few.
> If so,
>
> Reviewed-by: Matt Roper
>
> >
> > if (INTEL_GEN(dev_priv) >= 9 ||
> > IS_BROADWELL(dev_priv) || IS_HASWELL(dev_priv))
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index a4496f799af3..4d9ea05a6825 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -930,6 +930,7 @@ struct intel_crtc_state {
> > /* bitmask of visible planes (enum plane_id) */
> > u8 active_planes;
> > u8 nv12_planes;
> > + u8 c8_planes;
> >
> > /* bitmask of planes that will be updated during the commit */
> > u8 update_planes;
> > --
> > 2.19.2
> >
>
> --
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> (916) 356-2795
--
Ville Syrjälä
Intel
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 13/13] drm/i915: Disable pipe gamma when C8 pixel format is used
On Fri, Jan 11, 2019 at 07:08:23PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä
>
> Planes scanning out C8 will want to use the legacy lut as
> their palette. That means the LUT content are unikely to
> be useful for gamma correction on other planes. Thus we
> should disable pipe gamma for all the other planes. And
> we should reject any non legacy LUT configurations when
> C8 planes are present.
>
> Fixes the appearance of the hw cursor when running
> X -depth 8.
>
> Note that CHV with it's independent CGM degamma/gamma LUTs
> could probably use the CGM for gamma correction even when
> the legacy LUT is used for C8. But that would require a
> new uapi for configuring the legacy LUT and CGM LUTs at
> the same time. Totally not worth it.
>
> Signed-off-by: Ville Syrjälä
> ---
> drivers/gpu/drm/i915/intel_atomic_plane.c | 5 +
> drivers/gpu/drm/i915/intel_color.c| 8 +++-
> drivers/gpu/drm/i915/intel_drv.h | 1 +
> 3 files changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c
> b/drivers/gpu/drm/i915/intel_atomic_plane.c
> index 50be2c5dd76e..f311763867c4 100644
> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> @@ -119,6 +119,7 @@ int intel_plane_atomic_check_with_state(const struct
> intel_crtc_state *old_crtc_
>
> new_crtc_state->active_planes &= ~BIT(plane->id);
> new_crtc_state->nv12_planes &= ~BIT(plane->id);
> + new_crtc_state->c8_planes &= ~BIT(plane->id);
> new_plane_state->base.visible = false;
>
> if (!new_plane_state->base.crtc && !old_plane_state->base.crtc)
> @@ -136,6 +137,10 @@ int intel_plane_atomic_check_with_state(const struct
> intel_crtc_state *old_crtc_
> new_plane_state->base.fb->format->format == DRM_FORMAT_NV12)
> new_crtc_state->nv12_planes |= BIT(plane->id);
>
> + if (new_plane_state->base.visible &&
> + new_plane_state->base.fb->format->format == DRM_FORMAT_C8)
> + new_crtc_state->c8_planes |= BIT(plane->id);
> +
> if (new_plane_state->base.visible || old_plane_state->base.visible)
> new_crtc_state->update_planes |= BIT(plane->id);
>
> diff --git a/drivers/gpu/drm/i915/intel_color.c
> b/drivers/gpu/drm/i915/intel_color.c
> index 789b04bb51d2..c8d12653d77f 100644
> --- a/drivers/gpu/drm/i915/intel_color.c
> +++ b/drivers/gpu/drm/i915/intel_color.c
> @@ -691,7 +691,13 @@ int intel_color_check(struct intel_crtc_state
> *crtc_state)
> degamma_length = INTEL_INFO(dev_priv)->color.degamma_lut_size;
> gamma_length = INTEL_INFO(dev_priv)->color.gamma_lut_size;
>
> - crtc_state->gamma_enable = gamma_lut || degamma_lut;
> + /* C8 needs the legacy LUT all to itself */
> + if (crtc_state->c8_planes &&
> + !crtc_state_is_legacy_gamma(crtc_state))
> + return -EINVAL;
> +
> + crtc_state->gamma_enable = (gamma_lut || degamma_lut) &&
> + !crtc_state->c8_planes;
It's not really clear to me from the bspec...the legacy gamma will still
get used as the c8 palette even when the pipe gamma enable bit is off
for the plane? If so,
Reviewed-by: Matt Roper
>
> if (INTEL_GEN(dev_priv) >= 9 ||
> IS_BROADWELL(dev_priv) || IS_HASWELL(dev_priv))
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index a4496f799af3..4d9ea05a6825 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -930,6 +930,7 @@ struct intel_crtc_state {
> /* bitmask of visible planes (enum plane_id) */
> u8 active_planes;
> u8 nv12_planes;
> + u8 c8_planes;
>
> /* bitmask of planes that will be updated during the commit */
> u8 update_planes;
> --
> 2.19.2
>
--
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 13/13] drm/i915: Disable pipe gamma when C8 pixel format is used
>-Original Message-
>From: Ville Syrjala [mailto:ville.syrj...@linux.intel.com]
>Sent: Friday, January 11, 2019 10:38 PM
>To: intel-gfx@lists.freedesktop.org
>Cc: Shankar, Uma ; Roper, Matthew D
>
>Subject: [PATCH 13/13] drm/i915: Disable pipe gamma when C8 pixel format is
>used
>
>From: Ville Syrjälä
>
>Planes scanning out C8 will want to use the legacy lut as their palette. That
>means
>the LUT content are unikely to be useful for gamma correction on other planes.
Typo in unlikely.
With this fixed.
Reviewed-by: Uma Shankar
>Thus we should disable pipe gamma for all the other planes. And we should
>reject
>any non legacy LUT configurations when
>C8 planes are present.
>
>Fixes the appearance of the hw cursor when running X -depth 8.
>
>Note that CHV with it's independent CGM degamma/gamma LUTs could probably
>use the CGM for gamma correction even when the legacy LUT is used for C8. But
>that would require a new uapi for configuring the legacy LUT and CGM LUTs at
>the same time. Totally not worth it.
>
>Signed-off-by: Ville Syrjälä
>---
> drivers/gpu/drm/i915/intel_atomic_plane.c | 5 +
> drivers/gpu/drm/i915/intel_color.c| 8 +++-
> drivers/gpu/drm/i915/intel_drv.h | 1 +
> 3 files changed, 13 insertions(+), 1 deletion(-)
>
>diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c
>b/drivers/gpu/drm/i915/intel_atomic_plane.c
>index 50be2c5dd76e..f311763867c4 100644
>--- a/drivers/gpu/drm/i915/intel_atomic_plane.c
>+++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
>@@ -119,6 +119,7 @@ int intel_plane_atomic_check_with_state(const struct
>intel_crtc_state *old_crtc_
>
> new_crtc_state->active_planes &= ~BIT(plane->id);
> new_crtc_state->nv12_planes &= ~BIT(plane->id);
>+ new_crtc_state->c8_planes &= ~BIT(plane->id);
> new_plane_state->base.visible = false;
>
> if (!new_plane_state->base.crtc && !old_plane_state->base.crtc) @@ -
>136,6 +137,10 @@ int intel_plane_atomic_check_with_state(const struct
>intel_crtc_state *old_crtc_
> new_plane_state->base.fb->format->format == DRM_FORMAT_NV12)
> new_crtc_state->nv12_planes |= BIT(plane->id);
>
>+ if (new_plane_state->base.visible &&
>+ new_plane_state->base.fb->format->format == DRM_FORMAT_C8)
>+ new_crtc_state->c8_planes |= BIT(plane->id);
>+
> if (new_plane_state->base.visible || old_plane_state->base.visible)
> new_crtc_state->update_planes |= BIT(plane->id);
>
>diff --git a/drivers/gpu/drm/i915/intel_color.c
>b/drivers/gpu/drm/i915/intel_color.c
>index 789b04bb51d2..c8d12653d77f 100644
>--- a/drivers/gpu/drm/i915/intel_color.c
>+++ b/drivers/gpu/drm/i915/intel_color.c
>@@ -691,7 +691,13 @@ int intel_color_check(struct intel_crtc_state
>*crtc_state)
> degamma_length = INTEL_INFO(dev_priv)->color.degamma_lut_size;
> gamma_length = INTEL_INFO(dev_priv)->color.gamma_lut_size;
>
>- crtc_state->gamma_enable = gamma_lut || degamma_lut;
>+ /* C8 needs the legacy LUT all to itself */
>+ if (crtc_state->c8_planes &&
>+ !crtc_state_is_legacy_gamma(crtc_state))
>+ return -EINVAL;
>+
>+ crtc_state->gamma_enable = (gamma_lut || degamma_lut) &&
>+ !crtc_state->c8_planes;
>
> if (INTEL_GEN(dev_priv) >= 9 ||
> IS_BROADWELL(dev_priv) || IS_HASWELL(dev_priv)) diff --git
>a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>index a4496f799af3..4d9ea05a6825 100644
>--- a/drivers/gpu/drm/i915/intel_drv.h
>+++ b/drivers/gpu/drm/i915/intel_drv.h
>@@ -930,6 +930,7 @@ struct intel_crtc_state {
> /* bitmask of visible planes (enum plane_id) */
> u8 active_planes;
> u8 nv12_planes;
>+ u8 c8_planes;
>
> /* bitmask of planes that will be updated during the commit */
> u8 update_planes;
>--
>2.19.2
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 13/13] drm/i915: Disable pipe gamma when C8 pixel format is used
From: Ville Syrjälä
Planes scanning out C8 will want to use the legacy lut as
their palette. That means the LUT content are unikely to
be useful for gamma correction on other planes. Thus we
should disable pipe gamma for all the other planes. And
we should reject any non legacy LUT configurations when
C8 planes are present.
Fixes the appearance of the hw cursor when running
X -depth 8.
Note that CHV with it's independent CGM degamma/gamma LUTs
could probably use the CGM for gamma correction even when
the legacy LUT is used for C8. But that would require a
new uapi for configuring the legacy LUT and CGM LUTs at
the same time. Totally not worth it.
Signed-off-by: Ville Syrjälä
---
drivers/gpu/drm/i915/intel_atomic_plane.c | 5 +
drivers/gpu/drm/i915/intel_color.c| 8 +++-
drivers/gpu/drm/i915/intel_drv.h | 1 +
3 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c
b/drivers/gpu/drm/i915/intel_atomic_plane.c
index 50be2c5dd76e..f311763867c4 100644
--- a/drivers/gpu/drm/i915/intel_atomic_plane.c
+++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
@@ -119,6 +119,7 @@ int intel_plane_atomic_check_with_state(const struct
intel_crtc_state *old_crtc_
new_crtc_state->active_planes &= ~BIT(plane->id);
new_crtc_state->nv12_planes &= ~BIT(plane->id);
+ new_crtc_state->c8_planes &= ~BIT(plane->id);
new_plane_state->base.visible = false;
if (!new_plane_state->base.crtc && !old_plane_state->base.crtc)
@@ -136,6 +137,10 @@ int intel_plane_atomic_check_with_state(const struct
intel_crtc_state *old_crtc_
new_plane_state->base.fb->format->format == DRM_FORMAT_NV12)
new_crtc_state->nv12_planes |= BIT(plane->id);
+ if (new_plane_state->base.visible &&
+ new_plane_state->base.fb->format->format == DRM_FORMAT_C8)
+ new_crtc_state->c8_planes |= BIT(plane->id);
+
if (new_plane_state->base.visible || old_plane_state->base.visible)
new_crtc_state->update_planes |= BIT(plane->id);
diff --git a/drivers/gpu/drm/i915/intel_color.c
b/drivers/gpu/drm/i915/intel_color.c
index 789b04bb51d2..c8d12653d77f 100644
--- a/drivers/gpu/drm/i915/intel_color.c
+++ b/drivers/gpu/drm/i915/intel_color.c
@@ -691,7 +691,13 @@ int intel_color_check(struct intel_crtc_state *crtc_state)
degamma_length = INTEL_INFO(dev_priv)->color.degamma_lut_size;
gamma_length = INTEL_INFO(dev_priv)->color.gamma_lut_size;
- crtc_state->gamma_enable = gamma_lut || degamma_lut;
+ /* C8 needs the legacy LUT all to itself */
+ if (crtc_state->c8_planes &&
+ !crtc_state_is_legacy_gamma(crtc_state))
+ return -EINVAL;
+
+ crtc_state->gamma_enable = (gamma_lut || degamma_lut) &&
+ !crtc_state->c8_planes;
if (INTEL_GEN(dev_priv) >= 9 ||
IS_BROADWELL(dev_priv) || IS_HASWELL(dev_priv))
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index a4496f799af3..4d9ea05a6825 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -930,6 +930,7 @@ struct intel_crtc_state {
/* bitmask of visible planes (enum plane_id) */
u8 active_planes;
u8 nv12_planes;
+ u8 c8_planes;
/* bitmask of planes that will be updated during the commit */
u8 update_planes;
--
2.19.2
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
