On Fri, Oct 05, 2018 at 09:26:47AM +0000, Alexandru-Cosmin Gheorghe wrote:
> For some pixel formats .cpp structure in drm_format info it's not
> enough to describe the peculiarities of the pixel layout, for example
> tiled formats or packed formats at bit level.
>
> What's implemented here is to add three new members to drm_format_info
> that could describe such formats:
>
> - char_per_block[3]
> - block_w[3]
> - block_h[3]
>
> char_per_block will be put in a union alongside cpp, for transparent
> compatibility with the existing format descriptions.
>
> Regarding, block_w and block_h they are intended to be used through
> their equivalent getters drm_format_info_block_width /
> drm_format_info_block_height, the reason of the getters is to abstract
> the fact that for normal formats block_w and block_h will be unset/0,
> but the methods will be returning 1.
>
> Using that the following drm core functions had been updated to
> generically handle both block and non-block formats:
>
> - drm_fb_cma_get_gem_addr: for block formats it will just return the
> beginning of the block.
> - framebuffer_check: Updated to compute the minimum pitch as
> DIV_ROUND_UP(width * char_per_block, drm_format_info_block_width(info, i)
> * drm_format_info_block_height(info, i))
> - drm_gem_fb_create_with_funcs: Updated to compute the size of the
> last line of pixels with the same formula as for the minimum pitch.
> - In places where is not expecting to handle block formats, like fbdev
> helpers I just added some warnings in case the block width/height
> are greater than 1.
>
> Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@xxxxxxx>
> Reviewed-by: Liviu Dudau <liviu.dudau@xxxxxxx>
> ---
> drivers/gpu/drm/drm_fb_cma_helper.c | 21 ++++++++--
> drivers/gpu/drm/drm_fb_helper.c | 6 +++
> drivers/gpu/drm/drm_fourcc.c | 40 ++++++++++++++++++++
> drivers/gpu/drm/drm_framebuffer.c | 9 +++--
> drivers/gpu/drm/drm_gem_framebuffer_helper.c | 4 +-
> include/drm/drm_fourcc.h | 29 +++++++++++++-
> 6 files changed, 101 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c
> index 47e0e2f6642d..f8293f4d96cf 100644
> --- a/drivers/gpu/drm/drm_fb_cma_helper.c
> +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
> @@ -72,7 +72,9 @@ struct drm_gem_cma_object *drm_fb_cma_get_gem_obj(struct drm_framebuffer *fb,
> EXPORT_SYMBOL_GPL(drm_fb_cma_get_gem_obj);
>
> /**
> - * drm_fb_cma_get_gem_addr() - Get physical address for framebuffer
> + * drm_fb_cma_get_gem_addr() - Get physical address for framebuffer, for pixel
> + * formats where values are grouped in blocks this will get you the beginning of
> + * the block
> * @fb: The framebuffer
> * @state: Which state of drm plane
> * @plane: Which plane
> @@ -87,6 +89,14 @@ dma_addr_t drm_fb_cma_get_gem_addr(struct drm_framebuffer *fb,
> struct drm_gem_cma_object *obj;
> dma_addr_t paddr;
> u8 h_div = 1, v_div = 1;
> + u32 block_w = drm_format_info_block_width(fb->format, plane);
> + u32 block_h = drm_format_info_block_height(fb->format, plane);
> + u32 block_size = fb->format->char_per_block[plane];
> + u32 sample_x;
> + u32 sample_y;
> + u32 block_start_y;
> + u32 num_hblocks;
> +
>
> obj = drm_fb_cma_get_gem_obj(fb, plane);
> if (!obj)
> @@ -99,8 +109,13 @@ dma_addr_t drm_fb_cma_get_gem_addr(struct drm_framebuffer *fb,
> v_div = fb->format->vsub;
> }
>
> - paddr += (fb->format->cpp[plane] * (state->src_x >> 16)) / h_div;
> - paddr += (fb->pitches[plane] * (state->src_y >> 16)) / v_div;
> + sample_x = (state->src_x >> 16) / h_div;
> + sample_y = (state->src_y >> 16) / v_div;
> + block_start_y = (sample_y / block_h) * block_h;
> + num_hblocks = sample_x / block_w;
> +
> + paddr += fb->pitches[plane] * block_start_y;
> + paddr += block_size * num_hblocks;
>
> return paddr;
> }
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index a504a5e05676..9add0d7da744 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -1595,6 +1595,10 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
> if (var->pixclock != 0 || in_dbg_master())
> return -EINVAL;
>
> + if ((drm_format_info_block_width(fb->format, 0) > 1) ||
> + (drm_format_info_block_height(fb->format, 0) > 1))
> + return -EINVAL;
> +
> /*
> * Changes struct fb_var_screeninfo are currently not pushed back
> * to KMS, hence fail if different settings are requested.
> @@ -1969,6 +1973,8 @@ void drm_fb_helper_fill_var(struct fb_info *info, struct drm_fb_helper *fb_helpe
> {
> struct drm_framebuffer *fb = fb_helper->fb;
>
> + WARN_ON((drm_format_info_block_width(fb->format, 0) > 1) ||
> + (drm_format_info_block_height(fb->format, 0) > 1));
> info->pseudo_palette = fb_helper->pseudo_palette;
> info->var.xres_virtual = fb->width;
> info->var.yres_virtual = fb->height;
> diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
> index a154763257ae..0c9725ffd5e9 100644
> --- a/drivers/gpu/drm/drm_fourcc.c
> +++ b/drivers/gpu/drm/drm_fourcc.c
> @@ -403,3 +403,43 @@ int drm_format_plane_height(int height, uint32_t format, int plane)
> return height / info->vsub;
> }
> EXPORT_SYMBOL(drm_format_plane_height);
> +
> +/**
> + * drm_format_info_block_width - width in pixels of block.
> + * @info: pixel format info
> + * @plane: plane index
> + *
> + * Returns:
> + * The width in pixels of a block, depending on the plane index.
> + */
> +unsigned int drm_format_info_block_width(const struct drm_format_info *info,
> + int plane)
> +{
> + if (!info || plane >= info->num_planes)
> + return 0;
> +
> + if (!info->block_w[plane])
> + return 1;
> + return info->block_w[plane];
> +}
> +EXPORT_SYMBOL(drm_format_info_block_width);
> +
> +/**
> + * drm_format_info_block_height - height in pixels of a block
> + * @info: pixel format info
> + * @plane: plane index
> + *
> + * Returns:
> + * The height in pixels of a block, depending on the plane index.
> + */
> +unsigned int drm_format_info_block_height(const struct drm_format_info *info,
> + int plane)
> +{
> + if (!info || plane >= info->num_planes)
> + return 0;
> +
> + if (!info->block_h[plane])
> + return 1;
> + return info->block_h[plane];
> +}
> +EXPORT_SYMBOL(drm_format_info_block_height);
> diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
> index 3bf729d0aae5..4e14013788cd 100644
> --- a/drivers/gpu/drm/drm_framebuffer.c
> +++ b/drivers/gpu/drm/drm_framebuffer.c
> @@ -195,20 +195,23 @@ static int framebuffer_check(struct drm_device *dev,
> for (i = 0; i < info->num_planes; i++) {
> unsigned int width = fb_plane_width(r->width, info, i);
> unsigned int height = fb_plane_height(r->height, info, i);
> - unsigned int cpp = info->cpp[i];
> + unsigned int block_size = info->char_per_block[i];
> + u64 min_pitch = DIV_ROUND_UP((u64)width * block_size,
> + drm_format_info_block_width(info, i) *
> + drm_format_info_block_height(info, i));
I think a helper to compute the drm_pitch, including some kernel-doc that
explains what exactly it is (and why) would be good. For cleaner code,
less duplication and to have a good spot for docs.
>
> if (!r->handles[i]) {
> DRM_DEBUG_KMS("no buffer object handle for plane %d\n", i);
> return -EINVAL;
> }
>
> - if ((uint64_t) width * cpp > UINT_MAX)
> + if (min_pitch > UINT_MAX)
> return -ERANGE;
>
> if ((uint64_t) height * r->pitches[i] + r->offsets[i] > UINT_MAX)
> return -ERANGE;
>
> - if (r->pitches[i] < width * cpp) {
> + if (r->pitches[i] < min_pitch) {
> DRM_DEBUG_KMS("bad pitch %u for plane %d\n", r->pitches[i], i);
> return -EINVAL;
> }
> diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> index ded7a379ac35..e342511370e7 100644
> --- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> +++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> @@ -171,7 +171,9 @@ drm_gem_fb_create_with_funcs(struct drm_device *dev, struct drm_file *file,
> }
>
> min_size = (height - 1) * mode_cmd->pitches[i]
> - + width * info->cpp[i]
> + + DIV_ROUND_UP((u64)width * info->char_per_block[i],
> + drm_format_info_block_width(info, i) *
> + drm_format_info_block_height(info, i))
> + mode_cmd->offsets[i];
>
> if (objs[i]->size < min_size) {
> diff --git a/include/drm/drm_fourcc.h b/include/drm/drm_fourcc.h
> index 865ef60c17af..305e80847abc 100644
> --- a/include/drm/drm_fourcc.h
> +++ b/include/drm/drm_fourcc.h
> @@ -58,6 +58,24 @@ struct drm_mode_fb_cmd2;
> * use in new code and set to 0 for new formats.
> * @num_planes: Number of color planes (1 to 3)
> * @cpp: Number of bytes per pixel (per plane)
Need to make it clear this aliases with @char_per_block and is deprecated.
Same for @char_per_block.
> + * @char_per_block: Number of bytes per block (per plane), where blocks are
> + * defined as a rectangle of pixels which are stored next to each other in
> + * a byte aligned memory region.
> + * Together with block_w and block_h this could be used to properly
s/could/is/
> + * describe tiles in tiled formats or to describe groups of pixels in
> + * packed formats for which the memory needed for a single pixel it's not
> + * byte aligned.
> + * Cpp had been kept from historical reasons because there are a lot of
s/Cpp/@cpp/ for proper highlighting in the output.
> + * places in drivers where it's used. In drm core for generic code paths
> + * the preferred way is to use char_per_block, drm_format_info_block_width
@char_per_block and drm_format_info_block_width() for hyperlinks. Similar
with all the other occurences of a function name.
> + * and drm_format_info_block_height which should allow handling both block
s/should// - I hope your code actually works :-)
> + * and non-block formats in the same way.
> + * For formats that are intended to be used just with modifiers, cpp/
> + * char_per_block must be 0.
This is a bit vague imo. What about:
"For formats that are intended to be used only with non-linear modifiers,
both @cpp and @char_per_block must be 0 in the generic format table.
Drivers should supply accurate information from their
&drm_mode_config.get_format_info hook though."
Since I'm assuming that once you know the exact modifier, you can enlist
the core format checking code to help validate it.
> + * @block_w: Block width in pixels, this is intended to be accessed through
> + * drm_format_info_block_width
Since you alias cpp and char_per_block, pls make it really, really clear
that if this isn't set, then the code uses 1 as the default.
> + * @block_h: Block height in pixels, this is intended to be accessed through
> + * drm_format_info_block_height
Same here.
Please use the inline kerneldoc style, it's much more readable for long
stuff like here. If the inconsistency irks you, then please just move them
all to the inline style. Inline style also allows you to have proper
paragraph breaks with empty lines.
> * @hsub: Horizontal chroma subsampling factor
> * @vsub: Vertical chroma subsampling factor
> * @has_alpha: Does the format embeds an alpha component?
> @@ -67,7 +85,12 @@ struct drm_format_info {
> u32 format;
> u8 depth;
> u8 num_planes;
> - u8 cpp[3];
> + union {
> + u8 cpp[3];
> + u8 char_per_block[3];
> + };
> + u8 block_w[3];
> + u8 block_h[3];
> u8 hsub;
> u8 vsub;
> bool has_alpha;
> @@ -96,6 +119,10 @@ int drm_format_horz_chroma_subsampling(uint32_t format);
> int drm_format_vert_chroma_subsampling(uint32_t format);
> int drm_format_plane_width(int width, uint32_t format, int plane);
> int drm_format_plane_height(int height, uint32_t format, int plane);
> +unsigned int drm_format_info_block_width(const struct drm_format_info *info,
> + int plane);
> +unsigned int drm_format_info_block_height(const struct drm_format_info *info,
> + int plane);
> const char *drm_get_format_name(uint32_t format, struct drm_format_name_buf *buf);
>
> #endif /* __DRM_FOURCC_H__ */
With the polish addressed, this lgtm.
One question I have is validating this. I think a bunch of unit tests,
integrated into the existing kms selftests we already (so that
intel-gfx-ci and other CI can run it) would be great. Both for the small
helper functions (block width/height, but especially drm_pitch), but also
for the drm_framebuffer_create functions, so that we can exercise the
metric pile of validation corner cases.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
