diff mbox series

[v7,29/36] drm/vc4: tests: Remove vc4_dummy_plane structure

Message ID 20240222-kms-hdmi-connector-state-v7-29-8f4af575fce2@kernel.org (mailing list archive)
State New, archived
Headers show
Series drm/connector: Create HDMI Connector infrastructure | expand

Commit Message

Maxime Ripard Feb. 22, 2024, 6:14 p.m. UTC
The vc4_dummy_plane structure is an exact equivalent to vc4_plane, so we
don't need it.

Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 drivers/gpu/drm/vc4/tests/vc4_mock.c       |  6 ++----
 drivers/gpu/drm/vc4/tests/vc4_mock.h       |  9 ++-------
 drivers/gpu/drm/vc4/tests/vc4_mock_plane.c | 14 +++++---------
 3 files changed, 9 insertions(+), 20 deletions(-)

Comments

Maíra Canal Feb. 26, 2024, 12:29 p.m. UTC | #1
On 2/22/24 15:14, Maxime Ripard wrote:
> The vc4_dummy_plane structure is an exact equivalent to vc4_plane, so we

Maybe I understood incorrectly, but isn't the vc4_dummy_plane structure 
equivalent to drm_plane?

Best Regards,
- Maíra

> don't need it.
> 
> Signed-off-by: Maxime Ripard <mripard@kernel.org>
> ---
>   drivers/gpu/drm/vc4/tests/vc4_mock.c       |  6 ++----
>   drivers/gpu/drm/vc4/tests/vc4_mock.h       |  9 ++-------
>   drivers/gpu/drm/vc4/tests/vc4_mock_plane.c | 14 +++++---------
>   3 files changed, 9 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vc4/tests/vc4_mock.c b/drivers/gpu/drm/vc4/tests/vc4_mock.c
> index becb3dbaa548..0731a7d85d7a 100644
> --- a/drivers/gpu/drm/vc4/tests/vc4_mock.c
> +++ b/drivers/gpu/drm/vc4/tests/vc4_mock.c
> @@ -109,16 +109,14 @@ static const struct vc4_mock_desc vc5_mock =
>   static int __build_one_pipe(struct kunit *test, struct drm_device *drm,
>   			    const struct vc4_mock_pipe_desc *pipe)
>   {
> -	struct vc4_dummy_plane *dummy_plane;
>   	struct drm_plane *plane;
>   	struct vc4_dummy_crtc *dummy_crtc;
>   	struct drm_crtc *crtc;
>   	unsigned int i;
>   
> -	dummy_plane = vc4_dummy_plane(test, drm, DRM_PLANE_TYPE_PRIMARY);
> -	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, dummy_plane);
> +	plane = vc4_dummy_plane(test, drm, DRM_PLANE_TYPE_PRIMARY);
> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, plane);
>   
> -	plane = &dummy_plane->plane.base;
>   	dummy_crtc = vc4_mock_pv(test, drm, plane, pipe->data);
>   	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, dummy_crtc);
>   
> diff --git a/drivers/gpu/drm/vc4/tests/vc4_mock.h b/drivers/gpu/drm/vc4/tests/vc4_mock.h
> index 2d0b339bd9f3..002b6218960c 100644
> --- a/drivers/gpu/drm/vc4/tests/vc4_mock.h
> +++ b/drivers/gpu/drm/vc4/tests/vc4_mock.h
> @@ -21,13 +21,8 @@ struct drm_crtc *vc4_find_crtc_for_encoder(struct kunit *test,
>   	return NULL;
>   }
>   
> -struct vc4_dummy_plane {
> -	struct vc4_plane plane;
> -};
> -
> -struct vc4_dummy_plane *vc4_dummy_plane(struct kunit *test,
> -					struct drm_device *drm,
> -					enum drm_plane_type type);
> +struct drm_plane *vc4_dummy_plane(struct kunit *test, struct drm_device *drm,
> +				  enum drm_plane_type type);
>   
>   struct vc4_dummy_crtc {
>   	struct vc4_crtc crtc;
> diff --git a/drivers/gpu/drm/vc4/tests/vc4_mock_plane.c b/drivers/gpu/drm/vc4/tests/vc4_mock_plane.c
> index 62b18f5f41db..973f5f929097 100644
> --- a/drivers/gpu/drm/vc4/tests/vc4_mock_plane.c
> +++ b/drivers/gpu/drm/vc4/tests/vc4_mock_plane.c
> @@ -22,15 +22,12 @@ static const uint32_t vc4_dummy_plane_formats[] = {
>   	DRM_FORMAT_XRGB8888,
>   };
>   
> -struct vc4_dummy_plane *vc4_dummy_plane(struct kunit *test,
> -					struct drm_device *drm,
> -					enum drm_plane_type type)
> +struct drm_plane *vc4_dummy_plane(struct kunit *test, struct drm_device *drm,
> +				  enum drm_plane_type type)
>   {
> -	struct vc4_dummy_plane *dummy_plane;
>   	struct drm_plane *plane;
>   
> -	dummy_plane = drmm_universal_plane_alloc(drm,
> -						 struct vc4_dummy_plane, plane.base,
> +	plane = __drmm_universal_plane_alloc(drm, sizeof(struct drm_plane), 0,
>   						 0,
>   						 &vc4_dummy_plane_funcs,
>   						 vc4_dummy_plane_formats,
> @@ -38,10 +35,9 @@ struct vc4_dummy_plane *vc4_dummy_plane(struct kunit *test,
>   						 NULL,
>   						 DRM_PLANE_TYPE_PRIMARY,
>   						 NULL);
> -	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, dummy_plane);
> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, plane);
>   
> -	plane = &dummy_plane->plane.base;
>   	drm_plane_helper_add(plane, &vc4_dummy_plane_helper_funcs);
>   
> -	return dummy_plane;
> +	return plane;
>   }
>
Maxime Ripard Feb. 27, 2024, 1:02 p.m. UTC | #2
Hi Maíra,

Thanks for you reviews!

On Mon, Feb 26, 2024 at 09:29:32AM -0300, Maíra Canal wrote:
> On 2/22/24 15:14, Maxime Ripard wrote:
> > The vc4_dummy_plane structure is an exact equivalent to vc4_plane, so we
> 
> Maybe I understood incorrectly, but isn't the vc4_dummy_plane structure
> equivalent to drm_plane?

Both statements are true :)

vc4 itself uses vc4_plane to holds its plane-related content, but it
turns out that there's nothing in that structure anymore and vc4_plane
== drm_plane.

In our mock driver, we have another structure meant to store the
mock-plane-related content which doesn't have anything in it anymore,
and is thus equivalent to vc4_plane.

So, basically, vc4_dummy_plane == vc4_plane == drm_plane.

This patch is only about getting rid of vc4_dummy_plane though.

Is it clearer?

Maxime
Maíra Canal Feb. 27, 2024, 10:45 p.m. UTC | #3
Hi Maxime,

On 2/27/24 10:02, Maxime Ripard wrote:
> Hi Maíra,
> 
> Thanks for you reviews!
> 
> On Mon, Feb 26, 2024 at 09:29:32AM -0300, Maíra Canal wrote:
>> On 2/22/24 15:14, Maxime Ripard wrote:
>>> The vc4_dummy_plane structure is an exact equivalent to vc4_plane, so we
>>
>> Maybe I understood incorrectly, but isn't the vc4_dummy_plane structure
>> equivalent to drm_plane?
> 
> Both statements are true :)
> 
> vc4 itself uses vc4_plane to holds its plane-related content, but it
> turns out that there's nothing in that structure anymore and vc4_plane
> == drm_plane.
> 
> In our mock driver, we have another structure meant to store the
> mock-plane-related content which doesn't have anything in it anymore,
> and is thus equivalent to vc4_plane.
> 
> So, basically, vc4_dummy_plane == vc4_plane == drm_plane.
> 
> This patch is only about getting rid of vc4_dummy_plane though.
> 
> Is it clearer?
> 

Yeah, with that pointed out, you can add my:

Reviewed-by: Maíra Canal <mcanal@igalia.com>

Best Regards,
- Maíra

> Maxime
Maxime Ripard Feb. 28, 2024, 4:16 p.m. UTC | #4
On Tue, Feb 27, 2024 at 07:45:01PM -0300, Maíra Canal wrote:
> Hi Maxime,
> 
> On 2/27/24 10:02, Maxime Ripard wrote:
> > Hi Maíra,
> > 
> > Thanks for you reviews!
> > 
> > On Mon, Feb 26, 2024 at 09:29:32AM -0300, Maíra Canal wrote:
> > > On 2/22/24 15:14, Maxime Ripard wrote:
> > > > The vc4_dummy_plane structure is an exact equivalent to vc4_plane, so we
> > > 
> > > Maybe I understood incorrectly, but isn't the vc4_dummy_plane structure
> > > equivalent to drm_plane?
> > 
> > Both statements are true :)
> > 
> > vc4 itself uses vc4_plane to holds its plane-related content, but it
> > turns out that there's nothing in that structure anymore and vc4_plane
> > == drm_plane.
> > 
> > In our mock driver, we have another structure meant to store the
> > mock-plane-related content which doesn't have anything in it anymore,
> > and is thus equivalent to vc4_plane.
> > 
> > So, basically, vc4_dummy_plane == vc4_plane == drm_plane.
> > 
> > This patch is only about getting rid of vc4_dummy_plane though.
> > 
> > Is it clearer?
> > 
> 
> Yeah, with that pointed out, you can add my:

I'll rephrase for the next version then

> Reviewed-by: Maíra Canal <mcanal@igalia.com>

Thanks!
Maxime
John Stultz Feb. 29, 2024, 4:17 a.m. UTC | #5
On Wed, Feb 28, 2024 at 7:24 AM Maxime Ripard <mripard@redhat.com> wrote:
>
> I'm currently working on a platform that seems to have togglable RAM ECC
> support. Enabling ECC reduces the memory capacity and memory bandwidth,
> so while it's a good idea to protect most of the system, it's not worth
> it for things like framebuffers that won't really be affected by a
> bitflip.
>
> It's currently setup by enabling ECC on the entire memory, and then
> having a region of memory where ECC is disabled and where we're supposed
> to allocate from for allocations that don't need it.
>
> My first thought to support this was to create a reserved memory region
> for the !ECC memory, and to create a heap to allocate buffers from that
> region. That would leave the system protected by ECC, while enabling
> userspace to be nicer to the system by allocating buffers from the !ECC
> region if it doesn't need it.
>
> However, this creates basically a new combination compared to the one we
> already have (ie, physically contiguous vs virtually contiguous), and we
> probably would want to throw in cacheable vs non-cacheable too.
>
> If we had to provide new heaps for each variation, we would have 8 heaps
> (and 6 new ones), which could be fine I guess but would still increase
> quite a lot the number of heaps we have so far.
>
> Is it something that would be a problem? If it is, do you see another
> way to support those kind of allocations (like providing hints through
> the ioctl maybe?)?

So, the dma-buf heaps interface uses chardevs so that we can have a
lot of flexibility in the types of heaps (and don't have the risk of
bitmask exhaustion like ION had). So I don't see adding many
differently named heaps as particularly problematic.

That said, if there are truly generic properties (cacheable vs
non-cachable is maybe one of those) which apply to most heaps, I'm
open to making use of the flags. But I want to avoid having per-heap
flags, it really needs to be a generic attribute.

And I personally don't mind the idea of having things added as heaps
initially, and potentially upgrading them to flags if needed (allowing
heap drivers to optionally enumerate the old chardevs behind a config
option for backwards compatibility).

How common is the hardware that is going to provide this configurable
ECC option, and will you really want the option on all of the heap
types? Will there be any hardware constraint limitations caused by the
ECC/!ECC flags? (ie: Devices that can't use !ECC allocated buffers?)

If not, I wonder if it would make sense to have something more along
the lines using a fcntl()  like how F_SEAL_* is used with memfds?
With some of the discussion around "restricted"/"secure" heaps that
can change state, I've liked this idea of just allocating dmabufs from
normal heaps and then using fcntl or something similar to modify
properties of the buffer that are separate from the type of memory
that is needed to be allocated to satisfy device constraints.

thanks
-john
Maxime Ripard March 4, 2024, 1:46 p.m. UTC | #6
Hi John,

Thanks for your answer

On Wed, Feb 28, 2024 at 08:17:55PM -0800, John Stultz wrote:
> On Wed, Feb 28, 2024 at 7:24 AM Maxime Ripard <mripard@redhat.com> wrote:
> >
> > I'm currently working on a platform that seems to have togglable RAM ECC
> > support. Enabling ECC reduces the memory capacity and memory bandwidth,
> > so while it's a good idea to protect most of the system, it's not worth
> > it for things like framebuffers that won't really be affected by a
> > bitflip.
> >
> > It's currently setup by enabling ECC on the entire memory, and then
> > having a region of memory where ECC is disabled and where we're supposed
> > to allocate from for allocations that don't need it.
> >
> > My first thought to support this was to create a reserved memory region
> > for the !ECC memory, and to create a heap to allocate buffers from that
> > region. That would leave the system protected by ECC, while enabling
> > userspace to be nicer to the system by allocating buffers from the !ECC
> > region if it doesn't need it.
> >
> > However, this creates basically a new combination compared to the one we
> > already have (ie, physically contiguous vs virtually contiguous), and we
> > probably would want to throw in cacheable vs non-cacheable too.
> >
> > If we had to provide new heaps for each variation, we would have 8 heaps
> > (and 6 new ones), which could be fine I guess but would still increase
> > quite a lot the number of heaps we have so far.
> >
> > Is it something that would be a problem? If it is, do you see another
> > way to support those kind of allocations (like providing hints through
> > the ioctl maybe?)?
> 
> So, the dma-buf heaps interface uses chardevs so that we can have a
> lot of flexibility in the types of heaps (and don't have the risk of
> bitmask exhaustion like ION had). So I don't see adding many
> differently named heaps as particularly problematic.

Ok

> That said, if there are truly generic properties (cacheable vs
> non-cachable is maybe one of those) which apply to most heaps, I'm
> open to making use of the flags. But I want to avoid having per-heap
> flags, it really needs to be a generic attribute.
> 
> And I personally don't mind the idea of having things added as heaps
> initially, and potentially upgrading them to flags if needed (allowing
> heap drivers to optionally enumerate the old chardevs behind a config
> option for backwards compatibility).
> 
> How common is the hardware that is going to provide this configurable
> ECC option

In terms of number of SoCs with the feature, it's probably a handful. In
terms of number of units shipped, we're in the fairly common range :)

> and will you really want the option on all of the heap types?

Aside from the cacheable/uncacheable discussion, yes. We could probably
get away with only physically contiguous allocations at the moment
though, I'll double check.

> Will there be any hardware constraint limitations caused by the
> ECC/!ECC flags? (ie: Devices that can't use !ECC allocated buffers?)

My understanding is that there's no device restriction. It will be a
carved out memory so we will need to maintain a separate pool and it
will be limited in size, but that's pretty much the only one afaik.

> If not, I wonder if it would make sense to have something more along
> the lines using a fcntl()  like how F_SEAL_* is used with memfds?
> With some of the discussion around "restricted"/"secure" heaps that
> can change state, I've liked this idea of just allocating dmabufs from
> normal heaps and then using fcntl or something similar to modify
> properties of the buffer that are separate from the type of memory
> that is needed to be allocated to satisfy device constraints.

Sorry, I'm not super familiar with F_SEAL so I don't really follow what
you have in mind here. Do you have any additional resources I could read
to understand better what you're thinking about?

Also, if we were to modify the ECC attributes after the dma-buf has been
allocated by dma-buf, and if the !ECC memory is carved out only, then
wouldn't that mean we would need to reallocate the backing buffer for
that dma-buf?

Thanks!
Maxime
John Stultz March 4, 2024, 9:12 p.m. UTC | #7
On Mon, Mar 4, 2024 at 5:46 AM Maxime Ripard <mripard@redhat.com> wrote:
> On Wed, Feb 28, 2024 at 08:17:55PM -0800, John Stultz wrote:
> > On Wed, Feb 28, 2024 at 7:24 AM Maxime Ripard <mripard@redhat.com> wrote:
> > >
> > > I'm currently working on a platform that seems to have togglable RAM ECC
> > > support. Enabling ECC reduces the memory capacity and memory bandwidth,
> > > so while it's a good idea to protect most of the system, it's not worth
> > > it for things like framebuffers that won't really be affected by a
> > > bitflip.
> > >
> > > It's currently setup by enabling ECC on the entire memory, and then
> > > having a region of memory where ECC is disabled and where we're supposed
> > > to allocate from for allocations that don't need it.
> > >
> > > My first thought to support this was to create a reserved memory region
> > > for the !ECC memory, and to create a heap to allocate buffers from that
> > > region. That would leave the system protected by ECC, while enabling
> > > userspace to be nicer to the system by allocating buffers from the !ECC
> > > region if it doesn't need it.
> > >
> > > However, this creates basically a new combination compared to the one we
> > > already have (ie, physically contiguous vs virtually contiguous), and we
> > > probably would want to throw in cacheable vs non-cacheable too.
> > >
> > > If we had to provide new heaps for each variation, we would have 8 heaps
> > > (and 6 new ones), which could be fine I guess but would still increase
> > > quite a lot the number of heaps we have so far.
> > >
> > > Is it something that would be a problem? If it is, do you see another
> > > way to support those kind of allocations (like providing hints through
> > > the ioctl maybe?)?
> >
> > So, the dma-buf heaps interface uses chardevs so that we can have a
> > lot of flexibility in the types of heaps (and don't have the risk of
> > bitmask exhaustion like ION had). So I don't see adding many
> > differently named heaps as particularly problematic.
>
> Ok
>
> > That said, if there are truly generic properties (cacheable vs
> > non-cachable is maybe one of those) which apply to most heaps, I'm
> > open to making use of the flags. But I want to avoid having per-heap
> > flags, it really needs to be a generic attribute.
> >
> > And I personally don't mind the idea of having things added as heaps
> > initially, and potentially upgrading them to flags if needed (allowing
> > heap drivers to optionally enumerate the old chardevs behind a config
> > option for backwards compatibility).
> >
> > How common is the hardware that is going to provide this configurable
> > ECC option
>
> In terms of number of SoCs with the feature, it's probably a handful. In
> terms of number of units shipped, we're in the fairly common range :)
>

Sure, I guess I was trying to get a sense of is this a feature we'll
likely be seeing commonly across hardware (such that internal kernel
allocators would be considering it as a flag), or is it more tied to a
single vendor such that enabling/isolating it in a driver is the right
place in the abstraction to put it.


> > and will you really want the option on all of the heap types?
>
> Aside from the cacheable/uncacheable discussion, yes. We could probably
> get away with only physically contiguous allocations at the moment
> though, I'll double check.

Ok, that will be useful to know.

> > Will there be any hardware constraint limitations caused by the
> > ECC/!ECC flags? (ie: Devices that can't use !ECC allocated buffers?)
>
> My understanding is that there's no device restriction. It will be a
> carved out memory so we will need to maintain a separate pool and it
> will be limited in size, but that's pretty much the only one afaik.

Ok.

> > If not, I wonder if it would make sense to have something more along
> > the lines using a fcntl()  like how F_SEAL_* is used with memfds?
> > With some of the discussion around "restricted"/"secure" heaps that
> > can change state, I've liked this idea of just allocating dmabufs from
> > normal heaps and then using fcntl or something similar to modify
> > properties of the buffer that are separate from the type of memory
> > that is needed to be allocated to satisfy device constraints.
>
> Sorry, I'm not super familiar with F_SEAL so I don't really follow what
> you have in mind here. Do you have any additional resources I could read
> to understand better what you're thinking about?

See the File Sealing section: https://man7.org/linux/man-pages/man2/fcntl.2.html

> Also, if we were to modify the ECC attributes after the dma-buf has been
> allocated by dma-buf, and if the !ECC memory is carved out only, then
> wouldn't that mean we would need to reallocate the backing buffer for
> that dma-buf?

So yea, having to work on a larger pool likely makes this not useful
here, so apologies for the tangent.

To explain myself, part of what I'm thinking of is, the dmabuf heaps
(and really ION before it) try to solve how to allocate a buffer type
that can be used across a number of devices that may have different
constraints. So the focus was on "types of memory" to satisfy the
constraint (contiguous, non-contiguous, secure/restricted, etc), which
come down to what pages actually get used.  However, outside of the
"constraint type" the buffer may have, there are other "properties"
that may affect performance (like cacheability, and some variants of
"restricted buffers" which can change over their lifetime).  With ION
vendors would mix these two together in their vendor heaps, and with
out-of-tree dmabuf heaps it is also common to tangle types and
properties together.

So I'm sort of stewing on how to best distinguish between heaps for
"types of memory/pages" (ie: what's *required* to share the buffer
between devices) vs these buffer properties (which affect performance)
that may apply to multiple memory types.
(And I'm also not 100% convinced that distinguishing between this is
necessary, but casually mixing them feels messy to me)

For buffers where those properties might change over time (like some
variants of restricted buffers), I think the fnctl/F_SEAL_* idea makes
sense to allow the buffer to become restricted.

For cacheability, it seems likely an allocation flag would be nicest,
but we don't have upstream users and not a lot of heap types yet, thus
the out-of-tree "system-uncached" heap which sort of mixes types and
properties.

With ECC I was trying to get a sense of where it would sit between
this "type of memory" vs a "buffer property".  If internal allocators
are likely to consider it in a way similar to CMA (and with the pool
granular control, it sounds like it), then yeah, it probably should be
a type of memory, so a new heap name is likely the way to go - but
there is still the question of how to best support the various
combinations of (contiguous, cacheable) along with ECC.  But if it
were something that was dynamically controllable at a finer grained
level in the future, maybe it would be something like a  buffer
property.

thanks
-john
diff mbox series

Patch

diff --git a/drivers/gpu/drm/vc4/tests/vc4_mock.c b/drivers/gpu/drm/vc4/tests/vc4_mock.c
index becb3dbaa548..0731a7d85d7a 100644
--- a/drivers/gpu/drm/vc4/tests/vc4_mock.c
+++ b/drivers/gpu/drm/vc4/tests/vc4_mock.c
@@ -109,16 +109,14 @@  static const struct vc4_mock_desc vc5_mock =
 static int __build_one_pipe(struct kunit *test, struct drm_device *drm,
 			    const struct vc4_mock_pipe_desc *pipe)
 {
-	struct vc4_dummy_plane *dummy_plane;
 	struct drm_plane *plane;
 	struct vc4_dummy_crtc *dummy_crtc;
 	struct drm_crtc *crtc;
 	unsigned int i;
 
-	dummy_plane = vc4_dummy_plane(test, drm, DRM_PLANE_TYPE_PRIMARY);
-	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, dummy_plane);
+	plane = vc4_dummy_plane(test, drm, DRM_PLANE_TYPE_PRIMARY);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, plane);
 
-	plane = &dummy_plane->plane.base;
 	dummy_crtc = vc4_mock_pv(test, drm, plane, pipe->data);
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, dummy_crtc);
 
diff --git a/drivers/gpu/drm/vc4/tests/vc4_mock.h b/drivers/gpu/drm/vc4/tests/vc4_mock.h
index 2d0b339bd9f3..002b6218960c 100644
--- a/drivers/gpu/drm/vc4/tests/vc4_mock.h
+++ b/drivers/gpu/drm/vc4/tests/vc4_mock.h
@@ -21,13 +21,8 @@  struct drm_crtc *vc4_find_crtc_for_encoder(struct kunit *test,
 	return NULL;
 }
 
-struct vc4_dummy_plane {
-	struct vc4_plane plane;
-};
-
-struct vc4_dummy_plane *vc4_dummy_plane(struct kunit *test,
-					struct drm_device *drm,
-					enum drm_plane_type type);
+struct drm_plane *vc4_dummy_plane(struct kunit *test, struct drm_device *drm,
+				  enum drm_plane_type type);
 
 struct vc4_dummy_crtc {
 	struct vc4_crtc crtc;
diff --git a/drivers/gpu/drm/vc4/tests/vc4_mock_plane.c b/drivers/gpu/drm/vc4/tests/vc4_mock_plane.c
index 62b18f5f41db..973f5f929097 100644
--- a/drivers/gpu/drm/vc4/tests/vc4_mock_plane.c
+++ b/drivers/gpu/drm/vc4/tests/vc4_mock_plane.c
@@ -22,15 +22,12 @@  static const uint32_t vc4_dummy_plane_formats[] = {
 	DRM_FORMAT_XRGB8888,
 };
 
-struct vc4_dummy_plane *vc4_dummy_plane(struct kunit *test,
-					struct drm_device *drm,
-					enum drm_plane_type type)
+struct drm_plane *vc4_dummy_plane(struct kunit *test, struct drm_device *drm,
+				  enum drm_plane_type type)
 {
-	struct vc4_dummy_plane *dummy_plane;
 	struct drm_plane *plane;
 
-	dummy_plane = drmm_universal_plane_alloc(drm,
-						 struct vc4_dummy_plane, plane.base,
+	plane = __drmm_universal_plane_alloc(drm, sizeof(struct drm_plane), 0,
 						 0,
 						 &vc4_dummy_plane_funcs,
 						 vc4_dummy_plane_formats,
@@ -38,10 +35,9 @@  struct vc4_dummy_plane *vc4_dummy_plane(struct kunit *test,
 						 NULL,
 						 DRM_PLANE_TYPE_PRIMARY,
 						 NULL);
-	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, dummy_plane);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, plane);
 
-	plane = &dummy_plane->plane.base;
 	drm_plane_helper_add(plane, &vc4_dummy_plane_helper_funcs);
 
-	return dummy_plane;
+	return plane;
 }