drm/i915: make edp optimize config

Commit Message

Matt Atwood March 9, 2018, 1:17 a.m. UTC

From: Matt Atwood <[e-mail]>

Previously it was assumed that eDP panels would advertise the lowest link
rate required for their singular mode to function. With the introduction
of more advanced features there are advantages to a panel advertising a
higher rate then it needs for a its given mode. For panels that did, the
driver previously used a higher rate then necessary for that mode.

Signed-off-by: Matt Atwood <[e-mail]>
---
 drivers/gpu/drm/i915/intel_dp.c | 10 ----------
 1 file changed, 10 deletions(-)

Comments

Benson Leung March 9, 2018, 4:48 a.m. UTC | #1

Hi Matt,

Minor commit message nits.

On Thu, Mar 08, 2018 at 05:17:38PM -0800, [e-mail] wrote:
> From: Matt Atwood <[e-mail]>
> 
> Previously it was assumed that eDP panels would advertise the lowest link
> rate required for their singular mode to function. With the introduction
> of more advanced features there are advantages to a panel advertising a
> higher rate then
s/then/than

> it needs for a its given mode. 

Don't need "its" here.

> For panels that did, the
> driver previously used a higher rate then necessary for that mode.
>
s/then/than

 
> Signed-off-by: Matt Atwood <[e-mail]>

Tested-by: Benson Leung <[e-mail]>

Tested this on a panel and system with the following source and sink rates:


[    1.623225] [drm:intel_dp_print_rates] source rates: 162000, 216000, 270000, 324000, 432000, 540000
[    1.623230] [drm:intel_dp_print_rates] sink rates: 162000, 216000, 243000, 270000, 324000, 432000, 540000
[    1.623234] [drm:intel_dp_print_rates] common rates: 162000, 216000, 270000, 324000, 432000, 540000

Prior to this patch, the driver would pick and train at 540000:
[    2.865653] [drm:intel_dp_start_link_train] [CONNECTOR:76:eDP-1] Link Training Passed at Link Rate = 540000, Lane count = 4

After this patch, the driver picks and trains at 324000, which is enough
for the panel's native mode:
[    5.359499] [drm:intel_dp_start_link_train] [CONNECTOR:76:eDP-1] Link Training Passed at Link Rate = 324000, Lane count = 4

Thanks!
Benson

Jani Nikula March 9, 2018, 12:05 p.m. UTC | #2

On Thu, 08 Mar 2018, [e-mail] wrote:
> From: Matt Atwood <[e-mail]>
>
> Previously it was assumed that eDP panels would advertise the lowest link
> rate required for their singular mode to function. With the introduction
> of more advanced features there are advantages to a panel advertising a
> higher rate then it needs for a its given mode. For panels that did, the
> driver previously used a higher rate then necessary for that mode.
>
> Signed-off-by: Matt Atwood <[e-mail]>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 10 ----------
>  1 file changed, 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index a2eeede..aa6d77d 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1758,16 +1758,6 @@ intel_dp_compute_config(struct intel_encoder *encoder,
>  				      dev_priv->vbt.edp.bpp);
>  			bpp = dev_priv->vbt.edp.bpp;
>  		}
> -
> -		/*
> -		 * Use the maximum clock and number of lanes the eDP panel
> -		 * advertizes being capable of. The panels are generally
> -		 * designed to support only a single clock and lane
> -		 * configuration, and typically these values correspond to the
> -		 * native resolution of the panel.
> -		 */
> -		min_lane_count = max_lane_count;
> -		min_clock = max_clock;

Please see my reply to Manasi's identical patch [1]. If we apply this
as-is, it will regress and will be reverted.

BR,
Jani.


[1] http://patchwork.freedesktop.org/patch/msgid/[e-mail]


>  	}
>  
>  	for (; bpp >= 6*3; bpp -= 2*3) {

Matt Atwood March 10, 2018, 12:07 a.m. UTC | #3

On Fri, 2018-03-09 at 14:05 +0200, Jani Nikula wrote:
> On Thu, 08 Mar 2018, [e-mail] wrote:

> > 

> > From: Matt Atwood <[e-mail]>

> > 

> > Previously it was assumed that eDP panels would advertise the

> > lowest link

> > rate required for their singular mode to function. With the

> > introduction

> > of more advanced features there are advantages to a panel

> > advertising a

> > higher rate then it needs for a its given mode. For panels that

> > did, the

> > driver previously used a higher rate then necessary for that mode.

> > 

> > Signed-off-by: Matt Atwood <[e-mail]>

> > ---

> >  drivers/gpu/drm/i915/intel_dp.c | 10 ----------

> >  1 file changed, 10 deletions(-)

> > 

> > diff --git a/drivers/gpu/drm/i915/intel_dp.c

> > b/drivers/gpu/drm/i915/intel_dp.c

> > index a2eeede..aa6d77d 100644

> > --- a/drivers/gpu/drm/i915/intel_dp.c

> > +++ b/drivers/gpu/drm/i915/intel_dp.c

> > @@ -1758,16 +1758,6 @@ intel_dp_compute_config(struct intel_encoder

> > *encoder,

> >  				      dev_priv->vbt.edp.bpp);

> >  			bpp = dev_priv->vbt.edp.bpp;

> >  		}

> > -

> > -		/*

> > -		 * Use the maximum clock and number of lanes the

> > eDP panel

> > -		 * advertizes being capable of. The panels are

> > generally

> > -		 * designed to support only a single clock and

> > lane

> > -		 * configuration, and typically these values

> > correspond to the

> > -		 * native resolution of the panel.

> > -		 */

> > -		min_lane_count = max_lane_count;

> > -		min_clock = max_clock;

> Please see my reply to Manasi's identical patch [1]. If we apply this

> as-is, it will regress and will be reverted.

> 

> BR,

> Jani.

> 

> 

> [1] http://patchwork.freedesktop.org/patch/msgid/1520579339-14745-1-

> [e-mail]

to consolidate some of the information the bug that's referenced in
Manasi's patch is https://bugs.freedesktop.org/show_bug.cgi?id=73539. I
understand this bug as the following some panels may advertise a mode
that requires less then MAX_LANE_COUNT, however those panels would fail
if less lanes were used.

When this bug was filed the compute config inner for loop iterated on
rate and the outer iterated on lanes; this is now flipped. It *should*
be the case that the lowest frequency with the max amount of lanes is
preferred. Given the opposite behavior of the alogorithm to select I
dont think we'd come across this again. Even if I'm wrong we could
still min_lane_count = max_lane count and let the clock optimize
itself.

I guess my question is, is there also a bug where if MAX_RATE wasnt
used we saw a panel fail as well?

Matt
> 

> 

> > 

> >  	}

> >  

> >  	for (; bpp >= 6*3; bpp -= 2*3) {

Rodrigo Vivi March 12, 2018, 7:25 p.m. UTC | #4

On Sat, Mar 10, 2018 at 12:07:34AM +0000, Atwood, Matthew S wrote:
> On Fri, 2018-03-09 at 14:05 +0200, Jani Nikula wrote:
> > On Thu, 08 Mar 2018, [e-mail] wrote:
> > > 
> > > From: Matt Atwood <[e-mail]>
> > > 
> > > Previously it was assumed that eDP panels would advertise the
> > > lowest link
> > > rate required for their singular mode to function. With the
> > > introduction
> > > of more advanced features there are advantages to a panel
> > > advertising a
> > > higher rate then it needs for a its given mode. For panels that
> > > did, the
> > > driver previously used a higher rate then necessary for that mode.
> > > 
> > > Signed-off-by: Matt Atwood <[e-mail]>
> > > ---
> > >  drivers/gpu/drm/i915/intel_dp.c | 10 ----------
> > >  1 file changed, 10 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > b/drivers/gpu/drm/i915/intel_dp.c
> > > index a2eeede..aa6d77d 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -1758,16 +1758,6 @@ intel_dp_compute_config(struct intel_encoder
> > > *encoder,
> > >  				      dev_priv->vbt.edp.bpp);
> > >  			bpp = dev_priv->vbt.edp.bpp;
> > >  		}
> > > -
> > > -		/*
> > > -		 * Use the maximum clock and number of lanes the
> > > eDP panel
> > > -		 * advertizes being capable of. The panels are
> > > generally
> > > -		 * designed to support only a single clock and
> > > lane
> > > -		 * configuration, and typically these values
> > > correspond to the
> > > -		 * native resolution of the panel.
> > > -		 */
> > > -		min_lane_count = max_lane_count;
> > > -		min_clock = max_clock;
> > Please see my reply to Manasi's identical patch [1]. If we apply this
> > as-is, it will regress and will be reverted.
> > 
> > BR,
> > Jani.
> > 
> > 
> > [1] http://patchwork.freedesktop.org/patch/msgid/1520579339-14745-1-
> > [e-mail]
> to consolidate some of the information the bug that's referenced in
> Manasi's patch is https://bugs.freedesktop.org/show_bug.cgi?id=73539. I
> understand this bug as the following some panels may advertise a mode
> that requires less then MAX_LANE_COUNT, however those panels would fail
> if less lanes were used.
> 
> When this bug was filed the compute config inner for loop iterated on
> rate and the outer iterated on lanes; this is now flipped. It *should*
> be the case that the lowest frequency with the max amount of lanes is
> preferred. Given the opposite behavior of the alogorithm to select I
> dont think we'd come across this again. Even if I'm wrong we could
> still min_lane_count = max_lane count and let the clock optimize
> itself.
> 
> I guess my question is, is there also a bug where if MAX_RATE wasnt
> used we saw a panel fail as well?

Looking to eDP 1.4 spec I'm convinced that max link rate approach is
the ideal for eDP 1.3 and older, regardless of the issues we had on
previous attempts.

But eDP 1.4 seems to introduce the new link rates flexibility and selection
in order to provide improved system-specific link rate optimization
and power efficiency.

I believe what we want here is:

-		min_lane_count = max_lane_count;
-		min_clock = max_clock;
+		if (intel_dp->edp_dpcd[0] <= DP_EDP_13) {
+			min_lane_count = max_lane_count;
+			min_clock = max_clock;
+		}

for me it seems that eDP 1.4 brings eDP link rates handling close to
DP ones. So imho this move here should be safe.

Besides, with this difference in place,
we don't break the world and if new bugs appear we can
work on those individually to figure out if we are missing something
else and this block was only masking other issues.

Thoughts?

> 
> Matt
> > 
> > 
> > > 
> > >  	}
> > >  
> > >  	for (; bpp >= 6*3; bpp -= 2*3) {
> _______________________________________________
> Intel-gfx mailing list
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Patch

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index a2eeede..aa6d77d 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1758,16 +1758,6 @@  intel_dp_compute_config(struct intel_encoder *encoder,
 				      dev_priv->vbt.edp.bpp);
 			bpp = dev_priv->vbt.edp.bpp;
 		}
-
-		/*
-		 * Use the maximum clock and number of lanes the eDP panel
-		 * advertizes being capable of. The panels are generally
-		 * designed to support only a single clock and lane
-		 * configuration, and typically these values correspond to the
-		 * native resolution of the panel.
-		 */
-		min_lane_count = max_lane_count;
-		min_clock = max_clock;
 	}
 
 	for (; bpp >= 6*3; bpp -= 2*3) {