[PATCH 0/7] drm/udl: Convert to simple-pipe helpers and clean up

[PATCH 0/7] drm/udl: Convert to simple-pipe helpers and clean up

[Thomas Zimmermann]

With only a single display pipeline and primary plane, udl is perfect
for simple-pipe helpers. Patches 1 to 3 do the convertion. This enables
atomic modesetting for udl devices.

Patches 4 to 7 clean up handling of color depth and framebuffers. With
universal planes that come with simple pipe, display updates can be
implemented with DRM's damage handlers. The primary plane's formats
array allows to export the correct preferred color depth. The original
value was choosen for maximum compatibility, but did not represent the
device's capability.

The patchset has been tested by running the fb console, X11 and Weston
on a DisplayLink adapter.

Thomas Zimmermann (7):
  drm/udl: Init connector before encoder and CRTC
  drm/udl: Convert to struct drm_simple_display_pipe
  drm/udl: Remove unused encoder and CRTC code
  drm/udl: Set preferred color depth to 16 bpp
  drm/udl: Convert to drm_atomic_helper_dirtyfb()
  drm/udl: Remove struct udl_device.active_fb_16
  drm/udl: Move udl_handle_damage() into udl_modeset.c

 drivers/gpu/drm/udl/Makefile        |   2 +-
 drivers/gpu/drm/udl/udl_connector.c |  19 +-
 drivers/gpu/drm/udl/udl_drv.c       |  11 +-
 drivers/gpu/drm/udl/udl_drv.h       |  19 +-
 drivers/gpu/drm/udl/udl_encoder.c   |  70 -------
 drivers/gpu/drm/udl/udl_fb.c        | 216 ----------------------
 drivers/gpu/drm/udl/udl_main.c      |   3 -
 drivers/gpu/drm/udl/udl_modeset.c   | 276 +++++++++++++++++-----------
 8 files changed, 187 insertions(+), 429 deletions(-)
 delete mode 100644 drivers/gpu/drm/udl/udl_encoder.c
 delete mode 100644 drivers/gpu/drm/udl/udl_fb.c

--
2.23.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/7] drm/udl: Init connector before encoder and CRTC

[Thomas Zimmermann]

To mimic simple-pipe, we initialize the connector before the rest of
the display pipeline.

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

diff --git a/drivers/gpu/drm/udl/udl_connector.c b/drivers/gpu/drm/udl/udl_connector.c
index b4ae3e89a7b4..68e113ae44f7 100644
--- a/drivers/gpu/drm/udl/udl_connector.c
+++ b/drivers/gpu/drm/udl/udl_connector.c
@@ -123,14 +123,14 @@ static const struct drm_connector_funcs udl_connector_funcs = {
 	.set_property = udl_connector_set_property,
 };
 
-int udl_connector_init(struct drm_device *dev, struct drm_encoder *encoder)
+struct drm_connector *udl_connector_init(struct drm_device *dev)
 {
 	struct udl_drm_connector *udl_connector;
 	struct drm_connector *connector;
 
 	udl_connector = kzalloc(sizeof(struct udl_drm_connector), GFP_KERNEL);
 	if (!udl_connector)
-		return -ENOMEM;
+		return ERR_PTR(-ENOMEM);
 
 	connector = &udl_connector->connector;
 	drm_connector_init(dev, connector, &udl_connector_funcs,
@@ -138,9 +138,8 @@ int udl_connector_init(struct drm_device *dev, struct drm_encoder *encoder)
 	drm_connector_helper_add(connector, &udl_connector_helper_funcs);
 
 	drm_connector_register(connector);
-	drm_connector_attach_encoder(connector, encoder);
 	connector->polled = DRM_CONNECTOR_POLL_HPD |
 		DRM_CONNECTOR_POLL_CONNECT | DRM_CONNECTOR_POLL_DISCONNECT;
 
-	return 0;
+	return connector;
 }
diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
index 66cbe04f832a..8dc04717abac 100644
--- a/drivers/gpu/drm/udl/udl_drv.h
+++ b/drivers/gpu/drm/udl/udl_drv.h
@@ -78,7 +78,7 @@ struct udl_device {
 int udl_modeset_init(struct drm_device *dev);
 void udl_modeset_restore(struct drm_device *dev);
 void udl_modeset_cleanup(struct drm_device *dev);
-int udl_connector_init(struct drm_device *dev, struct drm_encoder *encoder);
+struct drm_connector *udl_connector_init(struct drm_device *dev);
 
 struct drm_encoder *udl_encoder_init(struct drm_device *dev);
 
diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
index 91af25caed64..5bb1522036c7 100644
--- a/drivers/gpu/drm/udl/udl_modeset.c
+++ b/drivers/gpu/drm/udl/udl_modeset.c
@@ -421,7 +421,10 @@ static const struct drm_mode_config_funcs udl_mode_funcs = {
 
 int udl_modeset_init(struct drm_device *dev)
 {
+	struct drm_connector *connector;
 	struct drm_encoder *encoder;
+	int ret;
+
 	drm_mode_config_init(dev);
 
 	dev->mode_config.min_width = 640;
@@ -435,13 +438,22 @@ int udl_modeset_init(struct drm_device *dev)
 
 	dev->mode_config.funcs = &udl_mode_funcs;
 
+	connector = udl_connector_init(dev);
+	if (IS_ERR(connector)) {
+		ret = PTR_ERR(connector);
+		goto err_drm_mode_config_cleanup;
+	}
+
 	udl_crtc_init(dev);
 
 	encoder = udl_encoder_init(dev);
-
-	udl_connector_init(dev, encoder);
+	drm_connector_attach_encoder(connector, encoder);
 
 	return 0;
+
+err_drm_mode_config_cleanup:
+	drm_mode_config_cleanup(dev);
+	return ret;
 }
 
 void udl_modeset_restore(struct drm_device *dev)
-- 
2.23.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/7] drm/udl: Init connector before encoder and CRTC

[Daniel Vetter]

On Tue, Nov 26, 2019 at 02:47:01PM +0100, Thomas Zimmermann wrote:
> To mimic simple-pipe, we initialize the connector before the rest of
> the display pipeline.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/udl/udl_connector.c |  7 +++----
>  drivers/gpu/drm/udl/udl_drv.h       |  2 +-
>  drivers/gpu/drm/udl/udl_modeset.c   | 16 ++++++++++++++--
>  3 files changed, 18 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/udl/udl_connector.c b/drivers/gpu/drm/udl/udl_connector.c
> index b4ae3e89a7b4..68e113ae44f7 100644
> --- a/drivers/gpu/drm/udl/udl_connector.c
> +++ b/drivers/gpu/drm/udl/udl_connector.c
> @@ -123,14 +123,14 @@ static const struct drm_connector_funcs udl_connector_funcs = {
>  	.set_property = udl_connector_set_property,
>  };
>  
> -int udl_connector_init(struct drm_device *dev, struct drm_encoder *encoder)
> +struct drm_connector *udl_connector_init(struct drm_device *dev)
>  {
>  	struct udl_drm_connector *udl_connector;
>  	struct drm_connector *connector;
>  
>  	udl_connector = kzalloc(sizeof(struct udl_drm_connector), GFP_KERNEL);
>  	if (!udl_connector)
> -		return -ENOMEM;
> +		return ERR_PTR(-ENOMEM);
>  
>  	connector = &udl_connector->connector;
>  	drm_connector_init(dev, connector, &udl_connector_funcs,
> @@ -138,9 +138,8 @@ int udl_connector_init(struct drm_device *dev, struct drm_encoder *encoder)
>  	drm_connector_helper_add(connector, &udl_connector_helper_funcs);
>  
>  	drm_connector_register(connector);

While at it, might want to ditch this line, it's a no-op and only meant to
be called by drivers for hotplugged connectors. But kinda separate patch,
plus there's an entire pile of drivers which get this wrong.

> -	drm_connector_attach_encoder(connector, encoder);
>  	connector->polled = DRM_CONNECTOR_POLL_HPD |
>  		DRM_CONNECTOR_POLL_CONNECT | DRM_CONNECTOR_POLL_DISCONNECT;
>  
> -	return 0;
> +	return connector;
>  }
> diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
> index 66cbe04f832a..8dc04717abac 100644
> --- a/drivers/gpu/drm/udl/udl_drv.h
> +++ b/drivers/gpu/drm/udl/udl_drv.h
> @@ -78,7 +78,7 @@ struct udl_device {
>  int udl_modeset_init(struct drm_device *dev);
>  void udl_modeset_restore(struct drm_device *dev);
>  void udl_modeset_cleanup(struct drm_device *dev);
> -int udl_connector_init(struct drm_device *dev, struct drm_encoder *encoder);
> +struct drm_connector *udl_connector_init(struct drm_device *dev);
>  
>  struct drm_encoder *udl_encoder_init(struct drm_device *dev);
>  
> diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
> index 91af25caed64..5bb1522036c7 100644
> --- a/drivers/gpu/drm/udl/udl_modeset.c
> +++ b/drivers/gpu/drm/udl/udl_modeset.c
> @@ -421,7 +421,10 @@ static const struct drm_mode_config_funcs udl_mode_funcs = {
>  
>  int udl_modeset_init(struct drm_device *dev)
>  {
> +	struct drm_connector *connector;
>  	struct drm_encoder *encoder;
> +	int ret;
> +
>  	drm_mode_config_init(dev);
>  
>  	dev->mode_config.min_width = 640;
> @@ -435,13 +438,22 @@ int udl_modeset_init(struct drm_device *dev)
>  
>  	dev->mode_config.funcs = &udl_mode_funcs;
>  
> +	connector = udl_connector_init(dev);
> +	if (IS_ERR(connector)) {
> +		ret = PTR_ERR(connector);
> +		goto err_drm_mode_config_cleanup;
> +	}
> +
>  	udl_crtc_init(dev);
>  
>  	encoder = udl_encoder_init(dev);
> -
> -	udl_connector_init(dev, encoder);
> +	drm_connector_attach_encoder(connector, encoder);
>  
>  	return 0;
> +
> +err_drm_mode_config_cleanup:
> +	drm_mode_config_cleanup(dev);
> +	return ret;
>  }
>  
>  void udl_modeset_restore(struct drm_device *dev)

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> -- 
> 2.23.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/7] drm/udl: Convert to struct drm_simple_display_pipe

[Thomas Zimmermann]

Udl has a single display pipeline with aprimary plane; perfect for
simple-pipe helpers. Convert it over. The old encoder and CRTC code
becomes unused and obsolete.

Exported formats for the primary plane are RGB565 and XRGB8888, with
the latter being emulated. The 16-bit format is the default and what
is used when communicating with the device.

This patch enables atomic modesetting for udl devices.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/udl/udl_connector.c |  12 +--
 drivers/gpu/drm/udl/udl_drv.c       |   9 +-
 drivers/gpu/drm/udl/udl_drv.h       |   4 +-
 drivers/gpu/drm/udl/udl_modeset.c   | 142 ++++++++++++++++++++++++----
 4 files changed, 136 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/udl/udl_connector.c b/drivers/gpu/drm/udl/udl_connector.c
index 68e113ae44f7..47ab8d150f1a 100644
--- a/drivers/gpu/drm/udl/udl_connector.c
+++ b/drivers/gpu/drm/udl/udl_connector.c
@@ -7,6 +7,7 @@
  * Copyright (C) 2009 Bernie Thompson <bernie@plugable.com>
  */
 
+#include <drm/drm_atomic_state_helper.h>
 #include <drm/drm_crtc_helper.h>
 #include <drm/drm_probe_helper.h>
 
@@ -90,13 +91,6 @@ udl_detect(struct drm_connector *connector, bool force)
 	return connector_status_connected;
 }
 
-static int udl_connector_set_property(struct drm_connector *connector,
-				      struct drm_property *property,
-				      uint64_t val)
-{
-	return 0;
-}
-
 static void udl_connector_destroy(struct drm_connector *connector)
 {
 	struct udl_drm_connector *udl_connector =
@@ -117,10 +111,12 @@ static const struct drm_connector_helper_funcs udl_connector_helper_funcs = {
 
 static const struct drm_connector_funcs udl_connector_funcs = {
 	.dpms = drm_helper_connector_dpms,
+	.reset = drm_atomic_helper_connector_reset,
 	.detect = udl_detect,
 	.fill_modes = drm_helper_probe_single_connector_modes,
 	.destroy = udl_connector_destroy,
-	.set_property = udl_connector_set_property,
+	.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)
diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c
index d5783fa32c5b..b3fa6bf41acc 100644
--- a/drivers/gpu/drm/udl/udl_drv.c
+++ b/drivers/gpu/drm/udl/udl_drv.c
@@ -21,17 +21,14 @@ static int udl_usb_suspend(struct usb_interface *interface,
 {
 	struct drm_device *dev = usb_get_intfdata(interface);
 
-	drm_kms_helper_poll_disable(dev);
-	return 0;
+	return drm_mode_config_helper_suspend(dev);
 }
 
 static int udl_usb_resume(struct usb_interface *interface)
 {
 	struct drm_device *dev = usb_get_intfdata(interface);
 
-	drm_kms_helper_poll_enable(dev);
-	udl_modeset_restore(dev);
-	return 0;
+	return drm_mode_config_helper_resume(dev);
 }
 
 DEFINE_DRM_GEM_FOPS(udl_driver_fops);
@@ -45,7 +42,7 @@ static void udl_driver_release(struct drm_device *dev)
 }
 
 static struct drm_driver driver = {
-	.driver_features = DRIVER_MODESET | DRIVER_GEM,
+	.driver_features = DRIVER_ATOMIC | DRIVER_GEM | DRIVER_MODESET,
 	.release = udl_driver_release,
 
 	/* gem hooks */
diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
index 8dc04717abac..23346bdc74bc 100644
--- a/drivers/gpu/drm/udl/udl_drv.h
+++ b/drivers/gpu/drm/udl/udl_drv.h
@@ -17,6 +17,7 @@
 #include <drm/drm_device.h>
 #include <drm/drm_framebuffer.h>
 #include <drm/drm_gem.h>
+#include <drm/drm_simple_kms_helper.h>
 
 struct drm_encoder;
 struct drm_mode_create_dumb;
@@ -53,6 +54,8 @@ struct udl_device {
 	struct usb_device *udev;
 	struct drm_crtc *crtc;
 
+	struct drm_simple_display_pipe display_pipe;
+
 	/* active framebuffer on the 16-bit channel */
 	const struct drm_framebuffer *active_fb_16;
 	spinlock_t active_fb_16_lock;
@@ -76,7 +79,6 @@ struct udl_device {
 
 /* modeset */
 int udl_modeset_init(struct drm_device *dev);
-void udl_modeset_restore(struct drm_device *dev);
 void udl_modeset_cleanup(struct drm_device *dev);
 struct drm_connector *udl_connector_init(struct drm_device *dev);
 
diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
index 5bb1522036c7..c8bd438de6e9 100644
--- a/drivers/gpu/drm/udl/udl_modeset.c
+++ b/drivers/gpu/drm/udl/udl_modeset.c
@@ -9,12 +9,16 @@
 
  */
 
+#include <drm/drm_atomic_helper.h>
 #include <drm/drm_crtc_helper.h>
+#include <drm/drm_gem_framebuffer_helper.h>
 #include <drm/drm_modeset_helper_vtables.h>
 #include <drm/drm_vblank.h>
 
 #include "udl_drv.h"
 
+#define UDL_COLOR_DEPTH_16BPP	0
+
 /*
  * All DisplayLink bulk operations start with 0xAF, followed by specific code
  * All operations are written to buffers which then later get sent to device
@@ -415,14 +419,126 @@ static int udl_crtc_init(struct drm_device *dev)
 	return 0;
 }
 
+/*
+ * Simple display pipeline
+ */
+
+static const uint32_t udl_simple_display_pipe_formats[] = {
+	DRM_FORMAT_RGB565,
+	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,
+			       struct drm_plane_state *plane_state)
+{
+	struct drm_crtc *crtc = &pipe->crtc;
+	struct drm_device *dev = crtc->dev;
+	struct drm_framebuffer *fb = plane_state->fb;
+	struct udl_device *udl = dev->dev_private;
+	struct drm_display_mode *mode = &crtc_state->mode;
+	char *buf;
+	char *wrptr;
+	int color_depth = UDL_COLOR_DEPTH_16BPP;
+
+	udl->crtc = crtc;
+
+	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.
+	 */
+	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(wrptr, DRM_MODE_DPMS_ON);
+	wrptr = udl_vidreg_unlock(wrptr);
+
+	wrptr = udl_dummy_render(wrptr);
+
+	spin_lock(&udl->active_fb_16_lock);
+	udl->active_fb_16 = fb;
+	spin_unlock(&udl->active_fb_16_lock);
+	udl->mode_buf_len = wrptr - buf;
+
+	udl_handle_damage(fb, 0, 0, fb->width, fb->height);
+
+	udl_crtc_dpms(&pipe->crtc, DRM_MODE_DPMS_ON);
+
+	crtc_state->no_vblank = true;
+}
+
+static void
+udl_simple_display_pipe_disable(struct drm_simple_display_pipe *pipe)
+{
+	udl_crtc_dpms(&pipe->crtc, DRM_MODE_DPMS_OFF);
+}
+
+static int
+udl_simple_display_pipe_check(struct drm_simple_display_pipe *pipe,
+			      struct drm_plane_state *plane_state,
+			      struct drm_crtc_state *crtc_state)
+{
+	return 0;
+}
+
+static void
+udl_simple_display_pipe_update(struct drm_simple_display_pipe *pipe,
+			       struct drm_plane_state *old_plane_state)
+{
+	struct drm_device *dev = pipe->crtc.dev;
+	struct udl_device *udl = dev->dev_private;
+	struct drm_framebuffer *fb = pipe->plane.state->fb;
+
+	spin_lock(&udl->active_fb_16_lock);
+	udl->active_fb_16 = fb;
+	spin_unlock(&udl->active_fb_16_lock);
+
+	if (!fb)
+		return;
+
+	udl_handle_damage(fb, 0, 0, fb->width, fb->height);
+}
+
+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,
+	.check = udl_simple_display_pipe_check,
+	.update = udl_simple_display_pipe_update,
+	.prepare_fb = drm_gem_fb_simple_display_pipe_prepare_fb,
+};
+
+/*
+ * Modesetting
+ */
+
 static const struct drm_mode_config_funcs udl_mode_funcs = {
 	.fb_create = udl_fb_user_fb_create,
+	.atomic_check  = drm_atomic_helper_check,
+	.atomic_commit = drm_atomic_helper_commit,
 };
 
 int udl_modeset_init(struct drm_device *dev)
 {
+	size_t format_count = ARRAY_SIZE(udl_simple_display_pipe_formats);
+	struct udl_device *udl = dev->dev_private;
 	struct drm_connector *connector;
-	struct drm_encoder *encoder;
 	int ret;
 
 	drm_mode_config_init(dev);
@@ -444,10 +560,16 @@ int udl_modeset_init(struct drm_device *dev)
 		goto err_drm_mode_config_cleanup;
 	}
 
-	udl_crtc_init(dev);
+	format_count = ARRAY_SIZE(udl_simple_display_pipe_formats);
 
-	encoder = udl_encoder_init(dev);
-	drm_connector_attach_encoder(connector, encoder);
+	ret = drm_simple_display_pipe_init(dev, &udl->display_pipe,
+					   &udl_simple_display_pipe_funcs,
+					   udl_simple_display_pipe_formats,
+					   format_count, NULL, connector);
+	if (ret)
+		goto err_drm_mode_config_cleanup;
+
+	drm_mode_config_reset(dev);
 
 	return 0;
 
@@ -456,18 +578,6 @@ int udl_modeset_init(struct drm_device *dev)
 	return ret;
 }
 
-void udl_modeset_restore(struct drm_device *dev)
-{
-	struct udl_device *udl = dev->dev_private;
-	struct drm_framebuffer *fb;
-
-	if (!udl->crtc || !udl->crtc->primary->fb)
-		return;
-	udl_crtc_commit(udl->crtc);
-	fb = udl->crtc->primary->fb;
-	udl_handle_damage(fb, 0, 0, fb->width, fb->height);
-}
-
 void udl_modeset_cleanup(struct drm_device *dev)
 {
 	drm_mode_config_cleanup(dev);
-- 
2.23.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/7] drm/udl: Convert to struct drm_simple_display_pipe

[Daniel Vetter]

On Tue, Nov 26, 2019 at 02:47:02PM +0100, Thomas Zimmermann wrote:
> Udl has a single display pipeline with aprimary plane; perfect for
> simple-pipe helpers. Convert it over. The old encoder and CRTC code
> becomes unused and obsolete.
> 
> Exported formats for the primary plane are RGB565 and XRGB8888, with
> the latter being emulated. The 16-bit format is the default and what
> is used when communicating with the device.
> 
> This patch enables atomic modesetting for udl devices.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/udl/udl_connector.c |  12 +--
>  drivers/gpu/drm/udl/udl_drv.c       |   9 +-
>  drivers/gpu/drm/udl/udl_drv.h       |   4 +-
>  drivers/gpu/drm/udl/udl_modeset.c   | 142 ++++++++++++++++++++++++----
>  4 files changed, 136 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/gpu/drm/udl/udl_connector.c b/drivers/gpu/drm/udl/udl_connector.c
> index 68e113ae44f7..47ab8d150f1a 100644
> --- a/drivers/gpu/drm/udl/udl_connector.c
> +++ b/drivers/gpu/drm/udl/udl_connector.c
> @@ -7,6 +7,7 @@
>   * Copyright (C) 2009 Bernie Thompson <bernie@plugable.com>
>   */
>  
> +#include <drm/drm_atomic_state_helper.h>
>  #include <drm/drm_crtc_helper.h>
>  #include <drm/drm_probe_helper.h>
>  
> @@ -90,13 +91,6 @@ udl_detect(struct drm_connector *connector, bool force)
>  	return connector_status_connected;
>  }
>  
> -static int udl_connector_set_property(struct drm_connector *connector,
> -				      struct drm_property *property,
> -				      uint64_t val)
> -{
> -	return 0;
> -}
> -
>  static void udl_connector_destroy(struct drm_connector *connector)
>  {
>  	struct udl_drm_connector *udl_connector =
> @@ -117,10 +111,12 @@ static const struct drm_connector_helper_funcs udl_connector_helper_funcs = {
>  
>  static const struct drm_connector_funcs udl_connector_funcs = {
>  	.dpms = drm_helper_connector_dpms,
> +	.reset = drm_atomic_helper_connector_reset,
>  	.detect = udl_detect,
>  	.fill_modes = drm_helper_probe_single_connector_modes,
>  	.destroy = udl_connector_destroy,
> -	.set_property = udl_connector_set_property,
> +	.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)
> diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c
> index d5783fa32c5b..b3fa6bf41acc 100644
> --- a/drivers/gpu/drm/udl/udl_drv.c
> +++ b/drivers/gpu/drm/udl/udl_drv.c
> @@ -21,17 +21,14 @@ static int udl_usb_suspend(struct usb_interface *interface,
>  {
>  	struct drm_device *dev = usb_get_intfdata(interface);
>  
> -	drm_kms_helper_poll_disable(dev);
> -	return 0;
> +	return drm_mode_config_helper_suspend(dev);
>  }
>  
>  static int udl_usb_resume(struct usb_interface *interface)
>  {
>  	struct drm_device *dev = usb_get_intfdata(interface);
>  
> -	drm_kms_helper_poll_enable(dev);
> -	udl_modeset_restore(dev);
> -	return 0;
> +	return drm_mode_config_helper_resume(dev);

Please mention in the commit message that you're also switching over to
the atomic-based generic suspend/resume helpers. Might even want to split
that out as a separate patch (but will require minor adjustements in
udl_modeset_restore).


>  }
>  
>  DEFINE_DRM_GEM_FOPS(udl_driver_fops);
> @@ -45,7 +42,7 @@ static void udl_driver_release(struct drm_device *dev)
>  }
>  
>  static struct drm_driver driver = {
> -	.driver_features = DRIVER_MODESET | DRIVER_GEM,
> +	.driver_features = DRIVER_ATOMIC | DRIVER_GEM | DRIVER_MODESET,
>  	.release = udl_driver_release,
>  
>  	/* gem hooks */
> diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
> index 8dc04717abac..23346bdc74bc 100644
> --- a/drivers/gpu/drm/udl/udl_drv.h
> +++ b/drivers/gpu/drm/udl/udl_drv.h
> @@ -17,6 +17,7 @@
>  #include <drm/drm_device.h>
>  #include <drm/drm_framebuffer.h>
>  #include <drm/drm_gem.h>
> +#include <drm/drm_simple_kms_helper.h>
>  
>  struct drm_encoder;
>  struct drm_mode_create_dumb;
> @@ -53,6 +54,8 @@ struct udl_device {
>  	struct usb_device *udev;
>  	struct drm_crtc *crtc;
>  
> +	struct drm_simple_display_pipe display_pipe;
> +
>  	/* active framebuffer on the 16-bit channel */
>  	const struct drm_framebuffer *active_fb_16;
>  	spinlock_t active_fb_16_lock;
> @@ -76,7 +79,6 @@ struct udl_device {
>  
>  /* modeset */
>  int udl_modeset_init(struct drm_device *dev);
> -void udl_modeset_restore(struct drm_device *dev);
>  void udl_modeset_cleanup(struct drm_device *dev);
>  struct drm_connector *udl_connector_init(struct drm_device *dev);
>  
> diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
> index 5bb1522036c7..c8bd438de6e9 100644
> --- a/drivers/gpu/drm/udl/udl_modeset.c
> +++ b/drivers/gpu/drm/udl/udl_modeset.c
> @@ -9,12 +9,16 @@
>  
>   */
>  
> +#include <drm/drm_atomic_helper.h>
>  #include <drm/drm_crtc_helper.h>
> +#include <drm/drm_gem_framebuffer_helper.h>
>  #include <drm/drm_modeset_helper_vtables.h>
>  #include <drm/drm_vblank.h>
>  
>  #include "udl_drv.h"
>  
> +#define UDL_COLOR_DEPTH_16BPP	0
> +
>  /*
>   * All DisplayLink bulk operations start with 0xAF, followed by specific code
>   * All operations are written to buffers which then later get sent to device
> @@ -415,14 +419,126 @@ static int udl_crtc_init(struct drm_device *dev)
>  	return 0;
>  }
>  
> +/*
> + * Simple display pipeline
> + */
> +
> +static const uint32_t udl_simple_display_pipe_formats[] = {
> +	DRM_FORMAT_RGB565,
> +	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,
> +			       struct drm_plane_state *plane_state)
> +{
> +	struct drm_crtc *crtc = &pipe->crtc;
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_framebuffer *fb = plane_state->fb;
> +	struct udl_device *udl = dev->dev_private;
> +	struct drm_display_mode *mode = &crtc_state->mode;
> +	char *buf;
> +	char *wrptr;
> +	int color_depth = UDL_COLOR_DEPTH_16BPP;
> +
> +	udl->crtc = crtc;
> +
> +	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.
> +	 */
> +	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(wrptr, DRM_MODE_DPMS_ON);
> +	wrptr = udl_vidreg_unlock(wrptr);
> +
> +	wrptr = udl_dummy_render(wrptr);
> +
> +	spin_lock(&udl->active_fb_16_lock);
> +	udl->active_fb_16 = fb;
> +	spin_unlock(&udl->active_fb_16_lock);
> +	udl->mode_buf_len = wrptr - buf;
> +
> +	udl_handle_damage(fb, 0, 0, fb->width, fb->height);
> +
> +	udl_crtc_dpms(&pipe->crtc, DRM_MODE_DPMS_ON);

Having a _dpms() function is very much legacy-modeset style. I think much
cleaner if you'd just inline the relevant part of the function here and
below.

> +
> +	crtc_state->no_vblank = true;
> +}
> +
> +static void
> +udl_simple_display_pipe_disable(struct drm_simple_display_pipe *pipe)
> +{
> +	udl_crtc_dpms(&pipe->crtc, DRM_MODE_DPMS_OFF);
> +}
> +
> +static int
> +udl_simple_display_pipe_check(struct drm_simple_display_pipe *pipe,
> +			      struct drm_plane_state *plane_state,
> +			      struct drm_crtc_state *crtc_state)
> +{
> +	return 0;
> +}
> +
> +static void
> +udl_simple_display_pipe_update(struct drm_simple_display_pipe *pipe,
> +			       struct drm_plane_state *old_plane_state)
> +{
> +	struct drm_device *dev = pipe->crtc.dev;
> +	struct udl_device *udl = dev->dev_private;
> +	struct drm_framebuffer *fb = pipe->plane.state->fb;
> +
> +	spin_lock(&udl->active_fb_16_lock);
> +	udl->active_fb_16 = fb;
> +	spin_unlock(&udl->active_fb_16_lock);
> +
> +	if (!fb)
> +		return;
> +
> +	udl_handle_damage(fb, 0, 0, fb->width, fb->height);
> +}
> +
> +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,
> +	.check = udl_simple_display_pipe_check,
> +	.update = udl_simple_display_pipe_update,
> +	.prepare_fb = drm_gem_fb_simple_display_pipe_prepare_fb,
> +};
> +
> +/*
> + * Modesetting
> + */
> +
>  static const struct drm_mode_config_funcs udl_mode_funcs = {
>  	.fb_create = udl_fb_user_fb_create,
> +	.atomic_check  = drm_atomic_helper_check,
> +	.atomic_commit = drm_atomic_helper_commit,
>  };
>  
>  int udl_modeset_init(struct drm_device *dev)
>  {
> +	size_t format_count = ARRAY_SIZE(udl_simple_display_pipe_formats);
> +	struct udl_device *udl = dev->dev_private;
>  	struct drm_connector *connector;
> -	struct drm_encoder *encoder;
>  	int ret;
>  
>  	drm_mode_config_init(dev);
> @@ -444,10 +560,16 @@ int udl_modeset_init(struct drm_device *dev)
>  		goto err_drm_mode_config_cleanup;
>  	}
>  
> -	udl_crtc_init(dev);
> +	format_count = ARRAY_SIZE(udl_simple_display_pipe_formats);
>  
> -	encoder = udl_encoder_init(dev);
> -	drm_connector_attach_encoder(connector, encoder);
> +	ret = drm_simple_display_pipe_init(dev, &udl->display_pipe,
> +					   &udl_simple_display_pipe_funcs,
> +					   udl_simple_display_pipe_formats,
> +					   format_count, NULL, connector);
> +	if (ret)
> +		goto err_drm_mode_config_cleanup;
> +
> +	drm_mode_config_reset(dev);
>  
>  	return 0;
>  
> @@ -456,18 +578,6 @@ int udl_modeset_init(struct drm_device *dev)
>  	return ret;
>  }
>  
> -void udl_modeset_restore(struct drm_device *dev)
> -{
> -	struct udl_device *udl = dev->dev_private;
> -	struct drm_framebuffer *fb;
> -
> -	if (!udl->crtc || !udl->crtc->primary->fb)
> -		return;
> -	udl_crtc_commit(udl->crtc);
> -	fb = udl->crtc->primary->fb;
> -	udl_handle_damage(fb, 0, 0, fb->width, fb->height);
> -}
> -
>  void udl_modeset_cleanup(struct drm_device *dev)
>  {
>  	drm_mode_config_cleanup(dev);

Where's all the deleted code that removes the udl crtc and encoder
code/structs?

Cheers, Daniel

> -- 
> 2.23.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/7] drm/udl: Remove unused encoder and CRTC code

[Thomas Zimmermann]

The removed functionality is provided by simple-pipe helpers.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/udl/Makefile      |   2 +-
 drivers/gpu/drm/udl/udl_drv.h     |   3 -
 drivers/gpu/drm/udl/udl_encoder.c |  70 ---------------
 drivers/gpu/drm/udl/udl_modeset.c | 138 ------------------------------
 4 files changed, 1 insertion(+), 212 deletions(-)
 delete mode 100644 drivers/gpu/drm/udl/udl_encoder.c

diff --git a/drivers/gpu/drm/udl/Makefile b/drivers/gpu/drm/udl/Makefile
index 9c42820ae33d..177ce74f4cf4 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_encoder.o udl_main.o udl_fb.o udl_transfer.o udl_gem.o
+udl-y := udl_drv.o udl_modeset.o udl_connector.o udl_main.o udl_fb.o udl_transfer.o udl_gem.o
 
 obj-$(CONFIG_DRM_UDL) := udl.o
diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
index 23346bdc74bc..77b57d6abd23 100644
--- a/drivers/gpu/drm/udl/udl_drv.h
+++ b/drivers/gpu/drm/udl/udl_drv.h
@@ -19,7 +19,6 @@
 #include <drm/drm_gem.h>
 #include <drm/drm_simple_kms_helper.h>
 
-struct drm_encoder;
 struct drm_mode_create_dumb;
 
 #define DRIVER_NAME		"udl"
@@ -82,8 +81,6 @@ int udl_modeset_init(struct drm_device *dev);
 void udl_modeset_cleanup(struct drm_device *dev);
 struct drm_connector *udl_connector_init(struct drm_device *dev);
 
-struct drm_encoder *udl_encoder_init(struct drm_device *dev);
-
 struct urb *udl_get_urb(struct drm_device *dev);
 
 int udl_submit_urb(struct drm_device *dev, struct urb *urb, size_t len);
diff --git a/drivers/gpu/drm/udl/udl_encoder.c b/drivers/gpu/drm/udl/udl_encoder.c
deleted file mode 100644
index 203f041e737c..000000000000
--- a/drivers/gpu/drm/udl/udl_encoder.c
+++ /dev/null
@@ -1,70 +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_encoder.h>
-#include <drm/drm_modeset_helper_vtables.h>
-
-#include "udl_drv.h"
-
-/* dummy encoder */
-static void udl_enc_destroy(struct drm_encoder *encoder)
-{
-	drm_encoder_cleanup(encoder);
-	kfree(encoder);
-}
-
-static void udl_encoder_disable(struct drm_encoder *encoder)
-{
-}
-
-static void udl_encoder_prepare(struct drm_encoder *encoder)
-{
-}
-
-static void udl_encoder_commit(struct drm_encoder *encoder)
-{
-}
-
-static void udl_encoder_mode_set(struct drm_encoder *encoder,
-				 struct drm_display_mode *mode,
-				 struct drm_display_mode *adjusted_mode)
-{
-}
-
-static void
-udl_encoder_dpms(struct drm_encoder *encoder, int mode)
-{
-}
-
-static const struct drm_encoder_helper_funcs udl_helper_funcs = {
-	.dpms = udl_encoder_dpms,
-	.prepare = udl_encoder_prepare,
-	.mode_set = udl_encoder_mode_set,
-	.commit = udl_encoder_commit,
-	.disable = udl_encoder_disable,
-};
-
-static const struct drm_encoder_funcs udl_enc_funcs = {
-	.destroy = udl_enc_destroy,
-};
-
-struct drm_encoder *udl_encoder_init(struct drm_device *dev)
-{
-	struct drm_encoder *encoder;
-
-	encoder = kzalloc(sizeof(struct drm_encoder), GFP_KERNEL);
-	if (!encoder)
-		return NULL;
-
-	drm_encoder_init(dev, encoder, &udl_enc_funcs, DRM_MODE_ENCODER_TMDS,
-			 NULL);
-	drm_encoder_helper_add(encoder, &udl_helper_funcs);
-	encoder->possible_crtcs = 1;
-	return encoder;
-}
diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
index c8bd438de6e9..72884cbda131 100644
--- a/drivers/gpu/drm/udl/udl_modeset.c
+++ b/drivers/gpu/drm/udl/udl_modeset.c
@@ -281,144 +281,6 @@ static void udl_crtc_dpms(struct drm_crtc *crtc, int mode)
 
 }
 
-#if 0
-static int
-udl_pipe_set_base_atomic(struct drm_crtc *crtc, struct drm_framebuffer *fb,
-			   int x, int y, enum mode_set_atomic state)
-{
-	return 0;
-}
-
-static int
-udl_pipe_set_base(struct drm_crtc *crtc, int x, int y,
-		    struct drm_framebuffer *old_fb)
-{
-	return 0;
-}
-#endif
-
-static int udl_crtc_mode_set(struct drm_crtc *crtc,
-			       struct drm_display_mode *mode,
-			       struct drm_display_mode *adjusted_mode,
-			       int x, int y,
-			       struct drm_framebuffer *old_fb)
-
-{
-	struct drm_device *dev = crtc->dev;
-	struct drm_framebuffer *fb = crtc->primary->fb;
-	struct udl_device *udl = dev->dev_private;
-	char *buf;
-	char *wrptr;
-	int color_depth = 0;
-
-	udl->crtc = crtc;
-
-	buf = (char *)udl->mode_buf;
-
-	/* for now we just clip 24 -> 16 - if we fix that fix this */
-	/*if  (crtc->fb->bits_per_pixel != 16)
-	  color_depth = 1; */
-
-	/* 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, adjusted_mode);
-	wrptr = udl_set_blank(wrptr, DRM_MODE_DPMS_ON);
-	wrptr = udl_vidreg_unlock(wrptr);
-
-	wrptr = udl_dummy_render(wrptr);
-
-	spin_lock(&udl->active_fb_16_lock);
-	udl->active_fb_16 = fb;
-	spin_unlock(&udl->active_fb_16_lock);
-	udl->mode_buf_len = wrptr - buf;
-
-	/* damage all of it */
-	udl_handle_damage(fb, 0, 0, fb->width, fb->height);
-	return 0;
-}
-
-
-static void udl_crtc_disable(struct drm_crtc *crtc)
-{
-	udl_crtc_dpms(crtc, DRM_MODE_DPMS_OFF);
-}
-
-static void udl_crtc_destroy(struct drm_crtc *crtc)
-{
-	drm_crtc_cleanup(crtc);
-	kfree(crtc);
-}
-
-static int udl_crtc_page_flip(struct drm_crtc *crtc,
-			      struct drm_framebuffer *fb,
-			      struct drm_pending_vblank_event *event,
-			      uint32_t page_flip_flags,
-			      struct drm_modeset_acquire_ctx *ctx)
-{
-	struct drm_device *dev = crtc->dev;
-	struct udl_device *udl = dev->dev_private;
-
-	spin_lock(&udl->active_fb_16_lock);
-	udl->active_fb_16 = fb;
-	spin_unlock(&udl->active_fb_16_lock);
-
-	udl_handle_damage(fb, 0, 0, fb->width, fb->height);
-
-	spin_lock_irq(&dev->event_lock);
-	if (event)
-		drm_crtc_send_vblank_event(crtc, event);
-	spin_unlock_irq(&dev->event_lock);
-	crtc->primary->fb = fb;
-
-	return 0;
-}
-
-static void udl_crtc_prepare(struct drm_crtc *crtc)
-{
-}
-
-static void udl_crtc_commit(struct drm_crtc *crtc)
-{
-	udl_crtc_dpms(crtc, DRM_MODE_DPMS_ON);
-}
-
-static const struct drm_crtc_helper_funcs udl_helper_funcs = {
-	.dpms = udl_crtc_dpms,
-	.mode_set = udl_crtc_mode_set,
-	.prepare = udl_crtc_prepare,
-	.commit = udl_crtc_commit,
-	.disable = udl_crtc_disable,
-};
-
-static const struct drm_crtc_funcs udl_crtc_funcs = {
-	.set_config = drm_crtc_helper_set_config,
-	.destroy = udl_crtc_destroy,
-	.page_flip = udl_crtc_page_flip,
-};
-
-static int udl_crtc_init(struct drm_device *dev)
-{
-	struct drm_crtc *crtc;
-
-	crtc = kzalloc(sizeof(struct drm_crtc) + sizeof(struct drm_connector *), GFP_KERNEL);
-	if (crtc == NULL)
-		return -ENOMEM;
-
-	drm_crtc_init(dev, crtc, &udl_crtc_funcs);
-	drm_crtc_helper_add(crtc, &udl_helper_funcs);
-
-	return 0;
-}
-
 /*
  * Simple display pipeline
  */
-- 
2.23.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/7] drm/udl: Remove unused encoder and CRTC code

[Daniel Vetter]

On Tue, Nov 26, 2019 at 02:47:03PM +0100, Thomas Zimmermann wrote:
> The removed functionality is provided by simple-pipe helpers.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>

Needs to be squashed into the previous patch, ideally with the new
functions in the same place as the old ones (as much as possible), so that
diff reviewing is easier. If you feel like that makes the previous patch
too big, split out the suspend/resume refactor.
-Daniel

> ---
>  drivers/gpu/drm/udl/Makefile      |   2 +-
>  drivers/gpu/drm/udl/udl_drv.h     |   3 -
>  drivers/gpu/drm/udl/udl_encoder.c |  70 ---------------
>  drivers/gpu/drm/udl/udl_modeset.c | 138 ------------------------------
>  4 files changed, 1 insertion(+), 212 deletions(-)
>  delete mode 100644 drivers/gpu/drm/udl/udl_encoder.c
> 
> diff --git a/drivers/gpu/drm/udl/Makefile b/drivers/gpu/drm/udl/Makefile
> index 9c42820ae33d..177ce74f4cf4 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_encoder.o udl_main.o udl_fb.o udl_transfer.o udl_gem.o
> +udl-y := udl_drv.o udl_modeset.o udl_connector.o udl_main.o udl_fb.o udl_transfer.o udl_gem.o
>  
>  obj-$(CONFIG_DRM_UDL) := udl.o
> diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
> index 23346bdc74bc..77b57d6abd23 100644
> --- a/drivers/gpu/drm/udl/udl_drv.h
> +++ b/drivers/gpu/drm/udl/udl_drv.h
> @@ -19,7 +19,6 @@
>  #include <drm/drm_gem.h>
>  #include <drm/drm_simple_kms_helper.h>
>  
> -struct drm_encoder;
>  struct drm_mode_create_dumb;
>  
>  #define DRIVER_NAME		"udl"
> @@ -82,8 +81,6 @@ int udl_modeset_init(struct drm_device *dev);
>  void udl_modeset_cleanup(struct drm_device *dev);
>  struct drm_connector *udl_connector_init(struct drm_device *dev);
>  
> -struct drm_encoder *udl_encoder_init(struct drm_device *dev);
> -
>  struct urb *udl_get_urb(struct drm_device *dev);
>  
>  int udl_submit_urb(struct drm_device *dev, struct urb *urb, size_t len);
> diff --git a/drivers/gpu/drm/udl/udl_encoder.c b/drivers/gpu/drm/udl/udl_encoder.c
> deleted file mode 100644
> index 203f041e737c..000000000000
> --- a/drivers/gpu/drm/udl/udl_encoder.c
> +++ /dev/null
> @@ -1,70 +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_encoder.h>
> -#include <drm/drm_modeset_helper_vtables.h>
> -
> -#include "udl_drv.h"
> -
> -/* dummy encoder */
> -static void udl_enc_destroy(struct drm_encoder *encoder)
> -{
> -	drm_encoder_cleanup(encoder);
> -	kfree(encoder);
> -}
> -
> -static void udl_encoder_disable(struct drm_encoder *encoder)
> -{
> -}
> -
> -static void udl_encoder_prepare(struct drm_encoder *encoder)
> -{
> -}
> -
> -static void udl_encoder_commit(struct drm_encoder *encoder)
> -{
> -}
> -
> -static void udl_encoder_mode_set(struct drm_encoder *encoder,
> -				 struct drm_display_mode *mode,
> -				 struct drm_display_mode *adjusted_mode)
> -{
> -}
> -
> -static void
> -udl_encoder_dpms(struct drm_encoder *encoder, int mode)
> -{
> -}
> -
> -static const struct drm_encoder_helper_funcs udl_helper_funcs = {
> -	.dpms = udl_encoder_dpms,
> -	.prepare = udl_encoder_prepare,
> -	.mode_set = udl_encoder_mode_set,
> -	.commit = udl_encoder_commit,
> -	.disable = udl_encoder_disable,
> -};
> -
> -static const struct drm_encoder_funcs udl_enc_funcs = {
> -	.destroy = udl_enc_destroy,
> -};
> -
> -struct drm_encoder *udl_encoder_init(struct drm_device *dev)
> -{
> -	struct drm_encoder *encoder;
> -
> -	encoder = kzalloc(sizeof(struct drm_encoder), GFP_KERNEL);
> -	if (!encoder)
> -		return NULL;
> -
> -	drm_encoder_init(dev, encoder, &udl_enc_funcs, DRM_MODE_ENCODER_TMDS,
> -			 NULL);
> -	drm_encoder_helper_add(encoder, &udl_helper_funcs);
> -	encoder->possible_crtcs = 1;
> -	return encoder;
> -}
> diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
> index c8bd438de6e9..72884cbda131 100644
> --- a/drivers/gpu/drm/udl/udl_modeset.c
> +++ b/drivers/gpu/drm/udl/udl_modeset.c
> @@ -281,144 +281,6 @@ static void udl_crtc_dpms(struct drm_crtc *crtc, int mode)
>  
>  }
>  
> -#if 0
> -static int
> -udl_pipe_set_base_atomic(struct drm_crtc *crtc, struct drm_framebuffer *fb,
> -			   int x, int y, enum mode_set_atomic state)
> -{
> -	return 0;
> -}
> -
> -static int
> -udl_pipe_set_base(struct drm_crtc *crtc, int x, int y,
> -		    struct drm_framebuffer *old_fb)
> -{
> -	return 0;
> -}
> -#endif
> -
> -static int udl_crtc_mode_set(struct drm_crtc *crtc,
> -			       struct drm_display_mode *mode,
> -			       struct drm_display_mode *adjusted_mode,
> -			       int x, int y,
> -			       struct drm_framebuffer *old_fb)
> -
> -{
> -	struct drm_device *dev = crtc->dev;
> -	struct drm_framebuffer *fb = crtc->primary->fb;
> -	struct udl_device *udl = dev->dev_private;
> -	char *buf;
> -	char *wrptr;
> -	int color_depth = 0;
> -
> -	udl->crtc = crtc;
> -
> -	buf = (char *)udl->mode_buf;
> -
> -	/* for now we just clip 24 -> 16 - if we fix that fix this */
> -	/*if  (crtc->fb->bits_per_pixel != 16)
> -	  color_depth = 1; */
> -
> -	/* 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, adjusted_mode);
> -	wrptr = udl_set_blank(wrptr, DRM_MODE_DPMS_ON);
> -	wrptr = udl_vidreg_unlock(wrptr);
> -
> -	wrptr = udl_dummy_render(wrptr);
> -
> -	spin_lock(&udl->active_fb_16_lock);
> -	udl->active_fb_16 = fb;
> -	spin_unlock(&udl->active_fb_16_lock);
> -	udl->mode_buf_len = wrptr - buf;
> -
> -	/* damage all of it */
> -	udl_handle_damage(fb, 0, 0, fb->width, fb->height);
> -	return 0;
> -}
> -
> -
> -static void udl_crtc_disable(struct drm_crtc *crtc)
> -{
> -	udl_crtc_dpms(crtc, DRM_MODE_DPMS_OFF);
> -}
> -
> -static void udl_crtc_destroy(struct drm_crtc *crtc)
> -{
> -	drm_crtc_cleanup(crtc);
> -	kfree(crtc);
> -}
> -
> -static int udl_crtc_page_flip(struct drm_crtc *crtc,
> -			      struct drm_framebuffer *fb,
> -			      struct drm_pending_vblank_event *event,
> -			      uint32_t page_flip_flags,
> -			      struct drm_modeset_acquire_ctx *ctx)
> -{
> -	struct drm_device *dev = crtc->dev;
> -	struct udl_device *udl = dev->dev_private;
> -
> -	spin_lock(&udl->active_fb_16_lock);
> -	udl->active_fb_16 = fb;
> -	spin_unlock(&udl->active_fb_16_lock);
> -
> -	udl_handle_damage(fb, 0, 0, fb->width, fb->height);
> -
> -	spin_lock_irq(&dev->event_lock);
> -	if (event)
> -		drm_crtc_send_vblank_event(crtc, event);
> -	spin_unlock_irq(&dev->event_lock);
> -	crtc->primary->fb = fb;
> -
> -	return 0;
> -}
> -
> -static void udl_crtc_prepare(struct drm_crtc *crtc)
> -{
> -}
> -
> -static void udl_crtc_commit(struct drm_crtc *crtc)
> -{
> -	udl_crtc_dpms(crtc, DRM_MODE_DPMS_ON);
> -}
> -
> -static const struct drm_crtc_helper_funcs udl_helper_funcs = {
> -	.dpms = udl_crtc_dpms,
> -	.mode_set = udl_crtc_mode_set,
> -	.prepare = udl_crtc_prepare,
> -	.commit = udl_crtc_commit,
> -	.disable = udl_crtc_disable,
> -};
> -
> -static const struct drm_crtc_funcs udl_crtc_funcs = {
> -	.set_config = drm_crtc_helper_set_config,
> -	.destroy = udl_crtc_destroy,
> -	.page_flip = udl_crtc_page_flip,
> -};
> -
> -static int udl_crtc_init(struct drm_device *dev)
> -{
> -	struct drm_crtc *crtc;
> -
> -	crtc = kzalloc(sizeof(struct drm_crtc) + sizeof(struct drm_connector *), GFP_KERNEL);
> -	if (crtc == NULL)
> -		return -ENOMEM;
> -
> -	drm_crtc_init(dev, crtc, &udl_crtc_funcs);
> -	drm_crtc_helper_add(crtc, &udl_helper_funcs);
> -
> -	return 0;
> -}
> -
>  /*
>   * Simple display pipeline
>   */
> -- 
> 2.23.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 4/7] drm/udl: Set preferred color depth to 16 bpp

[Thomas Zimmermann]

The current default color depth of 24 bpp is not even supported by
the driver. Being the native format for communicating with the adapter,
16 bpp is the correct choice.

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

diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c
index b3fa6bf41acc..e6c1cd77d4d4 100644
--- a/drivers/gpu/drm/udl/udl_drv.c
+++ b/drivers/gpu/drm/udl/udl_drv.c
@@ -105,7 +105,7 @@ static int udl_usb_probe(struct usb_interface *interface,
 
 	DRM_INFO("Initialized udl on minor %d\n", udl->drm.primary->index);
 
-	r = drm_fbdev_generic_setup(&udl->drm, 16);
+	r = drm_fbdev_generic_setup(&udl->drm, 0);
 	if (r)
 		goto err_drm_dev_unregister;
 
diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
index 72884cbda131..1b5a46a91cb4 100644
--- a/drivers/gpu/drm/udl/udl_modeset.c
+++ b/drivers/gpu/drm/udl/udl_modeset.c
@@ -412,7 +412,7 @@ int udl_modeset_init(struct drm_device *dev)
 	dev->mode_config.max_height = 2048;
 
 	dev->mode_config.prefer_shadow = 0;
-	dev->mode_config.preferred_depth = 24;
+	dev->mode_config.preferred_depth = 16;
 
 	dev->mode_config.funcs = &udl_mode_funcs;
 
-- 
2.23.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 5/7] drm/udl: Convert to drm_atomic_helper_dirtyfb()

[Thomas Zimmermann]

The infrastruture for atomic modesetting allows us to use the generic
code for dirty-FB and damage handling. Switch over udl and remove the
driver's implementation.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/udl/udl_drv.h     |  5 ---
 drivers/gpu/drm/udl/udl_fb.c      | 67 -------------------------------
 drivers/gpu/drm/udl/udl_modeset.c | 11 +++--
 3 files changed, 8 insertions(+), 75 deletions(-)

diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
index 77b57d6abd23..c6fd5c08f5fc 100644
--- a/drivers/gpu/drm/udl/udl_drv.h
+++ b/drivers/gpu/drm/udl/udl_drv.h
@@ -89,11 +89,6 @@ void udl_urb_completion(struct urb *urb);
 int udl_init(struct udl_device *udl);
 void udl_fini(struct drm_device *dev);
 
-struct drm_framebuffer *
-udl_fb_user_fb_create(struct drm_device *dev,
-		      struct drm_file *file,
-		      const struct drm_mode_fb_cmd2 *mode_cmd);
-
 int udl_render_hline(struct drm_device *dev, int log_bpp, struct urb **urb_ptr,
 		     const char *front, char **urb_buf_ptr,
 		     u32 byte_offset, u32 device_byte_offset, u32 byte_width,
diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c
index c1996ac73a1f..ed01ebaaf468 100644
--- a/drivers/gpu/drm/udl/udl_fb.c
+++ b/drivers/gpu/drm/udl/udl_fb.c
@@ -9,14 +9,9 @@
  */
 
 #include <linux/moduleparam.h>
-#include <linux/dma-buf.h>
 
-#include <drm/drm_crtc_helper.h>
-#include <drm/drm_drv.h>
 #include <drm/drm_fourcc.h>
-#include <drm/drm_gem_framebuffer_helper.h>
 #include <drm/drm_gem_shmem_helper.h>
-#include <drm/drm_modeset_helper.h>
 
 #include "udl_drv.h"
 
@@ -152,65 +147,3 @@ int udl_handle_damage(struct drm_framebuffer *fb, int x, int y,
 	drm_gem_shmem_vunmap(fb->obj[0], vaddr);
 	return ret;
 }
-
-static int udl_user_framebuffer_dirty(struct drm_framebuffer *fb,
-				      struct drm_file *file,
-				      unsigned flags, unsigned color,
-				      struct drm_clip_rect *clips,
-				      unsigned num_clips)
-{
-	struct udl_device *udl = fb->dev->dev_private;
-	struct dma_buf_attachment *import_attach;
-	int i;
-	int ret = 0;
-
-	drm_modeset_lock_all(fb->dev);
-
-	spin_lock(&udl->active_fb_16_lock);
-	if (udl->active_fb_16 != fb) {
-		spin_unlock(&udl->active_fb_16_lock);
-		goto unlock;
-	}
-	spin_unlock(&udl->active_fb_16_lock);
-
-	import_attach = fb->obj[0]->import_attach;
-
-	if (import_attach) {
-		ret = dma_buf_begin_cpu_access(import_attach->dmabuf,
-					       DMA_FROM_DEVICE);
-		if (ret)
-			goto unlock;
-	}
-
-	for (i = 0; i < num_clips; i++) {
-		ret = udl_handle_damage(fb, clips[i].x1, clips[i].y1,
-					clips[i].x2 - clips[i].x1,
-					clips[i].y2 - clips[i].y1);
-		if (ret)
-			break;
-	}
-
-	if (import_attach)
-		ret = dma_buf_end_cpu_access(import_attach->dmabuf,
-					     DMA_FROM_DEVICE);
-
- unlock:
-	drm_modeset_unlock_all(fb->dev);
-
-	return ret;
-}
-
-static const struct drm_framebuffer_funcs udlfb_funcs = {
-	.destroy	= drm_gem_fb_destroy,
-	.create_handle	= drm_gem_fb_create_handle,
-	.dirty		= udl_user_framebuffer_dirty,
-};
-
-struct drm_framebuffer *
-udl_fb_user_fb_create(struct drm_device *dev,
-		   struct drm_file *file,
-		   const struct drm_mode_fb_cmd2 *mode_cmd)
-{
-	return drm_gem_fb_create_with_funcs(dev, file, mode_cmd,
-					    &udlfb_funcs);
-}
diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
index 1b5a46a91cb4..aade61ad097b 100644
--- a/drivers/gpu/drm/udl/udl_modeset.c
+++ b/drivers/gpu/drm/udl/udl_modeset.c
@@ -11,6 +11,7 @@
 
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_crtc_helper.h>
+#include <drm/drm_damage_helper.h>
 #include <drm/drm_gem_framebuffer_helper.h>
 #include <drm/drm_modeset_helper_vtables.h>
 #include <drm/drm_vblank.h>
@@ -364,7 +365,9 @@ udl_simple_display_pipe_update(struct drm_simple_display_pipe *pipe,
 {
 	struct drm_device *dev = pipe->crtc.dev;
 	struct udl_device *udl = dev->dev_private;
-	struct drm_framebuffer *fb = pipe->plane.state->fb;
+	struct drm_plane_state *state = pipe->plane.state;
+	struct drm_framebuffer *fb = state->fb;
+	struct drm_rect rect;
 
 	spin_lock(&udl->active_fb_16_lock);
 	udl->active_fb_16 = fb;
@@ -373,7 +376,9 @@ udl_simple_display_pipe_update(struct drm_simple_display_pipe *pipe,
 	if (!fb)
 		return;
 
-	udl_handle_damage(fb, 0, 0, fb->width, fb->height);
+	if (drm_atomic_helper_damage_merged(old_plane_state, state, &rect))
+		udl_handle_damage(fb, rect.x1, rect.y1, rect.x2 - rect.x1,
+				  rect.y2 - rect.y1);
 }
 
 static const
@@ -391,7 +396,7 @@ struct drm_simple_display_pipe_funcs udl_simple_display_pipe_funcs = {
  */
 
 static const struct drm_mode_config_funcs udl_mode_funcs = {
-	.fb_create = udl_fb_user_fb_create,
+	.fb_create = drm_gem_fb_create_with_dirty,
 	.atomic_check  = drm_atomic_helper_check,
 	.atomic_commit = drm_atomic_helper_commit,
 };
-- 
2.23.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/7] drm/udl: Convert to drm_atomic_helper_dirtyfb()

[Daniel Vetter]

On Tue, Nov 26, 2019 at 02:47:05PM +0100, Thomas Zimmermann wrote:
> The infrastruture for atomic modesetting allows us to use the generic
> code for dirty-FB and damage handling. Switch over udl and remove the
> driver's implementation.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/udl/udl_drv.h     |  5 ---
>  drivers/gpu/drm/udl/udl_fb.c      | 67 -------------------------------
>  drivers/gpu/drm/udl/udl_modeset.c | 11 +++--
>  3 files changed, 8 insertions(+), 75 deletions(-)
> 
> diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
> index 77b57d6abd23..c6fd5c08f5fc 100644
> --- a/drivers/gpu/drm/udl/udl_drv.h
> +++ b/drivers/gpu/drm/udl/udl_drv.h
> @@ -89,11 +89,6 @@ void udl_urb_completion(struct urb *urb);
>  int udl_init(struct udl_device *udl);
>  void udl_fini(struct drm_device *dev);
>  
> -struct drm_framebuffer *
> -udl_fb_user_fb_create(struct drm_device *dev,
> -		      struct drm_file *file,
> -		      const struct drm_mode_fb_cmd2 *mode_cmd);
> -
>  int udl_render_hline(struct drm_device *dev, int log_bpp, struct urb **urb_ptr,
>  		     const char *front, char **urb_buf_ptr,
>  		     u32 byte_offset, u32 device_byte_offset, u32 byte_width,
> diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c
> index c1996ac73a1f..ed01ebaaf468 100644
> --- a/drivers/gpu/drm/udl/udl_fb.c
> +++ b/drivers/gpu/drm/udl/udl_fb.c
> @@ -9,14 +9,9 @@
>   */
>  
>  #include <linux/moduleparam.h>
> -#include <linux/dma-buf.h>
>  
> -#include <drm/drm_crtc_helper.h>
> -#include <drm/drm_drv.h>
>  #include <drm/drm_fourcc.h>
> -#include <drm/drm_gem_framebuffer_helper.h>
>  #include <drm/drm_gem_shmem_helper.h>
> -#include <drm/drm_modeset_helper.h>
>  
>  #include "udl_drv.h"
>  
> @@ -152,65 +147,3 @@ int udl_handle_damage(struct drm_framebuffer *fb, int x, int y,
>  	drm_gem_shmem_vunmap(fb->obj[0], vaddr);
>  	return ret;
>  }
> -
> -static int udl_user_framebuffer_dirty(struct drm_framebuffer *fb,
> -				      struct drm_file *file,
> -				      unsigned flags, unsigned color,
> -				      struct drm_clip_rect *clips,
> -				      unsigned num_clips)
> -{
> -	struct udl_device *udl = fb->dev->dev_private;
> -	struct dma_buf_attachment *import_attach;
> -	int i;
> -	int ret = 0;
> -
> -	drm_modeset_lock_all(fb->dev);
> -
> -	spin_lock(&udl->active_fb_16_lock);
> -	if (udl->active_fb_16 != fb) {
> -		spin_unlock(&udl->active_fb_16_lock);
> -		goto unlock;
> -	}
> -	spin_unlock(&udl->active_fb_16_lock);
> -
> -	import_attach = fb->obj[0]->import_attach;
> -
> -	if (import_attach) {
> -		ret = dma_buf_begin_cpu_access(import_attach->dmabuf,
> -					       DMA_FROM_DEVICE);
> -		if (ret)
> -			goto unlock;
> -	}
> -
> -	for (i = 0; i < num_clips; i++) {
> -		ret = udl_handle_damage(fb, clips[i].x1, clips[i].y1,
> -					clips[i].x2 - clips[i].x1,
> -					clips[i].y2 - clips[i].y1);
> -		if (ret)
> -			break;
> -	}
> -
> -	if (import_attach)
> -		ret = dma_buf_end_cpu_access(import_attach->dmabuf,
> -					     DMA_FROM_DEVICE);
> -
> - unlock:
> -	drm_modeset_unlock_all(fb->dev);
> -
> -	return ret;
> -}
> -
> -static const struct drm_framebuffer_funcs udlfb_funcs = {
> -	.destroy	= drm_gem_fb_destroy,
> -	.create_handle	= drm_gem_fb_create_handle,
> -	.dirty		= udl_user_framebuffer_dirty,
> -};
> -
> -struct drm_framebuffer *
> -udl_fb_user_fb_create(struct drm_device *dev,
> -		   struct drm_file *file,
> -		   const struct drm_mode_fb_cmd2 *mode_cmd)
> -{
> -	return drm_gem_fb_create_with_funcs(dev, file, mode_cmd,
> -					    &udlfb_funcs);
> -}
> diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
> index 1b5a46a91cb4..aade61ad097b 100644
> --- a/drivers/gpu/drm/udl/udl_modeset.c
> +++ b/drivers/gpu/drm/udl/udl_modeset.c
> @@ -11,6 +11,7 @@
>  
>  #include <drm/drm_atomic_helper.h>
>  #include <drm/drm_crtc_helper.h>
> +#include <drm/drm_damage_helper.h>
>  #include <drm/drm_gem_framebuffer_helper.h>
>  #include <drm/drm_modeset_helper_vtables.h>
>  #include <drm/drm_vblank.h>
> @@ -364,7 +365,9 @@ udl_simple_display_pipe_update(struct drm_simple_display_pipe *pipe,
>  {
>  	struct drm_device *dev = pipe->crtc.dev;
>  	struct udl_device *udl = dev->dev_private;
> -	struct drm_framebuffer *fb = pipe->plane.state->fb;
> +	struct drm_plane_state *state = pipe->plane.state;
> +	struct drm_framebuffer *fb = state->fb;
> +	struct drm_rect rect;
>  
>  	spin_lock(&udl->active_fb_16_lock);
>  	udl->active_fb_16 = fb;
> @@ -373,7 +376,9 @@ udl_simple_display_pipe_update(struct drm_simple_display_pipe *pipe,
>  	if (!fb)
>  		return;
>  
> -	udl_handle_damage(fb, 0, 0, fb->width, fb->height);
> +	if (drm_atomic_helper_damage_merged(old_plane_state, state, &rect))
> +		udl_handle_damage(fb, rect.x1, rect.y1, rect.x2 - rect.x1,
> +				  rect.y2 - rect.y1);

Please mention in the commit message that you've put the optimized damage
upload into the pipe_update function here. With that:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Aside, would be neat to roll out the damage property for udl. But I'm not
sure whether it's been wired to any generic kms userspace yet (and which)
... Worst case could just test it with the igts we have.

Cheers, Daniel

>  }
>  
>  static const
> @@ -391,7 +396,7 @@ struct drm_simple_display_pipe_funcs udl_simple_display_pipe_funcs = {
>   */
>  
>  static const struct drm_mode_config_funcs udl_mode_funcs = {
> -	.fb_create = udl_fb_user_fb_create,
> +	.fb_create = drm_gem_fb_create_with_dirty,
>  	.atomic_check  = drm_atomic_helper_check,
>  	.atomic_commit = drm_atomic_helper_commit,
>  };
> -- 
> 2.23.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/7] drm/udl: Convert to drm_atomic_helper_dirtyfb()

[Emil Velikov]

Hi Thomas,

On Tue, 26 Nov 2019 at 13:47, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> The infrastruture for atomic modesetting allows us to use the generic
> code for dirty-FB and damage handling. Switch over udl and remove the
> driver's implementation.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/udl/udl_drv.h     |  5 ---
>  drivers/gpu/drm/udl/udl_fb.c      | 67 -------------------------------
>  drivers/gpu/drm/udl/udl_modeset.c | 11 +++--
>  3 files changed, 8 insertions(+), 75 deletions(-)
>
> diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
> index 77b57d6abd23..c6fd5c08f5fc 100644
> --- a/drivers/gpu/drm/udl/udl_drv.h
> +++ b/drivers/gpu/drm/udl/udl_drv.h
> @@ -89,11 +89,6 @@ void udl_urb_completion(struct urb *urb);
>  int udl_init(struct udl_device *udl);
>  void udl_fini(struct drm_device *dev);
>
> -struct drm_framebuffer *
> -udl_fb_user_fb_create(struct drm_device *dev,
> -                     struct drm_file *file,
> -                     const struct drm_mode_fb_cmd2 *mode_cmd);
> -
>  int udl_render_hline(struct drm_device *dev, int log_bpp, struct urb **urb_ptr,
>                      const char *front, char **urb_buf_ptr,
>                      u32 byte_offset, u32 device_byte_offset, u32 byte_width,
> diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c
> index c1996ac73a1f..ed01ebaaf468 100644
> --- a/drivers/gpu/drm/udl/udl_fb.c
> +++ b/drivers/gpu/drm/udl/udl_fb.c
> @@ -9,14 +9,9 @@
>   */
>
>  #include <linux/moduleparam.h>
> -#include <linux/dma-buf.h>
>
> -#include <drm/drm_crtc_helper.h>
> -#include <drm/drm_drv.h>
>  #include <drm/drm_fourcc.h>
> -#include <drm/drm_gem_framebuffer_helper.h>
>  #include <drm/drm_gem_shmem_helper.h>
> -#include <drm/drm_modeset_helper.h>
>
>  #include "udl_drv.h"
>
> @@ -152,65 +147,3 @@ int udl_handle_damage(struct drm_framebuffer *fb, int x, int y,
>         drm_gem_shmem_vunmap(fb->obj[0], vaddr);
>         return ret;
>  }
> -
> -static int udl_user_framebuffer_dirty(struct drm_framebuffer *fb,
> -                                     struct drm_file *file,
> -                                     unsigned flags, unsigned color,
> -                                     struct drm_clip_rect *clips,
> -                                     unsigned num_clips)
> -{
> -       struct udl_device *udl = fb->dev->dev_private;
> -       struct dma_buf_attachment *import_attach;
> -       int i;
> -       int ret = 0;
> -
> -       drm_modeset_lock_all(fb->dev);
> -
> -       spin_lock(&udl->active_fb_16_lock);
> -       if (udl->active_fb_16 != fb) {
> -               spin_unlock(&udl->active_fb_16_lock);
> -               goto unlock;
> -       }
> -       spin_unlock(&udl->active_fb_16_lock);
> -
> -       import_attach = fb->obj[0]->import_attach;
> -
> -       if (import_attach) {
> -               ret = dma_buf_begin_cpu_access(import_attach->dmabuf,
> -                                              DMA_FROM_DEVICE);
> -               if (ret)
> -                       goto unlock;
> -       }
> -
> -       for (i = 0; i < num_clips; i++) {
> -               ret = udl_handle_damage(fb, clips[i].x1, clips[i].y1,
> -                                       clips[i].x2 - clips[i].x1,
> -                                       clips[i].y2 - clips[i].y1);
> -               if (ret)
> -                       break;
> -       }
> -
> -       if (import_attach)
> -               ret = dma_buf_end_cpu_access(import_attach->dmabuf,
> -                                            DMA_FROM_DEVICE);
> -
I cannot quite see how the existing atomic helper handles dma buf imports.
There are no dma_buf_begin_cpu_access/dma_buf_end_cpu_access calls around.

From a quick look, imports don't seem to be prohibited in the helper.

Am I missing something? Alternatively can you add a brief note in the
commit message.

Thanks
-Emil
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/7] drm/udl: Convert to drm_atomic_helper_dirtyfb()

[Daniel Vetter]

On Fri, Nov 29, 2019 at 06:04:02PM +0000, Emil Velikov wrote:
> Hi Thomas,
> 
> On Tue, 26 Nov 2019 at 13:47, Thomas Zimmermann <tzimmermann@suse.de> wrote:
> >
> > The infrastruture for atomic modesetting allows us to use the generic
> > code for dirty-FB and damage handling. Switch over udl and remove the
> > driver's implementation.
> >
> > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> > ---
> >  drivers/gpu/drm/udl/udl_drv.h     |  5 ---
> >  drivers/gpu/drm/udl/udl_fb.c      | 67 -------------------------------
> >  drivers/gpu/drm/udl/udl_modeset.c | 11 +++--
> >  3 files changed, 8 insertions(+), 75 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
> > index 77b57d6abd23..c6fd5c08f5fc 100644
> > --- a/drivers/gpu/drm/udl/udl_drv.h
> > +++ b/drivers/gpu/drm/udl/udl_drv.h
> > @@ -89,11 +89,6 @@ void udl_urb_completion(struct urb *urb);
> >  int udl_init(struct udl_device *udl);
> >  void udl_fini(struct drm_device *dev);
> >
> > -struct drm_framebuffer *
> > -udl_fb_user_fb_create(struct drm_device *dev,
> > -                     struct drm_file *file,
> > -                     const struct drm_mode_fb_cmd2 *mode_cmd);
> > -
> >  int udl_render_hline(struct drm_device *dev, int log_bpp, struct urb **urb_ptr,
> >                      const char *front, char **urb_buf_ptr,
> >                      u32 byte_offset, u32 device_byte_offset, u32 byte_width,
> > diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c
> > index c1996ac73a1f..ed01ebaaf468 100644
> > --- a/drivers/gpu/drm/udl/udl_fb.c
> > +++ b/drivers/gpu/drm/udl/udl_fb.c
> > @@ -9,14 +9,9 @@
> >   */
> >
> >  #include <linux/moduleparam.h>
> > -#include <linux/dma-buf.h>
> >
> > -#include <drm/drm_crtc_helper.h>
> > -#include <drm/drm_drv.h>
> >  #include <drm/drm_fourcc.h>
> > -#include <drm/drm_gem_framebuffer_helper.h>
> >  #include <drm/drm_gem_shmem_helper.h>
> > -#include <drm/drm_modeset_helper.h>
> >
> >  #include "udl_drv.h"
> >
> > @@ -152,65 +147,3 @@ int udl_handle_damage(struct drm_framebuffer *fb, int x, int y,
> >         drm_gem_shmem_vunmap(fb->obj[0], vaddr);
> >         return ret;
> >  }
> > -
> > -static int udl_user_framebuffer_dirty(struct drm_framebuffer *fb,
> > -                                     struct drm_file *file,
> > -                                     unsigned flags, unsigned color,
> > -                                     struct drm_clip_rect *clips,
> > -                                     unsigned num_clips)
> > -{
> > -       struct udl_device *udl = fb->dev->dev_private;
> > -       struct dma_buf_attachment *import_attach;
> > -       int i;
> > -       int ret = 0;
> > -
> > -       drm_modeset_lock_all(fb->dev);
> > -
> > -       spin_lock(&udl->active_fb_16_lock);
> > -       if (udl->active_fb_16 != fb) {
> > -               spin_unlock(&udl->active_fb_16_lock);
> > -               goto unlock;
> > -       }
> > -       spin_unlock(&udl->active_fb_16_lock);
> > -
> > -       import_attach = fb->obj[0]->import_attach;
> > -
> > -       if (import_attach) {
> > -               ret = dma_buf_begin_cpu_access(import_attach->dmabuf,
> > -                                              DMA_FROM_DEVICE);
> > -               if (ret)
> > -                       goto unlock;
> > -       }
> > -
> > -       for (i = 0; i < num_clips; i++) {
> > -               ret = udl_handle_damage(fb, clips[i].x1, clips[i].y1,
> > -                                       clips[i].x2 - clips[i].x1,
> > -                                       clips[i].y2 - clips[i].y1);
> > -               if (ret)
> > -                       break;
> > -       }
> > -
> > -       if (import_attach)
> > -               ret = dma_buf_end_cpu_access(import_attach->dmabuf,
> > -                                            DMA_FROM_DEVICE);
> > -
> I cannot quite see how the existing atomic helper handles dma buf imports.
> There are no dma_buf_begin_cpu_access/dma_buf_end_cpu_access calls around.
> 
> From a quick look, imports don't seem to be prohibited in the helper.
> 
> Am I missing something? Alternatively can you add a brief note in the
> commit message.

Great catch, that's indeeded missing (and it was also missing from the
existing code except for the dirty path here). I think a prep patch to
move the begin/end_cpu_access into udl_handle_damage, before this entire
series here, would be best.
-Daniel

> 
> Thanks
> -Emil

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 6/7] drm/udl: Remove struct udl_device.active_fb_16

[Thomas Zimmermann]

The udl driver stores the currently active framebuffer to know from
where to accept damage updates.

With the conversion to plane-state damage handling, this is not necessary
any longer. The currently active framebuffer and damaged area are always
stored in the plane state.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/udl/udl_drv.h     | 4 ----
 drivers/gpu/drm/udl/udl_fb.c      | 7 -------
 drivers/gpu/drm/udl/udl_main.c    | 3 ---
 drivers/gpu/drm/udl/udl_modeset.c | 9 ---------
 4 files changed, 23 deletions(-)

diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
index c6fd5c08f5fc..e540f8e64aa1 100644
--- a/drivers/gpu/drm/udl/udl_drv.h
+++ b/drivers/gpu/drm/udl/udl_drv.h
@@ -55,10 +55,6 @@ struct udl_device {
 
 	struct drm_simple_display_pipe display_pipe;
 
-	/* active framebuffer on the 16-bit channel */
-	const struct drm_framebuffer *active_fb_16;
-	spinlock_t active_fb_16_lock;
-
 	struct mutex gem_lock;
 
 	int sku_pixel_limit;
diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c
index ed01ebaaf468..3d8cf674dfa5 100644
--- a/drivers/gpu/drm/udl/udl_fb.c
+++ b/drivers/gpu/drm/udl/udl_fb.c
@@ -76,13 +76,6 @@ int udl_handle_damage(struct drm_framebuffer *fb, int x, int y,
 
 	log_bpp = __ffs(fb->format->cpp[0]);
 
-	spin_lock(&udl->active_fb_16_lock);
-	if (udl->active_fb_16 != fb) {
-		spin_unlock(&udl->active_fb_16_lock);
-		return 0;
-	}
-	spin_unlock(&udl->active_fb_16_lock);
-
 	vaddr = drm_gem_shmem_vmap(fb->obj[0]);
 	if (IS_ERR(vaddr)) {
 		DRM_ERROR("failed to vmap fb\n");
diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
index a23218fc7d8e..9b091b5b063e 100644
--- a/drivers/gpu/drm/udl/udl_main.c
+++ b/drivers/gpu/drm/udl/udl_main.c
@@ -317,9 +317,6 @@ int udl_init(struct udl_device *udl)
 
 	DRM_DEBUG("\n");
 
-	udl->active_fb_16 = NULL;
-	spin_lock_init(&udl->active_fb_16_lock);
-
 	mutex_init(&udl->gem_lock);
 
 	if (!udl_parse_vendor_descriptor(dev, udl->udev)) {
diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
index aade61ad097b..83e80083e0b2 100644
--- a/drivers/gpu/drm/udl/udl_modeset.c
+++ b/drivers/gpu/drm/udl/udl_modeset.c
@@ -333,9 +333,6 @@ udl_simple_display_pipe_enable(struct drm_simple_display_pipe *pipe,
 
 	wrptr = udl_dummy_render(wrptr);
 
-	spin_lock(&udl->active_fb_16_lock);
-	udl->active_fb_16 = fb;
-	spin_unlock(&udl->active_fb_16_lock);
 	udl->mode_buf_len = wrptr - buf;
 
 	udl_handle_damage(fb, 0, 0, fb->width, fb->height);
@@ -363,16 +360,10 @@ static void
 udl_simple_display_pipe_update(struct drm_simple_display_pipe *pipe,
 			       struct drm_plane_state *old_plane_state)
 {
-	struct drm_device *dev = pipe->crtc.dev;
-	struct udl_device *udl = dev->dev_private;
 	struct drm_plane_state *state = pipe->plane.state;
 	struct drm_framebuffer *fb = state->fb;
 	struct drm_rect rect;
 
-	spin_lock(&udl->active_fb_16_lock);
-	udl->active_fb_16 = fb;
-	spin_unlock(&udl->active_fb_16_lock);
-
 	if (!fb)
 		return;
 
-- 
2.23.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 6/7] drm/udl: Remove struct udl_device.active_fb_16

[Daniel Vetter]

On Tue, Nov 26, 2019 at 02:47:06PM +0100, Thomas Zimmermann wrote:
> The udl driver stores the currently active framebuffer to know from
> where to accept damage updates.
> 
> With the conversion to plane-state damage handling, this is not necessary
> any longer. The currently active framebuffer and damaged area are always
> stored in the plane state.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Imo makes sense to garbage-collect that as a follow-up since it's not
really functional stuff, so doesn't make sense to squash that into the
previous patch.
-Daniel


> ---
>  drivers/gpu/drm/udl/udl_drv.h     | 4 ----
>  drivers/gpu/drm/udl/udl_fb.c      | 7 -------
>  drivers/gpu/drm/udl/udl_main.c    | 3 ---
>  drivers/gpu/drm/udl/udl_modeset.c | 9 ---------
>  4 files changed, 23 deletions(-)
> 
> diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
> index c6fd5c08f5fc..e540f8e64aa1 100644
> --- a/drivers/gpu/drm/udl/udl_drv.h
> +++ b/drivers/gpu/drm/udl/udl_drv.h
> @@ -55,10 +55,6 @@ struct udl_device {
>  
>  	struct drm_simple_display_pipe display_pipe;
>  
> -	/* active framebuffer on the 16-bit channel */
> -	const struct drm_framebuffer *active_fb_16;
> -	spinlock_t active_fb_16_lock;
> -
>  	struct mutex gem_lock;
>  
>  	int sku_pixel_limit;
> diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c
> index ed01ebaaf468..3d8cf674dfa5 100644
> --- a/drivers/gpu/drm/udl/udl_fb.c
> +++ b/drivers/gpu/drm/udl/udl_fb.c
> @@ -76,13 +76,6 @@ int udl_handle_damage(struct drm_framebuffer *fb, int x, int y,
>  
>  	log_bpp = __ffs(fb->format->cpp[0]);
>  
> -	spin_lock(&udl->active_fb_16_lock);
> -	if (udl->active_fb_16 != fb) {
> -		spin_unlock(&udl->active_fb_16_lock);
> -		return 0;
> -	}
> -	spin_unlock(&udl->active_fb_16_lock);
> -
>  	vaddr = drm_gem_shmem_vmap(fb->obj[0]);
>  	if (IS_ERR(vaddr)) {
>  		DRM_ERROR("failed to vmap fb\n");
> diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
> index a23218fc7d8e..9b091b5b063e 100644
> --- a/drivers/gpu/drm/udl/udl_main.c
> +++ b/drivers/gpu/drm/udl/udl_main.c
> @@ -317,9 +317,6 @@ int udl_init(struct udl_device *udl)
>  
>  	DRM_DEBUG("\n");
>  
> -	udl->active_fb_16 = NULL;
> -	spin_lock_init(&udl->active_fb_16_lock);
> -
>  	mutex_init(&udl->gem_lock);
>  
>  	if (!udl_parse_vendor_descriptor(dev, udl->udev)) {
> diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
> index aade61ad097b..83e80083e0b2 100644
> --- a/drivers/gpu/drm/udl/udl_modeset.c
> +++ b/drivers/gpu/drm/udl/udl_modeset.c
> @@ -333,9 +333,6 @@ udl_simple_display_pipe_enable(struct drm_simple_display_pipe *pipe,
>  
>  	wrptr = udl_dummy_render(wrptr);
>  
> -	spin_lock(&udl->active_fb_16_lock);
> -	udl->active_fb_16 = fb;
> -	spin_unlock(&udl->active_fb_16_lock);
>  	udl->mode_buf_len = wrptr - buf;
>  
>  	udl_handle_damage(fb, 0, 0, fb->width, fb->height);
> @@ -363,16 +360,10 @@ static void
>  udl_simple_display_pipe_update(struct drm_simple_display_pipe *pipe,
>  			       struct drm_plane_state *old_plane_state)
>  {
> -	struct drm_device *dev = pipe->crtc.dev;
> -	struct udl_device *udl = dev->dev_private;
>  	struct drm_plane_state *state = pipe->plane.state;
>  	struct drm_framebuffer *fb = state->fb;
>  	struct drm_rect rect;
>  
> -	spin_lock(&udl->active_fb_16_lock);
> -	udl->active_fb_16 = fb;
> -	spin_unlock(&udl->active_fb_16_lock);
> -
>  	if (!fb)
>  		return;
>  
> -- 
> 2.23.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 7/7] drm/udl: Move udl_handle_damage() into udl_modeset.c

[Thomas Zimmermann]

The only caller of udl_handle_damage() in the plane-update function
in udl_modeset.c. Move udl_handle_damage() there, make it static, and
remove several left-over macros.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/udl/Makefile      |   2 +-
 drivers/gpu/drm/udl/udl_drv.h     |   3 -
 drivers/gpu/drm/udl/udl_fb.c      | 142 ------------------------------
 drivers/gpu/drm/udl/udl_modeset.c |  88 ++++++++++++++++++
 4 files changed, 89 insertions(+), 146 deletions(-)
 delete mode 100644 drivers/gpu/drm/udl/udl_fb.c

diff --git a/drivers/gpu/drm/udl/Makefile b/drivers/gpu/drm/udl/Makefile
index 177ce74f4cf4..b50179bb4de0 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_fb.o udl_transfer.o udl_gem.o
+udl-y := udl_drv.o udl_modeset.o udl_connector.o udl_main.o udl_transfer.o udl_gem.o
 
 obj-$(CONFIG_DRM_UDL) := udl.o
diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
index e540f8e64aa1..ab62a6aecd06 100644
--- a/drivers/gpu/drm/udl/udl_drv.h
+++ b/drivers/gpu/drm/udl/udl_drv.h
@@ -93,9 +93,6 @@ int udl_render_hline(struct drm_device *dev, int log_bpp, struct urb **urb_ptr,
 struct drm_gem_object *udl_driver_gem_create_object(struct drm_device *dev,
 						    size_t size);
 
-int udl_handle_damage(struct drm_framebuffer *fb, int x, int y,
-		      int width, int height);
-
 int udl_drop_usb(struct drm_device *dev);
 
 #define CMD_WRITE_RAW8   "\xAF\x60" /**< 8 bit raw write command. */
diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c
deleted file mode 100644
index 3d8cf674dfa5..000000000000
--- a/drivers/gpu/drm/udl/udl_fb.c
+++ /dev/null
@@ -1,142 +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 <linux/moduleparam.h>
-
-#include <drm/drm_fourcc.h>
-#include <drm/drm_gem_shmem_helper.h>
-
-#include "udl_drv.h"
-
-#define DL_ALIGN_UP(x, a) ALIGN(x, a)
-#define DL_ALIGN_DOWN(x, a) ALIGN_DOWN(x, a)
-
-/** Read the red component (0..255) of a 32 bpp colour. */
-#define DLO_RGB_GETRED(col) (uint8_t)((col) & 0xFF)
-
-/** Read the green component (0..255) of a 32 bpp colour. */
-#define DLO_RGB_GETGRN(col) (uint8_t)(((col) >> 8) & 0xFF)
-
-/** Read the blue component (0..255) of a 32 bpp colour. */
-#define DLO_RGB_GETBLU(col) (uint8_t)(((col) >> 16) & 0xFF)
-
-/** Return red/green component of a 16 bpp colour number. */
-#define DLO_RG16(red, grn) (uint8_t)((((red) & 0xF8) | ((grn) >> 5)) & 0xFF)
-
-/** Return green/blue component of a 16 bpp colour number. */
-#define DLO_GB16(grn, blu) (uint8_t)(((((grn) & 0x1C) << 3) | ((blu) >> 3)) & 0xFF)
-
-/** Return 8 bpp colour number from red, green and blue components. */
-#define DLO_RGB8(red, grn, blu) ((((red) << 5) | (((grn) & 3) << 3) | ((blu) & 7)) & 0xFF)
-
-#if 0
-static uint8_t rgb8(uint32_t col)
-{
-	uint8_t red = DLO_RGB_GETRED(col);
-	uint8_t grn = DLO_RGB_GETGRN(col);
-	uint8_t blu = DLO_RGB_GETBLU(col);
-
-	return DLO_RGB8(red, grn, blu);
-}
-
-static uint16_t rgb16(uint32_t col)
-{
-	uint8_t red = DLO_RGB_GETRED(col);
-	uint8_t grn = DLO_RGB_GETGRN(col);
-	uint8_t blu = DLO_RGB_GETBLU(col);
-
-	return (DLO_RG16(red, grn) << 8) + DLO_GB16(grn, blu);
-}
-#endif
-
-int udl_handle_damage(struct drm_framebuffer *fb, int x, int y,
-		      int width, int height)
-{
-	struct drm_device *dev = fb->dev;
-	struct udl_device *udl = to_udl(dev);
-	int i, ret;
-	char *cmd;
-	cycles_t start_cycles, end_cycles;
-	int bytes_sent = 0;
-	int bytes_identical = 0;
-	struct urb *urb;
-	int aligned_x;
-	int log_bpp;
-	void *vaddr;
-
-	if (WARN_ON(!is_power_of_2(fb->format->cpp[0])))
-		return -EINVAL;
-
-	log_bpp = __ffs(fb->format->cpp[0]);
-
-	vaddr = drm_gem_shmem_vmap(fb->obj[0]);
-	if (IS_ERR(vaddr)) {
-		DRM_ERROR("failed to vmap fb\n");
-		return 0;
-	}
-
-	aligned_x = DL_ALIGN_DOWN(x, sizeof(unsigned long));
-	width = DL_ALIGN_UP(width + (x-aligned_x), sizeof(unsigned long));
-	x = aligned_x;
-
-	if ((width <= 0) ||
-	    (x + width > fb->width) ||
-	    (y + height > fb->height)) {
-		ret = -EINVAL;
-		goto err_drm_gem_shmem_vunmap;
-	}
-
-	start_cycles = get_cycles();
-
-	urb = udl_get_urb(dev);
-	if (!urb)
-		goto out;
-	cmd = urb->transfer_buffer;
-
-	for (i = y; i < y + height ; i++) {
-		const int line_offset = fb->pitches[0] * i;
-		const int byte_offset = line_offset + (x << log_bpp);
-		const int dev_byte_offset = (fb->width * i + x) << log_bpp;
-		if (udl_render_hline(dev, log_bpp, &urb, (char *)vaddr,
-				     &cmd, byte_offset, dev_byte_offset,
-				     width << log_bpp,
-				     &bytes_identical, &bytes_sent))
-			goto error;
-	}
-
-	if (cmd > (char *) urb->transfer_buffer) {
-		/* Send partial buffer remaining before exiting */
-		int len;
-		if (cmd < (char *) urb->transfer_buffer + urb->transfer_buffer_length)
-			*cmd++ = 0xAF;
-		len = cmd - (char *) urb->transfer_buffer;
-		ret = udl_submit_urb(dev, urb, len);
-		bytes_sent += len;
-	} else
-		udl_urb_completion(urb);
-
-error:
-	atomic_add(bytes_sent, &udl->bytes_sent);
-	atomic_add(bytes_identical, &udl->bytes_identical);
-	atomic_add((width * height) << log_bpp, &udl->bytes_rendered);
-	end_cycles = get_cycles();
-	atomic_add(((unsigned int) ((end_cycles - start_cycles)
-		    >> 10)), /* Kcycles */
-		   &udl->cpu_kcycles_used);
-
-out:
-	drm_gem_shmem_vunmap(fb->obj[0], vaddr);
-
-	return 0;
-
-err_drm_gem_shmem_vunmap:
-	drm_gem_shmem_vunmap(fb->obj[0], vaddr);
-	return ret;
-}
diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
index 83e80083e0b2..7107c90672ae 100644
--- a/drivers/gpu/drm/udl/udl_modeset.c
+++ b/drivers/gpu/drm/udl/udl_modeset.c
@@ -12,7 +12,9 @@
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_crtc_helper.h>
 #include <drm/drm_damage_helper.h>
+#include <drm/drm_fourcc.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_vblank.h>
 
@@ -282,6 +284,92 @@ static void udl_crtc_dpms(struct drm_crtc *crtc, int mode)
 
 }
 
+static int udl_handle_damage(struct drm_framebuffer *fb, int x, int y,
+			     int width, int height)
+{
+	struct drm_device *dev = fb->dev;
+	struct udl_device *udl = to_udl(dev);
+	int i, ret;
+	char *cmd;
+	cycles_t start_cycles, end_cycles;
+	int bytes_sent = 0;
+	int bytes_identical = 0;
+	struct urb *urb;
+	int aligned_x;
+	int log_bpp;
+	void *vaddr;
+
+	if (WARN_ON(!is_power_of_2(fb->format->cpp[0])))
+		return -EINVAL;
+
+	log_bpp = __ffs(fb->format->cpp[0]);
+
+	vaddr = drm_gem_shmem_vmap(fb->obj[0]);
+	if (IS_ERR(vaddr)) {
+		DRM_ERROR("failed to vmap fb\n");
+		return 0;
+	}
+
+	aligned_x = ALIGN_DOWN(x, sizeof(unsigned long));
+	width = ALIGN(width + (x-aligned_x), sizeof(unsigned long));
+	x = aligned_x;
+
+	if ((width <= 0) ||
+	    (x + width > fb->width) ||
+	    (y + height > fb->height)) {
+		ret = -EINVAL;
+		goto err_drm_gem_shmem_vunmap;
+	}
+
+	start_cycles = get_cycles();
+
+	urb = udl_get_urb(dev);
+	if (!urb)
+		goto out;
+	cmd = urb->transfer_buffer;
+
+	for (i = y; i < y + height ; i++) {
+		const int line_offset = fb->pitches[0] * i;
+		const int byte_offset = line_offset + (x << log_bpp);
+		const int dev_byte_offset = (fb->width * i + x) << log_bpp;
+
+		if (udl_render_hline(dev, log_bpp, &urb, (char *)vaddr,
+				     &cmd, byte_offset, dev_byte_offset,
+				     width << log_bpp,
+				     &bytes_identical, &bytes_sent))
+			goto error;
+	}
+
+	if (cmd > (char *) urb->transfer_buffer) {
+		/* Send partial buffer remaining before exiting */
+		int len;
+		if (cmd < (char *) urb->transfer_buffer + urb->transfer_buffer_length)
+			*cmd++ = 0xAF;
+		len = cmd - (char *) urb->transfer_buffer;
+		ret = udl_submit_urb(dev, urb, len);
+		bytes_sent += len;
+	} else
+		udl_urb_completion(urb);
+
+error:
+	atomic_add(bytes_sent, &udl->bytes_sent);
+	atomic_add(bytes_identical, &udl->bytes_identical);
+	atomic_add((width * height) << log_bpp, &udl->bytes_rendered);
+	end_cycles = get_cycles();
+	atomic_add(((unsigned int) ((end_cycles - start_cycles)
+		    >> 10)), /* Kcycles */
+		   &udl->cpu_kcycles_used);
+
+out:
+	drm_gem_shmem_vunmap(fb->obj[0], vaddr);
+
+	return 0;
+
+err_drm_gem_shmem_vunmap:
+	drm_gem_shmem_vunmap(fb->obj[0], vaddr);
+	return ret;
+}
+
 /*
  * Simple display pipeline
  */
-- 
2.23.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 7/7] drm/udl: Move udl_handle_damage() into udl_modeset.c

[Daniel Vetter]

On Tue, Nov 26, 2019 at 02:47:07PM +0100, Thomas Zimmermann wrote:
> The only caller of udl_handle_damage() in the plane-update function
> in udl_modeset.c. Move udl_handle_damage() there, make it static, and
> remove several left-over macros.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/udl/Makefile      |   2 +-
>  drivers/gpu/drm/udl/udl_drv.h     |   3 -
>  drivers/gpu/drm/udl/udl_fb.c      | 142 ------------------------------
>  drivers/gpu/drm/udl/udl_modeset.c |  88 ++++++++++++++++++
>  4 files changed, 89 insertions(+), 146 deletions(-)
>  delete mode 100644 drivers/gpu/drm/udl/udl_fb.c
> 
> diff --git a/drivers/gpu/drm/udl/Makefile b/drivers/gpu/drm/udl/Makefile
> index 177ce74f4cf4..b50179bb4de0 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_fb.o udl_transfer.o udl_gem.o
> +udl-y := udl_drv.o udl_modeset.o udl_connector.o udl_main.o udl_transfer.o udl_gem.o
>  
>  obj-$(CONFIG_DRM_UDL) := udl.o
> diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
> index e540f8e64aa1..ab62a6aecd06 100644
> --- a/drivers/gpu/drm/udl/udl_drv.h
> +++ b/drivers/gpu/drm/udl/udl_drv.h
> @@ -93,9 +93,6 @@ int udl_render_hline(struct drm_device *dev, int log_bpp, struct urb **urb_ptr,
>  struct drm_gem_object *udl_driver_gem_create_object(struct drm_device *dev,
>  						    size_t size);
>  
> -int udl_handle_damage(struct drm_framebuffer *fb, int x, int y,
> -		      int width, int height);
> -
>  int udl_drop_usb(struct drm_device *dev);
>  
>  #define CMD_WRITE_RAW8   "\xAF\x60" /**< 8 bit raw write command. */
> diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c
> deleted file mode 100644
> index 3d8cf674dfa5..000000000000
> --- a/drivers/gpu/drm/udl/udl_fb.c
> +++ /dev/null
> @@ -1,142 +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 <linux/moduleparam.h>
> -
> -#include <drm/drm_fourcc.h>
> -#include <drm/drm_gem_shmem_helper.h>
> -
> -#include "udl_drv.h"
> -
> -#define DL_ALIGN_UP(x, a) ALIGN(x, a)
> -#define DL_ALIGN_DOWN(x, a) ALIGN_DOWN(x, a)
> -
> -/** Read the red component (0..255) of a 32 bpp colour. */
> -#define DLO_RGB_GETRED(col) (uint8_t)((col) & 0xFF)
> -
> -/** Read the green component (0..255) of a 32 bpp colour. */
> -#define DLO_RGB_GETGRN(col) (uint8_t)(((col) >> 8) & 0xFF)
> -
> -/** Read the blue component (0..255) of a 32 bpp colour. */
> -#define DLO_RGB_GETBLU(col) (uint8_t)(((col) >> 16) & 0xFF)
> -
> -/** Return red/green component of a 16 bpp colour number. */
> -#define DLO_RG16(red, grn) (uint8_t)((((red) & 0xF8) | ((grn) >> 5)) & 0xFF)
> -
> -/** Return green/blue component of a 16 bpp colour number. */
> -#define DLO_GB16(grn, blu) (uint8_t)(((((grn) & 0x1C) << 3) | ((blu) >> 3)) & 0xFF)
> -
> -/** Return 8 bpp colour number from red, green and blue components. */
> -#define DLO_RGB8(red, grn, blu) ((((red) << 5) | (((grn) & 3) << 3) | ((blu) & 7)) & 0xFF)
> -
> -#if 0
> -static uint8_t rgb8(uint32_t col)
> -{
> -	uint8_t red = DLO_RGB_GETRED(col);
> -	uint8_t grn = DLO_RGB_GETGRN(col);
> -	uint8_t blu = DLO_RGB_GETBLU(col);
> -
> -	return DLO_RGB8(red, grn, blu);
> -}
> -
> -static uint16_t rgb16(uint32_t col)
> -{
> -	uint8_t red = DLO_RGB_GETRED(col);
> -	uint8_t grn = DLO_RGB_GETGRN(col);
> -	uint8_t blu = DLO_RGB_GETBLU(col);
> -
> -	return (DLO_RG16(red, grn) << 8) + DLO_GB16(grn, blu);
> -}
> -#endif
> -
> -int udl_handle_damage(struct drm_framebuffer *fb, int x, int y,
> -		      int width, int height)
> -{
> -	struct drm_device *dev = fb->dev;
> -	struct udl_device *udl = to_udl(dev);
> -	int i, ret;
> -	char *cmd;
> -	cycles_t start_cycles, end_cycles;
> -	int bytes_sent = 0;
> -	int bytes_identical = 0;
> -	struct urb *urb;
> -	int aligned_x;
> -	int log_bpp;
> -	void *vaddr;
> -
> -	if (WARN_ON(!is_power_of_2(fb->format->cpp[0])))
> -		return -EINVAL;
> -
> -	log_bpp = __ffs(fb->format->cpp[0]);
> -
> -	vaddr = drm_gem_shmem_vmap(fb->obj[0]);
> -	if (IS_ERR(vaddr)) {
> -		DRM_ERROR("failed to vmap fb\n");
> -		return 0;
> -	}
> -
> -	aligned_x = DL_ALIGN_DOWN(x, sizeof(unsigned long));
> -	width = DL_ALIGN_UP(width + (x-aligned_x), sizeof(unsigned long));
> -	x = aligned_x;
> -
> -	if ((width <= 0) ||
> -	    (x + width > fb->width) ||
> -	    (y + height > fb->height)) {
> -		ret = -EINVAL;
> -		goto err_drm_gem_shmem_vunmap;
> -	}
> -
> -	start_cycles = get_cycles();
> -
> -	urb = udl_get_urb(dev);
> -	if (!urb)
> -		goto out;
> -	cmd = urb->transfer_buffer;
> -
> -	for (i = y; i < y + height ; i++) {
> -		const int line_offset = fb->pitches[0] * i;
> -		const int byte_offset = line_offset + (x << log_bpp);
> -		const int dev_byte_offset = (fb->width * i + x) << log_bpp;
> -		if (udl_render_hline(dev, log_bpp, &urb, (char *)vaddr,
> -				     &cmd, byte_offset, dev_byte_offset,
> -				     width << log_bpp,
> -				     &bytes_identical, &bytes_sent))
> -			goto error;
> -	}
> -
> -	if (cmd > (char *) urb->transfer_buffer) {
> -		/* Send partial buffer remaining before exiting */
> -		int len;
> -		if (cmd < (char *) urb->transfer_buffer + urb->transfer_buffer_length)
> -			*cmd++ = 0xAF;
> -		len = cmd - (char *) urb->transfer_buffer;
> -		ret = udl_submit_urb(dev, urb, len);
> -		bytes_sent += len;
> -	} else
> -		udl_urb_completion(urb);
> -
> -error:
> -	atomic_add(bytes_sent, &udl->bytes_sent);
> -	atomic_add(bytes_identical, &udl->bytes_identical);
> -	atomic_add((width * height) << log_bpp, &udl->bytes_rendered);
> -	end_cycles = get_cycles();
> -	atomic_add(((unsigned int) ((end_cycles - start_cycles)
> -		    >> 10)), /* Kcycles */
> -		   &udl->cpu_kcycles_used);
> -
> -out:
> -	drm_gem_shmem_vunmap(fb->obj[0], vaddr);
> -
> -	return 0;
> -
> -err_drm_gem_shmem_vunmap:
> -	drm_gem_shmem_vunmap(fb->obj[0], vaddr);
> -	return ret;
> -}
> diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
> index 83e80083e0b2..7107c90672ae 100644
> --- a/drivers/gpu/drm/udl/udl_modeset.c
> +++ b/drivers/gpu/drm/udl/udl_modeset.c
> @@ -12,7 +12,9 @@
>  #include <drm/drm_atomic_helper.h>
>  #include <drm/drm_crtc_helper.h>
>  #include <drm/drm_damage_helper.h>
> +#include <drm/drm_fourcc.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_vblank.h>
>  
> @@ -282,6 +284,92 @@ static void udl_crtc_dpms(struct drm_crtc *crtc, int mode)
>  
>  }
>  
> +static int udl_handle_damage(struct drm_framebuffer *fb, int x, int y,
> +			     int width, int height)
> +{
> +	struct drm_device *dev = fb->dev;
> +	struct udl_device *udl = to_udl(dev);
> +	int i, ret;
> +	char *cmd;
> +	cycles_t start_cycles, end_cycles;
> +	int bytes_sent = 0;
> +	int bytes_identical = 0;
> +	struct urb *urb;
> +	int aligned_x;
> +	int log_bpp;
> +	void *vaddr;
> +
> +	if (WARN_ON(!is_power_of_2(fb->format->cpp[0])))
> +		return -EINVAL;
> +
> +	log_bpp = __ffs(fb->format->cpp[0]);
> +
> +	vaddr = drm_gem_shmem_vmap(fb->obj[0]);
> +	if (IS_ERR(vaddr)) {
> +		DRM_ERROR("failed to vmap fb\n");
> +		return 0;
> +	}
> +
> +	aligned_x = ALIGN_DOWN(x, sizeof(unsigned long));
> +	width = ALIGN(width + (x-aligned_x), sizeof(unsigned long));
> +	x = aligned_x;
> +
> +	if ((width <= 0) ||
> +	    (x + width > fb->width) ||
> +	    (y + height > fb->height)) {
> +		ret = -EINVAL;
> +		goto err_drm_gem_shmem_vunmap;
> +	}
> +
> +	start_cycles = get_cycles();
> +
> +	urb = udl_get_urb(dev);
> +	if (!urb)
> +		goto out;
> +	cmd = urb->transfer_buffer;
> +
> +	for (i = y; i < y + height ; i++) {
> +		const int line_offset = fb->pitches[0] * i;
> +		const int byte_offset = line_offset + (x << log_bpp);
> +		const int dev_byte_offset = (fb->width * i + x) << log_bpp;
> +
> +		if (udl_render_hline(dev, log_bpp, &urb, (char *)vaddr,
> +				     &cmd, byte_offset, dev_byte_offset,
> +				     width << log_bpp,
> +				     &bytes_identical, &bytes_sent))
> +			goto error;
> +	}
> +
> +	if (cmd > (char *) urb->transfer_buffer) {
> +		/* Send partial buffer remaining before exiting */
> +		int len;
> +		if (cmd < (char *) urb->transfer_buffer + urb->transfer_buffer_length)
> +			*cmd++ = 0xAF;
> +		len = cmd - (char *) urb->transfer_buffer;
> +		ret = udl_submit_urb(dev, urb, len);
> +		bytes_sent += len;
> +	} else
> +		udl_urb_completion(urb);
> +
> +error:
> +	atomic_add(bytes_sent, &udl->bytes_sent);
> +	atomic_add(bytes_identical, &udl->bytes_identical);
> +	atomic_add((width * height) << log_bpp, &udl->bytes_rendered);
> +	end_cycles = get_cycles();
> +	atomic_add(((unsigned int) ((end_cycles - start_cycles)
> +		    >> 10)), /* Kcycles */
> +		   &udl->cpu_kcycles_used);
> +
> +out:
> +	drm_gem_shmem_vunmap(fb->obj[0], vaddr);
> +
> +	return 0;
> +
> +err_drm_gem_shmem_vunmap:
> +	drm_gem_shmem_vunmap(fb->obj[0], vaddr);
> +	return ret;
> +}
> +
>  /*
>   * Simple display pipeline
>   */
> -- 
> 2.23.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 7/7] drm/udl: Move udl_handle_damage() into udl_modeset.c

[Emil Velikov]

On Tue, 26 Nov 2019 at 13:47, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> The only caller of udl_handle_damage() in the plane-update function
> in udl_modeset.c. Move udl_handle_damage() there, make it static, and
> remove several left-over macros.
>
Personally I would have left the mechanic code motion from the dead
code removal.
Not a big deal though:
Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com>

There's few comments for follow-up work below.

> +static int udl_handle_damage(struct drm_framebuffer *fb, int x, int y,
> +                            int width, int height)
> +{
> +       struct drm_device *dev = fb->dev;
> +       struct udl_device *udl = to_udl(dev);
> +       int i, ret;
> +       char *cmd;
> +       cycles_t start_cycles, end_cycles;
> +       int bytes_sent = 0;
> +       int bytes_identical = 0;
> +       struct urb *urb;
> +       int aligned_x;
> +       int log_bpp;
> +       void *vaddr;
> +
> +       if (WARN_ON(!is_power_of_2(fb->format->cpp[0])))
> +               return -EINVAL;
> +

> +       log_bpp = __ffs(fb->format->cpp[0]);
> +
> +       vaddr = drm_gem_shmem_vmap(fb->obj[0]);
> +       if (IS_ERR(vaddr)) {
> +               DRM_ERROR("failed to vmap fb\n");
> +               return 0;
> +       }
> +
Might as well move this hunk ...

> +       aligned_x = ALIGN_DOWN(x, sizeof(unsigned long));
> +       width = ALIGN(width + (x-aligned_x), sizeof(unsigned long));
> +       x = aligned_x;
> +
> +       if ((width <= 0) ||
> +           (x + width > fb->width) ||
> +           (y + height > fb->height)) {
> +               ret = -EINVAL;
> +               goto err_drm_gem_shmem_vunmap;
> +       }
> +
> +       start_cycles = get_cycles();
> +
> +       urb = udl_get_urb(dev);
> +       if (!urb)
> +               goto out;
> +       cmd = urb->transfer_buffer;
> +
... here

> +       for (i = y; i < y + height ; i++) {
> +               const int line_offset = fb->pitches[0] * i;
> +               const int byte_offset = line_offset + (x << log_bpp);
> +               const int dev_byte_offset = (fb->width * i + x) << log_bpp;
> +
> +               if (udl_render_hline(dev, log_bpp, &urb, (char *)vaddr,
> +                                    &cmd, byte_offset, dev_byte_offset,
> +                                    width << log_bpp,
> +                                    &bytes_identical, &bytes_sent))
udl_render_hline() - drop the unused args bytes_identical and bytes_sent?

> +                       goto error;
> +       }
> +
> +       if (cmd > (char *) urb->transfer_buffer) {
> +               /* Send partial buffer remaining before exiting */
> +               int len;
> +               if (cmd < (char *) urb->transfer_buffer + urb->transfer_buffer_length)
> +                       *cmd++ = 0xAF;
> +               len = cmd - (char *) urb->transfer_buffer;
> +               ret = udl_submit_urb(dev, urb, len);
> +               bytes_sent += len;
Nit:

int len = cmd - (char *) urb->transfer_buffer;
if (len > 0) {
   /* Send partial buffer remaining before exiting */
   if (len < urb->transfer_buffer_length)
...

> +       } else
> +               udl_urb_completion(urb);
> +
> +error:

> +       atomic_add(bytes_sent, &udl->bytes_sent);
> +       atomic_add(bytes_identical, &udl->bytes_identical);
> +       atomic_add((width * height) << log_bpp, &udl->bytes_rendered);
> +       end_cycles = get_cycles();
> +       atomic_add(((unsigned int) ((end_cycles - start_cycles)
> +                   >> 10)), /* Kcycles */
> +                  &udl->cpu_kcycles_used);
> +
These atomics (+ lost_pixels) seem to be set-but-unused since day one.
We might as well kill them, alongside the associated get_cycles().

HTH
Emil
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 7/7] drm/udl: Move udl_handle_damage() into udl_modeset.c

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 4502 bytes --]

Hi

Am 02.12.19 um 10:27 schrieb Emil Velikov:
> On Tue, 26 Nov 2019 at 13:47, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>>
>> The only caller of udl_handle_damage() in the plane-update function
>> in udl_modeset.c. Move udl_handle_damage() there, make it static, and
>> remove several left-over macros.
>>
> Personally I would have left the mechanic code motion from the dead
> code removal.
> Not a big deal though:
> Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com>
> 
> There's few comments for follow-up work below.
> 
>> +static int udl_handle_damage(struct drm_framebuffer *fb, int x, int y,
>> +                            int width, int height)
>> +{
>> +       struct drm_device *dev = fb->dev;
>> +       struct udl_device *udl = to_udl(dev);
>> +       int i, ret;
>> +       char *cmd;
>> +       cycles_t start_cycles, end_cycles;
>> +       int bytes_sent = 0;
>> +       int bytes_identical = 0;
>> +       struct urb *urb;
>> +       int aligned_x;
>> +       int log_bpp;
>> +       void *vaddr;
>> +
>> +       if (WARN_ON(!is_power_of_2(fb->format->cpp[0])))
>> +               return -EINVAL;
>> +
> 
>> +       log_bpp = __ffs(fb->format->cpp[0]);
>> +
>> +       vaddr = drm_gem_shmem_vmap(fb->obj[0]);
>> +       if (IS_ERR(vaddr)) {
>> +               DRM_ERROR("failed to vmap fb\n");
>> +               return 0;
>> +       }
>> +
> Might as well move this hunk ...
> 
>> +       aligned_x = ALIGN_DOWN(x, sizeof(unsigned long));
>> +       width = ALIGN(width + (x-aligned_x), sizeof(unsigned long));
>> +       x = aligned_x;
>> +
>> +       if ((width <= 0) ||
>> +           (x + width > fb->width) ||
>> +           (y + height > fb->height)) {
>> +               ret = -EINVAL;
>> +               goto err_drm_gem_shmem_vunmap;
>> +       }
>> +
>> +       start_cycles = get_cycles();
>> +
>> +       urb = udl_get_urb(dev);
>> +       if (!urb)
>> +               goto out;
>> +       cmd = urb->transfer_buffer;
>> +
> ... here
> 
>> +       for (i = y; i < y + height ; i++) {
>> +               const int line_offset = fb->pitches[0] * i;
>> +               const int byte_offset = line_offset + (x << log_bpp);
>> +               const int dev_byte_offset = (fb->width * i + x) << log_bpp;
>> +
>> +               if (udl_render_hline(dev, log_bpp, &urb, (char *)vaddr,
>> +                                    &cmd, byte_offset, dev_byte_offset,
>> +                                    width << log_bpp,
>> +                                    &bytes_identical, &bytes_sent))
> udl_render_hline() - drop the unused args bytes_identical and bytes_sent?
> 
>> +                       goto error;
>> +       }
>> +
>> +       if (cmd > (char *) urb->transfer_buffer) {
>> +               /* Send partial buffer remaining before exiting */
>> +               int len;
>> +               if (cmd < (char *) urb->transfer_buffer + urb->transfer_buffer_length)
>> +                       *cmd++ = 0xAF;
>> +               len = cmd - (char *) urb->transfer_buffer;
>> +               ret = udl_submit_urb(dev, urb, len);
>> +               bytes_sent += len;
> Nit:
> 
> int len = cmd - (char *) urb->transfer_buffer;
> if (len > 0) {
>    /* Send partial buffer remaining before exiting */
>    if (len < urb->transfer_buffer_length)
> ...
> 
>> +       } else
>> +               udl_urb_completion(urb);
>> +
>> +error:
> 
>> +       atomic_add(bytes_sent, &udl->bytes_sent);
>> +       atomic_add(bytes_identical, &udl->bytes_identical);
>> +       atomic_add((width * height) << log_bpp, &udl->bytes_rendered);
>> +       end_cycles = get_cycles();
>> +       atomic_add(((unsigned int) ((end_cycles - start_cycles)
>> +                   >> 10)), /* Kcycles */
>> +                  &udl->cpu_kcycles_used);
>> +
> These atomics (+ lost_pixels) seem to be set-but-unused since day one.
> We might as well kill them, alongside the associated get_cycles().

I wanted to do this later. But since you brought it up and there's also
the issue where I forgot to convert the import_attach code, I'll send
out a series to clean up the damage handling and then get back to the
simple-pipe conversion.

Best regards
Thomas

> 
> HTH
> Emil
> 

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


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

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel