Re: [PATCH 16/21] drm/radeon: Use drm_fb_helper_fill_info
Den 26.03.2019 14.20, skrev Daniel Vetter: > This should not result in any changes. > > v2: Rebase > > Signed-off-by: Daniel Vetter > Cc: Alex Deucher > Cc: "Christian König" > Cc: "David (ChunMing) Zhou" > Cc: amd-gfx@lists.freedesktop.org > --- Acked-by: Noralf Trønnes ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v2 0/2] drm/drv: Rework drm_dev_unplug() (was: Remove drm_dev_unplug())
Den 08.02.2019 15.01, skrev Noralf Trønnes: > This series makes drm_dev_unplug() compatible with the upcoming > devm_drm_dev_init(), fixes a double drm_dev_unregister() situation and > simplifies the drm_device ref handling wrt to the last fd closed after > unregister. > > The first version of this patchset removed drm_dev_unplug(), see here > for the discussion as to why it is kept for the time being: > > [2/6] drm/drv: Prepare to remove drm_dev_unplug() > https://patchwork.freedesktop.org/patch/282902/ > > Noralf. > > Noralf Trønnes (2): > drm: Fix drm_release() and device unplug > drm/drv: drm_dev_unplug(): Move out drm_dev_put() call > > drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 1 + > drivers/gpu/drm/drm_drv.c | 5 - > drivers/gpu/drm/drm_file.c | 6 ++ > drivers/gpu/drm/udl/udl_drv.c | 1 + > drivers/gpu/drm/xen/xen_drm_front.c | 1 + > 5 files changed, 5 insertions(+), 9 deletions(-) > Applied to drm-misc-next, thanks for reviewing. Noralf. ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH v2 2/2] drm/drv: drm_dev_unplug(): Move out drm_dev_put() call
This makes it possible to use drm_dev_unplug() with the upcoming devm_drm_dev_init() which will do drm_dev_put() in its release callback. Cc: Alex Deucher Cc: Christian König Cc: David (ChunMing) Zhou Cc: Dave Airlie Cc: Sean Paul Cc: Oleksandr Andrushchenko Cc: Daniel Vetter Signed-off-by: Noralf Trønnes --- I will take this through drm-misc-next. Noralf. drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 1 + drivers/gpu/drm/drm_drv.c | 1 - drivers/gpu/drm/udl/udl_drv.c | 1 + drivers/gpu/drm/xen/xen_drm_front.c | 1 + 4 files changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c index a1bb3773087b..d1f37ba3c118 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c @@ -971,6 +971,7 @@ amdgpu_pci_remove(struct pci_dev *pdev) DRM_ERROR("Device removal is currently not supported outside of fbcon\n"); drm_dev_unplug(dev); + drm_dev_put(dev); pci_disable_device(pdev); pci_set_drvdata(pdev, NULL); } diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 05bbc2b622fc..b04982101fcb 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -376,7 +376,6 @@ void drm_dev_unplug(struct drm_device *dev) synchronize_srcu(_unplug_srcu); drm_dev_unregister(dev); - drm_dev_put(dev); } EXPORT_SYMBOL(drm_dev_unplug); diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c index 22cd2d13e272..53b7b8c04bc6 100644 --- a/drivers/gpu/drm/udl/udl_drv.c +++ b/drivers/gpu/drm/udl/udl_drv.c @@ -107,6 +107,7 @@ static void udl_usb_disconnect(struct usb_interface *interface) udl_fbdev_unplug(dev); udl_drop_usb(dev); drm_dev_unplug(dev); + drm_dev_put(dev); } /* diff --git a/drivers/gpu/drm/xen/xen_drm_front.c b/drivers/gpu/drm/xen/xen_drm_front.c index 3e78a832d7f9..84aa4d61dc42 100644 --- a/drivers/gpu/drm/xen/xen_drm_front.c +++ b/drivers/gpu/drm/xen/xen_drm_front.c @@ -582,6 +582,7 @@ static void xen_drm_drv_fini(struct xen_drm_front_info *front_info) drm_kms_helper_poll_fini(dev); drm_dev_unplug(dev); + drm_dev_put(dev); front_info->drm_info = NULL; -- 2.20.1 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH v2 0/2] drm/drv: Rework drm_dev_unplug() (was: Remove drm_dev_unplug())
This series makes drm_dev_unplug() compatible with the upcoming devm_drm_dev_init(), fixes a double drm_dev_unregister() situation and simplifies the drm_device ref handling wrt to the last fd closed after unregister. The first version of this patchset removed drm_dev_unplug(), see here for the discussion as to why it is kept for the time being: [2/6] drm/drv: Prepare to remove drm_dev_unplug() https://patchwork.freedesktop.org/patch/282902/ Noralf. Noralf Trønnes (2): drm: Fix drm_release() and device unplug drm/drv: drm_dev_unplug(): Move out drm_dev_put() call drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 1 + drivers/gpu/drm/drm_drv.c | 5 - drivers/gpu/drm/drm_file.c | 6 ++ drivers/gpu/drm/udl/udl_drv.c | 1 + drivers/gpu/drm/xen/xen_drm_front.c | 1 + 5 files changed, 5 insertions(+), 9 deletions(-) -- 2.20.1 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH v2 1/2] drm: Fix drm_release() and device unplug
If userspace has open fd(s) when drm_dev_unplug() is run, it will result in drm_dev_unregister() being called twice. First in drm_dev_unplug() and then later in drm_release() through the call to drm_put_dev(). Since userspace already holds a ref on drm_device through the drm_minor, it's not necessary to add extra ref counting based on no open file handles. Instead just drm_dev_put() unconditionally in drm_dev_unplug(). We now have this: - Userpace holds a ref on drm_device as long as there's open fd(s) - The driver holds a ref on drm_device as long as it's bound to the struct device When both sides are done with drm_device, it is released. Signed-off-by: Noralf Trønnes Reviewed-by: Oleksandr Andrushchenko Reviewed-by: Daniel Vetter Reviewed-by: Sean Paul --- drivers/gpu/drm/drm_drv.c | 6 +- drivers/gpu/drm/drm_file.c | 6 ++ 2 files changed, 3 insertions(+), 9 deletions(-) diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 381581b01d48..05bbc2b622fc 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -376,11 +376,7 @@ void drm_dev_unplug(struct drm_device *dev) synchronize_srcu(_unplug_srcu); drm_dev_unregister(dev); - - mutex_lock(_global_mutex); - if (dev->open_count == 0) - drm_dev_put(dev); - mutex_unlock(_global_mutex); + drm_dev_put(dev); } EXPORT_SYMBOL(drm_dev_unplug); diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c index 46f48f245eb5..3f20f598cd7c 100644 --- a/drivers/gpu/drm/drm_file.c +++ b/drivers/gpu/drm/drm_file.c @@ -479,11 +479,9 @@ int drm_release(struct inode *inode, struct file *filp) drm_file_free(file_priv); - if (!--dev->open_count) { + if (!--dev->open_count) drm_lastclose(dev); - if (drm_dev_is_unplugged(dev)) - drm_put_dev(dev); - } + mutex_unlock(_global_mutex); drm_minor_release(minor); -- 2.20.1 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [Intel-gfx] [PATCH 2/6] drm/drv: Prepare to remove drm_dev_unplug()
Den 06.02.2019 16.26, skrev Daniel Vetter: > On Tue, Feb 05, 2019 at 06:57:50PM +0100, Noralf Trønnes wrote: >> >> >> Den 05.02.2019 17.31, skrev Daniel Vetter: >>> On Tue, Feb 05, 2019 at 11:20:55AM +0100, Noralf Trønnes wrote: >>>> >>>> >>>> Den 05.02.2019 10.11, skrev Daniel Vetter: >>>>> On Mon, Feb 04, 2019 at 06:35:28PM +0100, Noralf Trønnes wrote: >>>>>> >>>>>> >>>>>> Den 04.02.2019 16.41, skrev Daniel Vetter: >>>>>>> On Sun, Feb 03, 2019 at 04:41:56PM +0100, Noralf Trønnes wrote: >>>>>>>> The only thing now that makes drm_dev_unplug() special is that it sets >>>>>>>> drm_device->unplugged. Move this code to drm_dev_unregister() so that >>>>>>>> we >>>>>>>> can remove drm_dev_unplug(). >>>>>>>> >>>>>>>> Signed-off-by: Noralf Trønnes >>>>>>>> --- >>>>>> >>>>>> [...] >>>>>> >>>>>>>> drivers/gpu/drm/drm_drv.c | 27 +++ >>>>>>>> include/drm/drm_drv.h | 10 -- >>>>>>>> 2 files changed, 19 insertions(+), 18 deletions(-) >>>>>>>> >>>>>>>> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c >>>>>>>> index 05bbc2b622fc..e0941200edc6 100644 >>>>>>>> --- a/drivers/gpu/drm/drm_drv.c >>>>>>>> +++ b/drivers/gpu/drm/drm_drv.c >>>>>>>> @@ -366,15 +366,6 @@ EXPORT_SYMBOL(drm_dev_exit); >>>>>>>> */ >>>>>>>> void drm_dev_unplug(struct drm_device *dev) >>>>>>>> { >>>>>>>> - /* >>>>>>>> - * After synchronizing any critical read section is guaranteed >>>>>>>> to see >>>>>>>> - * the new value of ->unplugged, and any critical section which >>>>>>>> might >>>>>>>> - * still have seen the old value of ->unplugged is guaranteed >>>>>>>> to have >>>>>>>> - * finished. >>>>>>>> - */ >>>>>>>> - dev->unplugged = true; >>>>>>>> - synchronize_srcu(_unplug_srcu); >>>>>>>> - >>>>>>>>drm_dev_unregister(dev); >>>>>>>>drm_dev_put(dev); >>>>>>>> } >>>>>>>> @@ -832,11 +823,14 @@ EXPORT_SYMBOL(drm_dev_register); >>>>>>>> * drm_dev_register() but does not deallocate the device. The caller >>>>>>>> must call >>>>>>>> * drm_dev_put() to drop their final reference. >>>>>>>> * >>>>>>>> - * A special form of unregistering for hotpluggable devices is >>>>>>>> drm_dev_unplug(), >>>>>>>> - * which can be called while there are still open users of @dev. >>>>>>>> + * This function can be called while there are still open users of >>>>>>>> @dev as long >>>>>>>> + * as the driver protects its device resources using drm_dev_enter() >>>>>>>> and >>>>>>>> + * drm_dev_exit(). >>>>>>>> * >>>>>>>> * This should be called first in the device teardown code to make >>>>>>>> sure >>>>>>>> - * userspace can't access the device instance any more. >>>>>>>> + * userspace can't access the device instance any more. Drivers that >>>>>>>> support >>>>>>>> + * device unplug will probably want to call >>>>>>>> drm_atomic_helper_shutdown() first >>>>>>> >>>>>>> Read once more with a bit more coffee, spotted this: >>>>>>> >>>>>>> s/first/afterwards/ - shutting down the hw before we've taken it away >>>>>>> from >>>>>>> userspace is kinda the wrong way round. It should be the inverse of >>>>>>> driver >>>>>>> load, which is 1) allocate structures 2) prep hw 3) register driver with >>>>>>> the world (simplified ofc). >>>>
Re: [Intel-gfx] [PATCH 2/6] drm/drv: Prepare to remove drm_dev_unplug()
Den 05.02.2019 17.31, skrev Daniel Vetter: > On Tue, Feb 05, 2019 at 11:20:55AM +0100, Noralf Trønnes wrote: >> >> >> Den 05.02.2019 10.11, skrev Daniel Vetter: >>> On Mon, Feb 04, 2019 at 06:35:28PM +0100, Noralf Trønnes wrote: >>>> >>>> >>>> Den 04.02.2019 16.41, skrev Daniel Vetter: >>>>> On Sun, Feb 03, 2019 at 04:41:56PM +0100, Noralf Trønnes wrote: >>>>>> The only thing now that makes drm_dev_unplug() special is that it sets >>>>>> drm_device->unplugged. Move this code to drm_dev_unregister() so that we >>>>>> can remove drm_dev_unplug(). >>>>>> >>>>>> Signed-off-by: Noralf Trønnes >>>>>> --- >>>> >>>> [...] >>>> >>>>>> drivers/gpu/drm/drm_drv.c | 27 +++ >>>>>> include/drm/drm_drv.h | 10 -- >>>>>> 2 files changed, 19 insertions(+), 18 deletions(-) >>>>>> >>>>>> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c >>>>>> index 05bbc2b622fc..e0941200edc6 100644 >>>>>> --- a/drivers/gpu/drm/drm_drv.c >>>>>> +++ b/drivers/gpu/drm/drm_drv.c >>>>>> @@ -366,15 +366,6 @@ EXPORT_SYMBOL(drm_dev_exit); >>>>>> */ >>>>>> void drm_dev_unplug(struct drm_device *dev) >>>>>> { >>>>>> -/* >>>>>> - * After synchronizing any critical read section is guaranteed >>>>>> to see >>>>>> - * the new value of ->unplugged, and any critical section which >>>>>> might >>>>>> - * still have seen the old value of ->unplugged is guaranteed >>>>>> to have >>>>>> - * finished. >>>>>> - */ >>>>>> -dev->unplugged = true; >>>>>> -synchronize_srcu(_unplug_srcu); >>>>>> - >>>>>> drm_dev_unregister(dev); >>>>>> drm_dev_put(dev); >>>>>> } >>>>>> @@ -832,11 +823,14 @@ EXPORT_SYMBOL(drm_dev_register); >>>>>> * drm_dev_register() but does not deallocate the device. The caller >>>>>> must call >>>>>> * drm_dev_put() to drop their final reference. >>>>>> * >>>>>> - * A special form of unregistering for hotpluggable devices is >>>>>> drm_dev_unplug(), >>>>>> - * which can be called while there are still open users of @dev. >>>>>> + * This function can be called while there are still open users of @dev >>>>>> as long >>>>>> + * as the driver protects its device resources using drm_dev_enter() and >>>>>> + * drm_dev_exit(). >>>>>> * >>>>>> * This should be called first in the device teardown code to make sure >>>>>> - * userspace can't access the device instance any more. >>>>>> + * userspace can't access the device instance any more. Drivers that >>>>>> support >>>>>> + * device unplug will probably want to call >>>>>> drm_atomic_helper_shutdown() first >>>>> >>>>> Read once more with a bit more coffee, spotted this: >>>>> >>>>> s/first/afterwards/ - shutting down the hw before we've taken it away from >>>>> userspace is kinda the wrong way round. It should be the inverse of driver >>>>> load, which is 1) allocate structures 2) prep hw 3) register driver with >>>>> the world (simplified ofc). >>>>> >>>> >>>> The problem is that drm_dev_unregister() sets the device as unplugged >>>> and if drm_atomic_helper_shutdown() is called afterwards it's not >>>> allowed to touch hardware. >>>> >>>> I know it's the wrong order, but the only way to do it in the right >>>> order is to have a separate function that sets unplugged: >>>> >>>>drm_dev_unregister(); >>>>drm_atomic_helper_shutdown(); >>>>drm_dev_set_unplugged(); >>> >>> Annoying ... but yeah calling _shutdown() before we stopped userspace is >>> also not going to work. Because userspace could quickly re-enable >>> something, and then the refcounts would be all wrong again and leaking >>>
Re: [Intel-gfx] [PATCH 2/6] drm/drv: Prepare to remove drm_dev_unplug()
Den 05.02.2019 10.11, skrev Daniel Vetter: > On Mon, Feb 04, 2019 at 06:35:28PM +0100, Noralf Trønnes wrote: >> >> >> Den 04.02.2019 16.41, skrev Daniel Vetter: >>> On Sun, Feb 03, 2019 at 04:41:56PM +0100, Noralf Trønnes wrote: >>>> The only thing now that makes drm_dev_unplug() special is that it sets >>>> drm_device->unplugged. Move this code to drm_dev_unregister() so that we >>>> can remove drm_dev_unplug(). >>>> >>>> Signed-off-by: Noralf Trønnes >>>> --- >> >> [...] >> >>>> drivers/gpu/drm/drm_drv.c | 27 +++ >>>> include/drm/drm_drv.h | 10 -- >>>> 2 files changed, 19 insertions(+), 18 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c >>>> index 05bbc2b622fc..e0941200edc6 100644 >>>> --- a/drivers/gpu/drm/drm_drv.c >>>> +++ b/drivers/gpu/drm/drm_drv.c >>>> @@ -366,15 +366,6 @@ EXPORT_SYMBOL(drm_dev_exit); >>>> */ >>>> void drm_dev_unplug(struct drm_device *dev) >>>> { >>>> - /* >>>> - * After synchronizing any critical read section is guaranteed to see >>>> - * the new value of ->unplugged, and any critical section which might >>>> - * still have seen the old value of ->unplugged is guaranteed to have >>>> - * finished. >>>> - */ >>>> - dev->unplugged = true; >>>> - synchronize_srcu(_unplug_srcu); >>>> - >>>>drm_dev_unregister(dev); >>>>drm_dev_put(dev); >>>> } >>>> @@ -832,11 +823,14 @@ EXPORT_SYMBOL(drm_dev_register); >>>> * drm_dev_register() but does not deallocate the device. The caller must >>>> call >>>> * drm_dev_put() to drop their final reference. >>>> * >>>> - * A special form of unregistering for hotpluggable devices is >>>> drm_dev_unplug(), >>>> - * which can be called while there are still open users of @dev. >>>> + * This function can be called while there are still open users of @dev >>>> as long >>>> + * as the driver protects its device resources using drm_dev_enter() and >>>> + * drm_dev_exit(). >>>> * >>>> * This should be called first in the device teardown code to make sure >>>> - * userspace can't access the device instance any more. >>>> + * userspace can't access the device instance any more. Drivers that >>>> support >>>> + * device unplug will probably want to call drm_atomic_helper_shutdown() >>>> first >>> >>> Read once more with a bit more coffee, spotted this: >>> >>> s/first/afterwards/ - shutting down the hw before we've taken it away from >>> userspace is kinda the wrong way round. It should be the inverse of driver >>> load, which is 1) allocate structures 2) prep hw 3) register driver with >>> the world (simplified ofc). >>> >> >> The problem is that drm_dev_unregister() sets the device as unplugged >> and if drm_atomic_helper_shutdown() is called afterwards it's not >> allowed to touch hardware. >> >> I know it's the wrong order, but the only way to do it in the right >> order is to have a separate function that sets unplugged: >> >> drm_dev_unregister(); >> drm_atomic_helper_shutdown(); >> drm_dev_set_unplugged(); > > Annoying ... but yeah calling _shutdown() before we stopped userspace is > also not going to work. Because userspace could quickly re-enable > something, and then the refcounts would be all wrong again and leaking > objects. > What happens with a USB device that is unplugged with open userspace, will that leak objects? > I get a bit the feeling we're over-optimizing here with trying to devm-ize > drm_dev_register. Just getting drm_device correctly devm-ized is a big > step forward already, and will open up a lot of TODO items across a lot of > drivers. E.g. we could add a drm_dev_kzalloc, for allocating all the drm_* > structs, which gets released together with drm_device. I think that's a > much clearer path forward, I think we all agree that getting the kfree out > of the driver codes is a good thing, and it would allow us to do this > correctly. > > Then once we have that and rolled out to a few drivers we can reconsider > the entire unregister/shutdown gordian knot here. Atm I just have no idea > how to do this properly :-/ > > Thoughts, other ideas? > Yeah, I'
Re: [Intel-gfx] [PATCH 2/6] drm/drv: Prepare to remove drm_dev_unplug()
Den 04.02.2019 16.41, skrev Daniel Vetter: > On Sun, Feb 03, 2019 at 04:41:56PM +0100, Noralf Trønnes wrote: >> The only thing now that makes drm_dev_unplug() special is that it sets >> drm_device->unplugged. Move this code to drm_dev_unregister() so that we >> can remove drm_dev_unplug(). >> >> Signed-off-by: Noralf Trønnes >> --- [...] >> drivers/gpu/drm/drm_drv.c | 27 +++ >> include/drm/drm_drv.h | 10 -- >> 2 files changed, 19 insertions(+), 18 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c >> index 05bbc2b622fc..e0941200edc6 100644 >> --- a/drivers/gpu/drm/drm_drv.c >> +++ b/drivers/gpu/drm/drm_drv.c >> @@ -366,15 +366,6 @@ EXPORT_SYMBOL(drm_dev_exit); >> */ >> void drm_dev_unplug(struct drm_device *dev) >> { >> -/* >> - * After synchronizing any critical read section is guaranteed to see >> - * the new value of ->unplugged, and any critical section which might >> - * still have seen the old value of ->unplugged is guaranteed to have >> - * finished. >> - */ >> -dev->unplugged = true; >> -synchronize_srcu(_unplug_srcu); >> - >> drm_dev_unregister(dev); >> drm_dev_put(dev); >> } >> @@ -832,11 +823,14 @@ EXPORT_SYMBOL(drm_dev_register); >> * drm_dev_register() but does not deallocate the device. The caller must >> call >> * drm_dev_put() to drop their final reference. >> * >> - * A special form of unregistering for hotpluggable devices is >> drm_dev_unplug(), >> - * which can be called while there are still open users of @dev. >> + * This function can be called while there are still open users of @dev as >> long >> + * as the driver protects its device resources using drm_dev_enter() and >> + * drm_dev_exit(). >> * >> * This should be called first in the device teardown code to make sure >> - * userspace can't access the device instance any more. >> + * userspace can't access the device instance any more. Drivers that support >> + * device unplug will probably want to call drm_atomic_helper_shutdown() >> first > > Read once more with a bit more coffee, spotted this: > > s/first/afterwards/ - shutting down the hw before we've taken it away from > userspace is kinda the wrong way round. It should be the inverse of driver > load, which is 1) allocate structures 2) prep hw 3) register driver with > the world (simplified ofc). > The problem is that drm_dev_unregister() sets the device as unplugged and if drm_atomic_helper_shutdown() is called afterwards it's not allowed to touch hardware. I know it's the wrong order, but the only way to do it in the right order is to have a separate function that sets unplugged: drm_dev_unregister(); drm_atomic_helper_shutdown(); drm_dev_set_unplugged(); Noralf. >> + * in order to disable the hardware on regular driver module unload. >> */ >> void drm_dev_unregister(struct drm_device *dev) >> { >> @@ -845,6 +839,15 @@ void drm_dev_unregister(struct drm_device *dev) >> if (drm_core_check_feature(dev, DRIVER_LEGACY)) >> drm_lastclose(dev); >> >> +/* >> + * After synchronizing any critical read section is guaranteed to see >> + * the new value of ->unplugged, and any critical section which might >> + * still have seen the old value of ->unplugged is guaranteed to have >> + * finished. >> + */ >> +dev->unplugged = true; >> +synchronize_srcu(_unplug_srcu); >> + >> dev->registered = false; >> >> drm_client_dev_unregister(dev); >> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h >> index ca46a45a9cce..c50696c82a42 100644 >> --- a/include/drm/drm_drv.h >> +++ b/include/drm/drm_drv.h >> @@ -736,13 +736,11 @@ void drm_dev_unplug(struct drm_device *dev); >> * drm_dev_is_unplugged - is a DRM device unplugged >> * @dev: DRM device >> * >> - * This function can be called to check whether a hotpluggable is unplugged. >> - * Unplugging itself is singalled through drm_dev_unplug(). If a device is >> - * unplugged, these two functions guarantee that any store before calling >> - * drm_dev_unplug() is visible to callers of this function after it >> completes >> + * This function can be called to check whether @dev is unregistered. This >> can >> + * be used to detect that the underlying parent device is gone. > > I think it'd be good to keep the first part, and just update the
Re: [PATCH 5/6] drm/xen: Use drm_dev_unregister()
Den 04.02.2019 11.42, skrev Oleksandr Andrushchenko: > On 2/3/19 5:41 PM, Noralf Trønnes wrote: >> drm_dev_unplug() has been stripped down and is going away. Open code its >> 2 remaining function calls. >> >> Also remove the drm_dev_is_unplugged() check since this can't be true >> before drm_dev_unregister() is called which happens after the check. >> >> Cc: Oleksandr Andrushchenko >> Signed-off-by: Noralf Trønnes >> --- >> drivers/gpu/drm/xen/xen_drm_front.c | 7 ++- >> 1 file changed, 2 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/gpu/drm/xen/xen_drm_front.c >> b/drivers/gpu/drm/xen/xen_drm_front.c >> index 3e78a832d7f9..5c5eb24c6342 100644 >> --- a/drivers/gpu/drm/xen/xen_drm_front.c >> +++ b/drivers/gpu/drm/xen/xen_drm_front.c >> @@ -576,12 +576,9 @@ static void xen_drm_drv_fini(struct xen_drm_front_info >> *front_info) >> if (!dev) >> return; >> >> -/* Nothing to do if device is already unplugged */ >> -if (drm_dev_is_unplugged(dev)) >> -return; > xen_drm_drv_fini is called when the backend changes its state [1], > so I just use the check above to prevent possible race conditions here, > e.g. do not allow to run unregister code if it is already in progress > So, I think we should keep this and probably just add a comment why it is > here Ok, it's just me not reading the code closely enough. I'll put it back in the next version. Noralf. >> - >> drm_kms_helper_poll_fini(dev); >> -drm_dev_unplug(dev); >> +drm_dev_unregister(dev); >> +drm_dev_put(dev); >> >> front_info->drm_info = NULL; >> > [1] https://elixir.bootlin.com/linux/v5.0-rc5/ident/displback_disconnect > ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH 1/6] drm: Fix drm_release() and device unplug
If userspace has open fd(s) when drm_dev_unplug() is run, it will result in drm_dev_unregister() being called twice. First in drm_dev_unplug() and then later in drm_release() through the call to drm_put_dev(). Since userspace already holds a ref on drm_device through the drm_minor, it's not necessary to add extra ref counting based on no open file handles. Instead just drm_dev_put() unconditionally in drm_dev_unplug(). We now has this: - Userpace holds a ref on drm_device as long as there's open fd(s) - The driver holds a ref on drm_device as long as it's bound to the struct device When both sides are done with drm_device, it is released. Signed-off-by: Noralf Trønnes --- drivers/gpu/drm/drm_drv.c | 6 +- drivers/gpu/drm/drm_file.c | 6 ++ 2 files changed, 3 insertions(+), 9 deletions(-) diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 381581b01d48..05bbc2b622fc 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -376,11 +376,7 @@ void drm_dev_unplug(struct drm_device *dev) synchronize_srcu(_unplug_srcu); drm_dev_unregister(dev); - - mutex_lock(_global_mutex); - if (dev->open_count == 0) - drm_dev_put(dev); - mutex_unlock(_global_mutex); + drm_dev_put(dev); } EXPORT_SYMBOL(drm_dev_unplug); diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c index 46f48f245eb5..3f20f598cd7c 100644 --- a/drivers/gpu/drm/drm_file.c +++ b/drivers/gpu/drm/drm_file.c @@ -479,11 +479,9 @@ int drm_release(struct inode *inode, struct file *filp) drm_file_free(file_priv); - if (!--dev->open_count) { + if (!--dev->open_count) drm_lastclose(dev); - if (drm_dev_is_unplugged(dev)) - drm_put_dev(dev); - } + mutex_unlock(_global_mutex); drm_minor_release(minor); -- 2.20.1 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH 0/6] drm/drv: Remove drm_dev_unplug()
This series removes drm_dev_unplug() and moves the unplugged state setting to drm_dev_unregister(). All drivers will now have access to the unplugged state if they so desire. The drm_device ref handling wrt to the last fd closed after unregister have been simplified, which also fixed a double drm_dev_unregister() situation. Noralf. Noralf Trønnes (6): drm: Fix drm_release() and device unplug drm/drv: Prepare to remove drm_dev_unplug() drm/amd: Use drm_dev_unregister() drm/udl: Use drm_dev_unregister() drm/xen: Use drm_dev_unregister() drm/drv: Remove drm_dev_unplug() drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 3 +- drivers/gpu/drm/drm_drv.c | 48 - drivers/gpu/drm/drm_file.c | 6 ++-- drivers/gpu/drm/udl/udl_drv.c | 3 +- drivers/gpu/drm/xen/xen_drm_front.c | 7 ++-- include/drm/drm_drv.h | 11 +++--- 6 files changed, 27 insertions(+), 51 deletions(-) -- 2.20.1 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH 3/6] drm/amd: Use drm_dev_unregister()
drm_dev_unplug() has been stripped down and is going away. Open code its 2 remaining function calls. Cc: Alex Deucher Cc: Christian König Cc: David (ChunMing) Zhou Signed-off-by: Noralf Trønnes --- drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c index a1bb3773087b..1265fc07120a 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c @@ -970,7 +970,8 @@ amdgpu_pci_remove(struct pci_dev *pdev) struct drm_device *dev = pci_get_drvdata(pdev); DRM_ERROR("Device removal is currently not supported outside of fbcon\n"); - drm_dev_unplug(dev); + drm_dev_unregister(dev); + drm_dev_put(dev); pci_disable_device(pdev); pci_set_drvdata(pdev, NULL); } -- 2.20.1 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH 2/6] drm/drv: Prepare to remove drm_dev_unplug()
The only thing now that makes drm_dev_unplug() special is that it sets drm_device->unplugged. Move this code to drm_dev_unregister() so that we can remove drm_dev_unplug(). Signed-off-by: Noralf Trønnes --- Maybe s/unplugged/unregistered/ ? I looked at drm_device->registered, but using that would mean that drm_dev_is_unplugged() would return before drm_device is registered. And given that its current purpose is to prevent race against connector registration, I stayed away from it. Noralf. drivers/gpu/drm/drm_drv.c | 27 +++ include/drm/drm_drv.h | 10 -- 2 files changed, 19 insertions(+), 18 deletions(-) diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 05bbc2b622fc..e0941200edc6 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -366,15 +366,6 @@ EXPORT_SYMBOL(drm_dev_exit); */ void drm_dev_unplug(struct drm_device *dev) { - /* -* After synchronizing any critical read section is guaranteed to see -* the new value of ->unplugged, and any critical section which might -* still have seen the old value of ->unplugged is guaranteed to have -* finished. -*/ - dev->unplugged = true; - synchronize_srcu(_unplug_srcu); - drm_dev_unregister(dev); drm_dev_put(dev); } @@ -832,11 +823,14 @@ EXPORT_SYMBOL(drm_dev_register); * drm_dev_register() but does not deallocate the device. The caller must call * drm_dev_put() to drop their final reference. * - * A special form of unregistering for hotpluggable devices is drm_dev_unplug(), - * which can be called while there are still open users of @dev. + * This function can be called while there are still open users of @dev as long + * as the driver protects its device resources using drm_dev_enter() and + * drm_dev_exit(). * * This should be called first in the device teardown code to make sure - * userspace can't access the device instance any more. + * userspace can't access the device instance any more. Drivers that support + * device unplug will probably want to call drm_atomic_helper_shutdown() first + * in order to disable the hardware on regular driver module unload. */ void drm_dev_unregister(struct drm_device *dev) { @@ -845,6 +839,15 @@ void drm_dev_unregister(struct drm_device *dev) if (drm_core_check_feature(dev, DRIVER_LEGACY)) drm_lastclose(dev); + /* +* After synchronizing any critical read section is guaranteed to see +* the new value of ->unplugged, and any critical section which might +* still have seen the old value of ->unplugged is guaranteed to have +* finished. +*/ + dev->unplugged = true; + synchronize_srcu(_unplug_srcu); + dev->registered = false; drm_client_dev_unregister(dev); diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h index ca46a45a9cce..c50696c82a42 100644 --- a/include/drm/drm_drv.h +++ b/include/drm/drm_drv.h @@ -736,13 +736,11 @@ void drm_dev_unplug(struct drm_device *dev); * drm_dev_is_unplugged - is a DRM device unplugged * @dev: DRM device * - * This function can be called to check whether a hotpluggable is unplugged. - * Unplugging itself is singalled through drm_dev_unplug(). If a device is - * unplugged, these two functions guarantee that any store before calling - * drm_dev_unplug() is visible to callers of this function after it completes + * This function can be called to check whether @dev is unregistered. This can + * be used to detect that the underlying parent device is gone. * - * WARNING: This function fundamentally races against drm_dev_unplug(). It is - * recommended that drivers instead use the underlying drm_dev_enter() and + * WARNING: This function fundamentally races against drm_dev_unregister(). It + * is recommended that drivers instead use the underlying drm_dev_enter() and * drm_dev_exit() function pairs. */ static inline bool drm_dev_is_unplugged(struct drm_device *dev) -- 2.20.1 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH 6/6] drm/drv: Remove drm_dev_unplug()
There are no users left. Signed-off-by: Noralf Trønnes --- drivers/gpu/drm/drm_drv.c | 17 - include/drm/drm_drv.h | 1 - 2 files changed, 18 deletions(-) diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index e0941200edc6..87210d4a9e53 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -354,23 +354,6 @@ void drm_dev_exit(int idx) } EXPORT_SYMBOL(drm_dev_exit); -/** - * drm_dev_unplug - unplug a DRM device - * @dev: DRM device - * - * This unplugs a hotpluggable DRM device, which makes it inaccessible to - * userspace operations. Entry-points can use drm_dev_enter() and - * drm_dev_exit() to protect device resources in a race free manner. This - * essentially unregisters the device like drm_dev_unregister(), but can be - * called while there are still open users of @dev. - */ -void drm_dev_unplug(struct drm_device *dev) -{ - drm_dev_unregister(dev); - drm_dev_put(dev); -} -EXPORT_SYMBOL(drm_dev_unplug); - /* * DRM internal mount * We want to be able to allocate our own "struct address_space" to control diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h index c50696c82a42..b8765a6fc092 100644 --- a/include/drm/drm_drv.h +++ b/include/drm/drm_drv.h @@ -730,7 +730,6 @@ void drm_dev_put(struct drm_device *dev); void drm_put_dev(struct drm_device *dev); bool drm_dev_enter(struct drm_device *dev, int *idx); void drm_dev_exit(int idx); -void drm_dev_unplug(struct drm_device *dev); /** * drm_dev_is_unplugged - is a DRM device unplugged -- 2.20.1 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH 4/6] drm/udl: Use drm_dev_unregister()
drm_dev_unplug() has been stripped down and is going away. Open code its 2 remaining function calls. Cc: Dave Airlie Cc: Sean Paul Signed-off-by: Noralf Trønnes --- drivers/gpu/drm/udl/udl_drv.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c index 22cd2d13e272..063e8e1e2641 100644 --- a/drivers/gpu/drm/udl/udl_drv.c +++ b/drivers/gpu/drm/udl/udl_drv.c @@ -106,7 +106,8 @@ static void udl_usb_disconnect(struct usb_interface *interface) drm_kms_helper_poll_disable(dev); udl_fbdev_unplug(dev); udl_drop_usb(dev); - drm_dev_unplug(dev); + drm_dev_unregister(dev); + drm_dev_put(dev); } /* -- 2.20.1 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH 5/6] drm/xen: Use drm_dev_unregister()
drm_dev_unplug() has been stripped down and is going away. Open code its 2 remaining function calls. Also remove the drm_dev_is_unplugged() check since this can't be true before drm_dev_unregister() is called which happens after the check. Cc: Oleksandr Andrushchenko Signed-off-by: Noralf Trønnes --- drivers/gpu/drm/xen/xen_drm_front.c | 7 ++- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/xen/xen_drm_front.c b/drivers/gpu/drm/xen/xen_drm_front.c index 3e78a832d7f9..5c5eb24c6342 100644 --- a/drivers/gpu/drm/xen/xen_drm_front.c +++ b/drivers/gpu/drm/xen/xen_drm_front.c @@ -576,12 +576,9 @@ static void xen_drm_drv_fini(struct xen_drm_front_info *front_info) if (!dev) return; - /* Nothing to do if device is already unplugged */ - if (drm_dev_is_unplugged(dev)) - return; - drm_kms_helper_poll_fini(dev); - drm_dev_unplug(dev); + drm_dev_unregister(dev); + drm_dev_put(dev); front_info->drm_info = NULL; -- 2.20.1 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 00/23] drm: Eliminate plane->fb/crtc usage for atomic drivers
Den 22.03.2018 19.49, skrev Ville Syrjälä: On Thu, Mar 22, 2018 at 05:51:35PM +0100, Noralf Trønnes wrote: tinydrm is also using plane->fb: $ grep -r "plane\.fb" drivers/gpu/drm/tinydrm/ drivers/gpu/drm/tinydrm/repaper.c: if (tdev->pipe.plane.fb != fb) drivers/gpu/drm/tinydrm/mipi-dbi.c: if (tdev->pipe.plane.fb != fb) drivers/gpu/drm/tinydrm/mipi-dbi.c: struct drm_framebuffer *fb = mipi->tinydrm.pipe.plane.fb; Oh dear, and naturally it's the most annoying one of the bunch. So we want to take the plane lock in the fb.dirty() hook to look at the fb, but mipi-dbi.c calls that directly from the bowels of its .atomic_enable() hook. So that would deadlock pretty neatly if the commit happens while already holding the lock. So we'd either need to plumb an acquire context into fb.dirty(), or maybe have tinydrm provide a lower level lockless dirty() hook that gets called by both (there are just too many layers in this particular cake to immediately see where such a hook would be best placed). Or maybe there's some other solution I didn't think of. Looking at this I'm also left wondering how this is supposed handle fb.dirty() getting called concurrently with a modeset. The dirty_mutex only seems to offer any protection for fb.dirty() against another fb.dirty()... I have been waiting for the 'page-flip with damage'[1] work to come to fruition so I could handle all flushing during atomic commit. The idea is to use the same damage rect for a generic dirtyfb callback. Maybe a temporary tinydrm specific solution is possible until that work lands, but I don't know enough about how to implement such a dirtyfb callback. I imagine it could look something like this: struct tinydrm_device { + struct drm_clip_rect dirtyfb_rect; }; static int tinydrm_fb_dirty(struct drm_framebuffer *fb, struct drm_file *file_priv, unsigned int flags, unsigned int color, struct drm_clip_rect *clips, unsigned int num_clips) { struct tinydrm_device *tdev = fb->dev->dev_private; struct drm_framebuffer *planefb; /* Grab whatever lock needed to check this */ planefb = tdev->pipe.plane.state->fb; /* fbdev can flush even when we're not interested */ if (fb != planefb) return 0; /* Protect dirtyfb_rect with a lock */ tinydrm_merge_clips(>dirty_rectfb, clips, num_clips, flags, fb->width, fb->height); /* * Somehow do an atomic commit that results in the atomic update * callback being called */ ret = drm_atomic_commit(state); ... } static void mipi_dbi_flush(struct drm_framebuffer *fb, struct drm_clip_rect *clip) { /* Flush out framebuffer */ } void mipi_dbi_pipe_update(struct drm_simple_display_pipe *pipe, struct drm_plane_state *old_state) { struct tinydrm_device *tdev = pipe_to_tinydrm(pipe); struct mipi_dbi *mipi = mipi_dbi_from_tinydrm(tdev); struct drm_framebuffer *fb = pipe->plane.state->fb; struct drm_crtc *crtc = >pipe.crtc; if (!fb || (fb == old_state->fb && !tdev->dirty_rect.x2)) goto out_vblank; /* Don't flush if the controller isn't initialized yet */ if (!mipi->enabled) goto out_vblank; if (fb != old_state->fb) { /* Page flip or new, flush all */ mipi_dbi_flush(fb, NULL); } else { /* dirtyfb ioctl */ mipi_dbi_flush(fb, >dirtyfb_rect); memset(>dirtyfb_rect, 0, sizeof(tdev->dirtyfb_rect)); } out_vblank: if (crtc->state->event) { spin_lock_irq(>dev->event_lock); drm_crtc_send_vblank_event(crtc, crtc->state->event); spin_unlock_irq(>dev->event_lock); crtc->state->event = NULL; } } This is called from the driver pipe enable callback after the controller has been initialized: void mipi_dbi_pipe_enable(struct drm_simple_display_pipe *pipe, struct drm_crtc_state *crtc_state) { struct tinydrm_device *tdev = pipe_to_tinydrm(pipe); struct mipi_dbi *mipi = mipi_dbi_from_tinydrm(tdev); - struct drm_framebuffer *fb = pipe->plane.fb; + struct drm_framebuffer *fb = pipe->plane.state->fb; DRM_DEBUG_KMS("\n"); mipi->enabled = true; - if (fb) - fb->funcs->dirty(fb, NULL, 0, 0, NULL, 0); + mipi_dbi_flush(fb, NULL); tinydrm_enable_backlight(mipi->backlight); } I can make patches if this makes any sense and you can help me with the dirtyfb implementation. Noralf. [1] https://lists.freedesktop.org/archives/dri-devel/2017-December/161003.html ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 00/23] drm: Eliminate plane->fb/crtc usage for atomic drivers
Den 22.03.2018 16.22, skrev Ville Syrjala: From: Ville SyrjäläI really just wanted to fix i915 to re-enable its planes afer load detection (a two line patch). This is what I actually ended up with after I ran into a framebuffer refcount leak with said two line patch. I've tested this on a few i915 boxes and so far it's looking good. Everything else is just compile tested. Entire series available here: git://github.com/vsyrjala/linux.git plane_fb_crtc_nuke Cc: Alex Deucher Cc: amd-gfx@lists.freedesktop.org Cc: Benjamin Gaignard Cc: Boris Brezillon Cc: ch...@chris-wilson.co.uk Cc: "Christian König" Cc: Daniel Vetter Cc: Dave Airlie Cc: David Airlie Cc: "David (ChunMing) Zhou" Cc: Eric Anholt Cc: freedr...@lists.freedesktop.org Cc: Gerd Hoffmann Cc: Harry Wentland Cc: Inki Dae Cc: Joonyoung Shim Cc: Kyungmin Park Cc: linux-arm-...@vger.kernel.org Cc: Maarten Lankhorst Cc: martin.pe...@free.fr Cc: Rob Clark Cc: Seung-Woo Kim Cc: Shawn Guo Cc: Sinclair Yeh Cc: Thomas Hellstrom Cc: Vincent Abriou Cc: virtualizat...@lists.linux-foundation.org Cc: VMware Graphics Ville Syrjälä (23): Revert "drm/atomic-helper: Fix leak in disable_all" drm/atomic-helper: Make drm_atomic_helper_disable_all() update the plane->fb pointers drm: Clear crtc->primary->crtc when disabling the crtc via setcrtc() drm/atomic-helper: WARN if legacy plane fb pointers are bogus when committing duplicated state drm: Add local 'plane' variable for primary/cursor planes drm: Adjust whitespace for legibility drm: Make the fb refcount handover less magic drm: Use plane->state->fb over plane->fb drm/i915: Stop consulting plane->fb drm/msm: Stop consulting plane->fb drm/sti: Stop consulting plane->fb drm/vmwgfx: Stop consulting plane->fb drm/zte: Stop consulting plane->fb drm/atmel-hlcdc: Stop using plane->fb drm: Stop updating plane->crtc/fb/old_fb on atomic drivers drm/amdgpu/dc: Stop updating plane->fb drm/i915: Stop updating plane->fb/crtc drm/exynos: Stop updating plane->crtc drm/msm: Stop updating plane->fb/crtc drm/virtio: Stop updating plane->fb drm/vc4: Stop updating plane->fb/crtc drm/i915: Restore planes after load detection drm/i915: Make force_load_detect effective even w/ DMI quirks/hotplug tinydrm is also using plane->fb: $ grep -r "plane\.fb" drivers/gpu/drm/tinydrm/ drivers/gpu/drm/tinydrm/repaper.c: if (tdev->pipe.plane.fb != fb) drivers/gpu/drm/tinydrm/mipi-dbi.c: if (tdev->pipe.plane.fb != fb) drivers/gpu/drm/tinydrm/mipi-dbi.c: struct drm_framebuffer *fb = mipi->tinydrm.pipe.plane.fb; drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c: pipe->plane.fb = fb; drivers/gpu/drm/tinydrm/ili9225.c: if (tdev->pipe.plane.fb != fb) drivers/gpu/drm/tinydrm/st7586.c: if (tdev->pipe.plane.fb != fb) Noralf. drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 - drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c | 12 +--- drivers/gpu/drm/drm_atomic.c | 55 ++-- drivers/gpu/drm/drm_atomic_helper.c | 79 ++- drivers/gpu/drm/drm_crtc.c| 51 ++- drivers/gpu/drm/drm_fb_helper.c | 7 -- drivers/gpu/drm/drm_framebuffer.c | 5 -- drivers/gpu/drm/drm_plane.c | 64 +++--- drivers/gpu/drm/drm_plane_helper.c| 4 +- drivers/gpu/drm/exynos/exynos_drm_plane.c | 2 - drivers/gpu/drm/i915/intel_crt.c | 6 ++ drivers/gpu/drm/i915/intel_display.c | 9 +-- drivers/gpu/drm/i915/intel_fbdev.c| 2 +- drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c | 3 +- drivers/gpu/drm/msm/disp/mdp4/mdp4_plane.c| 2 - drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c| 2 - drivers/gpu/drm/sti/sti_plane.c | 9 +-- drivers/gpu/drm/vc4/vc4_crtc.c| 3 - drivers/gpu/drm/virtio/virtgpu_display.c | 2 - drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 6 +- drivers/gpu/drm/zte/zx_vou.c | 2 +- include/drm/drm_atomic.h | 3 - 22 files changed, 143 insertions(+), 187 deletions(-) ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org
Re: [PATCH libdrm] drm: Remove create_handle() drm_framebuffer "virtual".
Den 09.08.2017 01.42, skrev Joe Kniss: Because all drivers currently use gem objects for framebuffer planes, the virtual create_handle() is not required. This change adds a struct drm_gem_object *gems[4] field to drm_framebuffer and removes create_handle() function pointer from drm_framebuffer_funcs. The corresponding *_create_handle() function is removed from each driver. In many cases this change eliminates a struct *_framebuffer object, as the only need for the derived struct is the addition of the gem object pointer. TESTED: compiled: allyesconfig ARCH=x86,arm platforms:i915, rockchip Signed-off-by: Joe Kniss--- Hi Joe, I'm also looking into adding gem objs to drm_framebuffer in this patch: [PATCH v2 01/22] drm: Add GEM backed framebuffer library https://lists.freedesktop.org/archives/dri-devel/2017-August/149782.html [...] diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c index ade319d10e70..f5f011b910b1 100644 --- a/drivers/gpu/drm/drm_fb_cma_helper.c +++ b/drivers/gpu/drm/drm_fb_cma_helper.c @@ -31,14 +31,9 @@ #define DEFAULT_FBDEFIO_DELAY_MS 50 -struct drm_fb_cma { - struct drm_framebuffer fb; - struct drm_gem_cma_object *obj[4]; -}; - struct drm_fbdev_cma { struct drm_fb_helperfb_helper; - struct drm_fb_cma *fb; + struct drm_framebuffer *fb; This fb pointer isn't necessary, since fb_helper already has one. Noralf. const struct drm_framebuffer_funcs *fb_funcs; }; @@ -72,7 +67,6 @@ struct drm_fbdev_cma { * * static struct drm_framebuffer_funcs driver_fb_funcs = { * .destroy = drm_fb_cma_destroy, - * .create_handle = drm_fb_cma_create_handle, * .dirty = driver_fb_dirty, * }; * @@ -90,67 +84,50 @@ static inline struct drm_fbdev_cma *to_fbdev_cma(struct drm_fb_helper *helper) return container_of(helper, struct drm_fbdev_cma, fb_helper); } -static inline struct drm_fb_cma *to_fb_cma(struct drm_framebuffer *fb) -{ - return container_of(fb, struct drm_fb_cma, fb); -} - void drm_fb_cma_destroy(struct drm_framebuffer *fb) { - struct drm_fb_cma *fb_cma = to_fb_cma(fb); int i; for (i = 0; i < 4; i++) { - if (fb_cma->obj[i]) - drm_gem_object_put_unlocked(_cma->obj[i]->base); + if (fb->gem_objs[i]) + drm_gem_object_put_unlocked(fb->gem_objs[i]); } drm_framebuffer_cleanup(fb); - kfree(fb_cma); + kfree(fb); } EXPORT_SYMBOL(drm_fb_cma_destroy); -int drm_fb_cma_create_handle(struct drm_framebuffer *fb, - struct drm_file *file_priv, unsigned int *handle) -{ - struct drm_fb_cma *fb_cma = to_fb_cma(fb); - - return drm_gem_handle_create(file_priv, - _cma->obj[0]->base, handle); -} -EXPORT_SYMBOL(drm_fb_cma_create_handle); - static struct drm_framebuffer_funcs drm_fb_cma_funcs = { .destroy= drm_fb_cma_destroy, - .create_handle = drm_fb_cma_create_handle, }; -static struct drm_fb_cma *drm_fb_cma_alloc(struct drm_device *dev, +static struct drm_framebuffer *drm_fb_cma_alloc(struct drm_device *dev, const struct drm_mode_fb_cmd2 *mode_cmd, struct drm_gem_cma_object **obj, unsigned int num_planes, const struct drm_framebuffer_funcs *funcs) { - struct drm_fb_cma *fb_cma; + struct drm_framebuffer *fb; int ret; int i; - fb_cma = kzalloc(sizeof(*fb_cma), GFP_KERNEL); - if (!fb_cma) + fb = kzalloc(sizeof(*fb), GFP_KERNEL); + if (!fb) return ERR_PTR(-ENOMEM); - drm_helper_mode_fill_fb_struct(dev, _cma->fb, mode_cmd); + drm_helper_mode_fill_fb_struct(dev, fb, mode_cmd); for (i = 0; i < num_planes; i++) - fb_cma->obj[i] = obj[i]; + fb->gem_objs[i] = [i]->base; - ret = drm_framebuffer_init(dev, _cma->fb, funcs); + ret = drm_framebuffer_init(dev, fb, funcs); if (ret) { dev_err(dev->dev, "Failed to initialize framebuffer: %d\n", ret); - kfree(fb_cma); + kfree(fb); return ERR_PTR(ret); } - return fb_cma; + return fb; } /** @@ -171,7 +148,7 @@ struct drm_framebuffer *drm_fb_cma_create_with_funcs(struct drm_device *dev, const struct drm_framebuffer_funcs *funcs) { const struct drm_format_info *info; - struct drm_fb_cma *fb_cma; + struct drm_framebuffer *fb; struct drm_gem_cma_object *objs[4]; struct drm_gem_object *obj; int ret; @@ -205,13 +182,13 @@ struct drm_framebuffer *drm_fb_cma_create_with_funcs(struct drm_device *dev, objs[i] = to_drm_gem_cma_obj(obj); } - fb_cma = drm_fb_cma_alloc(dev, mode_cmd, objs, i, funcs); - if (IS_ERR(fb_cma)) { - ret =