On 04/15, Tales Lelo da Aparecida wrote:

Fix a copypasta error. The caller of compose_plane() already checks primary_composer->map. In contrast, plane_composer->map is never verified here before handling.

Fixes: 7938f4218168 ("dma-buf-map: Rename to iosys-map") Reviewed-by: André Almeida andrealmeid@riseup.net Signed-off-by: Tales Lelo da Aparecida tales.aparecida@gmail.com

---

v2: detail the commit message with more information

drivers/gpu/drm/vkms/vkms_composer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c index c6a1036bf2ea..b47ac170108c 100644 --- a/drivers/gpu/drm/vkms/vkms_composer.c +++ b/drivers/gpu/drm/vkms/vkms_composer.c @@ -157,7 +157,7 @@ static void compose_plane(struct vkms_composer *primary_composer, void *vaddr; void (*pixel_blend)(const u8 *p_src, u8 *p_dst);

• if (WARN_ON(iosys_map_is_null(&primary_composer->map[0])))

• if (WARN_ON(iosys_map_is_null(&plane_composer->map[0]))) return;

I cherry-picked this one and applied to drm-misc-next.

Thanks,

Melissa

vaddr = plane_composer->map[0].vaddr;

2.35.1

On 04/15, Tales Lelo da Aparecida wrote:

Do not exit quietly from compose_plane. If any plane has an invalid map, propagate the error value upwards. While here, add log messages for the invalid index.

Signed-off-by: Tales Lelo da Aparecida tales.aparecida@gmail.com

drivers/gpu/drm/vkms/vkms_composer.c | 30 ++++++++++++++++++---------- 1 file changed, 20 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c index b47ac170108c..c0a3b53cd155 100644 --- a/drivers/gpu/drm/vkms/vkms_composer.c +++ b/drivers/gpu/drm/vkms/vkms_composer.c @@ -149,16 +149,16 @@ static void blend(void *vaddr_dst, void *vaddr_src, } }

-static void compose_plane(struct vkms_composer *primary_composer,

• 	  struct vkms_composer *plane_composer,
  

• 	  void *vaddr_out)
  

+static int compose_plane(struct vkms_composer *primary_composer,

• 	 struct vkms_composer *plane_composer,
  

• 	 void *vaddr_out)
  

{ struct drm_framebuffer *fb = &plane_composer->fb; void *vaddr; void (*pixel_blend)(const u8 *p_src, u8 *p_dst);

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

• return;
  

• return -EINVAL;
  
  

  vaddr = plane_composer->map[0].vaddr;

@@ -168,6 +168,8 @@ static void compose_plane(struct vkms_composer *primary_composer, pixel_blend = &x_blend;

blend(vaddr_out, vaddr, primary_composer, plane_composer, pixel_blend);

• return 0;

}

static int compose_active_planes(void **vaddr_out, @@ -177,7 +179,7 @@ static int compose_active_planes(void **vaddr_out, struct drm_framebuffer *fb = &primary_composer->fb; struct drm_gem_object *gem_obj = drm_gem_fb_get_obj(fb, 0); const void *vaddr;

• int i;

• int i, ret;

  if (!*vaddr_out) { *vaddr_out = kzalloc(gem_obj->size, GFP_KERNEL);

@@ -187,8 +189,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_composer->map[0]))) {

• DRM_DEBUG_DRIVER("Failed to compose. Invalid map in the primary plane.");
  

  return -EINVAL;

yes, good to have a debug msg here. I would say `mapping`

• }

  vaddr = primary_composer->map[0].vaddr;

@@ -198,10 +202,16 @@ static int compose_active_planes(void **vaddr_out, * planes should be in z-order and compose them associatively: * ((primary <- overlay) <- cursor) */

• for (i = 1; i < crtc_state->num_active_planes; i++)

• compose_plane(primary_composer,
  

• 	      crtc_state->active_planes[i]->composer,
  

• 	      *vaddr_out);
  

• for (i = 1; i < crtc_state->num_active_planes; i++) {

• ret = compose_plane(primary_composer,
  

• 		    crtc_state->active_planes[i]->composer,
  

• 		    *vaddr_out);
  

tbh, I'm not sure on changing compose_plane behaviour here to invalidate all composition in case of a invalid active plane mapping.

Warning about an inconsistent composition makes sense to me, but it would be better if we can prevent all resources consumption around each plane composition by checking the issue as soon as possible. One idea would be doing this validation at the time of active_plane selection. Another would be just check all active_plane mapping right before this loop.

What do you think?

• if (ret) {
  

• 	DRM_DEBUG_DRIVER("Failed to compose. Invalid map in the active_planes[%d].",
  

• 			 i);
  

• 	return ret;
  

• }
  

• }

  return 0;

}

2.35.1