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
       [not found] ` <20161018144840.7163-1-hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
@ 2016-10-19  2:42   ` Michel Dänzer
       [not found]     ` <2bc76f11-bb9e-8411-35ee-c76d535d933e-otUistvHUpPR7s880joybQ@public.gmane.org>
  0 siblings, 1 reply; 3+ messages in thread
From: Michel Dänzer @ 2016-10-19  2:42 UTC (permalink / raw)
  To: Hans de Goede; +Cc: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW

[ 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

^ permalink raw reply	[flat|nested] 3+ messages in thread