[PATCH 2/2] drm/udl: implement cursor.

[Chris Wilson]

On Wed, Mar 04, 2015 at 06:26:24PM -0800, Haixia Shi wrote:
> Signed-off-by: Haixia Shi <hshi at chromium.org>
> Reviewed-by: Stéphane Marchesin <marcheu at chromium.org>
> Tested-by: Haixia Shi <hshi at chromium.org>
> ---

> +static int udl_cursor_download(struct udl_cursor *cursor,
> +		struct drm_gem_object *obj)
> +{
> +	struct udl_gem_object *udl_gem_obj = to_udl_bo(obj);
> +	uint32_t *src_ptr, *dst_ptr;
> +	size_t i;
> +
> +	int ret = udl_gem_vmap(udl_gem_obj);
> +	if (ret != 0) {
> +		DRM_ERROR("failed to vmap cursor\n");
> +		return ret;
> +	}
> +
> +	src_ptr = udl_gem_obj->vmapping;
> +	dst_ptr = cursor->buffer;
> +	for (i = 0; i < UDL_CURSOR_BUF; ++i)
> +		dst_ptr[i] = cursor_blend_val32(le32_to_cpu(src_ptr[i]));

You never verify that udl_gem_obj->size >= UDL_CURSOR_BUF*sizeof(uint32_t)

> @@ -220,14 +216,21 @@ int udl_handle_damage(struct udl_framebuffer *fb, int x, int y,
>  		return 0;
>  	cmd = urb->transfer_buffer;
>  
> +	mutex_lock(&dev->struct_mutex);
> +	if (udl_cursor_alloc(&cursor_copy) == 0)
> +		udl_cursor_copy(cursor_copy, udl->cursor);
> +	mutex_unlock(&dev->struct_mutex);

Keeping a reference to the udl->cursor->buffer and making it COW looks
quite attrative now. Note that struct_mutex is completely the wrong lock
to be using here! It is not the lock you hold when you are modifing the
cursor. *

cursor = udl_cursor_get(udl->cursor);

return NULL on error and make the subsequent code deal with it (rather
than discarding the whole frame update). But if you take the COW reference
approach it should not fail (the failure will only occur when modifying
the cursor, where the error can be propagated back to the user
immediately).


> +static int udl_crtc_cursor_set(struct drm_crtc *crtc, struct drm_file *file,
> +		uint32_t handle, uint32_t width, uint32_t height)
> +{
> +	struct drm_device *dev = crtc->dev;
> +	struct udl_device *udl = dev->dev_private;
> +	int ret;
> +
> +	mutex_lock(&dev->struct_mutex);
> +	ret = udl_cursor_set(crtc, file, handle, width, height, udl->cursor);
> +	mutex_unlock(&dev->struct_mutex);

* Oh, I see. udl_crtc_cursor_set() is already locked, so you don't need
struct_mutex as well.

> +	if (ret) {
> +		DRM_ERROR("Failed to set UDL cursor\n");
> +		return ret;
> +	}
> +
> +	return udl_crtc_page_flip(crtc, NULL, NULL, 0);

You have to be kidding me! You redraw the entire frame because the
cursor changes is is moved! We might as well leave the cursor to
userspace, it will be much more performant.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre