[PATCH v5 07/19] media: vsp1: dl: Support one-shot entries in the display list
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Mar 19 10:00:27 UTC 2019
Hi Brian,
On Mon, Mar 18, 2019 at 04:59:21PM +0000, Brian Starkey wrote:
> Hi Laurent,
>
> Sorry for the delay, I was travelling last week and didn't find a
> chance to digest your diagrams (thanks for all the detail!)
No worries, and thank you for the time you're spending on this. In the
meantime I've found a solution that solves the race, using an entirely
different mechanism. It's all explained in v6 of the patch series.
> On Fri, Mar 08, 2019 at 02:24:40PM +0200, Laurent Pinchart wrote:
> > On Thu, Mar 07, 2019 at 12:28:08PM +0000, Brian Starkey wrote:
> > > On Wed, Mar 06, 2019 at 08:22:44PM +0200, Laurent Pinchart wrote:
> > >
> > > [snip]
> > >
> > > > I can always queue a new one, but I have no way of telling if the newly
> > > > queued list raced with the frame end interrupt.
> > > >
> > > > There's another register I'm not using that contains a shadow copy of
> > > > the registers list DMA address. It mirrors the address programmed by the
> > > > driver when there is no DMA of the registers list in progress, and
> > > > contains the address the of registers list being DMA'ed when a DMA is in
> > > > progress. I don't think I can do much with it, as reading it either
> > > > before or after reprogramming (to check if I can reprogram or if the
> > > > reprogram has been taken into account) is racy.
> > >
> > > When you say it mirrors the address programmed, is that latched when
> > > the update is accepted, or it will update the moment you program the
> > > address register?
> >
> > It is latched when the update is processed, at the next vblank following
> > the address programming. The timing diagram below shows vblank, the UPD
> > bit I use for synchronization, the registers list address register
> > exposed to the CPU, and the internal address used to DMA the registers
> > list. The address register is written four times to show how the
> > hardware reacts, but in practice there will be a single write per frame,
> > right after the beginning of vblank.
> >
> > DMA starts
> > | DMA ends
> > | |
> > V V
> > ___________
> > VBLANK ______________________| |________
> > ________________ ________________
> > UPD _____| |___|
> > _____ ______ _____________ ________ _______
> > ADDR register __A__X__B___X______C______X___D____X___E___
> > ______________________ ____________________
> > ADDR internal ___________A__________X__________C_________
> >
> > I can reprogram the address any number of times I want before the
> > vblank, but the update bit mechanism only lets me protect the race
> > related to the first write. For any subsequent write I won't be able to
> > tell whether it was complete before the hardware started the frame, or
> > arrived too late.
> >
> > > I assume the latter or you would have thought of this yourself (that
> > > seems like a really strange/not-useful behaviour!). But if it is the
> > > former you could:
> > >
> > > - In writeback start-of-frame, create a copy of the register list,
> > > disabling writeback.
> > > - Write the address of this copy to the register
> > >
> > > If/when an atomic commit comes in before you service the next
> > > end-of-frame:
> > >
> > > - Write the address of the new register list
> > > - Check the status register. If the "pending" bit is zero, you know
> > > you had a potential race.
> > > - Check the DMA address register. If it corresponds to the new
> > > scene, the new commit won the race, otherwise it's been delayed
> > > by a frame.
> >
> > The issue here is that there's a potential race if the pending (UPD) bit
> > is one too. If the commit arrives just before vblank, but the address is
> > written just after vblank, after the UPD bit has been reset to 0 by the
> > hardware, but before the vblank interrupt is processed, then the commit
> > won't be applied yet, and I will have no way to know. Compare the two
> > following scenarios:
> >
> > [1] [2] [3] [4] [5]
> > | | | | |
> > | V V | V
> > V __________ V __________
> > VBLANK _______________| |________________| |__
> > _________ _______________________
> > UPD _____| |___| |_____________
> > _____ _____________ _____________ _______________________
> > ADDR register __A__X______B______X______C______X___________D___________
> > ___________________________________________ _____________
> > ADDR internal _______A_______X______B____________________X______D______
> >
> > [1] Atomic commit, registers list address write, with writeback enabled
> > [2] DMA starts for first atomic commit
> > [3] Writeback disable registers list address write
> > [4] Next atomic commit, with writeback disabled
> > [5] DMA starts for second atomic commit
> >
> > [1] [2] [3] [4][5]
> > | | | | |
> > | V V V V
> > V __________ __________
> > VBLANK _______________| |________________| |__
> > _________ _______________________ __________
> > UPD _____| |___| |__|
> > _____ _____________ __________________________ __________
> > ADDR register __A__X______B______X_____________C____________X_____D____
> > ___________________________________________ _____________
> > ADDR internal _______A_______X______B____________________X______C______
> >
> > [1] Atomic commit, registers list address write, with writeback enabled
> > [2] DMA starts for first atomic commit
> > [3] Writeback disable registers list address write
> > [4] DMA starts for writeback disable registers list (3)
> > [5] Next atomic commit, with writeback disabled, performed right after
> > vblank but befrore the vblank interrupt is serviced
> >
> > The UPD bit is 1 after writing the ADDR register the second time in both
> > cases. Furthermore, if [4] and [5] are very close in the second case,
> > the UPD bit may read 1 just before [5] if the read comes before [4]:
> >
> > read UPD bit;
> > /* VBLANK [4] */
> > write ADDR register;
> >
> > I thus can't rely on UPD = 1 before the write meaning that the write was
> > performed before vblank, and I can't rely either on the UPD bit after
> > write, as it's 1 in both cases.
>
> My mistake, I got the UPD bit the wrong way around. I'm still not
> entirely sure why you can't use "ADDR internal" to determine which
> side won the race. It shows 'B' in the first case, and 'C' in the
> second.
Because ADDR internal isn't available to the CPU :-(
> When a new commit comes, unconditionally:
> - Write new address
> - Read status
>
> if status.UPD == 0 --> You know for sure your new commit was just
> latched.
> if status.UPD == 1 --> You need to check ADDR internal to see which
> of these three happened:
>
> 1) Your first case happened. We're somewhere in the middle of the
> frame. ADDR internal will show 'B', and you know commit 'D' is
> going on-screen at the next vblank.
>
> 2) Your second case happened. The new commit raced with the
> latching of writeback-disable and "lost". ADDR internal will
> show 'C', and the new commit is delayed by a frame
>
> 3) (Teeny tiny small window) In-between reading status and ADDR
> internal, the new commit was latched. ADDR internal will show
> 'D'. You know the new commit "won" so treat it the same as if
> UPD == 0 (which it will be, now).
>
> Anyway, it's all moot now that you've found the chained lists thing -
> that sounds ideal. I'll take a look at the new series shortly.
It's a neat hardware feature, yes. We were already using it for a
different purpose, I should have thought about it for writeback too from
the very beginning.
Please note I've sent a pull request for v7 as it has been fully
reviewed. Nonetheless, if you find issues, I can fix them on top.
> > I initially thought I could protect against the race using the following
> > procedure.
> >
> > - Single atomic commit, no IRQ delay
> >
> > [1] [2] [3] [4]
> > | | | |
> > | V V V
> > V __________ __________
> > VBLANK _______________| |________________| |__
> > _________
> > UPD _____| |_________________________________________
> > _____ ___________________________________________________
> > ADDR register __A__X___________________B_______________________________
> > _______________ _________________________________________
> > ADDR internal _______A_______X______B__________________________________
> >
> > [1] Atomic commit, registers list address write, with writeback enabled
> > [2] DMA starts for first atomic commit
> > [3] Frame start, disable writeback in-place in registers list B
> > [4] DMA starts for "patched" registers list, disables writeback
> >
> > - Two atomic commits, no IRQ delay
> >
> > [1] [2] [3] [4] [5]
> > | | | | |
> > | V V V V
> > V __________ __________
> > VBLANK _______________| |________________| |__
> > _________ ______________________
> > UPD _____| |____| |_____________
> > _____ ______________ ____________________________________
> > ADDR register __A__X______B_______X_________________C__________________
> > _______________ ___________________________ _____________
> > ADDR internal _______A_______X_____________B_____________X______C______
> >
> > [1] Atomic commit, registers list address write, with writeback enabled
> > [2] DMA starts for first atomic commit
> > [3] Next atomic commit, registers list address write, with writeback
> > disabled
> > [4] Frame start, disable writeback in-place in registers list B
> > [5] DMA starts for second atomic commit, disables writeback
> >
> > [3] and [4] can happen in any order, as long as they both come before
> > [5]. If [3] comes after [5], we're back to the previous case (Single
> > atomic commit, no IRQ delay).
> >
> > - Single atomic commit, IRQ delay
> >
> > [1] [2] [3] [4] [5]
> > | | | | |
> > | V V V V
> > V __________ __________
> > VBLANK _______________| |________________| |__
> > _________
> > UPD _____| |_________________________________________
> > _____ ___________________________________________________
> > ADDR register __A__X___________________B_______________________________
> > _______________ _________________________________________
> > ADDR internal _______A_______X______B__________________________________
> >
> > [1] Atomic commit, registers list address write, with writeback enabled
> > [2] DMA starts for first atomic commit
> > [3] Frame start, IRQ is delayed to [5]
> > [4] DMA starts for unmodified registers list, writeback still enable
> > [5] disable writeback in-place in registers list B, too late
> >
> > Here I need to detect that [5] was delayed after [4], and thus delay the
> > completion of the writeback job by one frame. This could be done by
> > checking the vblank interrupt status bit, if it is set then vblank
> > occurred and raced [5]. However, the same issue can also happen when no
> > race occurred if processing of the vblank interrupt for [2] is delayed
> > until [3]. Both the vblank interrupt and the frame start interrupt
> > status bits will be set, indicate a potential race.
> >
> > The time between [2] and [3] is very short compared to the time between
> > [3] and [4] and to interrupt latency in general, so we would have lots
> > of false positives.
> >
> > > >> You don't happen to have a DMA engine trigger or something you could
> > > >> use to do the register list modification at a guaranteed time do you?
> > > >
> > > > Not that I know of, unfortunately.
> > > >
> > > >> Are you always going to be protected by an IOMMU, preventing the
> > > >> writeback from trashing physical memory? If that's not the case, then
> > > >> the race can have pretty dire consequences.
> > > >
> > > > If the IOMMU is enabled in DT, yes. It's a system-level decision.
> > >
> > > Well, it's your driver at the end of the day. But for me, a known
> > > race-condition which would cause random memory corruption sounds like
> > > a really Bad Thing. Halving frame-rate on systems with no IOMMU seems
> > > preferable to me.
> >
> > It is a really bad thing. I think the decision should be taken by
> > Renesas though.
--
Regards,
Laurent Pinchart
More information about the dri-devel
mailing list