Re: drivers/gpu/drm/i915/intel_dp.c problem with commit dc22ee6fc18ce0f15424e753e8473c306ece95c1

Re: drivers/gpu/drm/i915/intel_dp.c problem with commit dc22ee6fc18ce0f15424e753e8473c306ece95c1

[Keith Packard]

[-- Attachment #1: Type: text/plain, Size: 2204 bytes --]

On Sat, 26 Nov 2011 09:50:41 -0800, Jerker Buud <b.jerker@gmail.com> wrote:

> 245,247c245,246
> <       if (!is_edp(intel_dp) &&
> <           (intel_dp_link_required(intel_dp, mode->clock)
> <            > intel_dp_max_data_rate(max_link_clock, max_lanes)))
> ---
> >       if (intel_dp_link_required(intel_dp, mode->clock)
> >           > intel_dp_max_data_rate(max_link_clock, max_lanes))

Ok, I think I understand why this change is required.

These machines are 'optimized' to provide the bare minimum connection
necessary between the CPU and the eDP panel -- a single lane is hooked
up, which is just enough to run the link at 18bpp, but not enough to run
it at 24bpp.

The problem here (in intel_dp_link_required) is that this is being
called to see which modes can be supported on the panel. Without the
CRTC being configured with the appropriate bpp value, this code assumes
24bpp.

I thought Adam Jackson had some code around that automatically switched
From 24bpp to 18bpp when the link didn't have enough bandwidth for
24bpp, but I can't find that now.

In any case, I think we need to centralize the bpp selection inside
intel_dp.c instead of having it in both places. Then, we can make that
depend on the requested mode as well as the machine configuration.

Here's a patch which uses the VBT configured bpp value in this case. I'd
love to hear whether this suffices to resolve your problem.

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 294f557..3e15479 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -212,10 +212,16 @@ intel_dp_link_required(struct intel_dp *intel_dp, int pixel_clock)
 {
 	struct drm_crtc *crtc = intel_dp->base.base.crtc;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	int bpp = 24;
+	struct drm_device *dev = intel_dp->base.base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	int bpp;
 
 	if (intel_crtc)
 		bpp = intel_crtc->bpp;
+	else if (intel_dp->base.type == INTEL_OUTPUT_EDP)
+		bpp = dev_priv->edp.bpp;
+	else
+		bpp = 24;
 
 	return (pixel_clock * bpp + 9) / 10;
 }

-- 
keith.packard@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 827 bytes --]

Re: drivers/gpu/drm/i915/intel_dp.c problem with commit dc22ee6fc18ce0f15424e753e8473c306ece95c1

[Patrik Kullman]

I checked out a clean v3.2-rc3, applied this patch and rebooted.

It still doesn't work and Caps Lock still doesn't respond, but I get
very subtle flickering of the screen, 2-3 seconds apart, kind of like
different shades of black.

It 'feels' as if some graphics card/link/monitor negotiation is taking place.

2011/11/26 Keith Packard <keith.packard@intel.com>:
> On Sat, 26 Nov 2011 09:50:41 -0800, Jerker Buud <b.jerker@gmail.com> wrote:
>
>> 245,247c245,246
>> <       if (!is_edp(intel_dp) &&
>> <           (intel_dp_link_required(intel_dp, mode->clock)
>> <            > intel_dp_max_data_rate(max_link_clock, max_lanes)))
>> ---
>> >       if (intel_dp_link_required(intel_dp, mode->clock)
>> >           > intel_dp_max_data_rate(max_link_clock, max_lanes))
>
> Ok, I think I understand why this change is required.
>
> These machines are 'optimized' to provide the bare minimum connection
> necessary between the CPU and the eDP panel -- a single lane is hooked
> up, which is just enough to run the link at 18bpp, but not enough to run
> it at 24bpp.
>
> The problem here (in intel_dp_link_required) is that this is being
> called to see which modes can be supported on the panel. Without the
> CRTC being configured with the appropriate bpp value, this code assumes
> 24bpp.
>
> I thought Adam Jackson had some code around that automatically switched
> From 24bpp to 18bpp when the link didn't have enough bandwidth for
> 24bpp, but I can't find that now.
>
> In any case, I think we need to centralize the bpp selection inside
> intel_dp.c instead of having it in both places. Then, we can make that
> depend on the requested mode as well as the machine configuration.
>
> Here's a patch which uses the VBT configured bpp value in this case. I'd
> love to hear whether this suffices to resolve your problem.
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 294f557..3e15479 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -212,10 +212,16 @@ intel_dp_link_required(struct intel_dp *intel_dp, int pixel_clock)
>  {
>        struct drm_crtc *crtc = intel_dp->base.base.crtc;
>        struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -       int bpp = 24;
> +       struct drm_device *dev = intel_dp->base.base.dev;
> +       struct drm_i915_private *dev_priv = dev->dev_private;
> +       int bpp;
>
>        if (intel_crtc)
>                bpp = intel_crtc->bpp;
> +       else if (intel_dp->base.type == INTEL_OUTPUT_EDP)
> +               bpp = dev_priv->edp.bpp;
> +       else
> +               bpp = 24;
>
>        return (pixel_clock * bpp + 9) / 10;
>  }
>
> --
> keith.packard@intel.com

Re: drivers/gpu/drm/i915/intel_dp.c problem with commit dc22ee6fc18ce0f15424e753e8473c306ece95c1

[Patrik Kullman]

Not sure if this is somehow related, but I experienced a weird issue
with the HDMI-output today.

I connected the computer to a (Acer) projector at work to hold a
presentation, but it didn't work.
In GNOME Displays, I could see a connected "Acer Technologies" screen,
its supported resolutions listed, I could even move the cursor as if a
screen was connected. Everything about Linux/GNOME seems to "think"
everything was working.

On the projector side of things, the Acer-logo was showing and the
source (HDMI1/2) was showing.
It didn't say "No Signal" as it can, but it didn't either come as far
as to "Process signal", which usually appears a moment before
everything is up and running.

Booting up Windows, everything worked perfectly.

When I got home, I connected the computer to my LCD TV and watched a
video without any issues.
Even sound through HDMI works great.

Please let me know if you think this might be related or if I should
start a new thread about this.

2011/11/29 Patrik Kullman <patrik.kullman@gmail.com>:
> I checked out a clean v3.2-rc3, applied this patch and rebooted.
>
> It still doesn't work and Caps Lock still doesn't respond, but I get
> very subtle flickering of the screen, 2-3 seconds apart, kind of like
> different shades of black.
>
> It 'feels' as if some graphics card/link/monitor negotiation is taking place.
>
> 2011/11/26 Keith Packard <keith.packard@intel.com>:
>> On Sat, 26 Nov 2011 09:50:41 -0800, Jerker Buud <b.jerker@gmail.com> wrote:
>>
>>> 245,247c245,246
>>> <       if (!is_edp(intel_dp) &&
>>> <           (intel_dp_link_required(intel_dp, mode->clock)
>>> <            > intel_dp_max_data_rate(max_link_clock, max_lanes)))
>>> ---
>>> >       if (intel_dp_link_required(intel_dp, mode->clock)
>>> >           > intel_dp_max_data_rate(max_link_clock, max_lanes))
>>
>> Ok, I think I understand why this change is required.
>>
>> These machines are 'optimized' to provide the bare minimum connection
>> necessary between the CPU and the eDP panel -- a single lane is hooked
>> up, which is just enough to run the link at 18bpp, but not enough to run
>> it at 24bpp.
>>
>> The problem here (in intel_dp_link_required) is that this is being
>> called to see which modes can be supported on the panel. Without the
>> CRTC being configured with the appropriate bpp value, this code assumes
>> 24bpp.
>>
>> I thought Adam Jackson had some code around that automatically switched
>> From 24bpp to 18bpp when the link didn't have enough bandwidth for
>> 24bpp, but I can't find that now.
>>
>> In any case, I think we need to centralize the bpp selection inside
>> intel_dp.c instead of having it in both places. Then, we can make that
>> depend on the requested mode as well as the machine configuration.
>>
>> Here's a patch which uses the VBT configured bpp value in this case. I'd
>> love to hear whether this suffices to resolve your problem.
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index 294f557..3e15479 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -212,10 +212,16 @@ intel_dp_link_required(struct intel_dp *intel_dp, int pixel_clock)
>>  {
>>        struct drm_crtc *crtc = intel_dp->base.base.crtc;
>>        struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>> -       int bpp = 24;
>> +       struct drm_device *dev = intel_dp->base.base.dev;
>> +       struct drm_i915_private *dev_priv = dev->dev_private;
>> +       int bpp;
>>
>>        if (intel_crtc)
>>                bpp = intel_crtc->bpp;
>> +       else if (intel_dp->base.type == INTEL_OUTPUT_EDP)
>> +               bpp = dev_priv->edp.bpp;
>> +       else
>> +               bpp = 24;
>>
>>        return (pixel_clock * bpp + 9) / 10;
>>  }
>>
>> --
>> keith.packard@intel.com