On Fri, Jun 26, 2015 at 01:57:22PM -0500, Derek Foreman wrote:
> On 22/06/15 11:25 AM, Daniel Stone wrote:
> > From: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
> >
> > Refactor the code for choosing the initial mode for an output from
> > create_output_for_connector() to drm_output_choose_initial_mode().
> >
> > This makes create_output_for_connector() slightly easier to read.
> >
> > v2: Document everything.
> >
> > Signed-off-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
> > Signed-off-by: Daniel Stone <daniels at collabora.com>
> > ---
> > src/compositor-drm.c | 168 ++++++++++++++++++++++++++++++++++++---------------
> > 1 file changed, 119 insertions(+), 49 deletions(-)
> >
> > diff --git a/src/compositor-drm.c b/src/compositor-drm.c
> > index 6d8684d..60f07f1 100644
> > --- a/src/compositor-drm.c
> > +++ b/src/compositor-drm.c
> > @@ -1211,6 +1211,17 @@ drm_output_destroy(struct weston_output *output_base)
> > free(output);
> > }
> >
> > +/**
> > + * Find the closest-matching mode for a given target
> > + *
> > + * Given a target mode, find the most suitable mode amongst the output's
> > + * current mode list to use, preferring the current mode if possible, to
> > + * avoid an expensive mode switch.
> > + *
> > + * @param output DRM output
> > + * @param target_mode Mode to attempt to match
> > + * @returns Pointer to a mode from the output's mode list
> > + */
>
> I guess the pedant in me would rather see the "unrelated" doc hunks in a
> separate patch.
>
> Either way,
> Reviewed-By: Derek Foreman <derekf at osg.samsung.com
I don't care about separating out the doc hunks. Docs are good and this
formats them well.
This organizes the code more cleanly and adds docs. I'd be fine seeing
this landed early before the rest of the patchset, but it appears to no
longer apply. If it's updated to current trunk I'd be +1 to landing it
asap, so you don't have to waste more time maintaining it.
Bryce
> > static struct drm_mode *
> > choose_mode (struct drm_output *output, struct weston_mode *target_mode)
> > {
> > @@ -1463,8 +1474,18 @@ init_pixman(struct drm_compositor *ec)
> > return pixman_renderer_init(&ec->base);
> > }
> >
> > +/**
> > + * Add a mode to output's mode list
> > + *
> > + * Copy the supplied DRM mode into a Weston mode structure, and add it to the
> > + * output's mode list.
> > + *
> > + * @param output DRM output to add mode to
> > + * @param info DRM mode structure to add
> > + * @returns Newly-allocated Weston/DRM mode structure
> > + */
> > static struct drm_mode *
> > -drm_output_add_mode(struct drm_output *output, drmModeModeInfo *info)
> > +drm_output_add_mode(struct drm_output *output, const drmModeModeInfo *info)
> > {
> > struct drm_mode *mode;
> > uint64_t refresh;
> > @@ -1977,6 +1998,97 @@ get_gbm_format_from_section(struct weston_config_section *section,
> > return ret;
> > }
> >
> > +/**
> > + * Choose suitable mode for an output
> > + *
> > + * Find the most suitable mode to use for initial setup (or reconfiguration on
> > + * hotplug etc) for a DRM output.
> > + *
> > + * @param output DRM output to choose mode for
> > + * @param kind Strategy and preference to use when choosing mode
> > + * @param width Desired width for this output
> > + * @param height Desired height for this output
> > + * @param current_mode Mode currently being displayed on this output
> > + * @param modeline Manually-entered mode (may be NULL)
> > + * @returns A mode from the output's mode list, or NULL if none available
> > + */
> > +static struct drm_mode *
> > +drm_output_choose_initial_mode(struct drm_output *output,
> > + enum output_config kind,
> > + int width, int height,
> > + const drmModeModeInfo *current_mode,
> > + const drmModeModeInfo *modeline)
> > +{
> > + struct drm_mode *preferred = NULL;
> > + struct drm_mode *current = NULL;
> > + struct drm_mode *configured = NULL;
> > + struct drm_mode *best = NULL;
> > + struct drm_mode *drm_mode;
> > +
> > + wl_list_for_each_reverse(drm_mode, &output->base.mode_list, base.link) {
> > + if (kind == OUTPUT_CONFIG_MODE &&
> > + width == drm_mode->base.width &&
> > + height == drm_mode->base.height)
> > + configured = drm_mode;
> > +
> > + if (memcmp(¤t_mode, &drm_mode->mode_info,
> > + sizeof *current_mode) == 0)
> > + current = drm_mode;
> > +
> > + if (drm_mode->base.flags & WL_OUTPUT_MODE_PREFERRED)
> > + preferred = drm_mode;
> > +
> > + best = drm_mode;
> > + }
> > +
> > + if (kind == OUTPUT_CONFIG_MODELINE) {
> > + configured = drm_output_add_mode(output, modeline);
> > + if (!configured)
> > + return NULL;
> > + }
> > +
> > + if (current == NULL && current_mode->clock != 0) {
> > + current = drm_output_add_mode(output, current_mode);
> > + if (!current)
> > + return NULL;
> > + }
> > +
> > + if (kind == OUTPUT_CONFIG_CURRENT)
> > + configured = current;
> > +
> > + if (option_current_mode && current)
> > + return current;
> > +
> > + if (configured)
> > + return configured;
> > +
> > + if (preferred)
> > + return preferred;
> > +
> > + if (current)
> > + return current;
> > +
> > + if (best)
> > + return best;
> > +
> > + weston_log("no available modes for %s\n", output->base.name);
> > + return NULL;
> > +}
> > +
> > +/**
> > + * Create and configure a Weston output structure
> > + *
> > + * Given a DRM connector, create a matching drm_output structure and add it
> > + * to Weston's output list.
> > + *
> > + * @param ec DRM compositor structure
> > + * @param resources DRM resources for this device
> > + * @param connector DRM connector to use for this new output
> > + * @param x Horizontal offset to use into global co-ordinate space
> > + * @param y Vertical offset to use into global co-ordinate space
> > + * @param drm_device udev device pointer
> > + * @returns 0 on success, or -1 on failure
> > + */
> > static int
> > create_output_for_connector(struct drm_compositor *ec,
> > drmModeRes *resources,
> > @@ -1984,7 +2096,7 @@ create_output_for_connector(struct drm_compositor *ec,
> > int x, int y, struct udev_device *drm_device)
> > {
> > struct drm_output *output;
> > - struct drm_mode *drm_mode, *next, *preferred, *current, *configured, *best;
> > + struct drm_mode *drm_mode, *next, *current;
> > struct weston_mode *m;
> > struct weston_config_section *section;
> > drmModeEncoder *encoder;
> > @@ -2092,54 +2204,12 @@ create_output_for_connector(struct drm_compositor *ec,
> > goto err_free;
> > }
> >
> > - preferred = NULL;
> > - current = NULL;
> > - configured = NULL;
> > - best = NULL;
> > -
> > - wl_list_for_each_reverse(drm_mode, &output->base.mode_list, base.link) {
> > - if (config == OUTPUT_CONFIG_MODE &&
> > - width == drm_mode->base.width &&
> > - height == drm_mode->base.height)
> > - configured = drm_mode;
> > - if (!memcmp(&crtc_mode, &drm_mode->mode_info, sizeof crtc_mode))
> > - current = drm_mode;
> > - if (drm_mode->base.flags & WL_OUTPUT_MODE_PREFERRED)
> > - preferred = drm_mode;
> > - best = drm_mode;
> > - }
> > -
> > - if (config == OUTPUT_CONFIG_MODELINE) {
> > - configured = drm_output_add_mode(output, &modeline);
> > - if (!configured)
> > - goto err_free;
> > - }
> > -
> > - if (current == NULL && crtc_mode.clock != 0) {
> > - current = drm_output_add_mode(output, &crtc_mode);
> > - if (!current)
> > - goto err_free;
> > - }
> > -
> > - if (config == OUTPUT_CONFIG_CURRENT)
> > - configured = current;
> > -
> > - if (option_current_mode && current)
> > - output->base.current_mode = ¤t->base;
> > - else if (configured)
> > - output->base.current_mode = &configured->base;
> > - else if (preferred)
> > - output->base.current_mode = &preferred->base;
> > - else if (current)
> > - output->base.current_mode = ¤t->base;
> > - else if (best)
> > - output->base.current_mode = &best->base;
> > -
> > - if (output->base.current_mode == NULL) {
> > - weston_log("no available modes for %s\n", output->base.name);
> > + current = drm_output_choose_initial_mode(output, config,
> > + width, height,
> > + &crtc_mode, &modeline);
> > + if (!current)
> > goto err_free;
> > - }
> > -
> > + output->base.current_mode = ¤t->base;
> > output->base.current_mode->flags |= WL_OUTPUT_MODE_CURRENT;
> >
> > weston_output_init(&output->base, &ec->base, x, y,
> >
>
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel