[v8,05/10] drm/i915/display: Extract i9xx_read_luts()
diff mbox series

Message ID 1566800772-18412-6-git-send-email-swati2.sharma@intel.com
State New
Headers show
Series
  • drm/i915: adding state checker for gamma lut value
Related show

Commit Message

Swati Sharma Aug. 26, 2019, 6:26 a.m. UTC
For the legacy gamma, have hw read out to create hw blob of gamma
lut values. Also, add function intel_color_lut_pack to convert hw value with
given bit_precision to lut property val.

Signed-off-by: Swati Sharma <swati2.sharma@intel.com>
---
 drivers/gpu/drm/i915/display/intel_color.c | 51 ++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_reg.h            |  3 ++
 2 files changed, 54 insertions(+)

Comments

Shankar, Uma Aug. 28, 2019, 3:55 p.m. UTC | #1
>-----Original Message-----
>From: Sharma, Swati2
>Sent: Monday, August 26, 2019 11:56 AM
>To: intel-gfx@lists.freedesktop.org
>Cc: Nikula, Jani <jani.nikula@intel.com>; Sharma, Shashank
><shashank.sharma@intel.com>; Manna, Animesh <animesh.manna@intel.com>;
>Nautiyal, Ankit K <ankit.k.nautiyal@intel.com>; daniel.vetter@ffwll.ch;
>ville.syrjala@linux.intel.com; Shankar, Uma <uma.shankar@intel.com>; Sharma,
>Swati2 <swati2.sharma@intel.com>
>Subject: [v8][PATCH 05/10] drm/i915/display: Extract i9xx_read_luts()
>
>For the legacy gamma, have hw read out to create hw blob of gamma lut values.

Would be better if we define platforms for which this is applicable (I mean what all is
considered legacy here)

>Also, add function intel_color_lut_pack to convert hw value with given bit_precision

Wrap this up within 75 characters.

>to lut property val.

Keep the version history, don't drop that.

