[PATCH v5 07/19] media: vsp1: dl: Support one-shot entries in the display list

[Laurent Pinchart]

Hi Liviu,

On Fri, Mar 08, 2019 at 03:02:39PM +0000, Liviu Dudau wrote:
> On Fri, Mar 08, 2019 at 02:46:08PM +0200, Laurent Pinchart wrote:
> > On Thu, Mar 07, 2019 at 04:31:40PM +0000, Liviu Dudau wrote:
> >> On Thu, Mar 07, 2019 at 03:48:23PM +0200, Laurent Pinchart wrote:
> >>> On Thu, Mar 07, 2019 at 11:52:18AM +0000, Liviu Dudau wrote:
> >>>> On Wed, Mar 06, 2019 at 08:01:53PM +0200, Laurent Pinchart wrote:
> >>>>> On Wed, Mar 06, 2019 at 02:20:51PM +0000, Liviu Dudau wrote:
> >>>>>> On Wed, Mar 06, 2019 at 01:14:40AM +0200, Laurent Pinchart wrote:
> > 
> > [snip]
> > 
> >>>>>>> We thus have three states for an atomic commit:
> >>>>>>> 
> >>>>>>> - active, where the corresponding registers list address has been
> >>>>>>>   written to the hardware, and processed
> >>>>>>> 
> >>>>>>> - queued, where the corresponding registers list address has been
> >>>>>>>   written to the hardware but not processed yet
> >>>>>>> 
> >>>>>>> - pending, where the corresponding registers list address hasn't been
> >>>>>>>   written to the hardware yet
> >>>>>>> 
> >>>>>>> The status bit mentioned above allows us to tell if a list exists in the
> >>>>>>> queued state.
> >>>>>>> 
> >>>>>>> At frame end time, if the status bit is set, we have potentially lost
> >>>>>>> the race between writing the new registers list and the frame end
> >>>>>>> interrupt, so we wait for one more vblank. Otherwise, if a list was
> >>>>>>> queued, we move it to the active state, and retire the active list. If a
> >>>>>>> list was pending, we write its address to the hardware, and move it to
> >>>>>>> the queued state.
> >>>> 
> >>>> It looks to me like the moving of the pending state into queued state is your
> >>>> opportunity to also in-place modify the writeback registers if the state does
> >>>> not have its own writeback request.
> >>> 
> >>> Moving from pending to queued means the pointer has been given to the
> >>> hardware, but not processed yet. I need to wait until the commit that
> >>> enables writeback is fully processed before modifying it in-place to
> >>> disable writeback, and that's at the frame start following the move from
> >>> the queued state to the active state.
> >> 
> >> I'm not attempting to (re)write your driver, only to explain my thinking process in
> >> a way that is easiest for me:
> > 
> > I wouldn't mind if you attempted to rewrite the driver if it ended up in
> > a better state :-)
> > 
> >> 1. driver prepares a new commit that might have a writeback and sets the
> >> pointer register to the new address. It then marks the commit as queued.
> >> 
> >> (optional) 2. driver receives a new commit that is marked as pending
> >> 
> >> 3. end-of-frame interrupt arrives
> >>      a. HW reads the new address and programs a DMA transfer to update registers.
> >>      b. driver reads the status bit and waits until status == 0.
> > 
> > Step b is not needed as the status bit is set to 0 as soon as the
> > hardware starts the DMA.
> 
> OK, however because I'm not sure when the driver's interrupt handler
> is called, I was trying to be safe and make sure the driver can
> proceed.
> 
> >>      c. driver marks queued commit as active and looks if it has any pending commits
> >>           - if yes, it looks at the pending commit if it has a writeback request
> >> 	        - if no, it updates the pending commit to write the register(s) that disable writeback
> >> 		- moves pending commit to queued
> >> 	  - if no, then it copies active commit and disables writeback. (*)
> > 
> > Pending commits are very rare. Userspace will usually queue the next
> > commit when it receives notification of completion of the previous
> > commit, so the next commit will arrive after vblank, with a single
> > commit per frame interval. At that point there will be an active commit,
> > and no queued commit, and the new commit will become the queued commit.
> > The "no" case will thus be hit in the vast majority of cases, preventing
> > the next userspace commit to be queued for the same frame, delaying it
> > by one frame, and thus halving the frame rate :-(
> 
> I'll guess you're referring to the (*) case of "no", but I'm not sure how the next
> userspace commit will be delayed ..... wait a bit .... a light bulb turned on
> somewhere in the darkness of my mind .... see below.
> 
> >>      d. driver signals vblank and any previous writebacks that might have been programmed
> >>         by the previously active commit
> >> 
> >> 4. ....
> >> 5. profit!
> >> 
> >> 
> >> (*) depending on how the HW behaves, it might be enough to create a stub
> >>     commit that only disables the writeback, with no other update.
> > 
> > A stub is enough, there's no need to reprogram everything.
> 
> Right, in that case, can you not look at the queued commit when the next userspace
> commit comes in and if it is the stub commit overwrite it with your new commit as
> if you're at step c. ?

I can't safely overwrite a queued commit, that's my core issue. Once a
commit is queued to the hardware, it can start being processed at any
time, and that could race with overwriting the commit. The hardware
doesn't give me a way to know if the overwrite took place before the
queued commit was processed, or afterwards.

I have however found a way to fix my issue in what I believe is a safe
way, and I've posted a v6 of the series. In a nutshell, the hardware
allows chaining registers lists. I can thus create a registers lists
with writeback enabled, and a second one that disables the writeback.
The two lists are chained, and the first one is queued to the hardware.
When the hardware finishes processing it, it automatically moves to the
chained list for the next frame, without software intervention. As the
chained list is not queued separately it doesn't interfere with the
mechanism that allows me to test if a list is queued before queuing the
next commit, so I can queue the next commit and it will take over the
chained list if it hasn't been processed yet.

> >> This way writeback commits will always be followed by another commit,
> >> even if it is a dummy one that only disables writeback.

-- 
Regards,

Laurent Pinchart