Re: [PATCH 2/4] drm/mgag200: Start/stop BMC scanout in BMC encoder helpers

[Thomas Zimmermann <tzimmermann@xxxxxxx>]

Am 04.07.24 um 14:23 schrieb Thomas Zimmermann:
> Hi
>
> Am 04.07.24 um 14:13 schrieb Jocelyn Falempe:
> > On 03/07/2024 15:40, Thomas Zimmermann wrote:
> > > Start and stop the BMC scanout from the BMC encoder's atomic_enable
> > > and atomic_disable helpers. The BMC stops scanning out at the beginning
> > > of a modeset operation and restarts the scanout at the end of the
> > > modeset.
> > >
> > > Only G200EW3 and G200WB require this procedure. Drop the original
> > > vidrst callbacks for these model's support, as they are now obsolete.
> >
> > Thanks, that makes the BMC connector more useful.
> >
> > I'm just wondering, in case you use both a VGA output and the BMC, 
> > before this patch, vidrst would be enabled, and after, as the BMC 
> > connector is "unconnected" it should be disabled ?
> >
> > Will that have a visible impact for users ?
>
> Oh, I didn't think of this case. That could likely be a problem. 
> Thanks for pointing this out.
>
> Let's drop patches 2 to 4 for now. I'd still like to take patch 1, as 
> it works on it own and makes sense.

Or maybe I'll send out something different. I'd like the BMC and CRTC to 
be more independent from each other. But apparently the CRTC really has 
to do all the work here.

Best regards
Thomas


