[PATCH 04/20] drm/ast: use DRM_FB_HELPER_DEFAULT_OPS for fb_ops

[Daniel Vetter]

On Wed, Oct 26, 2016 at 8:47 PM, Stefan Lengfeld
<contact at stefanchrist.eu> wrote:
>
> On Tue, Oct 25, 2016 at 04:57:37PM +0200, Daniel Vetter wrote:
>> On Thu, Sep 29, 2016 at 10:48:40PM +0200, Stefan Christ wrote:
>> > Cc: Dave Airlie <airlied at redhat.com>
>> > Signed-off-by: Stefan Christ <contact at stefanchrist.eu>
>> > ---
>> >  drivers/gpu/drm/ast/ast_fb.c | 6 +-----
>> >  1 file changed, 1 insertion(+), 5 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/ast/ast_fb.c b/drivers/gpu/drm/ast/ast_fb.c
>> > index c017a93..b604fdd 100644
>> > --- a/drivers/gpu/drm/ast/ast_fb.c
>> > +++ b/drivers/gpu/drm/ast/ast_fb.c
>> > @@ -150,14 +150,10 @@ static void ast_imageblit(struct fb_info *info,
>> >
>> >  static struct fb_ops astfb_ops = {
>> >     .owner = THIS_MODULE,
>> > -   .fb_check_var = drm_fb_helper_check_var,
>> > -   .fb_set_par = drm_fb_helper_set_par,
>> > +   DRM_FB_HELPER_DEFAULT_OPS,
>> >     .fb_fillrect = ast_fillrect,
>> >     .fb_copyarea = ast_copyarea,
>> >     .fb_imageblit = ast_imageblit,
>>
>> Ah, here's the likely reason for not sharing these, ast/cirrus have their
>> special dirtying stuff for fbdev emulation. And because the fbdev mmap
>> must have a stable pointer it can't use the ttm bo mmap magic of
>> automatically picking the right place for the mmap.
>>
>> I'd say just leave these 2 drivers out as special cases.
>> -Daniel
>
> Hmm. There are even more drivers using special implementations like the
> mgag200 using function mga_fillrect(), which is a wrapper around
> drm_fb_helper_sys_fillrect().

Hm, mga_fillrect is like ast/cirrus.

> Even if the drivers ast/cirrus/.. are left out, there are still two
> different fb_fillrect, fb_copyarea and fb_imageblit implementations:
>    1/  drm_fb_helper_sys_*() and
>    2/  drm_fb_helper_cfb_*()
> used by different drivers. I don't know which one should be preferred.

Hm, every day I learn something new about fbdev. Totally missed that
there's 2 different kinds of helpers, and I think we do indeed need
both.

> Including fb_debug_enter and fb_debug_leave in DRM_FB_HELPER_DEFAULT_OPS
> is not a problem since there is only a single implementation yet.
>
> Should I resend this series (without the first patch), but with
> additional memebers fb_debug_enter and fb_debug_leave?

Yeah I think that'd be reasonable. For the sys/cfb stuff, what about
adding new #defines for those 2, e.g. DRM_FB_HELPER_SYS_OPS and
DRM_FB_HELPER_CFB_OPS? Maybe as a follow-up series of course, if you
have time. When resending please pick up the acks/reviews from this
series (but annoted with a v1 or so.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch