[1/2] drm/amdgpu: fix re-program vm invalidate eng address range for gfxhub on resume

Submitted by Huang, Ray on May 17, 2017, 7:38 a.m.

Details

Message ID 1495006729-17310-1-git-send-email-ray.huang@amd.com
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in AMD X.Org drivers

Not browsing as part of any series.

Commit Message

Huang, Ray May 17, 2017, 7:38 a.m.
Signed-off-by: Huang Rui <ray.huang@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c b/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c
index 005075f..41313514 100644
--- a/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c
@@ -368,7 +368,7 @@  static int gfxhub_v1_0_suspend(void *handle)
 
 static int gfxhub_v1_0_resume(void *handle)
 {
-	return 0;
+	return gfxhub_v1_0_hw_init(handle);
 }
 
 static bool gfxhub_v1_0_is_idle(void *handle)

Comments

By this change, I suggest to remove mmhub/gfxhub_v1_0_ip_funcs and their 
IP block, unify them to gmc ip block, this way we cannot lost setting 
when resume back.

Regards,
David Zhou

On 2017年05月17日 15:38, Huang Rui wrote:
> Signed-off-by: Huang Rui <ray.huang@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c b/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c
> index 005075f..41313514 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c
> @@ -368,7 +368,7 @@ static int gfxhub_v1_0_suspend(void *handle)
>   
>   static int gfxhub_v1_0_resume(void *handle)
>   {
> -	return 0;
> +	return gfxhub_v1_0_hw_init(handle);
>   }
>   
>   static bool gfxhub_v1_0_is_idle(void *handle)
On Wed, May 17, 2017 at 03:43:47PM +0800, Zhou, David(ChunMing) wrote:
> By this change, I suggest to remove mmhub/gfxhub_v1_0_ip_funcs and their
> IP block, unify them to gmc ip block, this way we cannot lost setting
> when resume back.
> 

From hw side, wo won't have real gmc since this chip, mmhub and gfxhub(gc)
instead of it. Maybe we would better to align with hw desgin.

Thanks,
Rui

> Regards,
> David Zhou
> 
> On 2017年05月17日 15:38, Huang Rui wrote:
> > Signed-off-by: Huang Rui <ray.huang@amd.com>
> > ---
> >   drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c b/drivers/gpu/drm/amd/
> amdgpu/gfxhub_v1_0.c
> > index 005075f..41313514 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c
> > @@ -368,7 +368,7 @@ static int gfxhub_v1_0_suspend(void *handle)
> >  
> >   static int gfxhub_v1_0_resume(void *handle)
> >   {
> > -     return 0;
> > +     return gfxhub_v1_0_hw_init(handle);
> >   }
> >  
> >   static bool gfxhub_v1_0_is_idle(void *handle)
>
On 2017年05月17日 15:55, Huang Rui wrote:
> On Wed, May 17, 2017 at 03:43:47PM +0800, Zhou, David(ChunMing) wrote:
>> By this change, I suggest to remove mmhub/gfxhub_v1_0_ip_funcs and their
>> IP block, unify them to gmc ip block, this way we cannot lost setting
>> when resume back.
>>
>  From hw side, wo won't have real gmc since this chip, mmhub and gfxhub(gc)
> instead of it. Maybe we would better to align with hw desgin.
I don't see any advance, as you said, we still have gmc block in soc15, 
why not unify mmhub/gfxhub calls to gmc block?
We can keep mmhub/gfxhub_xxx.c file, but ip_funciton isn't necessary.

Regards,
David Zhou
>
> Thanks,
> Rui
>
>> Regards,
>> David Zhou
>>
>> On 2017年05月17日 15:38, Huang Rui wrote:
>>> Signed-off-by: Huang Rui <ray.huang@amd.com>
>>> ---
>>>    drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c b/drivers/gpu/drm/amd/
>> amdgpu/gfxhub_v1_0.c
>>> index 005075f..41313514 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c
>>> @@ -368,7 +368,7 @@ static int gfxhub_v1_0_suspend(void *handle)
>>>   
>>>    static int gfxhub_v1_0_resume(void *handle)
>>>    {
>>> -     return 0;
>>> +     return gfxhub_v1_0_hw_init(handle);
>>>    }
>>>   
>>>    static bool gfxhub_v1_0_is_idle(void *handle)
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Am 17.05.2017 um 09:55 schrieb Huang Rui:
> On Wed, May 17, 2017 at 03:43:47PM +0800, Zhou, David(ChunMing) wrote:
>> By this change, I suggest to remove mmhub/gfxhub_v1_0_ip_funcs and their
>> IP block, unify them to gmc ip block, this way we cannot lost setting
>> when resume back.
>>
>  From hw side, wo won't have real gmc since this chip, mmhub and gfxhub(gc)
> instead of it. Maybe we would better to align with hw desgin.

