| Message ID | 20200311145541.29186-3-andrzej.p@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add AFBC support for Rockchip | expand |
On Wed, Mar 11, 2020 at 10:55:37PM +0800, Andrzej Pietrasiewicz 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> Looks good to me. Reviewed-by: James Qian Wang <james.qian.wang@arm.com> Thanks James > --- > Documentation/gpu/todo.rst | 15 +++ > drivers/gpu/drm/drm_gem_framebuffer_helper.c | 108 +++++++++++++++++++ > include/drm/drm_framebuffer.h | 45 ++++++++ > include/drm/drm_gem_framebuffer_helper.h | 10 ++ > 4 files changed, 178 insertions(+) > > diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst > index 439656f55c5d..37a3a023c114 100644 > --- a/Documentation/gpu/todo.rst > +++ b/Documentation/gpu/todo.rst > @@ -404,6 +404,21 @@ Contact: Laurent Pinchart, respective driver maintainers > > Level: Intermediate > > +Encode cpp properly in malidp > +----------------------------- > + > +cpp (chars per pixel) is not encoded properly in malidp, zero is > +used instead. afbc implementation needs bpp or cpp, but if it is > +zero it needs to be provided elsewhere, and so the bpp field exists > +in struct drm_afbc_framebuffer. > + > +Properly encode cpp in malidp and remove the bpp field in struct > +drm_afbc_framebuffer. > + > +Contact: malidp maintainers > + > +Level: Intermediate > + > Core refactorings > ================= > > diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c > index 86c1907c579a..7e3982c36baa 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,107 @@ drm_gem_fb_create_with_dirty(struct drm_device *dev, struct drm_file *file, > } > EXPORT_SYMBOL_GPL(drm_gem_fb_create_with_dirty); > > +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; > + /* remove bpp when all users properly encode cpp in drm_format_info */ > + __u32 bpp; > + > + switch (mode_cmd->modifier[0] & AFBC_FORMAT_MOD_BLOCK_SIZE_MASK) { > + case AFBC_FORMAT_MOD_BLOCK_SIZE_16x16: > + afbc_fb->block_width = 16; > + afbc_fb->block_height = 16; > + break; > + case AFBC_FORMAT_MOD_BLOCK_SIZE_32x8: > + afbc_fb->block_width = 32; > + afbc_fb->block_height = 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", > + mode_cmd->modifier[0] > + & AFBC_FORMAT_MOD_BLOCK_SIZE_MASK); > + 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]; > + > + info = drm_get_format_info(dev, mode_cmd); > + /* > + * Change to always using info->cpp[0] > + * when all users properly encode it > + */ > + 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(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(). > + * It is caller's responsibility to put afbc_fb->base.obj objects in case > + * the call is unsuccessful. > + * > + * 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; > + > + objs = afbc_fb->base.obj; > + info = drm_get_format_info(dev, mode_cmd); > + if (!info) > + return -EINVAL; > + > + ret = drm_gem_afbc_min_size(dev, mode_cmd, afbc_fb); > + if (ret < 0) > + return ret; > + > + if (objs[0]->size < afbc_fb->afbc_size) > + return -EINVAL; > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(drm_gem_fb_afbc_init); > + > /** > * 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. > + */ > + u32 bpp; > +}; > + > +#define fb_to_afbc_fb(x) container_of(x, struct drm_afbc_framebuffer, base) > + > #endif > diff --git a/include/drm/drm_gem_framebuffer_helper.h b/include/drm/drm_gem_framebuffer_helper.h > index c029c1618661..6b013154911d 100644 > --- a/include/drm/drm_gem_framebuffer_helper.h > +++ b/include/drm/drm_gem_framebuffer_helper.h > @@ -1,6 +1,7 @@ > #ifndef __DRM_GEM_FB_HELPER_H__ > #define __DRM_GEM_FB_HELPER_H__ > > +struct drm_afbc_framebuffer; > struct drm_device; > struct drm_fb_helper_surface_size; > struct drm_file; > @@ -12,6 +13,8 @@ struct drm_plane; > struct drm_plane_state; > struct drm_simple_display_pipe; > > +#define AFBC_VENDOR_AND_TYPE_MASK GENMASK_ULL(63, 52) > + > struct drm_gem_object *drm_gem_fb_get_obj(struct drm_framebuffer *fb, > unsigned int plane); > void drm_gem_fb_destroy(struct drm_framebuffer *fb); > @@ -34,6 +37,13 @@ struct drm_framebuffer * > drm_gem_fb_create_with_dirty(struct drm_device *dev, struct drm_file *file, > const struct drm_mode_fb_cmd2 *mode_cmd); > > +#define drm_is_afbc(modifier) \ > + (((modifier) & AFBC_VENDOR_AND_TYPE_MASK) == DRM_FORMAT_MOD_ARM_AFBC(0)) > + > +int drm_gem_fb_afbc_init(struct drm_device *dev, > + const struct drm_mode_fb_cmd2 *mode_cmd, > + struct drm_afbc_framebuffer *afbc_fb); > + > int drm_gem_fb_prepare_fb(struct drm_plane *plane, > struct drm_plane_state *state); > int drm_gem_fb_simple_display_pipe_prepare_fb(struct drm_simple_display_pipe *pipe, > -- > 2.17.1
On Wed, Mar 11, 2020 at 03:55:37PM +0100, Andrzej Pietrasiewicz 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> Yeah I think this now looks like a fairly clean interface for drivers. On the first two core patches: Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch> Thanks for sticking around through all the fairly major revisions here! Cheers, Daniel > --- > Documentation/gpu/todo.rst | 15 +++ > drivers/gpu/drm/drm_gem_framebuffer_helper.c | 108 +++++++++++++++++++ > include/drm/drm_framebuffer.h | 45 ++++++++ > include/drm/drm_gem_framebuffer_helper.h | 10 ++ > 4 files changed, 178 insertions(+) > > diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst > index 439656f55c5d..37a3a023c114 100644 > --- a/Documentation/gpu/todo.rst > +++ b/Documentation/gpu/todo.rst > @@ -404,6 +404,21 @@ Contact: Laurent Pinchart, respective driver maintainers > > Level: Intermediate > > +Encode cpp properly in malidp > +----------------------------- > + > +cpp (chars per pixel) is not encoded properly in malidp, zero is > +used instead. afbc implementation needs bpp or cpp, but if it is > +zero it needs to be provided elsewhere, and so the bpp field exists > +in struct drm_afbc_framebuffer. > + > +Properly encode cpp in malidp and remove the bpp field in struct > +drm_afbc_framebuffer. > + > +Contact: malidp maintainers > + > +Level: Intermediate > + > Core refactorings > ================= > > diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c > index 86c1907c579a..7e3982c36baa 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,107 @@ drm_gem_fb_create_with_dirty(struct drm_device *dev, struct drm_file *file, > } > EXPORT_SYMBOL_GPL(drm_gem_fb_create_with_dirty); > > +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; > + /* remove bpp when all users properly encode cpp in drm_format_info */ > + __u32 bpp; > + > + switch (mode_cmd->modifier[0] & AFBC_FORMAT_MOD_BLOCK_SIZE_MASK) { > + case AFBC_FORMAT_MOD_BLOCK_SIZE_16x16: > + afbc_fb->block_width = 16; > + afbc_fb->block_height = 16; > + break; > + case AFBC_FORMAT_MOD_BLOCK_SIZE_32x8: > + afbc_fb->block_width = 32; > + afbc_fb->block_height = 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", > + mode_cmd->modifier[0] > + & AFBC_FORMAT_MOD_BLOCK_SIZE_MASK); > + 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]; > + > + info = drm_get_format_info(dev, mode_cmd); > + /* > + * Change to always using info->cpp[0] > + * when all users properly encode it > + */ > + 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(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(). > + * It is caller's responsibility to put afbc_fb->base.obj objects in case > + * the call is unsuccessful. > + * > + * 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; > + > + objs = afbc_fb->base.obj; > + info = drm_get_format_info(dev, mode_cmd); > + if (!info) > + return -EINVAL; > + > + ret = drm_gem_afbc_min_size(dev, mode_cmd, afbc_fb); > + if (ret < 0) > + return ret; > + > + if (objs[0]->size < afbc_fb->afbc_size) > + return -EINVAL; > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(drm_gem_fb_afbc_init); > + > /** > * 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. > + */ > + u32 bpp; > +}; > + > +#define fb_to_afbc_fb(x) container_of(x, struct drm_afbc_framebuffer, base) > + > #endif > diff --git a/include/drm/drm_gem_framebuffer_helper.h b/include/drm/drm_gem_framebuffer_helper.h > index c029c1618661..6b013154911d 100644 > --- a/include/drm/drm_gem_framebuffer_helper.h > +++ b/include/drm/drm_gem_framebuffer_helper.h > @@ -1,6 +1,7 @@ > #ifndef __DRM_GEM_FB_HELPER_H__ > #define __DRM_GEM_FB_HELPER_H__ > > +struct drm_afbc_framebuffer; > struct drm_device; > struct drm_fb_helper_surface_size; > struct drm_file; > @@ -12,6 +13,8 @@ struct drm_plane; > struct drm_plane_state; > struct drm_simple_display_pipe; > > +#define AFBC_VENDOR_AND_TYPE_MASK GENMASK_ULL(63, 52) > + > struct drm_gem_object *drm_gem_fb_get_obj(struct drm_framebuffer *fb, > unsigned int plane); > void drm_gem_fb_destroy(struct drm_framebuffer *fb); > @@ -34,6 +37,13 @@ struct drm_framebuffer * > drm_gem_fb_create_with_dirty(struct drm_device *dev, struct drm_file *file, > const struct drm_mode_fb_cmd2 *mode_cmd); > > +#define drm_is_afbc(modifier) \ > + (((modifier) & AFBC_VENDOR_AND_TYPE_MASK) == DRM_FORMAT_MOD_ARM_AFBC(0)) > + > +int drm_gem_fb_afbc_init(struct drm_device *dev, > + const struct drm_mode_fb_cmd2 *mode_cmd, > + struct drm_afbc_framebuffer *afbc_fb); > + > int drm_gem_fb_prepare_fb(struct drm_plane *plane, > struct drm_plane_state *state); > int drm_gem_fb_simple_display_pipe_prepare_fb(struct drm_simple_display_pipe *pipe, > -- > 2.17.1 >
On Wed, Mar 11, 2020 at 3:55 PM 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> > --- > Documentation/gpu/todo.rst | 15 +++ > drivers/gpu/drm/drm_gem_framebuffer_helper.c | 108 +++++++++++++++++++ > include/drm/drm_framebuffer.h | 45 ++++++++ > include/drm/drm_gem_framebuffer_helper.h | 10 ++ > 4 files changed, 178 insertions(+) > > diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst > index 439656f55c5d..37a3a023c114 100644 > --- a/Documentation/gpu/todo.rst > +++ b/Documentation/gpu/todo.rst > @@ -404,6 +404,21 @@ Contact: Laurent Pinchart, respective driver maintainers > > Level: Intermediate > > +Encode cpp properly in malidp > +----------------------------- > + > +cpp (chars per pixel) is not encoded properly in malidp, zero is > +used instead. afbc implementation needs bpp or cpp, but if it is > +zero it needs to be provided elsewhere, and so the bpp field exists > +in struct drm_afbc_framebuffer. > + > +Properly encode cpp in malidp and remove the bpp field in struct > +drm_afbc_framebuffer. > + > +Contact: malidp maintainers > + > +Level: Intermediate Just stumbled over this todo, which is really surprising. Also definitely not something I wanted to ack, earlier versions at least didn't have this. Why is this needed? drm_afbc_framebuffer contains a drm_framebuffer, which has a pointer to drm_format_info, which we're always setting (the core does that for all drivers, both for addfb and addfb2). Why is that not good enough to get at cpp for everyone? Cheers, Daniel > + > Core refactorings > ================= > > diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c > index 86c1907c579a..7e3982c36baa 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,107 @@ drm_gem_fb_create_with_dirty(struct drm_device *dev, struct drm_file *file, > } > EXPORT_SYMBOL_GPL(drm_gem_fb_create_with_dirty); > > +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; > + /* remove bpp when all users properly encode cpp in drm_format_info */ > + __u32 bpp; > + > + switch (mode_cmd->modifier[0] & AFBC_FORMAT_MOD_BLOCK_SIZE_MASK) { > + case AFBC_FORMAT_MOD_BLOCK_SIZE_16x16: > + afbc_fb->block_width = 16; > + afbc_fb->block_height = 16; > + break; > + case AFBC_FORMAT_MOD_BLOCK_SIZE_32x8: > + afbc_fb->block_width = 32; > + afbc_fb->block_height = 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", > + mode_cmd->modifier[0] > + & AFBC_FORMAT_MOD_BLOCK_SIZE_MASK); > + 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]; > + > + info = drm_get_format_info(dev, mode_cmd); > + /* > + * Change to always using info->cpp[0] > + * when all users properly encode it > + */ > + 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(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(). > + * It is caller's responsibility to put afbc_fb->base.obj objects in case > + * the call is unsuccessful. > + * > + * 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; > + > + objs = afbc_fb->base.obj; > + info = drm_get_format_info(dev, mode_cmd); > + if (!info) > + return -EINVAL; > + > + ret = drm_gem_afbc_min_size(dev, mode_cmd, afbc_fb); > + if (ret < 0) > + return ret; > + > + if (objs[0]->size < afbc_fb->afbc_size) > + return -EINVAL; > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(drm_gem_fb_afbc_init); > + > /** > * 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. > + */ > + u32 bpp; > +}; > + > +#define fb_to_afbc_fb(x) container_of(x, struct drm_afbc_framebuffer, base) > + > #endif > diff --git a/include/drm/drm_gem_framebuffer_helper.h b/include/drm/drm_gem_framebuffer_helper.h > index c029c1618661..6b013154911d 100644 > --- a/include/drm/drm_gem_framebuffer_helper.h > +++ b/include/drm/drm_gem_framebuffer_helper.h > @@ -1,6 +1,7 @@ > #ifndef __DRM_GEM_FB_HELPER_H__ > #define __DRM_GEM_FB_HELPER_H__ > > +struct drm_afbc_framebuffer; > struct drm_device; > struct drm_fb_helper_surface_size; > struct drm_file; > @@ -12,6 +13,8 @@ struct drm_plane; > struct drm_plane_state; > struct drm_simple_display_pipe; > > +#define AFBC_VENDOR_AND_TYPE_MASK GENMASK_ULL(63, 52) > + > struct drm_gem_object *drm_gem_fb_get_obj(struct drm_framebuffer *fb, > unsigned int plane); > void drm_gem_fb_destroy(struct drm_framebuffer *fb); > @@ -34,6 +37,13 @@ struct drm_framebuffer * > drm_gem_fb_create_with_dirty(struct drm_device *dev, struct drm_file *file, > const struct drm_mode_fb_cmd2 *mode_cmd); > > +#define drm_is_afbc(modifier) \ > + (((modifier) & AFBC_VENDOR_AND_TYPE_MASK) == DRM_FORMAT_MOD_ARM_AFBC(0)) > + > +int drm_gem_fb_afbc_init(struct drm_device *dev, > + const struct drm_mode_fb_cmd2 *mode_cmd, > + struct drm_afbc_framebuffer *afbc_fb); > + > int drm_gem_fb_prepare_fb(struct drm_plane *plane, > struct drm_plane_state *state); > int drm_gem_fb_simple_display_pipe_prepare_fb(struct drm_simple_display_pipe *pipe, > -- > 2.17.1 > -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
Hi Daniel, W dniu 30.03.2020 o 19:01, Daniel Vetter pisze: > On Wed, Mar 11, 2020 at 3:55 PM Andrzej Pietrasiewicz > <andrzej.p@collabora.com> wrote: >> >> The new struct contains afbc-specific data. <snip> >> diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst >> index 439656f55c5d..37a3a023c114 100644 >> --- a/Documentation/gpu/todo.rst >> +++ b/Documentation/gpu/todo.rst >> @@ -404,6 +404,21 @@ Contact: Laurent Pinchart, respective driver maintainers >> >> Level: Intermediate >> >> +Encode cpp properly in malidp >> +----------------------------- >> + >> +cpp (chars per pixel) is not encoded properly in malidp, zero is >> +used instead. afbc implementation needs bpp or cpp, but if it is >> +zero it needs to be provided elsewhere, and so the bpp field exists >> +in struct drm_afbc_framebuffer. >> + >> +Properly encode cpp in malidp and remove the bpp field in struct >> +drm_afbc_framebuffer. >> + >> +Contact: malidp maintainers >> + >> +Level: Intermediate > > Just stumbled over this todo, which is really surprising. Also > definitely not something I wanted to ack, earlier versions at least > didn't have this. > > Why is this needed? drm_afbc_framebuffer contains a drm_framebuffer, > which has a pointer to drm_format_info, which we're always setting > (the core does that for all drivers, both for addfb and addfb2). Why > is that not good enough to get at cpp for everyone? > > Cheers, Daniel > Let me quote James https://patchwork.freedesktop.org/patch/345603/#comment_653081: "Seems we can remove this bpp or no need to define it as a pass in argument for size check, maybe the komeda/malidp get_afbc_bpp() function mislead you that afbc formats may have vendor specific bpp. But the story is: for afbc only formats like DRM_FORMAT_YUV420_8BIT/10BIT, we have set nothing in drm_format_info, neither cpp nor block_size, so both malidp or komeda introduce a get_bpp(), but actually the two funcs basically are same. So my suggestion is we can temporary use the get_afbc_bpp() in malidp or komeda. and eventually I think we'd better set the block size for these formats, then we can defines a common get_bpp() like pitch". Andrzej
On Mon, Mar 30, 2020 at 7:44 PM Andrzej Pietrasiewicz <andrzej.p@collabora.com> wrote: > > Hi Daniel, > > W dniu 30.03.2020 o 19:01, Daniel Vetter pisze: > > On Wed, Mar 11, 2020 at 3:55 PM Andrzej Pietrasiewicz > > <andrzej.p@collabora.com> wrote: > >> > >> The new struct contains afbc-specific data. > > <snip> > > >> diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst > >> index 439656f55c5d..37a3a023c114 100644 > >> --- a/Documentation/gpu/todo.rst > >> +++ b/Documentation/gpu/todo.rst > >> @@ -404,6 +404,21 @@ Contact: Laurent Pinchart, respective driver maintainers > >> > >> Level: Intermediate > >> > >> +Encode cpp properly in malidp > >> +----------------------------- > >> + > >> +cpp (chars per pixel) is not encoded properly in malidp, zero is > >> +used instead. afbc implementation needs bpp or cpp, but if it is > >> +zero it needs to be provided elsewhere, and so the bpp field exists > >> +in struct drm_afbc_framebuffer. > >> + > >> +Properly encode cpp in malidp and remove the bpp field in struct > >> +drm_afbc_framebuffer. > >> + > >> +Contact: malidp maintainers > >> + > >> +Level: Intermediate > > > > Just stumbled over this todo, which is really surprising. Also > > definitely not something I wanted to ack, earlier versions at least > > didn't have this. > > > > Why is this needed? drm_afbc_framebuffer contains a drm_framebuffer, > > which has a pointer to drm_format_info, which we're always setting > > (the core does that for all drivers, both for addfb and addfb2). Why > > is that not good enough to get at cpp for everyone? > > > > Cheers, Daniel > > > > Let me quote James https://patchwork.freedesktop.org/patch/345603/#comment_653081: > > "Seems we can remove this bpp or no need to define it as a pass in argument > for size check, maybe the komeda/malidp get_afbc_bpp() function mislead > you that afbc formats may have vendor specific bpp. > > But the story is: > > for afbc only formats like DRM_FORMAT_YUV420_8BIT/10BIT, we have set > nothing in drm_format_info, neither cpp nor block_size, so both malidp > or komeda introduce a get_bpp(), but actually the two funcs basically > are same. > > So my suggestion is we can temporary use the get_afbc_bpp() in malidp > or komeda. and eventually I think we'd better set the block size > for these formats, then we can defines a common get_bpp() like pitch". Hm iirc we had some good reasons not to set the block size for these, but maybe with these afbc helpers that's changed. We could also compute cpp/bpp in the helper, starting from the format_info/fourcc code, for these special cases. Essentially move the exact computation that komeda/malidp do right now to set afbc->bpp and move it into the helper. Just kinda feels like unfinished work that we still leave this to drivers, that's some very quirky api. -Daniel
diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst index 439656f55c5d..37a3a023c114 100644 --- a/Documentation/gpu/todo.rst +++ b/Documentation/gpu/todo.rst @@ -404,6 +404,21 @@ Contact: Laurent Pinchart, respective driver maintainers Level: Intermediate +Encode cpp properly in malidp +----------------------------- + +cpp (chars per pixel) is not encoded properly in malidp, zero is +used instead. afbc implementation needs bpp or cpp, but if it is +zero it needs to be provided elsewhere, and so the bpp field exists +in struct drm_afbc_framebuffer. + +Properly encode cpp in malidp and remove the bpp field in struct +drm_afbc_framebuffer. + +Contact: malidp maintainers + +Level: Intermediate + Core refactorings ================= diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c index 86c1907c579a..7e3982c36baa 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,107 @@ drm_gem_fb_create_with_dirty(struct drm_device *dev, struct drm_file *file, } EXPORT_SYMBOL_GPL(drm_gem_fb_create_with_dirty); +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; + /* remove bpp when all users properly encode cpp in drm_format_info */ + __u32 bpp; + + switch (mode_cmd->modifier[0] & AFBC_FORMAT_MOD_BLOCK_SIZE_MASK) { + case AFBC_FORMAT_MOD_BLOCK_SIZE_16x16: + afbc_fb->block_width = 16; + afbc_fb->block_height = 16; + break; + case AFBC_FORMAT_MOD_BLOCK_SIZE_32x8: + afbc_fb->block_width = 32; + afbc_fb->block_height = 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", + mode_cmd->modifier[0] + & AFBC_FORMAT_MOD_BLOCK_SIZE_MASK); + 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]; + + info = drm_get_format_info(dev, mode_cmd); + /* + * Change to always using info->cpp[0] + * when all users properly encode it + */ + 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(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(). + * It is caller's responsibility to put afbc_fb->base.obj objects in case + * the call is unsuccessful. + * + * 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; + + objs = afbc_fb->base.obj; + info = drm_get_format_info(dev, mode_cmd); + if (!info) + return -EINVAL; + + ret = drm_gem_afbc_min_size(dev, mode_cmd, afbc_fb); + if (ret < 0) + return ret; + + if (objs[0]->size < afbc_fb->afbc_size) + return -EINVAL; + + return 0; +} +EXPORT_SYMBOL_GPL(drm_gem_fb_afbc_init); + /** * 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. + */ + u32 bpp; +}; + +#define fb_to_afbc_fb(x) container_of(x, struct drm_afbc_framebuffer, base) + #endif diff --git a/include/drm/drm_gem_framebuffer_helper.h b/include/drm/drm_gem_framebuffer_helper.h index c029c1618661..6b013154911d 100644 --- a/include/drm/drm_gem_framebuffer_helper.h +++ b/include/drm/drm_gem_framebuffer_helper.h @@ -1,6 +1,7 @@ #ifndef __DRM_GEM_FB_HELPER_H__ #define __DRM_GEM_FB_HELPER_H__ +struct drm_afbc_framebuffer; struct drm_device; struct drm_fb_helper_surface_size; struct drm_file; @@ -12,6 +13,8 @@ struct drm_plane; struct drm_plane_state; struct drm_simple_display_pipe; +#define AFBC_VENDOR_AND_TYPE_MASK GENMASK_ULL(63, 52) + struct drm_gem_object *drm_gem_fb_get_obj(struct drm_framebuffer *fb, unsigned int plane); void drm_gem_fb_destroy(struct drm_framebuffer *fb); @@ -34,6 +37,13 @@ struct drm_framebuffer * drm_gem_fb_create_with_dirty(struct drm_device *dev, struct drm_file *file, const struct drm_mode_fb_cmd2 *mode_cmd); +#define drm_is_afbc(modifier) \ + (((modifier) & AFBC_VENDOR_AND_TYPE_MASK) == DRM_FORMAT_MOD_ARM_AFBC(0)) + +int drm_gem_fb_afbc_init(struct drm_device *dev, + const struct drm_mode_fb_cmd2 *mode_cmd, + struct drm_afbc_framebuffer *afbc_fb); + int drm_gem_fb_prepare_fb(struct drm_plane *plane, struct drm_plane_state *state); int drm_gem_fb_simple_display_pipe_prepare_fb(struct drm_simple_display_pipe *pipe,
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> --- Documentation/gpu/todo.rst | 15 +++ drivers/gpu/drm/drm_gem_framebuffer_helper.c | 108 +++++++++++++++++++ include/drm/drm_framebuffer.h | 45 ++++++++ include/drm/drm_gem_framebuffer_helper.h | 10 ++ 4 files changed, 178 insertions(+)