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

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