On Tue, Apr 23, 2019 at 3:00 PM Liviu Dudau <Liviu.Dudau@xxxxxxx> wrote: > > On Tue, Apr 23, 2019 at 02:24:11PM +0200, Daniel Vetter wrote: > > On Tue, Apr 23, 2019 at 12:48 PM Ben Davis <Ben.Davis@xxxxxxx> wrote: > > > > > > Add support for scaling on writeback. To do this add writeback_dest_w > > > and writeback_dest_h as writeback connector properties to specify the > > > desired output dimensions. > > > Then implement downscaling on writeback for Malidp-550 and Malidp-650 > > > (upscaling on writeback is not supported on these devices). > > > > > > v2: Use 0 as default for writeback_w,h and so update range to use 1 as > > > minimum. > > > > Hm I missed that, I don't think that's good, since it prevents > > userspace from blindly writing back the properties it's read. We've > > tried hard to avoid that, since we're suggesting compositor can take a > > snapshot of all kms properties (including the ones they don't > > understand) and restore that on vt switching. Hence stuff like fence > > fds returning -1, and accepting -1 as NULL to make this work. > > Is that they way it should work, though? I remember a few good years > back, before KMS, there were lots of issues with X server hanging and > not restoring the vt mode correctly. Should it not be that on getting > back control of a DRM master, an application sets it last known good > mode that it had, before it lost (or gave away) control? Given that we've recommended both approaches I expect there's a mix of both approaches out there in the real world, with people just hacking things up until it works. That's also what we're doing with the in-kernel fbdev emulation. I'd say defacto this means it's all ill-defined, but also that we imo should make a best effort to support either approach. > > tldr; I think range needs to include 0, and we need make that a > > special thing, maybe enforced with a > > drm_connector_state_compute_writeback_dst_h/w, which takes > > crtc_state->mode.v/hdisplay into account if the writeback_dst_h/w is > > 0. > > Repeating just to make sure we're on the same page: we allow 0 as valid > range value, but writeback_dst_h/w ultimately gets updated before the > commit gets passed on to the driver to use crtc_state->mode.v/hdisplay > so as not to trigger any scaling. Correct? That would also break stuff, since then you'd scale the next time v/hdisplay are different. Except if we again clear them back to 0 on duplicate_state, which I guess would work since writeback is one-shot. I was thinking more of a helper function, but your approach + resetting always to 0 on duplicate_state + lots of comments/kerneldoc should work too. And I guess would be more robust, since driver authors can't accidentally use the wrong state since it's fixed up. -Daniel > > Best regards, > Liviu > > > > -Daniel > > > > > > > > v3: Rename properties to specify they are destination width/height. > > > Make sure the values from the properties are passed to > > > enable_memwrite rather than the framebuffer dimensions > > > > > > Ben Davis (2): > > > drm: Add writeback_dest_w,h properties > > > drm/malidp: Enable writeback scaling > > > > > > drivers/gpu/drm/arm/malidp_crtc.c | 49 +++++++++++--------- > > > drivers/gpu/drm/arm/malidp_crtc.h | 12 +++++ > > > drivers/gpu/drm/arm/malidp_drv.c | 10 +++-- > > > drivers/gpu/drm/arm/malidp_hw.c | 45 +++++++++++++------ > > > drivers/gpu/drm/arm/malidp_hw.h | 19 ++++++-- > > > drivers/gpu/drm/arm/malidp_mw.c | 94 ++++++++++++++++++++++++++++++--------- > > > drivers/gpu/drm/arm/malidp_regs.h | 1 + > > > drivers/gpu/drm/drm_atomic_uapi.c | 8 ++++ > > > drivers/gpu/drm/drm_writeback.c | 30 +++++++++++++ > > > include/drm/drm_connector.h | 4 ++ > > > include/drm/drm_mode_config.h | 10 +++++ > > > 11 files changed, 220 insertions(+), 62 deletions(-) > > > create mode 100644 drivers/gpu/drm/arm/malidp_crtc.h > > > > > > -- > > > 2.7.4 > > > > > > > > > -- > > Daniel Vetter > > Software Engineer, Intel Corporation > > +41 (0) 79 365 57 48 - http://blog.ffwll.ch > > -- > ==================== > | I would like to | > | fix the world, | > | but they're not | > | giving me the | > \ source code! / > --------------- > ¯\_(ツ)_/¯ -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
