Re: [RFC PATCH 00/11] Introduce writeback connectors
On Fri, Oct 14, 2016 at 2:39 PM, Brian Starkeywrote: >> - Besides the above property, writeback hardware can have provisions >> for scaling, color space conversion and rotation. This would mean that >> we'd eventually add more writeback specific props/params in >> drm_connector/drm_connector_state. Would we be okay adding more such >> props for connectors? > > > I've wondered the same thing about bloating non-writeback connectors > with writeback-specific stuff. If it does become significant, maybe > we should subclass drm_connector and add a drm_writeback_state pointer > to drm_connector_state. No pionters needed, just embedded drm_connector_state into drm_writeback_connector_state as the "base" member. Then we can provide ready-made atomic_set/get_property functions for the aditional writeback functionality. But tbh I'd only start doing that once we have a few more. It's purely an implementation change, with no effect on userspace. And if you go with my drm_writeback_connector_init idea, it won't even be an issue for drivers. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 00/11] Introduce writeback connectors
On Fri, Oct 14, 2016 at 01:39:15PM +0100, Brian Starkey wrote: > Hi Archit, > > On Fri, Oct 14, 2016 at 04:20:14PM +0530, Archit Taneja wrote: > >Hi Brian, > > > >On 10/11/2016 08:23 PM, Brian Starkey wrote: > >>Hi, > >> > >>This RFC series introduces a new connector type: > >> DRM_MODE_CONNECTOR_WRITEBACK > >>It is a follow-on from a previous discussion: [1] > >> > >>Writeback connectors are used to expose the memory writeback engines > >>found in some display controllers, which can write a CRTC's > >>composition result to a memory buffer. > >>This is useful e.g. for testing, screen-recording, screenshots, > >>wireless display, display cloning, memory-to-memory composition. > >> > >>Patches 1-7 include the core framework changes required, and patches > >>8-11 implement a writeback connector for the Mali-DP writeback engine. > >>The Mali-DP patches depend on this other series: [2]. > >> > >>The connector is given the FB_ID property for the output framebuffer, > >>and two new read-only properties: PIXEL_FORMATS and > >>PIXEL_FORMATS_SIZE, which expose the supported framebuffer pixel > >>formats of the engine. > >> > >>The EDID property is not exposed for writeback connectors. > >> > >>Writeback connector usage: > >>-- > >>Due to connector routing changes being treated as "full modeset" > >>operations, any client which wishes to use a writeback connector > >>should include the connector in every modeset. The writeback will not > >>actually become active until a framebuffer is attached. > >> > >>The writeback itself is enabled by attaching a framebuffer to the > >>FB_ID property of the connector. The driver must then ensure that the > >>CRTC content of that atomic commit is written into the framebuffer. > >> > >>The writeback works in a one-shot mode with each atomic commit. This > >>prevents the same content from being written multiple times. > >>In some cases (front-buffer rendering) there might be a desire for > >>continuous operation - I think a property could be added later for > >>this kind of control. > >> > >>Writeback can be disabled by setting FB_ID to zero. > >> > >>Known issues: > >>- > >> * I'm not sure what "DPMS" should mean for writeback connectors. > >> It could be used to disable writeback (even when a framebuffer is > >> attached), or it could be hidden entirely (which would break the > >> legacy DPMS call for writeback connectors). > >> * With Daniel's recent re-iteration of the userspace API rules, I > >> fully expect to provide some userspace code to support this. The > >> question is what, and where? We want to use writeback for testing, > >> so perhaps some tests in igt is suitable. > >> * Documentation. Probably some portion of this cover letter needs to > >> make it into Documentation/ > >> * Synchronisation. Our hardware will finish the writeback by the next > >> vsync. I've not implemented fence support here, but it would be an > >> obvious addition. > >> > >>See Also: > >>- > >>[1] https://lists.freedesktop.org/archives/dri-devel/2016-July/113197.html > >>[2] > >>https://lists.freedesktop.org/archives/dri-devel/2016-October/120486.html > >> > >>I welcome any comments, especially if this approach does/doesn't fit > >>well with anyone else's hardware. > > > >Thanks for working on this! Some points below. > > > >- Writeback hardware generally allows us to specify the region within > >the framebuffer we want to write to. It's analogous to the SRC_X/Y/W/H > >plane properties. We could have similar props for the writeback > >connectors, and maybe set them to the FB_ID dimensions if they aren't > >configured by userspace. > > > >- Besides the above property, writeback hardware can have provisions > >for scaling, color space conversion and rotation. This would mean that > >we'd eventually add more writeback specific props/params in > >drm_connector/drm_connector_state. Would we be okay adding more such > >props for connectors? > > I've wondered the same thing about bloating non-writeback connectors > with writeback-specific stuff. If it does become significant, maybe > we should subclass drm_connector and add a drm_writeback_state pointer > to drm_connector_state. > > Ville touched on scaling support previously, suggesting adding a > fixed_mode property (for all types of connectors) - on writeback this > would represent scaling the framebuffer, and on normal connectors it > could control output scaling (like panel-fitting). We got some patches [1] posted for i915 recently that added a bunch of new properties to control post-blending scaling, but I'm not sure I like the approach since it seems harder to reconcile with the current way we deal with scaling for eDP/LVDS/DSI/etc. So I'm still somewhat partial to the fixed mode idea. Just FYI. [1] https://lists.freedesktop.org/archives/intel-gfx/2016-August/105557.html > > Certainly destination coords, color-space converstion etc. are things > that are worth adding, but IMO
Re: [RFC PATCH 00/11] Introduce writeback connectors
Hi Archit, On Fri, Oct 14, 2016 at 04:20:14PM +0530, Archit Taneja wrote: Hi Brian, On 10/11/2016 08:23 PM, Brian Starkey wrote: Hi, This RFC series introduces a new connector type: DRM_MODE_CONNECTOR_WRITEBACK It is a follow-on from a previous discussion: [1] Writeback connectors are used to expose the memory writeback engines found in some display controllers, which can write a CRTC's composition result to a memory buffer. This is useful e.g. for testing, screen-recording, screenshots, wireless display, display cloning, memory-to-memory composition. Patches 1-7 include the core framework changes required, and patches 8-11 implement a writeback connector for the Mali-DP writeback engine. The Mali-DP patches depend on this other series: [2]. The connector is given the FB_ID property for the output framebuffer, and two new read-only properties: PIXEL_FORMATS and PIXEL_FORMATS_SIZE, which expose the supported framebuffer pixel formats of the engine. The EDID property is not exposed for writeback connectors. Writeback connector usage: -- Due to connector routing changes being treated as "full modeset" operations, any client which wishes to use a writeback connector should include the connector in every modeset. The writeback will not actually become active until a framebuffer is attached. The writeback itself is enabled by attaching a framebuffer to the FB_ID property of the connector. The driver must then ensure that the CRTC content of that atomic commit is written into the framebuffer. The writeback works in a one-shot mode with each atomic commit. This prevents the same content from being written multiple times. In some cases (front-buffer rendering) there might be a desire for continuous operation - I think a property could be added later for this kind of control. Writeback can be disabled by setting FB_ID to zero. Known issues: - * I'm not sure what "DPMS" should mean for writeback connectors. It could be used to disable writeback (even when a framebuffer is attached), or it could be hidden entirely (which would break the legacy DPMS call for writeback connectors). * With Daniel's recent re-iteration of the userspace API rules, I fully expect to provide some userspace code to support this. The question is what, and where? We want to use writeback for testing, so perhaps some tests in igt is suitable. * Documentation. Probably some portion of this cover letter needs to make it into Documentation/ * Synchronisation. Our hardware will finish the writeback by the next vsync. I've not implemented fence support here, but it would be an obvious addition. See Also: - [1] https://lists.freedesktop.org/archives/dri-devel/2016-July/113197.html [2] https://lists.freedesktop.org/archives/dri-devel/2016-October/120486.html I welcome any comments, especially if this approach does/doesn't fit well with anyone else's hardware. Thanks for working on this! Some points below. - Writeback hardware generally allows us to specify the region within the framebuffer we want to write to. It's analogous to the SRC_X/Y/W/H plane properties. We could have similar props for the writeback connectors, and maybe set them to the FB_ID dimensions if they aren't configured by userspace. - Besides the above property, writeback hardware can have provisions for scaling, color space conversion and rotation. This would mean that we'd eventually add more writeback specific props/params in drm_connector/drm_connector_state. Would we be okay adding more such props for connectors? I've wondered the same thing about bloating non-writeback connectors with writeback-specific stuff. If it does become significant, maybe we should subclass drm_connector and add a drm_writeback_state pointer to drm_connector_state. Ville touched on scaling support previously, suggesting adding a fixed_mode property (for all types of connectors) - on writeback this would represent scaling the framebuffer, and on normal connectors it could control output scaling (like panel-fitting). Certainly destination coords, color-space converstion etc. are things that are worth adding, but IMO I'd rather keep this initial implementation small so we can enable the basic case right away. For the most part, the additional things are "just properties" which should be easily added later without impacting the overall interface. Cheers, Brian Thanks, Archit Thanks, -Brian --- Brian Starkey (10): drm: add writeback connector type drm/fb-helper: skip writeback connectors drm: extract CRTC/plane disable from drm_framebuffer_remove drm: add __drm_framebuffer_remove_atomic drm: add fb to connector state drm: expose fb_id property for writeback connectors drm: add writeback-connector pixel format properties drm: mali-dp: rename malidp_input_format drm: mali-dp: add RGB writeback formats for DP550/DP650 drm: mali-dp: add writeback connector Liviu Dudau (1): drm: mali-dp: Add support for
Re: [RFC PATCH 00/11] Introduce writeback connectors
Hi Brian, On 10/11/2016 08:23 PM, Brian Starkey wrote: Hi, This RFC series introduces a new connector type: DRM_MODE_CONNECTOR_WRITEBACK It is a follow-on from a previous discussion: [1] Writeback connectors are used to expose the memory writeback engines found in some display controllers, which can write a CRTC's composition result to a memory buffer. This is useful e.g. for testing, screen-recording, screenshots, wireless display, display cloning, memory-to-memory composition. Patches 1-7 include the core framework changes required, and patches 8-11 implement a writeback connector for the Mali-DP writeback engine. The Mali-DP patches depend on this other series: [2]. The connector is given the FB_ID property for the output framebuffer, and two new read-only properties: PIXEL_FORMATS and PIXEL_FORMATS_SIZE, which expose the supported framebuffer pixel formats of the engine. The EDID property is not exposed for writeback connectors. Writeback connector usage: -- Due to connector routing changes being treated as "full modeset" operations, any client which wishes to use a writeback connector should include the connector in every modeset. The writeback will not actually become active until a framebuffer is attached. The writeback itself is enabled by attaching a framebuffer to the FB_ID property of the connector. The driver must then ensure that the CRTC content of that atomic commit is written into the framebuffer. The writeback works in a one-shot mode with each atomic commit. This prevents the same content from being written multiple times. In some cases (front-buffer rendering) there might be a desire for continuous operation - I think a property could be added later for this kind of control. Writeback can be disabled by setting FB_ID to zero. Known issues: - * I'm not sure what "DPMS" should mean for writeback connectors. It could be used to disable writeback (even when a framebuffer is attached), or it could be hidden entirely (which would break the legacy DPMS call for writeback connectors). * With Daniel's recent re-iteration of the userspace API rules, I fully expect to provide some userspace code to support this. The question is what, and where? We want to use writeback for testing, so perhaps some tests in igt is suitable. * Documentation. Probably some portion of this cover letter needs to make it into Documentation/ * Synchronisation. Our hardware will finish the writeback by the next vsync. I've not implemented fence support here, but it would be an obvious addition. See Also: - [1] https://lists.freedesktop.org/archives/dri-devel/2016-July/113197.html [2] https://lists.freedesktop.org/archives/dri-devel/2016-October/120486.html I welcome any comments, especially if this approach does/doesn't fit well with anyone else's hardware. Thanks for working on this! Some points below. - Writeback hardware generally allows us to specify the region within the framebuffer we want to write to. It's analogous to the SRC_X/Y/W/H plane properties. We could have similar props for the writeback connectors, and maybe set them to the FB_ID dimensions if they aren't configured by userspace. - Besides the above property, writeback hardware can have provisions for scaling, color space conversion and rotation. This would mean that we'd eventually add more writeback specific props/params in drm_connector/drm_connector_state. Would we be okay adding more such props for connectors? Thanks, Archit Thanks, -Brian --- Brian Starkey (10): drm: add writeback connector type drm/fb-helper: skip writeback connectors drm: extract CRTC/plane disable from drm_framebuffer_remove drm: add __drm_framebuffer_remove_atomic drm: add fb to connector state drm: expose fb_id property for writeback connectors drm: add writeback-connector pixel format properties drm: mali-dp: rename malidp_input_format drm: mali-dp: add RGB writeback formats for DP550/DP650 drm: mali-dp: add writeback connector Liviu Dudau (1): drm: mali-dp: Add support for writeback on DP550/DP650 drivers/gpu/drm/arm/Makefile|1 + drivers/gpu/drm/arm/malidp_crtc.c | 10 ++ drivers/gpu/drm/arm/malidp_drv.c| 25 +++- drivers/gpu/drm/arm/malidp_drv.h|5 + drivers/gpu/drm/arm/malidp_hw.c | 104 ++ drivers/gpu/drm/arm/malidp_hw.h | 27 +++- drivers/gpu/drm/arm/malidp_mw.c | 268 +++ drivers/gpu/drm/arm/malidp_planes.c |8 +- drivers/gpu/drm/arm/malidp_regs.h | 15 ++ drivers/gpu/drm/drm_atomic.c| 40 ++ drivers/gpu/drm/drm_atomic_helper.c |4 + drivers/gpu/drm/drm_connector.c | 79 ++- drivers/gpu/drm/drm_crtc.c | 14 +- drivers/gpu/drm/drm_fb_helper.c |4 + drivers/gpu/drm/drm_framebuffer.c | 249 drivers/gpu/drm/drm_ioctl.c |7 + include/drm/drmP.h
Re: [RFC PATCH 00/11] Introduce writeback connectors
Brian Starkeywrites: > Hi Eric, > > On Tue, Oct 11, 2016 at 12:01:14PM -0700, Eric Anholt wrote: >>Brian Starkey writes: >> >>> Hi, >>> >>> This RFC series introduces a new connector type: >>> DRM_MODE_CONNECTOR_WRITEBACK >>> It is a follow-on from a previous discussion: [1] >>> >>> Writeback connectors are used to expose the memory writeback engines >>> found in some display controllers, which can write a CRTC's >>> composition result to a memory buffer. >>> This is useful e.g. for testing, screen-recording, screenshots, >>> wireless display, display cloning, memory-to-memory composition. >>> >>> Patches 1-7 include the core framework changes required, and patches >>> 8-11 implement a writeback connector for the Mali-DP writeback engine. >>> The Mali-DP patches depend on this other series: [2]. >>> >>> The connector is given the FB_ID property for the output framebuffer, >>> and two new read-only properties: PIXEL_FORMATS and >>> PIXEL_FORMATS_SIZE, which expose the supported framebuffer pixel >>> formats of the engine. >>> >>> The EDID property is not exposed for writeback connectors. >>> >>> Writeback connector usage: >>> -- >>> Due to connector routing changes being treated as "full modeset" >>> operations, any client which wishes to use a writeback connector >>> should include the connector in every modeset. The writeback will not >>> actually become active until a framebuffer is attached. >>> >>> The writeback itself is enabled by attaching a framebuffer to the >>> FB_ID property of the connector. The driver must then ensure that the >>> CRTC content of that atomic commit is written into the framebuffer. >>> >>> The writeback works in a one-shot mode with each atomic commit. This >>> prevents the same content from being written multiple times. >>> In some cases (front-buffer rendering) there might be a desire for >>> continuous operation - I think a property could be added later for >>> this kind of control. >>> >>> Writeback can be disabled by setting FB_ID to zero. >> >>I think this sounds great, and the interface is just right IMO. >> > > Thanks, glad you like it! Hopefully you're equally agreeable with the > changes Daniel has been suggesting. Haven't seen anything objectionable there. >>> Known issues: >>> - >>> * I'm not sure what "DPMS" should mean for writeback connectors. >>>It could be used to disable writeback (even when a framebuffer is >>>attached), or it could be hidden entirely (which would break the >>>legacy DPMS call for writeback connectors). >>> * With Daniel's recent re-iteration of the userspace API rules, I >>>fully expect to provide some userspace code to support this. The >>>question is what, and where? We want to use writeback for testing, >>>so perhaps some tests in igt is suitable. >>> * Documentation. Probably some portion of this cover letter needs to >>>make it into Documentation/ >>> * Synchronisation. Our hardware will finish the writeback by the next >>>vsync. I've not implemented fence support here, but it would be an >>>obvious addition. >> >>My hardware won't necessarily finish by the next vsync -- it trickles >>out at whatever rate it can find memory bandwidth to get the job done, >>and fires an interrupt when it's finished. >> > > Is it bounded? You presumably have to finish the write-out before you > can change any input buffers? Yeah, I'm not sure what it would mean to try to swap my display list while write-out was happening. Each CRTC (each of which can only support one encoder at a time) has its own display list, though, so it could avoid blocking other modesets. >>So I would like some definition for how syncing works. One answer would >>be that these flips don't trigger their pageflip events until the >>writeback is done (so I need to collect both the vsync irq and the >>writeback irq before sending). Another would be that manage an >>independent fence for the writeback fb, so that you still immediately >>know when framebuffers from the previous scanout-only frame are idle. >> > > I much prefer the sound of the explicit fence approach. > > Hopefully we can agree that a new atomic commit can't be completed > whilst there's a writeback ongoing, otherwise managing the fence and > framebuffer lifetime sounds really tricky - they'd need to be decoupled > from the atomic_state and outlive the commit that spawned them. Oh, good point. I'm fine with that, but then my anticipated usecases for writeback are testing (don't care about performance) and fallback plane-squashing when a complicated modeset exceeds limits (in which case you have no simpler plane config to modeset to until the writeback is completed, anyway). signature.asc Description: PGP signature
Re: [RFC PATCH 00/11] Introduce writeback connectors
Hi Eric, On Tue, Oct 11, 2016 at 12:01:14PM -0700, Eric Anholt wrote: Brian Starkeywrites: Hi, This RFC series introduces a new connector type: DRM_MODE_CONNECTOR_WRITEBACK It is a follow-on from a previous discussion: [1] Writeback connectors are used to expose the memory writeback engines found in some display controllers, which can write a CRTC's composition result to a memory buffer. This is useful e.g. for testing, screen-recording, screenshots, wireless display, display cloning, memory-to-memory composition. Patches 1-7 include the core framework changes required, and patches 8-11 implement a writeback connector for the Mali-DP writeback engine. The Mali-DP patches depend on this other series: [2]. The connector is given the FB_ID property for the output framebuffer, and two new read-only properties: PIXEL_FORMATS and PIXEL_FORMATS_SIZE, which expose the supported framebuffer pixel formats of the engine. The EDID property is not exposed for writeback connectors. Writeback connector usage: -- Due to connector routing changes being treated as "full modeset" operations, any client which wishes to use a writeback connector should include the connector in every modeset. The writeback will not actually become active until a framebuffer is attached. The writeback itself is enabled by attaching a framebuffer to the FB_ID property of the connector. The driver must then ensure that the CRTC content of that atomic commit is written into the framebuffer. The writeback works in a one-shot mode with each atomic commit. This prevents the same content from being written multiple times. In some cases (front-buffer rendering) there might be a desire for continuous operation - I think a property could be added later for this kind of control. Writeback can be disabled by setting FB_ID to zero. I think this sounds great, and the interface is just right IMO. Thanks, glad you like it! Hopefully you're equally agreeable with the changes Daniel has been suggesting. I don't really see a use for continuous mode -- a sequence of one-shots makes a lot more sense because then you can know what data has changed, which anyone trying to use the writeback buffer would need to know. Agreed - we've never found a use for it. Known issues: - * I'm not sure what "DPMS" should mean for writeback connectors. It could be used to disable writeback (even when a framebuffer is attached), or it could be hidden entirely (which would break the legacy DPMS call for writeback connectors). * With Daniel's recent re-iteration of the userspace API rules, I fully expect to provide some userspace code to support this. The question is what, and where? We want to use writeback for testing, so perhaps some tests in igt is suitable. * Documentation. Probably some portion of this cover letter needs to make it into Documentation/ * Synchronisation. Our hardware will finish the writeback by the next vsync. I've not implemented fence support here, but it would be an obvious addition. My hardware won't necessarily finish by the next vsync -- it trickles out at whatever rate it can find memory bandwidth to get the job done, and fires an interrupt when it's finished. Is it bounded? You presumably have to finish the write-out before you can change any input buffers? So I would like some definition for how syncing works. One answer would be that these flips don't trigger their pageflip events until the writeback is done (so I need to collect both the vsync irq and the writeback irq before sending). Another would be that manage an independent fence for the writeback fb, so that you still immediately know when framebuffers from the previous scanout-only frame are idle. I much prefer the sound of the explicit fence approach. Hopefully we can agree that a new atomic commit can't be completed whilst there's a writeback ongoing, otherwise managing the fence and framebuffer lifetime sounds really tricky - they'd need to be decoupled from the atomic_state and outlive the commit that spawned them. Cheers, -Brian Also, tests for this in igt, please. Writeback in igt will give us so much more ability to cover KMS functionality on non-Intel hardware. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 00/11] Introduce writeback connectors
On Tue, Oct 11, 2016 at 10:24:23PM +0100, Brian Starkey wrote: > On Tue, Oct 11, 2016 at 10:02:43PM +0200, Daniel Vetter wrote: > > The problem with just that is that there's lots of different things > > that can feed into the overall needs_modeset variable. That's why we > > split it up into multiple booleans. > > > > So yes you're supposed to clear connectors_changed if there is some > > change that you can handle without a full modeset. If you want, think > > of connectors_changed as > > needs_modeset_due_to_change_in_connnector_state, but that's cumbersome > > to type and too long ;-) > > > > All right, got it :-). This intention wasn't clear to me from the > comments in the code. A patch to update the kernel-doc to make it clearer (there's mode_changed, connectors_changed and active_changed, plus drm_crtc_needs_modeset) would be awesome. I'm trying to write useful docs, but since I designed this all I sometimes forget to make the non-obvious assumptions clear enough. Volunteered? > > > > tbh I don't like that, I think it'd be better to make this truly > > > > one-shot. Otherwise we'll have real fun problems with hw where the > > > > writeback can take longer than a vblank (it happens ...). So one-shot, > > > > with auto-clearing to NULL/0 is imo the right approach. > > > > > > That's an interesting point about hardware which won't finish within > > > one frame; but I don't see how "true one-shot" helps. > > > > > > What's the expected behaviour if userspace makes a new atomic commit > > > with a writeback framebuffer whilst a previous writeback is ongoing? > > > > > > In both cases, you either need to block or fail the commit - whether > > > the framebuffer gets removed when it's done is immaterial. > > > > See Eric's question. We need to define that, and I think the simplest > > approach is a completion fence/sync_file. It's destaged now in 4.9, we > > can use them. I think the simplest uabi would be a pointer property > > (u64) where we write the fd of the fence we'll signal when write-out > > completes. > > > > That tells userspace that the previous writeback is finished, I agree that's > needed. It doesn't define any behaviour in case userspace asks for another > writeback before that fence fires though. Hm, good point. I guess we could just state that if userspace does a writeback, and issues a new writeback before both a) the atomic flip and b) the write back complete fence signalled will lead to undefined behaviour. Undefined as in: data corruption, rejected atomic commit or anything else than a kernel crash is allowed. This is similar to doing a page flip and starting to render to the old buffers before the flip event signalled completion: Userspace gets the mess it asked for ;-) -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 00/11] Introduce writeback connectors
On Tue, Oct 11, 2016 at 10:02:43PM +0200, Daniel Vetter wrote: On Tue, Oct 11, 2016 at 9:44 PM, Brian Starkeywrote: Hi, On Tue, Oct 11, 2016 at 07:01:33PM +0200, Daniel Vetter wrote: On Tue, Oct 11, 2016 at 6:43 PM, Brian Starkey wrote: Hi Daniel, Firstly thanks very much for having a look. On Tue, Oct 11, 2016 at 05:43:59PM +0200, Daniel Vetter wrote: On Tue, Oct 11, 2016 at 03:53:57PM +0100, Brian Starkey wrote: Hi, This RFC series introduces a new connector type: DRM_MODE_CONNECTOR_WRITEBACK It is a follow-on from a previous discussion: [1] Writeback connectors are used to expose the memory writeback engines found in some display controllers, which can write a CRTC's composition result to a memory buffer. This is useful e.g. for testing, screen-recording, screenshots, wireless display, display cloning, memory-to-memory composition. Patches 1-7 include the core framework changes required, and patches 8-11 implement a writeback connector for the Mali-DP writeback engine. The Mali-DP patches depend on this other series: [2]. The connector is given the FB_ID property for the output framebuffer, and two new read-only properties: PIXEL_FORMATS and PIXEL_FORMATS_SIZE, which expose the supported framebuffer pixel formats of the engine. The EDID property is not exposed for writeback connectors. Writeback connector usage: -- Due to connector routing changes being treated as "full modeset" operations, any client which wishes to use a writeback connector should include the connector in every modeset. The writeback will not actually become active until a framebuffer is attached. Erhm, this is just the default, drivers can override this. And we could change the atomic helpers to not mark a modeset as a modeset if the connector that changed is a writeback one. Hmm, maybe. I don't think it's ideal - the driver would need to re-implement drm_atomic_helper_check_modeset, which is quite a chunk of code (along with exposing update_connector_routing, mode_fixup, maybe others), and even after that it would have to lie and set crtc_state->connectors_changed to false so that drm_crtc_needs_modeset returns false to drm_atomic_check_only. You only need to update the property in your encoders's ->atomic_check function. No need for more, and I think being consistent with computing when you need a modeset is really a crucial part of the atomic ioctl that we should imo try to implement correctly as much as possible. Sorry I really don't follow. Which property? CRTC_ID? Userspace changing CRTC_ID will change connector_state->crtc (before we even get to a driver callback). After that, drm_atomic_helper_check_modeset derives connectors_changed based on the ->crtc pointers. After that, my encoder ->atomic_check *could* clear connectors_changed (or I could achieve the same thing by wrapping drm_atomic_helper_check), but it seems wrong to do so, considering that the connector routing *has* changed. If you think changing CRTC_ID shouldn't require a full modeset, I'd rather give drivers a ->needs_modeset callback to override the default drm_atomic_crtc_needs_modeset behaviour, instead of "tricking" it into returning false. The problem with just that is that there's lots of different things that can feed into the overall needs_modeset variable. That's why we split it up into multiple booleans. So yes you're supposed to clear connectors_changed if there is some change that you can handle without a full modeset. If you want, think of connectors_changed as needs_modeset_due_to_change_in_connnector_state, but that's cumbersome to type and too long ;-) All right, got it :-). This intention wasn't clear to me from the comments in the code. I can imagine some hardware will need a full modeset to changed the writeback CRTC binding anyway. Yup, and then they can upgrade this again. With all these flow-control booleans the idea is very much that helpers give a default that works for 90% of all cases, and driver callbacks can then change it for the other 10%. I tried to keep special-casing of writeback connectors in the core to a bare minimum, because I think it will quickly get messy and fragile otherwise. Please always make the disdinction between core and shared drm helpers. Special cases in core == probably not good. Special cases in helpers == perfectly fine imo. Honestly, I don't see modesetting the writeback connectors at start-of-day as a big problem. It's inconsistent. Claiming it needs a modeset when it doesn't isn't great. Making that more discoverable to userspace is the entire point of atomic. And there might be hw where reconfiguring for writeback might need a full modeset. I'm a little confused - what bit exactly is inconsistent? Not being truthful for when you need a modeset and when not. My implementation here is consistent with other connectors. Updating the writeback connector CRTC_ID
Re: [RFC PATCH 00/11] Introduce writeback connectors
On Tue, Oct 11, 2016 at 9:44 PM, Brian Starkeywrote: > Hi, > > > On Tue, Oct 11, 2016 at 07:01:33PM +0200, Daniel Vetter wrote: >> >> On Tue, Oct 11, 2016 at 6:43 PM, Brian Starkey >> wrote: >>> >>> Hi Daniel, >>> >>> Firstly thanks very much for having a look. >>> >>> >>> On Tue, Oct 11, 2016 at 05:43:59PM +0200, Daniel Vetter wrote: On Tue, Oct 11, 2016 at 03:53:57PM +0100, Brian Starkey wrote: > > > Hi, > > This RFC series introduces a new connector type: > DRM_MODE_CONNECTOR_WRITEBACK > It is a follow-on from a previous discussion: [1] > > Writeback connectors are used to expose the memory writeback engines > found in some display controllers, which can write a CRTC's > composition result to a memory buffer. > This is useful e.g. for testing, screen-recording, screenshots, > wireless display, display cloning, memory-to-memory composition. > > Patches 1-7 include the core framework changes required, and patches > 8-11 implement a writeback connector for the Mali-DP writeback engine. > The Mali-DP patches depend on this other series: [2]. > > The connector is given the FB_ID property for the output framebuffer, > and two new read-only properties: PIXEL_FORMATS and > PIXEL_FORMATS_SIZE, which expose the supported framebuffer pixel > formats of the engine. > > The EDID property is not exposed for writeback connectors. > > Writeback connector usage: > -- > Due to connector routing changes being treated as "full modeset" > operations, any client which wishes to use a writeback connector > should include the connector in every modeset. The writeback will not > actually become active until a framebuffer is attached. Erhm, this is just the default, drivers can override this. And we could change the atomic helpers to not mark a modeset as a modeset if the connector that changed is a writeback one. >>> >>> Hmm, maybe. I don't think it's ideal - the driver would need to >>> re-implement drm_atomic_helper_check_modeset, which is quite a chunk >>> of code (along with exposing update_connector_routing, mode_fixup, >>> maybe others), and even after that it would have to lie and set >>> crtc_state->connectors_changed to false so that >>> drm_crtc_needs_modeset returns false to drm_atomic_check_only. >> >> >> You only need to update the property in your encoders's ->atomic_check >> function. No need for more, and I think being consistent with >> computing when you need a modeset is really a crucial part of the >> atomic ioctl that we should imo try to implement correctly as much as >> possible. >> > > Sorry I really don't follow. Which property? CRTC_ID? > > Userspace changing CRTC_ID will change connector_state->crtc (before > we even get to a driver callback). > > After that, drm_atomic_helper_check_modeset derives connectors_changed > based on the ->crtc pointers. > > After that, my encoder ->atomic_check *could* clear > connectors_changed (or I could achieve the same thing by wrapping > drm_atomic_helper_check), but it seems wrong to do so, considering > that the connector routing *has* changed. > > If you think changing CRTC_ID shouldn't require a full modeset, I'd > rather give drivers a ->needs_modeset callback to override the default > drm_atomic_crtc_needs_modeset behaviour, instead of "tricking" it into > returning false. The problem with just that is that there's lots of different things that can feed into the overall needs_modeset variable. That's why we split it up into multiple booleans. So yes you're supposed to clear connectors_changed if there is some change that you can handle without a full modeset. If you want, think of connectors_changed as needs_modeset_due_to_change_in_connnector_state, but that's cumbersome to type and too long ;-) > I can imagine some hardware will need a full modeset to changed the > writeback CRTC binding anyway. Yup, and then they can upgrade this again. With all these flow-control booleans the idea is very much that helpers give a default that works for 90% of all cases, and driver callbacks can then change it for the other 10%. >>> I tried to keep special-casing of writeback connectors in the core to >>> a bare minimum, because I think it will quickly get messy and fragile >>> otherwise. >> >> >> Please always make the disdinction between core and shared drm >> helpers. Special cases in core == probably not good. Special cases in >> helpers == perfectly fine imo. >> >>> Honestly, I don't see modesetting the writeback connectors at >>> start-of-day as a big problem. >> >> >> It's inconsistent. Claiming it needs a modeset when it doesn't isn't >> great. Making that more discoverable to userspace is the entire point >> of atomic. And there might be hw where reconfiguring for writeback >> might need a full modeset. >> > > I'm a
Re: [RFC PATCH 00/11] Introduce writeback connectors
Hi, On Tue, Oct 11, 2016 at 07:01:33PM +0200, Daniel Vetter wrote: On Tue, Oct 11, 2016 at 6:43 PM, Brian Starkeywrote: Hi Daniel, Firstly thanks very much for having a look. On Tue, Oct 11, 2016 at 05:43:59PM +0200, Daniel Vetter wrote: On Tue, Oct 11, 2016 at 03:53:57PM +0100, Brian Starkey wrote: Hi, This RFC series introduces a new connector type: DRM_MODE_CONNECTOR_WRITEBACK It is a follow-on from a previous discussion: [1] Writeback connectors are used to expose the memory writeback engines found in some display controllers, which can write a CRTC's composition result to a memory buffer. This is useful e.g. for testing, screen-recording, screenshots, wireless display, display cloning, memory-to-memory composition. Patches 1-7 include the core framework changes required, and patches 8-11 implement a writeback connector for the Mali-DP writeback engine. The Mali-DP patches depend on this other series: [2]. The connector is given the FB_ID property for the output framebuffer, and two new read-only properties: PIXEL_FORMATS and PIXEL_FORMATS_SIZE, which expose the supported framebuffer pixel formats of the engine. The EDID property is not exposed for writeback connectors. Writeback connector usage: -- Due to connector routing changes being treated as "full modeset" operations, any client which wishes to use a writeback connector should include the connector in every modeset. The writeback will not actually become active until a framebuffer is attached. Erhm, this is just the default, drivers can override this. And we could change the atomic helpers to not mark a modeset as a modeset if the connector that changed is a writeback one. Hmm, maybe. I don't think it's ideal - the driver would need to re-implement drm_atomic_helper_check_modeset, which is quite a chunk of code (along with exposing update_connector_routing, mode_fixup, maybe others), and even after that it would have to lie and set crtc_state->connectors_changed to false so that drm_crtc_needs_modeset returns false to drm_atomic_check_only. You only need to update the property in your encoders's ->atomic_check function. No need for more, and I think being consistent with computing when you need a modeset is really a crucial part of the atomic ioctl that we should imo try to implement correctly as much as possible. Sorry I really don't follow. Which property? CRTC_ID? Userspace changing CRTC_ID will change connector_state->crtc (before we even get to a driver callback). After that, drm_atomic_helper_check_modeset derives connectors_changed based on the ->crtc pointers. After that, my encoder ->atomic_check *could* clear connectors_changed (or I could achieve the same thing by wrapping drm_atomic_helper_check), but it seems wrong to do so, considering that the connector routing *has* changed. If you think changing CRTC_ID shouldn't require a full modeset, I'd rather give drivers a ->needs_modeset callback to override the default drm_atomic_crtc_needs_modeset behaviour, instead of "tricking" it into returning false. I can imagine some hardware will need a full modeset to changed the writeback CRTC binding anyway. I tried to keep special-casing of writeback connectors in the core to a bare minimum, because I think it will quickly get messy and fragile otherwise. Please always make the disdinction between core and shared drm helpers. Special cases in core == probably not good. Special cases in helpers == perfectly fine imo. Honestly, I don't see modesetting the writeback connectors at start-of-day as a big problem. It's inconsistent. Claiming it needs a modeset when it doesn't isn't great. Making that more discoverable to userspace is the entire point of atomic. And there might be hw where reconfiguring for writeback might need a full modeset. I'm a little confused - what bit exactly is inconsistent? My implementation here is consistent with other connectors. Updating the writeback connector CRTC_ID property requires a full modeset, the same as other connectors. Changing the FB_ID does *not* require a full modeset, because our hardware has no such restriction. This is analogous to updating the FB_ID on our planes, and is consistent with the other instances of the FB_ID property. If there is hardware which does have a restriction on changing FB_ID, I think that driver must be responsible for handling it in the same way as drivers which can't handle plane updates without a full modeset. Are you saying that because setting CRTC_ID on Mali-DP is a no-op, it shouldn't require a full modeset? I'd rather somehow hard-code the CRTC_ID for our writeback connector to have it always attached to the CRTC in that case. The writeback itself is enabled by attaching a framebuffer to the FB_ID property of the connector. The driver must then ensure that the CRTC content of that atomic commit is written into the framebuffer. The writeback works in a one-shot mode
Re: [RFC PATCH 00/11] Introduce writeback connectors
Brian Starkeywrites: > Hi, > > This RFC series introduces a new connector type: > DRM_MODE_CONNECTOR_WRITEBACK > It is a follow-on from a previous discussion: [1] > > Writeback connectors are used to expose the memory writeback engines > found in some display controllers, which can write a CRTC's > composition result to a memory buffer. > This is useful e.g. for testing, screen-recording, screenshots, > wireless display, display cloning, memory-to-memory composition. > > Patches 1-7 include the core framework changes required, and patches > 8-11 implement a writeback connector for the Mali-DP writeback engine. > The Mali-DP patches depend on this other series: [2]. > > The connector is given the FB_ID property for the output framebuffer, > and two new read-only properties: PIXEL_FORMATS and > PIXEL_FORMATS_SIZE, which expose the supported framebuffer pixel > formats of the engine. > > The EDID property is not exposed for writeback connectors. > > Writeback connector usage: > -- > Due to connector routing changes being treated as "full modeset" > operations, any client which wishes to use a writeback connector > should include the connector in every modeset. The writeback will not > actually become active until a framebuffer is attached. > > The writeback itself is enabled by attaching a framebuffer to the > FB_ID property of the connector. The driver must then ensure that the > CRTC content of that atomic commit is written into the framebuffer. > > The writeback works in a one-shot mode with each atomic commit. This > prevents the same content from being written multiple times. > In some cases (front-buffer rendering) there might be a desire for > continuous operation - I think a property could be added later for > this kind of control. > > Writeback can be disabled by setting FB_ID to zero. I think this sounds great, and the interface is just right IMO. I don't really see a use for continuous mode -- a sequence of one-shots makes a lot more sense because then you can know what data has changed, which anyone trying to use the writeback buffer would need to know. > Known issues: > - > * I'm not sure what "DPMS" should mean for writeback connectors. >It could be used to disable writeback (even when a framebuffer is >attached), or it could be hidden entirely (which would break the >legacy DPMS call for writeback connectors). > * With Daniel's recent re-iteration of the userspace API rules, I >fully expect to provide some userspace code to support this. The >question is what, and where? We want to use writeback for testing, >so perhaps some tests in igt is suitable. > * Documentation. Probably some portion of this cover letter needs to >make it into Documentation/ > * Synchronisation. Our hardware will finish the writeback by the next >vsync. I've not implemented fence support here, but it would be an >obvious addition. My hardware won't necessarily finish by the next vsync -- it trickles out at whatever rate it can find memory bandwidth to get the job done, and fires an interrupt when it's finished. So I would like some definition for how syncing works. One answer would be that these flips don't trigger their pageflip events until the writeback is done (so I need to collect both the vsync irq and the writeback irq before sending). Another would be that manage an independent fence for the writeback fb, so that you still immediately know when framebuffers from the previous scanout-only frame are idle. Also, tests for this in igt, please. Writeback in igt will give us so much more ability to cover KMS functionality on non-Intel hardware. signature.asc Description: PGP signature
Re: [RFC PATCH 00/11] Introduce writeback connectors
On Tue, Oct 11, 2016 at 6:43 PM, Brian Starkeywrote: > Hi Daniel, > > Firstly thanks very much for having a look. > > > On Tue, Oct 11, 2016 at 05:43:59PM +0200, Daniel Vetter wrote: >> >> On Tue, Oct 11, 2016 at 03:53:57PM +0100, Brian Starkey wrote: >>> >>> Hi, >>> >>> This RFC series introduces a new connector type: >>> DRM_MODE_CONNECTOR_WRITEBACK >>> It is a follow-on from a previous discussion: [1] >>> >>> Writeback connectors are used to expose the memory writeback engines >>> found in some display controllers, which can write a CRTC's >>> composition result to a memory buffer. >>> This is useful e.g. for testing, screen-recording, screenshots, >>> wireless display, display cloning, memory-to-memory composition. >>> >>> Patches 1-7 include the core framework changes required, and patches >>> 8-11 implement a writeback connector for the Mali-DP writeback engine. >>> The Mali-DP patches depend on this other series: [2]. >>> >>> The connector is given the FB_ID property for the output framebuffer, >>> and two new read-only properties: PIXEL_FORMATS and >>> PIXEL_FORMATS_SIZE, which expose the supported framebuffer pixel >>> formats of the engine. >>> >>> The EDID property is not exposed for writeback connectors. >>> >>> Writeback connector usage: >>> -- >>> Due to connector routing changes being treated as "full modeset" >>> operations, any client which wishes to use a writeback connector >>> should include the connector in every modeset. The writeback will not >>> actually become active until a framebuffer is attached. >> >> >> Erhm, this is just the default, drivers can override this. And we could >> change the atomic helpers to not mark a modeset as a modeset if the >> connector that changed is a writeback one. >> > > Hmm, maybe. I don't think it's ideal - the driver would need to > re-implement drm_atomic_helper_check_modeset, which is quite a chunk > of code (along with exposing update_connector_routing, mode_fixup, > maybe others), and even after that it would have to lie and set > crtc_state->connectors_changed to false so that > drm_crtc_needs_modeset returns false to drm_atomic_check_only. You only need to update the property in your encoders's ->atomic_check function. No need for more, and I think being consistent with computing when you need a modeset is really a crucial part of the atomic ioctl that we should imo try to implement correctly as much as possible. > I tried to keep special-casing of writeback connectors in the core to > a bare minimum, because I think it will quickly get messy and fragile > otherwise. Please always make the disdinction between core and shared drm helpers. Special cases in core == probably not good. Special cases in helpers == perfectly fine imo. > Honestly, I don't see modesetting the writeback connectors at > start-of-day as a big problem. It's inconsistent. Claiming it needs a modeset when it doesn't isn't great. Making that more discoverable to userspace is the entire point of atomic. And there might be hw where reconfiguring for writeback might need a full modeset. >>> The writeback itself is enabled by attaching a framebuffer to the >>> FB_ID property of the connector. The driver must then ensure that the >>> CRTC content of that atomic commit is written into the framebuffer. >>> >>> The writeback works in a one-shot mode with each atomic commit. This >>> prevents the same content from being written multiple times. >>> In some cases (front-buffer rendering) there might be a desire for >>> continuous operation - I think a property could be added later for >>> this kind of control. >>> >>> Writeback can be disabled by setting FB_ID to zero. >> >> >> This seems to contradict itself: If it's one-shot, there's no need to >> disable it - it will auto-disable. > > > I should have explained one-shot more clearly. What I mean is, one > drmModeAtomicCommit == one write to memory. This is as opposed to > writing the same thing to memory every vsync until it is stopped > (which our HW is capable of doing). > > A subsequent drmModeAtomicCommit which doesn't touch the writeback FB_ID > will write (again - but with whatever scene updates) to the same > framebuffer. > > This continues for every drmModeAtomicCommit until FB_ID is set to > zero - to disable writing - or changed to a different framebuffer, in > which case we write to the new one. > > IMO this behaviour makes sense in the context of the rest of Atomic, > and as the FB_ID is indeed persistent across atomic commits, I think > it should be read-able. tbh I don't like that, I think it'd be better to make this truly one-shot. Otherwise we'll have real fun problems with hw where the writeback can take longer than a vblank (it happens ...). So one-shot, with auto-clearing to NULL/0 is imo the right approach. >> In other cases where we write a property as a one-shot thing (fences for >> android). In that case when you read that property it's always 0 (well,
Re: [RFC PATCH 00/11] Introduce writeback connectors
Hi Daniel, Firstly thanks very much for having a look. On Tue, Oct 11, 2016 at 05:43:59PM +0200, Daniel Vetter wrote: On Tue, Oct 11, 2016 at 03:53:57PM +0100, Brian Starkey wrote: Hi, This RFC series introduces a new connector type: DRM_MODE_CONNECTOR_WRITEBACK It is a follow-on from a previous discussion: [1] Writeback connectors are used to expose the memory writeback engines found in some display controllers, which can write a CRTC's composition result to a memory buffer. This is useful e.g. for testing, screen-recording, screenshots, wireless display, display cloning, memory-to-memory composition. Patches 1-7 include the core framework changes required, and patches 8-11 implement a writeback connector for the Mali-DP writeback engine. The Mali-DP patches depend on this other series: [2]. The connector is given the FB_ID property for the output framebuffer, and two new read-only properties: PIXEL_FORMATS and PIXEL_FORMATS_SIZE, which expose the supported framebuffer pixel formats of the engine. The EDID property is not exposed for writeback connectors. Writeback connector usage: -- Due to connector routing changes being treated as "full modeset" operations, any client which wishes to use a writeback connector should include the connector in every modeset. The writeback will not actually become active until a framebuffer is attached. Erhm, this is just the default, drivers can override this. And we could change the atomic helpers to not mark a modeset as a modeset if the connector that changed is a writeback one. Hmm, maybe. I don't think it's ideal - the driver would need to re-implement drm_atomic_helper_check_modeset, which is quite a chunk of code (along with exposing update_connector_routing, mode_fixup, maybe others), and even after that it would have to lie and set crtc_state->connectors_changed to false so that drm_crtc_needs_modeset returns false to drm_atomic_check_only. I tried to keep special-casing of writeback connectors in the core to a bare minimum, because I think it will quickly get messy and fragile otherwise. Honestly, I don't see modesetting the writeback connectors at start-of-day as a big problem. The writeback itself is enabled by attaching a framebuffer to the FB_ID property of the connector. The driver must then ensure that the CRTC content of that atomic commit is written into the framebuffer. The writeback works in a one-shot mode with each atomic commit. This prevents the same content from being written multiple times. In some cases (front-buffer rendering) there might be a desire for continuous operation - I think a property could be added later for this kind of control. Writeback can be disabled by setting FB_ID to zero. This seems to contradict itself: If it's one-shot, there's no need to disable it - it will auto-disable. I should have explained one-shot more clearly. What I mean is, one drmModeAtomicCommit == one write to memory. This is as opposed to writing the same thing to memory every vsync until it is stopped (which our HW is capable of doing). A subsequent drmModeAtomicCommit which doesn't touch the writeback FB_ID will write (again - but with whatever scene updates) to the same framebuffer. This continues for every drmModeAtomicCommit until FB_ID is set to zero - to disable writing - or changed to a different framebuffer, in which case we write to the new one. IMO this behaviour makes sense in the context of the rest of Atomic, and as the FB_ID is indeed persistent across atomic commits, I think it should be read-able. In other cases where we write a property as a one-shot thing (fences for android). In that case when you read that property it's always 0 (well, -1 for fences since file descriptor). That also avoids the issues when userspace unconditionally saves/restores all properties (this is needed for generic compositor switching). I think a better behaviour would be to do the same trick, with FB_ID on the connector always returning 0 as the current value. That encodes the one-shot behaviour directly. For one-shot vs continuous: Maybe we want to simply have a separate writeback property for continues, e.g. FB_WRITEBACK_ONE_SHOT_ID and FB_WRITEBACK_CONTINUOUS_ID. Known issues: - * I'm not sure what "DPMS" should mean for writeback connectors. It could be used to disable writeback (even when a framebuffer is attached), or it could be hidden entirely (which would break the legacy DPMS call for writeback connectors). dpms is legacy, in atomic land the only thing you have is "ACTIVE" on the crtc. it disables everything, i.e. also writeback. So removing the DPMS property is a viable option for writeback connectors in your opinion? * With Daniel's recent re-iteration of the userspace API rules, I fully expect to provide some userspace code to support this. The question is what, and where? We want to use writeback for testing, so perhaps some tests in igt is
Re: [RFC PATCH 00/11] Introduce writeback connectors
On Tue, Oct 11, 2016 at 03:53:57PM +0100, Brian Starkey wrote: > Hi, > > This RFC series introduces a new connector type: > DRM_MODE_CONNECTOR_WRITEBACK > It is a follow-on from a previous discussion: [1] > > Writeback connectors are used to expose the memory writeback engines > found in some display controllers, which can write a CRTC's > composition result to a memory buffer. > This is useful e.g. for testing, screen-recording, screenshots, > wireless display, display cloning, memory-to-memory composition. > > Patches 1-7 include the core framework changes required, and patches > 8-11 implement a writeback connector for the Mali-DP writeback engine. > The Mali-DP patches depend on this other series: [2]. > > The connector is given the FB_ID property for the output framebuffer, > and two new read-only properties: PIXEL_FORMATS and > PIXEL_FORMATS_SIZE, which expose the supported framebuffer pixel > formats of the engine. > > The EDID property is not exposed for writeback connectors. > > Writeback connector usage: > -- > Due to connector routing changes being treated as "full modeset" > operations, any client which wishes to use a writeback connector > should include the connector in every modeset. The writeback will not > actually become active until a framebuffer is attached. > > The writeback itself is enabled by attaching a framebuffer to the > FB_ID property of the connector. The driver must then ensure that the > CRTC content of that atomic commit is written into the framebuffer. > > The writeback works in a one-shot mode with each atomic commit. This > prevents the same content from being written multiple times. > In some cases (front-buffer rendering) there might be a desire for > continuous operation - I think a property could be added later for > this kind of control. I though people agreed that this sort of thing would go through v4l. Continously writing to the same buffer isn't perhaps all that sensible anyway, and so we'd need queueing, which is what v4l has already. Well, I guess we might add some queueing to atomic eventually? I guess for front buffer rendering type of thing you might have some use for a continuous mode targeting a single fb. Though I think peridically triggering a new write could do as well. Of course either way would likely tear horribly, and having multiple buffers seems like the better option. > > Writeback can be disabled by setting FB_ID to zero. > > Known issues: > - > * I'm not sure what "DPMS" should mean for writeback connectors. >It could be used to disable writeback (even when a framebuffer is >attached), or it could be hidden entirely (which would break the >legacy DPMS call for writeback connectors). > * With Daniel's recent re-iteration of the userspace API rules, I >fully expect to provide some userspace code to support this. The >question is what, and where? We want to use writeback for testing, >so perhaps some tests in igt is suitable. > * Documentation. Probably some portion of this cover letter needs to >make it into Documentation/ > * Synchronisation. Our hardware will finish the writeback by the next >vsync. I've not implemented fence support here, but it would be an >obvious addition. > > See Also: > - > [1] https://lists.freedesktop.org/archives/dri-devel/2016-July/113197.html > [2] https://lists.freedesktop.org/archives/dri-devel/2016-October/120486.html > > I welcome any comments, especially if this approach does/doesn't fit > well with anyone else's hardware. > > Thanks, > > -Brian > > --- > > Brian Starkey (10): > drm: add writeback connector type > drm/fb-helper: skip writeback connectors > drm: extract CRTC/plane disable from drm_framebuffer_remove > drm: add __drm_framebuffer_remove_atomic > drm: add fb to connector state > drm: expose fb_id property for writeback connectors > drm: add writeback-connector pixel format properties > drm: mali-dp: rename malidp_input_format > drm: mali-dp: add RGB writeback formats for DP550/DP650 > drm: mali-dp: add writeback connector > > Liviu Dudau (1): > drm: mali-dp: Add support for writeback on DP550/DP650 > > drivers/gpu/drm/arm/Makefile|1 + > drivers/gpu/drm/arm/malidp_crtc.c | 10 ++ > drivers/gpu/drm/arm/malidp_drv.c| 25 +++- > drivers/gpu/drm/arm/malidp_drv.h|5 + > drivers/gpu/drm/arm/malidp_hw.c | 104 ++ > drivers/gpu/drm/arm/malidp_hw.h | 27 +++- > drivers/gpu/drm/arm/malidp_mw.c | 268 > +++ > drivers/gpu/drm/arm/malidp_planes.c |8 +- > drivers/gpu/drm/arm/malidp_regs.h | 15 ++ > drivers/gpu/drm/drm_atomic.c| 40 ++ > drivers/gpu/drm/drm_atomic_helper.c |4 + > drivers/gpu/drm/drm_connector.c | 79 ++- > drivers/gpu/drm/drm_crtc.c | 14 +- > drivers/gpu/drm/drm_fb_helper.c |4 + > drivers/gpu/drm/drm_framebuffer.c
Re: [RFC PATCH 00/11] Introduce writeback connectors
On Tue, Oct 11, 2016 at 6:25 PM, Ville Syrjäläwrote: >> Writeback connector usage: >> -- >> Due to connector routing changes being treated as "full modeset" >> operations, any client which wishes to use a writeback connector >> should include the connector in every modeset. The writeback will not >> actually become active until a framebuffer is attached. >> >> The writeback itself is enabled by attaching a framebuffer to the >> FB_ID property of the connector. The driver must then ensure that the >> CRTC content of that atomic commit is written into the framebuffer. >> >> The writeback works in a one-shot mode with each atomic commit. This >> prevents the same content from being written multiple times. >> In some cases (front-buffer rendering) there might be a desire for >> continuous operation - I think a property could be added later for >> this kind of control. > > I though people agreed that this sort of thing would go through v4l. > Continously writing to the same buffer isn't perhaps all that sensible > anyway, and so we'd need queueing, which is what v4l has already. Well, > I guess we might add some queueing to atomic eventually? > > I guess for front buffer rendering type of thing you might have some > use for a continuous mode targeting a single fb. Though I think > peridically triggering a new write could do as well. Of course either > way would likely tear horribly, and having multiple buffers seems like > the better option Yeah, momentarily entirely forgot about v4l. I think making FB_ID one-shot (perhaps better to call it WRITEBACK_FB_ID to avoid confusion) is the right thing to do, and then push everything continuous to some form of drm/v4l integration. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 00/11] Introduce writeback connectors
On Tue, Oct 11, 2016 at 03:53:57PM +0100, Brian Starkey wrote: > Hi, > > This RFC series introduces a new connector type: > DRM_MODE_CONNECTOR_WRITEBACK > It is a follow-on from a previous discussion: [1] > > Writeback connectors are used to expose the memory writeback engines > found in some display controllers, which can write a CRTC's > composition result to a memory buffer. > This is useful e.g. for testing, screen-recording, screenshots, > wireless display, display cloning, memory-to-memory composition. > > Patches 1-7 include the core framework changes required, and patches > 8-11 implement a writeback connector for the Mali-DP writeback engine. > The Mali-DP patches depend on this other series: [2]. > > The connector is given the FB_ID property for the output framebuffer, > and two new read-only properties: PIXEL_FORMATS and > PIXEL_FORMATS_SIZE, which expose the supported framebuffer pixel > formats of the engine. > > The EDID property is not exposed for writeback connectors. > > Writeback connector usage: > -- > Due to connector routing changes being treated as "full modeset" > operations, any client which wishes to use a writeback connector > should include the connector in every modeset. The writeback will not > actually become active until a framebuffer is attached. Erhm, this is just the default, drivers can override this. And we could change the atomic helpers to not mark a modeset as a modeset if the connector that changed is a writeback one. > The writeback itself is enabled by attaching a framebuffer to the > FB_ID property of the connector. The driver must then ensure that the > CRTC content of that atomic commit is written into the framebuffer. > > The writeback works in a one-shot mode with each atomic commit. This > prevents the same content from being written multiple times. > In some cases (front-buffer rendering) there might be a desire for > continuous operation - I think a property could be added later for > this kind of control. > > Writeback can be disabled by setting FB_ID to zero. This seems to contradict itself: If it's one-shot, there's no need to disable it - it will auto-disable. In other cases where we write a property as a one-shot thing (fences for android). In that case when you read that property it's always 0 (well, -1 for fences since file descriptor). That also avoids the issues when userspace unconditionally saves/restores all properties (this is needed for generic compositor switching). I think a better behaviour would be to do the same trick, with FB_ID on the connector always returning 0 as the current value. That encodes the one-shot behaviour directly. For one-shot vs continuous: Maybe we want to simply have a separate writeback property for continues, e.g. FB_WRITEBACK_ONE_SHOT_ID and FB_WRITEBACK_CONTINUOUS_ID. > Known issues: > - > * I'm not sure what "DPMS" should mean for writeback connectors. >It could be used to disable writeback (even when a framebuffer is >attached), or it could be hidden entirely (which would break the >legacy DPMS call for writeback connectors). dpms is legacy, in atomic land the only thing you have is "ACTIVE" on the crtc. it disables everything, i.e. also writeback. > * With Daniel's recent re-iteration of the userspace API rules, I >fully expect to provide some userspace code to support this. The >question is what, and where? We want to use writeback for testing, >so perhaps some tests in igt is suitable. Hm, testing would be better as a debugfs interface, but I understand the appeal of doing this with atomic (since semantics fit so well). Another use-case of this is compositing, but if the main goal is igt and testing, I think integration into igt crc based testcases is a perfectly fine userspace. > * Documentation. Probably some portion of this cover letter needs to >make it into Documentation/ Yeah, an overview DOC: section in a separate source file (with all the the infrastructure work) would be great - aka needed from my pov ;-) > * Synchronisation. Our hardware will finish the writeback by the next >vsync. I've not implemented fence support here, but it would be an >obvious addition. Probably just want an additional WRITEBACK_FENCE_ID property to signal completion. Some hw definitely will take longer to write back than just a vblank. But we can delay that until it's needed. -Daniel > > See Also: > - > [1] https://lists.freedesktop.org/archives/dri-devel/2016-July/113197.html > [2] https://lists.freedesktop.org/archives/dri-devel/2016-October/120486.html > > I welcome any comments, especially if this approach does/doesn't fit > well with anyone else's hardware. > > Thanks, > > -Brian > > --- > > Brian Starkey (10): > drm: add writeback connector type > drm/fb-helper: skip writeback connectors > drm: extract CRTC/plane disable from drm_framebuffer_remove > drm: add __drm_framebuffer_remove_atomic >
[RFC PATCH 00/11] Introduce writeback connectors
Hi, This RFC series introduces a new connector type: DRM_MODE_CONNECTOR_WRITEBACK It is a follow-on from a previous discussion: [1] Writeback connectors are used to expose the memory writeback engines found in some display controllers, which can write a CRTC's composition result to a memory buffer. This is useful e.g. for testing, screen-recording, screenshots, wireless display, display cloning, memory-to-memory composition. Patches 1-7 include the core framework changes required, and patches 8-11 implement a writeback connector for the Mali-DP writeback engine. The Mali-DP patches depend on this other series: [2]. The connector is given the FB_ID property for the output framebuffer, and two new read-only properties: PIXEL_FORMATS and PIXEL_FORMATS_SIZE, which expose the supported framebuffer pixel formats of the engine. The EDID property is not exposed for writeback connectors. Writeback connector usage: -- Due to connector routing changes being treated as "full modeset" operations, any client which wishes to use a writeback connector should include the connector in every modeset. The writeback will not actually become active until a framebuffer is attached. The writeback itself is enabled by attaching a framebuffer to the FB_ID property of the connector. The driver must then ensure that the CRTC content of that atomic commit is written into the framebuffer. The writeback works in a one-shot mode with each atomic commit. This prevents the same content from being written multiple times. In some cases (front-buffer rendering) there might be a desire for continuous operation - I think a property could be added later for this kind of control. Writeback can be disabled by setting FB_ID to zero. Known issues: - * I'm not sure what "DPMS" should mean for writeback connectors. It could be used to disable writeback (even when a framebuffer is attached), or it could be hidden entirely (which would break the legacy DPMS call for writeback connectors). * With Daniel's recent re-iteration of the userspace API rules, I fully expect to provide some userspace code to support this. The question is what, and where? We want to use writeback for testing, so perhaps some tests in igt is suitable. * Documentation. Probably some portion of this cover letter needs to make it into Documentation/ * Synchronisation. Our hardware will finish the writeback by the next vsync. I've not implemented fence support here, but it would be an obvious addition. See Also: - [1] https://lists.freedesktop.org/archives/dri-devel/2016-July/113197.html [2] https://lists.freedesktop.org/archives/dri-devel/2016-October/120486.html I welcome any comments, especially if this approach does/doesn't fit well with anyone else's hardware. Thanks, -Brian --- Brian Starkey (10): drm: add writeback connector type drm/fb-helper: skip writeback connectors drm: extract CRTC/plane disable from drm_framebuffer_remove drm: add __drm_framebuffer_remove_atomic drm: add fb to connector state drm: expose fb_id property for writeback connectors drm: add writeback-connector pixel format properties drm: mali-dp: rename malidp_input_format drm: mali-dp: add RGB writeback formats for DP550/DP650 drm: mali-dp: add writeback connector Liviu Dudau (1): drm: mali-dp: Add support for writeback on DP550/DP650 drivers/gpu/drm/arm/Makefile|1 + drivers/gpu/drm/arm/malidp_crtc.c | 10 ++ drivers/gpu/drm/arm/malidp_drv.c| 25 +++- drivers/gpu/drm/arm/malidp_drv.h|5 + drivers/gpu/drm/arm/malidp_hw.c | 104 ++ drivers/gpu/drm/arm/malidp_hw.h | 27 +++- drivers/gpu/drm/arm/malidp_mw.c | 268 +++ drivers/gpu/drm/arm/malidp_planes.c |8 +- drivers/gpu/drm/arm/malidp_regs.h | 15 ++ drivers/gpu/drm/drm_atomic.c| 40 ++ drivers/gpu/drm/drm_atomic_helper.c |4 + drivers/gpu/drm/drm_connector.c | 79 ++- drivers/gpu/drm/drm_crtc.c | 14 +- drivers/gpu/drm/drm_fb_helper.c |4 + drivers/gpu/drm/drm_framebuffer.c | 249 drivers/gpu/drm/drm_ioctl.c |7 + include/drm/drmP.h |2 + include/drm/drm_atomic.h|3 + include/drm/drm_connector.h | 15 ++ include/drm/drm_crtc.h | 12 ++ include/uapi/drm/drm.h | 10 ++ include/uapi/drm/drm_mode.h |1 + 22 files changed, 830 insertions(+), 73 deletions(-) create mode 100644 drivers/gpu/drm/arm/malidp_mw.c -- 1.7.9.5 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html