From: Sam Ravnborg <sam@ravnborg.org> To: Thomas Zimmermann <tzimmermann@suse.de> Cc: airlied@linux.ie, dri-devel@lists.freedesktop.org, maarten.lankhorst@linux.intel.com, mripard@kernel.org, virtualization@lists.linux-foundation.org, noralf@tronnes.org, daniel@ffwll.ch, alexander.deucher@amd.com, spice-devel@lists.freedesktop.org, emil.velikov@collabora.com Subject: Re: [PATCH v2 1/4] drm/simple-kms: Add drm_simple_encoder_{init, create}() Date: Thu, 20 Feb 2020 19:50:50 +0100 [thread overview] Message-ID: <20200220185050.GB18993@ravnborg.org> (raw) In-Reply-To: <20200218084815.2137-2-tzimmermann@suse.de> 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
WARNING: multiple messages have this Message-ID (diff)
From: Sam Ravnborg <sam@ravnborg.org> To: Thomas Zimmermann <tzimmermann@suse.de> Cc: airlied@linux.ie, dri-devel@lists.freedesktop.org, virtualization@lists.linux-foundation.org, kraxel@redhat.com, alexander.deucher@amd.com, spice-devel@lists.freedesktop.org, emil.velikov@collabora.com Subject: Re: [PATCH v2 1/4] drm/simple-kms: Add drm_simple_encoder_{init, create}() Date: Thu, 20 Feb 2020 19:50:50 +0100 [thread overview] Message-ID: <20200220185050.GB18993@ravnborg.org> (raw) In-Reply-To: <20200218084815.2137-2-tzimmermann@suse.de> 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
next prev parent reply other threads:[~2020-02-20 18:50 UTC|newest] Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top 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-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 18:42 ` Sam Ravnborg 2020-02-20 18:42 ` Sam Ravnborg 2020-02-20 18:50 ` Sam Ravnborg [this message] 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-18 8:48 ` Thomas Zimmermann 2020-02-20 19:09 ` Sam Ravnborg 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 ` Thomas Zimmermann 2020-02-20 18:56 ` Sam Ravnborg 2020-02-20 18:56 ` Sam Ravnborg 2020-02-21 7:48 ` Thomas Zimmermann 2020-02-21 7:48 ` Thomas Zimmermann 2020-02-21 19:00 ` Sam Ravnborg 2020-02-21 19:00 ` Sam Ravnborg 2020-02-21 19:03 ` Daniel Vetter 2020-02-21 19:03 ` Daniel Vetter 2020-02-18 8:48 ` [PATCH v2 4/4] drm/qxl: " Thomas Zimmermann 2020-02-18 8:48 ` Thomas Zimmermann 2020-02-20 19:10 ` Sam Ravnborg 2020-02-20 19:10 ` Sam Ravnborg 2020-02-24 8:09 ` Gerd Hoffmann 2020-02-24 8:09 ` Gerd Hoffmann
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20200220185050.GB18993@ravnborg.org \ --to=sam@ravnborg.org \ --cc=airlied@linux.ie \ --cc=alexander.deucher@amd.com \ --cc=daniel@ffwll.ch \ --cc=dri-devel@lists.freedesktop.org \ --cc=emil.velikov@collabora.com \ --cc=maarten.lankhorst@linux.intel.com \ --cc=mripard@kernel.org \ --cc=noralf@tronnes.org \ --cc=spice-devel@lists.freedesktop.org \ --cc=tzimmermann@suse.de \ --cc=virtualization@lists.linux-foundation.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.