Re: [PATCH v3 08/11] drm/i915/dsb: function to trigger workload execution of DSB.

["Sharma, Shashank" <shashank.sharma@intel.com>]

On 8/28/2019 12:40 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)
>
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@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 +
>   2 files changed, 43 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c
> index 2d6e78868f2d..bc1734072f34 100644
> --- a/drivers/gpu/drm/i915/display/intel_dsb.c
> +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
> @@ -214,3 +214,45 @@ 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 tail;
> +
> +	if (!dsb->free_pos)
> +		return;
How would the user know if DSB commit failed, and it has to retry ? 
should we add a return value ?
> +
> +	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(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_DEBUG_KMS("TAIL_PTR write failed - dsb engine is busy.\n");
> +		goto reset;
> +	}
a space here will look good, and aligned to above.
> +	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)) {
This time limit of 1 is bspec recommended, or its by experience ?
> +		DRM_ERROR("Timed out waiting for DSB workload completion.\n");
> +		goto reset;
> +	}
> +
> +reset:
> +	dsb->free_pos = 0;
reset is just doing dsb->free_pos = 0; is that enough ? I think we need 
to do a complete reset of the buffer memset(dsb->buffer, 0) or something 
similar, else batch buffer will contain garbage values from previous 
weites.
> +	intel_dsb_disable_engine(dsb);
We still want to return a -ve value to indicate the write failed.
> +}
> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.h b/drivers/gpu/drm/i915/display/intel_dsb.h
> index c848747f52d9..f5d1f1bedc12 100644
> --- a/drivers/gpu/drm/i915/display/intel_dsb.h
> +++ b/drivers/gpu/drm/i915/display/intel_dsb.h
> @@ -44,5 +44,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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx