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

[Kieran Bingham]

Hi Laurent,

On 07/04/18 00:38, Laurent Pinchart wrote:
> 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 
>>
>> ---
>> 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(>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.

Agreed, Done.

> 
>> +struct v4l2_mbus_framefmt *format;
>> +
>> +format = vsp1_entity_get_pad_format(>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.
> 

Removed

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

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

[Laurent Pinchart]

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 
> 
> ---
> 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(>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(>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