[Intel-gfx] [PATCH v9 04/10] drm/i915/dsb: Indexed register write function for DSB.
Animesh Manna
animesh.manna at intel.com
Mon Sep 23 09:13:44 UTC 2019
On 9/23/2019 1:05 PM, Jani Nikula wrote:
> On Fri, 20 Sep 2019, Animesh Manna <animesh.manna at intel.com> wrote:
>> On 9/20/2019 5:48 PM, Jani Nikula wrote:
>>> On Fri, 20 Sep 2019, Animesh Manna <animesh.manna at intel.com> 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 at intel.com>
>>>> Cc: Imre Deak <imre.deak at intel.com>
>>>> Cc: Jani Nikula <jani.nikula at intel.com>
>>>> Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
>>>> Reviewed-by: Shashank Sharma <shashank.sharma at intel.com>
>>>> Signed-off-by: Animesh Manna <animesh.manna at intel.com>
>>>> ---
>>>> 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.
Thanks Jani and everyone who helped in review.
>
> I think you should be able to look at the contents of the buffer and
> decide based on that.
Yes, got the idea.
Regards,
Animesh
>
> 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 at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
More information about the Intel-gfx
mailing list