>-----Original Message----- >From: Ville Syrjälä [mailto:ville.syrjala@xxxxxxxxxxxxxxx] >Sent: Monday, April 8, 2019 3:50 PM >To: Shankar, Uma <uma.shankar@xxxxxxxxx> >Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; dri-devel@xxxxxxxxxxxxxxxxxxxxx; >dcastagna@xxxxxxxxxxxx; seanpaul@xxxxxxxxxxxx; Syrjala, Ville ><ville.syrjala@xxxxxxxxx>; harry.wentland@xxxxxxx; Lankhorst, Maarten ><maarten.lankhorst@xxxxxxxxx> >Subject: Re: [v2 5/7] drm/i915/icl: Add support for multi segmented >gamma mode > >On Mon, Apr 01, 2019 at 11:00:09PM +0530, Uma Shankar wrote: >> Gen11 introduced a new gamma mode i.e, multi segmented gamma mode. >> Added support for the same. >> >> Signed-off-by: Uma Shankar <uma.shankar@xxxxxxxxx> >> --- >> drivers/gpu/drm/i915/intel_color.c | 161 >++++++++++++++++++++++++++++++++++++- >> include/drm/drm_crtc.h | 3 + >> 2 files changed, 161 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_color.c >> b/drivers/gpu/drm/i915/intel_color.c >> index 84d93ec..d81de32 100644 >> --- a/drivers/gpu/drm/i915/intel_color.c >> +++ b/drivers/gpu/drm/i915/intel_color.c >> @@ -57,6 +57,12 @@ >> >> #define ILK_CSC_POSTOFF_LIMITED_RANGE (16 * (1 << 12) / 255) >> >> +#define LEGACY_PALETTE_MODE_8BIT BIT(0) >> +#define PRECISION_PALETTE_MODE_10BIT BIT(1) >> +#define INTERPOLATED_GAMMA_MODE_12BIT BIT(2) >> +#define MULTI_SEGMENTED_GAMMA_MODE_12BIT BIT(3) >> +#define SPLIT_GAMMA_MODE_12BIT BIT(4) >> + >> static const u16 ilk_csc_off_zero[3] = {}; >> >> static const u16 ilk_csc_coeff_identity[9] = { @@ -92,6 +98,9 @@ >> 0x0800, 0x0100, 0x0800, >> }; >> >> +#define GEN11_GET_MS10BITS_OF_LUT(lut) (((lut) >> 6) & 0x3FF) #define >> +GEN11_GET_LS6BITS_OF_LUT(lut) ((lut) & 0x3F) > >I think this is just the ilk+ 12.4 format. IIRC I had some kind of >ilk_lut_12p4_ldw/udw() funcs for these in my branch somewhere. >I'd like to see somehting like that in this patch. Sure, I will add those and use the same. Something similar to (i965_lut_10p6_udw) >> + >> static bool lut_is_legacy(const struct drm_property_blob *lut) { >> return drm_color_lut_size(lut) == LEGACY_LUT_LENGTH; @@ -670,16 >> +679,149 @@ static void glk_load_luts(const struct intel_crtc_state *crtc_state) >> bdw_load_gamma_lut(crtc_state, 0); >> } >> >> +static void icl_program_coarse_segment_lut(const struct intel_crtc_state >> + *crtc_state, >> + struct drm_color_lut *gamma_lut, >> + u32 offset) > >I think to match the current pattern we should have something like: > >icl_load_lut_coarse_segment(struct intel_crtc *crtc, > const struct drm_property_blob *blob); >icl_load_lut_fine_segment(struct intel_crtc *crtc, > const struct drm_property_blob *blob); Ok, will update this. >> +{ >> + 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_color_lut *lut = gamma_lut; >> + enum pipe pipe = crtc->pipe; >> + u32 i, lut_size, word; >> + >> + WARN_ON(offset & ~PAL_PREC_INDEX_VALUE_MASK); >> + >> + I915_WRITE(PREC_PAL_INDEX(pipe), >> + (offset ? PAL_PREC_SPLIT_MODE : 0) | >> + PAL_PREC_AUTO_INCREMENT | >> + offset); >> + >> + if (lut && crtc_state->base.gamma_mode_type == >> + MULTI_SEGMENTED_GAMMA_MODE_12BIT) { >> + lut_size = 9 + 514; >> + for (i = 9; i < lut_size; i++) { >> + /* For Even Index */ >> + word = (GEN11_GET_LS6BITS_OF_LUT(lut[i].red) << 20) | >> + (GEN11_GET_LS6BITS_OF_LUT(lut[i].green) << 10) | >> + GEN11_GET_LS6BITS_OF_LUT(lut[i].blue); >> + >> + I915_WRITE(PREC_PAL_DATA(pipe), word); >> + >> + /* For ODD index */ >> + word = (GEN11_GET_MS10BITS_OF_LUT(lut[i].red) << 20) | >> + (GEN11_GET_MS10BITS_OF_LUT(lut[i].green) << 10) >| >> + GEN11_GET_MS10BITS_OF_LUT(lut[i].blue); >> + >> + I915_WRITE(PREC_PAL_DATA(pipe), word); >> + } >> + } >> + >> + /* >> + * Program the max register to clamp values > 1.0. >> + * ToDo: Extend the ABI to be able to program values >> + * from 1.0 >> + */ >> + I915_WRITE(PREC_PAL_GC_MAX(pipe, 0), (1 << 16)); >> + I915_WRITE(PREC_PAL_GC_MAX(pipe, 1), (1 << 16)); >> + I915_WRITE(PREC_PAL_GC_MAX(pipe, 2), (1 << 16)); >> + >> + /* >> + * Program the max register to clamp values > 1.0. >> + * ToDo: Extend the ABI to be able to program values >> + * from 1.0 to 3.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)); > >Doubled thing. But we can just drop these and call >ivb_load_lut_10_max() from higher up instead. Actually one is GC_MAX and another EXT_GC_MAX. There seem to be 3 set of registers to be programed here. Sure, I will use the ivb helper here to optimize this. >> + >> + /* >> + * Program the gc max 2 register to clamp values > 1.0. >> + * ToDo: Extend the ABI to be able to program values >> + * 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)); >> + } >> +} >> + >> +static void icl_program_fine_segment_lut(const struct intel_crtc_state >> + *crtc_state, >> + struct drm_color_lut *gamma_lut, >> + u32 offset) >> +{ >> + struct drm_crtc *crtc = crtc_state->base.crtc; >> + struct drm_device *dev = crtc_state->base.crtc->dev; >> + struct drm_i915_private *dev_priv = to_i915(dev); >> + enum pipe pipe = to_intel_crtc(crtc)->pipe; >> + u32 i, word, lut_size = 9; >> + >> + WARN_ON(offset & ~PAL_PREC_MULTI_SEGMENT_INDEX_VALUE_MASK); >> + >> + I915_WRITE(PREC_PAL_MULTI_SEG_INDEX(pipe), >> + (PAL_PREC_AUTO_INCREMENT | offset)); >> + >> + if (gamma_lut) { >> + struct drm_color_lut *lut = >> + (struct drm_color_lut *)gamma_lut; >> + >> + for (i = 0; i < lut_size; i++) { >> + /* For Even Index */ >> + word = (GEN11_GET_LS6BITS_OF_LUT(lut[i].red) << 20) | >> + (GEN11_GET_LS6BITS_OF_LUT(lut[i].green) << 10) | >> + GEN11_GET_LS6BITS_OF_LUT(lut[i].blue); >> + >> + I915_WRITE(PREC_PAL_MULTI_SEG_DATA(pipe), word); >> + >> + /* For ODD index */ >> + word = (GEN11_GET_MS10BITS_OF_LUT(lut[i].red) << 20) | >> + (GEN11_GET_MS10BITS_OF_LUT(lut[i].green) << 10) >| >> + GEN11_GET_MS10BITS_OF_LUT(lut[i].blue); >> + >> + I915_WRITE(PREC_PAL_MULTI_SEG_DATA(pipe), word); >> + } >> + } >> +} >> + >> +static void icl_load_gamma_multi_segmented_lut(const struct intel_crtc_state >> + *crtc_state, u32 offset) >> +{ >> + const struct drm_property_blob *gamma_mode_blob = >> + crtc_state->base.gamma_mode; >> + struct drm_color_lut *gamma_lut; >> + int ret; >> + >> + if (gamma_mode_blob) { >> + struct drm_color_mode_lut *arg = gamma_mode_blob->data; >> + u64 __user *props_ptr = (u64 __user *)(unsigned long)(arg->lut); >> + >> + DRM_INFO("Gamma Mode=%s\n", arg->name); >> + gamma_lut = kmalloc((sizeof(struct drm_color_lut) * arg->count), >> + GFP_KERNEL); >> + ret = copy_from_user(gamma_lut, props_ptr, >> + sizeof(struct drm_color_lut) * arg->count); >> + } > >What's this doing here? This is to get the lut values in driver from userspace. The blob passed from user is struct drm_color_mode_lut { /* DRM_MODE_LUT_* */ __u32 flags; /* number of points on the curve */ __u32 count; /* Name of Gamma Mode */ char name[DRM_PROP_NAME_LEN]; /* Pointer to Lut elements */ __u64 lut; }; The lut values are passed as pointer in "lut", we are copying the same to kernel memory space here. >> + >> + icl_program_fine_segment_lut(crtc_state, gamma_lut, 0); >> + icl_program_coarse_segment_lut(crtc_state, gamma_lut, 0); } >> + >> static void icl_load_luts(const struct intel_crtc_state *crtc_state) >> { >> if (crtc_state->base.degamma_lut) >> glk_load_degamma_lut(crtc_state); >> >> - if (crtc_state_is_legacy_gamma(crtc_state)) >> + if (crtc_state_is_legacy_gamma(crtc_state)) { >> i9xx_load_luts(crtc_state); >> - else >> + } else if (crtc_state->base.gamma_mode_type == >> + MULTI_SEGMENTED_GAMMA_MODE_12BIT) { >> + icl_load_gamma_multi_segmented_lut(crtc_state, 0); >> + } else { >> /* ToDo: Add support for multi segment gamma LUT */ >> bdw_load_gamma_lut(crtc_state, 0); >> + } >> } >> >> static void cherryview_load_luts(const struct intel_crtc_state >> *crtc_state) @@ -1034,10 +1176,20 @@ static int glk_color_check(struct >intel_crtc_state *crtc_state) >> return 0; >> } >> >> -static u32 icl_gamma_mode(const struct intel_crtc_state *crtc_state) >> +static u32 icl_gamma_mode(struct intel_crtc_state *crtc_state) >> { >> + const struct drm_property_blob *gamma_mode_blob = >> + crtc_state->base.gamma_mode; >> + struct drm_color_mode_lut *arg; >> u32 gamma_mode = 0; >> >> + if (gamma_mode_blob) { >> + arg = gamma_mode_blob->data; >> + if (!strcmp(arg->name, "multi-segmented gamma")) >> + crtc_state->base.gamma_mode_type = >> + MULTI_SEGMENTED_GAMMA_MODE_12BIT; >> + } > >With the blob_enum this kind of stuff should disappear I think. I feel keeping 2 separate properties should be good as we can keep the capabilities immutable. And with the above structure userspace should be able to set any supported gamma_mode. Let me know your opinion on the same. Regards, Uma Shankar >> + >> if (crtc_state->base.degamma_lut) >> gamma_mode |= PRE_CSC_GAMMA_ENABLE; >> >> @@ -1048,6 +1200,9 @@ static u32 icl_gamma_mode(const struct intel_crtc_state >*crtc_state) >> if (!crtc_state->base.gamma_lut || >> crtc_state_is_legacy_gamma(crtc_state)) >> gamma_mode |= GAMMA_MODE_MODE_8BIT; >> + else if (crtc_state->base.gamma_mode_type == >> + MULTI_SEGMENTED_GAMMA_MODE_12BIT) >> + gamma_mode |= >GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED; >> else >> gamma_mode |= GAMMA_MODE_MODE_10BIT; >> >> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index >> bc8a2e7..a8d0b4c 100644 >> --- a/include/drm/drm_crtc.h >> +++ b/include/drm/drm_crtc.h >> @@ -262,6 +262,9 @@ struct drm_crtc_state { >> */ >> struct drm_property_blob *gamma_mode; >> >> + /* Gamma mode type programmed on the pipe */ >> + u32 gamma_mode_type; >> + >> /** >> * @degamma_lut: >> * >> -- >> 1.9.1 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx > >-- >Ville Syrjälä >Intel _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
