On Fri, 30 Aug 2019, Jani Nikula <jani.nik...@intel.com> wrote: > On Fri, 30 Aug 2019, Animesh Manna <animesh.ma...@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.nik...@intel.com> >> Cc: Rodrigo Vivi <rodrigo.v...@intel.com> >> Signed-off-by: Animesh Manna <animesh.ma...@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. 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. -- Jani Nikula, Intel Open Source Graphics Center _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx