[Intel-gfx] [PATCH v5 4/8] drm/cma-helper: Use the generic fbdev emulation

[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 at ffwll.ch> wrote:
> > On Thu, Aug 23, 2018 at 6:14 AM, John Stultz <john.stultz at linaro.org> wrote:
> >> On Mon, Aug 20, 2018 at 11:44 PM, John Stultz <john.stultz at linaro.org> 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.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch