On Mon, May 09, 2016 at 05:10:00PM +0300, Tomi Valkeinen wrote: > Hi Jyri, > > On 11/04/16 19:46, Jyri Sarha wrote: > > The LCDC in its simplicity does not fit too well into DRM atomic > > modeset abstractions. I wonder if I am doing the right thing in > > implementing the dummy primary plane and in implementing > > mode_set_nofb() crtc helper when the crtc actually needs the > > framebuffer to be there when configuring it. See individual patch > > descriptions for details. There is still lot of room for cleaning up > > but I would first like to know if I am moving at all to the right > > direction. > > I'm no expert on atomic modesetting, but here are my comments/questions: > > You add drm_crtc_vblank_off() to crtc_destroy() callback. Why is that? I > think you should call drm_crtc_vblank_on() in crtc_enable(), and > vblank_off in disable. > > You remove the setting of tilcdc_crtc_helper_funcs.dpms, but leave the > tilcdc_crtc_dpms, which you use elsewhere. I think all dpms stuff should > be removed from crtc, as it's all either "enable" or "disable". Git grep > shows that in omapdrm, there's just one reference to dpms, in > omap_connector.c: .dpms = drm_atomic_helper_connector_dpms > > It's not clear to me if a (primary) plane is required with atomic > universal planes and modesetting or not... I guess it is, when thinking > of how e.g. a framebuffer is configured to be shown on a screen when > using the atomic modesetting uapi. You need a primary plane, but atomic doesn't require that it's enabled. Which this simple display controller probably wont like, so seems like this implementation of a primary plane is a bit too simple. You also need a real plane for the cursor, if you want to support that with atomic. > But if it is required, it makes me wonder, are there other HWs out there > without any planes? The dummy plane implementation you added is not > complex, but is it something that should be implemented with DRM helper > funcs? There's a drm_simple_display_pipe floating around which seems perfectly suited to tilcdc. It's meant for the case where you have 1 plane, 1 crtc and 1 encoder maybe linking to different connectors. And it takes care of all the small bits for you, with a grand total of 5 callbacks, all of them optional. Might indeed be useful to rebase tilcdc on top of that, should be possible to nuke piles of code. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
