| Message ID | 20170131160319.9695-3-noralf@tronnes.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jan 31, 2017 at 05:03:14PM +0100, Noralf Trønnes wrote: > Add common functionality needed by many tinydrm drivers. > > Signed-off-by: Noralf Trønnes <noralf@tronnes.org> > Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > Documentation/gpu/tinydrm.rst | 9 + > drivers/gpu/drm/tinydrm/core/Makefile | 2 +- > drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c | 462 +++++++++++++++++++++++++ > include/drm/tinydrm/tinydrm-helpers.h | 100 ++++++ > 4 files changed, 572 insertions(+), 1 deletion(-) > create mode 100644 drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c > create mode 100644 include/drm/tinydrm/tinydrm-helpers.h > > diff --git a/Documentation/gpu/tinydrm.rst b/Documentation/gpu/tinydrm.rst > index ec4a20d..fb256d2 100644 > --- a/Documentation/gpu/tinydrm.rst > +++ b/Documentation/gpu/tinydrm.rst > @@ -19,3 +19,12 @@ Core functionality > > .. kernel-doc:: drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c > :export: > + > +Additional helpers > +================== > + > +.. kernel-doc:: include/drm/tinydrm/tinydrm-helpers.h > + :internal: > + > +.. kernel-doc:: drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c > + :export: > diff --git a/drivers/gpu/drm/tinydrm/core/Makefile b/drivers/gpu/drm/tinydrm/core/Makefile > index 4f14a0f..fb221e6 100644 > --- a/drivers/gpu/drm/tinydrm/core/Makefile > +++ b/drivers/gpu/drm/tinydrm/core/Makefile > @@ -1,3 +1,3 @@ > -tinydrm-y := tinydrm-core.o tinydrm-pipe.o > +tinydrm-y := tinydrm-core.o tinydrm-pipe.o tinydrm-helpers.o > > obj-$(CONFIG_DRM_TINYDRM) += tinydrm.o > diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c > new file mode 100644 > index 0000000..75e4e54 > --- /dev/null > +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c > @@ -0,0 +1,462 @@ > +/* > + * Copyright (C) 2016 Noralf Trønnes > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + */ > + > +#include <drm/tinydrm/tinydrm.h> > +#include <drm/tinydrm/tinydrm-helpers.h> > +#include <linux/backlight.h> > +#include <linux/pm.h> > +#include <linux/spi/spi.h> > +#include <linux/swab.h> > + > +static unsigned int spi_max; > +module_param(spi_max, uint, 0400); > +MODULE_PARM_DESC(spi_max, "Set a lower SPI max transfer size"); > + > +/** > + * tinydrm_merge_clips - Merge clip rectangles > + * @dst: Destination clip rectangle > + * @src: Source clip rectangle(s) > + * @num_clips: Number of @src clip rectangles > + * @flags: Dirty fb ioctl flags > + * @max_width: Maximum width of @dst > + * @max_height: Maximum height of @dst > + * > + * This function merges @src clip rectangle(s) into @dst. If @src is NULL, > + * @max_width and @min_width is used to set a full @dst clip rectangle. > + * > + * Returns: > + * true if it's a full clip, false otherwise > + */ > +bool tinydrm_merge_clips(struct drm_clip_rect *dst, > + struct drm_clip_rect *src, unsigned int num_clips, > + unsigned int flags, u32 max_width, u32 max_height) > +{ > + unsigned int i; > + > + if (!src || !num_clips) { > + dst->x1 = 0; > + dst->x2 = max_width; > + dst->y1 = 0; > + dst->y2 = max_height; > + return true; > + } > + > + dst->x1 = ~0; > + dst->y1 = ~0; > + dst->x2 = 0; > + dst->y2 = 0; > + > + for (i = 0; i < num_clips; i++) { > + if (flags & DRM_MODE_FB_DIRTY_ANNOTATE_COPY) > + i++; > + dst->x1 = min(dst->x1, src[i].x1); > + dst->x2 = max(dst->x2, src[i].x2); > + dst->y1 = min(dst->y1, src[i].y1); > + dst->y2 = max(dst->y2, src[i].y2); > + } > + > + if (dst->x2 > max_width || dst->y2 > max_height || > + dst->x1 >= dst->x2 || dst->y1 >= dst->y2) { > + DRM_DEBUG_KMS("Illegal clip: x1=%u, x2=%u, y1=%u, y2=%u\n", > + dst->x1, dst->x2, dst->y1, dst->y2); > + dst->x1 = 0; > + dst->y1 = 0; > + dst->x2 = max_width; > + dst->y2 = max_height; > + } > + > + return (dst->x2 - dst->x1) == max_width && > + (dst->y2 - dst->y1) == max_height; > +} > +EXPORT_SYMBOL(tinydrm_merge_clips); Should this perhaps be part of drm_rect.c? > +#ifdef CONFIG_BACKLIGHT_CLASS_DEVICE > +/** > + * tinydrm_of_find_backlight - Find backlight device in device-tree > + * @dev: Device > + * > + * This function looks for a DT node pointed to by a property named 'backlight' > + * and uses of_find_backlight_by_node() to get the backlight device. > + * Additionally if the brightness property is zero, it is set to > + * max_brightness. > + * > + * Returns: > + * NULL if there's no backlight property. > + * Error pointer -EPROBE_DEFER if the DT node is found, but no backlight device > + * is found. > + * If the backlight device is found, a pointer to the structure is returned. > + */ > +struct backlight_device *tinydrm_of_find_backlight(struct device *dev) > +{ > + struct backlight_device *backlight; > + struct device_node *np; > + > + np = of_parse_phandle(dev->of_node, "backlight", 0); > + if (!np) > + return NULL; > + > + backlight = of_find_backlight_by_node(np); > + of_node_put(np); > + > + if (!backlight) > + return ERR_PTR(-EPROBE_DEFER); > + > + if (!backlight->props.brightness) { > + backlight->props.brightness = backlight->props.max_brightness; > + DRM_DEBUG_KMS("Backlight brightness set to %d\n", > + backlight->props.brightness); > + } > + > + return backlight; > +} > +EXPORT_SYMBOL(tinydrm_of_find_backlight); > + > +/** > + * tinydrm_enable_backlight - Enable backlight helper > + * @backlight: Backlight device > + * > + * Returns: > + * Zero on success, negative error code on failure. > + */ > +int tinydrm_enable_backlight(struct backlight_device *backlight) > +{ > + unsigned int old_state; > + int ret; > + > + if (!backlight) > + return 0; > + > + old_state = backlight->props.state; > + backlight->props.state &= ~BL_CORE_FBBLANK; > + DRM_DEBUG_KMS("Backlight state: 0x%x -> 0x%x\n", old_state, > + backlight->props.state); > + > + ret = backlight_update_status(backlight); > + if (ret) > + DRM_ERROR("Failed to enable backlight %d\n", ret); > + > + return ret; > +} > +EXPORT_SYMBOL(tinydrm_enable_backlight); > + > +/** > + * tinydrm_disable_backlight - Disable backlight helper > + * @backlight: Backlight device > + * > + * Returns: > + * Zero on success, negative error code on failure. > + */ > +int tinydrm_disable_backlight(struct backlight_device *backlight) > +{ > + unsigned int old_state; > + int ret; > + > + if (!backlight) > + return 0; > + > + old_state = backlight->props.state; > + backlight->props.state |= BL_CORE_FBBLANK; > + DRM_DEBUG_KMS("Backlight state: 0x%x -> 0x%x\n", old_state, > + backlight->props.state); > + ret = backlight_update_status(backlight); > + if (ret) > + DRM_ERROR("Failed to disable backlight %d\n", ret); > + > + return ret; > +} > +EXPORT_SYMBOL(tinydrm_disable_backlight); > +#endif These look like they really should be part of the backlight subsystem. I don't see anything DRM specific about them. Well, except for the error messages. > +#ifdef CONFIG_SPI > + > +/** > + * tinydrm_spi_max_transfer_size - Determine max SPI transfer size > + * @spi: SPI device > + * @max_len: Maximum buffer size needed (optional) > + * > + * This function returns the maximum size to use for SPI transfers. It checks > + * the SPI master, the optional @max_len and the module parameter spi_max and > + * returns the smallest. > + * > + * Returns: > + * Maximum size for SPI transfers > + */ > +size_t tinydrm_spi_max_transfer_size(struct spi_device *spi, size_t max_len) > +{ > + size_t ret; > + > + ret = min(spi_max_transfer_size(spi), spi->master->max_dma_len); > + if (max_len) > + ret = min(ret, max_len); > + if (spi_max) > + ret = min_t(size_t, ret, spi_max); > + ret &= ~0x3; > + if (ret < 4) > + ret = 4; > + > + return ret; > +} > +EXPORT_SYMBOL(tinydrm_spi_max_transfer_size); > + > +/** > + * tinydrm_spi_bpw_supported - Check if bits per word is supported > + * @spi: SPI device > + * @bpw: Bits per word > + * > + * This function checks to see if the SPI master driver supports @bpw. > + * > + * Returns: > + * True if @bpw is supported, false otherwise. > + */ > +bool tinydrm_spi_bpw_supported(struct spi_device *spi, u8 bpw) > +{ > + u32 bpw_mask = spi->master->bits_per_word_mask; > + > + if (bpw == 8) > + return true; > + > + if (!bpw_mask) { > + dev_warn_once(&spi->dev, > + "bits_per_word_mask not set, assume 8-bit only\n"); > + return false; > + } > + > + if (bpw_mask & SPI_BPW_MASK(bpw)) > + return true; > + > + return false; > +} > +EXPORT_SYMBOL(tinydrm_spi_bpw_supported); > + > +static void > +tinydrm_dbg_spi_print(struct spi_device *spi, struct spi_transfer *tr, > + const void *buf, int idx, bool tx) > +{ > + u32 speed_hz = tr->speed_hz ? tr->speed_hz : spi->max_speed_hz; > + char linebuf[3 * 32]; > + > + hex_dump_to_buffer(buf, tr->len, 16, > + DIV_ROUND_UP(tr->bits_per_word, 8), > + linebuf, sizeof(linebuf), false); > + > + printk(KERN_DEBUG > + " tr(%i): speed=%u%s, bpw=%i, len=%u, %s_buf=[%s%s]\n", idx, > + speed_hz > 1000000 ? speed_hz / 1000000 : speed_hz / 1000, > + speed_hz > 1000000 ? "MHz" : "kHz", tr->bits_per_word, tr->len, > + tx ? "tx" : "rx", linebuf, tr->len > 16 ? " ..." : ""); > +} > + > +/* called through tinydrm_dbg_spi_message() */ > +void _tinydrm_dbg_spi_message(struct spi_device *spi, struct spi_message *m) > +{ > + struct spi_transfer *tmp; > + struct list_head *pos; > + int i = 0; > + > + list_for_each(pos, &m->transfers) { > + tmp = list_entry(pos, struct spi_transfer, transfer_list); > + > + if (tmp->tx_buf) > + tinydrm_dbg_spi_print(spi, tmp, tmp->tx_buf, i, true); > + if (tmp->rx_buf) > + tinydrm_dbg_spi_print(spi, tmp, tmp->rx_buf, i, false); > + i++; > + } > +} > +EXPORT_SYMBOL(_tinydrm_dbg_spi_message); > + > +/** > + * tinydrm_spi_transfer - SPI transfer helper > + * @spi: SPI device > + * @speed_hz: Override speed (optional) > + * @header: Optional header transfer > + * @bpw: Bits per word > + * @buf: Buffer to transfer > + * @len: Buffer length > + * > + * This SPI transfer helper breaks up the transfer of @buf into chunks which > + * the SPI master driver can handle. If the machine is Little Endian and the > + * SPI master driver doesn't support 16 bits per word, it swaps the bytes and > + * does a 8-bit transfer. > + * If @header is set, it is prepended to each SPI message. > + * > + * Returns: > + * Zero on success, negative error code on failure. > + */ > +int tinydrm_spi_transfer(struct spi_device *spi, u32 speed_hz, > + struct spi_transfer *header, u8 bpw, const void *buf, > + size_t len) > +{ > + struct spi_transfer tr = { > + .bits_per_word = bpw, > + .speed_hz = speed_hz, > + }; > + struct spi_message m; > + u16 *swap_buf = NULL; > + size_t max_chunk; > + size_t chunk; > + int ret = 0; > + > + if (WARN_ON_ONCE(bpw != 8 && bpw != 16)) > + return -EINVAL; > + > + max_chunk = tinydrm_spi_max_transfer_size(spi, 0); > + > + if (drm_debug & DRM_UT_DRIVER) > + pr_debug("[drm:%s] bpw=%u, max_chunk=%zu, transfers:\n", > + __func__, bpw, max_chunk); > + > + if (bpw == 16 && !tinydrm_spi_bpw_supported(spi, 16)) { > + tr.bits_per_word = 8; > + if (tinydrm_machine_little_endian()) { > + swap_buf = kmalloc(min(len, max_chunk), GFP_KERNEL); > + if (!swap_buf) > + return -ENOMEM; > + } > + } > + > + spi_message_init(&m); > + if (header) > + spi_message_add_tail(header, &m); > + spi_message_add_tail(&tr, &m); > + > + while (len) { > + chunk = min(len, max_chunk); > + > + tr.tx_buf = buf; > + tr.len = chunk; > + > + if (swap_buf) { > + const u16 *buf16 = buf; > + unsigned int i; > + > + for (i = 0; i < chunk / 2; i++) > + swap_buf[i] = swab16(buf16[i]); > + > + tr.tx_buf = swap_buf; > + } > + > + buf += chunk; > + len -= chunk; > + > + tinydrm_dbg_spi_message(spi, &m); > + ret = spi_sync(spi, &m); > + if (ret) > + return ret; > + }; > + > + return 0; > +} > +EXPORT_SYMBOL(tinydrm_spi_transfer); Similarly for the above. Thierry
On Mon, Feb 06, 2017 at 09:56:29AM +0100, Thierry Reding wrote: > On Tue, Jan 31, 2017 at 05:03:14PM +0100, Noralf Trønnes wrote: > > Add common functionality needed by many tinydrm drivers. > > > > Signed-off-by: Noralf Trønnes <noralf@tronnes.org> > > Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > --- > > Documentation/gpu/tinydrm.rst | 9 + > > drivers/gpu/drm/tinydrm/core/Makefile | 2 +- > > drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c | 462 +++++++++++++++++++++++++ > > include/drm/tinydrm/tinydrm-helpers.h | 100 ++++++ > > 4 files changed, 572 insertions(+), 1 deletion(-) > > create mode 100644 drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c > > create mode 100644 include/drm/tinydrm/tinydrm-helpers.h > > > > diff --git a/Documentation/gpu/tinydrm.rst b/Documentation/gpu/tinydrm.rst > > index ec4a20d..fb256d2 100644 > > --- a/Documentation/gpu/tinydrm.rst > > +++ b/Documentation/gpu/tinydrm.rst > > @@ -19,3 +19,12 @@ Core functionality > > > > .. kernel-doc:: drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c > > :export: > > + > > +Additional helpers > > +================== > > + > > +.. kernel-doc:: include/drm/tinydrm/tinydrm-helpers.h > > + :internal: > > + > > +.. kernel-doc:: drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c > > + :export: > > diff --git a/drivers/gpu/drm/tinydrm/core/Makefile b/drivers/gpu/drm/tinydrm/core/Makefile > > index 4f14a0f..fb221e6 100644 > > --- a/drivers/gpu/drm/tinydrm/core/Makefile > > +++ b/drivers/gpu/drm/tinydrm/core/Makefile > > @@ -1,3 +1,3 @@ > > -tinydrm-y := tinydrm-core.o tinydrm-pipe.o > > +tinydrm-y := tinydrm-core.o tinydrm-pipe.o tinydrm-helpers.o > > > > obj-$(CONFIG_DRM_TINYDRM) += tinydrm.o > > diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c > > new file mode 100644 > > index 0000000..75e4e54 > > --- /dev/null > > +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c > > @@ -0,0 +1,462 @@ > > +/* > > + * Copyright (C) 2016 Noralf Trønnes > > + * > > + * This program is free software; you can redistribute it and/or modify > > + * it under the terms of the GNU General Public License as published by > > + * the Free Software Foundation; either version 2 of the License, or > > + * (at your option) any later version. > > + */ > > + > > +#include <drm/tinydrm/tinydrm.h> > > +#include <drm/tinydrm/tinydrm-helpers.h> > > +#include <linux/backlight.h> > > +#include <linux/pm.h> > > +#include <linux/spi/spi.h> > > +#include <linux/swab.h> > > + > > +static unsigned int spi_max; > > +module_param(spi_max, uint, 0400); > > +MODULE_PARM_DESC(spi_max, "Set a lower SPI max transfer size"); > > + > > +/** > > + * tinydrm_merge_clips - Merge clip rectangles > > + * @dst: Destination clip rectangle > > + * @src: Source clip rectangle(s) > > + * @num_clips: Number of @src clip rectangles > > + * @flags: Dirty fb ioctl flags > > + * @max_width: Maximum width of @dst > > + * @max_height: Maximum height of @dst > > + * > > + * This function merges @src clip rectangle(s) into @dst. If @src is NULL, > > + * @max_width and @min_width is used to set a full @dst clip rectangle. > > + * > > + * Returns: > > + * true if it's a full clip, false otherwise > > + */ > > +bool tinydrm_merge_clips(struct drm_clip_rect *dst, > > + struct drm_clip_rect *src, unsigned int num_clips, > > + unsigned int flags, u32 max_width, u32 max_height) > > +{ > > + unsigned int i; > > + > > + if (!src || !num_clips) { > > + dst->x1 = 0; > > + dst->x2 = max_width; > > + dst->y1 = 0; > > + dst->y2 = max_height; > > + return true; > > + } > > + > > + dst->x1 = ~0; > > + dst->y1 = ~0; > > + dst->x2 = 0; > > + dst->y2 = 0; > > + > > + for (i = 0; i < num_clips; i++) { > > + if (flags & DRM_MODE_FB_DIRTY_ANNOTATE_COPY) > > + i++; > > + dst->x1 = min(dst->x1, src[i].x1); > > + dst->x2 = max(dst->x2, src[i].x2); > > + dst->y1 = min(dst->y1, src[i].y1); > > + dst->y2 = max(dst->y2, src[i].y2); > > + } > > + > > + if (dst->x2 > max_width || dst->y2 > max_height || > > + dst->x1 >= dst->x2 || dst->y1 >= dst->y2) { > > + DRM_DEBUG_KMS("Illegal clip: x1=%u, x2=%u, y1=%u, y2=%u\n", > > + dst->x1, dst->x2, dst->y1, dst->y2); > > + dst->x1 = 0; > > + dst->y1 = 0; > > + dst->x2 = max_width; > > + dst->y2 = max_height; > > + } > > + > > + return (dst->x2 - dst->x1) == max_width && > > + (dst->y2 - dst->y1) == max_height; > > +} > > +EXPORT_SYMBOL(tinydrm_merge_clips); > > Should this perhaps be part of drm_rect.c? drm_rect is about struct drm_rect, this here is struct drm_clip_rect. I think fixing this up is a lot bigger patch series, and we've postponed it already with the dirtyfb trakcing patches for the fbdev helpers. I think postponing once more is ok. > > +#ifdef CONFIG_BACKLIGHT_CLASS_DEVICE > > +/** > > + * tinydrm_of_find_backlight - Find backlight device in device-tree > > + * @dev: Device > > + * > > + * This function looks for a DT node pointed to by a property named 'backlight' > > + * and uses of_find_backlight_by_node() to get the backlight device. > > + * Additionally if the brightness property is zero, it is set to > > + * max_brightness. > > + * > > + * Returns: > > + * NULL if there's no backlight property. > > + * Error pointer -EPROBE_DEFER if the DT node is found, but no backlight device > > + * is found. > > + * If the backlight device is found, a pointer to the structure is returned. > > + */ > > +struct backlight_device *tinydrm_of_find_backlight(struct device *dev) > > +{ > > + struct backlight_device *backlight; > > + struct device_node *np; > > + > > + np = of_parse_phandle(dev->of_node, "backlight", 0); > > + if (!np) > > + return NULL; > > + > > + backlight = of_find_backlight_by_node(np); > > + of_node_put(np); > > + > > + if (!backlight) > > + return ERR_PTR(-EPROBE_DEFER); > > + > > + if (!backlight->props.brightness) { > > + backlight->props.brightness = backlight->props.max_brightness; > > + DRM_DEBUG_KMS("Backlight brightness set to %d\n", > > + backlight->props.brightness); > > + } > > + > > + return backlight; > > +} > > +EXPORT_SYMBOL(tinydrm_of_find_backlight); > > + > > +/** > > + * tinydrm_enable_backlight - Enable backlight helper > > + * @backlight: Backlight device > > + * > > + * Returns: > > + * Zero on success, negative error code on failure. > > + */ > > +int tinydrm_enable_backlight(struct backlight_device *backlight) > > +{ > > + unsigned int old_state; > > + int ret; > > + > > + if (!backlight) > > + return 0; > > + > > + old_state = backlight->props.state; > > + backlight->props.state &= ~BL_CORE_FBBLANK; > > + DRM_DEBUG_KMS("Backlight state: 0x%x -> 0x%x\n", old_state, > > + backlight->props.state); > > + > > + ret = backlight_update_status(backlight); > > + if (ret) > > + DRM_ERROR("Failed to enable backlight %d\n", ret); > > + > > + return ret; > > +} > > +EXPORT_SYMBOL(tinydrm_enable_backlight); > > + > > +/** > > + * tinydrm_disable_backlight - Disable backlight helper > > + * @backlight: Backlight device > > + * > > + * Returns: > > + * Zero on success, negative error code on failure. > > + */ > > +int tinydrm_disable_backlight(struct backlight_device *backlight) > > +{ > > + unsigned int old_state; > > + int ret; > > + > > + if (!backlight) > > + return 0; > > + > > + old_state = backlight->props.state; > > + backlight->props.state |= BL_CORE_FBBLANK; > > + DRM_DEBUG_KMS("Backlight state: 0x%x -> 0x%x\n", old_state, > > + backlight->props.state); > > + ret = backlight_update_status(backlight); > > + if (ret) > > + DRM_ERROR("Failed to disable backlight %d\n", ret); > > + > > + return ret; > > +} > > +EXPORT_SYMBOL(tinydrm_disable_backlight); > > +#endif > > These look like they really should be part of the backlight subsystem. I > don't see anything DRM specific about them. Well, except for the error > messages. So this is a bit an unpopular opinion with some folks, but I don't require anyone to submit new code to subsystems outside of drm for new drivers. Simply because it takes months to get stuff landed, and in general it's not worth the trouble. We have piles of stuff in drm and drm drivers that should be in core but isn't. Imo the only reasonable way is to merge as-is, then follow-up with a patch series to move the helper into the right subsystem. Most often unfortunately that follow-up patch series will just die. -Daniel
On Mon, Feb 06, 2017 at 10:09:18AM +0100, Daniel Vetter wrote: > On Mon, Feb 06, 2017 at 09:56:29AM +0100, Thierry Reding wrote: > > On Tue, Jan 31, 2017 at 05:03:14PM +0100, Noralf Trønnes wrote: [...] > > > +/** > > > + * tinydrm_merge_clips - Merge clip rectangles > > > + * @dst: Destination clip rectangle > > > + * @src: Source clip rectangle(s) > > > + * @num_clips: Number of @src clip rectangles > > > + * @flags: Dirty fb ioctl flags > > > + * @max_width: Maximum width of @dst > > > + * @max_height: Maximum height of @dst > > > + * > > > + * This function merges @src clip rectangle(s) into @dst. If @src is NULL, > > > + * @max_width and @min_width is used to set a full @dst clip rectangle. > > > + * > > > + * Returns: > > > + * true if it's a full clip, false otherwise > > > + */ > > > +bool tinydrm_merge_clips(struct drm_clip_rect *dst, > > > + struct drm_clip_rect *src, unsigned int num_clips, > > > + unsigned int flags, u32 max_width, u32 max_height) > > > +{ > > > + unsigned int i; > > > + > > > + if (!src || !num_clips) { > > > + dst->x1 = 0; > > > + dst->x2 = max_width; > > > + dst->y1 = 0; > > > + dst->y2 = max_height; > > > + return true; > > > + } > > > + > > > + dst->x1 = ~0; > > > + dst->y1 = ~0; > > > + dst->x2 = 0; > > > + dst->y2 = 0; > > > + > > > + for (i = 0; i < num_clips; i++) { > > > + if (flags & DRM_MODE_FB_DIRTY_ANNOTATE_COPY) > > > + i++; > > > + dst->x1 = min(dst->x1, src[i].x1); > > > + dst->x2 = max(dst->x2, src[i].x2); > > > + dst->y1 = min(dst->y1, src[i].y1); > > > + dst->y2 = max(dst->y2, src[i].y2); > > > + } > > > + > > > + if (dst->x2 > max_width || dst->y2 > max_height || > > > + dst->x1 >= dst->x2 || dst->y1 >= dst->y2) { > > > + DRM_DEBUG_KMS("Illegal clip: x1=%u, x2=%u, y1=%u, y2=%u\n", > > > + dst->x1, dst->x2, dst->y1, dst->y2); > > > + dst->x1 = 0; > > > + dst->y1 = 0; > > > + dst->x2 = max_width; > > > + dst->y2 = max_height; > > > + } > > > + > > > + return (dst->x2 - dst->x1) == max_width && > > > + (dst->y2 - dst->y1) == max_height; > > > +} > > > +EXPORT_SYMBOL(tinydrm_merge_clips); > > > > Should this perhaps be part of drm_rect.c? > > drm_rect is about struct drm_rect, this here is struct drm_clip_rect. I > think fixing this up is a lot bigger patch series, and we've postponed it > already with the dirtyfb trakcing patches for the fbdev helpers. I think > postponing once more is ok. Okay, that's fine with me. > > > +#ifdef CONFIG_BACKLIGHT_CLASS_DEVICE > > > +/** > > > + * tinydrm_of_find_backlight - Find backlight device in device-tree > > > + * @dev: Device > > > + * > > > + * This function looks for a DT node pointed to by a property named 'backlight' > > > + * and uses of_find_backlight_by_node() to get the backlight device. > > > + * Additionally if the brightness property is zero, it is set to > > > + * max_brightness. > > > + * > > > + * Returns: > > > + * NULL if there's no backlight property. > > > + * Error pointer -EPROBE_DEFER if the DT node is found, but no backlight device > > > + * is found. > > > + * If the backlight device is found, a pointer to the structure is returned. > > > + */ > > > +struct backlight_device *tinydrm_of_find_backlight(struct device *dev) > > > +{ [...] > > > +} > > > +EXPORT_SYMBOL(tinydrm_of_find_backlight); > > > + > > > +/** > > > + * tinydrm_enable_backlight - Enable backlight helper > > > + * @backlight: Backlight device > > > + * > > > + * Returns: > > > + * Zero on success, negative error code on failure. > > > + */ > > > +int tinydrm_enable_backlight(struct backlight_device *backlight) > > > +{ [...] > > > +} > > > +EXPORT_SYMBOL(tinydrm_enable_backlight); > > > + > > > +/** > > > + * tinydrm_disable_backlight - Disable backlight helper > > > + * @backlight: Backlight device > > > + * > > > + * Returns: > > > + * Zero on success, negative error code on failure. > > > + */ > > > +int tinydrm_disable_backlight(struct backlight_device *backlight) > > > +{ [...] > > > +} > > > +EXPORT_SYMBOL(tinydrm_disable_backlight); > > > +#endif > > > > These look like they really should be part of the backlight subsystem. I > > don't see anything DRM specific about them. Well, except for the error > > messages. > > So this is a bit an unpopular opinion with some folks, but I don't require > anyone to submit new code to subsystems outside of drm for new drivers. > Simply because it takes months to get stuff landed, and in general it's > not worth the trouble. "Not worth the trouble" is very subjective. If you look at the Linux kernel in general, one of the reasons why it works so well is because the changes we make apply to the kernel as a whole. Yes, sometimes that makes things more difficult and time-consuming, but it also means that the end result will be much more widely usable and therefore benefits everyone else in return. In my opinion that's a large part of why the kernel is so successful. > We have piles of stuff in drm and drm drivers that should be in core but > isn't. > > Imo the only reasonable way is to merge as-is, then follow-up with a patch > series to move the helper into the right subsystem. Most often > unfortunately that follow-up patch series will just die. Of course follow-up series die. That's because nobody cares to follow-up once their code has been merged. Collecting our own helpers or variants of subsystems is a great way of isolating ourselves from the rest of the community. I don't think that's a good solution in the long run at all. Thierry
On Mon, Feb 6, 2017 at 10:35 AM, Thierry Reding <thierry.reding@gmail.com> wrote: > > > > +EXPORT_SYMBOL(tinydrm_disable_backlight); > > > > +#endif > > > > > > These look like they really should be part of the backlight subsystem. > I > > > don't see anything DRM specific about them. Well, except for the error > > > messages. > > > > So this is a bit an unpopular opinion with some folks, but I don't > require > > anyone to submit new code to subsystems outside of drm for new drivers. > > Simply because it takes months to get stuff landed, and in general it's > > not worth the trouble. > > "Not worth the trouble" is very subjective. If you look at the Linux > kernel in general, one of the reasons why it works so well is because > the changes we make apply to the kernel as a whole. Yes, sometimes that > makes things more difficult and time-consuming, but it also means that > the end result will be much more widely usable and therefore benefits > everyone else in return. In my opinion that's a large part of why the > kernel is so successful. > > > We have piles of stuff in drm and drm drivers that should be in core but > > isn't. > > > > Imo the only reasonable way is to merge as-is, then follow-up with a > patch > > series to move the helper into the right subsystem. Most often > > unfortunately that follow-up patch series will just die. > > Of course follow-up series die. That's because nobody cares to follow-up > once their code has been merged. > > Collecting our own helpers or variants of subsystems is a great way of > isolating ourselves from the rest of the community. I don't think that's > a good solution in the long run at all. > We have a bunch of patch series that we resubmit for months and they go exactly nowhere. They don't die because we stop caring, they die because they die. Some of them we even need to constantly rebase and carry around in drm-tip since our CI would Oops or spew WARNIGs all over the place. There's simply some areas of the kernel which seem overloaded under patches and no one is willing or able to fix things, and I can't fix the entire kernel. Nor expect contributors (who have much less political weight to throw around than me) to do that and succeed. And we don't end up with worse code in the drm subsystem, since we can still do the refactoring within drm helpers and end up with clean drivers. I fully agree that it's not great for the kernel's future, but when I'm stuck with the option to get shit done or burning out playing the upstreaming game, the choice is easy. And in the end I care about open source gfx much more than the kernel, and I think for open source gfx's success it's crucial that we're welcoming to new contributors and don't throw up massive roadblocks. Open source gfx is tiny and still far away from world domination, we need _lots_ more people. If that means routing around other subsystems for them, I'm all for it. -Daniel
On Tue, 31 Jan 2017, Noralf Trønnes <noralf@tronnes.org> wrote:
> +#ifdef CONFIG_BACKLIGHT_CLASS_DEVICE
BACKLIGHT_CLASS_DEVICE is a tristate, you'll want
#if IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)
and probably either
depends on BACKLIGHT_CLASS_DEVICE
or
depends on BACKLIGHT_CLASS_DEVICE || BACKLIGHT_CLASS_DEVICE=n
in your Kconfig.
BR,
Jani.
On Mon, Feb 06, 2017 at 11:07:42AM +0100, Daniel Vetter wrote: > On Mon, Feb 6, 2017 at 10:35 AM, Thierry Reding <thierry.reding@gmail.com> > wrote: > > > > > > +EXPORT_SYMBOL(tinydrm_disable_backlight); > > > > > +#endif > > > > > > > > These look like they really should be part of the backlight subsystem. > > I > > > > don't see anything DRM specific about them. Well, except for the error > > > > messages. > > > > > > So this is a bit an unpopular opinion with some folks, but I don't > > require > > > anyone to submit new code to subsystems outside of drm for new drivers. > > > Simply because it takes months to get stuff landed, and in general it's > > > not worth the trouble. > > > > "Not worth the trouble" is very subjective. If you look at the Linux > > kernel in general, one of the reasons why it works so well is because > > the changes we make apply to the kernel as a whole. Yes, sometimes that > > makes things more difficult and time-consuming, but it also means that > > the end result will be much more widely usable and therefore benefits > > everyone else in return. In my opinion that's a large part of why the > > kernel is so successful. > > > > > We have piles of stuff in drm and drm drivers that should be in core but > > > isn't. > > > > > > Imo the only reasonable way is to merge as-is, then follow-up with a > > patch > > > series to move the helper into the right subsystem. Most often > > > unfortunately that follow-up patch series will just die. > > > > Of course follow-up series die. That's because nobody cares to follow-up > > once their code has been merged. > > > > Collecting our own helpers or variants of subsystems is a great way of > > isolating ourselves from the rest of the community. I don't think that's > > a good solution in the long run at all. > > > > We have a bunch of patch series that we resubmit for months and they go > exactly nowhere. They don't die because we stop caring, they die because > they die. Some of them we even need to constantly rebase and carry around > in drm-tip since our CI would Oops or spew WARNIGs all over the place. > There's simply some areas of the kernel which seem overloaded under patches > and no one is willing or able to fix things, and I can't fix the entire > kernel. Nor expect contributors (who have much less political weight to > throw around than me) to do that and succeed. And we don't end up with > worse code in the drm subsystem, since we can still do the refactoring > within drm helpers and end up with clean drivers. > > I fully agree that it's not great for the kernel's future, but when I'm > stuck with the option to get shit done or burning out playing the > upstreaming game, the choice is easy. And in the end I care about open > source gfx much more than the kernel, and I think for open source gfx's > success it's crucial that we're welcoming to new contributors and don't > throw up massive roadblocks. Open source gfx is tiny and still far away > from world domination, we need _lots_ more people. If that means routing > around other subsystems for them, I'm all for it. I can't say I fully agree with that sentiment. I do see how routing around subsystems can be useful occasionally. If nobody will merge the code, or if nobody cares, then by all means, let's make them DRM- specific helpers. But I think we need to at least try to do the right thing. If only to teach people what the right way is. If we start accepting such things by default, how can we expect contributors to even try? I also think that contributors will often end up contributing not only to DRM but to the kernel as a whole. As such it should be part of our mentoring to teach them about how the process works as a rule, even if the occasional exception is necessary to get things done. In this particular case, I know for a fact that both backlight and SPI maintainers are very responsive, so that's not a good excuse. Thierry
On Mon, Feb 06, 2017 at 12:08:47PM +0100, Thierry Reding wrote: > On Mon, Feb 06, 2017 at 11:07:42AM +0100, Daniel Vetter wrote: > > On Mon, Feb 6, 2017 at 10:35 AM, Thierry Reding <thierry.reding@gmail.com> > > wrote: > > > > > > > > +EXPORT_SYMBOL(tinydrm_disable_backlight); > > > > > > +#endif > > > > > > > > > > These look like they really should be part of the backlight subsystem. > > > I > > > > > don't see anything DRM specific about them. Well, except for the error > > > > > messages. > > > > > > > > So this is a bit an unpopular opinion with some folks, but I don't > > > require > > > > anyone to submit new code to subsystems outside of drm for new drivers. > > > > Simply because it takes months to get stuff landed, and in general it's > > > > not worth the trouble. > > > > > > "Not worth the trouble" is very subjective. If you look at the Linux > > > kernel in general, one of the reasons why it works so well is because > > > the changes we make apply to the kernel as a whole. Yes, sometimes that > > > makes things more difficult and time-consuming, but it also means that > > > the end result will be much more widely usable and therefore benefits > > > everyone else in return. In my opinion that's a large part of why the > > > kernel is so successful. > > > > > > > We have piles of stuff in drm and drm drivers that should be in core but > > > > isn't. > > > > > > > > Imo the only reasonable way is to merge as-is, then follow-up with a > > > patch > > > > series to move the helper into the right subsystem. Most often > > > > unfortunately that follow-up patch series will just die. > > > > > > Of course follow-up series die. That's because nobody cares to follow-up > > > once their code has been merged. > > > > > > Collecting our own helpers or variants of subsystems is a great way of > > > isolating ourselves from the rest of the community. I don't think that's > > > a good solution in the long run at all. > > > > > > > We have a bunch of patch series that we resubmit for months and they go > > exactly nowhere. They don't die because we stop caring, they die because > > they die. Some of them we even need to constantly rebase and carry around > > in drm-tip since our CI would Oops or spew WARNIGs all over the place. > > There's simply some areas of the kernel which seem overloaded under patches > > and no one is willing or able to fix things, and I can't fix the entire > > kernel. Nor expect contributors (who have much less political weight to > > throw around than me) to do that and succeed. And we don't end up with > > worse code in the drm subsystem, since we can still do the refactoring > > within drm helpers and end up with clean drivers. > > > > I fully agree that it's not great for the kernel's future, but when I'm > > stuck with the option to get shit done or burning out playing the > > upstreaming game, the choice is easy. And in the end I care about open > > source gfx much more than the kernel, and I think for open source gfx's > > success it's crucial that we're welcoming to new contributors and don't > > throw up massive roadblocks. Open source gfx is tiny and still far away > > from world domination, we need _lots_ more people. If that means routing > > around other subsystems for them, I'm all for it. > > I can't say I fully agree with that sentiment. I do see how routing > around subsystems can be useful occasionally. If nobody will merge the > code, or if nobody cares, then by all means, let's make them DRM- > specific helpers. > > But I think we need to at least try to do the right thing. If only to > teach people what the right way is. If we start accepting such things > by default, how can we expect contributors to even try? > > I also think that contributors will often end up contributing not only > to DRM but to the kernel as a whole. As such it should be part of our > mentoring to teach them about how the process works as a rule, even if > the occasional exception is necessary to get things done. > > In this particular case, I know for a fact that both backlight and SPI > maintainers are very responsive, so that's not a good excuse. I definitely don't want that we don't attempt this. But brought from years of experience, I recommend to merge first (with pre-refactoring already applied, but helpers only extracted, not yet at the right spot), and then follow up with. Because on average, there's way too many trees with overloaded maintainers who maybe look at your patch once per kernel release cycle. If you know that backlight and spi isn't one of these areas (anything that goes through takashi/sound is a similar good experience for us on the i915 side), then I guess we can try. But then Noralf has already written a few months worth of really great refactoring, and I'm seriously starting to feel guilty for volunteering him for all of this. Even though he seems to be really good at it, and seems to not mind, it's getting a bit silly. Given that I'd say up to Noralf. In short, there's always a balance. -Daniel
Den 06.02.2017 16.53, skrev Daniel Vetter: > On Mon, Feb 06, 2017 at 12:08:47PM +0100, Thierry Reding wrote: >> On Mon, Feb 06, 2017 at 11:07:42AM +0100, Daniel Vetter wrote: >>> On Mon, Feb 6, 2017 at 10:35 AM, Thierry Reding <thierry.reding@gmail.com> >>> wrote: >>> >>>>>>> +EXPORT_SYMBOL(tinydrm_disable_backlight); >>>>>>> +#endif >>>>>> These look like they really should be part of the backlight subsystem. >>>> I >>>>>> don't see anything DRM specific about them. Well, except for the error >>>>>> messages. >>>>> So this is a bit an unpopular opinion with some folks, but I don't >>>> require >>>>> anyone to submit new code to subsystems outside of drm for new drivers. >>>>> Simply because it takes months to get stuff landed, and in general it's >>>>> not worth the trouble. >>>> "Not worth the trouble" is very subjective. If you look at the Linux >>>> kernel in general, one of the reasons why it works so well is because >>>> the changes we make apply to the kernel as a whole. Yes, sometimes that >>>> makes things more difficult and time-consuming, but it also means that >>>> the end result will be much more widely usable and therefore benefits >>>> everyone else in return. In my opinion that's a large part of why the >>>> kernel is so successful. >>>> >>>>> We have piles of stuff in drm and drm drivers that should be in core but >>>>> isn't. >>>>> >>>>> Imo the only reasonable way is to merge as-is, then follow-up with a >>>> patch >>>>> series to move the helper into the right subsystem. Most often >>>>> unfortunately that follow-up patch series will just die. >>>> Of course follow-up series die. That's because nobody cares to follow-up >>>> once their code has been merged. >>>> >>>> Collecting our own helpers or variants of subsystems is a great way of >>>> isolating ourselves from the rest of the community. I don't think that's >>>> a good solution in the long run at all. >>>> >>> We have a bunch of patch series that we resubmit for months and they go >>> exactly nowhere. They don't die because we stop caring, they die because >>> they die. Some of them we even need to constantly rebase and carry around >>> in drm-tip since our CI would Oops or spew WARNIGs all over the place. >>> There's simply some areas of the kernel which seem overloaded under patches >>> and no one is willing or able to fix things, and I can't fix the entire >>> kernel. Nor expect contributors (who have much less political weight to >>> throw around than me) to do that and succeed. And we don't end up with >>> worse code in the drm subsystem, since we can still do the refactoring >>> within drm helpers and end up with clean drivers. >>> >>> I fully agree that it's not great for the kernel's future, but when I'm >>> stuck with the option to get shit done or burning out playing the >>> upstreaming game, the choice is easy. And in the end I care about open >>> source gfx much more than the kernel, and I think for open source gfx's >>> success it's crucial that we're welcoming to new contributors and don't >>> throw up massive roadblocks. Open source gfx is tiny and still far away >>> from world domination, we need _lots_ more people. If that means routing >>> around other subsystems for them, I'm all for it. >> I can't say I fully agree with that sentiment. I do see how routing >> around subsystems can be useful occasionally. If nobody will merge the >> code, or if nobody cares, then by all means, let's make them DRM- >> specific helpers. >> >> But I think we need to at least try to do the right thing. If only to >> teach people what the right way is. If we start accepting such things >> by default, how can we expect contributors to even try? >> >> I also think that contributors will often end up contributing not only >> to DRM but to the kernel as a whole. As such it should be part of our >> mentoring to teach them about how the process works as a rule, even if >> the occasional exception is necessary to get things done. >> >> In this particular case, I know for a fact that both backlight and SPI >> maintainers are very responsive, so that's not a good excuse. > I definitely don't want that we don't attempt this. But brought from years > of experience, I recommend to merge first (with pre-refactoring already > applied, but helpers only extracted, not yet at the right spot), and then > follow up with. Because on average, there's way too many trees with > overloaded maintainers who maybe look at your patch once per kernel > release cycle. > > If you know that backlight and spi isn't one of these areas (anything that > goes through takashi/sound is a similar good experience for us on the i915 > side), then I guess we can try. But then Noralf has already written a few > months worth of really great refactoring, and I'm seriously starting to > feel guilty for volunteering him for all of this. Even though he seems to > be really good at it, and seems to not mind, it's getting a bit silly. > Given that I'd say up to Noralf. > > In short, there's always a balance. Yes, it's a balance between the perfect and not's so perfect, the professinal and the amateur, and the drm expert and the newbie. If I knew how much time I would have to spend on tinydrm to get it merged, then I would never have started it. I wondered, a couple of months back, if I should just cut my losses and move to something else. dri-devel is a friendly place and I do get help to keep moving, but I get the feeling that drm is really a place for professionals that write kernel code 50 hours a week. And there's nothing wrong in that, drm is complex and maybe that kind of expertice and work-hours are needed to do work here. It's not that I plan on backing out now, I'm just putting down a few words about the challenge it has been for me as a hobbyist to meet drm. As for the specifics, Backlight enable/disable helpers, I haven't thought about those as backlight subsystem helpers. But I see some drm panel drivers that do the same, so it makes sense. But what I'm feeling is this: If I'm expected to get everything right, then I will "never" get this merged. This thing by itself isn't much, but adding up every little thing amounts to a lot. And it's not just "make this patch", it's probably clean up the drivers as well :-) tinydrm_spi_transfer() can print the content of the buffer. This is important at least until I have seen some real use of the drivers. I have emulation code for handling bit widths not supported by the SPI controller, so I want to see what goes over the wire. SPI core uses trace events to track what's going on, and I don't think I can get buffer dumping into that code. Don't get me wrong Thierry, I do appreciate that you take the time to look at the code. I'm just frustrated that it takes so long to get this right. I thought that all I needed now was a DT maintainer ack :-) Noralf.
> > I definitely don't want that we don't attempt this. But brought from years > of experience, I recommend to merge first (with pre-refactoring already > applied, but helpers only extracted, not yet at the right spot), and then > follow up with. Because on average, there's way too many trees with > overloaded maintainers who maybe look at your patch once per kernel > release cycle. > > If you know that backlight and spi isn't one of these areas (anything that > goes through takashi/sound is a similar good experience for us on the i915 > side), then I guess we can try. But then Noralf has already written a few > months worth of really great refactoring, and I'm seriously starting to > feel guilty for volunteering him for all of this. Even though he seems to > be really good at it, and seems to not mind, it's getting a bit silly. > Given that I'd say up to Noralf. > > In short, there's always a balance. I don't think we can make a rule for this, it will always depend on the code. There is always going to be stuff we put in drm that should go elsewhere, and stuff that is elsewhere that drm should use. I think however if we do add stuff like this, someone should keep track of them and try to make them get further into the kernel. In this case I don't think the patches are too insane to keep in drm and refactor up later, in other cases I'm sure it'll be lot more obvious (i.e. we could make the same argument for chunks of DAL :-) Dave.
On Mon, Feb 6, 2017 at 5:08 AM, Thierry Reding <thierry.reding@gmail.com> wrote: > On Mon, Feb 06, 2017 at 11:07:42AM +0100, Daniel Vetter wrote: >> On Mon, Feb 6, 2017 at 10:35 AM, Thierry Reding <thierry.reding@gmail.com> >> wrote: >> >> > > > > +EXPORT_SYMBOL(tinydrm_disable_backlight); >> > > > > +#endif >> > > > >> > > > These look like they really should be part of the backlight subsystem. >> > I >> > > > don't see anything DRM specific about them. Well, except for the error >> > > > messages. >> > > >> > > So this is a bit an unpopular opinion with some folks, but I don't >> > require >> > > anyone to submit new code to subsystems outside of drm for new drivers. >> > > Simply because it takes months to get stuff landed, and in general it's >> > > not worth the trouble. >> > >> > "Not worth the trouble" is very subjective. If you look at the Linux >> > kernel in general, one of the reasons why it works so well is because >> > the changes we make apply to the kernel as a whole. Yes, sometimes that >> > makes things more difficult and time-consuming, but it also means that >> > the end result will be much more widely usable and therefore benefits >> > everyone else in return. In my opinion that's a large part of why the >> > kernel is so successful. >> > >> > > We have piles of stuff in drm and drm drivers that should be in core but >> > > isn't. >> > > >> > > Imo the only reasonable way is to merge as-is, then follow-up with a >> > patch >> > > series to move the helper into the right subsystem. Most often >> > > unfortunately that follow-up patch series will just die. >> > >> > Of course follow-up series die. That's because nobody cares to follow-up >> > once their code has been merged. >> > >> > Collecting our own helpers or variants of subsystems is a great way of >> > isolating ourselves from the rest of the community. I don't think that's >> > a good solution in the long run at all. >> > >> >> We have a bunch of patch series that we resubmit for months and they go >> exactly nowhere. They don't die because we stop caring, they die because >> they die. Some of them we even need to constantly rebase and carry around >> in drm-tip since our CI would Oops or spew WARNIGs all over the place. >> There's simply some areas of the kernel which seem overloaded under patches >> and no one is willing or able to fix things, and I can't fix the entire >> kernel. Nor expect contributors (who have much less political weight to >> throw around than me) to do that and succeed. And we don't end up with >> worse code in the drm subsystem, since we can still do the refactoring >> within drm helpers and end up with clean drivers. >> >> I fully agree that it's not great for the kernel's future, but when I'm >> stuck with the option to get shit done or burning out playing the >> upstreaming game, the choice is easy. And in the end I care about open >> source gfx much more than the kernel, and I think for open source gfx's >> success it's crucial that we're welcoming to new contributors and don't >> throw up massive roadblocks. Open source gfx is tiny and still far away >> from world domination, we need _lots_ more people. If that means routing >> around other subsystems for them, I'm all for it. > > I can't say I fully agree with that sentiment. I do see how routing > around subsystems can be useful occasionally. If nobody will merge the > code, or if nobody cares, then by all means, let's make them DRM- > specific helpers. In this case, these backlight helpers aren't even common across DRM. They are tinydrm specific, but only in name and location. They look like they'd be helpful to panel-simple.c and other panel drivers, too. :) Who's to blame for duplication within DRM then? If only I had 1 location of OF graph code to clean-up... I get new DT functions all the time that other subsystems want, so I don't think the problem lies there. > But I think we need to at least try to do the right thing. If only to > teach people what the right way is. If we start accepting such things > by default, how can we expect contributors to even try? > > I also think that contributors will often end up contributing not only > to DRM but to the kernel as a whole. As such it should be part of our > mentoring to teach them about how the process works as a rule, even if > the occasional exception is necessary to get things done. Yes, it's important for contributors to learn to avoid "the platform problem"[1]. Rob [1] https://lwn.net/Articles/443531/
On Tue, Feb 07, 2017 at 08:28:16AM +1000, Dave Airlie wrote: > > > > I definitely don't want that we don't attempt this. But brought from years > > of experience, I recommend to merge first (with pre-refactoring already > > applied, but helpers only extracted, not yet at the right spot), and then > > follow up with. Because on average, there's way too many trees with > > overloaded maintainers who maybe look at your patch once per kernel > > release cycle. > > > > If you know that backlight and spi isn't one of these areas (anything that > > goes through takashi/sound is a similar good experience for us on the i915 > > side), then I guess we can try. But then Noralf has already written a few > > months worth of really great refactoring, and I'm seriously starting to > > feel guilty for volunteering him for all of this. Even though he seems to > > be really good at it, and seems to not mind, it's getting a bit silly. > > Given that I'd say up to Noralf. > > > > In short, there's always a balance. > > I don't think we can make a rule for this, it will always depend on the > code. There is always going to be stuff we put in drm that should go > elsewhere, and stuff that is elsewhere that drm should use. > > I think however if we do add stuff like this, someone should keep track > of them and try to make them get further into the kernel. In this case > I don't think the patches are too insane to keep in drm and refactor up > later, in other cases I'm sure it'll be lot more obvious (i.e. we could > make the same argument for chunks of DAL :-) DAL's not under this exception, since DAL is all about using drm stuff more (and maybe fixing a few things in drm helpers). My concern here is only about going outside of drm, where at least sometimes it's just utter pain to get even the most trivial bits merged. And holding up an entire driver for that seems silly, hence merge first, try to further refactor later. For DAL amd has started to work on things properly, and since Alex is now drm-misc committer that should all progress reasonably quickly. -Daniel
On Mon, Feb 06, 2017 at 04:55:55PM -0600, Rob Herring wrote: > On Mon, Feb 6, 2017 at 5:08 AM, Thierry Reding <thierry.reding@gmail.com> wrote: > > On Mon, Feb 06, 2017 at 11:07:42AM +0100, Daniel Vetter wrote: > >> On Mon, Feb 6, 2017 at 10:35 AM, Thierry Reding <thierry.reding@gmail.com> > >> wrote: > >> > >> > > > > +EXPORT_SYMBOL(tinydrm_disable_backlight); > >> > > > > +#endif > >> > > > > >> > > > These look like they really should be part of the backlight subsystem. > >> > I > >> > > > don't see anything DRM specific about them. Well, except for the error > >> > > > messages. > >> > > > >> > > So this is a bit an unpopular opinion with some folks, but I don't > >> > require > >> > > anyone to submit new code to subsystems outside of drm for new drivers. > >> > > Simply because it takes months to get stuff landed, and in general it's > >> > > not worth the trouble. > >> > > >> > "Not worth the trouble" is very subjective. If you look at the Linux > >> > kernel in general, one of the reasons why it works so well is because > >> > the changes we make apply to the kernel as a whole. Yes, sometimes that > >> > makes things more difficult and time-consuming, but it also means that > >> > the end result will be much more widely usable and therefore benefits > >> > everyone else in return. In my opinion that's a large part of why the > >> > kernel is so successful. > >> > > >> > > We have piles of stuff in drm and drm drivers that should be in core but > >> > > isn't. > >> > > > >> > > Imo the only reasonable way is to merge as-is, then follow-up with a > >> > patch > >> > > series to move the helper into the right subsystem. Most often > >> > > unfortunately that follow-up patch series will just die. > >> > > >> > Of course follow-up series die. That's because nobody cares to follow-up > >> > once their code has been merged. > >> > > >> > Collecting our own helpers or variants of subsystems is a great way of > >> > isolating ourselves from the rest of the community. I don't think that's > >> > a good solution in the long run at all. > >> > > >> > >> We have a bunch of patch series that we resubmit for months and they go > >> exactly nowhere. They don't die because we stop caring, they die because > >> they die. Some of them we even need to constantly rebase and carry around > >> in drm-tip since our CI would Oops or spew WARNIGs all over the place. > >> There's simply some areas of the kernel which seem overloaded under patches > >> and no one is willing or able to fix things, and I can't fix the entire > >> kernel. Nor expect contributors (who have much less political weight to > >> throw around than me) to do that and succeed. And we don't end up with > >> worse code in the drm subsystem, since we can still do the refactoring > >> within drm helpers and end up with clean drivers. > >> > >> I fully agree that it's not great for the kernel's future, but when I'm > >> stuck with the option to get shit done or burning out playing the > >> upstreaming game, the choice is easy. And in the end I care about open > >> source gfx much more than the kernel, and I think for open source gfx's > >> success it's crucial that we're welcoming to new contributors and don't > >> throw up massive roadblocks. Open source gfx is tiny and still far away > >> from world domination, we need _lots_ more people. If that means routing > >> around other subsystems for them, I'm all for it. > > > > I can't say I fully agree with that sentiment. I do see how routing > > around subsystems can be useful occasionally. If nobody will merge the > > code, or if nobody cares, then by all means, let's make them DRM- > > specific helpers. > > In this case, these backlight helpers aren't even common across DRM. > They are tinydrm specific, but only in name and location. They look > like they'd be helpful to panel-simple.c and other panel drivers, too. > :) > > Who's to blame for duplication within DRM then? If only I had 1 > location of OF graph code to clean-up... I get new DT functions all > the time that other subsystems want, so I don't think the problem lies > there. > > > But I think we need to at least try to do the right thing. If only to > > teach people what the right way is. If we start accepting such things > > by default, how can we expect contributors to even try? > > > > I also think that contributors will often end up contributing not only > > to DRM but to the kernel as a whole. As such it should be part of our > > mentoring to teach them about how the process works as a rule, even if > > the occasional exception is necessary to get things done. > > Yes, it's important for contributors to learn to avoid "the platform > problem"[1]. Stuff Noralf has done over the past few months to get tinydrm merged: - proper deferred io support in fbdev helpers - refactoring all drivers to use the same that rolled their own - bunch of refactoring all around in drm core - remove the debugfs cleanup code from drivers and move into core I don't think you can make a "platform problem" case here at all. And as I've said (and Noralf seems to go that way too) I think this is starting to get a bit silly, and in my opinion better to merge now and polish further once merged. At least if ever drm driver would need to do this much refactoring compared to the code size before we allow it to land I don't think we'd have any drivers really. So can we get some acks for landing this please? Or do we need to bikeshed this a few more months to make sure it doesn't ... -Daniel
On Tue, Feb 07, 2017 at 08:28:16AM +1000, Dave Airlie wrote: > > > > I definitely don't want that we don't attempt this. But brought from years > > of experience, I recommend to merge first (with pre-refactoring already > > applied, but helpers only extracted, not yet at the right spot), and then > > follow up with. Because on average, there's way too many trees with > > overloaded maintainers who maybe look at your patch once per kernel > > release cycle. > > > > If you know that backlight and spi isn't one of these areas (anything that > > goes through takashi/sound is a similar good experience for us on the i915 > > side), then I guess we can try. But then Noralf has already written a few > > months worth of really great refactoring, and I'm seriously starting to > > feel guilty for volunteering him for all of this. Even though he seems to > > be really good at it, and seems to not mind, it's getting a bit silly. > > Given that I'd say up to Noralf. > > > > In short, there's always a balance. > > I don't think we can make a rule for this, it will always depend on the > code. There is always going to be stuff we put in drm that should go > elsewhere, and stuff that is elsewhere that drm should use. > > I think however if we do add stuff like this, someone should keep track > of them and try to make them get further into the kernel. Yes, I think having some sort of TODO in drivers/gpu/drm could help track things that we know should eventually be moved out. It could serve as a list of janitorial tasks for newcomers that want to get their hands dirty and tackle relatively trivial tasks. That's not meant to devalue such contributions. Code would be in a mostly finished form, so it'd be mostly about moving things into the correct subsystem, interacting with maintainers, making sure things are kept working along the way, that kind of thing. No in-depth knowledge of DRM/KMS would be required, but perhaps be picked up along the way. Thierry
On Tue, Feb 07, 2017 at 12:11:28PM +0100, Thierry Reding wrote: > On Tue, Feb 07, 2017 at 08:28:16AM +1000, Dave Airlie wrote: > > > > > > I definitely don't want that we don't attempt this. But brought from years > > > of experience, I recommend to merge first (with pre-refactoring already > > > applied, but helpers only extracted, not yet at the right spot), and then > > > follow up with. Because on average, there's way too many trees with > > > overloaded maintainers who maybe look at your patch once per kernel > > > release cycle. > > > > > > If you know that backlight and spi isn't one of these areas (anything that > > > goes through takashi/sound is a similar good experience for us on the i915 > > > side), then I guess we can try. But then Noralf has already written a few > > > months worth of really great refactoring, and I'm seriously starting to > > > feel guilty for volunteering him for all of this. Even though he seems to > > > be really good at it, and seems to not mind, it's getting a bit silly. > > > Given that I'd say up to Noralf. > > > > > > In short, there's always a balance. > > > > I don't think we can make a rule for this, it will always depend on the > > code. There is always going to be stuff we put in drm that should go > > elsewhere, and stuff that is elsewhere that drm should use. > > > > I think however if we do add stuff like this, someone should keep track > > of them and try to make them get further into the kernel. > > Yes, I think having some sort of TODO in drivers/gpu/drm could help > track things that we know should eventually be moved out. It could serve > as a list of janitorial tasks for newcomers that want to get their hands > dirty and tackle relatively trivial tasks. We have this list already, it's at: http://www.x.org/wiki/DRMJanitors/ I guess I should highlight it more, maybe even add it to the docs? Eric just asked about it last week too. -Daniel
On Mon, Feb 06, 2017 at 11:11:27PM +0100, Noralf Trønnes wrote: > > Den 06.02.2017 16.53, skrev Daniel Vetter: > > On Mon, Feb 06, 2017 at 12:08:47PM +0100, Thierry Reding wrote: > > > On Mon, Feb 06, 2017 at 11:07:42AM +0100, Daniel Vetter wrote: > > > > On Mon, Feb 6, 2017 at 10:35 AM, Thierry Reding <thierry.reding@gmail.com> > > > > wrote: > > > > > > > > > > > > +EXPORT_SYMBOL(tinydrm_disable_backlight); > > > > > > > > +#endif > > > > > > > These look like they really should be part of the backlight subsystem. > > > > > I > > > > > > > don't see anything DRM specific about them. Well, except for the error > > > > > > > messages. > > > > > > So this is a bit an unpopular opinion with some folks, but I don't > > > > > require > > > > > > anyone to submit new code to subsystems outside of drm for new drivers. > > > > > > Simply because it takes months to get stuff landed, and in general it's > > > > > > not worth the trouble. > > > > > "Not worth the trouble" is very subjective. If you look at the Linux > > > > > kernel in general, one of the reasons why it works so well is because > > > > > the changes we make apply to the kernel as a whole. Yes, sometimes that > > > > > makes things more difficult and time-consuming, but it also means that > > > > > the end result will be much more widely usable and therefore benefits > > > > > everyone else in return. In my opinion that's a large part of why the > > > > > kernel is so successful. > > > > > > > > > > > We have piles of stuff in drm and drm drivers that should be in core but > > > > > > isn't. > > > > > > > > > > > > Imo the only reasonable way is to merge as-is, then follow-up with a > > > > > patch > > > > > > series to move the helper into the right subsystem. Most often > > > > > > unfortunately that follow-up patch series will just die. > > > > > Of course follow-up series die. That's because nobody cares to follow-up > > > > > once their code has been merged. > > > > > > > > > > Collecting our own helpers or variants of subsystems is a great way of > > > > > isolating ourselves from the rest of the community. I don't think that's > > > > > a good solution in the long run at all. > > > > > > > > > We have a bunch of patch series that we resubmit for months and they go > > > > exactly nowhere. They don't die because we stop caring, they die because > > > > they die. Some of them we even need to constantly rebase and carry around > > > > in drm-tip since our CI would Oops or spew WARNIGs all over the place. > > > > There's simply some areas of the kernel which seem overloaded under patches > > > > and no one is willing or able to fix things, and I can't fix the entire > > > > kernel. Nor expect contributors (who have much less political weight to > > > > throw around than me) to do that and succeed. And we don't end up with > > > > worse code in the drm subsystem, since we can still do the refactoring > > > > within drm helpers and end up with clean drivers. > > > > > > > > I fully agree that it's not great for the kernel's future, but when I'm > > > > stuck with the option to get shit done or burning out playing the > > > > upstreaming game, the choice is easy. And in the end I care about open > > > > source gfx much more than the kernel, and I think for open source gfx's > > > > success it's crucial that we're welcoming to new contributors and don't > > > > throw up massive roadblocks. Open source gfx is tiny and still far away > > > > from world domination, we need _lots_ more people. If that means routing > > > > around other subsystems for them, I'm all for it. > > > I can't say I fully agree with that sentiment. I do see how routing > > > around subsystems can be useful occasionally. If nobody will merge the > > > code, or if nobody cares, then by all means, let's make them DRM- > > > specific helpers. > > > > > > But I think we need to at least try to do the right thing. If only to > > > teach people what the right way is. If we start accepting such things > > > by default, how can we expect contributors to even try? > > > > > > I also think that contributors will often end up contributing not only > > > to DRM but to the kernel as a whole. As such it should be part of our > > > mentoring to teach them about how the process works as a rule, even if > > > the occasional exception is necessary to get things done. > > > > > > In this particular case, I know for a fact that both backlight and SPI > > > maintainers are very responsive, so that's not a good excuse. > > I definitely don't want that we don't attempt this. But brought from years > > of experience, I recommend to merge first (with pre-refactoring already > > applied, but helpers only extracted, not yet at the right spot), and then > > follow up with. Because on average, there's way too many trees with > > overloaded maintainers who maybe look at your patch once per kernel > > release cycle. > > > > If you know that backlight and spi isn't one of these areas (anything that > > goes through takashi/sound is a similar good experience for us on the i915 > > side), then I guess we can try. But then Noralf has already written a few > > months worth of really great refactoring, and I'm seriously starting to > > feel guilty for volunteering him for all of this. Even though he seems to > > be really good at it, and seems to not mind, it's getting a bit silly. > > Given that I'd say up to Noralf. > > > > In short, there's always a balance. > > Yes, it's a balance between the perfect and not's so perfect, > the professinal and the amateur, and the drm expert and the newbie. I may have come across as a bit rough. You've been doing great work, there's no doubt about that. I was merely raising this issue because I think it's something that we need to keep an eye out for. We don't want to end up in a situation where we duplicate code or keep code to DRM that would be useful outside of DRM. The link that Rob pointed to is very insightful, and I think it's very easy to loose touch with other parts of the kernel when you work on one subsystem (almost) exclusively. It's completely normal, but because of that I think it's all the more important that we remind ourselves to look beyond our noses and refactor beyond just DRM where possible. > If I knew how much time I would have to spend on tinydrm to get it > merged, then I would never have started it. I wondered, a couple of > months back, if I should just cut my losses and move to something else. > dri-devel is a friendly place and I do get help to keep moving, but I > get the feeling that drm is really a place for professionals that write > kernel code 50 hours a week. And there's nothing wrong in that, drm is > complex and maybe that kind of expertice and work-hours are needed to > do work here. I don't think that's true in general. If graphics, and especially the lowlevel parts of it, is something that you're interested in, then dri-devel is the perfect place for you. It doesn't matter if you work on it in your spare time or on the day job. It just so happens that most people end up working on it on the day job because there's so much work to do that spare time isn't usually enough to do all you want. This is probably true for most of the other areas of the kernel, too. It's not unusual for regular contributors to be offered a job to keep doing what they do. That's probably why we have a higher degree of professionalization in the kernel community as a whole. I don't think that's a bad thing, quite the contrary, it's what we had all been wishing for for a very long time. I can understand how that might be frustrating to hobbyists at times, but I hope we're at least not making things worse by actively discouraging hobbyists. > It's not that I plan on backing out now, I'm just putting down a few > words about the challenge it has been for me as a hobbyist to meet drm. > > > As for the specifics, > > Backlight enable/disable helpers, I haven't thought about those as > backlight subsystem helpers. But I see some drm panel drivers that do > the same, so it makes sense. > But what I'm feeling is this: > If I'm expected to get everything right, then I will "never" get this > merged. > This thing by itself isn't much, but adding up every little thing > amounts to a lot. And it's not just "make this patch", it's probably > clean up the drivers as well :-) Been there, done that. I think the problem that triggers the kind of response that I gave is that it's often a handful of people that end up doing the refactorings. Usually that handful of people is overloaded as it is, so refactorings end up taking very long and are very tiring. The easy way out, of course, is to require new patches to do the right thing to begin with, because people are often motivated by the fact that they will finally get their patch in. That's of course also a very good way of discouraging contributions because it may be more than people set out for. Unfortunately I don't think there's a good solution for this. But, as Daniel and Dave have already said, it shouldn't be a blocker for your series. It'd be really nice if you somehow found the time to follow up, though, after TinyDRM was merged. > tinydrm_spi_transfer() can print the content of the buffer. This is > important at least until I have seen some real use of the drivers. > I have emulation code for handling bit widths not supported by the SPI > controller, so I want to see what goes over the wire. SPI core uses > trace events to track what's going on, and I don't think I can get > buffer dumping into that code. I know Mark is a very reasonable person, so I think it can't hurt to try. Often people will end up rolling buffer dumping code of their own and having something in the SPI core to do that, possibly hidden behind a SPI_DEBUG_BUFFER Kconfig option or something like that, would save a lot of people a lot of time. Not sure if it would help, but if you end up writing such a patch and you Cc me when you post it, I'm going to ACK it. > Don't get me wrong Thierry, I do appreciate that you take the time to > look at the code. I'm just frustrated that it takes so long to get this > right. I thought that all I needed now was a DT maintainer ack :-) I think that's fine. You've done a great job so far and we can always fine-tune later on. Thierry
On Tue, Feb 07, 2017 at 12:21:28PM +0100, Daniel Vetter wrote: > On Tue, Feb 07, 2017 at 12:11:28PM +0100, Thierry Reding wrote: > > On Tue, Feb 07, 2017 at 08:28:16AM +1000, Dave Airlie wrote: > > > > > > > > I definitely don't want that we don't attempt this. But brought from years > > > > of experience, I recommend to merge first (with pre-refactoring already > > > > applied, but helpers only extracted, not yet at the right spot), and then > > > > follow up with. Because on average, there's way too many trees with > > > > overloaded maintainers who maybe look at your patch once per kernel > > > > release cycle. > > > > > > > > If you know that backlight and spi isn't one of these areas (anything that > > > > goes through takashi/sound is a similar good experience for us on the i915 > > > > side), then I guess we can try. But then Noralf has already written a few > > > > months worth of really great refactoring, and I'm seriously starting to > > > > feel guilty for volunteering him for all of this. Even though he seems to > > > > be really good at it, and seems to not mind, it's getting a bit silly. > > > > Given that I'd say up to Noralf. > > > > > > > > In short, there's always a balance. > > > > > > I don't think we can make a rule for this, it will always depend on the > > > code. There is always going to be stuff we put in drm that should go > > > elsewhere, and stuff that is elsewhere that drm should use. > > > > > > I think however if we do add stuff like this, someone should keep track > > > of them and try to make them get further into the kernel. > > > > Yes, I think having some sort of TODO in drivers/gpu/drm could help > > track things that we know should eventually be moved out. It could serve > > as a list of janitorial tasks for newcomers that want to get their hands > > dirty and tackle relatively trivial tasks. > > We have this list already, it's at: http://www.x.org/wiki/DRMJanitors/ > > I guess I should highlight it more, maybe even add it to the docs? Eric > just asked about it last week too. Yeah, I'm aware of that list. I think it's a little problematic that it's in a wiki and far removed from where the actual work is happening. I think we should just take that list and add it as a TODO in drivers/gpu/drm, or alternatively keep it as part of the GPU documentation. That way we can more easily mark things as done or add new stuff as work gets done. For cases like this I think we could just add new items as they are pointed out during review. For things that are already merged we can add items separately. Once the refactoring is done, the patch series can contain a final patch that simply removes the items again. I think that has much less potential to become out-dated than a separate wiki page. FWIW, I'll volunteer to move the list to git if we decide to go ahead with that. Thierry
On Tue, Feb 07, 2017 at 12:44:19PM +0100, Thierry Reding wrote: > On Tue, Feb 07, 2017 at 12:21:28PM +0100, Daniel Vetter wrote: > > On Tue, Feb 07, 2017 at 12:11:28PM +0100, Thierry Reding wrote: > > > On Tue, Feb 07, 2017 at 08:28:16AM +1000, Dave Airlie wrote: > > > > > > > > > > I definitely don't want that we don't attempt this. But brought from years > > > > > of experience, I recommend to merge first (with pre-refactoring already > > > > > applied, but helpers only extracted, not yet at the right spot), and then > > > > > follow up with. Because on average, there's way too many trees with > > > > > overloaded maintainers who maybe look at your patch once per kernel > > > > > release cycle. > > > > > > > > > > If you know that backlight and spi isn't one of these areas (anything that > > > > > goes through takashi/sound is a similar good experience for us on the i915 > > > > > side), then I guess we can try. But then Noralf has already written a few > > > > > months worth of really great refactoring, and I'm seriously starting to > > > > > feel guilty for volunteering him for all of this. Even though he seems to > > > > > be really good at it, and seems to not mind, it's getting a bit silly. > > > > > Given that I'd say up to Noralf. > > > > > > > > > > In short, there's always a balance. > > > > > > > > I don't think we can make a rule for this, it will always depend on the > > > > code. There is always going to be stuff we put in drm that should go > > > > elsewhere, and stuff that is elsewhere that drm should use. > > > > > > > > I think however if we do add stuff like this, someone should keep track > > > > of them and try to make them get further into the kernel. > > > > > > Yes, I think having some sort of TODO in drivers/gpu/drm could help > > > track things that we know should eventually be moved out. It could serve > > > as a list of janitorial tasks for newcomers that want to get their hands > > > dirty and tackle relatively trivial tasks. > > > > We have this list already, it's at: http://www.x.org/wiki/DRMJanitors/ > > > > I guess I should highlight it more, maybe even add it to the docs? Eric > > just asked about it last week too. > > Yeah, I'm aware of that list. I think it's a little problematic that > it's in a wiki and far removed from where the actual work is happening. > I think we should just take that list and add it as a TODO in > drivers/gpu/drm, or alternatively keep it as part of the GPU > documentation. That way we can more easily mark things as done or add > new stuff as work gets done. > > For cases like this I think we could just add new items as they are > pointed out during review. For things that are already merged we can > add items separately. Once the refactoring is done, the patch series > can contain a final patch that simply removes the items again. I think > that has much less potential to become out-dated than a separate wiki > page. > > FWIW, I'll volunteer to move the list to git if we decide to go ahead > with that. One upside of a wiki is that it's quicker to edit, if someone spots a drive-by refactoring they might not bother with the formal requirements of a full patch. Otoh, having it in-source-tree definitely has benefits, too. If you do the conversion I'd vote for Documentation/gpu/TODO.rst, and linking it into our documentation (maybe even cross-link from introduction.rst under a "Getting Started" heading, as reasonable ramp-up tasks after some checkpatch patches). I think that would help highlight it a bit. And of course the wiki page needs to be removed and replaced with a link to the new canonical thing (probably best to point at the source-file in drm-tip.git, that should be the most up-to-date). Thanks, Daniel
diff --git a/Documentation/gpu/tinydrm.rst b/Documentation/gpu/tinydrm.rst index ec4a20d..fb256d2 100644 --- a/Documentation/gpu/tinydrm.rst +++ b/Documentation/gpu/tinydrm.rst @@ -19,3 +19,12 @@ Core functionality .. kernel-doc:: drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c :export: + +Additional helpers +================== + +.. kernel-doc:: include/drm/tinydrm/tinydrm-helpers.h + :internal: + +.. kernel-doc:: drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c + :export: diff --git a/drivers/gpu/drm/tinydrm/core/Makefile b/drivers/gpu/drm/tinydrm/core/Makefile index 4f14a0f..fb221e6 100644 --- a/drivers/gpu/drm/tinydrm/core/Makefile +++ b/drivers/gpu/drm/tinydrm/core/Makefile @@ -1,3 +1,3 @@ -tinydrm-y := tinydrm-core.o tinydrm-pipe.o +tinydrm-y := tinydrm-core.o tinydrm-pipe.o tinydrm-helpers.o obj-$(CONFIG_DRM_TINYDRM) += tinydrm.o diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c new file mode 100644 index 0000000..75e4e54 --- /dev/null +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c @@ -0,0 +1,462 @@ +/* + * Copyright (C) 2016 Noralf Trønnes + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + */ + +#include <drm/tinydrm/tinydrm.h> +#include <drm/tinydrm/tinydrm-helpers.h> +#include <linux/backlight.h> +#include <linux/pm.h> +#include <linux/spi/spi.h> +#include <linux/swab.h> + +static unsigned int spi_max; +module_param(spi_max, uint, 0400); +MODULE_PARM_DESC(spi_max, "Set a lower SPI max transfer size"); + +/** + * tinydrm_merge_clips - Merge clip rectangles + * @dst: Destination clip rectangle + * @src: Source clip rectangle(s) + * @num_clips: Number of @src clip rectangles + * @flags: Dirty fb ioctl flags + * @max_width: Maximum width of @dst + * @max_height: Maximum height of @dst + * + * This function merges @src clip rectangle(s) into @dst. If @src is NULL, + * @max_width and @min_width is used to set a full @dst clip rectangle. + * + * Returns: + * true if it's a full clip, false otherwise + */ +bool tinydrm_merge_clips(struct drm_clip_rect *dst, + struct drm_clip_rect *src, unsigned int num_clips, + unsigned int flags, u32 max_width, u32 max_height) +{ + unsigned int i; + + if (!src || !num_clips) { + dst->x1 = 0; + dst->x2 = max_width; + dst->y1 = 0; + dst->y2 = max_height; + return true; + } + + dst->x1 = ~0; + dst->y1 = ~0; + dst->x2 = 0; + dst->y2 = 0; + + for (i = 0; i < num_clips; i++) { + if (flags & DRM_MODE_FB_DIRTY_ANNOTATE_COPY) + i++; + dst->x1 = min(dst->x1, src[i].x1); + dst->x2 = max(dst->x2, src[i].x2); + dst->y1 = min(dst->y1, src[i].y1); + dst->y2 = max(dst->y2, src[i].y2); + } + + if (dst->x2 > max_width || dst->y2 > max_height || + dst->x1 >= dst->x2 || dst->y1 >= dst->y2) { + DRM_DEBUG_KMS("Illegal clip: x1=%u, x2=%u, y1=%u, y2=%u\n", + dst->x1, dst->x2, dst->y1, dst->y2); + dst->x1 = 0; + dst->y1 = 0; + dst->x2 = max_width; + dst->y2 = max_height; + } + + return (dst->x2 - dst->x1) == max_width && + (dst->y2 - dst->y1) == max_height; +} +EXPORT_SYMBOL(tinydrm_merge_clips); + +/** + * tinydrm_memcpy - Copy clip buffer + * @dst: Destination buffer + * @vaddr: Source buffer + * @fb: DRM framebuffer + * @clip: Clip rectangle area to copy + */ +void tinydrm_memcpy(void *dst, void *vaddr, struct drm_framebuffer *fb, + struct drm_clip_rect *clip) +{ + unsigned int cpp = drm_format_plane_cpp(fb->format->format, 0); + unsigned int pitch = fb->pitches[0]; + void *src = vaddr + (clip->y1 * pitch) + (clip->x1 * cpp); + size_t len = (clip->x2 - clip->x1) * cpp; + unsigned int y; + + for (y = clip->y1; y < clip->y2; y++) { + memcpy(dst, src, len); + src += pitch; + dst += len; + } +} +EXPORT_SYMBOL(tinydrm_memcpy); + +/** + * tinydrm_swab16 - Swap bytes into clip buffer + * @dst: RGB565 destination buffer + * @vaddr: RGB565 source buffer + * @fb: DRM framebuffer + * @clip: Clip rectangle area to copy + */ +void tinydrm_swab16(u16 *dst, void *vaddr, struct drm_framebuffer *fb, + struct drm_clip_rect *clip) +{ + size_t len = (clip->x2 - clip->x1) * sizeof(u16); + unsigned int x, y; + u16 *src, *buf; + + /* + * The cma memory is write-combined so reads are uncached. + * Speed up by fetching one line at a time. + */ + buf = kmalloc(len, GFP_KERNEL); + if (!buf) + return; + + for (y = clip->y1; y < clip->y2; y++) { + src = vaddr + (y * fb->pitches[0]); + src += clip->x1; + memcpy(buf, src, len); + src = buf; + for (x = clip->x1; x < clip->x2; x++) + *dst++ = swab16(*src++); + } + + kfree(buf); +} +EXPORT_SYMBOL(tinydrm_swab16); + +/** + * tinydrm_xrgb8888_to_rgb565 - Convert XRGB8888 to RGB565 clip buffer + * @dst: RGB565 destination buffer + * @vaddr: XRGB8888 source buffer + * @fb: DRM framebuffer + * @clip: Clip rectangle area to copy + * @swap: Swap bytes + * + * Drivers can use this function for RGB565 devices that don't natively + * support XRGB8888. + */ +void tinydrm_xrgb8888_to_rgb565(u16 *dst, void *vaddr, + struct drm_framebuffer *fb, + struct drm_clip_rect *clip, bool swap) +{ + size_t len = (clip->x2 - clip->x1) * sizeof(u32); + unsigned int x, y; + u32 *src, *buf; + u16 val16; + + buf = kmalloc(len, GFP_KERNEL); + if (!buf) + return; + + for (y = clip->y1; y < clip->y2; y++) { + src = vaddr + (y * fb->pitches[0]); + src += clip->x1; + memcpy(buf, src, len); + src = buf; + for (x = clip->x1; x < clip->x2; x++) { + val16 = ((*src & 0x00F80000) >> 8) | + ((*src & 0x0000FC00) >> 5) | + ((*src & 0x000000F8) >> 3); + src++; + if (swap) + *dst++ = swab16(val16); + else + *dst++ = val16; + } + } + + kfree(buf); +} +EXPORT_SYMBOL(tinydrm_xrgb8888_to_rgb565); + +#ifdef CONFIG_BACKLIGHT_CLASS_DEVICE +/** + * tinydrm_of_find_backlight - Find backlight device in device-tree + * @dev: Device + * + * This function looks for a DT node pointed to by a property named 'backlight' + * and uses of_find_backlight_by_node() to get the backlight device. + * Additionally if the brightness property is zero, it is set to + * max_brightness. + * + * Returns: + * NULL if there's no backlight property. + * Error pointer -EPROBE_DEFER if the DT node is found, but no backlight device + * is found. + * If the backlight device is found, a pointer to the structure is returned. + */ +struct backlight_device *tinydrm_of_find_backlight(struct device *dev) +{ + struct backlight_device *backlight; + struct device_node *np; + + np = of_parse_phandle(dev->of_node, "backlight", 0); + if (!np) + return NULL; + + backlight = of_find_backlight_by_node(np); + of_node_put(np); + + if (!backlight) + return ERR_PTR(-EPROBE_DEFER); + + if (!backlight->props.brightness) { + backlight->props.brightness = backlight->props.max_brightness; + DRM_DEBUG_KMS("Backlight brightness set to %d\n", + backlight->props.brightness); + } + + return backlight; +} +EXPORT_SYMBOL(tinydrm_of_find_backlight); + +/** + * tinydrm_enable_backlight - Enable backlight helper + * @backlight: Backlight device + * + * Returns: + * Zero on success, negative error code on failure. + */ +int tinydrm_enable_backlight(struct backlight_device *backlight) +{ + unsigned int old_state; + int ret; + + if (!backlight) + return 0; + + old_state = backlight->props.state; + backlight->props.state &= ~BL_CORE_FBBLANK; + DRM_DEBUG_KMS("Backlight state: 0x%x -> 0x%x\n", old_state, + backlight->props.state); + + ret = backlight_update_status(backlight); + if (ret) + DRM_ERROR("Failed to enable backlight %d\n", ret); + + return ret; +} +EXPORT_SYMBOL(tinydrm_enable_backlight); + +/** + * tinydrm_disable_backlight - Disable backlight helper + * @backlight: Backlight device + * + * Returns: + * Zero on success, negative error code on failure. + */ +int tinydrm_disable_backlight(struct backlight_device *backlight) +{ + unsigned int old_state; + int ret; + + if (!backlight) + return 0; + + old_state = backlight->props.state; + backlight->props.state |= BL_CORE_FBBLANK; + DRM_DEBUG_KMS("Backlight state: 0x%x -> 0x%x\n", old_state, + backlight->props.state); + ret = backlight_update_status(backlight); + if (ret) + DRM_ERROR("Failed to disable backlight %d\n", ret); + + return ret; +} +EXPORT_SYMBOL(tinydrm_disable_backlight); +#endif + +#ifdef CONFIG_SPI + +/** + * tinydrm_spi_max_transfer_size - Determine max SPI transfer size + * @spi: SPI device + * @max_len: Maximum buffer size needed (optional) + * + * This function returns the maximum size to use for SPI transfers. It checks + * the SPI master, the optional @max_len and the module parameter spi_max and + * returns the smallest. + * + * Returns: + * Maximum size for SPI transfers + */ +size_t tinydrm_spi_max_transfer_size(struct spi_device *spi, size_t max_len) +{ + size_t ret; + + ret = min(spi_max_transfer_size(spi), spi->master->max_dma_len); + if (max_len) + ret = min(ret, max_len); + if (spi_max) + ret = min_t(size_t, ret, spi_max); + ret &= ~0x3; + if (ret < 4) + ret = 4; + + return ret; +} +EXPORT_SYMBOL(tinydrm_spi_max_transfer_size); + +/** + * tinydrm_spi_bpw_supported - Check if bits per word is supported + * @spi: SPI device + * @bpw: Bits per word + * + * This function checks to see if the SPI master driver supports @bpw. + * + * Returns: + * True if @bpw is supported, false otherwise. + */ +bool tinydrm_spi_bpw_supported(struct spi_device *spi, u8 bpw) +{ + u32 bpw_mask = spi->master->bits_per_word_mask; + + if (bpw == 8) + return true; + + if (!bpw_mask) { + dev_warn_once(&spi->dev, + "bits_per_word_mask not set, assume 8-bit only\n"); + return false; + } + + if (bpw_mask & SPI_BPW_MASK(bpw)) + return true; + + return false; +} +EXPORT_SYMBOL(tinydrm_spi_bpw_supported); + +static void +tinydrm_dbg_spi_print(struct spi_device *spi, struct spi_transfer *tr, + const void *buf, int idx, bool tx) +{ + u32 speed_hz = tr->speed_hz ? tr->speed_hz : spi->max_speed_hz; + char linebuf[3 * 32]; + + hex_dump_to_buffer(buf, tr->len, 16, + DIV_ROUND_UP(tr->bits_per_word, 8), + linebuf, sizeof(linebuf), false); + + printk(KERN_DEBUG + " tr(%i): speed=%u%s, bpw=%i, len=%u, %s_buf=[%s%s]\n", idx, + speed_hz > 1000000 ? speed_hz / 1000000 : speed_hz / 1000, + speed_hz > 1000000 ? "MHz" : "kHz", tr->bits_per_word, tr->len, + tx ? "tx" : "rx", linebuf, tr->len > 16 ? " ..." : ""); +} + +/* called through tinydrm_dbg_spi_message() */ +void _tinydrm_dbg_spi_message(struct spi_device *spi, struct spi_message *m) +{ + struct spi_transfer *tmp; + struct list_head *pos; + int i = 0; + + list_for_each(pos, &m->transfers) { + tmp = list_entry(pos, struct spi_transfer, transfer_list); + + if (tmp->tx_buf) + tinydrm_dbg_spi_print(spi, tmp, tmp->tx_buf, i, true); + if (tmp->rx_buf) + tinydrm_dbg_spi_print(spi, tmp, tmp->rx_buf, i, false); + i++; + } +} +EXPORT_SYMBOL(_tinydrm_dbg_spi_message); + +/** + * tinydrm_spi_transfer - SPI transfer helper + * @spi: SPI device + * @speed_hz: Override speed (optional) + * @header: Optional header transfer + * @bpw: Bits per word + * @buf: Buffer to transfer + * @len: Buffer length + * + * This SPI transfer helper breaks up the transfer of @buf into chunks which + * the SPI master driver can handle. If the machine is Little Endian and the + * SPI master driver doesn't support 16 bits per word, it swaps the bytes and + * does a 8-bit transfer. + * If @header is set, it is prepended to each SPI message. + * + * Returns: + * Zero on success, negative error code on failure. + */ +int tinydrm_spi_transfer(struct spi_device *spi, u32 speed_hz, + struct spi_transfer *header, u8 bpw, const void *buf, + size_t len) +{ + struct spi_transfer tr = { + .bits_per_word = bpw, + .speed_hz = speed_hz, + }; + struct spi_message m; + u16 *swap_buf = NULL; + size_t max_chunk; + size_t chunk; + int ret = 0; + + if (WARN_ON_ONCE(bpw != 8 && bpw != 16)) + return -EINVAL; + + max_chunk = tinydrm_spi_max_transfer_size(spi, 0); + + if (drm_debug & DRM_UT_DRIVER) + pr_debug("[drm:%s] bpw=%u, max_chunk=%zu, transfers:\n", + __func__, bpw, max_chunk); + + if (bpw == 16 && !tinydrm_spi_bpw_supported(spi, 16)) { + tr.bits_per_word = 8; + if (tinydrm_machine_little_endian()) { + swap_buf = kmalloc(min(len, max_chunk), GFP_KERNEL); + if (!swap_buf) + return -ENOMEM; + } + } + + spi_message_init(&m); + if (header) + spi_message_add_tail(header, &m); + spi_message_add_tail(&tr, &m); + + while (len) { + chunk = min(len, max_chunk); + + tr.tx_buf = buf; + tr.len = chunk; + + if (swap_buf) { + const u16 *buf16 = buf; + unsigned int i; + + for (i = 0; i < chunk / 2; i++) + swap_buf[i] = swab16(buf16[i]); + + tr.tx_buf = swap_buf; + } + + buf += chunk; + len -= chunk; + + tinydrm_dbg_spi_message(spi, &m); + ret = spi_sync(spi, &m); + if (ret) + return ret; + }; + + return 0; +} +EXPORT_SYMBOL(tinydrm_spi_transfer); + +#endif /* CONFIG_SPI */ diff --git a/include/drm/tinydrm/tinydrm-helpers.h b/include/drm/tinydrm/tinydrm-helpers.h new file mode 100644 index 0000000..78175fe --- /dev/null +++ b/include/drm/tinydrm/tinydrm-helpers.h @@ -0,0 +1,100 @@ +/* + * Copyright (C) 2016 Noralf Trønnes + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + */ + +#ifndef __LINUX_TINYDRM_HELPERS_H +#define __LINUX_TINYDRM_HELPERS_H + +struct backlight_device; +struct tinydrm_device; +struct drm_clip_rect; +struct spi_transfer; +struct spi_message; +struct spi_device; +struct device; + +/** + * tinydrm_machine_little_endian - Machine is little endian + * + * Returns: + * true if *defined(__LITTLE_ENDIAN)*, false otherwise + */ +static inline bool tinydrm_machine_little_endian(void) +{ +#if defined(__LITTLE_ENDIAN) + return true; +#else + return false; +#endif +} + +bool tinydrm_merge_clips(struct drm_clip_rect *dst, + struct drm_clip_rect *src, unsigned int num_clips, + unsigned int flags, u32 max_width, u32 max_height); +void tinydrm_memcpy(void *dst, void *vaddr, struct drm_framebuffer *fb, + struct drm_clip_rect *clip); +void tinydrm_swab16(u16 *dst, void *vaddr, struct drm_framebuffer *fb, + struct drm_clip_rect *clip); +void tinydrm_xrgb8888_to_rgb565(u16 *dst, void *vaddr, + struct drm_framebuffer *fb, + struct drm_clip_rect *clip, bool swap); + +#ifdef CONFIG_BACKLIGHT_CLASS_DEVICE +struct backlight_device *tinydrm_of_find_backlight(struct device *dev); +int tinydrm_enable_backlight(struct backlight_device *backlight); +int tinydrm_disable_backlight(struct backlight_device *backlight); +#else +static inline struct backlight_device * +tinydrm_of_find_backlight(struct device *dev) +{ + return NULL; +} + +static inline int tinydrm_enable_backlight(struct backlight_device *backlight) +{ + return 0; +} + +static inline int +tinydrm_disable_backlight(struct backlight_device *backlight) +{ + return 0; +} +#endif + +size_t tinydrm_spi_max_transfer_size(struct spi_device *spi, size_t max_len); +bool tinydrm_spi_bpw_supported(struct spi_device *spi, u8 bpw); +int tinydrm_spi_transfer(struct spi_device *spi, u32 speed_hz, + struct spi_transfer *header, u8 bpw, const void *buf, + size_t len); +void _tinydrm_dbg_spi_message(struct spi_device *spi, struct spi_message *m); + +#ifdef DEBUG +/** + * tinydrm_dbg_spi_message - Dump SPI message + * @spi: SPI device + * @m: SPI message + * + * Dumps info about the transfers in a SPI message including buffer content. + * DEBUG has to be defined for this function to be enabled alongside setting + * the DRM_UT_DRIVER bit of &drm_debug. + */ +static inline void tinydrm_dbg_spi_message(struct spi_device *spi, + struct spi_message *m) +{ + if (drm_debug & DRM_UT_DRIVER) + _tinydrm_dbg_spi_message(spi, m); +} +#else +static inline void tinydrm_dbg_spi_message(struct spi_device *spi, + struct spi_message *m) +{ +} +#endif /* DEBUG */ + +#endif /* __LINUX_TINYDRM_HELPERS_H */