Re: [PATCH v3 0/2] Add writeback scaling
On Tue, Apr 23, 2019 at 3:00 PM Liviu Dudau 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 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@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 0/2] Add writeback scaling
On Tue, Apr 23, 2019 at 02:24:11PM +0200, Daniel Vetter wrote: > On Tue, Apr 23, 2019 at 12:48 PM Ben Davis 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? > > 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? 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! / --- ¯\_(ツ)_/¯ ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 0/2] Add writeback scaling
On Tue, Apr 23, 2019 at 12:48 PM Ben Davis 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. 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. -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 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
