[PATCH v5 4/8] drm/cma-helper: Use the generic fbdev emulation
Daniel Vetter
daniel at ffwll.ch
Thu Aug 23 17:15:15 UTC 2018
On Thu, Aug 23, 2018 at 6:49 PM, Noralf Trønnes <noralf at tronnes.org> 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 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.
>
>
> 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
More information about the dri-devel
mailing list