diff mbox series

[PATCHv6,3/6] drm/arm/malidp: Factor-in framebuffer creation

Message ID 20200303120136.18294-4-andrzej.p@collabora.com (mailing list archive)
State New, archived
Headers show
Series Add AFBC support for Rockchip | expand

Commit Message

Andrzej Pietrasiewicz March 3, 2020, 12:01 p.m. UTC 
Consolidating framebuffer creation into one function will make it easier
to transition to generic afbc-aware helpers.

Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
---
 drivers/gpu/drm/arm/malidp_drv.c | 158 +++++++++++++------------------
 1 file changed, 67 insertions(+), 91 deletions(-)

Comments

Emil Velikov March 3, 2020, 4:51 p.m. UTC | #1 
Hi Andrzej,

On Tue, 3 Mar 2020 at 12:01, Andrzej Pietrasiewicz
<andrzej.p@collabora.com> wrote:
>
> Consolidating framebuffer creation into one function will make it easier
> to transition to generic afbc-aware helpers.
>
I'd suggest keeping the refactor a bit simpler.
Say - first folds the functions together. Then do the modifier[0] reshuffle.

As-is the patch seems to introduce a leak:


> +               objs = drm_gem_object_lookup(file, mode_cmd->handles[0]);
> +               if (!objs) {
> +                       DRM_DEBUG_KMS("Failed to lookup GEM object\n");
> +                       return ERR_PTR(-EINVAL);
> +               }
>
> -       objs = drm_gem_object_lookup(file, mode_cmd->handles[0]);
> -       if (!objs) {
> -               DRM_DEBUG_KMS("Failed to lookup GEM object\n");
> -               return false;
> -       }
> +               if (objs->size < afbc_size) {
> +                       DRM_DEBUG_KMS("buffer size (%zu) too small for AFBC buffer size = %u\n",
> +                                     objs->size, afbc_size);
> +                       drm_gem_object_put_unlocked(objs);
> +                       return ERR_PTR(-EINVAL);
> +               }
>
We're missing the drm_gem_object_put_unlocked() after the if block.

> -       if (objs->size < afbc_size) {
> -               DRM_DEBUG_KMS("buffer size (%zu) too small for AFBC buffer size = %u\n",
> -                             objs->size, afbc_size);
>                 drm_gem_object_put_unlocked(objs);
> -               return false;
> -       }
> -
> -       drm_gem_object_put_unlocked(objs);
> -
... namely this one ^^.


-Emil
Emil Velikov March 6, 2020, 6:25 p.m. UTC | #2 
On Tue, 3 Mar 2020 at 16:51, Emil Velikov <emil.l.velikov@gmail.com> wrote:

> > +               objs = drm_gem_object_lookup(file, mode_cmd->handles[0]);
> > +               if (!objs) {
> > +                       DRM_DEBUG_KMS("Failed to lookup GEM object\n");
> > +                       return ERR_PTR(-EINVAL);
> > +               }
> >
> > -       objs = drm_gem_object_lookup(file, mode_cmd->handles[0]);
> > -       if (!objs) {
> > -               DRM_DEBUG_KMS("Failed to lookup GEM object\n");
> > -               return false;
> > -       }
> > +               if (objs->size < afbc_size) {
> > +                       DRM_DEBUG_KMS("buffer size (%zu) too small for AFBC buffer size = %u\n",
> > +                                     objs->size, afbc_size);
> > +                       drm_gem_object_put_unlocked(objs);
> > +                       return ERR_PTR(-EINVAL);
> > +               }
> >
> We're missing the drm_gem_object_put_unlocked() after the if block.
>
Please ignore this comment - it's there, I'm simply blind :-\

-Emil
diff mbox series

Patch

diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c
index 37d92a06318e..b53fc01baf2b 100644
--- a/drivers/gpu/drm/arm/malidp_drv.c
+++ b/drivers/gpu/drm/arm/malidp_drv.c
@@ -269,112 +269,88 @@  static const struct drm_mode_config_helper_funcs malidp_mode_config_helpers = {
 	.atomic_commit_tail = malidp_atomic_commit_tail,
 };
 
-static bool
-malidp_verify_afbc_framebuffer_caps(struct drm_device *dev,
-				    const struct drm_mode_fb_cmd2 *mode_cmd)
+static struct drm_framebuffer *
+malidp_fb_create(struct drm_device *dev, struct drm_file *file,
+		 const struct drm_mode_fb_cmd2 *mode_cmd)
 {
-	if (malidp_format_mod_supported(dev, mode_cmd->pixel_format,
-					mode_cmd->modifier[0]) == false)
-		return false;
+	if (mode_cmd->modifier[0]) {
+		int n_superblocks = 0;
+		const struct drm_format_info *info;
+		struct drm_gem_object *objs = NULL;
+		u32 afbc_superblock_size = 0, afbc_superblock_height = 0;
+		u32 afbc_superblock_width = 0, afbc_size = 0;
+		int bpp = 0;
+
+		if (malidp_format_mod_supported(dev, mode_cmd->pixel_format,
+						mode_cmd->modifier[0]) == false)
+			return ERR_PTR(-EINVAL);
 
-	if (mode_cmd->offsets[0] != 0) {
-		DRM_DEBUG_KMS("AFBC buffers' plane offset should be 0\n");
-		return false;
-	}
+		if (mode_cmd->offsets[0] != 0) {
+			DRM_DEBUG_KMS("AFBC buffers' plane offset should be 0\n");
+			return ERR_PTR(-EINVAL);
+		}
 
-	switch (mode_cmd->modifier[0] & AFBC_SIZE_MASK) {
-	case AFBC_SIZE_16X16:
-		if ((mode_cmd->width % 16) || (mode_cmd->height % 16)) {
-			DRM_DEBUG_KMS("AFBC buffers must be aligned to 16 pixels\n");
-			return false;
+		switch (mode_cmd->modifier[0] & AFBC_SIZE_MASK) {
+		case AFBC_SIZE_16X16:
+			if ((mode_cmd->width % 16) || (mode_cmd->height % 16)) {
+				DRM_DEBUG_KMS("AFBC buffers must be aligned to 16 pixels\n");
+				return ERR_PTR(-EINVAL);
+			}
+			break;
+		default:
+			DRM_DEBUG_KMS("Unsupported AFBC block size\n");
+			return ERR_PTR(-EINVAL);
 		}
-		break;
-	default:
-		DRM_DEBUG_KMS("Unsupported AFBC block size\n");
-		return false;
-	}
 
-	return true;
-}
+		switch (mode_cmd->modifier[0] & AFBC_SIZE_MASK) {
+		case AFBC_SIZE_16X16:
+			afbc_superblock_height = 16;
+			afbc_superblock_width = 16;
+			break;
+		default:
+			DRM_DEBUG_KMS("AFBC superblock size is not supported\n");
+			return ERR_PTR(-EINVAL);
+		}
 
-static bool
-malidp_verify_afbc_framebuffer_size(struct drm_device *dev,
-				    struct drm_file *file,
-				    const struct drm_mode_fb_cmd2 *mode_cmd)
-{
-	int n_superblocks = 0;
-	const struct drm_format_info *info;
-	struct drm_gem_object *objs = NULL;
-	u32 afbc_superblock_size = 0, afbc_superblock_height = 0;
-	u32 afbc_superblock_width = 0, afbc_size = 0;
-	int bpp = 0;
-
-	switch (mode_cmd->modifier[0] & AFBC_SIZE_MASK) {
-	case AFBC_SIZE_16X16:
-		afbc_superblock_height = 16;
-		afbc_superblock_width = 16;
-		break;
-	default:
-		DRM_DEBUG_KMS("AFBC superblock size is not supported\n");
-		return false;
-	}
+		info = drm_get_format_info(dev, mode_cmd);
 
-	info = drm_get_format_info(dev, mode_cmd);
+		n_superblocks = (mode_cmd->width / afbc_superblock_width) *
+			(mode_cmd->height / afbc_superblock_height);
 
-	n_superblocks = (mode_cmd->width / afbc_superblock_width) *
-		(mode_cmd->height / afbc_superblock_height);
+		bpp = malidp_format_get_bpp(info->format);
 
-	bpp = malidp_format_get_bpp(info->format);
+		afbc_superblock_size =
+			(bpp * afbc_superblock_width * afbc_superblock_height)
+			/ BITS_PER_BYTE;
 
-	afbc_superblock_size = (bpp * afbc_superblock_width * afbc_superblock_height)
-				/ BITS_PER_BYTE;
+		afbc_size = ALIGN(n_superblocks * AFBC_HEADER_SIZE,
+				  AFBC_SUPERBLK_ALIGNMENT);
+		afbc_size += n_superblocks
+			* ALIGN(afbc_superblock_size, AFBC_SUPERBLK_ALIGNMENT);
 
-	afbc_size = ALIGN(n_superblocks * AFBC_HEADER_SIZE, AFBC_SUPERBLK_ALIGNMENT);
-	afbc_size += n_superblocks * ALIGN(afbc_superblock_size, AFBC_SUPERBLK_ALIGNMENT);
+		if ((mode_cmd->width * bpp) !=
+			(mode_cmd->pitches[0] * BITS_PER_BYTE)) {
+			DRM_DEBUG_KMS("Invalid value of (pitch * BITS_PER_BYTE) (=%u) "
+				      "should be same as width (=%u) * bpp (=%u)\n",
+				      (mode_cmd->pitches[0] * BITS_PER_BYTE),
+				      mode_cmd->width, bpp);
+			return ERR_PTR(-EINVAL);
+		}
 
-	if ((mode_cmd->width * bpp) != (mode_cmd->pitches[0] * BITS_PER_BYTE)) {
-		DRM_DEBUG_KMS("Invalid value of (pitch * BITS_PER_BYTE) (=%u) "
-			      "should be same as width (=%u) * bpp (=%u)\n",
-			      (mode_cmd->pitches[0] * BITS_PER_BYTE),
-			      mode_cmd->width, bpp);
-		return false;
-	}
+		objs = drm_gem_object_lookup(file, mode_cmd->handles[0]);
+		if (!objs) {
+			DRM_DEBUG_KMS("Failed to lookup GEM object\n");
+			return ERR_PTR(-EINVAL);
+		}
 
-	objs = drm_gem_object_lookup(file, mode_cmd->handles[0]);
-	if (!objs) {
-		DRM_DEBUG_KMS("Failed to lookup GEM object\n");
-		return false;
-	}
+		if (objs->size < afbc_size) {
+			DRM_DEBUG_KMS("buffer size (%zu) too small for AFBC buffer size = %u\n",
+				      objs->size, afbc_size);
+			drm_gem_object_put_unlocked(objs);
+			return ERR_PTR(-EINVAL);
+		}
 
-	if (objs->size < afbc_size) {
-		DRM_DEBUG_KMS("buffer size (%zu) too small for AFBC buffer size = %u\n",
-			      objs->size, afbc_size);
 		drm_gem_object_put_unlocked(objs);
-		return false;
-	}
-
-	drm_gem_object_put_unlocked(objs);
-
-	return true;
-}
-
-static bool
-malidp_verify_afbc_framebuffer(struct drm_device *dev, struct drm_file *file,
-			       const struct drm_mode_fb_cmd2 *mode_cmd)
-{
-	if (malidp_verify_afbc_framebuffer_caps(dev, mode_cmd))
-		return malidp_verify_afbc_framebuffer_size(dev, file, mode_cmd);
-
-	return false;
-}
-
-static struct drm_framebuffer *
-malidp_fb_create(struct drm_device *dev, struct drm_file *file,
-		 const struct drm_mode_fb_cmd2 *mode_cmd)
-{
-	if (mode_cmd->modifier[0]) {
-		if (!malidp_verify_afbc_framebuffer(dev, file, mode_cmd))
-			return ERR_PTR(-EINVAL);
 	}
 
 	return drm_gem_fb_create(dev, file, mode_cmd);