Hi Andrzej, On Monday, 21 October 2019 14:50:14 BST Ayan Halder wrote: > On Fri, Oct 11, 2019 at 01:18:10PM +0200, Andrzej Pietrasiewicz wrote: > > These are useful for other users of afbc, e.g. rockchip. > > > > Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@xxxxxxxxxxxxx> > > Hi Andrzej, > > Thanks a lot for doing this. Much appreciated. :) > It was on our TODO list for a long time. Seconded, thanks for the patch! > > I have cc-ed james.qian.wang@xxxxxxx, Mihail.Atanassov@xxxxxxx for > their comments as well. > > > --- > > drivers/gpu/drm/Kconfig | 4 ++ > > drivers/gpu/drm/Makefile | 1 + > > drivers/gpu/drm/arm/Kconfig | 1 + > > drivers/gpu/drm/arm/malidp_drv.c | 58 ++-------------- > > drivers/gpu/drm/drm_afbc.c | 114 +++++++++++++++++++++++++++++++ > > include/drm/drm_afbc.h | 25 +++++++ > > 6 files changed, 149 insertions(+), 54 deletions(-) > > create mode 100644 drivers/gpu/drm/drm_afbc.c > > create mode 100644 include/drm/drm_afbc.h > > > > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig > > index 3c88420e3497..00e3f90557f4 100644 > > --- a/drivers/gpu/drm/Kconfig > > +++ b/drivers/gpu/drm/Kconfig > > @@ -195,6 +195,10 @@ config DRM_SCHED > > tristate > > depends on DRM > > > > +config DRM_AFBC > > + tristate > > + depends on DRM > Adding a 'help' would be great here. Stealing the first line from > https://www.kernel.org/doc/html/latest/gpu/afbc.html > > "AFBC is a proprietary lossless image compression protocol and format. > It provides fine-grained random access and minimizes the amount of > data transferred between IP blocks." > > > + > > source "drivers/gpu/drm/i2c/Kconfig" > > > > source "drivers/gpu/drm/arm/Kconfig" > > diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile > > index 9f0d2ee35794..55368b668355 100644 > > --- a/drivers/gpu/drm/Makefile > > +++ b/drivers/gpu/drm/Makefile > > @@ -31,6 +31,7 @@ drm-$(CONFIG_OF) += drm_of.o > > drm-$(CONFIG_AGP) += drm_agpsupport.o > > drm-$(CONFIG_DEBUG_FS) += drm_debugfs.o drm_debugfs_crc.o > > drm-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o > > +drm-$(CONFIG_DRM_AFBC) += drm_afbc.o > > > > drm_vram_helper-y := drm_gem_vram_helper.o \ > > drm_vram_helper_common.o \ > > diff --git a/drivers/gpu/drm/arm/Kconfig b/drivers/gpu/drm/arm/Kconfig > > index a204103b3efb..25c3dc408cda 100644 > > --- a/drivers/gpu/drm/arm/Kconfig > > +++ b/drivers/gpu/drm/arm/Kconfig > > @@ -29,6 +29,7 @@ config DRM_MALI_DISPLAY > > select DRM_KMS_HELPER > > select DRM_KMS_CMA_HELPER > > select DRM_GEM_CMA_HELPER > > + select DRM_AFBC > > select VIDEOMODE_HELPERS > > help > > Choose this option if you want to compile the ARM Mali Display > > diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c > > index f25ec4382277..a67b69e08f63 100644 > > --- a/drivers/gpu/drm/arm/malidp_drv.c > > +++ b/drivers/gpu/drm/arm/malidp_drv.c This file is GPL-2.0-only. Shouldn't this be preserved when the substantive bit of the code in drm_afbc.c comes from here? > > @@ -16,6 +16,7 @@ > > #include <linux/debugfs.h> > > > > #include <drm/drmP.h> > > +#include <drm/drm_afbc.h> > > #include <drm/drm_atomic.h> > > #include <drm/drm_atomic_helper.h> > > #include <drm/drm_crtc.h> > > @@ -33,8 +34,6 @@ > > #include "malidp_hw.h" > > > > #define MALIDP_CONF_VALID_TIMEOUT 250 > > -#define AFBC_HEADER_SIZE 16 > > -#define AFBC_SUPERBLK_ALIGNMENT 128 > > > > static void malidp_write_gamma_table(struct malidp_hw_device *hwdev, > > u32 data[MALIDP_COEFFTAB_NUM_COEFFS]) > > @@ -275,24 +274,8 @@ malidp_verify_afbc_framebuffer_caps(struct drm_device *dev, > > mode_cmd->modifier[0]) == false) > > return false; > > > > - if (mode_cmd->offsets[0] != 0) { > > - DRM_DEBUG_KMS("AFBC buffers' plane offset should be 0\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 false; > > - } > > - break; > > - default: > > - DRM_DEBUG_KMS("Unsupported AFBC block size\n"); > > - return false; > > - } > > - > > - return true; > > + return drm_afbc_check_offset(dev, mode_cmd) && > > + drm_afbc_check_size_align(dev, mode_cmd); > > } > > > > static bool > > @@ -300,53 +283,20 @@ 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); > > - > > - n_superblocks = (mode_cmd->width / afbc_superblock_width) * > > - (mode_cmd->height / afbc_superblock_height); > > - > > bpp = malidp_format_get_bpp(info->format); > > > > - 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); > > - > > - 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 false; > > } > > > > - if (objs->size < afbc_size) { > > - DRM_DEBUG_KMS("buffer size (%zu) too small for AFBC buffer size = %u\n", > > - objs->size, afbc_size); > > + if (!drm_afbc_check_fb_size(dev, mode_cmd, objs, bpp)) { > > drm_gem_object_put_unlocked(objs); > > return false; > > } > Also can you do the code refactoring for komeda driver as well. > specifically komeda_fb_afbc_size_check(). I will let > james.qian.wang@xxxxxxx and Mihail.Atanassov@xxxxxxx have their > opinion on this. I'd say that it'd be really nice to get this work done for us ;) but it doesn't have to be in this patch but rather in a follow-up. > > > diff --git a/drivers/gpu/drm/drm_afbc.c b/drivers/gpu/drm/drm_afbc.c > > new file mode 100644 > > index 000000000000..3e8a9225fd2e > > --- /dev/null > > +++ b/drivers/gpu/drm/drm_afbc.c > > @@ -0,0 +1,114 @@ > > +// SPDX-License-Identifier: GPL-2.0+ > > +/* > > + * (C) 2019 Collabora Ltd. > > + * > > + * author: Andrzej Pietrasiewicz <andrzej.p@xxxxxxxxxxxxx> > > + * > > + */ > > +#include <linux/module.h> > > + > > +#include <drm/drm_afbc.h> > > +#include <drm/drm_device.h> > > +#include <drm/drm_fourcc.h> > > +#include <drm/drm_gem.h> > > +#include <drm/drm_mode.h> > > +#include <drm/drm_print.h> > > + > > +#define AFBC_HEADER_SIZE 16 > > +#define AFBC_SUPERBLK_ALIGNMENT 128 > > + > > +bool drm_afbc_check_offset(struct drm_device *dev, > > + const struct drm_mode_fb_cmd2 *mode_cmd) > > +{ > > + if (mode_cmd->offsets[0] != 0) { > > + DRM_DEBUG_KMS("AFBC buffers' plane offset should be 0\n"); > > + return false; > > + } > > + > > + return true; > > +} > > +EXPORT_SYMBOL_GPL(drm_afbc_check_offset); > > + > > +bool drm_afbc_check_size_align(struct drm_device *dev, > > + const struct drm_mode_fb_cmd2 *mode_cmd) > > +{ > > + > > + switch (mode_cmd->modifier[0] & AFBC_FORMAT_MOD_BLOCK_SIZE_MASK) { > > + case AFBC_FORMAT_MOD_BLOCK_SIZE_16x16: > > + if ((mode_cmd->width % 16) || (mode_cmd->height % 16)) { > > + DRM_DEBUG_KMS( > > + "AFBC buffer must be aligned to 16 pixels\n" > > + ); > > + return false; > > + } > > + break; > > + case AFBC_FORMAT_MOD_BLOCK_SIZE_32x8: > > + /* fall through */ > > + case AFBC_FORMAT_MOD_BLOCK_SIZE_64x4: > > + /* fall through */ > > + case AFBC_FORMAT_MOD_BLOCK_SIZE_32x8_64x4: > > + /* fall through */ > > + default: > > + DRM_DEBUG_KMS("Unsupported AFBC block size\n"); > > + return false; > > + } > > + > > + return true; > > +} > > +EXPORT_SYMBOL_GPL(drm_afbc_check_size_align); > > + > > +bool drm_afbc_check_fb_size(struct drm_device *dev, > > + const struct drm_mode_fb_cmd2 *mode_cmd, > > + struct drm_gem_object *objs, int bpp) > > +{ > > + int n_superblocks = 0; > > + u32 afbc_superblock_size = 0, afbc_superblock_height = 0; > > + u32 afbc_superblock_width = 0, afbc_size = 0; > > + > > + switch (mode_cmd->modifier[0] & AFBC_FORMAT_MOD_BLOCK_SIZE_MASK) { > > + case AFBC_FORMAT_MOD_BLOCK_SIZE_16x16: > > + afbc_superblock_height = 16; > > + afbc_superblock_width = 16; > > + break; > > + case AFBC_FORMAT_MOD_BLOCK_SIZE_32x8: > Copying from > https://cgit.freedesktop.org/drm/drm-tip/tree/drivers/gpu/drm/arm/display/komeda/komeda_framebuffer.c#n60 > afbc_superblock_width = 32; > afbc_superblock_height = 8; > > + /* fall through */ > > + case AFBC_FORMAT_MOD_BLOCK_SIZE_64x4: > > + /* fall through */ > > + case AFBC_FORMAT_MOD_BLOCK_SIZE_32x8_64x4: > > + /* fall through */ > > + default: > > + DRM_DEBUG_KMS("AFBC superblock size is not supported\n"); > > + return false; > > + } > Can you combine the two switch - case confitions (from this function > and the one in drm_afbc_check_size_align()) and put it in a separate > function (say drm_afbc_get_superblock_dimensions()) of its own ? > This will help to avoid code repetition. > I'm personally a bit on the fence about this - functions should ideally do one thing. That shared helper would be both getting the dimensions _and_ checking the mode_cmd is valid, so one place cares about one half of the function's results, and the other - the second half. ¯\_O_/¯, opinions, everybody has them :). > > + > > + n_superblocks = (mode_cmd->width / afbc_superblock_width) * > > + (mode_cmd->height / afbc_superblock_height); > > + > > + 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); > > + > > + 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; > > + } > > + > > + if (objs->size < afbc_size) { > > + DRM_DEBUG_KMS("buffer size (%zu) too small for AFBC buffer size = %u\n", > > + objs->size, afbc_size > > + ); > > + > > + return false; > > + } > > + > > + return true; > > +} > > +EXPORT_SYMBOL(drm_afbc_check_fb_size); > > diff --git a/include/drm/drm_afbc.h b/include/drm/drm_afbc.h > > new file mode 100644 > > index 000000000000..ce39c850217b > > --- /dev/null > > +++ b/include/drm/drm_afbc.h > > @@ -0,0 +1,25 @@ > > +/* SPDX-License-Identifier: GPL-2.0+ */ > > +/* > > + * (C) 2019 Collabora Ltd. > > + * > > + * author: Andrzej Pietrasiewicz <andrzej.p@xxxxxxxxxxxxx> > > + * > > + */ > > +#ifndef __DRM_AFBC_H__ > > +#define __DRM_AFBC_H__ > > + > > +struct drm_device; > > +struct drm_mode_fb_cmd2; > > +struct drm_gem_object; > > + > > +bool drm_afbc_check_offset(struct drm_device *dev, > > + const struct drm_mode_fb_cmd2 *mode_cmd); > > + > > +bool drm_afbc_check_size_align(struct drm_device *dev, > > + const struct drm_mode_fb_cmd2 *mode_cmd); > > + > > +bool drm_afbc_check_fb_size(struct drm_device *dev, > > + const struct drm_mode_fb_cmd2 *mode_cmd, > > + struct drm_gem_object *objs, int bpp); > > + > > +#endif /* __DRM_AFBC_H__ */ > > -- > > 2.17.1 > > > > _______________________________________________ > > dri-devel mailing list > > dri-devel@xxxxxxxxxxxxxxxxxxxxx > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > Overall the patch LGTM as-is, nice and straightforward mostly-cut-n-paste, just let's sort out the SPDX identifier question I have, then I'll be happy. -- Mihail _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
