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

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

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

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