[PATCH v4 1/3] drm: Add drm_get_acpi_edid() helper
Mario Limonciello
mario.limonciello at amd.com
Fri Feb 9 15:34:13 UTC 2024
On 2/9/2024 05:07, Daniel Vetter wrote:
> On Thu, Feb 08, 2024 at 11:57:11AM +0200, Jani Nikula wrote:
>> On Wed, 07 Feb 2024, Mario Limonciello <mario.limonciello at amd.com> wrote:
>>> Some manufacturers have intentionally put an EDID that differs from
>>> the EDID on the internal panel on laptops. Drivers can call this
>>> helper to attempt to fetch the EDID from the BIOS's ACPI _DDC method.
>>>
>>> Signed-off-by: Mario Limonciello <mario.limonciello at amd.com>
>>> ---
>>> drivers/gpu/drm/Kconfig | 5 +++
>>> drivers/gpu/drm/drm_edid.c | 77 ++++++++++++++++++++++++++++++++++++++
>>> include/drm/drm_edid.h | 1 +
>>> 3 files changed, 83 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
>>> index 6ec33d36f3a4..ec2bb71e8b36 100644
>>> --- a/drivers/gpu/drm/Kconfig
>>> +++ b/drivers/gpu/drm/Kconfig
>>> @@ -21,6 +21,11 @@ menuconfig DRM
>>> select KCMP
>>> select VIDEO_CMDLINE
>>> select VIDEO_NOMODESET
>>> + select ACPI_VIDEO if ACPI
>>> + select BACKLIGHT_CLASS_DEVICE if ACPI
>>> + select INPUT if ACPI
>>> + select X86_PLATFORM_DEVICES if ACPI && X86
>>> + select ACPI_WMI if ACPI && X86
>>
>> I think I'll defer to drm maintainers on whether this is okay or
>> something to be avoided.
>
> Uh yeah this is a bit much, and select just messes with everything. Just
> #ifdef this in the code with a dummy alternative, if users configure their
> kernel without acpi but need it, they get to keep all the pieces.
>
> Alternatively make a DRM_ACPI_HELPERS symbol, but imo a Kconfig for every
> function is also not great. And just using #ifdef in the code also works
> for CONFIG_OF, which is exactly the same thing for platforms using dt to
> describe hw.
>
> Also I'd expect ACPI code to already provide dummy functions if ACPI is
> provided, so you probably dont even need all that much #ifdef in the code.
>
> What we defo cant do is select platform/hw stuff just because you enable
> CONFIG_DRM.
> -Sima
The problem was with linking. I'll experiment with #ifdef for the next
version.
>
>>
>>
>>> help
>>> Kernel-level support for the Direct Rendering Infrastructure (DRI)
>>> introduced in XFree86 4.0. If you say Y here, you need to select
>>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>>> index 923c4423151c..c649b4f9fd8e 100644
>>> --- a/drivers/gpu/drm/drm_edid.c
>>> +++ b/drivers/gpu/drm/drm_edid.c
>>> @@ -28,6 +28,7 @@
>>> * DEALINGS IN THE SOFTWARE.
>>> */
>>>
>>> +#include <acpi/video.h>
>>> #include <linux/bitfield.h>
>>> #include <linux/cec.h>
>>> #include <linux/hdmi.h>
>>> @@ -2188,6 +2189,49 @@ drm_do_probe_ddc_edid(void *data, u8 *buf, unsigned int block, size_t len)
>>> return ret == xfers ? 0 : -1;
>>> }
>>>
>>> +/**
>>> + * drm_do_probe_acpi_edid() - get EDID information via ACPI _DDC
>>> + * @data: struct drm_device
>>> + * @buf: EDID data buffer to be filled
>>> + * @block: 128 byte EDID block to start fetching from
>>> + * @len: EDID data buffer length to fetch
>>> + *
>>> + * Try to fetch EDID information by calling acpi_video_get_edid() function.
>>> + *
>>> + * Return: 0 on success or error code on failure.
>>> + */
>>> +static int
>>> +drm_do_probe_acpi_edid(void *data, u8 *buf, unsigned int block, size_t len)
>>> +{
>>> + struct drm_device *ddev = data;
>>> + struct acpi_device *acpidev = ACPI_COMPANION(ddev->dev);
>>> + unsigned char start = block * EDID_LENGTH;
>>> + void *edid;
>>> + int r;
>>> +
>>> + if (!acpidev)
>>> + return -ENODEV;
>>> +
>>> + /* fetch the entire edid from BIOS */
>>> + r = acpi_video_get_edid(acpidev, ACPI_VIDEO_DISPLAY_LCD, -1, &edid);
>>> + if (r < 0) {
>>> + DRM_DEBUG_KMS("Failed to get EDID from ACPI: %d\n", r);
>>> + return -EINVAL;
>>> + }
>>> + if (len > r || start > r || start + len > r) {
>>> + r = -EINVAL;
>>> + goto cleanup;
>>> + }
>>> +
>>> + memcpy(buf, edid + start, len);
>>> + r = 0;
>>> +
>>> +cleanup:
>>> + kfree(edid);
>>> +
>>> + return r;
>>> +}
>>> +
>>> static void connector_bad_edid(struct drm_connector *connector,
>>> const struct edid *edid, int num_blocks)
>>> {
>>> @@ -2643,6 +2687,39 @@ struct edid *drm_get_edid(struct drm_connector *connector,
>>> }
>>> EXPORT_SYMBOL(drm_get_edid);
>>>
>>> +/**
>>> + * drm_get_acpi_edid - get EDID data, if available
>>
>> I'd prefer all the new EDID API to be named drm_edid_*. Makes a clean
>> break from the old API, and is more consistent.
>>
>> So perhaps drm_edid_read_acpi() to be in line with all the other struct
>> drm_edid based EDID reading functions.
>>
>>> + * @connector: connector we're probing
>>> + *
>>> + * Use the BIOS to attempt to grab EDID data if possible.
>>> + *
>>> + * The returned pointer must be freed using drm_edid_free().
>>> + *
>>> + * Return: Pointer to valid EDID or NULL if we couldn't find any.
>>> + */
>>> +const struct drm_edid *drm_get_acpi_edid(struct drm_connector *connector)
>>> +{
>>> + const struct drm_edid *drm_edid;
>>> +
>>> + switch (connector->connector_type) {
>>> + case DRM_MODE_CONNECTOR_LVDS:
>>> + case DRM_MODE_CONNECTOR_eDP:
>>> + break;
>>> + default:
>>> + return NULL;
>>> + }
>>> +
>>> + if (connector->force == DRM_FORCE_OFF)
>>> + return NULL;
>>> +
>>> + drm_edid = drm_edid_read_custom(connector, drm_do_probe_acpi_edid, connector->dev);
>>> +
>>> + /* Note: Do *not* call connector updates here. */
>>> +
>>> + return drm_edid;
>>> +}
>>> +EXPORT_SYMBOL(drm_get_acpi_edid);
>>> +
>>> /**
>>> * drm_edid_read_custom - Read EDID data using given EDID block read function
>>> * @connector: Connector to use
>>> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
>>> index 7923bc00dc7a..ca41be289fc6 100644
>>> --- a/include/drm/drm_edid.h
>>> +++ b/include/drm/drm_edid.h
>>> @@ -410,6 +410,7 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
>>> void *data);
>>> struct edid *drm_get_edid(struct drm_connector *connector,
>>> struct i2c_adapter *adapter);
>>> +const struct drm_edid *drm_get_acpi_edid(struct drm_connector *connector);
>>
>> There's a comment
>>
>> /* Interface based on struct drm_edid */
>>
>> towards the end of the file, gathering all the new API under it.
>>
>> Other than that, LGTM,
>>
>> BR,
>> Jani.
>>
>>> u32 drm_edid_get_panel_id(struct i2c_adapter *adapter);
>>> struct edid *drm_get_edid_switcheroo(struct drm_connector *connector,
>>> struct i2c_adapter *adapter);
>>
>> --
>> Jani Nikula, Intel
>
More information about the amd-gfx
mailing list