[Intel-gfx] [PATCH 1/2] drm/i915: Modify DP set clock to accomodate more eDP timings.

[Lee, Chon Ming]

On 08/30 10:28, Jani Nikula wrote:
> On Fri, 30 Aug 2013, Chon Ming Lee <chon.ming.lee at intel.com> wrote:
> > eDP 1.4 supports 4-5 extra link rates in additional to current 2 link
> > rate.  Create a structure to store the DPLL divisor data to improve
> > readability.
> 
> DP 1.2 already supports 3 link rates, right?

Yes, 3 link rate for DP 1.2, but most of the older intel gfx doesn't support the
highest 5.4Gbps link rate yet.

> 
> > Signed-off-by: Chon Ming Lee <chon.ming.lee at intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c |   48 +++++++++++++++++++-------------------
> >  1 files changed, 24 insertions(+), 24 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 2151d13..ab8a5ff 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -38,6 +38,19 @@
> >  
> >  #define DP_LINK_CHECK_TIMEOUT	(10 * 1000)
> >  
> > +struct dp_link_dpll{
> 
> Missing space before {.
> 
> > +	int link_bw;
> > +	struct dpll dpll;
> > +};
> > +
> > +static const struct dp_link_dpll gen4_dpll[] = 
> > +				{{ DP_LINK_BW_1_62, {2,23,8,2,10,0,0,0,0}},
> > +				{ DP_LINK_BW_2_7,  {1,14,2,1,10,0,0,0,0}}};
> > +
> > +static const struct dp_link_dpll pch_dpll[] = 
> > +				{{ DP_LINK_BW_1_62, {1,12,9,2,10,0,0,0,0}},
> > +				{ DP_LINK_BW_2_7,  {2,14,8,1,10,0,0,0,0}}};
> > +
> 
> Please switch these to use C99 designated initializers for clarity,
> something like this:
> 
> static const struct dp_link_dpll gen4_dpll[] = {
> 	{
> 		.link_bw = DP_LINK_BW_1_62,
> 		.dpll = {
> 			.n = 2,
> 			.m1 = 23, m2 = 8,
> 			p1 = 2, p2 = 10, 
> 		},
> 	}, {
> 		.link_bw = DP_LINK_BW_2_7,
> 		.dpll = {
> 			.n = 1,
> 			.m1 = 14, m2 = 2,
> 			p1 = 1, p2 = 10, 
> 		},
> 	}
> };
> 
Thanks, will make the correction.

> >  /**
> >   * is_edp - is the given port attached to an eDP panel (either CPU or PCH)
> >   * @intel_dp: DP struct
> > @@ -649,37 +662,24 @@ intel_dp_set_clock(struct intel_encoder *encoder,
> >  		   struct intel_crtc_config *pipe_config, int link_bw)
> >  {
> >  	struct drm_device *dev = encoder->base.dev;
> > +	int i;
> >  
> >  	if (IS_G4X(dev)) {
> > -		if (link_bw == DP_LINK_BW_1_62) {
> > -			pipe_config->dpll.p1 = 2;
> > -			pipe_config->dpll.p2 = 10;
> > -			pipe_config->dpll.n = 2;
> > -			pipe_config->dpll.m1 = 23;
> > -			pipe_config->dpll.m2 = 8;
> > -		} else {
> > -			pipe_config->dpll.p1 = 1;
> > -			pipe_config->dpll.p2 = 10;
> > -			pipe_config->dpll.n = 1;
> > -			pipe_config->dpll.m1 = 14;
> > -			pipe_config->dpll.m2 = 2;
> > +		for(i = 0; i < sizeof(gen4_dpll) / sizeof(struct dp_link_dpll); i++) {
> 
> Please use i < ARRAY_SIZE(gen4_dpll).

Already make this change.  After sent out the patch, realize I should this
ARRAY_SIZE.  Thanks to point this out.  
> 
> > +			if (link_bw == gen4_dpll[i].link_bw){
> > +				pipe_config->dpll = gen4_dpll[i].dpll;
> > +				break;
> > +			}
> >  		}
> 
> The old if-else used some values even if link_bw was bogus. I'm not sure
> how likely that is. But if the link_bw is not found in the table for
> some obscure reason (e.g. the hw doesn't support the link rate), this
> fails. Perhaps we should have a test for i == ARRAY_SIZE(gen4_dpll) here
> and complain loudly if we hit that, and perhaps use a fallback value.
> 

In intel_dp_compute_config, it will only assign either one of two link rates 
into link_bw before calling this function.  The link_bw won't be out of range.  

I would think the checking should be done prior to calling this function.  For
example, in eDP 1.4, instead of supporting more link rates, there is possible to
use single lane, but choose the largest link rate, or use 4 lanes, with lower
link rate.  If fail out here, might be too late to recover.   

> >  		pipe_config->clock_set = true;
> >  	} else if (IS_HASWELL(dev)) {
> >  		/* Haswell has special-purpose DP DDI clocks. */
> >  	} else if (HAS_PCH_SPLIT(dev)) {
> > -		if (link_bw == DP_LINK_BW_1_62) {
> > -			pipe_config->dpll.n = 1;
> > -			pipe_config->dpll.p1 = 2;
> > -			pipe_config->dpll.p2 = 10;
> > -			pipe_config->dpll.m1 = 12;
> > -			pipe_config->dpll.m2 = 9;
> > -		} else {
> > -			pipe_config->dpll.n = 2;
> > -			pipe_config->dpll.p1 = 1;
> > -			pipe_config->dpll.p2 = 10;
> > -			pipe_config->dpll.m1 = 14;
> > -			pipe_config->dpll.m2 = 8;
> > +		for(i = 0; i < sizeof(pch_dpll) / sizeof(struct dp_link_dpll); i++) {
> > +			if (link_bw == pch_dpll[i].link_bw){ 
> > +				pipe_config->dpll = pch_dpll[i].dpll;
> > +				break;
> > +			}
> >  		}
> 
> Same here.
> 
> BR,
> Jani.
> 
> 
> >  		pipe_config->clock_set = true;
> >  	} else if (IS_VALLEYVIEW(dev)) {
> > -- 
> > 1.7.7.6
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center