Hi Tomasz,
Thank you for the review.
On Wednesday 20 June 2012 17:41:54 Tomasz Stanislawski wrote:
> Hi Laurent,
> Thank you for the patch.
>
> On 06/20/2012 04:09 PM, Laurent Pinchart wrote:
> > Add support for the dma-buf exporter role to the frame buffer API. The
> > importer role isn't meaningful for frame buffer devices, as the frame
> > buffer device model doesn't allow using externally allocated memory.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> > ---
> >
> > Documentation/fb/api.txt | 36 ++++++++++++++++++++++++++++++++++++
> > drivers/video/fbmem.c | 36 ++++++++++++++++++++++++++++++++++++
> > include/linux/fb.h | 12 ++++++++++++
> > 3 files changed, 84 insertions(+), 0 deletions(-)
>
> [snip]
>
> > +The export a frame buffer as a dma-buf file descriptors, applications
> > call the +FBIOGET_DMABUF ioctl. The ioctl takes a pointer to a
> > fb_dmabuf_export +structure.
> > +
> > +struct fb_dmabuf_export {
> > + __u32 fd;
> > + __u32 flags;
> > +};
>
> What do you think about adding some reserved fields to struct
> fb_dmabuf_export to make it future-proof for DMABUF extensions?
Already done. I've added them at the last minute, and for some reason I've
sent the previous version of the patch.
> > +
> > +The flag field specifies the flags to be used when creating the dma-buf
> > file +descriptor. The only supported flag is O_CLOEXEC. If the call is
> > successful, +the driver will set the fd field to a file descriptor
> > corresponding to the +dma-buf object.
> > +
> > +Applications can then pass the file descriptors to another application or
> > +another device driver. The dma-buf object is automatically
> > reference-counted, +applications can and should close the file descriptor
> > as soon as they don't +need it anymore. The underlying dma-buf object
> > will not be freed before the +last device that uses the dma-buf object
> > releases it.
> > diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
> > index 0dff12a..400e449 100644
> > --- a/drivers/video/fbmem.c
> > +++ b/drivers/video/fbmem.c
> > @@ -15,6 +15,7 @@
> >
> > #include <linux/compat.h>
> > #include <linux/types.h>
> >
> > +#include <linux/dma-buf.h>
> >
> > #include <linux/errno.h>
> > #include <linux/kernel.h>
> > #include <linux/major.h>
> >
> > @@ -1074,6 +1075,23 @@ fb_blank(struct fb_info *info, int blank)
> >
> > return ret;
> >
> > }
> >
> > +#ifdef CONFIG_DMA_SHARED_BUFFER
> > +int
> > +fb_get_dmabuf(struct fb_info *info, int flags)
> > +{
> > + struct dma_buf *dmabuf;
> > +
> > + if (info->fbops->fb_dmabuf_export == NULL)
> > + return -ENOTTY;
> > +
> > + dmabuf = info->fbops->fb_dmabuf_export(info);
>
> IMO, it is not a good idea to delegate an implementation of DMABUF ops to
> the driver. Maybe exporting could be handled inside FB stack. As I
> understand, the FB stack needs only 'get-scatterlist' ops from an FB driver.
> All other DMABUF magic does not need driver involvement, does it?
Beside creating the sg-list, drivers also need to prevent the underlying
memory from being freed. If an application requests a frame buffer format or
size change, the driver could resize the underlying memory, which needs to be
prevented as long as the buffer is exported.
The dma-buf support implementation in the sh_mobile_lcdcfb driver also takes a
reference to the device and the driver for the duration of the export, to
prevent the driver from being unloaded and the device from being released.
static struct dma_buf *sh_mobile_dmabuf_export(struct fb_info *info)
{
struct sh_mobile_lcdc_chan *ch = info->par;
struct dma_buf *dmabuf;
dmabuf = dma_buf_export(ch, &sh_mobile_lcdc_dmabuf_ops, ch->fb_size, 0);
if (!IS_ERR(dmabuf)) {
/* We already hold references to the module and the device, so
* we don't have to take them prior to calling dma_buf_export().
*/
get_device(ch->lcdc->dev);
__module_get(THIS_MODULE);
}
return dmabuf;
}
This could be moved in the fbdev core though, as ch->lcdc->dev is available
trhough info->dev, and THIS_MODULE could be replaced with info->dev->driver-
>owner.
Moving the implementation to the fbdev core would thus require 3 callbacks to
lock the frame buffer, get the sg list, and release the frame buffer. However,
in practice, we might not have drivers that resize buffers when the format or
size is modified, in which case a single callback would be enough.
> > + if (IS_ERR(dmabuf))
> > + return PTR_ERR(dmabuf);
> > +
> > + return dma_buf_fd(dmabuf, flags);
> > +}
> > +#endif
> > +
>
> [snip]
>
> > diff --git a/include/linux/fb.h b/include/linux/fb.h
> > index ac3f1c6..c9fee75 100644
> > --- a/include/linux/fb.h
> > +++ b/include/linux/fb.h
> > @@ -701,6 +708,11 @@ struct fb_ops {
> >
> > /* called at KDB enter and leave time to prepare the console */
> > int (*fb_debug_enter)(struct fb_info *info);
> > int (*fb_debug_leave)(struct fb_info *info);
> >
> > +
> > +#ifdef CONFIG_DMA_SHARED_BUFFER
> > + /* Export the frame buffer as a dmabuf object */
> > + struct dma_buf *(*fb_dmabuf_export)(struct fb_info *info);
> > +#endif
>
> Memory trashing or even kernel crash may happen if a module compiled
> with CONFIG_DMA_SHARED_BUFFER enabled is loaded into kernel with
> CONFIG_DMA_SHARED_BUFFER disabled.
I'll remove the #ifdef. The memory savings are not worth the potential issues.
> > };
> >
> > #ifdef CONFIG_FB_TILEBLITTING
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
