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

[Sharma, Shashank]

On 9/12/2019 6:37 PM, Animesh Manna wrote:
>
>
> 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

Yeah, broken this way or other, better to warn as soon as possible.

With the above macro comment fixed,

Please feel free to use Reviewed-by: Shashank Sharma 
<shashank.sharma at intel.com>

- Shashank

>
>>
>> - 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
>