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@xxxxxxx>
> ---
> 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@xxxxxxxxxxxx>
> */
>
> +#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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
