-next queue and EDID stuff

-next queue and EDID stuff

[Dave Airlie]

Hi guys (but mainly ajax)

I have a bunch of EDID and quirk stuff outstanding,

I've made a bundle on patchwork for it
https://patchwork.kernel.org/bundle/airlied/edid-review/

Dave.

Re: -next queue and EDID stuff

[Adam Jackson]

On 8/23/12 7:50 PM, Dave Airlie wrote:
> Hi guys (but mainly ajax)
>
> I have a bunch of EDID and quirk stuff outstanding,
>
> I've made a bundle on patchwork for it
> https://patchwork.kernel.org/bundle/airlied/edid-review/

https://patchwork.kernel.org/patch/1364501/ - I'm nervous about this on 
non-EDDC monitors, things are touchy enough as it is.  Would prefer if 
we did the normal 2-message thing for blocks 0 and 1, and only did 3 
messages if segment != 0.

https://patchwork.kernel.org/patch/1310091/ - nak, the commit message 
itself gives away that we're doing else something wrong here.  If 
DISABLE_AUDIO is sufficient for one driver it should be sufficient for all.

For the rest (though I have some complaints that aren't enough to 
justify saying no):

Reviewed-by: Adam Jackson <ajax@redhat.com>

Paul's FORCE_REDUCED_BLANKING series makes me nervous about what those 
monitors will do over VGA, since from a conversation we had on IRC he 
hasn't been able to test that.

I don't really see the point in the EDID_MFG_ID() bit of the quirks 
rework.  Have we actually seen a monitor where that field was malformed 
in a way that would require it?

- ajax

Re: -next queue and EDID stuff

[Alex Deucher]

On Mon, Aug 27, 2012 at 4:24 PM, Adam Jackson <ajax@redhat.com> wrote:
> On 8/23/12 7:50 PM, Dave Airlie wrote:
>>
>> Hi guys (but mainly ajax)
>>
>> I have a bunch of EDID and quirk stuff outstanding,
>>
>> I've made a bundle on patchwork for it
>> https://patchwork.kernel.org/bundle/airlied/edid-review/
>
>
> https://patchwork.kernel.org/patch/1364501/ - I'm nervous about this on
> non-EDDC monitors, things are touchy enough as it is.  Would prefer if we
> did the normal 2-message thing for blocks 0 and 1, and only did 3 messages
> if segment != 0.
>
> https://patchwork.kernel.org/patch/1310091/ - nak, the commit message itself
> gives away that we're doing else something wrong here.  If DISABLE_AUDIO is
> sufficient for one driver it should be sufficient for all.
>
> For the rest (though I have some complaints that aren't enough to justify
> saying no):
>
> Reviewed-by: Adam Jackson <ajax@redhat.com>
>
> Paul's FORCE_REDUCED_BLANKING series makes me nervous about what those
> monitors will do over VGA, since from a conversation we had on IRC he hasn't
> been able to test that.

I asked the closed driver display team about these to see if we had
any generic rules for whole classes of monitors and they do not.  They
keep a small database of displays that need special tweaking and then
have a set of options that can be enabled by the user (always use
CVT-RB rather than GTF, etc.).  There's no generic solution since EDID
1.3 doesn't give us enough info to really enable generic rules on
whole classes of monitors and most VGA connectors still use EDID 1.3
with no extension blocks.  EDID 1.4 is better, but few VGA connectors
on monitors uses it.  Other than that, they also rely on the EDID.

Alex

>
> I don't really see the point in the EDID_MFG_ID() bit of the quirks rework.
> Have we actually seen a monitor where that field was malformed in a way that
> would require it?
>
> - ajax
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: -next queue and EDID stuff

[Adam Jackson]

On 8/27/12 4:34 PM, Alex Deucher wrote:
> On Mon, Aug 27, 2012 at 4:24 PM, Adam Jackson <ajax@redhat.com> wrote:
>> Paul's FORCE_REDUCED_BLANKING series makes me nervous about what those
>> monitors will do over VGA, since from a conversation we had on IRC he hasn't
>> been able to test that.
>
> I asked the closed driver display team about these to see if we had
> any generic rules for whole classes of monitors and they do not.  They
> keep a small database of displays that need special tweaking and then
> have a set of options that can be enabled by the user (always use
> CVT-RB rather than GTF, etc.).  There's no generic solution since EDID
> 1.3 doesn't give us enough info to really enable generic rules on
> whole classes of monitors and most VGA connectors still use EDID 1.3
> with no extension blocks.  EDID 1.4 is better, but few VGA connectors
> on monitors uses it.  Other than that, they also rely on the EDID.

I really wish there were a useful way to identify the display controller 
on the other end.  DDC/CI will give it to you, kind of, but any monitor 
that supports CI probably doesn't need quirking, at least not at this level.

Paul's series does pick specific vendor/model IDs as much as it can, but 
when the model is "0" one is a little wary about whether the vendor 
bothers to set the model ID meaningfully at all.

- ajax

Re: -next queue and EDID stuff

[Daniel Vetter]

On Mon, Aug 27, 2012 at 10:24 PM, Adam Jackson <ajax@redhat.com> wrote:
> On 8/23/12 7:50 PM, Dave Airlie wrote:
>>
>> Hi guys (but mainly ajax)
>>
>> I have a bunch of EDID and quirk stuff outstanding,
>>
>> I've made a bundle on patchwork for it
>> https://patchwork.kernel.org/bundle/airlied/edid-review/
>
>
> https://patchwork.kernel.org/patch/1364501/ - I'm nervous about this on
> non-EDDC monitors, things are touchy enough as it is.  Would prefer if we
> did the normal 2-message thing for blocks 0 and 1, and only did 3 messages
> if segment != 0.

