Re: [PATCH v2 3/8] v4l: vsp1: Convert display lists to use new fragment pool

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Kieran,

On Monday, 11 September 2017 23:27:39 EEST Kieran Bingham wrote:
> On 17/08/17 13:13, Laurent Pinchart wrote:
> > On Monday 14 Aug 2017 16:13:26 Kieran Bingham wrote:
> >> Adapt the dl->body0 object to use an object from the fragment pool.
> >> This greatly reduces the pressure on the TLB for IPMMU use cases, as
> >> all of the lists use a single allocation for the main body.
> >> 
> >> The CLU and LUT objects pre-allocate a pool containing two bodies,
> >> allowing a userspace update before the hardware has committed a previous
> >> set of tables.
> > 
> > I think you'll need three bodies, one for the DL queued to the hardware,
> > one for the pending DL and one for the new DL needed when you update the
> > LUT/CLU. Given that the VSP test suite hasn't caught this problem, we
> > also need a new test :-)
> > 
> >> Fragments are no longer 'freed' in interrupt context, but instead
> >> released back to their respective pools.  This allows us to remove the
> >> garbage collector in the DLM.
> >> 
> >> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> >> 
> >> ---
> >> 
> >> v2:
> >>  - Use dl->body0->max_entries to determine header offset, instead of the
> >>    global constant VSP1_DL_NUM_ENTRIES which is incorrect.
> >>  
> >>  - squash updates for LUT, CLU, and fragment cleanup into single patch.
> >>    (Not fully bisectable when separated)
> >> 
> >> ---
> >> 
> >>  drivers/media/platform/vsp1/vsp1_clu.c |  22 ++-
> >>  drivers/media/platform/vsp1/vsp1_clu.h |   1 +-
> >>  drivers/media/platform/vsp1/vsp1_dl.c  | 223 +++++---------------------
> >>  drivers/media/platform/vsp1/vsp1_dl.h  |   3 +-
> >>  drivers/media/platform/vsp1/vsp1_lut.c |  23 ++-
> >>  drivers/media/platform/vsp1/vsp1_lut.h |   1 +-
> >>  6 files changed, 90 insertions(+), 183 deletions(-)
> > 
> > This is a nice diffstat, but only if you add kerneldoc for the new
> > functions introduced in patch 2/8, otherwise the overall documentation
> > diffstat looks bad :-)

[snip]

> >> diff --git a/drivers/media/platform/vsp1/vsp1_dl.c
> >> b/drivers/media/platform/vsp1/vsp1_dl.c index aab9dd6ec0eb..6ffdc3549283
> >> 100644
> >> --- a/drivers/media/platform/vsp1/vsp1_dl.c
> >> +++ b/drivers/media/platform/vsp1/vsp1_dl.c

[snip]

> >>  static void vsp1_dl_list_free(struct vsp1_dl_list *dl)
> >>  {
> >> 
> >> -	vsp1_dl_body_cleanup(&dl->body0);
> >> -	list_splice_init(&dl->fragments, &dl->dlm->gc_fragments);
> >> +	vsp1_dl_fragment_put(dl->body0);
> >> +	vsp1_dl_list_fragments_free(dl);
> > 
> > I wonder whether the second line is actually needed. vsp1_dl_list_free()
> > is called from vsp1_dlm_destroy() for every entry in the dlm->free list. A
> > DL can only be put in that list by vsp1_dlm_create() or
> > __vsp1_dl_list_put(). The former creates lists with no fragment, while
> > the latter calls vsp1_dl_list_fragments_free() already.
> > 
> > If you're not entirely sure you could add a WARN_ON(!list_empty(&dl-
> > >fragments)) and run the test suite. A comment explaining why the
> > fragments list should already be empty here would be useful too.
> 
> You may be right here, but would you object to leaving it in ?
> 
> Isn't it correct to ensure that the list is completely cleaned up on
> release?
> 
> Furthermore - I would anticipate that in the future - 'body0' could be
> removed, (becoming a fragment) and thus this line would then be required.
> 
> ## /where 's/fragments/bodies/g' applies to the above text. ##

I'm fine with that for now.

> >> +
> >> 
> >>  	kfree(dl);
> >>  }

-- 
Regards,

Laurent Pinchart