[PATCH] drm/amdgpu: set bypass mode when uvd is idle.

[PATCH] drm/amdgpu: set bypass mode when uvd is idle.

[Rex Zhu]

Change-Id: If44f8e91d14f5ec7d4067691684866ef8d77724a
Signed-off-by: Rex Zhu <Rex.Zhu@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/uvd_v5_0.c | 3 ++-
 drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/uvd_v5_0.c b/drivers/gpu/drm/amd/amdgpu/uvd_v5_0.c
index 95303e2..0a6a0e7 100644
--- a/drivers/gpu/drm/amd/amdgpu/uvd_v5_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/uvd_v5_0.c
@@ -745,7 +745,8 @@ static int uvd_v5_0_set_clockgating_state(void *handle,
 	bool enable = (state == AMD_CG_STATE_GATE) ? true : false;
 	static int curstate = -1;
 
-	uvd_v5_0_set_bypass_mode(adev, enable);
+	if (!(adev->pg_flags & AMD_PG_SUPPORT_UVD))
+		uvd_v5_0_set_bypass_mode(adev, !enable);
 
 	if (!(adev->cg_flags & AMD_CG_SUPPORT_UVD_MGCG))
 		return 0;
diff --git a/drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c b/drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c
index a339b5c..b64829fe 100644
--- a/drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c
@@ -955,7 +955,8 @@ static int uvd_v6_0_set_clockgating_state(void *handle,
 	struct amdgpu_device *adev = (struct amdgpu_device *)handle;
 	bool enable = (state == AMD_CG_STATE_GATE) ? true : false;
 
-	uvd_v6_0_set_bypass_mode(adev, enable);
+	if (!(adev->pg_flags & AMD_PG_SUPPORT_UVD))
+		uvd_v6_0_set_bypass_mode(adev, !enable);
 
 	if (!(adev->cg_flags & AMD_CG_SUPPORT_UVD_MGCG))
 		return 0;
-- 
1.9.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu: set bypass mode when uvd is idle.

[Christian König]

Am 03.11.2016 um 09:14 schrieb Rex Zhu:
> Change-Id: If44f8e91d14f5ec7d4067691684866ef8d77724a
> Signed-off-by: Rex Zhu <Rex.Zhu@amd.com>

Reviewed-by: Christian König <christian.koenig@amd.com>.

> ---
>   drivers/gpu/drm/amd/amdgpu/uvd_v5_0.c | 3 ++-
>   drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c | 3 ++-
>   2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/uvd_v5_0.c b/drivers/gpu/drm/amd/amdgpu/uvd_v5_0.c
> index 95303e2..0a6a0e7 100644
> --- a/drivers/gpu/drm/amd/amdgpu/uvd_v5_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/uvd_v5_0.c
> @@ -745,7 +745,8 @@ static int uvd_v5_0_set_clockgating_state(void *handle,
>   	bool enable = (state == AMD_CG_STATE_GATE) ? true : false;
>   	static int curstate = -1;
>   
> -	uvd_v5_0_set_bypass_mode(adev, enable);
> +	if (!(adev->pg_flags & AMD_PG_SUPPORT_UVD))
> +		uvd_v5_0_set_bypass_mode(adev, !enable);
>   
>   	if (!(adev->cg_flags & AMD_CG_SUPPORT_UVD_MGCG))
>   		return 0;
> diff --git a/drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c b/drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c
> index a339b5c..b64829fe 100644
> --- a/drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c
> @@ -955,7 +955,8 @@ static int uvd_v6_0_set_clockgating_state(void *handle,
>   	struct amdgpu_device *adev = (struct amdgpu_device *)handle;
>   	bool enable = (state == AMD_CG_STATE_GATE) ? true : false;
>   
> -	uvd_v6_0_set_bypass_mode(adev, enable);
> +	if (!(adev->pg_flags & AMD_PG_SUPPORT_UVD))
> +		uvd_v6_0_set_bypass_mode(adev, !enable);
>   
>   	if (!(adev->cg_flags & AMD_CG_SUPPORT_UVD_MGCG))
>   		return 0;


_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH] drm/amdgpu: set bypass mode when uvd is idle.

[Deucher, Alexander]

> -----Original Message-----
> From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On Behalf
> Of Rex Zhu
> Sent: Thursday, November 03, 2016 4:14 AM
> To: amd-gfx@lists.freedesktop.org
> Cc: Zhu, Rex
> Subject: [PATCH] drm/amdgpu: set bypass mode when uvd is idle.
> 
> Change-Id: If44f8e91d14f5ec7d4067691684866ef8d77724a
> Signed-off-by: Rex Zhu <Rex.Zhu@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/uvd_v5_0.c | 3 ++-
>  drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c | 3 ++-
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/uvd_v5_0.c
> b/drivers/gpu/drm/amd/amdgpu/uvd_v5_0.c
> index 95303e2..0a6a0e7 100644
> --- a/drivers/gpu/drm/amd/amdgpu/uvd_v5_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/uvd_v5_0.c
> @@ -745,7 +745,8 @@ static int uvd_v5_0_set_clockgating_state(void
> *handle,
>  	bool enable = (state == AMD_CG_STATE_GATE) ? true : false;
>  	static int curstate = -1;
> 
> -	uvd_v5_0_set_bypass_mode(adev, enable);
> +	if (!(adev->pg_flags & AMD_PG_SUPPORT_UVD))
> +		uvd_v5_0_set_bypass_mode(adev, !enable);

Is there any harm in just always putting it into bypass mode or does it interact badly with PG?  Presumably it does (otherwise we wouldn't need this patch), it would be good to note why.

Alex

> 
>  	if (!(adev->cg_flags & AMD_CG_SUPPORT_UVD_MGCG))
>  		return 0;
> diff --git a/drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c
> b/drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c
> index a339b5c..b64829fe 100644
> --- a/drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c
> @@ -955,7 +955,8 @@ static int uvd_v6_0_set_clockgating_state(void
> *handle,
>  	struct amdgpu_device *adev = (struct amdgpu_device *)handle;
>  	bool enable = (state == AMD_CG_STATE_GATE) ? true : false;
> 
> -	uvd_v6_0_set_bypass_mode(adev, enable);
> +	if (!(adev->pg_flags & AMD_PG_SUPPORT_UVD))
> +		uvd_v6_0_set_bypass_mode(adev, !enable);
> 
>  	if (!(adev->cg_flags & AMD_CG_SUPPORT_UVD_MGCG))
>  		return 0;
> --
> 1.9.1
> 
> _______________________________________________
> 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

RE: [PATCH] drm/amdgpu: set bypass mode when uvd is idle.

[Zhu, Rex]

>>>Is there any harm in just always putting it into bypass mode or does it interact badly with PG?  Presumably it does (otherwise we wouldn't need this patch), it would be good to  
>>>note why.

Rex: when UVD PG enabled, DCLK/VCLK will be turn off when uvd is idle(DCLK=OFF). If we set bypass mode=1, dclk/vclk will be bypassed to an external ‘Bypass’ clock(DCLK = 100MHz)

So it is unnecessary to set bypass mode when PG enabled.

+uvd_v5_0_set_bypass_mode(adev, !enable);
This change is because tom's commit 72cb64c1f6a3a8129af341e90418a687c4971a40
Fix the sequence of UVD powergate function in smu7_clockgating.c.




Best Regards
Rex

-----Original Message-----
From: Deucher, Alexander 
Sent: Thursday, November 03, 2016 10:21 PM
To: Zhu, Rex; amd-gfx@lists.freedesktop.org
Cc: Zhu, Rex
Subject: RE: [PATCH] drm/amdgpu: set bypass mode when uvd is idle.

> -----Original Message-----
> From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On Behalf 
> Of Rex Zhu
> Sent: Thursday, November 03, 2016 4:14 AM
> To: amd-gfx@lists.freedesktop.org
> Cc: Zhu, Rex
> Subject: [PATCH] drm/amdgpu: set bypass mode when uvd is idle.
> 
> Change-Id: If44f8e91d14f5ec7d4067691684866ef8d77724a
> Signed-off-by: Rex Zhu <Rex.Zhu@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/uvd_v5_0.c | 3 ++-  
> drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c | 3 ++-
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/uvd_v5_0.c
> b/drivers/gpu/drm/amd/amdgpu/uvd_v5_0.c
> index 95303e2..0a6a0e7 100644
> --- a/drivers/gpu/drm/amd/amdgpu/uvd_v5_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/uvd_v5_0.c
> @@ -745,7 +745,8 @@ static int uvd_v5_0_set_clockgating_state(void
> *handle,
>  	bool enable = (state == AMD_CG_STATE_GATE) ? true : false;
>  	static int curstate = -1;
> 
> -	uvd_v5_0_set_bypass_mode(adev, enable);
> +	if (!(adev->pg_flags & AMD_PG_SUPPORT_UVD))
> +		uvd_v5_0_set_bypass_mode(adev, !enable);

Is there any harm in just always putting it into bypass mode or does it interact badly with PG?  Presumably it does (otherwise we wouldn't need this patch), it would be good to note why.

Alex

> 
>  	if (!(adev->cg_flags & AMD_CG_SUPPORT_UVD_MGCG))
>  		return 0;
> diff --git a/drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c
> b/drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c
> index a339b5c..b64829fe 100644
> --- a/drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c
> @@ -955,7 +955,8 @@ static int uvd_v6_0_set_clockgating_state(void
> *handle,
>  	struct amdgpu_device *adev = (struct amdgpu_device *)handle;
>  	bool enable = (state == AMD_CG_STATE_GATE) ? true : false;
> 
> -	uvd_v6_0_set_bypass_mode(adev, enable);
> +	if (!(adev->pg_flags & AMD_PG_SUPPORT_UVD))
> +		uvd_v6_0_set_bypass_mode(adev, !enable);
> 
>  	if (!(adev->cg_flags & AMD_CG_SUPPORT_UVD_MGCG))
>  		return 0;
> --
> 1.9.1
> 
> _______________________________________________
> 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

RE: [PATCH] drm/amdgpu: set bypass mode when uvd is idle.

[Deucher, Alexander]

> -----Original Message-----
> From: Zhu, Rex
> Sent: Thursday, November 03, 2016 11:18 PM
> To: Deucher, Alexander; amd-gfx@lists.freedesktop.org
> Subject: RE: [PATCH] drm/amdgpu: set bypass mode when uvd is idle.
> 
> >>>Is there any harm in just always putting it into bypass mode or does it
> interact badly with PG?  Presumably it does (otherwise we wouldn't need
> this patch), it would be good to
> >>>note why.
> 
> Rex: when UVD PG enabled, DCLK/VCLK will be turn off when uvd is
> idle(DCLK=OFF). If we set bypass mode=1, dclk/vclk will be bypassed to an
> external ‘Bypass’ clock(DCLK = 100MHz)
> 
> So it is unnecessary to set bypass mode when PG enabled.
> 
> +uvd_v5_0_set_bypass_mode(adev, !enable);
> This change is because tom's commit
> 72cb64c1f6a3a8129af341e90418a687c4971a40
> Fix the sequence of UVD powergate function in smu7_clockgating.c.
> 

Thanks for clarifying.  Can you update the commit message to include these details?  With that:
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>

> 
> 
> 
> Best Regards
> Rex
> 
> -----Original Message-----
> From: Deucher, Alexander
> Sent: Thursday, November 03, 2016 10:21 PM
> To: Zhu, Rex; amd-gfx@lists.freedesktop.org
> Cc: Zhu, Rex
> Subject: RE: [PATCH] drm/amdgpu: set bypass mode when uvd is idle.
> 
> > -----Original Message-----
> > From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On Behalf
> > Of Rex Zhu
> > Sent: Thursday, November 03, 2016 4:14 AM
> > To: amd-gfx@lists.freedesktop.org
> > Cc: Zhu, Rex
> > Subject: [PATCH] drm/amdgpu: set bypass mode when uvd is idle.
> >
> > Change-Id: If44f8e91d14f5ec7d4067691684866ef8d77724a
> > Signed-off-by: Rex Zhu <Rex.Zhu@amd.com>
> > ---
> >  drivers/gpu/drm/amd/amdgpu/uvd_v5_0.c | 3 ++-
> > drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c | 3 ++-
> >  2 files changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/uvd_v5_0.c
> > b/drivers/gpu/drm/amd/amdgpu/uvd_v5_0.c
> > index 95303e2..0a6a0e7 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/uvd_v5_0.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/uvd_v5_0.c
> > @@ -745,7 +745,8 @@ static int uvd_v5_0_set_clockgating_state(void
> > *handle,
> >  	bool enable = (state == AMD_CG_STATE_GATE) ? true : false;
> >  	static int curstate = -1;
> >
> > -	uvd_v5_0_set_bypass_mode(adev, enable);
> > +	if (!(adev->pg_flags & AMD_PG_SUPPORT_UVD))
> > +		uvd_v5_0_set_bypass_mode(adev, !enable);
> 
> Is there any harm in just always putting it into bypass mode or does it interact
> badly with PG?  Presumably it does (otherwise we wouldn't need this patch),
> it would be good to note why.
> 
> Alex
> 
> >
> >  	if (!(adev->cg_flags & AMD_CG_SUPPORT_UVD_MGCG))
> >  		return 0;
> > diff --git a/drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c
> > b/drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c
> > index a339b5c..b64829fe 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c
> > @@ -955,7 +955,8 @@ static int uvd_v6_0_set_clockgating_state(void
> > *handle,
> >  	struct amdgpu_device *adev = (struct amdgpu_device *)handle;
> >  	bool enable = (state == AMD_CG_STATE_GATE) ? true : false;
> >
> > -	uvd_v6_0_set_bypass_mode(adev, enable);
> > +	if (!(adev->pg_flags & AMD_PG_SUPPORT_UVD))
> > +		uvd_v6_0_set_bypass_mode(adev, !enable);
> >
> >  	if (!(adev->cg_flags & AMD_CG_SUPPORT_UVD_MGCG))
> >  		return 0;
> > --
> > 1.9.1
> >
> > _______________________________________________
> > 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

Re: [PATCH] drm/amdgpu: set bypass mode when uvd is idle.

[Andy Furniss]

Zhu, Rex wrote:
>>>> Is there any harm in just always putting it into bypass mode or
>>>> does it interact badly with PG?  Presumably it does (otherwise
>>>> we wouldn't need this patch), it would be good to note why.
>
> Rex: when UVD PG enabled, DCLK/VCLK will be turn off when uvd is
> idle(DCLK=OFF). If we set bypass mode=1, dclk/vclk will be bypassed
> to an external ‘Bypass’ clock(DCLK = 100MHz)
>
> So it is unnecessary to set bypass mode when PG enabled.
>
> +uvd_v5_0_set_bypass_mode(adev, !enable); This change is because
> tom's commit 72cb64c1f6a3a8129af341e90418a687c4971a40 Fix the
> sequence of UVD powergate function in smu7_clockgating.c.

I was about to file a bug till I tried this which fixes UVD perf
on my R9285 + agd5f drm-next-4.10-wip.

Additional unrelated question = I notice that UVD does not seem
to set other clocks quite high enough when used.

For playback the vo may bump things up a bit, but even then it can be a bit
borderline for playing high bitrate UHD with powerplay on auto.

Pure decode benchmarks like

ffmpeg -hwaccel vdpau -i high-bitrate-2160p60-vid -pix_fmt nv12 -f null -

go from 63 -> 81 fps, powerplay auto -> high.
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH] drm/amdgpu: set bypass mode when uvd is idle.

[Zhu, Rex]

[-- Attachment #1: Type: text/plain, Size: 4125 bytes --]

Hi Alex,

We don't need to set bypass mode for uvd.
For the issue uvd clock stay in high clock when uvd is idle, just need to set the default uvd clock to 100MHz when initialize.

Please Review the attached patch.

Best Regards
Rex

-----Original Message-----
From: Deucher, Alexander 
Sent: Friday, November 04, 2016 9:43 PM
To: Zhu, Rex; amd-gfx@lists.freedesktop.org
Subject: RE: [PATCH] drm/amdgpu: set bypass mode when uvd is idle.

> -----Original Message-----
> From: Zhu, Rex
> Sent: Thursday, November 03, 2016 11:18 PM
> To: Deucher, Alexander; amd-gfx@lists.freedesktop.org
> Subject: RE: [PATCH] drm/amdgpu: set bypass mode when uvd is idle.
> 
> >>>Is there any harm in just always putting it into bypass mode or 
> >>>does it
> interact badly with PG?  Presumably it does (otherwise we wouldn't 
> need this patch), it would be good to
> >>>note why.
> 
> Rex: when UVD PG enabled, DCLK/VCLK will be turn off when uvd is 
> idle(DCLK=OFF). If we set bypass mode=1, dclk/vclk will be bypassed to 
> an external ‘Bypass’ clock(DCLK = 100MHz)
> 
> So it is unnecessary to set bypass mode when PG enabled.
> 
> +uvd_v5_0_set_bypass_mode(adev, !enable);
> This change is because tom's commit
> 72cb64c1f6a3a8129af341e90418a687c4971a40
> Fix the sequence of UVD powergate function in smu7_clockgating.c.
> 

Thanks for clarifying.  Can you update the commit message to include these details?  With that:
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>

> 
> 
> 
> Best Regards
> Rex
> 
> -----Original Message-----
> From: Deucher, Alexander
> Sent: Thursday, November 03, 2016 10:21 PM
> To: Zhu, Rex; amd-gfx@lists.freedesktop.org
> Cc: Zhu, Rex
> Subject: RE: [PATCH] drm/amdgpu: set bypass mode when uvd is idle.
> 
> > -----Original Message-----
> > From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On 
> > Behalf Of Rex Zhu
> > Sent: Thursday, November 03, 2016 4:14 AM
> > To: amd-gfx@lists.freedesktop.org
> > Cc: Zhu, Rex
> > Subject: [PATCH] drm/amdgpu: set bypass mode when uvd is idle.
> >
> > Change-Id: If44f8e91d14f5ec7d4067691684866ef8d77724a
> > Signed-off-by: Rex Zhu <Rex.Zhu@amd.com>
> > ---
> >  drivers/gpu/drm/amd/amdgpu/uvd_v5_0.c | 3 ++- 
> > drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c | 3 ++-
> >  2 files changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/uvd_v5_0.c
> > b/drivers/gpu/drm/amd/amdgpu/uvd_v5_0.c
> > index 95303e2..0a6a0e7 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/uvd_v5_0.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/uvd_v5_0.c
> > @@ -745,7 +745,8 @@ static int uvd_v5_0_set_clockgating_state(void
> > *handle,
> >  	bool enable = (state == AMD_CG_STATE_GATE) ? true : false;
> >  	static int curstate = -1;
> >
> > -	uvd_v5_0_set_bypass_mode(adev, enable);
> > +	if (!(adev->pg_flags & AMD_PG_SUPPORT_UVD))
> > +		uvd_v5_0_set_bypass_mode(adev, !enable);
> 
> Is there any harm in just always putting it into bypass mode or does 
> it interact badly with PG?  Presumably it does (otherwise we wouldn't 
> need this patch), it would be good to note why.
> 
> Alex
> 
> >
> >  	if (!(adev->cg_flags & AMD_CG_SUPPORT_UVD_MGCG))
> >  		return 0;
> > diff --git a/drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c
> > b/drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c
> > index a339b5c..b64829fe 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c
> > @@ -955,7 +955,8 @@ static int uvd_v6_0_set_clockgating_state(void
> > *handle,
> >  	struct amdgpu_device *adev = (struct amdgpu_device *)handle;
> >  	bool enable = (state == AMD_CG_STATE_GATE) ? true : false;
> >
> > -	uvd_v6_0_set_bypass_mode(adev, enable);
> > +	if (!(adev->pg_flags & AMD_PG_SUPPORT_UVD))
> > +		uvd_v6_0_set_bypass_mode(adev, !enable);
> >
> >  	if (!(adev->cg_flags & AMD_CG_SUPPORT_UVD_MGCG))
> >  		return 0;
> > --
> > 1.9.1
> >
> > _______________________________________________
> > amd-gfx mailing list
> > amd-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[-- Attachment #2: 0001-drm-amdgpu-not-need-to-set-bypassmode-for-uvd5.0-uvd.patch --]
[-- Type: application/octet-stream, Size: 2808 bytes --]

From db61c9490cab56da27518f1cf8537d10b9310e88 Mon Sep 17 00:00:00 2001
From: Rex Zhu <Rex.Zhu@amd.com>
Date: Mon, 7 Nov 2016 15:52:45 +0800
Subject: [PATCH] drm/amdgpu: not need to set bypassmode for uvd5.0/uvd6.0

Change-Id: I1bc4659394de881f7ad7a6ed4b66c1fb7f153726
Signed-off-by: Rex Zhu <Rex.Zhu@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/uvd_v5_0.c | 15 ---------------
 drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c | 18 ++----------------
 2 files changed, 2 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/uvd_v5_0.c b/drivers/gpu/drm/amd/amdgpu/uvd_v5_0.c
index 95303e2..dadb6ab 100644
--- a/drivers/gpu/drm/amd/amdgpu/uvd_v5_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/uvd_v5_0.c
@@ -724,19 +724,6 @@ static void uvd_v5_0_set_hw_clock_gating(struct amdgpu_device *adev)
 }
 #endif
 
-static void uvd_v5_0_set_bypass_mode(struct amdgpu_device *adev, bool enable)
-{
-	u32 tmp = RREG32_SMC(ixGCK_DFS_BYPASS_CNTL);
-
-	if (enable)
-		tmp |= (GCK_DFS_BYPASS_CNTL__BYPASSDCLK_MASK |
-			GCK_DFS_BYPASS_CNTL__BYPASSVCLK_MASK);
-	else
-		tmp &= ~(GCK_DFS_BYPASS_CNTL__BYPASSDCLK_MASK |
-			 GCK_DFS_BYPASS_CNTL__BYPASSVCLK_MASK);
-
-	WREG32_SMC(ixGCK_DFS_BYPASS_CNTL, tmp);
-}
 
 static int uvd_v5_0_set_clockgating_state(void *handle,
 					  enum amd_clockgating_state state)
@@ -745,8 +732,6 @@ static int uvd_v5_0_set_clockgating_state(void *handle,
 	bool enable = (state == AMD_CG_STATE_GATE) ? true : false;
 	static int curstate = -1;
 
-	uvd_v5_0_set_bypass_mode(adev, enable);
-
 	if (!(adev->cg_flags & AMD_CG_SUPPORT_UVD_MGCG))
 		return 0;
 
diff --git a/drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c b/drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c
index a339b5c..00fad69 100644
--- a/drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c
@@ -151,6 +151,8 @@ static int uvd_v6_0_hw_init(void *handle)
 	uint32_t tmp;
 	int r;
 
+	amdgpu_asic_set_uvd_clocks(adev, 10000, 10000);
+
 	r = uvd_v6_0_start(adev);
 	if (r)
 		goto done;
@@ -935,28 +937,12 @@ static void uvd_v6_0_set_hw_clock_gating(struct amdgpu_device *adev)
 }
 #endif
 
-static void uvd_v6_0_set_bypass_mode(struct amdgpu_device *adev, bool enable)
-{
-	u32 tmp = RREG32_SMC(ixGCK_DFS_BYPASS_CNTL);
-
-	if (enable)
-		tmp |= (GCK_DFS_BYPASS_CNTL__BYPASSDCLK_MASK |
-			GCK_DFS_BYPASS_CNTL__BYPASSVCLK_MASK);
-	else
-		tmp &= ~(GCK_DFS_BYPASS_CNTL__BYPASSDCLK_MASK |
-			 GCK_DFS_BYPASS_CNTL__BYPASSVCLK_MASK);
-
-	WREG32_SMC(ixGCK_DFS_BYPASS_CNTL, tmp);
-}
-
 static int uvd_v6_0_set_clockgating_state(void *handle,
 					  enum amd_clockgating_state state)
 {
 	struct amdgpu_device *adev = (struct amdgpu_device *)handle;
 	bool enable = (state == AMD_CG_STATE_GATE) ? true : false;
 
-	uvd_v6_0_set_bypass_mode(adev, enable);
-
 	if (!(adev->cg_flags & AMD_CG_SUPPORT_UVD_MGCG))
 		return 0;
 
-- 
1.9.1


[-- Attachment #3: Type: text/plain, Size: 154 bytes --]

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH] drm/amdgpu: set bypass mode when uvd is idle.

[Deucher, Alexander]

> -----Original Message-----
> From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On Behalf
> Of Andy Furniss
> Sent: Sunday, November 06, 2016 3:31 PM
> To: Zhu, Rex; Deucher, Alexander; amd-gfx@lists.freedesktop.org
> Subject: Re: [PATCH] drm/amdgpu: set bypass mode when uvd is idle.
> 
> Zhu, Rex wrote:
> >>>> Is there any harm in just always putting it into bypass mode or
> >>>> does it interact badly with PG?  Presumably it does (otherwise
> >>>> we wouldn't need this patch), it would be good to note why.
> >
> > Rex: when UVD PG enabled, DCLK/VCLK will be turn off when uvd is
> > idle(DCLK=OFF). If we set bypass mode=1, dclk/vclk will be bypassed
> > to an external ‘Bypass’ clock(DCLK = 100MHz)
> >
> > So it is unnecessary to set bypass mode when PG enabled.
> >
> > +uvd_v5_0_set_bypass_mode(adev, !enable); This change is because
> > tom's commit 72cb64c1f6a3a8129af341e90418a687c4971a40 Fix the
> > sequence of UVD powergate function in smu7_clockgating.c.
> 
> I was about to file a bug till I tried this which fixes UVD perf
> on my R9285 + agd5f drm-next-4.10-wip.
> 
> Additional unrelated question = I notice that UVD does not seem
> to set other clocks quite high enough when used.
> 
> For playback the vo may bump things up a bit, but even then it can be a bit
> borderline for playing high bitrate UHD with powerplay on auto.
> 
> Pure decode benchmarks like
> 
> ffmpeg -hwaccel vdpau -i high-bitrate-2160p60-vid -pix_fmt nv12 -f null -
> 
> go from 63 -> 81 fps, powerplay auto -> high.

The UVD and gfx clocks are separate.  The gfx load for video decode operations is not generally great enough (CSC and maybe scaling) to generate enough gfx load to boost the gfx clocks to their highest level.  We plan to add an API to allow userspace applications to request a minimum floor for specific contexts, but it hasn't been implemented yet.  This is useful if you are trying to hit maximum decode rates, but may not always be the best choice for power usage.  You really only want to set the clocks high enough to hit the target frame rate.

Alex


_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH] drm/amdgpu: set bypass mode when uvd is idle.

[Deucher, Alexander]

> -----Original Message-----
> From: Zhu, Rex
> Sent: Monday, November 07, 2016 4:37 AM
> To: Deucher, Alexander; amd-gfx@lists.freedesktop.org
> Subject: RE: [PATCH] drm/amdgpu: set bypass mode when uvd is idle.
> 
> Hi Alex,
> 
> We don't need to set bypass mode for uvd.
> For the issue uvd clock stay in high clock when uvd is idle, just need to set the
> default uvd clock to 100MHz when initialize.
> 
> Please Review the attached patch.

Does amdgpu_asic_set_uvd_clocks() work reliably on all asics does it interact properly with UVD DPM?  Once we got UVD DPM working, we haven't really had much of a need for it.

Alex

> 
> Best Regards
> Rex
> 
> -----Original Message-----
> From: Deucher, Alexander
> Sent: Friday, November 04, 2016 9:43 PM
> To: Zhu, Rex; amd-gfx@lists.freedesktop.org
> Subject: RE: [PATCH] drm/amdgpu: set bypass mode when uvd is idle.
> 
> > -----Original Message-----
> > From: Zhu, Rex
> > Sent: Thursday, November 03, 2016 11:18 PM
> > To: Deucher, Alexander; amd-gfx@lists.freedesktop.org
> > Subject: RE: [PATCH] drm/amdgpu: set bypass mode when uvd is idle.
> >
> > >>>Is there any harm in just always putting it into bypass mode or
> > >>>does it
> > interact badly with PG?  Presumably it does (otherwise we wouldn't
> > need this patch), it would be good to
> > >>>note why.
> >
> > Rex: when UVD PG enabled, DCLK/VCLK will be turn off when uvd is
> > idle(DCLK=OFF). If we set bypass mode=1, dclk/vclk will be bypassed to
> > an external ‘Bypass’ clock(DCLK = 100MHz)
> >
> > So it is unnecessary to set bypass mode when PG enabled.
> >
> > +uvd_v5_0_set_bypass_mode(adev, !enable);
> > This change is because tom's commit
> > 72cb64c1f6a3a8129af341e90418a687c4971a40
> > Fix the sequence of UVD powergate function in smu7_clockgating.c.
> >
> 
> Thanks for clarifying.  Can you update the commit message to include these
> details?  With that:
> Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
> 
> >
> >
> >
> > Best Regards
> > Rex
> >
> > -----Original Message-----
> > From: Deucher, Alexander
> > Sent: Thursday, November 03, 2016 10:21 PM
> > To: Zhu, Rex; amd-gfx@lists.freedesktop.org
> > Cc: Zhu, Rex
> > Subject: RE: [PATCH] drm/amdgpu: set bypass mode when uvd is idle.
> >
> > > -----Original Message-----
> > > From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On
> > > Behalf Of Rex Zhu
> > > Sent: Thursday, November 03, 2016 4:14 AM
> > > To: amd-gfx@lists.freedesktop.org
> > > Cc: Zhu, Rex
> > > Subject: [PATCH] drm/amdgpu: set bypass mode when uvd is idle.
> > >
> > > Change-Id: If44f8e91d14f5ec7d4067691684866ef8d77724a
> > > Signed-off-by: Rex Zhu <Rex.Zhu@amd.com>
> > > ---
> > >  drivers/gpu/drm/amd/amdgpu/uvd_v5_0.c | 3 ++-
> > > drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c | 3 ++-
> > >  2 files changed, 4 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/amd/amdgpu/uvd_v5_0.c
> > > b/drivers/gpu/drm/amd/amdgpu/uvd_v5_0.c
> > > index 95303e2..0a6a0e7 100644
> > > --- a/drivers/gpu/drm/amd/amdgpu/uvd_v5_0.c
> > > +++ b/drivers/gpu/drm/amd/amdgpu/uvd_v5_0.c
> > > @@ -745,7 +745,8 @@ static int uvd_v5_0_set_clockgating_state(void
> > > *handle,
> > >  	bool enable = (state == AMD_CG_STATE_GATE) ? true : false;
> > >  	static int curstate = -1;
> > >
> > > -	uvd_v5_0_set_bypass_mode(adev, enable);
> > > +	if (!(adev->pg_flags & AMD_PG_SUPPORT_UVD))
> > > +		uvd_v5_0_set_bypass_mode(adev, !enable);
> >
> > Is there any harm in just always putting it into bypass mode or does
> > it interact badly with PG?  Presumably it does (otherwise we wouldn't
> > need this patch), it would be good to note why.
> >
> > Alex
> >
> > >
> > >  	if (!(adev->cg_flags & AMD_CG_SUPPORT_UVD_MGCG))
> > >  		return 0;
> > > diff --git a/drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c
> > > b/drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c
> > > index a339b5c..b64829fe 100644
> > > --- a/drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c
> > > +++ b/drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c
> > > @@ -955,7 +955,8 @@ static int uvd_v6_0_set_clockgating_state(void
> > > *handle,
> > >  	struct amdgpu_device *adev = (struct amdgpu_device *)handle;
> > >  	bool enable = (state == AMD_CG_STATE_GATE) ? true : false;
> > >
> > > -	uvd_v6_0_set_bypass_mode(adev, enable);
> > > +	if (!(adev->pg_flags & AMD_PG_SUPPORT_UVD))
> > > +		uvd_v6_0_set_bypass_mode(adev, !enable);
> > >
> > >  	if (!(adev->cg_flags & AMD_CG_SUPPORT_UVD_MGCG))
> > >  		return 0;
> > > --
> > > 1.9.1
> > >
> > > _______________________________________________
> > > 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

Re: [PATCH] drm/amdgpu: set bypass mode when uvd is idle.

[Andy Furniss]

Deucher, Alexander wrote:
>> -----Original Message----- From: amd-gfx
>> [mailto:amd-gfx-bounces@lists.freedesktop.org] On Behalf Of Andy
>> Furniss Sent: Sunday, November 06, 2016 3:31 PM To: Zhu, Rex;
>> Deucher, Alexander; amd-gfx@lists.freedesktop.org Subject: Re:
>> [PATCH] drm/amdgpu: set bypass mode when uvd is idle.
>>
>> Zhu, Rex wrote:
>>>>>> Is there any harm in just always putting it into bypass
>>>>>> mode or does it interact badly with PG?  Presumably it does
>>>>>> (otherwise we wouldn't need this patch), it would be good
>>>>>> to note why.
>>>
>>> Rex: when UVD PG enabled, DCLK/VCLK will be turn off when uvd is
>>> idle(DCLK=OFF). If we set bypass mode=1, dclk/vclk will be
>>> bypassed to an external ‘Bypass’ clock(DCLK = 100MHz)
>>>
>>> So it is unnecessary to set bypass mode when PG enabled.
>>>
>>> +uvd_v5_0_set_bypass_mode(adev, !enable); This change is because
>>> tom's commit 72cb64c1f6a3a8129af341e90418a687c4971a40 Fix the
>>> sequence of UVD powergate function in smu7_clockgating.c.
>>
>> I was about to file a bug till I tried this which fixes UVD perf on
>> my R9285 + agd5f drm-next-4.10-wip.
>>
>> Additional unrelated question = I notice that UVD does not seem to
>> set other clocks quite high enough when used.
>>
>> For playback the vo may bump things up a bit, but even then it can
>> be a bit borderline for playing high bitrate UHD with powerplay on
>> auto.
>>
>> Pure decode benchmarks like
>>
>> ffmpeg -hwaccel vdpau -i high-bitrate-2160p60-vid -pix_fmt nv12 -f
>> null -
>>
>> go from 63 -> 81 fps, powerplay auto -> high.
>
> The UVD and gfx clocks are separate.  The gfx load for video decode
> operations is not generally great enough (CSC and maybe scaling) to
> generate enough gfx load to boost the gfx clocks to their highest
> level.  We plan to add an API to allow userspace applications to
> request a minimum floor for specific contexts, but it hasn't been
> implemented yet.  This is useful if you are trying to hit maximum
> decode rates, but may not always be the best choice for power usage.
> You really only want to set the clocks high enough to hit the target
> frame rate.
>
> Alex

OK, thanks for the info.
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx