Re: [PATCH weston v5 30/42] compositor-drm: Introduce drm_output_state structure

[Fabien DESSENNE]

On 08/12/16 13:58, Daniel Stone wrote:
> Hi Fabien,
>
> On 21 November 2016 at 14:58, Fabien DESSENNE  wrote:
>> On 11/16/2016 03:25 PM, Daniel Stone wrote:
>>> +/**
>>> + * Mark a drm_output_state (the output's last state) as complete. This 
>>> handles
>>> + * any post-completion actions such as updating the repaint timer, 
>>> disabling the
>>> + * output, and finally freeing the state.
>>> + */
>>> +static void
>>> +drm_output_update_complete(struct drm_output *output, uint32_t flags,
>>> +  unsigned int sec, unsigned int usec)
>>> +{
>>> +   struct timespec ts;
>>> +
>>> +   drm_output_state_free(output->state_last);
>>> +   output->state_last = NULL;
>>> +
>>> +   if (output->destroy_pending) {
>>> +   drm_output_destroy(>base);
>>> +   goto out;
>>> +   }
>>> +   else if (output->disable_pending) {
>> Remove 'else'
> You're right that it isn't strictly necessary, as the 'goto' will jump
> away from it and make these branches mutually exclusive anyway. But I
> do like that it makes things clearer at a first read ('right, only one
> of these branches will ever be executed at a time'), and also it does
> match what the previous code this moved was doing.

OK, no problem Daniel. You may also consider keeping "} else if (...) {" 
on the same line.
BR
Fabien

>
> Cheers,
> Daniel
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH weston v5 30/42] compositor-drm: Introduce drm_output_state structure

[Daniel Stone]

Hi Fabien,

On 21 November 2016 at 14:58, Fabien DESSENNE  wrote:
> On 11/16/2016 03:25 PM, Daniel Stone wrote:
>> +/**
>> + * Mark a drm_output_state (the output's last state) as complete. This 
>> handles
>> + * any post-completion actions such as updating the repaint timer, 
>> disabling the
>> + * output, and finally freeing the state.
>> + */
>> +static void
>> +drm_output_update_complete(struct drm_output *output, uint32_t flags,
>> +  unsigned int sec, unsigned int usec)
>> +{
>> +   struct timespec ts;
>> +
>> +   drm_output_state_free(output->state_last);
>> +   output->state_last = NULL;
>> +
>> +   if (output->destroy_pending) {
>> +   drm_output_destroy(>base);
>> +   goto out;
>> +   }
>> +   else if (output->disable_pending) {
>
> Remove 'else'

You're right that it isn't strictly necessary, as the 'goto' will jump
away from it and make these branches mutually exclusive anyway. But I
do like that it makes things clearer at a first read ('right, only one
of these branches will ever be executed at a time'), and also it does
match what the previous code this moved was doing.

Cheers,
Daniel
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH weston v5 30/42] compositor-drm: Introduce drm_output_state structure

[Fabien DESSENNE]

Hi Daniel



On 11/16/2016 03:25 PM, Daniel Stone wrote:
> Currently this doesn't actually really do anything, but will be used in
> the future to track the state for both modeset and repaint requests.
> Completion of the request gives us a single request-completion path for
> both pageflip and vblank events.
>
> Signed-off-by: Daniel Stone 
>
> Differential Revision: https://phabricator.freedesktop.org/D1497
> ---
>   libweston/compositor-drm.c | 230 
> ++---
>   1 file changed, 197 insertions(+), 33 deletions(-)
>
> diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
> index c24129b..1089d77 100644
> --- a/libweston/compositor-drm.c
> +++ b/libweston/compositor-drm.c
> @@ -120,6 +120,19 @@ struct plane_properties {
>  uint64_t value;
>   };
>
> +/**
> + * Mode for drm_output_state_duplicate.
> + */
> +enum drm_output_state_duplicate_mode {
> +   DRM_OUTPUT_STATE_CLEAR_PLANES, /**< reset all planes to off */
> +   DRM_OUTPUT_STATE_PRESERVE_PLANES, /**< preserve plane state */
> +};
> +
> +enum drm_output_state_update_mode {
> +   DRM_OUTPUT_STATE_UPDATE_SYNCHRONOUS, /**< state already applied */
> +   DRM_OUTPUT_STATE_UPDATE_ASYNCHRONOUS, /**< pending event delivery */
> +};
> +
>   struct drm_backend {
>  struct weston_backend base;
>  struct weston_compositor *compositor;
> @@ -206,6 +219,16 @@ struct drm_edid {
>   };
>
>   /**
> + * Output state holds the dynamic state for one Weston output, i.e. a KMS 
> CRTC,
> + * plus >= 1 each of encoder/connector/plane. Since everything but the planes
> + * is currently statically assigned per-output, we mainly use this to track
> + * plane state.
> + */
> +struct drm_output_state {
> +   struct drm_output *output;
> +};
> +
> +/**
>* A plane represents one buffer, positioned within a CRTC, and stacked
>* relative to other planes on the same CRTC.
>*
> @@ -274,6 +297,10 @@ struct drm_output {
>  struct drm_fb *fb_current, *fb_pending, *fb_last;
>  struct backlight *backlight;
>
> +   struct drm_output_state *state_last;
> +   struct drm_output_state *state_cur;
> +   struct drm_output_state *state_pending;
> +
>  struct drm_fb *dumb[2];
>  pixman_image_t *image[2];
>  int current_image;
> @@ -641,6 +668,9 @@ drm_output_set_cursor(struct drm_output *output);
>   static void
>   drm_output_update_msc(struct drm_output *output, unsigned int seq);
>
> +static void
> +drm_output_destroy(struct weston_output *output_base);
> +
>   static int
>   drm_plane_crtc_supported(struct drm_output *output, struct drm_plane *plane)
>   {
> @@ -890,6 +920,113 @@ drm_fb_unref(struct drm_fb *fb)
>  }
>   }
>
> +/**
> + * Allocate a new, empty drm_output_state. This should not generally be used
> + * in the repaint cycle; see drm_output_state_duplicate.
> + */
> +static struct drm_output_state *
> +drm_output_state_alloc(struct drm_output *output)
> +{
> +   struct drm_output_state *state = calloc(1, sizeof(*state));
> +
> +   state->output = output;
> +
> +   return state;
> +}
> +
> +/**
> + * Duplicate an existing drm_output_state into a new one. This is generally
> + * used during the repaint cycle, to capture the existing state of an output
> + * and modify it to create a new state to be used.
> + *
> + * The mode determines whether the output will be reset to an a blank state,
> + * or an exact mirror of the current state.
> + */
> +static struct drm_output_state *
> +drm_output_state_duplicate(struct drm_output_state *src,
> +  enum drm_output_state_duplicate_mode plane_mode)
> +{
> +   struct drm_output_state *dst = malloc(sizeof(*dst));
> +
> +   assert(dst);
> +   memcpy(dst, src, sizeof(*dst));
> +
> +   return dst;
> +}
> +
> +/**
> + * Free an unused drm_output_state.
> + */
> +static void
> +drm_output_state_free(struct drm_output_state *state)
> +{
> +   if (!state)
> +   return;
> +
> +   free(state);
> +}
> +
> +/**
> + * Mark a drm_output_state (the output's last state) as complete. This 
> handles
> + * any post-completion actions such as updating the repaint timer, disabling 
> the
> + * output, and finally freeing the state.
> + */
> +static void
> +drm_output_update_complete(struct drm_output *output, uint32_t flags,
> +  unsigned int sec, unsigned int usec)
> +{
> +   struct timespec ts;
> +
> +   drm_output_state_free(output->state_last);
> +   output->state_last = NULL;
> +
> +   if (output->destroy_pending) {
> +   drm_output_destroy(>base);
> +   goto out;
> +   }
> +   else if (output->disable_pending) {

Remove 'else'

> +   weston_output_disable(>base);
> +   goto out;
> +   }
> +
> +   ts.tv_sec = sec;
> +   ts.tv_nsec = usec * 1000;
> +   weston_output_finish_frame(>base, , 

[PATCH weston v5 30/42] compositor-drm: Introduce drm_output_state structure

[Daniel Stone]

Currently this doesn't actually really do anything, but will be used in
the future to track the state for both modeset and repaint requests.
Completion of the request gives us a single request-completion path for
both pageflip and vblank events.

Signed-off-by: Daniel Stone 

Differential Revision: https://phabricator.freedesktop.org/D1497
---
 libweston/compositor-drm.c | 230 ++---
 1 file changed, 197 insertions(+), 33 deletions(-)

diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
index c24129b..1089d77 100644
--- a/libweston/compositor-drm.c
+++ b/libweston/compositor-drm.c
@@ -120,6 +120,19 @@ struct plane_properties {
uint64_t value;
 };
 
+/**
+ * Mode for drm_output_state_duplicate.
+ */
+enum drm_output_state_duplicate_mode {
+   DRM_OUTPUT_STATE_CLEAR_PLANES, /**< reset all planes to off */
+   DRM_OUTPUT_STATE_PRESERVE_PLANES, /**< preserve plane state */
+};
+
+enum drm_output_state_update_mode {
+   DRM_OUTPUT_STATE_UPDATE_SYNCHRONOUS, /**< state already applied */
+   DRM_OUTPUT_STATE_UPDATE_ASYNCHRONOUS, /**< pending event delivery */
+};
+
 struct drm_backend {
struct weston_backend base;
struct weston_compositor *compositor;
@@ -206,6 +219,16 @@ struct drm_edid {
 };
 
 /**
+ * Output state holds the dynamic state for one Weston output, i.e. a KMS CRTC,
+ * plus >= 1 each of encoder/connector/plane. Since everything but the planes
+ * is currently statically assigned per-output, we mainly use this to track
+ * plane state.
+ */
+struct drm_output_state {
+   struct drm_output *output;
+};
+
+/**
  * A plane represents one buffer, positioned within a CRTC, and stacked
  * relative to other planes on the same CRTC.
  *
@@ -274,6 +297,10 @@ struct drm_output {
struct drm_fb *fb_current, *fb_pending, *fb_last;
struct backlight *backlight;
 
+   struct drm_output_state *state_last;
+   struct drm_output_state *state_cur;
+   struct drm_output_state *state_pending;
+
struct drm_fb *dumb[2];
pixman_image_t *image[2];
int current_image;
@@ -641,6 +668,9 @@ drm_output_set_cursor(struct drm_output *output);
 static void
 drm_output_update_msc(struct drm_output *output, unsigned int seq);
 
+static void
+drm_output_destroy(struct weston_output *output_base);
+
 static int
 drm_plane_crtc_supported(struct drm_output *output, struct drm_plane *plane)
 {
@@ -890,6 +920,113 @@ drm_fb_unref(struct drm_fb *fb)
}
 }
 
+/**
+ * Allocate a new, empty drm_output_state. This should not generally be used
+ * in the repaint cycle; see drm_output_state_duplicate.
+ */
+static struct drm_output_state *
+drm_output_state_alloc(struct drm_output *output)
+{
+   struct drm_output_state *state = calloc(1, sizeof(*state));
+
+   state->output = output;
+
+   return state;
+}
+
+/**
+ * Duplicate an existing drm_output_state into a new one. This is generally
+ * used during the repaint cycle, to capture the existing state of an output
+ * and modify it to create a new state to be used.
+ *
+ * The mode determines whether the output will be reset to an a blank state,
+ * or an exact mirror of the current state.
+ */
+static struct drm_output_state *
+drm_output_state_duplicate(struct drm_output_state *src,
+  enum drm_output_state_duplicate_mode plane_mode)
+{
+   struct drm_output_state *dst = malloc(sizeof(*dst));
+
+   assert(dst);
+   memcpy(dst, src, sizeof(*dst));
+
+   return dst;
+}
+
+/**
+ * Free an unused drm_output_state.
+ */
+static void
+drm_output_state_free(struct drm_output_state *state)
+{
+   if (!state)
+   return;
+
+   free(state);
+}
+
+/**
+ * Mark a drm_output_state (the output's last state) as complete. This handles
+ * any post-completion actions such as updating the repaint timer, disabling 
the
+ * output, and finally freeing the state.
+ */
+static void
+drm_output_update_complete(struct drm_output *output, uint32_t flags,
+  unsigned int sec, unsigned int usec)
+{
+   struct timespec ts;
+
+   drm_output_state_free(output->state_last);
+   output->state_last = NULL;
+
+   if (output->destroy_pending) {
+   drm_output_destroy(>base);
+   goto out;
+   }
+   else if (output->disable_pending) {
+   weston_output_disable(>base);
+   goto out;
+   }
+
+   ts.tv_sec = sec;
+   ts.tv_nsec = usec * 1000;
+   weston_output_finish_frame(>base, , flags);
+
+   /* We can't call this from frame_notify, because the output's
+* repaint needed flag is cleared just after that */
+   if (output->recorder)
+   weston_output_schedule_repaint(>base);
+
+out:
+   output->destroy_pending = 0;
+   output->disable_pending = 0;
+}
+
+/**
+ * Mark an output state as current on the output, i.e. it has been
+ *