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

2018-08-24 Thread Laurent Pinchart
Hi John,

On Friday, 24 August 2018 00:12:46 EEST John Stultz wrote:
> On Thu, Aug 23, 2018 at 1:49 PM, Laurent Pinchart wrote:
> > On Thursday, 23 August 2018 20:48:40 EEST John Stultz wrote:
> >> On Thu, Aug 23, 2018 at 1:09 AM, Daniel Vetter  wrote:
> >>> On Thu, Aug 23, 2018 at 10:46:15AM +0300, Laurent Pinchart wrote:
>  Possibly slightly out of topic, but we're in 2018, is there any plan
>  to make SurfaceFlinger move away from FBDEV ?
> >>> 
> >>> Is surfaceflinger really using direct fbdev still (maybe for boot-up)?
> >>> Or is this just an artifact of the mali blob hwcomposer backend?
> >> 
> >> Mostly its due to the simple fbdev being a legacy solution on android
> >> that works out of the box.
> >> I do suspect the android devs hope to retire it, which is why I'm
> >> working on getting things going w/ the drm_hwcomposer right now so we
> >> can get away from the fbdev.
> > 
> > That would be good news. Are there many Android components other than
> > vendor- specific hwcomposer implementations that still use fbdev ?
> 
> So yea, I can't really speak about what the various vendors are doing,
> as I don't really know, but I'm aware there are still a few (in some
> cases major) vendors who still use fbdev on their shipping devices
> with their custom hwcomposer code.
> 
> Other then that, to my knowledge AOSP only has a default fallback
> hwcomposer that uses fbdev, which is what we've used here as we didn't
> want to take the vendor's proprietary hwcomposer blob.  But again,
> moving to the drm_hwcomposer is the shiny bright future, as soon as a
> few remaining issues are sorted upstream.

Last time I looked (and that was years ago) the init process also used fbdev 
to render the boot splash screen. Is it still the case ? If so is there any 
chance you could add a fix for that to your todo list ? :-)

-- 
Regards,

Laurent Pinchart



___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel


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

2018-08-23 Thread John Stultz
On Thu, Aug 23, 2018 at 1:49 PM, Laurent Pinchart
 wrote:
> On Thursday, 23 August 2018 20:48:40 EEST John Stultz wrote:
>> On Thu, Aug 23, 2018 at 1:09 AM, Daniel Vetter  wrote:
>> > On Thu, Aug 23, 2018 at 10:46:15AM +0300, Laurent Pinchart wrote:
>> >> Possibly slightly out of topic, but we're in 2018, is there any plan to
>> >> make SurfaceFlinger move away from FBDEV ?
>> >
>> > Is surfaceflinger really using direct fbdev still (maybe for boot-up)? Or
>> > is this just an artifact of the mali blob hwcomposer backend?
>>
>> Mostly its due to the simple fbdev being a legacy solution on android
>> that works out of the box.
>> I do suspect the android devs hope to retire it, which is why I'm
>> working on getting things going w/ the drm_hwcomposer right now so we
>> can get away from the fbdev.
>
> That would be good news. Are there many Android components other than vendor-
> specific hwcomposer implementations that still use fbdev ?

So yea, I can't really speak about what the various vendors are doing,
as I don't really know, but I'm aware there are still a few (in some
cases major) vendors who still use fbdev on their shipping devices
with their custom hwcomposer code.

Other then that, to my knowledge AOSP only has a default fallback
hwcomposer that uses fbdev, which is what we've used here as we didn't
want to take the vendor's proprietary hwcomposer blob.  But again,
moving to the drm_hwcomposer is the shiny bright future, as soon as a
few remaining issues are sorted upstream.

thanks
-john
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel


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

2018-08-23 Thread Laurent Pinchart
Hi John,

On Thursday, 23 August 2018 20:48:40 EEST John Stultz wrote:
> On Thu, Aug 23, 2018 at 1:09 AM, Daniel Vetter  wrote:
> > On Thu, Aug 23, 2018 at 10:46:15AM +0300, Laurent Pinchart wrote:
> >> On Thursday, 23 August 2018 07:14:08 EEST John Stultz wrote:
> >>> On Mon, Aug 20, 2018 at 11:44 PM, John Stultz 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.
> >> 
> >> Possibly slightly out of topic, but we're in 2018, is there any plan to
> >> make SurfaceFlinger move away from FBDEV ?
> > 
> > Is surfaceflinger really using direct fbdev still (maybe for boot-up)? Or
> > is this just an artifact of the mali blob hwcomposer backend?
> 
> Mostly its due to the simple fbdev being a legacy solution on android
> that works out of the box.
> I do suspect the android devs hope to retire it, which is why I'm
> working on getting things going w/ the drm_hwcomposer right now so we
> can get away from the fbdev.

That would be good news. Are there many Android components other than vendor-
specific hwcomposer implementations that still use fbdev ?

> But in the meantime, keeping the fbdev method running is important so we
> have a functioning device for testing AOSP w/ mainline.

-- 
Regards,

Laurent Pinchart



___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel


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

2018-08-23 Thread John Stultz
On Thu, Aug 23, 2018 at 1:09 AM, Daniel Vetter  wrote:
> On Thu, Aug 23, 2018 at 10:46:15AM +0300, Laurent Pinchart wrote:
>> Hi John,
>>
>> On Thursday, 23 August 2018 07:14:08 EEST John Stultz wrote:
>> > On Mon, Aug 20, 2018 at 11:44 PM, John Stultz 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.
>>
>> Possibly slightly out of topic, but we're in 2018, is there any plan to make
>> SurfaceFlinger move away from FBDEV ?
>
> Is surfaceflinger really using direct fbdev still (maybe for boot-up)? Or
> is this just an artifact of the mali blob hwcomposer backend?

Mostly its due to the simple fbdev being a legacy solution on android
that works out of the box.
I do suspect the android devs hope to retire it, which is why I'm
working on getting things going w/ the drm_hwcomposer right now so we
can get away from the fbdev. But in the meantime, keeping the fbdev
method running is important so we have a functioning device for
testing AOSP w/ mainline.

thanks
-john
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel


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

2018-08-23 Thread John Stultz
On Thu, Aug 23, 2018 at 12:46 AM, Laurent Pinchart
 wrote:
> Hi John,
>
> On Thursday, 23 August 2018 07:14:08 EEST John Stultz wrote:
>> On Mon, Aug 20, 2018 at 11:44 PM, John Stultz 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.
>
> Possibly slightly out of topic, but we're in 2018, is there any plan to make
> SurfaceFlinger move away from FBDEV ?

Yes. That is actually work-in-progress for both HiKey and HiKey960,
switching to use the drm_hwcomposer once we can get upstream project
into a sane state.

thanks
-john
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel


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

2018-08-23 Thread Daniel Vetter
On Thu, Aug 23, 2018 at 6:49 PM, Noralf Trønnes  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  wrote:

 On Thu, Aug 23, 2018 at 6:14 AM, John Stultz 
 wrote:
>
> On Mon, Aug 20, 2018 at 11:44 PM, John Stultz 
> 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(, 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 = _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, );
>>>  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 >
> 

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

2018-08-23 Thread 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  wrote:

On Thu, Aug 23, 2018 at 6:14 AM, John Stultz  wrote:

On Mon, Aug 20, 2018 at 11:44 PM, John Stultz  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(, 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 = _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, );
 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 = 

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

2018-08-23 Thread Daniel Vetter
On Thu, Aug 23, 2018 at 10:46:15AM +0300, Laurent Pinchart wrote:
> Hi John,
> 
> On Thursday, 23 August 2018 07:14:08 EEST John Stultz wrote:
> > On Mon, Aug 20, 2018 at 11:44 PM, John Stultz 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.
> 
> Possibly slightly out of topic, but we're in 2018, is there any plan to make 
> SurfaceFlinger move away from FBDEV ?

Is surfaceflinger really using direct fbdev still (maybe for boot-up)? Or
is this just an artifact of the mali blob hwcomposer backend?
-Daniel

> 
> > 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?
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> 
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel


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

2018-08-23 Thread Laurent Pinchart
Hi John,

On Thursday, 23 August 2018 07:14:08 EEST John Stultz wrote:
> On Mon, Aug 20, 2018 at 11:44 PM, John Stultz 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.

Possibly slightly out of topic, but we're in 2018, is there any plan to make 
SurfaceFlinger move away from FBDEV ?

> 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?

-- 
Regards,

Laurent Pinchart



___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel


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

2018-08-23 Thread 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  wrote:
> > On Thu, Aug 23, 2018 at 6:14 AM, John Stultz  wrote:
> >> On Mon, Aug 20, 2018 at 11:44 PM, John Stultz  
> >> 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(, 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 = _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, );
> 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
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel


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

2018-08-23 Thread John Stultz
On Wed, Aug 22, 2018 at 10:51 PM, Daniel Vetter  wrote:
> On Thu, Aug 23, 2018 at 6:14 AM, John Stultz  wrote:
>> On Mon, Aug 20, 2018 at 11:44 PM, John Stultz  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(, 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 = _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, );
if (ret < 0)


That way it only effects the fbdev + overalloc case?

thanks
-john
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel


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

2018-08-22 Thread Daniel Vetter
On Thu, Aug 23, 2018 at 6:14 AM, John Stultz  wrote:
> On Mon, Aug 20, 2018 at 11:44 PM, John Stultz  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).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel


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

2018-08-22 Thread John Stultz
On Mon, Aug 20, 2018 at 11:44 PM, John Stultz  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?

thanks
-john
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel


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

2018-08-22 Thread Daniel Vetter
On Tue, Aug 21, 2018 at 8:43 PM, John Stultz  wrote:
> On Tue, Aug 21, 2018 at 7:59 AM, Noralf Trønnes  wrote:
>> Den 21.08.2018 10.44, skrev Daniel Vetter:
>>> On Mon, Aug 20, 2018 at 11:44:56PM -0700, John Stultz wrote:

 Since we don't have a drm_gem_cma_object reference in
 drm_fb_helper_generic_probe(), I instead added:
 fbi->fix.smem_start = page_to_phys(virt_to_page(fbi->screen_buffer));

 And that got things working!

 So yea, I wanted to figure out if we are just missing the line above
 in drm_fb_helper_generic_probe()? Or if the kirin driver should be
 setting the fix.smem_start in some other fashion?
>>>
>>> With the generic helpers we can't use the generic fb_mmap() implementation
>>> in fbdev/core/fbmem.c anymore. Instead Noralf added a generic fb_mmap
>>> fb_ops callback in drm_fbdev_fb_mmap, which you're supposed to use. Can
>>> you pls double-check that this is wired up correctly for kirin?
>>>
>>> At least I don't see any other place where we use smem_start in the fbdev
>>> code. It does get copied to userspace, but userspace should never use
>>> that.
>>
>>
>> I googled 'FBIOGET_FSCREENINFO smem_start' and came across info[1]
>> about the Mali blob using it and HiKey seems to have a Mali GPU:
>>
>> 00:17  BorgCuba:
>> the mali blob is supposed to open the framebuffer device,
>> get "smem_start" and try to map this memory via MALI_IOC_MEM_MAP_EXT ioctl
>>
>> Could this be the problem here?
>
> It does look like that's the case. Looking for smem_start usage, for
> Android its the gralloc code that fetches it:
>
> https://android.googlesource.com/device/linaro/hikey/+/master/gralloc/framebuffer_device.cpp#353
> and then puts it into the private_handle_t:
>
> https://android.googlesource.com/device/linaro/hikey/+/master/gralloc/framebuffer_device.cpp#384
>
> Which I assume then gets used later in the opaque mali blob (which
> then results in a similarly opaque hang).
>
> While I'm perfectly understanding that folks are not interested as its
> related to mali (which is fine, I can carry a patch - working to move
> away from fbdev emulation anyway), I am wondering if it might cause
> trouble for other users, as smem_start was previously set and now its
> not, so it seems likely to break userspace examples like:
>https://www.linuxtv.org/downloads/v4l-dvb-apis-old/osd.html
>
> Or are those such old legacy they don't really count?

We've been pretty strict in drm about never ever exposing any physical
offsets to userspace. I'm mildly tempted to plug this even more if
possible, to stop more abuse. If you want mmap, fbdev has that. If you
want buffer sharing, use drm. Don't do buffer sharing by passing
physical addresses around in userspace, that idea is dead since about
8 years (back when we've done the initial dma-buf discussions).

So yeah, not going to care one bit here :-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel


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

2018-08-21 Thread John Stultz
On Tue, Aug 21, 2018 at 7:59 AM, Noralf Trønnes  wrote:
> Den 21.08.2018 10.44, skrev Daniel Vetter:
>> On Mon, Aug 20, 2018 at 11:44:56PM -0700, John Stultz wrote:
>>>
>>> Since we don't have a drm_gem_cma_object reference in
>>> drm_fb_helper_generic_probe(), I instead added:
>>> fbi->fix.smem_start = page_to_phys(virt_to_page(fbi->screen_buffer));
>>>
>>> And that got things working!
>>>
>>> So yea, I wanted to figure out if we are just missing the line above
>>> in drm_fb_helper_generic_probe()? Or if the kirin driver should be
>>> setting the fix.smem_start in some other fashion?
>>
>> With the generic helpers we can't use the generic fb_mmap() implementation
>> in fbdev/core/fbmem.c anymore. Instead Noralf added a generic fb_mmap
>> fb_ops callback in drm_fbdev_fb_mmap, which you're supposed to use. Can
>> you pls double-check that this is wired up correctly for kirin?
>>
>> At least I don't see any other place where we use smem_start in the fbdev
>> code. It does get copied to userspace, but userspace should never use
>> that.
>
>
> I googled 'FBIOGET_FSCREENINFO smem_start' and came across info[1]
> about the Mali blob using it and HiKey seems to have a Mali GPU:
>
> 00:17  BorgCuba:
> the mali blob is supposed to open the framebuffer device,
> get "smem_start" and try to map this memory via MALI_IOC_MEM_MAP_EXT ioctl
>
> Could this be the problem here?

It does look like that's the case. Looking for smem_start usage, for
Android its the gralloc code that fetches it:
   
https://android.googlesource.com/device/linaro/hikey/+/master/gralloc/framebuffer_device.cpp#353
and then puts it into the private_handle_t:
   
https://android.googlesource.com/device/linaro/hikey/+/master/gralloc/framebuffer_device.cpp#384

Which I assume then gets used later in the opaque mali blob (which
then results in a similarly opaque hang).

While I'm perfectly understanding that folks are not interested as its
related to mali (which is fine, I can carry a patch - working to move
away from fbdev emulation anyway), I am wondering if it might cause
trouble for other users, as smem_start was previously set and now its
not, so it seems likely to break userspace examples like:
   https://www.linuxtv.org/downloads/v4l-dvb-apis-old/osd.html

Or are those such old legacy they don't really count?

thanks
-john
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel


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

2018-08-21 Thread Daniel Vetter
On Tue, Aug 21, 2018 at 6:03 PM, Noralf Trønnes  wrote:
>
> Den 21.08.2018 17.41, skrev Daniel Vetter:
>>
>> On Tue, Aug 21, 2018 at 04:59:46PM +0200, Noralf Trønnes wrote:
>>>
>>> Den 21.08.2018 10.44, skrev Daniel Vetter:

 On Mon, Aug 20, 2018 at 11:44:56PM -0700, John Stultz wrote:
>
> On Tue, Jul 3, 2018 at 9:03 AM, Noralf Trønnes 
> wrote:
>>
>> This switches the CMA helper drivers that use its fbdev emulation over
>> to the generic fbdev emulation. It's the first phase of using generic
>> fbdev. A later phase will use DRM client callbacks for the
>> lastclose/hotplug/remove callbacks.
>>
>> There are currently 2 fbdev init/fini functions:
>> - drm_fb_cma_fbdev_init/drm_fb_cma_fbdev_fini
>> - drm_fbdev_cma_init/drm_fbdev_cma_fini
>>
>> This is because the work on generic fbdev came up during a fbdev
>> refactoring and thus wasn't completed. No point in completing that
>> refactoring when drivers will soon move to
>> drm_fb_helper_generic_probe().
>>
>> tinydrm uses drm_fb_cma_fbdev_init_with_funcs().
>>
>> Cc: Laurent Pinchart 
>> Signed-off-by: Noralf Trønnes 
>> Acked-by: Daniel Vetter 
>> ---
>>drivers/gpu/drm/drm_fb_cma_helper.c | 360
>> +---
>>include/drm/drm_fb_cma_helper.h |   3 -
>>2 files changed, 42 insertions(+), 321 deletions(-)
>
> ...
>>
>> -static int drm_fbdev_cma_defio_init(struct fb_info *fbi,
>> -   struct drm_gem_cma_object
>> *cma_obj)
>> -{
>> -   struct fb_deferred_io *fbdefio;
>> -   struct fb_ops *fbops;
>> -
>> -   /*
>> -* Per device structures are needed because:
>> -* fbops: fb_deferred_io_cleanup() clears fbops.fb_mmap
>> -* fbdefio: individual delays
>> -*/
>> -   fbdefio = kzalloc(sizeof(*fbdefio), GFP_KERNEL);
>> -   fbops = kzalloc(sizeof(*fbops), GFP_KERNEL);
>> -   if (!fbdefio || !fbops) {
>> -   kfree(fbdefio);
>> -   kfree(fbops);
>> -   return -ENOMEM;
>> -   }
>> -
>> -   /* can't be offset from vaddr since dirty() uses cma_obj */
>> -   fbi->screen_buffer = cma_obj->vaddr;
>> -   /* fb_deferred_io_fault() needs a physical address */
>> -   fbi->fix.smem_start =
>> page_to_phys(virt_to_page(fbi->screen_buffer));
>> -
>> -   *fbops = *fbi->fbops;
>> -   fbi->fbops = fbops;
>> -
>> -   fbdefio->delay = msecs_to_jiffies(DEFAULT_FBDEFIO_DELAY_MS);
>> -   fbdefio->deferred_io = drm_fb_helper_deferred_io;
>> -   fbi->fbdefio = fbdefio;
>> -   fb_deferred_io_init(fbi);
>> -   fbi->fbops->fb_mmap = drm_fbdev_cma_deferred_io_mmap;
>> -
>> -   return 0;
>> -}
>> -
>> -static void drm_fbdev_cma_defio_fini(struct fb_info *fbi)
>> -{
>> -   if (!fbi->fbdefio)
>> -   return;
>> -
>> -   fb_deferred_io_cleanup(fbi);
>> -   kfree(fbi->fbdefio);
>> -   kfree(fbi->fbops);
>> -}
>> -
>> -static int
>> -drm_fbdev_cma_create(struct drm_fb_helper *helper,
>> -   struct drm_fb_helper_surface_size *sizes)
>> -{
>> -   struct drm_fbdev_cma *fbdev_cma = to_fbdev_cma(helper);
>> -   struct drm_device *dev = helper->dev;
>> -   struct drm_gem_cma_object *obj;
>> -   struct drm_framebuffer *fb;
>> -   unsigned int bytes_per_pixel;
>> -   unsigned long offset;
>> -   struct fb_info *fbi;
>> -   size_t size;
>> -   int ret;
>> -
>> -   DRM_DEBUG_KMS("surface width(%d), height(%d) and bpp(%d)\n",
>> -   sizes->surface_width, sizes->surface_height,
>> -   sizes->surface_bpp);
>> -
>> -   bytes_per_pixel = DIV_ROUND_UP(sizes->surface_bpp, 8);
>> -   size = sizes->surface_width * sizes->surface_height *
>> bytes_per_pixel;
>> -   obj = drm_gem_cma_create(dev, size);
>> -   if (IS_ERR(obj))
>> -   return -ENOMEM;
>> -
>> -   fbi = drm_fb_helper_alloc_fbi(helper);
>> -   if (IS_ERR(fbi)) {
>> -   ret = PTR_ERR(fbi);
>> -   goto err_gem_free_object;
>> -   }
>> -
>> -   fb = drm_gem_fbdev_fb_create(dev, sizes, 0, >base,
>> -fbdev_cma->fb_funcs);
>> -   if (IS_ERR(fb)) {
>> -   dev_err(dev->dev, "Failed to allocate DRM
>> framebuffer.\n");
>> -   ret = PTR_ERR(fb);
>> -   goto err_fb_info_destroy;
>> -   }
>> -
>> -   helper->fb = fb;
>> -
>> -   fbi->par = helper;
>> -   fbi->flags = FBINFO_FLAG_DEFAULT;
>> -   

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

2018-08-21 Thread Noralf Trønnes


Den 21.08.2018 17.41, skrev Daniel Vetter:

On Tue, Aug 21, 2018 at 04:59:46PM +0200, Noralf Trønnes wrote:

Den 21.08.2018 10.44, skrev Daniel Vetter:

On Mon, Aug 20, 2018 at 11:44:56PM -0700, John Stultz wrote:

On Tue, Jul 3, 2018 at 9:03 AM, Noralf Trønnes  wrote:

This switches the CMA helper drivers that use its fbdev emulation over
to the generic fbdev emulation. It's the first phase of using generic
fbdev. A later phase will use DRM client callbacks for the
lastclose/hotplug/remove callbacks.

There are currently 2 fbdev init/fini functions:
- drm_fb_cma_fbdev_init/drm_fb_cma_fbdev_fini
- drm_fbdev_cma_init/drm_fbdev_cma_fini

This is because the work on generic fbdev came up during a fbdev
refactoring and thus wasn't completed. No point in completing that
refactoring when drivers will soon move to drm_fb_helper_generic_probe().

tinydrm uses drm_fb_cma_fbdev_init_with_funcs().

Cc: Laurent Pinchart 
Signed-off-by: Noralf Trønnes 
Acked-by: Daniel Vetter 
---
   drivers/gpu/drm/drm_fb_cma_helper.c | 360 
+---
   include/drm/drm_fb_cma_helper.h |   3 -
   2 files changed, 42 insertions(+), 321 deletions(-)

...

-static int drm_fbdev_cma_defio_init(struct fb_info *fbi,
-   struct drm_gem_cma_object *cma_obj)
-{
-   struct fb_deferred_io *fbdefio;
-   struct fb_ops *fbops;
-
-   /*
-* Per device structures are needed because:
-* fbops: fb_deferred_io_cleanup() clears fbops.fb_mmap
-* fbdefio: individual delays
-*/
-   fbdefio = kzalloc(sizeof(*fbdefio), GFP_KERNEL);
-   fbops = kzalloc(sizeof(*fbops), GFP_KERNEL);
-   if (!fbdefio || !fbops) {
-   kfree(fbdefio);
-   kfree(fbops);
-   return -ENOMEM;
-   }
-
-   /* can't be offset from vaddr since dirty() uses cma_obj */
-   fbi->screen_buffer = cma_obj->vaddr;
-   /* fb_deferred_io_fault() needs a physical address */
-   fbi->fix.smem_start = page_to_phys(virt_to_page(fbi->screen_buffer));
-
-   *fbops = *fbi->fbops;
-   fbi->fbops = fbops;
-
-   fbdefio->delay = msecs_to_jiffies(DEFAULT_FBDEFIO_DELAY_MS);
-   fbdefio->deferred_io = drm_fb_helper_deferred_io;
-   fbi->fbdefio = fbdefio;
-   fb_deferred_io_init(fbi);
-   fbi->fbops->fb_mmap = drm_fbdev_cma_deferred_io_mmap;
-
-   return 0;
-}
-
-static void drm_fbdev_cma_defio_fini(struct fb_info *fbi)
-{
-   if (!fbi->fbdefio)
-   return;
-
-   fb_deferred_io_cleanup(fbi);
-   kfree(fbi->fbdefio);
-   kfree(fbi->fbops);
-}
-
-static int
-drm_fbdev_cma_create(struct drm_fb_helper *helper,
-   struct drm_fb_helper_surface_size *sizes)
-{
-   struct drm_fbdev_cma *fbdev_cma = to_fbdev_cma(helper);
-   struct drm_device *dev = helper->dev;
-   struct drm_gem_cma_object *obj;
-   struct drm_framebuffer *fb;
-   unsigned int bytes_per_pixel;
-   unsigned long offset;
-   struct fb_info *fbi;
-   size_t size;
-   int ret;
-
-   DRM_DEBUG_KMS("surface width(%d), height(%d) and bpp(%d)\n",
-   sizes->surface_width, sizes->surface_height,
-   sizes->surface_bpp);
-
-   bytes_per_pixel = DIV_ROUND_UP(sizes->surface_bpp, 8);
-   size = sizes->surface_width * sizes->surface_height * bytes_per_pixel;
-   obj = drm_gem_cma_create(dev, size);
-   if (IS_ERR(obj))
-   return -ENOMEM;
-
-   fbi = drm_fb_helper_alloc_fbi(helper);
-   if (IS_ERR(fbi)) {
-   ret = PTR_ERR(fbi);
-   goto err_gem_free_object;
-   }
-
-   fb = drm_gem_fbdev_fb_create(dev, sizes, 0, >base,
-fbdev_cma->fb_funcs);
-   if (IS_ERR(fb)) {
-   dev_err(dev->dev, "Failed to allocate DRM framebuffer.\n");
-   ret = PTR_ERR(fb);
-   goto err_fb_info_destroy;
-   }
-
-   helper->fb = fb;
-
-   fbi->par = helper;
-   fbi->flags = FBINFO_FLAG_DEFAULT;
-   fbi->fbops = _fbdev_cma_ops;
-
-   drm_fb_helper_fill_fix(fbi, fb->pitches[0], fb->format->depth);
-   drm_fb_helper_fill_var(fbi, helper, sizes->fb_width, sizes->fb_height);
-
-   offset = fbi->var.xoffset * bytes_per_pixel;
-   offset += fbi->var.yoffset * fb->pitches[0];
-
-   dev->mode_config.fb_base = (resource_size_t)obj->paddr;
-   fbi->screen_base = obj->vaddr + offset;
-   fbi->fix.smem_start = (unsigned long)(obj->paddr + offset);

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).  This was relatively simple enough
to figure out and fix, but bumping 

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

2018-08-21 Thread Daniel Vetter
On Tue, Aug 21, 2018 at 04:59:46PM +0200, Noralf Trønnes wrote:
> 
> Den 21.08.2018 10.44, skrev Daniel Vetter:
> > On Mon, Aug 20, 2018 at 11:44:56PM -0700, John Stultz wrote:
> > > On Tue, Jul 3, 2018 at 9:03 AM, Noralf Trønnes  wrote:
> > > > This switches the CMA helper drivers that use its fbdev emulation over
> > > > to the generic fbdev emulation. It's the first phase of using generic
> > > > fbdev. A later phase will use DRM client callbacks for the
> > > > lastclose/hotplug/remove callbacks.
> > > > 
> > > > There are currently 2 fbdev init/fini functions:
> > > > - drm_fb_cma_fbdev_init/drm_fb_cma_fbdev_fini
> > > > - drm_fbdev_cma_init/drm_fbdev_cma_fini
> > > > 
> > > > This is because the work on generic fbdev came up during a fbdev
> > > > refactoring and thus wasn't completed. No point in completing that
> > > > refactoring when drivers will soon move to 
> > > > drm_fb_helper_generic_probe().
> > > > 
> > > > tinydrm uses drm_fb_cma_fbdev_init_with_funcs().
> > > > 
> > > > Cc: Laurent Pinchart 
> > > > Signed-off-by: Noralf Trønnes 
> > > > Acked-by: Daniel Vetter 
> > > > ---
> > > >   drivers/gpu/drm/drm_fb_cma_helper.c | 360 
> > > > +---
> > > >   include/drm/drm_fb_cma_helper.h |   3 -
> > > >   2 files changed, 42 insertions(+), 321 deletions(-)
> > > ...
> > > > -static int drm_fbdev_cma_defio_init(struct fb_info *fbi,
> > > > -   struct drm_gem_cma_object *cma_obj)
> > > > -{
> > > > -   struct fb_deferred_io *fbdefio;
> > > > -   struct fb_ops *fbops;
> > > > -
> > > > -   /*
> > > > -* Per device structures are needed because:
> > > > -* fbops: fb_deferred_io_cleanup() clears fbops.fb_mmap
> > > > -* fbdefio: individual delays
> > > > -*/
> > > > -   fbdefio = kzalloc(sizeof(*fbdefio), GFP_KERNEL);
> > > > -   fbops = kzalloc(sizeof(*fbops), GFP_KERNEL);
> > > > -   if (!fbdefio || !fbops) {
> > > > -   kfree(fbdefio);
> > > > -   kfree(fbops);
> > > > -   return -ENOMEM;
> > > > -   }
> > > > -
> > > > -   /* can't be offset from vaddr since dirty() uses cma_obj */
> > > > -   fbi->screen_buffer = cma_obj->vaddr;
> > > > -   /* fb_deferred_io_fault() needs a physical address */
> > > > -   fbi->fix.smem_start = 
> > > > page_to_phys(virt_to_page(fbi->screen_buffer));
> > > > -
> > > > -   *fbops = *fbi->fbops;
> > > > -   fbi->fbops = fbops;
> > > > -
> > > > -   fbdefio->delay = msecs_to_jiffies(DEFAULT_FBDEFIO_DELAY_MS);
> > > > -   fbdefio->deferred_io = drm_fb_helper_deferred_io;
> > > > -   fbi->fbdefio = fbdefio;
> > > > -   fb_deferred_io_init(fbi);
> > > > -   fbi->fbops->fb_mmap = drm_fbdev_cma_deferred_io_mmap;
> > > > -
> > > > -   return 0;
> > > > -}
> > > > -
> > > > -static void drm_fbdev_cma_defio_fini(struct fb_info *fbi)
> > > > -{
> > > > -   if (!fbi->fbdefio)
> > > > -   return;
> > > > -
> > > > -   fb_deferred_io_cleanup(fbi);
> > > > -   kfree(fbi->fbdefio);
> > > > -   kfree(fbi->fbops);
> > > > -}
> > > > -
> > > > -static int
> > > > -drm_fbdev_cma_create(struct drm_fb_helper *helper,
> > > > -   struct drm_fb_helper_surface_size *sizes)
> > > > -{
> > > > -   struct drm_fbdev_cma *fbdev_cma = to_fbdev_cma(helper);
> > > > -   struct drm_device *dev = helper->dev;
> > > > -   struct drm_gem_cma_object *obj;
> > > > -   struct drm_framebuffer *fb;
> > > > -   unsigned int bytes_per_pixel;
> > > > -   unsigned long offset;
> > > > -   struct fb_info *fbi;
> > > > -   size_t size;
> > > > -   int ret;
> > > > -
> > > > -   DRM_DEBUG_KMS("surface width(%d), height(%d) and bpp(%d)\n",
> > > > -   sizes->surface_width, sizes->surface_height,
> > > > -   sizes->surface_bpp);
> > > > -
> > > > -   bytes_per_pixel = DIV_ROUND_UP(sizes->surface_bpp, 8);
> > > > -   size = sizes->surface_width * sizes->surface_height * 
> > > > bytes_per_pixel;
> > > > -   obj = drm_gem_cma_create(dev, size);
> > > > -   if (IS_ERR(obj))
> > > > -   return -ENOMEM;
> > > > -
> > > > -   fbi = drm_fb_helper_alloc_fbi(helper);
> > > > -   if (IS_ERR(fbi)) {
> > > > -   ret = PTR_ERR(fbi);
> > > > -   goto err_gem_free_object;
> > > > -   }
> > > > -
> > > > -   fb = drm_gem_fbdev_fb_create(dev, sizes, 0, >base,
> > > > -fbdev_cma->fb_funcs);
> > > > -   if (IS_ERR(fb)) {
> > > > -   dev_err(dev->dev, "Failed to allocate DRM 
> > > > framebuffer.\n");
> > > > -   ret = PTR_ERR(fb);
> > > > -   goto err_fb_info_destroy;
> > > > -   }
> > > > -
> > > > -   helper->fb = fb;
> > > > -
> > > > -   fbi->par = helper;
> > > > -   fbi->flags = FBINFO_FLAG_DEFAULT;
> > > > -   fbi->fbops = _fbdev_cma_ops;
> > > 

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

2018-08-21 Thread Noralf Trønnes


Den 21.08.2018 10.44, skrev Daniel Vetter:

On Mon, Aug 20, 2018 at 11:44:56PM -0700, John Stultz wrote:

On Tue, Jul 3, 2018 at 9:03 AM, Noralf Trønnes  wrote:

This switches the CMA helper drivers that use its fbdev emulation over
to the generic fbdev emulation. It's the first phase of using generic
fbdev. A later phase will use DRM client callbacks for the
lastclose/hotplug/remove callbacks.

There are currently 2 fbdev init/fini functions:
- drm_fb_cma_fbdev_init/drm_fb_cma_fbdev_fini
- drm_fbdev_cma_init/drm_fbdev_cma_fini

This is because the work on generic fbdev came up during a fbdev
refactoring and thus wasn't completed. No point in completing that
refactoring when drivers will soon move to drm_fb_helper_generic_probe().

tinydrm uses drm_fb_cma_fbdev_init_with_funcs().

Cc: Laurent Pinchart 
Signed-off-by: Noralf Trønnes 
Acked-by: Daniel Vetter 
---
  drivers/gpu/drm/drm_fb_cma_helper.c | 360 +---
  include/drm/drm_fb_cma_helper.h |   3 -
  2 files changed, 42 insertions(+), 321 deletions(-)

...

-static int drm_fbdev_cma_defio_init(struct fb_info *fbi,
-   struct drm_gem_cma_object *cma_obj)
-{
-   struct fb_deferred_io *fbdefio;
-   struct fb_ops *fbops;
-
-   /*
-* Per device structures are needed because:
-* fbops: fb_deferred_io_cleanup() clears fbops.fb_mmap
-* fbdefio: individual delays
-*/
-   fbdefio = kzalloc(sizeof(*fbdefio), GFP_KERNEL);
-   fbops = kzalloc(sizeof(*fbops), GFP_KERNEL);
-   if (!fbdefio || !fbops) {
-   kfree(fbdefio);
-   kfree(fbops);
-   return -ENOMEM;
-   }
-
-   /* can't be offset from vaddr since dirty() uses cma_obj */
-   fbi->screen_buffer = cma_obj->vaddr;
-   /* fb_deferred_io_fault() needs a physical address */
-   fbi->fix.smem_start = page_to_phys(virt_to_page(fbi->screen_buffer));
-
-   *fbops = *fbi->fbops;
-   fbi->fbops = fbops;
-
-   fbdefio->delay = msecs_to_jiffies(DEFAULT_FBDEFIO_DELAY_MS);
-   fbdefio->deferred_io = drm_fb_helper_deferred_io;
-   fbi->fbdefio = fbdefio;
-   fb_deferred_io_init(fbi);
-   fbi->fbops->fb_mmap = drm_fbdev_cma_deferred_io_mmap;
-
-   return 0;
-}
-
-static void drm_fbdev_cma_defio_fini(struct fb_info *fbi)
-{
-   if (!fbi->fbdefio)
-   return;
-
-   fb_deferred_io_cleanup(fbi);
-   kfree(fbi->fbdefio);
-   kfree(fbi->fbops);
-}
-
-static int
-drm_fbdev_cma_create(struct drm_fb_helper *helper,
-   struct drm_fb_helper_surface_size *sizes)
-{
-   struct drm_fbdev_cma *fbdev_cma = to_fbdev_cma(helper);
-   struct drm_device *dev = helper->dev;
-   struct drm_gem_cma_object *obj;
-   struct drm_framebuffer *fb;
-   unsigned int bytes_per_pixel;
-   unsigned long offset;
-   struct fb_info *fbi;
-   size_t size;
-   int ret;
-
-   DRM_DEBUG_KMS("surface width(%d), height(%d) and bpp(%d)\n",
-   sizes->surface_width, sizes->surface_height,
-   sizes->surface_bpp);
-
-   bytes_per_pixel = DIV_ROUND_UP(sizes->surface_bpp, 8);
-   size = sizes->surface_width * sizes->surface_height * bytes_per_pixel;
-   obj = drm_gem_cma_create(dev, size);
-   if (IS_ERR(obj))
-   return -ENOMEM;
-
-   fbi = drm_fb_helper_alloc_fbi(helper);
-   if (IS_ERR(fbi)) {
-   ret = PTR_ERR(fbi);
-   goto err_gem_free_object;
-   }
-
-   fb = drm_gem_fbdev_fb_create(dev, sizes, 0, >base,
-fbdev_cma->fb_funcs);
-   if (IS_ERR(fb)) {
-   dev_err(dev->dev, "Failed to allocate DRM framebuffer.\n");
-   ret = PTR_ERR(fb);
-   goto err_fb_info_destroy;
-   }
-
-   helper->fb = fb;
-
-   fbi->par = helper;
-   fbi->flags = FBINFO_FLAG_DEFAULT;
-   fbi->fbops = _fbdev_cma_ops;
-
-   drm_fb_helper_fill_fix(fbi, fb->pitches[0], fb->format->depth);
-   drm_fb_helper_fill_var(fbi, helper, sizes->fb_width, sizes->fb_height);
-
-   offset = fbi->var.xoffset * bytes_per_pixel;
-   offset += fbi->var.yoffset * fb->pitches[0];
-
-   dev->mode_config.fb_base = (resource_size_t)obj->paddr;
-   fbi->screen_base = obj->vaddr + offset;
-   fbi->fix.smem_start = (unsigned long)(obj->paddr + offset);

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).  This was relatively simple enough
to figure out and fix, but bumping the values up on its own didn't
seem to resolve the issue entirely, as I started to see hard hangs on
bootup when 

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

2018-08-21 Thread Noralf Trønnes


Den 21.08.2018 08.44, skrev John Stultz:

On Tue, Jul 3, 2018 at 9:03 AM, Noralf Trønnes  wrote:

This switches the CMA helper drivers that use its fbdev emulation over
to the generic fbdev emulation. It's the first phase of using generic
fbdev. A later phase will use DRM client callbacks for the
lastclose/hotplug/remove callbacks.

There are currently 2 fbdev init/fini functions:
- drm_fb_cma_fbdev_init/drm_fb_cma_fbdev_fini
- drm_fbdev_cma_init/drm_fbdev_cma_fini

This is because the work on generic fbdev came up during a fbdev
refactoring and thus wasn't completed. No point in completing that
refactoring when drivers will soon move to drm_fb_helper_generic_probe().

tinydrm uses drm_fb_cma_fbdev_init_with_funcs().

Cc: Laurent Pinchart 
Signed-off-by: Noralf Trønnes 
Acked-by: Daniel Vetter 
---
  drivers/gpu/drm/drm_fb_cma_helper.c | 360 +---
  include/drm/drm_fb_cma_helper.h |   3 -
  2 files changed, 42 insertions(+), 321 deletions(-)

...

-static int drm_fbdev_cma_defio_init(struct fb_info *fbi,
-   struct drm_gem_cma_object *cma_obj)
-{
-   struct fb_deferred_io *fbdefio;
-   struct fb_ops *fbops;
-
-   /*
-* Per device structures are needed because:
-* fbops: fb_deferred_io_cleanup() clears fbops.fb_mmap
-* fbdefio: individual delays
-*/
-   fbdefio = kzalloc(sizeof(*fbdefio), GFP_KERNEL);
-   fbops = kzalloc(sizeof(*fbops), GFP_KERNEL);
-   if (!fbdefio || !fbops) {
-   kfree(fbdefio);
-   kfree(fbops);
-   return -ENOMEM;
-   }
-
-   /* can't be offset from vaddr since dirty() uses cma_obj */
-   fbi->screen_buffer = cma_obj->vaddr;
-   /* fb_deferred_io_fault() needs a physical address */
-   fbi->fix.smem_start = page_to_phys(virt_to_page(fbi->screen_buffer));
-
-   *fbops = *fbi->fbops;
-   fbi->fbops = fbops;
-
-   fbdefio->delay = msecs_to_jiffies(DEFAULT_FBDEFIO_DELAY_MS);
-   fbdefio->deferred_io = drm_fb_helper_deferred_io;
-   fbi->fbdefio = fbdefio;
-   fb_deferred_io_init(fbi);
-   fbi->fbops->fb_mmap = drm_fbdev_cma_deferred_io_mmap;
-
-   return 0;
-}
-
-static void drm_fbdev_cma_defio_fini(struct fb_info *fbi)
-{
-   if (!fbi->fbdefio)
-   return;
-
-   fb_deferred_io_cleanup(fbi);
-   kfree(fbi->fbdefio);
-   kfree(fbi->fbops);
-}
-
-static int
-drm_fbdev_cma_create(struct drm_fb_helper *helper,
-   struct drm_fb_helper_surface_size *sizes)
-{
-   struct drm_fbdev_cma *fbdev_cma = to_fbdev_cma(helper);
-   struct drm_device *dev = helper->dev;
-   struct drm_gem_cma_object *obj;
-   struct drm_framebuffer *fb;
-   unsigned int bytes_per_pixel;
-   unsigned long offset;
-   struct fb_info *fbi;
-   size_t size;
-   int ret;
-
-   DRM_DEBUG_KMS("surface width(%d), height(%d) and bpp(%d)\n",
-   sizes->surface_width, sizes->surface_height,
-   sizes->surface_bpp);
-
-   bytes_per_pixel = DIV_ROUND_UP(sizes->surface_bpp, 8);
-   size = sizes->surface_width * sizes->surface_height * bytes_per_pixel;
-   obj = drm_gem_cma_create(dev, size);
-   if (IS_ERR(obj))
-   return -ENOMEM;
-
-   fbi = drm_fb_helper_alloc_fbi(helper);
-   if (IS_ERR(fbi)) {
-   ret = PTR_ERR(fbi);
-   goto err_gem_free_object;
-   }
-
-   fb = drm_gem_fbdev_fb_create(dev, sizes, 0, >base,
-fbdev_cma->fb_funcs);
-   if (IS_ERR(fb)) {
-   dev_err(dev->dev, "Failed to allocate DRM framebuffer.\n");
-   ret = PTR_ERR(fb);
-   goto err_fb_info_destroy;
-   }
-
-   helper->fb = fb;
-
-   fbi->par = helper;
-   fbi->flags = FBINFO_FLAG_DEFAULT;
-   fbi->fbops = _fbdev_cma_ops;
-
-   drm_fb_helper_fill_fix(fbi, fb->pitches[0], fb->format->depth);
-   drm_fb_helper_fill_var(fbi, helper, sizes->fb_width, sizes->fb_height);
-
-   offset = fbi->var.xoffset * bytes_per_pixel;
-   offset += fbi->var.yoffset * fb->pitches[0];
-
-   dev->mode_config.fb_base = (resource_size_t)obj->paddr;
-   fbi->screen_base = obj->vaddr + offset;
-   fbi->fix.smem_start = (unsigned long)(obj->paddr + offset);

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).  This was relatively simple enough
to figure out and fix, but bumping the values up on its own didn't
seem to resolve the issue entirely, as I started to see hard hangs on
bootup when userspace started using the emulated fbdev device.

After 

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

2018-08-21 Thread Daniel Vetter
On Mon, Aug 20, 2018 at 11:44:56PM -0700, John Stultz wrote:
> On Tue, Jul 3, 2018 at 9:03 AM, Noralf Trønnes  wrote:
> > This switches the CMA helper drivers that use its fbdev emulation over
> > to the generic fbdev emulation. It's the first phase of using generic
> > fbdev. A later phase will use DRM client callbacks for the
> > lastclose/hotplug/remove callbacks.
> >
> > There are currently 2 fbdev init/fini functions:
> > - drm_fb_cma_fbdev_init/drm_fb_cma_fbdev_fini
> > - drm_fbdev_cma_init/drm_fbdev_cma_fini
> >
> > This is because the work on generic fbdev came up during a fbdev
> > refactoring and thus wasn't completed. No point in completing that
> > refactoring when drivers will soon move to drm_fb_helper_generic_probe().
> >
> > tinydrm uses drm_fb_cma_fbdev_init_with_funcs().
> >
> > Cc: Laurent Pinchart 
> > Signed-off-by: Noralf Trønnes 
> > Acked-by: Daniel Vetter 
> > ---
> >  drivers/gpu/drm/drm_fb_cma_helper.c | 360 
> > +---
> >  include/drm/drm_fb_cma_helper.h |   3 -
> >  2 files changed, 42 insertions(+), 321 deletions(-)
> ...
> > -static int drm_fbdev_cma_defio_init(struct fb_info *fbi,
> > -   struct drm_gem_cma_object *cma_obj)
> > -{
> > -   struct fb_deferred_io *fbdefio;
> > -   struct fb_ops *fbops;
> > -
> > -   /*
> > -* Per device structures are needed because:
> > -* fbops: fb_deferred_io_cleanup() clears fbops.fb_mmap
> > -* fbdefio: individual delays
> > -*/
> > -   fbdefio = kzalloc(sizeof(*fbdefio), GFP_KERNEL);
> > -   fbops = kzalloc(sizeof(*fbops), GFP_KERNEL);
> > -   if (!fbdefio || !fbops) {
> > -   kfree(fbdefio);
> > -   kfree(fbops);
> > -   return -ENOMEM;
> > -   }
> > -
> > -   /* can't be offset from vaddr since dirty() uses cma_obj */
> > -   fbi->screen_buffer = cma_obj->vaddr;
> > -   /* fb_deferred_io_fault() needs a physical address */
> > -   fbi->fix.smem_start = 
> > page_to_phys(virt_to_page(fbi->screen_buffer));
> > -
> > -   *fbops = *fbi->fbops;
> > -   fbi->fbops = fbops;
> > -
> > -   fbdefio->delay = msecs_to_jiffies(DEFAULT_FBDEFIO_DELAY_MS);
> > -   fbdefio->deferred_io = drm_fb_helper_deferred_io;
> > -   fbi->fbdefio = fbdefio;
> > -   fb_deferred_io_init(fbi);
> > -   fbi->fbops->fb_mmap = drm_fbdev_cma_deferred_io_mmap;
> > -
> > -   return 0;
> > -}
> > -
> > -static void drm_fbdev_cma_defio_fini(struct fb_info *fbi)
> > -{
> > -   if (!fbi->fbdefio)
> > -   return;
> > -
> > -   fb_deferred_io_cleanup(fbi);
> > -   kfree(fbi->fbdefio);
> > -   kfree(fbi->fbops);
> > -}
> > -
> > -static int
> > -drm_fbdev_cma_create(struct drm_fb_helper *helper,
> > -   struct drm_fb_helper_surface_size *sizes)
> > -{
> > -   struct drm_fbdev_cma *fbdev_cma = to_fbdev_cma(helper);
> > -   struct drm_device *dev = helper->dev;
> > -   struct drm_gem_cma_object *obj;
> > -   struct drm_framebuffer *fb;
> > -   unsigned int bytes_per_pixel;
> > -   unsigned long offset;
> > -   struct fb_info *fbi;
> > -   size_t size;
> > -   int ret;
> > -
> > -   DRM_DEBUG_KMS("surface width(%d), height(%d) and bpp(%d)\n",
> > -   sizes->surface_width, sizes->surface_height,
> > -   sizes->surface_bpp);
> > -
> > -   bytes_per_pixel = DIV_ROUND_UP(sizes->surface_bpp, 8);
> > -   size = sizes->surface_width * sizes->surface_height * 
> > bytes_per_pixel;
> > -   obj = drm_gem_cma_create(dev, size);
> > -   if (IS_ERR(obj))
> > -   return -ENOMEM;
> > -
> > -   fbi = drm_fb_helper_alloc_fbi(helper);
> > -   if (IS_ERR(fbi)) {
> > -   ret = PTR_ERR(fbi);
> > -   goto err_gem_free_object;
> > -   }
> > -
> > -   fb = drm_gem_fbdev_fb_create(dev, sizes, 0, >base,
> > -fbdev_cma->fb_funcs);
> > -   if (IS_ERR(fb)) {
> > -   dev_err(dev->dev, "Failed to allocate DRM framebuffer.\n");
> > -   ret = PTR_ERR(fb);
> > -   goto err_fb_info_destroy;
> > -   }
> > -
> > -   helper->fb = fb;
> > -
> > -   fbi->par = helper;
> > -   fbi->flags = FBINFO_FLAG_DEFAULT;
> > -   fbi->fbops = _fbdev_cma_ops;
> > -
> > -   drm_fb_helper_fill_fix(fbi, fb->pitches[0], fb->format->depth);
> > -   drm_fb_helper_fill_var(fbi, helper, sizes->fb_width, 
> > sizes->fb_height);
> > -
> > -   offset = fbi->var.xoffset * bytes_per_pixel;
> > -   offset += fbi->var.yoffset * fb->pitches[0];
> > -
> > -   dev->mode_config.fb_base = (resource_size_t)obj->paddr;
> > -   fbi->screen_base = obj->vaddr + offset;
> > -   fbi->fix.smem_start = (unsigned long)(obj->paddr + offset);
> 
> Hey Noralf, all,
>   I've been digging for a bit on the regression that this patch has
> tripped on the HiKey board 

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

2018-08-21 Thread John Stultz
On Tue, Jul 3, 2018 at 9:03 AM, Noralf Trønnes  wrote:
> This switches the CMA helper drivers that use its fbdev emulation over
> to the generic fbdev emulation. It's the first phase of using generic
> fbdev. A later phase will use DRM client callbacks for the
> lastclose/hotplug/remove callbacks.
>
> There are currently 2 fbdev init/fini functions:
> - drm_fb_cma_fbdev_init/drm_fb_cma_fbdev_fini
> - drm_fbdev_cma_init/drm_fbdev_cma_fini
>
> This is because the work on generic fbdev came up during a fbdev
> refactoring and thus wasn't completed. No point in completing that
> refactoring when drivers will soon move to drm_fb_helper_generic_probe().
>
> tinydrm uses drm_fb_cma_fbdev_init_with_funcs().
>
> Cc: Laurent Pinchart 
> Signed-off-by: Noralf Trønnes 
> Acked-by: Daniel Vetter 
> ---
>  drivers/gpu/drm/drm_fb_cma_helper.c | 360 
> +---
>  include/drm/drm_fb_cma_helper.h |   3 -
>  2 files changed, 42 insertions(+), 321 deletions(-)
...
> -static int drm_fbdev_cma_defio_init(struct fb_info *fbi,
> -   struct drm_gem_cma_object *cma_obj)
> -{
> -   struct fb_deferred_io *fbdefio;
> -   struct fb_ops *fbops;
> -
> -   /*
> -* Per device structures are needed because:
> -* fbops: fb_deferred_io_cleanup() clears fbops.fb_mmap
> -* fbdefio: individual delays
> -*/
> -   fbdefio = kzalloc(sizeof(*fbdefio), GFP_KERNEL);
> -   fbops = kzalloc(sizeof(*fbops), GFP_KERNEL);
> -   if (!fbdefio || !fbops) {
> -   kfree(fbdefio);
> -   kfree(fbops);
> -   return -ENOMEM;
> -   }
> -
> -   /* can't be offset from vaddr since dirty() uses cma_obj */
> -   fbi->screen_buffer = cma_obj->vaddr;
> -   /* fb_deferred_io_fault() needs a physical address */
> -   fbi->fix.smem_start = page_to_phys(virt_to_page(fbi->screen_buffer));
> -
> -   *fbops = *fbi->fbops;
> -   fbi->fbops = fbops;
> -
> -   fbdefio->delay = msecs_to_jiffies(DEFAULT_FBDEFIO_DELAY_MS);
> -   fbdefio->deferred_io = drm_fb_helper_deferred_io;
> -   fbi->fbdefio = fbdefio;
> -   fb_deferred_io_init(fbi);
> -   fbi->fbops->fb_mmap = drm_fbdev_cma_deferred_io_mmap;
> -
> -   return 0;
> -}
> -
> -static void drm_fbdev_cma_defio_fini(struct fb_info *fbi)
> -{
> -   if (!fbi->fbdefio)
> -   return;
> -
> -   fb_deferred_io_cleanup(fbi);
> -   kfree(fbi->fbdefio);
> -   kfree(fbi->fbops);
> -}
> -
> -static int
> -drm_fbdev_cma_create(struct drm_fb_helper *helper,
> -   struct drm_fb_helper_surface_size *sizes)
> -{
> -   struct drm_fbdev_cma *fbdev_cma = to_fbdev_cma(helper);
> -   struct drm_device *dev = helper->dev;
> -   struct drm_gem_cma_object *obj;
> -   struct drm_framebuffer *fb;
> -   unsigned int bytes_per_pixel;
> -   unsigned long offset;
> -   struct fb_info *fbi;
> -   size_t size;
> -   int ret;
> -
> -   DRM_DEBUG_KMS("surface width(%d), height(%d) and bpp(%d)\n",
> -   sizes->surface_width, sizes->surface_height,
> -   sizes->surface_bpp);
> -
> -   bytes_per_pixel = DIV_ROUND_UP(sizes->surface_bpp, 8);
> -   size = sizes->surface_width * sizes->surface_height * bytes_per_pixel;
> -   obj = drm_gem_cma_create(dev, size);
> -   if (IS_ERR(obj))
> -   return -ENOMEM;
> -
> -   fbi = drm_fb_helper_alloc_fbi(helper);
> -   if (IS_ERR(fbi)) {
> -   ret = PTR_ERR(fbi);
> -   goto err_gem_free_object;
> -   }
> -
> -   fb = drm_gem_fbdev_fb_create(dev, sizes, 0, >base,
> -fbdev_cma->fb_funcs);
> -   if (IS_ERR(fb)) {
> -   dev_err(dev->dev, "Failed to allocate DRM framebuffer.\n");
> -   ret = PTR_ERR(fb);
> -   goto err_fb_info_destroy;
> -   }
> -
> -   helper->fb = fb;
> -
> -   fbi->par = helper;
> -   fbi->flags = FBINFO_FLAG_DEFAULT;
> -   fbi->fbops = _fbdev_cma_ops;
> -
> -   drm_fb_helper_fill_fix(fbi, fb->pitches[0], fb->format->depth);
> -   drm_fb_helper_fill_var(fbi, helper, sizes->fb_width, 
> sizes->fb_height);
> -
> -   offset = fbi->var.xoffset * bytes_per_pixel;
> -   offset += fbi->var.yoffset * fb->pitches[0];
> -
> -   dev->mode_config.fb_base = (resource_size_t)obj->paddr;
> -   fbi->screen_base = obj->vaddr + offset;
> -   fbi->fix.smem_start = (unsigned long)(obj->paddr + offset);

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).  This was relatively simple enough
to figure out and