Patchwork [PATCHv3,2/2] radeon: use max_bus_speed to activate gen2 speeds

login
register
mail settings
Submitter Lucas Kannebley Tavares
Date April 11, 2013, 1:13 p.m.
Message ID <1365685994-32603-3-git-send-email-lucaskt@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/235737/
State Changes Requested, archived
Delegated to: Michael Ellerman
Headers show

Comments

Lucas Kannebley Tavares - April 11, 2013, 1:13 p.m.
radeon currently uses a drm function to get the speed capabilities for
the bus. However, this is a non-standard method of performing this
detection and this patch changes it to use the max_bus_speed attribute.
---
 drivers/gpu/drm/radeon/evergreen.c |    9 ++-------
 drivers/gpu/drm/radeon/r600.c      |    8 +-------
 drivers/gpu/drm/radeon/rv770.c     |    8 +-------
 3 files changed, 4 insertions(+), 21 deletions(-)
Bjorn Helgaas - April 12, 2013, 4:38 p.m.
On Thu, Apr 11, 2013 at 7:13 AM, Lucas Kannebley Tavares
<lucaskt@linux.vnet.ibm.com> wrote:
> radeon currently uses a drm function to get the speed capabilities for
> the bus. However, this is a non-standard method of performing this
> detection and this patch changes it to use the max_bus_speed attribute.
> ---
>  drivers/gpu/drm/radeon/evergreen.c |    9 ++-------
>  drivers/gpu/drm/radeon/r600.c      |    8 +-------
>  drivers/gpu/drm/radeon/rv770.c     |    8 +-------
>  3 files changed, 4 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/radeon/evergreen.c b/drivers/gpu/drm/radeon/evergreen.c
> index 305a657..3291f62 100644
> --- a/drivers/gpu/drm/radeon/evergreen.c
> +++ b/drivers/gpu/drm/radeon/evergreen.c
> @@ -3855,8 +3855,7 @@ void evergreen_fini(struct radeon_device *rdev)
>
>  void evergreen_pcie_gen2_enable(struct radeon_device *rdev)
>  {
> -       u32 link_width_cntl, speed_cntl, mask;
> -       int ret;
> +       u32 link_width_cntl, speed_cntl;
>
>         if (radeon_pcie_gen2 == 0)
>                 return;
> @@ -3871,11 +3870,7 @@ void evergreen_pcie_gen2_enable(struct radeon_device *rdev)
>         if (ASIC_IS_X2(rdev))
>                 return;
>
> -       ret = drm_pcie_get_speed_cap_mask(rdev->ddev, &mask);
> -       if (ret != 0)
> -               return;
> -
> -       if (!(mask & DRM_PCIE_SPEED_50))
> +       if (rdev->pdev->bus->max_bus_speed < PCIE_SPEED_5_0GT)

For devices on a root bus, we previously dereferenced a NULL pointer
in drm_pcie_get_speed_cap_mask() because pdev->bus->self is NULL on a
root bus.  (I think this is the original problem you tripped over,
Lucas.)

These patches fix that problem.  On pseries, where the device *is* on
a root bus, your patches set max_bus_speed so this will work as
expected.  On most other systems, max_bus_speed for root buses will be
PCI_SPEED_UNKNOWN (set in pci_alloc_bus() and never updated because
most arches don't have code like the pseries code you're adding).

PCI_SPEED_UNKNOWN = 0xff, so if we see another machine with a GPU on
the root bus, we'll attempt to enable Gen2 on the device even though
we have no idea what the bus will support.

That's why I originally suggested skipping the Gen2 stuff if
"max_bus_speed == PCI_SPEED_UNKNOWN".  I was just being conservative,
thinking that it's better to have a functional but slow GPU rather
than the unknown (to me) effects of enabling Gen2 on a link that might
not support it.  But I'm fine with this being either way.

It would be nice if we could get rid of drm_pcie_get_speed_cap_mask()
altogether.  It is exported, but I have no idea of anybody else uses
it.  Maybe it could at least be marked __deprecated now?

I don't know who should take these patches.  They don't touch
drivers/pci, but I'd be happy to push them, given the appropriate ACKs
from DRM and powerpc folks.

Bjorn

>                 return;
>
>         speed_cntl = RREG32_PCIE_P(PCIE_LC_SPEED_CNTL);
> diff --git a/drivers/gpu/drm/radeon/r600.c b/drivers/gpu/drm/radeon/r600.c
> index 0740db3..64b90c0 100644
> --- a/drivers/gpu/drm/radeon/r600.c
> +++ b/drivers/gpu/drm/radeon/r600.c
> @@ -4351,8 +4351,6 @@ static void r600_pcie_gen2_enable(struct radeon_device *rdev)
>  {
>         u32 link_width_cntl, lanes, speed_cntl, training_cntl, tmp;
>         u16 link_cntl2;
> -       u32 mask;
> -       int ret;
>
>         if (radeon_pcie_gen2 == 0)
>                 return;
> @@ -4371,11 +4369,7 @@ static void r600_pcie_gen2_enable(struct radeon_device *rdev)
>         if (rdev->family <= CHIP_R600)
>                 return;
>
> -       ret = drm_pcie_get_speed_cap_mask(rdev->ddev, &mask);
> -       if (ret != 0)
> -               return;
> -
> -       if (!(mask & DRM_PCIE_SPEED_50))
> +       if (rdev->pdev->bus->max_bus_speed < PCIE_SPEED_5_0GT)
>                 return;
>
>         speed_cntl = RREG32_PCIE_P(PCIE_LC_SPEED_CNTL);
> diff --git a/drivers/gpu/drm/radeon/rv770.c b/drivers/gpu/drm/radeon/rv770.c
> index d63fe1d..c683c36 100644
> --- a/drivers/gpu/drm/radeon/rv770.c
> +++ b/drivers/gpu/drm/radeon/rv770.c
> @@ -1238,8 +1238,6 @@ static void rv770_pcie_gen2_enable(struct radeon_device *rdev)
>  {
>         u32 link_width_cntl, lanes, speed_cntl, tmp;
>         u16 link_cntl2;
> -       u32 mask;
> -       int ret;
>
>         if (radeon_pcie_gen2 == 0)
>                 return;
> @@ -1254,11 +1252,7 @@ static void rv770_pcie_gen2_enable(struct radeon_device *rdev)
>         if (ASIC_IS_X2(rdev))
>                 return;
>
> -       ret = drm_pcie_get_speed_cap_mask(rdev->ddev, &mask);
> -       if (ret != 0)
> -               return;
> -
> -       if (!(mask & DRM_PCIE_SPEED_50))
> +       if (rdev->pdev->bus->max_bus_speed < PCIE_SPEED_5_0GT)
>                 return;
>
>         DRM_INFO("enabling PCIE gen 2 link speeds, disable with radeon.pcie_gen2=0\n");
> --
> 1.7.4.4
>
Dave Airlie - April 16, 2013, 3:17 a.m.
>>
>> diff --git a/drivers/gpu/drm/radeon/evergreen.c b/drivers/gpu/drm/radeon/evergreen.c
>> index 305a657..3291f62 100644
>> --- a/drivers/gpu/drm/radeon/evergreen.c
>> +++ b/drivers/gpu/drm/radeon/evergreen.c
>> @@ -3855,8 +3855,7 @@ void evergreen_fini(struct radeon_device *rdev)
>>
>>  void evergreen_pcie_gen2_enable(struct radeon_device *rdev)
>>  {
>> -       u32 link_width_cntl, speed_cntl, mask;
>> -       int ret;
>> +       u32 link_width_cntl, speed_cntl;
>>
>>         if (radeon_pcie_gen2 == 0)
>>                 return;
>> @@ -3871,11 +3870,7 @@ void evergreen_pcie_gen2_enable(struct radeon_device *rdev)
>>         if (ASIC_IS_X2(rdev))
>>                 return;
>>
>> -       ret = drm_pcie_get_speed_cap_mask(rdev->ddev, &mask);
>> -       if (ret != 0)
>> -               return;
>> -
>> -       if (!(mask & DRM_PCIE_SPEED_50))
>> +       if (rdev->pdev->bus->max_bus_speed < PCIE_SPEED_5_0GT)
>
> For devices on a root bus, we previously dereferenced a NULL pointer
> in drm_pcie_get_speed_cap_mask() because pdev->bus->self is NULL on a
> root bus.  (I think this is the original problem you tripped over,
> Lucas.)
>
> These patches fix that problem.  On pseries, where the device *is* on
> a root bus, your patches set max_bus_speed so this will work as
> expected.  On most other systems, max_bus_speed for root buses will be
> PCI_SPEED_UNKNOWN (set in pci_alloc_bus() and never updated because
> most arches don't have code like the pseries code you're adding).
>
> PCI_SPEED_UNKNOWN = 0xff, so if we see another machine with a GPU on
> the root bus, we'll attempt to enable Gen2 on the device even though
> we have no idea what the bus will support.
>
> That's why I originally suggested skipping the Gen2 stuff if
> "max_bus_speed == PCI_SPEED_UNKNOWN".  I was just being conservative,
> thinking that it's better to have a functional but slow GPU rather
> than the unknown (to me) effects of enabling Gen2 on a link that might
> not support it.  But I'm fine with this being either way.
>
> It would be nice if we could get rid of drm_pcie_get_speed_cap_mask()
> altogether.  It is exported, but I have no idea of anybody else uses
> it.  Maybe it could at least be marked __deprecated now?
>
> I don't know who should take these patches.  They don't touch
> drivers/pci, but I'd be happy to push them, given the appropriate ACKs
> from DRM and powerpc folks.
>

Acked-by: Dave Airlie <airlied@redhat.com>

I'm happy to see these go via pci tree to avoid interdependent trees.

Dave.
Lucas Kannebley Tavares - April 17, 2013, 12:38 p.m.
On 04/12/2013 01:38 PM, Bjorn Helgaas wrote:
> On Thu, Apr 11, 2013 at 7:13 AM, Lucas Kannebley Tavares
> <lucaskt@linux.vnet.ibm.com>  wrote:
>> radeon currently uses a drm function to get the speed capabilities for
>> the bus. However, this is a non-standard method of performing this
>> detection and this patch changes it to use the max_bus_speed attribute.
>> ---
>>   drivers/gpu/drm/radeon/evergreen.c |    9 ++-------
>>   drivers/gpu/drm/radeon/r600.c      |    8 +-------
>>   drivers/gpu/drm/radeon/rv770.c     |    8 +-------
>>   3 files changed, 4 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/radeon/evergreen.c b/drivers/gpu/drm/radeon/evergreen.c
>> index 305a657..3291f62 100644
>> --- a/drivers/gpu/drm/radeon/evergreen.c
>> +++ b/drivers/gpu/drm/radeon/evergreen.c
>> @@ -3855,8 +3855,7 @@ void evergreen_fini(struct radeon_device *rdev)
>>
>>   void evergreen_pcie_gen2_enable(struct radeon_device *rdev)
>>   {
>> -       u32 link_width_cntl, speed_cntl, mask;
>> -       int ret;
>> +       u32 link_width_cntl, speed_cntl;
>>
>>          if (radeon_pcie_gen2 == 0)
>>                  return;
>> @@ -3871,11 +3870,7 @@ void evergreen_pcie_gen2_enable(struct radeon_device *rdev)
>>          if (ASIC_IS_X2(rdev))
>>                  return;
>>
>> -       ret = drm_pcie_get_speed_cap_mask(rdev->ddev,&mask);
>> -       if (ret != 0)
>> -               return;
>> -
>> -       if (!(mask&  DRM_PCIE_SPEED_50))
>> +       if (rdev->pdev->bus->max_bus_speed<  PCIE_SPEED_5_0GT)
>
> For devices on a root bus, we previously dereferenced a NULL pointer
> in drm_pcie_get_speed_cap_mask() because pdev->bus->self is NULL on a
> root bus.  (I think this is the original problem you tripped over,
> Lucas.)
>
> These patches fix that problem.  On pseries, where the device *is* on
> a root bus, your patches set max_bus_speed so this will work as
> expected.  On most other systems, max_bus_speed for root buses will be
> PCI_SPEED_UNKNOWN (set in pci_alloc_bus() and never updated because
> most arches don't have code like the pseries code you're adding).
>
> PCI_SPEED_UNKNOWN = 0xff, so if we see another machine with a GPU on
> the root bus, we'll attempt to enable Gen2 on the device even though
> we have no idea what the bus will support.
>
> That's why I originally suggested skipping the Gen2 stuff if
> "max_bus_speed == PCI_SPEED_UNKNOWN".  I was just being conservative,
> thinking that it's better to have a functional but slow GPU rather
> than the unknown (to me) effects of enabling Gen2 on a link that might
> not support it.  But I'm fine with this being either way.

You're right here, of course. I'll test for it being different from 
5_0GT and 8_0GT instead. Though at some point I suppose someone will 
want to tackle Gen3 speeds.

>
> It would be nice if we could get rid of drm_pcie_get_speed_cap_mask()
> altogether.  It is exported, but I have no idea of anybody else uses
> it.  Maybe it could at least be marked __deprecated now?
>

I'll mark it.

> I don't know who should take these patches.  They don't touch
> drivers/pci, but I'd be happy to push them, given the appropriate ACKs
> from DRM and powerpc folks.
>
> Bjorn
>
>>                  return;
>>
>>          speed_cntl = RREG32_PCIE_P(PCIE_LC_SPEED_CNTL);
>> diff --git a/drivers/gpu/drm/radeon/r600.c b/drivers/gpu/drm/radeon/r600.c
>> index 0740db3..64b90c0 100644
>> --- a/drivers/gpu/drm/radeon/r600.c
>> +++ b/drivers/gpu/drm/radeon/r600.c
>> @@ -4351,8 +4351,6 @@ static void r600_pcie_gen2_enable(struct radeon_device *rdev)
>>   {
>>          u32 link_width_cntl, lanes, speed_cntl, training_cntl, tmp;
>>          u16 link_cntl2;
>> -       u32 mask;
>> -       int ret;
>>
>>          if (radeon_pcie_gen2 == 0)
>>                  return;
>> @@ -4371,11 +4369,7 @@ static void r600_pcie_gen2_enable(struct radeon_device *rdev)
>>          if (rdev->family<= CHIP_R600)
>>                  return;
>>
>> -       ret = drm_pcie_get_speed_cap_mask(rdev->ddev,&mask);
>> -       if (ret != 0)
>> -               return;
>> -
>> -       if (!(mask&  DRM_PCIE_SPEED_50))
>> +       if (rdev->pdev->bus->max_bus_speed<  PCIE_SPEED_5_0GT)
>>                  return;
>>
>>          speed_cntl = RREG32_PCIE_P(PCIE_LC_SPEED_CNTL);
>> diff --git a/drivers/gpu/drm/radeon/rv770.c b/drivers/gpu/drm/radeon/rv770.c
>> index d63fe1d..c683c36 100644
>> --- a/drivers/gpu/drm/radeon/rv770.c
>> +++ b/drivers/gpu/drm/radeon/rv770.c
>> @@ -1238,8 +1238,6 @@ static void rv770_pcie_gen2_enable(struct radeon_device *rdev)
>>   {
>>          u32 link_width_cntl, lanes, speed_cntl, tmp;
>>          u16 link_cntl2;
>> -       u32 mask;
>> -       int ret;
>>
>>          if (radeon_pcie_gen2 == 0)
>>                  return;
>> @@ -1254,11 +1252,7 @@ static void rv770_pcie_gen2_enable(struct radeon_device *rdev)
>>          if (ASIC_IS_X2(rdev))
>>                  return;
>>
>> -       ret = drm_pcie_get_speed_cap_mask(rdev->ddev,&mask);
>> -       if (ret != 0)
>> -               return;
>> -
>> -       if (!(mask&  DRM_PCIE_SPEED_50))
>> +       if (rdev->pdev->bus->max_bus_speed<  PCIE_SPEED_5_0GT)
>>                  return;
>>
>>          DRM_INFO("enabling PCIE gen 2 link speeds, disable with radeon.pcie_gen2=0\n");
>> --
>> 1.7.4.4
>>
>
Alex Deucher - April 17, 2013, 8:04 p.m.
On Wed, Apr 17, 2013 at 8:38 AM, Lucas Kannebley Tavares
<lucaskt@linux.vnet.ibm.com> wrote:
> On 04/12/2013 01:38 PM, Bjorn Helgaas wrote:
>>
>> On Thu, Apr 11, 2013 at 7:13 AM, Lucas Kannebley Tavares
>> <lucaskt@linux.vnet.ibm.com>  wrote:
>>>
>>> radeon currently uses a drm function to get the speed capabilities for
>>> the bus. However, this is a non-standard method of performing this
>>> detection and this patch changes it to use the max_bus_speed attribute.
>>> ---
>>>   drivers/gpu/drm/radeon/evergreen.c |    9 ++-------
>>>   drivers/gpu/drm/radeon/r600.c      |    8 +-------
>>>   drivers/gpu/drm/radeon/rv770.c     |    8 +-------
>>>   3 files changed, 4 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/radeon/evergreen.c
>>> b/drivers/gpu/drm/radeon/evergreen.c
>>> index 305a657..3291f62 100644
>>> --- a/drivers/gpu/drm/radeon/evergreen.c
>>> +++ b/drivers/gpu/drm/radeon/evergreen.c
>>> @@ -3855,8 +3855,7 @@ void evergreen_fini(struct radeon_device *rdev)
>>>
>>>   void evergreen_pcie_gen2_enable(struct radeon_device *rdev)
>>>   {
>>> -       u32 link_width_cntl, speed_cntl, mask;
>>> -       int ret;
>>> +       u32 link_width_cntl, speed_cntl;
>>>
>>>          if (radeon_pcie_gen2 == 0)
>>>                  return;
>>> @@ -3871,11 +3870,7 @@ void evergreen_pcie_gen2_enable(struct
>>> radeon_device *rdev)
>>>          if (ASIC_IS_X2(rdev))
>>>                  return;
>>>
>>> -       ret = drm_pcie_get_speed_cap_mask(rdev->ddev,&mask);
>>> -       if (ret != 0)
>>> -               return;
>>> -
>>> -       if (!(mask&  DRM_PCIE_SPEED_50))
>>>
>>> +       if (rdev->pdev->bus->max_bus_speed<  PCIE_SPEED_5_0GT)
>>
>>
>> For devices on a root bus, we previously dereferenced a NULL pointer
>> in drm_pcie_get_speed_cap_mask() because pdev->bus->self is NULL on a
>> root bus.  (I think this is the original problem you tripped over,
>> Lucas.)
>>
>> These patches fix that problem.  On pseries, where the device *is* on
>> a root bus, your patches set max_bus_speed so this will work as
>> expected.  On most other systems, max_bus_speed for root buses will be
>> PCI_SPEED_UNKNOWN (set in pci_alloc_bus() and never updated because
>> most arches don't have code like the pseries code you're adding).
>>
>> PCI_SPEED_UNKNOWN = 0xff, so if we see another machine with a GPU on
>> the root bus, we'll attempt to enable Gen2 on the device even though
>> we have no idea what the bus will support.
>>
>> That's why I originally suggested skipping the Gen2 stuff if
>> "max_bus_speed == PCI_SPEED_UNKNOWN".  I was just being conservative,
>> thinking that it's better to have a functional but slow GPU rather
>> than the unknown (to me) effects of enabling Gen2 on a link that might
>> not support it.  But I'm fine with this being either way.
>
>
> You're right here, of course. I'll test for it being different from 5_0GT
> and 8_0GT instead. Though at some point I suppose someone will want to
> tackle Gen3 speeds.

drm_pcie_get_speed_cap_mask() actually checked the pci bridge to see
what speeds it supported.  If the new code doesn't actually check the
bridge caps then the new code does not seem like a valid replacement
unless I'm missing something.

Alex

>
>
>>
>> It would be nice if we could get rid of drm_pcie_get_speed_cap_mask()
>> altogether.  It is exported, but I have no idea of anybody else uses
>> it.  Maybe it could at least be marked __deprecated now?
>>
>
> I'll mark it.
>
>> I don't know who should take these patches.  They don't touch
>> drivers/pci, but I'd be happy to push them, given the appropriate ACKs
>> from DRM and powerpc folks.
>>
>> Bjorn
>>
>>>                  return;
>>>
>>>          speed_cntl = RREG32_PCIE_P(PCIE_LC_SPEED_CNTL);
>>> diff --git a/drivers/gpu/drm/radeon/r600.c
>>> b/drivers/gpu/drm/radeon/r600.c
>>> index 0740db3..64b90c0 100644
>>> --- a/drivers/gpu/drm/radeon/r600.c
>>> +++ b/drivers/gpu/drm/radeon/r600.c
>>> @@ -4351,8 +4351,6 @@ static void r600_pcie_gen2_enable(struct
>>> radeon_device *rdev)
>>>   {
>>>          u32 link_width_cntl, lanes, speed_cntl, training_cntl, tmp;
>>>          u16 link_cntl2;
>>> -       u32 mask;
>>> -       int ret;
>>>
>>>          if (radeon_pcie_gen2 == 0)
>>>                  return;
>>> @@ -4371,11 +4369,7 @@ static void r600_pcie_gen2_enable(struct
>>> radeon_device *rdev)
>>>          if (rdev->family<= CHIP_R600)
>>>                  return;
>>>
>>> -       ret = drm_pcie_get_speed_cap_mask(rdev->ddev,&mask);
>>> -       if (ret != 0)
>>> -               return;
>>> -
>>> -       if (!(mask&  DRM_PCIE_SPEED_50))
>>>
>>> +       if (rdev->pdev->bus->max_bus_speed<  PCIE_SPEED_5_0GT)
>>>                  return;
>>>
>>>          speed_cntl = RREG32_PCIE_P(PCIE_LC_SPEED_CNTL);
>>> diff --git a/drivers/gpu/drm/radeon/rv770.c
>>> b/drivers/gpu/drm/radeon/rv770.c
>>> index d63fe1d..c683c36 100644
>>> --- a/drivers/gpu/drm/radeon/rv770.c
>>> +++ b/drivers/gpu/drm/radeon/rv770.c
>>> @@ -1238,8 +1238,6 @@ static void rv770_pcie_gen2_enable(struct
>>> radeon_device *rdev)
>>>   {
>>>          u32 link_width_cntl, lanes, speed_cntl, tmp;
>>>          u16 link_cntl2;
>>> -       u32 mask;
>>> -       int ret;
>>>
>>>          if (radeon_pcie_gen2 == 0)
>>>                  return;
>>> @@ -1254,11 +1252,7 @@ static void rv770_pcie_gen2_enable(struct
>>> radeon_device *rdev)
>>>          if (ASIC_IS_X2(rdev))
>>>                  return;
>>>
>>> -       ret = drm_pcie_get_speed_cap_mask(rdev->ddev,&mask);
>>> -       if (ret != 0)
>>> -               return;
>>> -
>>> -       if (!(mask&  DRM_PCIE_SPEED_50))
>>>
>>> +       if (rdev->pdev->bus->max_bus_speed<  PCIE_SPEED_5_0GT)
>>>                  return;
>>>
>>>          DRM_INFO("enabling PCIE gen 2 link speeds, disable with
>>> radeon.pcie_gen2=0\n");
>>> --
>>> 1.7.4.4
>>>
>>
>
>
> --
> Lucas Kannebley Tavares
> Software Engineer
> IBM Linux Technology Center
>
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
Bjorn Helgaas - April 17, 2013, 8:11 p.m.
On Wed, Apr 17, 2013 at 2:04 PM, Alex Deucher <alexdeucher@gmail.com> wrote:
> On Wed, Apr 17, 2013 at 8:38 AM, Lucas Kannebley Tavares
> <lucaskt@linux.vnet.ibm.com> wrote:
>> On 04/12/2013 01:38 PM, Bjorn Helgaas wrote:
>>>
>>> On Thu, Apr 11, 2013 at 7:13 AM, Lucas Kannebley Tavares
>>> <lucaskt@linux.vnet.ibm.com>  wrote:
>>>>
>>>> radeon currently uses a drm function to get the speed capabilities for
>>>> the bus. However, this is a non-standard method of performing this
>>>> detection and this patch changes it to use the max_bus_speed attribute.
>>>> ---
>>>>   drivers/gpu/drm/radeon/evergreen.c |    9 ++-------
>>>>   drivers/gpu/drm/radeon/r600.c      |    8 +-------
>>>>   drivers/gpu/drm/radeon/rv770.c     |    8 +-------
>>>>   3 files changed, 4 insertions(+), 21 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/radeon/evergreen.c
>>>> b/drivers/gpu/drm/radeon/evergreen.c
>>>> index 305a657..3291f62 100644
>>>> --- a/drivers/gpu/drm/radeon/evergreen.c
>>>> +++ b/drivers/gpu/drm/radeon/evergreen.c
>>>> @@ -3855,8 +3855,7 @@ void evergreen_fini(struct radeon_device *rdev)
>>>>
>>>>   void evergreen_pcie_gen2_enable(struct radeon_device *rdev)
>>>>   {
>>>> -       u32 link_width_cntl, speed_cntl, mask;
>>>> -       int ret;
>>>> +       u32 link_width_cntl, speed_cntl;
>>>>
>>>>          if (radeon_pcie_gen2 == 0)
>>>>                  return;
>>>> @@ -3871,11 +3870,7 @@ void evergreen_pcie_gen2_enable(struct
>>>> radeon_device *rdev)
>>>>          if (ASIC_IS_X2(rdev))
>>>>                  return;
>>>>
>>>> -       ret = drm_pcie_get_speed_cap_mask(rdev->ddev,&mask);
>>>> -       if (ret != 0)
>>>> -               return;
>>>> -
>>>> -       if (!(mask&  DRM_PCIE_SPEED_50))
>>>>
>>>> +       if (rdev->pdev->bus->max_bus_speed<  PCIE_SPEED_5_0GT)
>>>
>>>
>>> For devices on a root bus, we previously dereferenced a NULL pointer
>>> in drm_pcie_get_speed_cap_mask() because pdev->bus->self is NULL on a
>>> root bus.  (I think this is the original problem you tripped over,
>>> Lucas.)
>>>
>>> These patches fix that problem.  On pseries, where the device *is* on
>>> a root bus, your patches set max_bus_speed so this will work as
>>> expected.  On most other systems, max_bus_speed for root buses will be
>>> PCI_SPEED_UNKNOWN (set in pci_alloc_bus() and never updated because
>>> most arches don't have code like the pseries code you're adding).
>>>
>>> PCI_SPEED_UNKNOWN = 0xff, so if we see another machine with a GPU on
>>> the root bus, we'll attempt to enable Gen2 on the device even though
>>> we have no idea what the bus will support.
>>>
>>> That's why I originally suggested skipping the Gen2 stuff if
>>> "max_bus_speed == PCI_SPEED_UNKNOWN".  I was just being conservative,
>>> thinking that it's better to have a functional but slow GPU rather
>>> than the unknown (to me) effects of enabling Gen2 on a link that might
>>> not support it.  But I'm fine with this being either way.
>>
>>
>> You're right here, of course. I'll test for it being different from 5_0GT
>> and 8_0GT instead. Though at some point I suppose someone will want to
>> tackle Gen3 speeds.
>
> drm_pcie_get_speed_cap_mask() actually checked the pci bridge to see
> what speeds it supported.  If the new code doesn't actually check the
> bridge caps then the new code does not seem like a valid replacement
> unless I'm missing something.

The max_bus_speed in struct pci_bus is set in pci_set_bus_speed()
based on the PCIe, PCI-X, or AGP capabilities.  This happens when we
enumerate the bridge device.  Or, in this case, Lucas added
powerpc-specific code to set max_bus_speed for the root bus based on
platform-specific host bridge knowledge.

So it still does check the PCI bridge capabilities, just not as
directly as it did before.

Bjorn
Alex Deucher - April 17, 2013, 8:17 p.m.
On Wed, Apr 17, 2013 at 4:11 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Wed, Apr 17, 2013 at 2:04 PM, Alex Deucher <alexdeucher@gmail.com> wrote:
>> On Wed, Apr 17, 2013 at 8:38 AM, Lucas Kannebley Tavares
>> <lucaskt@linux.vnet.ibm.com> wrote:
>>> On 04/12/2013 01:38 PM, Bjorn Helgaas wrote:
>>>>
>>>> On Thu, Apr 11, 2013 at 7:13 AM, Lucas Kannebley Tavares
>>>> <lucaskt@linux.vnet.ibm.com>  wrote:
>>>>>
>>>>> radeon currently uses a drm function to get the speed capabilities for
>>>>> the bus. However, this is a non-standard method of performing this
>>>>> detection and this patch changes it to use the max_bus_speed attribute.
>>>>> ---
>>>>>   drivers/gpu/drm/radeon/evergreen.c |    9 ++-------
>>>>>   drivers/gpu/drm/radeon/r600.c      |    8 +-------
>>>>>   drivers/gpu/drm/radeon/rv770.c     |    8 +-------
>>>>>   3 files changed, 4 insertions(+), 21 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/radeon/evergreen.c
>>>>> b/drivers/gpu/drm/radeon/evergreen.c
>>>>> index 305a657..3291f62 100644
>>>>> --- a/drivers/gpu/drm/radeon/evergreen.c
>>>>> +++ b/drivers/gpu/drm/radeon/evergreen.c
>>>>> @@ -3855,8 +3855,7 @@ void evergreen_fini(struct radeon_device *rdev)
>>>>>
>>>>>   void evergreen_pcie_gen2_enable(struct radeon_device *rdev)
>>>>>   {
>>>>> -       u32 link_width_cntl, speed_cntl, mask;
>>>>> -       int ret;
>>>>> +       u32 link_width_cntl, speed_cntl;
>>>>>
>>>>>          if (radeon_pcie_gen2 == 0)
>>>>>                  return;
>>>>> @@ -3871,11 +3870,7 @@ void evergreen_pcie_gen2_enable(struct
>>>>> radeon_device *rdev)
>>>>>          if (ASIC_IS_X2(rdev))
>>>>>                  return;
>>>>>
>>>>> -       ret = drm_pcie_get_speed_cap_mask(rdev->ddev,&mask);
>>>>> -       if (ret != 0)
>>>>> -               return;
>>>>> -
>>>>> -       if (!(mask&  DRM_PCIE_SPEED_50))
>>>>>
>>>>> +       if (rdev->pdev->bus->max_bus_speed<  PCIE_SPEED_5_0GT)
>>>>
>>>>
>>>> For devices on a root bus, we previously dereferenced a NULL pointer
>>>> in drm_pcie_get_speed_cap_mask() because pdev->bus->self is NULL on a
>>>> root bus.  (I think this is the original problem you tripped over,
>>>> Lucas.)
>>>>
>>>> These patches fix that problem.  On pseries, where the device *is* on
>>>> a root bus, your patches set max_bus_speed so this will work as
>>>> expected.  On most other systems, max_bus_speed for root buses will be
>>>> PCI_SPEED_UNKNOWN (set in pci_alloc_bus() and never updated because
>>>> most arches don't have code like the pseries code you're adding).
>>>>
>>>> PCI_SPEED_UNKNOWN = 0xff, so if we see another machine with a GPU on
>>>> the root bus, we'll attempt to enable Gen2 on the device even though
>>>> we have no idea what the bus will support.
>>>>
>>>> That's why I originally suggested skipping the Gen2 stuff if
>>>> "max_bus_speed == PCI_SPEED_UNKNOWN".  I was just being conservative,
>>>> thinking that it's better to have a functional but slow GPU rather
>>>> than the unknown (to me) effects of enabling Gen2 on a link that might
>>>> not support it.  But I'm fine with this being either way.
>>>
>>>
>>> You're right here, of course. I'll test for it being different from 5_0GT
>>> and 8_0GT instead. Though at some point I suppose someone will want to
>>> tackle Gen3 speeds.
>>
>> drm_pcie_get_speed_cap_mask() actually checked the pci bridge to see
>> what speeds it supported.  If the new code doesn't actually check the
>> bridge caps then the new code does not seem like a valid replacement
>> unless I'm missing something.
>
> The max_bus_speed in struct pci_bus is set in pci_set_bus_speed()
> based on the PCIe, PCI-X, or AGP capabilities.  This happens when we
> enumerate the bridge device.  Or, in this case, Lucas added
> powerpc-specific code to set max_bus_speed for the root bus based on
> platform-specific host bridge knowledge.
>
> So it still does check the PCI bridge capabilities, just not as
> directly as it did before.

Ah, ok.  Thanks.  The previous comments about PCI_SPEED_UNKNOWN being
set in pci_alloc_bus() and never updated confused me.

Alex
Bjorn Helgaas - April 17, 2013, 8:30 p.m.
On Wed, Apr 17, 2013 at 2:17 PM, Alex Deucher <alexdeucher@gmail.com> wrote:
> On Wed, Apr 17, 2013 at 4:11 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> On Wed, Apr 17, 2013 at 2:04 PM, Alex Deucher <alexdeucher@gmail.com> wrote:
>>> On Wed, Apr 17, 2013 at 8:38 AM, Lucas Kannebley Tavares
>>> <lucaskt@linux.vnet.ibm.com> wrote:
>>>> On 04/12/2013 01:38 PM, Bjorn Helgaas wrote:
>>>>>
>>>>> On Thu, Apr 11, 2013 at 7:13 AM, Lucas Kannebley Tavares
>>>>> <lucaskt@linux.vnet.ibm.com>  wrote:
>>>>>>
>>>>>> radeon currently uses a drm function to get the speed capabilities for
>>>>>> the bus. However, this is a non-standard method of performing this
>>>>>> detection and this patch changes it to use the max_bus_speed attribute.
>>>>>> ---
>>>>>>   drivers/gpu/drm/radeon/evergreen.c |    9 ++-------
>>>>>>   drivers/gpu/drm/radeon/r600.c      |    8 +-------
>>>>>>   drivers/gpu/drm/radeon/rv770.c     |    8 +-------
>>>>>>   3 files changed, 4 insertions(+), 21 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/radeon/evergreen.c
>>>>>> b/drivers/gpu/drm/radeon/evergreen.c
>>>>>> index 305a657..3291f62 100644
>>>>>> --- a/drivers/gpu/drm/radeon/evergreen.c
>>>>>> +++ b/drivers/gpu/drm/radeon/evergreen.c
>>>>>> @@ -3855,8 +3855,7 @@ void evergreen_fini(struct radeon_device *rdev)
>>>>>>
>>>>>>   void evergreen_pcie_gen2_enable(struct radeon_device *rdev)
>>>>>>   {
>>>>>> -       u32 link_width_cntl, speed_cntl, mask;
>>>>>> -       int ret;
>>>>>> +       u32 link_width_cntl, speed_cntl;
>>>>>>
>>>>>>          if (radeon_pcie_gen2 == 0)
>>>>>>                  return;
>>>>>> @@ -3871,11 +3870,7 @@ void evergreen_pcie_gen2_enable(struct
>>>>>> radeon_device *rdev)
>>>>>>          if (ASIC_IS_X2(rdev))
>>>>>>                  return;
>>>>>>
>>>>>> -       ret = drm_pcie_get_speed_cap_mask(rdev->ddev,&mask);
>>>>>> -       if (ret != 0)
>>>>>> -               return;
>>>>>> -
>>>>>> -       if (!(mask&  DRM_PCIE_SPEED_50))
>>>>>>
>>>>>> +       if (rdev->pdev->bus->max_bus_speed<  PCIE_SPEED_5_0GT)
>>>>>
>>>>>
>>>>> For devices on a root bus, we previously dereferenced a NULL pointer
>>>>> in drm_pcie_get_speed_cap_mask() because pdev->bus->self is NULL on a
>>>>> root bus.  (I think this is the original problem you tripped over,
>>>>> Lucas.)
>>>>>
>>>>> These patches fix that problem.  On pseries, where the device *is* on
>>>>> a root bus, your patches set max_bus_speed so this will work as
>>>>> expected.  On most other systems, max_bus_speed for root buses will be
>>>>> PCI_SPEED_UNKNOWN (set in pci_alloc_bus() and never updated because
>>>>> most arches don't have code like the pseries code you're adding).
>>>>>
>>>>> PCI_SPEED_UNKNOWN = 0xff, so if we see another machine with a GPU on
>>>>> the root bus, we'll attempt to enable Gen2 on the device even though
>>>>> we have no idea what the bus will support.
>>>>>
>>>>> That's why I originally suggested skipping the Gen2 stuff if
>>>>> "max_bus_speed == PCI_SPEED_UNKNOWN".  I was just being conservative,
>>>>> thinking that it's better to have a functional but slow GPU rather
>>>>> than the unknown (to me) effects of enabling Gen2 on a link that might
>>>>> not support it.  But I'm fine with this being either way.
>>>>
>>>>
>>>> You're right here, of course. I'll test for it being different from 5_0GT
>>>> and 8_0GT instead. Though at some point I suppose someone will want to
>>>> tackle Gen3 speeds.
>>>
>>> drm_pcie_get_speed_cap_mask() actually checked the pci bridge to see
>>> what speeds it supported.  If the new code doesn't actually check the
>>> bridge caps then the new code does not seem like a valid replacement
>>> unless I'm missing something.
>>
>> The max_bus_speed in struct pci_bus is set in pci_set_bus_speed()
>> based on the PCIe, PCI-X, or AGP capabilities.  This happens when we
>> enumerate the bridge device.  Or, in this case, Lucas added
>> powerpc-specific code to set max_bus_speed for the root bus based on
>> platform-specific host bridge knowledge.
>>
>> So it still does check the PCI bridge capabilities, just not as
>> directly as it did before.
>
> Ah, ok.  Thanks.  The previous comments about PCI_SPEED_UNKNOWN being
> set in pci_alloc_bus() and never updated confused me.

Yeah, that's just for root buses where we don't have the host bridge
knowledge to figure it out.  The root bus case was broken in
drm_pcie_get_speed_cap_mask() anyway, because there is no upstream P2P
bridge to look at.  (That's why Lucas tripped over the null pointer
dereference in the first place.)

Patch

diff --git a/drivers/gpu/drm/radeon/evergreen.c b/drivers/gpu/drm/radeon/evergreen.c
index 305a657..3291f62 100644
--- a/drivers/gpu/drm/radeon/evergreen.c
+++ b/drivers/gpu/drm/radeon/evergreen.c
@@ -3855,8 +3855,7 @@  void evergreen_fini(struct radeon_device *rdev)
 
 void evergreen_pcie_gen2_enable(struct radeon_device *rdev)
 {
-	u32 link_width_cntl, speed_cntl, mask;
-	int ret;
+	u32 link_width_cntl, speed_cntl;
 
 	if (radeon_pcie_gen2 == 0)
 		return;
@@ -3871,11 +3870,7 @@  void evergreen_pcie_gen2_enable(struct radeon_device *rdev)
 	if (ASIC_IS_X2(rdev))
 		return;
 
-	ret = drm_pcie_get_speed_cap_mask(rdev->ddev, &mask);
-	if (ret != 0)
-		return;
-
-	if (!(mask & DRM_PCIE_SPEED_50))
+	if (rdev->pdev->bus->max_bus_speed < PCIE_SPEED_5_0GT)
 		return;
 
 	speed_cntl = RREG32_PCIE_P(PCIE_LC_SPEED_CNTL);
diff --git a/drivers/gpu/drm/radeon/r600.c b/drivers/gpu/drm/radeon/r600.c
index 0740db3..64b90c0 100644
--- a/drivers/gpu/drm/radeon/r600.c
+++ b/drivers/gpu/drm/radeon/r600.c
@@ -4351,8 +4351,6 @@  static void r600_pcie_gen2_enable(struct radeon_device *rdev)
 {
 	u32 link_width_cntl, lanes, speed_cntl, training_cntl, tmp;
 	u16 link_cntl2;
-	u32 mask;
-	int ret;
 
 	if (radeon_pcie_gen2 == 0)
 		return;
@@ -4371,11 +4369,7 @@  static void r600_pcie_gen2_enable(struct radeon_device *rdev)
 	if (rdev->family <= CHIP_R600)
 		return;
 
-	ret = drm_pcie_get_speed_cap_mask(rdev->ddev, &mask);
-	if (ret != 0)
-		return;
-
-	if (!(mask & DRM_PCIE_SPEED_50))
+	if (rdev->pdev->bus->max_bus_speed < PCIE_SPEED_5_0GT)
 		return;
 
 	speed_cntl = RREG32_PCIE_P(PCIE_LC_SPEED_CNTL);
diff --git a/drivers/gpu/drm/radeon/rv770.c b/drivers/gpu/drm/radeon/rv770.c
index d63fe1d..c683c36 100644
--- a/drivers/gpu/drm/radeon/rv770.c
+++ b/drivers/gpu/drm/radeon/rv770.c
@@ -1238,8 +1238,6 @@  static void rv770_pcie_gen2_enable(struct radeon_device *rdev)
 {
 	u32 link_width_cntl, lanes, speed_cntl, tmp;
 	u16 link_cntl2;
-	u32 mask;
-	int ret;
 
 	if (radeon_pcie_gen2 == 0)
 		return;
@@ -1254,11 +1252,7 @@  static void rv770_pcie_gen2_enable(struct radeon_device *rdev)
 	if (ASIC_IS_X2(rdev))
 		return;
 
-	ret = drm_pcie_get_speed_cap_mask(rdev->ddev, &mask);
-	if (ret != 0)
-		return;
-
-	if (!(mask & DRM_PCIE_SPEED_50))
+	if (rdev->pdev->bus->max_bus_speed < PCIE_SPEED_5_0GT)
 		return;
 
 	DRM_INFO("enabling PCIE gen 2 link speeds, disable with radeon.pcie_gen2=0\n");