[RFC 0/3] solving omapdrm/omapdss layering issues

[Rob Clark]

On Wed, Aug 1, 2012 at 4:21 AM, Tomi Valkeinen <tomi.valkeinen at ti.com> wrote:
> On Tue, 2012-07-31 at 09:45 -0500, Rob Clark wrote:
>> On Tue, Jul 31, 2012 at 8:40 AM, Tomi Valkeinen <tomi.valkeinen at ti.com> wrote:
>
>> > It's not really about being friendly. Omapdss tries to do as little as
>> > possible, while still supporting all its HW features. Shadow registers
>> > are a bit tricky creating this mess.
>>
>> What I mean by 'friendly' is it tries to abstract this for simple
>> users, like an fbdev driver.  But this really quickly breaks down w/ a
>
> No, that's not what omapdss tries to do. I'm not trying to hide the
> shadow registers and the GO bit behind the omapdss API, I'm just trying
> to make it work.
>
> The omapdss API was made with omapfb, so it's true that the API may not
> be good for omapdrm. But I'm happy to change the API.
>
>> And btw, I think the current mapping of drm_encoder to mgr in omapdrm
>> is not correct.  I'm just in the process of shuffling things around.
>> I think drm/kms actually maps quite nicely to the underlying hardware
>> with the following arrangement:
>>
>>  drm_plane -> ovl
>>  drm_crtc -> mgr
>>  drm_encoder -> DSI/DPI/HDMI/VENC encoder
>>  drm_connector -> pretty much what we call a panel driver today
>
> Hmm, what was the arrangement earlier?

it was previously:

  plane -> ovl
  crtc -> placeholder
  encoder -> mgr
  connector -> dssdev (encoder+panel)

although crtc is really the point where you should enable/disable
vblank irqs, so the new arrangement is somewhat cleaner (although on
my branch the encoder/connector part are not finished yet)

> I guess the fact is that DRM concepts do not really match the OMAP DSS
> hardware, and we'll have to use whatever gives us least problems.

Actually, I think it does map fairly well to the hardware.. at least
more so than to omapdss ;-)

The one area that kms mismatches a bit is decoupling of ovl from mgr
that we have in our hw..  I've partially solved that a while back w/
the patch in drm to add "private planes" so the omap_crtc internally
uses an omap_plane.  It isn't exposed to userspace to be able to
re-use the planes from unused crtcs, although I have some ideas about
that (but not yet time to work on it).

>> It would be quite useful if you could look at the omap_drm_apply
>> mechanism I had in omapdrm, because that seems like a quite
>> straightforward way to deal w/ shadowed registers.  I think it will
>
> Yes, it seems straightforward, but it's not =).
>
> I had a look at your omapdrm-on-dispc-2 branch. What you are doing there
> is quite similar to what omapdss was doing earlier. It's not going to
> work reliably with multiple outputs and fifomerge.
>
> Configuring things like overlay color mode are quite simple. They only
> affect that one overlay. Also things like manager default bg color are
> simple, they affect only that one manager.
>
> But enabling/disabling an overlay or a manager, changing the destination
> mgr of an overlay, fifomerge... Those are not simple. You can't do them
> directly, as you do in your branch.
>
> As an example, consider the case of enabling an overlay (vid1), and
> moving fifo buffers from currently enabled overlay (gfx) to vid1: you'll
> first need to take the fifo buffers from gfx, set GO, and wait for the
> settings to take effect. Only then you can set the fifo buffers for
> vid1, enable it and set GO bit.

hmm, it does sound like it needs a bit of a state machine to deal with
multi-step updates.. although that makes races more of a problem,
which was something I was trying hard to avoid.

For enabling/disabling an output (manager+encoder), this is relatively
infrequent, so it can afford to block to avoid races.  (Like userspace
enabling and then rapidly disabling an output part way through the
enable.)  But enabling/disabling an overlay, or adjusting position or
scanout address must not block.  And ideally, if possible, switching
an overlay between two managers should not block.

For fifomerge, if I understand correctly, it shouldn't really be
needed for functionality, but mainly as a power optimization?  If this
is the case I wonder about an approach of disabling fifomerge when
there are ongoing setting changes, and then setting it after things
settle down?  I'll have to think about it, but I was trying to avoid
needing a multi-step state machine to avoid the associated race
conditions, but if this is not possible then it is not possible.

> I didn't write omapdss's apply.c for fun or to make omapfb simpler. I
> made it because the shadow register system is complex, and we need to
> handle the tricky cases somewhere.
>
> So, as I said before, I believe you'll just end up writing similar code
> to what is currently in apply.c. It won't be as simple as your current
> branch.
>
> Also, as I mentioned earlier, you'll also need to handle the output side
> of the shadow registers. These come from the output drivers (DPI, DSI,
> etc, and indirectly from panel drivers). They are not currently handled
> in the best manner in omapdss, but Archit is working on that and in his
> version apply.c will handle also those properly.

The encoder/connector part of things is something that I have not
tackled yet.. but I expect if there is something that can handle
fifomerge, etc, then it should also be usable from the encoder code.

I need to have a closer look at the patches from Archit (I assume you
are talking about the series he sent earlier today) and see if that
makes things easier for me to map properly to kms encoder/connector.

> About your code, I see you have these pre and post apply callbacks that
> handle the configuration. Wouldn't it be rather easy to have omapdss's
> apply.c call these?

Possibly.. really what I am working on now is a proof of concept.  But
I think that once it works properly, if there is a way to shuffle
things around to get more re-use from omapfb/etc, then that would be a
good idea.  I'm not opposed to that.  But we at least need to figure
out how to get it working properly for drm/kms's needs.

> And then one thing I don't think you've considered is manual update
> displays. Of course, one option is to not support those with omapdrm,
> but that's quite a big decision. omapdss's apply.c handles those also.

well, mainly because it is only proof of concept so far, and I don't
actually have any hardware w/ a manual update display.  But I think
manual update needs some more work at a few layers.  We need userspace
xorg driver to call DRM_IOCTL_MODE_DIRTYFB at the appropriate times
(in case it is doing front buffer rendering), then on kernel side we
need to defer until gpu access has finished (similar to how a
page_flip is handled).  After that, if I understand properly, we can
use the same apply mechanism to kick the encoder to push the update
out to the display.

> Also, can you check again my mail "Re: OMAPDSS vsyncs/apply" Sat, 12 May
> 2012 10:01:24, about the request_config() suggestion. I think that would
> be somewhat similar to your pre/post callbacks. I'll try to write some
> prototype for the request_config suggestion so that it's easier to
> understand.

I'll look again, but as far as I remember that at least wasn't
addressing the performance issues from making overlay enable/disable
synchronous.  And fixing that would, I expect, trigger the same
problems that I already spent a few days debugging before switching
over to handle apply in omapdrm.

BR,
-R

>  Tomi
>
>