[PATCH] drm/i915: Rip out SUPPORTS_EDP

[PATCH] drm/i915: Rip out SUPPORTS_EDP

[Daniel Vetter]

It only controls the setting of the vbt.edp_support variable, which in
turn only controls one debug output plus can also force-disable the
lvds output.

Since the value only restricted this logic to mobile ilk there's the
slight risk that this will break lvds on desktop ilk or on snb/ivb
platforms. But with the vbt it's better when we know what's going on
here, so let's rip it out and see what happens.

Cc: Ben Widawsky <benjamin.widawsky@intel.com>
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_drv.h   | 1 -
 drivers/gpu/drm/i915/intel_bios.c | 6 ++----
 2 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index b1485a0..07de53c 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1672,7 +1672,6 @@ struct drm_i915_file_private {
 #define SUPPORTS_DIGITAL_OUTPUTS(dev)	(!IS_GEN2(dev) && !IS_PINEVIEW(dev))
 #define SUPPORTS_INTEGRATED_HDMI(dev)	(IS_G4X(dev) || IS_GEN5(dev))
 #define SUPPORTS_INTEGRATED_DP(dev)	(IS_G4X(dev) || IS_GEN5(dev))
-#define SUPPORTS_EDP(dev)		(IS_IRONLAKE_M(dev))
 #define SUPPORTS_TV(dev)		(INTEL_INFO(dev)->supports_tv)
 #define I915_HAS_HOTPLUG(dev)		 (INTEL_INFO(dev)->has_hotplug)
 
diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
index 0492b6f..e29bcae 100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -477,15 +477,13 @@ static void
 parse_driver_features(struct drm_i915_private *dev_priv,
 		       struct bdb_header *bdb)
 {
-	struct drm_device *dev = dev_priv->dev;
 	struct bdb_driver_features *driver;
 
 	driver = find_section(bdb, BDB_DRIVER_FEATURES);
 	if (!driver)
 		return;
 
-	if (SUPPORTS_EDP(dev) &&
-	    driver->lvds_config == BDB_DRIVER_FEATURE_EDP)
+	if (driver->lvds_config == BDB_DRIVER_FEATURE_EDP)
 		dev_priv->vbt.edp_support = 1;
 
 	if (driver->dual_frequency)
@@ -501,7 +499,7 @@ parse_edp(struct drm_i915_private *dev_priv, struct bdb_header *bdb)
 
 	edp = find_section(bdb, BDB_EDP);
 	if (!edp) {
-		if (SUPPORTS_EDP(dev_priv->dev) && dev_priv->vbt.edp_support)
+		if (dev_priv->vbt.edp_support)
 			DRM_DEBUG_KMS("No eDP BDB found but eDP panel supported.\n");
 		return;
 	}
-- 
1.8.4.rc3

Re: [PATCH] drm/i915: Rip out SUPPORTS_EDP

[Paulo Zanoni]

2013/9/21 Daniel Vetter <daniel.vetter@ffwll.ch>:
> It only controls the setting of the vbt.edp_support variable, which in
> turn only controls one debug output plus can also force-disable the
> lvds output.
>
> Since the value only restricted this logic to mobile ilk there's the
> slight risk that this will break lvds on desktop ilk or on snb/ivb
> platforms. But with the vbt it's better when we know what's going on
> here, so let's rip it out and see what happens.

I think this is a little bit too risky, but the patch does what it
says in the box and also mentions the risk... I was going to give a
Reviewed-by tag and leave the merge-or-not-merge question to the
maintainer, but I noticed the patch is already even merged (by
accident?).

Are we aware of any real-world machines with eDP + LVDS?

>
> Cc: Ben Widawsky <benjamin.widawsky@intel.com>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/i915_drv.h   | 1 -
>  drivers/gpu/drm/i915/intel_bios.c | 6 ++----
>  2 files changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index b1485a0..07de53c 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1672,7 +1672,6 @@ struct drm_i915_file_private {
>  #define SUPPORTS_DIGITAL_OUTPUTS(dev)  (!IS_GEN2(dev) && !IS_PINEVIEW(dev))
>  #define SUPPORTS_INTEGRATED_HDMI(dev)  (IS_G4X(dev) || IS_GEN5(dev))
>  #define SUPPORTS_INTEGRATED_DP(dev)    (IS_G4X(dev) || IS_GEN5(dev))
> -#define SUPPORTS_EDP(dev)              (IS_IRONLAKE_M(dev))
>  #define SUPPORTS_TV(dev)               (INTEL_INFO(dev)->supports_tv)
>  #define I915_HAS_HOTPLUG(dev)           (INTEL_INFO(dev)->has_hotplug)
>
> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> index 0492b6f..e29bcae 100644
> --- a/drivers/gpu/drm/i915/intel_bios.c
> +++ b/drivers/gpu/drm/i915/intel_bios.c
> @@ -477,15 +477,13 @@ static void
>  parse_driver_features(struct drm_i915_private *dev_priv,
>                        struct bdb_header *bdb)
>  {
> -       struct drm_device *dev = dev_priv->dev;
>         struct bdb_driver_features *driver;
>
>         driver = find_section(bdb, BDB_DRIVER_FEATURES);
>         if (!driver)
>                 return;
>
> -       if (SUPPORTS_EDP(dev) &&
> -           driver->lvds_config == BDB_DRIVER_FEATURE_EDP)
> +       if (driver->lvds_config == BDB_DRIVER_FEATURE_EDP)
>                 dev_priv->vbt.edp_support = 1;
>
>         if (driver->dual_frequency)
> @@ -501,7 +499,7 @@ parse_edp(struct drm_i915_private *dev_priv, struct bdb_header *bdb)
>
>         edp = find_section(bdb, BDB_EDP);
>         if (!edp) {
> -               if (SUPPORTS_EDP(dev_priv->dev) && dev_priv->vbt.edp_support)
> +               if (dev_priv->vbt.edp_support)
>                         DRM_DEBUG_KMS("No eDP BDB found but eDP panel supported.\n");
>                 return;
>         }
> --
> 1.8.4.rc3
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH] drm/i915: Rip out SUPPORTS_EDP

[Daniel Vetter]

On Mon, Sep 23, 2013 at 02:04:43PM -0300, Paulo Zanoni wrote:
> 2013/9/21 Daniel Vetter <daniel.vetter@ffwll.ch>:
> > It only controls the setting of the vbt.edp_support variable, which in
> > turn only controls one debug output plus can also force-disable the
> > lvds output.
> >
> > Since the value only restricted this logic to mobile ilk there's the
> > slight risk that this will break lvds on desktop ilk or on snb/ivb
> > platforms. But with the vbt it's better when we know what's going on
> > here, so let's rip it out and see what happens.
> 
> I think this is a little bit too risky, but the patch does what it
> says in the box and also mentions the risk... I was going to give a
> Reviewed-by tag and leave the merge-or-not-merge question to the
> maintainer, but I noticed the patch is already even merged (by
> accident?).

I was lazy and didn't create a seperate branch for it ;-) Can I still
smash your r-b on top?

> Are we aware of any real-world machines with eDP + LVDS?

I haven't seen any of those. What has been observed in the wild is lvds +
sdvo lvds, but I've never seen edp + some other panel anywhere ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: Rip out SUPPORTS_EDP

[Paulo Zanoni]

2013/9/24 Daniel Vetter <daniel@ffwll.ch>:
> On Mon, Sep 23, 2013 at 02:04:43PM -0300, Paulo Zanoni wrote:
>> 2013/9/21 Daniel Vetter <daniel.vetter@ffwll.ch>:
>> > It only controls the setting of the vbt.edp_support variable, which in
>> > turn only controls one debug output plus can also force-disable the
>> > lvds output.
>> >
>> > Since the value only restricted this logic to mobile ilk there's the
>> > slight risk that this will break lvds on desktop ilk or on snb/ivb
>> > platforms. But with the vbt it's better when we know what's going on
>> > here, so let's rip it out and see what happens.
>>
>> I think this is a little bit too risky, but the patch does what it
>> says in the box and also mentions the risk... I was going to give a
>> Reviewed-by tag and leave the merge-or-not-merge question to the
>> maintainer, but I noticed the patch is already even merged (by
>> accident?).
>
> I was lazy and didn't create a seperate branch for it ;-) Can I still
> smash your r-b on top?

Yes. Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

>
>> Are we aware of any real-world machines with eDP + LVDS?
>
> I haven't seen any of those. What has been observed in the wild is lvds +
> sdvo lvds, but I've never seen edp + some other panel anywhere ...

Now that I think more about it, we only have 1 set of panel power
sequencing registers. How would we do if we had 2 panels?


> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch



-- 
Paulo Zanoni

Re: [PATCH] drm/i915: Rip out SUPPORTS_EDP

[Daniel Vetter]

On Tue, Sep 24, 2013 at 3:05 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
>> I haven't seen any of those. What has been observed in the wild is lvds +
>> sdvo lvds, but I've never seen edp + some other panel anywhere ...
>
> Now that I think more about it, we only have 1 set of panel power
> sequencing registers. How would we do if we had 2 panels?

Iirc the sdvo encoder has it's own independent panel power sequencing
logic. That might explain some of the excessive retry loops we have in
that code in there ;-)

vlv has 2 sets of native panel power sequencers afaik, but vlv only
has edp support. So that would be 2x edp there.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch