[PATCH 1/5] drm/simple-kms-helper: Pass plane to prepare_fb() and cleanup_fb()
Daniel Vetter
daniel at ffwll.ch
Tue Oct 22 15:48:35 UTC 2019
On Tue, Oct 22, 2019 at 4:41 PM Thomas Zimmermann <tzimmermann at suse.de> wrote:
>
> Hi
>
> Am 22.10.19 um 16:14 schrieb Daniel Vetter:
> > On Tue, Oct 22, 2019 at 12:25:16PM +0200, Thomas Zimmermann wrote:
> >> Passing the plane structure to prepare_fb() and cleanup_fb() of
> >> struct drm_simple_display_pipe_funcs unifies the interface with
> >> struct drm_plane_helper_funcs. Implementations of these functions
> >> can now be shared between simple-pipeline and 'full-pipeline' drivers.
> >>
> >> Before, the functions received the simple display pipeline's structure
> >> as their first argument. Apparently no implementation needed that argument
> >> and anyone interested can still get it from the plane via container_of().
> >>
> >> As a side effect, drm_gem_fb_simple_display_pipe_prepare_fb() has been
> >> removed.
> >>
> >> Signed-off-by: Thomas Zimmermann <tzimmermann at suse.de>
> >
> > Hm ....
> >
> > I see your point, but having a vfunc which takes the wrong type just feels
> > wrong. Imo I'd just have added a simple pipe wrapper for the vram prepare.
>
> There's already mode_valid(), [1] which uses a similar exception. With
> existing 'prior art,' I didn't see the point of adding additional
> wrappers. Removing an existing wrapper function looked tempting, however. :)
Huh I totally missed that one. I guess that should get bikeshedded,
I'll type a patch.
-Daniel
> > But this should also be fine if we tune it at bit:
> > - Add a note to the kerneldoc explaining why the function signature is
> > "wrong".
> > - Add a drm_simple_display_plane_to_pipe helper for upcasting and
> > reference that one in the above note.
>
> Sure, no problem.
>
> Best regards
> Thomas
>
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/drm/drm_simple_kms_helper.h#n52
>
> > With that I think we still have a nice&clean design here. Still leaning
> > towards "just add another helper to plug in".
> >
> > Remaining parts of the series look really nice, but lets settle this
> > question here first.
> > -Daniel
> >
> >> ---
> >> drivers/gpu/drm/aspeed/aspeed_gfx_crtc.c | 2 +-
> >> drivers/gpu/drm/bochs/bochs_kms.c | 4 ++--
> >> drivers/gpu/drm/drm_gem_framebuffer_helper.c | 22 --------------------
> >> drivers/gpu/drm/drm_simple_kms_helper.c | 4 ++--
> >> drivers/gpu/drm/mcde/mcde_display.c | 2 +-
> >> drivers/gpu/drm/mxsfb/mxsfb_drv.c | 2 +-
> >> drivers/gpu/drm/pl111/pl111_display.c | 2 +-
> >> drivers/gpu/drm/tiny/hx8357d.c | 2 +-
> >> drivers/gpu/drm/tiny/ili9225.c | 2 +-
> >> drivers/gpu/drm/tiny/ili9341.c | 2 +-
> >> drivers/gpu/drm/tiny/mi0283qt.c | 2 +-
> >> drivers/gpu/drm/tiny/repaper.c | 2 +-
> >> drivers/gpu/drm/tiny/st7586.c | 2 +-
> >> drivers/gpu/drm/tiny/st7735r.c | 2 +-
> >> drivers/gpu/drm/tve200/tve200_display.c | 2 +-
> >> drivers/gpu/drm/xen/xen_drm_front_kms.c | 2 +-
> >> include/drm/drm_gem_framebuffer_helper.h | 4 +---
> >> include/drm/drm_plane.h | 2 +-
> >> include/drm/drm_simple_kms_helper.h | 6 +++---
> >> 19 files changed, 22 insertions(+), 46 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/aspeed/aspeed_gfx_crtc.c b/drivers/gpu/drm/aspeed/aspeed_gfx_crtc.c
> >> index 2184b8be6fd4..0fe72f46f397 100644
> >> --- a/drivers/gpu/drm/aspeed/aspeed_gfx_crtc.c
> >> +++ b/drivers/gpu/drm/aspeed/aspeed_gfx_crtc.c
> >> @@ -219,7 +219,7 @@ static const struct drm_simple_display_pipe_funcs aspeed_gfx_funcs = {
> >> .enable = aspeed_gfx_pipe_enable,
> >> .disable = aspeed_gfx_pipe_disable,
> >> .update = aspeed_gfx_pipe_update,
> >> - .prepare_fb = drm_gem_fb_simple_display_pipe_prepare_fb,
> >> + .prepare_fb = drm_gem_fb_prepare_fb,
> >> .enable_vblank = aspeed_gfx_enable_vblank,
> >> .disable_vblank = aspeed_gfx_disable_vblank,
> >> };
> >> diff --git a/drivers/gpu/drm/bochs/bochs_kms.c b/drivers/gpu/drm/bochs/bochs_kms.c
> >> index 02a9c1ed165b..0891640491eb 100644
> >> --- a/drivers/gpu/drm/bochs/bochs_kms.c
> >> +++ b/drivers/gpu/drm/bochs/bochs_kms.c
> >> @@ -69,7 +69,7 @@ static void bochs_pipe_update(struct drm_simple_display_pipe *pipe,
> >> }
> >> }
> >>
> >> -static int bochs_pipe_prepare_fb(struct drm_simple_display_pipe *pipe,
> >> +static int bochs_pipe_prepare_fb(struct drm_plane *plane,
> >> struct drm_plane_state *new_state)
> >> {
> >> struct drm_gem_vram_object *gbo;
> >> @@ -80,7 +80,7 @@ static int bochs_pipe_prepare_fb(struct drm_simple_display_pipe *pipe,
> >> return drm_gem_vram_pin(gbo, DRM_GEM_VRAM_PL_FLAG_VRAM);
> >> }
> >>
> >> -static void bochs_pipe_cleanup_fb(struct drm_simple_display_pipe *pipe,
> >> +static void bochs_pipe_cleanup_fb(struct drm_plane *plane,
> >> struct drm_plane_state *old_state)
> >> {
> >> struct drm_gem_vram_object *gbo;
> >> diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> >> index b9bcd310ca2d..c7ea288bef0a 100644
> >> --- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> >> +++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> >> @@ -300,25 +300,3 @@ int drm_gem_fb_prepare_fb(struct drm_plane *plane,
> >> return 0;
> >> }
> >> EXPORT_SYMBOL_GPL(drm_gem_fb_prepare_fb);
> >> -
> >> -/**
> >> - * drm_gem_fb_simple_display_pipe_prepare_fb - prepare_fb helper for
> >> - * &drm_simple_display_pipe
> >> - * @pipe: Simple display pipe
> >> - * @plane_state: Plane state
> >> - *
> >> - * This function uses drm_gem_fb_prepare_fb() to extract the exclusive fence
> >> - * from &drm_gem_object.resv and attaches it to plane state for the atomic
> >> - * helper to wait on. This is necessary to correctly implement implicit
> >> - * synchronization for any buffers shared as a struct &dma_buf. Drivers can use
> >> - * this as their &drm_simple_display_pipe_funcs.prepare_fb callback.
> >> - *
> >> - * See drm_atomic_set_fence_for_plane() for a discussion of implicit and
> >> - * explicit fencing in atomic modeset updates.
> >> - */
> >> -int drm_gem_fb_simple_display_pipe_prepare_fb(struct drm_simple_display_pipe *pipe,
> >> - struct drm_plane_state *plane_state)
> >> -{
> >> - return drm_gem_fb_prepare_fb(&pipe->plane, plane_state);
> >> -}
> >> -EXPORT_SYMBOL(drm_gem_fb_simple_display_pipe_prepare_fb);
> >> diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c b/drivers/gpu/drm/drm_simple_kms_helper.c
> >> index 046055719245..e51f336e67f0 100644
> >> --- a/drivers/gpu/drm/drm_simple_kms_helper.c
> >> +++ b/drivers/gpu/drm/drm_simple_kms_helper.c
> >> @@ -173,7 +173,7 @@ static int drm_simple_kms_plane_prepare_fb(struct drm_plane *plane,
> >> if (!pipe->funcs || !pipe->funcs->prepare_fb)
> >> return 0;
> >>
> >> - return pipe->funcs->prepare_fb(pipe, state);
> >> + return pipe->funcs->prepare_fb(&pipe->plane, state);
> >> }
> >>
> >> static void drm_simple_kms_plane_cleanup_fb(struct drm_plane *plane,
> >> @@ -185,7 +185,7 @@ static void drm_simple_kms_plane_cleanup_fb(struct drm_plane *plane,
> >> if (!pipe->funcs || !pipe->funcs->cleanup_fb)
> >> return;
> >>
> >> - pipe->funcs->cleanup_fb(pipe, state);
> >> + pipe->funcs->cleanup_fb(&pipe->plane, state);
> >> }
> >>
> >> static bool drm_simple_kms_format_mod_supported(struct drm_plane *plane,
> >> diff --git a/drivers/gpu/drm/mcde/mcde_display.c b/drivers/gpu/drm/mcde/mcde_display.c
> >> index 751454ae3cd1..31efbdcced5d 100644
> >> --- a/drivers/gpu/drm/mcde/mcde_display.c
> >> +++ b/drivers/gpu/drm/mcde/mcde_display.c
> >> @@ -1097,7 +1097,7 @@ static struct drm_simple_display_pipe_funcs mcde_display_funcs = {
> >> .enable = mcde_display_enable,
> >> .disable = mcde_display_disable,
> >> .update = mcde_display_update,
> >> - .prepare_fb = drm_gem_fb_simple_display_pipe_prepare_fb,
> >> + .prepare_fb = drm_gem_fb_prepare_fb,
> >> };
> >>
> >> int mcde_display_init(struct drm_device *drm)
> >> diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.c b/drivers/gpu/drm/mxsfb/mxsfb_drv.c
> >> index 497cf443a9af..547388b32c64 100644
> >> --- a/drivers/gpu/drm/mxsfb/mxsfb_drv.c
> >> +++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.c
> >> @@ -186,7 +186,7 @@ static struct drm_simple_display_pipe_funcs mxsfb_funcs = {
> >> .enable = mxsfb_pipe_enable,
> >> .disable = mxsfb_pipe_disable,
> >> .update = mxsfb_pipe_update,
> >> - .prepare_fb = drm_gem_fb_simple_display_pipe_prepare_fb,
> >> + .prepare_fb = drm_gem_fb_prepare_fb,
> >> .enable_vblank = mxsfb_pipe_enable_vblank,
> >> .disable_vblank = mxsfb_pipe_disable_vblank,
> >> };
> >> diff --git a/drivers/gpu/drm/pl111/pl111_display.c b/drivers/gpu/drm/pl111/pl111_display.c
> >> index 024771a4083e..e564c838a081 100644
> >> --- a/drivers/gpu/drm/pl111/pl111_display.c
> >> +++ b/drivers/gpu/drm/pl111/pl111_display.c
> >> @@ -441,7 +441,7 @@ static struct drm_simple_display_pipe_funcs pl111_display_funcs = {
> >> .enable = pl111_display_enable,
> >> .disable = pl111_display_disable,
> >> .update = pl111_display_update,
> >> - .prepare_fb = drm_gem_fb_simple_display_pipe_prepare_fb,
> >> + .prepare_fb = drm_gem_fb_prepare_fb,
> >> };
> >>
> >> static int pl111_clk_div_choose_div(struct clk_hw *hw, unsigned long rate,
> >> diff --git a/drivers/gpu/drm/tiny/hx8357d.c b/drivers/gpu/drm/tiny/hx8357d.c
> >> index 9af8ff84974f..235223190c04 100644
> >> --- a/drivers/gpu/drm/tiny/hx8357d.c
> >> +++ b/drivers/gpu/drm/tiny/hx8357d.c
> >> @@ -183,7 +183,7 @@ static const struct drm_simple_display_pipe_funcs hx8357d_pipe_funcs = {
> >> .enable = yx240qv29_enable,
> >> .disable = mipi_dbi_pipe_disable,
> >> .update = mipi_dbi_pipe_update,
> >> - .prepare_fb = drm_gem_fb_simple_display_pipe_prepare_fb,
> >> + .prepare_fb = drm_gem_fb_prepare_fb,
> >> };
> >>
> >> static const struct drm_display_mode yx350hv15_mode = {
> >> diff --git a/drivers/gpu/drm/tiny/ili9225.c b/drivers/gpu/drm/tiny/ili9225.c
> >> index c66acc566c2b..39b0d425359a 100644
> >> --- a/drivers/gpu/drm/tiny/ili9225.c
> >> +++ b/drivers/gpu/drm/tiny/ili9225.c
> >> @@ -342,7 +342,7 @@ static const struct drm_simple_display_pipe_funcs ili9225_pipe_funcs = {
> >> .enable = ili9225_pipe_enable,
> >> .disable = ili9225_pipe_disable,
> >> .update = ili9225_pipe_update,
> >> - .prepare_fb = drm_gem_fb_simple_display_pipe_prepare_fb,
> >> + .prepare_fb = drm_gem_fb_prepare_fb,
> >> };
> >>
> >> static const struct drm_display_mode ili9225_mode = {
> >> diff --git a/drivers/gpu/drm/tiny/ili9341.c b/drivers/gpu/drm/tiny/ili9341.c
> >> index 33b51dc7faa8..d107b6172bbb 100644
> >> --- a/drivers/gpu/drm/tiny/ili9341.c
> >> +++ b/drivers/gpu/drm/tiny/ili9341.c
> >> @@ -139,7 +139,7 @@ static const struct drm_simple_display_pipe_funcs ili9341_pipe_funcs = {
> >> .enable = yx240qv29_enable,
> >> .disable = mipi_dbi_pipe_disable,
> >> .update = mipi_dbi_pipe_update,
> >> - .prepare_fb = drm_gem_fb_simple_display_pipe_prepare_fb,
> >> + .prepare_fb = drm_gem_fb_prepare_fb,
> >> };
> >>
> >> static const struct drm_display_mode yx240qv29_mode = {
> >> diff --git a/drivers/gpu/drm/tiny/mi0283qt.c b/drivers/gpu/drm/tiny/mi0283qt.c
> >> index e2cfd9a17143..811143a17c77 100644
> >> --- a/drivers/gpu/drm/tiny/mi0283qt.c
> >> +++ b/drivers/gpu/drm/tiny/mi0283qt.c
> >> @@ -143,7 +143,7 @@ static const struct drm_simple_display_pipe_funcs mi0283qt_pipe_funcs = {
> >> .enable = mi0283qt_enable,
> >> .disable = mipi_dbi_pipe_disable,
> >> .update = mipi_dbi_pipe_update,
> >> - .prepare_fb = drm_gem_fb_simple_display_pipe_prepare_fb,
> >> + .prepare_fb = drm_gem_fb_prepare_fb,
> >> };
> >>
> >> static const struct drm_display_mode mi0283qt_mode = {
> >> diff --git a/drivers/gpu/drm/tiny/repaper.c b/drivers/gpu/drm/tiny/repaper.c
> >> index 76d179200775..9228d813d3d2 100644
> >> --- a/drivers/gpu/drm/tiny/repaper.c
> >> +++ b/drivers/gpu/drm/tiny/repaper.c
> >> @@ -874,7 +874,7 @@ static const struct drm_simple_display_pipe_funcs repaper_pipe_funcs = {
> >> .enable = repaper_pipe_enable,
> >> .disable = repaper_pipe_disable,
> >> .update = repaper_pipe_update,
> >> - .prepare_fb = drm_gem_fb_simple_display_pipe_prepare_fb,
> >> + .prepare_fb = drm_gem_fb_prepare_fb,
> >> };
> >>
> >> static int repaper_connector_get_modes(struct drm_connector *connector)
> >> diff --git a/drivers/gpu/drm/tiny/st7586.c b/drivers/gpu/drm/tiny/st7586.c
> >> index 3cc21a1b30c8..7b837c46bbfc 100644
> >> --- a/drivers/gpu/drm/tiny/st7586.c
> >> +++ b/drivers/gpu/drm/tiny/st7586.c
> >> @@ -281,7 +281,7 @@ static const struct drm_simple_display_pipe_funcs st7586_pipe_funcs = {
> >> .enable = st7586_pipe_enable,
> >> .disable = st7586_pipe_disable,
> >> .update = st7586_pipe_update,
> >> - .prepare_fb = drm_gem_fb_simple_display_pipe_prepare_fb,
> >> + .prepare_fb = drm_gem_fb_prepare_fb,
> >> };
> >>
> >> static const struct drm_display_mode st7586_mode = {
> >> diff --git a/drivers/gpu/drm/tiny/st7735r.c b/drivers/gpu/drm/tiny/st7735r.c
> >> index 3f4487c71684..4801d56a6820 100644
> >> --- a/drivers/gpu/drm/tiny/st7735r.c
> >> +++ b/drivers/gpu/drm/tiny/st7735r.c
> >> @@ -113,7 +113,7 @@ static const struct drm_simple_display_pipe_funcs jd_t18003_t01_pipe_funcs = {
> >> .enable = jd_t18003_t01_pipe_enable,
> >> .disable = mipi_dbi_pipe_disable,
> >> .update = mipi_dbi_pipe_update,
> >> - .prepare_fb = drm_gem_fb_simple_display_pipe_prepare_fb,
> >> + .prepare_fb = drm_gem_fb_prepare_fb,
> >> };
> >>
> >> static const struct drm_display_mode jd_t18003_t01_mode = {
> >> diff --git a/drivers/gpu/drm/tve200/tve200_display.c b/drivers/gpu/drm/tve200/tve200_display.c
> >> index d733bbc4ac0e..6173c61166bb 100644
> >> --- a/drivers/gpu/drm/tve200/tve200_display.c
> >> +++ b/drivers/gpu/drm/tve200/tve200_display.c
> >> @@ -297,7 +297,7 @@ static const struct drm_simple_display_pipe_funcs tve200_display_funcs = {
> >> .enable = tve200_display_enable,
> >> .disable = tve200_display_disable,
> >> .update = tve200_display_update,
> >> - .prepare_fb = drm_gem_fb_simple_display_pipe_prepare_fb,
> >> + .prepare_fb = drm_gem_fb_prepare_fb,
> >> .enable_vblank = tve200_display_enable_vblank,
> >> .disable_vblank = tve200_display_disable_vblank,
> >> };
> >> diff --git a/drivers/gpu/drm/xen/xen_drm_front_kms.c b/drivers/gpu/drm/xen/xen_drm_front_kms.c
> >> index 21ad1c359b61..5689bc91633a 100644
> >> --- a/drivers/gpu/drm/xen/xen_drm_front_kms.c
> >> +++ b/drivers/gpu/drm/xen/xen_drm_front_kms.c
> >> @@ -289,7 +289,7 @@ static const struct drm_simple_display_pipe_funcs display_funcs = {
> >> .mode_valid = display_mode_valid,
> >> .enable = display_enable,
> >> .disable = display_disable,
> >> - .prepare_fb = drm_gem_fb_simple_display_pipe_prepare_fb,
> >> + .prepare_fb = drm_gem_fb_prepare_fb,
> >> .update = display_update,
> >> };
> >>
> >> diff --git a/include/drm/drm_gem_framebuffer_helper.h b/include/drm/drm_gem_framebuffer_helper.h
> >> index d9f13fd25b0a..1b5a2ff43b4a 100644
> >> --- a/include/drm/drm_gem_framebuffer_helper.h
> >> +++ b/include/drm/drm_gem_framebuffer_helper.h
> >> @@ -10,7 +10,6 @@ struct drm_gem_object;
> >> struct drm_mode_fb_cmd2;
> >> struct drm_plane;
> >> struct drm_plane_state;
> >> -struct drm_simple_display_pipe;
> >>
> >> struct drm_gem_object *drm_gem_fb_get_obj(struct drm_framebuffer *fb,
> >> unsigned int plane);
> >> @@ -31,6 +30,5 @@ drm_gem_fb_create_with_dirty(struct drm_device *dev, struct drm_file *file,
> >>
> >> int drm_gem_fb_prepare_fb(struct drm_plane *plane,
> >> struct drm_plane_state *state);
> >> -int drm_gem_fb_simple_display_pipe_prepare_fb(struct drm_simple_display_pipe *pipe,
> >> - struct drm_plane_state *plane_state);
> >> +
> >> #endif
> >> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> >> index 3f396d94afe4..4065661eb497 100644
> >> --- a/include/drm/drm_plane.h
> >> +++ b/include/drm/drm_plane.h
> >> @@ -75,7 +75,7 @@ struct drm_plane_state {
> >> *
> >> * Drivers should store any implicit fence in this from their
> >> * &drm_plane_helper_funcs.prepare_fb callback. See drm_gem_fb_prepare_fb()
> >> - * and drm_gem_fb_simple_display_pipe_prepare_fb() for suitable helpers.
> >> + * for a suitable helper.
> >> */
> >> struct dma_fence *fence;
> >>
> >> diff --git a/include/drm/drm_simple_kms_helper.h b/include/drm/drm_simple_kms_helper.h
> >> index 4d89cd0a60db..9d6ae8c9769f 100644
> >> --- a/include/drm/drm_simple_kms_helper.h
> >> +++ b/include/drm/drm_simple_kms_helper.h
> >> @@ -114,9 +114,9 @@ struct drm_simple_display_pipe_funcs {
> >> * more details.
> >> *
> >> * Drivers which always have their buffers pinned should use
> >> - * drm_gem_fb_simple_display_pipe_prepare_fb() for this hook.
> >> + * drm_gem_fb_prepare_fb() for this hook.
> >> */
> >> - int (*prepare_fb)(struct drm_simple_display_pipe *pipe,
> >> + int (*prepare_fb)(struct drm_plane *plane,
> >> struct drm_plane_state *plane_state);
> >>
> >> /**
> >> @@ -126,7 +126,7 @@ struct drm_simple_display_pipe_funcs {
> >> * the documentation for the &drm_plane_helper_funcs.cleanup_fb hook for
> >> * more details.
> >> */
> >> - void (*cleanup_fb)(struct drm_simple_display_pipe *pipe,
> >> + void (*cleanup_fb)(struct drm_plane *plane,
> >> struct drm_plane_state *plane_state);
> >>
> >> /**
> >> --
> >> 2.23.0
> >>
> >
>
> --
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> (HRB 36809, AG Nürnberg)
> Geschäftsführer: Felix Imendörffer
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
More information about the dri-devel
mailing list