[PATCH] drm/omap: update for interlaced

[Semwal, Sumit]

Hi Rob,
On Thu, Sep 6, 2012 at 3:18 AM, Rob Clark  wrote:
> From: Rob Clark 
>
> 'struct omap_video_timings' was updated w/ a 'bool interlaced'.  Without
> a matching update in omap_connector, this field could have undefined
> values from the stack, which isn't quite ideal.
>
> Update the fxns to convert omapdss<->drm timings structs, and zero-init
> 'struct omap_video_timings' when it is declared on stack to avoid issues
> like this in the future.
>
> Signed-off-by: Rob Clark 
Feel free to use
Reviewed-by: Sumit Semwal 
> ---
>  drivers/staging/omapdrm/omap_connector.c |   17 -
>  1 file changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/staging/omapdrm/omap_connector.c 
> b/drivers/staging/omapdrm/omap_connector.c
> index 5e2856c..5f4a89b 100644
> --- a/drivers/staging/omapdrm/omap_connector.c
> +++ b/drivers/staging/omapdrm/omap_connector.c
> @@ -48,13 +48,10 @@ static inline void copy_timings_omap_to_drm(struct 
> drm_display_mode *mode,
> mode->vsync_end = mode->vsync_start + timings->vsw;
> mode->vtotal = mode->vsync_end + timings->vbp;
>
> -   /* note: whether or not it is interlaced, +/- h/vsync, etc,
> -* which should be set in the mode flags, is not exposed in
> -* the omap_video_timings struct.. but hdmi driver tracks
> -* those separately so all we have to have to set the mode
> -* is the way to recover these timings values, and the
> -* omap_dss_driver would do the rest.
> -*/
> +   mode->flags = 0;
> +
> +   if (timings->interlace)
> +   mode->flags |= DRM_MODE_FLAG_INTERLACE;
>  }
>
>  static inline void copy_timings_drm_to_omap(struct omap_video_timings 
> *timings,
> @@ -71,6 +68,8 @@ static inline void copy_timings_drm_to_omap(struct 
> omap_video_timings *timings,
> timings->vfp = mode->vsync_start - mode->vdisplay;
> timings->vsw = mode->vsync_end - mode->vsync_start;
> timings->vbp = mode->vtotal - mode->vsync_end;
> +
> +   timings->interlace = !!(mode->flags & DRM_MODE_FLAG_INTERLACE);
>  }
>
>  static void omap_connector_dpms(struct drm_connector *connector, int mode)
> @@ -187,7 +186,7 @@ static int omap_connector_get_modes(struct drm_connector 
> *connector)
> }
> } else {
> struct drm_display_mode *mode = drm_mode_create(dev);
> -   struct omap_video_timings timings;
> +   struct omap_video_timings timings = {0};
>
> dssdrv->get_timings(dssdev, &timings);
>
> @@ -291,7 +290,7 @@ void omap_connector_mode_set(struct drm_connector 
> *connector,
> struct omap_connector *omap_connector = to_omap_connector(connector);
> struct omap_dss_device *dssdev = omap_connector->dssdev;
> struct omap_dss_driver *dssdrv = dssdev->driver;
> -   struct omap_video_timings timings;
> +   struct omap_video_timings timings = {0};
>
> copy_timings_drm_to_omap(&timings, mode);
>
> --
> 1.7.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] drm/omap: update for interlaced

[Semwal, Sumit]

Hi Rob,
On Thu, Sep 6, 2012 at 3:18 AM, Rob Clark  wrote:
> From: Rob Clark 
>
> 'struct omap_video_timings' was updated w/ a 'bool interlaced'.  Without
> a matching update in omap_connector, this field could have undefined
> values from the stack, which isn't quite ideal.
>
> Update the fxns to convert omapdss<->drm timings structs, and zero-init
> 'struct omap_video_timings' when it is declared on stack to avoid issues
> like this in the future.
>
> Signed-off-by: Rob Clark 
Feel free to use
Reviewed-by: Sumit Semwal 
> ---
>  drivers/staging/omapdrm/omap_connector.c |   17 -
>  1 file changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/staging/omapdrm/omap_connector.c 
> b/drivers/staging/omapdrm/omap_connector.c
> index 5e2856c..5f4a89b 100644
> --- a/drivers/staging/omapdrm/omap_connector.c
> +++ b/drivers/staging/omapdrm/omap_connector.c
> @@ -48,13 +48,10 @@ static inline void copy_timings_omap_to_drm(struct 
> drm_display_mode *mode,
> mode->vsync_end = mode->vsync_start + timings->vsw;
> mode->vtotal = mode->vsync_end + timings->vbp;
>
> -   /* note: whether or not it is interlaced, +/- h/vsync, etc,
> -* which should be set in the mode flags, is not exposed in
> -* the omap_video_timings struct.. but hdmi driver tracks
> -* those separately so all we have to have to set the mode
> -* is the way to recover these timings values, and the
> -* omap_dss_driver would do the rest.
> -*/
> +   mode->flags = 0;
> +
> +   if (timings->interlace)
> +   mode->flags |= DRM_MODE_FLAG_INTERLACE;
>  }
>
>  static inline void copy_timings_drm_to_omap(struct omap_video_timings 
> *timings,
> @@ -71,6 +68,8 @@ static inline void copy_timings_drm_to_omap(struct 
> omap_video_timings *timings,
> timings->vfp = mode->vsync_start - mode->vdisplay;
> timings->vsw = mode->vsync_end - mode->vsync_start;
> timings->vbp = mode->vtotal - mode->vsync_end;
> +
> +   timings->interlace = !!(mode->flags & DRM_MODE_FLAG_INTERLACE);
>  }
>
>  static void omap_connector_dpms(struct drm_connector *connector, int mode)
> @@ -187,7 +186,7 @@ static int omap_connector_get_modes(struct drm_connector 
> *connector)
> }
> } else {
> struct drm_display_mode *mode = drm_mode_create(dev);
> -   struct omap_video_timings timings;
> +   struct omap_video_timings timings = {0};
>
> dssdrv->get_timings(dssdev, &timings);
>
> @@ -291,7 +290,7 @@ void omap_connector_mode_set(struct drm_connector 
> *connector,
> struct omap_connector *omap_connector = to_omap_connector(connector);
> struct omap_dss_device *dssdev = omap_connector->dssdev;
> struct omap_dss_driver *dssdrv = dssdev->driver;
> -   struct omap_video_timings timings;
> +   struct omap_video_timings timings = {0};
>
> copy_timings_drm_to_omap(&timings, mode);
>
> --
> 1.7.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] staging: omapdrm: Fix DMM sparse warnings

[Semwal, Sumit]

On Thu, Aug 9, 2012 at 10:26 PM, Rob Clark  wrote:
> On Thu, Aug 9, 2012 at 12:14 AM, Andy Gross  wrote:
>> Fix the following sparse warnings:
>>
>> drivers/staging/omapdrm/omap_dmm_tiler.c:123:13:
>>warning: symbol 'omap_dmm_irq_handler' was not declared.
>>Should it be static?
>>
>> drivers/staging/omapdrm/omap_dmm_tiler.c:370:24:
>>warning: Using plain integer as NULL pointer
>>
>> Signed-off-by: Andy Gross 
>
> Signed-off-by: Rob Clark 
Reviewed-by: Sumit Semwal 
>
>> ---
>>  drivers/staging/omapdrm/omap_dmm_tiler.c |4 ++--
>>  1 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/staging/omapdrm/omap_dmm_tiler.c 
>> b/drivers/staging/omapdrm/omap_dmm_tiler.c
>> index 8619783..ec7a5c8 100644
>> --- a/drivers/staging/omapdrm/omap_dmm_tiler.c
>> +++ b/drivers/staging/omapdrm/omap_dmm_tiler.c
>> @@ -120,7 +120,7 @@ static int wait_status(struct refill_engine *engine, 
>> uint32_t wait_mask)
>> return 0;
>>  }
>>
>> -irqreturn_t omap_dmm_irq_handler(int irq, void *arg)
>> +static irqreturn_t omap_dmm_irq_handler(int irq, void *arg)
>>  {
>> struct dmm *dmm = arg;
>> uint32_t status = readl(dmm->base + DMM_PAT_IRQSTATUS);
>> @@ -367,7 +367,7 @@ struct tiler_block *tiler_reserve_1d(size_t size)
>> int num_pages = (size + PAGE_SIZE - 1) >> PAGE_SHIFT;
>>
>> if (!block)
>> -   return 0;
>> +   return ERR_PTR(-ENOMEM);
>>
>> block->fmt = TILFMT_PAGE;
>>
>> --
>> 1.7.5.4
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
>> the body of a message to majordomo at vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> ___
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] staging: omapdrm: Fix DMM sparse warnings

[Semwal, Sumit]

On Thu, Aug 9, 2012 at 10:26 PM, Rob Clark  wrote:
> On Thu, Aug 9, 2012 at 12:14 AM, Andy Gross  wrote:
>> Fix the following sparse warnings:
>>
>> drivers/staging/omapdrm/omap_dmm_tiler.c:123:13:
>>warning: symbol 'omap_dmm_irq_handler' was not declared.
>>Should it be static?
>>
>> drivers/staging/omapdrm/omap_dmm_tiler.c:370:24:
>>warning: Using plain integer as NULL pointer
>>
>> Signed-off-by: Andy Gross 
>
> Signed-off-by: Rob Clark 
Reviewed-by: Sumit Semwal 
>
>> ---
>>  drivers/staging/omapdrm/omap_dmm_tiler.c |4 ++--
>>  1 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/staging/omapdrm/omap_dmm_tiler.c 
>> b/drivers/staging/omapdrm/omap_dmm_tiler.c
>> index 8619783..ec7a5c8 100644
>> --- a/drivers/staging/omapdrm/omap_dmm_tiler.c
>> +++ b/drivers/staging/omapdrm/omap_dmm_tiler.c
>> @@ -120,7 +120,7 @@ static int wait_status(struct refill_engine *engine, 
>> uint32_t wait_mask)
>> return 0;
>>  }
>>
>> -irqreturn_t omap_dmm_irq_handler(int irq, void *arg)
>> +static irqreturn_t omap_dmm_irq_handler(int irq, void *arg)
>>  {
>> struct dmm *dmm = arg;
>> uint32_t status = readl(dmm->base + DMM_PAT_IRQSTATUS);
>> @@ -367,7 +367,7 @@ struct tiler_block *tiler_reserve_1d(size_t size)
>> int num_pages = (size + PAGE_SIZE - 1) >> PAGE_SHIFT;
>>
>> if (!block)
>> -   return 0;
>> +   return ERR_PTR(-ENOMEM);
>>
>> block->fmt = TILFMT_PAGE;
>>
>> --
>> 1.7.5.4
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
>> the body of a message to majord...@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> ___
> 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

[PATCH 2/2] dma-buf: add helpers for attacher dma-parms

[Semwal, Sumit]

On Fri, Jul 20, 2012 at 10:09 PM, Rob Clark  wrote:
> Fyi, Daniel Vetter had suggested on IRC that it would be cleaner to
> have a single helper fxn that most-restrictive union of all attached
> device's dma_parms.  Really this should include dma_mask and
> coherent_dma_mask, I think.  But that touches a lot of other places in
> the code.  If no one objects to the cleanup of moving
> dma_mask/coherent_dma_mask into dma_parms, I'll do this first.
>
> So anyways, don't consider this patch yet for inclusion, I'll make an
> updated one based on dma_parms..
Hi Rob,
Any news on this patch-set?
>
> BR,
> -R
BR,
~Sumit.
>
> On Thu, Jul 19, 2012 at 11:23 AM, Rob Clark  wrote:
>> From: Rob Clark 
>>
>> Add some helpers to iterate through all attachers and get the most
>> restrictive segment size/count/boundary.
>>
>> Signed-off-by: Rob Clark 
>> ---
>>  drivers/base/dma-buf.c  |   63 
>> +++
>>  include/linux/dma-buf.h |   19 ++
>>  2 files changed, 82 insertions(+)
>>
>> diff --git a/drivers/base/dma-buf.c b/drivers/base/dma-buf.c
>> index 24e88fe..757ee20 100644
>> --- a/drivers/base/dma-buf.c
>> +++ b/drivers/base/dma-buf.c
>> @@ -192,6 +192,69 @@ void dma_buf_put(struct dma_buf *dmabuf)
>>  EXPORT_SYMBOL_GPL(dma_buf_put);
>>
>>  /**
>> + * dma_buf_max_seg_size - helper for exporters to get the minimum of
>> + * all attached device's max segment size
>> + */
>> +unsigned int dma_buf_max_seg_size(struct dma_buf *dmabuf)
>> +{
>> +   struct dma_buf_attachment *attach;
>> +   unsigned int max = (unsigned int)-1;
>> +
>> +   if (WARN_ON(!dmabuf))
>> +   return 0;
>> +
>> +   mutex_lock(&dmabuf->lock);
>> +   list_for_each_entry(attach, &dmabuf->attachments, node)
>> +   max = min(max, dma_get_max_seg_size(attach->dev));
>> +   mutex_unlock(&dmabuf->lock);
>> +
>> +   return max;
>> +}
>> +EXPORT_SYMBOL_GPL(dma_buf_max_seg_size);
>> +
>> +/**
>> + * dma_buf_max_seg_count - helper for exporters to get the minimum of
>> + * all attached device's max segment count
>> + */
>> +unsigned int dma_buf_max_seg_count(struct dma_buf *dmabuf)
>> +{
>> +   struct dma_buf_attachment *attach;
>> +   unsigned int max = (unsigned int)-1;
>> +
>> +   if (WARN_ON(!dmabuf))
>> +   return 0;
>> +
>> +   mutex_lock(&dmabuf->lock);
>> +   list_for_each_entry(attach, &dmabuf->attachments, node)
>> +   max = min(max, dma_get_max_seg_count(attach->dev));
>> +   mutex_unlock(&dmabuf->lock);
>> +
>> +   return max;
>> +}
>> +EXPORT_SYMBOL_GPL(dma_buf_max_seg_count);
>> +
>> +/**
>> + * dma_buf_get_seg_boundary - helper for exporters to get the most
>> + * restrictive segment alignment of all the attached devices
>> + */
>> +unsigned int dma_buf_get_seg_boundary(struct dma_buf *dmabuf)
>> +{
>> +   struct dma_buf_attachment *attach;
>> +   unsigned int mask = (unsigned int)-1;
>> +
>> +   if (WARN_ON(!dmabuf))
>> +   return 0;
>> +
>> +   mutex_lock(&dmabuf->lock);
>> +   list_for_each_entry(attach, &dmabuf->attachments, node)
>> +   mask &= dma_get_seg_boundary(attach->dev);
>> +   mutex_unlock(&dmabuf->lock);
>> +
>> +   return mask;
>> +}
>> +EXPORT_SYMBOL_GPL(dma_buf_get_seg_boundary);
>> +
>> +/**
>>   * dma_buf_attach - Add the device to dma_buf's attachments list; 
>> optionally,
>>   * calls attach() of dma_buf_ops to allow device-specific attach 
>> functionality
>>   * @dmabuf:[in]buffer to attach device to.
>> diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
>> index eb48f38..9533b9b 100644
>> --- a/include/linux/dma-buf.h
>> +++ b/include/linux/dma-buf.h
>> @@ -167,6 +167,10 @@ int dma_buf_fd(struct dma_buf *dmabuf, int flags);
>>  struct dma_buf *dma_buf_get(int fd);
>>  void dma_buf_put(struct dma_buf *dmabuf);
>>
>> +unsigned int dma_buf_max_seg_size(struct dma_buf *dmabuf);
>> +unsigned int dma_buf_max_seg_count(struct dma_buf *dmabuf);
>> +unsigned int dma_buf_get_seg_boundary(struct dma_buf *dmabuf);
>> +
>>  struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *,
>> enum dma_data_direction);
>>  void dma_buf_unmap_attachment(struct dma_buf_attachment *, struct sg_table 
>> *,
>> @@ -220,6 +224,21 @@ static inline void dma_buf_put(struct dma_buf *dmabuf)
>> return;
>>  }
>>
>> +static inline unsigned int dma_buf_max_seg_size(struct dma_buf *dmabuf)
>> +{
>> +   return 0;
>> +}
>> +
>> +static inline unsigned int dma_buf_max_seg_count(struct dma_buf *dmabuf)
>> +{
>> +   return 0;
>> +}
>> +
>> +static inline unsigned int dma_buf_get_seg_boundary(struct dma_buf *dmabuf)
>> +{
>> +   return 0;
>> +}
>> +
>>  static inline struct sg_table *dma_buf_map_attachment(
>> struct dma_buf_attachment *attach, enum dma_data_direction write)
>>  {
>> --
>> 1.7.9.5
>>

Re: [PATCH 2/2] dma-buf: add helpers for attacher dma-parms

[Semwal, Sumit]

On Fri, Jul 20, 2012 at 10:09 PM, Rob Clark  wrote:
> Fyi, Daniel Vetter had suggested on IRC that it would be cleaner to
> have a single helper fxn that most-restrictive union of all attached
> device's dma_parms.  Really this should include dma_mask and
> coherent_dma_mask, I think.  But that touches a lot of other places in
> the code.  If no one objects to the cleanup of moving
> dma_mask/coherent_dma_mask into dma_parms, I'll do this first.
>
> So anyways, don't consider this patch yet for inclusion, I'll make an
> updated one based on dma_parms..
Hi Rob,
Any news on this patch-set?
>
> BR,
> -R
BR,
~Sumit.
>
> On Thu, Jul 19, 2012 at 11:23 AM, Rob Clark  wrote:
>> From: Rob Clark 
>>
>> Add some helpers to iterate through all attachers and get the most
>> restrictive segment size/count/boundary.
>>
>> Signed-off-by: Rob Clark 
>> ---
>>  drivers/base/dma-buf.c  |   63 
>> +++
>>  include/linux/dma-buf.h |   19 ++
>>  2 files changed, 82 insertions(+)
>>
>> diff --git a/drivers/base/dma-buf.c b/drivers/base/dma-buf.c
>> index 24e88fe..757ee20 100644
>> --- a/drivers/base/dma-buf.c
>> +++ b/drivers/base/dma-buf.c
>> @@ -192,6 +192,69 @@ void dma_buf_put(struct dma_buf *dmabuf)
>>  EXPORT_SYMBOL_GPL(dma_buf_put);
>>
>>  /**
>> + * dma_buf_max_seg_size - helper for exporters to get the minimum of
>> + * all attached device's max segment size
>> + */
>> +unsigned int dma_buf_max_seg_size(struct dma_buf *dmabuf)
>> +{
>> +   struct dma_buf_attachment *attach;
>> +   unsigned int max = (unsigned int)-1;
>> +
>> +   if (WARN_ON(!dmabuf))
>> +   return 0;
>> +
>> +   mutex_lock(&dmabuf->lock);
>> +   list_for_each_entry(attach, &dmabuf->attachments, node)
>> +   max = min(max, dma_get_max_seg_size(attach->dev));
>> +   mutex_unlock(&dmabuf->lock);
>> +
>> +   return max;
>> +}
>> +EXPORT_SYMBOL_GPL(dma_buf_max_seg_size);
>> +
>> +/**
>> + * dma_buf_max_seg_count - helper for exporters to get the minimum of
>> + * all attached device's max segment count
>> + */
>> +unsigned int dma_buf_max_seg_count(struct dma_buf *dmabuf)
>> +{
>> +   struct dma_buf_attachment *attach;
>> +   unsigned int max = (unsigned int)-1;
>> +
>> +   if (WARN_ON(!dmabuf))
>> +   return 0;
>> +
>> +   mutex_lock(&dmabuf->lock);
>> +   list_for_each_entry(attach, &dmabuf->attachments, node)
>> +   max = min(max, dma_get_max_seg_count(attach->dev));
>> +   mutex_unlock(&dmabuf->lock);
>> +
>> +   return max;
>> +}
>> +EXPORT_SYMBOL_GPL(dma_buf_max_seg_count);
>> +
>> +/**
>> + * dma_buf_get_seg_boundary - helper for exporters to get the most
>> + * restrictive segment alignment of all the attached devices
>> + */
>> +unsigned int dma_buf_get_seg_boundary(struct dma_buf *dmabuf)
>> +{
>> +   struct dma_buf_attachment *attach;
>> +   unsigned int mask = (unsigned int)-1;
>> +
>> +   if (WARN_ON(!dmabuf))
>> +   return 0;
>> +
>> +   mutex_lock(&dmabuf->lock);
>> +   list_for_each_entry(attach, &dmabuf->attachments, node)
>> +   mask &= dma_get_seg_boundary(attach->dev);
>> +   mutex_unlock(&dmabuf->lock);
>> +
>> +   return mask;
>> +}
>> +EXPORT_SYMBOL_GPL(dma_buf_get_seg_boundary);
>> +
>> +/**
>>   * dma_buf_attach - Add the device to dma_buf's attachments list; 
>> optionally,
>>   * calls attach() of dma_buf_ops to allow device-specific attach 
>> functionality
>>   * @dmabuf:[in]buffer to attach device to.
>> diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
>> index eb48f38..9533b9b 100644
>> --- a/include/linux/dma-buf.h
>> +++ b/include/linux/dma-buf.h
>> @@ -167,6 +167,10 @@ int dma_buf_fd(struct dma_buf *dmabuf, int flags);
>>  struct dma_buf *dma_buf_get(int fd);
>>  void dma_buf_put(struct dma_buf *dmabuf);
>>
>> +unsigned int dma_buf_max_seg_size(struct dma_buf *dmabuf);
>> +unsigned int dma_buf_max_seg_count(struct dma_buf *dmabuf);
>> +unsigned int dma_buf_get_seg_boundary(struct dma_buf *dmabuf);
>> +
>>  struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *,
>> enum dma_data_direction);
>>  void dma_buf_unmap_attachment(struct dma_buf_attachment *, struct sg_table 
>> *,
>> @@ -220,6 +224,21 @@ static inline void dma_buf_put(struct dma_buf *dmabuf)
>> return;
>>  }
>>
>> +static inline unsigned int dma_buf_max_seg_size(struct dma_buf *dmabuf)
>> +{
>> +   return 0;
>> +}
>> +
>> +static inline unsigned int dma_buf_max_seg_count(struct dma_buf *dmabuf)
>> +{
>> +   return 0;
>> +}
>> +
>> +static inline unsigned int dma_buf_get_seg_boundary(struct dma_buf *dmabuf)
>> +{
>> +   return 0;
>> +}
>> +
>>  static inline struct sg_table *dma_buf_map_attachment(
>> struct dma_buf_attachment *attach, enum dma_data_direction write)
>>  {
>> --
>> 1.7.9.5
>>
___
dri-devel mailing l

[RFC 0/3] solving omapdrm/omapdss layering issues

[Semwal, Sumit]

Hi Rob, Tomi,
On Thu, Aug 2, 2012 at 7:46 PM, Rob Clark  wrote:
> On Thu, Aug 2, 2012 at 8:15 AM, Tomi Valkeinen  
> wrote:
>> On Thu, 2012-08-02 at 07:45 -0500, Rob Clark wrote:
>>> On Thu, Aug 2, 2012 at 2:13 AM, Tomi Valkeinen  
>>> wrote:
>>> > On Wed, 2012-08-01 at 09:25 -0500, Rob Clark wrote:
>>> >> On Wed, Aug 1, 2012 at 4:21 AM, Tomi Valkeinen >> >> ti.com> wrote:
>>> >
>>> >> > I guess the fact is that DRM concepts do not really match the OMAP DSS
>>> >> > hardware, and we'll have to use whatever gives us least problems.
>>> >>
>>> >> Actually, I think it does map fairly well to the hardware.. at least
>>> >> more so than to omapdss ;-)
>>> >
>>> > Hm, I'm not sure I understand, omapdss concepts map directly to the
>>> > hardware.
>>>
>>> I think it is mainly exposing the encoder and panel as two separate
>>> entities.. which seems to be what Archit is working on
>>
>> I still don't follow =) They are separate entities. Omapdss models the
>> HW quite directly, I think. It doesn't expose everything, though, as the
>> output drivers (dsi.c, dpi.c etc) are used via the panel drivers.
>
> right.. so we just need to expose the output drivers as separate
> entities, and let omapdrm propagate information such as timings
> between them
>
>>> in case of something like DVI bridge from DPI, this seems pretty
>>> straightforward.. only the connector needs to know about DDC stuff,
>>> which i2c to use and that sort of thing.  So at kms level we would
>>> have (for example) an omap_dpi_encoder which would be the same for DPI
>>> panel (connector) or DPI->DVI bridge.  For HDMI I'm still looking
>>> through the code to see how this would work.  Honestly I've looked
>>> less at this part of code and encoder related registers in the TRM,
>>> compared to the ovl/mgr parts, but at least from the 'DSS overview'
>>> picture in the TRM it seems to make sense ;-)
>>>
>>> KMS even exposes the idea that certain crtcs can connect to only
>>> certain encoders.  Or that you could you could have certain connectors
>>> switched between encoders.  For example if you had a hw w/ DPI out,
>>> and some mux to switch that back and forth between a DPI lcd panel and
>>> a DPI->DVI bridge.  (Ok, I'm not aware of any board that actually does
>>> this, but it is in theory possible.)  So we could expose possible
>>> video chain topologies to userspace in this way.
>>
>> OMAP3 SDP board has such a setup, with manual switch to select between
>> LCD and DVI.
>
> ahh, good to know.. so I'm not crazy for wanting to expose this
> possibility to userspace
>
>>> The other thing is that we don't need to propagate timings from the
>>> panel up to the mgr at the dss level.. kms is already handling this
>>> for us.  In my latest version, which I haven't pushed, I removed the
>>> 'struct omap_overlay_mgr' ptr from 'struct omap_dss_device'.
>>
>> You're thinking only about simple DPI cases. Consider this example, with
>> a DSI-to-DP bridge chip. What we have is the following flow of data:
>>
>> DISPC -> DSI -> DSI-2-DP -> DP monitor
>>
>> The timings you are thinking about are in the DISPC, but here they are
>> only one part of the link. And here the DISPC timings are not actually
>> the timings what the user is interested about. The user wants his
>> timings to be between DSI-2-DP chip and the DP monitor.
>>
>> Timings programmed to DISPC are not the same. The timings for DISPC come
>> from the DSI driver, and they may be very different than the user's
>> timings. With DSI video mode, the DISPC timings would have some
>> resemblance to the user's timings, mainly the time to send one line
>> would be the same. With DSI cmd mode, the DISPC timings would be
>> something totally different, most likely with 0 blank times and as fast
>> pixel clock as possible.
>
> hmm, well kms already has a concept of adjusted_timings, which could
> perhaps be used here to propagate the timings between crtc->encoder..
> although the order is probably backwards from what we want (it comes
> from the crtc to the encoder.. and if I understand properly we want it
> the other way and actually possibly from the connector).  But that
> isn't to say that internally in omapdrm the crtc couldn't get the
> adjusted timings from the connector.  So I still think the parameter
> flow doesn't need to be 'under the hood' in omapdss.
>
> And fwiw, the adjusted_timings stuff is handled by drm_crtc_helper
> fxns, so if the way the core kms handles it isn't what we want, we can
> just plug in our own fxn instead of using drm_crtc_helper_set_mode(),
> so that isn't really a big problem.
>
>> What omapdss does currently is that you set the user's timings to the
>> right side of the chain, which propagate back to DSS. This allows the
>> DSI-2-DP bridge use DSI timings that work optimally for the bridge, and
>> DSI driver will use DISPC timings that work optimally for it.
>>
>> And it's not only about timings above, but also other settings related
>> to the busses between the components. Clock 

Re: [RFC 0/3] solving omapdrm/omapdss layering issues

[Semwal, Sumit]

Hi Rob, Tomi,
On Thu, Aug 2, 2012 at 7:46 PM, Rob Clark  wrote:
> On Thu, Aug 2, 2012 at 8:15 AM, Tomi Valkeinen  wrote:
>> On Thu, 2012-08-02 at 07:45 -0500, Rob Clark wrote:
>>> On Thu, Aug 2, 2012 at 2:13 AM, Tomi Valkeinen  
>>> wrote:
>>> > On Wed, 2012-08-01 at 09:25 -0500, Rob Clark wrote:
>>> >> On Wed, Aug 1, 2012 at 4:21 AM, Tomi Valkeinen  
>>> >> wrote:
>>> >
>>> >> > I guess the fact is that DRM concepts do not really match the OMAP DSS
>>> >> > hardware, and we'll have to use whatever gives us least problems.
>>> >>
>>> >> Actually, I think it does map fairly well to the hardware.. at least
>>> >> more so than to omapdss ;-)
>>> >
>>> > Hm, I'm not sure I understand, omapdss concepts map directly to the
>>> > hardware.
>>>
>>> I think it is mainly exposing the encoder and panel as two separate
>>> entities.. which seems to be what Archit is working on
>>
>> I still don't follow =) They are separate entities. Omapdss models the
>> HW quite directly, I think. It doesn't expose everything, though, as the
>> output drivers (dsi.c, dpi.c etc) are used via the panel drivers.
>
> right.. so we just need to expose the output drivers as separate
> entities, and let omapdrm propagate information such as timings
> between them
>
>>> in case of something like DVI bridge from DPI, this seems pretty
>>> straightforward.. only the connector needs to know about DDC stuff,
>>> which i2c to use and that sort of thing.  So at kms level we would
>>> have (for example) an omap_dpi_encoder which would be the same for DPI
>>> panel (connector) or DPI->DVI bridge.  For HDMI I'm still looking
>>> through the code to see how this would work.  Honestly I've looked
>>> less at this part of code and encoder related registers in the TRM,
>>> compared to the ovl/mgr parts, but at least from the 'DSS overview'
>>> picture in the TRM it seems to make sense ;-)
>>>
>>> KMS even exposes the idea that certain crtcs can connect to only
>>> certain encoders.  Or that you could you could have certain connectors
>>> switched between encoders.  For example if you had a hw w/ DPI out,
>>> and some mux to switch that back and forth between a DPI lcd panel and
>>> a DPI->DVI bridge.  (Ok, I'm not aware of any board that actually does
>>> this, but it is in theory possible.)  So we could expose possible
>>> video chain topologies to userspace in this way.
>>
>> OMAP3 SDP board has such a setup, with manual switch to select between
>> LCD and DVI.
>
> ahh, good to know.. so I'm not crazy for wanting to expose this
> possibility to userspace
>
>>> The other thing is that we don't need to propagate timings from the
>>> panel up to the mgr at the dss level.. kms is already handling this
>>> for us.  In my latest version, which I haven't pushed, I removed the
>>> 'struct omap_overlay_mgr' ptr from 'struct omap_dss_device'.
>>
>> You're thinking only about simple DPI cases. Consider this example, with
>> a DSI-to-DP bridge chip. What we have is the following flow of data:
>>
>> DISPC -> DSI -> DSI-2-DP -> DP monitor
>>
>> The timings you are thinking about are in the DISPC, but here they are
>> only one part of the link. And here the DISPC timings are not actually
>> the timings what the user is interested about. The user wants his
>> timings to be between DSI-2-DP chip and the DP monitor.
>>
>> Timings programmed to DISPC are not the same. The timings for DISPC come
>> from the DSI driver, and they may be very different than the user's
>> timings. With DSI video mode, the DISPC timings would have some
>> resemblance to the user's timings, mainly the time to send one line
>> would be the same. With DSI cmd mode, the DISPC timings would be
>> something totally different, most likely with 0 blank times and as fast
>> pixel clock as possible.
>
> hmm, well kms already has a concept of adjusted_timings, which could
> perhaps be used here to propagate the timings between crtc->encoder..
> although the order is probably backwards from what we want (it comes
> from the crtc to the encoder.. and if I understand properly we want it
> the other way and actually possibly from the connector).  But that
> isn't to say that internally in omapdrm the crtc couldn't get the
> adjusted timings from the connector.  So I still think the parameter
> flow doesn't need to be 'under the hood' in omapdss.
>
> And fwiw, the adjusted_timings stuff is handled by drm_crtc_helper
> fxns, so if the way the core kms handles it isn't what we want, we can
> just plug in our own fxn instead of using drm_crtc_helper_set_mode(),
> so that isn't really a big problem.
>
>> What omapdss does currently is that you set the user's timings to the
>> right side of the chain, which propagate back to DSS. This allows the
>> DSI-2-DP bridge use DSI timings that work optimally for the bridge, and
>> DSI driver will use DISPC timings that work optimally for it.
>>
>> And it's not only about timings above, but also other settings related
>> to the busses between the components. Clock dividers

[PATCH] staging: drm/omap: remove reclaim_buffers callback

[Semwal, Sumit]

On Wed, Aug 1, 2012 at 3:02 PM, Chandrabhanu Mahapatra
 wrote:
> The reclaim_buffers callback has already been removed by Daniel Vetter
>  with his patch "drm: kill reclaim_buffers 
> callback"
> for which the kernel compilation fails with omapdrm support. So, the callback
> for reclaim_buffers is removed from omapdrm.
>
> Signed-off-by: Chandrabhanu Mahapatra 
Reviewed-by: Sumit Semwal 
> ---
>  drivers/staging/omapdrm/omap_drv.c |1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/drivers/staging/omapdrm/omap_drv.c 
> b/drivers/staging/omapdrm/omap_drv.c
> index 342645a..b8e79eb 100644
> --- a/drivers/staging/omapdrm/omap_drv.c
> +++ b/drivers/staging/omapdrm/omap_drv.c
> @@ -778,7 +778,6 @@ static struct drm_driver omap_drm_driver = {
> .irq_postinstall = dev_irq_postinstall,
> .irq_uninstall = dev_irq_uninstall,
> .irq_handler = dev_irq_handler,
> -   .reclaim_buffers = drm_core_reclaim_buffers,
>  #ifdef CONFIG_DEBUG_FS
> .debugfs_init = omap_debugfs_init,
> .debugfs_cleanup = omap_debugfs_cleanup,
> --
> 1.7.10
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] staging: drm/omap: remove reclaim_buffers callback

[Semwal, Sumit]

On Wed, Aug 1, 2012 at 3:02 PM, Chandrabhanu Mahapatra
 wrote:
> The reclaim_buffers callback has already been removed by Daniel Vetter
>  with his patch "drm: kill reclaim_buffers callback"
> for which the kernel compilation fails with omapdrm support. So, the callback
> for reclaim_buffers is removed from omapdrm.
>
> Signed-off-by: Chandrabhanu Mahapatra 
Reviewed-by: Sumit Semwal 
> ---
>  drivers/staging/omapdrm/omap_drv.c |1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/drivers/staging/omapdrm/omap_drv.c 
> b/drivers/staging/omapdrm/omap_drv.c
> index 342645a..b8e79eb 100644
> --- a/drivers/staging/omapdrm/omap_drv.c
> +++ b/drivers/staging/omapdrm/omap_drv.c
> @@ -778,7 +778,6 @@ static struct drm_driver omap_drm_driver = {
> .irq_postinstall = dev_irq_postinstall,
> .irq_uninstall = dev_irq_uninstall,
> .irq_handler = dev_irq_handler,
> -   .reclaim_buffers = drm_core_reclaim_buffers,
>  #ifdef CONFIG_DEBUG_FS
> .debugfs_init = omap_debugfs_init,
> .debugfs_cleanup = omap_debugfs_cleanup,
> --
> 1.7.10
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCHv6 00/13] Integration of videobuf2 with dmabuf

[Semwal, Sumit]

On Tue, May 29, 2012 at 6:25 AM, Laurent Pinchart
 wrote:
> Hi Tomasz,
Hi Tomasz, Laurent, Mauro,
>
> On Wednesday 23 May 2012 14:10:14 Tomasz Stanislawski wrote:
>> Hello everyone,
>> This patchset adds support for DMABUF [2] importing to V4L2 stack.
>> The support for DMABUF exporting was moved to separate patchset
>> due to dependency on patches for DMA mapping redesign by
>> Marek Szyprowski [4].
>
> Except for the small issue with patches 01/13 and 02/13, the set is ready for
> upstream as far as I'm concerned.
+1; Mauro: how do you think about this series? Getting it landed into
3.5 would make life lot easier :)
>
>> v6:
>> - fixed missing entry in v4l2_memory_names
>> - fixed a bug occuring after get_user_pages failure
>
> I've missed that one, what was it ?
>
>> - fixed a bug caused by using invalid vma for get_user_pages
>> - prepare/finish no longer call dma_sync for dmabuf buffers
>>
>> v5:
>> - removed change of importer/exporter behaviour
>> - fixes vb2_dc_pages_to_sgt basing on Laurent's hints
>> - changed pin/unpin words to lock/unlock in Doc
>>
>> v4:
>> - rebased on mainline 3.4-rc2
>> - included missing importing support for s5p-fimc and s5p-tv
>> - added patch for changing map/unmap for importers
>> - fixes to Documentation part
>> - coding style fixes
>> - pairing {map/unmap}_dmabuf in vb2-core
>> - fixing variable types and semantic of arguments in videobufb2-dma-contig.c
>>
>> v3:
>> - rebased on mainline 3.4-rc1
>> - split 'code refactor' patch to multiple smaller patches
>> - squashed fixes to Sumit's patches
>> - patchset is no longer dependant on 'DMA mapping redesign'
>> - separated path for handling IO and non-IO mappings
>> - add documentation for DMABUF importing to V4L
>> - removed all DMABUF exporter related code
>> - removed usage of dma_get_pages extension
>>
>> v2:
>> - extended VIDIOC_EXPBUF argument from integer memoffset to struct
>> ? v4l2_exportbuffer
>> - added patch that breaks DMABUF spec on (un)map_atachment callcacks but
>> allows to work with existing implementation of DMABUF prime in DRM
>> - all dma-contig code refactoring patches were squashed
>> - bugfixes
>>
>> v1: List of changes since [1].
>> - support for DMA api extension dma_get_pages, the function is used to
>> retrieve pages used to create DMA mapping.
>> - small fixes/code cleanup to videobuf2
>> - added prepare and finish callbacks to vb2 allocators, it is used keep
>> ? consistency between dma-cpu acess to the memory (by Marek Szyprowski)
>> - support for exporting of DMABUF buffer in V4L2 and Videobuf2, originated
>> from [3].
>> - support for dma-buf exporting in vb2-dma-contig allocator
>> - support for DMABUF for s5p-tv and s5p-fimc (capture interface) drivers,
>> ? originated from [3]
>> - changed handling for userptr buffers (by Marek Szyprowski, Andrzej
>> ? Pietrasiewicz)
>> - let mmap method to use dma_mmap_writecombine call (by Marek Szyprowski)
>>
>> [1]
>> http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/4296
>> 6/focus=42968 [2] https://lkml.org/lkml/2011/12/26/29
>> [3]
>> http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/3635
>> 4/focus=36355 [4]
>> http://thread.gmane.org/gmane.linux.kernel.cross-arch/12819
>>
>> Laurent Pinchart (2):
>> ? v4l: vb2-dma-contig: Shorten vb2_dma_contig prefix to vb2_dc
>> ? v4l: vb2-dma-contig: Reorder functions
>>
>> Marek Szyprowski (2):
>> ? v4l: vb2: add prepare/finish callbacks to allocators
>> ? v4l: vb2-dma-contig: add prepare/finish to dma-contig allocator
>>
>> Sumit Semwal (4):
>> ? v4l: Add DMABUF as a memory type
>> ? v4l: vb2: add support for shared buffer (dma_buf)
>> ? v4l: vb: remove warnings about MEMORY_DMABUF
>> ? v4l: vb2-dma-contig: add support for dma_buf importing
>>
>> Tomasz Stanislawski (5):
>> ? Documentation: media: description of DMABUF importing in V4L2
>> ? v4l: vb2-dma-contig: Remove unneeded allocation context structure
>> ? v4l: vb2-dma-contig: add support for scatterlist in userptr mode
>> ? v4l: s5p-tv: mixer: support for dmabuf importing
>> ? v4l: s5p-fimc: support for dmabuf importing
>>
>> ?Documentation/DocBook/media/v4l/compat.xml ? ? ? ? | ? ?4 +
>> ?Documentation/DocBook/media/v4l/io.xml ? ? ? ? ? ? | ?179 +++
>> ?.../DocBook/media/v4l/vidioc-create-bufs.xml ? ? ? | ? ?1 +
>> ?Documentation/DocBook/media/v4l/vidioc-qbuf.xml ? ?| ? 15 +
>> ?Documentation/DocBook/media/v4l/vidioc-reqbufs.xml | ? 45 +-
>> ?drivers/media/video/s5p-fimc/Kconfig ? ? ? ? ? ? ? | ? ?1 +
>> ?drivers/media/video/s5p-fimc/fimc-capture.c ? ? ? ?| ? ?2 +-
>> ?drivers/media/video/s5p-tv/Kconfig ? ? ? ? ? ? ? ? | ? ?1 +
>> ?drivers/media/video/s5p-tv/mixer_video.c ? ? ? ? ? | ? ?2 +-
>> ?drivers/media/video/v4l2-ioctl.c ? ? ? ? ? ? ? ? ? | ? ?1 +
>> ?drivers/media/video/videobuf-core.c ? ? ? ? ? ? ? ?| ? ?4 +
>> ?drivers/media/video/videobuf2-core.c ? ? ? ? ? ? ? | ?207 +++-
>> ?drivers/media/video/videobuf2-dma-contig.c ? ? ? ? | ?520 ++---
>> ?include/linux/videodev2.h ? ? ? 

Re: [PATCHv6 00/13] Integration of videobuf2 with dmabuf

[Semwal, Sumit]

On Tue, May 29, 2012 at 6:25 AM, Laurent Pinchart
 wrote:
> Hi Tomasz,
Hi Tomasz, Laurent, Mauro,
>
> On Wednesday 23 May 2012 14:10:14 Tomasz Stanislawski wrote:
>> Hello everyone,
>> This patchset adds support for DMABUF [2] importing to V4L2 stack.
>> The support for DMABUF exporting was moved to separate patchset
>> due to dependency on patches for DMA mapping redesign by
>> Marek Szyprowski [4].
>
> Except for the small issue with patches 01/13 and 02/13, the set is ready for
> upstream as far as I'm concerned.
+1; Mauro: how do you think about this series? Getting it landed into
3.5 would make life lot easier :)
>
>> v6:
>> - fixed missing entry in v4l2_memory_names
>> - fixed a bug occuring after get_user_pages failure
>
> I've missed that one, what was it ?
>
>> - fixed a bug caused by using invalid vma for get_user_pages
>> - prepare/finish no longer call dma_sync for dmabuf buffers
>>
>> v5:
>> - removed change of importer/exporter behaviour
>> - fixes vb2_dc_pages_to_sgt basing on Laurent's hints
>> - changed pin/unpin words to lock/unlock in Doc
>>
>> v4:
>> - rebased on mainline 3.4-rc2
>> - included missing importing support for s5p-fimc and s5p-tv
>> - added patch for changing map/unmap for importers
>> - fixes to Documentation part
>> - coding style fixes
>> - pairing {map/unmap}_dmabuf in vb2-core
>> - fixing variable types and semantic of arguments in videobufb2-dma-contig.c
>>
>> v3:
>> - rebased on mainline 3.4-rc1
>> - split 'code refactor' patch to multiple smaller patches
>> - squashed fixes to Sumit's patches
>> - patchset is no longer dependant on 'DMA mapping redesign'
>> - separated path for handling IO and non-IO mappings
>> - add documentation for DMABUF importing to V4L
>> - removed all DMABUF exporter related code
>> - removed usage of dma_get_pages extension
>>
>> v2:
>> - extended VIDIOC_EXPBUF argument from integer memoffset to struct
>>   v4l2_exportbuffer
>> - added patch that breaks DMABUF spec on (un)map_atachment callcacks but
>> allows to work with existing implementation of DMABUF prime in DRM
>> - all dma-contig code refactoring patches were squashed
>> - bugfixes
>>
>> v1: List of changes since [1].
>> - support for DMA api extension dma_get_pages, the function is used to
>> retrieve pages used to create DMA mapping.
>> - small fixes/code cleanup to videobuf2
>> - added prepare and finish callbacks to vb2 allocators, it is used keep
>>   consistency between dma-cpu acess to the memory (by Marek Szyprowski)
>> - support for exporting of DMABUF buffer in V4L2 and Videobuf2, originated
>> from [3].
>> - support for dma-buf exporting in vb2-dma-contig allocator
>> - support for DMABUF for s5p-tv and s5p-fimc (capture interface) drivers,
>>   originated from [3]
>> - changed handling for userptr buffers (by Marek Szyprowski, Andrzej
>>   Pietrasiewicz)
>> - let mmap method to use dma_mmap_writecombine call (by Marek Szyprowski)
>>
>> [1]
>> http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/4296
>> 6/focus=42968 [2] https://lkml.org/lkml/2011/12/26/29
>> [3]
>> http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/3635
>> 4/focus=36355 [4]
>> http://thread.gmane.org/gmane.linux.kernel.cross-arch/12819
>>
>> Laurent Pinchart (2):
>>   v4l: vb2-dma-contig: Shorten vb2_dma_contig prefix to vb2_dc
>>   v4l: vb2-dma-contig: Reorder functions
>>
>> Marek Szyprowski (2):
>>   v4l: vb2: add prepare/finish callbacks to allocators
>>   v4l: vb2-dma-contig: add prepare/finish to dma-contig allocator
>>
>> Sumit Semwal (4):
>>   v4l: Add DMABUF as a memory type
>>   v4l: vb2: add support for shared buffer (dma_buf)
>>   v4l: vb: remove warnings about MEMORY_DMABUF
>>   v4l: vb2-dma-contig: add support for dma_buf importing
>>
>> Tomasz Stanislawski (5):
>>   Documentation: media: description of DMABUF importing in V4L2
>>   v4l: vb2-dma-contig: Remove unneeded allocation context structure
>>   v4l: vb2-dma-contig: add support for scatterlist in userptr mode
>>   v4l: s5p-tv: mixer: support for dmabuf importing
>>   v4l: s5p-fimc: support for dmabuf importing
>>
>>  Documentation/DocBook/media/v4l/compat.xml         |    4 +
>>  Documentation/DocBook/media/v4l/io.xml             |  179 +++
>>  .../DocBook/media/v4l/vidioc-create-bufs.xml       |    1 +
>>  Documentation/DocBook/media/v4l/vidioc-qbuf.xml    |   15 +
>>  Documentation/DocBook/media/v4l/vidioc-reqbufs.xml |   45 +-
>>  drivers/media/video/s5p-fimc/Kconfig               |    1 +
>>  drivers/media/video/s5p-fimc/fimc-capture.c        |    2 +-
>>  drivers/media/video/s5p-tv/Kconfig                 |    1 +
>>  drivers/media/video/s5p-tv/mixer_video.c           |    2 +-
>>  drivers/media/video/v4l2-ioctl.c                   |    1 +
>>  drivers/media/video/videobuf-core.c                |    4 +
>>  drivers/media/video/videobuf2-core.c               |  207 +++-
>>  drivers/media/video/videobuf2-dma-contig.c         |  520 ++---
>>  include/linux/videodev2.h       

[PATCH 2/4] drm/exynos: remove unused codes in hdmi and mixer driver

[Semwal, Sumit]

Hi Joonyoung,

On Thu, Apr 5, 2012 at 3:53 PM, Joonyoung Shim  
wrote:
> Some defines and members in struct mixer_context aren't used, remove
> them.
>
> Signed-off-by: Joonyoung Shim 
> Signed-off-by: Kyungmin Park 
> ---
> ?drivers/gpu/drm/exynos/exynos_hdmi.c ?| ? ?1 -
> ?drivers/gpu/drm/exynos/exynos_mixer.c | ? 21 +
> ?2 files changed, 9 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c 
> b/drivers/gpu/drm/exynos/exynos_hdmi.c
> index 0a71317..340424f 100644
> --- a/drivers/gpu/drm/exynos/exynos_hdmi.c
> +++ b/drivers/gpu/drm/exynos/exynos_hdmi.c
> @@ -40,7 +40,6 @@
>
> ?#include "exynos_hdmi.h"
>
> -#define HDMI_OVERLAY_NUMBER ? ?3
> ?#define MAX_WIDTH ? ? ? ? ? ? ?1920
> ?#define MAX_HEIGHT ? ? ? ? ? ? 1080
> ?#define get_hdmi_context(dev) ?platform_get_drvdata(to_platform_device(dev))
> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c 
> b/drivers/gpu/drm/exynos/exynos_mixer.c
> index 495a7af..563092e 100644
> --- a/drivers/gpu/drm/exynos/exynos_mixer.c
> +++ b/drivers/gpu/drm/exynos/exynos_mixer.c
> @@ -37,7 +37,8 @@
> ?#include "exynos_drm_drv.h"
> ?#include "exynos_drm_hdmi.h"
>
> -#define HDMI_OVERLAY_NUMBER ? ?3
> +#define MIXER_WIN_NR ? ? ? ? ? 3
> +#define MIXER_DEFAULT_WIN ? ? ?0
Patch description says 'remove unused codes...', but looks like you're
adding more codes here? I think you should do the addition / deletion
in separate patches.
>
> ?#define get_mixer_context(dev) platform_get_drvdata(to_platform_device(dev))
>
> @@ -75,16 +76,12 @@ struct mixer_resources {
> ?};
>
> ?struct mixer_context {
> - ? ? ? struct fb_videomode ? ? *default_timing;
> - ? ? ? unsigned int ? ? ? ? ? ?default_win;
> - ? ? ? unsigned int ? ? ? ? ? ?default_bpp;
> ? ? ? ?unsigned int ? ? ? ? ? ?irq;
> ? ? ? ?int ? ? ? ? ? ? ? ? ? ? pipe;
> ? ? ? ?bool ? ? ? ? ? ? ? ? ? ?interlace;
> - ? ? ? bool ? ? ? ? ? ? ? ? ? ?vp_enabled;
>
> ? ? ? ?struct mixer_resources ?mixer_res;
> - ? ? ? struct hdmi_win_data ? ?win_data[HDMI_OVERLAY_NUMBER];
> + ? ? ? struct hdmi_win_data ? ?win_data[MIXER_WIN_NR];
> ?};
>
> ?static const u8 filter_y_horiz_tap8[] = {
> @@ -643,9 +640,9 @@ static void mixer_win_mode_set(void *ctx,
>
> ? ? ? ?win = overlay->zpos;
> ? ? ? ?if (win == DEFAULT_ZPOS)
> - ? ? ? ? ? ? ? win = mixer_ctx->default_win;
> + ? ? ? ? ? ? ? win = MIXER_DEFAULT_WIN;
>
> - ? ? ? if (win < 0 || win > HDMI_OVERLAY_NUMBER) {
> + ? ? ? if (win < 0 || win > MIXER_WIN_NR) {
> ? ? ? ? ? ? ? ?DRM_ERROR("overlay plane[%d] is wrong\n", win);
> ? ? ? ? ? ? ? ?return;
> ? ? ? ?}
> @@ -683,9 +680,9 @@ static void mixer_win_commit(void *ctx, int zpos)
> ? ? ? ?DRM_DEBUG_KMS("[%d] %s, win: %d\n", __LINE__, __func__, win);
>
> ? ? ? ?if (win == DEFAULT_ZPOS)
> - ? ? ? ? ? ? ? win = mixer_ctx->default_win;
> + ? ? ? ? ? ? ? win = MIXER_DEFAULT_WIN;
>
> - ? ? ? if (win < 0 || win > HDMI_OVERLAY_NUMBER) {
> + ? ? ? if (win < 0 || win > MIXER_WIN_NR) {
> ? ? ? ? ? ? ? ?DRM_ERROR("overlay plane[%d] is wrong\n", win);
> ? ? ? ? ? ? ? ?return;
> ? ? ? ?}
> @@ -706,9 +703,9 @@ static void mixer_win_disable(void *ctx, int zpos)
> ? ? ? ?DRM_DEBUG_KMS("[%d] %s, win: %d\n", __LINE__, __func__, win);
>
> ? ? ? ?if (win == DEFAULT_ZPOS)
> - ? ? ? ? ? ? ? win = mixer_ctx->default_win;
> + ? ? ? ? ? ? ? win = MIXER_DEFAULT_WIN;
>
> - ? ? ? if (win < 0 || win > HDMI_OVERLAY_NUMBER) {
> + ? ? ? if (win < 0 || win > MIXER_WIN_NR) {
> ? ? ? ? ? ? ? ?DRM_ERROR("overlay plane[%d] is wrong\n", win);
> ? ? ? ? ? ? ? ?return;
> ? ? ? ?}
> --
> 1.7.5.4
>
> ___
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
Best regards,
~Sumit.

Re: [PATCH 2/4] drm/exynos: remove unused codes in hdmi and mixer driver

[Semwal, Sumit]

Hi Joonyoung,

On Thu, Apr 5, 2012 at 3:53 PM, Joonyoung Shim  wrote:
> Some defines and members in struct mixer_context aren't used, remove
> them.
>
> Signed-off-by: Joonyoung Shim 
> Signed-off-by: Kyungmin Park 
> ---
>  drivers/gpu/drm/exynos/exynos_hdmi.c  |    1 -
>  drivers/gpu/drm/exynos/exynos_mixer.c |   21 +
>  2 files changed, 9 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c 
> b/drivers/gpu/drm/exynos/exynos_hdmi.c
> index 0a71317..340424f 100644
> --- a/drivers/gpu/drm/exynos/exynos_hdmi.c
> +++ b/drivers/gpu/drm/exynos/exynos_hdmi.c
> @@ -40,7 +40,6 @@
>
>  #include "exynos_hdmi.h"
>
> -#define HDMI_OVERLAY_NUMBER    3
>  #define MAX_WIDTH              1920
>  #define MAX_HEIGHT             1080
>  #define get_hdmi_context(dev)  platform_get_drvdata(to_platform_device(dev))
> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c 
> b/drivers/gpu/drm/exynos/exynos_mixer.c
> index 495a7af..563092e 100644
> --- a/drivers/gpu/drm/exynos/exynos_mixer.c
> +++ b/drivers/gpu/drm/exynos/exynos_mixer.c
> @@ -37,7 +37,8 @@
>  #include "exynos_drm_drv.h"
>  #include "exynos_drm_hdmi.h"
>
> -#define HDMI_OVERLAY_NUMBER    3
> +#define MIXER_WIN_NR           3
> +#define MIXER_DEFAULT_WIN      0
Patch description says 'remove unused codes...', but looks like you're
adding more codes here? I think you should do the addition / deletion
in separate patches.
>
>  #define get_mixer_context(dev) platform_get_drvdata(to_platform_device(dev))
>
> @@ -75,16 +76,12 @@ struct mixer_resources {
>  };
>
>  struct mixer_context {
> -       struct fb_videomode     *default_timing;
> -       unsigned int            default_win;
> -       unsigned int            default_bpp;
>        unsigned int            irq;
>        int                     pipe;
>        bool                    interlace;
> -       bool                    vp_enabled;
>
>        struct mixer_resources  mixer_res;
> -       struct hdmi_win_data    win_data[HDMI_OVERLAY_NUMBER];
> +       struct hdmi_win_data    win_data[MIXER_WIN_NR];
>  };
>
>  static const u8 filter_y_horiz_tap8[] = {
> @@ -643,9 +640,9 @@ static void mixer_win_mode_set(void *ctx,
>
>        win = overlay->zpos;
>        if (win == DEFAULT_ZPOS)
> -               win = mixer_ctx->default_win;
> +               win = MIXER_DEFAULT_WIN;
>
> -       if (win < 0 || win > HDMI_OVERLAY_NUMBER) {
> +       if (win < 0 || win > MIXER_WIN_NR) {
>                DRM_ERROR("overlay plane[%d] is wrong\n", win);
>                return;
>        }
> @@ -683,9 +680,9 @@ static void mixer_win_commit(void *ctx, int zpos)
>        DRM_DEBUG_KMS("[%d] %s, win: %d\n", __LINE__, __func__, win);
>
>        if (win == DEFAULT_ZPOS)
> -               win = mixer_ctx->default_win;
> +               win = MIXER_DEFAULT_WIN;
>
> -       if (win < 0 || win > HDMI_OVERLAY_NUMBER) {
> +       if (win < 0 || win > MIXER_WIN_NR) {
>                DRM_ERROR("overlay plane[%d] is wrong\n", win);
>                return;
>        }
> @@ -706,9 +703,9 @@ static void mixer_win_disable(void *ctx, int zpos)
>        DRM_DEBUG_KMS("[%d] %s, win: %d\n", __LINE__, __func__, win);
>
>        if (win == DEFAULT_ZPOS)
> -               win = mixer_ctx->default_win;
> +               win = MIXER_DEFAULT_WIN;
>
> -       if (win < 0 || win > HDMI_OVERLAY_NUMBER) {
> +       if (win < 0 || win > MIXER_WIN_NR) {
>                DRM_ERROR("overlay plane[%d] is wrong\n", win);
>                return;
>        }
> --
> 1.7.5.4
>
> ___
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
Best regards,
~Sumit.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/3] dma-buf: add support for kernel cpu access

[Semwal, Sumit]

Hi Daniel,
On Tue, Mar 6, 2012 at 12:27 AM, Daniel Vetter  
wrote:
> On Fri, Mar 2, 2012 at 23:24, Rob Clark  wrote:
>> Perhaps we should check somewhere for required dmabuf ops fxns (like
>> kmap_atomic here), rather than just calling unconditionally what might
>> be a null ptr. ?At least put it in the WARN_ON(), but it might be
>> nicer to catch a missing required fxns at export time, rather than
>> waiting for an importer to try and call it. ?Less likely that way, for
>> newly added required functions go unnoticed.
>>
>> (same comment applies below for the non-atomic variant.. and possibly
>> some other existing dmabuf ops)
>
> Agreed, I'll rework the patch to do that when rebasing onto Sumit's latest 
> tree.
In addition, you'd not need to check for !dmabuf->ops since the export
should already catch it.

As I sent in the other mail a while back, could you please rebase on
for-next at git://git.linaro.org/people/sumitsemwal/linux-dma-buf.git

Best regards,
~Sumit.
> -Daniel
> --

Re: [PATCH 2/3] dma-buf: add support for kernel cpu access

[Semwal, Sumit]

Hi Daniel,
On Tue, Mar 6, 2012 at 12:27 AM, Daniel Vetter  wrote:
> On Fri, Mar 2, 2012 at 23:24, Rob Clark  wrote:
>> Perhaps we should check somewhere for required dmabuf ops fxns (like
>> kmap_atomic here), rather than just calling unconditionally what might
>> be a null ptr.  At least put it in the WARN_ON(), but it might be
>> nicer to catch a missing required fxns at export time, rather than
>> waiting for an importer to try and call it.  Less likely that way, for
>> newly added required functions go unnoticed.
>>
>> (same comment applies below for the non-atomic variant.. and possibly
>> some other existing dmabuf ops)
>
> Agreed, I'll rework the patch to do that when rebasing onto Sumit's latest 
> tree.
In addition, you'd not need to check for !dmabuf->ops since the export
should already catch it.

As I sent in the other mail a while back, could you please rebase on
for-next at git://git.linaro.org/people/sumitsemwal/linux-dma-buf.git

Best regards,
~Sumit.
> -Daniel
> --
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Kernel Display and Video API Consolidation mini-summit at ELC 2012 - Notes

[Semwal, Sumit]

Hello Laurent, Everyone:


On Fri, Feb 17, 2012 at 4:55 AM, Laurent Pinchart
 wrote:
> Hello everybody,
>
> First of all, I would like to thank all the attendees for their participation
> in the mini-summit that helped make the meeting a success.
>

> *** ?dma-buf Implementation in V4L2 ***
>
> ?Goal: Implement the dma-buf API in V4L2.
>
> ?Sumit Semwal has submitted patches to implement the dma-buf importer role in
> ?videobuf2. Tomasz Stanislawski has then submitted incremental patches to add
> ?exporter role support.
>
> ?Action points:
> ?- Create a git branch to host all the latest patches. Sumit will provide
> ? ?that.
>
>
Against my Action Item: I have created the following branch at my
github (obviously, it is an RFC branch only)

tree: git://github.com/sumitsemwal/kernel-omap4.git
branch: 3.3rc3-v4l2-dmabuf-RFCv1

As the name partially suggests, it is based out of:
3.3-rc3 +
dmav6 [1] +
some minor dma-buf updates [2] +
my v4l2-as-importer RFC [3] +
Tomasz' RFC for v4l2-as-exporter (and related patches) [4]

Since Tomasz' RFC had a patch-pair which first removed and then added
drivers/media/video/videobuf2-dma-contig.c file, I 'combined' these
into one - but since the patch-pair heavily refactored the file, I am
not able to take responsibility of completeness / correctness of the
same.

[1]: 
http://git.infradead.org/users/kmpark/linux-samsung/shortlog/refs/heads/3.3-rc2-dma-v6
[2]: git://git.linaro.org/people/sumitsemwal/linux-3.x.git 'dev' branch
[3]: 
http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/42966/focus=42968
[4]: 
http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/43793

Best regards,
~Sumit.

Re: Kernel Display and Video API Consolidation mini-summit at ELC 2012 - Notes

[Semwal, Sumit]

Hello Laurent, Everyone:


On Fri, Feb 17, 2012 at 4:55 AM, Laurent Pinchart
 wrote:
> Hello everybody,
>
> First of all, I would like to thank all the attendees for their participation
> in the mini-summit that helped make the meeting a success.
>

> ***  dma-buf Implementation in V4L2 ***
>
>  Goal: Implement the dma-buf API in V4L2.
>
>  Sumit Semwal has submitted patches to implement the dma-buf importer role in
>  videobuf2. Tomasz Stanislawski has then submitted incremental patches to add
>  exporter role support.
>
>  Action points:
>  - Create a git branch to host all the latest patches. Sumit will provide
>    that.
>
>
Against my Action Item: I have created the following branch at my
github (obviously, it is an RFC branch only)

tree: git://github.com/sumitsemwal/kernel-omap4.git
branch: 3.3rc3-v4l2-dmabuf-RFCv1

As the name partially suggests, it is based out of:
3.3-rc3 +
dmav6 [1] +
some minor dma-buf updates [2] +
my v4l2-as-importer RFC [3] +
Tomasz' RFC for v4l2-as-exporter (and related patches) [4]

Since Tomasz' RFC had a patch-pair which first removed and then added
drivers/media/video/videobuf2-dma-contig.c file, I 'combined' these
into one - but since the patch-pair heavily refactored the file, I am
not able to take responsibility of completeness / correctness of the
same.

[1]: 
http://git.infradead.org/users/kmpark/linux-samsung/shortlog/refs/heads/3.3-rc2-dma-v6
[2]: git://git.linaro.org/people/sumitsemwal/linux-3.x.git 'dev' branch
[3]: 
http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/42966/focus=42968
[4]: 
http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/43793

Best regards,
~Sumit.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] dma-buf: add dma_data_direction to unmap dma_buf_op

[Semwal, Sumit]

Hi Laurent,

On Jan 30, 2012 7:48 PM, "Laurent Pinchart" <
laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Sumit,
>
> On Friday 27 January 2012 10:43:28 Sumit Semwal wrote:
> > Some exporters may use DMA map/unmap APIs in dma-buf ops, which require
> > enum dma_data_direction for both map and unmap operations.
> >
> > Thus, the unmap dma_buf_op also needs to have enum dma_data_direction as
> > a parameter.
> >
> > Reported-by: Tomasz Stanislawski 
> > Signed-off-by: Sumit Semwal 
> > ---
> >  drivers/base/dma-buf.c  |7 +--
> >  include/linux/dma-buf.h |8 +---
> >  2 files changed, 10 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/base/dma-buf.c b/drivers/base/dma-buf.c
> > index 8afe2dd..c9a945f 100644
> > --- a/drivers/base/dma-buf.c
> > +++ b/drivers/base/dma-buf.c
> > @@ -271,16 +271,19 @@ EXPORT_SYMBOL_GPL(dma_buf_map_attachment);
> >   * dma_buf_ops.
> >   * @attach:  [in]attachment to unmap buffer from
> >   * @sg_table:[in]scatterlist info of the buffer to unmap
> > + * @direction:  [in]direction of DMA transfer
> >   *
> >   */
> >  void dma_buf_unmap_attachment(struct dma_buf_attachment *attach,
> > - struct sg_table *sg_table)
> > + struct sg_table *sg_table,
> > + enum dma_data_direction direction)
> >  {
> >   if (WARN_ON(!attach || !attach->dmabuf || !sg_table))
> >   return;
> >
> >   mutex_lock(&attach->dmabuf->lock);
> > - attach->dmabuf->ops->unmap_dma_buf(attach, sg_table);
> > + attach->dmabuf->ops->unmap_dma_buf(attach, sg_table,
> > + direction);
> >   mutex_unlock(&attach->dmabuf->lock);
> >
> >  }
> > diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
> > index 86f6241..847b026 100644
> > --- a/include/linux/dma-buf.h
> > +++ b/include/linux/dma-buf.h
> > @@ -63,7 +63,8 @@ struct dma_buf_ops {
> >   struct sg_table * (*map_dma_buf)(struct dma_buf_attachment *,
> >   enum dma_data_direction);
> >   void (*unmap_dma_buf)(struct dma_buf_attachment *,
> > - struct sg_table *);
> > + struct sg_table *,
> > + enum dma_data_direction);
> >   /* TODO: Add try_map_dma_buf version, to return immed with -EBUSY
> >* if the call would block.
> >*/
> > @@ -122,7 +123,8 @@ void dma_buf_put(struct dma_buf *dmabuf);
> >
> >  struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *,
> >   enum dma_data_direction);
> > -void dma_buf_unmap_attachment(struct dma_buf_attachment *, struct
sg_table
> > *); +void dma_buf_unmap_attachment(struct dma_buf_attachment *, struct
> > sg_table *, + enum dma_data_direction);
> >  #else
> >
> >  static inline struct dma_buf_attachment *dma_buf_attach(struct dma_buf
> > *dmabuf, @@ -166,7 +168,7 @@ static inline struct sg_table
> > *dma_buf_map_attachment( }
> >
> >  static inline void dma_buf_unmap_attachment(struct dma_buf_attachment
> > *attach, -struct sg_table
*sg)
> > + struct sg_table *sg, enum dma_data_direction
write)
>
> s/write/dir/ (or direction) ?
>
:-) sure.

> >  {
> >   return;
> >  }
>
> --
> Regards,
>
> Laurent Pinchart
Best regards,
-Sumit.
-- next part --
An HTML attachment was scrubbed...
URL: 

Re: [PATCH] dma-buf: add dma_data_direction to unmap dma_buf_op

[Semwal, Sumit]

Hi Laurent,

On Jan 30, 2012 7:48 PM, "Laurent Pinchart" <
laurent.pinch...@ideasonboard.com> wrote:
>
> Hi Sumit,
>
> On Friday 27 January 2012 10:43:28 Sumit Semwal wrote:
> > Some exporters may use DMA map/unmap APIs in dma-buf ops, which require
> > enum dma_data_direction for both map and unmap operations.
> >
> > Thus, the unmap dma_buf_op also needs to have enum dma_data_direction as
> > a parameter.
> >
> > Reported-by: Tomasz Stanislawski 
> > Signed-off-by: Sumit Semwal 
> > ---
> >  drivers/base/dma-buf.c  |7 +--
> >  include/linux/dma-buf.h |8 +---
> >  2 files changed, 10 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/base/dma-buf.c b/drivers/base/dma-buf.c
> > index 8afe2dd..c9a945f 100644
> > --- a/drivers/base/dma-buf.c
> > +++ b/drivers/base/dma-buf.c
> > @@ -271,16 +271,19 @@ EXPORT_SYMBOL_GPL(dma_buf_map_attachment);
> >   * dma_buf_ops.
> >   * @attach:  [in]attachment to unmap buffer from
> >   * @sg_table:[in]scatterlist info of the buffer to unmap
> > + * @direction:  [in]direction of DMA transfer
> >   *
> >   */
> >  void dma_buf_unmap_attachment(struct dma_buf_attachment *attach,
> > - struct sg_table *sg_table)
> > + struct sg_table *sg_table,
> > + enum dma_data_direction direction)
> >  {
> >   if (WARN_ON(!attach || !attach->dmabuf || !sg_table))
> >   return;
> >
> >   mutex_lock(&attach->dmabuf->lock);
> > - attach->dmabuf->ops->unmap_dma_buf(attach, sg_table);
> > + attach->dmabuf->ops->unmap_dma_buf(attach, sg_table,
> > + direction);
> >   mutex_unlock(&attach->dmabuf->lock);
> >
> >  }
> > diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
> > index 86f6241..847b026 100644
> > --- a/include/linux/dma-buf.h
> > +++ b/include/linux/dma-buf.h
> > @@ -63,7 +63,8 @@ struct dma_buf_ops {
> >   struct sg_table * (*map_dma_buf)(struct dma_buf_attachment *,
> >   enum dma_data_direction);
> >   void (*unmap_dma_buf)(struct dma_buf_attachment *,
> > - struct sg_table *);
> > + struct sg_table *,
> > + enum dma_data_direction);
> >   /* TODO: Add try_map_dma_buf version, to return immed with -EBUSY
> >* if the call would block.
> >*/
> > @@ -122,7 +123,8 @@ void dma_buf_put(struct dma_buf *dmabuf);
> >
> >  struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *,
> >   enum dma_data_direction);
> > -void dma_buf_unmap_attachment(struct dma_buf_attachment *, struct
sg_table
> > *); +void dma_buf_unmap_attachment(struct dma_buf_attachment *, struct
> > sg_table *, + enum dma_data_direction);
> >  #else
> >
> >  static inline struct dma_buf_attachment *dma_buf_attach(struct dma_buf
> > *dmabuf, @@ -166,7 +168,7 @@ static inline struct sg_table
> > *dma_buf_map_attachment( }
> >
> >  static inline void dma_buf_unmap_attachment(struct dma_buf_attachment
> > *attach, -struct sg_table
*sg)
> > + struct sg_table *sg, enum dma_data_direction
write)
>
> s/write/dir/ (or direction) ?
>
:-) sure.

> >  {
> >   return;
> >  }
>
> --
> Regards,
>
> Laurent Pinchart
Best regards,
-Sumit.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] dma-buf: add dma_data_direction to unmap dma_buf_op

[Semwal, Sumit]

Please ignore! I will send out a new version in a minute.

Thanks and best regards,
~Sumit.



On Fri, Jan 27, 2012 at 3:04 PM, Sumit Semwal  wrote:
> Some exporters may use DMA map/unmap APIs in dma-buf ops, which require
> enum dma_data_direction while unmapping.
>
> Thus, the unmap dma_buf_op also needs to have enum dma_data_direction as
> a parameter.
>
> Reported-by: Tomasz Stanislawski 
> Signed-off-by: Sumit Semwal 
> ---
> ?drivers/base/Kconfig ? ?| ? ?2 +-
> ?drivers/base/dma-buf.c ?| ? ?7 +--
> ?include/linux/dma-buf.h | ? ?8 +---
> ?3 files changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
> index 7be9f79..5edc5db 100644
> --- a/drivers/base/Kconfig
> +++ b/drivers/base/Kconfig
> @@ -179,7 +179,7 @@ config GENERIC_CPU_DEVICES
> ?source "drivers/base/regmap/Kconfig"
>
> ?config DMA_SHARED_BUFFER
> - ? ? ? bool
> + ? ? ? bool "Temporary mechanism to enable build of dma-buf"
> ? ? ? ?default n
> ? ? ? ?select ANON_INODES
> ? ? ? ?depends on EXPERIMENTAL
> diff --git a/drivers/base/dma-buf.c b/drivers/base/dma-buf.c
> index 8afe2dd..c9a945f 100644
> --- a/drivers/base/dma-buf.c
> +++ b/drivers/base/dma-buf.c
> @@ -271,16 +271,19 @@ EXPORT_SYMBOL_GPL(dma_buf_map_attachment);
> ?* dma_buf_ops.
> ?* @attach: ? ?[in] ? ?attachment to unmap buffer from
> ?* @sg_table: ?[in] ? ?scatterlist info of the buffer to unmap
> + * @direction: ?[in] ? ?direction of DMA transfer
> ?*
> ?*/
> ?void dma_buf_unmap_attachment(struct dma_buf_attachment *attach,
> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? struct sg_table *sg_table)
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? struct sg_table *sg_table,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? enum dma_data_direction direction)
> ?{
> ? ? ? ?if (WARN_ON(!attach || !attach->dmabuf || !sg_table))
> ? ? ? ? ? ? ? ?return;
>
> ? ? ? ?mutex_lock(&attach->dmabuf->lock);
> - ? ? ? attach->dmabuf->ops->unmap_dma_buf(attach, sg_table);
> + ? ? ? attach->dmabuf->ops->unmap_dma_buf(attach, sg_table,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? direction);
> ? ? ? ?mutex_unlock(&attach->dmabuf->lock);
>
> ?}
> diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
> index 86f6241..847b026 100644
> --- a/include/linux/dma-buf.h
> +++ b/include/linux/dma-buf.h
> @@ -63,7 +63,8 @@ struct dma_buf_ops {
> ? ? ? ?struct sg_table * (*map_dma_buf)(struct dma_buf_attachment *,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?enum dma_data_direction);
> ? ? ? ?void (*unmap_dma_buf)(struct dma_buf_attachment *,
> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? struct sg_table *);
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? struct sg_table *,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? enum dma_data_direction);
> ? ? ? ?/* TODO: Add try_map_dma_buf version, to return immed with -EBUSY
> ? ? ? ? * if the call would block.
> ? ? ? ? */
> @@ -122,7 +123,8 @@ void dma_buf_put(struct dma_buf *dmabuf);
>
> ?struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?enum dma_data_direction);
> -void dma_buf_unmap_attachment(struct dma_buf_attachment *, struct sg_table 
> *);
> +void dma_buf_unmap_attachment(struct dma_buf_attachment *, struct sg_table *,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? enum dma_data_direction);
> ?#else
>
> ?static inline struct dma_buf_attachment *dma_buf_attach(struct dma_buf 
> *dmabuf,
> @@ -166,7 +168,7 @@ static inline struct sg_table *dma_buf_map_attachment(
> ?}
>
> ?static inline void dma_buf_unmap_attachment(struct dma_buf_attachment 
> *attach,
> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? struct sg_table *sg)
> + ? ? ? ? ? ? ? ? ? ? ? struct sg_table *sg, enum dma_data_direction write)
> ?{
> ? ? ? ?return;
> ?}
> --
> 1.7.5.4
>

Re: [PATCH] dma-buf: add dma_data_direction to unmap dma_buf_op

[Semwal, Sumit]

Please ignore! I will send out a new version in a minute.

Thanks and best regards,
~Sumit.



On Fri, Jan 27, 2012 at 3:04 PM, Sumit Semwal  wrote:
> Some exporters may use DMA map/unmap APIs in dma-buf ops, which require
> enum dma_data_direction while unmapping.
>
> Thus, the unmap dma_buf_op also needs to have enum dma_data_direction as
> a parameter.
>
> Reported-by: Tomasz Stanislawski 
> Signed-off-by: Sumit Semwal 
> ---
>  drivers/base/Kconfig    |    2 +-
>  drivers/base/dma-buf.c  |    7 +--
>  include/linux/dma-buf.h |    8 +---
>  3 files changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
> index 7be9f79..5edc5db 100644
> --- a/drivers/base/Kconfig
> +++ b/drivers/base/Kconfig
> @@ -179,7 +179,7 @@ config GENERIC_CPU_DEVICES
>  source "drivers/base/regmap/Kconfig"
>
>  config DMA_SHARED_BUFFER
> -       bool
> +       bool "Temporary mechanism to enable build of dma-buf"
>        default n
>        select ANON_INODES
>        depends on EXPERIMENTAL
> diff --git a/drivers/base/dma-buf.c b/drivers/base/dma-buf.c
> index 8afe2dd..c9a945f 100644
> --- a/drivers/base/dma-buf.c
> +++ b/drivers/base/dma-buf.c
> @@ -271,16 +271,19 @@ EXPORT_SYMBOL_GPL(dma_buf_map_attachment);
>  * dma_buf_ops.
>  * @attach:    [in]    attachment to unmap buffer from
>  * @sg_table:  [in]    scatterlist info of the buffer to unmap
> + * @direction:  [in]    direction of DMA transfer
>  *
>  */
>  void dma_buf_unmap_attachment(struct dma_buf_attachment *attach,
> -                               struct sg_table *sg_table)
> +                               struct sg_table *sg_table,
> +                               enum dma_data_direction direction)
>  {
>        if (WARN_ON(!attach || !attach->dmabuf || !sg_table))
>                return;
>
>        mutex_lock(&attach->dmabuf->lock);
> -       attach->dmabuf->ops->unmap_dma_buf(attach, sg_table);
> +       attach->dmabuf->ops->unmap_dma_buf(attach, sg_table,
> +                                               direction);
>        mutex_unlock(&attach->dmabuf->lock);
>
>  }
> diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
> index 86f6241..847b026 100644
> --- a/include/linux/dma-buf.h
> +++ b/include/linux/dma-buf.h
> @@ -63,7 +63,8 @@ struct dma_buf_ops {
>        struct sg_table * (*map_dma_buf)(struct dma_buf_attachment *,
>                                                enum dma_data_direction);
>        void (*unmap_dma_buf)(struct dma_buf_attachment *,
> -                                               struct sg_table *);
> +                                               struct sg_table *,
> +                                               enum dma_data_direction);
>        /* TODO: Add try_map_dma_buf version, to return immed with -EBUSY
>         * if the call would block.
>         */
> @@ -122,7 +123,8 @@ void dma_buf_put(struct dma_buf *dmabuf);
>
>  struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *,
>                                        enum dma_data_direction);
> -void dma_buf_unmap_attachment(struct dma_buf_attachment *, struct sg_table 
> *);
> +void dma_buf_unmap_attachment(struct dma_buf_attachment *, struct sg_table *,
> +                               enum dma_data_direction);
>  #else
>
>  static inline struct dma_buf_attachment *dma_buf_attach(struct dma_buf 
> *dmabuf,
> @@ -166,7 +168,7 @@ static inline struct sg_table *dma_buf_map_attachment(
>  }
>
>  static inline void dma_buf_unmap_attachment(struct dma_buf_attachment 
> *attach,
> -                                               struct sg_table *sg)
> +                       struct sg_table *sg, enum dma_data_direction write)
>  {
>        return;
>  }
> --
> 1.7.5.4
>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/3] dma-buf: Introduce dma buffer sharing mechanism

[Semwal, Sumit]

On Thu, Jan 26, 2012 at 1:37 AM, Daniel Vetter  wrote:
> On Wed, Jan 25, 2012 at 06:02:41PM +0100, Tomasz Stanislawski wrote:
>> Hi Sumit,
>>
>> On 12/26/2011 10:23 AM, Sumit Semwal wrote:
>> >This is the first step in defining a dma buffer sharing mechanism.
>> >
>> [snip]
>>
>> >+struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *,
>> >+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?enum dma_data_direction);
>> >+void dma_buf_unmap_attachment(struct dma_buf_attachment *, struct sg_table 
>> >*);
>>
>> I think that you should add enum dma_data_direction as an argument
>> unmap function. It was mentioned that the dma_buf_attachment should keep
>> cached and mapped sg_table for performance reasons. The field
>> dma_buf_attachment::priv seams to be a natural place to keep this sg_table.
>> To map a buffer the exporter calls dma_map_sg. It needs dma direction
>> as an argument. The problem is that dma_unmap_sg also needs this
>> argument but dma direction is not available neither in
>> dma_buf_unmap_attachment nor in unmap callback. Therefore the exporter
>> is forced to embed returned sg_table into a bigger structure where
>> dma direction is remembered. Refer to function vb2_dc_dmabuf_ops_map
>> at
>
> Oops, makes sense. I've totally overlooked that we need to pass in the dma
> direction also for the unmap call to the dma subsystem. Sumit, can you
> stitch together that small patch?

Right, of course. I will do that by tomorrow; it is a bank holiday
today here in India, so.

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

Re: [PATCH 1/3] dma-buf: Introduce dma buffer sharing mechanism

[Semwal, Sumit]

On Thu, Jan 26, 2012 at 1:37 AM, Daniel Vetter  wrote:
> On Wed, Jan 25, 2012 at 06:02:41PM +0100, Tomasz Stanislawski wrote:
>> Hi Sumit,
>>
>> On 12/26/2011 10:23 AM, Sumit Semwal wrote:
>> >This is the first step in defining a dma buffer sharing mechanism.
>> >
>> [snip]
>>
>> >+struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *,
>> >+                                    enum dma_data_direction);
>> >+void dma_buf_unmap_attachment(struct dma_buf_attachment *, struct sg_table 
>> >*);
>>
>> I think that you should add enum dma_data_direction as an argument
>> unmap function. It was mentioned that the dma_buf_attachment should keep
>> cached and mapped sg_table for performance reasons. The field
>> dma_buf_attachment::priv seams to be a natural place to keep this sg_table.
>> To map a buffer the exporter calls dma_map_sg. It needs dma direction
>> as an argument. The problem is that dma_unmap_sg also needs this
>> argument but dma direction is not available neither in
>> dma_buf_unmap_attachment nor in unmap callback. Therefore the exporter
>> is forced to embed returned sg_table into a bigger structure where
>> dma direction is remembered. Refer to function vb2_dc_dmabuf_ops_map
>> at
>
> Oops, makes sense. I've totally overlooked that we need to pass in the dma
> direction also for the unmap call to the dma subsystem. Sumit, can you
> stitch together that small patch?

Right, of course. I will do that by tomorrow; it is a bank holiday
today here in India, so.

regards,
~Sumit.
> -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

[Linaro-mm-sig] [PATCH 1/3] dma-buf: Introduce dma buffer sharing mechanism

[Semwal, Sumit]

On Fri, Jan 20, 2012 at 6:53 PM, Laurent Pinchart
 wrote:
> Hi Summit,
>
> Sorry for the late review. I know that this code is now in mainline, but I
> still have a couple of comments. I'll send patches if you agree with them.
Hi Laurent,

Thanks for your review; apologies for being late in replying - I was
OoO for last couple of days.
>
> On Monday 26 December 2011 10:23:15 Sumit Semwal wrote:

>>
>
> [snip]
>
>> diff --git a/drivers/base/dma-buf.c b/drivers/base/dma-buf.c
>> new file mode 100644
>> index 000..e38ad24
>> --- /dev/null
>> +++ b/drivers/base/dma-buf.c
>> @@ -0,0 +1,291 @@
>
> [snip]
>
>> +/**
>> + * dma_buf_export - Creates a new dma_buf, and associates an anon file
>> + * with this buffer, so it can be exported.
>> + * Also connect the allocator specific data and ops to the buffer.
>> + *
>> + * @priv: ? ?[in] ? ?Attach private data of allocator to this buffer
>> + * @ops: ? ? [in] ? ?Attach allocator-defined dma buf ops to the new buffer.
>> + * @size: ? ?[in] ? ?Size of the buffer
>> + * @flags: ? [in] ? ?mode flags for the file.
>> + *
>> + * Returns, on success, a newly created dma_buf object, which wraps the
>> + * supplied private data and operations for dma_buf_ops. On either missing
>> + * ops, or error in allocating struct dma_buf, will return negative error.
>> + *
>> + */
>> +struct dma_buf *dma_buf_export(void *priv, struct dma_buf_ops *ops,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? size_t size, int flags)
>> +{
>> + ? ? struct dma_buf *dmabuf;
>> + ? ? struct file *file;
>> +
>> + ? ? if (WARN_ON(!priv || !ops
>> + ? ? ? ? ? ? ? ? ? ? ? || !ops->map_dma_buf
>> + ? ? ? ? ? ? ? ? ? ? ? || !ops->unmap_dma_buf
>> + ? ? ? ? ? ? ? ? ? ? ? || !ops->release)) {
>> + ? ? ? ? ? ? return ERR_PTR(-EINVAL);
>> + ? ? }
>> +
>> + ? ? dmabuf = kzalloc(sizeof(struct dma_buf), GFP_KERNEL);
>> + ? ? if (dmabuf == NULL)
>> + ? ? ? ? ? ? return ERR_PTR(-ENOMEM);
>> +
>> + ? ? dmabuf->priv = priv;
>> + ? ? dmabuf->ops = ops;
>
> dmabuf->ops will never but NULL, but (see below)
>

>> +struct dma_buf_attachment *dma_buf_attach(struct dma_buf *dmabuf,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? struct device *dev)
>> +{
>> + ? ? struct dma_buf_attachment *attach;
>> + ? ? int ret;
>> +
>> + ? ? if (WARN_ON(!dmabuf || !dev || !dmabuf->ops))
>
> you still check dmabuf->ops here, as well as in several places below.
> Shouldn't these checks be removed ?
You're right - these can be removed.
>
>> + ? ? ? ? ? ? return ERR_PTR(-EINVAL);
>> +
>> + ? ? attach = kzalloc(sizeof(struct dma_buf_attachment), GFP_KERNEL);
>> + ? ? if (attach == NULL)
>> + ? ? ? ? ? ? goto err_alloc;
>
> What about returning ERR_PTR(-ENOMEM) directly here ?
>
Right; we can do that.
>> +
>> + ? ? mutex_lock(&dmabuf->lock);
>> +
>> + ? ? attach->dev = dev;
>> + ? ? attach->dmabuf = dmabuf;
>
> These two lines can be moved before mutex_lock().
>
:) Yes - thanks for catching this.

> --
> Regards,
>
> Laurent Pinchart

Let me know if you'd send patches for these, or should I just go ahead
and correct.

Best regards,
~Sumit.

[PATCH] dma-buf: Use EXPORT_SYMBOL

[Semwal, Sumit]

On Sat, Jan 21, 2012 at 11:02 PM, Daniel Vetter  wrote:
> On Fri, Jan 20, 2012 at 10:04:57AM -0800, Robert Morell wrote:
>> On Wed, Jan 18, 2012 at 01:10:04AM -0800, Semwal, Sumit wrote:
>> > On Wed, Jan 18, 2012 at 5:38 AM, Robert Morell  
>> > wrote:
>> > > EXPORT_SYMBOL_GPL is intended to be used for "an internal implementation
>> > > issue, and not really an interface". ?The dma-buf infrastructure is
>> > > explicitly intended as an interface between modules/drivers, so it
>> > > should use EXPORT_SYMBOL instead.
>> >
>> > + Konrad, Arnd, Mauro: there were strong objections on using
>> > EXPORT_SYMBOL in place of EXPORT_SYMBOL_GPL by all 3 of them; I
>> > suggest we first arrive at a consensus before merging this patch.
>>
>> This discussion seems to have stagnated; how do we move forward here?
>>
>> Sumit, as the primary author and new maintainer (congrats!) of the
>> dma-buf infrastructure, it seems like it's really your call how to
>> proceed. ?I'd still like to see this be something that we can use from
>> the nvidia and fglrx drivers for Xorg buffer sharing, as I and Dave have
>> argued in this thread. ?It really seems to me that this change on a
>> technical level won't have any adverse effect on the scenarios where it
>> can be used today, but it will allow it to be used more widely, which
>> will prevent duplication and fragmentation in the future and be greatly
>> appreciated by users of hardware such as Optimus.
>
> Given that I've participated quite a bit in the design of dma_buf as-is,
> let me throw in my totally irrelevant opinion, too ;-)
>
> I'll refrain from comment on the actual patch, it's obviously a hot topic.
> Furthermore I might need to ask Intel's legal dep for guidance to asses
> things wrt my own contributions to dma_buf.
>
> Otoh I'd like nvidia to be on board, especially when we're desingned
> additions to dma_buf required to make it really work for multiple gpus. In
> additions it looks like that the nvidia blob will only be an importer of a
> dma_buf, at least for the use-cases discussed here.
>
> So why don't you just ditch this patch here and add a small shim to your
> blob to interface with drm's prime as an importing driver? I personally
> would deem that acceptable and I think Dave wouldn't mind too much,
> either.

Hi Everyone!

(Apologies for delay in replying; was OoO for past couple of days)

Thanks very much for this discussion - a couple of things:

1. I am definitely willing to make changes as needed to get as many
devices / subsystems / frameworks to use the dma-buf infrastructure.
This could include changing the way symbols are exported, if that is
the *only* way to get things done.

2. With that premise, I quite like the idea that Daniel gave (of
course, in his capacity as one of the top contributors behind dma-buf
infrastructure, and like he said, not as an Intel employee) - so let
me ask the following:

Robert, Dave,
Technically speaking, is there no way that the EXPORT_SYMBOL_GPLed
symbols can be used by the binary blobs, possibly with an open-sourced
shim which provides the buffer-sharing interface to the binary blobs?
Are there any reasons to not consider this approach?

Also, if some of you are going to be at ELC mid-Feb at SFO, we could
meet up face-to-face and thrash out possible ways forward.
>
> Yours, Daniel
Thanks, and best regards,
~Sumit.
>
> Disclaimer: This is my own opinion and I do not speak as an Intel employee
> here.
> --
> Daniel Vetter
> Mail: daniel at ffwll.ch
> Mobile: +41 (0)79 365 57 48

Re: [Linaro-mm-sig] [PATCH 1/3] dma-buf: Introduce dma buffer sharing mechanism

[Semwal, Sumit]

On Fri, Jan 20, 2012 at 6:53 PM, Laurent Pinchart
 wrote:
> Hi Summit,
>
> Sorry for the late review. I know that this code is now in mainline, but I
> still have a couple of comments. I'll send patches if you agree with them.
Hi Laurent,

Thanks for your review; apologies for being late in replying - I was
OoO for last couple of days.
>
> On Monday 26 December 2011 10:23:15 Sumit Semwal wrote:

>>
>
> [snip]
>
>> diff --git a/drivers/base/dma-buf.c b/drivers/base/dma-buf.c
>> new file mode 100644
>> index 000..e38ad24
>> --- /dev/null
>> +++ b/drivers/base/dma-buf.c
>> @@ -0,0 +1,291 @@
>
> [snip]
>
>> +/**
>> + * dma_buf_export - Creates a new dma_buf, and associates an anon file
>> + * with this buffer, so it can be exported.
>> + * Also connect the allocator specific data and ops to the buffer.
>> + *
>> + * @priv:    [in]    Attach private data of allocator to this buffer
>> + * @ops:     [in]    Attach allocator-defined dma buf ops to the new buffer.
>> + * @size:    [in]    Size of the buffer
>> + * @flags:   [in]    mode flags for the file.
>> + *
>> + * Returns, on success, a newly created dma_buf object, which wraps the
>> + * supplied private data and operations for dma_buf_ops. On either missing
>> + * ops, or error in allocating struct dma_buf, will return negative error.
>> + *
>> + */
>> +struct dma_buf *dma_buf_export(void *priv, struct dma_buf_ops *ops,
>> +                             size_t size, int flags)
>> +{
>> +     struct dma_buf *dmabuf;
>> +     struct file *file;
>> +
>> +     if (WARN_ON(!priv || !ops
>> +                       || !ops->map_dma_buf
>> +                       || !ops->unmap_dma_buf
>> +                       || !ops->release)) {
>> +             return ERR_PTR(-EINVAL);
>> +     }
>> +
>> +     dmabuf = kzalloc(sizeof(struct dma_buf), GFP_KERNEL);
>> +     if (dmabuf == NULL)
>> +             return ERR_PTR(-ENOMEM);
>> +
>> +     dmabuf->priv = priv;
>> +     dmabuf->ops = ops;
>
> dmabuf->ops will never but NULL, but (see below)
>

>> +struct dma_buf_attachment *dma_buf_attach(struct dma_buf *dmabuf,
>> +                                       struct device *dev)
>> +{
>> +     struct dma_buf_attachment *attach;
>> +     int ret;
>> +
>> +     if (WARN_ON(!dmabuf || !dev || !dmabuf->ops))
>
> you still check dmabuf->ops here, as well as in several places below.
> Shouldn't these checks be removed ?
You're right - these can be removed.
>
>> +             return ERR_PTR(-EINVAL);
>> +
>> +     attach = kzalloc(sizeof(struct dma_buf_attachment), GFP_KERNEL);
>> +     if (attach == NULL)
>> +             goto err_alloc;
>
> What about returning ERR_PTR(-ENOMEM) directly here ?
>
Right; we can do that.
>> +
>> +     mutex_lock(&dmabuf->lock);
>> +
>> +     attach->dev = dev;
>> +     attach->dmabuf = dmabuf;
>
> These two lines can be moved before mutex_lock().
>
:) Yes - thanks for catching this.

> --
> Regards,
>
> Laurent Pinchart

Let me know if you'd send patches for these, or should I just go ahead
and correct.

Best regards,
~Sumit.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] dma-buf: Use EXPORT_SYMBOL

[Semwal, Sumit]

On Sat, Jan 21, 2012 at 11:02 PM, Daniel Vetter  wrote:
> On Fri, Jan 20, 2012 at 10:04:57AM -0800, Robert Morell wrote:
>> On Wed, Jan 18, 2012 at 01:10:04AM -0800, Semwal, Sumit wrote:
>> > On Wed, Jan 18, 2012 at 5:38 AM, Robert Morell  wrote:
>> > > EXPORT_SYMBOL_GPL is intended to be used for "an internal implementation
>> > > issue, and not really an interface".  The dma-buf infrastructure is
>> > > explicitly intended as an interface between modules/drivers, so it
>> > > should use EXPORT_SYMBOL instead.
>> >
>> > + Konrad, Arnd, Mauro: there were strong objections on using
>> > EXPORT_SYMBOL in place of EXPORT_SYMBOL_GPL by all 3 of them; I
>> > suggest we first arrive at a consensus before merging this patch.
>>
>> This discussion seems to have stagnated; how do we move forward here?
>>
>> Sumit, as the primary author and new maintainer (congrats!) of the
>> dma-buf infrastructure, it seems like it's really your call how to
>> proceed.  I'd still like to see this be something that we can use from
>> the nvidia and fglrx drivers for Xorg buffer sharing, as I and Dave have
>> argued in this thread.  It really seems to me that this change on a
>> technical level won't have any adverse effect on the scenarios where it
>> can be used today, but it will allow it to be used more widely, which
>> will prevent duplication and fragmentation in the future and be greatly
>> appreciated by users of hardware such as Optimus.
>
> Given that I've participated quite a bit in the design of dma_buf as-is,
> let me throw in my totally irrelevant opinion, too ;-)
>
> I'll refrain from comment on the actual patch, it's obviously a hot topic.
> Furthermore I might need to ask Intel's legal dep for guidance to asses
> things wrt my own contributions to dma_buf.
>
> Otoh I'd like nvidia to be on board, especially when we're desingned
> additions to dma_buf required to make it really work for multiple gpus. In
> additions it looks like that the nvidia blob will only be an importer of a
> dma_buf, at least for the use-cases discussed here.
>
> So why don't you just ditch this patch here and add a small shim to your
> blob to interface with drm's prime as an importing driver? I personally
> would deem that acceptable and I think Dave wouldn't mind too much,
> either.

Hi Everyone!

(Apologies for delay in replying; was OoO for past couple of days)

Thanks very much for this discussion - a couple of things:

1. I am definitely willing to make changes as needed to get as many
devices / subsystems / frameworks to use the dma-buf infrastructure.
This could include changing the way symbols are exported, if that is
the *only* way to get things done.

2. With that premise, I quite like the idea that Daniel gave (of
course, in his capacity as one of the top contributors behind dma-buf
infrastructure, and like he said, not as an Intel employee) - so let
me ask the following:

Robert, Dave,
Technically speaking, is there no way that the EXPORT_SYMBOL_GPLed
symbols can be used by the binary blobs, possibly with an open-sourced
shim which provides the buffer-sharing interface to the binary blobs?
Are there any reasons to not consider this approach?

Also, if some of you are going to be at ELC mid-Feb at SFO, we could
meet up face-to-face and thrash out possible ways forward.
>
> Yours, Daniel
Thanks, and best regards,
~Sumit.
>
> Disclaimer: This is my own opinion and I do not speak as an Intel employee
> here.
> --
> 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

[GIT PULL]: dma-buf tree: maintainer update

[Semwal, Sumit]

Hi Linus,

Post the merge of dma-buf tree that was (very kindly) sent by Dave
Airlie, various people involved in this project feel it is natural and
practical for me to be the maintainer of this code.

This is my first pull request to you, which only changes the
MAINTAINERS file - could you please pull from it? [If you'd just
prefer the patch, I could post that out as well.]

Thanks and best regards,
~Sumit.

The following changes since commit dcd6c92267155e70a94b3927bce681ce74b80d1f:

  Linux 3.3-rc1 (2012-01-19 15:04:48 -0800)

are available in the git repository at:
  git://git.linaro.org/people/sumitsemwal/linux-dma-buf.git for-linus-3.3

Sumit Semwal (1):
  MAINTAINERS: Add dma-buf sharing framework maintainer

 MAINTAINERS |   11 +++
 1 files changed, 11 insertions(+), 0 deletions(-)

[GIT PULL]: dma-buf tree: maintainer update

[Semwal, Sumit]

Hi Linus,

Post the merge of dma-buf tree that was (very kindly) sent by Dave
Airlie, various people involved in this project feel it is natural and
practical for me to be the maintainer of this code.

This is my first pull request to you, which only changes the
MAINTAINERS file - could you please pull from it? [If you'd just
prefer the patch, I could post that out as well.]

Thanks and best regards,
~Sumit.

The following changes since commit dcd6c92267155e70a94b3927bce681ce74b80d1f:

  Linux 3.3-rc1 (2012-01-19 15:04:48 -0800)

are available in the git repository at:
  git://git.linaro.org/people/sumitsemwal/linux-dma-buf.git for-linus-3.3

Sumit Semwal (1):
  MAINTAINERS: Add dma-buf sharing framework maintainer

 MAINTAINERS |   11 +++
 1 files changed, 11 insertions(+), 0 deletions(-)
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] dma-buf: Use EXPORT_SYMBOL

[Semwal, Sumit]

On Wed, Jan 18, 2012 at 5:38 AM, Robert Morell  wrote:
> EXPORT_SYMBOL_GPL is intended to be used for "an internal implementation
> issue, and not really an interface". ?The dma-buf infrastructure is
> explicitly intended as an interface between modules/drivers, so it
> should use EXPORT_SYMBOL instead.

+ Konrad, Arnd, Mauro: there were strong objections on using
EXPORT_SYMBOL in place of EXPORT_SYMBOL_GPL by all 3 of them; I
suggest we first arrive at a consensus before merging this patch.

Best regards,
~Sumit.

Re: [PATCH] dma-buf: Use EXPORT_SYMBOL

[Semwal, Sumit]

On Wed, Jan 18, 2012 at 5:38 AM, Robert Morell  wrote:
> EXPORT_SYMBOL_GPL is intended to be used for "an internal implementation
> issue, and not really an interface".  The dma-buf infrastructure is
> explicitly intended as an interface between modules/drivers, so it
> should use EXPORT_SYMBOL instead.

+ Konrad, Arnd, Mauro: there were strong objections on using
EXPORT_SYMBOL in place of EXPORT_SYMBOL_GPL by all 3 of them; I
suggest we first arrive at a consensus before merging this patch.

Best regards,
~Sumit.

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

[RFC v2 1/2] dma-buf: Introduce dma buffer sharing mechanism

[Semwal, Sumit]

On Tue, Jan 10, 2012 at 7:44 AM, Rob Clark  wrote:
> On Mon, Jan 9, 2012 at 7:34 PM, InKi Dae  wrote:
>> 2012/1/10 Rob Clark :
>> at least with no IOMMU, the memory information(containing physical
>> memory address) would be copied to vb2_xx_buf object if drm gem
>> exported its own buffer and vb2 wants to use that buffer at this time,
>> sg table is used to share that buffer. and the problem I pointed out
>> is that this buffer(also physical memory region) could be released by
>> vb2 framework(as you know, vb2_xx_buf object and the memory region for
>> buf->dma_addr pointing) but the Exporter(drm gem) couldn't know that
>> so some problems would be induced once drm gem tries to release or
>> access that buffer. and I have tried to resolve this issue adding
>> get_shared_cnt() callback to dma-buf.h but I'm not sure that this is
>> good way. maybe there would be better way.
Hi Inki,
As also mentioned in the documentation patch, importer (the user of
the buffer) - in this case for current RFC patches on
v4l2-as-a-user[1] vb2 framework - shouldn't release the backing memory
of the buffer directly - it should only use the dma-buf callbacks in
the right sequence to let the exporter know that it is done using this
buffer, so the exporter can release it if allowed and needed.
>
> the exporter (in this case your driver's drm/gem bits) shouldn't
> release that mapping / sgtable until the importer (in this case v4l2)
> calls dma_buf_unmap fxn..
>
> It would be an error if the importer did a dma_buf_put() without first
> calling dma_buf_unmap_attachment() (if currently mapped) and then
> dma_buf_detach() (if currently attached). ?Perhaps somewhere there
> should be some sanity checking debug code which could be enabled to do
> a WARN_ON() if the importer does the wrong thing. ?It shouldn't really
> be part of the API, I don't think, but it actually does seem like a
> good thing, esp. as new drivers start trying to use dmabuf, to have
> some debug options which could be enabled.
>
> It is entirely possible that something was missed on the vb2 patches,
> but the way it is intended to work is like this:
> https://github.com/robclark/kernel-omap4/blob/0961428143cd10269223e3d0f24bc3a66a96185f/drivers/media/video/videobuf2-core.c#L92
>
> where it does a detach() before the dma_buf_put(), and the vb2-contig
> backend checks here that it is also unmapped():
> https://github.com/robclark/kernel-omap4/blob/0961428143cd10269223e3d0f24bc3a66a96185f/drivers/media/video/videobuf2-dma-contig.c#L251

The proposed RFC for V4L2 adaptation at [1] does exactly the same
thing; detach() before dma_buf_put(), and check in vb2-contig backend
for unmapped() as mentioned above.

>
> BR,
> -R
>
BR,
Sumit.

[1]: V4l2 as a dma-buf user RFC:
http://comments.gmane.org/gmane.linux.drivers.video-input-infrastructure/42966

Re: [RFC v2 1/2] dma-buf: Introduce dma buffer sharing mechanism

[Semwal, Sumit]

On Tue, Jan 10, 2012 at 7:44 AM, Rob Clark  wrote:
> On Mon, Jan 9, 2012 at 7:34 PM, InKi Dae  wrote:
>> 2012/1/10 Rob Clark :
>> at least with no IOMMU, the memory information(containing physical
>> memory address) would be copied to vb2_xx_buf object if drm gem
>> exported its own buffer and vb2 wants to use that buffer at this time,
>> sg table is used to share that buffer. and the problem I pointed out
>> is that this buffer(also physical memory region) could be released by
>> vb2 framework(as you know, vb2_xx_buf object and the memory region for
>> buf->dma_addr pointing) but the Exporter(drm gem) couldn't know that
>> so some problems would be induced once drm gem tries to release or
>> access that buffer. and I have tried to resolve this issue adding
>> get_shared_cnt() callback to dma-buf.h but I'm not sure that this is
>> good way. maybe there would be better way.
Hi Inki,
As also mentioned in the documentation patch, importer (the user of
the buffer) - in this case for current RFC patches on
v4l2-as-a-user[1] vb2 framework - shouldn't release the backing memory
of the buffer directly - it should only use the dma-buf callbacks in
the right sequence to let the exporter know that it is done using this
buffer, so the exporter can release it if allowed and needed.
>
> the exporter (in this case your driver's drm/gem bits) shouldn't
> release that mapping / sgtable until the importer (in this case v4l2)
> calls dma_buf_unmap fxn..
>
> It would be an error if the importer did a dma_buf_put() without first
> calling dma_buf_unmap_attachment() (if currently mapped) and then
> dma_buf_detach() (if currently attached).  Perhaps somewhere there
> should be some sanity checking debug code which could be enabled to do
> a WARN_ON() if the importer does the wrong thing.  It shouldn't really
> be part of the API, I don't think, but it actually does seem like a
> good thing, esp. as new drivers start trying to use dmabuf, to have
> some debug options which could be enabled.
>
> It is entirely possible that something was missed on the vb2 patches,
> but the way it is intended to work is like this:
> https://github.com/robclark/kernel-omap4/blob/0961428143cd10269223e3d0f24bc3a66a96185f/drivers/media/video/videobuf2-core.c#L92
>
> where it does a detach() before the dma_buf_put(), and the vb2-contig
> backend checks here that it is also unmapped():
> https://github.com/robclark/kernel-omap4/blob/0961428143cd10269223e3d0f24bc3a66a96185f/drivers/media/video/videobuf2-dma-contig.c#L251

The proposed RFC for V4L2 adaptation at [1] does exactly the same
thing; detach() before dma_buf_put(), and check in vb2-contig backend
for unmapped() as mentioned above.

>
> BR,
> -R
>
BR,
Sumit.

[1]: V4l2 as a dma-buf user RFC:
http://comments.gmane.org/gmane.linux.drivers.video-input-infrastructure/42966
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[Linaro-mm-sig] [RFC v3 0/2] Introduce DMA buffer sharing mechanism

[Semwal, Sumit]

On Fri, Dec 23, 2011 at 10:50 PM, Rob Clark  wrote:
> On Fri, Dec 23, 2011 at 4:08 AM, Semwal, Sumit  wrote:
>> On Wed, Dec 21, 2011 at 1:50 AM, Dave Airlie  wrote:
>> 
>>>>
>>>> Hence for both patches:
>>>> Reviewed-by: Daniel Vetter 
>>>
>>> Yeah I'm with Daniel, I like this one, I can definitely build the drm
>>> buffer sharing layer on top of this.
>>>
>>> How do we see this getting merged? I'm quite happy to push it to Linus
>>> if we don't have an identified path, though it could go via a Linaro
>>> tree as well.
>>>
>>> so feel free to add:
>>> Reviewed-by: Dave Airlie 
>> Thanks Daniel and Dave!
>>
>> I guess we can start with staging for 3.3, and see how it shapes up. I
>> will post the latest patch version pretty soon.
>
> not sure about staging, but could make sense to mark as experimental.
Thanks, I will mark it experimental for the first version; we can
remove that once it is more widely used and tested.
>
>> Arnd, Dave: do you have any preference on the path it takes to get
>> merged? In my mind, Linaro tree might make more sense, but I would
>> leave it upto you gentlemen.
>
> Looks like Dave is making some progress on drm usage of buffer sharing
> between gpu's.. if that is ready to go in at the same time, it might
> be a bit logistically simpler for him to put dmabuf in the same pull
> req. ?I don't have strong preference one way or another, so do what is
> collectively simpler ;-)
:) Right - I am quite happy for it to get merged in either ways :)
>
> BR,
> -R
Best regards,
~Sumit.

Re: [Linaro-mm-sig] [RFC v3 0/2] Introduce DMA buffer sharing mechanism

[Semwal, Sumit]

On Fri, Dec 23, 2011 at 10:50 PM, Rob Clark  wrote:
> On Fri, Dec 23, 2011 at 4:08 AM, Semwal, Sumit  wrote:
>> On Wed, Dec 21, 2011 at 1:50 AM, Dave Airlie  wrote:
>> 
>>>>
>>>> Hence for both patches:
>>>> Reviewed-by: Daniel Vetter 
>>>
>>> Yeah I'm with Daniel, I like this one, I can definitely build the drm
>>> buffer sharing layer on top of this.
>>>
>>> How do we see this getting merged? I'm quite happy to push it to Linus
>>> if we don't have an identified path, though it could go via a Linaro
>>> tree as well.
>>>
>>> so feel free to add:
>>> Reviewed-by: Dave Airlie 
>> Thanks Daniel and Dave!
>>
>> I guess we can start with staging for 3.3, and see how it shapes up. I
>> will post the latest patch version pretty soon.
>
> not sure about staging, but could make sense to mark as experimental.
Thanks, I will mark it experimental for the first version; we can
remove that once it is more widely used and tested.
>
>> Arnd, Dave: do you have any preference on the path it takes to get
>> merged? In my mind, Linaro tree might make more sense, but I would
>> leave it upto you gentlemen.
>
> Looks like Dave is making some progress on drm usage of buffer sharing
> between gpu's.. if that is ready to go in at the same time, it might
> be a bit logistically simpler for him to put dmabuf in the same pull
> req.  I don't have strong preference one way or another, so do what is
> collectively simpler ;-)
:) Right - I am quite happy for it to get merged in either ways :)
>
> BR,
> -R
Best regards,
~Sumit.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[Linaro-mm-sig] [RFC v3 0/2] Introduce DMA buffer sharing mechanism

[Semwal, Sumit]

On Wed, Dec 21, 2011 at 1:50 AM, Dave Airlie  wrote:

>>
>> I think this is a really good v1 version of dma_buf. It contains all the
>> required bits (with well-specified semantics in the doc patch) to
>> implement some basic use-cases and start fleshing out the integration with
>> various subsystem (like drm and v4l). All the things still under
>> discussion like
>> - userspace mmap support
>> - more advanced (and more strictly specified) coherency models
>> - and shared infrastructure for implementing exporters
>> are imo much clearer once we have a few example drivers at hand and a
>> better understanding of some of the insane corner cases we need to be able
>> to handle.
>>
>> And I think any risk that the resulting clarifications will break a basic
>> use-case is really minimal, so I think it'd be great if this could go into
>> 3.3 (maybe as some kind of staging/experimental infrastructure).
>>
>> Hence for both patches:
>> Reviewed-by: Daniel Vetter 
>
> Yeah I'm with Daniel, I like this one, I can definitely build the drm
> buffer sharing layer on top of this.
>
> How do we see this getting merged? I'm quite happy to push it to Linus
> if we don't have an identified path, though it could go via a Linaro
> tree as well.
>
> so feel free to add:
> Reviewed-by: Dave Airlie 
Thanks Daniel and Dave!

I guess we can start with staging for 3.3, and see how it shapes up. I
will post the latest patch version pretty soon.

Arnd, Dave: do you have any preference on the path it takes to get
merged? In my mind, Linaro tree might make more sense, but I would
leave it upto you gentlemen.
>
> Dave.
Best regards,
~Sumit.

[Linaro-mm-sig] [RFC v2 1/2] dma-buf: Introduce dma buffer sharing mechanism

[Semwal, Sumit]

On Wed, Dec 21, 2011 at 10:57 PM, Arnd Bergmann  wrote:
> On Tuesday 20 December 2011, Daniel Vetter wrote:
>> > I'm thinking for a first version, we can get enough mileage out of it by 
>> > saying:
>> > 1) only exporter can mmap to userspace
>> > 2) only importers that do not need CPU access to buffer..

Thanks Rob - and the exporter can do the mmap outside of dma-buf
usage, right? I mean, we don't need to provide an mmap to dma_buf()
and restrict it to exporter, when the exporter has more 'control' of
the buffer anyways.
>
BR,
~Sumit.

[RFC v3 1/2] dma-buf: Introduce dma buffer sharing mechanism

[Semwal, Sumit]

Hi Konrad,

On Tue, Dec 20, 2011 at 10:06 PM, Konrad Rzeszutek Wilk
 wrote:
> On Mon, Dec 19, 2011 at 02:03:30PM +0530, Sumit Semwal wrote:
>> This is the first step in defining a dma buffer sharing mechanism.
>>
>> A new buffer object dma_buf is added, with operations and API to allow easy
>> sharing of this buffer object across devices.
>>
>> The framework allows:
>> - different devices to 'attach' themselves to this buffer, to facilitate
>> ? backing storage negotiation, using dma_buf_attach() API.
>
> Any thoughts of adding facility to track them? So you can see who is using 
> what?
Not for version 1, but it would be a useful addition once we start
using this mechanism.

>
>> - association of a file pointer with each user-buffer and associated
>> ? ?allocator-defined operations on that buffer. This operation is called the
>> ? ?'export' operation.
>
> ?'create'? or 'alloc' ?
>
> export implies an import somwhere and I don't think that is the case here.
I will rephrase it as suggested by Rob as well.

>
>> - this exported buffer-object to be shared with the other entity by asking 
>> for
>> ? ?its 'file-descriptor (fd)', and sharing the fd across.
>> - a received fd to get the buffer object back, where it can be accessed using
>> ? ?the associated exporter-defined operations.
>> - the exporter and user to share the scatterlist using map_dma_buf and
>> ? ?unmap_dma_buf operations.
>>
>> Atleast one 'attach()' call is required to be made prior to calling the
>> map_dma_buf() operation.
>
> for the whole memory region or just for the device itself?
Rob has very eloquently and kindly explained it in his reply.

>
>>

>> +/*
>> + * is_dma_buf_file - Check if struct file* is associated with dma_buf
>> + */
>> +static inline int is_dma_buf_file(struct file *file)
>> +{
>> + ? ? return file->f_op == &dma_buf_fops;
>> +}
>> +
>> +/**
>
> Wrong kerneldoc.
I looked into scripts/kernel-doc, and
Documentation/kernel-doc-na-HOWTO.txt => both these places mention
that the kernel-doc comments have to start with /**, and I couldn't
spot an error in what's wrong with my usage - would you please
elaborate on what you think is not right?
>

>> +/**
>> + * struct dma_buf_attachment - holds device-buffer attachment data
>
> OK, but what is the purpose of it?
I will add that in the comments.
>
>> + * @dmabuf: buffer for this attachment.
>> + * @dev: device attached to the buffer.
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?^^^ this
>> + * @node: list_head to allow manipulation of list of dma_buf_attachment.
>
> Just say: "list of dma_buf_attachment"'
ok.
>
>> + * @priv: exporter-specific attachment data.
>
> That "exporter-specific.." brings to my mind custom decleration forms. But 
> maybe that is me.
:) well, in context of dma-buf 'exporter', it makes sense.

>
>> + */
>> +struct dma_buf_attachment {
>> + ? ? struct dma_buf *dmabuf;
>> + ? ? struct device *dev;
>> + ? ? struct list_head node;
>> + ? ? void *priv;
>> +};
>
> Why don't you move the decleration of this below 'struct dma_buf'?
> It would easier than to read this structure..
I could do that, but then anyways I will have to do a
forward-declaration of dma_buf_attachment, since I have to use it in
dma_buf_ops. If it improves readability, I am happy to move it below
struct dma_buf.

>
>> +
>> +/**
>> + * struct dma_buf_ops - operations possible on struct dma_buf
>> + * @attach: allows different devices to 'attach' themselves to the given
>
> register?
>> + * ? ? ? buffer. It might return -EBUSY to signal that backing storage
>> + * ? ? ? is already allocated and incompatible with the requirements
>
> Wait.. allocated or attached?
This and above comment on 'register' are already answered by Rob in
his explanation of the sequence in earlier reply. [the Documentation
patch [2/2] also tries to explain it]

>
>> + * ? ? ? of requesting device. [optional]
>
> What is optional? The return value? Or the 'attach' call? If the later , say
> that in the first paragraph.
>
ok, sure. it is meant for the attach op.
>
>> + * @detach: detach a given device from this buffer. [optional]
>> + * @map_dma_buf: returns list of scatter pages allocated, increases usecount
>> + * ? ? ? ? ? ?of the buffer. Requires atleast one attach to be called
>> + * ? ? ? ? ? ?before. Returned sg list should already be mapped into
>> + * ? ? ? ? ? ?_device_ address space. This call may sleep. May also return
>
> Ok, there is some __might_sleep macro you should put on the function.
>
That's a nice suggestion; I will add it to the wrapper function for
map_dma_buf().

>> + * ? ? ? ? ? ?-EINTR.
>
> Ok. What is the return code if attach has _not_ been called?
Will document it to return -EINVAL if atleast on attach() hasn't been called.

>
>> + * @unmap_dma_buf: decreases usecount of buffer, might deallocate scatter
>> + * ? ? ? ? ? ? ?pages.
>> + * @release: release this buffer; to be called after the last dma_buf_put.
>> + * @sync_sg_for_cpu: sync the sg list for cpu.
>> + * @sync_sg_for_device: synch the sg list for

[Linaro-mm-sig] [RFC v3 0/2] Introduce DMA buffer sharing mechanism

[Semwal, Sumit]

On Wed, Dec 21, 2011 at 3:56 AM, Rob Clark  wrote:
> On Tue, Dec 20, 2011 at 2:20 PM, Dave Airlie  wrote:


>>>
>>> I think this is a really good v1 version of dma_buf. It contains all the
>>> required bits (with well-specified semantics in the doc patch) to
>>> implement some basic use-cases and start fleshing out the integration with
>>> various subsystem (like drm and v4l). All the things still under
>>> discussion like
>>> - userspace mmap support
>>> - more advanced (and more strictly specified) coherency models
>>> - and shared infrastructure for implementing exporters
>>> are imo much clearer once we have a few example drivers at hand and a
>>> better understanding of some of the insane corner cases we need to be able
>>> to handle.
>>>
>>> And I think any risk that the resulting clarifications will break a basic
>>> use-case is really minimal, so I think it'd be great if this could go into
>>> 3.3 (maybe as some kind of staging/experimental infrastructure).
>>>
>>> Hence for both patches:
>>> Reviewed-by: Daniel Vetter 
>>
>> Yeah I'm with Daniel, I like this one, I can definitely build the drm
>> buffer sharing layer on top of this.
>>
>> How do we see this getting merged? I'm quite happy to push it to Linus
>> if we don't have an identified path, though it could go via a Linaro
>> tree as well.
>>
>> so feel free to add:
>> Reviewed-by: Dave Airlie 
>
> fwiw, patches to share buffers between drm and v4l2 are here:
>
> https://github.com/robclark/kernel-omap4/commits/drmplane-dmabuf
>
> (need a bit of cleanup before the vb2 patches are submitted.. but that
> is unrelated to the dmabuf patches)
>
> so,
>
> Reviewed-and-Tested-by: Rob Clark 
Thanks Daniel, Dave, and Rob!
BR,
Sumit.
>
>> Dave.
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-media" in
>> the body of a message to majordomo at vger.kernel.org
>> More majordomo info at ?http://vger.kernel.org/majordomo-info.html

Re: [Linaro-mm-sig] [RFC v3 0/2] Introduce DMA buffer sharing mechanism

[Semwal, Sumit]

On Wed, Dec 21, 2011 at 1:50 AM, Dave Airlie  wrote:

>>
>> I think this is a really good v1 version of dma_buf. It contains all the
>> required bits (with well-specified semantics in the doc patch) to
>> implement some basic use-cases and start fleshing out the integration with
>> various subsystem (like drm and v4l). All the things still under
>> discussion like
>> - userspace mmap support
>> - more advanced (and more strictly specified) coherency models
>> - and shared infrastructure for implementing exporters
>> are imo much clearer once we have a few example drivers at hand and a
>> better understanding of some of the insane corner cases we need to be able
>> to handle.
>>
>> And I think any risk that the resulting clarifications will break a basic
>> use-case is really minimal, so I think it'd be great if this could go into
>> 3.3 (maybe as some kind of staging/experimental infrastructure).
>>
>> Hence for both patches:
>> Reviewed-by: Daniel Vetter 
>
> Yeah I'm with Daniel, I like this one, I can definitely build the drm
> buffer sharing layer on top of this.
>
> How do we see this getting merged? I'm quite happy to push it to Linus
> if we don't have an identified path, though it could go via a Linaro
> tree as well.
>
> so feel free to add:
> Reviewed-by: Dave Airlie 
Thanks Daniel and Dave!

I guess we can start with staging for 3.3, and see how it shapes up. I
will post the latest patch version pretty soon.

Arnd, Dave: do you have any preference on the path it takes to get
merged? In my mind, Linaro tree might make more sense, but I would
leave it upto you gentlemen.
>
> Dave.
Best regards,
~Sumit.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Linaro-mm-sig] [RFC v2 1/2] dma-buf: Introduce dma buffer sharing mechanism

[Semwal, Sumit]

On Wed, Dec 21, 2011 at 10:57 PM, Arnd Bergmann  wrote:
> On Tuesday 20 December 2011, Daniel Vetter wrote:
>> > I'm thinking for a first version, we can get enough mileage out of it by 
>> > saying:
>> > 1) only exporter can mmap to userspace
>> > 2) only importers that do not need CPU access to buffer..

Thanks Rob - and the exporter can do the mmap outside of dma-buf
usage, right? I mean, we don't need to provide an mmap to dma_buf()
and restrict it to exporter, when the exporter has more 'control' of
the buffer anyways.
>
BR,
~Sumit.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC v3 1/2] dma-buf: Introduce dma buffer sharing mechanism

[Semwal, Sumit]

Hi Konrad,

On Tue, Dec 20, 2011 at 10:06 PM, Konrad Rzeszutek Wilk
 wrote:
> On Mon, Dec 19, 2011 at 02:03:30PM +0530, Sumit Semwal wrote:
>> This is the first step in defining a dma buffer sharing mechanism.
>>
>> A new buffer object dma_buf is added, with operations and API to allow easy
>> sharing of this buffer object across devices.
>>
>> The framework allows:
>> - different devices to 'attach' themselves to this buffer, to facilitate
>>   backing storage negotiation, using dma_buf_attach() API.
>
> Any thoughts of adding facility to track them? So you can see who is using 
> what?
Not for version 1, but it would be a useful addition once we start
using this mechanism.

>
>> - association of a file pointer with each user-buffer and associated
>>    allocator-defined operations on that buffer. This operation is called the
>>    'export' operation.
>
>  'create'? or 'alloc' ?
>
> export implies an import somwhere and I don't think that is the case here.
I will rephrase it as suggested by Rob as well.

>
>> - this exported buffer-object to be shared with the other entity by asking 
>> for
>>    its 'file-descriptor (fd)', and sharing the fd across.
>> - a received fd to get the buffer object back, where it can be accessed using
>>    the associated exporter-defined operations.
>> - the exporter and user to share the scatterlist using map_dma_buf and
>>    unmap_dma_buf operations.
>>
>> Atleast one 'attach()' call is required to be made prior to calling the
>> map_dma_buf() operation.
>
> for the whole memory region or just for the device itself?
Rob has very eloquently and kindly explained it in his reply.

>
>>

>> +/*
>> + * is_dma_buf_file - Check if struct file* is associated with dma_buf
>> + */
>> +static inline int is_dma_buf_file(struct file *file)
>> +{
>> +     return file->f_op == &dma_buf_fops;
>> +}
>> +
>> +/**
>
> Wrong kerneldoc.
I looked into scripts/kernel-doc, and
Documentation/kernel-doc-na-HOWTO.txt => both these places mention
that the kernel-doc comments have to start with /**, and I couldn't
spot an error in what's wrong with my usage - would you please
elaborate on what you think is not right?
>

>> +/**
>> + * struct dma_buf_attachment - holds device-buffer attachment data
>
> OK, but what is the purpose of it?
I will add that in the comments.
>
>> + * @dmabuf: buffer for this attachment.
>> + * @dev: device attached to the buffer.
>                                ^^^ this
>> + * @node: list_head to allow manipulation of list of dma_buf_attachment.
>
> Just say: "list of dma_buf_attachment"'
ok.
>
>> + * @priv: exporter-specific attachment data.
>
> That "exporter-specific.." brings to my mind custom decleration forms. But 
> maybe that is me.
:) well, in context of dma-buf 'exporter', it makes sense.

>
>> + */
>> +struct dma_buf_attachment {
>> +     struct dma_buf *dmabuf;
>> +     struct device *dev;
>> +     struct list_head node;
>> +     void *priv;
>> +};
>
> Why don't you move the decleration of this below 'struct dma_buf'?
> It would easier than to read this structure..
I could do that, but then anyways I will have to do a
forward-declaration of dma_buf_attachment, since I have to use it in
dma_buf_ops. If it improves readability, I am happy to move it below
struct dma_buf.

>
>> +
>> +/**
>> + * struct dma_buf_ops - operations possible on struct dma_buf
>> + * @attach: allows different devices to 'attach' themselves to the given
>
> register?
>> + *       buffer. It might return -EBUSY to signal that backing storage
>> + *       is already allocated and incompatible with the requirements
>
> Wait.. allocated or attached?
This and above comment on 'register' are already answered by Rob in
his explanation of the sequence in earlier reply. [the Documentation
patch [2/2] also tries to explain it]

>
>> + *       of requesting device. [optional]
>
> What is optional? The return value? Or the 'attach' call? If the later , say
> that in the first paragraph.
>
ok, sure. it is meant for the attach op.
>
>> + * @detach: detach a given device from this buffer. [optional]
>> + * @map_dma_buf: returns list of scatter pages allocated, increases usecount
>> + *            of the buffer. Requires atleast one attach to be called
>> + *            before. Returned sg list should already be mapped into
>> + *            _device_ address space. This call may sleep. May also return
>
> Ok, there is some __might_sleep macro you should put on the function.
>
That's a nice suggestion; I will add it to the wrapper function for
map_dma_buf().

>> + *            -EINTR.
>
> Ok. What is the return code if attach has _not_ been called?
Will document it to return -EINVAL if atleast on attach() hasn't been called.

>
>> + * @unmap_dma_buf: decreases usecount of buffer, might deallocate scatter
>> + *              pages.
>> + * @release: release this buffer; to be called after the last dma_buf_put.
>> + * @sync_sg_for_cpu: sync the sg list for cpu.
>> + * @sync_sg_for_device: synch the sg list for

Re: [Linaro-mm-sig] [RFC v3 0/2] Introduce DMA buffer sharing mechanism

[Semwal, Sumit]

On Wed, Dec 21, 2011 at 3:56 AM, Rob Clark  wrote:
> On Tue, Dec 20, 2011 at 2:20 PM, Dave Airlie  wrote:


>>>
>>> I think this is a really good v1 version of dma_buf. It contains all the
>>> required bits (with well-specified semantics in the doc patch) to
>>> implement some basic use-cases and start fleshing out the integration with
>>> various subsystem (like drm and v4l). All the things still under
>>> discussion like
>>> - userspace mmap support
>>> - more advanced (and more strictly specified) coherency models
>>> - and shared infrastructure for implementing exporters
>>> are imo much clearer once we have a few example drivers at hand and a
>>> better understanding of some of the insane corner cases we need to be able
>>> to handle.
>>>
>>> And I think any risk that the resulting clarifications will break a basic
>>> use-case is really minimal, so I think it'd be great if this could go into
>>> 3.3 (maybe as some kind of staging/experimental infrastructure).
>>>
>>> Hence for both patches:
>>> Reviewed-by: Daniel Vetter 
>>
>> Yeah I'm with Daniel, I like this one, I can definitely build the drm
>> buffer sharing layer on top of this.
>>
>> How do we see this getting merged? I'm quite happy to push it to Linus
>> if we don't have an identified path, though it could go via a Linaro
>> tree as well.
>>
>> so feel free to add:
>> Reviewed-by: Dave Airlie 
>
> fwiw, patches to share buffers between drm and v4l2 are here:
>
> https://github.com/robclark/kernel-omap4/commits/drmplane-dmabuf
>
> (need a bit of cleanup before the vb2 patches are submitted.. but that
> is unrelated to the dmabuf patches)
>
> so,
>
> Reviewed-and-Tested-by: Rob Clark 
Thanks Daniel, Dave, and Rob!
BR,
Sumit.
>
>> Dave.
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-media" in
>> the body of a message to majord...@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[Linaro-mm-sig] [RFC v2 1/2] dma-buf: Introduce dma buffer sharing mechanism

[Semwal, Sumit]

Hi Arnd, Daniel,

On Mon, Dec 12, 2011 at 10:18 PM, Arnd Bergmann  wrote:
> On Saturday 10 December 2011, Daniel Vetter wrote:
>> If userspace (through some driver calls)
>> tries to do stupid things, it'll just get garbage. See
>> Message-ID: > mail.gmail.com>
>> for my reasons why it think this is the right way to go forward. So in
>> essence I'm really interested in the reasons why you want the kernel
>> to enforce this (or I'm completely missing what's the contentious
>> issue here).
>
> This has nothing to do with user space mappings. Whatever user space does,
> you get garbage if you don't invalidate cache lines that were introduced
> through speculative prefetching before you access cache lines that were
> DMA'd from a device.
I didn't see a consensus on whether dma_buf should enforce some form
of serialization within the API - so atleast for v1 of dma-buf, I
propose to 'not' impose a restriction, and we can tackle it (add new
ops or enforce as design?) whenever we see the first need of it - will
that be ok? [I am bending towards the thought that it is a problem to
solve at a bigger platform than dma_buf.]
>
> ? ? ? ?Arnd

Best regards,
~Sumit.
>
> --

Re: [Linaro-mm-sig] [RFC v2 1/2] dma-buf: Introduce dma buffer sharing mechanism

[Semwal, Sumit]

Hi Arnd, Daniel,

On Mon, Dec 12, 2011 at 10:18 PM, Arnd Bergmann  wrote:
> On Saturday 10 December 2011, Daniel Vetter wrote:
>> If userspace (through some driver calls)
>> tries to do stupid things, it'll just get garbage. See
>> Message-ID: 
>> 
>> for my reasons why it think this is the right way to go forward. So in
>> essence I'm really interested in the reasons why you want the kernel
>> to enforce this (or I'm completely missing what's the contentious
>> issue here).
>
> This has nothing to do with user space mappings. Whatever user space does,
> you get garbage if you don't invalidate cache lines that were introduced
> through speculative prefetching before you access cache lines that were
> DMA'd from a device.
I didn't see a consensus on whether dma_buf should enforce some form
of serialization within the API - so atleast for v1 of dma-buf, I
propose to 'not' impose a restriction, and we can tackle it (add new
ops or enforce as design?) whenever we see the first need of it - will
that be ok? [I am bending towards the thought that it is a problem to
solve at a bigger platform than dma_buf.]
>
>        Arnd

Best regards,
~Sumit.
>
> --
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[RFC v2 1/2] dma-buf: Introduce dma buffer sharing mechanism

[Semwal, Sumit]

Hi Daniel, Rob,


On Tue, Dec 6, 2011 at 3:41 AM, Rob Clark  wrote:
> On Mon, Dec 5, 2011 at 3:23 PM, Daniel Vetter  wrote:
>> On Mon, Dec 05, 2011 at 02:46:47PM -0600, Rob Clark wrote:
>>> On Mon, Dec 5, 2011 at 11:18 AM, Arnd Bergmann  wrote:
>>> > In the patch 2, you have a section about migration that mentions that
>>> > it is possible to export a buffer that can be migrated after it
>>> > is already mapped into one user driver. How does that work when
>>> > the physical addresses are mapped into a consumer device already?
>>>
>>> I think you can do physical migration if you are attached, but
>>> probably not if you are mapped.
>>
>> Yeah, that's very much how I see this, and also why map/unmap (at least
>> for simple users like v4l) should only bracket actual usage. GPU memory
>> managers need to be able to move around buffers while no one is using
>> them.
>>
>> [snip]
>>
>>> >> + ? ? /* allow allocator to take care of cache ops */
>>> >> + ? ? void (*sync_sg_for_cpu) (struct dma_buf *, struct device *);
>>> >> + ? ? void (*sync_sg_for_device)(struct dma_buf *, struct device *);
>>> >
>>> > I don't see how this works with multiple consumers: For the streaming
>>> > DMA mapping, there must be exactly one owner, either the device or
>>> > the CPU. Obviously, this rule needs to be extended when you get to
>>> > multiple devices and multiple device drivers, plus possibly user
>>> > mappings. Simply assigning the buffer to "the device" from one
>>> > driver does not block other drivers from touching the buffer, and
>>> > assigning it to "the cpu" does not stop other hardware that the
>>> > code calling sync_sg_for_cpu is not aware of.
>>> >
>>> > The only way to solve this that I can think of right now is to
>>> > mandate that the mappings are all coherent (i.e. noncachable
>>> > on noncoherent architectures like ARM). If you do that, you no
>>> > longer need the sync_sg_for_* calls.
>>>
>>> My original thinking was that you either need DMABUF_CPU_{PREP,FINI}
>>> ioctls and corresponding dmabuf ops, which userspace is required to
>>> call before / after CPU access. ?Or just remove mmap() and do the
>>> mmap() via allocating device and use that device's equivalent
>>> DRM_XYZ_GEM_CPU_{PREP,FINI} or DRM_XYZ_GEM_SET_DOMAIN ioctls. ?That
>>> would give you a way to (a) synchronize with gpu/asynchronous
>>> pipeline, (b) synchronize w/ multiple hw devices vs cpu accessing
>>> buffer (ie. wait all devices have dma_buf_unmap_attachment'd). ?And
>>> that gives you a convenient place to do cache operations on
>>> noncoherent architecture.
>>>
>>> I sort of preferred having the DMABUF shim because that lets you pass
>>> a buffer around userspace without the receiving code knowing about a
>>> device specific API. ?But the problem I eventually came around to: if
>>> your GL stack (or some other userspace component) is batching up
>>> commands before submission to kernel, the buffers you need to wait for
>>> completion might not even be submitted yet. ?So from kernel
>>> perspective they are "ready" for cpu access. ?Even though in fact they
>>> are not in a consistent state from rendering perspective. ?I don't
>>> really know a sane way to deal with that. ?Maybe the approach instead
>>> should be a userspace level API (in libkms/libdrm?) to provide
>>> abstraction for userspace access to buffers rather than dealing with
>>> this at the kernel level.
>>
>> Well, there's a reason GL has an explicit flush and extensions for sync
>> objects. It's to support such scenarios where the driver batches up gpu
>> commands before actually submitting them.
>
> Hmm.. what about other non-GL APIs.. ?maybe vaapi/vdpau or similar?
> (Or something that I haven't thought of.)
>
>> Also, recent gpus have all (or
>> shortly will grow) multiple execution pipelines, so it's also important
>> that you sync up with the right command stream. Syncing up with all of
>> them is generally frowned upon for obvious reasons ;-)
>
> Well, I guess I am happy enough with something that is at least
> functional. ?Usespace access would (I think) mainly be weird edge case
> type stuff. ?But...
>

>
>> On the topic of a coherency model for dmabuf, I think we need to look at
>> dma_buf_attachment_map/unmap (and also the mmap variants cpu_start and
>> cpu_finish or whatever they might get called) as barriers:
>>
>> So after a dma_buf_map, all previsously completed dma operations (i.e.
>> unmap already called) and any cpu writes (i.e. cpu_finish called) will be
>> coherent. Similar rule holds for cpu access through the userspace mmap,
>> only writes completed before the cpu_start will show up.
>>
>> Similar, writes done by the device are only guaranteed to show up after
>> the _unmap. Dito for cpu writes and cpu_finish.
>>
>> In short we always need two function calls to denote the start/end of the
>> "critical section".
>
> Yup, this was exactly my assumption. ?But I guess it is better to spell it 
> out.

Thanks for the excellent discussion - it indeed is ve

[RFC v2 1/2] dma-buf: Introduce dma buffer sharing mechanism

[Semwal, Sumit]

On Wed, Dec 7, 2011 at 3:41 PM, Arnd Bergmann  wrote:
> On Wednesday 07 December 2011, Semwal, Sumit wrote:
>> >
>> > Do you have a use case for making the interface compile-time disabled?
>> > I had assumed that any code using it would make no sense if it's not
>> > available so you don't actually need this.
>>
>> Ok. Though if we keep the interface compile-time disabled, the users
>> can actually check and fail or fall-back gracefully when the API is
>> not available; If I remove it, anyways the users would need to do the
>> same compile time check whether API is available or not, right?
>
> If you have to do a compile-time check for the config symbol, it's better
> to do it the way you did here than in the caller.
>
> My guess was that no caller would actually require this, because when you
> write a part of a subsystem to interact with the dma-buf infrastructure,
> you would always disable compilation of an extire file that deals with
> everything related to struct dma_buf, not just stub out the calls.

Right; that would be ideal, but we may not be able to ask each user to
do so - especially when the sharing part might be interspersed in
existing buffer handling code. So for now, I would like to keep it as
it-is.
>
> ? ? ? ?Arnd
>
BR,
~Sumit.
> --

[RFC v2 1/2] dma-buf: Introduce dma buffer sharing mechanism

[Semwal, Sumit]

Hi Arnd,

Thanks for your review!
On Mon, Dec 5, 2011 at 10:48 PM, Arnd Bergmann  wrote:
> On Friday 02 December 2011, Sumit Semwal wrote:
>> This is the first step in defining a dma buffer sharing mechanism.
>
> This looks very nice, but there are a few things I don't understand yet
> and a bunch of trivial comments I have about things I spotted.
>
> Do you have prototype exporter and consumer drivers that you can post
> for clarification?
>
> In the patch 2, you have a section about migration that mentions that
> it is possible to export a buffer that can be migrated after it
> is already mapped into one user driver. How does that work when
> the physical addresses are mapped into a consumer device already?
I guess I need to clear it up in the documentation - when I said "once
all ongoing access is completed" - I meant to say "once all current
users have finished accessing and have unmapped this buffer". So I
agree with Rob - the migration would only be possible for "attached
but unmapped" buffers. I will update the documentation.
>
>> diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
>> index 21cf46f..07d8095 100644
>> --- a/drivers/base/Kconfig
>> +++ b/drivers/base/Kconfig
>> @@ -174,4 +174,14 @@ config SYS_HYPERVISOR
>>
>> ?source "drivers/base/regmap/Kconfig"
>>
>> +config DMA_SHARED_BUFFER
>> + ? ? bool "Buffer framework to be shared between drivers"
>> + ? ? default n
>> + ? ? depends on ANON_INODES
>
> I would make this 'select ANON_INODES', like the other users of this
> feature.
Sure.

>
>> + ? ? return dmabuf;
>> +}
>> +EXPORT_SYMBOL(dma_buf_export);
>
> I agree with Konrad, this should definitely be EXPORT_SYMBOL_GPL,
> because it's really a low-level function that I would expect
> to get used by in-kernel subsystems providing the feature to
> users and having back-end drivers, but it's not the kind of thing
> we want out-of-tree drivers to mess with.
Agreed.

>
>> +/**
>> + * dma_buf_fd - returns a file descriptor for the given dma_buf
>> + * @dmabuf: ?[in] ? ?pointer to dma_buf for which fd is required.
>> + *
>> + * On success, returns an associated 'fd'. Else, returns error.
>> + */
>> +int dma_buf_fd(struct dma_buf *dmabuf)
>> +{
>> + ? ? int error, fd;
>> +
>> + ? ? if (!dmabuf->file)
>> + ? ? ? ? ? ? return -EINVAL;
>> +
>> + ? ? error = get_unused_fd_flags(0);
>
> Why not simply get_unused_fd()?
:) oversight. Will correct.

>
>> +/**
>> + * dma_buf_attach - Add the device to dma_buf's attachments list; 
>> optionally,
>> + * calls attach() of dma_buf_ops to allow device-specific attach 
>> functionality
>> + * @dmabuf: ?[in] ? ?buffer to attach device to.
>> + * @dev: ? ? [in] ? ?device to be attached.
>> + *
>> + * Returns struct dma_buf_attachment * for this attachment; may return NULL.
>> + *
>
> Or may return a negative error code. It's better to be consistent here:
> either always return NULL on error, or change the allocation error to
> ERR_PTR(-ENOMEM).
Ok, that makes sense.

>
>> + */
>> +struct dma_buf_attachment *dma_buf_attach(struct dma_buf *dmabuf,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? struct device *dev)
>> +{
>> + ? ? struct dma_buf_attachment *attach;
>> + ? ? int ret;
>> +
>> + ? ? BUG_ON(!dmabuf || !dev);
>> +
>> + ? ? attach = kzalloc(sizeof(struct dma_buf_attachment), GFP_KERNEL);
>> + ? ? if (attach == NULL)
>> + ? ? ? ? ? ? goto err_alloc;
>> +
>> + ? ? mutex_lock(&dmabuf->lock);
>> +
>> + ? ? attach->dev = dev;
>> + ? ? attach->dmabuf = dmabuf;
>> + ? ? if (dmabuf->ops->attach) {
>> + ? ? ? ? ? ? ret = dmabuf->ops->attach(dmabuf, dev, attach);
>> + ? ? ? ? ? ? if (!ret)
>> + ? ? ? ? ? ? ? ? ? ? goto err_attach;
>
> You probably mean "if (ret)" here instead of "if (!ret)", right?
yes - a stupid one! will correct.

>
>> + ? ? /* allow allocator to take care of cache ops */
>> + ? ? void (*sync_sg_for_cpu) (struct dma_buf *, struct device *);
>> + ? ? void (*sync_sg_for_device)(struct dma_buf *, struct device *);
>
> I don't see how this works with multiple consumers: For the streaming
> DMA mapping, there must be exactly one owner, either the device or
> the CPU. Obviously, this rule needs to be extended when you get to
> multiple devices and multiple device drivers, plus possibly user
> mappings. Simply assigning the buffer to "the device" from one
> driver does not block other drivers from touching the buffer, and
> assigning it to "the cpu" does not stop other hardware that the
> code calling sync_sg_for_cpu is not aware of.
>
> The only way to solve this that I can think of right now is to
> mandate that the mappings are all coherent (i.e. noncachable
> on noncoherent architectures like ARM). If you do that, you no
> longer need the sync_sg_for_* calls.
I will take yours and Daniel's suggestion, and remove these; if at all
they're needed, we can add them back again later, with
/s/device/attachment as suggested by Daniel.
>
>> +#ifdef CONFIG_DMA_SHARED_BUFFER
>
> Do you have a use case for making the interface compile-time disabled?
> I had as

Re: [RFC v2 1/2] dma-buf: Introduce dma buffer sharing mechanism

[Semwal, Sumit]

Hi Daniel, Rob,


On Tue, Dec 6, 2011 at 3:41 AM, Rob Clark  wrote:
> On Mon, Dec 5, 2011 at 3:23 PM, Daniel Vetter  wrote:
>> On Mon, Dec 05, 2011 at 02:46:47PM -0600, Rob Clark wrote:
>>> On Mon, Dec 5, 2011 at 11:18 AM, Arnd Bergmann  wrote:
>>> > In the patch 2, you have a section about migration that mentions that
>>> > it is possible to export a buffer that can be migrated after it
>>> > is already mapped into one user driver. How does that work when
>>> > the physical addresses are mapped into a consumer device already?
>>>
>>> I think you can do physical migration if you are attached, but
>>> probably not if you are mapped.
>>
>> Yeah, that's very much how I see this, and also why map/unmap (at least
>> for simple users like v4l) should only bracket actual usage. GPU memory
>> managers need to be able to move around buffers while no one is using
>> them.
>>
>> [snip]
>>
>>> >> +     /* allow allocator to take care of cache ops */
>>> >> +     void (*sync_sg_for_cpu) (struct dma_buf *, struct device *);
>>> >> +     void (*sync_sg_for_device)(struct dma_buf *, struct device *);
>>> >
>>> > I don't see how this works with multiple consumers: For the streaming
>>> > DMA mapping, there must be exactly one owner, either the device or
>>> > the CPU. Obviously, this rule needs to be extended when you get to
>>> > multiple devices and multiple device drivers, plus possibly user
>>> > mappings. Simply assigning the buffer to "the device" from one
>>> > driver does not block other drivers from touching the buffer, and
>>> > assigning it to "the cpu" does not stop other hardware that the
>>> > code calling sync_sg_for_cpu is not aware of.
>>> >
>>> > The only way to solve this that I can think of right now is to
>>> > mandate that the mappings are all coherent (i.e. noncachable
>>> > on noncoherent architectures like ARM). If you do that, you no
>>> > longer need the sync_sg_for_* calls.
>>>
>>> My original thinking was that you either need DMABUF_CPU_{PREP,FINI}
>>> ioctls and corresponding dmabuf ops, which userspace is required to
>>> call before / after CPU access.  Or just remove mmap() and do the
>>> mmap() via allocating device and use that device's equivalent
>>> DRM_XYZ_GEM_CPU_{PREP,FINI} or DRM_XYZ_GEM_SET_DOMAIN ioctls.  That
>>> would give you a way to (a) synchronize with gpu/asynchronous
>>> pipeline, (b) synchronize w/ multiple hw devices vs cpu accessing
>>> buffer (ie. wait all devices have dma_buf_unmap_attachment'd).  And
>>> that gives you a convenient place to do cache operations on
>>> noncoherent architecture.
>>>
>>> I sort of preferred having the DMABUF shim because that lets you pass
>>> a buffer around userspace without the receiving code knowing about a
>>> device specific API.  But the problem I eventually came around to: if
>>> your GL stack (or some other userspace component) is batching up
>>> commands before submission to kernel, the buffers you need to wait for
>>> completion might not even be submitted yet.  So from kernel
>>> perspective they are "ready" for cpu access.  Even though in fact they
>>> are not in a consistent state from rendering perspective.  I don't
>>> really know a sane way to deal with that.  Maybe the approach instead
>>> should be a userspace level API (in libkms/libdrm?) to provide
>>> abstraction for userspace access to buffers rather than dealing with
>>> this at the kernel level.
>>
>> Well, there's a reason GL has an explicit flush and extensions for sync
>> objects. It's to support such scenarios where the driver batches up gpu
>> commands before actually submitting them.
>
> Hmm.. what about other non-GL APIs..  maybe vaapi/vdpau or similar?
> (Or something that I haven't thought of.)
>
>> Also, recent gpus have all (or
>> shortly will grow) multiple execution pipelines, so it's also important
>> that you sync up with the right command stream. Syncing up with all of
>> them is generally frowned upon for obvious reasons ;-)
>
> Well, I guess I am happy enough with something that is at least
> functional.  Usespace access would (I think) mainly be weird edge case
> type stuff.  But...
>

>
>> On the topic of a coherency model for dmabuf, I think we need to look at
>> dma_buf_attachment_map/unmap (and also the mmap variants cpu_start and
>> cpu_finish or whatever they might get called) as barriers:
>>
>> So after a dma_buf_map, all previsously completed dma operations (i.e.
>> unmap already called) and any cpu writes (i.e. cpu_finish called) will be
>> coherent. Similar rule holds for cpu access through the userspace mmap,
>> only writes completed before the cpu_start will show up.
>>
>> Similar, writes done by the device are only guaranteed to show up after
>> the _unmap. Dito for cpu writes and cpu_finish.
>>
>> In short we always need two function calls to denote the start/end of the
>> "critical section".
>
> Yup, this was exactly my assumption.  But I guess it is better to spell it 
> out.

Thanks for the excellent discussion - it indeed is ve

Re: [RFC v2 1/2] dma-buf: Introduce dma buffer sharing mechanism

[Semwal, Sumit]

On Wed, Dec 7, 2011 at 3:41 PM, Arnd Bergmann  wrote:
> On Wednesday 07 December 2011, Semwal, Sumit wrote:
>> >
>> > Do you have a use case for making the interface compile-time disabled?
>> > I had assumed that any code using it would make no sense if it's not
>> > available so you don't actually need this.
>>
>> Ok. Though if we keep the interface compile-time disabled, the users
>> can actually check and fail or fall-back gracefully when the API is
>> not available; If I remove it, anyways the users would need to do the
>> same compile time check whether API is available or not, right?
>
> If you have to do a compile-time check for the config symbol, it's better
> to do it the way you did here than in the caller.
>
> My guess was that no caller would actually require this, because when you
> write a part of a subsystem to interact with the dma-buf infrastructure,
> you would always disable compilation of an extire file that deals with
> everything related to struct dma_buf, not just stub out the calls.

Right; that would be ideal, but we may not be able to ask each user to
do so - especially when the sharing part might be interspersed in
existing buffer handling code. So for now, I would like to keep it as
it-is.
>
>        Arnd
>
BR,
~Sumit.
> --
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC v2 1/2] dma-buf: Introduce dma buffer sharing mechanism

[Semwal, Sumit]

Hi Arnd,

Thanks for your review!
On Mon, Dec 5, 2011 at 10:48 PM, Arnd Bergmann  wrote:
> On Friday 02 December 2011, Sumit Semwal wrote:
>> This is the first step in defining a dma buffer sharing mechanism.
>
> This looks very nice, but there are a few things I don't understand yet
> and a bunch of trivial comments I have about things I spotted.
>
> Do you have prototype exporter and consumer drivers that you can post
> for clarification?
>
> In the patch 2, you have a section about migration that mentions that
> it is possible to export a buffer that can be migrated after it
> is already mapped into one user driver. How does that work when
> the physical addresses are mapped into a consumer device already?
I guess I need to clear it up in the documentation - when I said "once
all ongoing access is completed" - I meant to say "once all current
users have finished accessing and have unmapped this buffer". So I
agree with Rob - the migration would only be possible for "attached
but unmapped" buffers. I will update the documentation.
>
>> diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
>> index 21cf46f..07d8095 100644
>> --- a/drivers/base/Kconfig
>> +++ b/drivers/base/Kconfig
>> @@ -174,4 +174,14 @@ config SYS_HYPERVISOR
>>
>>  source "drivers/base/regmap/Kconfig"
>>
>> +config DMA_SHARED_BUFFER
>> +     bool "Buffer framework to be shared between drivers"
>> +     default n
>> +     depends on ANON_INODES
>
> I would make this 'select ANON_INODES', like the other users of this
> feature.
Sure.

>
>> +     return dmabuf;
>> +}
>> +EXPORT_SYMBOL(dma_buf_export);
>
> I agree with Konrad, this should definitely be EXPORT_SYMBOL_GPL,
> because it's really a low-level function that I would expect
> to get used by in-kernel subsystems providing the feature to
> users and having back-end drivers, but it's not the kind of thing
> we want out-of-tree drivers to mess with.
Agreed.

>
>> +/**
>> + * dma_buf_fd - returns a file descriptor for the given dma_buf
>> + * @dmabuf:  [in]    pointer to dma_buf for which fd is required.
>> + *
>> + * On success, returns an associated 'fd'. Else, returns error.
>> + */
>> +int dma_buf_fd(struct dma_buf *dmabuf)
>> +{
>> +     int error, fd;
>> +
>> +     if (!dmabuf->file)
>> +             return -EINVAL;
>> +
>> +     error = get_unused_fd_flags(0);
>
> Why not simply get_unused_fd()?
:) oversight. Will correct.

>
>> +/**
>> + * dma_buf_attach - Add the device to dma_buf's attachments list; 
>> optionally,
>> + * calls attach() of dma_buf_ops to allow device-specific attach 
>> functionality
>> + * @dmabuf:  [in]    buffer to attach device to.
>> + * @dev:     [in]    device to be attached.
>> + *
>> + * Returns struct dma_buf_attachment * for this attachment; may return NULL.
>> + *
>
> Or may return a negative error code. It's better to be consistent here:
> either always return NULL on error, or change the allocation error to
> ERR_PTR(-ENOMEM).
Ok, that makes sense.

>
>> + */
>> +struct dma_buf_attachment *dma_buf_attach(struct dma_buf *dmabuf,
>> +                                             struct device *dev)
>> +{
>> +     struct dma_buf_attachment *attach;
>> +     int ret;
>> +
>> +     BUG_ON(!dmabuf || !dev);
>> +
>> +     attach = kzalloc(sizeof(struct dma_buf_attachment), GFP_KERNEL);
>> +     if (attach == NULL)
>> +             goto err_alloc;
>> +
>> +     mutex_lock(&dmabuf->lock);
>> +
>> +     attach->dev = dev;
>> +     attach->dmabuf = dmabuf;
>> +     if (dmabuf->ops->attach) {
>> +             ret = dmabuf->ops->attach(dmabuf, dev, attach);
>> +             if (!ret)
>> +                     goto err_attach;
>
> You probably mean "if (ret)" here instead of "if (!ret)", right?
yes - a stupid one! will correct.

>
>> +     /* allow allocator to take care of cache ops */
>> +     void (*sync_sg_for_cpu) (struct dma_buf *, struct device *);
>> +     void (*sync_sg_for_device)(struct dma_buf *, struct device *);
>
> I don't see how this works with multiple consumers: For the streaming
> DMA mapping, there must be exactly one owner, either the device or
> the CPU. Obviously, this rule needs to be extended when you get to
> multiple devices and multiple device drivers, plus possibly user
> mappings. Simply assigning the buffer to "the device" from one
> driver does not block other drivers from touching the buffer, and
> assigning it to "the cpu" does not stop other hardware that the
> code calling sync_sg_for_cpu is not aware of.
>
> The only way to solve this that I can think of right now is to
> mandate that the mappings are all coherent (i.e. noncachable
> on noncoherent architectures like ARM). If you do that, you no
> longer need the sync_sg_for_* calls.
I will take yours and Daniel's suggestion, and remove these; if at all
they're needed, we can add them back again later, with
/s/device/attachment as suggested by Daniel.
>
>> +#ifdef CONFIG_DMA_SHARED_BUFFER
>
> Do you have a use case for making the interface compile-time disabled?
> I had as

[RFC v2 1/2] dma-buf: Introduce dma buffer sharing mechanism

[Semwal, Sumit]

Hi Konrad,

On Fri, Dec 2, 2011 at 10:41 PM, Konrad Rzeszutek Wilk
 wrote:
> On Fri, Dec 02, 2011 at 02:27:31PM +0530, Sumit Semwal wrote:
>> This is the first step in defining a dma buffer sharing mechanism.
>>

>>
>> [1]: https://wiki.linaro.org/OfficeofCTO/MemoryManagement
>> [2]: http://lwn.net/Articles/454389
>>
>> Signed-off-by: Sumit Semwal 
>> Signed-off-by: Sumit Semwal 
>
> You have a clone? You only need one SOB.
:) Thanks for your review - Well, not a clone, but I have two 'employers' :))

I have a rather weird reason for this - I am employed with Texas
Instruments, but working with Linaro as well. And due to some
'non-technical' reasons, I need to send this work from @ti.com mail
ID. At the same time, I would like to acknowledge that this work was
done as part of the Linaro umbrella, so I put another SOB @linaro.org.

>
>

>> + * Copyright(C) 2011 Linaro Limited. All rights reserved.
>> + * Author: Sumit Semwal 
>
> OK, so the SOB should be from @ti.com then.
>
>> + *

>> +static int dma_buf_mmap(struct file *file, struct vm_area_struct *vma)
>> +{
>> + ? ? struct dma_buf *dmabuf;
>> +
>> + ? ? if (!is_dma_buf_file(file))
>> + ? ? ? ? ? ? return -EINVAL;
>> +
>> + ? ? dmabuf = file->private_data;
>> +
>
> Should you check if dmabuf is NULL and or dmabuf->ops is NULL too?
>
> Hm, you probably don't need to check for dmabuf, but from
> looking at ?dma_buf_export one could pass ?a NULL for the ops.
see next comment
>
>> + ? ? if (!dmabuf->ops->mmap)
>> + ? ? ? ? ? ? return -EINVAL;
>> +
>> + ? ? return dmabuf->ops->mmap(dmabuf, vma);
>> +}
>> +
>> +static int dma_buf_release(struct inode *inode, struct file *file)
>> +{
>> + ? ? struct dma_buf *dmabuf;
>> +
>> + ? ? if (!is_dma_buf_file(file))
>> + ? ? ? ? ? ? return -EINVAL;
>> +
>> + ? ? dmabuf = file->private_data;
>> +
>
> No checking here for ops or ops->release?
Hmmm.. you're right, of course. for this common check in mmap and
release, I guess I'd add it to 'is_dma_buf_file()' helper [maybe call
it is_valid_dma_buf_file() or something similar]
>

>> +
>> +/**
>
> I don't think the ** is anymore the current kernel doc format.
thanks for catching this :) - will correct.
>
>> + * dma_buf_export - Creates a new dma_buf, and associates an anon file
>> + * with this buffer,so it can be exported.
>
> Put a space there.
ok
>
>> + * Also connect the allocator specific data and ops to the buffer.
>> + *
>> + * @priv: ? ?[in] ? ?Attach private data of allocator to this buffer
>> + * @ops: ? ? [in] ? ?Attach allocator-defined dma buf ops to the new buffer.
>> + * @flags: ? [in] ? ?mode flags for the file.
>> + *
>> + * Returns, on success, a newly created dma_buf object, which wraps the
>> + * supplied private data and operations for dma_buf_ops. On failure to
>> + * allocate the dma_buf object, it can return NULL.
>
> "it can" I think the right word is "it will".
Right.
>
>> + *
>> + */
>> +struct dma_buf *dma_buf_export(void *priv, struct dma_buf_ops *ops,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? int flags)
>> +{
>> + ? ? struct dma_buf *dmabuf;
>> + ? ? struct file *file;
>> +
>> + ? ? BUG_ON(!priv || !ops);
>
> Whoa. Crash the whole kernel b/c of this? No no. You should
> use WARN_ON and just return NULL.
ok
>
>> +
>> + ? ? dmabuf = kzalloc(sizeof(struct dma_buf), GFP_KERNEL);
>> + ? ? if (dmabuf == NULL)
>> + ? ? ? ? ? ? return dmabuf;
>
> Hmm, why not return ERR_PTR(-ENOMEM); ?
ok
>
>> +
>> + ? ? dmabuf->priv = priv;
>> + ? ? dmabuf->ops = ops;
>> +
>> + ? ? file = anon_inode_getfile("dmabuf", &dma_buf_fops, dmabuf, flags);
>> +
>> + ? ? dmabuf->file = file;
>> +
>> + ? ? mutex_init(&dmabuf->lock);
>> + ? ? INIT_LIST_HEAD(&dmabuf->attachments);
>> +
>> + ? ? return dmabuf;
>> +}
>> +EXPORT_SYMBOL(dma_buf_export);
>
> _GPL ?
sure; will change it.
>
>> +
>> +
>> +/**
>> + * dma_buf_fd - returns a file descriptor for the given dma_buf
>> + * @dmabuf: ?[in] ? ?pointer to dma_buf for which fd is required.
>> + *
>> + * On success, returns an associated 'fd'. Else, returns error.
>> + */
>> +int dma_buf_fd(struct dma_buf *dmabuf)
>> +{
>> + ? ? int error, fd;
>> +
>
> Should you check if dmabuf is NULL first?
yes.
>
>> + ? ? if (!dmabuf->file)
>> + ? ? ? ? ? ? return -EINVAL;
>> +
>> + ? ? error = get_unused_fd_flags(0);
>> + ? ? if (error < 0)
>> + ? ? ? ? ? ? return error;
>> + ? ? fd = error;
>> +
>> + ? ? fd_install(fd, dmabuf->file);
>> +
>> + ? ? return fd;
>> +}
>> +EXPORT_SYMBOL(dma_buf_fd);
>
> GPL?
sure; will change it.
>> +
>> +/**
>> + * dma_buf_get - returns the dma_buf structure related to an fd
>> + * @fd: ? ? ?[in] ? ?fd associated with the dma_buf to be returned
>> + *
>> + * On success, returns the dma_buf structure associated with an fd; uses
>> + * file's refcounting done by fget to increase refcount. returns ERR_PTR
>> + * otherwise.
>> + */
>> +struct dma_buf *dma_buf_get(int fd)
>> +{
>> + ? ? struct file *file;
>> +
>> + ? ? file = fget(fd);
>> +
>> + ? ? if (!file)
>> + ? ? ? ? ? ? return ERR_PTR(-EBADF);
>> +
>> + ? ? if (!is_dma_buf_

Re: [RFC v2 1/2] dma-buf: Introduce dma buffer sharing mechanism

[Semwal, Sumit]

Hi Konrad,

On Fri, Dec 2, 2011 at 10:41 PM, Konrad Rzeszutek Wilk
 wrote:
> On Fri, Dec 02, 2011 at 02:27:31PM +0530, Sumit Semwal wrote:
>> This is the first step in defining a dma buffer sharing mechanism.
>>

>>
>> [1]: https://wiki.linaro.org/OfficeofCTO/MemoryManagement
>> [2]: http://lwn.net/Articles/454389
>>
>> Signed-off-by: Sumit Semwal 
>> Signed-off-by: Sumit Semwal 
>
> You have a clone? You only need one SOB.
:) Thanks for your review - Well, not a clone, but I have two 'employers' :))

I have a rather weird reason for this - I am employed with Texas
Instruments, but working with Linaro as well. And due to some
'non-technical' reasons, I need to send this work from @ti.com mail
ID. At the same time, I would like to acknowledge that this work was
done as part of the Linaro umbrella, so I put another SOB @linaro.org.

>
>

>> + * Copyright(C) 2011 Linaro Limited. All rights reserved.
>> + * Author: Sumit Semwal 
>
> OK, so the SOB should be from @ti.com then.
>
>> + *

>> +static int dma_buf_mmap(struct file *file, struct vm_area_struct *vma)
>> +{
>> +     struct dma_buf *dmabuf;
>> +
>> +     if (!is_dma_buf_file(file))
>> +             return -EINVAL;
>> +
>> +     dmabuf = file->private_data;
>> +
>
> Should you check if dmabuf is NULL and or dmabuf->ops is NULL too?
>
> Hm, you probably don't need to check for dmabuf, but from
> looking at  dma_buf_export one could pass  a NULL for the ops.
see next comment
>
>> +     if (!dmabuf->ops->mmap)
>> +             return -EINVAL;
>> +
>> +     return dmabuf->ops->mmap(dmabuf, vma);
>> +}
>> +
>> +static int dma_buf_release(struct inode *inode, struct file *file)
>> +{
>> +     struct dma_buf *dmabuf;
>> +
>> +     if (!is_dma_buf_file(file))
>> +             return -EINVAL;
>> +
>> +     dmabuf = file->private_data;
>> +
>
> No checking here for ops or ops->release?
Hmmm.. you're right, of course. for this common check in mmap and
release, I guess I'd add it to 'is_dma_buf_file()' helper [maybe call
it is_valid_dma_buf_file() or something similar]
>

>> +
>> +/**
>
> I don't think the ** is anymore the current kernel doc format.
thanks for catching this :) - will correct.
>
>> + * dma_buf_export - Creates a new dma_buf, and associates an anon file
>> + * with this buffer,so it can be exported.
>
> Put a space there.
ok
>
>> + * Also connect the allocator specific data and ops to the buffer.
>> + *
>> + * @priv:    [in]    Attach private data of allocator to this buffer
>> + * @ops:     [in]    Attach allocator-defined dma buf ops to the new buffer.
>> + * @flags:   [in]    mode flags for the file.
>> + *
>> + * Returns, on success, a newly created dma_buf object, which wraps the
>> + * supplied private data and operations for dma_buf_ops. On failure to
>> + * allocate the dma_buf object, it can return NULL.
>
> "it can" I think the right word is "it will".
Right.
>
>> + *
>> + */
>> +struct dma_buf *dma_buf_export(void *priv, struct dma_buf_ops *ops,
>> +                             int flags)
>> +{
>> +     struct dma_buf *dmabuf;
>> +     struct file *file;
>> +
>> +     BUG_ON(!priv || !ops);
>
> Whoa. Crash the whole kernel b/c of this? No no. You should
> use WARN_ON and just return NULL.
ok
>
>> +
>> +     dmabuf = kzalloc(sizeof(struct dma_buf), GFP_KERNEL);
>> +     if (dmabuf == NULL)
>> +             return dmabuf;
>
> Hmm, why not return ERR_PTR(-ENOMEM); ?
ok
>
>> +
>> +     dmabuf->priv = priv;
>> +     dmabuf->ops = ops;
>> +
>> +     file = anon_inode_getfile("dmabuf", &dma_buf_fops, dmabuf, flags);
>> +
>> +     dmabuf->file = file;
>> +
>> +     mutex_init(&dmabuf->lock);
>> +     INIT_LIST_HEAD(&dmabuf->attachments);
>> +
>> +     return dmabuf;
>> +}
>> +EXPORT_SYMBOL(dma_buf_export);
>
> _GPL ?
sure; will change it.
>
>> +
>> +
>> +/**
>> + * dma_buf_fd - returns a file descriptor for the given dma_buf
>> + * @dmabuf:  [in]    pointer to dma_buf for which fd is required.
>> + *
>> + * On success, returns an associated 'fd'. Else, returns error.
>> + */
>> +int dma_buf_fd(struct dma_buf *dmabuf)
>> +{
>> +     int error, fd;
>> +
>
> Should you check if dmabuf is NULL first?
yes.
>
>> +     if (!dmabuf->file)
>> +             return -EINVAL;
>> +
>> +     error = get_unused_fd_flags(0);
>> +     if (error < 0)
>> +             return error;
>> +     fd = error;
>> +
>> +     fd_install(fd, dmabuf->file);
>> +
>> +     return fd;
>> +}
>> +EXPORT_SYMBOL(dma_buf_fd);
>
> GPL?
sure; will change it.
>> +
>> +/**
>> + * dma_buf_get - returns the dma_buf structure related to an fd
>> + * @fd:      [in]    fd associated with the dma_buf to be returned
>> + *
>> + * On success, returns the dma_buf structure associated with an fd; uses
>> + * file's refcounting done by fget to increase refcount. returns ERR_PTR
>> + * otherwise.
>> + */
>> +struct dma_buf *dma_buf_get(int fd)
>> +{
>> +     struct file *file;
>> +
>> +     file = fget(fd);
>> +
>> +     if (!file)
>> +             return ERR_PTR(-EBADF);
>> +
>> +     if (!is_dma_buf_

[Linaro-mm-sig] [RFC 1/2] dma-buf: Introduce dma buffer sharing mechanism

[Semwal, Sumit]

Hi Dave, Daniel, Rob,
>
> On Sun, Nov 27, 2011 at 12:29 PM, Rob Clark  wrote:
>>
>> On Sat, Nov 26, 2011 at 8:00 AM, Daniel Vetter  wrote:
>> > On Fri, Nov 25, 2011 at 17:28, Dave Airlie  wrote:
>> >> I've rebuilt my PRIME interface on top of dmabuf to see how it would
>> >> work,
>> >>
>> >> I've got primed gears running again on top, but I expect all my object
>> >> lifetime and memory ownership rules need fixing up (i.e. leaks like a
>> >> sieve).
>> >>
>> >> http://cgit.freedesktop.org/~airlied/linux/log/?h=drm-prime-dmabuf
>> >>
>> >> has the i915/nouveau patches for the kernel to produce the prime
>> >> interface.
>> >
>> > I've noticed that your implementations for get_scatterlist (at least
>> > for the i915 driver) doesn't return the sg table mapped into the
>> > device address space. I've checked and the documentation makes it
>> > clear that this should be the case (and we really need this to support
>> > certain insane hw), but the get/put_scatterlist names are a bit
>> > misleading. Proposal:
>> >
>> > - use struct sg_table instead of scatterlist like you've already done
>> > in you branch. Simply more consistent with the dma api.
>>
>> yup
>>
>> > - rename get/put_scatterlist into map/unmap for consistency with all
>> > the map/unmap dma api functions. The attachement would then serve as
>> > the abstract cookie to the backing storage, similar to how struct page
>> > * works as an abstract cookie for dma_map/unmap_page. The only special
>> > thing is that struct device * parameter because that's already part of
>> > the attachment.
>>
>> yup
>>
>> > - add new wrapper functions dma_buf_map_attachment and
>> > dma_buf_unmap_attachement to hide all the pointer/vtable-chasing that
>> > we currently expose to users of this interface.
>>
>> I thought that was one of the earlier comments on the initial dmabuf
>> patch, but either way: yup
>
Thanks for your comments; I will incorporate all of these in the next
version I'll send out.
>>
>>
>> BR,
>> -R
>
BR,
Sumit.
>>
>>
>> > Comments?
>> >
>> > Cheers, Daniel
>> > --
>> > Daniel Vetter
>> > daniel.vetter at ffwll.ch - +41 (0) 79 364 57 48 - http://blog.ffwll.ch
>> > --
>> > To unsubscribe from this list: send the line "unsubscribe linux-media"
>> > in
>> > the body of a message to majordomo at vger.kernel.org
>> > More majordomo info at ?http://vger.kernel.org/majordomo-info.html
>> >
>
>

[Linaro-mm-sig] [RFC 1/2] dma-buf: Introduce dma buffer sharing mechanism

[Semwal, Sumit]

Hi Dave, Daniel, Rob,

On Sun, Nov 27, 2011 at 12:29 PM, Rob Clark  wrote:

> On Sat, Nov 26, 2011 at 8:00 AM, Daniel Vetter  wrote:
> > On Fri, Nov 25, 2011 at 17:28, Dave Airlie  wrote:
> >> I've rebuilt my PRIME interface on top of dmabuf to see how it would
> work,
> >>
> >> I've got primed gears running again on top, but I expect all my object
> >> lifetime and memory ownership rules need fixing up (i.e. leaks like a
> >> sieve).
> >>
> >> http://cgit.freedesktop.org/~airlied/linux/log/?h=drm-prime-dmabuf
> >>
> >> has the i915/nouveau patches for the kernel to produce the prime
> interface.
> >
> > I've noticed that your implementations for get_scatterlist (at least
> > for the i915 driver) doesn't return the sg table mapped into the
> > device address space. I've checked and the documentation makes it
> > clear that this should be the case (and we really need this to support
> > certain insane hw), but the get/put_scatterlist names are a bit
> > misleading. Proposal:
> >
> > - use struct sg_table instead of scatterlist like you've already done
> > in you branch. Simply more consistent with the dma api.
>
> yup
>
> > - rename get/put_scatterlist into map/unmap for consistency with all
> > the map/unmap dma api functions. The attachement would then serve as
> > the abstract cookie to the backing storage, similar to how struct page
> > * works as an abstract cookie for dma_map/unmap_page. The only special
> > thing is that struct device * parameter because that's already part of
> > the attachment.
>
> yup
>
> > - add new wrapper functions dma_buf_map_attachment and
> > dma_buf_unmap_attachement to hide all the pointer/vtable-chasing that
> > we currently expose to users of this interface.
>
> I thought that was one of the earlier comments on the initial dmabuf
> patch, but either way: yup
>
Thanks for your comments; I will incorporate all of these in the next
version I'll send out.

>
> BR,
> -R
>
BR,
Sumit.

>
> > Comments?
> >
> > Cheers, Daniel
> > --
> > Daniel Vetter
> > daniel.vetter at ffwll.ch - +41 (0) 79 364 57 48 - http://blog.ffwll.ch
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-media" in
> > the body of a message to majordomo at vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >
>
-- next part --
An HTML attachment was scrubbed...
URL: 

Re: [Linaro-mm-sig] [RFC 1/2] dma-buf: Introduce dma buffer sharing mechanism

[Semwal, Sumit]

Hi Dave, Daniel, Rob,
>
> On Sun, Nov 27, 2011 at 12:29 PM, Rob Clark  wrote:
>>
>> On Sat, Nov 26, 2011 at 8:00 AM, Daniel Vetter  wrote:
>> > On Fri, Nov 25, 2011 at 17:28, Dave Airlie  wrote:
>> >> I've rebuilt my PRIME interface on top of dmabuf to see how it would
>> >> work,
>> >>
>> >> I've got primed gears running again on top, but I expect all my object
>> >> lifetime and memory ownership rules need fixing up (i.e. leaks like a
>> >> sieve).
>> >>
>> >> http://cgit.freedesktop.org/~airlied/linux/log/?h=drm-prime-dmabuf
>> >>
>> >> has the i915/nouveau patches for the kernel to produce the prime
>> >> interface.
>> >
>> > I've noticed that your implementations for get_scatterlist (at least
>> > for the i915 driver) doesn't return the sg table mapped into the
>> > device address space. I've checked and the documentation makes it
>> > clear that this should be the case (and we really need this to support
>> > certain insane hw), but the get/put_scatterlist names are a bit
>> > misleading. Proposal:
>> >
>> > - use struct sg_table instead of scatterlist like you've already done
>> > in you branch. Simply more consistent with the dma api.
>>
>> yup
>>
>> > - rename get/put_scatterlist into map/unmap for consistency with all
>> > the map/unmap dma api functions. The attachement would then serve as
>> > the abstract cookie to the backing storage, similar to how struct page
>> > * works as an abstract cookie for dma_map/unmap_page. The only special
>> > thing is that struct device * parameter because that's already part of
>> > the attachment.
>>
>> yup
>>
>> > - add new wrapper functions dma_buf_map_attachment and
>> > dma_buf_unmap_attachement to hide all the pointer/vtable-chasing that
>> > we currently expose to users of this interface.
>>
>> I thought that was one of the earlier comments on the initial dmabuf
>> patch, but either way: yup
>
Thanks for your comments; I will incorporate all of these in the next
version I'll send out.
>>
>>
>> BR,
>> -R
>
BR,
Sumit.
>>
>>
>> > Comments?
>> >
>> > Cheers, Daniel
>> > --
>> > Daniel Vetter
>> > daniel.vet...@ffwll.ch - +41 (0) 79 364 57 48 - http://blog.ffwll.ch
>> > --
>> > To unsubscribe from this list: send the line "unsubscribe linux-media"
>> > in
>> > the body of a message to majord...@vger.kernel.org
>> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> >
>
>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Linaro-mm-sig] [RFC 1/2] dma-buf: Introduce dma buffer sharing mechanism

[Semwal, Sumit]

Hi Dave, Daniel, Rob,

On Sun, Nov 27, 2011 at 12:29 PM, Rob Clark  wrote:

> On Sat, Nov 26, 2011 at 8:00 AM, Daniel Vetter  wrote:
> > On Fri, Nov 25, 2011 at 17:28, Dave Airlie  wrote:
> >> I've rebuilt my PRIME interface on top of dmabuf to see how it would
> work,
> >>
> >> I've got primed gears running again on top, but I expect all my object
> >> lifetime and memory ownership rules need fixing up (i.e. leaks like a
> >> sieve).
> >>
> >> http://cgit.freedesktop.org/~airlied/linux/log/?h=drm-prime-dmabuf
> >>
> >> has the i915/nouveau patches for the kernel to produce the prime
> interface.
> >
> > I've noticed that your implementations for get_scatterlist (at least
> > for the i915 driver) doesn't return the sg table mapped into the
> > device address space. I've checked and the documentation makes it
> > clear that this should be the case (and we really need this to support
> > certain insane hw), but the get/put_scatterlist names are a bit
> > misleading. Proposal:
> >
> > - use struct sg_table instead of scatterlist like you've already done
> > in you branch. Simply more consistent with the dma api.
>
> yup
>
> > - rename get/put_scatterlist into map/unmap for consistency with all
> > the map/unmap dma api functions. The attachement would then serve as
> > the abstract cookie to the backing storage, similar to how struct page
> > * works as an abstract cookie for dma_map/unmap_page. The only special
> > thing is that struct device * parameter because that's already part of
> > the attachment.
>
> yup
>
> > - add new wrapper functions dma_buf_map_attachment and
> > dma_buf_unmap_attachement to hide all the pointer/vtable-chasing that
> > we currently expose to users of this interface.
>
> I thought that was one of the earlier comments on the initial dmabuf
> patch, but either way: yup
>
Thanks for your comments; I will incorporate all of these in the next
version I'll send out.

>
> BR,
> -R
>
BR,
Sumit.

>
> > Comments?
> >
> > Cheers, Daniel
> > --
> > Daniel Vetter
> > daniel.vet...@ffwll.ch - +41 (0) 79 364 57 48 - http://blog.ffwll.ch
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-media" in
> > the body of a message to majord...@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >
>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[RFC 2/2] dma-buf: Documentation for buffer sharing framework

[Semwal, Sumit]

Hi Randy,
On Thu, Oct 13, 2011 at 4:00 AM, Randy Dunlap  wrote:
> On 10/11/2011 02:23 AM, Sumit Semwal wrote:
>> Add documentation for dma buffer sharing framework, explaining the
>> various operations, members and API of the dma buffer sharing
>> framework.
>>
>> Signed-off-by: Sumit Semwal 
>> Signed-off-by: Sumit Semwal 
>> ---
>> ?Documentation/dma-buf-sharing.txt | ?210 
>> +

>> + ? ?if the new buffer-user has stricter 'backing-storage constraints', and 
>> the
>> + ? ?exporter can handle these constraints, the exporter can just stall on 
>> the
>> + ? ?get_scatterlist till all outstanding access is completed (as signalled 
>> by
>
> ? ? ? ? ? ? ? ? ? ? ? until
>
Thanks for your review; I will update all these in the next version.
>> + ? ?put_scatterlist).
>> + ? ?Once all ongoing access is completed, the exporter could potentially 
>> move
>> + ? ?the buffer to the stricter backing-storage, and then allow further
>> + ? ?{get,put}_scatterlist operations from any buffer-user from the migrated
>> + ? ?backing-storage.
>> +
>> + ? If the exporter cannot fulfill the backing-storage constraints of the new
>> + ? buffer-user device as requested, dma_buf_attach() would return an error 
>> to
>> + ? denote non-compatibility of the new buffer-sharing request with the 
>> current
>> + ? buffer.
>> +
>> + ? If the exporter chooses not to allow an attach() operation once a
>> + ? get_scatterlist has been called, it simply returns an error.
>> +
>> +- mmap file operation
>> + ? An mmap() file operation is provided for the fd associated with the 
>> buffer.
>> + ? If the exporter defines an mmap operation, the mmap() fop calls this to 
>> allow
>> + ? mmap for devices that might need it; if not, it returns an error.
>> +
>> +References:
>> +[1] struct dma_buf_ops in include/linux/dma-buf.h
>> +[2] All interfaces mentioned above defined in include/linux/dma-buf.h
>
>
> --
> ~Randy
> *** Remember to use Documentation/SubmitChecklist when testing your code ***
>
Best regards,
~Sumit.

Re: [RFC 2/2] dma-buf: Documentation for buffer sharing framework

[Semwal, Sumit]

Hi Randy,
On Thu, Oct 13, 2011 at 4:00 AM, Randy Dunlap  wrote:
> On 10/11/2011 02:23 AM, Sumit Semwal wrote:
>> Add documentation for dma buffer sharing framework, explaining the
>> various operations, members and API of the dma buffer sharing
>> framework.
>>
>> Signed-off-by: Sumit Semwal 
>> Signed-off-by: Sumit Semwal 
>> ---
>>  Documentation/dma-buf-sharing.txt |  210 
>> +

>> +    if the new buffer-user has stricter 'backing-storage constraints', and 
>> the
>> +    exporter can handle these constraints, the exporter can just stall on 
>> the
>> +    get_scatterlist till all outstanding access is completed (as signalled 
>> by
>
>                       until
>
Thanks for your review; I will update all these in the next version.
>> +    put_scatterlist).
>> +    Once all ongoing access is completed, the exporter could potentially 
>> move
>> +    the buffer to the stricter backing-storage, and then allow further
>> +    {get,put}_scatterlist operations from any buffer-user from the migrated
>> +    backing-storage.
>> +
>> +   If the exporter cannot fulfill the backing-storage constraints of the new
>> +   buffer-user device as requested, dma_buf_attach() would return an error 
>> to
>> +   denote non-compatibility of the new buffer-sharing request with the 
>> current
>> +   buffer.
>> +
>> +   If the exporter chooses not to allow an attach() operation once a
>> +   get_scatterlist has been called, it simply returns an error.
>> +
>> +- mmap file operation
>> +   An mmap() file operation is provided for the fd associated with the 
>> buffer.
>> +   If the exporter defines an mmap operation, the mmap() fop calls this to 
>> allow
>> +   mmap for devices that might need it; if not, it returns an error.
>> +
>> +References:
>> +[1] struct dma_buf_ops in include/linux/dma-buf.h
>> +[2] All interfaces mentioned above defined in include/linux/dma-buf.h
>
>
> --
> ~Randy
> *** Remember to use Documentation/SubmitChecklist when testing your code ***
>
Best regards,
~Sumit.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel