| Message ID | 20181005092606.21100-2-alexandru-cosmin.gheorghe@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add method to describe tile/bit_level_packed formats | expand |
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@arm.com> > Reviewed-by: Liviu Dudau <liviu.dudau@arm.com> > --- > 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
Hi Daniel, Thanks for having a look. On Fri, Oct 05, 2018 at 04:51:48PM +0200, Daniel Vetter wrote: > 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@arm.com> > > Reviewed-by: Liviu Dudau <liviu.dudau@arm.com> > > --- > > 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. Stark3y had the same suggestion during our internal review :). Does this sounds good to you? drm_format_info_min_pitch(info, plane, buffer_width) > > > > > 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. All of them are reasonable to me, so I'll apply them in the next version. > > 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 So, you are thinking of adding tests here drivers/gpu/drm/selftests/ ? Looking inside the folder, it seems more like a stub than proper test suite for the drm framework, or am I missing something ? I did run tests for all supported formats by the mali-dp, with our internal testsuite and everything was OK for all the fourcc enabled in mali-dp I'm planning to run the igt on top of mali-dp, just to double check I didn't change any of the behaviours. Any idea if there is any public igt CI that I could point to a kernel branch, to test this change on something else other than mali-dp? Shouldn't that be enough for validating this patches? > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
On Mon, Oct 08, 2018 at 09:52:04AM +0000, Alexandru-Cosmin Gheorghe wrote: > Hi Daniel, > > Thanks for having a look. > > On Fri, Oct 05, 2018 at 04:51:48PM +0200, Daniel Vetter wrote: > > 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@arm.com> > > > Reviewed-by: Liviu Dudau <liviu.dudau@arm.com> > > > --- > > > 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. > > Stark3y had the same suggestion during our internal review :). Can we do more of that internal review in public please? Adding r-b tags from internal stuff where there's 0 reasons for internal review (it's not an embargoed feature) isn't great. If the issue is that you can't find your stuff on dri-devel, then maybe create your own private turf like amd/intel have. But I think arm is small enough still that this doesn't make any sense. > Does this sounds good to you? > drm_format_info_min_pitch(info, plane, buffer_width) Ack. > > > 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. > > All of them are reasonable to me, so I'll apply them in the next > version. > > > > > 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 > > So, you are thinking of adding tests here drivers/gpu/drm/selftests/ ? > Looking inside the folder, it seems more like a stub than proper > test suite for the drm framework, or am I missing something ? It's indeed very incomplete still. > I did run tests for all supported formats by the mali-dp, with our > internal testsuite and everything was OK for all the fourcc enabled > in mali-dp Your internal test suite is to no value to the overall community :-) Can we get those upstreamed somewhere, ideally as part of igt? > I'm planning to run the igt on top of mali-dp, just to double check I > didn't change any of the behaviours. Any idea if there is any public > igt CI that I could point to a kernel branch, to test this change on > something else other than mali-dp? Atm only way to get it to pre-merge test stuff is Cc: intel-gfx m-l. > Shouldn't that be enough for validating this patches? I don't think so, right now we don't have specific tests for e.g. the new min_pitch function you're adding. Igt is still fairly far away from providing full coverage. And it can't provide coverage for new features ofc, without someone adding those tests. Also, I'm not worried about you validating this correctly, that's kinda your problem. I'm worried about keeping things working going forward, and for that we need: - public tests - run in a public CI Yes I know for the 2nd point intel-gfx-ci isn't there yet, but my long-term plan is to run all the generic igt tests plus kernel self-tests on virtual machines (on top of vkms+vgem) in gitlab CI. Cheers, Daniel
On Thu, Oct 11, 2018 at 10:29:42AM +0200, Daniel Vetter wrote: > On Mon, Oct 08, 2018 at 09:52:04AM +0000, Alexandru-Cosmin Gheorghe wrote: > > Hi Daniel, > > > > Thanks for having a look. > > > > On Fri, Oct 05, 2018 at 04:51:48PM +0200, Daniel Vetter wrote: > > > 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@arm.com> > > > > Reviewed-by: Liviu Dudau <liviu.dudau@arm.com> > > > > --- > > > > 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. > > > > Stark3y had the same suggestion during our internal review :). > > Can we do more of that internal review in public please? Adding r-b tags > from internal stuff where there's 0 reasons for internal review (it's not > an embargoed feature) isn't great. I'm in the same room with both Liviu and Stark3y, so it's hard to avoid internal review before going public. I was in doubt if I should add r-b tags, probably I should've ommit them and leave that come trough the mailing list. > > If the issue is that you can't find your stuff on dri-devel, then maybe > create your own private turf like amd/intel have. But I think arm is small > enough still that this doesn't make any sense. I agree, there is no need for a private turf. > > > > Does this sounds good to you? > > drm_format_info_min_pitch(info, plane, buffer_width) > > Ack. > > > > > 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. > > > > All of them are reasonable to me, so I'll apply them in the next > > version. > > > > > > > > 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 > > > > So, you are thinking of adding tests here drivers/gpu/drm/selftests/ ? > > Looking inside the folder, it seems more like a stub than proper > > test suite for the drm framework, or am I missing something ? > > It's indeed very incomplete still. > > > I did run tests for all supported formats by the mali-dp, with our > > internal testsuite and everything was OK for all the fourcc enabled > > in mali-dp > > Your internal test suite is to no value to the overall community :-) Can > we get those upstreamed somewhere, ideally as part of igt? I agree with you that having one single test suite would make our life easier. Hoewever, the way our internal test suite evolved means that integrating it to igt would basically mean writing it from scratch, so it's hard to justify the effort. > > > I'm planning to run the igt on top of mali-dp, just to double check I > > didn't change any of the behaviours. Any idea if there is any public > > igt CI that I could point to a kernel branch, to test this change on > > something else other than mali-dp? > > Atm only way to get it to pre-merge test stuff is Cc: intel-gfx m-l. > > > Shouldn't that be enough for validating this patches? > > I don't think so, right now we don't have specific tests for e.g. the new > min_pitch function you're adding. Igt is still fairly far away from > providing full coverage. And it can't provide coverage for new features > ofc, without someone adding those tests. Proabably drm/selftests makes more sense for the drm_format_info* helpers I'm hoping is relatively painless to add, I'm not very sure about check_framebuffer and drm_framebuffer_create, but I'll have a look into it. > > Also, I'm not worried about you validating this correctly, that's kinda > your problem. I'm worried about keeping things working going forward, and > for that we need: > - public tests > - run in a public CI > > Yes I know for the 2nd point intel-gfx-ci isn't there yet, but my > long-term plan is to run all the generic igt tests plus kernel self-tests > on virtual machines (on top of vkms+vgem) in gitlab CI. > > Cheers, Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
On Thu, Oct 11, 2018 at 11:58 AM Alexandru-Cosmin Gheorghe <Alexandru-Cosmin.Gheorghe@arm.com> wrote: > > On Thu, Oct 11, 2018 at 10:29:42AM +0200, Daniel Vetter wrote: > > On Mon, Oct 08, 2018 at 09:52:04AM +0000, Alexandru-Cosmin Gheorghe wrote: > > > Shouldn't that be enough for validating this patches? > > > > I don't think so, right now we don't have specific tests for e.g. the new > > min_pitch function you're adding. Igt is still fairly far away from > > providing full coverage. And it can't provide coverage for new features > > ofc, without someone adding those tests. > > Proabably drm/selftests makes more sense for the drm_format_info* > helpers I'm hoping is relatively painless to add, I'm not very sure > about check_framebuffer and drm_framebuffer_create, but I'll have a > look into it. This might help: https://www.spinics.net/lists/dri-devel/msg191131.html and on the igt side: https://lists.freedesktop.org/archives/igt-dev/2018-October/006257.html I've already suggested we rename that bucket of self-tests to drm_kms, so that we can stuff all kinds of kms related selftests in there. Instead of having to wire up the scaffolding for all of them individually. -Daniel
On Thu, Oct 11, 2018 at 10:58:30AM +0100, Alexandru-Cosmin Gheorghe wrote: > On Thu, Oct 11, 2018 at 10:29:42AM +0200, Daniel Vetter wrote: > > On Mon, Oct 08, 2018 at 09:52:04AM +0000, Alexandru-Cosmin Gheorghe wrote: > > > Hi Daniel, > > > > > > Thanks for having a look. > > > > > > On Fri, Oct 05, 2018 at 04:51:48PM +0200, Daniel Vetter wrote: > > > > 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@arm.com> > > > > > Reviewed-by: Liviu Dudau <liviu.dudau@arm.com> > > > > > --- > > > > > 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. > > > > > > Stark3y had the same suggestion during our internal review :). > > > > Can we do more of that internal review in public please? Adding r-b tags > > from internal stuff where there's 0 reasons for internal review (it's not > > an embargoed feature) isn't great. > > I'm in the same room with both Liviu and Stark3y, so it's hard to > avoid internal review before going public. I was in doubt if I should add > r-b tags, probably I should've ommit them and leave that come trough the > mailing list. Just to add a bit of clarification here, we're usually not doing internal reviews for public patches, this was more like Alex doing an internal RFC series because it was more convenient with the travel and people's calendars to do it that way, and as a result he added the R-b (which I agree, could've been (re)collected in the public ML). > > > > > If the issue is that you can't find your stuff on dri-devel, then maybe > > create your own private turf like amd/intel have. But I think arm is small > > enough still that this doesn't make any sense. > > I agree, there is no need for a private turf. > > > > > > > > Does this sounds good to you? > > > drm_format_info_min_pitch(info, plane, buffer_width) > > > > Ack. > > > > > > > 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. > > > > > > All of them are reasonable to me, so I'll apply them in the next > > > version. > > > > > > > > > > > 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 > > > > > > So, you are thinking of adding tests here drivers/gpu/drm/selftests/ ? > > > Looking inside the folder, it seems more like a stub than proper > > > test suite for the drm framework, or am I missing something ? > > > > It's indeed very incomplete still. > > > > > I did run tests for all supported formats by the mali-dp, with our > > > internal testsuite and everything was OK for all the fourcc enabled > > > in mali-dp > > > > Your internal test suite is to no value to the overall community :-) Can > > we get those upstreamed somewhere, ideally as part of igt? On balance of things, how many more drivers are we going to cover when going from internal test suite to igt? +1 (intel)? +3 (finger in the air)? By looking at the commit log is hard to judge the traction of igt in the "overall community". Best regards, Liviu > > I agree with you that having one single test suite would make our life > easier. Hoewever, the way our internal test suite evolved means that > integrating it to igt would basically mean writing it from scratch, so > it's hard to justify the effort. > > > > > > I'm planning to run the igt on top of mali-dp, just to double check I > > > didn't change any of the behaviours. Any idea if there is any public > > > igt CI that I could point to a kernel branch, to test this change on > > > something else other than mali-dp? > > > > Atm only way to get it to pre-merge test stuff is Cc: intel-gfx m-l. > > > > > Shouldn't that be enough for validating this patches? > > > > I don't think so, right now we don't have specific tests for e.g. the new > > min_pitch function you're adding. Igt is still fairly far away from > > providing full coverage. And it can't provide coverage for new features > > ofc, without someone adding those tests. > > Proabably drm/selftests makes more sense for the drm_format_info* > helpers I'm hoping is relatively painless to add, I'm not very sure > about check_framebuffer and drm_framebuffer_create, but I'll have a > look into it. > > > > > Also, I'm not worried about you validating this correctly, that's kinda > > your problem. I'm worried about keeping things working going forward, and > > for that we need: > > - public tests > > - run in a public CI > > > > Yes I know for the 2nd point intel-gfx-ci isn't there yet, but my > > long-term plan is to run all the generic igt tests plus kernel self-tests > > on virtual machines (on top of vkms+vgem) in gitlab CI. > > > > Cheers, Daniel > > -- > > Daniel Vetter > > Software Engineer, Intel Corporation > > http://blog.ffwll.ch > > -- > Cheers, > Alex G
On Thu, Oct 11, 2018 at 12:11 PM Liviu Dudau <Liviu.Dudau@arm.com> wrote: > On Thu, Oct 11, 2018 at 10:58:30AM +0100, Alexandru-Cosmin Gheorghe wrote: > > On Thu, Oct 11, 2018 at 10:29:42AM +0200, Daniel Vetter wrote: > > > On Mon, Oct 08, 2018 at 09:52:04AM +0000, Alexandru-Cosmin Gheorghe wrote: > > > > > 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 > > > > > > > > So, you are thinking of adding tests here drivers/gpu/drm/selftests/ ? > > > > Looking inside the folder, it seems more like a stub than proper > > > > test suite for the drm framework, or am I missing something ? > > > > > > It's indeed very incomplete still. > > > > > > > I did run tests for all supported formats by the mali-dp, with our > > > > internal testsuite and everything was OK for all the fourcc enabled > > > > in mali-dp > > > > > > Your internal test suite is to no value to the overall community :-) Can > > > we get those upstreamed somewhere, ideally as part of igt? > > On balance of things, how many more drivers are we going to cover when > going from internal test suite to igt? +1 (intel)? +3 (finger in the > air)? By looking at the commit log is hard to judge the traction of igt in > the "overall community". With my intel hat on I don't particularly care how you test mali, and I don't think you folks care how we test intel. (Ok maybe there's a shared interest in having high quality upstream drivers so that OS people can upgrade without fear of regressions across any of the hw vendors, but that's a long story and somewhat an aside). But there's piles of shared infrastructure, and that's really where I think a common test suite, ideally run as part of drm core CI (I'm working on that, doesn't exist yet). That's where the shared CI efforts really pay off imo. There's also some value in trying to make sure all drivers work the same across all drivers. Both of these pieces need to be there to be able to refactor the drm subsystem aggressively, which we'll need to do because hw is changing all the time. If you don't care about any of this, then pushing your driver to upstream doesn't make a hole lot of sense imo :-) Cheers, Daniel
On Thu, Oct 11, 2018 at 02:40:10PM +0200, Daniel Vetter wrote: > On Thu, Oct 11, 2018 at 12:11 PM Liviu Dudau <Liviu.Dudau@arm.com> wrote: > > On Thu, Oct 11, 2018 at 10:58:30AM +0100, Alexandru-Cosmin Gheorghe wrote: > > > On Thu, Oct 11, 2018 at 10:29:42AM +0200, Daniel Vetter wrote: > > > > On Mon, Oct 08, 2018 at 09:52:04AM +0000, Alexandru-Cosmin Gheorghe wrote: > > > > > > 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 > > > > > > > > > > So, you are thinking of adding tests here drivers/gpu/drm/selftests/ ? > > > > > Looking inside the folder, it seems more like a stub than proper > > > > > test suite for the drm framework, or am I missing something ? > > > > > > > > It's indeed very incomplete still. > > > > > > > > > I did run tests for all supported formats by the mali-dp, with our > > > > > internal testsuite and everything was OK for all the fourcc enabled > > > > > in mali-dp > > > > > > > > Your internal test suite is to no value to the overall community :-) Can > > > > we get those upstreamed somewhere, ideally as part of igt? > > > > On balance of things, how many more drivers are we going to cover when > > going from internal test suite to igt? +1 (intel)? +3 (finger in the > > air)? By looking at the commit log is hard to judge the traction of igt in > > the "overall community". > > With my intel hat on I don't particularly care how you test mali, and > I don't think you folks care how we test intel. (Ok maybe there's a > shared interest in having high quality upstream drivers so that OS > people can upgrade without fear of regressions across any of the hw > vendors, but that's a long story and somewhat an aside). > > But there's piles of shared infrastructure, and that's really where I > think a common test suite, ideally run as part of drm core CI (I'm > working on that, doesn't exist yet). That's where the shared CI > efforts really pay off imo. There's also some value in trying to make > sure all drivers work the same across all drivers. Both of these > pieces need to be there to be able to refactor the drm subsystem > aggressively, which we'll need to do because hw is changing all the > time. If you don't care about any of this, then pushing your driver to > upstream doesn't make a hole lot of sense imo :-) Nah, I wasn't looking to start a holy war, it was a genuine question. I don't remember exactly the timeline but I do admit that I was a bit puzzled (confused?) by the appearance of drivers/gpu/drm/selftests given the existence of igt, so I though one of them is the "newer" thing to support. I was on the fence on which one is worth backing, hence the quantitative question (how many more drivers actively get tested with igt?). I know that I have used igt to test mali-dp before submission, we also had some patches that need refreshing to add writeback support (sorry for being slow on those), but I haven't seen people contributing in earnest to igt support for their drivers (vc4 is the newest, amdgpu from a few months ago, I thought there was some adreno stuff but I can't find right now). So for the passer-bys (aka managers) it is hard (sometimes) to see the benefit of investing effort in something that doesn't look like a clear winner. Maybe igt could use some "Supported drivers" section in the README so we can point it out to our decision makers? Best regards, Liviu > > Cheers, Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Thu, Oct 11, 2018 at 2:53 PM Liviu Dudau <Liviu.Dudau@arm.com> wrote: > On Thu, Oct 11, 2018 at 02:40:10PM +0200, Daniel Vetter wrote: > > On Thu, Oct 11, 2018 at 12:11 PM Liviu Dudau <Liviu.Dudau@arm.com> wrote: > > > On Thu, Oct 11, 2018 at 10:58:30AM +0100, Alexandru-Cosmin Gheorghe wrote: > > > > On Thu, Oct 11, 2018 at 10:29:42AM +0200, Daniel Vetter wrote: > > > > > On Mon, Oct 08, 2018 at 09:52:04AM +0000, Alexandru-Cosmin Gheorghe wrote: > > > > > > > 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 > > > > > > > > > > > > So, you are thinking of adding tests here drivers/gpu/drm/selftests/ ? > > > > > > Looking inside the folder, it seems more like a stub than proper > > > > > > test suite for the drm framework, or am I missing something ? > > > > > > > > > > It's indeed very incomplete still. > > > > > > > > > > > I did run tests for all supported formats by the mali-dp, with our > > > > > > internal testsuite and everything was OK for all the fourcc enabled > > > > > > in mali-dp > > > > > > > > > > Your internal test suite is to no value to the overall community :-) Can > > > > > we get those upstreamed somewhere, ideally as part of igt? > > > > > > On balance of things, how many more drivers are we going to cover when > > > going from internal test suite to igt? +1 (intel)? +3 (finger in the > > > air)? By looking at the commit log is hard to judge the traction of igt in > > > the "overall community". > > > > With my intel hat on I don't particularly care how you test mali, and > > I don't think you folks care how we test intel. (Ok maybe there's a > > shared interest in having high quality upstream drivers so that OS > > people can upgrade without fear of regressions across any of the hw > > vendors, but that's a long story and somewhat an aside). > > > > But there's piles of shared infrastructure, and that's really where I > > think a common test suite, ideally run as part of drm core CI (I'm > > working on that, doesn't exist yet). That's where the shared CI > > efforts really pay off imo. There's also some value in trying to make > > sure all drivers work the same across all drivers. Both of these > > pieces need to be there to be able to refactor the drm subsystem > > aggressively, which we'll need to do because hw is changing all the > > time. If you don't care about any of this, then pushing your driver to > > upstream doesn't make a hole lot of sense imo :-) > > Nah, I wasn't looking to start a holy war, it was a genuine question. I > don't remember exactly the timeline but I do admit that I was a bit > puzzled (confused?) by the appearance of drivers/gpu/drm/selftests given > the existence of igt, so I though one of them is the "newer" thing to > support. I was on the fence on which one is worth backing, hence the > quantitative question (how many more drivers actively get tested with > igt?). They're two sides of the same coin really: - igt does (mostly) blackbox testing from userspace, with very minimal ties into the driver through debugfs. - selftests run within the kernel, have deep knowledge of the implementation (not just the uapi), and are mostly unit tests of individual bits&pieces. Currently we need some minimal wrappers in igt to be able to run the kernel selftests using igt (so that it's all under one overall testing umbrella). But the ksefltests can also be run without igt. Doing the selftests in igt would unnecessarily tie igt to the kernel, a massive maintenance headaches. And we want to be able to run igt uapi tests on older/newer kernels, so that we can make sure we don't break back/forward-compatibility. So merging igt into the kernel (which some othe tests-suites have done to make maintenanace easier) doesn't make much sense imo. Depending upon what you're doing it's better to pick igt or selftests (or both, for different aspects) as the best way to create regression-tests for your new feature. For entirely internal work (like the new block_* description for pixel formats) I think in-kernel unit-tests are the most effective way to regression test them, hence my recommendation to look into selftests. For new uapi you generally want some igts, but perhaps augmented with self-tests for tricky internal implementation details. That was my recommendation for the dirty rectangle, where the validation code is an igt testcase and a bunch of self-tests. > I know that I have used igt to test mali-dp before submission, we also > had some patches that need refreshing to add writeback support (sorry > for being slow on those), but I haven't seen people contributing in > earnest to igt support for their drivers (vc4 is the newest, amdgpu from > a few months ago, I thought there was some adreno stuff but I can't find > right now). So for the passer-bys (aka managers) it is hard (sometimes) > to see the benefit of investing effort in something that doesn't look > like a clear winner. Igt is supposed to run on any kms driver without changes. There's no need to add special code in there. The vc4/amdgpu/freedreno code is for the gem side, where you do need special code to test-drive the driver-private ioctl interfaces. Now of course it's not perfect, and occasionally we need to remove some intel-ism. But e.g. just recently vmwgfx people made a small patch series to be able to run igt on vmwgfx, and it was very minimal. And vmwgfx is probably the most "special" kms driver we have in-tree, so I think we're doing a fairly good job overall. > Maybe igt could use some "Supported drivers" section in the README so we > can point it out to our decision makers? All of them, or at least all the kms ones? Or are you more looking for "actively supported", as in people running them in some CI regularly? The answer for the later I frankly don't even know ... -Daniel
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)); 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) + * @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 + * 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 + * 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 + * and drm_format_info_block_height which should allow handling both block + * 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. + * @block_w: Block width in pixels, this is intended to be accessed through + * drm_format_info_block_width + * @block_h: Block height in pixels, this is intended to be accessed through + * drm_format_info_block_height * @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__ */