On Wed, Apr 4, 2018 at 3:37 PM, Lucas Stach <l.stach@xxxxxxxxxxxxxx> wrote:
> Am Montag, den 02.04.2018, 21:50 +0300 schrieb Laurent Pinchart:
>> The __omap_gem_get_pages() function is a wrapper around
>> omap_gem_attach_pages() that returns the omap_obj->pages pointer through
>> a function argument. Some callers don't need the pages pointer, and all
>> of them can access omap_obj->pages directly. To simplify the code merge
>> the __omap_gem_get_pages() wrapper with omap_gem_attach_pages() and
>> update the callers accordingly.
>>
>> > Signed-off-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
>> ---
>> drivers/gpu/drm/omapdrm/omap_gem.c | 62 ++++++++++++++------------------------
>> 1 file changed, 23 insertions(+), 39 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c
>> index 6cfcf60cffe3..13fea207343e 100644
>> --- a/drivers/gpu/drm/omapdrm/omap_gem.c
>> +++ b/drivers/gpu/drm/omapdrm/omap_gem.c
>> @@ -222,7 +222,10 @@ static void omap_gem_evict(struct drm_gem_object *obj)
>> * Page Management
>> */
>>
>> -/** ensure backing pages are allocated */
>> +/*
>> + * Ensure backing pages are allocated. Must be called with the omap_obj.lock
>> + * held.
>> + */
>
> Drive-by comment: I always find it hard to validate those comment-only
> lock prerequisites, especially if callstacks get deeper.
>
> What we do in etnaviv is to make those lock comments executable by
> using lockdep_assert_held() to validate the locking assumptions. This
> makes sure that if you ever manage to violate the locking in a code
> rework, a lockdep enabled debug build will explode right at the spot.
+1 on this. I've gone as far as removing all the locking related
comments in core drm code because most of it was misleading or
outright wrong. The runtime checks have a much higher chance of
actually being correct :-)
-Daniel
>
> Regards,
> Lucas
>
>> static int omap_gem_attach_pages(struct drm_gem_object *obj)
>> {
>> > struct drm_device *dev = obj->dev;
>> @@ -232,7 +235,12 @@ static int omap_gem_attach_pages(struct drm_gem_object *obj)
>> > int i, ret;
>> > dma_addr_t *addrs;
>>
>> > - WARN_ON(omap_obj->pages);
>> > + /*
>> > + * If not using shmem (in which case backing pages don't need to be
>> > + * allocated) or if pages are already allocated we're done.
>> > + */
>> > + if (!(omap_obj->flags & OMAP_BO_MEM_SHMEM) || omap_obj->pages)
>> > + return 0;
>>
>> > pages = drm_gem_get_pages(obj);
>> > if (IS_ERR(pages)) {
>> @@ -288,29 +296,6 @@ static int omap_gem_attach_pages(struct drm_gem_object *obj)
>> > return ret;
>> }
>>
>> -/* acquire pages when needed (for example, for DMA where physically
>> - * contiguous buffer is not required
>> - */
>> -static int __omap_gem_get_pages(struct drm_gem_object *obj,
>> > - struct page ***pages)
>> -{
>> > - struct omap_gem_object *omap_obj = to_omap_bo(obj);
>> > - int ret = 0;
>> -
>> > - if ((omap_obj->flags & OMAP_BO_MEM_SHMEM) && !omap_obj->pages) {
>> > - ret = omap_gem_attach_pages(obj);
>> > - if (ret) {
>> > - dev_err(obj->dev->dev, "could not attach pages\n");
>> > - return ret;
>> > - }
>> > - }
>> -
>> > - /* TODO: even phys-contig.. we should have a list of pages? */
>> > - *pages = omap_obj->pages;
>> -
>> > - return 0;
>> -}
>> -
>> /** release backing pages */
>> static void omap_gem_detach_pages(struct drm_gem_object *obj)
>> {
>> @@ -516,7 +501,6 @@ int omap_gem_fault(struct vm_fault *vmf)
>> > struct drm_gem_object *obj = vma->vm_private_data;
>> > struct omap_gem_object *omap_obj = to_omap_bo(obj);
>> > struct drm_device *dev = obj->dev;
>> > - struct page **pages;
>> > int ret;
>>
>> > /* Make sure we don't parallel update on a fault, nor move or remove
>> @@ -525,7 +509,7 @@ int omap_gem_fault(struct vm_fault *vmf)
>> > mutex_lock(&dev->struct_mutex);
>>
>> > /* if a shmem backed object, make sure we have pages attached now */
>> > - ret = __omap_gem_get_pages(obj, &pages);
>> > + ret = omap_gem_attach_pages(obj);
>> > if (ret)
>> > goto fail;
>>
>> @@ -694,12 +678,12 @@ int omap_gem_roll(struct drm_gem_object *obj, u32 roll)
>>
>> > /* if we aren't mapped yet, we don't need to do anything */
>> > if (omap_obj->block) {
>> > - struct page **pages;
>> -
>> > - ret = __omap_gem_get_pages(obj, &pages);
>> > + ret = omap_gem_attach_pages(obj);
>> > if (ret)
>> > goto fail;
>> > - ret = tiler_pin(omap_obj->block, pages, npages, roll, true);
>> +
>> > + ret = tiler_pin(omap_obj->block, omap_obj->pages, npages,
>> > + roll, true);
>> > if (ret)
>> > dev_err(obj->dev->dev, "could not repin: %d\n", ret);
>> > }
>> @@ -810,14 +794,13 @@ int omap_gem_pin(struct drm_gem_object *obj, dma_addr_t *dma_addr)
>>
>> > if (!omap_gem_is_contiguous(omap_obj) && priv->has_dmm) {
>> > if (omap_obj->dma_addr_cnt == 0) {
>> > - struct page **pages;
>> > u32 npages = obj->size >> PAGE_SHIFT;
>> > enum tiler_fmt fmt = gem2fmt(omap_obj->flags);
>> > struct tiler_block *block;
>>
>> > BUG_ON(omap_obj->block);
>>
>> > - ret = __omap_gem_get_pages(obj, &pages);
>> > + ret = omap_gem_attach_pages(obj);
>> > if (ret)
>> > goto fail;
>>
>> @@ -837,7 +820,7 @@ int omap_gem_pin(struct drm_gem_object *obj, dma_addr_t *dma_addr)
>> > }
>>
>> > /* TODO: enable async refill.. */
>> > - ret = tiler_pin(block, pages, npages,
>> > + ret = tiler_pin(block, omap_obj->pages, npages,
>> > omap_obj->roll, true);
>> > if (ret) {
>> > tiler_release(block);
>> @@ -946,16 +929,18 @@ int omap_gem_tiled_stride(struct drm_gem_object *obj, u32 orient)
>> int omap_gem_get_pages(struct drm_gem_object *obj, struct page ***pages,
>> > bool remap)
>> {
>> > + struct omap_gem_object *omap_obj = to_omap_bo(obj);
>> > int ret;
>> +
>> > if (!remap) {
>> > - struct omap_gem_object *omap_obj = to_omap_bo(obj);
>> > if (!omap_obj->pages)
>> > return -ENOMEM;
>> > *pages = omap_obj->pages;
>> > return 0;
>> > }
>> > mutex_lock(&obj->dev->struct_mutex);
>> > - ret = __omap_gem_get_pages(obj, pages);
>> > + ret = omap_gem_attach_pages(obj);
>> > + *pages = omap_obj->pages;
>> > mutex_unlock(&obj->dev->struct_mutex);
>> > return ret;
>> }
>> @@ -980,13 +965,12 @@ void *omap_gem_vaddr(struct drm_gem_object *obj)
>> > struct omap_gem_object *omap_obj = to_omap_bo(obj);
>> > WARN_ON(!mutex_is_locked(&obj->dev->struct_mutex));
>> > if (!omap_obj->vaddr) {
>> > - struct page **pages;
>> > int ret;
>>
>> > - ret = __omap_gem_get_pages(obj, &pages);
>> > + ret = omap_gem_attach_pages(obj);
>> > if (ret)
>> > return ERR_PTR(ret);
>> > - omap_obj->vaddr = vmap(pages, obj->size >> PAGE_SHIFT,
>> > + omap_obj->vaddr = vmap(omap_obj->pages, obj->size >> PAGE_SHIFT,
>> > VM_MAP, pgprot_writecombine(PAGE_KERNEL));
>> > }
>> > return omap_obj->vaddr;
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
