Re: [PATCH weston] build: don't manually parse the weston.ini.in templates
On 28 June 2018 at 10:58, Quentin Glidic
wrote:
> On 6/27/18 3:04 PM, Emil Velikov wrote:
>>
>> From: Emil Velikov
>>
>> Adding those to configure.ac ensures that:
>> - the weston.ini files are {re,}generated only when needed
>> - the .in files are shipped in the tarball
>> - all the manual handling of the above can be removed ;-)
>
>
> Did you actually test that?
> In configure.ac, "bindir" is "${prefix}/bin" (default value), so you’d end
> up with "path=${prefix}/bin/weston-flower", and obviously, it won’t launch.
> Also, though it’s not that used, you can override directories at "make"
> time.
> In the end, Makefile is the only place we know the full usable value for
> directories. (Otherwise, the only case it’ll work is when you pass all the
> directories as full paths to "./configure".)
>
Hmm I think a good point is to step back a bit and say how the
weston.ini files should be used.
Are they meant for builddir only usage (a), are they sort of a
template that one should copy (b) or other (c).
The patch from Emre suggest (b). My current assumption is on the same
page, based on the bindir/weston-foo (and friends) instances in
weston.ini.in.
I wonder if "make allows you to override everything" is not it's bane.
Just because you can, don't mean one should.
All in all people who thinker with that should really know what they're doing.
Having the weston.ini files generated at "make all" means that those
variables are honoured only at "make all".
Aka relying that you can override them is a recipe for disaster.
-Emil
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel
RE: [PATCH 0/5] Implement support for drm properties "GAMMA_LUT" and "CTM"
>Hi Harsha, >Weston needs to be rebuilt. But, no new library versions dependencies. > >All right. > >On Mon, Jun 25, 2018 at 10:21 AM, Harsha Manjula Mallikarjun (RBEI/ECF3) > wrote: >Hi Matheus, > >>From: Matheus Santana [mailto:e...@cin.ufpe.br] >>Sent: Friday, June 22, 2018 8:33 PM >>To: Harsha Manjula Mallikarjun (RBEI/ECF3) >> >>Cc: wayland-devel@lists.freedesktop.org >>Subject: Re: [PATCH 0/5] Implement support for drm properties "GAMMA_LUT" and >>"CTM" >> >>Hi Harsha, >>I am unable to run the tests you introduced. Tests execution gets stuck after >>reporting success for surface-global-test. > >Surface-global-test terminates the compositor. May be this is the reason. I do >not see a stuck test rather a terminated compositor. > >For testing I modified Weston.ini as follows: >modules=ivi-controller.so, surface-global-test.so,gamma-test.so,ctm-test.so > >Are you running tests in an another way? > >I'm running them by just issuing `make check` after building weston. I >basically execute three commands (within weston's root dir): >$ ./autogen.sh --prefix=$WLD >$ make >$ make check > >The output for `autogen.sh` (which is the same regardless patches application) >is attached. >All tests in suite run fine (24 passed, 1 skipped) when running these commands >without the paches. >I'm applying the patches by downloading the mbox and `git am patches.mbox`. >Are you able to run all tests in suite by just `make check` after building the >project? Thank you for providing this information. I was cross compiling for an x86 hardware where ctm and gamma_lut are supported. It was not possible to run "make check" with reasonable efforts spent. Now fixed the stuck problem with version v3. Also tested on Ubuntu and on a custom hardware. On custom hardware I have weston running with drm-backend. > >Best regards, >Matheus >P.S.: it shouldn't make a difference but I've applied the new version you >provided and tried it again without luck. > > >Is it necessary building weston against any newer libraries versions for >running the new tests? > >Thanks, >Matheus > >>On Fri, Jun 22, 2018 at 10:23 AM, >>wrote: >>From: Harsha M M >> >>current drm backend uses drmModeCrtcSetGamma to set the gamma look up table. >>This api is a legacy now. DRM backend should support setting of gamma look up >>table through GAMMA_LUT property to keep up with the latest DRM. >> >>DRM has support for setting color transformation matrix. This is useful for >>tuning additional display settings like brightness, saturation and hue. >> >>This patch series adds support for setting of these two drm properties along >>with respective tests. >> >>Harsha M M (5): >> compositor-drm: Implement support for GAMMA_LUT drm property >> libweston: provide support to set color transformation matrix for >>output >> compositor-drm: add support for CTM property >> tests: add test for setting gamma >> tests: add test for color transformation matrix >> >> Makefile.am| 13 ++- >> libweston/compositor-drm.c | 149 +-- >> libweston/compositor.h | 12 +++ >> tests/ctm-test.c | 210 ++ >> tests/gamma-test.c | 248 >> + >> 5 files changed, 625 insertions(+), 7 deletions(-) >> create mode 100644 tests/ctm-test.c >> create mode 100644 tests/gamma-test.c >> >>-- >>2.7.4 >> >>___ >>wayland-devel mailing list >>wayland-devel@lists.freedesktop.org >>https://lists.freedesktop.org/mailman/listinfo/wayland-devel Best regards, Harsha MM RBEI/ECF3 ___ 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 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_RATIO 4
> +#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, base.link) {
> + uint32_t flags = mode->mode_info.flags;
> +
> + src_aspect = drm_to_weston_mode_aspect_ratio(flags);
[PATCH v3 weston 1/5] compositor-drm: Implement support for GAMMA_LUT drm property
From: Harsha M M
drmModeCrtcSetGamma is a legacy now. Use the generic drm property
to set Gamma. If GAMMA_LUT is not supported then legacy api will
be used.
Signed-off-by: Harsha M M
---
libweston/compositor-drm.c | 83 ++
1 file changed, 77 insertions(+), 6 deletions(-)
diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
index 8b1ea66..27c95da 100644
--- a/libweston/compositor-drm.c
+++ b/libweston/compositor-drm.c
@@ -196,12 +196,16 @@ static const struct drm_property_info connector_props[] =
{
enum wdrm_crtc_property {
WDRM_CRTC_MODE_ID = 0,
WDRM_CRTC_ACTIVE,
+ WDRM_CRTC_GAMMA_LUT,
+ WDRM_CRTC_GAMMA_LUT_SIZE,
WDRM_CRTC__COUNT
};
static const struct drm_property_info crtc_props[] = {
[WDRM_CRTC_MODE_ID] = { .name = "MODE_ID", },
[WDRM_CRTC_ACTIVE] = { .name = "ACTIVE", },
+ [WDRM_CRTC_GAMMA_LUT] = { .name = "GAMMA_LUT", },
+ [WDRM_CRTC_GAMMA_LUT_SIZE] = { .name = "GAMMA_LUT_SIZE", },
};
/**
@@ -1752,16 +1756,63 @@ drm_output_set_gamma(struct weston_output *output_base,
struct drm_output *output = to_drm_output(output_base);
struct drm_backend *backend =
to_drm_backend(output->base.compositor);
+ uint32_t gamma_prop_id;
+ uint32_t gamma_size_prop_id;
+ struct drm_color_lut *lut = NULL;
+ uint32_t gamma_blobid = 0;
+ uint32_t loop;
+
/* check */
if (output_base->gamma_size != size)
return;
- rc = drmModeCrtcSetGamma(backend->drm.fd,
-output->crtc_id,
-size, r, g, b);
- if (rc)
- weston_log("set gamma failed: %m\n");
+ gamma_prop_id = output->props_crtc[WDRM_CRTC_GAMMA_LUT].prop_id;
+ gamma_size_prop_id = output->props_crtc[WDRM_CRTC_GAMMA_LUT_SIZE].
+ prop_id;
+
+ if (gamma_prop_id && gamma_size_prop_id) {
+ lut = malloc(sizeof(struct drm_color_lut) * size);
+ if (!lut) {
+ weston_log("failed to allocate memory for gamma lut\n");
+ goto out;
+ }
+
+ for (loop = 0; loop < size; loop++) {
+ lut[loop].red = r[loop];
+ lut[loop].green = g[loop];
+ lut[loop].blue = b[loop];
+ }
+
+ rc = drmModeCreatePropertyBlob(backend->drm.fd, lut,
+ sizeof(struct drm_color_lut) * size, _blobid);
+ if (rc) {
+ weston_log("failed to create drm gamma blob: %m\n");
+ goto out;
+ }
+
+ rc = drmModeObjectSetProperty(backend->drm.fd, output->crtc_id,
+ DRM_MODE_OBJECT_CRTC, gamma_prop_id, gamma_blobid);
+
+ if (rc)
+ weston_log("set of gamma lut property failed for crtc
%d: %m\n",
+output->crtc_id);
+ } else {
+ rc = drmModeCrtcSetGamma(backend->drm.fd,
+output->crtc_id,
+size, r, g, b);
+ if (rc)
+ weston_log("set gamma failed for crtc %d: %m\n",
+ output->crtc_id);
+
+ return;
+ }
+out:
+ if (lut)
+ free(lut);
+ if (gamma_blobid)
+ drmModeDestroyPropertyBlob(backend->drm.fd, gamma_blobid);
+
}
/* Determine the type of vblank synchronization to use for the output.
@@ -4758,15 +4809,35 @@ drm_output_init_gamma_size(struct drm_output *output)
{
struct drm_backend *backend = to_drm_backend(output->base.compositor);
drmModeCrtc *crtc;
+ uint32_t gamma_prop_id;
+ uint32_t gamma_size_prop_id;
+ struct drm_property_info *props_crtc;
+ drmModeObjectProperties *props;
assert(output->base.compositor);
assert(output->crtc_id != 0);
+
crtc = drmModeGetCrtc(backend->drm.fd, output->crtc_id);
if (!crtc)
return -1;
output->base.gamma_size = crtc->gamma_size;
-
+ props_crtc = >props_crtc[0];
+ gamma_prop_id = props_crtc[WDRM_CRTC_GAMMA_LUT].prop_id;
+ gamma_size_prop_id = props_crtc[WDRM_CRTC_GAMMA_LUT_SIZE].prop_id;
+
+ if (gamma_prop_id && gamma_size_prop_id) {
+ props = drmModeObjectGetProperties(backend->drm.fd,
+ output->crtc_id,
+ DRM_MODE_OBJECT_CRTC);
+ if (props) {
+ output->base.gamma_size = drm_property_get_value(
+ _crtc[WDRM_CRTC_GAMMA_LUT_SIZE],
+ props,
+
[PATCH v3 weston 5/5] tests: add test for color transformation matrix
From: Harsha M M
v3:
--Terminate the compositor after test completion to fix stuck
problem during make check
--Do not launch client binary as it is optional for the test.
Signed-off-by: Harsha M M
---
Makefile.am | 8 ++-
tests/ctm-test.c | 207 +++
2 files changed, 214 insertions(+), 1 deletion(-)
create mode 100644 tests/ctm-test.c
diff --git a/Makefile.am b/Makefile.am
index 6b1c560..f8cda15 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -1239,7 +1239,8 @@ module_tests =\
plugin-registry-test.la \
surface-test.la \
surface-global-test.la \
- gamma-test.la
+ gamma-test.la \
+ ctm-test.la
weston_tests = \
bad_buffer.weston \
@@ -1382,6 +1383,11 @@ gamma_test_la_LIBADD = $(test_module_libadd)
gamma_test_la_LDFLAGS = $(test_module_ldflags)
gamma_test_la_CFLAGS = $(AM_CFLAGS) $(COMPOSITOR_CFLAGS)
+ctm_test_la_SOURCES = tests/ctm-test.c
+ctm_test_la_LIBADD = $(test_module_libadd)
+ctm_test_la_LDFLAGS = $(test_module_ldflags)
+ctm_test_la_CFLAGS = $(AM_CFLAGS) $(COMPOSITOR_CFLAGS)
+
#
# Internal tests - tests functionality of the testsuite itself
#
diff --git a/tests/ctm-test.c b/tests/ctm-test.c
new file mode 100644
index 000..03a42c9
--- /dev/null
+++ b/tests/ctm-test.c
@@ -0,0 +1,207 @@
+/*
+ * Copyright © 2018 Advanced Driver Information Technology, GmbH.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining
+ * a copy of this software and associated documentation files (the
+ * "Software"), to deal in the Software without restriction, including
+ * without limitation the rights to use, copy, modify, merge, publish,
+ * distribute, sublicense, and/or sell copies of the Software, and to
+ * permit persons to whom the Software is furnished to do so, subject to
+ * the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the
+ * next paragraph) shall be included in all copies or substantial
+ * portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+ * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
+ * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
+ * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
+ * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ */
+#include "config.h"
+
+#include
+#include
+#include
+#include
+#include
+
+#include "compositor.h"
+#include "compositor/weston.h"
+#include "shared/helpers.h"
+#include
+
+struct ctm_tobject {
+ struct weston_compositor *w_compositor;
+ struct wl_listener output_add_listener;
+ struct wl_listener compositor_destroy;
+ struct wl_display *display;
+ struct wl_event_source *ctm_update_timer;
+ struct wl_list output_list;
+};
+
+struct output_obj {
+ struct weston_output *output;
+ struct wl_listener output_remove_listener;
+ struct weston_matrix* ctm_mat;
+ float brightness;
+ bool test_complete;
+
+ struct wl_list link;
+};
+
+
+/* gamma change interval in milli seconds*/
+#define CTM_UPDATE_INTERVAL 1000
+
+static void
+compute_ctm_mat(struct weston_matrix *ctm_mat, float brightness)
+{
+ uint32_t i;
+
+ /*set all diagonal elements of the matrix*/
+ for (i = 0; i < 4; i++) {
+ ctm_mat->d[(i * 4) + i] = brightness;
+ }
+}
+
+static int
+ctm_update_event(void *data)
+{
+ struct ctm_tobject *obj;
+ struct output_obj *op_obj;
+ bool all_tests_complete = true;
+
+ obj = (struct ctm_tobject *)data;
+
+ wl_list_for_each(op_obj, >output_list, link) {
+ /*compute Ctm lookup table*/
+ if (!op_obj->test_complete) {
+ op_obj->brightness += 0.1;
+ if (op_obj->brightness > 2.0) {
+ op_obj->test_complete = true;
+ /*End the test with ctm set to 1*/
+ op_obj->brightness = 1.0;
+ }
+
+
+ compute_ctm_mat(op_obj->ctm_mat, op_obj->brightness);
+
+ op_obj->output->set_ctm(op_obj->output,
+ op_obj->ctm_mat);
+
+ all_tests_complete=false;
+ }
+ }
+ if (all_tests_complete) {
+ if (obj->ctm_update_timer) {
+ wl_event_source_remove(obj->ctm_update_timer);
+ obj->ctm_update_timer = NULL;
+ }
+
[PATCH v3 weston 2/5] libweston: provide support to set color transformation matrix for output
From: Harsha M M
v2:
--Fix grammatical errors in set_ctm interface description
Signed-off-by: Harsha M M
---
libweston/compositor.h | 12
1 file changed, 12 insertions(+)
diff --git a/libweston/compositor.h b/libweston/compositor.h
index c2c40ee..484b489 100644
--- a/libweston/compositor.h
+++ b/libweston/compositor.h
@@ -270,6 +270,18 @@ struct weston_output {
uint16_t *g,
uint16_t *b);
+ /** Set color tranformation matrix for output
+*
+* @param output_base: is the output to set tranformation for.
+* @param ctm_matrix: 4x4 floating point matrix. Only matrix
+*member "d" of weston_matrix is relevant here.
+*
+* One of the examples of using this matrix is, for tuning the output
+* color with respect to hue saturation and brightness.
+*/
+ void (*set_ctm)(struct weston_output *output_base,
+ struct weston_matrix *ctm_matrix);
+
struct weston_timeline_object timeline;
bool enabled; /**< is in the output_list, not pending list */
--
2.7.4
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel
[PATCH v3 weston 3/5] compositor-drm: add support for CTM property
From: Harsha M M
Implement support for setting color transformation matrix
using drm CTM property.
Signed-off-by: Harsha M M
---
libweston/compositor-drm.c | 66 ++
1 file changed, 66 insertions(+)
diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
index 27c95da..7aa30a7 100644
--- a/libweston/compositor-drm.c
+++ b/libweston/compositor-drm.c
@@ -198,6 +198,7 @@ enum wdrm_crtc_property {
WDRM_CRTC_ACTIVE,
WDRM_CRTC_GAMMA_LUT,
WDRM_CRTC_GAMMA_LUT_SIZE,
+ WDRM_CRTC_CTM,
WDRM_CRTC__COUNT
};
@@ -206,6 +207,7 @@ static const struct drm_property_info crtc_props[] = {
[WDRM_CRTC_ACTIVE] = { .name = "ACTIVE", },
[WDRM_CRTC_GAMMA_LUT] = { .name = "GAMMA_LUT", },
[WDRM_CRTC_GAMMA_LUT_SIZE] = { .name = "GAMMA_LUT_SIZE", },
+ [WDRM_CRTC_CTM] = { .name = "CTM", },
};
/**
@@ -1748,6 +1750,67 @@ drm_output_render(struct drm_output_state *state,
pixman_region32_t *damage)
>primary_plane.damage, damage);
}
+static int64_t
+drm_convert_to_ctm_format(float coefficient)
+{
+ int64_t ret;
+ int64_t max_coefficient = (int64_t)(1ULL << 31);
+
+ /* convert float to S31.32 format as needed by drm_color_ctm
+* matrix co-effecients*/
+ if (coefficient <= (float)-max_coefficient)
+ ret = (int64_t)(~0ULL);
+ else if (coefficient >= (float)max_coefficient)
+ ret = (int64_t)(~(1ULL << 63));
+ else if (coefficient < 0) {
+ ret = (int64_t)((-coefficient * ((int64_t) 1L << 32)) + 0.5);
+ ret |= 1ULL << 63;
+ } else
+ ret = (int64_t)((coefficient * ((int64_t) 1L << 32)) + 0.5);
+
+ return ret;
+}
+
+static void
+drm_output_set_ctm(struct weston_output *output_base,
+ struct weston_matrix *ctm_matrix)
+{
+ struct drm_output *output = to_drm_output(output_base);
+ struct drm_backend *backend = to_drm_backend(output->base.compositor);
+ struct drm_color_ctm ctm;
+ uint32_t ctm_blobid = 0;
+ uint32_t ctm_prop_id;
+ int row;
+ int col;
+ int rc;
+ float coefficient;
+
+ ctm_prop_id = output->props_crtc[WDRM_CRTC_CTM].prop_id;
+ if (!ctm_prop_id)
+ return;
+
+ for (row = 0; row < 3; row++) {
+ for (col = 0; col < 3; col++) {
+ coefficient = ctm_matrix->d[(row * 4) + col];
+ ctm.matrix[(row * 3) + col] =
+ drm_convert_to_ctm_format(coefficient);
+ }
+ }
+
+ rc = drmModeCreatePropertyBlob(backend->drm.fd, , sizeof(ctm),
+ _blobid);
+ if (!rc) {
+ rc = drmModeObjectSetProperty(backend->drm.fd, output->crtc_id,
+ DRM_MODE_OBJECT_CRTC, ctm_prop_id,
+ ctm_blobid);
+ if (rc)
+ weston_log("failed to set ctm for crtc %d: %m\n",
+ output->crtc_id);
+
+ drmModeDestroyPropertyBlob(backend->drm.fd, ctm_blobid);
+ }
+}
+
static void
drm_output_set_gamma(struct weston_output *output_base,
uint16_t size, uint16_t *r, uint16_t *g, uint16_t *b)
@@ -5166,6 +5229,9 @@ drm_output_enable(struct weston_output *base)
output->base.set_dpms = drm_set_dpms;
output->base.switch_mode = drm_output_switch_mode;
output->base.set_gamma = drm_output_set_gamma;
+
+ if (output->props_crtc[WDRM_CRTC_CTM].prop_id)
+ output->base.set_ctm = drm_output_set_ctm;
if (output->cursor_plane)
weston_compositor_stack_plane(b->compositor,
--
2.7.4
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel
[PATCH v3 weston 0/5] Implement support for drm properties "GAMMA_LUT" and "CTM"
From: Harsha M M This patch series implements support for GAMMA_LUT and CTM drm properties. v3 fixes --Problem with gamma and ctm tests getting stuck during make check. Solution is to terminate the compositor after the test completes. --Fixes white space errors Harsha M M (5): compositor-drm: Implement support for GAMMA_LUT drm property libweston: provide support to set color transformation matrix for output compositor-drm: add support for CTM property tests: add test for setting gamma tests: add test for color transformation matrix Makefile.am| 13 ++- libweston/compositor-drm.c | 149 +-- libweston/compositor.h | 12 +++ tests/ctm-test.c | 207 ++ tests/gamma-test.c | 246 + 5 files changed, 620 insertions(+), 7 deletions(-) create mode 100644 tests/ctm-test.c create mode 100644 tests/gamma-test.c -- 2.7.4 ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
[PATCH v3 weston 4/5] tests: add test for setting gamma
From: Harsha M M
v3:
--Terminate the compositor after test completion to fix stuck
problem during make check
--Do not launch client binary as it is optional for the test.
Signed-off-by: Harsha M M
---
Makefile.am| 7 +-
tests/gamma-test.c | 246 +
2 files changed, 252 insertions(+), 1 deletion(-)
create mode 100644 tests/gamma-test.c
diff --git a/Makefile.am b/Makefile.am
index 551df2b..6b1c560 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -1238,7 +1238,8 @@ shared_tests =\
module_tests = \
plugin-registry-test.la \
surface-test.la \
- surface-global-test.la
+ surface-global-test.la \
+ gamma-test.la
weston_tests = \
bad_buffer.weston \
@@ -1376,6 +1377,10 @@ nodist_libtest_client_la_SOURCES =
\
libtest_client_la_CFLAGS = $(AM_CFLAGS) $(TEST_CLIENT_CFLAGS) $(CAIRO_CFLAGS)
libtest_client_la_LIBADD = libshared.la libtest-runner.la $(TEST_CLIENT_LIBS)
$(CAIRO_LIBS)
+gamma_test_la_SOURCES = tests/gamma-test.c
+gamma_test_la_LIBADD = $(test_module_libadd)
+gamma_test_la_LDFLAGS = $(test_module_ldflags)
+gamma_test_la_CFLAGS = $(AM_CFLAGS) $(COMPOSITOR_CFLAGS)
#
# Internal tests - tests functionality of the testsuite itself
diff --git a/tests/gamma-test.c b/tests/gamma-test.c
new file mode 100644
index 000..d4782ff
--- /dev/null
+++ b/tests/gamma-test.c
@@ -0,0 +1,246 @@
+/*
+ * Copyright © 2018 Advanced Driver Information Technology, GmbH.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining
+ * a copy of this software and associated documentation files (the
+ * "Software"), to deal in the Software without restriction, including
+ * without limitation the rights to use, copy, modify, merge, publish,
+ * distribute, sublicense, and/or sell copies of the Software, and to
+ * permit persons to whom the Software is furnished to do so, subject to
+ * the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the
+ * next paragraph) shall be included in all copies or substantial
+ * portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+ * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
+ * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
+ * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
+ * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ */
+#include "config.h"
+
+#include
+#include
+#include
+#include
+#include
+
+#include "compositor.h"
+#include "compositor/weston.h"
+#include "shared/helpers.h"
+#include
+
+struct gamma_tobject {
+ struct weston_compositor *w_compositor;
+ struct wl_listener output_add_listener;
+ struct wl_listener compositor_destroy;
+ struct wl_display *display;
+ struct wl_event_source *gamma_update_timer;
+ struct wl_list output_list;
+};
+
+struct output_obj {
+ struct weston_output *output;
+ struct wl_listener output_remove_listener;
+ float gamma;
+ bool test_complete;
+
+ struct wl_list link;
+};
+
+
+/* gamma change interval in milli seconds*/
+#define GAMMA_UPDATE_INTERVAL 1000
+#define COLOR_COMPONENT_DEPTH 16
+#define GET_GAMMA_COEFF(color, max_color, exp) pow((double)(color) / \
+ (double)(max_color), \
+ ((double)1.0 / (double)exp))
+
+static void
+compute_gamma_lut(float gamma, uint16_t gamma_sz,
+ uint16_t *r, uint16_t *g, uint16_t *b)
+{
+ double r_coeff;
+ double g_coeff;
+ double b_coeff;
+ uint32_t r_value;
+ uint32_t b_value;
+ uint32_t g_value;
+ uint32_t l_value = 0;
+ uint32_t max_value = (((uint32_t)1 << COLOR_COMPONENT_DEPTH) - 1);
+ uint16_t l_index;
+ uint16_t step_size;
+ uint16_t step_fraction;
+
+ step_size = max_value / (gamma_sz - 1);
+ step_fraction = max_value % (gamma_sz - 1);
+
+ for (l_index = 0; l_index < gamma_sz; l_index++)
+ {
+ /* Compute integral part and closest round up for contribution
+ * from fractional part */
+ l_value = l_index * step_size +
+ (((l_index * step_fraction) + ((gamma_sz - 1) >> 1)) /
+ (gamma_sz - 1));
+ r_coeff = GET_GAMMA_COEFF(l_value, max_value, 1.0/gamma);
+ g_coeff = GET_GAMMA_COEFF(l_value, max_value, 1.0/gamma);
+ b_coeff = GET_GAMMA_COEFF(l_value, max_value, 1.0/gamma);
[PATCH] fullscreen-shell: Add missing license tag
Although it would probably default to the license at the root of the repository anyway, it's best to be explicit about it, and also be consistent with the other extensions. The copyright holders have been assembled from git history and the README. Signed-off-by: Johan Klokkhammer Helsing --- .../fullscreen-shell-unstable-v1.xml | 25 +++ 1 file changed, 25 insertions(+) diff --git a/unstable/fullscreen-shell/fullscreen-shell-unstable-v1.xml b/unstable/fullscreen-shell/fullscreen-shell-unstable-v1.xml index 7d141ee..1bca7b7 100644 --- a/unstable/fullscreen-shell/fullscreen-shell-unstable-v1.xml +++ b/unstable/fullscreen-shell/fullscreen-shell-unstable-v1.xml @@ -1,6 +1,31 @@ + +Copyright © 2016 Yong Bakos +Copyright © 2015 Jason Ekstrand +Copyright © 2015 Jonas Ådahl + +Permission is hereby granted, free of charge, to any person obtaining a +copy of this software and associated documentation files (the "Software"), +to deal in the Software without restriction, including without limitation +the rights to use, copy, modify, merge, publish, distribute, sublicense, +and/or sell copies of the Software, and to permit persons to whom the +Software is furnished to do so, subject to the following conditions: + +The above copyright notice and this permission notice (including the next +paragraph) shall be included in all copies or substantial portions of the +Software. + +THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL +THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING +FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER +DEALINGS IN THE SOFTWARE. + + Displays a single surface per output. -- 2.18.0 ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
[PATCH] fullscreen-shell: Add missing license tag
Although it would probably default to the license at the root of the repository anyway, it's best to be explicit about it, and also be consistent with the other extensions. The copyright holders have been assembled from git history and the README. Signed-off-by: Jonas Klokkhammer Helsing --- .../fullscreen-shell-unstable-v1.xml | 25 +++ 1 file changed, 25 insertions(+) diff --git a/unstable/fullscreen-shell/fullscreen-shell-unstable-v1.xml b/unstable/fullscreen-shell/fullscreen-shell-unstable-v1.xml index 7d141ee..1bca7b7 100644 --- a/unstable/fullscreen-shell/fullscreen-shell-unstable-v1.xml +++ b/unstable/fullscreen-shell/fullscreen-shell-unstable-v1.xml @@ -1,6 +1,31 @@ + +Copyright © 2016 Yong Bakos +Copyright © 2015 Jason Ekstrand +Copyright © 2015 Jonas Ådahl + +Permission is hereby granted, free of charge, to any person obtaining a +copy of this software and associated documentation files (the "Software"), +to deal in the Software without restriction, including without limitation +the rights to use, copy, modify, merge, publish, distribute, sublicense, +and/or sell copies of the Software, and to permit persons to whom the +Software is furnished to do so, subject to the following conditions: + +The above copyright notice and this permission notice (including the next +paragraph) shall be included in all copies or substantial portions of the +Software. + +THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL +THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING +FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER +DEALINGS IN THE SOFTWARE. + + Displays a single surface per output. -- 2.18.0 ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: client: remove definition of wl_global
On Tue, 26 Jun 2018 19:27:03 +0200 Markus Ongyerth wrote: > Hi, > > I once again seem to be missing the original message for some reason, so if I > failed to properly fix up my headers again, I'm refering to [1] > > Reviewed-By: Markus Ongyerth > > [1] https://patchwork.freedesktop.org/patch/204915/ Came through fine, thanks a bunch :-) Pushed: a9ff9cc..7cbaa87 master -> master Thanks pq pgpJ7grfnLQam.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] doc: update IANA MIME types registry URL
On Mon, 18 Jun 2018 14:18:07 -0300 Matheus Santana wrote: > Reviewed-by: Matheus Santana > > Nice! FTP isn't even working for me (always get unauthorized). Pushed: bb1a8ca..a9ff9cc master -> master The update to the published docs will have to wait for someone to either regenerate them manually or set up CI to publish them. I just filed https://gitlab.freedesktop.org/wayland/wayland/issues/48 Thanks, pq > > On Mon, Jun 18, 2018 at 7:58 AM, Simon Ser wrote: > > > Use a more official one, served over HTTP rather than FTP. > > --- > > doc/publican/sources/Protocol.xml | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/doc/publican/sources/Protocol.xml b/doc/publican/sources/ > > Protocol.xml > > index 9fdee9a..fedaaab 100644 > > --- a/doc/publican/sources/Protocol.xml > > +++ b/doc/publican/sources/Protocol.xml > > @@ -479,7 +479,7 @@ > > > > > >MIME is defined in RFC's 2045-2049. A > > - ftp://ftp.isi.edu/in-notes/iana/assignments/media- > > types/"> > > + https://www.iana.org/assignments/media-types/media- > > types.xhtml"> > >registry of MIME types is maintained by the Internet > > Assigned > >Numbers Authority (IANA). > > > > -- pgprd4CPO0NAC.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-web 2/3] building: use enable-llvm option
On Tue, 26 Jun 2018 15:03:55 +0100 Emil Velikov wrote: > On 21 June 2018 at 19:54, Matheus Santana wrote: > > Fix > > > > error: --enable-llvm is required when building r300 > > > Fwiw, this error is strictly for performance reasons. If having a > LLVM-free driver is more important than performance, one can remove > the check. > That aside, the series is > > Reviewed-by: Emil Velikov Hi, thanks for the patches and the review, all three pushed: ac2b383..8119f6f master -> master I'm not sure what xkbcommon was doing in the libinput section, since Weston itself depends on it still. The section about libunwind could be removed, since Weston does not use it anymore. I suppose it would be nice to not maintain a Mesa build guide but link to an official build guide, or is there a benefit for having a simplified guide here? Thanks, pq pgpyyM2NDaw5r.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] man: remove redundant word in weston.ini(5)
On Tue, 26 Jun 2018 15:38:06 +0100 Emil Velikov wrote: > On 22 June 2018 at 22:00, Matheus Santana wrote: > > Signed-off-by: Matheus Santana > > --- > > man/weston.ini.man | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/man/weston.ini.man b/man/weston.ini.man > > index 027eae0..02c9b03 100644 > > --- a/man/weston.ini.man > > +++ b/man/weston.ini.man > > @@ -468,7 +468,7 @@ denoting the scaling multiplier for the output. > > .RE > > .TP 7 > > .BI "seat=" name > > -The logical seat name that that this output should be associated with. If > > this > > +The logical seat name that this output should be associated with. If this > > is set then the seat's input will be confined to the output that has the > > seat > > set on it. The expectation is that this functionality will be used in a > > multiheaded environment with a single compositor for multiple output and > > input > > Nice catch. > > Reviewed-by: Emil Velikov Pushed: 78a42116..6a699b1a master -> master Thanks, pq pgpdskD84sNLa.pgp Description: OpenPGP digital signature ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH 0/5] Implement support for drm properties "GAMMA_LUT" and "CTM"
On Tue, 26 Jun 2018 13:47:22 -0300 Matheus Santana wrote: > On Tue, Jun 26, 2018 at 11:51 AM, Emil Velikov > wrote: > > Matheus your assumption is correct. make check should pass, without > > any local changes. > > > > Quick look points the following: > > - the drm (compositor-drm) backend supports the new functionality > > - the default backend (see tests/weston-tests-env) is headless > > (compositor-headless) > > > > So we shouldn't run drm-specific tests with `make check`, maybe? Currently we do not have any DRM-specific tests. It is possible to choose the backend to run with, see https://wayland.freedesktop.org/testing.html , and if a test is DRM-specific, it should skip on other backends. Thanks, pq pgpp3YjMAsftZ.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] contributing: review rules for bugs
On Thu, 28 Jun 2018 10:41:43 +0100 Emil Velikov wrote: > On 28 June 2018 at 10:12, Pekka Paalanen wrote: > > It's the spirit of the guidelines that matters, they are not to be > > interpreted by the letter, and there may always be special > > circumstances where the reasonable action is to overlook something. > > Even with the guidelines, we still rely very much on the individual > > judgement. The important thing is to get people to the right mindset in > > general. > > > > Perhaps the above would be worth a paragraph in CONTRUBUTING.md? > > > It was meant as food for thought, inspired by some long threads I've > seen where devs discuss various compiler warnings. > You're spot on being terse and providing a feel (or spirit as you call > it) is the key point. > > Apologies for the distraction this has caused :-\ Oh, no worries, these are important things to discuss, so we set up the right expectations. I think I should add that paragraph to Review Section, exactly to set the right mindset. Thanks, pq pgp2r_Iw61XAE.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] build: don't manually parse the weston.ini.in templates
On 6/27/18 3:04 PM, Emil Velikov wrote:
From: Emil Velikov
Adding those to configure.ac ensures that:
- the weston.ini files are {re,}generated only when needed
- the .in files are shipped in the tarball
- all the manual handling of the above can be removed ;-)
Did you actually test that?
In configure.ac, "bindir" is "${prefix}/bin" (default value), so you’d
end up with "path=${prefix}/bin/weston-flower", and obviously, it won’t
launch.
Also, though it’s not that used, you can override directories at "make"
time.
In the end, Makefile is the only place we know the full usable value for
directories. (Otherwise, the only case it’ll work is when you pass all
the directories as full paths to "./configure".)
Thanks,
Note: the abs_top_builddir for weston-flower was swapped with the correct
bindir. Squashed in here since it was never worked correctly :-\
Signed-off-by: Emil Velikov
---
Shout if you feel strongly about splitting the weston-flower fix.
Based on top of Emre's "ivi-shell: use install paths in example config"
patch.
---
Makefile.am | 22 ++
configure.ac | 2 +-
weston.ini.in | 2 +-
3 files changed, 4 insertions(+), 22 deletions(-)
diff --git a/Makefile.am b/Makefile.am
index 637dd239..2095aa5a 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -12,23 +12,7 @@ BUILT_SOURCES =
AM_DISTCHECK_CONFIGURE_FLAGS = --disable-setuid-install
-EXTRA_DIST = weston.ini.in ivi-shell/weston.ini.in
-
-weston.ini : $(srcdir)/weston.ini.in
- $(AM_V_GEN)$(SED) \
- -e 's|@bindir[@]|$(bindir)|g' \
- -e 's|@abs_top_builddir[@]|$(abs_top_builddir)|g' \
- -e 's|@libexecdir[@]|$(libexecdir)|g' \
- $< > $@
-
-ivi-shell/weston.ini : $(srcdir)/ivi-shell/weston.ini.in
- $(AM_V_GEN)$(MKDIR_P) $(dir $@) && $(SED) \
- -e 's|@bindir[@]|$(bindir)|g' \
- -e 's|@libexecdir[@]|$(libexecdir)|g' \
- -e 's|@westondatadir[@]|$(westondatadir)|g' \
- $< > $@
-
-all-local : weston.ini ivi-shell/weston.ini
+EXTRA_DIST =
AM_CFLAGS = $(GCC_CFLAGS)
@@ -43,9 +27,7 @@ AM_CPPFLAGS = \
-DLIBEXECDIR='"$(libexecdir)"'\
-DBINDIR='"$(bindir)"'
-CLEANFILES = weston.ini\
- ivi-shell/weston.ini\
- internal-screenshot-00.png \
+CLEANFILES = internal-screenshot-00.png\
$(BUILT_SOURCES)
# Libtool race fix
diff --git a/configure.ac b/configure.ac
index 2a62b62a..9b7071e7 100644
--- a/configure.ac
+++ b/configure.ac
@@ -680,7 +680,7 @@ if test "x$enable_systemd_notify" = "xyes"; then
PKG_CHECK_MODULES(SYSTEMD_DAEMON, [libsystemd])
fi
-AC_CONFIG_FILES([Makefile libweston/version.h compositor/weston.pc])
+AC_CONFIG_FILES([Makefile weston.ini ivi-shell/weston.ini libweston/version.h
compositor/weston.pc])
# AC_CONFIG_FILES needs the full name when running autoconf, so we need to use
# libweston_abi_version here, and outside [] because of m4 quoting rules
diff --git a/weston.ini.in b/weston.ini.in
index 257c4ec4..e743cc49 100644
--- a/weston.ini.in
+++ b/weston.ini.in
@@ -38,7 +38,7 @@ path=/usr/bin/google-chrome
[launcher]
icon=/usr/share/icons/gnome/24x24/apps/arts.png
-path=@abs_top_builddir@/weston-flower
+path=@bindir@/weston-flower
[input-method]
path=@libexecdir@/weston-keyboard
--
Quentin “Sardem FF7” Glidic
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH wayland] contributing: review rules for bugs
On 28 June 2018 at 10:12, Pekka Paalanen wrote: > On Wed, 27 Jun 2018 17:49:34 +0100 > Emil Velikov wrote: > >> Hi Pekka, >> >> A couple small ideas come to mind: >> >> On 27 June 2018 at 14:47, Pekka Paalanen wrote: >> > 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. >> > >> > Cc: Daniel Stone >> > Signed-off-by: Pekka Paalanen >> > --- >> > CONTRIBUTING.md | 19 --- >> > 1 file changed, 16 insertions(+), 3 deletions(-) >> > >> > diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md >> > index 70d0eca..51bef89 100644 >> > --- a/CONTRIBUTING.md >> > +++ b/CONTRIBUTING.md >> > @@ -223,11 +223,24 @@ 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. >> Compiler warnings vary greatly on the compiler and it's version. It >> does become a nuisance when committer/reviewer is using a newer >> version which flags dozens of warnings. > > Yes. There is no need to get out of one's way to hunt for possible > warnings, but if someone does notice new warnings, they should be > fixed. If a reviewer gets them but the author/submitter doesn't, the > reviewer can help with them. It's common sense to me. > > I've been wondering if the CI build should actually turn -Werror on, > but that's better to discuss after if/when we migrate to MR-based work > flow. > >> That aside, worth loosening this for patches where existing code is >> copied or moved? > > It is already loosened: they are not new warnings if they were there > before. If code motion results in new warnings, they should ideally be > fixed. If the fix makes the code motion harder to review, it's ok to > fix it in another patch before or after. > > I think we also need to be picky on how much details we are going to > write down here. It is very easy to extend the guidelines with details > so far that it becomes too long to read. That is why I try to be as > terse as possible. > > It's the spirit of the guidelines that matters, they are not to be > interpreted by the letter, and there may always be special > circumstances where the reasonable action is to overlook something. > Even with the guidelines, we still rely very much on the individual > judgement. The important thing is to get people to the right mindset in > general. > > Perhaps the above would be worth a paragraph in CONTRUBUTING.md? > It was meant as food for thought, inspired by some long threads I've seen where devs discuss various compiler warnings. You're spot on being terse and providing a feel (or spirit as you call it) is the key point. Apologies for the distraction this has caused :-\ -Emil ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH wayland] contributing: review rules for bugs
On Wed, 27 Jun 2018 17:49:34 +0100 Emil Velikov wrote: > Hi Pekka, > > A couple small ideas come to mind: > > On 27 June 2018 at 14:47, Pekka Paalanen wrote: > > 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. > > > > Cc: Daniel Stone > > Signed-off-by: Pekka Paalanen > > --- > > CONTRIBUTING.md | 19 --- > > 1 file changed, 16 insertions(+), 3 deletions(-) > > > > diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md > > index 70d0eca..51bef89 100644 > > --- a/CONTRIBUTING.md > > +++ b/CONTRIBUTING.md > > @@ -223,11 +223,24 @@ 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. > Compiler warnings vary greatly on the compiler and it's version. It > does become a nuisance when committer/reviewer is using a newer > version which flags dozens of warnings. Yes. There is no need to get out of one's way to hunt for possible warnings, but if someone does notice new warnings, they should be fixed. If a reviewer gets them but the author/submitter doesn't, the reviewer can help with them. It's common sense to me. I've been wondering if the CI build should actually turn -Werror on, but that's better to discuss after if/when we migrate to MR-based work flow. > That aside, worth loosening this for patches where existing code is > copied or moved? It is already loosened: they are not new warnings if they were there before. If code motion results in new warnings, they should ideally be fixed. If the fix makes the code motion harder to review, it's ok to fix it in another patch before or after. I think we also need to be picky on how much details we are going to write down here. It is very easy to extend the guidelines with details so far that it becomes too long to read. That is why I try to be as terse as possible. It's the spirit of the guidelines that matters, they are not to be interpreted by the letter, and there may always be special circumstances where the reasonable action is to overlook something. Even with the guidelines, we still rely very much on the individual judgement. The important thing is to get people to the right mindset in general. Perhaps the above would be worth a paragraph in CONTRUBUTING.md? > > > > -- In a patch series, every intermediate step produces correct code as well. > > +- In a patch series, every intermediate step produces correct and > > warning-free > > +code as well. > > > It makes sense to move this as the final bullet-point and let it refer > to the whole section. > Namely: > > - In a patch series, every intermediate step adheres to the guidelines above. Yes, a good suggestion. Thanks, pq pgpjg3DhtCnXZ.pgp Description: OpenPGP digital signature ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH libinput 1/5] tools: if the execdir is the builddir, add it to the path
On Thu, 28 Jun 2018 09:51:39 +1000
Peter Hutterer wrote:
> 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.
>
> Signed-off-by: Peter Hutterer
> ---
> Similar-ish to the git approach Emil suggested in [1] but git is a lot more
> involved here. Since we only need this for debugging tools, we can pick the
> simpler approach here and simply ignore most of the corner cases.
>
> [1] https://lists.freedesktop.org/archives/wayland-devel/2018-June/038658.html
Hi Peter,
this is an interesting idea. My worry is that
tools_execdir_is_builddir() might incorrectly return NULL and you
wouldn't know, so I would propose to add a test that specifically tests
this function to return the correct path when running under the test
suite. Probably quite difficult to test the opposite too?
The code looks correct to me, but I haven't read it in the context of
the surrounding code. I might some day steal this for Weston. :-)
Thanks,
pq
>
> meson.build| 1 +
> tools/shared.c | 31 ++-
> tools/shared.h | 3 +++
> 3 files changed, 34 insertions(+), 1 deletion(-)
>
> diff --git a/meson.build b/meson.build
> index 58e909b6..5580086f 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
> diff --git a/tools/shared.c b/tools/shared.c
> index f2ed1fd0..776b3d3e 100644
> --- a/tools/shared.c
> +++ b/tools/shared.c
> @@ -467,18 +467,47 @@ 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];
> + char *pathsep;
> + ssize_t sz;
> +
> + sz = readlink("/proc/self/exe", execdir, sizeof(execdir));
> + if (sz <= 0 || sz == sizeof(execdir))
> + return NULL;
> +
> + 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 e30bc39f..364babe6 100644
> --- a/tools/shared.h
> +++ b/tools/shared.h
> @@ -119,4 +119,7 @@ tools_list_device_quirks(struct quirks_context *ctx,
>void (*callback)(void *userdata, const char *str),
>void *userdata);
>
> +char *
> +tools_execdir_is_builddir(void);
> +
> #endif
pgpWrM02dWC_d.pgp
Description: OpenPGP digital signature
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel
