[PATCH 00/16] drm/udl: Better modesetting, hot-unplug, protocol

[PATCH 00/16] drm/udl: Better modesetting, hot-unplug, protocol

[Thomas Zimmermann]

This patchset reworks the udl driver's modesetting code.

Patches #1 to #5 improve the connector code with various updates.

Patches #6 to #10 improve the modesetting code. Patch #7 replaces the
simple-KMS helpers with the regular atomic helpers. Patch #9 adds DRM
hot-unplugging. The driver had some unplugging support via USB functions,
the DRM side was probably not prepared yet. Patch #10 changes damage
updates to the damage iterator. This minimizes the amount of data to
send over USB.

Patches #11 to #16 add protocol constants for the various UDL display
operations.

Tested with X11, console and Weston.

Thomas Zimmermann (16):
  drm/udl: Rename struct udl_drm_connector to struct udl_connector
  drm/udl: Test pixel limit in mode-config's mode-valid function
  drm/udl: Use USB timeout constant when reading EDID
  drm/udl: Various improvements to the connector
  drm/udl: Move connector to modesetting code
  drm/udl: Remove udl_simple_display_pipe_mode_valid()
  drm/udl: Convert to atomic-modesetting helpers
  drm/udl: Simplify modesetting in CRTC's enable function
  drm/udl: Support DRM hot-unplugging
  drm/udl: Use damage iterator
  drm/udl: Move register constants to udl_proto.h
  drm/udl: Add constants for display-mode registers
  drm/udl: Add register constants for color depth
  drm/udl: Add register constants for video locks
  drm/udl: Add register constants for framebuffer scanout addresses
  drm/udl: Add constants for commands

 drivers/gpu/drm/udl/Makefile        |   2 +-
 drivers/gpu/drm/udl/udl_connector.c | 139 -------
 drivers/gpu/drm/udl/udl_connector.h |  15 -
 drivers/gpu/drm/udl/udl_drv.h       |  42 +--
 drivers/gpu/drm/udl/udl_modeset.c   | 567 ++++++++++++++++++----------
 drivers/gpu/drm/udl/udl_proto.h     |  62 +++
 drivers/gpu/drm/udl/udl_transfer.c  |   7 +-
 7 files changed, 450 insertions(+), 384 deletions(-)
 delete mode 100644 drivers/gpu/drm/udl/udl_connector.c
 delete mode 100644 drivers/gpu/drm/udl/udl_connector.h
 create mode 100644 drivers/gpu/drm/udl/udl_proto.h


base-commit: d8deedaa0fcd8192715a052a0239bee3f74a8fb1
-- 
2.37.3

[PATCH 01/16] drm/udl: Rename struct udl_drm_connector to struct udl_connector

[Thomas Zimmermann]

Remove the _drm_ infix from struct udl_drm_connector and introduce a
macro for upcasting from struct drm_connector. No functional changes.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/udl/udl_connector.c | 19 +++++--------------
 drivers/gpu/drm/udl/udl_connector.h | 10 ++++++++--
 2 files changed, 13 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/udl/udl_connector.c b/drivers/gpu/drm/udl/udl_connector.c
index fade4c7adbf7..3c8068626384 100644
--- a/drivers/gpu/drm/udl/udl_connector.c
+++ b/drivers/gpu/drm/udl/udl_connector.c
@@ -46,10 +46,7 @@ static int udl_get_edid_block(void *data, u8 *buf, unsigned int block,
 
 static int udl_get_modes(struct drm_connector *connector)
 {
-	struct udl_drm_connector *udl_connector =
-					container_of(connector,
-					struct udl_drm_connector,
-					connector);
+	struct udl_connector *udl_connector = to_udl_connector(connector);
 
 	drm_connector_update_edid_property(connector, udl_connector->edid);
 	if (udl_connector->edid)
@@ -74,10 +71,7 @@ static enum drm_connector_status
 udl_detect(struct drm_connector *connector, bool force)
 {
 	struct udl_device *udl = to_udl(connector->dev);
-	struct udl_drm_connector *udl_connector =
-					container_of(connector,
-					struct udl_drm_connector,
-					connector);
+	struct udl_connector *udl_connector = to_udl_connector(connector);
 
 	/* cleanup previous edid */
 	if (udl_connector->edid != NULL) {
@@ -94,10 +88,7 @@ udl_detect(struct drm_connector *connector, bool force)
 
 static void udl_connector_destroy(struct drm_connector *connector)
 {
-	struct udl_drm_connector *udl_connector =
-					container_of(connector,
-					struct udl_drm_connector,
-					connector);
+	struct udl_connector *udl_connector = to_udl_connector(connector);
 
 	drm_connector_cleanup(connector);
 	kfree(udl_connector->edid);
@@ -120,10 +111,10 @@ static const struct drm_connector_funcs udl_connector_funcs = {
 
 struct drm_connector *udl_connector_init(struct drm_device *dev)
 {
-	struct udl_drm_connector *udl_connector;
+	struct udl_connector *udl_connector;
 	struct drm_connector *connector;
 
-	udl_connector = kzalloc(sizeof(struct udl_drm_connector), GFP_KERNEL);
+	udl_connector = kzalloc(sizeof(*udl_connector), GFP_KERNEL);
 	if (!udl_connector)
 		return ERR_PTR(-ENOMEM);
 
diff --git a/drivers/gpu/drm/udl/udl_connector.h b/drivers/gpu/drm/udl/udl_connector.h
index 7f2d392df173..74ad68fd3cc9 100644
--- a/drivers/gpu/drm/udl/udl_connector.h
+++ b/drivers/gpu/drm/udl/udl_connector.h
@@ -1,15 +1,21 @@
 #ifndef __UDL_CONNECTOR_H__
 #define __UDL_CONNECTOR_H__
 
-#include <drm/drm_crtc.h>
+#include <linux/container_of.h>
+
+#include <drm/drm_connector.h>
 
 struct edid;
 
-struct udl_drm_connector {
+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)
+{
+	return container_of(connector, struct udl_connector, connector);
+}
 
 #endif //__UDL_CONNECTOR_H__
-- 
2.37.3

Re: [PATCH 01/16] drm/udl: Rename struct udl_drm_connector to struct udl_connector

[Javier Martinez Canillas]

Hello Thomas,

On 9/19/22 15:03, Thomas Zimmermann wrote:
> Remove the _drm_ infix from struct udl_drm_connector and introduce a
> macro for upcasting from struct drm_connector. No functional changes.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

[PATCH 02/16] drm/udl: Test pixel limit in mode-config's mode-valid function

[Thomas Zimmermann]

The sku_pixel_limit is a per-device property, similar to the amount
of available video memory. Move the respective mode-valid test from
the connector to the mode-config structure.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/udl/udl_connector.c | 14 --------------
 drivers/gpu/drm/udl/udl_modeset.c   | 14 ++++++++++++++
 2 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/udl/udl_connector.c b/drivers/gpu/drm/udl/udl_connector.c
index 3c8068626384..e9539829032c 100644
--- a/drivers/gpu/drm/udl/udl_connector.c
+++ b/drivers/gpu/drm/udl/udl_connector.c
@@ -54,19 +54,6 @@ static int udl_get_modes(struct drm_connector *connector)
 	return 0;
 }
 
-static enum drm_mode_status udl_mode_valid(struct drm_connector *connector,
-			  struct drm_display_mode *mode)
-{
-	struct udl_device *udl = to_udl(connector->dev);
-	if (!udl->sku_pixel_limit)
-		return 0;
-
-	if (mode->vdisplay * mode->hdisplay > udl->sku_pixel_limit)
-		return MODE_VIRTUAL_Y;
-
-	return 0;
-}
-
 static enum drm_connector_status
 udl_detect(struct drm_connector *connector, bool force)
 {
@@ -97,7 +84,6 @@ static void udl_connector_destroy(struct drm_connector *connector)
 
 static const struct drm_connector_helper_funcs udl_connector_helper_funcs = {
 	.get_modes = udl_get_modes,
-	.mode_valid = udl_mode_valid,
 };
 
 static const struct drm_connector_funcs udl_connector_funcs = {
diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
index ec6876f449f3..c7adc29a53a1 100644
--- a/drivers/gpu/drm/udl/udl_modeset.c
+++ b/drivers/gpu/drm/udl/udl_modeset.c
@@ -407,8 +407,22 @@ static const struct drm_simple_display_pipe_funcs udl_simple_display_pipe_funcs
  * Modesetting
  */
 
+static enum drm_mode_status udl_mode_config_mode_valid(struct drm_device *dev,
+						       const struct drm_display_mode *mode)
+{
+	struct udl_device *udl = to_udl(dev);
+
+	if (udl->sku_pixel_limit) {
+		if (mode->vdisplay * mode->hdisplay > udl->sku_pixel_limit)
+			return MODE_MEM;
+	}
+
+	return MODE_OK;
+}
+
 static const struct drm_mode_config_funcs udl_mode_funcs = {
 	.fb_create = drm_gem_fb_create_with_dirty,
+	.mode_valid = udl_mode_config_mode_valid,
 	.atomic_check  = drm_atomic_helper_check,
 	.atomic_commit = drm_atomic_helper_commit,
 };
-- 
2.37.3

Re: [PATCH 02/16] drm/udl: Test pixel limit in mode-config's mode-valid function

[Javier Martinez Canillas]

On 9/19/22 15:03, Thomas Zimmermann wrote:
> The sku_pixel_limit is a per-device property, similar to the amount
> of available video memory. Move the respective mode-valid test from
> the connector to the mode-config structure.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---

[...]

> +static enum drm_mode_status udl_mode_config_mode_valid(struct drm_device *dev,
> +						       const struct drm_display_mode *mode)
> +{
> +	struct udl_device *udl = to_udl(dev);
> +
> +	if (udl->sku_pixel_limit) {
> +		if (mode->vdisplay * mode->hdisplay > udl->sku_pixel_limit)
> +			return MODE_MEM;
> +	}
> +
> +	return MODE_OK;
> +}
> +
>  static const struct drm_mode_config_funcs udl_mode_funcs = {
>  	.fb_create = drm_gem_fb_create_with_dirty,
> +	.mode_valid = udl_mode_config_mode_valid,
>  	.atomic_check  = drm_atomic_helper_check,
>  	.atomic_commit = drm_atomic_helper_commit,
>  };

It's always confusing to me whether something has to be in the .mode_valid
for drm_mode_config helper function or for the drm_crtc_helper_funcs. This
driver is still using the simple-KMS at this point so that will be in the
udl_simple_display_pipe_mode_valid() if should be the latter.

In this case since it seems to be about a pixel limit, it might make sense
to have this constraint for the DRM mode config. But since it depends on the
{h,v}display, I thought that needed to ask if instead should be for the CRTC.

Any in case,

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 02/16] drm/udl: Test pixel limit in mode-config's mode-valid function

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 2194 bytes --]

Hi

Am 29.09.22 um 15:20 schrieb Javier Martinez Canillas:
> On 9/19/22 15:03, Thomas Zimmermann wrote:
>> The sku_pixel_limit is a per-device property, similar to the amount
>> of available video memory. Move the respective mode-valid test from
>> the connector to the mode-config structure.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
> 
> [...]
> 
>> +static enum drm_mode_status udl_mode_config_mode_valid(struct drm_device *dev,
>> +						       const struct drm_display_mode *mode)
>> +{
>> +	struct udl_device *udl = to_udl(dev);
>> +
>> +	if (udl->sku_pixel_limit) {
>> +		if (mode->vdisplay * mode->hdisplay > udl->sku_pixel_limit)
>> +			return MODE_MEM;
>> +	}
>> +
>> +	return MODE_OK;
>> +}
>> +
>>   static const struct drm_mode_config_funcs udl_mode_funcs = {
>>   	.fb_create = drm_gem_fb_create_with_dirty,
>> +	.mode_valid = udl_mode_config_mode_valid,
>>   	.atomic_check  = drm_atomic_helper_check,
>>   	.atomic_commit = drm_atomic_helper_commit,
>>   };
> 
> It's always confusing to me whether something has to be in the .mode_valid
> for drm_mode_config helper function or for the drm_crtc_helper_funcs. This
> driver is still using the simple-KMS at this point so that will be in the
> udl_simple_display_pipe_mode_valid() if should be the latter.
> 
> In this case since it seems to be about a pixel limit, it might make sense
> to have this constraint for the DRM mode config. But since it depends on the
> {h,v}display, I thought that needed to ask if instead should be for the CRTC.

We're testing modes against limitations of the driver or hardware. The 
rule of thumb is to use the mode_valid function of the component that 
imposes the limitation.  In the case at hand, the limit is the maximum 
number of pixels that the adapter can store. So it goes into the 
device-wide mode_valid.

Best regards
Thomas

> 
> Any in case,
> 
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

[PATCH 03/16] drm/udl: Use USB timeout constant when reading EDID

[Thomas Zimmermann]

Set the USB control-message timeout to the USB default of 5 seconds.
Done for consistency with other uses of usb_control_msg() in udl and
other drivers.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/udl/udl_connector.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/udl/udl_connector.c b/drivers/gpu/drm/udl/udl_connector.c
index e9539829032c..cb3d6820eaf9 100644
--- a/drivers/gpu/drm/udl/udl_connector.c
+++ b/drivers/gpu/drm/udl/udl_connector.c
@@ -31,7 +31,7 @@ static int udl_get_edid_block(void *data, u8 *buf, unsigned int block,
 		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, 1000);
+				      0xA1, read_buff, 2, USB_CTRL_GET_TIMEOUT);
 		if (ret < 1) {
 			DRM_ERROR("Read EDID byte %d failed err %x\n", i, ret);
 			kfree(read_buff);
-- 
2.37.3

Re: [PATCH 03/16] drm/udl: Use USB timeout constant when reading EDID

[Javier Martinez Canillas]

On 9/19/22 15:03, Thomas Zimmermann wrote:
> Set the USB control-message timeout to the USB default of 5 seconds.
> Done for consistency with other uses of usb_control_msg() in udl and
> other drivers.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/udl/udl_connector.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/udl/udl_connector.c b/drivers/gpu/drm/udl/udl_connector.c
> index e9539829032c..cb3d6820eaf9 100644
> --- a/drivers/gpu/drm/udl/udl_connector.c
> +++ b/drivers/gpu/drm/udl/udl_connector.c
> @@ -31,7 +31,7 @@ static int udl_get_edid_block(void *data, u8 *buf, unsigned int block,
>  		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, 1000);
> +				      0xA1, read_buff, 2, USB_CTRL_GET_TIMEOUT);

Agreed, much better than an arbitrary 1 sec.

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

[PATCH 04/16] drm/udl: Various improvements to the connector

[Thomas Zimmermann]

Add style fixes, better error handling and reporting, and minor
clean-up changes to the connector code before moving the code to
the rest of the modesetting pipeline.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/udl/udl_connector.c | 64 ++++++++++++++++++-----------
 1 file changed, 40 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/udl/udl_connector.c b/drivers/gpu/drm/udl/udl_connector.c
index cb3d6820eaf9..538b47ffa67f 100644
--- a/drivers/gpu/drm/udl/udl_connector.c
+++ b/drivers/gpu/drm/udl/udl_connector.c
@@ -15,56 +15,64 @@
 #include "udl_connector.h"
 #include "udl_drv.h"
 
-static int udl_get_edid_block(void *data, u8 *buf, unsigned int block,
-			       size_t len)
+static int udl_get_edid_block(void *data, u8 *buf, unsigned int block, size_t len)
 {
-	int ret, i;
-	u8 *read_buff;
 	struct udl_device *udl = data;
+	struct drm_device *dev = &udl->drm;
 	struct usb_device *udev = udl_to_usb_device(udl);
+	u8 *read_buff;
+	int ret;
+	size_t i;
 
 	read_buff = kmalloc(2, GFP_KERNEL);
 	if (!read_buff)
-		return -1;
+		return -ENOMEM;
 
 	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 < 1) {
-			DRM_ERROR("Read EDID byte %d failed err %x\n", i, ret);
-			kfree(read_buff);
-			return -1;
+		if (ret < 0) {
+			drm_err(dev, "Read EDID byte %zu failed err %x\n", i, ret);
+			goto err_kfree;
+		} else if (ret < 1) {
+			ret = -EIO;
+			drm_err(dev, "Read EDID byte %zu failed\n", i);
+			goto err_kfree;
 		}
+
 		buf[i] = read_buff[1];
 	}
 
 	kfree(read_buff);
+
 	return 0;
+
+err_kfree:
+	kfree(read_buff);
+	return ret;
 }
 
-static int udl_get_modes(struct drm_connector *connector)
+static int udl_connector_helper_get_modes(struct drm_connector *connector)
 {
 	struct udl_connector *udl_connector = to_udl_connector(connector);
 
 	drm_connector_update_edid_property(connector, udl_connector->edid);
 	if (udl_connector->edid)
 		return drm_add_edid_modes(connector, udl_connector->edid);
+
 	return 0;
 }
 
-static enum drm_connector_status
-udl_detect(struct drm_connector *connector, bool force)
+static enum drm_connector_status udl_connector_detect(struct drm_connector *connector, bool force)
 {
 	struct udl_device *udl = to_udl(connector->dev);
 	struct udl_connector *udl_connector = to_udl_connector(connector);
 
-	/* cleanup previous edid */
-	if (udl_connector->edid != NULL) {
-		kfree(udl_connector->edid);
-		udl_connector->edid = NULL;
-	}
+	/* cleanup previous EDID */
+	kfree(udl_connector->edid);
 
 	udl_connector->edid = drm_do_get_edid(connector, udl_get_edid_block, udl);
 	if (!udl_connector->edid)
@@ -79,38 +87,46 @@ static void udl_connector_destroy(struct drm_connector *connector)
 
 	drm_connector_cleanup(connector);
 	kfree(udl_connector->edid);
-	kfree(connector);
+	kfree(udl_connector);
 }
 
 static const struct drm_connector_helper_funcs udl_connector_helper_funcs = {
-	.get_modes = udl_get_modes,
+	.get_modes = udl_connector_helper_get_modes,
 };
 
 static const struct drm_connector_funcs udl_connector_funcs = {
 	.reset = drm_atomic_helper_connector_reset,
-	.detect = udl_detect,
+	.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,
-	.atomic_destroy_state   = drm_atomic_helper_connector_destroy_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;
-	drm_connector_init(dev, connector, &udl_connector_funcs,
-			   DRM_MODE_CONNECTOR_VGA);
+	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_HPD |
-		DRM_CONNECTOR_POLL_CONNECT | DRM_CONNECTOR_POLL_DISCONNECT;
+			    DRM_CONNECTOR_POLL_CONNECT |
+			    DRM_CONNECTOR_POLL_DISCONNECT;
 
 	return connector;
+
+err_kfree:
+	kfree(udl_connector);
+	return ERR_PTR(ret);
 }
-- 
2.37.3

Re: [PATCH 04/16] drm/udl: Various improvements to the connector

[Javier Martinez Canillas]

On 9/19/22 15:03, Thomas Zimmermann wrote:
> Add style fixes, better error handling and reporting, and minor
> clean-up changes to the connector code before moving the code to
> the rest of the modesetting pipeline.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

[PATCH 05/16] drm/udl: Move connector to modesetting code

[Thomas Zimmermann]

Move the connector next to the rest of the modesetting code. No
functional changes.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/udl/Makefile        |   2 +-
 drivers/gpu/drm/udl/udl_connector.c | 132 ----------------------------
 drivers/gpu/drm/udl/udl_connector.h |  21 -----
 drivers/gpu/drm/udl/udl_drv.h       |  11 +++
 drivers/gpu/drm/udl/udl_modeset.c   | 122 +++++++++++++++++++++++++
 5 files changed, 134 insertions(+), 154 deletions(-)
 delete mode 100644 drivers/gpu/drm/udl/udl_connector.c
 delete mode 100644 drivers/gpu/drm/udl/udl_connector.h

diff --git a/drivers/gpu/drm/udl/Makefile b/drivers/gpu/drm/udl/Makefile
index 24d61f61d7db..3f6db179455d 100644
--- a/drivers/gpu/drm/udl/Makefile
+++ b/drivers/gpu/drm/udl/Makefile
@@ -1,4 +1,4 @@
 # SPDX-License-Identifier: GPL-2.0-only
-udl-y := udl_drv.o udl_modeset.o udl_connector.o udl_main.o udl_transfer.o
+udl-y := udl_drv.o udl_modeset.o udl_main.o udl_transfer.o
 
 obj-$(CONFIG_DRM_UDL) := udl.o
diff --git a/drivers/gpu/drm/udl/udl_connector.c b/drivers/gpu/drm/udl/udl_connector.c
deleted file mode 100644
index 538b47ffa67f..000000000000
--- a/drivers/gpu/drm/udl/udl_connector.c
+++ /dev/null
@@ -1,132 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0-only
-/*
- * Copyright (C) 2012 Red Hat
- * based in parts on udlfb.c:
- * Copyright (C) 2009 Roberto De Ioris <roberto@unbit.it>
- * Copyright (C) 2009 Jaya Kumar <jayakumar.lkml@gmail.com>
- * Copyright (C) 2009 Bernie Thompson <bernie@plugable.com>
- */
-
-#include <drm/drm_atomic_state_helper.h>
-#include <drm/drm_edid.h>
-#include <drm/drm_crtc_helper.h>
-#include <drm/drm_probe_helper.h>
-
-#include "udl_connector.h"
-#include "udl_drv.h"
-
-static int udl_get_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 ret;
-	size_t i;
-
-	read_buff = kmalloc(2, GFP_KERNEL);
-	if (!read_buff)
-		return -ENOMEM;
-
-	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_kfree;
-		} else if (ret < 1) {
-			ret = -EIO;
-			drm_err(dev, "Read EDID byte %zu failed\n", i);
-			goto err_kfree;
-		}
-
-		buf[i] = read_buff[1];
-	}
-
-	kfree(read_buff);
-
-	return 0;
-
-err_kfree:
-	kfree(read_buff);
-	return ret;
-}
-
-static int udl_connector_helper_get_modes(struct drm_connector *connector)
-{
-	struct udl_connector *udl_connector = to_udl_connector(connector);
-
-	drm_connector_update_edid_property(connector, udl_connector->edid);
-	if (udl_connector->edid)
-		return drm_add_edid_modes(connector, udl_connector->edid);
-
-	return 0;
-}
-
-static enum drm_connector_status udl_connector_detect(struct drm_connector *connector, bool force)
-{
-	struct udl_device *udl = to_udl(connector->dev);
-	struct udl_connector *udl_connector = to_udl_connector(connector);
-
-	/* cleanup previous EDID */
-	kfree(udl_connector->edid);
-
-	udl_connector->edid = drm_do_get_edid(connector, udl_get_edid_block, udl);
-	if (!udl_connector->edid)
-		return connector_status_disconnected;
-
-	return connector_status_connected;
-}
-
-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_helper_funcs udl_connector_helper_funcs = {
-	.get_modes = udl_connector_helper_get_modes,
-};
-
-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,
-	.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_HPD |
-			    DRM_CONNECTOR_POLL_CONNECT |
-			    DRM_CONNECTOR_POLL_DISCONNECT;
-
-	return connector;
-
-err_kfree:
-	kfree(udl_connector);
-	return ERR_PTR(ret);
-}
diff --git a/drivers/gpu/drm/udl/udl_connector.h b/drivers/gpu/drm/udl/udl_connector.h
deleted file mode 100644
index 74ad68fd3cc9..000000000000
--- a/drivers/gpu/drm/udl/udl_connector.h
+++ /dev/null
@@ -1,21 +0,0 @@
-#ifndef __UDL_CONNECTOR_H__
-#define __UDL_CONNECTOR_H__
-
-#include <linux/container_of.h>
-
-#include <drm/drm_connector.h>
-
-struct edid;
-
-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)
-{
-	return container_of(connector, struct udl_connector, connector);
-}
-
-#endif //__UDL_CONNECTOR_H__
diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
index b4cc7cc568c7..d7a3d495f2e7 100644
--- a/drivers/gpu/drm/udl/udl_drv.h
+++ b/drivers/gpu/drm/udl/udl_drv.h
@@ -46,6 +46,17 @@ struct urb_list {
 	size_t size;
 };
 
+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)
+{
+	return container_of(connector, struct udl_connector, connector);
+}
+
 struct udl_device {
 	struct drm_device drm;
 	struct device *dev;
diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
index c7adc29a53a1..93e7554e83fa 100644
--- a/drivers/gpu/drm/udl/udl_modeset.c
+++ b/drivers/gpu/drm/udl/udl_modeset.c
@@ -11,11 +11,13 @@
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_crtc_helper.h>
 #include <drm/drm_damage_helper.h>
+#include <drm/drm_edid.h>
 #include <drm/drm_fourcc.h>
 #include <drm/drm_gem_atomic_helper.h>
 #include <drm/drm_gem_framebuffer_helper.h>
 #include <drm/drm_gem_shmem_helper.h>
 #include <drm/drm_modeset_helper_vtables.h>
+#include <drm/drm_probe_helper.h>
 #include <drm/drm_vblank.h>
 
 #include "udl_drv.h"
@@ -403,6 +405,126 @@ static const struct drm_simple_display_pipe_funcs udl_simple_display_pipe_funcs
 	DRM_GEM_SIMPLE_DISPLAY_PIPE_SHADOW_PLANE_FUNCS,
 };
 
+/*
+ * Connector
+ */
+
+static int udl_connector_helper_get_modes(struct drm_connector *connector)
+{
+	struct udl_connector *udl_connector = to_udl_connector(connector);
+
+	drm_connector_update_edid_property(connector, udl_connector->edid);
+	if (udl_connector->edid)
+		return drm_add_edid_modes(connector, udl_connector->edid);
+
+	return 0;
+}
+
+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)
+{
+	struct udl_device *udl = data;
+	struct drm_device *dev = &udl->drm;
+	struct usb_device *udev = udl_to_usb_device(udl);
+	u8 *read_buff;
+	int ret;
+	size_t i;
+
+	read_buff = kmalloc(2, GFP_KERNEL);
+	if (!read_buff)
+		return -ENOMEM;
+
+	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_kfree;
+		} else if (ret < 1) {
+			ret = -EIO;
+			drm_err(dev, "Read EDID byte %zu failed\n", i);
+			goto err_kfree;
+		}
+
+		buf[i] = read_buff[1];
+	}
+
+	kfree(read_buff);
+
+	return 0;
+
+err_kfree:
+	kfree(read_buff);
+	return ret;
+}
+
+static enum drm_connector_status udl_connector_detect(struct drm_connector *connector, bool force)
+{
+	struct udl_device *udl = to_udl(connector->dev);
+	struct udl_connector *udl_connector = to_udl_connector(connector);
+
+	/* cleanup previous EDID */
+	kfree(udl_connector->edid);
+
+	udl_connector->edid = drm_do_get_edid(connector, udl_get_edid_block, udl);
+	if (!udl_connector->edid)
+		return connector_status_disconnected;
+
+	return connector_status_connected;
+}
+
+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,
+	.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_HPD |
+			    DRM_CONNECTOR_POLL_CONNECT |
+			    DRM_CONNECTOR_POLL_DISCONNECT;
+
+	return connector;
+
+err_kfree:
+	kfree(udl_connector);
+	return ERR_PTR(ret);
+}
+
 /*
  * Modesetting
  */
-- 
2.37.3

Re: [PATCH 05/16] drm/udl: Move connector to modesetting code

[Javier Martinez Canillas]

On 9/19/22 15:03, Thomas Zimmermann wrote:
> Move the connector next to the rest of the modesetting code. No
> functional changes.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---

Indeed. I wondered why the connector could was special to require a
separate source code file.

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

[PATCH 06/16] drm/udl: Remove udl_simple_display_pipe_mode_valid()

[Thomas Zimmermann]

Remove the empty function udl_simple_display_pipe_mode_valid() and
let simple-KMS helpers accept the modes. No functional changes.

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

diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
index 93e7554e83fa..7f8bde89396d 100644
--- a/drivers/gpu/drm/udl/udl_modeset.c
+++ b/drivers/gpu/drm/udl/udl_modeset.c
@@ -310,13 +310,6 @@ static const uint32_t udl_simple_display_pipe_formats[] = {
 	DRM_FORMAT_XRGB8888,
 };
 
-static enum drm_mode_status
-udl_simple_display_pipe_mode_valid(struct drm_simple_display_pipe *pipe,
-				   const struct drm_display_mode *mode)
-{
-	return MODE_OK;
-}
-
 static void
 udl_simple_display_pipe_enable(struct drm_simple_display_pipe *pipe,
 			       struct drm_crtc_state *crtc_state,
@@ -398,7 +391,6 @@ udl_simple_display_pipe_update(struct drm_simple_display_pipe *pipe,
 }
 
 static const struct drm_simple_display_pipe_funcs udl_simple_display_pipe_funcs = {
-	.mode_valid = udl_simple_display_pipe_mode_valid,
 	.enable = udl_simple_display_pipe_enable,
 	.disable = udl_simple_display_pipe_disable,
 	.update = udl_simple_display_pipe_update,
-- 
2.37.3

Re: [PATCH 06/16] drm/udl: Remove udl_simple_display_pipe_mode_valid()

[Javier Martinez Canillas]

On 9/19/22 15:03, Thomas Zimmermann wrote:
> Remove the empty function udl_simple_display_pipe_mode_valid() and
> let simple-KMS helpers accept the modes. No functional changes.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

[PATCH 07/16] drm/udl: Convert to atomic-modesetting helpers

[Thomas Zimmermann]

Replace simple-KMS helpers with regular atomic-modesetting helpers.
The simple-KMS helpers introduce a mid-layer abstraction without
added functionality. Using regular atomic helpers makes the driver's
implementation more discoverable and simplifies code sharing.

The conversion effectively open-codes the simple-KMS functions and
data structure within udl. No functional changes.

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

diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
index d7a3d495f2e7..e96166ce2919 100644
--- a/drivers/gpu/drm/udl/udl_drv.h
+++ b/drivers/gpu/drm/udl/udl_drv.h
@@ -14,10 +14,13 @@
 #include <linux/mm_types.h>
 #include <linux/usb.h>
 
+#include <drm/drm_connector.h>
+#include <drm/drm_crtc.h>
 #include <drm/drm_device.h>
+#include <drm/drm_encoder.h>
 #include <drm/drm_framebuffer.h>
 #include <drm/drm_gem.h>
-#include <drm/drm_simple_kms_helper.h>
+#include <drm/drm_plane.h>
 
 struct drm_mode_create_dumb;
 
@@ -62,7 +65,9 @@ struct udl_device {
 	struct device *dev;
 	struct device *dmadev;
 
-	struct drm_simple_display_pipe display_pipe;
+	struct drm_plane primary_plane;
+	struct drm_crtc crtc;
+	struct drm_encoder encoder;
 
 	struct mutex gem_lock;
 
diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
index 7f8bde89396d..f83d0feff434 100644
--- a/drivers/gpu/drm/udl/udl_modeset.c
+++ b/drivers/gpu/drm/udl/udl_modeset.c
@@ -8,6 +8,7 @@
  * Copyright (C) 2009 Bernie Thompson <bernie@plugable.com>
  */
 
+#include <drm/drm_atomic.h>
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_crtc_helper.h>
 #include <drm/drm_damage_helper.h>
@@ -17,6 +18,7 @@
 #include <drm/drm_gem_framebuffer_helper.h>
 #include <drm/drm_gem_shmem_helper.h>
 #include <drm/drm_modeset_helper_vtables.h>
+#include <drm/drm_plane_helper.h>
 #include <drm/drm_probe_helper.h>
 #include <drm/drm_vblank.h>
 
@@ -302,61 +304,105 @@ static int udl_handle_damage(struct drm_framebuffer *fb,
 }
 
 /*
- * Simple display pipeline
+ * Primary plane
  */
 
-static const uint32_t udl_simple_display_pipe_formats[] = {
+static const uint32_t udl_primary_plane_formats[] = {
 	DRM_FORMAT_RGB565,
 	DRM_FORMAT_XRGB8888,
 };
 
-static void
-udl_simple_display_pipe_enable(struct drm_simple_display_pipe *pipe,
-			       struct drm_crtc_state *crtc_state,
-			       struct drm_plane_state *plane_state)
+static const uint64_t udl_primary_plane_fmtmods[] = {
+	DRM_FORMAT_MOD_LINEAR,
+	DRM_FORMAT_MOD_INVALID
+};
+
+static void udl_primary_plane_helper_atomic_update(struct drm_plane *plane,
+						   struct drm_atomic_state *state)
 {
-	struct drm_crtc *crtc = &pipe->crtc;
-	struct drm_device *dev = crtc->dev;
+	struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, plane);
+	struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(plane_state);
 	struct drm_framebuffer *fb = plane_state->fb;
+	struct drm_plane_state *old_plane_state = drm_atomic_get_old_plane_state(state, plane);
+	struct drm_rect rect;
+
+	if (drm_atomic_helper_damage_merged(old_plane_state, plane_state, &rect))
+		udl_handle_damage(fb, &shadow_plane_state->data[0], &rect);
+}
+
+static void udl_primary_plane_helper_atomic_disable(struct drm_plane *plane,
+						    struct drm_atomic_state *state)
+{
+	/*
+	 * Keep this function here to avoid calling atomic_update when
+	 * the plane is being disabled.
+	 *
+	 * TODO: Maybe clear the adapter's display buffer from here.
+	 */
+}
+
+static const struct drm_plane_helper_funcs udl_primary_plane_helper_funcs = {
+	DRM_GEM_SHADOW_PLANE_HELPER_FUNCS,
+	.atomic_check = drm_plane_helper_atomic_check,
+	.atomic_update = udl_primary_plane_helper_atomic_update,
+	.atomic_disable = udl_primary_plane_helper_atomic_disable,
+};
+
+static const struct drm_plane_funcs udl_primary_plane_funcs = {
+	.update_plane = drm_atomic_helper_update_plane,
+	.disable_plane = drm_atomic_helper_disable_plane,
+	.destroy = drm_plane_cleanup,
+	DRM_GEM_SHADOW_PLANE_FUNCS,
+};
+
+/*
+ * CRTC
+ */
+
+static int udl_crtc_helper_atomic_check(struct drm_crtc *crtc, struct drm_atomic_state *state)
+{
+	struct drm_crtc_state *new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
+
+	return drm_atomic_helper_check_crtc_state(new_crtc_state, false);
+}
+
+static void udl_crtc_helper_atomic_enable(struct drm_crtc *crtc, struct drm_atomic_state *state)
+{
+	struct drm_device *dev = crtc->dev;
 	struct udl_device *udl = to_udl(dev);
+	struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
 	struct drm_display_mode *mode = &crtc_state->mode;
-	struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(plane_state);
-	struct drm_rect clip = DRM_RECT_INIT(0, 0, fb->width, fb->height);
 	char *buf;
 	char *wrptr;
 	int color_depth = UDL_COLOR_DEPTH_16BPP;
 
 	buf = (char *)udl->mode_buf;
 
-	/* This first section has to do with setting the base address on the
-	 * controller associated with the display. There are 2 base
-	 * pointers, currently, we only use the 16 bpp segment.
+	/*
+	 * This first section has to do with setting the base address on
+	 * the controller associated with the display. There are 2 base
+	 * pointers. Currently, we only use the 16 bpp segment.
 	 */
+
 	wrptr = udl_vidreg_lock(buf);
 	wrptr = udl_set_color_depth(wrptr, color_depth);
 	/* set base for 16bpp segment to 0 */
 	wrptr = udl_set_base16bpp(wrptr, 0);
 	/* set base for 8bpp segment to end of fb */
 	wrptr = udl_set_base8bpp(wrptr, 2 * mode->vdisplay * mode->hdisplay);
-
 	wrptr = udl_set_vid_cmds(wrptr, mode);
 	wrptr = udl_set_blank_mode(wrptr, UDL_BLANK_MODE_ON);
 	wrptr = udl_vidreg_unlock(wrptr);
-
 	wrptr = udl_dummy_render(wrptr);
 
 	udl->mode_buf_len = wrptr - buf;
 
-	udl_handle_damage(fb, &shadow_plane_state->data[0], &clip);
-
 	/* enable display */
 	udl_crtc_write_mode_to_hw(crtc);
 }
 
-static void
-udl_simple_display_pipe_disable(struct drm_simple_display_pipe *pipe)
+static void udl_crtc_helper_atomic_disable(struct drm_crtc *crtc, struct drm_atomic_state *state)
 {
-	struct drm_crtc *crtc = &pipe->crtc;
 	struct drm_device *dev = crtc->dev;
 	struct urb *urb;
 	char *buf;
@@ -374,27 +420,27 @@ udl_simple_display_pipe_disable(struct drm_simple_display_pipe *pipe)
 	udl_submit_urb(dev, urb, buf - (char *)urb->transfer_buffer);
 }
 
-static void
-udl_simple_display_pipe_update(struct drm_simple_display_pipe *pipe,
-			       struct drm_plane_state *old_plane_state)
-{
-	struct drm_plane_state *state = pipe->plane.state;
-	struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(state);
-	struct drm_framebuffer *fb = state->fb;
-	struct drm_rect rect;
+static const struct drm_crtc_helper_funcs udl_crtc_helper_funcs = {
+	.atomic_check = udl_crtc_helper_atomic_check,
+	.atomic_enable = udl_crtc_helper_atomic_enable,
+	.atomic_disable = udl_crtc_helper_atomic_disable,
+};
 
-	if (!fb)
-		return;
+static const struct drm_crtc_funcs udl_crtc_funcs = {
+	.reset = drm_atomic_helper_crtc_reset,
+	.destroy = drm_crtc_cleanup,
+	.set_config = drm_atomic_helper_set_config,
+	.page_flip = drm_atomic_helper_page_flip,
+	.atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
+	.atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
+};
 
-	if (drm_atomic_helper_damage_merged(old_plane_state, state, &rect))
-		udl_handle_damage(fb, &shadow_plane_state->data[0], &rect);
-}
+/*
+ * Encoder
+ */
 
-static const struct drm_simple_display_pipe_funcs udl_simple_display_pipe_funcs = {
-	.enable = udl_simple_display_pipe_enable,
-	.disable = udl_simple_display_pipe_disable,
-	.update = udl_simple_display_pipe_update,
-	DRM_GEM_SIMPLE_DISPLAY_PIPE_SHADOW_PLANE_FUNCS,
+static const struct drm_encoder_funcs udl_encoder_funcs = {
+	.destroy = drm_encoder_cleanup,
 };
 
 /*
@@ -534,7 +580,7 @@ static enum drm_mode_status udl_mode_config_mode_valid(struct drm_device *dev,
 	return MODE_OK;
 }
 
-static const struct drm_mode_config_funcs udl_mode_funcs = {
+static const struct drm_mode_config_funcs udl_mode_config_funcs = {
 	.fb_create = drm_gem_fb_create_with_dirty,
 	.mode_valid = udl_mode_config_mode_valid,
 	.atomic_check  = drm_atomic_helper_check,
@@ -543,8 +589,10 @@ static const struct drm_mode_config_funcs udl_mode_funcs = {
 
 int udl_modeset_init(struct drm_device *dev)
 {
-	size_t format_count = ARRAY_SIZE(udl_simple_display_pipe_formats);
 	struct udl_device *udl = to_udl(dev);
+	struct drm_plane *primary_plane;
+	struct drm_crtc *crtc;
+	struct drm_encoder *encoder;
 	struct drm_connector *connector;
 	int ret;
 
@@ -554,28 +602,42 @@ int udl_modeset_init(struct drm_device *dev)
 
 	dev->mode_config.min_width = 640;
 	dev->mode_config.min_height = 480;
-
 	dev->mode_config.max_width = 2048;
 	dev->mode_config.max_height = 2048;
-
-	dev->mode_config.prefer_shadow = 0;
 	dev->mode_config.preferred_depth = 16;
+	dev->mode_config.funcs = &udl_mode_config_funcs;
+
+	primary_plane = &udl->primary_plane;
+	ret = drm_universal_plane_init(dev, primary_plane, 0,
+				       &udl_primary_plane_funcs,
+				       udl_primary_plane_formats,
+				       ARRAY_SIZE(udl_primary_plane_formats),
+				       udl_primary_plane_fmtmods,
+				       DRM_PLANE_TYPE_PRIMARY, NULL);
+	if (ret)
+		return ret;
+	drm_plane_helper_add(primary_plane, &udl_primary_plane_helper_funcs);
+	drm_plane_enable_fb_damage_clips(primary_plane);
+
+	crtc = &udl->crtc;
+	ret = drm_crtc_init_with_planes(dev, crtc, primary_plane, NULL,
+					&udl_crtc_funcs, NULL);
+	if (ret)
+		return ret;
+	drm_crtc_helper_add(crtc, &udl_crtc_helper_funcs);
 
-	dev->mode_config.funcs = &udl_mode_funcs;
+	encoder = &udl->encoder;
+	ret = drm_encoder_init(dev, encoder, &udl_encoder_funcs, DRM_MODE_ENCODER_DAC, NULL);
+	if (ret)
+		return ret;
+	encoder->possible_crtcs = drm_crtc_mask(crtc);
 
 	connector = udl_connector_init(dev);
 	if (IS_ERR(connector))
 		return PTR_ERR(connector);
-
-	format_count = ARRAY_SIZE(udl_simple_display_pipe_formats);
-
-	ret = drm_simple_display_pipe_init(dev, &udl->display_pipe,
-					   &udl_simple_display_pipe_funcs,
-					   udl_simple_display_pipe_formats,
-					   format_count, NULL, connector);
+	ret = drm_connector_attach_encoder(connector, encoder);
 	if (ret)
 		return ret;
-	drm_plane_enable_fb_damage_clips(&udl->display_pipe.plane);
 
 	drm_mode_config_reset(dev);
 
-- 
2.37.3

Re: [PATCH 07/16] drm/udl: Convert to atomic-modesetting helpers

[Javier Martinez Canillas]

On 9/19/22 15:03, Thomas Zimmermann wrote:
> Replace simple-KMS helpers with regular atomic-modesetting helpers.
> The simple-KMS helpers introduce a mid-layer abstraction without
> added functionality. Using regular atomic helpers makes the driver's
> implementation more discoverable and simplifies code sharing.
> 
> The conversion effectively open-codes the simple-KMS functions and
> data structure within udl. No functional changes.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---

Looks good to me.

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

[PATCH 08/16] drm/udl: Simplify modesetting in CRTC's enable function

[Thomas Zimmermann]

Inline a modesetting helper in the CRTC's enable function. Build the
command set directly in the USB URB's buffer and drop an intermediate
buffer. No functional changes.

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

diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
index e96166ce2919..b090b6cebdc4 100644
--- a/drivers/gpu/drm/udl/udl_drv.h
+++ b/drivers/gpu/drm/udl/udl_drv.h
@@ -74,9 +74,6 @@ struct udl_device {
 	int sku_pixel_limit;
 
 	struct urb_list urbs;
-
-	char mode_buf[1024];
-	uint32_t mode_buf_len;
 };
 
 #define to_udl(x) container_of(x, struct udl_device, drm)
diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
index f83d0feff434..97d68cf88bd8 100644
--- a/drivers/gpu/drm/udl/udl_modeset.c
+++ b/drivers/gpu/drm/udl/udl_modeset.c
@@ -214,31 +214,6 @@ static char *udl_dummy_render(char *wrptr)
 	return wrptr;
 }
 
-static int udl_crtc_write_mode_to_hw(struct drm_crtc *crtc)
-{
-	struct drm_device *dev = crtc->dev;
-	struct udl_device *udl = to_udl(dev);
-	struct urb *urb;
-	char *buf;
-	int retval;
-
-	if (udl->mode_buf_len == 0) {
-		DRM_ERROR("No mode set\n");
-		return -EINVAL;
-	}
-
-	urb = udl_get_urb(dev);
-	if (!urb)
-		return -ENOMEM;
-
-	buf = (char *)urb->transfer_buffer;
-
-	memcpy(buf, udl->mode_buf, udl->mode_buf_len);
-	retval = udl_submit_urb(dev, urb, udl->mode_buf_len);
-	DRM_DEBUG("write mode info %d\n", udl->mode_buf_len);
-	return retval;
-}
-
 static long udl_log_cpp(unsigned int cpp)
 {
 	if (WARN_ON(!is_power_of_2(cpp)))
@@ -369,36 +344,28 @@ static int udl_crtc_helper_atomic_check(struct drm_crtc *crtc, struct drm_atomic
 static void udl_crtc_helper_atomic_enable(struct drm_crtc *crtc, struct drm_atomic_state *state)
 {
 	struct drm_device *dev = crtc->dev;
-	struct udl_device *udl = to_udl(dev);
 	struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
 	struct drm_display_mode *mode = &crtc_state->mode;
+	struct urb *urb;
 	char *buf;
-	char *wrptr;
-	int color_depth = UDL_COLOR_DEPTH_16BPP;
-
-	buf = (char *)udl->mode_buf;
 
-	/*
-	 * This first section has to do with setting the base address on
-	 * the controller associated with the display. There are 2 base
-	 * pointers. Currently, we only use the 16 bpp segment.
-	 */
+	urb = udl_get_urb(dev);
+	if (!urb)
+		return;
 
-	wrptr = udl_vidreg_lock(buf);
-	wrptr = udl_set_color_depth(wrptr, color_depth);
+	buf = (char *)urb->transfer_buffer;
+	buf = udl_vidreg_lock(buf);
+	buf = udl_set_color_depth(buf, UDL_COLOR_DEPTH_16BPP);
 	/* set base for 16bpp segment to 0 */
-	wrptr = udl_set_base16bpp(wrptr, 0);
+	buf = udl_set_base16bpp(buf, 0);
 	/* set base for 8bpp segment to end of fb */
-	wrptr = udl_set_base8bpp(wrptr, 2 * mode->vdisplay * mode->hdisplay);
-	wrptr = udl_set_vid_cmds(wrptr, mode);
-	wrptr = udl_set_blank_mode(wrptr, UDL_BLANK_MODE_ON);
-	wrptr = udl_vidreg_unlock(wrptr);
-	wrptr = udl_dummy_render(wrptr);
-
-	udl->mode_buf_len = wrptr - buf;
+	buf = udl_set_base8bpp(buf, 2 * mode->vdisplay * mode->hdisplay);
+	buf = udl_set_vid_cmds(buf, mode);
+	buf = udl_set_blank_mode(buf, UDL_BLANK_MODE_ON);
+	buf = udl_vidreg_unlock(buf);
+	buf = udl_dummy_render(buf);
 
-	/* enable display */
-	udl_crtc_write_mode_to_hw(crtc);
+	udl_submit_urb(dev, urb, buf - (char *)urb->transfer_buffer);
 }
 
 static void udl_crtc_helper_atomic_disable(struct drm_crtc *crtc, struct drm_atomic_state *state)
-- 
2.37.3

Re: [PATCH 08/16] drm/udl: Simplify modesetting in CRTC's enable function

[Javier Martinez Canillas]

On 9/19/22 15:04, Thomas Zimmermann wrote:
> Inline a modesetting helper in the CRTC's enable function. Build the
> command set directly in the USB URB's buffer and drop an intermediate
> buffer. No functional changes.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

[PATCH 09/16] drm/udl: Support DRM hot-unplugging

[Thomas Zimmermann]

Add drm_dev_enter() and drm_dev_exit() to the various modesetting
functions that interact with the device. After hot-unplugging the
device, these functions will return early. So far, the udl driver
relied on USB interfaces to handle unplugging of the device.

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

diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
index 97d68cf88bd8..aaa828034a04 100644
--- a/drivers/gpu/drm/udl/udl_modeset.c
+++ b/drivers/gpu/drm/udl/udl_modeset.c
@@ -12,6 +12,7 @@
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_crtc_helper.h>
 #include <drm/drm_damage_helper.h>
+#include <drm/drm_drv.h>
 #include <drm/drm_edid.h>
 #include <drm/drm_fourcc.h>
 #include <drm/drm_gem_atomic_helper.h>
@@ -295,14 +296,21 @@ static const uint64_t udl_primary_plane_fmtmods[] = {
 static void udl_primary_plane_helper_atomic_update(struct drm_plane *plane,
 						   struct drm_atomic_state *state)
 {
+	struct drm_device *dev = plane->dev;
 	struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, plane);
 	struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(plane_state);
 	struct drm_framebuffer *fb = plane_state->fb;
 	struct drm_plane_state *old_plane_state = drm_atomic_get_old_plane_state(state, plane);
 	struct drm_rect rect;
+	int idx;
+
+	if (!drm_dev_enter(dev, &idx))
+		return;
 
 	if (drm_atomic_helper_damage_merged(old_plane_state, plane_state, &rect))
 		udl_handle_damage(fb, &shadow_plane_state->data[0], &rect);
+
+	drm_dev_exit(idx);
 }
 
 static void udl_primary_plane_helper_atomic_disable(struct drm_plane *plane,
@@ -348,10 +356,14 @@ static void udl_crtc_helper_atomic_enable(struct drm_crtc *crtc, struct drm_atom
 	struct drm_display_mode *mode = &crtc_state->mode;
 	struct urb *urb;
 	char *buf;
+	int idx;
+
+	if (!drm_dev_enter(dev, &idx))
+		return;
 
 	urb = udl_get_urb(dev);
 	if (!urb)
-		return;
+		goto out;
 
 	buf = (char *)urb->transfer_buffer;
 	buf = udl_vidreg_lock(buf);
@@ -366,6 +378,9 @@ static void udl_crtc_helper_atomic_enable(struct drm_crtc *crtc, struct drm_atom
 	buf = udl_dummy_render(buf);
 
 	udl_submit_urb(dev, urb, buf - (char *)urb->transfer_buffer);
+
+out:
+	drm_dev_exit(idx);
 }
 
 static void udl_crtc_helper_atomic_disable(struct drm_crtc *crtc, struct drm_atomic_state *state)
@@ -373,10 +388,14 @@ static void udl_crtc_helper_atomic_disable(struct drm_crtc *crtc, struct drm_ato
 	struct drm_device *dev = crtc->dev;
 	struct urb *urb;
 	char *buf;
+	int idx;
+
+	if (!drm_dev_enter(dev, &idx))
+		return;
 
 	urb = udl_get_urb(dev);
 	if (!urb)
-		return;
+		goto out;
 
 	buf = (char *)urb->transfer_buffer;
 	buf = udl_vidreg_lock(buf);
@@ -385,6 +404,9 @@ static void udl_crtc_helper_atomic_disable(struct drm_crtc *crtc, struct drm_ato
 	buf = udl_dummy_render(buf);
 
 	udl_submit_urb(dev, urb, buf - (char *)urb->transfer_buffer);
+
+out:
+	drm_dev_exit(idx);
 }
 
 static const struct drm_crtc_helper_funcs udl_crtc_helper_funcs = {
@@ -471,17 +493,26 @@ static int udl_get_edid_block(void *data, u8 *buf, unsigned int block, size_t le
 
 static enum drm_connector_status udl_connector_detect(struct drm_connector *connector, bool force)
 {
-	struct udl_device *udl = to_udl(connector->dev);
+	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;
+	int idx;
 
 	/* 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)
+	if (!drm_dev_enter(dev, &idx))
 		return connector_status_disconnected;
 
-	return connector_status_connected;
+	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;
 }
 
 static void udl_connector_destroy(struct drm_connector *connector)
-- 
2.37.3

Re: [PATCH 09/16] drm/udl: Support DRM hot-unplugging

[Javier Martinez Canillas]

On 9/19/22 15:04, Thomas Zimmermann wrote:
> Add drm_dev_enter() and drm_dev_exit() to the various modesetting
> functions that interact with the device. After hot-unplugging the
> device, these functions will return early. So far, the udl driver
> relied on USB interfaces to handle unplugging of the device.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

[PATCH 10/16] drm/udl: Use damage iterator

[Thomas Zimmermann]

Use a damage iterator to process damage areas individually. Merging
damage areas can resul tin large updates of unchanged framebuffer
regions. As USB is rather slow, it's better to process damage areas
individually and hence minimize USB-transfered data.

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

diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
index aaa828034a04..6fa4c8b5c2c6 100644
--- a/drivers/gpu/drm/udl/udl_modeset.c
+++ b/drivers/gpu/drm/udl/udl_modeset.c
@@ -238,15 +238,9 @@ static int udl_handle_damage(struct drm_framebuffer *fb,
 		return ret;
 	log_bpp = ret;
 
-	ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
-	if (ret)
-		return ret;
-
 	urb = udl_get_urb(dev);
-	if (!urb) {
-		ret = -ENOMEM;
-		goto out_drm_gem_fb_end_cpu_access;
-	}
+	if (!urb)
+		return -ENOMEM;
 	cmd = urb->transfer_buffer;
 
 	for (i = clip->y1; i < clip->y2; i++) {
@@ -258,7 +252,7 @@ static int udl_handle_damage(struct drm_framebuffer *fb,
 				       &cmd, byte_offset, dev_byte_offset,
 				       byte_width);
 		if (ret)
-			goto out_drm_gem_fb_end_cpu_access;
+			return ret;
 	}
 
 	if (cmd > (char *)urb->transfer_buffer) {
@@ -272,11 +266,7 @@ static int udl_handle_damage(struct drm_framebuffer *fb,
 		udl_urb_completion(urb);
 	}
 
-	ret = 0;
-
-out_drm_gem_fb_end_cpu_access:
-	drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
-	return ret;
+	return 0;
 }
 
 /*
@@ -301,16 +291,26 @@ static void udl_primary_plane_helper_atomic_update(struct drm_plane *plane,
 	struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(plane_state);
 	struct drm_framebuffer *fb = plane_state->fb;
 	struct drm_plane_state *old_plane_state = drm_atomic_get_old_plane_state(state, plane);
-	struct drm_rect rect;
-	int idx;
+	struct drm_atomic_helper_damage_iter iter;
+	struct drm_rect damage;
+	int ret, idx;
 
-	if (!drm_dev_enter(dev, &idx))
+	ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
+	if (ret)
 		return;
 
-	if (drm_atomic_helper_damage_merged(old_plane_state, plane_state, &rect))
-		udl_handle_damage(fb, &shadow_plane_state->data[0], &rect);
+	if (!drm_dev_enter(dev, &idx))
+		goto out_drm_gem_fb_end_cpu_access;
+
+	drm_atomic_helper_damage_iter_init(&iter, old_plane_state, plane_state);
+	drm_atomic_for_each_plane_damage(&iter, &damage) {
+		udl_handle_damage(fb, &shadow_plane_state->data[0], &damage);
+	}
 
 	drm_dev_exit(idx);
+
+out_drm_gem_fb_end_cpu_access:
+	drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
 }
 
 static void udl_primary_plane_helper_atomic_disable(struct drm_plane *plane,
-- 
2.37.3

Re: [PATCH 10/16] drm/udl: Use damage iterator

[Javier Martinez Canillas]

On 9/19/22 15:04, Thomas Zimmermann wrote:
> Use a damage iterator to process damage areas individually. Merging
> damage areas can resul tin large updates of unchanged framebuffer

result in

> regions. As USB is rather slow, it's better to process damage areas
> individually and hence minimize USB-transfered data.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

but I've a comment below.

>  
>  /*
> @@ -301,16 +291,26 @@ static void udl_primary_plane_helper_atomic_update(struct drm_plane *plane,
>  	struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(plane_state);
>  	struct drm_framebuffer *fb = plane_state->fb;
>  	struct drm_plane_state *old_plane_state = drm_atomic_get_old_plane_state(state, plane);
> -	struct drm_rect rect;
> -	int idx;
> +	struct drm_atomic_helper_damage_iter iter;
> +	struct drm_rect damage;
> +	int ret, idx;
>  
> -	if (!drm_dev_enter(dev, &idx))
> +	ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
> +	if (ret)
>  		return;
>

This is an unrelated change. The sync was made in udl_handle_damage() before
and you are moving to udl_primary_plane_helper_atomic_update() in this patch.

I don't have a strong opinion and I see that there are drivers that do once
before iterating over the damage areas and others do the sync for each damage
area update. But it would be good to mention that this change is done too and
provided some justification.

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 10/16] drm/udl: Use damage iterator

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 2075 bytes --]

Hi

Am 05.10.22 um 00:28 schrieb Javier Martinez Canillas:
> On 9/19/22 15:04, Thomas Zimmermann wrote:
>> Use a damage iterator to process damage areas individually. Merging
>> damage areas can resul tin large updates of unchanged framebuffer
> 
> result in
> 
>> regions. As USB is rather slow, it's better to process damage areas
>> individually and hence minimize USB-transfered data.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
> 
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> 
> but I've a comment below.
> 
>>   
>>   /*
>> @@ -301,16 +291,26 @@ static void udl_primary_plane_helper_atomic_update(struct drm_plane *plane,
>>   	struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(plane_state);
>>   	struct drm_framebuffer *fb = plane_state->fb;
>>   	struct drm_plane_state *old_plane_state = drm_atomic_get_old_plane_state(state, plane);
>> -	struct drm_rect rect;
>> -	int idx;
>> +	struct drm_atomic_helper_damage_iter iter;
>> +	struct drm_rect damage;
>> +	int ret, idx;
>>   
>> -	if (!drm_dev_enter(dev, &idx))
>> +	ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
>> +	if (ret)
>>   		return;
>>
> 
> This is an unrelated change. The sync was made in udl_handle_damage() before
> and you are moving to udl_primary_plane_helper_atomic_update() in this patch.
> 
> I don't have a strong opinion and I see that there are drivers that do once
> before iterating over the damage areas and others do the sync for each damage
> area update. But it would be good to mention that this change is done too and
> provided some justification.

OK, I'll do that. Very briefly; it's for minimizing the overheads of the 
possible locking and synchronization, and to maybe it keeps possible 
writers out while we copy from the buffer.

Best regards
Thomas

> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

[PATCH 11/16] drm/udl: Move register constants to udl_proto.h

[Thomas Zimmermann]

Move the existing register constants to a new file in preparation of
adding more of them. Renaming is intentional. No functional changes.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/udl/udl_drv.h     |  9 ---------
 drivers/gpu/drm/udl/udl_modeset.c | 11 +++++------
 drivers/gpu/drm/udl/udl_proto.h   | 16 ++++++++++++++++
 3 files changed, 21 insertions(+), 15 deletions(-)
 create mode 100644 drivers/gpu/drm/udl/udl_proto.h

diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
index b090b6cebdc4..580989087c54 100644
--- a/drivers/gpu/drm/udl/udl_drv.h
+++ b/drivers/gpu/drm/udl/udl_drv.h
@@ -112,13 +112,4 @@ int udl_select_std_channel(struct udl_device *udl);
 #define CMD_WRITE_COPY16 "\xAF\x6A" /**< 16 bit copy command. */
 #define CMD_WRITE_RLX16  "\xAF\x6B" /**< 16 bit extended run length command. */
 
-/* On/Off for driving the DisplayLink framebuffer to the display */
-#define UDL_REG_BLANK_MODE		0x1f
-
-#define UDL_BLANK_MODE_ON		0x00 /* hsync and vsync on, visible */
-#define UDL_BLANK_MODE_BLANKED		0x01 /* hsync and vsync on, blanked */
-#define UDL_BLANK_MODE_VSYNC_OFF	0x03 /* vsync off, blanked */
-#define UDL_BLANK_MODE_HSYNC_OFF	0x05 /* hsync off, blanked */
-#define UDL_BLANK_MODE_POWERDOWN	0x07 /* powered off; requires modeset */
-
 #endif
diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
index 6fa4c8b5c2c6..e80ed218563e 100644
--- a/drivers/gpu/drm/udl/udl_modeset.c
+++ b/drivers/gpu/drm/udl/udl_modeset.c
@@ -24,8 +24,7 @@
 #include <drm/drm_vblank.h>
 
 #include "udl_drv.h"
-
-#define UDL_COLOR_DEPTH_16BPP	0
+#include "udl_proto.h"
 
 /*
  * All DisplayLink bulk operations start with 0xAF, followed by specific code
@@ -52,7 +51,7 @@ static char *udl_vidreg_unlock(char *buf)
 
 static char *udl_set_blank_mode(char *buf, u8 mode)
 {
-	return udl_set_register(buf, UDL_REG_BLANK_MODE, mode);
+	return udl_set_register(buf, UDL_REG_BLANKMODE, mode);
 }
 
 static char *udl_set_color_depth(char *buf, u8 selection)
@@ -367,13 +366,13 @@ static void udl_crtc_helper_atomic_enable(struct drm_crtc *crtc, struct drm_atom
 
 	buf = (char *)urb->transfer_buffer;
 	buf = udl_vidreg_lock(buf);
-	buf = udl_set_color_depth(buf, UDL_COLOR_DEPTH_16BPP);
+	buf = udl_set_color_depth(buf, UDL_COLORDEPTH_16BPP);
 	/* set base for 16bpp segment to 0 */
 	buf = udl_set_base16bpp(buf, 0);
 	/* set base for 8bpp segment to end of fb */
 	buf = udl_set_base8bpp(buf, 2 * mode->vdisplay * mode->hdisplay);
 	buf = udl_set_vid_cmds(buf, mode);
-	buf = udl_set_blank_mode(buf, UDL_BLANK_MODE_ON);
+	buf = udl_set_blank_mode(buf, UDL_BLANKMODE_ON);
 	buf = udl_vidreg_unlock(buf);
 	buf = udl_dummy_render(buf);
 
@@ -399,7 +398,7 @@ static void udl_crtc_helper_atomic_disable(struct drm_crtc *crtc, struct drm_ato
 
 	buf = (char *)urb->transfer_buffer;
 	buf = udl_vidreg_lock(buf);
-	buf = udl_set_blank_mode(buf, UDL_BLANK_MODE_POWERDOWN);
+	buf = udl_set_blank_mode(buf, UDL_BLANKMODE_POWERDOWN);
 	buf = udl_vidreg_unlock(buf);
 	buf = udl_dummy_render(buf);
 
diff --git a/drivers/gpu/drm/udl/udl_proto.h b/drivers/gpu/drm/udl/udl_proto.h
new file mode 100644
index 000000000000..3f5b8e832b99
--- /dev/null
+++ b/drivers/gpu/drm/udl/udl_proto.h
@@ -0,0 +1,16 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#ifndef UDL_PROTO_H
+#define UDL_PROTO_H
+
+#define UDL_COLORDEPTH_16BPP		0
+
+/* On/Off for driving the DisplayLink framebuffer to the display */
+#define UDL_REG_BLANKMODE		0x1f
+#define UDL_BLANKMODE_ON		0x00 /* hsync and vsync on, visible */
+#define UDL_BLANKMODE_BLANKED		0x01 /* hsync and vsync on, blanked */
+#define UDL_BLANKMODE_VSYNC_OFF		0x03 /* vsync off, blanked */
+#define UDL_BLANKMODE_HSYNC_OFF		0x05 /* hsync off, blanked */
+#define UDL_BLANKMODE_POWERDOWN		0x07 /* powered off; requires modeset */
+
+#endif
-- 
2.37.3

Re: [PATCH 11/16] drm/udl: Move register constants to udl_proto.h

[Javier Martinez Canillas]

On 9/19/22 15:04, Thomas Zimmermann wrote:
> Move the existing register constants to a new file in preparation of
> adding more of them. Renaming is intentional. No functional changes.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

[PATCH 12/16] drm/udl: Add constants for display-mode registers

[Thomas Zimmermann]

Add constants for the registers the contain various display-mode
parameters and update the mode-setting function. No functional
changes.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/udl/udl_modeset.c | 102 ++++++++++--------------------
 drivers/gpu/drm/udl/udl_proto.h   |  15 +++++
 2 files changed, 47 insertions(+), 70 deletions(-)

diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
index e80ed218563e..04135ebd41d3 100644
--- a/drivers/gpu/drm/udl/udl_modeset.c
+++ b/drivers/gpu/drm/udl/udl_modeset.c
@@ -126,78 +126,40 @@ static char *udl_set_register_lfsr16(char *wrptr, u8 reg, u16 value)
 }
 
 /*
- * This takes a standard fbdev screeninfo struct and all of its monitor mode
- * details and converts them into the DisplayLink equivalent register commands.
-  ERR(vreg(dev,               0x00, (color_depth == 16) ? 0 : 1));
-  ERR(vreg_lfsr16(dev,        0x01, xDisplayStart));
-  ERR(vreg_lfsr16(dev,        0x03, xDisplayEnd));
-  ERR(vreg_lfsr16(dev,        0x05, yDisplayStart));
-  ERR(vreg_lfsr16(dev,        0x07, yDisplayEnd));
-  ERR(vreg_lfsr16(dev,        0x09, xEndCount));
-  ERR(vreg_lfsr16(dev,        0x0B, hSyncStart));
-  ERR(vreg_lfsr16(dev,        0x0D, hSyncEnd));
-  ERR(vreg_big_endian(dev,    0x0F, hPixels));
-  ERR(vreg_lfsr16(dev,        0x11, yEndCount));
-  ERR(vreg_lfsr16(dev,        0x13, vSyncStart));
-  ERR(vreg_lfsr16(dev,        0x15, vSyncEnd));
-  ERR(vreg_big_endian(dev,    0x17, vPixels));
-  ERR(vreg_little_endian(dev, 0x1B, pixelClock5KHz));
-
-  ERR(vreg(dev,               0x1F, 0));
-
-  ERR(vbuf(dev, WRITE_VIDREG_UNLOCK, DSIZEOF(WRITE_VIDREG_UNLOCK)));
+ * Takes a DRM display mode and converts it into the DisplayLink
+ * equivalent register commands.
  */
-static char *udl_set_vid_cmds(char *wrptr, struct drm_display_mode *mode)
+static char *udl_set_display_mode(char *buf, struct drm_display_mode *mode)
 {
-	u16 xds, yds;
-	u16 xde, yde;
-	u16 yec;
+	u16 reg01 = mode->crtc_htotal - mode->crtc_hsync_start;
+	u16 reg03 = reg01 + mode->crtc_hdisplay;
+	u16 reg05 = mode->crtc_vtotal - mode->crtc_vsync_start;
+	u16 reg07 = reg05 + mode->crtc_vdisplay;
+	u16 reg09 = mode->crtc_htotal - 1;
+	u16 reg0b = 1; /* libdlo hardcodes hsync start to 1 */
+	u16 reg0d = mode->crtc_hsync_end - mode->crtc_hsync_start + 1;
+	u16 reg0f = mode->hdisplay;
+	u16 reg11 = mode->crtc_vtotal;
+	u16 reg13 = 0; /* libdlo hardcodes vsync start to 0 */
+	u16 reg15 = mode->crtc_vsync_end - mode->crtc_vsync_start;
+	u16 reg17 = mode->crtc_vdisplay;
+	u16 reg1b = mode->clock / 5;
+
+	buf = udl_set_register_lfsr16(buf, UDL_REG_XDISPLAYSTART, reg01);
+	buf = udl_set_register_lfsr16(buf, UDL_REG_XDISPLAYEND, reg03);
+	buf = udl_set_register_lfsr16(buf, UDL_REG_YDISPLAYSTART, reg05);
+	buf = udl_set_register_lfsr16(buf, UDL_REG_YDISPLAYEND, reg07);
+	buf = udl_set_register_lfsr16(buf, UDL_REG_XENDCOUNT, reg09);
+	buf = udl_set_register_lfsr16(buf, UDL_REG_HSYNCSTART, reg0b);
+	buf = udl_set_register_lfsr16(buf, UDL_REG_HSYNCEND, reg0d);
+	buf = udl_set_register_16(buf, UDL_REG_HPIXELS, reg0f);
+	buf = udl_set_register_lfsr16(buf, UDL_REG_YENDCOUNT, reg11);
+	buf = udl_set_register_lfsr16(buf, UDL_REG_VSYNCSTART, reg13);
+	buf = udl_set_register_lfsr16(buf, UDL_REG_VSYNCEND, reg15);
+	buf = udl_set_register_16(buf, UDL_REG_VPIXELS, reg17);
+	buf = udl_set_register_16be(buf, UDL_REG_PIXELCLOCK5KHZ, reg1b);
 
-	/* x display start */
-	xds = mode->crtc_htotal - mode->crtc_hsync_start;
-	wrptr = udl_set_register_lfsr16(wrptr, 0x01, xds);
-	/* x display end */
-	xde = xds + mode->crtc_hdisplay;
-	wrptr = udl_set_register_lfsr16(wrptr, 0x03, xde);
-
-	/* y display start */
-	yds = mode->crtc_vtotal - mode->crtc_vsync_start;
-	wrptr = udl_set_register_lfsr16(wrptr, 0x05, yds);
-	/* y display end */
-	yde = yds + mode->crtc_vdisplay;
-	wrptr = udl_set_register_lfsr16(wrptr, 0x07, yde);
-
-	/* x end count is active + blanking - 1 */
-	wrptr = udl_set_register_lfsr16(wrptr, 0x09,
-					mode->crtc_htotal - 1);
-
-	/* libdlo hardcodes hsync start to 1 */
-	wrptr = udl_set_register_lfsr16(wrptr, 0x0B, 1);
-
-	/* hsync end is width of sync pulse + 1 */
-	wrptr = udl_set_register_lfsr16(wrptr, 0x0D,
-					mode->crtc_hsync_end - mode->crtc_hsync_start + 1);
-
-	/* hpixels is active pixels */
-	wrptr = udl_set_register_16(wrptr, 0x0F, mode->hdisplay);
-
-	/* yendcount is vertical active + vertical blanking */
-	yec = mode->crtc_vtotal;
-	wrptr = udl_set_register_lfsr16(wrptr, 0x11, yec);
-
-	/* libdlo hardcodes vsync start to 0 */
-	wrptr = udl_set_register_lfsr16(wrptr, 0x13, 0);
-
-	/* vsync end is width of vsync pulse */
-	wrptr = udl_set_register_lfsr16(wrptr, 0x15, mode->crtc_vsync_end - mode->crtc_vsync_start);
-
-	/* vpixels is active pixels */
-	wrptr = udl_set_register_16(wrptr, 0x17, mode->crtc_vdisplay);
-
-	wrptr = udl_set_register_16be(wrptr, 0x1B,
-				      mode->clock / 5);
-
-	return wrptr;
+	return buf;
 }
 
 static char *udl_dummy_render(char *wrptr)
@@ -371,7 +333,7 @@ static void udl_crtc_helper_atomic_enable(struct drm_crtc *crtc, struct drm_atom
 	buf = udl_set_base16bpp(buf, 0);
 	/* set base for 8bpp segment to end of fb */
 	buf = udl_set_base8bpp(buf, 2 * mode->vdisplay * mode->hdisplay);
-	buf = udl_set_vid_cmds(buf, mode);
+	buf = udl_set_display_mode(buf, mode);
 	buf = udl_set_blank_mode(buf, UDL_BLANKMODE_ON);
 	buf = udl_vidreg_unlock(buf);
 	buf = udl_dummy_render(buf);
diff --git a/drivers/gpu/drm/udl/udl_proto.h b/drivers/gpu/drm/udl/udl_proto.h
index 3f5b8e832b99..5a6c960bd10d 100644
--- a/drivers/gpu/drm/udl/udl_proto.h
+++ b/drivers/gpu/drm/udl/udl_proto.h
@@ -5,6 +5,21 @@
 
 #define UDL_COLORDEPTH_16BPP		0
 
+/* Display-mode settings */
+#define UDL_REG_XDISPLAYSTART		0x01
+#define UDL_REG_XDISPLAYEND		0x03
+#define UDL_REG_YDISPLAYSTART		0x05
+#define UDL_REG_YDISPLAYEND		0x07
+#define UDL_REG_XENDCOUNT		0x09
+#define UDL_REG_HSYNCSTART		0x0b
+#define UDL_REG_HSYNCEND		0x0d
+#define UDL_REG_HPIXELS			0x0f
+#define UDL_REG_YENDCOUNT		0x11
+#define UDL_REG_VSYNCSTART		0x13
+#define UDL_REG_VSYNCEND		0x15
+#define UDL_REG_VPIXELS			0x17
+#define UDL_REG_PIXELCLOCK5KHZ		0x1b
+
 /* On/Off for driving the DisplayLink framebuffer to the display */
 #define UDL_REG_BLANKMODE		0x1f
 #define UDL_BLANKMODE_ON		0x00 /* hsync and vsync on, visible */
-- 
2.37.3

Re: [PATCH 12/16] drm/udl: Add constants for display-mode registers

[Javier Martinez Canillas]

On 9/19/22 15:04, Thomas Zimmermann wrote:
> Add constants for the registers the contain various display-mode
> parameters and update the mode-setting function. No functional
> changes.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

[PATCH 13/16] drm/udl: Add register constants for color depth

[Thomas Zimmermann]

Add the register constants for setting the color depth. The driver
only uses 16bpp. No functional changes.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/udl/udl_modeset.c | 2 +-
 drivers/gpu/drm/udl/udl_proto.h   | 3 +++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
index 04135ebd41d3..1e28eb1e295f 100644
--- a/drivers/gpu/drm/udl/udl_modeset.c
+++ b/drivers/gpu/drm/udl/udl_modeset.c
@@ -56,7 +56,7 @@ static char *udl_set_blank_mode(char *buf, u8 mode)
 
 static char *udl_set_color_depth(char *buf, u8 selection)
 {
-	return udl_set_register(buf, 0x00, selection);
+	return udl_set_register(buf, UDL_REG_COLORDEPTH, selection);
 }
 
 static char *udl_set_base16bpp(char *wrptr, u32 base)
diff --git a/drivers/gpu/drm/udl/udl_proto.h b/drivers/gpu/drm/udl/udl_proto.h
index 5a6c960bd10d..22bc1ae8420c 100644
--- a/drivers/gpu/drm/udl/udl_proto.h
+++ b/drivers/gpu/drm/udl/udl_proto.h
@@ -3,7 +3,10 @@
 #ifndef UDL_PROTO_H
 #define UDL_PROTO_H
 
+/* Color depth */
+#define UDL_REG_COLORDEPTH		0x00
 #define UDL_COLORDEPTH_16BPP		0
+#define UDL_COLORDEPTH_24BPP		1
 
 /* Display-mode settings */
 #define UDL_REG_XDISPLAYSTART		0x01
-- 
2.37.3

Re: [PATCH 13/16] drm/udl: Add register constants for color depth

[Javier Martinez Canillas]

On 9/19/22 15:04, Thomas Zimmermann wrote:
> Add the register constants for setting the color depth. The driver
> only uses 16bpp. No functional changes.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

[PATCH 14/16] drm/udl: Add register constants for video locks

[Thomas Zimmermann]

Add register constants for the video lock. No functional changes.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/udl/udl_modeset.c | 4 ++--
 drivers/gpu/drm/udl/udl_proto.h   | 5 +++++
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
index 1e28eb1e295f..b6aebfaae03d 100644
--- a/drivers/gpu/drm/udl/udl_modeset.c
+++ b/drivers/gpu/drm/udl/udl_modeset.c
@@ -41,12 +41,12 @@ static char *udl_set_register(char *buf, u8 reg, u8 val)
 
 static char *udl_vidreg_lock(char *buf)
 {
-	return udl_set_register(buf, 0xFF, 0x00);
+	return udl_set_register(buf, UDL_REG_VIDREG, UDL_VIDREG_LOCK);
 }
 
 static char *udl_vidreg_unlock(char *buf)
 {
-	return udl_set_register(buf, 0xFF, 0xFF);
+	return udl_set_register(buf, UDL_REG_VIDREG, UDL_VIDREG_UNLOCK);
 }
 
 static char *udl_set_blank_mode(char *buf, u8 mode)
diff --git a/drivers/gpu/drm/udl/udl_proto.h b/drivers/gpu/drm/udl/udl_proto.h
index 22bc1ae8420c..8e7d1a090644 100644
--- a/drivers/gpu/drm/udl/udl_proto.h
+++ b/drivers/gpu/drm/udl/udl_proto.h
@@ -31,4 +31,9 @@
 #define UDL_BLANKMODE_HSYNC_OFF		0x05 /* hsync off, blanked */
 #define UDL_BLANKMODE_POWERDOWN		0x07 /* powered off; requires modeset */
 
+/* Lock/unlock video registers */
+#define UDL_REG_VIDREG			0xff
+#define UDL_VIDREG_LOCK			0x00
+#define UDL_VIDREG_UNLOCK		0xff
+
 #endif
-- 
2.37.3

Re: [PATCH 14/16] drm/udl: Add register constants for video locks

[Javier Martinez Canillas]

On 9/19/22 15:04, Thomas Zimmermann wrote:
> Add register constants for the video lock. No functional changes.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

[PATCH 15/16] drm/udl: Add register constants for framebuffer scanout addresses

[Thomas Zimmermann]

Add register constants for the framebuffer scanout addresses and
update the related helper functions. No functional changes.

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

diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
index b6aebfaae03d..df0b70f3ddf1 100644
--- a/drivers/gpu/drm/udl/udl_modeset.c
+++ b/drivers/gpu/drm/udl/udl_modeset.c
@@ -59,23 +59,35 @@ static char *udl_set_color_depth(char *buf, u8 selection)
 	return udl_set_register(buf, UDL_REG_COLORDEPTH, selection);
 }
 
-static char *udl_set_base16bpp(char *wrptr, u32 base)
+static char *udl_set_base16bpp(char *buf, u32 base)
 {
 	/* the base pointer is 16 bits wide, 0x20 is hi byte. */
-	wrptr = udl_set_register(wrptr, 0x20, base >> 16);
-	wrptr = udl_set_register(wrptr, 0x21, base >> 8);
-	return udl_set_register(wrptr, 0x22, base);
+	u8 reg20 = (base & 0xff0000) >> 16;
+	u8 reg21 = (base & 0x00ff00) >> 8;
+	u8 reg22 = (base & 0x0000ff);
+
+	buf = udl_set_register(buf, UDL_REG_BASE16BPP_ADDR2, reg20);
+	buf = udl_set_register(buf, UDL_REG_BASE16BPP_ADDR1, reg21);
+	buf = udl_set_register(buf, UDL_REG_BASE16BPP_ADDR0, reg22);
+
+	return buf;
 }
 
 /*
  * DisplayLink HW has separate 16bpp and 8bpp framebuffers.
  * In 24bpp modes, the low 323 RGB bits go in the 8bpp framebuffer
  */
-static char *udl_set_base8bpp(char *wrptr, u32 base)
+static char *udl_set_base8bpp(char *buf, u32 base)
 {
-	wrptr = udl_set_register(wrptr, 0x26, base >> 16);
-	wrptr = udl_set_register(wrptr, 0x27, base >> 8);
-	return udl_set_register(wrptr, 0x28, base);
+	u8 reg26 = (base & 0xff0000) >> 16;
+	u8 reg27 = (base & 0x00ff00) >> 8;
+	u8 reg28 = (base & 0x0000ff);
+
+	buf = udl_set_register(buf, UDL_REG_BASE8BPP_ADDR2, reg26);
+	buf = udl_set_register(buf, UDL_REG_BASE8BPP_ADDR1, reg27);
+	buf = udl_set_register(buf, UDL_REG_BASE8BPP_ADDR0, reg28);
+
+	return buf;
 }
 
 static char *udl_set_register_16(char *wrptr, u8 reg, u16 value)
diff --git a/drivers/gpu/drm/udl/udl_proto.h b/drivers/gpu/drm/udl/udl_proto.h
index 8e7d1a090644..3e7fcb43cb04 100644
--- a/drivers/gpu/drm/udl/udl_proto.h
+++ b/drivers/gpu/drm/udl/udl_proto.h
@@ -31,6 +31,14 @@
 #define UDL_BLANKMODE_HSYNC_OFF		0x05 /* hsync off, blanked */
 #define UDL_BLANKMODE_POWERDOWN		0x07 /* powered off; requires modeset */
 
+/* Framebuffer address */
+#define UDL_REG_BASE16BPP_ADDR2		0x20
+#define UDL_REG_BASE16BPP_ADDR1		0x21
+#define UDL_REG_BASE16BPP_ADDR0		0x22
+#define UDL_REG_BASE8BPP_ADDR2		0x26
+#define UDL_REG_BASE8BPP_ADDR1		0x27
+#define UDL_REG_BASE8BPP_ADDR0		0x28
+
 /* Lock/unlock video registers */
 #define UDL_REG_VIDREG			0xff
 #define UDL_VIDREG_LOCK			0x00
-- 
2.37.3

Re: [PATCH 15/16] drm/udl: Add register constants for framebuffer scanout addresses

[Javier Martinez Canillas]

On 9/19/22 15:04, Thomas Zimmermann wrote:
> Add register constants for the framebuffer scanout addresses and
> update the related helper functions. No functional changes.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

[...]

> +	u8 reg20 = (base & 0xff0000) >> 16;
> +	u8 reg21 = (base & 0x00ff00) >> 8;
> +	u8 reg22 = (base & 0x0000ff);
> +

Maybe would be cleaner to use the FIELD_PREP() and GENMASK() macros instead ?

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 15/16] drm/udl: Add register constants for framebuffer scanout addresses

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 907 bytes --]

Hi

Am 05.10.22 um 00:59 schrieb Javier Martinez Canillas:
> On 9/19/22 15:04, Thomas Zimmermann wrote:
>> Add register constants for the framebuffer scanout addresses and
>> update the related helper functions. No functional changes.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
> 
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> 
> [...]
> 
>> +	u8 reg20 = (base & 0xff0000) >> 16;
>> +	u8 reg21 = (base & 0x00ff00) >> 8;
>> +	u8 reg22 = (base & 0x0000ff);
>> +
> 
> Maybe would be cleaner to use the FIELD_PREP() and GENMASK() macros instead ?
> 

Thank you for reviewing my patchset. I'll update the patch with these 
macros.

Best regards
Thomas


-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

[PATCH 16/16] drm/udl: Add constants for commands

[Thomas Zimmermann]

Add constants for the various commands that the driver can send to
the device and update the respective helper functions. No functional
changes.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/udl/udl_drv.h      | 10 ----------
 drivers/gpu/drm/udl/udl_modeset.c  | 16 +++++++++-------
 drivers/gpu/drm/udl/udl_proto.h    | 15 +++++++++++++++
 drivers/gpu/drm/udl/udl_transfer.c |  7 ++++---
 4 files changed, 28 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
index 580989087c54..282ebd6c02fd 100644
--- a/drivers/gpu/drm/udl/udl_drv.h
+++ b/drivers/gpu/drm/udl/udl_drv.h
@@ -102,14 +102,4 @@ int udl_render_hline(struct drm_device *dev, int log_bpp, struct urb **urb_ptr,
 int udl_drop_usb(struct drm_device *dev);
 int udl_select_std_channel(struct udl_device *udl);
 
-#define CMD_WRITE_RAW8   "\xAF\x60" /**< 8 bit raw write command. */
-#define CMD_WRITE_RL8    "\xAF\x61" /**< 8 bit run length command. */
-#define CMD_WRITE_COPY8  "\xAF\x62" /**< 8 bit copy command. */
-#define CMD_WRITE_RLX8   "\xAF\x63" /**< 8 bit extended run length command. */
-
-#define CMD_WRITE_RAW16  "\xAF\x68" /**< 16 bit raw write command. */
-#define CMD_WRITE_RL16   "\xAF\x69" /**< 16 bit run length command. */
-#define CMD_WRITE_COPY16 "\xAF\x6A" /**< 16 bit copy command. */
-#define CMD_WRITE_RLX16  "\xAF\x6B" /**< 16 bit extended run length command. */
-
 #endif
diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
index df0b70f3ddf1..1814bc84effe 100644
--- a/drivers/gpu/drm/udl/udl_modeset.c
+++ b/drivers/gpu/drm/udl/udl_modeset.c
@@ -27,15 +27,17 @@
 #include "udl_proto.h"
 
 /*
- * All DisplayLink bulk operations start with 0xAF, followed by specific code
- * All operations are written to buffers which then later get sent to device
+ * All DisplayLink bulk operations start with 0xaf (UDL_MSG_BULK), followed by
+ * a specific command code. All operations are written to a command buffer, which
+ * the driver sends to the device.
  */
 static char *udl_set_register(char *buf, u8 reg, u8 val)
 {
-	*buf++ = 0xAF;
-	*buf++ = 0x20;
+	*buf++ = UDL_MSG_BULK;
+	*buf++ = UDL_CMD_WRITEREG;
 	*buf++ = reg;
 	*buf++ = val;
+
 	return buf;
 }
 
@@ -176,8 +178,8 @@ static char *udl_set_display_mode(char *buf, struct drm_display_mode *mode)
 
 static char *udl_dummy_render(char *wrptr)
 {
-	*wrptr++ = 0xAF;
-	*wrptr++ = 0x6A; /* copy */
+	*wrptr++ = UDL_MSG_BULK;
+	*wrptr++ = UDL_CMD_WRITECOPY16;
 	*wrptr++ = 0x00; /* from addr */
 	*wrptr++ = 0x00;
 	*wrptr++ = 0x00;
@@ -232,7 +234,7 @@ static int udl_handle_damage(struct drm_framebuffer *fb,
 		/* Send partial buffer remaining before exiting */
 		int len;
 		if (cmd < (char *)urb->transfer_buffer + urb->transfer_buffer_length)
-			*cmd++ = 0xAF;
+			*cmd++ = UDL_MSG_BULK;
 		len = cmd - (char *)urb->transfer_buffer;
 		ret = udl_submit_urb(dev, urb, len);
 	} else {
diff --git a/drivers/gpu/drm/udl/udl_proto.h b/drivers/gpu/drm/udl/udl_proto.h
index 3e7fcb43cb04..b3b199ffc616 100644
--- a/drivers/gpu/drm/udl/udl_proto.h
+++ b/drivers/gpu/drm/udl/udl_proto.h
@@ -3,6 +3,21 @@
 #ifndef UDL_PROTO_H
 #define UDL_PROTO_H
 
+#define UDL_MSG_BULK		0xaf
+
+/* Register access */
+#define UDL_CMD_WRITEREG	0x20 /* See register constants below */
+
+/* Framebuffer access */
+#define UDL_CMD_WRITERAW8	0x60 /* 8 bit raw write command. */
+#define UDL_CMD_WRITERL8	0x61 /* 8 bit run length command. */
+#define UDL_CMD_WRITECOPY8	0x62 /* 8 bit copy command. */
+#define UDL_CMD_WRITERLX8	0x63 /* 8 bit extended run length command. */
+#define UDL_CMD_WRITERAW16	0x68 /* 16 bit raw write command. */
+#define UDL_CMD_WRITERL16	0x69 /* 16 bit run length command. */
+#define UDL_CMD_WRITECOPY16	0x6a /* 16 bit copy command. */
+#define UDL_CMD_WRITERLX16	0x6b /* 16 bit extended run length command. */
+
 /* Color depth */
 #define UDL_REG_COLORDEPTH		0x00
 #define UDL_COLORDEPTH_16BPP		0
diff --git a/drivers/gpu/drm/udl/udl_transfer.c b/drivers/gpu/drm/udl/udl_transfer.c
index b57844632dbd..5ff1037a3453 100644
--- a/drivers/gpu/drm/udl/udl_transfer.c
+++ b/drivers/gpu/drm/udl/udl_transfer.c
@@ -10,6 +10,7 @@
 #include <asm/unaligned.h>
 
 #include "udl_drv.h"
+#include "udl_proto.h"
 
 #define MAX_CMD_PIXELS		255
 
@@ -89,8 +90,8 @@ static void udl_compress_hline16(
 		const u8 *cmd_pixel_start, *cmd_pixel_end = NULL;
 		uint16_t pixel_val16;
 
-		*cmd++ = 0xaf;
-		*cmd++ = 0x6b;
+		*cmd++ = UDL_MSG_BULK;
+		*cmd++ = UDL_CMD_WRITERLX16;
 		*cmd++ = (uint8_t) ((dev_addr >> 16) & 0xFF);
 		*cmd++ = (uint8_t) ((dev_addr >> 8) & 0xFF);
 		*cmd++ = (uint8_t) ((dev_addr) & 0xFF);
@@ -152,7 +153,7 @@ static void udl_compress_hline16(
 	if (cmd_buffer_end <= MIN_RLX_CMD_BYTES + cmd) {
 		/* Fill leftover bytes with no-ops */
 		if (cmd_buffer_end > cmd)
-			memset(cmd, 0xAF, cmd_buffer_end - cmd);
+			memset(cmd, UDL_MSG_BULK, cmd_buffer_end - cmd);
 		cmd = (uint8_t *) cmd_buffer_end;
 	}
 
-- 
2.37.3

Re: [PATCH 16/16] drm/udl: Add constants for commands

[Javier Martinez Canillas]

On 9/19/22 15:04, Thomas Zimmermann wrote:
> Add constants for the various commands that the driver can send to
> the device and update the respective helper functions. No functional
> changes.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat