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 ; Sharma, Shashank >> ; Manna, Animesh ; >> Nautiyal, Ankit K ; daniel.vetter@ffwll.ch; >> ville.syrjala@linux.intel.com; Shankar, Uma ; Sharma, >> Swati2 >> 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 >> --- >> 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