Hi Kieran,
On Thu, Mar 14, 2019 at 08:28:27AM +0000, Kieran Bingham wrote:
> On 13/03/2019 15:56, Laurent Pinchart wrote:
> > On Wed, Mar 13, 2019 at 11:42:34AM +0000, Kieran Bingham wrote:
> >> On 13/03/2019 00:05, Laurent Pinchart wrote:
> >>> Extend the vsp1_du_atomic_flush() API with writeback support by adding
> >>> format, pitch and memory addresses of the writeback framebuffer.
> >>> Writeback completion is reported through the existing frame completion
> >>> callback with a new VSP1_DU_STATUS_WRITEBACK status flag.
> >>>
> >>> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx>
>
> My concerns have been addressed here:
>
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@xxxxxxxxxxxxxxxx>
>
> >>> ---
> >>> drivers/media/platform/vsp1/vsp1_dl.c | 14 ++++++++++++++
> >>> drivers/media/platform/vsp1/vsp1_dl.h | 3 ++-
> >>> drivers/media/platform/vsp1/vsp1_drm.c | 25 ++++++++++++++++++++++++-
> >>> include/media/vsp1.h | 15 +++++++++++++++
> >>> 4 files changed, 55 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/media/platform/vsp1/vsp1_dl.c b/drivers/media/platform/vsp1/vsp1_dl.c
> >>> index ed7cda4130f2..104b6f514536 100644
> >>> --- a/drivers/media/platform/vsp1/vsp1_dl.c
> >>> +++ b/drivers/media/platform/vsp1/vsp1_dl.c
> >>> @@ -958,6 +958,9 @@ void vsp1_dl_list_commit(struct vsp1_dl_list *dl, unsigned int dl_flags)
> >>> *
> >>> * The VSP1_DL_FRAME_END_INTERNAL flag indicates that the display list that just
> >>> * became active had been queued with the internal notification flag.
> >>> + *
> >>> + * The VSP1_DL_FRAME_END_WRITEBACK flag indicates that the previously active
> >>> + * display list had been queued with the writeback flag.
> >>
> >> How does this interact with the possibility of the writeback being
> >> disabled by the WPF in the event of it failing to get a DL.
> >>
> >> It's only a small corner case, but will the 'writeback' report back as
> >> though it succeeded? (without writing to memory, and thus giving an
> >> unmodified buffer back?)
> >
> > Wrteback completion will never be reported in that case. This shouldn't
> > happen as we should never fail to get a display list. Do you think it
> > would be better to fake completion ?
>
> Would this lack of completion cause a hang while DRM waits for the
> completion to occur? I guess this would timeout after some period.
Not in the kernel as far as I can tell, but userspace could then wait
forever.
> I'm not sure what's worse. To hang / block for a timeout - or just
> return a 'bad buffer'. We would know in the VSP that the completion has
> failed, so we could signal a failure, but I think as the rest of the DRM
> model goes with a timeout if the flip_done fails to complete for
> example, we should follow that.
>
> So leave this as is, and perhaps lets make sure the core writeback
> framework will report a warning if it hits a time out.
There's no timeout handling for writeback in the core.
> If we ever fail to get a display list - we will have bigger issues which
> will propogate elsewhere :)
Yes, I think so too. This should really never happen.
> >>> */
> >>> unsigned int vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm)
> >>> {
> >>> @@ -995,6 +998,17 @@ unsigned int vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm)
> >>> if (status & VI6_STATUS_FLD_STD(dlm->index))
> >>> goto done;
> >>>
> >>> + /*
> >>> + * If the active display list has the writeback flag set, the frame
> >>> + * completion marks the end of the writeback capture. Return the
> >>> + * VSP1_DL_FRAME_END_WRITEBACK flag and reset the display list's
> >>> + * writeback flag.
> >>> + */
> >>> + if (dlm->active && (dlm->active->flags & VSP1_DL_FRAME_END_WRITEBACK)) {
> >>> + flags |= VSP1_DL_FRAME_END_WRITEBACK;
> >>> + dlm->active->flags &= ~VSP1_DL_FRAME_END_WRITEBACK;
> >>> + }
> >>> +
> >>> /*
> >>> * The device starts processing the queued display list right after the
> >>> * frame end interrupt. The display list thus becomes active.
> >>> diff --git a/drivers/media/platform/vsp1/vsp1_dl.h b/drivers/media/platform/vsp1/vsp1_dl.h
> >>> index e0fdb145e6ed..4d7bcfdc9bd9 100644
> >>> --- a/drivers/media/platform/vsp1/vsp1_dl.h
> >>> +++ b/drivers/media/platform/vsp1/vsp1_dl.h
> >>> @@ -18,7 +18,8 @@ struct vsp1_dl_list;
> >>> struct vsp1_dl_manager;
> >>>
> >>> #define VSP1_DL_FRAME_END_COMPLETED BIT(0)
> >>> -#define VSP1_DL_FRAME_END_INTERNAL BIT(1)
> >>> +#define VSP1_DL_FRAME_END_WRITEBACK BIT(1)
> >>
> >> So below BIT(2) (code above) the flags match the externally exposed
> >> bitfield for the VSP1_DU_STATUS_
> >>
> >> While above (code below), are 'private' bitfields.
> >>
> >> Should this requirement be documented here somehow? especially the
> >> mapping of FRAME_END_{COMPLETED,WRITEBACK} to
> >> DU_STATUS_{COMPLETED,WRITEBACK}.
> >
> > I've added a comment here, as explained in my reply to your review of
> > 10/18, to document this.
>
> Great.
>
> >>> +#define VSP1_DL_FRAME_END_INTERNAL BIT(2)
> >>>
> >>> /**
> >>> * struct vsp1_dl_ext_cmd - Extended Display command
> >>> diff --git a/drivers/media/platform/vsp1/vsp1_drm.c b/drivers/media/platform/vsp1/vsp1_drm.c
> >>> index 0367f88135bf..16826bf184c7 100644
> >>> --- a/drivers/media/platform/vsp1/vsp1_drm.c
> >>> +++ b/drivers/media/platform/vsp1/vsp1_drm.c
> >>> @@ -37,7 +37,9 @@ static void vsp1_du_pipeline_frame_end(struct vsp1_pipeline *pipe,
> >>>
> >>> if (drm_pipe->du_complete) {
> >>> struct vsp1_entity *uif = drm_pipe->uif;
> >>> - unsigned int status = completion & VSP1_DU_STATUS_COMPLETE;
> >>> + unsigned int status = completion
> >>> + & (VSP1_DU_STATUS_COMPLETE |
> >>> + VSP1_DU_STATUS_WRITEBACK);
> >>> u32 crc;
> >>>
> >>> crc = uif ? vsp1_uif_get_crc(to_uif(&uif->subdev)) : 0;
> >>> @@ -541,6 +543,8 @@ static void vsp1_du_pipeline_configure(struct vsp1_pipeline *pipe)
> >>>
> >>> if (drm_pipe->force_brx_release)
> >>> dl_flags |= VSP1_DL_FRAME_END_INTERNAL;
> >>> + if (pipe->output->writeback)
> >>> + dl_flags |= VSP1_DL_FRAME_END_WRITEBACK;
> >>>
> >>> dl = vsp1_dl_list_get(pipe->output->dlm);
> >>> dlb = vsp1_dl_list_get_body0(dl);
> >>> @@ -870,12 +874,31 @@ void vsp1_du_atomic_flush(struct device *dev, unsigned int pipe_index,
> >>> struct vsp1_device *vsp1 = dev_get_drvdata(dev);
> >>> struct vsp1_drm_pipeline *drm_pipe = &vsp1->drm->pipe[pipe_index];
> >>> struct vsp1_pipeline *pipe = &drm_pipe->pipe;
> >>> + int ret;
> >>>
> >>> drm_pipe->crc = cfg->crc;
> >>>
> >>> mutex_lock(&vsp1->drm->lock);
> >>> +
> >>> + if (pipe->output->has_writeback && cfg->writeback.pixelformat) {
> >>
> >> Is pipe->output->has_writeback necessary here? Can
> >> cfg->writeback.pixelformat be set if pipe->output->has_writeback is false?
> >>
> >> Hrm ... actually - Perhaps it is useful. It validates both sides of the
> >> system.
> >>
> >> pipe->output->has_writeback is a capability of the VSP, where as
> >> cfg->writeback.pixelformat is a 'request' from the DU.
> >
> > Correct, I think it's best to check both, to ensure we don't try to
> > queue a writeback request on a system that doesn't support writeback. On
> > the other hand this shouldn't happen as the DU driver shouldn't expose
> > writeback to userspace in that case, so if you don't think the check is
> > worth it I can remove the has_writeback field completely.
>
> It's a cheap check, I don't think it is too much of an issue - but I
> agree (if we don't already) then we should make sure userspace does not
> see a writeback functionality if it is not supported through the whole
> pipeline (i.e. including the capability in the VSP1).
>
> That would make me lean towards removing this check here - *iff* we
> guarantee that the VSP will only be asked to do write back when it's
> possible.
Unless there's a bug in the DU side, we have such a guarantee. I'll
remove the check.
> >>> + const struct vsp1_du_writeback_config *wb_cfg = &cfg->writeback;
> >>> +
> >>> + ret = vsp1_du_pipeline_set_rwpf_format(vsp1, pipe->output,
> >>> + wb_cfg->pixelformat,
> >>> + wb_cfg->pitch);
> >>> + if (WARN_ON(ret < 0))
> >>> + goto done;
> >>> +
> >>> + pipe->output->mem.addr[0] = wb_cfg->mem[0];
> >>> + pipe->output->mem.addr[1] = wb_cfg->mem[1];
> >>> + pipe->output->mem.addr[2] = wb_cfg->mem[2];
> >>> + pipe->output->writeback = true;
> >>> + }
> >>> +
> >>> vsp1_du_pipeline_setup_inputs(vsp1, pipe);
> >>> vsp1_du_pipeline_configure(pipe);
> >>> +
> >>> +done:
> >>> mutex_unlock(&vsp1->drm->lock);
> >>> }
> >>> EXPORT_SYMBOL_GPL(vsp1_du_atomic_flush);
> >>> diff --git a/include/media/vsp1.h b/include/media/vsp1.h
> >>> index 877496936487..cc1b0d42ce95 100644
> >>> --- a/include/media/vsp1.h
> >>> +++ b/include/media/vsp1.h
> >>> @@ -18,6 +18,7 @@ struct device;
> >>> int vsp1_du_init(struct device *dev);
> >>>
> >>> #define VSP1_DU_STATUS_COMPLETE BIT(0)
> >>> +#define VSP1_DU_STATUS_WRITEBACK BIT(1)
> >>>
> >>> /**
> >>> * struct vsp1_du_lif_config - VSP LIF configuration
> >>> @@ -83,12 +84,26 @@ struct vsp1_du_crc_config {
> >>> unsigned int index;
> >>> };
> >>>
> >>> +/**
> >>> + * struct vsp1_du_writeback_config - VSP writeback configuration parameters
> >>> + * @pixelformat: plane pixel format (V4L2 4CC)
> >>> + * @pitch: line pitch in bytes for the first plane
> >>> + * @mem: DMA memory address for each plane of the frame buffer
> >>> + */
> >>> +struct vsp1_du_writeback_config {
> >>> + u32 pixelformat;
> >>> + unsigned int pitch;
> >>> + dma_addr_t mem[3];
> >>> +};
> >>> +
> >>> /**
> >>> * struct vsp1_du_atomic_pipe_config - VSP atomic pipe configuration parameters
> >>> * @crc: CRC computation configuration
> >>> + * @writeback: writeback configuration
> >>> */
> >>> struct vsp1_du_atomic_pipe_config {
> >>> struct vsp1_du_crc_config crc;
> >>> + struct vsp1_du_writeback_config writeback;
> >>> };
> >>>
> >>> void vsp1_du_atomic_begin(struct device *dev, unsigned int pipe_index);
> >
>
> --
> Regards
> --
> Kieran
--
Regards,
Laurent Pinchart
