On Fri, Feb 22, 2019 at 04:46:29PM +0200, Laurent Pinchart wrote:
> Hi Brian,
>
> On Fri, Feb 22, 2019 at 02:30:03PM +0000, Brian Starkey wrote:
> > On Thu, Feb 21, 2019 at 12:32:00PM +0200, Laurent Pinchart wrote:
> > > One-shot entries are used as an alternative to committing a complete new
> > > display list when a couple of registers need to be written for one frame
> > > and then reset to another value for all subsequent frames. This will be
> > > used to implement writeback support that will need to enable writeback
> > > for the duration of a single frame.
> > >
> > > Signed-off-by: Laurent Pinchart
> > > <laurent.pinchart+rene...@ideasonboard.com>
> > > ---
> > > drivers/media/platform/vsp1/vsp1_dl.c | 78 +++++++++++++++++++++++++++
> > > drivers/media/platform/vsp1/vsp1_dl.h | 3 ++
> > > 2 files changed, 81 insertions(+)
> > >
> > > diff --git a/drivers/media/platform/vsp1/vsp1_dl.c
> > > b/drivers/media/platform/vsp1/vsp1_dl.c
> > > index 886b3a69d329..7b4d252bfde7 100644
> > > --- a/drivers/media/platform/vsp1/vsp1_dl.c
> > > +++ b/drivers/media/platform/vsp1/vsp1_dl.c
> > > @@ -115,6 +115,12 @@ struct vsp1_dl_body {
> > >
> > > unsigned int num_entries;
> > > unsigned int max_entries;
> > > +
> > > + unsigned int num_patches;
> > > + struct {
> > > + struct vsp1_dl_entry *entry;
> > > + u32 data;
> > > + } patches[2];
> > > };
> > >
> > > /**
> > > @@ -361,6 +367,7 @@ void vsp1_dl_body_put(struct vsp1_dl_body *dlb)
> > > return;
> > >
> > > dlb->num_entries = 0;
> > > + dlb->num_patches = 0;
> > >
> > > spin_lock_irqsave(&dlb->pool->lock, flags);
> > > list_add_tail(&dlb->free, &dlb->pool->free);
> > > @@ -388,6 +395,47 @@ void vsp1_dl_body_write(struct vsp1_dl_body *dlb,
> > > u32 reg, u32 data)
> > > dlb->num_entries++;
> > > }
> > >
> > > +/**
> > > + * vsp1_dl_body_write_oneshot - Write a register to a display list body
> > > for a
> > > + * single frame
> > > + * @dlb: The body
> > > + * @reg: The register address
> > > + * @value: The register value
> > > + * @reset_value: The value to reset the register to at the next vblank
> > > + *
> > > + * Display lists in continuous mode are re-used by the hardware for
> > > successive
> > > + * frames until a new display list is committed. Changing the VSP
> > > configuration
> > > + * normally requires creating and committing a new display list. This
> > > function
> > > + * offers an alternative race-free way by writing a @value to the
> > > @register in
> > > + * the display list body for a single frame, specifying in @reset_value
> > > the
> > > + * value to reset the register to one vblank after the display list is
> > > + * committed.
> > > + *
> > > + * The maximum number of one-shot entries is limited to 2 per display
> > > list body,
> > > + * and one-shot entries are counted in the total number of entries
> > > specified
> > > + * when the body is allocated by vsp1_dl_body_alloc().
> > > + */
> > > +void vsp1_dl_body_write_oneshot(struct vsp1_dl_body *dlb, u32 reg, u32
> > > value,
> > > + u32 reset_value)
> > > +{
> > > + if (WARN_ONCE(dlb->num_entries >= dlb->max_entries,
> > > + "DLB size exceeded (max %u)", dlb->max_entries))
> > > + return;
> > > +
> > > + if (WARN_ONCE(dlb->num_patches >= ARRAY_SIZE(dlb->patches),
> > > + "DLB patches size exceeded (max %zu)",
> > > + ARRAY_SIZE(dlb->patches)))
> > > + return;
> > > +
> > > + dlb->patches[dlb->num_patches].entry = &dlb->entries[dlb->num_entries];
> > > + dlb->patches[dlb->num_patches].data = reset_value;
> > > + dlb->num_patches++;
> > > +
> > > + dlb->entries[dlb->num_entries].addr = reg;
> > > + dlb->entries[dlb->num_entries].data = value;
> > > + dlb->num_entries++;
> > > +}
> > > +
> > > /*
> > > -----------------------------------------------------------------------------
> > > * Display List Extended Command Management
> > > */
> > > @@ -652,6 +700,7 @@ static void __vsp1_dl_list_put(struct vsp1_dl_list
> > > *dl)
> > > * has at least one body, thus we reinitialise the entries list.
> > > */
> > > dl->body0->num_entries = 0;
> > > + dl->body0->num_patches = 0;
> > >
> > > list_add_tail(&dl->list, &dl->dlm->free);
> > > }
> > > @@ -930,6 +979,35 @@ void vsp1_dl_list_commit(struct vsp1_dl_list *dl,
> > > unsigned int dl_flags)
> > > * Display List Manager
> > > */
> > >
> > > +/**
> > > + * vsp1_dlm_irq_display_start - Display list handler for the display
> > > start
> > > + * interrupt
> > > + * @dlm: the display list manager
> > > + *
> > > + * Apply all one-shot patches registered for the active display list.
> > > + */
> > > +void vsp1_dlm_irq_display_start(struct vsp1_dl_manager *dlm)
> > > +{
> > > + struct vsp1_dl_body *dlb;
> > > + struct vsp1_dl_list *dl;
> > > + unsigned int i;
> > > +
> > > + spin_lock(&dlm->lock);
> > > +
> > > + dl = dlm->active;
> > > + if (!dl)
> > > + goto done;
> > > +
> > > + list_for_each_entry(dlb, &dl->bodies, list) {
> > > + for (i = 0; i < dlb->num_patches; ++i)
> > > + dlb->patches[i].entry->data = dlb->patches[i].data;
> > > + dlb->num_patches = 0;
> > > + }
> > > +
> > > +done:
> > > + spin_unlock(&dlm->lock);
> > > +}
> > > +
> >
> > We've got some HW which doesn't support one-shot writeback, and use a
> > similar trick to try and disable writeback immediately after the flip.
> >
> > We ran into issues where the "start" interrupt wouldn't run in time to
> > make sure the writeback disable was committed before the next frame.
> > We have to keep track of whether the disable really happened in time,
> > before we release the output buffer.
> >
> > Might you have a similar problem here?
>
> We may, but there's no provision at the hardware level to check if the
> configuration updated happened in time. I could add some safety checks
> but I believe they would be racy in the best case :-(
We managed to find (what I believe to be...) a non-racy way, but it
will of course depend a lot on the HW behaviour, so I'll leave it to
your best judgement.
We basically have a "configuration committed" interrupt which we can
use to set a flag indicating writeback was disabled.
>
> Note that we have the duration of a complete frame to disable writeback,
> as we receive an interrupt when the frame starts, and have until vblank
> to update the configuration. It's thus slightly better than having to
> disable writeback between vblank and the start of the next frame.
Yeah... we have a whole frame too. I'm struggling to find our wiki
page with the data, but anecdotally there's some (out-of-tree) drivers
which keep interrupts masked for a _really long_ time. It's nice if
you don't have to care about those :-)
-Brian
>
> > > /**
> > > * vsp1_dlm_irq_frame_end - Display list handler for the frame end
> > > interrupt
> > > * @dlm: the display list manager
> > > diff --git a/drivers/media/platform/vsp1/vsp1_dl.h
> > > b/drivers/media/platform/vsp1/vsp1_dl.h
> > > index e0fdb145e6ed..f845607abc4c 100644
> > > --- a/drivers/media/platform/vsp1/vsp1_dl.h
> > > +++ b/drivers/media/platform/vsp1/vsp1_dl.h
> > > @@ -54,6 +54,7 @@ struct vsp1_dl_manager *vsp1_dlm_create(struct
> > > vsp1_device *vsp1,
> > > unsigned int prealloc);
> > > void vsp1_dlm_destroy(struct vsp1_dl_manager *dlm);
> > > void vsp1_dlm_reset(struct vsp1_dl_manager *dlm);
> > > +void vsp1_dlm_irq_display_start(struct vsp1_dl_manager *dlm);
> > > unsigned int vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm);
> > > struct vsp1_dl_body *vsp1_dlm_dl_body_get(struct vsp1_dl_manager *dlm);
> > >
> > > @@ -71,6 +72,8 @@ struct vsp1_dl_body *vsp1_dl_body_get(struct
> > > vsp1_dl_body_pool *pool);
> > > void vsp1_dl_body_put(struct vsp1_dl_body *dlb);
> > >
> > > void vsp1_dl_body_write(struct vsp1_dl_body *dlb, u32 reg, u32 data);
> > > +void vsp1_dl_body_write_oneshot(struct vsp1_dl_body *dlb, u32 reg, u32
> > > value,
> > > + u32 reset_value);
> > > int vsp1_dl_list_add_body(struct vsp1_dl_list *dl, struct vsp1_dl_body
> > > *dlb);
> > > int vsp1_dl_list_add_chain(struct vsp1_dl_list *head, struct
> > > vsp1_dl_list *dl);
> > >
>
> --
> Regards,
>
> Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
