From mboxrd@z Thu Jan 1 00:00:00 1970 From: Rodrigo Siqueira Subject: Re: [PATCH v2,1/2] drm/vkms: Use alpha for blending in blend() function Date: Thu, 31 Jan 2019 10:44:09 -0200 Message-ID: <20190131124409.c2dimvn4ztpnp35j@smtp.gmail.com> References: <48ecdf48d153d0ca3b954ba737ae9a0b35ef3ad5.1548798106.git.mamtashukla555@gmail.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0343498054==" Return-path: Received: from mail-qt1-x841.google.com (mail-qt1-x841.google.com [IPv6:2607:f8b0:4864:20::841]) by gabe.freedesktop.org (Postfix) with ESMTPS id E53A26E6AC for ; Thu, 31 Jan 2019 12:44:14 +0000 (UTC) Received: by mail-qt1-x841.google.com with SMTP id l12so3224371qtf.8 for ; Thu, 31 Jan 2019 04:44:14 -0800 (PST) In-Reply-To: <48ecdf48d153d0ca3b954ba737ae9a0b35ef3ad5.1548798106.git.mamtashukla555@gmail.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Mamta Shukla Cc: hamohammed.sa@gmail.com, dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org --===============0343498054== Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="ee7gzq6mcslljvfw" Content-Disposition: inline --ee7gzq6mcslljvfw Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Hi, First of all, thanks for your patch :) I tested your patch against the tests that you implemented in the IGT [1]. All the alpha tests passed, but there was a weird warning that says: $ sudo IGT_FORCE_DRIVER=3Dvkms ./tests/kms_cursor_crc --run-subtest cursor= -alpha-opaque IGT-Version: 1.23-g8d81c2c2 (x86_64) (Linux: 5.0.0-rc1-VKMS-RULES+ x86_64) Force option used: Using driver vkms Starting subtest: cursor-alpha-opaque (kms_cursor_crc:423) igt_debugfs-WARNING: Warning on condition all_zero in= function crc_sanity_checks, file ../lib/igt_debugfs.c:901 (kms_cursor_crc:423) igt_debugfs-WARNING: Suspicious CRC: All values are 0. Beginning cursor-alpha-opaque on pipe A, connector Virtual-2 =20 cursor-alpha-opaque on pipe A, connector Virtual-2: PASSED =20 Subtest cursor-alpha-opaque: SUCCESS (0.315s) Do you know the reason for that? Could you detail this issue? Is it possible to fix it? You can see the other comments inline. [1] https://patchwork.freedesktop.org/series/55944/ On 01/30, Mamta Shukla wrote: > Use the alpha value to blend vaddr_src with vaddr_dst instead > of overwriting it in blend(). >=20 > Signed-off-by: Mamta Shukla > --- > changes in v2: > -Use macro to avoid code duplication > -Add spaces around '/' and '-' > -Remove spaces at the end of the line >=20 > drivers/gpu/drm/vkms/vkms_crc.c | 25 +++++++++++++++++++++++-- > 1 file changed, 23 insertions(+), 2 deletions(-) >=20 > diff --git a/drivers/gpu/drm/vkms/vkms_crc.c b/drivers/gpu/drm/vkms/vkms_= crc.c > index 9d9e8146db90..dc6cb4c2cced 100644 > --- a/drivers/gpu/drm/vkms/vkms_crc.c > +++ b/drivers/gpu/drm/vkms/vkms_crc.c > @@ -5,6 +5,8 @@ > #include > #include > =20 > +#define BITSHIFT(val,i) (u8)((*(u32 *)(val)) >> i & 0xff) - Take care with the macros definition, since you can create a precedence problem. For example, here, you didn't surround =E2=80=9Ci=E2=80=9D with = =E2=80=9C()=E2=80=9D. - At the end of this operation you cast all the value to u8. In this sense, why do you need the 0xff in the end? - I=E2=80=99m worried about the little and big endian issues here. If I understood well, this macro could fail on a big-endian environment. Is it right? Did I miss something? Could you explain to me what going to happen in the big and endian case? - Finally, did you take a look at =E2=80=9Cinclude/linux/bitops.h=E2=80=9D = and =E2=80=9Cinclude/linux/kernel.h=E2=80=9D? These headers have a bunch of u= seful macros and functions; probably you can find something useful for you in this file.=20 > + > /** > * compute_crc - Compute CRC value on output frame > * > @@ -71,6 +73,9 @@ static void blend(void *vaddr_dst, void *vaddr_src, > int y_limit =3D y_src + h_dst; > int x_limit =3D x_src + w_dst; > =20 > + u8 alpha, r_src, r_dst, g_src, g_dst, b_src, b_dst; > + u8 r_alpha, g_alpha, b_alpha; > + > for (i =3D y_src, i_dst =3D y_dst; i < y_limit; ++i) { > for (j =3D x_src, j_dst =3D x_dst; j < x_limit; ++j) { > offset_dst =3D crc_dst->offset > @@ -79,9 +84,25 @@ static void blend(void *vaddr_dst, void *vaddr_src, > offset_src =3D crc_src->offset > + (i * crc_src->pitch) > + (j * crc_src->cpp); > + =09 > + /*Currently handles alpha values for fully opaque or fully transparen= t*/ > + alpha =3D (u8)((*(u32 *)vaddr_src + offset_src) >> 24); > + alpha =3D alpha / 255; > + r_src =3D BITSHIFT(vaddr_src + offset_src, 16); > + g_src =3D BITSHIFT(vaddr_src + offset_src, 8); > + b_src =3D BITSHIFT(vaddr_src + offset_src, 0); If I correctly understood, you have an u32 values which gave you 4 bytes; because of this, you have one byte for Red, Green, Blue, and Alpha. The above operations extracts each value, one by one. In this sense, why do we need all of this bitwise operation since you can access it as an array of chars? Something like this (draft alert): char *cursor_addr =3D (char*)vaddr_src + offset_src; r_src =3D cursor_addr[2]; g_src =3D cursor_addr[1]; b_src =3D cursor_addr[0]; ... There's any restriction for that? Is it related to the big and little endian issue? Finally, is it ok to make pointer operation with void* in the kernel? > + r_dst =3D BITSHIFT(vaddr_dst + offset_dst, 16); > + g_dst =3D BITSHIFT(vaddr_dst + offset_dst, 8); > + b_dst =3D BITSHIFT(vaddr_dst + offset_dst, 0); > + > + /*Pre-multiplied alpha for blending */ > + r_alpha =3D (r_src) + (r_dst * (1 - alpha)); > + g_alpha =3D (g_src) + (g_dst * (1 - alpha)); > + b_alpha =3D (b_src) + (b_dst * (1 - alpha)); > + memset(vaddr_dst + offset_dst, b_alpha, sizeof(u8)); > + memset(vaddr_dst + offset_dst + 1, g_alpha, sizeof(u8)); > + memset(vaddr_dst + offset_dst + 2, r_alpha, sizeof(u8)); IMHO, I prefer to move all above alpha operations for an inline function on top of =E2=80=9Cblend()=E2=80=9D with the goal of improve the code reada= bility.=20 =20 > - memcpy(vaddr_dst + offset_dst, > - vaddr_src + offset_src, sizeof(u32)); > } > i_dst++; > } > --=20 > 2.17.1 >=20 Finally, your patch introduces some checkpatch warnings and errors. I highly recommend you to use checkpatch in your patches before you send it. In the link below [2], there=E2=80=99s a section named =E2=80=9CGit pos= t-commit hooks=E2=80=9D; take a look at it. [2] https://kernelnewbies.org/FirstKernelPatch Again, thanks for your patch. Best Regards --=20 Rodrigo Siqueira https://siqueira.tech Graduate Student Department of Computer Science University of S=C3=A3o Paulo --ee7gzq6mcslljvfw Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEE4tZ+ii1mjMCMQbfkWJzP/comvP8FAlxS7ZkACgkQWJzP/com vP/ayRAAsDMk75w1Qn6wmJu/MYn/UlKU7qz45tqAIowmPBHsXyRWnT9duTqb2nuw 0r626v2cbVMm2AYH3GLvSNO/c4rmPj0/9XGlbmE9vBgjZBArf0jXyWcY1AecC2r2 49CB/tmzY+5zFBQ8nLjEwxs89BK2JhqZWPS14KonEWb5KyFCDgnGmwHCa4ESqovd 0w8Vc4IUV89L3tRrEG2xsalAQggatBummDbnpOwhhXpkG0UesqY6s8fl+kjrFfWf IMQPwP9jqcVTIi9CNWjEQo4v7sY32XdNBKWNEWyQl3VV81uTU6mqSl4wH5CKjrdL dMCB4wsbsHt9hUqDJuGUjxkRYifIBCoj38PbGj4IQLMSkTdMsF5MqS0YmTFMYW/E y18E/N44koI1+NnEj7J1pClDukm3vzUTdh4V45B4huQPsDES49k6+POk6sKyMVvj aUgANRjj7y9I7KsZHS1khq65J+vFLP8x3hr+lWQyehqT+qXtiMotxO1tIG3MJITS hI2GltOtlp4RcPzQ1vzqZTVYcTASG7553/0v1TfZZVBJp8Qy5qhX9WheINz/2UXT AhavJOSII9juOy6rbOi/tOdAj31wrkOc0guJyJ8b/P4wWaER8qk+zUqX0yUPzLeH 7hBF5NLz5DyWNpPyAjFvUv/YtTaFsqVsG/S11I4NY2jOYBx1FB4= =RJJh -----END PGP SIGNATURE----- --ee7gzq6mcslljvfw-- --===============0343498054== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVsCg== --===============0343498054==--