[PATCH 2/8] drm/atmel: ditch fb_create wrapper

[PATCH 2/8] drm/atmel: ditch fb_create wrapper

[Daniel Vetter]

Spotted while looking through them all.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: Boris Brezillon <bbrezillon@kernel.org>
Cc: Nicolas Ferre <nicolas.ferre@microchip.com>
Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
Cc: Ludovic Desroches <ludovic.desroches@microchip.com>
Cc: linux-arm-kernel@lists.infradead.org
---
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
index 92640298ad41..8dc917a1270b 100644
--- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
+++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
@@ -557,12 +557,6 @@ static irqreturn_t atmel_hlcdc_dc_irq_handler(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
-static struct drm_framebuffer *atmel_hlcdc_fb_create(struct drm_device *dev,
-		struct drm_file *file_priv, const struct drm_mode_fb_cmd2 *mode_cmd)
-{
-	return drm_gem_fb_create(dev, file_priv, mode_cmd);
-}
-
 struct atmel_hlcdc_dc_commit {
 	struct work_struct work;
 	struct drm_device *dev;
@@ -657,7 +651,7 @@ static int atmel_hlcdc_dc_atomic_commit(struct drm_device *dev,
 }
 
 static const struct drm_mode_config_funcs mode_config_funcs = {
-	.fb_create = atmel_hlcdc_fb_create,
+	.fb_create = drm_gem_fb_create,
 	.atomic_check = drm_atomic_helper_check,
 	.atomic_commit = atmel_hlcdc_dc_atomic_commit,
 };
-- 
2.24.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/8] drm/atmel: ditch fb_create wrapper

[Boris Brezillon]

On Fri, 15 Nov 2019 10:21:14 +0100
Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> Spotted while looking through them all.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Sam Ravnborg <sam@ravnborg.org>
> Cc: Boris Brezillon <bbrezillon@kernel.org>

Acked-by: Boris Brezillon <boris.brezillon@collabora.com>

> Cc: Nicolas Ferre <nicolas.ferre@microchip.com>
> Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
> Cc: Ludovic Desroches <ludovic.desroches@microchip.com>
> Cc: linux-arm-kernel@lists.infradead.org
> ---
>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 8 +-------
>  1 file changed, 1 insertion(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> index 92640298ad41..8dc917a1270b 100644
> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> @@ -557,12 +557,6 @@ static irqreturn_t atmel_hlcdc_dc_irq_handler(int irq, void *data)
>  	return IRQ_HANDLED;
>  }
>  
> -static struct drm_framebuffer *atmel_hlcdc_fb_create(struct drm_device *dev,
> -		struct drm_file *file_priv, const struct drm_mode_fb_cmd2 *mode_cmd)
> -{
> -	return drm_gem_fb_create(dev, file_priv, mode_cmd);
> -}
> -
>  struct atmel_hlcdc_dc_commit {
>  	struct work_struct work;
>  	struct drm_device *dev;
> @@ -657,7 +651,7 @@ static int atmel_hlcdc_dc_atomic_commit(struct drm_device *dev,
>  }
>  
>  static const struct drm_mode_config_funcs mode_config_funcs = {
> -	.fb_create = atmel_hlcdc_fb_create,
> +	.fb_create = drm_gem_fb_create,
>  	.atomic_check = drm_atomic_helper_check,
>  	.atomic_commit = atmel_hlcdc_dc_atomic_commit,
>  };


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/8] drm/atmel: ditch fb_create wrapper

[Daniel Vetter]

On Fri, Nov 15, 2019 at 10:33:24AM +0100, Boris Brezillon wrote:
> On Fri, 15 Nov 2019 10:21:14 +0100
> Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> 
> > Spotted while looking through them all.
> > 
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > Cc: Sam Ravnborg <sam@ravnborg.org>
> > Cc: Boris Brezillon <bbrezillon@kernel.org>
> 
> Acked-by: Boris Brezillon <boris.brezillon@collabora.com>

Merged, thanks for taking a look.
-Daniel

> 
> > Cc: Nicolas Ferre <nicolas.ferre@microchip.com>
> > Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
> > Cc: Ludovic Desroches <ludovic.desroches@microchip.com>
> > Cc: linux-arm-kernel@lists.infradead.org
> > ---
> >  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 8 +-------
> >  1 file changed, 1 insertion(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> > index 92640298ad41..8dc917a1270b 100644
> > --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> > +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> > @@ -557,12 +557,6 @@ static irqreturn_t atmel_hlcdc_dc_irq_handler(int irq, void *data)
> >  	return IRQ_HANDLED;
> >  }
> >  
> > -static struct drm_framebuffer *atmel_hlcdc_fb_create(struct drm_device *dev,
> > -		struct drm_file *file_priv, const struct drm_mode_fb_cmd2 *mode_cmd)
> > -{
> > -	return drm_gem_fb_create(dev, file_priv, mode_cmd);
> > -}
> > -
> >  struct atmel_hlcdc_dc_commit {
> >  	struct work_struct work;
> >  	struct drm_device *dev;
> > @@ -657,7 +651,7 @@ static int atmel_hlcdc_dc_atomic_commit(struct drm_device *dev,
> >  }
> >  
> >  static const struct drm_mode_config_funcs mode_config_funcs = {
> > -	.fb_create = atmel_hlcdc_fb_create,
> > +	.fb_create = drm_gem_fb_create,
> >  	.atomic_check = drm_atomic_helper_check,
> >  	.atomic_commit = atmel_hlcdc_dc_atomic_commit,
> >  };
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/8] drm/atmel: ditch fb_create wrapper

[Sam Ravnborg]

Hi Daniel.
On Tue, Nov 19, 2019 at 10:22:31PM +0100, Daniel Vetter wrote:
> On Fri, Nov 15, 2019 at 10:33:24AM +0100, Boris Brezillon wrote:
> > On Fri, 15 Nov 2019 10:21:14 +0100
> > Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > 
> > > Spotted while looking through them all.
> > > 
> > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > > Cc: Sam Ravnborg <sam@ravnborg.org>
> > > Cc: Boris Brezillon <bbrezillon@kernel.org>
> > 
> > Acked-by: Boris Brezillon <boris.brezillon@collabora.com>
> 
> Merged, thanks for taking a look.

Hi Daniel, thanks for merging this.

	Sam

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/8] drm/mediatek: don't open-code drm_gem_fb_create

[Daniel Vetter]

Aside: There's a few other fb_create implementations which
simply check for valid buffer format (or an approximation thereof),
and then call drm_gem_fb_create. For atomic drivers at least we could
walk all planes and make sure the format/modifier combo is valid,
and remove even more code.

For non-atomic drivers that's not possible, since the format list for
the primary buffer might be garbage (and most likely it is).

Also delete mtk_drm_fb.[hc] since it would now only contain one
function.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: CK Hu <ck.hu@mediatek.com>
Cc: Philipp Zabel <p.zabel@pengutronix.de>
Cc: Matthias Brugger <matthias.bgg@gmail.com>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-mediatek@lists.infradead.org
---
 drivers/gpu/drm/mediatek/mtk_drm_drv.c   | 16 ++++-
 drivers/gpu/drm/mediatek/mtk_drm_fb.c    | 92 ------------------------
 drivers/gpu/drm/mediatek/mtk_drm_fb.h    | 13 ----
 drivers/gpu/drm/mediatek/mtk_drm_plane.c |  1 -
 4 files changed, 15 insertions(+), 107 deletions(-)
 delete mode 100644 drivers/gpu/drm/mediatek/mtk_drm_fb.c
 delete mode 100644 drivers/gpu/drm/mediatek/mtk_drm_fb.h

diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
index 84d14213d992..2b1c122066ea 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
@@ -16,8 +16,10 @@
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_drv.h>
 #include <drm/drm_fb_helper.h>
+#include <drm/drm_fourcc.h>
 #include <drm/drm_gem.h>
 #include <drm/drm_gem_cma_helper.h>
+#include <drm/drm_gem_framebuffer_helper.h>
 #include <drm/drm_of.h>
 #include <drm/drm_probe_helper.h>
 #include <drm/drm_vblank.h>
@@ -27,7 +29,6 @@
 #include "mtk_drm_ddp.h"
 #include "mtk_drm_ddp_comp.h"
 #include "mtk_drm_drv.h"
-#include "mtk_drm_fb.h"
 #include "mtk_drm_gem.h"
 
 #define DRIVER_NAME "mediatek"
@@ -115,6 +116,19 @@ static int mtk_atomic_commit(struct drm_device *drm,
 	return 0;
 }
 
+static struct drm_framebuffer *
+mtk_drm_mode_fb_create(struct drm_device *dev,
+		       struct drm_file *file,
+		       const struct drm_mode_fb_cmd2 *cmd)
+{
+	const struct drm_format_info *info = drm_get_format_info(dev, cmd);
+
+	if (info->num_planes != 1)
+		return ERR_PTR(-EINVAL);
+
+	return drm_gem_fb_create(dev, file, cmd);
+}
+
 static const struct drm_mode_config_funcs mtk_drm_mode_config_funcs = {
 	.fb_create = mtk_drm_mode_fb_create,
 	.atomic_check = drm_atomic_helper_check,
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_fb.c b/drivers/gpu/drm/mediatek/mtk_drm_fb.c
deleted file mode 100644
index 3f230a28a2dc..000000000000
--- a/drivers/gpu/drm/mediatek/mtk_drm_fb.c
+++ /dev/null
@@ -1,92 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0-only
-/*
- * Copyright (c) 2015 MediaTek Inc.
- */
-
-#include <linux/dma-buf.h>
-#include <linux/dma-resv.h>
-
-#include <drm/drm_modeset_helper.h>
-#include <drm/drm_fb_helper.h>
-#include <drm/drm_fourcc.h>
-#include <drm/drm_gem.h>
-#include <drm/drm_gem_framebuffer_helper.h>
-
-#include "mtk_drm_drv.h"
-#include "mtk_drm_fb.h"
-#include "mtk_drm_gem.h"
-
-static const struct drm_framebuffer_funcs mtk_drm_fb_funcs = {
-	.create_handle = drm_gem_fb_create_handle,
-	.destroy = drm_gem_fb_destroy,
-};
-
-static struct drm_framebuffer *mtk_drm_framebuffer_init(struct drm_device *dev,
-					const struct drm_mode_fb_cmd2 *mode,
-					struct drm_gem_object *obj)
-{
-	const struct drm_format_info *info = drm_get_format_info(dev, mode);
-	struct drm_framebuffer *fb;
-	int ret;
-
-	if (info->num_planes != 1)
-		return ERR_PTR(-EINVAL);
-
-	fb = kzalloc(sizeof(*fb), GFP_KERNEL);
-	if (!fb)
-		return ERR_PTR(-ENOMEM);
-
-	drm_helper_mode_fill_fb_struct(dev, fb, mode);
-
-	fb->obj[0] = obj;
-
-	ret = drm_framebuffer_init(dev, fb, &mtk_drm_fb_funcs);
-	if (ret) {
-		DRM_ERROR("failed to initialize framebuffer\n");
-		kfree(fb);
-		return ERR_PTR(ret);
-	}
-
-	return fb;
-}
-
-struct drm_framebuffer *mtk_drm_mode_fb_create(struct drm_device *dev,
-					       struct drm_file *file,
-					       const struct drm_mode_fb_cmd2 *cmd)
-{
-	const struct drm_format_info *info = drm_get_format_info(dev, cmd);
-	struct drm_framebuffer *fb;
-	struct drm_gem_object *gem;
-	unsigned int width = cmd->width;
-	unsigned int height = cmd->height;
-	unsigned int size, bpp;
-	int ret;
-
-	if (info->num_planes != 1)
-		return ERR_PTR(-EINVAL);
-
-	gem = drm_gem_object_lookup(file, cmd->handles[0]);
-	if (!gem)
-		return ERR_PTR(-ENOENT);
-
-	bpp = info->cpp[0];
-	size = (height - 1) * cmd->pitches[0] + width * bpp;
-	size += cmd->offsets[0];
-
-	if (gem->size < size) {
-		ret = -EINVAL;
-		goto unreference;
-	}
-
-	fb = mtk_drm_framebuffer_init(dev, cmd, gem);
-	if (IS_ERR(fb)) {
-		ret = PTR_ERR(fb);
-		goto unreference;
-	}
-
-	return fb;
-
-unreference:
-	drm_gem_object_put_unlocked(gem);
-	return ERR_PTR(ret);
-}
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_fb.h b/drivers/gpu/drm/mediatek/mtk_drm_fb.h
deleted file mode 100644
index eb64d26001c6..000000000000
--- a/drivers/gpu/drm/mediatek/mtk_drm_fb.h
+++ /dev/null
@@ -1,13 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0-only */
-/*
- * Copyright (c) 2015 MediaTek Inc.
- */
-
-#ifndef MTK_DRM_FB_H
-#define MTK_DRM_FB_H
-
-struct drm_framebuffer *mtk_drm_mode_fb_create(struct drm_device *dev,
-					       struct drm_file *file,
-					       const struct drm_mode_fb_cmd2 *cmd);
-
-#endif /* MTK_DRM_FB_H */
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_plane.c b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
index 3b0cc91c7023..540ef2faa40a 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_plane.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
@@ -13,7 +13,6 @@
 #include "mtk_drm_crtc.h"
 #include "mtk_drm_ddp_comp.h"
 #include "mtk_drm_drv.h"
-#include "mtk_drm_fb.h"
 #include "mtk_drm_gem.h"
 #include "mtk_drm_plane.h"
 
-- 
2.24.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/8] drm/mediatek: don't open-code drm_gem_fb_create

[CK Hu]

Hi, Daniel:

On Fri, 2019-11-15 at 10:21 +0100, Daniel Vetter wrote:
> Aside: There's a few other fb_create implementations which
> simply check for valid buffer format (or an approximation thereof),
> and then call drm_gem_fb_create. For atomic drivers at least we could
> walk all planes and make sure the format/modifier combo is valid,
> and remove even more code.
> 
> For non-atomic drivers that's not possible, since the format list for
> the primary buffer might be garbage (and most likely it is).
> 
> Also delete mtk_drm_fb.[hc] since it would now only contain one
> function.

Acked-by: CK Hu <ck.hu@mediatek.com>

> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: CK Hu <ck.hu@mediatek.com>
> Cc: Philipp Zabel <p.zabel@pengutronix.de>
> Cc: Matthias Brugger <matthias.bgg@gmail.com>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-mediatek@lists.infradead.org
> ---
>  drivers/gpu/drm/mediatek/mtk_drm_drv.c   | 16 ++++-
>  drivers/gpu/drm/mediatek/mtk_drm_fb.c    | 92 ------------------------
>  drivers/gpu/drm/mediatek/mtk_drm_fb.h    | 13 ----
>  drivers/gpu/drm/mediatek/mtk_drm_plane.c |  1 -
>  4 files changed, 15 insertions(+), 107 deletions(-)
>  delete mode 100644 drivers/gpu/drm/mediatek/mtk_drm_fb.c
>  delete mode 100644 drivers/gpu/drm/mediatek/mtk_drm_fb.h
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> index 84d14213d992..2b1c122066ea 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> @@ -16,8 +16,10 @@
>  #include <drm/drm_atomic_helper.h>
>  #include <drm/drm_drv.h>
>  #include <drm/drm_fb_helper.h>
> +#include <drm/drm_fourcc.h>
>  #include <drm/drm_gem.h>
>  #include <drm/drm_gem_cma_helper.h>
> +#include <drm/drm_gem_framebuffer_helper.h>
>  #include <drm/drm_of.h>
>  #include <drm/drm_probe_helper.h>
>  #include <drm/drm_vblank.h>
> @@ -27,7 +29,6 @@
>  #include "mtk_drm_ddp.h"
>  #include "mtk_drm_ddp_comp.h"
>  #include "mtk_drm_drv.h"
> -#include "mtk_drm_fb.h"
>  #include "mtk_drm_gem.h"
>  
>  #define DRIVER_NAME "mediatek"
> @@ -115,6 +116,19 @@ static int mtk_atomic_commit(struct drm_device *drm,
>  	return 0;
>  }
>  
> +static struct drm_framebuffer *
> +mtk_drm_mode_fb_create(struct drm_device *dev,
> +		       struct drm_file *file,
> +		       const struct drm_mode_fb_cmd2 *cmd)
> +{
> +	const struct drm_format_info *info = drm_get_format_info(dev, cmd);
> +
> +	if (info->num_planes != 1)
> +		return ERR_PTR(-EINVAL);
> +
> +	return drm_gem_fb_create(dev, file, cmd);
> +}
> +
>  static const struct drm_mode_config_funcs mtk_drm_mode_config_funcs = {
>  	.fb_create = mtk_drm_mode_fb_create,
>  	.atomic_check = drm_atomic_helper_check,
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_fb.c b/drivers/gpu/drm/mediatek/mtk_drm_fb.c
> deleted file mode 100644
> index 3f230a28a2dc..000000000000
> --- a/drivers/gpu/drm/mediatek/mtk_drm_fb.c
> +++ /dev/null
> @@ -1,92 +0,0 @@
> -// SPDX-License-Identifier: GPL-2.0-only
> -/*
> - * Copyright (c) 2015 MediaTek Inc.
> - */
> -
> -#include <linux/dma-buf.h>
> -#include <linux/dma-resv.h>
> -
> -#include <drm/drm_modeset_helper.h>
> -#include <drm/drm_fb_helper.h>
> -#include <drm/drm_fourcc.h>
> -#include <drm/drm_gem.h>
> -#include <drm/drm_gem_framebuffer_helper.h>
> -
> -#include "mtk_drm_drv.h"
> -#include "mtk_drm_fb.h"
> -#include "mtk_drm_gem.h"
> -
> -static const struct drm_framebuffer_funcs mtk_drm_fb_funcs = {
> -	.create_handle = drm_gem_fb_create_handle,
> -	.destroy = drm_gem_fb_destroy,
> -};
> -
> -static struct drm_framebuffer *mtk_drm_framebuffer_init(struct drm_device *dev,
> -					const struct drm_mode_fb_cmd2 *mode,
> -					struct drm_gem_object *obj)
> -{
> -	const struct drm_format_info *info = drm_get_format_info(dev, mode);
> -	struct drm_framebuffer *fb;
> -	int ret;
> -
> -	if (info->num_planes != 1)
> -		return ERR_PTR(-EINVAL);
> -
> -	fb = kzalloc(sizeof(*fb), GFP_KERNEL);
> -	if (!fb)
> -		return ERR_PTR(-ENOMEM);
> -
> -	drm_helper_mode_fill_fb_struct(dev, fb, mode);
> -
> -	fb->obj[0] = obj;
> -
> -	ret = drm_framebuffer_init(dev, fb, &mtk_drm_fb_funcs);
> -	if (ret) {
> -		DRM_ERROR("failed to initialize framebuffer\n");
> -		kfree(fb);
> -		return ERR_PTR(ret);
> -	}
> -
> -	return fb;
> -}
> -
> -struct drm_framebuffer *mtk_drm_mode_fb_create(struct drm_device *dev,
> -					       struct drm_file *file,
> -					       const struct drm_mode_fb_cmd2 *cmd)
> -{
> -	const struct drm_format_info *info = drm_get_format_info(dev, cmd);
> -	struct drm_framebuffer *fb;
> -	struct drm_gem_object *gem;
> -	unsigned int width = cmd->width;
> -	unsigned int height = cmd->height;
> -	unsigned int size, bpp;
> -	int ret;
> -
> -	if (info->num_planes != 1)
> -		return ERR_PTR(-EINVAL);
> -
> -	gem = drm_gem_object_lookup(file, cmd->handles[0]);
> -	if (!gem)
> -		return ERR_PTR(-ENOENT);
> -
> -	bpp = info->cpp[0];
> -	size = (height - 1) * cmd->pitches[0] + width * bpp;
> -	size += cmd->offsets[0];
> -
> -	if (gem->size < size) {
> -		ret = -EINVAL;
> -		goto unreference;
> -	}
> -
> -	fb = mtk_drm_framebuffer_init(dev, cmd, gem);
> -	if (IS_ERR(fb)) {
> -		ret = PTR_ERR(fb);
> -		goto unreference;
> -	}
> -
> -	return fb;
> -
> -unreference:
> -	drm_gem_object_put_unlocked(gem);
> -	return ERR_PTR(ret);
> -}
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_fb.h b/drivers/gpu/drm/mediatek/mtk_drm_fb.h
> deleted file mode 100644
> index eb64d26001c6..000000000000
> --- a/drivers/gpu/drm/mediatek/mtk_drm_fb.h
> +++ /dev/null
> @@ -1,13 +0,0 @@
> -/* SPDX-License-Identifier: GPL-2.0-only */
> -/*
> - * Copyright (c) 2015 MediaTek Inc.
> - */
> -
> -#ifndef MTK_DRM_FB_H
> -#define MTK_DRM_FB_H
> -
> -struct drm_framebuffer *mtk_drm_mode_fb_create(struct drm_device *dev,
> -					       struct drm_file *file,
> -					       const struct drm_mode_fb_cmd2 *cmd);
> -
> -#endif /* MTK_DRM_FB_H */
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_plane.c b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> index 3b0cc91c7023..540ef2faa40a 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> @@ -13,7 +13,6 @@
>  #include "mtk_drm_crtc.h"
>  #include "mtk_drm_ddp_comp.h"
>  #include "mtk_drm_drv.h"
> -#include "mtk_drm_fb.h"
>  #include "mtk_drm_gem.h"
>  #include "mtk_drm_plane.h"
>  

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/8] drm/mediatek: don't open-code drm_gem_fb_create

[Daniel Vetter]

On Fri, Nov 22, 2019 at 03:42:39PM +0800, CK Hu wrote:
> Hi, Daniel:
> 
> On Fri, 2019-11-15 at 10:21 +0100, Daniel Vetter wrote:
> > Aside: There's a few other fb_create implementations which
> > simply check for valid buffer format (or an approximation thereof),
> > and then call drm_gem_fb_create. For atomic drivers at least we could
> > walk all planes and make sure the format/modifier combo is valid,
> > and remove even more code.
> > 
> > For non-atomic drivers that's not possible, since the format list for
> > the primary buffer might be garbage (and most likely it is).
> > 
> > Also delete mtk_drm_fb.[hc] since it would now only contain one
> > function.
> 
> Acked-by: CK Hu <ck.hu@mediatek.com>

Pushed to drm-misc-next, thanks for taking a look.
-Daniel

> 
> > 
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: CK Hu <ck.hu@mediatek.com>
> > Cc: Philipp Zabel <p.zabel@pengutronix.de>
> > Cc: Matthias Brugger <matthias.bgg@gmail.com>
> > Cc: linux-arm-kernel@lists.infradead.org
> > Cc: linux-mediatek@lists.infradead.org
> > ---
> >  drivers/gpu/drm/mediatek/mtk_drm_drv.c   | 16 ++++-
> >  drivers/gpu/drm/mediatek/mtk_drm_fb.c    | 92 ------------------------
> >  drivers/gpu/drm/mediatek/mtk_drm_fb.h    | 13 ----
> >  drivers/gpu/drm/mediatek/mtk_drm_plane.c |  1 -
> >  4 files changed, 15 insertions(+), 107 deletions(-)
> >  delete mode 100644 drivers/gpu/drm/mediatek/mtk_drm_fb.c
> >  delete mode 100644 drivers/gpu/drm/mediatek/mtk_drm_fb.h
> > 
> > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> > index 84d14213d992..2b1c122066ea 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> > @@ -16,8 +16,10 @@
> >  #include <drm/drm_atomic_helper.h>
> >  #include <drm/drm_drv.h>
> >  #include <drm/drm_fb_helper.h>
> > +#include <drm/drm_fourcc.h>
> >  #include <drm/drm_gem.h>
> >  #include <drm/drm_gem_cma_helper.h>
> > +#include <drm/drm_gem_framebuffer_helper.h>
> >  #include <drm/drm_of.h>
> >  #include <drm/drm_probe_helper.h>
> >  #include <drm/drm_vblank.h>
> > @@ -27,7 +29,6 @@
> >  #include "mtk_drm_ddp.h"
> >  #include "mtk_drm_ddp_comp.h"
> >  #include "mtk_drm_drv.h"
> > -#include "mtk_drm_fb.h"
> >  #include "mtk_drm_gem.h"
> >  
> >  #define DRIVER_NAME "mediatek"
> > @@ -115,6 +116,19 @@ static int mtk_atomic_commit(struct drm_device *drm,
> >  	return 0;
> >  }
> >  
> > +static struct drm_framebuffer *
> > +mtk_drm_mode_fb_create(struct drm_device *dev,
> > +		       struct drm_file *file,
> > +		       const struct drm_mode_fb_cmd2 *cmd)
> > +{
> > +	const struct drm_format_info *info = drm_get_format_info(dev, cmd);
> > +
> > +	if (info->num_planes != 1)
> > +		return ERR_PTR(-EINVAL);
> > +
> > +	return drm_gem_fb_create(dev, file, cmd);
> > +}
> > +
> >  static const struct drm_mode_config_funcs mtk_drm_mode_config_funcs = {
> >  	.fb_create = mtk_drm_mode_fb_create,
> >  	.atomic_check = drm_atomic_helper_check,
> > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_fb.c b/drivers/gpu/drm/mediatek/mtk_drm_fb.c
> > deleted file mode 100644
> > index 3f230a28a2dc..000000000000
> > --- a/drivers/gpu/drm/mediatek/mtk_drm_fb.c
> > +++ /dev/null
> > @@ -1,92 +0,0 @@
> > -// SPDX-License-Identifier: GPL-2.0-only
> > -/*
> > - * Copyright (c) 2015 MediaTek Inc.
> > - */
> > -
> > -#include <linux/dma-buf.h>
> > -#include <linux/dma-resv.h>
> > -
> > -#include <drm/drm_modeset_helper.h>
> > -#include <drm/drm_fb_helper.h>
> > -#include <drm/drm_fourcc.h>
> > -#include <drm/drm_gem.h>
> > -#include <drm/drm_gem_framebuffer_helper.h>
> > -
> > -#include "mtk_drm_drv.h"
> > -#include "mtk_drm_fb.h"
> > -#include "mtk_drm_gem.h"
> > -
> > -static const struct drm_framebuffer_funcs mtk_drm_fb_funcs = {
> > -	.create_handle = drm_gem_fb_create_handle,
> > -	.destroy = drm_gem_fb_destroy,
> > -};
> > -
> > -static struct drm_framebuffer *mtk_drm_framebuffer_init(struct drm_device *dev,
> > -					const struct drm_mode_fb_cmd2 *mode,
> > -					struct drm_gem_object *obj)
> > -{
> > -	const struct drm_format_info *info = drm_get_format_info(dev, mode);
> > -	struct drm_framebuffer *fb;
> > -	int ret;
> > -
> > -	if (info->num_planes != 1)
> > -		return ERR_PTR(-EINVAL);
> > -
> > -	fb = kzalloc(sizeof(*fb), GFP_KERNEL);
> > -	if (!fb)
> > -		return ERR_PTR(-ENOMEM);
> > -
> > -	drm_helper_mode_fill_fb_struct(dev, fb, mode);
> > -
> > -	fb->obj[0] = obj;
> > -
> > -	ret = drm_framebuffer_init(dev, fb, &mtk_drm_fb_funcs);
> > -	if (ret) {
> > -		DRM_ERROR("failed to initialize framebuffer\n");
> > -		kfree(fb);
> > -		return ERR_PTR(ret);
> > -	}
> > -
> > -	return fb;
> > -}
> > -
> > -struct drm_framebuffer *mtk_drm_mode_fb_create(struct drm_device *dev,
> > -					       struct drm_file *file,
> > -					       const struct drm_mode_fb_cmd2 *cmd)
> > -{
> > -	const struct drm_format_info *info = drm_get_format_info(dev, cmd);
> > -	struct drm_framebuffer *fb;
> > -	struct drm_gem_object *gem;
> > -	unsigned int width = cmd->width;
> > -	unsigned int height = cmd->height;
> > -	unsigned int size, bpp;
> > -	int ret;
> > -
> > -	if (info->num_planes != 1)
> > -		return ERR_PTR(-EINVAL);
> > -
> > -	gem = drm_gem_object_lookup(file, cmd->handles[0]);
> > -	if (!gem)
> > -		return ERR_PTR(-ENOENT);
> > -
> > -	bpp = info->cpp[0];
> > -	size = (height - 1) * cmd->pitches[0] + width * bpp;
> > -	size += cmd->offsets[0];
> > -
> > -	if (gem->size < size) {
> > -		ret = -EINVAL;
> > -		goto unreference;
> > -	}
> > -
> > -	fb = mtk_drm_framebuffer_init(dev, cmd, gem);
> > -	if (IS_ERR(fb)) {
> > -		ret = PTR_ERR(fb);
> > -		goto unreference;
> > -	}
> > -
> > -	return fb;
> > -
> > -unreference:
> > -	drm_gem_object_put_unlocked(gem);
> > -	return ERR_PTR(ret);
> > -}
> > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_fb.h b/drivers/gpu/drm/mediatek/mtk_drm_fb.h
> > deleted file mode 100644
> > index eb64d26001c6..000000000000
> > --- a/drivers/gpu/drm/mediatek/mtk_drm_fb.h
> > +++ /dev/null
> > @@ -1,13 +0,0 @@
> > -/* SPDX-License-Identifier: GPL-2.0-only */
> > -/*
> > - * Copyright (c) 2015 MediaTek Inc.
> > - */
> > -
> > -#ifndef MTK_DRM_FB_H
> > -#define MTK_DRM_FB_H
> > -
> > -struct drm_framebuffer *mtk_drm_mode_fb_create(struct drm_device *dev,
> > -					       struct drm_file *file,
> > -					       const struct drm_mode_fb_cmd2 *cmd);
> > -
> > -#endif /* MTK_DRM_FB_H */
> > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_plane.c b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> > index 3b0cc91c7023..540ef2faa40a 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> > @@ -13,7 +13,6 @@
> >  #include "mtk_drm_crtc.h"
> >  #include "mtk_drm_ddp_comp.h"
> >  #include "mtk_drm_drv.h"
> > -#include "mtk_drm_fb.h"
> >  #include "mtk_drm_gem.h"
> >  #include "mtk_drm_plane.h"
> >  
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 4/8] drm/rockchip: Use drm_gem_fb_create_with_dirty

[Daniel Vetter]

If rockchip would switch over to the generic fbdev setup we could
grabage collect even more of all this code (all of the remaining fb
handling code really).

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Sandy Huang <hjc@rock-chips.com>
Cc: "Heiko Stübner" <heiko@sntech.de>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-rockchip@lists.infradead.org
---
 drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 54 +---------------------
 1 file changed, 1 insertion(+), 53 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
index ca01234c037c..081dbdaa0b07 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
@@ -53,64 +53,12 @@ rockchip_fb_alloc(struct drm_device *dev, const struct drm_mode_fb_cmd2 *mode_cm
 	return fb;
 }
 
-static struct drm_framebuffer *
-rockchip_user_fb_create(struct drm_device *dev, struct drm_file *file_priv,
-			const struct drm_mode_fb_cmd2 *mode_cmd)
-{
-	const struct drm_format_info *info = drm_get_format_info(dev,
-								 mode_cmd);
-	struct drm_framebuffer *fb;
-	struct drm_gem_object *objs[ROCKCHIP_MAX_FB_BUFFER];
-	struct drm_gem_object *obj;
-	int num_planes = min_t(int, info->num_planes, ROCKCHIP_MAX_FB_BUFFER);
-	int ret;
-	int i;
-
-	for (i = 0; i < num_planes; i++) {
-		unsigned int width = mode_cmd->width / (i ? info->hsub : 1);
-		unsigned int height = mode_cmd->height / (i ? info->vsub : 1);
-		unsigned int min_size;
-
-		obj = drm_gem_object_lookup(file_priv, mode_cmd->handles[i]);
-		if (!obj) {
-			DRM_DEV_ERROR(dev->dev,
-				      "Failed to lookup GEM object\n");
-			ret = -ENXIO;
-			goto err_gem_object_unreference;
-		}
-
-		min_size = (height - 1) * mode_cmd->pitches[i] +
-			mode_cmd->offsets[i] +
-			width * info->cpp[i];
-
-		if (obj->size < min_size) {
-			drm_gem_object_put_unlocked(obj);
-			ret = -EINVAL;
-			goto err_gem_object_unreference;
-		}
-		objs[i] = obj;
-	}
-
-	fb = rockchip_fb_alloc(dev, mode_cmd, objs, i);
-	if (IS_ERR(fb)) {
-		ret = PTR_ERR(fb);
-		goto err_gem_object_unreference;
-	}
-
-	return fb;
-
-err_gem_object_unreference:
-	for (i--; i >= 0; i--)
-		drm_gem_object_put_unlocked(objs[i]);
-	return ERR_PTR(ret);
-}
-
 static const struct drm_mode_config_helper_funcs rockchip_mode_config_helpers = {
 	.atomic_commit_tail = drm_atomic_helper_commit_tail_rpm,
 };
 
 static const struct drm_mode_config_funcs rockchip_drm_mode_config_funcs = {
-	.fb_create = rockchip_user_fb_create,
+	.fb_create = drm_gem_fb_create,
 	.output_poll_changed = drm_fb_helper_output_poll_changed,
 	.atomic_check = drm_atomic_helper_check,
 	.atomic_commit = drm_atomic_helper_commit,
-- 
2.24.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 4/8] drm/rockchip: Use drm_gem_fb_create_with_dirty

[Andrzej Pietrasiewicz]

Hi Daniel,

W dniu 15.11.2019 o 10:21, Daniel Vetter pisze:
> If rockchip would switch over to the generic fbdev setup we could
> grabage collect even more of all this code (all of the remaining fb
> handling code really).
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Sandy Huang <hjc@rock-chips.com>
> Cc: "Heiko Stübner" <heiko@sntech.de>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-rockchip@lists.infradead.org

I carried out limited testing with modetest on a rockpi4,
using this command:

for i in `./modetest -c | grep ^[[:space:]]*[1-9][0-9]*x[1-9][0-9]* | cut -f3 
-d" " | grep -v i$ | uniq`; do ./modetest -s41:$i -C; done

All modes (excluding those whose names end with an "i", e.g. 1920x1080i)
produced sensible output which seems no different to what is produced
when the patch in question is not applied.

If such a test scope is acceptable, you can add my

Tested-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com>

> ---
>   drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 54 +---------------------
>   1 file changed, 1 insertion(+), 53 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> index ca01234c037c..081dbdaa0b07 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> @@ -53,64 +53,12 @@ rockchip_fb_alloc(struct drm_device *dev, const struct drm_mode_fb_cmd2 *mode_cm
>   	return fb;
>   }
>   
> -static struct drm_framebuffer *
> -rockchip_user_fb_create(struct drm_device *dev, struct drm_file *file_priv,
> -			const struct drm_mode_fb_cmd2 *mode_cmd)
> -{
> -	const struct drm_format_info *info = drm_get_format_info(dev,
> -								 mode_cmd);
> -	struct drm_framebuffer *fb;
> -	struct drm_gem_object *objs[ROCKCHIP_MAX_FB_BUFFER];
> -	struct drm_gem_object *obj;
> -	int num_planes = min_t(int, info->num_planes, ROCKCHIP_MAX_FB_BUFFER);
> -	int ret;
> -	int i;
> -
> -	for (i = 0; i < num_planes; i++) {
> -		unsigned int width = mode_cmd->width / (i ? info->hsub : 1);
> -		unsigned int height = mode_cmd->height / (i ? info->vsub : 1);
> -		unsigned int min_size;
> -
> -		obj = drm_gem_object_lookup(file_priv, mode_cmd->handles[i]);
> -		if (!obj) {
> -			DRM_DEV_ERROR(dev->dev,
> -				      "Failed to lookup GEM object\n");
> -			ret = -ENXIO;
> -			goto err_gem_object_unreference;
> -		}
> -
> -		min_size = (height - 1) * mode_cmd->pitches[i] +
> -			mode_cmd->offsets[i] +
> -			width * info->cpp[i];
> -
> -		if (obj->size < min_size) {
> -			drm_gem_object_put_unlocked(obj);
> -			ret = -EINVAL;
> -			goto err_gem_object_unreference;
> -		}
> -		objs[i] = obj;
> -	}
> -
> -	fb = rockchip_fb_alloc(dev, mode_cmd, objs, i);
> -	if (IS_ERR(fb)) {
> -		ret = PTR_ERR(fb);
> -		goto err_gem_object_unreference;
> -	}
> -
> -	return fb;
> -
> -err_gem_object_unreference:
> -	for (i--; i >= 0; i--)
> -		drm_gem_object_put_unlocked(objs[i]);
> -	return ERR_PTR(ret);
> -}
> -
>   static const struct drm_mode_config_helper_funcs rockchip_mode_config_helpers = {
>   	.atomic_commit_tail = drm_atomic_helper_commit_tail_rpm,
>   };
>   
>   static const struct drm_mode_config_funcs rockchip_drm_mode_config_funcs = {
> -	.fb_create = rockchip_user_fb_create,
> +	.fb_create = drm_gem_fb_create,
>   	.output_poll_changed = drm_fb_helper_output_poll_changed,
>   	.atomic_check = drm_atomic_helper_check,
>   	.atomic_commit = drm_atomic_helper_commit,
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 4/8] drm/rockchip: Use drm_gem_fb_create_with_dirty

[Andrzej Pietrasiewicz]

Hi Daniel,

After applying this patch there are some slight differences
in the effective behavior of the code.

I can't tell if they are important, please see below.

Andrzej

W dniu 15.11.2019 o 10:21, Daniel Vetter pisze:
> If rockchip would switch over to the generic fbdev setup we could
> grabage collect even more of all this code (all of the remaining fb
> handling code really).
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Sandy Huang <hjc@rock-chips.com>
> Cc: "Heiko Stübner" <heiko@sntech.de>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-rockchip@lists.infradead.org
> ---
>   drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 54 +---------------------
>   1 file changed, 1 insertion(+), 53 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> index ca01234c037c..081dbdaa0b07 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> @@ -53,64 +53,12 @@ rockchip_fb_alloc(struct drm_device *dev, const struct drm_mode_fb_cmd2 *mode_cm
>   	return fb;
>   }
>   
> -static struct drm_framebuffer *
> -rockchip_user_fb_create(struct drm_device *dev, struct drm_file *file_priv,
> -			const struct drm_mode_fb_cmd2 *mode_cmd)
> -{
> -	const struct drm_format_info *info = drm_get_format_info(dev,
> -								 mode_cmd);
> -	struct drm_framebuffer *fb;
> -	struct drm_gem_object *objs[ROCKCHIP_MAX_FB_BUFFER];
> -	struct drm_gem_object *obj;
> -	int num_planes = min_t(int, info->num_planes, ROCKCHIP_MAX_FB_BUFFER);
> -	int ret;
> -	int i;
> -
> -	for (i = 0; i < num_planes; i++) {

drm_gem_fb_create_with_funcs(), if no error happens,
iterates exactly info->num_planes times,
but the function being removed here iterates
min_t(int, info->num_planes, 3) times.

Is it ensured earlier elsewhere that info->num_planes does not exceed 3?

> -		unsigned int width = mode_cmd->width / (i ? info->hsub : 1);
> -		unsigned int height = mode_cmd->height / (i ? info->vsub : 1);
> -		unsigned int min_size;
> -
> -		obj = drm_gem_object_lookup(file_priv, mode_cmd->handles[i]);
> -		if (!obj) {
> -			DRM_DEV_ERROR(dev->dev,
> -				      "Failed to lookup GEM object\n");
> -			ret = -ENXIO;
> -			goto err_gem_object_unreference;
> -		}
> -
> -		min_size = (height - 1) * mode_cmd->pitches[i] +
> -			mode_cmd->offsets[i] +
> -			width * info->cpp[i];

This computation in drm_gem_fb_create_with_funcs looks like this:

		min_size = (height - 1) * mode_cmd->pitches[i]
			 + drm_format_info_min_pitch(info, i, width)
			 + mode_cmd->offsets[i];

Perhaps that's actually the same thing?

> -
> -		if (obj->size < min_size) {
> -			drm_gem_object_put_unlocked(obj);
> -			ret = -EINVAL;
> -			goto err_gem_object_unreference;
> -		}
> -		objs[i] = obj;
> -	}
> -
> -	fb = rockchip_fb_alloc(dev, mode_cmd, objs, i);
> -	if (IS_ERR(fb)) {
> -		ret = PTR_ERR(fb);
> -		goto err_gem_object_unreference;
> -	}
> -
> -	return fb;
> -
> -err_gem_object_unreference:
> -	for (i--; i >= 0; i--)
> -		drm_gem_object_put_unlocked(objs[i]);
> -	return ERR_PTR(ret);
> -}
> -
>   static const struct drm_mode_config_helper_funcs rockchip_mode_config_helpers = {
>   	.atomic_commit_tail = drm_atomic_helper_commit_tail_rpm,
>   };
>   
>   static const struct drm_mode_config_funcs rockchip_drm_mode_config_funcs = {
> -	.fb_create = rockchip_user_fb_create,
> +	.fb_create = drm_gem_fb_create,

That way you leave out the ->dirty() callback from
static const struct drm_framebuffer_funcs rockchip_drm_fb_funcs

I'd say instead:

struct drm_framebuffer *
rockchip_gem_fb_create(struct drm_device *dev, struct drm_file *file,
		       const struct drm_mode_fb_cmd2 *mode_cmd)
{
	return drm_gem_fb_create_with_funcs(dev, file, mode_cmd,
					    &rockchip_drm_fb_funcs);
}

and then

+	.fb_create = rockchip_gem_fb_create,

>   	.output_poll_changed = drm_fb_helper_output_poll_changed,
>   	.atomic_check = drm_atomic_helper_check,
>   	.atomic_commit = drm_atomic_helper_commit,
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 4/8] drm/rockchip: Use drm_gem_fb_create_with_dirty

[Daniel Vetter]

On Wed, Nov 27, 2019 at 6:33 PM Andrzej Pietrasiewicz
<andrzej.p@collabora.com> wrote:
>
> Hi Daniel,
>
> After applying this patch there are some slight differences
> in the effective behavior of the code.
>
> I can't tell if they are important, please see below.
>
> Andrzej
>
> W dniu 15.11.2019 o 10:21, Daniel Vetter pisze:
> > If rockchip would switch over to the generic fbdev setup we could
> > grabage collect even more of all this code (all of the remaining fb
> > handling code really).
> >
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > Cc: Sandy Huang <hjc@rock-chips.com>
> > Cc: "Heiko Stübner" <heiko@sntech.de>
> > Cc: linux-arm-kernel@lists.infradead.org
> > Cc: linux-rockchip@lists.infradead.org
> > ---
> >   drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 54 +---------------------
> >   1 file changed, 1 insertion(+), 53 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> > index ca01234c037c..081dbdaa0b07 100644
> > --- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> > @@ -53,64 +53,12 @@ rockchip_fb_alloc(struct drm_device *dev, const struct drm_mode_fb_cmd2 *mode_cm
> >       return fb;
> >   }
> >
> > -static struct drm_framebuffer *
> > -rockchip_user_fb_create(struct drm_device *dev, struct drm_file *file_priv,
> > -                     const struct drm_mode_fb_cmd2 *mode_cmd)
> > -{
> > -     const struct drm_format_info *info = drm_get_format_info(dev,
> > -                                                              mode_cmd);
> > -     struct drm_framebuffer *fb;
> > -     struct drm_gem_object *objs[ROCKCHIP_MAX_FB_BUFFER];
> > -     struct drm_gem_object *obj;
> > -     int num_planes = min_t(int, info->num_planes, ROCKCHIP_MAX_FB_BUFFER);
> > -     int ret;
> > -     int i;
> > -
> > -     for (i = 0; i < num_planes; i++) {
>
> drm_gem_fb_create_with_funcs(), if no error happens,
> iterates exactly info->num_planes times,
> but the function being removed here iterates
> min_t(int, info->num_planes, 3) times.
>
> Is it ensured earlier elsewhere that info->num_planes does not exceed 3?

rockchip only supports fourcc codes with at most 3 planes. Now we're
not checking for that here, it'll only be caught later on in the
ATOMIC ioctl. So would be nice to fix, but it's a preexisting bug (the
todo.rst patch has an idea how to fix this for good). So really
doesn't matter whether we fill out the 4th plane or not. Also iirc we
don't have any fourcc code with 4 planes right now.

> > -             unsigned int width = mode_cmd->width / (i ? info->hsub : 1);
> > -             unsigned int height = mode_cmd->height / (i ? info->vsub : 1);
> > -             unsigned int min_size;
> > -
> > -             obj = drm_gem_object_lookup(file_priv, mode_cmd->handles[i]);
> > -             if (!obj) {
> > -                     DRM_DEV_ERROR(dev->dev,
> > -                                   "Failed to lookup GEM object\n");
> > -                     ret = -ENXIO;
> > -                     goto err_gem_object_unreference;
> > -             }
> > -
> > -             min_size = (height - 1) * mode_cmd->pitches[i] +
> > -                     mode_cmd->offsets[i] +
> > -                     width * info->cpp[i];
>
> This computation in drm_gem_fb_create_with_funcs looks like this:
>
>                 min_size = (height - 1) * mode_cmd->pitches[i]
>                          + drm_format_info_min_pitch(info, i, width)
>                          + mode_cmd->offsets[i];
>
> Perhaps that's actually the same thing?
>
> > -
> > -             if (obj->size < min_size) {
> > -                     drm_gem_object_put_unlocked(obj);
> > -                     ret = -EINVAL;
> > -                     goto err_gem_object_unreference;
> > -             }
> > -             objs[i] = obj;
> > -     }
> > -
> > -     fb = rockchip_fb_alloc(dev, mode_cmd, objs, i);
> > -     if (IS_ERR(fb)) {
> > -             ret = PTR_ERR(fb);
> > -             goto err_gem_object_unreference;
> > -     }
> > -
> > -     return fb;
> > -
> > -err_gem_object_unreference:
> > -     for (i--; i >= 0; i--)
> > -             drm_gem_object_put_unlocked(objs[i]);
> > -     return ERR_PTR(ret);
> > -}
> > -
> >   static const struct drm_mode_config_helper_funcs rockchip_mode_config_helpers = {
> >       .atomic_commit_tail = drm_atomic_helper_commit_tail_rpm,
> >   };
> >
> >   static const struct drm_mode_config_funcs rockchip_drm_mode_config_funcs = {
> > -     .fb_create = rockchip_user_fb_create,
> > +     .fb_create = drm_gem_fb_create,
>
> That way you leave out the ->dirty() callback from
> static const struct drm_framebuffer_funcs rockchip_drm_fb_funcs

Oops indeed. Subject of this patch is right, but the actual patch
isn't, this should be drm_gem_fb_create_with_dirty, then it's a 100%
matching replacement. Thanks for catching this, I'll send out an
updated patch.
-Daniel

>
> I'd say instead:
>
> struct drm_framebuffer *
> rockchip_gem_fb_create(struct drm_device *dev, struct drm_file *file,
>                        const struct drm_mode_fb_cmd2 *mode_cmd)
> {
>         return drm_gem_fb_create_with_funcs(dev, file, mode_cmd,
>                                             &rockchip_drm_fb_funcs);
> }
>
> and then
>
> +       .fb_create = rockchip_gem_fb_create,
>
> >       .output_poll_changed = drm_fb_helper_output_poll_changed,
> >       .atomic_check = drm_atomic_helper_check,
> >       .atomic_commit = drm_atomic_helper_commit,
> >
>


-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel