Re: [Freedreno] [RFC] drm/atomic+msm: add helper to implement legacy dirtyfb

[Deepak Singh Rawat]

> >>> 1) Expose a dirty (or content_age property)
> >>> 2) Attach a clip-rect blob property.
> >>> 3) Fake a page-flip by ping-ponging two frame-buffers pointing to the
> >>> same
> >>> underlying buffer object.
> >>>
> >>> Are you saying that people are already using 3) and we should keep
> using
> >>> that?
> >>
> >> I'm saying they're using 3b), flip the same buffer wrapped in the same
> >> drm_framebuffer, and expect it to work.
> >>
> >> The only advantage dirtyfb has is that it allows you to supply the
> >> optimized upload rectangles, but at the cost of a funny api (it's working
> >> on the fb, not the plane/crtc you want to upload) and lack of drm_event
> to
> >> confirm when exactly you uploaded your stuff. But imo they should be
> the
> >> same underlying operation.
> >>
> >
> > FWIW, vmwgfx has always treated a dirtyfb as a pure front-buffer like
> > rendering operation without any synchronization,
> > We've guaranteed that only the rects that are present are uploaded, but
> only
> > xf86-video-vmware has taken advantage of this to keep
> > CPU- and GPU rendered content apart.
> > I think we've at some point run into problems with number of cliprects,
> (Old
> > KDE lock screen?) and use multiple dirtyfb for the same
> > update...
> 
> Ok, if we can hit this in practices then I think it's ok to have the
> limit. Just need to make sure userspace actually condenses the
> cliprects down to something within the limit, since with atomic flips
> you can't just do multiple updates - that would tear badly.

So I think the conclusion is to have damage clip rect limit with proper
documentation stating limitation of doing multiple updates.

> 
> Wrt not syncing: I think general use pretty clearly says lots of
> userspace renders into buffers with gpus (not even necessarily your
> own) and then expects dirtyfb or a flip to upload all the bits.
> Otherwise Rob Clark wouldn't need his patch. Given that I think we
> need to make general semantics follow that requirement - I don't
> expect it'll harm vmwgfx since it doesn't render into the frontbuffer
> anyway?
> 
> > IIRC the reason for working with the fb, is that it's much easier for
> > user-space, which doesn't have to care where planes are scanning out and
> > why.
> > "Present my new content on any screen that's affected".
> 
> Yeah, dirtyfb makes tons of sense for frontbuffer-rendering X, which
> also doesn't do per-scanout pixmaps. But for atomic flips you really
> want to flip on the crtc (or well plane), since otherwise with
> multiple planes it comes up all teared up. vmwgfx doesn't care I guess
> since you only have 1 primary plane, but all the SoC chips very much
> do.
> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - https://urldefense.proofpoint.com/v2/url?u=http-
> 3A__blog.ffwll.ch=DwIBaQ=uilaK90D4TOVoH58JNXRgQ=zOOG28inJK
> 0762SxAf-cyZdStnd2NQpRu98lJP2iYGw=XKgN7GPElFapBWdozPSC-
> 9rcfKmDvQC1QHhsFghexWc=SH9q5tw-
> UqpUBJVrr2v1mLpRo28Aau7iJ3YWlrgbPmU=
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [RFC] drm/atomic+msm: add helper to implement legacy dirtyfb

[Rob Clark]

On Thu, Apr 5, 2018 at 9:30 AM, Thomas Hellstrom  wrote:
> On 04/04/2018 11:56 AM, Daniel Vetter wrote:
>>
>> On Wed, Apr 04, 2018 at 11:10:08AM +0200, Thomas Hellstrom wrote:
>>>
>>> On 04/04/2018 10:43 AM, Daniel Vetter wrote:

 On Wed, Apr 04, 2018 at 10:22:21AM +0200, Thomas Hellstrom wrote:
>
> Hi,
>
> On 04/04/2018 08:58 AM, Daniel Vetter wrote:
>>
>> On Wed, Apr 4, 2018 at 12:42 AM, Rob Clark 
>> wrote:
>>>
>>> Add an atomic helper to implement dirtyfb support.  This is needed to
>>> support DSI command-mode panels with x11 userspace (ie. when we can't
>>> rely on pageflips to trigger a flush to the panel).
>>>
>>> To signal to the driver that the async atomic update needs to
>>> synchronize with fences, even though the fb didn't change, the
>>> drm_atomic_state::dirty flag is added.
>>>
>>> Signed-off-by: Rob Clark 
>>> ---
>>> Background: there are a number of different folks working on getting
>>> upstream kernel working on various different phones/tablets with qcom
>>> SoC's.. many of them have command mode panels, so we kind of need a
>>> way to support the legacy dirtyfb ioctl for x11 support.
>>>
>>> I know there is work on a proprer non-legacy atomic property for
>>> userspace to communicate dirty-rect(s) to the kernel, so this can
>>> be improved from triggering a full-frame flush once that is in
>>> place.  But we kinda needa a stop-gap solution.
>>>
>>> I had considered an in-driver solution for this, but things get a
>>> bit tricky if userspace ands up combining dirtyfb ioctls with page-
>>> flips, because we need to synchronize setting various CTL.FLUSH bits
>>> with setting the CTL.START bit.  (ie. really all we need to do for
>>> cmd mode panels is bang CTL.START, but is this ends up racing with
>>> pageflips setting FLUSH bits, then bad things.)  The easiest soln
>>> is to wrap this up as an atomic commit and rely on the worker to
>>> serialize things.  Hence adding an atomic dirtyfb helper.
>>>
>>> I guess at least the helper, with some small addition to translate
>>> and pass-thru the dirty rect(s) is useful to the final atomic dirty-
>>> rect property solution.  Depending on how far off that is, a stop-
>>> gap solution could be useful.
>>
>> Adding Noralf, who iirc already posted the full dirty helpers already
>> somewhere.
>> -Daniel
>
> I've asked Deepak to RFC the core changes suggested for the full dirty
> blob
> on dri-devel. It builds on DisplayLink's suggestion, with a simple
> helper to
> get to the desired coordinates.
>
> One thing to perhaps discuss is how we would like to fit this with
> front-buffer rendering and the dirty ioctl. In the page-flip context,
> the
> dirty rects, like egl's swapbuffer_with_damage is a hint to restrict
> the
> damage region that can be fully ignored by the driver, new content is
> indicated by a new framebuffer.
>
> We could do the same for frontbuffer rendering: Either set a dirty flag
> like
> you do here, or provide a content_age state member. Since we clear the
> dirty
> flag on state copies, I guess that would be sufficient. The blob
> rectangles
> would then become a hint to restrict the damage region.

 I'm not entirely following here - I thought for frontbuffer rendering
 the
 dirty rects have always just been a hint, and that the driver was always
 free to re-upload the entire buffer to the screen.

 And through a helper like Rob's proposing here (and have floated around
 in
 different versions already) we'd essentially map a frontbuffer dirtyfb
 call to a fake flip with dirty rect. Manual upload drivers already need
 to
 upload the entire screen if they get a flip, since some userspace uses
 that to flush out frontbuffer rendering (instead of calling dirtyfb).

 So from that pov the new dirty flag is kinda not necessary imo.

> Another approach would be to have the presence of dirty rects without
> framebuffer change to indicate frontbuffer rendering.
>
> I think I like the first approach best, although it may be tempting for
> user-space apps to just set the dirty bit instead of providing the full
> damage region.

 Or I'm not following you here, because I don't quite see the difference
 between dirtyfb and a flip.
 -Daniel
>>>
>>> OK, let me rephrase:
>>>
>>>  From the driver's point-of-view, in the atomic world, new content and
>>> the
>>> need for manual upload is indicated by a change in fb attached to the
>>> plane.
>>>
>>> With Rob's patch here, (correct me if I'm wrong) in addition new content
>>> and
>>> the need for manual upload is identified by the dirty flag, (since the fb
>>> 

Re: [Freedreno] [RFC] drm/atomic+msm: add helper to implement legacy dirtyfb

[Daniel Vetter]

On Thu, Apr 5, 2018 at 3:30 PM, Thomas Hellstrom  wrote:
> On 04/04/2018 11:56 AM, Daniel Vetter wrote:
>>
>> On Wed, Apr 04, 2018 at 11:10:08AM +0200, Thomas Hellstrom wrote:
>>>
>>> On 04/04/2018 10:43 AM, Daniel Vetter wrote:

 On Wed, Apr 04, 2018 at 10:22:21AM +0200, Thomas Hellstrom wrote:
>
> Hi,
>
> On 04/04/2018 08:58 AM, Daniel Vetter wrote:
>>
>> On Wed, Apr 4, 2018 at 12:42 AM, Rob Clark 
>> wrote:
>>>
>>> Add an atomic helper to implement dirtyfb support.  This is needed to
>>> support DSI command-mode panels with x11 userspace (ie. when we can't
>>> rely on pageflips to trigger a flush to the panel).
>>>
>>> To signal to the driver that the async atomic update needs to
>>> synchronize with fences, even though the fb didn't change, the
>>> drm_atomic_state::dirty flag is added.
>>>
>>> Signed-off-by: Rob Clark 
>>> ---
>>> Background: there are a number of different folks working on getting
>>> upstream kernel working on various different phones/tablets with qcom
>>> SoC's.. many of them have command mode panels, so we kind of need a
>>> way to support the legacy dirtyfb ioctl for x11 support.
>>>
>>> I know there is work on a proprer non-legacy atomic property for
>>> userspace to communicate dirty-rect(s) to the kernel, so this can
>>> be improved from triggering a full-frame flush once that is in
>>> place.  But we kinda needa a stop-gap solution.
>>>
>>> I had considered an in-driver solution for this, but things get a
>>> bit tricky if userspace ands up combining dirtyfb ioctls with page-
>>> flips, because we need to synchronize setting various CTL.FLUSH bits
>>> with setting the CTL.START bit.  (ie. really all we need to do for
>>> cmd mode panels is bang CTL.START, but is this ends up racing with
>>> pageflips setting FLUSH bits, then bad things.)  The easiest soln
>>> is to wrap this up as an atomic commit and rely on the worker to
>>> serialize things.  Hence adding an atomic dirtyfb helper.
>>>
>>> I guess at least the helper, with some small addition to translate
>>> and pass-thru the dirty rect(s) is useful to the final atomic dirty-
>>> rect property solution.  Depending on how far off that is, a stop-
>>> gap solution could be useful.
>>
>> Adding Noralf, who iirc already posted the full dirty helpers already
>> somewhere.
>> -Daniel
>
> I've asked Deepak to RFC the core changes suggested for the full dirty
> blob
> on dri-devel. It builds on DisplayLink's suggestion, with a simple
> helper to
> get to the desired coordinates.
>
> One thing to perhaps discuss is how we would like to fit this with
> front-buffer rendering and the dirty ioctl. In the page-flip context,
> the
> dirty rects, like egl's swapbuffer_with_damage is a hint to restrict
> the
> damage region that can be fully ignored by the driver, new content is
> indicated by a new framebuffer.
>
> We could do the same for frontbuffer rendering: Either set a dirty flag
> like
> you do here, or provide a content_age state member. Since we clear the
> dirty
> flag on state copies, I guess that would be sufficient. The blob
> rectangles
> would then become a hint to restrict the damage region.

 I'm not entirely following here - I thought for frontbuffer rendering
 the
 dirty rects have always just been a hint, and that the driver was always
 free to re-upload the entire buffer to the screen.

 And through a helper like Rob's proposing here (and have floated around
 in
 different versions already) we'd essentially map a frontbuffer dirtyfb
 call to a fake flip with dirty rect. Manual upload drivers already need
 to
 upload the entire screen if they get a flip, since some userspace uses
 that to flush out frontbuffer rendering (instead of calling dirtyfb).

 So from that pov the new dirty flag is kinda not necessary imo.

> Another approach would be to have the presence of dirty rects without
> framebuffer change to indicate frontbuffer rendering.
>
> I think I like the first approach best, although it may be tempting for
> user-space apps to just set the dirty bit instead of providing the full
> damage region.

 Or I'm not following you here, because I don't quite see the difference
 between dirtyfb and a flip.
 -Daniel
>>>
>>> OK, let me rephrase:
>>>
>>>  From the driver's point-of-view, in the atomic world, new content and
>>> the
>>> need for manual upload is indicated by a change in fb attached to the
>>> plane.
>>>
>>> With Rob's patch here, (correct me if I'm wrong) in addition new content
>>> and
>>> the need for manual upload is identified by the dirty flag, (since the fb
>>> 

Re: [Freedreno] [RFC] drm/atomic+msm: add helper to implement legacy dirtyfb

[Thomas Hellstrom]

On 04/04/2018 11:56 AM, Daniel Vetter wrote:

On Wed, Apr 04, 2018 at 11:10:08AM +0200, Thomas Hellstrom wrote:

On 04/04/2018 10:43 AM, Daniel Vetter wrote:

On Wed, Apr 04, 2018 at 10:22:21AM +0200, Thomas Hellstrom wrote:

Hi,

On 04/04/2018 08:58 AM, Daniel Vetter wrote:

On Wed, Apr 4, 2018 at 12:42 AM, Rob Clark  wrote:

Add an atomic helper to implement dirtyfb support.  This is needed to
support DSI command-mode panels with x11 userspace (ie. when we can't
rely on pageflips to trigger a flush to the panel).

To signal to the driver that the async atomic update needs to
synchronize with fences, even though the fb didn't change, the
drm_atomic_state::dirty flag is added.

Signed-off-by: Rob Clark 
---
Background: there are a number of different folks working on getting
upstream kernel working on various different phones/tablets with qcom
SoC's.. many of them have command mode panels, so we kind of need a
way to support the legacy dirtyfb ioctl for x11 support.

I know there is work on a proprer non-legacy atomic property for
userspace to communicate dirty-rect(s) to the kernel, so this can
be improved from triggering a full-frame flush once that is in
place.  But we kinda needa a stop-gap solution.

I had considered an in-driver solution for this, but things get a
bit tricky if userspace ands up combining dirtyfb ioctls with page-
flips, because we need to synchronize setting various CTL.FLUSH bits
with setting the CTL.START bit.  (ie. really all we need to do for
cmd mode panels is bang CTL.START, but is this ends up racing with
pageflips setting FLUSH bits, then bad things.)  The easiest soln
is to wrap this up as an atomic commit and rely on the worker to
serialize things.  Hence adding an atomic dirtyfb helper.

I guess at least the helper, with some small addition to translate
and pass-thru the dirty rect(s) is useful to the final atomic dirty-
rect property solution.  Depending on how far off that is, a stop-
gap solution could be useful.

Adding Noralf, who iirc already posted the full dirty helpers already somewhere.
-Daniel

I've asked Deepak to RFC the core changes suggested for the full dirty blob
on dri-devel. It builds on DisplayLink's suggestion, with a simple helper to
get to the desired coordinates.

One thing to perhaps discuss is how we would like to fit this with
front-buffer rendering and the dirty ioctl. In the page-flip context, the
dirty rects, like egl's swapbuffer_with_damage is a hint to restrict the
damage region that can be fully ignored by the driver, new content is
indicated by a new framebuffer.

We could do the same for frontbuffer rendering: Either set a dirty flag like
you do here, or provide a content_age state member. Since we clear the dirty
flag on state copies, I guess that would be sufficient. The blob rectangles
would then become a hint to restrict the damage region.

I'm not entirely following here - I thought for frontbuffer rendering the
dirty rects have always just been a hint, and that the driver was always
free to re-upload the entire buffer to the screen.

And through a helper like Rob's proposing here (and have floated around in
different versions already) we'd essentially map a frontbuffer dirtyfb
call to a fake flip with dirty rect. Manual upload drivers already need to
upload the entire screen if they get a flip, since some userspace uses
that to flush out frontbuffer rendering (instead of calling dirtyfb).

So from that pov the new dirty flag is kinda not necessary imo.


Another approach would be to have the presence of dirty rects without
framebuffer change to indicate frontbuffer rendering.

I think I like the first approach best, although it may be tempting for
user-space apps to just set the dirty bit instead of providing the full
damage region.

Or I'm not following you here, because I don't quite see the difference
between dirtyfb and a flip.
-Daniel

OK, let me rephrase:

 From the driver's point-of-view, in the atomic world, new content and the
need for manual upload is indicated by a change in fb attached to the plane.

With Rob's patch here, (correct me if I'm wrong) in addition new content and
the need for manual upload is identified by the dirty flag, (since the fb
stays the same and the driver thus never identifies a page-flip).

Hm, I'm not entirely sure Rob's approach is correct. Imo a pageflip
(atomic or not) should result in the entire buffer getting uploaded. The
dirty flag is kinda redundant, a flip with the same buffer works the same
way as a dirtyfb with the entire buffer as the dirty rectangle.


In both these situations, clip rects can provide a hint to restrict the
dirty region.

Now I was thinking about the preferred way for user-space to communicate
front buffer rendering through the atomic ioctl:

1) Expose a dirty (or content_age property)
2) Attach a clip-rect blob property.
3) Fake a page-flip by ping-ponging two frame-buffers pointing to the same
underlying buffer object.

Re: [Freedreno] [RFC] drm/atomic+msm: add helper to implement legacy dirtyfb

[Rob Clark]

On Wed, Apr 4, 2018 at 1:41 PM, Noralf Trønnes  wrote:
>
>
> Den 04.04.2018 00.42, skrev Rob Clark:
>>
>> Add an atomic helper to implement dirtyfb support.  This is needed to
>> support DSI command-mode panels with x11 userspace (ie. when we can't
>> rely on pageflips to trigger a flush to the panel).
>>
>> To signal to the driver that the async atomic update needs to
>> synchronize with fences, even though the fb didn't change, the
>> drm_atomic_state::dirty flag is added.
>>
>> Signed-off-by: Rob Clark 
>> ---
>> Background: there are a number of different folks working on getting
>> upstream kernel working on various different phones/tablets with qcom
>> SoC's.. many of them have command mode panels, so we kind of need a
>> way to support the legacy dirtyfb ioctl for x11 support.
>>
>> I know there is work on a proprer non-legacy atomic property for
>> userspace to communicate dirty-rect(s) to the kernel, so this can
>> be improved from triggering a full-frame flush once that is in
>> place.  But we kinda needa a stop-gap solution.
>>
>> I had considered an in-driver solution for this, but things get a
>> bit tricky if userspace ands up combining dirtyfb ioctls with page-
>> flips, because we need to synchronize setting various CTL.FLUSH bits
>> with setting the CTL.START bit.  (ie. really all we need to do for
>> cmd mode panels is bang CTL.START, but is this ends up racing with
>> pageflips setting FLUSH bits, then bad things.)  The easiest soln
>> is to wrap this up as an atomic commit and rely on the worker to
>> serialize things.  Hence adding an atomic dirtyfb helper.
>>
>> I guess at least the helper, with some small addition to translate
>> and pass-thru the dirty rect(s) is useful to the final atomic dirty-
>> rect property solution.  Depending on how far off that is, a stop-
>> gap solution could be useful.
>
>
> I have been waiting for someone to address this since I'm not skilled
> enough myself to tackle the atomic machinery. It would be be nice to do
> all flushing during commit since that would mean that the tinydrm drivers
> could be driven solely through drm_simple_display_pipe_funcs. I wouldn't
> have to wire through the dirty callback like I do now.
>
> I see that you use a nonblocking commit. What happens if a new dirtyfb
> comes in before the previous is done?

I'm relying on the workqueue for committing the async part of
non-blocking commits to serialize things.  This was actually something
I pretty badly need for msm (otherwise pageflip + dirtyfb causes races
for settings various FLUSH/START bits which need to happen in a
certain order)

This was what killed my earlier lazy plan of handling dirtyfb in drm/msm ;-)

> If we could save the dirty clips, then I could use this in tinydrm.
> A full flush over SPI takes ~50ms so I need to shave off where I can.
>
> This works for me in my tinydrm world:
>
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index c64225274807..218cb36757fa 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -28,6 +28,7 @@
>  #include 
>  #include 
>
> +struct drm_clip_rect;
>  struct drm_crtc;
>  struct drm_printer;
>  struct drm_modeset_acquire_ctx;
> @@ -85,6 +86,9 @@ struct drm_plane_state {
>  */
> bool dirty;
>
> +   struct drm_clip_rect *dirty_clips;
> +   unsigned int num_dirty_clips;
> +
> /**
>  * @fence:
>  *
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> b/drivers/gpu/drm/drm_atomic_helper.c
> index 8066c508ab50..e00b8247b7c5 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -3521,6 +3521,8 @@ void __drm_atomic_helper_plane_duplicate_state(struct
> drm_plane *plane,
> drm_framebuffer_get(state->fb);
>
> state->dirty = false;
> +   state->dirty_clips = NULL;
> +   state->num_dirty_clips = 0;
> state->fence = NULL;
> state->commit = NULL;
>  }
> @@ -3567,6 +3569,8 @@ void __drm_atomic_helper_plane_destroy_state(struct
> drm_plane_state *state)
>
> if (state->commit)
> drm_crtc_commit_put(state->commit);
> +
> +   kfree(state->dirty_clips);
>  }
>  EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state);
>
> @@ -3877,8 +3881,20 @@ int drm_atomic_helper_dirtyfb(struct drm_framebuffer
> *fb,
> struct drm_modeset_acquire_ctx ctx;
> struct drm_atomic_state *state;
> struct drm_plane *plane;
> +   bool fb_found = false;
> int ret = 0;
>
> +   /* fbdev can flush even when we don't want it to */
> +   drm_for_each_plane(plane, fb->dev) {
> +   if (plane->state->fb == fb) {
> +   fb_found = true;
> +   break;
> +   }
> +   }
> +
> +   if (!fb_found)
> +   return 0;
> +
> /*
>  * When called from ioctl, we are interruptable, but not when
>  * called 

Re: [Freedreno] [RFC] drm/atomic+msm: add helper to implement legacy dirtyfb

[Noralf Trønnes]

Den 04.04.2018 19.56, skrev Daniel Vetter:

On Wed, Apr 4, 2018 at 7:41 PM, Noralf Trønnes  wrote:


Den 04.04.2018 00.42, skrev Rob Clark:

Add an atomic helper to implement dirtyfb support.  This is needed to
support DSI command-mode panels with x11 userspace (ie. when we can't
rely on pageflips to trigger a flush to the panel).

To signal to the driver that the async atomic update needs to
synchronize with fences, even though the fb didn't change, the
drm_atomic_state::dirty flag is added.

Signed-off-by: Rob Clark 
---
Background: there are a number of different folks working on getting
upstream kernel working on various different phones/tablets with qcom
SoC's.. many of them have command mode panels, so we kind of need a
way to support the legacy dirtyfb ioctl for x11 support.

I know there is work on a proprer non-legacy atomic property for
userspace to communicate dirty-rect(s) to the kernel, so this can
be improved from triggering a full-frame flush once that is in
place.  But we kinda needa a stop-gap solution.

I had considered an in-driver solution for this, but things get a
bit tricky if userspace ands up combining dirtyfb ioctls with page-
flips, because we need to synchronize setting various CTL.FLUSH bits
with setting the CTL.START bit.  (ie. really all we need to do for
cmd mode panels is bang CTL.START, but is this ends up racing with
pageflips setting FLUSH bits, then bad things.)  The easiest soln
is to wrap this up as an atomic commit and rely on the worker to
serialize things.  Hence adding an atomic dirtyfb helper.

I guess at least the helper, with some small addition to translate
and pass-thru the dirty rect(s) is useful to the final atomic dirty-
rect property solution.  Depending on how far off that is, a stop-
gap solution could be useful.


I have been waiting for someone to address this since I'm not skilled
enough myself to tackle the atomic machinery. It would be be nice to do
all flushing during commit since that would mean that the tinydrm drivers
could be driven solely through drm_simple_display_pipe_funcs. I wouldn't
have to wire through the dirty callback like I do now.

I see that you use a nonblocking commit. What happens if a new dirtyfb
comes in before the previous is done?

We could reuse the same trick we're doing in the fbdev code, of
pushing the commit to a worker and doing it in a blocking fashion. Or
at least wait for it to finish (can be done with the crtc_state->event
stuff). That way userspace can pile us up in dirtyfb calls, but we'd
do at most one per frame. More isn't really useful anyway.


If we could save the dirty clips, then I could use this in tinydrm.
A full flush over SPI takes ~50ms so I need to shave off where I can.

This works for me in my tinydrm world:

One thing I thought you've had around somewhere is some additional
tracking code, so we don't have to take all the plane mutexes in the
->dirtyfb callback. Does that exist, or was that just a figment of my
imagination?


I haven't looked at this at all since I know way too little about how
atomic works and after the discussion started with damage on page flips,
I knew I just had to wait for someone other than me to do this. And I
hardly know anything about how the multitude of userspace operates.
So I'm sorry, but I can't add much to this discussion, I fall off rather
quickly when the details and corner cases are discussed.
All I can do is state the needs of tinydrm :-)

Noralf.

___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [RFC] drm/atomic+msm: add helper to implement legacy dirtyfb

[Noralf Trønnes]

Den 04.04.2018 00.42, skrev Rob Clark:

Add an atomic helper to implement dirtyfb support.  This is needed to
support DSI command-mode panels with x11 userspace (ie. when we can't
rely on pageflips to trigger a flush to the panel).

To signal to the driver that the async atomic update needs to
synchronize with fences, even though the fb didn't change, the
drm_atomic_state::dirty flag is added.

Signed-off-by: Rob Clark 
---
Background: there are a number of different folks working on getting
upstream kernel working on various different phones/tablets with qcom
SoC's.. many of them have command mode panels, so we kind of need a
way to support the legacy dirtyfb ioctl for x11 support.

I know there is work on a proprer non-legacy atomic property for
userspace to communicate dirty-rect(s) to the kernel, so this can
be improved from triggering a full-frame flush once that is in
place.  But we kinda needa a stop-gap solution.

I had considered an in-driver solution for this, but things get a
bit tricky if userspace ands up combining dirtyfb ioctls with page-
flips, because we need to synchronize setting various CTL.FLUSH bits
with setting the CTL.START bit.  (ie. really all we need to do for
cmd mode panels is bang CTL.START, but is this ends up racing with
pageflips setting FLUSH bits, then bad things.)  The easiest soln
is to wrap this up as an atomic commit and rely on the worker to
serialize things.  Hence adding an atomic dirtyfb helper.

I guess at least the helper, with some small addition to translate
and pass-thru the dirty rect(s) is useful to the final atomic dirty-
rect property solution.  Depending on how far off that is, a stop-
gap solution could be useful.


I have been waiting for someone to address this since I'm not skilled
enough myself to tackle the atomic machinery. It would be be nice to do
all flushing during commit since that would mean that the tinydrm drivers
could be driven solely through drm_simple_display_pipe_funcs. I wouldn't
have to wire through the dirty callback like I do now.

I see that you use a nonblocking commit. What happens if a new dirtyfb
comes in before the previous is done?

If we could save the dirty clips, then I could use this in tinydrm.
A full flush over SPI takes ~50ms so I need to shave off where I can.

This works for me in my tinydrm world:

diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
index c64225274807..218cb36757fa 100644
--- a/include/drm/drm_plane.h
+++ b/include/drm/drm_plane.h
@@ -28,6 +28,7 @@
 #include 
 #include 

+struct drm_clip_rect;
 struct drm_crtc;
 struct drm_printer;
 struct drm_modeset_acquire_ctx;
@@ -85,6 +86,9 @@ struct drm_plane_state {
 */
    bool dirty;

+   struct drm_clip_rect *dirty_clips;
+   unsigned int num_dirty_clips;
+
    /**
 * @fence:
 *
diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
b/drivers/gpu/drm/drm_atomic_helper.c

index 8066c508ab50..e00b8247b7c5 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -3521,6 +3521,8 @@ void 
__drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane,

    drm_framebuffer_get(state->fb);

    state->dirty = false;
+   state->dirty_clips = NULL;
+   state->num_dirty_clips = 0;
    state->fence = NULL;
    state->commit = NULL;
 }
@@ -3567,6 +3569,8 @@ void 
__drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state)


    if (state->commit)
    drm_crtc_commit_put(state->commit);
+
+   kfree(state->dirty_clips);
 }
 EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state);

@@ -3877,8 +3881,20 @@ int drm_atomic_helper_dirtyfb(struct 
drm_framebuffer *fb,

    struct drm_modeset_acquire_ctx ctx;
    struct drm_atomic_state *state;
    struct drm_plane *plane;
+   bool fb_found = false;
    int ret = 0;

+   /* fbdev can flush even when we don't want it to */
+   drm_for_each_plane(plane, fb->dev) {
+   if (plane->state->fb == fb) {
+   fb_found = true;
+   break;
+   }
+   }
+
+   if (!fb_found)
+   return 0;
+
    /*
 * When called from ioctl, we are interruptable, but not when
 * called internally (ie. defio worker)
@@ -3907,6 +3923,25 @@ int drm_atomic_helper_dirtyfb(struct 
drm_framebuffer *fb,

    }

    plane_state->dirty = true;
+   if (clips && num_clips)
+   plane_state->dirty_clips = kmemdup(clips, 
num_clips * sizeof(*clips),

+ GFP_KERNEL);
+   else
+   plane_state->dirty_clips = 
kzalloc(sizeof(*clips), GFP_KERNEL);

+
+   if (!plane_state->dirty_clips) {
+   ret = -ENOMEM;
+   goto out;
+   }
+
+   if (clips && num_clips) {
+   plane_state->num_dirty_clips = 

Re: [Freedreno] [RFC] drm/atomic+msm: add helper to implement legacy dirtyfb

[Daniel Vetter]

On Wed, Apr 4, 2018 at 3:24 PM, Rob Clark  wrote:
> On Wed, Apr 4, 2018 at 8:16 AM, Daniel Vetter  wrote:
>> On Wed, Apr 04, 2018 at 07:40:32AM -0400, Rob Clark wrote:
>>> On Wed, Apr 4, 2018 at 5:56 AM, Daniel Vetter  wrote:
>>> > On Wed, Apr 04, 2018 at 11:10:08AM +0200, Thomas Hellstrom wrote:
>>> >> On 04/04/2018 10:43 AM, Daniel Vetter wrote:
>>> >> > On Wed, Apr 04, 2018 at 10:22:21AM +0200, Thomas Hellstrom wrote:
>>> >> > > Hi,
>>> >> > >
>>> >> > > On 04/04/2018 08:58 AM, Daniel Vetter wrote:
>>> >> > > > On Wed, Apr 4, 2018 at 12:42 AM, Rob Clark  
>>> >> > > > wrote:
>>> >> > > > > Add an atomic helper to implement dirtyfb support.  This is 
>>> >> > > > > needed to
>>> >> > > > > support DSI command-mode panels with x11 userspace (ie. when we 
>>> >> > > > > can't
>>> >> > > > > rely on pageflips to trigger a flush to the panel).
>>> >> > > > >
>>> >> > > > > To signal to the driver that the async atomic update needs to
>>> >> > > > > synchronize with fences, even though the fb didn't change, the
>>> >> > > > > drm_atomic_state::dirty flag is added.
>>> >> > > > >
>>> >> > > > > Signed-off-by: Rob Clark 
>>> >> > > > > ---
>>> >> > > > > Background: there are a number of different folks working on 
>>> >> > > > > getting
>>> >> > > > > upstream kernel working on various different phones/tablets with 
>>> >> > > > > qcom
>>> >> > > > > SoC's.. many of them have command mode panels, so we kind of 
>>> >> > > > > need a
>>> >> > > > > way to support the legacy dirtyfb ioctl for x11 support.
>>> >> > > > >
>>> >> > > > > I know there is work on a proprer non-legacy atomic property for
>>> >> > > > > userspace to communicate dirty-rect(s) to the kernel, so this can
>>> >> > > > > be improved from triggering a full-frame flush once that is in
>>> >> > > > > place.  But we kinda needa a stop-gap solution.
>>> >> > > > >
>>> >> > > > > I had considered an in-driver solution for this, but things get a
>>> >> > > > > bit tricky if userspace ands up combining dirtyfb ioctls with 
>>> >> > > > > page-
>>> >> > > > > flips, because we need to synchronize setting various CTL.FLUSH 
>>> >> > > > > bits
>>> >> > > > > with setting the CTL.START bit.  (ie. really all we need to do 
>>> >> > > > > for
>>> >> > > > > cmd mode panels is bang CTL.START, but is this ends up racing 
>>> >> > > > > with
>>> >> > > > > pageflips setting FLUSH bits, then bad things.)  The easiest soln
>>> >> > > > > is to wrap this up as an atomic commit and rely on the worker to
>>> >> > > > > serialize things.  Hence adding an atomic dirtyfb helper.
>>> >> > > > >
>>> >> > > > > I guess at least the helper, with some small addition to 
>>> >> > > > > translate
>>> >> > > > > and pass-thru the dirty rect(s) is useful to the final atomic 
>>> >> > > > > dirty-
>>> >> > > > > rect property solution.  Depending on how far off that is, a 
>>> >> > > > > stop-
>>> >> > > > > gap solution could be useful.
>>> >> > > > Adding Noralf, who iirc already posted the full dirty helpers 
>>> >> > > > already somewhere.
>>> >> > > > -Daniel
>>> >> > > I've asked Deepak to RFC the core changes suggested for the full 
>>> >> > > dirty blob
>>> >> > > on dri-devel. It builds on DisplayLink's suggestion, with a simple 
>>> >> > > helper to
>>> >> > > get to the desired coordinates.
>>> >> > >
>>> >> > > One thing to perhaps discuss is how we would like to fit this with
>>> >> > > front-buffer rendering and the dirty ioctl. In the page-flip 
>>> >> > > context, the
>>> >> > > dirty rects, like egl's swapbuffer_with_damage is a hint to restrict 
>>> >> > > the
>>> >> > > damage region that can be fully ignored by the driver, new content is
>>> >> > > indicated by a new framebuffer.
>>> >> > >
>>> >> > > We could do the same for frontbuffer rendering: Either set a dirty 
>>> >> > > flag like
>>> >> > > you do here, or provide a content_age state member. Since we clear 
>>> >> > > the dirty
>>> >> > > flag on state copies, I guess that would be sufficient. The blob 
>>> >> > > rectangles
>>> >> > > would then become a hint to restrict the damage region.
>>> >> > I'm not entirely following here - I thought for frontbuffer rendering 
>>> >> > the
>>> >> > dirty rects have always just been a hint, and that the driver was 
>>> >> > always
>>> >> > free to re-upload the entire buffer to the screen.
>>> >> >
>>> >> > And through a helper like Rob's proposing here (and have floated 
>>> >> > around in
>>> >> > different versions already) we'd essentially map a frontbuffer dirtyfb
>>> >> > call to a fake flip with dirty rect. Manual upload drivers already 
>>> >> > need to
>>> >> > upload the entire screen if they get a flip, since some userspace uses
>>> >> > that to flush out frontbuffer rendering (instead of calling dirtyfb).
>>> >> >
>>> >> > So from that pov the new dirty flag is kinda not necessary imo.
>>> >> >
>>> >> > > Another approach would be to have the 

Re: [Freedreno] [RFC] drm/atomic+msm: add helper to implement legacy dirtyfb

[Rob Clark]

On Wed, Apr 4, 2018 at 8:16 AM, Daniel Vetter  wrote:
> On Wed, Apr 04, 2018 at 07:40:32AM -0400, Rob Clark wrote:
>> On Wed, Apr 4, 2018 at 5:56 AM, Daniel Vetter  wrote:
>> > On Wed, Apr 04, 2018 at 11:10:08AM +0200, Thomas Hellstrom wrote:
>> >> On 04/04/2018 10:43 AM, Daniel Vetter wrote:
>> >> > On Wed, Apr 04, 2018 at 10:22:21AM +0200, Thomas Hellstrom wrote:
>> >> > > Hi,
>> >> > >
>> >> > > On 04/04/2018 08:58 AM, Daniel Vetter wrote:
>> >> > > > On Wed, Apr 4, 2018 at 12:42 AM, Rob Clark  
>> >> > > > wrote:
>> >> > > > > Add an atomic helper to implement dirtyfb support.  This is 
>> >> > > > > needed to
>> >> > > > > support DSI command-mode panels with x11 userspace (ie. when we 
>> >> > > > > can't
>> >> > > > > rely on pageflips to trigger a flush to the panel).
>> >> > > > >
>> >> > > > > To signal to the driver that the async atomic update needs to
>> >> > > > > synchronize with fences, even though the fb didn't change, the
>> >> > > > > drm_atomic_state::dirty flag is added.
>> >> > > > >
>> >> > > > > Signed-off-by: Rob Clark 
>> >> > > > > ---
>> >> > > > > Background: there are a number of different folks working on 
>> >> > > > > getting
>> >> > > > > upstream kernel working on various different phones/tablets with 
>> >> > > > > qcom
>> >> > > > > SoC's.. many of them have command mode panels, so we kind of need 
>> >> > > > > a
>> >> > > > > way to support the legacy dirtyfb ioctl for x11 support.
>> >> > > > >
>> >> > > > > I know there is work on a proprer non-legacy atomic property for
>> >> > > > > userspace to communicate dirty-rect(s) to the kernel, so this can
>> >> > > > > be improved from triggering a full-frame flush once that is in
>> >> > > > > place.  But we kinda needa a stop-gap solution.
>> >> > > > >
>> >> > > > > I had considered an in-driver solution for this, but things get a
>> >> > > > > bit tricky if userspace ands up combining dirtyfb ioctls with 
>> >> > > > > page-
>> >> > > > > flips, because we need to synchronize setting various CTL.FLUSH 
>> >> > > > > bits
>> >> > > > > with setting the CTL.START bit.  (ie. really all we need to do for
>> >> > > > > cmd mode panels is bang CTL.START, but is this ends up racing with
>> >> > > > > pageflips setting FLUSH bits, then bad things.)  The easiest soln
>> >> > > > > is to wrap this up as an atomic commit and rely on the worker to
>> >> > > > > serialize things.  Hence adding an atomic dirtyfb helper.
>> >> > > > >
>> >> > > > > I guess at least the helper, with some small addition to translate
>> >> > > > > and pass-thru the dirty rect(s) is useful to the final atomic 
>> >> > > > > dirty-
>> >> > > > > rect property solution.  Depending on how far off that is, a stop-
>> >> > > > > gap solution could be useful.
>> >> > > > Adding Noralf, who iirc already posted the full dirty helpers 
>> >> > > > already somewhere.
>> >> > > > -Daniel
>> >> > > I've asked Deepak to RFC the core changes suggested for the full 
>> >> > > dirty blob
>> >> > > on dri-devel. It builds on DisplayLink's suggestion, with a simple 
>> >> > > helper to
>> >> > > get to the desired coordinates.
>> >> > >
>> >> > > One thing to perhaps discuss is how we would like to fit this with
>> >> > > front-buffer rendering and the dirty ioctl. In the page-flip context, 
>> >> > > the
>> >> > > dirty rects, like egl's swapbuffer_with_damage is a hint to restrict 
>> >> > > the
>> >> > > damage region that can be fully ignored by the driver, new content is
>> >> > > indicated by a new framebuffer.
>> >> > >
>> >> > > We could do the same for frontbuffer rendering: Either set a dirty 
>> >> > > flag like
>> >> > > you do here, or provide a content_age state member. Since we clear 
>> >> > > the dirty
>> >> > > flag on state copies, I guess that would be sufficient. The blob 
>> >> > > rectangles
>> >> > > would then become a hint to restrict the damage region.
>> >> > I'm not entirely following here - I thought for frontbuffer rendering 
>> >> > the
>> >> > dirty rects have always just been a hint, and that the driver was always
>> >> > free to re-upload the entire buffer to the screen.
>> >> >
>> >> > And through a helper like Rob's proposing here (and have floated around 
>> >> > in
>> >> > different versions already) we'd essentially map a frontbuffer dirtyfb
>> >> > call to a fake flip with dirty rect. Manual upload drivers already need 
>> >> > to
>> >> > upload the entire screen if they get a flip, since some userspace uses
>> >> > that to flush out frontbuffer rendering (instead of calling dirtyfb).
>> >> >
>> >> > So from that pov the new dirty flag is kinda not necessary imo.
>> >> >
>> >> > > Another approach would be to have the presence of dirty rects without
>> >> > > framebuffer change to indicate frontbuffer rendering.
>> >> > >
>> >> > > I think I like the first approach best, although it may be tempting 
>> >> > > for
>> >> > > user-space apps to just set the 

Re: [Freedreno] [RFC] drm/atomic+msm: add helper to implement legacy dirtyfb

[Maarten Lankhorst]

Op 04-04-18 om 14:26 schreef Daniel Vetter:
> On Wed, Apr 4, 2018 at 2:05 PM, Rob Clark  wrote:
>> On Wed, Apr 4, 2018 at 7:49 AM, Maarten Lankhorst
>>  wrote:
>>> Op 04-04-18 om 13:37 schreef Rob Clark:
 On Wed, Apr 4, 2018 at 6:36 AM, Maarten Lankhorst
  wrote:
> Op 04-04-18 om 12:21 schreef Daniel Vetter:
>> On Wed, Apr 04, 2018 at 12:03:00PM +0200, Daniel Vetter wrote:
>>> On Tue, Apr 03, 2018 at 06:42:23PM -0400, Rob Clark wrote:
 Add an atomic helper to implement dirtyfb support.  This is needed to
 support DSI command-mode panels with x11 userspace (ie. when we can't
 rely on pageflips to trigger a flush to the panel).

 To signal to the driver that the async atomic update needs to
 synchronize with fences, even though the fb didn't change, the
 drm_atomic_state::dirty flag is added.

 Signed-off-by: Rob Clark 
 ---
 Background: there are a number of different folks working on getting
 upstream kernel working on various different phones/tablets with qcom
 SoC's.. many of them have command mode panels, so we kind of need a
 way to support the legacy dirtyfb ioctl for x11 support.

 I know there is work on a proprer non-legacy atomic property for
 userspace to communicate dirty-rect(s) to the kernel, so this can
 be improved from triggering a full-frame flush once that is in
 place.  But we kinda needa a stop-gap solution.

 I had considered an in-driver solution for this, but things get a
 bit tricky if userspace ands up combining dirtyfb ioctls with page-
 flips, because we need to synchronize setting various CTL.FLUSH bits
 with setting the CTL.START bit.  (ie. really all we need to do for
 cmd mode panels is bang CTL.START, but is this ends up racing with
 pageflips setting FLUSH bits, then bad things.)  The easiest soln
 is to wrap this up as an atomic commit and rely on the worker to
 serialize things.  Hence adding an atomic dirtyfb helper.

 I guess at least the helper, with some small addition to translate
 and pass-thru the dirty rect(s) is useful to the final atomic dirty-
 rect property solution.  Depending on how far off that is, a stop-
 gap solution could be useful.

  drivers/gpu/drm/drm_atomic_helper.c | 66 
 +
  drivers/gpu/drm/msm/msm_atomic.c|  5 ++-
  drivers/gpu/drm/msm/msm_fb.c|  1 +
  include/drm/drm_atomic_helper.h |  4 +++
  include/drm/drm_plane.h |  9 +
  5 files changed, 84 insertions(+), 1 deletion(-)

 diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
 b/drivers/gpu/drm/drm_atomic_helper.c
 index c35654591c12..a578dc681b27 100644
 --- a/drivers/gpu/drm/drm_atomic_helper.c
 +++ b/drivers/gpu/drm/drm_atomic_helper.c
 @@ -3504,6 +3504,7 @@ void 
 __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane,
 if (state->fb)
 drm_framebuffer_get(state->fb);

 +   state->dirty = false;
 state->fence = NULL;
 state->commit = NULL;
  }
 @@ -3847,6 +3848,71 @@ int drm_atomic_helper_legacy_gamma_set(struct 
 drm_crtc *crtc,
  }
  EXPORT_SYMBOL(drm_atomic_helper_legacy_gamma_set);

 +/**
 + * drm_atomic_helper_dirtyfb - helper for dirtyfb
 + *
 + * A helper to implement drm_framebuffer_funcs::dirty
 + */
 +int drm_atomic_helper_dirtyfb(struct drm_framebuffer *fb,
 + struct drm_file *file_priv, unsigned flags,
 + unsigned color, struct drm_clip_rect *clips,
 + unsigned num_clips)
 +{
 +   struct drm_modeset_acquire_ctx ctx;
 +   struct drm_atomic_state *state;
 +   struct drm_plane *plane;
 +   int ret = 0;
 +
 +   /*
 +* When called from ioctl, we are interruptable, but not when
 +* called internally (ie. defio worker)
 +*/
 +   drm_modeset_acquire_init(,
 +   file_priv ? DRM_MODESET_ACQUIRE_INTERRUPTIBLE : 0);
 +
 +   state = drm_atomic_state_alloc(fb->dev);
 +   if (!state) {
 +   ret = -ENOMEM;
 +   goto out;
 +   }
 +   state->acquire_ctx = 
 +
 +retry:
 +   drm_for_each_plane(plane, fb->dev) {
 +   struct drm_plane_state *plane_state;
 +
 +   if (plane->state->fb != fb)
 + 

Re: [Freedreno] [RFC] drm/atomic+msm: add helper to implement legacy dirtyfb

[Daniel Vetter]

On Wed, Apr 4, 2018 at 2:05 PM, Rob Clark  wrote:
> On Wed, Apr 4, 2018 at 7:49 AM, Maarten Lankhorst
>  wrote:
>> Op 04-04-18 om 13:37 schreef Rob Clark:
>>> On Wed, Apr 4, 2018 at 6:36 AM, Maarten Lankhorst
>>>  wrote:
 Op 04-04-18 om 12:21 schreef Daniel Vetter:
> On Wed, Apr 04, 2018 at 12:03:00PM +0200, Daniel Vetter wrote:
>> On Tue, Apr 03, 2018 at 06:42:23PM -0400, Rob Clark wrote:
>>> Add an atomic helper to implement dirtyfb support.  This is needed to
>>> support DSI command-mode panels with x11 userspace (ie. when we can't
>>> rely on pageflips to trigger a flush to the panel).
>>>
>>> To signal to the driver that the async atomic update needs to
>>> synchronize with fences, even though the fb didn't change, the
>>> drm_atomic_state::dirty flag is added.
>>>
>>> Signed-off-by: Rob Clark 
>>> ---
>>> Background: there are a number of different folks working on getting
>>> upstream kernel working on various different phones/tablets with qcom
>>> SoC's.. many of them have command mode panels, so we kind of need a
>>> way to support the legacy dirtyfb ioctl for x11 support.
>>>
>>> I know there is work on a proprer non-legacy atomic property for
>>> userspace to communicate dirty-rect(s) to the kernel, so this can
>>> be improved from triggering a full-frame flush once that is in
>>> place.  But we kinda needa a stop-gap solution.
>>>
>>> I had considered an in-driver solution for this, but things get a
>>> bit tricky if userspace ands up combining dirtyfb ioctls with page-
>>> flips, because we need to synchronize setting various CTL.FLUSH bits
>>> with setting the CTL.START bit.  (ie. really all we need to do for
>>> cmd mode panels is bang CTL.START, but is this ends up racing with
>>> pageflips setting FLUSH bits, then bad things.)  The easiest soln
>>> is to wrap this up as an atomic commit and rely on the worker to
>>> serialize things.  Hence adding an atomic dirtyfb helper.
>>>
>>> I guess at least the helper, with some small addition to translate
>>> and pass-thru the dirty rect(s) is useful to the final atomic dirty-
>>> rect property solution.  Depending on how far off that is, a stop-
>>> gap solution could be useful.
>>>
>>>  drivers/gpu/drm/drm_atomic_helper.c | 66 
>>> +
>>>  drivers/gpu/drm/msm/msm_atomic.c|  5 ++-
>>>  drivers/gpu/drm/msm/msm_fb.c|  1 +
>>>  include/drm/drm_atomic_helper.h |  4 +++
>>>  include/drm/drm_plane.h |  9 +
>>>  5 files changed, 84 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
>>> b/drivers/gpu/drm/drm_atomic_helper.c
>>> index c35654591c12..a578dc681b27 100644
>>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>>> @@ -3504,6 +3504,7 @@ void 
>>> __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane,
>>> if (state->fb)
>>> drm_framebuffer_get(state->fb);
>>>
>>> +   state->dirty = false;
>>> state->fence = NULL;
>>> state->commit = NULL;
>>>  }
>>> @@ -3847,6 +3848,71 @@ int drm_atomic_helper_legacy_gamma_set(struct 
>>> drm_crtc *crtc,
>>>  }
>>>  EXPORT_SYMBOL(drm_atomic_helper_legacy_gamma_set);
>>>
>>> +/**
>>> + * drm_atomic_helper_dirtyfb - helper for dirtyfb
>>> + *
>>> + * A helper to implement drm_framebuffer_funcs::dirty
>>> + */
>>> +int drm_atomic_helper_dirtyfb(struct drm_framebuffer *fb,
>>> + struct drm_file *file_priv, unsigned flags,
>>> + unsigned color, struct drm_clip_rect *clips,
>>> + unsigned num_clips)
>>> +{
>>> +   struct drm_modeset_acquire_ctx ctx;
>>> +   struct drm_atomic_state *state;
>>> +   struct drm_plane *plane;
>>> +   int ret = 0;
>>> +
>>> +   /*
>>> +* When called from ioctl, we are interruptable, but not when
>>> +* called internally (ie. defio worker)
>>> +*/
>>> +   drm_modeset_acquire_init(,
>>> +   file_priv ? DRM_MODESET_ACQUIRE_INTERRUPTIBLE : 0);
>>> +
>>> +   state = drm_atomic_state_alloc(fb->dev);
>>> +   if (!state) {
>>> +   ret = -ENOMEM;
>>> +   goto out;
>>> +   }
>>> +   state->acquire_ctx = 
>>> +
>>> +retry:
>>> +   drm_for_each_plane(plane, fb->dev) {
>>> +   struct drm_plane_state *plane_state;
>>> +
>>> +   if (plane->state->fb != fb)
>>> +   continue;
>>> +
>>> +   plane_state = drm_atomic_get_plane_state(state, plane);
>>> +   if 

Re: [Freedreno] [RFC] drm/atomic+msm: add helper to implement legacy dirtyfb

[Maarten Lankhorst]

Op 04-04-18 om 14:05 schreef Rob Clark:
> On Wed, Apr 4, 2018 at 7:49 AM, Maarten Lankhorst
>  wrote:
>> Op 04-04-18 om 13:37 schreef Rob Clark:
>>> On Wed, Apr 4, 2018 at 6:36 AM, Maarten Lankhorst
>>>  wrote:
 Op 04-04-18 om 12:21 schreef Daniel Vetter:
> On Wed, Apr 04, 2018 at 12:03:00PM +0200, Daniel Vetter wrote:
>> On Tue, Apr 03, 2018 at 06:42:23PM -0400, Rob Clark wrote:
>>> Add an atomic helper to implement dirtyfb support.  This is needed to
>>> support DSI command-mode panels with x11 userspace (ie. when we can't
>>> rely on pageflips to trigger a flush to the panel).
>>>
>>> To signal to the driver that the async atomic update needs to
>>> synchronize with fences, even though the fb didn't change, the
>>> drm_atomic_state::dirty flag is added.
>>>
>>> Signed-off-by: Rob Clark 
>>> ---
>>> Background: there are a number of different folks working on getting
>>> upstream kernel working on various different phones/tablets with qcom
>>> SoC's.. many of them have command mode panels, so we kind of need a
>>> way to support the legacy dirtyfb ioctl for x11 support.
>>>
>>> I know there is work on a proprer non-legacy atomic property for
>>> userspace to communicate dirty-rect(s) to the kernel, so this can
>>> be improved from triggering a full-frame flush once that is in
>>> place.  But we kinda needa a stop-gap solution.
>>>
>>> I had considered an in-driver solution for this, but things get a
>>> bit tricky if userspace ands up combining dirtyfb ioctls with page-
>>> flips, because we need to synchronize setting various CTL.FLUSH bits
>>> with setting the CTL.START bit.  (ie. really all we need to do for
>>> cmd mode panels is bang CTL.START, but is this ends up racing with
>>> pageflips setting FLUSH bits, then bad things.)  The easiest soln
>>> is to wrap this up as an atomic commit and rely on the worker to
>>> serialize things.  Hence adding an atomic dirtyfb helper.
>>>
>>> I guess at least the helper, with some small addition to translate
>>> and pass-thru the dirty rect(s) is useful to the final atomic dirty-
>>> rect property solution.  Depending on how far off that is, a stop-
>>> gap solution could be useful.
>>>
>>>  drivers/gpu/drm/drm_atomic_helper.c | 66 
>>> +
>>>  drivers/gpu/drm/msm/msm_atomic.c|  5 ++-
>>>  drivers/gpu/drm/msm/msm_fb.c|  1 +
>>>  include/drm/drm_atomic_helper.h |  4 +++
>>>  include/drm/drm_plane.h |  9 +
>>>  5 files changed, 84 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
>>> b/drivers/gpu/drm/drm_atomic_helper.c
>>> index c35654591c12..a578dc681b27 100644
>>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>>> @@ -3504,6 +3504,7 @@ void 
>>> __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane,
>>> if (state->fb)
>>> drm_framebuffer_get(state->fb);
>>>
>>> +   state->dirty = false;
>>> state->fence = NULL;
>>> state->commit = NULL;
>>>  }
>>> @@ -3847,6 +3848,71 @@ int drm_atomic_helper_legacy_gamma_set(struct 
>>> drm_crtc *crtc,
>>>  }
>>>  EXPORT_SYMBOL(drm_atomic_helper_legacy_gamma_set);
>>>
>>> +/**
>>> + * drm_atomic_helper_dirtyfb - helper for dirtyfb
>>> + *
>>> + * A helper to implement drm_framebuffer_funcs::dirty
>>> + */
>>> +int drm_atomic_helper_dirtyfb(struct drm_framebuffer *fb,
>>> + struct drm_file *file_priv, unsigned flags,
>>> + unsigned color, struct drm_clip_rect *clips,
>>> + unsigned num_clips)
>>> +{
>>> +   struct drm_modeset_acquire_ctx ctx;
>>> +   struct drm_atomic_state *state;
>>> +   struct drm_plane *plane;
>>> +   int ret = 0;
>>> +
>>> +   /*
>>> +* When called from ioctl, we are interruptable, but not when
>>> +* called internally (ie. defio worker)
>>> +*/
>>> +   drm_modeset_acquire_init(,
>>> +   file_priv ? DRM_MODESET_ACQUIRE_INTERRUPTIBLE : 0);
>>> +
>>> +   state = drm_atomic_state_alloc(fb->dev);
>>> +   if (!state) {
>>> +   ret = -ENOMEM;
>>> +   goto out;
>>> +   }
>>> +   state->acquire_ctx = 
>>> +
>>> +retry:
>>> +   drm_for_each_plane(plane, fb->dev) {
>>> +   struct drm_plane_state *plane_state;
>>> +
>>> +   if (plane->state->fb != fb)
>>> +   continue;
>>> +
>>> +   plane_state = drm_atomic_get_plane_state(state, plane);
>>> +   if (IS_ERR(plane_state)) {
>>> +   

Re: [Freedreno] [RFC] drm/atomic+msm: add helper to implement legacy dirtyfb

[Daniel Vetter]

On Wed, Apr 04, 2018 at 07:35:58AM -0400, Rob Clark wrote:
> On Wed, Apr 4, 2018 at 6:21 AM, Daniel Vetter  wrote:
> > On Wed, Apr 04, 2018 at 12:03:00PM +0200, Daniel Vetter wrote:
> >> On Tue, Apr 03, 2018 at 06:42:23PM -0400, Rob Clark wrote:
> >> > Add an atomic helper to implement dirtyfb support.  This is needed to
> >> > support DSI command-mode panels with x11 userspace (ie. when we can't
> >> > rely on pageflips to trigger a flush to the panel).
> >> >
> >> > To signal to the driver that the async atomic update needs to
> >> > synchronize with fences, even though the fb didn't change, the
> >> > drm_atomic_state::dirty flag is added.
> >> >
> >> > Signed-off-by: Rob Clark 
> >> > ---
> >> > Background: there are a number of different folks working on getting
> >> > upstream kernel working on various different phones/tablets with qcom
> >> > SoC's.. many of them have command mode panels, so we kind of need a
> >> > way to support the legacy dirtyfb ioctl for x11 support.
> >> >
> >> > I know there is work on a proprer non-legacy atomic property for
> >> > userspace to communicate dirty-rect(s) to the kernel, so this can
> >> > be improved from triggering a full-frame flush once that is in
> >> > place.  But we kinda needa a stop-gap solution.
> >> >
> >> > I had considered an in-driver solution for this, but things get a
> >> > bit tricky if userspace ands up combining dirtyfb ioctls with page-
> >> > flips, because we need to synchronize setting various CTL.FLUSH bits
> >> > with setting the CTL.START bit.  (ie. really all we need to do for
> >> > cmd mode panels is bang CTL.START, but is this ends up racing with
> >> > pageflips setting FLUSH bits, then bad things.)  The easiest soln
> >> > is to wrap this up as an atomic commit and rely on the worker to
> >> > serialize things.  Hence adding an atomic dirtyfb helper.
> >> >
> >> > I guess at least the helper, with some small addition to translate
> >> > and pass-thru the dirty rect(s) is useful to the final atomic dirty-
> >> > rect property solution.  Depending on how far off that is, a stop-
> >> > gap solution could be useful.
> >> >
> >> >  drivers/gpu/drm/drm_atomic_helper.c | 66 
> >> > +
> >> >  drivers/gpu/drm/msm/msm_atomic.c|  5 ++-
> >> >  drivers/gpu/drm/msm/msm_fb.c|  1 +
> >> >  include/drm/drm_atomic_helper.h |  4 +++
> >> >  include/drm/drm_plane.h |  9 +
> >> >  5 files changed, 84 insertions(+), 1 deletion(-)
> >> >
> >> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
> >> > b/drivers/gpu/drm/drm_atomic_helper.c
> >> > index c35654591c12..a578dc681b27 100644
> >> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> >> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> >> > @@ -3504,6 +3504,7 @@ void 
> >> > __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane,
> >> > if (state->fb)
> >> > drm_framebuffer_get(state->fb);
> >> >
> >> > +   state->dirty = false;
> >> > state->fence = NULL;
> >> > state->commit = NULL;
> >> >  }
> >> > @@ -3847,6 +3848,71 @@ int drm_atomic_helper_legacy_gamma_set(struct 
> >> > drm_crtc *crtc,
> >> >  }
> >> >  EXPORT_SYMBOL(drm_atomic_helper_legacy_gamma_set);
> >> >
> >> > +/**
> >> > + * drm_atomic_helper_dirtyfb - helper for dirtyfb
> >> > + *
> >> > + * A helper to implement drm_framebuffer_funcs::dirty
> >> > + */
> >> > +int drm_atomic_helper_dirtyfb(struct drm_framebuffer *fb,
> >> > + struct drm_file *file_priv, unsigned flags,
> >> > + unsigned color, struct drm_clip_rect *clips,
> >> > + unsigned num_clips)
> >> > +{
> >> > +   struct drm_modeset_acquire_ctx ctx;
> >> > +   struct drm_atomic_state *state;
> >> > +   struct drm_plane *plane;
> >> > +   int ret = 0;
> >> > +
> >> > +   /*
> >> > +* When called from ioctl, we are interruptable, but not when
> >> > +* called internally (ie. defio worker)
> >> > +*/
> >> > +   drm_modeset_acquire_init(,
> >> > +   file_priv ? DRM_MODESET_ACQUIRE_INTERRUPTIBLE : 0);
> >> > +
> >> > +   state = drm_atomic_state_alloc(fb->dev);
> >> > +   if (!state) {
> >> > +   ret = -ENOMEM;
> >> > +   goto out;
> >> > +   }
> >> > +   state->acquire_ctx = 
> >> > +
> >> > +retry:
> >> > +   drm_for_each_plane(plane, fb->dev) {
> >> > +   struct drm_plane_state *plane_state;
> >> > +
> >> > +   if (plane->state->fb != fb)
> >> > +   continue;
> >> > +
> >> > +   plane_state = drm_atomic_get_plane_state(state, plane);
> >> > +   if (IS_ERR(plane_state)) {
> >> > +   ret = PTR_ERR(plane_state);
> >> > +   goto out;
> >> > +   }
> >> > +
> >> > +   plane_state->dirty = true;
> >> > +   }
> >> > +
> >> > +   ret = drm_atomic_nonblocking_commit(state);
> >> > +
> >> > +out:
> >> > +   if (ret == -EDEADLK) {
> >> > +   

Re: [Freedreno] [RFC] drm/atomic+msm: add helper to implement legacy dirtyfb

[Daniel Vetter]

On Wed, Apr 04, 2018 at 07:40:32AM -0400, Rob Clark wrote:
> On Wed, Apr 4, 2018 at 5:56 AM, Daniel Vetter  wrote:
> > On Wed, Apr 04, 2018 at 11:10:08AM +0200, Thomas Hellstrom wrote:
> >> On 04/04/2018 10:43 AM, Daniel Vetter wrote:
> >> > On Wed, Apr 04, 2018 at 10:22:21AM +0200, Thomas Hellstrom wrote:
> >> > > Hi,
> >> > >
> >> > > On 04/04/2018 08:58 AM, Daniel Vetter wrote:
> >> > > > On Wed, Apr 4, 2018 at 12:42 AM, Rob Clark  
> >> > > > wrote:
> >> > > > > Add an atomic helper to implement dirtyfb support.  This is needed 
> >> > > > > to
> >> > > > > support DSI command-mode panels with x11 userspace (ie. when we 
> >> > > > > can't
> >> > > > > rely on pageflips to trigger a flush to the panel).
> >> > > > >
> >> > > > > To signal to the driver that the async atomic update needs to
> >> > > > > synchronize with fences, even though the fb didn't change, the
> >> > > > > drm_atomic_state::dirty flag is added.
> >> > > > >
> >> > > > > Signed-off-by: Rob Clark 
> >> > > > > ---
> >> > > > > Background: there are a number of different folks working on 
> >> > > > > getting
> >> > > > > upstream kernel working on various different phones/tablets with 
> >> > > > > qcom
> >> > > > > SoC's.. many of them have command mode panels, so we kind of need a
> >> > > > > way to support the legacy dirtyfb ioctl for x11 support.
> >> > > > >
> >> > > > > I know there is work on a proprer non-legacy atomic property for
> >> > > > > userspace to communicate dirty-rect(s) to the kernel, so this can
> >> > > > > be improved from triggering a full-frame flush once that is in
> >> > > > > place.  But we kinda needa a stop-gap solution.
> >> > > > >
> >> > > > > I had considered an in-driver solution for this, but things get a
> >> > > > > bit tricky if userspace ands up combining dirtyfb ioctls with page-
> >> > > > > flips, because we need to synchronize setting various CTL.FLUSH 
> >> > > > > bits
> >> > > > > with setting the CTL.START bit.  (ie. really all we need to do for
> >> > > > > cmd mode panels is bang CTL.START, but is this ends up racing with
> >> > > > > pageflips setting FLUSH bits, then bad things.)  The easiest soln
> >> > > > > is to wrap this up as an atomic commit and rely on the worker to
> >> > > > > serialize things.  Hence adding an atomic dirtyfb helper.
> >> > > > >
> >> > > > > I guess at least the helper, with some small addition to translate
> >> > > > > and pass-thru the dirty rect(s) is useful to the final atomic 
> >> > > > > dirty-
> >> > > > > rect property solution.  Depending on how far off that is, a stop-
> >> > > > > gap solution could be useful.
> >> > > > Adding Noralf, who iirc already posted the full dirty helpers 
> >> > > > already somewhere.
> >> > > > -Daniel
> >> > > I've asked Deepak to RFC the core changes suggested for the full dirty 
> >> > > blob
> >> > > on dri-devel. It builds on DisplayLink's suggestion, with a simple 
> >> > > helper to
> >> > > get to the desired coordinates.
> >> > >
> >> > > One thing to perhaps discuss is how we would like to fit this with
> >> > > front-buffer rendering and the dirty ioctl. In the page-flip context, 
> >> > > the
> >> > > dirty rects, like egl's swapbuffer_with_damage is a hint to restrict 
> >> > > the
> >> > > damage region that can be fully ignored by the driver, new content is
> >> > > indicated by a new framebuffer.
> >> > >
> >> > > We could do the same for frontbuffer rendering: Either set a dirty 
> >> > > flag like
> >> > > you do here, or provide a content_age state member. Since we clear the 
> >> > > dirty
> >> > > flag on state copies, I guess that would be sufficient. The blob 
> >> > > rectangles
> >> > > would then become a hint to restrict the damage region.
> >> > I'm not entirely following here - I thought for frontbuffer rendering the
> >> > dirty rects have always just been a hint, and that the driver was always
> >> > free to re-upload the entire buffer to the screen.
> >> >
> >> > And through a helper like Rob's proposing here (and have floated around 
> >> > in
> >> > different versions already) we'd essentially map a frontbuffer dirtyfb
> >> > call to a fake flip with dirty rect. Manual upload drivers already need 
> >> > to
> >> > upload the entire screen if they get a flip, since some userspace uses
> >> > that to flush out frontbuffer rendering (instead of calling dirtyfb).
> >> >
> >> > So from that pov the new dirty flag is kinda not necessary imo.
> >> >
> >> > > Another approach would be to have the presence of dirty rects without
> >> > > framebuffer change to indicate frontbuffer rendering.
> >> > >
> >> > > I think I like the first approach best, although it may be tempting for
> >> > > user-space apps to just set the dirty bit instead of providing the full
> >> > > damage region.
> >> > Or I'm not following you here, because I don't quite see the difference
> >> > between dirtyfb and a flip.
> >> > -Daniel
> >>
> >> OK, let me 

Re: [Freedreno] [RFC] drm/atomic+msm: add helper to implement legacy dirtyfb

[Daniel Vetter]

On Wed, Apr 04, 2018 at 01:46:37PM +0200, Thomas Hellstrom wrote:
> On 04/04/2018 12:28 PM, Thomas Hellstrom wrote:
> > On 04/04/2018 11:56 AM, Daniel Vetter wrote:
> > > On Wed, Apr 04, 2018 at 11:10:08AM +0200, Thomas Hellstrom wrote:
> > > > On 04/04/2018 10:43 AM, Daniel Vetter wrote:
> > > > > On Wed, Apr 04, 2018 at 10:22:21AM +0200, Thomas Hellstrom wrote:
> > > > > > Hi,
> > > > > > 
> > > > > > On 04/04/2018 08:58 AM, Daniel Vetter wrote:
> > > > > > > On Wed, Apr 4, 2018 at 12:42 AM, Rob Clark
> > > > > > >  wrote:
> > > > > > > > Add an atomic helper to implement dirtyfb
> > > > > > > > support.  This is needed to
> > > > > > > > support DSI command-mode panels with x11
> > > > > > > > userspace (ie. when we can't
> > > > > > > > rely on pageflips to trigger a flush to the panel).
> > > > > > > > 
> > > > > > > > To signal to the driver that the async atomic update needs to
> > > > > > > > synchronize with fences, even though the fb didn't change, the
> > > > > > > > drm_atomic_state::dirty flag is added.
> > > > > > > > 
> > > > > > > > Signed-off-by: Rob Clark 
> > > > > > > > ---
> > > > > > > > Background: there are a number of different
> > > > > > > > folks working on getting
> > > > > > > > upstream kernel working on various different
> > > > > > > > phones/tablets with qcom
> > > > > > > > SoC's.. many of them have command mode panels, so we kind of 
> > > > > > > > need a
> > > > > > > > way to support the legacy dirtyfb ioctl for x11 support.
> > > > > > > > 
> > > > > > > > I know there is work on a proprer non-legacy atomic property for
> > > > > > > > userspace to communicate dirty-rect(s) to the kernel, so this 
> > > > > > > > can
> > > > > > > > be improved from triggering a full-frame flush once that is in
> > > > > > > > place.  But we kinda needa a stop-gap solution.
> > > > > > > > 
> > > > > > > > I had considered an in-driver solution for this, but things get 
> > > > > > > > a
> > > > > > > > bit tricky if userspace ands up combining dirtyfb ioctls with 
> > > > > > > > page-
> > > > > > > > flips, because we need to synchronize setting
> > > > > > > > various CTL.FLUSH bits
> > > > > > > > with setting the CTL.START bit.  (ie. really all we need to do 
> > > > > > > > for
> > > > > > > > cmd mode panels is bang CTL.START, but is this ends up racing 
> > > > > > > > with
> > > > > > > > pageflips setting FLUSH bits, then bad things.)  The easiest 
> > > > > > > > soln
> > > > > > > > is to wrap this up as an atomic commit and rely on the worker to
> > > > > > > > serialize things.  Hence adding an atomic dirtyfb helper.
> > > > > > > > 
> > > > > > > > I guess at least the helper, with some small addition to 
> > > > > > > > translate
> > > > > > > > and pass-thru the dirty rect(s) is useful to the
> > > > > > > > final atomic dirty-
> > > > > > > > rect property solution.  Depending on how far off that is, a 
> > > > > > > > stop-
> > > > > > > > gap solution could be useful.
> > > > > > > Adding Noralf, who iirc already posted the full
> > > > > > > dirty helpers already somewhere.
> > > > > > > -Daniel
> > > > > > I've asked Deepak to RFC the core changes suggested for
> > > > > > the full dirty blob
> > > > > > on dri-devel. It builds on DisplayLink's suggestion,
> > > > > > with a simple helper to
> > > > > > get to the desired coordinates.
> > > > > > 
> > > > > > One thing to perhaps discuss is how we would like to fit this with
> > > > > > front-buffer rendering and the dirty ioctl. In the
> > > > > > page-flip context, the
> > > > > > dirty rects, like egl's swapbuffer_with_damage is a hint
> > > > > > to restrict the
> > > > > > damage region that can be fully ignored by the driver, new content 
> > > > > > is
> > > > > > indicated by a new framebuffer.
> > > > > > 
> > > > > > We could do the same for frontbuffer rendering: Either
> > > > > > set a dirty flag like
> > > > > > you do here, or provide a content_age state member.
> > > > > > Since we clear the dirty
> > > > > > flag on state copies, I guess that would be sufficient.
> > > > > > The blob rectangles
> > > > > > would then become a hint to restrict the damage region.
> > > > > I'm not entirely following here - I thought for frontbuffer
> > > > > rendering the
> > > > > dirty rects have always just been a hint, and that the
> > > > > driver was always
> > > > > free to re-upload the entire buffer to the screen.
> > > > > 
> > > > > And through a helper like Rob's proposing here (and have
> > > > > floated around in
> > > > > different versions already) we'd essentially map a frontbuffer dirtyfb
> > > > > call to a fake flip with dirty rect. Manual upload drivers
> > > > > already need to
> > > > > upload the entire screen if they get a flip, since some userspace uses
> > > > > that to flush out frontbuffer rendering (instead of calling dirtyfb).
> > > > > 
> > > > > So from that pov the new dirty flag is kinda not necessary imo.
> > > > > 
> > > > > > Another 

Re: [Freedreno] [RFC] drm/atomic+msm: add helper to implement legacy dirtyfb

[Rob Clark]

On Wed, Apr 4, 2018 at 7:49 AM, Maarten Lankhorst
 wrote:
> Op 04-04-18 om 13:37 schreef Rob Clark:
>> On Wed, Apr 4, 2018 at 6:36 AM, Maarten Lankhorst
>>  wrote:
>>> Op 04-04-18 om 12:21 schreef Daniel Vetter:
 On Wed, Apr 04, 2018 at 12:03:00PM +0200, Daniel Vetter wrote:
> On Tue, Apr 03, 2018 at 06:42:23PM -0400, Rob Clark wrote:
>> Add an atomic helper to implement dirtyfb support.  This is needed to
>> support DSI command-mode panels with x11 userspace (ie. when we can't
>> rely on pageflips to trigger a flush to the panel).
>>
>> To signal to the driver that the async atomic update needs to
>> synchronize with fences, even though the fb didn't change, the
>> drm_atomic_state::dirty flag is added.
>>
>> Signed-off-by: Rob Clark 
>> ---
>> Background: there are a number of different folks working on getting
>> upstream kernel working on various different phones/tablets with qcom
>> SoC's.. many of them have command mode panels, so we kind of need a
>> way to support the legacy dirtyfb ioctl for x11 support.
>>
>> I know there is work on a proprer non-legacy atomic property for
>> userspace to communicate dirty-rect(s) to the kernel, so this can
>> be improved from triggering a full-frame flush once that is in
>> place.  But we kinda needa a stop-gap solution.
>>
>> I had considered an in-driver solution for this, but things get a
>> bit tricky if userspace ands up combining dirtyfb ioctls with page-
>> flips, because we need to synchronize setting various CTL.FLUSH bits
>> with setting the CTL.START bit.  (ie. really all we need to do for
>> cmd mode panels is bang CTL.START, but is this ends up racing with
>> pageflips setting FLUSH bits, then bad things.)  The easiest soln
>> is to wrap this up as an atomic commit and rely on the worker to
>> serialize things.  Hence adding an atomic dirtyfb helper.
>>
>> I guess at least the helper, with some small addition to translate
>> and pass-thru the dirty rect(s) is useful to the final atomic dirty-
>> rect property solution.  Depending on how far off that is, a stop-
>> gap solution could be useful.
>>
>>  drivers/gpu/drm/drm_atomic_helper.c | 66 
>> +
>>  drivers/gpu/drm/msm/msm_atomic.c|  5 ++-
>>  drivers/gpu/drm/msm/msm_fb.c|  1 +
>>  include/drm/drm_atomic_helper.h |  4 +++
>>  include/drm/drm_plane.h |  9 +
>>  5 files changed, 84 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
>> b/drivers/gpu/drm/drm_atomic_helper.c
>> index c35654591c12..a578dc681b27 100644
>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>> @@ -3504,6 +3504,7 @@ void 
>> __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane,
>> if (state->fb)
>> drm_framebuffer_get(state->fb);
>>
>> +   state->dirty = false;
>> state->fence = NULL;
>> state->commit = NULL;
>>  }
>> @@ -3847,6 +3848,71 @@ int drm_atomic_helper_legacy_gamma_set(struct 
>> drm_crtc *crtc,
>>  }
>>  EXPORT_SYMBOL(drm_atomic_helper_legacy_gamma_set);
>>
>> +/**
>> + * drm_atomic_helper_dirtyfb - helper for dirtyfb
>> + *
>> + * A helper to implement drm_framebuffer_funcs::dirty
>> + */
>> +int drm_atomic_helper_dirtyfb(struct drm_framebuffer *fb,
>> + struct drm_file *file_priv, unsigned flags,
>> + unsigned color, struct drm_clip_rect *clips,
>> + unsigned num_clips)
>> +{
>> +   struct drm_modeset_acquire_ctx ctx;
>> +   struct drm_atomic_state *state;
>> +   struct drm_plane *plane;
>> +   int ret = 0;
>> +
>> +   /*
>> +* When called from ioctl, we are interruptable, but not when
>> +* called internally (ie. defio worker)
>> +*/
>> +   drm_modeset_acquire_init(,
>> +   file_priv ? DRM_MODESET_ACQUIRE_INTERRUPTIBLE : 0);
>> +
>> +   state = drm_atomic_state_alloc(fb->dev);
>> +   if (!state) {
>> +   ret = -ENOMEM;
>> +   goto out;
>> +   }
>> +   state->acquire_ctx = 
>> +
>> +retry:
>> +   drm_for_each_plane(plane, fb->dev) {
>> +   struct drm_plane_state *plane_state;
>> +
>> +   if (plane->state->fb != fb)
>> +   continue;
>> +
>> +   plane_state = drm_atomic_get_plane_state(state, plane);
>> +   if (IS_ERR(plane_state)) {
>> +   ret = PTR_ERR(plane_state);
>> +   goto out;
>> +   }
>> +
>> +   plane_state->dirty = true;

Re: [Freedreno] [RFC] drm/atomic+msm: add helper to implement legacy dirtyfb

[Maarten Lankhorst]

Op 04-04-18 om 13:37 schreef Rob Clark:
> On Wed, Apr 4, 2018 at 6:36 AM, Maarten Lankhorst
>  wrote:
>> Op 04-04-18 om 12:21 schreef Daniel Vetter:
>>> On Wed, Apr 04, 2018 at 12:03:00PM +0200, Daniel Vetter wrote:
 On Tue, Apr 03, 2018 at 06:42:23PM -0400, Rob Clark wrote:
> Add an atomic helper to implement dirtyfb support.  This is needed to
> support DSI command-mode panels with x11 userspace (ie. when we can't
> rely on pageflips to trigger a flush to the panel).
>
> To signal to the driver that the async atomic update needs to
> synchronize with fences, even though the fb didn't change, the
> drm_atomic_state::dirty flag is added.
>
> Signed-off-by: Rob Clark 
> ---
> Background: there are a number of different folks working on getting
> upstream kernel working on various different phones/tablets with qcom
> SoC's.. many of them have command mode panels, so we kind of need a
> way to support the legacy dirtyfb ioctl for x11 support.
>
> I know there is work on a proprer non-legacy atomic property for
> userspace to communicate dirty-rect(s) to the kernel, so this can
> be improved from triggering a full-frame flush once that is in
> place.  But we kinda needa a stop-gap solution.
>
> I had considered an in-driver solution for this, but things get a
> bit tricky if userspace ands up combining dirtyfb ioctls with page-
> flips, because we need to synchronize setting various CTL.FLUSH bits
> with setting the CTL.START bit.  (ie. really all we need to do for
> cmd mode panels is bang CTL.START, but is this ends up racing with
> pageflips setting FLUSH bits, then bad things.)  The easiest soln
> is to wrap this up as an atomic commit and rely on the worker to
> serialize things.  Hence adding an atomic dirtyfb helper.
>
> I guess at least the helper, with some small addition to translate
> and pass-thru the dirty rect(s) is useful to the final atomic dirty-
> rect property solution.  Depending on how far off that is, a stop-
> gap solution could be useful.
>
>  drivers/gpu/drm/drm_atomic_helper.c | 66 
> +
>  drivers/gpu/drm/msm/msm_atomic.c|  5 ++-
>  drivers/gpu/drm/msm/msm_fb.c|  1 +
>  include/drm/drm_atomic_helper.h |  4 +++
>  include/drm/drm_plane.h |  9 +
>  5 files changed, 84 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
> b/drivers/gpu/drm/drm_atomic_helper.c
> index c35654591c12..a578dc681b27 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -3504,6 +3504,7 @@ void 
> __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane,
> if (state->fb)
> drm_framebuffer_get(state->fb);
>
> +   state->dirty = false;
> state->fence = NULL;
> state->commit = NULL;
>  }
> @@ -3847,6 +3848,71 @@ int drm_atomic_helper_legacy_gamma_set(struct 
> drm_crtc *crtc,
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_legacy_gamma_set);
>
> +/**
> + * drm_atomic_helper_dirtyfb - helper for dirtyfb
> + *
> + * A helper to implement drm_framebuffer_funcs::dirty
> + */
> +int drm_atomic_helper_dirtyfb(struct drm_framebuffer *fb,
> + struct drm_file *file_priv, unsigned flags,
> + unsigned color, struct drm_clip_rect *clips,
> + unsigned num_clips)
> +{
> +   struct drm_modeset_acquire_ctx ctx;
> +   struct drm_atomic_state *state;
> +   struct drm_plane *plane;
> +   int ret = 0;
> +
> +   /*
> +* When called from ioctl, we are interruptable, but not when
> +* called internally (ie. defio worker)
> +*/
> +   drm_modeset_acquire_init(,
> +   file_priv ? DRM_MODESET_ACQUIRE_INTERRUPTIBLE : 0);
> +
> +   state = drm_atomic_state_alloc(fb->dev);
> +   if (!state) {
> +   ret = -ENOMEM;
> +   goto out;
> +   }
> +   state->acquire_ctx = 
> +
> +retry:
> +   drm_for_each_plane(plane, fb->dev) {
> +   struct drm_plane_state *plane_state;
> +
> +   if (plane->state->fb != fb)
> +   continue;
> +
> +   plane_state = drm_atomic_get_plane_state(state, plane);
> +   if (IS_ERR(plane_state)) {
> +   ret = PTR_ERR(plane_state);
> +   goto out;
> +   }
> +
> +   plane_state->dirty = true;
> +   }
> +
> +   ret = drm_atomic_nonblocking_commit(state);
> +
> +out:
> +   if (ret == -EDEADLK) {
> +   drm_atomic_state_clear(state);
> +   ret = 

Re: [Freedreno] [RFC] drm/atomic+msm: add helper to implement legacy dirtyfb

[Rob Clark]

On Wed, Apr 4, 2018 at 5:56 AM, Daniel Vetter  wrote:
> On Wed, Apr 04, 2018 at 11:10:08AM +0200, Thomas Hellstrom wrote:
>> On 04/04/2018 10:43 AM, Daniel Vetter wrote:
>> > On Wed, Apr 04, 2018 at 10:22:21AM +0200, Thomas Hellstrom wrote:
>> > > Hi,
>> > >
>> > > On 04/04/2018 08:58 AM, Daniel Vetter wrote:
>> > > > On Wed, Apr 4, 2018 at 12:42 AM, Rob Clark  wrote:
>> > > > > Add an atomic helper to implement dirtyfb support.  This is needed to
>> > > > > support DSI command-mode panels with x11 userspace (ie. when we can't
>> > > > > rely on pageflips to trigger a flush to the panel).
>> > > > >
>> > > > > To signal to the driver that the async atomic update needs to
>> > > > > synchronize with fences, even though the fb didn't change, the
>> > > > > drm_atomic_state::dirty flag is added.
>> > > > >
>> > > > > Signed-off-by: Rob Clark 
>> > > > > ---
>> > > > > Background: there are a number of different folks working on getting
>> > > > > upstream kernel working on various different phones/tablets with qcom
>> > > > > SoC's.. many of them have command mode panels, so we kind of need a
>> > > > > way to support the legacy dirtyfb ioctl for x11 support.
>> > > > >
>> > > > > I know there is work on a proprer non-legacy atomic property for
>> > > > > userspace to communicate dirty-rect(s) to the kernel, so this can
>> > > > > be improved from triggering a full-frame flush once that is in
>> > > > > place.  But we kinda needa a stop-gap solution.
>> > > > >
>> > > > > I had considered an in-driver solution for this, but things get a
>> > > > > bit tricky if userspace ands up combining dirtyfb ioctls with page-
>> > > > > flips, because we need to synchronize setting various CTL.FLUSH bits
>> > > > > with setting the CTL.START bit.  (ie. really all we need to do for
>> > > > > cmd mode panels is bang CTL.START, but is this ends up racing with
>> > > > > pageflips setting FLUSH bits, then bad things.)  The easiest soln
>> > > > > is to wrap this up as an atomic commit and rely on the worker to
>> > > > > serialize things.  Hence adding an atomic dirtyfb helper.
>> > > > >
>> > > > > I guess at least the helper, with some small addition to translate
>> > > > > and pass-thru the dirty rect(s) is useful to the final atomic dirty-
>> > > > > rect property solution.  Depending on how far off that is, a stop-
>> > > > > gap solution could be useful.
>> > > > Adding Noralf, who iirc already posted the full dirty helpers already 
>> > > > somewhere.
>> > > > -Daniel
>> > > I've asked Deepak to RFC the core changes suggested for the full dirty 
>> > > blob
>> > > on dri-devel. It builds on DisplayLink's suggestion, with a simple 
>> > > helper to
>> > > get to the desired coordinates.
>> > >
>> > > One thing to perhaps discuss is how we would like to fit this with
>> > > front-buffer rendering and the dirty ioctl. In the page-flip context, the
>> > > dirty rects, like egl's swapbuffer_with_damage is a hint to restrict the
>> > > damage region that can be fully ignored by the driver, new content is
>> > > indicated by a new framebuffer.
>> > >
>> > > We could do the same for frontbuffer rendering: Either set a dirty flag 
>> > > like
>> > > you do here, or provide a content_age state member. Since we clear the 
>> > > dirty
>> > > flag on state copies, I guess that would be sufficient. The blob 
>> > > rectangles
>> > > would then become a hint to restrict the damage region.
>> > I'm not entirely following here - I thought for frontbuffer rendering the
>> > dirty rects have always just been a hint, and that the driver was always
>> > free to re-upload the entire buffer to the screen.
>> >
>> > And through a helper like Rob's proposing here (and have floated around in
>> > different versions already) we'd essentially map a frontbuffer dirtyfb
>> > call to a fake flip with dirty rect. Manual upload drivers already need to
>> > upload the entire screen if they get a flip, since some userspace uses
>> > that to flush out frontbuffer rendering (instead of calling dirtyfb).
>> >
>> > So from that pov the new dirty flag is kinda not necessary imo.
>> >
>> > > Another approach would be to have the presence of dirty rects without
>> > > framebuffer change to indicate frontbuffer rendering.
>> > >
>> > > I think I like the first approach best, although it may be tempting for
>> > > user-space apps to just set the dirty bit instead of providing the full
>> > > damage region.
>> > Or I'm not following you here, because I don't quite see the difference
>> > between dirtyfb and a flip.
>> > -Daniel
>>
>> OK, let me rephrase:
>>
>> From the driver's point-of-view, in the atomic world, new content and the
>> need for manual upload is indicated by a change in fb attached to the plane.
>>
>> With Rob's patch here, (correct me if I'm wrong) in addition new content and
>> the need for manual upload is identified by the dirty flag, (since the fb
>> stays the same and the driver 

Re: [Freedreno] [RFC] drm/atomic+msm: add helper to implement legacy dirtyfb

[Rob Clark]

On Wed, Apr 4, 2018 at 6:36 AM, Maarten Lankhorst
 wrote:
> Op 04-04-18 om 12:21 schreef Daniel Vetter:
>> On Wed, Apr 04, 2018 at 12:03:00PM +0200, Daniel Vetter wrote:
>>> On Tue, Apr 03, 2018 at 06:42:23PM -0400, Rob Clark wrote:
 Add an atomic helper to implement dirtyfb support.  This is needed to
 support DSI command-mode panels with x11 userspace (ie. when we can't
 rely on pageflips to trigger a flush to the panel).

 To signal to the driver that the async atomic update needs to
 synchronize with fences, even though the fb didn't change, the
 drm_atomic_state::dirty flag is added.

 Signed-off-by: Rob Clark 
 ---
 Background: there are a number of different folks working on getting
 upstream kernel working on various different phones/tablets with qcom
 SoC's.. many of them have command mode panels, so we kind of need a
 way to support the legacy dirtyfb ioctl for x11 support.

 I know there is work on a proprer non-legacy atomic property for
 userspace to communicate dirty-rect(s) to the kernel, so this can
 be improved from triggering a full-frame flush once that is in
 place.  But we kinda needa a stop-gap solution.

 I had considered an in-driver solution for this, but things get a
 bit tricky if userspace ands up combining dirtyfb ioctls with page-
 flips, because we need to synchronize setting various CTL.FLUSH bits
 with setting the CTL.START bit.  (ie. really all we need to do for
 cmd mode panels is bang CTL.START, but is this ends up racing with
 pageflips setting FLUSH bits, then bad things.)  The easiest soln
 is to wrap this up as an atomic commit and rely on the worker to
 serialize things.  Hence adding an atomic dirtyfb helper.

 I guess at least the helper, with some small addition to translate
 and pass-thru the dirty rect(s) is useful to the final atomic dirty-
 rect property solution.  Depending on how far off that is, a stop-
 gap solution could be useful.

  drivers/gpu/drm/drm_atomic_helper.c | 66 
 +
  drivers/gpu/drm/msm/msm_atomic.c|  5 ++-
  drivers/gpu/drm/msm/msm_fb.c|  1 +
  include/drm/drm_atomic_helper.h |  4 +++
  include/drm/drm_plane.h |  9 +
  5 files changed, 84 insertions(+), 1 deletion(-)

 diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
 b/drivers/gpu/drm/drm_atomic_helper.c
 index c35654591c12..a578dc681b27 100644
 --- a/drivers/gpu/drm/drm_atomic_helper.c
 +++ b/drivers/gpu/drm/drm_atomic_helper.c
 @@ -3504,6 +3504,7 @@ void 
 __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane,
 if (state->fb)
 drm_framebuffer_get(state->fb);

 +   state->dirty = false;
 state->fence = NULL;
 state->commit = NULL;
  }
 @@ -3847,6 +3848,71 @@ int drm_atomic_helper_legacy_gamma_set(struct 
 drm_crtc *crtc,
  }
  EXPORT_SYMBOL(drm_atomic_helper_legacy_gamma_set);

 +/**
 + * drm_atomic_helper_dirtyfb - helper for dirtyfb
 + *
 + * A helper to implement drm_framebuffer_funcs::dirty
 + */
 +int drm_atomic_helper_dirtyfb(struct drm_framebuffer *fb,
 + struct drm_file *file_priv, unsigned flags,
 + unsigned color, struct drm_clip_rect *clips,
 + unsigned num_clips)
 +{
 +   struct drm_modeset_acquire_ctx ctx;
 +   struct drm_atomic_state *state;
 +   struct drm_plane *plane;
 +   int ret = 0;
 +
 +   /*
 +* When called from ioctl, we are interruptable, but not when
 +* called internally (ie. defio worker)
 +*/
 +   drm_modeset_acquire_init(,
 +   file_priv ? DRM_MODESET_ACQUIRE_INTERRUPTIBLE : 0);
 +
 +   state = drm_atomic_state_alloc(fb->dev);
 +   if (!state) {
 +   ret = -ENOMEM;
 +   goto out;
 +   }
 +   state->acquire_ctx = 
 +
 +retry:
 +   drm_for_each_plane(plane, fb->dev) {
 +   struct drm_plane_state *plane_state;
 +
 +   if (plane->state->fb != fb)
 +   continue;
 +
 +   plane_state = drm_atomic_get_plane_state(state, plane);
 +   if (IS_ERR(plane_state)) {
 +   ret = PTR_ERR(plane_state);
 +   goto out;
 +   }
 +
 +   plane_state->dirty = true;
 +   }
 +
 +   ret = drm_atomic_nonblocking_commit(state);
 +
 +out:
 +   if (ret == -EDEADLK) {
 +   drm_atomic_state_clear(state);
 +   ret = drm_modeset_backoff();
 +   if (!ret)
 +   goto retry;
 +   }
 +
 +   drm_atomic_state_put(state);
 +
 +   

Re: [Freedreno] [RFC] drm/atomic+msm: add helper to implement legacy dirtyfb

[Rob Clark]

On Wed, Apr 4, 2018 at 6:21 AM, Daniel Vetter  wrote:
> On Wed, Apr 04, 2018 at 12:03:00PM +0200, Daniel Vetter wrote:
>> On Tue, Apr 03, 2018 at 06:42:23PM -0400, Rob Clark wrote:
>> > Add an atomic helper to implement dirtyfb support.  This is needed to
>> > support DSI command-mode panels with x11 userspace (ie. when we can't
>> > rely on pageflips to trigger a flush to the panel).
>> >
>> > To signal to the driver that the async atomic update needs to
>> > synchronize with fences, even though the fb didn't change, the
>> > drm_atomic_state::dirty flag is added.
>> >
>> > Signed-off-by: Rob Clark 
>> > ---
>> > Background: there are a number of different folks working on getting
>> > upstream kernel working on various different phones/tablets with qcom
>> > SoC's.. many of them have command mode panels, so we kind of need a
>> > way to support the legacy dirtyfb ioctl for x11 support.
>> >
>> > I know there is work on a proprer non-legacy atomic property for
>> > userspace to communicate dirty-rect(s) to the kernel, so this can
>> > be improved from triggering a full-frame flush once that is in
>> > place.  But we kinda needa a stop-gap solution.
>> >
>> > I had considered an in-driver solution for this, but things get a
>> > bit tricky if userspace ands up combining dirtyfb ioctls with page-
>> > flips, because we need to synchronize setting various CTL.FLUSH bits
>> > with setting the CTL.START bit.  (ie. really all we need to do for
>> > cmd mode panels is bang CTL.START, but is this ends up racing with
>> > pageflips setting FLUSH bits, then bad things.)  The easiest soln
>> > is to wrap this up as an atomic commit and rely on the worker to
>> > serialize things.  Hence adding an atomic dirtyfb helper.
>> >
>> > I guess at least the helper, with some small addition to translate
>> > and pass-thru the dirty rect(s) is useful to the final atomic dirty-
>> > rect property solution.  Depending on how far off that is, a stop-
>> > gap solution could be useful.
>> >
>> >  drivers/gpu/drm/drm_atomic_helper.c | 66 
>> > +
>> >  drivers/gpu/drm/msm/msm_atomic.c|  5 ++-
>> >  drivers/gpu/drm/msm/msm_fb.c|  1 +
>> >  include/drm/drm_atomic_helper.h |  4 +++
>> >  include/drm/drm_plane.h |  9 +
>> >  5 files changed, 84 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
>> > b/drivers/gpu/drm/drm_atomic_helper.c
>> > index c35654591c12..a578dc681b27 100644
>> > --- a/drivers/gpu/drm/drm_atomic_helper.c
>> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
>> > @@ -3504,6 +3504,7 @@ void 
>> > __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane,
>> > if (state->fb)
>> > drm_framebuffer_get(state->fb);
>> >
>> > +   state->dirty = false;
>> > state->fence = NULL;
>> > state->commit = NULL;
>> >  }
>> > @@ -3847,6 +3848,71 @@ int drm_atomic_helper_legacy_gamma_set(struct 
>> > drm_crtc *crtc,
>> >  }
>> >  EXPORT_SYMBOL(drm_atomic_helper_legacy_gamma_set);
>> >
>> > +/**
>> > + * drm_atomic_helper_dirtyfb - helper for dirtyfb
>> > + *
>> > + * A helper to implement drm_framebuffer_funcs::dirty
>> > + */
>> > +int drm_atomic_helper_dirtyfb(struct drm_framebuffer *fb,
>> > + struct drm_file *file_priv, unsigned flags,
>> > + unsigned color, struct drm_clip_rect *clips,
>> > + unsigned num_clips)
>> > +{
>> > +   struct drm_modeset_acquire_ctx ctx;
>> > +   struct drm_atomic_state *state;
>> > +   struct drm_plane *plane;
>> > +   int ret = 0;
>> > +
>> > +   /*
>> > +* When called from ioctl, we are interruptable, but not when
>> > +* called internally (ie. defio worker)
>> > +*/
>> > +   drm_modeset_acquire_init(,
>> > +   file_priv ? DRM_MODESET_ACQUIRE_INTERRUPTIBLE : 0);
>> > +
>> > +   state = drm_atomic_state_alloc(fb->dev);
>> > +   if (!state) {
>> > +   ret = -ENOMEM;
>> > +   goto out;
>> > +   }
>> > +   state->acquire_ctx = 
>> > +
>> > +retry:
>> > +   drm_for_each_plane(plane, fb->dev) {
>> > +   struct drm_plane_state *plane_state;
>> > +
>> > +   if (plane->state->fb != fb)
>> > +   continue;
>> > +
>> > +   plane_state = drm_atomic_get_plane_state(state, plane);
>> > +   if (IS_ERR(plane_state)) {
>> > +   ret = PTR_ERR(plane_state);
>> > +   goto out;
>> > +   }
>> > +
>> > +   plane_state->dirty = true;
>> > +   }
>> > +
>> > +   ret = drm_atomic_nonblocking_commit(state);
>> > +
>> > +out:
>> > +   if (ret == -EDEADLK) {
>> > +   drm_atomic_state_clear(state);
>> > +   ret = drm_modeset_backoff();
>> > +   if (!ret)
>> > +   goto retry;
>> > +   }
>> > +
>> > +   drm_atomic_state_put(state);
>> > +
>> > +   drm_modeset_drop_locks();
>> > +   drm_modeset_acquire_fini();
>> > +
>> > +   

Re: [Freedreno] [RFC] drm/atomic+msm: add helper to implement legacy dirtyfb

[Thomas Hellstrom]

On 04/04/2018 10:43 AM, Daniel Vetter wrote:

On Wed, Apr 04, 2018 at 10:22:21AM +0200, Thomas Hellstrom wrote:

Hi,

On 04/04/2018 08:58 AM, Daniel Vetter wrote:

On Wed, Apr 4, 2018 at 12:42 AM, Rob Clark  wrote:

Add an atomic helper to implement dirtyfb support.  This is needed to
support DSI command-mode panels with x11 userspace (ie. when we can't
rely on pageflips to trigger a flush to the panel).

To signal to the driver that the async atomic update needs to
synchronize with fences, even though the fb didn't change, the
drm_atomic_state::dirty flag is added.

Signed-off-by: Rob Clark 
---
Background: there are a number of different folks working on getting
upstream kernel working on various different phones/tablets with qcom
SoC's.. many of them have command mode panels, so we kind of need a
way to support the legacy dirtyfb ioctl for x11 support.

I know there is work on a proprer non-legacy atomic property for
userspace to communicate dirty-rect(s) to the kernel, so this can
be improved from triggering a full-frame flush once that is in
place.  But we kinda needa a stop-gap solution.

I had considered an in-driver solution for this, but things get a
bit tricky if userspace ands up combining dirtyfb ioctls with page-
flips, because we need to synchronize setting various CTL.FLUSH bits
with setting the CTL.START bit.  (ie. really all we need to do for
cmd mode panels is bang CTL.START, but is this ends up racing with
pageflips setting FLUSH bits, then bad things.)  The easiest soln
is to wrap this up as an atomic commit and rely on the worker to
serialize things.  Hence adding an atomic dirtyfb helper.

I guess at least the helper, with some small addition to translate
and pass-thru the dirty rect(s) is useful to the final atomic dirty-
rect property solution.  Depending on how far off that is, a stop-
gap solution could be useful.

Adding Noralf, who iirc already posted the full dirty helpers already somewhere.
-Daniel

I've asked Deepak to RFC the core changes suggested for the full dirty blob
on dri-devel. It builds on DisplayLink's suggestion, with a simple helper to
get to the desired coordinates.

One thing to perhaps discuss is how we would like to fit this with
front-buffer rendering and the dirty ioctl. In the page-flip context, the
dirty rects, like egl's swapbuffer_with_damage is a hint to restrict the
damage region that can be fully ignored by the driver, new content is
indicated by a new framebuffer.

We could do the same for frontbuffer rendering: Either set a dirty flag like
you do here, or provide a content_age state member. Since we clear the dirty
flag on state copies, I guess that would be sufficient. The blob rectangles
would then become a hint to restrict the damage region.

I'm not entirely following here - I thought for frontbuffer rendering the
dirty rects have always just been a hint, and that the driver was always
free to re-upload the entire buffer to the screen.

And through a helper like Rob's proposing here (and have floated around in
different versions already) we'd essentially map a frontbuffer dirtyfb
call to a fake flip with dirty rect. Manual upload drivers already need to
upload the entire screen if they get a flip, since some userspace uses
that to flush out frontbuffer rendering (instead of calling dirtyfb).

So from that pov the new dirty flag is kinda not necessary imo.


Another approach would be to have the presence of dirty rects without
framebuffer change to indicate frontbuffer rendering.

I think I like the first approach best, although it may be tempting for
user-space apps to just set the dirty bit instead of providing the full
damage region.

Or I'm not following you here, because I don't quite see the difference
between dirtyfb and a flip.
-Daniel


OK, let me rephrase:

From the driver's point-of-view, in the atomic world, new content and 
the need for manual upload is indicated by a change in fb attached to 
the plane.


With Rob's patch here, (correct me if I'm wrong) in addition new content 
and the need for manual upload is identified by the dirty flag, (since 
the fb stays the same and the driver thus never identifies a page-flip).


In both these situations, clip rects can provide a hint to restrict the 
dirty region.


Now I was thinking about the preferred way for user-space to communicate 
front buffer rendering through the atomic ioctl:


1) Expose a dirty (or content_age property)
2) Attach a clip-rect blob property.
3) Fake a page-flip by ping-ponging two frame-buffers pointing to the 
same underlying buffer object.


Are you saying that people are already using 3) and we should keep using 
that?


/Thomas




___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [RFC] drm/atomic+msm: add helper to implement legacy dirtyfb

[Thomas Hellstrom]

On 04/04/2018 11:56 AM, Daniel Vetter wrote:

On Wed, Apr 04, 2018 at 11:10:08AM +0200, Thomas Hellstrom wrote:

On 04/04/2018 10:43 AM, Daniel Vetter wrote:

On Wed, Apr 04, 2018 at 10:22:21AM +0200, Thomas Hellstrom wrote:

Hi,

On 04/04/2018 08:58 AM, Daniel Vetter wrote:

On Wed, Apr 4, 2018 at 12:42 AM, Rob Clark  wrote:

Add an atomic helper to implement dirtyfb support.  This is needed to
support DSI command-mode panels with x11 userspace (ie. when we can't
rely on pageflips to trigger a flush to the panel).

To signal to the driver that the async atomic update needs to
synchronize with fences, even though the fb didn't change, the
drm_atomic_state::dirty flag is added.

Signed-off-by: Rob Clark 
---
Background: there are a number of different folks working on getting
upstream kernel working on various different phones/tablets with qcom
SoC's.. many of them have command mode panels, so we kind of need a
way to support the legacy dirtyfb ioctl for x11 support.

I know there is work on a proprer non-legacy atomic property for
userspace to communicate dirty-rect(s) to the kernel, so this can
be improved from triggering a full-frame flush once that is in
place.  But we kinda needa a stop-gap solution.

I had considered an in-driver solution for this, but things get a
bit tricky if userspace ands up combining dirtyfb ioctls with page-
flips, because we need to synchronize setting various CTL.FLUSH bits
with setting the CTL.START bit.  (ie. really all we need to do for
cmd mode panels is bang CTL.START, but is this ends up racing with
pageflips setting FLUSH bits, then bad things.)  The easiest soln
is to wrap this up as an atomic commit and rely on the worker to
serialize things.  Hence adding an atomic dirtyfb helper.

I guess at least the helper, with some small addition to translate
and pass-thru the dirty rect(s) is useful to the final atomic dirty-
rect property solution.  Depending on how far off that is, a stop-
gap solution could be useful.

Adding Noralf, who iirc already posted the full dirty helpers already somewhere.
-Daniel

I've asked Deepak to RFC the core changes suggested for the full dirty blob
on dri-devel. It builds on DisplayLink's suggestion, with a simple helper to
get to the desired coordinates.

One thing to perhaps discuss is how we would like to fit this with
front-buffer rendering and the dirty ioctl. In the page-flip context, the
dirty rects, like egl's swapbuffer_with_damage is a hint to restrict the
damage region that can be fully ignored by the driver, new content is
indicated by a new framebuffer.

We could do the same for frontbuffer rendering: Either set a dirty flag like
you do here, or provide a content_age state member. Since we clear the dirty
flag on state copies, I guess that would be sufficient. The blob rectangles
would then become a hint to restrict the damage region.

I'm not entirely following here - I thought for frontbuffer rendering the
dirty rects have always just been a hint, and that the driver was always
free to re-upload the entire buffer to the screen.

And through a helper like Rob's proposing here (and have floated around in
different versions already) we'd essentially map a frontbuffer dirtyfb
call to a fake flip with dirty rect. Manual upload drivers already need to
upload the entire screen if they get a flip, since some userspace uses
that to flush out frontbuffer rendering (instead of calling dirtyfb).

So from that pov the new dirty flag is kinda not necessary imo.


Another approach would be to have the presence of dirty rects without
framebuffer change to indicate frontbuffer rendering.

I think I like the first approach best, although it may be tempting for
user-space apps to just set the dirty bit instead of providing the full
damage region.

Or I'm not following you here, because I don't quite see the difference
between dirtyfb and a flip.
-Daniel

OK, let me rephrase:

 From the driver's point-of-view, in the atomic world, new content and the
need for manual upload is indicated by a change in fb attached to the plane.

With Rob's patch here, (correct me if I'm wrong) in addition new content and
the need for manual upload is identified by the dirty flag, (since the fb
stays the same and the driver thus never identifies a page-flip).

Hm, I'm not entirely sure Rob's approach is correct. Imo a pageflip
(atomic or not) should result in the entire buffer getting uploaded. The
dirty flag is kinda redundant, a flip with the same buffer works the same
way as a dirtyfb with the entire buffer as the dirty rectangle.


In both these situations, clip rects can provide a hint to restrict the
dirty region.

Now I was thinking about the preferred way for user-space to communicate
front buffer rendering through the atomic ioctl:

1) Expose a dirty (or content_age property)
2) Attach a clip-rect blob property.
3) Fake a page-flip by ping-ponging two frame-buffers pointing to the same
underlying buffer object.

Re: [Freedreno] [RFC] drm/atomic+msm: add helper to implement legacy dirtyfb

[Maarten Lankhorst]

Op 04-04-18 om 12:21 schreef Daniel Vetter:
> On Wed, Apr 04, 2018 at 12:03:00PM +0200, Daniel Vetter wrote:
>> On Tue, Apr 03, 2018 at 06:42:23PM -0400, Rob Clark wrote:
>>> Add an atomic helper to implement dirtyfb support.  This is needed to
>>> support DSI command-mode panels with x11 userspace (ie. when we can't
>>> rely on pageflips to trigger a flush to the panel).
>>>
>>> To signal to the driver that the async atomic update needs to
>>> synchronize with fences, even though the fb didn't change, the
>>> drm_atomic_state::dirty flag is added.
>>>
>>> Signed-off-by: Rob Clark 
>>> ---
>>> Background: there are a number of different folks working on getting
>>> upstream kernel working on various different phones/tablets with qcom
>>> SoC's.. many of them have command mode panels, so we kind of need a
>>> way to support the legacy dirtyfb ioctl for x11 support.
>>>
>>> I know there is work on a proprer non-legacy atomic property for
>>> userspace to communicate dirty-rect(s) to the kernel, so this can
>>> be improved from triggering a full-frame flush once that is in
>>> place.  But we kinda needa a stop-gap solution.
>>>
>>> I had considered an in-driver solution for this, but things get a
>>> bit tricky if userspace ands up combining dirtyfb ioctls with page-
>>> flips, because we need to synchronize setting various CTL.FLUSH bits
>>> with setting the CTL.START bit.  (ie. really all we need to do for
>>> cmd mode panels is bang CTL.START, but is this ends up racing with
>>> pageflips setting FLUSH bits, then bad things.)  The easiest soln
>>> is to wrap this up as an atomic commit and rely on the worker to
>>> serialize things.  Hence adding an atomic dirtyfb helper.
>>>
>>> I guess at least the helper, with some small addition to translate
>>> and pass-thru the dirty rect(s) is useful to the final atomic dirty-
>>> rect property solution.  Depending on how far off that is, a stop-
>>> gap solution could be useful.
>>>
>>>  drivers/gpu/drm/drm_atomic_helper.c | 66 
>>> +
>>>  drivers/gpu/drm/msm/msm_atomic.c|  5 ++-
>>>  drivers/gpu/drm/msm/msm_fb.c|  1 +
>>>  include/drm/drm_atomic_helper.h |  4 +++
>>>  include/drm/drm_plane.h |  9 +
>>>  5 files changed, 84 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
>>> b/drivers/gpu/drm/drm_atomic_helper.c
>>> index c35654591c12..a578dc681b27 100644
>>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>>> @@ -3504,6 +3504,7 @@ void __drm_atomic_helper_plane_duplicate_state(struct 
>>> drm_plane *plane,
>>> if (state->fb)
>>> drm_framebuffer_get(state->fb);
>>>  
>>> +   state->dirty = false;
>>> state->fence = NULL;
>>> state->commit = NULL;
>>>  }
>>> @@ -3847,6 +3848,71 @@ int drm_atomic_helper_legacy_gamma_set(struct 
>>> drm_crtc *crtc,
>>>  }
>>>  EXPORT_SYMBOL(drm_atomic_helper_legacy_gamma_set);
>>>  
>>> +/**
>>> + * drm_atomic_helper_dirtyfb - helper for dirtyfb
>>> + *
>>> + * A helper to implement drm_framebuffer_funcs::dirty
>>> + */
>>> +int drm_atomic_helper_dirtyfb(struct drm_framebuffer *fb,
>>> + struct drm_file *file_priv, unsigned flags,
>>> + unsigned color, struct drm_clip_rect *clips,
>>> + unsigned num_clips)
>>> +{
>>> +   struct drm_modeset_acquire_ctx ctx;
>>> +   struct drm_atomic_state *state;
>>> +   struct drm_plane *plane;
>>> +   int ret = 0;
>>> +
>>> +   /*
>>> +* When called from ioctl, we are interruptable, but not when
>>> +* called internally (ie. defio worker)
>>> +*/
>>> +   drm_modeset_acquire_init(,
>>> +   file_priv ? DRM_MODESET_ACQUIRE_INTERRUPTIBLE : 0);
>>> +
>>> +   state = drm_atomic_state_alloc(fb->dev);
>>> +   if (!state) {
>>> +   ret = -ENOMEM;
>>> +   goto out;
>>> +   }
>>> +   state->acquire_ctx = 
>>> +
>>> +retry:
>>> +   drm_for_each_plane(plane, fb->dev) {
>>> +   struct drm_plane_state *plane_state;
>>> +
>>> +   if (plane->state->fb != fb)
>>> +   continue;
>>> +
>>> +   plane_state = drm_atomic_get_plane_state(state, plane);
>>> +   if (IS_ERR(plane_state)) {
>>> +   ret = PTR_ERR(plane_state);
>>> +   goto out;
>>> +   }
>>> +
>>> +   plane_state->dirty = true;
>>> +   }
>>> +
>>> +   ret = drm_atomic_nonblocking_commit(state);
>>> +
>>> +out:
>>> +   if (ret == -EDEADLK) {
>>> +   drm_atomic_state_clear(state);
>>> +   ret = drm_modeset_backoff();
>>> +   if (!ret)
>>> +   goto retry;
>>> +   }
>>> +
>>> +   drm_atomic_state_put(state);
>>> +
>>> +   drm_modeset_drop_locks();
>>> +   drm_modeset_acquire_fini();
>>> +
>>> +   return ret;
>>> +
>>> +}
>>> +EXPORT_SYMBOL(drm_atomic_helper_dirtyfb);
>>> +
>>>  /**
>>>   * __drm_atomic_helper_private_duplicate_state - 

Re: [Freedreno] [RFC] drm/atomic+msm: add helper to implement legacy dirtyfb

[Daniel Vetter]

On Tue, Apr 03, 2018 at 06:42:23PM -0400, Rob Clark wrote:
> Add an atomic helper to implement dirtyfb support.  This is needed to
> support DSI command-mode panels with x11 userspace (ie. when we can't
> rely on pageflips to trigger a flush to the panel).
> 
> To signal to the driver that the async atomic update needs to
> synchronize with fences, even though the fb didn't change, the
> drm_atomic_state::dirty flag is added.
> 
> Signed-off-by: Rob Clark 
> ---
> Background: there are a number of different folks working on getting
> upstream kernel working on various different phones/tablets with qcom
> SoC's.. many of them have command mode panels, so we kind of need a
> way to support the legacy dirtyfb ioctl for x11 support.
> 
> I know there is work on a proprer non-legacy atomic property for
> userspace to communicate dirty-rect(s) to the kernel, so this can
> be improved from triggering a full-frame flush once that is in
> place.  But we kinda needa a stop-gap solution.
> 
> I had considered an in-driver solution for this, but things get a
> bit tricky if userspace ands up combining dirtyfb ioctls with page-
> flips, because we need to synchronize setting various CTL.FLUSH bits
> with setting the CTL.START bit.  (ie. really all we need to do for
> cmd mode panels is bang CTL.START, but is this ends up racing with
> pageflips setting FLUSH bits, then bad things.)  The easiest soln
> is to wrap this up as an atomic commit and rely on the worker to
> serialize things.  Hence adding an atomic dirtyfb helper.
> 
> I guess at least the helper, with some small addition to translate
> and pass-thru the dirty rect(s) is useful to the final atomic dirty-
> rect property solution.  Depending on how far off that is, a stop-
> gap solution could be useful.
> 
>  drivers/gpu/drm/drm_atomic_helper.c | 66 
> +
>  drivers/gpu/drm/msm/msm_atomic.c|  5 ++-
>  drivers/gpu/drm/msm/msm_fb.c|  1 +
>  include/drm/drm_atomic_helper.h |  4 +++
>  include/drm/drm_plane.h |  9 +
>  5 files changed, 84 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
> b/drivers/gpu/drm/drm_atomic_helper.c
> index c35654591c12..a578dc681b27 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -3504,6 +3504,7 @@ void __drm_atomic_helper_plane_duplicate_state(struct 
> drm_plane *plane,
>   if (state->fb)
>   drm_framebuffer_get(state->fb);
>  
> + state->dirty = false;
>   state->fence = NULL;
>   state->commit = NULL;
>  }
> @@ -3847,6 +3848,71 @@ int drm_atomic_helper_legacy_gamma_set(struct drm_crtc 
> *crtc,
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_legacy_gamma_set);
>  
> +/**
> + * drm_atomic_helper_dirtyfb - helper for dirtyfb
> + *
> + * A helper to implement drm_framebuffer_funcs::dirty
> + */
> +int drm_atomic_helper_dirtyfb(struct drm_framebuffer *fb,
> +   struct drm_file *file_priv, unsigned flags,
> +   unsigned color, struct drm_clip_rect *clips,
> +   unsigned num_clips)
> +{
> + struct drm_modeset_acquire_ctx ctx;
> + struct drm_atomic_state *state;
> + struct drm_plane *plane;
> + int ret = 0;
> +
> + /*
> +  * When called from ioctl, we are interruptable, but not when
> +  * called internally (ie. defio worker)
> +  */
> + drm_modeset_acquire_init(,
> + file_priv ? DRM_MODESET_ACQUIRE_INTERRUPTIBLE : 0);
> +
> + state = drm_atomic_state_alloc(fb->dev);
> + if (!state) {
> + ret = -ENOMEM;
> + goto out;
> + }
> + state->acquire_ctx = 
> +
> +retry:
> + drm_for_each_plane(plane, fb->dev) {
> + struct drm_plane_state *plane_state;
> +
> + if (plane->state->fb != fb)
> + continue;
> +
> + plane_state = drm_atomic_get_plane_state(state, plane);
> + if (IS_ERR(plane_state)) {
> + ret = PTR_ERR(plane_state);
> + goto out;
> + }
> +
> + plane_state->dirty = true;
> + }
> +
> + ret = drm_atomic_nonblocking_commit(state);
> +
> +out:
> + if (ret == -EDEADLK) {
> + drm_atomic_state_clear(state);
> + ret = drm_modeset_backoff();
> + if (!ret)
> + goto retry;
> + }
> +
> + drm_atomic_state_put(state);
> +
> + drm_modeset_drop_locks();
> + drm_modeset_acquire_fini();
> +
> + return ret;
> +
> +}
> +EXPORT_SYMBOL(drm_atomic_helper_dirtyfb);
> +
>  /**
>   * __drm_atomic_helper_private_duplicate_state - copy atomic private state
>   * @obj: CRTC object
> diff --git a/drivers/gpu/drm/msm/msm_atomic.c 
> b/drivers/gpu/drm/msm/msm_atomic.c
> index bf5f8c39f34d..bb55a048e98b 100644
> --- a/drivers/gpu/drm/msm/msm_atomic.c
> +++ b/drivers/gpu/drm/msm/msm_atomic.c
> @@ -201,7 

Re: [Freedreno] [RFC] drm/atomic+msm: add helper to implement legacy dirtyfb

[Daniel Vetter]

On Wed, Apr 04, 2018 at 11:10:08AM +0200, Thomas Hellstrom wrote:
> On 04/04/2018 10:43 AM, Daniel Vetter wrote:
> > On Wed, Apr 04, 2018 at 10:22:21AM +0200, Thomas Hellstrom wrote:
> > > Hi,
> > > 
> > > On 04/04/2018 08:58 AM, Daniel Vetter wrote:
> > > > On Wed, Apr 4, 2018 at 12:42 AM, Rob Clark  wrote:
> > > > > Add an atomic helper to implement dirtyfb support.  This is needed to
> > > > > support DSI command-mode panels with x11 userspace (ie. when we can't
> > > > > rely on pageflips to trigger a flush to the panel).
> > > > > 
> > > > > To signal to the driver that the async atomic update needs to
> > > > > synchronize with fences, even though the fb didn't change, the
> > > > > drm_atomic_state::dirty flag is added.
> > > > > 
> > > > > Signed-off-by: Rob Clark 
> > > > > ---
> > > > > Background: there are a number of different folks working on getting
> > > > > upstream kernel working on various different phones/tablets with qcom
> > > > > SoC's.. many of them have command mode panels, so we kind of need a
> > > > > way to support the legacy dirtyfb ioctl for x11 support.
> > > > > 
> > > > > I know there is work on a proprer non-legacy atomic property for
> > > > > userspace to communicate dirty-rect(s) to the kernel, so this can
> > > > > be improved from triggering a full-frame flush once that is in
> > > > > place.  But we kinda needa a stop-gap solution.
> > > > > 
> > > > > I had considered an in-driver solution for this, but things get a
> > > > > bit tricky if userspace ands up combining dirtyfb ioctls with page-
> > > > > flips, because we need to synchronize setting various CTL.FLUSH bits
> > > > > with setting the CTL.START bit.  (ie. really all we need to do for
> > > > > cmd mode panels is bang CTL.START, but is this ends up racing with
> > > > > pageflips setting FLUSH bits, then bad things.)  The easiest soln
> > > > > is to wrap this up as an atomic commit and rely on the worker to
> > > > > serialize things.  Hence adding an atomic dirtyfb helper.
> > > > > 
> > > > > I guess at least the helper, with some small addition to translate
> > > > > and pass-thru the dirty rect(s) is useful to the final atomic dirty-
> > > > > rect property solution.  Depending on how far off that is, a stop-
> > > > > gap solution could be useful.
> > > > Adding Noralf, who iirc already posted the full dirty helpers already 
> > > > somewhere.
> > > > -Daniel
> > > I've asked Deepak to RFC the core changes suggested for the full dirty 
> > > blob
> > > on dri-devel. It builds on DisplayLink's suggestion, with a simple helper 
> > > to
> > > get to the desired coordinates.
> > > 
> > > One thing to perhaps discuss is how we would like to fit this with
> > > front-buffer rendering and the dirty ioctl. In the page-flip context, the
> > > dirty rects, like egl's swapbuffer_with_damage is a hint to restrict the
> > > damage region that can be fully ignored by the driver, new content is
> > > indicated by a new framebuffer.
> > > 
> > > We could do the same for frontbuffer rendering: Either set a dirty flag 
> > > like
> > > you do here, or provide a content_age state member. Since we clear the 
> > > dirty
> > > flag on state copies, I guess that would be sufficient. The blob 
> > > rectangles
> > > would then become a hint to restrict the damage region.
> > I'm not entirely following here - I thought for frontbuffer rendering the
> > dirty rects have always just been a hint, and that the driver was always
> > free to re-upload the entire buffer to the screen.
> > 
> > And through a helper like Rob's proposing here (and have floated around in
> > different versions already) we'd essentially map a frontbuffer dirtyfb
> > call to a fake flip with dirty rect. Manual upload drivers already need to
> > upload the entire screen if they get a flip, since some userspace uses
> > that to flush out frontbuffer rendering (instead of calling dirtyfb).
> > 
> > So from that pov the new dirty flag is kinda not necessary imo.
> > 
> > > Another approach would be to have the presence of dirty rects without
> > > framebuffer change to indicate frontbuffer rendering.
> > > 
> > > I think I like the first approach best, although it may be tempting for
> > > user-space apps to just set the dirty bit instead of providing the full
> > > damage region.
> > Or I'm not following you here, because I don't quite see the difference
> > between dirtyfb and a flip.
> > -Daniel
> 
> OK, let me rephrase:
> 
> From the driver's point-of-view, in the atomic world, new content and the
> need for manual upload is indicated by a change in fb attached to the plane.
> 
> With Rob's patch here, (correct me if I'm wrong) in addition new content and
> the need for manual upload is identified by the dirty flag, (since the fb
> stays the same and the driver thus never identifies a page-flip).

Hm, I'm not entirely sure Rob's approach is correct. Imo a pageflip
(atomic or not) should result in the entire