On Fri, 10 Jun 2022, Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> wrote:
> On Wed, Jun 08, 2022 at 10:50:35AM +0300, Jani Nikula wrote:
>> Add new functions drm_edid_read(), drm_edid_read_ddc(), and
>> drm_edid_read_custom() to replace drm_get_edid() and drm_do_get_edid()
>> for reading the EDID. The transition is expected to happen over a fairly
>> long time.
>>
>> Note that the new drm_edid_read*() functions do not do any of the
>> connector updates anymore. The reading and parsing will be completely
>> separated from each other.
>>
>> Add new functions drm_edid_alloc(), drm_edid_dup(), and drm_edid_free()
>> for allocating and freeing drm_edid containers.
>>
>> Cc: David Airlie <airlied@xxxxxxxx>
>> Cc: Daniel Vetter <daniel@xxxxxxxx>
>> Signed-off-by: Jani Nikula <jani.nikula@xxxxxxxxx>
>> ---
>> drivers/gpu/drm/drm_edid.c | 245 +++++++++++++++++++++++++++++++++----
>> include/drm/drm_edid.h | 9 ++
>> 2 files changed, 230 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>> index 2beaa48301c1..2bdaf1e34a9d 100644
>> --- a/drivers/gpu/drm/drm_edid.c
>> +++ b/drivers/gpu/drm/drm_edid.c
>> @@ -2226,29 +2226,9 @@ static enum edid_block_status edid_block_read(void *block, unsigned int block_nu
>> return status;
>> }
>>
>> -/**
>> - * drm_do_get_edid - get EDID data using a custom EDID block read function
>> - * @connector: connector we're probing
>> - * @read_block: EDID block read function
>> - * @context: private data passed to the block read function
>> - *
>> - * When the I2C adapter connected to the DDC bus is hidden behind a device that
>> - * exposes a different interface to read EDID blocks this function can be used
>> - * to get EDID data using a custom block read function.
>> - *
>> - * As in the general case the DDC bus is accessible by the kernel at the I2C
>> - * level, drivers must make all reasonable efforts to expose it as an I2C
>> - * adapter and use drm_get_edid() instead of abusing this function.
>> - *
>> - * The EDID may be overridden using debugfs override_edid or firmware EDID
>> - * (drm_load_edid_firmware() and drm.edid_firmware parameter), in this priority
>> - * order. Having either of them bypasses actual EDID reads.
>> - *
>> - * Return: Pointer to valid EDID or NULL if we couldn't find any.
>> - */
>> -struct edid *drm_do_get_edid(struct drm_connector *connector,
>> - read_block_fn read_block,
>> - void *context)
>> +static struct edid *_drm_do_get_edid(struct drm_connector *connector,
>> + read_block_fn read_block, void *context,
>> + size_t *size)
>> {
>> enum edid_block_status status;
>> int i, invalid_blocks = 0;
>> @@ -2315,14 +2295,125 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
>> }
>>
>> ok:
>> + if (size)
>> + *size = alloc_size;
>> +
>> return edid;
>>
>> fail:
>> kfree(edid);
>> return NULL;
>> }
>> +
>> +/**
>> + * drm_do_get_edid - get EDID data using a custom EDID block read function
>> + * @connector: connector we're probing
>> + * @read_block: EDID block read function
>> + * @context: private data passed to the block read function
>> + *
>> + * When the I2C adapter connected to the DDC bus is hidden behind a device that
>> + * exposes a different interface to read EDID blocks this function can be used
>> + * to get EDID data using a custom block read function.
>> + *
>> + * As in the general case the DDC bus is accessible by the kernel at the I2C
>> + * level, drivers must make all reasonable efforts to expose it as an I2C
>> + * adapter and use drm_get_edid() instead of abusing this function.
>> + *
>> + * The EDID may be overridden using debugfs override_edid or firmware EDID
>> + * (drm_load_edid_firmware() and drm.edid_firmware parameter), in this priority
>> + * order. Having either of them bypasses actual EDID reads.
>> + *
>> + * Return: Pointer to valid EDID or NULL if we couldn't find any.
>> + */
>> +struct edid *drm_do_get_edid(struct drm_connector *connector,
>> + read_block_fn read_block,
>> + void *context)
>> +{
>> + return _drm_do_get_edid(connector, read_block, context, NULL);
>> +}
>> EXPORT_SYMBOL_GPL(drm_do_get_edid);
>>
>> +/* Allocate struct drm_edid container *without* duplicating the edid data */
>> +static const struct drm_edid *_drm_edid_alloc(const void *edid, size_t size)
>> +{
>> + struct drm_edid *drm_edid;
>> +
>> + if (!edid || !size || size < EDID_LENGTH)
>> + return NULL;
>> +
>> + drm_edid = kzalloc(sizeof(*drm_edid), GFP_KERNEL);
>> + if (drm_edid) {
>> + drm_edid->edid = edid;
>> + drm_edid->size = size;
>> + }
>> +
>> + return drm_edid;
>> +}
>> +
>> +/**
>> + * drm_edid_alloc - Allocate a new drm_edid container
>> + * @edid: Pointer to raw EDID data
>> + * @size: Size of memory allocated for EDID
>> + *
>> + * Allocate a new drm_edid container. Do not calculate edid size from edid, pass
>> + * the actual size that has been allocated for the data. There is no validation
>> + * of the raw EDID data against the size, but at least the EDID base block must
>> + * fit in the buffer.
>> + *
>> + * The returned pointer must be freed using drm_edid_free().
>> + *
>> + * Return: drm_edid container, or NULL on errors
>> + */
>> +const struct drm_edid *drm_edid_alloc(const void *edid, size_t size)
>> +{
>> + const struct drm_edid *drm_edid;
>> +
>> + if (!edid || !size || size < EDID_LENGTH)
>> + return NULL;
>> +
>> + edid = kmemdup(edid, size, GFP_KERNEL);
>> + if (!edid)
>> + return NULL;
>> +
>> + drm_edid = _drm_edid_alloc(edid, size);
>> + if (!drm_edid)
>> + kfree(edid);
>> +
>> + return drm_edid;
>> +}
>> +EXPORT_SYMBOL(drm_edid_alloc);
>> +
>> +/**
>> + * drm_edid_dup - Duplicate a drm_edid container
>> + * @drm_edid: EDID to duplicate
>> + *
>> + * The returned pointer must be freed using drm_edid_free().
>> + *
>> + * Returns: drm_edid container copy, or NULL on errors
>> + */
>> +const struct drm_edid *drm_edid_dup(const struct drm_edid *drm_edid)
>> +{
>> + if (!drm_edid)
>> + return NULL;
>> +
>> + return drm_edid_alloc(drm_edid->edid, drm_edid->size);
>> +}
>> +EXPORT_SYMBOL(drm_edid_dup);
>> +
>> +/**
>> + * drm_edid_free - Free the drm_edid container
>> + * @drm_edid: EDID to free
>> + */
>> +void drm_edid_free(const struct drm_edid *drm_edid)
>> +{
>> + if (!drm_edid)
>> + return;
>> +
>> + kfree(drm_edid->edid);
>> + kfree(drm_edid);
>> +}
>> +EXPORT_SYMBOL(drm_edid_free);
>> +
>> /**
>> * drm_probe_ddc() - probe DDC presence
>> * @adapter: I2C adapter to probe
>> @@ -2359,12 +2450,118 @@ struct edid *drm_get_edid(struct drm_connector *connector,
>> if (connector->force == DRM_FORCE_UNSPECIFIED && !drm_probe_ddc(adapter))
>> return NULL;
>>
>> - edid = drm_do_get_edid(connector, drm_do_probe_ddc_edid, adapter);
>> + edid = _drm_do_get_edid(connector, drm_do_probe_ddc_edid, adapter, NULL);
>> drm_connector_update_edid_property(connector, edid);
>> return edid;
>> }
>> EXPORT_SYMBOL(drm_get_edid);
>>
>> +/**
>> + * drm_edid_read_custom - Read EDID data using given EDID block read function
>> + * @connector: Connector to use
>> + * @read_block: EDID block read function
>> + * @context: Private data passed to the block read function
>> + *
>> + * When the I2C adapter connected to the DDC bus is hidden behind a device that
>> + * exposes a different interface to read EDID blocks this function can be used
>> + * to get EDID data using a custom block read function.
>> + *
>> + * As in the general case the DDC bus is accessible by the kernel at the I2C
>> + * level, drivers must make all reasonable efforts to expose it as an I2C
>> + * adapter and use drm_edid_read() or drm_edid_read_ddc() instead of abusing
>> + * this function.
>> + *
>> + * The EDID may be overridden using debugfs override_edid or firmware EDID
>> + * (drm_load_edid_firmware() and drm.edid_firmware parameter), in this priority
>> + * order. Having either of them bypasses actual EDID reads.
>> + *
>> + * The returned pointer must be freed using drm_edid_free().
>> + *
>> + * Return: Pointer to EDID, or NULL if probe/read failed.
>> + */
>> +const struct drm_edid *drm_edid_read_custom(struct drm_connector *connector,
>> + read_block_fn read_block,
>> + void *context)
>> +{
>> + const struct drm_edid *drm_edid;
>> + struct edid *edid;
>> + size_t size = 0;
>> +
>> + edid = _drm_do_get_edid(connector, read_block, context, &size);
>> + if (!edid)
>> + return NULL;
>> +
>> + /* Sanity check for now */
>> + drm_WARN_ON(connector->dev, !size);
>> +
>> + drm_edid = _drm_edid_alloc(edid, size);
>> + if (!drm_edid)
>> + kfree(edid);
>> +
>> + return drm_edid;
>> +}
>> +EXPORT_SYMBOL(drm_edid_read_custom);
>> +
>> +/**
>> + * drm_edid_read_ddc - Read EDID data using given I2C adapter
>> + * @connector: Connector to use
>> + * @adapter: I2C adapter to use for DDC
>> + *
>> + * Read EDID using the given I2C adapter.
>> + *
>> + * The EDID may be overridden using debugfs override_edid or firmware EDID
>> + * (drm_load_edid_firmware() and drm.edid_firmware parameter), in this priority
>> + * order. Having either of them bypasses actual EDID reads.
>> + *
>> + * Prefer initializing connector->ddc with drm_connector_init_with_ddc() and
>> + * using drm_edid_read() instead of this function.
>> + *
>> + * The returned pointer must be freed using drm_edid_free().
>> + *
>> + * Return: Pointer to EDID, or NULL if probe/read failed.
>> + */
>> +const struct drm_edid *drm_edid_read_ddc(struct drm_connector *connector,
>> + struct i2c_adapter *adapter)
>> +{
>> + const struct drm_edid *drm_edid;
>> +
>> + if (connector->force == DRM_FORCE_OFF)
>> + return NULL;
>> +
>> + if (connector->force == DRM_FORCE_UNSPECIFIED && !drm_probe_ddc(adapter))
>> + return NULL;
>> +
>> + drm_edid = drm_edid_read_custom(connector, drm_do_probe_ddc_edid, adapter);
>> +
>> + /* Note: Do *not* call connector updates here. */
>> +
>> + return drm_edid;
>> +}
>> +EXPORT_SYMBOL(drm_edid_read_ddc);
>> +
>> +/**
>> + * drm_edid_read - Read EDID data using connector's I2C adapter
>> + * @connector: Connector to use
>> + *
>> + * Read EDID using the connector's I2C adapter.
>> + *
>> + * The EDID may be overridden using debugfs override_edid or firmware EDID
>> + * (drm_load_edid_firmware() and drm.edid_firmware parameter), in this priority
>> + * order. Having either of them bypasses actual EDID reads.
>> + *
>> + * The returned pointer must be freed using drm_edid_free().
>> + *
>> + * Return: Pointer to EDID, or NULL if probe/read failed.
>> + */
>> +const struct drm_edid *drm_edid_read(struct drm_connector *connector)
>> +{
>> + if (drm_WARN_ON(connector->dev, !connector->ddc))
>> + return NULL;
>> +
>> + return drm_edid_read_ddc(connector, connector->ddc);
>> +}
>> +EXPORT_SYMBOL(drm_edid_read);
>
> I'm wondering if we need this drm_edid_read() vs. drm_edid_read_ddc()
> split? Ie. are there cases where connector->ddc wouldn't be populated
> for some reason but we still want to use drm_edid_read_ddc()?
I want to promote initializing connector->ddc and using drm_edid_read()
with that. However, for e.g. i915, HDMI is the only connector where we
have connector->ddc initialized. I just didn't want to take on
converting that part too, not yet, not as part of this series.
BR,
Jani.
>
>> +
>> static u32 edid_extract_panel_id(const struct edid *edid)
>> {
>> /*
>> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
>> index 95ac09ef41b2..9d2d78135dee 100644
>> --- a/include/drm/drm_edid.h
>> +++ b/include/drm/drm_edid.h
>> @@ -594,6 +594,15 @@ drm_display_mode_from_cea_vic(struct drm_device *dev,
>> u8 video_code);
>>
>> /* Interface based on struct drm_edid */
>> +const struct drm_edid *drm_edid_alloc(const void *edid, size_t size);
>> +const struct drm_edid *drm_edid_dup(const struct drm_edid *drm_edid);
>> +void drm_edid_free(const struct drm_edid *drm_edid);
>> +const struct drm_edid *drm_edid_read(struct drm_connector *connector);
>> +const struct drm_edid *drm_edid_read_ddc(struct drm_connector *connector,
>> + struct i2c_adapter *adapter);
>> +const struct drm_edid *drm_edid_read_custom(struct drm_connector *connector,
>> + int (*read_block)(void *context, u8 *buf, unsigned int block, size_t len),
>> + void *context);
>> const u8 *drm_find_edid_extension(const struct drm_edid *drm_edid,
>> int ext_id, int *ext_index);
>>
>> --
>> 2.30.2
--
Jani Nikula, Intel Open Source Graphics Center
