Re: [PATCH] drm/amdgpu: Change the virtual_display type from int to char*.

2016-08-09 Thread Michel Dänzer 
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 

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(_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
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH] drm/amdgpu: Change the virtual_display type from int to char*.

2016-08-08 Thread Deng, Emily 


> -Original Message-
> From: Edward O'Callaghan [mailto:funfunc...@folklore1984.net]
> Sent: Tuesday, August 09, 2016 11:54 AM
> To: Deng, Emily <emily.d...@amd.com>; amd-gfx@lists.freedesktop.org
> Subject: Re: [PATCH] drm/amdgpu: Change the virtual_display type from int to
> char*.
> 
> 
> 
> On 08/09/2016 01:47 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.d...@amd.com>
> > ---
> >  drivers/gpu/drm/amd/amdgpu/amdgpu.h|  3 ++-
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 31
> +-
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c|  6 +++---
> >  drivers/gpu/drm/amd/amdgpu/cik.c   |  2 +-
> >  drivers/gpu/drm/amd/amdgpu/vi.c|  2 +-
> >  5 files changed, 37 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> > index c309eaf..b6bed4e 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> > @@ -93,7 +93,7 @@ extern unsigned amdgpu_pcie_lane_cap;  extern
> > unsigned amdgpu_cg_mask;  extern unsigned amdgpu_pg_mask;  extern int
> > amdgpu_sclk_deep_sleep_en; -extern int amdgpu_virtual_display;
> > +extern char *amdgpu_virtual_display;
> >
> >  #define AMDGPU_WAIT_IDLE_TIMEOUT_IN_MS 3000
> >  #define AMDGPU_MAX_USEC_TIMEOUT10  /*
> 100 ms */
> > @@ -2142,6 +2142,7 @@ struct amdgpu_device {
> > struct kfd_dev  *kfd;
> >
> > struct amdgpu_virtualization virtualization;
> > +   int enable_virtual_display;
> >  };
> >
> >  bool amdgpu_device_is_px(struct drm_device *dev); diff --git
> > a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > index 68cce12..62efe14 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > @@ -47,6 +47,7 @@
> >  #endif
> >  #include "vi.h"
> >  #include "bif/bif_4_1_d.h"
> > +#include 
> >
> >  static int amdgpu_debugfs_regs_init(struct amdgpu_device *adev);
> > static void amdgpu_debugfs_regs_cleanup(struct amdgpu_device *adev);
> > @@ -1182,11 +1183,39 @@ 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));
> > +
> > +   pciaddstr = kstrdup(amdgpu_virtual_display, GFP_KERNEL);
> > +   pciaddstr_tmp = pciaddstr;
> > +   while ((pciaddname = strsep(_tmp, ";"))) {
> > +   if (!strncmp(pci_address_name, pciaddname,
> sizeof(pci_address_name))) {
> > +   adev->enable_virtual_display = 1;
> > +   break;
> > +   }
> > +   }
> > +
> > +   DRM_INFO("virtual display string:%s, %s:virtual_display:%d\n",
> > +amdgpu_virtual_display, pci_address_name,
> > +adev->enable_virtual_display);
> > +
> > +   if(pciaddstr)
> > +   kfree(pciaddstr);
> kfree() should be NULL-safe so no need for the branch.
> 
[[EmilyD]] Ok, I will remove, thanks.
> Kind Regards,
> Edward.
> 
> > +   }
> > +}
> > +
> >  static int amdgpu_early_init(struct amdgpu_device *adev)  {
> > int i, r;
> >
> > -   DRM_INFO("virtual display enabled:%d\n", amdgpu_virtual_display);
> > +   amdgpu_whether_enable_virtual_display(adev);
> >
> > switch (adev->asic_type) {
> > case CHIP_TOPAZ:
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> > index 7007c

Re: [PATCH] drm/amdgpu: Change the virtual_display type from int to char*.

2016-08-08 Thread Edward O'Callaghan 


On 08/09/2016 01:47 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 
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu.h|  3 ++-
>  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 31 
> +-
>  drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c|  6 +++---
>  drivers/gpu/drm/amd/amdgpu/cik.c   |  2 +-
>  drivers/gpu/drm/amd/amdgpu/vi.c|  2 +-
>  5 files changed, 37 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index c309eaf..b6bed4e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -93,7 +93,7 @@ extern unsigned amdgpu_pcie_lane_cap;
>  extern unsigned amdgpu_cg_mask;
>  extern unsigned amdgpu_pg_mask;
>  extern int amdgpu_sclk_deep_sleep_en;
> -extern int amdgpu_virtual_display;
> +extern char *amdgpu_virtual_display;
>  
>  #define AMDGPU_WAIT_IDLE_TIMEOUT_IN_MS   3000
>  #define AMDGPU_MAX_USEC_TIMEOUT  10  /* 100 ms */
> @@ -2142,6 +2142,7 @@ struct amdgpu_device {
>   struct kfd_dev  *kfd;
>  
>   struct amdgpu_virtualization virtualization;
> + int enable_virtual_display;
>  };
>  
>  bool amdgpu_device_is_px(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 68cce12..62efe14 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -47,6 +47,7 @@
>  #endif
>  #include "vi.h"
>  #include "bif/bif_4_1_d.h"
> +#include 
>  
>  static int amdgpu_debugfs_regs_init(struct amdgpu_device *adev);
>  static void amdgpu_debugfs_regs_cleanup(struct amdgpu_device *adev);
> @@ -1182,11 +1183,39 @@ 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));
> +
> + pciaddstr = kstrdup(amdgpu_virtual_display, GFP_KERNEL);
> + pciaddstr_tmp = pciaddstr;
> + while ((pciaddname = strsep(_tmp, ";"))) {
> + if (!strncmp(pci_address_name, pciaddname, 
> sizeof(pci_address_name))) {
> + adev->enable_virtual_display = 1;
> + break;
> + }
> + }
> +
> + DRM_INFO("virtual display string:%s, %s:virtual_display:%d\n", 
> amdgpu_virtual_display, pci_address_name, adev->enable_virtual_display);
> +
> + if(pciaddstr)
> + kfree(pciaddstr);
kfree() should be NULL-safe so no need for the branch.

Kind Regards,
Edward.

> + }
> +}
> +
>  static int amdgpu_early_init(struct amdgpu_device *adev)
>  {
>   int i, r;
>  
> - DRM_INFO("virtual display enabled:%d\n", amdgpu_virtual_display);
> + amdgpu_whether_enable_virtual_display(adev);
>  
>   switch (adev->asic_type) {
>   case CHIP_TOPAZ:
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index 7007cf7..4202602 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -90,7 +90,7 @@ unsigned amdgpu_pcie_gen_cap = 0;
>  unsigned amdgpu_pcie_lane_cap = 0;
>  unsigned amdgpu_cg_mask = 0x;
>  unsigned amdgpu_pg_mask = 0x;
> -int amdgpu_virtual_display = 0;
> +char *amdgpu_virtual_display = NULL;
>  
>  MODULE_PARM_DESC(vramlimit, "Restrict VRAM for testing, in megabytes");
>  module_param_named(vramlimit, amdgpu_vram_limit, int, 0600);
> @@ -190,8 +190,8 @@ module_param_named(cg_mask, amdgpu_cg_mask, uint, 0444);
>  MODULE_PARM_DESC(pg_mask, "Powergating flags mask (0 = disable power 
> gating)");
>  module_param_named(pg_mask, amdgpu_pg_mask, uint, 0444);
>  
> -MODULE_PARM_DESC(virtual_display, "enable virtual display (0 = disable 
> virtual display)");
> -module_param_named(virtual_display, amdgpu_virtual_display, int, 0444);
> +MODULE_PARM_DESC(virtual_display, "Enable virtual display feature (the 
> virtual_display will be set like xx:xx.x;xx:xx.x)");
> +module_param_named(virtual_display, amdgpu_virtual_display, charp, 0444);
>  
>  static const struct pci_device_id pciidlist[] = {
>  #ifdef