Re: [PATCH v2] drm/amdgpu: update IP count INFO query

[Sundararaju, Sathishkumar]

On 9/18/2023 7:57 PM, Alex Deucher wrote:

On Sat, Sep 16, 2023 at 11:01 PM Sathishkumar S
 wrote:

update the query to return the number of functional
instances where there is more than an instance of the requested
type and for others continue to return one.

v2: count must reflect the actual number of engines (Alex)

Signed-off-by: Sathishkumar S 
Reviewed-by: Leo Liu 
---
  drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 92 +
  1 file changed, 63 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
index 24d866ed5922..56273a7c9028 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
@@ -200,6 +200,44 @@ int amdgpu_driver_load_kms(struct amdgpu_device *adev, 
unsigned long flags)
 return r;
  }

+static enum amd_ip_block_type amdgpu_ip_get_block_type(
+   struct amdgpu_device *adev, uint32_t ip)
+{
+   enum amd_ip_block_type type;
+
+   switch (ip) {
+   case AMDGPU_HW_IP_GFX:
+   type = AMD_IP_BLOCK_TYPE_GFX;
+   break;
+   case AMDGPU_HW_IP_COMPUTE:
+   type = AMD_IP_BLOCK_TYPE_GFX;
+   break;
+   case AMDGPU_HW_IP_DMA:
+   type = AMD_IP_BLOCK_TYPE_SDMA;
+   break;
+   case AMDGPU_HW_IP_UVD:
+   case AMDGPU_HW_IP_UVD_ENC:
+   type = AMD_IP_BLOCK_TYPE_UVD;
+   break;
+   case AMDGPU_HW_IP_VCE:
+   type = AMD_IP_BLOCK_TYPE_VCE;
+   break;
+   case AMDGPU_HW_IP_VCN_DEC:
+   case AMDGPU_HW_IP_VCN_ENC:
+   type = AMD_IP_BLOCK_TYPE_VCN;
+   break;
+   case AMDGPU_HW_IP_VCN_JPEG:
+   type = (amdgpu_device_ip_get_ip_block(adev, 
AMD_IP_BLOCK_TYPE_JPEG)) ?
+  AMD_IP_BLOCK_TYPE_JPEG : 
AMD_IP_BLOCK_TYPE_VCN;
+   break;
+   default:
+   type = AMD_IP_BLOCK_TYPE_NUM;
+   break;
+   }
+
+   return type;
+}
+
  static int amdgpu_firmware_info(struct drm_amdgpu_info_firmware *fw_info,
 struct drm_amdgpu_query_fw *query_fw,
 struct amdgpu_device *adev)
@@ -600,45 +638,41 @@ int amdgpu_info_ioctl(struct drm_device *dev, void *data, 
struct drm_file *filp)
 }
 case AMDGPU_INFO_HW_IP_COUNT: {
 enum amd_ip_block_type type;
+   struct amdgpu_ip_block *ip_block = NULL;
 uint32_t count = 0;

-   switch (info->query_hw_ip.type) {
-   case AMDGPU_HW_IP_GFX:
-   type = AMD_IP_BLOCK_TYPE_GFX;
-   break;
-   case AMDGPU_HW_IP_COMPUTE:
-   type = AMD_IP_BLOCK_TYPE_GFX;
-   break;
-   case AMDGPU_HW_IP_DMA:
-   type = AMD_IP_BLOCK_TYPE_SDMA;
-   break;
-   case AMDGPU_HW_IP_UVD:
-   type = AMD_IP_BLOCK_TYPE_UVD;
+   type = amdgpu_ip_get_block_type(adev, info->query_hw_ip.type);
+   ip_block = amdgpu_device_ip_get_ip_block(adev, type);
+   if (!ip_block || !ip_block->status.valid)
+   return -EINVAL;
+
+   switch (type) {
+   case AMD_IP_BLOCK_TYPE_GFX:
+   case AMD_IP_BLOCK_TYPE_VCE:
+   count = 1;
 break;
-   case AMDGPU_HW_IP_VCE:
-   type = AMD_IP_BLOCK_TYPE_VCE;
+   case AMD_IP_BLOCK_TYPE_SDMA:
+   count = adev->sdma.num_instances;
 break;
-   case AMDGPU_HW_IP_UVD_ENC:
-   type = AMD_IP_BLOCK_TYPE_UVD;
+   case AMD_IP_BLOCK_TYPE_JPEG:
+   count = adev->jpeg.num_jpeg_inst * 
adev->jpeg.num_jpeg_rings;
 break;
-   case AMDGPU_HW_IP_VCN_DEC:
-   case AMDGPU_HW_IP_VCN_ENC:
-   type = AMD_IP_BLOCK_TYPE_VCN;
+   case AMD_IP_BLOCK_TYPE_VCN:
+   count = (info->query_hw_ip.type == 
AMDGPU_HW_IP_VCN_ENC) ?
+   adev->vcn.num_vcn_inst * 
adev->vcn.num_enc_rings :
+   adev->vcn.num_vcn_inst;

Is this correct?  JPEG has a 1:1 mapping from ring to engine, but I
think VCN is usually multiple rings per engine.  Shouldn't this just
return adev->vcn.num_vcn_inst?

Alex


Right, I am sorry I misunderstood this and made the change, for vcn it 
must be adev->vcn.num_vcn_inst, multiple rings here do not represent 
separate engines.


Regards,

Sathish




 break;
-   case AMDGPU_HW_IP_VCN_JPEG:
-   type = (amdgpu_device_ip_get_ip_block(adev, 
AMD_IP_BLOCK_TYPE_JPEG)) ?
-   

Re: [PATCH v2] drm/amdgpu: update IP count INFO query

[Alex Deucher]

On Sat, Sep 16, 2023 at 11:01 PM Sathishkumar S
 wrote:
>
> update the query to return the number of functional
> instances where there is more than an instance of the requested
> type and for others continue to return one.
>
> v2: count must reflect the actual number of engines (Alex)
>
> Signed-off-by: Sathishkumar S 
> Reviewed-by: Leo Liu 
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 92 +
>  1 file changed, 63 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> index 24d866ed5922..56273a7c9028 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> @@ -200,6 +200,44 @@ int amdgpu_driver_load_kms(struct amdgpu_device *adev, 
> unsigned long flags)
> return r;
>  }
>
> +static enum amd_ip_block_type amdgpu_ip_get_block_type(
> +   struct amdgpu_device *adev, uint32_t ip)
> +{
> +   enum amd_ip_block_type type;
> +
> +   switch (ip) {
> +   case AMDGPU_HW_IP_GFX:
> +   type = AMD_IP_BLOCK_TYPE_GFX;
> +   break;
> +   case AMDGPU_HW_IP_COMPUTE:
> +   type = AMD_IP_BLOCK_TYPE_GFX;
> +   break;
> +   case AMDGPU_HW_IP_DMA:
> +   type = AMD_IP_BLOCK_TYPE_SDMA;
> +   break;
> +   case AMDGPU_HW_IP_UVD:
> +   case AMDGPU_HW_IP_UVD_ENC:
> +   type = AMD_IP_BLOCK_TYPE_UVD;
> +   break;
> +   case AMDGPU_HW_IP_VCE:
> +   type = AMD_IP_BLOCK_TYPE_VCE;
> +   break;
> +   case AMDGPU_HW_IP_VCN_DEC:
> +   case AMDGPU_HW_IP_VCN_ENC:
> +   type = AMD_IP_BLOCK_TYPE_VCN;
> +   break;
> +   case AMDGPU_HW_IP_VCN_JPEG:
> +   type = (amdgpu_device_ip_get_ip_block(adev, 
> AMD_IP_BLOCK_TYPE_JPEG)) ?
> +  AMD_IP_BLOCK_TYPE_JPEG : 
> AMD_IP_BLOCK_TYPE_VCN;
> +   break;
> +   default:
> +   type = AMD_IP_BLOCK_TYPE_NUM;
> +   break;
> +   }
> +
> +   return type;
> +}
> +
>  static int amdgpu_firmware_info(struct drm_amdgpu_info_firmware *fw_info,
> struct drm_amdgpu_query_fw *query_fw,
> struct amdgpu_device *adev)
> @@ -600,45 +638,41 @@ int amdgpu_info_ioctl(struct drm_device *dev, void 
> *data, struct drm_file *filp)
> }
> case AMDGPU_INFO_HW_IP_COUNT: {
> enum amd_ip_block_type type;
> +   struct amdgpu_ip_block *ip_block = NULL;
> uint32_t count = 0;
>
> -   switch (info->query_hw_ip.type) {
> -   case AMDGPU_HW_IP_GFX:
> -   type = AMD_IP_BLOCK_TYPE_GFX;
> -   break;
> -   case AMDGPU_HW_IP_COMPUTE:
> -   type = AMD_IP_BLOCK_TYPE_GFX;
> -   break;
> -   case AMDGPU_HW_IP_DMA:
> -   type = AMD_IP_BLOCK_TYPE_SDMA;
> -   break;
> -   case AMDGPU_HW_IP_UVD:
> -   type = AMD_IP_BLOCK_TYPE_UVD;
> +   type = amdgpu_ip_get_block_type(adev, info->query_hw_ip.type);
> +   ip_block = amdgpu_device_ip_get_ip_block(adev, type);
> +   if (!ip_block || !ip_block->status.valid)
> +   return -EINVAL;
> +
> +   switch (type) {
> +   case AMD_IP_BLOCK_TYPE_GFX:
> +   case AMD_IP_BLOCK_TYPE_VCE:
> +   count = 1;
> break;
> -   case AMDGPU_HW_IP_VCE:
> -   type = AMD_IP_BLOCK_TYPE_VCE;
> +   case AMD_IP_BLOCK_TYPE_SDMA:
> +   count = adev->sdma.num_instances;
> break;
> -   case AMDGPU_HW_IP_UVD_ENC:
> -   type = AMD_IP_BLOCK_TYPE_UVD;
> +   case AMD_IP_BLOCK_TYPE_JPEG:
> +   count = adev->jpeg.num_jpeg_inst * 
> adev->jpeg.num_jpeg_rings;
> break;
> -   case AMDGPU_HW_IP_VCN_DEC:
> -   case AMDGPU_HW_IP_VCN_ENC:
> -   type = AMD_IP_BLOCK_TYPE_VCN;
> +   case AMD_IP_BLOCK_TYPE_VCN:
> +   count = (info->query_hw_ip.type == 
> AMDGPU_HW_IP_VCN_ENC) ?
> +   adev->vcn.num_vcn_inst * 
> adev->vcn.num_enc_rings :
> +   adev->vcn.num_vcn_inst;

Is this correct?  JPEG has a 1:1 mapping from ring to engine, but I
think VCN is usually multiple rings per engine.  Shouldn't this just
return adev->vcn.num_vcn_inst?

Alex


> break;
> -   case AMDGPU_HW_IP_VCN_JPEG:
> -   type = (amdgpu_device_ip_get_ip_block(adev, 
> AMD_IP_BLOCK_TYPE_JPEG)) ?
> -