[Patch 3/4] drm/omap: Add virtual plane DT parsing support

[Tomi Valkeinen]

Hi Benoit,

On 02/03/18 15:48, Benoit Parrot wrote:
> Virtual planes are used to extend display size capability for display
> larger than 2048 pixels by splitting the frame buffer equally between
> two physical planes.
> 
> Here we are adding DT support to parse 'plane' child nodes which
> describe how logical planes are mapped to physical plane(s) and
> which crtc they are available on.
> 
> Signed-off-by: Benoit Parrot <bparrot at ti.com>
> ---
>  drivers/gpu/drm/omapdrm/dss/dispc.c   | 110 ++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/omapdrm/dss/omapdss.h |  11 ++++
>  2 files changed, 121 insertions(+)
> 
> diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c b/drivers/gpu/drm/omapdrm/dss/dispc.c
> index 624dee22f46b..559b70d9762d 100644
> --- a/drivers/gpu/drm/omapdrm/dss/dispc.c
> +++ b/drivers/gpu/drm/omapdrm/dss/dispc.c
> @@ -4334,6 +4334,115 @@ static u32 dispc_get_memory_bandwidth_limit(void)
>  	return limit;
>  }
>  
> +static struct device_node *dispc_of_get_plane_by_id(struct device_node *node,
> +						    u32 id)
> +{
> +	struct device_node *plane;
> +
> +	for_each_child_of_node(node, plane) {
> +		u32 plane_id = 0;
> +
> +		if (of_node_cmp(plane->name, "plane") != 0)
> +			continue;
> +		of_property_read_u32(plane, "reg", &plane_id);
> +		if (id == plane_id)
> +			break;

I think the flow is more readable, if here you "return plane", and at
the end of the func "return NULL".

> +	}
> +
> +	return plane;
> +}
> +
> +static int dispc_parse_dt_plane_data(struct dispc_plane_mappings *plane)

You could rename the parameter to "map", for example. Using 'plane'
there is quite confusing.

> +{
> +	struct platform_device *pdev = dispc.pdev;
> +	struct device_node *np = pdev->dev.of_node;
> +	struct device_node *ep;
> +	struct property *prop;
> +	const __be32 *cur;
> +	u32 v;
> +	u32 num_ovls = dispc_get_num_ovls();
> +	unsigned long int hw_plane_mask = (1 << num_ovls) - 1;

u32?

> +	u32 num_planes;
> +	int i, index;
> +
> +	if (!np)
> +		return 0;
> +
> +	for (i = 0; i < num_ovls; i++) {
> +		ep = dispc_of_get_plane_by_id(np, i);
> +		if (!ep)
> +			break;
> +		if (!of_property_read_bool(ep, "hw-planes")) {
> +			dev_err(&pdev->dev,
> +				"malformed plane node: hw-planes missing.\n");
> +			return -EINVAL;
> +		}
> +
> +		index = 0;
> +		of_property_for_each_u32(ep, "hw-planes", prop, cur, v) {
> +			if (v >= num_ovls) {
> +				dev_err(&pdev->dev,
> +					"hw-planes property: '%d' out-of-range.\n",
> +					v);
> +				return -EINVAL;
> +			}
> +			if (!(hw_plane_mask & BIT_MASK(v))) {
> +				dev_err(&pdev->dev,
> +					"hw-planes property: '%d' used more than once.\n",
> +					v);
> +				return -EINVAL;
> +			}
> +			clear_bit(v, &hw_plane_mask);

Why do you use hw_plane_mask this way? It feels inverse to me, usually
one sets a bit when something is there. And e.g. crtc_mask here is used
the other way.

> +
> +			if (index == 0) {
> +				plane->plane[i].main_id = v;
> +			} else if (index == 1) {
> +				plane->plane[i].aux_id = v;
> +				plane->plane[i].is_virtual = true;
> +			} else if (index > 1) {
> +				dev_err(&pdev->dev,
> +					"hw-planes property: more than 2 values specified.\n");
> +				return -EINVAL;
> +			}
> +			index++;
> +		}
> +
> +		of_property_for_each_u32(ep, "hw-crtcs", prop, cur, v) {
> +			if (v >= num_ovls) {

This should check against num_ovl_mgrs.

> +				dev_err(&pdev->dev,
> +					"hw-crtcs property: '%d' out-of-range.\n",
> +					v);
> +				return -EINVAL;
> +			}
> +			plane->plane[i].crtc_mask |= 1 << v;
> +		}
> +	}
> +
> +	num_planes = i;
> +
> +	if (num_planes) {
> +		dev_dbg(&pdev->dev, "Plane definitions found from DT:");
> +		for (i = 0; i < num_planes; i++) {
> +			if (plane->plane[i].is_virtual) {
> +				dev_dbg(&pdev->dev,
> +					"plane%d: virtual hw-planes: %d, %d crtc_mask: 0x%04x",
> +					i, plane->plane[i].main_id,
> +					plane->plane[i].aux_id,
> +					plane->plane[i].crtc_mask);
> +			} else {
> +				dev_dbg(&pdev->dev,
> +					"plane%d: hw-planes: %d crtc_mask: 0x%04x",
> +					i, plane->plane[i].main_id,
> +					plane->plane[i].crtc_mask);
> +			}
> +		}
> +	}
> +
> +	plane->num_planes = num_planes;
> +
> +	return 0;
> +}
> +
>  /*
>   * Workaround for errata i734 in DSS dispc
>   *  - LCD1 Gamma Correction Is Not Working When GFX Pipe Is Disabled
> @@ -4525,6 +4634,7 @@ static const struct dispc_ops dispc_ops = {
>  	.ovl_enable = dispc_ovl_enable,
>  	.ovl_setup = dispc_ovl_setup,
>  	.ovl_get_color_modes = dispc_ovl_get_color_modes,
> +	.get_plane_mapping = dispc_parse_dt_plane_data,
>  };
>  
>  /* DISPC HW IP initialisation */
> diff --git a/drivers/gpu/drm/omapdrm/dss/omapdss.h b/drivers/gpu/drm/omapdrm/dss/omapdss.h
> index f8f83e826a56..b8c9b30bf5ed 100644
> --- a/drivers/gpu/drm/omapdrm/dss/omapdss.h
> +++ b/drivers/gpu/drm/omapdrm/dss/omapdss.h
> @@ -637,6 +637,16 @@ void omapdss_set_is_initialized(bool set);
>  struct device_node *dss_of_port_get_parent_device(struct device_node *port);
>  u32 dss_of_port_get_port_number(struct device_node *port);
>  
> +struct dispc_plane_mappings {
> +	struct {
> +		u32 main_id;
> +		u32 aux_id;
> +		u32 crtc_mask;
> +		bool	is_virtual;

is_virtual has a tab whereas the others don't.

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki