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

["Sharma, Swati2" <swati2.sharma@intel.com>]

[-- Attachment #1.1: Type: text/plain, Size: 4851 bytes --]

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


-- 
~Swati Sharma


[-- Attachment #1.2: Type: text/html, Size: 7554 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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