Re: [PATCH v3 2/7] drm/tinydrm: Add helper functions

[Daniel Vetter]

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@xxxxxxxxxxx>
> > Acked-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
> > ---
> >  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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch