On Mon, Jul 27, 2020 at 1:37 PM Thomas Zimmermann <tzimmermann@xxxxxxx> wrote:
>
> Hi
>
> Am 27.07.20 um 12:42 schrieb daniel@xxxxxxxx:
> > On Mon, Jul 27, 2020 at 09:37:06AM +0200, Thomas Zimmermann wrote:
> >> Store the image size as part of the HW cursor info, so that the
> >> cursor show function doesn't require the information from the
> >> caller. No functional changes.
> >
> > Uh just pass the state structure and done? All these "store random stuff
> > in private structures" (they're not even atomic state structures, it's the
> > driver private thing even!) is very non-atomic. And I see zero reasons why
> > you have to do this, the cursor stays around.
>
> It's not random stuff. Ast cannot use ARGB8888 for cursors. Anything in
> ast_private.cursor represents cursor hardware state (not DRM state);
> duplicated for double buffering.
>
> * gbo: two perma-pinned GEM objects at the end of VRAM. It's the HW
> cursor buffer in ARGB4444 format. The userspace's cursor image is
> converted to ARGB4444 and copied into the current backbuffer.
>
> * vaddr: A mapping of the gbo's into kernel address space. We don't
> want to map the gbo on each update, so they are mapped once and the
> kernel address is stored in vaddr.
>
> * size: the size of each HW buffer. We could use the value in the fb,
> but storing this as well makes the cursor code self-contained.
Yeah, but this kind of stuff should be in the ast_plane_state. Not in
ast_private, that latter option is very non-atomic and results in all
kinds of coordination fun.
-Daniel
>
> Best regards
> Thomas
>
> > -Daniel
> >
> >>
> >> Signed-off-by: Thomas Zimmermann <tzimmermann@xxxxxxx>
> >> Fixes: 4961eb60f145 ("drm/ast: Enable atomic modesetting")
> >> Cc: Thomas Zimmermann <tzimmermann@xxxxxxx>
> >> Cc: Gerd Hoffmann <kraxel@xxxxxxxxxx>
> >> Cc: Dave Airlie <airlied@xxxxxxxxxx>
> >> Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
> >> Cc: Sam Ravnborg <sam@xxxxxxxxxxxx>
> >> Cc: Emil Velikov <emil.l.velikov@xxxxxxxxx>
> >> Cc: "Y.C. Chen" <yc_chen@xxxxxxxxxxxxxx>
> >> Cc: <stable@xxxxxxxxxxxxxxx> # v5.6+
> >> ---
> >> drivers/gpu/drm/ast/ast_cursor.c | 13 +++++++++++--
> >> drivers/gpu/drm/ast/ast_drv.h | 7 +++++--
> >> drivers/gpu/drm/ast/ast_mode.c | 8 +-------
> >> 3 files changed, 17 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/ast/ast_cursor.c b/drivers/gpu/drm/ast/ast_cursor.c
> >> index acf0d23514e8..8642a0ce9da6 100644
> >> --- a/drivers/gpu/drm/ast/ast_cursor.c
> >> +++ b/drivers/gpu/drm/ast/ast_cursor.c
> >> @@ -87,6 +87,8 @@ int ast_cursor_init(struct ast_private *ast)
> >>
> >> ast->cursor.gbo[i] = gbo;
> >> ast->cursor.vaddr[i] = vaddr;
> >> + ast->cursor.size[i].width = 0;
> >> + ast->cursor.size[i].height = 0;
> >> }
> >>
> >> return drmm_add_action_or_reset(dev, ast_cursor_release, NULL);
> >> @@ -194,6 +196,9 @@ int ast_cursor_blit(struct ast_private *ast, struct drm_framebuffer *fb)
> >> /* do data transfer to cursor BO */
> >> update_cursor_image(dst, src, fb->width, fb->height);
> >>
> >> + ast->cursor.size[ast->cursor.next_index].width = fb->width;
> >> + ast->cursor.size[ast->cursor.next_index].height = fb->height;
> >> +
> >> drm_gem_vram_vunmap(gbo, src);
> >> drm_gem_vram_unpin(gbo);
> >>
> >> @@ -249,14 +254,18 @@ static void ast_cursor_set_location(struct ast_private *ast, u16 x, u16 y,
> >> ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xc7, y1);
> >> }
> >>
> >> -void ast_cursor_show(struct ast_private *ast, int x, int y,
> >> - unsigned int offset_x, unsigned int offset_y)
> >> +void ast_cursor_show(struct ast_private *ast, int x, int y)
> >> {
> >> + unsigned int offset_x, offset_y;
> >> u8 x_offset, y_offset;
> >> u8 __iomem *dst, __iomem *sig;
> >> u8 jreg;
> >>
> >> dst = ast->cursor.vaddr[ast->cursor.next_index];
> >> + offset_x = AST_MAX_HWC_WIDTH -
> >> + ast->cursor.size[ast->cursor.next_index].width;
> >> + offset_y = AST_MAX_HWC_HEIGHT -
> >> + ast->cursor.size[ast->cursor.next_index].height;
> >>
> >> sig = dst + AST_HWC_SIZE;
> >> writel(x, sig + AST_HWC_SIGNATURE_X);
> >> diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
> >> index e3a264ac7ee2..57414b429db3 100644
> >> --- a/drivers/gpu/drm/ast/ast_drv.h
> >> +++ b/drivers/gpu/drm/ast/ast_drv.h
> >> @@ -116,6 +116,10 @@ struct ast_private {
> >> struct {
> >> struct drm_gem_vram_object *gbo[AST_DEFAULT_HWC_NUM];
> >> void __iomem *vaddr[AST_DEFAULT_HWC_NUM];
> >> + struct {
> >> + unsigned int width;
> >> + unsigned int height;
> >> + } size[AST_DEFAULT_HWC_NUM];
> >> unsigned int next_index;
> >> } cursor;
> >>
> >> @@ -311,8 +315,7 @@ void ast_release_firmware(struct drm_device *dev);
> >> int ast_cursor_init(struct ast_private *ast);
> >> int ast_cursor_blit(struct ast_private *ast, struct drm_framebuffer *fb);
> >> void ast_cursor_page_flip(struct ast_private *ast);
> >> -void ast_cursor_show(struct ast_private *ast, int x, int y,
> >> - unsigned int offset_x, unsigned int offset_y);
> >> +void ast_cursor_show(struct ast_private *ast, int x, int y);
> >> void ast_cursor_hide(struct ast_private *ast);
> >>
> >> #endif
> >> diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
> >> index 3680a000b812..5b2b39c93033 100644
> >> --- a/drivers/gpu/drm/ast/ast_mode.c
> >> +++ b/drivers/gpu/drm/ast/ast_mode.c
> >> @@ -671,20 +671,14 @@ ast_cursor_plane_helper_atomic_update(struct drm_plane *plane,
> >> struct drm_plane_state *old_state)
> >> {
> >> struct drm_plane_state *state = plane->state;
> >> - struct drm_framebuffer *fb = state->fb;
> >> struct ast_private *ast = plane->dev->dev_private;
> >> - unsigned int offset_x, offset_y;
> >> -
> >> - offset_x = AST_MAX_HWC_WIDTH - fb->width;
> >> - offset_y = AST_MAX_HWC_WIDTH - fb->height;
> >>
> >> if (state->fb != old_state->fb) {
> >> /* A new cursor image was installed. */
> >> ast_cursor_page_flip(ast);
> >> }
> >>
> >> - ast_cursor_show(ast, state->crtc_x, state->crtc_y,
> >> - offset_x, offset_y);
> >> + ast_cursor_show(ast, state->crtc_x, state->crtc_y);
> >> }
> >>
> >> static void
> >> --
> >> 2.27.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
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
