Re: [PATCH V10 35/46] drm/amd/display: add shaper and blend colorops for 1D Curve Custom LUT
Hi Alex, kernel test robot noticed the following build warnings: [auto build test WARNING on linus/master] [also build test WARNING on v6.16-rc2] [cannot apply to drm-exynos/exynos-drm-next drm/drm-next drm-intel/for-linux-next drm-intel/for-linux-next-fixes drm-misc/drm-misc-next drm-tip/drm-tip next-20250617] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Alex-Hung/drm-Add-helper-for-conversion-from-signed-magnitude/20250617-123027 base: linus/master patch link: https://lore.kernel.org/r/20250617041746.2884343-36-alex.hung%40amd.com patch subject: [PATCH V10 35/46] drm/amd/display: add shaper and blend colorops for 1D Curve Custom LUT config: x86_64-buildonly-randconfig-006-20250618 (https://download.01.org/0day-ci/archive/20250618/202506181213.s2kcnsrm-...@intel.com/config) compiler: gcc-12 (Debian 12.2.0-14) 12.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250618/202506181213.s2kcnsrm-...@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot | Closes: https://lore.kernel.org/oe-kbuild-all/202506181213.s2kcnsrm-...@intel.com/ All warnings (new ones prefixed by >>): >> Warning: >> drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm_color.c:442 Excess >> function parameter 'is_legacy' description in '__drm_lut_32_to_dc_gamma' -- 0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki
Re: [PATCH V9 16/43] drm/colorop: Add 3x4 CTM type
On Tue, Jun 17, 2025 at 05:33:20AM +, Borah, Chaitanya Kumar wrote: > > > > -Original Message- > > From: Pekka Paalanen > > Sent: Monday, June 16, 2025 7:02 PM > > To: Borah, Chaitanya Kumar > > Cc: Alex Hung ; dri-de...@lists.freedesktop.org; amd- > > g...@lists.freedesktop.org; wayland-devel@lists.freedesktop.org; > > harry.wentl...@amd.com; leo@amd.com; ville.syrj...@linux.intel.com; > > m...@igalia.com; jad...@redhat.com; sebastian.w...@redhat.com; > > shashank.sha...@amd.com; ago...@nvidia.com; jos...@froggi.es; > > mdaen...@redhat.com; aleix...@kde.org; xaver.h...@gmail.com; > > victo...@system76.com; dan...@ffwll.ch; Shankar, Uma > > ; quic_nas...@quicinc.com; > > quic_cbr...@quicinc.com; quic_abhin...@quicinc.com; mar...@marcan.st; > > liviu.du...@arm.com; sashamcint...@google.com; > > louis.chau...@bootlin.com; Daniel Stone > > Subject: Re: [PATCH V9 16/43] drm/colorop: Add 3x4 CTM type > > > > On Mon, 16 Jun 2025 11:30:23 + > > "Borah, Chaitanya Kumar" wrote: > > > > > > -Original Message- > > > > From: Alex Hung > > > > Sent: Wednesday, April 30, 2025 6:41 AM > > > > To: dri-de...@lists.freedesktop.org; amd-...@lists.freedesktop.org > > > > Cc: wayland-devel@lists.freedesktop.org; harry.wentl...@amd.com; > > > > alex.h...@amd.com; leo@amd.com; ville.syrj...@linux.intel.com; > > > > pekka.paala...@collabora.com; cont...@emersion.fr; m...@igalia.com; > > > > jad...@redhat.com; sebastian.w...@redhat.com; > > > > shashank.sha...@amd.com; ago...@nvidia.com; jos...@froggi.es; > > > > mdaen...@redhat.com; aleix...@kde.org; xaver.h...@gmail.com; > > > > victo...@system76.com; dan...@ffwll.ch; Shankar, Uma > > > > ; quic_nas...@quicinc.com; > > > > quic_cbr...@quicinc.com; quic_abhin...@quicinc.com; > > > > mar...@marcan.st; liviu.du...@arm.com; sashamcint...@google.com; > > > > Borah, Chaitanya Kumar ; > > > > louis.chau...@bootlin.com; Daniel Stone > > > > Subject: [PATCH V9 16/43] drm/colorop: Add 3x4 CTM type > > > > > > > > From: Harry Wentland > > > > > > > > This type is used to support a 3x4 matrix in colorops. A 3x4 matrix > > > > uses the last column as a "bias" column. Some HW exposes support for > > > > 3x4. The calculation looks like: > > > > > > > > out matrixin > > > > |R| |0 1 2 3 | | R | > > > > |G| = |4 5 6 7 | x | G | > > > > |B| |8 9 10 11| | B | > > > >|1.0| > > > > > > > > This is also the first colorop where we need a blob property to > > > > program the property. For that we'll introduce a new DATA property > > > > that can be used by all colorop TYPEs requiring a blob. The way a > > > > DATA blob is read depends on the TYPE of the colorop. > > > > > > > > We only create the DATA property for property types that need it. > > > > > > Is there any value to adding pre-offsets [1] in the uapi? > > > > > > |R/Cr|| c0 c1 c2 | ( |R/Cr| |preoff0| ) |postoff0| > > > |G/Y | = | c3 c4 c5 | x ( |G/Y | + |preoff1| ) + |postoff1| > > > |B/Cb| | c6 c7 c8 | ( |B/Cb| |preoff2| ) |postoff2| > > > > > > Handling limited range values is one use case that I can think of. > > > > Hi, > > > > in the mathematical sense, no. A pre-offset can always be converted into a > > post-offset by multiplying it with the 3x3 matrix (and adding to the > > existing > > post-offset). This can be pre-computed, no need to do it separately for > > every > > pixel. > > > > For hardware reasons, I have no idea. > > Thank you for the reply, Pekka. Our hardware does allow programming > Pre-offsets separately. > Currently I can't think of a particular advantage of that if mathematically a > post-offset does the job but I will keep this thread posted if I find > something out. Just FYI we have three different kinds of hardware: - 3x3 matrix - 3x3 matrix + programmable pre offsets + hardcoded post offsets for limited range conversion - 3x3 matrix + programmable pre and post offsets -- Ville Syrjälä Intel
Re: [PATCH V10 19/46] drm/tests: Add a few tests around drm_fixed.h
On 2025-06-17 07:04, Maíra Canal wrote:
> Hi Alex,
>
> On 17/06/25 01:17, Alex Hung wrote:
>> From: Harry Wentland
>>
>> While working on the CTM implementation of VKMS I had to ascertain
>> myself of a few assumptions. One of those is whether drm_fixed.h
>> treats its numbers using signed-magnitude or twos-complement. It is
>> twos-complement.
>>
>> In order to make someone else's day easier I am adding the
>> drm_test_int2fixp test that validates the above assumption.
>>
>> I am also adding a test for the new sm2fixp function that converts
>> from a signed-magnitude fixed point to the twos-complement fixed
>> point.
>>
>> Reviewed-by: Louis Chauvet
>> Signed-off-by: Alex Hung
>> Signed-off-by: Harry Wentland
>> Reviewed-by: Daniel Stone
>> ---
>> v7:
>> - Fix checkpatch warnings (Louis Chauvet)
>>
>> v6:
>> - add missing MODULE_DESCRIPTION (Jeff Johnson)
>> - fix buffer overflow
>>
>>
>> drivers/gpu/drm/tests/Makefile | 3 +-
>> drivers/gpu/drm/tests/drm_fixp_test.c | 71 +++
>> 2 files changed, 73 insertions(+), 1 deletion(-)
>> create mode 100644 drivers/gpu/drm/tests/drm_fixp_test.c
>>
>> diff --git a/drivers/gpu/drm/tests/Makefile b/drivers/gpu/drm/tests/
>> Makefile
>> index 3afd6587df08..91437cf34d92 100644
>> --- a/drivers/gpu/drm/tests/Makefile
>> +++ b/drivers/gpu/drm/tests/Makefile
>> @@ -23,6 +23,7 @@ obj-$(CONFIG_DRM_KUNIT_TEST) += \
>> drm_modes_test.o \
>> drm_plane_helper_test.o \
>> drm_probe_helper_test.o \
>> - drm_rect_test.o
>> + drm_rect_test.o \
>> + drm_fixp_test.o
>> CFLAGS_drm_mm_test.o := $(DISABLE_STRUCTLEAK_PLUGIN)
>> diff --git a/drivers/gpu/drm/tests/drm_fixp_test.c b/drivers/gpu/drm/
>> tests/drm_fixp_test.c
>> new file mode 100644
>> index ..de91177af213
>> --- /dev/null
>> +++ b/drivers/gpu/drm/tests/drm_fixp_test.c
>> @@ -0,0 +1,71 @@
>> +// SPDX-License-Identifier: MIT
>> +/*
>> + * Copyright 2022 Advanced Micro Devices, Inc.
>> + */
>> +
>> +#include
>> +#include
>> +
>> +static void drm_test_sm2fixp(struct kunit *test)
>> +{
>> + KUNIT_EXPECT_EQ(test, 0x7fffll, ((1ull << 63) - 1));
>> +
>> + /* 1 */
>> + KUNIT_EXPECT_EQ(test, drm_int2fixp(1), drm_sm2fixp(1ull <<
>> DRM_FIXED_POINT));
>> +
>> + /* -1 */
>> + KUNIT_EXPECT_EQ(test, drm_int2fixp(-1),
>> + drm_sm2fixp((1ull << 63) | (1ull << DRM_FIXED_POINT)));
>> +
>> + /* 0.5 */
>> + KUNIT_EXPECT_EQ(test, drm_fixp_from_fraction(1, 2),
>> + drm_sm2fixp(1ull << (DRM_FIXED_POINT - 1)));
>> +
>> + /* -0.5 */
>> + KUNIT_EXPECT_EQ(test, drm_fixp_from_fraction(-1, 2),
>> + drm_sm2fixp((1ull << 63) | (1ull << (DRM_FIXED_POINT -
>> 1;
>> +}
>> +
>> +static void drm_test_int2fixp(struct kunit *test)
>> +{
>> + /* 1 */
>> + KUNIT_EXPECT_EQ(test, 1ll << 32, drm_int2fixp(1));
>> +
>> + /* -1 */
>> + KUNIT_EXPECT_EQ(test, -(1ll << 32), drm_int2fixp(-1));
>> +
>> + /* 1 + (-1) = 0 */
>> + KUNIT_EXPECT_EQ(test, 0, drm_int2fixp(1) + drm_int2fixp(-1));
>> +
>> + /* 1 / 2 */
>> + KUNIT_EXPECT_EQ(test, 1ll << 31, drm_fixp_from_fraction(1, 2));
>> +
>> + /* -0.5 */
>> + KUNIT_EXPECT_EQ(test, -(1ll << 31), drm_fixp_from_fraction(-1, 2));
>> +
>> + /* (1 / 2) + (-1) = 0.5 */
>> + KUNIT_EXPECT_EQ(test, 1ll << 31, drm_fixp_from_fraction(-1, 2) +
>> drm_int2fixp(1));
>> +
>> + /* (1 / 2) - 1) = 0.5 */
>> + KUNIT_EXPECT_EQ(test, -(1ll << 31), drm_fixp_from_fraction(1, 2)
>> + drm_int2fixp(-1));
>> +
>> + /* (1 / 2) - 1) = 0.5 */
>> + KUNIT_EXPECT_EQ(test, -(1ll << 31), drm_fixp_from_fraction(1, 2)
>> - drm_int2fixp(1));
>> +}
>> +
>> +static struct kunit_case drm_fixp_tests[] = {
>> + KUNIT_CASE(drm_test_int2fixp),
>> + KUNIT_CASE(drm_test_sm2fixp),
>> + { }
>> +};
>> +
>> +static struct kunit_suite drm_rect_test_suite = {
>
> Instead of `drm_rect_test_suite`, how about `drm_fixp_test_suite`?
>
Of course. I forgot to rename it when copy-pasting the
example code.
Thanks.
Harry
>> + .name = "drm_fixp",
>> + .test_cases = drm_fixp_tests,
>> +};
>> +
>> +kunit_test_suite(drm_rect_test_suite);
>> +
>> +MODULE_AUTHOR("AMD");
>> +MODULE_LICENSE("GPL and additional rights");
>
> From the kernel docs, "GPL and additional rights" shouldn't be used in
> new code [1].
>
> [1] https://docs.kernel.org/process/license-rules.html#id1
>
> Best Regards,
> - Maíra
>
>> +MODULE_DESCRIPTION("Unit tests for drm_fixed.h");
>
