[Intel-gfx] [PATCH v4 08/10] drm/i915/dsb: Enable gamma lut programming using DSB.
Animesh Manna
animesh.manna at intel.com
Tue Sep 3 11:05:38 UTC 2019
Hi,
On 9/3/2019 1:38 PM, Jani Nikula wrote:
> On Fri, 30 Aug 2019, Jani Nikula <jani.nikula at intel.com> wrote:
>> On Fri, 30 Aug 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.
>>>
>>> v1: Initial version.
>>> v2: Directly call dsb-api at callsites. (Jani)
>>>
>>> Cc: Jani Nikula <jani.nikula at intel.com>
>>> Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
>>> Signed-off-by: Animesh Manna <animesh.manna at intel.com>
>>> ---
>>> drivers/gpu/drm/i915/display/intel_color.c | 64 ++++++++++++++--------
>>> drivers/gpu/drm/i915/i915_drv.h | 2 +
>>> 2 files changed, 43 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
>>> index 71a0201437a9..e4090d8e4547 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_color.c
>>> +++ b/drivers/gpu/drm/i915/display/intel_color.c
>>> @@ -589,34 +589,38 @@ static void bdw_load_lut_10(struct intel_crtc *crtc,
>>> const struct drm_color_lut *lut = blob->data;
>>> int i, lut_size = drm_color_lut_size(blob);
>>> enum pipe pipe = crtc->pipe;
>>> + struct intel_dsb *dsb = dev_priv->dsb;
>> No. Don't put dsb in dev_priv. You have 12 DSB engines, 3 per pipe. You
>> have intel_crtc for that.
>>
>> Was this not clear from my previous review?
>>
>> You have tons of functions here that will never have a DSB engine, it
>> makes no sense to convert all of them to use the DSB.
> To clarify:
>
> All functions that could and should use the DSB, need to be converted to
> use intel_dsb_write and friends. That means replacing I915_WRITE with
> the relevant intel_dsb_write calls. *NOT* having if (dsb)
> intel_dsb_write else I915_WRITE. As agreed a long time ago,
> intel_dsb_write should handle the non-DSB cases by falling back to
> regular intel_uncore_write.
>
> My point is, if there are functions that will never be called on a
> platform that has DSB, there's no point in converting them over to use
> DSB. Obviously there are e.g. legacy gamma functions that also get
> called on newer platforms.
>
> Maybe I was hasty in my assesment, need to double check if all of these
> paths are reachable from DSB platforms.
Currently multi-segmented gamma (12 bit gamma) is enabled by default for
icl+ platform.
Will it be fine to enable only 12 bit gamma through DSB and 8-bit/10-bit
can be enabled later based on need?
Regards,
Animesh
>
> BR,
> Jani.
>
>
>>>
>>> - I915_WRITE(PREC_PAL_INDEX(pipe), prec_index |
>>> - PAL_PREC_AUTO_INCREMENT);
>>> + intel_dsb_reg_write(dsb, PREC_PAL_INDEX(pipe),
>>> + prec_index | PAL_PREC_AUTO_INCREMENT);
>>>
>>> for (i = 0; i < hw_lut_size; i++) {
>>> /* We discard half the user entries in split gamma mode */
>>> const struct drm_color_lut *entry =
>>> &lut[i * (lut_size - 1) / (hw_lut_size - 1)];
>>>
>>> - I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_10(entry));
>>> + intel_dsb_indexed_reg_write(dsb, PREC_PAL_DATA(pipe),
>>> + ilk_lut_10(entry));
>>> }
>>>
>>> /*
>>> * Reset the index, otherwise it prevents the legacy palette to be
>>> * written properly.
>>> */
>>> - I915_WRITE(PREC_PAL_INDEX(pipe), 0);
>>> + intel_dsb_reg_write(dsb, PREC_PAL_INDEX(pipe), 0);
>>> }
>>>
>>> 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 = dev_priv->dsb;
>>> 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 +628,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);
>>> }
>>> }
>>>
>>> @@ -788,12 +795,13 @@ icl_load_gcmax(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);
>>> + struct intel_dsb *dsb = dev_priv->dsb;
>>> 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
>>> @@ -803,6 +811,7 @@ icl_program_gamma_superfine_segment(const struct intel_crtc_state *crtc_state)
>>> 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 = dev_priv->dsb;
>>> enum pipe pipe = crtc->pipe;
>>> u32 i;
>>>
>>> @@ -813,15 +822,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));
>>> }
>>> }
>>>
>>> @@ -833,6 +843,7 @@ icl_program_gamma_multi_segment(const struct intel_crtc_state *crtc_state)
>>> 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 = dev_priv->dsb;
>>> enum pipe pipe = crtc->pipe;
>>> u32 i;
>>>
>>> @@ -847,11 +858,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 +881,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 */
>>> @@ -980,7 +995,10 @@ void intel_color_load_luts(const struct intel_crtc_state *crtc_state)
>>> {
>>> struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
>>>
>>> + dev_priv->dsb = intel_dsb_get(to_intel_crtc(crtc_state->base.crtc));
>> No, don't do this. Don't store crtc specific info in a device global
>> structure.
>>
>>> dev_priv->display.load_luts(crtc_state);
>>> + intel_dsb_commit(dev_priv->dsb);
>>> + intel_dsb_put(dev_priv->dsb);
>> Please localize the use of DSB where needed. Move the gets and puts
>> within the relevant platform specific ->load_luts hooks.
>>
>>> }
>>>
>>> void intel_color_commit(const struct intel_crtc_state *crtc_state)
>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>>> index 7aa11e3dd413..98f546c4ad49 100644
>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>> @@ -1760,6 +1760,8 @@ struct drm_i915_private {
>>> /* Mutex to protect the above hdcp component related values. */
>>> struct mutex hdcp_comp_mutex;
>>>
>>> + struct intel_dsb *dsb;
>>> +
>>> /*
>>> * NOTE: This is the dri1/ums dungeon, don't add stuff here. Your patch
>>> * will be rejected. Instead look for a better place.
More information about the Intel-gfx
mailing list