Re: [PATCH 3/5] drm/vc4: Use drm_atomic_helper_check_plane_state() to simplify the logic

[Eric Anholt <eric@anholt.net>]

[-- Attachment #1.1: Type: text/plain, Size: 1785 bytes --]

Boris Brezillon <boris.brezillon@bootlin.com> writes:

> From: Eric Anholt <eric@anholt.net>
>
> drm_atomic_helper_check_plane_state() takes care of checking the
> scaling capabilities and calculating the clipped X/Y offsets for us.
>
> Rely on this function instead of open-coding the logic. While at it, we
> get rid of a few fields in vc4_plane_state that can be easily extracted
> from drm_plane_state.
>
> Incidentally, it seems to fix a problem we had with negative X/Y
> positioning of YUV planes.
>
> Signed-off-by: Eric Anholt <eric@anholt.net>
> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> ---
>  drivers/gpu/drm/vc4/vc4_drv.h   |   9 --
>  drivers/gpu/drm/vc4/vc4_plane.c | 210 +++++++++++++++++++++-------------------
>  2 files changed, 111 insertions(+), 108 deletions(-)

I feel like you ought to grab authorship of this patch -- you've made a
bunch of good changes since my WIP patch.

>  	if (num_planes > 1) {
> -		vc4_state->is_yuv = true;
> -
> -		h_subsample = drm_format_horz_chroma_subsampling(format);
> -		v_subsample = drm_format_vert_chroma_subsampling(format);
> -		vc4_state->src_w[1] = vc4_state->src_w[0] / h_subsample;
> -		vc4_state->src_h[1] = vc4_state->src_h[0] / v_subsample;
> +		src_w /= h_subsample;
> +		src_h /= v_subsample;

I'm a little nervous about leaving src_w/src_h divided after this block,
in case we end up using them in some other calculation in a later
change.  Could we just move the divide into the vc4_get_scaling_mode()
call?

In general, I'm not enthusiastic about having src_w computations in 5
places now.  Was keeping it in the vc4 state that much of a problem?

I'm not saying no, but copy-and-paste of these kinds of calculations are
also a frequent source of bugs when you miss a x->y or w->h change.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel