[Intel-gfx] [PATCH v2 05/15] drm/i915/dsb: Indexed register write function for DSB.

[Animesh Manna]

Hi,


On 8/21/2019 11:57 PM, Chris Wilson wrote:
> Quoting Animesh Manna (2019-08-21 07:32:25)
>> DSB can program large set of data through indexed register write
>> (opcode 0x9) in one shot. Will be using for bulk register programming
>> e.g. gamma lut programming, HDR meta data programming.
>>
>> 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>
>> Signed-off-by: Animesh Manna <animesh.manna at intel.com>
>> ---
>>   drivers/gpu/drm/i915/display/intel_dsb.c | 42 ++++++++++++++++++++++++
>>   drivers/gpu/drm/i915/display/intel_dsb.h |  6 ++++
>>   2 files changed, 48 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c
>> index 8a9d082b1601..4fe8cac6246a 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dsb.c
>> +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
>> @@ -22,6 +22,7 @@
>>   #define DSB_OPCODE_INDEXED_WRITE       0x9
>>   #define DSB_OPCODE_POLL                        0xA
>>   #define DSB_BYTE_EN                    (0xf << 20)
>> +#define DSB_REG_VALUE_MASK             0xfffff
>>   
>>   struct intel_dsb *
>>   intel_dsb_get(struct intel_crtc *crtc)
>> @@ -79,6 +80,42 @@ intel_dsb_get(struct intel_crtc *crtc)
>>          return dsb;
>>   }
>>   
>> +static void intel_dsb_indexed_reg_write(struct intel_dsb *dsb, i915_reg_t reg,
>> +                                       u32 val)
>> +{
>> +       u32 *buf = dsb->cmd_buf;
>> +       u32 reg_val;
>> +
>> +       reg_val = buf[dsb->ins_start_offset + 1] & DSB_REG_VALUE_MASK;
> Uncached read.
>
>> +       if (reg_val != i915_mmio_reg_offset(reg)) {
>> +               /* Every instruction should be 8 byte aligned. */
>> +               if (dsb->free_pos & 0x1)
>> +                       dsb->free_pos++;
> dsb->free_pos = ALIGN(dsb->free_pos, 2);

Ok.
>
>> +
>> +               /* Update the size. */
>> +               dsb->ins_start_offset = dsb->free_pos;
>> +               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]++;
> Uncached read and write. So far this is working out to be _more_
> expensive than mmio.

Understood your concern, currently tried to align with I915_WRITE() call 
and coded accordingly.
If we can introduce a separate call for auto-increment register like below.

I915_WRITE_BULK(<reg-offset>, <buf-start-address>, <count>);

The implementation will be simpler and more optimized.
Good to know your feedback or any better way can be done.

Regards,
Animesh
> -Chris