Hi Jacopo,
On Tue, Jun 18, 2024 at 01:07:40PM +0200, Jacopo Mondi wrote:
> On Wed, Nov 22, 2023 at 06:30:00AM GMT, Laurent Pinchart wrote:
> > The vsp1_partition_window structure is used to store the horizontal size
> > of a partition window. This is all that is currently needed, as all
> > partitions span the whole image vertically. The horizontal window size
> > is retrieved in the .configure_partition() handler from the
> > vsp1_partition_window structure, and the vertical window size from the
> > subdev state.
> >
> > Accessing the subdev state in the .configure_partition() handler is
> > problematic in the context of moving to the V4L2 subdev active state
> > API, as .configure_partition() is called in non-interruptable context,
> > and the state lock can't be taken. To avoid this, start by storing the
> > vertical size in the window, replacing the custom vsp1_partition_window
> > structure with a v4l2_rect. Retrieving the vertical size from the window
> > in .configure_partition() will be done in a subsequent change.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx>
> > ---
> > .../media/platform/renesas/vsp1/vsp1_entity.h | 3 +--
> > .../media/platform/renesas/vsp1/vsp1_pipe.c | 6 ++++--
> > .../media/platform/renesas/vsp1/vsp1_pipe.h | 21 +++++--------------
> > .../media/platform/renesas/vsp1/vsp1_rpf.c | 2 +-
> > .../media/platform/renesas/vsp1/vsp1_sru.c | 4 +++-
> > .../media/platform/renesas/vsp1/vsp1_uds.c | 10 ++++-----
> > .../media/platform/renesas/vsp1/vsp1_wpf.c | 2 +-
> > 7 files changed, 20 insertions(+), 28 deletions(-)
> >
> > diff --git a/drivers/media/platform/renesas/vsp1/vsp1_entity.h b/drivers/media/platform/renesas/vsp1/vsp1_entity.h
> > index 7b86b2fef3e5..f67f60677644 100644
> > --- a/drivers/media/platform/renesas/vsp1/vsp1_entity.h
> > +++ b/drivers/media/platform/renesas/vsp1/vsp1_entity.h
> > @@ -19,7 +19,6 @@ struct vsp1_dl_body;
> > struct vsp1_dl_list;
> > struct vsp1_pipeline;
> > struct vsp1_partition;
> > -struct vsp1_partition_window;
> >
> > enum vsp1_entity_type {
> > VSP1_ENTITY_BRS,
> > @@ -91,7 +90,7 @@ struct vsp1_entity_operations {
> > unsigned int (*max_width)(struct vsp1_entity *, struct vsp1_pipeline *);
> > void (*partition)(struct vsp1_entity *, struct vsp1_pipeline *,
> > struct vsp1_partition *, unsigned int,
> > - struct vsp1_partition_window *);
> > + struct v4l2_rect *);
> > };
> >
> > struct vsp1_entity {
> > diff --git a/drivers/media/platform/renesas/vsp1/vsp1_pipe.c b/drivers/media/platform/renesas/vsp1/vsp1_pipe.c
> > index ca6817f45dd2..8eba3cda1e3d 100644
> > --- a/drivers/media/platform/renesas/vsp1/vsp1_pipe.c
> > +++ b/drivers/media/platform/renesas/vsp1/vsp1_pipe.c
> > @@ -459,7 +459,7 @@ void vsp1_pipeline_propagate_alpha(struct vsp1_pipeline *pipe,
> > static void vsp1_pipeline_propagate_partition(struct vsp1_pipeline *pipe,
> > struct vsp1_partition *partition,
> > unsigned int index,
> > - struct vsp1_partition_window *window)
> > + struct v4l2_rect *window)
> > {
> > struct vsp1_entity *entity;
> >
> > @@ -485,7 +485,7 @@ void vsp1_pipeline_calculate_partition(struct vsp1_pipeline *pipe,
> > unsigned int index)
> > {
> > const struct v4l2_mbus_framefmt *format;
> > - struct vsp1_partition_window window;
> > + struct v4l2_rect window;
> > unsigned int modulus;
> >
> > /*
> > @@ -498,6 +498,8 @@ void vsp1_pipeline_calculate_partition(struct vsp1_pipeline *pipe,
> > /* Initialise the partition with sane starting conditions. */
> > window.left = index * div_size;
> > window.width = div_size;
> > + window.top = 0;
> > + window.height = format->height;
> >
> > modulus = format->width % div_size;
> >
> > diff --git a/drivers/media/platform/renesas/vsp1/vsp1_pipe.h b/drivers/media/platform/renesas/vsp1/vsp1_pipe.h
> > index 3d2e35ac8fa0..c1f411227de7 100644
> > --- a/drivers/media/platform/renesas/vsp1/vsp1_pipe.h
> > +++ b/drivers/media/platform/renesas/vsp1/vsp1_pipe.h
> > @@ -53,17 +53,6 @@ enum vsp1_pipeline_state {
> > VSP1_PIPELINE_STOPPING,
> > };
> >
> > -/*
> > - * struct vsp1_partition_window - Partition window coordinates
> > - * @left: horizontal coordinate of the partition start in pixels relative to the
> > - * left edge of the image
> > - * @width: partition width in pixels
> > - */
> > -struct vsp1_partition_window {
> > - unsigned int left;
> > - unsigned int width;
> > -};
> > -
> > /*
> > * struct vsp1_partition - A description of a slice for the partition algorithm
> > * @rpf: The RPF partition window configuration
> > @@ -73,11 +62,11 @@ struct vsp1_partition_window {
> > * @wpf: The WPF partition window configuration
> > */
> > struct vsp1_partition {
> > - struct vsp1_partition_window rpf[VSP1_MAX_RPF];
> > - struct vsp1_partition_window uds_sink;
> > - struct vsp1_partition_window uds_source;
> > - struct vsp1_partition_window sru;
> > - struct vsp1_partition_window wpf;
> > + struct v4l2_rect rpf[VSP1_MAX_RPF];
> > + struct v4l2_rect uds_sink;
> > + struct v4l2_rect uds_source;
> > + struct v4l2_rect sru;
> > + struct v4l2_rect wpf;
> > };
> >
> > /*
> > diff --git a/drivers/media/platform/renesas/vsp1/vsp1_rpf.c b/drivers/media/platform/renesas/vsp1/vsp1_rpf.c
> > index 3b8a62299226..862751616646 100644
> > --- a/drivers/media/platform/renesas/vsp1/vsp1_rpf.c
> > +++ b/drivers/media/platform/renesas/vsp1/vsp1_rpf.c
> > @@ -366,7 +366,7 @@ static void rpf_partition(struct vsp1_entity *entity,
> > struct vsp1_pipeline *pipe,
> > struct vsp1_partition *partition,
> > unsigned int partition_idx,
> > - struct vsp1_partition_window *window)
> > + struct v4l2_rect *window)
> > {
> > struct vsp1_rwpf *rpf = to_rwpf(&entity->subdev);
> >
> > diff --git a/drivers/media/platform/renesas/vsp1/vsp1_sru.c b/drivers/media/platform/renesas/vsp1/vsp1_sru.c
> > index 2c67aae0d5fd..f35187daa643 100644
> > --- a/drivers/media/platform/renesas/vsp1/vsp1_sru.c
> > +++ b/drivers/media/platform/renesas/vsp1/vsp1_sru.c
> > @@ -324,7 +324,7 @@ static void sru_partition(struct vsp1_entity *entity,
> > struct vsp1_pipeline *pipe,
> > struct vsp1_partition *partition,
> > unsigned int partition_idx,
> > - struct vsp1_partition_window *window)
> > + struct v4l2_rect *window)
> > {
> > struct vsp1_sru *sru = to_sru(&entity->subdev);
> > struct v4l2_mbus_framefmt *input;
> > @@ -339,6 +339,8 @@ static void sru_partition(struct vsp1_entity *entity,
> > if (input->width != output->width) {
> > window->width /= 2;
> > window->left /= 2;
> > + window->height /= 2;
> > + window->top /= 2;
>
> If partitions vertically span the whole image, is this required ?
I believe so. The .partition() operation is called iteratively on all
entities in the pipeline, and each entity is expected to update the
window. When the SRU upscales by two, the input size is half of the
output size. At the moment the entities upstream of the SRU don't use
the window's vertical size, but that will change (if I recall correctly)
in further patches. In any case it's best to keep the information
correct even when it's not used (except if we decide the information
will never be used, in which case we should drop it, but that's not the
case here).
> > }
> >
> > partition->sru = *window;
> > diff --git a/drivers/media/platform/renesas/vsp1/vsp1_uds.c b/drivers/media/platform/renesas/vsp1/vsp1_uds.c
> > index 737362ca2315..4a14fd3baac1 100644
> > --- a/drivers/media/platform/renesas/vsp1/vsp1_uds.c
> > +++ b/drivers/media/platform/renesas/vsp1/vsp1_uds.c
> > @@ -363,16 +363,12 @@ static void uds_partition(struct vsp1_entity *entity,
> > struct vsp1_pipeline *pipe,
> > struct vsp1_partition *partition,
> > unsigned int partition_idx,
> > - struct vsp1_partition_window *window)
> > + struct v4l2_rect *window)
> > {
> > struct vsp1_uds *uds = to_uds(&entity->subdev);
> > const struct v4l2_mbus_framefmt *output;
> > const struct v4l2_mbus_framefmt *input;
> >
> > - /* Initialise the partition state. */
> > - partition->uds_sink = *window;
> > - partition->uds_source = *window;
> > -
> > input = v4l2_subdev_state_get_format(uds->entity.state, UDS_PAD_SINK);
> > output = v4l2_subdev_state_get_format(uds->entity.state,
> > UDS_PAD_SOURCE);
> > @@ -381,6 +377,10 @@ static void uds_partition(struct vsp1_entity *entity,
> > / output->width;
> > partition->uds_sink.left = window->left * input->width
> > / output->width;
> > + partition->uds_sink.height = input->height;
> > + partition->uds_sink.top = 0;
> > +
> > + partition->uds_source = *window;
> >
> > *window = partition->uds_sink;
> > }
> > diff --git a/drivers/media/platform/renesas/vsp1/vsp1_wpf.c b/drivers/media/platform/renesas/vsp1/vsp1_wpf.c
> > index 80fe7571f4ff..f8d1e2f47691 100644
> > --- a/drivers/media/platform/renesas/vsp1/vsp1_wpf.c
> > +++ b/drivers/media/platform/renesas/vsp1/vsp1_wpf.c
> > @@ -515,7 +515,7 @@ static void wpf_partition(struct vsp1_entity *entity,
> > struct vsp1_pipeline *pipe,
> > struct vsp1_partition *partition,
> > unsigned int partition_idx,
> > - struct vsp1_partition_window *window)
> > + struct v4l2_rect *window)
> > {
> > partition->wpf = *window;
> > }
--
Regards,
Laurent Pinchart
