RE: [PATCH v3 3/8] drm: Add bus timings helper

[Fabrizio Castro]

Hi Daniel,

Thank you for your feedback!

> From: Daniel Vetter <daniel.vetter@xxxxxxxx> On Behalf Of Daniel Vetter
> Sent: 07 November 2019 19:30
> Subject: Re: [PATCH v3 3/8] drm: Add bus timings helper
> 
> On Thu, Nov 07, 2019 at 09:26:21PM +0200, Laurent Pinchart wrote:
> > Hi Fabrizio,
> >
> > Thank you for the patch.
> >
> > On Wed, Aug 28, 2019 at 07:36:37PM +0100, Fabrizio Castro wrote:
> > > Helper to provide bus timing information.
> >
> > You may want to expand this a bit. And actually fix it too, as the
> > helper you introduce isn't related to timings (same for the subject
> > line).
> 
> Also the kerneldoc needs to be pulled into the templates under
> Documentation/gpu. And since it's just one function, why not put this into
> drm_of.c? Gets rid of a pile of overhead.

Yeah, you are right, will try and pull this into drm_of.c in v4.

Thanks!

Fab

> 
> >
> > > Signed-off-by: Fabrizio Castro <fabrizio.castro@xxxxxxxxxxxxxx>
> > >
> > > ---
> > > v2->v3:
> > > * new patch
> > > ---
> > >  drivers/gpu/drm/Makefile          |  3 +-
> > >  drivers/gpu/drm/drm_bus_timings.c | 97 +++++++++++++++++++++++++++++++++++++++
> > >  include/drm/drm_bus_timings.h     | 21 +++++++++
> > >  3 files changed, 120 insertions(+), 1 deletion(-)
> > >  create mode 100644 drivers/gpu/drm/drm_bus_timings.c
> > >  create mode 100644 include/drm/drm_bus_timings.h
> > >
> > > diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> > > index 9f0d2ee..a270063 100644
> > > --- a/drivers/gpu/drm/Makefile
> > > +++ b/drivers/gpu/drm/Makefile
> > > @@ -17,7 +17,8 @@ drm-y       :=	drm_auth.o drm_cache.o \
> > >  		drm_plane.o drm_color_mgmt.o drm_print.o \
> > >  		drm_dumb_buffers.o drm_mode_config.o drm_vblank.o \
> > >  		drm_syncobj.o drm_lease.o drm_writeback.o drm_client.o \
> > > -		drm_client_modeset.o drm_atomic_uapi.o drm_hdcp.o
> > > +		drm_client_modeset.o drm_atomic_uapi.o drm_hdcp.o \
> > > +		drm_bus_timings.o
> > >
> > >  drm-$(CONFIG_DRM_LEGACY) += drm_legacy_misc.o drm_bufs.o drm_context.o drm_dma.o drm_scatter.o drm_lock.o
> > >  drm-$(CONFIG_DRM_LIB_RANDOM) += lib/drm_random.o
> > > diff --git a/drivers/gpu/drm/drm_bus_timings.c b/drivers/gpu/drm/drm_bus_timings.c
> > > new file mode 100644
> > > index 0000000..e2ecd22
> > > --- /dev/null
> > > +++ b/drivers/gpu/drm/drm_bus_timings.c
> > > @@ -0,0 +1,97 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> 
> DRM core is supposed to be MIT.
> -Daniel
> 
> > > +#include <drm/drm_bus_timings.h>
> > > +#include <linux/errno.h>
> > > +#include <linux/of_graph.h>
> > > +#include <linux/of.h>
> > > +#include <linux/types.h>
> > > +
> > > +#define DRM_OF_LVDS_ODD		1
> > > +#define DRM_OF_LVDS_EVEN	2
> > > +
> > > +static int drm_of_lvds_get_port_pixels_type(struct device_node *port_node)
> > > +{
> > > +	bool even_pixels, odd_pixels;
> > > +
> > > +	even_pixels = of_property_read_bool(port_node, "dual-lvds-even-pixels");
> > > +	odd_pixels = of_property_read_bool(port_node, "dual-lvds-odd-pixels");
> > > +	return  even_pixels * DRM_OF_LVDS_EVEN + odd_pixels * DRM_OF_LVDS_ODD;
> >
> > s/  / /
> >
> > But I would make these bitflags.
> >
> > enum drm_of_lvds_pixels {
> > 	DRM_OF_LVDS_EVEN = BIT(0),
> > 	DRM_OF_LVDS_ODD = BIT(1),
> > };
> >
> > static int drm_of_lvds_get_port_pixels_type(struct device_node *port)
> > {
> > 	bool even_pixels = of_property_read_bool(port, "dual-lvds-even-pixels");
> > 	bool odd_pixels = of_property_read_bool(port, "dual-lvds-odd-pixels");
> >
> > 	return (even_pixels ? DRM_OF_LVDS_EVEN : 0) |
> > 	       (odd_pixels ? DRM_OF_LVDS_ODD : 0);
> > }
> >
> > > +}
> > > +
> > > +/**
> > > + * drm_of_lvds_get_dual_link_configuration - get the dual-LVDS configuration
> >
> > Should we name this drm_of_lvds_get_dual_link_pixel_order to better
> > reflect its purpose ?
> >
> > > + * @p1: device tree node corresponding to the first port of the source
> > > + * @p2: device tree node corresponding to the second port of the source
> >
> > Maybe port1 and port2 to make this more explicit ?
> >
> > > + *
> > > + * An LVDS dual-link bus is made of two connections, even pixels transit on one
> > > + * connection, and odd pixels transit on the other connection.
> >
> > To match the DT bindings documentation, I would recommand
> >
> > "An LVDS dual-link connection is made of two links, with even pixels
> > transitting on one link, and odd pixels on the other link."
> >
> > > + * This function walks the DT (from the source ports to the sink ports) looking
> > > + * for a dual-LVDS bus. A dual-LVDS bus is identfied by markers found on the DT
> > > + * ports of the sink device(s). If such a bus is found, this function returns
> > > + * its configuration (either p1 connected to the even pixels port and p2
> > > + * connected to the odd pixels port, or p1 connected to the odd pixels port and
> > > + * p2 connected to the even pixels port).
> >
> > "walking the DT" sounds like the function goes through the whole graph.
> > How about the following ?
> >
> > /**
> >  * drm_of_lvds_get_dual_link_pixel_order - Get LVDS dual-link pixel order
> >  * @port1: First DT port node of the Dual-link LVDS source
> >  * @port2: Second DT port node of the Dual-link LVDS source
> >  *
> >  * An LVDS dual-link connection is made of two links, with even pixels
> >  * transitting on one link, and odd pixels on the other link. This function
> >  * returns, for two ports of an LVDS dual-link source, which port shall transmit
> >  * the even and off pixels, based on the requirements of the connected sink.
> >  *
> >  * The pixel order is determined from the dual-lvds-even-pixels and
> >  * dual-lvds-odd-pixels properties in the sink's DT port nodes. If those
> >  * properties are not present, or if their usage is not valid, this function
> >  * returns -EINVAL.
> >  *
> >  * @port1 and @port2 are typically DT sibling nodes, but may have different
> >  * parents when, for instance, two separate LVDS encoders carry the even and odd
> >  * pixels.
> >  *
> >  * Return:
> >  * * DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS - @port1 carries even pixels and @port2
> >  *   carries odd pixels
> >  * * DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS - @port1 carries odd pixels and @port1
> >  *   carries even pixels
> >  * * -EINVAL - @port1 and @port2 are not connected to a dual-link LVDS sink, or
> >  *   the sink configuration is invalid
> >  */
> >
> > We could also add -EPIPE as a return code for the case where port1 or
> > port2 are not connected.
> >
> > > + *
> > > + * Return: A code describing the bus configuration when a valid dual-LVDS bus is
> > > + * found, or an error code when no valid dual-LVDS bus is found
> > > + *
> > > + * Possible codes for the bus configuration are:
> > > + *
> > > + * - DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS: when p1 is connected to the even pixels
> > > + *   port and p2 is connected to the odd pixels port
> > > + * - DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS: when p1 is connected to the odd pixels
> > > + *   port and p2 is connected to the even pixels port
> > > + *
> > > + */
> > > +int drm_of_lvds_get_dual_link_configuration(const struct device_node *p1,
> > > +					    const struct device_node *p2)
> > > +{
> > > +	struct device_node *remote_p1 = NULL, *remote_p2 = NULL;
> > > +	struct device_node *parent_p1 = NULL, *parent_p2 = NULL;
> >
> > There's no need to initialize those two variables.
> >
> > > +	struct device_node *ep1 = NULL, *ep2 = NULL;
> > > +	u32 reg_p1, reg_p2;
> > > +	int ret = -EINVAL, remote_p1_pt, remote_p2_pt;
> >
> > Please split this last line, as it otherwise hides the initialization of
> > ret in the middle.
> >
> > > +
> > > +	if (!p1 || !p2)
> > > +		return ret;
> >
> > You can return -EINVAL directly.
> >
> >
> > > +	if (of_property_read_u32(p1, "reg", &reg_p1) ||
> > > +	    of_property_read_u32(p2, "reg", &reg_p2))
> > > +		return ret;
> >
> > Same here.
> >
> > > +	parent_p1 = of_get_parent(p1);
> > > +	parent_p2 = of_get_parent(p2);
> > > +	if (!parent_p1 || !parent_p2)
> > > +		goto done;
> > > +	ep1 = of_graph_get_endpoint_by_regs(parent_p1, reg_p1, 0);
> > > +	ep2 = of_graph_get_endpoint_by_regs(parent_p2, reg_p2, 0);
> > > +	if (!ep1 || !ep2)
> > > +		goto done;
> >
> > If you only support the first endpoint, this should be mentioned in the
> > documentation. Alternatively you could pass the endpoint nodes instead
> > of the port nodes, or you could pass the endpoint number.
> >
> > It's also a bit inefficient to use of_graph_get_endpoint_by_regs() when
> > you already have the port nodes. How about adding the following helper
> > function ?
> >
> > struct device_node *of_graph_get_port_endpoint(struct device_node *port, int reg)
> > {
> > 	struct device_node *endpoint = NULL;
> >
> > 	for_each_child_of_node(port, endpoint) {
> > 		u32 id;
> >
> > 		if (!of_node_name_eq(endpoint, "endpoint") ||
> > 			continue;
> >
> > 		if (reg == -1)
> > 			return endpoint;
> >
> > 		if (of_property_read_u32(node, "reg", &id) < 0)
> > 			continue;
> >
> > 		if (reg == id)
> > 			return endpoint;
> > 	}
> >
> > 	return NULL;
> > }
> >
> > If you're concerned that adding a core helper would delay this patch
> > series, you could add it as a local helper, and move it to of_graph.h in
> > a second step.
> >
> > > +	remote_p1 = of_graph_get_remote_port(ep1);
> > > +	remote_p2 = of_graph_get_remote_port(ep2);
> > > +	if (!remote_p1 || !remote_p2)
> > > +		goto done;
> > > +	remote_p1_pt = drm_of_lvds_get_port_pixels_type(remote_p1);
> > > +	remote_p2_pt = drm_of_lvds_get_port_pixels_type(remote_p2);
> > > +	/*
> > > +	 * A valid dual-lVDS bus is found when one remote port is marked with
> > > +	 * "dual-lvds-even-pixels", and the other remote port is marked with
> > > +	 * "dual-lvds-odd-pixels", bail out if the markers are not right.
> > > +	 */
> > > +	if (!remote_p1_pt || !remote_p2_pt ||
> > > +	    remote_p1_pt + remote_p2_pt != DRM_OF_LVDS_EVEN + DRM_OF_LVDS_ODD)
> > > +		goto done;
> > > +	if (remote_p1_pt == DRM_OF_LVDS_EVEN)
> > > +		/* The sink expects even pixels through the first port */
> > > +		ret = DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS;
> > > +	else
> > > +		/* The sink expects odd pixels through the first port */
> > > +		ret = DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS;
> > > +
> > > +done:
> > > +	of_node_put(ep1);
> > > +	of_node_put(ep2);
> > > +	of_node_put(parent_p1);
> > > +	of_node_put(parent_p2);
> > > +	of_node_put(remote_p1);
> > > +	of_node_put(remote_p2);
> > > +	return ret;
> >
> > This is heavy, I would add blank lines to make the code easier to read.
> >
> > > +}
> > > +EXPORT_SYMBOL_GPL(drm_of_lvds_get_dual_link_configuration);
> > > diff --git a/include/drm/drm_bus_timings.h b/include/drm/drm_bus_timings.h
> > > new file mode 100644
> > > index 0000000..db8a385
> > > --- /dev/null
> > > +++ b/include/drm/drm_bus_timings.h
> > > @@ -0,0 +1,21 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > +#ifndef __DRM_BUS_TIMINGS__
> > > +#define __DRM_BUS_TIMINGS__
> > > +
> > > +struct device_node;
> > > +
> > > +#define DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS	0
> > > +#define DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS	1
> >
> > These should be documented with kerneldoc. How about also turning them
> > into an enum ?
> >
> > > +
> > > +#ifdef CONFIG_OF
> > > +int drm_of_lvds_get_dual_link_configuration(const struct device_node *p1,
> > > +					    const struct device_node *p2);
> > > +#else
> > > +int drm_of_lvds_get_dual_link_configuration(const struct device_node *p1,
> > > +					    const struct device_node *p2)
> > > +{
> > > +	return -EINVAL;
> > > +}
> > > +#endif
> > > +
> > > +#endif /* __DRM_BUS_TIMINGS__ */
> >
> > --
> > Regards,
> >
> > Laurent Pinchart
> 
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch