[PATCH 2/5] drm: port the drm backend to the new init api
Pekka Paalanen
ppaalanen at gmail.com
Thu Apr 7 07:37:49 UTC 2016
On Thu, 7 Apr 2016 09:04:46 +0300
Giulio Camuffo <giuliocamuffo at gmail.com> wrote:
> 2016-04-06 11:37 GMT+03:00 Pekka Paalanen <ppaalanen at gmail.com>:
> > On Wed, 9 Mar 2016 16:49:29 -0800
> > Bryce Harrington <bryce at osg.samsung.com> wrote:
> >
> >> From: Giulio Camuffo <giuliocamuffo at gmail.com>
> >>
> >> Signed-off-by: Bryce Harrington <bryce at osg.samsung.com>
> >> Reviewed-by: Quentin Glidic <sardemff7+git at sardemff7.net>
> >> Acked-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
> >> ---
> >> v4: Update to current trunk
> >> - Add missing param doc for mode in drm_output_choose_initial_mode
> >> - Rebase to account for code changes by 91880f1e to make vt
> >> switching configurable.
> >>
> >> Makefile.am | 3 +
> >> src/compositor-drm.c | 196 ++++++++++++++++++---------------------------------
> >> src/compositor.h | 2 -
> >> src/main.c | 94 +++++++++++++++++++++++-
> >> 4 files changed, 165 insertions(+), 130 deletions(-)
> >
> > Hi Giulio and Bryce,
> >
> > I'm sorry it has taken so long for me to come back to this.
> >
> >> diff --git a/Makefile.am b/Makefile.am
> >> index fe08d94..9330f0e 100644
> >> --- a/Makefile.am
> >> +++ b/Makefile.am
> >> @@ -72,6 +72,7 @@ weston_SOURCES = \
> >> src/log.c \
> >> src/compositor.c \
> >> src/compositor.h \
> >> + src/compositor-drm.h \
> >> src/input.c \
> >> src/data-device.c \
> >> src/screenshooter.c \
> >> @@ -207,6 +208,7 @@ westonincludedir = $(includedir)/weston
> >> westoninclude_HEADERS = \
> >> src/version.h \
> >> src/compositor.h \
> >> + src/compositor-drm.h \
> >> src/timeline-object.h \
> >> shared/matrix.h \
> >> shared/config-parser.h \
> >> @@ -276,6 +278,7 @@ drm_backend_la_CFLAGS = \
> >> $(AM_CFLAGS)
> >> drm_backend_la_SOURCES = \
> >> src/compositor-drm.c \
> >> + src/compositor-drm.h \
> >> $(INPUT_BACKEND_SOURCES) \
> >> shared/helpers.h \
> >> shared/timespec-util.h \
> >> diff --git a/src/compositor-drm.c b/src/compositor-drm.c
> >> index e01f6b9..111c882 100644
> >> --- a/src/compositor-drm.c
> >> +++ b/src/compositor-drm.c
> >
> >> @@ -2185,30 +2162,45 @@ get_gbm_format_from_section(struct weston_config_section *section,
> >> * Find the most suitable mode to use for initial setup (or reconfiguration on
> >> * hotplug etc) for a DRM output.
> >> *
> >> + * @param backend The DRM backend object
> >> * @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 mode Controls how to select the mode
> >> + * @param config Desired configuration for the 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)
> >> +drm_output_choose_initial_mode(struct drm_backend *backend,
> >> + struct drm_output *output,
> >> + enum weston_drm_backend_output_mode mode,
> >> + struct weston_drm_backend_output_config *config,
> >> + const drmModeModeInfo *current_mode)
> >> {
> >> 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;
> >> + drmModeModeInfo modeline;
> >> + int32_t width, height;
> >> +
> >> + if (mode == WESTON_DRM_BACKEND_OUTPUT_PREFERRED && config->modeline) {
> >> + if (sscanf(config->modeline, "%dx%d", &width, &height) != 2) {
> >> + width = -1;
> >> +
> >> + if (parse_modeline(config->modeline, &modeline) == 0) {
> >> + configured = drm_output_add_mode(output, &modeline);
> >> + if (!configured)
> >> + return NULL;
> >> + } else {
> >> + weston_log("Invalid modeline \"%s\" for output %s\n",
> >> + config->modeline, output->base.name);
> >> + }
> >> + }
> >> + }
> >
> > Hmm, this logic above looks a little odd to me. If mode=PREFERRED means
> > the user wants the preferred mode, why would it look at modeline at all?
> >
> > Is this changing the meaning of modeline to override the PREFERRED
> > setting?
> >
> > Maybe this is an API refactorization artifact, since originally we got
> > a single config file key "mode" which determined several arguments used
> > here, so if the caller (main) keeps working the same way as before, it
> > is impossible to have both mode=PREFERRED and modeline set at the same
> > time.
> >
> > Am I missing something?
>
> This is the definition of the relevant enum in compositor-drm.h:
>
> enum weston_drm_backend_output_mode {
> /** The output is disabled */
> WESTON_DRM_BACKEND_OUTPUT_OFF,
> /** The output will use the current active mode */
> WESTON_DRM_BACKEND_OUTPUT_CURRENT,
> /** The output will use the preferred mode. A modeline can be provided
> * by setting weston_backend_output_config::modeline in the form of
> * "WIDTHxHEIGHT" or in the form of an explicit modeline calculated
> * using e.g. the cvt tool. If a valid modeline is supplied it will be
> * used, if invalid or NULL the preferred available mode will be used. */
> WESTON_DRM_BACKEND_OUTPUT_PREFERRED,
> };
>
> So WESTON_DRM_BACKEND_OUTPUT_PREFERRED is not equivalent to
> DRM_MODE_TYPE_PREFERRED. Instead of having both a PREFERRED and
> MODELINE entries in that enum PREFERRED is used for both, and if the
> given modeline is NULL it has the same meaning as
> DRM_MODE_TYPE_PREFERRED.
> So maybe the name is not the best one.
> WESTON_DRM_BACKEND_OUTPUT_PREFERRED_OR_MODELINE would be more
> explicit.
Ok. The naming is a bit surprising, but the documentation is there and
explains it. That's good enough for now, IMO.
> >
> > If this to add a new feature of being able to add custom modes without
> > actually setting them? If so, I would split that into a separate patch,
> > since it's a new feature and not about just porting to the new API. And
> > in that case one might also want to set multiple new custom modes, so
> > this isn't even a sufficient API to my understanding.
> >
> > I would think that if mode=PREFERRED, we get the EDID mode tagged as
> > preferred, period. If mode=MODELINE (I forget what the alternatives
> > actually were), then we inspect the modeline argument and set that.
>
> This is how it was in a previous iteration of the patch iirc, but
> while it was ok as a weston internal api i think that as a library api
> it would be less than optimal. Using MODELINE without providing a
> modeline doesn't make sense, and also using PREFERRED while providing
> a modeline doesn't. And then we also had MODE, which was different
> from MODELINE, and all expected the modeline string to have different
> formats. So instead i think a simple enum with specified behavior
> depending to the modeline format is simpler.
I'm ok for MODE(LINE) parsing both an actual modeline and just a "WxH"
or "WxH at R" shorthands.
However, I would like the explicitness of PREFERRED vs. MODE(LINE).
Yes, the caller must then set the modeline field according to the enum
chosen, but I don't see that as a bad thing. Let's just add a sanity
check that enforces the enum to match modeline field set/NULL.
Not all combinations of the config struct fields' values need to be
allowed. I think it is even desireable to define enforced dependencies
between related fields, so that we don't have to support combinations
we never even thought of.
Anyway, I won't require this change now. The implementation is as
documented, and that's fine.
If we want to be able to inject several custom modelines, all this will
change again, so not worth the hassle right now. In fact,
choose_initial_mode would actually belong in the main.c side, IMO, but
let's not go that far now. It would be a major change to the whole way
outputs are being taken into use.
Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 811 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20160407/14931ea0/attachment-0001.sig>
More information about the wayland-devel
mailing list