Re: [PATCH drm-misc-next v2 1/1] drm/vmwgfx: add drm_panic support for stdu

[Ryosuke Yasuoka <ryasuoka@redhat.com>]

On Tue, Sep 09, 2025 at 03:48:01PM -0500, Ian Forbes wrote:
> On Mon, Sep 8, 2025 at 9:12 AM Ryosuke Yasuoka <ryasuoka@redhat.com> wrote:
> 
> > +static int
> > +vmw_stdu_primary_plane_get_scanout_buffer(struct drm_plane *plane,
> > +                                         struct drm_scanout_buffer *sb)
> > +{
> > +       struct drm_plane_state *state = plane->state;
> > +       struct drm_crtc *crtc = state->crtc;
> > +       struct vmw_private *dev_priv = vmw_priv(crtc->dev);
> > +
> > +       if (!plane->state || !plane->state->fb || !plane->state->visible)
> > +               return -ENODEV;
> > +
> > +       sb->format = plane->state->fb->format;
> > +       sb->width = plane->state->fb->width;
> > +       sb->height = plane->state->fb->height;
> > +       sb->pitch[0] = plane->state->fb->pitches[0];
> > +
> > +       u64 size = sb->height * sb->pitch[0];
> > +
> > +       sb->map[0].vaddr = memremap(dev_priv->vram_start, size, MEMREMAP_WT);
> > +
> > +       if (!sb->map[0].vaddr)
> > +               return -ENOMEM;
> > +
> > +       return 0;
> > +}
> > +
> > +static void vmw_stdu_primary_plane_panic_flush(struct drm_plane *plane)
> > +{
> > +       vmw_ldu_primary_plane_panic_flush(plane);
> > +}
> > +
> >  static void
> >  vmw_stdu_crtc_atomic_flush(struct drm_crtc *crtc,
> >                            struct drm_atomic_state *state)
> > @@ -1506,6 +1538,8 @@ drm_plane_helper_funcs vmw_stdu_primary_plane_helper_funcs = {
> >         .atomic_update = vmw_stdu_primary_plane_atomic_update,
> >         .prepare_fb = vmw_stdu_primary_plane_prepare_fb,
> >         .cleanup_fb = vmw_stdu_primary_plane_cleanup_fb,
> > +       .get_scanout_buffer = vmw_stdu_primary_plane_get_scanout_buffer,
> > +       .panic_flush = vmw_stdu_primary_plane_panic_flush,
> >  };
> >
> >  static const struct drm_crtc_helper_funcs vmw_stdu_crtc_helper_funcs = {
> > --
> > 2.51.0
> >
> 
> You only need the code I've highlighted in this reply with some minor changes.
> 
> 1. You can call `vmw_kms_write_svga` directly in `panic_flush`. There
> is no need to mark the buffer as dirty or send any commands.

In my test environment, it will be unstable without dirty command's
submission. In test environment, which is Virtual box, I've
observed serveral instances of the panic screen appearing.
It sometimes appears immediately as expected, and at other times,
there's a delay of about 15 to 20 seconds to appear the screen.
Another pattern, it's necessary to switch back and forth between
the Virtual Box console window and other windows.

With command submission, we can stably get a panic screen. Even if it
failed due to some reasons, we may get the panic screen ultimately. So I
think we should leave vmw_kms_ldu_panic_do_bo_dirty() to submit
commands.

What do you think?

> 2. The format should be hardcoded to RGB565 to minimize the risk of
> overrunning VRAM. Adjust the pitch accordingly to 2x width.

I see. drm panic supports multiple pitches. Checking pitch type 
in get_scanout_buffer is not difficult.

> 3. The sizes should be vmw_priv->initial_width, and
> vmw_priv->initial_height. These are the safe sizes for VRAM in early
> boot and will work in panic as well.

OK. I'll try to change this.

> 4. You want to ensure `get_scanout_buffer` only succeeds once when
> using display unit 0 since all calls to this function will return the
> same VRAM.

OK. I'll add a check in get_scanout_buffer. To confirm this, Is it
sufficient to check if lds->num_active is 1, or do I need to check
other places or values?

> 5. memremap flags should be `MEMREMAP_WB | MEMREMAP_DEC`

OK. I'll chenge the flag like this.

> 6. Move the panic related functions to the vmwgfx_kms.c files since
> they work in all DU modes.

OK. I'll try to move them in vmwgfx_kms.c

Thank you for your review and detailed comment.

Ryosuke