From mboxrd@z Thu Jan 1 00:00:00 1970 From: Eric Anholt Subject: Re: [PATCH 3/5] drm/vc4: Use drm_atomic_helper_check_plane_state() to simplify the logic Date: Fri, 27 Jul 2018 13:38:08 -0700 Message-ID: <87effo1kjz.fsf@anholt.net> References: <20180725153209.14366-1-boris.brezillon@bootlin.com> <20180725153209.14366-4-boris.brezillon@bootlin.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0781595891==" Return-path: Received: from anholt.net (anholt.net [50.246.234.109]) by gabe.freedesktop.org (Postfix) with ESMTP id 6F8EF6E29D for ; Fri, 27 Jul 2018 20:38:12 +0000 (UTC) In-Reply-To: <20180725153209.14366-4-boris.brezillon@bootlin.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" Cc: David Airlie , Boris Brezillon , dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org --===============0781595891== Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha512; protocol="application/pgp-signature" --=-=-= Content-Type: text/plain Boris Brezillon writes: > From: Eric Anholt > > 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 > Signed-off-by: Boris Brezillon > --- > 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. --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEE/JuuFDWp9/ZkuCBXtdYpNtH8nugFAltbgrAACgkQtdYpNtH8 nugSOhAAjUIbIQiAqpE452sQWbE296j9v5Xt8+8VdoapxwBWzqMuoDNgC4JjqNGj neEqj3Gud3uj5XOqKhXqzGmLZ0OsKvmRJn+JKkeCHwBKrKjjqJWS0N/dpQMD8QWB 6vYPipFCtDy+ftpM7hZVn+Gth+zQdNdRhtP2dudEmASl9H455EUgFINsho/mZ0KR PDNIyU61fOZAsT5Hhu5RPCxMReqc76lDc2drGb4nkYiFyUDAJHA3+xxy25bltaed yU+PfP9ahyEBOrBkVhL/rieAxMzVpXmJcDHBvfSGEQWVh/g+UyHnstGGqisYEgyK nS0C7V1Oup2fGqYy1OBoKcLFPteJs9Y6YuLV6aQH+XpLBIQ5iOSHLEBxpmr/vX9T O54VVxOiddqf0K4faFIm6UPraWEjdzSRAPlMffsZwJSexeTscPpp2hnoqTlLVbcu c26fwRvC7/3IPjvI7ozzifKT01DSkxNlRcVFcsGxhHM5jvEElCzwsqhAnqeQ2T5K QFfolNOXw4gg7vMxcubgjUbFCXKaerbIGbHGPow2iZImpHBSGt/TL/PTzS9OAJEP /bHd9YRXNrZspd6eFQdzcNVk9pQ0J7bJ6j0nqlJ2jutKBTEdzH1dLRv+cHX43VYP mS75zgo2SvWk6Y2NqeGL3h4DfnecVxcVy21JKb4bCnBwwijBmwU= =F68y -----END PGP SIGNATURE----- --=-=-=-- --===============0781595891== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVsCg== --===============0781595891==--