| Message ID | 20180823152343.6474-2-brian.starkey@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Groundwork for AFBC YUV formats | expand |
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 > --- > 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 >
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. 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
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. 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? -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
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
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 > > > --- > > 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); You might want to pass info here instead of fb->format because fb is NULL. ie drm_format_info_plane_bpp(info, 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 > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi Ayan, On Mon, Sep 10, 2018 at 03:11:34PM +0100, Ayan Halder wrote: >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: [snip] >> > unsigned int min_size; >> > + u8 bpp = drm_format_info_plane_bpp(fb->format, i); >You might want to pass info here instead of fb->format because fb is >NULL. ie drm_format_info_plane_bpp(info, i); > Thanks, good catch. I have to admit I only compile-tested these. -Brian
On Mon, Sep 10, 2018 at 09:50:03AM +0100, Brian Starkey wrote: > 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. Ah, I missed that. > > 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. It's format_info we're talking about. What I mean is that you just set all these to 0 and let the format_info code ignore it. And then having a bespoke drm_format_check_afbc helper function or similar, which checks all the layout restrictions of afbc. I still maintain that bpp and tile_size are equavalent, and we really don't need both. Both are defacto a form of numerator/denumerator. If you don't like that you have to introduce "fake" tiles for afbc, then we can rename tile_size to numerator and tile_h/w to denumerator_h/w. Doesn't change one bit of the math. bpp simply hardcodes a denumerator of 8, and I don't see why we need that special case. Except if you love to write redundant self tests for all the math :-) So two options that I think are reasonable: - one common numerator/denumerator. I don't care how you call that bikeshed. - don't check afbc using format_info, have your own helper that does that using custom code. Cheers, Daniel > 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 > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Mon, Sep 10, 2018 at 09:53:25PM +0200, Daniel Vetter wrote: >On Mon, Sep 10, 2018 at 09:50:03AM +0100, Brian Starkey wrote: >> 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. > >Ah, I missed that. > >> > 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. > >It's format_info we're talking about. What I mean is that you just set all >these to 0 and let the format_info code ignore it. And then having a >bespoke drm_format_check_afbc helper function or similar, which checks all >the layout restrictions of afbc. > >I still maintain that bpp and tile_size are equavalent, and we really >don't need both. Both are defacto a form of numerator/denumerator. If you >don't like that you have to introduce "fake" tiles for afbc, then we can >rename tile_size to numerator and tile_h/w to denumerator_h/w. Doesn't >change one bit of the math. bpp simply hardcodes a denumerator of 8, and I >don't see why we need that special case. Except if you love to write >redundant self tests for all the math :-) > >So two options that I think are reasonable: >- one common numerator/denumerator. I don't care how you call that > bikeshed. Sorry for being dense, but I'm still struggling to get my head around what you're suggesting. In particular "bpp simply hardcodes a denumerator of 8" didn't make any sense to me. Could you give concrete examples for how you think this would look for e.g. - DRM_FORMAT_VUY101010. 30-bits per pixel, no tiling. - DRM_FORMAT_Y0L2. 16-bits per pixel, 2x2 pixel tiles I think we need two things: - The size, in bits, of a tile - The width and height, in pixels, of a tile (currently implicitly 1x1) Do you disagree? Are you just saying that instead of adding .bpp I should be replacing .cpp with .bpp wholesale? >- don't check afbc using format_info, have your own helper that does that > using custom code. We can do this, no problem. Thanks, -Brian
On Mon, Sep 10, 2018 at 09:53:25PM +0200, Daniel Vetter wrote: > On Mon, Sep 10, 2018 at 09:50:03AM +0100, Brian Starkey wrote: > > 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. > > Ah, I missed that. > > > > 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. > > It's format_info we're talking about. What I mean is that you just set all > these to 0 and let the format_info code ignore it. And then having a > bespoke drm_format_check_afbc helper function or similar, which checks all > the layout restrictions of afbc. > > I still maintain that bpp and tile_size are equavalent, and we really > don't need both. Both are defacto a form of numerator/denumerator. If you > don't like that you have to introduce "fake" tiles for afbc, then we can > rename tile_size to numerator and tile_h/w to denumerator_h/w. Doesn't > change one bit of the math. bpp simply hardcodes a denumerator of 8, and I > don't see why we need that special case. Except if you love to write > redundant self tests for all the math :-) My $.02 worth of thoughts: I get the fact that Daniel doesn't like us to add 3 new variables into format_info (bpp, tile_w, tile_h) and that adding a "bits_per_unit" variable should be able to take care of linear (where unit = 1 pixel) and tiled (where unit = tile_w * tile_h pixels) formats. And I also see Daniel's option 2 below, where he says it is reasonable to check AFBC without using format_info. However, the problem we are trying to solve is 2 fold: we are trying to calculate the size of the framebuffer (and the "bits_per_unit" or Brian's bpp is useful for that), but we also try to validate the sizes passed by userspace based on the drm_fourcc.h+modifier info. In that case, the driver still needs to store somewhere the tile_w/tile_h for that given format in order to check that the stride is a whole multiple of tile sizes, so we thought that putting it in format_info is not entirely pointless, because others might use those variables in order to do their driver specific validation, without creating new structures. Did I capture the discussion correctly? If so, can we agree that it is not just the framebuffer size calculation that matters and that tiled formats validation requires a tile_w/tile_h info, therefore Alex's patches and Brian's need to be discussed separately (so that we can bikeshed on whether format_info is the right place or not)? Best regards, Liviu > > So two options that I think are reasonable: > - one common numerator/denumerator. I don't care how you call that > bikeshed. > - don't check afbc using format_info, have your own helper that does that > using custom code. > > Cheers, Daniel > > > 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 > > _______________________________________________ > > dri-devel mailing list > > dri-devel@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
On Wed, Sep 12, 2018 at 04:27:04PM +0100, Liviu Dudau wrote: >On Mon, Sep 10, 2018 at 09:53:25PM +0200, Daniel Vetter wrote: >> On Mon, Sep 10, 2018 at 09:50:03AM +0100, Brian Starkey wrote: >> > 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. >> >> Ah, I missed that. >> >> > > 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. >> >> It's format_info we're talking about. What I mean is that you just set all >> these to 0 and let the format_info code ignore it. And then having a >> bespoke drm_format_check_afbc helper function or similar, which checks all >> the layout restrictions of afbc. >> >> I still maintain that bpp and tile_size are equavalent, and we really >> don't need both. Both are defacto a form of numerator/denumerator. If you >> don't like that you have to introduce "fake" tiles for afbc, then we can >> rename tile_size to numerator and tile_h/w to denumerator_h/w. Doesn't >> change one bit of the math. bpp simply hardcodes a denumerator of 8, and I >> don't see why we need that special case. Except if you love to write >> redundant self tests for all the math :-) > >My $.02 worth of thoughts: > >I get the fact that Daniel doesn't like us to add 3 new variables into >format_info (bpp, tile_w, tile_h) and that adding a "bits_per_unit" >variable should be able to take care of linear (where unit = 1 pixel) >and tiled (where unit = tile_w * tile_h pixels) formats. And I also see >Daniel's option 2 below, where he says it is reasonable to check AFBC >without using format_info. > >However, the problem we are trying to solve is 2 fold: we are trying to >calculate the size of the framebuffer (and the "bits_per_unit" or >Brian's bpp is useful for that), but we also try to validate the sizes >passed by userspace based on the drm_fourcc.h+modifier info. In that >case, the driver still needs to store somewhere the tile_w/tile_h for >that given format in order to check that the stride is a whole multiple >of tile sizes, so we thought that putting it in format_info is not >entirely pointless, because others might use those variables in order to >do their driver specific validation, without creating new structures. Even if we only wanted to do one of those things (calculate the size in bytes of the framebuffer), we need to know the bits_per_unit, as well as the number of units. Without the second (+third) variables, we can't figure out number of units and so we can't figure out the size. Separate point: modifiers certainly shouldn't come in to this discussion - modifier-related validation is always going to need to be in the driver, and is a separate thing entirely (not contained in drm_format_info). > >Did I capture the discussion correctly? If so, can we agree that it is >not just the framebuffer size calculation that matters and that tiled >formats validation requires a tile_w/tile_h info, therefore Alex's >patches and Brian's need to be discussed separately (so that we can >bikeshed on whether format_info is the right place or not)? It looks to me that the right thing to do is set cpp to zero for the "non integer" formats I want to add, and entirely defer to drivers to figure those out (after all, any size calculation the core does for them will be moot, because they are only valid with modifiers, and modifiers can change the size). How the tiled stuff (Y0L0, Y0L2) gets described can be a separate topic. If it happens to support non-integer cpp formats as a side effect, then more the better - when someone comes along with a non-integer-cpp format which actually _is_ supported in linear layout they can use it. I hope Daniel can describe in more detail exactly how he sees that bit working though, because I don't think that either Alex or me understand that yet. Cheers, -Brian > >Best regards, >Liviu > >> >> So two options that I think are reasonable: >> - one common numerator/denumerator. I don't care how you call that >> bikeshed. >> - don't check afbc using format_info, have your own helper that does that >> using custom code. >> >> Cheers, Daniel >> >> > 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 >> > _______________________________________________ >> > dri-devel mailing list >> > dri-devel@lists.freedesktop.org >> > https://lists.freedesktop.org/mailman/listinfo/dri-devel >> >> -- >> Daniel Vetter >> Software Engineer, Intel Corporation >> http://blog.ffwll.ch > >-- >==================== >| I would like to | >| fix the world, | >| but they're not | >| giving me the | > \ source code! / > --------------- > ¯\_(ツ)_/¯
On Wed, Sep 12, 2018 at 3:52 PM, Brian Starkey <brian.starkey@arm.com> wrote: > On Mon, Sep 10, 2018 at 09:53:25PM +0200, Daniel Vetter wrote: >> >> On Mon, Sep 10, 2018 at 09:50:03AM +0100, Brian Starkey wrote: >>> >>> 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. >> >> >> Ah, I missed that. >> >>> > 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. >> >> >> It's format_info we're talking about. What I mean is that you just set all >> these to 0 and let the format_info code ignore it. And then having a >> bespoke drm_format_check_afbc helper function or similar, which checks all >> the layout restrictions of afbc. >> >> I still maintain that bpp and tile_size are equavalent, and we really >> don't need both. Both are defacto a form of numerator/denumerator. If you >> don't like that you have to introduce "fake" tiles for afbc, then we can >> rename tile_size to numerator and tile_h/w to denumerator_h/w. Doesn't >> change one bit of the math. bpp simply hardcodes a denumerator of 8, and I >> don't see why we need that special case. Except if you love to write >> redundant self tests for all the math :-) >> >> So two options that I think are reasonable: >> - one common numerator/denumerator. I don't care how you call that >> bikeshed. > > > Sorry for being dense, but I'm still struggling to get my head around > what you're suggesting. In particular "bpp simply hardcodes a > denumerator of 8" didn't make any sense to me. Could you give concrete > examples for how you think this would look for e.g. > > - DRM_FORMAT_VUY101010. 30-bits per pixel, no tiling. > - DRM_FORMAT_Y0L2. 16-bits per pixel, 2x2 pixel tiles Ok, a few examples: 4 cpp: tile_size = 4, tile_h/w = 1 30 bpp: In cpp that's 30 / 8 = 15 / 4. So would be a tile_size = 15, tile_w = 4, tile_h = 1. If you check the math, this matches exactly all the same addfb values as what you have in your bpp computation (but only if you simplify the quotient). 16 bpp, 2x2 tiles: No real math needed here, tile_size = 2 * 2 * 2 = 8, tile_h/w = 2. > I think we need two things: > - The size, in bits, of a tile Nope, you only need it in bytes, because in the end all buffers must align to bytes. The generic formula is: X bpp: tile_size = X / gcd(X, 8), tile_w = 8 / gcd(X, 8), tile_h = 1. No bits needed anywhere. > - The width and height, in pixels, of a tile (currently implicitly > 1x1) Yeah, but only if you use the cpp thing. We could even throw that out, and entirely replace it with tile_size, with the rule that if tile_h/w = 0, then you assume they're both 1. > Do you disagree? Are you just saying that instead of adding .bpp I > should be replacing .cpp with .bpp wholesale? Hopefully the above explains what I mean, and demonstrates that you can express any bpp in terms of tile_size/tile_w. Because bpp is just a special quotient, with a fixed 8 divisor. -Daniel >> - don't check afbc using format_info, have your own helper that does that >> using custom code. > > > We can do this, no problem. > > Thanks, > -Brian > > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Wed, Sep 12, 2018 at 5:27 PM, Liviu Dudau <liviu.dudau@arm.com> wrote: > On Mon, Sep 10, 2018 at 09:53:25PM +0200, Daniel Vetter wrote: >> On Mon, Sep 10, 2018 at 09:50:03AM +0100, Brian Starkey wrote: >> > 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. >> >> Ah, I missed that. >> >> > > 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. >> >> It's format_info we're talking about. What I mean is that you just set all >> these to 0 and let the format_info code ignore it. And then having a >> bespoke drm_format_check_afbc helper function or similar, which checks all >> the layout restrictions of afbc. >> >> I still maintain that bpp and tile_size are equavalent, and we really >> don't need both. Both are defacto a form of numerator/denumerator. If you >> don't like that you have to introduce "fake" tiles for afbc, then we can >> rename tile_size to numerator and tile_h/w to denumerator_h/w. Doesn't >> change one bit of the math. bpp simply hardcodes a denumerator of 8, and I >> don't see why we need that special case. Except if you love to write >> redundant self tests for all the math :-) > > My $.02 worth of thoughts: > > I get the fact that Daniel doesn't like us to add 3 new variables into > format_info (bpp, tile_w, tile_h) and that adding a "bits_per_unit" > variable should be able to take care of linear (where unit = 1 pixel) > and tiled (where unit = tile_w * tile_h pixels) formats. And I also see > Daniel's option 2 below, where he says it is reasonable to check AFBC > without using format_info. > > However, the problem we are trying to solve is 2 fold: we are trying to > calculate the size of the framebuffer (and the "bits_per_unit" or > Brian's bpp is useful for that), but we also try to validate the sizes > passed by userspace based on the drm_fourcc.h+modifier info. In that > case, the driver still needs to store somewhere the tile_w/tile_h for > that given format in order to check that the stride is a whole multiple > of tile sizes, so we thought that putting it in format_info is not > entirely pointless, because others might use those variables in order to > do their driver specific validation, without creating new structures. > > Did I capture the discussion correctly? If so, can we agree that it is > not just the framebuffer size calculation that matters and that tiled > formats validation requires a tile_w/tile_h info, therefore Alex's > patches and Brian's need to be discussed separately (so that we can > bikeshed on whether format_info is the right place or not)? I dont think this captures the gist of what I have in mind. I've laid out the math in another reply. What I don't want is to add bpp and tile_size/h/w, because that's redundant, and you can express any bpp value in terms of tile_size/w. We should probably also throw out cpp, but that's a bit more involved (well, just need a wrapper to compute cpp and then a cocci script and done). -Daniel > Best regards, > Liviu > >> >> So two options that I think are reasonable: >> - one common numerator/denumerator. I don't care how you call that >> bikeshed. >> - don't check afbc using format_info, have your own helper that does that >> using custom code. >> >> Cheers, Daniel >> >> > 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 >> > _______________________________________________ >> > dri-devel mailing list >> > dri-devel@lists.freedesktop.org >> > https://lists.freedesktop.org/mailman/listinfo/dri-devel >> >> -- >> Daniel Vetter >> Software Engineer, Intel Corporation >> http://blog.ffwll.ch > > -- > ==================== > | I would like to | > | fix the world, | > | but they're not | > | giving me the | > \ source code! / > --------------- > ¯\_(ツ)_/¯ > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
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);
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> --- 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(-)