| Message ID | 1480092544-1725-2-git-send-email-brian.starkey@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 25 Nov 2016 16:48:59 +0000 Brian Starkey <brian.starkey@arm.com> wrote: > > 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(&connector->head, &config->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(&connector->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 0000000..6b2ac45 > --- /dev/null > +++ b/include/drm/drm_writeback.h > @@ -0,0 +1,78 @@ > +/* > + * (C) COPYRIGHT 2016 ARM Limited. All rights reserved. > + * Author: Brian Starkey <brian.starkey@arm.com> > + * > + * 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 <drm/drm_connector.h> > +#include <linux/workqueue.h> > + > +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? > + > + /** > + * @pixel_formats_blob_ptr: > + * > + * DRM blob property data for the pixel formats list on writeback > + * connectors > + * See also drm_writeback_connector_init() > + */ > + struct drm_property_blob *pixel_formats_blob_ptr; > + > + /** @job_lock: Protects job_queue */ > + spinlock_t job_lock; > + /** > + * @job_queue: > + * > + * Holds a list of a connector's writeback jobs; the last item is the > + * most recent. The first item may be either waiting for the hardware > + * to begin writing, or currently being written. > + * > + * See also: drm_writeback_queue_job() and > + * drm_writeback_signal_completion() > + */ > + struct list_head job_queue; > +};
Hi Boris, On Fri, Apr 14, 2017 at 12:08:23PM +0200, Boris Brezillon wrote: >On Fri, 25 Nov 2016 16:48:59 +0000 >Brian Starkey <brian.starkey@arm.com> wrote: > > >> >> 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(&connector->head, &config->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(&connector->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 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. > >Anyway, wait for Daniel's feedback before doing this change. > Am I expecting some more feedback from Daniel? >> }; >> >> /** >> 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 0000000..6b2ac45 >> --- /dev/null >> +++ b/include/drm/drm_writeback.h >> @@ -0,0 +1,78 @@ >> +/* >> + * (C) COPYRIGHT 2016 ARM Limited. All rights reserved. >> + * Author: Brian Starkey <brian.starkey@arm.com> >> + * >> + * 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 <drm/drm_connector.h> >> +#include <linux/workqueue.h> >> + >> +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? > Yeah that's not a bad idea. The encoder funcs could be passed in to drm_writeback_connector_init() (in which case adding a writeback encoder type would also make sense). Thanks for the review, -Brian >> + >> + /** >> + * @pixel_formats_blob_ptr: >> + * >> + * DRM blob property data for the pixel formats list on writeback >> + * connectors >> + * See also drm_writeback_connector_init() >> + */ >> + struct drm_property_blob *pixel_formats_blob_ptr; >> + >> + /** @job_lock: Protects job_queue */ >> + spinlock_t job_lock; >> + /** >> + * @job_queue: >> + * >> + * Holds a list of a connector's writeback jobs; the last item is the >> + * most recent. The first item may be either waiting for the hardware >> + * to begin writing, or currently being written. >> + * >> + * See also: drm_writeback_queue_job() and >> + * drm_writeback_signal_completion() >> + */ >> + struct list_head job_queue; >> +};
Hi Brian, On Tue, 18 Apr 2017 18:34:43 +0100 Brian Starkey <brian.starkey@arm.com> 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. 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. > > >> }; > >> > >> /** > >> 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 0000000..6b2ac45 > >> --- /dev/null > >> +++ b/include/drm/drm_writeback.h > >> @@ -0,0 +1,78 @@ > >> +/* > >> + * (C) COPYRIGHT 2016 ARM Limited. All rights reserved. > >> + * Author: Brian Starkey <brian.starkey@arm.com> > >> + * > >> + * 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 <drm/drm_connector.h> > >> +#include <linux/workqueue.h> > >> + > >> +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? > > > > Yeah that's not a bad idea. The encoder funcs could be passed in to > drm_writeback_connector_init() (in which case adding a writeback > encoder type would also make sense). Well, the more I look at it the more I find it weird to represent this writeback feature as a connector. To me, it seems to be closer to a plane object (DRM_PLANE_TYPE_WRITEBACK?) than a real connector. Actually, the Atmel HLCDC IP use the same register interface to expose the overlay planes and writeback features. Of course, representing it as a plane requires patching the core to allow enabling a CRTC that has no active connectors connected to it if at least one writeback plane is enabled and connected to the CRTC, but it should be doable. 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(). 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 :-).
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 <brian.starkey@arm.com> 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. :-) > >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 it looked comparatively large. >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. > >> >> >> }; >> >> >> >> /** >> >> 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 0000000..6b2ac45 >> >> --- /dev/null >> >> +++ b/include/drm/drm_writeback.h >> >> @@ -0,0 +1,78 @@ >> >> +/* >> >> + * (C) COPYRIGHT 2016 ARM Limited. All rights reserved. >> >> + * Author: Brian Starkey <brian.starkey@arm.com> >> >> + * >> >> + * 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 <drm/drm_connector.h> >> >> +#include <linux/workqueue.h> >> >> + >> >> +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? >> > >> >> Yeah that's not a bad idea. The encoder funcs could be passed in to >> drm_writeback_connector_init() (in which case adding a writeback >> encoder type would also make sense). > >Well, the more I look at it the more I find it weird to represent this >writeback feature as a connector. To me, it seems to be closer to a >plane object (DRM_PLANE_TYPE_WRITEBACK?) than a real connector. >Actually, the Atmel HLCDC IP use the same register interface to expose >the overlay planes and writeback features. >Of course, representing it as a plane requires patching the core to >allow enabling a CRTC that has no active connectors connected to it if >at least one writeback plane is enabled and connected to the CRTC, but >it should be doable. Could you expand a bit on how you think planes fit better? It is something we've previously talked about internally, but so far I'm not convinced :-) >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. 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]. 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. 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. On the other hand, a writeback connector gains a few extra properties over a normal connector, but most stuff stays the same. The pipeline setup looks the same as normal to userspace, you don't need a CRTC to be active with no connectors, output cloning is the same etc. Thanks, -Brian [1] https://lists.freedesktop.org/archives/dri-devel/2016-July/113329.html
On Wed, 19 Apr 2017 10:51:23 +0100 Brian Starkey <brian.starkey@arm.com> wrote: > 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 <brian.starkey@arm.com> 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. :-) > > > > >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 it looked comparatively large. > > >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. > > > >> > >> >> }; > >> >> > >> >> /** > >> >> 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 0000000..6b2ac45 > >> >> --- /dev/null > >> >> +++ b/include/drm/drm_writeback.h > >> >> @@ -0,0 +1,78 @@ > >> >> +/* > >> >> + * (C) COPYRIGHT 2016 ARM Limited. All rights reserved. > >> >> + * Author: Brian Starkey <brian.starkey@arm.com> > >> >> + * > >> >> + * 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 <drm/drm_connector.h> > >> >> +#include <linux/workqueue.h> > >> >> + > >> >> +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? > >> > > >> > >> Yeah that's not a bad idea. The encoder funcs could be passed in to > >> drm_writeback_connector_init() (in which case adding a writeback > >> encoder type would also make sense). > > > >Well, the more I look at it the more I find it weird to represent this > >writeback feature as a connector. To me, it seems to be closer to a > >plane object (DRM_PLANE_TYPE_WRITEBACK?) than a real connector. > >Actually, the Atmel HLCDC IP use the same register interface to expose > >the overlay planes and writeback features. > >Of course, representing it as a plane requires patching the core to > >allow enabling a CRTC that has no active connectors connected to it if > >at least one writeback plane is enabled and connected to the CRTC, but > >it should be doable. > > 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, ...). > 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 ;-). > > >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. > > 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. > > On the other hand, a writeback connector gains a few extra properties > over a normal connector, but most stuff stays the same. The pipeline > setup looks the same as normal to userspace, you don't need a CRTC to > be active with no connectors, output cloning is the same etc. > > Thanks, > -Brian > > [1] https://lists.freedesktop.org/archives/dri-devel/2016-July/113329.html >
On Wed, Apr 19, 2017 at 01:34:34PM +0200, Boris Brezillon wrote: >On Wed, 19 Apr 2017 10:51:23 +0100 >Brian Starkey <brian.starkey@arm.com> wrote: [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
On Fri, 25 Nov 2016 16:48:59 +0000 Brian Starkey <brian.starkey@arm.com> wrote: > +/** > + * 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.
On Fri, May 05, 2017 at 10:22:19AM +0200, Boris Brezillon wrote: > On Fri, 25 Nov 2016 16:48:59 +0000 > Brian Starkey <brian.starkey@arm.com> wrote: > > > +/** > > + * 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
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 <drm/drm_mode.h> #include <drm/drm_plane_helper.h> #include <drm/drm_print.h> +#include <drm/drm_writeback.h> #include <linux/sync_file.h> #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, connector->name, + state->crtc->base.id); + return -EINVAL; + } + + return 0; +} + +/** * drm_atomic_get_plane_state - get plane state * @state: global atomic state object * @plane: plane to get state object for @@ -1087,6 +1128,12 @@ int drm_atomic_connector_set_property(struct drm_connector *connector, * now?) atomic writes to DPMS property: */ return -EINVAL; + } else if (property == config->writeback_fb_id_property) { + struct drm_framebuffer *fb = drm_framebuffer_lookup(dev, val); + int ret = drm_atomic_set_writeback_fb_for_connector(state, fb); + if (fb) + drm_framebuffer_unreference(fb); + return ret; } else if (connector->funcs->atomic_set_property) { return connector->funcs->atomic_set_property(connector, state, property, val); @@ -1135,6 +1182,9 @@ static void drm_atomic_connector_print_state(struct drm_printer *p, *val = (state->crtc) ? state->crtc->base.id : 0; } else if (property == config->dpms_property) { *val = connector->dpms; + } else if (property == config->writeback_fb_id_property) { + /* Writeback framebuffer is one-shot, write and forget */ + *val = 0; } else if (connector->funcs->atomic_get_property) { return connector->funcs->atomic_get_property(connector, state, property, val); @@ -1347,6 +1397,75 @@ int drm_atomic_get_property(struct drm_mode_object *obj, } EXPORT_SYMBOL(drm_atomic_set_crtc_for_connector); +/* + * drm_atomic_get_writeback_job - return or allocate a writeback job + * @conn_state: Connector state to get the job for + * + * Writeback jobs have a different lifetime to the atomic state they are + * associated with. This convenience function takes care of allocating a job + * if there isn't yet one associated with the connector state, otherwise + * it just returns the existing job. + * + * Returns: The writeback job for the given connector state + */ +static struct drm_writeback_job * +drm_atomic_get_writeback_job(struct drm_connector_state *conn_state) +{ + WARN_ON(conn_state->connector->connector_type != + DRM_MODE_CONNECTOR_WRITEBACK); + + if (!conn_state->writeback_job) + conn_state->writeback_job = + kzalloc(sizeof(*conn_state->writeback_job), GFP_KERNEL); + + return conn_state->writeback_job; +} + +/** + * drm_atomic_set_writeback_fb_for_connector - set writeback framebuffer + * @conn_state: atomic state object for the connector + * @fb: fb to use for the connector + * + * This is used to set the framebuffer for a writeback connector, which outputs + * to a buffer instead of an actual physical connector. + * Changing the assigned framebuffer requires us to grab a reference to the new + * fb and drop the reference to the old fb, if there is one. This function + * takes care of all these details besides updating the pointer in the + * state object itself. + * + * Note: The only way conn_state can already have an fb set is if the commit + * sets the property more than once. + * + * See also: drm_writeback_connector_init() + * + * Returns: 0 on success + */ +int drm_atomic_set_writeback_fb_for_connector( + struct drm_connector_state *conn_state, + struct drm_framebuffer *fb) +{ + struct drm_writeback_job *job = + drm_atomic_get_writeback_job(conn_state); + if (!job) + return -ENOMEM; + + if (job->fb) + drm_framebuffer_unreference(job->fb); + if (fb) + drm_framebuffer_reference(fb); + job->fb = fb; + + if (fb) + DRM_DEBUG_ATOMIC("Set [FB:%d] for connector state %p\n", + fb->base.id, conn_state); + else + DRM_DEBUG_ATOMIC("Set [NOFB] for connector state %p\n", + conn_state); + + return 0; +} +EXPORT_SYMBOL(drm_atomic_set_writeback_fb_for_connector); + /** * drm_atomic_add_affected_connectors - add connectors for crtc * @state: atomic state @@ -1501,6 +1620,8 @@ int drm_atomic_check_only(struct drm_atomic_state *state) struct drm_plane_state *plane_state; struct drm_crtc *crtc; struct drm_crtc_state *crtc_state; + struct drm_connector *conn; + struct drm_connector_state *conn_state; int i, ret = 0; DRM_DEBUG_ATOMIC("checking %p\n", state); @@ -1523,6 +1644,15 @@ int drm_atomic_check_only(struct drm_atomic_state *state) } } + for_each_connector_in_state(state, conn, conn_state, i) { + ret = drm_atomic_connector_check(conn, conn_state); + if (ret) { + DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] atomic core check failed\n", + conn->base.id, conn->name); + return ret; + } + } + if (config->funcs->atomic_check) ret = config->funcs->atomic_check(state->dev, state); diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 0b16587..75812b9 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -30,6 +30,7 @@ #include <drm/drm_plane_helper.h> #include <drm/drm_crtc_helper.h> #include <drm/drm_atomic_helper.h> +#include <drm/drm_writeback.h> #include <linux/dma-fence.h> #include "drm_crtc_internal.h" @@ -3192,6 +3193,9 @@ void drm_atomic_helper_connector_reset(struct drm_connector *connector) memcpy(state, connector->state, sizeof(*state)); if (state->crtc) drm_connector_reference(connector); + + /* Don't copy over a writeback job, they are used only once */ + state->writeback_job = NULL; } EXPORT_SYMBOL(__drm_atomic_helper_connector_duplicate_state); @@ -3319,6 +3323,8 @@ struct drm_atomic_state * */ if (state->crtc) drm_connector_unreference(state->connector); + if (state->writeback_job) + drm_writeback_cleanup_job(state->writeback_job); } EXPORT_SYMBOL(__drm_atomic_helper_connector_destroy_state); 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" }, }; void drm_connector_ida_init(void) @@ -235,7 +236,8 @@ int drm_connector_init(struct drm_device *dev, list_add_tail(&connector->head, &config->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)) drm_object_attach_property(&connector->base, config->edid_property, 0); diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c new file mode 100644 index 0000000..75a1dbf --- /dev/null +++ b/drivers/gpu/drm/drm_writeback.c @@ -0,0 +1,230 @@ +/* + * (C) COPYRIGHT 2016 ARM Limited. All rights reserved. + * Author: Brian Starkey <brian.starkey@arm.com> + * + * 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. + */ + +#include <drm/drm_crtc.h> +#include <drm/drm_property.h> +#include <drm/drm_writeback.h> +#include <drm/drmP.h> + +/** + * DOC: overview + * + * Writeback connectors are used to expose hardware which can write the output + * from a CRTC to a memory buffer. They are used and act similarly to other + * types of connectors, with some important differences: + * - Writeback connectors don't provide a way to output visually to the user. + * - Writeback connectors should always report as "disconnected" (so that + * clients which don't understand them will ignore them). + * - Writeback connectors don't have EDID. + * + * A framebuffer may only be attached to a writeback connector when the + * connector is attached to a CRTC. The WRITEBACK_FB_ID property which sets the + * framebuffer applies only to a single commit (see below). A framebuffer may + * not be attached while the CRTC is off. + * + * Writeback connectors have some additional properties, which userspace + * can use to query and control them: + * + * "WRITEBACK_FB_ID": + * Write-only object property storing a DRM_MODE_OBJECT_FB: it stores the + * framebuffer to be written by the writeback connector. This property is + * similar to the FB_ID property on planes, but will always read as zero + * and is not preserved across commits. + * Userspace must set this property to an output buffer every time it + * wishes the buffer to get filled. + * + * "PIXEL_FORMATS": + * Immutable blob property to store the supported pixel formats table. The + * data is an array of u32 DRM_FORMAT_* fourcc values. + * Userspace can use this blob to find out what pixel formats are supported + * by the connector's writeback engine. + */ + +static bool create_writeback_properties(struct drm_device *dev) +{ + struct drm_property *prop; + + if (!dev->mode_config.writeback_fb_id_property) { + prop = drm_property_create_object(dev, DRM_MODE_PROP_ATOMIC, + "WRITEBACK_FB_ID", + DRM_MODE_OBJECT_FB); + if (!prop) + return false; + dev->mode_config.writeback_fb_id_property = prop; + } + + if (!dev->mode_config.writeback_pixel_formats_property) { + prop = drm_property_create(dev, DRM_MODE_PROP_BLOB | DRM_MODE_PROP_IMMUTABLE, + "PIXEL_FORMATS", 0); + if (!prop) + return false; + dev->mode_config.writeback_pixel_formats_property = prop; + } + + return true; +} + +/** + * 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) +{ + int ret; + struct drm_property_blob *blob; + struct drm_connector *connector = &wb_connector->base; + struct drm_mode_config *config = &dev->mode_config; + + if (!create_writeback_properties(dev)) + return -EINVAL; + + blob = drm_property_create_blob(dev, n_formats * sizeof(*formats), + formats); + if (IS_ERR(blob)) + return PTR_ERR(blob); + + ret = drm_connector_init(dev, connector, funcs, + DRM_MODE_CONNECTOR_WRITEBACK); + if (ret) + goto fail; + + INIT_LIST_HEAD(&wb_connector->job_queue); + spin_lock_init(&wb_connector->job_lock); + + drm_object_attach_property(&connector->base, + config->writeback_fb_id_property, 0); + + drm_object_attach_property(&connector->base, + config->writeback_pixel_formats_property, + blob->base.id); + wb_connector->pixel_formats_blob_ptr = blob; + + return 0; + +fail: + drm_property_unreference_blob(blob); + return ret; +} +EXPORT_SYMBOL(drm_writeback_connector_init); + +/** + * @drm_writeback_queue_job: Queue a writeback job for later signalling + * @wb_connector: The writeback connector to queue a job on + * @job: The job to queue + * + * This function adds a job to the job_queue for a writeback connector. It + * should be considered to take ownership of the writeback job, and so any other + * references to the job must be cleared after calling this function. + * + * Drivers must ensure that for a given writeback connector, jobs are queued in + * exactly the same order as they will be completed by the hardware (and + * signaled via drm_writeback_signal_completion). + * + * For every call to drm_writeback_queue_job() there must be exactly one call to + * drm_writeback_signal_completion() + * + * See also: drm_writeback_signal_completion() + */ +void drm_writeback_queue_job(struct drm_writeback_connector *wb_connector, + struct drm_writeback_job *job) +{ + unsigned long flags; + + spin_lock_irqsave(&wb_connector->job_lock, flags); + list_add_tail(&job->list_entry, &wb_connector->job_queue); + spin_unlock_irqrestore(&wb_connector->job_lock, flags); +} +EXPORT_SYMBOL(drm_writeback_queue_job); + +/** + * @drm_writeback_cleanup_job: Cleanup and free a writeback job + * @job: The writeback job to free + * + * Drops any references held by the writeback job, and frees the structure. + */ +void drm_writeback_cleanup_job(struct drm_writeback_job *job) +{ + if (!job) + return; + + if (job->fb) + drm_framebuffer_unreference(job->fb); + kfree(job); +} +EXPORT_SYMBOL(drm_writeback_cleanup_job); + +/* + * @cleanup_work: deferred cleanup of a writeback job + * + * The job cannot be cleaned up directly in drm_writeback_signal_completion, + * because it may be called in interrupt context. Dropping the framebuffer + * reference can sleep, and so the cleanup is deferred to a workqueue. + */ +static void cleanup_work(struct work_struct *work) +{ + struct drm_writeback_job *job = container_of(work, + struct drm_writeback_job, + cleanup_work); + drm_writeback_cleanup_job(job); +} + +/** + * @drm_writeback_signal_completion: Signal the completion of a writeback job + * @wb_connector: The writeback connector whose job is complete + * + * Drivers should call this to signal the completion of a previously queued + * writeback job. It should be called as soon as possible after the hardware + * has finished writing, and may be called from interrupt context. + * It is the driver's responsibility to ensure that for a given connector, the + * hardware completes writeback jobs in the same order as they are queued. + * + * Unless the driver is holding its own reference to the framebuffer, it must + * not be accessed after calling this function. + * + * See also: drm_writeback_queue_job() + */ +void +drm_writeback_signal_completion(struct drm_writeback_connector *wb_connector) +{ + unsigned long flags; + struct drm_writeback_job *job; + + spin_lock_irqsave(&wb_connector->job_lock, flags); + job = list_first_entry_or_null(&wb_connector->job_queue, + struct drm_writeback_job, + list_entry); + if (job) + list_del(&job->list_entry); + spin_unlock_irqrestore(&wb_connector->job_lock, flags); + + if (WARN_ON(!job)) + return; + + INIT_WORK(&job->cleanup_work, cleanup_work); + queue_work(system_long_wq, &job->cleanup_work); +} +EXPORT_SYMBOL(drm_writeback_signal_completion); diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h index c0eaec7..476561d 100644 --- a/include/drm/drm_atomic.h +++ b/include/drm/drm_atomic.h @@ -351,6 +351,9 @@ void drm_atomic_set_fence_for_plane(struct drm_plane_state *plane_state, int __must_check drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state, struct drm_crtc *crtc); +int drm_atomic_set_writeback_fb_for_connector( + struct drm_connector_state *conn_state, + struct drm_framebuffer *fb); int __must_check drm_atomic_add_affected_connectors(struct drm_atomic_state *state, struct drm_crtc *crtc); 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; }; /** 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 0000000..6b2ac45 --- /dev/null +++ b/include/drm/drm_writeback.h @@ -0,0 +1,78 @@ +/* + * (C) COPYRIGHT 2016 ARM Limited. All rights reserved. + * Author: Brian Starkey <brian.starkey@arm.com> + * + * 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 <drm/drm_connector.h> +#include <linux/workqueue.h> + +struct drm_writeback_connector { + struct drm_connector base; + + /** + * @pixel_formats_blob_ptr: + * + * DRM blob property data for the pixel formats list on writeback + * connectors + * See also drm_writeback_connector_init() + */ + struct drm_property_blob *pixel_formats_blob_ptr; + + /** @job_lock: Protects job_queue */ + spinlock_t job_lock; + /** + * @job_queue: + * + * Holds a list of a connector's writeback jobs; the last item is the + * most recent. The first item may be either waiting for the hardware + * to begin writing, or currently being written. + * + * See also: drm_writeback_queue_job() and + * drm_writeback_signal_completion() + */ + struct list_head job_queue; +}; + +struct drm_writeback_job { + /** + * @cleanup_work: + * + * Used to allow drm_writeback_signal_completion to defer dropping the + * framebuffer reference to a workqueue. + */ + struct work_struct cleanup_work; + /** + * @list_entry: + * + * List item for the connector's @job_queue + */ + struct list_head list_entry; + /** + * @fb: + * + * Framebuffer to be written to by the writeback connector. Do not set + * directly, use drm_atomic_set_writeback_fb_for_connector() + */ + struct drm_framebuffer *fb; +}; + +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); + +void drm_writeback_queue_job(struct drm_writeback_connector *wb_connector, + struct drm_writeback_job *job); + +void drm_writeback_cleanup_job(struct drm_writeback_job *job); + +void +drm_writeback_signal_completion(struct drm_writeback_connector *wb_connector); +#endif diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h index ebf622f..ae5e4f7 100644 --- a/include/uapi/drm/drm_mode.h +++ b/include/uapi/drm/drm_mode.h @@ -263,6 +263,7 @@ struct drm_mode_get_encoder { #define DRM_MODE_CONNECTOR_VIRTUAL 15 #define DRM_MODE_CONNECTOR_DSI 16 #define DRM_MODE_CONNECTOR_DPI 17 +#define DRM_MODE_CONNECTOR_WRITEBACK 18 struct drm_mode_get_connector {
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 <brian.starkey@arm.com> --- 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