Expose more EDID fields to userspace

Expose more EDID fields to userspace

[Simon Ser]

Hi all,

Right now, the kernel parses EDIDs and exposes some of the data to
userspace. For instance, drmModeConnector has mm{Width,Height} and
subpixel.

Generally, userspace also has another EDID parser. For instance,
wlroots uses it just to get the make/model/serial. I've talked about
this at XDC 2018, and someone mentioned it could be a good idea to
de-duplicate the work.

Would it be reasonable to expose those as DRM connector properties?

Thanks,
--
Simon Ser
https://emersion.fr

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: Expose more EDID fields to userspace

[Daniel Vetter]

On Sun, Dec 23, 2018 at 09:16:13AM +0000, Simon Ser wrote:
> Hi all,
> 
> Right now, the kernel parses EDIDs and exposes some of the data to
> userspace. For instance, drmModeConnector has mm{Width,Height} and
> subpixel.
> 
> Generally, userspace also has another EDID parser. For instance,
> wlroots uses it just to get the make/model/serial. I've talked about
> this at XDC 2018, and someone mentioned it could be a good idea to
> de-duplicate the work.

Could have been me ...

> Would it be reasonable to expose those as DRM connector properties?

Yes, very much. Aside from the kernel-side implementation all we need is
the userspace per

https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#open-source-userspace-requirements

Important: Don't merge the userspace side into your main branch before
it's all reviewed.

And ideally some igts (we have infrastructure to inject edids and hence
can check that the right stuff comes back):

https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#testing-and-validation

Cheers, Daniel

> 
> Thanks,
> --
> Simon Ser
> https://emersion.fr
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: Expose more EDID fields to userspace

[Simon Ser]

On Monday, December 24, 2018 11:23 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Sun, Dec 23, 2018 at 09:16:13AM +0000, Simon Ser wrote:
>
> > Hi all,
> > Right now, the kernel parses EDIDs and exposes some of the data to
> > userspace. For instance, drmModeConnector has mm{Width,Height} and
> > subpixel.
> > Generally, userspace also has another EDID parser. For instance,
> > wlroots uses it just to get the make/model/serial. I've talked about
> > this at XDC 2018, and someone mentioned it could be a good idea to
> > de-duplicate the work.
>
> Could have been me ...
>
> > Would it be reasonable to expose those as DRM connector properties?
>
> Yes, very much. Aside from the kernel-side implementation all we need is
> the userspace per
>
> https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#open-source-userspace-requirements
>
> Important: Don't merge the userspace side into your main branch before
> it's all reviewed.
>
> And ideally some igts (we have infrastructure to inject edids and hence
> can check that the right stuff comes back):
>
> https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#testing-and-validation

Thanks! I've began to implement this, but I'm wondering what's the best
way to expose this piece of information to userspace.

I can think of two solutions:

1. A single IDENTIFICATION blob containing one struct with
   make/serial/model.
2. Three properties: MAKE, MODEL, SERIAL.

I also wonder if it's worth it to expose both the uint32_t for serial
and the serial string, or just the serial string. Some monitors don't
have a serial string, but we can generate one from the uint32_t (e.g.
hex). Same applies to model.

Thoughts?
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: Expose more EDID fields to userspace

[Daniel Vetter]

On Mon, Dec 31, 2018 at 12:57:24PM +0000, Simon Ser wrote:
> On Monday, December 24, 2018 11:23 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Sun, Dec 23, 2018 at 09:16:13AM +0000, Simon Ser wrote:
> >
> > > Hi all,
> > > Right now, the kernel parses EDIDs and exposes some of the data to
> > > userspace. For instance, drmModeConnector has mm{Width,Height} and
> > > subpixel.
> > > Generally, userspace also has another EDID parser. For instance,
> > > wlroots uses it just to get the make/model/serial. I've talked about
> > > this at XDC 2018, and someone mentioned it could be a good idea to
> > > de-duplicate the work.
> >
> > Could have been me ...
> >
> > > Would it be reasonable to expose those as DRM connector properties?
> >
> > Yes, very much. Aside from the kernel-side implementation all we need is
> > the userspace per
> >
> > https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#open-source-userspace-requirements
> >
> > Important: Don't merge the userspace side into your main branch before
> > it's all reviewed.
> >
> > And ideally some igts (we have infrastructure to inject edids and hence
> > can check that the right stuff comes back):
> >
> > https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#testing-and-validation
> 
> Thanks! I've began to implement this, but I'm wondering what's the best
> way to expose this piece of information to userspace.
> 
> I can think of two solutions:
> 
> 1. A single IDENTIFICATION blob containing one struct with
>    make/serial/model.
> 2. Three properties: MAKE, MODEL, SERIAL.
> 
> I also wonder if it's worth it to expose both the uint32_t for serial
> and the serial string, or just the serial string. Some monitors don't
> have a serial string, but we can generate one from the uint32_t (e.g.
> hex). Same applies to model.
> 
> Thoughts?

Best to pull in some other compositor people and get them to agree. From a
kernel pov I'm fine with whatever userspace preferes.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: Expose more EDID fields to userspace

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 1257 bytes --]

Daniel Vetter <daniel@ffwll.ch> writes:

> Best to pull in some other compositor people and get them to agree. From a
> kernel pov I'm fine with whatever userspace preferes.

Hrm. It would be good to have everyone using the same interpretation of
EDID data; in particular, where the kernel has quirks that change the
interpretation, user space should be consistent with that.

Unless we expose all of the EDID data, then user space may still have to
parse EDID. If the kernel has EDID quirks, it might be good to to make
those affect the "raw" EDID data visible to use space so that values the
kernel supplies separately are consistent with values extracted from the
"raw" EDID data.

Doing this in the kernel does make it harder to quickly supply fixes for
a specific user space application. This will probably lead to
kludge-arounds in user space that could depend on kernel
version. Perhaps these EDID capabilities in the kernel should be
versioned separately?

I see good benefits from having user space able to see how the kernel is
interpreting EDID so that it can adapt as appropriate, but we should be
cautious about moving functionality into the kernel that would be more
easily maintained up in user space.

-- 
-keith

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: Expose more EDID fields to userspace

[Brian Starkey]

On Mon, Jan 07, 2019 at 07:57:54AM -0800, Keith Packard wrote:
> Daniel Vetter <daniel@ffwll.ch> writes:
> 
> > Best to pull in some other compositor people and get them to agree. From a
> > kernel pov I'm fine with whatever userspace preferes.
> 
> Hrm. It would be good to have everyone using the same interpretation of
> EDID data; in particular, where the kernel has quirks that change the
> interpretation, user space should be consistent with that.
> 
> Unless we expose all of the EDID data, then user space may still have to
> parse EDID. If the kernel has EDID quirks, it might be good to to make
> those affect the "raw" EDID data visible to use space so that values the
> kernel supplies separately are consistent with values extracted from the
> "raw" EDID data.

If the quirks can be re-encoded back into an EDID representation, then
this sounds like a fairly good approach to me.

> 
> Doing this in the kernel does make it harder to quickly supply fixes for
> a specific user space application. This will probably lead to
> kludge-arounds in user space that could depend on kernel
> version. Perhaps these EDID capabilities in the kernel should be
> versioned separately?
> 
> I see good benefits from having user space able to see how the kernel is
> interpreting EDID so that it can adapt as appropriate, but we should be
> cautious about moving functionality into the kernel that would be more
> easily maintained up in user space.
> 

I agree. It seems likely that whatever happens (some) userspace is
still going to implement (some) EDID parsing functionality, so it's
hard to reason about what belongs where. Shared code in userspace
(libdrm?) may well be better than exposing it from the kernel.

If it is exposed by the kernel, then it's still non-obvious to me
how the kernel exposes that information/interpretation. Adding
a property for every potentially-useful field really doesn't scale
well, and what fields are useful isn't obvious - e.g. serial string vs
serial no., as mentioned by Simon.

Uma's recent series: "Add HDR Metadata Parsing and handling in DRM
layer"[1] is a good example of more stuff which userspace would want to
parse out of the EDID (supported display colorimetry and transfer
functions).

It would be nice to avoid duplicating all the CEA extension parsing
code, but the EDID/CEA data structure is extensible by design. So the
kernel API would need to be similarly extensible, or we'll just
balloon loads of properties... and then the kernel API would likely
just end up just looking similar to the CEA block anyway.

Cheers,
-Brian

[1] https://lists.freedesktop.org/archives/dri-devel/2018-December/200154.html

> -- 
> -keith



> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: Expose more EDID fields to userspace

[Pekka Paalanen]

[-- Attachment #1.1: Type: text/plain, Size: 4302 bytes --]

On Mon, 7 Jan 2019 17:07:09 +0000
Brian Starkey <Brian.Starkey@arm.com> wrote:

> On Mon, Jan 07, 2019 at 07:57:54AM -0800, Keith Packard wrote:
> > Daniel Vetter <daniel@ffwll.ch> writes:
> >   
> > > Best to pull in some other compositor people and get them to agree. From a
> > > kernel pov I'm fine with whatever userspace preferes.  
> > 
> > Hrm. It would be good to have everyone using the same interpretation of
> > EDID data; in particular, where the kernel has quirks that change the
> > interpretation, user space should be consistent with that.
> > 
> > Unless we expose all of the EDID data, then user space may still have to
> > parse EDID. If the kernel has EDID quirks, it might be good to to make
> > those affect the "raw" EDID data visible to use space so that values the
> > kernel supplies separately are consistent with values extracted from the
> > "raw" EDID data.  
> 
> If the quirks can be re-encoded back into an EDID representation, then
> this sounds like a fairly good approach to me.
> 
> > 
> > Doing this in the kernel does make it harder to quickly supply fixes for
> > a specific user space application. This will probably lead to
> > kludge-arounds in user space that could depend on kernel
> > version. Perhaps these EDID capabilities in the kernel should be
> > versioned separately?
> > 
> > I see good benefits from having user space able to see how the kernel is
> > interpreting EDID so that it can adapt as appropriate, but we should be
> > cautious about moving functionality into the kernel that would be more
> > easily maintained up in user space.
> >   
> 
> I agree. It seems likely that whatever happens (some) userspace is
> still going to implement (some) EDID parsing functionality, so it's
> hard to reason about what belongs where. Shared code in userspace
> (libdrm?) may well be better than exposing it from the kernel.
> 
> If it is exposed by the kernel, then it's still non-obvious to me
> how the kernel exposes that information/interpretation. Adding
> a property for every potentially-useful field really doesn't scale
> well, and what fields are useful isn't obvious - e.g. serial string vs
> serial no., as mentioned by Simon.
> 
> Uma's recent series: "Add HDR Metadata Parsing and handling in DRM
> layer"[1] is a good example of more stuff which userspace would want to
> parse out of the EDID (supported display colorimetry and transfer
> functions).
> 
> It would be nice to avoid duplicating all the CEA extension parsing
> code, but the EDID/CEA data structure is extensible by design. So the
> kernel API would need to be similarly extensible, or we'll just
> balloon loads of properties... and then the kernel API would likely
> just end up just looking similar to the CEA block anyway.
> 
> Cheers,
> -Brian
> 
> [1] https://lists.freedesktop.org/archives/dri-devel/2018-December/200154.html

I would agree with an effort to establish a userspace EDID parsing
library in any case. As mentioned above, there will probably be too
much to expose via kernel UABI, or it will become just another
encoded format that again should have a shared parser library in
userspace.

Would it be possible to architect the library so that it would be
shared with the kernel? Maybe the quirks database could be shared
with the kernel as well? That way both kernel and userspace would
more or less agree on the parsing details.

I've been dreaming of a "liboutput" that would e.g. parse EDID,
generate CVT video mode timings, and whatnot that all display
servers more or less will duplicate. Once upon a time Ajax started
minitru IIRC...

Another thing for "liboutput" was a device description database,
whose first use would have been the "non-desktop" property. Because
we don't expose monitors through udev to have udev rules tag them
with interesting bits.

Imagine this: monitors exposed as devices via udev, tagged with
helpers as regular monitor, HMD, TV, projector, ... and all EDID
fields decoded as well. And quirks in hwdb. But I suppose that
won't happen, because a "monitor device node" would have no other
use. Except... programmatical monitor controls? Like backlight,
brightness, contrast, and so on.

Ah, nevermind. :-)


Thanks,
pq

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: Expose more EDID fields to userspace

[Ville Syrjälä]

On Wed, Jan 16, 2019 at 08:35:26PM +0200, Pekka Paalanen wrote:
> On Mon, 7 Jan 2019 17:07:09 +0000
> Brian Starkey <Brian.Starkey@arm.com> wrote:
> 
> > On Mon, Jan 07, 2019 at 07:57:54AM -0800, Keith Packard wrote:
> > > Daniel Vetter <daniel@ffwll.ch> writes:
> > >   
> > > > Best to pull in some other compositor people and get them to agree. From a
> > > > kernel pov I'm fine with whatever userspace preferes.  
> > > 
> > > Hrm. It would be good to have everyone using the same interpretation of
> > > EDID data; in particular, where the kernel has quirks that change the
> > > interpretation, user space should be consistent with that.
> > > 
> > > Unless we expose all of the EDID data, then user space may still have to
> > > parse EDID. If the kernel has EDID quirks, it might be good to to make
> > > those affect the "raw" EDID data visible to use space so that values the
> > > kernel supplies separately are consistent with values extracted from the
> > > "raw" EDID data.  
> > 
> > If the quirks can be re-encoded back into an EDID representation, then
> > this sounds like a fairly good approach to me.
> > 
> > > 
> > > Doing this in the kernel does make it harder to quickly supply fixes for
> > > a specific user space application. This will probably lead to
> > > kludge-arounds in user space that could depend on kernel
> > > version. Perhaps these EDID capabilities in the kernel should be
> > > versioned separately?
> > > 
> > > I see good benefits from having user space able to see how the kernel is
> > > interpreting EDID so that it can adapt as appropriate, but we should be
> > > cautious about moving functionality into the kernel that would be more
> > > easily maintained up in user space.
> > >   
> > 
> > I agree. It seems likely that whatever happens (some) userspace is
> > still going to implement (some) EDID parsing functionality, so it's
> > hard to reason about what belongs where. Shared code in userspace
> > (libdrm?) may well be better than exposing it from the kernel.
> > 
> > If it is exposed by the kernel, then it's still non-obvious to me
> > how the kernel exposes that information/interpretation. Adding
> > a property for every potentially-useful field really doesn't scale
> > well, and what fields are useful isn't obvious - e.g. serial string vs
> > serial no., as mentioned by Simon.
> > 
> > Uma's recent series: "Add HDR Metadata Parsing and handling in DRM
> > layer"[1] is a good example of more stuff which userspace would want to
> > parse out of the EDID (supported display colorimetry and transfer
> > functions).
> > 
> > It would be nice to avoid duplicating all the CEA extension parsing
> > code, but the EDID/CEA data structure is extensible by design. So the
> > kernel API would need to be similarly extensible, or we'll just
> > balloon loads of properties... and then the kernel API would likely
> > just end up just looking similar to the CEA block anyway.
> > 
> > Cheers,
> > -Brian
> > 
> > [1] https://lists.freedesktop.org/archives/dri-devel/2018-December/200154.html
> 
> I would agree with an effort to establish a userspace EDID parsing
> library in any case. As mentioned above, there will probably be too
> much to expose via kernel UABI, or it will become just another
> encoded format that again should have a shared parser library in
> userspace.
> 
> Would it be possible to architect the library so that it would be
> shared with the kernel? Maybe the quirks database could be shared
> with the kernel as well? That way both kernel and userspace would
> more or less agree on the parsing details.

IIRC long ago I did manage to build drm_edid.c in userspace.
My idea was to potentially fuzz it without oopsing the kernel.
But I didn't get beyond just making it build before getting
distracted by something shinier.

Taking that further would involve pulling the edid parser into
a userspace helper entirely. Unfortunately that would open up
the big can of "how do we parse the edid during boot?" worms.
But I do kinda like the idea of not having the kernel parse
untrusted input directly.

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: Expose more EDID fields to userspace

[Adam Jackson]

On Wed, 2019-01-16 at 20:35 +0200, Pekka Paalanen wrote:

> I would agree with an effort to establish a userspace EDID parsing
> library in any case. As mentioned above, there will probably be too
> much to expose via kernel UABI, or it will become just another
> encoded format that again should have a shared parser library in
> userspace.
> 
> Would it be possible to architect the library so that it would be
> shared with the kernel? Maybe the quirks database could be shared
> with the kernel as well? That way both kernel and userspace would
> more or less agree on the parsing details.

If the kernel wanted to expose its quirks db somehow the library could
certainly be taught to use it. However there are likely to be quirks
relevant only to userspace (see below), making the kernel carry that
doesn't make a ton of sense.

> I've been dreaming of a "liboutput" that would e.g. parse EDID,
> generate CVT video mode timings, and whatnot that all display
> servers more or less will duplicate. Once upon a time Ajax started
> minitru IIRC...

It's still not the worst idea, I'd be happy to revisit it.

Part of the problem with the idea is that EDID parsing is not
unambiguous. There are several fields for "physical output size" with
slightly different semantics, which one do you want? There are both
structured and free-form ways to encode monitor name and serial number.
There are multiple ways to ask for 1920x1080, how many slightly
different timings do you want? Some more-modern displays have DisplayID
blocks too, which we're not fetching, and which can easily disagree
with EDID; which version of reality would you like?

Me personally I'm happy to make policy decisions about this kind of
thing, but there are likely to be cases where a display server wants to
make more subtle decisions, in which case it's likely to ignore any
"cooked" queries you might of the library and instead try parsing the
data itself. Many of these decisions currently don't affect the kernel
at all (we don't use physical size for anything, but userspace thinks
it wants to know about DPI...).

> Another thing for "liboutput" was a device description database,
> whose first use would have been the "non-desktop" property. Because
> we don't expose monitors through udev to have udev rules tag them
> with interesting bits.
> 
> Imagine this: monitors exposed as devices via udev, tagged with
> helpers as regular monitor, HMD, TV, projector, ... and all EDID
> fields decoded as well. And quirks in hwdb. But I suppose that
> won't happen, because a "monitor device node" would have no other
> use. Except... programmatical monitor controls? Like backlight,
> brightness, contrast, and so on.

This could be interesting. I'd half-written DDC/CI support for drm a
while ago, which would allow us to expose that kind of thing. But to be
broadly useful you'd want to mirror the same kind of knobs for like
platform backlight control, and so far the kernel has steadfastly
refused to believe it can possibly associate a backlight controller
with a drm output in any userspace-useful way.

- ajax

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: Expose more EDID fields to userspace

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 2725 bytes --]

Adam Jackson <ajax@redhat.com> writes:

> If the kernel wanted to expose its quirks db somehow the library could
> certainly be taught to use it. However there are likely to be quirks
> relevant only to userspace (see below), making the kernel carry that
> doesn't make a ton of sense.

We do expose some of the quirks to user space, but not as a database,
and not consistently. Some of the quirks just match EDID data to drive
some decision (like non-desktop). Other quirks override some EDID data
that is 'wrong'. For these latter instances, I wonder if we shouldn't be
re-writing the EDID data that user space gets? Or at least making sure
the quirked values are available to user space?

> Part of the problem with the idea is that EDID parsing is not
> unambiguous. There are several fields for "physical output size" with
> slightly different semantics, which one do you want? There are both
> structured and free-form ways to encode monitor name and serial
> number.

Hrm. For places where the kernel *does* parse the data, it would sure be
nice to have the kernel version to at least make that
consistent. Model/serial data used to select quirks seems like something
we should be exposing?

I'm getting the feeling that either extreme approach (parse all of the
EDID data, or parse none of it) is not going to work and that our
current technique of picking some EDID-derived data to expose as
separate parsed values, and some to leave for user space to discover for
itself is where we'll be stuck for at least the near term.

If we come to agreement that this approach is what we'll be doing, then
I'd like to have a couple of suggestions:

 1) Document what we've got today

 2) Document basic guidelines of when to expose parsed EDID-derived data
    and when to just leave it in the EDID block for user space

 3) Plan on exposing all values which the kernel *does* use internally
    as parsed-out properties. Especially values which get quirked,
    possibly exposing both the "raw" value and the "quirk" value.

 4) Decide if we want to let user-space in on the quirking fun. I can
    imagine a user-space helper that gets run at hot-plug time, reads
    the EDID data and then pokes quirk values into the kernel.

 5) Decide, as a matter of policy, whether the kernel will ever edit
    EDID data that gets exposed to user space. I can think of good
    reasons on both sides, but I think we need to hash out what the
    kernel will do so that user space knows what's going on.

I think what I want is for kernel and user space to at least be
operating on the same data, even if we end up re-implementing a pile of
code up in user space.

-- 
-keith

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: Expose more EDID fields to userspace

[Daniel Vetter]

On Wed, Jan 16, 2019 at 12:32:58PM -0800, Keith Packard wrote:
> Adam Jackson <ajax@redhat.com> writes:
> 
> > If the kernel wanted to expose its quirks db somehow the library could
> > certainly be taught to use it. However there are likely to be quirks
> > relevant only to userspace (see below), making the kernel carry that
> > doesn't make a ton of sense.
> 
> We do expose some of the quirks to user space, but not as a database,
> and not consistently. Some of the quirks just match EDID data to drive
> some decision (like non-desktop). Other quirks override some EDID data
> that is 'wrong'. For these latter instances, I wonder if we shouldn't be
> re-writing the EDID data that user space gets? Or at least making sure
> the quirked values are available to user space?
> 
> > Part of the problem with the idea is that EDID parsing is not
> > unambiguous. There are several fields for "physical output size" with
> > slightly different semantics, which one do you want? There are both
> > structured and free-form ways to encode monitor name and serial
> > number.
> 
> Hrm. For places where the kernel *does* parse the data, it would sure be
> nice to have the kernel version to at least make that
> consistent. Model/serial data used to select quirks seems like something
> we should be exposing?
> 
> I'm getting the feeling that either extreme approach (parse all of the
> EDID data, or parse none of it) is not going to work and that our
> current technique of picking some EDID-derived data to expose as
> separate parsed values, and some to leave for user space to discover for
> itself is where we'll be stuck for at least the near term.
> 
> If we come to agreement that this approach is what we'll be doing, then
> I'd like to have a couple of suggestions:
> 
>  1) Document what we've got today

Connector properties are documented here:

https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#standard-connector-properties

I think we've caught up and have them all documented now, at least all the
ones that have been standardized. There's not much, from a quick look we
expose the following immutable props for userspace's benefit:

EDID, PATH, TILE (this has been used for other composite screens, not just
MST), non_desktop, "panel orientation". Plus vrr_capable, which is
documented in a separate chapter. All the other properties are meant
to be set, or not about parsing sink information.

For all the other bits I think it'd be good to do a better job, but most
likely we'll just muddle on. Exhibit A: The super consistent naming
scheme for properties we have, see above :-)

But in case we're going to do better, fully agree on documenting all this.
-Daniel

>  2) Document basic guidelines of when to expose parsed EDID-derived data
>     and when to just leave it in the EDID block for user space
> 
>  3) Plan on exposing all values which the kernel *does* use internally
>     as parsed-out properties. Especially values which get quirked,
>     possibly exposing both the "raw" value and the "quirk" value.
> 
>  4) Decide if we want to let user-space in on the quirking fun. I can
>     imagine a user-space helper that gets run at hot-plug time, reads
>     the EDID data and then pokes quirk values into the kernel.
> 
>  5) Decide, as a matter of policy, whether the kernel will ever edit
>     EDID data that gets exposed to user space. I can think of good
>     reasons on both sides, but I think we need to hash out what the
>     kernel will do so that user space knows what's going on.
> 
> I think what I want is for kernel and user space to at least be
> operating on the same data, even if we end up re-implementing a pile of
> code up in user space.
> 
> -- 
> -keith



> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel


-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: Expose more EDID fields to userspace

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 605 bytes --]

Daniel Vetter <daniel@ffwll.ch> writes:

> Connector properties are documented here:
>
> https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#standard-connector-properties

Cool. Seems like we might want a bit more organization here to make it
clear which of these are derived from the connected monitor.

It would be good to read through the source code to find other EDID-ish
data being used within the kernel. Things like the vendor and model info
might be useful as properties, especially if there might be quirks to
'fix' the edid values and drive other kernel behavior.

-- 
-keith

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: Expose more EDID fields to userspace

[Alex Deucher]

On Wed, Jan 16, 2019 at 1:35 PM Pekka Paalanen <ppaalanen@gmail.com> wrote:
>
> On Mon, 7 Jan 2019 17:07:09 +0000
> Brian Starkey <Brian.Starkey@arm.com> wrote:
>
> > On Mon, Jan 07, 2019 at 07:57:54AM -0800, Keith Packard wrote:
> > > Daniel Vetter <daniel@ffwll.ch> writes:
> > >
> > > > Best to pull in some other compositor people and get them to agree. From a
> > > > kernel pov I'm fine with whatever userspace preferes.
> > >
> > > Hrm. It would be good to have everyone using the same interpretation of
> > > EDID data; in particular, where the kernel has quirks that change the
> > > interpretation, user space should be consistent with that.
> > >
> > > Unless we expose all of the EDID data, then user space may still have to
> > > parse EDID. If the kernel has EDID quirks, it might be good to to make
> > > those affect the "raw" EDID data visible to use space so that values the
> > > kernel supplies separately are consistent with values extracted from the
> > > "raw" EDID data.
> >
> > If the quirks can be re-encoded back into an EDID representation, then
> > this sounds like a fairly good approach to me.
> >
> > >
> > > Doing this in the kernel does make it harder to quickly supply fixes for
> > > a specific user space application. This will probably lead to
> > > kludge-arounds in user space that could depend on kernel
> > > version. Perhaps these EDID capabilities in the kernel should be
> > > versioned separately?
> > >
> > > I see good benefits from having user space able to see how the kernel is
> > > interpreting EDID so that it can adapt as appropriate, but we should be
> > > cautious about moving functionality into the kernel that would be more
> > > easily maintained up in user space.
> > >
> >
> > I agree. It seems likely that whatever happens (some) userspace is
> > still going to implement (some) EDID parsing functionality, so it's
> > hard to reason about what belongs where. Shared code in userspace
> > (libdrm?) may well be better than exposing it from the kernel.
> >
> > If it is exposed by the kernel, then it's still non-obvious to me
> > how the kernel exposes that information/interpretation. Adding
> > a property for every potentially-useful field really doesn't scale
> > well, and what fields are useful isn't obvious - e.g. serial string vs
> > serial no., as mentioned by Simon.
> >
> > Uma's recent series: "Add HDR Metadata Parsing and handling in DRM
> > layer"[1] is a good example of more stuff which userspace would want to
> > parse out of the EDID (supported display colorimetry and transfer
> > functions).
> >
> > It would be nice to avoid duplicating all the CEA extension parsing
> > code, but the EDID/CEA data structure is extensible by design. So the
> > kernel API would need to be similarly extensible, or we'll just
> > balloon loads of properties... and then the kernel API would likely
> > just end up just looking similar to the CEA block anyway.
> >
> > Cheers,
> > -Brian
> >
> > [1] https://lists.freedesktop.org/archives/dri-devel/2018-December/200154.html
>
> I would agree with an effort to establish a userspace EDID parsing
> library in any case. As mentioned above, there will probably be too
> much to expose via kernel UABI, or it will become just another
> encoded format that again should have a shared parser library in
> userspace.
>
> Would it be possible to architect the library so that it would be
> shared with the kernel? Maybe the quirks database could be shared
> with the kernel as well? That way both kernel and userspace would
> more or less agree on the parsing details.

Maybe make it part of libdrm and import the shared files periodically
like we do for driver ioctl interfaces?

Alex

>
> I've been dreaming of a "liboutput" that would e.g. parse EDID,
> generate CVT video mode timings, and whatnot that all display
> servers more or less will duplicate. Once upon a time Ajax started
> minitru IIRC...
>
> Another thing for "liboutput" was a device description database,
> whose first use would have been the "non-desktop" property. Because
> we don't expose monitors through udev to have udev rules tag them
> with interesting bits.
>
> Imagine this: monitors exposed as devices via udev, tagged with
> helpers as regular monitor, HMD, TV, projector, ... and all EDID
> fields decoded as well. And quirks in hwdb. But I suppose that
> won't happen, because a "monitor device node" would have no other
> use. Except... programmatical monitor controls? Like backlight,
> brightness, contrast, and so on.
>
> Ah, nevermind. :-)
>
>
> Thanks,
> pq
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: Expose more EDID fields to userspace

[Daniel Vetter]

On Thu, Jan 17, 2019 at 8:59 PM Alex Deucher <alexdeucher@gmail.com> wrote:
>
> On Wed, Jan 16, 2019 at 1:35 PM Pekka Paalanen <ppaalanen@gmail.com> wrote:
> >
> > On Mon, 7 Jan 2019 17:07:09 +0000
> > Brian Starkey <Brian.Starkey@arm.com> wrote:
> >
> > > On Mon, Jan 07, 2019 at 07:57:54AM -0800, Keith Packard wrote:
> > > > Daniel Vetter <daniel@ffwll.ch> writes:
> > > >
> > > > > Best to pull in some other compositor people and get them to agree. From a
> > > > > kernel pov I'm fine with whatever userspace preferes.
> > > >
> > > > Hrm. It would be good to have everyone using the same interpretation of
> > > > EDID data; in particular, where the kernel has quirks that change the
> > > > interpretation, user space should be consistent with that.
> > > >
> > > > Unless we expose all of the EDID data, then user space may still have to
> > > > parse EDID. If the kernel has EDID quirks, it might be good to to make
> > > > those affect the "raw" EDID data visible to use space so that values the
> > > > kernel supplies separately are consistent with values extracted from the
> > > > "raw" EDID data.
> > >
> > > If the quirks can be re-encoded back into an EDID representation, then
> > > this sounds like a fairly good approach to me.
> > >
> > > >
> > > > Doing this in the kernel does make it harder to quickly supply fixes for
> > > > a specific user space application. This will probably lead to
> > > > kludge-arounds in user space that could depend on kernel
> > > > version. Perhaps these EDID capabilities in the kernel should be
> > > > versioned separately?
> > > >
> > > > I see good benefits from having user space able to see how the kernel is
> > > > interpreting EDID so that it can adapt as appropriate, but we should be
> > > > cautious about moving functionality into the kernel that would be more
> > > > easily maintained up in user space.
> > > >
> > >
> > > I agree. It seems likely that whatever happens (some) userspace is
> > > still going to implement (some) EDID parsing functionality, so it's
> > > hard to reason about what belongs where. Shared code in userspace
> > > (libdrm?) may well be better than exposing it from the kernel.
> > >
> > > If it is exposed by the kernel, then it's still non-obvious to me
> > > how the kernel exposes that information/interpretation. Adding
> > > a property for every potentially-useful field really doesn't scale
> > > well, and what fields are useful isn't obvious - e.g. serial string vs
> > > serial no., as mentioned by Simon.
> > >
> > > Uma's recent series: "Add HDR Metadata Parsing and handling in DRM
> > > layer"[1] is a good example of more stuff which userspace would want to
> > > parse out of the EDID (supported display colorimetry and transfer
> > > functions).
> > >
> > > It would be nice to avoid duplicating all the CEA extension parsing
> > > code, but the EDID/CEA data structure is extensible by design. So the
> > > kernel API would need to be similarly extensible, or we'll just
> > > balloon loads of properties... and then the kernel API would likely
> > > just end up just looking similar to the CEA block anyway.
> > >
> > > Cheers,
> > > -Brian
> > >
> > > [1] https://lists.freedesktop.org/archives/dri-devel/2018-December/200154.html
> >
> > I would agree with an effort to establish a userspace EDID parsing
> > library in any case. As mentioned above, there will probably be too
> > much to expose via kernel UABI, or it will become just another
> > encoded format that again should have a shared parser library in
> > userspace.
> >
> > Would it be possible to architect the library so that it would be
> > shared with the kernel? Maybe the quirks database could be shared
> > with the kernel as well? That way both kernel and userspace would
> > more or less agree on the parsing details.
>
> Maybe make it part of libdrm and import the shared files periodically
> like we do for driver ioctl interfaces?

We could also merge libdrm into the kernel ... Just as a wild idea to
consider at least. I think it would also help in other areas, keeping
the headers in sync, documenting the uapi properly, and all that.
Would lockstep libdrm release with kernel releases, but most
drivers+igt fixed that by just having copies of the headers in their
tree.
-Daniel

>
> Alex
>
> >
> > I've been dreaming of a "liboutput" that would e.g. parse EDID,
> > generate CVT video mode timings, and whatnot that all display
> > servers more or less will duplicate. Once upon a time Ajax started
> > minitru IIRC...
> >
> > Another thing for "liboutput" was a device description database,
> > whose first use would have been the "non-desktop" property. Because
> > we don't expose monitors through udev to have udev rules tag them
> > with interesting bits.
> >
> > Imagine this: monitors exposed as devices via udev, tagged with
> > helpers as regular monitor, HMD, TV, projector, ... and all EDID
> > fields decoded as well. And quirks in hwdb. But I suppose that
> > won't happen, because a "monitor device node" would have no other
> > use. Except... programmatical monitor controls? Like backlight,
> > brightness, contrast, and so on.
> >
> > Ah, nevermind. :-)
> >
> >
> > Thanks,
> > pq
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: Expose more EDID fields to userspace

[Stéphane Marchesin]

[-- Attachment #1.1: Type: text/plain, Size: 4977 bytes --]

On Mon, Jan 7, 2019, 09:07 Brian Starkey <Brian.Starkey@arm.com wrote:

> On Mon, Jan 07, 2019 at 07:57:54AM -0800, Keith Packard wrote:
> > Daniel Vetter <daniel@ffwll.ch> writes:
> >
> > > Best to pull in some other compositor people and get them to agree.
> From a
> > > kernel pov I'm fine with whatever userspace preferes.
> >
> > Hrm. It would be good to have everyone using the same interpretation of
> > EDID data; in particular, where the kernel has quirks that change the
> > interpretation, user space should be consistent with that.
> >
> > Unless we expose all of the EDID data, then user space may still have to
> > parse EDID. If the kernel has EDID quirks, it might be good to to make
> > those affect the "raw" EDID data visible to use space so that values the
> > kernel supplies separately are consistent with values extracted from the
> > "raw" EDID data.
>
> If the quirks can be re-encoded back into an EDID representation, then
> this sounds like a fairly good approach to me.
>


I don't have strong feelings for against this approach, but if we do this,
I think we should ensure we keep providing the original EDID to user space.
The contents of EDIDs have so many implications that even the kernel isn't
always in the best position to decide if a rewrite is a good idea.

For a simple example, we can look at the max pixel clock which is reported
in the EDID. If the EDID gets rewritten with a lower pixel clock that
matches what the link can do, user space loses the ability to pop up a UI
dialog telling the user that if they were using a better cable, they could
get higher resolutions. Something similar already happens today with some
display dongles which decide to rewrite EDIDs based on their own
limitations. It prevents user space from showing a dialog recommending a
better dongle. Of course one could argue the dongle is protecting itself
here :)



> >
> > Doing this in the kernel does make it harder to quickly supply fixes for
> > a specific user space application. This will probably lead to
> > kludge-arounds in user space that could depend on kernel
> > version. Perhaps these EDID capabilities in the kernel should be
> > versioned separately?
> >
> > I see good benefits from having user space able to see how the kernel is
> > interpreting EDID so that it can adapt as appropriate, but we should be
> > cautious about moving functionality into the kernel that would be more
> > easily maintained up in user space.
> >
>
> I agree. It seems likely that whatever happens (some) userspace is
> still going to implement (some) EDID parsing functionality, so it's
> hard to reason about what belongs where. Shared code in userspace
> (libdrm?) may well be better than exposing it from the kernel.
>
> If it is exposed by the kernel, then it's still non-obvious to me
> how the kernel exposes that information/interpretation. Adding
> a property for every potentially-useful field really doesn't scale
> well, and what fields are useful isn't obvious - e.g. serial string vs
> serial no., as mentioned by Simon.
>
> Uma's recent series: "Add HDR Metadata Parsing and handling in DRM
> layer"[1] is a good example of more stuff which userspace would want to
> parse out of the EDID (supported display colorimetry and transfer
> functions).
>

FWIW for Chrome OS we do parse the color space in user space, since as you
mention this isn't available through the DRM properties.

Tangentially related, the content of these color points is often very...
"buggy". We have to do some sanity checking before deciding to use it or
not. That's why I think that even with all the information parsed by the
kernel, you still need another layer...



>
> It would be nice to avoid duplicating all the CEA extension parsing
> code, but the EDID/CEA data structure is extensible by design. So the
> kernel API would need to be similarly extensible, or we'll just
> balloon loads of properties... and then the kernel API would likely
> just end up just looking similar to the CEA block anyway.
>

Yes I like the idea of parsing in user space, since it doesn't require new
kernel changes at all, and typically updating a user space library is
simpler than changing kernel versions. Frankly it feels to me that the
kernel doesn't really have a business here except passing through the raw
EDID contents to a component which knows better.

Stéphane



>
> Cheers,
> -Brian
>
> [1]
> https://lists.freedesktop.org/archives/dri-devel/2018-December/200154.html
>
> > --
> > -keith
>
>
>
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>

[-- Attachment #1.2: Type: text/html, Size: 6861 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: Expose more EDID fields to userspace

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 2759 bytes --]

Stéphane Marchesin <stephane.marchesin@gmail.com> writes:

> I don't have strong feelings for against this approach, but if we do this,
> I think we should ensure we keep providing the original EDID to user space.
> The contents of EDIDs have so many implications that even the kernel isn't
> always in the best position to decide if a rewrite is a good idea.

Yeah, I think I've talked myself out of modifying EDID too. What I'm
thinking this afternoon is that we should ensure that every value
derived from EDID, (and potentially modified by the kernel), needs to be
exposed to user space so that it too can play with the same
information.

If we could get a common EDID parsing library shared between kernel and
user space, that would at least give everyone the same view of the data.

> For a simple example, we can look at the max pixel clock which is reported
> in the EDID. If the EDID gets rewritten with a lower pixel clock that
> matches what the link can do, user space loses the ability to pop up a UI
> dialog telling the user that if they were using a better cable, they could
> get higher resolutions. Something similar already happens today with some
> display dongles which decide to rewrite EDIDs based on their own
> limitations. It prevents user space from showing a dialog recommending a
> better dongle. Of course one could argue the dongle is protecting itself
> here :)

Oh, that's a fine point -- what this exposes is that user space should
be able to see the lower pixel clock value which the kernel is
using. And doing that separate from the EDID data means it can explain
what's going on.

> FWIW for Chrome OS we do parse the color space in user space, since as you
> mention this isn't available through the DRM properties.

If this isn't used by the kernel, then doing it in user space is
probably the right plan.

> Tangentially related, the content of these color points is often very...
> "buggy". We have to do some sanity checking before deciding to use it or
> not. That's why I think that even with all the information parsed by the
> kernel, you still need another layer...

Right, having a shared library and database for these kinds of quirks
would be awesome.

> Yes I like the idea of parsing in user space, since it doesn't require new
> kernel changes at all, and typically updating a user space library is
> simpler than changing kernel versions. Frankly it feels to me that the
> kernel doesn't really have a business here except passing through the raw
> EDID contents to a component which knows better.

Agreed, as long as we also fix the kernel to tell user space what it's
doing with the EDID data, especially where it's modifying values.

-- 
-keith

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel