Re: [PATCH v5 14/19] drm: writeback: Add job prepare and cleanup operations
Hi Liviu,
On Tue, Feb 26, 2019 at 06:39:40PM +, Liviu Dudau wrote:
> On Thu, Feb 21, 2019 at 12:32:07PM +0200, Laurent Pinchart wrote:
> > As writeback jobs contain a framebuffer, drivers may need to prepare and
> > cleanup them the same way they can prepare and cleanup framebuffers for
> > planes. Add two new optional connector helper operations,
> > .prepare_writeback_job() and .cleanup_writeback_job() to support this.
>
> I'm having a bit of a hard time parsing the above paragraph. I think that
> what you are saying is that you need to prepare and cleanup the framebuffers
> that
> writeback jobs have, but it also can be read that you need to prepare/cleanup
> the actual jobs. If the latter, then I'm curious to know what is special
> about the jobs that you need preparing/cleaning up.
I meant the framebuffers, but I wouldn't be surprised if the jobs were
later extended with more fields that would require similar preparation.
That's why I think prepare/cleanup job operations make sense.
> > The job prepare operation is called from
> > drm_atomic_helper_prepare_planes() to avoid a new atomic commit helper
> > that would need to be called by all drivers not using
> > drm_atomic_helper_commit(). The job cleanup operation is called from the
> > existing drm_writeback_cleanup_job() function, invoked both when
> > destroying the job as part of a aborted commit, or when the job
> > completes.
> >
> > The drm_writeback_job structure is extended with a priv field to let
> > drivers store per-job data, such as mappings related to the writeback
> > framebuffer.
>
> Could the driver store in this priv field a structure that contains the
> connector, whereby removing the need for a back-pointer?
The priv field points to an opaque structure, forcing drivers to use a
standard structure there would make the API more complex in my opinion,
without any added value I can see.
> > For internal plumbing reasons the drm_writeback_job structure needs to
> > store a back-pointer to the drm_writeback_connector. To avoid pushing
> > too much writeback-specific knowledge to drm_atomic_uapi.c, create a
> > drm_writeback_set_fb() function, move the writeback job setup code
> > there, and set the connector backpointer. The prepare_signaling()
> > function doesn't need to allocate writeback jobs and can ignore
> > connectors without a job, as it is called after the writeback jobs are
> > allocated to store framebuffers, and a writeback fence with a
> > framebuffer is an invalid configuration that gets rejected by the commit
> > check.
> >
> > Signed-off-by: Laurent Pinchart
> > ---
> > drivers/gpu/drm/drm_atomic_helper.c | 11 ++
> > drivers/gpu/drm/drm_atomic_uapi.c| 31 +
> > drivers/gpu/drm/drm_writeback.c | 43
> > include/drm/drm_modeset_helper_vtables.h | 7
> > include/drm/drm_writeback.h | 28 ++-
> > 5 files changed, 96 insertions(+), 24 deletions(-)
> >
> > This patch is currently missing documentation for the
> > .prepare_writeback_job() and .cleanup_writeback_job() operations. I plan
> > to fix this, but first wanted feedback on the direction taken. I'm not
> > entirely happy with the priv pointer in the drm_writeback_job structure,
> > but adding a full state duplicate/destroy machinery for that structure
> > was equally unappealing to me.
> >
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> > b/drivers/gpu/drm/drm_atomic_helper.c
> > index 6fe2303fccd9..70a4886c6e65 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -2245,10 +2245,21 @@
> > EXPORT_SYMBOL(drm_atomic_helper_commit_cleanup_done);
> > int drm_atomic_helper_prepare_planes(struct drm_device *dev,
> > struct drm_atomic_state *state)
> > {
> > + struct drm_connector *connector;
> > + struct drm_connector_state *new_conn_state;
> > struct drm_plane *plane;
> > struct drm_plane_state *new_plane_state;
> > int ret, i, j;
> >
> > + for_each_new_connector_in_state(state, connector, new_conn_state, i) {
> > + if (!new_conn_state->writeback_job)
> > + continue;
> > +
> > + ret = drm_writeback_prepare_job(new_conn_state->writeback_job);
> > + if (ret < 0)
> > + return ret;
> > + }
> > +
> > for_each_new_plane_in_state(state, plane, new_plane_state, i) {
> > const struct drm_plane_helper_funcs *funcs;
> >
> > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
> > b/drivers/gpu/drm/drm_atomic_uapi.c
> > index c40889888a16..e802152a01ad 100644
> > --- a/drivers/gpu/drm/drm_atomic_uapi.c
> > +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> > @@ -647,28 +647,15 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
> > return 0;
> > }
> >
> > -static struct drm_writeback_job *
> > -drm_atomic_get_writeback_job(struct drm_connector_state *conn_state)
> > -{
> >
Re: [PATCH v5 14/19] drm: writeback: Add job prepare and cleanup operations
Hi Laurent,
On Thu, Feb 21, 2019 at 12:32:07PM +0200, Laurent Pinchart wrote:
> As writeback jobs contain a framebuffer, drivers may need to prepare and
> cleanup them the same way they can prepare and cleanup framebuffers for
> planes. Add two new optional connector helper operations,
> .prepare_writeback_job() and .cleanup_writeback_job() to support this.
I'm having a bit of a hard time parsing the above paragraph. I think that
what you are saying is that you need to prepare and cleanup the framebuffers
that
writeback jobs have, but it also can be read that you need to prepare/cleanup
the actual jobs. If the latter, then I'm curious to know what is special
about the jobs that you need preparing/cleaning up.
>
> The job prepare operation is called from
> drm_atomic_helper_prepare_planes() to avoid a new atomic commit helper
> that would need to be called by all drivers not using
> drm_atomic_helper_commit(). The job cleanup operation is called from the
> existing drm_writeback_cleanup_job() function, invoked both when
> destroying the job as part of a aborted commit, or when the job
> completes.
>
> The drm_writeback_job structure is extended with a priv field to let
> drivers store per-job data, such as mappings related to the writeback
> framebuffer.
Could the driver store in this priv field a structure that contains the
connector, whereby removing the need for a back-pointer?
>
> For internal plumbing reasons the drm_writeback_job structure needs to
> store a back-pointer to the drm_writeback_connector. To avoid pushing
> too much writeback-specific knowledge to drm_atomic_uapi.c, create a
> drm_writeback_set_fb() function, move the writeback job setup code
> there, and set the connector backpointer. The prepare_signaling()
> function doesn't need to allocate writeback jobs and can ignore
> connectors without a job, as it is called after the writeback jobs are
> allocated to store framebuffers, and a writeback fence with a
> framebuffer is an invalid configuration that gets rejected by the commit
> check.
>
> Signed-off-by: Laurent Pinchart
> ---
> drivers/gpu/drm/drm_atomic_helper.c | 11 ++
> drivers/gpu/drm/drm_atomic_uapi.c| 31 +
> drivers/gpu/drm/drm_writeback.c | 43
> include/drm/drm_modeset_helper_vtables.h | 7
> include/drm/drm_writeback.h | 28 ++-
> 5 files changed, 96 insertions(+), 24 deletions(-)
>
> This patch is currently missing documentation for the
> .prepare_writeback_job() and .cleanup_writeback_job() operations. I plan
> to fix this, but first wanted feedback on the direction taken. I'm not
> entirely happy with the priv pointer in the drm_writeback_job structure,
> but adding a full state duplicate/destroy machinery for that structure
> was equally unappealing to me.
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> b/drivers/gpu/drm/drm_atomic_helper.c
> index 6fe2303fccd9..70a4886c6e65 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -2245,10 +2245,21 @@ EXPORT_SYMBOL(drm_atomic_helper_commit_cleanup_done);
> int drm_atomic_helper_prepare_planes(struct drm_device *dev,
>struct drm_atomic_state *state)
> {
> + struct drm_connector *connector;
> + struct drm_connector_state *new_conn_state;
> struct drm_plane *plane;
> struct drm_plane_state *new_plane_state;
> int ret, i, j;
>
> + for_each_new_connector_in_state(state, connector, new_conn_state, i) {
> + if (!new_conn_state->writeback_job)
> + continue;
> +
> + ret = drm_writeback_prepare_job(new_conn_state->writeback_job);
> + if (ret < 0)
> + return ret;
> + }
> +
> for_each_new_plane_in_state(state, plane, new_plane_state, i) {
> const struct drm_plane_helper_funcs *funcs;
>
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
> b/drivers/gpu/drm/drm_atomic_uapi.c
> index c40889888a16..e802152a01ad 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -647,28 +647,15 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
> return 0;
> }
>
> -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;
> -}
> -
> static 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)
> -
Re: [PATCH v5 14/19] drm: writeback: Add job prepare and cleanup operations
On Fri, Feb 22, 2019 at 04:49:53PM +0200, Laurent Pinchart wrote: > > That would be the connector helper functions, not the connector > functions, right ? Yes, sorry. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v5 14/19] drm: writeback: Add job prepare and cleanup operations
Hi Brian, On Fri, Feb 22, 2019 at 01:50:01PM +, Brian Starkey wrote: > On Fri, Feb 22, 2019 at 12:12:00AM +0200, Laurent Pinchart wrote: > > On Thu, Feb 21, 2019 at 06:12:03PM +, Brian Starkey wrote: > >> On Thu, Feb 21, 2019 at 12:32:07PM +0200, Laurent Pinchart wrote: > >>> As writeback jobs contain a framebuffer, drivers may need to prepare and > >>> cleanup them the same way they can prepare and cleanup framebuffers for > >>> planes. Add two new optional connector helper operations, > >>> .prepare_writeback_job() and .cleanup_writeback_job() to support this. > >>> > >>> The job prepare operation is called from > >>> drm_atomic_helper_prepare_planes() to avoid a new atomic commit helper > >>> that would need to be called by all drivers not using > >>> drm_atomic_helper_commit(). The job cleanup operation is called from the > >>> existing drm_writeback_cleanup_job() function, invoked both when > >>> destroying the job as part of a aborted commit, or when the job > >>> completes. > >>> > >>> The drm_writeback_job structure is extended with a priv field to let > >>> drivers store per-job data, such as mappings related to the writeback > >>> framebuffer. > >>> > >>> For internal plumbing reasons the drm_writeback_job structure needs to > >>> store a back-pointer to the drm_writeback_connector. To avoid pushing > >>> too much writeback-specific knowledge to drm_atomic_uapi.c, create a > >>> drm_writeback_set_fb() function, move the writeback job setup code > >>> there, and set the connector backpointer. The prepare_signaling() > >>> function doesn't need to allocate writeback jobs and can ignore > >>> connectors without a job, as it is called after the writeback jobs are > >>> allocated to store framebuffers, and a writeback fence with a > >>> framebuffer is an invalid configuration that gets rejected by the commit > >>> check. > >>> > >>> Signed-off-by: Laurent Pinchart > >>> > >> > >> I probably need to revisit this with fresh eyes tomorrow, but how come > >> the prepared-ness and whatever is being prepared can't be put into a > >> subclassed framebuffer? That seems more natural to me, than tracking > >> it with the job. It's really the framebuffer we're preparing after > >> all. > > > > In my patch series I indeed need to track information related to the > > framebuffer only, but is it a good idea to limit writeback > > prepare/cleanup to that ? Other drivers may have different needs. > > IMO better to write the code for the case we know, rather than > speculate. > > > Furthermore, the mapping needs to exist for the lifetime of the > > writeback job, so I'd have to add a counter to the framebuffer subclass > > to track these too. While doable, it starts sounding a bit like a hack, > > and may create race conditions. > > You're probably right. As you were saying to Daniel - writeback things > don't have the same lifetime as the state, so the job is the only > place to store things which would normally go in the state. > > >> I guess if you have the same framebuffer in use for multiple things, > >> you might need to track it separately? Can the mappings be shared in > >> that case? > > > > Speaking about my case, the mapping is per-CRTC, so if I use the same > > framebuffer for multiple CRTCs I'd need separate mappings. Other drivers > > may have simpler or more complex needs. > > > > When you'll revisit this with fresh eyes, I'd like to know if you think > > it would be worth it replacing the priv pointer with allocate/destroy > > operations to allow subclassing the writeback job. We could possibly do > > without the destroy operation if we mandate embedding the writeback job > > as the first field of the subclass and usage of kmalloc (& friends) to > > allocate the subclass structure. We could even do without the allocate > > operation if we just stored the size of the subclass in the writeback > > connector itself, but that would depart from what we usually do in DRM. > > I think enabling the job to be subclassed would be a fine solution, > with functions which are as close as possible to the other DRM > objects. > > I'm not sure there's a lot of advantage in skipping allocate/destroy; > it should be fine to just have helpers for the common case. > > I wonder if renaming it to "writeback_state" instead of > "writeback_job" is too confusing (because it doesn't _quite_ act like > the other states, even though its purpose is very similar). I'd keep job, as it works a bit differently from states. > > I would also like to know if I should create a writeback connector > > operations structure to store the prepare/cleanup and perhaps > > allocate/destroy operations instead of adding them to the connector > > helper funcs. > > I think just adding the functions to the existing connector > operations is overall less clutter, but I don't mind either way. That would be the connector helper functions, not the connector functions, right ? Daniel, do you have a preference ? > >>> --- >
Re: [PATCH v5 14/19] drm: writeback: Add job prepare and cleanup operations
On Fri, Feb 22, 2019 at 12:12:00AM +0200, Laurent Pinchart wrote: > Hi Brian, > > On Thu, Feb 21, 2019 at 06:12:03PM +, Brian Starkey wrote: > > On Thu, Feb 21, 2019 at 12:32:07PM +0200, Laurent Pinchart wrote: > > > As writeback jobs contain a framebuffer, drivers may need to prepare and > > > cleanup them the same way they can prepare and cleanup framebuffers for > > > planes. Add two new optional connector helper operations, > > > .prepare_writeback_job() and .cleanup_writeback_job() to support this. > > > > > > The job prepare operation is called from > > > drm_atomic_helper_prepare_planes() to avoid a new atomic commit helper > > > that would need to be called by all drivers not using > > > drm_atomic_helper_commit(). The job cleanup operation is called from the > > > existing drm_writeback_cleanup_job() function, invoked both when > > > destroying the job as part of a aborted commit, or when the job > > > completes. > > > > > > The drm_writeback_job structure is extended with a priv field to let > > > drivers store per-job data, such as mappings related to the writeback > > > framebuffer. > > > > > > For internal plumbing reasons the drm_writeback_job structure needs to > > > store a back-pointer to the drm_writeback_connector. To avoid pushing > > > too much writeback-specific knowledge to drm_atomic_uapi.c, create a > > > drm_writeback_set_fb() function, move the writeback job setup code > > > there, and set the connector backpointer. The prepare_signaling() > > > function doesn't need to allocate writeback jobs and can ignore > > > connectors without a job, as it is called after the writeback jobs are > > > allocated to store framebuffers, and a writeback fence with a > > > framebuffer is an invalid configuration that gets rejected by the commit > > > check. > > > > > > Signed-off-by: Laurent Pinchart > > > > > > > I probably need to revisit this with fresh eyes tomorrow, but how come > > the prepared-ness and whatever is being prepared can't be put into a > > subclassed framebuffer? That seems more natural to me, than tracking > > it with the job. It's really the framebuffer we're preparing after > > all. > > In my patch series I indeed need to track information related to the > framebuffer only, but is it a good idea to limit writeback > prepare/cleanup to that ? Other drivers may have different needs. IMO better to write the code for the case we know, rather than speculate. > > Furthermore, the mapping needs to exist for the lifetime of the > writeback job, so I'd have to add a counter to the framebuffer subclass > to track these too. While doable, it starts sounding a bit like a hack, > and may create race conditions. > You're probably right. As you were saying to Daniel - writeback things don't have the same lifetime as the state, so the job is the only place to store things which would normally go in the state. > > I guess if you have the same framebuffer in use for multiple things, > > you might need to track it separately? Can the mappings be shared in > > that case? > > Speaking about my case, the mapping is per-CRTC, so if I use the same > framebuffer for multiple CRTCs I'd need separate mappings. Other drivers > may have simpler or more complex needs. > > When you'll revisit this with fresh eyes, I'd like to know if you think > it would be worth it replacing the priv pointer with allocate/destroy > operations to allow subclassing the writeback job. We could possibly do > without the destroy operation if we mandate embedding the writeback job > as the first field of the subclass and usage of kmalloc (& friends) to > allocate the subclass structure. We could even do without the allocate > operation if we just stored the size of the subclass in the writeback > connector itself, but that would depart from what we usually do in DRM. > I think enabling the job to be subclassed would be a fine solution, with functions which are as close as possible to the other DRM objects. I'm not sure there's a lot of advantage in skipping allocate/destroy; it should be fine to just have helpers for the common case. I wonder if renaming it to "writeback_state" instead of "writeback_job" is too confusing (because it doesn't _quite_ act like the other states, even though its purpose is very similar). > I would also like to know if I should create a writeback connector > operations structure to store the prepare/cleanup and perhaps > allocate/destroy operations instead of adding them to the connector > helper funcs. I think just adding the functions to the existing connector operations is overall less clutter, but I don't mind either way. -Brian > > > > --- > > > drivers/gpu/drm/drm_atomic_helper.c | 11 ++ > > > drivers/gpu/drm/drm_atomic_uapi.c| 31 + > > > drivers/gpu/drm/drm_writeback.c | 43 > > > include/drm/drm_modeset_helper_vtables.h | 7 > > > include/drm/drm_writeback.h | 28
Re: [PATCH v5 14/19] drm: writeback: Add job prepare and cleanup operations
Hi Brian,
On Thu, Feb 21, 2019 at 06:12:03PM +, Brian Starkey wrote:
> On Thu, Feb 21, 2019 at 12:32:07PM +0200, Laurent Pinchart wrote:
> > As writeback jobs contain a framebuffer, drivers may need to prepare and
> > cleanup them the same way they can prepare and cleanup framebuffers for
> > planes. Add two new optional connector helper operations,
> > .prepare_writeback_job() and .cleanup_writeback_job() to support this.
> >
> > The job prepare operation is called from
> > drm_atomic_helper_prepare_planes() to avoid a new atomic commit helper
> > that would need to be called by all drivers not using
> > drm_atomic_helper_commit(). The job cleanup operation is called from the
> > existing drm_writeback_cleanup_job() function, invoked both when
> > destroying the job as part of a aborted commit, or when the job
> > completes.
> >
> > The drm_writeback_job structure is extended with a priv field to let
> > drivers store per-job data, such as mappings related to the writeback
> > framebuffer.
> >
> > For internal plumbing reasons the drm_writeback_job structure needs to
> > store a back-pointer to the drm_writeback_connector. To avoid pushing
> > too much writeback-specific knowledge to drm_atomic_uapi.c, create a
> > drm_writeback_set_fb() function, move the writeback job setup code
> > there, and set the connector backpointer. The prepare_signaling()
> > function doesn't need to allocate writeback jobs and can ignore
> > connectors without a job, as it is called after the writeback jobs are
> > allocated to store framebuffers, and a writeback fence with a
> > framebuffer is an invalid configuration that gets rejected by the commit
> > check.
> >
> > Signed-off-by: Laurent Pinchart
>
> I probably need to revisit this with fresh eyes tomorrow, but how come
> the prepared-ness and whatever is being prepared can't be put into a
> subclassed framebuffer? That seems more natural to me, than tracking
> it with the job. It's really the framebuffer we're preparing after
> all.
In my patch series I indeed need to track information related to the
framebuffer only, but is it a good idea to limit writeback
prepare/cleanup to that ? Other drivers may have different needs.
Furthermore, the mapping needs to exist for the lifetime of the
writeback job, so I'd have to add a counter to the framebuffer subclass
to track these too. While doable, it starts sounding a bit like a hack,
and may create race conditions.
> I guess if you have the same framebuffer in use for multiple things,
> you might need to track it separately? Can the mappings be shared in
> that case?
Speaking about my case, the mapping is per-CRTC, so if I use the same
framebuffer for multiple CRTCs I'd need separate mappings. Other drivers
may have simpler or more complex needs.
When you'll revisit this with fresh eyes, I'd like to know if you think
it would be worth it replacing the priv pointer with allocate/destroy
operations to allow subclassing the writeback job. We could possibly do
without the destroy operation if we mandate embedding the writeback job
as the first field of the subclass and usage of kmalloc (& friends) to
allocate the subclass structure. We could even do without the allocate
operation if we just stored the size of the subclass in the writeback
connector itself, but that would depart from what we usually do in DRM.
I would also like to know if I should create a writeback connector
operations structure to store the prepare/cleanup and perhaps
allocate/destroy operations instead of adding them to the connector
helper funcs.
> > ---
> > drivers/gpu/drm/drm_atomic_helper.c | 11 ++
> > drivers/gpu/drm/drm_atomic_uapi.c| 31 +
> > drivers/gpu/drm/drm_writeback.c | 43
> > include/drm/drm_modeset_helper_vtables.h | 7
> > include/drm/drm_writeback.h | 28 ++-
> > 5 files changed, 96 insertions(+), 24 deletions(-)
> >
> > This patch is currently missing documentation for the
> > .prepare_writeback_job() and .cleanup_writeback_job() operations. I plan
> > to fix this, but first wanted feedback on the direction taken. I'm not
> > entirely happy with the priv pointer in the drm_writeback_job structure,
> > but adding a full state duplicate/destroy machinery for that structure
> > was equally unappealing to me.
> >
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> > b/drivers/gpu/drm/drm_atomic_helper.c
> > index 6fe2303fccd9..70a4886c6e65 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -2245,10 +2245,21 @@
> > EXPORT_SYMBOL(drm_atomic_helper_commit_cleanup_done);
> > int drm_atomic_helper_prepare_planes(struct drm_device *dev,
> > struct drm_atomic_state *state)
> > {
> > + struct drm_connector *connector;
> > + struct drm_connector_state *new_conn_state;
> > struct drm_plane *plane;
> > struct drm_plane_state
Re: [PATCH v5 14/19] drm: writeback: Add job prepare and cleanup operations
Hi Laurent,
On Thu, Feb 21, 2019 at 12:32:07PM +0200, Laurent Pinchart wrote:
> As writeback jobs contain a framebuffer, drivers may need to prepare and
> cleanup them the same way they can prepare and cleanup framebuffers for
> planes. Add two new optional connector helper operations,
> .prepare_writeback_job() and .cleanup_writeback_job() to support this.
>
> The job prepare operation is called from
> drm_atomic_helper_prepare_planes() to avoid a new atomic commit helper
> that would need to be called by all drivers not using
> drm_atomic_helper_commit(). The job cleanup operation is called from the
> existing drm_writeback_cleanup_job() function, invoked both when
> destroying the job as part of a aborted commit, or when the job
> completes.
>
> The drm_writeback_job structure is extended with a priv field to let
> drivers store per-job data, such as mappings related to the writeback
> framebuffer.
>
> For internal plumbing reasons the drm_writeback_job structure needs to
> store a back-pointer to the drm_writeback_connector. To avoid pushing
> too much writeback-specific knowledge to drm_atomic_uapi.c, create a
> drm_writeback_set_fb() function, move the writeback job setup code
> there, and set the connector backpointer. The prepare_signaling()
> function doesn't need to allocate writeback jobs and can ignore
> connectors without a job, as it is called after the writeback jobs are
> allocated to store framebuffers, and a writeback fence with a
> framebuffer is an invalid configuration that gets rejected by the commit
> check.
>
> Signed-off-by: Laurent Pinchart
I probably need to revisit this with fresh eyes tomorrow, but how come
the prepared-ness and whatever is being prepared can't be put into a
subclassed framebuffer? That seems more natural to me, than tracking
it with the job. It's really the framebuffer we're preparing after
all.
I guess if you have the same framebuffer in use for multiple things,
you might need to track it separately? Can the mappings be shared in
that case?
> ---
> drivers/gpu/drm/drm_atomic_helper.c | 11 ++
> drivers/gpu/drm/drm_atomic_uapi.c| 31 +
> drivers/gpu/drm/drm_writeback.c | 43
> include/drm/drm_modeset_helper_vtables.h | 7
> include/drm/drm_writeback.h | 28 ++-
> 5 files changed, 96 insertions(+), 24 deletions(-)
>
> This patch is currently missing documentation for the
> .prepare_writeback_job() and .cleanup_writeback_job() operations. I plan
> to fix this, but first wanted feedback on the direction taken. I'm not
> entirely happy with the priv pointer in the drm_writeback_job structure,
> but adding a full state duplicate/destroy machinery for that structure
> was equally unappealing to me.
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> b/drivers/gpu/drm/drm_atomic_helper.c
> index 6fe2303fccd9..70a4886c6e65 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -2245,10 +2245,21 @@ EXPORT_SYMBOL(drm_atomic_helper_commit_cleanup_done);
> int drm_atomic_helper_prepare_planes(struct drm_device *dev,
>struct drm_atomic_state *state)
> {
> + struct drm_connector *connector;
> + struct drm_connector_state *new_conn_state;
> struct drm_plane *plane;
> struct drm_plane_state *new_plane_state;
> int ret, i, j;
>
> + for_each_new_connector_in_state(state, connector, new_conn_state, i) {
> + if (!new_conn_state->writeback_job)
> + continue;
> +
> + ret = drm_writeback_prepare_job(new_conn_state->writeback_job);
> + if (ret < 0)
> + return ret;
> + }
> +
> for_each_new_plane_in_state(state, plane, new_plane_state, i) {
> const struct drm_plane_helper_funcs *funcs;
>
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
> b/drivers/gpu/drm/drm_atomic_uapi.c
> index c40889888a16..e802152a01ad 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -647,28 +647,15 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
> return 0;
> }
>
> -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;
> -}
> -
> static 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;
> + int ret;
>
> -
