The commit 45b58669e532 ("drm/ssd130x: Allocate buffer in the plane's
.atomic_check() callback") moved the buffers allocation to be done in
the primary plane's .atomic_check() callback.
But it missed that since the driver uses a shadow-buffered plane, the
__drm_gem_{reset,duplicate,destroy}_shadow_plane() helper functions
must be used in the struct drm_plane_funcs handlers.
This was missed because the mentioned commit did not remove the macro
DRM_GEM_SHADOW_PLANE_FUNCS, which leads to the custom plane's atomic
state management handlers to not be used.
Fixes: 45b58669e532 ("drm/ssd130x: Allocate buffer in the plane's .atomic_check() callback")
Reported-by: Arnd Bergmann <[email protected]>
Suggested-by: Thomas Zimmermann <[email protected]>
Signed-off-by: Javier Martinez Canillas <[email protected]>
---
drivers/gpu/drm/solomon/ssd130x.c | 16 +++++++++-------
1 file changed, 9 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
index d2f8dd6a6347..971c425340c1 100644
--- a/drivers/gpu/drm/solomon/ssd130x.c
+++ b/drivers/gpu/drm/solomon/ssd130x.c
@@ -142,7 +142,7 @@ const struct ssd130x_deviceinfo ssd130x_variants[] = {
EXPORT_SYMBOL_NS_GPL(ssd130x_variants, DRM_SSD130X);
struct ssd130x_plane_state {
- struct drm_plane_state base;
+ struct drm_shadow_plane_state base;
/* Intermediate buffer to convert pixels from XRGB8888 to HW format */
u8 *buffer;
/* Buffer to store pixels in HW format and written to the panel */
@@ -151,7 +151,7 @@ struct ssd130x_plane_state {
static inline struct ssd130x_plane_state *to_ssd130x_plane_state(struct drm_plane_state *state)
{
- return container_of(state, struct ssd130x_plane_state, base);
+ return container_of(state, struct ssd130x_plane_state, base.base);
}
static inline struct ssd130x_device *drm_to_ssd130x(struct drm_device *drm)
@@ -689,11 +689,12 @@ static void ssd130x_primary_plane_reset(struct drm_plane *plane)
if (!ssd130x_state)
return;
- __drm_atomic_helper_plane_reset(plane, &ssd130x_state->base);
+ __drm_gem_reset_shadow_plane(plane, &ssd130x_state->base);
}
static struct drm_plane_state *ssd130x_primary_plane_duplicate_state(struct drm_plane *plane)
{
+ struct drm_shadow_plane_state *new_shadow_plane_state;
struct ssd130x_plane_state *old_ssd130x_state;
struct ssd130x_plane_state *ssd130x_state;
@@ -709,9 +710,11 @@ static struct drm_plane_state *ssd130x_primary_plane_duplicate_state(struct drm_
ssd130x_state->buffer = NULL;
ssd130x_state->data_array = NULL;
- __drm_atomic_helper_plane_duplicate_state(plane, &ssd130x_state->base);
+ new_shadow_plane_state = &ssd130x_state->base;
- return &ssd130x_state->base;
+ __drm_gem_duplicate_shadow_plane_state(plane, new_shadow_plane_state);
+
+ return &new_shadow_plane_state->base;
}
static void ssd130x_primary_plane_destroy_state(struct drm_plane *plane,
@@ -722,7 +725,7 @@ static void ssd130x_primary_plane_destroy_state(struct drm_plane *plane,
kfree(ssd130x_state->data_array);
kfree(ssd130x_state->buffer);
- __drm_atomic_helper_plane_destroy_state(&ssd130x_state->base);
+ __drm_gem_destroy_shadow_plane_state(&ssd130x_state->base);
kfree(ssd130x_state);
}
@@ -741,7 +744,6 @@ static const struct drm_plane_funcs ssd130x_primary_plane_funcs = {
.atomic_duplicate_state = ssd130x_primary_plane_duplicate_state,
.atomic_destroy_state = ssd130x_primary_plane_destroy_state,
.destroy = drm_plane_cleanup,
- DRM_GEM_SHADOW_PLANE_FUNCS,
};
static enum drm_mode_status ssd130x_crtc_helper_mode_valid(struct drm_crtc *crtc,
--
2.41.0
Hi Javier
Am 27.07.23 um 16:04 schrieb Javier Martinez Canillas:
> The commit 45b58669e532 ("drm/ssd130x: Allocate buffer in the plane's
> .atomic_check() callback") moved the buffers allocation to be done in
> the primary plane's .atomic_check() callback.
>
> But it missed that since the driver uses a shadow-buffered plane, the
> __drm_gem_{reset,duplicate,destroy}_shadow_plane() helper functions
> must be used in the struct drm_plane_funcs handlers.
>
> This was missed because the mentioned commit did not remove the macro
> DRM_GEM_SHADOW_PLANE_FUNCS, which leads to the custom plane's atomic
> state management handlers to not be used.
>
> Fixes: 45b58669e532 ("drm/ssd130x: Allocate buffer in the plane's .atomic_check() callback")
> Reported-by: Arnd Bergmann <[email protected]>
Reported-by needs to be followed by
Closes: <url>
where <url> would point to Arnd's bug report on dri-devel.
> Suggested-by: Thomas Zimmermann <[email protected]>
> Signed-off-by: Javier Martinez Canillas <[email protected]>
This looks correct now. Thanks for fixing this bug quickly. With the
Close added:
Reviewed-by: Thomas Zimmermann <[email protected]>
Best regards
Thomas
> ---
>
> drivers/gpu/drm/solomon/ssd130x.c | 16 +++++++++-------
> 1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
> index d2f8dd6a6347..971c425340c1 100644
> --- a/drivers/gpu/drm/solomon/ssd130x.c
> +++ b/drivers/gpu/drm/solomon/ssd130x.c
> @@ -142,7 +142,7 @@ const struct ssd130x_deviceinfo ssd130x_variants[] = {
> EXPORT_SYMBOL_NS_GPL(ssd130x_variants, DRM_SSD130X);
>
> struct ssd130x_plane_state {
> - struct drm_plane_state base;
> + struct drm_shadow_plane_state base;
> /* Intermediate buffer to convert pixels from XRGB8888 to HW format */
> u8 *buffer;
> /* Buffer to store pixels in HW format and written to the panel */
> @@ -151,7 +151,7 @@ struct ssd130x_plane_state {
>
> static inline struct ssd130x_plane_state *to_ssd130x_plane_state(struct drm_plane_state *state)
> {
> - return container_of(state, struct ssd130x_plane_state, base);
> + return container_of(state, struct ssd130x_plane_state, base.base);
> }
>
> static inline struct ssd130x_device *drm_to_ssd130x(struct drm_device *drm)
> @@ -689,11 +689,12 @@ static void ssd130x_primary_plane_reset(struct drm_plane *plane)
> if (!ssd130x_state)
> return;
>
> - __drm_atomic_helper_plane_reset(plane, &ssd130x_state->base);
> + __drm_gem_reset_shadow_plane(plane, &ssd130x_state->base);
> }
>
> static struct drm_plane_state *ssd130x_primary_plane_duplicate_state(struct drm_plane *plane)
> {
> + struct drm_shadow_plane_state *new_shadow_plane_state;
> struct ssd130x_plane_state *old_ssd130x_state;
> struct ssd130x_plane_state *ssd130x_state;
>
> @@ -709,9 +710,11 @@ static struct drm_plane_state *ssd130x_primary_plane_duplicate_state(struct drm_
> ssd130x_state->buffer = NULL;
> ssd130x_state->data_array = NULL;
>
> - __drm_atomic_helper_plane_duplicate_state(plane, &ssd130x_state->base);
> + new_shadow_plane_state = &ssd130x_state->base;
>
> - return &ssd130x_state->base;
> + __drm_gem_duplicate_shadow_plane_state(plane, new_shadow_plane_state);
> +
> + return &new_shadow_plane_state->base;
> }
>
> static void ssd130x_primary_plane_destroy_state(struct drm_plane *plane,
> @@ -722,7 +725,7 @@ static void ssd130x_primary_plane_destroy_state(struct drm_plane *plane,
> kfree(ssd130x_state->data_array);
> kfree(ssd130x_state->buffer);
>
> - __drm_atomic_helper_plane_destroy_state(&ssd130x_state->base);
> + __drm_gem_destroy_shadow_plane_state(&ssd130x_state->base);
>
> kfree(ssd130x_state);
> }
> @@ -741,7 +744,6 @@ static const struct drm_plane_funcs ssd130x_primary_plane_funcs = {
> .atomic_duplicate_state = ssd130x_primary_plane_duplicate_state,
> .atomic_destroy_state = ssd130x_primary_plane_destroy_state,
> .destroy = drm_plane_cleanup,
> - DRM_GEM_SHADOW_PLANE_FUNCS,
> };
>
> static enum drm_mode_status ssd130x_crtc_helper_mode_valid(struct drm_crtc *crtc,
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)
Javier Martinez Canillas <[email protected]> writes:
> Thomas Zimmermann <[email protected]> writes:
>
> Hello Thomas,
>
>> Hi Javier
>>
>> Am 27.07.23 um 16:04 schrieb Javier Martinez Canillas:
>>> The commit 45b58669e532 ("drm/ssd130x: Allocate buffer in the plane's
>>> .atomic_check() callback") moved the buffers allocation to be done in
>>> the primary plane's .atomic_check() callback.
>>>
>>> But it missed that since the driver uses a shadow-buffered plane, the
>>> __drm_gem_{reset,duplicate,destroy}_shadow_plane() helper functions
>>> must be used in the struct drm_plane_funcs handlers.
>>>
>>> This was missed because the mentioned commit did not remove the macro
>>> DRM_GEM_SHADOW_PLANE_FUNCS, which leads to the custom plane's atomic
>>> state management handlers to not be used.
>>>
>>> Fixes: 45b58669e532 ("drm/ssd130x: Allocate buffer in the plane's .atomic_check() callback")
>>> Reported-by: Arnd Bergmann <[email protected]>
>>
>> Reported-by needs to be followed by
>>
>> Closes: <url>
>>
>> where <url> would point to Arnd's bug report on dri-devel.
>>
>
> Ah, I thought checkpatch complaining about it but since we already add a
> Link: with dim, didn't know what to add there. Makes sense to add Arnd's
> report indeed.
>
> I can include it when applying instead of posting a v2 if you don't mind.
>
Pushed to drm-misc (drm-misc-next), adding:
Closes: https://lore.kernel.org/dri-devel/[email protected]
Thanks!
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Thomas Zimmermann <[email protected]> writes:
Hello Thomas,
> Hi Javier
>
> Am 27.07.23 um 16:04 schrieb Javier Martinez Canillas:
>> The commit 45b58669e532 ("drm/ssd130x: Allocate buffer in the plane's
>> .atomic_check() callback") moved the buffers allocation to be done in
>> the primary plane's .atomic_check() callback.
>>
>> But it missed that since the driver uses a shadow-buffered plane, the
>> __drm_gem_{reset,duplicate,destroy}_shadow_plane() helper functions
>> must be used in the struct drm_plane_funcs handlers.
>>
>> This was missed because the mentioned commit did not remove the macro
>> DRM_GEM_SHADOW_PLANE_FUNCS, which leads to the custom plane's atomic
>> state management handlers to not be used.
>>
>> Fixes: 45b58669e532 ("drm/ssd130x: Allocate buffer in the plane's .atomic_check() callback")
>> Reported-by: Arnd Bergmann <[email protected]>
>
> Reported-by needs to be followed by
>
> Closes: <url>
>
> where <url> would point to Arnd's bug report on dri-devel.
>
Ah, I thought checkpatch complaining about it but since we already add a
Link: with dim, didn't know what to add there. Makes sense to add Arnd's
report indeed.
I can include it when applying instead of posting a v2 if you don't mind.
>> Suggested-by: Thomas Zimmermann <[email protected]>
>> Signed-off-by: Javier Martinez Canillas <[email protected]>
>
> This looks correct now. Thanks for fixing this bug quickly. With the
> Close added:
>
> Reviewed-by: Thomas Zimmermann <[email protected]>
>
Thanks and to you for pointing that out.
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Am 27.07.23 um 17:16 schrieb Javier Martinez Canillas:
> Thomas Zimmermann <[email protected]> writes:
>
> Hello Thomas,
>
>> Hi Javier
>>
>> Am 27.07.23 um 16:04 schrieb Javier Martinez Canillas:
>>> The commit 45b58669e532 ("drm/ssd130x: Allocate buffer in the plane's
>>> .atomic_check() callback") moved the buffers allocation to be done in
>>> the primary plane's .atomic_check() callback.
>>>
>>> But it missed that since the driver uses a shadow-buffered plane, the
>>> __drm_gem_{reset,duplicate,destroy}_shadow_plane() helper functions
>>> must be used in the struct drm_plane_funcs handlers.
>>>
>>> This was missed because the mentioned commit did not remove the macro
>>> DRM_GEM_SHADOW_PLANE_FUNCS, which leads to the custom plane's atomic
>>> state management handlers to not be used.
>>>
>>> Fixes: 45b58669e532 ("drm/ssd130x: Allocate buffer in the plane's .atomic_check() callback")
>>> Reported-by: Arnd Bergmann <[email protected]>
>>
>> Reported-by needs to be followed by
>>
>> Closes: <url>
>>
>> where <url> would point to Arnd's bug report on dri-devel.
>>
>
> Ah, I thought checkpatch complaining about it but since we already add a
> Link: with dim, didn't know what to add there. Makes sense to add Arnd's
> report indeed.
>
> I can include it when applying instead of posting a v2 if you don't mind.
Sure NP
>
>>> Suggested-by: Thomas Zimmermann <[email protected]>
>>> Signed-off-by: Javier Martinez Canillas <[email protected]>
>>
>> This looks correct now. Thanks for fixing this bug quickly. With the
>> Close added:
>>
>> Reviewed-by: Thomas Zimmermann <[email protected]>
>>
>
> Thanks and to you for pointing that out.
>
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)