[3/5] drm/amdgpu: Paper over the drm_driver mangling for virt

Commit Message

Daniel Vetter Oct. 30, 2020, 10:11 a.m. UTC

Prep work to make drm_device->driver const.

Signed-off-by: Daniel Vetter <[e-mail]>
Cc: Alex Deucher <[e-mail]>
Cc: "Christian König" <[e-mail]>
Cc: Evan Quan <[e-mail]>
Cc: Felix Kuehling <[e-mail]>
Cc: Hawking Zhang <[e-mail]>
Cc: Andrey Grodzovsky <[e-mail]>
Cc: Luben Tuikov <[e-mail]>
Cc: Thomas Zimmermann <[e-mail]>
Cc: Monk Liu <[e-mail]>
Cc: Yintian Tao <[e-mail]>
Cc: Dennis Li <[e-mail]>
Cc: shaoyunl <[e-mail]>
Cc: Bokun Zhang <[e-mail]>
Cc: "Stanley.Yang" <[e-mail]>
Cc: Wenhui Sheng <[e-mail]>
Cc: chen gong <[e-mail]>
Signed-off-by: Daniel Vetter <[e-mail]>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c  |  8 ++++----
 drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c | 12 +++++++++++-
 2 files changed, 15 insertions(+), 5 deletions(-)

Comments

Liu, Monk Oct. 30, 2020, 10:41 a.m. UTC | #1

[AMD Official Use Only - Internal Distribution Only]

What's the purpose of the patch sets

e.g.: what bug can those 5 patches fix or what feature provided

for this particular one (3/5) I didn't see how it helpful, could you give a background  ?

thanks

Daniel Vetter Oct. 30, 2020, 12:04 p.m. UTC | #2

On Fri, Oct 30, 2020 at 11:41 AM Liu, Monk <[e-mail]> wrote:
>
> [AMD Official Use Only - Internal Distribution Only]
>
> What's the purpose of the patch sets
>
> e.g.: what bug can those 5 patches fix or what feature provided
>
> for this particular one (3/5) I didn't see how it helpful, could you give a background  ?

It's good to make function tables const, so that they can be write
protected. More resilience against exploits and all that. This patch
here is needed to be able to make drm_device->driver const so that all
other drivers can make their drm_driver structure const. Would be good
to fully fix up amdgpu like in the comment, but I'm not going that in
this series here.
-Daniel

>
> thanks
> _____________________________________
> Monk Liu|GPU Virtualization Team |AMD
>
>
> -----Original Message-----
> From: Daniel Vetter <[e-mail]>
> Sent: Friday, October 30, 2020 6:11 PM
> To: DRI Development <[e-mail]>
> Cc: Intel Graphics Development <[e-mail]>; Daniel Vetter <[e-mail]>; Deucher, Alexander <[e-mail]>; Koenig, Christian <[e-mail]>; Quan, Evan <[e-mail]>; Kuehling, Felix <[e-mail]>; Zhang, Hawking <[e-mail]>; Grodzovsky, Andrey <[e-mail]>; Tuikov, Luben <[e-mail]>; Thomas Zimmermann <[e-mail]>; Liu, Monk <[e-mail]>; Yintian Tao <[e-mail]>; Li, Dennis <[e-mail]>; Liu, Shaoyun <[e-mail]>; Zhang, Bokun <[e-mail]>; Yang, Stanley <[e-mail]>; Sheng, Wenhui <[e-mail]>; Gong, Curry <[e-mail]>; Daniel Vetter <[e-mail]>
> Subject: [PATCH 3/5] drm/amdgpu: Paper over the drm_driver mangling for virt
>
> Prep work to make drm_device->driver const.
>
> Signed-off-by: Daniel Vetter <[e-mail]>
> Cc: Alex Deucher <[e-mail]>
> Cc: "Christian König" <[e-mail]>
> Cc: Evan Quan <[e-mail]>
> Cc: Felix Kuehling <[e-mail]>
> Cc: Hawking Zhang <[e-mail]>
> Cc: Andrey Grodzovsky <[e-mail]>
> Cc: Luben Tuikov <[e-mail]>
> Cc: Thomas Zimmermann <[e-mail]>
> Cc: Monk Liu <[e-mail]>
> Cc: Yintian Tao <[e-mail]>
> Cc: Dennis Li <[e-mail]>
> Cc: shaoyunl <[e-mail]>
> Cc: Bokun Zhang <[e-mail]>
> Cc: "Stanley.Yang" <[e-mail]>
> Cc: Wenhui Sheng <[e-mail]>
> Cc: chen gong <[e-mail]>
> Signed-off-by: Daniel Vetter <[e-mail]>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c  |  8 ++++----  drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c | 12 +++++++++++-
>  2 files changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index 024c3b70b1aa..3d337f13ae4e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -1093,7 +1093,7 @@ static const struct pci_device_id pciidlist[] = {
>
>  MODULE_DEVICE_TABLE(pci, pciidlist);
>
> -static struct drm_driver kms_driver;
> +struct drm_driver amdgpu_kms_driver;
>
>  static int amdgpu_pci_probe(struct pci_dev *pdev,
>      const struct pci_device_id *ent) @@ -1164,7 +1164,7 @@ static int amdgpu_pci_probe(struct pci_dev *pdev,
>  if (ret)
>  return ret;
>
> -adev = devm_drm_dev_alloc(&pdev->dev, &kms_driver, typeof(*adev), ddev);
> +adev = devm_drm_dev_alloc(&pdev->dev, &amdgpu_kms_driver,
> +typeof(*adev), ddev);
>  if (IS_ERR(adev))
>  return PTR_ERR(adev);
>
> @@ -1508,7 +1508,7 @@ int amdgpu_file_to_fpriv(struct file *filp, struct amdgpu_fpriv **fpriv)
>  return 0;
>  }
>
> -static struct drm_driver kms_driver = {
> +struct drm_driver amdgpu_kms_driver = {
>  .driver_features =
>      DRIVER_ATOMIC |
>      DRIVER_GEM |
> @@ -1571,7 +1571,7 @@ static int __init amdgpu_init(void)
>  goto error_fence;
>
>  DRM_INFO("amdgpu kernel modesetting enabled.\n");
> -kms_driver.num_ioctls = amdgpu_max_kms_ioctl;
> +amdgpu_kms_driver.num_ioctls = amdgpu_max_kms_ioctl;
>  amdgpu_register_atpx_handler();
>
>  /* Ignore KFD init failures. Normal when CONFIG_HSA_AMD is not set. */ diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
> index d0aea5e39531..dde4c449c284 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
> @@ -45,13 +45,23 @@ bool amdgpu_virt_mmio_blocked(struct amdgpu_device *adev)
>  return RREG32_NO_KIQ(0xc040) == 0xffffffff;  }
>
> +extern struct drm_driver amdgpu_kms_driver;
> +
>  void amdgpu_virt_init_setting(struct amdgpu_device *adev)  {
>  /* enable virtual display */
>  if (adev->mode_info.num_crtc == 0)
>  adev->mode_info.num_crtc = 1;
>  adev->enable_virtual_display = true;
> -adev_to_drm(adev)->driver->driver_features &= ~DRIVER_ATOMIC;
> +
> +/*
> + * FIXME: Either make virt support atomic or make sure you have two
> + * drm_driver structs, these kind of tricks are only ok when there's
> + * guaranteed only a single device per system. This should also be done
> + * before struct drm_device is initialized.
> + */
> +amdgpu_kms_driver.driver_features &= ~DRIVER_ATOMIC;
> +
>  adev->cg_flags = 0;
>  adev->pg_flags = 0;
>  }
> --
> 2.28.0
>

Alex Deucher Oct. 30, 2020, 6:47 p.m. UTC | #3

On Fri, Oct 30, 2020 at 6:11 AM Daniel Vetter <[e-mail]> wrote:
>
> Prep work to make drm_device->driver const.
>
> Signed-off-by: Daniel Vetter <[e-mail]>
> Cc: Alex Deucher <[e-mail]>
> Cc: "Christian König" <[e-mail]>
> Cc: Evan Quan <[e-mail]>
> Cc: Felix Kuehling <[e-mail]>
> Cc: Hawking Zhang <[e-mail]>
> Cc: Andrey Grodzovsky <[e-mail]>
> Cc: Luben Tuikov <[e-mail]>
> Cc: Thomas Zimmermann <[e-mail]>
> Cc: Monk Liu <[e-mail]>
> Cc: Yintian Tao <[e-mail]>
> Cc: Dennis Li <[e-mail]>
> Cc: shaoyunl <[e-mail]>
> Cc: Bokun Zhang <[e-mail]>
> Cc: "Stanley.Yang" <[e-mail]>
> Cc: Wenhui Sheng <[e-mail]>
> Cc: chen gong <[e-mail]>
> Signed-off-by: Daniel Vetter <[e-mail]>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c  |  8 ++++----
>  drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c | 12 +++++++++++-
>  2 files changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index 024c3b70b1aa..3d337f13ae4e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -1093,7 +1093,7 @@ static const struct pci_device_id pciidlist[] = {
>
>  MODULE_DEVICE_TABLE(pci, pciidlist);
>
> -static struct drm_driver kms_driver;
> +struct drm_driver amdgpu_kms_driver;
>
>  static int amdgpu_pci_probe(struct pci_dev *pdev,
>                             const struct pci_device_id *ent)
> @@ -1164,7 +1164,7 @@ static int amdgpu_pci_probe(struct pci_dev *pdev,
>         if (ret)
>                 return ret;
>
> -       adev = devm_drm_dev_alloc(&pdev->dev, &kms_driver, typeof(*adev), ddev);
> +       adev = devm_drm_dev_alloc(&pdev->dev, &amdgpu_kms_driver, typeof(*adev), ddev);
>         if (IS_ERR(adev))
>                 return PTR_ERR(adev);
>
> @@ -1508,7 +1508,7 @@ int amdgpu_file_to_fpriv(struct file *filp, struct amdgpu_fpriv **fpriv)
>         return 0;
>  }
>
> -static struct drm_driver kms_driver = {
> +struct drm_driver amdgpu_kms_driver = {
>         .driver_features =
>             DRIVER_ATOMIC |
>             DRIVER_GEM |
> @@ -1571,7 +1571,7 @@ static int __init amdgpu_init(void)
>                 goto error_fence;
>
>         DRM_INFO("amdgpu kernel modesetting enabled.\n");
> -       kms_driver.num_ioctls = amdgpu_max_kms_ioctl;
> +       amdgpu_kms_driver.num_ioctls = amdgpu_max_kms_ioctl;
>         amdgpu_register_atpx_handler();
>
>         /* Ignore KFD init failures. Normal when CONFIG_HSA_AMD is not set. */
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
> index d0aea5e39531..dde4c449c284 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
> @@ -45,13 +45,23 @@ bool amdgpu_virt_mmio_blocked(struct amdgpu_device *adev)
>         return RREG32_NO_KIQ(0xc040) == 0xffffffff;
>  }
>
> +extern struct drm_driver amdgpu_kms_driver;
> +
>  void amdgpu_virt_init_setting(struct amdgpu_device *adev)
>  {
>         /* enable virtual display */
>         if (adev->mode_info.num_crtc == 0)
>                 adev->mode_info.num_crtc = 1;
>         adev->enable_virtual_display = true;
> -       adev_to_drm(adev)->driver->driver_features &= ~DRIVER_ATOMIC;
> +
> +       /*
> +        * FIXME: Either make virt support atomic or make sure you have two
> +        * drm_driver structs, these kind of tricks are only ok when there's
> +        * guaranteed only a single device per system. This should also be done
> +        * before struct drm_device is initialized.
> +        */
> +       amdgpu_kms_driver.driver_features &= ~DRIVER_ATOMIC;

There is additional DRIVER_ATOMIC in amdgpu_pci_probe() for older
chips without atomic support.

Alex

> +
>         adev->cg_flags = 0;
>         adev->pg_flags = 0;
>  }
> --
> 2.28.0
>
> _______________________________________________
> dri-devel mailing list
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Luben Tuikov Oct. 31, 2020, 5:09 a.m. UTC | #4

On 2020-10-30 08:04, Daniel Vetter wrote:
> On Fri, Oct 30, 2020 at 11:41 AM Liu, Monk <[e-mail]> wrote:
>>
>> [AMD Official Use Only - Internal Distribution Only]
>>
>> What's the purpose of the patch sets
>>
>> e.g.: what bug can those 5 patches fix or what feature provided
>>
>> for this particular one (3/5) I didn't see how it helpful, could you give a background  ?
> 
> It's good to make function tables const, so that they can be write
> protected. More resilience against exploits and all that. This patch
> here is needed to be able to make drm_device->driver const so that all
> other drivers can make their drm_driver structure const. Would be good
> to fully fix up amdgpu like in the comment, but I'm not going that in
> this series here.
> -Daniel

Hi Daniel,

I feel that that's a good change.

But if you can clarify this for me... Is this leading
towards a single instance of a struct drm_driver per
low-level driver, i.e. amdgpu?

And as such, being able to be defined as const?

So that we have many GPU devices driven by one
low-level driver (amdgpu_drv), represented by one
const drm_driver (and thus const)?

Which would imply that if varied devices can be handled
by a single low-level driver, whose struct drm_driver
settings cannot be shared among subset of devices (say
very old and new), then the low-level driver
would have to create more than one "const" struct drm_driver?

Regards,
Luben

> 
>>
>> thanks
>> _____________________________________
>> Monk Liu|GPU Virtualization Team |AMD
>>
>>
>> -----Original Message-----
>> From: Daniel Vetter <[e-mail]>
>> Sent: Friday, October 30, 2020 6:11 PM
>> To: DRI Development <[e-mail]>
>> Cc: Intel Graphics Development <[e-mail]>; Daniel Vetter <[e-mail]>; Deucher, Alexander <[e-mail]>; Koenig, Christian <[e-mail]>; Quan, Evan <[e-mail]>; Kuehling, Felix <[e-mail]>; Zhang, Hawking <[e-mail]>; Grodzovsky, Andrey <[e-mail]>; Tuikov, Luben <[e-mail]>; Thomas Zimmermann <[e-mail]>; Liu, Monk <[e-mail]>; Yintian Tao <[e-mail]>; Li, Dennis <[e-mail]>; Liu, Shaoyun <[e-mail]>; Zhang, Bokun <[e-mail]>; Yang, Stanley <[e-mail]>; Sheng, Wenhui <[e-mail]>; Gong, Curry <[e-mail]>; Daniel Vetter <[e-mail]>
>> Subject: [PATCH 3/5] drm/amdgpu: Paper over the drm_driver mangling for virt
>>
>> Prep work to make drm_device->driver const.
>>
>> Signed-off-by: Daniel Vetter <[e-mail]>
>> Cc: Alex Deucher <[e-mail]>
>> Cc: "Christian König" <[e-mail]>
>> Cc: Evan Quan <[e-mail]>
>> Cc: Felix Kuehling <[e-mail]>
>> Cc: Hawking Zhang <[e-mail]>
>> Cc: Andrey Grodzovsky <[e-mail]>
>> Cc: Luben Tuikov <[e-mail]>
>> Cc: Thomas Zimmermann <[e-mail]>
>> Cc: Monk Liu <[e-mail]>
>> Cc: Yintian Tao <[e-mail]>
>> Cc: Dennis Li <[e-mail]>
>> Cc: shaoyunl <[e-mail]>
>> Cc: Bokun Zhang <[e-mail]>
>> Cc: "Stanley.Yang" <[e-mail]>
>> Cc: Wenhui Sheng <[e-mail]>
>> Cc: chen gong <[e-mail]>
>> Signed-off-by: Daniel Vetter <[e-mail]>
>> ---
>>  drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c  |  8 ++++----  drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c | 12 +++++++++++-
>>  2 files changed, 15 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>> index 024c3b70b1aa..3d337f13ae4e 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>> @@ -1093,7 +1093,7 @@ static const struct pci_device_id pciidlist[] = {
>>
>>  MODULE_DEVICE_TABLE(pci, pciidlist);
>>
>> -static struct drm_driver kms_driver;
>> +struct drm_driver amdgpu_kms_driver;
>>
>>  static int amdgpu_pci_probe(struct pci_dev *pdev,
>>      const struct pci_device_id *ent) @@ -1164,7 +1164,7 @@ static int amdgpu_pci_probe(struct pci_dev *pdev,
>>  if (ret)
>>  return ret;
>>
>> -adev = devm_drm_dev_alloc(&pdev->dev, &kms_driver, typeof(*adev), ddev);
>> +adev = devm_drm_dev_alloc(&pdev->dev, &amdgpu_kms_driver,
>> +typeof(*adev), ddev);
>>  if (IS_ERR(adev))
>>  return PTR_ERR(adev);
>>
>> @@ -1508,7 +1508,7 @@ int amdgpu_file_to_fpriv(struct file *filp, struct amdgpu_fpriv **fpriv)
>>  return 0;
>>  }
>>
>> -static struct drm_driver kms_driver = {
>> +struct drm_driver amdgpu_kms_driver = {
>>  .driver_features =
>>      DRIVER_ATOMIC |
>>      DRIVER_GEM |
>> @@ -1571,7 +1571,7 @@ static int __init amdgpu_init(void)
>>  goto error_fence;
>>
>>  DRM_INFO("amdgpu kernel modesetting enabled.\n");
>> -kms_driver.num_ioctls = amdgpu_max_kms_ioctl;
>> +amdgpu_kms_driver.num_ioctls = amdgpu_max_kms_ioctl;
>>  amdgpu_register_atpx_handler();
>>
>>  /* Ignore KFD init failures. Normal when CONFIG_HSA_AMD is not set. */ diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
>> index d0aea5e39531..dde4c449c284 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
>> @@ -45,13 +45,23 @@ bool amdgpu_virt_mmio_blocked(struct amdgpu_device *adev)
>>  return RREG32_NO_KIQ(0xc040) == 0xffffffff;  }
>>
>> +extern struct drm_driver amdgpu_kms_driver;
>> +
>>  void amdgpu_virt_init_setting(struct amdgpu_device *adev)  {
>>  /* enable virtual display */
>>  if (adev->mode_info.num_crtc == 0)
>>  adev->mode_info.num_crtc = 1;
>>  adev->enable_virtual_display = true;
>> -adev_to_drm(adev)->driver->driver_features &= ~DRIVER_ATOMIC;
>> +
>> +/*
>> + * FIXME: Either make virt support atomic or make sure you have two
>> + * drm_driver structs, these kind of tricks are only ok when there's
>> + * guaranteed only a single device per system. This should also be done
>> + * before struct drm_device is initialized.
>> + */
>> +amdgpu_kms_driver.driver_features &= ~DRIVER_ATOMIC;
>> +
>>  adev->cg_flags = 0;
>>  adev->pg_flags = 0;
>>  }
>> --
>> 2.28.0
>>
> 
>

Daniel Vetter Oct. 31, 2020, 1:57 p.m. UTC | #5

On Fri, Oct 30, 2020 at 7:47 PM Alex Deucher <[e-mail]> wrote:
>
> On Fri, Oct 30, 2020 at 6:11 AM Daniel Vetter <[e-mail]> wrote:
> >
> > Prep work to make drm_device->driver const.
> >
> > Signed-off-by: Daniel Vetter <[e-mail]>
> > Cc: Alex Deucher <[e-mail]>
> > Cc: "Christian König" <[e-mail]>
> > Cc: Evan Quan <[e-mail]>
> > Cc: Felix Kuehling <[e-mail]>
> > Cc: Hawking Zhang <[e-mail]>
> > Cc: Andrey Grodzovsky <[e-mail]>
> > Cc: Luben Tuikov <[e-mail]>
> > Cc: Thomas Zimmermann <[e-mail]>
> > Cc: Monk Liu <[e-mail]>
> > Cc: Yintian Tao <[e-mail]>
> > Cc: Dennis Li <[e-mail]>
> > Cc: shaoyunl <[e-mail]>
> > Cc: Bokun Zhang <[e-mail]>
> > Cc: "Stanley.Yang" <[e-mail]>
> > Cc: Wenhui Sheng <[e-mail]>
> > Cc: chen gong <[e-mail]>
> > Signed-off-by: Daniel Vetter <[e-mail]>
> > ---
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c  |  8 ++++----
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c | 12 +++++++++++-
> >  2 files changed, 15 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> > index 024c3b70b1aa..3d337f13ae4e 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> > @@ -1093,7 +1093,7 @@ static const struct pci_device_id pciidlist[] = {
> >
> >  MODULE_DEVICE_TABLE(pci, pciidlist);
> >
> > -static struct drm_driver kms_driver;
> > +struct drm_driver amdgpu_kms_driver;
> >
> >  static int amdgpu_pci_probe(struct pci_dev *pdev,
> >                             const struct pci_device_id *ent)
> > @@ -1164,7 +1164,7 @@ static int amdgpu_pci_probe(struct pci_dev *pdev,
> >         if (ret)
> >                 return ret;
> >
> > -       adev = devm_drm_dev_alloc(&pdev->dev, &kms_driver, typeof(*adev), ddev);
> > +       adev = devm_drm_dev_alloc(&pdev->dev, &amdgpu_kms_driver, typeof(*adev), ddev);
> >         if (IS_ERR(adev))
> >                 return PTR_ERR(adev);
> >
> > @@ -1508,7 +1508,7 @@ int amdgpu_file_to_fpriv(struct file *filp, struct amdgpu_fpriv **fpriv)
> >         return 0;
> >  }
> >
> > -static struct drm_driver kms_driver = {
> > +struct drm_driver amdgpu_kms_driver = {
> >         .driver_features =
> >             DRIVER_ATOMIC |
> >             DRIVER_GEM |
> > @@ -1571,7 +1571,7 @@ static int __init amdgpu_init(void)
> >                 goto error_fence;
> >
> >         DRM_INFO("amdgpu kernel modesetting enabled.\n");
> > -       kms_driver.num_ioctls = amdgpu_max_kms_ioctl;
> > +       amdgpu_kms_driver.num_ioctls = amdgpu_max_kms_ioctl;
> >         amdgpu_register_atpx_handler();
> >
> >         /* Ignore KFD init failures. Normal when CONFIG_HSA_AMD is not set. */
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
> > index d0aea5e39531..dde4c449c284 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
> > @@ -45,13 +45,23 @@ bool amdgpu_virt_mmio_blocked(struct amdgpu_device *adev)
> >         return RREG32_NO_KIQ(0xc040) == 0xffffffff;
> >  }
> >
> > +extern struct drm_driver amdgpu_kms_driver;
> > +
> >  void amdgpu_virt_init_setting(struct amdgpu_device *adev)
> >  {
> >         /* enable virtual display */
> >         if (adev->mode_info.num_crtc == 0)
> >                 adev->mode_info.num_crtc = 1;
> >         adev->enable_virtual_display = true;
> > -       adev_to_drm(adev)->driver->driver_features &= ~DRIVER_ATOMIC;
> > +
> > +       /*
> > +        * FIXME: Either make virt support atomic or make sure you have two
> > +        * drm_driver structs, these kind of tricks are only ok when there's
> > +        * guaranteed only a single device per system. This should also be done
> > +        * before struct drm_device is initialized.
> > +        */
> > +       amdgpu_kms_driver.driver_features &= ~DRIVER_ATOMIC;
>
> There is additional DRIVER_ATOMIC in amdgpu_pci_probe() for older
> chips without atomic support.

That would need to be fixed for making the amdgpu drm_driver
structures constant, but that's not what I'm doing here. I'm only
removing the usage of the drm_device->driver pointer, to allow that to
become constant. Untangling the flow to make the amdgpu_kms_driver
const looked a bit more involved than just a  simple patch.
-Daniel

> Alex
>
> > +
> >         adev->cg_flags = 0;
> >         adev->pg_flags = 0;
> >  }
> > --
> > 2.28.0
> >
> > _______________________________________________
> > dri-devel mailing list
> > [e-mail]
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel

Daniel Vetter Nov. 1, 2020, 9:59 a.m. UTC | #6

On Sat, Oct 31, 2020 at 6:09 AM Luben Tuikov <[e-mail]> wrote:
>
> On 2020-10-30 08:04, Daniel Vetter wrote:
> > On Fri, Oct 30, 2020 at 11:41 AM Liu, Monk <[e-mail]> wrote:
> >>
> >> [AMD Official Use Only - Internal Distribution Only]
> >>
> >> What's the purpose of the patch sets
> >>
> >> e.g.: what bug can those 5 patches fix or what feature provided
> >>
> >> for this particular one (3/5) I didn't see how it helpful, could you give a background  ?
> >
> > It's good to make function tables const, so that they can be write
> > protected. More resilience against exploits and all that. This patch
> > here is needed to be able to make drm_device->driver const so that all
> > other drivers can make their drm_driver structure const. Would be good
> > to fully fix up amdgpu like in the comment, but I'm not going that in
> > this series here.
> > -Daniel
>
> Hi Daniel,
>
> I feel that that's a good change.
>
> But if you can clarify this for me... Is this leading
> towards a single instance of a struct drm_driver per
> low-level driver, i.e. amdgpu?
>
> And as such, being able to be defined as const?
>
> So that we have many GPU devices driven by one
> low-level driver (amdgpu_drv), represented by one
> const drm_driver (and thus const)?
>
> Which would imply that if varied devices can be handled
> by a single low-level driver, whose struct drm_driver
> settings cannot be shared among subset of devices (say
> very old and new), then the low-level driver
> would have to create more than one "const" struct drm_driver?

This is already the case, minus the const. Which is why it's
problemantic if you change that shared drm_driver instance at runtime
from a specific driver, since you always change it for all instances.
-Daniel

>
> Regards,
> Luben
>
> >
> >>
> >> thanks
> >> _____________________________________
> >> Monk Liu|GPU Virtualization Team |AMD
> >>
> >>
> >> -----Original Message-----
> >> From: Daniel Vetter <[e-mail]>
> >> Sent: Friday, October 30, 2020 6:11 PM
> >> To: DRI Development <[e-mail]>
> >> Cc: Intel Graphics Development <[e-mail]>; Daniel Vetter <[e-mail]>; Deucher, Alexander <[e-mail]>; Koenig, Christian <[e-mail]>; Quan, Evan <[e-mail]>; Kuehling, Felix <[e-mail]>; Zhang, Hawking <[e-mail]>; Grodzovsky, Andrey <[e-mail]>; Tuikov, Luben <[e-mail]>; Thomas Zimmermann <[e-mail]>; Liu, Monk <[e-mail]>; Yintian Tao <[e-mail]>; Li, Dennis <[e-mail]>; Liu, Shaoyun <[e-mail]>; Zhang, Bokun <[e-mail]>; Yang, Stanley <[e-mail]>; Sheng, Wenhui <[e-mail]>; Gong, Curry <[e-mail]>; Daniel Vetter <[e-mail]>
> >> Subject: [PATCH 3/5] drm/amdgpu: Paper over the drm_driver mangling for virt
> >>
> >> Prep work to make drm_device->driver const.
> >>
> >> Signed-off-by: Daniel Vetter <[e-mail]>
> >> Cc: Alex Deucher <[e-mail]>
> >> Cc: "Christian König" <[e-mail]>
> >> Cc: Evan Quan <[e-mail]>
> >> Cc: Felix Kuehling <[e-mail]>
> >> Cc: Hawking Zhang <[e-mail]>
> >> Cc: Andrey Grodzovsky <[e-mail]>
> >> Cc: Luben Tuikov <[e-mail]>
> >> Cc: Thomas Zimmermann <[e-mail]>
> >> Cc: Monk Liu <[e-mail]>
> >> Cc: Yintian Tao <[e-mail]>
> >> Cc: Dennis Li <[e-mail]>
> >> Cc: shaoyunl <[e-mail]>
> >> Cc: Bokun Zhang <[e-mail]>
> >> Cc: "Stanley.Yang" <[e-mail]>
> >> Cc: Wenhui Sheng <[e-mail]>
> >> Cc: chen gong <[e-mail]>
> >> Signed-off-by: Daniel Vetter <[e-mail]>
> >> ---
> >>  drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c  |  8 ++++----  drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c | 12 +++++++++++-
> >>  2 files changed, 15 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> >> index 024c3b70b1aa..3d337f13ae4e 100644
> >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> >> @@ -1093,7 +1093,7 @@ static const struct pci_device_id pciidlist[] = {
> >>
> >>  MODULE_DEVICE_TABLE(pci, pciidlist);
> >>
> >> -static struct drm_driver kms_driver;
> >> +struct drm_driver amdgpu_kms_driver;
> >>
> >>  static int amdgpu_pci_probe(struct pci_dev *pdev,
> >>      const struct pci_device_id *ent) @@ -1164,7 +1164,7 @@ static int amdgpu_pci_probe(struct pci_dev *pdev,
> >>  if (ret)
> >>  return ret;
> >>
> >> -adev = devm_drm_dev_alloc(&pdev->dev, &kms_driver, typeof(*adev), ddev);
> >> +adev = devm_drm_dev_alloc(&pdev->dev, &amdgpu_kms_driver,
> >> +typeof(*adev), ddev);
> >>  if (IS_ERR(adev))
> >>  return PTR_ERR(adev);
> >>
> >> @@ -1508,7 +1508,7 @@ int amdgpu_file_to_fpriv(struct file *filp, struct amdgpu_fpriv **fpriv)
> >>  return 0;
> >>  }
> >>
> >> -static struct drm_driver kms_driver = {
> >> +struct drm_driver amdgpu_kms_driver = {
> >>  .driver_features =
> >>      DRIVER_ATOMIC |
> >>      DRIVER_GEM |
> >> @@ -1571,7 +1571,7 @@ static int __init amdgpu_init(void)
> >>  goto error_fence;
> >>
> >>  DRM_INFO("amdgpu kernel modesetting enabled.\n");
> >> -kms_driver.num_ioctls = amdgpu_max_kms_ioctl;
> >> +amdgpu_kms_driver.num_ioctls = amdgpu_max_kms_ioctl;
> >>  amdgpu_register_atpx_handler();
> >>
> >>  /* Ignore KFD init failures. Normal when CONFIG_HSA_AMD is not set. */ diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
> >> index d0aea5e39531..dde4c449c284 100644
> >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
> >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
> >> @@ -45,13 +45,23 @@ bool amdgpu_virt_mmio_blocked(struct amdgpu_device *adev)
> >>  return RREG32_NO_KIQ(0xc040) == 0xffffffff;  }
> >>
> >> +extern struct drm_driver amdgpu_kms_driver;
> >> +
> >>  void amdgpu_virt_init_setting(struct amdgpu_device *adev)  {
> >>  /* enable virtual display */
> >>  if (adev->mode_info.num_crtc == 0)
> >>  adev->mode_info.num_crtc = 1;
> >>  adev->enable_virtual_display = true;
> >> -adev_to_drm(adev)->driver->driver_features &= ~DRIVER_ATOMIC;
> >> +
> >> +/*
> >> + * FIXME: Either make virt support atomic or make sure you have two
> >> + * drm_driver structs, these kind of tricks are only ok when there's
> >> + * guaranteed only a single device per system. This should also be done
> >> + * before struct drm_device is initialized.
> >> + */
> >> +amdgpu_kms_driver.driver_features &= ~DRIVER_ATOMIC;
> >> +
> >>  adev->cg_flags = 0;
> >>  adev->pg_flags = 0;
> >>  }
> >> --
> >> 2.28.0
> >>
> >
> >
>

Daniel Vetter Nov. 1, 2020, 10:01 a.m. UTC | #7

On Sat, Oct 31, 2020 at 2:57 PM Daniel Vetter <[e-mail]> wrote:
>
> On Fri, Oct 30, 2020 at 7:47 PM Alex Deucher <[e-mail]> wrote:
> >
> > On Fri, Oct 30, 2020 at 6:11 AM Daniel Vetter <[e-mail]> wrote:
> > >
> > > Prep work to make drm_device->driver const.
> > >
> > > Signed-off-by: Daniel Vetter <[e-mail]>
> > > Cc: Alex Deucher <[e-mail]>
> > > Cc: "Christian König" <[e-mail]>
> > > Cc: Evan Quan <[e-mail]>
> > > Cc: Felix Kuehling <[e-mail]>
> > > Cc: Hawking Zhang <[e-mail]>
> > > Cc: Andrey Grodzovsky <[e-mail]>
> > > Cc: Luben Tuikov <[e-mail]>
> > > Cc: Thomas Zimmermann <[e-mail]>
> > > Cc: Monk Liu <[e-mail]>
> > > Cc: Yintian Tao <[e-mail]>
> > > Cc: Dennis Li <[e-mail]>
> > > Cc: shaoyunl <[e-mail]>
> > > Cc: Bokun Zhang <[e-mail]>
> > > Cc: "Stanley.Yang" <[e-mail]>
> > > Cc: Wenhui Sheng <[e-mail]>
> > > Cc: chen gong <[e-mail]>
> > > Signed-off-by: Daniel Vetter <[e-mail]>
> > > ---
> > >  drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c  |  8 ++++----
> > >  drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c | 12 +++++++++++-
> > >  2 files changed, 15 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> > > index 024c3b70b1aa..3d337f13ae4e 100644
> > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> > > @@ -1093,7 +1093,7 @@ static const struct pci_device_id pciidlist[] = {
> > >
> > >  MODULE_DEVICE_TABLE(pci, pciidlist);
> > >
> > > -static struct drm_driver kms_driver;
> > > +struct drm_driver amdgpu_kms_driver;
> > >
> > >  static int amdgpu_pci_probe(struct pci_dev *pdev,
> > >                             const struct pci_device_id *ent)
> > > @@ -1164,7 +1164,7 @@ static int amdgpu_pci_probe(struct pci_dev *pdev,
> > >         if (ret)
> > >                 return ret;
> > >
> > > -       adev = devm_drm_dev_alloc(&pdev->dev, &kms_driver, typeof(*adev), ddev);
> > > +       adev = devm_drm_dev_alloc(&pdev->dev, &amdgpu_kms_driver, typeof(*adev), ddev);
> > >         if (IS_ERR(adev))
> > >                 return PTR_ERR(adev);
> > >
> > > @@ -1508,7 +1508,7 @@ int amdgpu_file_to_fpriv(struct file *filp, struct amdgpu_fpriv **fpriv)
> > >         return 0;
> > >  }
> > >
> > > -static struct drm_driver kms_driver = {
> > > +struct drm_driver amdgpu_kms_driver = {
> > >         .driver_features =
> > >             DRIVER_ATOMIC |
> > >             DRIVER_GEM |
> > > @@ -1571,7 +1571,7 @@ static int __init amdgpu_init(void)
> > >                 goto error_fence;
> > >
> > >         DRM_INFO("amdgpu kernel modesetting enabled.\n");
> > > -       kms_driver.num_ioctls = amdgpu_max_kms_ioctl;
> > > +       amdgpu_kms_driver.num_ioctls = amdgpu_max_kms_ioctl;
> > >         amdgpu_register_atpx_handler();
> > >
> > >         /* Ignore KFD init failures. Normal when CONFIG_HSA_AMD is not set. */
> > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
> > > index d0aea5e39531..dde4c449c284 100644
> > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
> > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
> > > @@ -45,13 +45,23 @@ bool amdgpu_virt_mmio_blocked(struct amdgpu_device *adev)
> > >         return RREG32_NO_KIQ(0xc040) == 0xffffffff;
> > >  }
> > >
> > > +extern struct drm_driver amdgpu_kms_driver;
> > > +
> > >  void amdgpu_virt_init_setting(struct amdgpu_device *adev)
> > >  {
> > >         /* enable virtual display */
> > >         if (adev->mode_info.num_crtc == 0)
> > >                 adev->mode_info.num_crtc = 1;
> > >         adev->enable_virtual_display = true;
> > > -       adev_to_drm(adev)->driver->driver_features &= ~DRIVER_ATOMIC;
> > > +
> > > +       /*
> > > +        * FIXME: Either make virt support atomic or make sure you have two
> > > +        * drm_driver structs, these kind of tricks are only ok when there's
> > > +        * guaranteed only a single device per system. This should also be done
> > > +        * before struct drm_device is initialized.
> > > +        */
> > > +       amdgpu_kms_driver.driver_features &= ~DRIVER_ATOMIC;
> >
> > There is additional DRIVER_ATOMIC in amdgpu_pci_probe() for older
> > chips without atomic support.
>
> That would need to be fixed for making the amdgpu drm_driver
> structures constant, but that's not what I'm doing here. I'm only
> removing the usage of the drm_device->driver pointer, to allow that to
> become constant. Untangling the flow to make the amdgpu_kms_driver
> const looked a bit more involved than just a  simple patch.

On second look, this changes the drm_device->driver_features flag,
which was added to avoid having to change the drm_driver one. So
that's actually all ok (and just the virt code here is broken). But
amdgpu also updates num_ioctl and other stuff, and that's a fairly
invasive patch.

I'm also not sure whether this code here can just be switched over
from drm_driver->driver_features to drm_device->driver_features. So
given all this, ok as-is and you guys figure out how to patch this
properly, or want me to change something in this patch?

Cheers, Daniel

>
> > Alex
> >
> > > +
> > >         adev->cg_flags = 0;
> > >         adev->pg_flags = 0;
> > >  }
> > > --
> > > 2.28.0
> > >
> > > _______________________________________________
> > > dri-devel mailing list
> > > [e-mail]
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

Alex Deucher Nov. 3, 2020, 4:49 p.m. UTC | #8

On Sun, Nov 1, 2020 at 5:01 AM Daniel Vetter <[e-mail]> wrote:
>
> On Sat, Oct 31, 2020 at 2:57 PM Daniel Vetter <[e-mail]> wrote:
> >
> > On Fri, Oct 30, 2020 at 7:47 PM Alex Deucher <[e-mail]> wrote:
> > >
> > > On Fri, Oct 30, 2020 at 6:11 AM Daniel Vetter <[e-mail]> wrote:
> > > >
> > > > Prep work to make drm_device->driver const.
> > > >
> > > > Signed-off-by: Daniel Vetter <[e-mail]>
> > > > Cc: Alex Deucher <[e-mail]>
> > > > Cc: "Christian König" <[e-mail]>
> > > > Cc: Evan Quan <[e-mail]>
> > > > Cc: Felix Kuehling <[e-mail]>
> > > > Cc: Hawking Zhang <[e-mail]>
> > > > Cc: Andrey Grodzovsky <[e-mail]>
> > > > Cc: Luben Tuikov <[e-mail]>
> > > > Cc: Thomas Zimmermann <[e-mail]>
> > > > Cc: Monk Liu <[e-mail]>
> > > > Cc: Yintian Tao <[e-mail]>
> > > > Cc: Dennis Li <[e-mail]>
> > > > Cc: shaoyunl <[e-mail]>
> > > > Cc: Bokun Zhang <[e-mail]>
> > > > Cc: "Stanley.Yang" <[e-mail]>
> > > > Cc: Wenhui Sheng <[e-mail]>
> > > > Cc: chen gong <[e-mail]>
> > > > Signed-off-by: Daniel Vetter <[e-mail]>
> > > > ---
> > > >  drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c  |  8 ++++----
> > > >  drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c | 12 +++++++++++-
> > > >  2 files changed, 15 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> > > > index 024c3b70b1aa..3d337f13ae4e 100644
> > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> > > > @@ -1093,7 +1093,7 @@ static const struct pci_device_id pciidlist[] = {
> > > >
> > > >  MODULE_DEVICE_TABLE(pci, pciidlist);
> > > >
> > > > -static struct drm_driver kms_driver;
> > > > +struct drm_driver amdgpu_kms_driver;
> > > >
> > > >  static int amdgpu_pci_probe(struct pci_dev *pdev,
> > > >                             const struct pci_device_id *ent)
> > > > @@ -1164,7 +1164,7 @@ static int amdgpu_pci_probe(struct pci_dev *pdev,
> > > >         if (ret)
> > > >                 return ret;
> > > >
> > > > -       adev = devm_drm_dev_alloc(&pdev->dev, &kms_driver, typeof(*adev), ddev);
> > > > +       adev = devm_drm_dev_alloc(&pdev->dev, &amdgpu_kms_driver, typeof(*adev), ddev);
> > > >         if (IS_ERR(adev))
> > > >                 return PTR_ERR(adev);
> > > >
> > > > @@ -1508,7 +1508,7 @@ int amdgpu_file_to_fpriv(struct file *filp, struct amdgpu_fpriv **fpriv)
> > > >         return 0;
> > > >  }
> > > >
> > > > -static struct drm_driver kms_driver = {
> > > > +struct drm_driver amdgpu_kms_driver = {
> > > >         .driver_features =
> > > >             DRIVER_ATOMIC |
> > > >             DRIVER_GEM |
> > > > @@ -1571,7 +1571,7 @@ static int __init amdgpu_init(void)
> > > >                 goto error_fence;
> > > >
> > > >         DRM_INFO("amdgpu kernel modesetting enabled.\n");
> > > > -       kms_driver.num_ioctls = amdgpu_max_kms_ioctl;
> > > > +       amdgpu_kms_driver.num_ioctls = amdgpu_max_kms_ioctl;
> > > >         amdgpu_register_atpx_handler();
> > > >
> > > >         /* Ignore KFD init failures. Normal when CONFIG_HSA_AMD is not set. */
> > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
> > > > index d0aea5e39531..dde4c449c284 100644
> > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
> > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
> > > > @@ -45,13 +45,23 @@ bool amdgpu_virt_mmio_blocked(struct amdgpu_device *adev)
> > > >         return RREG32_NO_KIQ(0xc040) == 0xffffffff;
> > > >  }
> > > >
> > > > +extern struct drm_driver amdgpu_kms_driver;
> > > > +
> > > >  void amdgpu_virt_init_setting(struct amdgpu_device *adev)
> > > >  {
> > > >         /* enable virtual display */
> > > >         if (adev->mode_info.num_crtc == 0)
> > > >                 adev->mode_info.num_crtc = 1;
> > > >         adev->enable_virtual_display = true;
> > > > -       adev_to_drm(adev)->driver->driver_features &= ~DRIVER_ATOMIC;
> > > > +
> > > > +       /*
> > > > +        * FIXME: Either make virt support atomic or make sure you have two
> > > > +        * drm_driver structs, these kind of tricks are only ok when there's
> > > > +        * guaranteed only a single device per system. This should also be done
> > > > +        * before struct drm_device is initialized.
> > > > +        */
> > > > +       amdgpu_kms_driver.driver_features &= ~DRIVER_ATOMIC;
> > >
> > > There is additional DRIVER_ATOMIC in amdgpu_pci_probe() for older
> > > chips without atomic support.
> >
> > That would need to be fixed for making the amdgpu drm_driver
> > structures constant, but that's not what I'm doing here. I'm only
> > removing the usage of the drm_device->driver pointer, to allow that to
> > become constant. Untangling the flow to make the amdgpu_kms_driver
> > const looked a bit more involved than just a  simple patch.
>
> On second look, this changes the drm_device->driver_features flag,
> which was added to avoid having to change the drm_driver one. So
> that's actually all ok (and just the virt code here is broken). But
> amdgpu also updates num_ioctl and other stuff, and that's a fairly
> invasive patch.

We don't change the number of ioctls:
const int amdgpu_max_kms_ioctl = ARRAY_SIZE(amdgpu_ioctls_kms);
So I think the only thing here is the driver features flag for the
virt display code, or am I missing something?

Alex


>
> I'm also not sure whether this code here can just be switched over
> from drm_driver->driver_features to drm_device->driver_features. So
> given all this, ok as-is and you guys figure out how to patch this
> properly, or want me to change something in this patch?
>
> Cheers, Daniel
>
> >
> > > Alex
> > >
> > > > +
> > > >         adev->cg_flags = 0;
> > > >         adev->pg_flags = 0;
> > > >  }
> > > > --
> > > > 2.28.0
> > > >
> > > > _______________________________________________
> > > > dri-devel mailing list
> > > > [e-mail]
> > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
> >
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
>
>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

Daniel Vetter Nov. 4, 2020, 9:31 a.m. UTC | #9

On Tue, Nov 03, 2020 at 11:49:40AM -0500, Alex Deucher wrote:
> On Sun, Nov 1, 2020 at 5:01 AM Daniel Vetter <[e-mail]> wrote:
> >
> > On Sat, Oct 31, 2020 at 2:57 PM Daniel Vetter <[e-mail]> wrote:
> > >
> > > On Fri, Oct 30, 2020 at 7:47 PM Alex Deucher <[e-mail]> wrote:
> > > >
> > > > On Fri, Oct 30, 2020 at 6:11 AM Daniel Vetter <[e-mail]> wrote:
> > > > >
> > > > > Prep work to make drm_device->driver const.
> > > > >
> > > > > Signed-off-by: Daniel Vetter <[e-mail]>
> > > > > Cc: Alex Deucher <[e-mail]>
> > > > > Cc: "Christian König" <[e-mail]>
> > > > > Cc: Evan Quan <[e-mail]>
> > > > > Cc: Felix Kuehling <[e-mail]>
> > > > > Cc: Hawking Zhang <[e-mail]>
> > > > > Cc: Andrey Grodzovsky <[e-mail]>
> > > > > Cc: Luben Tuikov <[e-mail]>
> > > > > Cc: Thomas Zimmermann <[e-mail]>
> > > > > Cc: Monk Liu <[e-mail]>
> > > > > Cc: Yintian Tao <[e-mail]>
> > > > > Cc: Dennis Li <[e-mail]>
> > > > > Cc: shaoyunl <[e-mail]>
> > > > > Cc: Bokun Zhang <[e-mail]>
> > > > > Cc: "Stanley.Yang" <[e-mail]>
> > > > > Cc: Wenhui Sheng <[e-mail]>
> > > > > Cc: chen gong <[e-mail]>
> > > > > Signed-off-by: Daniel Vetter <[e-mail]>
> > > > > ---
> > > > >  drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c  |  8 ++++----
> > > > >  drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c | 12 +++++++++++-
> > > > >  2 files changed, 15 insertions(+), 5 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> > > > > index 024c3b70b1aa..3d337f13ae4e 100644
> > > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> > > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> > > > > @@ -1093,7 +1093,7 @@ static const struct pci_device_id pciidlist[] = {
> > > > >
> > > > >  MODULE_DEVICE_TABLE(pci, pciidlist);
> > > > >
> > > > > -static struct drm_driver kms_driver;
> > > > > +struct drm_driver amdgpu_kms_driver;
> > > > >
> > > > >  static int amdgpu_pci_probe(struct pci_dev *pdev,
> > > > >                             const struct pci_device_id *ent)
> > > > > @@ -1164,7 +1164,7 @@ static int amdgpu_pci_probe(struct pci_dev *pdev,
> > > > >         if (ret)
> > > > >                 return ret;
> > > > >
> > > > > -       adev = devm_drm_dev_alloc(&pdev->dev, &kms_driver, typeof(*adev), ddev);
> > > > > +       adev = devm_drm_dev_alloc(&pdev->dev, &amdgpu_kms_driver, typeof(*adev), ddev);
> > > > >         if (IS_ERR(adev))
> > > > >                 return PTR_ERR(adev);
> > > > >
> > > > > @@ -1508,7 +1508,7 @@ int amdgpu_file_to_fpriv(struct file *filp, struct amdgpu_fpriv **fpriv)
> > > > >         return 0;
> > > > >  }
> > > > >
> > > > > -static struct drm_driver kms_driver = {
> > > > > +struct drm_driver amdgpu_kms_driver = {
> > > > >         .driver_features =
> > > > >             DRIVER_ATOMIC |
> > > > >             DRIVER_GEM |
> > > > > @@ -1571,7 +1571,7 @@ static int __init amdgpu_init(void)
> > > > >                 goto error_fence;
> > > > >
> > > > >         DRM_INFO("amdgpu kernel modesetting enabled.\n");
> > > > > -       kms_driver.num_ioctls = amdgpu_max_kms_ioctl;
> > > > > +       amdgpu_kms_driver.num_ioctls = amdgpu_max_kms_ioctl;
> > > > >         amdgpu_register_atpx_handler();
> > > > >
> > > > >         /* Ignore KFD init failures. Normal when CONFIG_HSA_AMD is not set. */
> > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
> > > > > index d0aea5e39531..dde4c449c284 100644
> > > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
> > > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
> > > > > @@ -45,13 +45,23 @@ bool amdgpu_virt_mmio_blocked(struct amdgpu_device *adev)
> > > > >         return RREG32_NO_KIQ(0xc040) == 0xffffffff;
> > > > >  }
> > > > >
> > > > > +extern struct drm_driver amdgpu_kms_driver;
> > > > > +
> > > > >  void amdgpu_virt_init_setting(struct amdgpu_device *adev)
> > > > >  {
> > > > >         /* enable virtual display */
> > > > >         if (adev->mode_info.num_crtc == 0)
> > > > >                 adev->mode_info.num_crtc = 1;
> > > > >         adev->enable_virtual_display = true;
> > > > > -       adev_to_drm(adev)->driver->driver_features &= ~DRIVER_ATOMIC;
> > > > > +
> > > > > +       /*
> > > > > +        * FIXME: Either make virt support atomic or make sure you have two
> > > > > +        * drm_driver structs, these kind of tricks are only ok when there's
> > > > > +        * guaranteed only a single device per system. This should also be done
> > > > > +        * before struct drm_device is initialized.
> > > > > +        */
> > > > > +       amdgpu_kms_driver.driver_features &= ~DRIVER_ATOMIC;
> > > >
> > > > There is additional DRIVER_ATOMIC in amdgpu_pci_probe() for older
> > > > chips without atomic support.
> > >
> > > That would need to be fixed for making the amdgpu drm_driver
> > > structures constant, but that's not what I'm doing here. I'm only
> > > removing the usage of the drm_device->driver pointer, to allow that to
> > > become constant. Untangling the flow to make the amdgpu_kms_driver
> > > const looked a bit more involved than just a  simple patch.
> >
> > On second look, this changes the drm_device->driver_features flag,
> > which was added to avoid having to change the drm_driver one. So
> > that's actually all ok (and just the virt code here is broken). But
> > amdgpu also updates num_ioctl and other stuff, and that's a fairly
> > invasive patch.
> 
> We don't change the number of ioctls:
> const int amdgpu_max_kms_ioctl = ARRAY_SIZE(amdgpu_ioctls_kms);
> So I think the only thing here is the driver features flag for the
> virt display code, or am I missing something?

You set the num_ioctl at runtime, not compile time. That's enough to
prevent constification. Moving that around to make it compile time means a
_lot_ of code shuffling, much more than I felt was a good idea for me to
do :-)

And for the virt case you could use drm_device->driver_flags instead to
avoid this.

Note I don't really care for this series whether you're changing your
drm_driver at runtime or not, I just want to make it possible for drivers
to make it const, which means drm_device->driver must be a const pointer.
Whether you want to make the amdgpu drm_driver const or not is up to you.
-Daniel

> 
> Alex
> 
> 
> >
> > I'm also not sure whether this code here can just be switched over
> > from drm_driver->driver_features to drm_device->driver_features. So
> > given all this, ok as-is and you guys figure out how to patch this
> > properly, or want me to change something in this patch?
> >
> > Cheers, Daniel
> >
> > >
> > > > Alex
> > > >
> > > > > +
> > > > >         adev->cg_flags = 0;
> > > > >         adev->pg_flags = 0;
> > > > >  }
> > > > > --
> > > > > 2.28.0
> > > > >
> > > > > _______________________________________________
> > > > > dri-devel mailing list
> > > > > [e-mail]
> > > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > >
> > >
> > >
> > > --
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > http://blog.ffwll.ch
> >
> >
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch

Patch

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index 024c3b70b1aa..3d337f13ae4e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -1093,7 +1093,7 @@  static const struct pci_device_id pciidlist[] = {
 
 MODULE_DEVICE_TABLE(pci, pciidlist);
 
-static struct drm_driver kms_driver;
+struct drm_driver amdgpu_kms_driver;
 
 static int amdgpu_pci_probe(struct pci_dev *pdev,
 			    const struct pci_device_id *ent)
@@ -1164,7 +1164,7 @@  static int amdgpu_pci_probe(struct pci_dev *pdev,
 	if (ret)
 		return ret;
 
-	adev = devm_drm_dev_alloc(&pdev->dev, &kms_driver, typeof(*adev), ddev);
+	adev = devm_drm_dev_alloc(&pdev->dev, &amdgpu_kms_driver, typeof(*adev), ddev);
 	if (IS_ERR(adev))
 		return PTR_ERR(adev);
 
@@ -1508,7 +1508,7 @@  int amdgpu_file_to_fpriv(struct file *filp, struct amdgpu_fpriv **fpriv)
 	return 0;
 }
 
-static struct drm_driver kms_driver = {
+struct drm_driver amdgpu_kms_driver = {
 	.driver_features =
 	    DRIVER_ATOMIC |
 	    DRIVER_GEM |
@@ -1571,7 +1571,7 @@  static int __init amdgpu_init(void)
 		goto error_fence;
 
 	DRM_INFO("amdgpu kernel modesetting enabled.\n");
-	kms_driver.num_ioctls = amdgpu_max_kms_ioctl;
+	amdgpu_kms_driver.num_ioctls = amdgpu_max_kms_ioctl;
 	amdgpu_register_atpx_handler();
 
 	/* Ignore KFD init failures. Normal when CONFIG_HSA_AMD is not set. */
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
index d0aea5e39531..dde4c449c284 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
@@ -45,13 +45,23 @@  bool amdgpu_virt_mmio_blocked(struct amdgpu_device *adev)
 	return RREG32_NO_KIQ(0xc040) == 0xffffffff;
 }
 
+extern struct drm_driver amdgpu_kms_driver;
+
 void amdgpu_virt_init_setting(struct amdgpu_device *adev)
 {
 	/* enable virtual display */
 	if (adev->mode_info.num_crtc == 0)
 		adev->mode_info.num_crtc = 1;
 	adev->enable_virtual_display = true;
-	adev_to_drm(adev)->driver->driver_features &= ~DRIVER_ATOMIC;
+
+	/*
+	 * FIXME: Either make virt support atomic or make sure you have two
+	 * drm_driver structs, these kind of tricks are only ok when there's
+	 * guaranteed only a single device per system. This should also be done
+	 * before struct drm_device is initialized.
+	 */
+	amdgpu_kms_driver.driver_features &= ~DRIVER_ATOMIC;
+
 	adev->cg_flags = 0;
 	adev->pg_flags = 0;
 }