[PATCH 0/3] Support mclk switching when monitors are in sync

[PATCH 0/3] Support mclk switching when monitors are in sync

[Alex Deucher]

This patch set enables mclk switching with multiple monitors when all
monitors are sync.  Normally mclk switching is not available with
multiple monitors because the vblank timing does not line up.  However,
if the timing is identical, the display driver can sync up the displays
in some cases.  Check for these cases and allow mclk switch when
possible.

Alex Deucher (3):
  drm/amdgpu/powerplay/smu7: enable mclk switching if monitors are
    synced
  drm/amdgpu/powerplay/vega10: enable mclk switching if monitors are
    synced
  drm/amd/display: update bw_calcs to take pipe sync into account (v2)

 .../gpu/drm/amd/display/dc/calcs/dce_calcs.c  | 49 ++++++++++++++++++-
 .../gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c  |  7 +--
 .../drm/amd/powerplay/hwmgr/vega10_hwmgr.c    |  3 +-
 3 files changed, 53 insertions(+), 6 deletions(-)

-- 
2.20.1

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

[PATCH 1/3] drm/amdgpu/powerplay/smu7: enable mclk switching if monitors are synced

[Alex Deucher]

If DC has synced the displays, we can enable mclk switching to
save power.

Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
---
 drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c b/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
index 3c1084de5d59..34f95e0e3ea4 100644
--- a/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
+++ b/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
@@ -2956,9 +2956,10 @@ static int smu7_apply_state_adjust_rules(struct pp_hwmgr *hwmgr,
 	if (hwmgr->display_config->num_display == 0)
 		disable_mclk_switching = false;
 	else
-		disable_mclk_switching = ((1 < hwmgr->display_config->num_display) ||
-					  disable_mclk_switching_for_frame_lock ||
-					  smu7_vblank_too_short(hwmgr, hwmgr->display_config->min_vblank_time));
+		disable_mclk_switching = ((1 < hwmgr->display_config->num_display) &&
+					  !hwmgr->display_config->multi_monitor_in_sync) ||
+			disable_mclk_switching_for_frame_lock ||
+			smu7_vblank_too_short(hwmgr, hwmgr->display_config->min_vblank_time);
 
 	sclk = smu7_ps->performance_levels[0].engine_clock;
 	mclk = smu7_ps->performance_levels[0].memory_clock;
-- 
2.20.1

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

[PATCH 2/3] drm/amdgpu/powerplay/vega10: enable mclk switching if monitors are synced

[Alex Deucher]

If DC has synced the displays, we can enable mclk switching to
save power.

Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
---
 drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c b/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c
index 948c54cb9c5d..d08493b67b67 100644
--- a/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c
+++ b/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c
@@ -3220,7 +3220,8 @@ static int vega10_apply_state_adjust_rules(struct pp_hwmgr *hwmgr,
 	if (hwmgr->display_config->num_display == 0)
 		disable_mclk_switching = false;
 	else
-		disable_mclk_switching = (hwmgr->display_config->num_display > 1) ||
+		disable_mclk_switching = ((1 < hwmgr->display_config->num_display) &&
+					  !hwmgr->display_config->multi_monitor_in_sync) ||
 			disable_mclk_switching_for_frame_lock ||
 			disable_mclk_switching_for_vr ||
 			force_mclk_high;
-- 
2.20.1

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

[PATCH 3/3] drm/amd/display: update bw_calcs to take pipe sync into account (v2)

[Alex Deucher]

Properly set all_displays_in_sync so that when the data is
propagated to powerplay, it's set properly and we can enable
mclk switching when all monitors are in sync.

v2: fix logic, clean up

Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
---
 .../gpu/drm/amd/display/dc/calcs/dce_calcs.c  | 49 ++++++++++++++++++-
 1 file changed, 47 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/dc/calcs/dce_calcs.c b/drivers/gpu/drm/amd/display/dc/calcs/dce_calcs.c
index 9f12e21f8b9b..8d904647fb0f 100644
--- a/drivers/gpu/drm/amd/display/dc/calcs/dce_calcs.c
+++ b/drivers/gpu/drm/amd/display/dc/calcs/dce_calcs.c
@@ -25,6 +25,7 @@
 
 #include <linux/slab.h>
 
+#include "resource.h"
 #include "dm_services.h"
 #include "dce_calcs.h"
 #include "dc.h"
@@ -2977,6 +2978,50 @@ static void populate_initial_data(
 	data->number_of_displays = num_displays;
 }
 
+static bool all_displays_in_sync(const struct pipe_ctx pipe[],
+				 int pipe_count,
+				 uint32_t number_of_displays)
+{
+	const struct pipe_ctx *unsynced_pipes[MAX_PIPES] = { NULL };
+	int group_size = 1;
+	int i, j;
+
+	for (i = 0; i < pipe_count; i++) {
+		if (!pipe[i].stream)
+			continue;
+
+		unsynced_pipes[i] = &pipe[i];
+	}
+
+	for (i = 0; i < pipe_count; i++) {
+		const struct pipe_ctx *pipe_set[MAX_PIPES];
+
+		if (!unsynced_pipes[i])
+			continue;
+
+		pipe_set[0] = unsynced_pipes[i];
+		unsynced_pipes[i] = NULL;
+
+		/* Add tg to the set, search rest of the tg's for ones with
+		 * same timing, add all tgs with same timing to the group
+		 */
+		for (j = i + 1; j < pipe_count; j++) {
+			if (!unsynced_pipes[j])
+				continue;
+
+			if (resource_are_streams_timing_synchronizable(
+					unsynced_pipes[j]->stream,
+					pipe_set[0]->stream)) {
+				pipe_set[group_size] = unsynced_pipes[j];
+				unsynced_pipes[j] = NULL;
+				group_size++;
+			}
+		}
+	}
+
+	return (group_size == number_of_displays) ? true : false;
+}
+
 /**
  * Return:
  *	true -	Display(s) configuration supported.
@@ -2998,8 +3043,8 @@ bool bw_calcs(struct dc_context *ctx,
 
 	populate_initial_data(pipe, pipe_count, data);
 
-	/*TODO: this should be taken out calcs output and assigned during timing sync for pplib use*/
-	calcs_output->all_displays_in_sync = false;
+	calcs_output->all_displays_in_sync = all_displays_in_sync(pipe, pipe_count,
+								  data->number_of_displays);
 
 	if (data->number_of_displays != 0) {
 		uint8_t yclk_lvl, sclk_lvl;
-- 
2.20.1

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

Re: [PATCH 3/3] drm/amd/display: update bw_calcs to take pipe sync into account (v2)

[Kazlauskas, Nicholas]

On 8/22/19 11:36 AM, Alex Deucher wrote:
> Properly set all_displays_in_sync so that when the data is
> propagated to powerplay, it's set properly and we can enable
> mclk switching when all monitors are in sync.
> 
> v2: fix logic, clean up
> 
> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
> ---
>   .../gpu/drm/amd/display/dc/calcs/dce_calcs.c  | 49 ++++++++++++++++++-
>   1 file changed, 47 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/display/dc/calcs/dce_calcs.c b/drivers/gpu/drm/amd/display/dc/calcs/dce_calcs.c
> index 9f12e21f8b9b..8d904647fb0f 100644
> --- a/drivers/gpu/drm/amd/display/dc/calcs/dce_calcs.c
> +++ b/drivers/gpu/drm/amd/display/dc/calcs/dce_calcs.c
> @@ -25,6 +25,7 @@
>   
>   #include <linux/slab.h>
>   
> +#include "resource.h"
>   #include "dm_services.h"
>   #include "dce_calcs.h"
>   #include "dc.h"
> @@ -2977,6 +2978,50 @@ static void populate_initial_data(
>   	data->number_of_displays = num_displays;
>   }
>   
> +static bool all_displays_in_sync(const struct pipe_ctx pipe[],
> +				 int pipe_count,
> +				 uint32_t number_of_displays)
> +{
> +	const struct pipe_ctx *unsynced_pipes[MAX_PIPES] = { NULL };
> +	int group_size = 1;
> +	int i, j;
> +
> +	for (i = 0; i < pipe_count; i++) {
> +		if (!pipe[i].stream)

This bit differs from program_timing_sync, but since this is for dce and 
we don't do pipe split or MPO I think it's probably fine that you're not 
checking top_pipe here.

Wouldn't hurt to have that logic here though.

> +			continue;
> +
> +		unsynced_pipes[i] = &pipe[i];
> +	}
> +
> +	for (i = 0; i < pipe_count; i++) {
> +		const struct pipe_ctx *pipe_set[MAX_PIPES];
> +
> +		if (!unsynced_pipes[i])
> +			continue;
> +
> +		pipe_set[0] = unsynced_pipes[i];
> +		unsynced_pipes[i] = NULL;
> +
> +		/* Add tg to the set, search rest of the tg's for ones with
> +		 * same timing, add all tgs with same timing to the group
> +		 */
> +		for (j = i + 1; j < pipe_count; j++) {
> +			if (!unsynced_pipes[j])
> +				continue;
> +
> +			if (resource_are_streams_timing_synchronizable(
> +					unsynced_pipes[j]->stream,
> +					pipe_set[0]->stream)) {
> +				pipe_set[group_size] = unsynced_pipes[j];
> +				unsynced_pipes[j] = NULL;
> +				group_size++;
> +			}
> +		}
> +	}
> +
> +	return (group_size == number_of_displays) ? true : false;

I think this logic is functional but it looks incorrect at first glance 
because group_size doesn't get reset. What ends up happening is the 
first pipe of each group doesn't get added to group_size.

I feel that this would be more clear as:

static bool all_displays_in_sync(const struct pipe_ctx pipe[], int 
pipe_count)
{
	const struct pipe_ctx *active_pipes[MAX_PIPES];
	int i, num_active_pipes = 0;

	for (i = 0; i < pipe_count; i++) {
		if (!pipe[i].stream || pipe[i].top_pipe)
			continue;

		active_pipes[num_active_pipes++] = &pipe[i];
	}

	if (!num_active_pipes)
		return false;

	for (i = 1; i < num_active_pipes; ++i)
		if (!resource_are_streams_timing_synchronizable(
			    active_pipes[0]->stream, active_pipes[i]->stream))
			return false;

	return true;
}

But I haven't tested this.

Nicholas Kazlauskas


> +}
> +
>   /**
>    * Return:
>    *	true -	Display(s) configuration supported.
> @@ -2998,8 +3043,8 @@ bool bw_calcs(struct dc_context *ctx,
>   
>   	populate_initial_data(pipe, pipe_count, data);
>   
> -	/*TODO: this should be taken out calcs output and assigned during timing sync for pplib use*/
> -	calcs_output->all_displays_in_sync = false;
> +	calcs_output->all_displays_in_sync = all_displays_in_sync(pipe, pipe_count,
> +								  data->number_of_displays);
>   
>   	if (data->number_of_displays != 0) {
>   		uint8_t yclk_lvl, sclk_lvl;
> 

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

Re: [PATCH 3/3] drm/amd/display: update bw_calcs to take pipe sync into account (v2)

[Alex Deucher]

On Thu, Aug 22, 2019 at 12:25 PM Kazlauskas, Nicholas
<Nicholas.Kazlauskas@amd.com> wrote:
>
> On 8/22/19 11:36 AM, Alex Deucher wrote:
> > Properly set all_displays_in_sync so that when the data is
> > propagated to powerplay, it's set properly and we can enable
> > mclk switching when all monitors are in sync.
> >
> > v2: fix logic, clean up
> >
> > Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
> > ---
> >   .../gpu/drm/amd/display/dc/calcs/dce_calcs.c  | 49 ++++++++++++++++++-
> >   1 file changed, 47 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/display/dc/calcs/dce_calcs.c b/drivers/gpu/drm/amd/display/dc/calcs/dce_calcs.c
> > index 9f12e21f8b9b..8d904647fb0f 100644
> > --- a/drivers/gpu/drm/amd/display/dc/calcs/dce_calcs.c
> > +++ b/drivers/gpu/drm/amd/display/dc/calcs/dce_calcs.c
> > @@ -25,6 +25,7 @@
> >
> >   #include <linux/slab.h>
> >
> > +#include "resource.h"
> >   #include "dm_services.h"
> >   #include "dce_calcs.h"
> >   #include "dc.h"
> > @@ -2977,6 +2978,50 @@ static void populate_initial_data(
> >       data->number_of_displays = num_displays;
> >   }
> >
> > +static bool all_displays_in_sync(const struct pipe_ctx pipe[],
> > +                              int pipe_count,
> > +                              uint32_t number_of_displays)
> > +{
> > +     const struct pipe_ctx *unsynced_pipes[MAX_PIPES] = { NULL };
> > +     int group_size = 1;
> > +     int i, j;
> > +
> > +     for (i = 0; i < pipe_count; i++) {
> > +             if (!pipe[i].stream)
>
> This bit differs from program_timing_sync, but since this is for dce and
> we don't do pipe split or MPO I think it's probably fine that you're not
> checking top_pipe here.
>
> Wouldn't hurt to have that logic here though.
>

I had checked for top_pipe here originally, but it was always NULL so
unsynced_pipes never got populated.  Maybe that is not populated
properly at this point?

> > +                     continue;
> > +
> > +             unsynced_pipes[i] = &pipe[i];
> > +     }
> > +
> > +     for (i = 0; i < pipe_count; i++) {
> > +             const struct pipe_ctx *pipe_set[MAX_PIPES];
> > +
> > +             if (!unsynced_pipes[i])
> > +                     continue;
> > +
> > +             pipe_set[0] = unsynced_pipes[i];
> > +             unsynced_pipes[i] = NULL;
> > +
> > +             /* Add tg to the set, search rest of the tg's for ones with
> > +              * same timing, add all tgs with same timing to the group
> > +              */
> > +             for (j = i + 1; j < pipe_count; j++) {
> > +                     if (!unsynced_pipes[j])
> > +                             continue;
> > +
> > +                     if (resource_are_streams_timing_synchronizable(
> > +                                     unsynced_pipes[j]->stream,
> > +                                     pipe_set[0]->stream)) {
> > +                             pipe_set[group_size] = unsynced_pipes[j];
> > +                             unsynced_pipes[j] = NULL;
> > +                             group_size++;
> > +                     }
> > +             }
> > +     }
> > +
> > +     return (group_size == number_of_displays) ? true : false;
>
> I think this logic is functional but it looks incorrect at first glance
> because group_size doesn't get reset. What ends up happening is the
> first pipe of each group doesn't get added to group_size.
>
> I feel that this would be more clear as:
>
> static bool all_displays_in_sync(const struct pipe_ctx pipe[], int
> pipe_count)
> {
>         const struct pipe_ctx *active_pipes[MAX_PIPES];
>         int i, num_active_pipes = 0;
>
>         for (i = 0; i < pipe_count; i++) {
>                 if (!pipe[i].stream || pipe[i].top_pipe)
>                         continue;
>
>                 active_pipes[num_active_pipes++] = &pipe[i];
>         }
>
>         if (!num_active_pipes)
>                 return false;
>
>         for (i = 1; i < num_active_pipes; ++i)
>                 if (!resource_are_streams_timing_synchronizable(
>                             active_pipes[0]->stream, active_pipes[i]->stream))
>                         return false;
>
>         return true;
> }

Yes, that's much cleaner.  Thanks!

Alex

>
> But I haven't tested this.
>
> Nicholas Kazlauskas
>
>
> > +}
> > +
> >   /**
> >    * Return:
> >    *  true -  Display(s) configuration supported.
> > @@ -2998,8 +3043,8 @@ bool bw_calcs(struct dc_context *ctx,
> >
> >       populate_initial_data(pipe, pipe_count, data);
> >
> > -     /*TODO: this should be taken out calcs output and assigned during timing sync for pplib use*/
> > -     calcs_output->all_displays_in_sync = false;
> > +     calcs_output->all_displays_in_sync = all_displays_in_sync(pipe, pipe_count,
> > +                                                               data->number_of_displays);
> >
> >       if (data->number_of_displays != 0) {
> >               uint8_t yclk_lvl, sclk_lvl;
> >
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 3/3] drm/amd/display: update bw_calcs to take pipe sync into account (v2)

[Kazlauskas, Nicholas]

On 8/22/19 12:31 PM, Alex Deucher wrote:
> On Thu, Aug 22, 2019 at 12:25 PM Kazlauskas, Nicholas
> <Nicholas.Kazlauskas@amd.com> wrote:
>>
>> On 8/22/19 11:36 AM, Alex Deucher wrote:
>>> Properly set all_displays_in_sync so that when the data is
>>> propagated to powerplay, it's set properly and we can enable
>>> mclk switching when all monitors are in sync.
>>>
>>> v2: fix logic, clean up
>>>
>>> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
>>> ---
>>>    .../gpu/drm/amd/display/dc/calcs/dce_calcs.c  | 49 ++++++++++++++++++-
>>>    1 file changed, 47 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/display/dc/calcs/dce_calcs.c b/drivers/gpu/drm/amd/display/dc/calcs/dce_calcs.c
>>> index 9f12e21f8b9b..8d904647fb0f 100644
>>> --- a/drivers/gpu/drm/amd/display/dc/calcs/dce_calcs.c
>>> +++ b/drivers/gpu/drm/amd/display/dc/calcs/dce_calcs.c
>>> @@ -25,6 +25,7 @@
>>>
>>>    #include <linux/slab.h>
>>>
>>> +#include "resource.h"
>>>    #include "dm_services.h"
>>>    #include "dce_calcs.h"
>>>    #include "dc.h"
>>> @@ -2977,6 +2978,50 @@ static void populate_initial_data(
>>>        data->number_of_displays = num_displays;
>>>    }
>>>
>>> +static bool all_displays_in_sync(const struct pipe_ctx pipe[],
>>> +                              int pipe_count,
>>> +                              uint32_t number_of_displays)
>>> +{
>>> +     const struct pipe_ctx *unsynced_pipes[MAX_PIPES] = { NULL };
>>> +     int group_size = 1;
>>> +     int i, j;
>>> +
>>> +     for (i = 0; i < pipe_count; i++) {
>>> +             if (!pipe[i].stream)
>>
>> This bit differs from program_timing_sync, but since this is for dce and
>> we don't do pipe split or MPO I think it's probably fine that you're not
>> checking top_pipe here.
>>
>> Wouldn't hurt to have that logic here though.
>>
> 
> I had checked for top_pipe here originally, but it was always NULL so
> unsynced_pipes never got populated.  Maybe that is not populated
> properly at this point?

The presence of a top_pipe on a pipe indicates that the pipe is part of 
a blending chain. A NULL top_pipe value indicates that the current pipe 
is the top of the chain.

It should be NULL for all pipes on DCE ASICs.

Nicholas Kazlauskas

> 
>>> +                     continue;
>>> +
>>> +             unsynced_pipes[i] = &pipe[i];
>>> +     }
>>> +
>>> +     for (i = 0; i < pipe_count; i++) {
>>> +             const struct pipe_ctx *pipe_set[MAX_PIPES];
>>> +
>>> +             if (!unsynced_pipes[i])
>>> +                     continue;
>>> +
>>> +             pipe_set[0] = unsynced_pipes[i];
>>> +             unsynced_pipes[i] = NULL;
>>> +
>>> +             /* Add tg to the set, search rest of the tg's for ones with
>>> +              * same timing, add all tgs with same timing to the group
>>> +              */
>>> +             for (j = i + 1; j < pipe_count; j++) {
>>> +                     if (!unsynced_pipes[j])
>>> +                             continue;
>>> +
>>> +                     if (resource_are_streams_timing_synchronizable(
>>> +                                     unsynced_pipes[j]->stream,
>>> +                                     pipe_set[0]->stream)) {
>>> +                             pipe_set[group_size] = unsynced_pipes[j];
>>> +                             unsynced_pipes[j] = NULL;
>>> +                             group_size++;
>>> +                     }
>>> +             }
>>> +     }
>>> +
>>> +     return (group_size == number_of_displays) ? true : false;
>>
>> I think this logic is functional but it looks incorrect at first glance
>> because group_size doesn't get reset. What ends up happening is the
>> first pipe of each group doesn't get added to group_size.
>>
>> I feel that this would be more clear as:
>>
>> static bool all_displays_in_sync(const struct pipe_ctx pipe[], int
>> pipe_count)
>> {
>>          const struct pipe_ctx *active_pipes[MAX_PIPES];
>>          int i, num_active_pipes = 0;
>>
>>          for (i = 0; i < pipe_count; i++) {
>>                  if (!pipe[i].stream || pipe[i].top_pipe)
>>                          continue;
>>
>>                  active_pipes[num_active_pipes++] = &pipe[i];
>>          }
>>
>>          if (!num_active_pipes)
>>                  return false;
>>
>>          for (i = 1; i < num_active_pipes; ++i)
>>                  if (!resource_are_streams_timing_synchronizable(
>>                              active_pipes[0]->stream, active_pipes[i]->stream))
>>                          return false;
>>
>>          return true;
>> }
> 
> Yes, that's much cleaner.  Thanks!
> 
> Alex
> 
>>
>> But I haven't tested this.
>>
>> Nicholas Kazlauskas
>>
>>
>>> +}
>>> +
>>>    /**
>>>     * Return:
>>>     *  true -  Display(s) configuration supported.
>>> @@ -2998,8 +3043,8 @@ bool bw_calcs(struct dc_context *ctx,
>>>
>>>        populate_initial_data(pipe, pipe_count, data);
>>>
>>> -     /*TODO: this should be taken out calcs output and assigned during timing sync for pplib use*/
>>> -     calcs_output->all_displays_in_sync = false;
>>> +     calcs_output->all_displays_in_sync = all_displays_in_sync(pipe, pipe_count,
>>> +                                                               data->number_of_displays);
>>>
>>>        if (data->number_of_displays != 0) {
>>>                uint8_t yclk_lvl, sclk_lvl;
>>>
>>

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