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
