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@xxxxxxxxxxxxxxxx>
>
> ---
> 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
