Re: [PATCH v3 03/11] drm/i915/dsb: single register write function for DSB.

[Animesh Manna <animesh.manna@intel.com>]

Hi,


On 8/28/2019 8:46 PM, Sharma, Shashank wrote:
>
> On 8/28/2019 12:40 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.
>>
>> Cc: Jani Nikula <jani.nikula@intel.com>
>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
>> ---
>>   drivers/gpu/drm/i915/display/intel_dsb.c | 36 ++++++++++++++++++++++++
>>   drivers/gpu/drm/i915/display/intel_dsb.h |  9 ++++++
>>   2 files changed, 45 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c 
>> b/drivers/gpu/drm/i915/display/intel_dsb.c
>> index a2845df90573..df288446caeb 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dsb.c
>> +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
>> @@ -9,6 +9,20 @@
>>     #define DSB_BUF_SIZE    (2 * PAGE_SIZE)
>>   +/* DSB opcodes. */
>> +#define DSB_OPCODE_SHIFT        24
>> +#define DSB_OPCODE_NOOP            0x0
>> +#define DSB_OPCODE_MMIO_WRITE        0x1
>> +#define DSB_OPCODE_WAIT_FOR_US        0x2
>> +#define DSB_OPCODE_WAIT_FOR_LINES    0x3
>> +#define DSB_OPCODE_WAIT_FOR_VBLANK    0x4
>> +#define DSB_OPCODE_WAIT_FOR_SL_IN    0x5
> May be SL_IR (to indicate in range)
>> +#define DSB_OPCODE_WAIT_FOR_SL_OUT    0x6
> May be SL_OOR (out of range) or _OR
>> +#define DSB_OPCODE_GENERATE_INT 0x7
>> +#define DSB_OPCODE_INDEXED_WRITE    0x9
>> +#define DSB_OPCODE_POLL            0xA
>> +#define DSB_BYTE_EN            (0xf << 20)
>
> But its a nitpick but as we are going by shifts for OPCODE, may be 
> would look consistent to create 2 different macros for enable also.
>
> #define DSB_BYTE_EN            0xF
>
> #define DSB_BYTE_EN_SHIFT        20

Ok.

>
>
>> +
>
> This patch is using only 3 of the defined commands: 
> DSB_OPCODE_MMIO_WRITE, DSB_OPCODE_SHIFT, DSB_BYTE_EN
>
> All other definitions are unused. Please add them only in the patch 
> where they are being used.

Ok.

>
>>   struct intel_dsb *
>>   intel_dsb_get(struct intel_crtc *crtc)
>>   {
>> @@ -81,3 +95,25 @@ void intel_dsb_put(struct intel_dsb *dsb)
>>           mutex_unlock(&i915->drm.struct_mutex);
>>       }
>>   }
>> +
>> +void intel_dsb_reg_write(struct intel_dsb *dsb, i915_reg_t reg, u32 
>> val)
>> +{
>> +    struct intel_crtc *crtc = dsb->crtc;
>> +    struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>> +    u32 *buf = dsb->cmd_buf;
>> +
> Do we need a HAS_DSB() check here ? Or would it be taken care in the 
> caller ?

After passing from HAS_DSB() check will get the dsb->cmd_buf, so having 
non-null cmd_buf implies HAS_DSB is passed.

>> +    if (!buf) {
>> +        I915_WRITE(reg, val);
>
> We can debate on if this is a good idea to fallback to I915_write() if 
> DSB is not available. May be we should think about why are we seeing 
> this condition (!buf), and that can be checked only when I see the 
> caller.
>
> For now, lets assume we will fallback to I915_WRITE();

Having (!buf) check will help to identify platform which are not having 
DSB support.
As per agreed design with Jani we want to replace I915_WRITE with 
dsp-write api for all platform and for older platform will fallback to 
i915_WRITE call.

>
>> +        return;
>> +    }
>> +
>> +    if (WARN_ON(dsb->free_pos >= DSB_BUF_SIZE)) {
>> +        DRM_DEBUG_KMS("DSB buffer overflow.\n");
> Wouldn't it make sense to do a I915_WRITE(reg, val) here also, like 
> above ?

Wanted to be capture this error and don't want to pass through fallback 
mechanism.
Based on need will fine tune DSB buffer size in future if needed.

>
>> +        return;
>> +    }
>> +
>> +    buf[dsb->free_pos++] = val;
>> +    buf[dsb->free_pos++] = (DSB_OPCODE_MMIO_WRITE  <<
>> +                DSB_OPCODE_SHIFT) | DSB_BYTE_EN |
>> +                i915_mmio_reg_offset(reg);
> Once the write is done, are we planning to reset this free_pos ? May 
> be in some upcoming patch in the series ?

Yes, after commit will be done.

Regards,
Animesh

>> +}
>> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.h 
>> b/drivers/gpu/drm/i915/display/intel_dsb.h
>> index 4a4091cadc1e..1b33ab118640 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;
>>   @@ -22,10 +24,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 cmd_buf_tail.
>> +     */
>> +    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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx