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

[Thomas Zimmermann <tzimmermann@xxxxxxx>]

Hi

Am 20.07.22 um 10:17 schrieb Jocelyn Falempe:
> 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.

Good point. I'll make this conditional.

Somehow planes are missing a callback that only gets executed when a 
plane gets enabled. there's atomic_disable for disabling and atomic 
update is supposed to update and enable and the plane.

Best regards
Thomas

> --
>
> 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,

--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

Attachment: OpenPGP_signature
Description: OpenPGP digital signature