Yeah, I had patches for doing exactly this but then decided dropping it.

What we should do is to have any one implementation of the code handling 
both hubs and then have both as separate IP block in the list.

Anyway for now both patches are Reviewed-by: Christian König 
<christian.koenig@amd.com>.

I assume that finally fixes S3 on Vega10?

Regards,
Christian.

>
> Thanks,
> Rui
>
>> Regards,
>> David Zhou
>>
>> On 2017年05月17日 15:38, Huang Rui wrote:
>>> Signed-off-by: Huang Rui <ray.huang@amd.com>
>>> ---
>>>    drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c b/drivers/gpu/drm/amd/
>> amdgpu/gfxhub_v1_0.c
>>> index 005075f..41313514 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c
>>> @@ -368,7 +368,7 @@ static int gfxhub_v1_0_suspend(void *handle)
>>>   
>>>    static int gfxhub_v1_0_resume(void *handle)
>>>    {
>>> -     return 0;
>>> +     return gfxhub_v1_0_hw_init(handle);
>>>    }
>>>   
>>>    static bool gfxhub_v1_0_is_idle(void *handle)
see here, I already find dis-advance:
enum amd_ip_block_type {
         AMD_IP_BLOCK_TYPE_COMMON,
         AMD_IP_BLOCK_TYPE_GMC,
         AMD_IP_BLOCK_TYPE_IH,
         AMD_IP_BLOCK_TYPE_SMC,
         AMD_IP_BLOCK_TYPE_PSP,
         AMD_IP_BLOCK_TYPE_DCE,
         AMD_IP_BLOCK_TYPE_GFX,
         AMD_IP_BLOCK_TYPE_SDMA,
         AMD_IP_BLOCK_TYPE_UVD,
         AMD_IP_BLOCK_TYPE_VCE,
         AMD_IP_BLOCK_TYPE_ACP,
         AMD_IP_BLOCK_TYPE_GFXHUB,
         AMD_IP_BLOCK_TYPE_MMHUB
};
resume will follow this sequence.
but initial sequence is :
               amdgpu_ip_block_add(adev, &vega10_common_ip_block);
                 amdgpu_ip_block_add(adev, &gfxhub_v1_0_ip_block);
                 amdgpu_ip_block_add(adev, &mmhub_v1_0_ip_block);
                 amdgpu_ip_block_add(adev, &gmc_v9_0_ip_block);
                 amdgpu_ip_block_add(adev, &vega10_ih_ip_block);
                 if (amdgpu_fw_load_type == 2 || amdgpu_fw_load_type == -1)
                         amdgpu_ip_block_add(adev, &psp_v3_1_ip_block);
                 if (!amdgpu_sriov_vf(adev))
                         amdgpu_ip_block_add(adev, &amdgpu_pp_ip_block);
                 if (adev->enable_virtual_display || amdgpu_sriov_vf(adev))
                         amdgpu_ip_block_add(adev, &dce_virtual_ip_block);
#if defined(CONFIG_DRM_AMD_DC)
                 else if (amdgpu_device_has_dc_support(adev))
                         amdgpu_ip_block_add(adev, &dm_ip_block);
#else
#       warning "Enable CONFIG_DRM_AMD_DC for display support on SOC15."
#endif
                 amdgpu_ip_block_add(adev, &gfx_v9_0_ip_block);
                 amdgpu_ip_block_add(adev, &sdma_v4_0_ip_block);
                 amdgpu_ip_block_add(adev, &uvd_v7_0_ip_block);
                 amdgpu_ip_block_add(adev, &vce_v4_0_ip_block);


They are different. I remember I asked you if they are same, don't know 
why you answer 'yes'.

With s3 problem still in there, please do this improvement asap.

Regards,
David Zhou

On 2017年05月17日 16:11, zhoucm1 wrote:
>
>
> On 2017年05月17日 15:55, Huang Rui wrote:
>> On Wed, May 17, 2017 at 03:43:47PM +0800, Zhou, David(ChunMing) wrote:
>>> By this change, I suggest to remove mmhub/gfxhub_v1_0_ip_funcs and 
>>> their
>>> IP block, unify them to gmc ip block, this way we cannot lost setting
>>> when resume back.
>>>
>>  From hw side, wo won't have real gmc since this chip, mmhub and 
>> gfxhub(gc)
>> instead of it. Maybe we would better to align with hw desgin.
> I don't see any advance, as you said, we still have gmc block in 
> soc15, why not unify mmhub/gfxhub calls to gmc block?
> We can keep mmhub/gfxhub_xxx.c file, but ip_funciton isn't necessary.
>
> Regards,
> David Zhou
>>
>> Thanks,
>> Rui
>>
>>> Regards,
>>> David Zhou
>>>
>>> On 2017年05月17日 15:38, Huang Rui wrote:
>>>> Signed-off-by: Huang Rui <ray.huang@amd.com>
>>>> ---
>>>>    drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c 
>>>> b/drivers/gpu/drm/amd/
>>> amdgpu/gfxhub_v1_0.c
>>>> index 005075f..41313514 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c
>>>> @@ -368,7 +368,7 @@ static int gfxhub_v1_0_suspend(void *handle)
>>>>      static int gfxhub_v1_0_resume(void *handle)
>>>>    {
>>>> -     return 0;
>>>> +     return gfxhub_v1_0_hw_init(handle);
>>>>    }
>>>>      static bool gfxhub_v1_0_is_idle(void *handle)
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Am 17.05.2017 um 10:11 schrieb zhoucm1:
>
>
> On 2017年05月17日 15:55, Huang Rui wrote:
>> On Wed, May 17, 2017 at 03:43:47PM +0800, Zhou, David(ChunMing) wrote:
>>> By this change, I suggest to remove mmhub/gfxhub_v1_0_ip_funcs and 
>>> their
>>> IP block, unify them to gmc ip block, this way we cannot lost setting
>>> when resume back.
>>>
>>  From hw side, wo won't have real gmc since this chip, mmhub and 
>> gfxhub(gc)
>> instead of it. Maybe we would better to align with hw desgin.
> I don't see any advance, as you said, we still have gmc block in 
> soc15, why not unify mmhub/gfxhub calls to gmc block?
> We can keep mmhub/gfxhub_xxx.c file, but ip_funciton isn't necessary.

Well they are two hardware block, but are mostly identical programmed 
(the MMHUB has a few more bits for guaranteed bandwith, but we ignore 
those at the moment).

So it doesn't make much sense having two separate code instances to 
handle them.

One major problem still remaining is that our generated register headers 
are crap for this. You can't for example include both headers at the 
same time.

Regards,
Christian.

>
> Regards,
> David Zhou
>>
>> Thanks,
>> Rui
>>
>>> Regards,
>>> David Zhou
>>>
>>> On 2017年05月17日 15:38, Huang Rui wrote:
>>>> Signed-off-by: Huang Rui <ray.huang@amd.com>
>>>> ---
>>>>    drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c 
>>>> b/drivers/gpu/drm/amd/
>>> amdgpu/gfxhub_v1_0.c
>>>> index 005075f..41313514 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c
>>>> @@ -368,7 +368,7 @@ static int gfxhub_v1_0_suspend(void *handle)
>>>>      static int gfxhub_v1_0_resume(void *handle)
>>>>    {
>>>> -     return 0;
>>>> +     return gfxhub_v1_0_hw_init(handle);
>>>>    }
>>>>      static bool gfxhub_v1_0_is_idle(void *handle)
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>
On 2017年05月17日 16:19, Christian König wrote:
> Am 17.05.2017 um 10:11 schrieb zhoucm1:
>>
>>
>> On 2017年05月17日 15:55, Huang Rui wrote:
>>> On Wed, May 17, 2017 at 03:43:47PM +0800, Zhou, David(ChunMing) wrote:
>>>> By this change, I suggest to remove mmhub/gfxhub_v1_0_ip_funcs and 
>>>> their
>>>> IP block, unify them to gmc ip block, this way we cannot lost setting
>>>> when resume back.
>>>>
>>>  From hw side, wo won't have real gmc since this chip, mmhub and 
>>> gfxhub(gc)
>>> instead of it. Maybe we would better to align with hw desgin.
>> I don't see any advance, as you said, we still have gmc block in 
>> soc15, why not unify mmhub/gfxhub calls to gmc block?
>> We can keep mmhub/gfxhub_xxx.c file, but ip_funciton isn't necessary.
>
> Well they are two hardware block, but are mostly identical programmed 
> (the MMHUB has a few more bits for guaranteed bandwith, but we ignore 
> those at the moment).
>
> So it doesn't make much sense having two separate code instances to 
> handle them.
>
> One major problem still remaining is that our generated register 
> headers are crap for this. You can't for example include both headers 
> at the same time.
As I said before, we can keep these two seperate files, just remove 
ip_functions, but call them from gmc file.
like gmc_v9_init()
{
     gfxhub_init();
     mmhub_init();
}

Regards,
David Zhou
>
> Regards,
> Christian.
>
>>
>> Regards,
>> David Zhou
>>>
>>> Thanks,
>>> Rui
>>>
>>>> Regards,
>>>> David Zhou
>>>>
>>>> On 2017年05月17日 15:38, Huang Rui wrote:
>>>>> Signed-off-by: Huang Rui <ray.huang@amd.com>
>>>>> ---
>>>>>    drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c | 2 +-
>>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c 
>>>>> b/drivers/gpu/drm/amd/
>>>> amdgpu/gfxhub_v1_0.c
>>>>> index 005075f..41313514 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c
>>>>> @@ -368,7 +368,7 @@ static int gfxhub_v1_0_suspend(void *handle)
>>>>>      static int gfxhub_v1_0_resume(void *handle)
>>>>>    {
>>>>> -     return 0;
>>>>> +     return gfxhub_v1_0_hw_init(handle);
>>>>>    }
>>>>>      static bool gfxhub_v1_0_is_idle(void *handle)
>>> _______________________________________________
>>> amd-gfx mailing list
>>> amd-gfx@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>
>
On Wed, May 17, 2017 at 04:16:31PM +0800, Koenig, Christian wrote:
> Am 17.05.2017 um 09:55 schrieb Huang Rui:
> > On Wed, May 17, 2017 at 03:43:47PM +0800, Zhou, David(ChunMing) wrote:
> >> By this change, I suggest to remove mmhub/gfxhub_v1_0_ip_funcs and their
> >> IP block, unify them to gmc ip block, this way we cannot lost setting
> >> when resume back.
> >>
> >  From hw side, wo won't have real gmc since this chip, mmhub and gfxhub(gc)
> > instead of it. Maybe we would better to align with hw desgin.
> 
> Yeah, I had patches for doing exactly this but then decided dropping it.
> 
> What we should do is to have any one implementation of the code handling
> both hubs and then have both as separate IP block in the list.
> 
> Anyway for now both patches are Reviewed-by: Christian König
> <christian.koenig@amd.com>.
> 
> I assume that finally fixes S3 on Vega10?
> 

No, but these fixes have improvement for the result.
We can pass a round of gfx/cp/sdma test.

But if we try to test a round again, it will encounter VM fault.
Will tell you the detailed in another thread.

Thanks,
Rui
Am 17.05.2017 um 10:22 schrieb zhoucm1:
>
>
> On 2017年05月17日 16:19, Christian König wrote:
>> Am 17.05.2017 um 10:11 schrieb zhoucm1:
>>>
>>>
>>> On 2017年05月17日 15:55, Huang Rui wrote:
>>>> On Wed, May 17, 2017 at 03:43:47PM +0800, Zhou, David(ChunMing) wrote:
>>>>> By this change, I suggest to remove mmhub/gfxhub_v1_0_ip_funcs and 
>>>>> their
>>>>> IP block, unify them to gmc ip block, this way we cannot lost setting
>>>>> when resume back.
>>>>>
>>>>  From hw side, wo won't have real gmc since this chip, mmhub and 
>>>> gfxhub(gc)
>>>> instead of it. Maybe we would better to align with hw desgin.
>>> I don't see any advance, as you said, we still have gmc block in 
>>> soc15, why not unify mmhub/gfxhub calls to gmc block?
>>> We can keep mmhub/gfxhub_xxx.c file, but ip_funciton isn't necessary.
>>
>> Well they are two hardware block, but are mostly identical programmed 
>> (the MMHUB has a few more bits for guaranteed bandwith, but we ignore 
>> those at the moment).
>>
>> So it doesn't make much sense having two separate code instances to 
>> handle them.
>>
>> One major problem still remaining is that our generated register 
>> headers are crap for this. You can't for example include both headers 
>> at the same time.
> As I said before, we can keep these two seperate files, just remove 
> ip_functions, but call them from gmc file.
> like gmc_v9_init()
> {
>     gfxhub_init();
>     mmhub_init();
> }

The init order isn't so critical here. My concern is rather that we have 
the duplicated code.

But I agree that we sooner or later should clean up the init order on 
load/resume to be the same.

Regards,
Christian.

>
> Regards,
> David Zhou
>>
>> Regards,
>> Christian.
>>
>>>
>>> Regards,
>>> David Zhou
>>>>
>>>> Thanks,
>>>> Rui
>>>>
>>>>> Regards,
>>>>> David Zhou
>>>>>
>>>>> On 2017年05月17日 15:38, Huang Rui wrote:
>>>>>> Signed-off-by: Huang Rui <ray.huang@amd.com>
>>>>>> ---
>>>>>>    drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c | 2 +-
>>>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c 
>>>>>> b/drivers/gpu/drm/amd/
>>>>> amdgpu/gfxhub_v1_0.c
>>>>>> index 005075f..41313514 100644
>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c
>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c
>>>>>> @@ -368,7 +368,7 @@ static int gfxhub_v1_0_suspend(void *handle)
>>>>>>      static int gfxhub_v1_0_resume(void *handle)
>>>>>>    {
>>>>>> -     return 0;
>>>>>> +     return gfxhub_v1_0_hw_init(handle);
>>>>>>    }
>>>>>>      static bool gfxhub_v1_0_is_idle(void *handle)
>>>> _______________________________________________
>>>> amd-gfx mailing list
>>>> amd-gfx@lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>>
>>
>