Re: [PATCH v7 6/8] media: vsp1: Refactor display list configure operations

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

Hi Kieran,

Thank you for the patch.

On Thursday, 8 March 2018 02:05:29 EEST Kieran Bingham wrote:
> The entities provide a single .configure operation which configures the
> object into the target display list, based on the vsp1_entity_params
> selection.
> 
> This restricts us to a single function prototype for both static
> configuration (the pre-stream INIT stage) and the dynamic runtime stages
> for both each frame - and each partition therein.
> 
> Split the configure function into two parts, '.configure_stream()' and
> '.configure_frame()', merging both the VSP1_ENTITY_PARAMS_RUNTIME and
> VSP1_ENTITY_PARAMS_PARTITION stages into a single call through the
> .configure_frame(). The configuration for individual partitions is
> handled by passing the partition number to the configure call, and
> processing any runtime stage actions on the first partition only.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
> ---
> v7
>  - Fix formatting and white space
>  - s/prepare/configure_stream/
>  - s/configure/configure_frame/
> 
>  drivers/media/platform/vsp1/vsp1_bru.c    |  12 +-
>  drivers/media/platform/vsp1/vsp1_clu.c    |  50 +---
>  drivers/media/platform/vsp1/vsp1_dl.h     |   1 +-
>  drivers/media/platform/vsp1/vsp1_drm.c    |  21 +--
>  drivers/media/platform/vsp1/vsp1_entity.c |  17 +-
>  drivers/media/platform/vsp1/vsp1_entity.h |  33 +--
>  drivers/media/platform/vsp1/vsp1_hgo.c    |  12 +-
>  drivers/media/platform/vsp1/vsp1_hgt.c    |  12 +-
>  drivers/media/platform/vsp1/vsp1_hsit.c   |  12 +-
>  drivers/media/platform/vsp1/vsp1_lif.c    |  12 +-
>  drivers/media/platform/vsp1/vsp1_lut.c    |  32 +-
>  drivers/media/platform/vsp1/vsp1_rpf.c    | 164 ++++++-------
>  drivers/media/platform/vsp1/vsp1_sru.c    |  12 +-
>  drivers/media/platform/vsp1/vsp1_uds.c    |  57 ++--
>  drivers/media/platform/vsp1/vsp1_video.c  |  24 +--
>  drivers/media/platform/vsp1/vsp1_wpf.c    | 299 ++++++++++++-----------
>  16 files changed, 378 insertions(+), 392 deletions(-)

[snip]

> diff --git a/drivers/media/platform/vsp1/vsp1_clu.c
> b/drivers/media/platform/vsp1/vsp1_clu.c index b2a39a6ef7e4..b8d8af6d4910
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_clu.c
> +++ b/drivers/media/platform/vsp1/vsp1_clu.c
> @@ -213,37 +213,36 @@ static const struct v4l2_subdev_ops clu_ops = {
>  /*
> ---------------------------------------------------------------------------
> -- * VSP1 Entity Operations
>   */
> +static void clu_configure_stream(struct vsp1_entity *entity,
> +				 struct vsp1_pipeline *pipe,
> +				 struct vsp1_dl_list *dl)
> +{
> +	struct vsp1_clu *clu = to_clu(&entity->subdev);
> +
> +	/*
> +	 * The yuv_mode can't be changed during streaming. Cache it internally
> +	 * for future runtime configuration calls.
> +	 */

I'd move this comment right before the vsp1_entity_get_pad_format() call to 
keep all variable declarations together.

> +	struct v4l2_mbus_framefmt *format;
> +
> +	format = vsp1_entity_get_pad_format(&clu->entity,
> +					    clu->entity.config,
> +					    CLU_PAD_SINK);
> +	clu->yuv_mode = format->code == MEDIA_BUS_FMT_AYUV8_1X32;
> +}

[snip]


> diff --git a/drivers/media/platform/vsp1/vsp1_dl.h
> b/drivers/media/platform/vsp1/vsp1_dl.h index 7e820ac6865a..f45083251644
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_dl.h
> +++ b/drivers/media/platform/vsp1/vsp1_dl.h
> @@ -41,7 +41,6 @@ vsp1_dl_body_pool_create(struct vsp1_device *vsp1,
> unsigned int num_bodies, void vsp1_dl_body_pool_destroy(struct
> vsp1_dl_body_pool *pool);
>  struct vsp1_dl_body *vsp1_dl_body_get(struct vsp1_dl_body_pool *pool);
>  void vsp1_dl_body_put(struct vsp1_dl_body *dlb);
> -

This is an unrelated change.

>  void vsp1_dl_body_write(struct vsp1_dl_body *dlb, u32 reg, u32 data);
>  int vsp1_dl_list_add_body(struct vsp1_dl_list *dl, struct vsp1_dl_body
> *dlb);
>  int vsp1_dl_list_add_chain(struct vsp1_dl_list *head, struct vsp1_dl_list
>  *dl);

[snip]

> diff --git a/drivers/media/platform/vsp1/vsp1_entity.h
> b/drivers/media/platform/vsp1/vsp1_entity.h index
> 408602ebeb97..b44ed5414fc3 100644
> --- a/drivers/media/platform/vsp1/vsp1_entity.h
> +++ b/drivers/media/platform/vsp1/vsp1_entity.h

[snip]

> @@ -80,8 +68,10 @@ struct vsp1_route {
>  /**
>   * struct vsp1_entity_operations - Entity operations
>   * @destroy:	Destroy the entity.
> - * @configure:	Setup the hardware based on the entity state (pipeline,
> formats,
> - *		selection rectangles, ...)
> + * @configure_stream:	Setup the initial hardware parameters for the 
stream
> + *			(pipeline, formats)

Instead of initial I would say "Setup hardware parameters that stay constant 
for the whole stream (pipeline, formats)", or possible "that don't vary 
between frames" instead.

> + * @configure_frame:	Configure the runtime parameters for each partition
> + *			(rectangles, buffer addresses, ...)

Maybe "for each frame and each partition thereof" ?

I think we mentioned, when discussing naming, the option of also having a 
configure_partition() operation. Do you think that would make sense ? The 
fact that the partition parameter to the .configure_frame() operation is used 
for the sole purpose of checking whether to configure frame-related parameters 
when partition == 0 makes me think that having two separate operations could 
make sense.

>   * @max_width:	Return the max supported width of data that the entity can
>   *		process in a single operation.
>   * @partition:	Process the partition construction based on this entity's

[snip]

-- 
Regards,

Laurent Pinchart