Nouveau - Aug 2017 - [PATCH libdrm] drm: Remove create_handle() drm_framebuffer "virtual".

On Wed, Aug 9, 2017 at 12:14 PM, Noralf Trønnes <noralf at tronnes.org>
wrote:
>
> Den 09.08.2017 01.42, skrev Joe Kniss:
>>
>> Because all drivers currently use gem objects for framebuffer planes,
>> the virtual create_handle() is not required.  This change adds a
>> struct drm_gem_object *gems[4] field to drm_framebuffer and removes
>> create_handle() function pointer from drm_framebuffer_funcs.  The
>> corresponding *_create_handle() function is removed from each driver.
>>
>> In many cases this change eliminates a struct *_framebuffer object,
>> as the only need for the derived struct is the addition of the gem
>> object pointer.
>>
>> TESTED: compiled: allyesconfig ARCH=x86,arm platforms:i915, rockchip
>>
>> Signed-off-by: Joe Kniss <djmk at google.com>
>> ---
>
>
> Hi Joe,
>
> I'm also looking into adding gem objs to drm_framebuffer in this patch:
> [PATCH v2 01/22] drm: Add GEM backed framebuffer library
> https://lists.freedesktop.org/archives/dri-devel/2017-August/149782.html
>
Great.  There's only minimal overlap here.  I'll rebase this change on
yours
once it's in.
> [...]
>
>> diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c
>> b/drivers/gpu/drm/drm_fb_cma_helper.c
>> index ade319d10e70..f5f011b910b1 100644
>> --- a/drivers/gpu/drm/drm_fb_cma_helper.c
>> +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
>> @@ -31,14 +31,9 @@
>>     #define DEFAULT_FBDEFIO_DELAY_MS 50
>>   -struct drm_fb_cma {
>> -       struct drm_framebuffer          fb;
>> -       struct drm_gem_cma_object       *obj[4];
>> -};
>> -
>>   struct drm_fbdev_cma {
>>         struct drm_fb_helper    fb_helper;
>> -       struct drm_fb_cma       *fb;
>> +       struct drm_framebuffer  *fb;
>
>
> This fb pointer isn't necessary, since fb_helper already has one.
>
I'll remove it... but I am sure when I look deeper there will be more
of these in the various drivers too.
> Noralf.
>
>
-joe

On Wed, Aug 09, 2017 at 04:13:05PM -0700, Joe Kniss
wrote:
> On Wed, Aug 9, 2017 at 12:14 PM, Noralf Tr?nnes <noralf at
tronnes.org> wrote:
> >
> > Den 09.08.2017 01.42, skrev Joe Kniss:
> >>
> >> Because all drivers currently use gem objects for framebuffer
planes,
> >> the virtual create_handle() is not required.  This change adds a
> >> struct drm_gem_object *gems[4] field to drm_framebuffer and
removes
> >> create_handle() function pointer from drm_framebuffer_funcs.  The
> >> corresponding *_create_handle() function is removed from each
driver.
> >>
> >> In many cases this change eliminates a struct *_framebuffer
object,
> >> as the only need for the derived struct is the addition of the gem
> >> object pointer.
> >>
> >> TESTED: compiled: allyesconfig ARCH=x86,arm platforms:i915,
rockchip
> >>
> >> Signed-off-by: Joe Kniss <djmk at google.com>
> >> ---
> >
> >
> > Hi Joe,
> >
> > I'm also looking into adding gem objs to drm_framebuffer in this
patch:
> > [PATCH v2 01/22] drm: Add GEM backed framebuffer library
> >
https://lists.freedesktop.org/archives/dri-devel/2017-August/149782.html
> >
> 
> Great.  There's only minimal overlap here.  I'll rebase this change
on yours
> once it's in.
Even better than waiting: Reviewing each another's patches. Noralf has
commit rights and knows how driver-wide refactoring works, so you're all
covered to get this landed, if you provide a bit of review for the review
market :-)

Cheers, Daniel
> 
> > [...]
> >
> >> diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c
> >> b/drivers/gpu/drm/drm_fb_cma_helper.c
> >> index ade319d10e70..f5f011b910b1 100644
> >> --- a/drivers/gpu/drm/drm_fb_cma_helper.c
> >> +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
> >> @@ -31,14 +31,9 @@
> >>     #define DEFAULT_FBDEFIO_DELAY_MS 50
> >>   -struct drm_fb_cma {
> >> -       struct drm_framebuffer          fb;
> >> -       struct drm_gem_cma_object       *obj[4];
> >> -};
> >> -
> >>   struct drm_fbdev_cma {
> >>         struct drm_fb_helper    fb_helper;
> >> -       struct drm_fb_cma       *fb;
> >> +       struct drm_framebuffer  *fb;
> >
> >
> > This fb pointer isn't necessary, since fb_helper already has one.
> >
> 
> I'll remove it... but I am sure when I look deeper there will be more
> of these in the various drivers too.
> 
> > Noralf.
> >
> >
> 
> -joe
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

On Wed, Aug 9, 2017 at 4:13 PM, Joe Kniss <djmk at google.com>
wrote:
> On Wed, Aug 9, 2017 at 12:14 PM, Noralf Trønnes <noralf at
tronnes.org> wrote:
>>
>> Den 09.08.2017 01.42, skrev Joe Kniss:
>>>
>>> Because all drivers currently use gem objects for framebuffer
planes,
>>> the virtual create_handle() is not required.  This change adds a
>>> struct drm_gem_object *gems[4] field to drm_framebuffer and removes
>>> create_handle() function pointer from drm_framebuffer_funcs.  The
>>> corresponding *_create_handle() function is removed from each
driver.
>>>
>>> In many cases this change eliminates a struct *_framebuffer object,
>>> as the only need for the derived struct is the addition of the gem
>>> object pointer.
>>>
>>> TESTED: compiled: allyesconfig ARCH=x86,arm platforms:i915,
rockchip
>>>
>>> Signed-off-by: Joe Kniss <djmk at google.com>
>>> ---
>>
>>
>> Hi Joe,
>>
>> I'm also looking into adding gem objs to drm_framebuffer in this
patch:
>> [PATCH v2 01/22] drm: Add GEM backed framebuffer library
>>
https://lists.freedesktop.org/archives/dri-devel/2017-August/149782.html
>>
>
> Great.  There's only minimal overlap here.  I'll rebase this change
on yours
> once it's in.
>
>> [...]
>>
>>> diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c
>>> b/drivers/gpu/drm/drm_fb_cma_helper.c
>>> index ade319d10e70..f5f011b910b1 100644
>>> --- a/drivers/gpu/drm/drm_fb_cma_helper.c
>>> +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
>>> @@ -31,14 +31,9 @@
>>>     #define DEFAULT_FBDEFIO_DELAY_MS 50
>>>   -struct drm_fb_cma {
>>> -       struct drm_framebuffer          fb;
>>> -       struct drm_gem_cma_object       *obj[4];
>>> -};
>>> -
>>>   struct drm_fbdev_cma {
>>>         struct drm_fb_helper    fb_helper;
>>> -       struct drm_fb_cma       *fb;
>>> +       struct drm_framebuffer  *fb;
>>
>>
>> This fb pointer isn't necessary, since fb_helper already has one.
>>
So, looking deeper into this, it seems that the struct
drm_framebuffer_funcs *fb_funcs is also redundant here?  In which case
this whole struct can go...
>
> I'll remove it... but I am sure when I look deeper there will be more
> of these in the various drivers too.
>
>> Noralf.
>>
>>
>
> -joe

On 10 August 2017 at 20:29, Joe Kniss <djmk at google.com>
wrote:
> On Wed, Aug 9, 2017 at 4:13 PM, Joe Kniss <djmk at google.com> wrote:
>> On Wed, Aug 9, 2017 at 12:14 PM, Noralf Trønnes <noralf at
tronnes.org> wrote:
>>>
>>> Den 09.08.2017 01.42, skrev Joe Kniss:
>>>>
>>>> Because all drivers currently use gem objects for framebuffer
planes,
>>>> the virtual create_handle() is not required.  This change adds
a
>>>> struct drm_gem_object *gems[4] field to drm_framebuffer and
removes
>>>> create_handle() function pointer from drm_framebuffer_funcs. 
The
>>>> corresponding *_create_handle() function is removed from each
driver.
>>>>
>>>> In many cases this change eliminates a struct *_framebuffer
object,
>>>> as the only need for the derived struct is the addition of the
gem
>>>> object pointer.
>>>>
>>>> TESTED: compiled: allyesconfig ARCH=x86,arm platforms:i915,
rockchip
>>>>
>>>> Signed-off-by: Joe Kniss <djmk at google.com>
>>>> ---
>>>
>>>
>>> Hi Joe,
>>>
>>> I'm also looking into adding gem objs to drm_framebuffer in
this patch:
>>> [PATCH v2 01/22] drm: Add GEM backed framebuffer library
>>>
https://lists.freedesktop.org/archives/dri-devel/2017-August/149782.html
>>>
>>
>> Great.  There's only minimal overlap here.  I'll rebase this
change on yours
>> once it's in.
>>
>>> [...]
>>>
>>>> diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c
>>>> b/drivers/gpu/drm/drm_fb_cma_helper.c
>>>> index ade319d10e70..f5f011b910b1 100644
>>>> --- a/drivers/gpu/drm/drm_fb_cma_helper.c
>>>> +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
>>>> @@ -31,14 +31,9 @@
>>>>     #define DEFAULT_FBDEFIO_DELAY_MS 50
>>>>   -struct drm_fb_cma {
>>>> -       struct drm_framebuffer          fb;
>>>> -       struct drm_gem_cma_object       *obj[4];
>>>> -};
>>>> -
>>>>   struct drm_fbdev_cma {
>>>>         struct drm_fb_helper    fb_helper;
>>>> -       struct drm_fb_cma       *fb;
>>>> +       struct drm_framebuffer  *fb;
>>>
>>>
>>> This fb pointer isn't necessary, since fb_helper already has
one.
>>>
>
> So, looking deeper into this, it seems that the struct
> drm_framebuffer_funcs *fb_funcs is also redundant here?  In which case
> this whole struct can go...
>
I think you're spot on.

Perhaps the goal was to allow drivers to use separate funcs for fb vs fbdev?
Not sure how well that will fair, yet again, all current users use the
same funcs for both.

-Emil