Re: [RFC PATCH libinput] tools: run list-quirks from the git data when running from the builddir
On Mon, Jun 25, 2018 at 11:06:36AM +0300, Pekka Paalanen wrote:
> On Fri, 22 Jun 2018 12:48:53 +1000
> Peter Hutterer wrote:
>
> > I'd like some comment on this from someone who knows this stuff better than
> > I do. The main goal is to have the same binary default to different search
> > paths, depending whether it's the installed version or the one run from the
> > build directory. This is the only solution I could come up with and it
> > works, but I'm not sure how much of that is coincidence vs "yes, this is how
> > ELF stripping works".
> >
> > Any comments welcome. Or other solutions to this. Thanks
> >
> > ---
> >
> > Push the git data directory into a special variable in a custom ELF section.
> > Then after install, strip that ELF section out. The result is that when
> > we're
> > running from the builddir, the data path is set and we read from there. When
> > running from the system install, the data path is zeroes only and we're
> > running with the defaults.
>
> Hi Peter,
>
> this is certainly interesting. I don't know about the correctness, but
> I have been wondering about how to disable the test suite scaffolding
> (e.g. WESTON_MODULE_MAP env var) in an installed weston instance.
>
> I've also been thinking about how to expose private functions from a
> library for a test suite but prevent those from being accessed in an
> installed version. Maybe that could be an array of function pointers
> declared in a special section like you did, so it would disappear on
> install.
Ok, with Dodji Seketeli's help I figured out the trick and the various
pieces needed to get this working correctly.
Part 1, the const char in the custom section:
static const char valuestring[]
__attribute__ ((section ("custom_section_name")))
= "SOME VALUE";
Part 2, accessing that value:
volatile const char *foo = valuestring;
if (*foo == '\0') printf("section was removed\n");
Note that foo must be declared volatile, otherwise the compiler will
optimise the code and replace *foo with the first character of the string
('S' in this case) and this all fails. This happens even at gcc -O0 but the
above is correct for -O0 and -O2 (after staring at the disassembly).
If you pass the lot into a function like e.g. strlen() this is not an issue
because the compiler won't optimise that out. But I didn't check that in
detail.
Part 3, removal of the section:
dd if=/dev/zero of=zeroes.bin bs=8 count=1
objcopy --update-section custom_section_name=zeroes.bin $file $file.tmp
mv $file.tmp $file
This *replaces* the content of the section with 8 zero bytes. Which should
be enough to avoid accidental access of the subsequent region.
I don't have any immediate use for it because for list-quirks I went with
the "subtract the prefix" approach. But it'll come in handy one day to
remove traces of the test scaffolding.
Cheers,
Peter
> > Signed-off-by: Peter Hutterer
> > ---
> > meson.build | 5 +
> > tools/libinput-list-quirks.c| 12
> > tools/strip-builddir-section.sh | 18 ++
> > 3 files changed, 35 insertions(+)
> > create mode 100644 tools/strip-builddir-section.sh
> >
> > diff --git a/meson.build b/meson.build
> > index b650bbde..0b9dc751 100644
> > --- a/meson.build
> > +++ b/meson.build
> > @@ -479,6 +479,11 @@ libinput_list_quirks =
> > executable('libinput-list-quirks',
> > install_dir : libinput_tool_path,
> > install : true
> > )
> > +meson.add_install_script('tools/strip-builddir-section.sh',
> > +libinput_tool_path,
> > +'libinput-list-quirks')
> > +
> > +
> > test('validate-quirks',
> > libinput_list_quirks,
> > args: ['--validate-only',
> > '--data-dir=@0@'.format(join_paths(meson.source_root(), 'data'))]
> > diff --git a/tools/libinput-list-quirks.c b/tools/libinput-list-quirks.c
> > index 533c4f3a..1207ade3 100644
> > --- a/tools/libinput-list-quirks.c
> > +++ b/tools/libinput-list-quirks.c
> > @@ -207,6 +207,13 @@ usage(void)
> >"Validate the database\n");
> > }
> >
> > +/* When running from builddir, we use the git data tree by default.
> > + This section is stripped on install.
> > + */
> > +static const char builddir_data_path[]
> > + __attribute__ ((section ("builddir_section")))
> > + = LIBINPUT_DATA_SRCDIR;
> > +
> > int
> > main(int argc, char **argv)
> > {
> > @@ -219,6 +226,11 @@ main(int argc, char **argv)
> > struct quirks_context *quirks;
> > bool validate = false;
> >
> > + /* After the section is stripped, builddir_data_path is zeroes */
> > + data_path = builddir_data_path;
> > + if (data_path && strlen(data_path) == 0)
> > + data_path = NULL;
> > +
> > while (1) {
> > int c;
> > int option_index = 0;
> > diff --git a/tools/strip-builddir-section.sh
> >
[PATCH v2 libinput 1/5] tools: if the execdir is the builddir, add it to the path
When running libinput tools from the builddir, look up the subtools in the
builddir as well. Otherwise, add the install prefix to the list of lookup
locations.
This ensures that a) we're running builddir stuff against builddir stuff, but
also b) that we're not running builddir stuff against installed stuff because
that may give us false positives.
The test was squashed in from a separate patch and was
Reviewed-by: Pekka Paalanen
Signed-off-by: Peter Hutterer
---
Changes to v1:
- squashed in the 6/5 patch for the test case pq reviewed
- had a bug with readlink which doesn't terminate the string, adding an
extra \0 here
- set LD_LIBRARY_PATH in the env for the builddir lookup test (the installed
version). Otherwise on a machine without libinput installed, we get
libinput.so lookup errors.
Also, yay, the CI suite found two bugs :)
Though weirdly enough, some of them only triggered on F28 and the readlink
one only triggered on F28 without libwacom. *shrug*
meson.build| 18 ++
tools/helper-copy-and-exec-from-tmp.sh | 18 ++
tools/shared.c | 35 ++-
tools/shared.h | 3 ++
tools/test-builddir-lookup.c | 47 ++
5 files changed, 120 insertions(+), 1 deletion(-)
create mode 100755 tools/helper-copy-and-exec-from-tmp.sh
create mode 100644 tools/test-builddir-lookup.c
diff --git a/meson.build b/meson.build
index 58e909b6..6f32c2f2 100644
--- a/meson.build
+++ b/meson.build
@@ -31,6 +31,7 @@ add_project_arguments(cppflags, language : 'cpp')
config_h = configuration_data()
config_h.set('_GNU_SOURCE', '1')
+config_h.set_quoted('MESON_BUILD_ROOT', meson.build_root())
prefix = '''#define _GNU_SOURCE 1
#include
@@ -671,6 +672,23 @@ executable('ptraccel-debug',
install : false
)
+# the libinput tools check whether we execute from the builddir, this is
+# the test to verify that lookup. We test twice, once as normal test
+# run from the builddir, once after copying to /tmp
+test_builddir_lookup = executable('test-builddir-lookup',
+ 'tools/test-builddir-lookup.c',
+ dependencies : [ dep_tools_shared],
+ include_directories : [includes_src,
includes_include],
+ install : false)
+test('tools-builddir-lookup',
+ test_builddir_lookup,
+ args : ['--builddir-is-set'])
+test('tools-builddir-lookup-installed',
+ find_program('tools/helper-copy-and-exec-from-tmp.sh'),
+ args : [test_builddir_lookup.full_path(), '--builddir-is-null'],
+ env : ['LD_LIBRARY_PATH=@0@'.format(meson.build_root())],
+ workdir : '/tmp')
+
tests
test_symbols_leak = find_program('test/symbols-leak-test.in')
diff --git a/tools/helper-copy-and-exec-from-tmp.sh
b/tools/helper-copy-and-exec-from-tmp.sh
new file mode 100755
index ..2ae8ec64
--- /dev/null
+++ b/tools/helper-copy-and-exec-from-tmp.sh
@@ -0,0 +1,18 @@
+#!/bin/bash -x
+#
+# Usage: helper-copy-and-exec-from-tmp.sh /path/to/binary [args]
+#
+# Copies the given binary into a unique file in /tmp and executes it with
+# [args]. Exits with the same return code as the binary did.
+
+executable="$1"
+shift
+
+target_name=$(mktemp)
+cp "$executable" "$target_name"
+chmod +x "$target_name"
+
+"$target_name" "$@"
+rc=$?
+rm "$target_name"
+exit $rc
diff --git a/tools/shared.c b/tools/shared.c
index f2ed1fd0..74c0cdad 100644
--- a/tools/shared.c
+++ b/tools/shared.c
@@ -467,18 +467,51 @@ out:
return is_touchpad;
}
+/* Try to read the directory we're executing from and if it matches the
+ * builddir, return it as path. Otherwise, return NULL.
+ */
+char *
+tools_execdir_is_builddir(void)
+{
+ char execdir[PATH_MAX] = {0};
+ char *pathsep;
+ ssize_t sz;
+
+ sz = readlink("/proc/self/exe", execdir, sizeof(execdir) - 1);
+ if (sz <= 0 || sz == sizeof(execdir) - 1)
+ return NULL;
+
+ /* readlink doesn't terminate the string and readlink says
+ anything past sz is undefined */
+ execdir[sz + 1] = '\0';
+
+ pathsep = strrchr(execdir, '/');
+ if (!pathsep)
+ return NULL;
+
+ *pathsep = '\0';
+ if (!streq(execdir, MESON_BUILD_ROOT))
+ return NULL;
+
+ return safe_strdup(execdir);
+}
+
static inline void
setup_path(void)
{
const char *path = getenv("PATH");
char new_path[PATH_MAX];
+ char *builddir;
+
+ builddir = tools_execdir_is_builddir();
snprintf(new_path,
sizeof(new_path),
"%s:%s",
-LIBINPUT_TOOL_PATH,
+builddir ? builddir : LIBINPUT_TOOL_PATH,
path ? path : "");
setenv("PATH", new_path, 1);
+ free(builddir);
}
int
diff --git a/tools/shared.h b/tools/shared.h
index
Upcoming release reminder
Hi all, Just a quick reminder that we're due for alpha shortly, with the following intended release schedule: Alpha - July 10th Beta - July 24th RC1 - August 7th First possible release August 14th. Thanks, Derek ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH weston] simple-dmabuf-drm: fix build with --disable-egl
Hi Emilio, On 2 July 2018 at 16:22, Emilio Pozuelo Monfort wrote: > Signed-off-by: Emilio Pozuelo Monfort > --- > I tried a build with --disable-egl as I didn't have the headers > installed, and it broke here. The EGL usage here seemed optional so I > did that, but I didn't run-test the result. If it would make more sense > to disable the client if EGL support is disabled I can do that. > > clients/simple-dmabuf-drm.c | 2 ++ > 1 file changed, 2 insertions(+) > Fairly orthogonal question, aimed at wayland devs: Why does this "simple" app has per-device code instead of using gbm_bo_map/unmap? API has been around for 2 years and every half recent Mesa driver has support for it. Only the really old ones do not radeon (r100), r200, nouveau_vieux and i915. Thanks Emil ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH wayland 4/8] fixup! scanner: use c99 initializers for the request/events arrays
On 3 July 2018 at 12:28, Pekka Paalanen wrote:
> On Thu, 14 Jun 2018 16:49:41 +0100
> Emil Velikov wrote:
>
>> ---
>> tests/data/example-code.c | 238
>>
>> tests/data/small-code-core.c| 8 +-
>> tests/data/small-code.c | 8 +-
>> tests/data/small-private-code.c | 8 +-
>> 4 files changed, 131 insertions(+), 131 deletions(-)
>
>> diff --git a/tests/data/small-private-code.c
>> b/tests/data/small-private-code.c
>> index 35fa653..84f77de 100644
>> --- a/tests/data/small-private-code.c
>> +++ b/tests/data/small-private-code.c
>> @@ -54,13 +54,13 @@ static const struct wl_interface *types[] = {
>> };
>>
>> static const struct wl_message intf_A_requests[] = {
>> - { "rq1", "sun", types + 0 },
>> - { "rq2", "nsiufho", types + 1 },
>> - { "destroy", "", types + 0 },
>> + { .name = "rq1", .signature = "sun", .types = [0] },
>> + { .name = "rq2", .signature = "nsiufho", .types = [1] },
>> + { .name = "destroy", .signature = "", .types = [0] },
>> };
>
> Hi,
>
> this change doesn't look more readable to me than the old version.
> Adding the field names only clutters things and makes the lines longer
> for no added information: it is always the exact same fields set in the
> same order, and there are only three of them.
>
> The change from 'types + i' to '[i]' seems ok though.
>
Seems like these dead trivial patches are inciting some bike-shedding.
Proceed as you wish, sadly I'm out of patience.
Peace
Emil
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH wayland 8/8] fixup! scanner: initialize .{method, event}_count via ARRAY_SIZE
On 3 July 2018 at 12:45, Pekka Paalanen wrote:
> On Thu, 14 Jun 2018 16:49:45 +0100
> Emil Velikov wrote:
>
>> ---
>> tests/data/example-code.c | 73
>> +
>> tests/data/small-code-core.c| 5 +--
>> tests/data/small-code.c | 5 +--
>> tests/data/small-private-code.c | 5 +--
>> 4 files changed, 46 insertions(+), 42 deletions(-)
>>
>> diff --git a/tests/data/example-code.c b/tests/data/example-code.c
>> index 2e1f73b..65d9651 100644
>> --- a/tests/data/example-code.c
>> +++ b/tests/data/example-code.c
>> @@ -146,6 +146,7 @@ static const struct wl_interface *types[] = {
>> [94] = _surface_interface,
>> };
>>
>> +#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
>> static const struct wl_message wl_display_requests[] = {
>> { .name = "sync", .signature = "n", .types = [8] },
>> { .name = "get_registry", .signature = "n", .types = [9] },
>> @@ -158,8 +159,8 @@ static const struct wl_message wl_display_events[] = {
>>
>> WL_EXPORT const struct wl_interface wl_display_interface = {
>> .name = "wl_display", .version = 1,
>> - .method_count = 2, .methods = wl_display_requests,
>> - .event_count = 2, .events = wl_display_events,
>> + .method_count = ARRAY_SIZE(wl_display_requests), .methods =
>> wl_display_requests,
>> + .event_count = ARRAY_SIZE(wl_display_events), .events =
>> wl_display_events,
>> };
>
> Hi,
>
> this change is not an obvious improvement to me. "method_count = 2" is
> pretty clear, that combined with "methods = foo" does not seem to
> leave anything to be desired.
>
> If this code was hand-written, then I would be cheering for ARRAY_SIZE
> for sure, but it's not. It all comes from a generator that gets the
> count right.
>
> I suppose we'd need an opinion from someone who is less familiar with
> Wayland C bindings.
>
Sure ARRAY_SIZE is used for getting thins "right" although it also
improves _readability_.
Pre-patch (series) it took me an embarrassing amount of time trying to
understand any of the magic created by the scanner.
Perhaps I'm just silly in trying to spare the next reader some time...
It's trivial change and basic coding practise.
HTH
Emil
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH] protocol: warn clients about some wl_output properties
Hi,
one small comment below.
2018-07-03 (火) の 09:01 -0400 に Simon Ser さんは書きました:
> All wl_output properties don't always make sense for all
> compositors.
>
> Some compositors might not implement a "global compositor space",
> (e.g. 3D compositors) in which case properties like x and y don't
> make sense.
>
> Some compositors might expose virtual outputs, in which case modes,
> make and model are not relevant.
>
> In a lot of these situations, information from xdg_output is better
> suited.
> ---
> Regular clients shouldn't use most of these properties anyway.
> wl_output is not meant to be an output management protocol. wl_output
> should only expose enough information to allow clients to display
> themselves properly. I don't think there's any legitimate use-case
> to read geometry.{x,y,make,model} and modes for regular clients. Some
> clients (e.g. xwayland) need more information and support an
> additional
> protocol (xdg-output) that's better suited.
>
> Because of this, compositors should be allowed to fake these
> properties,
> and clients should be prepared to receive fake data.
>
> I'm not completely happy with this patch. IMHO we should completely
> deprecate modes because I really fail to see how clients will use
> this
> information properly.
I'm not sure if that was the original intention, but having the modes
IMO is required to you use the (still unstable) fullscreen-shell
protocol. There you really need this kind of information to make
sensible choices about your window size. Of course, this does not mean
that the mode information must or should be conveyed over wl_output in
general, it could as well be a protocol specific to fullscreen-shell.
> To do so we can probably just bump the protocol
> version and make this event optional?
>
> protocol/wayland.xml | 15 ++-
> 1 file changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/protocol/wayland.xml b/protocol/wayland.xml
> index b5662e0..b4ffcd5 100644
> --- a/protocol/wayland.xml
> +++ b/protocol/wayland.xml
> @@ -2399,6 +2399,13 @@
> The geometry event describes geometric properties of the
> output.
> The event is sent when binding to the output object and
> whenever
> any of the properties change.
> +
> + Note: wl_output only advertises partial information about
> the output
> + position and identification. Some compositors, for instance
> those not
> + implementing a desktop-style output layout or those exposing
> virtual
> + outputs, might fake this information. Instead of using x and
> y, clients
> + should use xdg_output.logical_position. Instead of using
> make and model,
> + clients should use xdg_output.name and
> xdg_output.description.
>
> summary="x position within the global compositor space"/>
> @@ -2443,7 +2450,13 @@
> the output device. This is not necessarily the same as
> the output size in the global compositor space. For
> instance,
> the output may be scaled, as described in wl_output.scale,
> - or transformed, as described in wl_output.transform.
> + or transformed, as described in wl_output.transform. Clients
> + willing to retrieve the output size in the global compositor
> + space should use xdg_output.logical_size instead.
> +
> + Note: this information is not always meaningful for all
> outputs. Some
> + compositors, such as those exposing virtual outputs, might
> fake the
> + refresh rate or the size.
>
>
>
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel
[PATCH] protocol: warn clients about some wl_output properties
All wl_output properties don't always make sense for all
compositors.
Some compositors might not implement a "global compositor space",
(e.g. 3D compositors) in which case properties like x and y don't
make sense.
Some compositors might expose virtual outputs, in which case modes,
make and model are not relevant.
In a lot of these situations, information from xdg_output is better
suited.
---
Regular clients shouldn't use most of these properties anyway.
wl_output is not meant to be an output management protocol. wl_output
should only expose enough information to allow clients to display
themselves properly. I don't think there's any legitimate use-case
to read geometry.{x,y,make,model} and modes for regular clients. Some
clients (e.g. xwayland) need more information and support an additional
protocol (xdg-output) that's better suited.
Because of this, compositors should be allowed to fake these properties,
and clients should be prepared to receive fake data.
I'm not completely happy with this patch. IMHO we should completely
deprecate modes because I really fail to see how clients will use this
information properly. To do so we can probably just bump the protocol
version and make this event optional?
protocol/wayland.xml | 15 ++-
1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/protocol/wayland.xml b/protocol/wayland.xml
index b5662e0..b4ffcd5 100644
--- a/protocol/wayland.xml
+++ b/protocol/wayland.xml
@@ -2399,6 +2399,13 @@
The geometry event describes geometric properties of the output.
The event is sent when binding to the output object and whenever
any of the properties change.
+
+ Note: wl_output only advertises partial information about the output
+ position and identification. Some compositors, for instance those not
+ implementing a desktop-style output layout or those exposing virtual
+ outputs, might fake this information. Instead of using x and y, clients
+ should use xdg_output.logical_position. Instead of using make and model,
+ clients should use xdg_output.name and xdg_output.description.
@@ -2443,7 +2450,13 @@
the output device. This is not necessarily the same as
the output size in the global compositor space. For instance,
the output may be scaled, as described in wl_output.scale,
- or transformed, as described in wl_output.transform.
+ or transformed, as described in wl_output.transform. Clients
+ willing to retrieve the output size in the global compositor
+ space should use xdg_output.logical_size instead.
+
+ Note: this information is not always meaningful for all outputs. Some
+ compositors, such as those exposing virtual outputs, might fake the
+ refresh rate or the size.
--
2.18.0
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH wayland 8/8] fixup! scanner: initialize .{method, event}_count via ARRAY_SIZE
On July 3, 2018 12:45 PM, Pekka Paalanen wrote:
> On Thu, 14 Jun 2018 16:49:45 +0100
> Emil Velikov wrote:
>
> > ---
> > tests/data/example-code.c | 73
> > +
> > tests/data/small-code-core.c| 5 +--
> > tests/data/small-code.c | 5 +--
> > tests/data/small-private-code.c | 5 +--
> > 4 files changed, 46 insertions(+), 42 deletions(-)
> >
> > diff --git a/tests/data/example-code.c b/tests/data/example-code.c
> > index 2e1f73b..65d9651 100644
> > --- a/tests/data/example-code.c
> > +++ b/tests/data/example-code.c
> > @@ -146,6 +146,7 @@ static const struct wl_interface *types[] = {
> > [94] = _surface_interface,
> > };
> >
> > +#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
> > static const struct wl_message wl_display_requests[] = {
> > { .name = "sync", .signature = "n", .types = [8] },
> > { .name = "get_registry", .signature = "n", .types = [9] },
> > @@ -158,8 +159,8 @@ static const struct wl_message wl_display_events[] = {
> >
> > WL_EXPORT const struct wl_interface wl_display_interface = {
> > .name = "wl_display", .version = 1,
> > - .method_count = 2, .methods = wl_display_requests,
> > - .event_count = 2, .events = wl_display_events,
> > + .method_count = ARRAY_SIZE(wl_display_requests), .methods =
> > wl_display_requests,
> > + .event_count = ARRAY_SIZE(wl_display_events), .events =
> > wl_display_events,
> > };
>
> Hi,
>
> this change is not an obvious improvement to me. "method_count = 2" is
> pretty clear, that combined with "methods = foo" does not seem to
> leave anything to be desired.
>
> If this code was hand-written, then I would be cheering for ARRAY_SIZE
> for sure, but it's not. It all comes from a generator that gets the
> count right.
>
> I suppose we'd need an opinion from someone who is less familiar with
> Wayland C bindings.
Hi,
I'm not sure if I qualify, but I would agree with Pekka here. This generated
code isn't meant to be maintained or edited by humans. This code is read-only,
and is readable enough as is IMHO for the reasons Pekka wrote.
> Thanks,
> pq
> ___
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH wayland 8/8] fixup! scanner: initialize .{method, event}_count via ARRAY_SIZE
On Thu, 14 Jun 2018 16:49:45 +0100
Emil Velikov wrote:
> ---
> tests/data/example-code.c | 73
> +
> tests/data/small-code-core.c| 5 +--
> tests/data/small-code.c | 5 +--
> tests/data/small-private-code.c | 5 +--
> 4 files changed, 46 insertions(+), 42 deletions(-)
>
> diff --git a/tests/data/example-code.c b/tests/data/example-code.c
> index 2e1f73b..65d9651 100644
> --- a/tests/data/example-code.c
> +++ b/tests/data/example-code.c
> @@ -146,6 +146,7 @@ static const struct wl_interface *types[] = {
> [94] = _surface_interface,
> };
>
> +#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
> static const struct wl_message wl_display_requests[] = {
> { .name = "sync", .signature = "n", .types = [8] },
> { .name = "get_registry", .signature = "n", .types = [9] },
> @@ -158,8 +159,8 @@ static const struct wl_message wl_display_events[] = {
>
> WL_EXPORT const struct wl_interface wl_display_interface = {
> .name = "wl_display", .version = 1,
> - .method_count = 2, .methods = wl_display_requests,
> - .event_count = 2, .events = wl_display_events,
> + .method_count = ARRAY_SIZE(wl_display_requests), .methods =
> wl_display_requests,
> + .event_count = ARRAY_SIZE(wl_display_events), .events =
> wl_display_events,
> };
Hi,
this change is not an obvious improvement to me. "method_count = 2" is
pretty clear, that combined with "methods = foo" does not seem to
leave anything to be desired.
If this code was hand-written, then I would be cheering for ARRAY_SIZE
for sure, but it's not. It all comes from a generator that gets the
count right.
I suppose we'd need an opinion from someone who is less familiar with
Wayland C bindings.
Thanks,
pq
pgpUYlUmFMBpD.pgp
Description: OpenPGP digital signature
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH wayland 6/8] fixup! scanner: use c99 initializers for the interface symbols
On Thu, 14 Jun 2018 16:49:43 +0100
Emil Velikov wrote:
> ---
> tests/data/example-code.c | 124
> +++-
> tests/data/small-code-core.c| 6 +-
> tests/data/small-code.c | 6 +-
> tests/data/small-private-code.c | 6 +-
> 4 files changed, 67 insertions(+), 75 deletions(-)
>
> diff --git a/tests/data/example-code.c b/tests/data/example-code.c
> index 4a41b8b..2e1f73b 100644
> --- a/tests/data/example-code.c
> +++ b/tests/data/example-code.c
> @@ -157,9 +157,9 @@ static const struct wl_message wl_display_events[] = {
> };
>
> WL_EXPORT const struct wl_interface wl_display_interface = {
> - "wl_display", 1,
> - 2, wl_display_requests,
> - 2, wl_display_events,
> + .name = "wl_display", .version = 1,
> + .method_count = 2, .methods = wl_display_requests,
> + .event_count = 2, .events = wl_display_events,
> };
>
> WL_EXPORT const struct wl_interface wl_callback_interface = {
> - "wl_callback", 1,
> - 0, NULL,
> - 1, wl_callback_events,
> + .name = "wl_callback", .version = 1,
> + .event_count = 1, .events = wl_callback_events,
> };
>
> static const struct wl_message wl_compositor_requests[] = {
> @@ -193,9 +192,8 @@ static const struct wl_message wl_compositor_requests[] =
> {
> };
>
> WL_EXPORT const struct wl_interface wl_compositor_interface = {
> - "wl_compositor", 4,
> - 2, wl_compositor_requests,
> - 0, NULL,
> + .name = "wl_compositor", .version = 4,
> + .method_count = 2, .methods = wl_compositor_requests,
> };
Hi,
this change seems more worthwhile than patches 3+4, because this is
already on multiple lines, the added length doesn't hurt that much, and
I think the field names are actually informative since there are more
fields. They are also not repeated so much. Getting rid of the NULL
entries is also nice in this case, maybe not so much for arrays where
one is looking at a range of elements and noticing the index skipping
NULL entries would be extra effort to read.
Acked-by: Pekka Paalanen
Thanks,
pq
pgpz2DxyL1Ok_.pgp
Description: OpenPGP digital signature
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH v5] compositor-drm: Add aspect-ratio parsing support
Hi Shashank, Thanks for the detailed explanation about the relationship between the aspect-ratio, CEA modes and VIC codes. I will capture these and try to add this as part of man pages for a better understanding. Regards, Ankit On 7/3/2018 4:05 PM, Sharma, Shashank wrote: Regards Shashank On 7/3/2018 3:26 PM, Pekka Paalanen wrote: On Tue, 3 Jul 2018 14:42:06 +0530 "Sharma, Shashank" wrote: Hello Pekka, Thanks for your attention and code review of the patch series. Ankit will respond on rest of the review comments, I would take one related to mode with implicit aspect ratio information. Please find my comment inline. Regards Shashank On 6/28/2018 7:09 PM, Pekka Paalanen wrote: On Sun, 20 May 2018 18:33:01 +0530 "Nautiyal, Ankit K" wrote: From: Ankit Nautiyal The flag bits 19-22 of the connector modes, provide the aspect-ratio information. This information can be stored in flags bits of the weston mode structure, so that it can used for setting a mode with a particular aspect-ratio. Currently, DRM layer supports aspect-ratio with atomic-modesetting by default. For legacy modeset path, the user-space needs to set the drm client cap for aspect-ratio, if it wants aspect-ratio information in modes. This patch: - preserves aspect-ratio flags from kernel video modes and accommodates it in wayland mode. - uses aspect-ratio to pick the appropriate mode during modeset. - changes the mode format in configuration file weston.ini to accommodate aspect-ratio information as: WIDTHxHEIGHT at REFRESH-RATE ASPECT-RATIO The aspect-ratio should be given as . - modifies the man pages to explain the usage of different mode format configurations in weston.ini. v2: As per recommendation from Pekka Paalanen, Quentin Glidic, Daniel Stone, dropped the aspect-ratio info from wayland protocol, thereby avoiding exposure of aspect-ratio to the client. v3: As suggested by Pekka Paalanen, added aspect_ratio field to store aspect-ratio information from the drm. Also added drm client capability for aspect-ratio, as recommended by Daniel Vetter. v4: Minor modifications and fixes as suggested by Pekka Paalanen. v5: Rebased, fixed some styling issues, and added aspect-ratio information while printing weston_modes. Signed-off-by: Ankit Nautiyal Acked-by: Pekka Paalanen (v4) --- libweston/compositor-drm.c | 115 + libweston/compositor-drm.h | 21 +++ libweston/compositor.h | 1 + man/weston.ini.man | 13 +++-- 4 files changed, 136 insertions(+), 14 deletions(-) Hi Ankit, it's been a long while since I last looked at this, so forgive me if I go in circles with my review comments. I see the aspect ratio bits are in Linus' RC kernel. I would like to get this merged for the next release which basically means during the next week if you can. diff --git a/man/weston.ini.man b/man/weston.ini.man index f237fd60..e982cac9 100644 --- a/man/weston.ini.man +++ b/man/weston.ini.man @@ -371,10 +371,15 @@ The DRM backend accepts different modes: .PP .RS 10 .nf -.BR "WIDTHxHEIGHT" "Resolution size width and height in pixels" -.BR "preferred " "Uses the preferred mode" -.BR "current " "Uses the current crt controller mode" -.BR "off " "Disables the output" +.BR "WIDTHxHEIGHT " "Resolution size width and height in pixels" +.BR "WIDTHxHEIGHT@RR" "Resolution as above and refresh-rate in Hertz" +.BR "WIDTHxHEIGHT@RR RATIO " "Resolution as above and aspect-ratio as length:breadth" I think the length:breadth could be confusing. H:V? hor:vert? Or simply A:B, assuming that aspect ratio is general knowledge. It would also be worth to list all the acceptable values, because there are only few. One cannot give e.g. 8:6 and assume Weston figures out it means 4:3. If they are explicitly listed, we could just use RATIO and not split the value further. Since these are specific to the DRM-backend, they would be more logical in weston-drm.man. + +e.g. 720x576@50 4:3, 1920x1080@60 16:9, 2560x1080@60 64:27, 4096x2160@60 256:135 etc. What is the point of giving the aspect ratio explicitly if the resolution directly results in the same aspect ratio? Shouldn't the examples show cases where it actually matters, i.e. when the display shows non-square pixels? E.g. 1024x768 16:9 The implementation also makes a difference between implicit aspect ratio (that is, NONE) and explicit aspect ratio even if the two are equal. Is that intentional? Is there an actual difference? Yes there is an actual difference, let me try to explain this. For this, we should first understand difference between CEA-modes Vs non-CEA-modes. As you might already know, CEA defines timings of a video mode, which is considered as standard for HDMI certification and compliance testing. CEA defines each and every parameter of a video mode, like h/vactive, h/vfront, h/vback including aspect ratio
Re: [PATCH wayland 4/8] fixup! scanner: use c99 initializers for the request/events arrays
On Thu, 14 Jun 2018 16:49:41 +0100
Emil Velikov wrote:
> ---
> tests/data/example-code.c | 238
>
> tests/data/small-code-core.c| 8 +-
> tests/data/small-code.c | 8 +-
> tests/data/small-private-code.c | 8 +-
> 4 files changed, 131 insertions(+), 131 deletions(-)
> diff --git a/tests/data/small-private-code.c b/tests/data/small-private-code.c
> index 35fa653..84f77de 100644
> --- a/tests/data/small-private-code.c
> +++ b/tests/data/small-private-code.c
> @@ -54,13 +54,13 @@ static const struct wl_interface *types[] = {
> };
>
> static const struct wl_message intf_A_requests[] = {
> - { "rq1", "sun", types + 0 },
> - { "rq2", "nsiufho", types + 1 },
> - { "destroy", "", types + 0 },
> + { .name = "rq1", .signature = "sun", .types = [0] },
> + { .name = "rq2", .signature = "nsiufho", .types = [1] },
> + { .name = "destroy", .signature = "", .types = [0] },
> };
Hi,
this change doesn't look more readable to me than the old version.
Adding the field names only clutters things and makes the lines longer
for no added information: it is always the exact same fields set in the
same order, and there are only three of them.
The change from 'types + i' to '[i]' seems ok though.
Thanks,
pq
pgphdt0V4eSKm.pgp
Description: OpenPGP digital signature
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH wayland 2/8] fixup! scanner: use c99 initializers for the wl_interface * array
On Thu, 14 Jun 2018 16:49:39 +0100
Emil Velikov wrote:
> ---
> tests/data/example-code.c | 190
>
> tests/data/small-code-core.c| 16 ++--
> tests/data/small-code.c | 16 ++--
> tests/data/small-private-code.c | 16 ++--
> 4 files changed, 119 insertions(+), 119 deletions(-)
Hi,
Acked-by: Pekka Paalanen
I'll leave upgrading to R-b for closer to landing this series when I
will test running with this.
Thanks,
pq
>
> diff --git a/tests/data/example-code.c b/tests/data/example-code.c
> index bc03fe5..ccfa9ef 100644
> --- a/tests/data/example-code.c
> +++ b/tests/data/example-code.c
> @@ -49,101 +49,101 @@ extern const struct wl_interface wl_surface_interface;
> extern const struct wl_interface wl_touch_interface;
>
> static const struct wl_interface *types[] = {
> - NULL,
> - NULL,
> - NULL,
> - NULL,
> - NULL,
> - NULL,
> - NULL,
> - NULL,
> - _callback_interface,
> - _registry_interface,
> - _surface_interface,
> - _region_interface,
> - _buffer_interface,
> - NULL,
> - NULL,
> - NULL,
> - NULL,
> - NULL,
> - _shm_pool_interface,
> - NULL,
> - NULL,
> - _data_source_interface,
> - _surface_interface,
> - _surface_interface,
> - NULL,
> - _data_source_interface,
> - NULL,
> - _data_offer_interface,
> - NULL,
> - _surface_interface,
> - NULL,
> - NULL,
> - _data_offer_interface,
> - _data_offer_interface,
> - _data_source_interface,
> - _data_device_interface,
> - _seat_interface,
> - _shell_surface_interface,
> - _surface_interface,
> - _seat_interface,
> - NULL,
> - _seat_interface,
> - NULL,
> - NULL,
> - _surface_interface,
> - NULL,
> - NULL,
> - NULL,
> - NULL,
> - NULL,
> - _output_interface,
> - _seat_interface,
> - NULL,
> - _surface_interface,
> - NULL,
> - NULL,
> - NULL,
> - _output_interface,
> - _buffer_interface,
> - NULL,
> - NULL,
> - _callback_interface,
> - _region_interface,
> - _region_interface,
> - _output_interface,
> - _output_interface,
> - _pointer_interface,
> - _keyboard_interface,
> - _touch_interface,
> - NULL,
> - _surface_interface,
> - NULL,
> - NULL,
> - NULL,
> - _surface_interface,
> - NULL,
> - NULL,
> - NULL,
> - _surface_interface,
> - NULL,
> - _surface_interface,
> - NULL,
> - NULL,
> - _surface_interface,
> - NULL,
> - NULL,
> - _surface_interface,
> - NULL,
> - NULL,
> - NULL,
> - _subsurface_interface,
> - _surface_interface,
> - _surface_interface,
> - _surface_interface,
> - _surface_interface,
> + [0] = NULL,
> + [1] = NULL,
> + [2] = NULL,
> + [3] = NULL,
> + [4] = NULL,
> + [5] = NULL,
> + [6] = NULL,
> + [7] = NULL,
> + [8] = _callback_interface,
> + [9] = _registry_interface,
> + [10] = _surface_interface,
> + [11] = _region_interface,
> + [12] = _buffer_interface,
> + [13] = NULL,
> + [14] = NULL,
> + [15] = NULL,
> + [16] = NULL,
> + [17] = NULL,
> + [18] = _shm_pool_interface,
> + [19] = NULL,
> + [20] = NULL,
> + [21] = _data_source_interface,
> + [22] = _surface_interface,
> + [23] = _surface_interface,
> + [24] = NULL,
> + [25] = _data_source_interface,
> + [26] = NULL,
> + [27] = _data_offer_interface,
> + [28] = NULL,
> + [29] = _surface_interface,
> + [30] = NULL,
> + [31] = NULL,
> + [32] = _data_offer_interface,
> + [33] = _data_offer_interface,
> + [34] = _data_source_interface,
> + [35] = _data_device_interface,
> + [36] = _seat_interface,
> + [37] = _shell_surface_interface,
> + [38] = _surface_interface,
> + [39] = _seat_interface,
> + [40] = NULL,
> + [41] = _seat_interface,
> + [42] = NULL,
> + [43] = NULL,
> + [44] = _surface_interface,
> + [45] = NULL,
> + [46] = NULL,
> + [47] = NULL,
> + [48] = NULL,
> + [49] = NULL,
> + [50] = _output_interface,
> + [51] = _seat_interface,
> + [52] = NULL,
> + [53] = _surface_interface,
> + [54] = NULL,
> + [55] = NULL,
> + [56] = NULL,
> + [57] = _output_interface,
> + [58] = _buffer_interface,
> + [59] = NULL,
> + [60] = NULL,
> + [61] = _callback_interface,
> + [62] = _region_interface,
> + [63] = _region_interface,
> + [64] = _output_interface,
> + [65] = _output_interface,
> + [66] = _pointer_interface,
> + [67] = _keyboard_interface,
> + [68] = _touch_interface,
> + [69] = NULL,
> + [70] = _surface_interface,
> + [71] = NULL,
> + [72] = NULL,
> + [73] = NULL,
> + [74] = _surface_interface,
> + [75] = NULL,
> + [76] = NULL,
> + [77] = NULL,
>
[PATCH] protocol: allow to send a zero physical output size
Physical size doesn't always make sense for all outputs. In case it's not available or not relevant, allow compositors to send zero. --- In practice this doesn't seem to break any client toolkit. We've been doing that for a long time in wlroots. protocol/wayland.xml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/protocol/wayland.xml b/protocol/wayland.xml index b5662e0..141038b 100644 --- a/protocol/wayland.xml +++ b/protocol/wayland.xml @@ -2399,6 +2399,9 @@ The geometry event describes geometric properties of the output. The event is sent when binding to the output object and whenever any of the properties change. + + The physical size can be set to zero if it doesn't make sense for this + output (e.g. for projectors or virtual outputs). -- 2.18.0 ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH weston] simple-dmabuf-drm: fix build with --disable-egl
On Tue, 3 Jul 2018 12:35:30 +0200 Emilio Pozuelo Monfort wrote: > On 03/07/18 11:00, Pekka Paalanen wrote: > > On Mon, 2 Jul 2018 17:22:30 +0200 > > Emilio Pozuelo Monfort wrote: > > > >> Signed-off-by: Emilio Pozuelo Monfort > >> --- > >> I tried a build with --disable-egl as I didn't have the headers > >> installed, and it broke here. The EGL usage here seemed optional so I > >> did that, but I didn't run-test the result. If it would make more sense > >> to disable the client if EGL support is disabled I can do that. > >> > >> clients/simple-dmabuf-drm.c | 2 ++ > >> 1 file changed, 2 insertions(+) > >> > >> diff --git a/clients/simple-dmabuf-drm.c b/clients/simple-dmabuf-drm.c > >> index fcab30e5..1ae3a2dd 100644 > >> --- a/clients/simple-dmabuf-drm.c > >> +++ b/clients/simple-dmabuf-drm.c > >> @@ -863,6 +863,7 @@ create_display(int opts, int format) > >>display->req_dmabuf_immediate = opts & OPT_IMMEDIATE; > >>display->req_dmabuf_modifiers = (format == DRM_FORMAT_NV12); > >> > >> +#if ENABLE_EGL > >>/* > >> * hard code format if the platform egl doesn't support format > >> * querying / advertising. > >> @@ -871,6 +872,7 @@ create_display(int opts, int format) > >>if (extensions && !weston_check_egl_extension(extensions, > >>"EGL_EXT_image_dma_buf_import_modifiers")) > >>display->xrgb_format_found = 1; > >> +#endif > >> > >>display->registry = wl_display_get_registry(display->display); > >>wl_registry_add_listener(display->registry, > > > > Hi, > > > > that's very strange. This program does not use EGL or even GBM, that's > > a completely dead hunk of code you're ifdeffing there as I can see. > > Would be better to just remove it completely, and make sure the build > > does not link libEGL or GBM. Include for shared/platform.h should be > > useless too. > > It's not dead code, it's a fallback mechanism in case the EGL platform doesn't > support EGL_EXT_image_dma_buf_import_modifiers. The problem is that > configure.ac > checks for EGL for simple-dmabuf-drm-client, but the client is not disabled if > one builds with --disable-egl. Perhaps I should do that instead. After all, > disable-egl disables the gl-renderer, which is needed for zwp_linux_dmabuf, > which is needed by simple-dmabuf-drm-client. What I meant was that nothing calls eglGetDisplay or equivalent. It may be inspecting the EGL Client extensions, but it won't do anything with that information. The Wayland extension advertises modifier support completely independently. In fact, if the compositor did not advertise xrgb through zwp_linux_dmabuf, the flag for it should never be set. I believe you'd be fixing a bug by simply deleting that code. No, two bugs: the flag and the build. :-) Thanks, pq pgpdBq43kgINd.pgp Description: OpenPGP digital signature ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH v5] compositor-drm: Add aspect-ratio parsing support
On Tue, 3 Jul 2018 15:56:10 +0530
"Nautiyal, Ankit K" wrote:
> Hi Pekka,
>
> Thanks for the review and the comments.
>
> I agree with all of the suggestions, will send a patch in a day or two.
>
> Discussion about the CEA with aspect ratio and and Non- CEA modes is
> currently in progress in another thread,
>
> lets close it in the other thread itself.
>
> Please find my responses inline for the remaining comments:
>
>
> On 6/28/2018 7:09 PM, Pekka Paalanen wrote:
> > On Sun, 20 May 2018 18:33:01 +0530
> > "Nautiyal, Ankit K" wrote:
> >
> >> From: Ankit Nautiyal
> >>
> >> The flag bits 19-22 of the connector modes, provide the aspect-ratio
> >> information. This information can be stored in flags bits of the
> >> weston mode structure, so that it can used for setting a mode with a
> >> particular aspect-ratio.
> >> Currently, DRM layer supports aspect-ratio with atomic-modesetting by
> >> default. For legacy modeset path, the user-space needs to set the
> >> drm client cap for aspect-ratio, if it wants aspect-ratio information
> >> in modes.
> >>
> >> This patch:
> >> - preserves aspect-ratio flags from kernel video modes and
> >>accommodates it in wayland mode.
> >> - uses aspect-ratio to pick the appropriate mode during modeset.
> >> - changes the mode format in configuration file weston.ini to
> >>accommodate aspect-ratio information as:
> >>WIDTHxHEIGHT at REFRESH-RATE ASPECT-RATIO
> >>The aspect-ratio should be given as .
> >> - modifies the man pages to explain the usage of different mode format
> >>configurations in weston.ini.
> >>
> >> v2: As per recommendation from Pekka Paalanen, Quentin Glidic,
> >> Daniel Stone, dropped the aspect-ratio info from wayland protocol,
> >> thereby avoiding exposure of aspect-ratio to the client.
> >>
> >> v3: As suggested by Pekka Paalanen, added aspect_ratio field to store
> >> aspect-ratio information from the drm. Also added drm client
> >> capability for aspect-ratio, as recommended by Daniel Vetter.
> >>
> >> v4: Minor modifications and fixes as suggested by Pekka Paalanen.
> >>
> >> v5: Rebased, fixed some styling issues, and added aspect-ratio
> >> information while printing weston_modes.
> >> Signed-off-by: Ankit Nautiyal
> >>
> >> Acked-by: Pekka Paalanen (v4)
> >> ---
> >> libweston/compositor-drm.c | 115 +
> >> libweston/compositor-drm.h | 21 +++
> >> libweston/compositor.h | 1 +
> >> man/weston.ini.man | 13 +++--
> >> 4 files changed, 136 insertions(+), 14 deletions(-)
> > Hi Ankit,
> >
> > it's been a long while since I last looked at this, so forgive me if I
> > go in circles with my review comments. I see the aspect ratio bits are
> > in Linus' RC kernel. I would like to get this merged for the next
> > release which basically means during the next week if you can.
> >> diff --git a/libweston/compositor-drm.h b/libweston/compositor-drm.h
> >> index 68f93eab..61ad44dc 100644
> >> --- a/libweston/compositor-drm.h
> >> +++ b/libweston/compositor-drm.h
> >> @@ -52,6 +52,27 @@ enum weston_drm_backend_output_mode {
> >>WESTON_DRM_BACKEND_OUTPUT_PREFERRED,
> >> };
> >>
> >> +/**
> >> + * aspect ratio info taken from the drmModeModeInfo flag bits 19-22,
> >> + * which should be used to fill the aspect ratio field in weston_mode.
> >> + */
> >> +#define DRM_MODE_FLAG_PIC_AR_BITS_POS 19
> >> +#ifndef DRM_MODE_FLAG_PIC_AR_MASK
> >> +#define DRM_MODE_FLAG_PIC_AR_MASK (0xF << DRM_MODE_FLAG_PIC_AR_BITS_POS)
> >> +#endif
> >> +
> > The above is ok in compositor-drm.h, but it could be in
> > compositor-drm.c instead, since it's not used anywhere else so far.
>
> Noted. Will do in next patch-set.
>
> > The below could be in compositor.h instead so that...
> >
> >> +enum weston_mode_aspect_ratio {
> >> + /** The picture aspect ratio values, for the aspect_ratio field of
> >> + * weston_mode. The values here, are taken from
> >> + * DRM_MODE_PICTURE_ASPECT_* from drm_mode.h.
> >> + */
> >> + WESTON_MODE_PIC_AR_NONE = 0,/* DRM_MODE_PICTURE_ASPECT_NONE */
> >> + WESTON_MODE_PIC_AR_4_3 = 1, /* DRM_MODE_PICTURE_ASPECT_4_3 */
> >> + WESTON_MODE_PIC_AR_16_9 = 2,/* DRM_MODE_PICTURE_ASPECT_16_9 */
> >> + WESTON_MODE_PIC_AR_64_27 = 3, /* DRM_MODE_PICTURE_ASPECT_64_27 */
> >> + WESTON_MODE_PIC_AR_256_135 = 4, /* DRM_MODE_PICTURE_ASPECT_256_135*/
> >> +};
> >> +
> >> #define WESTON_DRM_OUTPUT_API_NAME "weston_drm_output_api_v1"
> >>
> >> struct weston_drm_output_api {
> >> diff --git a/libweston/compositor.h b/libweston/compositor.h
> >> index 47337d8a..3dedbbfe 100644
> >> --- a/libweston/compositor.h
> >> +++ b/libweston/compositor.h
> >> @@ -95,6 +95,7 @@ enum weston_led {
> >>
> >> struct weston_mode {
> >>uint32_t flags;
> >> + uint8_t aspect_ratio;
> > ...the type of this could be enum weston_mode_aspect_ratio. None of it
> > actually depends on libdrm, so compositor.h would be fine. The docs are
> >
Re: [PATCH weston] simple-dmabuf-drm: fix build with --disable-egl
On 03/07/18 11:00, Pekka Paalanen wrote: > On Mon, 2 Jul 2018 17:22:30 +0200 > Emilio Pozuelo Monfort wrote: > >> Signed-off-by: Emilio Pozuelo Monfort >> --- >> I tried a build with --disable-egl as I didn't have the headers >> installed, and it broke here. The EGL usage here seemed optional so I >> did that, but I didn't run-test the result. If it would make more sense >> to disable the client if EGL support is disabled I can do that. >> >> clients/simple-dmabuf-drm.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/clients/simple-dmabuf-drm.c b/clients/simple-dmabuf-drm.c >> index fcab30e5..1ae3a2dd 100644 >> --- a/clients/simple-dmabuf-drm.c >> +++ b/clients/simple-dmabuf-drm.c >> @@ -863,6 +863,7 @@ create_display(int opts, int format) >> display->req_dmabuf_immediate = opts & OPT_IMMEDIATE; >> display->req_dmabuf_modifiers = (format == DRM_FORMAT_NV12); >> >> +#if ENABLE_EGL >> /* >> * hard code format if the platform egl doesn't support format >> * querying / advertising. >> @@ -871,6 +872,7 @@ create_display(int opts, int format) >> if (extensions && !weston_check_egl_extension(extensions, >> "EGL_EXT_image_dma_buf_import_modifiers")) >> display->xrgb_format_found = 1; >> +#endif >> >> display->registry = wl_display_get_registry(display->display); >> wl_registry_add_listener(display->registry, > > Hi, > > that's very strange. This program does not use EGL or even GBM, that's > a completely dead hunk of code you're ifdeffing there as I can see. > Would be better to just remove it completely, and make sure the build > does not link libEGL or GBM. Include for shared/platform.h should be > useless too. It's not dead code, it's a fallback mechanism in case the EGL platform doesn't support EGL_EXT_image_dma_buf_import_modifiers. The problem is that configure.ac checks for EGL for simple-dmabuf-drm-client, but the client is not disabled if one builds with --disable-egl. Perhaps I should do that instead. After all, disable-egl disables the gl-renderer, which is needed for zwp_linux_dmabuf, which is needed by simple-dmabuf-drm-client. Thoughts? Emilio ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH v5] compositor-drm: Add aspect-ratio parsing support
Regards Shashank On 7/3/2018 3:26 PM, Pekka Paalanen wrote: On Tue, 3 Jul 2018 14:42:06 +0530 "Sharma, Shashank" wrote: Hello Pekka, Thanks for your attention and code review of the patch series. Ankit will respond on rest of the review comments, I would take one related to mode with implicit aspect ratio information. Please find my comment inline. Regards Shashank On 6/28/2018 7:09 PM, Pekka Paalanen wrote: On Sun, 20 May 2018 18:33:01 +0530 "Nautiyal, Ankit K" wrote: From: Ankit Nautiyal The flag bits 19-22 of the connector modes, provide the aspect-ratio information. This information can be stored in flags bits of the weston mode structure, so that it can used for setting a mode with a particular aspect-ratio. Currently, DRM layer supports aspect-ratio with atomic-modesetting by default. For legacy modeset path, the user-space needs to set the drm client cap for aspect-ratio, if it wants aspect-ratio information in modes. This patch: - preserves aspect-ratio flags from kernel video modes and accommodates it in wayland mode. - uses aspect-ratio to pick the appropriate mode during modeset. - changes the mode format in configuration file weston.ini to accommodate aspect-ratio information as: WIDTHxHEIGHT at REFRESH-RATE ASPECT-RATIO The aspect-ratio should be given as . - modifies the man pages to explain the usage of different mode format configurations in weston.ini. v2: As per recommendation from Pekka Paalanen, Quentin Glidic, Daniel Stone, dropped the aspect-ratio info from wayland protocol, thereby avoiding exposure of aspect-ratio to the client. v3: As suggested by Pekka Paalanen, added aspect_ratio field to store aspect-ratio information from the drm. Also added drm client capability for aspect-ratio, as recommended by Daniel Vetter. v4: Minor modifications and fixes as suggested by Pekka Paalanen. v5: Rebased, fixed some styling issues, and added aspect-ratio information while printing weston_modes. Signed-off-by: Ankit Nautiyal Acked-by: Pekka Paalanen (v4) --- libweston/compositor-drm.c | 115 + libweston/compositor-drm.h | 21 +++ libweston/compositor.h | 1 + man/weston.ini.man | 13 +++-- 4 files changed, 136 insertions(+), 14 deletions(-) Hi Ankit, it's been a long while since I last looked at this, so forgive me if I go in circles with my review comments. I see the aspect ratio bits are in Linus' RC kernel. I would like to get this merged for the next release which basically means during the next week if you can. diff --git a/man/weston.ini.man b/man/weston.ini.man index f237fd60..e982cac9 100644 --- a/man/weston.ini.man +++ b/man/weston.ini.man @@ -371,10 +371,15 @@ The DRM backend accepts different modes: .PP .RS 10 .nf -.BR "WIDTHxHEIGHT" "Resolution size width and height in pixels" -.BR "preferred " "Uses the preferred mode" -.BR "current " "Uses the current crt controller mode" -.BR "off " "Disables the output" +.BR "WIDTHxHEIGHT " "Resolution size width and height in pixels" +.BR "WIDTHxHEIGHT@RR" "Resolution as above and refresh-rate in Hertz" +.BR "WIDTHxHEIGHT@RR RATIO " "Resolution as above and aspect-ratio as length:breadth" I think the length:breadth could be confusing. H:V? hor:vert? Or simply A:B, assuming that aspect ratio is general knowledge. It would also be worth to list all the acceptable values, because there are only few. One cannot give e.g. 8:6 and assume Weston figures out it means 4:3. If they are explicitly listed, we could just use RATIO and not split the value further. Since these are specific to the DRM-backend, they would be more logical in weston-drm.man. + +e.g. 720x576@50 4:3, 1920x1080@60 16:9, 2560x1080@60 64:27, 4096x2160@60 256:135 etc. What is the point of giving the aspect ratio explicitly if the resolution directly results in the same aspect ratio? Shouldn't the examples show cases where it actually matters, i.e. when the display shows non-square pixels? E.g. 1024x768 16:9 The implementation also makes a difference between implicit aspect ratio (that is, NONE) and explicit aspect ratio even if the two are equal. Is that intentional? Is there an actual difference? Yes there is an actual difference, let me try to explain this. For this, we should first understand difference between CEA-modes Vs non-CEA-modes. As you might already know, CEA defines timings of a video mode, which is considered as standard for HDMI certification and compliance testing. CEA defines each and every parameter of a video mode, like h/vactive, h/vfront, h/vback including aspect ratio information. This unique videmode is given a Video Identification Code (VIC) which is unique. For example, VIC=4 is 1280x720@60 aspect 16:9. Hi, this was new to me, thank you for explaining. Now, the videomode would be considered a CEA mode, only if it contains aspect ratio information. In other
[PATCH wayland 4/4] contributing: document the release cycle freezes
From: Pekka Paalanen These should be the conventions we have been using since 1.0, written down more accurately. Signed-off-by: Pekka Paalanen --- CONTRIBUTING.md | 35 +++ 1 file changed, 35 insertions(+) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 9442d75..4273d99 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -304,5 +304,40 @@ Maintainers and committers should encourage contributors to request commit rights, especially junior contributors tend to underestimate their skills. +Stabilising for releases + + +A release cycle ends with a stable release which also starts a new cycle and +lifts any code freezes. Gradual code freezing towards a stable release starts +with an alpha release. The release stages of a cycle are: + +- **Alpha release**: +Signified by version number #.#.91. +Major features must have landed before this. Major features include +invasive code motion and refactoring, high risk changes, and new stable +library ABI. + +- **Beta release**: +Signified by version number #.#.92. +Minor features must have landed before this. Minor features include all +new features that are not major, low risk changes, clean-ups, and +documentation. Stable ABI that was new in the alpha release can be removed +before a beta release if necessary. + +- **Release candidates (RC)**: +Signified by version number #.#.93 and up to #.#.99. +Bug fixes that are not release critical must have landed before this. +Release critical bug fixes can still be landed after this, but they may +call for another RC. + +- **Stable release**: +Signified by version number #.#.0. +Ideally no changes since the last RC. + +Mind that version #.#.90 is never released. It is used during development when +no code freeze is in effect. Stable branches and point releases are not covered +by the above. + + [git documentation]: http://git-scm.com/documentation [notes on commit messages]: http://who-t.blogspot.de/2009/12/on-commit-messages.html -- 2.16.4 ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
[PATCH v2 wayland 2/4] contributing: review rules for bugs
From: Pekka Paalanen Half of the ideas came from Daniel but most of them are reworded, the rest are my thoughts. Mention compiler warnings specifically, and be more explicit on what kind of code or bugs or bug fixes are acceptable or not. Clarify commit scope. v2: move the "In a patch series" rule to the bottom, reworded. Cc: Daniel Stone Signed-off-by: Pekka Paalanen --- CONTRIBUTING.md | 18 +++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 538613f..cbe02a3 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -228,11 +228,21 @@ include tests excercising the additions in the test suite. - The code fits the existing software architecture, e.g. no layering violations. -- The code is correct and does not ignore corner-cases. +- The code is correct and does not introduce new failures for existing users, +does not add new corner-case bugs, and does not introduce new compiler +warnings. -- In a patch series, every intermediate step produces correct code as well. +- The patch does what it says in the commit message and changes nothing else. -- The code does what it says in the commit message and changes nothing else. +- The patch is a single logical change. If the commit message addresses +multiple points, it is a hint that the commit might need splitting up. + +- A bug fix should target the underlying root cause instead of hiding symptoms. +If a complete fix is not practical, partial fixes are acceptable if they come +with code comments and filed Gitlab issues for the remaining bugs. + +- The bug root cause rule applies to external software components as well, e.g. +do not work around kernel driver issues in userspace. - The test suite passes. @@ -250,6 +260,8 @@ clarity suffers. - The code adheres to the style guidelines. +- In a patch series, every intermediate step adheres to the above guidelines. + Commit rights = -- 2.16.4 ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
[PATCH wayland 1/4] contributing: how to read the review rules
From: Pekka Paalanen This is to avoid fighting around the letter of the guidelines. This is not a protocol spec. Signed-off-by: Pekka Paalanen --- CONTRIBUTING.md | 5 + 1 file changed, 5 insertions(+) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 70d0eca..538613f 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -206,6 +206,11 @@ people with commit access, there needs to be at least one Acked-by from someone with commit access. A person with commit access is expected to be able to evaluate the patch with respect to the project scope and architecture. +The below review guidelines are intended to be interpreted in spirit, not by +the letter. There may be circumstances where some guidelines are better +ignored. We rely very much on the judgement of reviewers and commit rights +holders. + During review, the following matters should be checked: - The commit message explains why the change is being made. -- 2.16.4 ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH v5] compositor-drm: Add aspect-ratio parsing support
Hi Pekka,
Thanks for the review and the comments.
I agree with all of the suggestions, will send a patch in a day or two.
Discussion about the CEA with aspect ratio and and Non- CEA modes is
currently in progress in another thread,
lets close it in the other thread itself.
Please find my responses inline for the remaining comments:
On 6/28/2018 7:09 PM, Pekka Paalanen wrote:
On Sun, 20 May 2018 18:33:01 +0530
"Nautiyal, Ankit K" wrote:
From: Ankit Nautiyal
The flag bits 19-22 of the connector modes, provide the aspect-ratio
information. This information can be stored in flags bits of the
weston mode structure, so that it can used for setting a mode with a
particular aspect-ratio.
Currently, DRM layer supports aspect-ratio with atomic-modesetting by
default. For legacy modeset path, the user-space needs to set the
drm client cap for aspect-ratio, if it wants aspect-ratio information
in modes.
This patch:
- preserves aspect-ratio flags from kernel video modes and
accommodates it in wayland mode.
- uses aspect-ratio to pick the appropriate mode during modeset.
- changes the mode format in configuration file weston.ini to
accommodate aspect-ratio information as:
WIDTHxHEIGHT at REFRESH-RATE ASPECT-RATIO
The aspect-ratio should be given as .
- modifies the man pages to explain the usage of different mode format
configurations in weston.ini.
v2: As per recommendation from Pekka Paalanen, Quentin Glidic,
Daniel Stone, dropped the aspect-ratio info from wayland protocol,
thereby avoiding exposure of aspect-ratio to the client.
v3: As suggested by Pekka Paalanen, added aspect_ratio field to store
aspect-ratio information from the drm. Also added drm client
capability for aspect-ratio, as recommended by Daniel Vetter.
v4: Minor modifications and fixes as suggested by Pekka Paalanen.
v5: Rebased, fixed some styling issues, and added aspect-ratio
information while printing weston_modes.
Signed-off-by: Ankit Nautiyal
Acked-by: Pekka Paalanen (v4)
---
libweston/compositor-drm.c | 115 +
libweston/compositor-drm.h | 21 +++
libweston/compositor.h | 1 +
man/weston.ini.man | 13 +++--
4 files changed, 136 insertions(+), 14 deletions(-)
Hi Ankit,
it's been a long while since I last looked at this, so forgive me if I
go in circles with my review comments. I see the aspect ratio bits are
in Linus' RC kernel. I would like to get this merged for the next
release which basically means during the next week if you can.
I didn't see the aspect ratio bits in libdrm master yet, I guess it
should be ok to re-import the headers from kernel to libdrm by now.
diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
index 287431eb..e1d79e49 100644
--- a/libweston/compositor-drm.c
+++ b/libweston/compositor-drm.c
@@ -73,6 +73,10 @@
#define DRM_CLIENT_CAP_UNIVERSAL_PLANES 2
#endif
+#ifndef DRM_CLIENT_CAP_ASPECT_RATIO
+#define DRM_CLIENT_CAP_ASPECT_RATIO4
+#endif
+
#ifndef DRM_CAP_CURSOR_WIDTH
#define DRM_CAP_CURSOR_WIDTH 0x8
#endif
@@ -272,6 +276,8 @@ struct drm_backend {
uint32_t pageflip_timeout;
bool shutting_down;
+
+ bool aspect_ratio_supported;
};
struct drm_mode {
@@ -3049,6 +3055,19 @@ err:
drmModeSetCursor(b->drm.fd, output->crtc_id, 0, 0, 0);
}
+/*
+ * Get the aspect-ratio from drmModeModeInfo mode flags.
+ *
+ * @param drm_mode_flags- flags from drmModeModeInfo structure.
+ * @returns aspect-ratio as encoded in enum 'weston_mode_aspect_ratio'.
+ */
+static uint8_t
+drm_to_weston_mode_aspect_ratio(uint32_t drm_mode_flags)
+{
+ return (drm_mode_flags & DRM_MODE_FLAG_PIC_AR_MASK) >>
+ DRM_MODE_FLAG_PIC_AR_BITS_POS;
+}
+
static void
drm_assign_planes(struct weston_output *output_base, void *repaint_data)
{
@@ -3179,25 +3198,44 @@ drm_assign_planes(struct weston_output *output_base,
void *repaint_data)
static struct drm_mode *
choose_mode (struct drm_output *output, struct weston_mode *target_mode)
{
- struct drm_mode *tmp_mode = NULL, *mode;
+ struct drm_mode *tmp_mode = NULL, *mode_fall_back = NULL, *mode;
+ uint8_t src_aspect = 0;
+ uint8_t target_aspect = 0;
+ struct drm_backend *b;
+ b = to_drm_backend(output->base.compositor);
+ target_aspect = target_mode->aspect_ratio;
+ src_aspect = output->base.current_mode->aspect_ratio;
if (output->base.current_mode->width == target_mode->width &&
output->base.current_mode->height == target_mode->height &&
(output->base.current_mode->refresh == target_mode->refresh ||
-target_mode->refresh == 0))
- return to_drm_mode(output->base.current_mode);
+target_mode->refresh == 0)) {
+ if (!b->aspect_ratio_supported || src_aspect == target_aspect)
+ return (struct drm_mode *)output->base.current_mode;
Please keep to_drm_mode() instead of
Re: [PATCH v5] compositor-drm: Add aspect-ratio parsing support
On Tue, 3 Jul 2018 14:42:06 +0530 "Sharma, Shashank" wrote: > Hello Pekka, > > Thanks for your attention and code review of the patch series. > Ankit will respond on rest of the review comments, I would take one > related to mode with implicit aspect ratio information. > Please find my comment inline. > > Regards > Shashank > On 6/28/2018 7:09 PM, Pekka Paalanen wrote: > > On Sun, 20 May 2018 18:33:01 +0530 > > "Nautiyal, Ankit K" wrote: > > > >> From: Ankit Nautiyal > >> > >> The flag bits 19-22 of the connector modes, provide the aspect-ratio > >> information. This information can be stored in flags bits of the > >> weston mode structure, so that it can used for setting a mode with a > >> particular aspect-ratio. > >> Currently, DRM layer supports aspect-ratio with atomic-modesetting by > >> default. For legacy modeset path, the user-space needs to set the > >> drm client cap for aspect-ratio, if it wants aspect-ratio information > >> in modes. > >> > >> This patch: > >> - preserves aspect-ratio flags from kernel video modes and > >>accommodates it in wayland mode. > >> - uses aspect-ratio to pick the appropriate mode during modeset. > >> - changes the mode format in configuration file weston.ini to > >>accommodate aspect-ratio information as: > >>WIDTHxHEIGHT at REFRESH-RATE ASPECT-RATIO > >>The aspect-ratio should be given as . > >> - modifies the man pages to explain the usage of different mode format > >>configurations in weston.ini. > >> > >> v2: As per recommendation from Pekka Paalanen, Quentin Glidic, > >> Daniel Stone, dropped the aspect-ratio info from wayland protocol, > >> thereby avoiding exposure of aspect-ratio to the client. > >> > >> v3: As suggested by Pekka Paalanen, added aspect_ratio field to store > >> aspect-ratio information from the drm. Also added drm client > >> capability for aspect-ratio, as recommended by Daniel Vetter. > >> > >> v4: Minor modifications and fixes as suggested by Pekka Paalanen. > >> > >> v5: Rebased, fixed some styling issues, and added aspect-ratio > >> information while printing weston_modes. > >> Signed-off-by: Ankit Nautiyal > >> > >> Acked-by: Pekka Paalanen (v4) > >> --- > >> libweston/compositor-drm.c | 115 + > >> libweston/compositor-drm.h | 21 +++ > >> libweston/compositor.h | 1 + > >> man/weston.ini.man | 13 +++-- > >> 4 files changed, 136 insertions(+), 14 deletions(-) > > Hi Ankit, > > > > it's been a long while since I last looked at this, so forgive me if I > > go in circles with my review comments. I see the aspect ratio bits are > > in Linus' RC kernel. I would like to get this merged for the next > > release which basically means during the next week if you can. > >> diff --git a/man/weston.ini.man b/man/weston.ini.man > >> index f237fd60..e982cac9 100644 > >> --- a/man/weston.ini.man > >> +++ b/man/weston.ini.man > >> @@ -371,10 +371,15 @@ The DRM backend accepts different modes: > >> .PP > >> .RS 10 > >> .nf > >> -.BR "WIDTHxHEIGHT" "Resolution size width and height in pixels" > >> -.BR "preferred " "Uses the preferred mode" > >> -.BR "current " "Uses the current crt controller mode" > >> -.BR "off " "Disables the output" > >> +.BR "WIDTHxHEIGHT " "Resolution size width and height in > >> pixels" > >> +.BR "WIDTHxHEIGHT@RR" "Resolution as above and refresh-rate > >> in Hertz" > >> +.BR "WIDTHxHEIGHT@RR RATIO " "Resolution as above and aspect-ratio > >> as length:breadth" > > I think the length:breadth could be confusing. H:V? hor:vert? Or simply > > A:B, assuming that aspect ratio is general knowledge. > > > > It would also be worth to list all the acceptable values, because there > > are only few. One cannot give e.g. 8:6 and assume Weston figures out it > > means 4:3. If they are explicitly listed, we could just use RATIO and > > not split the value further. > > > > Since these are specific to the DRM-backend, they would be more logical > > in weston-drm.man. > > > >> + > >> +e.g. 720x576@50 4:3, 1920x1080@60 16:9, 2560x1080@60 64:27, 4096x2160@60 > >> 256:135 etc. > > What is the point of giving the aspect ratio explicitly if the > > resolution directly results in the same aspect ratio? > > > > Shouldn't the examples show cases where it actually matters, i.e. when > > the display shows non-square pixels? E.g. 1024x768 16:9 > > > > The implementation also makes a difference between implicit aspect > > ratio (that is, NONE) and explicit aspect ratio even if the two are > > equal. Is that intentional? Is there an actual difference? > Yes there is an actual difference, let me try to explain this. > For this, we should first understand difference between CEA-modes Vs > non-CEA-modes. As you might already know, CEA defines timings of a video > mode, which is considered as standard for HDMI certification and > compliance testing. CEA defines each and every
Re: [PATCH weston] parse_modeline: Ignore case of {h,v}sync flags
Hi, On Tue, Jul 03, 2018 at 12:04:37PM +0300, Pekka Paalanen wrote: > On Tue, 26 Jun 2018 20:40:08 +0200 > Guido Günther wrote: > > > Some modeline generators put out e.g. +HSync instead of +hsync. Accept > > that too since it's not ambigous. > > > > Signed-off-by: Guido Günther > > --- > > libweston/compositor-drm.c | 8 > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c > > index 8b1ea66d..1c9b7134 100644 > > --- a/libweston/compositor-drm.c > > +++ b/libweston/compositor-drm.c > > @@ -4341,16 +4341,16 @@ parse_modeline(const char *s, drmModeModeInfo *mode) > > return -1; > > > > mode->clock = fclock * 1000; > > - if (strcmp(hsync, "+hsync") == 0) > > + if (strcasecmp(hsync, "+hsync") == 0) > > mode->flags |= DRM_MODE_FLAG_PHSYNC; > > - else if (strcmp(hsync, "-hsync") == 0) > > + else if (strcasecmp(hsync, "-hsync") == 0) > > mode->flags |= DRM_MODE_FLAG_NHSYNC; > > else > > return -1; > > > > - if (strcmp(vsync, "+vsync") == 0) > > + if (strcasecmp(vsync, "+vsync") == 0) > > mode->flags |= DRM_MODE_FLAG_PVSYNC; > > - else if (strcmp(vsync, "-vsync") == 0) > > + else if (strcasecmp(vsync, "-vsync") == 0) > > mode->flags |= DRM_MODE_FLAG_NVSYNC; > > else > > return -1; > > Hi, > > I suppose this does no harm. Pushed: Yeah, that was my intend when sending the patch. >35280448..92278e08 master -> master Thanks, -- Guido ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH v5] compositor-drm: Add aspect-ratio parsing support
Hello Pekka,
Thanks for your attention and code review of the patch series.
Ankit will respond on rest of the review comments, I would take one
related to mode with implicit aspect ratio information.
Please find my comment inline.
Regards
Shashank
On 6/28/2018 7:09 PM, Pekka Paalanen wrote:
On Sun, 20 May 2018 18:33:01 +0530
"Nautiyal, Ankit K" wrote:
From: Ankit Nautiyal
The flag bits 19-22 of the connector modes, provide the aspect-ratio
information. This information can be stored in flags bits of the
weston mode structure, so that it can used for setting a mode with a
particular aspect-ratio.
Currently, DRM layer supports aspect-ratio with atomic-modesetting by
default. For legacy modeset path, the user-space needs to set the
drm client cap for aspect-ratio, if it wants aspect-ratio information
in modes.
This patch:
- preserves aspect-ratio flags from kernel video modes and
accommodates it in wayland mode.
- uses aspect-ratio to pick the appropriate mode during modeset.
- changes the mode format in configuration file weston.ini to
accommodate aspect-ratio information as:
WIDTHxHEIGHT at REFRESH-RATE ASPECT-RATIO
The aspect-ratio should be given as .
- modifies the man pages to explain the usage of different mode format
configurations in weston.ini.
v2: As per recommendation from Pekka Paalanen, Quentin Glidic,
Daniel Stone, dropped the aspect-ratio info from wayland protocol,
thereby avoiding exposure of aspect-ratio to the client.
v3: As suggested by Pekka Paalanen, added aspect_ratio field to store
aspect-ratio information from the drm. Also added drm client
capability for aspect-ratio, as recommended by Daniel Vetter.
v4: Minor modifications and fixes as suggested by Pekka Paalanen.
v5: Rebased, fixed some styling issues, and added aspect-ratio
information while printing weston_modes.
Signed-off-by: Ankit Nautiyal
Acked-by: Pekka Paalanen (v4)
---
libweston/compositor-drm.c | 115 +
libweston/compositor-drm.h | 21 +++
libweston/compositor.h | 1 +
man/weston.ini.man | 13 +++--
4 files changed, 136 insertions(+), 14 deletions(-)
Hi Ankit,
it's been a long while since I last looked at this, so forgive me if I
go in circles with my review comments. I see the aspect ratio bits are
in Linus' RC kernel. I would like to get this merged for the next
release which basically means during the next week if you can.
I didn't see the aspect ratio bits in libdrm master yet, I guess it
should be ok to re-import the headers from kernel to libdrm by now.
diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
index 287431eb..e1d79e49 100644
--- a/libweston/compositor-drm.c
+++ b/libweston/compositor-drm.c
@@ -73,6 +73,10 @@
#define DRM_CLIENT_CAP_UNIVERSAL_PLANES 2
#endif
+#ifndef DRM_CLIENT_CAP_ASPECT_RATIO
+#define DRM_CLIENT_CAP_ASPECT_RATIO4
+#endif
+
#ifndef DRM_CAP_CURSOR_WIDTH
#define DRM_CAP_CURSOR_WIDTH 0x8
#endif
@@ -272,6 +276,8 @@ struct drm_backend {
uint32_t pageflip_timeout;
bool shutting_down;
+
+ bool aspect_ratio_supported;
};
struct drm_mode {
@@ -3049,6 +3055,19 @@ err:
drmModeSetCursor(b->drm.fd, output->crtc_id, 0, 0, 0);
}
+/*
+ * Get the aspect-ratio from drmModeModeInfo mode flags.
+ *
+ * @param drm_mode_flags- flags from drmModeModeInfo structure.
+ * @returns aspect-ratio as encoded in enum 'weston_mode_aspect_ratio'.
+ */
+static uint8_t
+drm_to_weston_mode_aspect_ratio(uint32_t drm_mode_flags)
+{
+ return (drm_mode_flags & DRM_MODE_FLAG_PIC_AR_MASK) >>
+ DRM_MODE_FLAG_PIC_AR_BITS_POS;
+}
+
static void
drm_assign_planes(struct weston_output *output_base, void *repaint_data)
{
@@ -3179,25 +3198,44 @@ drm_assign_planes(struct weston_output *output_base,
void *repaint_data)
static struct drm_mode *
choose_mode (struct drm_output *output, struct weston_mode *target_mode)
{
- struct drm_mode *tmp_mode = NULL, *mode;
+ struct drm_mode *tmp_mode = NULL, *mode_fall_back = NULL, *mode;
+ uint8_t src_aspect = 0;
+ uint8_t target_aspect = 0;
+ struct drm_backend *b;
+ b = to_drm_backend(output->base.compositor);
+ target_aspect = target_mode->aspect_ratio;
+ src_aspect = output->base.current_mode->aspect_ratio;
if (output->base.current_mode->width == target_mode->width &&
output->base.current_mode->height == target_mode->height &&
(output->base.current_mode->refresh == target_mode->refresh ||
-target_mode->refresh == 0))
- return to_drm_mode(output->base.current_mode);
+target_mode->refresh == 0)) {
+ if (!b->aspect_ratio_supported || src_aspect == target_aspect)
+ return (struct drm_mode *)output->base.current_mode;
Please keep to_drm_mode() instead of converting it back to a cast.
+ }
wl_list_for_each(mode, >base.mode_list,
Re: [PATCH weston] parse_modeline: Ignore case of {h,v}sync flags
On Tue, 26 Jun 2018 20:40:08 +0200 Guido Günther wrote: > Some modeline generators put out e.g. +HSync instead of +hsync. Accept > that too since it's not ambigous. > > Signed-off-by: Guido Günther > --- > libweston/compositor-drm.c | 8 > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c > index 8b1ea66d..1c9b7134 100644 > --- a/libweston/compositor-drm.c > +++ b/libweston/compositor-drm.c > @@ -4341,16 +4341,16 @@ parse_modeline(const char *s, drmModeModeInfo *mode) > return -1; > > mode->clock = fclock * 1000; > - if (strcmp(hsync, "+hsync") == 0) > + if (strcasecmp(hsync, "+hsync") == 0) > mode->flags |= DRM_MODE_FLAG_PHSYNC; > - else if (strcmp(hsync, "-hsync") == 0) > + else if (strcasecmp(hsync, "-hsync") == 0) > mode->flags |= DRM_MODE_FLAG_NHSYNC; > else > return -1; > > - if (strcmp(vsync, "+vsync") == 0) > + if (strcasecmp(vsync, "+vsync") == 0) > mode->flags |= DRM_MODE_FLAG_PVSYNC; > - else if (strcmp(vsync, "-vsync") == 0) > + else if (strcasecmp(vsync, "-vsync") == 0) > mode->flags |= DRM_MODE_FLAG_NVSYNC; > else > return -1; Hi, I suppose this does no harm. Pushed: 35280448..92278e08 master -> master Thanks, pq pgpVmghQeJVr7.pgp Description: OpenPGP digital signature ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH weston] simple-dmabuf-drm: fix build with --disable-egl
On Mon, 2 Jul 2018 17:22:30 +0200 Emilio Pozuelo Monfort wrote: > Signed-off-by: Emilio Pozuelo Monfort > --- > I tried a build with --disable-egl as I didn't have the headers > installed, and it broke here. The EGL usage here seemed optional so I > did that, but I didn't run-test the result. If it would make more sense > to disable the client if EGL support is disabled I can do that. > > clients/simple-dmabuf-drm.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/clients/simple-dmabuf-drm.c b/clients/simple-dmabuf-drm.c > index fcab30e5..1ae3a2dd 100644 > --- a/clients/simple-dmabuf-drm.c > +++ b/clients/simple-dmabuf-drm.c > @@ -863,6 +863,7 @@ create_display(int opts, int format) > display->req_dmabuf_immediate = opts & OPT_IMMEDIATE; > display->req_dmabuf_modifiers = (format == DRM_FORMAT_NV12); > > +#if ENABLE_EGL > /* >* hard code format if the platform egl doesn't support format >* querying / advertising. > @@ -871,6 +872,7 @@ create_display(int opts, int format) > if (extensions && !weston_check_egl_extension(extensions, > "EGL_EXT_image_dma_buf_import_modifiers")) > display->xrgb_format_found = 1; > +#endif > > display->registry = wl_display_get_registry(display->display); > wl_registry_add_listener(display->registry, Hi, that's very strange. This program does not use EGL or even GBM, that's a completely dead hunk of code you're ifdeffing there as I can see. Would be better to just remove it completely, and make sure the build does not link libEGL or GBM. Include for shared/platform.h should be useless too. Thanks, pq pgp_UUw9OSWrO.pgp Description: OpenPGP digital signature ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
RE: Wayland content-protection extension
> -Original Message- > From: Pekka Paalanen [mailto:ppaala...@gmail.com] > Sent: Tuesday, July 3, 2018 1:14 PM > To: C, Ramalingam > Cc: Arnaud Vrac ; Nautiyal, Ankit K > ; wayland-devel@lists.freedesktop.org; Daniel > Stone ; Sharma, Shashank > > Subject: Re: Wayland content-protection extension > > On Mon, 2 Jul 2018 20:37:23 +0530 > Ramalingam C wrote: > > > On Monday 02 July 2018 07:26 PM, Pekka Paalanen wrote: > > > > Ok, so it is acceptable to transmit protected content to a > > > potentially unprotected display until the app reacts to the event. > > > That makes things very simple. > > > > Yes. > > > > Even HDCP spec provides the tolerance of at least 1Sec for link integrity > > check. > > As a design we should inform the link status change to the content > > provider as soon as possible. > > Hi Ram, > > I've read your three replies, and it all sounds good to me. I'll be on > holidays from > July 12 to August 1 and likely won't respond during that time, so I hope > you're all > set. Thanks a lot Pekka! We are good to start. If we need some help while proceeding, we will reach out to you and the community! Thanks, Ram. > > > Thanks, > pq ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: Wayland content-protection extension
On Mon, 2 Jul 2018 20:37:23 +0530 Ramalingam C wrote: > On Monday 02 July 2018 07:26 PM, Pekka Paalanen wrote: > > Ok, so it is acceptable to transmit protected content to a potentially > > unprotected display until the app reacts to the event. That makes > > things very simple. > > Yes. > > Even HDCP spec provides the tolerance of at least 1Sec for link integrity > check. > As a design we should inform the link status change to the content provider as > soon as possible. Hi Ram, I've read your three replies, and it all sounds good to me. I'll be on holidays from July 12 to August 1 and likely won't respond during that time, so I hope you're all set. Thanks, pq pgpFrTKI9RPWC.pgp Description: OpenPGP digital signature ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: Users of libweston?
On Mon, 2 Jul 2018 09:34:40 -0500 Matt Hoosier wrote: > On Mon, Jul 2, 2018 at 4:10 AM Pekka Paalanen wrote: > > > On Fri, 29 Jun 2018 10:05:58 -0500 > > Matt Hoosier wrote: > > > > > Hi all, > > > > > > Pekka's recent comments about wanting to enable set-top boxes built with > > > libweston to do DRM content got me to wondering: > > > > > > Who all is using libweston directly (as opposed to running > > /usr/bin/weston > > > possibly with custom shells plugins or similar)? For my own purposes, I > > > just use the full compositor because it's pretty lean and mean anyway, > > and > > > I can do what I need by loading plugins. > > > > Hi Matt, > > > > I wouldn't be surprised if there weren't many users yet. There is a > > huge amount of things I'd like to do before I could comfortably propose > > using libweston. I still think it needs to be a goal in mind all the > > time though, otherwise we'll never get there. :-) > > > > IMO the major point of using libweston instead of weston is to be able > > to customize the UX any way you want - all the stuff and policy that is > > currecntly hardcoded in main.c and the desktop-shell plugin. Making all > > configurable is probably not feasible. > > > > My hope with gaining set-top box etc. use cases is to gather more people > > developing for Weston, people who could be dedicated in the long run. > > Maybe that could gain us more upstream maintainers. > > > > > > Thanks, > > pq > > > > Hi -- > > Yeah, that all makes sense. I certainly didn't mean to imply any criticism > with the question. Oh, none taken! I'm happy to explain what vague vision I might have, since discussion will help to clarify it and seeking acceptance is important to me. > I wasn't yet following the conversations on > wayland-devel when Weston got overhauled to split out its core compositor > features as libweston, so I wasn't positive exactly who the intended users > are. I suppose that maybe there's little chance that Mutter or Kwin would > ever adopt this (although that would be an amazing proof of generality). > > The question mainly came from my observation that the modularity of the > plugin system in Weston seems _so effective_ that it almost completely > subsumes the need for using libweston. I run several out-of-tree plugins > (one of them providing an alternative to desktop-shell, and a few others > doing random runtime things such as integrating with systemd -- yes, I > know there's an official Weston plugin for that ;) ), and the > /usr/bin/weston entrypoint still seems to hold up very well for all this. > > The bit about being able to work around policy choices made in main.c does > make sense. > > On the topic of modularity: what was the reason why the libweston overhaul > nixed the ability for out-of-tree plugins to contribute new backends? (This > is just a curiosity question -- I'm don't have any particular opposition.) It was an idea to limit the API surface of libweston. It's the same for renderers, those are meant to be only built-in so far. There is a huge amount of API to go through, separate into public and private (actually to frontend, backend, renderer, shell, other plugins, ...), sanitize, and possibly redesign, that I wanted to restrict the scope at first. Once the stuff used by main.c and shell plugins is in shape, we could look at doing a backend ABI if there's demand. But, if libweston has public ABIs in every direction, it might be hard to eventually stabilize libweston. I suppose one reason why the plugin model is currently so powerful is that libweston doesn't really have a private API, everything is more or less public. It's the trade-off between power/flexibility and stability. Exposing everything lets external plugins do anything they want, but it also means we break the ABI on practically every release. Reducing ABI surface to avoid breaking it in every turn will limit what plugins can do. I've been thinking that a stable libweston ABI is a worthy long-term goal. Is it? Or is the libweston major version bumping working too well? ;-) Thanks, pq pgpdS1pR6Wh9V.pgp Description: OpenPGP digital signature ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH] compositor-headless: Report a more realistic physical size
On Mon, 02 Jul 2018 10:29:48 -0400 Simon Ser wrote: > > On Thu, 12 Apr 2018 09:31:48 +0200 > > Johan Klokkhammer Helsing wrote: > > > > > Some clients rely on the physical size to determine the physical DPI. > > > With the > > > previous implementation, we would report 1px==1mm, which is a DPI of 25.4, > > > which is incredibly low. > > > > > > The problem is solved by setting a physical size so the DPI is close to 72 > > > instead. If the output is scaled, the DPI is set to the corresponding > > > multiple > > > of 72. > > > > > > This makes the headless backend more usable for automated testing of DPI > > > sensitive functionality such as point sized fonts. > > > > > > Signed-off-by: Johan Klokkhammer Helsing > > > --- > > > libweston/compositor-headless.c | 16 > > > 1 file changed, 12 insertions(+), 4 deletions(-) > > > > Hi, > > > > this is a good idea, but could you rebase this patch to master? > > > > > > Thanks, > > pq > > Hi, > > Would it make sense to change the protocol to allow compositors to send a zero > physical size in case it isn't relevant? I think it's often full of lies anyway, because usually the reported numbers come directly from EDID. I've heard rumours of something like width=16 height=9 (mm? cm?) etc. when the physical size is unknowable (e.g. a projector) but they still want to send the aspect ratio. If existing toolkits can cope with it, zero would be fine. Whether they do, I really don't know. Something about aspect ratio might need to be taken into account though. We've never really had non-square pixels, so the assumption of square pixels is probably used throughout the stack. If output information implied non-square pixels, does it mean client pixels will be shown equally non-square or are they scaled assuming they are originally square? OTOH, headless reporting a sensible fake size for a fake output is still a good idea in my opinion. Thanks, pq pgpSgvOhLkjkh.pgp Description: OpenPGP digital signature ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
