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

[Daniel Vetter <daniel@ffwll.ch>]

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