[Mesa-dev] [Mesa-stable] [PATCH] gallium/winsys/kms: fix leaking gem handle.

[Tao Wu(吴涛@Eng)]

Thanks for your review. I got some reply from somebody eles that these
functions actually doesn't work with mainline kernel and it's supposed
to work with modified kernel on chrome os. So perhaps a right fix
should be still on kernel side instead of mesa side. So I will draw
back this
now.

Here is the thread:
https://chromium-review.googlesource.com/c/chromiumos/overlays/chromiumos-overlay/+/764820

On Mon, Nov 13, 2017 at 7:17 AM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
> Hi Tao Wu,
>
> Welcome to Mesa!
>
> On 12 November 2017 at 07:00, Tao Wu <lepton at google.com> wrote:
>> When handle was got from drmPrimeFDToHandle, it can't be destroyed with
>> destroy_dumb. Change to use drm_gem_close to release it. Otherwise video
>> ram could get leaked.
>>
>> Signed-off-by: Tao Wu <lepton at google.com>
>> CC: <mesa-stable at lists.freedesktop.org>
>> ---
>>  src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c | 21 ++++++++++++++++++++-
>>  1 file changed, 20 insertions(+), 1 deletion(-)
>>
>> 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 22e1c936ac5..f7cd09b6aa9 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
>> @@ -27,6 +27,7 @@
>>   *
>>   **************************************************************************/
>>
>> +#include <errno.h>
>>  #include <stdio.h>
>>  #include <stdlib.h>
>>  #include <stddef.h>
>> @@ -172,7 +173,25 @@ kms_sw_displaytarget_destroy(struct sw_winsys *ws,
>>
>>     memset(&destroy_req, 0, sizeof destroy_req);
>>     destroy_req.handle = kms_sw_dt->handle;
>> -   drmIoctl(kms_sw->fd, DRM_IOCTL_MODE_DESTROY_DUMB, &destroy_req);
>> +   int ret = drmIoctl(kms_sw->fd, DRM_IOCTL_MODE_DESTROY_DUMB, &destroy_req);
>> +   /* If handle was from drmPrimeFDToHandle, then kms_sw->fd is connected
>> +    * as render, we have to use drm_gem_close to release it.
>> +    */
>> +   if (ret < 0) {
>> +      if (errno == EACCES) {
> The patch should resolve the leak, although I'm unsure about the
> explicit EACCESS check.
> AFAICT there is no documentation which that said errno will always
> (and only) be returned in this case.
>
> A more robust solution is to have separate bo_list for the
> prime/imported buffers. This way will ensure we use the correct ioctl.
> What do you think, do you think you can give that a spin?
>
> Thanks
> Emil