Re: [PATCHv7 2/6] drm/core: Add drm_afbc_framebuffer and a corresponding helper
On Mon, Mar 30, 2020 at 7:44 PM Andrzej Pietrasiewicz 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 > > wrote: > >> > >> The new struct contains afbc-specific data. > > > > >> 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 -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCHv7 2/6] drm/core: Add drm_afbc_framebuffer and a corresponding helper
Hi Daniel, W dniu 30.03.2020 o 19:01, Daniel Vetter pisze: On Wed, Mar 11, 2020 at 3:55 PM Andrzej Pietrasiewicz wrote: The new struct contains afbc-specific data. 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 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCHv7 2/6] drm/core: Add drm_afbc_framebuffer and a corresponding helper
On Wed, Mar 11, 2020 at 3:55 PM 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
> ---
> 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
> #include
>
> +#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 *
Re: [PATCHv7 2/6] drm/core: Add drm_afbc_framebuffer and a corresponding helper
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
Yeah I think this now looks like a fairly clean interface for drivers. On
the first two core patches:
Acked-by: Daniel Vetter
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
> #include
>
> +#define AFBC_HEADER_SIZE 16
> +#define AFBC_TH_LAYOUT_ALIGNMENT 8
> +#define AFBC_HDR_ALIGN 64
> +#define AFBC_SUPERBLOCK_PIXELS 256
> +#define AFBC_SUPERBLOCK_ALIGNMENT128
> +#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
Re: [PATCHv7 2/6] drm/core: Add drm_afbc_framebuffer and a corresponding helper
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
Looks good to me.
Reviewed-by: James Qian Wang
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
> #include
>
> +#define AFBC_HEADER_SIZE 16
> +#define AFBC_TH_LAYOUT_ALIGNMENT 8
> +#define AFBC_HDR_ALIGN 64
> +#define AFBC_SUPERBLOCK_PIXELS 256
> +#define AFBC_SUPERBLOCK_ALIGNMENT128
> +#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
> + *
> +
