| Message ID | 20190827191026.26175-4-animesh.manna@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | DSB enablement. | expand |
On 8/28/2019 12:40 AM, Animesh Manna wrote: > DSB support single register write through opcode 0x1. Generic > api created which accumulate all single register write in a batch > buffer and once DSB is triggered, it will program all the registers > at the same time. > > Cc: Jani Nikula <jani.nikula@intel.com> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Signed-off-by: Animesh Manna <animesh.manna@intel.com> > --- > drivers/gpu/drm/i915/display/intel_dsb.c | 36 ++++++++++++++++++++++++ > drivers/gpu/drm/i915/display/intel_dsb.h | 9 ++++++ > 2 files changed, 45 insertions(+) > > diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c > index a2845df90573..df288446caeb 100644 > --- a/drivers/gpu/drm/i915/display/intel_dsb.c > +++ b/drivers/gpu/drm/i915/display/intel_dsb.c > @@ -9,6 +9,20 @@ > > #define DSB_BUF_SIZE (2 * PAGE_SIZE) > > +/* DSB opcodes. */ > +#define DSB_OPCODE_SHIFT 24 > +#define DSB_OPCODE_NOOP 0x0 > +#define DSB_OPCODE_MMIO_WRITE 0x1 > +#define DSB_OPCODE_WAIT_FOR_US 0x2 > +#define DSB_OPCODE_WAIT_FOR_LINES 0x3 > +#define DSB_OPCODE_WAIT_FOR_VBLANK 0x4 > +#define DSB_OPCODE_WAIT_FOR_SL_IN 0x5 May be SL_IR (to indicate in range) > +#define DSB_OPCODE_WAIT_FOR_SL_OUT 0x6 May be SL_OOR (out of range) or _OR > +#define DSB_OPCODE_GENERATE_INT 0x7 > +#define DSB_OPCODE_INDEXED_WRITE 0x9 > +#define DSB_OPCODE_POLL 0xA > +#define DSB_BYTE_EN (0xf << 20) But its a nitpick but as we are going by shifts for OPCODE, may be would look consistent to create 2 different macros for enable also. #define DSB_BYTE_EN 0xF #define DSB_BYTE_EN_SHIFT 20 > + This patch is using only 3 of the defined commands: DSB_OPCODE_MMIO_WRITE, DSB_OPCODE_SHIFT, DSB_BYTE_EN All other definitions are unused. Please add them only in the patch where they are being used. > struct intel_dsb * > intel_dsb_get(struct intel_crtc *crtc) > { > @@ -81,3 +95,25 @@ void intel_dsb_put(struct intel_dsb *dsb) > mutex_unlock(&i915->drm.struct_mutex); > } > } > + > +void intel_dsb_reg_write(struct intel_dsb *dsb, i915_reg_t reg, u32 val) > +{ > + struct intel_crtc *crtc = dsb->crtc; > + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > + u32 *buf = dsb->cmd_buf; > + Do we need a HAS_DSB() check here ? Or would it be taken care in the caller ? > + if (!buf) { > + I915_WRITE(reg, val); We can debate on if this is a good idea to fallback to I915_write() if DSB is not available. May be we should think about why are we seeing this condition (!buf), and that can be checked only when I see the caller. For now, lets assume we will fallback to I915_WRITE(); > + return; > + } > + > + if (WARN_ON(dsb->free_pos >= DSB_BUF_SIZE)) { > + DRM_DEBUG_KMS("DSB buffer overflow.\n"); Wouldn't it make sense to do a I915_WRITE(reg, val) here also, like above ? > + return; > + } > + > + buf[dsb->free_pos++] = val; > + buf[dsb->free_pos++] = (DSB_OPCODE_MMIO_WRITE << > + DSB_OPCODE_SHIFT) | DSB_BYTE_EN | > + i915_mmio_reg_offset(reg); Once the write is done, are we planning to reset this free_pos ? May be in some upcoming patch in the series ? > +} > diff --git a/drivers/gpu/drm/i915/display/intel_dsb.h b/drivers/gpu/drm/i915/display/intel_dsb.h > index 4a4091cadc1e..1b33ab118640 100644 > --- a/drivers/gpu/drm/i915/display/intel_dsb.h > +++ b/drivers/gpu/drm/i915/display/intel_dsb.h > @@ -6,6 +6,8 @@ > #ifndef _INTEL_DSB_H > #define _INTEL_DSB_H > > +#include "i915_reg.h" > + > struct intel_crtc; > struct i915_vma; > > @@ -22,10 +24,17 @@ struct intel_dsb { > enum dsb_id id; > u32 *cmd_buf; > struct i915_vma *vma; > + > + /* > + * free_pos will point the first free entry position > + * and help in calculating cmd_buf_tail. > + */ > + int free_pos; > }; > > struct intel_dsb * > intel_dsb_get(struct intel_crtc *crtc); > void intel_dsb_put(struct intel_dsb *dsb); > +void intel_dsb_reg_write(struct intel_dsb *dsb, i915_reg_t reg, u32 val); > > #endif
Hi, On 8/28/2019 8:46 PM, Sharma, Shashank wrote: > > On 8/28/2019 12:40 AM, Animesh Manna wrote: >> DSB support single register write through opcode 0x1. Generic >> api created which accumulate all single register write in a batch >> buffer and once DSB is triggered, it will program all the registers >> at the same time. >> >> Cc: Jani Nikula <jani.nikula@intel.com> >> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> >> Signed-off-by: Animesh Manna <animesh.manna@intel.com> >> --- >> drivers/gpu/drm/i915/display/intel_dsb.c | 36 ++++++++++++++++++++++++ >> drivers/gpu/drm/i915/display/intel_dsb.h | 9 ++++++ >> 2 files changed, 45 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c >> b/drivers/gpu/drm/i915/display/intel_dsb.c >> index a2845df90573..df288446caeb 100644 >> --- a/drivers/gpu/drm/i915/display/intel_dsb.c >> +++ b/drivers/gpu/drm/i915/display/intel_dsb.c >> @@ -9,6 +9,20 @@ >> #define DSB_BUF_SIZE (2 * PAGE_SIZE) >> +/* DSB opcodes. */ >> +#define DSB_OPCODE_SHIFT 24 >> +#define DSB_OPCODE_NOOP 0x0 >> +#define DSB_OPCODE_MMIO_WRITE 0x1 >> +#define DSB_OPCODE_WAIT_FOR_US 0x2 >> +#define DSB_OPCODE_WAIT_FOR_LINES 0x3 >> +#define DSB_OPCODE_WAIT_FOR_VBLANK 0x4 >> +#define DSB_OPCODE_WAIT_FOR_SL_IN 0x5 > May be SL_IR (to indicate in range) >> +#define DSB_OPCODE_WAIT_FOR_SL_OUT 0x6 > May be SL_OOR (out of range) or _OR >> +#define DSB_OPCODE_GENERATE_INT 0x7 >> +#define DSB_OPCODE_INDEXED_WRITE 0x9 >> +#define DSB_OPCODE_POLL 0xA >> +#define DSB_BYTE_EN (0xf << 20) > > But its a nitpick but as we are going by shifts for OPCODE, may be > would look consistent to create 2 different macros for enable also. > > #define DSB_BYTE_EN 0xF > > #define DSB_BYTE_EN_SHIFT 20 Ok. > > >> + > > This patch is using only 3 of the defined commands: > DSB_OPCODE_MMIO_WRITE, DSB_OPCODE_SHIFT, DSB_BYTE_EN > > All other definitions are unused. Please add them only in the patch > where they are being used. Ok. > >> struct intel_dsb * >> intel_dsb_get(struct intel_crtc *crtc) >> { >> @@ -81,3 +95,25 @@ void intel_dsb_put(struct intel_dsb *dsb) >> mutex_unlock(&i915->drm.struct_mutex); >> } >> } >> + >> +void intel_dsb_reg_write(struct intel_dsb *dsb, i915_reg_t reg, u32 >> val) >> +{ >> + struct intel_crtc *crtc = dsb->crtc; >> + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); >> + u32 *buf = dsb->cmd_buf; >> + > Do we need a HAS_DSB() check here ? Or would it be taken care in the > caller ? After passing from HAS_DSB() check will get the dsb->cmd_buf, so having non-null cmd_buf implies HAS_DSB is passed. >> + if (!buf) { >> + I915_WRITE(reg, val); > > We can debate on if this is a good idea to fallback to I915_write() if > DSB is not available. May be we should think about why are we seeing > this condition (!buf), and that can be checked only when I see the > caller. > > For now, lets assume we will fallback to I915_WRITE(); Having (!buf) check will help to identify platform which are not having DSB support. As per agreed design with Jani we want to replace I915_WRITE with dsp-write api for all platform and for older platform will fallback to i915_WRITE call. > >> + return; >> + } >> + >> + if (WARN_ON(dsb->free_pos >= DSB_BUF_SIZE)) { >> + DRM_DEBUG_KMS("DSB buffer overflow.\n"); > Wouldn't it make sense to do a I915_WRITE(reg, val) here also, like > above ? Wanted to be capture this error and don't want to pass through fallback mechanism. Based on need will fine tune DSB buffer size in future if needed. > >> + return; >> + } >> + >> + buf[dsb->free_pos++] = val; >> + buf[dsb->free_pos++] = (DSB_OPCODE_MMIO_WRITE << >> + DSB_OPCODE_SHIFT) | DSB_BYTE_EN | >> + i915_mmio_reg_offset(reg); > Once the write is done, are we planning to reset this free_pos ? May > be in some upcoming patch in the series ? Yes, after commit will be done. Regards, Animesh >> +} >> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.h >> b/drivers/gpu/drm/i915/display/intel_dsb.h >> index 4a4091cadc1e..1b33ab118640 100644 >> --- a/drivers/gpu/drm/i915/display/intel_dsb.h >> +++ b/drivers/gpu/drm/i915/display/intel_dsb.h >> @@ -6,6 +6,8 @@ >> #ifndef _INTEL_DSB_H >> #define _INTEL_DSB_H >> +#include "i915_reg.h" >> + >> struct intel_crtc; >> struct i915_vma; >> @@ -22,10 +24,17 @@ struct intel_dsb { >> enum dsb_id id; >> u32 *cmd_buf; >> struct i915_vma *vma; >> + >> + /* >> + * free_pos will point the first free entry position >> + * and help in calculating cmd_buf_tail. >> + */ >> + int free_pos; >> }; >> struct intel_dsb * >> intel_dsb_get(struct intel_crtc *crtc); >> void intel_dsb_put(struct intel_dsb *dsb); >> +void intel_dsb_reg_write(struct intel_dsb *dsb, i915_reg_t reg, u32 >> val); >> #endif
diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c index a2845df90573..df288446caeb 100644 --- a/drivers/gpu/drm/i915/display/intel_dsb.c +++ b/drivers/gpu/drm/i915/display/intel_dsb.c @@ -9,6 +9,20 @@ #define DSB_BUF_SIZE (2 * PAGE_SIZE) +/* DSB opcodes. */ +#define DSB_OPCODE_SHIFT 24 +#define DSB_OPCODE_NOOP 0x0 +#define DSB_OPCODE_MMIO_WRITE 0x1 +#define DSB_OPCODE_WAIT_FOR_US 0x2 +#define DSB_OPCODE_WAIT_FOR_LINES 0x3 +#define DSB_OPCODE_WAIT_FOR_VBLANK 0x4 +#define DSB_OPCODE_WAIT_FOR_SL_IN 0x5 +#define DSB_OPCODE_WAIT_FOR_SL_OUT 0x6 +#define DSB_OPCODE_GENERATE_INT 0x7 +#define DSB_OPCODE_INDEXED_WRITE 0x9 +#define DSB_OPCODE_POLL 0xA +#define DSB_BYTE_EN (0xf << 20) + struct intel_dsb * intel_dsb_get(struct intel_crtc *crtc) { @@ -81,3 +95,25 @@ void intel_dsb_put(struct intel_dsb *dsb) mutex_unlock(&i915->drm.struct_mutex); } } + +void intel_dsb_reg_write(struct intel_dsb *dsb, i915_reg_t reg, u32 val) +{ + struct intel_crtc *crtc = dsb->crtc; + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); + u32 *buf = dsb->cmd_buf; + + if (!buf) { + I915_WRITE(reg, val); + return; + } + + if (WARN_ON(dsb->free_pos >= DSB_BUF_SIZE)) { + DRM_DEBUG_KMS("DSB buffer overflow.\n"); + return; + } + + buf[dsb->free_pos++] = val; + buf[dsb->free_pos++] = (DSB_OPCODE_MMIO_WRITE << + DSB_OPCODE_SHIFT) | DSB_BYTE_EN | + i915_mmio_reg_offset(reg); +} diff --git a/drivers/gpu/drm/i915/display/intel_dsb.h b/drivers/gpu/drm/i915/display/intel_dsb.h index 4a4091cadc1e..1b33ab118640 100644 --- a/drivers/gpu/drm/i915/display/intel_dsb.h +++ b/drivers/gpu/drm/i915/display/intel_dsb.h @@ -6,6 +6,8 @@ #ifndef _INTEL_DSB_H #define _INTEL_DSB_H +#include "i915_reg.h" + struct intel_crtc; struct i915_vma; @@ -22,10 +24,17 @@ struct intel_dsb { enum dsb_id id; u32 *cmd_buf; struct i915_vma *vma; + + /* + * free_pos will point the first free entry position + * and help in calculating cmd_buf_tail. + */ + int free_pos; }; struct intel_dsb * intel_dsb_get(struct intel_crtc *crtc); void intel_dsb_put(struct intel_dsb *dsb); +void intel_dsb_reg_write(struct intel_dsb *dsb, i915_reg_t reg, u32 val); #endif
DSB support single register write through opcode 0x1. Generic api created which accumulate all single register write in a batch buffer and once DSB is triggered, it will program all the registers at the same time. Cc: Jani Nikula <jani.nikula@intel.com> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> Signed-off-by: Animesh Manna <animesh.manna@intel.com> --- drivers/gpu/drm/i915/display/intel_dsb.c | 36 ++++++++++++++++++++++++ drivers/gpu/drm/i915/display/intel_dsb.h | 9 ++++++ 2 files changed, 45 insertions(+)