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. >> 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 > 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. BR, Jani. > 3) Get panel EDID. > -- Jani Nikula, Intel
