Re: [WESTON PATCH 2/2] compositor-drm: Watchdog timer implementation
On 14-10-29 05:40 PM, Bill Spitzak wrote:
On 10/29/2014 12:19 PM, Frederic Plourde wrote:
Mhh... you mean something like :
static int
drm_output_watchdog_timer_create(struct drm_output *output)
{
struct wl_event_loop *loop = NULL;
struct weston_compositor *ec = output_base->compositor;
loop = wl_display_get_event_loop(ec->wl_display);
if (loop) {
output->wdt->timer = wl_event_loop_add_timer(loop,
drm_output_watchdog_bark,
ec->watchdog_timer_timeout);
blah..blah...
Yes that is what I meant.
If so, then I think I'd prefer to completely remove
'drm_output_watchdog_timer_create" and inline it directly from
'create_output_for_connector', since it's not a very big function anyway
and we're not sure as of today that we're going to reuse this for
anything else.
Probably that is ok as well. The main problem is that it is impossible
to inline the "bark" function so one part of the code is in a
different place, which may mean it is clearer to put the reference to
that function up next to it too.
what do you think ?
I'm usually not a big fan of static functions with hard-coded values,
but if you tell me it's better for clarity... I might be tempted, dunno.
Generally I don't think a generic function should be written until you
have at least 2 instances where it is used.
I think you won me over... Let's pretend we're not going to use that for
anything else for now.
patch v2 coming up next :)
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [WESTON PATCH 2/2] compositor-drm: Watchdog timer implementation
On 10/29/2014 12:19 PM, Frederic Plourde wrote:
Mhh... you mean something like :
static int
drm_output_watchdog_timer_create(struct drm_output *output)
{
struct wl_event_loop *loop = NULL;
struct weston_compositor *ec = output_base->compositor;
loop = wl_display_get_event_loop(ec->wl_display);
if (loop) {
output->wdt->timer = wl_event_loop_add_timer(loop,
drm_output_watchdog_bark,
ec->watchdog_timer_timeout);
blah..blah...
Yes that is what I meant.
If so, then I think I'd prefer to completely remove
'drm_output_watchdog_timer_create" and inline it directly from
'create_output_for_connector', since it's not a very big function anyway
and we're not sure as of today that we're going to reuse this for
anything else.
Probably that is ok as well. The main problem is that it is impossible
to inline the "bark" function so one part of the code is in a different
place, which may mean it is clearer to put the reference to that
function up next to it too.
what do you think ?
I'm usually not a big fan of static functions with hard-coded values,
but if you tell me it's better for clarity... I might be tempted, dunno.
Generally I don't think a generic function should be written until you
have at least 2 instances where it is used.
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [WESTON PATCH 2/2] compositor-drm: Watchdog timer implementation
On 14-10-29 02:45 PM, Bill Spitzak wrote:
On 10/29/2014 11:13 AM, Frederic Plourde wrote:
This watchdog timer (WDT) implementation is software only and includes
basic features usually found in a WDT. We simply exit Weston gracefully
with an exit code when the dog barks.
The watchdog timeout value can be set via weston.ini by adding a
watchdog-timer-timeout= entry under a new [compositor-drm]
section. Setting this timeout to 0 disables the watchdog feature.
+/* Creates a watchdog timer. Note that timer is not armed by default */
+static int
+drm_output_watchdog_timer_create(struct watchdog_timer *wdt, struct
wl_display *display,
+ wl_event_loop_timer_func_t timeout_func, void
*timer_arg,
+ uint32_t timeout)
+{...
I think it would be better to remove all these arguments and just make
this function directly use the values that are being passed to it now.
Right now it is a bit hard to find what function is called on timeout
because I have to locate the call to this function. Also this should
print to the log the error rather than the caller doing so.
Mhh... you mean something like :
static int
drm_output_watchdog_timer_create(struct drm_output *output)
{
struct wl_event_loop *loop = NULL;
struct weston_compositor *ec = output_base->compositor;
loop = wl_display_get_event_loop(ec->wl_display);
if (loop) {
output->wdt->timer = wl_event_loop_add_timer(loop,
drm_output_watchdog_bark,
ec->watchdog_timer_timeout);
blah..blah...
?
If so, then I think I'd prefer to completely remove
'drm_output_watchdog_timer_create" and inline it directly from
'create_output_for_connector', since it's not a very big function anyway
and we're not sure as of today that we're going to reuse this for
anything else.
In that case, I'd also remove 'drm_output_watchdog_timer_destroy' and
inline it in 'drm_output_destroy'
what do you think ?
I'm usually not a big fan of static functions with hard-coded values,
but if you tell me it's better for clarity... I might be tempted, dunno.
+/* Start/reset/arm the dog's timer. This can also be used to 'pat'
the dog */
+static int
+drm_output_watchdog_timer_start(struct watchdog_timer *wdt)
+{
+if (wdt->timer)
+return wl_event_source_timer_update(wdt->timer, wdt->timeout);
+
+return -1;
+}
Instead of the caller doing the logging I would do it here. You could
skip the log message if it failed to create the timer earlier since
that would have printed a message.
fair.
IMHO this is a better method for making these functions as long as
they are static and not intended to be reused.
Also wondering if weston_compositor_exit_with_code could be avoided by
fixing things to use atexit and then directly calling exit(). Or at
least fix it up enough that when you want to exit with an error it is
ok to skip the cleanup code that was missed.
I'd like to hear Pekka on that one ;-)
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [WESTON PATCH 2/2] compositor-drm: Watchdog timer implementation
On 10/29/2014 11:13 AM, Frederic Plourde wrote:
This watchdog timer (WDT) implementation is software only and includes
basic features usually found in a WDT. We simply exit Weston gracefully
with an exit code when the dog barks.
The watchdog timeout value can be set via weston.ini by adding a
watchdog-timer-timeout= entry under a new [compositor-drm]
section. Setting this timeout to 0 disables the watchdog feature.
+/* Creates a watchdog timer. Note that timer is not armed by default */
+static int
+drm_output_watchdog_timer_create(struct watchdog_timer *wdt, struct wl_display
*display,
+ wl_event_loop_timer_func_t
timeout_func, void *timer_arg,
+ uint32_t timeout)
+{...
I think it would be better to remove all these arguments and just make
this function directly use the values that are being passed to it now.
Right now it is a bit hard to find what function is called on timeout
because I have to locate the call to this function. Also this should
print to the log the error rather than the caller doing so.
+/* Start/reset/arm the dog's timer. This can also be used to 'pat' the dog */
+static int
+drm_output_watchdog_timer_start(struct watchdog_timer *wdt)
+{
+ if (wdt->timer)
+ return wl_event_source_timer_update(wdt->timer, wdt->timeout);
+
+ return -1;
+}
Instead of the caller doing the logging I would do it here. You could
skip the log message if it failed to create the timer earlier since that
would have printed a message.
IMHO this is a better method for making these functions as long as they
are static and not intended to be reused.
Also wondering if weston_compositor_exit_with_code could be avoided by
fixing things to use atexit and then directly calling exit(). Or at
least fix it up enough that when you want to exit with an error it is ok
to skip the cleanup code that was missed.
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
