Hi Kieran,
On Monday, 11 September 2017 23:30:25 EEST Kieran Bingham wrote:
> On 17/08/17 13:13, Laurent Pinchart wrote:
> > On Monday 14 Aug 2017 16:13:25 Kieran Bingham wrote:
> >> Each display list allocates a body to store register values in a dma
> >> accessible buffer from a dma_alloc_wc() allocation. Each of these
> >> results in an entry in the TLB, and a large number of display list
> >> allocations adds pressure to this resource.
> >>
> >> Reduce TLB pressure on the IPMMUs by allocating multiple display list
> >> bodies in a single allocation, and providing these to the display list
> >> through a 'fragment pool'. A pool can be allocated by the display list
> >> manager or entities which require their own body allocations.
> >>
> >> Signed-off-by: Kieran Bingham <kieran.bingham+rene...@ideasonboard.com>
> >>
> >> ---
> >>
> >> v2:
> >> - assign dlb->dma correctly
> >>
> >> ---
> >>
> >> drivers/media/platform/vsp1/vsp1_dl.c | 129 +++++++++++++++++++++++++++-
> >> drivers/media/platform/vsp1/vsp1_dl.h | 8 ++-
> >> 2 files changed, 137 insertions(+)
> >>
> >> diff --git a/drivers/media/platform/vsp1/vsp1_dl.c
> >> b/drivers/media/platform/vsp1/vsp1_dl.c index cb4625ae13c2..aab9dd6ec0eb
> >> 100644
> >> --- a/drivers/media/platform/vsp1/vsp1_dl.c
> >> +++ b/drivers/media/platform/vsp1/vsp1_dl.c
[snip]
> >> /*
> >> + * Fragment pool's reduce the pressure on the iommu TLB by allocating a
> >> single
> >> + * large area of DMA memory and allocating it as a pool of fragment
> >> bodies
> >> + */
> >
> > Could you document non-static function using kerneldoc ? Parameters to
> > this function would benefit from some documentation. I'd also like to see
> > the fragment get/put functions documented, as you remove existing
> > kerneldoc for the alloc/free existing functions in patch 3/8.
>
> Ah yes of course.
>
> >> +struct vsp1_dl_fragment_pool *
> >> +vsp1_dl_fragment_pool_alloc(struct vsp1_device *vsp1, unsigned int qty,
> >
> > I think I would name this function vsp1_dl_fragment_pool_create(), as it
> > does more than just allocating memory. Similarly I'd call the free
> > function vsp1_dl_fragment_pool_destroy().
>
> That sounds reasonable. Done.
>
> > qty is a bit vague, I'd rename it to num_fragments.
>
> Ok with me.
>
> >> + unsigned int num_entries, size_t extra_size)
> >> +{
> >> + struct vsp1_dl_fragment_pool *pool;
> >> + size_t dlb_size;
> >> + unsigned int i;
> >> +
> >> + pool = kzalloc(sizeof(*pool), GFP_KERNEL);
> >> + if (!pool)
> >> + return NULL;
> >> +
> >> + pool->vsp1 = vsp1;
> >> +
> >> + dlb_size = num_entries * sizeof(struct vsp1_dl_entry) + extra_size;
> >
> > extra_size is only used by vsp1_dlm_create(), to allocate extra memory for
> > the display list header. We need one header per display list, not per
> > display list body.
>
> Good catch, that will take a little bit of reworking.
I didn't propose a fix for this as I wasn't sure how to fix it properly. I
thus won't complain too loudly if you can't fix it either and waste a bit of
memory :-) But in that case please add a comment to explain what's going on.
> >> + pool->size = dlb_size * qty;
> >> +
> >> + pool->bodies = kcalloc(qty, sizeof(*pool->bodies), GFP_KERNEL);
> >> + if (!pool->bodies) {
> >> + kfree(pool);
> >> + return NULL;
> >> + }
> >> +
> >> + pool->mem = dma_alloc_wc(vsp1->bus_master, pool->size, &pool->dma,
> >> + GFP_KERNEL);
> >
> > This is a weird indentation.
>
> I know! - Not sure how that slipped by :)
>
> >> + if (!pool->mem) {
> >> + kfree(pool->bodies);
> >> + kfree(pool);
> >> + return NULL;
> >> + }
> >> +
> >> + spin_lock_init(&pool->lock);
> >> + INIT_LIST_HEAD(&pool->free);
> >> +
> >> + for (i = 0; i < qty; ++i) {
> >> + struct vsp1_dl_body *dlb = &pool->bodies[i];
> >> +
> >> + dlb->pool = pool;
> >> + dlb->max_entries = num_entries;
> >> +
> >> + dlb->dma = pool->dma + i * dlb_size;
> >> + dlb->entries = pool->mem + i * dlb_size;
> >> +
> >> + list_add_tail(&dlb->free, &pool->free);
> >> + }
> >> +
> >> + return pool;
> >> +}
> >> +
> >> +void vsp1_dl_fragment_pool_free(struct vsp1_dl_fragment_pool *pool)
> >> +{
> >> + if (!pool)
> >> + return;
> >
> > Can this happen ?
>
> I was mirroring 'kfree()' support here ... such that error paths can be
> simple.
>
> Would you prefer that it's required to be valid (non-null) pointer?
>
> Actually - I think it is better to leave this for now - as we now call this
> function from the .destroy() entity functions ...
It was a genuine question :-) We have more control over the
vsp1_dl_fragment_pool_free() callers as the function is internal to the
driver. If we have real use cases for pool being NULL then let's keep the
check.
> >> +
> >> + if (pool->mem)
> >> + dma_free_wc(pool->vsp1->bus_master, pool->size, pool->mem,
> >> + pool->dma);
> >> +
> >> + kfree(pool->bodies);
> >> + kfree(pool);
> >> +}
[snip]
--
Regards,
Laurent Pinchart
