On Wed, Aug 1, 2012 at 11:53 AM, Rob Clark <rob.clark@xxxxxxxxxx> wrote: > On Wed, Aug 1, 2012 at 11:46 AM, Archit Taneja <archit@xxxxxx> 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@xxxxxx> >>> 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@xxxxxx> >>>>> 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@xxxxxxxxxxxxxxx >>> 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@xxxxxxxxxxxxxxx >> 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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
