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

[Hyungwon Hwang <human.hwang@samsung.com>]

Hello Tobias,

On Tue, 10 Nov 2015 14:24:11 +0100
Tobias Jakobi <tjakobi@math.uni-bielefeld.de> wrote:

> Hello Hyungwon,
> 
> 
> Hyungwon Hwang wrote:
> > Hello Tobias,
> > 
> > On Mon, 09 Nov 2015 10:47:02 +0100
> > Tobias Jakobi <tjakobi@math.uni-bielefeld.de> 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.
> well, that was just my way of thinking. But I guess most people have
> experience using the libc, so the naming should look at least
> 'familiar'.
> 
> 
> 
> > 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.
> Ok, so just that I understand this correctly. Your issue is with the
> commit the description of the test or with the commit description of
> the patch that introduces g2d_move()?
> 
> Because I don't see what you point out in the test commit description:
> 
> "
> tests/exynos: add test for g2d_move
> 
> 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.
> "
> 
> I intentionally avoid to write "...move this pattern around...", so
> instead I choose "shift".
> 
> I'm not a native speaker, so I'm clueless how to formulate this in a
> more clear way.

I'm also not a native speaker, so maybe I could not figure out the
subtle difference between move and shift. I just thought that 'shift'
was just the synonym of 'move'.

> 
> 
> > 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.
> I could add solid fills of the 'empty' areas after each move()
> operation. Would that be more in line what you think the test should
> do?

No. Because g2d_move() is to g2d_copy() what memmove() is to memcpy(),
the current implementation seems enough.

What about change 'move' to 'copy' in the function description?

* g2d_move - copy content inside single buffer.
*     Similar to 'memmove' this copies a rectangular region
*     of the provided buffer to another location (the source
*     and destination region potentially overlapping)

Best regards,
Hyungwon Hwang

> 
> 
> With best wishes,
> Tobias
> 
> 
> 
> 
> > 
> > 
> > BRs,
> > Hyungwon Hwang
> > 
> --
> To unsubscribe from this list: send the line "unsubscribe
> linux-samsung-soc" in the body of a message to
> majordomo@vger.kernel.org More majordomo info at
> http://vger.kernel.org/majordomo-info.html