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 <kieran.bingham+rene...@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.
Agreed, Done.
>
>> + 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.
>
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 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.
OK, I'll give this a go now ...
Right ... it's looking good. A good clear separation
>
>> * @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]
>
