On 1/29/2024 10:46, Jani Nikula wrote:
On Mon, 29 Jan 2024, Mario Limonciello <mario.limonciello@xxxxxxx> wrote:
On 1/29/2024 03:39, Jani Nikula wrote:
On Fri, 26 Jan 2024, Mario Limonciello <mario.limonciello@xxxxxxx> wrote:
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
index 9caba10315a8..c7e1563a46d3 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
@@ -278,6 +278,11 @@ static void amdgpu_connector_get_edid(struct drm_connector *connector)
struct amdgpu_device *adev = drm_to_adev(dev);
struct amdgpu_connector *amdgpu_connector = to_amdgpu_connector(connector);
+ if (amdgpu_connector->edid)
+ return;
+
+ /* if the BIOS specifies the EDID via _DDC, prefer this */
+ amdgpu_connector->edid = amdgpu_acpi_edid(adev, connector);
Imagine the EDID returned by acpi_video_get_edid() has edid->extensions
bigger than 4. Of course it should not, but you have no guarantees, and
it originates outside of the kernel.
The real fix is to have the function return a struct drm_edid which
tracks the allocation size separately. Unfortunately, it requires a
bunch of changes along the way. We've mostly done it in i915, and I've
sent a series to do this in drm/bridge [1].
Looking at it again, perhaps the ACPI code should just return a blob,
and the drm code should have a helper to wrap that around struct
drm_edid, so that the ACPI code does not have to depend on drm. Basic
idea remains.
I'd ideally like to split this stuff and Melissa's rework to be
independent if possible. I'll see if that's actually feasible.
Bottom line, we should stop using struct edid in drivers. They'll all
parse the info differently, and from what I've seen, often wrong.
Thanks for the feedback. In that case this specific change should
probably rebase on the Melissa's work
https://lore.kernel.org/amd-gfx/20240126163429.56714-1-mwen@xxxxxxxxxx/
after she takes into account the feedback.
Let me ask you this though - do you think that after that's done should
we let all drivers get EDID from BIOS as a priority? Or would you
prefer that this is unique to amdgpu?
If the reason for having this is that the panel EDID contains some
garbage, that's certainly not unique to amdgpu... :p
OK; maybe a helper in DRM that wraps the ACPI code then and amdgpu will
use the helper for this series.
I'm also thinking it makes sense to have a new /proc/cmdline setup
option to ignore the BIOS for EDID. I'm hoping that since Windows uses
_DDC that BIOS will be higher quality; but you know; BIOS =)
Something like:
1) If user specifies on kernel command line and puts an EDID in
/lib/firmware use that.
2) If BIOS has EDID in _DDC and it's eDP panel, use that.
I think we should also look into this. We currently don't do this, and
it might help with some machines. However, gut feeling says it's
probably better to keep this as a per driver decision instead of trying
to bolt it into drm helpers.
OK; I'll wire up the helper and if you want to use in the future you can
too then.
BR,
Jani.
3) Get panel EDID.
