Re: [PATCH/RFC 3/3] drm: atari: Add a DRM driver for Atari graphics hardware

From: Geert Uytterhoeven <geert@linux-m68k.org>
To: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
	Maxime Ripard <mripard@kernel.org>,
	David Airlie <airlied@gmail.com>, Daniel Vetter <daniel@ffwll.ch>,
	Michael Schmitz <schmitzmic@gmail.com>,
	linux-m68k@vger.kernel.org, Helge Deller <deller@gmx.de>,
	linux-fbdev@vger.kernel.org, dri-devel@lists.freedesktop.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH/RFC 3/3] drm: atari: Add a DRM driver for Atari graphics hardware
Date: Mon, 28 Nov 2022 14:08:02 +0100	[thread overview]
Message-ID: <CAMuHMdWcY1i-oFKu11Lk-urFi=E3TXinWKBL98hMhkVPzQPfZQ@mail.gmail.com> (raw)
In-Reply-To: <488b261a-843e-de94-bace-16364e6f9d92@suse.de>

Hi Thomas,

On Sat, Nov 26, 2022 at 3:51 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> that's an interesting driver. I left a few comments below.

Thanks for your comments!

> Am 25.11.22 um 21:31 schrieb Geert Uytterhoeven:
> > Supported formats:
> >    - C[1248],
> >    - RG16 (both standard DRM (little-endian) and native (big-endian)),
> >    - XR24.
> >
> > RG16 and XR24 are only supported with the underlying RGB565 hardware
> > mode on Falcon, and are subject to hardware restrictions (limited to
> > e.g. "qvga" and "hvga" modes).
> >
> > All formats use a shadow buffer (TODO: BE RG16 buffers from ST-RAM).
> > Initial mode setting works, later mode changes sometimes fail.

> > --- a/drivers/gpu/drm/tiny/Kconfig
> > +++ b/drivers/gpu/drm/tiny/Kconfig
> > @@ -10,6 +10,14 @@ config DRM_ARCPGU
> >
> >         If M is selected the module will be called arcpgu.
> >
> > +config DRM_ATARI
> > +     tristate "DRM support for Atari native chipset"
> > +     depends on DRM && ATARI
> > +     select DRM_KMS_HELPER
> > +     select DRM_GEM_SHMEM_HELPER
>
> Alphabetical sorting of the select statements.

OK

(I blame the chain of DRM_KMS_CMA_HELPER -> DRM_GEM_CMA_HELPER ->
 DRM_GEM_DMA_HELPER renames ;-)

> > --- a/drivers/video/fbdev/atafb.c
> > +++ b/drivers/gpu/drm/tiny/atari_drm.c

> > @@ -2288,11 +2279,13 @@ static struct fb_hwswitch tt_switch = {
> >   static struct fb_hwswitch falcon_switch = {
> >       .detect         = falcon_detect,
> >       .encode_fix     = falcon_encode_fix,
> > +     .config_init    = falcon_config_init,
> >       .decode_var     = falcon_decode_var,
> >       .encode_var     = falcon_encode_var,
> >       .get_par        = falcon_get_par,
> >       .set_par        = falcon_set_par,
> >       .set_screen_base = set_screen_base,
> > +     .set_col_reg    = falcon_set_col_reg,
> >       .blank          = falcon_blank,
> >       .pan_display    = falcon_pan_display,
> >   };
> > @@ -2302,11 +2295,13 @@ static struct fb_hwswitch falcon_switch = {
> >   static struct fb_hwswitch st_switch = {
> >       .detect         = stste_detect,
> >       .encode_fix     = stste_encode_fix,
> > +     .config_init    = stste_config_init,
> >       .decode_var     = stste_decode_var,
> >       .encode_var     = stste_encode_var,
> >       .get_par        = stste_get_par,
> >       .set_par        = stste_set_par,
> >       .set_screen_base = stste_set_screen_base,
> > +     .set_col_reg    = stste_set_col_reg,
> >       .pan_display    = pan_display
> >   };
> >   #endif
> > @@ -2315,10 +2310,12 @@ static struct fb_hwswitch st_switch = {
> >   static struct fb_hwswitch ext_switch = {
> >       .detect         = ext_detect,
> >       .encode_fix     = ext_encode_fix,
> > +     .config_init    = ext_config_init,
> >       .decode_var     = ext_decode_var,
> >       .encode_var     = ext_encode_var,
> >       .get_par        = ext_get_par,
> >       .set_par        = ext_set_par,
> > +     .set_col_reg    = ext_set_col_reg,
> >   };
> >   #endif
>
> This design is problematic. It recreates fbdev interfaces within DRM and
> makes it very hard to convert the driver to good DRM code. I suggest to
> branch at the outer-most point for each supported model. So each model
> effectively receives it's own mode-config pipeline. Common code can
> still be shared.
>
> For a good example, I'd refer to the latest mgag200 driver, which
> implements this pattern for the variety of revisions of its hardware.

I do not intend to keep this internal API.
It should be reworked by removing all fbdev'isms.

> > +// FIXME helpers from
> > +// "[PATCH v2 5/15] drm/fbconv: Add DRM <-> fbdev pixel-format conversion"
> > +// by Thomas Zimmermann <tzimmermann@suse.de>
>
> All these FIXMEs will need to be resolved.

Definitely.

> > +static void atari_drm_pipe_update(struct drm_simple_display_pipe *pipe,
> > +                               struct drm_plane_state *old_plane_state)
> > +{
> > +     struct atari_drm_device *atari_drm = atari_drm_from_pipe(pipe);
> > +     struct drm_plane_state *plane_state = pipe->plane.state;
> > +     struct drm_shadow_plane_state *shadow_plane_state =
> > +             to_drm_shadow_plane_state(plane_state);
> > +     struct drm_framebuffer *fb = plane_state->fb;
> > +     struct drm_crtc *crtc = &pipe->crtc;

> > +     // FIXME removing the block below triggers WARN_ON(new_crtc_state->event) in drivers/gpu/drm/drm_atomic_helper.c:2475 drm_atomic_helper_commit_hw_done
> > +     // FIXME I still see that warning when running modetest

BTW, do you know why I see that warning?
It happens less when sprinkling debug prints all over the place.

> > +     if (crtc->state->event) {
> > +             spin_lock_irq(&crtc->dev->event_lock);
> > +             drm_crtc_send_vblank_event(crtc, crtc->state->event);
> > +             crtc->state->event = NULL;
> > +             spin_unlock_irq(&crtc->dev->event_lock);
> > +     }
> > +}

> > +static struct drm_framebuffer*
> > +atari_drm_fb_create(struct drm_device *dev, struct drm_file *file_priv,
> > +                 const struct drm_mode_fb_cmd2 *mode_cmd)
> > +{
> > +     struct drm_framebuffer *fb;
> > +
> > +     switch (mode_cmd->pixel_format) {
> > +     case DRM_FORMAT_C1:
> > +     case DRM_FORMAT_C2:
> > +     case DRM_FORMAT_C4:
> > +             break;
> > +
> > +     case DRM_FORMAT_C8:
> > +             // FIXME TT & Falcon only
> > +             break;
> > +
> > +     case DRM_FORMAT_RGB565:
> > +     case DRM_FORMAT_RGB565 | DRM_FORMAT_BIG_ENDIAN:
> > +             // FIXME Falcon only
> > +             break;
> > +
> > +     case DRM_FORMAT_XRGB8888:
> > +             // FIXME
> > +             break;
> > +
> > +     default:
> > +             return ERR_PTR(-EINVAL);
> > +     }
>
> The format checks don't belong here. IIRC the format will be validated
> when you try to set the framebuffer for a plane during the atomic commit

OK

> > +
> > +     if (atari_drm_check_size(mode_cmd->width, mode_cmd->height, NULL) < 0)
> > +             return ERR_PTR(-EINVAL);
>
> Same here. The size checks are performed when the framebuffer is used.

FTR, this was based on cirrus_fb_create()...

> > +
> > +     // FIXME allocate C1 and RGB565 in ST-RAM?
> > +     fb = drm_gem_fb_create_with_dirty(dev, file_priv, mode_cmd);

BTW, how do I allocate a buffer from graphics memory?
Any example to point me to?

> > +     drm_WARN_ON_ONCE(dev, fb->pitches[0] > fb->width *
> > +                           drm_format_info_bpp(fb->format, 0) / 8);
> > +     return fb;
> > +}
> > +
> > +static const struct drm_mode_config_funcs atari_drm_mode_config_funcs = {
> > +     .fb_create = atari_drm_fb_create,
> > +     .atomic_check = drm_atomic_helper_check,        // FIXME
> > +     .atomic_commit = drm_atomic_helper_commit,      // FIXME
>
> Why FIXMEs? Thes elines appear correct.

Because I had the feeling I should pass my own functions...

>
> > +};

> > +static int __init atari_drm_probe(struct platform_device *pdev)
> > +{
> > +     struct atari_drm_device *atari_drm;
> > +     int pad, detected_mode, error;
> > +     struct drm_device *dev;
> > +     unsigned long mem_req;
> > +     char *option = NULL;
> > +     int ret;
> > +
> > +     if (fb_get_options("atafb", &option))
> > +             return -ENODEV;
>
> That function is not available here.

It is.

But now I see drm_connector_get_cmdline_mode() already calls
fb_get_options(), and handles this when calling drm_connector_init(),
albeit a bit late (it should bail out before changing any hardware
state)?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds