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@xxxxxxx>
> > Reviewed-by: Liviu Dudau <liviu.dudau@xxxxxxx>
> > ---
> > drivers/gpu/drm/drm_fb_cma_helper.c | 21 ++++++++--
> > drivers/gpu/drm/drm_fb_helper.c | 6 +++
> > drivers/gpu/drm/drm_fourcc.c | 40 ++++++++++++++++++++
> > drivers/gpu/drm/drm_framebuffer.c | 9 +++--
> > drivers/gpu/drm/drm_gem_framebuffer_helper.c | 4 +-
> > include/drm/drm_fourcc.h | 29 +++++++++++++-
> > 6 files changed, 101 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c
> > index 47e0e2f6642d..f8293f4d96cf 100644
> > --- a/drivers/gpu/drm/drm_fb_cma_helper.c
> > +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
> > @@ -72,7 +72,9 @@ struct drm_gem_cma_object *drm_fb_cma_get_gem_obj(struct drm_framebuffer *fb,
> > EXPORT_SYMBOL_GPL(drm_fb_cma_get_gem_obj);
> >
> > /**
> > - * drm_fb_cma_get_gem_addr() - Get physical address for framebuffer
> > + * drm_fb_cma_get_gem_addr() - Get physical address for framebuffer, for pixel
> > + * formats where values are grouped in blocks this will get you the beginning of
> > + * the block
> > * @fb: The framebuffer
> > * @state: Which state of drm plane
> > * @plane: Which plane
> > @@ -87,6 +89,14 @@ dma_addr_t drm_fb_cma_get_gem_addr(struct drm_framebuffer *fb,
> > struct drm_gem_cma_object *obj;
> > dma_addr_t paddr;
> > u8 h_div = 1, v_div = 1;
> > + u32 block_w = drm_format_info_block_width(fb->format, plane);
> > + u32 block_h = drm_format_info_block_height(fb->format, plane);
> > + u32 block_size = fb->format->char_per_block[plane];
> > + u32 sample_x;
> > + u32 sample_y;
> > + u32 block_start_y;
> > + u32 num_hblocks;
> > +
> >
> > obj = drm_fb_cma_get_gem_obj(fb, plane);
> > if (!obj)
> > @@ -99,8 +109,13 @@ dma_addr_t drm_fb_cma_get_gem_addr(struct drm_framebuffer *fb,
> > v_div = fb->format->vsub;
> > }
> >
> > - paddr += (fb->format->cpp[plane] * (state->src_x >> 16)) / h_div;
> > - paddr += (fb->pitches[plane] * (state->src_y >> 16)) / v_div;
> > + sample_x = (state->src_x >> 16) / h_div;
> > + sample_y = (state->src_y >> 16) / v_div;
> > + block_start_y = (sample_y / block_h) * block_h;
> > + num_hblocks = sample_x / block_w;
> > +
> > + paddr += fb->pitches[plane] * block_start_y;
> > + paddr += block_size * num_hblocks;
> >
> > return paddr;
> > }
> > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> > index a504a5e05676..9add0d7da744 100644
> > --- a/drivers/gpu/drm/drm_fb_helper.c
> > +++ b/drivers/gpu/drm/drm_fb_helper.c
> > @@ -1595,6 +1595,10 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
> > if (var->pixclock != 0 || in_dbg_master())
> > return -EINVAL;
> >
> > + if ((drm_format_info_block_width(fb->format, 0) > 1) ||
> > + (drm_format_info_block_height(fb->format, 0) > 1))
> > + return -EINVAL;
> > +
> > /*
> > * Changes struct fb_var_screeninfo are currently not pushed back
> > * to KMS, hence fail if different settings are requested.
> > @@ -1969,6 +1973,8 @@ void drm_fb_helper_fill_var(struct fb_info *info, struct drm_fb_helper *fb_helpe
> > {
> > struct drm_framebuffer *fb = fb_helper->fb;
> >
> > + WARN_ON((drm_format_info_block_width(fb->format, 0) > 1) ||
> > + (drm_format_info_block_height(fb->format, 0) > 1));
> > info->pseudo_palette = fb_helper->pseudo_palette;
> > info->var.xres_virtual = fb->width;
> > info->var.yres_virtual = fb->height;
> > diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
> > index a154763257ae..0c9725ffd5e9 100644
> > --- a/drivers/gpu/drm/drm_fourcc.c
> > +++ b/drivers/gpu/drm/drm_fourcc.c
> > @@ -403,3 +403,43 @@ int drm_format_plane_height(int height, uint32_t format, int plane)
> > return height / info->vsub;
> > }
> > EXPORT_SYMBOL(drm_format_plane_height);
> > +
> > +/**
> > + * drm_format_info_block_width - width in pixels of block.
> > + * @info: pixel format info
> > + * @plane: plane index
> > + *
> > + * Returns:
> > + * The width in pixels of a block, depending on the plane index.
> > + */
> > +unsigned int drm_format_info_block_width(const struct drm_format_info *info,
> > + int plane)
> > +{
> > + if (!info || plane >= info->num_planes)
> > + return 0;
> > +
> > + if (!info->block_w[plane])
> > + return 1;
> > + return info->block_w[plane];
> > +}
> > +EXPORT_SYMBOL(drm_format_info_block_width);
> > +
> > +/**
> > + * drm_format_info_block_height - height in pixels of a block
> > + * @info: pixel format info
> > + * @plane: plane index
> > + *
> > + * Returns:
> > + * The height in pixels of a block, depending on the plane index.
> > + */
> > +unsigned int drm_format_info_block_height(const struct drm_format_info *info,
> > + int plane)
> > +{
> > + if (!info || plane >= info->num_planes)
> > + return 0;
> > +
> > + if (!info->block_h[plane])
> > + return 1;
> > + return info->block_h[plane];
> > +}
> > +EXPORT_SYMBOL(drm_format_info_block_height);
> > diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
> > index 3bf729d0aae5..4e14013788cd 100644
> > --- a/drivers/gpu/drm/drm_framebuffer.c
> > +++ b/drivers/gpu/drm/drm_framebuffer.c
> > @@ -195,20 +195,23 @@ static int framebuffer_check(struct drm_device *dev,
> > for (i = 0; i < info->num_planes; i++) {
> > unsigned int width = fb_plane_width(r->width, info, i);
> > unsigned int height = fb_plane_height(r->height, info, i);
> > - unsigned int cpp = info->cpp[i];
> > + unsigned int block_size = info->char_per_block[i];
> > + u64 min_pitch = DIV_ROUND_UP((u64)width * block_size,
> > + drm_format_info_block_width(info, i) *
> > + drm_format_info_block_height(info, i));
>
> I think a helper to compute the drm_pitch, including some kernel-doc that
> explains what exactly it is (and why) would be good. For cleaner code,
> less duplication and to have a good spot for docs.
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
--
Cheers,
Alex G
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
