| Message ID | 20190313000532.7087-12-laurent.pinchart+renesas@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | R-Car DU display writeback support | expand |
Hi Laurent, 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@ideasonboard.com> > --- > 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?) > */ > 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}. > +#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. > + 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); >
Hi Kieran, 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@ideasonboard.com> > > --- > > 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 ? > > */ > > 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. > > +#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. > > + 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);
Hi Laurent, On 13/03/2019 15:56, Laurent Pinchart wrote: > Hi Kieran, > > 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@ideasonboard.com> My concerns have been addressed here: Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> >>> --- >>> 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. 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. If we ever fail to get a display list - we will have bigger issues which will propogate elsewhere :) >>> */ >>> 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. >>> + 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); >
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@ideasonboard.com> > > My concerns have been addressed here: > > Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> > > >>> --- > >>> 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
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. */ 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) +#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) { + 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);
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@ideasonboard.com> --- 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(-)