Hi Kieran, Thank you for the patch.
On Friday 04 Nov 2016 18:19:30 Kieran Bingham wrote: > The vsp1_pipe object context variables for div_size and > current_partition allowed state to be maintained through processing the > partitions during processing. > > Now that the partition tables are calculated during stream on, there is > no requirement to store these variables in the pipe object. > > Utilise local variables for the processing as required. > > Signed-off-by: Kieran Bingham <[email protected]> > --- > drivers/media/platform/vsp1/vsp1_pipe.h | 4 ---- > drivers/media/platform/vsp1/vsp1_video.c | 19 +++++++++---------- > 2 files changed, 9 insertions(+), 14 deletions(-) > > diff --git a/drivers/media/platform/vsp1/vsp1_pipe.h > b/drivers/media/platform/vsp1/vsp1_pipe.h index 3af96c4ea244..9e108ddcceb6 > 100644 > --- a/drivers/media/platform/vsp1/vsp1_pipe.h > +++ b/drivers/media/platform/vsp1/vsp1_pipe.h > @@ -82,10 +82,8 @@ enum vsp1_pipeline_state { > * @uds_input: entity at the input of the UDS, if the UDS is present > * @entities: list of entities in the pipeline > * @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 { > @@ -117,10 +115,8 @@ struct vsp1_pipeline { > > struct vsp1_dl_list *dl; > > - unsigned int div_size; > unsigned int partitions; > struct v4l2_rect partition; > - unsigned int current_partition; > struct v4l2_rect part_table[VSP1_PIPE_MAX_PARTITIONS]; > }; > > diff --git a/drivers/media/platform/vsp1/vsp1_video.c > b/drivers/media/platform/vsp1/vsp1_video.c index c4a8c30df108..9efaab2cc982 > 100644 > --- a/drivers/media/platform/vsp1/vsp1_video.c > +++ b/drivers/media/platform/vsp1/vsp1_video.c > @@ -268,7 +268,6 @@ static void vsp1_video_pipeline_setup_partitions(struct > vsp1_pipeline *pipe) > > /* Gen2 hardware doesn't require image partitioning. */ > 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,7 +283,6 @@ 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++) > @@ -356,11 +354,12 @@ static void vsp1_video_frame_end(struct vsp1_pipeline > *pipe, } > > static void vsp1_video_pipeline_run_partition(struct vsp1_pipeline *pipe, > - struct vsp1_dl_list *dl) > + struct vsp1_dl_list *dl, > + unsigned int partition_number) > { > struct vsp1_entity *entity; > > - pipe->partition = pipe->part_table[pipe->current_partition]; > + pipe->partition = pipe->part_table[partition_number]; > > list_for_each_entry(entity, &pipe->entities, list_pipe) { > if (entity->ops->configure) > @@ -373,6 +372,7 @@ static void vsp1_video_pipeline_run(struct vsp1_pipeline > *pipe) { > struct vsp1_device *vsp1 = pipe->output->entity.vsp1; > struct vsp1_entity *entity; > + unsigned int current_partition = 0; I would call thus partition, current_partition is a bit long and doesn't carry much useful extra information in my opinion. There's also no need to initialize the variable to 0 if you hardcode the 0 value in the first call to vsp1_video_pipeline_run_partition(), as the for loop then takes care of initializing the variable to 1. Apart from that, Reviewed-by: Laurent Pinchart <[email protected]> > > if (!pipe->dl) > pipe->dl = vsp1_dl_list_get(pipe->output->dlm); > @@ -389,13 +389,12 @@ static void vsp1_video_pipeline_run(struct > vsp1_pipeline *pipe) } > > /* Run the first partition */ > - pipe->current_partition = 0; > - vsp1_video_pipeline_run_partition(pipe, pipe->dl); > + vsp1_video_pipeline_run_partition(pipe, pipe->dl, current_partition); > > /* Process consecutive partitions as necessary */ > - for (pipe->current_partition = 1; > - pipe->current_partition < pipe->partitions; > - pipe->current_partition++) { > + for (current_partition = 1; > + current_partition < pipe->partitions; > + current_partition++) { > struct vsp1_dl_list *dl; > > /* > @@ -415,7 +414,7 @@ static void vsp1_video_pipeline_run(struct vsp1_pipeline > *pipe) break; > } > > - vsp1_video_pipeline_run_partition(pipe, dl); > + vsp1_video_pipeline_run_partition(pipe, dl, current_partition); > vsp1_dl_list_add_chain(pipe->dl, dl); > } -- Regards, Laurent Pinchart
