| Message ID | 1475182136-15191-5-git-send-email-contact@stefanchrist.eu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
Hi Daniel, 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(). 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. 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? Kind regards, Stefan Christ > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch >
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. -Daniel
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
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
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, - .fb_pan_display = drm_fb_helper_pan_display, - .fb_blank = drm_fb_helper_blank, - .fb_setcmap = drm_fb_helper_setcmap, .fb_debug_enter = drm_fb_helper_debug_enter, .fb_debug_leave = drm_fb_helper_debug_leave, };
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(-)