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

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Oct 25, 2011 at 11:43:09AM -0500, Rob Clark wrote:
> On Tue, Oct 25, 2011 at 9:09 AM, Jesse Barnes <jbarnes@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
+-------+
|YYYYYYY|
|YYYYYYY|
|YYYYYYY|
|YYYYYYY|
+---+---+
|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: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48