[PATCH 0/8] vc4: Convert to drm_atomic_helper_commit

[PATCH 0/8] vc4: Convert to drm_atomic_helper_commit

[Maxime Ripard]

Hi,

Here's a conversion of vc4 to remove the hand-rolled atomic_commit helper from
vc4 in favour of the generic one.

This requires some rework of vc4, but also a new hook and some documentation
for corner-cases in the DRM core that have been reported and explained by
Daniel recently.

Let me know what you think,
Maxime

Maxime Ripard (8):
  drm: Introduce an atomic_commit_setup function
  drm: Document use-after-free gotcha with private objects
  drm/vc4: kms: Move HVS state helpers around
  drm/vc4: kms: Simplify a bit the private obj state hooks
  drm/vc4: Simplify a bit the global atomic_check
  drm/vc4: kms: Wait on previous FIFO users before a commit
  drm/vc4: kms: Remove async modeset semaphore
  drm/vc4: kms: Convert to atomic helpers

 drivers/gpu/drm/drm_atomic_helper.c      |   6 +
 drivers/gpu/drm/vc4/vc4_crtc.c           |  13 --
 drivers/gpu/drm/vc4/vc4_drv.h            |   2 -
 drivers/gpu/drm/vc4/vc4_kms.c            | 269 +++++++++++------------
 include/drm/drm_atomic.h                 |  18 ++
 include/drm/drm_modeset_helper_vtables.h |  18 ++
 6 files changed, 173 insertions(+), 153 deletions(-)

-- 
2.28.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/8] drm: Introduce an atomic_commit_setup function

[Maxime Ripard]

Private objects storing a state shared across all CRTCs need to be
carefully handled to avoid a use-after-free issue.

The proper way to do this to track all the commits using that shared
state and wait for the previous commits to be done before going on with
the current one to avoid the reordering of commits that could occur.

However, this commit setup needs to be done after
drm_atomic_helper_setup_commit(), because before the CRTC commit
structure hasn't been allocated before, and before the workqueue is
scheduled, because we would be potentially reordered already otherwise.

That means that drivers currently have to roll their own
drm_atomic_helper_commit() function, even though it would be identical
if not for the commit setup.

Let's introduce a hook to do so that would be called as part of
drm_atomic_helper_commit, allowing us to reuse the atomic helpers.

Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/drm_atomic_helper.c      |  6 ++++++
 include/drm/drm_modeset_helper_vtables.h | 18 ++++++++++++++++++
 2 files changed, 24 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index ddd0e3239150..7d69c7844dfc 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -2083,8 +2083,11 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
 	struct drm_plane *plane;
 	struct drm_plane_state *old_plane_state, *new_plane_state;
 	struct drm_crtc_commit *commit;
+	const struct drm_mode_config_helper_funcs *funcs;
 	int i, ret;
 
+	funcs = state->dev->mode_config.helper_private;
+
 	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
 		commit = kzalloc(sizeof(*commit), GFP_KERNEL);
 		if (!commit)
@@ -2169,6 +2172,9 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
 		new_plane_state->commit = drm_crtc_commit_get(commit);
 	}
 
+	if (funcs && funcs->atomic_commit_setup)
+		return funcs->atomic_commit_setup(state);
+
 	return 0;
 }
 EXPORT_SYMBOL(drm_atomic_helper_setup_commit);
diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
index f2de050085be..56470baf0513 100644
--- a/include/drm/drm_modeset_helper_vtables.h
+++ b/include/drm/drm_modeset_helper_vtables.h
@@ -1396,6 +1396,24 @@ struct drm_mode_config_helper_funcs {
 	 * drm_atomic_helper_commit_tail().
 	 */
 	void (*atomic_commit_tail)(struct drm_atomic_state *state);
+
+	/**
+	 * @atomic_commit_setup:
+	 *
+	 * This hook is used by the default atomic_commit() hook implemented in
+	 * drm_atomic_helper_commit() together with the nonblocking helpers (see
+	 * drm_atomic_helper_setup_commit()) to extend the DRM commit setup. It
+	 * is not used by the atomic helpers.
+	 *
+	 * This function is called at the end of
+	 * drm_atomic_helper_setup_commit(), so once the commit has been
+	 * properly setup across the generic DRM object states. It allows
+	 * drivers to do some additional commit tracking that isn't related to a
+	 * CRTC, plane or connector, typically a private object.
+	 *
+	 * This hook is optional.
+	 */
+	int (*atomic_commit_setup)(struct drm_atomic_state *state);
 };
 
 #endif
-- 
2.28.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/8] drm: Introduce an atomic_commit_setup function

[Daniel Vetter]

On Fri, Nov 13, 2020 at 04:29:49PM +0100, Maxime Ripard wrote:
> Private objects storing a state shared across all CRTCs need to be
> carefully handled to avoid a use-after-free issue.
> 
> The proper way to do this to track all the commits using that shared
> state and wait for the previous commits to be done before going on with
> the current one to avoid the reordering of commits that could occur.
> 
> However, this commit setup needs to be done after
> drm_atomic_helper_setup_commit(), because before the CRTC commit
> structure hasn't been allocated before, and before the workqueue is
> scheduled, because we would be potentially reordered already otherwise.
> 
> That means that drivers currently have to roll their own
> drm_atomic_helper_commit() function, even though it would be identical
> if not for the commit setup.
> 
> Let's introduce a hook to do so that would be called as part of
> drm_atomic_helper_commit, allowing us to reuse the atomic helpers.
> 
> Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>

Should probably wait with merging until we have the entire vc4 user ready
too. And I think the kerneldoc needs to be further improved, see
suggestions below.

> ---
>  drivers/gpu/drm/drm_atomic_helper.c      |  6 ++++++
>  include/drm/drm_modeset_helper_vtables.h | 18 ++++++++++++++++++
>  2 files changed, 24 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index ddd0e3239150..7d69c7844dfc 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -2083,8 +2083,11 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
>  	struct drm_plane *plane;
>  	struct drm_plane_state *old_plane_state, *new_plane_state;
>  	struct drm_crtc_commit *commit;
> +	const struct drm_mode_config_helper_funcs *funcs;
>  	int i, ret;
>  
> +	funcs = state->dev->mode_config.helper_private;
> +
>  	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
>  		commit = kzalloc(sizeof(*commit), GFP_KERNEL);
>  		if (!commit)
> @@ -2169,6 +2172,9 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
>  		new_plane_state->commit = drm_crtc_commit_get(commit);
>  	}
>  
> +	if (funcs && funcs->atomic_commit_setup)
> +		return funcs->atomic_commit_setup(state);
> +
>  	return 0;
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_setup_commit);
> diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
> index f2de050085be..56470baf0513 100644
> --- a/include/drm/drm_modeset_helper_vtables.h
> +++ b/include/drm/drm_modeset_helper_vtables.h
> @@ -1396,6 +1396,24 @@ struct drm_mode_config_helper_funcs {
>  	 * drm_atomic_helper_commit_tail().
>  	 */
>  	void (*atomic_commit_tail)(struct drm_atomic_state *state);
> +
> +	/**
> +	 * @atomic_commit_setup:
> +	 *
> +	 * This hook is used by the default atomic_commit() hook implemented in
> +	 * drm_atomic_helper_commit() together with the nonblocking helpers (see
> +	 * drm_atomic_helper_setup_commit()) to extend the DRM commit setup. It

I think a link from drm_atomic_helper_setup_commit to this new hook here
would be useful too, maybe together with a note that waiting for these
additional synchronization points can be done at the start of
atomic_commit_tail.

> +	 * is not used by the atomic helpers.
> +	 *
> +	 * This function is called at the end of
> +	 * drm_atomic_helper_setup_commit(), so once the commit has been
> +	 * properly setup across the generic DRM object states. It allows
> +	 * drivers to do some additional commit tracking that isn't related to a
> +	 * CRTC, plane or connector, typically a private object.
> +	 *
> +	 * This hook is optional.
> +	 */
> +	int (*atomic_commit_setup)(struct drm_atomic_state *state);

I think the kerneldoc for drm_private_obj should also explain when it is
necessary to use this, and when not:

- when the private state is a tied to an existing drm object (drm_crtc,
  drm_plane or drm_crtc) and never moves, then sufficient synchronization
  is already guaranteed by that superior object. This could even hold
  when the private object can be e.g. reassigned between planes, but
  always stays on the same crtc.

- if the private state object can be globally reassigned, then
  drm_crtc_commit synchronization points need to be set up in
  ->atomic_commit_setup and waited on as the first step in
  ->atomic_commit_tail


Also I just realized that a drm_private_state->obj backpointer to
drm_private_obj might be rather useful. Or at least more consistent with
all the other state objects.

Cheers, Daniel


>  };
>  
>  #endif
> -- 
> 2.28.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/8] drm: Introduce an atomic_commit_setup function

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 1407 bytes --]

Hi Daniel,

Thanks for your review

On Fri, Nov 13, 2020 at 10:02:40PM +0100, Daniel Vetter wrote:
> > +	 * is not used by the atomic helpers.
> > +	 *
> > +	 * This function is called at the end of
> > +	 * drm_atomic_helper_setup_commit(), so once the commit has been
> > +	 * properly setup across the generic DRM object states. It allows
> > +	 * drivers to do some additional commit tracking that isn't related to a
> > +	 * CRTC, plane or connector, typically a private object.
> > +	 *
> > +	 * This hook is optional.
> > +	 */
> > +	int (*atomic_commit_setup)(struct drm_atomic_state *state);
> 
> I think the kerneldoc for drm_private_obj should also explain when it is
> necessary to use this, and when not:
> 
> - when the private state is a tied to an existing drm object (drm_crtc,
>   drm_plane or drm_crtc) and never moves, then sufficient synchronization
>   is already guaranteed by that superior object. This could even hold
>   when the private object can be e.g. reassigned between planes, but
>   always stays on the same crtc.
> 
> - if the private state object can be globally reassigned, then
>   drm_crtc_commit synchronization points need to be set up in
>   ->atomic_commit_setup and waited on as the first step in
>   ->atomic_commit_tail

Does the comment added on patch 2 sufficient for you, or would you
extend it / make it clearer?

Maxime

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/8] drm: Introduce an atomic_commit_setup function

[Daniel Vetter]

On Fri, Nov 20, 2020 at 2:34 PM Maxime Ripard <maxime@cerno.tech> wrote:
>
> Hi Daniel,
>
> Thanks for your review
>
> On Fri, Nov 13, 2020 at 10:02:40PM +0100, Daniel Vetter wrote:
> > > +    * is not used by the atomic helpers.
> > > +    *
> > > +    * This function is called at the end of
> > > +    * drm_atomic_helper_setup_commit(), so once the commit has been
> > > +    * properly setup across the generic DRM object states. It allows
> > > +    * drivers to do some additional commit tracking that isn't related to a
> > > +    * CRTC, plane or connector, typically a private object.
> > > +    *
> > > +    * This hook is optional.
> > > +    */
> > > +   int (*atomic_commit_setup)(struct drm_atomic_state *state);
> >
> > I think the kerneldoc for drm_private_obj should also explain when it is
> > necessary to use this, and when not:
> >
> > - when the private state is a tied to an existing drm object (drm_crtc,
> >   drm_plane or drm_crtc) and never moves, then sufficient synchronization
> >   is already guaranteed by that superior object. This could even hold
> >   when the private object can be e.g. reassigned between planes, but
> >   always stays on the same crtc.
> >
> > - if the private state object can be globally reassigned, then
> >   drm_crtc_commit synchronization points need to be set up in
> >   ->atomic_commit_setup and waited on as the first step in
> >   ->atomic_commit_tail
>
> Does the comment added on patch 2 sufficient for you, or would you
> extend it / make it clearer?

Lol stateless reviewer. Yeah I think the text there is good, but we
probably want to make sure there's links to all the other pieces in
all the places. So maybe replace "typically in a private object" with
"tracked in a struct drm_private_obj" so we get that link. And maybe a
note to look there for additional information.

Same in the other places. In generally I think you can never have too
many links in kerneldoc, since they're both useful in the generated
html, but also for navigating the code with cscope or similar (at
least here this works splendidly).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/8] drm: Introduce an atomic_commit_setup function

[Thomas Zimmermann]

[-- Attachment #1.1.1.1: Type: text/plain, Size: 4778 bytes --]

Hi

Am 13.11.20 um 16:29 schrieb Maxime Ripard:
> Private objects storing a state shared across all CRTCs need to be
> carefully handled to avoid a use-after-free issue.
> 
> The proper way to do this to track all the commits using that shared
> state and wait for the previous commits to be done before going on with
> the current one to avoid the reordering of commits that could occur.
> 
> However, this commit setup needs to be done after
> drm_atomic_helper_setup_commit(), because before the CRTC commit
> structure hasn't been allocated before, and before the workqueue is
> scheduled, because we would be potentially reordered already otherwise.
> 
> That means that drivers currently have to roll their own
> drm_atomic_helper_commit() function, even though it would be identical
> if not for the commit setup.
> 
> Let's introduce a hook to do so that would be called as part of
> drm_atomic_helper_commit, allowing us to reuse the atomic helpers.
> 
> Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---
>   drivers/gpu/drm/drm_atomic_helper.c      |  6 ++++++
>   include/drm/drm_modeset_helper_vtables.h | 18 ++++++++++++++++++
>   2 files changed, 24 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index ddd0e3239150..7d69c7844dfc 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -2083,8 +2083,11 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
>   	struct drm_plane *plane;
>   	struct drm_plane_state *old_plane_state, *new_plane_state;
>   	struct drm_crtc_commit *commit;
> +	const struct drm_mode_config_helper_funcs *funcs;
>   	int i, ret;
>   
> +	funcs = state->dev->mode_config.helper_private;
> +
>   	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
>   		commit = kzalloc(sizeof(*commit), GFP_KERNEL);
>   		if (!commit)
> @@ -2169,6 +2172,9 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
>   		new_plane_state->commit = drm_crtc_commit_get(commit);
>   	}
>   
> +	if (funcs && funcs->atomic_commit_setup)
> +		return funcs->atomic_commit_setup(state);
> +
>   	return 0;
>   }
>   EXPORT_SYMBOL(drm_atomic_helper_setup_commit);
> diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
> index f2de050085be..56470baf0513 100644
> --- a/include/drm/drm_modeset_helper_vtables.h
> +++ b/include/drm/drm_modeset_helper_vtables.h
> @@ -1396,6 +1396,24 @@ struct drm_mode_config_helper_funcs {
>   	 * drm_atomic_helper_commit_tail().
>   	 */
>   	void (*atomic_commit_tail)(struct drm_atomic_state *state);
> +
> +	/**
> +	 * @atomic_commit_setup:
> +	 *
> +	 * This hook is used by the default atomic_commit() hook implemented in
> +	 * drm_atomic_helper_commit() together with the nonblocking helpers (see
> +	 * drm_atomic_helper_setup_commit()) to extend the DRM commit setup. It
> +	 * is not used by the atomic helpers.
> +	 *
> +	 * This function is called at the end of
> +	 * drm_atomic_helper_setup_commit(), so once the commit has been
> +	 * properly setup across the generic DRM object states. It allows
> +	 * drivers to do some additional commit tracking that isn't related to a
> +	 * CRTC, plane or connector, typically a private object.
> +	 *
> +	 * This hook is optional.
> +	 */
> +	int (*atomic_commit_setup)(struct drm_atomic_state *state);

It feels hacky and screwed-on to me. I'd suggest to add an 
atomic_commit_prepare callback that is called by drm_atomic_helper where 
it currently calls drm_atomic_helper_setup_commit(). The default 
implementation  would include setup_commit and prepare_planes. Some 
example code for drm_atomic_helper.c

static int commit_prepare(state)
{
	drm_atomic_helper_setup_commit(state)

	drm_atomic_helper_prepare_planes(state)
}

int drm_atomic_helper_commit()
{
	if (async_update) {
		...
	}

	if (funcs->atomic_commit_prepare)
		funcs->atomic_commit_prepare(state)
	else
		commit_prepare(state)

	/* the rest of the current function below */
	INIT_WORK(&state->commit_work, commit_work);
	...
}

Drivers that implement atomic_commit_prepare would be expected to call 
drm_atomic_helper_setup_commit() and drm_atomic_helper_prepare_planes() 
or their own implementation of them.

The whole construct mimics how commit tails work.

Best regards
Thomas


>   };
>   
>   #endif
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer

[-- Attachment #1.1.1.2: OpenPGP_0x680DC11D530B7A23.asc --]
[-- Type: application/pgp-keys, Size: 7535 bytes --]

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/8] drm: Introduce an atomic_commit_setup function

[Daniel Vetter]

On Thu, Nov 19, 2020 at 10:59:42AM +0100, Thomas Zimmermann wrote:
> Hi
> 
> Am 13.11.20 um 16:29 schrieb Maxime Ripard:
> > Private objects storing a state shared across all CRTCs need to be
> > carefully handled to avoid a use-after-free issue.
> > 
> > The proper way to do this to track all the commits using that shared
> > state and wait for the previous commits to be done before going on with
> > the current one to avoid the reordering of commits that could occur.
> > 
> > However, this commit setup needs to be done after
> > drm_atomic_helper_setup_commit(), because before the CRTC commit
> > structure hasn't been allocated before, and before the workqueue is
> > scheduled, because we would be potentially reordered already otherwise.
> > 
> > That means that drivers currently have to roll their own
> > drm_atomic_helper_commit() function, even though it would be identical
> > if not for the commit setup.
> > 
> > Let's introduce a hook to do so that would be called as part of
> > drm_atomic_helper_commit, allowing us to reuse the atomic helpers.
> > 
> > Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> > ---
> >   drivers/gpu/drm/drm_atomic_helper.c      |  6 ++++++
> >   include/drm/drm_modeset_helper_vtables.h | 18 ++++++++++++++++++
> >   2 files changed, 24 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > index ddd0e3239150..7d69c7844dfc 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -2083,8 +2083,11 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
> >   	struct drm_plane *plane;
> >   	struct drm_plane_state *old_plane_state, *new_plane_state;
> >   	struct drm_crtc_commit *commit;
> > +	const struct drm_mode_config_helper_funcs *funcs;
> >   	int i, ret;
> > +	funcs = state->dev->mode_config.helper_private;
> > +
> >   	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
> >   		commit = kzalloc(sizeof(*commit), GFP_KERNEL);
> >   		if (!commit)
> > @@ -2169,6 +2172,9 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
> >   		new_plane_state->commit = drm_crtc_commit_get(commit);
> >   	}
> > +	if (funcs && funcs->atomic_commit_setup)
> > +		return funcs->atomic_commit_setup(state);
> > +
> >   	return 0;
> >   }
> >   EXPORT_SYMBOL(drm_atomic_helper_setup_commit);
> > diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
> > index f2de050085be..56470baf0513 100644
> > --- a/include/drm/drm_modeset_helper_vtables.h
> > +++ b/include/drm/drm_modeset_helper_vtables.h
> > @@ -1396,6 +1396,24 @@ struct drm_mode_config_helper_funcs {
> >   	 * drm_atomic_helper_commit_tail().
> >   	 */
> >   	void (*atomic_commit_tail)(struct drm_atomic_state *state);
> > +
> > +	/**
> > +	 * @atomic_commit_setup:
> > +	 *
> > +	 * This hook is used by the default atomic_commit() hook implemented in
> > +	 * drm_atomic_helper_commit() together with the nonblocking helpers (see
> > +	 * drm_atomic_helper_setup_commit()) to extend the DRM commit setup. It
> > +	 * is not used by the atomic helpers.
> > +	 *
> > +	 * This function is called at the end of
> > +	 * drm_atomic_helper_setup_commit(), so once the commit has been
> > +	 * properly setup across the generic DRM object states. It allows
> > +	 * drivers to do some additional commit tracking that isn't related to a
> > +	 * CRTC, plane or connector, typically a private object.
> > +	 *
> > +	 * This hook is optional.
> > +	 */
> > +	int (*atomic_commit_setup)(struct drm_atomic_state *state);
> 
> It feels hacky and screwed-on to me. I'd suggest to add an
> atomic_commit_prepare callback that is called by drm_atomic_helper where it
> currently calls drm_atomic_helper_setup_commit(). The default implementation
> would include setup_commit and prepare_planes. Some example code for
> drm_atomic_helper.c
> 
> static int commit_prepare(state)
> {
> 	drm_atomic_helper_setup_commit(state)
> 
> 	drm_atomic_helper_prepare_planes(state)
> }
> 
> int drm_atomic_helper_commit()
> {
> 	if (async_update) {
> 		...
> 	}
> 
> 	if (funcs->atomic_commit_prepare)
> 		funcs->atomic_commit_prepare(state)
> 	else
> 		commit_prepare(state)
> 
> 	/* the rest of the current function below */
> 	INIT_WORK(&state->commit_work, commit_work);
> 	...
> }
> 
> Drivers that implement atomic_commit_prepare would be expected to call
> drm_atomic_helper_setup_commit() and drm_atomic_helper_prepare_planes() or
> their own implementation of them.
> 
> The whole construct mimics how commit tails work.

Yeah what I suggested is a bit much midlayer, but we've done what you
suggested above with all drivers rolling their own atomic_commit. It
wasn't pretty. There's still drivers like vc4 which haven't switched, and
which have their own locking and synchronization.

Hence why I think the callback approach here is better, gives drivers less
excuses to roll their own and make a mess.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/8] drm: Introduce an atomic_commit_setup function

[Thomas Zimmermann]

[-- Attachment #1.1.1.1: Type: text/plain, Size: 5804 bytes --]

Hi

Am 19.11.20 um 16:32 schrieb Daniel Vetter:
> On Thu, Nov 19, 2020 at 10:59:42AM +0100, Thomas Zimmermann wrote:
>> Hi
>>
>> Am 13.11.20 um 16:29 schrieb Maxime Ripard:
>>> Private objects storing a state shared across all CRTCs need to be
>>> carefully handled to avoid a use-after-free issue.
>>>
>>> The proper way to do this to track all the commits using that shared
>>> state and wait for the previous commits to be done before going on with
>>> the current one to avoid the reordering of commits that could occur.
>>>
>>> However, this commit setup needs to be done after
>>> drm_atomic_helper_setup_commit(), because before the CRTC commit
>>> structure hasn't been allocated before, and before the workqueue is
>>> scheduled, because we would be potentially reordered already otherwise.
>>>
>>> That means that drivers currently have to roll their own
>>> drm_atomic_helper_commit() function, even though it would be identical
>>> if not for the commit setup.
>>>
>>> Let's introduce a hook to do so that would be called as part of
>>> drm_atomic_helper_commit, allowing us to reuse the atomic helpers.
>>>
>>> Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
>>> ---
>>>    drivers/gpu/drm/drm_atomic_helper.c      |  6 ++++++
>>>    include/drm/drm_modeset_helper_vtables.h | 18 ++++++++++++++++++
>>>    2 files changed, 24 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
>>> index ddd0e3239150..7d69c7844dfc 100644
>>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>>> @@ -2083,8 +2083,11 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
>>>    	struct drm_plane *plane;
>>>    	struct drm_plane_state *old_plane_state, *new_plane_state;
>>>    	struct drm_crtc_commit *commit;
>>> +	const struct drm_mode_config_helper_funcs *funcs;
>>>    	int i, ret;
>>> +	funcs = state->dev->mode_config.helper_private;
>>> +
>>>    	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
>>>    		commit = kzalloc(sizeof(*commit), GFP_KERNEL);
>>>    		if (!commit)
>>> @@ -2169,6 +2172,9 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
>>>    		new_plane_state->commit = drm_crtc_commit_get(commit);
>>>    	}
>>> +	if (funcs && funcs->atomic_commit_setup)
>>> +		return funcs->atomic_commit_setup(state);
>>> +
>>>    	return 0;
>>>    }
>>>    EXPORT_SYMBOL(drm_atomic_helper_setup_commit);
>>> diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
>>> index f2de050085be..56470baf0513 100644
>>> --- a/include/drm/drm_modeset_helper_vtables.h
>>> +++ b/include/drm/drm_modeset_helper_vtables.h
>>> @@ -1396,6 +1396,24 @@ struct drm_mode_config_helper_funcs {
>>>    	 * drm_atomic_helper_commit_tail().
>>>    	 */
>>>    	void (*atomic_commit_tail)(struct drm_atomic_state *state);
>>> +
>>> +	/**
>>> +	 * @atomic_commit_setup:
>>> +	 *
>>> +	 * This hook is used by the default atomic_commit() hook implemented in
>>> +	 * drm_atomic_helper_commit() together with the nonblocking helpers (see
>>> +	 * drm_atomic_helper_setup_commit()) to extend the DRM commit setup. It
>>> +	 * is not used by the atomic helpers.
>>> +	 *
>>> +	 * This function is called at the end of
>>> +	 * drm_atomic_helper_setup_commit(), so once the commit has been
>>> +	 * properly setup across the generic DRM object states. It allows
>>> +	 * drivers to do some additional commit tracking that isn't related to a
>>> +	 * CRTC, plane or connector, typically a private object.
>>> +	 *
>>> +	 * This hook is optional.
>>> +	 */
>>> +	int (*atomic_commit_setup)(struct drm_atomic_state *state);
>>
>> It feels hacky and screwed-on to me. I'd suggest to add an
>> atomic_commit_prepare callback that is called by drm_atomic_helper where it
>> currently calls drm_atomic_helper_setup_commit(). The default implementation
>> would include setup_commit and prepare_planes. Some example code for
>> drm_atomic_helper.c
>>
>> static int commit_prepare(state)
>> {
>> 	drm_atomic_helper_setup_commit(state)
>>
>> 	drm_atomic_helper_prepare_planes(state)
>> }
>>
>> int drm_atomic_helper_commit()
>> {
>> 	if (async_update) {
>> 		...
>> 	}
>>
>> 	if (funcs->atomic_commit_prepare)
>> 		funcs->atomic_commit_prepare(state)
>> 	else
>> 		commit_prepare(state)
>>
>> 	/* the rest of the current function below */
>> 	INIT_WORK(&state->commit_work, commit_work);
>> 	...
>> }
>>
>> Drivers that implement atomic_commit_prepare would be expected to call
>> drm_atomic_helper_setup_commit() and drm_atomic_helper_prepare_planes() or
>> their own implementation of them.
>>
>> The whole construct mimics how commit tails work.
> 
> Yeah what I suggested is a bit much midlayer, but we've done what you
> suggested above with all drivers rolling their own atomic_commit. It
> wasn't pretty. There's still drivers like vc4 which haven't switched, and
> which have their own locking and synchronization.
> 
> Hence why I think the callback approach here is better, gives drivers less
> excuses to roll their own and make a mess.

But it sounds like you'll regret this. As soon as a driver has to do 
additional stuff at the beginning of the setup function, another helper 
will be required, and so on. Maybe rather go with the commit_prepare 
helper and push driver authors to not use it.

Best regards
Thomas

> -Daniel
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer

[-- Attachment #1.1.1.2: OpenPGP_0x680DC11D530B7A23.asc --]
[-- Type: application/pgp-keys, Size: 7535 bytes --]

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/8] drm: Introduce an atomic_commit_setup function

[Daniel Vetter]

On Fri, Nov 20, 2020 at 9:39 AM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> Hi
>
> Am 19.11.20 um 16:32 schrieb Daniel Vetter:
> > On Thu, Nov 19, 2020 at 10:59:42AM +0100, Thomas Zimmermann wrote:
> >> Hi
> >>
> >> Am 13.11.20 um 16:29 schrieb Maxime Ripard:
> >>> Private objects storing a state shared across all CRTCs need to be
> >>> carefully handled to avoid a use-after-free issue.
> >>>
> >>> The proper way to do this to track all the commits using that shared
> >>> state and wait for the previous commits to be done before going on with
> >>> the current one to avoid the reordering of commits that could occur.
> >>>
> >>> However, this commit setup needs to be done after
> >>> drm_atomic_helper_setup_commit(), because before the CRTC commit
> >>> structure hasn't been allocated before, and before the workqueue is
> >>> scheduled, because we would be potentially reordered already otherwise.
> >>>
> >>> That means that drivers currently have to roll their own
> >>> drm_atomic_helper_commit() function, even though it would be identical
> >>> if not for the commit setup.
> >>>
> >>> Let's introduce a hook to do so that would be called as part of
> >>> drm_atomic_helper_commit, allowing us to reuse the atomic helpers.
> >>>
> >>> Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> >>> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> >>> ---
> >>>    drivers/gpu/drm/drm_atomic_helper.c      |  6 ++++++
> >>>    include/drm/drm_modeset_helper_vtables.h | 18 ++++++++++++++++++
> >>>    2 files changed, 24 insertions(+)
> >>>
> >>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> >>> index ddd0e3239150..7d69c7844dfc 100644
> >>> --- a/drivers/gpu/drm/drm_atomic_helper.c
> >>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> >>> @@ -2083,8 +2083,11 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
> >>>     struct drm_plane *plane;
> >>>     struct drm_plane_state *old_plane_state, *new_plane_state;
> >>>     struct drm_crtc_commit *commit;
> >>> +   const struct drm_mode_config_helper_funcs *funcs;
> >>>     int i, ret;
> >>> +   funcs = state->dev->mode_config.helper_private;
> >>> +
> >>>     for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
> >>>             commit = kzalloc(sizeof(*commit), GFP_KERNEL);
> >>>             if (!commit)
> >>> @@ -2169,6 +2172,9 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
> >>>             new_plane_state->commit = drm_crtc_commit_get(commit);
> >>>     }
> >>> +   if (funcs && funcs->atomic_commit_setup)
> >>> +           return funcs->atomic_commit_setup(state);
> >>> +
> >>>     return 0;
> >>>    }
> >>>    EXPORT_SYMBOL(drm_atomic_helper_setup_commit);
> >>> diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
> >>> index f2de050085be..56470baf0513 100644
> >>> --- a/include/drm/drm_modeset_helper_vtables.h
> >>> +++ b/include/drm/drm_modeset_helper_vtables.h
> >>> @@ -1396,6 +1396,24 @@ struct drm_mode_config_helper_funcs {
> >>>      * drm_atomic_helper_commit_tail().
> >>>      */
> >>>     void (*atomic_commit_tail)(struct drm_atomic_state *state);
> >>> +
> >>> +   /**
> >>> +    * @atomic_commit_setup:
> >>> +    *
> >>> +    * This hook is used by the default atomic_commit() hook implemented in
> >>> +    * drm_atomic_helper_commit() together with the nonblocking helpers (see
> >>> +    * drm_atomic_helper_setup_commit()) to extend the DRM commit setup. It
> >>> +    * is not used by the atomic helpers.
> >>> +    *
> >>> +    * This function is called at the end of
> >>> +    * drm_atomic_helper_setup_commit(), so once the commit has been
> >>> +    * properly setup across the generic DRM object states. It allows
> >>> +    * drivers to do some additional commit tracking that isn't related to a
> >>> +    * CRTC, plane or connector, typically a private object.
> >>> +    *
> >>> +    * This hook is optional.
> >>> +    */
> >>> +   int (*atomic_commit_setup)(struct drm_atomic_state *state);
> >>
> >> It feels hacky and screwed-on to me. I'd suggest to add an
> >> atomic_commit_prepare callback that is called by drm_atomic_helper where it
> >> currently calls drm_atomic_helper_setup_commit(). The default implementation
> >> would include setup_commit and prepare_planes. Some example code for
> >> drm_atomic_helper.c
> >>
> >> static int commit_prepare(state)
> >> {
> >>      drm_atomic_helper_setup_commit(state)
> >>
> >>      drm_atomic_helper_prepare_planes(state)
> >> }
> >>
> >> int drm_atomic_helper_commit()
> >> {
> >>      if (async_update) {
> >>              ...
> >>      }
> >>
> >>      if (funcs->atomic_commit_prepare)
> >>              funcs->atomic_commit_prepare(state)
> >>      else
> >>              commit_prepare(state)
> >>
> >>      /* the rest of the current function below */
> >>      INIT_WORK(&state->commit_work, commit_work);
> >>      ...
> >> }
> >>
> >> Drivers that implement atomic_commit_prepare would be expected to call
> >> drm_atomic_helper_setup_commit() and drm_atomic_helper_prepare_planes() or
> >> their own implementation of them.
> >>
> >> The whole construct mimics how commit tails work.
> >
> > Yeah what I suggested is a bit much midlayer, but we've done what you
> > suggested above with all drivers rolling their own atomic_commit. It
> > wasn't pretty. There's still drivers like vc4 which haven't switched, and
> > which have their own locking and synchronization.
> >
> > Hence why I think the callback approach here is better, gives drivers less
> > excuses to roll their own and make a mess.
>
> But it sounds like you'll regret this. As soon as a driver has to do
> additional stuff at the beginning of the setup function, another helper
> will be required, and so on. Maybe rather go with the commit_prepare
> helper and push driver authors to not use it.

For the other thing we already have callbacks (it's prepare_plane).
The use case for this is also fairly minimal (and this should be clear
when the kerneldoc is fully updated).

The thing is, avoiding the midlayer mistake doesn't mean no callbacks.
It just means the topmost entry point should be a driver callback too,
and ideally all the pieces are still fairly modular. We check all
these boxes.

Your option otoh means a bunch more code in vc4 (after Maxime's series
is done) for not much reason. Plus I'm really not seeing the concern.
Also, rule of thumb is to do clean design when we have 3 cases, and
hack things up for the first 2. We're at 1.

Also note that the 2nd part of this is also not in the
atomic_commit_tail callback. But we get away with that because the
driver handling can be done at the top of atomic_commit_tail, hence
there's no need for an atomic_commit_wait_for_dependencies.
-Daniel

>
> Best regards
> Thomas
>
> > -Daniel
> >
>
> --
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> (HRB 36809, AG Nürnberg)
> Geschäftsführer: Felix Imendörffer



-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/8] drm: Document use-after-free gotcha with private objects

[Maxime Ripard]

The private objects have a gotcha that could result in a use-after-free,
make sure it's properly documented.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 include/drm/drm_atomic.h | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index 413fd0ca56a8..24b52b3a459f 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -248,6 +248,24 @@ struct drm_private_state_funcs {
  *    drm_dev_register()
  * 2/ all calls to drm_atomic_private_obj_fini() must be done after calling
  *    drm_dev_unregister()
+ *
+ * If that private object is used to store a state shared my multiple
+ * CRTCs, proper care must be taken to ensure that non-blocking commits are
+ * properly ordered to avoid a use-after-free issue.
+ *
+ * Indeed, assuming a sequence of two non-blocking commits on two different
+ * CRTCs using different planes and connectors, so with no resources shared,
+ * there's no guarantee on which commit is going to happen first. However, the
+ * second commit will consider the first private state its old state, and will
+ * be in charge of freeing it whenever the second commit is done.
+ *
+ * If the first commit happens after it, it will consider its private state the
+ * new state and will be likely to access it, resulting in an access to a freed
+ * memory region. A way to circumvent this is to store (and get a reference to)
+ * the crtc commit in our private state in
+ * &drm_mode_config_helper_funcs.atomic_commit_setup, and then wait for that
+ * commit to complete as part of
+ * &drm_mode_config_helper_funcs.atomic_commit_tail.
  */
 struct drm_private_obj {
 	/**
-- 
2.28.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/8] drm: Document use-after-free gotcha with private objects

[Daniel Vetter]

On Fri, Nov 13, 2020 at 04:29:50PM +0100, Maxime Ripard wrote:
> The private objects have a gotcha that could result in a use-after-free,
> make sure it's properly documented.
> 
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---
>  include/drm/drm_atomic.h | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index 413fd0ca56a8..24b52b3a459f 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -248,6 +248,24 @@ struct drm_private_state_funcs {
>   *    drm_dev_register()
>   * 2/ all calls to drm_atomic_private_obj_fini() must be done after calling
>   *    drm_dev_unregister()
> + *
> + * If that private object is used to store a state shared my multiple

s/my/by/

> + * CRTCs, proper care must be taken to ensure that non-blocking commits are
> + * properly ordered to avoid a use-after-free issue.
> + *
> + * Indeed, assuming a sequence of two non-blocking commits on two different
> + * CRTCs using different planes and connectors, so with no resources shared,
> + * there's no guarantee on which commit is going to happen first. However, the
> + * second commit will consider the first private state its old state, and will
> + * be in charge of freeing it whenever the second commit is done.
> + *
> + * If the first commit happens after it, it will consider its private state the
> + * new state and will be likely to access it, resulting in an access to a freed
> + * memory region. A way to circumvent this is to store (and get a reference to)

s/A way to circumvent/Driver should/

And maybe make the paragraph break here and remove the previous one in the
middle of your example.

> + * the crtc commit in our private state in

&struct drm_crtc_commit so it's linked properly

> + * &drm_mode_config_helper_funcs.atomic_commit_setup, and then wait for that
> + * commit to complete as part of

s/as part of/as the first step of/
> + * &drm_mode_config_helper_funcs.atomic_commit_tail.

And maybe add "... similar to drm_atomic_helper_wait_for_dependencies()"

With the nits addressed:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>



>   */
>  struct drm_private_obj {
>  	/**
> -- 
> 2.28.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/8] drm/vc4: kms: Move HVS state helpers around

[Maxime Ripard]

We're going to use those helpers in functions higher in that file, let's
move it around.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/vc4/vc4_kms.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
index 7ef164afa9e2..d6712924681e 100644
--- a/drivers/gpu/drm/vc4/vc4_kms.c
+++ b/drivers/gpu/drm/vc4/vc4_kms.c
@@ -182,6 +182,19 @@ vc4_ctm_commit(struct vc4_dev *vc4, struct drm_atomic_state *state)
 		  VC4_SET_FIELD(ctm_state->fifo, SCALER_OLEDOFFS_DISPFIFO));
 }
 
+static struct vc4_hvs_state *
+vc4_hvs_get_global_state(struct drm_atomic_state *state)
+{
+	struct vc4_dev *vc4 = to_vc4_dev(state->dev);
+	struct drm_private_state *priv_state;
+
+	priv_state = drm_atomic_get_private_obj_state(state, &vc4->hvs_channels);
+	if (IS_ERR(priv_state))
+		return ERR_CAST(priv_state);
+
+	return to_vc4_hvs_state(priv_state);
+}
+
 static void vc4_hvs_pv_muxing_commit(struct vc4_dev *vc4,
 				     struct drm_atomic_state *state)
 {
@@ -730,19 +743,6 @@ static int vc4_hvs_channels_obj_init(struct vc4_dev *vc4)
 	return drmm_add_action_or_reset(&vc4->base, vc4_hvs_channels_obj_fini, NULL);
 }
 
-static struct vc4_hvs_state *
-vc4_hvs_get_global_state(struct drm_atomic_state *state)
-{
-	struct vc4_dev *vc4 = to_vc4_dev(state->dev);
-	struct drm_private_state *priv_state;
-
-	priv_state = drm_atomic_get_private_obj_state(state, &vc4->hvs_channels);
-	if (IS_ERR(priv_state))
-		return ERR_CAST(priv_state);
-
-	return to_vc4_hvs_state(priv_state);
-}
-
 /*
  * The BCM2711 HVS has up to 7 output connected to the pixelvalves and
  * the TXP (and therefore all the CRTCs found on that platform).
-- 
2.28.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/8] drm/vc4: kms: Move HVS state helpers around

[Thomas Zimmermann]

[-- Attachment #1.1.1.1: Type: text/plain, Size: 2217 bytes --]

I'd merge this into the patch that introduces the function.

Am 13.11.20 um 16:29 schrieb Maxime Ripard:
> We're going to use those helpers in functions higher in that file, let's
> move it around.
> 
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---
>   drivers/gpu/drm/vc4/vc4_kms.c | 26 +++++++++++++-------------
>   1 file changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
> index 7ef164afa9e2..d6712924681e 100644
> --- a/drivers/gpu/drm/vc4/vc4_kms.c
> +++ b/drivers/gpu/drm/vc4/vc4_kms.c
> @@ -182,6 +182,19 @@ vc4_ctm_commit(struct vc4_dev *vc4, struct drm_atomic_state *state)
>   		  VC4_SET_FIELD(ctm_state->fifo, SCALER_OLEDOFFS_DISPFIFO));
>   }
>   
> +static struct vc4_hvs_state *
> +vc4_hvs_get_global_state(struct drm_atomic_state *state)
> +{
> +	struct vc4_dev *vc4 = to_vc4_dev(state->dev);
> +	struct drm_private_state *priv_state;
> +
> +	priv_state = drm_atomic_get_private_obj_state(state, &vc4->hvs_channels);
> +	if (IS_ERR(priv_state))
> +		return ERR_CAST(priv_state);
> +
> +	return to_vc4_hvs_state(priv_state);
> +}
> +
>   static void vc4_hvs_pv_muxing_commit(struct vc4_dev *vc4,
>   				     struct drm_atomic_state *state)
>   {
> @@ -730,19 +743,6 @@ static int vc4_hvs_channels_obj_init(struct vc4_dev *vc4)
>   	return drmm_add_action_or_reset(&vc4->base, vc4_hvs_channels_obj_fini, NULL);
>   }
>   
> -static struct vc4_hvs_state *
> -vc4_hvs_get_global_state(struct drm_atomic_state *state)
> -{
> -	struct vc4_dev *vc4 = to_vc4_dev(state->dev);
> -	struct drm_private_state *priv_state;
> -
> -	priv_state = drm_atomic_get_private_obj_state(state, &vc4->hvs_channels);
> -	if (IS_ERR(priv_state))
> -		return ERR_CAST(priv_state);
> -
> -	return to_vc4_hvs_state(priv_state);
> -}
> -
>   /*
>    * The BCM2711 HVS has up to 7 output connected to the pixelvalves and
>    * the TXP (and therefore all the CRTCs found on that platform).
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer

[-- Attachment #1.1.1.2: OpenPGP_0x680DC11D530B7A23.asc --]
[-- Type: application/pgp-keys, Size: 7535 bytes --]

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 4/8] drm/vc4: kms: Simplify a bit the private obj state hooks

[Maxime Ripard]

Some fields that we're going to add cannot be just copied over to the
new state, and thus kmemdup is a bit unnecessary. Let's move to kzalloc
instead, and clean it up in the process.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/vc4/vc4_kms.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
index d6712924681e..3d0065df10f9 100644
--- a/drivers/gpu/drm/vc4/vc4_kms.c
+++ b/drivers/gpu/drm/vc4/vc4_kms.c
@@ -695,23 +695,25 @@ static int vc4_load_tracker_obj_init(struct vc4_dev *vc4)
 static struct drm_private_state *
 vc4_hvs_channels_duplicate_state(struct drm_private_obj *obj)
 {
+	struct vc4_hvs_state *old_state = to_vc4_hvs_state(obj->state);
 	struct vc4_hvs_state *state;
 
-	state = kmemdup(obj->state, sizeof(*state), GFP_KERNEL);
+	state = kzalloc(sizeof(*state), GFP_KERNEL);
 	if (!state)
 		return NULL;
 
 	__drm_atomic_helper_private_obj_duplicate_state(obj, &state->base);
 
+	state->unassigned_channels = old_state->unassigned_channels;
+
 	return &state->base;
 }
 
 static void vc4_hvs_channels_destroy_state(struct drm_private_obj *obj,
 					   struct drm_private_state *state)
 {
-	struct vc4_hvs_state *hvs_state;
+	struct vc4_hvs_state *hvs_state = to_vc4_hvs_state(state);
 
-	hvs_state = to_vc4_hvs_state(state);
 	kfree(hvs_state);
 }
 
-- 
2.28.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/8] drm/vc4: kms: Simplify a bit the private obj state hooks

[Thomas Zimmermann]

[-- Attachment #1.1.1.1: Type: text/plain, Size: 1847 bytes --]

Maybe merge this into the commit that introduces the functionality.

Am 13.11.20 um 16:29 schrieb Maxime Ripard:
> Some fields that we're going to add cannot be just copied over to the
> new state, and thus kmemdup is a bit unnecessary. Let's move to kzalloc
> instead, and clean it up in the process.
> 
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---
>   drivers/gpu/drm/vc4/vc4_kms.c | 8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
> index d6712924681e..3d0065df10f9 100644
> --- a/drivers/gpu/drm/vc4/vc4_kms.c
> +++ b/drivers/gpu/drm/vc4/vc4_kms.c
> @@ -695,23 +695,25 @@ static int vc4_load_tracker_obj_init(struct vc4_dev *vc4)
>   static struct drm_private_state *
>   vc4_hvs_channels_duplicate_state(struct drm_private_obj *obj)
>   {
> +	struct vc4_hvs_state *old_state = to_vc4_hvs_state(obj->state);
>   	struct vc4_hvs_state *state;
>   
> -	state = kmemdup(obj->state, sizeof(*state), GFP_KERNEL);
> +	state = kzalloc(sizeof(*state), GFP_KERNEL);
>   	if (!state)
>   		return NULL;
>   
>   	__drm_atomic_helper_private_obj_duplicate_state(obj, &state->base);
>   
> +	state->unassigned_channels = old_state->unassigned_channels;
> +
>   	return &state->base;
>   }
>   
>   static void vc4_hvs_channels_destroy_state(struct drm_private_obj *obj,
>   					   struct drm_private_state *state)
>   {
> -	struct vc4_hvs_state *hvs_state;
> +	struct vc4_hvs_state *hvs_state = to_vc4_hvs_state(state);
>   
> -	hvs_state = to_vc4_hvs_state(state);
>   	kfree(hvs_state);
>   }
>   
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer

[-- Attachment #1.1.1.2: OpenPGP_0x680DC11D530B7A23.asc --]
[-- Type: application/pgp-keys, Size: 7535 bytes --]

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 5/8] drm/vc4: Simplify a bit the global atomic_check

[Maxime Ripard]

When we can't allocate a new channel, we can simply return instead of
having to handle both cases, and that simplifies a bit the code.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/vc4/vc4_kms.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
index 3d0065df10f9..3034a5a6637e 100644
--- a/drivers/gpu/drm/vc4/vc4_kms.c
+++ b/drivers/gpu/drm/vc4/vc4_kms.c
@@ -794,6 +794,7 @@ static int vc4_pv_muxing_atomic_check(struct drm_device *dev,
 			to_vc4_crtc_state(new_crtc_state);
 		struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc);
 		unsigned int matching_channels;
+		unsigned int channel;
 
 		/* Nothing to do here, let's skip it */
 		if ((old_crtc_state->enable && new_crtc_state->enable) ||
@@ -837,14 +838,12 @@ static int vc4_pv_muxing_atomic_check(struct drm_device *dev,
 		 * but it works so far.
 		 */
 		matching_channels = hvs_state->unassigned_channels & vc4_crtc->data->hvs_available_channels;
-		if (matching_channels) {
-			unsigned int channel = ffs(matching_channels) - 1;
-
-			new_vc4_crtc_state->assigned_channel = channel;
-			hvs_state->unassigned_channels &= ~BIT(channel);
-		} else {
+		if (!matching_channels)
 			return -EINVAL;
-		}
+
+		channel = ffs(matching_channels) - 1;
+		new_vc4_crtc_state->assigned_channel = channel;
+		hvs_state->unassigned_channels &= ~BIT(channel);
 	}
 
 	return 0;
-- 
2.28.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/8] drm/vc4: Simplify a bit the global atomic_check

[Thomas Zimmermann]

[-- Attachment #1.1.1.1: Type: text/plain, Size: 1889 bytes --]



Am 13.11.20 um 16:29 schrieb Maxime Ripard:
> When we can't allocate a new channel, we can simply return instead of
> having to handle both cases, and that simplifies a bit the code.
> 
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>

> ---
>   drivers/gpu/drm/vc4/vc4_kms.c | 13 ++++++-------
>   1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
> index 3d0065df10f9..3034a5a6637e 100644
> --- a/drivers/gpu/drm/vc4/vc4_kms.c
> +++ b/drivers/gpu/drm/vc4/vc4_kms.c
> @@ -794,6 +794,7 @@ static int vc4_pv_muxing_atomic_check(struct drm_device *dev,
>   			to_vc4_crtc_state(new_crtc_state);
>   		struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc);
>   		unsigned int matching_channels;
> +		unsigned int channel;
>   
>   		/* Nothing to do here, let's skip it */
>   		if ((old_crtc_state->enable && new_crtc_state->enable) ||
> @@ -837,14 +838,12 @@ static int vc4_pv_muxing_atomic_check(struct drm_device *dev,
>   		 * but it works so far.
>   		 */
>   		matching_channels = hvs_state->unassigned_channels & vc4_crtc->data->hvs_available_channels;
> -		if (matching_channels) {
> -			unsigned int channel = ffs(matching_channels) - 1;
> -
> -			new_vc4_crtc_state->assigned_channel = channel;
> -			hvs_state->unassigned_channels &= ~BIT(channel);
> -		} else {
> +		if (!matching_channels)
>   			return -EINVAL;
> -		}
> +
> +		channel = ffs(matching_channels) - 1;
> +		new_vc4_crtc_state->assigned_channel = channel;
> +		hvs_state->unassigned_channels &= ~BIT(channel);
>   	}
>   
>   	return 0;
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer

[-- Attachment #1.1.1.2: OpenPGP_0x680DC11D530B7A23.asc --]
[-- Type: application/pgp-keys, Size: 7535 bytes --]

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 6/8] drm/vc4: kms: Wait on previous FIFO users before a commit

[Maxime Ripard]

If we're having two subsequent, non-blocking, commits on two different
CRTCs that share no resources, there's no guarantee on the order of
execution of both commits.

However, the second one will consider the first one as the old state,
and will be in charge of freeing it once that second commit is done.

If the first commit happens after that second commit, it might access
some resources related to its state that has been freed, resulting in a
use-after-free bug.

The standard DRM objects are protected against this, but our HVS private
state isn't so let's make sure we wait for all the previous FIFO users
to finish their commit before going with our own.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/vc4/vc4_kms.c | 118 +++++++++++++++++++++++++++++++++-
 1 file changed, 117 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
index 3034a5a6637e..849bc6b4cea4 100644
--- a/drivers/gpu/drm/vc4/vc4_kms.c
+++ b/drivers/gpu/drm/vc4/vc4_kms.c
@@ -40,6 +40,11 @@ static struct vc4_ctm_state *to_vc4_ctm_state(struct drm_private_state *priv)
 struct vc4_hvs_state {
 	struct drm_private_state base;
 	unsigned int unassigned_channels;
+
+	struct {
+		unsigned in_use: 1;
+		struct drm_crtc_commit *last_user;
+	} fifo_state[HVS_NUM_CHANNELS];
 };
 
 static struct vc4_hvs_state *
@@ -182,6 +187,32 @@ vc4_ctm_commit(struct vc4_dev *vc4, struct drm_atomic_state *state)
 		  VC4_SET_FIELD(ctm_state->fifo, SCALER_OLEDOFFS_DISPFIFO));
 }
 
+static struct vc4_hvs_state *
+vc4_hvs_get_new_global_state(struct drm_atomic_state *state)
+{
+	struct vc4_dev *vc4 = to_vc4_dev(state->dev);
+	struct drm_private_state *priv_state;
+
+	priv_state = drm_atomic_get_new_private_obj_state(state, &vc4->hvs_channels);
+	if (IS_ERR(priv_state))
+		return ERR_CAST(priv_state);
+
+	return to_vc4_hvs_state(priv_state);
+}
+
+static struct vc4_hvs_state *
+vc4_hvs_get_old_global_state(struct drm_atomic_state *state)
+{
+	struct vc4_dev *vc4 = to_vc4_dev(state->dev);
+	struct drm_private_state *priv_state;
+
+	priv_state = drm_atomic_get_old_private_obj_state(state, &vc4->hvs_channels);
+	if (IS_ERR(priv_state))
+		return ERR_CAST(priv_state);
+
+	return to_vc4_hvs_state(priv_state);
+}
+
 static struct vc4_hvs_state *
 vc4_hvs_get_global_state(struct drm_atomic_state *state)
 {
@@ -310,6 +341,7 @@ vc4_atomic_complete_commit(struct drm_atomic_state *state)
 	struct vc4_hvs *hvs = vc4->hvs;
 	struct drm_crtc_state *new_crtc_state;
 	struct drm_crtc *crtc;
+	struct vc4_hvs_state *old_hvs_state;
 	int i;
 
 	for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
@@ -329,6 +361,32 @@ vc4_atomic_complete_commit(struct drm_atomic_state *state)
 
 	drm_atomic_helper_wait_for_dependencies(state);
 
+	old_hvs_state = vc4_hvs_get_old_global_state(state);
+	if (!old_hvs_state)
+		return;
+
+	for_each_old_crtc_in_state(state, crtc, crtc_state, i) {
+		struct vc4_crtc_state *vc4_crtc_state =
+			to_vc4_crtc_state(crtc_state);
+		unsigned int channel =
+			vc4_crtc_state->assigned_channel;
+
+		if (channel == VC4_HVS_CHANNEL_DISABLED)
+			continue;
+
+		if (!old_hvs_state->fifo_state[channel].in_use)
+			continue;
+
+		commit = old_hvs_state->fifo_state[i].last_user;
+		ret = wait_for_completion_timeout(&commit->hw_done, 10 * HZ);
+		if (!ret)
+			DRM_DEV_ERROR(dev, "Timed out waiting for hw_done\n");
+
+		ret = wait_for_completion_timeout(&commit->flip_done, 10 * HZ);
+		if (!ret)
+			DRM_DEV_ERROR(dev, "Timed out waiting for flip_done\n");
+	}
+
 	drm_atomic_helper_commit_modeset_disables(dev, state);
 
 	vc4_ctm_commit(vc4, state);
@@ -368,6 +426,36 @@ static void commit_work(struct work_struct *work)
 	vc4_atomic_complete_commit(state);
 }
 
+static int vc4_atomic_commit_setup(struct drm_atomic_state *state)
+{
+	struct drm_crtc_state *crtc_state;
+	struct vc4_hvs_state *hvs_state;
+	struct drm_crtc *crtc;
+	unsigned int i;
+
+	hvs_state = vc4_hvs_get_new_global_state(state);
+	if (!hvs_state)
+		return -EINVAL;
+
+	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
+		struct vc4_crtc_state *vc4_crtc_state =
+			to_vc4_crtc_state(crtc_state);
+		unsigned int channel =
+			vc4_crtc_state->assigned_channel;
+
+		if (channel == VC4_HVS_CHANNEL_DISABLED)
+			continue;
+
+		if (!hvs_state->fifo_state[channel].in_use)
+			continue;
+
+		hvs_state->fifo_state[channel].last_user =
+			drm_crtc_commit_get(crtc_state->commit);
+	}
+
+	return 0;
+}
+
 /**
  * vc4_atomic_commit - commit validated state object
  * @dev: DRM device
@@ -697,6 +785,7 @@ vc4_hvs_channels_duplicate_state(struct drm_private_obj *obj)
 {
 	struct vc4_hvs_state *old_state = to_vc4_hvs_state(obj->state);
 	struct vc4_hvs_state *state;
+	unsigned int i;
 
 	state = kzalloc(sizeof(*state), GFP_KERNEL);
 	if (!state)
@@ -706,6 +795,16 @@ vc4_hvs_channels_duplicate_state(struct drm_private_obj *obj)
 
 	state->unassigned_channels = old_state->unassigned_channels;
 
+	for (i = 0; i < HVS_NUM_CHANNELS; i++) {
+		state->fifo_state[i].in_use = old_state->fifo_state[i].in_use;
+
+		if (!old_state->fifo_state[i].last_user)
+			continue;
+
+		state->fifo_state[i].last_user =
+			drm_crtc_commit_get(old_state->fifo_state[i].last_user);
+	}
+
 	return &state->base;
 }
 
@@ -713,6 +812,14 @@ static void vc4_hvs_channels_destroy_state(struct drm_private_obj *obj,
 					   struct drm_private_state *state)
 {
 	struct vc4_hvs_state *hvs_state = to_vc4_hvs_state(state);
+	unsigned int i;
+
+	for (i = 0; i < HVS_NUM_CHANNELS; i++) {
+		if (!hvs_state->fifo_state[i].last_user)
+			continue;
+
+		drm_crtc_commit_put(hvs_state->fifo_state[i].last_user);
+	}
 
 	kfree(hvs_state);
 }
@@ -808,7 +915,10 @@ static int vc4_pv_muxing_atomic_check(struct drm_device *dev,
 
 		/* If we're disabling our CRTC, we put back our channel */
 		if (old_crtc_state->enable && !new_crtc_state->enable) {
-			hvs_state->unassigned_channels |= BIT(old_vc4_crtc_state->assigned_channel);
+			channel = old_vc4_crtc_state->assigned_channel;
+
+			hvs_state->unassigned_channels |= BIT(channel);
+			hvs_state->fifo_state[channel].in_use = false;
 			new_vc4_crtc_state->assigned_channel = VC4_HVS_CHANNEL_DISABLED;
 			continue;
 		}
@@ -844,6 +954,7 @@ static int vc4_pv_muxing_atomic_check(struct drm_device *dev,
 		channel = ffs(matching_channels) - 1;
 		new_vc4_crtc_state->assigned_channel = channel;
 		hvs_state->unassigned_channels &= ~BIT(channel);
+		hvs_state->fifo_state[channel].in_use = true;
 	}
 
 	return 0;
@@ -869,6 +980,10 @@ vc4_atomic_check(struct drm_device *dev, struct drm_atomic_state *state)
 	return vc4_load_tracker_atomic_check(state);
 }
 
+static struct drm_mode_config_helper_funcs vc4_mode_config_helpers = {
+	.atomic_commit_setup	= vc4_atomic_commit_setup,
+};
+
 static const struct drm_mode_config_funcs vc4_mode_funcs = {
 	.atomic_check = vc4_atomic_check,
 	.atomic_commit = vc4_atomic_commit,
@@ -912,6 +1027,7 @@ int vc4_kms_load(struct drm_device *dev)
 	}
 
 	dev->mode_config.funcs = &vc4_mode_funcs;
+	dev->mode_config.helper_private = &vc4_mode_config_helpers;
 	dev->mode_config.preferred_depth = 24;
 	dev->mode_config.async_page_flip = true;
 	dev->mode_config.allow_fb_modifiers = true;
-- 
2.28.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 6/8] drm/vc4: kms: Wait on previous FIFO users before a commit

[Thomas Zimmermann]

[-- Attachment #1.1.1.1: Type: text/plain, Size: 9228 bytes --]

Hi

Am 13.11.20 um 16:29 schrieb Maxime Ripard:
> If we're having two subsequent, non-blocking, commits on two different
> CRTCs that share no resources, there's no guarantee on the order of
> execution of both commits.

Can there only ever be two commits that flip order?

> 
> However, the second one will consider the first one as the old state,
> and will be in charge of freeing it once that second commit is done.
> 
> If the first commit happens after that second commit, it might access
> some resources related to its state that has been freed, resulting in a
> use-after-free bug.
> 
> The standard DRM objects are protected against this, but our HVS private
> state isn't so let's make sure we wait for all the previous FIFO users
> to finish their commit before going with our own.

I'd appreciate a comment in the code that explains a bit how this works. 
It's sort of clear to me, but not enough to fully get it.

> 
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---
>   drivers/gpu/drm/vc4/vc4_kms.c | 118 +++++++++++++++++++++++++++++++++-
>   1 file changed, 117 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
> index 3034a5a6637e..849bc6b4cea4 100644
> --- a/drivers/gpu/drm/vc4/vc4_kms.c
> +++ b/drivers/gpu/drm/vc4/vc4_kms.c
> @@ -40,6 +40,11 @@ static struct vc4_ctm_state *to_vc4_ctm_state(struct drm_private_state *priv)
>   struct vc4_hvs_state {
>   	struct drm_private_state base;
>   	unsigned int unassigned_channels;
> +
> +	struct {
> +		unsigned in_use: 1;
> +		struct drm_crtc_commit *last_user;

Can these updates run concurrently? If so, the concurrency control via 
in_use is dubious.

I find last_user to be confusing. Maybe pending_commit makes sense?


> +	} fifo_state[HVS_NUM_CHANNELS];
>   };
>   
>   static struct vc4_hvs_state *
> @@ -182,6 +187,32 @@ vc4_ctm_commit(struct vc4_dev *vc4, struct drm_atomic_state *state)
>   		  VC4_SET_FIELD(ctm_state->fifo, SCALER_OLEDOFFS_DISPFIFO));
>   }
>   
> +static struct vc4_hvs_state *
> +vc4_hvs_get_new_global_state(struct drm_atomic_state *state)
> +{
> +	struct vc4_dev *vc4 = to_vc4_dev(state->dev);
> +	struct drm_private_state *priv_state;
> +
> +	priv_state = drm_atomic_get_new_private_obj_state(state, &vc4->hvs_channels);
> +	if (IS_ERR(priv_state))
> +		return ERR_CAST(priv_state);
> +
> +	return to_vc4_hvs_state(priv_state);
> +}
> +
> +static struct vc4_hvs_state *
> +vc4_hvs_get_old_global_state(struct drm_atomic_state *state)
> +{
> +	struct vc4_dev *vc4 = to_vc4_dev(state->dev);
> +	struct drm_private_state *priv_state;
> +
> +	priv_state = drm_atomic_get_old_private_obj_state(state, &vc4->hvs_channels);
> +	if (IS_ERR(priv_state))
> +		return ERR_CAST(priv_state);
> +
> +	return to_vc4_hvs_state(priv_state);
> +}
> +
>   static struct vc4_hvs_state *
>   vc4_hvs_get_global_state(struct drm_atomic_state *state)
>   {
> @@ -310,6 +341,7 @@ vc4_atomic_complete_commit(struct drm_atomic_state *state)
>   	struct vc4_hvs *hvs = vc4->hvs;
>   	struct drm_crtc_state *new_crtc_state;
>   	struct drm_crtc *crtc;
> +	struct vc4_hvs_state *old_hvs_state;
>   	int i;
>   
>   	for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
> @@ -329,6 +361,32 @@ vc4_atomic_complete_commit(struct drm_atomic_state *state)
>   
>   	drm_atomic_helper_wait_for_dependencies(state);
>   
> +	old_hvs_state = vc4_hvs_get_old_global_state(state);
> +	if (!old_hvs_state)
> +		return;
> +
> +	for_each_old_crtc_in_state(state, crtc, crtc_state, i) {
> +		struct vc4_crtc_state *vc4_crtc_state =
> +			to_vc4_crtc_state(crtc_state);
> +		unsigned int channel =
> +			vc4_crtc_state->assigned_channel;
> +
> +		if (channel == VC4_HVS_CHANNEL_DISABLED)
> +			continue;
> +
> +		if (!old_hvs_state->fifo_state[channel].in_use)
> +			continue;
> +
> +		commit = old_hvs_state->fifo_state[i].last_user;
> +		ret = wait_for_completion_timeout(&commit->hw_done, 10 * HZ);

For these returned values I'd use a separate variable, say 'complete' or 
'done'. It's an unsigned long and not the negative errno code that one 
would expect in ret.

> +		if (!ret)
> +			DRM_DEV_ERROR(dev, "Timed out waiting for hw_done\n");

 From the comments in drm_print.h, I think drm_err() is preferred over 
DRM_DEV_ERROR().

> +
> +		ret = wait_for_completion_timeout(&commit->flip_done, 10 * HZ);
> +		if (!ret)
> +			DRM_DEV_ERROR(dev, "Timed out waiting for flip_done\n");
> +	}
> +
>   	drm_atomic_helper_commit_modeset_disables(dev, state);
>   
>   	vc4_ctm_commit(vc4, state);
> @@ -368,6 +426,36 @@ static void commit_work(struct work_struct *work)
>   	vc4_atomic_complete_commit(state);
>   }
>   
> +static int vc4_atomic_commit_setup(struct drm_atomic_state *state)
> +{
> +	struct drm_crtc_state *crtc_state;
> +	struct vc4_hvs_state *hvs_state;
> +	struct drm_crtc *crtc;
> +	unsigned int i;
> +
> +	hvs_state = vc4_hvs_get_new_global_state(state);
> +	if (!hvs_state)
> +		return -EINVAL;
> +
> +	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
> +		struct vc4_crtc_state *vc4_crtc_state =
> +			to_vc4_crtc_state(crtc_state);
> +		unsigned int channel =
> +			vc4_crtc_state->assigned_channel;
> +
> +		if (channel == VC4_HVS_CHANNEL_DISABLED)
> +			continue;
> +
> +		if (!hvs_state->fifo_state[channel].in_use)
> +			continue;
> +
> +		hvs_state->fifo_state[channel].last_user =
> +			drm_crtc_commit_get(crtc_state->commit);
> +	}
> +
> +	return 0;
> +}
> +
>   /**
>    * vc4_atomic_commit - commit validated state object
>    * @dev: DRM device
> @@ -697,6 +785,7 @@ vc4_hvs_channels_duplicate_state(struct drm_private_obj *obj)
>   {
>   	struct vc4_hvs_state *old_state = to_vc4_hvs_state(obj->state);
>   	struct vc4_hvs_state *state;
> +	unsigned int i;
>   
>   	state = kzalloc(sizeof(*state), GFP_KERNEL);
>   	if (!state)
> @@ -706,6 +795,16 @@ vc4_hvs_channels_duplicate_state(struct drm_private_obj *obj)
>   
>   	state->unassigned_channels = old_state->unassigned_channels;
>   
> +	for (i = 0; i < HVS_NUM_CHANNELS; i++) {
> +		state->fifo_state[i].in_use = old_state->fifo_state[i].in_use;
> +
> +		if (!old_state->fifo_state[i].last_user)
> +			continue;
> +
> +		state->fifo_state[i].last_user =
> +			drm_crtc_commit_get(old_state->fifo_state[i].last_user);

A pure style issue: I'd avoid continue and instead write this as

if (old_state->fifo_state[i].last_user)
     state->fifo_state[i].last_user = drm_crtc_commit_get(...)

Here and in destroy_state.

> +	}
> +
>   	return &state->base;
>   }
>   
> @@ -713,6 +812,14 @@ static void vc4_hvs_channels_destroy_state(struct drm_private_obj *obj,
>   					   struct drm_private_state *state)
>   {
>   	struct vc4_hvs_state *hvs_state = to_vc4_hvs_state(state);
> +	unsigned int i;
> +
> +	for (i = 0; i < HVS_NUM_CHANNELS; i++) {
> +		if (!hvs_state->fifo_state[i].last_user)
> +			continue;
> +
> +		drm_crtc_commit_put(hvs_state->fifo_state[i].last_user);
> +	}
>   
>   	kfree(hvs_state);
>   }
> @@ -808,7 +915,10 @@ static int vc4_pv_muxing_atomic_check(struct drm_device *dev,
>   
>   		/* If we're disabling our CRTC, we put back our channel */
>   		if (old_crtc_state->enable && !new_crtc_state->enable) {
> -			hvs_state->unassigned_channels |= BIT(old_vc4_crtc_state->assigned_channel);
> +			channel = old_vc4_crtc_state->assigned_channel;
> +
> +			hvs_state->unassigned_channels |= BIT(channel);
> +			hvs_state->fifo_state[channel].in_use = false;

It looks like in_use correlates with the bit in unassigned_channels. 
Could you drop in_use entirely?

>   			new_vc4_crtc_state->assigned_channel = VC4_HVS_CHANNEL_DISABLED;
>   			continue;
>   		}
> @@ -844,6 +954,7 @@ static int vc4_pv_muxing_atomic_check(struct drm_device *dev,
>   		channel = ffs(matching_channels) - 1;
>   		new_vc4_crtc_state->assigned_channel = channel;
>   		hvs_state->unassigned_channels &= ~BIT(channel);
> +		hvs_state->fifo_state[channel].in_use = true;
>   	}
>   
>   	return 0;
> @@ -869,6 +980,10 @@ vc4_atomic_check(struct drm_device *dev, struct drm_atomic_state *state)
>   	return vc4_load_tracker_atomic_check(state);
>   }
>   
> +static struct drm_mode_config_helper_funcs vc4_mode_config_helpers = {
> +	.atomic_commit_setup	= vc4_atomic_commit_setup,

OK, I assume we'll go with this callback.

Best regards
Thomas

> +};
> +
>   static const struct drm_mode_config_funcs vc4_mode_funcs = {
>   	.atomic_check = vc4_atomic_check,
>   	.atomic_commit = vc4_atomic_commit,
> @@ -912,6 +1027,7 @@ int vc4_kms_load(struct drm_device *dev)
>   	}
>   
>   	dev->mode_config.funcs = &vc4_mode_funcs;
> +	dev->mode_config.helper_private = &vc4_mode_config_helpers;
>   	dev->mode_config.preferred_depth = 24;
>   	dev->mode_config.async_page_flip = true;
>   	dev->mode_config.allow_fb_modifiers = true;
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer

[-- Attachment #1.1.1.2: OpenPGP_0x680DC11D530B7A23.asc --]
[-- Type: application/pgp-keys, Size: 7535 bytes --]

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 6/8] drm/vc4: kms: Wait on previous FIFO users before a commit

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 2646 bytes --]

Hi Thomas,

On Fri, Nov 20, 2020 at 02:19:45PM +0100, Thomas Zimmermann wrote:
> Am 13.11.20 um 16:29 schrieb Maxime Ripard:
> > If we're having two subsequent, non-blocking, commits on two different
> > CRTCs that share no resources, there's no guarantee on the order of
> > execution of both commits.
> 
> Can there only ever be two commits that flip order?

It needs a bit of bad luck, but the patch 2 provides a bit more details.

Basically, if there's two subsequent non-blocking commits, affecting
different CRTCs without anything shared between those CRTCs (so no
plane, encoder or connector in common), you have no guarantee on the
commit order.

Most of the time it's not a big deal precisely because they don't share
anything. However if the private state is shared between the CRTCs then
it becomes an issue and we need to make sure that the ordering is
respected.

> > However, the second one will consider the first one as the old state,
> > and will be in charge of freeing it once that second commit is done.
> > 
> > If the first commit happens after that second commit, it might access
> > some resources related to its state that has been freed, resulting in a
> > use-after-free bug.
> > 
> > The standard DRM objects are protected against this, but our HVS private
> > state isn't so let's make sure we wait for all the previous FIFO users
> > to finish their commit before going with our own.
> 
> I'd appreciate a comment in the code that explains a bit how this works.
> It's sort of clear to me, but not enough to fully get it.

I'm not sure to get what "this" means and what do you want me to comment
there?

> > 
> > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> > ---
> >   drivers/gpu/drm/vc4/vc4_kms.c | 118 +++++++++++++++++++++++++++++++++-
> >   1 file changed, 117 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
> > index 3034a5a6637e..849bc6b4cea4 100644
> > --- a/drivers/gpu/drm/vc4/vc4_kms.c
> > +++ b/drivers/gpu/drm/vc4/vc4_kms.c
> > @@ -40,6 +40,11 @@ static struct vc4_ctm_state *to_vc4_ctm_state(struct drm_private_state *priv)
> >   struct vc4_hvs_state {
> >   	struct drm_private_state base;
> >   	unsigned int unassigned_channels;
> > +
> > +	struct {
> > +		unsigned in_use: 1;
> > +		struct drm_crtc_commit *last_user;
> 
> Can these updates run concurrently? If so, the concurrency control via
> in_use is dubious.

No, there's only ever one commit being done. We just need to make sure
they are in the right order.

I'll address your other comments

Maxime

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 7/8] drm/vc4: kms: Remove async modeset semaphore

[Maxime Ripard]

Now that we have proper ordering guaranteed by the previous patch, the
semaphore is redundant and can be removed.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/vc4/vc4_crtc.c | 13 -------------
 drivers/gpu/drm/vc4/vc4_drv.h  |  2 --
 drivers/gpu/drm/vc4/vc4_kms.c  | 20 +-------------------
 3 files changed, 1 insertion(+), 34 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
index 29b77f4b4e56..65d43e2e1d51 100644
--- a/drivers/gpu/drm/vc4/vc4_crtc.c
+++ b/drivers/gpu/drm/vc4/vc4_crtc.c
@@ -699,7 +699,6 @@ vc4_async_page_flip_complete(struct vc4_seqno_cb *cb)
 		container_of(cb, struct vc4_async_flip_state, cb);
 	struct drm_crtc *crtc = flip_state->crtc;
 	struct drm_device *dev = crtc->dev;
-	struct vc4_dev *vc4 = to_vc4_dev(dev);
 	struct drm_plane *plane = crtc->primary;
 
 	vc4_plane_async_set_fb(plane, flip_state->fb);
@@ -731,8 +730,6 @@ vc4_async_page_flip_complete(struct vc4_seqno_cb *cb)
 	}
 
 	kfree(flip_state);
-
-	up(&vc4->async_modeset);
 }
 
 /* Implements async (non-vblank-synced) page flips.
@@ -747,7 +744,6 @@ static int vc4_async_page_flip(struct drm_crtc *crtc,
 			       uint32_t flags)
 {
 	struct drm_device *dev = crtc->dev;
-	struct vc4_dev *vc4 = to_vc4_dev(dev);
 	struct drm_plane *plane = crtc->primary;
 	int ret = 0;
 	struct vc4_async_flip_state *flip_state;
@@ -776,15 +772,6 @@ static int vc4_async_page_flip(struct drm_crtc *crtc,
 	flip_state->crtc = crtc;
 	flip_state->event = event;
 
-	/* Make sure all other async modesetes have landed. */
-	ret = down_interruptible(&vc4->async_modeset);
-	if (ret) {
-		drm_framebuffer_put(fb);
-		vc4_bo_dec_usecnt(bo);
-		kfree(flip_state);
-		return ret;
-	}
-
 	/* Save the current FB before it's replaced by the new one in
 	 * drm_atomic_set_fb_for_plane(). We'll need the old FB in
 	 * vc4_async_page_flip_complete() to decrement the BO usecnt and keep
diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
index 9eefd76cb09e..60062afba7b6 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.h
+++ b/drivers/gpu/drm/vc4/vc4_drv.h
@@ -215,8 +215,6 @@ struct vc4_dev {
 		struct work_struct reset_work;
 	} hangcheck;
 
-	struct semaphore async_modeset;
-
 	struct drm_modeset_lock ctm_state_lock;
 	struct drm_private_obj ctm_manager;
 	struct drm_private_obj hvs_channels;
diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
index 849bc6b4cea4..79ab7b8a5e0e 100644
--- a/drivers/gpu/drm/vc4/vc4_kms.c
+++ b/drivers/gpu/drm/vc4/vc4_kms.c
@@ -414,8 +414,6 @@ vc4_atomic_complete_commit(struct drm_atomic_state *state)
 		clk_set_min_rate(hvs->core_clk, 0);
 
 	drm_atomic_state_put(state);
-
-	up(&vc4->async_modeset);
 }
 
 static void commit_work(struct work_struct *work)
@@ -473,14 +471,9 @@ static int vc4_atomic_commit(struct drm_device *dev,
 			     struct drm_atomic_state *state,
 			     bool nonblock)
 {
-	struct vc4_dev *vc4 = to_vc4_dev(dev);
 	int ret;
 
 	if (state->async_update) {
-		ret = down_interruptible(&vc4->async_modeset);
-		if (ret)
-			return ret;
-
 		ret = drm_atomic_helper_prepare_planes(dev, state);
 		if (ret) {
 			up(&vc4->async_modeset);
@@ -491,8 +484,6 @@ static int vc4_atomic_commit(struct drm_device *dev,
 
 		drm_atomic_helper_cleanup_planes(dev, state);
 
-		up(&vc4->async_modeset);
-
 		return 0;
 	}
 
@@ -508,21 +499,14 @@ static int vc4_atomic_commit(struct drm_device *dev,
 
 	INIT_WORK(&state->commit_work, commit_work);
 
-	ret = down_interruptible(&vc4->async_modeset);
-	if (ret)
-		return ret;
-
 	ret = drm_atomic_helper_prepare_planes(dev, state);
-	if (ret) {
-		up(&vc4->async_modeset);
+	if (ret)
 		return ret;
-	}
 
 	if (!nonblock) {
 		ret = drm_atomic_helper_wait_for_fences(dev, state, true);
 		if (ret) {
 			drm_atomic_helper_cleanup_planes(dev, state);
-			up(&vc4->async_modeset);
 			return ret;
 		}
 	}
@@ -1006,8 +990,6 @@ int vc4_kms_load(struct drm_device *dev)
 		vc4->load_tracker_enabled = true;
 	}
 
-	sema_init(&vc4->async_modeset, 1);
-
 	/* Set support for vblank irq fast disable, before drm_vblank_init() */
 	dev->vblank_disable_immediate = true;
 
-- 
2.28.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 7/8] drm/vc4: kms: Remove async modeset semaphore

[Thomas Zimmermann]

[-- Attachment #1.1.1.1: Type: text/plain, Size: 4933 bytes --]



Am 13.11.20 um 16:29 schrieb Maxime Ripard:
> Now that we have proper ordering guaranteed by the previous patch, the
> semaphore is redundant and can be removed.
> 
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>

Acked-by: Thomas Zimmermann <tzimmermann@suse.de>

> ---
>   drivers/gpu/drm/vc4/vc4_crtc.c | 13 -------------
>   drivers/gpu/drm/vc4/vc4_drv.h  |  2 --
>   drivers/gpu/drm/vc4/vc4_kms.c  | 20 +-------------------
>   3 files changed, 1 insertion(+), 34 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
> index 29b77f4b4e56..65d43e2e1d51 100644
> --- a/drivers/gpu/drm/vc4/vc4_crtc.c
> +++ b/drivers/gpu/drm/vc4/vc4_crtc.c
> @@ -699,7 +699,6 @@ vc4_async_page_flip_complete(struct vc4_seqno_cb *cb)
>   		container_of(cb, struct vc4_async_flip_state, cb);
>   	struct drm_crtc *crtc = flip_state->crtc;
>   	struct drm_device *dev = crtc->dev;
> -	struct vc4_dev *vc4 = to_vc4_dev(dev);
>   	struct drm_plane *plane = crtc->primary;
>   
>   	vc4_plane_async_set_fb(plane, flip_state->fb);
> @@ -731,8 +730,6 @@ vc4_async_page_flip_complete(struct vc4_seqno_cb *cb)
>   	}
>   
>   	kfree(flip_state);
> -
> -	up(&vc4->async_modeset);
>   }
>   
>   /* Implements async (non-vblank-synced) page flips.
> @@ -747,7 +744,6 @@ static int vc4_async_page_flip(struct drm_crtc *crtc,
>   			       uint32_t flags)
>   {
>   	struct drm_device *dev = crtc->dev;
> -	struct vc4_dev *vc4 = to_vc4_dev(dev);
>   	struct drm_plane *plane = crtc->primary;
>   	int ret = 0;
>   	struct vc4_async_flip_state *flip_state;
> @@ -776,15 +772,6 @@ static int vc4_async_page_flip(struct drm_crtc *crtc,
>   	flip_state->crtc = crtc;
>   	flip_state->event = event;
>   
> -	/* Make sure all other async modesetes have landed. */
> -	ret = down_interruptible(&vc4->async_modeset);
> -	if (ret) {
> -		drm_framebuffer_put(fb);
> -		vc4_bo_dec_usecnt(bo);
> -		kfree(flip_state);
> -		return ret;
> -	}
> -
>   	/* Save the current FB before it's replaced by the new one in
>   	 * drm_atomic_set_fb_for_plane(). We'll need the old FB in
>   	 * vc4_async_page_flip_complete() to decrement the BO usecnt and keep
> diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
> index 9eefd76cb09e..60062afba7b6 100644
> --- a/drivers/gpu/drm/vc4/vc4_drv.h
> +++ b/drivers/gpu/drm/vc4/vc4_drv.h
> @@ -215,8 +215,6 @@ struct vc4_dev {
>   		struct work_struct reset_work;
>   	} hangcheck;
>   
> -	struct semaphore async_modeset;
> -
>   	struct drm_modeset_lock ctm_state_lock;
>   	struct drm_private_obj ctm_manager;
>   	struct drm_private_obj hvs_channels;
> diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
> index 849bc6b4cea4..79ab7b8a5e0e 100644
> --- a/drivers/gpu/drm/vc4/vc4_kms.c
> +++ b/drivers/gpu/drm/vc4/vc4_kms.c
> @@ -414,8 +414,6 @@ vc4_atomic_complete_commit(struct drm_atomic_state *state)
>   		clk_set_min_rate(hvs->core_clk, 0);
>   
>   	drm_atomic_state_put(state);
> -
> -	up(&vc4->async_modeset);
>   }
>   
>   static void commit_work(struct work_struct *work)
> @@ -473,14 +471,9 @@ static int vc4_atomic_commit(struct drm_device *dev,
>   			     struct drm_atomic_state *state,
>   			     bool nonblock)
>   {
> -	struct vc4_dev *vc4 = to_vc4_dev(dev);
>   	int ret;
>   
>   	if (state->async_update) {
> -		ret = down_interruptible(&vc4->async_modeset);
> -		if (ret)
> -			return ret;
> -
>   		ret = drm_atomic_helper_prepare_planes(dev, state);
>   		if (ret) {
>   			up(&vc4->async_modeset);
> @@ -491,8 +484,6 @@ static int vc4_atomic_commit(struct drm_device *dev,
>   
>   		drm_atomic_helper_cleanup_planes(dev, state);
>   
> -		up(&vc4->async_modeset);
> -
>   		return 0;
>   	}
>   
> @@ -508,21 +499,14 @@ static int vc4_atomic_commit(struct drm_device *dev,
>   
>   	INIT_WORK(&state->commit_work, commit_work);
>   
> -	ret = down_interruptible(&vc4->async_modeset);
> -	if (ret)
> -		return ret;
> -
>   	ret = drm_atomic_helper_prepare_planes(dev, state);
> -	if (ret) {
> -		up(&vc4->async_modeset);
> +	if (ret)
>   		return ret;
> -	}
>   
>   	if (!nonblock) {
>   		ret = drm_atomic_helper_wait_for_fences(dev, state, true);
>   		if (ret) {
>   			drm_atomic_helper_cleanup_planes(dev, state);
> -			up(&vc4->async_modeset);
>   			return ret;
>   		}
>   	}
> @@ -1006,8 +990,6 @@ int vc4_kms_load(struct drm_device *dev)
>   		vc4->load_tracker_enabled = true;
>   	}
>   
> -	sema_init(&vc4->async_modeset, 1);
> -
>   	/* Set support for vblank irq fast disable, before drm_vblank_init() */
>   	dev->vblank_disable_immediate = true;
>   
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer

[-- Attachment #1.1.1.2: OpenPGP_0x680DC11D530B7A23.asc --]
[-- Type: application/pgp-keys, Size: 7535 bytes --]

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 8/8] drm/vc4: kms: Convert to atomic helpers

[Maxime Ripard]

Now that the semaphore is gone, our atomic_commit implementation is
basically drm_atomic_helper_commit with a somewhat custom commit_tail,
the main difference being that we're using wait_for_flip_done instead of
wait_for_vblanks used in the drm_atomic_helper_commit_tail helper.

Let's switch to using drm_atomic_helper_commit.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/vc4/vc4_kms.c | 112 +---------------------------------
 1 file changed, 3 insertions(+), 109 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
index 79ab7b8a5e0e..ede5d2b6ac65 100644
--- a/drivers/gpu/drm/vc4/vc4_kms.c
+++ b/drivers/gpu/drm/vc4/vc4_kms.c
@@ -333,8 +333,7 @@ static void vc5_hvs_pv_muxing_commit(struct vc4_dev *vc4,
 	}
 }
 
-static void
-vc4_atomic_complete_commit(struct drm_atomic_state *state)
+static void vc4_atomic_commit_tail(struct drm_atomic_state *state)
 {
 	struct drm_device *dev = state->dev;
 	struct vc4_dev *vc4 = to_vc4_dev(dev);
@@ -357,10 +356,6 @@ vc4_atomic_complete_commit(struct drm_atomic_state *state)
 	if (vc4->hvs->hvs5)
 		clk_set_min_rate(hvs->core_clk, 500000000);
 
-	drm_atomic_helper_wait_for_fences(dev, state, false);
-
-	drm_atomic_helper_wait_for_dependencies(state);
-
 	old_hvs_state = vc4_hvs_get_old_global_state(state);
 	if (!old_hvs_state)
 		return;
@@ -408,20 +403,8 @@ vc4_atomic_complete_commit(struct drm_atomic_state *state)
 
 	drm_atomic_helper_cleanup_planes(dev, state);
 
-	drm_atomic_helper_commit_cleanup_done(state);
-
 	if (vc4->hvs->hvs5)
 		clk_set_min_rate(hvs->core_clk, 0);
-
-	drm_atomic_state_put(state);
-}
-
-static void commit_work(struct work_struct *work)
-{
-	struct drm_atomic_state *state = container_of(work,
-						      struct drm_atomic_state,
-						      commit_work);
-	vc4_atomic_complete_commit(state);
 }
 
 static int vc4_atomic_commit_setup(struct drm_atomic_state *state)
@@ -454,96 +437,6 @@ static int vc4_atomic_commit_setup(struct drm_atomic_state *state)
 	return 0;
 }
 
-/**
- * vc4_atomic_commit - commit validated state object
- * @dev: DRM device
- * @state: the driver state object
- * @nonblock: nonblocking commit
- *
- * This function commits a with drm_atomic_helper_check() pre-validated state
- * object. This can still fail when e.g. the framebuffer reservation fails. For
- * now this doesn't implement asynchronous commits.
- *
- * RETURNS
- * Zero for success or -errno.
- */
-static int vc4_atomic_commit(struct drm_device *dev,
-			     struct drm_atomic_state *state,
-			     bool nonblock)
-{
-	int ret;
-
-	if (state->async_update) {
-		ret = drm_atomic_helper_prepare_planes(dev, state);
-		if (ret) {
-			up(&vc4->async_modeset);
-			return ret;
-		}
-
-		drm_atomic_helper_async_commit(dev, state);
-
-		drm_atomic_helper_cleanup_planes(dev, state);
-
-		return 0;
-	}
-
-	/* We know for sure we don't want an async update here. Set
-	 * state->legacy_cursor_update to false to prevent
-	 * drm_atomic_helper_setup_commit() from auto-completing
-	 * commit->flip_done.
-	 */
-	state->legacy_cursor_update = false;
-	ret = drm_atomic_helper_setup_commit(state, nonblock);
-	if (ret)
-		return ret;
-
-	INIT_WORK(&state->commit_work, commit_work);
-
-	ret = drm_atomic_helper_prepare_planes(dev, state);
-	if (ret)
-		return ret;
-
-	if (!nonblock) {
-		ret = drm_atomic_helper_wait_for_fences(dev, state, true);
-		if (ret) {
-			drm_atomic_helper_cleanup_planes(dev, state);
-			return ret;
-		}
-	}
-
-	/*
-	 * This is the point of no return - everything below never fails except
-	 * when the hw goes bonghits. Which means we can commit the new state on
-	 * the software side now.
-	 */
-
-	BUG_ON(drm_atomic_helper_swap_state(state, false) < 0);
-
-	/*
-	 * Everything below can be run asynchronously without the need to grab
-	 * any modeset locks at all under one condition: It must be guaranteed
-	 * that the asynchronous work has either been cancelled (if the driver
-	 * supports it, which at least requires that the framebuffers get
-	 * cleaned up with drm_atomic_helper_cleanup_planes()) or completed
-	 * before the new state gets committed on the software side with
-	 * drm_atomic_helper_swap_state().
-	 *
-	 * This scheme allows new atomic state updates to be prepared and
-	 * checked in parallel to the asynchronous completion of the previous
-	 * update. Which is important since compositors need to figure out the
-	 * composition of the next frame right after having submitted the
-	 * current layout.
-	 */
-
-	drm_atomic_state_get(state);
-	if (nonblock)
-		queue_work(system_unbound_wq, &state->commit_work);
-	else
-		vc4_atomic_complete_commit(state);
-
-	return 0;
-}
-
 static struct drm_framebuffer *vc4_fb_create(struct drm_device *dev,
 					     struct drm_file *file_priv,
 					     const struct drm_mode_fb_cmd2 *mode_cmd)
@@ -966,11 +859,12 @@ vc4_atomic_check(struct drm_device *dev, struct drm_atomic_state *state)
 
 static struct drm_mode_config_helper_funcs vc4_mode_config_helpers = {
 	.atomic_commit_setup	= vc4_atomic_commit_setup,
+	.atomic_commit_tail	= vc4_atomic_commit_tail,
 };
 
 static const struct drm_mode_config_funcs vc4_mode_funcs = {
 	.atomic_check = vc4_atomic_check,
-	.atomic_commit = vc4_atomic_commit,
+	.atomic_commit = drm_atomic_helper_commit,
 	.fb_create = vc4_fb_create,
 };
 
-- 
2.28.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 8/8] drm/vc4: kms: Convert to atomic helpers

[Thomas Zimmermann]

[-- Attachment #1.1.1.1: Type: text/plain, Size: 6176 bytes --]



Am 13.11.20 um 16:29 schrieb Maxime Ripard:
> Now that the semaphore is gone, our atomic_commit implementation is
> basically drm_atomic_helper_commit with a somewhat custom commit_tail,
> the main difference being that we're using wait_for_flip_done instead of
> wait_for_vblanks used in the drm_atomic_helper_commit_tail helper.
> 
> Let's switch to using drm_atomic_helper_commit.
> 
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>

Acked-by: Thomas Zimmermann <tzimmermann@suse.de>

> ---
>   drivers/gpu/drm/vc4/vc4_kms.c | 112 +---------------------------------
>   1 file changed, 3 insertions(+), 109 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
> index 79ab7b8a5e0e..ede5d2b6ac65 100644
> --- a/drivers/gpu/drm/vc4/vc4_kms.c
> +++ b/drivers/gpu/drm/vc4/vc4_kms.c
> @@ -333,8 +333,7 @@ static void vc5_hvs_pv_muxing_commit(struct vc4_dev *vc4,
>   	}
>   }
>   
> -static void
> -vc4_atomic_complete_commit(struct drm_atomic_state *state)
> +static void vc4_atomic_commit_tail(struct drm_atomic_state *state)
>   {
>   	struct drm_device *dev = state->dev;
>   	struct vc4_dev *vc4 = to_vc4_dev(dev);
> @@ -357,10 +356,6 @@ vc4_atomic_complete_commit(struct drm_atomic_state *state)
>   	if (vc4->hvs->hvs5)
>   		clk_set_min_rate(hvs->core_clk, 500000000);
>   
> -	drm_atomic_helper_wait_for_fences(dev, state, false);
> -
> -	drm_atomic_helper_wait_for_dependencies(state);
> -
>   	old_hvs_state = vc4_hvs_get_old_global_state(state);
>   	if (!old_hvs_state)
>   		return;
> @@ -408,20 +403,8 @@ vc4_atomic_complete_commit(struct drm_atomic_state *state)
>   
>   	drm_atomic_helper_cleanup_planes(dev, state);
>   
> -	drm_atomic_helper_commit_cleanup_done(state);
> -
>   	if (vc4->hvs->hvs5)
>   		clk_set_min_rate(hvs->core_clk, 0);
> -
> -	drm_atomic_state_put(state);
> -}
> -
> -static void commit_work(struct work_struct *work)
> -{
> -	struct drm_atomic_state *state = container_of(work,
> -						      struct drm_atomic_state,
> -						      commit_work);
> -	vc4_atomic_complete_commit(state);
>   }
>   
>   static int vc4_atomic_commit_setup(struct drm_atomic_state *state)
> @@ -454,96 +437,6 @@ static int vc4_atomic_commit_setup(struct drm_atomic_state *state)
>   	return 0;
>   }
>   
> -/**
> - * vc4_atomic_commit - commit validated state object
> - * @dev: DRM device
> - * @state: the driver state object
> - * @nonblock: nonblocking commit
> - *
> - * This function commits a with drm_atomic_helper_check() pre-validated state
> - * object. This can still fail when e.g. the framebuffer reservation fails. For
> - * now this doesn't implement asynchronous commits.
> - *
> - * RETURNS
> - * Zero for success or -errno.
> - */
> -static int vc4_atomic_commit(struct drm_device *dev,
> -			     struct drm_atomic_state *state,
> -			     bool nonblock)
> -{
> -	int ret;
> -
> -	if (state->async_update) {
> -		ret = drm_atomic_helper_prepare_planes(dev, state);
> -		if (ret) {
> -			up(&vc4->async_modeset);
> -			return ret;
> -		}
> -
> -		drm_atomic_helper_async_commit(dev, state);
> -
> -		drm_atomic_helper_cleanup_planes(dev, state);
> -
> -		return 0;
> -	}
> -
> -	/* We know for sure we don't want an async update here. Set
> -	 * state->legacy_cursor_update to false to prevent
> -	 * drm_atomic_helper_setup_commit() from auto-completing
> -	 * commit->flip_done.
> -	 */
> -	state->legacy_cursor_update = false;
> -	ret = drm_atomic_helper_setup_commit(state, nonblock);
> -	if (ret)
> -		return ret;
> -
> -	INIT_WORK(&state->commit_work, commit_work);
> -
> -	ret = drm_atomic_helper_prepare_planes(dev, state);
> -	if (ret)
> -		return ret;
> -
> -	if (!nonblock) {
> -		ret = drm_atomic_helper_wait_for_fences(dev, state, true);
> -		if (ret) {
> -			drm_atomic_helper_cleanup_planes(dev, state);
> -			return ret;
> -		}
> -	}
> -
> -	/*
> -	 * This is the point of no return - everything below never fails except
> -	 * when the hw goes bonghits. Which means we can commit the new state on
> -	 * the software side now.
> -	 */
> -
> -	BUG_ON(drm_atomic_helper_swap_state(state, false) < 0);
> -
> -	/*
> -	 * Everything below can be run asynchronously without the need to grab
> -	 * any modeset locks at all under one condition: It must be guaranteed
> -	 * that the asynchronous work has either been cancelled (if the driver
> -	 * supports it, which at least requires that the framebuffers get
> -	 * cleaned up with drm_atomic_helper_cleanup_planes()) or completed
> -	 * before the new state gets committed on the software side with
> -	 * drm_atomic_helper_swap_state().
> -	 *
> -	 * This scheme allows new atomic state updates to be prepared and
> -	 * checked in parallel to the asynchronous completion of the previous
> -	 * update. Which is important since compositors need to figure out the
> -	 * composition of the next frame right after having submitted the
> -	 * current layout.
> -	 */
> -
> -	drm_atomic_state_get(state);
> -	if (nonblock)
> -		queue_work(system_unbound_wq, &state->commit_work);
> -	else
> -		vc4_atomic_complete_commit(state);
> -
> -	return 0;
> -}
> -
>   static struct drm_framebuffer *vc4_fb_create(struct drm_device *dev,
>   					     struct drm_file *file_priv,
>   					     const struct drm_mode_fb_cmd2 *mode_cmd)
> @@ -966,11 +859,12 @@ vc4_atomic_check(struct drm_device *dev, struct drm_atomic_state *state)
>   
>   static struct drm_mode_config_helper_funcs vc4_mode_config_helpers = {
>   	.atomic_commit_setup	= vc4_atomic_commit_setup,
> +	.atomic_commit_tail	= vc4_atomic_commit_tail,
>   };
>   
>   static const struct drm_mode_config_funcs vc4_mode_funcs = {
>   	.atomic_check = vc4_atomic_check,
> -	.atomic_commit = vc4_atomic_commit,
> +	.atomic_commit = drm_atomic_helper_commit,
>   	.fb_create = vc4_fb_create,
>   };
>   
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer

[-- Attachment #1.1.1.2: OpenPGP_0x680DC11D530B7A23.asc --]
[-- Type: application/pgp-keys, Size: 7535 bytes --]

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel