| Message ID | 20190911191133.23383-4-animesh.manna@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | DSB enablement. | expand |
On 9/12/2019 12:41 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. > > v1: Initial version. > v2: Unused macro removed and cosmetic changes done. (Shashank) > v3: set free_pos to zero in dsb-put() instead dsb-get() and > a cosmetic change. (Shashank) > > Cc: Jani Nikula <jani.nikula@intel.com> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Cc: Shashank Sharma <shashank.sharma@intel.com> > Signed-off-by: Animesh Manna <animesh.manna@intel.com> > --- > drivers/gpu/drm/i915/display/intel_dsb.c | 30 ++++++++++++++++++++++++ > drivers/gpu/drm/i915/display/intel_dsb.h | 9 +++++++ > 2 files changed, 39 insertions(+) > > diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c > index 7c1b1574788c..e2c383352145 100644 > --- a/drivers/gpu/drm/i915/display/intel_dsb.c > +++ b/drivers/gpu/drm/i915/display/intel_dsb.c > @@ -9,6 +9,13 @@ > > #define DSB_BUF_SIZE (2 * PAGE_SIZE) > > +/* DSB opcodes. */ > +#define DSB_OPCODE_SHIFT 24 > +#define DSB_OPCODE_MMIO_WRITE 0x1 > +#define DSB_OPCODE_INDEXED_WRITE 0x9 We are not using this macro here, this should go to the Batch /INDEXED_WRITE patch. > +#define DSB_BYTE_EN 0xF > +#define DSB_BYTE_EN_SHIFT 20 > + > struct intel_dsb * > intel_dsb_get(struct intel_crtc *crtc) > { > @@ -66,5 +73,28 @@ void intel_dsb_put(struct intel_dsb *dsb) > i915_vma_unpin_and_release(&dsb->vma, 0); > mutex_unlock(&i915->drm.struct_mutex); > dsb->cmd_buf = NULL; > + dsb->free_pos = 0; > + } > +} > + I hope this addition of braces are due to diff's adjustment. > +void intel_dsb_reg_write(struct intel_dsb *dsb, i915_reg_t reg, u32 val) > +{ > + struct intel_crtc *crtc = container_of(dsb, typeof(*crtc), dsb); > + 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"); why shouldn't we do a I915_WRITE(reg, val) here too? This is single register write, and we can handle this. > + return; > } > + > + buf[dsb->free_pos++] = val; > + buf[dsb->free_pos++] = (DSB_OPCODE_MMIO_WRITE << DSB_OPCODE_SHIFT) | > + (DSB_BYTE_EN << DSB_BYTE_EN_SHIFT) | > + 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 27eb68eb5392..31b87dcfe160 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; > > @@ -21,10 +23,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 tail of command buffer. > + */ > + 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
On Thu, 12 Sep 2019, "Sharma, Shashank" <shashank.sharma@intel.com> wrote: > On 9/12/2019 12:41 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. >> >> v1: Initial version. >> v2: Unused macro removed and cosmetic changes done. (Shashank) >> v3: set free_pos to zero in dsb-put() instead dsb-get() and >> a cosmetic change. (Shashank) >> >> Cc: Jani Nikula <jani.nikula@intel.com> >> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> >> Cc: Shashank Sharma <shashank.sharma@intel.com> >> Signed-off-by: Animesh Manna <animesh.manna@intel.com> >> --- >> drivers/gpu/drm/i915/display/intel_dsb.c | 30 ++++++++++++++++++++++++ >> drivers/gpu/drm/i915/display/intel_dsb.h | 9 +++++++ >> 2 files changed, 39 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c >> index 7c1b1574788c..e2c383352145 100644 >> --- a/drivers/gpu/drm/i915/display/intel_dsb.c >> +++ b/drivers/gpu/drm/i915/display/intel_dsb.c >> @@ -9,6 +9,13 @@ >> >> #define DSB_BUF_SIZE (2 * PAGE_SIZE) >> >> +/* DSB opcodes. */ >> +#define DSB_OPCODE_SHIFT 24 >> +#define DSB_OPCODE_MMIO_WRITE 0x1 >> +#define DSB_OPCODE_INDEXED_WRITE 0x9 > We are not using this macro here, this should go to the Batch > /INDEXED_WRITE patch. >> +#define DSB_BYTE_EN 0xF >> +#define DSB_BYTE_EN_SHIFT 20 >> + >> struct intel_dsb * >> intel_dsb_get(struct intel_crtc *crtc) >> { >> @@ -66,5 +73,28 @@ void intel_dsb_put(struct intel_dsb *dsb) >> i915_vma_unpin_and_release(&dsb->vma, 0); >> mutex_unlock(&i915->drm.struct_mutex); >> dsb->cmd_buf = NULL; >> + dsb->free_pos = 0; >> + } >> +} >> + > I hope this addition of braces are due to diff's adjustment. >> +void intel_dsb_reg_write(struct intel_dsb *dsb, i915_reg_t reg, u32 val) >> +{ >> + struct intel_crtc *crtc = container_of(dsb, typeof(*crtc), dsb); >> + 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"); > why shouldn't we do a I915_WRITE(reg, val) here too? This is single > register write, and we can handle this. That would assume it's okay to directly mmio write this and the subsequent values, and write the batch already stored in the buffer afterwards. BR, Jani. >> + return; >> } >> + >> + buf[dsb->free_pos++] = val; >> + buf[dsb->free_pos++] = (DSB_OPCODE_MMIO_WRITE << DSB_OPCODE_SHIFT) | >> + (DSB_BYTE_EN << DSB_BYTE_EN_SHIFT) | >> + 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 27eb68eb5392..31b87dcfe160 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; >> >> @@ -21,10 +23,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 tail of command buffer. >> + */ >> + 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
On 9/12/2019 6:21 PM, Jani Nikula wrote: > On Thu, 12 Sep 2019, "Sharma, Shashank" <shashank.sharma@intel.com> wrote: >> On 9/12/2019 12:41 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. >>> >>> v1: Initial version. >>> v2: Unused macro removed and cosmetic changes done. (Shashank) >>> v3: set free_pos to zero in dsb-put() instead dsb-get() and >>> a cosmetic change. (Shashank) >>> >>> Cc: Jani Nikula <jani.nikula@intel.com> >>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> >>> Cc: Shashank Sharma <shashank.sharma@intel.com> >>> Signed-off-by: Animesh Manna <animesh.manna@intel.com> >>> --- >>> drivers/gpu/drm/i915/display/intel_dsb.c | 30 ++++++++++++++++++++++++ >>> drivers/gpu/drm/i915/display/intel_dsb.h | 9 +++++++ >>> 2 files changed, 39 insertions(+) >>> >>> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c >>> index 7c1b1574788c..e2c383352145 100644 >>> --- a/drivers/gpu/drm/i915/display/intel_dsb.c >>> +++ b/drivers/gpu/drm/i915/display/intel_dsb.c >>> @@ -9,6 +9,13 @@ >>> >>> #define DSB_BUF_SIZE (2 * PAGE_SIZE) >>> >>> +/* DSB opcodes. */ >>> +#define DSB_OPCODE_SHIFT 24 >>> +#define DSB_OPCODE_MMIO_WRITE 0x1 >>> +#define DSB_OPCODE_INDEXED_WRITE 0x9 >> We are not using this macro here, this should go to the Batch >> /INDEXED_WRITE patch. >>> +#define DSB_BYTE_EN 0xF >>> +#define DSB_BYTE_EN_SHIFT 20 >>> + >>> struct intel_dsb * >>> intel_dsb_get(struct intel_crtc *crtc) >>> { >>> @@ -66,5 +73,28 @@ void intel_dsb_put(struct intel_dsb *dsb) >>> i915_vma_unpin_and_release(&dsb->vma, 0); >>> mutex_unlock(&i915->drm.struct_mutex); >>> dsb->cmd_buf = NULL; >>> + dsb->free_pos = 0; >>> + } >>> +} >>> + >> I hope this addition of braces are due to diff's adjustment. >>> +void intel_dsb_reg_write(struct intel_dsb *dsb, i915_reg_t reg, u32 val) >>> +{ >>> + struct intel_crtc *crtc = container_of(dsb, typeof(*crtc), dsb); >>> + 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"); >> why shouldn't we do a I915_WRITE(reg, val) here too? This is single >> register write, and we can handle this. > That would assume it's okay to directly mmio write this and the > subsequent values, and write the batch already stored in the buffer > afterwards. This is single write API, so I won't expect this to be called in an indexed context, also, we have exceeded the buffer size already, so no subsequent DSB write would be possible anyways. But I can still see it would be some kind of mess only, so doesn't really matter if we do this I915_write or not :). - Shashank > BR, > Jani. > >>> + return; >>> } >>> + >>> + buf[dsb->free_pos++] = val; >>> + buf[dsb->free_pos++] = (DSB_OPCODE_MMIO_WRITE << DSB_OPCODE_SHIFT) | >>> + (DSB_BYTE_EN << DSB_BYTE_EN_SHIFT) | >>> + 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 27eb68eb5392..31b87dcfe160 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; >>> >>> @@ -21,10 +23,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 tail of command buffer. >>> + */ >>> + 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
On 9/12/2019 6:30 PM, Sharma, Shashank wrote: > > On 9/12/2019 6:21 PM, Jani Nikula wrote: >> On Thu, 12 Sep 2019, "Sharma, Shashank" <shashank.sharma@intel.com> >> wrote: >>> On 9/12/2019 12:41 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. >>>> >>>> v1: Initial version. >>>> v2: Unused macro removed and cosmetic changes done. (Shashank) >>>> v3: set free_pos to zero in dsb-put() instead dsb-get() and >>>> a cosmetic change. (Shashank) >>>> >>>> Cc: Jani Nikula <jani.nikula@intel.com> >>>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> >>>> Cc: Shashank Sharma <shashank.sharma@intel.com> >>>> Signed-off-by: Animesh Manna <animesh.manna@intel.com> >>>> --- >>>> drivers/gpu/drm/i915/display/intel_dsb.c | 30 >>>> ++++++++++++++++++++++++ >>>> drivers/gpu/drm/i915/display/intel_dsb.h | 9 +++++++ >>>> 2 files changed, 39 insertions(+) >>>> >>>> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c >>>> b/drivers/gpu/drm/i915/display/intel_dsb.c >>>> index 7c1b1574788c..e2c383352145 100644 >>>> --- a/drivers/gpu/drm/i915/display/intel_dsb.c >>>> +++ b/drivers/gpu/drm/i915/display/intel_dsb.c >>>> @@ -9,6 +9,13 @@ >>>> #define DSB_BUF_SIZE (2 * PAGE_SIZE) >>>> +/* DSB opcodes. */ >>>> +#define DSB_OPCODE_SHIFT 24 >>>> +#define DSB_OPCODE_MMIO_WRITE 0x1 >>>> +#define DSB_OPCODE_INDEXED_WRITE 0x9 >>> We are not using this macro here, this should go to the Batch >>> /INDEXED_WRITE patch. >>>> +#define DSB_BYTE_EN 0xF >>>> +#define DSB_BYTE_EN_SHIFT 20 >>>> + >>>> struct intel_dsb * >>>> intel_dsb_get(struct intel_crtc *crtc) >>>> { >>>> @@ -66,5 +73,28 @@ void intel_dsb_put(struct intel_dsb *dsb) >>>> i915_vma_unpin_and_release(&dsb->vma, 0); >>>> mutex_unlock(&i915->drm.struct_mutex); >>>> dsb->cmd_buf = NULL; >>>> + dsb->free_pos = 0; >>>> + } >>>> +} >>>> + >>> I hope this addition of braces are due to diff's adjustment. >>>> +void intel_dsb_reg_write(struct intel_dsb *dsb, i915_reg_t reg, >>>> u32 val) >>>> +{ >>>> + struct intel_crtc *crtc = container_of(dsb, typeof(*crtc), dsb); >>>> + 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"); >>> why shouldn't we do a I915_WRITE(reg, val) here too? This is single >>> register write, and we can handle this. >> That would assume it's okay to directly mmio write this and the >> subsequent values, and write the batch already stored in the buffer >> afterwards. > > This is single write API, so I won't expect this to be called in an > indexed context, also, we have exceeded the buffer size already, so no > subsequent DSB write would be possible anyways. But I can still see it > would be some kind of mess only, so doesn't really matter if we do > this I915_write or not :). Adding I915_WRITE can be done, but I feel better to break the functionality where we have buffer overflow and based on that we can fine tune the buffer size. If a set of register is targetted to write through DSB then some writing through MMIO and and rest writing though DSB may not a nice thing. So added only debug log to capture the issue. Regards, Animesh > > - Shashank > >> BR, >> Jani. >> >>>> + return; >>>> } >>>> + >>>> + buf[dsb->free_pos++] = val; >>>> + buf[dsb->free_pos++] = (DSB_OPCODE_MMIO_WRITE << >>>> DSB_OPCODE_SHIFT) | >>>> + (DSB_BYTE_EN << DSB_BYTE_EN_SHIFT) | >>>> + 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 27eb68eb5392..31b87dcfe160 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; >>>> @@ -21,10 +23,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 tail of command buffer. >>>> + */ >>>> + 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
On 9/12/2019 6:37 PM, Animesh Manna wrote: > > > On 9/12/2019 6:30 PM, Sharma, Shashank wrote: >> >> On 9/12/2019 6:21 PM, Jani Nikula wrote: >>> On Thu, 12 Sep 2019, "Sharma, Shashank" <shashank.sharma@intel.com> >>> wrote: >>>> On 9/12/2019 12:41 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. >>>>> >>>>> v1: Initial version. >>>>> v2: Unused macro removed and cosmetic changes done. (Shashank) >>>>> v3: set free_pos to zero in dsb-put() instead dsb-get() and >>>>> a cosmetic change. (Shashank) >>>>> >>>>> Cc: Jani Nikula <jani.nikula@intel.com> >>>>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> >>>>> Cc: Shashank Sharma <shashank.sharma@intel.com> >>>>> Signed-off-by: Animesh Manna <animesh.manna@intel.com> >>>>> --- >>>>> drivers/gpu/drm/i915/display/intel_dsb.c | 30 >>>>> ++++++++++++++++++++++++ >>>>> drivers/gpu/drm/i915/display/intel_dsb.h | 9 +++++++ >>>>> 2 files changed, 39 insertions(+) >>>>> >>>>> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c >>>>> b/drivers/gpu/drm/i915/display/intel_dsb.c >>>>> index 7c1b1574788c..e2c383352145 100644 >>>>> --- a/drivers/gpu/drm/i915/display/intel_dsb.c >>>>> +++ b/drivers/gpu/drm/i915/display/intel_dsb.c >>>>> @@ -9,6 +9,13 @@ >>>>> #define DSB_BUF_SIZE (2 * PAGE_SIZE) >>>>> +/* DSB opcodes. */ >>>>> +#define DSB_OPCODE_SHIFT 24 >>>>> +#define DSB_OPCODE_MMIO_WRITE 0x1 >>>>> +#define DSB_OPCODE_INDEXED_WRITE 0x9 >>>> We are not using this macro here, this should go to the Batch >>>> /INDEXED_WRITE patch. >>>>> +#define DSB_BYTE_EN 0xF >>>>> +#define DSB_BYTE_EN_SHIFT 20 >>>>> + >>>>> struct intel_dsb * >>>>> intel_dsb_get(struct intel_crtc *crtc) >>>>> { >>>>> @@ -66,5 +73,28 @@ void intel_dsb_put(struct intel_dsb *dsb) >>>>> i915_vma_unpin_and_release(&dsb->vma, 0); >>>>> mutex_unlock(&i915->drm.struct_mutex); >>>>> dsb->cmd_buf = NULL; >>>>> + dsb->free_pos = 0; >>>>> + } >>>>> +} >>>>> + >>>> I hope this addition of braces are due to diff's adjustment. >>>>> +void intel_dsb_reg_write(struct intel_dsb *dsb, i915_reg_t reg, >>>>> u32 val) >>>>> +{ >>>>> + struct intel_crtc *crtc = container_of(dsb, typeof(*crtc), dsb); >>>>> + 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"); >>>> why shouldn't we do a I915_WRITE(reg, val) here too? This is single >>>> register write, and we can handle this. >>> That would assume it's okay to directly mmio write this and the >>> subsequent values, and write the batch already stored in the buffer >>> afterwards. >> >> This is single write API, so I won't expect this to be called in an >> indexed context, also, we have exceeded the buffer size already, so >> no subsequent DSB write would be possible anyways. But I can still >> see it would be some kind of mess only, so doesn't really matter if >> we do this I915_write or not :). > > Adding I915_WRITE can be done, but I feel better to break the > functionality where we have buffer overflow and based on that we can > fine tune the buffer size. > If a set of register is targetted to write through DSB then some > writing through MMIO and and rest writing though DSB may not a nice > thing. > So added only debug log to capture the issue. > > Regards, > Animesh Yeah, broken this way or other, better to warn as soon as possible. With the above macro comment fixed, Please feel free to use Reviewed-by: Shashank Sharma <shashank.sharma@intel.com> - Shashank > >> >> - Shashank >> >>> BR, >>> Jani. >>> >>>>> + return; >>>>> } >>>>> + >>>>> + buf[dsb->free_pos++] = val; >>>>> + buf[dsb->free_pos++] = (DSB_OPCODE_MMIO_WRITE << >>>>> DSB_OPCODE_SHIFT) | >>>>> + (DSB_BYTE_EN << DSB_BYTE_EN_SHIFT) | >>>>> + 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 27eb68eb5392..31b87dcfe160 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; >>>>> @@ -21,10 +23,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 tail of command buffer. >>>>> + */ >>>>> + 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 7c1b1574788c..e2c383352145 100644 --- a/drivers/gpu/drm/i915/display/intel_dsb.c +++ b/drivers/gpu/drm/i915/display/intel_dsb.c @@ -9,6 +9,13 @@ #define DSB_BUF_SIZE (2 * PAGE_SIZE) +/* DSB opcodes. */ +#define DSB_OPCODE_SHIFT 24 +#define DSB_OPCODE_MMIO_WRITE 0x1 +#define DSB_OPCODE_INDEXED_WRITE 0x9 +#define DSB_BYTE_EN 0xF +#define DSB_BYTE_EN_SHIFT 20 + struct intel_dsb * intel_dsb_get(struct intel_crtc *crtc) { @@ -66,5 +73,28 @@ void intel_dsb_put(struct intel_dsb *dsb) i915_vma_unpin_and_release(&dsb->vma, 0); mutex_unlock(&i915->drm.struct_mutex); dsb->cmd_buf = NULL; + dsb->free_pos = 0; + } +} + +void intel_dsb_reg_write(struct intel_dsb *dsb, i915_reg_t reg, u32 val) +{ + struct intel_crtc *crtc = container_of(dsb, typeof(*crtc), dsb); + 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 << DSB_BYTE_EN_SHIFT) | + 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 27eb68eb5392..31b87dcfe160 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; @@ -21,10 +23,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 tail of command buffer. + */ + 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. v1: Initial version. v2: Unused macro removed and cosmetic changes done. (Shashank) v3: set free_pos to zero in dsb-put() instead dsb-get() and a cosmetic change. (Shashank) Cc: Jani Nikula <jani.nikula@intel.com> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> Cc: Shashank Sharma <shashank.sharma@intel.com> Signed-off-by: Animesh Manna <animesh.manna@intel.com> --- drivers/gpu/drm/i915/display/intel_dsb.c | 30 ++++++++++++++++++++++++ drivers/gpu/drm/i915/display/intel_dsb.h | 9 +++++++ 2 files changed, 39 insertions(+)