[PATCH 3/4] drm/udl: Kill pending URBs at suspend and disconnect
Thomas Zimmermann
tzimmermann at suse.de
Tue Aug 16 14:01:34 UTC 2022
Hi Takashi
Am 16.08.22 um 15:55 schrieb Takashi Iwai:
> On Tue, 09 Aug 2022 11:19:30 +0200,
> Takashi Iwai wrote:
>>
>> On Tue, 09 Aug 2022 11:13:46 +0200,
>> Thomas Zimmermann wrote:
>>>
>>> Hi
>>>
>>> Am 09.08.22 um 11:03 schrieb Takashi Iwai:
>>>> On Tue, 09 Aug 2022 09:41:19 +0200,
>>>> Thomas Zimmermann wrote:
>>>>>
>>>>> Hi
>>>>>
>>>>> Am 09.08.22 um 09:15 schrieb Takashi Iwai:
>>>>>> On Tue, 09 Aug 2022 09:13:16 +0200,
>>>>>> Thomas Zimmermann wrote:
>>>>>>>
>>>>>>> Hi
>>>>>>>
>>>>>>> Am 04.08.22 um 09:58 schrieb Takashi Iwai:
>>>>>>>> At both suspend and disconnect, we should rather cancel the pending
>>>>>>>> URBs immediately. For the suspend case, the display will be turned
>>>>>>>> off, so it makes no sense to process the rendering. And for the
>>>>>>>> disconnect case, the device may be no longer accessible, hence we
>>>>>>>> shouldn't do any submission.
>>>>>>>>
>>>>>>>> Tested-by: Thomas Zimmermann <tzimmermann at suse.de>
>>>>>>>> Signed-off-by: Takashi Iwai <tiwai at suse.de>
>>>>>>>> ---
>>>>>>>> drivers/gpu/drm/udl/udl_drv.h | 2 ++
>>>>>>>> drivers/gpu/drm/udl/udl_main.c | 25 ++++++++++++++++++++++---
>>>>>>>> drivers/gpu/drm/udl/udl_modeset.c | 2 ++
>>>>>>>> 3 files changed, 26 insertions(+), 3 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
>>>>>>>> index f01e50c5b7b7..28aaf75d71cf 100644
>>>>>>>> --- a/drivers/gpu/drm/udl/udl_drv.h
>>>>>>>> +++ b/drivers/gpu/drm/udl/udl_drv.h
>>>>>>>> @@ -39,6 +39,7 @@ struct urb_node {
>>>>>>>> struct urb_list {
>>>>>>>> struct list_head list;
>>>>>>>> + struct list_head in_flight;
>>>>>>>> spinlock_t lock;
>>>>>>>> wait_queue_head_t sleep;
>>>>>>>> int available;
>>>>>>>> @@ -84,6 +85,7 @@ static inline struct urb *udl_get_urb(struct drm_device *dev)
>>>>>>>> int udl_submit_urb(struct drm_device *dev, struct urb *urb,
>>>>>>>> size_t len);
>>>>>>>> int udl_sync_pending_urbs(struct drm_device *dev);
>>>>>>>> +void udl_kill_pending_urbs(struct drm_device *dev);
>>>>>>>> void udl_urb_completion(struct urb *urb);
>>>>>>>> int udl_init(struct udl_device *udl);
>>>>>>>> diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
>>>>>>>> index 93615648414b..47204b7eb10e 100644
>>>>>>>> --- a/drivers/gpu/drm/udl/udl_main.c
>>>>>>>> +++ b/drivers/gpu/drm/udl/udl_main.c
>>>>>>>> @@ -135,7 +135,7 @@ void udl_urb_completion(struct urb *urb)
>>>>>>>> urb->transfer_buffer_length = udl->urbs.size; /* reset to actual */
>>>>>>>> spin_lock_irqsave(&udl->urbs.lock, flags);
>>>>>>>> - list_add_tail(&unode->entry, &udl->urbs.list);
>>>>>>>> + list_move(&unode->entry, &udl->urbs.list);
>>>>>>>> udl->urbs.available++;
>>>>>>>> spin_unlock_irqrestore(&udl->urbs.lock, flags);
>>>>>>>> @@ -180,6 +180,7 @@ static int udl_alloc_urb_list(struct
>>>>>>>> drm_device *dev, int count, size_t size)
>>>>>>>> retry:
>>>>>>>> udl->urbs.size = size;
>>>>>>>> INIT_LIST_HEAD(&udl->urbs.list);
>>>>>>>> + INIT_LIST_HEAD(&udl->urbs.in_flight);
>>>>>>>> init_waitqueue_head(&udl->urbs.sleep);
>>>>>>>> udl->urbs.count = 0;
>>>>>>>> @@ -246,7 +247,7 @@ struct urb *udl_get_urb_timeout(struct drm_device *dev, long timeout)
>>>>>>>> }
>>>>>>>> unode = list_first_entry(&udl->urbs.list, struct urb_node,
>>>>>>>> entry);
>>>>>>>> - list_del_init(&unode->entry);
>>>>>>>> + list_move(&unode->entry, &udl->urbs.in_flight);
>>>>>>>> udl->urbs.available--;
>>>>>>>> unlock:
>>>>>>>> @@ -279,7 +280,7 @@ int udl_sync_pending_urbs(struct drm_device *dev)
>>>>>>>> spin_lock_irq(&udl->urbs.lock);
>>>>>>>> /* 2 seconds as a sane timeout */
>>>>>>>> if (!wait_event_lock_irq_timeout(udl->urbs.sleep,
>>>>>>>> - udl->urbs.available == udl->urbs.count,
>>>>>>>> + list_empty(&udl->urbs.in_flight),
>>>>>>>> udl->urbs.lock,
>>>>>>>> msecs_to_jiffies(2000)))
>>>>>>>> ret = -ETIMEDOUT;
>>>>>>>> @@ -287,6 +288,23 @@ int udl_sync_pending_urbs(struct drm_device *dev)
>>>>>>>> return ret;
>>>>>>>> }
>>>>>>>> +/* kill pending URBs */
>>>>>>>> +void udl_kill_pending_urbs(struct drm_device *dev)
>>>>>>>> +{
>>>>>>>> + struct udl_device *udl = to_udl(dev);
>>>>>>>> + struct urb_node *unode;
>>>>>>>> +
>>>>>>>> + spin_lock_irq(&udl->urbs.lock);
>>>>>>>> + while (!list_empty(&udl->urbs.in_flight)) {
>>>>>>>> + unode = list_first_entry(&udl->urbs.in_flight,
>>>>>>>> + struct urb_node, entry);
>>>>>>>> + spin_unlock_irq(&udl->urbs.lock);
>>>>>>>> + usb_kill_urb(unode->urb);
>>>>>>>> + spin_lock_irq(&udl->urbs.lock);
>>>>>>>> + }
>>>>>>>> + spin_unlock_irq(&udl->urbs.lock);
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> int udl_init(struct udl_device *udl)
>>>>>>>> {
>>>>>>>> struct drm_device *dev = &udl->drm;
>>>>>>>> @@ -335,6 +353,7 @@ int udl_drop_usb(struct drm_device *dev)
>>>>>>>> {
>>>>>>>> struct udl_device *udl = to_udl(dev);
>>>>>>>> + udl_kill_pending_urbs(dev);
>>>>>>>> udl_free_urb_list(dev);
>>>>>>>> put_device(udl->dmadev);
>>>>>>>> udl->dmadev = NULL;
>>>>>>>> diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
>>>>>>>> index 50025606b6ad..169110d8fc2e 100644
>>>>>>>> --- a/drivers/gpu/drm/udl/udl_modeset.c
>>>>>>>> +++ b/drivers/gpu/drm/udl/udl_modeset.c
>>>>>>>> @@ -397,6 +397,8 @@ udl_simple_display_pipe_disable(struct drm_simple_display_pipe *pipe)
>>>>>>>> struct urb *urb;
>>>>>>>> char *buf;
>>>>>>>> + udl_kill_pending_urbs(dev);
>>>>>>>> +
>>>>>>>
>>>>>>> I already reviewed the patchset, but I have another comment. I think
>>>>>>> we should only kill urbs from within the suspend handler. Same for the
>>>>>>> call to the URB-sync function in patch 2.
>>>>>>>
>>>>>>> This disable function is part of the regular modeset path. It's
>>>>>>> probably not appropriate to outright remove pending URBs here. This
>>>>>>> can lead to failed modesets, which would have succeeded otherwise.
>>>>>>
>>>>>> Well, the device shall be turned off right after that point, so the
>>>>>> all pending rendering makes little sense, no?
>>>>>
>>>>> udl_simple_display_pipe_disable() only disables the display, but not
>>>>> the device. The kill operation here could potentially kill some valid
>>>>> modeset operation that was still going on. And who knows what the
>>>>> device state is after that.
>>>>
>>>> But udl_simple_display_pipe_disable() invokes UDL_BLANK_MODE_POWERDOWN
>>>> command right after the place I've put udl_kill_pending_urbs(). So it
>>>> shall blank / turn off the power (of the device, as it has a single
>>>> output). And the URB completion doesn't do any error handling but
>>>> just re-links URB chain and wakes up the queue. So killing a pending
>>>> URB would nothing but canceling the in-flight URBs, and there should
>>>> be no disturbance to the modeset operation itself, as the screen will
>>>> be blanked immediately.
>>>
>>> The blank mode is essentially DPMS. It's unrelated to the device's
>>> display mode.
>>
>> The function invokes the UDL_BLANK_MODE_POWERDOWN command; that will
>> discard the whole rendered picture. And, the counterpart,
>> udl_simple_display_pipe_enable(), re-initializes the mode fully from
>> the scratch again.
>> So what's the point to continue rendering that is immediately cleared
>> (from the screen and from the device state)? Killing pending URBs
>> doesn't influence on the internal (modeset) state of the driver.
>
> In anyway, this patchset seems problematic around the disconnection,
> and maybe this particular one is no much improvement, better to drop
> for now.
>
> I'll resubmit the v2 patch set including your resume fixes later.
I already merged the patches before seeing the discussion on the rsp bug
report. If you submit an update, maybe you can do so with Fixes tags?
Best regards
Thomas
>
>
> thanks,
>
> Takashi
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 840 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20220816/ce1c3e00/attachment-0001.sig>
More information about the dri-devel
mailing list