>Signed-off-by: Swati Sharma <swati2.sharma@intel.com>
>---
> drivers/gpu/drm/i915/display/intel_color.c | 51 ++++++++++++++++++++++++++++++
> drivers/gpu/drm/i915/i915_reg.h            |  3 ++
> 2 files changed, 54 insertions(+)
>
>diff --git a/drivers/gpu/drm/i915/display/intel_color.c
>b/drivers/gpu/drm/i915/display/intel_color.c
>index 27727a1..45e0ee8 100644
>--- a/drivers/gpu/drm/i915/display/intel_color.c
>+++ b/drivers/gpu/drm/i915/display/intel_color.c
>@@ -1521,6 +1521,56 @@ bool intel_color_lut_equal(struct drm_property_blob
>*blob1,
> 	return true;
> }
>
>+/* convert hw value with given bit_precision to lut property val */
>+static u32 intel_color_lut_pack(u32 val, u32 bit_precision) {
>+	u32 max = 0xffff >> (16 - bit_precision);
>+
>+	val = clamp_val(val, 0, max);
>+
>+	if (bit_precision < 16)
>+		val <<= 16 - bit_precision;
>+
>+	return val;
>+}
>+
>+static struct drm_property_blob *
>+i9xx_read_lut_8(struct intel_crtc_state *crtc_state) {

Would be good to add some comments describing the rationale of this
function. Why 8 etc.

>+	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
>+	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>+	enum pipe pipe = crtc->pipe;
>+	struct drm_property_blob *blob;
>+	struct drm_color_lut *blob_data;
>+	u32 i, val;
>+
>+	blob = drm_property_create_blob(&dev_priv->drm,
>+					sizeof(struct drm_color_lut) * 256,

Have a macro for 256. Explain why 256, add comments.

>+					NULL);
>+	if (IS_ERR(blob))
>+		return NULL;
>+
>+	blob_data = blob->data;
>+
>+	for (i = 0; i < 256; i++) {

Add the macro for 256.

>+		if (HAS_GMCH(dev_priv))
>+			val = I915_READ(PALETTE(pipe, i));
>+		else
>+			val = I915_READ(LGC_PALETTE(pipe, i));
>+
>+		blob_data[i].red =
>intel_color_lut_pack(REG_FIELD_GET(LGC_PALETTE_RED_MASK, val), 8);
>+		blob_data[i].green =
>intel_color_lut_pack(REG_FIELD_GET(LGC_PALETTE_GREEN_MASK, val), 8);
>+		blob_data[i].blue =
>intel_color_lut_pack(REG_FIELD_GET(LGC_PALETTE_BLUE_MASK, val), 8);
>+	}
>+
>+	return blob;
>+}
>+
>+void i9xx_read_luts(struct intel_crtc_state *crtc_state) {
>+	crtc_state->base.gamma_lut = i9xx_read_lut_8(crtc_state); }
>+
> void intel_color_init(struct intel_crtc *crtc)  {
> 	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); @@ -1541,6
>+1591,7 @@ void intel_color_init(struct intel_crtc *crtc)
> 			dev_priv->display.color_check = i9xx_color_check;
> 			dev_priv->display.color_commit = i9xx_color_commit;
> 			dev_priv->display.load_luts = i9xx_load_luts;
>+			dev_priv->display.read_luts = i9xx_read_luts;
> 		}
> 	} else {
> 		if (INTEL_GEN(dev_priv) >= 11)
>diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index
>a092b34..b687faa 100644
>--- a/drivers/gpu/drm/i915/i915_reg.h
>+++ b/drivers/gpu/drm/i915/i915_reg.h
>@@ -7192,6 +7192,9 @@ enum {
> /* legacy palette */
> #define _LGC_PALETTE_A           0x4a000
> #define _LGC_PALETTE_B           0x4a800
>+#define LGC_PALETTE_RED_MASK     REG_GENMASK(23, 16)
>+#define LGC_PALETTE_GREEN_MASK   REG_GENMASK(15, 8)
>+#define LGC_PALETTE_BLUE_MASK    REG_GENMASK(7, 0)
> #define LGC_PALETTE(pipe, i) _MMIO(_PIPE(pipe, _LGC_PALETTE_A,
>_LGC_PALETTE_B) + (i) * 4)
>
> /* ilk/snb precision palette */
>--
>1.9.1
Swati Sharma Aug. 28, 2019, 8:29 p.m. UTC | #2
On 28-Aug-19 9:25 PM, Shankar, Uma wrote:
>
>> -----Original Message-----
>> From: Sharma, Swati2
>> Sent: Monday, August 26, 2019 11:56 AM
>> To: intel-gfx@lists.freedesktop.org
>> Cc: Nikula, Jani <jani.nikula@intel.com>; Sharma, Shashank
>> <shashank.sharma@intel.com>; Manna, Animesh <animesh.manna@intel.com>;
>> Nautiyal, Ankit K <ankit.k.nautiyal@intel.com>; daniel.vetter@ffwll.ch;
>> ville.syrjala@linux.intel.com; Shankar, Uma <uma.shankar@intel.com>; Sharma,
>> Swati2 <swati2.sharma@intel.com>
>> Subject: [v8][PATCH 05/10] drm/i915/display: Extract i9xx_read_luts()
>>
>> For the legacy gamma, have hw read out to create hw blob of gamma lut values.
> Would be better if we define platforms for which this is applicable (I mean what all is
> considered legacy here)
>
>> Also, add function intel_color_lut_pack to convert hw value with given bit_precision
> Wrap this up within 75 characters.
>
>> to lut property val.
> Keep the version history, don't drop that.
>
>> Signed-off-by: Swati Sharma <swati2.sharma@intel.com>
>> ---
>> drivers/gpu/drm/i915/display/intel_color.c | 51 ++++++++++++++++++++++++++++++
>> drivers/gpu/drm/i915/i915_reg.h            |  3 ++
>> 2 files changed, 54 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_color.c
>> b/drivers/gpu/drm/i915/display/intel_color.c
>> index 27727a1..45e0ee8 100644
>> --- a/drivers/gpu/drm/i915/display/intel_color.c
>> +++ b/drivers/gpu/drm/i915/display/intel_color.c
>> @@ -1521,6 +1521,56 @@ bool intel_color_lut_equal(struct drm_property_blob
>> *blob1,
>> 	return true;
>> }
>>
>> +/* convert hw value with given bit_precision to lut property val */
>> +static u32 intel_color_lut_pack(u32 val, u32 bit_precision) {
>> +	u32 max = 0xffff >> (16 - bit_precision);
>> +
>> +	val = clamp_val(val, 0, max);
>> +
>> +	if (bit_precision < 16)
>> +		val <<= 16 - bit_precision;
>> +
>> +	return val;
>> +}
>> +
>> +static struct drm_property_blob *
>> +i9xx_read_lut_8(struct intel_crtc_state *crtc_state) {
> Would be good to add some comments describing the rationale of this
> function. Why 8 etc.
Func is written similar to load luts for i9xx.Therefore didn't explain 8.
Do I need to add comment for all the functions/platform? Won't commit 
message
sufficient enough?
>
>> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
>> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>> +	enum pipe pipe = crtc->pipe;
>> +	struct drm_property_blob *blob;
>> +	struct drm_color_lut *blob_data;
>> +	u32 i, val;
>> +
>> +	blob = drm_property_create_blob(&dev_priv->drm,
>> +					sizeof(struct drm_color_lut) * 256,
> Have a macro for 256. Explain why 256, add comments.
This is similar to load luts, since nothing new i added so didn't give
explanation. I can re-use LEGACY_LUT_LENGTH for this, since wanted these
functions to be similar to load_luts, therefore kept same.
>
>> +					NULL);
>> +	if (IS_ERR(blob))
>> +		return NULL;
>> +
>> +	blob_data = blob->data;
>> +
>> +	for (i = 0; i < 256; i++) {
> Add the macro for 256.
Macro already there LEGACY_LUT_LENGTH. Should i reuse that?
>
>> +		if (HAS_GMCH(dev_priv))
>> +			val = I915_READ(PALETTE(pipe, i));
>> +		else
>> +			val = I915_READ(LGC_PALETTE(pipe, i));
>> +
>> +		blob_data[i].red =
>> intel_color_lut_pack(REG_FIELD_GET(LGC_PALETTE_RED_MASK, val), 8);
>> +		blob_data[i].green =
>> intel_color_lut_pack(REG_FIELD_GET(LGC_PALETTE_GREEN_MASK, val), 8);
>> +		blob_data[i].blue =
>> intel_color_lut_pack(REG_FIELD_GET(LGC_PALETTE_BLUE_MASK, val), 8);
>> +	}
>> +
>> +	return blob;
>> +}
>> +
>> +void i9xx_read_luts(struct intel_crtc_state *crtc_state) {
>> +	crtc_state->base.gamma_lut = i9xx_read_lut_8(crtc_state); }
>> +
>> void intel_color_init(struct intel_crtc *crtc)  {
>> 	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); @@ -1541,6
>> +1591,7 @@ void intel_color_init(struct intel_crtc *crtc)
>> 			dev_priv->display.color_check = i9xx_color_check;
>> 			dev_priv->display.color_commit = i9xx_color_commit;
>> 			dev_priv->display.load_luts = i9xx_load_luts;
>> +			dev_priv->display.read_luts = i9xx_read_luts;
>> 		}
>> 	} else {
>> 		if (INTEL_GEN(dev_priv) >= 11)
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index
>> a092b34..b687faa 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -7192,6 +7192,9 @@ enum {
>> /* legacy palette */
>> #define _LGC_PALETTE_A           0x4a000
>> #define _LGC_PALETTE_B           0x4a800
>> +#define LGC_PALETTE_RED_MASK     REG_GENMASK(23, 16)
>> +#define LGC_PALETTE_GREEN_MASK   REG_GENMASK(15, 8)
>> +#define LGC_PALETTE_BLUE_MASK    REG_GENMASK(7, 0)
>> #define LGC_PALETTE(pipe, i) _MMIO(_PIPE(pipe, _LGC_PALETTE_A,
>> _LGC_PALETTE_B) + (i) * 4)
>>
>> /* ilk/snb precision palette */
>> --
>> 1.9.1
Shankar, Uma Aug. 29, 2019, 3:15 p.m. UTC | #3
From: Sharma, Swati2
Sent: Thursday, August 29, 2019 2:00 AM
To: Shankar, Uma <uma.shankar@intel.com>; intel-gfx@lists.freedesktop.org
Cc: Nikula, Jani <jani.nikula@intel.com>; Sharma, Shashank <shashank.sharma@intel.com>; Manna, Animesh <animesh.manna@intel.com>; Nautiyal, Ankit K <ankit.k.nautiyal@intel.com>; daniel.vetter@ffwll.ch; ville.syrjala@linux.intel.com
Subject: Re: [v8][PATCH 05/10] drm/i915/display: Extract i9xx_read_luts()

On 28-Aug-19 9:25 PM, Shankar, Uma wrote:





-----Original Message-----

From: Sharma, Swati2

Sent: Monday, August 26, 2019 11:56 AM

To: intel-gfx@lists.freedesktop.org<mailto:intel-gfx@lists.freedesktop.org>

Cc: Nikula, Jani <jani.nikula@intel.com><mailto:jani.nikula@intel.com>; Sharma, Shashank

<shashank.sharma@intel.com><mailto:shashank.sharma@intel.com>; Manna, Animesh <animesh.manna@intel.com><mailto:animesh.manna@intel.com>;

Nautiyal, Ankit K <ankit.k.nautiyal@intel.com><mailto:ankit.k.nautiyal@intel.com>; daniel.vetter@ffwll.ch<mailto:daniel.vetter@ffwll.ch>;

ville.syrjala@linux.intel.com<mailto:ville.syrjala@linux.intel.com>; Shankar, Uma <uma.shankar@intel.com><mailto:uma.shankar@intel.com>; Sharma,

Swati2 <swati2.sharma@intel.com><mailto:swati2.sharma@intel.com>

Subject: [v8][PATCH 05/10] drm/i915/display: Extract i9xx_read_luts()



For the legacy gamma, have hw read out to create hw blob of gamma lut values.



Would be better if we define platforms for which this is applicable (I mean what all is

considered legacy here)



Also, add function intel_color_lut_pack to convert hw value with given bit_precision



Wrap this up within 75 characters.



to lut property val.



Keep the version history, don't drop that.



Signed-off-by: Swati Sharma <swati2.sharma@intel.com><mailto:swati2.sharma@intel.com>

---

drivers/gpu/drm/i915/display/intel_color.c | 51 ++++++++++++++++++++++++++++++

drivers/gpu/drm/i915/i915_reg.h            |  3 ++

2 files changed, 54 insertions(+)



diff --git a/drivers/gpu/drm/i915/display/intel_color.c

b/drivers/gpu/drm/i915/display/intel_color.c

index 27727a1..45e0ee8 100644

--- a/drivers/gpu/drm/i915/display/intel_color.c

+++ b/drivers/gpu/drm/i915/display/intel_color.c

@@ -1521,6 +1521,56 @@ bool intel_color_lut_equal(struct drm_property_blob

*blob1,

   return true;

}



+/* convert hw value with given bit_precision to lut property val */

+static u32 intel_color_lut_pack(u32 val, u32 bit_precision) {

+  u32 max = 0xffff >> (16 - bit_precision);

+

+  val = clamp_val(val, 0, max);

+

+  if (bit_precision < 16)

+         val <<= 16 - bit_precision;

+

+  return val;

+}

+

+static struct drm_property_blob *

+i9xx_read_lut_8(struct intel_crtc_state *crtc_state) {



Would be good to add some comments describing the rationale of this

function. Why 8 etc.
Func is written similar to load luts for i9xx.Therefore didn't explain 8.
Do I need to add comment for all the functions/platform? Won't commit message
sufficient enough?

Commit message will not be visible while browsing the code. Would be good to add comments explaining the rationale.



+  struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);

+  struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);

+  enum pipe pipe = crtc->pipe;

+  struct drm_property_blob *blob;

+  struct drm_color_lut *blob_data;

+  u32 i, val;

+

+  blob = drm_property_create_blob(&dev_priv->drm,

+                                sizeof(struct drm_color_lut) * 256,



Have a macro for 256. Explain why 256, add comments.
This is similar to load luts, since nothing new i added so didn't give
explanation. I can re-use LEGACY_LUT_LENGTH for this, since wanted these
functions to be similar to load_luts, therefore kept same.

Ok, we still can use the macro.



+                                NULL);

+  if (IS_ERR(blob))

+         return NULL;

+

+  blob_data = blob->data;

+

+  for (i = 0; i < 256; i++) {



Add the macro for 256.
Macro already there LEGACY_LUT_LENGTH. Should i reuse that?

Yes you can reuse it.





+         if (HAS_GMCH(dev_priv))

+                 val = I915_READ(PALETTE(pipe, i));

+         else

+                 val = I915_READ(LGC_PALETTE(pipe, i));

+

+         blob_data[i].red =

intel_color_lut_pack(REG_FIELD_GET(LGC_PALETTE_RED_MASK, val), 8);

+         blob_data[i].green =

intel_color_lut_pack(REG_FIELD_GET(LGC_PALETTE_GREEN_MASK, val), 8);

+         blob_data[i].blue =

intel_color_lut_pack(REG_FIELD_GET(LGC_PALETTE_BLUE_MASK, val), 8);

+  }

+

+  return blob;

+}

+

+void i9xx_read_luts(struct intel_crtc_state *crtc_state) {

+  crtc_state->base.gamma_lut = i9xx_read_lut_8(crtc_state); }

+

void intel_color_init(struct intel_crtc *crtc)  {

   struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); @@ -1541,6

+1591,7 @@ void intel_color_init(struct intel_crtc *crtc)

                  dev_priv->display.color_check = i9xx_color_check;

                  dev_priv->display.color_commit = i9xx_color_commit;

                  dev_priv->display.load_luts = i9xx_load_luts;

+                 dev_priv->display.read_luts = i9xx_read_luts;

          }

   } else {

          if (INTEL_GEN(dev_priv) >= 11)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index

a092b34..b687faa 100644

--- a/drivers/gpu/drm/i915/i915_reg.h

+++ b/drivers/gpu/drm/i915/i915_reg.h

@@ -7192,6 +7192,9 @@ enum {

/* legacy palette */

#define _LGC_PALETTE_A           0x4a000

#define _LGC_PALETTE_B           0x4a800

+#define LGC_PALETTE_RED_MASK     REG_GENMASK(23, 16)

+#define LGC_PALETTE_GREEN_MASK   REG_GENMASK(15, 8)

+#define LGC_PALETTE_BLUE_MASK    REG_GENMASK(7, 0)

#define LGC_PALETTE(pipe, i) _MMIO(_PIPE(pipe, _LGC_PALETTE_A,

_LGC_PALETTE_B) + (i) * 4)



/* ilk/snb precision palette */

--

1.9.1






--

~Swati Sharma

Patch
diff mbox series

diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
index 27727a1..45e0ee8 100644
--- a/drivers/gpu/drm/i915/display/intel_color.c
+++ b/drivers/gpu/drm/i915/display/intel_color.c
@@ -1521,6 +1521,56 @@  bool intel_color_lut_equal(struct drm_property_blob *blob1,
 	return true;
 }
 
+/* convert hw value with given bit_precision to lut property val */
+static u32 intel_color_lut_pack(u32 val, u32 bit_precision)
+{
+	u32 max = 0xffff >> (16 - bit_precision);
+
+	val = clamp_val(val, 0, max);
+
+	if (bit_precision < 16)
+		val <<= 16 - bit_precision;
+
+	return val;
+}
+
+static struct drm_property_blob *
+i9xx_read_lut_8(struct intel_crtc_state *crtc_state)
+{
+	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
+	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
+	enum pipe pipe = crtc->pipe;
+	struct drm_property_blob *blob;
+	struct drm_color_lut *blob_data;
+	u32 i, val;
+
+	blob = drm_property_create_blob(&dev_priv->drm,
+					sizeof(struct drm_color_lut) * 256,
+					NULL);
+	if (IS_ERR(blob))
+		return NULL;
+
+	blob_data = blob->data;
+
+	for (i = 0; i < 256; i++) {
+		if (HAS_GMCH(dev_priv))
+			val = I915_READ(PALETTE(pipe, i));
+		else
+			val = I915_READ(LGC_PALETTE(pipe, i));
+
+		blob_data[i].red = intel_color_lut_pack(REG_FIELD_GET(LGC_PALETTE_RED_MASK, val), 8);
+		blob_data[i].green = intel_color_lut_pack(REG_FIELD_GET(LGC_PALETTE_GREEN_MASK, val), 8);
+		blob_data[i].blue = intel_color_lut_pack(REG_FIELD_GET(LGC_PALETTE_BLUE_MASK, val), 8);
+	}
+
+	return blob;
+}
+
+void i9xx_read_luts(struct intel_crtc_state *crtc_state)
+{
+	crtc_state->base.gamma_lut = i9xx_read_lut_8(crtc_state);
+}
+
 void intel_color_init(struct intel_crtc *crtc)
 {
 	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
@@ -1541,6 +1591,7 @@  void intel_color_init(struct intel_crtc *crtc)
 			dev_priv->display.color_check = i9xx_color_check;
 			dev_priv->display.color_commit = i9xx_color_commit;
 			dev_priv->display.load_luts = i9xx_load_luts;
+			dev_priv->display.read_luts = i9xx_read_luts;
 		}
 	} else {
 		if (INTEL_GEN(dev_priv) >= 11)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index a092b34..b687faa 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -7192,6 +7192,9 @@  enum {
 /* legacy palette */
 #define _LGC_PALETTE_A           0x4a000
 #define _LGC_PALETTE_B           0x4a800
+#define LGC_PALETTE_RED_MASK     REG_GENMASK(23, 16)
+#define LGC_PALETTE_GREEN_MASK   REG_GENMASK(15, 8)
+#define LGC_PALETTE_BLUE_MASK    REG_GENMASK(7, 0)
 #define LGC_PALETTE(pipe, i) _MMIO(_PIPE(pipe, _LGC_PALETTE_A, _LGC_PALETTE_B) + (i) * 4)
 
 /* ilk/snb precision palette */