Re: [igt-dev] [PATCH i-g-t] tests/kms_plane_alpha_blend: Paint FBs with cairo source operator

["Kazlauskas, Nicholas" <Nicholas.Kazlauskas@amd.com>]

On 2/22/19 1:57 PM, Ville Syrjälä wrote:
> On Fri, Feb 22, 2019 at 05:46:08PM +0000, Kazlauskas, Nicholas wrote:
>> On 2/22/19 12:39 PM, Ville Syrjälä wrote:
>>> On Fri, Feb 22, 2019 at 12:08:46PM -0500, Nicholas Kazlauskas wrote:
>>>> Cairo defaults to using a blended fill, so when IGT tries to paint on
>>>> top of a framebuffer in these tests with an alpha less than 1.0 the
>>>> previous framebuffer contents will still be present underneath.
>>>>
>>>> On amdgpu this is an issue because the framebuffer is created in
>>>> an unitialized state and can have contents from previous tests
>>>> still within memory.
>>>
>>> igt_fb more or less assumes all fbs start out black. Sounds like you
>>> need a memset() or equivalent somewhere.
>>
>> The buffer could also be painted to 0 implicitly using cairo. We also
>> need a patch at some point to not generate the suspicious CRC read
>> warning since our black screens are (0, 0, 0) and we rely on tests
>> leaving garbage in the framebuffer to avoid it.
> 
> I guess just 'if (is_i915) check_suspicious_crc()' should help.
> Though I'm not 100% sure we can't get those on some i915 platforms too.

Another solution here would just be to modify the CRC in amdgpu before 
returning it to userspace. Like XOR the value with some constant. Though 
that would be a little less elegant since there could still be the 
chance we get a CRC value back with that exact constant.

> 
>>
>> However, this patch would still be needed since the fill will still be
>> blended by default. When creating transparent or semi-transparent
>> buffers they should be filled in with the cairo source operator or the
>> alpha channel won't be set to the correct value.
> 
> AFAICS if the dest alpha and color are both 0.0 the only fb that is
> wrong is argb_fb_0 since the current code will in fact give us
> alpha of 1.0. draw_squares() with OVER should still give us the
> same results as we'd get with SOURCE.
> 
> I guess this means any test which is currently using argb_fb_0
> is definitely not testing per-pixel alpha.
> 
> So argb_transparant() and basic_alpha() are simply useless atm?
> 
> constant_alpha_min() sets pixel_blend_mode to None anyway so
> doesn't really matter what we have in the alpha channel.
> 
> Not sure why those tests aren't doing the gray_fb thing the other tests
> are using...
> 
> Anyways, no point in blending when you don't have to so
> the patch seems to make sense to me either way:
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Thanks for the review. Like you said, the main problem is with argb_fb_0 
and argb_transparent and basic_alpha but it's better to be explicit 
about intent with the square filling too IMO.

Nicholas Kazlauskas

> 
>>
>> Nicholas Kazlauskas
>>
>>>
>>>>
>>>> This patch explicitly sets the cairo filling operator to SOURCE to
>>>> overwrite the contents when performing fills with alpha < 1.0 in
>>>> this test.
>>>>
>>>> It also fixes the creation of the transparent FB. My guess as for why
>>>> this test previously passed on i915 is because the primary FB was
>>>> also black, so the CRC would match even if the black overlay plane
>>>> was partially transparent.
>>>>
>>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>> Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
>>>> ---
>>>>    tests/kms_plane_alpha_blend.c | 4 +++-
>>>>    1 file changed, 3 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/tests/kms_plane_alpha_blend.c b/tests/kms_plane_alpha_blend.c
>>>> index 1d9d8933..d7dc44ab 100644
>>>> --- a/tests/kms_plane_alpha_blend.c
>>>> +++ b/tests/kms_plane_alpha_blend.c
>>>> @@ -83,6 +83,7 @@ static void draw_squares(struct igt_fb *fb, int w, int h, double a)
>>>>    {
>>>>    	cairo_t *cr = igt_get_cairo_ctx(fb->fd, fb);
>>>>    
>>>> +	cairo_set_operator(cr, CAIRO_OPERATOR_SOURCE);
>>>>    	igt_paint_color_alpha(cr, 0, 0,         w / 2, h / 2, 1., 0., 0., a);
>>>>    	igt_paint_color_alpha(cr, w / 2, 0,     w / 2, h / 2, 0., 1., 0., a);
>>>>    	igt_paint_color_alpha(cr, 0, h / 2,     w / 2, h / 2, 0., 0., 1., a);
>>>> @@ -197,7 +198,8 @@ static void prepare_crtc(data_t *data, igt_output_t *output, enum pipe pipe)
>>>>    			      &data->argb_fb_0);
>>>>    
>>>>    		cr = igt_get_cairo_ctx(data->gfx_fd, &data->argb_fb_0);
>>>> -		igt_paint_color_alpha(cr, 0, 0, w, h, 0., 0., 0., 1.0);
>>>> +		cairo_set_operator(cr, CAIRO_OPERATOR_SOURCE);
>>>> +		igt_paint_color_alpha(cr, 0, 0, w, h, 0., 0., 0., 0.0);
>>>>    		igt_put_cairo_ctx(data->gfx_fd, &data->argb_fb_0, cr);
>>>>    
>>>>    		igt_create_fb(data->gfx_fd, w, h,
>>>> -- 
>>>> 2.17.1
>>>>
>>>> _______________________________________________
>>>> igt-dev mailing list
>>>> igt-dev@lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/igt-dev
>>>
>>
> 

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev