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

[Liviu Dudau]

On Fri, Mar 08, 2019 at 02:46:08PM +0200, Laurent Pinchart wrote:
> Hi Liviu,
> 
> 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. ?

Best regards,
Liviu

> 
> > 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

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