Re: [PATCH 1/6] drm: Add writeback connector type

[Brian Starkey]

On Wed, Apr 19, 2017 at 01:34:34PM +0200, Boris Brezillon wrote:
> On Wed, 19 Apr 2017 10:51:23 +0100
> Brian Starkey <brian.starkey@xxxxxxx> wrote:

[snip]

> > Could you expand a bit on how you think planes fit better?
>
> Just had the impression that the writeback feature was conceptually
> closer to a plane object (which is attached buffers and expose ways to
> specify which portion of the buffer should be used, provides way to
> atomically switch 2 buffers, ...).

Yeah sort-of, except that SRC_X/Y/W/H doesn't mean the same for an
"output" plane as an "input" plane (and CRTC_X/Y/W/H similarly,
probably other properties too).

In atomic land, the swapping of buffers is really just the swapping of
object IDs via properties - I don't think planes actually have
anything special in terms of buffer handling, except for all the
legacy state handling cruft.

> > It is
> > something we've previously talked about internally, but so far I'm not
> > convinced :-)
>
> Okay, as I said, I don't know all the history, hence my questions ;-).

I think that history was here in our office rather than on the list
anyway.

> > >By doing that, we would also get rid of these fake connector/encoder
> > >objects as well as the fake modes we are returning in
> > >connector->get_modes().
> >
> > What makes the connector/encoder fake? They represent a real piece of
> > hardware just the same as a drm_plane would.
>
> Well, that's probably subject to interpretation, but I don't consider
> these writeback encoders/connectors as real encoders/connectors. They
> are definitely real HW blocks, but not what we usually consider as an
> encoder/connector.

This is true

> > I don't mind dropping the mode list and letting userspace just make
> > up whatever timing it wants - it'd need to do the same if writeback
> > was a plane - but in some respects giving it a list of modes the same
> > way we normally do seems nicer for userspace.
> >
> > >
> > >As far as I can tell, the VC4 and Atmel HLCDC IP should fit pretty well
> > >in this model, not sure about the mali-dp though.
> > >
> > >Brian, did you consider this approach, and if you did, can you detail
> > >why you decided to expose this feature as a connector?
> > >
> > >Daniel (or anyone else), please step in if you think this is a stupid
> > >idea :-).
> >
> > Ville originally suggested using a connector, which Eric followed up
> > by saying that's what he was thinking of for VC4 writeback too[1].
>
> Thanks for the pointer.
>
> > That was my initial reason for focussing on a connector-based
> > approach.
> >
> > I prefer connector over plane conceptually because it keeps with the
> > established data flow: planes are sources, connectors are sinks.
>
> Okay, it's a valid point.
>
> > In some respects the plane _object_ looks like it would fit - it has a
> > pixel format list and an FB_ID. But everything else would be acting
> > the exact opposite to a normal plane, and I think there's a bunch of
> > baked-in assumptions in the kernel and userspace around CRTCs always
> > having at least one connector.
>
> Yep, but writeback connectors are already different enough to not be
> considered as regular connectors, so userspace programs will have to
> handle them differently anyway (pass a framebuffer and pixel format to
> it before adding them to the display pipeline).
>
> Anyway, I see this approach has already been suggested in [1], and you
> all agreed that the writeback feature should be exposed as a connector,
> so I'll just stop here :-).
>
> Thanks for taking the time to explain the rationale behind this
> decision.

No problem, now is the right time to be discussing the decision before
we merge something wrong.

Are you happy enough with the connector approach then? Any concerns
with going ahead with it?

Cheers,
-Brian