On Thu, 12 Sep 2019, "Sharma, Shashank" <shashank.sharma@xxxxxxxxx> wrote:
> On 9/12/2019 7:09 PM, Jani Nikula wrote:
>> On Thu, 12 Sep 2019, "Sharma, Shashank" <shashank.sharma@xxxxxxxxx> wrote:
>>> On 9/12/2019 12:41 AM, Animesh Manna wrote:
>>>> 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.
>>>>
>>>> v1: Initial version.
>>>> v2: Optimized code few places. (Chris)
>>>> v3: USed DRM_ERROR for dsb head/tail programming failure. (Shashank)
>>>>
>>>> Cc: Imre Deak <imre.deak@xxxxxxxxx>
>>>> Cc: Jani Nikula <jani.nikula@xxxxxxxxx>
>>>> Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
>>>> Cc: Shashank Sharma <shashank.sharma@xxxxxxxxx>
>>>> Signed-off-by: Animesh Manna <animesh.manna@xxxxxxxxx>
>>>> ---
>>>> drivers/gpu/drm/i915/display/intel_dsb.c | 42 ++++++++++++++++++++++++
>>>> drivers/gpu/drm/i915/display/intel_dsb.h | 1 +
>>>> drivers/gpu/drm/i915/i915_reg.h | 2 ++
>>>> 3 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 2b0ffc0afb74..eea86afb0583 100644
>>>> --- a/drivers/gpu/drm/i915/display/intel_dsb.c
>>>> +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
>>>> @@ -212,3 +212,45 @@ void intel_dsb_reg_write(struct intel_dsb *dsb, i915_reg_t reg, u32 val)
>>>> (DSB_BYTE_EN << DSB_BYTE_EN_SHIFT) |
>>>> i915_mmio_reg_offset(reg);
>>>> }
>>>> +
>>>> +void intel_dsb_commit(struct intel_dsb *dsb)
>>>> +{
>>>> + struct intel_crtc *crtc = container_of(dsb, typeof(*crtc), dsb);
>>>> + struct drm_device *dev = crtc->base.dev;
>>>> + struct drm_i915_private *dev_priv = to_i915(dev);
>>>> + enum pipe pipe = crtc->pipe;
>>>> + u32 tail;
>>>> +
>>>> + if (!dsb->free_pos)
>>>> + return;
>>>> +
>>>> + if (!intel_dsb_enable_engine(dsb))
>>>> + goto reset;
>>>> +
>>>> + if (is_dsb_busy(dsb)) {
>>>> + DRM_ERROR("HEAD_PTR write failed - dsb engine is busy.\n");
>>>> + goto reset;
>>>> + }
>>>> + I915_WRITE(DSB_HEAD(pipe, dsb->id), i915_ggtt_offset(dsb->vma));
>>>> +
>>>> + tail = ALIGN(dsb->free_pos * 4, CACHELINE_BYTES);
>>>> + if (tail > dsb->free_pos * 4)
>>>> + memset(&dsb->cmd_buf[dsb->free_pos], 0,
>>>> + (tail - dsb->free_pos * 4));
>>>> +
>>>> + if (is_dsb_busy(dsb)) {
>>>> + DRM_ERROR("TAIL_PTR write failed - dsb engine is busy.\n");
>>>> + goto reset;
>>>> + }
>>>> + DRM_DEBUG_KMS("DSB execution started - head 0x%x, tail 0x%x\n",
>>>> + i915_ggtt_offset(dsb->vma), tail);
>>>> + I915_WRITE(DSB_TAIL(pipe, dsb->id), i915_ggtt_offset(dsb->vma) + tail);
>>>> + if (wait_for(!is_dsb_busy(dsb), 1)) {
>>>> + DRM_ERROR("Timed out waiting for DSB workload completion.\n");
>>>> + goto reset;
>>>> + }
>>>> +
>>>> +reset:
>>>> + dsb->free_pos = 0;
>>>> + intel_dsb_disable_engine(dsb);
>>> I am still not very convince if a commit function should be void, I
>>> would still want it to return success or failure, so that we would know
>>> if the last operation was successful or not.
>>>
>>> I would wait Jani N to comment here, on what he feels about this.
>> The question becomes, what do you *do* with the return value? If none of
>> the callers are going to use it, don't return it.
>
> I was thinking we should check the return value of the DSB commit (if
> not writes), so that we would be aware that the register programming
> failed, and later even can think about a fallback method. Too ambitious ?
Easy to add later if needed, I think.
BR,
Jani.
>
> - Shashank
>
>> BR,
>> Jani.
>>
>>> - Shashank
>>>
>>>> +}
>>>> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.h b/drivers/gpu/drm/i915/display/intel_dsb.h
>>>> index 9b2522f20bfb..7389c8c5b665 100644
>>>> --- a/drivers/gpu/drm/i915/display/intel_dsb.h
>>>> +++ b/drivers/gpu/drm/i915/display/intel_dsb.h
>>>> @@ -43,5 +43,6 @@ 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);
>>>> +void intel_dsb_commit(struct intel_dsb *dsb);
>>>>
>>>> #endif
>>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>>>> index 2dbaa49f5c74..c77b5066d8dd 100644
>>>> --- a/drivers/gpu/drm/i915/i915_reg.h
>>>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>>>> @@ -11687,6 +11687,8 @@ enum skl_power_gate {
>>>> #define _DSBSL_INSTANCE_BASE 0x70B00
>>>> #define DSBSL_INSTANCE(pipe, id) (_DSBSL_INSTANCE_BASE + \
>>>> (pipe) * 0x1000 + (id) * 100)
>>>> +#define DSB_HEAD(pipe, id) _MMIO(DSBSL_INSTANCE(pipe, id) + 0x0)
>>>> +#define DSB_TAIL(pipe, id) _MMIO(DSBSL_INSTANCE(pipe, id) + 0x4)
>>>> #define DSB_CTRL(pipe, id) _MMIO(DSBSL_INSTANCE(pipe, id) + 0x8)
>>>> #define DSB_ENABLE (1 << 31)
>>>> #define DSB_STATUS (1 << 0)
--
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
