On 04/07/2020 08:23, Sam Ravnborg wrote:

I tried to take a look at this - came up with the following untested hack. The idea is that we in mode_config can specify if we need the cfb variants. (I do not know what cfb is acronym for?) Then when we setup the framebuffer we select the relevant fbops.

The oops refers to drm_fb_helper_dirty_work, so I think it is the memcpy in drm_fb_helper_dirty_blit_real() that hits us.

For now I used fb_memcpy_tofb() - but that is a macro that is expanded depending on the architecture. I think we can do btter if this works.

Sam

diff --git a/drivers/gpu/drm/bochs/bochs_kms.c b/drivers/gpu/drm/bochs/bochs_kms.c index 853081d186d5..1609ac6efbcb 100644 --- a/drivers/gpu/drm/bochs/bochs_kms.c +++ b/drivers/gpu/drm/bochs/bochs_kms.c @@ -151,6 +151,7 @@ int bochs_kms_init(struct bochs_device *bochs) bochs->dev->mode_config.preferred_depth = 24; bochs->dev->mode_config.prefer_shadow = 0; bochs->dev->mode_config.prefer_shadow_fbdev = 1;

• bochs->dev->mode_config.use_cfb_for_fbdev = true; bochs->dev->mode_config.quirk_addfb_prefer_host_byte_order = true;

  bochs->dev->mode_config.funcs = &bochs_mode_funcs;

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 170aa7689110..44e833b2f015 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -382,8 +382,13 @@ static void drm_fb_helper_dirty_blit_real(struct drm_fb_helper *fb_helper, size_t len = (clip->x2 - clip->x1) * cpp; unsigned int y;

• // TODO for (y = clip->y1; y < clip->y2; y++) {

• memcpy(dst, src, len);
  

• if (fb_helper->dev->mode_config.use_cfb_for_fbdev)
  

• 	fb_memcpy_tofb(dst, src, len);
  

• else
  

• 	memcpy(dst, src, len);
  

• src += fb->pitches[0]; dst += fb->pitches[0]; }

@@ -2017,6 +2022,21 @@ static const struct fb_ops drm_fbdev_fb_ops = { .fb_imageblit = drm_fb_helper_sys_imageblit, };

+static const struct fb_ops drm_fbdev_cfb_fb_ops = {

• .owner = THIS_MODULE,
• DRM_FB_HELPER_DEFAULT_OPS,
• .fb_open = drm_fbdev_fb_open,
• .fb_release = drm_fbdev_fb_release,
• .fb_destroy = drm_fbdev_fb_destroy,
• .fb_mmap = drm_fbdev_fb_mmap,
• .fb_read = drm_fb_helper_sys_read,
• .fb_write = drm_fb_helper_sys_write,
• .fb_fillrect = drm_fb_helper_cfb_fillrect,
• .fb_copyarea = drm_fb_helper_cfb_copyarea,
• .fb_imageblit = drm_fb_helper_cfb_imageblit,

+};

static struct fb_deferred_io drm_fbdev_defio = { .delay = HZ / 20, .deferred_io = drm_fb_helper_deferred_io, @@ -2057,7 +2077,11 @@ static int drm_fb_helper_generic_probe(struct drm_fb_helper *fb_helper, if (IS_ERR(fbi)) return PTR_ERR(fbi);

• fbi->fbops = &drm_fbdev_fb_ops;

• if (fb_helper->dev->mode_config.use_cfb_for_fbdev)

• fbi->fbops = &drm_fbdev_cfb_fb_ops;
  

• else

• fbi->fbops = &drm_fbdev_fb_ops;
  

• fbi->screen_size = fb->height * fb->pitches[0]; fbi->fix.smem_len = fbi->screen_size;

diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h index 6c3ef49b46b3..dce9adf7d189 100644 --- a/include/drm/drm_mode_config.h +++ b/include/drm/drm_mode_config.h @@ -865,6 +865,15 @@ struct drm_mode_config { */ bool prefer_shadow_fbdev;

• /**

• * @use_cfb_for_fbdev:
  

• *
  

• * Use cfb variants of drm_fb_helper_cfb_{fillrect,copyarea,imageblit}
  

• * The cfb variants are required when the CPU do not allow direct
  

• * access to the framebuffer (for example sparc64)
  

• */
  

• bool use_cfb_for_fbdev;
• /**
  • @quirk_addfb_prefer_xbgr_30bpp:

Thanks for the quick response, Sam! I tried the above diff and it stills seems to provoke a memory access error (although now the kernel hangs rather than panicking and continuing to boot).

Digging a bit more I can see the fault occurring in cfb_imageblit():

IN: cfb_imageblit 0x00000000007ad8c0: ldsb [ %i5 ], %g1 0x00000000007ad8c4: add %g3, 4, %i3 0x00000000007ad8c8: sra %g1, %g2, %g1 0x00000000007ad8cc: and %g1, %o5, %g1 0x00000000007ad8d0: srl %g1, 0, %g1 0x00000000007ad8d4: sllx %g1, 2, %g1 0x00000000007ad8d8: ld [ %o4 + %g1 ], %g1 0x00000000007ad8dc: and %i2, %g1, %g1 0x00000000007ad8e0: xor %g1, %i1, %g1 0x00000000007ad8e4: sta %g1, [ %g3 ] #ASI_PHYS_BYPASS_EC_WITH_EBIT ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

0x00000000007ad8e8: cmp %g2, 0 0x00000000007ad8ec: bne %icc, 0x7ad8b4 0x00000000007ad8f0: mov %i3, %g3

According to gdbstub the destination address in $g3 looks like this:

Breakpoint 1, 0x00000000007ad8e4 in cfb_imageblit () (gdb) i r $g3 g3 0x100220000 4297195520

The 0x100220000 address still isn't right. On sun4u the PCI address space is mapped at physical address 0x1fe00000000 and adding these two together gives 0x1ff00220000 which seems closer, but still not the correct framebuffer address 0x1ff22000000 which is reported at boot:

[ 9.007161] [drm] Found bochs VGA, ID 0xb0c5. [ 9.007840] [drm] Framebuffer size 16384 kB @ 0x1ff22000000, mmio @ 0x1ff23000000.

ATB,

Mark.

Hi Mark.

On Sat, Jul 04, 2020 at 03:16:47PM +0100, Mark Cave-Ayland wrote:

On 04/07/2020 14:41, Sam Ravnborg wrote:

...

I think what is happening is that the bochs driver request a shadow copy for the frambuffer. And with the change to fbops we now use the cfb_ functions to write to the shadow framebuffer - which is not in any IO space. So this does not work.

So maybe all we need is the fix in drm_fb_helper_dirty_blit_real(). If you try to undo the change so fbops is set to &drm_fbdev_fb_ops, but keep the fix in drm_fb_helper_dirty_blit_real() how does it then behave?

Bingo! I just tried that and the framebuffer is now working under qemu-system-sparc64 again - thank you so much for the help! From what you said I guess drm_fb_helper_dirty_blit_real() is responsible syncing the shadow copy?

Below is the current working diff based upon your previous one: it certainly feels like the difference in memcpy() behaviour should be hidden away in fb_memcpy_tofb() or similar.

From your feedback so far I thnk the minimal fix would be like this:

--- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c .. static void drm_fb_helper_dirty_blit_real(struct drm_fb_helper *fb_helper, size_t len = (clip->x2 - clip->x1) * cpp; unsigned int y;

    for (y = clip->y1; y < clip->y2; y++) {

•           memcpy(dst, src, len);
  

•           fb_memcpy_tofb(dst, src, len);
            src += fb->pitches[0];
            dst += fb->pitches[0];
    }
  

(Hand edited, patch s not a valid syntax)

But I need feedback from someone that know all this a bit better to judge if this is an OK change. For once - this will only work with shadow buffers.

Sam

diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h index 6c3ef49b46b3..dce9adf7d189 100644 --- a/include/drm/drm_mode_config.h +++ b/include/drm/drm_mode_config.h @@ -865,6 +865,15 @@ struct drm_mode_config { */ bool prefer_shadow_fbdev;

•   /**
  

•    * @use_cfb_for_fbdev:
  

•    *
  

•    * Use cfb variants of drm_fb_helper_cfb_{fillrect,copyarea,imageblit}
  

•    * The cfb variants are required when the CPU do not allow direct
  

•    * access to the framebuffer (for example sparc64)
  

•    */
  

•   bool use_cfb_for_fbdev;
  

•   /**
     * @quirk_addfb_prefer_xbgr_30bpp:
     *
  
  

...

I did not find time to follow your instructions to test this myself with qemu - sorry.

No worries, I do appreciate that as a maintainer it can be hard to fit these things around life, family, job etc.

ATB,

Mark. _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel

Yes, that's correct - I can confirm that the simplified diff below works:

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 5609e164805f..83af05fac604 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -399,7 +399,7 @@ static void drm_fb_helper_dirty_blit_real(struct drm_fb_helper *fb_helper, unsigned int y;

    for (y = clip->y1; y < clip->y2; y++) {

•           memcpy(dst, src, len);
  

•           fb_memcpy_tofb(dst, src, len);
  

fb_memcpy_tofb is #defined to sbus_memcpy_toio @ sparc which looks wrong to me given that this is a pci not a sbus device. sparc also has memcpy_toio which looks better to me.

There are blit helpers in drm_format_helper.c which already use memcpy_toio(), I guess we should do the same here. Not fully sure we can use memcpy_toio() unconditionally here. Given that a shadow framebuffer makes sense only in case the real framebuffer is not in normal ram we probably can.

take care, Gerd

Thanks Gerd - I've just tested the diff below with memcpy_toio() and that works too:

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 5609e164805f..4d05b0ab1592 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -399,7 +399,7 @@ static void drm_fb_helper_dirty_blit_real(struct drm_fb_helper *fb_helper, unsigned int y;

    for (y = clip->y1; y < clip->y2; y++) {

•           memcpy(dst, src, len);
  

•           memcpy_toio(dst, src, len);
            src += fb->pitches[0];
            dst += fb->pitches[0];
    }
  
  

Presumably there is some existing mechanism that ensures SPARC will always choose a shadow framebuffer?

bochs-drm always runs with a shadow framebuffer (that allows to swap the real framebuffer into and out of vram as needed). With other drivers this is in the hands of the driver. It might not be needed, virtio-gpu for example uses normal ram as framebuffer storage.

take care, Gerd

On 07/07/2020 20:52, Sam Ravnborg wrote:

Hi Gerd.

On Tue, Jul 07, 2020 at 09:03:41AM +0200, Gerd Hoffmann wrote:

...

...

Yes, that's correct - I can confirm that the simplified diff below works:

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 5609e164805f..83af05fac604 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -399,7 +399,7 @@ static void drm_fb_helper_dirty_blit_real(struct drm_fb_helper *fb_helper, unsigned int y;

    for (y = clip->y1; y < clip->y2; y++) {

•           memcpy(dst, src, len);
  

•           fb_memcpy_tofb(dst, src, len);
  

fb_memcpy_tofb is #defined to sbus_memcpy_toio @ sparc which looks wrong to me given that this is a pci not a sbus device. sparc also has memcpy_toio which looks better to me.

Looked at sbus_memcpy_toio and memcpy_toio for sparc64. They are essential the same. Only read bytes in little-endian format, the other read bytes in big-endian format. So thats the same.

I will prepare a proper patch with focus on fixin sparc64 only.

Thanks Sam! If you want to add me as a CC then I'm happy to test if needed.

...

There are blit helpers in drm_format_helper.c which already use memcpy_toio(), I guess we should do the same here. Not fully sure we can use memcpy_toio() unconditionally here. Given that a shadow framebuffer makes sense only in case the real framebuffer is not in normal ram we probably can.

Not so sure about this part. We unconditionally enable the use of a shadow framebuffer. But on some archs the framebuffer is not in io space - but then on these architectures memcpy_toio boils down to a simple memcpy. So maybe in the end everything is fine.

I'm afraid this part is beyond my current knowledge of the various framebuffer implementations within the kernel :/

ATB,

Mark.