* [PATCH v2 0/4] drm: Provide a simple encoder
@ 2020-02-18 8:48 Thomas Zimmermann
2020-02-18 8:48 ` [PATCH v2 1/4] drm/simple-kms: Add drm_simple_encoder_{init, create}() Thomas Zimmermann
` (3 more replies)
0 siblings, 4 replies; 14+ messages in thread
From: Thomas Zimmermann @ 2020-02-18 8:48 UTC (permalink / raw)
To: airlied, daniel, maarten.lankhorst, mripard, kraxel, noralf, sam,
alexander.deucher, emil.velikov
Cc: spice-devel, Thomas Zimmermann, dri-devel, virtualization
Many DRM drivers implement an encoder with an empty implementation. This
patchset adds drm_simple_encoder_init() and drm_simple_encoder_create(),
which can be used by drivers 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.
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 | 83 ++++++++++++++++++++++++-
drivers/gpu/drm/mgag200/mgag200_drv.h | 7 ---
drivers/gpu/drm/mgag200/mgag200_mode.c | 61 +-----------------
drivers/gpu/drm/qxl/qxl_display.c | 18 +-----
include/drm/drm_simple_kms_helper.h | 7 +++
7 files changed, 102 insertions(+), 105 deletions(-)
--
2.25.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 1/4] drm/simple-kms: Add drm_simple_encoder_{init, create}()
2020-02-18 8:48 [PATCH v2 0/4] drm: Provide a simple encoder Thomas Zimmermann
@ 2020-02-18 8:48 ` Thomas Zimmermann
2020-02-20 18:42 ` Sam Ravnborg
2020-02-20 18:50 ` Sam Ravnborg
2020-02-18 8:48 ` [PATCH v2 2/4] drm/ast: Use simple encoder Thomas Zimmermann
` (2 subsequent siblings)
3 siblings, 2 replies; 14+ messages in thread
From: Thomas Zimmermann @ 2020-02-18 8:48 UTC (permalink / raw)
To: airlied, daniel, maarten.lankhorst, mripard, kraxel, noralf, sam,
alexander.deucher, emil.velikov
Cc: spice-devel, Thomas Zimmermann, dri-devel, virtualization
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.
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>
---
drivers/gpu/drm/drm_simple_kms_helper.c | 83 ++++++++++++++++++++++++-
include/drm/drm_simple_kms_helper.h | 7 +++
2 files changed, 87 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..745c2f34c42b 100644
--- a/drivers/gpu/drm/drm_simple_kms_helper.c
+++ b/drivers/gpu/drm/drm_simple_kms_helper.c
@@ -26,12 +26,90 @@
* entity. Some flexibility for code reuse is provided through a separately
* allocated &drm_connector object and supporting optional &drm_bridge
* encoder drivers.
+ *
+ * Many drivers use an encoder with an empty implementation. Such encoders
+ * fulfill the minimum requirements of the display pipeline, but don't add
+ * additional functionality. The simple-encoder functions
+ * drm_simple_encoder_init() and drm_simple_encoder_create() provide an
+ * appropriate implementation.
*/
-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. The
+ * encoder will be released automatically. 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.
+ *
+ * Also see drm_simple_encoder_create().
+ *
+ * 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 void drm_encoder_destroy(struct drm_encoder *encoder)
+{
+ drm_encoder_cleanup(encoder);
+ kfree(encoder);
+}
+
+static const struct drm_encoder_funcs drm_simple_encoder_funcs_destroy = {
+ .destroy = drm_encoder_destroy,
+};
+
+/**
+ * drm_simple_encoder_create - Allocate and initialize an encoder
+ * @dev: drm device
+ * @encoder_type: user visible type of the encoder
+ *
+ * Allocates and initialises an encoder that has no further functionality. The
+ * encoder will be destroyed automatically as part of the mode-setting cleanup.
+ *
+ * See drm_simple_encoder_init() for more information.
+ *
+ * Returns:
+ * The encoder on success, a pointer-encoder error code on failure.
+ */
+struct drm_encoder *drm_simple_encoder_create(struct drm_device *dev,
+ int encoder_type)
+{
+ struct drm_encoder *encoder;
+ int ret;
+
+ encoder = kzalloc(sizeof(*encoder), GFP_KERNEL);
+ if (!encoder)
+ return ERR_PTR(-ENOMEM);
+ ret = drm_encoder_init(dev, encoder,
+ &drm_simple_encoder_funcs_destroy,
+ encoder_type, NULL);
+ if (ret)
+ goto err_kfree;
+
+ return encoder;
+
+err_kfree:
+ kfree(encoder);
+ return ERR_PTR(ret);
+}
+EXPORT_SYMBOL(drm_simple_encoder_create);
+
static enum drm_mode_status
drm_simple_kms_crtc_mode_valid(struct drm_crtc *crtc,
const struct drm_display_mode *mode)
@@ -288,8 +366,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..54d5066d90c7 100644
--- a/include/drm/drm_simple_kms_helper.h
+++ b/include/drm/drm_simple_kms_helper.h
@@ -181,4 +181,11 @@ 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);
+
+struct drm_encoder *drm_simple_encoder_create(struct drm_device *dev,
+ int encoder_type);
+
#endif /* __LINUX_DRM_SIMPLE_KMS_HELPER_H */
--
2.25.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v2 2/4] drm/ast: Use simple encoder
2020-02-18 8:48 [PATCH v2 0/4] drm: Provide a simple encoder Thomas Zimmermann
2020-02-18 8:48 ` [PATCH v2 1/4] drm/simple-kms: Add drm_simple_encoder_{init, create}() Thomas Zimmermann
@ 2020-02-18 8:48 ` Thomas Zimmermann
2020-02-20 19:09 ` Sam Ravnborg
2020-02-18 8:48 ` [PATCH v2 3/4] drm/mgag200: " Thomas Zimmermann
2020-02-18 8:48 ` [PATCH v2 4/4] drm/qxl: " Thomas Zimmermann
3 siblings, 1 reply; 14+ messages in thread
From: Thomas Zimmermann @ 2020-02-18 8:48 UTC (permalink / raw)
To: airlied, daniel, maarten.lankhorst, mripard, kraxel, noralf, sam,
alexander.deucher, emil.velikov
Cc: spice-devel, Thomas Zimmermann, dri-devel, virtualization
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>
---
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;
}
--
2.25.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v2 3/4] drm/mgag200: Use simple encoder
2020-02-18 8:48 [PATCH v2 0/4] drm: Provide a simple encoder Thomas Zimmermann
2020-02-18 8:48 ` [PATCH v2 1/4] drm/simple-kms: Add drm_simple_encoder_{init, create}() Thomas Zimmermann
2020-02-18 8:48 ` [PATCH v2 2/4] drm/ast: Use simple encoder Thomas Zimmermann
@ 2020-02-18 8:48 ` Thomas Zimmermann
2020-02-20 18:56 ` Sam Ravnborg
2020-02-18 8:48 ` [PATCH v2 4/4] drm/qxl: " Thomas Zimmermann
3 siblings, 1 reply; 14+ messages in thread
From: Thomas Zimmermann @ 2020-02-18 8:48 UTC (permalink / raw)
To: airlied, daniel, maarten.lankhorst, mripard, kraxel, noralf, sam,
alexander.deucher, emil.velikov
Cc: spice-devel, Thomas Zimmermann, dri-devel, virtualization
The mgag200 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>
---
drivers/gpu/drm/mgag200/mgag200_drv.h | 7 ---
drivers/gpu/drm/mgag200/mgag200_mode.c | 61 ++------------------------
2 files changed, 3 insertions(+), 65 deletions(-)
diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h
index aa32aad222c2..9bb9e8e14539 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;
diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
index 62a8e9ccb16d..957ea1057b6c 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,72 +1450,16 @@ 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)
+ encoder = drm_simple_encoder_create(dev, DRM_MODE_ENCODER_DAC);
+ if (IS_ERR(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;
}
--
2.25.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v2 4/4] drm/qxl: Use simple encoder
2020-02-18 8:48 [PATCH v2 0/4] drm: Provide a simple encoder Thomas Zimmermann
` (2 preceding siblings ...)
2020-02-18 8:48 ` [PATCH v2 3/4] drm/mgag200: " Thomas Zimmermann
@ 2020-02-18 8:48 ` Thomas Zimmermann
2020-02-20 19:10 ` Sam Ravnborg
2020-02-24 8:09 ` Gerd Hoffmann
3 siblings, 2 replies; 14+ messages in thread
From: Thomas Zimmermann @ 2020-02-18 8:48 UTC (permalink / raw)
To: airlied, daniel, maarten.lankhorst, mripard, kraxel, noralf, sam,
alexander.deucher, emil.velikov
Cc: spice-devel, Thomas Zimmermann, dri-devel, virtualization
The qxl 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>
---
drivers/gpu/drm/qxl/qxl_display.c | 18 +++---------------
1 file changed, 3 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c
index ab4f8dd00400..9c0e1add59fb 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)
@@ -1098,15 +1087,14 @@ 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);
+ drm_simple_encoder_init(dev, &qxl_output->enc,
+ DRM_MODE_ENCODER_VIRTUAL);
/* 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,
--
2.25.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v2 1/4] drm/simple-kms: Add drm_simple_encoder_{init, create}()
2020-02-18 8:48 ` [PATCH v2 1/4] drm/simple-kms: Add drm_simple_encoder_{init, create}() Thomas Zimmermann
@ 2020-02-20 18:42 ` Sam Ravnborg
2020-02-20 18:50 ` Sam Ravnborg
1 sibling, 0 replies; 14+ messages in thread
From: Sam Ravnborg @ 2020-02-20 18:42 UTC (permalink / raw)
To: Thomas Zimmermann
Cc: airlied, dri-devel, virtualization, kraxel, alexander.deucher,
spice-devel, emil.velikov
Hi Thomas.
On Tue, Feb 18, 2020 at 09:48:12AM +0100, Thomas Zimmermann wrote:
> 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.
>
> 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>
> ---
> drivers/gpu/drm/drm_simple_kms_helper.c | 83 ++++++++++++++++++++++++-
> include/drm/drm_simple_kms_helper.h | 7 +++
> 2 files changed, 87 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..745c2f34c42b 100644
> --- a/drivers/gpu/drm/drm_simple_kms_helper.c
> +++ b/drivers/gpu/drm/drm_simple_kms_helper.c
> @@ -26,12 +26,90 @@
> * entity. Some flexibility for code reuse is provided through a separately
> * allocated &drm_connector object and supporting optional &drm_bridge
> * encoder drivers.
> + *
> + * Many drivers use an encoder with an empty implementation. Such encoders
> + * fulfill the minimum requirements of the display pipeline, but don't add
> + * additional functionality. The simple-encoder functions
> + * drm_simple_encoder_init() and drm_simple_encoder_create() provide an
> + * appropriate implementation.
This paragraph reads a bit strange to me - I read this as a
justification for addding a generic encoded that can be used by exisitg
drivers.
How about something like this:
Many drivers requires a very simple encoder that only fullfill
the minimum requirements of the display pipeline and do not add
any extra functionslity.
The simple-encoder functions drm_simple_encoder_init() and
drm_simple_encoder_create() provides an impålmentation of such
a simple encoder.
The simple encoder includes automatically release of resources.
And then leave it to the changelog to tell what should be done in
existing drivers.
> */
>
> -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. The
> + * encoder will be released automatically. 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.
> + *
> + * Also see drm_simple_encoder_create().
s/Also see/See also/??
> + *
> + * 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 void drm_encoder_destroy(struct drm_encoder *encoder)
> +{
> + drm_encoder_cleanup(encoder);
> + kfree(encoder);
> +}
> +
> +static const struct drm_encoder_funcs drm_simple_encoder_funcs_destroy = {
> + .destroy = drm_encoder_destroy,
> +};
> +
> +/**
> + * drm_simple_encoder_create - Allocate and initialize an encoder
> + * @dev: drm device
> + * @encoder_type: user visible type of the encoder
> + *
> + * Allocates and initialises an encoder that has no further functionality. The
> + * encoder will be destroyed automatically as part of the mode-setting cleanup.
> + *
> + * See drm_simple_encoder_init() for more information.
> + *
> + * Returns:
> + * The encoder on success, a pointer-encoder error code on failure.
pointer-encoded?
> + */
> +struct drm_encoder *drm_simple_encoder_create(struct drm_device *dev,
> + int encoder_type)
> +{
> + struct drm_encoder *encoder;
> + int ret;
> +
> + encoder = kzalloc(sizeof(*encoder), GFP_KERNEL);
> + if (!encoder)
> + return ERR_PTR(-ENOMEM);
> + ret = drm_encoder_init(dev, encoder,
> + &drm_simple_encoder_funcs_destroy,
> + encoder_type, NULL);
> + if (ret)
> + goto err_kfree;
> +
> + return encoder;
> +
> +err_kfree:
> + kfree(encoder);
> + return ERR_PTR(ret);
> +}
> +EXPORT_SYMBOL(drm_simple_encoder_create);
> +
> static enum drm_mode_status
> drm_simple_kms_crtc_mode_valid(struct drm_crtc *crtc,
> const struct drm_display_mode *mode)
> @@ -288,8 +366,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..54d5066d90c7 100644
> --- a/include/drm/drm_simple_kms_helper.h
> +++ b/include/drm/drm_simple_kms_helper.h
> @@ -181,4 +181,11 @@ 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);
> +
> +struct drm_encoder *drm_simple_encoder_create(struct drm_device *dev,
> + int encoder_type);
> +
> #endif /* __LINUX_DRM_SIMPLE_KMS_HELPER_H */
Sam
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 1/4] drm/simple-kms: Add drm_simple_encoder_{init, create}()
2020-02-18 8:48 ` [PATCH v2 1/4] drm/simple-kms: Add drm_simple_encoder_{init, create}() Thomas Zimmermann
2020-02-20 18:42 ` Sam Ravnborg
@ 2020-02-20 18:50 ` Sam Ravnborg
1 sibling, 0 replies; 14+ messages in thread
From: Sam Ravnborg @ 2020-02-20 18:50 UTC (permalink / raw)
To: Thomas Zimmermann
Cc: airlied, dri-devel, virtualization, kraxel, alexander.deucher,
spice-devel, emil.velikov
Hi Thomas.
On Tue, Feb 18, 2020 at 09:48:12AM +0100, Thomas Zimmermann wrote:
> 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.
>
> 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>
> ---
> drivers/gpu/drm/drm_simple_kms_helper.c | 83 ++++++++++++++++++++++++-
> include/drm/drm_simple_kms_helper.h | 7 +++
> 2 files changed, 87 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..745c2f34c42b 100644
> --- a/drivers/gpu/drm/drm_simple_kms_helper.c
> +++ b/drivers/gpu/drm/drm_simple_kms_helper.c
> @@ -26,12 +26,90 @@
> * entity. Some flexibility for code reuse is provided through a separately
> * allocated &drm_connector object and supporting optional &drm_bridge
> * encoder drivers.
> + *
> + * Many drivers use an encoder with an empty implementation. Such encoders
> + * fulfill the minimum requirements of the display pipeline, but don't add
> + * additional functionality. The simple-encoder functions
> + * drm_simple_encoder_init() and drm_simple_encoder_create() provide an
> + * appropriate implementation.
> */
>
> -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. The
> + * encoder will be released automatically.
I got confused here. The comment says the encoder is automatically
released. But in this case we have a preallocated encoder (maybe
embedded in ast_private or something.
So the encoder is - as I understnad it - not released. But it is cleaned
up as it is also documented in the next sentences.
Sorry if I am dense, just returned from some travelling...
Sam
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.
> + *
> + * Also see drm_simple_encoder_create().
> + *
> + * 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 void drm_encoder_destroy(struct drm_encoder *encoder)
> +{
> + drm_encoder_cleanup(encoder);
> + kfree(encoder);
> +}
> +
> +static const struct drm_encoder_funcs drm_simple_encoder_funcs_destroy = {
> + .destroy = drm_encoder_destroy,
> +};
> +
> +/**
> + * drm_simple_encoder_create - Allocate and initialize an encoder
> + * @dev: drm device
> + * @encoder_type: user visible type of the encoder
> + *
> + * Allocates and initialises an encoder that has no further functionality. The
> + * encoder will be destroyed automatically as part of the mode-setting cleanup.
> + *
> + * See drm_simple_encoder_init() for more information.
> + *
> + * Returns:
> + * The encoder on success, a pointer-encoder error code on failure.
> + */
> +struct drm_encoder *drm_simple_encoder_create(struct drm_device *dev,
> + int encoder_type)
> +{
> + struct drm_encoder *encoder;
> + int ret;
> +
> + encoder = kzalloc(sizeof(*encoder), GFP_KERNEL);
> + if (!encoder)
> + return ERR_PTR(-ENOMEM);
> + ret = drm_encoder_init(dev, encoder,
> + &drm_simple_encoder_funcs_destroy,
> + encoder_type, NULL);
> + if (ret)
> + goto err_kfree;
> +
> + return encoder;
> +
> +err_kfree:
> + kfree(encoder);
> + return ERR_PTR(ret);
> +}
> +EXPORT_SYMBOL(drm_simple_encoder_create);
> +
> static enum drm_mode_status
> drm_simple_kms_crtc_mode_valid(struct drm_crtc *crtc,
> const struct drm_display_mode *mode)
> @@ -288,8 +366,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..54d5066d90c7 100644
> --- a/include/drm/drm_simple_kms_helper.h
> +++ b/include/drm/drm_simple_kms_helper.h
> @@ -181,4 +181,11 @@ 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);
> +
> +struct drm_encoder *drm_simple_encoder_create(struct drm_device *dev,
> + int encoder_type);
> +
> #endif /* __LINUX_DRM_SIMPLE_KMS_HELPER_H */
> --
> 2.25.0
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 3/4] drm/mgag200: Use simple encoder
2020-02-18 8:48 ` [PATCH v2 3/4] drm/mgag200: " Thomas Zimmermann
@ 2020-02-20 18:56 ` Sam Ravnborg
2020-02-21 7:48 ` Thomas Zimmermann
0 siblings, 1 reply; 14+ messages in thread
From: Sam Ravnborg @ 2020-02-20 18:56 UTC (permalink / raw)
To: Thomas Zimmermann
Cc: airlied, dri-devel, virtualization, kraxel, alexander.deucher,
spice-devel, emil.velikov
Hi Thomas.
On Tue, Feb 18, 2020 at 09:48:14AM +0100, Thomas Zimmermann wrote:
> The mgag200 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>
> ---
> drivers/gpu/drm/mgag200/mgag200_drv.h | 7 ---
> drivers/gpu/drm/mgag200/mgag200_mode.c | 61 ++------------------------
> 2 files changed, 3 insertions(+), 65 deletions(-)
>
> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h
> index aa32aad222c2..9bb9e8e14539 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;
Any particular reason why the drm_encoder is not embedded in struct
mga_device?
I found it more elegant - like you did it for ast in the previous patch.
I also noted there is one more unused "last_dpms" - but it is outside
the scope of this patch to remove it.
Sam
> diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
> index 62a8e9ccb16d..957ea1057b6c 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,72 +1450,16 @@ 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)
> + encoder = drm_simple_encoder_create(dev, DRM_MODE_ENCODER_DAC);
> + if (IS_ERR(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;
> }
>
> --
> 2.25.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 2/4] drm/ast: Use simple encoder
2020-02-18 8:48 ` [PATCH v2 2/4] drm/ast: Use simple encoder Thomas Zimmermann
@ 2020-02-20 19:09 ` Sam Ravnborg
0 siblings, 0 replies; 14+ messages in thread
From: Sam Ravnborg @ 2020-02-20 19:09 UTC (permalink / raw)
To: Thomas Zimmermann
Cc: airlied, dri-devel, virtualization, kraxel, alexander.deucher,
spice-devel, emil.velikov
Hi Thomas.
On Tue, Feb 18, 2020 at 09:48:13AM +0100, Thomas Zimmermann wrote:
> 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>
From browsign the code - looks good:
Acked-by: Sam Ravnborg <sam@ravnborg.org>
Sam
> ---
> 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;
> }
>
> --
> 2.25.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 4/4] drm/qxl: Use simple encoder
2020-02-18 8:48 ` [PATCH v2 4/4] drm/qxl: " Thomas Zimmermann
@ 2020-02-20 19:10 ` Sam Ravnborg
2020-02-24 8:09 ` Gerd Hoffmann
1 sibling, 0 replies; 14+ messages in thread
From: Sam Ravnborg @ 2020-02-20 19:10 UTC (permalink / raw)
To: Thomas Zimmermann
Cc: airlied, dri-devel, virtualization, kraxel, alexander.deucher,
spice-devel, emil.velikov
Hi Thomas.
On Tue, Feb 18, 2020 at 09:48:15AM +0100, Thomas Zimmermann wrote:
> The qxl 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>
I looked at best_encoder - but could not see we could do anything.
So from browsing the code:
Acked-by: Sam Ravnborg <sam@ravnborg.org>
Sam
> ---
> drivers/gpu/drm/qxl/qxl_display.c | 18 +++---------------
> 1 file changed, 3 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c
> index ab4f8dd00400..9c0e1add59fb 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)
> @@ -1098,15 +1087,14 @@ 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);
> + drm_simple_encoder_init(dev, &qxl_output->enc,
> + DRM_MODE_ENCODER_VIRTUAL);
>
> /* 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,
> --
> 2.25.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 3/4] drm/mgag200: Use simple encoder
2020-02-20 18:56 ` Sam Ravnborg
@ 2020-02-21 7:48 ` Thomas Zimmermann
2020-02-21 19:00 ` Sam Ravnborg
0 siblings, 1 reply; 14+ messages in thread
From: Thomas Zimmermann @ 2020-02-21 7:48 UTC (permalink / raw)
To: Sam Ravnborg
Cc: airlied, dri-devel, virtualization, kraxel, alexander.deucher,
spice-devel, emil.velikov
[-- Attachment #1.1.1: Type: text/plain, Size: 4987 bytes --]
Hi Sam
thanks for reviewing the patch set.
Am 20.02.20 um 19:56 schrieb Sam Ravnborg:
> Hi Thomas.
>
> On Tue, Feb 18, 2020 at 09:48:14AM +0100, Thomas Zimmermann wrote:
>> The mgag200 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>
>> ---
>> drivers/gpu/drm/mgag200/mgag200_drv.h | 7 ---
>> drivers/gpu/drm/mgag200/mgag200_mode.c | 61 ++------------------------
>> 2 files changed, 3 insertions(+), 65 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h
>> index aa32aad222c2..9bb9e8e14539 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;
>
> Any particular reason why the drm_encoder is not embedded in struct
> mga_device?
>
> I found it more elegant - like you did it for ast in the previous patch.
I think I wanted something that uses drm_simple_encoder_create(). But I
can change that. The embedded variant is indeed better.
Best regards
Thomas
>
> I also noted there is one more unused "last_dpms" - but it is outside
> the scope of this patch to remove it.
>
> Sam
>
>> diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
>> index 62a8e9ccb16d..957ea1057b6c 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,72 +1450,16 @@ 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)
>> + encoder = drm_simple_encoder_create(dev, DRM_MODE_ENCODER_DAC);
>> + if (IS_ERR(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;
>> }
>>
>> --
>> 2.25.0
--
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: 160 bytes --]
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 3/4] drm/mgag200: Use simple encoder
2020-02-21 7:48 ` Thomas Zimmermann
@ 2020-02-21 19:00 ` Sam Ravnborg
2020-02-21 19:03 ` Daniel Vetter
0 siblings, 1 reply; 14+ messages in thread
From: Sam Ravnborg @ 2020-02-21 19:00 UTC (permalink / raw)
To: Thomas Zimmermann
Cc: airlied, dri-devel, virtualization, kraxel, alexander.deucher,
spice-devel, emil.velikov
Hi Thomas.
On Fri, Feb 21, 2020 at 08:48:48AM +0100, Thomas Zimmermann wrote:
> Hi Sam
>
> thanks for reviewing the patch set.
>
> Am 20.02.20 um 19:56 schrieb Sam Ravnborg:
> > Hi Thomas.
> >
> > On Tue, Feb 18, 2020 at 09:48:14AM +0100, Thomas Zimmermann wrote:
> >> The mgag200 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>
> >> ---
> >> drivers/gpu/drm/mgag200/mgag200_drv.h | 7 ---
> >> drivers/gpu/drm/mgag200/mgag200_mode.c | 61 ++------------------------
> >> 2 files changed, 3 insertions(+), 65 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h
> >> index aa32aad222c2..9bb9e8e14539 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;
> >
> > Any particular reason why the drm_encoder is not embedded in struct
> > mga_device?
> >
> > I found it more elegant - like you did it for ast in the previous patch.
>
> I think I wanted something that uses drm_simple_encoder_create(). But I
> can change that. The embedded variant is indeed better.
You should consider to drop drm_simple_encoder_create() until there
is a driver that really needs it.
Sam
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 3/4] drm/mgag200: Use simple encoder
2020-02-21 19:00 ` Sam Ravnborg
@ 2020-02-21 19:03 ` Daniel Vetter
0 siblings, 0 replies; 14+ messages in thread
From: Daniel Vetter @ 2020-02-21 19:03 UTC (permalink / raw)
To: Sam Ravnborg
Cc: airlied, dri-devel, virtualization, kraxel, Thomas Zimmermann,
alexander.deucher, spice-devel, emil.velikov
On Fri, Feb 21, 2020 at 08:00:57PM +0100, Sam Ravnborg wrote:
> Hi Thomas.
>
> On Fri, Feb 21, 2020 at 08:48:48AM +0100, Thomas Zimmermann wrote:
> > Hi Sam
> >
> > thanks for reviewing the patch set.
> >
> > Am 20.02.20 um 19:56 schrieb Sam Ravnborg:
> > > Hi Thomas.
> > >
> > > On Tue, Feb 18, 2020 at 09:48:14AM +0100, Thomas Zimmermann wrote:
> > >> The mgag200 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>
> > >> ---
> > >> drivers/gpu/drm/mgag200/mgag200_drv.h | 7 ---
> > >> drivers/gpu/drm/mgag200/mgag200_mode.c | 61 ++------------------------
> > >> 2 files changed, 3 insertions(+), 65 deletions(-)
> > >>
> > >> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h
> > >> index aa32aad222c2..9bb9e8e14539 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;
> > >
> > > Any particular reason why the drm_encoder is not embedded in struct
> > > mga_device?
> > >
> > > I found it more elegant - like you did it for ast in the previous patch.
> >
> > I think I wanted something that uses drm_simple_encoder_create(). But I
> > can change that. The embedded variant is indeed better.
>
> You should consider to drop drm_simple_encoder_create() until there
> is a driver that really needs it.
Yeah +1 on only the _init version. The create version really should use
drmm_kzalloc I think, but we're not quite there yet :-)
-Daniel
--
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 4/4] drm/qxl: Use simple encoder
2020-02-18 8:48 ` [PATCH v2 4/4] drm/qxl: " Thomas Zimmermann
2020-02-20 19:10 ` Sam Ravnborg
@ 2020-02-24 8:09 ` Gerd Hoffmann
1 sibling, 0 replies; 14+ messages in thread
From: Gerd Hoffmann @ 2020-02-24 8:09 UTC (permalink / raw)
To: Thomas Zimmermann
Cc: airlied, virtualization, dri-devel, alexander.deucher,
spice-devel, sam, emil.velikov
On Tue, Feb 18, 2020 at 09:48:15AM +0100, Thomas Zimmermann wrote:
> The qxl 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: Gerd Hoffmann <kraxel@redhat.com>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2020-02-24 8:09 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-18 8:48 [PATCH v2 0/4] drm: Provide a simple encoder Thomas Zimmermann
2020-02-18 8:48 ` [PATCH v2 1/4] drm/simple-kms: Add drm_simple_encoder_{init, create}() Thomas Zimmermann
2020-02-20 18:42 ` Sam Ravnborg
2020-02-20 18:50 ` Sam Ravnborg
2020-02-18 8:48 ` [PATCH v2 2/4] drm/ast: Use simple encoder Thomas Zimmermann
2020-02-20 19:09 ` Sam Ravnborg
2020-02-18 8:48 ` [PATCH v2 3/4] drm/mgag200: " Thomas Zimmermann
2020-02-20 18:56 ` Sam Ravnborg
2020-02-21 7:48 ` Thomas Zimmermann
2020-02-21 19:00 ` Sam Ravnborg
2020-02-21 19:03 ` Daniel Vetter
2020-02-18 8:48 ` [PATCH v2 4/4] drm/qxl: " Thomas Zimmermann
2020-02-20 19:10 ` Sam Ravnborg
2020-02-24 8:09 ` Gerd Hoffmann
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).