[PATCH 1/3] drm/ast: Do full modeset if the primary plane's format changes

[PATCH 1/3] drm/ast: Do full modeset if the primary plane's format changes

[Thomas Zimmermann]

The atomic modesetting code tried to distinguish format changes from
full modesetting operations. In practice, this was buggy and the format
registers were often updated even for simple pageflips.

Instead do a full modeset if the primary plane changes formats. It's
just as rare as an actual mode change, so there will be no performance
penalty.

The patch also replaces a reference to drm_crtc_state.allow_modeset with
the correct call to drm_atomic_crtc_needs_modeset().

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Fixes: 4961eb60f145 ("drm/ast: Enable atomic modesetting")
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Dave Airlie <airlied@redhat.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: Emil Velikov <emil.l.velikov@gmail.com>
Cc: "Y.C. Chen" <yc_chen@aspeedtech.com>
Cc: <stable@vger.kernel.org> # v5.6+
---
 drivers/gpu/drm/ast/ast_mode.c | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index 154cd877d9d1..3680a000b812 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -527,8 +527,8 @@ static const uint32_t ast_primary_plane_formats[] = {
 static int ast_primary_plane_helper_atomic_check(struct drm_plane *plane,
 						 struct drm_plane_state *state)
 {
-	struct drm_crtc_state *crtc_state;
-	struct ast_crtc_state *ast_crtc_state;
+	struct drm_crtc_state *crtc_state, *old_crtc_state;
+	struct ast_crtc_state *ast_crtc_state, *old_ast_crtc_state;
 	int ret;
 
 	if (!state->crtc)
@@ -550,6 +550,15 @@ static int ast_primary_plane_helper_atomic_check(struct drm_plane *plane,
 
 	ast_crtc_state->format = state->fb->format;
 
+	old_crtc_state = drm_atomic_get_old_crtc_state(state->state, state->crtc);
+	if (!old_crtc_state)
+		return 0;
+	old_ast_crtc_state = to_ast_crtc_state(old_crtc_state);
+	if (!old_ast_crtc_state)
+		return 0;
+	if (ast_crtc_state->format != old_ast_crtc_state->format)
+		crtc_state->mode_changed = true;
+
 	return 0;
 }
 
@@ -775,18 +784,18 @@ static void ast_crtc_helper_atomic_flush(struct drm_crtc *crtc,
 
 	ast_state = to_ast_crtc_state(crtc->state);
 
-	format = ast_state->format;
-	if (!format)
+	if (!drm_atomic_crtc_needs_modeset(crtc->state))
 		return;
 
+	format = ast_state->format;
+	if (drm_WARN_ON_ONCE(dev, !format))
+		return; /* BUG: We didn't set format in primary check(). */
+
 	vbios_mode_info = &ast_state->vbios_mode_info;
 
 	ast_set_color_reg(ast, format);
 	ast_set_vbios_color_reg(ast, format, vbios_mode_info);
 
-	if (!crtc->state->mode_changed)
-		return;
-
 	adjusted_mode = &crtc->state->adjusted_mode;
 
 	ast_set_vbios_mode_reg(ast, adjusted_mode, vbios_mode_info);
-- 
2.27.0

Re: [PATCH 1/3] drm/ast: Do full modeset if the primary plane's format changes

[daniel]

On Mon, Jul 27, 2020 at 09:37:05AM +0200, Thomas Zimmermann wrote:
> The atomic modesetting code tried to distinguish format changes from
> full modesetting operations. In practice, this was buggy and the format
> registers were often updated even for simple pageflips.

Nah it's not buggy, it's intentional. Most hw can update formats in a flip
withouth having to shut down the hw to do so.


> Instead do a full modeset if the primary plane changes formats. It's
> just as rare as an actual mode change, so there will be no performance
> penalty.
> 
> The patch also replaces a reference to drm_crtc_state.allow_modeset with
> the correct call to drm_atomic_crtc_needs_modeset().
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Fixes: 4961eb60f145 ("drm/ast: Enable atomic modesetting")
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Dave Airlie <airlied@redhat.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Sam Ravnborg <sam@ravnborg.org>
> Cc: Emil Velikov <emil.l.velikov@gmail.com>
> Cc: "Y.C. Chen" <yc_chen@aspeedtech.com>
> Cc: <stable@vger.kernel.org> # v5.6+
> ---
>  drivers/gpu/drm/ast/ast_mode.c | 23 ++++++++++++++++-------
>  1 file changed, 16 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
> index 154cd877d9d1..3680a000b812 100644
> --- a/drivers/gpu/drm/ast/ast_mode.c
> +++ b/drivers/gpu/drm/ast/ast_mode.c
> @@ -527,8 +527,8 @@ static const uint32_t ast_primary_plane_formats[] = {
>  static int ast_primary_plane_helper_atomic_check(struct drm_plane *plane,
>  						 struct drm_plane_state *state)
>  {
> -	struct drm_crtc_state *crtc_state;
> -	struct ast_crtc_state *ast_crtc_state;
> +	struct drm_crtc_state *crtc_state, *old_crtc_state;
> +	struct ast_crtc_state *ast_crtc_state, *old_ast_crtc_state;
>  	int ret;
>  
>  	if (!state->crtc)
> @@ -550,6 +550,15 @@ static int ast_primary_plane_helper_atomic_check(struct drm_plane *plane,
>  
>  	ast_crtc_state->format = state->fb->format;
>  
> +	old_crtc_state = drm_atomic_get_old_crtc_state(state->state, state->crtc);
> +	if (!old_crtc_state)
> +		return 0;
> +	old_ast_crtc_state = to_ast_crtc_state(old_crtc_state);
> +	if (!old_ast_crtc_state)

The above two if checks should never fail, I'd wrap them in a WARN_ON.

> +		return 0;
> +	if (ast_crtc_state->format != old_ast_crtc_state->format)
> +		crtc_state->mode_changed = true;
> +
>  	return 0;
>  }
>  
> @@ -775,18 +784,18 @@ static void ast_crtc_helper_atomic_flush(struct drm_crtc *crtc,
>  
>  	ast_state = to_ast_crtc_state(crtc->state);
>  
> -	format = ast_state->format;
> -	if (!format)
> +	if (!drm_atomic_crtc_needs_modeset(crtc->state))
>  		return;
>  
> +	format = ast_state->format;
> +	if (drm_WARN_ON_ONCE(dev, !format))
> +		return; /* BUG: We didn't set format in primary check(). */

Hm that entire ast_state->format machinery looks kinda strange, can't you
just look up the primary plane state everywhere and that's it?
drm_framebuffer are fully invariant and refcounted to the state, so there
really shouldn't be any need to copy format around.

But that's maybe for a next patch. With the commit message clarified that
everything works as designed, and maybe the two WARN_ON added:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> +
>  	vbios_mode_info = &ast_state->vbios_mode_info;
>  
>  	ast_set_color_reg(ast, format);
>  	ast_set_vbios_color_reg(ast, format, vbios_mode_info);
>  
> -	if (!crtc->state->mode_changed)
> -		return;
> -
>  	adjusted_mode = &crtc->state->adjusted_mode;
>  
>  	ast_set_vbios_mode_reg(ast, adjusted_mode, vbios_mode_info);
> -- 
> 2.27.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 1/3] drm/ast: Do full modeset if the primary plane's format changes

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 4969 bytes --]

Hi

Am 27.07.20 um 12:40 schrieb daniel@ffwll.ch:
> On Mon, Jul 27, 2020 at 09:37:05AM +0200, Thomas Zimmermann wrote:
>> The atomic modesetting code tried to distinguish format changes from
>> full modesetting operations. In practice, this was buggy and the format
>> registers were often updated even for simple pageflips.
> 
> Nah it's not buggy, it's intentional. Most hw can update formats in a flip
> withouth having to shut down the hw to do so.

Admittedly it was intentional when I write it, but it never really
worked. I think it might have even updated these color registers on each
frame.

> 
> 
>> Instead do a full modeset if the primary plane changes formats. It's
>> just as rare as an actual mode change, so there will be no performance
>> penalty.
>>
>> The patch also replaces a reference to drm_crtc_state.allow_modeset with
>> the correct call to drm_atomic_crtc_needs_modeset().
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> Fixes: 4961eb60f145 ("drm/ast: Enable atomic modesetting")
>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
>> Cc: Gerd Hoffmann <kraxel@redhat.com>
>> Cc: Dave Airlie <airlied@redhat.com>
>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> Cc: Sam Ravnborg <sam@ravnborg.org>
>> Cc: Emil Velikov <emil.l.velikov@gmail.com>
>> Cc: "Y.C. Chen" <yc_chen@aspeedtech.com>
>> Cc: <stable@vger.kernel.org> # v5.6+
>> ---
>>  drivers/gpu/drm/ast/ast_mode.c | 23 ++++++++++++++++-------
>>  1 file changed, 16 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
>> index 154cd877d9d1..3680a000b812 100644
>> --- a/drivers/gpu/drm/ast/ast_mode.c
>> +++ b/drivers/gpu/drm/ast/ast_mode.c
>> @@ -527,8 +527,8 @@ static const uint32_t ast_primary_plane_formats[] = {
>>  static int ast_primary_plane_helper_atomic_check(struct drm_plane *plane,
>>  						 struct drm_plane_state *state)
>>  {
>> -	struct drm_crtc_state *crtc_state;
>> -	struct ast_crtc_state *ast_crtc_state;
>> +	struct drm_crtc_state *crtc_state, *old_crtc_state;
>> +	struct ast_crtc_state *ast_crtc_state, *old_ast_crtc_state;
>>  	int ret;
>>  
>>  	if (!state->crtc)
>> @@ -550,6 +550,15 @@ static int ast_primary_plane_helper_atomic_check(struct drm_plane *plane,
>>  
>>  	ast_crtc_state->format = state->fb->format;
>>  
>> +	old_crtc_state = drm_atomic_get_old_crtc_state(state->state, state->crtc);
>> +	if (!old_crtc_state)
>> +		return 0;
>> +	old_ast_crtc_state = to_ast_crtc_state(old_crtc_state);
>> +	if (!old_ast_crtc_state)
> 
> The above two if checks should never fail, I'd wrap them in a WARN_ON.

Really? But what's the old state when the first mode is being programmed?

> 
>> +		return 0;
>> +	if (ast_crtc_state->format != old_ast_crtc_state->format)
>> +		crtc_state->mode_changed = true;
>> +
>>  	return 0;
>>  }
>>  
>> @@ -775,18 +784,18 @@ static void ast_crtc_helper_atomic_flush(struct drm_crtc *crtc,
>>  
>>  	ast_state = to_ast_crtc_state(crtc->state);
>>  
>> -	format = ast_state->format;
>> -	if (!format)
>> +	if (!drm_atomic_crtc_needs_modeset(crtc->state))
>>  		return;
>>  
>> +	format = ast_state->format;
>> +	if (drm_WARN_ON_ONCE(dev, !format))
>> +		return; /* BUG: We didn't set format in primary check(). */
> 
> Hm that entire ast_state->format machinery looks kinda strange, can't you
> just look up the primary plane state everywhere and that's it?
> drm_framebuffer are fully invariant and refcounted to the state, so there
> really shouldn't be any need to copy format around.

ast_state->format is the format that has to be programmed in
atomic_flush(). If it's NULL, the current format was used. Updating the
primary plane's format also requires the vbios info, which depends on
CRTC state. So it's collected in the CRTC's atomic_check().

It felt natural to use the various atomic_check() functions to collect
and store and store away these structures, and later use them in
atomic_flush().

I'd prefer to keep the current design. It's the one that worked best
while writing the atomic-modesetting support for ast.

Best regard
Thomas

> 
> But that's maybe for a next patch. With the commit message clarified that
> everything works as designed, and maybe the two WARN_ON added:
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
>> +
>>  	vbios_mode_info = &ast_state->vbios_mode_info;
>>  
>>  	ast_set_color_reg(ast, format);
>>  	ast_set_vbios_color_reg(ast, format, vbios_mode_info);
>>  
>> -	if (!crtc->state->mode_changed)
>> -		return;
>> -
>>  	adjusted_mode = &crtc->state->adjusted_mode;
>>  
>>  	ast_set_vbios_mode_reg(ast, adjusted_mode, vbios_mode_info);
>> -- 
>> 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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 516 bytes --]

Re: [PATCH 1/3] drm/ast: Do full modeset if the primary plane's format changes

[Daniel Vetter]

On Mon, Jul 27, 2020 at 1:24 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> Hi
>
> Am 27.07.20 um 12:40 schrieb daniel@ffwll.ch:
> > On Mon, Jul 27, 2020 at 09:37:05AM +0200, Thomas Zimmermann wrote:
> >> The atomic modesetting code tried to distinguish format changes from
> >> full modesetting operations. In practice, this was buggy and the format
> >> registers were often updated even for simple pageflips.
> >
> > Nah it's not buggy, it's intentional. Most hw can update formats in a flip
> > withouth having to shut down the hw to do so.
>
> Admittedly it was intentional when I write it, but it never really
> worked. I think it might have even updated these color registers on each
> frame.
>
> >
> >
> >> Instead do a full modeset if the primary plane changes formats. It's
> >> just as rare as an actual mode change, so there will be no performance
> >> penalty.
> >>
> >> The patch also replaces a reference to drm_crtc_state.allow_modeset with
> >> the correct call to drm_atomic_crtc_needs_modeset().
> >>
> >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> >> Fixes: 4961eb60f145 ("drm/ast: Enable atomic modesetting")
> >> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> >> Cc: Gerd Hoffmann <kraxel@redhat.com>
> >> Cc: Dave Airlie <airlied@redhat.com>
> >> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> >> Cc: Sam Ravnborg <sam@ravnborg.org>
> >> Cc: Emil Velikov <emil.l.velikov@gmail.com>
> >> Cc: "Y.C. Chen" <yc_chen@aspeedtech.com>
> >> Cc: <stable@vger.kernel.org> # v5.6+
> >> ---
> >>  drivers/gpu/drm/ast/ast_mode.c | 23 ++++++++++++++++-------
> >>  1 file changed, 16 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
> >> index 154cd877d9d1..3680a000b812 100644
> >> --- a/drivers/gpu/drm/ast/ast_mode.c
> >> +++ b/drivers/gpu/drm/ast/ast_mode.c
> >> @@ -527,8 +527,8 @@ static const uint32_t ast_primary_plane_formats[] = {
> >>  static int ast_primary_plane_helper_atomic_check(struct drm_plane *plane,
> >>                                               struct drm_plane_state *state)
> >>  {
> >> -    struct drm_crtc_state *crtc_state;
> >> -    struct ast_crtc_state *ast_crtc_state;
> >> +    struct drm_crtc_state *crtc_state, *old_crtc_state;
> >> +    struct ast_crtc_state *ast_crtc_state, *old_ast_crtc_state;
> >>      int ret;
> >>
> >>      if (!state->crtc)
> >> @@ -550,6 +550,15 @@ static int ast_primary_plane_helper_atomic_check(struct drm_plane *plane,
> >>
> >>      ast_crtc_state->format = state->fb->format;
> >>
> >> +    old_crtc_state = drm_atomic_get_old_crtc_state(state->state, state->crtc);
> >> +    if (!old_crtc_state)
> >> +            return 0;
> >> +    old_ast_crtc_state = to_ast_crtc_state(old_crtc_state);
> >> +    if (!old_ast_crtc_state)
> >
> > The above two if checks should never fail, I'd wrap them in a WARN_ON.
>
> Really? But what's the old state when the first mode is being programmed?

Uh, atomic _always_ has a state. That's why you need to call
drm_mode_config_reset, to get this entire machinery started. Also, the
sw state is supposed to always match the hw state (at least once all
the nonblocking commit workers have caught up).

> >
> >> +            return 0;
> >> +    if (ast_crtc_state->format != old_ast_crtc_state->format)
> >> +            crtc_state->mode_changed = true;
> >> +
> >>      return 0;
> >>  }
> >>
> >> @@ -775,18 +784,18 @@ static void ast_crtc_helper_atomic_flush(struct drm_crtc *crtc,
> >>
> >>      ast_state = to_ast_crtc_state(crtc->state);
> >>
> >> -    format = ast_state->format;
> >> -    if (!format)
> >> +    if (!drm_atomic_crtc_needs_modeset(crtc->state))
> >>              return;
> >>
> >> +    format = ast_state->format;
> >> +    if (drm_WARN_ON_ONCE(dev, !format))
> >> +            return; /* BUG: We didn't set format in primary check(). */
> >
> > Hm that entire ast_state->format machinery looks kinda strange, can't you
> > just look up the primary plane state everywhere and that's it?
> > drm_framebuffer are fully invariant and refcounted to the state, so there
> > really shouldn't be any need to copy format around.
>
> ast_state->format is the format that has to be programmed in
> atomic_flush(). If it's NULL, the current format was used. Updating the
> primary plane's format also requires the vbios info, which depends on
> CRTC state. So it's collected in the CRTC's atomic_check().
>
> It felt natural to use the various atomic_check() functions to collect
> and store and store away these structures, and later use them in
> atomic_flush().
>
> I'd prefer to keep the current design. It's the one that worked best
> while writing the atomic-modesetting support for ast.

So if you have actual checks in atomic_check for validation, then this
makes sense - it avoids computing stuff twice and getting in wrong in
one case.

But from reading ast code all you do is store the temporary lookup in
there, and that's really just confusing. You can just look up the
atomic state, at least in atomic_check (in the commit side it's mildly
more annoying, we need to fix a few things). But it's also not totally
horrible ofc, just would be nice to improve this.
-Daniel


> Best regard
> Thomas
>
> >
> > But that's maybe for a next patch. With the commit message clarified that
> > everything works as designed, and maybe the two WARN_ON added:
> >
> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> >
> >> +
> >>      vbios_mode_info = &ast_state->vbios_mode_info;
> >>
> >>      ast_set_color_reg(ast, format);
> >>      ast_set_vbios_color_reg(ast, format, vbios_mode_info);
> >>
> >> -    if (!crtc->state->mode_changed)
> >> -            return;
> >> -
> >>      adjusted_mode = &crtc->state->adjusted_mode;
> >>
> >>      ast_set_vbios_mode_reg(ast, adjusted_mode, vbios_mode_info);
> >> --
> >> 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

[PATCH 2/3] drm/ast: Store image size in HW cursor info

[Thomas Zimmermann]

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.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Fixes: 4961eb60f145 ("drm/ast: Enable atomic modesetting")
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Dave Airlie <airlied@redhat.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: Emil Velikov <emil.l.velikov@gmail.com>
Cc: "Y.C. Chen" <yc_chen@aspeedtech.com>
Cc: <stable@vger.kernel.org> # 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

Re: [PATCH 2/3] drm/ast: Store image size in HW cursor info

[daniel]

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.
-Daniel

> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Fixes: 4961eb60f145 ("drm/ast: Enable atomic modesetting")
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Dave Airlie <airlied@redhat.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Sam Ravnborg <sam@ravnborg.org>
> Cc: Emil Velikov <emil.l.velikov@gmail.com>
> Cc: "Y.C. Chen" <yc_chen@aspeedtech.com>
> Cc: <stable@vger.kernel.org> # 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
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 2/3] drm/ast: Store image size in HW cursor info

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 6031 bytes --]

Hi

Am 27.07.20 um 12:42 schrieb daniel@ffwll.ch:
> 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.

Best regards
Thomas

> -Daniel
> 
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> Fixes: 4961eb60f145 ("drm/ast: Enable atomic modesetting")
>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
>> Cc: Gerd Hoffmann <kraxel@redhat.com>
>> Cc: Dave Airlie <airlied@redhat.com>
>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> Cc: Sam Ravnborg <sam@ravnborg.org>
>> Cc: Emil Velikov <emil.l.velikov@gmail.com>
>> Cc: "Y.C. Chen" <yc_chen@aspeedtech.com>
>> Cc: <stable@vger.kernel.org> # 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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 516 bytes --]

Re: [PATCH 2/3] drm/ast: Store image size in HW cursor info

[Daniel Vetter]

On Mon, Jul 27, 2020 at 1:37 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> Hi
>
> Am 27.07.20 um 12:42 schrieb daniel@ffwll.ch:
> > 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@suse.de>
> >> Fixes: 4961eb60f145 ("drm/ast: Enable atomic modesetting")
> >> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> >> Cc: Gerd Hoffmann <kraxel@redhat.com>
> >> Cc: Dave Airlie <airlied@redhat.com>
> >> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> >> Cc: Sam Ravnborg <sam@ravnborg.org>
> >> Cc: Emil Velikov <emil.l.velikov@gmail.com>
> >> Cc: "Y.C. Chen" <yc_chen@aspeedtech.com>
> >> Cc: <stable@vger.kernel.org> # 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

[PATCH 3/3] drm/ast: Disable cursor while switching display modes

[Thomas Zimmermann]

The ast's HW cursor requires the primary plane and CRTC to display
at a correct mode and format. This is not the case while switching
display modes, which can lead to the screen turing permanently dark.

As a workaround, the ast driver now disables active HW cursors while
the mode switch takes place. It also synchronizes with the vertical
refresh to give HW cursor and primary plane some time to catch up on
each other.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Fixes: 4961eb60f145 ("drm/ast: Enable atomic modesetting")
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Dave Airlie <airlied@redhat.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: Emil Velikov <emil.l.velikov@gmail.com>
Cc: "Y.C. Chen" <yc_chen@aspeedtech.com>
Cc: <stable@vger.kernel.org> # v5.6+
---
 drivers/gpu/drm/ast/ast_drv.h  |  2 ++
 drivers/gpu/drm/ast/ast_mode.c | 53 +++++++++++++++++++++++++++++++++-
 2 files changed, 54 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
index 57414b429db3..564670b5d2ee 100644
--- a/drivers/gpu/drm/ast/ast_drv.h
+++ b/drivers/gpu/drm/ast/ast_drv.h
@@ -162,6 +162,8 @@ void ast_driver_unload(struct drm_device *dev);
 
 #define AST_IO_MM_OFFSET		(0x380)
 
+#define AST_IO_VGAIR1_VREFRESH		BIT(3)
+
 #define __ast_read(x) \
 static inline u##x ast_read##x(struct ast_private *ast, u32 reg) { \
 u##x val = 0;\
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index 5b2b39c93033..e18365bbc08c 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -514,6 +514,17 @@ static void ast_set_start_address_crt1(struct ast_private *ast,
 
 }
 
+static void ast_wait_for_vretrace(struct ast_private *ast)
+{
+	unsigned long timeout = jiffies + HZ;
+	u8 vgair1;
+
+	do {
+		vgair1 = ast_io_read8(ast, AST_IO_INPUT_STATUS1_READ);
+	} while (!(vgair1 & AST_IO_VGAIR1_VREFRESH) &&
+		 time_before(jiffies, timeout));
+}
+
 /*
  * Primary plane
  */
@@ -666,6 +677,14 @@ static int ast_cursor_plane_helper_atomic_check(struct drm_plane *plane,
 	return 0;
 }
 
+static bool ast_disable_cursor_during_modeset(struct drm_plane *cursor_plane)
+{
+	const struct drm_plane_state *cursor_state = cursor_plane->state;
+
+	return cursor_state && cursor_state->visible && cursor_state->crtc &&
+	       drm_atomic_crtc_needs_modeset(cursor_state->crtc->state);
+}
+
 static void
 ast_cursor_plane_helper_atomic_update(struct drm_plane *plane,
 				      struct drm_plane_state *old_state)
@@ -678,7 +697,12 @@ ast_cursor_plane_helper_atomic_update(struct drm_plane *plane,
 		ast_cursor_page_flip(ast);
 	}
 
-	ast_cursor_show(ast, state->crtc_x, state->crtc_y);
+	/*
+	 * For modesets, delay show() until end of atomic_flush(). See the
+	 * atomic_begin() helper for more information.
+	 */
+	if (!ast_disable_cursor_during_modeset(plane))
+		ast_cursor_show(ast, state->crtc_x, state->crtc_y);
 }
 
 static void
@@ -764,6 +788,22 @@ static void ast_crtc_helper_atomic_begin(struct drm_crtc *crtc,
 	struct ast_private *ast = to_ast_private(crtc->dev);
 
 	ast_open_key(ast);
+
+	/*
+	 * HW cursors require the underlying primary plane and CRTC to
+	 * display a valid mode and image. This is not the case during
+	 * full modeset operations. So we temporarily disable any active
+	 * HW cursor and re-enable it at the end of the atomic_flush()
+	 * helper. The busy waiting allows the code to sync with the
+	 * vertical refresh.
+	 *
+	 * We only do this during *full* modesets. It does not affect
+	 * simple pageflips on the planes.
+	 */
+	if (ast_disable_cursor_during_modeset(&ast->cursor_plane)) {
+		ast_cursor_hide(ast);
+		ast_wait_for_vretrace(ast);
+	}
 }
 
 static void ast_crtc_helper_atomic_flush(struct drm_crtc *crtc,
@@ -771,6 +811,7 @@ static void ast_crtc_helper_atomic_flush(struct drm_crtc *crtc,
 {
 	struct drm_device *dev = crtc->dev;
 	struct ast_private *ast = to_ast_private(dev);
+	struct drm_plane_state *cursor_state = ast->cursor_plane.state;
 	struct ast_crtc_state *ast_state;
 	const struct drm_format_info *format;
 	struct ast_vbios_mode_info *vbios_mode_info;
@@ -799,6 +840,16 @@ static void ast_crtc_helper_atomic_flush(struct drm_crtc *crtc,
 	ast_set_dclk_reg(ast, adjusted_mode, vbios_mode_info);
 	ast_set_crtthd_reg(ast);
 	ast_set_sync_reg(ast, adjusted_mode, vbios_mode_info);
+
+	/*
+	 * Re-enabling the HW cursor; if any. See the atomic_begin() helper
+	 * for more information.
+	 */
+	if (ast_disable_cursor_during_modeset(&ast->cursor_plane)) {
+		ast_wait_for_vretrace(ast);
+		ast_cursor_show(ast, cursor_state->crtc_x,
+				cursor_state->crtc_y);
+	}
 }
 
 static void
-- 
2.27.0

Re: [PATCH 3/3] drm/ast: Disable cursor while switching display modes

[daniel]

On Mon, Jul 27, 2020 at 09:37:07AM +0200, Thomas Zimmermann wrote:
> The ast's HW cursor requires the primary plane and CRTC to display
> at a correct mode and format. This is not the case while switching
> display modes, which can lead to the screen turing permanently dark.
> 
> As a workaround, the ast driver now disables active HW cursors while
> the mode switch takes place. It also synchronizes with the vertical
> refresh to give HW cursor and primary plane some time to catch up on
> each other.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Fixes: 4961eb60f145 ("drm/ast: Enable atomic modesetting")

Since you already do modeset when changing primary plane I think the much
cleaner solution is to use drm_atomic_helper_disable_planes_on_crtc() and
drm_atomic_helper_commit_planes() with flags =
DRM_PLANE_COMMIT_ACTIVE_ONLY or so, with corresponding changes in
atomic_commit_tail. Much cleaner instead of hand-rolling this all in
callbacks.

Note that with atomic helpers it is _very_ much encouraged to throw the
helper structure into the wind and write your own stuff, this thing is
intentionally very modular. This is to avoid incomprehensible drivers that
are forced to hack around the helper midlayer in their callbacks like the
below very much looks like.
-Daniel

> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Dave Airlie <airlied@redhat.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Sam Ravnborg <sam@ravnborg.org>
> Cc: Emil Velikov <emil.l.velikov@gmail.com>
> Cc: "Y.C. Chen" <yc_chen@aspeedtech.com>
> Cc: <stable@vger.kernel.org> # v5.6+
> ---
>  drivers/gpu/drm/ast/ast_drv.h  |  2 ++
>  drivers/gpu/drm/ast/ast_mode.c | 53 +++++++++++++++++++++++++++++++++-
>  2 files changed, 54 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
> index 57414b429db3..564670b5d2ee 100644
> --- a/drivers/gpu/drm/ast/ast_drv.h
> +++ b/drivers/gpu/drm/ast/ast_drv.h
> @@ -162,6 +162,8 @@ void ast_driver_unload(struct drm_device *dev);
>  
>  #define AST_IO_MM_OFFSET		(0x380)
>  
> +#define AST_IO_VGAIR1_VREFRESH		BIT(3)
> +
>  #define __ast_read(x) \
>  static inline u##x ast_read##x(struct ast_private *ast, u32 reg) { \
>  u##x val = 0;\
> diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
> index 5b2b39c93033..e18365bbc08c 100644
> --- a/drivers/gpu/drm/ast/ast_mode.c
> +++ b/drivers/gpu/drm/ast/ast_mode.c
> @@ -514,6 +514,17 @@ static void ast_set_start_address_crt1(struct ast_private *ast,
>  
>  }
>  
> +static void ast_wait_for_vretrace(struct ast_private *ast)
> +{
> +	unsigned long timeout = jiffies + HZ;
> +	u8 vgair1;
> +
> +	do {
> +		vgair1 = ast_io_read8(ast, AST_IO_INPUT_STATUS1_READ);
> +	} while (!(vgair1 & AST_IO_VGAIR1_VREFRESH) &&
> +		 time_before(jiffies, timeout));
> +}
> +
>  /*
>   * Primary plane
>   */
> @@ -666,6 +677,14 @@ static int ast_cursor_plane_helper_atomic_check(struct drm_plane *plane,
>  	return 0;
>  }
>  
> +static bool ast_disable_cursor_during_modeset(struct drm_plane *cursor_plane)
> +{
> +	const struct drm_plane_state *cursor_state = cursor_plane->state;
> +
> +	return cursor_state && cursor_state->visible && cursor_state->crtc &&
> +	       drm_atomic_crtc_needs_modeset(cursor_state->crtc->state);
> +}
> +
>  static void
>  ast_cursor_plane_helper_atomic_update(struct drm_plane *plane,
>  				      struct drm_plane_state *old_state)
> @@ -678,7 +697,12 @@ ast_cursor_plane_helper_atomic_update(struct drm_plane *plane,
>  		ast_cursor_page_flip(ast);
>  	}
>  
> -	ast_cursor_show(ast, state->crtc_x, state->crtc_y);
> +	/*
> +	 * For modesets, delay show() until end of atomic_flush(). See the
> +	 * atomic_begin() helper for more information.
> +	 */
> +	if (!ast_disable_cursor_during_modeset(plane))
> +		ast_cursor_show(ast, state->crtc_x, state->crtc_y);
>  }
>  
>  static void
> @@ -764,6 +788,22 @@ static void ast_crtc_helper_atomic_begin(struct drm_crtc *crtc,
>  	struct ast_private *ast = to_ast_private(crtc->dev);
>  
>  	ast_open_key(ast);
> +
> +	/*
> +	 * HW cursors require the underlying primary plane and CRTC to
> +	 * display a valid mode and image. This is not the case during
> +	 * full modeset operations. So we temporarily disable any active
> +	 * HW cursor and re-enable it at the end of the atomic_flush()
> +	 * helper. The busy waiting allows the code to sync with the
> +	 * vertical refresh.
> +	 *
> +	 * We only do this during *full* modesets. It does not affect
> +	 * simple pageflips on the planes.
> +	 */
> +	if (ast_disable_cursor_during_modeset(&ast->cursor_plane)) {
> +		ast_cursor_hide(ast);
> +		ast_wait_for_vretrace(ast);
> +	}
>  }
>  
>  static void ast_crtc_helper_atomic_flush(struct drm_crtc *crtc,
> @@ -771,6 +811,7 @@ static void ast_crtc_helper_atomic_flush(struct drm_crtc *crtc,
>  {
>  	struct drm_device *dev = crtc->dev;
>  	struct ast_private *ast = to_ast_private(dev);
> +	struct drm_plane_state *cursor_state = ast->cursor_plane.state;
>  	struct ast_crtc_state *ast_state;
>  	const struct drm_format_info *format;
>  	struct ast_vbios_mode_info *vbios_mode_info;
> @@ -799,6 +840,16 @@ static void ast_crtc_helper_atomic_flush(struct drm_crtc *crtc,
>  	ast_set_dclk_reg(ast, adjusted_mode, vbios_mode_info);
>  	ast_set_crtthd_reg(ast);
>  	ast_set_sync_reg(ast, adjusted_mode, vbios_mode_info);
> +
> +	/*
> +	 * Re-enabling the HW cursor; if any. See the atomic_begin() helper
> +	 * for more information.
> +	 */
> +	if (ast_disable_cursor_during_modeset(&ast->cursor_plane)) {
> +		ast_wait_for_vretrace(ast);
> +		ast_cursor_show(ast, cursor_state->crtc_x,
> +				cursor_state->crtc_y);
> +	}
>  }
>  
>  static void
> -- 
> 2.27.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch