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

[Sharma, Shashank]

On 9/12/2019 7:09 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:
>>> 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 at intel.com>
>>> 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 | 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 ?

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