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.
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
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
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
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
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
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.
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
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.
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
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!
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
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!
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
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!
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!
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...
(Problem description: http://thread.gmane.org/gmane.comp.video.dri.devel/71327)
With kind regards, Baurzhan.
dri-devel@lists.freedesktop.org
-
Adam Jackson -
Alex Deucher -
Baurzhan Ismagulov -
Ben Skeggs -
Daniel Vetter -
Dave Airlie -
Ian Pilcher