On Tue, Apr 16, 2019 at 2:43 PM Liviu Dudau <liviu.du...@arm.com> 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.du...@arm.com> 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 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
