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

From: Stefan Lengfeld <contact@stefanchrist.eu>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: Dave Airlie <airlied@redhat.com>,
	dri-devel <dri-devel@lists.freedesktop.org>
Subject: Re: [PATCH 04/20] drm/ast: use DRM_FB_HELPER_DEFAULT_OPS for fb_ops
Date: Wed, 9 Nov 2016 22:33:30 +0100	[thread overview]
Message-ID: <20161109213330.GA12423@sill.h.stcim.de> (raw)
In-Reply-To: <20161108171231.7bsku2c2qd4mhxbq@phenom.ffwll.local>

Hi Daniel,

On Tue, Nov 08, 2016 at 06:12:31PM +0100, Daniel Vetter wrote:
> On Wed, Oct 26, 2016 at 09:15:15PM +0200, Daniel Vetter wrote:
> > On Wed, Oct 26, 2016 at 8:47 PM, Stefan Lengfeld
> > <contact@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@redhat.com>
> > >> > Signed-off-by: Stefan Christ <contact@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.
> 
> Are you still working on a v2, or should I just pick up v1 for now?
> -Daniel

I will send a v2 patch set in the next days.

Kind regards,
	Stefan Christ
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel