| Message ID | 1473984244-4349-3-git-send-email-manasi.d.navare@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. > > 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:
On Mon, 26 Sep 2016, Jani Nikula <jani.nikula@linux.intel.com> 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. Also double checked the DP link CTS spec. AFAICT none of the tests expect the source to use the max clock or max lane count directly. (Automated test request is another matter, and we should look at it.) I think patches 1-2 are based on an incorrect interpretation of the spec and tests. BR, Jani. > > BR, > Jani. > > >> >> 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:
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. 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
On Tue, Sep 27, 2016 at 04:39:38PM +0300, Jani Nikula wrote: > On Mon, 26 Sep 2016, Jani Nikula <jani.nikula@linux.intel.com> 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. > > Also double checked the DP link CTS spec. AFAICT none of the tests > expect the source to use the max clock or max lane count > directly. (Automated test request is another matter, and we should look > at it.) > > I think patches 1-2 are based on an incorrect interpretation of the spec > and tests. > > BR, > Jani. > I have the patches for handling the automated test request from DPR for compliance testing as mentioned in the CTS spec. But they have dependencies on these patches (1-6) so I will submit them after these get merged. Regards Manasi > > > > > BR, > > Jani. > > > > > >> > >> 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
On Wed, 28 Sep 2016, Manasi Navare <manasi.d.navare@intel.com> wrote: > On Tue, Sep 27, 2016 at 04:39:38PM +0300, Jani Nikula wrote: >> On Mon, 26 Sep 2016, Jani Nikula <jani.nikula@linux.intel.com> 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. >> >> Also double checked the DP link CTS spec. AFAICT none of the tests >> expect the source to use the max clock or max lane count >> directly. (Automated test request is another matter, and we should look >> at it.) >> >> I think patches 1-2 are based on an incorrect interpretation of the spec >> and tests. >> >> BR, >> Jani. >> > > I have the patches for handling the automated test request from DPR for > compliance testing as mentioned in the CTS spec. But they have dependencies > on these patches (1-6) so I will submit them after these get merged. We need to re-evaluate the ordering of the patches. BR, Jani. > > Regards > Manasi > >> >> > >> > BR, >> > Jani. >> > >> > >> >> >> >> 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
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. > > 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
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
On Wed, Sep 28, 2016 at 10:14:45AM +0300, Jani Nikula wrote: > On Wed, 28 Sep 2016, Manasi Navare <manasi.d.navare@intel.com> wrote: > > On Tue, Sep 27, 2016 at 04:39:38PM +0300, Jani Nikula wrote: > >> On Mon, 26 Sep 2016, Jani Nikula <jani.nikula@linux.intel.com> 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. > >> > >> Also double checked the DP link CTS spec. AFAICT none of the tests > >> expect the source to use the max clock or max lane count > >> directly. (Automated test request is another matter, and we should look > >> at it.) > >> > >> I think patches 1-2 are based on an incorrect interpretation of the spec > >> and tests. > >> > >> BR, > >> Jani. > >> > > > > I have the patches for handling the automated test request from DPR for > > compliance testing as mentioned in the CTS spec. But they have dependencies > > on these patches (1-6) so I will submit them after these get merged. > > We need to re-evaluate the ordering of the patches. > > BR, > Jani. > Thanks for your feedback. I am reevaluating the need for this patch based on your recommendation. I am currently testing it with keeping the optimal values of the link rate here and only train at the max link rate when we recieve the test request from DPR 120. In this case we would need to configure the PLLs base don the requested target link rate and train the link at that rate and after the test is done restore the original state of the PLLs. Does that sound like a good approach, is this what you were suggesting? Regards Manasi > > > > Regards > > Manasi > > > >> > >> > > >> > BR, > >> > Jani. > >> > > >> > > >> >> > >> >> 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
On Wed, 28 Sep 2016, Manasi Navare <manasi.d.navare@intel.com> wrote: > 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? If we go by the idea in [1], I think this will mean storing the parameters in test request, and having the userspace do another modeset (via sending a hotplug uevent), where we'll use the requested parameters. I'll still need to double check this complies with the CTS, but my first impression was yes. If the lane/rate do not match what's expected, the sink will play along until it can do the test request, and after that it will wait for another write of the lane/rate. Of course, this will need an userspace that listens to uevents and does modesets, but this should be the case with your usual desktop environment. [1] http://mid.mail-archive.com/8737kjlzfr.fsf@intel.com > 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? Same here, we'll store the failing parameters, prune the modes that need those parameters, and have the userspace try again. The really big upside of this approach is that we'll get error propagation from modeset, and the modeset/training sequence is always the same. BR, Jani. > > 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
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:
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. 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(-)