[PATCH 5/7] drm/plane-helper: Export individual helpers
Thomas Zimmermann
tzimmermann at suse.de
Thu Aug 11 18:32:19 UTC 2022
Hi
Am 11.08.22 um 18:41 schrieb Daniel Vetter:
> On Wed, Jul 20, 2022 at 10:30:56AM +0200, Thomas Zimmermann wrote:
>> Export the individual plane helpers that make up the plane functions and
>> align the naming with other helpers. The plane helpers are for non-atomic
>> modesetting and exporting them will simplify a later conversion of drivers
>> to atomic modesetting.
>>
>> With struct drm_plane_funcs removed from drm_plane_helper.h, also remove
>> the include statements. It only needs linux/types.h for uint32_t and a
>> number of forward declarations.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann at suse.de>
>
> So my commit message was maybe not super motivated, but I intentionally
> hid these because atomic drivers where using them, which is all bad no
> good. Now they're more exposed again, which sucks a bit. Exporting the
> complete function table for legacy helpers (and the one open-coded one in
> nouveau, not sure we could not move that back to drm_crtc_init) feels much
> safer against abuse to me.
>
> I'm not entirely clear why we're doing this, because the include untangle
> could also have been achieved with a struct forward decl, which is what
> we're usually doing. Can we walk this back please, or am I missing
> something here?
I don't think you miss anything. It's just ugly to export the complete
funcs table. If we roll that change back, let's add a comment that
states the rational you wrote here.
Best regards
Thomas
>
> Cheers, Daniel
>
>> ---
>> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 3 +-
>> drivers/gpu/drm/armada/armada_plane.c | 2 +-
>> drivers/gpu/drm/drm_modeset_helper.c | 8 ++-
>> drivers/gpu/drm/drm_plane_helper.c | 70 ++++++++++++++-----
>> drivers/gpu/drm/nouveau/dispnv04/crtc.c | 8 ++-
>> drivers/gpu/drm/qxl/qxl_display.c | 4 +-
>> drivers/gpu/drm/vboxvideo/vbox_mode.c | 4 +-
>> include/drm/drm_plane_helper.h | 22 ++++--
>> 8 files changed, 88 insertions(+), 33 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> index 3e83fed540e8..1548f0a1b1c0 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> @@ -86,6 +86,7 @@
>> #include <drm/drm_vblank.h>
>> #include <drm/drm_audio_component.h>
>> #include <drm/drm_gem_atomic_helper.h>
>> +#include <drm/drm_plane_helper.h>
>>
>> #include "ivsrcid/dcn/irqsrcs_dcn_1_0.h"
>>
>> @@ -7824,7 +7825,7 @@ static void dm_drm_plane_destroy_state(struct drm_plane *plane,
>> static const struct drm_plane_funcs dm_plane_funcs = {
>> .update_plane = drm_atomic_helper_update_plane,
>> .disable_plane = drm_atomic_helper_disable_plane,
>> - .destroy = drm_primary_helper_destroy,
>> + .destroy = drm_plane_helper_destroy,
>> .reset = dm_drm_plane_reset,
>> .atomic_duplicate_state = dm_drm_plane_duplicate_state,
>> .atomic_destroy_state = dm_drm_plane_destroy_state,
>> diff --git a/drivers/gpu/drm/armada/armada_plane.c b/drivers/gpu/drm/armada/armada_plane.c
>> index 959d7f0a5108..cc47c032dbc1 100644
>> --- a/drivers/gpu/drm/armada/armada_plane.c
>> +++ b/drivers/gpu/drm/armada/armada_plane.c
>> @@ -288,7 +288,7 @@ struct drm_plane_state *armada_plane_duplicate_state(struct drm_plane *plane)
>> static const struct drm_plane_funcs armada_primary_plane_funcs = {
>> .update_plane = drm_atomic_helper_update_plane,
>> .disable_plane = drm_atomic_helper_disable_plane,
>> - .destroy = drm_primary_helper_destroy,
>> + .destroy = drm_plane_helper_destroy,
>> .reset = armada_plane_reset,
>> .atomic_duplicate_state = armada_plane_duplicate_state,
>> .atomic_destroy_state = drm_atomic_helper_plane_destroy_state,
>> diff --git a/drivers/gpu/drm/drm_modeset_helper.c b/drivers/gpu/drm/drm_modeset_helper.c
>> index 0f08319453b2..bd609a978848 100644
>> --- a/drivers/gpu/drm/drm_modeset_helper.c
>> +++ b/drivers/gpu/drm/drm_modeset_helper.c
>> @@ -108,6 +108,12 @@ static const uint32_t safe_modeset_formats[] = {
>> DRM_FORMAT_ARGB8888,
>> };
>>
>> +static const struct drm_plane_funcs primary_plane_funcs = {
>> + .update_plane = drm_plane_helper_update_primary,
>> + .disable_plane = drm_plane_helper_disable_primary,
>> + .destroy = drm_plane_helper_destroy,
>> +};
>> +
>> static struct drm_plane *create_primary_plane(struct drm_device *dev)
>> {
>> struct drm_plane *primary;
>> @@ -127,7 +133,7 @@ static struct drm_plane *create_primary_plane(struct drm_device *dev)
>>
>> /* possible_crtc's will be filled in later by crtc_init */
>> ret = drm_universal_plane_init(dev, primary, 0,
>> - &drm_primary_helper_funcs,
>> + &primary_plane_funcs,
>> safe_modeset_formats,
>> ARRAY_SIZE(safe_modeset_formats),
>> NULL,
>> diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c
>> index bc08edd69030..c7785967f5bf 100644
>> --- a/drivers/gpu/drm/drm_plane_helper.c
>> +++ b/drivers/gpu/drm/drm_plane_helper.c
>> @@ -145,13 +145,36 @@ static int drm_plane_helper_check_update(struct drm_plane *plane,
>> return 0;
>> }
>>
>> -static int drm_primary_helper_update(struct drm_plane *plane, struct drm_crtc *crtc,
>> - struct drm_framebuffer *fb,
>> - int crtc_x, int crtc_y,
>> - unsigned int crtc_w, unsigned int crtc_h,
>> - uint32_t src_x, uint32_t src_y,
>> - uint32_t src_w, uint32_t src_h,
>> - struct drm_modeset_acquire_ctx *ctx)
>> +/**
>> + * drm_plane_helper_update_primary - Helper for updating primary planes
>> + * @plane: plane to update
>> + * @crtc: the plane's new CRTC
>> + * @fb: the plane's new framebuffer
>> + * @crtc_x: x coordinate within CRTC
>> + * @crtc_y: y coordinate within CRTC
>> + * @crtc_w: width coordinate within CRTC
>> + * @crtc_h: height coordinate within CRTC
>> + * @src_x: x coordinate within source
>> + * @src_y: y coordinate within source
>> + * @src_w: width coordinate within source
>> + * @src_h: height coordinate within source
>> + * @ctx: modeset locking context
>> + *
>> + * This helper validates the given parameters and updates the primary plane.
>> + *
>> + * This function is only useful for non-atomic modesetting. Don't use
>> + * it in new drivers.
>> + *
>> + * Returns:
>> + * Zero on success, or an errno code otherwise.
>> + */
>> +int drm_plane_helper_update_primary(struct drm_plane *plane, struct drm_crtc *crtc,
>> + struct drm_framebuffer *fb,
>> + int crtc_x, int crtc_y,
>> + unsigned int crtc_w, unsigned int crtc_h,
>> + uint32_t src_x, uint32_t src_y,
>> + uint32_t src_w, uint32_t src_h,
>> + struct drm_modeset_acquire_ctx *ctx)
>> {
>> struct drm_mode_set set = {
>> .crtc = crtc,
>> @@ -218,31 +241,40 @@ static int drm_primary_helper_update(struct drm_plane *plane, struct drm_crtc *c
>> kfree(connector_list);
>> return ret;
>> }
>> +EXPORT_SYMBOL(drm_plane_helper_update_primary);
>>
>> -static int drm_primary_helper_disable(struct drm_plane *plane,
>> - struct drm_modeset_acquire_ctx *ctx)
>> +/**
>> + * drm_plane_helper_disable_primary - Helper for disabling primary planes
>> + * @plane: plane to disable
>> + * @ctx: modeset locking context
>> + *
>> + * This helper returns an error when trying to disable the primary
>> + * plane.
>> + *
>> + * This function is only useful for non-atomic modesetting. Don't use
>> + * it in new drivers.
>> + *
>> + * Returns:
>> + * An errno code.
>> + */
>> +int drm_plane_helper_disable_primary(struct drm_plane *plane,
>> + struct drm_modeset_acquire_ctx *ctx)
>> {
>> return -EINVAL;
>> }
>> +EXPORT_SYMBOL(drm_plane_helper_disable_primary);
>>
>> /**
>> - * drm_primary_helper_destroy() - Helper for primary plane destruction
>> + * drm_plane_helper_destroy() - Helper for primary plane destruction
>> * @plane: plane to destroy
>> *
>> * Provides a default plane destroy handler for primary planes. This handler
>> * is called during CRTC destruction. We disable the primary plane, remove
>> * it from the DRM plane list, and deallocate the plane structure.
>> */
>> -void drm_primary_helper_destroy(struct drm_plane *plane)
>> +void drm_plane_helper_destroy(struct drm_plane *plane)
>> {
>> drm_plane_cleanup(plane);
>> kfree(plane);
>> }
>> -EXPORT_SYMBOL(drm_primary_helper_destroy);
>> -
>> -const struct drm_plane_funcs drm_primary_helper_funcs = {
>> - .update_plane = drm_primary_helper_update,
>> - .disable_plane = drm_primary_helper_disable,
>> - .destroy = drm_primary_helper_destroy,
>> -};
>> -EXPORT_SYMBOL(drm_primary_helper_funcs);
>> +EXPORT_SYMBOL(drm_plane_helper_destroy);
>> diff --git a/drivers/gpu/drm/nouveau/dispnv04/crtc.c b/drivers/gpu/drm/nouveau/dispnv04/crtc.c
>> index f9e962fd94d0..660c4cbc0b3d 100644
>> --- a/drivers/gpu/drm/nouveau/dispnv04/crtc.c
>> +++ b/drivers/gpu/drm/nouveau/dispnv04/crtc.c
>> @@ -1275,6 +1275,12 @@ static const uint32_t modeset_formats[] = {
>> DRM_FORMAT_XRGB1555,
>> };
>>
>> +static const struct drm_plane_funcs nv04_primary_plane_funcs = {
>> + .update_plane = drm_plane_helper_update_primary,
>> + .disable_plane = drm_plane_helper_disable_primary,
>> + .destroy = drm_plane_helper_destroy,
>> +};
>> +
>> static struct drm_plane *
>> create_primary_plane(struct drm_device *dev)
>> {
>> @@ -1289,7 +1295,7 @@ create_primary_plane(struct drm_device *dev)
>>
>> /* possible_crtc's will be filled in later by crtc_init */
>> ret = drm_universal_plane_init(dev, primary, 0,
>> - &drm_primary_helper_funcs,
>> + &nv04_primary_plane_funcs,
>> modeset_formats,
>> ARRAY_SIZE(modeset_formats), NULL,
>> DRM_PLANE_TYPE_PRIMARY, NULL);
>> diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c
>> index 2e8949863d6b..a152a7c6db21 100644
>> --- a/drivers/gpu/drm/qxl/qxl_display.c
>> +++ b/drivers/gpu/drm/qxl/qxl_display.c
>> @@ -902,7 +902,7 @@ static const struct drm_plane_helper_funcs qxl_cursor_helper_funcs = {
>> static const struct drm_plane_funcs qxl_cursor_plane_funcs = {
>> .update_plane = drm_atomic_helper_update_plane,
>> .disable_plane = drm_atomic_helper_disable_plane,
>> - .destroy = drm_primary_helper_destroy,
>> + .destroy = drm_plane_helper_destroy,
>> .reset = drm_atomic_helper_plane_reset,
>> .atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
>> .atomic_destroy_state = drm_atomic_helper_plane_destroy_state,
>> @@ -924,7 +924,7 @@ static const struct drm_plane_helper_funcs primary_helper_funcs = {
>> static const struct drm_plane_funcs qxl_primary_plane_funcs = {
>> .update_plane = drm_atomic_helper_update_plane,
>> .disable_plane = drm_atomic_helper_disable_plane,
>> - .destroy = drm_primary_helper_destroy,
>> + .destroy = drm_plane_helper_destroy,
>> .reset = drm_atomic_helper_plane_reset,
>> .atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
>> .atomic_destroy_state = drm_atomic_helper_plane_destroy_state,
>> diff --git a/drivers/gpu/drm/vboxvideo/vbox_mode.c b/drivers/gpu/drm/vboxvideo/vbox_mode.c
>> index 604ebfbef314..341edd982cb3 100644
>> --- a/drivers/gpu/drm/vboxvideo/vbox_mode.c
>> +++ b/drivers/gpu/drm/vboxvideo/vbox_mode.c
>> @@ -477,7 +477,7 @@ static const struct drm_plane_helper_funcs vbox_cursor_helper_funcs = {
>> static const struct drm_plane_funcs vbox_cursor_plane_funcs = {
>> .update_plane = drm_atomic_helper_update_plane,
>> .disable_plane = drm_atomic_helper_disable_plane,
>> - .destroy = drm_primary_helper_destroy,
>> + .destroy = drm_plane_helper_destroy,
>> DRM_GEM_SHADOW_PLANE_FUNCS,
>> };
>>
>> @@ -496,7 +496,7 @@ static const struct drm_plane_helper_funcs vbox_primary_helper_funcs = {
>> static const struct drm_plane_funcs vbox_primary_plane_funcs = {
>> .update_plane = drm_atomic_helper_update_plane,
>> .disable_plane = drm_atomic_helper_disable_plane,
>> - .destroy = drm_primary_helper_destroy,
>> + .destroy = drm_plane_helper_destroy,
>> .reset = drm_atomic_helper_plane_reset,
>> .atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
>> .atomic_destroy_state = drm_atomic_helper_plane_destroy_state,
>> diff --git a/include/drm/drm_plane_helper.h b/include/drm/drm_plane_helper.h
>> index ff85ef41cb33..1781fab24dd6 100644
>> --- a/include/drm/drm_plane_helper.h
>> +++ b/include/drm/drm_plane_helper.h
>> @@ -24,12 +24,22 @@
>> #ifndef DRM_PLANE_HELPER_H
>> #define DRM_PLANE_HELPER_H
>>
>> -#include <drm/drm_rect.h>
>> -#include <drm/drm_crtc.h>
>> -#include <drm/drm_modeset_helper_vtables.h>
>> -#include <drm/drm_modeset_helper.h>
>> +#include <linux/types.h>
>>
>> -void drm_primary_helper_destroy(struct drm_plane *plane);
>> -extern const struct drm_plane_funcs drm_primary_helper_funcs;
>> +struct drm_crtc;
>> +struct drm_framebuffer;
>> +struct drm_modeset_acquire_ctx;
>> +struct drm_plane;
>> +
>> +int drm_plane_helper_update_primary(struct drm_plane *plane, struct drm_crtc *crtc,
>> + struct drm_framebuffer *fb,
>> + int crtc_x, int crtc_y,
>> + unsigned int crtc_w, unsigned int crtc_h,
>> + uint32_t src_x, uint32_t src_y,
>> + uint32_t src_w, uint32_t src_h,
>> + struct drm_modeset_acquire_ctx *ctx);
>> +int drm_plane_helper_disable_primary(struct drm_plane *plane,
>> + struct drm_modeset_acquire_ctx *ctx);
>> +void drm_plane_helper_destroy(struct drm_plane *plane);
>>
>> #endif
>> --
>> 2.36.1
>>
>
--
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 840 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20220811/d266c254/attachment.sig>
More information about the dri-devel
mailing list