[PATCH 01/11] drm: add plane support

[Jesse Barnes]

On Wed, 26 Oct 2011 14:40:07 +0900
Joonyoung Shim  wrote:

> 10/25/2011 06:46 PM, Jesse Barnes ? ?:
> 
> [snip]
> > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> > index 8020798..d7f03aa 100644
> > --- a/include/drm/drm_crtc.h
> > +++ b/include/drm/drm_crtc.h
> > @@ -44,6 +44,7 @@ struct drm_framebuffer;
> >   #define DRM_MODE_OBJECT_PROPERTY 0xb0b0b0b0
> >   #define DRM_MODE_OBJECT_FB 0xfbfbfbfb
> >   #define DRM_MODE_OBJECT_BLOB 0x
> > +#define DRM_MODE_OBJECT_PLANE 0x
> >
> >   struct drm_mode_object {
> > uint32_t id;
> > @@ -278,6 +279,7 @@ struct drm_crtc;
> >   struct drm_connector;
> >   struct drm_encoder;
> >   struct drm_pending_vblank_event;
> > +struct drm_plane;
> >
> >   /**
> >* drm_crtc_funcs - control CRTCs for a given device
> > @@ -536,6 +538,58 @@ struct drm_connector {
> >   };
> >
> >   /**
> > + * drm_plane_funcs - driver plane control functions
> > + * @update_plane: update the plane configuration
> > + */
> > +struct drm_plane_funcs {
> > +   int (*update_plane)(struct drm_plane *plane,
> > +   struct drm_crtc *crtc, struct drm_framebuffer *fb,
> > +   int crtc_x, int crtc_y,
> > +   unsigned int crtc_w, unsigned int crtc_h,
> > +   uint32_t src_x, uint32_t src_y,
> > +   uint32_t src_w, uint32_t src_h);
> > +   int (*disable_plane)(struct drm_plane *plane);
> > +};
> 
> How about add destroy() function and call it in
> drm_mode_config_cleanup()?

Oh good idea; destroy will be needed for hot plug.

-- 
Jesse Barnes, Intel Open Source Technology Center
-- next part --
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: 

Re: [PATCH 01/11] drm: add plane support

[Jesse Barnes]

On Wed, 26 Oct 2011 14:40:07 +0900
Joonyoung Shim jy0922.s...@samsung.com wrote:

 10/25/2011 06:46 PM, Jesse Barnes 쓴 글:
 
 [snip]
  diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
  index 8020798..d7f03aa 100644
  --- a/include/drm/drm_crtc.h
  +++ b/include/drm/drm_crtc.h
  @@ -44,6 +44,7 @@ struct drm_framebuffer;
#define DRM_MODE_OBJECT_PROPERTY 0xb0b0b0b0
#define DRM_MODE_OBJECT_FB 0xfbfbfbfb
#define DRM_MODE_OBJECT_BLOB 0x
  +#define DRM_MODE_OBJECT_PLANE 0x
 
struct drm_mode_object {
  uint32_t id;
  @@ -278,6 +279,7 @@ struct drm_crtc;
struct drm_connector;
struct drm_encoder;
struct drm_pending_vblank_event;
  +struct drm_plane;
 
/**
 * drm_crtc_funcs - control CRTCs for a given device
  @@ -536,6 +538,58 @@ struct drm_connector {
};
 
/**
  + * drm_plane_funcs - driver plane control functions
  + * @update_plane: update the plane configuration
  + */
  +struct drm_plane_funcs {
  +   int (*update_plane)(struct drm_plane *plane,
  +   struct drm_crtc *crtc, struct drm_framebuffer *fb,
  +   int crtc_x, int crtc_y,
  +   unsigned int crtc_w, unsigned int crtc_h,
  +   uint32_t src_x, uint32_t src_y,
  +   uint32_t src_w, uint32_t src_h);
  +   int (*disable_plane)(struct drm_plane *plane);
  +};
 
 How about add destroy() function and call it in
 drm_mode_config_cleanup()?

Oh good idea; destroy will be needed for hot plug.

-- 
Jesse Barnes, Intel Open Source Technology Center


signature.asc
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 01/11] drm: add plane support

[Joonyoung Shim]

10/25/2011 06:46 PM, Jesse Barnes ? ?:

[snip]
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 8020798..d7f03aa 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -44,6 +44,7 @@ struct drm_framebuffer;
>   #define DRM_MODE_OBJECT_PROPERTY 0xb0b0b0b0
>   #define DRM_MODE_OBJECT_FB 0xfbfbfbfb
>   #define DRM_MODE_OBJECT_BLOB 0x
> +#define DRM_MODE_OBJECT_PLANE 0x
>
>   struct drm_mode_object {
>   uint32_t id;
> @@ -278,6 +279,7 @@ struct drm_crtc;
>   struct drm_connector;
>   struct drm_encoder;
>   struct drm_pending_vblank_event;
> +struct drm_plane;
>
>   /**
>* drm_crtc_funcs - control CRTCs for a given device
> @@ -536,6 +538,58 @@ struct drm_connector {
>   };
>
>   /**
> + * drm_plane_funcs - driver plane control functions
> + * @update_plane: update the plane configuration
> + */
> +struct drm_plane_funcs {
> + int (*update_plane)(struct drm_plane *plane,
> + struct drm_crtc *crtc, struct drm_framebuffer *fb,
> + int crtc_x, int crtc_y,
> + unsigned int crtc_w, unsigned int crtc_h,
> + uint32_t src_x, uint32_t src_y,
> + uint32_t src_w, uint32_t src_h);
> + int (*disable_plane)(struct drm_plane *plane);
> +};

How about add destroy() function and call it in
drm_mode_config_cleanup()?

[PATCH 01/11] drm: add plane support

[Joonyoung Shim]

10/25/2011 08:18 PM, Jesse Barnes ? ?:
> On Tue, 25 Oct 2011 19:53:02 +0900
> Joonyoung Shim  wrote:
>>> +/**
>>> + * drm_plane - central DRM plane control structure
>>> + * @dev: DRM device this plane belongs to
>>> + * @kdev: kernel device
>>> + * @attr: kdev attributes
>>> + * @head: for list management
>>> + * @base: base mode object
>>> + * @crtc_x: x position of plane (relative to pipe base)
>>> + * @crtc_y: y position of plane
>>> + * @x: x offset into fb
>>> + * @y: y offset into fb
>> Above 4 members don't be used.
> Oops yeah, I'll fix up the comments.
>
>>> +
>>> +struct drm_mode_get_plane {
>>> +   __u64 format_type_ptr;
>>> +   __u32 plane_id;
>>> +
>>> +   __u32 crtc_id;
>>> +   __u32 fb_id;
>>> +
>>> +   __u32 possible_crtcs;
>>> +   __u32 gamma_size;
>>> +
>>> +   __u32 count_format_types;
>>> +};
>> I wonder why doesn't give to user crtc_x, crtc_y, crtc_w, crtc_h
>> information?
> It could, but the caller should already know was my thinking.  Would
> you like those bits returned?

I saw such codes in initial your RFC patch version so i just have
wondered why it is removed. It can be added later if needs.

Thanks.

[PATCH 01/11] drm: add plane support

[Daniel Vetter]

On Tue, Oct 25, 2011 at 11:43:09AM -0500, Rob Clark wrote:
> On Tue, Oct 25, 2011 at 9:09 AM, Jesse Barnes  
> wrote:
> > diff --git a/include/drm/drm_mode.h b/include/drm/drm_mode.h
> > index 34a0d22..dafe8df 100644
> > --- a/include/drm/drm_mode.h
> > +++ b/include/drm/drm_mode.h
> > @@ -272,8 +272,9 @@ struct drm_mode_fb_cmd2 {
> > ? ? ? ?__u32 bpp;
> > ? ? ? ?__u32 depth;
> > ? ? ? ?__u32 pixel_format; /* fourcc code from videodev2.h */
> > - ? ? ? /* driver specific handle */
> > - ? ? ? __u32 handle;
> > + ? ? ? __u32 handle_count;
> > + ? ? ? /* driver specific buffer object handle array */
> > + ? ? ? __u64 handles;
> > ?};
> 
> 
> Ok, after a bit of discussion with Jakob on IRC, this is what we arrived at:
> 
> 1) It would be nice to have the option for multi-planar formats to be
> able to use one single buffer object, or one buffer object per plane.
> In the case of one buffer per plane, the order is dictated by the
> fourcc.
> 
> 2) We do need per-plane stride for some formats, in particular I420
> which is a bit ambiguous otherwise (ie. is the stride of the U and V
> planes half the stride as the Y plane, or the same?)
> 
> 3) Maybe we need per-plane offsets, but I think this case could be
> handled by just using one bo per plane, so left it out
> 
> 4) bpp/depth are redundant w/ the pixel_format. Just in case, as a
> future placeholder, and inspired by 'struct v4l2_pix_format', add a
> priv field.  Although making the field big enough to hold a pointer if
> absolutely really needed.
> 
> struct drm_mode_fb_cmd2 {
>   __u32 fb_id;
>   __u32 width, height;
>   __u32 pixel_format; /* fourcc code from videodev2.h */
>   __u64 priv;  /* private data, depends on pixelformat */
> 
>   /* in case of planar formats, either one buffer object,
>* or one buffer object per plane, is allowed.  In the
>* case per-plane bo's, the order is dictated by the
>* fourcc.. ie. NV12 (http://fourcc.org/yuv.php#NV12)
>* is described as:
>*
>*   YUV 4:2:0 image with a plane of 8 bit Y samples
>*   followed by an interleaved U/V plane containing
>*   8 bit 2x2 subsampled colour difference samples.
>*
>* So it would consist of Y as first buffer and UV as
>* second buffer.  In the case that only a single bo
>* is used then buffer[1].handle should be zero.  (But
>* a plane specific pitch could still be specified.)
>*/
>   struct {
> __u32 pitch;
> /* driver specific handle */
> __u32 handle;

Why not just add a
__u32 offset;
__u32 rsvd;
and call it a day. This thing is pretty big already, so that bloat doesn't
matter that much. Maybe spec that buffer[0].offset must be zero. With that
you can also do I420 with the following layout
+---+
|YYY|
|YYY|
|YYY|
|YYY|
+---+---+
|UUU|VVV|
|UUU|VVV|
+---+---+

i.e. stride_U == stride_V, with their lines meshed into one line.
-Daniel

>   } buffer[16];
>  };
> 
> 
> BR,
> -R
> ___
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Mail: daniel at ffwll.ch
Mobile: +41 (0)79 365 57 48

[PATCH 01/11] drm: add plane support

[Joonyoung Shim]

10/25/2011 06:46 PM, Jesse Barnes ? ?:
> Planes are a bit like half-CRTCs.  They have a location and fb, but
> don't drive outputs directly.  Add support for handling them to the core
> KMS code.
>
> Signed-off-by: Jesse Barnes
> ---
>   drivers/gpu/drm/drm_crtc.c |  236 
> +++-
>   drivers/gpu/drm/drm_drv.c  |3 +
>   include/drm/drm.h  |3 +
>   include/drm/drm_crtc.h |   71 +-
>   include/drm/drm_mode.h |   33 ++
>   5 files changed, 343 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index fe738f0..0e129b1 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -535,6 +535,48 @@ void drm_encoder_cleanup(struct drm_encoder *encoder)
>   }
>   EXPORT_SYMBOL(drm_encoder_cleanup);
>
> +void drm_plane_init(struct drm_device *dev, struct drm_plane *plane,
> + unsigned long possible_crtcs,
> + const struct drm_plane_funcs *funcs,
> + uint32_t *formats, uint32_t format_count)
> +{
> + mutex_lock(>mode_config.mutex);
> +
> + plane->dev = dev;
> + drm_mode_object_get(dev,>base, DRM_MODE_OBJECT_PLANE);
> + plane->funcs = funcs;
> + plane->format_types = kmalloc(sizeof(uint32_t) * format_count,
> +   GFP_KERNEL);
> + if (!plane->format_types) {
> + DRM_DEBUG_KMS("out of memory when allocating plane\n");
> + drm_mode_object_put(dev,>base);
> + return;
> + }
> +
> + memcpy(plane->format_types, formats, format_count);
> + plane->format_count = format_count;
> + plane->possible_crtcs = possible_crtcs;
> +
> + list_add_tail(>head,>mode_config.plane_list);
> + dev->mode_config.num_plane++;
> +
> + mutex_unlock(>mode_config.mutex);
> +}
> +EXPORT_SYMBOL(drm_plane_init);
> +
> +void drm_plane_cleanup(struct drm_plane *plane)
> +{
> + struct drm_device *dev = plane->dev;
> +
> + mutex_lock(>mode_config.mutex);
> + kfree(plane->format_types);
> + drm_mode_object_put(dev,>base);
> + list_del(>head);
> + dev->mode_config.num_plane--;
> + mutex_unlock(>mode_config.mutex);
> +}
> +EXPORT_SYMBOL(drm_plane_cleanup);
> +
>   /**
>* drm_mode_create - create a new display mode
>* @dev: DRM device
> @@ -866,6 +908,7 @@ void drm_mode_config_init(struct drm_device *dev)
>   INIT_LIST_HEAD(>mode_config.encoder_list);
>   INIT_LIST_HEAD(>mode_config.property_list);
>   INIT_LIST_HEAD(>mode_config.property_blob_list);
> + INIT_LIST_HEAD(>mode_config.plane_list);
>   idr_init(>mode_config.crtc_idr);
>
>   mutex_lock(>mode_config.mutex);
> @@ -1466,6 +1509,193 @@ out:
>   }
>
>   /**
> + * drm_mode_getplane_res - get plane info
> + * @dev: DRM device
> + * @data: ioctl data
> + * @file_priv: DRM file info
> + *
> + * Return an plane count and set of IDs.
> + */
> +int drm_mode_getplane_res(struct drm_device *dev, void *data,
> + struct drm_file *file_priv)
> +{
> + struct drm_mode_get_plane_res *plane_resp = data;
> + struct drm_mode_config *config;
> + struct drm_plane *plane;
> + uint32_t __user *plane_ptr;
> + int copied = 0, ret = 0;
> +
> + if (!drm_core_check_feature(dev, DRIVER_MODESET))
> + return -EINVAL;
> +
> + mutex_lock(>mode_config.mutex);
> + config =>mode_config;
> +
> + /*
> +  * This ioctl is called twice, once to determine how much space is
> +  * needed, and the 2nd time to fill it.
> +  */
> + if (config->num_plane&&
> + (plane_resp->count_planes>= config->num_plane)) {
> + plane_ptr = (uint32_t *)(unsigned long)plane_resp->plane_id_ptr;
> +
> + list_for_each_entry(plane,>plane_list, head) {
> + if (put_user(plane->base.id, plane_ptr + copied)) {
> + ret = -EFAULT;
> + goto out;
> + }
> + copied++;
> + }
> + }
> + plane_resp->count_planes = config->num_plane;
> +
> +out:
> + mutex_unlock(>mode_config.mutex);
> + return ret;
> +}
> +
> +/**
> + * drm_mode_getplane - get plane info
> + * @dev: DRM device
> + * @data: ioctl data
> + * @file_priv: DRM file info
> + *
> + * Return plane info, including formats supported, gamma size, any
> + * current fb, etc.
> + */
> +int drm_mode_getplane(struct drm_device *dev, void *data,
> + struct drm_file *file_priv)
> +{
> + struct drm_mode_get_plane *plane_resp = data;
> + struct drm_mode_object *obj;
> + struct drm_plane *plane;
> + uint32_t __user *format_ptr;
> + int ret = 0;
> +
> + if (!drm_core_check_feature(dev, DRIVER_MODESET))
> + return -EINVAL;
> +
> + mutex_lock(>mode_config.mutex);
> + obj = drm_mode_object_find(dev, plane_resp->plane_id,
> + 

[PATCH 01/11] drm: add plane support

[Jesse Barnes]

Here's a diff I can roll in if it looks ok.  It adds the ability to
specify multiple handles for a single fb to better accommodate planar
configs.  I think Rob has convinced me that this is a good idea...
comments appreciated.

Thanks,
Jesse

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index a30b9d4..0cc2077 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -1923,7 +1923,8 @@ int drm_mode_addfb(struct drm_device *dev,
r.bpp = or->bpp;
r.depth = or->depth;
r.pixel_format = 0;
-   r.handle = or->handle;
+   r.handle_count = 1;
+   r.handles = (u64)>handle;

if (!drm_core_check_feature(dev, DRIVER_MODESET))
return -EINVAL;
diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index cd7e04d..2c7f200 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -7619,8 +7619,9 @@ intel_user_framebuffer_create(struct drm_device *dev,
  struct drm_mode_fb_cmd2 *mode_cmd)
 {
struct drm_i915_gem_object *obj;
+   u32 *handles = (u32 *)mode_cmd->handles;

-   obj = to_intel_bo(drm_gem_object_lookup(dev, filp, mode_cmd->handle));
+   obj = to_intel_bo(drm_gem_object_lookup(dev, filp, handles[0]));
if (>base == NULL)
return ERR_PTR(-ENOENT);

diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c 
b/drivers/gpu/drm/nouveau/nouveau_display.c
index 7a428a9..cb9b868 100644
--- a/drivers/gpu/drm/nouveau/nouveau_display.c
+++ b/drivers/gpu/drm/nouveau/nouveau_display.c
@@ -128,9 +128,10 @@ nouveau_user_framebuffer_create(struct drm_device *dev,
 {
struct nouveau_framebuffer *nouveau_fb;
struct drm_gem_object *gem;
+   u32 *handles = (u32 *)mode_cmd->handles;
int ret;

-   gem = drm_gem_object_lookup(dev, file_priv, mode_cmd->handle);
+   gem = drm_gem_object_lookup(dev, file_priv, handles);
if (!gem)
return ERR_PTR(-ENOENT);

diff --git a/drivers/gpu/drm/radeon/radeon_display.c 
b/drivers/gpu/drm/radeon/radeon_display.c
index ae803f8..63a6d91 100644
--- a/drivers/gpu/drm/radeon/radeon_display.c
+++ b/drivers/gpu/drm/radeon/radeon_display.c
@@ -1128,11 +1128,12 @@ radeon_user_framebuffer_create(struct drm_device *dev,
 {
struct drm_gem_object *obj;
struct radeon_framebuffer *radeon_fb;
+   u32 *handles = (u32 *)mode_cmd->handles;

-   obj = drm_gem_object_lookup(dev, file_priv, mode_cmd->handle);
+   obj = drm_gem_object_lookup(dev, file_priv, handles[0]);
if (obj ==  NULL) {
dev_err(>pdev->dev, "No GEM object associated to handle 
0x%08X, "
-   "can't create framebuffer\n", mode_cmd->handle);
+   "can't create framebuffer\n", handles[0]);
return ERR_PTR(-ENOENT);
}

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c 
b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
index 2a1b802..0ad7456 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
@@ -866,7 +866,7 @@ static struct drm_framebuffer *vmw_kms_fb_create(struct 
drm_device *dev,
 */

ret = vmw_user_surface_lookup_handle(dev_priv, tfile,
-mode_cmd->handle, );
+mode_cmd->handles[0], );
if (ret)
goto try_dmabuf;

diff --git a/drivers/staging/gma500/framebuffer.c 
b/drivers/staging/gma500/framebuffer.c
index 85f47d5..ee91ffe 100644
--- a/drivers/staging/gma500/framebuffer.c
+++ b/drivers/staging/gma500/framebuffer.c
@@ -496,7 +496,7 @@ static struct drm_framebuffer *psb_user_framebuffer_create
 *  Find the GEM object and thus the gtt range object that is
 *  to back this space
 */
-   obj = drm_gem_object_lookup(dev, filp, cmd->handle);
+   obj = drm_gem_object_lookup(dev, filp, cmd->handles[0]);
if (obj == NULL)
return ERR_PTR(-ENOENT);

diff --git a/include/drm/drm_mode.h b/include/drm/drm_mode.h
index 34a0d22..dafe8df 100644
--- a/include/drm/drm_mode.h
+++ b/include/drm/drm_mode.h
@@ -272,8 +272,9 @@ struct drm_mode_fb_cmd2 {
__u32 bpp;
__u32 depth;
__u32 pixel_format; /* fourcc code from videodev2.h */
-   /* driver specific handle */
-   __u32 handle;
+   __u32 handle_count;
+   /* driver specific buffer object handle array */
+   __u64 handles;
 };

 #define DRM_MODE_FB_DIRTY_ANNOTATE_COPY 0x01

[PATCH 01/11] drm: add plane support

[Daniel Vetter]

On Tue, Oct 25, 2011 at 02:26:07PM +0100, Alan Cox wrote:
> > As discussed with Jesse on irc, drm fb handling is fragile. Current rules:
> > - fbs are not reference counted, hence when destroying we need to disable
> >   all crtcs (and now also planes) that use them. drm_framebuffer_cleanup
> >   does that atm
> > - drivers that hold onto fbs after the kms core drops the corresponding
> >   pointer needs to hold a ref onto the underlying backing storage (like
> >   e.g. for pageflip on the to-be-flipped-out fb as long as it might still
> >   be scanned out).
> > 
> > We need proper refcounting for these ... But for now this patch is missing
> > the plane cleanup in drm_framebuffer_cleanup.
> 
> I'd rather we fixed the framebuffer kref stuff as part of doing this
> rather than have a poorer API because of something we have to fix anyway.

Imo we should do things piece by piece. Fixing up drm fb refcounting is
imo a rather low-prio thing for core drm. And I've already asked Rob
Clarke whether he can clean up some of the confiusion in the kms
framebuffer->destroy() functions.

> It shouldn't be *that* hard to fix, at least for this kind of use
> case. Resize locking, fb moving etc are ugly issues, refcount shouldn't
> be, and the tty layer also refcounts so we can only have the fb objects
> themselves to worry about as we can defer fb destruction to tty close or

I'm talking solely about kms framebuffers. I.e. completely orthogonal to
any issues you're seeing wrt kms<->linux fb subsystem integration. Or I'm
completely misunderstanding you here ...
-Daniel
-- 
Daniel Vetter
Mail: daniel at ffwll.ch
Mobile: +41 (0)79 365 57 48

[PATCH 01/11] drm: add plane support

[Jesse Barnes]

On Tue, 25 Oct 2011 14:26:07 +0100
Alan Cox  wrote:

> > As discussed with Jesse on irc, drm fb handling is fragile. Current
> > rules:
> > - fbs are not reference counted, hence when destroying we need to
> > disable all crtcs (and now also planes) that use them.
> > drm_framebuffer_cleanup does that atm
> > - drivers that hold onto fbs after the kms core drops the
> > corresponding pointer needs to hold a ref onto the underlying
> > backing storage (like e.g. for pageflip on the to-be-flipped-out fb
> > as long as it might still be scanned out).
> > 
> > We need proper refcounting for these ... But for now this patch is
> > missing the plane cleanup in drm_framebuffer_cleanup.
> 
> I'd rather we fixed the framebuffer kref stuff as part of doing this
> rather than have a poorer API because of something we have to fix
> anyway.
> 
> It shouldn't be *that* hard to fix, at least for this kind of use
> case. Resize locking, fb moving etc are ugly issues, refcount
> shouldn't be, and the tty layer also refcounts so we can only have
> the fb objects themselves to worry about as we can defer fb
> destruction to tty close or drm last unref even for those with
> consoles on them.

Oh it doesn't affect the userspace API so I don't think it's urgent.
But I agree, it would be nice to clean up fb management a bit...  Any
volunteers? :)

Thanks,
Jesse

[PATCH 01/11] drm: add plane support

[Rob Clark]

On Tue, Oct 25, 2011 at 2:41 PM, Daniel Vetter  wrote:
> On Tue, Oct 25, 2011 at 11:43:09AM -0500, Rob Clark wrote:
>> On Tue, Oct 25, 2011 at 9:09 AM, Jesse Barnes  
>> wrote:
>> > diff --git a/include/drm/drm_mode.h b/include/drm/drm_mode.h
>> > index 34a0d22..dafe8df 100644
>> > --- a/include/drm/drm_mode.h
>> > +++ b/include/drm/drm_mode.h
>> > @@ -272,8 +272,9 @@ struct drm_mode_fb_cmd2 {
>> > ? ? ? ?__u32 bpp;
>> > ? ? ? ?__u32 depth;
>> > ? ? ? ?__u32 pixel_format; /* fourcc code from videodev2.h */
>> > - ? ? ? /* driver specific handle */
>> > - ? ? ? __u32 handle;
>> > + ? ? ? __u32 handle_count;
>> > + ? ? ? /* driver specific buffer object handle array */
>> > + ? ? ? __u64 handles;
>> > ?};
>>
>>
>> Ok, after a bit of discussion with Jakob on IRC, this is what we arrived at:
>>
>> 1) It would be nice to have the option for multi-planar formats to be
>> able to use one single buffer object, or one buffer object per plane.
>> In the case of one buffer per plane, the order is dictated by the
>> fourcc.
>>
>> 2) We do need per-plane stride for some formats, in particular I420
>> which is a bit ambiguous otherwise (ie. is the stride of the U and V
>> planes half the stride as the Y plane, or the same?)
>>
>> 3) Maybe we need per-plane offsets, but I think this case could be
>> handled by just using one bo per plane, so left it out
>>
>> 4) bpp/depth are redundant w/ the pixel_format. Just in case, as a
>> future placeholder, and inspired by 'struct v4l2_pix_format', add a
>> priv field. ?Although making the field big enough to hold a pointer if
>> absolutely really needed.
>>
>> struct drm_mode_fb_cmd2 {
>> ? __u32 fb_id;
>> ? __u32 width, height;
>> ? __u32 pixel_format; /* fourcc code from videodev2.h */
>> ? __u64 priv; ?/* private data, depends on pixelformat */
>>
>> ? /* in case of planar formats, either one buffer object,
>> ? ?* or one buffer object per plane, is allowed. ?In the
>> ? ?* case per-plane bo's, the order is dictated by the
>> ? ?* fourcc.. ie. NV12 (http://fourcc.org/yuv.php#NV12)
>> ? ?* is described as:
>> ? ?*
>> ? ?* ? YUV 4:2:0 image with a plane of 8 bit Y samples
>> ? ?* ? followed by an interleaved U/V plane containing
>> ? ?* ? 8 bit 2x2 subsampled colour difference samples.
>> ? ?*
>> ? ?* So it would consist of Y as first buffer and UV as
>> ? ?* second buffer. ?In the case that only a single bo
>> ? ?* is used then buffer[1].handle should be zero. ?(But
>> ? ?* a plane specific pitch could still be specified.)
>> ? ?*/
>> ? struct {
>> ? ? __u32 pitch;
>> ? ? /* driver specific handle */
>> ? ? __u32 handle;
>
> Why not just add a
> ? ? ? ?__u32 offset;
> ? ? ? ?__u32 rsvd;
> and call it a day. This thing is pretty big already, so that bloat doesn't
> matter that much. Maybe spec that buffer[0].offset must be zero. With that
> you can also do I420 with the following layout
> +---+
> |YYY|
> |YYY|
> |YYY|
> |YYY|
> +---+---+
> |UUU|VVV|
> |UUU|VVV|
> +---+---+
>
> i.e. stride_U == stride_V, with their lines meshed into one line.
> -Daniel
>

I've no objection to an offset field.. I was mostly just trying to
avoid that we keep adding things until we get XvImageFormatValues..

We should either add an offset plus pad field as Daniel suggested, or
at least add a u64 reserved field which could be made into made into
an offset field later if needed.

OTOH, is the format you described *really* still I420?

BR,
-R

>> ? } buffer[16];
>> ?};
>>
>>
>> BR,
>> -R
>> ___
>> dri-devel mailing list
>> dri-devel at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Daniel Vetter
> Mail: daniel at ffwll.ch
> Mobile: +41 (0)79 365 57 48
> ___
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>

[PATCH 01/11] drm: add plane support

[Jesse Barnes]

On Tue, 25 Oct 2011 13:58:55 +0200
Daniel Vetter  wrote:

> On Tue, Oct 25, 2011 at 11:46:56AM +0200, Jesse Barnes wrote:
> > Planes are a bit like half-CRTCs.  They have a location and fb, but
> > don't drive outputs directly.  Add support for handling them to the
> > core KMS code.
> > 
> > Signed-off-by: Jesse Barnes 
> 
> As discussed with Jesse on irc, drm fb handling is fragile. Current
> rules:
> - fbs are not reference counted, hence when destroying we need to
> disable all crtcs (and now also planes) that use them.
> drm_framebuffer_cleanup does that atm
> - drivers that hold onto fbs after the kms core drops the
> corresponding pointer needs to hold a ref onto the underlying backing
> storage (like e.g. for pageflip on the to-be-flipped-out fb as long
> as it might still be scanned out).
> 
> We need proper refcounting for these ... But for now this patch is
> missing the plane cleanup in drm_framebuffer_cleanup.

Ah yeah that's a better place for the disable plane call I currently
have in the intel specific code...  I'll fix up and test.

> Otherwise I think going with just the src and dst rect for set_plane
> is about the only sensible thing given the crazy hw out there. But I
> lack the knowledge about that kind of hw (and video stuff in
> general), so I'll refrain from slapping my r-b on these two.

Yeah, I think the drivers just need to be able to calculate the scaling
level from the params and program them into whatever regs they happen
to have (on Intel fortunately the scaling is figured out by hw when we
program in the source & dest values, subject to some restrictions).


Thanks,
Jesse

[PATCH 01/11] drm: add plane support

[Alan Cox]

> As discussed with Jesse on irc, drm fb handling is fragile. Current rules:
> - fbs are not reference counted, hence when destroying we need to disable
>   all crtcs (and now also planes) that use them. drm_framebuffer_cleanup
>   does that atm
> - drivers that hold onto fbs after the kms core drops the corresponding
>   pointer needs to hold a ref onto the underlying backing storage (like
>   e.g. for pageflip on the to-be-flipped-out fb as long as it might still
>   be scanned out).
> 
> We need proper refcounting for these ... But for now this patch is missing
> the plane cleanup in drm_framebuffer_cleanup.

I'd rather we fixed the framebuffer kref stuff as part of doing this
rather than have a poorer API because of something we have to fix anyway.

It shouldn't be *that* hard to fix, at least for this kind of use
case. Resize locking, fb moving etc are ugly issues, refcount shouldn't
be, and the tty layer also refcounts so we can only have the fb objects
themselves to worry about as we can defer fb destruction to tty close or
drm last unref even for those with consoles on them.

Alan

[PATCH 01/11] drm: add plane support

[Daniel Vetter]

On Tue, Oct 25, 2011 at 11:46:56AM +0200, Jesse Barnes wrote:
> Planes are a bit like half-CRTCs.  They have a location and fb, but
> don't drive outputs directly.  Add support for handling them to the core
> KMS code.
> 
> Signed-off-by: Jesse Barnes 

As discussed with Jesse on irc, drm fb handling is fragile. Current rules:
- fbs are not reference counted, hence when destroying we need to disable
  all crtcs (and now also planes) that use them. drm_framebuffer_cleanup
  does that atm
- drivers that hold onto fbs after the kms core drops the corresponding
  pointer needs to hold a ref onto the underlying backing storage (like
  e.g. for pageflip on the to-be-flipped-out fb as long as it might still
  be scanned out).

We need proper refcounting for these ... But for now this patch is missing
the plane cleanup in drm_framebuffer_cleanup.

Otherwise I think going with just the src and dst rect for set_plane is
about the only sensible thing given the crazy hw out there. But I lack the
knowledge about that kind of hw (and video stuff in general), so I'll
refrain from slapping my r-b on these two.

Cheers, Daniel
-- 
Daniel Vetter
Mail: daniel at ffwll.ch
Mobile: +41 (0)79 365 57 48

[PATCH 01/11] drm: add plane support

[Jesse Barnes]

On Tue, 25 Oct 2011 19:53:02 +0900
Joonyoung Shim  wrote:
> > +/**
> > + * drm_plane - central DRM plane control structure
> > + * @dev: DRM device this plane belongs to
> > + * @kdev: kernel device
> > + * @attr: kdev attributes
> > + * @head: for list management
> > + * @base: base mode object
> > + * @crtc_x: x position of plane (relative to pipe base)
> > + * @crtc_y: y position of plane
> > + * @x: x offset into fb
> > + * @y: y offset into fb
> Above 4 members don't be used.

Oops yeah, I'll fix up the comments.

> > +
> > +struct drm_mode_get_plane {
> > +   __u64 format_type_ptr;
> > +   __u32 plane_id;
> > +
> > +   __u32 crtc_id;
> > +   __u32 fb_id;
> > +
> > +   __u32 possible_crtcs;
> > +   __u32 gamma_size;
> > +
> > +   __u32 count_format_types;
> > +};
> 
> I wonder why doesn't give to user crtc_x, crtc_y, crtc_w, crtc_h
> information?

It could, but the caller should already know was my thinking.  Would
you like those bits returned?

Jesse

[PATCH 01/11] drm: add plane support

[Jesse Barnes]

Planes are a bit like half-CRTCs.  They have a location and fb, but
don't drive outputs directly.  Add support for handling them to the core
KMS code.

Signed-off-by: Jesse Barnes 
---
 drivers/gpu/drm/drm_crtc.c |  236 +++-
 drivers/gpu/drm/drm_drv.c  |3 +
 include/drm/drm.h  |3 +
 include/drm/drm_crtc.h |   71 +-
 include/drm/drm_mode.h |   33 ++
 5 files changed, 343 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index fe738f0..0e129b1 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -535,6 +535,48 @@ void drm_encoder_cleanup(struct drm_encoder *encoder)
 }
 EXPORT_SYMBOL(drm_encoder_cleanup);

+void drm_plane_init(struct drm_device *dev, struct drm_plane *plane,
+   unsigned long possible_crtcs,
+   const struct drm_plane_funcs *funcs,
+   uint32_t *formats, uint32_t format_count)
+{
+   mutex_lock(>mode_config.mutex);
+
+   plane->dev = dev;
+   drm_mode_object_get(dev, >base, DRM_MODE_OBJECT_PLANE);
+   plane->funcs = funcs;
+   plane->format_types = kmalloc(sizeof(uint32_t) * format_count,
+ GFP_KERNEL);
+   if (!plane->format_types) {
+   DRM_DEBUG_KMS("out of memory when allocating plane\n");
+   drm_mode_object_put(dev, >base);
+   return;
+   }
+
+   memcpy(plane->format_types, formats, format_count);
+   plane->format_count = format_count;
+   plane->possible_crtcs = possible_crtcs;
+
+   list_add_tail(>head, >mode_config.plane_list);
+   dev->mode_config.num_plane++;
+
+   mutex_unlock(>mode_config.mutex);
+}
+EXPORT_SYMBOL(drm_plane_init);
+
+void drm_plane_cleanup(struct drm_plane *plane)
+{
+   struct drm_device *dev = plane->dev;
+
+   mutex_lock(>mode_config.mutex);
+   kfree(plane->format_types);
+   drm_mode_object_put(dev, >base);
+   list_del(>head);
+   dev->mode_config.num_plane--;
+   mutex_unlock(>mode_config.mutex);
+}
+EXPORT_SYMBOL(drm_plane_cleanup);
+
 /**
  * drm_mode_create - create a new display mode
  * @dev: DRM device
@@ -866,6 +908,7 @@ void drm_mode_config_init(struct drm_device *dev)
INIT_LIST_HEAD(>mode_config.encoder_list);
INIT_LIST_HEAD(>mode_config.property_list);
INIT_LIST_HEAD(>mode_config.property_blob_list);
+   INIT_LIST_HEAD(>mode_config.plane_list);
idr_init(>mode_config.crtc_idr);

mutex_lock(>mode_config.mutex);
@@ -1466,6 +1509,193 @@ out:
 }

 /**
+ * drm_mode_getplane_res - get plane info
+ * @dev: DRM device
+ * @data: ioctl data
+ * @file_priv: DRM file info
+ *
+ * Return an plane count and set of IDs.
+ */
+int drm_mode_getplane_res(struct drm_device *dev, void *data,
+   struct drm_file *file_priv)
+{
+   struct drm_mode_get_plane_res *plane_resp = data;
+   struct drm_mode_config *config;
+   struct drm_plane *plane;
+   uint32_t __user *plane_ptr;
+   int copied = 0, ret = 0;
+
+   if (!drm_core_check_feature(dev, DRIVER_MODESET))
+   return -EINVAL;
+
+   mutex_lock(>mode_config.mutex);
+   config = >mode_config;
+
+   /*
+* This ioctl is called twice, once to determine how much space is
+* needed, and the 2nd time to fill it.
+*/
+   if (config->num_plane &&
+   (plane_resp->count_planes >= config->num_plane)) {
+   plane_ptr = (uint32_t *)(unsigned long)plane_resp->plane_id_ptr;
+
+   list_for_each_entry(plane, >plane_list, head) {
+   if (put_user(plane->base.id, plane_ptr + copied)) {
+   ret = -EFAULT;
+   goto out;
+   }
+   copied++;
+   }
+   }
+   plane_resp->count_planes = config->num_plane;
+
+out:
+   mutex_unlock(>mode_config.mutex);
+   return ret;
+}
+
+/**
+ * drm_mode_getplane - get plane info
+ * @dev: DRM device
+ * @data: ioctl data
+ * @file_priv: DRM file info
+ *
+ * Return plane info, including formats supported, gamma size, any
+ * current fb, etc.
+ */
+int drm_mode_getplane(struct drm_device *dev, void *data,
+   struct drm_file *file_priv)
+{
+   struct drm_mode_get_plane *plane_resp = data;
+   struct drm_mode_object *obj;
+   struct drm_plane *plane;
+   uint32_t __user *format_ptr;
+   int ret = 0;
+
+   if (!drm_core_check_feature(dev, DRIVER_MODESET))
+   return -EINVAL;
+
+   mutex_lock(>mode_config.mutex);
+   obj = drm_mode_object_find(dev, plane_resp->plane_id,
+  DRM_MODE_OBJECT_PLANE);
+   if (!obj) {
+   ret = -EINVAL;
+   goto out;
+   }
+   plane = obj_to_plane(obj);
+
+   if (plane->crtc)
+  

[PATCH 01/11] drm: add plane support

[Rob Clark]

On Tue, Oct 25, 2011 at 9:09 AM, Jesse Barnes  
wrote:
> diff --git a/include/drm/drm_mode.h b/include/drm/drm_mode.h
> index 34a0d22..dafe8df 100644
> --- a/include/drm/drm_mode.h
> +++ b/include/drm/drm_mode.h
> @@ -272,8 +272,9 @@ struct drm_mode_fb_cmd2 {
> ? ? ? ?__u32 bpp;
> ? ? ? ?__u32 depth;
> ? ? ? ?__u32 pixel_format; /* fourcc code from videodev2.h */
> - ? ? ? /* driver specific handle */
> - ? ? ? __u32 handle;
> + ? ? ? __u32 handle_count;
> + ? ? ? /* driver specific buffer object handle array */
> + ? ? ? __u64 handles;
> ?};


Ok, after a bit of discussion with Jakob on IRC, this is what we arrived at:

1) It would be nice to have the option for multi-planar formats to be
able to use one single buffer object, or one buffer object per plane.
In the case of one buffer per plane, the order is dictated by the
fourcc.

2) We do need per-plane stride for some formats, in particular I420
which is a bit ambiguous otherwise (ie. is the stride of the U and V
planes half the stride as the Y plane, or the same?)

3) Maybe we need per-plane offsets, but I think this case could be
handled by just using one bo per plane, so left it out

4) bpp/depth are redundant w/ the pixel_format. Just in case, as a
future placeholder, and inspired by 'struct v4l2_pix_format', add a
priv field.  Although making the field big enough to hold a pointer if
absolutely really needed.

struct drm_mode_fb_cmd2 {
  __u32 fb_id;
  __u32 width, height;
  __u32 pixel_format; /* fourcc code from videodev2.h */
  __u64 priv;  /* private data, depends on pixelformat */

  /* in case of planar formats, either one buffer object,
   * or one buffer object per plane, is allowed.  In the
   * case per-plane bo's, the order is dictated by the
   * fourcc.. ie. NV12 (http://fourcc.org/yuv.php#NV12)
   * is described as:
   *
   *   YUV 4:2:0 image with a plane of 8 bit Y samples
   *   followed by an interleaved U/V plane containing
   *   8 bit 2x2 subsampled colour difference samples.
   *
   * So it would consist of Y as first buffer and UV as
   * second buffer.  In the case that only a single bo
   * is used then buffer[1].handle should be zero.  (But
   * a plane specific pitch could still be specified.)
   */
  struct {
__u32 pitch;
/* driver specific handle */
__u32 handle;
  } buffer[16];
 };


BR,
-R

[PATCH 01/11] drm: add plane support

[Jesse Barnes]

Planes are a bit like half-CRTCs.  They have a location and fb, but
don't drive outputs directly.  Add support for handling them to the core
KMS code.

Signed-off-by: Jesse Barnes jbar...@virtuousgeek.org
---
 drivers/gpu/drm/drm_crtc.c |  236 +++-
 drivers/gpu/drm/drm_drv.c  |3 +
 include/drm/drm.h  |3 +
 include/drm/drm_crtc.h |   71 +-
 include/drm/drm_mode.h |   33 ++
 5 files changed, 343 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index fe738f0..0e129b1 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -535,6 +535,48 @@ void drm_encoder_cleanup(struct drm_encoder *encoder)
 }
 EXPORT_SYMBOL(drm_encoder_cleanup);
 
+void drm_plane_init(struct drm_device *dev, struct drm_plane *plane,
+   unsigned long possible_crtcs,
+   const struct drm_plane_funcs *funcs,
+   uint32_t *formats, uint32_t format_count)
+{
+   mutex_lock(dev-mode_config.mutex);
+
+   plane-dev = dev;
+   drm_mode_object_get(dev, plane-base, DRM_MODE_OBJECT_PLANE);
+   plane-funcs = funcs;
+   plane-format_types = kmalloc(sizeof(uint32_t) * format_count,
+ GFP_KERNEL);
+   if (!plane-format_types) {
+   DRM_DEBUG_KMS(out of memory when allocating plane\n);
+   drm_mode_object_put(dev, plane-base);
+   return;
+   }
+
+   memcpy(plane-format_types, formats, format_count);
+   plane-format_count = format_count;
+   plane-possible_crtcs = possible_crtcs;
+
+   list_add_tail(plane-head, dev-mode_config.plane_list);
+   dev-mode_config.num_plane++;
+
+   mutex_unlock(dev-mode_config.mutex);
+}
+EXPORT_SYMBOL(drm_plane_init);
+
+void drm_plane_cleanup(struct drm_plane *plane)
+{
+   struct drm_device *dev = plane-dev;
+
+   mutex_lock(dev-mode_config.mutex);
+   kfree(plane-format_types);
+   drm_mode_object_put(dev, plane-base);
+   list_del(plane-head);
+   dev-mode_config.num_plane--;
+   mutex_unlock(dev-mode_config.mutex);
+}
+EXPORT_SYMBOL(drm_plane_cleanup);
+
 /**
  * drm_mode_create - create a new display mode
  * @dev: DRM device
@@ -866,6 +908,7 @@ void drm_mode_config_init(struct drm_device *dev)
INIT_LIST_HEAD(dev-mode_config.encoder_list);
INIT_LIST_HEAD(dev-mode_config.property_list);
INIT_LIST_HEAD(dev-mode_config.property_blob_list);
+   INIT_LIST_HEAD(dev-mode_config.plane_list);
idr_init(dev-mode_config.crtc_idr);
 
mutex_lock(dev-mode_config.mutex);
@@ -1466,6 +1509,193 @@ out:
 }
 
 /**
+ * drm_mode_getplane_res - get plane info
+ * @dev: DRM device
+ * @data: ioctl data
+ * @file_priv: DRM file info
+ *
+ * Return an plane count and set of IDs.
+ */
+int drm_mode_getplane_res(struct drm_device *dev, void *data,
+   struct drm_file *file_priv)
+{
+   struct drm_mode_get_plane_res *plane_resp = data;
+   struct drm_mode_config *config;
+   struct drm_plane *plane;
+   uint32_t __user *plane_ptr;
+   int copied = 0, ret = 0;
+
+   if (!drm_core_check_feature(dev, DRIVER_MODESET))
+   return -EINVAL;
+
+   mutex_lock(dev-mode_config.mutex);
+   config = dev-mode_config;
+
+   /*
+* This ioctl is called twice, once to determine how much space is
+* needed, and the 2nd time to fill it.
+*/
+   if (config-num_plane 
+   (plane_resp-count_planes = config-num_plane)) {
+   plane_ptr = (uint32_t *)(unsigned long)plane_resp-plane_id_ptr;
+
+   list_for_each_entry(plane, config-plane_list, head) {
+   if (put_user(plane-base.id, plane_ptr + copied)) {
+   ret = -EFAULT;
+   goto out;
+   }
+   copied++;
+   }
+   }
+   plane_resp-count_planes = config-num_plane;
+
+out:
+   mutex_unlock(dev-mode_config.mutex);
+   return ret;
+}
+
+/**
+ * drm_mode_getplane - get plane info
+ * @dev: DRM device
+ * @data: ioctl data
+ * @file_priv: DRM file info
+ *
+ * Return plane info, including formats supported, gamma size, any
+ * current fb, etc.
+ */
+int drm_mode_getplane(struct drm_device *dev, void *data,
+   struct drm_file *file_priv)
+{
+   struct drm_mode_get_plane *plane_resp = data;
+   struct drm_mode_object *obj;
+   struct drm_plane *plane;
+   uint32_t __user *format_ptr;
+   int ret = 0;
+
+   if (!drm_core_check_feature(dev, DRIVER_MODESET))
+   return -EINVAL;
+
+   mutex_lock(dev-mode_config.mutex);
+   obj = drm_mode_object_find(dev, plane_resp-plane_id,
+  DRM_MODE_OBJECT_PLANE);
+   if (!obj) {
+   ret = -EINVAL;
+   goto out;
+   

Re: [PATCH 01/11] drm: add plane support

[Joonyoung Shim]

10/25/2011 06:46 PM, Jesse Barnes 쓴 글:

Planes are a bit like half-CRTCs.  They have a location and fb, but
don't drive outputs directly.  Add support for handling them to the core
KMS code.

Signed-off-by: Jesse Barnesjbar...@virtuousgeek.org
---
  drivers/gpu/drm/drm_crtc.c |  236 +++-
  drivers/gpu/drm/drm_drv.c  |3 +
  include/drm/drm.h  |3 +
  include/drm/drm_crtc.h |   71 +-
  include/drm/drm_mode.h |   33 ++
  5 files changed, 343 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index fe738f0..0e129b1 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -535,6 +535,48 @@ void drm_encoder_cleanup(struct drm_encoder *encoder)
  }
  EXPORT_SYMBOL(drm_encoder_cleanup);

+void drm_plane_init(struct drm_device *dev, struct drm_plane *plane,
+   unsigned long possible_crtcs,
+   const struct drm_plane_funcs *funcs,
+   uint32_t *formats, uint32_t format_count)
+{
+   mutex_lock(dev-mode_config.mutex);
+
+   plane-dev = dev;
+   drm_mode_object_get(dev,plane-base, DRM_MODE_OBJECT_PLANE);
+   plane-funcs = funcs;
+   plane-format_types = kmalloc(sizeof(uint32_t) * format_count,
+ GFP_KERNEL);
+   if (!plane-format_types) {
+   DRM_DEBUG_KMS(out of memory when allocating plane\n);
+   drm_mode_object_put(dev,plane-base);
+   return;
+   }
+
+   memcpy(plane-format_types, formats, format_count);
+   plane-format_count = format_count;
+   plane-possible_crtcs = possible_crtcs;
+
+   list_add_tail(plane-head,dev-mode_config.plane_list);
+   dev-mode_config.num_plane++;
+
+   mutex_unlock(dev-mode_config.mutex);
+}
+EXPORT_SYMBOL(drm_plane_init);
+
+void drm_plane_cleanup(struct drm_plane *plane)
+{
+   struct drm_device *dev = plane-dev;
+
+   mutex_lock(dev-mode_config.mutex);
+   kfree(plane-format_types);
+   drm_mode_object_put(dev,plane-base);
+   list_del(plane-head);
+   dev-mode_config.num_plane--;
+   mutex_unlock(dev-mode_config.mutex);
+}
+EXPORT_SYMBOL(drm_plane_cleanup);
+
  /**
   * drm_mode_create - create a new display mode
   * @dev: DRM device
@@ -866,6 +908,7 @@ void drm_mode_config_init(struct drm_device *dev)
INIT_LIST_HEAD(dev-mode_config.encoder_list);
INIT_LIST_HEAD(dev-mode_config.property_list);
INIT_LIST_HEAD(dev-mode_config.property_blob_list);
+   INIT_LIST_HEAD(dev-mode_config.plane_list);
idr_init(dev-mode_config.crtc_idr);

mutex_lock(dev-mode_config.mutex);
@@ -1466,6 +1509,193 @@ out:
  }

  /**
+ * drm_mode_getplane_res - get plane info
+ * @dev: DRM device
+ * @data: ioctl data
+ * @file_priv: DRM file info
+ *
+ * Return an plane count and set of IDs.
+ */
+int drm_mode_getplane_res(struct drm_device *dev, void *data,
+   struct drm_file *file_priv)
+{
+   struct drm_mode_get_plane_res *plane_resp = data;
+   struct drm_mode_config *config;
+   struct drm_plane *plane;
+   uint32_t __user *plane_ptr;
+   int copied = 0, ret = 0;
+
+   if (!drm_core_check_feature(dev, DRIVER_MODESET))
+   return -EINVAL;
+
+   mutex_lock(dev-mode_config.mutex);
+   config =dev-mode_config;
+
+   /*
+* This ioctl is called twice, once to determine how much space is
+* needed, and the 2nd time to fill it.
+*/
+   if (config-num_plane
+   (plane_resp-count_planes= config-num_plane)) {
+   plane_ptr = (uint32_t *)(unsigned long)plane_resp-plane_id_ptr;
+
+   list_for_each_entry(plane,config-plane_list, head) {
+   if (put_user(plane-base.id, plane_ptr + copied)) {
+   ret = -EFAULT;
+   goto out;
+   }
+   copied++;
+   }
+   }
+   plane_resp-count_planes = config-num_plane;
+
+out:
+   mutex_unlock(dev-mode_config.mutex);
+   return ret;
+}
+
+/**
+ * drm_mode_getplane - get plane info
+ * @dev: DRM device
+ * @data: ioctl data
+ * @file_priv: DRM file info
+ *
+ * Return plane info, including formats supported, gamma size, any
+ * current fb, etc.
+ */
+int drm_mode_getplane(struct drm_device *dev, void *data,
+   struct drm_file *file_priv)
+{
+   struct drm_mode_get_plane *plane_resp = data;
+   struct drm_mode_object *obj;
+   struct drm_plane *plane;
+   uint32_t __user *format_ptr;
+   int ret = 0;
+
+   if (!drm_core_check_feature(dev, DRIVER_MODESET))
+   return -EINVAL;
+
+   mutex_lock(dev-mode_config.mutex);
+   obj = drm_mode_object_find(dev, plane_resp-plane_id,
+  DRM_MODE_OBJECT_PLANE);
+   if (!obj) {
+   

Re: [PATCH 01/11] drm: add plane support

[Daniel Vetter]

On Tue, Oct 25, 2011 at 11:46:56AM +0200, Jesse Barnes wrote:
 Planes are a bit like half-CRTCs.  They have a location and fb, but
 don't drive outputs directly.  Add support for handling them to the core
 KMS code.
 
 Signed-off-by: Jesse Barnes jbar...@virtuousgeek.org

As discussed with Jesse on irc, drm fb handling is fragile. Current rules:
- fbs are not reference counted, hence when destroying we need to disable
  all crtcs (and now also planes) that use them. drm_framebuffer_cleanup
  does that atm
- drivers that hold onto fbs after the kms core drops the corresponding
  pointer needs to hold a ref onto the underlying backing storage (like
  e.g. for pageflip on the to-be-flipped-out fb as long as it might still
  be scanned out).

We need proper refcounting for these ... But for now this patch is missing
the plane cleanup in drm_framebuffer_cleanup.

Otherwise I think going with just the src and dst rect for set_plane is
about the only sensible thing given the crazy hw out there. But I lack the
knowledge about that kind of hw (and video stuff in general), so I'll
refrain from slapping my r-b on these two.

Cheers, Daniel
-- 
Daniel Vetter
Mail: dan...@ffwll.ch
Mobile: +41 (0)79 365 57 48
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 01/11] drm: add plane support

[Jesse Barnes]

On Tue, 25 Oct 2011 13:58:55 +0200
Daniel Vetter dan...@ffwll.ch wrote:

 On Tue, Oct 25, 2011 at 11:46:56AM +0200, Jesse Barnes wrote:
  Planes are a bit like half-CRTCs.  They have a location and fb, but
  don't drive outputs directly.  Add support for handling them to the
  core KMS code.
  
  Signed-off-by: Jesse Barnes jbar...@virtuousgeek.org
 
 As discussed with Jesse on irc, drm fb handling is fragile. Current
 rules:
 - fbs are not reference counted, hence when destroying we need to
 disable all crtcs (and now also planes) that use them.
 drm_framebuffer_cleanup does that atm
 - drivers that hold onto fbs after the kms core drops the
 corresponding pointer needs to hold a ref onto the underlying backing
 storage (like e.g. for pageflip on the to-be-flipped-out fb as long
 as it might still be scanned out).
 
 We need proper refcounting for these ... But for now this patch is
 missing the plane cleanup in drm_framebuffer_cleanup.

Ah yeah that's a better place for the disable plane call I currently
have in the intel specific code...  I'll fix up and test.

 Otherwise I think going with just the src and dst rect for set_plane
 is about the only sensible thing given the crazy hw out there. But I
 lack the knowledge about that kind of hw (and video stuff in
 general), so I'll refrain from slapping my r-b on these two.

Yeah, I think the drivers just need to be able to calculate the scaling
level from the params and program them into whatever regs they happen
to have (on Intel fortunately the scaling is figured out by hw when we
program in the source  dest values, subject to some restrictions).


Thanks,
Jesse
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 01/11] drm: add plane support

[Alan Cox]

 As discussed with Jesse on irc, drm fb handling is fragile. Current rules:
 - fbs are not reference counted, hence when destroying we need to disable
   all crtcs (and now also planes) that use them. drm_framebuffer_cleanup
   does that atm
 - drivers that hold onto fbs after the kms core drops the corresponding
   pointer needs to hold a ref onto the underlying backing storage (like
   e.g. for pageflip on the to-be-flipped-out fb as long as it might still
   be scanned out).
 
 We need proper refcounting for these ... But for now this patch is missing
 the plane cleanup in drm_framebuffer_cleanup.

I'd rather we fixed the framebuffer kref stuff as part of doing this
rather than have a poorer API because of something we have to fix anyway.

It shouldn't be *that* hard to fix, at least for this kind of use
case. Resize locking, fb moving etc are ugly issues, refcount shouldn't
be, and the tty layer also refcounts so we can only have the fb objects
themselves to worry about as we can defer fb destruction to tty close or
drm last unref even for those with consoles on them.

Alan
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 01/11] drm: add plane support

[Jesse Barnes]

On Tue, 25 Oct 2011 14:26:07 +0100
Alan Cox a...@lxorguk.ukuu.org.uk wrote:

  As discussed with Jesse on irc, drm fb handling is fragile. Current
  rules:
  - fbs are not reference counted, hence when destroying we need to
  disable all crtcs (and now also planes) that use them.
  drm_framebuffer_cleanup does that atm
  - drivers that hold onto fbs after the kms core drops the
  corresponding pointer needs to hold a ref onto the underlying
  backing storage (like e.g. for pageflip on the to-be-flipped-out fb
  as long as it might still be scanned out).
  
  We need proper refcounting for these ... But for now this patch is
  missing the plane cleanup in drm_framebuffer_cleanup.
 
 I'd rather we fixed the framebuffer kref stuff as part of doing this
 rather than have a poorer API because of something we have to fix
 anyway.
 
 It shouldn't be *that* hard to fix, at least for this kind of use
 case. Resize locking, fb moving etc are ugly issues, refcount
 shouldn't be, and the tty layer also refcounts so we can only have
 the fb objects themselves to worry about as we can defer fb
 destruction to tty close or drm last unref even for those with
 consoles on them.

Oh it doesn't affect the userspace API so I don't think it's urgent.
But I agree, it would be nice to clean up fb management a bit...  Any
volunteers? :)

Thanks,
Jesse
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 01/11] drm: add plane support

[Daniel Vetter]

On Tue, Oct 25, 2011 at 02:26:07PM +0100, Alan Cox wrote:
  As discussed with Jesse on irc, drm fb handling is fragile. Current rules:
  - fbs are not reference counted, hence when destroying we need to disable
all crtcs (and now also planes) that use them. drm_framebuffer_cleanup
does that atm
  - drivers that hold onto fbs after the kms core drops the corresponding
pointer needs to hold a ref onto the underlying backing storage (like
e.g. for pageflip on the to-be-flipped-out fb as long as it might still
be scanned out).
  
  We need proper refcounting for these ... But for now this patch is missing
  the plane cleanup in drm_framebuffer_cleanup.
 
 I'd rather we fixed the framebuffer kref stuff as part of doing this
 rather than have a poorer API because of something we have to fix anyway.

Imo we should do things piece by piece. Fixing up drm fb refcounting is
imo a rather low-prio thing for core drm. And I've already asked Rob
Clarke whether he can clean up some of the confiusion in the kms
framebuffer-destroy() functions.

 It shouldn't be *that* hard to fix, at least for this kind of use
 case. Resize locking, fb moving etc are ugly issues, refcount shouldn't
 be, and the tty layer also refcounts so we can only have the fb objects
 themselves to worry about as we can defer fb destruction to tty close or

I'm talking solely about kms framebuffers. I.e. completely orthogonal to
any issues you're seeing wrt kms-linux fb subsystem integration. Or I'm
completely misunderstanding you here ...
-Daniel
-- 
Daniel Vetter
Mail: dan...@ffwll.ch
Mobile: +41 (0)79 365 57 48
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 01/11] drm: add plane support

[Jesse Barnes]

Here's a diff I can roll in if it looks ok.  It adds the ability to
specify multiple handles for a single fb to better accommodate planar
configs.  I think Rob has convinced me that this is a good idea...
comments appreciated.

Thanks,
Jesse

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index a30b9d4..0cc2077 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -1923,7 +1923,8 @@ int drm_mode_addfb(struct drm_device *dev,
r.bpp = or-bpp;
r.depth = or-depth;
r.pixel_format = 0;
-   r.handle = or-handle;
+   r.handle_count = 1;
+   r.handles = (u64)or-handle;
 
if (!drm_core_check_feature(dev, DRIVER_MODESET))
return -EINVAL;
diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index cd7e04d..2c7f200 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -7619,8 +7619,9 @@ intel_user_framebuffer_create(struct drm_device *dev,
  struct drm_mode_fb_cmd2 *mode_cmd)
 {
struct drm_i915_gem_object *obj;
+   u32 *handles = (u32 *)mode_cmd-handles;
 
-   obj = to_intel_bo(drm_gem_object_lookup(dev, filp, mode_cmd-handle));
+   obj = to_intel_bo(drm_gem_object_lookup(dev, filp, handles[0]));
if (obj-base == NULL)
return ERR_PTR(-ENOENT);
 
diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c 
b/drivers/gpu/drm/nouveau/nouveau_display.c
index 7a428a9..cb9b868 100644
--- a/drivers/gpu/drm/nouveau/nouveau_display.c
+++ b/drivers/gpu/drm/nouveau/nouveau_display.c
@@ -128,9 +128,10 @@ nouveau_user_framebuffer_create(struct drm_device *dev,
 {
struct nouveau_framebuffer *nouveau_fb;
struct drm_gem_object *gem;
+   u32 *handles = (u32 *)mode_cmd-handles;
int ret;
 
-   gem = drm_gem_object_lookup(dev, file_priv, mode_cmd-handle);
+   gem = drm_gem_object_lookup(dev, file_priv, handles);
if (!gem)
return ERR_PTR(-ENOENT);
 
diff --git a/drivers/gpu/drm/radeon/radeon_display.c 
b/drivers/gpu/drm/radeon/radeon_display.c
index ae803f8..63a6d91 100644
--- a/drivers/gpu/drm/radeon/radeon_display.c
+++ b/drivers/gpu/drm/radeon/radeon_display.c
@@ -1128,11 +1128,12 @@ radeon_user_framebuffer_create(struct drm_device *dev,
 {
struct drm_gem_object *obj;
struct radeon_framebuffer *radeon_fb;
+   u32 *handles = (u32 *)mode_cmd-handles;
 
-   obj = drm_gem_object_lookup(dev, file_priv, mode_cmd-handle);
+   obj = drm_gem_object_lookup(dev, file_priv, handles[0]);
if (obj ==  NULL) {
dev_err(dev-pdev-dev, No GEM object associated to handle 
0x%08X, 
-   can't create framebuffer\n, mode_cmd-handle);
+   can't create framebuffer\n, handles[0]);
return ERR_PTR(-ENOENT);
}
 
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c 
b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
index 2a1b802..0ad7456 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
@@ -866,7 +866,7 @@ static struct drm_framebuffer *vmw_kms_fb_create(struct 
drm_device *dev,
 */
 
ret = vmw_user_surface_lookup_handle(dev_priv, tfile,
-mode_cmd-handle, surface);
+mode_cmd-handles[0], surface);
if (ret)
goto try_dmabuf;
 
diff --git a/drivers/staging/gma500/framebuffer.c 
b/drivers/staging/gma500/framebuffer.c
index 85f47d5..ee91ffe 100644
--- a/drivers/staging/gma500/framebuffer.c
+++ b/drivers/staging/gma500/framebuffer.c
@@ -496,7 +496,7 @@ static struct drm_framebuffer *psb_user_framebuffer_create
 *  Find the GEM object and thus the gtt range object that is
 *  to back this space
 */
-   obj = drm_gem_object_lookup(dev, filp, cmd-handle);
+   obj = drm_gem_object_lookup(dev, filp, cmd-handles[0]);
if (obj == NULL)
return ERR_PTR(-ENOENT);
 
diff --git a/include/drm/drm_mode.h b/include/drm/drm_mode.h
index 34a0d22..dafe8df 100644
--- a/include/drm/drm_mode.h
+++ b/include/drm/drm_mode.h
@@ -272,8 +272,9 @@ struct drm_mode_fb_cmd2 {
__u32 bpp;
__u32 depth;
__u32 pixel_format; /* fourcc code from videodev2.h */
-   /* driver specific handle */
-   __u32 handle;
+   __u32 handle_count;
+   /* driver specific buffer object handle array */
+   __u64 handles;
 };
 
 #define DRM_MODE_FB_DIRTY_ANNOTATE_COPY 0x01
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 01/11] drm: add plane support

[Rob Clark]

On Tue, Oct 25, 2011 at 9:09 AM, Jesse Barnes jbar...@virtuousgeek.org wrote:
 diff --git a/include/drm/drm_mode.h b/include/drm/drm_mode.h
 index 34a0d22..dafe8df 100644
 --- a/include/drm/drm_mode.h
 +++ b/include/drm/drm_mode.h
 @@ -272,8 +272,9 @@ struct drm_mode_fb_cmd2 {
        __u32 bpp;
        __u32 depth;
        __u32 pixel_format; /* fourcc code from videodev2.h */
 -       /* driver specific handle */
 -       __u32 handle;
 +       __u32 handle_count;
 +       /* driver specific buffer object handle array */
 +       __u64 handles;
  };


Ok, after a bit of discussion with Jakob on IRC, this is what we arrived at:

1) It would be nice to have the option for multi-planar formats to be
able to use one single buffer object, or one buffer object per plane.
In the case of one buffer per plane, the order is dictated by the
fourcc.

2) We do need per-plane stride for some formats, in particular I420
which is a bit ambiguous otherwise (ie. is the stride of the U and V
planes half the stride as the Y plane, or the same?)

3) Maybe we need per-plane offsets, but I think this case could be
handled by just using one bo per plane, so left it out

4) bpp/depth are redundant w/ the pixel_format. Just in case, as a
future placeholder, and inspired by 'struct v4l2_pix_format', add a
priv field.  Although making the field big enough to hold a pointer if
absolutely really needed.

struct drm_mode_fb_cmd2 {
  __u32 fb_id;
  __u32 width, height;
  __u32 pixel_format; /* fourcc code from videodev2.h */
  __u64 priv;  /* private data, depends on pixelformat */

  /* in case of planar formats, either one buffer object,
   * or one buffer object per plane, is allowed.  In the
   * case per-plane bo's, the order is dictated by the
   * fourcc.. ie. NV12 (http://fourcc.org/yuv.php#NV12)
   * is described as:
   *
   *   YUV 4:2:0 image with a plane of 8 bit Y samples
   *   followed by an interleaved U/V plane containing
   *   8 bit 2x2 subsampled colour difference samples.
   *
   * So it would consist of Y as first buffer and UV as
   * second buffer.  In the case that only a single bo
   * is used then buffer[1].handle should be zero.  (But
   * a plane specific pitch could still be specified.)
   */
  struct {
__u32 pitch;
/* driver specific handle */
__u32 handle;
  } buffer[16];
 };


BR,
-R
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 01/11] drm: add plane support

[Daniel Vetter]

On Tue, Oct 25, 2011 at 11:43:09AM -0500, Rob Clark wrote:
 On Tue, Oct 25, 2011 at 9:09 AM, Jesse Barnes jbar...@virtuousgeek.org 
 wrote:
  diff --git a/include/drm/drm_mode.h b/include/drm/drm_mode.h
  index 34a0d22..dafe8df 100644
  --- a/include/drm/drm_mode.h
  +++ b/include/drm/drm_mode.h
  @@ -272,8 +272,9 @@ struct drm_mode_fb_cmd2 {
         __u32 bpp;
         __u32 depth;
         __u32 pixel_format; /* fourcc code from videodev2.h */
  -       /* driver specific handle */
  -       __u32 handle;
  +       __u32 handle_count;
  +       /* driver specific buffer object handle array */
  +       __u64 handles;
   };
 
 
 Ok, after a bit of discussion with Jakob on IRC, this is what we arrived at:
 
 1) It would be nice to have the option for multi-planar formats to be
 able to use one single buffer object, or one buffer object per plane.
 In the case of one buffer per plane, the order is dictated by the
 fourcc.
 
 2) We do need per-plane stride for some formats, in particular I420
 which is a bit ambiguous otherwise (ie. is the stride of the U and V
 planes half the stride as the Y plane, or the same?)
 
 3) Maybe we need per-plane offsets, but I think this case could be
 handled by just using one bo per plane, so left it out
 
 4) bpp/depth are redundant w/ the pixel_format. Just in case, as a
 future placeholder, and inspired by 'struct v4l2_pix_format', add a
 priv field.  Although making the field big enough to hold a pointer if
 absolutely really needed.
 
 struct drm_mode_fb_cmd2 {
   __u32 fb_id;
   __u32 width, height;
   __u32 pixel_format; /* fourcc code from videodev2.h */
   __u64 priv;  /* private data, depends on pixelformat */
 
   /* in case of planar formats, either one buffer object,
* or one buffer object per plane, is allowed.  In the
* case per-plane bo's, the order is dictated by the
* fourcc.. ie. NV12 (http://fourcc.org/yuv.php#NV12)
* is described as:
*
*   YUV 4:2:0 image with a plane of 8 bit Y samples
*   followed by an interleaved U/V plane containing
*   8 bit 2x2 subsampled colour difference samples.
*
* So it would consist of Y as first buffer and UV as
* second buffer.  In the case that only a single bo
* is used then buffer[1].handle should be zero.  (But
* a plane specific pitch could still be specified.)
*/
   struct {
 __u32 pitch;
 /* driver specific handle */
 __u32 handle;

Why not just add a
__u32 offset;
__u32 rsvd;
and call it a day. This thing is pretty big already, so that bloat doesn't
matter that much. Maybe spec that buffer[0].offset must be zero. With that
you can also do I420 with the following layout
+---+
|YYY|
|YYY|
|YYY|
|YYY|
+---+---+
|UUU|VVV|
|UUU|VVV|
+---+---+

i.e. stride_U == stride_V, with their lines meshed into one line.
-Daniel

   } buffer[16];
  };
 
 
 BR,
 -R
 ___
 dri-devel mailing list
 dri-devel@lists.freedesktop.org
 http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Mail: dan...@ffwll.ch
Mobile: +41 (0)79 365 57 48
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 01/11] drm: add plane support

[Rob Clark]

On Tue, Oct 25, 2011 at 2:41 PM, Daniel Vetter dan...@ffwll.ch wrote:
 On Tue, Oct 25, 2011 at 11:43:09AM -0500, Rob Clark wrote:
 On Tue, Oct 25, 2011 at 9:09 AM, Jesse Barnes jbar...@virtuousgeek.org 
 wrote:
  diff --git a/include/drm/drm_mode.h b/include/drm/drm_mode.h
  index 34a0d22..dafe8df 100644
  --- a/include/drm/drm_mode.h
  +++ b/include/drm/drm_mode.h
  @@ -272,8 +272,9 @@ struct drm_mode_fb_cmd2 {
         __u32 bpp;
         __u32 depth;
         __u32 pixel_format; /* fourcc code from videodev2.h */
  -       /* driver specific handle */
  -       __u32 handle;
  +       __u32 handle_count;
  +       /* driver specific buffer object handle array */
  +       __u64 handles;
   };


 Ok, after a bit of discussion with Jakob on IRC, this is what we arrived at:

 1) It would be nice to have the option for multi-planar formats to be
 able to use one single buffer object, or one buffer object per plane.
 In the case of one buffer per plane, the order is dictated by the
 fourcc.

 2) We do need per-plane stride for some formats, in particular I420
 which is a bit ambiguous otherwise (ie. is the stride of the U and V
 planes half the stride as the Y plane, or the same?)

 3) Maybe we need per-plane offsets, but I think this case could be
 handled by just using one bo per plane, so left it out

 4) bpp/depth are redundant w/ the pixel_format. Just in case, as a
 future placeholder, and inspired by 'struct v4l2_pix_format', add a
 priv field.  Although making the field big enough to hold a pointer if
 absolutely really needed.

 struct drm_mode_fb_cmd2 {
   __u32 fb_id;
   __u32 width, height;
   __u32 pixel_format; /* fourcc code from videodev2.h */
   __u64 priv;  /* private data, depends on pixelformat */

   /* in case of planar formats, either one buffer object,
    * or one buffer object per plane, is allowed.  In the
    * case per-plane bo's, the order is dictated by the
    * fourcc.. ie. NV12 (http://fourcc.org/yuv.php#NV12)
    * is described as:
    *
    *   YUV 4:2:0 image with a plane of 8 bit Y samples
    *   followed by an interleaved U/V plane containing
    *   8 bit 2x2 subsampled colour difference samples.
    *
    * So it would consist of Y as first buffer and UV as
    * second buffer.  In the case that only a single bo
    * is used then buffer[1].handle should be zero.  (But
    * a plane specific pitch could still be specified.)
    */
   struct {
     __u32 pitch;
     /* driver specific handle */
     __u32 handle;

 Why not just add a
        __u32 offset;
        __u32 rsvd;
 and call it a day. This thing is pretty big already, so that bloat doesn't
 matter that much. Maybe spec that buffer[0].offset must be zero. With that
 you can also do I420 with the following layout
 +---+
 |YYY|
 |YYY|
 |YYY|
 |YYY|
 +---+---+
 |UUU|VVV|
 |UUU|VVV|
 +---+---+

 i.e. stride_U == stride_V, with their lines meshed into one line.
 -Daniel


I've no objection to an offset field.. I was mostly just trying to
avoid that we keep adding things until we get XvImageFormatValues..

We should either add an offset plus pad field as Daniel suggested, or
at least add a u64 reserved field which could be made into made into
an offset field later if needed.

OTOH, is the format you described *really* still I420?

BR,
-R

   } buffer[16];
  };


 BR,
 -R
 ___
 dri-devel mailing list
 dri-devel@lists.freedesktop.org
 http://lists.freedesktop.org/mailman/listinfo/dri-devel

 --
 Daniel Vetter
 Mail: dan...@ffwll.ch
 Mobile: +41 (0)79 365 57 48
 ___
 dri-devel mailing list
 dri-devel@lists.freedesktop.org
 http://lists.freedesktop.org/mailman/listinfo/dri-devel

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 01/11] drm: add plane support

[Joonyoung Shim]

10/25/2011 08:18 PM, Jesse Barnes 쓴 글:

On Tue, 25 Oct 2011 19:53:02 +0900
Joonyoung Shimjy0922.s...@samsung.com  wrote:

+/**
+ * drm_plane - central DRM plane control structure
+ * @dev: DRM device this plane belongs to
+ * @kdev: kernel device
+ * @attr: kdev attributes
+ * @head: for list management
+ * @base: base mode object
+ * @crtc_x: x position of plane (relative to pipe base)
+ * @crtc_y: y position of plane
+ * @x: x offset into fb
+ * @y: y offset into fb

Above 4 members don't be used.

Oops yeah, I'll fix up the comments.


+
+struct drm_mode_get_plane {
+   __u64 format_type_ptr;
+   __u32 plane_id;
+
+   __u32 crtc_id;
+   __u32 fb_id;
+
+   __u32 possible_crtcs;
+   __u32 gamma_size;
+
+   __u32 count_format_types;
+};

I wonder why doesn't give to user crtc_x, crtc_y, crtc_w, crtc_h
information?

It could, but the caller should already know was my thinking.  Would
you like those bits returned?


I saw such codes in initial your RFC patch version so i just have
wondered why it is removed. It can be added later if needs.

Thanks.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 01/11] drm: add plane support

[Joonyoung Shim]

10/25/2011 06:46 PM, Jesse Barnes 쓴 글:

[snip]

diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 8020798..d7f03aa 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -44,6 +44,7 @@ struct drm_framebuffer;
  #define DRM_MODE_OBJECT_PROPERTY 0xb0b0b0b0
  #define DRM_MODE_OBJECT_FB 0xfbfbfbfb
  #define DRM_MODE_OBJECT_BLOB 0x
+#define DRM_MODE_OBJECT_PLANE 0x

  struct drm_mode_object {
uint32_t id;
@@ -278,6 +279,7 @@ struct drm_crtc;
  struct drm_connector;
  struct drm_encoder;
  struct drm_pending_vblank_event;
+struct drm_plane;

  /**
   * drm_crtc_funcs - control CRTCs for a given device
@@ -536,6 +538,58 @@ struct drm_connector {
  };

  /**
+ * drm_plane_funcs - driver plane control functions
+ * @update_plane: update the plane configuration
+ */
+struct drm_plane_funcs {
+   int (*update_plane)(struct drm_plane *plane,
+   struct drm_crtc *crtc, struct drm_framebuffer *fb,
+   int crtc_x, int crtc_y,
+   unsigned int crtc_w, unsigned int crtc_h,
+   uint32_t src_x, uint32_t src_y,
+   uint32_t src_w, uint32_t src_h);
+   int (*disable_plane)(struct drm_plane *plane);
+};


How about add destroy() function and call it in
drm_mode_config_cleanup()?

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel