[igt-dev] [PATCH i-g-t, v2, 1/2] tests/kms_cursor_crc.c: Improve test_cursor_alpha()

[Wentland, Harry]

On 2019-01-31 11:27 a.m., Mamta Shukla wrote:
> Thank you for the feedback.
> On Thu, Jan 31, 2019 at 6:19 PM Rodrigo Siqueira
> <rodrigosiqueiramelo at gmail.com> wrote:
>>
>> Hi,
>>
>> Thanks for your patch! :)
>>
>> The patch looks good for me.
>>
>> I just have one question: why did you split the patch into two parts?
>> IMHO this series can be a single patch.
>>
> The second patch in the series includes addition of igt_remove_fb() to remove
> framebuffer initialized earlier using igt_create_fb(). As this deals
> with a separate
> subtest i.e cursor-size-change. That's why created a separate patch.
> 
> Patch-[1] https://patchwork.freedesktop.org/patch/281891/
> 
> Should I include the name of the subtest as well in commit message to
> make it  more clear?
> 

I think it's fine like this, although more detail usually doesn't hurt.

Either way, both patches are
Reviewed-by: Harry Wentland <harry.wentland at amd.com>

Harry

>> Best Regards
>> Rodrigo Siqueira
>>
>> On 01/30, Mamta Shukla wrote:
>>> In the kms_cursor_crc, the subtest to check exterme alpha values failed due
>>> to different CRCs observed in hardware and software rendering. This patch fixes
>>> CRC mismatch error in cursor-alpha-opaque and cursor-alpha-transparent subtests
>>> by improving the order in which cursor was rendered in hardware test. Changes
>>> made in test_cursor_alpha() to ensure cursor rendering in hardware test and
>>> acquire CRC value:
>>> - Add drmModeSetCursor() to set cursor image.
>>> - Remove igt_display_commit() to reduce the chance of getting crc values cleared
>>>   before it is acquired.
>>> - Add igt_remove_fb() to remove locally created framebuffer after disabling the
>>>   cursor plane in HW test
>>>
>>>   Also, aligned this function with test_cursor_size and after these changes
>>>   got passing results for alpha blending support added in VKMS CRC API.
>>>
>>> Signed-off-by: Mamta Shukla <mamtashukla555 at gmail.com>
>>> ---
>>> changes in v2:
>>> -Modify commit message
>>> -Add spaces around '='
>>>
>>>  tests/kms_cursor_crc.c | 15 ++++++++-------
>>>  1 file changed, 8 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/tests/kms_cursor_crc.c b/tests/kms_cursor_crc.c
>>> index 3c9856d9..144b44b0 100644
>>> --- a/tests/kms_cursor_crc.c
>>> +++ b/tests/kms_cursor_crc.c
>>> @@ -411,26 +411,28 @@ static void test_cursor_alpha(data_t *data, double a)
>>>       igt_crc_t crc, ref_crc;
>>>       cairo_t *cr;
>>>       uint32_t fb_id;
>>> -     int curw=data->curw;
>>> -     int curh=data->curh;
>>> +     int curw = data->curw;
>>> +     int curh = data->curh;
>>> +     int ret;
>>>
>>>       /*alpha cursor fb*/
>>> -     fb_id = igt_create_color_fb(data->drm_fd, curw, curh,
>>> +     fb_id = igt_create_fb(data->drm_fd, curw, curh,
>>>                                   DRM_FORMAT_ARGB8888,
>>>                                   LOCAL_DRM_FORMAT_MOD_NONE,
>>> -                                 1.0, 1.0, 1.0,
>>>                                   &data->fb);
>>>       igt_assert(fb_id);
>>>       cr = igt_get_cairo_ctx(data->drm_fd, &data->fb);
>>> -     draw_cursor(cr, 0, 0, curw, curh, a);
>>> +     igt_paint_color_alpha(cr, 0, 0, curw, curh, 1.0, 1.0, 1.0, a);
>>>       igt_put_cairo_ctx(data->drm_fd, &data->fb, cr);
>>>
>>>       /*Hardware Test*/
>>>       cursor_enable(data);
>>> -     igt_display_commit(display);
>>> +     ret = drmModeSetCursor(data->drm_fd, data->output->config.crtc->crtc_id, data->fb.gem_handle, curw, curh);
>>> +     igt_assert_eq(ret, 0);
>>>       igt_wait_for_vblank(data->drm_fd, data->pipe);
>>>       igt_pipe_crc_collect_crc(pipe_crc, &crc);
>>>       cursor_disable(data);
>>> +     igt_remove_fb(data->drm_fd, &data->fb);
>>>
>>>       /*Software Test*/
>>>       cr = igt_get_cairo_ctx(data->drm_fd, &data->primary_fb);
>>> @@ -447,7 +449,6 @@ static void test_cursor_alpha(data_t *data, double a)
>>>       igt_paint_color(cr, 0, 0, data->screenw, data->screenh,
>>>                       0.0, 0.0, 0.0);
>>>       igt_put_cairo_ctx(data->drm_fd, &data->primary_fb, cr);
>>> -     igt_remove_fb(data->drm_fd, &data->fb);
>>>  }
>>>
>>>  static void test_cursor_transparent(data_t *data)
>>> --
>>> 2.17.1
>>>
>>
>> --
>> Rodrigo Siqueira
>> https://siqueira.tech
>> Graduate Student
>> Department of Computer Science
>> University of São Paulo
> 
> 
>