From: Thomas Zimmermann <tzimmermann@suse.de>
To: "Noralf Trønnes" <noralf@tronnes.org>,
daniel@ffwll.ch, airlied@gmail.com, mripard@kernel.org,
maarten.lankhorst@linux.intel.com, thierry.reding@gmail.com,
sam@ravnborg.org, emma@anholt.net, david@lechnology.com,
kamlesh.gurudasani@gmail.com, javierm@redhat.com
Cc: dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 6/8] drm/mipi-dbi: Support shadow-plane state
Date: Mon, 28 Nov 2022 13:10:41 +0100 [thread overview]
Message-ID: <3c33aa53-1eb1-d6de-e769-63819c03a13c@suse.de> (raw)
In-Reply-To: <8da60926-5c19-5198-72f0-efc6a633b1b5@tronnes.org>
[-- Attachment #1.1: Type: text/plain, Size: 11115 bytes --]
Hi
Am 25.11.22 um 18:48 schrieb Noralf Trønnes:
>
>
> Den 21.11.2022 11.45, skrev Thomas Zimmermann:
>> Introduce struct drm_mipi_dbi_plane_state that contains state related to
>> MIPI DBI. It currently only inherits from struct drm_shadow_plane_state,
>> so that MIPI DBI drivers can use the vmap'ed GEM-buffer memory. Implement
>> state helpers, the {begin,end}_fb_access helpers and wire up everything.
>>
>> With this commit, MIPI DBI drivers can access the GEM object's memory
>> that is provided by shadow-plane state. The actual changes to drivers
>> are implemented separately. The new struct drm_mipi_dbi_plane was added
>> to avoid exposing struct drm_shadow_plane_state directly. The latter is
>> a detail of the actual implementation and having it in the MIPI driver
>> interface seems unintuitive.
>
> I don't understand this reasoning. The update functions still uses
> drm_shadow_plane_state in order to access ->data[0]. If you want to
> avoid exposing it, can't you add an accessor function for ->data[0]
> instead? That would actually be useful to me at least since when I first
> read the shadow plane code I didn't understand what data really was
> referring to. fb_map would have been more clear to me.
There's nothing wrong with accessing shadow-plane state directly. I
simply found it non-intuitive to leave MIPI without it's own plane-state
structure. From the perspective of a MIPI-based driver, up-casting to a
shadow-plane state feels arbitrary. Upcasting to a MIPI plane state
appears logical.
Anyway, if using the shadow-plane state without the mipi plane state is
preferred, I'll change the code accordingly.
Best regards
Thomas
>
> Noralf.
>
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>> drivers/gpu/drm/drm_mipi_dbi.c | 113 +++++++++++++++++++++++++++++++++
>> drivers/gpu/drm/tiny/ili9225.c | 5 ++
>> drivers/gpu/drm/tiny/st7586.c | 5 ++
>> include/drm/drm_mipi_dbi.h | 30 ++++++++-
>> 4 files changed, 152 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/drm_mipi_dbi.c b/drivers/gpu/drm/drm_mipi_dbi.c
>> index 40e59a3a6481e..3030344d25b48 100644
>> --- a/drivers/gpu/drm/drm_mipi_dbi.c
>> +++ b/drivers/gpu/drm/drm_mipi_dbi.c
>> @@ -436,6 +436,119 @@ void mipi_dbi_pipe_disable(struct drm_simple_display_pipe *pipe)
>> }
>> EXPORT_SYMBOL(mipi_dbi_pipe_disable);
>>
>> +/**
>> + * mipi_dbi_pipe_begin_fb_access - MIPI DBI pipe begin-access helper
>> + * @pipe: Display pipe
>> + * @plane_state: Plane state
>> + *
>> + * This function implements struct &drm_simple_display_funcs.begin_fb_access.
>> + *
>> + * See drm_gem_begin_shadow_fb_access() for details and mipi_dbi_pipe_cleanup_fb()
>> + * for cleanup.
>> + *
>> + * Returns:
>> + * 0 on success, or a negative errno code otherwise.
>> + */
>> +int mipi_dbi_pipe_begin_fb_access(struct drm_simple_display_pipe *pipe,
>> + struct drm_plane_state *plane_state)
>> +{
>> + return drm_gem_begin_shadow_fb_access(&pipe->plane, plane_state);
>> +}
>> +EXPORT_SYMBOL(mipi_dbi_pipe_begin_fb_access);
>> +
>> +/**
>> + * mipi_dbi_pipe_end_fb_access - MIPI DBI pipe end-access helper
>> + * @pipe: Display pipe
>> + * @plane_state: Plane state
>> + *
>> + * This function implements struct &drm_simple_display_funcs.end_fb_access.
>> + *
>> + * See mipi_dbi_pipe_begin_fb_access().
>> + */
>> +void mipi_dbi_pipe_end_fb_access(struct drm_simple_display_pipe *pipe,
>> + struct drm_plane_state *plane_state)
>> +{
>> + drm_gem_end_shadow_fb_access(&pipe->plane, plane_state);
>> +}
>> +EXPORT_SYMBOL(mipi_dbi_pipe_end_fb_access);
>> +
>> +/**
>> + * mipi_dbi_pipe_reset_plane - MIPI DBI plane-reset helper
>> + * @pipe: Display pipe
>> + *
>> + * This function implements struct &drm_simple_display_funcs.reset_plane
>> + * for MIPI DBI planes.
>> + */
>> +void mipi_dbi_pipe_reset_plane(struct drm_simple_display_pipe *pipe)
>> +{
>> + struct drm_plane *plane = &pipe->plane;
>> + struct mipi_dbi_plane_state *mipi_dbi_plane_state;
>> +
>> + if (plane->state) {
>> + mipi_dbi_pipe_destroy_plane_state(pipe, plane->state);
>> + plane->state = NULL; /* must be set to NULL here */
>> + }
>> +
>> + mipi_dbi_plane_state = kzalloc(sizeof(*mipi_dbi_plane_state), GFP_KERNEL);
>> + if (!mipi_dbi_plane_state)
>> + return;
>> + __drm_gem_reset_shadow_plane(plane, &mipi_dbi_plane_state->shadow_plane_state);
>> +}
>> +EXPORT_SYMBOL(mipi_dbi_pipe_reset_plane);
>> +
>> +/**
>> + * mipi_dbi_pipe_duplicate_plane_state - duplicates MIPI DBI plane state
>> + * @pipe: Display pipe
>> + *
>> + * This function implements struct &drm_simple_display_funcs.duplicate_plane_state
>> + * for MIPI DBI planes.
>> + *
>> + * See drm_gem_duplicate_shadow_plane_state() for additional details.
>> + *
>> + * Returns:
>> + * A pointer to a new plane state on success, or NULL otherwise.
>> + */
>> +struct drm_plane_state *mipi_dbi_pipe_duplicate_plane_state(struct drm_simple_display_pipe *pipe)
>> +{
>> + struct drm_plane *plane = &pipe->plane;
>> + struct drm_plane_state *plane_state = plane->state;
>> + struct mipi_dbi_plane_state *new_mipi_dbi_plane_state;
>> + struct drm_shadow_plane_state *new_shadow_plane_state;
>> +
>> + if (!plane_state)
>> + return NULL;
>> +
>> + new_mipi_dbi_plane_state = kzalloc(sizeof(*new_mipi_dbi_plane_state), GFP_KERNEL);
>> + if (!new_mipi_dbi_plane_state)
>> + return NULL;
>> + new_shadow_plane_state = &new_mipi_dbi_plane_state->shadow_plane_state;
>> +
>> + __drm_gem_duplicate_shadow_plane_state(plane, new_shadow_plane_state);
>> +
>> + return &new_shadow_plane_state->base;
>> +}
>> +EXPORT_SYMBOL(mipi_dbi_pipe_duplicate_plane_state);
>> +
>> +/**
>> + * mipi_dbi_pipe_destroy_plane_state - cleans up MIPI DBI plane state
>> + * @pipe: Display pipe
>> + * @plane_state: Plane state
>> + *
>> + * This function implements struct drm_simple_display_funcs.destroy_plane_state
>> + * for MIPI DBI planes.
>> + *
>> + * See drm_gem_destroy_shadow_plane_state() for additional details.
>> + */
>> +void mipi_dbi_pipe_destroy_plane_state(struct drm_simple_display_pipe *pipe,
>> + struct drm_plane_state *plane_state)
>> +{
>> + struct mipi_dbi_plane_state *mipi_dbi_plane_state = to_mipi_dbi_plane_state(plane_state);
>> +
>> + __drm_gem_destroy_shadow_plane_state(&mipi_dbi_plane_state->shadow_plane_state);
>> + kfree(mipi_dbi_plane_state);
>> +}
>> +EXPORT_SYMBOL(mipi_dbi_pipe_destroy_plane_state);
>> +
>> static int mipi_dbi_connector_get_modes(struct drm_connector *connector)
>> {
>> struct mipi_dbi_dev *dbidev = drm_to_mipi_dbi_dev(connector->dev);
>> diff --git a/drivers/gpu/drm/tiny/ili9225.c b/drivers/gpu/drm/tiny/ili9225.c
>> index 0115c4090f9e7..9e55ce28b4552 100644
>> --- a/drivers/gpu/drm/tiny/ili9225.c
>> +++ b/drivers/gpu/drm/tiny/ili9225.c
>> @@ -349,6 +349,11 @@ static const struct drm_simple_display_pipe_funcs ili9225_pipe_funcs = {
>> .enable = ili9225_pipe_enable,
>> .disable = ili9225_pipe_disable,
>> .update = ili9225_pipe_update,
>> + .begin_fb_access = mipi_dbi_pipe_begin_fb_access,
>> + .end_fb_access = mipi_dbi_pipe_end_fb_access,
>> + .reset_plane = mipi_dbi_pipe_reset_plane,
>> + .duplicate_plane_state = mipi_dbi_pipe_duplicate_plane_state,
>> + .destroy_plane_state = mipi_dbi_pipe_destroy_plane_state,
>> };
>>
>> static const struct drm_display_mode ili9225_mode = {
>> diff --git a/drivers/gpu/drm/tiny/st7586.c b/drivers/gpu/drm/tiny/st7586.c
>> index e773b1f2fd5f3..76b13cefc904f 100644
>> --- a/drivers/gpu/drm/tiny/st7586.c
>> +++ b/drivers/gpu/drm/tiny/st7586.c
>> @@ -277,6 +277,11 @@ static const struct drm_simple_display_pipe_funcs st7586_pipe_funcs = {
>> .enable = st7586_pipe_enable,
>> .disable = st7586_pipe_disable,
>> .update = st7586_pipe_update,
>> + .begin_fb_access = mipi_dbi_pipe_begin_fb_access,
>> + .end_fb_access = mipi_dbi_pipe_end_fb_access,
>> + .reset_plane = mipi_dbi_pipe_reset_plane,
>> + .duplicate_plane_state = mipi_dbi_pipe_duplicate_plane_state,
>> + .destroy_plane_state = mipi_dbi_pipe_destroy_plane_state,
>> };
>>
>> static const struct drm_display_mode st7586_mode = {
>> diff --git a/include/drm/drm_mipi_dbi.h b/include/drm/drm_mipi_dbi.h
>> index 36ac8495566b0..0213d4aae0326 100644
>> --- a/include/drm/drm_mipi_dbi.h
>> +++ b/include/drm/drm_mipi_dbi.h
>> @@ -10,6 +10,7 @@
>>
>> #include <linux/mutex.h>
>> #include <drm/drm_device.h>
>> +#include <drm/drm_gem_atomic_helper.h>
>> #include <drm/drm_simple_kms_helper.h>
>>
>> struct drm_rect;
>> @@ -18,6 +19,19 @@ struct iosys_map;
>> struct regulator;
>> struct spi_device;
>>
>> +/**
>> + * struct mipi_dbi_plane_state - MIPI DBI plane state
>> + */
>> +struct mipi_dbi_plane_state {
>> + struct drm_shadow_plane_state shadow_plane_state;
>> +};
>> +
>> +static inline struct mipi_dbi_plane_state *
>> +to_mipi_dbi_plane_state(struct drm_plane_state *plane_state)
>> +{
>> + return container_of(plane_state, struct mipi_dbi_plane_state, shadow_plane_state.base);
>> +}
>> +
>> /**
>> * struct mipi_dbi - MIPI DBI interface
>> */
>> @@ -164,6 +178,15 @@ void mipi_dbi_enable_flush(struct mipi_dbi_dev *dbidev,
>> struct drm_crtc_state *crtc_state,
>> struct drm_plane_state *plan_state);
>> void mipi_dbi_pipe_disable(struct drm_simple_display_pipe *pipe);
>> +int mipi_dbi_pipe_begin_fb_access(struct drm_simple_display_pipe *pipe,
>> + struct drm_plane_state *plane_state);
>> +void mipi_dbi_pipe_end_fb_access(struct drm_simple_display_pipe *pipe,
>> + struct drm_plane_state *plane_state);
>> +void mipi_dbi_pipe_reset_plane(struct drm_simple_display_pipe *pipe);
>> +struct drm_plane_state *mipi_dbi_pipe_duplicate_plane_state(struct drm_simple_display_pipe *pipe);
>> +void mipi_dbi_pipe_destroy_plane_state(struct drm_simple_display_pipe *pipe,
>> + struct drm_plane_state *plane_state);
>> +
>> void mipi_dbi_hw_reset(struct mipi_dbi *dbi);
>> bool mipi_dbi_display_is_on(struct mipi_dbi *dbi);
>> int mipi_dbi_poweron_reset(struct mipi_dbi_dev *dbidev);
>> @@ -223,6 +246,11 @@ static inline void mipi_dbi_debugfs_init(struct drm_minor *minor) {}
>> .mode_valid = mipi_dbi_pipe_mode_valid, \
>> .enable = (enable_), \
>> .disable = mipi_dbi_pipe_disable, \
>> - .update = mipi_dbi_pipe_update
>> + .update = mipi_dbi_pipe_update, \
>> + .begin_fb_access = mipi_dbi_pipe_begin_fb_access, \
>> + .end_fb_access = mipi_dbi_pipe_end_fb_access, \
>> + .reset_plane = mipi_dbi_pipe_reset_plane, \
>> + .duplicate_plane_state = mipi_dbi_pipe_duplicate_plane_state, \
>> + .destroy_plane_state = mipi_dbi_pipe_destroy_plane_state
>>
>> #endif /* __LINUX_MIPI_DBI_H */
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]
next prev parent reply other threads:[~2022-11-28 12:10 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-11-21 10:45 [PATCH 0/8] drm/mipi-dbi: Convert to shadow-plane helpers Thomas Zimmermann
2022-11-21 10:45 ` [PATCH 1/8] drm/simple-kms: Remove drm_gem_simple_display_pipe_prepare_fb() Thomas Zimmermann
2022-11-25 16:45 ` Noralf Trønnes
2022-11-21 10:45 ` [PATCH 2/8] drm/ili9225: Call MIPI DBI mode_valid helper Thomas Zimmermann
2022-11-25 16:52 ` Noralf Trønnes
2022-11-21 10:45 ` [PATCH 3/8] drm/st7586: " Thomas Zimmermann
2022-11-25 16:52 ` Noralf Trønnes
2022-11-21 10:45 ` [PATCH 4/8] drm/mipi-dbi: Initialize default driver functions with macro Thomas Zimmermann
2022-11-25 17:05 ` Noralf Trønnes
2022-11-21 10:45 ` [PATCH 5/8] drm/mipi-dbi: Prepare framebuffer copy operation in pipe-update helpers Thomas Zimmermann
2022-11-25 17:39 ` Noralf Trønnes
2022-12-02 11:27 ` Thomas Zimmermann
2022-12-02 11:50 ` Thomas Zimmermann
2022-12-02 12:22 ` Noralf Trønnes
2022-12-02 12:04 ` Noralf Trønnes
2022-11-21 10:45 ` [PATCH 6/8] drm/mipi-dbi: Support shadow-plane state Thomas Zimmermann
2022-11-25 17:48 ` Noralf Trønnes
2022-11-28 12:10 ` Thomas Zimmermann [this message]
2022-11-28 13:06 ` Noralf Trønnes
2022-11-21 10:45 ` [PATCH 7/8] drm/mipi-dbi: Use shadow-plane mappings Thomas Zimmermann
2022-11-28 13:07 ` Noralf Trønnes
2022-11-21 10:45 ` [PATCH 8/8] drm/mipi-dbi: Move drm_dev_{enter, exit}() out from fb_dirty functions Thomas Zimmermann
2022-11-25 17:56 ` [PATCH 8/8] drm/mipi-dbi: Move drm_dev_{enter,exit}() " Noralf Trønnes
2022-11-21 12:27 ` [PATCH 0/8] drm/mipi-dbi: Convert to shadow-plane helpers Noralf Trønnes
2022-11-21 12:41 ` Thomas Zimmermann
2022-11-21 15:14 ` Noralf Trønnes
2022-11-21 15:22 ` Javier Martinez Canillas
2022-11-25 18:00 ` Noralf Trønnes
2022-11-30 7:19 ` Javier Martinez Canillas
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=3c33aa53-1eb1-d6de-e769-63819c03a13c@suse.de \
--to=tzimmermann@suse.de \
--cc=airlied@gmail.com \
--cc=daniel@ffwll.ch \
--cc=david@lechnology.com \
--cc=dri-devel@lists.freedesktop.org \
--cc=emma@anholt.net \
--cc=javierm@redhat.com \
--cc=kamlesh.gurudasani@gmail.com \
--cc=maarten.lankhorst@linux.intel.com \
--cc=mripard@kernel.org \
--cc=noralf@tronnes.org \
--cc=sam@ravnborg.org \
--cc=thierry.reding@gmail.com \
/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: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).