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

[John Stultz]

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?

thanks
-john