Re: [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 <noralf@xxxxxxxxxxx> 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 <robdclark@xxxxxxxxx>
> > > ---
> > > 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.