Re: [PATCH 1/6] drm: Add writeback connector type
On Fri, May 05, 2017 at 10:22:19AM +0200, Boris Brezillon wrote: > On Fri, 25 Nov 2016 16:48:59 + > Brian Starkeywrote: > > > +/** > > + * drm_writeback_connector_init - Initialize a writeback connector and its > > properties > > + * @dev: DRM device > > + * @wb_connector: Writeback connector to initialize > > + * @funcs: Connector funcs vtable > > + * @formats: Array of supported pixel formats for the writeback engine > > + * @n_formats: Length of the formats array > > + * > > + * This function creates the writeback-connector-specific properties if > > they > > + * have not been already created, initializes the connector as > > + * type DRM_MODE_CONNECTOR_WRITEBACK, and correctly initializes the > > property > > + * values. > > + * > > + * Drivers should always use this function instead of drm_connector_init() > > to > > + * set up writeback connectors. > > + * > > + * Returns: 0 on success, or a negative error code > > + */ > > +int drm_writeback_connector_init(struct drm_device *dev, > > +struct drm_writeback_connector *wb_connector, > > +const struct drm_connector_funcs *funcs, > > +u32 *formats, int n_formats) > > This should probably be 'const u32 *formats', since developers are > likely to define a this array with a 'static const' specifier in their > driver. Fixed in the v4. Thanks, Liviu -- | I would like to | | fix the world, | | but they're not | | giving me the | \ source code! / --- ¯\_(ツ)_/¯
Re: [PATCH 1/6] drm: Add writeback connector type
On Fri, 25 Nov 2016 16:48:59 + Brian Starkeywrote: > +/** > + * drm_writeback_connector_init - Initialize a writeback connector and its > properties > + * @dev: DRM device > + * @wb_connector: Writeback connector to initialize > + * @funcs: Connector funcs vtable > + * @formats: Array of supported pixel formats for the writeback engine > + * @n_formats: Length of the formats array > + * > + * This function creates the writeback-connector-specific properties if they > + * have not been already created, initializes the connector as > + * type DRM_MODE_CONNECTOR_WRITEBACK, and correctly initializes the property > + * values. > + * > + * Drivers should always use this function instead of drm_connector_init() to > + * set up writeback connectors. > + * > + * Returns: 0 on success, or a negative error code > + */ > +int drm_writeback_connector_init(struct drm_device *dev, > + struct drm_writeback_connector *wb_connector, > + const struct drm_connector_funcs *funcs, > + u32 *formats, int n_formats) This should probably be 'const u32 *formats', since developers are likely to define a this array with a 'static const' specifier in their driver.
Re: [PATCH 1/6] drm: Add writeback connector type
On Wed, Apr 19, 2017 at 01:34:34PM +0200, Boris Brezillon wrote: On Wed, 19 Apr 2017 10:51:23 +0100 Brian Starkeywrote: [snip] Could you expand a bit on how you think planes fit better? Just had the impression that the writeback feature was conceptually closer to a plane object (which is attached buffers and expose ways to specify which portion of the buffer should be used, provides way to atomically switch 2 buffers, ...). Yeah sort-of, except that SRC_X/Y/W/H doesn't mean the same for an "output" plane as an "input" plane (and CRTC_X/Y/W/H similarly, probably other properties too). In atomic land, the swapping of buffers is really just the swapping of object IDs via properties - I don't think planes actually have anything special in terms of buffer handling, except for all the legacy state handling cruft. It is something we've previously talked about internally, but so far I'm not convinced :-) Okay, as I said, I don't know all the history, hence my questions ;-). I think that history was here in our office rather than on the list anyway. >By doing that, we would also get rid of these fake connector/encoder >objects as well as the fake modes we are returning in >connector->get_modes(). What makes the connector/encoder fake? They represent a real piece of hardware just the same as a drm_plane would. Well, that's probably subject to interpretation, but I don't consider these writeback encoders/connectors as real encoders/connectors. They are definitely real HW blocks, but not what we usually consider as an encoder/connector. This is true I don't mind dropping the mode list and letting userspace just make up whatever timing it wants - it'd need to do the same if writeback was a plane - but in some respects giving it a list of modes the same way we normally do seems nicer for userspace. > >As far as I can tell, the VC4 and Atmel HLCDC IP should fit pretty well >in this model, not sure about the mali-dp though. > >Brian, did you consider this approach, and if you did, can you detail >why you decided to expose this feature as a connector? > >Daniel (or anyone else), please step in if you think this is a stupid >idea :-). Ville originally suggested using a connector, which Eric followed up by saying that's what he was thinking of for VC4 writeback too[1]. Thanks for the pointer. That was my initial reason for focussing on a connector-based approach. I prefer connector over plane conceptually because it keeps with the established data flow: planes are sources, connectors are sinks. Okay, it's a valid point. In some respects the plane _object_ looks like it would fit - it has a pixel format list and an FB_ID. But everything else would be acting the exact opposite to a normal plane, and I think there's a bunch of baked-in assumptions in the kernel and userspace around CRTCs always having at least one connector. Yep, but writeback connectors are already different enough to not be considered as regular connectors, so userspace programs will have to handle them differently anyway (pass a framebuffer and pixel format to it before adding them to the display pipeline). Anyway, I see this approach has already been suggested in [1], and you all agreed that the writeback feature should be exposed as a connector, so I'll just stop here :-). Thanks for taking the time to explain the rationale behind this decision. No problem, now is the right time to be discussing the decision before we merge something wrong. Are you happy enough with the connector approach then? Any concerns with going ahead with it? Cheers, -Brian
Re: [PATCH 1/6] drm: Add writeback connector type
On Wed, 19 Apr 2017 10:51:23 +0100 Brian Starkeywrote: > On Tue, Apr 18, 2017 at 09:57:17PM +0200, Boris Brezillon wrote: > >Hi Brian, > > > >On Tue, 18 Apr 2017 18:34:43 +0100 > >Brian Starkey wrote: > > > >> >> @@ -214,6 +214,19 @@ struct drm_connector_state { > >> >> struct drm_encoder *best_encoder; > >> >> > >> >> struct drm_atomic_state *state; > >> >> + > >> >> + /** > >> >> +* @writeback_job: Writeback job for writeback connectors > >> >> +* > >> >> +* Holds the framebuffer for a writeback connector. As the > >> >> writeback > >> >> +* completion may be asynchronous to the normal commit cycle, > >> >> the > >> >> +* writeback job lifetime is managed separately from the normal > >> >> atomic > >> >> +* state by this object. > >> >> +* > >> >> +* See also: drm_writeback_queue_job() and > >> >> +* drm_writeback_signal_completion() > >> >> +*/ > >> >> + struct drm_writeback_job *writeback_job; > >> > > >> >Maybe I'm wrong, but is feels weird to have the writeback_job field > >> >directly embedded in drm_connector_state, while drm_writeback_connector > >> >inherits from drm_connector. > >> > > >> >IMO, either you decide to directly put the drm_writeback_connector's > >> >job_xxx fields in drm_connector and keep the drm_connector_state as is, > >> >or you create a drm_writeback_connector_state which inherits from > >> >drm_connector_state and embeds the writeback_job field. > >> > >> I did spend a decent amount of time looking at tracking the writeback > >> state along with the normal connector state. I couldn't come up with > >> anything I liked. > >> > >> As the comment mentions, one of the problems is that you have to make > >> sure the relevant parts of the connector_state stay around until the > >> writeback is finished. That means you've got to block before > >> "swap_state()" until the previous writeback is done, and that > >> effectively limits your frame rate to refresh/2. > >> > >> The Mali-DP HW doesn't have that limitation - we can queue up a new > >> commit while the current writeback is ongoing. For that reason I > >> didn't want to impose such a limitation in the framework. > >> > >> In v1 I allowed that by making the Mali-DP driver hold its own > >> references to the relevant bits of the state for as long as it needed > >> them. In v3 I moved most of that code back to the core (in part > >> because Gustavo didn't like me signalling the DRM-"owned" fence from > >> my driver code directly). I think the new approach of "queue_job()" > >> and "signal_job()" reduces the amount of tricky code in drivers, and > >> is generally more clear (also familiar, when compared to vsync > >> events). > >> > >> I'm certain there's other ways to do it (refcount atomic states?), but > >> it seemed like a biggish overhaul to achieve what would basically be > >> the same thing. > >> > >> I was expecting each driver supporting writeback to have its own > >> different requirements around writeback lifetime/duration. For example > >> I think VC4 specifically came up, in that its writeback could take > >> several frames, whereas on Mali-DP we either finish within the frame > >> or we fail. > >> > >> Letting the driver manage its writeback_job lifetime seemed like a > >> reasonable way to handle all that, with the documentation stating the > >> only behaviour which is guaranteed to work on all drivers: > >> > >>* Userspace should wait for this fence to signal before making > >> another > >>* commit affecting any of the same CRTCs, Planes or Connectors. > >>* **Failure to do so will result in undefined behaviour.** > >>* For this reason it is strongly recommended that all userspace > >>* applications making use of writeback connectors *always* retrieve > >> an > >>* out-fence for the commit and use it appropriately. > >> > >> > >> > >> ... so all of that is why the _job fields don't live in a *_state > >> structure directly, and instead have to live in the separately-managed > >> structure pointed to by ->writeback_job. > >> > >> Now, I did look at creating drm_writeback_connector_state, but as it > >> would only be holding the job pointer (see above) it didn't seem worth > >> scattering around the > >> > >> if (conn_state->connector->connector_type == > >> DRM_MODE_CONNECTOR_WRITEBACK) > >> > >> checks everywhere before up-casting - {clear,reset,duplicate}_state(), > >> prepare_signalling(), complete_signalling(), etc. It just touched a > >> lot of code for the sake of an extra pointer field in each connector > >> state. > >> > >> I can easily revisit that part if you like. > > > >I think there's a misunderstanding. I was just suggesting to be > >consistent in the inheritance vs 'one object to handle everything' > >approach. > > doh.. right yeah I misread. Sorry for the tangent. :-) >
Re: [PATCH 1/6] drm: Add writeback connector type
On Tue, Apr 18, 2017 at 09:57:17PM +0200, Boris Brezillon wrote: Hi Brian, On Tue, 18 Apr 2017 18:34:43 +0100 Brian Starkeywrote: >> @@ -214,6 +214,19 @@ struct drm_connector_state { >>struct drm_encoder *best_encoder; >> >>struct drm_atomic_state *state; >> + >> + /** >> + * @writeback_job: Writeback job for writeback connectors >> + * >> + * Holds the framebuffer for a writeback connector. As the writeback >> + * completion may be asynchronous to the normal commit cycle, the >> + * writeback job lifetime is managed separately from the normal atomic >> + * state by this object. >> + * >> + * See also: drm_writeback_queue_job() and >> + * drm_writeback_signal_completion() >> + */ >> + struct drm_writeback_job *writeback_job; > >Maybe I'm wrong, but is feels weird to have the writeback_job field >directly embedded in drm_connector_state, while drm_writeback_connector >inherits from drm_connector. > >IMO, either you decide to directly put the drm_writeback_connector's >job_xxx fields in drm_connector and keep the drm_connector_state as is, >or you create a drm_writeback_connector_state which inherits from >drm_connector_state and embeds the writeback_job field. I did spend a decent amount of time looking at tracking the writeback state along with the normal connector state. I couldn't come up with anything I liked. As the comment mentions, one of the problems is that you have to make sure the relevant parts of the connector_state stay around until the writeback is finished. That means you've got to block before "swap_state()" until the previous writeback is done, and that effectively limits your frame rate to refresh/2. The Mali-DP HW doesn't have that limitation - we can queue up a new commit while the current writeback is ongoing. For that reason I didn't want to impose such a limitation in the framework. In v1 I allowed that by making the Mali-DP driver hold its own references to the relevant bits of the state for as long as it needed them. In v3 I moved most of that code back to the core (in part because Gustavo didn't like me signalling the DRM-"owned" fence from my driver code directly). I think the new approach of "queue_job()" and "signal_job()" reduces the amount of tricky code in drivers, and is generally more clear (also familiar, when compared to vsync events). I'm certain there's other ways to do it (refcount atomic states?), but it seemed like a biggish overhaul to achieve what would basically be the same thing. I was expecting each driver supporting writeback to have its own different requirements around writeback lifetime/duration. For example I think VC4 specifically came up, in that its writeback could take several frames, whereas on Mali-DP we either finish within the frame or we fail. Letting the driver manage its writeback_job lifetime seemed like a reasonable way to handle all that, with the documentation stating the only behaviour which is guaranteed to work on all drivers: * Userspace should wait for this fence to signal before making another * commit affecting any of the same CRTCs, Planes or Connectors. * **Failure to do so will result in undefined behaviour.** * For this reason it is strongly recommended that all userspace * applications making use of writeback connectors *always* retrieve an * out-fence for the commit and use it appropriately. ... so all of that is why the _job fields don't live in a *_state structure directly, and instead have to live in the separately-managed structure pointed to by ->writeback_job. Now, I did look at creating drm_writeback_connector_state, but as it would only be holding the job pointer (see above) it didn't seem worth scattering around the if (conn_state->connector->connector_type == DRM_MODE_CONNECTOR_WRITEBACK) checks everywhere before up-casting - {clear,reset,duplicate}_state(), prepare_signalling(), complete_signalling(), etc. It just touched a lot of code for the sake of an extra pointer field in each connector state. I can easily revisit that part if you like. I think there's a misunderstanding. I was just suggesting to be consistent in the inheritance vs 'one object to handle everything' approach. doh.. right yeah I misread. Sorry for the tangent. :-) I'm perfectly fine with embedding the writeback_job pointer directly in drm_connector_state, but then it would IMO make more sense to do the same for the drm_connector object (embed drm_writeback_connector fields into drm_connector instead of making drm_writeback_connector inherit from drm_connector). I agree that it's inconsistent. I guess I did it out of pragmatism - there's quite a lot of new fields in drm_writeback_connector, and the code needed to support it was comparatively small. On the other hand there's only one additional field in drm_connector_state and the code required to subclass
Re: [PATCH 1/6] drm: Add writeback connector type
Hi Brian, On Tue, 18 Apr 2017 18:34:43 +0100 Brian Starkeywrote: > >> @@ -214,6 +214,19 @@ struct drm_connector_state { > >>struct drm_encoder *best_encoder; > >> > >>struct drm_atomic_state *state; > >> + > >> + /** > >> + * @writeback_job: Writeback job for writeback connectors > >> + * > >> + * Holds the framebuffer for a writeback connector. As the writeback > >> + * completion may be asynchronous to the normal commit cycle, the > >> + * writeback job lifetime is managed separately from the normal atomic > >> + * state by this object. > >> + * > >> + * See also: drm_writeback_queue_job() and > >> + * drm_writeback_signal_completion() > >> + */ > >> + struct drm_writeback_job *writeback_job; > > > >Maybe I'm wrong, but is feels weird to have the writeback_job field > >directly embedded in drm_connector_state, while drm_writeback_connector > >inherits from drm_connector. > > > >IMO, either you decide to directly put the drm_writeback_connector's > >job_xxx fields in drm_connector and keep the drm_connector_state as is, > >or you create a drm_writeback_connector_state which inherits from > >drm_connector_state and embeds the writeback_job field. > > I did spend a decent amount of time looking at tracking the writeback > state along with the normal connector state. I couldn't come up with > anything I liked. > > As the comment mentions, one of the problems is that you have to make > sure the relevant parts of the connector_state stay around until the > writeback is finished. That means you've got to block before > "swap_state()" until the previous writeback is done, and that > effectively limits your frame rate to refresh/2. > > The Mali-DP HW doesn't have that limitation - we can queue up a new > commit while the current writeback is ongoing. For that reason I > didn't want to impose such a limitation in the framework. > > In v1 I allowed that by making the Mali-DP driver hold its own > references to the relevant bits of the state for as long as it needed > them. In v3 I moved most of that code back to the core (in part > because Gustavo didn't like me signalling the DRM-"owned" fence from > my driver code directly). I think the new approach of "queue_job()" > and "signal_job()" reduces the amount of tricky code in drivers, and > is generally more clear (also familiar, when compared to vsync > events). > > I'm certain there's other ways to do it (refcount atomic states?), but > it seemed like a biggish overhaul to achieve what would basically be > the same thing. > > I was expecting each driver supporting writeback to have its own > different requirements around writeback lifetime/duration. For example > I think VC4 specifically came up, in that its writeback could take > several frames, whereas on Mali-DP we either finish within the frame > or we fail. > > Letting the driver manage its writeback_job lifetime seemed like a > reasonable way to handle all that, with the documentation stating the > only behaviour which is guaranteed to work on all drivers: > >* Userspace should wait for this fence to signal before making another >* commit affecting any of the same CRTCs, Planes or Connectors. >* **Failure to do so will result in undefined behaviour.** >* For this reason it is strongly recommended that all userspace >* applications making use of writeback connectors *always* retrieve an >* out-fence for the commit and use it appropriately. > > > > ... so all of that is why the _job fields don't live in a *_state > structure directly, and instead have to live in the separately-managed > structure pointed to by ->writeback_job. > > Now, I did look at creating drm_writeback_connector_state, but as it > would only be holding the job pointer (see above) it didn't seem worth > scattering around the > > if (conn_state->connector->connector_type == > DRM_MODE_CONNECTOR_WRITEBACK) > > checks everywhere before up-casting - {clear,reset,duplicate}_state(), > prepare_signalling(), complete_signalling(), etc. It just touched a > lot of code for the sake of an extra pointer field in each connector > state. > > I can easily revisit that part if you like. I think there's a misunderstanding. I was just suggesting to be consistent in the inheritance vs 'one object to handle everything' approach. I'm perfectly fine with embedding the writeback_job pointer directly in drm_connector_state, but then it would IMO make more sense to do the same for the drm_connector object (embed drm_writeback_connector fields into drm_connector instead of making drm_writeback_connector inherit from drm_connector). Anyway, that's just a detail. > > > > >Anyway, wait for Daniel's feedback before doing this change. > > > > Am I expecting some more feedback from Daniel? No, I was just saying that before doing the changes I was suggesting, you should wait for Daniel's opinion, because I might be wrong. > >
Re: [PATCH 1/6] drm: Add writeback connector type
Hi Boris, On Fri, Apr 14, 2017 at 12:08:23PM +0200, Boris Brezillon wrote: On Fri, 25 Nov 2016 16:48:59 + Brian Starkeywrote: diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c index b5c6a8e..6bbd93f 100644 --- a/drivers/gpu/drm/drm_connector.c +++ b/drivers/gpu/drm/drm_connector.c @@ -86,6 +86,7 @@ struct drm_conn_prop_enum_list { { DRM_MODE_CONNECTOR_VIRTUAL, "Virtual" }, { DRM_MODE_CONNECTOR_DSI, "DSI" }, { DRM_MODE_CONNECTOR_DPI, "DPI" }, + { DRM_MODE_CONNECTOR_WRITEBACK, "Writeback" }, Is there a reason we have a Writeback connector, but keep using a Virtual encoder to connect it to the CRTC? Wouldn't it make more sense to also add a Writeback encoder? Only that a writeback connector is functionally and conceptually quite different from the existing connector types, whereas the "encoder" (which realistically only exists because the framework forces it to) acts pretty much like any other. }; void drm_connector_ida_init(void) @@ -235,7 +236,8 @@ int drm_connector_init(struct drm_device *dev, list_add_tail(>head, >connector_list); config->num_connector++; - if (connector_type != DRM_MODE_CONNECTOR_VIRTUAL) + if ((connector_type != DRM_MODE_CONNECTOR_VIRTUAL) && + (connector_type != DRM_MODE_CONNECTOR_WRITEBACK)) Nitpick: you don't need the extra parenthesis: if (connector_type != DRM_MODE_CONNECTOR_VIRTUAL && connector_type != DRM_MODE_CONNECTOR_WRITEBACK) Yeah fair enough, I can drop them. drm_object_attach_property(>base, config->edid_property, 0); diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h index 34f9741..dc4910d6 100644 --- a/include/drm/drm_connector.h +++ b/include/drm/drm_connector.h @@ -214,6 +214,19 @@ struct drm_connector_state { struct drm_encoder *best_encoder; struct drm_atomic_state *state; + + /** +* @writeback_job: Writeback job for writeback connectors +* +* Holds the framebuffer for a writeback connector. As the writeback +* completion may be asynchronous to the normal commit cycle, the +* writeback job lifetime is managed separately from the normal atomic +* state by this object. +* +* See also: drm_writeback_queue_job() and +* drm_writeback_signal_completion() +*/ + struct drm_writeback_job *writeback_job; Maybe I'm wrong, but is feels weird to have the writeback_job field directly embedded in drm_connector_state, while drm_writeback_connector inherits from drm_connector. IMO, either you decide to directly put the drm_writeback_connector's job_xxx fields in drm_connector and keep the drm_connector_state as is, or you create a drm_writeback_connector_state which inherits from drm_connector_state and embeds the writeback_job field. I did spend a decent amount of time looking at tracking the writeback state along with the normal connector state. I couldn't come up with anything I liked. As the comment mentions, one of the problems is that you have to make sure the relevant parts of the connector_state stay around until the writeback is finished. That means you've got to block before "swap_state()" until the previous writeback is done, and that effectively limits your frame rate to refresh/2. The Mali-DP HW doesn't have that limitation - we can queue up a new commit while the current writeback is ongoing. For that reason I didn't want to impose such a limitation in the framework. In v1 I allowed that by making the Mali-DP driver hold its own references to the relevant bits of the state for as long as it needed them. In v3 I moved most of that code back to the core (in part because Gustavo didn't like me signalling the DRM-"owned" fence from my driver code directly). I think the new approach of "queue_job()" and "signal_job()" reduces the amount of tricky code in drivers, and is generally more clear (also familiar, when compared to vsync events). I'm certain there's other ways to do it (refcount atomic states?), but it seemed like a biggish overhaul to achieve what would basically be the same thing. I was expecting each driver supporting writeback to have its own different requirements around writeback lifetime/duration. For example I think VC4 specifically came up, in that its writeback could take several frames, whereas on Mali-DP we either finish within the frame or we fail. Letting the driver manage its writeback_job lifetime seemed like a reasonable way to handle all that, with the documentation stating the only behaviour which is guaranteed to work on all drivers: * Userspace should wait for this fence to signal before making another * commit affecting any of the same CRTCs, Planes or Connectors. * **Failure to do so will result in undefined
Re: [PATCH 1/6] drm: Add writeback connector type
On Fri, 25 Nov 2016 16:48:59 + Brian Starkeywrote: > > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c > index b5c6a8e..6bbd93f 100644 > --- a/drivers/gpu/drm/drm_connector.c > +++ b/drivers/gpu/drm/drm_connector.c > @@ -86,6 +86,7 @@ struct drm_conn_prop_enum_list { > { DRM_MODE_CONNECTOR_VIRTUAL, "Virtual" }, > { DRM_MODE_CONNECTOR_DSI, "DSI" }, > { DRM_MODE_CONNECTOR_DPI, "DPI" }, > + { DRM_MODE_CONNECTOR_WRITEBACK, "Writeback" }, Is there a reason we have a Writeback connector, but keep using a Virtual encoder to connect it to the CRTC? Wouldn't it make more sense to also add a Writeback encoder? > }; > > void drm_connector_ida_init(void) > @@ -235,7 +236,8 @@ int drm_connector_init(struct drm_device *dev, > list_add_tail(>head, >connector_list); > config->num_connector++; > > - if (connector_type != DRM_MODE_CONNECTOR_VIRTUAL) > + if ((connector_type != DRM_MODE_CONNECTOR_VIRTUAL) && > + (connector_type != DRM_MODE_CONNECTOR_WRITEBACK)) Nitpick: you don't need the extra parenthesis: if (connector_type != DRM_MODE_CONNECTOR_VIRTUAL && connector_type != DRM_MODE_CONNECTOR_WRITEBACK) > drm_object_attach_property(>base, > config->edid_property, > 0); > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h > index 34f9741..dc4910d6 100644 > --- a/include/drm/drm_connector.h > +++ b/include/drm/drm_connector.h > @@ -214,6 +214,19 @@ struct drm_connector_state { > struct drm_encoder *best_encoder; > > struct drm_atomic_state *state; > + > + /** > + * @writeback_job: Writeback job for writeback connectors > + * > + * Holds the framebuffer for a writeback connector. As the writeback > + * completion may be asynchronous to the normal commit cycle, the > + * writeback job lifetime is managed separately from the normal atomic > + * state by this object. > + * > + * See also: drm_writeback_queue_job() and > + * drm_writeback_signal_completion() > + */ > + struct drm_writeback_job *writeback_job; Maybe I'm wrong, but is feels weird to have the writeback_job field directly embedded in drm_connector_state, while drm_writeback_connector inherits from drm_connector. IMO, either you decide to directly put the drm_writeback_connector's job_xxx fields in drm_connector and keep the drm_connector_state as is, or you create a drm_writeback_connector_state which inherits from drm_connector_state and embeds the writeback_job field. Anyway, wait for Daniel's feedback before doing this change. > }; > > /** > diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h > index bf9991b2..3d3d07f 100644 > --- a/include/drm/drm_mode_config.h > +++ b/include/drm/drm_mode_config.h > @@ -634,6 +634,20 @@ struct drm_mode_config { >*/ > struct drm_property *suggested_y_property; > > + /** > + * @writeback_fb_id_property: Property for writeback connectors, storing > + * the ID of the output framebuffer. > + * See also: drm_writeback_connector_init() > + */ > + struct drm_property *writeback_fb_id_property; > + /** > + * @writeback_pixel_formats_property: Property for writeback connectors, > + * storing an array of the supported pixel formats for the writeback > + * engine (read-only). > + * See also: drm_writeback_connector_init() > + */ > + struct drm_property *writeback_pixel_formats_property; > + > /* dumb ioctl parameters */ > uint32_t preferred_depth, prefer_shadow; > > diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h > new file mode 100644 > index 000..6b2ac45 > --- /dev/null > +++ b/include/drm/drm_writeback.h > @@ -0,0 +1,78 @@ > +/* > + * (C) COPYRIGHT 2016 ARM Limited. All rights reserved. > + * Author: Brian Starkey > + * > + * This program is free software and is provided to you under the terms of > the > + * GNU General Public License version 2 as published by the Free Software > + * Foundation, and any use by you of this program is subject to the terms > + * of such GNU licence. > + */ > + > +#ifndef __DRM_WRITEBACK_H__ > +#define __DRM_WRITEBACK_H__ > +#include > +#include > + > +struct drm_writeback_connector { > + struct drm_connector base; AFAIU, a writeback connector will always require an 'dummy' encoder to make the DRM framework happy (AFAIK, a connector is always connected to a CRTC through an encoder). Wouldn't it make more sense to have a drm_encoder object embedded in drm_writeback_connector so that people don't have to declare an extra structure containing both the drm_writeback_connector connector and a drm_encoder? Is there a good reason to keep them separate? > + > + /** > + *
[PATCH 1/6] drm: Add writeback connector type
Writeback connectors represent writeback engines which can write the CRTC output to a memory framebuffer. Add a writeback connector type and related support functions. Drivers should initialize a writeback connector with drm_writeback_connector_init() which takes care of setting up all the writeback-specific details on top of the normal functionality of drm_connector_init(). Writeback connectors have a WRITEBACK_FB_ID property, used to set the output framebuffer, and a PIXEL_FORMATS blob used to expose the supported writeback formats to userspace. When a framebuffer is attached to a writeback connector with the WRITEBACK_FB_ID property, it is used only once (for the commit in which it was included), and userspace can never read back the value of WRITEBACK_FB_ID. WRITEBACK_FB_ID can only be set if the connector is attached to a CRTC. Changes since v1: - Added drm_writeback.c + documentation - Added helper to initialize writeback connector in one go - Added core checks - Squashed into a single commit - Dropped the client cap - Writeback framebuffers are no longer persistent Changes since v2: Daniel Vetter: - Subclass drm_connector to drm_writeback_connector - Relax check to allow CRTC to be set without an FB - Add some writeback_ prefixes - Drop PIXEL_FORMATS_SIZE property, as it was unnecessary Gustavo Padovan: - Add drm_writeback_job to handle writeback signalling centrally Signed-off-by: Brian Starkey--- Documentation/gpu/drm-kms.rst |9 ++ drivers/gpu/drm/Makefile|2 +- drivers/gpu/drm/drm_atomic.c| 130 drivers/gpu/drm/drm_atomic_helper.c |6 + drivers/gpu/drm/drm_connector.c |4 +- drivers/gpu/drm/drm_writeback.c | 230 +++ include/drm/drm_atomic.h|3 + include/drm/drm_connector.h | 13 ++ include/drm/drm_mode_config.h | 14 +++ include/drm/drm_writeback.h | 78 include/uapi/drm/drm_mode.h |1 + 11 files changed, 488 insertions(+), 2 deletions(-) create mode 100644 drivers/gpu/drm/drm_writeback.c create mode 100644 include/drm/drm_writeback.h diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst index 568f3c2..3a4f35b 100644 --- a/Documentation/gpu/drm-kms.rst +++ b/Documentation/gpu/drm-kms.rst @@ -122,6 +122,15 @@ Connector Functions Reference .. kernel-doc:: drivers/gpu/drm/drm_connector.c :export: +Writeback Connectors + + +.. kernel-doc:: drivers/gpu/drm/drm_writeback.c + :doc: overview + +.. kernel-doc:: drivers/gpu/drm/drm_writeback.c + :export: + Encoder Abstraction === diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile index 883f3e7..3209aa4 100644 --- a/drivers/gpu/drm/Makefile +++ b/drivers/gpu/drm/Makefile @@ -16,7 +16,7 @@ drm-y :=drm_auth.o drm_bufs.o drm_cache.o \ drm_framebuffer.o drm_connector.o drm_blend.o \ drm_encoder.o drm_mode_object.o drm_property.o \ drm_plane.o drm_color_mgmt.o drm_print.o \ - drm_dumb_buffers.o drm_mode_config.o + drm_dumb_buffers.o drm_mode_config.o drm_writeback.o drm-$(CONFIG_COMPAT) += drm_ioc32.o drm-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_gem_cma_helper.o diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index b476ec5..343e2b7 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -31,6 +31,7 @@ #include #include #include +#include #include #include "drm_crtc_internal.h" @@ -659,6 +660,46 @@ static void drm_atomic_crtc_print_state(struct drm_printer *p, } /** + * drm_atomic_connector_check - check connector state + * @connector: connector to check + * @state: connector state to check + * + * Provides core sanity checks for connector state. + * + * RETURNS: + * Zero on success, error code on failure + */ +static int drm_atomic_connector_check(struct drm_connector *connector, + struct drm_connector_state *state) +{ + struct drm_crtc_state *crtc_state; + struct drm_writeback_job *writeback_job = state->writeback_job; + + if ((connector->connector_type != DRM_MODE_CONNECTOR_WRITEBACK) || + !writeback_job) + return 0; + + if (writeback_job->fb && !state->crtc) { + DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] framebuffer without CRTC\n", +connector->base.id, connector->name); + return -EINVAL; + } + + if (state->crtc) + crtc_state = drm_atomic_get_existing_crtc_state(state->state, + state->crtc); + + if (writeback_job->fb && !crtc_state->active) { + DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] has framebuffer, but [CRTC:%d] is off\n", +connector->base.id,