Re: [PATCHv6 2/6] drm/core: Add drm_afbc_framebuffer and a corresponding helper

Hi Andrzej,

On Tue, 3 Mar 2020 at 12:01, Andrzej Pietrasiewicz andrzej.p@collabora.com wrote:

The new struct contains afbc-specific data.

The new function can be used by drivers which support afbc to complete the preparation of struct drm_afbc_framebuffer. It must be called after allocating the said struct and calling drm_gem_fb_init_with_funcs().

Signed-off-by: Andrzej Pietrasiewicz andrzej.p@collabora.com

drivers/gpu/drm/drm_gem_framebuffer_helper.c | 121 +++++++++++++++++++ include/drm/drm_framebuffer.h | 45 +++++++ include/drm/drm_gem_framebuffer_helper.h | 11 ++ 3 files changed, 177 insertions(+)

diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c index 388a080cd2df..2a30f5b6829f 100644 --- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c +++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c @@ -21,6 +21,13 @@ #include <drm/drm_modeset_helper.h> #include <drm/drm_simple_kms_helper.h>

+#define AFBC_HEADER_SIZE 16 +#define AFBC_TH_LAYOUT_ALIGNMENT 8 +#define AFBC_HDR_ALIGN 64 +#define AFBC_SUPERBLOCK_PIXELS 256 +#define AFBC_SUPERBLOCK_ALIGNMENT 128 +#define AFBC_TH_BODY_START_ALIGNMENT 4096

/**

• DOC: overview

@@ -302,6 +309,120 @@ drm_gem_fb_create_with_dirty(struct drm_device *dev, struct drm_file *file, } EXPORT_SYMBOL_GPL(drm_gem_fb_create_with_dirty);

+/**

• • drm_afbc_get_superblock_wh - extract afbc block width/height from modifier
• • @modifier: the modifier to be looked at
• • @w: address of a place to store the block width
• • @h: address of a place to store the block height
• • Returns: true if the modifier describes a supported block size
• */

+bool drm_afbc_get_superblock_wh(u64 modifier, u32 *w, u32 *h) +{

•   switch (modifier & AFBC_FORMAT_MOD_BLOCK_SIZE_MASK) {
  

•   case AFBC_FORMAT_MOD_BLOCK_SIZE_16x16:
  

•           *w = 16;
  

•           *h = 16;
  

•           break;
  

•   case AFBC_FORMAT_MOD_BLOCK_SIZE_32x8:
  

•           *w = 32;
  

•           *h = 8;
  

•           break;
  

•   /* no user exists yet - fall through */
  

•   case AFBC_FORMAT_MOD_BLOCK_SIZE_64x4:
  

•   case AFBC_FORMAT_MOD_BLOCK_SIZE_32x8_64x4:
  

•   default:
  

•           DRM_DEBUG_KMS("Invalid AFBC_FORMAT_MOD_BLOCK_SIZE: %lld.\n",
  

•                         modifier & AFBC_FORMAT_MOD_BLOCK_SIZE_MASK);
  

•           return false;
  

•   }
  

•   return true;
  

+} +EXPORT_SYMBOL_GPL(drm_afbc_get_superblock_wh);

+static int drm_gem_afbc_min_size(struct drm_device *dev,

•                            const struct drm_mode_fb_cmd2 *mode_cmd,
  

•                            struct drm_afbc_framebuffer *afbc_fb)
  

+{

•   const struct drm_format_info *info;
  

•   u32 n_blocks, w_alignment, h_alignment, hdr_alignment;
  

•   u32 tmp_bpp; /* remove when all users properly encode cpp in drm_format_info */
  

•   if (!drm_afbc_get_superblock_wh(mode_cmd->modifier[0], &afbc_fb->block_width, &afbc_fb->block_height))
  

•           return -EINVAL;
  

•   /* tiled header afbc */
  

•   w_alignment = afbc_fb->block_width;
  

•   h_alignment = afbc_fb->block_height;
  

•   hdr_alignment = AFBC_HDR_ALIGN;
  

•   if (mode_cmd->modifier[0] & AFBC_FORMAT_MOD_TILED) {
  

•           w_alignment *= AFBC_TH_LAYOUT_ALIGNMENT;
  

•           h_alignment *= AFBC_TH_LAYOUT_ALIGNMENT;
  

•           hdr_alignment = AFBC_TH_BODY_START_ALIGNMENT;
  

•   }
  

•   afbc_fb->aligned_width = ALIGN(mode_cmd->width, w_alignment);
  

•   afbc_fb->aligned_height = ALIGN(mode_cmd->height, h_alignment);
  

•   afbc_fb->offset = mode_cmd->offsets[0];
  

•   /* Change to always using info->cpp[0] when all users properly encode it */
  

•   info = drm_get_format_info(dev, mode_cmd);
  

•   tmp_bpp = info->cpp[0] ? info->cpp[0] * 8 : afbc_fb->bpp;
  

•   n_blocks = (afbc_fb->aligned_width * afbc_fb->aligned_height) / AFBC_SUPERBLOCK_PIXELS;
  

•   afbc_fb->afbc_size = ALIGN(n_blocks * AFBC_HEADER_SIZE, hdr_alignment);
  

•   afbc_fb->afbc_size += n_blocks * ALIGN(tmp_bpp * AFBC_SUPERBLOCK_PIXELS / 8, AFBC_SUPERBLOCK_ALIGNMENT);
  

•   return 0;
  

+}

+/**

• • drm_gem_fb_afbc_init() - Helper function for drivers using afbc to

• •                     fill and validate all the afbc-specific
    

• •                     struct drm_afbc_framebuffer members
    

• • @dev: DRM device
• • @afbc_fb: afbc-specific framebuffer
• • @mode_cmd: Metadata from the userspace framebuffer creation request
• • @afbc_fb: afbc framebuffer
• • This function can be used by drivers which support afbc to complete
• • the preparation of struct drm_afbc_framebuffer. It must be called after
• • allocating the said struct and calling drm_gem_fb_init_with_funcs().
• • Returns:
• • Zero on success or a negative error value on failure.
• */

+int drm_gem_fb_afbc_init(struct drm_device *dev, const struct drm_mode_fb_cmd2 *mode_cmd,

•                    struct drm_afbc_framebuffer *afbc_fb)
  

+{

•   const struct drm_format_info *info;
  

•   struct drm_gem_object **objs;
  

•   int ret = -EINVAL, i;
  

•   objs = afbc_fb->base.obj;
  

•   info = drm_get_format_info(dev, mode_cmd);
  

•   if (!info)
  

•           goto error;
  

•   ret = drm_gem_afbc_min_size(dev, mode_cmd, afbc_fb);
  

•   if (ret < 0)
  

•           goto error;
  

•   if (objs[0]->size < afbc_fb->afbc_size) {
  

•           ret = -EINVAL;
  

•           goto error;
  

•   }
  

•   return 0;
  

+error:

•   for (i = 0; i < info->num_planes; i++)
  

•           drm_gem_object_put_unlocked(objs[i]);
  

Here we put and object we did not get. This type of asymmetric API usage isn't common, thus it's fairly fragile. Let's leave the put in the caller.

•   return ret;
  

+}

/**

• drm_gem_fb_prepare_fb() - Prepare a GEM backed framebuffer
• @plane: Plane

diff --git a/include/drm/drm_framebuffer.h b/include/drm/drm_framebuffer.h index c0e0256e3e98..e9f1b0e2968d 100644 --- a/include/drm/drm_framebuffer.h +++ b/include/drm/drm_framebuffer.h @@ -297,4 +297,49 @@ int drm_framebuffer_plane_width(int width, int drm_framebuffer_plane_height(int height, const struct drm_framebuffer *fb, int plane);

+/**

• • struct drm_afbc_framebuffer - a special afbc frame buffer object
• • A derived class of struct drm_framebuffer, dedicated for afbc use cases.
• */

+struct drm_afbc_framebuffer {

•   /**
  

•    * @base: base framebuffer structure.
  

•    */
  

•   struct drm_framebuffer base;
  

•   /**
  

•    * @block_widht: width of a single afbc block
  

•    */
  

•   u32 block_width;
  

•   /**
  

•    * @block_widht: height of a single afbc block
  

•    */
  

•   u32 block_height;
  

•   /**
  

•    * @aligned_width: aligned frame buffer width
  

•    */
  

•   u32 aligned_width;
  

•   /**
  

•    * @aligned_height: aligned frame buffer height
  

•    */
  

•   u32 aligned_height;
  

•   /**
  

•    * @offset: offset of the first afbc header
  

•    */
  

•   u32 offset;
  

•   /**
  

•    * @afbc_size: minimum size of afbc buffer
  

•    */
  

•   u32 afbc_size;
  

•   /**
  

•    * @bpp: bpp value for this afbc buffer
  

•    * To be removed when users such as malidp
  

•    * properly store the cpp in drm_format_info.
  

•    * New users should not start using this field.
  

•    */
  

Are you planning to look into this? Alternatively let's add an entry in Documentation/gpu/todo.rst.

Nit: Line wrap the newly introduced code to 80 columns. IIRC scripts/checkpatch.pl will complain loudly about it.

-Emil