| Message ID | 20230727140453.577445-1-javierm@redhat.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | drm/ssd130x: Use shadow-buffer helpers when managing plane's state | expand |
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 <arnd@arndb.de> 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 <tzimmermann@suse.de> > Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> This looks correct now. Thanks for fixing this bug quickly. With the Close added: Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de> 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 <tzimmermann@suse.de> 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 <arnd@arndb.de> > > 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 <tzimmermann@suse.de> >> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> > > This looks correct now. Thanks for fixing this bug quickly. With the > Close added: > > Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de> > Thanks and to you for pointing that out.
Am 27.07.23 um 17:16 schrieb Javier Martinez Canillas: > Thomas Zimmermann <tzimmermann@suse.de> 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 <arnd@arndb.de> >> >> 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 <tzimmermann@suse.de> >>> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> >> >> This looks correct now. Thanks for fixing this bug quickly. With the >> Close added: >> >> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de> >> > > Thanks and to you for pointing that out. >
Javier Martinez Canillas <javierm@redhat.com> writes: > Thomas Zimmermann <tzimmermann@suse.de> 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 <arnd@arndb.de> >> >> 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/20230727122412.2464210-1-arnd@kernel.org Thanks!
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,
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 <arnd@arndb.de> Suggested-by: Thomas Zimmermann <tzimmermann@suse.de> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> --- drivers/gpu/drm/solomon/ssd130x.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-)