| Message ID | 20191126134707.22385-3-tzimmermann@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/udl: Convert to simple-pipe helpers and clean up | expand |
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 >
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);
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(-)