Messages in this thread | | | Date | Mon, 10 Sep 2018 09:50:03 +0100 | From | Brian Starkey <> | Subject | Re: [RFC PATCH v2 1/3] drm/fourcc: Add 'bpp' field for formats with non-integer bytes-per-pixel |
| |
Hi,
On Fri, Sep 07, 2018 at 09:28:44PM +0200, Daniel Vetter wrote: >On Fri, Sep 07, 2018 at 01:45:36PM +0100, Brian Starkey wrote: >> Hi Daniel, >> >> On Fri, Aug 31, 2018 at 10:17:30AM +0200, Daniel Vetter wrote: >> > On Thu, Aug 23, 2018 at 04:23:41PM +0100, Brian Starkey wrote: >> > > Some formats have a non-integer number of bytes per pixel, which can't >> > > be handled with the existing 'cpp' field in drm_format_info. To handle >> > > these formats, add a 'bpp' field, which is only used if cpp[0] == 0. >> > > >> > > This updates all the users of format->cpp in the core DRM code, >> > > converting them to use a new function to get the bits-per-pixel for any >> > > format. >> > > >> > > It's assumed that drivers will use the 'bpp' field when they add support >> > > for pixel formats with non-integer bytes-per-pixel. >> > > >> > > Signed-off-by: Brian Starkey <brian.starkey@arm.com> >> > >> > I assume you still require that stuff is eventually aligned to bytes? In >> > that case, can we subsume this into the tile work Alex is doing? It's >> > essentially just another special case of having storage-size units >> > measured in bytes which span more than 1x1 pixel. And I kinda don't want a >> > metric pile of special cases here in the format code, because that just >> > means every driver handles a different subset, with different bugs. >> > -Daniel >> >> Sorry for the delay, been struggling to free some cycles to think >> about this. >> >> I'm not sure how to pull this in with the tiling stuff. In the AFBC >> case then our AFBC superblocks are always nice round numbers (256 >> pixels), and so it does end up being a multiple of bytes. >> >> However, AFBC supports different superblock sizes, so picking just one >> doesn't really work out, and putting AFBC in the core format table >> which reflects AFBC doesn't seem good. >> >> We could make something up (e.g. call these formats "tiled" with 2x4 >> tiles, which guarantees a multiple of 8), but it would be an >> arbitrarily-selected lie, which often seems to spell trouble. If we >> did do that, would you re-define cpp as "bytes-per-tile"? Otherwise >> we still need to add a new field anyway. >> >> What's the pile of special cases you're worried about? The helper I've >> added here means that drivers which need to care can use one API and >> not implement their own bugs. > >I'm confused ... the new bits-per-pixel stuff you're adding here is for >yuv formats, not afbc. I'm just suggesting we have only 1 way of >describing such formats that need more descriptive power than cpp, whether >they have some kind of pixel-groups or small tiles.
Well, not really. The three formats which have non-integer cpp are: DRM_FORMAT_VUY101010, DRM_FORMAT_YUV420_8BIT and DRM_FORMAT_YUV420_10BIT. These formats are only valid with non-linear modifiers (no linear encoding is defined). Mali only supports them with AFBC.
The formats themselves have no notion of tiling or grouping - the modifier adds that. I'm not aware of any non-AFBC uses of these formats, so I don't want to "make up" a small-tile layout restriction for them.
> >For very special stuff like afbc you need to validate in the driver >anyway, too complicated. So I have no idea why you bring this up here?
Sure, we can just let drivers provide their own format_info's for these, if that's what you prefer. The core format checking code can error out if it ever encounters them.
Cheers, -Brian
>-Daniel > >> >> Cheers, >> -Brian >> >> > >> > > --- >> > > drivers/gpu/drm/drm_fb_cma_helper.c | 6 +++- >> > > drivers/gpu/drm/drm_fb_helper.c | 8 +++-- >> > > drivers/gpu/drm/drm_fourcc.c | 50 ++++++++++++++++++++++++++++ >> > > drivers/gpu/drm/drm_framebuffer.c | 8 ++--- >> > > drivers/gpu/drm/drm_gem_framebuffer_helper.c | 3 +- >> > > include/drm/drm_fourcc.h | 4 +++ >> > > 6 files changed, 70 insertions(+), 9 deletions(-) >> > > >> > > diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c >> > > index 186d00adfb5f..e279d70d3e60 100644 >> > > --- a/drivers/gpu/drm/drm_fb_cma_helper.c >> > > +++ b/drivers/gpu/drm/drm_fb_cma_helper.c >> > > @@ -118,13 +118,17 @@ dma_addr_t drm_fb_cma_get_gem_addr(struct drm_framebuffer *fb, >> > > { >> > > struct drm_gem_cma_object *obj; >> > > dma_addr_t paddr; >> > > + u8 bpp = drm_format_info_plane_bpp(fb->format, plane); >> > > + >> > > + /* This can't work for non-integer bytes-per-pixel */ >> > > + WARN_ON(bpp % 8); >> > > >> > > obj = drm_fb_cma_get_gem_obj(fb, plane); >> > > if (!obj) >> > > return 0; >> > > >> > > paddr = obj->paddr + fb->offsets[plane]; >> > > - paddr += fb->format->cpp[plane] * (state->src_x >> 16); >> > > + paddr += (bpp / 8) * (state->src_x >> 16); >> > > paddr += fb->pitches[plane] * (state->src_y >> 16); >> > > >> > > return paddr; >> > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c >> > > index 0646b108030b..ab369f250af4 100644 >> > > --- a/drivers/gpu/drm/drm_fb_helper.c >> > > +++ b/drivers/gpu/drm/drm_fb_helper.c >> > > @@ -1572,6 +1572,7 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var, >> > > struct drm_fb_helper *fb_helper = info->par; >> > > struct drm_framebuffer *fb = fb_helper->fb; >> > > int depth; >> > > + u8 bpp = drm_format_info_plane_bpp(fb->format, 0); >> > > >> > > if (var->pixclock != 0 || in_dbg_master()) >> > > return -EINVAL; >> > > @@ -1580,14 +1581,14 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var, >> > > * Changes struct fb_var_screeninfo are currently not pushed back >> > > * to KMS, hence fail if different settings are requested. >> > > */ >> > > - if (var->bits_per_pixel != fb->format->cpp[0] * 8 || >> > > + if (var->bits_per_pixel != bpp || >> > > var->xres > fb->width || var->yres > fb->height || >> > > var->xres_virtual > fb->width || var->yres_virtual > fb->height) { >> > > DRM_DEBUG("fb requested width/height/bpp can't fit in current fb " >> > > "request %dx%d-%d (virtual %dx%d) > %dx%d-%d\n", >> > > var->xres, var->yres, var->bits_per_pixel, >> > > var->xres_virtual, var->yres_virtual, >> > > - fb->width, fb->height, fb->format->cpp[0] * 8); >> > > + fb->width, fb->height, bpp); >> > > return -EINVAL; >> > > } >> > > >> > > @@ -1949,11 +1950,12 @@ void drm_fb_helper_fill_var(struct fb_info *info, struct drm_fb_helper *fb_helpe >> > > uint32_t fb_width, uint32_t fb_height) >> > > { >> > > struct drm_framebuffer *fb = fb_helper->fb; >> > > + u8 bpp = drm_format_info_plane_bpp(fb->format, 0); >> > > >> > > info->pseudo_palette = fb_helper->pseudo_palette; >> > > info->var.xres_virtual = fb->width; >> > > info->var.yres_virtual = fb->height; >> > > - info->var.bits_per_pixel = fb->format->cpp[0] * 8; >> > > + info->var.bits_per_pixel = bpp; >> > > info->var.accel_flags = FB_ACCELF_TEXT; >> > > info->var.xoffset = 0; >> > > info->var.yoffset = 0; >> > > diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c >> > > index 3b42c25bd58d..bb28919c32f3 100644 >> > > --- a/drivers/gpu/drm/drm_fourcc.c >> > > +++ b/drivers/gpu/drm/drm_fourcc.c >> > > @@ -272,10 +272,60 @@ int drm_format_plane_cpp(uint32_t format, int plane) >> > > if (!info || plane >= info->num_planes) >> > > return 0; >> > > >> > > + /* >> > > + * Not valid for formats with non-integer cpp, >> > > + * use drm_format{_info}_plane_bpp instead >> > > + */ >> > > + WARN_ON(!info->cpp[0]); >> > > + >> > > return info->cpp[plane]; >> > > } >> > > EXPORT_SYMBOL(drm_format_plane_cpp); >> > > >> > > +/** >> > > + * drm_format_plane_bpp - determine the bits per pixel value >> > > + * @format: pixel format (DRM_FORMAT_*) >> > > + * @plane: plane index >> > > + * >> > > + * Returns: >> > > + * The bits per pixel value for the specified plane. >> > > + */ >> > > +int drm_format_plane_bpp(uint32_t format, int plane) >> > > +{ >> > > + const struct drm_format_info *info; >> > > + >> > > + info = drm_format_info(format); >> > > + if (!info) >> > > + return 0; >> > > + >> > > + return drm_format_info_plane_bpp(info, plane); >> > > +} >> > > +EXPORT_SYMBOL(drm_format_plane_bpp); >> > > + >> > > +/** >> > > + * drm_format_info_plane_bpp - determine the bits per pixel value >> > > + * >> > > + * Convenience function which handles formats with both integer >> > > + * and non-integer bytes-per-pixel. >> > > + * >> > > + * @format: pixel format info structure >> > > + * @plane: plane index >> > > + * >> > > + * Returns: >> > > + * The bits per pixel value for the specified plane. >> > > + */ >> > > +int drm_format_info_plane_bpp(const struct drm_format_info *info, int plane) >> > > +{ >> > > + if (plane >= info->num_planes) >> > > + return 0; >> > > + >> > > + if (info->cpp[0]) >> > > + return info->cpp[plane] * 8; >> > > + >> > > + return info->bpp[plane]; >> > > +} >> > > +EXPORT_SYMBOL(drm_format_info_plane_bpp); >> > > + >> > > /** >> > > * drm_format_horz_chroma_subsampling - get the horizontal chroma subsampling factor >> > > * @format: pixel format (DRM_FORMAT_*) >> > > diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c >> > > index 8c4d32adcc17..7e00360ff70d 100644 >> > > --- a/drivers/gpu/drm/drm_framebuffer.c >> > > +++ b/drivers/gpu/drm/drm_framebuffer.c >> > > @@ -185,20 +185,20 @@ 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 bpp = drm_format_info_plane_bpp(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 ((uint64_t) DIV_ROUND_UP(width * bpp, 8) > 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 ((uint64_t) r->pitches[i] * 8 < (uint64_t) width * bpp) { >> > > DRM_DEBUG_KMS("bad pitch %u for plane %d\n", r->pitches[i], i); >> > > return -EINVAL; >> > > } >> > > @@ -476,7 +476,7 @@ int drm_mode_getfb(struct drm_device *dev, >> > > r->height = fb->height; >> > > r->width = fb->width; >> > > r->depth = fb->format->depth; >> > > - r->bpp = fb->format->cpp[0] * 8; >> > > + r->bpp = drm_format_info_plane_bpp(fb->format, 0); >> > > r->pitch = fb->pitches[0]; >> > > >> > > /* GET_FB() is an unprivileged ioctl so we must not return a >> > > diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c >> > > index acfbc0641a06..dfe224ccaeba 100644 >> > > --- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c >> > > +++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c >> > > @@ -161,6 +161,7 @@ drm_gem_fb_create_with_funcs(struct drm_device *dev, struct drm_file *file, >> > > unsigned int width = mode_cmd->width / (i ? info->hsub : 1); >> > > unsigned int height = mode_cmd->height / (i ? info->vsub : 1); >> > > unsigned int min_size; >> > > + u8 bpp = drm_format_info_plane_bpp(fb->format, i); >> > > >> > > objs[i] = drm_gem_object_lookup(file, mode_cmd->handles[i]); >> > > if (!objs[i]) { >> > > @@ -170,7 +171,7 @@ 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(width * bpp, 8) >> > > + 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 3e86408dac9f..d4af4dab1623 100644 >> > > --- a/include/drm/drm_fourcc.h >> > > +++ b/include/drm/drm_fourcc.h >> > > @@ -36,6 +36,7 @@ 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) >> > > + * @bpp: Number of bits per pixel (per plane), only valid if cpp[0] == 0. >> > > * @hsub: Horizontal chroma subsampling factor >> > > * @vsub: Vertical chroma subsampling factor >> > > * @has_alpha: Does the format embeds an alpha component? >> > > @@ -45,6 +46,7 @@ struct drm_format_info { >> > > u8 depth; >> > > u8 num_planes; >> > > u8 cpp[3]; >> > > + u8 bpp[3]; >> > > u8 hsub; >> > > u8 vsub; >> > > bool has_alpha; >> > > @@ -66,6 +68,8 @@ drm_get_format_info(struct drm_device *dev, >> > > uint32_t drm_mode_legacy_fb_format(uint32_t bpp, uint32_t depth); >> > > int drm_format_num_planes(uint32_t format); >> > > int drm_format_plane_cpp(uint32_t format, int plane); >> > > +int drm_format_plane_bpp(uint32_t format, int plane); >> > > +int drm_format_info_plane_bpp(const struct drm_format_info *format, int plane); >> > > 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); >> > > -- >> > > 2.16.1 >> > > >> > >> > -- >> > Daniel Vetter >> > Software Engineer, Intel Corporation >> > http://blog.ffwll.ch > >-- >Daniel Vetter >Software Engineer, Intel Corporation >http://blog.ffwll.ch
| |