Re: [PATCH v2 00/13] drm/exynos: async G2D and g2d_move()

[Emil Velikov]

On 27 November 2015 at 02:11, Hyungwon Hwang <human.hw...@samsung.com> wrote:
> Dear All,
>
> On Thu, 26 Nov 2015 16:35:29 +
> Emil Velikov <emil.l.veli...@gmail.com> wrote:
>
>> Hi Tobias,
>>
>> On 22 November 2015 at 18:48, Tobias Jakobi
>> <tjak...@math.uni-bielefeld.de> wrote:
>> > Hello,
>> >
>> > this series mostly touches G2D code. It introduces the following:
>> >
>> > (1) drmHandleEvent2() is added to enable processing of
>> > vendor-specific events. This will be used to expose asynchronous
>> > operation of the G2D. The necessary kernel infrastructure is
>> > already there since a lot of kernel versions. [This touches libdrm
>> > core code!]
>> >
>> Considering the shortage of input on this can you please respin the
>> series without it (and related work mentioned below). This way we can
>> pick merge the remaining work now and discuss (ping) about the rest.
>
> I am confused a little here. Did you mean that adding drmHandleEvent2()
> is not necessary, and the related code which uses drmHandleEvent2() must
> be re-implemented?
>
I mean that there has been no interest/collaboration from anyone else.
The shortage of userspace, doesn't help much either. Both of these can
be hand-waved, somewhat, if this was exynos only change, but it isn't
:-(

Thus in order to not stall the remaining patches, I'm suggesting that
we split this [and other patches that depend on it] into a
separate/parallel series.

>>
>> > (2) The necessary infrastructure to handle G2D events. This includes
>> > adding g2d_config_event() and g2d_exec2() to the public API.
>> > A test application is provided to ensure that everything works
>> > as expected.
>> >
>> With above in mind the g2d event will need to be split out, although
>> g2d_exec2() should be ok (although of limited use), imho.
>>
>> > (3) A small performance test application which can be used to
>> > measure the speed of solid color clear operations. Interesting for
>> > benchmarking and plotting colorful graphs (e.g. through
>> > Mathematica).
>> >
>> > (4) g2d_move() which works similar to g2d_copy() but like the C
>> > memmove() properly handles overlapping buffer copies.
>> > Again a test application is present to check that this
>> > indeed does what it should.
>> >
>> > (5) Various small changes. A framebuffer colorformat fix for the
>> > general G2D test application. Moving the currently unused
>> > g2d_reset() to the public API.
>> I am more of a "add API when it's needed" kind of person, although I
>> cannot see anything serious misuse that can arise from g2d_reset().
>>
>> > Adding a counterpart to
>> > exynos_bo_map() to unmap buffers again.
>> >
>> The exynos bo map compatibility was broken a few times already so I'm
>> wondering if we really want this one. I guess that with the lack of
>> any (outside of tizen) user space things cannot go that wrong :-P
>>
>
> Yes. I agree that adding them at this time is not needed and it would
> be OK to add them, when the userspace using them comes.
>
You guys/gals are paving the way for exynos - so it's not my call to
decide. I was just pointing out some questionable bits from the past.

As I mentioned Tizen, do you plan on cleaning up your libdrm (and
other?) patches and sending them over ?


In case I haven't mentioned this before - feel free to come and join
us on IRC channel #dri-devel (at FreeNode). Many kernel and userspace
DRM developers hang out in there and you can poke, collaborate and
discuss things in real time :-) This invite includes youself, other
exynos DRM developers and everyone else interested.


Thanks for the picking up Tobias' work, Hyungwon!

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

Re: [PATCH v2 08/13] exynos: fimg2d: add g2d_set_direction

[Emil Velikov]

On 22 November 2015 at 18:48, Tobias Jakobi
 wrote:
> This allows setting the two direction registers, which specify how
> the engine blits pixels. This can be used for overlapping blits,
> which happen e.g. when 'moving' a rectangular region inside a
> fixed buffer.
>
> Reviewed-by: Hyungwon Hwang 
> Signed-off-by: Tobias Jakobi 
> ---
>  exynos/exynos_fimg2d.c | 13 +
>  exynos/exynos_fimg2d.h | 38 ++
>  2 files changed, 51 insertions(+)
>
> diff --git a/exynos/exynos_fimg2d.c b/exynos/exynos_fimg2d.c
> index e997d4b..4d5419c 100644
> --- a/exynos/exynos_fimg2d.c
> +++ b/exynos/exynos_fimg2d.c
> @@ -242,6 +242,19 @@ static void g2d_add_base_addr(struct g2d_context *ctx, 
> struct g2d_image *img,
>  }
>
>  /*
> + * g2d_set_direction - setup direction register (useful for overlapping 
> blits).
> + *
> + * @ctx: a pointer to g2d_context structure.
> + * @dir: a pointer to the g2d_direction_val structure.
> + */
> +static void g2d_set_direction(struct g2d_context *ctx,
> +   const union g2d_direction_val *dir)
> +{
> +   g2d_add_cmd(ctx, SRC_MASK_DIRECT_REG, dir->val[0]);
> +   g2d_add_cmd(ctx, DST_PAT_DIRECT_REG, dir->val[1]);
> +}
> +
> +/*
>   * g2d_reset - reset fimg2d hardware.
>   *
>   * @ctx: a pointer to g2d_context structure.
> diff --git a/exynos/exynos_fimg2d.h b/exynos/exynos_fimg2d.h
> index c6b58ac..9eee7c0 100644
> --- a/exynos/exynos_fimg2d.h
> +++ b/exynos/exynos_fimg2d.h
> @@ -189,6 +189,11 @@ enum e_g2d_exec_flag {
> G2D_EXEC_FLAG_ASYNC = (1 << 0)
>  };
>
> +enum e_g2d_dir_mode {
> +   G2D_DIR_MODE_POSITIVE = 0,
> +   G2D_DIR_MODE_NEGATIVE = 1
> +};
> +
>  union g2d_point_val {
> unsigned int val;
> struct {
> @@ -269,6 +274,39 @@ union g2d_blend_func_val {
> } data;
>  };
>
> +union g2d_direction_val {
> +   unsigned int val[2];
> +   struct {
> +   /* SRC_MSK_DIRECT_REG [0:1] (source) */
> +   enum e_g2d_dir_mode src_x_direction:1;
> +   enum e_g2d_dir_mode src_y_direction:1;
> +
> +   /* SRC_MSK_DIRECT_REG [2:3] */
> +   unsigned intreversed1:2;
> +
> +   /* SRC_MSK_DIRECT_REG [4:5] (mask) */
> +   enum e_g2d_dir_mode mask_x_direction:1;
> +   enum e_g2d_dir_mode mask_y_direction:1;
> +
> +   /* SRC_MSK_DIRECT_REG [6:31] */
> +   unsigned intpadding1:26;
> +
> +   /* DST_PAT_DIRECT_REG [0:1] (destination) */
> +   enum e_g2d_dir_mode dst_x_direction:1;
> +   enum e_g2d_dir_mode dst_y_direction:1;
> +
> +   /* DST_PAT_DIRECT_REG [2:3] */
> +   unsigned intreversed2:2;
> +
> +   /* DST_PAT_DIRECT_REG [4:5] (pattern) */
> +   enum e_g2d_dir_mode pat_x_direction:1;
> +   enum e_g2d_dir_mode pat_y_direction:1;
> +
> +   /* DST_PAT_DIRECT_REG [6:31] */
> +   unsigned intpadding2:26;
> +   } data;
> +};
> +
You don't really want all this in the public header. At the moment
you're using it only internally so might as well not make it part of
the API.

Thanks
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

Re: [PATCH v2 00/13] drm/exynos: async G2D and g2d_move()

[Emil Velikov]

Hi Tobias,

On 22 November 2015 at 18:48, Tobias Jakobi
 wrote:
> Hello,
>
> this series mostly touches G2D code. It introduces the following:
>
> (1) drmHandleEvent2() is added to enable processing of vendor-specific
> events. This will be used to expose asynchronous operation of the
> G2D. The necessary kernel infrastructure is already there since
> a lot of kernel versions. [This touches libdrm core code!]
>
Considering the shortage of input on this can you please respin the
series without it (and related work mentioned below). This way we can
pick merge the remaining work now and discuss (ping) about the rest.

