Re: [igt-dev] [PATCH i-g-t] [i-g-t] tests/kms_plane_scaling: Change minimum fb height for YUV 420 planar

[Jani Nikula <jani.nikula@linux.intel.com>]

On Fri, 16 Mar 2018, "Srinivas, Vidya" <vidya.srinivas@intel.com> wrote:
>> -----Original Message-----
>> From: Jani Nikula [mailto:jani.nikula@linux.intel.com]
>> Sent: Thursday, March 15, 2018 4:21 PM
>> To: Srinivas, Vidya <vidya.srinivas@intel.com>; igt-dev@lists.freedesktop.org
>> Cc: Kamath, Sunil <sunil.kamath@intel.com>; Syrjala, Ville
>> <ville.syrjala@intel.com>
>> Subject: Re: [igt-dev] [PATCH i-g-t] [i-g-t] tests/kms_plane_scaling: Change
>> minimum fb height for YUV 420 planar
>> 
>> On Thu, 15 Mar 2018, Vidya Srinivas <vidya.srinivas@intel.com> wrote:
>> > When the plane scaling is used with YUV 420 planar formats, the height
>> > should be a minimum of 16 scanlines as per BSpec.
>> > Minimum of 8 scanlines is for non-YUV 420 planar formats.
>> > Patch changes the minimum fb height being used in the test in case of
>> > YUV 420 planar (currently done for NV12, as that is the only planar
>> > format supported)
>> >
>> > v2: Addressed review comments from Maarten
>> 
>> This doesn't really say anything meaningful. In the future, please state the
>> actual changes.
>> 
>> > Signed-off-by: Uma Shankar <uma.shankar@intel.com>
>> > Signed-off-by: Vidya Srinivas <vidya.srinivas@intel.com>
>> > ---
>> >  tests/kms_plane_scaling.c | 2 ++
>> >  1 file changed, 2 insertions(+)
>> >
>> > diff --git a/tests/kms_plane_scaling.c b/tests/kms_plane_scaling.c
>> > index 4408e5c..36fcfc0 100644
>> > --- a/tests/kms_plane_scaling.c
>> > +++ b/tests/kms_plane_scaling.c
>> > @@ -131,6 +131,8 @@ static void check_scaling_pipe_plane_rot(data_t
>> > *d, igt_plane_t *plane,
>> >
>> >  	/* create buffer in the range of  min and max source side limit.*/
>> >  	width = height = 9;
>> > +	if (pixel_format == DRM_FORMAT_NV12)
>> > +		width = height = 17;
>> 
>> IMO it's easier to read when you have an if-else instead of overwriting the
>> previous values in some cases.
>> 
>
> Sorry about that. I see the patch is merged already.
> When I push any other patch, will address this issue.

Hey, no apologies necessary. Maarten already did the important part of
reviewing the functionality, this was just me opinioning on style.

BR,
Jani.

>
> Regards
> Vidya
>
>> BR,
>> Jani.
>> 
>> >  	igt_create_color_fb(display->drm_fd, width, height,
>> >  		       pixel_format, tiling, 0.0, 1.0, 0.0, &d->fb[0]);
>> >  	igt_plane_set_fb(plane, &d->fb[0]);
>> 
>> --
>> Jani Nikula, Intel Open Source Technology Center

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev