[Intel-gfx] [PATCH v6 03/10] drm/i915/dsb: single register write function for DSB.

[Animesh Manna]

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 at 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 at intel.com>
>>>> Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
>>>> Cc: 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 | 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