RE: [PATCH 1/2] drm/amdgpu: Remove the sriov checking and add firmware checking

2018-08-16 Thread Deng, Emily
>-Original Message-
>From: Christian König 
>Sent: Thursday, August 16, 2018 9:17 PM
>To: Deng, Emily ; amd-gfx@lists.freedesktop.org
>Subject: Re: [PATCH 1/2] drm/amdgpu: Remove the sriov checking and add
>firmware checking
>
>Am 16.08.2018 um 15:05 schrieb Emily Deng:
>> Unify bare metal and sriov, and add firmware checking for reg write
>> and reg wait unify command.
>>
>> Signed-off-by: Emily Deng 
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h |  2 ++
>>   drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c   | 47
>-
>>   2 files changed, 48 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
>> index 53e9e2a..f172e92 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
>> @@ -274,6 +274,8 @@ struct amdgpu_gfx {
>>  uint32_trlc_srls_feature_version;
>>  uint32_tmec_feature_version;
>>  uint32_tmec2_feature_version;
>> +boolmec_fw_write_wait;
>> +boolme_fw_write_wait;
>>  struct amdgpu_ring  gfx_ring[AMDGPU_MAX_GFX_RINGS];
>>  unsignednum_gfx_rings;
>>  struct amdgpu_ring
>   compute_ring[AMDGPU_MAX_COMPUTE_RINGS];
>> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
>> b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
>> index 76d979e..76e8973 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
>> @@ -482,6 +482,47 @@ static void gfx_v9_0_init_rlc_ext_microcode(struct
>amdgpu_device *adev)
>>  le32_to_cpu(rlc_hdr-
>>reg_list_format_direct_reg_list_length);
>>   }
>>
>> +static void gfx_v9_0_check_fw_write_wait(struct amdgpu_device *adev)
>> +{
>> +adev->gfx.me_fw_write_wait = false;
>> +adev->gfx.mec_fw_write_wait = false;
>> +
>> +switch (adev->asic_type) {
>> +case CHIP_VEGA10:
>> +if ((adev->gfx.me_fw_version >= 0x009c) && (adev-
>>gfx.me_feature_version >= 42) &&
>> +(adev->gfx.pfp_fw_version >=  0x00b1) && (adev-
>>gfx.pfp_feature_version >= 42))
>> +adev->gfx.me_fw_write_wait = true;
>
>Well that looks like the correct solution to me. Only a nit pick left to fix, 
>the
>coding style looks a bit off.
>
>For example the "if" should be indented so that "(adev.." in both lines is on 
>the
>same column:
>
>if ((adev ) &&
>     (adev
>
>And the "adev->gfx.me_fw_write_wait = true;" is to far to the right.
>
Thanks, will modify as your suggestion.
>With that fixed the patch is Acked-by: Christian König
>.
>
>Regards,
>Christian.
>
>> +
>> +if ((adev->gfx.mec_fw_version >=  0x0193) && (adev-
>>gfx.mec_feature_version >= 42))
>> +adev->gfx.mec_fw_write_wait = true;
>> +break;
>> +case CHIP_VEGA12:
>> +if ((adev->gfx.me_fw_version >= 0x009c) && (adev-
>>gfx.me_feature_version >= 44) &&
>> +(adev->gfx.pfp_fw_version >=  0x00b2) && (adev-
>>gfx.pfp_feature_version >= 44))
>> +adev->gfx.me_fw_write_wait = true;
>> +
>> +if ((adev->gfx.mec_fw_version >=  0x0196) && (adev-
>>gfx.mec_feature_version >= 44))
>> +adev->gfx.mec_fw_write_wait = true;
>> +break;
>> +case CHIP_VEGA20:
>> +if ((adev->gfx.me_fw_version >= 0x009c) && (adev-
>>gfx.me_feature_version >= 44) &&
>> +(adev->gfx.pfp_fw_version >=  0x00b2) && (adev-
>>gfx.pfp_feature_version >= 44))
>> +adev->gfx.me_fw_write_wait = true;
>> +
>> +if ((adev->gfx.mec_fw_version >=  0x0197) && (adev-
>>gfx.mec_feature_version >= 44))
>> +adev->gfx.mec_fw_write_wait = true;
>> +break;
>> +case CHIP_RAVEN:
>> +if ((adev->gfx.me_fw_version >= 0x009c) && (adev-
>>gfx.me_feature_version >= 42) &&
>> +(adev->gfx

Re: [PATCH 1/2] drm/amdgpu: Remove the sriov checking and add firmware checking

2018-08-16 Thread Christian König

Am 16.08.2018 um 15:05 schrieb Emily Deng:

Unify bare metal and sriov, and add firmware checking for
reg write and reg wait unify command.

Signed-off-by: Emily Deng 
---
  drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h |  2 ++
  drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c   | 47 -
  2 files changed, 48 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
index 53e9e2a..f172e92 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
@@ -274,6 +274,8 @@ struct amdgpu_gfx {
uint32_trlc_srls_feature_version;
uint32_tmec_feature_version;
uint32_tmec2_feature_version;
+   boolmec_fw_write_wait;
+   boolme_fw_write_wait;
struct amdgpu_ring  gfx_ring[AMDGPU_MAX_GFX_RINGS];
unsignednum_gfx_rings;
struct amdgpu_ring  compute_ring[AMDGPU_MAX_COMPUTE_RINGS];
diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c 
b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
index 76d979e..76e8973 100644
--- a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
@@ -482,6 +482,47 @@ static void gfx_v9_0_init_rlc_ext_microcode(struct 
amdgpu_device *adev)

le32_to_cpu(rlc_hdr->reg_list_format_direct_reg_list_length);
  }
  
+static void gfx_v9_0_check_fw_write_wait(struct amdgpu_device *adev)

+{
+   adev->gfx.me_fw_write_wait = false;
+   adev->gfx.mec_fw_write_wait = false;
+
+   switch (adev->asic_type) {
+   case CHIP_VEGA10:
+   if ((adev->gfx.me_fw_version >= 0x009c) && 
(adev->gfx.me_feature_version >= 42) &&
+   (adev->gfx.pfp_fw_version >=  0x00b1) && 
(adev->gfx.pfp_feature_version >= 42))
+   adev->gfx.me_fw_write_wait = true;


Well that looks like the correct solution to me. Only a nit pick left to 
fix, the coding style looks a bit off.


For example the "if" should be indented so that "(adev.." in both lines 
is on the same column:


if ((adev ) &&
    (adev

And the "adev->gfx.me_fw_write_wait = true;" is to far to the right.

With that fixed the patch is Acked-by: Christian König 
.


Regards,
Christian.


+
+   if ((adev->gfx.mec_fw_version >=  0x0193) && 
(adev->gfx.mec_feature_version >= 42))
+   adev->gfx.mec_fw_write_wait = true;
+   break;
+   case CHIP_VEGA12:
+   if ((adev->gfx.me_fw_version >= 0x009c) && 
(adev->gfx.me_feature_version >= 44) &&
+   (adev->gfx.pfp_fw_version >=  0x00b2) && 
(adev->gfx.pfp_feature_version >= 44))
+   adev->gfx.me_fw_write_wait = true;
+
+   if ((adev->gfx.mec_fw_version >=  0x0196) && 
(adev->gfx.mec_feature_version >= 44))
+   adev->gfx.mec_fw_write_wait = true;
+   break;
+   case CHIP_VEGA20:
+   if ((adev->gfx.me_fw_version >= 0x009c) && 
(adev->gfx.me_feature_version >= 44) &&
+   (adev->gfx.pfp_fw_version >=  0x00b2) && 
(adev->gfx.pfp_feature_version >= 44))
+   adev->gfx.me_fw_write_wait = true;
+
+   if ((adev->gfx.mec_fw_version >=  0x0197) && 
(adev->gfx.mec_feature_version >= 44))
+   adev->gfx.mec_fw_write_wait = true;
+   break;
+   case CHIP_RAVEN:
+   if ((adev->gfx.me_fw_version >= 0x009c) && 
(adev->gfx.me_feature_version >= 42) &&
+   (adev->gfx.pfp_fw_version >=  0x00b1) && 
(adev->gfx.pfp_feature_version >= 42))
+   adev->gfx.me_fw_write_wait = true;
+
+   if ((adev->gfx.mec_fw_version >=  0x0192) && 
(adev->gfx.mec_feature_version >= 42))
+   adev->gfx.mec_fw_write_wait = true;
+   break;
+   }
+}
+
  static int gfx_v9_0_init_microcode(struct amdgpu_device *adev)
  {
const char *chip_name;
@@ -716,6 +757,7 @@ static int gfx_v9_0_init_microcode(struct amdgpu_device 
*adev)
}
  
  out:

+   gfx_v9_0_check_fw_write_wait(adev);
if (err) {
dev_err(adev->dev,
"gfx9: Failed to load firmware \"%s\"\n",
@@ -4348,8 +4390,11 @@ static void gfx_v9_0_ring_emit_reg_write_reg_wait(struct 
amdgpu_ring *ring,
  uint32_t ref, uint32_t mask)
  {
int usepfp = (ring->funcs->type == AMDGPU_RING_TYPE_GFX);
+   struct amdgpu_device *adev = ring->adev;
+   bool fw_version_ok = (ring->funcs->type == AMDGPU_RING_TYPE_GFX) ?
+   adev->gfx.me_fw_write_wait : adev->gfx.mec_fw_write_wait;
  
-	if