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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux