Re: [PATCH 16/21] drm/radeon: Use drm_fb_helper_fill_info

[Noralf Trønnes]

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())

[Noralf Trønnes]

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

[Noralf Trønnes]

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())

[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(-)

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

[Noralf Trønnes]

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()

[Noralf Trønnes]

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()

[Noralf Trønnes]

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()

[Noralf Trønnes]

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()

[Noralf Trønnes]

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()

[Noralf Trønnes]

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

[Noralf Trønnes]

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()

[Noralf Trønnes]

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()

[Noralf Trønnes]

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()

[Noralf Trønnes]

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()

[Noralf Trønnes]

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()

[Noralf Trønnes]

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()

[Noralf Trønnes]

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

[Noralf Trønnes]

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

[Noralf Trønnes]

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".

[Noralf Trønnes]

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 =