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
