Hi Kieran,
On Wed, Mar 13, 2019 at 10:59:18AM +0000, Kieran Bingham wrote:
> On 13/03/2019 00:05, Laurent Pinchart wrote:
> > Add support for the writeback feature of the WPF, to enable capturing
> > frames at the WPF output for display pipelines. To enable writeback the
> > vsp1_rwpf structure mem field must be set to the address of the
> > writeback buffer and the writeback field set to true before the WPF
> > .configure_stream() and .configure_partition() are called. The WPF will
> > enable writeback in the display list for a single frame, and writeback
> > will then be automatically disabled.
>
> This looks good.
>
> Took some time to go through it while I argue with myself, but I think I
> reached an agreement with me in the end :)
>
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@xxxxxxxxxxxxxxxx>
>
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx>
> > ---
> > drivers/media/platform/vsp1/vsp1_rwpf.h | 2 +
> > drivers/media/platform/vsp1/vsp1_wpf.c | 73 ++++++++++++++++++++++---
> > 2 files changed, 66 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/media/platform/vsp1/vsp1_rwpf.h b/drivers/media/platform/vsp1/vsp1_rwpf.h
> > index 70742ecf766f..910990b27617 100644
> > --- a/drivers/media/platform/vsp1/vsp1_rwpf.h
> > +++ b/drivers/media/platform/vsp1/vsp1_rwpf.h
> > @@ -35,6 +35,7 @@ struct vsp1_rwpf {
> > struct v4l2_ctrl_handler ctrls;
> >
> > struct vsp1_video *video;
> > + bool has_writeback;
> >
> > unsigned int max_width;
> > unsigned int max_height;
> > @@ -61,6 +62,7 @@ struct vsp1_rwpf {
> > } flip;
> >
> > struct vsp1_rwpf_memory mem;
> > + bool writeback;
>
> Does this need to be initialised to false somewhere?
>
> answering my own question;
> No - because we allocate the "struct vsp1_rwpf *wpf" with devm_kzalloc.
>
> > struct vsp1_dl_manager *dlm;
> > };
> > diff --git a/drivers/media/platform/vsp1/vsp1_wpf.c b/drivers/media/platform/vsp1/vsp1_wpf.c
> > index fc5c1b0f6633..390ac478336d 100644
> > --- a/drivers/media/platform/vsp1/vsp1_wpf.c
> > +++ b/drivers/media/platform/vsp1/vsp1_wpf.c
> > @@ -232,6 +232,27 @@ static void vsp1_wpf_destroy(struct vsp1_entity *entity)
> > vsp1_dlm_destroy(wpf->dlm);
> > }
> >
> > +static int wpf_configure_writeback_chain(struct vsp1_rwpf *wpf,
> > + struct vsp1_dl_list *dl)
> > +{
> > + unsigned int index = wpf->entity.index;
> > + struct vsp1_dl_list *dl_next;
> > + struct vsp1_dl_body *dlb;
> > +
> > + dl_next = vsp1_dl_list_get(wpf->dlm);> + if (!dl_next) {
> > + dev_err(wpf->entity.vsp1->dev,
> > + "Failed to obtain a dl list, disabling writeback\n");
> > + return -ENOMEM;
> > + }
> > +
> > + dlb = vsp1_dl_list_get_body0(dl_next);
> > + vsp1_dl_body_write(dlb, VI6_WPF_WRBCK_CTRL(index), 0);
> > + vsp1_dl_list_add_chain(dl, dl_next);
>
> Two thoughts for future consideration.
>
> 1) There was a patch I had floated to reduce the allocations of the pool
> sizes. This would need to be checked if it's ever reconsidered, as we
> now use an extra DL.
>
> This is currently allocated as 64 lists in vsp1_wpf_create() with:
>
> wpf->dlm = vsp1_dlm_create(vsp1, index, 64);
>
> which is actually 65 lists because there's a + 1 in vsp1_dlm_create(),
> so I think we have more than we'll ever need for a display pipeline
> currently.
>
>
> 2) I did think we could pre-allocate this write back display list and
> re-use it, by always attaching the same "writeback disable display list"
> to the chain.
>
> If we do that - we'll have to be careful about the refcounting of the
> chained list as it will automatically be put back on to the dlm->free
> list currently when the frame completes.
Yes, and that's why I haven't done so yet. I think it can be implemented
on top of this series as it's an optimization.
> I think it's probably only a small optimisation to re-use the list
> anyway, so just getting a new one and chaining it is certainly adequate
> for this solution.
>
> > +
> > + return 0;
> > +}
> > +
> > static void wpf_configure_stream(struct vsp1_entity *entity,
> > struct vsp1_pipeline *pipe,
> > struct vsp1_dl_list *dl,
> > @@ -241,9 +262,11 @@ static void wpf_configure_stream(struct vsp1_entity *entity,
> > struct vsp1_device *vsp1 = wpf->entity.vsp1;
> > const struct v4l2_mbus_framefmt *source_format;
> > const struct v4l2_mbus_framefmt *sink_format;
> > + unsigned int index = wpf->entity.index;
> > unsigned int i;
> > u32 outfmt = 0;
> > u32 srcrpf = 0;
> > + int ret;
> >
> > sink_format = vsp1_entity_get_pad_format(&wpf->entity,
> > wpf->entity.config,
> > @@ -251,8 +274,9 @@ static void wpf_configure_stream(struct vsp1_entity *entity,
> > source_format = vsp1_entity_get_pad_format(&wpf->entity,
> > wpf->entity.config,
> > RWPF_PAD_SOURCE);
> > +
> > /* Format */
> > - if (!pipe->lif) {
> > + if (!pipe->lif || wpf->writeback) {
> > const struct v4l2_pix_format_mplane *format = &wpf->format;
> > const struct vsp1_format_info *fmtinfo = wpf->fmtinfo;
> >
> > @@ -277,8 +301,7 @@ static void wpf_configure_stream(struct vsp1_entity *entity,
> >
> > vsp1_wpf_write(wpf, dlb, VI6_WPF_DSWAP, fmtinfo->swap);
> >
> > - if (vsp1_feature(vsp1, VSP1_HAS_WPF_HFLIP) &&
> > - wpf->entity.index == 0)
> > + if (vsp1_feature(vsp1, VSP1_HAS_WPF_HFLIP) && index == 0)
> > vsp1_wpf_write(wpf, dlb, VI6_WPF_ROT_CTRL,
> > VI6_WPF_ROT_CTRL_LN16 |
> > (256 << VI6_WPF_ROT_CTRL_LMEM_WD_SHIFT));
> > @@ -289,11 +312,9 @@ static void wpf_configure_stream(struct vsp1_entity *entity,
> >
> > wpf->outfmt = outfmt;
> >
> > - vsp1_dl_body_write(dlb, VI6_DPR_WPF_FPORCH(wpf->entity.index),
> > + vsp1_dl_body_write(dlb, VI6_DPR_WPF_FPORCH(index),
> > VI6_DPR_WPF_FPORCH_FP_WPFN);
> >
> > - vsp1_dl_body_write(dlb, VI6_WPF_WRBCK_CTRL(wpf->entity.index), 0);
> > -
> > /*
> > * Sources. If the pipeline has a single input and BRx is not used,
> > * configure it as the master layer. Otherwise configure all
> > @@ -319,9 +340,26 @@ static void wpf_configure_stream(struct vsp1_entity *entity,
> > vsp1_wpf_write(wpf, dlb, VI6_WPF_SRCRPF, srcrpf);
> >
> > /* Enable interrupts. */
> > - vsp1_dl_body_write(dlb, VI6_WPF_IRQ_STA(wpf->entity.index), 0);
> > - vsp1_dl_body_write(dlb, VI6_WPF_IRQ_ENB(wpf->entity.index),
> > + vsp1_dl_body_write(dlb, VI6_WPF_IRQ_STA(index), 0);
> > + vsp1_dl_body_write(dlb, VI6_WPF_IRQ_ENB(index),
> > VI6_WFP_IRQ_ENB_DFEE);
> > +
> > + /*
> > + * Configure writeback for display pipelines (the wpf writeback flag is
> > + * never set for memory-to-memory pipelines). Start by adding a chained
> > + * display list to disable writeback after a single frame, and process
> > + * to enable writeback. If the display list allocation fails don't
> > + * enable writeback as we wouldn't be able to safely disable it,
> > + * resulting in possible memory corruption.
> > + */
> > + if (wpf->writeback) {
> > + ret = wpf_configure_writeback_chain(wpf, dl);
> > + if (ret < 0)
> > + wpf->writeback = false;
> > + }
> > +
> > + vsp1_dl_body_write(dlb, VI6_WPF_WRBCK_CTRL(index),
> > + wpf->writeback ? VI6_WPF_WRBCK_CTRL_WBMD : 0);
> > }
> >
> > static void wpf_configure_frame(struct vsp1_entity *entity,
> > @@ -391,7 +429,11 @@ static void wpf_configure_partition(struct vsp1_entity *entity,
> > (0 << VI6_WPF_SZCLIP_OFST_SHIFT) |
> > (height << VI6_WPF_SZCLIP_SIZE_SHIFT));
> >
> > - if (pipe->lif)
> > + /*
> > + * For display pipelines without writeback enabled there's no memory
> > + * address to configure, return now.
> > + */
> > + if (pipe->lif && !wpf->writeback)
> > return;
> >
> > /*
> > @@ -480,6 +522,12 @@ static void wpf_configure_partition(struct vsp1_entity *entity,
> > vsp1_wpf_write(wpf, dlb, VI6_WPF_DSTM_ADDR_Y, mem.addr[0]);
> > vsp1_wpf_write(wpf, dlb, VI6_WPF_DSTM_ADDR_C0, mem.addr[1]);
> > vsp1_wpf_write(wpf, dlb, VI6_WPF_DSTM_ADDR_C1, mem.addr[2]);
> > +
> > + /*
> > + * Writeback operates in single-shot mode and lasts for a single frame,
> > + * reset the writeback flag to false for the next frame.
> > + */
> > + wpf->writeback = false;
> > }
> >
> > static unsigned int wpf_max_width(struct vsp1_entity *entity,
> > @@ -530,6 +578,13 @@ struct vsp1_rwpf *vsp1_wpf_create(struct vsp1_device *vsp1, unsigned int index)
> > wpf->max_height = WPF_GEN3_MAX_HEIGHT;
> > }
> >
> > + /*
> > + * On Gen3 WPFs with a LIF output can also write to memory for display
> > + * writeback.
> > + */
> > + if (vsp1->info->gen > 2 && index < vsp1->info->lif_count)
> > + wpf->has_writeback = true;
> > +
> > wpf->entity.ops = &wpf_entity_ops;
> > wpf->entity.type = VSP1_ENTITY_WPF;
> > wpf->entity.index = index;
--
Regards,
Laurent Pinchart
