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

From: Chris Wilson <chris@chris-wilson.co.uk>
To: Animesh Manna <animesh.manna@intel.com>, intel-gfx@lists.freedesktop.org
Cc: Jani Nikula <jani.nikula@intel.com>
Subject: Re: [PATCH v2 10/15] drm/i915/dsb: function to trigger workload execution of DSB.
Date: Wed, 21 Aug 2019 19:43:45 +0100	[thread overview]
Message-ID: <156641302579.20466.3161372104145606896@skylake-alporthouse-com> (raw)
In-Reply-To: <20190821063236.19705-11-animesh.manna@intel.com>

Quoting Animesh Manna (2019-08-21 07:32:30)
> 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.
> 
> 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 | 43 ++++++++++++++++++++++++
>  drivers/gpu/drm/i915/display/intel_dsb.h |  1 +
>  2 files changed, 44 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c
> index f97d0c06a049..7e0a9b37f702 100644
> --- a/drivers/gpu/drm/i915/display/intel_dsb.c
> +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
> @@ -191,3 +191,46 @@ 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 cmd_buf_tail, cmd_buf_size;
> +
> +       if (!dsb->free_pos)
> +               return;
> +
> +       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_PTR(pipe, dsb->id), dsb->cmd_buf_head);
> +
> +       cmd_buf_size = dsb->free_pos * 4;
> +       cmd_buf_tail = round_up((dsb->cmd_buf_head + cmd_buf_size),
> +                               CACHELINE_BYTES);

head is already page-aligned.

tail = ALIGN(dst->free_pos * 4, CACHELINE_BYTES);
I915_WRITE(DSB_TAIL(pipe, dsb->id), i915_ggtt_offset(dsb->vma) + tail);

> +
> +       if (is_dsb_busy(dsb)) {
> +               DRM_DEBUG_KMS("TAIL_PTR write failed - dsb engine is busy.\n");
> +               goto reset;
> +       }
> +       DRM_DEBUG_KMS("DSB execution started - buf-size %u, head 0x%x,"
> +                     "tail 0x%x\n", cmd_buf_size, dsb->cmd_buf_head,
> +                     cmd_buf_tail);
> +       I915_WRITE(DSB_TAIL_PTR(pipe, dsb->id), cmd_buf_tail);

This looks very suspect. You enable the HW before setting up the cmdbuf,
so what is executing? Is the execution latched on TAIL or the CTL? Is it
latched at all?

> +       if (wait_for(!is_dsb_busy(dsb), 1)) {
> +               DRM_ERROR("Timed out waiting for DSB workload completion.\n");
> +               goto reset;
> +       }
> +
> +reset:
> +       memset(dsb->cmd_buf, 0, DSB_BUF_SIZE);

Again, why?
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx