On Thu, 04 Apr 2024, Thomas Zimmermann <tzimmermann@xxxxxxx> 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. 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. 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@xxxxxxx> > --- > 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, -- Jani Nikula, Intel
