| Message ID | 1494233482-23403-2-git-send-email-tomi.valkeinen@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 05/08/17 11:51, Tomi Valkeinen wrote: > Add omap_gem_put_paddr_locked() which is a version of > omap_gem_put_paddr() that expects the caller to hold the struct_mutex. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> Looks trivial enough. Reviewed-by: Jyri Sarha <jsarha@ti.com> > --- > drivers/gpu/drm/omapdrm/omap_gem.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c > index 68a75b829b71..5d73dccc1383 100644 > --- a/drivers/gpu/drm/omapdrm/omap_gem.c > +++ b/drivers/gpu/drm/omapdrm/omap_gem.c > @@ -873,12 +873,12 @@ int omap_gem_get_paddr(struct drm_gem_object *obj, > /* Release physical address, when DMA is no longer being performed.. this > * could potentially unpin and unmap buffers from TILER > */ > -void omap_gem_put_paddr(struct drm_gem_object *obj) > + > +static void omap_gem_put_paddr_locked(struct drm_gem_object *obj) > { > struct omap_gem_object *omap_obj = to_omap_bo(obj); > int ret; > > - mutex_lock(&obj->dev->struct_mutex); > if (omap_obj->paddr_cnt > 0) { > omap_obj->paddr_cnt--; > if (omap_obj->paddr_cnt == 0) { > @@ -896,7 +896,12 @@ void omap_gem_put_paddr(struct drm_gem_object *obj) > omap_obj->block = NULL; > } > } > +} > > +void omap_gem_put_paddr(struct drm_gem_object *obj) > +{ > + mutex_lock(&obj->dev->struct_mutex); > + omap_gem_put_paddr_locked(obj); > mutex_unlock(&obj->dev->struct_mutex); > } > >
Hi Tomi, Thank you for the patch. On Monday 08 May 2017 11:51:21 Tomi Valkeinen wrote: > Add omap_gem_put_paddr_locked() which is a version of > omap_gem_put_paddr() that expects the caller to hold the struct_mutex. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> > --- > drivers/gpu/drm/omapdrm/omap_gem.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c > b/drivers/gpu/drm/omapdrm/omap_gem.c index 68a75b829b71..5d73dccc1383 > 100644 > --- a/drivers/gpu/drm/omapdrm/omap_gem.c > +++ b/drivers/gpu/drm/omapdrm/omap_gem.c > @@ -873,12 +873,12 @@ int omap_gem_get_paddr(struct drm_gem_object *obj, > /* Release physical address, when DMA is no longer being performed.. this > * could potentially unpin and unmap buffers from TILER > */ > -void omap_gem_put_paddr(struct drm_gem_object *obj) > + > +static void omap_gem_put_paddr_locked(struct drm_gem_object *obj) > { > struct omap_gem_object *omap_obj = to_omap_bo(obj); > int ret; > > - mutex_lock(&obj->dev->struct_mutex); > if (omap_obj->paddr_cnt > 0) { > omap_obj->paddr_cnt--; > if (omap_obj->paddr_cnt == 0) { You could now decrease the indentation level in this function by adding a few returns. This doesn't call for a v2, but as you'll have to rebase anyway due to the function name change, you can throw that change in at the same time :-) Apart from that, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> On a side note, it might be worth it changing the count field to a refcount_t, this would give us a WARN_ON() if the pin/unpin calls are unbalanced. > @@ -896,7 +896,12 @@ void omap_gem_put_paddr(struct drm_gem_object *obj) > omap_obj->block = NULL; > } > } > +} > > +void omap_gem_put_paddr(struct drm_gem_object *obj) > +{ > + mutex_lock(&obj->dev->struct_mutex); > + omap_gem_put_paddr_locked(obj); > mutex_unlock(&obj->dev->struct_mutex); > }
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki On 11/08/17 17:09, Laurent Pinchart wrote: > Hi Tomi, > > Thank you for the patch. > > On Monday 08 May 2017 11:51:21 Tomi Valkeinen wrote: >> Add omap_gem_put_paddr_locked() which is a version of >> omap_gem_put_paddr() that expects the caller to hold the struct_mutex. >> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> >> --- >> drivers/gpu/drm/omapdrm/omap_gem.c | 9 +++++++-- >> 1 file changed, 7 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c >> b/drivers/gpu/drm/omapdrm/omap_gem.c index 68a75b829b71..5d73dccc1383 >> 100644 >> --- a/drivers/gpu/drm/omapdrm/omap_gem.c >> +++ b/drivers/gpu/drm/omapdrm/omap_gem.c >> @@ -873,12 +873,12 @@ int omap_gem_get_paddr(struct drm_gem_object *obj, >> /* Release physical address, when DMA is no longer being performed.. this >> * could potentially unpin and unmap buffers from TILER >> */ >> -void omap_gem_put_paddr(struct drm_gem_object *obj) >> + >> +static void omap_gem_put_paddr_locked(struct drm_gem_object *obj) >> { >> struct omap_gem_object *omap_obj = to_omap_bo(obj); >> int ret; >> >> - mutex_lock(&obj->dev->struct_mutex); >> if (omap_obj->paddr_cnt > 0) { >> omap_obj->paddr_cnt--; >> if (omap_obj->paddr_cnt == 0) { > > You could now decrease the indentation level in this function by adding a few > returns. This doesn't call for a v2, but as you'll have to rebase anyway due > to the function name change, you can throw that change in at the same time :-) I actually already have the function name changed, I just didn't send an updated patch only for that... I don't want to make this patch more confusing by changing indents etc, but yes, that can be done in a separate patch. > Apart from that, > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > On a side note, it might be worth it changing the count field to a refcount_t, > this would give us a WARN_ON() if the pin/unpin calls are unbalanced. I wonder why we have a plain if() there for the case where this function is called for paddr_cnt == 0. That should only happen when we have a kernel bug, I believe, so I would have expected an least a warning print. So yes, if refcount_t gives us warnings, it's a good change. Tomi
diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c index 68a75b829b71..5d73dccc1383 100644 --- a/drivers/gpu/drm/omapdrm/omap_gem.c +++ b/drivers/gpu/drm/omapdrm/omap_gem.c @@ -873,12 +873,12 @@ int omap_gem_get_paddr(struct drm_gem_object *obj, /* Release physical address, when DMA is no longer being performed.. this * could potentially unpin and unmap buffers from TILER */ -void omap_gem_put_paddr(struct drm_gem_object *obj) + +static void omap_gem_put_paddr_locked(struct drm_gem_object *obj) { struct omap_gem_object *omap_obj = to_omap_bo(obj); int ret; - mutex_lock(&obj->dev->struct_mutex); if (omap_obj->paddr_cnt > 0) { omap_obj->paddr_cnt--; if (omap_obj->paddr_cnt == 0) { @@ -896,7 +896,12 @@ void omap_gem_put_paddr(struct drm_gem_object *obj) omap_obj->block = NULL; } } +} +void omap_gem_put_paddr(struct drm_gem_object *obj) +{ + mutex_lock(&obj->dev->struct_mutex); + omap_gem_put_paddr_locked(obj); mutex_unlock(&obj->dev->struct_mutex); }
Add omap_gem_put_paddr_locked() which is a version of omap_gem_put_paddr() that expects the caller to hold the struct_mutex. Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> --- drivers/gpu/drm/omapdrm/omap_gem.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)