Re: [PATCH v2 11/14] v4l: vsp1: Add support for header display lists in continuous mode

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Laurent,

On 26/06/17 19:12, Laurent Pinchart wrote:
> The VSP supports both header and headerless display lists. The latter is
> easier to use when the VSP feeds data directly to the DU in continuous
> mode, and the driver thus uses headerless display lists for DU operation
> and header display lists otherwise.
> 
> Headerless display lists are only available on WPF.0. This has never
> been an issue so far, as only WPF.0 is connected to the DU. However, on
> H3 ES2.0, the VSP-DL instance has both WPF.0 and WPF.1 connected to the
> DU. We thus can't use headerless display lists unconditionally for DU
> operation.

Would it be crazy to suggest we drop headerless display lists?

If we must support header display lists in continuous mode - Rather than having
2 cases for continuous modes to support (having to support headerless, on WPF.0,
and header on WPF.1) if we just use your header loop trick - would that simplify
our code maintenance?

(We can always remove headerless support later if you agree, this is more of an
idea at the moment)


> Implement support for continuous mode with header display lists, and use
> it for DU operation on WPF outputs that don't support headerless mode.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx>

Except for future discussion points, I can't see anything wrong here so:

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@xxxxxxxxxxxxxxxx>

> ---
>  drivers/media/platform/vsp1/vsp1_dl.c   | 195 +++++++++++++++++++++-----------
>  drivers/media/platform/vsp1/vsp1_regs.h |   1 +
>  2 files changed, 127 insertions(+), 69 deletions(-)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_dl.c b/drivers/media/platform/vsp1/vsp1_dl.c
> index bb92be4fe0f0..8b5cbb6b7a70 100644
> --- a/drivers/media/platform/vsp1/vsp1_dl.c
> +++ b/drivers/media/platform/vsp1/vsp1_dl.c
> @@ -95,6 +95,7 @@ enum vsp1_dl_mode {
>   * struct vsp1_dl_manager - Display List manager
>   * @index: index of the related WPF
>   * @mode: display list operation mode (header or headerless)
> + * @singleshot: execute the display list in single-shot mode
>   * @vsp1: the VSP1 device
>   * @lock: protects the free, active, queued, pending and gc_fragments lists
>   * @free: array of all free display lists
> @@ -107,6 +108,7 @@ enum vsp1_dl_mode {
>  struct vsp1_dl_manager {
>  	unsigned int index;
>  	enum vsp1_dl_mode mode;
> +	bool singleshot;
>  	struct vsp1_device *vsp1;
>  
>  	spinlock_t lock;
> @@ -437,6 +439,7 @@ int vsp1_dl_list_add_chain(struct vsp1_dl_list *head,
>  
>  static void vsp1_dl_list_fill_header(struct vsp1_dl_list *dl, bool is_last)
>  {
> +	struct vsp1_dl_manager *dlm = dl->dlm;
>  	struct vsp1_dl_header_list *hdr = dl->header->lists;
>  	struct vsp1_dl_body *dlb;
>  	unsigned int num_lists = 0;
> @@ -461,85 +464,145 @@ static void vsp1_dl_list_fill_header(struct vsp1_dl_list *dl, bool is_last)
>  
>  	dl->header->num_lists = num_lists;
>  
> -	/*
> -	 * If this display list's chain is not empty, we are on a list, where
> -	 * the next item in the list is the display list entity which should be
> -	 * automatically queued by the hardware.
> -	 */
>  	if (!list_empty(&dl->chain) && !is_last) {
> +		/*
> +		 * If this display list's chain is not empty, we are on a list,
> +		 * and the next item is the display list that we must queue for
> +		 * automatic processing by the hardware.
> +		 */
>  		struct vsp1_dl_list *next = list_next_entry(dl, chain);
>  
>  		dl->header->next_header = next->dma;
>  		dl->header->flags = VSP1_DLH_AUTO_START;
> +	} else if (!dlm->singleshot) {
> +		/*
> +		 * if the display list manager works in continuous mode, the VSP
> +		 * should loop over the display list continuously until
> +		 * instructed to do otherwise.
> +		 */
> +		dl->header->next_header = dl->dma;
> +		dl->header->flags = VSP1_DLH_INT_ENABLE | VSP1_DLH_AUTO_START;

This is a neat trick - and I wonder if actually - it gives us even more
flexibility in our control.

Presently on continuous mode - I believe we have to reset the WPF to 'stop' ...
In this mode - I suspect we could update the display list - (albeit that might
be a bit racy) to disable the AUTO_START. Maybe that's better .. Maybe it's worse :)

>  	} else {
> +		/*
> +		 * Otherwise, in mem-to-mem mode, we work in single-shot mode
> +		 * and the next display list must not be started automatically.
> +		 */
>  		dl->header->flags = VSP1_DLH_INT_ENABLE;
>  	}
>  }
>  
> -void vsp1_dl_list_commit(struct vsp1_dl_list *dl)
> +static bool vsp1_dl_list_hw_update_pending(struct vsp1_dl_manager *dlm)
>  {
> -	struct vsp1_dl_manager *dlm = dl->dlm;
>  	struct vsp1_device *vsp1 = dlm->vsp1;
> -	unsigned long flags;
> -	bool update;
> -
> -	if (dl->dlm->mode == VSP1_DL_MODE_HEADER) {
> -		struct vsp1_dl_list *dl_child;
> -
> -		/*
> -		 * In header mode the caller guarantees that the hardware is
> -		 * idle at this point.
> -		 */
>  
> -		/* Fill the header for the head and chained display lists. */
> -		vsp1_dl_list_fill_header(dl, list_empty(&dl->chain));
> -
> -		list_for_each_entry(dl_child, &dl->chain, chain) {
> -			bool last = list_is_last(&dl_child->chain, &dl->chain);
> +	if (!dlm->queued)
> +		return false;
>  
> -			vsp1_dl_list_fill_header(dl_child, last);
> -		}
> -	}
> +	/*
> +	 * Check whether the VSP1 has taken the update. In headerless mode the
> +	 * hardware indicates this by clearing the UPD bit in the DL_BODY_SIZE
> +	 * register, and in header mode by clearing the UPDHDR bit in the CMD
> +	 * register.
> +	 */
> +	if (dlm->mode == VSP1_DL_MODE_HEADERLESS)
> +		return !!(vsp1_read(vsp1, VI6_DL_BODY_SIZE)
> +			  & VI6_DL_BODY_SIZE_UPD);
> +	else
> +		return !!(vsp1_read(vsp1, VI6_CMD(dlm->index) & VI6_CMD_UPDHDR));
> +}
>  
> -	spin_lock_irqsave(&dlm->lock, flags);
> +static void vsp1_dl_list_hw_enqueue(struct vsp1_dl_list *dl)
> +{
> +	struct vsp1_dl_manager *dlm = dl->dlm;
> +	struct vsp1_device *vsp1 = dlm->vsp1;
>  
> -	if (dl->dlm->mode == VSP1_DL_MODE_HEADER) {
> +	if (dlm->mode == VSP1_DL_MODE_HEADERLESS) {
>  		/*
> -		 * Commit the head display list to hardware. Chained headers
> -		 * will auto-start.
> +		 * In headerless mode, program the hardware directly with the
> +		 * display list body address and size and set the UPD bit. The
> +		 * bit will be cleared by the hardware when the display list
> +		 * processing starts.
> +		 */
> +		vsp1_write(vsp1, VI6_DL_HDR_ADDR(0), dl->body0.dma);
> +		vsp1_write(vsp1, VI6_DL_BODY_SIZE, VI6_DL_BODY_SIZE_UPD |
> +			   (dl->body0.num_entries * sizeof(*dl->header->lists)));
> +	} else {
> +		/*
> +		 * In header mode, program the display list header address. If
> +		 * the hardware is idle (single-shot mode or first frame in
> +		 * continuous mode) it will then be started independently. If
> +		 * the hardware is operating, the VI6_DL_HDR_REF_ADDR register
> +		 * will be updated with the display list address.
>  		 */
>  		vsp1_write(vsp1, VI6_DL_HDR_ADDR(dlm->index), dl->dma);
> -
> -		dlm->active = dl;
> -		goto done;
>  	}
> +}
> +
> +static void vsp1_dl_list_commit_continuous(struct vsp1_dl_list *dl)
> +{
> +	struct vsp1_dl_manager *dlm = dl->dlm;
>  
>  	/*
> -	 * Once the UPD bit has been set the hardware can start processing the
> -	 * display list at any time and we can't touch the address and size
> -	 * registers. In that case mark the update as pending, it will be
> -	 * queued up to the hardware by the frame end interrupt handler.
> +	 * If a previous display list has been queued to the hardware but not
> +	 * processed yet, the VSP can start processing it at any time. In that
> +	 * case we can't replace the queued list by the new one, as we could
> +	 * race with the hardware. We thus mark the update as pending, it will
> +	 * be queued up to the hardware by the frame end interrupt handler.
>  	 */
> -	update = !!(vsp1_read(vsp1, VI6_DL_BODY_SIZE) & VI6_DL_BODY_SIZE_UPD);
> -	if (update) {
> +	if (vsp1_dl_list_hw_update_pending(dlm)) {
>  		__vsp1_dl_list_put(dlm->pending);
>  		dlm->pending = dl;
> -		goto done;
> +		return;
>  	}
>  
>  	/*
> -	 * Program the hardware with the display list body address and size.
> -	 * The UPD bit will be cleared by the device when the display list is
> -	 * processed.
> +	 * Pass the new display list to the hardware and mark it as queued. It
> +	 * will become active when the hardware starts processing it.
>  	 */
> -	vsp1_write(vsp1, VI6_DL_HDR_ADDR(0), dl->body0.dma);
> -	vsp1_write(vsp1, VI6_DL_BODY_SIZE, VI6_DL_BODY_SIZE_UPD |
> -		   (dl->body0.num_entries * sizeof(*dl->header->lists)));
> +	vsp1_dl_list_hw_enqueue(dl);
>  
>  	__vsp1_dl_list_put(dlm->queued);
>  	dlm->queued = dl;
> +}
> +
> +static void vsp1_dl_list_commit_singleshot(struct vsp1_dl_list *dl)
> +{
> +	struct vsp1_dl_manager *dlm = dl->dlm;
> +
> +	/*
> +	 * When working in single-shot mode, the caller guarantees that the
> +	 * hardware is idle at this point. Just commit the head display list
> +	 * to hardware. Chained lists will be started automatically.
> +	 */
> +	vsp1_dl_list_hw_enqueue(dl);
> +
> +	dlm->active = dl;
> +}

I'm pleased to see the _commit_singleshot split out. That certainly makes that
case read easier :)


> +
> +void vsp1_dl_list_commit(struct vsp1_dl_list *dl)
> +{
> +	struct vsp1_dl_manager *dlm = dl->dlm;
> +	struct vsp1_dl_list *dl_child;
> +	unsigned long flags;
> +
> +	if (dlm->mode == VSP1_DL_MODE_HEADER) {
> +		/* Fill the header for the head and chained display lists. */
> +		vsp1_dl_list_fill_header(dl, list_empty(&dl->chain));
> +
> +		list_for_each_entry(dl_child, &dl->chain, chain) {
> +			bool last = list_is_last(&dl_child->chain, &dl->chain);
> +
> +			vsp1_dl_list_fill_header(dl_child, last);
> +		}
> +	}
> +
> +	spin_lock_irqsave(&dlm->lock, flags);
> +
> +	if (dlm->singleshot)
> +		vsp1_dl_list_commit_singleshot(dl);
> +	else
> +		vsp1_dl_list_commit_continuous(dl);
>  
> -done:
>  	spin_unlock_irqrestore(&dlm->lock, flags);
>  }
>  
> @@ -558,31 +621,28 @@ void vsp1_dl_list_commit(struct vsp1_dl_list *dl)
>   */
>  bool vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm)
>  {
> -	struct vsp1_device *vsp1 = dlm->vsp1;
>  	bool completed = false;
>  
>  	spin_lock(&dlm->lock);
>  
> -	__vsp1_dl_list_put(dlm->active);
> -	dlm->active = NULL;
> -
>  	/*
> -	 * Header mode is used for mem-to-mem pipelines only. We don't need to
> -	 * perform any operation as there can't be any new display list queued
> -	 * in that case.
> +	 * The mem-to-mem pipelines work in single-shot mode. No new display
> +	 * list can be queued, we don't have to do anything.
>  	 */
> -	if (dlm->mode == VSP1_DL_MODE_HEADER) {
> +	if (dlm->singleshot) {
> +		__vsp1_dl_list_put(dlm->active);
> +		dlm->active = NULL;
>  		completed = true;
>  		goto done;
>  	}
>  
>  	/*
> -	 * The UPD bit set indicates that the commit operation raced with the
> -	 * interrupt and occurred after the frame end event and UPD clear but
> -	 * before interrupt processing. The hardware hasn't taken the update
> -	 * into account yet, we'll thus skip one frame and retry.
> +	 * If the commit operation raced with the interrupt and occurred after
> +	 * the frame end event but before interrupt processing, the hardware
> +	 * hasn't taken the update into account yet. We have to skip one frame
> +	 * and retry.
>  	 */
> -	if (vsp1_read(vsp1, VI6_DL_BODY_SIZE) & VI6_DL_BODY_SIZE_UPD)
> +	if (vsp1_dl_list_hw_update_pending(dlm))
>  		goto done;
>  
>  	/*
> @@ -590,24 +650,20 @@ bool vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm)
>  	 * frame end interrupt. The display list thus becomes active.
>  	 */
>  	if (dlm->queued) {
> +		__vsp1_dl_list_put(dlm->active);
>  		dlm->active = dlm->queued;
>  		dlm->queued = NULL;
>  		completed = true;
>  	}
>  
>  	/*
> -	 * Now that the UPD bit has been cleared we can queue the next display
> -	 * list to the hardware if one has been prepared.
> +	 * Now that the VSP has started processing the queued display list, we
> +	 * can queue the pending display list to the hardware if one has been
> +	 * prepared.
>  	 */
>  	if (dlm->pending) {
> -		struct vsp1_dl_list *dl = dlm->pending;
> -
> -		vsp1_write(vsp1, VI6_DL_HDR_ADDR(0), dl->body0.dma);
> -		vsp1_write(vsp1, VI6_DL_BODY_SIZE, VI6_DL_BODY_SIZE_UPD |
> -			   (dl->body0.num_entries *
> -			    sizeof(*dl->header->lists)));
> -
> -		dlm->queued = dl;
> +		vsp1_dl_list_hw_enqueue(dlm->pending);
> +		dlm->queued = dlm->pending;
>  		dlm->pending = NULL;
>  	}
>  
> @@ -700,6 +756,7 @@ struct vsp1_dl_manager *vsp1_dlm_create(struct vsp1_device *vsp1,
>  	dlm->index = index;
>  	dlm->mode = index == 0 && !vsp1->info->uapi
>  		  ? VSP1_DL_MODE_HEADERLESS : VSP1_DL_MODE_HEADER;
> +	dlm->singleshot = vsp1->info->uapi;
>  	dlm->vsp1 = vsp1;
>  
>  	spin_lock_init(&dlm->lock);
> diff --git a/drivers/media/platform/vsp1/vsp1_regs.h b/drivers/media/platform/vsp1/vsp1_regs.h
> index 007e2bbc26c0..58d0bea963a6 100644
> --- a/drivers/media/platform/vsp1/vsp1_regs.h
> +++ b/drivers/media/platform/vsp1/vsp1_regs.h
> @@ -18,6 +18,7 @@
>   */
>  
>  #define VI6_CMD(n)			(0x0000 + (n) * 4)
> +#define VI6_CMD_UPDHDR			(1 << 4)
>  #define VI6_CMD_STRCMD			(1 << 0)
>  
>  #define VI6_CLK_DCSWT			0x0018
> 
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux