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

[Archit Taneja]

Hi,

On Wednesday 01 August 2012 07:55 PM, Rob Clark wrote:
> 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.

I guess the work Tomi is talking about is already merged in 3.6. It 
ensures that interface drivers(DSI/HDMI) don't do direct DISPC register 
writes on overlay managers. For example, when HDMI's timings are 
changed, the TV manager's DISPC_SIZE_DIGIT needs to be configured, and 
it's a shadow register. There was no guarantee previously that when the 
HDMI driver writes to this register the GO bit of the TV manager is clear.

The stuff I posted today is a part of a bigger series, it's final aim is 
to have an entity in omapdss which is an equivalent of drm_encoder in 
your new drm arrangement, i.e, an entity which represents an interface. 
We call it outputs, a manager would now connect to an output instead of 
a panel, and the output would now connect to the panel. So the 
connection will be like:

ovl->manager->output->device

The whole set is in the tree below, I'm posting the set out in smaller 
parts.

git://gitorious.org/~boddob/linux-omap-dss2/archit-dss2-clone.git 
out_work_23_july

Archit

>
>> 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
>>
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>