[Intel-gfx] [PATCH v2 10/15] drm/i915/dsb: function to trigger workload execution of DSB.

[Animesh Manna]

Hi,


On 8/22/2019 12:13 AM, Chris Wilson wrote:
> Quoting Animesh Manna (2019-08-21 07:32:30)
>> Batch buffer will be created through dsb-reg-write function which can have
>> single/multiple request based on usecase and once the buffer is ready
>> commit function will trigger the execution of the batch buffer. All
>> the registers will be updated simultaneously.
>>
>> 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 | 43 ++++++++++++++++++++++++
>>   drivers/gpu/drm/i915/display/intel_dsb.h |  1 +
>>   2 files changed, 44 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c
>> index f97d0c06a049..7e0a9b37f702 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dsb.c
>> +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
>> @@ -191,3 +191,46 @@ void intel_dsb_reg_write(struct intel_dsb *dsb, i915_reg_t reg, u32 val)
>>                                  DSB_OPCODE_SHIFT) | DSB_BYTE_EN |
>>                                  i915_mmio_reg_offset(reg);
>>   }
>> +
>> +void intel_dsb_commit(struct intel_dsb *dsb)
>> +{
>> +       struct intel_crtc *crtc = dsb->crtc;
>> +       struct drm_device *dev = crtc->base.dev;
>> +       struct drm_i915_private *dev_priv = to_i915(dev);
>> +       enum pipe pipe = crtc->pipe;
>> +       u32 cmd_buf_tail, cmd_buf_size;
>> +
>> +       if (!dsb->free_pos)
>> +               return;
>> +
>> +       if (!intel_dsb_enable_engine(dsb))
>> +               goto reset;
>> +
>> +       if (is_dsb_busy(dsb)) {
>> +               DRM_DEBUG_KMS("HEAD_PTR write failed - dsb engine is busy.\n");
>> +               goto reset;
>> +       }
>> +       I915_WRITE(DSB_HEAD_PTR(pipe, dsb->id), dsb->cmd_buf_head);
>> +
>> +       cmd_buf_size = dsb->free_pos * 4;
>> +       cmd_buf_tail = round_up((dsb->cmd_buf_head + cmd_buf_size),
>> +                               CACHELINE_BYTES);
> head is already page-aligned.
>
> tail = ALIGN(dst->free_pos * 4, CACHELINE_BYTES);
> I915_WRITE(DSB_TAIL(pipe, dsb->id), i915_ggtt_offset(dsb->vma) + tail);

Ok.
>
>> +
>> +       if (is_dsb_busy(dsb)) {
>> +               DRM_DEBUG_KMS("TAIL_PTR write failed - dsb engine is busy.\n");
>> +               goto reset;
>> +       }
>> +       DRM_DEBUG_KMS("DSB execution started - buf-size %u, head 0x%x,"
>> +                     "tail 0x%x\n", cmd_buf_size, dsb->cmd_buf_head,
>> +                     cmd_buf_tail);
>> +       I915_WRITE(DSB_TAIL_PTR(pipe, dsb->id), cmd_buf_tail);
> This looks very suspect. You enable the HW before setting up the cmdbuf,
> so what is executing? Is the execution latched on TAIL or the CTL? Is it
> latched at all?

Yes, execution is latched with TAIL - it the trigger to start dsb execution.
>
>> +       if (wait_for(!is_dsb_busy(dsb), 1)) {
>> +               DRM_ERROR("Timed out waiting for DSB workload completion.\n");
>> +               goto reset;
>> +       }
>> +
>> +reset:
>> +       memset(dsb->cmd_buf, 0, DSB_BUF_SIZE);
> Again, why?

Can be optimized by filling zero in the unused space before 
DSB_TAIL_PTR. Will do.
Currently have single commit between get and put call.
For multiple commit we can use same buffer, so want to clear the buffer 
so that can be used for next commit call.

Regards,
Animesh
> -Chris