diff mbox

-next queue and EDID stuff

Message ID 503D9810.60104@gmail.com
State New, archived
Headers show

Commit Message

Ian Pilcher Aug. 29, 2012, 4:18 a.m. UTC
On 08/28/2012 07:13 PM, Adam Jackson wrote:
> On 8/28/12 7:33 PM, Ian Pilcher wrote:
>> Actually, I believe that the error is probably in the Intel driver.  As
>> I understand it, it shouldn't be sending audio InfoFrames to a non-HDMI
>> display.
> 
> If that's the case then I'd still say "we're doing something else wrong
> here".  Quirks - at least at the core drm level - are not for working
> around broken drivers, they're for working around broken displays.

And I'd agree.

(Although I suppose one *could* argue that the display is broken in 2
ways -- it reports audio capabilities that aren't really there, and it
gets confused by any InfoFrames -- AVI or audio.)

I don't have the knowledge or time to fix the Intel driver, but I've
always planned to at least bugzilla the issue.  I can't reasonably do
so, however, until the user-defined quirks infrastructure is in place,
so that the behavior can be demonstrated.

If you prefer to leave the display broken with Intel GPUs, you can
always just remove the EDID_QUIRK_NO_AUDIO flag:

 	 * provide some room for user-supplied quirks.

Comments

Daniel Vetter Aug. 29, 2012, 7:56 a.m. UTC | #1
On Tue, Aug 28, 2012 at 11:18:24PM -0500, Ian Pilcher wrote:
> On 08/28/2012 07:13 PM, Adam Jackson wrote:
> > On 8/28/12 7:33 PM, Ian Pilcher wrote:
> >> Actually, I believe that the error is probably in the Intel driver.  As
> >> I understand it, it shouldn't be sending audio InfoFrames to a non-HDMI
> >> display.
> > 
> > If that's the case then I'd still say "we're doing something else wrong
> > here".  Quirks - at least at the core drm level - are not for working
> > around broken drivers, they're for working around broken displays.
> 
> And I'd agree.
> 
> (Although I suppose one *could* argue that the display is broken in 2
> ways -- it reports audio capabilities that aren't really there, and it
> gets confused by any InfoFrames -- AVI or audio.)
> 
> I don't have the knowledge or time to fix the Intel driver, but I've
> always planned to at least bugzilla the issue.  I can't reasonably do
> so, however, until the user-defined quirks infrastructure is in place,
> so that the behavior can be demonstrated.
> 
> If you prefer to leave the display broken with Intel GPUs, you can
> always just remove the EDID_QUIRK_NO_AUDIO flag:

Wrt intel infoframes issues: Have you retested on latest 3.6-rc kernels?
We've fixed quite a few bugs for our infoframe support recently ...
-Daniel

> 
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -160,6 +160,10 @@  union edid_quirk
> edid_quirk_list[EDID_QUIRK_LIST_SIZE] = {
>  	{ { { { EDID_MFG_ID('V', 'S', 'C'), cpu_to_le16(5020) } },
>  		EDID_QUIRK_FORCE_REDUCED_BLANKING } },
> 
> +	/* LG L246WP */
> +	{ { { { EDID_MFG_ID('G', 'S', 'M'), cpu_to_le16(0x563f) } },
> +		EDID_QUIRK_DISABLE_INFOFRAMES } },
> +
>  	/*
>  	 * When adding built-in quirks, please adjust EDID_QUIRK_LIST_SIZE to
>  	 * provide some room for user-supplied quirks.
> 
> -- 
> ========================================================================
> Ian Pilcher                                         arequipeno@gmail.com
> "If you're going to shift my paradigm ... at least buy me dinner first."
> ========================================================================
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
Ian Pilcher Aug. 29, 2012, 7:53 p.m. UTC | #2
On 08/29/2012 02:56 AM, Daniel Vetter wrote:
> Wrt intel infoframes issues: Have you retested on latest 3.6-rc kernels?
> We've fixed quite a few bugs for our infoframe support recently ...

Tested just now.  The behavior has changed.  I now need to use *both*
flags to make my display work with an Intel GPU.  (Previously, the
NO_AUDIO flag was sufficient to make the display work with Intel
graphics.)  Presumably, the Intel driver is now sending AVI InfoFrames.

Thinking a bit more about this, I'm starting to rethink my assertion
that the Intel driver is at fault here.  On one hand, it doesn't make
any sense to send audio InfoFrames to a non-HDMI display.  (BTW, I'm
assuming that a display with a DisplayPort port will show up as HDMI.)

On the other hand, it can be argued that the DRM layer is giving
conflicting information to the driver -- drm_detect_hdmi_monitor is
returning FALSE, but drm_detect_monitor_audio is returning TRUE.  AFAIK,
this doesn't make any sense either.

This leads me to propose that drm_detect_monitor_audio return FALSE if
either EDID_QUIRK_DISABLE_INFOFRAMES or EDID_QUIRK_NO_AUDIO is set.

bool drm_detect_monitor_audio(struct edid *edid)
{
         u8 *edid_ext;
         int i, j;
         bool has_audio = false;
         int start_offset, end_offset;
         char buf[EDID_DISPLAY_ID_BUF_SIZE];

         if (edid_get_quirks(edid) & (EDID_QUIRK_NO_AUDIO |
                                      EDID_QUIRK_DISABLE_INFOFRAMES)) {
                 DRM_INFO("Disabling HDMI audio on display %s "
                          "due to EDID quirk\n",
                          drm_edid_display_id_format(edid->display_id,
                                                     buf, 1));
                 goto end;
         }

         ...

EDID_QUIRK_DISABLE_INFOFRAMES would then be sufficient to make the LG
L246WP work with both nVidia and Intel GPUs.  (ATI GPUs should work as
well; I just don't have any hardware to test.)

Ajax - Does this address your objections?  If so, I'll spin another
patch set.

Thanks!
Adam Jackson Aug. 29, 2012, 9:38 p.m. UTC | #3
On 8/29/12 3:53 PM, Ian Pilcher wrote:

> Thinking a bit more about this, I'm starting to rethink my assertion
> that the Intel driver is at fault here.  On one hand, it doesn't make
> any sense to send audio InfoFrames to a non-HDMI display.  (BTW, I'm
> assuming that a display with a DisplayPort port will show up as HDMI.)

Nope.

If you connect to the DP port on a monitor, it will claim to be DP: the 
EDID block version will be 1.4, and the digital display feature byte 
will indicate that it's DP:

http://cgit.freedesktop.org/xorg/app/edid-decode/tree/edid-decode.c#n1145

If you connect to the HDMI port on a monitor, it will claim to be HDMI: 
the EDID block version will be 1.3 or higher, there will be a CEA 
extension block, and there will be a vendor-specific data block within 
that extension with a vendor OUI matching that of the HDMI consortium:

http://cgit.freedesktop.org/xorg/app/edid-decode/tree/edid-decode.c#n640

> On the other hand, it can be argued that the DRM layer is giving
> conflicting information to the driver -- drm_detect_hdmi_monitor is
> returning FALSE, but drm_detect_monitor_audio is returning TRUE.  AFAIK,
> this doesn't make any sense either.

This might be the actual issue.  drm_detect_monitor_audio() can return 
true for several reasons, but it does _not_ make any claim that the 
monitor is HDMI, only that it has a CEA extension block.  Which is 
certainly ugly, but that's how CEA defined their botch of an extension 
block.

So to me, that says that drivers need to ask _both_ whether the monitor 
supports audio and whether it's HDMI before sending HDMI-specific audio 
signalling.

- ajax
Ian Pilcher Aug. 30, 2012, 5:23 a.m. UTC | #4
On 08/29/2012 04:38 PM, Adam Jackson wrote:
> So to me, that says that drivers need to ask _both_ whether the monitor
> supports audio and whether it's HDMI before sending HDMI-specific audio
> signalling.

OK, so the burden is on the individual drivers.

In terms of moving forward with the rest of the EDID quirk stuff, are
you OK with either of these options:

* Remove EDID_QUIRK_NO_AUDIO from the flags for the LG L246WP.  It won't
  work "out of the box" with the Intel driver until that driver is
  fixed to not send audio InfoFrames to non-HDMI displays (but anyone
  who has the combination will be able to add their own quirk to make
  it work).

* Leave both flags, because both are accurate.  The display is confused
  by any InfoFrames (audio or AVI), and it is reporting non-existent
  audio capabilities.  The fact that the combination of the two flags
  makes the display work with Intel GPUs is a happy/sad/neutral
  accident, and the driver's behavior is still considered broken.

  (Essentially, this would mean just editing the comments to reflect the
  above reasoning.)

If needed, I can easily create a new patch for either option.  Just let
me know.

Thanks!
Daniel Vetter Aug. 30, 2012, 7:41 a.m. UTC | #5
On Thu, Aug 30, 2012 at 12:23:43AM -0500, Ian Pilcher wrote:
> On 08/29/2012 04:38 PM, Adam Jackson wrote:
> > So to me, that says that drivers need to ask _both_ whether the monitor
> > supports audio and whether it's HDMI before sending HDMI-specific audio
> > signalling.
> 
> OK, so the burden is on the individual drivers.
> 
> In terms of moving forward with the rest of the EDID quirk stuff, are
> you OK with either of these options:
> 
> * Remove EDID_QUIRK_NO_AUDIO from the flags for the LG L246WP.  It won't
>   work "out of the box" with the Intel driver until that driver is
>   fixed to not send audio InfoFrames to non-HDMI displays (but anyone
>   who has the combination will be able to add their own quirk to make
>   it work).
> 
> * Leave both flags, because both are accurate.  The display is confused
>   by any InfoFrames (audio or AVI), and it is reporting non-existent
>   audio capabilities.  The fact that the combination of the two flags
>   makes the display work with Intel GPUs is a happy/sad/neutral
>   accident, and the driver's behavior is still considered broken.
> 
>   (Essentially, this would mean just editing the comments to reflect the
>   above reasoning.)
> 
> If needed, I can easily create a new patch for either option.  Just let
> me know.

I've decided that simply fixing the intel driver is easier. Patch on it's
way, please test.

Thanks, Daniel
Ian Pilcher Sept. 4, 2012, 2:02 p.m. UTC | #6
On 08/30/2012 12:23 AM, Ian Pilcher wrote:
> 
> * Remove EDID_QUIRK_NO_AUDIO from the flags for the LG L246WP.  It won't
>   work "out of the box" with the Intel driver until that driver is
>   fixed to not send audio InfoFrames to non-HDMI displays (but anyone
>   who has the combination will be able to add their own quirk to make
>   it work).
> 
> * Leave both flags, because both are accurate.  The display is confused
>   by any InfoFrames (audio or AVI), and it is reporting non-existent
>   audio capabilities.  The fact that the combination of the two flags
>   makes the display work with Intel GPUs is a happy/sad/neutral
>   accident, and the driver's behavior is still considered broken.
> 
>   (Essentially, this would mean just editing the comments to reflect the
>   above reasoning.)

Ajax -

I *really* want to close the loop on this.  If you can tell me which of
the above options you prefer, I will create a new patch set, including
the drm_monitor_has_hdmi_audio() function that you suggested in your
note on 8/30.

Thanks!
Ian Pilcher Sept. 11, 2012, 1:59 p.m. UTC | #7
Ping

On 09/04/2012 09:02 AM, Ian Pilcher wrote:
> On 08/30/2012 12:23 AM, Ian Pilcher wrote:
>>
>> * Remove EDID_QUIRK_NO_AUDIO from the flags for the LG L246WP.  It won't
>>   work "out of the box" with the Intel driver until that driver is
>>   fixed to not send audio InfoFrames to non-HDMI displays (but anyone
>>   who has the combination will be able to add their own quirk to make
>>   it work).
>>
>> * Leave both flags, because both are accurate.  The display is confused
>>   by any InfoFrames (audio or AVI), and it is reporting non-existent
>>   audio capabilities.  The fact that the combination of the two flags
>>   makes the display work with Intel GPUs is a happy/sad/neutral
>>   accident, and the driver's behavior is still considered broken.
>>
>>   (Essentially, this would mean just editing the comments to reflect the
>>   above reasoning.)
> 
> Ajax -
> 
> I *really* want to close the loop on this.  If you can tell me which of
> the above options you prefer, I will create a new patch set, including
> the drm_monitor_has_hdmi_audio() function that you suggested in your
> note on 8/30.
> 
> Thanks!
>
diff mbox

Patch

--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -160,6 +160,10 @@   union edid_quirk
edid_quirk_list[EDID_QUIRK_LIST_SIZE] = {
 	{ { { { EDID_MFG_ID('V', 'S', 'C'), cpu_to_le16(5020) } },
 		EDID_QUIRK_FORCE_REDUCED_BLANKING } },

+	/* LG L246WP */
+	{ { { { EDID_MFG_ID('G', 'S', 'M'), cpu_to_le16(0x563f) } },
+		EDID_QUIRK_DISABLE_INFOFRAMES } },
+
 	/*
 	 * When adding built-in quirks, please adjust EDID_QUIRK_LIST_SIZE to