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
Attachment:
signature.asc
Description: PGP signature
