On 09/08/16 02:43 PM, Emily Deng wrote:
> For virtual display feature, as there may be mutiple GPUs,
> for user could choose whiche GPU need to enable this feature, change
> the type of virtual_display from int to char*. The variable will be set
> like this virtual_display="xx:xx.x;xx:xx.x;".
>
> Signed-off-by: Emily Deng <Emily.Deng at amd.com>
When sending updated patches, it's nice to include a changelog
describing what changed between revisions. This can be added either in
the commit log itself or after the --- marker in the e-mail generated by
git format-patch.
> @@ -1182,11 +1183,38 @@ int amdgpu_ip_block_version_cmp(struct amdgpu_device *adev,
> return 1;
> }
>
> +static void amdgpu_whether_enable_virtual_display(struct amdgpu_device *adev)
> +{
> + adev->enable_virtual_display = 0;
> +
> + if (amdgpu_virtual_display) {
> + char pci_address_name[8];
> + char *pciaddstr, *pciaddstr_tmp, *pciaddname;
> + struct drm_device *ddev = adev->ddev;
> +
> + sprintf(pci_address_name, "%02x:%02x.%d", ddev->pdev->bus->number,
> + PCI_SLOT(ddev->pdev->devfn), PCI_FUNC(ddev->pdev->devfn));
Some systems have multiple PCI domains. The domain number needs to be
included, before the bus number.
OTOH I'm not sure the PCI function number needs to be included. Do we
ever do anything with a non-0 PCI function?
> + pciaddstr = kstrdup(amdgpu_virtual_display, GFP_KERNEL);
> + pciaddstr_tmp = pciaddstr;
> + while ((pciaddname = strsep(&pciaddstr_tmp, ";"))) {
> + if (!strncmp(pci_address_name, pciaddname, sizeof(pci_address_name))) {
Please use strcmp instead of strncmp here, otherwise it will match
different strings which contain the PCI address as a prefix.
Other than that, looks good to me.
--
Earthling Michel Dänzer | http://www.amd.com
Libre software enthusiast | Mesa and X developer
