Re: [Mesa-dev] [PATCH] gallium/winsys/kms: Close drm device filedescriptor on kms_dri_sw_winsys release

[Emil Velikov]

On 14 September 2016 at 15:04, Emil Velikov  wrote:
> Hi Lukasz,
>
> On 5 September 2016 at 17:48, Lukasz Spintzyk  
> wrote:
>> This closes filedescriptor owned by kms_dri_sw_winsys struct. It fixes issue
>> where removal of udl or evdi module used by DisplayLink devices was 
>> impossible
>> due to not closed filedescriptors.
>>
>> When this file descriptor was not closed then command
>> rmmod udl was returning error "Module udl is in use".
>> By this fix xserver does not prevent module removal when usb device
>> is unplugged.
>>
>> Signed-off-by: Lukasz Spintzyk 
>> ---
>>  src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c 
>> b/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
>> index 07eca99..f06ccef 100644
>> --- a/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
>> +++ b/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
>> @@ -371,6 +371,9 @@ kms_sw_displaytarget_display(struct sw_winsys *ws,
>>  static void
>>  kms_destroy_sw_winsys(struct sw_winsys *winsys)
>>  {
>> +   struct kms_sw_winsys *kms_sw = kms_sw_winsys(winsys);
>> +
>> +   close(kms_sw->fd);
> AFAICT not even a single driver/winsys takes ownership of the fd. As
> such they should not close() it.
>
> From a quick skim - on the st/dri side, we explicitly provide new fd
> (we dup the one passed from the upper layers) to the driver. Similarly
> in vl/dri3 we open the device, yet neither of these closes the fd.
>
> Imho it might be worth going through the code paths adding comments
> about the fd ownership at different stages while fixing any leaks that
> you/others come across.
>
> Note: Multiple displays/Xinerama and GL-VDPAU interop are sensitive on
> the area, so I'd suggest testing things carefully and poking the last
> dev who was working in the area (git log/blame are your friend).
>
Actually before embarking on the above make sure that the mesa that
you're using has commit 459cc94507071eec18b746f57a4ec82578a38b54. I've
had a chat with Hans yesterday and seemingly the origin/story behind
it is (almost) the same as yours.

-Emil


> -Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] gallium/winsys/kms: Close drm device filedescriptor on kms_dri_sw_winsys release

[Emil Velikov]

Hi Lukasz,

On 5 September 2016 at 17:48, Lukasz Spintzyk  wrote:
> This closes filedescriptor owned by kms_dri_sw_winsys struct. It fixes issue
> where removal of udl or evdi module used by DisplayLink devices was impossible
> due to not closed filedescriptors.
>
> When this file descriptor was not closed then command
> rmmod udl was returning error "Module udl is in use".
> By this fix xserver does not prevent module removal when usb device
> is unplugged.
>
> Signed-off-by: Lukasz Spintzyk 
> ---
>  src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c 
> b/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
> index 07eca99..f06ccef 100644
> --- a/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
> +++ b/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
> @@ -371,6 +371,9 @@ kms_sw_displaytarget_display(struct sw_winsys *ws,
>  static void
>  kms_destroy_sw_winsys(struct sw_winsys *winsys)
>  {
> +   struct kms_sw_winsys *kms_sw = kms_sw_winsys(winsys);
> +
> +   close(kms_sw->fd);
AFAICT not even a single driver/winsys takes ownership of the fd. As
such they should not close() it.

From a quick skim - on the st/dri side, we explicitly provide new fd
(we dup the one passed from the upper layers) to the driver. Similarly
in vl/dri3 we open the device, yet neither of these closes the fd.

Imho it might be worth going through the code paths adding comments
about the fd ownership at different stages while fixing any leaks that
you/others come across.

Note: Multiple displays/Xinerama and GL-VDPAU interop are sensitive on
the area, so I'd suggest testing things carefully and poking the last
dev who was working in the area (git log/blame are your friend).

-Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] gallium/winsys/kms: Close drm device filedescriptor on kms_dri_sw_winsys release

[Łukasz Spintzyk]

Hi,

Can I do something to have it merged to the source tree?

Fix was already tested, works fine :)

regards
Łukasz Spintzyk

2016-09-05 18:48 GMT+02:00 Lukasz Spintzyk :

> This closes filedescriptor owned by kms_dri_sw_winsys struct. It fixes
> issue
> where removal of udl or evdi module used by DisplayLink devices was
> impossible
> due to not closed filedescriptors.
>
> When this file descriptor was not closed then command
> rmmod udl was returning error "Module udl is in use".
> By this fix xserver does not prevent module removal when usb device
> is unplugged.
>
> Signed-off-by: Lukasz Spintzyk 
> ---
>  src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
> b/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
> index 07eca99..f06ccef 100644
> --- a/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
> +++ b/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
> @@ -371,6 +371,9 @@ kms_sw_displaytarget_display(struct sw_winsys *ws,
>  static void
>  kms_destroy_sw_winsys(struct sw_winsys *winsys)
>  {
> +   struct kms_sw_winsys *kms_sw = kms_sw_winsys(winsys);
> +
> +   close(kms_sw->fd);
> FREE(winsys);
>  }
>
> --
> 2.7.4
>
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH] gallium/winsys/kms: Close drm device filedescriptor on kms_dri_sw_winsys release

[Lukasz Spintzyk]

This closes filedescriptor owned by kms_dri_sw_winsys struct. It fixes issue
where removal of udl or evdi module used by DisplayLink devices was impossible
due to not closed filedescriptors.

When this file descriptor was not closed then command
rmmod udl was returning error "Module udl is in use".
By this fix xserver does not prevent module removal when usb device
is unplugged.

Signed-off-by: Lukasz Spintzyk 
---
 src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c 
b/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
index 07eca99..f06ccef 100644
--- a/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
+++ b/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
@@ -371,6 +371,9 @@ kms_sw_displaytarget_display(struct sw_winsys *ws,
 static void
 kms_destroy_sw_winsys(struct sw_winsys *winsys)
 {
+   struct kms_sw_winsys *kms_sw = kms_sw_winsys(winsys);
+
+   close(kms_sw->fd);
FREE(winsys);
 }
 
-- 
2.7.4

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH] gallium/winsys/kms: Close drm device filedescriptor on kms_dri_sw_winsys release

[Lukasz Spintzyk]

Hi,

This patch fixes issue where unloading kernel modules udl or evdi was
impossible due to not closed file descriptor.
As this library is loaded by Xorg after unpluging device the process
is still keeping file open making unload impossible.

Steps to reproduce:
 - Install latest xorg-server:
To reproduce the issue you have to use latest xorg-server 1.18.4
https://lists.x.org/archives/xorg-devel/2016-July/050459.html
(There was an issue fixed in commit 5b4ced6d3a4c309e1792ac49017fb961a7262e7f 
which was crashing xserver and preventing clear device removal)
 - Plug-in usb 2.0 DisplayLink device. Additional display should be added.
 - Unplug usb device.
 - run command rmmod udl
You should see message:
rmmod: ERROR: Module udl is in use

This is my first patch here. I hope that it will not mess-up to much.

regards
Lukasz Spintzyk

Lukasz Spintzyk (1):
  gallium/winsys/kms: Close drm device filedescriptor on
kms_dri_sw_winsys release

 src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c | 3 +++
 1 file changed, 3 insertions(+)

-- 
2.7.4

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev