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

[Inki Dae <daeinki@gmail.com>]

Hi, all.

SW Kim <devusr.opensw <at> gmail.com> writes:

> 
> Daniel Vetter <daniel <at> ffwll.ch> writes:
> 
> [snip]
> 
> > > 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 */
> > > 
> 
> [snip]
> 
> > >   struct {
> > >     __u32 pitch;
> > >     /* driver specific handle */
> > >     __u32 handle;
> > 
> > Why not just add a
> > 	__u32 offset;
> > 	__u32 rsvd;
> 
> For normal NV12 format, I agree that just offset is enough.
> But considering more specific format like NV12M, that has two separated 
memory 
> spaces, IMHO, it needs handle per each plane.
> 
> Each plane of NV12M has it own base address and space between these two 
memory 
> spaces can be used by others.
> 
> You can refer following link about NV12M used by V4L2.
> http://linuxtv.org/downloads/v4l-dvb-apis/V4L2-PIX-FMT-NV12M.html
> 
> > 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];
> 
> and just question, is there any meaning about buffer array size 16?
> 
> > >  };
> 
> [snip]
> 
> Thanks and Regards,
> - SW Kim
> 

I have a opinion for multi planer. before that, I'd like to mention some 
considerations.

I think adding any variables such as offset or handle(as plane count) to 
drm_mode_fb_cmd2 structure could occurs that in application point of view, 
user could be confused because with the ways above, user needs to allocate 
buffers as plane count(it depends on pixel format) and also user should know 
buffer size according to pixel format. so I think it's good way that user sets 
only pixel format and resolution(width, height) and then specific gem 
framework allocates buffers according to pixel format as plane count and then 
user sets the gem handle to drm_mode_fb_cmd2 structure. specific gem framework 
could get all buffers through only the gem handle.

below is my simple idea.
1. user requests buffer allocation with pixel format and resolution through 
gem framework.
2. gem framework checks pixel format.
3. specific gem framework allocates buffers as plane count according to the 
pixel format. (please, know that gem framework provides just interface so 
acctual implementation would be done at specific gem framework)
4. user gets the gem handle from gem framework.
5. user sets the gem handle to specific drm framebuffer through 
drm_mode_addfb2 function.
6. user requests setcrtc with fb id and crtc id.
7. drm framework sets framebuffer(corresponding to fb id) to drm_mode_set.
8. crtc calls set_config callbacks to configure hardware.
9. specific crtc framework gets all buffers through framebuffer(actually, it 
would be specific framebuffer)and sets them to overlay registers of hardware 
appropriately.


like this, how about using framebuffer and gem framework instead of plane? I'd 
be glad to give me your opinions.

Thank you,
Inki Dae.