Correct sequencing of usage of DRM writeback connector

Correct sequencing of usage of DRM writeback connector

[Hoosier, Matt]

Hi,

There is a discussion ongoing over in the compositor world about the implication of this cautionary wording found in the documentation for the DRM_MODE_CONNECTOR_WRITEBACK connectors:

>  *  "WRITEBACK_OUT_FENCE_PTR":
>  *	Userspace can use this property to provide a pointer for the kernel to
>  *	fill with a sync_file file descriptor, which will signal once the
>  *	writeback is finished. The value should be the address of a 32-bit
>  *	signed integer, cast to a u64.
>  *	Userspace should wait for this fence to signal before making another
>  *	commit affecting any of the same CRTCs, Planes or Connectors.
>  *	**Failure to do so will result in undefined behaviour.**
>  *	For this reason it is strongly recommended that all userspace
>  *	applications making use of writeback connectors *always* retrieve an
>  *	out-fence for the commit and use it appropriately.
>  *	From userspace, this property will always read as zero.

The question is whether it's realistic to hope that a DRM writeback connector can produce results on every frame, and do so without dragging down the frame-rate for the connector.

The wording in the documentation above suggests that it is very likely the fence fd won't signal userspace until after the vblank following the scanout during which the writeback was applied (call that frame N). This would mean that the compositor driving the connector would typically be unable to legally queue a page flip for frame N+1.

Is this the right interpretation? Is the writeback hardware typically even designed with a streaming use-case in mind? Maybe it's just intended for occasional static screenshots.

Matt Hoosier

Re: Correct sequencing of usage of DRM writeback connector

[Daniel Vetter]

On Mon, Jun 17, 2024 at 01:41:59PM +0000, Hoosier, Matt wrote:
> Hi,
> 
> There is a discussion ongoing over in the compositor world about the implication of this cautionary wording found in the documentation for the DRM_MODE_CONNECTOR_WRITEBACK connectors:
> 
> >  *  "WRITEBACK_OUT_FENCE_PTR":
> >  *	Userspace can use this property to provide a pointer for the kernel to
> >  *	fill with a sync_file file descriptor, which will signal once the
> >  *	writeback is finished. The value should be the address of a 32-bit
> >  *	signed integer, cast to a u64.
> >  *	Userspace should wait for this fence to signal before making another
> >  *	commit affecting any of the same CRTCs, Planes or Connectors.
> >  *	**Failure to do so will result in undefined behaviour.**
> >  *	For this reason it is strongly recommended that all userspace
> >  *	applications making use of writeback connectors *always* retrieve an
> >  *	out-fence for the commit and use it appropriately.
> >  *	From userspace, this property will always read as zero.
> 
> The question is whether it's realistic to hope that a DRM writeback
> connector can produce results on every frame, and do so without dragging
> down the frame-rate for the connector.
> 
> The wording in the documentation above suggests that it is very likely
> the fence fd won't signal userspace until after the vblank following the
> scanout during which the writeback was applied (call that frame N). This
> would mean that the compositor driving the connector would typically be
> unable to legally queue a page flip for frame N+1.
> 
> Is this the right interpretation? Is the writeback hardware typically
> even designed with a streaming use-case in mind? Maybe it's just
> intended for occasional static screenshots.

So typically writeback hardware needs its separate crtc (at least the
examples I know of) and doesn't make a lot of guarantees that it's fast
enough for real time use. Since it's a separate crtc it shouldn't hold up
the main composition loop, and so this should be all fine.

If/when we have hardware and driver support where you can use the
writeback connector as a real-time streamout kind of thing, then we need
to change all this, because with the current implementation, there's
indeed the possibility that funny things can happen if you ignore the
notice (funny as in data corruption, not funny as the kernel crashes of
course).

If we already have devices where you can use writeback together with real
outputs, then I guess that counts as an oopsie :-/

Cheers, Sima
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: Correct sequencing of usage of DRM writeback connector

[Brian Starkey]

Hi,

On Mon, Jun 17, 2024 at 05:16:36PM +0200, Daniel Vetter wrote:
>On Mon, Jun 17, 2024 at 01:41:59PM +0000, Hoosier, Matt wrote:
>> Hi,
>>
>> There is a discussion ongoing over in the compositor world about the implication of this cautionary wording found in the documentation for the DRM_MODE_CONNECTOR_WRITEBACK connectors:
>>
>> >  *  "WRITEBACK_OUT_FENCE_PTR":
>> >  *	Userspace can use this property to provide a pointer for the kernel to
>> >  *	fill with a sync_file file descriptor, which will signal once the
>> >  *	writeback is finished. The value should be the address of a 32-bit
>> >  *	signed integer, cast to a u64.
>> >  *	Userspace should wait for this fence to signal before making another
>> >  *	commit affecting any of the same CRTCs, Planes or Connectors.
>> >  *	**Failure to do so will result in undefined behaviour.**
>> >  *	For this reason it is strongly recommended that all userspace
>> >  *	applications making use of writeback connectors *always* retrieve an
>> >  *	out-fence for the commit and use it appropriately.
>> >  *	From userspace, this property will always read as zero.
>>
>> The question is whether it's realistic to hope that a DRM writeback
>> connector can produce results on every frame, and do so without dragging
>> down the frame-rate for the connector.
>>
>> The wording in the documentation above suggests that it is very likely
>> the fence fd won't signal userspace until after the vblank following the
>> scanout during which the writeback was applied (call that frame N). This
>> would mean that the compositor driving the connector would typically be
>> unable to legally queue a page flip for frame N+1.
>>
>> Is this the right interpretation? Is the writeback hardware typically
>> even designed with a streaming use-case in mind? Maybe it's just
>> intended for occasional static screenshots.
>
>So typically writeback hardware needs its separate crtc (at least the
>examples I know of) and doesn't make a lot of guarantees that it's fast
>enough for real time use. Since it's a separate crtc it shouldn't hold up
>the main composition loop, and so this should be all fine.

On Mali-DP and Komeda at least, you can use writeback on the same CRTC
that is driving a "real" display, and it should generally work. If the
writeback doesn't keep up then the HW will signal an error, but it was
designed to work in-sync with real scanout, on the same pipe.

>
>If/when we have hardware and driver support where you can use the
>writeback connector as a real-time streamout kind of thing, then we need
>to change all this, because with the current implementation, there's
>indeed the possibility that funny things can happen if you ignore the
>notice (funny as in data corruption, not funny as the kernel crashes of
>course).

Indeed, the wording was added (from what I remember from so long
ago...) because it sounded like different HW made very different
guarantees/non-guarantees about what data would be written when, so
perhaps you'd end up with some pixels from the next frame in your
buffer or something.

Taking Mali-DP/Komeda again, the writeback configuration is latched
along with everything else, and writeback throughput permitting, it
should "just work" if you submit a new writeback every frame. It
drains out the last of the data during vblank, before starting on the
next frame. That doesn't help the "general case" though.

>
>If we already have devices where you can use writeback together with real
>outputs, then I guess that counts as an oopsie :-/

Well "works fine" fits into the "undefined behaviour" bucket, just as
well as "corrupts your fb" does :-)

-Brian

>
>Cheers, Sima
>-- 
>Daniel Vetter
>Software Engineer, Intel Corporation
>http://blog.ffwll.ch

RE: Correct sequencing of usage of DRM writeback connector

[Hoosier, Matt]

>> >> >> There is a discussion ongoing over in the compositor world about the implication 
>> Hi,
>>
>> On Mon, Jun 17, 2024 at 05:16:36PM +0200, Daniel Vetter wrote:
>> >On Mon, Jun 17, 2024 at 01:41:59PM +0000, Hoosier, Matt wrote:
>> >> Hi,
>> >>
>> >> There is a discussion ongoing over in the compositor world about the implication of this cautionary wording found in the documentation for the DRM_MODE_CONNECTOR_WRITEBACK connectors:
>> >>
>> >> >  *  "WRITEBACK_OUT_FENCE_PTR":
>> >> >  *	Userspace can use this property to provide a pointer for the kernel to
>> >> >  *	fill with a sync_file file descriptor, which will signal once the
>> >> >  *	writeback is finished. The value should be the address of a 32-bit
>> >> >  *	signed integer, cast to a u64.
>> >> >  *	Userspace should wait for this fence to signal before making another
>> >> >  *	commit affecting any of the same CRTCs, Planes or Connectors.
>> >> >  *	**Failure to do so will result in undefined behaviour.**
>> >> >  *	For this reason it is strongly recommended that all userspace
>> >> >  *	applications making use of writeback connectors *always* retrieve an
>> >> >  *	out-fence for the commit and use it appropriately.
>> >> >  *	From userspace, this property will always read as zero.
>> >>
>> >> The question is whether it's realistic to hope that a DRM writeback
>> >> connector can produce results on every frame, and do so without dragging
>> >> down the frame-rate for the connector.
>> >>
>> >> The wording in the documentation above suggests that it is very likely
>> >> the fence fd won't signal userspace until after the vblank following the
>> >> scanout during which the writeback was applied (call that frame N). This
>> >> would mean that the compositor driving the connector would typically be
>> >> unable to legally queue a page flip for frame N+1.
>> >>
>> >> Is this the right interpretation? Is the writeback hardware typically
>> >> even designed with a streaming use-case in mind? Maybe it's just
>> >> intended for occasional static screenshots.
>> >
>> >So typically writeback hardware needs its separate crtc (at least the
>> >examples I know of) and doesn't make a lot of guarantees that it's fast
>> >enough for real time use. Since it's a separate crtc it shouldn't hold up
>> >the main composition loop, and so this should be all fine.

Hmm, I don't think this matches the implementation. What I see -- sometimes people call this "concurrent writeback" -- is that the writeback connector is wired directly to the CRTC that's feeding the regular connector whose picture is getting captured.

Something like (for frame N):

* DP-1
  * CRTC_ID = crtc-0
* plane-0
  * CRTC_ID = crtc-0
  * FB_ID = fb-0

* Writeback-1
  * CRTC_ID = crtc-0
  * WRITEBACK_FB_ID = fb-1
  * WRITEBACK_OUT_FENCE_PTR = <whatever>


Are you saying that that for frame N+1, we should switch DP-1 to use a different CRTC while the writeback is still busy being retired into fb-1 through crtc-0?

>>
>> On Mali-DP and Komeda at least, you can use writeback on the same CRTC
>> that is driving a "real" display, and it should generally work. If the
>> writeback doesn't keep up then the HW will signal an error, but it was
>> designed to work in-sync with real scanout, on the same pipe.
>>
>> >
>> >If/when we have hardware and driver support where you can use the
>> >writeback connector as a real-time streamout kind of thing, then we need
>> >to change all this, because with the current implementation, there's
>> >indeed the possibility that funny things can happen if you ignore the
>> >notice (funny as in data corruption, not funny as the kernel crashes of
>> >course).
>>
>> Indeed, the wording was added (from what I remember from so long
>> ago...) because it sounded like different HW made very different
>> guarantees/non-guarantees about what data would be written when, so
>> perhaps you'd end up with some pixels from the next frame in your
>> buffer or something.
>>
>> Taking Mali-DP/Komeda again, the writeback configuration is latched
>> along with everything else, and writeback throughput permitting, it
>> should "just work" if you submit a new writeback every frame. It
>> drains out the last of the data during vblank, before starting on the
>> next frame. That doesn't help the "general case" though.

Are you saying that on hardware whose writeback implementation is amenable, the drivers will generally fire the fence FD in time for userspace to post a next frame to the real connector's CRTC for the immediately following frame? Or are you just saying that some hardware could support it, but that the DRM framework's insistence that userspace waits until the fence fires might still make it artificially too slow to drive the hardware to its full capacity?

I want to be a good citizen and do stuff by the book here. :-P

>>
>> >
>> >If we already have devices where you can use writeback together with real
>> >outputs, then I guess that counts as an oopsie :-/
>>
>> Well "works fine" fits into the "undefined behaviour" bucket, just as
>> well as "corrupts your fb" does :-)
>>
>> -Brian

Re: Correct sequencing of usage of DRM writeback connector

[Abhinav Kumar]

Hi

On 6/17/2024 9:54 AM, Brian Starkey wrote:
> Hi,
> 
> On Mon, Jun 17, 2024 at 05:16:36PM +0200, Daniel Vetter wrote:
>> On Mon, Jun 17, 2024 at 01:41:59PM +0000, Hoosier, Matt wrote:
>>> Hi,
>>>
>>> There is a discussion ongoing over in the compositor world about the implication of this cautionary wording found in the documentation for the DRM_MODE_CONNECTOR_WRITEBACK connectors:
>>>
>>>>   *  "WRITEBACK_OUT_FENCE_PTR":
>>>>   *	Userspace can use this property to provide a pointer for the kernel to
>>>>   *	fill with a sync_file file descriptor, which will signal once the
>>>>   *	writeback is finished. The value should be the address of a 32-bit
>>>>   *	signed integer, cast to a u64.
>>>>   *	Userspace should wait for this fence to signal before making another
>>>>   *	commit affecting any of the same CRTCs, Planes or Connectors.
>>>>   *	**Failure to do so will result in undefined behaviour.**
>>>>   *	For this reason it is strongly recommended that all userspace
>>>>   *	applications making use of writeback connectors *always* retrieve an
>>>>   *	out-fence for the commit and use it appropriately.
>>>>   *	From userspace, this property will always read as zero.
>>>
>>> The question is whether it's realistic to hope that a DRM writeback
>>> connector can produce results on every frame, and do so without dragging
>>> down the frame-rate for the connector.
>>>
>>> The wording in the documentation above suggests that it is very likely
>>> the fence fd won't signal userspace until after the vblank following the
>>> scanout during which the writeback was applied (call that frame N). This
>>> would mean that the compositor driving the connector would typically be
>>> unable to legally queue a page flip for frame N+1.
>>>
>>> Is this the right interpretation? Is the writeback hardware typically
>>> even designed with a streaming use-case in mind? Maybe it's just
>>> intended for occasional static screenshots.
>>
>> So typically writeback hardware needs its separate crtc (at least the
>> examples I know of) and doesn't make a lot of guarantees that it's fast
>> enough for real time use. Since it's a separate crtc it shouldn't hold up
>> the main composition loop, and so this should be all fine.
> 
> On Mali-DP and Komeda at least, you can use writeback on the same CRTC
> that is driving a "real" display, and it should generally work. If the
> writeback doesn't keep up then the HW will signal an error, but it was
> designed to work in-sync with real scanout, on the same pipe.
> 

Same with MSM hardware. You can use writeback with same CRTC that is 
driving a "real" display and yes we call it concurrent writeback. So I 
think it is correct in the documentation to expect to wait till this is 
signaled if the same CRTC is being used.

>>
>> If/when we have hardware and driver support where you can use the
>> writeback connector as a real-time streamout kind of thing, then we need
>> to change all this, because with the current implementation, there's
>> indeed the possibility that funny things can happen if you ignore the
>> notice (funny as in data corruption, not funny as the kernel crashes of
>> course).
> 
> Indeed, the wording was added (from what I remember from so long
> ago...) because it sounded like different HW made very different
> guarantees/non-guarantees about what data would be written when, so
> perhaps you'd end up with some pixels from the next frame in your
> buffer or something.
> 
> Taking Mali-DP/Komeda again, the writeback configuration is latched
> along with everything else, and writeback throughput permitting, it
> should "just work" if you submit a new writeback every frame. It
> drains out the last of the data during vblank, before starting on the
> next frame. That doesn't help the "general case" though.
> 

Would it be fair to summarize it like below:

1) If the same CRTC is shared with the real time display, then the 
hardware is expected to fire this every frame so userspace should wait 
till this is signaled.

2) If a different CRTC is used for the writeback, then the composition 
loop for the real time display should not block on this unless its a 
mirroring use-case, then we will be throttled by the lowest refresh rate 
anyway.

>>
>> If we already have devices where you can use writeback together with real
>> outputs, then I guess that counts as an oopsie :-/
> 
> Well "works fine" fits into the "undefined behaviour" bucket, just as
> well as "corrupts your fb" does :-)
> 
> -Brian
> 
>>
>> Cheers, Sima
>> -- 
>> Daniel Vetter
>> Software Engineer, Intel Corporation
>> http://blog.ffwll.ch

Re: Correct sequencing of usage of DRM writeback connector

[Dmitry Baryshkov]

On Mon, Jun 17, 2024 at 05:36:34PM GMT, Hoosier, Matt wrote:
> >> >> >> There is a discussion ongoing over in the compositor world about the implication 
> >> Hi,
> >>
> >> On Mon, Jun 17, 2024 at 05:16:36PM +0200, Daniel Vetter wrote:
> >> >On Mon, Jun 17, 2024 at 01:41:59PM +0000, Hoosier, Matt wrote:
> >> >> Hi,
> >> >>
> >> >> There is a discussion ongoing over in the compositor world about the implication of this cautionary wording found in the documentation for the DRM_MODE_CONNECTOR_WRITEBACK connectors:
> >> >>
> >> >> >  *  "WRITEBACK_OUT_FENCE_PTR":
> >> >> >  *	Userspace can use this property to provide a pointer for the kernel to
> >> >> >  *	fill with a sync_file file descriptor, which will signal once the
> >> >> >  *	writeback is finished. The value should be the address of a 32-bit
> >> >> >  *	signed integer, cast to a u64.
> >> >> >  *	Userspace should wait for this fence to signal before making another
> >> >> >  *	commit affecting any of the same CRTCs, Planes or Connectors.
> >> >> >  *	**Failure to do so will result in undefined behaviour.**
> >> >> >  *	For this reason it is strongly recommended that all userspace
> >> >> >  *	applications making use of writeback connectors *always* retrieve an
> >> >> >  *	out-fence for the commit and use it appropriately.
> >> >> >  *	From userspace, this property will always read as zero.
> >> >>
> >> >> The question is whether it's realistic to hope that a DRM writeback
> >> >> connector can produce results on every frame, and do so without dragging
> >> >> down the frame-rate for the connector.
> >> >>
> >> >> The wording in the documentation above suggests that it is very likely
> >> >> the fence fd won't signal userspace until after the vblank following the
> >> >> scanout during which the writeback was applied (call that frame N). This
> >> >> would mean that the compositor driving the connector would typically be
> >> >> unable to legally queue a page flip for frame N+1.
> >> >>
> >> >> Is this the right interpretation? Is the writeback hardware typically
> >> >> even designed with a streaming use-case in mind? Maybe it's just
> >> >> intended for occasional static screenshots.
> >> >
> >> >So typically writeback hardware needs its separate crtc (at least the
> >> >examples I know of) and doesn't make a lot of guarantees that it's fast
> >> >enough for real time use. Since it's a separate crtc it shouldn't hold up
> >> >the main composition loop, and so this should be all fine.
> 
> Hmm, I don't think this matches the implementation. What I see -- sometimes people call this "concurrent writeback" -- is that the writeback connector is wired directly to the CRTC that's feeding the regular connector whose picture is getting captured.
> 
> Something like (for frame N):
> 
> * DP-1
>   * CRTC_ID = crtc-0
> * plane-0
>   * CRTC_ID = crtc-0
>   * FB_ID = fb-0
> 
> * Writeback-1
>   * CRTC_ID = crtc-0
>   * WRITEBACK_FB_ID = fb-1
>   * WRITEBACK_OUT_FENCE_PTR = <whatever>
> 
> 
> Are you saying that that for frame N+1, we should switch DP-1 to use a different CRTC while the writeback is still busy being retired into fb-1 through crtc-0?
> 
> >>
> >> On Mali-DP and Komeda at least, you can use writeback on the same CRTC
> >> that is driving a "real" display, and it should generally work. If the
> >> writeback doesn't keep up then the HW will signal an error, but it was
> >> designed to work in-sync with real scanout, on the same pipe.
> >>
> >> >
> >> >If/when we have hardware and driver support where you can use the
> >> >writeback connector as a real-time streamout kind of thing, then we need
> >> >to change all this, because with the current implementation, there's
> >> >indeed the possibility that funny things can happen if you ignore the
> >> >notice (funny as in data corruption, not funny as the kernel crashes of
> >> >course).
> >>
> >> Indeed, the wording was added (from what I remember from so long
> >> ago...) because it sounded like different HW made very different
> >> guarantees/non-guarantees about what data would be written when, so
> >> perhaps you'd end up with some pixels from the next frame in your
> >> buffer or something.
> >>
> >> Taking Mali-DP/Komeda again, the writeback configuration is latched
> >> along with everything else, and writeback throughput permitting, it
> >> should "just work" if you submit a new writeback every frame. It
> >> drains out the last of the data during vblank, before starting on the
> >> next frame. That doesn't help the "general case" though.
> 
> Are you saying that on hardware whose writeback implementation is
> amenable, the drivers will generally fire the fence FD in time for
> userspace to post a next frame to the real connector's CRTC for the
> immediately following frame? Or are you just saying that some hardware
> could support it, but that the DRM framework's insistence that
> userspace waits until the fence fires might still make it artificially
> too slow to drive the hardware to its full capacity?

Current DRM uAPI for writeback supports single-frame jobs. Userspace
submits an FB, it waits for the fence, then submits next FB, etc. This
is not well-suitable (nor it is defined in a way suitable) for the
streamed operations, where you want to generate a stream of N frames per
second. Even though userspace sets a mode which includes a particular
frame rate, the WB API has no guarantee that the FPS will be or can be
met.

In my opinion a proper uAPI for the streamed Writeback should be closer
to v4l2, where you submit several buffers in advance together with the
corresponding fences and then they are filled one by one and the
corresponding fence is signalled to userspace. 

Note, in my humble opinion, it should be perfectly possible to setup
writeback as a clone to the existing connector (if a clone mode is
suppored by the hardware) and then to supply the jobs (FB+fence)
occasionally rather than for each frame. Such userspace operation should
not cause any degradation on the main connector.

> 
> I want to be a good citizen and do stuff by the book here. :-P
> 
> >>
> >> >
> >> >If we already have devices where you can use writeback together with real
> >> >outputs, then I guess that counts as an oopsie :-/
> >>
> >> Well "works fine" fits into the "undefined behaviour" bucket, just as
> >> well as "corrupts your fb" does :-)
> >>
> >> -Brian
> 

-- 
With best wishes
Dmitry

Re: Correct sequencing of usage of DRM writeback connector

[Dmitry Baryshkov]

On Mon, Jun 17, 2024 at 11:28:35AM GMT, Abhinav Kumar wrote:
> Hi
> 
> On 6/17/2024 9:54 AM, Brian Starkey wrote:
> > Hi,
> > 
> > On Mon, Jun 17, 2024 at 05:16:36PM +0200, Daniel Vetter wrote:
> > > On Mon, Jun 17, 2024 at 01:41:59PM +0000, Hoosier, Matt wrote:
> > > > Hi,
> > > > 
> > > > There is a discussion ongoing over in the compositor world about the implication of this cautionary wording found in the documentation for the DRM_MODE_CONNECTOR_WRITEBACK connectors:
> > > > 
> > > > >   *  "WRITEBACK_OUT_FENCE_PTR":
> > > > >   *	Userspace can use this property to provide a pointer for the kernel to
> > > > >   *	fill with a sync_file file descriptor, which will signal once the
> > > > >   *	writeback is finished. The value should be the address of a 32-bit
> > > > >   *	signed integer, cast to a u64.
> > > > >   *	Userspace should wait for this fence to signal before making another
> > > > >   *	commit affecting any of the same CRTCs, Planes or Connectors.
> > > > >   *	**Failure to do so will result in undefined behaviour.**
> > > > >   *	For this reason it is strongly recommended that all userspace
> > > > >   *	applications making use of writeback connectors *always* retrieve an
> > > > >   *	out-fence for the commit and use it appropriately.
> > > > >   *	From userspace, this property will always read as zero.
> > > > 
> > > > The question is whether it's realistic to hope that a DRM writeback
> > > > connector can produce results on every frame, and do so without dragging
> > > > down the frame-rate for the connector.
> > > > 
> > > > The wording in the documentation above suggests that it is very likely
> > > > the fence fd won't signal userspace until after the vblank following the
> > > > scanout during which the writeback was applied (call that frame N). This
> > > > would mean that the compositor driving the connector would typically be
> > > > unable to legally queue a page flip for frame N+1.
> > > > 
> > > > Is this the right interpretation? Is the writeback hardware typically
> > > > even designed with a streaming use-case in mind? Maybe it's just
> > > > intended for occasional static screenshots.
> > > 
> > > So typically writeback hardware needs its separate crtc (at least the
> > > examples I know of) and doesn't make a lot of guarantees that it's fast
> > > enough for real time use. Since it's a separate crtc it shouldn't hold up
> > > the main composition loop, and so this should be all fine.
> > 
> > On Mali-DP and Komeda at least, you can use writeback on the same CRTC
> > that is driving a "real" display, and it should generally work. If the
> > writeback doesn't keep up then the HW will signal an error, but it was
> > designed to work in-sync with real scanout, on the same pipe.
> > 
> 
> Same with MSM hardware. You can use writeback with same CRTC that is driving
> a "real" display and yes we call it concurrent writeback. So I think it is
> correct in the documentation to expect to wait till this is signaled if the
> same CRTC is being used.
> 
> > > 
> > > If/when we have hardware and driver support where you can use the
> > > writeback connector as a real-time streamout kind of thing, then we need
> > > to change all this, because with the current implementation, there's
> > > indeed the possibility that funny things can happen if you ignore the
> > > notice (funny as in data corruption, not funny as the kernel crashes of
> > > course).
> > 
> > Indeed, the wording was added (from what I remember from so long
> > ago...) because it sounded like different HW made very different
> > guarantees/non-guarantees about what data would be written when, so
> > perhaps you'd end up with some pixels from the next frame in your
> > buffer or something.
> > 
> > Taking Mali-DP/Komeda again, the writeback configuration is latched
> > along with everything else, and writeback throughput permitting, it
> > should "just work" if you submit a new writeback every frame. It
> > drains out the last of the data during vblank, before starting on the
> > next frame. That doesn't help the "general case" though.
> > 
> 
> Would it be fair to summarize it like below:
> 
> 1) If the same CRTC is shared with the real time display, then the hardware
> is expected to fire this every frame so userspace should wait till this is
> signaled.

As I wrote in response to another email in this thread, IMO existing
uAPI doesn't fully allow this. There is no way to enforce 'vblank'
handling onto the userspace. So userspace should be able to supply at
least two buffers and then after the vblank it should be able to enqueue
the next buffer, while the filled buffer is automatically dequeued by
the driver and is not used for further image output.

> 
> 2) If a different CRTC is used for the writeback, then the composition loop
> for the real time display should not block on this unless its a mirroring
> use-case, then we will be throttled by the lowest refresh rate anyway.

what is mirroring in this case? You have specified that a different CRTC
is being used.

> 
> > > 
> > > If we already have devices where you can use writeback together with real
> > > outputs, then I guess that counts as an oopsie :-/
> > 
> > Well "works fine" fits into the "undefined behaviour" bucket, just as
> > well as "corrupts your fb" does :-)


-- 
With best wishes
Dmitry

Re: Correct sequencing of usage of DRM writeback connector

[Brian Starkey]

Hi,

On Mon, Jun 17, 2024 at 10:48:47PM UTC, Dmitry Baryshkov wrote:
> On Mon, Jun 17, 2024 at 05:36:34PM GMT, Hoosier, Matt wrote:
> > >> >> >> There is a discussion ongoing over in the compositor world about the implication 
> > >> Hi,
> > >>
> > >> On Mon, Jun 17, 2024 at 05:16:36PM +0200, Daniel Vetter wrote:
> > >> >On Mon, Jun 17, 2024 at 01:41:59PM +0000, Hoosier, Matt wrote:
> > >> >> Hi,
> > >> >>
> > >> >> There is a discussion ongoing over in the compositor world about the implication of this cautionary wording found in the documentation for the DRM_MODE_CONNECTOR_WRITEBACK connectors:
> > >> >>
> > >> >> >  *  "WRITEBACK_OUT_FENCE_PTR":
> > >> >> >  *	Userspace can use this property to provide a pointer for the kernel to
> > >> >> >  *	fill with a sync_file file descriptor, which will signal once the
> > >> >> >  *	writeback is finished. The value should be the address of a 32-bit
> > >> >> >  *	signed integer, cast to a u64.
> > >> >> >  *	Userspace should wait for this fence to signal before making another
> > >> >> >  *	commit affecting any of the same CRTCs, Planes or Connectors.
> > >> >> >  *	**Failure to do so will result in undefined behaviour.**
> > >> >> >  *	For this reason it is strongly recommended that all userspace
> > >> >> >  *	applications making use of writeback connectors *always* retrieve an
> > >> >> >  *	out-fence for the commit and use it appropriately.
> > >> >> >  *	From userspace, this property will always read as zero.
> > >> >>
> > >> >> The question is whether it's realistic to hope that a DRM writeback
> > >> >> connector can produce results on every frame, and do so without dragging
> > >> >> down the frame-rate for the connector.
> > >> >>
> > >> >> The wording in the documentation above suggests that it is very likely
> > >> >> the fence fd won't signal userspace until after the vblank following the
> > >> >> scanout during which the writeback was applied (call that frame N). This
> > >> >> would mean that the compositor driving the connector would typically be
> > >> >> unable to legally queue a page flip for frame N+1.
> > >> >>
> > >> >> Is this the right interpretation? Is the writeback hardware typically
> > >> >> even designed with a streaming use-case in mind? Maybe it's just
> > >> >> intended for occasional static screenshots.
> > >> >
> > >> >So typically writeback hardware needs its separate crtc (at least the
> > >> >examples I know of) and doesn't make a lot of guarantees that it's fast
> > >> >enough for real time use. Since it's a separate crtc it shouldn't hold up
> > >> >the main composition loop, and so this should be all fine.
> > 
> > Hmm, I don't think this matches the implementation. What I see -- sometimes people call this "concurrent writeback" -- is that the writeback connector is wired directly to the CRTC that's feeding the regular connector whose picture is getting captured.
> > 
> > Something like (for frame N):
> > 
> > * DP-1
> >   * CRTC_ID = crtc-0
> > * plane-0
> >   * CRTC_ID = crtc-0
> >   * FB_ID = fb-0
> > 
> > * Writeback-1
> >   * CRTC_ID = crtc-0
> >   * WRITEBACK_FB_ID = fb-1
> >   * WRITEBACK_OUT_FENCE_PTR = <whatever>
> > 
> > 
> > Are you saying that that for frame N+1, we should switch DP-1 to use a different CRTC while the writeback is still busy being retired into fb-1 through crtc-0?

The "letter" of the API is that you need to wait for the Writeback-1
fence before making another commit to any of the resources - which
effectively means halving your update rate for crtc-0.

The fence might fire before vsync, so you might have a very small
window for getting a new commit in on the very next frame, but
realistically it's unlikely.

Logically, if the writeback is working line-by-line alongside scanout
then it won't have the last pixels until the end of the last line, so
it can't signal completion until at least the start of vblank.

> > 
> > >>
> > >> On Mali-DP and Komeda at least, you can use writeback on the same CRTC
> > >> that is driving a "real" display, and it should generally work. If the
> > >> writeback doesn't keep up then the HW will signal an error, but it was
> > >> designed to work in-sync with real scanout, on the same pipe.
> > >>
> > >> >
> > >> >If/when we have hardware and driver support where you can use the
> > >> >writeback connector as a real-time streamout kind of thing, then we need
> > >> >to change all this, because with the current implementation, there's
> > >> >indeed the possibility that funny things can happen if you ignore the
> > >> >notice (funny as in data corruption, not funny as the kernel crashes of
> > >> >course).
> > >>
> > >> Indeed, the wording was added (from what I remember from so long
> > >> ago...) because it sounded like different HW made very different
> > >> guarantees/non-guarantees about what data would be written when, so
> > >> perhaps you'd end up with some pixels from the next frame in your
> > >> buffer or something.
> > >>
> > >> Taking Mali-DP/Komeda again, the writeback configuration is latched
> > >> along with everything else, and writeback throughput permitting, it
> > >> should "just work" if you submit a new writeback every frame. It
> > >> drains out the last of the data during vblank, before starting on the
> > >> next frame. That doesn't help the "general case" though.
> > 
> > Are you saying that on hardware whose writeback implementation is
> > amenable, the drivers will generally fire the fence FD in time for
> > userspace to post a next frame to the real connector's CRTC for the
> > immediately following frame? Or are you just saying that some hardware
> > could support it, but that the DRM framework's insistence that
> > userspace waits until the fence fires might still make it artificially
> > too slow to drive the hardware to its full capacity?
> 

I'm saying that on Mali-DP and Komeda, you can submit another frame
*before* the Writeback-1 fence fires. On that HW, you should be able
to submit a frame including a new writeback FB, at full refresh rate,
and it should "just work."

The writeback code was written to permit this, but it's not
"discoverable" via the API whether it's OK on a particular device; and
the reason for the defensive wording is that some people said it
*wasn't* OK on theirs - I seem to remember VC4 writeback makes very
few timing guarantees.

Maybe it should be discoverable. I don't know how hard it would be to
describe everyone's constraints.

> Current DRM uAPI for writeback supports single-frame jobs. Userspace
> submits an FB, it waits for the fence, then submits next FB, etc. This
> is not well-suitable (nor it is defined in a way suitable) for the
> streamed operations, where you want to generate a stream of N frames per
> second. Even though userspace sets a mode which includes a particular
> frame rate, the WB API has no guarantee that the FPS will be or can be
> met.
> 
> In my opinion a proper uAPI for the streamed Writeback should be closer
> to v4l2, where you submit several buffers in advance together with the
> corresponding fences and then they are filled one by one and the
> corresponding fence is signalled to userspace. 
> 

At the time of introducing writeback connectors, we also discussed
having a more "streaming" API, or even having an actual V4L2 device
representing the frame stream. At the time we were most interested in
single frames for static-screen and testing purposes, and there wasn't
a consensus on how to make a streaming API, so we didn't do it.

> Note, in my humble opinion, it should be perfectly possible to setup
> writeback as a clone to the existing connector (if a clone mode is
> suppored by the hardware) and then to supply the jobs (FB+fence)
> occasionally rather than for each frame. Such userspace operation should
> not cause any degradation on the main connector.

This is fine and supported by the API. If the HW supports it, you can
have a CRTC scanning out to a real display, and then every Nth frame
add a writeback framebuffer to capture that frame.

IIRC you can attach a Writeback connector without a framebuffer to a
CRTC, to support exactly this use case.

So on Frame 0:

* DP-1
  * CRTC_ID = crtc-0
* plane-0
  * CRTC_ID = crtc-0
  * FB_ID = fb-0

* Writeback-1
  * CRTC_ID = crtc-0
  * WRITEBACK_FB_ID = 0 (no framebuffer)
  * WRITEBACK_OUT_FENCE_PTR = NULL

Then on Frame 'N' where you want a writeback, and a non-modeset
commit, simply add a framebuffer:

* DP-1
  * CRTC_ID = crtc-0
* plane-0
  * CRTC_ID = crtc-0
  * FB_ID = fb-1

* Writeback-1
  * CRTC_ID = crtc-0
  * WRITEBACK_FB_ID = fb-9
  * WRITEBACK_OUT_FENCE_PTR = <whatever>

Frame 'N+1' with no writeback, don't add a framebuffer:

* DP-1
  * CRTC_ID = crtc-0
* plane-0
  * CRTC_ID = crtc-0
  * FB_ID = fb-2

* Writeback-1
  * CRTC_ID = crtc-0
  * WRITEBACK_FB_ID = 0
  * WRITEBACK_OUT_FENCE_PTR = NULL

> 
> > 
> > I want to be a good citizen and do stuff by the book here. :-P
> > 

Yeah... so as you pointed out initially, if you've got a writeback
connector attached to your scanout CRTC, "by the book" means basically
reducing update rate. Obviously if you use an entirely separate CRTC
for writeback, they operate independently.

Thanks,
-Brian

> > >>
> > >> >
> > >> >If we already have devices where you can use writeback together with real
> > >> >outputs, then I guess that counts as an oopsie :-/
> > >>
> > >> Well "works fine" fits into the "undefined behaviour" bucket, just as
> > >> well as "corrupts your fb" does :-)
> > >>
> > >> -Brian
> > 
> 
> -- 
> With best wishes
> Dmitry

Re: Correct sequencing of usage of DRM writeback connector

[Daniel Vetter]

On Mon, Jun 17, 2024 at 10:52:27PM +0300, Dmitry Baryshkov wrote:
> On Mon, Jun 17, 2024 at 11:28:35AM GMT, Abhinav Kumar wrote:
> > Hi
> > 
> > On 6/17/2024 9:54 AM, Brian Starkey wrote:
> > > Hi,
> > > 
> > > On Mon, Jun 17, 2024 at 05:16:36PM +0200, Daniel Vetter wrote:
> > > > On Mon, Jun 17, 2024 at 01:41:59PM +0000, Hoosier, Matt wrote:
> > > > > Hi,
> > > > > 
> > > > > There is a discussion ongoing over in the compositor world about the implication of this cautionary wording found in the documentation for the DRM_MODE_CONNECTOR_WRITEBACK connectors:
> > > > > 
> > > > > >   *  "WRITEBACK_OUT_FENCE_PTR":
> > > > > >   *	Userspace can use this property to provide a pointer for the kernel to
> > > > > >   *	fill with a sync_file file descriptor, which will signal once the
> > > > > >   *	writeback is finished. The value should be the address of a 32-bit
> > > > > >   *	signed integer, cast to a u64.
> > > > > >   *	Userspace should wait for this fence to signal before making another
> > > > > >   *	commit affecting any of the same CRTCs, Planes or Connectors.
> > > > > >   *	**Failure to do so will result in undefined behaviour.**
> > > > > >   *	For this reason it is strongly recommended that all userspace
> > > > > >   *	applications making use of writeback connectors *always* retrieve an
> > > > > >   *	out-fence for the commit and use it appropriately.
> > > > > >   *	From userspace, this property will always read as zero.
> > > > > 
> > > > > The question is whether it's realistic to hope that a DRM writeback
> > > > > connector can produce results on every frame, and do so without dragging
> > > > > down the frame-rate for the connector.
> > > > > 
> > > > > The wording in the documentation above suggests that it is very likely
> > > > > the fence fd won't signal userspace until after the vblank following the
> > > > > scanout during which the writeback was applied (call that frame N). This
> > > > > would mean that the compositor driving the connector would typically be
> > > > > unable to legally queue a page flip for frame N+1.
> > > > > 
> > > > > Is this the right interpretation? Is the writeback hardware typically
> > > > > even designed with a streaming use-case in mind? Maybe it's just
> > > > > intended for occasional static screenshots.
> > > > 
> > > > So typically writeback hardware needs its separate crtc (at least the
> > > > examples I know of) and doesn't make a lot of guarantees that it's fast
> > > > enough for real time use. Since it's a separate crtc it shouldn't hold up
> > > > the main composition loop, and so this should be all fine.
> > > 
> > > On Mali-DP and Komeda at least, you can use writeback on the same CRTC
> > > that is driving a "real" display, and it should generally work. If the
> > > writeback doesn't keep up then the HW will signal an error, but it was
> > > designed to work in-sync with real scanout, on the same pipe.
> > > 
> > 
> > Same with MSM hardware. You can use writeback with same CRTC that is driving
> > a "real" display and yes we call it concurrent writeback. So I think it is
> > correct in the documentation to expect to wait till this is signaled if the
> > same CRTC is being used.

TIL

> > > > If/when we have hardware and driver support where you can use the
> > > > writeback connector as a real-time streamout kind of thing, then we need
> > > > to change all this, because with the current implementation, there's
> > > > indeed the possibility that funny things can happen if you ignore the
> > > > notice (funny as in data corruption, not funny as the kernel crashes of
> > > > course).
> > > 
> > > Indeed, the wording was added (from what I remember from so long
> > > ago...) because it sounded like different HW made very different
> > > guarantees/non-guarantees about what data would be written when, so
> > > perhaps you'd end up with some pixels from the next frame in your
> > > buffer or something.
> > > 
> > > Taking Mali-DP/Komeda again, the writeback configuration is latched
> > > along with everything else, and writeback throughput permitting, it
> > > should "just work" if you submit a new writeback every frame. It
> > > drains out the last of the data during vblank, before starting on the
> > > next frame. That doesn't help the "general case" though.
> > > 
> > 
> > Would it be fair to summarize it like below:
> > 
> > 1) If the same CRTC is shared with the real time display, then the hardware
> > is expected to fire this every frame so userspace should wait till this is
> > signaled.
> 
> As I wrote in response to another email in this thread, IMO existing
> uAPI doesn't fully allow this. There is no way to enforce 'vblank'
> handling onto the userspace. So userspace should be able to supply at
> least two buffers and then after the vblank it should be able to enqueue
> the next buffer, while the filled buffer is automatically dequeued by
> the driver and is not used for further image output.

Yeah if you want streaming writeback we need a queue depth of at least 2
in the kms api. Will help a lot on all hardware, but on some it's required
because the time when the writeback buffer is fully flushed is after the
point of no return for the next frame (which is when the vblank event is
supposed to go out).

I think over the years we've slowly inched forward to make at least the
drm code safe for a queue depth of 2 in the atomic machinery, but the
writeback and driver code probably needs a bunch of work.
-Sima

> 
> > 
> > 2) If a different CRTC is used for the writeback, then the composition loop
> > for the real time display should not block on this unless its a mirroring
> > use-case, then we will be throttled by the lowest refresh rate anyway.
> 
> what is mirroring in this case? You have specified that a different CRTC
> is being used.
> 
> > 
> > > > 
> > > > If we already have devices where you can use writeback together with real
> > > > outputs, then I guess that counts as an oopsie :-/
> > > 
> > > Well "works fine" fits into the "undefined behaviour" bucket, just as
> > > well as "corrupts your fb" does :-)
> 
> 
> -- 
> With best wishes
> Dmitry

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: Correct sequencing of usage of DRM writeback connector

[Dmitry Baryshkov]

On Tue, 18 Jun 2024 at 12:33, Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Mon, Jun 17, 2024 at 10:52:27PM +0300, Dmitry Baryshkov wrote:
> > On Mon, Jun 17, 2024 at 11:28:35AM GMT, Abhinav Kumar wrote:
> > > Hi
> > >
> > > On 6/17/2024 9:54 AM, Brian Starkey wrote:
> > > > Hi,
> > > >
> > > > On Mon, Jun 17, 2024 at 05:16:36PM +0200, Daniel Vetter wrote:
> > > > > On Mon, Jun 17, 2024 at 01:41:59PM +0000, Hoosier, Matt wrote:

> > > > > If/when we have hardware and driver support where you can use the
> > > > > writeback connector as a real-time streamout kind of thing, then we need
> > > > > to change all this, because with the current implementation, there's
> > > > > indeed the possibility that funny things can happen if you ignore the
> > > > > notice (funny as in data corruption, not funny as the kernel crashes of
> > > > > course).
> > > >
> > > > Indeed, the wording was added (from what I remember from so long
> > > > ago...) because it sounded like different HW made very different
> > > > guarantees/non-guarantees about what data would be written when, so
> > > > perhaps you'd end up with some pixels from the next frame in your
> > > > buffer or something.
> > > >
> > > > Taking Mali-DP/Komeda again, the writeback configuration is latched
> > > > along with everything else, and writeback throughput permitting, it
> > > > should "just work" if you submit a new writeback every frame. It
> > > > drains out the last of the data during vblank, before starting on the
> > > > next frame. That doesn't help the "general case" though.
> > > >
> > >
> > > Would it be fair to summarize it like below:
> > >
> > > 1) If the same CRTC is shared with the real time display, then the hardware
> > > is expected to fire this every frame so userspace should wait till this is
> > > signaled.
> >
> > As I wrote in response to another email in this thread, IMO existing
> > uAPI doesn't fully allow this. There is no way to enforce 'vblank'
> > handling onto the userspace. So userspace should be able to supply at
> > least two buffers and then after the vblank it should be able to enqueue
> > the next buffer, while the filled buffer is automatically dequeued by
> > the driver and is not used for further image output.
>
> Yeah if you want streaming writeback we need a queue depth of at least 2
> in the kms api. Will help a lot on all hardware, but on some it's required
> because the time when the writeback buffer is fully flushed is after the
> point of no return for the next frame (which is when the vblank event is
> supposed to go out).
>
> I think over the years we've slowly inched forward to make at least the
> drm code safe for a queue depth of 2 in the atomic machinery, but the
> writeback and driver code probably needs a bunch of work.

Do you mean handling the queue by allowing userspace to commit 'next' FB_ID?

I was leaning towards extending the uAPI with something like explicit
WRITEBACK_FB_ID_QUEUED and WRITEBACK_OUT_FENCE_PTR_QUEUED properties.
This way once the fence has been reached, the drm_writeback might
automatically put the old framebuffer, move _QUEUED to normal props
and then signal the userspace. This way the single-frame writeback
drivers can support the old API, while allowing cloned-writeback
drivers to implement the streaming approach. Also, this allows drivers
to do clever tricks, like forbidding the _QUEUED operation if the
refresh rate for the writeback connector is too high.


> -Sima
>
> >
> > >
> > > 2) If a different CRTC is used for the writeback, then the composition loop
> > > for the real time display should not block on this unless its a mirroring
> > > use-case, then we will be throttled by the lowest refresh rate anyway.
> >
> > what is mirroring in this case? You have specified that a different CRTC
> > is being used.
> >
> > >
> > > > >
> > > > > If we already have devices where you can use writeback together with real
> > > > > outputs, then I guess that counts as an oopsie :-/
> > > >
> > > > Well "works fine" fits into the "undefined behaviour" bucket, just as
> > > > well as "corrupts your fb" does :-)


-- 
With best wishes
Dmitry

RE: Correct sequencing of usage of DRM writeback connector

[Hoosier, Matt]

