Enhancing EDID quirk functionality

[Adam Jackson]

On Mon, 2012-05-07 at 14:50 -0500, Ian Pilcher wrote:
> On 05/03/2012 02:42 PM, Adam Jackson wrote:
> > I'd like to see documentation for the bit values of the quirks as well.
> > And, ideally, this would also have some runtime API for manipulating the
> > quirk list, so that way you can test new quirks without needing a reboot
> > cycle.

I should point out that I don't think any of my objections are
necessarily blockers to getting your work merged.

> I agree that the bit values should be documented.  I'm not sure where
> that documentation should go, however, since I can't find any
> documentation of the existing drm module parameters.  Tell me where it
> should go, and I'll happily write the doc.

Well the standard place is Documentation/kernel-parameters.txt.  Sadly
it appears to only document one drm parameter at the moment...

But there's also Documentation/EDID/ now.

> I also agree that it would be nice to be able to manipulate the quirk
> list at runtime, and I did think about trying to enable that.  I held
> off for a couple of reasons:
> 
> 1) I'm a total noob at kernel code, so things like in-kernel locking,
>    sysfs, memory management, etc., that would be required for a more
>    dynamic API are all new to me.
> 
>    That said, I'm more that willing to give it a go, if I can get some
>    guidance on those (and similar) topics.
>
> 2) I'm not sure how a runtime API should work.  The simplest possibility
>    is to just take a string, parse it, and overwrite the old extra
>    quirk list with the new list.  The downside to this is that all of
>    the existing extra quirks need to be repeated to change a single
>    quirk.

Files in sysfs are probably the least unnatural thing.  It's weird to
express this as an ioctl since the quirk list is global drm state and
the device nodes are, well, per-device.

You can copypasta much of that from the existing stuff in drm_sysfs.c, I
bet.  You'll need to hang the control file off the toplevel class, like
the 'version' file does, and you'll probably need to introduce a new
mutex around the quirk list to handle userspace changing it racing with
the driver reading it.

As far as interface, I'd probably say something like:

# echo "GSM:0x563f:0x180" > /sys/class/drm/edid_quirks

You'd match on the first two fields and then simply clobber the value.
If you don't find an existing match you add a new one.  If you find a
match and the new value is 0, you can delete it.  cat'ing the file gives
you a list of all programmed quirks.  If you wanted a shorthand for
"delete all" you could maybe make a 0-length write do it?  Like:

# echo "" > /sys/class/drm/edid_quirks

> > To close the loop all the way on that I'd also want to be able to scrape
> > the quirk list back out from that API, but that's not completely clean
> > right now.
> 
> Sound like a couple of sysfs files to me, one for the built-in quirks
> and one for the extra quirks -- maybe one quirk per line?  See my
> comments about the sysfs API above.
> 
> > We're being a little cavalier with the quirk list as it
> > stands because we don't differentiate among phy layers, and I can easily
> > imagine a monitor that needs a quirk on DVI but where the same quirk on
> > the same monitors' VGA would break it.  I don't think this has caused
> > problems yet, but.
> 
> Now you're above my pay grade.  What little I've read discovered about
> the way DisplayPort, HDMI, VGA, and DVI play together makes me think
> this is a nightmare best deferred, hopefully forever.

Yeah, I wouldn't worry about it, just noting it for posterity.  We
should try to keep track of this for future quirks but I'm not going to
lose sleep over what we've got.

> > Where the EDID for DP-1 appears to be truncated: the "extension" field
> > (second byte from the end) is 1 as you'd expect for an HDMI monitor, but
> > there's no extension block.  How big of a file do you get from
> > /sys/class/drm/*/edid for that port?
> 
> The EDID data in sysfs is 256 bytes, which I believe means that it does
> include the extension block.
> 
> I just tried connecting an HDMI TV to my laptop, and I saw the same
> behavior -- 256-byte edid file in sysfs, but "xrandr --verbose" only
> shows 128 bytes.  When I attach the same TV to my workstation with Intel
> "HD 2000" graphics, "xrandr --verbose" shows all 256 bytes of EDID data.
> 
> So it appears that the full data is being read by both systems, but the
> behavior of xrandr (or presumably whatever API xrandr uses to get the
> EDID data that it displays) differs between the two drivers.  Fun.

Well thankfully that's just the X nouveau driver being broken, and
shouldn't affect actual output setup.  I could have sworn I'd fixed
nouveau for this before.  I'll send the fix along, thanks for catching
it.

- ajax
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part
URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20120507/67406281/attachment.pgp>