I've already bikeshedded this patch to death, it's now in proper shape imo:

Subject: [PATCH V4] drm: edid: add support for E-DDC
Message-id: <1345887836-15314-2-git-send-email-s.shirish@samsung.com>

Cheers, Daniel
-- 
Daniel Vetter
daniel.vetter@ffwll.ch - +41 (0) 79 364 57 48 - http://blog.ffwll.ch

Re: -next queue and EDID stuff

[Ben Skeggs]

On Mon, Aug 27, 2012 at 04:24:02PM -0400, Adam Jackson wrote:
> On 8/23/12 7:50 PM, Dave Airlie wrote:
> >Hi guys (but mainly ajax)
> >
> >I have a bunch of EDID and quirk stuff outstanding,
> >
> >I've made a bundle on patchwork for it
> >https://patchwork.kernel.org/bundle/airlied/edid-review/
> 
> https://patchwork.kernel.org/patch/1364501/ - I'm nervous about this
> on non-EDDC monitors, things are touchy enough as it is.  Would
> prefer if we did the normal 2-message thing for blocks 0 and 1, and
> only did 3 messages if segment != 0.
> 
> https://patchwork.kernel.org/patch/1310091/ - nak, the commit
> message itself gives away that we're doing else something wrong
> here.  If DISABLE_AUDIO is sufficient for one driver it should be
> sufficient for all.
I believe the issue in this case was that the monitor screws up when it
receives AVI InfoFrames.  Nouveau will send them for HDMI monitors even
if audio isn't supported.

> 
> For the rest (though I have some complaints that aren't enough to
> justify saying no):
> 
> Reviewed-by: Adam Jackson <ajax@redhat.com>
> 
> Paul's FORCE_REDUCED_BLANKING series makes me nervous about what
> those monitors will do over VGA, since from a conversation we had on
> IRC he hasn't been able to test that.
> 
> I don't really see the point in the EDID_MFG_ID() bit of the quirks
> rework.  Have we actually seen a monitor where that field was
> malformed in a way that would require it?
> 
> - ajax
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: -next queue and EDID stuff

[Baurzhan Ismagulov]

Hello,

On 8/23/12 7:50 PM, Dave Airlie wrote:
> Hi guys (but mainly ajax)
>
> I have a bunch of EDID and quirk stuff outstanding,
>
> I've made a bundle on patchwork for it
> https://patchwork.kernel.org/bundle/airlied/edid-review/

I'd also like to get feedback about these:

http://www.radix50.net/~ibr/0001-drm-Make-edid_quirk_list-const.patch
http://www.radix50.net/~ibr/0002-drm-Add-quirk-for-Samsung-SyncMaster-2443BW.patch

(Problem description: http://thread.gmane.org/gmane.comp.video.dri.devel/71327)

With kind regards,
Baurzhan.

Re: -next queue and EDID stuff

[Ian Pilcher]

On 08/27/2012 03:24 PM, Adam Jackson wrote:
> https://patchwork.kernel.org/patch/1310091/ - nak, the commit message
> itself gives away that we're doing else something wrong here.  If
> DISABLE_AUDIO is sufficient for one driver it should be sufficient for all.

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.

> I don't really see the point in the EDID_MFG_ID() bit of the quirks
> rework.  Have we actually seen a monitor where that field was malformed
> in a way that would require it?

It makes the code to compare, parse, and display the IDs a lot cleaner
(IMO).  I have no idea if there are monitors with malformed IDs out
there, but it seems better to have code that will be able to gracefully
handle the situation if it ever does arise.

Hope that clears a couple of things up.

-- 
========================================================================
Ian Pilcher                                         arequipeno@gmail.com
"If you're going to shift my paradigm ... at least buy me dinner first."
========================================================================

Re: -next queue and EDID stuff

[Adam Jackson]

On 8/28/12 7:33 PM, Ian Pilcher wrote:
> On 08/27/2012 03:24 PM, Adam Jackson wrote:
>> https://patchwork.kernel.org/patch/1310091/ - nak, the commit message
>> itself gives away that we're doing else something wrong here.  If
>> DISABLE_AUDIO is sufficient for one driver it should be sufficient for all.
>
> 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.

- ajax

Re: -next queue and EDID stuff

[Ian Pilcher]

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:

--- 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."
========================================================================

Re: -next queue and EDID stuff

[Daniel Vetter]

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

-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: -next queue and EDID stuff

[Ian Pilcher]

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!

-- 
========================================================================
Ian Pilcher                                         arequipeno@gmail.com
"If you're going to shift my paradigm ... at least buy me dinner first."
========================================================================

Re: -next queue and EDID stuff

[Adam Jackson]

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

Re: -next queue and EDID stuff

[Ian Pilcher]

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!

-- 
========================================================================
Ian Pilcher                                         arequipeno@gmail.com
"If you're going to shift my paradigm ... at least buy me dinner first."
========================================================================

Re: -next queue and EDID stuff

[Daniel Vetter]

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
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: -next queue and EDID stuff

[Ian Pilcher]

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                                         arequipeno@gmail.com
"If you're going to shift my paradigm ... at least buy me dinner first."
========================================================================

Re: -next queue and EDID stuff

[Ian Pilcher]

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!
> 


-- 
========================================================================
Ian Pilcher                                         arequipeno@gmail.com
"If you're going to shift my paradigm ... at least buy me dinner first."
========================================================================