from:"Stefan Lengfeld"

[PATCH 1/3] drm/ast: use DRM_FB_HELPER_DEFAULT_OPS for fb_ops

2018-03-14 Thread Stefan Lengfeld

Small refactoring. Use the helper define DRM_FB_HELPER_DEFAULT_OPS that
provides the default implementations of the drm_fb_helper functions for
struct fb_ops.

Now the driver implements an additional member of the struct fb_ops:

.fb_ioctl   = drm_fb_helper_ioctl

This change is not tested.

Cc: Dave Airlie <airl...@redhat.com>
Signed-off-by: Stefan Lengfeld <cont...@stefanchrist.eu>
---
 drivers/gpu/drm/ast/ast_fb.c | 8 +---
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_fb.c b/drivers/gpu/drm/ast/ast_fb.c
index 0cd827e11fa2..e19b3bffe10b 100644
--- a/drivers/gpu/drm/ast/ast_fb.c
+++ b/drivers/gpu/drm/ast/ast_fb.c
@@ -149,16 +149,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,
 };
 
 static int astfb_create_object(struct ast_fbdev *afbdev,
-- 
2.16.1

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

[PATCH 0/3] Final round for DRM_FB_HELPER_DEFAULT_OPS usage

2018-03-14 Thread Stefan Lengfeld

Hi folks,

this is a follow up patch set to my initial tree wide refactoring

Subject: Introduce DRM_FB_HELPER_DEFAULT_OPS for struct fb_ops
https://patchwork.freedesktop.org/series/13099/

nearly one and a half year ago. It contains three patches for drivers

ast, hisilicon and mgag200

that are currently not using DRM_FB_HELPER_DEFAULT_OPS. 

If the conversion is appropriate for your driver, you can pick up the
patch. If not (because it may enable untested functionality), you can
just ignore it. It's only a gentle remainder in case the conversion was
missed in the first version/round.


In detail: One and a half year ago the drivers 'ast' and 'mgag200' did
not use much of the drm_fb_helper functions. Now they do, so using
DRM_FB_HELPER_DEFAULT_OPS seems beneficial. 

The drm driver 'hisilicon' was either missed in the initial patch set or
the driver was just added later.

Except the three drivers above, there are only two drivers not using
DRM_FB_HELPER_DEFAULT_OPS left in the whole tree (v4.16-rc1): 'cirrus'
and 'vmwgfx'.  The former is marked as *do-not-touch* by checkpatch.pl
and the later implements all fb_ops functions on it's on. No need for a
conversion.

Last but not least: The driver 'omapdrm' just received a patch for the
next kernel version to drop DRM_FB_HELPER_DEFAULT_OPS again. It causes a
compiler waring 'Initializer entry defined twice', because the driver
reassigns .fb_pan_display which is already provided in
DRM_FB_HELPER_DEFAULT_OPS.

Kind Regards,
Stefan

Cc: Dave Airlie <airl...@redhat.com>
Cc: Rongrong Zou <zourongr...@gmail.com>
Cc: Xinliang Liu <z.liuxinli...@hisilicon.com>
---

Stefan Lengfeld (3):
  drm/ast: use DRM_FB_HELPER_DEFAULT_OPS for fb_ops
  drm/hisilicon: use DRM_FB_HELPER_DEFAULT_OPS for fb_ops
  drm/mgag200: use DRM_FB_HELPER_DEFAULT_OPS for fb_ops

 drivers/gpu/drm/ast/ast_fb.c  | 8 +---
 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_fbdev.c | 6 +-
 drivers/gpu/drm/mgag200/mgag200_fb.c  | 6 +-
 3 files changed, 3 insertions(+), 17 deletions(-)

-- 
2.16.1

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

[PATCH 3/3] drm/mgag200: use DRM_FB_HELPER_DEFAULT_OPS for fb_ops

2018-03-14 Thread Stefan Lengfeld

Small refactoring. Use the helper define DRM_FB_HELPER_DEFAULT_OPS that
provides the default implementations of the drm_fb_helper functions for
struct fb_ops.

Now the driver implements three additional members of the struct fb_ops:

.fb_debug_enter = drm_fb_helper_debug_enter,
.fb_debug_leave = drm_fb_helper_debug_leave,
.fb_ioctl   = drm_fb_helper_ioctl

These changes are not tested.

Cc: Dave Airlie <airl...@redhat.com>
Signed-off-by: Stefan Lengfeld <cont...@stefanchrist.eu>
---
 drivers/gpu/drm/mgag200/mgag200_fb.c | 6 +-
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_fb.c 
b/drivers/gpu/drm/mgag200/mgag200_fb.c
index 30726c9fe28c..e02c11b6d27d 100644
--- a/drivers/gpu/drm/mgag200/mgag200_fb.c
+++ b/drivers/gpu/drm/mgag200/mgag200_fb.c
@@ -125,14 +125,10 @@ static void mga_imageblit(struct fb_info *info,
 
 static struct fb_ops mgag200fb_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 = mga_fillrect,
.fb_copyarea = mga_copyarea,
.fb_imageblit = mga_imageblit,
-   .fb_pan_display = drm_fb_helper_pan_display,
-   .fb_blank = drm_fb_helper_blank,
-   .fb_setcmap = drm_fb_helper_setcmap,
 };
 
 static int mgag200fb_create_object(struct mga_fbdev *afbdev,
-- 
2.16.1

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

[PATCH 2/3] drm/hisilicon: use DRM_FB_HELPER_DEFAULT_OPS for fb_ops

2018-03-14 Thread Stefan Lengfeld

Small refactoring. Use the helper define DRM_FB_HELPER_DEFAULT_OPS that
provides the default implementations of the drm_fb_helper functions for
struct fb_ops.

Now the driver implements three additional members of the struct fb_ops:

.fb_debug_enter = drm_fb_helper_debug_enter,
.fb_debug_leave = drm_fb_helper_debug_leave,
.fb_ioctl   = drm_fb_helper_ioctl

These changes are not tested.

Cc: Rongrong Zou <zourongr...@gmail.com>
Cc: Xinliang Liu <z.liuxinli...@hisilicon.com>
Signed-off-by: Stefan Lengfeld <cont...@stefanchrist.eu>
---
 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_fbdev.c | 6 +-
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_fbdev.c 
b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_fbdev.c
index b92595c477ef..b18b20535cff 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_fbdev.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_fbdev.c
@@ -43,14 +43,10 @@ static int hibmcfb_create_object(
 
 static struct fb_ops hibmc_drm_fb_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 = drm_fb_helper_sys_fillrect,
.fb_copyarea = drm_fb_helper_sys_copyarea,
.fb_imageblit = drm_fb_helper_sys_imageblit,
-   .fb_pan_display = drm_fb_helper_pan_display,
-   .fb_blank = drm_fb_helper_blank,
-   .fb_setcmap = drm_fb_helper_setcmap,
 };
 
 static int hibmc_drm_fb_create(struct drm_fb_helper *helper,
-- 
2.16.1

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

Re: [PATCH v4 1/1] drm/fb-helper: implement ioctl FBIO_WAITFORVSYNC

2017-03-05 Thread Stefan Lengfeld

Hi Maxime,

On Tue, Feb 28, 2017 at 04:36:51PM +0100, Maxime Ripard wrote:
> Implement legacy framebuffer ioctl FBIO_WAITFORVSYNC in the generic
> framebuffer emulation driver. Legacy framebuffer users like non kms/drm
> based OpenGL(ES)/EGL implementations may require the ioctl to
> synchronize drawing or buffer flip for double buffering. It is tested on
> the i.MX6.
> 
> Signed-off-by: Maxime Ripard <maxime.rip...@free-electrons.com>
> Tested-by: Neil Armstrong <narmstr...@baylibre.com>
> ---
>  drivers/gpu/drm/drm_fb_helper.c | 68 ++-
>  include/drm/drm_fb_helper.h | 12 +-
>  2 files changed, 79 insertions(+), 1 deletion(-)

I have tested the current HEAD of drm-misc-next. It includes this patch
and the patch ("drm/fb-helper: Add multi buffer support for cma fbdev").
On my i.MX6 board with HDMI output both patches work as advertised.
Multi-buffering with ioctls FBIO_WAITFORVSYNC and FBIOPAN_DISPLAY is
possible.

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

Re: [PATCH v4 1/1] drm/fb-helper: implement ioctl FBIO_WAITFORVSYNC

2017-03-01 Thread Stefan Lengfeld

Hi Maxime,

On Tue, Feb 28, 2017 at 04:36:51PM +0100, Maxime Ripard wrote:
> Implement legacy framebuffer ioctl FBIO_WAITFORVSYNC in the generic
> framebuffer emulation driver. Legacy framebuffer users like non kms/drm
> based OpenGL(ES)/EGL implementations may require the ioctl to 
>  
> synchronize drawing or buffer flip for double buffering. It is tested on
> the i.MX6.
>
> Signed-off-by: Maxime Ripard 
> Tested-by: Neil Armstrong 

I will test this patch and the already applied patch to drm-next on the
i.MX6 platform in the next days.

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

Re: [PATCH v3 2/2] drm/fb-helper: implement ioctl FBIO_WAITFORVSYNC

2017-02-23 Thread Stefan Lengfeld

Hi Maxime, Hi Ville,

On Wed, Feb 22, 2017 at 04:52:33PM -0800, Maxime Ripard wrote:
> Hi Ville, Stefan,
> 
> On Tue, Feb 21, 2017 at 12:55:01PM +0200, Ville Syrjälä wrote:
> > On Tue, Feb 21, 2017 at 11:00:59AM +0100, Stefan Lengfeld wrote:
> > > 
> > > Furthmore most exiting userspace code just passes the value "0" as the
> > > argument. For example DirectFB:
> > > 
> > >  static const int zero = 0;
> > >  [...]
> > >  if (ioctl( dfb_fbdev->fd, FBIO_WAITFORVSYNC,  ))
> > 
> > Again the matrox driver is different. And looks like unichrome might have
> > done something even more exotic with the argument.
> > 
> > But I do agree that using this with the kms fbdev implementation could
> > be quite problematic as you can't actually know which crtcs the
> > fb_helper has picked.
> 
> I'm not sure what a good solution might be then. Is just waiting for
> always the first crtc acceptable? All the code I could find is passing
> 0 as an argument anyway, so that's effectively the same behaviour
> anyway here. And getting tearing might be a good argument in favour of
> moving to KMS :)
> 

I also vote for the first solution: just waiting for the first enabled
crtc. It's deterministic and userspace can avoid tearing at least for
one crtc. If the system has more active scanouts at the same time with
different timings, userspace has to use the newer kms/drm API.

It is the same argument as already posted by Michel Dänzer:

 > 2/ Wait for a single vsync event on all active crtcs as Ville Syrjälä
 >described here [2] in the optimal implementation.

 FWIW, this seems like a bad idea, since with multiple active CRTCs it
 would make it essentially random at which intervals the ioctl returns,
 and which CRTCs are in vertical blank when it does. So it would be
 useless for both timing and tearing prevention purposes, i.e. more or
 less completely useless.

So I think the implementation (waiting only for the first crtc) of ioctl
FBIO_WAITFORVSYNC is a good enough implementation and works the most
already existing userspace code out there.

Kind regards,
Stefan Lengfeld

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

Re: [PATCH v3 2/2] drm/fb-helper: implement ioctl FBIO_WAITFORVSYNC

2017-02-21 Thread Stefan Lengfeld

Hi Maxime,

On Wed, Feb 15, 2017 at 05:19:09PM +0100, Maxime Ripard wrote:
> From: Stefan Christ <s.chr...@phytec.de>
> 

Maybe change the author here. Only the boilerplate code looks my original
patch. The real code is your work ;-)

> Implement legacy framebuffer ioctl FBIO_WAITFORVSYNC in the generic
> framebuffer emulation driver. Legacy framebuffer users like non kms/drm
> based OpenGL(ES)/EGL implementations may require the ioctl to
> synchronize drawing or buffer flip for double buffering. It is tested on
> the i.MX6.
> 
> Code is based on
> 
> https://github.com/Xilinx/linux-xlnx/blob/master/drivers/gpu/drm/xilinx/xilinx_drm_fb.c#L196
> 
> Signed-off-by: Stefan Christ <s.chr...@phytec.de>
> Signed-off-by: Maxime Ripard <maxime.rip...@free-electrons.com>
> ---
>  drivers/gpu/drm/drm_fb_helper.c | 63 ++-
>  include/drm/drm_fb_helper.h | 12 +-
>  2 files changed, 74 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index c6de87abaca8..15ee9641c725 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -1240,6 +1240,69 @@ int drm_fb_helper_setcmap(struct fb_cmap *cmap, struct 
> fb_info *info)
>  EXPORT_SYMBOL(drm_fb_helper_setcmap);
>  
>  /**
> + * drm_fb_helper_ioctl - legacy ioctl implementation
> + * @info: fbdev registered by the helper
> + * @cmd: ioctl command
> + * @arg: ioctl argument
> + *
> + * A helper to implement the standard fbdev ioctl. Only
> + * FBIO_WAITFORVSYNC is implemented for now.
> + */
> +int drm_fb_helper_ioctl(struct fb_info *info, unsigned int cmd,
> + unsigned long arg)
> +{
> + struct drm_fb_helper *fb_helper = info->par;
> + struct drm_device *dev = fb_helper->dev;
> + struct drm_mode_set *mode_set;
> + struct drm_crtc *crtc;
> + u32 karg;
> + int ret = 0;
> +
> + mutex_lock(>mode_config.mutex);
> + if (!drm_fb_helper_is_bound(fb_helper)) {
> + ret = -EBUSY;
> + goto unlock;
> + }
> +
> + switch (cmd) {
> + case FBIO_WAITFORVSYNC:
> + if (get_user(karg, (__u32 __user *)arg)) {
> + ret = -EFAULT;
> + goto unlock;
> + }
> +
> + if (karg >= fb_helper->crtc_count) {
> + ret = -EINVAL;
> + goto unlock;
> + }

Ville Syrjälä said [1]:

FBIO_WAITFORVSYNC takes the crtc as a parmeter, so I'm not sure we want
to do this for all the crtcs. Though what that crtc means for fb is
rather poorly defined.

Don't think it takes the crtc as a parameter in 'arg'. When you look at the
existing fb_ioctl implementations in the directory drivers/video/fbdev/, the
argument 'arg' is either ignored or must be '0'.

Furthmore most exiting userspace code just passes the value "0" as the
argument. For example DirectFB:

 static const int zero = 0;
 [...]
 if (ioctl( dfb_fbdev->fd, FBIO_WAITFORVSYNC,  ))

I see two options here: 

1/ Just wait for the first vsync event on the first enabled crtc. This is
   a good approximation for the old framebuffer API. It has no concept of
   multiple crtcs with concurrenctly running scan out processes. (Maybe apart
   from extra vendor implementations). So if there are really more than one 
active
   crtcs in the system, bad luck with framebuffer API. There will be tearing.
   Userspace has to use the new DRM/KMS API.

2/ Wait for a single vsync event on all active crtcs as Ville Syrjälä 
   described here [2] in the optimal implementation.

Kind regards / Mit freundlichen Grüßen
Stefan Lengfeld

[1] https://lists.freedesktop.org/archives/dri-devel/2017-February/132617.html
[2] https://lists.freedesktop.org/archives/dri-devel/2017-February/132820.html
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 1/2] drm/fb-helper: Add multi buffer support for cma fbdev

2017-02-20 Thread Stefan Lengfeld

Hi Maxime,

sorry, I have missed the discussion about the double buffering/virtual
surface size patch series two weeks ago. My comments about the patch are
inline:

On Wed, Feb 15, 2017 at 05:19:08PM +0100, Maxime Ripard wrote:
> From: Xinliang Liu <xinliang@linaro.org>

Mabye you should take the authorship here. Taking the credit and the
blame, because the patch was heavily modified by you and me. But I don't
really know what the offical police about that is.

> 
> This patch add a config to support to create multi buffer for cma fbdev.
> Such as double buffer and triple buffer.
> 
> Cma fbdev is convient to add a legency fbdev. And still many Android
> devices use fbdev now and at least double buffer is needed for these
> Android devices, so that a buffer flip can be operated. It will need
> some time for Android device vendors to abondon legency fbdev. So multi
> buffer for fbdev is needed.
> 
> Signed-off-by: Xinliang Liu <xinliang@linaro.org>
> [s.chr...@phytec.de: Picking patch from
>  https://lkml.org/lkml/2015/9/14/188]
> Signed-off-by: Stefan Christ <s.chr...@phytec.de>
> Signed-off-by: Maxime Ripard <maxime.rip...@free-electrons.com>

Reviewed-by: Stefan Lengfeld <cont...@stefanchrist.eu>

My surname has changed from "Christ" to "Lengfeld" recently. So my
review tag contains the new name.

> ---
>  drivers/gpu/drm/Kconfig |  9 +
>  drivers/gpu/drm/drm_fb_helper.c | 10 ++
>  2 files changed, 19 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index ebfe8404c25f..700c8b8e57a9 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -84,6 +84,15 @@ config DRM_FBDEV_EMULATION
>  
> If in doubt, say "Y".
>  
> +config DRM_FBDEV_OVERALLOC
> + int "Overallocation of the fbdev buffer"
> + depends on DRM_FBDEV_EMULATION
> + default 100
> + help
> +   Defines the fbdev buffer overallocation in percent. Default
> +   is 100. Typical values for double buffering will be 200,
> +   triple buffering 300.
> +
>  config DRM_LOAD_EDID_FIRMWARE
>   bool "Allow to specify an EDID data set instead of probing for it"
>   depends on DRM_KMS_HELPER
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index e934b541feea..c6de87abaca8 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -48,6 +48,12 @@ module_param_named(fbdev_emulation, drm_fbdev_emulation, 
> bool, 0600);
>  MODULE_PARM_DESC(fbdev_emulation,
>"Enable legacy fbdev emulation [default=true]");
>  
> +static int drm_fbdev_overalloc = CONFIG_DRM_FBDEV_OVERALLOC;
> +module_param(drm_fbdev_overalloc, int, 0444);

Maybe the variable should be of type "uint" instead of "int". This would
rule out the negative numbers error case.

> +MODULE_PARM_DESC(drm_fbdev_overalloc,
> +  "Overallocation of the fbdev buffer (%) [default="
> +  __MODULE_STRING(CONFIG_DRM_FBDEV_OVERALLOC) "]");
> +
>  static LIST_HEAD(kernel_fb_helper_list);
>  static DEFINE_MUTEX(kernel_fb_helper_lock);
>  
> @@ -1573,6 +1579,10 @@ static int drm_fb_helper_single_fb_probe(struct 
> drm_fb_helper *fb_helper,
>   sizes.fb_height = sizes.surface_height = 768;
>   }
>  
> + /* Handle our overallocation */
> + sizes.surface_height *= drm_fbdev_overalloc;
> + sizes.surface_height /= 100;
> +

The code can trigger an arithmetic overflow, but I think we can ignore
this error case here.

But there should be a check for drm_fbdev_overalloc not be smaller than
100. If it is smaller, the variable drm_fbdev_overalloc should have the
default value "100". Otherwise the virtual surface height can be smaller
than the physical height. This could trigger a lot of errors in existing
code paths.

Kind regards,
Stefan Lengfeld

>   /* push down into drivers */
>   ret = (*fb_helper->funcs->fb_probe)(fb_helper, );
>   if (ret < 0)
> -- 
> git-series 0.8.11
> ___
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

2016-11-09 Thread Stefan Lengfeld

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
> >  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 
> > >> > Signed-off-by: Stefan Christ 
> > >> > ---
> > >> >  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

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

2016-10-26 Thread Stefan Lengfeld

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 
> > Signed-off-by: Stefan Christ 
> > ---
> >  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
>

[PATCH 1/3] drm/ast: use DRM_FB_HELPER_DEFAULT_OPS for fb_ops

[PATCH 0/3] Final round for DRM_FB_HELPER_DEFAULT_OPS usage

[PATCH 3/3] drm/mgag200: use DRM_FB_HELPER_DEFAULT_OPS for fb_ops

[PATCH 2/3] drm/hisilicon: use DRM_FB_HELPER_DEFAULT_OPS for fb_ops

Re: [PATCH v4 1/1] drm/fb-helper: implement ioctl FBIO_WAITFORVSYNC

Re: [PATCH v4 1/1] drm/fb-helper: implement ioctl FBIO_WAITFORVSYNC

Re: [PATCH v3 2/2] drm/fb-helper: implement ioctl FBIO_WAITFORVSYNC

Re: [PATCH v3 2/2] drm/fb-helper: implement ioctl FBIO_WAITFORVSYNC

Re: [PATCH v3 1/2] drm/fb-helper: Add multi buffer support for cma fbdev

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

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

11 matches

Site Navigation

Mail list logo

Footer information