Re: [PATCH] dmaengine: pl330: Fix race in residue reporting

[Sjoerd Simons]

On Sat, 2015-11-07 at 21:40 +0900, Krzysztof Kozlowski wrote:
> W dniu 06.11.2015 o 20:11, Sjoerd Simons pisze:
> > When a transfer completes there is a small window between the
> > descriptor
> > being unset as the current active one in the thread and it being
> > marked
> > as done. This causes the residue to be incorrectly set when
> > pl330_tx_status is run in that window. Practically this caused
> > issue for me with audio playback as the residue goes up during a
> > transfer (as the in-progress transfer is no longer accounted for),
> > which makes the higher levels think the audio ringbuffer wrapped
> > around
> > and thus did a sudden big jump forward.
> > 
> > To resolve this, add a field protected by the dma engine lock to
> > indicate the transfer is done but the status hasn't been updated
> > yet.
> > 
> > Also fix the locking in pl330_tx_status, as the function inspects
> > the
> > threads req_running field and queries the dma engine for the
> > current
> > state of the running transfer the dma engine lock needs to be held
> > to
> > ensure the active descriptor doesn't change underneath it.
> > 
> > Signed-off-by: Sjoerd Simons 
> > 
> > ---
> > 
> >  drivers/dma/pl330.c | 10 +-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
> > index 17ee758..6c8243b 100644
> > --- a/drivers/dma/pl330.c
> > +++ b/drivers/dma/pl330.c
> > @@ -503,6 +503,8 @@ struct dma_pl330_desc {
> >     struct pl330_reqcfg rqcfg;
> >  
> >     enum desc_status status;
> > +   /* Transfer completed, but not yet moved to DONE state */
> > +   bool xferred;
> >  
> >     int bytes_requested;
> >     bool last;
> > @@ -1463,6 +1465,9 @@ static void dma_pl330_rqcb(struct
> > dma_pl330_desc *desc, enum pl330_op_err err)
> >     spin_lock_irqsave(>lock, flags);
> >  
> >     desc->status = DONE;
> > +   spin_lock(>thread->dmac->lock);
> > +   desc->xferred = false;
> > +   spin_unlock(>thread->dmac->lock);
> >  
> >     spin_unlock_irqrestore(>lock, flags);
> >  
> > @@ -1595,6 +1600,7 @@ static int pl330_update(struct pl330_dmac
> > *pl330)
> >  
> >     /* Detach the req */
> >     descdone = thrd->req[active].desc;
> > +   descdone->xferred = true;
> >     thrd->req[active].desc = NULL;
> 
> Looking at the code indeed the small window could happen. How can I
> reproduce it? Can you describe your system?

I'm using a Rock 2 square (RK3288 based) board, running Debian Jessie
and simply playing back audio using vlc & pulseaudio.

For some reason when using vlc, pulseaudio is polling the hw pointer
position very very often and seems to hit this tiny window a few times
a minute depending a bit on system load. This causes some audiable
issues and eventually underflowing (as vlc fills the pulseaudio buffer
at a constant rate, but due to this, pulse pushes the data faster then
the actual playback rate every so often).

> As for the change itself, how about adding a new value to
> desc_status?
> Now you are actually introducing a semi-status field.

The reason i picked this way this was was due to the current locking
scheme. The locking order is channel, then controller (when locking
both). While processing the events (and update the active descriptor),
the controller is locked so the status can't be directly updated (as
that's protected by the channel lock). And we can't grab the channel
lock without dopping the controller one first :).

Keeping one status field might well be possible, but would require at
least reworking the locking here.



> Best regards,
> Krzysztof
> 
> >  
> >     thrd->req_running = -1;
> > @@ -2250,13 +2256,14 @@ pl330_tx_status(struct dma_chan *chan,
> > dma_cookie_t cookie,
> >     goto out;
> >  
> >     spin_lock_irqsave(>lock, flags);
> > +   spin_lock(>thread->dmac->lock);
> >  
> >     if (pch->thread->req_running != -1)
> >     running = pch->thread->req[pch->thread-
> > >req_running].desc;
> >  
> >     /* Check in pending list */
> >     list_for_each_entry(desc, >work_list, node) {
> > -   if (desc->status == DONE)
> > +   if (desc->xferred || desc->status == DONE)
> >     transferred = desc->bytes_requested;
> >     else if (running && desc == running)
> >     transferred =
> > @@ -2281,6 +2288,7 @@ pl330_tx_status(struct dma_chan *chan,
> > dma_cookie_t cookie,
> >     if (desc->last)
> >     residual = 0;
> >     }
> > +   spin_unlock(>thread->dmac->lock);
> >     spin_unlock_irqrestore(>lock, flags);
> >  
> >  out:
> > 
> 

-- 
Sjoerd Simons
Collabora Ltd.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] dmaengine: pl330: Fix race in residue reporting

[Krzysztof Kozlowski]

On 09.11.2015 22:12, Sjoerd Simons wrote:
> On Sat, 2015-11-07 at 21:40 +0900, Krzysztof Kozlowski wrote:
>> W dniu 06.11.2015 o 20:11, Sjoerd Simons pisze:
>>> When a transfer completes there is a small window between the
>>> descriptor
>>> being unset as the current active one in the thread and it being
>>> marked
>>> as done. This causes the residue to be incorrectly set when
>>> pl330_tx_status is run in that window. Practically this caused
>>> issue for me with audio playback as the residue goes up during a
>>> transfer (as the in-progress transfer is no longer accounted for),
>>> which makes the higher levels think the audio ringbuffer wrapped
>>> around
>>> and thus did a sudden big jump forward.
>>>
>>> To resolve this, add a field protected by the dma engine lock to
>>> indicate the transfer is done but the status hasn't been updated
>>> yet.
>>>
>>> Also fix the locking in pl330_tx_status, as the function inspects
>>> the
>>> threads req_running field and queries the dma engine for the
>>> current
>>> state of the running transfer the dma engine lock needs to be held
>>> to
>>> ensure the active descriptor doesn't change underneath it.
>>>
>>> Signed-off-by: Sjoerd Simons 
>>>
>>> ---
>>>
>>>  drivers/dma/pl330.c | 10 +-
>>>  1 file changed, 9 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
>>> index 17ee758..6c8243b 100644
>>> --- a/drivers/dma/pl330.c
>>> +++ b/drivers/dma/pl330.c
>>> @@ -503,6 +503,8 @@ struct dma_pl330_desc {
>>> struct pl330_reqcfg rqcfg;
>>>  
>>> enum desc_status status;
>>> +   /* Transfer completed, but not yet moved to DONE state */
>>> +   bool xferred;
>>>  
>>> int bytes_requested;
>>> bool last;
>>> @@ -1463,6 +1465,9 @@ static void dma_pl330_rqcb(struct
>>> dma_pl330_desc *desc, enum pl330_op_err err)
>>> spin_lock_irqsave(>lock, flags);
>>>  
>>> desc->status = DONE;
>>> +   spin_lock(>thread->dmac->lock);
>>> +   desc->xferred = false;
>>> +   spin_unlock(>thread->dmac->lock);
>>>  
>>> spin_unlock_irqrestore(>lock, flags);
>>>  
>>> @@ -1595,6 +1600,7 @@ static int pl330_update(struct pl330_dmac
>>> *pl330)
>>>  
>>> /* Detach the req */
>>> descdone = thrd->req[active].desc;
>>> +   descdone->xferred = true;
>>> thrd->req[active].desc = NULL;
>>
>> Looking at the code indeed the small window could happen. How can I
>> reproduce it? Can you describe your system?
> 
> I'm using a Rock 2 square (RK3288 based) board, running Debian Jessie
> and simply playing back audio using vlc & pulseaudio.
> 
> For some reason when using vlc, pulseaudio is polling the hw pointer
> position very very often and seems to hit this tiny window a few times
> a minute depending a bit on system load. This causes some audiable
> issues and eventually underflowing (as vlc fills the pulseaudio buffer
> at a constant rate, but due to this, pulse pushes the data faster then
> the actual playback rate every so often).

Thanks for description, I don't use pulseaudio on my testing setup but
I'll try to reproduce it anyway.

> 
>> As for the change itself, how about adding a new value to
>> desc_status?
>> Now you are actually introducing a semi-status field.
> 
> The reason i picked this way this was was due to the current locking
> scheme. The locking order is channel, then controller (when locking
> both). While processing the events (and update the active descriptor),
> the controller is locked so the status can't be directly updated (as
> that's protected by the channel lock). And we can't grab the channel
> lock without dopping the controller one first :).
> 
> Keeping one status field might well be possible, but would require at
> least reworking the locking here.

Right, the locking requires new field... If going this way could you
describe this rationale and information about locking a comment? I mean
the answer you gave above - the locking requires such field and that the
field is protected actually by controller lock, not by channel.

On the other hand, I am looking and the code and looking and I wonder:
do you really need to protect writing to the "xferred" field? In
pl330_update() you are setting it to true while getting the list of done
descriptors.
The descriptors done are then removed from the thread.

In that path then you are setting it to false in dma_pl330_rqcb() for
these done descriptors - this should not be executed concurrently with
above.

The dma_pl330_rqcb() is also called from abort and fail paths but these
are use controller's lock (e.g. pl330_dotask()).

Just thinking out loud... maybe I am missing something here.

Best regards,
Krzysztof



> 
> 
>> Best regards,
>> Krzysztof
>>
>>>  
>>> thrd->req_running = -1;
>>> @@ -2250,13 +2256,14 @@ pl330_tx_status(struct dma_chan *chan,
>>> dma_cookie_t cookie,
>>> goto out;
>>>  
>>> 

Re: [PATCH v4] tests/exynos: add fimg2d performance analysis

[Hyungwon Hwang]

Hello Tobias,

On Mon, 09 Nov 2015 10:47:13 +0100
Tobias Jakobi  wrote:

> Hello Hyungwon,
> 
> 
> Hyungwon Hwang wrote:
> > Hello,
> > 
> > I think this patch should update .gitignore, not for adding the
> > built binary to untracked file list.
> Thanks!
> 
> 
> > Also, I want to make clear about the purpose of this test program.
> > What do you want to get after this test? This program runs G2D with
> > randomly chosen number of pixel and shows the elapsed time to do
> > that. I run it on my board. But I could not find any meaning of the
> > test. If you just want to know the execution time of solid fill,
> > what about get the width and height from user and run the same tests
> > iteratively for more accurate result? Or at least, increasing
> > number of pixels?
> The test is to measure the dependency between amount of pixels the G2D
> has to process and the amount of time for the G2D to process such
> pixels.
> 
> It's exactly what a performance test should do, measure the time it
> takes for a certain workload to complete.
> 
> In particular the test wants to answer the question if the dependency
> stated above is of linear type.
> 
> Of course it's not, since we have setup time, so at least it should be
> affine linear. But even that is not true, since you see subtle
> 'branching' when doing high density plots (that's why I added export
> of the data to Mathematica).
> 
> 
> What you ask for (user input) is in fact already implemented. The user
> can specify the buffer width and height, which in turn limits the size
> of the rectangle that is solid filled.
> 
> If you want smaller rectangles filled, decrease buffer width and
> height, if you want bigger ones filled, increase.
> 
> 
> The second purpose is to stress test the G2D, as already indicated in
> the commit description. The G2D can be overclocked quite a lot under
> certain conditions. With increase MIF/INT voltages I can run it with
> 400MHz instead of the 200MHz defaults. The application can now be used
> to check stability. E.g. if voltages are too low the system can
> quickly lock-up.
> 
> In particular one could also check how processing time depends on the
> clock rate of the G2D. One interesting question here is how memory
> bandwidth limits us.
> 
> 
> 
> With best wishes,
> Tobias

Yes. I agree with the broad view. Please see the below, I run the test
2 times in a row.

root@localhost:~# ./exynos_fimg2d_perf  -i 10 -w 1024 -h 1024   
exynos/fimg2d: G2D version (4.1).
starting simple G2D performance test
buffer width = 1024, buffer height = 1024, iterations = 10
num_pixels = 136000, usecs = 236000
num_pixels = 8492, usecs = 47083
num_pixels = 100688, usecs = 200042
num_pixels = 141312, usecs = 216667
num_pixels = 39962, usecs = 92708
num_pixels = 95046, usecs = 156542
num_pixels = 2562, usecs = 34666
num_pixels = 176485, usecs = 326916
num_pixels = 17760, usecs = 56625
num_pixels = 1625, usecs = 31833
starting multi G2D performance test (batch size = 3)
buffer width = 1024, buffer height = 1024, iterations = 10
num_pixels = 245180, usecs = 385083
num_pixels = 276320, usecs = 398625
num_pixels = 196807, usecs = 35
num_pixels = 305540, usecs = 420458
num_pixels = 65978, usecs = 120250
num_pixels = 265028, usecs = 379417
num_pixels = 139079, usecs = 213667
num_pixels = 24970, usecs = 67625
num_pixels = 46808, usecs = 114125
num_pixels = 100804, usecs = 179750
root@localhost:~# ./exynos_fimg2d_perf  -i 10 -w 1024 -h 1024 
exynos/fimg2d: G2D version (4.1).
starting simple G2D performance test
buffer width = 1024, buffer height = 1024, iterations = 10
num_pixels = 18676, usecs = 95541
num_pixels = 117056, usecs = 218875
num_pixels = 80784, usecs = 137209
num_pixels = 427, usecs = 33209
num_pixels = 238044, usecs = 403041
num_pixels = 4392, usecs = 37709
num_pixels = 19880, usecs = 59750
num_pixels = 3666, usecs = 36542
num_pixels = 4630, usecs = 36166
num_pixels = 70834, usecs = 125917
starting multi G2D performance test (batch size = 3)
buffer width = 1024, buffer height = 1024, iterations = 10
num_pixels = 216516, usecs = 347042
num_pixels = 242863, usecs = 422417
num_pixels = 28176, usecs = 72292
num_pixels = 110713, usecs = 179167
num_pixels = 292266, usecs = 431750
num_pixels = 274127, usecs = 392833
num_pixels = 291659, usecs = 415875
num_pixels = 140202, usecs = 218833
num_pixels = 122400, usecs = 193084
num_pixels = 168647, usecs = 251375

As you said, I can adjust the buffer width and height. But because the
program choose the number of pixel to process randomly, I can't compare
the result after I modified something (clock, or something else like
you mentioned). Also I can figure out the tendency between the number
of pixels and the processing time after I draw the graph. But it is too
hard not by doing that, because the number of pixels are not in
increasing order at least. I think that this program will be more useful
if the user can control the whole situation more tightly. But if you
need 

Re: [PATCH 09/13] exynos/fimg2d: add g2d_move

[Hyungwon Hwang]

Hello Tobias,

On Mon, 09 Nov 2015 10:47:02 +0100
Tobias Jakobi  wrote:

> Hello Hyungwon,
> 
> 
> Hyungwon Hwang wrote:
> > Hello Tobias,
> > 
> > I was in vacation last week, so I could run your code today. I found
> > that what g2d_move() does is actually copying not moving, because
> > the operation does not clear the previous area.
> I choose g2d_move() because we also have memcpy() and memmove() in
> libc. Like in libc 'moving' memory doesn't actually move it, like you
> would move a real object, since it's undefined what 'empty' memory
> should be.
> 
> The same applies here. It's not defined what 'empty' areas of the
> buffer should be.
> 
> 
> > Would it be possible to
> > generalize g2d_copy() works better, so it could works well in case
> > of the src buffer and dst buffer being same.
> I think this would break g2d_copy() backward compatibility.
> 
> I also want the user to explicitly request this. The user should make
> sure what requirements he has for the buffers in question. Are the
> buffers disjoint or not?
> 
> 
> > If it is possible, I think it
> > would be better way to do that. If it is not, at least chaning the
> > function name is needed. I tested it on my Odroid U3 board.
> I don't have a strong opinion on the naming. Any recommendations?
> 
> I still think the naming is fine though, since it mirrors libc's
> naming. And the user is supposed to read the documentation anyway.

> 
> 
> 
> With best wishes,
> Tobias

In that manner following glibc, I agree that the naming is reasonable.
I commented like that previously, because at the first time when I run
the test, I think that the result seems like a bug. The test program
said that it was a move test, but the operation seemed copying. It
would be just OK if it is well documented or printed when runs the test
that the test does not do anything about the previous area
intentionally.


BRs,
Hyungwon Hwang

> 
> > 
> > Best regards,
> > Hyungwon Hwang
> > 
> > On Tue, 22 Sep 2015 17:54:58 +0200
> > Tobias Jakobi  wrote:
> > 
> >> We already have g2d_copy() which implements G2D copy
> >> operations from one buffer to another. However we can't
> >> do a overlapping copy operation in one buffer.
> >>
> >> Add g2d_move() which acts like the standard memmove()
> >> and properly handles overlapping copies.
> >>
> >> Signed-off-by: Tobias Jakobi 
> >> ---
> >>  exynos/exynos_fimg2d.c | 94
> >> ++
> >> exynos/exynos_fimg2d.h |  3 ++ 2 files changed, 97 insertions(+)
> >>
> >> diff --git a/exynos/exynos_fimg2d.c b/exynos/exynos_fimg2d.c
> >> index 4d5419c..8703629 100644
> >> --- a/exynos/exynos_fimg2d.c
> >> +++ b/exynos/exynos_fimg2d.c
> >> @@ -540,6 +540,100 @@ g2d_copy(struct g2d_context *ctx, struct
> >> g2d_image *src, }
> >>  
> >>  /**
> >> + * g2d_move - move content inside single buffer.
> >> + *Similar to 'memmove' this moves a rectangular region
> >> + *of the provided buffer to another location (the source
> >> + *and destination region potentially overlapping).
> >> + *
> >> + * @ctx: a pointer to g2d_context structure.
> >> + * @img: a pointer to g2d_image structure providing
> >> + *buffer information.
> >> + * @src_x: x position of source rectangle.
> >> + * @src_y: y position of source rectangle.
> >> + * @dst_x: x position of destination rectangle.
> >> + * @dst_y: y position of destination rectangle.
> >> + * @w: width of rectangle to move.
> >> + * @h: height of rectangle to move.
> >> + */
> >> +int
> >> +g2d_move(struct g2d_context *ctx, struct g2d_image *img,
> >> +  unsigned int src_x, unsigned int src_y,
> >> +  unsigned int dst_x, unsigned dst_y, unsigned int
> >> w,
> >> +  unsigned int h)
> >> +{
> >> +  union g2d_rop4_val rop4;
> >> +  union g2d_point_val pt;
> >> +  union g2d_direction_val dir;
> >> +  unsigned int src_w, src_h, dst_w, dst_h;
> >> +
> >> +  src_w = w;
> >> +  src_h = h;
> >> +  if (src_x + img->width > w)
> >> +  src_w = img->width - src_x;
> >> +  if (src_y + img->height > h)
> >> +  src_h = img->height - src_y;
> >> +
> >> +  dst_w = w;
> >> +  dst_h = w;
> >> +  if (dst_x + img->width > w)
> >> +  dst_w = img->width - dst_x;
> >> +  if (dst_y + img->height > h)
> >> +  dst_h = img->height - dst_y;
> >> +
> >> +  w = MIN(src_w, dst_w);
> >> +  h = MIN(src_h, dst_h);
> >> +
> >> +  if (w == 0 || h == 0) {
> >> +  fprintf(stderr, MSG_PREFIX "invalid width or
> >> height.\n");
> >> +  return -EINVAL;
> >> +  }
> >> +
> >> +  if (g2d_check_space(ctx, 13, 2))
> >> +  return -ENOSPC;
> >> +
> >> +  g2d_add_cmd(ctx, DST_SELECT_REG, G2D_SELECT_MODE_BGCOLOR);
> >> +  g2d_add_cmd(ctx, SRC_SELECT_REG, G2D_SELECT_MODE_NORMAL);
> >> +
> >> +  g2d_add_cmd(ctx, DST_COLOR_MODE_REG, img->color_mode);
> >> +  g2d_add_cmd(ctx, SRC_COLOR_MODE_REG, img->color_mode);
> 

Re: [Y2038] [PATCH v2 9/9] [media] omap3isp: support 64-bit version of omap3isp_stat_data

[Arnd Bergmann]

On Monday 09 November 2015 22:09:26 Laurent Pinchart wrote:
> Hi Arnd,
> 
> Thank you for the patch.
> 
> On Thursday 17 September 2015 23:19:40 Arnd Bergmann wrote:
> > C libraries with 64-bit time_t use an incompatible format for
> > struct omap3isp_stat_data. This changes the kernel code to
> > support either version, by moving over the normal handling
> > to the 64-bit variant, and adding compatiblity code to handle
> > the old binary format with the existing ioctl command code.
> > 
> > Fortunately, the command code includes the size of the structure,
> > so the difference gets handled automatically.
> 
> We plan to design a new interface to handle statistics in V4L2. That API 
> should support proper 64-bit timestamps out of the box, and will be 
> implemented by the OMAP3 ISP driver. Userspace should then move to it. I 
> wonder if it's worth it to fix the existing VIDIOC_OMAP3ISP_STAT_REQ ioctl 
> given that I expect it to have a handful of users at most.

We still need to do something to the driver. The alternative would
be to make the existing ioctl command optional at kernel compile-time
so we can still build the driver once we remove the 'struct timeval'
definition. That patch would add slightly less complexity here
but also lose functionality.

As my patch here depends on the struct v4l2_timeval I introduced in
an earlier patch of the series, we will have to change it anyways,
but I'd prefer to keep the basic idea. Let's get back to this one
after the v4l_buffer replacement work is done.

Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 9/9] [media] omap3isp: support 64-bit version of omap3isp_stat_data

[Laurent Pinchart]

Hi Arnd,

Thank you for the patch.

On Thursday 17 September 2015 23:19:40 Arnd Bergmann wrote:
> C libraries with 64-bit time_t use an incompatible format for
> struct omap3isp_stat_data. This changes the kernel code to
> support either version, by moving over the normal handling
> to the 64-bit variant, and adding compatiblity code to handle
> the old binary format with the existing ioctl command code.
> 
> Fortunately, the command code includes the size of the structure,
> so the difference gets handled automatically.

We plan to design a new interface to handle statistics in V4L2. That API 
should support proper 64-bit timestamps out of the box, and will be 
implemented by the OMAP3 ISP driver. Userspace should then move to it. I 
wonder if it's worth it to fix the existing VIDIOC_OMAP3ISP_STAT_REQ ioctl 
given that I expect it to have a handful of users at most.

> Signed-off-by: Arnd Bergmann 
> ---
>  drivers/media/platform/omap3isp/isph3a_aewb.c |  2 ++
>  drivers/media/platform/omap3isp/isph3a_af.c   |  2 ++
>  drivers/media/platform/omap3isp/isphist.c |  2 ++
>  drivers/media/platform/omap3isp/ispstat.c | 18 --
>  drivers/media/platform/omap3isp/ispstat.h |  2 ++
>  include/uapi/linux/omap3isp.h | 19 +++
>  6 files changed, 43 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/omap3isp/isph3a_aewb.c
> b/drivers/media/platform/omap3isp/isph3a_aewb.c index
> ccaf92f39236..2d567725608f 100644
> --- a/drivers/media/platform/omap3isp/isph3a_aewb.c
> +++ b/drivers/media/platform/omap3isp/isph3a_aewb.c
> @@ -250,6 +250,8 @@ static long h3a_aewb_ioctl(struct v4l2_subdev *sd,
> unsigned int cmd, void *arg) return omap3isp_stat_config(stat, arg);
>   case VIDIOC_OMAP3ISP_STAT_REQ:
>   return omap3isp_stat_request_statistics(stat, arg);
> + case VIDIOC_OMAP3ISP_STAT_REQ_TIME32:
> + return omap3isp_stat_request_statistics_time32(stat, arg);
>   case VIDIOC_OMAP3ISP_STAT_EN: {
>   unsigned long *en = arg;
>   return omap3isp_stat_enable(stat, !!*en);
> diff --git a/drivers/media/platform/omap3isp/isph3a_af.c
> b/drivers/media/platform/omap3isp/isph3a_af.c index
> 92937f7eecef..2ac02371318b 100644
> --- a/drivers/media/platform/omap3isp/isph3a_af.c
> +++ b/drivers/media/platform/omap3isp/isph3a_af.c
> @@ -314,6 +314,8 @@ static long h3a_af_ioctl(struct v4l2_subdev *sd,
> unsigned int cmd, void *arg) return omap3isp_stat_config(stat, arg);
>   case VIDIOC_OMAP3ISP_STAT_REQ:
>   return omap3isp_stat_request_statistics(stat, arg);
> + case VIDIOC_OMAP3ISP_STAT_REQ_TIME32:
> + return omap3isp_stat_request_statistics_time32(stat, arg);
>   case VIDIOC_OMAP3ISP_STAT_EN: {
>   int *en = arg;
>   return omap3isp_stat_enable(stat, !!*en);
> diff --git a/drivers/media/platform/omap3isp/isphist.c
> b/drivers/media/platform/omap3isp/isphist.c index
> 7138b043a4aa..669b97b079ee 100644
> --- a/drivers/media/platform/omap3isp/isphist.c
> +++ b/drivers/media/platform/omap3isp/isphist.c
> @@ -436,6 +436,8 @@ static long hist_ioctl(struct v4l2_subdev *sd, unsigned
> int cmd, void *arg) return omap3isp_stat_config(stat, arg);
>   case VIDIOC_OMAP3ISP_STAT_REQ:
>   return omap3isp_stat_request_statistics(stat, arg);
> + case VIDIOC_OMAP3ISP_STAT_REQ_TIME32:
> + return omap3isp_stat_request_statistics_time32(stat, arg);
>   case VIDIOC_OMAP3ISP_STAT_EN: {
>   int *en = arg;
>   return omap3isp_stat_enable(stat, !!*en);
> diff --git a/drivers/media/platform/omap3isp/ispstat.c
> b/drivers/media/platform/omap3isp/ispstat.c index
> fa96e330c563..3d70332422b5 100644
> --- a/drivers/media/platform/omap3isp/ispstat.c
> +++ b/drivers/media/platform/omap3isp/ispstat.c
> @@ -496,8 +496,7 @@ int omap3isp_stat_request_statistics(struct ispstat
> *stat, return PTR_ERR(buf);
>   }
> 
> - data->ts.tv_sec = buf->ts.tv_sec;
> - data->ts.tv_usec = buf->ts.tv_usec;
> + data->ts = buf->ts;
>   data->config_counter = buf->config_counter;
>   data->frame_number = buf->frame_number;
>   data->buf_size = buf->buf_size;
> @@ -509,6 +508,21 @@ int omap3isp_stat_request_statistics(struct ispstat
> *stat, return 0;
>  }
> 
> +int omap3isp_stat_request_statistics_time32(struct ispstat *stat,
> + struct omap3isp_stat_data_time32 *data)
> +{
> + struct omap3isp_stat_data data64;
> + int ret;
> +
> + ret = omap3isp_stat_request_statistics(stat, );
> +
> + data->ts.tv_sec = data64.ts.tv_sec;
> + data->ts.tv_usec = data64.ts.tv_usec;
> + memcpy(>buf, , sizeof(*data) - sizeof(data->ts));
> +
> + return ret;
> +}
> +
>  /*
>   * omap3isp_stat_config - Receives new statistic engine configuration.
>   * @new_conf: Pointer to config structure.
> diff --git a/drivers/media/platform/omap3isp/ispstat.h
> 

Re: [PATCH 09/13] exynos/fimg2d: add g2d_move

[Tobias Jakobi]

Hello Hyungwon,


Hyungwon Hwang wrote:
> Hello Tobias,
> 
> I was in vacation last week, so I could run your code today. I found
> that what g2d_move() does is actually copying not moving, because the
> operation does not clear the previous area.
I choose g2d_move() because we also have memcpy() and memmove() in libc.
Like in libc 'moving' memory doesn't actually move it, like you would
move a real object, since it's undefined what 'empty' memory should be.

The same applies here. It's not defined what 'empty' areas of the buffer
should be.


> Would it be possible to
> generalize g2d_copy() works better, so it could works well in case of
> the src buffer and dst buffer being same.
I think this would break g2d_copy() backward compatibility.

I also want the user to explicitly request this. The user should make
sure what requirements he has for the buffers in question. Are the
buffers disjoint or not?


> If it is possible, I think it
> would be better way to do that. If it is not, at least chaning the
> function name is needed. I tested it on my Odroid U3 board.
I don't have a strong opinion on the naming. Any recommendations?

I still think the naming is fine though, since it mirrors libc's naming.
And the user is supposed to read the documentation anyway.



With best wishes,
Tobias

> 
> Best regards,
> Hyungwon Hwang
> 
> On Tue, 22 Sep 2015 17:54:58 +0200
> Tobias Jakobi  wrote:
> 
>> We already have g2d_copy() which implements G2D copy
>> operations from one buffer to another. However we can't
>> do a overlapping copy operation in one buffer.
>>
>> Add g2d_move() which acts like the standard memmove()
>> and properly handles overlapping copies.
>>
>> Signed-off-by: Tobias Jakobi 
>> ---
>>  exynos/exynos_fimg2d.c | 94
>> ++
>> exynos/exynos_fimg2d.h |  3 ++ 2 files changed, 97 insertions(+)
>>
>> diff --git a/exynos/exynos_fimg2d.c b/exynos/exynos_fimg2d.c
>> index 4d5419c..8703629 100644
>> --- a/exynos/exynos_fimg2d.c
>> +++ b/exynos/exynos_fimg2d.c
>> @@ -540,6 +540,100 @@ g2d_copy(struct g2d_context *ctx, struct
>> g2d_image *src, }
>>  
>>  /**
>> + * g2d_move - move content inside single buffer.
>> + *  Similar to 'memmove' this moves a rectangular region
>> + *  of the provided buffer to another location (the source
>> + *  and destination region potentially overlapping).
>> + *
>> + * @ctx: a pointer to g2d_context structure.
>> + * @img: a pointer to g2d_image structure providing
>> + *  buffer information.
>> + * @src_x: x position of source rectangle.
>> + * @src_y: y position of source rectangle.
>> + * @dst_x: x position of destination rectangle.
>> + * @dst_y: y position of destination rectangle.
>> + * @w: width of rectangle to move.
>> + * @h: height of rectangle to move.
>> + */
>> +int
>> +g2d_move(struct g2d_context *ctx, struct g2d_image *img,
>> +unsigned int src_x, unsigned int src_y,
>> +unsigned int dst_x, unsigned dst_y, unsigned int w,
>> +unsigned int h)
>> +{
>> +union g2d_rop4_val rop4;
>> +union g2d_point_val pt;
>> +union g2d_direction_val dir;
>> +unsigned int src_w, src_h, dst_w, dst_h;
>> +
>> +src_w = w;
>> +src_h = h;
>> +if (src_x + img->width > w)
>> +src_w = img->width - src_x;
>> +if (src_y + img->height > h)
>> +src_h = img->height - src_y;
>> +
>> +dst_w = w;
>> +dst_h = w;
>> +if (dst_x + img->width > w)
>> +dst_w = img->width - dst_x;
>> +if (dst_y + img->height > h)
>> +dst_h = img->height - dst_y;
>> +
>> +w = MIN(src_w, dst_w);
>> +h = MIN(src_h, dst_h);
>> +
>> +if (w == 0 || h == 0) {
>> +fprintf(stderr, MSG_PREFIX "invalid width or
>> height.\n");
>> +return -EINVAL;
>> +}
>> +
>> +if (g2d_check_space(ctx, 13, 2))
>> +return -ENOSPC;
>> +
>> +g2d_add_cmd(ctx, DST_SELECT_REG, G2D_SELECT_MODE_BGCOLOR);
>> +g2d_add_cmd(ctx, SRC_SELECT_REG, G2D_SELECT_MODE_NORMAL);
>> +
>> +g2d_add_cmd(ctx, DST_COLOR_MODE_REG, img->color_mode);
>> +g2d_add_cmd(ctx, SRC_COLOR_MODE_REG, img->color_mode);
>> +
>> +g2d_add_base_addr(ctx, img, g2d_dst);
>> +g2d_add_base_addr(ctx, img, g2d_src);
>> +
>> +g2d_add_cmd(ctx, DST_STRIDE_REG, img->stride);
>> +g2d_add_cmd(ctx, SRC_STRIDE_REG, img->stride);
>> +
>> +dir.val[0] = dir.val[1] = 0;
>> +
>> +if (dst_x >= src_x)
>> +dir.data.src_x_direction = dir.data.dst_x_direction
>> = 1;
>> +if (dst_y >= src_y)
>> +dir.data.src_y_direction = dir.data.dst_y_direction
>> = 1; +
>> +g2d_set_direction(ctx, );
>> +
>> +pt.data.x = src_x;
>> +pt.data.y = src_y;
>> +g2d_add_cmd(ctx, SRC_LEFT_TOP_REG, pt.val);
>> +pt.data.x = src_x + w;
>> +pt.data.y = src_y + h;
>> +g2d_add_cmd(ctx, SRC_RIGHT_BOTTOM_REG, pt.val);
>> +
>> +pt.data.x = dst_x;
>> +

Re: [PATCH 10/13] tests/exynos: add test for g2d_move

[Tobias Jakobi]

Hello Hyungwon,


Hyungwon Hwang wrote:
> Hello,
> 
> I think this patch should update .gitignore, not for adding the built
> binary to untracked file list.
good point. I should do this for the event test as well I guess.

Going to respin the series.


With best wishes,
Tobias


> But without it, it looks good to me, and I tested it on my Odroid U3
> board.
> 
> Tested-by: Hyungwon Hwang 
> Reviewed-by: Hyungwon Hwang 
> 
> Best regards,
> Hyungwon Hwang
> 
> 
> On Tue, 22 Sep 2015 17:54:59 +0200
> Tobias Jakobi  wrote:
> 
>> To check if g2d_move() works properly we create a small checkerboard
>> pattern in the center of the screen and then shift this pattern
>> around with g2d_move(). The pattern should be properly preserved
>> by the operation.
>>
>> Signed-off-by: Tobias Jakobi 
>> ---
>>  tests/exynos/exynos_fimg2d_test.c | 132
>> ++ 1 file changed, 132
>> insertions(+)
>>
>> diff --git a/tests/exynos/exynos_fimg2d_test.c
>> b/tests/exynos/exynos_fimg2d_test.c index dfb00a0..797fb6e 100644
>> --- a/tests/exynos/exynos_fimg2d_test.c
>> +++ b/tests/exynos/exynos_fimg2d_test.c
>> @@ -313,6 +313,130 @@ fail:
>>  return ret;
>>  }
>>  
>> +static int g2d_move_test(struct exynos_device *dev,
>> +struct exynos_bo *tmp,
>> +struct exynos_bo *buf,
>> +enum e_g2d_buf_type type)
>> +{
>> +struct g2d_context *ctx;
>> +struct g2d_image img = {0}, tmp_img = {0};
>> +unsigned int img_w, img_h, count;
>> +int cur_x, cur_y;
>> +void *checkerboard;
>> +int ret;
>> +
>> +static const struct g2d_step {
>> +int x, y;
>> +} steps[] = {
>> +{ 1,  0}, { 0,  1},
>> +{-1,  0}, { 0, -1},
>> +{ 1,  1}, {-1, -1},
>> +{ 1, -1}, {-1,  1},
>> +{ 2,  1}, { 1,  2},
>> +{-2, -1}, {-1, -2},
>> +{ 2, -1}, { 1, -2},
>> +{-2,  1}, {-1,  2}
>> +};
>> +static const unsigned int num_steps =
>> +sizeof(steps) / sizeof(struct g2d_step);
>> +
>> +ctx = g2d_init(dev->fd);
>> +if (!ctx)
>> +return -EFAULT;
>> +
>> +img.bo[0] = buf->handle;
>> +
>> +/* create pattern of half the screen size */
>> +checkerboard = create_checkerboard_pattern(screen_width /
>> 64, screen_height / 64, 32);
>> +if (!checkerboard) {
>> +ret = -EFAULT;
>> +goto fail;
>> +}
>> +
>> +img_w = (screen_width / 64) * 32;
>> +img_h = (screen_height / 64) * 32;
>> +
>> +switch (type) {
>> +case G2D_IMGBUF_GEM:
>> +memcpy(tmp->vaddr, checkerboard, img_w * img_h * 4);
>> +tmp_img.bo[0] = tmp->handle;
>> +break;
>> +case G2D_IMGBUF_USERPTR:
>> +tmp_img.user_ptr[0].userptr = (unsigned
>> long)checkerboard;
>> +tmp_img.user_ptr[0].size = img_w * img_h * 4;
>> +break;
>> +case G2D_IMGBUF_COLOR:
>> +default:
>> +ret = -EFAULT;
>> +goto fail;
>> +}
>> +
>> +/* solid fill framebuffer with white color */
>> +img.width = screen_width;
>> +img.height = screen_height;
>> +img.stride = screen_width * 4;
>> +img.buf_type = G2D_IMGBUF_GEM;
>> +img.color_mode = G2D_COLOR_FMT_ARGB | G2D_ORDER_AXRGB;
>> +img.color = 0x;
>> +
>> +/* put checkerboard pattern in the center of the framebuffer
>> */
>> +cur_x = (screen_width - img_w) / 2;
>> +cur_y = (screen_height - img_h) / 2;
>> +tmp_img.width = img_w;
>> +tmp_img.height = img_h;
>> +tmp_img.stride = img_w * 4;
>> +tmp_img.buf_type = type;
>> +tmp_img.color_mode = G2D_COLOR_FMT_ARGB |
>> G2D_ORDER_AXRGB; +
>> +ret = g2d_solid_fill(ctx, , 0, 0, screen_width,
>> screen_height) ||
>> +g2d_copy(ctx, _img, , 0, 0, cur_x, cur_y,
>> img_w, img_h); +
>> +if (!ret)
>> +ret = g2d_exec(ctx);
>> +if (ret < 0)
>> +goto fail;
>> +
>> +printf("move test with %s.\n",
>> +type == G2D_IMGBUF_GEM ? "gem" : "userptr");
>> +
>> +srand(time(NULL));
>> +for (count = 0; count < 256; ++count) {
>> +const struct g2d_step *s;
>> +
>> +/* select step and validate it */
>> +while (1) {
>> +s = [random() % num_steps];
>> +
>> +if (cur_x + s->x < 0 || cur_y + s->y < 0 ||
>> +cur_x + img_w + s->x >= screen_width
>> ||
>> +cur_y + img_h + s->y >=
>> screen_height)
>> +continue;
>> +else
>> +break;
>> +}
>> +
>> +ret = g2d_move(ctx, , cur_x, cur_y, cur_x +
>> s->x, cur_y + s->y,
>> +img_w, img_h);

Re: [PATCH 10/13] tests/exynos: add test for g2d_move

[Emil Velikov]

On 9 November 2015 at 09:47, Tobias Jakobi
 wrote:
> Hello Hyungwon,
>
> Hyungwon Hwang wrote:
>> Hello,
>>
>> I think this patch should update .gitignore, not for adding the built
>> binary to untracked file list.
> good point. I should do this for the event test as well I guess.
>
> Going to respin the series.
>
Imho you can do that as a single patch on top. There is no problem if
git status prints out a binary name for a couple of commits.

Regards,
Emil
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html