On Fri, 20 Sep 2019, Animesh Manna <animesh.manna@xxxxxxxxx> wrote:
> On 9/20/2019 5:48 PM, Jani Nikula wrote:
>> On Fri, 20 Sep 2019, Animesh Manna <animesh.manna@xxxxxxxxx> wrote:
>>> DSB can program large set of data through indexed register write
>>> (opcode 0x9) in one shot. DSB feature can be used for bulk register
>>> programming e.g. gamma lut programming, HDR meta data programming.
>>>
>>> v1: initial version.
>>> v2: simplified code by using ALIGN(). (Chris)
>>> v3: ascii table added as code comment. (Shashank)
>>> v4: cosmetic changes done. (Shashank)
>>> v5: reset ins_start_offset. (Jani)
>>> v6: update ins_start_offset in inel_dsb_reg_write.
>>>
>>> Cc: Shashank Sharma <shashank.sharma@xxxxxxxxx>
>>> Cc: Imre Deak <imre.deak@xxxxxxxxx>
>>> Cc: Jani Nikula <jani.nikula@xxxxxxxxx>
>>> Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
>>> Reviewed-by: Shashank Sharma <shashank.sharma@xxxxxxxxx>
>>> Signed-off-by: Animesh Manna <animesh.manna@xxxxxxxxx>
>>> ---
>>> drivers/gpu/drm/i915/display/intel_dsb.c | 68 ++++++++++++++++++++++++
>>> drivers/gpu/drm/i915/display/intel_dsb.h | 9 ++++
>>> 2 files changed, 77 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c
>>> index f94cd6dc98b6..faa853b08458 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_dsb.c
>>> +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
>>> @@ -12,8 +12,10 @@
>>> /* 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
>>> +#define DSB_REG_VALUE_MASK 0xfffff
>>>
>>> struct intel_dsb *
>>> intel_dsb_get(struct intel_crtc *crtc)
>>> @@ -83,9 +85,74 @@ void intel_dsb_put(struct intel_dsb *dsb)
>>> mutex_unlock(&i915->drm.struct_mutex);
>>> dsb->cmd_buf = NULL;
>>> dsb->free_pos = 0;
>>> + dsb->ins_start_offset = 0;
>>> }
>>> }
>>>
>>> +void intel_dsb_indexed_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;
>>> + u32 reg_val;
>>> +
>>> + if (!buf) {
>>> + I915_WRITE(reg, val);
>>> + return;
>>> + }
>>> +
>>> + if (WARN_ON(dsb->free_pos >= DSB_BUF_SIZE)) {
>>> + DRM_DEBUG_KMS("DSB buffer overflow\n");
>>> + return;
>>> + }
>>> +
>>> + /*
>>> + * For example the buffer will look like below for 3 dwords for auto
>>> + * increment register:
>>> + * +--------------------------------------------------------+
>>> + * | size = 3 | offset &| value1 | value2 | value3 | zero |
>>> + * | | opcode | | | | |
>>> + * +--------------------------------------------------------+
>>> + * + + + + + + +
>>> + * 0 4 8 12 16 20 24
>>> + * Byte
>>> + *
>>> + * As every instruction is 8 byte aligned the index of dsb instruction
>>> + * will start always from even number while dealing with u32 array. If
>>> + * we are writing odd no of dwords, Zeros will be added in the end for
>>> + * padding.
>>> + */
>>> + reg_val = buf[dsb->ins_start_offset + 1] & DSB_REG_VALUE_MASK;
>>> + if (reg_val != i915_mmio_reg_offset(reg)) {
>>> + /* Every instruction should be 8 byte aligned. */
>>> + dsb->free_pos = ALIGN(dsb->free_pos, 2);
>>> +
>>> + dsb->ins_start_offset = dsb->free_pos;
>>> +
>>> + /* Update the size. */
>>> + buf[dsb->free_pos++] = 1;
>>> +
>>> + /* Update the opcode and reg. */
>>> + buf[dsb->free_pos++] = (DSB_OPCODE_INDEXED_WRITE <<
>>> + DSB_OPCODE_SHIFT) |
>>> + i915_mmio_reg_offset(reg);
>>> +
>>> + /* Update the value. */
>>> + buf[dsb->free_pos++] = val;
>>> + } else {
>>> + /* Update the new value. */
>>> + buf[dsb->free_pos++] = val;
>>> +
>>> + /* Update the size. */
>>> + buf[dsb->ins_start_offset]++;
>>> + }
>>> +
>>> + /* if number of data words is odd, then the last dword should be 0.*/
>>> + if (dsb->free_pos & 0x1)
>>> + buf[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);
>>> @@ -102,6 +169,7 @@ void intel_dsb_reg_write(struct intel_dsb *dsb, i915_reg_t reg, u32 val)
>>> return;
>>> }
>>>
>>> + dsb->ins_start_offset = dsb->free_pos;
>> Okay, I'm being a pedant, but that's kind of part of the job
>> description, I'm afraid.
>>
>> What if:
>>
>> intel_dsb_get()
>> intel_dsb_reg_write(dsb, FOO, 0);
>> intel_dsb_indexed_reg_write(dsb, FOO, 0);
>> intel_dsb_commit()
>> intel_dsb_put()
>
> Hi Jani,
>
> I am trying to think a scenario where may write the same register which
> is having auto-increment capability using both intel_dsb_reg_write and
> intel_dsb_indexed_reg_write.
> To set the auto increment mode we may need to write a different register
> to control auto-increment mode.
> If there is any practical scenario, do you want to me to add now?
It's not a likely scenario, I've pushed the series, thanks for the
patches and review.
I think you should be able to look at the contents of the buffer and
decide based on that.
BR,
Jani.
>
> Currently checking register value only while creating buffer for
> auto-increment register. If we want to add the above then we might need
> like below because we are introducing a variability factor regarding
> dsb-api to write the same register in consequent call.
>
> enum dsb_write_type {
> NORMAL,
> INDEX
> };
>
> struct last_write {
> u32 reg_val;
> enum dsb_write_type type;
> }
>
> then, last write can be updated in intel_dsb_reg_write() as NORMAL write
> and later will check in intel_dsb_indexed_reg_write() to identify above
> mentioned case.
> Please let me know your suggestion, will do accordingly.
>
> Regards,
> Animesh
>
>>
>> BR,
>> Jani.
>>
>>> buf[dsb->free_pos++] = val;
>>> buf[dsb->free_pos++] = (DSB_OPCODE_MMIO_WRITE << DSB_OPCODE_SHIFT) |
>>> (DSB_BYTE_EN << DSB_BYTE_EN_SHIFT) |
>>> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.h b/drivers/gpu/drm/i915/display/intel_dsb.h
>>> index 0686d67b34d5..2ae22f7309a7 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_dsb.h
>>> +++ b/drivers/gpu/drm/i915/display/intel_dsb.h
>>> @@ -30,11 +30,20 @@ struct intel_dsb {
>>> * and help in calculating tail of command buffer.
>>> */
>>> int free_pos;
>>> +
>>> + /*
>>> + * ins_start_offset will help to store start address of the dsb
>>> + * instuction and help in identifying the batch of auto-increment
>>> + * register.
>>> + */
>>> + u32 ins_start_offset;
>>> };
>>>
>>> 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);
>>> +void intel_dsb_indexed_reg_write(struct intel_dsb *dsb, i915_reg_t reg,
>>> + u32 val);
>>>
>>> #endif
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
