[PATCH] xf86drm: remove to open the DRM device unnecessarily

[PATCH] xf86drm: remove to open the DRM device unnecessarily

[Joonyoung Shim]

This is to remove to open the DRM device unnecessarily as call
drmAvailable() when name is NULL or drm_server_info is NULL in
drmOpenWithType function.

Signed-off-by: Joonyoung Shim <jy0922.shim@samsung.com>
---
 xf86drm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xf86drm.c b/xf86drm.c
index b5a174b..900e4b1 100644
--- a/xf86drm.c
+++ b/xf86drm.c
@@ -725,7 +725,7 @@ int drmOpen(const char *name, const char *busid)
  */
 int drmOpenWithType(const char *name, const char *busid, int type)
 {
-    if (!drmAvailable() && name != NULL && drm_server_info) {
+    if (name != NULL && drm_server_info && !drmAvailable()) {
 	/* try to load the kernel module */
 	if (!drm_server_info->load_module(name)) {
 	    drmMsg("[drm] failed to load kernel module \"%s\"\n", name);
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

RE: [PATCH] xf86drm: remove to open the DRM device unnecessarily

[Zhou, Jammy]

Reviewed-by: Jammy Zhou <Jammy.Zhou@amd.com>

Regards,
Jammy

-----Original Message-----
From: dri-devel [mailto:dri-devel-bounces@lists.freedesktop.org] On Behalf Of Joonyoung Shim
Sent: Thursday, May 28, 2015 8:58 AM
To: dri-devel@lists.freedesktop.org
Cc: emil.l.velikov@gmail.com
Subject: [PATCH] xf86drm: remove to open the DRM device unnecessarily

This is to remove to open the DRM device unnecessarily as call
drmAvailable() when name is NULL or drm_server_info is NULL in drmOpenWithType function.

Signed-off-by: Joonyoung Shim <jy0922.shim@samsung.com>
---
 xf86drm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xf86drm.c b/xf86drm.c
index b5a174b..900e4b1 100644
--- a/xf86drm.c
+++ b/xf86drm.c
@@ -725,7 +725,7 @@ int drmOpen(const char *name, const char *busid)
  */
 int drmOpenWithType(const char *name, const char *busid, int type)  {
-    if (!drmAvailable() && name != NULL && drm_server_info) {
+    if (name != NULL && drm_server_info && !drmAvailable()) {
 	/* try to load the kernel module */
 	if (!drm_server_info->load_module(name)) {
 	    drmMsg("[drm] failed to load kernel module \"%s\"\n", name);
--
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] xf86drm: remove to open the DRM device unnecessarily

[Emil Velikov]

On 28 May 2015 at 00:57, Joonyoung Shim <jy0922.shim@samsung.com> wrote:
> This is to remove to open the DRM device unnecessarily as call
> drmAvailable() when name is NULL or drm_server_info is NULL in
> drmOpenWithType function.
>
Why do you believe that calling drmAvailable() is not necessary ? If
that's the case should one just nuke the call all together ?

-Emil
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] xf86drm: remove to open the DRM device unnecessarily

[Daniel Kurtz]

[-- Attachment #1.1: Type: text/plain, Size: 729 bytes --]

It's not necessary if we are about to skip the rest of the if clause anyway
because name is NULL.
On May 28, 2015 9:14 PM, "Emil Velikov" <emil.l.velikov@gmail.com> wrote:

> On 28 May 2015 at 00:57, Joonyoung Shim <jy0922.shim@samsung.com> wrote:
> > This is to remove to open the DRM device unnecessarily as call
> > drmAvailable() when name is NULL or drm_server_info is NULL in
> > drmOpenWithType function.
> >
> Why do you believe that calling drmAvailable() is not necessary ? If
> that's the case should one just nuke the call all together ?
>
> -Emil
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>

[-- Attachment #1.2: Type: text/html, Size: 1226 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] xf86drm: remove to open the DRM device unnecessarily

[Emil Velikov]

Seems like I'm either too subtle and/or too stingy earlier.

If drmAvailable() returns false, we have two options:
 - opt for the old-schoold (dri1) and ask drm_server_info to load the
module for us, or
 - bail out, as neither drmOpenByBusid() or drmOpenByName() will be
able to open the device considering that a DRM module is not loaded.

So what I was hinting earlier was to make the above more obvious,
rather than reordering the arguments in the if clause. How does that
sound ?

Thanks
Emil

On 28 May 2015 at 15:15, Daniel Kurtz <djkurtz@google.com> wrote:
> It's not necessary if we are about to skip the rest of the if clause anyway
> because name is NULL.
>
> On May 28, 2015 9:14 PM, "Emil Velikov" <emil.l.velikov@gmail.com> wrote:
>>
>> On 28 May 2015 at 00:57, Joonyoung Shim <jy0922.shim@samsung.com> wrote:
>> > This is to remove to open the DRM device unnecessarily as call
>> > drmAvailable() when name is NULL or drm_server_info is NULL in
>> > drmOpenWithType function.
>> >
>> Why do you believe that calling drmAvailable() is not necessary ? If
>> that's the case should one just nuke the call all together ?
>>
>> -Emil
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] xf86drm: remove to open the DRM device unnecessarily

[Joonyoung Shim]

On 05/29/2015 12:51 AM, Emil Velikov wrote:
> Seems like I'm either too subtle and/or too stingy earlier.
> 
> If drmAvailable() returns false, we have two options:
>  - opt for the old-schoold (dri1) and ask drm_server_info to load the
> module for us, or
>  - bail out, as neither drmOpenByBusid() or drmOpenByName() will be
> able to open the device considering that a DRM module is not loaded.
> 
> So what I was hinting earlier was to make the above more obvious,
> rather than reordering the arguments in the if clause. How does that
> sound ?
> 

I'm unhappy about to open DRM device always via drmAvailable(). IMHO
it's enough to check DRM device can be open by drmOpenByBusid() or
drmOpenByName() if don't load module and actually i expect DRM device
is open only once when call drmOpenWithType().

> Thanks
> Emil
> 
> On 28 May 2015 at 15:15, Daniel Kurtz <djkurtz@google.com> wrote:
>> It's not necessary if we are about to skip the rest of the if clause anyway
>> because name is NULL.
>>
>> On May 28, 2015 9:14 PM, "Emil Velikov" <emil.l.velikov@gmail.com> wrote:
>>>
>>> On 28 May 2015 at 00:57, Joonyoung Shim <jy0922.shim@samsung.com> wrote:
>>>> This is to remove to open the DRM device unnecessarily as call
>>>> drmAvailable() when name is NULL or drm_server_info is NULL in
>>>> drmOpenWithType function.
>>>>
>>> Why do you believe that calling drmAvailable() is not necessary ? If
>>> that's the case should one just nuke the call all together ?
>>>
>>> -Emil
>>> _______________________________________________
>>> dri-devel mailing list
>>> dri-devel@lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] xf86drm: remove to open the DRM device unnecessarily

[Emil Velikov]

On 29 May 2015 at 07:50, Joonyoung Shim <jy0922.shim@samsung.com> wrote:
> On 05/29/2015 12:51 AM, Emil Velikov wrote:
>> Seems like I'm either too subtle and/or too stingy earlier.
>>
>> If drmAvailable() returns false, we have two options:
>>  - opt for the old-schoold (dri1) and ask drm_server_info to load the
>> module for us, or
>>  - bail out, as neither drmOpenByBusid() or drmOpenByName() will be
>> able to open the device considering that a DRM module is not loaded.
>>
>> So what I was hinting earlier was to make the above more obvious,
>> rather than reordering the arguments in the if clause. How does that
>> sound ?
>>
>
> I'm unhappy about to open DRM device always via drmAvailable(). IMHO
> it's enough to check DRM device can be open by drmOpenByBusid() or
> drmOpenByName() if don't load module and actually i expect DRM device
> is open only once when call drmOpenWithType().
>
Seems that checking via drmAvailable() is quicker than going through
the whole drmOpen*. On the other side the former does not cater for
render only devices... fun.

Seems like this is the price to pay, considering that our current
libdrm caters after both UMS(DRI1) and KMS(DRI2+) drivers. In the
kernel we did have some nice cleanup (props to Daniel V and others)
although in the userspace side things are less polished.

Perhaps we can get libdrm 3 soon or start annotating the old functions
as depreciated ? Curious if one should add libdrm_ancient.so (or
similar) to ensure that things don't break when building old apps
against new libdrm ?

TLDR: No objections against the patch, but we should consider
cleaning/splitting the ancient APIs.

-Emil
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] xf86drm: remove to open the DRM device unnecessarily

[Joonyoung Shim]

On 05/30/2015 10:20 PM, Emil Velikov wrote:
> On 29 May 2015 at 07:50, Joonyoung Shim <jy0922.shim@samsung.com> wrote:
>> On 05/29/2015 12:51 AM, Emil Velikov wrote:
>>> Seems like I'm either too subtle and/or too stingy earlier.
>>>
>>> If drmAvailable() returns false, we have two options:
>>>  - opt for the old-schoold (dri1) and ask drm_server_info to load the
>>> module for us, or
>>>  - bail out, as neither drmOpenByBusid() or drmOpenByName() will be
>>> able to open the device considering that a DRM module is not loaded.
>>>
>>> So what I was hinting earlier was to make the above more obvious,
>>> rather than reordering the arguments in the if clause. How does that
>>> sound ?
>>>
>>
>> I'm unhappy about to open DRM device always via drmAvailable(). IMHO
>> it's enough to check DRM device can be open by drmOpenByBusid() or
>> drmOpenByName() if don't load module and actually i expect DRM device
>> is open only once when call drmOpenWithType().
>>
> Seems that checking via drmAvailable() is quicker than going through
> the whole drmOpen*. On the other side the former does not cater for
> render only devices... fun.
> 

If want to check via drmAvailable(), we can call it regardless of
drmOpen*.

> Seems like this is the price to pay, considering that our current
> libdrm caters after both UMS(DRI1) and KMS(DRI2+) drivers. In the
> kernel we did have some nice cleanup (props to Daniel V and others)
> although in the userspace side things are less polished.
> 
> Perhaps we can get libdrm 3 soon or start annotating the old functions
> as depreciated ? Curious if one should add libdrm_ancient.so (or
> similar) to ensure that things don't break when building old apps
> against new libdrm ?
> 
> TLDR: No objections against the patch, but we should consider
> cleaning/splitting the ancient APIs.
> 

Agree but what i wanted to say is just to remove unnecessary to open DRM
device in drmOpen*.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel