[PATCH v2 0/5] drm/udl: Convert to struct drm_edid

[PATCH v2 0/5] drm/udl: Convert to struct drm_edid

[Thomas Zimmermann]

Convert udl to use struct drm_edid and its helpers. Also clean up
a few things in the process.

Patch 1 fixes a bug.

Patches 2 to 4 convert the current EDID handling to struct drm_edid
and its helpers. Patch 4 also separates the helpers for .get_modes()
and .detect_ctx() from each other.

Patch 5 removes the obsolete struct udl_connector.

v2:
- drop the generic EDID probing (Jani)
- fixes

Thomas Zimmermann (5):
  drm/udl: Remove DRM_CONNECTOR_POLL_HPD
  drm/udl: Move drm_dev_{enter,exit}() into udl_get_edid_block()
  drm/udl: Clean up Makefile
  drm/udl: Untangle .get_modes() and .detect_ctx()
  drm/udl: Remove struct udl_connector

 drivers/gpu/drm/udl/Makefile      |   8 +-
 drivers/gpu/drm/udl/udl_drv.h     |  12 +--
 drivers/gpu/drm/udl/udl_edid.c    |  79 +++++++++++++++++
 drivers/gpu/drm/udl/udl_edid.h    |  15 ++++
 drivers/gpu/drm/udl/udl_modeset.c | 138 +++++++-----------------------
 5 files changed, 131 insertions(+), 121 deletions(-)
 create mode 100644 drivers/gpu/drm/udl/udl_edid.c
 create mode 100644 drivers/gpu/drm/udl/udl_edid.h

-- 
2.44.0

[PATCH v2 1/5] drm/udl: Remove DRM_CONNECTOR_POLL_HPD

[Thomas Zimmermann]

DisplayLink devices do not generate hotplug events. Remove the poll
flag DRM_CONNECTOR_POLL_HPD, as it may not be specified together with
DRM_CONNECTOR_POLL_CONNECT or DRM_CONNECTOR_POLL_DISCONNECT.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Fixes: afdfc4c6f55f ("drm/udl: Fixed problem with UDL adpater reconnection")
Cc: Robert Tarasov <tutankhamen@chromium.org>
Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: Dave Airlie <airlied@redhat.com>
Cc: Sean Paul <sean@poorly.run>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: dri-devel@lists.freedesktop.org
Cc: <stable@vger.kernel.org> # v4.15+
---
 drivers/gpu/drm/udl/udl_modeset.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
index 7702359c90c22..751da3a294c44 100644
--- a/drivers/gpu/drm/udl/udl_modeset.c
+++ b/drivers/gpu/drm/udl/udl_modeset.c
@@ -527,8 +527,7 @@ struct drm_connector *udl_connector_init(struct drm_device *dev)
 
 	drm_connector_helper_add(connector, &udl_connector_helper_funcs);
 
-	connector->polled = DRM_CONNECTOR_POLL_HPD |
-			    DRM_CONNECTOR_POLL_CONNECT |
+	connector->polled = DRM_CONNECTOR_POLL_CONNECT |
 			    DRM_CONNECTOR_POLL_DISCONNECT;
 
 	return connector;
-- 
2.44.0

Re: [PATCH v2 1/5] drm/udl: Remove DRM_CONNECTOR_POLL_HPD

[Jani Nikula]

On Wed, 10 Apr 2024, Thomas Zimmermann <tzimmermann@suse.de> wrote:
> DisplayLink devices do not generate hotplug events. Remove the poll
> flag DRM_CONNECTOR_POLL_HPD, as it may not be specified together with
> DRM_CONNECTOR_POLL_CONNECT or DRM_CONNECTOR_POLL_DISCONNECT.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Fixes: afdfc4c6f55f ("drm/udl: Fixed problem with UDL adpater reconnection")
> Cc: Robert Tarasov <tutankhamen@chromium.org>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: Dave Airlie <airlied@redhat.com>
> Cc: Sean Paul <sean@poorly.run>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: dri-devel@lists.freedesktop.org
> Cc: <stable@vger.kernel.org> # v4.15+

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

> ---
>  drivers/gpu/drm/udl/udl_modeset.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
> index 7702359c90c22..751da3a294c44 100644
> --- a/drivers/gpu/drm/udl/udl_modeset.c
> +++ b/drivers/gpu/drm/udl/udl_modeset.c
> @@ -527,8 +527,7 @@ struct drm_connector *udl_connector_init(struct drm_device *dev)
>  
>  	drm_connector_helper_add(connector, &udl_connector_helper_funcs);
>  
> -	connector->polled = DRM_CONNECTOR_POLL_HPD |
> -			    DRM_CONNECTOR_POLL_CONNECT |
> +	connector->polled = DRM_CONNECTOR_POLL_CONNECT |
>  			    DRM_CONNECTOR_POLL_DISCONNECT;
>  
>  	return connector;

-- 
Jani Nikula, Intel

[PATCH v2 2/5] drm/udl: Move drm_dev_{enter, exit}() into udl_get_edid_block()

[Thomas Zimmermann]

Protect the code in udl_get_edid_block() with drm_dev_enter() and
drm_dev_exit(), so that all callers automatically invoke it. The
function uses hardware resources, which can be hot-unplugged at
any time. The other code in udl_connector_detect() does not use the
resources of the hardware device and therefore does not require
protection.

This change will allow to use udl_get_edid_block() in various
contexts easily.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/udl/udl_modeset.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
index 751da3a294c44..3df9fc38388b4 100644
--- a/drivers/gpu/drm/udl/udl_modeset.c
+++ b/drivers/gpu/drm/udl/udl_modeset.c
@@ -434,13 +434,18 @@ static int udl_get_edid_block(void *data, u8 *buf, unsigned int block, size_t le
 	struct drm_device *dev = &udl->drm;
 	struct usb_device *udev = udl_to_usb_device(udl);
 	u8 *read_buff;
-	int ret;
+	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;
 
@@ -449,20 +454,23 @@ static int udl_get_edid_block(void *data, u8 *buf, unsigned int block, size_t le
 				      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_kfree;
+			goto err_drm_dev_exit;
 		} else if (ret < 1) {
 			ret = -EIO;
 			drm_err(dev, "Read EDID byte %zu failed\n", i);
-			goto err_kfree;
+			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;
@@ -474,21 +482,15 @@ static enum drm_connector_status udl_connector_detect(struct drm_connector *conn
 	struct udl_device *udl = to_udl(dev);
 	struct udl_connector *udl_connector = to_udl_connector(connector);
 	enum drm_connector_status status = connector_status_disconnected;
-	int idx;
 
 	/* cleanup previous EDID */
 	kfree(udl_connector->edid);
 	udl_connector->edid = NULL;
 
-	if (!drm_dev_enter(dev, &idx))
-		return connector_status_disconnected;
-
 	udl_connector->edid = drm_do_get_edid(connector, udl_get_edid_block, udl);
 	if (udl_connector->edid)
 		status = connector_status_connected;
 
-	drm_dev_exit(idx);
-
 	return status;
 }
 
-- 
2.44.0

Re: [PATCH v2 2/5] drm/udl: Move drm_dev_{enter,exit}() into udl_get_edid_block()

[Jani Nikula]

On Wed, 10 Apr 2024, Thomas Zimmermann <tzimmermann@suse.de> wrote:
> Protect the code in udl_get_edid_block() with drm_dev_enter() and
> drm_dev_exit(), so that all callers automatically invoke it. The
> function uses hardware resources, which can be hot-unplugged at
> any time. The other code in udl_connector_detect() does not use the
> resources of the hardware device and therefore does not require
> protection.
>
> This change will allow to use udl_get_edid_block() in various
> contexts easily.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

> ---
>  drivers/gpu/drm/udl/udl_modeset.c | 20 +++++++++++---------
>  1 file changed, 11 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
> index 751da3a294c44..3df9fc38388b4 100644
> --- a/drivers/gpu/drm/udl/udl_modeset.c
> +++ b/drivers/gpu/drm/udl/udl_modeset.c
> @@ -434,13 +434,18 @@ static int udl_get_edid_block(void *data, u8 *buf, unsigned int block, size_t le
>  	struct drm_device *dev = &udl->drm;
>  	struct usb_device *udev = udl_to_usb_device(udl);
>  	u8 *read_buff;
> -	int ret;
> +	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;
>  
> @@ -449,20 +454,23 @@ static int udl_get_edid_block(void *data, u8 *buf, unsigned int block, size_t le
>  				      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_kfree;
> +			goto err_drm_dev_exit;
>  		} else if (ret < 1) {
>  			ret = -EIO;
>  			drm_err(dev, "Read EDID byte %zu failed\n", i);
> -			goto err_kfree;
> +			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;
> @@ -474,21 +482,15 @@ static enum drm_connector_status udl_connector_detect(struct drm_connector *conn
>  	struct udl_device *udl = to_udl(dev);
>  	struct udl_connector *udl_connector = to_udl_connector(connector);
>  	enum drm_connector_status status = connector_status_disconnected;
> -	int idx;
>  
>  	/* cleanup previous EDID */
>  	kfree(udl_connector->edid);
>  	udl_connector->edid = NULL;
>  
> -	if (!drm_dev_enter(dev, &idx))
> -		return connector_status_disconnected;
> -
>  	udl_connector->edid = drm_do_get_edid(connector, udl_get_edid_block, udl);
>  	if (udl_connector->edid)
>  		status = connector_status_connected;
>  
> -	drm_dev_exit(idx);
> -
>  	return status;
>  }

-- 
Jani Nikula, Intel

[PATCH v2 3/5] drm/udl: Clean up Makefile

[Thomas Zimmermann]

Clean up Makefile before listing new object files. No functional
changes.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/udl/Makefile | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/udl/Makefile b/drivers/gpu/drm/udl/Makefile
index 3f6db179455d1..00690741db376 100644
--- a/drivers/gpu/drm/udl/Makefile
+++ b/drivers/gpu/drm/udl/Makefile
@@ -1,4 +1,9 @@
 # SPDX-License-Identifier: GPL-2.0-only
-udl-y := udl_drv.o udl_modeset.o udl_main.o udl_transfer.o
+
+udl-y := \
+	udl_drv.o \
+	udl_main.o \
+	udl_modeset.o \
+	udl_transfer.o
 
 obj-$(CONFIG_DRM_UDL) := udl.o
-- 
2.44.0

Re: [PATCH v2 3/5] drm/udl: Clean up Makefile

[Jani Nikula]

On Wed, 10 Apr 2024, Thomas Zimmermann <tzimmermann@suse.de> wrote:
> Clean up Makefile before listing new object files. No functional
> changes.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

> ---
>  drivers/gpu/drm/udl/Makefile | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/udl/Makefile b/drivers/gpu/drm/udl/Makefile
> index 3f6db179455d1..00690741db376 100644
> --- a/drivers/gpu/drm/udl/Makefile
> +++ b/drivers/gpu/drm/udl/Makefile
> @@ -1,4 +1,9 @@
>  # SPDX-License-Identifier: GPL-2.0-only
> -udl-y := udl_drv.o udl_modeset.o udl_main.o udl_transfer.o
> +
> +udl-y := \
> +	udl_drv.o \
> +	udl_main.o \
> +	udl_modeset.o \
> +	udl_transfer.o
>  
>  obj-$(CONFIG_DRM_UDL) := udl.o

-- 
Jani Nikula, Intel

[PATCH v2 4/5] drm/udl: Untangle .get_modes() and .detect_ctx()

[Thomas Zimmermann]

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. But this
relied on the DRM helpers to call the functions in the correct order.
When no EDID could be retrieved, .detect() regularly printed a warning
to the kernel log.

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. 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(). The latter helper
is 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.

v2:
- implement udl_probe_edid() within udl
- reword commit description

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/udl/Makefile      |  1 +
 drivers/gpu/drm/udl/udl_drv.h     |  2 -
 drivers/gpu/drm/udl/udl_edid.c    | 79 +++++++++++++++++++++++++++
 drivers/gpu/drm/udl/udl_edid.h    | 15 ++++++
 drivers/gpu/drm/udl/udl_modeset.c | 90 +++++++------------------------
 5 files changed, 114 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..626f1badea90a
--- /dev/null
+++ b/drivers/gpu/drm/udl/udl_edid.c
@@ -0,0 +1,79 @@
+// 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)
+{
+	static const u8 no_edid[8] = {0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00};
+	u8 hdr[8];
+	int ret;
+
+	ret = udl_read_edid_block(udl, hdr, 0, sizeof(hdr));
+	if (ret)
+		return false;
+
+	/*
+	 * The adapter sends all-zeros if no monitor has been
+	 * connected. We consider anything else a connection.
+	 */
+	return memcmp(no_edid, hdr, 8) != 0;
+}
+
+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,
-- 
2.44.0

Re: [PATCH v2 4/5] drm/udl: Untangle .get_modes() and .detect_ctx()

[Jani Nikula]

On Wed, 10 Apr 2024, Thomas Zimmermann <tzimmermann@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. But this
> relied on the DRM helpers to call the functions in the correct order.
> When no EDID could be retrieved, .detect() regularly printed a warning
> to the kernel log.
>
> 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. 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(). The latter helper
> is 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.
>
> v2:
> - implement udl_probe_edid() within udl
> - reword commit description
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/udl/Makefile      |  1 +
>  drivers/gpu/drm/udl/udl_drv.h     |  2 -
>  drivers/gpu/drm/udl/udl_edid.c    | 79 +++++++++++++++++++++++++++
>  drivers/gpu/drm/udl/udl_edid.h    | 15 ++++++
>  drivers/gpu/drm/udl/udl_modeset.c | 90 +++++++------------------------
>  5 files changed, 114 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..626f1badea90a
> --- /dev/null
> +++ b/drivers/gpu/drm/udl/udl_edid.c
> @@ -0,0 +1,79 @@
> +// 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)
> +{
> +	static const u8 no_edid[8] = {0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00};
> +	u8 hdr[8];
> +	int ret;
> +
> +	ret = udl_read_edid_block(udl, hdr, 0, sizeof(hdr));
> +	if (ret)
> +		return false;
> +
> +	/*
> +	 * The adapter sends all-zeros if no monitor has been
> +	 * connected. We consider anything else a connection.
> +	 */
> +	return memcmp(no_edid, hdr, 8) != 0;

Nitpick, this works, but you can drop the no_edid buf by using:

	return memchr_inv(hdr, 0, sizeof(hdr));

Up to you,

Reviewed-by: Jani Nikula <jani.nikula@intel.com>


> +}
> +
> +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

Re: [PATCH v2 4/5] drm/udl: Untangle .get_modes() and .detect_ctx()

[Thomas Zimmermann]

Hi

Am 10.05.24 um 14:17 schrieb Jani Nikula:

>> +	/*
>> +	 * The adapter sends all-zeros if no monitor has been
>> +	 * connected. We consider anything else a connection.
>> +	 */
>> +	return memcmp(no_edid, hdr, 8) != 0;
> Nitpick, this works, but you can drop the no_edid buf by using:
>
> 	return memchr_inv(hdr, 0, sizeof(hdr));

Makes sense to me. Thanks for reviewing.

Best regards
Thomas

>
> Up to you,
>
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
>
>
>> +}
>> +
>> +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)

Re: [PATCH v2 4/5] drm/udl: Untangle .get_modes() and .detect_ctx()

[Jani Nikula]

On Fri, 10 May 2024, Thomas Zimmermann <tzimmermann@suse.de> wrote:
> Am 10.05.24 um 14:17 schrieb Jani Nikula:
>>> +	/*
>>> +	 * The adapter sends all-zeros if no monitor has been
>>> +	 * connected. We consider anything else a connection.
>>> +	 */
>>> +	return memcmp(no_edid, hdr, 8) != 0;
>> Nitpick, this works, but you can drop the no_edid buf by using:
>>
>> 	return memchr_inv(hdr, 0, sizeof(hdr));
>
> Makes sense to me. Thanks for reviewing.

Btw I wrote [1] because !memchr_inv() is a bit obscure.

BR,
Jani.


[1] https://lore.kernel.org/r/20240527094320.2653177-1-jani.nikula@intel.com


-- 
Jani Nikula, Intel

[PATCH v2 5/5] drm/udl: Remove struct udl_connector

[Thomas Zimmermann]

Udl's struct udl_connector is an empty wrapper around struct
drm_connector. Remove it. Allocate the connector as part of struct
udl_device and inline the init function into its only caller.

v2:
- fix return value in udl_modeset_init() (Dan)

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/udl/udl_drv.h     | 10 +------
 drivers/gpu/drm/udl/udl_modeset.c | 49 +++++++------------------------
 2 files changed, 11 insertions(+), 48 deletions(-)

diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
index f112cfb270f31..1eb716d9dad57 100644
--- a/drivers/gpu/drm/udl/udl_drv.h
+++ b/drivers/gpu/drm/udl/udl_drv.h
@@ -49,15 +49,6 @@ struct urb_list {
 	size_t size;
 };
 
-struct udl_connector {
-	struct drm_connector connector;
-};
-
-static inline struct udl_connector *to_udl_connector(struct drm_connector *connector)
-{
-	return container_of(connector, struct udl_connector, connector);
-}
-
 struct udl_device {
 	struct drm_device drm;
 	struct device *dev;
@@ -66,6 +57,7 @@ struct udl_device {
 	struct drm_plane primary_plane;
 	struct drm_crtc crtc;
 	struct drm_encoder encoder;
+	struct drm_connector connector;
 
 	struct mutex gem_lock;
 
diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
index 4236ce57f5945..bbb04f98886a2 100644
--- a/drivers/gpu/drm/udl/udl_modeset.c
+++ b/drivers/gpu/drm/udl/udl_modeset.c
@@ -444,49 +444,14 @@ static const struct drm_connector_helper_funcs udl_connector_helper_funcs = {
 	.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);
-}
-
 static const struct drm_connector_funcs udl_connector_funcs = {
 	.reset = drm_atomic_helper_connector_reset,
 	.fill_modes = drm_helper_probe_single_connector_modes,
-	.destroy = udl_connector_destroy,
+	.destroy = drm_connector_cleanup,
 	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
 	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
 };
 
-struct drm_connector *udl_connector_init(struct drm_device *dev)
-{
-	struct udl_connector *udl_connector;
-	struct drm_connector *connector;
-	int ret;
-
-	udl_connector = kzalloc(sizeof(*udl_connector), GFP_KERNEL);
-	if (!udl_connector)
-		return ERR_PTR(-ENOMEM);
-
-	connector = &udl_connector->connector;
-	ret = drm_connector_init(dev, connector, &udl_connector_funcs, DRM_MODE_CONNECTOR_VGA);
-	if (ret)
-		goto err_kfree;
-
-	drm_connector_helper_add(connector, &udl_connector_helper_funcs);
-
-	connector->polled = DRM_CONNECTOR_POLL_CONNECT |
-			    DRM_CONNECTOR_POLL_DISCONNECT;
-
-	return connector;
-
-err_kfree:
-	kfree(udl_connector);
-	return ERR_PTR(ret);
-}
-
 /*
  * Modesetting
  */
@@ -556,9 +521,15 @@ int udl_modeset_init(struct drm_device *dev)
 		return ret;
 	encoder->possible_crtcs = drm_crtc_mask(crtc);
 
-	connector = udl_connector_init(dev);
-	if (IS_ERR(connector))
-		return PTR_ERR(connector);
+	connector = &udl->connector;
+	ret = drm_connector_init(dev, connector, &udl_connector_funcs, DRM_MODE_CONNECTOR_VGA);
+	if (ret)
+		return ret;
+	drm_connector_helper_add(connector, &udl_connector_helper_funcs);
+
+	connector->polled = DRM_CONNECTOR_POLL_CONNECT |
+			    DRM_CONNECTOR_POLL_DISCONNECT;
+
 	ret = drm_connector_attach_encoder(connector, encoder);
 	if (ret)
 		return ret;
-- 
2.44.0

Re: [PATCH v2 5/5] drm/udl: Remove struct udl_connector

[Jani Nikula]

On Wed, 10 Apr 2024, Thomas Zimmermann <tzimmermann@suse.de> wrote:
> Udl's struct udl_connector is an empty wrapper around struct
> drm_connector. Remove it. Allocate the connector as part of struct
> udl_device and inline the init function into its only caller.
>
> v2:
> - fix return value in udl_modeset_init() (Dan)
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>

The patch does what it says on the box, but I don't know the driver
enough to say if this is the reasonable thing to do. With that,

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

> ---
>  drivers/gpu/drm/udl/udl_drv.h     | 10 +------
>  drivers/gpu/drm/udl/udl_modeset.c | 49 +++++++------------------------
>  2 files changed, 11 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
> index f112cfb270f31..1eb716d9dad57 100644
> --- a/drivers/gpu/drm/udl/udl_drv.h
> +++ b/drivers/gpu/drm/udl/udl_drv.h
> @@ -49,15 +49,6 @@ struct urb_list {
>  	size_t size;
>  };
>  
> -struct udl_connector {
> -	struct drm_connector connector;
> -};
> -
> -static inline struct udl_connector *to_udl_connector(struct drm_connector *connector)
> -{
> -	return container_of(connector, struct udl_connector, connector);
> -}
> -
>  struct udl_device {
>  	struct drm_device drm;
>  	struct device *dev;
> @@ -66,6 +57,7 @@ struct udl_device {
>  	struct drm_plane primary_plane;
>  	struct drm_crtc crtc;
>  	struct drm_encoder encoder;
> +	struct drm_connector connector;
>  
>  	struct mutex gem_lock;
>  
> diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
> index 4236ce57f5945..bbb04f98886a2 100644
> --- a/drivers/gpu/drm/udl/udl_modeset.c
> +++ b/drivers/gpu/drm/udl/udl_modeset.c
> @@ -444,49 +444,14 @@ static const struct drm_connector_helper_funcs udl_connector_helper_funcs = {
>  	.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);
> -}
> -
>  static const struct drm_connector_funcs udl_connector_funcs = {
>  	.reset = drm_atomic_helper_connector_reset,
>  	.fill_modes = drm_helper_probe_single_connector_modes,
> -	.destroy = udl_connector_destroy,
> +	.destroy = drm_connector_cleanup,
>  	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
>  	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
>  };
>  
> -struct drm_connector *udl_connector_init(struct drm_device *dev)
> -{
> -	struct udl_connector *udl_connector;
> -	struct drm_connector *connector;
> -	int ret;
> -
> -	udl_connector = kzalloc(sizeof(*udl_connector), GFP_KERNEL);
> -	if (!udl_connector)
> -		return ERR_PTR(-ENOMEM);
> -
> -	connector = &udl_connector->connector;
> -	ret = drm_connector_init(dev, connector, &udl_connector_funcs, DRM_MODE_CONNECTOR_VGA);
> -	if (ret)
> -		goto err_kfree;
> -
> -	drm_connector_helper_add(connector, &udl_connector_helper_funcs);
> -
> -	connector->polled = DRM_CONNECTOR_POLL_CONNECT |
> -			    DRM_CONNECTOR_POLL_DISCONNECT;
> -
> -	return connector;
> -
> -err_kfree:
> -	kfree(udl_connector);
> -	return ERR_PTR(ret);
> -}
> -
>  /*
>   * Modesetting
>   */
> @@ -556,9 +521,15 @@ int udl_modeset_init(struct drm_device *dev)
>  		return ret;
>  	encoder->possible_crtcs = drm_crtc_mask(crtc);
>  
> -	connector = udl_connector_init(dev);
> -	if (IS_ERR(connector))
> -		return PTR_ERR(connector);
> +	connector = &udl->connector;
> +	ret = drm_connector_init(dev, connector, &udl_connector_funcs, DRM_MODE_CONNECTOR_VGA);
> +	if (ret)
> +		return ret;
> +	drm_connector_helper_add(connector, &udl_connector_helper_funcs);
> +
> +	connector->polled = DRM_CONNECTOR_POLL_CONNECT |
> +			    DRM_CONNECTOR_POLL_DISCONNECT;
> +
>  	ret = drm_connector_attach_encoder(connector, encoder);
>  	if (ret)
>  		return ret;

-- 
Jani Nikula, Intel