> (2) The necessary infrastructure to handle G2D events. This includes
> adding g2d_config_event() and g2d_exec2() to the public API.
> A test application is provided to ensure that everything works
> as expected.
>
With above in mind the g2d event will need to be split out, although
g2d_exec2() should be ok (although of limited use), imho.

> (3) A small performance test application which can be used to measure
> the speed of solid color clear operations. Interesting for
> benchmarking and plotting colorful graphs (e.g. through
> Mathematica).
>
> (4) g2d_move() which works similar to g2d_copy() but like the C
> memmove() properly handles overlapping buffer copies.
> Again a test application is present to check that this
> indeed does what it should.
>
> (5) Various small changes. A framebuffer colorformat fix for the
> general G2D test application. Moving the currently unused
> g2d_reset() to the public API.
I am more of a "add API when it's needed" kind of person, although I
cannot see anything serious misuse that can arise from g2d_reset().

> Adding a counterpart to
> exynos_bo_map() to unmap buffers again.
>
The exynos bo map compatibility was broken a few times already so I'm
wondering if we really want this one. I guess that with the lack of
any (outside of tizen) user space things cannot go that wrong :-P

Thanks
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

Re: [PATCH v2 08/13] exynos: fimg2d: add g2d_set_direction

[Emil Velikov]

On 26 November 2015 at 16:41, Tobias Jakobi  wrote:
> Hello Emil,
>
> my main system which I also used for development was stolen on Tuesday,
> so I won't be working on this series anytime soon. If anyone wants to
> pick it up, please go ahead.
>
Sad to hear that x2. You've been doing some good work, despite that we
weren't always quick to review/pick it. Hope you had some insurance
and you manage to sort things out, relatively stress free.

Inki, Hyungwon, other Exynos devs,

Will you guys be able to pick any of this work ? Most of it can go in
with the small suggestion in this patch addressed.

Thanks
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

Re: [PATCH 00/25] Exynos DRM: new life of IPP (Image Post Processing) subsystem

[Emil Velikov]

Hello Marek,

On 10 November 2015 at 13:23, Marek Szyprowski  wrote:
> Dear All,
>
> This patch series introduces a new life into Exynos IPP (Image Post
> Processing) subsystem by integrating it (transparently for userspace
> applications) with Exynos DRM core plane management. This means that all
> CRTC drivers transparently get support for standard features of IPP
> subsystem like rotation and scaling.
>
> Support for features not supported natively by CRTC drivers is
> implemented with a help of temporary framebuffers, where image data is
> processed by IPP subsystem before performing the scanout by a CRTC driver.
>
> This patchset is a first version of this 'new feature' and I would like
> get some comments on the proposed approach. I plan to continue working
> on enhancing Exynos DRM drivers and especially do the cleanup the IPP
> subsystem.
>
> Most of the new features are added by the last 2 patches. All other
> patches are bugfixes in various Exynos DRM subdrivers and significant
> core rewrite - introducing a subclass of drm_plane_state was needed and
> all drivers have been converted to use it. Some initial cleanups in IPP
> subsystem were also needed to let Exynos core to call it internally from
> the driver core. This part will be cleaned even more in the future.
>
>
> My solution has been tested on Exynos4412-based Odroid U3 and
> Exynos5420-based Odroid XU3. To check rotation, cropping and scaling
> I've developed a simple test application, which use atomic mode
> setting/page flipping API. You can download it here:
>
> https://git.linaro.org/?p=people/marek.szyprowski/atomictest.git
>
Can I interest you in up-streaming this code into place such as libdrm
? This way other developers can benefit from your tool.

Admittedly I've not looked at the tool but it seems to me that it
duplicates some of the functionality we already in modetest (+atomic).
The latter of which send by your colleague Hyungwon Hwang [1] (and I
seemingly forgot do a final review/push). If you can share a few
cycles worth of review and/or importing your app.

Thanks
Emil

[1] http://lists.freedesktop.org/archives/dri-devel/2015-September/090748.html
--
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 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

Re: [PATCH 06/13] tests/exynos: add fimg2d event test

[Emil Velikov]

On 30 October 2015 at 11:16, Tobias Jakobi
 wrote:
> Hello Hyungwon,
>
> first of all thanks for reviewing the series!
>
>
>
> Hyungwon Hwang wrote:
>> On Tue, 22 Sep 2015 17:54:55 +0200
>> Tobias Jakobi  wrote:
>>

>>> +evhandler->evctx.base.version = DRM_EVENT_CONTEXT_VERSION;
>>> +evhandler->evctx.version = EXYNOS_EVENT_CONTEXT_VERSION;
>>
>> The versions must be set not using XXX_EVENT_CONTEXT_VERSION. After the
>> versions are bumped, the event will contains wrong version info.
> Hmm, I don't see how this is true. Both DRM_EVENT_CONTEXT_VERSION and
> EXYNOS_EVENT_CONTEXT_VERSION come from the public libdrm header. If the
> version in the public header is bumped, then it's also bumped here. So I
> don't see the issue.
>
The issue is that the public header defines the interface available,
while you set the version that you implement. Currently those match,
but if/when we expand the API (bumping the version in the header) and
rebuild your program we will crash.

Before you ask - yes current libdrm users are not doing the right
thing and should be updated one of these days.

-Emil
P.S. Having some deja-vu here... I thought I mentioned this a while back.
--
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 06/13] tests/exynos: add fimg2d event test

[Emil Velikov]

On 30 October 2015 at 11:28, Tobias Jakobi
<tjak...@math.uni-bielefeld.de> wrote:
> Hello Emil,
>
>
> Emil Velikov wrote:
>> On 30 October 2015 at 11:16, Tobias Jakobi
>> <tjak...@math.uni-bielefeld.de> wrote:
>>> Hello Hyungwon,
>>>
>>> first of all thanks for reviewing the series!
>>>
>>>
>>>
>>> Hyungwon Hwang wrote:
>>>> On Tue, 22 Sep 2015 17:54:55 +0200
>>>> Tobias Jakobi <tjak...@math.uni-bielefeld.de> wrote:
>>>>
>>
>>>>> +evhandler->evctx.base.version = DRM_EVENT_CONTEXT_VERSION;
>>>>> +evhandler->evctx.version = EXYNOS_EVENT_CONTEXT_VERSION;
>>>>
>>>> The versions must be set not using XXX_EVENT_CONTEXT_VERSION. After the
>>>> versions are bumped, the event will contains wrong version info.
>>> Hmm, I don't see how this is true. Both DRM_EVENT_CONTEXT_VERSION and
>>> EXYNOS_EVENT_CONTEXT_VERSION come from the public libdrm header. If the
>>> version in the public header is bumped, then it's also bumped here. So I
>>> don't see the issue.
>>>
>> The issue is that the public header defines the interface available,
>> while you set the version that you implement. Currently those match,
>> but if/when we expand the API (bumping the version in the header) and
>> rebuild your program we will crash.
> Hmm, I'm still not sure I understand this. Do you mean rebuilding the
> tests out-of-tree and then running them against a newer/older libdrm
> version?
>
> Because from my understanding the tests are always build together with
> libdrm anyway. Or am I misunderstanding here something?
>
We're not talking about building out of tree or anything like that
here. The following example should illustrate what I have in mind.
Greatly appreciated if you can explain it in your own words.


Currently

* xf86drm.h
#define DRM_EVENT_CONTEXT_VERSION 2
struct drmEventContext {
   int version;
   ...
   void (*page_flip_handler)(...);
}

* user
struct foo {
   .version = ...VERSION // 2
   ...
}


API bump

* xf86drm.h
#define DRM_EVENT_CONTEXT_VERSION 3
struct drmEventContext {
   int version;
   ...
   void (*page_flip_handler)(...);
   void (*page_flip_handler2)(...);
}

