Many DRM drivers implement an encoder with an empty implementation. This patchset adds drm_simple_encoder_init(), which drivers can use instead. Except for the destroy callback, the simple encoder's implementation is empty.
The patchset also converts 4 encoder instances to use the simple-encoder helpers. But there are at least 11 other drivers which can use the helper and I think I did not examine all drivers yet.
The patchset was smoke-tested on mgag200 by running the fbdev console and Gnome on X11.
v4: * print error messages with drm_err() (Sam) * qxl: handle errors of drm_simple_encoder_init() (Sam) v3: * remove drm_simple_encoder_create() for lack of users (Sam, Daniel) * provide more precise documentation (Sam) v2: * move simple encoder to KMS helpers (Daniel) * remove name argument; simplifies implementation (Gerd) * don't allocate with devm_ interfaces; unsafe with DRM (Noralf)
Thomas Zimmermann (4): drm/simple-kms: Add drm_simple_encoder_{init,create}() drm/ast: Use simple encoder drm/mgag200: Use simple encoder drm/qxl: Use simple encoder
drivers/gpu/drm/ast/ast_drv.h | 6 +- drivers/gpu/drm/ast/ast_mode.c | 25 +++---- drivers/gpu/drm/drm_simple_kms_helper.c | 34 +++++++++- drivers/gpu/drm/mgag200/mgag200_drv.h | 9 +-- drivers/gpu/drm/mgag200/mgag200_mode.c | 86 ++++--------------------- drivers/gpu/drm/qxl/qxl_display.c | 29 ++++----- include/drm/drm_simple_kms_helper.h | 4 ++ 7 files changed, 71 insertions(+), 122 deletions(-)
-- 2.25.0
This patch makes the internal encoder implementation of the simple KMS helpers available to drivers.
These simple-encoder helpers initialize an encoder with an empty implementation. This covers the requirements of most of the existing DRM drivers. A call to drm_simple_encoder_create() allocates and initializes an encoder instance, a call to drm_simple_encoder_init() initializes a pre-allocated instance.
v3: * remove drm_simple_encoder_create(); not required yet * provide more precise documentation v2: * move simple encoder to KMS helpers * remove name argument; simplifies implementation * don't allocate with devm_ interfaces; unsafe with DRM
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de Reviewed-by: Sam Ravnborg sam@ravnborg.org --- drivers/gpu/drm/drm_simple_kms_helper.c | 34 ++++++++++++++++++++++--- include/drm/drm_simple_kms_helper.h | 4 +++ 2 files changed, 35 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c b/drivers/gpu/drm/drm_simple_kms_helper.c index 15fb516ae2d8..04309e4660de 100644 --- a/drivers/gpu/drm/drm_simple_kms_helper.c +++ b/drivers/gpu/drm/drm_simple_kms_helper.c @@ -26,12 +26,41 @@ * entity. Some flexibility for code reuse is provided through a separately * allocated &drm_connector object and supporting optional &drm_bridge * encoder drivers. + * + * Many drivers require only a very simple encoder that fulfills the minimum + * requirements of the display pipeline and does not add additional + * functionality. The function drm_simple_encoder_init() provides an + * implementation of such an encoder. */
-static const struct drm_encoder_funcs drm_simple_kms_encoder_funcs = { +static const struct drm_encoder_funcs drm_simple_encoder_funcs_cleanup = { .destroy = drm_encoder_cleanup, };
+/** + * drm_simple_encoder_init - Initialize a preallocated encoder + * @dev: drm device + * @funcs: callbacks for this encoder + * @encoder_type: user visible type of the encoder + * + * Initialises a preallocated encoder that has no further functionality. + * Settings for possible CRTC and clones are left to their initial values. + * The encoder will be cleaned up automatically as part of the mode-setting + * cleanup. + * + * Returns: + * Zero on success, error code on failure. + */ +int drm_simple_encoder_init(struct drm_device *dev, + struct drm_encoder *encoder, + int encoder_type) +{ + return drm_encoder_init(dev, encoder, + &drm_simple_encoder_funcs_cleanup, + encoder_type, NULL); +} +EXPORT_SYMBOL(drm_simple_encoder_init); + static enum drm_mode_status drm_simple_kms_crtc_mode_valid(struct drm_crtc *crtc, const struct drm_display_mode *mode) @@ -288,8 +317,7 @@ int drm_simple_display_pipe_init(struct drm_device *dev, return ret;
encoder->possible_crtcs = drm_crtc_mask(crtc); - ret = drm_encoder_init(dev, encoder, &drm_simple_kms_encoder_funcs, - DRM_MODE_ENCODER_NONE, NULL); + ret = drm_simple_encoder_init(dev, encoder, DRM_MODE_ENCODER_NONE); if (ret || !connector) return ret;
diff --git a/include/drm/drm_simple_kms_helper.h b/include/drm/drm_simple_kms_helper.h index e253ba7bea9d..a026375464ff 100644 --- a/include/drm/drm_simple_kms_helper.h +++ b/include/drm/drm_simple_kms_helper.h @@ -181,4 +181,8 @@ int drm_simple_display_pipe_init(struct drm_device *dev, const uint64_t *format_modifiers, struct drm_connector *connector);
+int drm_simple_encoder_init(struct drm_device *dev, + struct drm_encoder *encoder, + int encoder_type); + #endif /* __LINUX_DRM_SIMPLE_KMS_HELPER_H */
The ast driver uses an empty implementation for its encoder. Replace the code with the generic simple encoder.
v2: * rebase onto new simple-encoder interface
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de Acked-by: Sam Ravnborg sam@ravnborg.org --- drivers/gpu/drm/ast/ast_drv.h | 6 +----- drivers/gpu/drm/ast/ast_mode.c | 25 ++++++++----------------- 2 files changed, 9 insertions(+), 22 deletions(-)
diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h index f5d8780776ae..656d591b154b 100644 --- a/drivers/gpu/drm/ast/ast_drv.h +++ b/drivers/gpu/drm/ast/ast_drv.h @@ -121,6 +121,7 @@ struct ast_private { unsigned int next_index; } cursor;
+ struct drm_encoder encoder; struct drm_plane primary_plane; struct drm_plane cursor_plane;
@@ -238,13 +239,8 @@ struct ast_crtc { u8 offset_x, offset_y; };
-struct ast_encoder { - struct drm_encoder base; -}; - #define to_ast_crtc(x) container_of(x, struct ast_crtc, base) #define to_ast_connector(x) container_of(x, struct ast_connector, base) -#define to_ast_encoder(x) container_of(x, struct ast_encoder, base)
struct ast_vbios_stdtable { u8 misc; diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c index 562ea6d9df13..7a9f20a2fd30 100644 --- a/drivers/gpu/drm/ast/ast_mode.c +++ b/drivers/gpu/drm/ast/ast_mode.c @@ -40,6 +40,7 @@ #include <drm/drm_gem_vram_helper.h> #include <drm/drm_plane_helper.h> #include <drm/drm_probe_helper.h> +#include <drm/drm_simple_kms_helper.h>
#include "ast_drv.h" #include "ast_tables.h" @@ -968,28 +969,18 @@ static int ast_crtc_init(struct drm_device *dev) * Encoder */
-static void ast_encoder_destroy(struct drm_encoder *encoder) -{ - drm_encoder_cleanup(encoder); - kfree(encoder); -} - -static const struct drm_encoder_funcs ast_enc_funcs = { - .destroy = ast_encoder_destroy, -}; - static int ast_encoder_init(struct drm_device *dev) { - struct ast_encoder *ast_encoder; + struct ast_private *ast = dev->dev_private; + struct drm_encoder *encoder = &ast->encoder; + int ret;
- ast_encoder = kzalloc(sizeof(struct ast_encoder), GFP_KERNEL); - if (!ast_encoder) - return -ENOMEM; + ret = drm_simple_encoder_init(dev, encoder, DRM_MODE_ENCODER_DAC); + if (ret) + return ret;
- drm_encoder_init(dev, &ast_encoder->base, &ast_enc_funcs, - DRM_MODE_ENCODER_DAC, NULL); + encoder->possible_crtcs = 1;
- ast_encoder->base.possible_crtcs = 1; return 0; }
The mgag200 driver uses an empty implementation for its encoder. Replace the code with the generic simple encoder.
v4: * print error message with drm_err() v3: * init pre-allocated encoder with drm_simple_encoder_init() v2: * rebase onto new simple-encoder interface
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de Reviewed-by: Sam Ravnborg sam@ravnborg.org --- drivers/gpu/drm/mgag200/mgag200_drv.h | 9 +-- drivers/gpu/drm/mgag200/mgag200_mode.c | 86 ++++---------------------- 2 files changed, 13 insertions(+), 82 deletions(-)
diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h index aa32aad222c2..9691252d6233 100644 --- a/drivers/gpu/drm/mgag200/mgag200_drv.h +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h @@ -95,7 +95,6 @@ #define MATROX_DPMS_CLEARED (-1)
#define to_mga_crtc(x) container_of(x, struct mga_crtc, base) -#define to_mga_encoder(x) container_of(x, struct mga_encoder, base) #define to_mga_connector(x) container_of(x, struct mga_connector, base)
struct mga_crtc { @@ -110,12 +109,6 @@ struct mga_mode_info { struct mga_crtc *crtc; };
-struct mga_encoder { - struct drm_encoder base; - int last_dpms; -}; - - struct mga_i2c_chan { struct i2c_adapter adapter; struct drm_device *dev; @@ -185,6 +178,8 @@ struct mga_device {
/* SE model number stored in reg 0x1e24 */ u32 unique_rev_id; + + struct drm_encoder encoder; };
static inline enum mga_type diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c index 62a8e9ccb16d..d90e83959fca 100644 --- a/drivers/gpu/drm/mgag200/mgag200_mode.c +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c @@ -15,6 +15,7 @@ #include <drm/drm_fourcc.h> #include <drm/drm_plane_helper.h> #include <drm/drm_probe_helper.h> +#include <drm/drm_simple_kms_helper.h>
#include "mgag200_drv.h"
@@ -1449,76 +1450,6 @@ static void mga_crtc_init(struct mga_device *mdev) drm_crtc_helper_add(&mga_crtc->base, &mga_helper_funcs); }
-/* - * The encoder comes after the CRTC in the output pipeline, but before - * the connector. It's responsible for ensuring that the digital - * stream is appropriately converted into the output format. Setup is - * very simple in this case - all we have to do is inform qemu of the - * colour depth in order to ensure that it displays appropriately - */ - -/* - * These functions are analagous to those in the CRTC code, but are intended - * to handle any encoder-specific limitations - */ -static void mga_encoder_mode_set(struct drm_encoder *encoder, - struct drm_display_mode *mode, - struct drm_display_mode *adjusted_mode) -{ - -} - -static void mga_encoder_dpms(struct drm_encoder *encoder, int state) -{ - return; -} - -static void mga_encoder_prepare(struct drm_encoder *encoder) -{ -} - -static void mga_encoder_commit(struct drm_encoder *encoder) -{ -} - -static void mga_encoder_destroy(struct drm_encoder *encoder) -{ - struct mga_encoder *mga_encoder = to_mga_encoder(encoder); - drm_encoder_cleanup(encoder); - kfree(mga_encoder); -} - -static const struct drm_encoder_helper_funcs mga_encoder_helper_funcs = { - .dpms = mga_encoder_dpms, - .mode_set = mga_encoder_mode_set, - .prepare = mga_encoder_prepare, - .commit = mga_encoder_commit, -}; - -static const struct drm_encoder_funcs mga_encoder_encoder_funcs = { - .destroy = mga_encoder_destroy, -}; - -static struct drm_encoder *mga_encoder_init(struct drm_device *dev) -{ - struct drm_encoder *encoder; - struct mga_encoder *mga_encoder; - - mga_encoder = kzalloc(sizeof(struct mga_encoder), GFP_KERNEL); - if (!mga_encoder) - return NULL; - - encoder = &mga_encoder->base; - encoder->possible_crtcs = 0x1; - - drm_encoder_init(dev, encoder, &mga_encoder_encoder_funcs, - DRM_MODE_ENCODER_DAC, NULL); - drm_encoder_helper_add(encoder, &mga_encoder_helper_funcs); - - return encoder; -} - - static int mga_vga_get_modes(struct drm_connector *connector) { struct mga_connector *mga_connector = to_mga_connector(connector); @@ -1686,8 +1617,9 @@ static struct drm_connector *mga_vga_init(struct drm_device *dev)
int mgag200_modeset_init(struct mga_device *mdev) { - struct drm_encoder *encoder; + struct drm_encoder *encoder = &mdev->encoder; struct drm_connector *connector; + int ret;
mdev->mode_info.mode_config_initialized = true;
@@ -1698,11 +1630,15 @@ int mgag200_modeset_init(struct mga_device *mdev)
mga_crtc_init(mdev);
- encoder = mga_encoder_init(mdev->dev); - if (!encoder) { - DRM_ERROR("mga_encoder_init failed\n"); - return -1; + ret = drm_simple_encoder_init(mdev->dev, encoder, + DRM_MODE_ENCODER_DAC); + if (ret) { + drm_err(mdev->dev, + "drm_simple_encoder_init() failed, error %d\n", + ret); + return ret; } + encoder->possible_crtcs = 0x1;
connector = mga_vga_init(mdev->dev); if (!connector) {
The qxl driver uses an empty implementation for its encoder. Replace the code with the generic simple encoder.
v4: * handle errors returned from drm_simple_encoder_init() v2: * rebase onto new simple-encoder interface
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de Reviewed-by: Sam Ravnborg sam@ravnborg.org Acked-by: Gerd Hoffmann kraxel@redhat.com Signed-off-by: Thomas Zimmermann tzimmermann@suse.de --- drivers/gpu/drm/qxl/qxl_display.c | 29 ++++++++++++++--------------- 1 file changed, 14 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c index ab4f8dd00400..09583a08e141 100644 --- a/drivers/gpu/drm/qxl/qxl_display.c +++ b/drivers/gpu/drm/qxl/qxl_display.c @@ -31,6 +31,7 @@ #include <drm/drm_gem_framebuffer_helper.h> #include <drm/drm_plane_helper.h> #include <drm/drm_probe_helper.h> +#include <drm/drm_simple_kms_helper.h>
#include "qxl_drv.h" #include "qxl_object.h" @@ -1007,9 +1008,6 @@ static struct drm_encoder *qxl_best_encoder(struct drm_connector *connector) return &qxl_output->enc; }
-static const struct drm_encoder_helper_funcs qxl_enc_helper_funcs = { -}; - static const struct drm_connector_helper_funcs qxl_connector_helper_funcs = { .get_modes = qxl_conn_get_modes, .mode_valid = qxl_conn_mode_valid, @@ -1059,15 +1057,6 @@ static const struct drm_connector_funcs qxl_connector_funcs = { .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, };
-static void qxl_enc_destroy(struct drm_encoder *encoder) -{ - drm_encoder_cleanup(encoder); -} - -static const struct drm_encoder_funcs qxl_enc_funcs = { - .destroy = qxl_enc_destroy, -}; - static int qxl_mode_create_hotplug_mode_update_property(struct qxl_device *qdev) { if (qdev->hotplug_mode_update_property) @@ -1086,6 +1075,7 @@ static int qdev_output_init(struct drm_device *dev, int num_output) struct qxl_output *qxl_output; struct drm_connector *connector; struct drm_encoder *encoder; + int ret;
qxl_output = kzalloc(sizeof(struct qxl_output), GFP_KERNEL); if (!qxl_output) @@ -1098,15 +1088,19 @@ static int qdev_output_init(struct drm_device *dev, int num_output) drm_connector_init(dev, &qxl_output->base, &qxl_connector_funcs, DRM_MODE_CONNECTOR_VIRTUAL);
- drm_encoder_init(dev, &qxl_output->enc, &qxl_enc_funcs, - DRM_MODE_ENCODER_VIRTUAL, NULL); + ret = drm_simple_encoder_init(dev, &qxl_output->enc, + DRM_MODE_ENCODER_VIRTUAL); + if (ret) { + drm_err(dev, "drm_simple_encoder_init() failed, error %d\n", + ret); + goto err_drm_connector_cleanup; + }
/* we get HPD via client monitors config */ connector->polled = DRM_CONNECTOR_POLL_HPD; encoder->possible_crtcs = 1 << num_output; drm_connector_attach_encoder(&qxl_output->base, &qxl_output->enc); - drm_encoder_helper_add(encoder, &qxl_enc_helper_funcs); drm_connector_helper_add(connector, &qxl_connector_helper_funcs);
drm_object_attach_property(&connector->base, @@ -1116,6 +1110,11 @@ static int qdev_output_init(struct drm_device *dev, int num_output) drm_object_attach_property(&connector->base, dev->mode_config.suggested_y_property, 0); return 0; + +err_drm_connector_cleanup: + drm_connector_cleanup(&qxl_output->base); + kfree(qxl_output); + return ret; }
static struct drm_framebuffer *
Hi Thomas.
On Fri, Feb 28, 2020 at 09:18:24AM +0100, Thomas Zimmermann wrote:
Many DRM drivers implement an encoder with an empty implementation. This patchset adds drm_simple_encoder_init(), which drivers can use instead. Except for the destroy callback, the simple encoder's implementation is empty.
The patchset also converts 4 encoder instances to use the simple-encoder helpers. But there are at least 11 other drivers which can use the helper and I think I did not examine all drivers yet.
The patchset was smoke-tested on mgag200 by running the fbdev console and Gnome on X11.
v4:
- print error messages with drm_err() (Sam)
- qxl: handle errors of drm_simple_encoder_init() (Sam)
Looked through the patches - all looked good. IMO ready to apply.
Sam
dri-devel@lists.freedesktop.org
-
Sam Ravnborg -
Thomas Zimmermann