Re: [RFC v3 11/12] drm/client: Add bootsplash client

[Max Staudt]

Thanks for CCing!

I like the idea of this patchset. As far as I understand, this multiplexing is 
exactly what I would have had to write in order to port the bootsplash to DRM. 
And we finally get rid of the driver-specific FB emulation hacks, too. Good 
riddance.

Thanks for the initiative, Noralf!

I've stopped working on the bootsplash since there seemed to be no more 
interest in it - but if there is, I can port it to such an in-kernel DRM client 
architecture once it's in.


Max

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC v3 11/12] drm/client: Add bootsplash client

[Daniel Vetter]

On Thu, Feb 22, 2018 at 09:06:52PM +0100, Noralf Trønnes wrote:
> Just a hack to test the client API.
> 
> Signed-off-by: Noralf Trønnes 

Adding the suse folks who submitted the bootsplash a while ago, would be
great if they could pick this up and run with it.

> ---
>  drivers/gpu/drm/client/Kconfig  |   5 +
>  drivers/gpu/drm/client/Makefile |   1 +
>  drivers/gpu/drm/client/drm_bootsplash.c | 205 
> 
>  3 files changed, 211 insertions(+)
>  create mode 100644 drivers/gpu/drm/client/drm_bootsplash.c
> 
> diff --git a/drivers/gpu/drm/client/Kconfig b/drivers/gpu/drm/client/Kconfig
> index 73902ab44c75..16cf1e14620a 100644
> --- a/drivers/gpu/drm/client/Kconfig
> +++ b/drivers/gpu/drm/client/Kconfig
> @@ -17,4 +17,9 @@ config DRM_CLIENT_FBDEV
>   help
> Generic fbdev emulation
>  
> +config DRM_CLIENT_BOOTSPLASH
> + tristate "DRM Bootsplash"
> + help
> +   DRM Bootsplash
> +
>  endmenu
> diff --git a/drivers/gpu/drm/client/Makefile b/drivers/gpu/drm/client/Makefile
> index 3ff694429dec..8660530e4646 100644
> --- a/drivers/gpu/drm/client/Makefile
> +++ b/drivers/gpu/drm/client/Makefile
> @@ -1,3 +1,4 @@
>  # SPDX-License-Identifier: GPL-2.0
>  
>  obj-$(CONFIG_DRM_CLIENT_FBDEV) += drm_fbdev.o
> +obj-$(CONFIG_DRM_CLIENT_BOOTSPLASH) += drm_bootsplash.o
> diff --git a/drivers/gpu/drm/client/drm_bootsplash.c 
> b/drivers/gpu/drm/client/drm_bootsplash.c
> new file mode 100644
> index ..43c703606e74
> --- /dev/null
> +++ b/drivers/gpu/drm/client/drm_bootsplash.c
> @@ -0,0 +1,205 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +struct drm_bootsplash {
> + struct drm_client_dev *client;
> + struct drm_client_display *display;
> + struct drm_client_buffer *buffer[2];
> + struct work_struct worker;
> + bool stop;
> +};
> +
> +static u32 drm_bootsplash_color_table[3] = {
> + 0x00ff, 0xff00, 0x00ff,
> +};
> +
> +/* Draw a box with changing colors */
> +static void
> +drm_bootsplash_draw(struct drm_client_buffer *buffer, unsigned int sequence)
> +{
> + unsigned int x, y;
> + u32 *pix;
> +
> + pix = buffer->vaddr;
> + pix += ((buffer->height / 2) - 50) * buffer->width;
> + pix += (buffer->width / 2) - 50;
> +
> + for (y = 0; y < 100; y++) {
> + for (x = 0; x < 100; x++)
> + *pix++ = drm_bootsplash_color_table[sequence];
> + pix += buffer->width - 100;
> + }
> +}
> +
> +static void drm_bootsplash_worker(struct work_struct *work)
> +{
> + struct drm_bootsplash *splash = container_of(work, struct 
> drm_bootsplash,
> +  worker);
> + struct drm_event *event;
> + unsigned int i = 0, sequence = 0, fb_id;
> + int ret;
> +
> + while (!splash->stop) {
> + /* Are we still in charge? */
> + fb_id = drm_client_display_current_fb(splash->display);
> + if (fb_id != splash->buffer[i]->fb_ids[0])
> + break;
> +
> + /*
> +  * We can race with userspace here between checking and doing
> +  * the page flip, so double buffering isn't such a good idea.
> +  * Tearing probably isn't a problem on a presumably small splash
> +  * animation. I've kept it to test the page flip code.
> +  */

I think a much cleaner way to solve all this is to tie it into our master
tracking. If there is a master (even if it's not displaying anything),
then none of the in-kernel clients should display anything.

If there is not, then we grab a temporary/weak master reference which
blocks other masters for the time being, but only until we've complete our
drawing operation. Then the in-kernel client drops that weak reference
again. A very simply solution would be to simply hold the device's master
lock (but I'm not sure whether that would result in deadlocks).

That would mean something like

if (!drm_master_try_internal_acquire())
/* someone else is master */
break;

here instead of your fb id check.
> +
> + i = !i;
> + drm_bootsplash_draw(splash->buffer[i], sequence++);
> + if (sequence == 3)
> + sequence = 0;
> +
> + ret = drm_client_display_page_flip(splash->display,
> +splash->buffer[i]->fb_ids[0],
> +true);
> + if (!ret) {
> + event = drm_client_read_event(splash->client, true);
> + if (!IS_ERR(event))
> + kfree(event);
> + }

And
drm_master_interal_relase()

here before we sleep again. 

[RFC v3 11/12] drm/client: Add bootsplash client

[Noralf Trønnes]

Just a hack to test the client API.

Signed-off-by: Noralf Trønnes 
---
 drivers/gpu/drm/client/Kconfig  |   5 +
 drivers/gpu/drm/client/Makefile |   1 +
 drivers/gpu/drm/client/drm_bootsplash.c | 205 
 3 files changed, 211 insertions(+)
 create mode 100644 drivers/gpu/drm/client/drm_bootsplash.c

diff --git a/drivers/gpu/drm/client/Kconfig b/drivers/gpu/drm/client/Kconfig
index 73902ab44c75..16cf1e14620a 100644
--- a/drivers/gpu/drm/client/Kconfig
+++ b/drivers/gpu/drm/client/Kconfig
@@ -17,4 +17,9 @@ config DRM_CLIENT_FBDEV
help
  Generic fbdev emulation
 
+config DRM_CLIENT_BOOTSPLASH
+   tristate "DRM Bootsplash"
+   help
+ DRM Bootsplash
+
 endmenu
diff --git a/drivers/gpu/drm/client/Makefile b/drivers/gpu/drm/client/Makefile
index 3ff694429dec..8660530e4646 100644
--- a/drivers/gpu/drm/client/Makefile
+++ b/drivers/gpu/drm/client/Makefile
@@ -1,3 +1,4 @@
 # SPDX-License-Identifier: GPL-2.0
 
 obj-$(CONFIG_DRM_CLIENT_FBDEV) += drm_fbdev.o
+obj-$(CONFIG_DRM_CLIENT_BOOTSPLASH) += drm_bootsplash.o
diff --git a/drivers/gpu/drm/client/drm_bootsplash.c 
b/drivers/gpu/drm/client/drm_bootsplash.c
new file mode 100644
index ..43c703606e74
--- /dev/null
+++ b/drivers/gpu/drm/client/drm_bootsplash.c
@@ -0,0 +1,205 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+#include 
+#include 
+#include 
+
+struct drm_bootsplash {
+   struct drm_client_dev *client;
+   struct drm_client_display *display;
+   struct drm_client_buffer *buffer[2];
+   struct work_struct worker;
+   bool stop;
+};
+
+static u32 drm_bootsplash_color_table[3] = {
+   0x00ff, 0xff00, 0x00ff,
+};
+
+/* Draw a box with changing colors */
+static void
+drm_bootsplash_draw(struct drm_client_buffer *buffer, unsigned int sequence)
+{
+   unsigned int x, y;
+   u32 *pix;
+
+   pix = buffer->vaddr;
+   pix += ((buffer->height / 2) - 50) * buffer->width;
+   pix += (buffer->width / 2) - 50;
+
+   for (y = 0; y < 100; y++) {
+   for (x = 0; x < 100; x++)
+   *pix++ = drm_bootsplash_color_table[sequence];
+   pix += buffer->width - 100;
+   }
+}
+
+static void drm_bootsplash_worker(struct work_struct *work)
+{
+   struct drm_bootsplash *splash = container_of(work, struct 
drm_bootsplash,
+worker);
+   struct drm_event *event;
+   unsigned int i = 0, sequence = 0, fb_id;
+   int ret;
+
+   while (!splash->stop) {
+   /* Are we still in charge? */
+   fb_id = drm_client_display_current_fb(splash->display);
+   if (fb_id != splash->buffer[i]->fb_ids[0])
+   break;
+
+   /*
+* We can race with userspace here between checking and doing
+* the page flip, so double buffering isn't such a good idea.
+* Tearing probably isn't a problem on a presumably small splash
+* animation. I've kept it to test the page flip code.
+*/
+
+   i = !i;
+   drm_bootsplash_draw(splash->buffer[i], sequence++);
+   if (sequence == 3)
+   sequence = 0;
+
+   ret = drm_client_display_page_flip(splash->display,
+  splash->buffer[i]->fb_ids[0],
+  true);
+   if (!ret) {
+   event = drm_client_read_event(splash->client, true);
+   if (!IS_ERR(event))
+   kfree(event);
+   }
+   msleep(500);
+   }
+
+   for (i = 0; i < 2; i++)
+   drm_client_framebuffer_delete(splash->buffer[i]);
+   drm_client_display_free(splash->display);
+}
+
+static int drm_bootsplash_setup(struct drm_bootsplash *splash)
+{
+   struct drm_client_dev *client = splash->client;
+   struct drm_client_buffer *buffer[2];
+   struct drm_client_display *display;
+   struct drm_mode_modeinfo *mode;
+   int ret, i;
+
+   display = drm_client_display_get_first_enabled(client, false);
+   if (IS_ERR(display))
+   return PTR_ERR(display);
+   if (!display)
+   return -ENOENT;
+
+   mode = drm_client_display_first_mode(display);
+   if (!mode) {
+   ret = -EINVAL;
+   goto err_free_display;
+   }
+
+   for (i = 0; i < 2; i++) {
+   buffer[i] = drm_client_framebuffer_create(client, mode,
+ DRM_FORMAT_XRGB);
+   if (IS_ERR(buffer[i])) {
+   ret = PTR_ERR(buffer[i]);
+   goto err_free_buffer;
+   }
+   }
+
+