| Message ID | 20191011111813.20851-2-andrzej.p@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | AFBC for Rockchip | expand |
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@collabora.com> Hi Andrzej, Thanks a lot for doing this. Much appreciated. :) It was on our TODO list for a long time. I have cc-ed james.qian.wang@arm.com, Mihail.Atanassov@arm.com 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 > @@ -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@arm.com and Mihail.Atanassov@arm.com have their opinion on this. > 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@collabora.com> > + * > + */ > +#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. > + > + 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@collabora.com> > + * > + */ > +#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@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
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@collabora.com> > > 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@arm.com, Mihail.Atanassov@arm.com 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@arm.com and Mihail.Atanassov@arm.com 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@collabora.com> > > + * > > + */ > > +#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@collabora.com> > > + * > > + */ > > +#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@lists.freedesktop.org > > 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.
This series adds AFBC support for Rockchip. It is inspired by: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/refs/heads/factory-gru-9017.B-chromeos-4.4/drivers/gpu/drm/rockchip/rockchip_drm_vop.c The first patch factors out some afbc helper functions, as they are useful in general. The second and third patches use the helpers and the fourth patch adds implementation proper of AFBC support for Rockchip. v1..v2: - addressed comments from Daniel Stone, Ayan Halder, Mihail Atanassov - coding style fixes Andrzej Pietrasiewicz (4): drm/arm: Factor out generic afbc helpers drm/malidp: use afbc helpers drm/komeda: use afbc helpers drm/rockchip: Add support for afbc drivers/gpu/drm/Kconfig | 8 + drivers/gpu/drm/Makefile | 1 + drivers/gpu/drm/arm/Kconfig | 1 + .../arm/display/komeda/komeda_format_caps.h | 1 - .../arm/display/komeda/komeda_framebuffer.c | 44 +++--- drivers/gpu/drm/arm/malidp_drv.c | 66 ++------ drivers/gpu/drm/drm_afbc.c | 129 ++++++++++++++++ drivers/gpu/drm/rockchip/Kconfig | 1 + drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 42 ++++++ drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 141 +++++++++++++++++- drivers/gpu/drm/rockchip/rockchip_drm_vop.h | 12 ++ drivers/gpu/drm/rockchip/rockchip_vop_reg.c | 84 ++++++++++- include/drm/drm_afbc.h | 36 +++++ 13 files changed, 480 insertions(+), 86 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 + 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 @@ -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; } 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@collabora.com> + * + */ +#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: + /* 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; + } + + 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@collabora.com> + * + */ +#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__ */
These are useful for other users of afbc, e.g. rockchip. Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com> --- 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