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

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 5663 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]