Re: [PATCH] drm/tests: Alloc drm_device on drm_exec tests
On 28/07/23 11:33, Maxime Ripard wrote:
> Hi,
>
> On Thu, Jul 27, 2023 at 04:22:59PM -0300, Arthur Grillo wrote:
>> The drm_exec tests where crashing[0] because of a null dereference. This
>> is caused by a new access of the `driver` attribute of `struct
>> drm_driver` on drm_gem_private_object_init(). Alloc the drm_device to
>> fix that.
>>
>> [0]
>> [15:05:24] == drm_exec (6 subtests) ===
>> [15:05:24] [PASSED] sanitycheck
>> ^CERROR:root:Build interruption occurred. Cleaning console.
>> [15:05:50] [ERROR] Test: drm_exec: missing expected subtest!
>> [15:05:50] BUG: kernel NULL pointer dereference, address: 00b0
>> [15:05:50] #PF: supervisor read access in kernel mode
>> [15:05:50] #PF: error_code(0x) - not-present page
>> [15:05:50] PGD 0 P4D 0
>> [15:05:50] Oops: [#1] PREEMPT NOPTI
>> [15:05:50] CPU: 0 PID: 23 Comm: kunit_try_catch Tainted: G N
>> 6.4.0-rc7-02032-ge6303f323b1a #69
>> [15:05:50] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
>> 1.16.2-1.fc37 04/01/2014
>> [15:05:50] RIP: 0010:drm_gem_private_object_init+0x60/0xc0
>>
>> Fixes: e6303f323b1a ("drm: manager to keep track of GPUs VA mappings")
>> Signed-off-by: Arthur Grillo
>
> You should Cc the list of people returned by scripts/get_maintainers.pl
> for all your patches.
>
The email of my patch that arrived in my inbox has all the people
returned by scripts/get_maintainers.pl on Cc, but the email on the list
has some people left out. Maybe is something wrong with my git
send-email configuration. I will have a look on that.
Best Regards,
~Arthur Grillo
>> ---
>> drivers/gpu/drm/tests/drm_exec_test.c | 36 +--
>> 1 file changed, 29 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/tests/drm_exec_test.c
>> b/drivers/gpu/drm/tests/drm_exec_test.c
>> index 727ac267682e..df31f89a7945 100644
>> --- a/drivers/gpu/drm/tests/drm_exec_test.c
>> +++ b/drivers/gpu/drm/tests/drm_exec_test.c
>> @@ -12,11 +12,31 @@
>>
>> #include
>> #include
>> +#include
>> #include
>> +#include
>>
>> #include "../lib/drm_random.h"
>>
>> -static struct drm_device dev;
>> +static struct device *dev;
>> +static struct drm_device *drm;
>> +
>> +static int test_init(struct kunit *test)
>> +{
>> +dev = drm_kunit_helper_alloc_device(test);
>> +KUNIT_ASSERT_NOT_ERR_OR_NULL(test, dev);
>> +
>> +drm = __drm_kunit_helper_alloc_drm_device(test, dev, sizeof(*drm), 0,
>> + DRIVER_MODESET);
>> +KUNIT_ASSERT_NOT_ERR_OR_NULL(test, drm);
>> +
>> +return 0;
>> +}
>> +
>> +static void test_exit(struct kunit *test)
>> +{
>> +drm_kunit_helper_free_device(test, dev);
>> +}
>
> You shouldn't be using a global variable here. test->priv is meant for
> those kind of things.
>
> Maxime
Re: [PATCH] drm/tests: Remove CONFIG_DRM_FBDEV_EMULATION on .kunitconfig
On 27/07/23 05:33, Arnd Bergmann wrote: > On Thu, Jul 27, 2023, at 00:03, Arthur Grillo wrote: >> Using the `kunit_tool` with the command: >> >> tools/testing/kunit/kunit.py run --kunitconfig=drivers/gpu/drm/tests/ >> >> Lead to this error[0]. Fix it by expliciting removing the >> CONFIG_DRM_FBDEV_EMULATION. >> >> [0] >> ERROR:root: >> WARNING: unmet direct dependencies detected for FRAMEBUFFER_CONSOLE >> Depends on [n]: VT [=n] && FB_CORE [=y] && !UML [=y] >> Selected by [y]: >> - DRM_FBDEV_EMULATION [=y] && HAS_IOMEM [=y] && DRM [=y] && !EXPERT [=n] >> > > I think that's a bug in the Kconfig files that should be fixed > by enforcing the correct set of dependencies. I have not encountered Agree, I also didn't find the error on the dependencies, so I made this patch to see what you guys thought. Maybe Javier's take is the correct fix. ~Arthur Grillo > this in my randconfig builds (with a lot of other fixes applied) > > In linux-next, CONFIG_VT cannot be disabled if CONFIG_EXPERT=n, > so this does not happen. > >> diff --git a/drivers/gpu/drm/tests/.kunitconfig >> b/drivers/gpu/drm/tests/.kunitconfig >> index 6ec04b4c979d..c50b5a12faae 100644 >> --- a/drivers/gpu/drm/tests/.kunitconfig >> +++ b/drivers/gpu/drm/tests/.kunitconfig >> @@ -1,3 +1,4 @@ >> CONFIG_KUNIT=y >> CONFIG_DRM=y >> CONFIG_DRM_KUNIT_TEST=y >> +CONFIG_DRM_FBDEV_EMULATION=n >> >> base-commit: 45b58669e532bcdfd6e1593488d1f23eabd55428 > > Changing the local config should not be required after fixing > the Kconfig files. > > Arnd
Re: [PATCH] drm/tests: Remove CONFIG_DRM_FBDEV_EMULATION on .kunitconfig
On 27/07/23 13:07, Javier Martinez Canillas wrote: > "Arnd Bergmann" writes: > >> On Thu, Jul 27, 2023, at 00:03, Arthur Grillo wrote: >>> Using the `kunit_tool` with the command: >>> >>> tools/testing/kunit/kunit.py run --kunitconfig=drivers/gpu/drm/tests/ >>> >>> Lead to this error[0]. Fix it by expliciting removing the >>> CONFIG_DRM_FBDEV_EMULATION. >>> >>> [0] >>> ERROR:root: >>> WARNING: unmet direct dependencies detected for FRAMEBUFFER_CONSOLE >>> Depends on [n]: VT [=n] && FB_CORE [=y] && !UML [=y] >>> Selected by [y]: >>> - DRM_FBDEV_EMULATION [=y] && HAS_IOMEM [=y] && DRM [=y] && !EXPERT [=n] >>> >> >> I think that's a bug in the Kconfig files that should be fixed >> by enforcing the correct set of dependencies. I have not encountered >> this in my randconfig builds (with a lot of other fixes applied) >> >> In linux-next, CONFIG_VT cannot be disabled if CONFIG_EXPERT=n, >> so this does not happen. >> > >>> diff --git a/drivers/gpu/drm/tests/.kunitconfig >>> b/drivers/gpu/drm/tests/.kunitconfig >>> index 6ec04b4c979d..c50b5a12faae 100644 >>> --- a/drivers/gpu/drm/tests/.kunitconfig >>> +++ b/drivers/gpu/drm/tests/.kunitconfig >>> @@ -1,3 +1,4 @@ >>> CONFIG_KUNIT=y >>> CONFIG_DRM=y >>> CONFIG_DRM_KUNIT_TEST=y >>> +CONFIG_DRM_FBDEV_EMULATION=n >>> >>> base-commit: 45b58669e532bcdfd6e1593488d1f23eabd55428 >> >> Changing the local config should not be required after fixing >> the Kconfig files. >> > > CONFIG_VT can only be disabled if CONFIG_EXPERT=y but I also see that it > does not default to 'y' if !UML. Also FRAMEBUFFER_CONSOLE depends on !UML > but DRM_FBDEV_EMULATION selects FRAMEBUFFER_CONSOLE if !EXPERT. > > Maybe we should include !UML in that condition to? Something like this: > > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig > index 0d499669d653..734332f222ea 100644 > --- a/drivers/gpu/drm/Kconfig > +++ b/drivers/gpu/drm/Kconfig > @@ -135,7 +135,7 @@ config DRM_DEBUG_MODESET_LOCK > config DRM_FBDEV_EMULATION > bool "Enable legacy fbdev support for your modesetting driver" > depends on DRM > - select FRAMEBUFFER_CONSOLE if !EXPERT > + select FRAMEBUFFER_CONSOLE if (!EXPERT && !UML) > select FRAMEBUFFER_CONSOLE_DETECT_PRIMARY if FRAMEBUFFER_CONSOLE > default y > help > > > With that I'm able to run the DRM kunit tests wihtout the mentioned > problem. But I'm not sure if that is the correct fix or not. It works here too, I just don't understand why this commit caused this bug, as it did not touch this line. >
Re: [PATCH v4] drm/vkms: Add support to 1D gamma LUT
On 02/07/23 18:37, Maira Canal wrote: > On 6/27/23 05:12, Pekka Paalanen wrote: >> On Mon, 26 Jun 2023 14:35:25 -0300 >> Maira Canal wrote: >> >>> Hi Pekka, >>> >>> On 6/26/23 05:17, Pekka Paalanen wrote: On Sat, 24 Jun 2023 18:48:08 -0300 Maira Canal wrote: > Hi Arthur, > > Thanks for working on this feature for the VKMS! > > On 6/21/23 16:41, Arthur Grillo wrote: >> Support a 1D gamma LUT with interpolation for each color channel on the >> VKMS driver. Add a check for the LUT length by creating >> vkms_atomic_check(). >> >> Tested with: >> igt@kms_color@gamma >> igt@kms_color@legacy-gamma >> igt@kms_color@invalid-gamma-lut-sizes > > Could you also mention that this will make it possible to run the test > igt@kms_plane@pixel-format? > > Also, you mentioned to me that the performance degraded with this new > feature, but I wasn't able to see it while running the VKMS CI. I > performed a couple of tests and I didn't see any significant performance > issue. > > Could you please run a benchmark and share the results with us? This way > we can atest that this new feature will not affect significantly the > VKMS performance. It would be nice to have a small brief of this > benchmark on the commit message as well. > > Attesting that there isn't a performance issue and adding those nits to > the commit message, you can add my > > Reviewed-by: Maíra Canal > > on the next version. Hi, perfomance testing is good indeed. As future work, could there be a document describing how to test VKMS performance? >>> >>> I'll try to select a couple of more meaningful IGT tests to describe how >>> to test the VKMS performance and also add a document to describe how to >>> run this tests. >>> >>> Recently, I added a VKMS must-pass testlist to IGT. This testlist >>> tries to assure that regressions will not be introduced into VKMS. But, >>> I failed to introduce a documentation on the kernel side pointing to >>> this new testlist... I'll also work on it. >>> "I ran IGT@blah 100 times and it took xx seconds before and yy seconds after" does not really give someone like me an idea of what was actually measured. For example blending overhead increase could be completely lost in opaque pixel copying noise if the test case has only few pixels to blend, e.g. a cursor plane, not to mention the overhead of launching an IGT test in the first place. >>> >>> About the IGT overhead, I don't know exactly how we could escape from >>> it. Maybe writing KUnit tests to the VKMS's composition functions, such >>> as blend(). Anyway, we would have the overhead of the KUnit framework. >>> I mean, for whatever framework we choose, there'll be an overhead... >>> >>> Do you have any other ideas on how to test VKMS with less overhead? >> >> Maybe put the repeat loop and time measurement inside the code of a few >> chosen IGT tests? >> >> So that it loops only the KMS programming and somehow ensures VKMS has >> finished processing each update before doing the next cycle. I presume >> VKMS does not have a timer-based refresh cycle that might add CPU idle >> time? Writeback should be included in the measurement too, but inspecting >> writeback results should not. >> >> Once all that is in place, then each performance test needs to use >> appropriate operations. E.g. if testing blending performance, use >> almost full-screen planes. > > ^ Grillo, any chance you could work on something like this for the > performance measurements? > Yeah, I can do something like this. Sorry for the delay I think I will have those measurements on this week. ~Grillo >> >> What's the overhead of KUnit framework? Can you not do the same there, >> put the repeat loop and time measurement inside the test to cover only >> the interesting code? >> >> Unit-testing the composition function performance might be ideal. >> > > I'll try to work on some unit tests for, at least, the composition > section of VKMS. I believe that they will be very valuable for the > maintenance and performance evaluation. > > Thanks for your valuable inputs on the VKMS! > > Best Regards, > - Maíra > >> Depending on the type of test, if the CRTC mode and planes are big >> enough, maybe there is no need to repeat even. But testing presumably >> fast things like moving a cursor plane will likely need repeating in >> order to produce stable numbers. >> >> >> Thanks, >> pq >> >>> >>> Best Regards, >>> - Maíra >>> Something that would guide new developers in running meaningful benchmarks would be nice. Should e.g. IGT have explicit (VKMS) performance tests that need to be run manually, since evaluation of the result is not feasible automatically? Or a benchmark mode in correctness tests that would run the identical operation N times and measure the time be
Re: [PATCH] drm/tests: Add test case for drm_rect_clip_scaled()
On 14/06/23 14:54, nelsonbogado99 wrote:
> From: Nelson Bogado
>
> To evaluate the behavior of the drm_rect_clip_scaled() helper function
> with misplaced rectangles and improve the robustness and quality of it.
>
> The new test was executed using the following command:
>
> $ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/gpu/drm/tests/
> [01:48:12] == drm_rect (10 subtests) ==
> ...
> [01:48:12] [PASSED] drm_test_rect_clip_scaled_out_of_bounds
>
> Signed-off-by: Nelson Bogado
> ---
> drivers/gpu/drm/tests/drm_rect_test.c | 53 +++
> 1 file changed, 53 insertions(+)
Hello,
here are a couple of suggestions to make the code more readable ;).
>
> diff --git a/drivers/gpu/drm/tests/drm_rect_test.c
> b/drivers/gpu/drm/tests/drm_rect_test.c
> index 76332cd2ead8..b3bfdd420123 100644
> --- a/drivers/gpu/drm/tests/drm_rect_test.c
> +++ b/drivers/gpu/drm/tests/drm_rect_test.c
> @@ -209,6 +209,58 @@ static void
> drm_test_rect_clip_scaled_signed_vs_unsigned(struct kunit *test)
> KUNIT_EXPECT_FALSE_MSG(test, drm_rect_visible(&src), "Source should not
> be visible\n");
> }
>
> +static void drm_test_rect_clip_scaled_out_of_bounds(struct kunit *test)
> +{
> + /* Definition of the rectangles and visible variables */
I think it would be great to decrease the amount of comments, you don't need
to comment that you're declaring some variable. Maybe just keep the comments
that explain what you're testing
> + struct drm_rect src, dst, clip;
> + bool visible;
> +
> + /*
> + * Both rectangles are completely out of bounds, initialize the src,
> + * dst and clip rectangles with specific coordinates and sizes.
> + */
like this one.
> + drm_rect_init(&src, -10, -10, -5, -5);
> + drm_rect_init(&dst, -20, -20, -15, -15);
> + drm_rect_init(&clip, 0, 0, 100, 100);
> +
> + /* Function call drm_rect_clip_scaled to determine visibility */
> + visible = drm_rect_clip_scaled(&src, &dst, &clip);
> +
> + /* Check expected results */
> + KUNIT_EXPECT_FALSE_MSG(test, visible, "Destination should not be
> visible\n");
> + KUNIT_EXPECT_FALSE_MSG(test, drm_rect_visible(&src), "Source should not
> be visible\n");
> +
> + /*
> + * Only source rectangle is out of bounds, reinitialize the src,
> + * dst and clip rectangles with new values.
> + */
> + drm_rect_init(&src, -10, -10, -5, -5);
Use `DRM_RECT_INIT` instead, this way you don't need to pass the variable as an
argument. I think it would be more readable.
~Arthur Grillo
> + drm_rect_init(&dst, 0, 0, 10, 10);
> + drm_rect_init(&clip, 0, 0, 100, 100);
> +
> + /* Function call drm_rect_clip_scaled to determine visibility */
> + visible = drm_rect_clip_scaled(&src, &dst, &clip);
> +
> + /* Check expected results */
> + KUNIT_EXPECT_TRUE_MSG(test, visible, "Destination should be
> visible\n\n");
> + KUNIT_EXPECT_FALSE_MSG(test, drm_rect_visible(&src), "Source should not
> be visible\n");
> +
> + /*
> + * Only source rectangle is out of bounds, reinitialize the src,
> + * dst and clip rectangles with new values.
> + */
> + drm_rect_init(&src, 0, 0, 10, 10);
> + drm_rect_init(&dst, -10, -10, -5, -5);
> + drm_rect_init(&clip, 0, 0, 100, 100);
> +
> + /* Function call drm_rect_clip_scaled to determine visibility */
> + visible = drm_rect_clip_scaled(&src, &dst, &clip);
> +
> + /* Check expected results */
> + KUNIT_EXPECT_FALSE_MSG(test, visible, "Destination should not be
> visible\n");
> + KUNIT_EXPECT_FALSE_MSG(test, drm_rect_visible(&src), "Source should not
> be visible\n");
> +}
> +
> struct drm_rect_intersect_case {
> const char *description;
> struct drm_rect r1, r2;
> @@ -511,6 +563,7 @@ static struct kunit_case drm_rect_tests[] = {
> KUNIT_CASE(drm_test_rect_clip_scaled_not_clipped),
> KUNIT_CASE(drm_test_rect_clip_scaled_clipped),
> KUNIT_CASE(drm_test_rect_clip_scaled_signed_vs_unsigned),
> + KUNIT_CASE(drm_test_rect_clip_scaled_out_of_bounds),
> KUNIT_CASE_PARAM(drm_test_rect_intersect,
> drm_rect_intersect_gen_params),
> KUNIT_CASE_PARAM(drm_test_rect_calc_hscale, drm_rect_scale_gen_params),
> KUNIT_CASE_PARAM(drm_test_rect_calc_vscale, drm_rect_scale_gen_params),
Re: [PATCH v2] drm/vkms: Fix race-condition between the hrtimer and the atomic commit
On 23/05/23 09:32, Maíra Canal wrote:
> Currently, it is possible for the composer to be set as enabled and then
> as disabled without a proper call for the vkms_vblank_simulate(). This
> is problematic, because the driver would skip one CRC output, causing CRC
> tests to fail. Therefore, we need to make sure that, for each time the
> composer is set as enabled, a composer job is added to the queue.
>
> In order to provide this guarantee, add a mutex that will lock before
> the composer is set as enabled and will unlock only after the composer
> job is added to the queue. This way, we can have a guarantee that the
> driver won't skip a CRC entry.
>
> This race-condition is affecting the IGT test "writeback-check-output",
> making the test fail and also, leaking writeback framebuffers, as the
> writeback job is queued, but it is not signaled. This patch avoids both
> problems.
>
> [v2]:
> * Create a new mutex and keep the spinlock across the atomic commit in
> order to avoid interrupts that could result in deadlocks.
>
> Signed-off-by: Maíra Canal
Great catch!
Reviewed-by: Arthur Grillo
> ---
> drivers/gpu/drm/vkms/vkms_composer.c | 9 +++--
> drivers/gpu/drm/vkms/vkms_crtc.c | 9 +
> drivers/gpu/drm/vkms/vkms_drv.h | 4 +++-
> 3 files changed, 15 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/vkms/vkms_composer.c
> b/drivers/gpu/drm/vkms/vkms_composer.c
> index 906d3df40cdb..b12188fd6b38 100644
> --- a/drivers/gpu/drm/vkms/vkms_composer.c
> +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> @@ -320,10 +320,15 @@ void vkms_set_composer(struct vkms_output *out, bool
> enabled)
> if (enabled)
> drm_crtc_vblank_get(&out->crtc);
>
> - spin_lock_irq(&out->lock);
> + mutex_lock(&out->enabled_lock);
> old_enabled = out->composer_enabled;
> out->composer_enabled = enabled;
> - spin_unlock_irq(&out->lock);
> +
> + /* the composition wasn't enabled, so unlock the lock to make sure the
> lock
> + * will be balanced even if we have a failed commit
> + */
> + if (!out->composer_enabled)
> + mutex_unlock(&out->enabled_lock);
>
> if (old_enabled)
> drm_crtc_vblank_put(&out->crtc);
> diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c
> b/drivers/gpu/drm/vkms/vkms_crtc.c
> index 515f6772b866..3079013c8b32 100644
> --- a/drivers/gpu/drm/vkms/vkms_crtc.c
> +++ b/drivers/gpu/drm/vkms/vkms_crtc.c
> @@ -16,7 +16,7 @@ static enum hrtimer_restart vkms_vblank_simulate(struct
> hrtimer *timer)
> struct drm_crtc *crtc = &output->crtc;
> struct vkms_crtc_state *state;
> u64 ret_overrun;
> - bool ret, fence_cookie;
> + bool ret, fence_cookie, composer_enabled;
>
> fence_cookie = dma_fence_begin_signalling();
>
> @@ -25,15 +25,15 @@ static enum hrtimer_restart vkms_vblank_simulate(struct
> hrtimer *timer)
> if (ret_overrun != 1)
> pr_warn("%s: vblank timer overrun\n", __func__);
>
> - spin_lock(&output->lock);
> ret = drm_crtc_handle_vblank(crtc);
> if (!ret)
> DRM_ERROR("vkms failure on handling vblank");
>
> state = output->composer_state;
> - spin_unlock(&output->lock);
> + composer_enabled = output->composer_enabled;
> + mutex_unlock(&output->enabled_lock);
>
> - if (state && output->composer_enabled) {
> + if (state && composer_enabled) {
> u64 frame = drm_crtc_accurate_vblank_count(crtc);
>
> /* update frame_start only if a queued vkms_composer_worker()
> @@ -292,6 +292,7 @@ int vkms_crtc_init(struct drm_device *dev, struct
> drm_crtc *crtc,
>
> spin_lock_init(&vkms_out->lock);
> spin_lock_init(&vkms_out->composer_lock);
> + mutex_init(&vkms_out->enabled_lock);
>
> vkms_out->composer_workq = alloc_ordered_workqueue("vkms_composer", 0);
> if (!vkms_out->composer_workq)
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> index 5f1a0a44a78c..dcf4e302fb4d 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.h
> +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> @@ -100,8 +100,10 @@ struct vkms_output {
> struct workqueue_struct *composer_workq;
> /* protects concurrent access to composer */
> spinlock_t lock;
> + /* guarantees that if the composer is enabled, a job will be queued */
> + struct mutex enabled_lock;
>
> - /* protected by @lock */
> + /* protected by @enabled_lock */
> bool composer_enabled;
> struct vkms_crtc_state *composer_state;
>
Re: [PATCH 0/3] drm/vkms: Minor Improvements
On 15/05/23 10:52, Maíra Canal wrote: > This series addresses some minor improvements to the writeback > functionality. The first patch intends to reduce the critical section > of a spinlock by removing assignments that don't need to be protected > by a lock. The second patch enables the support for ARGB on the > writeback. Finally, the third patch refactors the pixel conversion > functions of the writeback functionality. This patch is a follow-up of > a previous patchset [1], in which Melissa suggested to apply the same > refactor to the writeback pixel conversion functions. > > [1] > https://lore.kernel.org/dri-devel/20230418130525.128733-1-mca...@igalia.com/T/ > > Best Regards, > - Maíra On the entire patchset, Reviewed-by: Arthur Grillo Best Regards, ~Arthur Grillo > > Maíra Canal (3): > drm/vkms: Reduce critical section > drm/vkms: Enable ARGB support for writeback > drm/vkms: Isolate writeback pixel conversion functions > > drivers/gpu/drm/vkms/vkms_composer.c | 4 +- > drivers/gpu/drm/vkms/vkms_drv.h | 4 +- > drivers/gpu/drm/vkms/vkms_formats.c | 140 +++--- > drivers/gpu/drm/vkms/vkms_formats.h | 2 +- > drivers/gpu/drm/vkms/vkms_writeback.c | 9 +- > 5 files changed, 68 insertions(+), 91 deletions(-) >
Re: [PATCH v3 1/2] drm: Add fixed-point helper to get rounded integer values
On 12/05/23 07:40, Maíra Canal wrote:
> Create a new fixed-point helper to allow us to return the rounded value
> of our fixed point value.
>
> [v2]:
> * Create the function drm_fixp2int_round() (Melissa Wen).
> [v3]:
> * Use drm_fixp2int() instead of shifting manually (Arthur Grillo).
>
> Signed-off-by: Maíra Canal
> ---
Reviewed-by: Arthur Grillo
Best Regards,
~Arthur Grillo
> include/drm/drm_fixed.h | 6 ++
> 1 file changed, 6 insertions(+)
>
> diff --git a/include/drm/drm_fixed.h b/include/drm/drm_fixed.h
> index 255645c1f9a8..6ea339d5de08 100644
> --- a/include/drm/drm_fixed.h
> +++ b/include/drm/drm_fixed.h
> @@ -71,6 +71,7 @@ static inline u32 dfixed_div(fixed20_12 A, fixed20_12 B)
> }
>
> #define DRM_FIXED_POINT 32
> +#define DRM_FIXED_POINT_HALF 16
> #define DRM_FIXED_ONE(1ULL << DRM_FIXED_POINT)
> #define DRM_FIXED_DECIMAL_MASK (DRM_FIXED_ONE - 1)
> #define DRM_FIXED_DIGITS_MASK(~DRM_FIXED_DECIMAL_MASK)
> @@ -87,6 +88,11 @@ static inline int drm_fixp2int(s64 a)
> return ((s64)a) >> DRM_FIXED_POINT;
> }
>
> +static inline int drm_fixp2int_round(s64 a)
> +{
> + return drm_fixp2int(a + (1 << (DRM_FIXED_POINT_HALF - 1)));
> +}
> +
> static inline int drm_fixp2int_ceil(s64 a)
> {
> if (a > 0)
Re: [PATCH v2 2/2] drm/vkms: Fix RGB565 pixel conversion
On 07/05/23 17:28, Maíra Canal wrote:
> Currently, the pixel conversion isn't rounding the fixed-point values
> before assigning it to the RGB coefficients, which is causing the IGT
> pixel-format tests to fail. So, use the drm_fixp2int_round() fixed-point
> helper to round the values when assigning it to the RGB coefficients.
>
> Tested with igt@kms_plane@pixel-format and
> igt@kms_plane@pixel-format-source-clamping.
>
> Fixes: 89b03aeaef16 ("drm/vkms: fix 32bit compilation error by replacing
> macros")
> Signed-off-by: Maíra Canal
> ---
Reviewed-by: Arthur Grillo
Best Regards,
~Arthur Grillo
>
> v1 -> v2:
> https://lore.kernel.org/dri-devel/20230425153353.238844-1-mca...@igalia.com/T/
>
> * Use drm_fixp2int_round() to fix the pixel conversion instead of casting
> the values to s32 (Melissa Wen).
>
> ---
> drivers/gpu/drm/vkms/vkms_formats.c | 12 ++--
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/vkms/vkms_formats.c
> b/drivers/gpu/drm/vkms/vkms_formats.c
> index 8d948c73741e..b11342026485 100644
> --- a/drivers/gpu/drm/vkms/vkms_formats.c
> +++ b/drivers/gpu/drm/vkms/vkms_formats.c
> @@ -97,9 +97,9 @@ static void RGB565_to_argb_u16(u8 *src_pixels, struct
> pixel_argb_u16 *out_pixel)
> s64 fp_b = drm_int2fixp(rgb_565 & 0x1f);
>
> out_pixel->a = (u16)0x;
> - out_pixel->r = drm_fixp2int(drm_fixp_mul(fp_r, fp_rb_ratio));
> - out_pixel->g = drm_fixp2int(drm_fixp_mul(fp_g, fp_g_ratio));
> - out_pixel->b = drm_fixp2int(drm_fixp_mul(fp_b, fp_rb_ratio));
> + out_pixel->r = drm_fixp2int_round(drm_fixp_mul(fp_r, fp_rb_ratio));
> + out_pixel->g = drm_fixp2int_round(drm_fixp_mul(fp_g, fp_g_ratio));
> + out_pixel->b = drm_fixp2int_round(drm_fixp_mul(fp_b, fp_rb_ratio));
> }
>
> void vkms_compose_row(struct line_buffer *stage_buffer, struct
> vkms_plane_state *plane, int y)
> @@ -216,9 +216,9 @@ static void argb_u16_to_RGB565(struct vkms_frame_info
> *frame_info,
> s64 fp_g = drm_int2fixp(in_pixels[x].g);
> s64 fp_b = drm_int2fixp(in_pixels[x].b);
>
> - u16 r = drm_fixp2int(drm_fixp_div(fp_r, fp_rb_ratio));
> - u16 g = drm_fixp2int(drm_fixp_div(fp_g, fp_g_ratio));
> - u16 b = drm_fixp2int(drm_fixp_div(fp_b, fp_rb_ratio));
> + u16 r = drm_fixp2int_round(drm_fixp_div(fp_r, fp_rb_ratio));
> + u16 g = drm_fixp2int_round(drm_fixp_div(fp_g, fp_g_ratio));
> + u16 b = drm_fixp2int_round(drm_fixp_div(fp_b, fp_rb_ratio));
>
> *dst_pixels = cpu_to_le16(r << 11 | g << 5 | b);
> }
Re: [PATCH v2 1/2] drm: Add fixed-point helper to get rounded integer values
On 07/05/23 17:28, Maíra Canal wrote:
> Create a new fixed-point helper to allow us to return the rounded value
> of our fixed-point value.
>
> Signed-off-by: Maíra Canal
> ---
>
> v1 -> v2:
> https://lore.kernel.org/dri-devel/20230425153353.238844-1-mca...@igalia.com/T/
>
> * New patch
> * Create the function drm_fixp2int_round() (Melissa Wen).
>
> ---
> include/drm/drm_fixed.h | 6 ++
> 1 file changed, 6 insertions(+)
>
> diff --git a/include/drm/drm_fixed.h b/include/drm/drm_fixed.h
> index 255645c1f9a8..d695d0411e2d 100644
> --- a/include/drm/drm_fixed.h
> +++ b/include/drm/drm_fixed.h
> @@ -71,6 +71,7 @@ static inline u32 dfixed_div(fixed20_12 A, fixed20_12 B)
> }
>
> #define DRM_FIXED_POINT 32
> +#define DRM_FIXED_POINT_HALF 16
> #define DRM_FIXED_ONE(1ULL << DRM_FIXED_POINT)
> #define DRM_FIXED_DECIMAL_MASK (DRM_FIXED_ONE - 1)
> #define DRM_FIXED_DIGITS_MASK(~DRM_FIXED_DECIMAL_MASK)
> @@ -87,6 +88,11 @@ static inline int drm_fixp2int(s64 a)
> return ((s64)a) >> DRM_FIXED_POINT;
> }
>
> +static inline int drm_fixp2int_round(s64 a)
> +{
> + return ((s64)a + (1 << (DRM_FIXED_POINT_HALF - 1))) >> DRM_FIXED_POINT;
I think it would be great to use drm_fixp2int, instead of shifting manually,
like drm_fixp2int_ceil does.
Best Regards,
~Arthur Grillo
> +}
> +
> static inline int drm_fixp2int_ceil(s64 a)
> {
> if (a > 0)
Re: [PATCH v2 1/2] drm/vkms: allow full alpha blending on all planes
On 20/04/23 20:22, Maíra Canal wrote:
> Before commit bc0d7fdefec6 ("drm: vkms: Supports to the case where
> primary plane doesn't match the CRTC"), the composition was executed on
> top of the primary plane. Therefore, the primary plane was not able to
> support the alpha channel. After commit bc0d7fdefec6, this is possible,
> as the composition is now executed on top of the CRTC.
>
> So, allow all planes to support the alpha channel, making full alpha
> blending possible in vkms.
>
> Signed-off-by: Maíra Canal
> Reviewed-by: Melissa Wen
> ---
On both:
Reviewed-by: Arthur Grillo
Best Regards,
~Arthur Grillo
>
> v1 -> v2:
> https://lore.kernel.org/dri-devel/20230414190913.106633-1-mca...@igalia.com/T/
>
> * s/vkms_primary_helper_funcs/vkms_plane_helper_funcs (Melissa Wen)
> * Add Melissa's Reviewed-by
>
> ---
> drivers/gpu/drm/vkms/vkms_plane.c | 34 +++
> 1 file changed, 3 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/gpu/drm/vkms/vkms_plane.c
> b/drivers/gpu/drm/vkms/vkms_plane.c
> index c41cec7dcb70..c2888e5266bc 100644
> --- a/drivers/gpu/drm/vkms/vkms_plane.c
> +++ b/drivers/gpu/drm/vkms/vkms_plane.c
> @@ -12,12 +12,6 @@
> #include "vkms_formats.h"
>
> static const u32 vkms_formats[] = {
> - DRM_FORMAT_XRGB,
> - DRM_FORMAT_XRGB16161616,
> - DRM_FORMAT_RGB565
> -};
> -
> -static const u32 vkms_plane_formats[] = {
> DRM_FORMAT_ARGB,
> DRM_FORMAT_XRGB,
> DRM_FORMAT_XRGB16161616,
> @@ -185,7 +179,7 @@ static void vkms_cleanup_fb(struct drm_plane *plane,
> drm_gem_fb_vunmap(fb, shadow_plane_state->map);
> }
>
> -static const struct drm_plane_helper_funcs vkms_primary_helper_funcs = {
> +static const struct drm_plane_helper_funcs vkms_plane_helper_funcs = {
> .atomic_update = vkms_plane_atomic_update,
> .atomic_check = vkms_plane_atomic_check,
> .prepare_fb = vkms_prepare_fb,
> @@ -196,38 +190,16 @@ struct vkms_plane *vkms_plane_init(struct vkms_device
> *vkmsdev,
> enum drm_plane_type type, int index)
> {
> struct drm_device *dev = &vkmsdev->drm;
> - const struct drm_plane_helper_funcs *funcs;
> struct vkms_plane *plane;
> - const u32 *formats;
> - int nformats;
> -
> - switch (type) {
> - case DRM_PLANE_TYPE_PRIMARY:
> - formats = vkms_formats;
> - nformats = ARRAY_SIZE(vkms_formats);
> - funcs = &vkms_primary_helper_funcs;
> - break;
> - case DRM_PLANE_TYPE_CURSOR:
> - case DRM_PLANE_TYPE_OVERLAY:
> - formats = vkms_plane_formats;
> - nformats = ARRAY_SIZE(vkms_plane_formats);
> - funcs = &vkms_primary_helper_funcs;
> - break;
> - default:
> - formats = vkms_formats;
> - nformats = ARRAY_SIZE(vkms_formats);
> - funcs = &vkms_primary_helper_funcs;
> - break;
> - }
>
> plane = drmm_universal_plane_alloc(dev, struct vkms_plane, base, 1 <<
> index,
> &vkms_plane_funcs,
> -formats, nformats,
> +vkms_formats,
> ARRAY_SIZE(vkms_formats),
> NULL, type, NULL);
> if (IS_ERR(plane))
> return plane;
>
> - drm_plane_helper_add(&plane->base, funcs);
> + drm_plane_helper_add(&plane->base, &vkms_plane_helper_funcs);
>
> return plane;
> }
Re: [PATCH v4 3/5] drm/tests: Add test cases for drm_rect_calc_vscale()
On 17/04/23 13:19, Maíra Canal wrote:
> On 4/6/23 08:53, Arthur Grillo wrote:
>> Insert parameterized test for the drm_rect_calc_vscale() to ensure
>> correctness and prevent future regressions. Besides the test for the
>> usual case, tests the exceptions.
>>
>> It uses the same struct from drm_rect_calc_hscale().
>>
>> Signed-off-by: Arthur Grillo
>> ---
>> drivers/gpu/drm/tests/drm_rect_test.c | 59 +++
>> 1 file changed, 59 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/tests/drm_rect_test.c
>> b/drivers/gpu/drm/tests/drm_rect_test.c
>> index 44150545c1bc..a1fd9b2c5128 100644
>> --- a/drivers/gpu/drm/tests/drm_rect_test.c
>> +++ b/drivers/gpu/drm/tests/drm_rect_test.c
>> @@ -414,6 +414,64 @@ static void drm_test_rect_calc_hscale(struct kunit
>> *test)
>> KUNIT_EXPECT_EQ(test, scaling_factor, params->expected_scaling_factor);
>> }
>> +static const struct drm_rect_scale_case drm_rect_vscale_cases[] = {
>> +{
>> +.name = "normal use",
>> +.src = DRM_RECT_INIT(0, 0, 0, 2 << 16),
>> +.dst = DRM_RECT_INIT(0, 0, 0, 1 << 16),
>> +.min_range = 0, .max_range = INT_MAX,
>> +.expected_scaling_factor = 2,
>> +},
>> +{
>> +.name = "out of max range",
>> +.src = DRM_RECT_INIT(0, 0, 0, 10 << 16),
>> +.dst = DRM_RECT_INIT(0, 0, 0, 1 << 16),
>> +.min_range = 3, .max_range = 5,
>> +.expected_scaling_factor = -ERANGE,
>> +},
>> +{
>> +.name = "out of min range",
>> +.src = DRM_RECT_INIT(0, 0, 0, 2 << 16),
>> +.dst = DRM_RECT_INIT(0, 0, 0, 1 << 16),
>> +.min_range = 3, .max_range = 5,
>> +.expected_scaling_factor = -ERANGE,
>> +},
>> +{
>> +.name = "zero dst height",
>> +.src = DRM_RECT_INIT(0, 0, 0, 2 << 16),
>> +.dst = DRM_RECT_INIT(0, 0, 0, 0 << 16),
>> +.min_range = 0, .max_range = INT_MAX,
>> +.expected_scaling_factor = 0,
>> +},
>> +{
>> +.name = "negative src height",
>> +.src = DRM_RECT_INIT(0, 0, 0, -(1 << 16)),
>> +.dst = DRM_RECT_INIT(0, 0, 0, 1 << 16),
>> +.min_range = 0, .max_range = INT_MAX,
>> +.expected_scaling_factor = -EINVAL,
>> +},
>> +{
>> +.name = "negative dst height",
>> +.src = DRM_RECT_INIT(0, 0, 0, 1 << 16),
>> +.dst = DRM_RECT_INIT(0, 0, 0, -(1 << 16)),
>> +.min_range = 0, .max_range = INT_MAX,
>> +.expected_scaling_factor = -EINVAL,
>> +},
>> +};
>> +
>
> Would it possible to use the same parameter array for vscale and hscale?
>
> Best Regards,
> - Maíra Canal
>
Instead of having drm_rect directly, I could create an src and dst
length. Or maybe have drm_rects that increase on both sizes.
Regards,
~Arthur Grillo
>> +KUNIT_ARRAY_PARAM(drm_rect_vscale, drm_rect_vscale_cases,
>> drm_rect_scale_case_desc);
>> +
>> +static void drm_test_rect_calc_vscale(struct kunit *test)
>> +{
>> +const struct drm_rect_scale_case *params = test->param_value;
>> +int scaling_factor;
>> +
>> +scaling_factor = drm_rect_calc_vscale(¶ms->src, ¶ms->dst,
>> + params->min_range, params->max_range);
>> +
>> +KUNIT_EXPECT_EQ(test, scaling_factor, params->expected_scaling_factor);
>> +}
>> +
>> static struct kunit_case drm_rect_tests[] = {
>> KUNIT_CASE(drm_test_rect_clip_scaled_div_by_zero),
>> KUNIT_CASE(drm_test_rect_clip_scaled_not_clipped),
>> @@ -421,6 +479,7 @@ static struct kunit_case drm_rect_tests[] = {
>> KUNIT_CASE(drm_test_rect_clip_scaled_signed_vs_unsigned),
>> KUNIT_CASE_PARAM(drm_test_rect_intersect,
>> drm_rect_intersect_gen_params),
>> KUNIT_CASE_PARAM(drm_test_rect_calc_hscale,
>> drm_rect_hscale_gen_params),
>> +KUNIT_CASE_PARAM(drm_test_rect_calc_vscale, drm_rect_vscale_gen_params),
>> { }
>> };
>>
Re: [PATCH v3 1/6] drm/vkms: isolate pixel conversion functionality
On 17/04/23 09:10, Maíra Canal wrote:
> Currently, the pixel conversion functions repeat the same loop to
> iterate the rows. Instead of repeating the same code for each pixel
> format, create a function to wrap the loop and isolate the pixel
> conversion functionality.
>
> Suggested-by: Arthur Grillo
> Signed-off-by: Maíra Canal
I really liked how simple the conversion function turned out :-).
Reviewed-by: Arthur Grillo
Regards,
~Arthur Grillo
> ---
> drivers/gpu/drm/vkms/vkms_composer.c | 4 +-
> drivers/gpu/drm/vkms/vkms_drv.h | 4 +-
> drivers/gpu/drm/vkms/vkms_formats.c | 125 +++
> drivers/gpu/drm/vkms/vkms_formats.h | 2 +-
> drivers/gpu/drm/vkms/vkms_plane.c| 2 +-
> 5 files changed, 56 insertions(+), 81 deletions(-)
>
> diff --git a/drivers/gpu/drm/vkms/vkms_composer.c
> b/drivers/gpu/drm/vkms/vkms_composer.c
> index 8e53fa80742b..80164e79af00 100644
> --- a/drivers/gpu/drm/vkms/vkms_composer.c
> +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> @@ -99,7 +99,7 @@ static void blend(struct vkms_writeback_job *wb,
> if (!check_y_limit(plane[i]->frame_info, y))
> continue;
>
> - plane[i]->plane_read(stage_buffer,
> plane[i]->frame_info, y);
> + vkms_compose_row(stage_buffer, plane[i], y);
> pre_mul_alpha_blend(plane[i]->frame_info, stage_buffer,
> output_buffer);
> }
> @@ -118,7 +118,7 @@ static int check_format_funcs(struct vkms_crtc_state
> *crtc_state,
> u32 n_active_planes = crtc_state->num_active_planes;
>
> for (size_t i = 0; i < n_active_planes; i++)
> - if (!planes[i]->plane_read)
> + if (!planes[i]->pixel_read)
> return -1;
>
> if (active_wb && !active_wb->wb_write)
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> index 4a248567efb2..f152d54baf76 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.h
> +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> @@ -56,8 +56,7 @@ struct vkms_writeback_job {
> struct vkms_plane_state {
> struct drm_shadow_plane_state base;
> struct vkms_frame_info *frame_info;
> - void (*plane_read)(struct line_buffer *buffer,
> -const struct vkms_frame_info *frame_info, int y);
> + void (*pixel_read)(u8 *src_buffer, struct pixel_argb_u16 *out_pixel);
> };
>
> struct vkms_plane {
> @@ -155,6 +154,7 @@ int vkms_verify_crc_source(struct drm_crtc *crtc, const
> char *source_name,
> /* Composer Support */
> void vkms_composer_worker(struct work_struct *work);
> void vkms_set_composer(struct vkms_output *out, bool enabled);
> +void vkms_compose_row(struct line_buffer *stage_buffer, struct
> vkms_plane_state *plane, int y);
>
> /* Writeback */
> int vkms_enable_writeback_connector(struct vkms_device *vkmsdev);
> diff --git a/drivers/gpu/drm/vkms/vkms_formats.c
> b/drivers/gpu/drm/vkms/vkms_formats.c
> index d4950688b3f1..bd542fd00698 100644
> --- a/drivers/gpu/drm/vkms/vkms_formats.c
> +++ b/drivers/gpu/drm/vkms/vkms_formats.c
> @@ -42,100 +42,75 @@ static void *get_packed_src_addr(const struct
> vkms_frame_info *frame_info, int y
> return packed_pixels_addr(frame_info, x_src, y_src);
> }
>
> -static void ARGB_to_argb_u16(struct line_buffer *stage_buffer,
> - const struct vkms_frame_info *frame_info, int
> y)
> +static void ARGB_to_argb_u16(u8 *src_pixels, struct pixel_argb_u16
> *out_pixel)
> {
> - struct pixel_argb_u16 *out_pixels = stage_buffer->pixels;
> - u8 *src_pixels = get_packed_src_addr(frame_info, y);
> - int x_limit = min_t(size_t, drm_rect_width(&frame_info->dst),
> - stage_buffer->n_pixels);
> -
> - for (size_t x = 0; x < x_limit; x++, src_pixels += 4) {
> - /*
> - * The 257 is the "conversion ratio". This number is obtained
> by the
> - * (2^16 - 1) / (2^8 - 1) division. Which, in this case, tries
> to get
> - * the best color value in a pixel format with more
> possibilities.
> - * A similar idea applies to others RGB color conversions.
> - */
> - out_pixels[x].a = (u16)src_pixels[3] * 257;
> - out_pixels[x].r = (u16)src_pixels[2] * 257;
> - out_pixels[x].g = (u16)src_pixels[1] * 257;
> - out_pixels[x].b = (u16)src_pixels[0] * 257;
> - }
> + /*
> + * The 257 is the "conversion ratio". This number is obtained by the
> + * (2^16 - 1) / (2^8 - 1) division. Which, in this case, tries to get
> + * the best color value in a pixel format with more possibilities.
> + * A similar idea applies to others RGB color conversions.
> + */
> + out_pixel->a = (u16)src_pixels[3] * 257;
> + out_pixel->r = (u16)src_pixels[2] * 257;
> + out_pixel->g = (u16)src_pi
Re: [PATCH v2 1/7] drm/vkms: isolate pixel conversion functionality
On 14/04/23 10:51, Maíra Canal wrote:
> Currently, the pixel conversion functions repeat the same loop to
> iterate the rows. Instead of repeating the same code for each pixel
> format, create a function to wrap the loop and isolate the pixel
> conversion functionality.
>
> Suggested-by: Arthur Grillo
> Signed-off-by: Maíra Canal
> ---
> drivers/gpu/drm/vkms/vkms_composer.c | 4 +-
> drivers/gpu/drm/vkms/vkms_drv.h | 4 +-
> drivers/gpu/drm/vkms/vkms_formats.c | 136 ---
> drivers/gpu/drm/vkms/vkms_formats.h | 2 +-
> drivers/gpu/drm/vkms/vkms_plane.c| 2 +-
> 5 files changed, 65 insertions(+), 83 deletions(-)
>
> diff --git a/drivers/gpu/drm/vkms/vkms_composer.c
> b/drivers/gpu/drm/vkms/vkms_composer.c
> index 8e53fa80742b..80164e79af00 100644
> --- a/drivers/gpu/drm/vkms/vkms_composer.c
> +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> @@ -99,7 +99,7 @@ static void blend(struct vkms_writeback_job *wb,
> if (!check_y_limit(plane[i]->frame_info, y))
> continue;
>
> - plane[i]->plane_read(stage_buffer,
> plane[i]->frame_info, y);
> + vkms_compose_row(stage_buffer, plane[i], y);
> pre_mul_alpha_blend(plane[i]->frame_info, stage_buffer,
> output_buffer);
> }
> @@ -118,7 +118,7 @@ static int check_format_funcs(struct vkms_crtc_state
> *crtc_state,
> u32 n_active_planes = crtc_state->num_active_planes;
>
> for (size_t i = 0; i < n_active_planes; i++)
> - if (!planes[i]->plane_read)
> + if (!planes[i]->pixel_read)
> return -1;
>
> if (active_wb && !active_wb->wb_write)
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> index 4a248567efb2..d7ad813d7ae7 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.h
> +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> @@ -56,8 +56,7 @@ struct vkms_writeback_job { struct vkms_plane_state {
> struct drm_shadow_plane_state base;
> struct vkms_frame_info *frame_info;
> - void (*plane_read)(struct line_buffer *buffer,
> -const struct vkms_frame_info *frame_info, int y);
> + void (*pixel_read)(u16 *src_buffer, struct pixel_argb_u16 *out_pixels,
> int x);
> };
>
> struct vkms_plane {
> @@ -155,6 +154,7 @@ int vkms_verify_crc_source(struct drm_crtc *crtc, const
> char *source_name,
> /* Composer Support */
> void vkms_composer_worker(struct work_struct *work);
> void vkms_set_composer(struct vkms_output *out, bool enabled);
> +void vkms_compose_row(struct line_buffer *stage_buffer, struct
> vkms_plane_state *plane, int y);
>
> /* Writeback */
> int vkms_enable_writeback_connector(struct vkms_device *vkmsdev);
> diff --git a/drivers/gpu/drm/vkms/vkms_formats.c
> b/drivers/gpu/drm/vkms/vkms_formats.c
> index d4950688b3f1..02b0fc5a0839 100644
> --- a/drivers/gpu/drm/vkms/vkms_formats.c
> +++ b/drivers/gpu/drm/vkms/vkms_formats.c
> @@ -42,100 +42,82 @@ static void *get_packed_src_addr(const struct
> vkms_frame_info *frame_info, int y
> return packed_pixels_addr(frame_info, x_src, y_src);
> }
>
> -static void ARGB_to_argb_u16(struct line_buffer *stage_buffer,
> - const struct vkms_frame_info *frame_info, int
> y)
> +static void ARGB_to_argb_u16(u16 *src_pixels, struct pixel_argb_u16
> *out_pixels, int x)
Do you need to pass the x coordinate? You don't change it on those
functions. By not passing it, the conversion function would be even more
isolated.
> {
> - struct pixel_argb_u16 *out_pixels = stage_buffer->pixels;
> - u8 *src_pixels = get_packed_src_addr(frame_info, y);
> - int x_limit = min_t(size_t, drm_rect_width(&frame_info->dst),
> - stage_buffer->n_pixels);
> -
> - for (size_t x = 0; x < x_limit; x++, src_pixels += 4) {
> - /*
> - * The 257 is the "conversion ratio". This number is obtained
> by the
> - * (2^16 - 1) / (2^8 - 1) division. Which, in this case, tries
> to get
> - * the best color value in a pixel format with more
> possibilities.
> - * A similar idea applies to others RGB color conversions.
> - */
> - out_pixels[x].a = (u16)src_pixels[3] * 257;
> - out_pixels[x].r = (u16)src_pixels[2] * 257;
> - out_pixels[x].g = (u16)src_pixels[1] * 257;
> - out_pixels[x].b = (u16)src_pixels[0] * 257;
> - }
> + u8 *pixels = (u8 *)src_pixels;
> +
> + /*
> + * The 257 is the "conversion ratio". This number is obtained by the
> + * (2^16 - 1) / (2^8 - 1) division. Which, in this case, tries to get
> + * the best color value in a pixel format with more possibilities.
> + * A similar idea applies to others RGB color conversions.
> + */
> + out_pixels[x].a = (u16)pixels[3] * 257
Re: [PATCH v2 1/5] drm/tests: Test drm_rect_intersect()
On 03/04/23 12:33, Maíra Canal wrote:
> Hi Arthur,
>
> On 3/27/23 10:38, Arthur Grillo wrote:
>> Insert test for the drm_rect_intersect() function, it also create a
>> helper for comparing drm_rects more easily.
>>
>> Signed-off-by: Arthur Grillo
>> ---
>> drivers/gpu/drm/tests/drm_rect_test.c | 139 ++
>> 1 file changed, 139 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/tests/drm_rect_test.c
>> b/drivers/gpu/drm/tests/drm_rect_test.c
>> index e9809ea32696..3654c0be3d6b 100644
>> --- a/drivers/gpu/drm/tests/drm_rect_test.c
>> +++ b/drivers/gpu/drm/tests/drm_rect_test.c
>> @@ -9,6 +9,17 @@
>> #include
>> +#include
>
> Is this include really needed? I was able to compile without it.
>
>> +
>> +static void drm_rect_compare(struct kunit *test, const struct drm_rect *r,
>> + const struct drm_rect *expected)
>> +{
>> +KUNIT_EXPECT_EQ(test, r->x1, expected->x1);
>
> Maybe it would be nice to have a message here that shows the current x1
> and the expected x1. Same for the other dimensions.
>
Doesn't KUnit already output this information when the values don't
match?
>> +KUNIT_EXPECT_EQ(test, r->y1, expected->y1);
>> +KUNIT_EXPECT_EQ(test, drm_rect_width(r), drm_rect_width(expected));
>> +KUNIT_EXPECT_EQ(test, drm_rect_height(r), drm_rect_height(expected));
>> +}
>> +
>> static void drm_test_rect_clip_scaled_div_by_zero(struct kunit *test)
>> {
>> struct drm_rect src, dst, clip;
>> @@ -196,11 +207,139 @@ static void
>> drm_test_rect_clip_scaled_signed_vs_unsigned(struct kunit *test)
>> KUNIT_EXPECT_FALSE_MSG(test, drm_rect_visible(&src), "Source should
>> not be visible\n");
>> }
>> +struct drm_rect_intersect_case {
>> +const char *description;
>> +struct drm_rect r1, r2;
>> +bool should_be_visible;
>> +struct drm_rect expected_intersection;
>> +};
>> +
>> +static const struct drm_rect_intersect_case drm_rect_intersect_cases[] = {
>> +{
>> +.description = "top-left X bottom-right",
>> +.r1 = DRM_RECT_INIT(1, 1, 2, 2),
>> +.r2 = DRM_RECT_INIT(0, 0, 2, 2),
>> +.should_be_visible = true,
>> +.expected_intersection = DRM_RECT_INIT(1, 1, 1, 1),
>> +},
>> +{
>> +.description = "top-right X bottom-left",
>> +.r1 = DRM_RECT_INIT(0, 0, 2, 2),
>> +.r2 = DRM_RECT_INIT(1, -1, 2, 2),
>> +.should_be_visible = true,
>> +.expected_intersection = DRM_RECT_INIT(1, 0, 1, 1),
>> +},
>> +{
>> +.description = "bottom-left X top-right",
>> +.r1 = DRM_RECT_INIT(1, -1, 2, 2),
>> +.r2 = DRM_RECT_INIT(0, 0, 2, 2),
>> +.should_be_visible = true,
>> +.expected_intersection = DRM_RECT_INIT(1, 0, 1, 1),
>> +},
>> +{
>> +.description = "bottom-right X top-left",
>> +.r1 = DRM_RECT_INIT(0, 0, 2, 2),
>> +.r2 = DRM_RECT_INIT(1, 1, 2, 2),
>> +.should_be_visible = true,
>> +.expected_intersection = DRM_RECT_INIT(1, 1, 1, 1),
>> +},
>> +{
>> +.description = "right X left",
>> +.r1 = DRM_RECT_INIT(0, 0, 2, 1),
>> +.r2 = DRM_RECT_INIT(1, 0, 3, 1),
>> +.should_be_visible = true,
>> +.expected_intersection = DRM_RECT_INIT(1, 0, 1, 1),
>> +},
>> +{
>> +.description = "left X right",
>> +.r1 = DRM_RECT_INIT(1, 0, 3, 1),
>> +.r2 = DRM_RECT_INIT(0, 0, 2, 1),
>> +.should_be_visible = true,
>> +.expected_intersection = DRM_RECT_INIT(1, 0, 1, 1),
>> +},
>> +{
>> +.description = "up X bottom",
>> +.r1 = DRM_RECT_INIT(0, 0, 1, 2),
>> +.r2 = DRM_RECT_INIT(0, -1, 1, 3),
>> +.should_be_visible = true,
>> +.expected_intersection = DRM_RECT_INIT(0, 0, 1, 2),
>> +},
>> +{
>> +.description = "bottom X up",
>> +.r1 = DRM_RECT_INIT(0, -1, 1, 3),
>> +.r2 = DRM_RECT_INIT(0, 0, 1, 2),
>> +.should_be_visible = true,
>> +.expected_intersection = DRM_RECT_INIT(0, 0, 1, 2),
>> +},
>> +{
>> +.description = "touching corner",
>> +.r1 = DRM_RECT_INIT(0, 0, 1, 1),
>> +.r2 = DRM_RECT_INIT(1, 1, 2, 2),
>> +.should_be_visible = false,
>> +.expected_intersection = DRM_RECT_INIT(1, 1, 0, 0),
>> +},
>> +{
>> +.description = "touching side",
>> +.r1 = DRM_RECT_INIT(0, 0, 1, 1),
>> +.r2 = DRM_RECT_INIT(1, 0, 1, 1),
>> +.should_be_visible = false,
>> +.expected_intersection = DRM_RECT_INIT(1, 0, 0, 1),
>> +},
>> +{
>> +.description = "equal rects",
>> +.r1 = DRM_RECT_INIT(0, 0, 2, 2),
>> +.r2 = DRM_RECT_INIT(0, 0, 2, 2),
>> +.should_be_visible = true,
>> +.expected_intersection = DRM_RECT_INIT(0, 0, 2, 2),
>> +},
>> +{
>> +.description = "inside another",
>> +.r1 = DRM_RECT_INIT(0, 0, 2, 2),
>> +.r2 = DRM_RECT_INIT(1, 1, 1, 1),
>>
Re: [PATCH 1/5] drm/tests: Test drm_rect_intersect()
On 22/03/23 11:26, Ville Syrjälä wrote:
> On Wed, Mar 22, 2023 at 11:06:57AM -0300, Arthur Grillo wrote:
>> Insert test for the drm_rect_intersect() function, it also create a
>> helper for comparing drm_rects more easily.
>>
>> Signed-off-by: Arthur Grillo
>> ---
>> drivers/gpu/drm/tests/drm_rect_test.c | 30 +++
>> 1 file changed, 30 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/tests/drm_rect_test.c
>> b/drivers/gpu/drm/tests/drm_rect_test.c
>> index e9809ea32696..f700984f70a8 100644
>> --- a/drivers/gpu/drm/tests/drm_rect_test.c
>> +++ b/drivers/gpu/drm/tests/drm_rect_test.c
>> @@ -9,6 +9,15 @@
>>
>> #include
>>
>> +static void drm_rect_compare(struct kunit *test, const struct drm_rect *r,
>> + const struct drm_rect *expected)
>> +{
>> +KUNIT_EXPECT_EQ(test, r->x1, expected->x1);
>> +KUNIT_EXPECT_EQ(test, r->y1, expected->y1);
>> +KUNIT_EXPECT_EQ(test, drm_rect_width(r), drm_rect_width(expected));
>> +KUNIT_EXPECT_EQ(test, drm_rect_height(r), drm_rect_height(expected));
>> +}
>
> We already have a drm_rect_equals().
>
I knew about drm_rect_equals(), but it only returns a boolean value, I
thought that it would be better to test each attribute independently
to find errors easily in the tested functions.
>> +
>> static void drm_test_rect_clip_scaled_div_by_zero(struct kunit *test)
>> {
>> struct drm_rect src, dst, clip;
>> @@ -196,11 +205,32 @@ static void
>> drm_test_rect_clip_scaled_signed_vs_unsigned(struct kunit *test)
>> KUNIT_EXPECT_FALSE_MSG(test, drm_rect_visible(&src), "Source should not
>> be visible\n");
>> }
>>
>> +static void drm_test_rect_intersect(struct kunit *test)
>> +{
>> +struct drm_rect r1, r2;
>> +bool visible;
>> +
>> +drm_rect_init(&r1, 0, 0, 2, 2);
>> +drm_rect_init(&r2, 1, 1, 2, 2);
>> +
>> +visible = drm_rect_intersect(&r1, &r2);
>> +
>> +KUNIT_EXPECT_TRUE_MSG(test, visible, "Interserction should be a visible
>> rect");
>> +drm_rect_compare(test, &r1, &DRM_RECT_INIT(1, 1, 1, 1));
>> +
>> +drm_rect_init(&r1, 0, 0, 1, 1);
>> +
>
> I would re-init r2 here too to make it easier to see what we're
> actually testing.
Great idea.
>
> I would probably test a few more variations of the overlap between
> the rectangles, eg: equal rectangles, one smaller one fully inside
> the bigger one, overlaps across different edges/corners.
>
Okay, I will make more tests for this function.
>> +visible = drm_rect_intersect(&r1, &r2);
>> +
>> +KUNIT_EXPECT_FALSE_MSG(test, visible, "Interserction should not be a
>> visible rect");
>> +}
>> +
>> static struct kunit_case drm_rect_tests[] = {
>> KUNIT_CASE(drm_test_rect_clip_scaled_div_by_zero),
>> KUNIT_CASE(drm_test_rect_clip_scaled_not_clipped),
>> KUNIT_CASE(drm_test_rect_clip_scaled_clipped),
>> KUNIT_CASE(drm_test_rect_clip_scaled_signed_vs_unsigned),
>> +KUNIT_CASE(drm_test_rect_intersect),
>> { }
>> };
>>
>> --
>> 2.39.2
>
Thank you for your review!
Regards,
~Arthur Grillo
Re: [PATCH v3 1/2] drm/format-helper: Add Kunit tests for drm_fb_xrgb8888_to_mono()
Hello Javier, On 14/03/23 16:08, Javier Martinez Canillas wrote: > Arthur Grillo writes: > > Hello Arthur, > >> Extend the existing test cases to test the conversion from XRGB to >> monochromatic. >> >> Signed-off-by: Arthur Grillo >> --- > > Patch looks good to me: > > Reviewed-by: Javier Martinez Canillas > > Please let me know if you need someone to push this to the drm-misc tree. > It would be nice if you did, as I don't have commit rights on this tree.
Re: [PATCH v2] drm/format-helper: Make conversion_buf_size() support sub-byte pixel fmts
Hi,
On 07/03/23 18:50, Javier Martinez Canillas wrote:
> There are DRM fourcc formats that have pixels smaller than a byte, but the
> conversion_buf_size() function assumes that pixels are a multiple of bytes
> and use the struct drm_format_info .cpp field to calculate the dst_pitch.
>
> Instead, calculate it by using the bits per pixel (bpp) and divide it by 8
> to account for formats that have sub-byte pixels.
>
> Signed-off-by: Javier Martinez Canillas
> ---
> Tested by making sure that the following command still succeeds:
>
> ./tools/testing/kunit/kunit.py run \
> --kunitconfig=drivers/gpu/drm/tests/.kunitconfig
>
> Changes in v2:
> - Drop an unused variable, that was pointed out by the kernel robot.
>
> drivers/gpu/drm/tests/drm_format_helper_test.c | 7 +--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/tests/drm_format_helper_test.c
> b/drivers/gpu/drm/tests/drm_format_helper_test.c
> index 9536829c6e3a..84b5cc29c8fc 100644
> --- a/drivers/gpu/drm/tests/drm_format_helper_test.c
> +++ b/drivers/gpu/drm/tests/drm_format_helper_test.c
> @@ -409,12 +409,15 @@ static size_t conversion_buf_size(u32 dst_format,
> unsigned int dst_pitch,
> const struct drm_rect *clip)
> {
> const struct drm_format_info *dst_fi = drm_format_info(dst_format);
> + unsigned int bpp;
>
> if (!dst_fi)
> return -EINVAL;
>
> - if (!dst_pitch)
> - dst_pitch = drm_rect_width(clip) * dst_fi->cpp[0];
> + if (!dst_pitch) {
> + bpp = drm_format_info_bpp(dst_fi, 0);
> + dst_pitch = DIV_ROUND_UP(drm_rect_width(clip) * bpp, 8);
> + }
>
> return dst_pitch * drm_rect_height(clip);
> }
Ran it on UML, arm and powerpc with my patch above it, All looks good
:).
Reviewed-by: Arthur Grillo
Thanks,
Grillo
Re: [PATCH] drm/format_helper: Add Kunit tests for drm_fb_xrgb8888_to_mono()
On 02/03/23 17:01, Arthur Grillo wrote:
> Extend the existing test cases to test the conversion from XRGB to
> monochromatic.
>
> Signed-off-by: Arthur Grillo
> ---
> .../gpu/drm/tests/drm_format_helper_test.c| 73 +++
> 1 file changed, 73 insertions(+)
>
> diff --git a/drivers/gpu/drm/tests/drm_format_helper_test.c
> b/drivers/gpu/drm/tests/drm_format_helper_test.c
> index 9536829c6e3a..0610341e7349 100644
> --- a/drivers/gpu/drm/tests/drm_format_helper_test.c
> +++ b/drivers/gpu/drm/tests/drm_format_helper_test.c
> @@ -67,6 +67,11 @@ struct convert_to_argb2101010_result {
> const u32 expected[TEST_BUF_SIZE];
> };
>
> +struct convert_to_mono_result {
> + unsigned int dst_pitch;
> + const u8 expected[TEST_BUF_SIZE];
> +};
> +
> struct convert_xrgb_case {
> const char *name;
> unsigned int pitch;
> @@ -82,6 +87,7 @@ struct convert_xrgb_case {
> struct convert_to_argb_result argb_result;
> struct convert_to_xrgb2101010_result xrgb2101010_result;
> struct convert_to_argb2101010_result argb2101010_result;
> + struct convert_to_mono_result mono_result;
> };
>
> static struct convert_xrgb_case convert_xrgb_cases[] = {
> @@ -131,6 +137,10 @@ static struct convert_xrgb_case
> convert_xrgb_cases[] = {
> .dst_pitch = 0,
> .expected = { 0xFFF0 },
> },
> + .mono_result = {
> + .dst_pitch = 0,
> + .expected = { 0x00 },
> + },
> },
> {
> .name = "single_pixel_clip_rectangle",
> @@ -181,6 +191,10 @@ static struct convert_xrgb_case
> convert_xrgb_cases[] = {
> .dst_pitch = 0,
> .expected = { 0xFFF0 },
> },
> + .mono_result = {
> + .dst_pitch = 0,
> + .expected = { 0x00 },
> + },
> },
> {
> /* Well known colors: White, black, red, green, blue, magenta,
> @@ -293,6 +307,15 @@ static struct convert_xrgb_case
> convert_xrgb_cases[] = {
> 0xFC00, 0xC00F,
> },
> },
> + .mono_result = {
> + .dst_pitch = 0,
> + .expected = {
> + 0x01,
> + 0x02,
> + 0x00,
> + 0x03,
> + },
> + },
> },
> {
> /* Randomly picked colors. Full buffer within the clip area. */
> @@ -392,6 +415,14 @@ static struct convert_xrgb_case
> convert_xrgb_cases[] = {
> 0xEA20300C, 0xDB1705CD, 0xC3844672, 0x,
> 0x,
> },
> },
> + .mono_result = {
> + .dst_pitch = 0,
> + .expected = {
> + 0x00,
> + 0x00,
> + 0x00,
> + },
> + },
Now I notice that this test is not so useful for this conversion.
Maybe I will change the colors of the test to stop the expected
array from being just zeros on the v2.
> },
> };
>
> @@ -419,6 +450,17 @@ static size_t conversion_buf_size(u32 dst_format,
> unsigned int dst_pitch,
> return dst_pitch * drm_rect_height(clip);
> }
>
> +static size_t conversion_buf_size_mono(unsigned int dst_pitch, const struct
> drm_rect *clip)
> +{
> + if (!dst_pitch) {
> + unsigned int linepixels = drm_rect_width(clip) * 1;
> +
> + dst_pitch = DIV_ROUND_UP(linepixels, 8);
> + }
> +
> + return dst_pitch * drm_rect_height(clip);
> +}
> +
> static u16 *le16buf_to_cpu(struct kunit *test, const __le16 *buf, size_t
> buf_size)
> {
> u16 *dst = NULL;
> @@ -789,6 +831,36 @@ static void drm_test_fb_xrgb_to_argb2101010(struct
> kunit *test)
> KUNIT_EXPECT_MEMEQ(test, buf, result->expected, dst_size);
> }
>
> +static void drm_test_fb_xrgb_to_mono(struct kunit *test)
> +{
> + const struct convert_xrgb_case *params = test->param_value;
> + const struct convert_to_mono_result *result = ¶ms->mono_result;
> + size_t dst_size;
> + u8 *buf = NULL;
> + __le32 *xrgb = NULL;
> + struct iosys_map dst, src;
> +
> + struct drm_framebuffer fb = {
> + .format = drm_format_info(DRM_FORMAT_XRGB),
> + .pitches = { params->pitch, 0, 0 },
> + };
> +
> + dst_size = conversion_buf_size_mono(result->dst_pitch, ¶ms->clip);
> +
> + KUNIT_ASSERT_GT(test, dst_size, 0);
> +
> + buf = kunit_kzalloc(test, dst_size, GFP_KERNEL);
> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, buf);
> + iosys_map_set_vaddr(&dst, buf);
> +
> + xrgb = cpubuf_to_le32(test, params
Re: [PATCH] drm/format_helper: Add Kunit tests for drm_fb_xrgb8888_to_mono()
On 07/03/23 18:55, Javier Martinez Canillas wrote:
> Javier Martinez Canillas writes:
>
> [...]
>
>>
>>> +static size_t conversion_buf_size_mono(unsigned int dst_pitch, const
>>> struct drm_rect *clip)
>>> +{
>>> + if (!dst_pitch) {
>>> + unsigned int linepixels = drm_rect_width(clip) * 1;
>>> +
>>> + dst_pitch = DIV_ROUND_UP(linepixels, 8);
>>> + }
>>> +
>>> + return dst_pitch * drm_rect_height(clip);
>>> +}
>>> +
>>
>> I don't think you need a new helper only for this. There are other
>> formats that have sub-byte pixels, so you may want to instead make
>> the existing conversion_buf_size() function more general.
>>
>> Could you please base on the following patch that I just posted?
>>
>> https://lists.freedesktop.org/archives/dri-devel/2023-March/394466.html
>>
>
> I've posted a v2 since the kernel robot found a build warning on v1:
>
> https://lists.freedesktop.org/archives/dri-devel/2023-March/394473.html
>
> This time I have also tested your patch rebased on top of my v2, and
> your tests are passing too:
>
> [22:46:16] == drm_test_fb_xrgb_to_mono ===
> [22:46:16] [PASSED] single_pixel_source_buffer
>
> [22:46:16] [PASSED] single_pixel_clip_rectangle
> [22:46:16] [PASSED] well_known_colors
>
> [22:46:16] [PASSED] destination_pitch
>
> The version of your patch I that tested is the following:
>
> From def1b2c04c812d53ebcda17c2d34d16f311ad406 Mon Sep 17 00:00:00 2001
> From: Arthur Grillo
> Date: Thu, 2 Mar 2023 17:01:31 -0300
> Subject: [PATCH] drm/format_helper: Add Kunit tests for
> drm_fb_xrgb_to_mono()
>
> Extend the existing test cases to test the conversion from XRGB to
> monochromatic.
>
> Signed-off-by: Arthur Grillo
> Reviewed-by: Javier Martinez Canillas
> ---
> .../gpu/drm/tests/drm_format_helper_test.c| 63 +++
> 1 file changed, 63 insertions(+)
>
> diff --git a/drivers/gpu/drm/tests/drm_format_helper_test.c
> b/drivers/gpu/drm/tests/drm_format_helper_test.c
> index 84b5cc29c8fc..c9cd8a7741ee 100644
> --- a/drivers/gpu/drm/tests/drm_format_helper_test.c
> +++ b/drivers/gpu/drm/tests/drm_format_helper_test.c
> @@ -67,6 +67,11 @@ struct convert_to_argb2101010_result {
> const u32 expected[TEST_BUF_SIZE];
> };
>
> +struct convert_to_mono_result {
> + unsigned int dst_pitch;
> + const u8 expected[TEST_BUF_SIZE];
> +};
> +
> struct convert_xrgb_case {
> const char *name;
> unsigned int pitch;
> @@ -82,6 +87,7 @@ struct convert_xrgb_case {
> struct convert_to_argb_result argb_result;
> struct convert_to_xrgb2101010_result xrgb2101010_result;
> struct convert_to_argb2101010_result argb2101010_result;
> + struct convert_to_mono_result mono_result;
> };
>
> static struct convert_xrgb_case convert_xrgb_cases[] = {
> @@ -131,6 +137,10 @@ static struct convert_xrgb_case
> convert_xrgb_cases[] = {
> .dst_pitch = 0,
> .expected = { 0xFFF0 },
> },
> + .mono_result = {
> + .dst_pitch = 0,
> + .expected = { 0x00 },
> + },
> },
> {
> .name = "single_pixel_clip_rectangle",
> @@ -181,6 +191,10 @@ static struct convert_xrgb_case
> convert_xrgb_cases[] = {
> .dst_pitch = 0,
> .expected = { 0xFFF0 },
> },
> + .mono_result = {
> + .dst_pitch = 0,
> + .expected = { 0x00 },
> + },
> },
> {
> /* Well known colors: White, black, red, green, blue, magenta,
> @@ -293,6 +307,15 @@ static struct convert_xrgb_case
> convert_xrgb_cases[] = {
> 0xFC00, 0xC00F,
> },
> },
> + .mono_result = {
> + .dst_pitch = 0,
> + .expected = {
> + 0x01,
> + 0x02,
> + 0x00,
> + 0x03,
> + },
> + },
> },
> {
> /* Randomly picked colors. Full buffer within the clip area. */
> @@ -392,6 +415,14 @@ static struct convert_xrgb_case
> convert_xrgb_cases[] = {
> 0xEA20300C, 0xDB1705CD, 0xC3844672, 0x,
> 0x,
> },
> },
> + .mono_result = {
> + .dst_pitch = 0,
> + .expected = {
> + 0x00,
> + 0x00,
> + 0x00,
> + },
> + },
> },
> };
>