* user (hasn't been updated, only rebuilt)
struct foo {
   .version = ...VERSION // 3
   ...
   .page_flip_handler2 = 0 // ... or garbage.
}


* xf86drmMode.c
int drmHandleEvent(...)
{
...
if (foo.version >= 3)
   foo.page_flip_handler2(); // boom !
else
   foo.page_flip_handler();
...
}


Also worth mentioning is that the issue is rather wide spread rather
than limited to libdrm :'(

>
>> Before you ask - yes current libdrm users are not doing the right
>> thing and should be updated one of these days.
> Maybe a dumb question, but what would be the right thing to do in may case.
>
> Define my own MY_XZY_EVENT_CONTEXT_VERSION and use these?
>
No need for extra defines imho. Just set the versions to 2 and 1 for
evctx.base.version and evctx.version respectively.


Glad to see some of the Samsung/Exynos people looking this way :-)

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

Re: [PATCH 06/13] tests/exynos: add fimg2d event test

[Emil Velikov]

On 30 October 2015 at 14:28, Tobias Jakobi
 wrote:

> OK, I see what you mean. However this shouldn't happen as long as the
> user properly zero initializes the event context structures, right?
>
It pains me to say it but in this day and age, not everyone zero
initializes their structs (be that via C99 initailizers, memset or
just {}).

> OK, going to do this then. In any case, can the user assume that the
> event structures only "grow", in the sense that new fields are added to it?
>
That is correct. Note that this approach is also used elsewhere - the
DRI driver <> loader model, also the drm ioctls structs are extended
in a similar way (minus the actual version field).

With the current knowledge in hand, perhaps we could have done things
differently... But that's another story :-)

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

Re: [PATCH] drm/exynos: dp: remove suspend/resume functions

[Emil Velikov]

Hi Inki,

On 30 September 2015 at 12:21, Inki Dae  wrote:
> This patch removes unnecessary pm suspend/resume functions.
>
> All kms sub drivers will be controlled by top of Exynos drm driver
> and connector dpms so these sub drivers shouldn't have their own
> pm interfaces.
>
Not sure if you've noticed but this patch seems to do the opposite of
what Gustavo was aiming with an earlier series [1].

Cheers,
Emil

[1] http://lists.freedesktop.org/archives/dri-devel/2015-September/089800.html
--
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] drm/exynos: dp: remove suspend/resume functions

[Emil Velikov]

On 30 September 2015 at 14:46, Inki Dae <inki@samsung.com> wrote:
> Hi Emil,
>
> On 2015년 09월 30일 21:19, Emil Velikov wrote:
>> Hi Inki,
>>
>> On 30 September 2015 at 12:21, Inki Dae <inki@samsung.com> wrote:
>>> This patch removes unnecessary pm suspend/resume functions.
>>>
>>> All kms sub drivers will be controlled by top of Exynos drm driver
>>> and connector dpms so these sub drivers shouldn't have their own
>>> pm interfaces.
>>>
>> Not sure if you've noticed but this patch seems to do the opposite of
>> what Gustavo was aiming with an earlier series [1].
>
> I removed just the interfaces related to sleep pm not runtime pm. From
> long ago, Linux DRM drivers - especially ARM DRM drivers - have been
> stepping forward to a integrated DRM driver so it'd be reasonable for
> sleep pm operations of all KMS drivers are controlled by top of DRM
> driver. It means that Exynos drm driver has already sleep pm interfaces.
>
> However, I think a power domain of each kms device couldn't be
> controlled by the top in case of ARM SoC because kms devices can use a
> different power domain each other according to Vendor SoC so runtime pm
> should be controlled by each kms driver, which will be triggered by DRM top.
>
> And Gustavo's patch set you mentioned - now being reviewed - will add
> only runtime pm interfaces to each kms driver.
>
Ack. Seems like I misread your patch. Thank you for the comprehensive
answer and apologies for the noise.

Thanks
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

Re: [PATCH v2 0/9] drm/exynos: rewrite fimg2d error handling

[Emil Velikov]

Hi Tobias,

On 8 September 2015 at 16:22, Tobias Jakobi
 wrote:
> Hello,
>
> during the discussion about the last patchset touching the
> fimg2d code, it became apparent that the error handling for
> the command submission is currently unsatisfactory.
>
> This series rewrites the handling. All functions that submit
> command buffers now first check if enough space is available
> and only then proceed to build the command buffers.
>
> In particular the command buffer is no longer left in a
> half-finished state, since parameters passed to the functions
> are now validated before command submission. For this some
> validation functions are introduced.
>
> This should also increase performance if the bottleneck is
> the submission part, since adding commands to the buffer
> is now more lightweight.
>
> Last but not least some prefix was added to messages printed
> by fprintf and printf, and the G2D context struct was moved
> out of the public header.
>
>
> Please review and let me know if I can improve anything!
>
Considering that there were no more objections with the series I've
scooped them up.

Thanks for clearing this and sticking around !
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

Re: [PATCH v4 03/16] drm: bridge: analogix/dp: split exynos dp driver to bridge dir

[Emil Velikov]

[Dropping the CC list]

Hi Yakir Yang,

On 1 September 2015 at 06:49, Yakir Yang  wrote:
> Split the dp core driver from exynos directory to bridge
> directory, and rename the core driver to analogix_dp_*,
> leave the platform code to analogix_dp-exynos.
>
> Signed-off-by: Yakir Yang 
> ---
> Changes in v4:
> - Take Rob suggest, update "analogix,hpd-gpios" to "hpd-gpios" DT propery.
> - Take Jingoo suggest, rename "analogix_dp-exynos.c" file name to 
> "exynos_dp.c"
> - Take Archit suggest, create a separate folder for analogix code in bridge/
>
"Take X suggest", is grammatically incorrect. You should use
"suggestion(s)" or alternatively use the following approach.

- Create a separate folder for analogix code in bridge/ (Archit)

Cheers,
Emil

P.S. Why do you resend the whole series (some 10+ patches) when only a
few patches have been changed ? Are all the patches changed whist
missing that information (vX: rebase on top of A)
--
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 07/14] exynos/fimg2d: add g2d_validate_xyz() functions

[Emil Velikov]

On 31 August 2015 at 14:18, Inki Dae  wrote:
> On 2015년 08월 24일 23:14, Tobias Jakobi wrote:
>> The G2D headers define a number of modes through enums
>> (like e.g. color, select, repeat, etc.).
>>
>> This introduces g2d_validate_select_mode() and
>> g2d_validate_blending_op() which validate a
>> select mode or blending operation respectively.
>>
>> Signed-off-by: Tobias Jakobi 
>> ---
>>  exynos/exynos_fimg2d.c | 44 
>>  1 file changed, 44 insertions(+)
>>
>> diff --git a/exynos/exynos_fimg2d.c b/exynos/exynos_fimg2d.c
>> index 2e04f4a..781aff5 100644
>> --- a/exynos/exynos_fimg2d.c
>> +++ b/exynos/exynos_fimg2d.c
>> @@ -119,6 +119,50 @@ static unsigned int g2d_check_space(const struct 
>> g2d_context *ctx,
>>  }
>>
>>  /*
>> + * g2d_validate_select_mode - validate select mode.
>> + *
>> + * @mode: the mode to validate
>> + */
>> +static unsigned int g2d_validate_select_mode(
>> + enum e_g2d_select_mode mode)
>> +{
>> + switch (mode) {
>> + case G2D_SELECT_MODE_NORMAL:
>> + case G2D_SELECT_MODE_FGCOLOR:
>> + case G2D_SELECT_MODE_BGCOLOR:
>> + return 0;
>> + }
>
> It's strange use a bit. Just check the range like below,
>
> First, please add G2D_SELECT_MODE_MAX which has 3 << 0, and
>
> if (G2D_SELECT_MODE_MAX < mode) {
> fprintf(strerr, "invalid command(0x%x)\n", mode);
> return -EINVAL;
> }
>
Listing every value might seem like an overkill, indeed. Yet I think
it's the more robust approach.

By adding _MAX to the API we effectively lock it down. That can be
avoided, plus the compiler will warn us when new values are added to
the enum. If someone starts getting smart (due to the _MAX) and adds
G2D_SELECT_MODE_FOO = -1, the above check will fail :(

> And I think it'd be better to change return type of this function to int,
>
Good idea.

Cheers,
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

Re: [PATCH 04/14] exynos/fimg2d: check buffer space in g2d_solid_fill()

[Emil Velikov]

On 31 August 2015 at 20:27, Tobias Jakobi  wrote:
> Hello!
>
> Inki Dae wrote:
>> On 2015년 08월 24일 23:13, Tobias Jakobi wrote:

>>> +if (g2d_check_space(ctx, 7, 1))
>>> +return -ENOSPC;
>>
>> You can make 3 and 4 patches to one. These should be same patch.
> Hmm, so which 3 (respectively 4) patches should be squashed?
>
I believe he meant "squash the introduction of the function and its
uses into a single patch".

Not sure how much value that brings, so I'll let you guys decide on
the bike shed colour :-)

-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

Re: [PATCH 00/14] drm/exynos: rewrite fimg2d error handling

[Emil Velikov]

Hi Tobias,

On 24 August 2015 at 15:13, Tobias Jakobi tjak...@math.uni-bielefeld.de wrote:
 Hello,

 during the discussion about the last patchset touching the
 fimg2d code, it became apparent that the error handling for
 the command submission is currently unsatisfactory.

 This series rewrites the handling. All functions that submit
 command buffers now first check if enough space is available
 and only then proceed to build the command buffers.

 In particular the command buffer is no longer left in a
 half-finished state, since parameters passed to the functions
 are now validated before command submission. For this some
 validation functions are introduced.

 This should also increase performance if the bottleneck is
 the submission part, since adding commands to the buffer
 is now more lightweight.

 Last but not least some prefix was added to messages printed
 by fprintf and printf, and the G2D context struct was moved
 out of the public header.


Thanks for going with my earlier suggestion and untangling all this.

I've went through the lot and it looks great afaict. Fwiw for the series
Reviewed-by: Emil Velikov emil.l.veli...@gmail.com

As pretty much none of this is hardware specific and/or requires
additional knowledge of the kernel module I'm inclined to pull this in
even if we don't get too many reviewers. We better keep it around for
a couple of weeks in case others are swamped with unrelated work atm,
yet willing to take a look.

Cheers,
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

Re: [PATCH v2 0/9] drm/exynos: cleanups and small fixes for libdrm

[Emil Velikov]

On 12 June 2015 at 18:15, Tobias Jakobi tjak...@math.uni-bielefeld.de wrote:
 Hello,

 I've split off the Exynos specific bits, so this is just some cleanups and 
 small fixes. Everything can be reviewed without knowledge about the Exynos 
 platform. My hope is that I can get at least some of the patches from my last 
 series upstream.

 With best wishes,
 Tobias

 Changes in v2:
 - Made it clear that the error handling is currently 'unsatisfactory', but 
 that this is going to be adressed in a later patch.
 - Point out that removed code also wasn't used anywhere in the past (by 
 inspection of git history).

Big thanks for the update. It will make things clear a week/month down
the road - when we've forgotten pretty much everything in the area :-)
Considering how trivial these are and the silence from the
Samsung/Exynos folk, I'll be pushing these later on today.

If some of the dead code ends up required, we can always bring it back.

Cheers,
Emil
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in

Re: [PATCH 4/9] tests/exynos: clean struct connector

[Emil Velikov]

On 10 June 2015 at 14:42, Tobias Jakobi tjak...@math.uni-bielefeld.de wrote:
 Remove all unused struct members.

Mentioning if they were used at some point in the past will be great.

Thanks
Emil

 Signed-off-by: Tobias Jakobi tjak...@math.uni-bielefeld.de
 ---
  tests/exynos/exynos_fimg2d_test.c | 10 --
  1 file changed, 10 deletions(-)

 diff --git a/tests/exynos/exynos_fimg2d_test.c 
 b/tests/exynos/exynos_fimg2d_test.c
 index 64dacfa..6af9277 100644
 --- a/tests/exynos/exynos_fimg2d_test.c
 +++ b/tests/exynos/exynos_fimg2d_test.c
 @@ -65,17 +65,9 @@ struct fimg2d_test_case {
  struct connector {
 uint32_t id;
 char mode_str[64];
 -   char format_str[5];
 -   unsigned int fourcc;
 drmModeModeInfo *mode;
 drmModeEncoder *encoder;
 int crtc;
 -   int pipe;
 -   int plane_zpos;
 -   unsigned int fb_id[2], current_fb_id;
 -   struct timeval start;
 -
 -   int swap_count;
  };

  static void connector_find_mode(int fd, struct connector *c,
 @@ -750,8 +742,6 @@ int main(int argc, char **argv)
 if (ret  0)
 goto err_destroy_buffer;

 -   con.plane_zpos = -1;
 -
 memset(bo-vaddr, 0xff, screen_width * screen_height * 4);

 ret = drm_set_crtc(dev, con, fb_id);
 --
 2.0.5

--
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 8/9] tests/exynos: remove connector_find_plane

[Emil Velikov]

On 10 June 2015 at 14:42, Tobias Jakobi tjak...@math.uni-bielefeld.de wrote:
 No test uses DRM planes at the moment so this function
 is never called.

Similar to the other patch - mention if this been used previously. I'm
assuming that the exynos guys might have some work stashed which
depends on this ?

-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

Re: [PATCH 1/9] exynos: fimg2d: fix return codes

[Emil Velikov]

On 10 June 2015 at 14:42, Tobias Jakobi tjak...@math.uni-bielefeld.de wrote:
 Even if flushing the command buffer doesn't succeed, the
 G2D calls would still return zero. Fix this by just passing
 the flush return code.

 Signed-off-by: Tobias Jakobi tjak...@math.uni-bielefeld.de
 ---
  exynos/exynos_fimg2d.c | 20 +---
  1 file changed, 5 insertions(+), 15 deletions(-)

 diff --git a/exynos/exynos_fimg2d.c b/exynos/exynos_fimg2d.c
 index 86ae898..5ea42e6 100644
 --- a/exynos/exynos_fimg2d.c
 +++ b/exynos/exynos_fimg2d.c
 @@ -330,9 +330,7 @@ g2d_solid_fill(struct g2d_context *ctx, struct g2d_image 
 *img,
 bitblt.data.fast_solid_color_fill_en = 1;
 g2d_add_cmd(ctx, BITBLT_COMMAND_REG, bitblt.val);

 -   g2d_flush(ctx);
 -
 -   return 0;
 +   return g2d_flush(ctx);
  }

  /**
 @@ -415,9 +413,7 @@ g2d_copy(struct g2d_context *ctx, struct g2d_image *src,
 rop4.data.unmasked_rop3 = G2D_ROP3_SRC;
 g2d_add_cmd(ctx, ROP4_REG, rop4.val);

 -   g2d_flush(ctx);
 -
 -   return 0;
 +   return g2d_flush(ctx);
  }

  /**
 @@ -527,9 +523,7 @@ g2d_copy_with_scale(struct g2d_context *ctx, struct 
 g2d_image *src,
 pt.data.y = dst_y + dst_h;
 g2d_add_cmd(ctx, DST_RIGHT_BOTTOM_REG, pt.val);

 -   g2d_flush(ctx);
 -
 -   return 0;
 +   return g2d_flush(ctx);
  }

  /**
 @@ -636,9 +630,7 @@ g2d_blend(struct g2d_context *ctx, struct g2d_image *src,
 pt.data.y = dst_y + h;
 g2d_add_cmd(ctx, DST_RIGHT_BOTTOM_REG, pt.val);

 -   g2d_flush(ctx);
 -
 -   return 0;
 +   return g2d_flush(ctx);
  }

  /**
 @@ -766,7 +758,5 @@ g2d_scale_and_blend(struct g2d_context *ctx, struct 
 g2d_image *src,
 pt.data.y = dst_y + dst_h;
 g2d_add_cmd(ctx, DST_RIGHT_BOTTOM_REG, pt.val);

Strictly speaking g2d_add_cmd() can fail, and all the functions that
build upon it. In the latter case most ignore the return value which
is a bit bad. That plus the fact that these are part of the public API
makes things easier to misuse.

One way to avoid all this is to implement an internal function that
does the size checks ahead of time, and drop them from g2d_add_cmd(),
apart from this patch.

The nouveau mesa drivers do a similar thing - see PUSH_SPACE().

-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

Re: [PATCH] libgencec: Add userspace library for the generic CEC kernel interface

[Emil Velikov]

Hi Kamil,

It seems that you've only incorporated the libgencec.pc suggestion.
Did you change your mind about the others, found out something funny
with them (if so can you let me know what) or simply forgot about them
?

On 30 April 2015 at 11:25, Kamil Debski k.deb...@samsung.com wrote:
 Hi Emil,

 From: linux-media-ow...@vger.kernel.org [mailto:linux-media-
 ow...@vger.kernel.org] On Behalf Of Emil Velikov
 Sent: Wednesday, April 29, 2015 5:00 PM

 Hi Kamil,

 Allow me to put a few suggestions:

 On 29 April 2015 at 11:02, Kamil Debski k.deb...@samsung.com wrote:

  diff --git a/m4/.gitkeep b/m4/.gitkeep new file mode 100644 index
  000..e69de29
 Haven't seen any projects doing this. Curious what the benefits of
 keeping and empty folder might be ?

 When I run autoreconf -i it complained about missing m4 folder, hence I added
 this filler file such that the folder is created. Any suggestion on how to do
 this better?

Ahh yes - that lovely message. It turns out that older versions of
automake will even error out [1], rather than just printing out a
warning. A handy workaround would be to add a .gitignore (and a second
one in the top folder) list. Plus it will slim down the untracked
files list and let you clearly see when git add was missed :-)

Cheers
Emil

[1] https://bugzilla.redhat.com/show_bug.cgi?id=901333
--
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 2/5] exynos: add EXYNOS_G2D_EVENT to drmHandleEvent

[Emil Velikov]

Hi Tobias,

On 30/03/15 13:04, Tobias Jakobi wrote:
 Hello,
 
 On 2015-03-30 02:02, Rob Clark wrote:
 so, iirc, vmwgfx also has some custom events..  not really sure if
 they have their own hand-rolled drmHandleEvent() or if they have
 another way of catching those.

 Maybe we need some more flexible way to register handlers for driver
 custom events?  But everyone adding their own thing to
 drmHandleEvent() doesn't really seem so nice..  that said, I'm not
 sure how much to care.  If it is just exynos and vmwgfx, then telling
 them to use there own version of of drmHandleEvent() might be ok.  But
 if driver custom events somehow become more popular, maybe we want a
 better solution..
 
 would something like this work for you guys:
 https://www.math.uni-bielefeld.de/~tjakobi/archive/0001-custom-events.patch
 
 (this is not compile tested or anything, just a draft)
 
 Basically this introduces drmHandleEvent2, which is drmHandleEvent with
 two additional arguments. The first one being a function pointer through
 which the 'remaining' events (which are not handled by the common code)
 are handled, and some (opaque) pointer to data that the handler might need.
 
 In the vendor specific code I then introcuded exynos_handle_event which
 calls dramHandleEvent2 with a properly setup handler. vmwgfx could do
 the same here I guess.
 
I'm assuming that one of the concerns here is about adding API for a
single (and not so common) user to the core library.

From a quick look at the mesa and xf86-video-vmware I cannot see the
vmwgfx driver using events. It has some definitions in vmwgfx_drm.h but
that's about it.

That aside - the drmHandleEvent2 approach looks like a massive
improvement over the original patch. Personally I do not see any
problems with it and think that it's a good way forward.

Perhaps you can come over to #dri-devel and ping the devs to get some
more feedback. As the topic is not a priority for most of them your
suggestions has mostly gone unnoticed.

Cheers,
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

Re: [PATCH 3/3] drm/exynos: mixer: add 2x scaling to mixer_graph_buffer

[Emil Velikov]

Hi Tobias,

On 1 April 2015 at 14:29, Tobias Jakobi tjak...@math.uni-bielefeld.de wrote:
 diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c 
 b/drivers/gpu/drm/exynos/exynos_mixer.c
 index 5ab0e32..6822b36 100644
 --- a/drivers/gpu/drm/exynos/exynos_mixer.c
 +++ b/drivers/gpu/drm/exynos/exynos_mixer.c

 @@ -528,9 +552,8 @@ static void mixer_graph_buffer(struct mixer_context *ctx, 
 int win)
 fmt = ARGB;
 }

 -   /* 2x scaling feature */
 -   x_ratio = 0;
 -   y_ratio = 0;
 +   /* check if mixer supports requested scaling setup */
 +   if (mixer_setup_scale(plane, x_ratio, y_ratio)) return;

Having return on the same line as the if statement will make
checkpatch.pl unhappy :-(

Cheers
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

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

[Emil Velikov]

On 25/03/15 18:27, Tobias Jakobi wrote:
 Hello,
 
 the new version should fix most of the mentioned issues.
 
 Tobias Jakobi wrote:
 As a general note I would recommend keeping statements on separate lines
 (none of if (foo) far()) as it makes debugging easier.
 OK, changing this.
 Except for this, I didn't change it since I don't see the point. In fact
 I think that splitting the few occurrences makes the code less readable.
 
Beauty is in the eye of the beholder.

 I haven't worked with getopt before, so I gave it a try and made all
 these hardcoded parameters configurable.
 
Nice :-)

Cheers,
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

Re: [PATCH v2 1/5] tests/exynos: add fimg2d performance analysis

[Emil Velikov]

On 25/03/15 16:48, Tobias Jakobi wrote:
 Currently only fast solid color clear performance is measured.
 A large buffer is allocated and solid color clear operations
 are executed on it with randomly chosen properties (position
 and size of the region, clear color). Execution time is
 measured and output together with the amount of pixels
 processed.
 
 The 'simple' variant only executes one G2D command buffer at
 a time, while the 'multi' variant executes multiple ones. This
 can be used to measure setup/exec overhead.
 
 The test also serves a stability check. If clocks/voltages are
 too high or low respectively, the test quickly reveals this.
 
 v2: Add GPLv2 header, argument handling and documentation.
 Tool is only installed when requested.
 
 Signed-off-by: Tobias Jakobi tjak...@math.uni-bielefeld.de
 ---
  tests/exynos/Makefile.am  |  19 ++-
  tests/exynos/exynos_fimg2d_perf.c | 320 
 ++
  2 files changed, 337 insertions(+), 2 deletions(-)
  create mode 100644 tests/exynos/exynos_fimg2d_perf.c
 
 diff --git a/tests/exynos/Makefile.am b/tests/exynos/Makefile.am
 index b21d016..e82d199 100644
 --- a/tests/exynos/Makefile.am
 +++ b/tests/exynos/Makefile.am
 @@ -5,16 +5,31 @@ AM_CFLAGS = \
   -I $(top_srcdir)/exynos \
   -I $(top_srcdir)
  
 +bin_PROGRAMS =
 +noinst_PROGRAMS =
 +
  if HAVE_LIBKMS
  if HAVE_INSTALL_TESTS
 -bin_PROGRAMS = \
 +bin_PROGRAMS += \
   exynos_fimg2d_test
  else
 -noinst_PROGRAMS = \
 +noinst_PROGRAMS += \
   exynos_fimg2d_test
  endif
  endif
  
 +if HAVE_INSTALL_TESTS
 +bin_PROGRAMS += \
 + exynos_fimg2d_perf
 +else
 +noinst_PROGRAMS += \
 + exynos_fimg2d_perf
 +endif
 +
One can simplify this as below, although I will not worry too much about
it :) That aside would be nice to hear some feedback from the Exynos and
the old school libdrm devs on the core changes.


if HAVE_LIBKMS
FOO = \
exynos_fimg2d_test
endif

if HAVE_INSTALL_TESTS
bin_PROGRAMS = \
$(FOO) \
exynos_fimg2d_perf
else
noinst_PROGRAMS = \
$(FOO) \
exynos_fimg2d_perf
endif


-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

Re: [PATCH 2/5] exynos: add EXYNOS_G2D_EVENT to drmHandleEvent

[Emil Velikov]

Hi Tobias,
On 22/03/15 16:29, Tobias Jakobi wrote:
 Hello Emil,
 Emil Velikov wrote:
 I fear we are not (yet) allowed to do either of these changes.

 The exynos/exynos_drm.h header is (supposed to) be in sync/come from the
 kernel. And changes here are to be reflected only when the corresponding
 patch lands in drm-next (as per RELEASING).
 the point here is, that the current header in libdrm in _not_ in sync
 with the one in the kernel. It's hopelessly outdated, but mainly because
 exynos/libdrm doesn't use any new functionality provided by some update.
 
 Here's the current kernel header:
 https://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/tree/include/uapi/drm/exynos_drm.h
 
 The event stuff has been there since 2012:
 https://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/commit/include/drm/exynos_drm.h?id=d7f1642c90ab5eb2d7c48af0581c993094f97e1a
 
 The only reason why I haven't added the IPP things, is because I don't
 intend to work on this for the moment.
 
Hmmm good point. Seems like the changes went in before I started
following exynos development. If it's in an upstream kernel, then it's
save to land in libdrm. Objection withdrawn.

I have an idea how we can get things back into shape (wrt headers being
out if sync) - I will propose/post a solution shortly.

 Wrt extending the current drmEventContext, I'm not sure that adding
 device specific changes to it are allowed.
 Why shouldn't it? Isn't drmHandleEvent supposed to handle all kinds of
 DRM events that the kernel produces?
 
Bth I'm not familiar with the code in question, although a quick grep
indicates that libdrm does not contain any driver specific information.
That is aside from the include/drm headers, although they are not part
of the library or its interface.

Maybe some of the more experienced devs can share some light ?

Thanks
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

Re: [PATCH 2/5] exynos: add EXYNOS_G2D_EVENT to drmHandleEvent

[Emil Velikov]

On 20/03/15 22:25, Tobias Jakobi wrote:
 This event is specific to Exynos G2D DRM driver. Only
 process it when Exynos support is enabled.
 
 Signed-off-by: Tobias Jakobi tjak...@math.uni-bielefeld.de
 ---
  exynos/exynos_drm.h | 12 
  xf86drm.h   |  7 ++-
  xf86drmMode.c   | 18 ++
  3 files changed, 36 insertions(+), 1 deletion(-)
 
I fear we are not (yet) allowed to do either of these changes.

The exynos/exynos_drm.h header is (supposed to) be in sync/come from the
kernel. And changes here are to be reflected only when the corresponding
patch lands in drm-next (as per RELEASING).

Wrt extending the current drmEventContext, I'm not sure that adding
device specific changes to it are allowed.

Wish I would give you some better news but... I cannot sorry :-\

-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

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

[Emil Velikov]

On 20/03/15 22:25, Tobias Jakobi wrote:
 Currently only fast solid color clear performance is measured.
 A large buffer is allocated and solid color clear operations
 are executed on it with randomly chosen properties (position
 and size of the region, clear color). Execution time is
 measured and output together with the amount of pixels
 processed.
 
 The 'simple' variant only executes one G2D command buffer at
 a time, while the 'multi' variant executes multiple ones. This
 can be used to measure setup/exec overhead.
 
 The test also serves a stability check. If clocks/voltages are
 too high or low respectively, the test quickly reveals this.
 
 Signed-off-by: Tobias Jakobi tjak...@math.uni-bielefeld.de
Hi Tobias,

As most of this series is quite Exynos specific I will cover the misc
bits, and leave the core part to someone familiar with the hardware.

 ---
  tests/exynos/Makefile.am  |   8 +-
  tests/exynos/exynos_fimg2d_perf.c | 245 
 ++
  2 files changed, 252 insertions(+), 1 deletion(-)
  create mode 100644 tests/exynos/exynos_fimg2d_perf.c
 
 diff --git a/tests/exynos/Makefile.am b/tests/exynos/Makefile.am
 index b21d016..243f957 100644
 --- a/tests/exynos/Makefile.am
 +++ b/tests/exynos/Makefile.am
 @@ -5,9 +5,11 @@ AM_CFLAGS = \
   -I $(top_srcdir)/exynos \
   -I $(top_srcdir)
  
 +bin_PROGRAMS = exynos_fimg2d_perf
 +
Might I suggest that we treat this (and your follow up utility) as a
test ? I.e. use

if HAVE_INSTALL_TESTS
bin_PROGRAMS = \
exynos_fimg2d_perf
else
noinst_PROGRAMS = \
exynos_fimg2d_perf
endif

and amend the block below appropriately ?

  if HAVE_LIBKMS
  if HAVE_INSTALL_TESTS
 -bin_PROGRAMS = \
 +bin_PROGRAMS += \
   exynos_fimg2d_test
  else
  noinst_PROGRAMS = \
 @@ -15,6 +17,10 @@ noinst_PROGRAMS = \
  endif
  endif
  
 +exynos_fimg2d_perf_LDADD = \
 + $(top_builddir)/libdrm.la \
 + $(top_builddir)/exynos/libdrm_exynos.la
 +
  exynos_fimg2d_test_LDADD = \
   $(top_builddir)/libdrm.la \
   $(top_builddir)/libkms/libkms.la \
 diff --git a/tests/exynos/exynos_fimg2d_perf.c 
 b/tests/exynos/exynos_fimg2d_perf.c
 new file mode 100644
 index 000..f45cacc
 --- /dev/null
 +++ b/tests/exynos/exynos_fimg2d_perf.c
 @@ -0,0 +1,245 @@
Can you add a licence to this file. Would be nice if it's covered by
X/MIT so that *BSD folk and others can use your tool.

 +#include stdlib.h
 +#include stdio.h
 +#include time.h
 +
 +#include xf86drm.h
 +
 +#include exynos_drm.h
 +#include exynos_drmif.h
 +#include exynos_fimg2d.h
 +
 +/* Enable to format the output so that it can be fed into Mathematica. */
 +#define OUTPUT_MATHEMATICA 0
 +
 +static int fimg2d_perf_simple(struct exynos_bo *bo, struct g2d_context *ctx,
 + unsigned buf_width, unsigned buf_height, unsigned 
 iterations)
 +{
 + struct timespec tspec = { 0 };
 + struct g2d_image img = { 0 };
 +
 + unsigned long long g2d_time;
 + unsigned i;
 + int ret = 0;
 +
 + img.width = buf_width;
 + img.height = buf_height;
 + img.stride = buf_width * 4;
 + img.color_mode = G2D_COLOR_FMT_ARGB | G2D_ORDER_AXRGB;
 + img.buf_type = G2D_IMGBUF_GEM;
 + img.bo[0] = bo-handle;
 +
 + srand(time(NULL));
 +
 + printf(starting simple G2D performance test\n);
 + printf(buffer width = %u, buffer height = %u, iterations = %u\n,
 + buf_width, buf_height, iterations);
 +
 +#if (OUTPUT_MATHEMATICA == 1)
 + putchar('{');
 +#endif
 +
I'm suspecting that having this as a runtime option will be better.
Something like ./exynos_fimg2d_perf --output mathematica ?

As a general note I would recommend keeping statements on separate lines
(none of if (foo) far()) as it makes debugging easier.

Although all of the above are my take on things, so checks with the
Exynos crew if they feel otherwise :-)

Cheers,
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

Re: [PATCH 1/7] tests/exynos: fimg2d: add a checkerboard test

[Emil Velikov]

On 11/03/15 19:38, Tobias Jakobi wrote:
 This makes it easier to spot memory corruptions which don't become
 visible when using a plain buffer filled with a solid color (so
 corruptions that are just a permutation of the bytes in the buffer).
 
 Signed-off-by: Tobias Jakobi tjak...@math.uni-bielefeld.de
 Reviewed-by: Inki Dae inki@samsung.com
 Tested-by: Joonyoung Shim jy0922.s...@samsung.com
Thank you Tobias.
The series is in master now.

-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

Re: [v3] libdrm: improvements to userspace exynos component

[Emil Velikov]

