On Thu, Aug 23, 2018 at 6:49 PM, Noralf Trønnes <noralf@xxxxxxxxxxx> wrote: > > Den 23.08.2018 09.37, skrev Daniel Vetter: >> >> On Wed, Aug 22, 2018 at 11:21:11PM -0700, John Stultz wrote: >>> >>> On Wed, Aug 22, 2018 at 10:51 PM, Daniel Vetter <daniel@xxxxxxxx> wrote: >>>> >>>> On Thu, Aug 23, 2018 at 6:14 AM, John Stultz <john.stultz@xxxxxxxxxx> >>>> wrote: >>>>> >>>>> On Mon, Aug 20, 2018 at 11:44 PM, John Stultz <john.stultz@xxxxxxxxxx> >>>>> wrote: >>>>>> >>>>>> Hey Noralf, all, >>>>>> I've been digging for a bit on the regression that this patch has >>>>>> tripped on the HiKey board as reported here: >>>>>> https://lkml.org/lkml/2018/8/16/81 >>>>>> >>>>>> The first issue was that the kirin driver was setting >>>>>> mode_config.max_width/height = 2048, which was causing errors as the >>>>>> the requested resolution was 1920x2160 (due to surfaceflinger >>>>>> requesting y*2 for page flipping). >>>>> >>>>> Hey Noralf, >>>>> Sorry, I know your probably sick of me. But I just wanted to circle >>>>> around on this little bit. So part of the issue I found earlier, was >>>>> that I'm running w/ CONFIG_DRM_FBDEV_OVERALLOC=200, to support >>>>> Surfaceflinger's request for page flipping. This is what makes the Y >>>>> resolution 2160, which runs afoul of the new max_height check of 2048 >>>>> in the generic code. >>>>> >>>>> I was checking with Xinliang, who know the kirin display hardware, >>>>> about the max_height being set to 2048 to ensure bumping it up wasn't >>>>> a problem, but he said 2048x2048 was unfortunately not arbitrary, and >>>>> that was the hard limit of the display hardware. However, with >>>>> overalloc, the 1920x2160 res fbdev should still be ok, as only >>>>> 1920x1080 is actually displayed at one time. >>>>> >>>>> So it seems like we might need to multiply the max_height by the >>>>> overalloc factor when we are checking it in >>>>> drm_internal_framebuffer_create? >>>>> >>>>> Does that approach sound sane, or would folks prefer something >>>>> different? >>>> >>>> I guess we could simply not check against the height limit when >>>> allocating framebuffers. But we've done that for userspace buffers >>>> since forever (they just allocate 2 buffers for page-flipping), so I >>>> have no idea what would all break if we'd suddenly lift this >>>> restriction. And whether we'd lift it for fbdev only or for everyone >>>> doesn't really make much of a difference, since either this works, or >>>> it doesn't (across all chips). >>> >>> That feels a bit more risky then what I was thinking. What about >>> something like (apologies, whitespace corrupted) >>> >>> diff --git a/drivers/gpu/drm/drm_fb_helper.c >>> b/drivers/gpu/drm/drm_fb_helper.c >>> index fe7e545..0424a71 100644 >>> --- a/drivers/gpu/drm/drm_fb_helper.c >>> +++ b/drivers/gpu/drm/drm_fb_helper.c >>> @@ -1810,6 +1810,7 @@ static int drm_fb_helper_single_fb_probe(struct >>> drm_fb_helper *fb_helper, >>> int i; >>> struct drm_fb_helper_surface_size sizes; >>> int gamma_size = 0; >>> + struct drm_mode_config *config; >>> >>> memset(&sizes, 0, sizeof(struct drm_fb_helper_surface_size)); >>> sizes.surface_depth = 24; >>> @@ -1910,6 +1911,11 @@ static int drm_fb_helper_single_fb_probe(struct >>> drm_fb_helper *fb_helper, >>> sizes.surface_height *= drm_fbdev_overalloc; >>> sizes.surface_height /= 100; >>> >>> + config = &fb_helper->client.dev->mode_config; >>> + config->max_height *= drm_fbdev_overalloc; >>> + config->max_height /= 100; >>> + >>> + >>> /* push down into drivers */ >>> ret = (*fb_helper->funcs->fb_probe)(fb_helper, &sizes); >>> if (ret < 0) >>> >>> >>> That way it only effects the fbdev + overalloc case? >> >> Still changes it for everyone, not just fbdev, if you enable overalloc. >> You'd need to reset. >> >> Another, cleaner way to fix this would be to overallocate the buffer, but >> have the drm_framebuffer limited. But that means we need to change the >> fbdev scrolling logic. And the entire interface between fbdev helpers and >> drivers needs a rework, since atm the driver allocates the drm_framebuffer >> for you. That's what userspace can/will do in this case I guess. Has all >> the issues of scrolling by moving the drm_fb without hw knowledge. >> >> I guess maybe just dropping the max_height check in fbdev is ok, if we put >> a really big comment/FIXME there. Or maybe make it conditional on >> fbdev_overalloc being at the default value, that'd probably be better >> even. > > > Maybe something like this could work: > > int drm_fb_helper_generic_probe(struct drm_fb_helper *fb_helper, > struct drm_fb_helper_surface_size *sizes) > { > DRM_DEBUG_KMS("surface width(%d), height(%d) and bpp(%d)\n", > sizes->surface_width, sizes->surface_height, > sizes->surface_bpp); > > format = drm_mode_legacy_fb_format(sizes->surface_bpp, > sizes->surface_depth); > > if (drm_fbdev_overalloc > 100 && > sizes->surface_height > > fb_helper->client.dev->mode_config.max_height) { > u32 divisor = drm_fbdev_overalloc / 100; > > buffer = drm_client_buffer_create(client, sizes->surface_width, > sizes->surface_height, format); > if (IS_ERR(buffer)) > return PTR_ERR(buffer); > > ret = drm_client_buffer_addfb(buffer, sizes->surface_width, > sizes->surface_height / divisor, format); Doesn't work, because the fbdev code checks against the framebuffer size when we "flip", i.e. when the viewport gets moved. And atm there's no support in the fbdev emulation code to re-create the drm_framebuffer for overallocated buffers. Hence why the proper solution is a pile more work than just this. -Daniel > if (ret) { > drm_client_buffer_delete(buffer); > return ret; > } > > buffer->fb->height = sizes->surface_height; > } else { > buffer = drm_client_framebuffer_create(client, sizes->surface_width, > sizes->surface_height, format); > if (IS_ERR(buffer)) > return PTR_ERR(buffer); > } > > > Noralf. > -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
