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

[Noralf Trønnes]

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);
         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.