drm/edid: Add option to report basic audio support with a generic edid

[popcornmix]

Hardware that fails to read the edid is quite common. The kernel provides a mechanism to use one a of a few pre-built generic edid files for getting a basic video mode.

However there is no easy was to add basic audio support, which requires a CTA extension block to report capabilities.

Add an module option to request this.

[6by9]

Looks fine in principle. A couple of checkpatch complaints.

IMHO It'd be nicer if it took drm.edid_firmware=edid/800x600.bin+audio or equivalent instead of the separate module parameter of support_audio. Having it independent sort of implies that it will add audio support to any EDID, not just to the builtin ones.

[popcornmix]

Updated. I've gone for drm.edid_firmware=audio+edid/800x600.bin as its cleaner to parse.
I couldn't see a way of parsing +audio without creating a new, modified string, or a new match_string function (that takes a length).

I could switch if it's preferred (I do slightly prefer the +audio form).

[6by9]

I think the cleanest way would be to have a new version of match_string that does

if (!strncmp(item, string, strlen(item)))

so that it matches against the start of generic_edid_name, and then you could look for the +audio on the end with

if (strncmp(&name[strlen(generic_edid_name[builtin])], "+audio", 6) 

One to bounce off Maxime to get his views.

[popcornmix]

@mripard I'd be interested in your opinion.

[mripard]

The separator used in parameters is usually the comma, so having something like edid_firmware=path,audio would be more consistent.

That being said, my recollection was that last time we discussed it, we settled down on providing EDID firmware files with audio support which wouldn't require anything in the kernel?

It feels a bit weird to me to tell the kernel to load the EDIDs from somewhere, but then also tell it that actually they should be modified because it's not really what we want.

[popcornmix]

That being said, my recollection was that last time we discussed it, we settled down on providing EDID firmware files with audio support which wouldn't require anything in the kernel?

are you saying that discussion was with us? I don't recall that as the outcome. How would the firmware files be distributed (on ours and other distributions)?

Using a physical file makes things a lot more complicated. e.g. on LibreELEC (where this would be a common request), the rootfs is readonly/squashfs, so using a custom edid requires an initramfs. A command line solution is much more usable.

I could understand the argument for a new embedded file, so the edids are not modified, but there is a pre-made 256 byte pseudo file that supports audio. But that would slightly increase RAM required if you support multiple resolutions. Although I feel only 1080p (and perhaps 720p) will cover almost all use cases (displays that support audio are much more likely to support the more standard modes).

[6by9]

The separator used in parameters is usually the comma, so having something like edid_firmware=path,audio would be more consistent.

We've already got comma separating connector specific edid files, eg edid_firmware=HDMI-A-1:path1,HDMI-A-2:path2. So either audio then becomes a global flag to add audio to all EDIDs (not what's currently being proposed), or comma can't be used.

It feels a bit weird to me to tell the kernel to load the EDIDs from somewhere, but then also tell it that actually they should be modified because it's not really what we want.

This is "loading" the internal built in EDIDs, not loading a file from anywhere.
The other option would be to extend the generic_edid_names table and have additional encoded EDIDs with audio support, however you then have the awkardness of those without audio being 128bytes whilst with audio is 256 bytes. Possible, but it just feels wasteful.

[macmpi]

Sorry to bump as I'm interested in the feature outcome...
Is there a resolution for the pending issues, or has it been addressed upstream?

[macmpi]

@popcornmix any plan to move this cool feature forward?

[popcornmix]

It sounds like this scheme is unlikely to be supported upstream so I don't think this PR will move forward.
You are free to use this commit in a self-built kernel.

I wouldn't mind creating a tool to create custom edid files based on a set of user choices but it is low priority on the todo list.

[6by9]

All the builtin EDIDs are gone in 6.9, so definitely a dead end
torvalds/linux@89ac522

I seem to recall seeing a library for creating basic EDIDs. I'll try and find it again.

[6by9]

https://codeberg.org/ral/pasedid was a library I'd been pointed to at one point.
Then again just trying it, and it's barfed on the first two valid EDID files I've tried decoding.

Maxime also has https://github.com/mripard/redid which he uses from the CI test framework in https://github.com/mripard/dradis. It looks like https://github.com/mripard/dradis/blob/main/src/helpers.rs#L158-L256 could be persuaded to be vaguely useful.
Now to collar someone who really knows Rust...

[macmpi]

Thanks @popcornmix and @6by9 for the quick note and references.
Sad there's no other rescue than external file requirement for a baseline "just-works" audio&video

[mripard]

Maxime also has https://github.com/mripard/redid which he uses from the CI test framework in https://github.com/mripard/dradis. It looks like https://github.com/mripard/dradis/blob/main/src/helpers.rs#L158-L256 could be persuaded to be vaguely useful. Now to collar someone who really knows Rust...

I also have a tool to generate an EDID from a YAML file. It's not in a great shape right now, but I'll work on cleaning it up.