diff mbox series

[1/2] drm/edid: Add a EDID edp panel quirk for forcing max lane count

Message ID 20190402215235.29895-1-manasi.d.navare@intel.com (mailing list archive)
State New, archived
Headers show
Series [1/2] drm/edid: Add a EDID edp panel quirk for forcing max lane count | expand

Commit Message

Navare, Manasi April 2, 2019, 9:52 p.m. UTC
For certain eDP 1.4 panels, we need to use max lane count for the
link training to succeed.

This patch adds a EDID quirk for such eDP panels using
their vendor ID and product ID to force using max lane count in the driver.

Cc: Clint Taylor <Clinton.A.Taylor@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Tested-by: Albert Astals Cid <aacid@kde.org>
Tested-by: Emanuele Panigati <ilpanich@gmail.com>
Tested-by: Ralgor <ralgorfdb@compuspex.org>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=109959
Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
---
 drivers/gpu/drm/drm_edid.c  | 10 ++++++++++
 include/drm/drm_connector.h |  5 +++++
 2 files changed, 15 insertions(+)

Comments

Clint Taylor April 2, 2019, 9:08 p.m. UTC | #1
On 4/2/19 2:52 PM, Manasi Navare wrote:
> For certain eDP 1.4 panels, we need to use max lane count for the
> link training to succeed.
>
> This patch adds a EDID quirk for such eDP panels using
> their vendor ID and product ID to force using max lane count in the driver.
>
> Cc: Clint Taylor <Clinton.A.Taylor@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Tested-by: Albert Astals Cid <aacid@kde.org>
> Tested-by: Emanuele Panigati <ilpanich@gmail.com>
> Tested-by: Ralgor <ralgorfdb@compuspex.org>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=109959
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> ---
>   drivers/gpu/drm/drm_edid.c  | 10 ++++++++++
>   include/drm/drm_connector.h |  5 +++++
>   2 files changed, 15 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 2c22ea446075..fbc661806484 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -82,6 +82,8 @@
>   #define EDID_QUIRK_FORCE_10BPC			(1 << 11)
>   /* Non desktop display (i.e. HMD) */
>   #define EDID_QUIRK_NON_DESKTOP			(1 << 12)
> +/* Force max lane count */

Nit. Would like a little more descriptive comment about this quirk. 
Maybe even /* Force use of max lane count for link training */

> +#define EDID_QUIRK_FORCE_MAX_LANE_COUNT		(1 << 13)
>   
>   struct detailed_mode_closure {
>   	struct drm_connector *connector;
> @@ -189,6 +191,10 @@ static const struct edid_quirk {
>   
>   	/* OSVR HDK and HDK2 VR Headsets */
>   	{ "SVR", 0x1019, EDID_QUIRK_NON_DESKTOP },
> +
> +	/* SHP eDP 1.4 panel only works with max lane count */
> +	{ "SHP", 0x149a, EDID_QUIRK_FORCE_MAX_LANE_COUNT },
> +	{ "SHP", 0x148e, EDID_QUIRK_FORCE_MAX_LANE_COUNT },
>   };
>   
>   /*
> @@ -4463,6 +4469,7 @@ drm_reset_display_info(struct drm_connector *connector)
>   	memset(&info->hdmi, 0, sizeof(info->hdmi));
>   
>   	info->non_desktop = 0;
> +	info->force_max_lane_count = 0;
>   }
>   
>   u32 drm_add_display_info(struct drm_connector *connector, const struct edid *edid)
> @@ -4744,6 +4751,9 @@ int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid)
>   	if (quirks & EDID_QUIRK_FORCE_12BPC)
>   		connector->display_info.bpc = 12;
>   
> +	if (quirks & EDID_QUIRK_FORCE_MAX_LANE_COUNT)
> +		connector->display_info.force_max_lane_count = true;
> +
>   	return num_modes;
>   }
>   EXPORT_SYMBOL(drm_add_edid_modes);
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 02a131202add..45436d40ffe3 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -457,6 +457,11 @@ struct drm_display_info {
>   	 * @non_desktop: Non desktop display (HMD).
>   	 */
>   	bool non_desktop;
> +
> +	/**
> +	 * @force_max_lane_count: Link training requires max lane count to pass
> +	 */
> +	bool force_max_lane_count;
>   };
>   
>   int drm_display_info_set_bus_formats(struct drm_display_info *info,

Rest of the patch appears sane for a quirk.

Reviewed-by: Clint Taylor <Clinton.A.Taylor@intel.com>

-Clint
Ville Syrjala April 3, 2019, 12:14 p.m. UTC | #2
On Tue, Apr 02, 2019 at 02:52:34PM -0700, Manasi Navare wrote:
> For certain eDP 1.4 panels, we need to use max lane count for the
> link training to succeed.
> 
> This patch adds a EDID quirk for such eDP panels using
> their vendor ID and product ID to force using max lane count in the driver.

Rather than opening the quirk can of worms I think we should consider
changing the retry loop to do something more sensible than what it's
doing now. The current behaviour of "start at optimal settings (which
can be either min lanes or min rate), and then reduce lanes/rate until
stuff works" overlooks several possible combinations. One possible
approach could be to start the retry loop with max lanes + max rate
after the optimal settings have failed. It probably won't give you
the best power consumption, but at least you get a picture on the
screen if even a single lane count + rate combo works.

> 
> Cc: Clint Taylor <Clinton.A.Taylor@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Tested-by: Albert Astals Cid <aacid@kde.org>
> Tested-by: Emanuele Panigati <ilpanich@gmail.com>
> Tested-by: Ralgor <ralgorfdb@compuspex.org>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=109959
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> ---
>  drivers/gpu/drm/drm_edid.c  | 10 ++++++++++
>  include/drm/drm_connector.h |  5 +++++
>  2 files changed, 15 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 2c22ea446075..fbc661806484 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -82,6 +82,8 @@
>  #define EDID_QUIRK_FORCE_10BPC			(1 << 11)
>  /* Non desktop display (i.e. HMD) */
>  #define EDID_QUIRK_NON_DESKTOP			(1 << 12)
> +/* Force max lane count */
> +#define EDID_QUIRK_FORCE_MAX_LANE_COUNT		(1 << 13)
>  
>  struct detailed_mode_closure {
>  	struct drm_connector *connector;
> @@ -189,6 +191,10 @@ static const struct edid_quirk {
>  
>  	/* OSVR HDK and HDK2 VR Headsets */
>  	{ "SVR", 0x1019, EDID_QUIRK_NON_DESKTOP },
> +
> +	/* SHP eDP 1.4 panel only works with max lane count */
> +	{ "SHP", 0x149a, EDID_QUIRK_FORCE_MAX_LANE_COUNT },
> +	{ "SHP", 0x148e, EDID_QUIRK_FORCE_MAX_LANE_COUNT },
>  };
>  
>  /*
> @@ -4463,6 +4469,7 @@ drm_reset_display_info(struct drm_connector *connector)
>  	memset(&info->hdmi, 0, sizeof(info->hdmi));
>  
>  	info->non_desktop = 0;
> +	info->force_max_lane_count = 0;
>  }
>  
>  u32 drm_add_display_info(struct drm_connector *connector, const struct edid *edid)
> @@ -4744,6 +4751,9 @@ int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid)
>  	if (quirks & EDID_QUIRK_FORCE_12BPC)
>  		connector->display_info.bpc = 12;
>  
> +	if (quirks & EDID_QUIRK_FORCE_MAX_LANE_COUNT)
> +		connector->display_info.force_max_lane_count = true;
> +
>  	return num_modes;
>  }
>  EXPORT_SYMBOL(drm_add_edid_modes);
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 02a131202add..45436d40ffe3 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -457,6 +457,11 @@ struct drm_display_info {
>  	 * @non_desktop: Non desktop display (HMD).
>  	 */
>  	bool non_desktop;
> +
> +	/**
> +	 * @force_max_lane_count: Link training requires max lane count to pass
> +	 */
> +	bool force_max_lane_count;
>  };
>  
>  int drm_display_info_set_bus_formats(struct drm_display_info *info,
> -- 
> 2.19.1
Daniel Vetter April 3, 2019, 1:19 p.m. UTC | #3
On Wed, Apr 03, 2019 at 03:14:51PM +0300, Ville Syrjälä wrote:
> On Tue, Apr 02, 2019 at 02:52:34PM -0700, Manasi Navare wrote:
> > For certain eDP 1.4 panels, we need to use max lane count for the
> > link training to succeed.
> > 
> > This patch adds a EDID quirk for such eDP panels using
> > their vendor ID and product ID to force using max lane count in the driver.
> 
> Rather than opening the quirk can of worms I think we should consider
> changing the retry loop to do something more sensible than what it's
> doing now. The current behaviour of "start at optimal settings (which
> can be either min lanes or min rate), and then reduce lanes/rate until
> stuff works" overlooks several possible combinations. One possible
> approach could be to start the retry loop with max lanes + max rate
> after the optimal settings have failed. It probably won't give you
> the best power consumption, but at least you get a picture on the
> screen if even a single lane count + rate combo works.

Hm yeah I guess this is an approach we haven't tried yet ... I think we've
tried everything else already.
-Daniel

> 
> > 
> > Cc: Clint Taylor <Clinton.A.Taylor@intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Tested-by: Albert Astals Cid <aacid@kde.org>
> > Tested-by: Emanuele Panigati <ilpanich@gmail.com>
> > Tested-by: Ralgor <ralgorfdb@compuspex.org>
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=109959
> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > ---
> >  drivers/gpu/drm/drm_edid.c  | 10 ++++++++++
> >  include/drm/drm_connector.h |  5 +++++
> >  2 files changed, 15 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > index 2c22ea446075..fbc661806484 100644
> > --- a/drivers/gpu/drm/drm_edid.c
> > +++ b/drivers/gpu/drm/drm_edid.c
> > @@ -82,6 +82,8 @@
> >  #define EDID_QUIRK_FORCE_10BPC			(1 << 11)
> >  /* Non desktop display (i.e. HMD) */
> >  #define EDID_QUIRK_NON_DESKTOP			(1 << 12)
> > +/* Force max lane count */
> > +#define EDID_QUIRK_FORCE_MAX_LANE_COUNT		(1 << 13)
> >  
> >  struct detailed_mode_closure {
> >  	struct drm_connector *connector;
> > @@ -189,6 +191,10 @@ static const struct edid_quirk {
> >  
> >  	/* OSVR HDK and HDK2 VR Headsets */
> >  	{ "SVR", 0x1019, EDID_QUIRK_NON_DESKTOP },
> > +
> > +	/* SHP eDP 1.4 panel only works with max lane count */
> > +	{ "SHP", 0x149a, EDID_QUIRK_FORCE_MAX_LANE_COUNT },
> > +	{ "SHP", 0x148e, EDID_QUIRK_FORCE_MAX_LANE_COUNT },
> >  };
> >  
> >  /*
> > @@ -4463,6 +4469,7 @@ drm_reset_display_info(struct drm_connector *connector)
> >  	memset(&info->hdmi, 0, sizeof(info->hdmi));
> >  
> >  	info->non_desktop = 0;
> > +	info->force_max_lane_count = 0;
> >  }
> >  
> >  u32 drm_add_display_info(struct drm_connector *connector, const struct edid *edid)
> > @@ -4744,6 +4751,9 @@ int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid)
> >  	if (quirks & EDID_QUIRK_FORCE_12BPC)
> >  		connector->display_info.bpc = 12;
> >  
> > +	if (quirks & EDID_QUIRK_FORCE_MAX_LANE_COUNT)
> > +		connector->display_info.force_max_lane_count = true;
> > +
> >  	return num_modes;
> >  }
> >  EXPORT_SYMBOL(drm_add_edid_modes);
> > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > index 02a131202add..45436d40ffe3 100644
> > --- a/include/drm/drm_connector.h
> > +++ b/include/drm/drm_connector.h
> > @@ -457,6 +457,11 @@ struct drm_display_info {
> >  	 * @non_desktop: Non desktop display (HMD).
> >  	 */
> >  	bool non_desktop;
> > +
> > +	/**
> > +	 * @force_max_lane_count: Link training requires max lane count to pass
> > +	 */
> > +	bool force_max_lane_count;
> >  };
> >  
> >  int drm_display_info_set_bus_formats(struct drm_display_info *info,
> > -- 
> > 2.19.1
> 
> -- 
> Ville Syrjälä
> Intel
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Navare, Manasi April 3, 2019, 6:37 p.m. UTC | #4
On Wed, Apr 03, 2019 at 03:14:51PM +0300, Ville Syrjälä wrote:
> On Tue, Apr 02, 2019 at 02:52:34PM -0700, Manasi Navare wrote:
> > For certain eDP 1.4 panels, we need to use max lane count for the
> > link training to succeed.
> > 
> > This patch adds a EDID quirk for such eDP panels using
> > their vendor ID and product ID to force using max lane count in the driver.
> 
> Rather than opening the quirk can of worms I think we should consider
> changing the retry loop to do something more sensible than what it's
> doing now. The current behaviour of "start at optimal settings (which
> can be either min lanes or min rate), and then reduce lanes/rate until
> stuff works" overlooks several possible combinations. One possible
> approach could be to start the retry loop with max lanes + max rate
> after the optimal settings have failed. It probably won't give you
> the best power consumption, but at least you get a picture on the
> screen if even a single lane count + rate combo works.
>

So you are saying that for eDP only we should modify the retry function to
retry with max lanes and max rate so what we used to do earlier with < eDP 1.4?

Hmm I could try doing that, the only concern I have there is that certain eDP
panels just need a retry at same parameters to work so for such panels
where the lower values of link rate/lane count work with just an extra retry
we would still be using max link rate /lane count now with this change.

Or are you suggesting doing the retry with same params for edp < 1.4 and for all
edp 1.4 , we retry with , max link rate lane ocunt?

Manasi

> > 
> > Cc: Clint Taylor <Clinton.A.Taylor@intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Tested-by: Albert Astals Cid <aacid@kde.org>
> > Tested-by: Emanuele Panigati <ilpanich@gmail.com>
> > Tested-by: Ralgor <ralgorfdb@compuspex.org>
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=109959
> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > ---
> >  drivers/gpu/drm/drm_edid.c  | 10 ++++++++++
> >  include/drm/drm_connector.h |  5 +++++
> >  2 files changed, 15 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > index 2c22ea446075..fbc661806484 100644
> > --- a/drivers/gpu/drm/drm_edid.c
> > +++ b/drivers/gpu/drm/drm_edid.c
> > @@ -82,6 +82,8 @@
> >  #define EDID_QUIRK_FORCE_10BPC			(1 << 11)
> >  /* Non desktop display (i.e. HMD) */
> >  #define EDID_QUIRK_NON_DESKTOP			(1 << 12)
> > +/* Force max lane count */
> > +#define EDID_QUIRK_FORCE_MAX_LANE_COUNT		(1 << 13)
> >  
> >  struct detailed_mode_closure {
> >  	struct drm_connector *connector;
> > @@ -189,6 +191,10 @@ static const struct edid_quirk {
> >  
> >  	/* OSVR HDK and HDK2 VR Headsets */
> >  	{ "SVR", 0x1019, EDID_QUIRK_NON_DESKTOP },
> > +
> > +	/* SHP eDP 1.4 panel only works with max lane count */
> > +	{ "SHP", 0x149a, EDID_QUIRK_FORCE_MAX_LANE_COUNT },
> > +	{ "SHP", 0x148e, EDID_QUIRK_FORCE_MAX_LANE_COUNT },
> >  };
> >  
> >  /*
> > @@ -4463,6 +4469,7 @@ drm_reset_display_info(struct drm_connector *connector)
> >  	memset(&info->hdmi, 0, sizeof(info->hdmi));
> >  
> >  	info->non_desktop = 0;
> > +	info->force_max_lane_count = 0;
> >  }
> >  
> >  u32 drm_add_display_info(struct drm_connector *connector, const struct edid *edid)
> > @@ -4744,6 +4751,9 @@ int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid)
> >  	if (quirks & EDID_QUIRK_FORCE_12BPC)
> >  		connector->display_info.bpc = 12;
> >  
> > +	if (quirks & EDID_QUIRK_FORCE_MAX_LANE_COUNT)
> > +		connector->display_info.force_max_lane_count = true;
> > +
> >  	return num_modes;
> >  }
> >  EXPORT_SYMBOL(drm_add_edid_modes);
> > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > index 02a131202add..45436d40ffe3 100644
> > --- a/include/drm/drm_connector.h
> > +++ b/include/drm/drm_connector.h
> > @@ -457,6 +457,11 @@ struct drm_display_info {
> >  	 * @non_desktop: Non desktop display (HMD).
> >  	 */
> >  	bool non_desktop;
> > +
> > +	/**
> > +	 * @force_max_lane_count: Link training requires max lane count to pass
> > +	 */
> > +	bool force_max_lane_count;
> >  };
> >  
> >  int drm_display_info_set_bus_formats(struct drm_display_info *info,
> > -- 
> > 2.19.1
> 
> -- 
> Ville Syrjälä
> Intel
Ville Syrjala April 3, 2019, 6:55 p.m. UTC | #5
On Wed, Apr 03, 2019 at 11:37:21AM -0700, Manasi Navare wrote:
> On Wed, Apr 03, 2019 at 03:14:51PM +0300, Ville Syrjälä wrote:
> > On Tue, Apr 02, 2019 at 02:52:34PM -0700, Manasi Navare wrote:
> > > For certain eDP 1.4 panels, we need to use max lane count for the
> > > link training to succeed.
> > > 
> > > This patch adds a EDID quirk for such eDP panels using
> > > their vendor ID and product ID to force using max lane count in the driver.
> > 
> > Rather than opening the quirk can of worms I think we should consider
> > changing the retry loop to do something more sensible than what it's
> > doing now. The current behaviour of "start at optimal settings (which
> > can be either min lanes or min rate), and then reduce lanes/rate until
> > stuff works" overlooks several possible combinations. One possible
> > approach could be to start the retry loop with max lanes + max rate
> > after the optimal settings have failed. It probably won't give you
> > the best power consumption, but at least you get a picture on the
> > screen if even a single lane count + rate combo works.
> >
> 
> So you are saying that for eDP only we should modify the retry function to
> retry with max lanes and max rate so what we used to do earlier with < eDP 1.4?
> 
> Hmm I could try doing that, the only concern I have there is that certain eDP
> panels just need a retry at same parameters to work so for such panels
> where the lower values of link rate/lane count work with just an extra retry
> we would still be using max link rate /lane count now with this change.

If the panels are borked then I wouln't worry about it as long a picture
appaears on the screen. And if the extra training cycle is because of
some bug in our code then we should figure it out what that bug is.

> 
> Or are you suggesting doing the retry with same params for edp < 1.4 and for all
> edp 1.4 , we retry with , max link rate lane ocunt?

retrain_fail()
{
	if (!use_max_params) {
		use_max_params = true;
		rate = max;
		lanes = max;
	} else {
		reduce rate/lanes as usual
	}
}

compute_config()
{
	if (use_max_params) {
		limits.min = limits.max;
	}
	...
}

or something like that.
Navare, Manasi April 3, 2019, 7:07 p.m. UTC | #6
On Wed, Apr 03, 2019 at 09:55:56PM +0300, Ville Syrjälä wrote:
> On Wed, Apr 03, 2019 at 11:37:21AM -0700, Manasi Navare wrote:
> > On Wed, Apr 03, 2019 at 03:14:51PM +0300, Ville Syrjälä wrote:
> > > On Tue, Apr 02, 2019 at 02:52:34PM -0700, Manasi Navare wrote:
> > > > For certain eDP 1.4 panels, we need to use max lane count for the
> > > > link training to succeed.
> > > > 
> > > > This patch adds a EDID quirk for such eDP panels using
> > > > their vendor ID and product ID to force using max lane count in the driver.
> > > 
> > > Rather than opening the quirk can of worms I think we should consider
> > > changing the retry loop to do something more sensible than what it's
> > > doing now. The current behaviour of "start at optimal settings (which
> > > can be either min lanes or min rate), and then reduce lanes/rate until
> > > stuff works" overlooks several possible combinations. One possible
> > > approach could be to start the retry loop with max lanes + max rate
> > > after the optimal settings have failed. It probably won't give you
> > > the best power consumption, but at least you get a picture on the
> > > screen if even a single lane count + rate combo works.
> > >
> > 
> > So you are saying that for eDP only we should modify the retry function to
> > retry with max lanes and max rate so what we used to do earlier with < eDP 1.4?
> > 
> > Hmm I could try doing that, the only concern I have there is that certain eDP
> > panels just need a retry at same parameters to work so for such panels
> > where the lower values of link rate/lane count work with just an extra retry
> > we would still be using max link rate /lane count now with this change.
> 
> If the panels are borked then I wouln't worry about it as long a picture
> appaears on the screen. And if the extra training cycle is because of
> some bug in our code then we should figure it out what that bug is.
>

But we should do this retrain() in our existing modeset_retry() and intel_dp_get_fallback_values()
and only do it for eDP right?
Because DP we have the retrain loops as per the compliance, if we change that that will
affect all the compliance tests.

Manasi
 
> > 
> > Or are you suggesting doing the retry with same params for edp < 1.4 and for all
> > edp 1.4 , we retry with , max link rate lane ocunt?
> 
> retrain_fail()
> {
> 	if (!use_max_params) {
> 		use_max_params = true;
> 		rate = max;
> 		lanes = max;
> 	} else {
> 		reduce rate/lanes as usual
> 	}
> }
> 
> compute_config()
> {
> 	if (use_max_params) {
> 		limits.min = limits.max;
> 	}
> 	...
> }
> 
> or something like that.
> 
> -- 
> Ville Syrjälä
> Intel
Ville Syrjala April 3, 2019, 7:22 p.m. UTC | #7
On Wed, Apr 03, 2019 at 12:07:35PM -0700, Manasi Navare wrote:
> On Wed, Apr 03, 2019 at 09:55:56PM +0300, Ville Syrjälä wrote:
> > On Wed, Apr 03, 2019 at 11:37:21AM -0700, Manasi Navare wrote:
> > > On Wed, Apr 03, 2019 at 03:14:51PM +0300, Ville Syrjälä wrote:
> > > > On Tue, Apr 02, 2019 at 02:52:34PM -0700, Manasi Navare wrote:
> > > > > For certain eDP 1.4 panels, we need to use max lane count for the
> > > > > link training to succeed.
> > > > > 
> > > > > This patch adds a EDID quirk for such eDP panels using
> > > > > their vendor ID and product ID to force using max lane count in the driver.
> > > > 
> > > > Rather than opening the quirk can of worms I think we should consider
> > > > changing the retry loop to do something more sensible than what it's
> > > > doing now. The current behaviour of "start at optimal settings (which
> > > > can be either min lanes or min rate), and then reduce lanes/rate until
> > > > stuff works" overlooks several possible combinations. One possible
> > > > approach could be to start the retry loop with max lanes + max rate
> > > > after the optimal settings have failed. It probably won't give you
> > > > the best power consumption, but at least you get a picture on the
> > > > screen if even a single lane count + rate combo works.
> > > >
> > > 
> > > So you are saying that for eDP only we should modify the retry function to
> > > retry with max lanes and max rate so what we used to do earlier with < eDP 1.4?
> > > 
> > > Hmm I could try doing that, the only concern I have there is that certain eDP
> > > panels just need a retry at same parameters to work so for such panels
> > > where the lower values of link rate/lane count work with just an extra retry
> > > we would still be using max link rate /lane count now with this change.
> > 
> > If the panels are borked then I wouln't worry about it as long a picture
> > appaears on the screen. And if the extra training cycle is because of
> > some bug in our code then we should figure it out what that bug is.
> >
> 
> But we should do this retrain() in our existing modeset_retry() and intel_dp_get_fallback_values()
> and only do it for eDP right?
> Because DP we have the retrain loops as per the compliance, if we change that that will
> affect all the compliance tests.

I have a rather low opinion on the compliance tests. If they are
preventing sensible real world things then what good are they?
Someone should probably figure out what it would take to make
them more useful...

But anyways, I guess we can start with eDP. If it looks like
DP could benefit as well we might have to consider it.
Navare, Manasi April 3, 2019, 7:52 p.m. UTC | #8
On Wed, Apr 03, 2019 at 10:22:16PM +0300, Ville Syrjälä wrote:
> On Wed, Apr 03, 2019 at 12:07:35PM -0700, Manasi Navare wrote:
> > On Wed, Apr 03, 2019 at 09:55:56PM +0300, Ville Syrjälä wrote:
> > > On Wed, Apr 03, 2019 at 11:37:21AM -0700, Manasi Navare wrote:
> > > > On Wed, Apr 03, 2019 at 03:14:51PM +0300, Ville Syrjälä wrote:
> > > > > On Tue, Apr 02, 2019 at 02:52:34PM -0700, Manasi Navare wrote:
> > > > > > For certain eDP 1.4 panels, we need to use max lane count for the
> > > > > > link training to succeed.
> > > > > > 
> > > > > > This patch adds a EDID quirk for such eDP panels using
> > > > > > their vendor ID and product ID to force using max lane count in the driver.
> > > > > 
> > > > > Rather than opening the quirk can of worms I think we should consider
> > > > > changing the retry loop to do something more sensible than what it's
> > > > > doing now. The current behaviour of "start at optimal settings (which
> > > > > can be either min lanes or min rate), and then reduce lanes/rate until
> > > > > stuff works" overlooks several possible combinations. One possible
> > > > > approach could be to start the retry loop with max lanes + max rate
> > > > > after the optimal settings have failed. It probably won't give you
> > > > > the best power consumption, but at least you get a picture on the
> > > > > screen if even a single lane count + rate combo works.
> > > > >
> > > > 
> > > > So you are saying that for eDP only we should modify the retry function to
> > > > retry with max lanes and max rate so what we used to do earlier with < eDP 1.4?
> > > > 
> > > > Hmm I could try doing that, the only concern I have there is that certain eDP
> > > > panels just need a retry at same parameters to work so for such panels
> > > > where the lower values of link rate/lane count work with just an extra retry
> > > > we would still be using max link rate /lane count now with this change.
> > > 
> > > If the panels are borked then I wouln't worry about it as long a picture
> > > appaears on the screen. And if the extra training cycle is because of
> > > some bug in our code then we should figure it out what that bug is.
> > >
> > 
> > But we should do this retrain() in our existing modeset_retry() and intel_dp_get_fallback_values()
> > and only do it for eDP right?
> > Because DP we have the retrain loops as per the compliance, if we change that that will
> > affect all the compliance tests.
> 
> I have a rather low opinion on the compliance tests. If they are
> preventing sensible real world things then what good are they?
> Someone should probably figure out what it would take to make
> them more useful...
> 
> But anyways, I guess we can start with eDP. If it looks like
> DP could benefit as well we might have to consider it.
>

I agree, i will add a patch for edp first and have the folks test it with the
edp panels that needed the quirk.
DP we really havent had any similar failures where we absolutely need max vales.

Manasi
 
> -- 
> Ville Syrjälä
> Intel
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
diff mbox series

Patch

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 2c22ea446075..fbc661806484 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -82,6 +82,8 @@ 
 #define EDID_QUIRK_FORCE_10BPC			(1 << 11)
 /* Non desktop display (i.e. HMD) */
 #define EDID_QUIRK_NON_DESKTOP			(1 << 12)
+/* Force max lane count */
+#define EDID_QUIRK_FORCE_MAX_LANE_COUNT		(1 << 13)
 
 struct detailed_mode_closure {
 	struct drm_connector *connector;
@@ -189,6 +191,10 @@  static const struct edid_quirk {
 
 	/* OSVR HDK and HDK2 VR Headsets */
 	{ "SVR", 0x1019, EDID_QUIRK_NON_DESKTOP },
+
+	/* SHP eDP 1.4 panel only works with max lane count */
+	{ "SHP", 0x149a, EDID_QUIRK_FORCE_MAX_LANE_COUNT },
+	{ "SHP", 0x148e, EDID_QUIRK_FORCE_MAX_LANE_COUNT },
 };
 
 /*
@@ -4463,6 +4469,7 @@  drm_reset_display_info(struct drm_connector *connector)
 	memset(&info->hdmi, 0, sizeof(info->hdmi));
 
 	info->non_desktop = 0;
+	info->force_max_lane_count = 0;
 }
 
 u32 drm_add_display_info(struct drm_connector *connector, const struct edid *edid)
@@ -4744,6 +4751,9 @@  int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid)
 	if (quirks & EDID_QUIRK_FORCE_12BPC)
 		connector->display_info.bpc = 12;
 
+	if (quirks & EDID_QUIRK_FORCE_MAX_LANE_COUNT)
+		connector->display_info.force_max_lane_count = true;
+
 	return num_modes;
 }
 EXPORT_SYMBOL(drm_add_edid_modes);
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 02a131202add..45436d40ffe3 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -457,6 +457,11 @@  struct drm_display_info {
 	 * @non_desktop: Non desktop display (HMD).
 	 */
 	bool non_desktop;
+
+	/**
+	 * @force_max_lane_count: Link training requires max lane count to pass
+	 */
+	bool force_max_lane_count;
 };
 
 int drm_display_info_set_bus_formats(struct drm_display_info *info,