Hi Jacopo,
On Tue, Jun 18, 2024 at 12:45:42PM +0200, Jacopo Mondi wrote:
> On Wed, Nov 22, 2023 at 06:29:59AM GMT, Laurent Pinchart wrote:
> > The entity .configure_partition() function operates on a partition, and
> > has to retrieve that partition from the pipeline's current partition
> > field. Pass the partition pointer to the function to make it clearer
> > what partition it operates on, and remove the vsp1_pipeline.partition
> > field.
> >
> > This change clearly shows that the DRM pipeline doesn't use partitions,
> > which makes entity implementation more complex and error-prone. This
> > will be addressed in a further cleanup.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx>
> > ---
> > drivers/media/platform/renesas/vsp1/vsp1_drm.c | 2 +-
> > drivers/media/platform/renesas/vsp1/vsp1_entity.c | 4 +++-
> > drivers/media/platform/renesas/vsp1/vsp1_entity.h | 2 ++
> > drivers/media/platform/renesas/vsp1/vsp1_pipe.h | 2 --
> > drivers/media/platform/renesas/vsp1/vsp1_rpf.c | 5 +++--
> > drivers/media/platform/renesas/vsp1/vsp1_uds.c | 2 +-
> > drivers/media/platform/renesas/vsp1/vsp1_video.c | 5 ++---
> > drivers/media/platform/renesas/vsp1/vsp1_wpf.c | 5 +++--
> > 8 files changed, 15 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/media/platform/renesas/vsp1/vsp1_drm.c b/drivers/media/platform/renesas/vsp1/vsp1_drm.c
> > index 9b087bd8df7d..3954c138fa7b 100644
> > --- a/drivers/media/platform/renesas/vsp1/vsp1_drm.c
> > +++ b/drivers/media/platform/renesas/vsp1/vsp1_drm.c
> > @@ -569,7 +569,7 @@ static void vsp1_du_pipeline_configure(struct vsp1_pipeline *pipe)
> > vsp1_entity_route_setup(entity, pipe, dlb);
> > vsp1_entity_configure_stream(entity, pipe, dl, dlb);
> > vsp1_entity_configure_frame(entity, pipe, dl, dlb);
> > - vsp1_entity_configure_partition(entity, pipe, dl, dlb);
> > + vsp1_entity_configure_partition(entity, pipe, NULL, dl, dlb);
>
> Are was passing a NULL pointer to rpf_configure_partition() and
> wpf_configure_partition() now ? Will this commit break bisection ?
I don't think it will, becase pipe->partition is set in
vsp1_video_pipeline_run_partition() only, which is not called for DRM
pipelines. Entities already get a NULL pipe->partition in DRM pipelines,
so this patch doesn't change the behaviour.
> Can't you temporary keep 'struct vsp1_pipeline.partition' around and
> pass it down here ?
>
> > }
> >
> > vsp1_dl_list_commit(dl, dl_flags);
> > diff --git a/drivers/media/platform/renesas/vsp1/vsp1_entity.c b/drivers/media/platform/renesas/vsp1/vsp1_entity.c
> > index 8d39f1ee00ab..e9de75de8bde 100644
> > --- a/drivers/media/platform/renesas/vsp1/vsp1_entity.c
> > +++ b/drivers/media/platform/renesas/vsp1/vsp1_entity.c
> > @@ -89,11 +89,13 @@ void vsp1_entity_configure_frame(struct vsp1_entity *entity,
> >
> > void vsp1_entity_configure_partition(struct vsp1_entity *entity,
> > struct vsp1_pipeline *pipe,
> > + const struct vsp1_partition *partition,
> > struct vsp1_dl_list *dl,
> > struct vsp1_dl_body *dlb)
> > {
> > if (entity->ops->configure_partition)
> > - entity->ops->configure_partition(entity, pipe, dl, dlb);
> > + entity->ops->configure_partition(entity, pipe, partition,
> > + dl, dlb);
> > }
> >
> > /* -----------------------------------------------------------------------------
> > diff --git a/drivers/media/platform/renesas/vsp1/vsp1_entity.h b/drivers/media/platform/renesas/vsp1/vsp1_entity.h
> > index 802c0c2acab0..7b86b2fef3e5 100644
> > --- a/drivers/media/platform/renesas/vsp1/vsp1_entity.h
> > +++ b/drivers/media/platform/renesas/vsp1/vsp1_entity.h
> > @@ -85,6 +85,7 @@ struct vsp1_entity_operations {
> > struct vsp1_dl_list *, struct vsp1_dl_body *);
> > void (*configure_partition)(struct vsp1_entity *,
> > struct vsp1_pipeline *,
> > + const struct vsp1_partition *,
> > struct vsp1_dl_list *,
> > struct vsp1_dl_body *);
> > unsigned int (*max_width)(struct vsp1_entity *, struct vsp1_pipeline *);
> > @@ -155,6 +156,7 @@ void vsp1_entity_configure_frame(struct vsp1_entity *entity,
> >
> > void vsp1_entity_configure_partition(struct vsp1_entity *entity,
> > struct vsp1_pipeline *pipe,
> > + const struct vsp1_partition *partition,
> > struct vsp1_dl_list *dl,
> > struct vsp1_dl_body *dlb);
> >
> > diff --git a/drivers/media/platform/renesas/vsp1/vsp1_pipe.h b/drivers/media/platform/renesas/vsp1/vsp1_pipe.h
> > index 840fd3288efb..3d2e35ac8fa0 100644
> > --- a/drivers/media/platform/renesas/vsp1/vsp1_pipe.h
> > +++ b/drivers/media/platform/renesas/vsp1/vsp1_pipe.h
> > @@ -106,7 +106,6 @@ struct vsp1_partition {
> > * @configured: when false the @stream_config shall be written to the hardware
> > * @interlaced: True when the pipeline is configured in interlaced mode
> > * @partitions: The number of partitions used to process one frame
> > - * @partition: The current partition for configuration to process
> > * @part_table: The pre-calculated partitions used by the pipeline
> > */
> > struct vsp1_pipeline {
> > @@ -146,7 +145,6 @@ struct vsp1_pipeline {
> > bool interlaced;
> >
> > unsigned int partitions;
> > - struct vsp1_partition *partition;
> > struct vsp1_partition *part_table;
> >
> > u32 underrun_count;
> > diff --git a/drivers/media/platform/renesas/vsp1/vsp1_rpf.c b/drivers/media/platform/renesas/vsp1/vsp1_rpf.c
> > index 42b0c5655404..3b8a62299226 100644
> > --- a/drivers/media/platform/renesas/vsp1/vsp1_rpf.c
> > +++ b/drivers/media/platform/renesas/vsp1/vsp1_rpf.c
> > @@ -280,6 +280,7 @@ static void rpf_configure_frame(struct vsp1_entity *entity,
> >
> > static void rpf_configure_partition(struct vsp1_entity *entity,
> > struct vsp1_pipeline *pipe,
> > + const struct vsp1_partition *partition,
> > struct vsp1_dl_list *dl,
> > struct vsp1_dl_body *dlb)
> > {
> > @@ -311,8 +312,8 @@ static void rpf_configure_partition(struct vsp1_entity *entity,
> > * 'width' need to be adjusted.
> > */
> > if (pipe->partitions > 1) {
> > - crop.width = pipe->partition->rpf[rpf->entity.index].width;
> > - crop.left += pipe->partition->rpf[rpf->entity.index].left;
> > + crop.width = partition->rpf[rpf->entity.index].width;
> > + crop.left += partition->rpf[rpf->entity.index].left;
> > }
> >
> > if (pipe->interlaced) {
> > diff --git a/drivers/media/platform/renesas/vsp1/vsp1_uds.c b/drivers/media/platform/renesas/vsp1/vsp1_uds.c
> > index 887b1f70611a..737362ca2315 100644
> > --- a/drivers/media/platform/renesas/vsp1/vsp1_uds.c
> > +++ b/drivers/media/platform/renesas/vsp1/vsp1_uds.c
> > @@ -300,11 +300,11 @@ static void uds_configure_stream(struct vsp1_entity *entity,
> >
> > static void uds_configure_partition(struct vsp1_entity *entity,
> > struct vsp1_pipeline *pipe,
> > + const struct vsp1_partition *partition,
> > struct vsp1_dl_list *dl,
> > struct vsp1_dl_body *dlb)
> > {
> > struct vsp1_uds *uds = to_uds(&entity->subdev);
> > - struct vsp1_partition *partition = pipe->partition;
> > const struct v4l2_mbus_framefmt *output;
> >
> > output = v4l2_subdev_state_get_format(uds->entity.state,
> > diff --git a/drivers/media/platform/renesas/vsp1/vsp1_video.c b/drivers/media/platform/renesas/vsp1/vsp1_video.c
> > index ea5773af54d6..6a8db541543a 100644
> > --- a/drivers/media/platform/renesas/vsp1/vsp1_video.c
> > +++ b/drivers/media/platform/renesas/vsp1/vsp1_video.c
> > @@ -240,13 +240,12 @@ static void vsp1_video_pipeline_run_partition(struct vsp1_pipeline *pipe,
> > struct vsp1_dl_list *dl,
> > unsigned int partition)
> > {
> > + struct vsp1_partition *part = &pipe->part_table[partition];
> > struct vsp1_dl_body *dlb = vsp1_dl_list_get_body0(dl);
> > struct vsp1_entity *entity;
> >
> > - pipe->partition = &pipe->part_table[partition];
> > -
> > list_for_each_entry(entity, &pipe->entities, list_pipe)
> > - vsp1_entity_configure_partition(entity, pipe, dl, dlb);
> > + vsp1_entity_configure_partition(entity, pipe, part, dl, dlb);
> > }
> >
> > static void vsp1_video_pipeline_run(struct vsp1_pipeline *pipe)
> > diff --git a/drivers/media/platform/renesas/vsp1/vsp1_wpf.c b/drivers/media/platform/renesas/vsp1/vsp1_wpf.c
> > index 5129181b8217..80fe7571f4ff 100644
> > --- a/drivers/media/platform/renesas/vsp1/vsp1_wpf.c
> > +++ b/drivers/media/platform/renesas/vsp1/vsp1_wpf.c
> > @@ -363,6 +363,7 @@ static void wpf_configure_frame(struct vsp1_entity *entity,
> >
> > static void wpf_configure_partition(struct vsp1_entity *entity,
> > struct vsp1_pipeline *pipe,
> > + const struct vsp1_partition *partition,
> > struct vsp1_dl_list *dl,
> > struct vsp1_dl_body *dlb)
> > {
> > @@ -390,8 +391,8 @@ static void wpf_configure_partition(struct vsp1_entity *entity,
> > * multiple slices.
> > */
> > if (pipe->partitions > 1) {
> > - width = pipe->partition->wpf.width;
> > - left = pipe->partition->wpf.left;
> > + width = partition->wpf.width;
> > + left = partition->wpf.left;
> > }
> >
> > vsp1_wpf_write(wpf, dlb, VI6_WPF_HSZCLIP, VI6_WPF_SZCLIP_EN |
--
Regards,
Laurent Pinchart
