[Intel-gfx] [PATCH v3 04/11] drm/i915/dsb: Indexed register write function for DSB.

[Animesh Manna]

Hi,


On 8/28/2019 10:16 PM, Sharma, Shashank wrote:
>
> On 8/28/2019 12:40 AM, Animesh Manna wrote:
>> DSB can program large set of data through indexed register write
>> (opcode 0x9) in one shot. Will be using for bulk register programming
> Reshuffle-> This feature can be used for bulk register programming e.g.

DSB can program large set of data through indexed register write
(opcode 0x9) in one shot. This feature can be used for bulk register
programming e.g. gamma lut programming, HDR meta data programming.

Hope it is fine now?

>
>> e.g. gamma lut programming, HDR meta data programming.
>>
>> v1: Initial version.
>>
>> v2: simplified code by using ALIGN(). (Chris)
>>
>> 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 | 48 ++++++++++++++++++++++++
>>   drivers/gpu/drm/i915/display/intel_dsb.h |  8 ++++
>>   2 files changed, 56 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c 
>> b/drivers/gpu/drm/i915/display/intel_dsb.c
>> index df288446caeb..520f2bbcc8ae 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)
>> @@ -96,6 +97,53 @@ void intel_dsb_put(struct intel_dsb *dsb)
>>       }
>>   }
>>   +void intel_dsb_indexed_reg_write(struct intel_dsb *dsb, i915_reg_t 
>> reg,
>> +                 u32 val)
> We might need one space here, to get this aligned to start of the line 
> above (or is this due to my mail client ?).

After applying the patch looks ok maybe some editor issue.

>> +{
>> +    struct intel_crtc *crtc = dsb->crtc;
>> +    struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>> +    u32 *buf = dsb->cmd_buf;
>> +    u32 reg_val;
>> +
> - Do we need a HAS_DSB check at start or the caller will take care of 
> it ?

As explained before non-null buf implies HAS_DSB check is passed.

>> +    if (!buf) {
>> +        I915_WRITE(reg, val);
> I am under the assumption that indexed reg write is to write multiple 
> registers, and 'reg' is the base value of the register set. I dont 
> think it makes sense to write a single register here in case of DSB 
> failure ?

No, here plan is to replace I915_WRITE with dsb-write and for that 
I915_WRITE is needed for platforms which does not support.

>> +        return;
>> +    }
>> +
>> +    if (WARN_ON(dsb->free_pos >= DSB_BUF_SIZE)) {
>> +        DRM_DEBUG_KMS("DSB buffer overflow.\n");
>> +        return;
>> +    }
>> +    reg_val = buf[dsb->ins_start_offset + 1] & DSB_REG_VALUE_MASK;
>
> Why do we have this +1 here ? on the very first run (when 
> ins_start_offset is 0), this will fetch reg_val = buf[1]; Is this what 
> we want to do ?

Yes reg offset is stored in buf[x+1] position and buf[x] is for size, 
where x is starting index of indexed-write-dsb-instruction.

>
>> +    if (reg_val != i915_mmio_reg_offset(reg)) {
>> +        /* Every instruction should be 8 byte aligned. */
>> +        dsb->free_pos = ALIGN(dsb->free_pos, 2);
> The comment says that you want the position to be 8 bytes align, but 
> the factor sent to the macro is 2 ?

Yes dsb->free_pos is a index of 4 byte array. The new instruction can 
start from 0, 2, 4, 6 ... etc.

>> +
>> +        /* Update the size. */
>> +        dsb->ins_start_offset = dsb->free_pos;
> This comment is irrelevant, you are not updating the size, you are 
> caching the base memory location.

Yes .. will fix.

>> +        buf[dsb->free_pos++] = 1;
>
> What does this indicate ? What is 1 ?
>
> It would be great if you can add an aski graph/table and explain what 
> does this DSB command buffer actually contains as per your design.

Yes, as per offline discussion will add in code-comment with ascii 
graph/table.

>
>
>> +
>> +        /* 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]++;
>> +    }
>
> The code is looking unnecessarily complicated as ins_start_offset and 
> free_pos are being used for multiple reasons. I guess you can use some 
> local variables like:
>
> u32 base = count = ALIGN();
>
>  {
>
>     buf [count++] = size;
>
>    buf [count++] = command;
>
>   buf [count++] = value;
>
> }
>
> dsb->start_offset = base;
>
> dsb->free_pos = count;

Here the whole buffer not getting in one shot.

>
>> +
>> +    /* 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 = dsb->crtc;
>> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.h 
>> b/drivers/gpu/drm/i915/display/intel_dsb.h
>> index 1b33ab118640..c848747f52d9 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dsb.h
>> +++ b/drivers/gpu/drm/i915/display/intel_dsb.h
>> @@ -30,11 +30,19 @@ struct intel_dsb {
>>        * and help in calculating cmd_buf_tail.
>>        */
>>       int free_pos;
>> +
>> +    /*
>> +     * ins_start_offset will help to store start address
>> +     * of the dsb instuction of auto-increment register.
>> +     */
>> +    u32 ins_start_offset;
> How about the variable to be named as base_offset ? Also, do we need 
> to keep this saved, even when the writing is done ?

Yes it is needed to compare the register value of the next write. As 
agreed offline we can have ins_start_offset as variable-name.

>>   };
>>     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);
>
> Cross check the alignment of second line.

Yes looks fine after applying the patch.

Regards,
Animesh

>
> - Shashank
>
>>     #endif