Re: [PATCH xf86-video-amdgpu] amdgpu_probe: Do not close server managed drm fds

Re: [PATCH xf86-video-amdgpu] amdgpu_probe: Do not close server managed drm fds

[Michel Dänzer]

[ Moving to the amd-gfx mailing list, where xf86-video-amdgpu (and -ati)
patches are reviewed ]

On 18/10/16 11:48 PM, Hans de Goede wrote:
> This fixes the xserver only seeing AMD/ATI devices supported by the amdgpu
> driver, as by the time xf86-video-ati gets a chance to probe them, the
> fd has been closed.

That basically makes sense, but I have one question: Why does amdgpu get
probed in the first place in that case? I was under the impression that
this should only happen for GPUs bound to the amdgpu kernel driver, due
to Section "OutputClass" in /usr/share/X11/xorg.conf.d/10-amdgpu.conf .


> diff --git a/src/amdgpu_probe.c b/src/amdgpu_probe.c
> index 213d245..5d17fe5 100644
> --- a/src/amdgpu_probe.c
> +++ b/src/amdgpu_probe.c
> @@ -142,6 +142,15 @@ static int amdgpu_kernel_open_fd(ScrnInfoPtr pScrn, char *busid,
>  	return fd;
>  }
>  
> +static void amdgpu_kernel_close_fd(AMDGPUEntPtr pAMDGPUEnt)
> +{
> +#ifdef XF86_PDEV_SERVER_FD
> +	if (!(pAMDGPUEnt->platform_dev &&
> +	      pAMDGPUEnt->platform_dev->flags & XF86_PDEV_SERVER_FD))
> +#endif
> +		drmClose(pAMDGPUEnt->fd);
> +}

AMDGPUFreeRec could also be refactored to call this function.


> @@ -164,7 +173,7 @@ static Bool amdgpu_open_drm_master(ScrnInfoPtr pScrn, AMDGPUEntPtr pAMDGPUEnt,
>  	if (err != 0) {
>  		xf86DrvMsg(pScrn->scrnIndex, X_ERROR,
>  			   "[drm] failed to set drm interface version.\n");
> -		drmClose(pAMDGPUEnt->fd);
> +		amdgpu_kernel_close_fd(pAMDGPUEnt);
>  		return FALSE;
>  	}
> @@ -252,7 +261,7 @@ static Bool amdgpu_get_scrninfo(int entity_num, struct pci_device *pci_dev)
>  	return TRUE;
>  
>  error_amdgpu:
> -	drmClose(pAMDGPUEnt->fd);
> +	amdgpu_kernel_close_fd(pAMDGPUEnt);
>  error_fd:
>  	free(pPriv->ptr);
>  error:

These two hunks aren't really necessary, right? These only get called
from amdgpu_pci_probe, in which case pAMDGPUEnt->platform_dev remains NULL.


> @@ -347,6 +356,7 @@ amdgpu_platform_probe(DriverPtr pDriver,
>  
>  		pPriv->ptr = xnfcalloc(sizeof(AMDGPUEntRec), 1);
>  		pAMDGPUEnt = pPriv->ptr;
> +		pAMDGPUEnt->platform_dev = dev;
>  		pAMDGPUEnt->fd = amdgpu_kernel_open_fd(pScrn, busid, dev);
>  		if (pAMDGPUEnt->fd < 0)
>  			goto error_fd;
> @@ -365,7 +375,6 @@ amdgpu_platform_probe(DriverPtr pDriver,
>  		pAMDGPUEnt = pPriv->ptr;
>  		pAMDGPUEnt->fd_ref++;
>  	}
> -	pAMDGPUEnt->platform_dev = dev;
>  
>  	xf86SetEntityInstanceForScreen(pScrn, pEnt->index,
>  				       xf86GetNumEntityInstances(pEnt->

These two hunks aren't really necessary either, are they?


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH xf86-video-amdgpu] amdgpu_probe: Do not close server managed drm fds

[Hans de Goede]

Hi,

On 19-10-16 04:42, Michel Dänzer wrote:
>
> [ Moving to the amd-gfx mailing list, where xf86-video-amdgpu (and -ati)
> patches are reviewed ]
>
> On 18/10/16 11:48 PM, Hans de Goede wrote:
>> This fixes the xserver only seeing AMD/ATI devices supported by the amdgpu
>> driver, as by the time xf86-video-ati gets a chance to probe them, the
>> fd has been closed.
>
> That basically makes sense, but I have one question: Why does amdgpu get
> probed in the first place in that case? I was under the impression that
> this should only happen for GPUs bound to the amdgpu kernel driver, due
> to Section "OutputClass" in /usr/share/X11/xorg.conf.d/10-amdgpu.conf .

Good question, I honestly don't know and I do not have time to investigate.
You should be able to reproduce this yourself, if not let me know.

>> diff --git a/src/amdgpu_probe.c b/src/amdgpu_probe.c
>> index 213d245..5d17fe5 100644
>> --- a/src/amdgpu_probe.c
>> +++ b/src/amdgpu_probe.c
>> @@ -142,6 +142,15 @@ static int amdgpu_kernel_open_fd(ScrnInfoPtr pScrn, char *busid,
>>  	return fd;
>>  }
>>
>> +static void amdgpu_kernel_close_fd(AMDGPUEntPtr pAMDGPUEnt)
>> +{
>> +#ifdef XF86_PDEV_SERVER_FD
>> +	if (!(pAMDGPUEnt->platform_dev &&
>> +	      pAMDGPUEnt->platform_dev->flags & XF86_PDEV_SERVER_FD))
>> +#endif
>> +		drmClose(pAMDGPUEnt->fd);
>> +}
>
> AMDGPUFreeRec could also be refactored to call this function.

Ack.

>> @@ -164,7 +173,7 @@ static Bool amdgpu_open_drm_master(ScrnInfoPtr pScrn, AMDGPUEntPtr pAMDGPUEnt,
>>  	if (err != 0) {
>>  		xf86DrvMsg(pScrn->scrnIndex, X_ERROR,
>>  			   "[drm] failed to set drm interface version.\n");
>> -		drmClose(pAMDGPUEnt->fd);
>> +		amdgpu_kernel_close_fd(pAMDGPUEnt);
>>  		return FALSE;
>>  	}
>> @@ -252,7 +261,7 @@ static Bool amdgpu_get_scrninfo(int entity_num, struct pci_device *pci_dev)
>>  	return TRUE;
>>
>>  error_amdgpu:
>> -	drmClose(pAMDGPUEnt->fd);
>> +	amdgpu_kernel_close_fd(pAMDGPUEnt);
>>  error_fd:
>>  	free(pPriv->ptr);
>>  error:
>
> These two hunks aren't really necessary, right? These only get called
> from amdgpu_pci_probe, in which case pAMDGPUEnt->platform_dev remains NULL.

The names of the functions do not imply amdgpu_pci_probe() use only, so even
though that is true today it might not be true in the future. IOW better
safe then sorry.

>> @@ -347,6 +356,7 @@ amdgpu_platform_probe(DriverPtr pDriver,
>>
>>  		pPriv->ptr = xnfcalloc(sizeof(AMDGPUEntRec), 1);
>>  		pAMDGPUEnt = pPriv->ptr;
>> +		pAMDGPUEnt->platform_dev = dev;
>>  		pAMDGPUEnt->fd = amdgpu_kernel_open_fd(pScrn, busid, dev);
>>  		if (pAMDGPUEnt->fd < 0)
>>  			goto error_fd;
>> @@ -365,7 +375,6 @@ amdgpu_platform_probe(DriverPtr pDriver,
>>  		pAMDGPUEnt = pPriv->ptr;
>>  		pAMDGPUEnt->fd_ref++;
>>  	}
>> -	pAMDGPUEnt->platform_dev = dev;
>>
>>  	xf86SetEntityInstanceForScreen(pScrn, pEnt->index,
>>  				       xf86GetNumEntityInstances(pEnt->
>
> These two hunks aren't really necessary either, are they?

Actually they are, quoting some more of the (new after the patch) code:

                 pAMDGPUEnt->platform_dev = dev;
                 pAMDGPUEnt->fd = amdgpu_kernel_open_fd(pScrn, busid, dev);
                 if (pAMDGPUEnt->fd < 0)
                         goto error_fd;

                 pAMDGPUEnt->fd_ref = 1;

                 if (amdgpu_device_initialize(pAMDGPUEnt->fd,
                                              &major_version,
                                              &minor_version,
                                              &pAMDGPUEnt->pDev)) {
                         xf86DrvMsg(pScrn->scrnIndex, X_ERROR,
                                    "amdgpu_device_initialize failed\n");
                         goto error_amdgpu;
		}

...

error_amdgpu:
         amdgpu_kernel_close_fd(pAMDGPUEnt);
error_fd:
         free(pPriv->ptr);
error:
         free(busid);
         return FALSE;
}

And amdgpu_kernel_close_fd accesses pAMDGPUEnt->platform_dev, so it needs
to be set earlier then before.

Shall I send a v2 with AMDGPUFreeRec refactored to call amdgpu_kernel_close_fd
and the rest kept as is ?

Regards,

Hans
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH xf86-video-amdgpu] amdgpu_probe: Do not close server managed drm fds

[Michel Dänzer]

On 19/10/16 06:49 PM, Hans de Goede wrote:
> On 19-10-16 04:42, Michel Dänzer wrote:
>> On 18/10/16 11:48 PM, Hans de Goede wrote:
>>> This fixes the xserver only seeing AMD/ATI devices supported by the
>>> amdgpu
>>> driver, as by the time xf86-video-ati gets a chance to probe them, the
>>> fd has been closed.
>>
>> That basically makes sense, but I have one question: Why does amdgpu get
>> probed in the first place in that case? I was under the impression that
>> this should only happen for GPUs bound to the amdgpu kernel driver, due
>> to Section "OutputClass" in /usr/share/X11/xorg.conf.d/10-amdgpu.conf .
> 
> Good question, I honestly don't know and I do not have time to investigate.
> You should be able to reproduce this yourself, if not let me know.

I haven't run into this myself and am not sure how I could reproduce it.
Anyway, the patch is clearly the right thing to do regardless, so it's
not a big deal but just curiosity on my part.


>>> @@ -164,7 +173,7 @@ static Bool amdgpu_open_drm_master(ScrnInfoPtr
>>> pScrn, AMDGPUEntPtr pAMDGPUEnt,
>>>      if (err != 0) {
>>>          xf86DrvMsg(pScrn->scrnIndex, X_ERROR,
>>>                 "[drm] failed to set drm interface version.\n");
>>> -        drmClose(pAMDGPUEnt->fd);
>>> +        amdgpu_kernel_close_fd(pAMDGPUEnt);
>>>          return FALSE;
>>>      }
>>> @@ -252,7 +261,7 @@ static Bool amdgpu_get_scrninfo(int entity_num,
>>> struct pci_device *pci_dev)
>>>      return TRUE;
>>>
>>>  error_amdgpu:
>>> -    drmClose(pAMDGPUEnt->fd);
>>> +    amdgpu_kernel_close_fd(pAMDGPUEnt);
>>>  error_fd:
>>>      free(pPriv->ptr);
>>>  error:
>>
>> These two hunks aren't really necessary, right? These only get called
>> from amdgpu_pci_probe, in which case pAMDGPUEnt->platform_dev remains
>> NULL.
> 
> The names of the functions do not imply amdgpu_pci_probe() use only, so
> even
> though that is true today it might not be true in the future. IOW better
> safe then sorry.

Makes sense.


>>> @@ -347,6 +356,7 @@ amdgpu_platform_probe(DriverPtr pDriver,
>>>
>>>          pPriv->ptr = xnfcalloc(sizeof(AMDGPUEntRec), 1);
>>>          pAMDGPUEnt = pPriv->ptr;
>>> +        pAMDGPUEnt->platform_dev = dev;
>>>          pAMDGPUEnt->fd = amdgpu_kernel_open_fd(pScrn, busid, dev);
>>>          if (pAMDGPUEnt->fd < 0)
>>>              goto error_fd;
>>> @@ -365,7 +375,6 @@ amdgpu_platform_probe(DriverPtr pDriver,
>>>          pAMDGPUEnt = pPriv->ptr;
>>>          pAMDGPUEnt->fd_ref++;
>>>      }
>>> -    pAMDGPUEnt->platform_dev = dev;
>>>
>>>      xf86SetEntityInstanceForScreen(pScrn, pEnt->index,
>>>                         xf86GetNumEntityInstances(pEnt->
>>
>> These two hunks aren't really necessary either, are they?
> 
> Actually they are, quoting some more of the (new after the patch) code:

[...]

> And amdgpu_kernel_close_fd accesses pAMDGPUEnt->platform_dev, so it needs
> to be set earlier then before.

Gotcha, thanks.


> Shall I send a v2 with AMDGPUFreeRec refactored to call
> amdgpu_kernel_close_fd
> and the rest kept as is ?

Yes, please.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx