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

[Jani Nikula <jani.nikula@intel.com>]

On Thu, 12 Sep 2019, "Sharma, Shashank" <shashank.sharma@intel.com> wrote:
> On 9/12/2019 7:09 PM, Jani Nikula wrote:
>> On Thu, 12 Sep 2019, "Sharma, Shashank" <shashank.sharma@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@intel.com>
>>>> Cc: Jani Nikula <jani.nikula@intel.com>
>>>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>>> Cc: Shashank Sharma <shashank.sharma@intel.com>
>>>> Signed-off-by: Animesh Manna <animesh.manna@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 ?

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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx