diff mbox series

[v3,17/20] drm/drv: Switch skeleton to drm_mode_config_create_initial_state()

Message ID 20260424-drm-mode-config-init-v3-17-8b68d9db0d8b@kernel.org (mailing list archive)
State New
Headers
Series drm/atomic: Rework initial state allocation |

Commit Message

Maxime Ripard April 24, 2026, 10:18 a.m. UTC 
The driver skeleton currently recommends calling
drm_mode_config_reset() at probe time to create the initial state.

Now that drm_mode_config_create_initial_state() exists to handle
initial state allocation without hardware side effects, update the
skeleton to recommend it instead.

Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 drivers/gpu/drm/drm_drv.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Laurent Pinchart May 4, 2026, 6:02 p.m. UTC | #1 
On Fri, Apr 24, 2026 at 12:18:57PM +0200, Maxime Ripard wrote:
> The driver skeleton currently recommends calling
> drm_mode_config_reset() at probe time to create the initial state.
> 
> Now that drm_mode_config_create_initial_state() exists to handle
> initial state allocation without hardware side effects, update the
> skeleton to recommend it instead.
> 
> Signed-off-by: Maxime Ripard <mripard@kernel.org>
> ---
>  drivers/gpu/drm/drm_drv.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 985c283cf59f..f537556b06a8 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -340,11 +340,13 @@ void drm_minor_release(struct drm_minor *minor)
>   *
>   *		// Further setup, display pipeline etc
>   *
>   *		platform_set_drvdata(pdev, drm);
>   *
> - *		drm_mode_config_reset(drm);
> + *		ret = drm_mode_config_create_initial_state(drm);
> + *		if (ret)
> + *			return ret;

There's one point I'm still not sure to understand properly. The
skeleton example (and the tidss driver, which you convert to the new API
in this series) both call drm_mode_config_helper_resume(). This in turn
calls drm_atomic_helper_resume(), and drm_mode_config_reset(). For
drivers that implement .atomic_create_state() instead of .reset() (such
as tidss, after its conversion in this series), drm_mode_config_reset()
will call the drm_mode_config_*_create_state() helpers, which allocate
and initialize a new state (through .atomic_create_state()), and store
that new state in the object's ->state field. Won't this leak the state
previously stored there ?

>   *
>   *		ret = drm_dev_register(drm);
>   *		if (ret)
>   *			return ret;
>   *
Maxime Ripard May 12, 2026, 11:20 a.m. UTC | #2 
On Mon, May 04, 2026 at 09:02:16PM +0300, Laurent Pinchart wrote:
> On Fri, Apr 24, 2026 at 12:18:57PM +0200, Maxime Ripard wrote:
> > The driver skeleton currently recommends calling
> > drm_mode_config_reset() at probe time to create the initial state.
> > 
> > Now that drm_mode_config_create_initial_state() exists to handle
> > initial state allocation without hardware side effects, update the
> > skeleton to recommend it instead.
> > 
> > Signed-off-by: Maxime Ripard <mripard@kernel.org>
> > ---
> >  drivers/gpu/drm/drm_drv.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> > index 985c283cf59f..f537556b06a8 100644
> > --- a/drivers/gpu/drm/drm_drv.c
> > +++ b/drivers/gpu/drm/drm_drv.c
> > @@ -340,11 +340,13 @@ void drm_minor_release(struct drm_minor *minor)
> >   *
> >   *		// Further setup, display pipeline etc
> >   *
> >   *		platform_set_drvdata(pdev, drm);
> >   *
> > - *		drm_mode_config_reset(drm);
> > + *		ret = drm_mode_config_create_initial_state(drm);
> > + *		if (ret)
> > + *			return ret;
> 
> There's one point I'm still not sure to understand properly. The
> skeleton example (and the tidss driver, which you convert to the new API
> in this series) both call drm_mode_config_helper_resume(). This in turn
> calls drm_atomic_helper_resume(), and drm_mode_config_reset(). For
> drivers that implement .atomic_create_state() instead of .reset() (such
> as tidss, after its conversion in this series), drm_mode_config_reset()
> will call the drm_mode_config_*_create_state() helpers, which allocate
> and initialize a new state (through .atomic_create_state()), and store
> that new state in the object's ->state field. Won't this leak the state
> previously stored there ?

Thanks for spotting this, you're totally right!

I'll fix it in the next version
Maxime
diff mbox series

Patch

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 985c283cf59f..f537556b06a8 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -340,11 +340,13 @@  void drm_minor_release(struct drm_minor *minor)
  *
  *		// Further setup, display pipeline etc
  *
  *		platform_set_drvdata(pdev, drm);
  *
- *		drm_mode_config_reset(drm);
+ *		ret = drm_mode_config_create_initial_state(drm);
+ *		if (ret)
+ *			return ret;
  *
  *		ret = drm_dev_register(drm);
  *		if (ret)
  *			return ret;
  *