[PATCH RFC 00/11] drm/tilcdc: Atomic modeset support

[Daniel Vetter]

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