> From: Brian Starkey <brian.starkey@arm.com> 
> Sent: Tuesday, June 18, 2024 4:11 AM
> To: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Cc: Hoosier, Matt <Matt.Hoosier@garmin.com>; Daniel Vetter <daniel@ffwll.ch>; dri-devel@lists.freedesktop.org; Pekka Paalanen <pekka.paalanen@collabora.com>; nd@arm.com
> Subject: Re: Correct sequencing of usage of DRM writeback connector
> 
> Hi,
> 
> On Mon, Jun 17, 2024 at 10:48:47PM UTC, Dmitry Baryshkov wrote:
> > On Mon, Jun 17, 2024 at 05:36:34PM GMT, Hoosier, Matt wrote:
> > > >> >> >> There is a discussion ongoing over in the compositor world about the implication 
> > > >> Hi,
> > > >>
> > > >> On Mon, Jun 17, 2024 at 05:16:36PM +0200, Daniel Vetter wrote:
> > > >> >On Mon, Jun 17, 2024 at 01:41:59PM +0000, Hoosier, Matt wrote:
> > > >> >> Hi,
> > > >> >>
> > > >> >> There is a discussion ongoing over in the compositor world about the implication of this cautionary wording found in the documentation for the DRM_MODE_CONNECTOR_WRITEBACK connectors:
> > > >> >>
> > > >> >> >  *  "WRITEBACK_OUT_FENCE_PTR":
> > > >> >> >  *	Userspace can use this property to provide a pointer for the kernel to
> > > >> >> >  *	fill with a sync_file file descriptor, which will signal once the
> > > >> >> >  *	writeback is finished. The value should be the address of a 32-bit
> > > >> >> >  *	signed integer, cast to a u64.
> > > >> >> >  *	Userspace should wait for this fence to signal before making another
> > > >> >> >  *	commit affecting any of the same CRTCs, Planes or Connectors.
> > > >> >> >  *	**Failure to do so will result in undefined behaviour.**
> > > >> >> >  *	For this reason it is strongly recommended that all userspace
> > > >> >> >  *	applications making use of writeback connectors *always* retrieve an
> > > >> >> >  *	out-fence for the commit and use it appropriately.
> > > >> >> >  *	From userspace, this property will always read as zero.
> > > >> >>
> > > >> >> The question is whether it's realistic to hope that a DRM writeback
> > > >> >> connector can produce results on every frame, and do so without dragging
> > > >> >> down the frame-rate for the connector.
> > > >> >>
> > > >> >> The wording in the documentation above suggests that it is very likely
> > > >> >> the fence fd won't signal userspace until after the vblank following the
> > > >> >> scanout during which the writeback was applied (call that frame N). This
> > > >> >> would mean that the compositor driving the connector would typically be
> > > >> >> unable to legally queue a page flip for frame N+1.
> > > >> >>
> > > >> >> Is this the right interpretation? Is the writeback hardware typically
> > > >> >> even designed with a streaming use-case in mind? Maybe it's just
> > > >> >> intended for occasional static screenshots.
> > > >> >
> > > >> >So typically writeback hardware needs its separate crtc (at least the
> > > >> >examples I know of) and doesn't make a lot of guarantees that it's fast
> > > >> >enough for real time use. Since it's a separate crtc it shouldn't hold up
> > > >> >the main composition loop, and so this should be all fine.
> > > 
> > > Hmm, I don't think this matches the implementation. What I see -- sometimes people call this "concurrent writeback" -- is that the writeback connector is wired directly to the CRTC that's feeding the regular connector whose picture is getting captured.
> > > 
> > > Something like (for frame N):
> > > 
> > > * DP-1
> > >   * CRTC_ID = crtc-0
> > > * plane-0
> > >   * CRTC_ID = crtc-0
> > >   * FB_ID = fb-0
> > > 
> > > * Writeback-1
> > >   * CRTC_ID = crtc-0
> > >   * WRITEBACK_FB_ID = fb-1
> > >   * WRITEBACK_OUT_FENCE_PTR = <whatever>
> > > 
> > > 
> > > Are you saying that that for frame N+1, we should switch DP-1 to use a different CRTC while the writeback is still busy being retired into fb-1 through crtc-0?
> 
> The "letter" of the API is that you need to wait for the Writeback-1
> fence before making another commit to any of the resources - which
> effectively means halving your update rate for crtc-0.
> 
> The fence might fire before vsync, so you might have a very small
> window for getting a new commit in on the very next frame, but
> realistically it's unlikely.
> 
> Logically, if the writeback is working line-by-line alongside scanout
> then it won't have the last pixels until the end of the last line, so
> it can't signal completion until at least the start of vblank.
> 
> > > 
> > > >>
> > > >> On Mali-DP and Komeda at least, you can use writeback on the same CRTC
> > > >> that is driving a "real" display, and it should generally work. If the
> > > >> writeback doesn't keep up then the HW will signal an error, but it was
> > > >> designed to work in-sync with real scanout, on the same pipe.
> > > >>
> > > >> >
> > > >> >If/when we have hardware and driver support where you can use the
> > > >> >writeback connector as a real-time streamout kind of thing, then we need
> > > >> >to change all this, because with the current implementation, there's
> > > >> >indeed the possibility that funny things can happen if you ignore the
> > > >> >notice (funny as in data corruption, not funny as the kernel crashes of
> > > >> >course).
> > > >>
> > > >> Indeed, the wording was added (from what I remember from so long
> > > >> ago...) because it sounded like different HW made very different
> > > >> guarantees/non-guarantees about what data would be written when, so
> > > >> perhaps you'd end up with some pixels from the next frame in your
> > > >> buffer or something.
> > > >>
> > > >> Taking Mali-DP/Komeda again, the writeback configuration is latched
> > > >> along with everything else, and writeback throughput permitting, it
> > > >> should "just work" if you submit a new writeback every frame. It
> > > >> drains out the last of the data during vblank, before starting on the
> > > >> next frame. That doesn't help the "general case" though.
> > > 
> > > Are you saying that on hardware whose writeback implementation is
> > > amenable, the drivers will generally fire the fence FD in time for
> > > userspace to post a next frame to the real connector's CRTC for the
> > > immediately following frame? Or are you just saying that some hardware
> > > could support it, but that the DRM framework's insistence that
> > > userspace waits until the fence fires might still make it artificially
> > > too slow to drive the hardware to its full capacity?
> > 
> 
> I'm saying that on Mali-DP and Komeda, you can submit another frame
> *before* the Writeback-1 fence fires. On that HW, you should be able
> to submit a frame including a new writeback FB, at full refresh rate,
> and it should "just work."
> 
> The writeback code was written to permit this, but it's not
> "discoverable" via the API whether it's OK on a particular device; and
> the reason for the defensive wording is that some people said it
> *wasn't* OK on theirs - I seem to remember VC4 writeback makes very
> few timing guarantees.
> 
> Maybe it should be discoverable. I don't know how hard it would be to
> describe everyone's constraints.
> 
> > Current DRM uAPI for writeback supports single-frame jobs. Userspace
> > submits an FB, it waits for the fence, then submits next FB, etc. This
> > is not well-suitable (nor it is defined in a way suitable) for the
> > streamed operations, where you want to generate a stream of N frames per
> > second. Even though userspace sets a mode which includes a particular
> > frame rate, the WB API has no guarantee that the FPS will be or can be
> > met.
> > 
> > In my opinion a proper uAPI for the streamed Writeback should be closer
> > to v4l2, where you submit several buffers in advance together with the
> > corresponding fences and then they are filled one by one and the
> > corresponding fence is signalled to userspace. 
> > 
> 
> At the time of introducing writeback connectors, we also discussed
> having a more "streaming" API, or even having an actual V4L2 device
> representing the frame stream. At the time we were most interested in
> single frames for static-screen and testing purposes, and there wasn't
> a consensus on how to make a streaming API, so we didn't do it.
> 
> > Note, in my humble opinion, it should be perfectly possible to setup
> > writeback as a clone to the existing connector (if a clone mode is
> > suppored by the hardware) and then to supply the jobs (FB+fence)
> > occasionally rather than for each frame. Such userspace operation should
> > not cause any degradation on the main connector.
> 
> This is fine and supported by the API. If the HW supports it, you can
> have a CRTC scanning out to a real display, and then every Nth frame
> add a writeback framebuffer to capture that frame.
> 
> IIRC you can attach a Writeback connector without a framebuffer to a
> CRTC, to support exactly this use case.
> 
> So on Frame 0:
> 
> * DP-1
>   * CRTC_ID = crtc-0
> * plane-0
>   * CRTC_ID = crtc-0
>   * FB_ID = fb-0
> 
> * Writeback-1
>   * CRTC_ID = crtc-0
>   * WRITEBACK_FB_ID = 0 (no framebuffer)
>   * WRITEBACK_OUT_FENCE_PTR = NULL
> 
> Then on Frame 'N' where you want a writeback, and a non-modeset
> commit, simply add a framebuffer:
> 
> * DP-1
>   * CRTC_ID = crtc-0
> * plane-0
>   * CRTC_ID = crtc-0
>   * FB_ID = fb-1
> 
> * Writeback-1
>   * CRTC_ID = crtc-0
>   * WRITEBACK_FB_ID = fb-9
>   * WRITEBACK_OUT_FENCE_PTR = <whatever>
> 
> Frame 'N+1' with no writeback, don't add a framebuffer:
> 
> * DP-1
>   * CRTC_ID = crtc-0
> * plane-0
>   * CRTC_ID = crtc-0
>   * FB_ID = fb-2
> 
> * Writeback-1
>   * CRTC_ID = crtc-0
>   * WRITEBACK_FB_ID = 0
>   * WRITEBACK_OUT_FENCE_PTR = NULL
> 
> > 
> > > 
> > > I want to be a good citizen and do stuff by the book here. :-P
> > > 
> 
> Yeah... so as you pointed out initially, if you've got a writeback
> connector attached to your scanout CRTC, "by the book" means basically
> reducing update rate. Obviously if you use an entirely separate CRTC
> for writeback, they operate independently.

Okay, got it.

Thanks to everybody for the help interpreting the right way to use this API for now.

> 
> Thanks,
> -Brian
> 
> > > >>
> > > >> >
> > > >> >If we already have devices where you can use writeback together with real
> > > >> >outputs, then I guess that counts as an oopsie :-/
> > > >>
> > > >> Well "works fine" fits into the "undefined behaviour" bucket, just as
> > > >> well as "corrupts your fb" does :-)
> > > >>
> > > >> -Brian
> > > 
> > 
> > -- 
> > With best wishes
> > Dmitry
>

Re: Correct sequencing of usage of DRM writeback connector

[Abhinav Kumar]

On 6/18/2024 2:33 AM, Daniel Vetter wrote:
> On Mon, Jun 17, 2024 at 10:52:27PM +0300, Dmitry Baryshkov wrote:
>> On Mon, Jun 17, 2024 at 11:28:35AM GMT, Abhinav Kumar wrote:
>>> Hi
>>>
>>> On 6/17/2024 9:54 AM, Brian Starkey wrote:
>>>> Hi,
>>>>
>>>> On Mon, Jun 17, 2024 at 05:16:36PM +0200, Daniel Vetter wrote:
>>>>> On Mon, Jun 17, 2024 at 01:41:59PM +0000, Hoosier, Matt wrote:
>>>>>> Hi,
>>>>>>
>>>>>> There is a discussion ongoing over in the compositor world about the implication of this cautionary wording found in the documentation for the DRM_MODE_CONNECTOR_WRITEBACK connectors:
>>>>>>
>>>>>>>    *  "WRITEBACK_OUT_FENCE_PTR":
>>>>>>>    *	Userspace can use this property to provide a pointer for the kernel to
>>>>>>>    *	fill with a sync_file file descriptor, which will signal once the
>>>>>>>    *	writeback is finished. The value should be the address of a 32-bit
>>>>>>>    *	signed integer, cast to a u64.
>>>>>>>    *	Userspace should wait for this fence to signal before making another
>>>>>>>    *	commit affecting any of the same CRTCs, Planes or Connectors.
>>>>>>>    *	**Failure to do so will result in undefined behaviour.**
>>>>>>>    *	For this reason it is strongly recommended that all userspace
>>>>>>>    *	applications making use of writeback connectors *always* retrieve an
>>>>>>>    *	out-fence for the commit and use it appropriately.
>>>>>>>    *	From userspace, this property will always read as zero.
>>>>>>
>>>>>> The question is whether it's realistic to hope that a DRM writeback
>>>>>> connector can produce results on every frame, and do so without dragging
>>>>>> down the frame-rate for the connector.
>>>>>>
>>>>>> The wording in the documentation above suggests that it is very likely
>>>>>> the fence fd won't signal userspace until after the vblank following the
>>>>>> scanout during which the writeback was applied (call that frame N). This
>>>>>> would mean that the compositor driving the connector would typically be
>>>>>> unable to legally queue a page flip for frame N+1.
>>>>>>
>>>>>> Is this the right interpretation? Is the writeback hardware typically
>>>>>> even designed with a streaming use-case in mind? Maybe it's just
>>>>>> intended for occasional static screenshots.
>>>>>
>>>>> So typically writeback hardware needs its separate crtc (at least the
>>>>> examples I know of) and doesn't make a lot of guarantees that it's fast
>>>>> enough for real time use. Since it's a separate crtc it shouldn't hold up
>>>>> the main composition loop, and so this should be all fine.
>>>>
>>>> On Mali-DP and Komeda at least, you can use writeback on the same CRTC
>>>> that is driving a "real" display, and it should generally work. If the
>>>> writeback doesn't keep up then the HW will signal an error, but it was
>>>> designed to work in-sync with real scanout, on the same pipe.
>>>>
>>>
>>> Same with MSM hardware. You can use writeback with same CRTC that is driving
>>> a "real" display and yes we call it concurrent writeback. So I think it is
>>> correct in the documentation to expect to wait till this is signaled if the
>>> same CRTC is being used.
> 
> TIL
> 
>>>>> If/when we have hardware and driver support where you can use the
>>>>> writeback connector as a real-time streamout kind of thing, then we need
>>>>> to change all this, because with the current implementation, there's
>>>>> indeed the possibility that funny things can happen if you ignore the
>>>>> notice (funny as in data corruption, not funny as the kernel crashes of
>>>>> course).
>>>>
>>>> Indeed, the wording was added (from what I remember from so long
>>>> ago...) because it sounded like different HW made very different
>>>> guarantees/non-guarantees about what data would be written when, so
>>>> perhaps you'd end up with some pixels from the next frame in your
>>>> buffer or something.
>>>>
>>>> Taking Mali-DP/Komeda again, the writeback configuration is latched
>>>> along with everything else, and writeback throughput permitting, it
>>>> should "just work" if you submit a new writeback every frame. It
>>>> drains out the last of the data during vblank, before starting on the
>>>> next frame. That doesn't help the "general case" though.
>>>>
>>>
>>> Would it be fair to summarize it like below:
>>>
>>> 1) If the same CRTC is shared with the real time display, then the hardware
>>> is expected to fire this every frame so userspace should wait till this is
>>> signaled.
>>
>> As I wrote in response to another email in this thread, IMO existing
>> uAPI doesn't fully allow this. There is no way to enforce 'vblank'
>> handling onto the userspace. So userspace should be able to supply at
>> least two buffers and then after the vblank it should be able to enqueue
>> the next buffer, while the filled buffer is automatically dequeued by
>> the driver and is not used for further image output.
> 

Sorry for the late response. What I meant was, if we are using 
concurrent writeback with the real time display, it should be capable of 
running at the same speed as the real time display. I do not have the 
numbers to share but atleast that's the expectation.

But, yes I do admit that current UAPI does not fully allow having a 
queue depth for WB FBs. And having it will help us.

> Yeah if you want streaming writeback we need a queue depth of at least 2
> in the kms api. Will help a lot on all hardware, but on some it's required
> because the time when the writeback buffer is fully flushed is after the
> point of no return for the next frame (which is when the vblank event is
> supposed to go out).
> 
> I think over the years we've slowly inched forward to make at least the
> drm code safe for a queue depth of 2 in the atomic machinery, but the
> writeback and driver code probably needs a bunch of work.
> -Sima
> 
>>
>>>
>>> 2) If a different CRTC is used for the writeback, then the composition loop
>>> for the real time display should not block on this unless its a mirroring
>>> use-case, then we will be throttled by the lowest refresh rate anyway.
>>
>> what is mirroring in this case? You have specified that a different CRTC
>> is being used.
>>

Definition of mirroring could be thought of in two ways:

1) in clone mode, the WB is running at the same rate as the real time 
display and hence if we are mirroring the content this way there is same 
CRTC.

2) lets say I want to mirror my content using wifi display but the 
end-monitor is running on a different resolution and fps, then I cannot 
use clone mode in this case right because the CRTC which the writeback 
is using will be programmed for a different mode than the real time display.

For the second case, it is still mirroring the content but with a 
different CRTC so will be slowed down by the slowest display otherwise 
the displays will go out of sync. This is what I meant in this use-case.

>>>
>>>>>
>>>>> If we already have devices where you can use writeback together with real
>>>>> outputs, then I guess that counts as an oopsie :-/
>>>>
>>>> Well "works fine" fits into the "undefined behaviour" bucket, just as
>>>> well as "corrupts your fb" does :-)
>>
>>
>> -- 
>> With best wishes
>> Dmitry
> 

Re: Correct sequencing of usage of DRM writeback connector

[Daniel Vetter]

On Tue, Jun 18, 2024 at 07:10:58PM -0700, Abhinav Kumar wrote:
> 
> 
> On 6/18/2024 2:33 AM, Daniel Vetter wrote:
> > On Mon, Jun 17, 2024 at 10:52:27PM +0300, Dmitry Baryshkov wrote:
> > > On Mon, Jun 17, 2024 at 11:28:35AM GMT, Abhinav Kumar wrote:
> > > > Hi
> > > > 
> > > > On 6/17/2024 9:54 AM, Brian Starkey wrote:
> > > > > Hi,
> > > > > 
> > > > > On Mon, Jun 17, 2024 at 05:16:36PM +0200, Daniel Vetter wrote:
> > > > > > On Mon, Jun 17, 2024 at 01:41:59PM +0000, Hoosier, Matt wrote:
> > > > > > > Hi,
> > > > > > > 
> > > > > > > There is a discussion ongoing over in the compositor world about the implication of this cautionary wording found in the documentation for the DRM_MODE_CONNECTOR_WRITEBACK connectors:
> > > > > > > 
> > > > > > > >    *  "WRITEBACK_OUT_FENCE_PTR":
> > > > > > > >    *	Userspace can use this property to provide a pointer for the kernel to
> > > > > > > >    *	fill with a sync_file file descriptor, which will signal once the
> > > > > > > >    *	writeback is finished. The value should be the address of a 32-bit
> > > > > > > >    *	signed integer, cast to a u64.
> > > > > > > >    *	Userspace should wait for this fence to signal before making another
> > > > > > > >    *	commit affecting any of the same CRTCs, Planes or Connectors.
> > > > > > > >    *	**Failure to do so will result in undefined behaviour.**
> > > > > > > >    *	For this reason it is strongly recommended that all userspace
> > > > > > > >    *	applications making use of writeback connectors *always* retrieve an
> > > > > > > >    *	out-fence for the commit and use it appropriately.
> > > > > > > >    *	From userspace, this property will always read as zero.
> > > > > > > 
> > > > > > > The question is whether it's realistic to hope that a DRM writeback
> > > > > > > connector can produce results on every frame, and do so without dragging
> > > > > > > down the frame-rate for the connector.
> > > > > > > 
> > > > > > > The wording in the documentation above suggests that it is very likely
> > > > > > > the fence fd won't signal userspace until after the vblank following the
> > > > > > > scanout during which the writeback was applied (call that frame N). This
> > > > > > > would mean that the compositor driving the connector would typically be
> > > > > > > unable to legally queue a page flip for frame N+1.
> > > > > > > 
> > > > > > > Is this the right interpretation? Is the writeback hardware typically
> > > > > > > even designed with a streaming use-case in mind? Maybe it's just
> > > > > > > intended for occasional static screenshots.
> > > > > > 
> > > > > > So typically writeback hardware needs its separate crtc (at least the
> > > > > > examples I know of) and doesn't make a lot of guarantees that it's fast
> > > > > > enough for real time use. Since it's a separate crtc it shouldn't hold up
> > > > > > the main composition loop, and so this should be all fine.
> > > > > 
> > > > > On Mali-DP and Komeda at least, you can use writeback on the same CRTC
> > > > > that is driving a "real" display, and it should generally work. If the
> > > > > writeback doesn't keep up then the HW will signal an error, but it was
> > > > > designed to work in-sync with real scanout, on the same pipe.
> > > > > 
> > > > 
> > > > Same with MSM hardware. You can use writeback with same CRTC that is driving
> > > > a "real" display and yes we call it concurrent writeback. So I think it is
> > > > correct in the documentation to expect to wait till this is signaled if the
> > > > same CRTC is being used.
> > 
> > TIL
> > 
> > > > > > If/when we have hardware and driver support where you can use the
> > > > > > writeback connector as a real-time streamout kind of thing, then we need
> > > > > > to change all this, because with the current implementation, there's
> > > > > > indeed the possibility that funny things can happen if you ignore the
> > > > > > notice (funny as in data corruption, not funny as the kernel crashes of
> > > > > > course).
> > > > > 
> > > > > Indeed, the wording was added (from what I remember from so long
> > > > > ago...) because it sounded like different HW made very different
> > > > > guarantees/non-guarantees about what data would be written when, so
> > > > > perhaps you'd end up with some pixels from the next frame in your
> > > > > buffer or something.
> > > > > 
> > > > > Taking Mali-DP/Komeda again, the writeback configuration is latched
> > > > > along with everything else, and writeback throughput permitting, it
> > > > > should "just work" if you submit a new writeback every frame. It
> > > > > drains out the last of the data during vblank, before starting on the
> > > > > next frame. That doesn't help the "general case" though.
> > > > > 
> > > > 
> > > > Would it be fair to summarize it like below:
> > > > 
> > > > 1) If the same CRTC is shared with the real time display, then the hardware
> > > > is expected to fire this every frame so userspace should wait till this is
> > > > signaled.
> > > 
> > > As I wrote in response to another email in this thread, IMO existing
> > > uAPI doesn't fully allow this. There is no way to enforce 'vblank'
> > > handling onto the userspace. So userspace should be able to supply at
> > > least two buffers and then after the vblank it should be able to enqueue
> > > the next buffer, while the filled buffer is automatically dequeued by
> > > the driver and is not used for further image output.
> > 
> 
> Sorry for the late response. What I meant was, if we are using concurrent
> writeback with the real time display, it should be capable of running at the
> same speed as the real time display. I do not have the numbers to share but
> atleast that's the expectation.
> 
> But, yes I do admit that current UAPI does not fully allow having a queue
> depth for WB FBs. And having it will help us.
> 
> > Yeah if you want streaming writeback we need a queue depth of at least 2
> > in the kms api. Will help a lot on all hardware, but on some it's required
> > because the time when the writeback buffer is fully flushed is after the
> > point of no return for the next frame (which is when the vblank event is
> > supposed to go out).
> > 
> > I think over the years we've slowly inched forward to make at least the
> > drm code safe for a queue depth of 2 in the atomic machinery, but the
> > writeback and driver code probably needs a bunch of work.
> > -Sima
> > 
> > > 
> > > > 
> > > > 2) If a different CRTC is used for the writeback, then the composition loop
> > > > for the real time display should not block on this unless its a mirroring
> > > > use-case, then we will be throttled by the lowest refresh rate anyway.
> > > 
> > > what is mirroring in this case? You have specified that a different CRTC
> > > is being used.
> > > 
> 
> Definition of mirroring could be thought of in two ways:
> 
> 1) in clone mode, the WB is running at the same rate as the real time
> display and hence if we are mirroring the content this way there is same
> CRTC.
> 
> 2) lets say I want to mirror my content using wifi display but the
> end-monitor is running on a different resolution and fps, then I cannot use
> clone mode in this case right because the CRTC which the writeback is using
> will be programmed for a different mode than the real time display.
> 
> For the second case, it is still mirroring the content but with a different
> CRTC so will be slowed down by the slowest display otherwise the displays
> will go out of sync. This is what I meant in this use-case.

Separate CRTC I think should work, because you can run the 2 queues in
parallel. The issue is only with the single crtc use-case because:

- the writeback finishes only a bit (depends upon how the hw flushes out
  writebacks) after the next vblank period has started.

- at least on some hardware you need to submit the next kms state _before_
  the vblank period has started. And even on hw/drivers where this is not
  the case, only having the vblank window to submit the next kms atomic
  state is really a bit too small.

As soon as you have 2 crtc you can untangle these and drive them with 2
loops, and still hit every frame with the writeback (since the separate
writeback crtc can run a tiny bit behind the display one).

Of course if your userspace only has one redraw loop (android suffered
from that for years, not sure it's fixed), then yes you'll slow down.
-Sima

> 
> > > > 
> > > > > > 
> > > > > > If we already have devices where you can use writeback together with real
> > > > > > outputs, then I guess that counts as an oopsie :-/
> > > > > 
> > > > > Well "works fine" fits into the "undefined behaviour" bucket, just as
> > > > > well as "corrupts your fb" does :-)
> > > 
> > > 
> > > -- 
> > > With best wishes
> > > Dmitry
> > 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: Correct sequencing of usage of DRM writeback connector

[Daniel Vetter]

On Tue, Jun 18, 2024 at 12:48:13PM +0300, Dmitry Baryshkov wrote:
> On Tue, 18 Jun 2024 at 12:33, Daniel Vetter <daniel@ffwll.ch> wrote:
> >
> > On Mon, Jun 17, 2024 at 10:52:27PM +0300, Dmitry Baryshkov wrote:
> > > On Mon, Jun 17, 2024 at 11:28:35AM GMT, Abhinav Kumar wrote:
> > > > Hi
> > > >
> > > > On 6/17/2024 9:54 AM, Brian Starkey wrote:
> > > > > Hi,
> > > > >
> > > > > On Mon, Jun 17, 2024 at 05:16:36PM +0200, Daniel Vetter wrote:
> > > > > > On Mon, Jun 17, 2024 at 01:41:59PM +0000, Hoosier, Matt wrote:
> 
> > > > > > If/when we have hardware and driver support where you can use the
> > > > > > writeback connector as a real-time streamout kind of thing, then we need
> > > > > > to change all this, because with the current implementation, there's
> > > > > > indeed the possibility that funny things can happen if you ignore the
> > > > > > notice (funny as in data corruption, not funny as the kernel crashes of
> > > > > > course).
> > > > >
> > > > > Indeed, the wording was added (from what I remember from so long
> > > > > ago...) because it sounded like different HW made very different
> > > > > guarantees/non-guarantees about what data would be written when, so
> > > > > perhaps you'd end up with some pixels from the next frame in your
> > > > > buffer or something.
> > > > >
> > > > > Taking Mali-DP/Komeda again, the writeback configuration is latched
> > > > > along with everything else, and writeback throughput permitting, it
> > > > > should "just work" if you submit a new writeback every frame. It
> > > > > drains out the last of the data during vblank, before starting on the
> > > > > next frame. That doesn't help the "general case" though.
> > > > >
> > > >
> > > > Would it be fair to summarize it like below:
> > > >
> > > > 1) If the same CRTC is shared with the real time display, then the hardware
> > > > is expected to fire this every frame so userspace should wait till this is
> > > > signaled.
> > >
> > > As I wrote in response to another email in this thread, IMO existing
> > > uAPI doesn't fully allow this. There is no way to enforce 'vblank'
> > > handling onto the userspace. So userspace should be able to supply at
> > > least two buffers and then after the vblank it should be able to enqueue
> > > the next buffer, while the filled buffer is automatically dequeued by
> > > the driver and is not used for further image output.
> >
> > Yeah if you want streaming writeback we need a queue depth of at least 2
> > in the kms api. Will help a lot on all hardware, but on some it's required
> > because the time when the writeback buffer is fully flushed is after the
> > point of no return for the next frame (which is when the vblank event is
> > supposed to go out).
> >
> > I think over the years we've slowly inched forward to make at least the
> > drm code safe for a queue depth of 2 in the atomic machinery, but the
> > writeback and driver code probably needs a bunch of work.
> 
> Do you mean handling the queue by allowing userspace to commit 'next' FB_ID?
> 
> I was leaning towards extending the uAPI with something like explicit
> WRITEBACK_FB_ID_QUEUED and WRITEBACK_OUT_FENCE_PTR_QUEUED properties.
> This way once the fence has been reached, the drm_writeback might
> automatically put the old framebuffer, move _QUEUED to normal props
> and then signal the userspace. This way the single-frame writeback
> drivers can support the old API, while allowing cloned-writeback
> drivers to implement the streaming approach. Also, this allows drivers
> to do clever tricks, like forbidding the _QUEUED operation if the
> refresh rate for the writeback connector is too high.

Eh I think we should just allow atomic commits with a queue depth > 1. I
think that's both the cleanest uapi, and also the cleanest on the driver
side, since I just don't want to think about what happens when we have
multiple commits going on at the same time on the same crtc.

On the core/helper side we've tried to get there slowly by explicitly
accessing old/new state and never accessing plane/crtc->state directly.
That's really all that should be needed.

I guess to make things easier for drivers we could do an intermediate uapi
where we allow queue depth, but only for commits that don't require a
modeset, since those tend to be much simpler. Or maybe even allow the
driver to fully control where it can handle a queue depth > 1.

Essentially roughly this, minus all the safety checks we'll probably need
to add in various places. Note: extremely incomplete :-)


diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 63ee81e478b9..31c4e124eb5a 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -2136,7 +2136,7 @@ static int stall_checks(struct drm_crtc *crtc, bool nonblock)
 	spin_lock(&crtc->commit_lock);
 	i = 0;
 	list_for_each_entry(commit, &crtc->commit_list, commit_entry) {
-		if (i == 0) {
+		if (i == 1) {
 			completed = try_wait_for_completion(&commit->flip_done);
 			/*
 			 * Userspace is not allowed to get ahead of the previous
@@ -2150,7 +2150,7 @@ static int stall_checks(struct drm_crtc *crtc, bool nonblock)
 
 				return -EBUSY;
 			}
-		} else if (i == 1) {
+		} else if (i == 2) {
 			stall_commit = drm_crtc_commit_get(commit);
 			break;
 		}
@@ -3015,7 +3015,7 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state,
 	struct drm_private_obj *obj;
 	struct drm_private_state *old_obj_state, *new_obj_state;
 
-	if (stall) {
+	if (stall && 0) {
 		/*
 		 * We have to stall for hw_done here before
 		 * drm_atomic_helper_wait_for_dependencies() because flip

But I hope it sketches the idea at least.

Cheers, Sima

> 
> 
> > -Sima
> >
> > >
> > > >
> > > > 2) If a different CRTC is used for the writeback, then the composition loop
> > > > for the real time display should not block on this unless its a mirroring
> > > > use-case, then we will be throttled by the lowest refresh rate anyway.
> > >
> > > what is mirroring in this case? You have specified that a different CRTC
> > > is being used.
> > >
> > > >
> > > > > >
> > > > > > If we already have devices where you can use writeback together with real
> > > > > > outputs, then I guess that counts as an oopsie :-/
> > > > >
> > > > > Well "works fine" fits into the "undefined behaviour" bucket, just as
> > > > > well as "corrupts your fb" does :-)
> 
> 
> -- 
> With best wishes
> Dmitry

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch