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

[Rob Clark]

On Wed, Aug 1, 2012 at 11:53 AM, Rob Clark <rob.clark at linaro.org> wrote:
> On Wed, Aug 1, 2012 at 11:46 AM, Archit Taneja <archit at ti.com> wrote:
>> 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.
>
> ahh, ok.. actually I've commented out (I think) all of the mgr
> register updates from the HDMI driver for my prototype.  These already
> get set properly from the kms crtc (going through GO bit / apply
> mechanism to synchronize w/ GO bit), so I don't even need the
> interface/panel driver to set this stuff up.
>
>> 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:
>
> Ok.. this would help.  I'll take a look.  I do request that
> interfaces/panels don't set any mgr/timing related registers.  I had

sorry, that should say "any mgr/ovl related registers".. basically not
having any under-the-hood connections between the entities at the
omapdss level would be ideal

BR,
-R

> to comment all this stuff out in my prototype.  Really we want to set
> the timings separately on the crtc (mgr) / encoder (interface) /
> connector (panel.. not sure if it is needed, though?).  KMS will take
> care of propagating the timings through the pipeline.
>
> BR,
> -R
>
>> 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
>>>
>>
>> --
>> 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