[Intel-gfx] [PATCH v6 08/10] drm/i915/dsb: Enable gamma lut programming using DSB.
Animesh Manna
animesh.manna at intel.com
Thu Sep 12 13:26:51 UTC 2019
On 9/12/2019 6:37 PM, Jani Nikula wrote:
> On Thu, 12 Sep 2019, Animesh Manna <animesh.manna at intel.com> wrote:
>> Gamma lut programming can be programmed using DSB
>> where bulk register programming can be done using indexed
>> register write which takes number of data and the mmio offset
>> to be written.
>>
>> Currently enabled for 12-bit gamma LUT which is enabled by
>> default and later 8-bit/10-bit will be enabled in future
>> based on need.
>>
>> v1: Initial version.
>> v2: Directly call dsb-api at callsites. (Jani)
>> v3:
>> - modified the code as per single dsb instance per crtc. (Shashank)
>> - Added dsb get/put call in platform specific load_lut hook. (Jani)
>> - removed dsb pointer from dev_priv. (Jani)
>> v4: simplified code by dropping ref-count implementation. (Shashank)
>>
>> Cc: Jani Nikula <jani.nikula at intel.com>
>> Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
>> Cc: Shashank Sharma <shashank.sharma at intel.com>
>> Signed-off-by: Animesh Manna <animesh.manna at intel.com>
>> ---
>> drivers/gpu/drm/i915/display/intel_color.c | 57 +++++++++++++---------
>> 1 file changed, 35 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
>> index 318308dc136c..b6b9f0e5166b 100644
>> --- a/drivers/gpu/drm/i915/display/intel_color.c
>> +++ b/drivers/gpu/drm/i915/display/intel_color.c
>> @@ -611,12 +611,13 @@ static void bdw_load_lut_10(struct intel_crtc *crtc,
>> static void ivb_load_lut_ext_max(struct intel_crtc *crtc)
>> {
>> struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>> + struct intel_dsb *dsb = intel_dsb_get(crtc);
> When you've done enough kernel programming, you learn to expect get/put
> to be paired, and assume it's a bug if this isn't the case.
Already have done it in my previous version,
https://patchwork.freedesktop.org/patch/329481/?series=63013&rev=5
https://patchwork.freedesktop.org/patch/329482/?series=63013&rev=5
Can you please suggest if it is ok?
Regards,
Animesh
>
> So yeah, I did say it's okay not to have refcounting at first, *but* the
> expectation that get/put go in pairs is so deeply ingrained that I
> didn't even think to say you shouldn't have this kind of asymmetry.
>
> So this creates a problem, how do we pass the dsb pointer here, as
> without refcounting you can't actually have the put here as well,
> because you throw the stuff out before the commit.
>
> Maybe the easy answer is that we should just do the get and put at
> intel_crtc_init and intel_crtc_destroy. Or we could do it at atomic
> check and free.
>
> Ville, which approach would conflict with your future vblank worker
> stuff the least?
>
>
> BR,
> Jani.
>
>
>
>> enum pipe pipe = crtc->pipe;
>>
>> /* Program the max register to clamp values > 1.0. */
>> - I915_WRITE(PREC_PAL_EXT_GC_MAX(pipe, 0), 1 << 16);
>> - I915_WRITE(PREC_PAL_EXT_GC_MAX(pipe, 1), 1 << 16);
>> - I915_WRITE(PREC_PAL_EXT_GC_MAX(pipe, 2), 1 << 16);
>> + intel_dsb_reg_write(dsb, PREC_PAL_EXT_GC_MAX(pipe, 0), 1 << 16);
>> + intel_dsb_reg_write(dsb, PREC_PAL_EXT_GC_MAX(pipe, 1), 1 << 16);
>> + intel_dsb_reg_write(dsb, PREC_PAL_EXT_GC_MAX(pipe, 2), 1 << 16);
>>
>> /*
>> * Program the gc max 2 register to clamp values > 1.0.
>> @@ -624,9 +625,12 @@ static void ivb_load_lut_ext_max(struct intel_crtc *crtc)
>> * from 3.0 to 7.0
>> */
>> if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv)) {
>> - I915_WRITE(PREC_PAL_EXT2_GC_MAX(pipe, 0), 1 << 16);
>> - I915_WRITE(PREC_PAL_EXT2_GC_MAX(pipe, 1), 1 << 16);
>> - I915_WRITE(PREC_PAL_EXT2_GC_MAX(pipe, 2), 1 << 16);
>> + intel_dsb_reg_write(dsb, PREC_PAL_EXT2_GC_MAX(pipe, 0),
>> + 1 << 16);
>> + intel_dsb_reg_write(dsb, PREC_PAL_EXT2_GC_MAX(pipe, 1),
>> + 1 << 16);
>> + intel_dsb_reg_write(dsb, PREC_PAL_EXT2_GC_MAX(pipe, 2),
>> + 1 << 16);
>> }
>> }
>>
>> @@ -787,22 +791,22 @@ icl_load_gcmax(const struct intel_crtc_state *crtc_state,
>> const struct drm_color_lut *color)
>> {
>> struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
>> - struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>> + struct intel_dsb *dsb = intel_dsb_get(crtc);
>> enum pipe pipe = crtc->pipe;
>>
>> /* Fixme: LUT entries are 16 bit only, so we can prog 0xFFFF max */
>> - I915_WRITE(PREC_PAL_GC_MAX(pipe, 0), color->red);
>> - I915_WRITE(PREC_PAL_GC_MAX(pipe, 1), color->green);
>> - I915_WRITE(PREC_PAL_GC_MAX(pipe, 2), color->blue);
>> + intel_dsb_reg_write(dsb, PREC_PAL_GC_MAX(pipe, 0), color->red);
>> + intel_dsb_reg_write(dsb, PREC_PAL_GC_MAX(pipe, 1), color->green);
>> + intel_dsb_reg_write(dsb, PREC_PAL_GC_MAX(pipe, 2), color->blue);
>> }
>>
>> static void
>> icl_program_gamma_superfine_segment(const 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);
>> const struct drm_property_blob *blob = crtc_state->base.gamma_lut;
>> const struct drm_color_lut *lut = blob->data;
>> + struct intel_dsb *dsb = intel_dsb_get(crtc);
>> enum pipe pipe = crtc->pipe;
>> u32 i;
>>
>> @@ -813,15 +817,16 @@ icl_program_gamma_superfine_segment(const struct intel_crtc_state *crtc_state)
>> * Superfine segment has 9 entries, corresponding to values
>> * 0, 1/(8 * 128 * 256), 2/(8 * 128 * 256) .... 8/(8 * 128 * 256).
>> */
>> - I915_WRITE(PREC_PAL_MULTI_SEG_INDEX(pipe), PAL_PREC_AUTO_INCREMENT);
>> + intel_dsb_reg_write(dsb, PREC_PAL_MULTI_SEG_INDEX(pipe),
>> + PAL_PREC_AUTO_INCREMENT);
>>
>> for (i = 0; i < 9; i++) {
>> const struct drm_color_lut *entry = &lut[i];
>>
>> - I915_WRITE(PREC_PAL_MULTI_SEG_DATA(pipe),
>> - ilk_lut_12p4_ldw(entry));
>> - I915_WRITE(PREC_PAL_MULTI_SEG_DATA(pipe),
>> - ilk_lut_12p4_udw(entry));
>> + intel_dsb_indexed_reg_write(dsb, PREC_PAL_MULTI_SEG_DATA(pipe),
>> + ilk_lut_12p4_ldw(entry));
>> + intel_dsb_indexed_reg_write(dsb, PREC_PAL_MULTI_SEG_DATA(pipe),
>> + ilk_lut_12p4_udw(entry));
>> }
>> }
>>
>> @@ -829,10 +834,10 @@ static void
>> icl_program_gamma_multi_segment(const 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);
>> const struct drm_property_blob *blob = crtc_state->base.gamma_lut;
>> const struct drm_color_lut *lut = blob->data;
>> const struct drm_color_lut *entry;
>> + struct intel_dsb *dsb = intel_dsb_get(crtc);
>> enum pipe pipe = crtc->pipe;
>> u32 i;
>>
>> @@ -847,11 +852,13 @@ icl_program_gamma_multi_segment(const struct intel_crtc_state *crtc_state)
>> * PAL_PREC_INDEX[0] and PAL_PREC_INDEX[1] map to seg2[1],
>> * with seg2[0] being unused by the hardware.
>> */
>> - I915_WRITE(PREC_PAL_INDEX(pipe), PAL_PREC_AUTO_INCREMENT);
>> + intel_dsb_reg_write(dsb, PREC_PAL_INDEX(pipe), PAL_PREC_AUTO_INCREMENT);
>> for (i = 1; i < 257; i++) {
>> entry = &lut[i * 8];
>> - I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_12p4_ldw(entry));
>> - I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_12p4_udw(entry));
>> + intel_dsb_indexed_reg_write(dsb, PREC_PAL_DATA(pipe),
>> + ilk_lut_12p4_ldw(entry));
>> + intel_dsb_indexed_reg_write(dsb, PREC_PAL_DATA(pipe),
>> + ilk_lut_12p4_udw(entry));
>> }
>>
>> /*
>> @@ -868,8 +875,10 @@ icl_program_gamma_multi_segment(const struct intel_crtc_state *crtc_state)
>> */
>> for (i = 0; i < 256; i++) {
>> entry = &lut[i * 8 * 128];
>> - I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_12p4_ldw(entry));
>> - I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_12p4_udw(entry));
>> + intel_dsb_indexed_reg_write(dsb, PREC_PAL_DATA(pipe),
>> + ilk_lut_12p4_ldw(entry));
>> + intel_dsb_indexed_reg_write(dsb, PREC_PAL_DATA(pipe),
>> + ilk_lut_12p4_udw(entry));
>> }
>>
>> /* The last entry in the LUT is to be programmed in GCMAX */
>> @@ -882,6 +891,7 @@ static void icl_load_luts(const struct intel_crtc_state *crtc_state)
>> {
>> const struct drm_property_blob *gamma_lut = crtc_state->base.gamma_lut;
>> struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
>> + struct intel_dsb *dsb = intel_dsb_get(crtc);
>>
>> if (crtc_state->base.degamma_lut)
>> glk_load_degamma_lut(crtc_state);
>> @@ -900,6 +910,9 @@ static void icl_load_luts(const struct intel_crtc_state *crtc_state)
>> bdw_load_lut_10(crtc, gamma_lut, PAL_PREC_INDEX_VALUE(0));
>> ivb_load_lut_ext_max(crtc);
>> }
>> +
>> + intel_dsb_commit(dsb);
>> + intel_dsb_put(dsb);
>> }
>>
>> static u32 chv_cgm_degamma_ldw(const struct drm_color_lut *color)
More information about the Intel-gfx
mailing list