Re: [PATCH v2 08/14] drm/mgag200: Set SCROFF in primary-plane code

[Jocelyn Falempe <jfalempe@xxxxxxxxxx>]

On 18/07/2022 11:27, Thomas Zimmermann wrote:
> The SCROFF bit controls reading the primary plane's scanout buffer
> from video memory. Set it from primary-plane code, instead of CRTC
> code.

I'm a bit concerned about the performance impact of this patch.

Previously, the SCROFF bit and msleep(20) was done only when 
enabling/disabling display.
With this patch, it will be done for each frame, which will negatively 
impact performances.

--

Jocelyn
> Signed-off-by: Thomas Zimmermann <tzimmermann@xxxxxxx>
> Reviewed-by: Jocelyn Falempe <jfalempe@xxxxxxxxxx>
> Tested-by: Jocelyn Falempe <jfalempe@xxxxxxxxxx>
> Acked-by: Sam Ravnborg <sam@xxxxxxxxxxxx>
> ---
>   drivers/gpu/drm/mgag200/mgag200_mode.c | 33 ++++++++++++++------------
>   1 file changed, 18 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
> index 39509dd84b23..789e02b8615f 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_mode.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
> @@ -550,7 +550,7 @@ static void mgag200_g200ev_set_hiprilvl(struct mga_device *mdev)
>   
>   static void mgag200_enable_display(struct mga_device *mdev)
>   {
> -	u8 seq0, seq1, crtcext1;
> +	u8 seq0, crtcext1;
>   
>   	RREG_SEQ(0x00, seq0);
>   	seq0 |= MGAREG_SEQ0_SYNCRST |
> @@ -564,12 +564,6 @@ static void mgag200_enable_display(struct mga_device *mdev)
>   	mga_wait_vsync(mdev);
>   	mga_wait_busy(mdev);
>   
> -	RREG_SEQ(0x01, seq1);
> -	seq1 &= ~MGAREG_SEQ1_SCROFF;
> -	WREG_SEQ(0x01, seq1);
> -
> -	msleep(20);
> -
>   	RREG_ECRT(0x01, crtcext1);
>   	crtcext1 &= ~MGAREG_CRTCEXT1_VSYNCOFF;
>   	crtcext1 &= ~MGAREG_CRTCEXT1_HSYNCOFF;
> @@ -578,7 +572,7 @@ static void mgag200_enable_display(struct mga_device *mdev)
>   
>   static void mgag200_disable_display(struct mga_device *mdev)
>   {
> -	u8 seq0, seq1, crtcext1;
> +	u8 seq0, crtcext1;
>   
>   	RREG_SEQ(0x00, seq0);
>   	seq0 &= ~MGAREG_SEQ0_SYNCRST;
> @@ -591,12 +585,6 @@ static void mgag200_disable_display(struct mga_device *mdev)
>   	mga_wait_vsync(mdev);
>   	mga_wait_busy(mdev);
>   
> -	RREG_SEQ(0x01, seq1);
> -	seq1 |= MGAREG_SEQ1_SCROFF;
> -	WREG_SEQ(0x01, seq1);
> -
> -	msleep(20);
> -
>   	RREG_ECRT(0x01, crtcext1);
>   	crtcext1 |= MGAREG_CRTCEXT1_VSYNCOFF |
>   		    MGAREG_CRTCEXT1_HSYNCOFF;
> @@ -676,6 +664,7 @@ static void mgag200_primary_plane_helper_atomic_update(struct drm_plane *plane,
>   	struct drm_framebuffer *fb = plane_state->fb;
>   	struct drm_atomic_helper_damage_iter iter;
>   	struct drm_rect damage;
> +	u8 seq1;
>   
>   	if (!fb)
>   		return;
> @@ -688,11 +677,25 @@ static void mgag200_primary_plane_helper_atomic_update(struct drm_plane *plane,
>   	/* Always scanout image at VRAM offset 0 */
>   	mgag200_set_startadd(mdev, (u32)0);
>   	mgag200_set_offset(mdev, fb);
> +
> +	RREG_SEQ(0x01, seq1);
> +	seq1 &= ~MGAREG_SEQ1_SCROFF;
> +	WREG_SEQ(0x01, seq1);
> +	msleep(20);
>   }
>   
>   static void mgag200_primary_plane_helper_atomic_disable(struct drm_plane *plane,
>   							struct drm_atomic_state *old_state)
> -{ }
> +{
> +	struct drm_device *dev = plane->dev;
> +	struct mga_device *mdev = to_mga_device(dev);
> +	u8 seq1;
> +
> +	RREG_SEQ(0x01, seq1);
> +	seq1 |= MGAREG_SEQ1_SCROFF;
> +	WREG_SEQ(0x01, seq1);
> +	msleep(20);
> +}
>   
>   static const struct drm_plane_helper_funcs mgag200_primary_plane_helper_funcs = {
>   	DRM_GEM_SHADOW_PLANE_HELPER_FUNCS,