> -----Original Message----- > From: Intel-gfx [mailto:intel-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx] On Behalf Of Jani > Nikula > Sent: Friday, August 30, 2019 7:02 PM > To: Manna, Animesh <animesh.manna@xxxxxxxxx>; intel-gfx@xxxxxxxxxxxxxxxxxxxxx > Subject: Re: [PATCH v4 08/10] drm/i915/dsb: Enable gamma lut > programming using DSB. > > On Fri, 30 Aug 2019, Animesh Manna <animesh.manna@xxxxxxxxx> 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@xxxxxxxxx> > > Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx> > > Signed-off-by: Animesh Manna <animesh.manna@xxxxxxxxx> > > --- > > 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. > Jani, I was thinking even among the 3 engines available per pipe, would it make more sense to keep them reserve on the basis of user ? like DSB0 for all pipe operations, DSB1 for all plane, and DSB2 for all encoder related stuff. We can create a DSB user (like we have for scaler) and index these engines based on that. Do you think so ? > > > > - 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. > > -- > Jani Nikula, Intel Open Source Graphics Center > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx > https://lists.freedesktop.org/mailman/listinfo/intel-gfx _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
