[PATCH xf86-video-amdgpu] Fix the X crash for pci_probe path

[Zhou, Jammy]

Hi Michel,

> If DRM master isn't dropped by another X server, how can the PCI probe path succeed? Is it just a timing issue, and the other X server does drop DRM master after the platform probe fails but before the PCI probe is attempted?

Yes, it is a timing issue, and currently there is no good way to wait for the complete of drmDropMaster in the later X servers. Fortunately, with the crash fixed in the PCI probe path, drmSetInterfaceVersion can pass in this alternative path.

I will update the patch later as you suggested.

Regards,
Jammy

-----Original Message-----
From: Michel Dänzer [mailto:michel at daenzer.net] 
Sent: Monday, October 26, 2015 6:03 PM
To: Zhou, Jammy
Cc: xorg-driver-ati at lists.x.org
Subject: Re: [PATCH xf86-video-amdgpu] Fix the X crash for pci_probe path


Hi Jammy,


first of all, I think the subject should be something like "Fix crash in PCI probe path".


On 26.10.2015 16:46, Jammy Zhou wrote:
> For xf86CallDriverProbe, xf86platformProbeDev can usually pass, but it 
> may fail in some cases (i.e, multiple X servers)

"i.e." is an abbreviation of "that is", but I suspect you mean "for example", which can be abbreviated as "e.g.".


> because of the error
> "failed to set DRM interface version 1.4: Permission denied"
> 
> drmSetInterfaceVersion requires drm master, but when do the platform 
> probe for the second X server, the master may not be dropped by previous X server.
> The platform probe fails in this case, and then the pci prove patch is used.

Spelling: "pci prove patch" => "PCI probe path"?

If DRM master isn't dropped by another X server, how can the PCI probe path succeed? Is it just a timing issue, and the other X server does drop DRM master after the platform probe fails but before the PCI probe is attempted?


Anyway, maybe the Git log should say more about exactly why the crash occurs (AMDGPUPTR(pScrn) returns NULL) and how the patch fixes it, instead of the specific circumstances which resulted in the PCI probe path being attempted for you. I think the PCI probe path might be attempted first in some cases anyway.


> Change-Id: Ic4581e3e932157c09a535a198e80601d5f710688

I'd remove this.


> diff --git a/src/amdgpu_probe.c b/src/amdgpu_probe.c index 
> 481271b..c041401 100644
> --- a/src/amdgpu_probe.c
> +++ b/src/amdgpu_probe.c
> @@ -183,6 +183,7 @@ static Bool amdgpu_open_drm_master(ScrnInfoPtr pScrn)
>  		return FALSE;
>  	}
>  
> +	info->drmmode.fd = pAMDGPUEnt->fd = info->dri2.drm_fd;
>  	return TRUE;
>  }
>  
> @@ -234,13 +235,23 @@ static Bool amdgpu_get_scrninfo(int entity_num, void *pci_dev)
>  	if (!pPriv->ptr) {
>  		uint32_t major_version;
>  		uint32_t minor_version;
> +		AMDGPUInfoPtr info;
>  
>  		pPriv->ptr = xnfcalloc(sizeof(AMDGPUEntRec), 1);
>  		pAMDGPUEnt = pPriv->ptr;
> +		if (!pPriv->ptr)
> +			return FALSE;
> +
> +		pScrn->driverPrivate = xnfcalloc(sizeof(AMDGPUInfoRec), 1);
> +		if (!pScrn->driverPrivate)
> +			goto error_private;
>  
> -		if (amdgpu_open_drm_master(pScrn)) {
> +		info = (AMDGPUInfoPtr)pScrn->driverPrivate;
> +		info->pEnt = pEnt;
> +		info->PciInfo = xf86GetPciInfoForEntity(info->pEnt->index);
> +
> +		if (!amdgpu_open_drm_master(pScrn))
>  			goto error_fd;
> -		}
>  
>  		pAMDGPUEnt->fd_ref = 1;
>  

I think it would be better to make amdgpu_open_drm_master use
pAMDGPUEnt->fd instead of info->dri2.drm_fd, eliminating all
AMDGPUInfoPtr related code from amdgpu_probe.c.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer