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

[Michel Dänzer]

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