> Best regards
> Thomas
>
> > > Signed-off-by: Thomas Zimmermann <tzimmermann@xxxxxxx>
> > > ---
> > >   drivers/gpu/drm/mgag200/mgag200_bmc.c     | 24 
> > > +++++++++++++++++++++--
> > >   drivers/gpu/drm/mgag200/mgag200_drv.h     |  4 +---
> > >   drivers/gpu/drm/mgag200/mgag200_g200ew3.c |  2 --
> > >   drivers/gpu/drm/mgag200/mgag200_g200wb.c  |  2 --
> > >   4 files changed, 23 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/mgag200/mgag200_bmc.c 
> > > b/drivers/gpu/drm/mgag200/mgag200_bmc.c
> > > index cb5400333862..74feb6d8db45 100644
> > > --- a/drivers/gpu/drm/mgag200/mgag200_bmc.c
> > > +++ b/drivers/gpu/drm/mgag200/mgag200_bmc.c
> > > @@ -14,7 +14,7 @@ static struct mgag200_bmc_connector 
> > > *to_mgag200_bmc_connector(struct drm_connect
> > >       return container_of(connector, struct mgag200_bmc_connector, 
> > > base);
> > >   }
> > >   -void mgag200_bmc_disable_vidrst(struct mga_device *mdev)
> > > +static void mgag200_bmc_disable_vidrst(struct mga_device *mdev)
> > >   {
> > >       u8 tmp;
> > >       int iter_max;
> > > @@ -73,7 +73,16 @@ void mgag200_bmc_disable_vidrst(struct mga_device 
> > > *mdev)
> > >       }
> > >   }
> > >   -void mgag200_bmc_enable_vidrst(struct mga_device *mdev)
> > > +static void mgag200_bmc_encoder_helper_atomic_disable(struct 
> > > drm_encoder *encoder,
> > > +                              struct drm_atomic_state *state)
> > > +{
> > > +    struct mga_device *mdev = to_mga_device(encoder->dev);
> > > +
> > > +    if (mdev->info->has_vidrst)
> > > +        mgag200_bmc_disable_vidrst(mdev);
> > > +}
> > > +
> > > +static void mgag200_bmc_enable_vidrst(struct mga_device *mdev)
> > >   {
> > >       u8 tmp;
> > >   @@ -103,6 +112,15 @@ void mgag200_bmc_enable_vidrst(struct 
> > > mga_device *mdev)
> > >       WREG_DAC(MGA1064_GEN_IO_DATA, tmp);
> > >   }
> > >   +static void mgag200_bmc_encoder_helper_atomic_enable(struct 
> > > drm_encoder *encoder,
> > > +                             struct drm_atomic_state *state)
> > > +{
> > > +    struct mga_device *mdev = to_mga_device(encoder->dev);
> > > +
> > > +    if (mdev->info->has_vidrst)
> > > +        mgag200_bmc_enable_vidrst(mdev);
> > > +}
> > > +
> > >   static int mgag200_bmc_encoder_helper_atomic_check(struct 
> > > drm_encoder *encoder,
> > >                              struct drm_crtc_state *crtc_state,
> > >                              struct drm_connector_state *conn_state)
> > > @@ -119,6 +137,8 @@ static int 
> > > mgag200_bmc_encoder_helper_atomic_check(struct drm_encoder *encoder,
> > >   }
> > >     static const struct drm_encoder_helper_funcs 
> > > mgag200_bmc_encoder_helper_funcs = {
> > > +    .atomic_disable = mgag200_bmc_encoder_helper_atomic_disable,
> > > +    .atomic_enable = mgag200_bmc_encoder_helper_atomic_enable,
> > >       .atomic_check = mgag200_bmc_encoder_helper_atomic_check,
> > >   };
> > >   diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h 
> > > b/drivers/gpu/drm/mgag200/mgag200_drv.h
> > > index 4b75613de882..b1365795234b 100644
> > > --- a/drivers/gpu/drm/mgag200/mgag200_drv.h
> > > +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
> > > @@ -442,9 +442,7 @@ int mgag200_mode_config_init(struct mga_device 
> > > *mdev, resource_size_t vram_avail
> > >   /* mgag200_vga.c */
> > >   int mgag200_vga_output_init(struct mga_device *mdev);
> > >   -                /* mgag200_bmc.c */
> > > -void mgag200_bmc_disable_vidrst(struct mga_device *mdev);
> > > -void mgag200_bmc_enable_vidrst(struct mga_device *mdev);
> > > +/* mgag200_bmc.c */
> > >   int mgag200_bmc_output_init(struct mga_device *mdev, struct 
> > > drm_connector *physical_connector);
> > >     #endif                /* __MGAG200_DRV_H__ */
> > > diff --git a/drivers/gpu/drm/mgag200/mgag200_g200ew3.c 
> > > b/drivers/gpu/drm/mgag200/mgag200_g200ew3.c
> > > index 839401e8b465..265f3e95830a 100644
> > > --- a/drivers/gpu/drm/mgag200/mgag200_g200ew3.c
> > > +++ b/drivers/gpu/drm/mgag200/mgag200_g200ew3.c
> > > @@ -146,8 +146,6 @@ static const struct mgag200_device_info 
> > > mgag200_g200ew3_device_info =
> > >       MGAG200_DEVICE_INFO_INIT(2048, 2048, 0, true, 0, 1, false);
> > >     static const struct mgag200_device_funcs 
> > > mgag200_g200ew3_device_funcs = {
> > > -    .disable_vidrst = mgag200_bmc_disable_vidrst,
> > > -    .enable_vidrst = mgag200_bmc_enable_vidrst,
> > >       .pixpllc_atomic_check = mgag200_g200ew3_pixpllc_atomic_check,
> > >       .pixpllc_atomic_update = mgag200_g200wb_pixpllc_atomic_update, 
> > > // same as G200WB
> > >   };
> > > diff --git a/drivers/gpu/drm/mgag200/mgag200_g200wb.c 
> > > b/drivers/gpu/drm/mgag200/mgag200_g200wb.c
> > > index 835df0f4fc13..e25477347c3e 100644
> > > --- a/drivers/gpu/drm/mgag200/mgag200_g200wb.c
> > > +++ b/drivers/gpu/drm/mgag200/mgag200_g200wb.c
> > > @@ -280,8 +280,6 @@ static const struct mgag200_device_info 
> > > mgag200_g200wb_device_info =
> > >       MGAG200_DEVICE_INFO_INIT(1280, 1024, 31877, true, 0, 1, false);
> > >     static const struct mgag200_device_funcs 
> > > mgag200_g200wb_device_funcs = {
> > > -    .disable_vidrst = mgag200_bmc_disable_vidrst,
> > > -    .enable_vidrst = mgag200_bmc_enable_vidrst,
> > >       .pixpllc_atomic_check = mgag200_g200wb_pixpllc_atomic_check,
> > >       .pixpllc_atomic_update = mgag200_g200wb_pixpllc_atomic_update,
> > >   };

--
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)