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

[Alexandre Demers <alexandre.f.demers@gmail.com>]

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