Hi Maarten,

Yeah that would be much simpler I think.

Damien also looked at this patch over my shoulder and had additional 
comments.
So more or less proxying :

- Not sure why we're exposing the new enums and the 
igt_atomic_popuplate_* macros in igt_kms.h.
   After all we're not using them anywhere outside igt_kms.c.

- We have a couple of properties ids stored in igt_kms.h 
(background_property & rotation_property), it would make sense to get 
rid of those and use the new created arrays instead.

Cheers,

-
Lionel

On 08/03/16 15:56, Maarten Lankhorst wrote:
> Op 07-03-16 om 13:50 schreef Lionel Landwerlin:
>> Hi Pratik,
>>
>> I'm really looking forward to get this merged.
>> Just a few comments on the plane commit part below.
> Would the following diff to the patch satisfy you? It would clean up things a lot.
>
> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> index 1f738e14f52f..454ff7552b4a 100644
> --- a/lib/igt_kms.c
> +++ b/lib/igt_kms.c
> @@ -1534,14 +1534,6 @@ igt_atomic_prepare_plane_commit(igt_plane_t *plane, igt_output_t *output,
>   
>   	igt_display_t *display = output->display;
>   	uint32_t fb_id, crtc_id;
> -	uint32_t src_x;
> -	uint32_t src_y;
> -	uint32_t src_w;
> -	uint32_t src_h;
> -	int32_t crtc_x;
> -	int32_t crtc_y;
> -	uint32_t crtc_w;
> -	uint32_t crtc_h;
>   
>   	igt_assert(plane->drm_plane);
>   
> @@ -1552,54 +1544,30 @@ igt_atomic_prepare_plane_commit(igt_plane_t *plane, igt_output_t *output,
>   	fb_id = igt_plane_get_fb_id(plane);
>   	crtc_id = output->config.crtc->crtc_id;
>   
> -	if ((plane->fb_changed || plane->size_changed) && fb_id == 0) {
> -
> -		LOG(display,
> -		    "%s: populating plane data: pipe %s, plane %d, disabling\n",
> -		     igt_output_name(output),
> -		     kmstest_pipe_name(output->config.pipe),
> -		     plane->index);
> -
> -		/* populate plane req */
> -		igt_atomic_populate_plane_req(req, plane, IGT_PLANE_CRTC_ID, 0);
> -		igt_atomic_populate_plane_req(req, plane, IGT_PLANE_FB_ID, 0);
> -
> -		igt_atomic_populate_plane_req(req, plane, IGT_PLANE_SRC_X, IGT_FIXED(0, 0));
> -		igt_atomic_populate_plane_req(req, plane, IGT_PLANE_SRC_Y, IGT_FIXED(0, 0));
> -		igt_atomic_populate_plane_req(req, plane, IGT_PLANE_SRC_W, IGT_FIXED(0, 0));
> -		igt_atomic_populate_plane_req(req, plane, IGT_PLANE_SRC_H, IGT_FIXED(0, 0));
> -
> -		igt_atomic_populate_plane_req(req, plane, IGT_PLANE_CRTC_X, 0);
> -		igt_atomic_populate_plane_req(req, plane, IGT_PLANE_CRTC_Y, 0);
> -		igt_atomic_populate_plane_req(req, plane, IGT_PLANE_CRTC_W, 0);
> -		igt_atomic_populate_plane_req(req, plane, IGT_PLANE_CRTC_H, 0);
> -
> -	} else if (plane->fb_changed || plane->position_changed ||
> -			plane->size_changed) {
> -
> -		src_x = IGT_FIXED(plane->fb->src_x, 0); /* src_x */
> -		src_y = IGT_FIXED(plane->fb->src_y, 0); /* src_y */
> -		src_w = IGT_FIXED(plane->fb->src_w, 0); /* src_w */
> -		src_h = IGT_FIXED(plane->fb->src_h, 0); /* src_h */
> -		crtc_x = plane->crtc_x;
> -		crtc_y = plane->crtc_y;
> -		crtc_w = plane->crtc_w;
> -		crtc_h = plane->crtc_h;
> -
> -		LOG(display,
> -		    "%s: populating plane data: %s.%d, fb %u, src = (%d, %d) "
> -		    "%ux%u dst = (%u, %u) %ux%u\n",
> -		    igt_output_name(output),
> -		    kmstest_pipe_name(output->config.pipe),
> -		    plane->index,
> -		    fb_id,
> -		    src_x >> 16, src_y >> 16, src_w >> 16, src_h >> 16,
> -		    crtc_x, crtc_y, crtc_w, crtc_h);
> -
> -
> -		/* populate plane req */
> -		igt_atomic_populate_plane_req(req, plane, IGT_PLANE_CRTC_ID, crtc_id);
> +	LOG(display,
> +	    "%s: populating plane data: %s.%d, fb %u, src = (%d, %d) "
> +	    "%ux%u dst = (%u, %u) %ux%u\n",
> +	    igt_output_name(output),
> +	    kmstest_pipe_name(output->config.pipe),
> +	    plane->index,
> +	    fb_id,
> +	    src_x >> 16, src_y >> 16, src_w >> 16, src_h >> 16,
> +	    crtc_x, crtc_y, crtc_w, crtc_h);
> +
> +	if (plane->fb_changed) {
> +		igt_atomic_populate_plane_req(req, plane, IGT_PLANE_CRTC_ID, fb_id ? crtc_id : 0);
>   		igt_atomic_populate_plane_req(req, plane, IGT_PLANE_FB_ID, fb_id);
> +	}
> +
> +	if (plane->position_changed || plane->size_changed) {
> +		uint32_t src_x = IGT_FIXED(plane->fb->src_x, 0); /* src_x */
> +		uint32_t src_y = IGT_FIXED(plane->fb->src_y, 0); /* src_y */
> +		uint32_t src_w = IGT_FIXED(plane->fb->src_w, 0); /* src_w */
> +		uint32_t src_h = IGT_FIXED(plane->fb->src_h, 0); /* src_h */
> +		int32_t crtc_x = plane->crtc_x;
> +		int32_t crtc_y = plane->crtc_y;
> +		uint32_t crtc_w = plane->crtc_w;
> +		uint32_t crtc_h = plane->crtc_h;
>   
>   		igt_atomic_populate_plane_req(req, plane, IGT_PLANE_SRC_X, src_x);
>   		igt_atomic_populate_plane_req(req, plane, IGT_PLANE_SRC_Y, src_y);
> @@ -1610,18 +1578,15 @@ igt_atomic_prepare_plane_commit(igt_plane_t *plane, igt_output_t *output,
>   		igt_atomic_populate_plane_req(req, plane, IGT_PLANE_CRTC_Y, crtc_y);
>   		igt_atomic_populate_plane_req(req, plane, IGT_PLANE_CRTC_W, crtc_w);
>   		igt_atomic_populate_plane_req(req, plane, IGT_PLANE_CRTC_H, crtc_h);
> -
> -		if (plane->rotation_changed)
> -			igt_atomic_populate_plane_req(req, plane,
> -				IGT_PLANE_ROTATION, plane->rotation);
> -
>   	}
>   
> +	if (plane->rotation_changed)
> +		igt_atomic_populate_plane_req(req, plane,
> +			IGT_PLANE_ROTATION, plane->rotation);
> +
>   	plane->fb_changed = false;
>   	plane->position_changed = false;
>   	plane->size_changed = false;

I missed that, but I think we need rotation_changed = false; here too.

> -
> -	return;
>   }
>   
>   
>