* [PATCH] drm/vgem: Fix vgem_init to get drm device avaliable.
@ 2017-10-25 23:02 Deepak Sharma
2017-10-30 13:03 ` Sean Paul
2017-10-30 13:23 ` Emil Velikov
0 siblings, 2 replies; 8+ messages in thread
From: Deepak Sharma @ 2017-10-25 23:02 UTC (permalink / raw)
To: dri-devel; +Cc: Alexander.Deucher, marcheu, Deepak Sharma
From: Deepak Sharma <deepak.sharma@amd.com>
Modify vgem_init to take platform dev as parent in drm_dev_init.
This will make drm device available at "/sys/devices/platform/vgem"
in x86 chromebook.
Signed-off-by: Deepak Sharma <deepak.sharma@amd.com>
---
drivers/gpu/drm/vgem/vgem_drv.c | 15 +++++++--------
1 file changed, 7 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/vgem/vgem_drv.c b/drivers/gpu/drm/vgem/vgem_drv.c
index c938af8c40cf..17e2eafc62b8 100644
--- a/drivers/gpu/drm/vgem/vgem_drv.c
+++ b/drivers/gpu/drm/vgem/vgem_drv.c
@@ -471,31 +471,30 @@ static int __init vgem_init(void)
if (!vgem_device)
return -ENOMEM;
- ret = drm_dev_init(&vgem_device->drm, &vgem_driver, NULL);
- if (ret)
- goto out_free;
-
vgem_device->platform =
platform_device_register_simple("vgem", -1, NULL, 0);
if (IS_ERR(vgem_device->platform)) {
ret = PTR_ERR(vgem_device->platform);
- goto out_fini;
+ goto out_free;
}
dma_coerce_mask_and_coherent(&vgem_device->platform->dev,
DMA_BIT_MASK(64));
+ ret = drm_dev_init(&vgem_device->drm, &vgem_driver, &vgem_device->platform->dev);
+ if (ret)
+ goto out_unregister;
/* Final step: expose the device/driver to userspace */
ret = drm_dev_register(&vgem_device->drm, 0);
if (ret)
- goto out_unregister;
+ goto out_fini;
return 0;
-out_unregister:
- platform_device_unregister(vgem_device->platform);
out_fini:
drm_dev_fini(&vgem_device->drm);
+out_unregister:
+ platform_device_unregister(vgem_device->platform);
out_free:
kfree(vgem_device);
return ret;
--
2.14.2
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/vgem: Fix vgem_init to get drm device avaliable.
2017-10-25 23:02 [PATCH] drm/vgem: Fix vgem_init to get drm device avaliable Deepak Sharma
@ 2017-10-30 13:03 ` Sean Paul
2017-10-30 13:23 ` Emil Velikov
1 sibling, 0 replies; 8+ messages in thread
From: Sean Paul @ 2017-10-30 13:03 UTC (permalink / raw)
To: Deepak Sharma; +Cc: Alexander.Deucher, marcheu, dri-devel
On Wed, Oct 25, 2017 at 04:02:26PM -0700, Deepak Sharma wrote:
> From: Deepak Sharma <deepak.sharma@amd.com>
>
> Modify vgem_init to take platform dev as parent in drm_dev_init.
> This will make drm device available at "/sys/devices/platform/vgem"
> in x86 chromebook.
>
> Signed-off-by: Deepak Sharma <deepak.sharma@amd.com>
Reviewed-by: Sean Paul <seanpaul@chromium.org>
> ---
> drivers/gpu/drm/vgem/vgem_drv.c | 15 +++++++--------
> 1 file changed, 7 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/vgem/vgem_drv.c b/drivers/gpu/drm/vgem/vgem_drv.c
> index c938af8c40cf..17e2eafc62b8 100644
> --- a/drivers/gpu/drm/vgem/vgem_drv.c
> +++ b/drivers/gpu/drm/vgem/vgem_drv.c
> @@ -471,31 +471,30 @@ static int __init vgem_init(void)
> if (!vgem_device)
> return -ENOMEM;
>
> - ret = drm_dev_init(&vgem_device->drm, &vgem_driver, NULL);
> - if (ret)
> - goto out_free;
> -
> vgem_device->platform =
> platform_device_register_simple("vgem", -1, NULL, 0);
> if (IS_ERR(vgem_device->platform)) {
> ret = PTR_ERR(vgem_device->platform);
> - goto out_fini;
> + goto out_free;
> }
>
> dma_coerce_mask_and_coherent(&vgem_device->platform->dev,
> DMA_BIT_MASK(64));
> + ret = drm_dev_init(&vgem_device->drm, &vgem_driver, &vgem_device->platform->dev);
> + if (ret)
> + goto out_unregister;
>
> /* Final step: expose the device/driver to userspace */
> ret = drm_dev_register(&vgem_device->drm, 0);
> if (ret)
> - goto out_unregister;
> + goto out_fini;
>
> return 0;
>
> -out_unregister:
> - platform_device_unregister(vgem_device->platform);
> out_fini:
> drm_dev_fini(&vgem_device->drm);
> +out_unregister:
> + platform_device_unregister(vgem_device->platform);
> out_free:
> kfree(vgem_device);
> return ret;
> --
> 2.14.2
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/vgem: Fix vgem_init to get drm device avaliable.
2017-10-25 23:02 [PATCH] drm/vgem: Fix vgem_init to get drm device avaliable Deepak Sharma
2017-10-30 13:03 ` Sean Paul
@ 2017-10-30 13:23 ` Emil Velikov
2017-11-09 23:46 ` Sharma, Deepak
1 sibling, 1 reply; 8+ messages in thread
From: Emil Velikov @ 2017-10-30 13:23 UTC (permalink / raw)
To: Deepak Sharma; +Cc: Alex Deucher, Stéphane Marchesin, ML dri-devel
On 26 October 2017 at 00:02, Deepak Sharma <Deepak.Sharma@amd.com> wrote:
> From: Deepak Sharma <deepak.sharma@amd.com>
>
> Modify vgem_init to take platform dev as parent in drm_dev_init.
> This will make drm device available at "/sys/devices/platform/vgem"
> in x86 chromebook.
>
Shouldn't one update the drm_dev_init/drm_dev_alloc documentation
while doing this?
But more importantly, this will change the "unique" string (see
drm_dev_set_unique).
The topic around it rather convoluted and messy, so please check this
change doesn't cause subtle regressions.
There's a doc hunk in drm_ioctl.c to begin with, plus userspace such
as IGT [1] might rely on the current behaviour.
HTH
Emil
[1] https://cgit.freedesktop.org/drm/igt-gpu-tools/
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH] drm/vgem: Fix vgem_init to get drm device avaliable.
2017-10-30 13:23 ` Emil Velikov
@ 2017-11-09 23:46 ` Sharma, Deepak
2017-11-10 13:10 ` Emil Velikov
0 siblings, 1 reply; 8+ messages in thread
From: Sharma, Deepak @ 2017-11-09 23:46 UTC (permalink / raw)
To: Emil Velikov; +Cc: Deucher, Alexander, Stéphane Marchesin, ML dri-devel
-----Original Message-----
From: Emil Velikov [mailto:emil.l.velikov@gmail.com]
Sent: Monday, October 30, 2017 6:23 AM
To: Sharma, Deepak <Deepak.Sharma@amd.com>
Cc: ML dri-devel <dri-devel@lists.freedesktop.org>; Deucher, Alexander <Alexander.Deucher@amd.com>; Stéphane Marchesin <marcheu@chromium.org>
Subject: Re: [PATCH] drm/vgem: Fix vgem_init to get drm device avaliable.
On 26 October 2017 at 00:02, Deepak Sharma <Deepak.Sharma@amd.com> wrote:
> From: Deepak Sharma <deepak.sharma@amd.com>
>
> Modify vgem_init to take platform dev as parent in drm_dev_init.
> This will make drm device available at "/sys/devices/platform/vgem"
> in x86 chromebook.
>
Shouldn't one update the drm_dev_init/drm_dev_alloc documentation while doing this?
But more importantly, this will change the "unique" string (see drm_dev_set_unique).
Sorry I did not get your comment about updating drm_dev_init/drm_dev_alloc documentation
for this change. Do you see any issue if this "unique string " is changed
The topic around it rather convoluted and messy, so please check this change doesn't cause subtle regressions.
There's a doc hunk in drm_ioctl.c to begin with, plus userspace such as IGT [1] might rely on the current behaviour.
HTH
Emil
[1] https://cgit.freedesktop.org/drm/igt-gpu-tools/
I did run vgem test from IGT to check for regression , do you suspect regression in other tests ?
Thanks,
Deepak
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/vgem: Fix vgem_init to get drm device avaliable.
2017-11-09 23:46 ` Sharma, Deepak
@ 2017-11-10 13:10 ` Emil Velikov
2017-11-15 21:25 ` Sharma, Deepak
0 siblings, 1 reply; 8+ messages in thread
From: Emil Velikov @ 2017-11-10 13:10 UTC (permalink / raw)
To: Sharma, Deepak; +Cc: Deucher, Alexander, Stéphane Marchesin, ML dri-devel
On 9 November 2017 at 23:46, Sharma, Deepak <Deepak.Sharma@amd.com> wrote:
>>>
>>> Modify vgem_init to take platform dev as parent in drm_dev_init.
>>> This will make drm device available at "/sys/devices/platform/vgem"
>>> in x86 chromebook.
>>>
>> Shouldn't one update the drm_dev_init/drm_dev_alloc documentation while doing this?
>> But more importantly, this will change the "unique" string (see drm_dev_set_unique).
>
> Sorry I did not get your comment about updating drm_dev_init/drm_dev_alloc documentation
> for this change. Do you see any issue if this "unique string " is changed
>
VGEM is unlike other DRM drivers and I'm not sure if there is any
userspace that depends on the exact value.
If there's none - great.
I'd still recommend updating the two functions' documentation, ideally
coupled with enforcing for *parent to be non NULL.
Could be code as follow-up though.
>> The topic around it rather convoluted and messy, so please check this change doesn't cause subtle regressions.
>> There's a doc hunk in drm_ioctl.c to begin with, plus userspace such as IGT [1] might rely on the current behaviour.
>>
>> HTH
>> Emil
>
> [1] https://cgit.freedesktop.org/drm/igt-gpu-tools/
>
> I did run vgem test from IGT to check for regression , do you suspect regression in other tests ?
>
There are two vgem only and a handful of others. Quick grep shows:
tests/amdgpu/amd_prime.c
tests/gem_concurrent_all.c
tests/gem_exec_await.c
tests/gem_exec_fence.c
tests/gem_exec_latency.c
tests/gem_exec_schedule.c
tests/gem_ringfill.c
tests/gem_wait.c
tests/prime_vgem.c
tests/vgem_basic.c
tests/vgem_slow.c
Most of which use i915 <> vgem. If you don't have the HW to test, one
can use the Intel GFX trybot.
Just keep [1] in the To/CC list and you'll get a report with the results.
HTH
Emil
[1] intel-gfx-trybot@lists.freedesktop.org
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH] drm/vgem: Fix vgem_init to get drm device avaliable.
2017-11-10 13:10 ` Emil Velikov
@ 2017-11-15 21:25 ` Sharma, Deepak
2017-11-16 15:40 ` Emil Velikov
0 siblings, 1 reply; 8+ messages in thread
From: Sharma, Deepak @ 2017-11-15 21:25 UTC (permalink / raw)
To: Emil Velikov
Cc: Deucher, Alexander, Stéphane Marchesin, intel-gfx-trybot,
ML dri-devel
-----Original Message-----
From: Emil Velikov [mailto:emil.l.velikov@gmail.com]
Sent: Friday, November 10, 2017 5:11 AM
To: Sharma, Deepak <Deepak.Sharma@amd.com>
Cc: ML dri-devel <dri-devel@lists.freedesktop.org>; Deucher, Alexander <Alexander.Deucher@amd.com>; Stéphane Marchesin <marcheu@chromium.org>
Subject: Re: [PATCH] drm/vgem: Fix vgem_init to get drm device avaliable.
On 9 November 2017 at 23:46, Sharma, Deepak <Deepak.Sharma@amd.com> wrote:
>>>
>>> Modify vgem_init to take platform dev as parent in drm_dev_init.
>>> This will make drm device available at "/sys/devices/platform/vgem"
>>> in x86 chromebook.
>>>
>> Shouldn't one update the drm_dev_init/drm_dev_alloc documentation while doing this?
>> But more importantly, this will change the "unique" string (see drm_dev_set_unique).
>
> Sorry I did not get your comment about updating
> drm_dev_init/drm_dev_alloc documentation for this change. Do you see
> any issue if this "unique string " is changed
>
VGEM is unlike other DRM drivers and I'm not sure if there is any userspace that depends on the exact value.
If there's none - great.
I'd still recommend updating the two functions' documentation, ideally coupled with enforcing for *parent to be non NULL.
Could be code as follow-up though.
If I got it correctly you are referring "Note that for purely virtual devices @parent can be NULL" for said two functions.
I think changes might be required if it was "should/must be NULL"?
>> The topic around it rather convoluted and messy, so please check this change doesn't cause subtle regressions.
>> There's a doc hunk in drm_ioctl.c to begin with, plus userspace such as IGT [1] might rely on the current behaviour.
>>
>> HTH
>> Emil
>
> [1] https://cgit.freedesktop.org/drm/igt-gpu-tools/
>
> I did run vgem test from IGT to check for regression , do you suspect regression in other tests ?
>
There are two vgem only and a handful of others. Quick grep shows:
tests/amdgpu/amd_prime.c
tests/gem_concurrent_all.c
tests/gem_exec_await.c
tests/gem_exec_fence.c
tests/gem_exec_latency.c
tests/gem_exec_schedule.c
tests/gem_ringfill.c
tests/gem_wait.c
tests/prime_vgem.c
tests/vgem_basic.c
tests/vgem_slow.c
Most of which use i915 <> vgem. If you don't have the HW to test, one can use the Intel GFX trybot.
Just keep [1] in the To/CC list and you'll get a report with the results.
Thanks. I have added Intel GFX trybot in CC, that should be sufficient or I need to send patch again using git send-mail?
HTH
Emil
[1] intel-gfx-trybot@lists.freedesktop.org
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/vgem: Fix vgem_init to get drm device avaliable.
2017-11-15 21:25 ` Sharma, Deepak
@ 2017-11-16 15:40 ` Emil Velikov
2017-11-20 21:22 ` Sharma, Deepak
0 siblings, 1 reply; 8+ messages in thread
From: Emil Velikov @ 2017-11-16 15:40 UTC (permalink / raw)
To: Sharma, Deepak
Cc: Deucher, Alexander, Stéphane Marchesin, intel-gfx-trybot,
ML dri-devel
On 15 November 2017 at 21:25, Sharma, Deepak <Deepak.Sharma@amd.com> wrote:
>>
>> I'd still recommend updating the two functions' documentation, ideally coupled with enforcing for *parent to be non NULL.
>> Could be code as follow-up though.
>>
> If I got it correctly you are referring "Note that for purely virtual devices @parent can be NULL" for said two functions.
> I think changes might be required if it was "should/must be NULL"?
Since you're changing the behaviour:
The statement "Note that for purely virtual devices @parent can be
NULL" is never true and should be dropped.
Additionally, you want to update the functions to error out when
parent is NULL since it indicates a driver bug.
For the drm_dev_set_unique hunk one can drop the comment (it's NA) and
simplify to:
ret = drm_dev_set_unique(dev, dev_name(parent));
As mentioned before - it can be code as follow-up.
>>
>> Most of which use i915 <> vgem. If you don't have the HW to test, one can use the Intel GFX trybot.
>> Just keep [1] in the To/CC list and you'll get a report with the results.
>>
>
> Thanks. I have added Intel GFX trybot in CC, that should be sufficient or I need to send patch again using git send-mail?
>
Right, should have been clearer - the actual patches should be send/cc'd.
Otherwise one has know way of retrieving (and thus testing) the patch ;-)
HTH
Emil
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH] drm/vgem: Fix vgem_init to get drm device avaliable.
2017-11-16 15:40 ` Emil Velikov
@ 2017-11-20 21:22 ` Sharma, Deepak
0 siblings, 0 replies; 8+ messages in thread
From: Sharma, Deepak @ 2017-11-20 21:22 UTC (permalink / raw)
To: Emil Velikov; +Cc: Deucher, Alexander, Stéphane Marchesin, ML dri-devel
I got your point . I can push follow up patch with document change and code update as discussed here.
Keeping that I think this patch should be ok to land?
Thanks,
Deepak
-----Original Message-----
From: Emil Velikov [mailto:emil.l.velikov@gmail.com]
Sent: Thursday, November 16, 2017 7:40 AM
To: Sharma, Deepak <Deepak.Sharma@amd.com>
Cc: ML dri-devel <dri-devel@lists.freedesktop.org>; Deucher, Alexander <Alexander.Deucher@amd.com>; Stéphane Marchesin <marcheu@chromium.org>; intel-gfx-trybot@lists.freedesktop.org
Subject: Re: [PATCH] drm/vgem: Fix vgem_init to get drm device avaliable.
On 15 November 2017 at 21:25, Sharma, Deepak <Deepak.Sharma@amd.com> wrote:
>>
>> I'd still recommend updating the two functions' documentation, ideally coupled with enforcing for *parent to be non NULL.
>> Could be code as follow-up though.
>>
> If I got it correctly you are referring "Note that for purely virtual devices @parent can be NULL" for said two functions.
> I think changes might be required if it was "should/must be NULL"?
Since you're changing the behaviour:
The statement "Note that for purely virtual devices @parent can be NULL" is never true and should be dropped.
Additionally, you want to update the functions to error out when parent is NULL since it indicates a driver bug.
For the drm_dev_set_unique hunk one can drop the comment (it's NA) and simplify to:
ret = drm_dev_set_unique(dev, dev_name(parent));
As mentioned before - it can be code as follow-up.
>>
>> Most of which use i915 <> vgem. If you don't have the HW to test, one can use the Intel GFX trybot.
>> Just keep [1] in the To/CC list and you'll get a report with the results.
>>
>
> Thanks. I have added Intel GFX trybot in CC, that should be sufficient or I need to send patch again using git send-mail?
>
Right, should have been clearer - the actual patches should be send/cc'd.
Otherwise one has know way of retrieving (and thus testing) the patch ;-)
HTH
Emil
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-11-20 21:56 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-10-25 23:02 [PATCH] drm/vgem: Fix vgem_init to get drm device avaliable Deepak Sharma
2017-10-30 13:03 ` Sean Paul
2017-10-30 13:23 ` Emil Velikov
2017-11-09 23:46 ` Sharma, Deepak
2017-11-10 13:10 ` Emil Velikov
2017-11-15 21:25 ` Sharma, Deepak
2017-11-16 15:40 ` Emil Velikov
2017-11-20 21:22 ` Sharma, Deepak
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.