Re: [PATCH 3/4] [v5] drm/i915/color: Extract icl_read_luts()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 15-Oct-19 6:04 PM, Ville Syrjälä wrote:
On Thu, Oct 10, 2019 at 04:20:04PM +0530, Sharma, Swati2 wrote:
On 09-Oct-19 7:46 PM, Ville Syrjälä wrote:
On Wed, Oct 09, 2019 at 12:25:41PM +0530, Swati Sharma wrote:
For icl+, have hw read out to create hw blob of gamma
lut values. icl+ platforms supports multi segmented gamma
mode by default, add hw lut creation for this mode.

This will be used to validate gamma programming using dsb
(display state buffer) which is a tgl specific feature.

Major change done-removal of readouts of coarse and fine segments
because PAL_PREC_DATA register isn't giving propoer values.
State checker limited only to "fine segment"

v2: -readout code for multisegmented gamma has to come
       up with some intermediate entries that aren't preserved
       in hardware (Jani N)
      -linear interpolation (Ville)
      -moved common code to check gamma_enable to specific funcs,
       since icl doesn't support that
v3: -use u16 instead of __u16 [Jani N]
      -used single lut [Jani N]
      -improved and more readable for loops [Jani N]
      -read values directly to actual locations and then fill gaps [Jani N]
      -moved cleaning to patch 1 [Jani N]
      -renamed icl_read_lut_multi_seg() to icl_read_lut_multi_segment to
       make it similar to icl_load_luts()
      -renamed icl_compute_interpolated_gamma_blob() to
       icl_compute_interpolated_gamma_lut_values() more sensible, I guess
v4: -removed interpolated func for creating gamma lut values
      -removed readouts of fine and coarse segments, failure to read PAL_PREC_DATA
       correctly
v5: -added gamma_enable check inside read_luts()

Signed-off-by: Swati Sharma <swati2.sharma@xxxxxxxxx>
---
   drivers/gpu/drm/i915/display/intel_color.c | 114 ++++++++++++++++++---
   drivers/gpu/drm/i915/i915_reg.h            |   6 ++
   2 files changed, 108 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
index fa44eb73d088..614e0ad386ca 100644
--- a/drivers/gpu/drm/i915/display/intel_color.c
+++ b/drivers/gpu/drm/i915/display/intel_color.c
@@ -1477,6 +1477,25 @@ static int glk_gamma_precision(const struct intel_crtc_state *crtc_state)
   	}
   }
+static int icl_gamma_precision(const struct intel_crtc_state *crtc_state)
+{
+	if ((crtc_state->gamma_mode & POST_CSC_GAMMA_ENABLE) == 0)
+		return 0;
+
+	switch (crtc_state->gamma_mode & GAMMA_MODE_MODE_MASK) {
+	case GAMMA_MODE_MODE_8BIT:
+		return 8;
+	case GAMMA_MODE_MODE_10BIT:
+		return 10;
+	case GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED:
+		return 16;
+	default:
+		MISSING_CASE(crtc_state->gamma_mode);
+		return 0;
+	}
+
+}
+
   int intel_color_get_gamma_bit_precision(const struct intel_crtc_state *crtc_state)
   {
   	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
@@ -1488,7 +1507,9 @@ int intel_color_get_gamma_bit_precision(const struct intel_crtc_state *crtc_stat
   		else
   			return i9xx_gamma_precision(crtc_state);
   	} else {
-		if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv))
+		if (INTEL_GEN(dev_priv) >= 11)
+			return icl_gamma_precision(crtc_state);
+		else if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv))
   			return glk_gamma_precision(crtc_state);
   		else if (IS_IRONLAKE(dev_priv))
   			return ilk_gamma_precision(crtc_state);
@@ -1519,6 +1540,20 @@ static bool intel_color_lut_entry_equal(struct drm_color_lut *lut1,
   	return true;
   }
+static bool intel_color_lut_entry_multi_equal(struct drm_color_lut *lut1,
+					      struct drm_color_lut *lut2,
+					      int lut_size, u32 err)
+{
+	int i;
+
+	for (i = 0; i < 9; i++) {
+		if (!err_check(&lut1[i], &lut2[i], err))
+			return false;
+	}
+
+	return true;
+}
+
   bool intel_color_lut_equal(struct drm_property_blob *blob1,
   			   struct drm_property_blob *blob2,
   			   u32 gamma_mode, u32 bit_precision)
@@ -1537,16 +1572,8 @@ bool intel_color_lut_equal(struct drm_property_blob *blob1,
   	lut_size2 = drm_color_lut_size(blob2);
/* check sw and hw lut size */
-	switch (gamma_mode) {
-	case GAMMA_MODE_MODE_8BIT:
-	case GAMMA_MODE_MODE_10BIT:
-		if (lut_size1 != lut_size2)
-			return false;
-		break;
-	default:
-		MISSING_CASE(gamma_mode);
-			return false;
-	}
+	if (lut_size1 != lut_size2)
+		return false;
lut1 = blob1->data;
   	lut2 = blob2->data;
@@ -1554,13 +1581,18 @@ bool intel_color_lut_equal(struct drm_property_blob *blob1,
   	err = 0xffff >> bit_precision;
/* check sw and hw lut entry to be equal */
-	switch (gamma_mode) {
+	switch (gamma_mode & GAMMA_MODE_MODE_MASK) {
   	case GAMMA_MODE_MODE_8BIT:
   	case GAMMA_MODE_MODE_10BIT:
   		if (!intel_color_lut_entry_equal(lut1, lut2,
   						 lut_size2, err))
   			return false;
   		break;
+	case GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED:
+		if (!intel_color_lut_entry_multi_equal(lut1, lut2,
+						       lut_size2, err))

I don't think you need a new function for that. Just pass 9 as the size
to intel_color_lut_entry_equal() ?

I had made a separate function for multi-segmented gamma since there
will be 3 loops for comparing superfine, fine and course segments which
wont go with intel_lut_entry_equal() structure.

Right now we are limiting to superfine segment only but in future we
will add for other segments too (once we get fix from h/w)

Func() should look like this. Actually there is no need to passing
lut_size only in this function if we continue with this function only.

+static bool intel_color_lut_entry_multi_equal(struct drm_color_lut *lut1,
+                                             struct drm_color_lut *lut2,
+                                             int lut_size, u32 err)
+{
+       int i;
+
+       for (i = 0; i < 9; i++) {
+               if (!err_check(&lut1[i], &lut2[i], err))
+                       return false;
+       }
+
+       for (i = 1; i < 257; i++) {
+               if (!err_check(&lut1[i * 8], &lut2[i * 8], err))
+                       return false;
+       }
+
+       for (i = 0; i < 256; i++) {
+               if (!err_check(&lut1[i * 8 * 128], &lut2[i * 8 * 128], err))
+                       return false;
+       }
+
+       return true;
+}
+
Please suggest.

There's not much point in duplicating code until it's proven to
be required. Who knows when the hw gets fixed, maybe never.
Now, got your point. Thanks!



Hmm, should probably rename that to just intel_color_lut_equal() since
it checks the entire LUT (or at least the specified subset) and not
just a single entry...
Already intel_color_lut_equal() function exits, should i rename that to
intel_color_blob_equal() or intel_color_gamma_blob()? and make this func intel_color_lut_entry_equal() to intel_color_lut_equal() as suggested by you?
Please comment!

This will be fine for this segment but for other two segments it won't
work. Right?

We could generalize it to take start+end+stride. Dunno if that would be
particularly beneficial though.



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




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux