[PATCH 6/7] drm/udl: Untangle .get_modes() and .detect_ctx()
Thomas Zimmermann
tzimmermann at suse.de
Fri Apr 5 07:38:09 UTC 2024
Hi
Am 05.04.24 um 09:09 schrieb Jani Nikula:
> On Thu, 04 Apr 2024, Thomas Zimmermann <tzimmermann at suse.de> wrote:
>> Provide separate implementations of .get_modes() and .detect_ctx()
>> from struct drm_connector. Switch to struct drm_edid.
>>
>> Udl's .detect() helper used to fetch the EDID from the adapter and the
>> .get_modes() helper provided display modes from the data. Switching to
>> the new helpers around struct drm_edid separates both from each other. The
>> .get_modes() helper now fetches the EDID by itself and the .detect_ctx()
>> helper only tests for its presence.
> FWIW, this is what I had for UDL in my branch of various drm_edid
> conversions:
>
> https://gitlab.freedesktop.org/jani/linux/-/commit/c6357a778182eff7acfb1eb832809377f799edaf
>
> You seem to claim that there's inherent value in separating detect and
> get_modes hooks from each other, with the former not reading the full
> EDID.
Yes. If udl stores the EDID in struct udl_connector and later uses it in
get_modes, the detect always has to run before get_modes. That logic is
deeply hidden in the DRM probe helpers. So in any case, I want get_modes
to read the EDID by itself.
In detect or detect_ctx, using drm_edid_read_custom() might do
unnecessary USB transfers. Maybe not a problem per-se, but udl is
already pushing the limits of its USB 2 bus. And the read function also
leaves a warning about the all-zero EDID data in the kernel log every
few seconds. Probably from [1].
[1]
https://elixir.bootlin.com/linux/v6.8/source/drivers/gpu/drm/drm_edid.c#L2389
>
> If you do read the full EDID using drm_edid_read_custom() in detect,
> you'll get more validation of the EDID for free, with no need to add
> extra drm edid interfaces for probing with optional header validation.
>
> Some drivers need to use EDID contents for the purpose of detect
> too. Perhaps not UDL.
>
> Also let's look at the override/firmware EDID mechanism. If you have a
> completely broken EDID, the newly added drm_edid_probe_custom() will
> never detect connected. You'll need to use connector forcing, even if
> the DDC probe would have been enough.
The EDID probe helper can take this into account. If it's just the code
at [2], it looks simple enough to re-use.
[2]
https://elixir.bootlin.com/linux/v6.8/source/drivers/gpu/drm/drm_edid.c#L2373
Best regards
Thomas
>
> BR,
> Jani.
>
>> The patch does a number of things to implement this.
>>
>> - Move udl_get_edid_block() to udl_edid.c and rename it to
>> udl_read_edid_block(). Then use the helper to implement probing in
>> udl_probe_edid() and reading in udl_edid_read(). Both udl helpers
>> are build on top of DRM helpers.
>>
>> - Replace the existing code in .get_modes() and .detect() with udl's
>> new EDID helpers. The new code behaves like DRM's similar DDC-based
>> helpers. Instead of .detect(), udl now implements .detect_ctx().
>>
>> - Remove the edid data from struct udl_connector. The field cached
>> the EDID data between calls to .detect() and .get_modes(), but is now
>> unused.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann at suse.de>
>> ---
>> drivers/gpu/drm/udl/Makefile | 1 +
>> drivers/gpu/drm/udl/udl_drv.h | 2 -
>> drivers/gpu/drm/udl/udl_edid.c | 67 +++++++++++++++++++++++
>> drivers/gpu/drm/udl/udl_edid.h | 15 ++++++
>> drivers/gpu/drm/udl/udl_modeset.c | 90 +++++++------------------------
>> 5 files changed, 102 insertions(+), 73 deletions(-)
>> create mode 100644 drivers/gpu/drm/udl/udl_edid.c
>> create mode 100644 drivers/gpu/drm/udl/udl_edid.h
>>
>> diff --git a/drivers/gpu/drm/udl/Makefile b/drivers/gpu/drm/udl/Makefile
>> index 00690741db376..43d69a16af183 100644
>> --- a/drivers/gpu/drm/udl/Makefile
>> +++ b/drivers/gpu/drm/udl/Makefile
>> @@ -2,6 +2,7 @@
>>
>> udl-y := \
>> udl_drv.o \
>> + udl_edid.o \
>> udl_main.o \
>> udl_modeset.o \
>> udl_transfer.o
>> diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
>> index 282ebd6c02fda..f112cfb270f31 100644
>> --- a/drivers/gpu/drm/udl/udl_drv.h
>> +++ b/drivers/gpu/drm/udl/udl_drv.h
>> @@ -51,8 +51,6 @@ struct urb_list {
>>
>> struct udl_connector {
>> struct drm_connector connector;
>> - /* last udl_detect edid */
>> - struct edid *edid;
>> };
>>
>> static inline struct udl_connector *to_udl_connector(struct drm_connector *connector)
>> diff --git a/drivers/gpu/drm/udl/udl_edid.c b/drivers/gpu/drm/udl/udl_edid.c
>> new file mode 100644
>> index 0000000000000..caa9641996e92
>> --- /dev/null
>> +++ b/drivers/gpu/drm/udl/udl_edid.c
>> @@ -0,0 +1,67 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +
>> +#include <drm/drm_drv.h>
>> +#include <drm/drm_edid.h>
>> +
>> +#include "udl_drv.h"
>> +#include "udl_edid.h"
>> +
>> +static int udl_read_edid_block(void *data, u8 *buf, unsigned int block, size_t len)
>> +{
>> + struct udl_device *udl = data;
>> + struct drm_device *dev = &udl->drm;
>> + struct usb_device *udev = udl_to_usb_device(udl);
>> + u8 *read_buff;
>> + int idx, ret;
>> + size_t i;
>> +
>> + read_buff = kmalloc(2, GFP_KERNEL);
>> + if (!read_buff)
>> + return -ENOMEM;
>> +
>> + if (!drm_dev_enter(dev, &idx)) {
>> + ret = -ENODEV;
>> + goto err_kfree;
>> + }
>> +
>> + for (i = 0; i < len; i++) {
>> + int bval = (i + block * EDID_LENGTH) << 8;
>> +
>> + ret = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
>> + 0x02, (0x80 | (0x02 << 5)), bval,
>> + 0xA1, read_buff, 2, USB_CTRL_GET_TIMEOUT);
>> + if (ret < 0) {
>> + drm_err(dev, "Read EDID byte %zu failed err %x\n", i, ret);
>> + goto err_drm_dev_exit;
>> + } else if (ret < 1) {
>> + ret = -EIO;
>> + drm_err(dev, "Read EDID byte %zu failed\n", i);
>> + goto err_drm_dev_exit;
>> + }
>> +
>> + buf[i] = read_buff[1];
>> + }
>> +
>> + drm_dev_exit(idx);
>> + kfree(read_buff);
>> +
>> + return 0;
>> +
>> +err_drm_dev_exit:
>> + drm_dev_exit(idx);
>> +err_kfree:
>> + kfree(read_buff);
>> + return ret;
>> +}
>> +
>> +bool udl_probe_edid(struct udl_device *udl)
>> +{
>> + return drm_edid_probe_custom(udl_read_edid_block, udl, true);
>> +}
>> +
>> +const struct drm_edid *udl_edid_read(struct drm_connector *connector)
>> +{
>> + struct udl_device *udl = to_udl(connector->dev);
>> +
>> + return drm_edid_read_custom(connector, udl_read_edid_block, udl);
>> +}
>> diff --git a/drivers/gpu/drm/udl/udl_edid.h b/drivers/gpu/drm/udl/udl_edid.h
>> new file mode 100644
>> index 0000000000000..fe15ff3752b7d
>> --- /dev/null
>> +++ b/drivers/gpu/drm/udl/udl_edid.h
>> @@ -0,0 +1,15 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +
>> +#ifndef UDL_EDID_H
>> +#define UDL_EDID_H
>> +
>> +#include <linux/types.h>
>> +
>> +struct drm_connector;
>> +struct drm_edid;
>> +struct udl_device;
>> +
>> +bool udl_probe_edid(struct udl_device *udl);
>> +const struct drm_edid *udl_edid_read(struct drm_connector *connector);
>> +
>> +#endif
>> diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
>> index 3df9fc38388b4..4236ce57f5945 100644
>> --- a/drivers/gpu/drm/udl/udl_modeset.c
>> +++ b/drivers/gpu/drm/udl/udl_modeset.c
>> @@ -25,6 +25,7 @@
>> #include <drm/drm_vblank.h>
>>
>> #include "udl_drv.h"
>> +#include "udl_edid.h"
>> #include "udl_proto.h"
>>
>> /*
>> @@ -415,97 +416,44 @@ static const struct drm_encoder_funcs udl_encoder_funcs = {
>>
>> static int udl_connector_helper_get_modes(struct drm_connector *connector)
>> {
>> - struct udl_connector *udl_connector = to_udl_connector(connector);
>> + const struct drm_edid *drm_edid;
>> + int count;
>>
>> - drm_connector_update_edid_property(connector, udl_connector->edid);
>> - if (udl_connector->edid)
>> - return drm_add_edid_modes(connector, udl_connector->edid);
>> + drm_edid = udl_edid_read(connector);
>> + drm_edid_connector_update(connector, drm_edid);
>> + count = drm_edid_connector_add_modes(connector);
>> + drm_edid_free(drm_edid);
>>
>> - return 0;
>> + return count;
>> }
>>
>> -static const struct drm_connector_helper_funcs udl_connector_helper_funcs = {
>> - .get_modes = udl_connector_helper_get_modes,
>> -};
>> -
>> -static int udl_get_edid_block(void *data, u8 *buf, unsigned int block, size_t len)
>> +static int udl_connector_helper_detect_ctx(struct drm_connector *connector,
>> + struct drm_modeset_acquire_ctx *ctx,
>> + bool force)
>> {
>> - struct udl_device *udl = data;
>> - struct drm_device *dev = &udl->drm;
>> - struct usb_device *udev = udl_to_usb_device(udl);
>> - u8 *read_buff;
>> - int idx, ret;
>> - size_t i;
>> -
>> - read_buff = kmalloc(2, GFP_KERNEL);
>> - if (!read_buff)
>> - return -ENOMEM;
>> + struct udl_device *udl = to_udl(connector->dev);
>>
>> - if (!drm_dev_enter(dev, &idx)) {
>> - ret = -ENODEV;
>> - goto err_kfree;
>> - }
>> -
>> - for (i = 0; i < len; i++) {
>> - int bval = (i + block * EDID_LENGTH) << 8;
>> -
>> - ret = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
>> - 0x02, (0x80 | (0x02 << 5)), bval,
>> - 0xA1, read_buff, 2, USB_CTRL_GET_TIMEOUT);
>> - if (ret < 0) {
>> - drm_err(dev, "Read EDID byte %zu failed err %x\n", i, ret);
>> - goto err_drm_dev_exit;
>> - } else if (ret < 1) {
>> - ret = -EIO;
>> - drm_err(dev, "Read EDID byte %zu failed\n", i);
>> - goto err_drm_dev_exit;
>> - }
>> -
>> - buf[i] = read_buff[1];
>> - }
>> + if (udl_probe_edid(udl))
>> + return connector_status_connected;
>>
>> - drm_dev_exit(idx);
>> - kfree(read_buff);
>> -
>> - return 0;
>> -
>> -err_drm_dev_exit:
>> - drm_dev_exit(idx);
>> -err_kfree:
>> - kfree(read_buff);
>> - return ret;
>> + return connector_status_disconnected;
>> }
>>
>> -static enum drm_connector_status udl_connector_detect(struct drm_connector *connector, bool force)
>> -{
>> - struct drm_device *dev = connector->dev;
>> - struct udl_device *udl = to_udl(dev);
>> - struct udl_connector *udl_connector = to_udl_connector(connector);
>> - enum drm_connector_status status = connector_status_disconnected;
>> -
>> - /* cleanup previous EDID */
>> - kfree(udl_connector->edid);
>> - udl_connector->edid = NULL;
>> -
>> - udl_connector->edid = drm_do_get_edid(connector, udl_get_edid_block, udl);
>> - if (udl_connector->edid)
>> - status = connector_status_connected;
>> -
>> - return status;
>> -}
>> +static const struct drm_connector_helper_funcs udl_connector_helper_funcs = {
>> + .get_modes = udl_connector_helper_get_modes,
>> + .detect_ctx = udl_connector_helper_detect_ctx,
>> +};
>>
>> static void udl_connector_destroy(struct drm_connector *connector)
>> {
>> struct udl_connector *udl_connector = to_udl_connector(connector);
>>
>> drm_connector_cleanup(connector);
>> - kfree(udl_connector->edid);
>> kfree(udl_connector);
>> }
>>
>> static const struct drm_connector_funcs udl_connector_funcs = {
>> .reset = drm_atomic_helper_connector_reset,
>> - .detect = udl_connector_detect,
>> .fill_modes = drm_helper_probe_single_connector_modes,
>> .destroy = udl_connector_destroy,
>> .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
--
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)
More information about the dri-devel
mailing list