Re: [v2,1/2] drm: Add writeback_w,h properties

[Daniel Vetter]

On Tue, Apr 16, 2019 at 2:43 PM Liviu Dudau <Liviu.Dudau@xxxxxxx> wrote:
>
> On Tue, Apr 16, 2019 at 11:55:34AM +0200, Daniel Vetter wrote:
> > On Tue, Apr 16, 2019 at 11:17 AM Liviu Dudau <Liviu.Dudau@xxxxxxx> wrote:
> > >
> > > On Tue, Apr 16, 2019 at 09:34:20AM +0200, Daniel Vetter wrote:
> > > > On Mon, Apr 15, 2019 at 10:20:45AM +0100, Liviu Dudau wrote:
> > > > > On Mon, Apr 15, 2019 at 08:59:30AM +0100, james qian wang (Arm Technology China) wrote:
> > > > > > Hi Ben:
> > > > > > Do we real need these two individual properties for specifing the writeback
> > > > > > w/h, can we use fb->w/h ?
> > > > > > And since these two properties are added as common and standard properties,
> > > > > > it influnce all existing write-back implementation which all assumed
> > > > > > writeback size as fb->w/h.
> > > > >
> > > > > The idea of having these additional properties was to maintain backwards
> > > > > compatibility (of some sort). If you don't set writeback_w/h then the
> > > > > assumption is that they are the same as fb->w/h, but I can see how it's
> > > > > going to be confusing to have fb->w/h different from crtc->w/h and
> > > > > different from writeback_w/h.
> > > >
> > > > Since we already have crtc_w/h independent of writeback_fb_w/h, do we need
> > > > another set of w/h? Are all the interactions between these tree
> > > > well-defined?
> > >
> > > No, we probably don't need another set of w/h values. As for the
> > > interaction, I propose the following:
> > >
> > > - writeback is only attached to a connector, so the crtc_x/y/w/h are the
> > >   "input source" parameters into the writeback. Given that we put in the
> > >   writeback the content of the CRTC, I suggest we ignore x,y (consider
> > >   them to be always 0 for the writeback output).
> >
> > There's no crtc_x/y.
>
> Bah, sorry about that!
>
> >                       And what I meant with crtc_w/h is
> > crtc_state->mode->h/vdisplay. And you can already express scaling with
> > that, by setting the plane_state->crtc_x/y/h/w parameters as you wish.
>
> We don't have a plane associated with the writeback, it's a connector
> with a framebuffer attached to it. :(

I meant the input planes, I know there's no output/writeback planes.
Afaiui what we want to do is:

planes -> plane composition hw -> some scaling -> writeback

We can model this already (I think at least, that's really my question
here) by adjustinv mode->h/vdisplay and ofc also adjusting dst_x/y/w/h
of all the input planes. But is that good enough?

With your proposal (either autoscaling per fb->w/h from the writeback
fb, or the writeback_w/h properties we'd have double-scaling:

planes -> plane composition hw -> some scaling (because of
crtc->mode.h/vdisplay) -> more scaling (due to writeback h/w, whether
as props or from the fb) -> writeback

Scaling twice with nothing else in the pipeline seems silly to me.

> > > - writeback has a framebuffer attached, so we can use the fb->w/h to
> > >   determine if we need to do any scaling of the output.
> >
> > Hm, not sure we want that. You might want to automatically round
> > fb->w/h to whatever (ok right now we don't do that I think anywhere),
> > so forcing the output to match the fb->h/w doesn't make sense for me.
> > It's also inconsistent with how we treat fb attached to planes (where
> > we have the full set of src coordinates).
>
> The writeback will have to fit into that framebuffer, so it can't be
> bigger than fb->w/h. Smaller, yes.

Yeah we should probably check that in core atomic code. Like we do
already for planes in drm_atomic_plane_check()

> > > > Atm I'm assuming that writeback is using crtc_w/h into the fb, at offset
> > > > 0, not scaled any further. But the planes themselves can be scaled into
> > > > the crtc_w/h window ofc.
> > >
> > > Our hardware has the ability to also write back one of the planes and
> > > scale it during writeback, but we have no way of expressing that in DRM
> > > via connectors, so we are going to ignore that case. The supported usecase
> > > is for the planes to be scaled into the "composition" area which is the size
> > > of the CRTC, and then writeback that to memory, possibly with scaling (we
> > > officially only going to support downscaling due to external dependencies
> > > on the memory clock that is needed for supporting upscaling).
> >
> > Hm, see above, you can already express that:
> > - set crtc_state->mode->h/vdisplay to what you want the resulting
> > writeback image to be sized at.
> > - place the plane however you want using the plane properties within
> > that crtc window.
>
> The way we agreed ~2 years ago in the community to implement this was to have
> the writeback as a connector that gets the "output" of the CRTC and writes it into
> memory rather than on phosphorus. We have not added a plane to that, but
> in hindsight we might find it would have been useful.

Still not talking about planes on the output side here :-) Adding a
plane would also just give you double-scaling (but with the benefit
that you can set writeback_x/y too, plus select a window of the
overall crtc to write back).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch