| Message ID | 20230627182239.15676-1-gcarlos@disroot.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: Replace drm_framebuffer plane size functions with its equivalents | expand |
Hi Carlos, Em 27/06/2023 15:22, Carlos Eduardo Gallo Filho escreveu: [...] > > So, replace each drm_framebuffer_plane_{width,height} and > fb_plane_{width,height} call to drm_format_info_plane_{width,height} > and remove them. > I see that with this replace, there's a small code change from return DIV_ROUND_UP(width, format->hsub); to return width / info->hsub; is there any case that the replaced function will give different results?
Hi André, thanks for review! On 7/12/23 20:30, André Almeida wrote: > Hi Carlos, > > Em 27/06/2023 15:22, Carlos Eduardo Gallo Filho escreveu: > [...] >> >> So, replace each drm_framebuffer_plane_{width,height} and >> fb_plane_{width,height} call to drm_format_info_plane_{width,height} >> and remove them. >> > > I see that with this replace, there's a small code change from > > return DIV_ROUND_UP(width, format->hsub); > > to > return width / info->hsub; > > is there any case that the replaced function will give different results? Well, short answer: Yes, and I'm already thinking on how do address it. Taking a look at some drivers, I could notice that almost every driver do some similar calculating to obtain the size of a plane given the size of the first (I guess that they would be using these functions though). So, I stated that nearly all drivers implements this as a regular division, with exception of exynos, sun4i and i915 (counting with the replaced function), which all has at some point a DIV_ROUND_UP involving hsub or vsub. Curiously, even the i915 having a DIV_ROUND_UP in some places, it also has regular division involving hsub/vsub in others, which leads me to guess if the chosen rounding method really matters. I also discovered the existence of the intel_plane_check_src_coordinates() function, that do some checks, which one of them consist of ensuring that for a plane, both source coordinates and sizes are multiples of the vsub and hsub, implying that no division rounding should occurs at all when it's used. However, I couldn't state if this function is always called for every source on every plane, so I can't assume anything from this. Furthermore, I found the 33f673aa55e96 ("drm: Remove fb hsub/vsub alignment requirement") commit, that explains the motivation for having DIV_ROUND_UP on drm_framebuffer_plane_{width,height} functions. It says that the DIV_ROUND_UP is needed on places where the source isn't necessarily aligned with respect to the subsampling factors, that should be the sane default for a core function. Saying that, I thought some ways to address this problem: 1. Replace the regular division on drm_format_info_plane_{width,height} functions to DIV_ROUND_UP so that we assert that this function is always correct (as it seems that the places where regular division is used assumes alignment, implying no division rounding at all). 2. Create DIV_ROUND_UP variants of drm_format_info_plane_{width,height} functions and use each of them in the right place. Maybe let the default be the one with DIV_ROUND_UP and the variant with regular division, naming it as something like "drm_format_info_aligned_plane_{width,height}". I personally would prefer the second alternative, as it provides more flexibility and avoids using DIV_ROUND_UP unnecessarily. If nobody states anything wrong with this approach, I'll be sending the second version of this patch with it. Thanks, Carlos
Hi, On Mon, Jul 17, 2023 at 04:04:43PM -0300, Carlos wrote: > On 7/12/23 20:30, André Almeida wrote: > > Hi Carlos, > > > > Em 27/06/2023 15:22, Carlos Eduardo Gallo Filho escreveu: > > [...] > > > > > > So, replace each drm_framebuffer_plane_{width,height} and > > > fb_plane_{width,height} call to drm_format_info_plane_{width,height} > > > and remove them. > > > > > > > I see that with this replace, there's a small code change from > > > > return DIV_ROUND_UP(width, format->hsub); > > > > to > > return width / info->hsub; > > > > is there any case that the replaced function will give different results? > > Well, short answer: Yes, and I'm already thinking on how do address it. > > Taking a look at some drivers, I could notice that almost every driver do > some similar calculating to obtain the size of a plane given the size of > the first (I guess that they would be using these functions though). So, I > stated that nearly all drivers implements this as a regular division, with > exception of exynos, sun4i and i915 (counting with the replaced function), > which all has at some point a DIV_ROUND_UP involving hsub or vsub. > > Curiously, even the i915 having a DIV_ROUND_UP in some places, it also > has regular division involving hsub/vsub in others, which leads me to > guess if the chosen rounding method really matters. I also discovered > the existence of the intel_plane_check_src_coordinates() function, > that do some checks, which one of them consist of ensuring that for a > plane, both source coordinates and sizes are multiples of the vsub and > hsub, implying that no division rounding should occurs at all when it's > used. However, I couldn't state if this function is always called for > every source on every plane, so I can't assume anything from this. > > Furthermore, I found the 33f673aa55e96 ("drm: Remove fb hsub/vsub > alignment requirement") commit, that explains the motivation for having > DIV_ROUND_UP on drm_framebuffer_plane_{width,height} functions. It says > that the DIV_ROUND_UP is needed on places where the > source isn't necessarily aligned with respect to the subsampling factors, > that should be the sane default for a core function. Honestly, I don't think there's a problem, but rather something that was done in each and every driver differently and without a second thought. I think we should indeed converge to a single helper, and if that helper is broken fix it. It will be broken for everyone anyway. So I can definitely see a patch that adds DIV_ROUND_UP() to drm_plane_info_plane_width and height, and then the first patch of yours. > Saying that, I thought some ways to address this problem: > > 1. Replace the regular division on drm_format_info_plane_{width,height} > functions to DIV_ROUND_UP so that we assert that this function is > always correct (as it seems that the places where regular division > is used assumes alignment, implying no division rounding at all). +1 > 2. Create DIV_ROUND_UP variants of drm_format_info_plane_{width,height} > functions and use each of them in the right place. Maybe > let the default be the one with DIV_ROUND_UP and the > variant with regular division, naming it as something like > "drm_format_info_aligned_plane_{width,height}". No, that won't work. Provided with a choice, a driver is most likely to cargo-cult it anyway. And it's not like we know what we should do here. Maxime
diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c index aff3746dedfb..efed4cd7965e 100644 --- a/drivers/gpu/drm/drm_framebuffer.c +++ b/drivers/gpu/drm/drm_framebuffer.c @@ -151,24 +151,6 @@ int drm_mode_addfb_ioctl(struct drm_device *dev, return drm_mode_addfb(dev, data, file_priv); } -static int fb_plane_width(int width, - const struct drm_format_info *format, int plane) -{ - if (plane == 0) - return width; - - return DIV_ROUND_UP(width, format->hsub); -} - -static int fb_plane_height(int height, - const struct drm_format_info *format, int plane) -{ - if (plane == 0) - return height; - - return DIV_ROUND_UP(height, format->vsub); -} - static int framebuffer_check(struct drm_device *dev, const struct drm_mode_fb_cmd2 *r) { @@ -196,8 +178,8 @@ static int framebuffer_check(struct drm_device *dev, info = drm_get_format_info(dev, r); 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 width = drm_format_info_plane_width(info, r->width, i); + unsigned int height = drm_format_info_plane_height(info, r->height, i); unsigned int block_size = info->char_per_block[i]; u64 min_pitch = drm_format_info_min_pitch(info, i, width); @@ -1136,44 +1118,6 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb) } EXPORT_SYMBOL(drm_framebuffer_remove); -/** - * drm_framebuffer_plane_width - width of the plane given the first plane - * @width: width of the first plane - * @fb: the framebuffer - * @plane: plane index - * - * Returns: - * The width of @plane, given that the width of the first plane is @width. - */ -int drm_framebuffer_plane_width(int width, - const struct drm_framebuffer *fb, int plane) -{ - if (plane >= fb->format->num_planes) - return 0; - - return fb_plane_width(width, fb->format, plane); -} -EXPORT_SYMBOL(drm_framebuffer_plane_width); - -/** - * drm_framebuffer_plane_height - height of the plane given the first plane - * @height: height of the first plane - * @fb: the framebuffer - * @plane: plane index - * - * Returns: - * The height of @plane, given that the height of the first plane is @height. - */ -int drm_framebuffer_plane_height(int height, - const struct drm_framebuffer *fb, int plane) -{ - if (plane >= fb->format->num_planes) - return 0; - - return fb_plane_height(height, fb->format, plane); -} -EXPORT_SYMBOL(drm_framebuffer_plane_height); - void drm_framebuffer_print_info(struct drm_printer *p, unsigned int indent, const struct drm_framebuffer *fb) { @@ -1189,8 +1133,8 @@ void drm_framebuffer_print_info(struct drm_printer *p, unsigned int indent, for (i = 0; i < fb->format->num_planes; i++) { drm_printf_indent(p, indent + 1, "size[%u]=%dx%d\n", i, - drm_framebuffer_plane_width(fb->width, fb, i), - drm_framebuffer_plane_height(fb->height, fb, i)); + drm_format_info_plane_width(fb->format, fb->width, i), + drm_format_info_plane_height(fb->format, fb->height, i)); drm_printf_indent(p, indent + 1, "pitch[%u]=%u\n", i, fb->pitches[i]); drm_printf_indent(p, indent + 1, "offset[%u]=%u\n", i, fb->offsets[i]); drm_printf_indent(p, indent + 1, "obj[%u]:%s\n", i, diff --git a/drivers/gpu/drm/i915/display/intel_fb.c b/drivers/gpu/drm/i915/display/intel_fb.c index 446bbf7986b6..5de2453ff7a3 100644 --- a/drivers/gpu/drm/i915/display/intel_fb.c +++ b/drivers/gpu/drm/i915/display/intel_fb.c @@ -1113,7 +1113,7 @@ static int intel_fb_offset_to_xy(int *x, int *y, return -EINVAL; } - height = drm_framebuffer_plane_height(fb->height, fb, color_plane); + height = drm_format_info_plane_height(fb->format, fb->height, color_plane); height = ALIGN(height, intel_tile_height(fb, color_plane)); /* Catch potential overflows early */ diff --git a/include/drm/drm_framebuffer.h b/include/drm/drm_framebuffer.h index 0dcc07b68654..80ece7b6dd9b 100644 --- a/include/drm/drm_framebuffer.h +++ b/include/drm/drm_framebuffer.h @@ -292,11 +292,6 @@ static inline void drm_framebuffer_assign(struct drm_framebuffer **p, &fb->head != (&(dev)->mode_config.fb_list); \ fb = list_next_entry(fb, head)) -int drm_framebuffer_plane_width(int width, - const struct drm_framebuffer *fb, int plane); -int drm_framebuffer_plane_height(int height, - const struct drm_framebuffer *fb, int plane); - /** * struct drm_afbc_framebuffer - a special afbc frame buffer object *
The functions drm_framebuffer_plane{width,height} and fb_plane_{width,height} do exactly the same job of its equivalents drm_format_info_plane_{width,height} from drm_fourcc. The only reason to have these functions on drm_framebuffer would be if they would added a abstraction layer to call it just passing a drm_framebuffer pointer and the desired plane index, which is not the case, where these functions actually implements just part of it. In the actual implementation, every call to both drm_framebuffer_plane_{width,height} and fb_plane_{width,height} should pass some drm_framebuffer attribute, which is the same as calling the drm_format_info_plane_{width,height} functions. The drm_format_info_pane_{width,height} functions are much more consistent in both its implementation and its location on code. The kind of calculation that they do is intrinsically derivated from the drm_format_info struct and has not to do with drm_framebuffer, except by the potential motivation described above, which is still not a good justificative to have drm_framebuffer functions to calculate it. So, replace each drm_framebuffer_plane_{width,height} and fb_plane_{width,height} call to drm_format_info_plane_{width,height} and remove them. Signed-off-by: Carlos Eduardo Gallo Filho <gcarlos@disroot.org> --- drivers/gpu/drm/drm_framebuffer.c | 64 ++----------------------- drivers/gpu/drm/i915/display/intel_fb.c | 2 +- include/drm/drm_framebuffer.h | 5 -- 3 files changed, 5 insertions(+), 66 deletions(-)