[PATCH] drm: Replace drm_framebuffer plane size functions with its equivalents

[Maxime Ripard]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20230719/7377e854/attachment.sig>