Re: [PATCH 6/8] v4l: vsp1: Allow entities to participate in the partition algorithm

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Kieran,

Thank you for the patch.

On Friday 10 Feb 2017 20:27:34 Kieran Bingham wrote:
> The configuration of the pipeline, and entities directly affects the
> inputs required to each entity for the partition algorithm. Thus it
> makes sense to involve those entities in the decision making process.
> 
> Extend the entity ops API to provide an optional '.partition' call. This
> allows entities which may effect the partition window, to process based
> on their configuration.
> 
> Entities implementing this operation must return their required input
> parameters, which will be passed down the chain. This creates a process
> whereby each entity describes what is required to satisfy the required
> output to it's predecessor in the pipeline.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> ---
>  drivers/media/platform/vsp1/vsp1_entity.h |  8 ++++-
>  drivers/media/platform/vsp1/vsp1_pipe.c   | 22 ++++++++++++-
>  drivers/media/platform/vsp1/vsp1_pipe.h   | 35 +++++++++++++++++--
>  drivers/media/platform/vsp1/vsp1_rpf.c    | 33 +++++++++---------
>  drivers/media/platform/vsp1/vsp1_sru.c    | 29 +++++++++++++++-
>  drivers/media/platform/vsp1/vsp1_uds.c    | 45 ++++++++++++++++++++++--
>  drivers/media/platform/vsp1/vsp1_video.c  | 32 ++++++++++-------
>  drivers/media/platform/vsp1/vsp1_wpf.c    | 29 +++++++++++----
>  8 files changed, 195 insertions(+), 38 deletions(-)

[snip]

> diff --git a/drivers/media/platform/vsp1/vsp1_pipe.c
> b/drivers/media/platform/vsp1/vsp1_pipe.c index 280ba0804699..16f2eada54d5
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_pipe.c
> +++ b/drivers/media/platform/vsp1/vsp1_pipe.c
> @@ -331,6 +331,28 @@ void vsp1_pipeline_propagate_alpha(struct vsp1_pipeline
> *pipe, vsp1_uds_set_alpha(pipe->uds, dl, alpha);
>  }
> 
> +/*
> + * Propagate the partition calculations through the pipeline
> + *
> + * Work backwards through the pipe, allowing each entity to update
> + * the partition parameters based on it's configuration, and the entity

s/it's/its/

> + * connected to it's source. Each entity must produce the partition

Ditto.

> + * required for the next entity in the routing.

Maybe "for the previous entity in the pipeline" ?

> + */
> +void vsp1_pipeline_propagate_partition(struct vsp1_pipeline *pipe,
> +				       struct vsp1_partition *partition,
> +				       unsigned int index,
> +				       struct vsp1_partition_rect *rect)
> +{
> +	struct vsp1_entity *entity;
> +
> +	list_for_each_entry_reverse(entity, &pipe->entities, list_pipe) {
> +		if (entity->ops->partition)
> +			rect = entity->ops->partition(entity, pipe, partition,
> +						      index, rect);

How about modifying the rect argument in place ? I think that would simplify 
the code.

> +	}
> +}
> +
>  void vsp1_pipelines_suspend(struct vsp1_device *vsp1)
>  {
>  	unsigned long flags;
> diff --git a/drivers/media/platform/vsp1/vsp1_pipe.h
> b/drivers/media/platform/vsp1/vsp1_pipe.h index 6494c4c75023..718ca0a5eca7
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_pipe.h
> +++ b/drivers/media/platform/vsp1/vsp1_pipe.h
> @@ -60,11 +60,32 @@ enum vsp1_pipeline_state {
>  };
> 
>  /*
> + * struct vsp1_partition_rect
> + *
> + * replicates struct v4l2_rect, but with an offset to apply
> + */
> +struct vsp1_partition_rect {

Let's name this vsp1_partition_window, as that's what it describes.

> +	__s32   left;
> +	__s32   top;
> +	__u32   width;
> +	__u32   height;
> +	__u32   offset;
> +};
> +
> +/*
>   * struct vsp1_partition - A description of each partition slice performed
> by HW
> - * @dest: The position and dimension of this partition in the destination
> image
> + * @rpf: The RPF partition window configuration
> + * @uds_sink: The UDS input partition window configuration
> + * @uds_source: The UDS output partition window configuration
> + * @sru: The SRU partition window configuration
> + * @wpf: The WPF partition window configuration
>   */
>  struct vsp1_partition {
> -	struct v4l2_rect dest;
> +	struct vsp1_partition_rect rpf;
> +	struct vsp1_partition_rect uds_sink;
> +	struct vsp1_partition_rect uds_source;
> +	struct vsp1_partition_rect sru;
> +	struct vsp1_partition_rect wpf;
>  };
> 
>  /*
> @@ -117,6 +138,11 @@ struct vsp1_pipeline {
>  	struct vsp1_entity *uds;
>  	struct vsp1_entity *uds_input;
> 
> +	/*
> +	 * The order of this list should be representative of the order and

I'd say it "must be identical to the order of the entities in the pipeline".

> +	 * routing of the the pipeline, as it is assumed by the partition
> +	 * algorithm that we can walk this list in sequence.
> +	 */
>  	struct list_head entities;
> 
>  	struct vsp1_dl_list *dl;
> @@ -139,6 +165,11 @@ void vsp1_pipeline_frame_end(struct vsp1_pipeline
> *pipe); void vsp1_pipeline_propagate_alpha(struct vsp1_pipeline *pipe,
>  				   struct vsp1_dl_list *dl, unsigned int 
alpha);
> 
> +void vsp1_pipeline_propagate_partition(struct vsp1_pipeline *pipe,
> +				       struct vsp1_partition *partition,
> +				       unsigned int index,
> +				       struct vsp1_partition_rect *rect);
> +
>  void vsp1_pipelines_suspend(struct vsp1_device *vsp1);
>  void vsp1_pipelines_resume(struct vsp1_device *vsp1);
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_rpf.c
> b/drivers/media/platform/vsp1/vsp1_rpf.c index df380a237118..94541ab4ca36
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_rpf.c
> +++ b/drivers/media/platform/vsp1/vsp1_rpf.c

[snip]

> +/*
> + * Perform RPF specific calculations for the Partition Algorithm

Is the "Partition Algorithm" such an almighty power that it requires caps ? 
:-) I think I'd drop the comment, the operation kerneldoc in vsp1_entity.h 
should be enough.

> + */
> +struct vsp1_partition_rect *rpf_partition(struct vsp1_entity *entity,

This function should be static. Same comment for the other modules.

> +					  struct vsp1_pipeline *pipe,
> +					  struct vsp1_partition *partition,
> +					  unsigned int partition_idx,
> +					  struct vsp1_partition_rect *dest)
> +{
> +	/* Duplicate the target configuration to the RPF */
> +	partition->rpf = *dest;
> +
> +	return &partition->rpf;
> +}

[snip]

-- 
Regards,

Laurent Pinchart