Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1407069 > unrolled thread

Re: [PATCH] Add virtio gpu driver.

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] Add virtio gpu driver. Daniel Vetter <daniel@ffwll.ch> - 2016-05-25 18:50 +0200
    Re: [PATCH] Add virtio gpu driver. Emil Velikov <emil.l.velikov@gmail.com> - 2016-05-25 18:50 +0200
    Re: [PATCH] Add virtio gpu driver. Gerd Hoffmann <kraxel@redhat.com> - 2016-05-27 09:50 +0200
      Re: [PATCH] Add virtio gpu driver. Daniel Vetter <daniel@ffwll.ch> - 2016-05-27 11:10 +0200
        Re: [PATCH] Add virtio gpu driver. Gerd Hoffmann <kraxel@redhat.com> - 2016-05-30 16:00 +0200
          Re: [PATCH] Add virtio gpu driver. Daniel Vetter <daniel@ffwll.ch> - 2016-05-30 16:50 +0200
            Re: [PATCH] Add virtio gpu driver. Gerd Hoffmann <kraxel@redhat.com> - 2016-05-31 08:30 +0200
              Re: [PATCH] Add virtio gpu driver. Daniel Vetter <daniel@ffwll.ch> - 2016-05-31 09:00 +0200
              Re: [PATCH] Add virtio gpu driver. Pekka Paalanen <ppaalanen@gmail.com> - 2016-05-31 09:10 +0200

#1407069 — Re: [PATCH] Add virtio gpu driver.

On Mon, Mar 30, 2015 at 4:49 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Mon, Mar 30, 2015 at 02:23:47PM +0200, Gerd Hoffmann wrote:
>> > > Signed-off-by: Dave Airlie <airlied@redhat.com>
>> > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>> >
>> > Standard request from my side for new drm drivers (especially if they're
>> > this simple): Can you please update the drivers to latest drm internal
>> > interfaces, i.e. using universal planes and atomic?
>>
>> Up'n'running.  Incremental patch:
>>
>> https://www.kraxel.org/cgit/linux/commit/?h=virtio-gpu-2d&id=b8edf4f38a1ec5a50f6ac8948521a12f862d3d5a
>>
>> v2 coming, but I'll go over the other reviews first.
>
> Looking good. Wrt pageflip the current MO is to handroll it in your
> driver, common approach is to use the msm async commit implementation
> msm_atomic_commit. The issue is simply that right now there's still no
> useable generic vblank callback support (drm_irq.c is a mess) hence why
> the core helpers don't support async flips yet.

I guess I didn't do a good job at looking at your v2: Cursor is still
using legacy interfaces and not a proper plane. Would be awesome if
you could fix that up. Atomic drivers really shouldn't use the legacy
cursor interfaces any more at all.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[toc] | [next] | [standalone]

#1407073

On 25 May 2016 at 17:40, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Mon, Mar 30, 2015 at 4:49 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
>> On Mon, Mar 30, 2015 at 02:23:47PM +0200, Gerd Hoffmann wrote:
>>> > > Signed-off-by: Dave Airlie <airlied@redhat.com>
>>> > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>>> >
>>> > Standard request from my side for new drm drivers (especially if they're
>>> > this simple): Can you please update the drivers to latest drm internal
>>> > interfaces, i.e. using universal planes and atomic?
>>>
>>> Up'n'running.  Incremental patch:
>>>
>>> https://www.kraxel.org/cgit/linux/commit/?h=virtio-gpu-2d&id=b8edf4f38a1ec5a50f6ac8948521a12f862d3d5a
>>>
>>> v2 coming, but I'll go over the other reviews first.
>>
>> Looking good. Wrt pageflip the current MO is to handroll it in your
>> driver, common approach is to use the msm async commit implementation
>> msm_atomic_commit. The issue is simply that right now there's still no
>> useable generic vblank callback support (drm_irq.c is a mess) hence why
>> the core helpers don't support async flips yet.
>
> I guess I didn't do a good job at looking at your v2: Cursor is still
> using legacy interfaces and not a proper plane. Would be awesome if
> you could fix that up. Atomic drivers really shouldn't use the legacy
> cursor interfaces any more at all.

Wild idea:
Worth adding if (drm_core_check_feature(dev, DRIVER_ATOMIC)  {
printf("abort abort"); return; }
style of checks for the legacy (preatomic) kms helpers ?

Or does it feel like an overkill ?

-Emil

[toc] | [prev] | [next] | [standalone]

#1407882

[Multipart message — attachments visible in raw view] — view raw

  Hi,

> I guess I didn't do a good job at looking at your v2: Cursor is still
> using legacy interfaces and not a proper plane. Would be awesome if
> you could fix that up. Atomic drivers really shouldn't use the legacy
> cursor interfaces any more at all.
> -Daniel

Figured that one for the most part, see attached draft.

The only thing I'm wondering is how the hotspot is handled.
drm_mode_cursor_universal doesn't even look at req->hot_{x,y}.

/me looks confused.

cheers,
  Gerd

[toc] | [prev] | [next] | [standalone]

#1407945

On Fri, May 27, 2016 at 09:48:22AM +0200, Gerd Hoffmann wrote:
> > I guess I didn't do a good job at looking at your v2: Cursor is still
> > using legacy interfaces and not a proper plane. Would be awesome if
> > you could fix that up. Atomic drivers really shouldn't use the legacy
> > cursor interfaces any more at all.
> > -Daniel
> 
> Figured that one for the most part, see attached draft.
> 
> The only thing I'm wondering is how the hotspot is handled.
> drm_mode_cursor_universal doesn't even look at req->hot_{x,y}.
> 
> /me looks confused.

No need to, you're simply the first virtual driver to wire up atomic
cursors. Hence some gaps to be filled out.

First we need to wire up the state tracking scaffolding for atomic:
- add hot_x/y to drm_plane_state
- add property pointers for hot_x/y to dev->mode_config (like we have for
  all the other atomic props like "SRC_X").
- add encode/decode support for these properties to
  drm_atomic_plane_get_property and drm_atomic_plane_set_property, similar
  again to "SRC_X" and friends
- add a small core function to registerr HOT_X/HOT_Y for a (cursor) plane,
  e.g. drm_plane_register_hotspot(). That should allocate the properties
  (if they don't exist yet) and then attach those props to the cursor. We
  don't want those props everywhere, but only on drivers that support/need
  them, aka virtual hw.

With that a real atomic driver will be able to move the cursor and it's
hotspot around, all nicely done in an atomic commit. But it won't work yet
for userspace for legacy applications. For that we need a notch more:
- one option would be to add hot_x/hot_y to the ->update_plane hook, but
  that has massive trickling effects throughout the subsystem. Probably
  not what we want to do.
- 2nd option would be to add a DRIVER_ATOMIC check to
  drm_mode_cursor_common and call a new drm_mode_cursor_atomic in that
  case:

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 3e52a6ecf6c0..2f15ce2c6bf4 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -3046,7 +3046,10 @@ static int drm_mode_cursor_common(struct drm_device *dev,
 	 */
 	drm_modeset_lock_crtc(crtc, crtc->cursor);
 	if (crtc->cursor) {
-		ret = drm_mode_cursor_universal(crtc, req, file_priv);
+		if (drm_core_check_feature(DRIVER_ATOMIC))
+			ret = drm_mode_cursor_atomic(crtc, req, file_priv);
+		else
+			ret = drm_mode_cursor_universal(crtc, req, file_priv);
 		goto out;
 	}
 
  drm_mode_cursor_atomic would simply be a fusing of
  drm_mode_cursor_universal + drm_atomic_helper_update_plane (dump all the
  intermediate variables and store directly in the plane state), with the
  addition of also storing hot_x/y into the plane state.

Sorry that this turned into a bit of a project, I've forgotten that we
haven't wired up hot_x/y at all for atomic ...

If you don't want to bother with the atomic properties (only needed for
atomic userspace), then just adding hot_x/y to drm_plane_state is all you
need from the first group of tasks.

Your patch below to implement the atomic cursor looks reasonable. Although
personally I'd go with a separate vfunc table for the cursor so that you
can avoid that ugly switch in the atomic_update hook.

Cheers, Daniel

> 
> cheers,
>   Gerd
> 

> From fb1d0700a46d850ec9f931304a9e99854a3ce5e9 Mon Sep 17 00:00:00 2001
> From: Gerd Hoffmann <kraxel@redhat.com>
> Date: Thu, 26 May 2016 11:42:52 +0200
> Subject: [PATCH] [wip] virtio-gpu: switch to atomic cursor interfaces
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  drivers/gpu/drm/virtio/virtgpu_display.c | 102 +++-----------------------
>  drivers/gpu/drm/virtio/virtgpu_drv.h     |   1 +
>  drivers/gpu/drm/virtio/virtgpu_plane.c   | 122 ++++++++++++++++++++++++-------
>  3 files changed, 109 insertions(+), 116 deletions(-)
> 
> diff --git a/drivers/gpu/drm/virtio/virtgpu_display.c b/drivers/gpu/drm/virtio/virtgpu_display.c
> index 5990cab..d6b16d1 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_display.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_display.c
> @@ -29,8 +29,8 @@
>  #include <drm/drm_crtc_helper.h>
>  #include <drm/drm_atomic_helper.h>
>  
> -#define XRES_MIN   320
> -#define YRES_MIN   200
> +#define XRES_MIN    32
> +#define YRES_MIN    32
>  
>  #define XRES_DEF  1024
>  #define YRES_DEF   768
> @@ -38,86 +38,6 @@
>  #define XRES_MAX  8192
>  #define YRES_MAX  8192
>  
> -static void
> -virtio_gpu_hide_cursor(struct virtio_gpu_device *vgdev,
> -		       struct virtio_gpu_output *output)
> -{
> -	output->cursor.hdr.type = cpu_to_le32(VIRTIO_GPU_CMD_UPDATE_CURSOR);
> -	output->cursor.resource_id = 0;
> -	virtio_gpu_cursor_ping(vgdev, output);
> -}
> -
> -static int virtio_gpu_crtc_cursor_set(struct drm_crtc *crtc,
> -				      struct drm_file *file_priv,
> -				      uint32_t handle,
> -				      uint32_t width,
> -				      uint32_t height,
> -				      int32_t hot_x, int32_t hot_y)
> -{
> -	struct virtio_gpu_device *vgdev = crtc->dev->dev_private;
> -	struct virtio_gpu_output *output =
> -		container_of(crtc, struct virtio_gpu_output, crtc);
> -	struct drm_gem_object *gobj = NULL;
> -	struct virtio_gpu_object *qobj = NULL;
> -	struct virtio_gpu_fence *fence = NULL;
> -	int ret = 0;
> -
> -	if (handle == 0) {
> -		virtio_gpu_hide_cursor(vgdev, output);
> -		return 0;
> -	}
> -
> -	/* lookup the cursor */
> -	gobj = drm_gem_object_lookup(crtc->dev, file_priv, handle);
> -	if (gobj == NULL)
> -		return -ENOENT;
> -
> -	qobj = gem_to_virtio_gpu_obj(gobj);
> -
> -	if (!qobj->hw_res_handle) {
> -		ret = -EINVAL;
> -		goto out;
> -	}
> -
> -	virtio_gpu_cmd_transfer_to_host_2d(vgdev, qobj->hw_res_handle, 0,
> -					   cpu_to_le32(64),
> -					   cpu_to_le32(64),
> -					   0, 0, &fence);
> -	ret = virtio_gpu_object_reserve(qobj, false);
> -	if (!ret) {
> -		reservation_object_add_excl_fence(qobj->tbo.resv,
> -						  &fence->f);
> -		fence_put(&fence->f);
> -		virtio_gpu_object_unreserve(qobj);
> -		virtio_gpu_object_wait(qobj, false);
> -	}
> -
> -	output->cursor.hdr.type = cpu_to_le32(VIRTIO_GPU_CMD_UPDATE_CURSOR);
> -	output->cursor.resource_id = cpu_to_le32(qobj->hw_res_handle);
> -	output->cursor.hot_x = cpu_to_le32(hot_x);
> -	output->cursor.hot_y = cpu_to_le32(hot_y);
> -	virtio_gpu_cursor_ping(vgdev, output);
> -	ret = 0;
> -
> -out:
> -	drm_gem_object_unreference_unlocked(gobj);
> -	return ret;
> -}
> -
> -static int virtio_gpu_crtc_cursor_move(struct drm_crtc *crtc,
> -				    int x, int y)
> -{
> -	struct virtio_gpu_device *vgdev = crtc->dev->dev_private;
> -	struct virtio_gpu_output *output =
> -		container_of(crtc, struct virtio_gpu_output, crtc);
> -
> -	output->cursor.hdr.type = cpu_to_le32(VIRTIO_GPU_CMD_MOVE_CURSOR);
> -	output->cursor.pos.x = cpu_to_le32(x);
> -	output->cursor.pos.y = cpu_to_le32(y);
> -	virtio_gpu_cursor_ping(vgdev, output);
> -	return 0;
> -}
> -
>  static int virtio_gpu_page_flip(struct drm_crtc *crtc,
>  				struct drm_framebuffer *fb,
>  				struct drm_pending_vblank_event *event,
> @@ -164,8 +84,6 @@ static int virtio_gpu_page_flip(struct drm_crtc *crtc,
>  }
>  
>  static const struct drm_crtc_funcs virtio_gpu_crtc_funcs = {
> -	.cursor_set2            = virtio_gpu_crtc_cursor_set,
> -	.cursor_move            = virtio_gpu_crtc_cursor_move,
>  	.set_config             = drm_atomic_helper_set_config,
>  	.destroy                = drm_crtc_cleanup,
>  
> @@ -406,7 +324,7 @@ static int vgdev_output_init(struct virtio_gpu_device *vgdev, int index)
>  	struct drm_connector *connector = &output->conn;
>  	struct drm_encoder *encoder = &output->enc;
>  	struct drm_crtc *crtc = &output->crtc;
> -	struct drm_plane *plane;
> +	struct drm_plane *primary, *cursor;
>  
>  	output->index = index;
>  	if (index == 0) {
> @@ -415,14 +333,18 @@ static int vgdev_output_init(struct virtio_gpu_device *vgdev, int index)
>  		output->info.r.height = cpu_to_le32(YRES_DEF);
>  	}
>  
> -	plane = virtio_gpu_plane_init(vgdev, index);
> -	if (IS_ERR(plane))
> -		return PTR_ERR(plane);
> -	drm_crtc_init_with_planes(dev, crtc, plane, NULL,
> +	primary = virtio_gpu_plane_init(vgdev, DRM_PLANE_TYPE_PRIMARY, index);
> +	if (IS_ERR(primary))
> +		return PTR_ERR(primary);
> +	cursor = virtio_gpu_plane_init(vgdev, DRM_PLANE_TYPE_CURSOR, index);
> +	if (IS_ERR(cursor))
> +		return PTR_ERR(cursor);
> +	drm_crtc_init_with_planes(dev, crtc, primary, cursor,
>  				  &virtio_gpu_crtc_funcs, NULL);
>  	drm_mode_crtc_set_gamma_size(crtc, 256);
>  	drm_crtc_helper_add(crtc, &virtio_gpu_crtc_helper_funcs);
> -	plane->crtc = crtc;
> +	primary->crtc = crtc;
> +	cursor->crtc = crtc;
>  
>  	drm_connector_init(dev, connector, &virtio_gpu_connector_funcs,
>  			   DRM_MODE_CONNECTOR_VIRTUAL);
> diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h
> index 8f486f4..a1f7e9d 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_drv.h
> +++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
> @@ -335,6 +335,7 @@ void virtio_gpu_modeset_fini(struct virtio_gpu_device *vgdev);
>  
>  /* virtio_gpu_plane.c */
>  struct drm_plane *virtio_gpu_plane_init(struct virtio_gpu_device *vgdev,
> +					enum drm_plane_type type,
>  					int index);
>  
>  /* virtio_gpu_ttm.c */
> diff --git a/drivers/gpu/drm/virtio/virtgpu_plane.c b/drivers/gpu/drm/virtio/virtgpu_plane.c
> index 70b44a2..d68270f 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_plane.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_plane.c
> @@ -38,6 +38,10 @@ static const uint32_t virtio_gpu_formats[] = {
>  	DRM_FORMAT_ABGR8888,
>  };
>  
> +static const uint32_t virtio_gpu_cursor_formats[] = {
> +	DRM_FORMAT_ARGB8888,
> +};
> +
>  static void virtio_gpu_plane_destroy(struct drm_plane *plane)
>  {
>  	kfree(plane);
> @@ -63,39 +67,97 @@ static void virtio_gpu_plane_atomic_update(struct drm_plane *plane,
>  {
>  	struct drm_device *dev = plane->dev;
>  	struct virtio_gpu_device *vgdev = dev->dev_private;
> -	struct virtio_gpu_output *output = drm_crtc_to_virtio_gpu_output(plane->crtc);
> +	struct virtio_gpu_output *output = NULL;
>  	struct virtio_gpu_framebuffer *vgfb;
> -	struct virtio_gpu_object *bo;
> +	struct virtio_gpu_fence *fence = NULL;
> +	struct virtio_gpu_object *bo = NULL;
>  	uint32_t handle;
> +	int ret = 0;
> +
> +	if (plane->state->crtc)
> +		output = drm_crtc_to_virtio_gpu_output(plane->state->crtc);
> +	if (old_state->crtc)
> +		output = drm_crtc_to_virtio_gpu_output(old_state->crtc);
> +	WARN_ON(!output);
>  
>  	if (plane->state->fb) {
>  		vgfb = to_virtio_gpu_framebuffer(plane->state->fb);
>  		bo = gem_to_virtio_gpu_obj(vgfb->obj);
>  		handle = bo->hw_res_handle;
> -		if (bo->dumb) {
> -			virtio_gpu_cmd_transfer_to_host_2d
> -				(vgdev, handle, 0,
> -				 cpu_to_le32(plane->state->crtc_w),
> -				 cpu_to_le32(plane->state->crtc_h),
> -				 plane->state->crtc_x, plane->state->crtc_y, NULL);
> -		}
>  	} else {
>  		handle = 0;
>  	}
>  
> -	DRM_DEBUG("handle 0x%x, crtc %dx%d+%d+%d\n", handle,
> -		  plane->state->crtc_w, plane->state->crtc_h,
> -		  plane->state->crtc_x, plane->state->crtc_y);
> -	virtio_gpu_cmd_set_scanout(vgdev, output->index, handle,
> -				   plane->state->crtc_w,
> -				   plane->state->crtc_h,
> -				   plane->state->crtc_x,
> -				   plane->state->crtc_y);
> -	virtio_gpu_cmd_resource_flush(vgdev, handle,
> -				      plane->state->crtc_x,
> -				      plane->state->crtc_y,
> -				      plane->state->crtc_w,
> -				      plane->state->crtc_h);
> +	switch (plane->type) {
> +	case DRM_PLANE_TYPE_CURSOR:
> +		if (bo && bo->dumb && (plane->state->fb != old_state->fb)) {
> +			/* new cursor -- update & wait */
> +			virtio_gpu_cmd_transfer_to_host_2d
> +				(vgdev, handle, 0,
> +				 cpu_to_le32(plane->state->crtc_w),
> +				 cpu_to_le32(plane->state->crtc_h),
> +				 0, 0, &fence);
> +			ret = virtio_gpu_object_reserve(bo, false);
> +			if (!ret) {
> +				reservation_object_add_excl_fence(bo->tbo.resv,
> +								  &fence->f);
> +				fence_put(&fence->f);
> +				fence = NULL;
> +				virtio_gpu_object_unreserve(bo);
> +				virtio_gpu_object_wait(bo, false);
> +			}
> +		}
> +
> +		if (plane->state->fb != old_state->fb) {
> +			DRM_DEBUG("cursor update, handle %d, +%d+%d\n", handle,
> +				  plane->state->crtc_x,
> +				  plane->state->crtc_y);
> +			output->cursor.hdr.type =
> +				cpu_to_le32(VIRTIO_GPU_CMD_UPDATE_CURSOR);
> +			output->cursor.resource_id = cpu_to_le32(handle);
> +#if 0
> +			output->cursor.hot_x = cpu_to_le32(hot_x);
> +			output->cursor.hot_y = cpu_to_le32(hot_y);
> +#endif
> +		} else {
> +			DRM_DEBUG("cursor move +%d+%d\n",
> +				  plane->state->crtc_x,
> +				  plane->state->crtc_y);
> +			output->cursor.hdr.type =
> +				cpu_to_le32(VIRTIO_GPU_CMD_MOVE_CURSOR);
> +		}
> +		output->cursor.pos.x = cpu_to_le32(plane->state->crtc_x);
> +		output->cursor.pos.y = cpu_to_le32(plane->state->crtc_y);
> +		virtio_gpu_cursor_ping(vgdev, output);
> +		break;
> +
> +	case DRM_PLANE_TYPE_PRIMARY:
> +		DRM_DEBUG("primary, handle 0x%x, crtc %dx%d+%d+%d\n", handle,
> +			  plane->state->crtc_w, plane->state->crtc_h,
> +			  plane->state->crtc_x, plane->state->crtc_y);
> +		if (bo && bo->dumb) {
> +			virtio_gpu_cmd_transfer_to_host_2d
> +				(vgdev, handle, 0,
> +				 cpu_to_le32(plane->state->crtc_w),
> +				 cpu_to_le32(plane->state->crtc_h),
> +				 plane->state->crtc_x, plane->state->crtc_y,
> +				 &fence);
> +		}
> +		virtio_gpu_cmd_set_scanout(vgdev, output->index, handle,
> +					   plane->state->crtc_w,
> +					   plane->state->crtc_h,
> +					   plane->state->crtc_x,
> +					   plane->state->crtc_y);
> +		virtio_gpu_cmd_resource_flush(vgdev, handle,
> +					      plane->state->crtc_x,
> +					      plane->state->crtc_y,
> +					      plane->state->crtc_w,
> +					      plane->state->crtc_h);
> +		break;
> +
> +	default:
> +		WARN_ON(true);
> +	}
>  }
>  
>  
> @@ -105,21 +167,29 @@ static const struct drm_plane_helper_funcs virtio_gpu_plane_helper_funcs = {
>  };
>  
>  struct drm_plane *virtio_gpu_plane_init(struct virtio_gpu_device *vgdev,
> +					enum drm_plane_type type,
>  					int index)
>  {
>  	struct drm_device *dev = vgdev->ddev;
>  	struct drm_plane *plane;
> -	int ret;
> +	const uint32_t *formats;
> +	int ret, nformats;
>  
>  	plane = kzalloc(sizeof(*plane), GFP_KERNEL);
>  	if (!plane)
>  		return ERR_PTR(-ENOMEM);
>  
> +	if (type == DRM_PLANE_TYPE_CURSOR) {
> +		formats = virtio_gpu_cursor_formats;
> +		nformats = ARRAY_SIZE(virtio_gpu_cursor_formats);
> +	} else {
> +		formats = virtio_gpu_formats;
> +		nformats = ARRAY_SIZE(virtio_gpu_formats);
> +	}
>  	ret = drm_universal_plane_init(dev, plane, 1 << index,
>  				       &virtio_gpu_plane_funcs,
> -				       virtio_gpu_formats,
> -				       ARRAY_SIZE(virtio_gpu_formats),
> -				       DRM_PLANE_TYPE_PRIMARY, NULL);
> +				       formats, nformats,
> +				       type, NULL);
>  	if (ret)
>  		goto err_plane_init;
>  
> -- 
> 1.8.3.1
> 


-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[toc] | [prev] | [next] | [standalone]

#1409142

  Hi,

> - add a small core function to registerr HOT_X/HOT_Y for a (cursor) plane,
>   e.g. drm_plane_register_hotspot(). That should allocate the properties
>   (if they don't exist yet) and then attach those props to the cursor. We
>   don't want those props everywhere, but only on drivers that support/need
>   them, aka virtual hw.

Hmm, why is this special to virtual hw?

>  	if (crtc->cursor) {
> -		ret = drm_mode_cursor_universal(crtc, req, file_priv);
> +		if (drm_core_check_feature(DRIVER_ATOMIC))
> +			ret = drm_mode_cursor_atomic(crtc, req, file_priv);
> +		else
> +			ret = drm_mode_cursor_universal(crtc, req, file_priv);
>  		goto out;

>   drm_mode_cursor_atomic would simply be a fusing of
>   drm_mode_cursor_universal + drm_atomic_helper_update_plane (dump all the
>   intermediate variables and store directly in the plane state), with the
>   addition of also storing hot_x/y into the plane state.

Hmm, that'll either make drm_mode_cursor_atomic a big cut+pasted
function, or need quite some refactoring to move common code into
functions callable from both drm_mode_cursor_atomic
+drm_mode_cursor_universal ...

Why attach the hotspot to the plane?  Wouldn't it make more sense to
make it a framebuffer property?

cheers,
  Gerd

[toc] | [prev] | [next] | [standalone]

#1409184

On Mon, May 30, 2016 at 03:50:36PM +0200, Gerd Hoffmann wrote:
>   Hi,
> 
> > - add a small core function to registerr HOT_X/HOT_Y for a (cursor) plane,
> >   e.g. drm_plane_register_hotspot(). That should allocate the properties
> >   (if they don't exist yet) and then attach those props to the cursor. We
> >   don't want those props everywhere, but only on drivers that support/need
> >   them, aka virtual hw.
> 
> Hmm, why is this special to virtual hw?
> 
> >  	if (crtc->cursor) {
> > -		ret = drm_mode_cursor_universal(crtc, req, file_priv);
> > +		if (drm_core_check_feature(DRIVER_ATOMIC))
> > +			ret = drm_mode_cursor_atomic(crtc, req, file_priv);
> > +		else
> > +			ret = drm_mode_cursor_universal(crtc, req, file_priv);
> >  		goto out;
> 
> >   drm_mode_cursor_atomic would simply be a fusing of
> >   drm_mode_cursor_universal + drm_atomic_helper_update_plane (dump all the
> >   intermediate variables and store directly in the plane state), with the
> >   addition of also storing hot_x/y into the plane state.
> 
> Hmm, that'll either make drm_mode_cursor_atomic a big cut+pasted
> function, or need quite some refactoring to move common code into
> functions callable from both drm_mode_cursor_atomic
> +drm_mode_cursor_universal ...
> 
> Why attach the hotspot to the plane?  Wouldn't it make more sense to
> make it a framebuffer property?

We don't have properties on the framebuffer. I guess you /could/ just add
it internally to struct drm_framebuffer, and not bother exposing to
userspace. I guess that would be a lot simpler, but it also means that
atomic userspace can't use hotspots before we add properties to fbs. And
doing that is a bit tricky since drm_framebuffer objects are meant to be
invariant - this assumption is deeply in-grained into the code all over
the place, everything just compares pointers when semantically it means to
compare the entire fb (including backing storage pointer/offsets and
everything).

So would be a bit more work to wire up for atomic userspace, but indeed a
lot less work to implement. I'm totally happy if you go with that tradeoff
;-)

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[toc] | [prev] | [next] | [standalone]

#1409858

  Hi,

> > Why attach the hotspot to the plane?  Wouldn't it make more sense to
> > make it a framebuffer property?
> 
> We don't have properties on the framebuffer. I guess you /could/ just add
> it internally to struct drm_framebuffer, and not bother exposing to
> userspace. I guess that would be a lot simpler,

Yes.  I can simply stick the hotspot into drm_framebuffer in
drm_mode_cursor_universal() and pick up the values in the driver's plane
update function.

>  but it also means that
> atomic userspace can't use hotspots before we add properties to fbs. And
> doing that is a bit tricky since drm_framebuffer objects are meant to be
> invariant - this assumption is deeply in-grained into the code all over
> the place, everything just compares pointers when semantically it means to
> compare the entire fb (including backing storage pointer/offsets and
> everything).

Hmm, the hotspot location for a given cursor image is invariant too, so
I don't think that would be a big issue.

I'd expect userspace define a bunch of cursors, then switch between them
(instead of defining a single cursor, then constantly updating it).

cheers,
  Gerd

[toc] | [prev] | [next] | [standalone]

#1409910

On Tue, May 31, 2016 at 8:29 AM, Gerd Hoffmann <kraxel@redhat.com> wrote:
>>  but it also means that
>> atomic userspace can't use hotspots before we add properties to fbs. And
>> doing that is a bit tricky since drm_framebuffer objects are meant to be
>> invariant - this assumption is deeply in-grained into the code all over
>> the place, everything just compares pointers when semantically it means to
>> compare the entire fb (including backing storage pointer/offsets and
>> everything).
>
> Hmm, the hotspot location for a given cursor image is invariant too, so
> I don't think that would be a big issue.
>
> I'd expect userspace define a bunch of cursors, then switch between them
> (instead of defining a single cursor, then constantly updating it).

Agreed, conceptually it would be a really nice fit to put the hotspot
into the invariant drm_framebuffer. I just meant to say that you need
to add a bit of new uapi to make it happen, since we can't reuse our
existing get/set_prop functions (since those only change props after
the object is created). And we also can't use our existing ioctls to
enumerate properties of an object (since chicken-egg: you want the
list of properties before creating a new framebuffer, so can't use
that framebuffer to enumerate them).

But really not a big concern.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[toc] | [prev] | [next] | [standalone]

#1409914

[Multipart message — attachments visible in raw view] — view raw

On Tue, 31 May 2016 08:29:20 +0200
Gerd Hoffmann <kraxel@redhat.com> wrote:

>   Hi,
> 
> > > Why attach the hotspot to the plane?  Wouldn't it make more sense to
> > > make it a framebuffer property?  
> > 
> > We don't have properties on the framebuffer. I guess you /could/ just add
> > it internally to struct drm_framebuffer, and not bother exposing to
> > userspace. I guess that would be a lot simpler,  
> 
> Yes.  I can simply stick the hotspot into drm_framebuffer in
> drm_mode_cursor_universal() and pick up the values in the driver's plane
> update function.
> 
> >  but it also means that
> > atomic userspace can't use hotspots before we add properties to fbs. And
> > doing that is a bit tricky since drm_framebuffer objects are meant to be
> > invariant - this assumption is deeply in-grained into the code all over
> > the place, everything just compares pointers when semantically it means to
> > compare the entire fb (including backing storage pointer/offsets and
> > everything).  
> 
> Hmm, the hotspot location for a given cursor image is invariant too, so
> I don't think that would be a big issue.
> 
> I'd expect userspace define a bunch of cursors, then switch between them
> (instead of defining a single cursor, then constantly updating it).

Except updating a single cursor (well, two alternating buffers) is
exactly what Weston does, since there is no "set of cursors". On
Wayland, a cursor is just a regular surface like any other with
arbitrary content from a client, except it happens to be associated
with a pointer device.

Furthermore, in Weston a cursor plane is not special in any way. *Any*
client surface can go on the cursor plane if it fits. Universal planes,
and all that.

That's one existing userspace. I suppose that is sub-optimal for
virtual drivers, isn't it? But what else could Weston do without having
separate paths for "normal DRM" vs. "virtual DRM"?


Thanks,
pq

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web