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
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
