[PATCH] drm/amdgpu: fix num_rbs exposed to userspace

[PATCH] drm/amdgpu: fix num_rbs exposed to userspace

[Alex Deucher]

This was accidently broken for harvest cards when the
code was refactored for Polaris support.

Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
Cc: stable@vger.kernel.org
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
index 9ab28ca..e5c22cd 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
@@ -459,7 +459,7 @@ static int amdgpu_info_ioctl(struct drm_device *dev, void *data, struct drm_file
 			dev_info.max_memory_clock = adev->pm.default_mclk * 10;
 		}
 		dev_info.enabled_rb_pipes_mask = adev->gfx.config.backend_enable_mask;
-		dev_info.num_rb_pipes = adev->gfx.config.num_rbs;
+		dev_info.num_rb_pipes = adev->gfx.config.max_backends_per_se;
 		dev_info.num_hw_gfx_contexts = adev->gfx.config.max_hw_contexts;
 		dev_info._pad = 0;
 		dev_info.ids_flags = 0;
-- 
2.5.5

Re: [amd-gfx] [PATCH] drm/amdgpu: fix num_rbs exposed to userspace

[Nicolai Hähnle]

On 17.06.2016 16:20, Alex Deucher wrote:
> This was accidently broken for harvest cards when the
> code was refactored for Polaris support.
>
> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
> Cc: stable@vger.kernel.org
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> index 9ab28ca..e5c22cd 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> @@ -459,7 +459,7 @@ static int amdgpu_info_ioctl(struct drm_device *dev, void *data, struct drm_file
>   			dev_info.max_memory_clock = adev->pm.default_mclk * 10;
>   		}
>   		dev_info.enabled_rb_pipes_mask = adev->gfx.config.backend_enable_mask;
> -		dev_info.num_rb_pipes = adev->gfx.config.num_rbs;
> +		dev_info.num_rb_pipes = adev->gfx.config.max_backends_per_se;

At a glance, that looks suspicious to me. num_rb_pipes becomes rb_pipes 
in libdrmm and then num_render_backends. We divide num_render_backends 
by the number of SEs * SHs in radeonsi.

In a nutshell, radeonsi expects this to be the total number of RBs 
(including disabled/harvested ones).

Nicolai

>   		dev_info.num_hw_gfx_contexts = adev->gfx.config.max_hw_contexts;
>   		dev_info._pad = 0;
>   		dev_info.ids_flags = 0;
>

Re: [amd-gfx] [PATCH] drm/amdgpu: fix num_rbs exposed to userspace

[Alex Deucher]

On Fri, Jun 17, 2016 at 11:31 AM, Nicolai Hähnle <nhaehnle@gmail.com> wrote:
> On 17.06.2016 16:20, Alex Deucher wrote:
>>
>> This was accidently broken for harvest cards when the
>> code was refactored for Polaris support.
>>
>> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
>> Cc: stable@vger.kernel.org
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>> index 9ab28ca..e5c22cd 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>> @@ -459,7 +459,7 @@ static int amdgpu_info_ioctl(struct drm_device *dev,
>> void *data, struct drm_file
>>                         dev_info.max_memory_clock = adev->pm.default_mclk
>> * 10;
>>                 }
>>                 dev_info.enabled_rb_pipes_mask =
>> adev->gfx.config.backend_enable_mask;
>> -               dev_info.num_rb_pipes = adev->gfx.config.num_rbs;
>> +               dev_info.num_rb_pipes =
>> adev->gfx.config.max_backends_per_se;
>
>
> At a glance, that looks suspicious to me. num_rb_pipes becomes rb_pipes in
> libdrmm and then num_render_backends. We divide num_render_backends by the
> number of SEs * SHs in radeonsi.
>
> In a nutshell, radeonsi expects this to be the total number of RBs
> (including disabled/harvested ones).

Right.  that's what this patch does.
adev->gfx.config.max_backends_per_se is the total number of RBs per SE
available on the asic. adev->gfx.config.num_rbs is the total number of
enabled RBs (max - disabled).  For non-harvest cards, they are the
same.

Alex

>
> Nicolai
>
>
>>                 dev_info.num_hw_gfx_contexts =
>> adev->gfx.config.max_hw_contexts;
>>                 dev_info._pad = 0;
>>                 dev_info.ids_flags = 0;
>>
>

Re: [amd-gfx] [PATCH] drm/amdgpu: fix num_rbs exposed to userspace

[Alexandre Demers]

On 2016-06-17 11:37, Alex Deucher wrote:
> On Fri, Jun 17, 2016 at 11:31 AM, Nicolai Hähnle <nhaehnle@gmail.com> wrote:
>> On 17.06.2016 16:20, Alex Deucher wrote:
>>> This was accidently broken for harvest cards when the
>>> code was refactored for Polaris support.
>>>
>>> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
>>> Cc: stable@vger.kernel.org
>>> ---
>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>>> index 9ab28ca..e5c22cd 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>>> @@ -459,7 +459,7 @@ static int amdgpu_info_ioctl(struct drm_device *dev,
>>> void *data, struct drm_file
>>>                          dev_info.max_memory_clock = adev->pm.default_mclk
>>> * 10;
>>>                  }
>>>                  dev_info.enabled_rb_pipes_mask =
>>> adev->gfx.config.backend_enable_mask;
>>> -               dev_info.num_rb_pipes = adev->gfx.config.num_rbs;
>>> +               dev_info.num_rb_pipes =
>>> adev->gfx.config.max_backends_per_se;
>>
>> At a glance, that looks suspicious to me. num_rb_pipes becomes rb_pipes in
>> libdrmm and then num_render_backends. We divide num_render_backends by the
>> number of SEs * SHs in radeonsi.
>>
>> In a nutshell, radeonsi expects this to be the total number of RBs
>> (including disabled/harvested ones).
> Right.  that's what this patch does.
> adev->gfx.config.max_backends_per_se is the total number of RBs per SE
> available on the asic. adev->gfx.config.num_rbs is the total number of
> enabled RBs (max - disabled).  For non-harvest cards, they are the
> same.
>
> Alex
If I may suggest for consistency with what you are describing and 
between the way the variables are named, at some point:
- .num_rbs could be renamed to .num_enabled_rbs or .total_enabled_rbs or 
.max_enabled_rbs
- .max_backends_per_se could be renamed to .max_rbs_per_se

My two cents here from an external POV.

Alexandre Demers
>
>> Nicolai
>>
>>
>>>                  dev_info.num_hw_gfx_contexts =
>>> adev->gfx.config.max_hw_contexts;
>>>                  dev_info._pad = 0;
>>>                  dev_info.ids_flags = 0;
>>>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [amd-gfx] [PATCH] drm/amdgpu: fix num_rbs exposed to userspace

[Nicolai Hähnle]

On 17.06.2016 17:37, Alex Deucher wrote:
> On Fri, Jun 17, 2016 at 11:31 AM, Nicolai Hähnle <nhaehnle@gmail.com> wrote:
>> On 17.06.2016 16:20, Alex Deucher wrote:
>>>
>>> This was accidently broken for harvest cards when the
>>> code was refactored for Polaris support.
>>>
>>> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
>>> Cc: stable@vger.kernel.org
>>> ---
>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>>> index 9ab28ca..e5c22cd 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>>> @@ -459,7 +459,7 @@ static int amdgpu_info_ioctl(struct drm_device *dev,
>>> void *data, struct drm_file
>>>                          dev_info.max_memory_clock = adev->pm.default_mclk
>>> * 10;
>>>                  }
>>>                  dev_info.enabled_rb_pipes_mask =
>>> adev->gfx.config.backend_enable_mask;
>>> -               dev_info.num_rb_pipes = adev->gfx.config.num_rbs;
>>> +               dev_info.num_rb_pipes =
>>> adev->gfx.config.max_backends_per_se;
>>
>>
>> At a glance, that looks suspicious to me. num_rb_pipes becomes rb_pipes in
>> libdrmm and then num_render_backends. We divide num_render_backends by the
>> number of SEs * SHs in radeonsi.
>>
>> In a nutshell, radeonsi expects this to be the total number of RBs
>> (including disabled/harvested ones).
>
> Right.  that's what this patch does.
> adev->gfx.config.max_backends_per_se is the total number of RBs per SE
> available on the asic. adev->gfx.config.num_rbs is the total number of
> enabled RBs (max - disabled).  For non-harvest cards, they are the
> same.

But the total number of RBs is different from the total number of RBs 
per SE...

Nicolai

>
> Alex
>
>>
>> Nicolai
>>
>>
>>>                  dev_info.num_hw_gfx_contexts =
>>> adev->gfx.config.max_hw_contexts;
>>>                  dev_info._pad = 0;
>>>                  dev_info.ids_flags = 0;
>>>
>>

Re: [amd-gfx] [PATCH] drm/amdgpu: fix num_rbs exposed to userspace

[Alex Deucher]

On Fri, Jun 17, 2016 at 1:36 PM, Nicolai Hähnle <nhaehnle@gmail.com> wrote:
> On 17.06.2016 17:37, Alex Deucher wrote:
>>
>> On Fri, Jun 17, 2016 at 11:31 AM, Nicolai Hähnle <nhaehnle@gmail.com>
>> wrote:
>>>
>>> On 17.06.2016 16:20, Alex Deucher wrote:
>>>>
>>>>
>>>> This was accidently broken for harvest cards when the
>>>> code was refactored for Polaris support.
>>>>
>>>> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
>>>> Cc: stable@vger.kernel.org
>>>> ---
>>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>>>> index 9ab28ca..e5c22cd 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>>>> @@ -459,7 +459,7 @@ static int amdgpu_info_ioctl(struct drm_device *dev,
>>>> void *data, struct drm_file
>>>>                          dev_info.max_memory_clock =
>>>> adev->pm.default_mclk
>>>> * 10;
>>>>                  }
>>>>                  dev_info.enabled_rb_pipes_mask =
>>>> adev->gfx.config.backend_enable_mask;
>>>> -               dev_info.num_rb_pipes = adev->gfx.config.num_rbs;
>>>> +               dev_info.num_rb_pipes =
>>>> adev->gfx.config.max_backends_per_se;
>>>
>>>
>>>
>>> At a glance, that looks suspicious to me. num_rb_pipes becomes rb_pipes
>>> in
>>> libdrmm and then num_render_backends. We divide num_render_backends by
>>> the
>>> number of SEs * SHs in radeonsi.
>>>
>>> In a nutshell, radeonsi expects this to be the total number of RBs
>>> (including disabled/harvested ones).
>>
>>
>> Right.  that's what this patch does.
>> adev->gfx.config.max_backends_per_se is the total number of RBs per SE
>> available on the asic. adev->gfx.config.num_rbs is the total number of
>> enabled RBs (max - disabled).  For non-harvest cards, they are the
>> same.
>
>
> But the total number of RBs is different from the total number of RBs per
> SE...
>

Whoops yes.  New patch sent.

Alex

> Nicolai
>
>
>>
>> Alex
>>
>>>
>>> Nicolai
>>>
>>>
>>>>                  dev_info.num_hw_gfx_contexts =
>>>> adev->gfx.config.max_hw_contexts;
>>>>                  dev_info._pad = 0;
>>>>                  dev_info.ids_flags = 0;
>>>>
>>>
>