From: Hans de Goede <hdegoede@redhat.com>
To: Daniel Vetter <daniel.vetter@ffwll.ch>,
DRI Development <dri-devel@lists.freedesktop.org>
Cc: Daniel Vetter <daniel.vetter@intel.com>,
Intel Graphics Development <intel-gfx@lists.freedesktop.org>,
m.felsch@pengutronix.de
Subject: Re: [PATCH 45/51] drm/gm12u320: Simplify upload work
Date: Thu, 27 Feb 2020 20:04:07 +0100 [thread overview]
Message-ID: <39a5091b-cf17-0cd9-cf43-a7a8b3cd7b78@redhat.com> (raw)
In-Reply-To: <20200227181522.2711142-46-daniel.vetter@ffwll.ch>
Hi,
On 2/27/20 7:15 PM, Daniel Vetter wrote:
> Instead of having a work item that never stops (which really should be
> a kthread), with a dedicated workqueue to not upset anyone else, use a
> delayed work. A bunch of changes:
>
> - We can throw out all the custom wakeup and requeue logic and state
> tracking. If we schedule the work with a 0 delay it'll get
> scheduled immediately.
>
> - Persistent state (frame & draw_status_timeout) need to be moved out
> of the work.
>
> - diff is bigger than the changes, biggest chunk is reindenting the
> work fn because it lost its while loop.
>
> Lots of code deleting as consequence all over. Specifically we can
> delete the drm_driver.release code now!
>
> v2: Review from Hans:
> - Use mod_delayed_work in the plane update path to make sure we do
> actually schedule immediately). In the worker we still want
> queue_delayed_work, which won't modify the timeout when the work is
> already scheduled. Which is exactly what we want if the work races
> with a plane update.
> - Switch to system_long_wq, Hans says on usb2 a plane upload can take
> 80 ms.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Hans de Goede <hdegoede@redhat.com>
> Cc: "Noralf Trønnes" <noralf@tronnes.org>
Patch looks good to me:
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Regards,
Hans
> ---
> drivers/gpu/drm/tiny/gm12u320.c | 171 +++++++++++++-------------------
> 1 file changed, 68 insertions(+), 103 deletions(-)
>
> diff --git a/drivers/gpu/drm/tiny/gm12u320.c b/drivers/gpu/drm/tiny/gm12u320.c
> index c22b2ee470eb..bbc4ef4456e6 100644
> --- a/drivers/gpu/drm/tiny/gm12u320.c
> +++ b/drivers/gpu/drm/tiny/gm12u320.c
> @@ -89,13 +89,12 @@ struct gm12u320_device {
> unsigned char *cmd_buf;
> unsigned char *data_buf[GM12U320_BLOCK_COUNT];
> struct {
> - bool run;
> - struct workqueue_struct *workq;
> - struct work_struct work;
> - wait_queue_head_t waitq;
> + struct delayed_work work;
> struct mutex lock;
> struct drm_framebuffer *fb;
> struct drm_rect rect;
> + int frame;
> + int draw_status_timeout;
> } fb_update;
> };
>
> @@ -183,19 +182,9 @@ static int gm12u320_usb_alloc(struct gm12u320_device *gm12u320)
> data_block_footer, DATA_BLOCK_FOOTER_SIZE);
> }
>
> - gm12u320->fb_update.workq = create_singlethread_workqueue(DRIVER_NAME);
> - if (!gm12u320->fb_update.workq)
> - return -ENOMEM;
> -
> return 0;
> }
>
> -static void gm12u320_usb_free(struct gm12u320_device *gm12u320)
> -{
> - if (gm12u320->fb_update.workq)
> - destroy_workqueue(gm12u320->fb_update.workq);
> -}
> -
> static int gm12u320_misc_request(struct gm12u320_device *gm12u320,
> u8 req_a, u8 req_b,
> u8 arg_a, u8 arg_b, u8 arg_c, u8 arg_d)
> @@ -338,80 +327,77 @@ static void gm12u320_copy_fb_to_blocks(struct gm12u320_device *gm12u320)
> static void gm12u320_fb_update_work(struct work_struct *work)
> {
> struct gm12u320_device *gm12u320 =
> - container_of(work, struct gm12u320_device, fb_update.work);
> - int draw_status_timeout = FIRST_FRAME_TIMEOUT;
> + container_of(to_delayed_work(work), struct gm12u320_device,
> + fb_update.work);
> int block, block_size, len;
> - int frame = 0;
> int ret = 0;
>
> - while (gm12u320->fb_update.run) {
> - gm12u320_copy_fb_to_blocks(gm12u320);
> -
> - for (block = 0; block < GM12U320_BLOCK_COUNT; block++) {
> - if (block == GM12U320_BLOCK_COUNT - 1)
> - block_size = DATA_LAST_BLOCK_SIZE;
> - else
> - block_size = DATA_BLOCK_SIZE;
> -
> - /* Send data command to device */
> - memcpy(gm12u320->cmd_buf, cmd_data, CMD_SIZE);
> - gm12u320->cmd_buf[8] = block_size & 0xff;
> - gm12u320->cmd_buf[9] = block_size >> 8;
> - gm12u320->cmd_buf[20] = 0xfc - block * 4;
> - gm12u320->cmd_buf[21] = block | (frame << 7);
> -
> - ret = usb_bulk_msg(gm12u320->udev,
> - usb_sndbulkpipe(gm12u320->udev, DATA_SND_EPT),
> - gm12u320->cmd_buf, CMD_SIZE, &len,
> - CMD_TIMEOUT);
> - if (ret || len != CMD_SIZE)
> - goto err;
> -
> - /* Send data block to device */
> - ret = usb_bulk_msg(gm12u320->udev,
> - usb_sndbulkpipe(gm12u320->udev, DATA_SND_EPT),
> - gm12u320->data_buf[block], block_size,
> - &len, DATA_TIMEOUT);
> - if (ret || len != block_size)
> - goto err;
> -
> - /* Read status */
> - ret = usb_bulk_msg(gm12u320->udev,
> - usb_rcvbulkpipe(gm12u320->udev, DATA_RCV_EPT),
> - gm12u320->cmd_buf, READ_STATUS_SIZE, &len,
> - CMD_TIMEOUT);
> - if (ret || len != READ_STATUS_SIZE)
> - goto err;
> - }
> + gm12u320_copy_fb_to_blocks(gm12u320);
> +
> + for (block = 0; block < GM12U320_BLOCK_COUNT; block++) {
> + if (block == GM12U320_BLOCK_COUNT - 1)
> + block_size = DATA_LAST_BLOCK_SIZE;
> + else
> + block_size = DATA_BLOCK_SIZE;
> +
> + /* Send data command to device */
> + memcpy(gm12u320->cmd_buf, cmd_data, CMD_SIZE);
> + gm12u320->cmd_buf[8] = block_size & 0xff;
> + gm12u320->cmd_buf[9] = block_size >> 8;
> + gm12u320->cmd_buf[20] = 0xfc - block * 4;
> + gm12u320->cmd_buf[21] =
> + block | (gm12u320->fb_update.frame << 7);
>
> - /* Send draw command to device */
> - memcpy(gm12u320->cmd_buf, cmd_draw, CMD_SIZE);
> ret = usb_bulk_msg(gm12u320->udev,
> usb_sndbulkpipe(gm12u320->udev, DATA_SND_EPT),
> - gm12u320->cmd_buf, CMD_SIZE, &len, CMD_TIMEOUT);
> + gm12u320->cmd_buf, CMD_SIZE, &len,
> + CMD_TIMEOUT);
> if (ret || len != CMD_SIZE)
> goto err;
>
> + /* Send data block to device */
> + ret = usb_bulk_msg(gm12u320->udev,
> + usb_sndbulkpipe(gm12u320->udev, DATA_SND_EPT),
> + gm12u320->data_buf[block], block_size,
> + &len, DATA_TIMEOUT);
> + if (ret || len != block_size)
> + goto err;
> +
> /* Read status */
> ret = usb_bulk_msg(gm12u320->udev,
> usb_rcvbulkpipe(gm12u320->udev, DATA_RCV_EPT),
> gm12u320->cmd_buf, READ_STATUS_SIZE, &len,
> - draw_status_timeout);
> + CMD_TIMEOUT);
> if (ret || len != READ_STATUS_SIZE)
> goto err;
> -
> - draw_status_timeout = CMD_TIMEOUT;
> - frame = !frame;
> -
> - /*
> - * We must draw a frame every 2s otherwise the projector
> - * switches back to showing its logo.
> - */
> - wait_event_timeout(gm12u320->fb_update.waitq,
> - !gm12u320->fb_update.run ||
> - gm12u320->fb_update.fb != NULL,
> - IDLE_TIMEOUT);
> }
> +
> + /* Send draw command to device */
> + memcpy(gm12u320->cmd_buf, cmd_draw, CMD_SIZE);
> + ret = usb_bulk_msg(gm12u320->udev,
> + usb_sndbulkpipe(gm12u320->udev, DATA_SND_EPT),
> + gm12u320->cmd_buf, CMD_SIZE, &len, CMD_TIMEOUT);
> + if (ret || len != CMD_SIZE)
> + goto err;
> +
> + /* Read status */
> + ret = usb_bulk_msg(gm12u320->udev,
> + usb_rcvbulkpipe(gm12u320->udev, DATA_RCV_EPT),
> + gm12u320->cmd_buf, READ_STATUS_SIZE, &len,
> + gm12u320->fb_update.draw_status_timeout);
> + if (ret || len != READ_STATUS_SIZE)
> + goto err;
> +
> + gm12u320->fb_update.draw_status_timeout = CMD_TIMEOUT;
> + gm12u320->fb_update.frame = !gm12u320->fb_update.frame;
> +
> + /*
> + * We must draw a frame every 2s otherwise the projector
> + * switches back to showing its logo.
> + */
> + queue_delayed_work(system_long_wq, &gm12u320->fb_update.work,
> + IDLE_TIMEOUT);
> +
> return;
> err:
> /* Do not log errors caused by module unload or device unplug */
> @@ -446,36 +432,24 @@ static void gm12u320_fb_mark_dirty(struct drm_framebuffer *fb,
> mutex_unlock(&gm12u320->fb_update.lock);
>
> if (wakeup)
> - wake_up(&gm12u320->fb_update.waitq);
> + mod_delayed_work(system_long_wq, &gm12u320->fb_update.work, 0);
>
> if (old_fb)
> drm_framebuffer_put(old_fb);
> }
>
> -static void gm12u320_start_fb_update(struct gm12u320_device *gm12u320)
> -{
> - mutex_lock(&gm12u320->fb_update.lock);
> - gm12u320->fb_update.run = true;
> - mutex_unlock(&gm12u320->fb_update.lock);
> -
> - queue_work(gm12u320->fb_update.workq, &gm12u320->fb_update.work);
> -}
> -
> static void gm12u320_stop_fb_update(struct gm12u320_device *gm12u320)
> {
> - mutex_lock(&gm12u320->fb_update.lock);
> - gm12u320->fb_update.run = false;
> - mutex_unlock(&gm12u320->fb_update.lock);
> + struct drm_framebuffer *old_fb;
>
> - wake_up(&gm12u320->fb_update.waitq);
> - cancel_work_sync(&gm12u320->fb_update.work);
> + cancel_delayed_work_sync(&gm12u320->fb_update.work);
>
> mutex_lock(&gm12u320->fb_update.lock);
> - if (gm12u320->fb_update.fb) {
> - drm_framebuffer_put(gm12u320->fb_update.fb);
> - gm12u320->fb_update.fb = NULL;
> - }
> + old_fb = gm12u320->fb_update.fb;
> + gm12u320->fb_update.fb = NULL;
> mutex_unlock(&gm12u320->fb_update.lock);
> +
> + drm_framebuffer_put(old_fb);
> }
>
> static int gm12u320_set_ecomode(struct gm12u320_device *gm12u320)
> @@ -583,11 +557,11 @@ static void gm12u320_pipe_enable(struct drm_simple_display_pipe *pipe,
> struct drm_crtc_state *crtc_state,
> struct drm_plane_state *plane_state)
> {
> - struct gm12u320_device *gm12u320 = pipe->crtc.dev->dev_private;
> struct drm_rect rect = { 0, 0, GM12U320_USER_WIDTH, GM12U320_HEIGHT };
> + struct gm12u320_device *gm12u320 = pipe->crtc.dev->dev_private;
>
> + gm12u320->fb_update.draw_status_timeout = FIRST_FRAME_TIMEOUT;
> gm12u320_fb_mark_dirty(plane_state->fb, &rect);
> - gm12u320_start_fb_update(gm12u320);
> }
>
> static void gm12u320_pipe_disable(struct drm_simple_display_pipe *pipe)
> @@ -622,13 +596,6 @@ static const uint64_t gm12u320_pipe_modifiers[] = {
> DRM_FORMAT_MOD_INVALID
> };
>
> -static void gm12u320_driver_release(struct drm_device *dev)
> -{
> - struct gm12u320_device *gm12u320 = dev->dev_private;
> -
> - gm12u320_usb_free(gm12u320);
> -}
> -
> DEFINE_DRM_GEM_FOPS(gm12u320_fops);
>
> static struct drm_driver gm12u320_drm_driver = {
> @@ -640,7 +607,6 @@ static struct drm_driver gm12u320_drm_driver = {
> .major = DRIVER_MAJOR,
> .minor = DRIVER_MINOR,
>
> - .release = gm12u320_driver_release,
> .fops = &gm12u320_fops,
> DRM_GEM_SHMEM_DRIVER_OPS,
> };
> @@ -670,9 +636,8 @@ static int gm12u320_usb_probe(struct usb_interface *interface,
> return -ENOMEM;
>
> gm12u320->udev = interface_to_usbdev(interface);
> - INIT_WORK(&gm12u320->fb_update.work, gm12u320_fb_update_work);
> + INIT_DELAYED_WORK(&gm12u320->fb_update.work, gm12u320_fb_update_work);
> mutex_init(&gm12u320->fb_update.lock);
> - init_waitqueue_head(&gm12u320->fb_update.waitq);
>
> dev = &gm12u320->dev;
> ret = devm_drm_dev_init(&interface->dev, dev, &gm12u320_drm_driver);
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
next prev parent reply other threads:[~2020-02-27 19:04 UTC|newest]
Thread overview: 95+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-02-27 18:14 [PATCH 00/51] drm managed resources, v3 Daniel Vetter
2020-02-27 18:14 ` [PATCH 01/51] mm/sl[uo]b: export __kmalloc_track(_node)_caller Daniel Vetter
2020-02-27 18:14 ` [PATCH 02/51] drm/i915: Don't clear drvdata in ->release Daniel Vetter
2020-02-27 18:14 ` [PATCH 03/51] drm: add managed resources tied to drm_device Daniel Vetter
2020-02-28 22:45 ` Sam Ravnborg
2020-02-28 23:14 ` Daniel Vetter
2020-02-29 11:17 ` Sam Ravnborg
2020-02-29 11:28 ` Daniel Vetter
2020-03-02 9:22 ` Jani Nikula
2020-03-02 9:36 ` Daniel Vetter
2020-03-02 9:39 ` Daniel Vetter
2020-03-02 16:34 ` Sam Ravnborg
2020-02-27 18:14 ` [PATCH 04/51] drm: Set final_kfree in drm_dev_alloc Daniel Vetter
2020-02-27 18:14 ` [PATCH 05/51] drm/mipi_dbi: Use drmm_add_final_kfree in all drivers Daniel Vetter
2020-02-27 20:55 ` Sam Ravnborg
2020-02-27 18:14 ` [PATCH 06/51] drm/udl: Use drmm_add_final_kfree Daniel Vetter
2020-02-27 20:57 ` Sam Ravnborg
2020-02-27 18:14 ` [PATCH 07/51] drm/qxl: " Daniel Vetter
2020-02-27 18:14 ` [PATCH 08/51] drm/i915: " Daniel Vetter
2020-02-27 18:14 ` [PATCH 09/51] drm/cirrus: " Daniel Vetter
2020-02-27 21:01 ` Sam Ravnborg
2020-02-27 18:14 ` [PATCH 10/51] drm/v3d: " Daniel Vetter
2020-02-27 18:14 ` [PATCH 11/51] drm/tidss: " Daniel Vetter
2020-02-27 18:14 ` [PATCH 12/51] drm/mcde: " Daniel Vetter
2020-02-27 18:14 ` [PATCH 13/51] drm/vgem: " Daniel Vetter
2020-02-27 21:02 ` Sam Ravnborg
2020-02-27 18:14 ` [PATCH 14/51] drm/vkms: " Daniel Vetter
2020-02-27 18:59 ` Rodrigo Siqueira
2020-02-27 18:14 ` [PATCH 15/51] drm/repaper: " Daniel Vetter
2020-02-27 18:14 ` [PATCH 16/51] drm/inigenic: " Daniel Vetter
2020-03-02 20:12 ` Paul Cercueil
2020-02-27 18:14 ` [PATCH 17/51] drm/gm12u320: " Daniel Vetter
2020-02-27 18:14 ` [PATCH 18/51] drm/<drivers>: " Daniel Vetter
2020-02-27 18:35 ` Liviu Dudau
2020-02-27 18:14 ` [PATCH 19/51] drm: Cleanups after drmm_add_final_kfree rollout Daniel Vetter
2020-02-27 18:14 ` [PATCH 20/51] drm: Handle dev->unique with drmm_ Daniel Vetter
2020-02-27 18:14 ` [PATCH 21/51] drm: Use drmm_ for drm_dev_init cleanup Daniel Vetter
2020-02-27 18:14 ` [PATCH 22/51] drm: manage drm_minor cleanup with drmm_ Daniel Vetter
2020-02-27 18:14 ` [PATCH 23/51] drm: Manage drm_gem_init " Daniel Vetter
2020-02-27 18:14 ` [PATCH 24/51] drm: Manage drm_vblank_cleanup " Daniel Vetter
2020-02-27 18:14 ` [PATCH 25/51] drm: Garbage collect drm_dev_fini Daniel Vetter
2020-02-27 18:14 ` [PATCH 26/51] drm: Manage drm_mode_config_init with drmm_ Daniel Vetter
2020-02-28 7:30 ` Thomas Zimmermann
2020-02-28 8:43 ` Daniel Vetter
2020-02-28 9:56 ` Thomas Zimmermann
2020-02-28 20:26 ` Sam Ravnborg
2020-02-28 23:11 ` Daniel Vetter
2020-02-29 10:59 ` Sam Ravnborg
2020-03-02 14:09 ` Daniel Vetter
2020-02-27 18:14 ` [PATCH 27/51] drm/bochs: Remove leftover drm_atomic_helper_shutdown Daniel Vetter
2020-02-27 18:14 ` [PATCH 28/51] drm/bochs: Drop explicit drm_mode_config_cleanup Daniel Vetter
2020-02-27 18:15 ` [PATCH 29/51] drm/cirrus: Drop explicit drm_mode_config_cleanup call Daniel Vetter
2020-02-28 20:32 ` Sam Ravnborg
2020-02-27 18:15 ` [PATCH 30/51] drm/cirrus: Fully embrace devm_ Daniel Vetter
2020-02-27 18:15 ` [PATCH 31/51] drm/ingenic: Drop explicit drm_mode_config_cleanup call Daniel Vetter
2020-03-02 20:13 ` Paul Cercueil
2020-02-27 18:15 ` [PATCH 32/51] drm/mcde: " Daniel Vetter
2020-02-27 18:15 ` [PATCH 33/51] drm/mcde: More devm_drm_dev_init Daniel Vetter
2020-02-27 18:15 ` [PATCH 34/51] drm/meson: Drop explicit drm_mode_config_cleanup call Daniel Vetter
2020-02-27 18:15 ` [PATCH 35/51] drm/pl111: " Daniel Vetter
2020-02-27 18:15 ` [PATCH 36/51] drm/rcar-du: " Daniel Vetter
2020-02-27 18:15 ` [PATCH 37/51] drm/rockchip: " Daniel Vetter
2020-02-28 21:19 ` kbuild test robot
2020-02-28 23:34 ` Daniel Vetter
2020-02-29 1:53 ` kbuild test robot
2020-02-27 18:15 ` [PATCH 38/51] drm/stm: " Daniel Vetter
2020-02-27 18:15 ` [PATCH 39/51] drm/shmob: " Daniel Vetter
2020-02-27 18:15 ` [PATCH 40/51] drm/mtk: " Daniel Vetter
2020-02-27 18:15 ` [PATCH 41/51] drm/tidss: " Daniel Vetter
2020-02-27 18:15 ` [PATCH 42/51] drm/gm12u320: More drmm_ Daniel Vetter
2020-02-27 18:15 ` [PATCH 43/51] drm/gm12u320: Use devm_drm_dev_init Daniel Vetter
2020-02-27 18:15 ` [PATCH 44/51] drm/gm12u320: Use helpers for shutdown/suspend/resume Daniel Vetter
2020-02-27 18:15 ` [PATCH 45/51] drm/gm12u320: Simplify upload work Daniel Vetter
2020-02-27 19:04 ` Hans de Goede [this message]
2020-02-27 18:15 ` [PATCH 46/51] drm/repaper: Drop explicit drm_mode_config_cleanup call Daniel Vetter
2020-02-27 18:15 ` [PATCH 47/51] drm/mipi-dbi: Move drm_mode_config_init into mipi library Daniel Vetter
2020-02-28 20:34 ` Sam Ravnborg
2020-02-27 18:15 ` [PATCH 48/51] drm/mipi-dbi: Drop explicit drm_mode_config_cleanup call Daniel Vetter
2020-02-28 20:36 ` Sam Ravnborg
2020-02-27 18:15 ` [PATCH 49/51] drm/udl: " Daniel Vetter
2020-02-28 20:37 ` Sam Ravnborg
2020-02-27 18:15 ` [PATCH 50/51] drm/udl: drop drm_driver.release hook Daniel Vetter
2020-02-28 7:43 ` Thomas Zimmermann
2020-02-28 8:40 ` Daniel Vetter
2020-02-28 11:46 ` Thomas Zimmermann
2020-02-28 17:43 ` Daniel Vetter
2020-03-02 8:14 ` Thomas Zimmermann
2020-03-02 9:01 ` Daniel Vetter
2020-02-27 18:15 ` [PATCH 51/51] drm: Add docs for managed resources Daniel Vetter
2020-02-27 20:53 ` Sam Ravnborg
-- strict thread matches above, loose matches on Subject: below --
2020-03-23 14:48 [PATCH 00/51] drm_device managed resources, v5 Daniel Vetter
2020-03-23 14:49 ` [PATCH 45/51] drm/gm12u320: Simplify upload work Daniel Vetter
2020-03-02 22:25 [PATCH 00/51] drm_device managed resources, v4 Daniel Vetter
2020-03-02 22:26 ` [PATCH 45/51] drm/gm12u320: Simplify upload work Daniel Vetter
2020-02-21 21:02 [PATCH 00/51] drm managed resources, v2 Daniel Vetter
2020-02-21 21:03 ` [PATCH 45/51] drm/gm12u320: Simplify upload work Daniel Vetter
2020-02-22 12:30 ` Hans de Goede
2020-02-22 13:00 ` Daniel Vetter
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=39a5091b-cf17-0cd9-cf43-a7a8b3cd7b78@redhat.com \
--to=hdegoede@redhat.com \
--cc=daniel.vetter@ffwll.ch \
--cc=daniel.vetter@intel.com \
--cc=dri-devel@lists.freedesktop.org \
--cc=intel-gfx@lists.freedesktop.org \
--cc=m.felsch@pengutronix.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).