Hi Laurent,
Thanks for the review,
V2 to be posted after testing.
On 13/02/17 21:21, Laurent Pinchart wrote:
> Hi Kieran,
>
> Thank you for the patch.
>
> On Friday 04 Nov 2016 18:19:29 Kieran Bingham wrote:
>> Previously the active window and partition sizes for each partition is
>
> s/is/were/
Fixed.
>
>> calculated for each partition every frame. This data is constant and
>> only needs to be calculated once at the start of the stream.
>>
>> Extend the vsp1_pipe object to store the maximum number of partitions
>> possible and pre-calculate the partition sizes into this table.
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@xxxxxxxxxxxxxxxx>
>> ---
>> drivers/media/platform/vsp1/vsp1_pipe.h | 6 ++++++
>> drivers/media/platform/vsp1/vsp1_video.c | 8 ++++++--
>> 2 files changed, 12 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/media/platform/vsp1/vsp1_pipe.h
>> b/drivers/media/platform/vsp1/vsp1_pipe.h index f181949824c9..3af96c4ea244
>> 100644
>> --- a/drivers/media/platform/vsp1/vsp1_pipe.h
>> +++ b/drivers/media/platform/vsp1/vsp1_pipe.h
>> @@ -20,6 +20,9 @@
>>
>> #include <media/media-entity.h>
>>
>> +/* Max Video Width / Min Partition Size = 8190/128 */
>> +#define VSP1_PIPE_MAX_PARTITIONS 64
>> +
^ This define is removed with dynamic allocations.
>> struct vsp1_dl_list;
>> struct vsp1_rwpf;
>>
>> @@ -81,7 +84,9 @@ enum vsp1_pipeline_state {
>> * @dl: display list associated with the pipeline
>> * @div_size: The maximum allowed partition size for the pipeline
>> * @partitions: The number of partitions used to process one frame
>> + * @partition: The current partition for configuration to process
>> * @current_partition: The partition number currently being configured
>> + * @part_table: The pre-calculated partitions used by the pipeline
>> */
>> struct vsp1_pipeline {
>> struct media_pipeline pipe;
>> @@ -116,6 +121,7 @@ struct vsp1_pipeline {
>> unsigned int partitions;
>> struct v4l2_rect partition;
>> unsigned int current_partition;
>> + struct v4l2_rect part_table[VSP1_PIPE_MAX_PARTITIONS];
>
> That's an extra 1kB or kmalloc'ed data. I'd prefer allocating it dynamically
> as needed.
Ok, allocating with kcalloc in vsp1_video_pipeline_setup_partitions() (The
earliest opportunity to know how many partitions are needed)
Free'd in vsp1_video_stop_streaming()
>> };
>>
>> void vsp1_pipeline_reset(struct vsp1_pipeline *pipe);
>> diff --git a/drivers/media/platform/vsp1/vsp1_video.c
>> b/drivers/media/platform/vsp1/vsp1_video.c index 6d43c02bbc56..c4a8c30df108
>> 100644
>> --- a/drivers/media/platform/vsp1/vsp1_video.c
>> +++ b/drivers/media/platform/vsp1/vsp1_video.c
>> @@ -255,6 +255,7 @@ static void vsp1_video_pipeline_setup_partitions(struct
>> vsp1_pipeline *pipe) const struct v4l2_mbus_framefmt *format;
>> struct vsp1_entity *entity;
>> unsigned int div_size;
>> + int i;
>
> i can never be negative, you can make it an unsigned int.
Fixed.
> Apart from that,
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
Thanks, I'll tentatively add this tag, but please check you are happy with the
dynamic allocation on the repost v2 :-) !
>
>> /*
>> * Partitions are computed on the size before rotation, use the format
>> @@ -269,6 +270,7 @@ static void vsp1_video_pipeline_setup_partitions(struct
>> vsp1_pipeline *pipe) if (vsp1->info->gen == 2) {
>> pipe->div_size = div_size;
>> pipe->partitions = 1;
>> + pipe->part_table[0] = vsp1_video_partition(pipe, div_size, 0);
>> return;
>> }
>>
>> @@ -284,6 +286,9 @@ static void vsp1_video_pipeline_setup_partitions(struct
>> vsp1_pipeline *pipe)
>>
>> pipe->div_size = div_size;
>> pipe->partitions = DIV_ROUND_UP(format->width, div_size);
>> +
>> + for (i = 0; i < pipe->partitions; i++)
>> + pipe->part_table[i] = vsp1_video_partition(pipe, div_size, i);
>> }
>>
>> /* ------------------------------------------------------------------------
>> @@ -355,8 +360,7 @@ static void vsp1_video_pipeline_run_partition(struct
>> vsp1_pipeline *pipe, {
>> struct vsp1_entity *entity;
>>
>> - pipe->partition = vsp1_video_partition(pipe, pipe->div_size,
>> - pipe->current_partition);
>> + pipe->partition = pipe->part_table[pipe->current_partition];
>>
>> list_for_each_entry(entity, &pipe->entities, list_pipe) {
>> if (entity->ops->configure)
>
