<div>>+static void</div><div>>+probe_udev_for_graphics (state_t *state)</div><div>>+{</div><div>>+ struct udev_enumerate *udev_enum;</div><div>>+ struct udev_list_entry *entries, *entry;</div><div>>+</div>
<div>>+ assert (state != NULL);</div><div>>+</div><div>>+ udev_enum = udev_enumerate_new (state->udev);</div><div>>+ udev_enumerate_add_match_subsystem (udev_enum, "graphics");</div><div>>So this makes the assumption that the fb device will always be set up</div>
<div>>after the drm device is.</div><div>>I think that's a valid assumption in practice, but there should</div><div>>probably be a comment to that effect.</div><div><br></div><div>I don't think that assumption is being made. For context, the full list of udev calls there is:</div>
<div><br></div><div><div>+ udev_enum = udev_enumerate_new (state->udev);</div><div>+ udev_enumerate_add_match_subsystem (udev_enum, "graphics");</div><div>+ udev_enumerate_add_match_tag(udev_enum, "seat");</div>
<div>+ udev_enumerate_add_match_is_initialized(udev_enum);</div><div>+ udev_enumerate_scan_devices (udev_enum);</div><div>+</div><div>+ entries = udev_enumerate_get_list_entry (udev_enum);</div></div><div><br></div><div>
Unless I'm mistaken about how to drive the udev API, that should just simply gather a list of all graphics devices with the "seat" tag that have been fully initialized out of the list of all the devices that udev has detected at that point; the ordering with framebuffer devices shouldn't come into play.</div>
<div><br></div><div><div>>Alternatively, it could look for events in the drm subsystem, but then</div><div>>care would need to be taken to ensure we aren't acting on events "too</div><div>>soon" while things are partially initialized.</div>
</div><div><br></div><div>This is what's happening with setup_and_watch_udev(). In that function, I watch the "graphics" subsystem with a udev_monitor and tell the plymouth event loop to handle events on that udev_monitor with on_udev_graphics_event(). As far as I remember, udev events don't fire until the device is fully initialized.</div>
<div><br></div><div>Thanks,</div><div>Kevin</div><br><div class="gmail_quote">On Thu, Feb 28, 2013 at 10:31 PM, Ray Strode <span dir="ltr"><<a href="mailto:halfline@gmail.com" target="_blank">halfline@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hey,<br>
<div><div class="h5"><br>
On Mon, Feb 25, 2013 at 11:39 PM, Kevin Murphy <<a href="mailto:kemurphy.cmu@gmail.com">kemurphy.cmu@gmail.com</a>> wrote:<br>
> Hello everyone,<br>
><br>
> At some point last year, I started on adding udev support to plymouth. I<br>
> eventually ran out of time to work on it, but the attached patch is as far<br>
> as I got (I've rebased it to the current master as of 5 days ago). It looks<br>
> like I'll be able to work on this a bit more in my spare time again, so I'd<br>
> appreciate any comments you may have. The next step I was going to take was<br>
> to try to avoid loading and unloading the renderer each time we show and<br>
> hide the splash screen. Ray mentioned that one way to do this might involve<br>
> ply_renderer_close_input_source. There is a relevant conversation log from<br>
> IRC at <a href="http://people.freedesktop.org/~halfline/renderer-discussion.txt" target="_blank">http://people.freedesktop.org/~halfline/renderer-discussion.txt</a> from<br>
> nearly a year ago, as well.<br>
</div></div>Thanks for looking into this! It's sorely need feature. I'll give your<br>
patches a try soon, but wanted to give a few drive by comments below<br>
from doing a quick read though:<br>
<br>
--- a/src/Makefile.am<br>
+++ b/src/Makefile.am<br>
@@ -20,6 +20,7 @@ plymouthd_CFLAGS = $(PLYMOUTH_CFLAGS)<br>
\<br>
-DPLYMOUTH_POLICY_DIR=\"$(PLYMOUTH_POLICY_DIR)/\" \<br>
-DPLYMOUTH_RUNTIME_DIR=\"$(PLYMOUTH_RUNTIME_DIR)\" \<br>
-DPLYMOUTH_CONF_DIR=\"$(PLYMOUTH_CONF_DIR)/\"<br>
+plymouthd_LDFLAGS = -ludev<br>
Should use PKG_CHECK_MODULES(... libudev ...) in <a href="http://configure.ac" target="_blank">configure.ac</a><br>
<br>
--- a/src/main.c<br>
+++ b/src/main.c<br>
typedef struct<br>
{<br>
+ const char *fb;<br>
+ const char *card;<br>
+ ply_renderer_t *renderer;<br>
+} ply_pixel_device_t;<br>
<br>
It seems like there may be some conceptual overlap between what a<br>
pixel device is and what a renderer is. Maybe they should be<br>
consolidated? if there's a compelling reason they shouldn't be so be<br>
it, though.<br>
<br>
The other thing is main.c is already pretty fat. It would be good if<br>
we could farm out logic to other source files.<br>
@@ -455,6 +478,336 @@ show_default_splash (state_t *state)<br>
<br><div style="text-align:right">+static void</div><div style="text-align:right">+probe_udev_for_graphics (state_t *state)</div><div style="text-align:right">+{</div><div style="text-align:right">+ struct udev_enumerate *udev_enum;</div>
<div style="text-align:right">+ struct udev_list_entry *entries, *entry;</div><div style="text-align:right">+</div><div style="text-align:right">+ assert (state != NULL);</div><div style="text-align:right">+</div><div style="text-align:right">
+ udev_enum = udev_enumerate_new (state->udev);</div><div style="text-align:right">+ udev_enumerate_add_match_subsystem (udev_enum, "graphics");</div><div style="text-align:right">So this makes the assumption that the fb device will always be set up</div>
<div style="text-align:right">after the drm device is.</div><div style="text-align:right">I think that's a valid assumption in practice, but there should</div><div style="text-align:right">probably be a comment to that effect.</div>
<div style="text-align:right"><br></div><div style="text-align:right">Alternatively, it could look for events in the drm subsystem, but then</div><div style="text-align:right">care would need to be taken to ensure we aren't acting on events "too</div>
<div style="text-align:right">soon" while things are partially initialized.</div>
<br>
Will try this soon!<br>
<span class="HOEnZb"><font color="#888888"><br>
--Ray<br>
</font></span></blockquote></div><br>