On 11 March 2015 at 13:31, Inki Dae inki@samsung.com wrote:
 Hi,

 On 2015년 02월 24일 23:20, Tobias Jakobi wrote:
 Hello,

 here are some miscellaneous improvements (small features, bugfixes, spelling 
 fixes, etc.) for the exynos component of libdrm. The general idea is to let 
 userspace use the G2D engine functionality more
 efficiently.

 If someone is interested in an application that actually makes use of this, 
 the RetroArch frontend has a custom video backend:
 https://github.com/libretro/RetroArch/blob/master/gfx/drivers/exynos_gfx.c


 Please review and let me know what I can improve.

 v2:
 - Mention value of G2D scaling normalization factor (02/15).
 - Moved patch (04/15) description from commit message to source itself, like 
 suggested by Joonyoung Shim.

 v3:
 I integrated the suggestions by Emil Velikov and added two additional 
 patches to the series. One doing the header cleanup that Emil point out, 
 another one just a trivial whitespace thing. I'm resending
 the whole series, since number of patches and order changed.

Wohoo Inki's back !

 Looks good to me except for 14/17 patch. For the patch, I commented
 already, which may be very trivial but should be fixed correctly. I
 think Emil could fix it before merging it.

 To Emil,
 Could you please fix the patch 14/17 like below?,

 before
 -   /* Global Alpha : Set by ALPHA_REG(0x618) */
 +   /* Global Color : Set by ALPHA_REG(0x618) */

 after
 -   /* Global Alpha : Set by ALPHA_REG(0x618) */
 +   /* Global Color and Alpha: Set by ALPHA_REG(0x618) */


If you don't mind I'll leave this to Tobias as a follow up patch, as
the one you've looked at is already in master.

 for all patches - 1 though 17, Signed-off-by : Inki Dae
 inki@samsung.com

I believe you mean Reviewed-by, isn't that right ?

Tobias,
Can you please re-spin the remaining patches on top of master, and
address Inki's comment.
Also please add Inki's and Joonyoung Shim's tags.

Thanks
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

Re: [v3] libdrm: improvements to userspace exynos component

[Emil Velikov]

On 08/03/15 23:12, Tobias Jakobi wrote:
 Hello Emil,
 Emil Velikov wrote:
 I'm planning to push the above mentioned patches around lunchtime this
 Tuesday. If anyone has objections please speak up.
 I doubt you're going to hear anything from Inki Dae.
Despite that it might sound a bit rude, I sincerely hope that you're
wrong on this one. I'm would assume that the lack of reply from Inki was
due to being him busy, rather than ignoring you.

 Anyway, thanks for considering to merge this, even though it lacks the
 review of the exynos-drm maintainer.
 
 At least this gives me confidence to continue to work on this (I also
 want to expose async g2d operation to userspace).
 
Glad that you're still planning to work on this. Ideally the
Exynos/Samsung guys will take a look at the rest of your patches which
require someone with greater knowledge of the hardware than me :-)

Cheers,
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

Re: [v3] libdrm: improvements to userspace exynos component

[Emil Velikov]

On 25 February 2015 at 15:54, Emil Velikov emil.l.veli...@gmail.com wrote:
 On 24 February 2015 at 14:20, Tobias Jakobi
 tjak...@math.uni-bielefeld.de wrote:
 Hello,

 here are some miscellaneous improvements (small features, bugfixes, spelling 
 fixes, etc.) for the exynos component of libdrm. The general idea is to let 
 userspace use the G2D engine functionality more
 efficiently.

 If someone is interested in an application that actually makes use of this, 
 the RetroArch frontend has a custom video backend:
 https://github.com/libretro/RetroArch/blob/master/gfx/drivers/exynos_gfx.c


 Please review and let me know what I can improve.

 v2:
 - Mention value of G2D scaling normalization factor (02/15).
 - Moved patch (04/15) description from commit message to source itself, like 
 suggested by Joonyoung Shim.

 v3:
 I integrated the suggestions by Emil Velikov and added two additional 
 patches to the series. One doing the header cleanup that Emil point out, 
 another one just a trivial whitespace thing. I'm resending
 the whole series, since number of patches and order changed.

 Thanks for the update. For the future please add the tags
 (tested-by,etc.) when resending patches.
 Don't worry about these I'll add them before pushing. The two extra
 patches look good imho.

 Inki,

 This cleanup series has been around for a little while now. v3 can be found at
 http://lists.freedesktop.org/archives/dri-devel/2015-February/078111.html

 Do you mind if we apply the trivial and tested* patches within the
 next week or so ?
 The more serious patches hw specific, etc are lacking preview and can
 be committed at a later stage.

 Thanks
 Emil

 [*] Patches 2-4, 6, 8, 10, 12, 14-17, out of which Joonyoung Shim has
 tested all but 12 and 17.
I'm planning to push the above mentioned patches around lunchtime this
Tuesday. If anyone has objections please speak up.

-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

Re: [v3] libdrm: improvements to userspace exynos component

[Emil Velikov]

On 24 February 2015 at 14:20, Tobias Jakobi
tjak...@math.uni-bielefeld.de wrote:
 Hello,

 here are some miscellaneous improvements (small features, bugfixes, spelling 
 fixes, etc.) for the exynos component of libdrm. The general idea is to let 
 userspace use the G2D engine functionality more
 efficiently.

 If someone is interested in an application that actually makes use of this, 
 the RetroArch frontend has a custom video backend:
 https://github.com/libretro/RetroArch/blob/master/gfx/drivers/exynos_gfx.c


 Please review and let me know what I can improve.

 v2:
 - Mention value of G2D scaling normalization factor (02/15).
 - Moved patch (04/15) description from commit message to source itself, like 
 suggested by Joonyoung Shim.

 v3:
 I integrated the suggestions by Emil Velikov and added two additional patches 
 to the series. One doing the header cleanup that Emil point out, another one 
 just a trivial whitespace thing. I'm resending
 the whole series, since number of patches and order changed.

Thanks for the update. For the future please add the tags
(tested-by,etc.) when resending patches.
Don't worry about these I'll add them before pushing. The two extra
patches look good imho.

Inki,

This cleanup series has been around for a little while now. v3 can be found at
http://lists.freedesktop.org/archives/dri-devel/2015-February/078111.html

Do you mind if we apply the trivial and tested* patches within the
next week or so ?
The more serious patches hw specific, etc are lacking preview and can
be committed at a later stage.

Thanks
Emil

[*] Patches 2-4, 6, 8, 10, 12, 14-17, out of which Joonyoung Shim has
tested all but 12 and 17.
--
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 04/15] tests/exynos: disable the G2D userptr/blend test

[Emil Velikov]

On 16/02/15 13:46, Tobias Jakobi wrote:
 v2: Move the commit description into the patch itself.
 Signed-off-by: Tobias Jakobi tjak...@math.uni-bielefeld.de
 ---
  tests/exynos/exynos_fimg2d_test.c | 8 
  1 file changed, 8 insertions(+)
 
 diff --git a/tests/exynos/exynos_fimg2d_test.c 
 b/tests/exynos/exynos_fimg2d_test.c
 index aa140e5..55d2970 100644
 --- a/tests/exynos/exynos_fimg2d_test.c
 +++ b/tests/exynos/exynos_fimg2d_test.c
 @@ -788,11 +788,19 @@ int main(int argc, char **argv)
  
   getchar();
  
 +  /* The blend test uses the userptr functionality of exynos-drm, which *
 +   * is currently not safe to use. If the kernel hasn't been build with *
 +   * exynos-iommu support, then the blend test is going to produce (kernel) *
 +   * memory corruption, eventually leading to a system crash.   *
 +   **
 +   * Disable the test for now, until the kernel code has been sanitized.
 */
 +#if 0
I cannot see a part of libdrm that uses this commenting format. Perhaps
use the more common:

/*
 * Some comment
 */

Cheers,
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

Re: [PATCH v2 12/15] exynos: add fimg2d header to common includes

[Emil Velikov]

On 16/02/15 13:46, Tobias Jakobi wrote:
 The reason for this change is to let userspace use the header.
 Currently 'make install' does not install it.
 
Hi Tobias,

Afaict that this was done intentionally. I believe the Samsung guys got
this out only to fulfil the no drm(render) driver without open
userspace policy.

Although it's nice to see actual user(s) (outside of libdrm) perhaps the
header could be cleaned up (#define TRUE 0, #define FALSE -1) a bit
before that ?

Either way it's up-to the Samsung/Exynos people to make the call.

Cheers
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

Re: [v2] libdrm: improvements to userspace exynos component

[Emil Velikov]

On 16/02/15 13:46, Tobias Jakobi wrote:
 Hello,
 
 here are some miscellaneous improvements (small features, bugfixes, spelling 
 fixes, etc.) for the exynos component of libdrm. The general idea is to let 
 userspace use the G2D engine functionality more 
 efficiently.
 
 If someone is interested in an application that actually makes use of this, 
 the RetroArch frontend has a custom video backend:
 https://github.com/libretro/RetroArch/blob/master/gfx/drivers/exynos_gfx.c
 
 
 Please review and let me know what I can improve.
 
 v2:
 - Mention value of G2D scaling normalization factor (02/15).
 - Moved patch (04/15) description from commit message to source itself, like 
 suggested by Joonyoung Shim.
 
Hi Tobias,

Imho these are some nice cleanups. I believe that the Samsung/Exynos
people need to comment on the device specific ones, but I've checked
2-4, 6, 8, 10, 13-15 and they are quite ok (baring a trivial comment)

Reviewed-by: Emil Velikov emil.l.veli...@gmail.com

Thanks
Emil

P.S. Please don't recent the entire series, unless needed.

--
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 08/15] exynos: introduce g2d_add_base_addr helper function

[Emil Velikov]

On 16/02/15 13:46, Tobias Jakobi wrote:
 In almost all functions the base address register is written, so it
 makes sense to have a helper function for this.
 
 Signed-off-by: Tobias Jakobi tjak...@math.uni-bielefeld.de
 ---
  exynos/exynos_fimg2d.c | 87 
 +++---
  1 file changed, 33 insertions(+), 54 deletions(-)
 
 diff --git a/exynos/exynos_fimg2d.c b/exynos/exynos_fimg2d.c
 index b79081e..c08974a 100644
 --- a/exynos/exynos_fimg2d.c
 +++ b/exynos/exynos_fimg2d.c
 @@ -41,6 +41,11 @@
  
  #define MIN(a, b)((a)  (b) ? (a) : (b))
  
 +enum g2d_base_addr_reg {
 + g2d_dst = 0,
 + g2d_src
 +};
 +
  static unsigned int g2d_get_scaling(unsigned int src, unsigned int dst)
  {
   /* The G2D hw scaling factor is a normalized inverse of the scaling 
 factor. *
 @@ -132,6 +137,25 @@ static int g2d_add_cmd(struct g2d_context *ctx, unsigned 
 long cmd,
  }
  
  /*
 + * g2d_add_base_addr - helper function to set dst/src base address register.
 + *
 + * @ctx: a pointer to g2d_context structure.
 + * @img: a pointer to the dst/src g2d_image structure.
 + * @reg: the register that should be set.
 + */
 +static void g2d_add_base_addr(struct g2d_context *ctx, struct g2d_image *img,
 + enum g2d_base_addr_reg reg)
 +{
 + const unsigned long cmd = (reg == g2d_dst) ? DST_BASE_ADDR_REG : 
 SRC_BASE_ADDR_REG;
 +
Can we wrap this to 80 columns please ?

-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

Re: [PATCH v2 06/15] tests/exynos: introduce wait_for_user_input

[Emil Velikov]

On 16/02/15 13:46, Tobias Jakobi wrote:
 Currently getchar() is used to pause execution after each test.
 The user isn't informed if one is supposed to do anything for
 the tests to continue, so print a simple message to make this
 more clear.
 
 Signed-off-by: Tobias Jakobi tjak...@math.uni-bielefeld.de
 ---
  tests/exynos/exynos_fimg2d_test.c | 20 
  1 file changed, 16 insertions(+), 4 deletions(-)
 
 diff --git a/tests/exynos/exynos_fimg2d_test.c 
 b/tests/exynos/exynos_fimg2d_test.c
 index 55d2970..446a6c6 100644
 --- a/tests/exynos/exynos_fimg2d_test.c
 +++ b/tests/exynos/exynos_fimg2d_test.c
 @@ -237,6 +237,18 @@ void *create_checkerboard_pattern(unsigned int 
 num_tiles_x,
   return buf;
  }
  
 +static void wait_for_user_input(int last)
 +{
 + printf(press ENTER to );
 +
 + if (last)
 + printf(exit test application\n);
 + else
 + printf(skip to next test\n);
 +
If interested you can compact this as

printf(press ENTER to %s\n, last ? exit test application :
   skip to next test);


Cheers
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

Re: [PATCH 1/2] exynos: Don't use DRM_EXYNOS_GEM_{MAP_OFFSET/MMAP} ioctls

[Emil Velikov]

On 26 January 2015 at 01:42, Hyungwon Hwang human.hw...@samsung.com wrote:
 Dear Tobias,

 Thanks for fixing it.

 Signed-off-by: Hyungwon Hwang human.hw...@samsung.com

Hi Hyungwon Hwang

I'm a bit confused about the use of s-o-b here. I've assumed that
libdrm follows the kernel style [1] for those type of things ?
Would you have anything handy which I can read on the topic ?

Cheers,
Emil

[1] 
https://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/tree/Documentation/SubmittingPatches#n358

A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
--
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 1/2] exynos: Don't use DRM_EXYNOS_GEM_{MAP_OFFSET/MMAP} ioctls

[Emil Velikov]

On 27 January 2015 at 01:05, Hyungwon Hwang human.hw...@samsung.com wrote:
 Dear Emil,

 On Mon, 26 Jan 2015 19:05:51 +
 Emil Velikov emil.l.veli...@gmail.com wrote:

 On 26 January 2015 at 01:42, Hyungwon Hwang human.hw...@samsung.com
 wrote:
  Dear Tobias,
 
  Thanks for fixing it.
 
  Signed-off-by: Hyungwon Hwang human.hw...@samsung.com
 
 Hi Hyungwon Hwang

 I'm a bit confused about the use of s-o-b here. I've assumed that
 libdrm follows the kernel style [1] for those type of things ?
 Would you have anything handy which I can read on the topic ?

 I also think that DRI-devel mailing list follow the kernel rule.
Sweet I'm not loosing my mind (much).

[...]
 Was it wrong to add s-o-b here? Should I have used Reviewed-by here?
 I am sorry, if I broke the rule and confused you.

Don't think any apology is due, I was just hinting that the patch has
the s-o-b already :P
Where as what I should have used that depends on your intent and/or
the action undertaken.

Thanks
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

Re: [PATCH] drm/exynos: resolve infinite loop issue on non multi-platform

[Emil Velikov]

Hi Inki,

With all respect,

On 06/11/14 14:10, Inki Dae wrote: This patch resovles the infinite
loop issue incurred
 when Exyno drm driver is enabled but all kms drivers
 are disabled on Exynos board by returning -EPROBE_DEFER
 only in case that there is kms device registered.
 
I believe it's preferred to add changelog, the original reporter (so
that they can test) and the bug report in the commit message. Something
like the following:

v2:
 - Use drm_component_list the rather than of_machine_is_compatible to
check for presence of an Exynos SoC.

Cc: Matwey V. Kornilov matwey.korni...@gmail.com
References: https://bugzilla.kernel.org/show_bug.cgi?id=87691

--
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] drm/exynos: resolve infinite loop issue on non multi-platform

[Emil Velikov]

On 06/11/14 15:44, Emil Velikov wrote:
 Hi Inki,
 
 With all respect,
 
 On 06/11/14 14:10, Inki Dae wrote: This patch resovles the infinite
 loop issue incurred
 when Exyno drm driver is enabled but all kms drivers
 are disabled on Exynos board by returning -EPROBE_DEFER
 only in case that there is kms device registered.

 I believe it's preferred to add changelog, the original reporter (so
 that they can test) and the bug report in the commit message. Something
 like the following:
 
 v2:
  - Use drm_component_list the rather than of_machine_is_compatible to
 check for presence of an Exynos SoC.
 
 Cc: Matwey V. Kornilov matwey.korni...@gmail.com
 References: https://bugzilla.kernel.org/show_bug.cgi?id=87691
 
I'm a genius. Please disregard the above.

-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