diff mbox series

[RESEND,v6,2/9] drm: vkms: Rename `vkms_composer` to `vkms_frame_info`

Message ID 20220819182411.20246-3-igormtorrente@gmail.com (mailing list archive)
State New, archived
Headers show
Series Add new formats support to vkms | expand

Commit Message

Igor Matheus Andrade Torrente Aug. 19, 2022, 6:24 p.m. UTC 
Changes the name of this struct to a more meaningful name.
A name that represents better what this struct is about.

Composer is the code that do the compositing of the planes.
This struct contains information on the frame used in the output
composition. Thus, vkms_frame_info is a better name to represent
this.

V5: Fix a commit message typo(Melissa Wen).

Reviewed-by: Melissa Wen <mwen@igalia.com>
Signed-off-by: Igor Torrente <igormtorrente@gmail.com>
---
 drivers/gpu/drm/vkms/vkms_composer.c | 87 ++++++++++++++--------------
 drivers/gpu/drm/vkms/vkms_drv.h      |  6 +-
 drivers/gpu/drm/vkms/vkms_plane.c    | 38 ++++++------
 3 files changed, 66 insertions(+), 65 deletions(-)

Comments

Melissa Wen Aug. 20, 2022, 11 a.m. UTC | #1 
On 08/19, Igor Torrente wrote:
> Changes the name of this struct to a more meaningful name.
> A name that represents better what this struct is about.
> 
> Composer is the code that do the compositing of the planes.
> This struct contains information on the frame used in the output
> composition. Thus, vkms_frame_info is a better name to represent
> this.
> 
> V5: Fix a commit message typo(Melissa Wen).
> 
> Reviewed-by: Melissa Wen <mwen@igalia.com>
> Signed-off-by: Igor Torrente <igormtorrente@gmail.com>
> ---
>  drivers/gpu/drm/vkms/vkms_composer.c | 87 ++++++++++++++--------------
>  drivers/gpu/drm/vkms/vkms_drv.h      |  6 +-
>  drivers/gpu/drm/vkms/vkms_plane.c    | 38 ++++++------
>  3 files changed, 66 insertions(+), 65 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
> index 775b97766e08..0aded4e87e60 100644
> --- a/drivers/gpu/drm/vkms/vkms_composer.c
> +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> @@ -11,11 +11,11 @@
>  #include "vkms_drv.h"
>  
>  static u32 get_pixel_from_buffer(int x, int y, const u8 *buffer,
> -				 const struct vkms_composer *composer)
> +				 const struct vkms_frame_info *frame_info)
>  {
>  	u32 pixel;
> -	int src_offset = composer->offset + (y * composer->pitch)
> -				      + (x * composer->cpp);
> +	int src_offset = frame_info->offset + (y * frame_info->pitch)
> +					    + (x * frame_info->cpp);
>  
>  	pixel = *(u32 *)&buffer[src_offset];
>  
> @@ -26,24 +26,24 @@ static u32 get_pixel_from_buffer(int x, int y, const u8 *buffer,
>   * compute_crc - Compute CRC value on output frame
>   *
>   * @vaddr: address to final framebuffer
> - * @composer: framebuffer's metadata
> + * @frame_info: framebuffer's metadata
>   *
>   * returns CRC value computed using crc32 on the visible portion of
>   * the final framebuffer at vaddr_out
>   */
>  static uint32_t compute_crc(const u8 *vaddr,
> -			    const struct vkms_composer *composer)
> +			    const struct vkms_frame_info *frame_info)
>  {
>  	int x, y;
>  	u32 crc = 0, pixel = 0;
> -	int x_src = composer->src.x1 >> 16;
> -	int y_src = composer->src.y1 >> 16;
> -	int h_src = drm_rect_height(&composer->src) >> 16;
> -	int w_src = drm_rect_width(&composer->src) >> 16;
> +	int x_src = frame_info->src.x1 >> 16;
> +	int y_src = frame_info->src.y1 >> 16;
> +	int h_src = drm_rect_height(&frame_info->src) >> 16;
> +	int w_src = drm_rect_width(&frame_info->src) >> 16;
>  
>  	for (y = y_src; y < y_src + h_src; ++y) {
>  		for (x = x_src; x < x_src + w_src; ++x) {
> -			pixel = get_pixel_from_buffer(x, y, vaddr, composer);
> +			pixel = get_pixel_from_buffer(x, y, vaddr, frame_info);
>  			crc = crc32_le(crc, (void *)&pixel, sizeof(u32));
>  		}
>  	}
> @@ -98,8 +98,8 @@ static void x_blend(const u8 *xrgb_src, u8 *xrgb_dst)
>   * blend - blend value at vaddr_src with value at vaddr_dst
>   * @vaddr_dst: destination address
>   * @vaddr_src: source address
> - * @dst_composer: destination framebuffer's metadata
> - * @src_composer: source framebuffer's metadata
> + * @dst_frame_info: destination framebuffer's metadata
> + * @src_frame_info: source framebuffer's metadata
>   * @pixel_blend: blending equation based on plane format
>   *
>   * Blend the vaddr_src value with the vaddr_dst value using a pixel blend
> @@ -111,33 +111,33 @@ static void x_blend(const u8 *xrgb_src, u8 *xrgb_dst)
>   * pixel color values
>   */
>  static void blend(void *vaddr_dst, void *vaddr_src,
> -		  struct vkms_composer *dst_composer,
> -		  struct vkms_composer *src_composer,
> +		  struct vkms_frame_info *dst_frame_info,
> +		  struct vkms_frame_info *src_frame_info,
>  		  void (*pixel_blend)(const u8 *, u8 *))
>  {
>  	int i, j, j_dst, i_dst;
>  	int offset_src, offset_dst;
>  	u8 *pixel_dst, *pixel_src;
>  
> -	int x_src = src_composer->src.x1 >> 16;
> -	int y_src = src_composer->src.y1 >> 16;
> +	int x_src = src_frame_info->src.x1 >> 16;
> +	int y_src = src_frame_info->src.y1 >> 16;
>  
> -	int x_dst = src_composer->dst.x1;
> -	int y_dst = src_composer->dst.y1;
> -	int h_dst = drm_rect_height(&src_composer->dst);
> -	int w_dst = drm_rect_width(&src_composer->dst);
> +	int x_dst = src_frame_info->dst.x1;
> +	int y_dst = src_frame_info->dst.y1;
> +	int h_dst = drm_rect_height(&src_frame_info->dst);
> +	int w_dst = drm_rect_width(&src_frame_info->dst);
>  
>  	int y_limit = y_src + h_dst;
>  	int x_limit = x_src + w_dst;
>  
>  	for (i = y_src, i_dst = y_dst; i < y_limit; ++i) {
>  		for (j = x_src, j_dst = x_dst; j < x_limit; ++j) {
> -			offset_dst = dst_composer->offset
> -				     + (i_dst * dst_composer->pitch)
> -				     + (j_dst++ * dst_composer->cpp);
> -			offset_src = src_composer->offset
> -				     + (i * src_composer->pitch)
> -				     + (j * src_composer->cpp);
> +			offset_dst = dst_frame_info->offset
> +				     + (i_dst * dst_frame_info->pitch)
> +				     + (j_dst++ * dst_frame_info->cpp);
> +			offset_src = src_frame_info->offset
> +				     + (i * src_frame_info->pitch)
> +				     + (j * src_frame_info->cpp);
>  
>  			pixel_src = (u8 *)(vaddr_src + offset_src);
>  			pixel_dst = (u8 *)(vaddr_dst + offset_dst);
> @@ -149,32 +149,33 @@ static void blend(void *vaddr_dst, void *vaddr_src,
>  	}
>  }
>  
> -static void compose_plane(struct vkms_composer *primary_composer,
> -			  struct vkms_composer *plane_composer,
> +static void compose_plane(struct vkms_frame_info *primary_plane_info,
> +			  struct vkms_frame_info *plane_frame_info,
>  			  void *vaddr_out)
>  {
> -	struct drm_framebuffer *fb = &plane_composer->fb;
> +	struct drm_framebuffer *fb = &plane_frame_info->fb;
>  	void *vaddr;
>  	void (*pixel_blend)(const u8 *p_src, u8 *p_dst);
>  
> -	if (WARN_ON(iosys_map_is_null(&plane_composer->map[0])))
> +	if (WARN_ON(iosys_map_is_null(&primary_plane_info->map[0])))
^ here you are reintroducing an error that we were checking primary
plane repeatedly, instead of plane_composer (renamed to:
plane_frame_info here). The issue is fixed in a following patch of this
series when you decouple check_iosys_map.
But I don't mind fixing it before apply.

>  		return;
>  
> -	vaddr = plane_composer->map[0].vaddr;
> +	vaddr = plane_frame_info->map[0].vaddr;
>  
>  	if (fb->format->format == DRM_FORMAT_ARGB8888)
>  		pixel_blend = &alpha_blend;
>  	else
>  		pixel_blend = &x_blend;
>  
> -	blend(vaddr_out, vaddr, primary_composer, plane_composer, pixel_blend);
> +	blend(vaddr_out, vaddr, primary_plane_info,
> +	      plane_frame_info, pixel_blend);
>  }
>  
>  static int compose_active_planes(void **vaddr_out,
> -				 struct vkms_composer *primary_composer,
> +				 struct vkms_frame_info *primary_plane_info,
>  				 struct vkms_crtc_state *crtc_state)
>  {
> -	struct drm_framebuffer *fb = &primary_composer->fb;
> +	struct drm_framebuffer *fb = &primary_plane_info->fb;
>  	struct drm_gem_object *gem_obj = drm_gem_fb_get_obj(fb, 0);
>  	const void *vaddr;
>  	int i;
> @@ -187,10 +188,10 @@ static int compose_active_planes(void **vaddr_out,
>  		}
>  	}
>  
> -	if (WARN_ON(iosys_map_is_null(&primary_composer->map[0])))
> +	if (WARN_ON(iosys_map_is_null(&primary_plane_info->map[0])))
>  		return -EINVAL;
>  
> -	vaddr = primary_composer->map[0].vaddr;
> +	vaddr = primary_plane_info->map[0].vaddr;
>  
>  	memcpy(*vaddr_out, vaddr, gem_obj->size);
>  
> @@ -199,8 +200,8 @@ static int compose_active_planes(void **vaddr_out,
>  	 * ((primary <- overlay) <- cursor)
>  	 */
>  	for (i = 1; i < crtc_state->num_active_planes; i++)
> -		compose_plane(primary_composer,
> -			      crtc_state->active_planes[i]->composer,
> +		compose_plane(primary_plane_info,
> +			      crtc_state->active_planes[i]->frame_info,
>  			      *vaddr_out);
>  
>  	return 0;
> @@ -222,7 +223,7 @@ void vkms_composer_worker(struct work_struct *work)
>  						composer_work);
>  	struct drm_crtc *crtc = crtc_state->base.crtc;
>  	struct vkms_output *out = drm_crtc_to_vkms_output(crtc);
> -	struct vkms_composer *primary_composer = NULL;
> +	struct vkms_frame_info *primary_plane_info = NULL;
>  	struct vkms_plane_state *act_plane = NULL;
>  	bool crc_pending, wb_pending;
>  	void *vaddr_out = NULL;
> @@ -250,16 +251,16 @@ void vkms_composer_worker(struct work_struct *work)
>  	if (crtc_state->num_active_planes >= 1) {
>  		act_plane = crtc_state->active_planes[0];
>  		if (act_plane->base.base.plane->type == DRM_PLANE_TYPE_PRIMARY)
> -			primary_composer = act_plane->composer;
> +			primary_plane_info = act_plane->frame_info;
>  	}
>  
> -	if (!primary_composer)
> +	if (!primary_plane_info)
>  		return;
>  
>  	if (wb_pending)
>  		vaddr_out = crtc_state->active_writeback->data[0].vaddr;
>  
> -	ret = compose_active_planes(&vaddr_out, primary_composer,
> +	ret = compose_active_planes(&vaddr_out, primary_plane_info,
>  				    crtc_state);
>  	if (ret) {
>  		if (ret == -EINVAL && !wb_pending)
> @@ -267,7 +268,7 @@ void vkms_composer_worker(struct work_struct *work)
>  		return;
>  	}
>  
> -	crc32 = compute_crc(vaddr_out, primary_composer);
> +	crc32 = compute_crc(vaddr_out, primary_plane_info);
>  
>  	if (wb_pending) {
>  		drm_writeback_signal_completion(&out->wb_connector, 0);
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> index 36fbab5989d1..5199c5f18e17 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.h
> +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> @@ -27,7 +27,7 @@ struct vkms_writeback_job {
>  	struct iosys_map data[DRM_FORMAT_MAX_PLANES];
>  };
>  
> -struct vkms_composer {
> +struct vkms_frame_info {
>  	struct drm_framebuffer fb;
>  	struct drm_rect src, dst;
>  	struct iosys_map map[DRM_FORMAT_MAX_PLANES];
> @@ -39,11 +39,11 @@ struct vkms_composer {
>  /**
>   * vkms_plane_state - Driver specific plane state
>   * @base: base plane state
> - * @composer: data required for composing computation
> + * @frame_info: data required for composing computation
>   */
>  struct vkms_plane_state {
>  	struct drm_shadow_plane_state base;
> -	struct vkms_composer *composer;
> +	struct vkms_frame_info *frame_info;
>  };
>  
>  struct vkms_plane {
> diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
> index d8eb674b49a6..fcae6c508f4b 100644
> --- a/drivers/gpu/drm/vkms/vkms_plane.c
> +++ b/drivers/gpu/drm/vkms/vkms_plane.c
> @@ -24,20 +24,20 @@ static struct drm_plane_state *
>  vkms_plane_duplicate_state(struct drm_plane *plane)
>  {
>  	struct vkms_plane_state *vkms_state;
> -	struct vkms_composer *composer;
> +	struct vkms_frame_info *frame_info;
>  
>  	vkms_state = kzalloc(sizeof(*vkms_state), GFP_KERNEL);
>  	if (!vkms_state)
>  		return NULL;
>  
> -	composer = kzalloc(sizeof(*composer), GFP_KERNEL);
> -	if (!composer) {
> -		DRM_DEBUG_KMS("Couldn't allocate composer\n");
> +	frame_info = kzalloc(sizeof(*frame_info), GFP_KERNEL);
> +	if (!frame_info) {
> +		DRM_DEBUG_KMS("Couldn't allocate frame_info\n");
>  		kfree(vkms_state);
>  		return NULL;
>  	}
>  
> -	vkms_state->composer = composer;
> +	vkms_state->frame_info = frame_info;
>  
>  	__drm_gem_duplicate_shadow_plane_state(plane, &vkms_state->base);
>  
> @@ -54,12 +54,12 @@ static void vkms_plane_destroy_state(struct drm_plane *plane,
>  		/* dropping the reference we acquired in
>  		 * vkms_primary_plane_update()
>  		 */
> -		if (drm_framebuffer_read_refcount(&vkms_state->composer->fb))
> -			drm_framebuffer_put(&vkms_state->composer->fb);
> +		if (drm_framebuffer_read_refcount(&vkms_state->frame_info->fb))
> +			drm_framebuffer_put(&vkms_state->frame_info->fb);
>  	}
>  
> -	kfree(vkms_state->composer);
> -	vkms_state->composer = NULL;
> +	kfree(vkms_state->frame_info);
> +	vkms_state->frame_info = NULL;
>  
>  	__drm_gem_destroy_shadow_plane_state(&vkms_state->base);
>  	kfree(vkms_state);
> @@ -99,7 +99,7 @@ static void vkms_plane_atomic_update(struct drm_plane *plane,
>  	struct vkms_plane_state *vkms_plane_state;
>  	struct drm_shadow_plane_state *shadow_plane_state;
>  	struct drm_framebuffer *fb = new_state->fb;
> -	struct vkms_composer *composer;
> +	struct vkms_frame_info *frame_info;
>  
>  	if (!new_state->crtc || !fb)
>  		return;
> @@ -107,15 +107,15 @@ static void vkms_plane_atomic_update(struct drm_plane *plane,
>  	vkms_plane_state = to_vkms_plane_state(new_state);
>  	shadow_plane_state = &vkms_plane_state->base;
>  
> -	composer = vkms_plane_state->composer;
> -	memcpy(&composer->src, &new_state->src, sizeof(struct drm_rect));
> -	memcpy(&composer->dst, &new_state->dst, sizeof(struct drm_rect));
> -	memcpy(&composer->fb, fb, sizeof(struct drm_framebuffer));
> -	memcpy(&composer->map, &shadow_plane_state->data, sizeof(composer->map));
> -	drm_framebuffer_get(&composer->fb);
> -	composer->offset = fb->offsets[0];
> -	composer->pitch = fb->pitches[0];
> -	composer->cpp = fb->format->cpp[0];
> +	frame_info = vkms_plane_state->frame_info;
> +	memcpy(&frame_info->src, &new_state->src, sizeof(struct drm_rect));
> +	memcpy(&frame_info->dst, &new_state->dst, sizeof(struct drm_rect));
> +	memcpy(&frame_info->fb, fb, sizeof(struct drm_framebuffer));
> +	memcpy(&frame_info->map, &shadow_plane_state->data, sizeof(frame_info->map));
> +	drm_framebuffer_get(&frame_info->fb);
> +	frame_info->offset = fb->offsets[0];
> +	frame_info->pitch = fb->pitches[0];
> +	frame_info->cpp = fb->format->cpp[0];
>  }
>  
>  static int vkms_plane_atomic_check(struct drm_plane *plane,
> -- 
> 2.30.2
>
Igor Matheus Andrade Torrente Aug. 22, 2022, 6:29 p.m. UTC | #2 
Hi Mellisa,

On 8/20/22 08:00, Melissa Wen wrote:
> On 08/19, Igor Torrente wrote:
>> Changes the name of this struct to a more meaningful name.
>> A name that represents better what this struct is about.
>>
>> Composer is the code that do the compositing of the planes.
>> This struct contains information on the frame used in the output
>> composition. Thus, vkms_frame_info is a better name to represent
>> this.
>>
>> V5: Fix a commit message typo(Melissa Wen).
>>
>> Reviewed-by: Melissa Wen <mwen@igalia.com>
>> Signed-off-by: Igor Torrente <igormtorrente@gmail.com>
>> ---
>>   drivers/gpu/drm/vkms/vkms_composer.c | 87 ++++++++++++++--------------
>>   drivers/gpu/drm/vkms/vkms_drv.h      |  6 +-
>>   drivers/gpu/drm/vkms/vkms_plane.c    | 38 ++++++------
>>   3 files changed, 66 insertions(+), 65 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
>> index 775b97766e08..0aded4e87e60 100644
>> --- a/drivers/gpu/drm/vkms/vkms_composer.c
>> +++ b/drivers/gpu/drm/vkms/vkms_composer.c
>> @@ -11,11 +11,11 @@
>>   #include "vkms_drv.h"
>>   
>>   static u32 get_pixel_from_buffer(int x, int y, const u8 *buffer,
>> -				 const struct vkms_composer *composer)
>> +				 const struct vkms_frame_info *frame_info)
>>   {
>>   	u32 pixel;
>> -	int src_offset = composer->offset + (y * composer->pitch)
>> -				      + (x * composer->cpp);
>> +	int src_offset = frame_info->offset + (y * frame_info->pitch)
>> +					    + (x * frame_info->cpp);
>>   
>>   	pixel = *(u32 *)&buffer[src_offset];
>>   
>> @@ -26,24 +26,24 @@ static u32 get_pixel_from_buffer(int x, int y, const u8 *buffer,
>>    * compute_crc - Compute CRC value on output frame
>>    *
>>    * @vaddr: address to final framebuffer
>> - * @composer: framebuffer's metadata
>> + * @frame_info: framebuffer's metadata
>>    *
>>    * returns CRC value computed using crc32 on the visible portion of
>>    * the final framebuffer at vaddr_out
>>    */
>>   static uint32_t compute_crc(const u8 *vaddr,
>> -			    const struct vkms_composer *composer)
>> +			    const struct vkms_frame_info *frame_info)
>>   {
>>   	int x, y;
>>   	u32 crc = 0, pixel = 0;
>> -	int x_src = composer->src.x1 >> 16;
>> -	int y_src = composer->src.y1 >> 16;
>> -	int h_src = drm_rect_height(&composer->src) >> 16;
>> -	int w_src = drm_rect_width(&composer->src) >> 16;
>> +	int x_src = frame_info->src.x1 >> 16;
>> +	int y_src = frame_info->src.y1 >> 16;
>> +	int h_src = drm_rect_height(&frame_info->src) >> 16;
>> +	int w_src = drm_rect_width(&frame_info->src) >> 16;
>>   
>>   	for (y = y_src; y < y_src + h_src; ++y) {
>>   		for (x = x_src; x < x_src + w_src; ++x) {
>> -			pixel = get_pixel_from_buffer(x, y, vaddr, composer);
>> +			pixel = get_pixel_from_buffer(x, y, vaddr, frame_info);
>>   			crc = crc32_le(crc, (void *)&pixel, sizeof(u32));
>>   		}
>>   	}
>> @@ -98,8 +98,8 @@ static void x_blend(const u8 *xrgb_src, u8 *xrgb_dst)
>>    * blend - blend value at vaddr_src with value at vaddr_dst
>>    * @vaddr_dst: destination address
>>    * @vaddr_src: source address
>> - * @dst_composer: destination framebuffer's metadata
>> - * @src_composer: source framebuffer's metadata
>> + * @dst_frame_info: destination framebuffer's metadata
>> + * @src_frame_info: source framebuffer's metadata
>>    * @pixel_blend: blending equation based on plane format
>>    *
>>    * Blend the vaddr_src value with the vaddr_dst value using a pixel blend
>> @@ -111,33 +111,33 @@ static void x_blend(const u8 *xrgb_src, u8 *xrgb_dst)
>>    * pixel color values
>>    */
>>   static void blend(void *vaddr_dst, void *vaddr_src,
>> -		  struct vkms_composer *dst_composer,
>> -		  struct vkms_composer *src_composer,
>> +		  struct vkms_frame_info *dst_frame_info,
>> +		  struct vkms_frame_info *src_frame_info,
>>   		  void (*pixel_blend)(const u8 *, u8 *))
>>   {
>>   	int i, j, j_dst, i_dst;
>>   	int offset_src, offset_dst;
>>   	u8 *pixel_dst, *pixel_src;
>>   
>> -	int x_src = src_composer->src.x1 >> 16;
>> -	int y_src = src_composer->src.y1 >> 16;
>> +	int x_src = src_frame_info->src.x1 >> 16;
>> +	int y_src = src_frame_info->src.y1 >> 16;
>>   
>> -	int x_dst = src_composer->dst.x1;
>> -	int y_dst = src_composer->dst.y1;
>> -	int h_dst = drm_rect_height(&src_composer->dst);
>> -	int w_dst = drm_rect_width(&src_composer->dst);
>> +	int x_dst = src_frame_info->dst.x1;
>> +	int y_dst = src_frame_info->dst.y1;
>> +	int h_dst = drm_rect_height(&src_frame_info->dst);
>> +	int w_dst = drm_rect_width(&src_frame_info->dst);
>>   
>>   	int y_limit = y_src + h_dst;
>>   	int x_limit = x_src + w_dst;
>>   
>>   	for (i = y_src, i_dst = y_dst; i < y_limit; ++i) {
>>   		for (j = x_src, j_dst = x_dst; j < x_limit; ++j) {
>> -			offset_dst = dst_composer->offset
>> -				     + (i_dst * dst_composer->pitch)
>> -				     + (j_dst++ * dst_composer->cpp);
>> -			offset_src = src_composer->offset
>> -				     + (i * src_composer->pitch)
>> -				     + (j * src_composer->cpp);
>> +			offset_dst = dst_frame_info->offset
>> +				     + (i_dst * dst_frame_info->pitch)
>> +				     + (j_dst++ * dst_frame_info->cpp);
>> +			offset_src = src_frame_info->offset
>> +				     + (i * src_frame_info->pitch)
>> +				     + (j * src_frame_info->cpp);
>>   
>>   			pixel_src = (u8 *)(vaddr_src + offset_src);
>>   			pixel_dst = (u8 *)(vaddr_dst + offset_dst);
>> @@ -149,32 +149,33 @@ static void blend(void *vaddr_dst, void *vaddr_src,
>>   	}
>>   }
>>   
>> -static void compose_plane(struct vkms_composer *primary_composer,
>> -			  struct vkms_composer *plane_composer,
>> +static void compose_plane(struct vkms_frame_info *primary_plane_info,
>> +			  struct vkms_frame_info *plane_frame_info,
>>   			  void *vaddr_out)
>>   {
>> -	struct drm_framebuffer *fb = &plane_composer->fb;
>> +	struct drm_framebuffer *fb = &plane_frame_info->fb;
>>   	void *vaddr;
>>   	void (*pixel_blend)(const u8 *p_src, u8 *p_dst);
>>   
>> -	if (WARN_ON(iosys_map_is_null(&plane_composer->map[0])))
>> +	if (WARN_ON(iosys_map_is_null(&primary_plane_info->map[0])))
> ^ here you are reintroducing an error that we were checking primary
> plane repeatedly, instead of plane_composer (renamed to:
> plane_frame_info here). The issue is fixed in a following patch of this
> series when you decouple check_iosys_map.
> But I don't mind fixing it before apply.
Should I simply delete this line in the patch? Or there's something
else to do?

> 
>>   		return;
>>   
>> -	vaddr = plane_composer->map[0].vaddr;
>> +	vaddr = plane_frame_info->map[0].vaddr;
>>   
>>   	if (fb->format->format == DRM_FORMAT_ARGB8888)
>>   		pixel_blend = &alpha_blend;
>>   	else
>>   		pixel_blend = &x_blend;
>>   
>> -	blend(vaddr_out, vaddr, primary_composer, plane_composer, pixel_blend);
>> +	blend(vaddr_out, vaddr, primary_plane_info,
>> +	      plane_frame_info, pixel_blend);
>>   }
>>   
>>   static int compose_active_planes(void **vaddr_out,
>> -				 struct vkms_composer *primary_composer,
>> +				 struct vkms_frame_info *primary_plane_info,
>>   				 struct vkms_crtc_state *crtc_state)
>>   {
>> -	struct drm_framebuffer *fb = &primary_composer->fb;
>> +	struct drm_framebuffer *fb = &primary_plane_info->fb;
>>   	struct drm_gem_object *gem_obj = drm_gem_fb_get_obj(fb, 0);
>>   	const void *vaddr;
>>   	int i;
>> @@ -187,10 +188,10 @@ static int compose_active_planes(void **vaddr_out,
>>   		}
>>   	}
>>   
>> -	if (WARN_ON(iosys_map_is_null(&primary_composer->map[0])))
>> +	if (WARN_ON(iosys_map_is_null(&primary_plane_info->map[0])))
>>   		return -EINVAL;
>>   
>> -	vaddr = primary_composer->map[0].vaddr;
>> +	vaddr = primary_plane_info->map[0].vaddr;
>>   
>>   	memcpy(*vaddr_out, vaddr, gem_obj->size);
>>   
>> @@ -199,8 +200,8 @@ static int compose_active_planes(void **vaddr_out,
>>   	 * ((primary <- overlay) <- cursor)
>>   	 */
>>   	for (i = 1; i < crtc_state->num_active_planes; i++)
>> -		compose_plane(primary_composer,
>> -			      crtc_state->active_planes[i]->composer,
>> +		compose_plane(primary_plane_info,
>> +			      crtc_state->active_planes[i]->frame_info,
>>   			      *vaddr_out);
>>   
>>   	return 0;
>> @@ -222,7 +223,7 @@ void vkms_composer_worker(struct work_struct *work)
>>   						composer_work);
>>   	struct drm_crtc *crtc = crtc_state->base.crtc;
>>   	struct vkms_output *out = drm_crtc_to_vkms_output(crtc);
>> -	struct vkms_composer *primary_composer = NULL;
>> +	struct vkms_frame_info *primary_plane_info = NULL;
>>   	struct vkms_plane_state *act_plane = NULL;
>>   	bool crc_pending, wb_pending;
>>   	void *vaddr_out = NULL;
>> @@ -250,16 +251,16 @@ void vkms_composer_worker(struct work_struct *work)
>>   	if (crtc_state->num_active_planes >= 1) {
>>   		act_plane = crtc_state->active_planes[0];
>>   		if (act_plane->base.base.plane->type == DRM_PLANE_TYPE_PRIMARY)
>> -			primary_composer = act_plane->composer;
>> +			primary_plane_info = act_plane->frame_info;
>>   	}
>>   
>> -	if (!primary_composer)
>> +	if (!primary_plane_info)
>>   		return;
>>   
>>   	if (wb_pending)
>>   		vaddr_out = crtc_state->active_writeback->data[0].vaddr;
>>   
>> -	ret = compose_active_planes(&vaddr_out, primary_composer,
>> +	ret = compose_active_planes(&vaddr_out, primary_plane_info,
>>   				    crtc_state);
>>   	if (ret) {
>>   		if (ret == -EINVAL && !wb_pending)
>> @@ -267,7 +268,7 @@ void vkms_composer_worker(struct work_struct *work)
>>   		return;
>>   	}
>>   
>> -	crc32 = compute_crc(vaddr_out, primary_composer);
>> +	crc32 = compute_crc(vaddr_out, primary_plane_info);
>>   
>>   	if (wb_pending) {
>>   		drm_writeback_signal_completion(&out->wb_connector, 0);
>> diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
>> index 36fbab5989d1..5199c5f18e17 100644
>> --- a/drivers/gpu/drm/vkms/vkms_drv.h
>> +++ b/drivers/gpu/drm/vkms/vkms_drv.h
>> @@ -27,7 +27,7 @@ struct vkms_writeback_job {
>>   	struct iosys_map data[DRM_FORMAT_MAX_PLANES];
>>   };
>>   
>> -struct vkms_composer {
>> +struct vkms_frame_info {
>>   	struct drm_framebuffer fb;
>>   	struct drm_rect src, dst;
>>   	struct iosys_map map[DRM_FORMAT_MAX_PLANES];
>> @@ -39,11 +39,11 @@ struct vkms_composer {
>>   /**
>>    * vkms_plane_state - Driver specific plane state
>>    * @base: base plane state
>> - * @composer: data required for composing computation
>> + * @frame_info: data required for composing computation
>>    */
>>   struct vkms_plane_state {
>>   	struct drm_shadow_plane_state base;
>> -	struct vkms_composer *composer;
>> +	struct vkms_frame_info *frame_info;
>>   };
>>   
>>   struct vkms_plane {
>> diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
>> index d8eb674b49a6..fcae6c508f4b 100644
>> --- a/drivers/gpu/drm/vkms/vkms_plane.c
>> +++ b/drivers/gpu/drm/vkms/vkms_plane.c
>> @@ -24,20 +24,20 @@ static struct drm_plane_state *
>>   vkms_plane_duplicate_state(struct drm_plane *plane)
>>   {
>>   	struct vkms_plane_state *vkms_state;
>> -	struct vkms_composer *composer;
>> +	struct vkms_frame_info *frame_info;
>>   
>>   	vkms_state = kzalloc(sizeof(*vkms_state), GFP_KERNEL);
>>   	if (!vkms_state)
>>   		return NULL;
>>   
>> -	composer = kzalloc(sizeof(*composer), GFP_KERNEL);
>> -	if (!composer) {
>> -		DRM_DEBUG_KMS("Couldn't allocate composer\n");
>> +	frame_info = kzalloc(sizeof(*frame_info), GFP_KERNEL);
>> +	if (!frame_info) {
>> +		DRM_DEBUG_KMS("Couldn't allocate frame_info\n");
>>   		kfree(vkms_state);
>>   		return NULL;
>>   	}
>>   
>> -	vkms_state->composer = composer;
>> +	vkms_state->frame_info = frame_info;
>>   
>>   	__drm_gem_duplicate_shadow_plane_state(plane, &vkms_state->base);
>>   
>> @@ -54,12 +54,12 @@ static void vkms_plane_destroy_state(struct drm_plane *plane,
>>   		/* dropping the reference we acquired in
>>   		 * vkms_primary_plane_update()
>>   		 */
>> -		if (drm_framebuffer_read_refcount(&vkms_state->composer->fb))
>> -			drm_framebuffer_put(&vkms_state->composer->fb);
>> +		if (drm_framebuffer_read_refcount(&vkms_state->frame_info->fb))
>> +			drm_framebuffer_put(&vkms_state->frame_info->fb);
>>   	}
>>   
>> -	kfree(vkms_state->composer);
>> -	vkms_state->composer = NULL;
>> +	kfree(vkms_state->frame_info);
>> +	vkms_state->frame_info = NULL;
>>   
>>   	__drm_gem_destroy_shadow_plane_state(&vkms_state->base);
>>   	kfree(vkms_state);
>> @@ -99,7 +99,7 @@ static void vkms_plane_atomic_update(struct drm_plane *plane,
>>   	struct vkms_plane_state *vkms_plane_state;
>>   	struct drm_shadow_plane_state *shadow_plane_state;
>>   	struct drm_framebuffer *fb = new_state->fb;
>> -	struct vkms_composer *composer;
>> +	struct vkms_frame_info *frame_info;
>>   
>>   	if (!new_state->crtc || !fb)
>>   		return;
>> @@ -107,15 +107,15 @@ static void vkms_plane_atomic_update(struct drm_plane *plane,
>>   	vkms_plane_state = to_vkms_plane_state(new_state);
>>   	shadow_plane_state = &vkms_plane_state->base;
>>   
>> -	composer = vkms_plane_state->composer;
>> -	memcpy(&composer->src, &new_state->src, sizeof(struct drm_rect));
>> -	memcpy(&composer->dst, &new_state->dst, sizeof(struct drm_rect));
>> -	memcpy(&composer->fb, fb, sizeof(struct drm_framebuffer));
>> -	memcpy(&composer->map, &shadow_plane_state->data, sizeof(composer->map));
>> -	drm_framebuffer_get(&composer->fb);
>> -	composer->offset = fb->offsets[0];
>> -	composer->pitch = fb->pitches[0];
>> -	composer->cpp = fb->format->cpp[0];
>> +	frame_info = vkms_plane_state->frame_info;
>> +	memcpy(&frame_info->src, &new_state->src, sizeof(struct drm_rect));
>> +	memcpy(&frame_info->dst, &new_state->dst, sizeof(struct drm_rect));
>> +	memcpy(&frame_info->fb, fb, sizeof(struct drm_framebuffer));
>> +	memcpy(&frame_info->map, &shadow_plane_state->data, sizeof(frame_info->map));
>> +	drm_framebuffer_get(&frame_info->fb);
>> +	frame_info->offset = fb->offsets[0];
>> +	frame_info->pitch = fb->pitches[0];
>> +	frame_info->cpp = fb->format->cpp[0];
>>   }
>>   
>>   static int vkms_plane_atomic_check(struct drm_plane *plane,
>> -- 
>> 2.30.2
>>
Melissa Wen Aug. 22, 2022, 6:37 p.m. UTC | #3 
On 08/22, Igor Matheus Andrade Torrente wrote:
> Hi Mellisa,
> 
> On 8/20/22 08:00, Melissa Wen wrote:
> > On 08/19, Igor Torrente wrote:
> > > Changes the name of this struct to a more meaningful name.
> > > A name that represents better what this struct is about.
> > > 
> > > Composer is the code that do the compositing of the planes.
> > > This struct contains information on the frame used in the output
> > > composition. Thus, vkms_frame_info is a better name to represent
> > > this.
> > > 
> > > V5: Fix a commit message typo(Melissa Wen).
> > > 
> > > Reviewed-by: Melissa Wen <mwen@igalia.com>
> > > Signed-off-by: Igor Torrente <igormtorrente@gmail.com>
> > > ---
> > >   drivers/gpu/drm/vkms/vkms_composer.c | 87 ++++++++++++++--------------
> > >   drivers/gpu/drm/vkms/vkms_drv.h      |  6 +-
> > >   drivers/gpu/drm/vkms/vkms_plane.c    | 38 ++++++------
> > >   3 files changed, 66 insertions(+), 65 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
> > > index 775b97766e08..0aded4e87e60 100644
> > > --- a/drivers/gpu/drm/vkms/vkms_composer.c
> > > +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> > > @@ -11,11 +11,11 @@
> > >   #include "vkms_drv.h"
> > >   static u32 get_pixel_from_buffer(int x, int y, const u8 *buffer,
> > > -				 const struct vkms_composer *composer)
> > > +				 const struct vkms_frame_info *frame_info)
> > >   {
> > >   	u32 pixel;
> > > -	int src_offset = composer->offset + (y * composer->pitch)
> > > -				      + (x * composer->cpp);
> > > +	int src_offset = frame_info->offset + (y * frame_info->pitch)
> > > +					    + (x * frame_info->cpp);
> > >   	pixel = *(u32 *)&buffer[src_offset];
> > > @@ -26,24 +26,24 @@ static u32 get_pixel_from_buffer(int x, int y, const u8 *buffer,
> > >    * compute_crc - Compute CRC value on output frame
> > >    *
> > >    * @vaddr: address to final framebuffer
> > > - * @composer: framebuffer's metadata
> > > + * @frame_info: framebuffer's metadata
> > >    *
> > >    * returns CRC value computed using crc32 on the visible portion of
> > >    * the final framebuffer at vaddr_out
> > >    */
> > >   static uint32_t compute_crc(const u8 *vaddr,
> > > -			    const struct vkms_composer *composer)
> > > +			    const struct vkms_frame_info *frame_info)
> > >   {
> > >   	int x, y;
> > >   	u32 crc = 0, pixel = 0;
> > > -	int x_src = composer->src.x1 >> 16;
> > > -	int y_src = composer->src.y1 >> 16;
> > > -	int h_src = drm_rect_height(&composer->src) >> 16;
> > > -	int w_src = drm_rect_width(&composer->src) >> 16;
> > > +	int x_src = frame_info->src.x1 >> 16;
> > > +	int y_src = frame_info->src.y1 >> 16;
> > > +	int h_src = drm_rect_height(&frame_info->src) >> 16;
> > > +	int w_src = drm_rect_width(&frame_info->src) >> 16;
> > >   	for (y = y_src; y < y_src + h_src; ++y) {
> > >   		for (x = x_src; x < x_src + w_src; ++x) {
> > > -			pixel = get_pixel_from_buffer(x, y, vaddr, composer);
> > > +			pixel = get_pixel_from_buffer(x, y, vaddr, frame_info);
> > >   			crc = crc32_le(crc, (void *)&pixel, sizeof(u32));
> > >   		}
> > >   	}
> > > @@ -98,8 +98,8 @@ static void x_blend(const u8 *xrgb_src, u8 *xrgb_dst)
> > >    * blend - blend value at vaddr_src with value at vaddr_dst
> > >    * @vaddr_dst: destination address
> > >    * @vaddr_src: source address
> > > - * @dst_composer: destination framebuffer's metadata
> > > - * @src_composer: source framebuffer's metadata
> > > + * @dst_frame_info: destination framebuffer's metadata
> > > + * @src_frame_info: source framebuffer's metadata
> > >    * @pixel_blend: blending equation based on plane format
> > >    *
> > >    * Blend the vaddr_src value with the vaddr_dst value using a pixel blend
> > > @@ -111,33 +111,33 @@ static void x_blend(const u8 *xrgb_src, u8 *xrgb_dst)
> > >    * pixel color values
> > >    */
> > >   static void blend(void *vaddr_dst, void *vaddr_src,
> > > -		  struct vkms_composer *dst_composer,
> > > -		  struct vkms_composer *src_composer,
> > > +		  struct vkms_frame_info *dst_frame_info,
> > > +		  struct vkms_frame_info *src_frame_info,
> > >   		  void (*pixel_blend)(const u8 *, u8 *))
> > >   {
> > >   	int i, j, j_dst, i_dst;
> > >   	int offset_src, offset_dst;
> > >   	u8 *pixel_dst, *pixel_src;
> > > -	int x_src = src_composer->src.x1 >> 16;
> > > -	int y_src = src_composer->src.y1 >> 16;
> > > +	int x_src = src_frame_info->src.x1 >> 16;
> > > +	int y_src = src_frame_info->src.y1 >> 16;
> > > -	int x_dst = src_composer->dst.x1;
> > > -	int y_dst = src_composer->dst.y1;
> > > -	int h_dst = drm_rect_height(&src_composer->dst);
> > > -	int w_dst = drm_rect_width(&src_composer->dst);
> > > +	int x_dst = src_frame_info->dst.x1;
> > > +	int y_dst = src_frame_info->dst.y1;
> > > +	int h_dst = drm_rect_height(&src_frame_info->dst);
> > > +	int w_dst = drm_rect_width(&src_frame_info->dst);
> > >   	int y_limit = y_src + h_dst;
> > >   	int x_limit = x_src + w_dst;
> > >   	for (i = y_src, i_dst = y_dst; i < y_limit; ++i) {
> > >   		for (j = x_src, j_dst = x_dst; j < x_limit; ++j) {
> > > -			offset_dst = dst_composer->offset
> > > -				     + (i_dst * dst_composer->pitch)
> > > -				     + (j_dst++ * dst_composer->cpp);
> > > -			offset_src = src_composer->offset
> > > -				     + (i * src_composer->pitch)
> > > -				     + (j * src_composer->cpp);
> > > +			offset_dst = dst_frame_info->offset
> > > +				     + (i_dst * dst_frame_info->pitch)
> > > +				     + (j_dst++ * dst_frame_info->cpp);
> > > +			offset_src = src_frame_info->offset
> > > +				     + (i * src_frame_info->pitch)
> > > +				     + (j * src_frame_info->cpp);
> > >   			pixel_src = (u8 *)(vaddr_src + offset_src);
> > >   			pixel_dst = (u8 *)(vaddr_dst + offset_dst);
> > > @@ -149,32 +149,33 @@ static void blend(void *vaddr_dst, void *vaddr_src,
> > >   	}
> > >   }
> > > -static void compose_plane(struct vkms_composer *primary_composer,
> > > -			  struct vkms_composer *plane_composer,
> > > +static void compose_plane(struct vkms_frame_info *primary_plane_info,
> > > +			  struct vkms_frame_info *plane_frame_info,
> > >   			  void *vaddr_out)
> > >   {
> > > -	struct drm_framebuffer *fb = &plane_composer->fb;
> > > +	struct drm_framebuffer *fb = &plane_frame_info->fb;
> > >   	void *vaddr;
> > >   	void (*pixel_blend)(const u8 *p_src, u8 *p_dst);
> > > -	if (WARN_ON(iosys_map_is_null(&plane_composer->map[0])))
> > > +	if (WARN_ON(iosys_map_is_null(&primary_plane_info->map[0])))
> > ^ here you are reintroducing an error that we were checking primary
> > plane repeatedly, instead of plane_composer (renamed to:
> > plane_frame_info here). The issue is fixed in a following patch of this
> > series when you decouple check_iosys_map.
> > But I don't mind fixing it before apply.
> Should I simply delete this line in the patch? Or there's something
> else to do?

No, you just need to check the correct plane (plane_frame_info), that means:

- if (WARN_ON(iosys_map_is_null(&plane_composer->map[0])))
+ if (WARN_ON(iosys_map_is_null(&plane_frame_info->map[0])))

because here you are renaming `plane_composer` to `plane_frame_info`,
and `primary_plane_info->map[0]` is already checked in the following
compose_active_planes() function.

Thanks,

Melissa

> 
> > 
> > >   		return;
> > > -	vaddr = plane_composer->map[0].vaddr;
> > > +	vaddr = plane_frame_info->map[0].vaddr;
> > >   	if (fb->format->format == DRM_FORMAT_ARGB8888)
> > >   		pixel_blend = &alpha_blend;
> > >   	else
> > >   		pixel_blend = &x_blend;
> > > -	blend(vaddr_out, vaddr, primary_composer, plane_composer, pixel_blend);
> > > +	blend(vaddr_out, vaddr, primary_plane_info,
> > > +	      plane_frame_info, pixel_blend);
> > >   }
> > >   static int compose_active_planes(void **vaddr_out,
> > > -				 struct vkms_composer *primary_composer,
> > > +				 struct vkms_frame_info *primary_plane_info,
> > >   				 struct vkms_crtc_state *crtc_state)
> > >   {
> > > -	struct drm_framebuffer *fb = &primary_composer->fb;
> > > +	struct drm_framebuffer *fb = &primary_plane_info->fb;
> > >   	struct drm_gem_object *gem_obj = drm_gem_fb_get_obj(fb, 0);
> > >   	const void *vaddr;
> > >   	int i;
> > > @@ -187,10 +188,10 @@ static int compose_active_planes(void **vaddr_out,
> > >   		}
> > >   	}
> > > -	if (WARN_ON(iosys_map_is_null(&primary_composer->map[0])))
> > > +	if (WARN_ON(iosys_map_is_null(&primary_plane_info->map[0])))
> > >   		return -EINVAL;
> > > -	vaddr = primary_composer->map[0].vaddr;
> > > +	vaddr = primary_plane_info->map[0].vaddr;
> > >   	memcpy(*vaddr_out, vaddr, gem_obj->size);
> > > @@ -199,8 +200,8 @@ static int compose_active_planes(void **vaddr_out,
> > >   	 * ((primary <- overlay) <- cursor)
> > >   	 */
> > >   	for (i = 1; i < crtc_state->num_active_planes; i++)
> > > -		compose_plane(primary_composer,
> > > -			      crtc_state->active_planes[i]->composer,
> > > +		compose_plane(primary_plane_info,
> > > +			      crtc_state->active_planes[i]->frame_info,
> > >   			      *vaddr_out);
> > >   	return 0;
> > > @@ -222,7 +223,7 @@ void vkms_composer_worker(struct work_struct *work)
> > >   						composer_work);
> > >   	struct drm_crtc *crtc = crtc_state->base.crtc;
> > >   	struct vkms_output *out = drm_crtc_to_vkms_output(crtc);
> > > -	struct vkms_composer *primary_composer = NULL;
> > > +	struct vkms_frame_info *primary_plane_info = NULL;
> > >   	struct vkms_plane_state *act_plane = NULL;
> > >   	bool crc_pending, wb_pending;
> > >   	void *vaddr_out = NULL;
> > > @@ -250,16 +251,16 @@ void vkms_composer_worker(struct work_struct *work)
> > >   	if (crtc_state->num_active_planes >= 1) {
> > >   		act_plane = crtc_state->active_planes[0];
> > >   		if (act_plane->base.base.plane->type == DRM_PLANE_TYPE_PRIMARY)
> > > -			primary_composer = act_plane->composer;
> > > +			primary_plane_info = act_plane->frame_info;
> > >   	}
> > > -	if (!primary_composer)
> > > +	if (!primary_plane_info)
> > >   		return;
> > >   	if (wb_pending)
> > >   		vaddr_out = crtc_state->active_writeback->data[0].vaddr;
> > > -	ret = compose_active_planes(&vaddr_out, primary_composer,
> > > +	ret = compose_active_planes(&vaddr_out, primary_plane_info,
> > >   				    crtc_state);
> > >   	if (ret) {
> > >   		if (ret == -EINVAL && !wb_pending)
> > > @@ -267,7 +268,7 @@ void vkms_composer_worker(struct work_struct *work)
> > >   		return;
> > >   	}
> > > -	crc32 = compute_crc(vaddr_out, primary_composer);
> > > +	crc32 = compute_crc(vaddr_out, primary_plane_info);
> > >   	if (wb_pending) {
> > >   		drm_writeback_signal_completion(&out->wb_connector, 0);
> > > diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> > > index 36fbab5989d1..5199c5f18e17 100644
> > > --- a/drivers/gpu/drm/vkms/vkms_drv.h
> > > +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> > > @@ -27,7 +27,7 @@ struct vkms_writeback_job {
> > >   	struct iosys_map data[DRM_FORMAT_MAX_PLANES];
> > >   };
> > > -struct vkms_composer {
> > > +struct vkms_frame_info {
> > >   	struct drm_framebuffer fb;
> > >   	struct drm_rect src, dst;
> > >   	struct iosys_map map[DRM_FORMAT_MAX_PLANES];
> > > @@ -39,11 +39,11 @@ struct vkms_composer {
> > >   /**
> > >    * vkms_plane_state - Driver specific plane state
> > >    * @base: base plane state
> > > - * @composer: data required for composing computation
> > > + * @frame_info: data required for composing computation
> > >    */
> > >   struct vkms_plane_state {
> > >   	struct drm_shadow_plane_state base;
> > > -	struct vkms_composer *composer;
> > > +	struct vkms_frame_info *frame_info;
> > >   };
> > >   struct vkms_plane {
> > > diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
> > > index d8eb674b49a6..fcae6c508f4b 100644
> > > --- a/drivers/gpu/drm/vkms/vkms_plane.c
> > > +++ b/drivers/gpu/drm/vkms/vkms_plane.c
> > > @@ -24,20 +24,20 @@ static struct drm_plane_state *
> > >   vkms_plane_duplicate_state(struct drm_plane *plane)
> > >   {
> > >   	struct vkms_plane_state *vkms_state;
> > > -	struct vkms_composer *composer;
> > > +	struct vkms_frame_info *frame_info;
> > >   	vkms_state = kzalloc(sizeof(*vkms_state), GFP_KERNEL);
> > >   	if (!vkms_state)
> > >   		return NULL;
> > > -	composer = kzalloc(sizeof(*composer), GFP_KERNEL);
> > > -	if (!composer) {
> > > -		DRM_DEBUG_KMS("Couldn't allocate composer\n");
> > > +	frame_info = kzalloc(sizeof(*frame_info), GFP_KERNEL);
> > > +	if (!frame_info) {
> > > +		DRM_DEBUG_KMS("Couldn't allocate frame_info\n");
> > >   		kfree(vkms_state);
> > >   		return NULL;
> > >   	}
> > > -	vkms_state->composer = composer;
> > > +	vkms_state->frame_info = frame_info;
> > >   	__drm_gem_duplicate_shadow_plane_state(plane, &vkms_state->base);
> > > @@ -54,12 +54,12 @@ static void vkms_plane_destroy_state(struct drm_plane *plane,
> > >   		/* dropping the reference we acquired in
> > >   		 * vkms_primary_plane_update()
> > >   		 */
> > > -		if (drm_framebuffer_read_refcount(&vkms_state->composer->fb))
> > > -			drm_framebuffer_put(&vkms_state->composer->fb);
> > > +		if (drm_framebuffer_read_refcount(&vkms_state->frame_info->fb))
> > > +			drm_framebuffer_put(&vkms_state->frame_info->fb);
> > >   	}
> > > -	kfree(vkms_state->composer);
> > > -	vkms_state->composer = NULL;
> > > +	kfree(vkms_state->frame_info);
> > > +	vkms_state->frame_info = NULL;
> > >   	__drm_gem_destroy_shadow_plane_state(&vkms_state->base);
> > >   	kfree(vkms_state);
> > > @@ -99,7 +99,7 @@ static void vkms_plane_atomic_update(struct drm_plane *plane,
> > >   	struct vkms_plane_state *vkms_plane_state;
> > >   	struct drm_shadow_plane_state *shadow_plane_state;
> > >   	struct drm_framebuffer *fb = new_state->fb;
> > > -	struct vkms_composer *composer;
> > > +	struct vkms_frame_info *frame_info;
> > >   	if (!new_state->crtc || !fb)
> > >   		return;
> > > @@ -107,15 +107,15 @@ static void vkms_plane_atomic_update(struct drm_plane *plane,
> > >   	vkms_plane_state = to_vkms_plane_state(new_state);
> > >   	shadow_plane_state = &vkms_plane_state->base;
> > > -	composer = vkms_plane_state->composer;
> > > -	memcpy(&composer->src, &new_state->src, sizeof(struct drm_rect));
> > > -	memcpy(&composer->dst, &new_state->dst, sizeof(struct drm_rect));
> > > -	memcpy(&composer->fb, fb, sizeof(struct drm_framebuffer));
> > > -	memcpy(&composer->map, &shadow_plane_state->data, sizeof(composer->map));
> > > -	drm_framebuffer_get(&composer->fb);
> > > -	composer->offset = fb->offsets[0];
> > > -	composer->pitch = fb->pitches[0];
> > > -	composer->cpp = fb->format->cpp[0];
> > > +	frame_info = vkms_plane_state->frame_info;
> > > +	memcpy(&frame_info->src, &new_state->src, sizeof(struct drm_rect));
> > > +	memcpy(&frame_info->dst, &new_state->dst, sizeof(struct drm_rect));
> > > +	memcpy(&frame_info->fb, fb, sizeof(struct drm_framebuffer));
> > > +	memcpy(&frame_info->map, &shadow_plane_state->data, sizeof(frame_info->map));
> > > +	drm_framebuffer_get(&frame_info->fb);
> > > +	frame_info->offset = fb->offsets[0];
> > > +	frame_info->pitch = fb->pitches[0];
> > > +	frame_info->cpp = fb->format->cpp[0];
> > >   }
> > >   static int vkms_plane_atomic_check(struct drm_plane *plane,
> > > -- 
> > > 2.30.2
> > > 
>
Igor Matheus Andrade Torrente Aug. 22, 2022, 7:43 p.m. UTC | #4 
On 8/22/22 15:37, Melissa Wen wrote:
> On 08/22, Igor Matheus Andrade Torrente wrote:
>> Hi Mellisa,
>>
>> On 8/20/22 08:00, Melissa Wen wrote:
>>> On 08/19, Igor Torrente wrote:
>>>> Changes the name of this struct to a more meaningful name.
>>>> A name that represents better what this struct is about.
>>>>
>>>> Composer is the code that do the compositing of the planes.
>>>> This struct contains information on the frame used in the output
>>>> composition. Thus, vkms_frame_info is a better name to represent
>>>> this.
>>>>
>>>> V5: Fix a commit message typo(Melissa Wen).
>>>>
>>>> Reviewed-by: Melissa Wen <mwen@igalia.com>
>>>> Signed-off-by: Igor Torrente <igormtorrente@gmail.com>
>>>> ---
>>>>    drivers/gpu/drm/vkms/vkms_composer.c | 87 ++++++++++++++--------------
>>>>    drivers/gpu/drm/vkms/vkms_drv.h      |  6 +-
>>>>    drivers/gpu/drm/vkms/vkms_plane.c    | 38 ++++++------
>>>>    3 files changed, 66 insertions(+), 65 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
>>>> index 775b97766e08..0aded4e87e60 100644
>>>> --- a/drivers/gpu/drm/vkms/vkms_composer.c
>>>> +++ b/drivers/gpu/drm/vkms/vkms_composer.c
>>>> @@ -11,11 +11,11 @@
>>>>    #include "vkms_drv.h"
>>>>    static u32 get_pixel_from_buffer(int x, int y, const u8 *buffer,
>>>> -				 const struct vkms_composer *composer)
>>>> +				 const struct vkms_frame_info *frame_info)
>>>>    {
>>>>    	u32 pixel;
>>>> -	int src_offset = composer->offset + (y * composer->pitch)
>>>> -				      + (x * composer->cpp);
>>>> +	int src_offset = frame_info->offset + (y * frame_info->pitch)
>>>> +					    + (x * frame_info->cpp);
>>>>    	pixel = *(u32 *)&buffer[src_offset];
>>>> @@ -26,24 +26,24 @@ static u32 get_pixel_from_buffer(int x, int y, const u8 *buffer,
>>>>     * compute_crc - Compute CRC value on output frame
>>>>     *
>>>>     * @vaddr: address to final framebuffer
>>>> - * @composer: framebuffer's metadata
>>>> + * @frame_info: framebuffer's metadata
>>>>     *
>>>>     * returns CRC value computed using crc32 on the visible portion of
>>>>     * the final framebuffer at vaddr_out
>>>>     */
>>>>    static uint32_t compute_crc(const u8 *vaddr,
>>>> -			    const struct vkms_composer *composer)
>>>> +			    const struct vkms_frame_info *frame_info)
>>>>    {
>>>>    	int x, y;
>>>>    	u32 crc = 0, pixel = 0;
>>>> -	int x_src = composer->src.x1 >> 16;
>>>> -	int y_src = composer->src.y1 >> 16;
>>>> -	int h_src = drm_rect_height(&composer->src) >> 16;
>>>> -	int w_src = drm_rect_width(&composer->src) >> 16;
>>>> +	int x_src = frame_info->src.x1 >> 16;
>>>> +	int y_src = frame_info->src.y1 >> 16;
>>>> +	int h_src = drm_rect_height(&frame_info->src) >> 16;
>>>> +	int w_src = drm_rect_width(&frame_info->src) >> 16;
>>>>    	for (y = y_src; y < y_src + h_src; ++y) {
>>>>    		for (x = x_src; x < x_src + w_src; ++x) {
>>>> -			pixel = get_pixel_from_buffer(x, y, vaddr, composer);
>>>> +			pixel = get_pixel_from_buffer(x, y, vaddr, frame_info);
>>>>    			crc = crc32_le(crc, (void *)&pixel, sizeof(u32));
>>>>    		}
>>>>    	}
>>>> @@ -98,8 +98,8 @@ static void x_blend(const u8 *xrgb_src, u8 *xrgb_dst)
>>>>     * blend - blend value at vaddr_src with value at vaddr_dst
>>>>     * @vaddr_dst: destination address
>>>>     * @vaddr_src: source address
>>>> - * @dst_composer: destination framebuffer's metadata
>>>> - * @src_composer: source framebuffer's metadata
>>>> + * @dst_frame_info: destination framebuffer's metadata
>>>> + * @src_frame_info: source framebuffer's metadata
>>>>     * @pixel_blend: blending equation based on plane format
>>>>     *
>>>>     * Blend the vaddr_src value with the vaddr_dst value using a pixel blend
>>>> @@ -111,33 +111,33 @@ static void x_blend(const u8 *xrgb_src, u8 *xrgb_dst)
>>>>     * pixel color values
>>>>     */
>>>>    static void blend(void *vaddr_dst, void *vaddr_src,
>>>> -		  struct vkms_composer *dst_composer,
>>>> -		  struct vkms_composer *src_composer,
>>>> +		  struct vkms_frame_info *dst_frame_info,
>>>> +		  struct vkms_frame_info *src_frame_info,
>>>>    		  void (*pixel_blend)(const u8 *, u8 *))
>>>>    {
>>>>    	int i, j, j_dst, i_dst;
>>>>    	int offset_src, offset_dst;
>>>>    	u8 *pixel_dst, *pixel_src;
>>>> -	int x_src = src_composer->src.x1 >> 16;
>>>> -	int y_src = src_composer->src.y1 >> 16;
>>>> +	int x_src = src_frame_info->src.x1 >> 16;
>>>> +	int y_src = src_frame_info->src.y1 >> 16;
>>>> -	int x_dst = src_composer->dst.x1;
>>>> -	int y_dst = src_composer->dst.y1;
>>>> -	int h_dst = drm_rect_height(&src_composer->dst);
>>>> -	int w_dst = drm_rect_width(&src_composer->dst);
>>>> +	int x_dst = src_frame_info->dst.x1;
>>>> +	int y_dst = src_frame_info->dst.y1;
>>>> +	int h_dst = drm_rect_height(&src_frame_info->dst);
>>>> +	int w_dst = drm_rect_width(&src_frame_info->dst);
>>>>    	int y_limit = y_src + h_dst;
>>>>    	int x_limit = x_src + w_dst;
>>>>    	for (i = y_src, i_dst = y_dst; i < y_limit; ++i) {
>>>>    		for (j = x_src, j_dst = x_dst; j < x_limit; ++j) {
>>>> -			offset_dst = dst_composer->offset
>>>> -				     + (i_dst * dst_composer->pitch)
>>>> -				     + (j_dst++ * dst_composer->cpp);
>>>> -			offset_src = src_composer->offset
>>>> -				     + (i * src_composer->pitch)
>>>> -				     + (j * src_composer->cpp);
>>>> +			offset_dst = dst_frame_info->offset
>>>> +				     + (i_dst * dst_frame_info->pitch)
>>>> +				     + (j_dst++ * dst_frame_info->cpp);
>>>> +			offset_src = src_frame_info->offset
>>>> +				     + (i * src_frame_info->pitch)
>>>> +				     + (j * src_frame_info->cpp);
>>>>    			pixel_src = (u8 *)(vaddr_src + offset_src);
>>>>    			pixel_dst = (u8 *)(vaddr_dst + offset_dst);
>>>> @@ -149,32 +149,33 @@ static void blend(void *vaddr_dst, void *vaddr_src,
>>>>    	}
>>>>    }
>>>> -static void compose_plane(struct vkms_composer *primary_composer,
>>>> -			  struct vkms_composer *plane_composer,
>>>> +static void compose_plane(struct vkms_frame_info *primary_plane_info,
>>>> +			  struct vkms_frame_info *plane_frame_info,
>>>>    			  void *vaddr_out)
>>>>    {
>>>> -	struct drm_framebuffer *fb = &plane_composer->fb;
>>>> +	struct drm_framebuffer *fb = &plane_frame_info->fb;
>>>>    	void *vaddr;
>>>>    	void (*pixel_blend)(const u8 *p_src, u8 *p_dst);
>>>> -	if (WARN_ON(iosys_map_is_null(&plane_composer->map[0])))
>>>> +	if (WARN_ON(iosys_map_is_null(&primary_plane_info->map[0])))
>>> ^ here you are reintroducing an error that we were checking primary
>>> plane repeatedly, instead of plane_composer (renamed to:
>>> plane_frame_info here). The issue is fixed in a following patch of this
>>> series when you decouple check_iosys_map.
>>> But I don't mind fixing it before apply.
>> Should I simply delete this line in the patch? Or there's something
>> else to do?
> 
> No, you just need to check the correct plane (plane_frame_info), that means:
> 
> - if (WARN_ON(iosys_map_is_null(&plane_composer->map[0])))
> + if (WARN_ON(iosys_map_is_null(&plane_frame_info->map[0])))
> 
> because here you are renaming `plane_composer` to `plane_frame_info`,
> and `primary_plane_info->map[0]` is already checked in the following
> compose_active_planes() function.

Ohh. Got it now!

Thanks!

> 
> Thanks,
> 
> Melissa
> 
>>
>>>
>>>>    		return;
>>>> -	vaddr = plane_composer->map[0].vaddr;
>>>> +	vaddr = plane_frame_info->map[0].vaddr;
>>>>    	if (fb->format->format == DRM_FORMAT_ARGB8888)
>>>>    		pixel_blend = &alpha_blend;
>>>>    	else
>>>>    		pixel_blend = &x_blend;
>>>> -	blend(vaddr_out, vaddr, primary_composer, plane_composer, pixel_blend);
>>>> +	blend(vaddr_out, vaddr, primary_plane_info,
>>>> +	      plane_frame_info, pixel_blend);
>>>>    }
>>>>    static int compose_active_planes(void **vaddr_out,
>>>> -				 struct vkms_composer *primary_composer,
>>>> +				 struct vkms_frame_info *primary_plane_info,
>>>>    				 struct vkms_crtc_state *crtc_state)
>>>>    {
>>>> -	struct drm_framebuffer *fb = &primary_composer->fb;
>>>> +	struct drm_framebuffer *fb = &primary_plane_info->fb;
>>>>    	struct drm_gem_object *gem_obj = drm_gem_fb_get_obj(fb, 0);
>>>>    	const void *vaddr;
>>>>    	int i;
>>>> @@ -187,10 +188,10 @@ static int compose_active_planes(void **vaddr_out,
>>>>    		}
>>>>    	}
>>>> -	if (WARN_ON(iosys_map_is_null(&primary_composer->map[0])))
>>>> +	if (WARN_ON(iosys_map_is_null(&primary_plane_info->map[0])))
>>>>    		return -EINVAL;
>>>> -	vaddr = primary_composer->map[0].vaddr;
>>>> +	vaddr = primary_plane_info->map[0].vaddr;
>>>>    	memcpy(*vaddr_out, vaddr, gem_obj->size);
>>>> @@ -199,8 +200,8 @@ static int compose_active_planes(void **vaddr_out,
>>>>    	 * ((primary <- overlay) <- cursor)
>>>>    	 */
>>>>    	for (i = 1; i < crtc_state->num_active_planes; i++)
>>>> -		compose_plane(primary_composer,
>>>> -			      crtc_state->active_planes[i]->composer,
>>>> +		compose_plane(primary_plane_info,
>>>> +			      crtc_state->active_planes[i]->frame_info,
>>>>    			      *vaddr_out);
>>>>    	return 0;
>>>> @@ -222,7 +223,7 @@ void vkms_composer_worker(struct work_struct *work)
>>>>    						composer_work);
>>>>    	struct drm_crtc *crtc = crtc_state->base.crtc;
>>>>    	struct vkms_output *out = drm_crtc_to_vkms_output(crtc);
>>>> -	struct vkms_composer *primary_composer = NULL;
>>>> +	struct vkms_frame_info *primary_plane_info = NULL;
>>>>    	struct vkms_plane_state *act_plane = NULL;
>>>>    	bool crc_pending, wb_pending;
>>>>    	void *vaddr_out = NULL;
>>>> @@ -250,16 +251,16 @@ void vkms_composer_worker(struct work_struct *work)
>>>>    	if (crtc_state->num_active_planes >= 1) {
>>>>    		act_plane = crtc_state->active_planes[0];
>>>>    		if (act_plane->base.base.plane->type == DRM_PLANE_TYPE_PRIMARY)
>>>> -			primary_composer = act_plane->composer;
>>>> +			primary_plane_info = act_plane->frame_info;
>>>>    	}
>>>> -	if (!primary_composer)
>>>> +	if (!primary_plane_info)
>>>>    		return;
>>>>    	if (wb_pending)
>>>>    		vaddr_out = crtc_state->active_writeback->data[0].vaddr;
>>>> -	ret = compose_active_planes(&vaddr_out, primary_composer,
>>>> +	ret = compose_active_planes(&vaddr_out, primary_plane_info,
>>>>    				    crtc_state);
>>>>    	if (ret) {
>>>>    		if (ret == -EINVAL && !wb_pending)
>>>> @@ -267,7 +268,7 @@ void vkms_composer_worker(struct work_struct *work)
>>>>    		return;
>>>>    	}
>>>> -	crc32 = compute_crc(vaddr_out, primary_composer);
>>>> +	crc32 = compute_crc(vaddr_out, primary_plane_info);
>>>>    	if (wb_pending) {
>>>>    		drm_writeback_signal_completion(&out->wb_connector, 0);
>>>> diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
>>>> index 36fbab5989d1..5199c5f18e17 100644
>>>> --- a/drivers/gpu/drm/vkms/vkms_drv.h
>>>> +++ b/drivers/gpu/drm/vkms/vkms_drv.h
>>>> @@ -27,7 +27,7 @@ struct vkms_writeback_job {
>>>>    	struct iosys_map data[DRM_FORMAT_MAX_PLANES];
>>>>    };
>>>> -struct vkms_composer {
>>>> +struct vkms_frame_info {
>>>>    	struct drm_framebuffer fb;
>>>>    	struct drm_rect src, dst;
>>>>    	struct iosys_map map[DRM_FORMAT_MAX_PLANES];
>>>> @@ -39,11 +39,11 @@ struct vkms_composer {
>>>>    /**
>>>>     * vkms_plane_state - Driver specific plane state
>>>>     * @base: base plane state
>>>> - * @composer: data required for composing computation
>>>> + * @frame_info: data required for composing computation
>>>>     */
>>>>    struct vkms_plane_state {
>>>>    	struct drm_shadow_plane_state base;
>>>> -	struct vkms_composer *composer;
>>>> +	struct vkms_frame_info *frame_info;
>>>>    };
>>>>    struct vkms_plane {
>>>> diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
>>>> index d8eb674b49a6..fcae6c508f4b 100644
>>>> --- a/drivers/gpu/drm/vkms/vkms_plane.c
>>>> +++ b/drivers/gpu/drm/vkms/vkms_plane.c
>>>> @@ -24,20 +24,20 @@ static struct drm_plane_state *
>>>>    vkms_plane_duplicate_state(struct drm_plane *plane)
>>>>    {
>>>>    	struct vkms_plane_state *vkms_state;
>>>> -	struct vkms_composer *composer;
>>>> +	struct vkms_frame_info *frame_info;
>>>>    	vkms_state = kzalloc(sizeof(*vkms_state), GFP_KERNEL);
>>>>    	if (!vkms_state)
>>>>    		return NULL;
>>>> -	composer = kzalloc(sizeof(*composer), GFP_KERNEL);
>>>> -	if (!composer) {
>>>> -		DRM_DEBUG_KMS("Couldn't allocate composer\n");
>>>> +	frame_info = kzalloc(sizeof(*frame_info), GFP_KERNEL);
>>>> +	if (!frame_info) {
>>>> +		DRM_DEBUG_KMS("Couldn't allocate frame_info\n");
>>>>    		kfree(vkms_state);
>>>>    		return NULL;
>>>>    	}
>>>> -	vkms_state->composer = composer;
>>>> +	vkms_state->frame_info = frame_info;
>>>>    	__drm_gem_duplicate_shadow_plane_state(plane, &vkms_state->base);
>>>> @@ -54,12 +54,12 @@ static void vkms_plane_destroy_state(struct drm_plane *plane,
>>>>    		/* dropping the reference we acquired in
>>>>    		 * vkms_primary_plane_update()
>>>>    		 */
>>>> -		if (drm_framebuffer_read_refcount(&vkms_state->composer->fb))
>>>> -			drm_framebuffer_put(&vkms_state->composer->fb);
>>>> +		if (drm_framebuffer_read_refcount(&vkms_state->frame_info->fb))
>>>> +			drm_framebuffer_put(&vkms_state->frame_info->fb);
>>>>    	}
>>>> -	kfree(vkms_state->composer);
>>>> -	vkms_state->composer = NULL;
>>>> +	kfree(vkms_state->frame_info);
>>>> +	vkms_state->frame_info = NULL;
>>>>    	__drm_gem_destroy_shadow_plane_state(&vkms_state->base);
>>>>    	kfree(vkms_state);
>>>> @@ -99,7 +99,7 @@ static void vkms_plane_atomic_update(struct drm_plane *plane,
>>>>    	struct vkms_plane_state *vkms_plane_state;
>>>>    	struct drm_shadow_plane_state *shadow_plane_state;
>>>>    	struct drm_framebuffer *fb = new_state->fb;
>>>> -	struct vkms_composer *composer;
>>>> +	struct vkms_frame_info *frame_info;
>>>>    	if (!new_state->crtc || !fb)
>>>>    		return;
>>>> @@ -107,15 +107,15 @@ static void vkms_plane_atomic_update(struct drm_plane *plane,
>>>>    	vkms_plane_state = to_vkms_plane_state(new_state);
>>>>    	shadow_plane_state = &vkms_plane_state->base;
>>>> -	composer = vkms_plane_state->composer;
>>>> -	memcpy(&composer->src, &new_state->src, sizeof(struct drm_rect));
>>>> -	memcpy(&composer->dst, &new_state->dst, sizeof(struct drm_rect));
>>>> -	memcpy(&composer->fb, fb, sizeof(struct drm_framebuffer));
>>>> -	memcpy(&composer->map, &shadow_plane_state->data, sizeof(composer->map));
>>>> -	drm_framebuffer_get(&composer->fb);
>>>> -	composer->offset = fb->offsets[0];
>>>> -	composer->pitch = fb->pitches[0];
>>>> -	composer->cpp = fb->format->cpp[0];
>>>> +	frame_info = vkms_plane_state->frame_info;
>>>> +	memcpy(&frame_info->src, &new_state->src, sizeof(struct drm_rect));
>>>> +	memcpy(&frame_info->dst, &new_state->dst, sizeof(struct drm_rect));
>>>> +	memcpy(&frame_info->fb, fb, sizeof(struct drm_framebuffer));
>>>> +	memcpy(&frame_info->map, &shadow_plane_state->data, sizeof(frame_info->map));
>>>> +	drm_framebuffer_get(&frame_info->fb);
>>>> +	frame_info->offset = fb->offsets[0];
>>>> +	frame_info->pitch = fb->pitches[0];
>>>> +	frame_info->cpp = fb->format->cpp[0];
>>>>    }
>>>>    static int vkms_plane_atomic_check(struct drm_plane *plane,
>>>> -- 
>>>> 2.30.2
>>>>
>>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
index 775b97766e08..0aded4e87e60 100644
--- a/drivers/gpu/drm/vkms/vkms_composer.c
+++ b/drivers/gpu/drm/vkms/vkms_composer.c
@@ -11,11 +11,11 @@ 
 #include "vkms_drv.h"
 
 static u32 get_pixel_from_buffer(int x, int y, const u8 *buffer,
-				 const struct vkms_composer *composer)
+				 const struct vkms_frame_info *frame_info)
 {
 	u32 pixel;
-	int src_offset = composer->offset + (y * composer->pitch)
-				      + (x * composer->cpp);
+	int src_offset = frame_info->offset + (y * frame_info->pitch)
+					    + (x * frame_info->cpp);
 
 	pixel = *(u32 *)&buffer[src_offset];
 
@@ -26,24 +26,24 @@  static u32 get_pixel_from_buffer(int x, int y, const u8 *buffer,
  * compute_crc - Compute CRC value on output frame
  *
  * @vaddr: address to final framebuffer
- * @composer: framebuffer's metadata
+ * @frame_info: framebuffer's metadata
  *
  * returns CRC value computed using crc32 on the visible portion of
  * the final framebuffer at vaddr_out
  */
 static uint32_t compute_crc(const u8 *vaddr,
-			    const struct vkms_composer *composer)
+			    const struct vkms_frame_info *frame_info)
 {
 	int x, y;
 	u32 crc = 0, pixel = 0;
-	int x_src = composer->src.x1 >> 16;
-	int y_src = composer->src.y1 >> 16;
-	int h_src = drm_rect_height(&composer->src) >> 16;
-	int w_src = drm_rect_width(&composer->src) >> 16;
+	int x_src = frame_info->src.x1 >> 16;
+	int y_src = frame_info->src.y1 >> 16;
+	int h_src = drm_rect_height(&frame_info->src) >> 16;
+	int w_src = drm_rect_width(&frame_info->src) >> 16;
 
 	for (y = y_src; y < y_src + h_src; ++y) {
 		for (x = x_src; x < x_src + w_src; ++x) {
-			pixel = get_pixel_from_buffer(x, y, vaddr, composer);
+			pixel = get_pixel_from_buffer(x, y, vaddr, frame_info);
 			crc = crc32_le(crc, (void *)&pixel, sizeof(u32));
 		}
 	}
@@ -98,8 +98,8 @@  static void x_blend(const u8 *xrgb_src, u8 *xrgb_dst)
  * blend - blend value at vaddr_src with value at vaddr_dst
  * @vaddr_dst: destination address
  * @vaddr_src: source address
- * @dst_composer: destination framebuffer's metadata
- * @src_composer: source framebuffer's metadata
+ * @dst_frame_info: destination framebuffer's metadata
+ * @src_frame_info: source framebuffer's metadata
  * @pixel_blend: blending equation based on plane format
  *
  * Blend the vaddr_src value with the vaddr_dst value using a pixel blend
@@ -111,33 +111,33 @@  static void x_blend(const u8 *xrgb_src, u8 *xrgb_dst)
  * pixel color values
  */
 static void blend(void *vaddr_dst, void *vaddr_src,
-		  struct vkms_composer *dst_composer,
-		  struct vkms_composer *src_composer,
+		  struct vkms_frame_info *dst_frame_info,
+		  struct vkms_frame_info *src_frame_info,
 		  void (*pixel_blend)(const u8 *, u8 *))
 {
 	int i, j, j_dst, i_dst;
 	int offset_src, offset_dst;
 	u8 *pixel_dst, *pixel_src;
 
-	int x_src = src_composer->src.x1 >> 16;
-	int y_src = src_composer->src.y1 >> 16;
+	int x_src = src_frame_info->src.x1 >> 16;
+	int y_src = src_frame_info->src.y1 >> 16;
 
-	int x_dst = src_composer->dst.x1;
-	int y_dst = src_composer->dst.y1;
-	int h_dst = drm_rect_height(&src_composer->dst);
-	int w_dst = drm_rect_width(&src_composer->dst);
+	int x_dst = src_frame_info->dst.x1;
+	int y_dst = src_frame_info->dst.y1;
+	int h_dst = drm_rect_height(&src_frame_info->dst);
+	int w_dst = drm_rect_width(&src_frame_info->dst);
 
 	int y_limit = y_src + h_dst;
 	int x_limit = x_src + w_dst;
 
 	for (i = y_src, i_dst = y_dst; i < y_limit; ++i) {
 		for (j = x_src, j_dst = x_dst; j < x_limit; ++j) {
-			offset_dst = dst_composer->offset
-				     + (i_dst * dst_composer->pitch)
-				     + (j_dst++ * dst_composer->cpp);
-			offset_src = src_composer->offset
-				     + (i * src_composer->pitch)
-				     + (j * src_composer->cpp);
+			offset_dst = dst_frame_info->offset
+				     + (i_dst * dst_frame_info->pitch)
+				     + (j_dst++ * dst_frame_info->cpp);
+			offset_src = src_frame_info->offset
+				     + (i * src_frame_info->pitch)
+				     + (j * src_frame_info->cpp);
 
 			pixel_src = (u8 *)(vaddr_src + offset_src);
 			pixel_dst = (u8 *)(vaddr_dst + offset_dst);
@@ -149,32 +149,33 @@  static void blend(void *vaddr_dst, void *vaddr_src,
 	}
 }
 
-static void compose_plane(struct vkms_composer *primary_composer,
-			  struct vkms_composer *plane_composer,
+static void compose_plane(struct vkms_frame_info *primary_plane_info,
+			  struct vkms_frame_info *plane_frame_info,
 			  void *vaddr_out)
 {
-	struct drm_framebuffer *fb = &plane_composer->fb;
+	struct drm_framebuffer *fb = &plane_frame_info->fb;
 	void *vaddr;
 	void (*pixel_blend)(const u8 *p_src, u8 *p_dst);
 
-	if (WARN_ON(iosys_map_is_null(&plane_composer->map[0])))
+	if (WARN_ON(iosys_map_is_null(&primary_plane_info->map[0])))
 		return;
 
-	vaddr = plane_composer->map[0].vaddr;
+	vaddr = plane_frame_info->map[0].vaddr;
 
 	if (fb->format->format == DRM_FORMAT_ARGB8888)
 		pixel_blend = &alpha_blend;
 	else
 		pixel_blend = &x_blend;
 
-	blend(vaddr_out, vaddr, primary_composer, plane_composer, pixel_blend);
+	blend(vaddr_out, vaddr, primary_plane_info,
+	      plane_frame_info, pixel_blend);
 }
 
 static int compose_active_planes(void **vaddr_out,
-				 struct vkms_composer *primary_composer,
+				 struct vkms_frame_info *primary_plane_info,
 				 struct vkms_crtc_state *crtc_state)
 {
-	struct drm_framebuffer *fb = &primary_composer->fb;
+	struct drm_framebuffer *fb = &primary_plane_info->fb;
 	struct drm_gem_object *gem_obj = drm_gem_fb_get_obj(fb, 0);
 	const void *vaddr;
 	int i;
@@ -187,10 +188,10 @@  static int compose_active_planes(void **vaddr_out,
 		}
 	}
 
-	if (WARN_ON(iosys_map_is_null(&primary_composer->map[0])))
+	if (WARN_ON(iosys_map_is_null(&primary_plane_info->map[0])))
 		return -EINVAL;
 
-	vaddr = primary_composer->map[0].vaddr;
+	vaddr = primary_plane_info->map[0].vaddr;
 
 	memcpy(*vaddr_out, vaddr, gem_obj->size);
 
@@ -199,8 +200,8 @@  static int compose_active_planes(void **vaddr_out,
 	 * ((primary <- overlay) <- cursor)
 	 */
 	for (i = 1; i < crtc_state->num_active_planes; i++)
-		compose_plane(primary_composer,
-			      crtc_state->active_planes[i]->composer,
+		compose_plane(primary_plane_info,
+			      crtc_state->active_planes[i]->frame_info,
 			      *vaddr_out);
 
 	return 0;
@@ -222,7 +223,7 @@  void vkms_composer_worker(struct work_struct *work)
 						composer_work);
 	struct drm_crtc *crtc = crtc_state->base.crtc;
 	struct vkms_output *out = drm_crtc_to_vkms_output(crtc);
-	struct vkms_composer *primary_composer = NULL;
+	struct vkms_frame_info *primary_plane_info = NULL;
 	struct vkms_plane_state *act_plane = NULL;
 	bool crc_pending, wb_pending;
 	void *vaddr_out = NULL;
@@ -250,16 +251,16 @@  void vkms_composer_worker(struct work_struct *work)
 	if (crtc_state->num_active_planes >= 1) {
 		act_plane = crtc_state->active_planes[0];
 		if (act_plane->base.base.plane->type == DRM_PLANE_TYPE_PRIMARY)
-			primary_composer = act_plane->composer;
+			primary_plane_info = act_plane->frame_info;
 	}
 
-	if (!primary_composer)
+	if (!primary_plane_info)
 		return;
 
 	if (wb_pending)
 		vaddr_out = crtc_state->active_writeback->data[0].vaddr;
 
-	ret = compose_active_planes(&vaddr_out, primary_composer,
+	ret = compose_active_planes(&vaddr_out, primary_plane_info,
 				    crtc_state);
 	if (ret) {
 		if (ret == -EINVAL && !wb_pending)
@@ -267,7 +268,7 @@  void vkms_composer_worker(struct work_struct *work)
 		return;
 	}
 
-	crc32 = compute_crc(vaddr_out, primary_composer);
+	crc32 = compute_crc(vaddr_out, primary_plane_info);
 
 	if (wb_pending) {
 		drm_writeback_signal_completion(&out->wb_connector, 0);
diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
index 36fbab5989d1..5199c5f18e17 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -27,7 +27,7 @@  struct vkms_writeback_job {
 	struct iosys_map data[DRM_FORMAT_MAX_PLANES];
 };
 
-struct vkms_composer {
+struct vkms_frame_info {
 	struct drm_framebuffer fb;
 	struct drm_rect src, dst;
 	struct iosys_map map[DRM_FORMAT_MAX_PLANES];
@@ -39,11 +39,11 @@  struct vkms_composer {
 /**
  * vkms_plane_state - Driver specific plane state
  * @base: base plane state
- * @composer: data required for composing computation
+ * @frame_info: data required for composing computation
  */
 struct vkms_plane_state {
 	struct drm_shadow_plane_state base;
-	struct vkms_composer *composer;
+	struct vkms_frame_info *frame_info;
 };
 
 struct vkms_plane {
diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
index d8eb674b49a6..fcae6c508f4b 100644
--- a/drivers/gpu/drm/vkms/vkms_plane.c
+++ b/drivers/gpu/drm/vkms/vkms_plane.c
@@ -24,20 +24,20 @@  static struct drm_plane_state *
 vkms_plane_duplicate_state(struct drm_plane *plane)
 {
 	struct vkms_plane_state *vkms_state;
-	struct vkms_composer *composer;
+	struct vkms_frame_info *frame_info;
 
 	vkms_state = kzalloc(sizeof(*vkms_state), GFP_KERNEL);
 	if (!vkms_state)
 		return NULL;
 
-	composer = kzalloc(sizeof(*composer), GFP_KERNEL);
-	if (!composer) {
-		DRM_DEBUG_KMS("Couldn't allocate composer\n");
+	frame_info = kzalloc(sizeof(*frame_info), GFP_KERNEL);
+	if (!frame_info) {
+		DRM_DEBUG_KMS("Couldn't allocate frame_info\n");
 		kfree(vkms_state);
 		return NULL;
 	}
 
-	vkms_state->composer = composer;
+	vkms_state->frame_info = frame_info;
 
 	__drm_gem_duplicate_shadow_plane_state(plane, &vkms_state->base);
 
@@ -54,12 +54,12 @@  static void vkms_plane_destroy_state(struct drm_plane *plane,
 		/* dropping the reference we acquired in
 		 * vkms_primary_plane_update()
 		 */
-		if (drm_framebuffer_read_refcount(&vkms_state->composer->fb))
-			drm_framebuffer_put(&vkms_state->composer->fb);
+		if (drm_framebuffer_read_refcount(&vkms_state->frame_info->fb))
+			drm_framebuffer_put(&vkms_state->frame_info->fb);
 	}
 
-	kfree(vkms_state->composer);
-	vkms_state->composer = NULL;
+	kfree(vkms_state->frame_info);
+	vkms_state->frame_info = NULL;
 
 	__drm_gem_destroy_shadow_plane_state(&vkms_state->base);
 	kfree(vkms_state);
@@ -99,7 +99,7 @@  static void vkms_plane_atomic_update(struct drm_plane *plane,
 	struct vkms_plane_state *vkms_plane_state;
 	struct drm_shadow_plane_state *shadow_plane_state;
 	struct drm_framebuffer *fb = new_state->fb;
-	struct vkms_composer *composer;
+	struct vkms_frame_info *frame_info;
 
 	if (!new_state->crtc || !fb)
 		return;
@@ -107,15 +107,15 @@  static void vkms_plane_atomic_update(struct drm_plane *plane,
 	vkms_plane_state = to_vkms_plane_state(new_state);
 	shadow_plane_state = &vkms_plane_state->base;
 
-	composer = vkms_plane_state->composer;
-	memcpy(&composer->src, &new_state->src, sizeof(struct drm_rect));
-	memcpy(&composer->dst, &new_state->dst, sizeof(struct drm_rect));
-	memcpy(&composer->fb, fb, sizeof(struct drm_framebuffer));
-	memcpy(&composer->map, &shadow_plane_state->data, sizeof(composer->map));
-	drm_framebuffer_get(&composer->fb);
-	composer->offset = fb->offsets[0];
-	composer->pitch = fb->pitches[0];
-	composer->cpp = fb->format->cpp[0];
+	frame_info = vkms_plane_state->frame_info;
+	memcpy(&frame_info->src, &new_state->src, sizeof(struct drm_rect));
+	memcpy(&frame_info->dst, &new_state->dst, sizeof(struct drm_rect));
+	memcpy(&frame_info->fb, fb, sizeof(struct drm_framebuffer));
+	memcpy(&frame_info->map, &shadow_plane_state->data, sizeof(frame_info->map));
+	drm_framebuffer_get(&frame_info->fb);
+	frame_info->offset = fb->offsets[0];
+	frame_info->pitch = fb->pitches[0];
+	frame_info->cpp = fb->format->cpp[0];
 }
 
 static int vkms_plane_atomic_check(struct drm_plane *plane,