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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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.

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

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

> 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel