Re: [Intel-gfx] [PATCH 10/11] drm/i915: Share {csc, gamma}_enable calculation for ilk/snb vs. ivb+

2022-11-03 Thread Nautiyal, Ankit K 

LGTM.

Reviewed-by: Ankit Nautiyal 

On 10/26/2022 5:09 PM, Ville Syrjala wrote:

From: Ville Syrjälä 

ilk/snb vs. ivb+ hardware is mostly identical except for the addition
of the split gamma mode on ivb. Thus we can share the csc_enable
and gamma_enable calculation for both variants. Pull that stuff
into a few helpers.

Note that this also fills in the missing ctm/degamma stuff into
ilk_color_check() pretty much, so for good measure let's also
add a few extra checks relating to that, although we still don't
expose ctm/degamma to userspace. But now it'll be trivial to do
so if we wish.

Signed-off-by: Ville Syrjälä 
---
  drivers/gpu/drm/i915/display/intel_color.c | 49 ++
  1 file changed, 32 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_color.c 
b/drivers/gpu/drm/i915/display/intel_color.c
index 926784f266f2..33871bfacee7 100644
--- a/drivers/gpu/drm/i915/display/intel_color.c
+++ b/drivers/gpu/drm/i915/display/intel_color.c
@@ -1442,6 +1442,20 @@ static int chv_color_check(struct intel_crtc_state 
*crtc_state)
return 0;
  }
  
+static bool ilk_gamma_enable(const struct intel_crtc_state *crtc_state)

+{
+   return (crtc_state->hw.gamma_lut ||
+   crtc_state->hw.degamma_lut) &&
+   !crtc_state->c8_planes;
+}
+
+static bool ilk_csc_enable(const struct intel_crtc_state *crtc_state)
+{
+   return crtc_state->output_format != INTEL_OUTPUT_FORMAT_RGB ||
+   ilk_csc_limited_range(crtc_state) ||
+   crtc_state->hw.ctm;
+}
+
  static u32 ilk_gamma_mode(const struct intel_crtc_state *crtc_state)
  {
if (!crtc_state->gamma_enable ||
@@ -1487,22 +1501,29 @@ static void ilk_assign_luts(struct intel_crtc_state 
*crtc_state)
  
  static int ilk_color_check(struct intel_crtc_state *crtc_state)

  {
+   struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc->dev);
int ret;
  
  	ret = check_luts(crtc_state);

if (ret)
return ret;
  
-	crtc_state->gamma_enable =

-   crtc_state->hw.gamma_lut &&
-   !crtc_state->c8_planes;
+   if (crtc_state->hw.degamma_lut && crtc_state->hw.gamma_lut) {
+   drm_dbg_kms(>drm,
+   "Degamma and gamma together are not possible\n");
+   return -EINVAL;
+   }
  
-	/*

-* We don't expose the ctm on ilk/snb currently, also RGB
-* limited range output is handled by the hw automagically.
-*/
-   crtc_state->csc_enable =
-   crtc_state->output_format != INTEL_OUTPUT_FORMAT_RGB;
+   if (crtc_state->output_format != INTEL_OUTPUT_FORMAT_RGB &&
+   crtc_state->hw.ctm) {
+   drm_dbg_kms(>drm,
+   "YCbCr and CTM together are not possible\n");
+   return -EINVAL;
+   }
+
+   crtc_state->gamma_enable = ilk_gamma_enable(crtc_state);
+
+   crtc_state->csc_enable = ilk_csc_enable(crtc_state);
  
  	crtc_state->gamma_mode = ilk_gamma_mode(crtc_state);
  
@@ -1546,7 +1567,6 @@ static u32 ivb_csc_mode(const struct intel_crtc_state *crtc_state)

  static int ivb_color_check(struct intel_crtc_state *crtc_state)
  {
struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc->dev);
-   bool limited_color_range = ilk_csc_limited_range(crtc_state);
int ret;
  
  	ret = check_luts(crtc_state);

@@ -1567,14 +1587,9 @@ static int ivb_color_check(struct intel_crtc_state 
*crtc_state)
return -EINVAL;
}
  
-	crtc_state->gamma_enable =

-   (crtc_state->hw.gamma_lut ||
-crtc_state->hw.degamma_lut) &&
-   !crtc_state->c8_planes;
+   crtc_state->gamma_enable = ilk_gamma_enable(crtc_state);
  
-	crtc_state->csc_enable =

-   crtc_state->output_format != INTEL_OUTPUT_FORMAT_RGB ||
-   crtc_state->hw.ctm || limited_color_range;
+   crtc_state->csc_enable = ilk_csc_enable(crtc_state);
  
  	crtc_state->gamma_mode = ivb_gamma_mode(crtc_state);

[Intel-gfx] [PATCH 10/11] drm/i915: Share {csc, gamma}_enable calculation for ilk/snb vs. ivb+

2022-10-26 Thread Ville Syrjala 
From: Ville Syrjälä 

ilk/snb vs. ivb+ hardware is mostly identical except for the addition
of the split gamma mode on ivb. Thus we can share the csc_enable
and gamma_enable calculation for both variants. Pull that stuff
into a few helpers.

Note that this also fills in the missing ctm/degamma stuff into
ilk_color_check() pretty much, so for good measure let's also
add a few extra checks relating to that, although we still don't
expose ctm/degamma to userspace. But now it'll be trivial to do
so if we wish.

Signed-off-by: Ville Syrjälä 
---
 drivers/gpu/drm/i915/display/intel_color.c | 49 ++
 1 file changed, 32 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_color.c 
b/drivers/gpu/drm/i915/display/intel_color.c
index 926784f266f2..33871bfacee7 100644
--- a/drivers/gpu/drm/i915/display/intel_color.c
+++ b/drivers/gpu/drm/i915/display/intel_color.c
@@ -1442,6 +1442,20 @@ static int chv_color_check(struct intel_crtc_state 
*crtc_state)
return 0;
 }
 
+static bool ilk_gamma_enable(const struct intel_crtc_state *crtc_state)
+{
+   return (crtc_state->hw.gamma_lut ||
+   crtc_state->hw.degamma_lut) &&
+   !crtc_state->c8_planes;
+}
+
+static bool ilk_csc_enable(const struct intel_crtc_state *crtc_state)
+{
+   return crtc_state->output_format != INTEL_OUTPUT_FORMAT_RGB ||
+   ilk_csc_limited_range(crtc_state) ||
+   crtc_state->hw.ctm;
+}
+
 static u32 ilk_gamma_mode(const struct intel_crtc_state *crtc_state)
 {
if (!crtc_state->gamma_enable ||
@@ -1487,22 +1501,29 @@ static void ilk_assign_luts(struct intel_crtc_state 
*crtc_state)
 
 static int ilk_color_check(struct intel_crtc_state *crtc_state)
 {
+   struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc->dev);
int ret;
 
ret = check_luts(crtc_state);
if (ret)
return ret;
 
-   crtc_state->gamma_enable =
-   crtc_state->hw.gamma_lut &&
-   !crtc_state->c8_planes;
+   if (crtc_state->hw.degamma_lut && crtc_state->hw.gamma_lut) {
+   drm_dbg_kms(>drm,
+   "Degamma and gamma together are not possible\n");
+   return -EINVAL;
+   }
 
-   /*
-* We don't expose the ctm on ilk/snb currently, also RGB
-* limited range output is handled by the hw automagically.
-*/
-   crtc_state->csc_enable =
-   crtc_state->output_format != INTEL_OUTPUT_FORMAT_RGB;
+   if (crtc_state->output_format != INTEL_OUTPUT_FORMAT_RGB &&
+   crtc_state->hw.ctm) {
+   drm_dbg_kms(>drm,
+   "YCbCr and CTM together are not possible\n");
+   return -EINVAL;
+   }
+
+   crtc_state->gamma_enable = ilk_gamma_enable(crtc_state);
+
+   crtc_state->csc_enable = ilk_csc_enable(crtc_state);
 
crtc_state->gamma_mode = ilk_gamma_mode(crtc_state);
 
@@ -1546,7 +1567,6 @@ static u32 ivb_csc_mode(const struct intel_crtc_state 
*crtc_state)
 static int ivb_color_check(struct intel_crtc_state *crtc_state)
 {
struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc->dev);
-   bool limited_color_range = ilk_csc_limited_range(crtc_state);
int ret;
 
ret = check_luts(crtc_state);
@@ -1567,14 +1587,9 @@ static int ivb_color_check(struct intel_crtc_state 
*crtc_state)
return -EINVAL;
}
 
-   crtc_state->gamma_enable =
-   (crtc_state->hw.gamma_lut ||
-crtc_state->hw.degamma_lut) &&
-   !crtc_state->c8_planes;
+   crtc_state->gamma_enable = ilk_gamma_enable(crtc_state);
 
-   crtc_state->csc_enable =
-   crtc_state->output_format != INTEL_OUTPUT_FORMAT_RGB ||
-   crtc_state->hw.ctm || limited_color_range;
+   crtc_state->csc_enable = ilk_csc_enable(crtc_state);
 
crtc_state->gamma_mode = ivb_gamma_mode(crtc_state);
 
-- 
2.37.4