Hi Thomas,
On Sat, Nov 26, 2022 at 3:51 PM Thomas Zimmermann <tzimmermann@xxxxxxx> 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@xxxxxxx>
>
> 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@xxxxxxxxxxxxxx
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
