Re: [PATCH v3 2/6] drm/i915: Remove the link rate and lane count loop in compute config

From: Manasi Navare <manasi.d.navare@intel.com>
To: Jani Nikula <jani.nikula@linux.intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH v3 2/6] drm/i915: Remove the link rate and lane	count loop in compute config
Date: Wed, 28 Sep 2016 09:45:20 -0700	[thread overview]
Message-ID: <20160928164520.GA21331@intel.com> (raw)
In-Reply-To: <87fuoko4n6.fsf@intel.com>

On Wed, Sep 28, 2016 at 10:38:37AM +0300, Jani Nikula wrote:
> On Wed, 28 Sep 2016, Manasi Navare <manasi.d.navare@intel.com> wrote:
> > On Mon, Sep 26, 2016 at 04:41:27PM +0300, Jani Nikula wrote:
> >> On Fri, 16 Sep 2016, Manasi Navare <manasi.d.navare@intel.com> wrote:
> >> > While configuring the pipe during modeset, it should use
> >> > max clock and max lane count and reduce the bpp until
> >> > the requested mode rate is less than or equal to
> >> > available link BW.
> >> > This is required to pass DP Compliance.
> >> 
> >> As I wrote in reply to patch 1/6, this is not a DP spec requirement. The
> >> link policy maker can freely choose the link parameters as long as the
> >> sink supports them.
> >> 
> >> BR,
> >> Jani.
> >> 
> >>
> >
> > Thanks for your review feedback.
> > This change was driven by Video Pattern generation tests in CTS spec. Eg: In
> > test 4.3.3.1, the test requests 640x480 @ max link rate of 2.7Gbps and 4 lanes.
> > The test will pass if it sets the link rate to 2.7 and lane count = 4.
> > But in the existing implementation, this video mode request triggers a modeset
> > but the compute_config function starts with the lowest link rate and lane count and
> > trains the link at 1.62 and 4 lanes which does not match the expeced values of link
> > rate = 2.7 and lane count = 4 and the test fails. 
> 
> Again, the test does not require us to use the maximum parameters by
> default. It allows us to use optimal parameters by default, and use the
> sink issued automated test request to change the link parameters to what
> the test wants.
> 
> Look at the table in CTS 4.3.3.1. There's a test for 640x480 with 1.62
> Gbps and 1 lane. And then there's a test for 640x480 with 2.7 Gbps and 4
> lanes. What you're suggesting is to use excessive bandwidth for the mode
> by default just because the test has been designed to be lax and allow
> certain parameters at a minimum, instead of requiring optimal
> parameters.
> 
> I do not think this is a change we want to make for DP SST, and it is
> not a DP spec or compliance requirement.
> 
> BR,
> Jani.
> 
>

So if we let the driver chose the optimal link rate and lane count then for
640x480 it will chose 1.62 and 4 lanes. So then the automated test request
will issue the test request for the maximum link rate lets say 5.4 and 4 lanes.
At this point we will have to re set the plls and the clocks to train the link at 
5.4link rate and 4 lane count before proceeding to handling the video pattern request.
Are you recommending to doing the entire pll set up and retraining of the link here to the 
target link rate which will be the max link rate?

What about the tests 4.3.1.4 that expect the link rate to fall back to the lower link
rate due the forced failures in the CR/Channel EQ phases? For these cases we do need
upfront link training and starting the link training at the upfront values falling back
to the lower values. What do you think? 

Regards
Manasi 
> >
> > Regards
> > Manasi
> >  
> >> >
> >> > v3:
> >> > * Add Debug print if requested mode cannot be supported
> >> > during modeset (Dhinakaran Pandiyan)
> >> > v2:
> >> > * Removed the loop since we use max values of clock
> >> > and lane count (Dhinakaran Pandiyan)
> >> >
> >> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> >> > ---
> >> >  drivers/gpu/drm/i915/intel_dp.c | 22 ++++++++--------------
> >> >  1 file changed, 8 insertions(+), 14 deletions(-)
> >> >
> >> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> >> > index d81c67cb..65b4559 100644
> >> > --- a/drivers/gpu/drm/i915/intel_dp.c
> >> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> >> > @@ -1644,23 +1644,17 @@ intel_dp_compute_config(struct intel_encoder *encoder,
> >> >  	for (; bpp >= 6*3; bpp -= 2*3) {
> >> >  		mode_rate = intel_dp_link_required(adjusted_mode->crtc_clock,
> >> >  						   bpp);
> >> > +		clock = max_clock;
> >> > +		lane_count = max_lane_count;
> >> > +		link_clock = common_rates[clock];
> >> > +		link_avail = intel_dp_max_data_rate(link_clock,
> >> > +						    lane_count);
> >> >  
> >> > -		for (clock = min_clock; clock <= max_clock; clock++) {
> >> > -			for (lane_count = min_lane_count;
> >> > -				lane_count <= max_lane_count;
> >> > -				lane_count <<= 1) {
> >> > -
> >> > -				link_clock = common_rates[clock];
> >> > -				link_avail = intel_dp_max_data_rate(link_clock,
> >> > -								    lane_count);
> >> > -
> >> > -				if (mode_rate <= link_avail) {
> >> > -					goto found;
> >> > -				}
> >> > -			}
> >> > -		}
> >> > +		if (mode_rate <= link_avail)
> >> > +			goto found;
> >> >  	}
> >> >  
> >> > +	DRM_DEBUG_KMS("Requested Mode Rate not supported\n");
> >> >  	return false;
> >> >  
> >> >  found:
> >> 
> >> -- 
> >> Jani Nikula, Intel Open Source Technology Center
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx