Re: [PATCH] drm: don't call ->firstopen for KMS drivers

From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: DRI Development <dri-devel@lists.freedesktop.org>
Subject: Re: [PATCH] drm: don't call ->firstopen for KMS drivers
Date: Mon, 22 Jul 2013 11:02:48 +0200	[thread overview]
Message-ID: <1733671.WOoQMfelD9@avalon> (raw)
In-Reply-To: <1373726710-28891-1-git-send-email-daniel.vetter@ffwll.ch>

Hi Daniel,

Thank you for the patch.

On Saturday 13 July 2013 16:45:10 Daniel Vetter wrote:
> It has way too much potential for driver writers to do stupid things
> like delayed hw setup because the load sequence is somehow racy (e.g.
> the imx driver in staging). So don't call it for modesetting drivers,
> which reduces the complexity of the drm core -> driver interface a
> notch.
> 
> v2: Don't forget to update DocBook.
> 
> v3: Go with Laurent's slightly more elaborate proposal for the DocBook
> update. Add a few words on top of his diff to elaborate a bit on what
> KMS drivers should and shouldn't do in lastclose. There was already a
> paragraph present talking about restoring properties, I've simply
> extended that one.
> 
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  Documentation/DocBook/drm.tmpl | 27 ++++++++++++++++-----------
>  drivers/gpu/drm/drm_fops.c     |  3 ++-
>  2 files changed, 18 insertions(+), 12 deletions(-)
> 
> diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
> index 4d54ac8..52d5eda 100644
> --- a/Documentation/DocBook/drm.tmpl
> +++ b/Documentation/DocBook/drm.tmpl
> @@ -2422,18 +2422,18 @@ void (*postclose) (struct drm_device *, struct
> drm_file *);</synopsis> </abstract>
>        <para>
>          The <methodname>firstopen</methodname> method is called by the DRM
> core -	when an application opens a device that has no other opened file
> handle. -	Similarly the <methodname>lastclose</methodname> method is 
called
> when -	the last application holding a file handle opened on the device
> closes -	it. Both methods are mostly used for UMS (User Mode Setting)
> drivers to -	acquire and release device resources which should be done in
> the -	<methodname>load</methodname> and <methodname>unload</methodname>
> -	methods for KMS drivers.
> +	for legacy UMS (User Mode Setting) drivers only when an application
> +	opens a device that has no other opened file handle. UMS drivers can
> +	implement it to acquire device resources. KMS drivers can't use the
> +	method and must acquire resources in the <methodname>load</methodname>
> +	method instead.
>        </para>
>        <para>
> -        Note that the <methodname>lastclose</methodname> method is also
> called -	at module unload time or, for hot-pluggable devices, when the
> device is -	unplugged. The <methodname>firstopen</methodname> and
> +	Similarly the <methodname>lastclose</methodname> method is called when
> +	the last application holding a file handle opened on the device closes
> +	it, for both UMS and KMS drivers. Additionally, the method is also
> +	called at module unload time or, for hot-pluggable devices, when the
> +	device is unplugged. The <methodname>firstopen</methodname> and
>  	<methodname>lastclose</methodname> calls can thus be unbalanced.
>        </para>
>        <para>
> @@ -2462,7 +2462,12 @@ void (*postclose) (struct drm_device *, struct
> drm_file *);</synopsis> <para>
>          The <methodname>lastclose</methodname> method should restore CRTC
> and plane properties to default value, so that a subsequent open of the
> -	device will not inherit state from the previous user.
> +	device will not inherit state from the previous user. It can also be
> +	used to execute delayed power switching state changes, e.g. in
> +	conjunction with the vga-switcheroo infrastructure. Beyond that KMS
> +	drivers should not do any further cleanup. Only legacy UMS drivers might
> +	need to clean up device state so that the vga console or an independent
> +	fbdev driver could take over.
>        </para>
>      </sect2>
>      <sect2>
> diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
> index 57e3014..fcde7d4 100644
> --- a/drivers/gpu/drm/drm_fops.c
> +++ b/drivers/gpu/drm/drm_fops.c
> @@ -51,7 +51,8 @@ static int drm_setup(struct drm_device * dev)
>  	int i;
>  	int ret;
> 
> -	if (dev->driver->firstopen) {
> +	if (dev->driver->firstopen &&
> +	    !drm_core_check_feature(dev, DRIVER_MODESET)) {
>  		ret = dev->driver->firstopen(dev);
>  		if (ret != 0)
>  			return ret;
-- 
Regards,

Laurent Pinchart