Skip to contentSkip to conversation

OMAP: DSS2: Framework to handle omap version specific DSS features

PATCH Superseded Archit Taneja 2 participants 11 messages Aug 26, 2010 – Sep 7, 2010

Subsystems (3)
Arm (32-bit) Arm sub-architectures Framebuffer layer

v1 of 4 Superseded Latest is v4 →

lore
Share & tools
lore

Archit Taneja

author
#0 lore raw

cover OMAP: DSS2: Framework to handle omap version specific DSS features

This is a simple approach to prevent scattered cpu_is_omapxxxx checks in DSS2 by bringing all omap version specific DSS features/values to one single place, initialize them and expose a set of functions to DSS2 driver files which return the value/existance of a feature.

This method provides the following:
-A set of functions which give the value of a feature. -Functions which add and return the start and end bits of a register field
which varies across omaps.
-A function which checks if a DSS2 feature exists or not on the present
omap (omap_dss_has_feature)

Whenever a new DSS feature is introduced, its value/existance is specified for all omaps in omap_dss_feature_init().

This patch also fills up a minimal list of features in omap_dss_features_init(). The second patch in the series makes use of dss_features to remove some of the existing omapxxxx checks.

Archit Taneja (2):
OMAP: DSS2: Introduce dss_features files
OMAP: DSS2: Use dss_features framework on DSS2 code

 arch/arm/plat-omap/include/plat/display.h |   31 -----
 drivers/video/omap2/dss/Makefile          |    2 +-
 drivers/video/omap2/dss/core.c            |    5 +
 drivers/video/omap2/dss/dispc.c           |   56 +++++----
 drivers/video/omap2/dss/dss_features.c    |  197 +++++++++++++++++++++++++++++
 drivers/video/omap2/dss/dss_features.h    |   48 +++++++
 drivers/video/omap2/dss/manager.c         |   33 +++---
 drivers/video/omap2/dss/overlay.c         |   24 ++--
 8 files changed, 308 insertions(+), 88 deletions(-)
 create mode 100644 drivers/video/omap2/dss/dss_features.c
 create mode 100644 drivers/video/omap2/dss/dss_features.h
author
#1 lore raw

PATCH 1/2 OMAP: DSS2: Introduce dss_features files

Add dss_features.c and dss_features.h for the dss_features framework

Signed-off-by Archit Taneja <[email protected]>
---
 drivers/video/omap2/dss/dss_features.c |  197 ++++++++++++++++++++++++++++++++
 drivers/video/omap2/dss/dss_features.h |   48 ++++++++
 2 files changed, 245 insertions(+), 0 deletions(-)
 create mode 100644 drivers/video/omap2/dss/dss_features.c
 create mode 100644 drivers/video/omap2/dss/dss_features.h
drivers/video/omap2/dss/dss_features.c new file +197 -0
new file mode 100644
index 0000000..0ac18d2
@@ -0,0 +1,197 @@
+/*
+ * linux/drivers/video/omap2/dss/dss_features.c
+ *
+ * Copyright (C) 2010 Texas Instruments
+ * Author: Archit Taneja <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/kernel.h>
+#include <linux/types.h>
+#include <linux/err.h>
+#include <linux/slab.h>
+
+#include <plat/display.h>
+#include <plat/cpu.h>
+
+#include "dss_features.h"
+
+static struct list_head reg_field_list;
+static int num_reg_fields;
+
+/* Defines a generic omap register field */
+struct dss_reg_field {
+	struct list_head list;
+	int id;
+	u8 start, end;
+};
+
+/* This struct is divided into 2 sets: the first gives a value corresponding
+ * to a feature, the second tells if a dss feature exists or not */
+static struct
+{
+	int num_mgrs;
+	int num_ovls;
+	enum omap_display_type supported_displays[MAX_DSS_MANAGERS];
+	enum omap_color_mode supported_color_modes[MAX_DSS_OVERLAYS];
+
+	bool has_feature[MAX_DSS_FEATURES];
+} omap_dss_features;
+
+/* Functions to add/fetch the start and end bits of a DSS register field */
+static void dss_add_reg_field(int id, u8 start, u8 end)
+{
+	struct dss_reg_field *field;
+	field = kzalloc(sizeof(*field), GFP_KERNEL);
+
+	++num_reg_fields;
+
+	field->id = id;
+	field->start = start;
+	field->end = end;
+
+	list_add_tail(&field->list, &reg_field_list);
+}
+
+void omap_dss_get_reg_field(int id, u8 *start, u8 *end)
+{
+	struct dss_reg_field *field;
+
+	list_for_each_entry(field, &reg_field_list, list) {
+		if (field->id == id) {
+			*start = field->start;
+			*end = field->end;
+			return;
+		}
+	}
+	BUG();
+}
+EXPORT_SYMBOL(omap_dss_get_reg_field);
+
+/* Functions returning values related to a DSS feature */
+int omap_dss_num_mgrs(void)
+{
+	return omap_dss_features.num_mgrs;
+}
+EXPORT_SYMBOL(omap_dss_num_mgrs);
+
+int omap_dss_num_ovls(void)
+{
+	return omap_dss_features.num_ovls;
+}
+EXPORT_SYMBOL(omap_dss_num_ovls);
+
+enum omap_display_type omap_dss_supported_displays(int id)
+{
+	return omap_dss_features.supported_displays[id];
+}
+EXPORT_SYMBOL(omap_dss_supported_displays);
+
+enum omap_color_mode omap_dss_supported_color_modes(int id)
+{
+	return omap_dss_features.supported_color_modes[id];
+}
+EXPORT_SYMBOL(omap_dss_supported_color_modes);
+
+/* DSS has_feature check */
+bool omap_dss_has_feature(int id)
+{
+	return omap_dss_features.has_feature[id];
+}
+EXPORT_SYMBOL(omap_dss_has_feature);
+
+void omap_dss_features_init(void)
+{
+	INIT_LIST_HEAD(&reg_field_list);
+	num_reg_fields = 0;
+
+	omap_dss_features.num_mgrs = 2;
+	omap_dss_features.num_ovls = 3;
+
+	omap_dss_features.supported_displays[OMAP_DSS_CHANNEL_LCD] =
+				OMAP_DISPLAY_TYPE_DPI | OMAP_DISPLAY_TYPE_DBI |
+				OMAP_DISPLAY_TYPE_SDI | OMAP_DISPLAY_TYPE_DSI;
+
+	omap_dss_features.supported_displays[OMAP_DSS_CHANNEL_DIGIT] =
+				OMAP_DISPLAY_TYPE_VENC;
+
+	omap_dss_features.has_feature[GLOBAL_ALPHA_VIDEO1] = false;
+
+	if (cpu_is_omap34xx()) {
+		omap_dss_features.supported_color_modes[OMAP_DSS_GFX] =
+			OMAP_DSS_COLOR_CLUT1 | OMAP_DSS_COLOR_CLUT2 |
+			OMAP_DSS_COLOR_CLUT4 | OMAP_DSS_COLOR_CLUT8 |
+			OMAP_DSS_COLOR_RGB12U | OMAP_DSS_COLOR_ARGB16 |
+			OMAP_DSS_COLOR_RGB16 | OMAP_DSS_COLOR_RGB24U |
+			OMAP_DSS_COLOR_RGB24P | OMAP_DSS_COLOR_ARGB32 |
+			OMAP_DSS_COLOR_RGBA32 | OMAP_DSS_COLOR_RGBX32;
+
+		omap_dss_features.supported_color_modes[OMAP_DSS_VIDEO1] =
+			OMAP_DSS_COLOR_RGB24U | OMAP_DSS_COLOR_RGB24P |
+			OMAP_DSS_COLOR_RGB12U | OMAP_DSS_COLOR_RGB16 |
+			OMAP_DSS_COLOR_YUV2 | OMAP_DSS_COLOR_UYVY;
+
+		omap_dss_features.supported_color_modes[OMAP_DSS_VIDEO2] =
+			OMAP_DSS_COLOR_RGB12U | OMAP_DSS_COLOR_ARGB16 |
+			OMAP_DSS_COLOR_RGB16 | OMAP_DSS_COLOR_RGB24U |
+			OMAP_DSS_COLOR_RGB24P | OMAP_DSS_COLOR_YUV2 |
+			OMAP_DSS_COLOR_UYVY | OMAP_DSS_COLOR_ARGB32 |
+			OMAP_DSS_COLOR_RGBA32 | OMAP_DSS_COLOR_RGBX32;
+
+		omap_dss_features.has_feature[GLOBAL_ALPHA] = true;
+
+		dss_add_reg_field(FIRHINC, 12, 0);
+		dss_add_reg_field(FIRVINC, 28, 16);
+		dss_add_reg_field(FIFOLOWTHRESHOLD, 11, 0);
+		dss_add_reg_field(FIFOHIGHTHRESHOLD, 27, 16);
+		dss_add_reg_field(FIFOSIZE, 10, 0);
+	} else {
+		/* cpu_is_omap24xx() */
+		omap_dss_features.supported_color_modes[OMAP_DSS_GFX] =
+			OMAP_DSS_COLOR_CLUT1 | OMAP_DSS_COLOR_CLUT2 |
+			OMAP_DSS_COLOR_CLUT4 | OMAP_DSS_COLOR_CLUT8 |
+			OMAP_DSS_COLOR_RGB12U | OMAP_DSS_COLOR_RGB16 |
+			OMAP_DSS_COLOR_RGB24U | OMAP_DSS_COLOR_RGB24P;
+
+		omap_dss_features.supported_color_modes[OMAP_DSS_VIDEO1] =
+		omap_dss_features.supported_color_modes[OMAP_DSS_VIDEO2] =
+			OMAP_DSS_COLOR_RGB16 | OMAP_DSS_COLOR_RGB24U |
+			OMAP_DSS_COLOR_RGB24P | OMAP_DSS_COLOR_YUV2 |
+			OMAP_DSS_COLOR_UYVY;
+
+		omap_dss_features.has_feature[GLOBAL_ALPHA] = false;
+
+		dss_add_reg_field(FIRHINC, 11, 0);
+		dss_add_reg_field(FIRVINC, 12, 0);
+		dss_add_reg_field(FIFOLOWTHRESHOLD, 8, 0);
+		dss_add_reg_field(FIFOHIGHTHRESHOLD, 24, 16);
+		dss_add_reg_field(FIFOSIZE, 8, 0);
+	}
+}
+EXPORT_SYMBOL(omap_dss_features_init);
+
+void omap_dss_features_exit(void)
+{
+	/* We only need to free the register field list for now */
+	struct dss_reg_field *field;
+
+	while (!list_empty(&reg_field_list)) {
+		field = list_first_entry(&reg_field_list,
+				struct dss_reg_field, list);
+		list_del(&field->list);
+		kfree(field);
+	}
+	num_reg_fields = 0;
+}
+EXPORT_SYMBOL(omap_dss_features_exit);
drivers/video/omap2/dss/dss_features.h new file +48 -0

48 lines not on this page · raw message

Signature · 1 line
1.7.0.4
#2 lore raw

Re: [PATCH 1/2] OMAP: DSS2: Introduce dss_features files

Hi,

Quoted 3 lines · ext Archit Taneja

Add dss_features.c and dss_features.h for the dss_features framework

Signed-off-by Archit Taneja <[email protected]>

Would a more static approach be cleaner? I mean something like this (pseudo code):

static struct omap_dss_features omap3_dss_features = {
	/* array of register definitions */
	.registers = {
		{
			.register = FIRHINC,
			.high = 12,
			.low = 0,
		},
		{
			.register = FIRVINC,
			.high = 28,
			.low = 16,
		},
		...
	},

	/* array of feature ids */
	.features = {
		GLOBAL_ALPHA,
		GLOBAL_ALPHA_VIDEO1,
		...
	},
};

And then the code would select the omapX_dss_features struct to use depending on the omap version.

Also, the feature/register defines should have some prefix, and perhaps they could be inside an enum?

Tomi

author
#3 lore raw

RE: [PATCH 1/2] OMAP: DSS2: Introduce dss_features files

Hi,

Quoted 34 lines · Tomi Valkeinen

Hi,

Quoted 3 lines · ext Archit Taneja

Add dss_features.c and dss_features.h for the dss_features framework

Signed-off-by Archit Taneja <[email protected]>

Would a more static approach be cleaner? I mean something like this (pseudo code):

static struct omap_dss_features omap3_dss_features = {
	/* array of register definitions */
	.registers = {
		{
			.register = FIRHINC,
			.high = 12,
			.low = 0,
		},
		{
			.register = FIRVINC,
			.high = 28,
			.low = 16,
		},
		...
	},

	/* array of feature ids */
	.features = {
		GLOBAL_ALPHA,
		GLOBAL_ALPHA_VIDEO1,
		...
	},
};
Quoted 2 lines

And then the code would select the omapX_dss_features struct to use depending on the omap version.

It looks way cleaner out here , not sure though what it would look like once the whole file is filled up :)

One issue with having separate omapX_dss_features is something which might may come later on : If there is a feature/value which is same in omap2 and omap3 but different in omap4, we will sill need to fill up all the structs completely.

We can get around this by filling one single struct on runtime :

all omap2 specific features filled up here
{
...
...
}

all common between omap2 and omap3 filled up here
{
...
...
}

all omap3 specific filled up here
{
...
...
}

And so on..

This will save up space, but won't give a very clean look ..

I think by the time all omap4(and 3630) features are added and remaining omap2,3 checks are removed we will have atleast 20 or so register fields and a dozen features. Initializing them statically for all omaps(including the ES_REV's) will really stretch up the file. But it will still look clean :)

What's your call on this?

Archit

Quoted 3 lines

Also, the feature/register defines should have some prefix, and perhaps they could be inside an enum?

Yes we can do this, I'll see while reworking what looks better(enum vs defines)

Archit

Quoted 2 lines

Tomi

#4 lore raw

RE: [PATCH 1/2] OMAP: DSS2: Introduce dss_features files

Quoted 69 lines · Archit

Hi,

Quoted 34 lines · Tomi Valkeinen

Hi,

Quoted 3 lines · ext Archit Taneja

Add dss_features.c and dss_features.h for the dss_features framework

Signed-off-by Archit Taneja <[email protected]>

Would a more static approach be cleaner? I mean something like this (pseudo code):

static struct omap_dss_features omap3_dss_features = {
	/* array of register definitions */
	.registers = {
		{
			.register = FIRHINC,
			.high = 12,
			.low = 0,
		},
		{
			.register = FIRVINC,
			.high = 28,
			.low = 16,
		},
		...
	},

	/* array of feature ids */
	.features = {
		GLOBAL_ALPHA,
		GLOBAL_ALPHA_VIDEO1,
		...
	},
};
Quoted 2 lines

And then the code would select the omapX_dss_features struct to use depending on the omap version.

It looks way cleaner out here , not sure though what it would look like once the whole file is filled up :)

One issue with having separate omapX_dss_features is something which might may come later on : If there is a feature/value which is same in omap2 and omap3 but different in omap4, we will sill need to fill up all the structs completely.

We can get around this by filling one single struct on runtime :

all omap2 specific features filled up here
{
...
...
}

all common between omap2 and omap3 filled up here
{
...
...
}

all omap3 specific filled up here
{
...
...
}

And so on..

Quoted 8 lines

This will save up space, but won't give a very clean look ..

I think by the time all omap4(and 3630) features are added and remaining omap2,3 checks are removed we will have atleast 20 or so register fields and a dozen features. Initializing them statically for all omaps(including the ES_REV's) will really stretch up the file. But it will still look clean :)

What's your call on this?

I'd try with static structs. I agree that if there are lots of features/registers which are the same on multiple omaps, there's redundant code needed. But it just needs to be written once, and having lots of if(omap_xxxx()) lines will be a nightmare to maintain.

Or if things get really long, we could use multiple files, one for each omap.

Also, we can make the definitions a bit shorter by using macros where suitable. For example the registers could be defined with something like FEAT_REG(FIRHINC, 12, 0), which will will the fields properly.

Or there could be a define for feature IDs, which could be appended. Like:

#define FEAT_ID_OMAP2 AAA,\
	BBB,\
	CCC
#define FEAT_ID_OMAP3 FEAT_ID_OMAP2,\
	DDD,\
	EEE,\

But... That's getting a bit nasty, I wouldn't do things like that if not absolutely needed =).

Tomi

author
#5 lore raw

RE: [PATCH 1/2] OMAP: DSS2: Introduce dss_features files

Hi,

Quoted 82 lines · Tomi Valkeinen
Quoted 40 lines · Archit

Hi,

Quoted 34 lines · Tomi Valkeinen

Hi,

Quoted 3 lines · ext Archit Taneja

Add dss_features.c and dss_features.h for the dss_features framework

Signed-off-by Archit Taneja <[email protected]>

Would a more static approach be cleaner? I mean something like this (pseudo code):

static struct omap_dss_features omap3_dss_features = {
	/* array of register definitions */
	.registers = {
		{
			.register = FIRHINC,
			.high = 12,
			.low = 0,
		},
		{
			.register = FIRVINC,
			.high = 28,
			.low = 16,
		},
		...
	},

	/* array of feature ids */
	.features = {
		GLOBAL_ALPHA,
		GLOBAL_ALPHA_VIDEO1,
		...
	},
};
Quoted 2 lines

And then the code would select the omapX_dss_features struct to use depending on the omap version.

Quoted 6 lines

It looks way cleaner out here , not sure though what it would look like once the whole file is filled up :)

One issue with having separate omapX_dss_features is something which might may come later on : If there is a feature/value which is same in omap2 and omap3 but different in omap4, we will sill need

to fill up all the structs completely.

Quoted 20 lines

We can get around this by filling one single struct on runtime :

all omap2 specific features filled up here { ...
...
}

all common between omap2 and omap3 filled up here { ... ... }

all omap3 specific filled up here
{
...
...
}

And so on..

This will save up space, but won't give a very clean look ..

Quoted 7 lines

I think by the time all omap4(and 3630) features are added and remaining omap2,3 checks are removed we will have atleast 20 or so register fields and a dozen features. Initializing them statically for all omaps(including the ES_REV's) will really stretch up the file. But it will still look clean :)

What's your call on this?

I'd try with static structs. I agree that if there are lots of features/registers which are the same on multiple omaps, there's redundant code needed. But it just needs to be written once, and having lots of if(omap_xxxx()) lines will be a nightmare to maintain.

Quoted 7 lines

Or if things get really long, we could use multiple files, one for each omap.

Also, we can make the definitions a bit shorter by using macros where suitable. For example the registers could be defined with something like FEAT_REG(FIRHINC, 12, 0), which will will the fields properly.

Okay, I will try this out.

Archit

Quoted 14 lines

Or there could be a define for feature IDs, which could be appended. Like:

#define FEAT_ID_OMAP2 AAA,\
	BBB,\
	CCC
#define FEAT_ID_OMAP3 FEAT_ID_OMAP2,\
	DDD,\
	EEE,\

But... That's getting a bit nasty, I wouldn't do things like that if not absolutely needed =).

Tomi

#6 lore raw

Re: [PATCH 1/2] OMAP: DSS2: Introduce dss_features files

In reply to Archit Taneja

Hi,

Quoted 3 lines · ext Archit Taneja

Add dss_features.c and dss_features.h for the dss_features framework

Signed-off-by Archit Taneja <[email protected]>

And some more comments inline

Quoted 83 lines
---
 drivers/video/omap2/dss/dss_features.c |  197 ++++++++++++++++++++++++++++++++
 drivers/video/omap2/dss/dss_features.h |   48 ++++++++
 2 files changed, 245 insertions(+), 0 deletions(-)
 create mode 100644 drivers/video/omap2/dss/dss_features.c
 create mode 100644 drivers/video/omap2/dss/dss_features.h
drivers/video/omap2/dss/dss_features.c new file +70 -0
new file mode 100644
index 0000000..0ac18d2
@@ -0,0 +1,197 @@
+/*
+ * linux/drivers/video/omap2/dss/dss_features.c
+ *
+ * Copyright (C) 2010 Texas Instruments
+ * Author: Archit Taneja <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/kernel.h>
+#include <linux/types.h>
+#include <linux/err.h>
+#include <linux/slab.h>
+
+#include <plat/display.h>
+#include <plat/cpu.h>
+
+#include "dss_features.h"
+
+static struct list_head reg_field_list;
+static int num_reg_fields;
+
+/* Defines a generic omap register field */
+struct dss_reg_field {
+	struct list_head list;
+	int id;
+	u8 start, end;
+};
+
+/* This struct is divided into 2 sets: the first gives a value corresponding
+ * to a feature, the second tells if a dss feature exists or not */
+static struct
+{
+	int num_mgrs;
+	int num_ovls;
+	enum omap_display_type supported_displays[MAX_DSS_MANAGERS];
+	enum omap_color_mode supported_color_modes[MAX_DSS_OVERLAYS];
+
+	bool has_feature[MAX_DSS_FEATURES];
+} omap_dss_features;
+
+/* Functions to add/fetch the start and end bits of a DSS register field */
+static void dss_add_reg_field(int id, u8 start, u8 end)
+{
+	struct dss_reg_field *field;
+	field = kzalloc(sizeof(*field), GFP_KERNEL);
+
+	++num_reg_fields;
+
+	field->id = id;
+	field->start = start;
+	field->end = end;
+
+	list_add_tail(&field->list, &reg_field_list);
+}
+
+void omap_dss_get_reg_field(int id, u8 *start, u8 *end)
+{
+	struct dss_reg_field *field;
+

drivers/video/omap2/dss/dss_features.c line 80

Quoted 10 lines
drivers/video/omap2/dss/dss_features.c new file +10 -0
+	list_for_each_entry(field, &reg_field_list, list) {
+		if (field->id == id) {
+			*start = field->start;
+			*end = field->end;
+			return;
+		}
+	}
+	BUG();
+}
+EXPORT_SYMBOL(omap_dss_get_reg_field);

I don't think these need to be exported. The only users for these should be inside DSS driver. And, as you've probably noticed, I've been using (usually) omap_dss_* prefix for exported functions, and (usually) dss_* prefix for non-exported, non-static functions.

Although the naming convention inside DSS is sadly quite inconsistent... =)

drivers/video/omap2/dss/dss_features.c line 87

Quoted 6 lines
drivers/video/omap2/dss/dss_features.c new file +6 -0
+/* Functions returning values related to a DSS feature */
+int omap_dss_num_mgrs(void)
+{
+	return omap_dss_features.num_mgrs;
+}
+EXPORT_SYMBOL(omap_dss_num_mgrs);

Perhaps this should be dss_get_num_mgrs().

drivers/video/omap2/dss/dss_features.c line 93

Quoted 6 lines
drivers/video/omap2/dss/dss_features.c new file +6 -0
+
+int omap_dss_num_ovls(void)
+{
+	return omap_dss_features.num_ovls;
+}
+EXPORT_SYMBOL(omap_dss_num_ovls);

And "get" here too. And for the functions below.

Quoted 8 lines
drivers/video/omap2/dss/dss_features.c new file +8 -0
+enum omap_display_type omap_dss_supported_displays(int id)
+{
+	return omap_dss_features.supported_displays[id];
+}
+EXPORT_SYMBOL(omap_dss_supported_displays);
+
+enum omap_color_mode omap_dss_supported_color_modes(int id)
+{

drivers/video/omap2/dss/dss_features.c line 112

Quoted 10 lines
drivers/video/omap2/dss/dss_features.c new file +10 -0
+	return omap_dss_features.supported_color_modes[id];
+}
+EXPORT_SYMBOL(omap_dss_supported_color_modes);
+
+/* DSS has_feature check */
+bool omap_dss_has_feature(int id)
+{
+	return omap_dss_features.has_feature[id];
+}
+EXPORT_SYMBOL(omap_dss_has_feature);

Tomi

author
#7 lore raw

RE: [PATCH 1/2] OMAP: DSS2: Introduce dss_features files

Hi,

Quoted 98 lines · Tomi Valkeinen

Hi,

Quoted 3 lines · ext Archit Taneja

Add dss_features.c and dss_features.h for the dss_features framework

Signed-off-by Archit Taneja <[email protected]>

And some more comments inline

Quoted 2 lines
---
 drivers/video/omap2/dss/dss_features.c |  197

++++++++++++++++++++++++++++++++

Quoted 73 lines
 drivers/video/omap2/dss/dss_features.h |   48 ++++++++
 2 files changed, 245 insertions(+), 0 deletions(-)  create mode
100644 drivers/video/omap2/dss/dss_features.c
 create mode 100644 drivers/video/omap2/dss/dss_features.h
drivers/video/omap2/dss/dss_features.c new file +11 -0
new file mode 100644
index 0000000..0ac18d2
@@ -0,0 +1,197 @@
+/*
+ * linux/drivers/video/omap2/dss/dss_features.c
+ *
+ * Copyright (C) 2010 Texas Instruments
+ * Author: Archit Taneja <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or +modify it
+ * under the terms of the GNU General Public License version 2 as
+published by + * the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, +but

WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY +or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public +License for + * more details.
+ *
+ * You should have received a copy of the GNU General Public License +along with + * this program. If not, see <http://www.gnu.org/licenses/>. + */

drivers/video/omap2/dss/dss_features.c new file +13 -0
+
+#include <linux/kernel.h>
+#include <linux/types.h>
+#include <linux/err.h>
+#include <linux/slab.h>
+
+#include <plat/display.h>
+#include <plat/cpu.h>
+
+#include "dss_features.h"
+
+static struct list_head reg_field_list; static int num_reg_fields; +
+/* Defines a generic omap register field */ struct dss_reg_field { +	struct

list_head list; + int id;

drivers/video/omap2/dss/dss_features.c new file +5 -0
+	u8 start, end;
+};
+
+/* This struct is divided into 2 sets: the first gives a value
+corresponding + * to a feature, the second tells if a dss feature exists or

not */ +static struct { + int num_mgrs;

drivers/video/omap2/dss/dss_features.c new file +23 -0
+	int num_ovls;
+	enum omap_display_type supported_displays[MAX_DSS_MANAGERS];
+	enum omap_color_mode supported_color_modes[MAX_DSS_OVERLAYS]; +
+	bool has_feature[MAX_DSS_FEATURES];
+} omap_dss_features;
+
+/* Functions to add/fetch the start and end bits of a DSS register
+field */ static void dss_add_reg_field(int id, u8 start, u8 end) {
+	struct dss_reg_field *field;
+	field = kzalloc(sizeof(*field), GFP_KERNEL);
+
+	++num_reg_fields;
+
+	field->id = id;
+	field->start = start;
+	field->end = end;
+
+	list_add_tail(&field->list, &reg_field_list); }
+
+void omap_dss_get_reg_field(int id, u8 *start, u8 *end) {
+	struct dss_reg_field *field;
+
+	list_for_each_entry(field, &reg_field_list, list) { +		if (field->id ==

id) { + *start = field->start;

drivers/video/omap2/dss/dss_features.c line 80

Quoted 7 lines
drivers/video/omap2/dss/dss_features.c new file +7 -0
+			*end = field->end;
+			return;
+		}
+	}
+	BUG();
+}
+EXPORT_SYMBOL(omap_dss_get_reg_field);

I don't think these need to be exported. The only users for
these should be inside DSS driver. And, as you've probably noticed, I've been using (usually) omap_dss_* prefix for exported functions, and (usually) dss_* prefix for non-exported, non-static functions.

Quoted 2 lines

Although the naming convention inside DSS is sadly quite inconsistent... =)

Yes, exporting them is unnecessary, I will correct this.

There are 2 functions in the present code:
omap_dss_get_num_overlays()
omap_dss_get_num_overlay_managers()

They look very similar to the dss_feature functions but are required. I will need to come up with new names for them to avoid confusion.

We could probably give a naming like : dss_feat_get_num_mgrs() etc so that it doesn't confuse things.

Archit

Quoted 34 lines

drivers/video/omap2/dss/dss_features.c line 87

Quoted 5 lines
drivers/video/omap2/dss/dss_features.c new file +4 -0
+/* Functions returning values related to a DSS feature */ int
+omap_dss_num_mgrs(void) { +	return omap_dss_features.num_mgrs;
+}
+EXPORT_SYMBOL(omap_dss_num_mgrs);

Perhaps this should be dss_get_num_mgrs().

drivers/video/omap2/dss/dss_features.c line 93

Quoted 6 lines
drivers/video/omap2/dss/dss_features.c new file +6 -0
+
+int omap_dss_num_ovls(void)
+{
+	return omap_dss_features.num_ovls;
+}
+EXPORT_SYMBOL(omap_dss_num_ovls);

And "get" here too. And for the functions below.

Quoted 5 lines
drivers/video/omap2/dss/dss_features.c new file +5 -0
+enum omap_display_type omap_dss_supported_displays(int id) {
+	return omap_dss_features.supported_displays[id];
+}
+EXPORT_SYMBOL(omap_dss_supported_displays);
+

drivers/video/omap2/dss/dss_features.c line 112

Quoted 10 lines
drivers/video/omap2/dss/dss_features.c new file +10 -0
+enum omap_color_mode omap_dss_supported_color_modes(int id) {
+	return omap_dss_features.supported_color_modes[id]; +}
+EXPORT_SYMBOL(omap_dss_supported_color_modes);
+
+/* DSS has_feature check */
+bool omap_dss_has_feature(int id)
+{
+	return omap_dss_features.has_feature[id];
+}
+EXPORT_SYMBOL(omap_dss_has_feature);

Tomi

#8 lore raw

RE: [PATCH 1/2] OMAP: DSS2: Introduce dss_features files

Quoted 11 lines · Archit

Hi,

There are 2 functions in the present code:
omap_dss_get_num_overlays()
omap_dss_get_num_overlay_managers()

They look very similar to the dss_feature functions but are required. I will need to come up with new names for them to avoid confusion.

We could probably give a naming like : dss_feat_get_num_mgrs() etc so that it doesn't confuse things.

Yes, that sounds good. And the defines/enums could be FEAT_xxx, perhaps? They are DSS internal defines anyway, so DSS_FEAT_xxx may not be needed, but it's good to have some common prefix.

Tomi

author
#9 lore raw

PATCH 2/2 OMAP: DSS2: Use dss_features framework on DSS2 code

In reply to Archit Taneja

init/exit functions of dss_features during dss_probe and remove, and, the following features are made omapxxxx independent:
- number of managers, overlays
- supported color modes for each overlay
- supported displays for each manager
- global aplha, and restriction of global alpha for video1 pipeline
- The register fields : FIRHINC, FIRVINC, FIFOHIGHTHRESHOLD FIFOLOWTHRESHOLD, FIFOSIZE are made omapxxxx independent

Signed-off-by Archit Taneja <[email protected]>
---
 arch/arm/plat-omap/include/plat/display.h |   31 ----------------
 drivers/video/omap2/dss/Makefile          |    2 +-
 drivers/video/omap2/dss/core.c            |    5 +++
 drivers/video/omap2/dss/dispc.c           |   56 ++++++++++++++++-------------
 drivers/video/omap2/dss/manager.c         |   33 ++++++++---------
 drivers/video/omap2/dss/overlay.c         |   24 +++++-------
 6 files changed, 63 insertions(+), 88 deletions(-)
arch/arm/plat-omap/include/plat/display.h +0 -31
index 8bd15bd..c915a66
@@ -81,37 +81,6 @@ enum omap_color_mode {
 	OMAP_DSS_COLOR_ARGB32	= 1 << 11, /* ARGB32 */
 	OMAP_DSS_COLOR_RGBA32	= 1 << 12, /* RGBA32 */
 	OMAP_DSS_COLOR_RGBX32	= 1 << 13, /* RGBx32 */
-
-	OMAP_DSS_COLOR_GFX_OMAP2 =
-		OMAP_DSS_COLOR_CLUT1 | OMAP_DSS_COLOR_CLUT2 |
-		OMAP_DSS_COLOR_CLUT4 | OMAP_DSS_COLOR_CLUT8 |
-		OMAP_DSS_COLOR_RGB12U | OMAP_DSS_COLOR_RGB16 |
-		OMAP_DSS_COLOR_RGB24U | OMAP_DSS_COLOR_RGB24P,
-
-	OMAP_DSS_COLOR_VID_OMAP2 =
-		OMAP_DSS_COLOR_RGB16 | OMAP_DSS_COLOR_RGB24U |
-		OMAP_DSS_COLOR_RGB24P | OMAP_DSS_COLOR_YUV2 |
-		OMAP_DSS_COLOR_UYVY,
-
-	OMAP_DSS_COLOR_GFX_OMAP3 =
-		OMAP_DSS_COLOR_CLUT1 | OMAP_DSS_COLOR_CLUT2 |
-		OMAP_DSS_COLOR_CLUT4 | OMAP_DSS_COLOR_CLUT8 |
-		OMAP_DSS_COLOR_RGB12U | OMAP_DSS_COLOR_ARGB16 |
-		OMAP_DSS_COLOR_RGB16 | OMAP_DSS_COLOR_RGB24U |
-		OMAP_DSS_COLOR_RGB24P | OMAP_DSS_COLOR_ARGB32 |
-		OMAP_DSS_COLOR_RGBA32 | OMAP_DSS_COLOR_RGBX32,
-
-	OMAP_DSS_COLOR_VID1_OMAP3 =
-		OMAP_DSS_COLOR_RGB12U | OMAP_DSS_COLOR_RGB16 |
-		OMAP_DSS_COLOR_RGB24U | OMAP_DSS_COLOR_RGB24P |
-		OMAP_DSS_COLOR_YUV2 | OMAP_DSS_COLOR_UYVY,
-
-	OMAP_DSS_COLOR_VID2_OMAP3 =
-		OMAP_DSS_COLOR_RGB12U | OMAP_DSS_COLOR_ARGB16 |
-		OMAP_DSS_COLOR_RGB16 | OMAP_DSS_COLOR_RGB24U |
-		OMAP_DSS_COLOR_RGB24P | OMAP_DSS_COLOR_YUV2 |
-		OMAP_DSS_COLOR_UYVY | OMAP_DSS_COLOR_ARGB32 |
-		OMAP_DSS_COLOR_RGBA32 | OMAP_DSS_COLOR_RGBX32,
 };
 
 enum omap_lcd_display_type {
drivers/video/omap2/dss/Makefile +1 -1
index d71b5d9..7db17b5
@@ -1,5 +1,5 @@
 obj-$(CONFIG_OMAP2_DSS) += omapdss.o
-omapdss-y := core.o dss.o dispc.o display.o manager.o overlay.o
+omapdss-y := core.o dss.o dss_features.o dispc.o display.o manager.o overlay.o
 omapdss-$(CONFIG_OMAP2_DSS_DPI) += dpi.o
 omapdss-$(CONFIG_OMAP2_DSS_RFBI) += rfbi.o
 omapdss-$(CONFIG_OMAP2_DSS_VENC) += venc.o
drivers/video/omap2/dss/core.c +5 -0
index b3a498f..9fac285
@@ -37,6 +37,7 @@
 #include <plat/clock.h>
 
 #include "dss.h"
+#include "dss_features.h"
 
 static struct {
 	struct platform_device *pdev;

@@ -502,6 +503,8 @@ static int omap_dss_probe(struct platform_device *pdev)
 
 	core.pdev = pdev;
 
+	omap_dss_features_init();
+
 	dss_init_overlay_managers(pdev);
 	dss_init_overlays(pdev);
 

@@ -679,6 +682,8 @@ static int omap_dss_remove(struct platform_device *pdev)
 	dss_uninit_overlays(pdev);
 	dss_uninit_overlay_managers(pdev);
 
+	omap_dss_features_exit();
+
 	for (i = 0; i < pdata->num_devices; ++i)
 		omap_dss_unregister_device(pdata->devices[i]);
drivers/video/omap2/dss/dispc.c +31 -25

126 lines not on this page · raw message

drivers/video/omap2/dss/manager.c +16 -17
index 6a649ab..99add9c
@@ -33,6 +33,7 @@
 #include <plat/cpu.h>
 
 #include "dss.h"
+#include "dss_features.h"
 
 static int num_managers;
 static struct list_head manager_list;

@@ -448,8 +449,8 @@ struct manager_cache_data {
 
 static struct {
 	spinlock_t lock;
-	struct overlay_cache_data overlay_cache[3];
-	struct manager_cache_data manager_cache[2];
+	struct overlay_cache_data overlay_cache[MAX_DSS_OVERLAYS];
+	struct manager_cache_data manager_cache[MAX_DSS_MANAGERS];
 
 	bool irq_enabled;
 } dss_cache;

@@ -882,12 +883,12 @@ static int configure_dispc(void)
 {
 	struct overlay_cache_data *oc;
 	struct manager_cache_data *mc;
-	const int num_ovls = ARRAY_SIZE(dss_cache.overlay_cache);
-	const int num_mgrs = ARRAY_SIZE(dss_cache.manager_cache);
+	const int num_ovls = omap_dss_num_ovls();
+	const int num_mgrs = omap_dss_num_mgrs();
 	int i;
 	int r;
-	bool mgr_busy[2];
-	bool mgr_go[2];
+	bool mgr_busy[MAX_DSS_MANAGERS];
+	bool mgr_go[MAX_DSS_MANAGERS];
 	bool busy;
 
 	r = 0;

@@ -989,7 +990,7 @@ void dss_setup_partial_planes(struct omap_dss_device *dssdev,
 {
 	struct overlay_cache_data *oc;
 	struct manager_cache_data *mc;
-	const int num_ovls = ARRAY_SIZE(dss_cache.overlay_cache);
+	const int num_ovls = omap_dss_num_ovls();
 	struct omap_overlay_manager *mgr;
 	int i;
 	u16 x, y, w, h;

@@ -1121,8 +1122,8 @@ void dss_start_update(struct omap_dss_device *dssdev)
 {
 	struct manager_cache_data *mc;
 	struct overlay_cache_data *oc;
-	const int num_ovls = ARRAY_SIZE(dss_cache.overlay_cache);
-	const int num_mgrs = ARRAY_SIZE(dss_cache.manager_cache);
+	const int num_ovls = omap_dss_num_ovls();
+	const int num_mgrs = omap_dss_num_mgrs();
 	struct omap_overlay_manager *mgr;
 	int i;
 

@@ -1151,10 +1152,10 @@ static void dss_apply_irq_handler(void *data, u32 mask)
 {
 	struct manager_cache_data *mc;
 	struct overlay_cache_data *oc;
-	const int num_ovls = ARRAY_SIZE(dss_cache.overlay_cache);
-	const int num_mgrs = ARRAY_SIZE(dss_cache.manager_cache);
+	const int num_ovls = omap_dss_num_ovls();
+	const int num_mgrs = omap_dss_num_mgrs();
 	int i, r;
-	bool mgr_busy[2];
+	bool mgr_busy[MAX_DSS_MANAGERS];
 
 	mgr_busy[0] = dispc_go_busy(0);
 	mgr_busy[1] = dispc_go_busy(1);

@@ -1461,7 +1462,7 @@ int dss_init_overlay_managers(struct platform_device *pdev)
 
 	num_managers = 0;
 
-	for (i = 0; i < 2; ++i) {
+	for (i = 0; i < omap_dss_num_mgrs(); ++i) {
 		struct omap_overlay_manager *mgr;
 		mgr = kzalloc(sizeof(*mgr), GFP_KERNEL);
 

@@ -1471,14 +1472,10 @@ int dss_init_overlay_managers(struct platform_device *pdev)
 		case 0:
 			mgr->name = "lcd";
 			mgr->id = OMAP_DSS_CHANNEL_LCD;
-			mgr->supported_displays =
-				OMAP_DISPLAY_TYPE_DPI | OMAP_DISPLAY_TYPE_DBI |
-				OMAP_DISPLAY_TYPE_SDI | OMAP_DISPLAY_TYPE_DSI;
 			break;
 		case 1:
 			mgr->name = "tv";
 			mgr->id = OMAP_DSS_CHANNEL_DIGIT;
-			mgr->supported_displays = OMAP_DISPLAY_TYPE_VENC;
 			break;
 		}
 

@@ -1494,6 +1491,8 @@ int dss_init_overlay_managers(struct platform_device *pdev)
 		mgr->disable = &dss_mgr_disable;
 
 		mgr->caps = OMAP_DSS_OVL_MGR_CAP_DISPC;
+		mgr->supported_displays =
+				omap_dss_supported_displays(mgr->id);
 
 		dss_overlay_setup_dispc_manager(mgr);
drivers/video/omap2/dss/overlay.c +10 -14
index 244dca8..072aa49
@@ -35,6 +35,7 @@
 #include <plat/cpu.h>
 
 #include "dss.h"
+#include "dss_features.h"
 
 static int num_overlays;
 static struct list_head overlay_list;

@@ -237,7 +238,8 @@ static ssize_t overlay_global_alpha_store(struct omap_overlay *ovl,
 	/* Video1 plane does not support global alpha
 	 * to always make it 255 completely opaque
 	 */
-	if (ovl->id == OMAP_DSS_VIDEO1)
+	if (!omap_dss_has_feature(GLOBAL_ALPHA_VIDEO1) &&
+			ovl->id == OMAP_DSS_VIDEO1)
 		info.global_alpha = 255;
 	else
 		info.global_alpha = simple_strtoul(buf, NULL, 10);

@@ -510,11 +512,11 @@ static void omap_dss_add_overlay(struct omap_overlay *overlay)
 	list_add_tail(&overlay->list, &overlay_list);
 }
 
-static struct omap_overlay *dispc_overlays[3];
+static struct omap_overlay *dispc_overlays[MAX_DSS_OVERLAYS];
 
 void dss_overlay_setup_dispc_manager(struct omap_overlay_manager *mgr)
 {
-	mgr->num_overlays = 3;
+	mgr->num_overlays = omap_dss_num_ovls();
 	mgr->overlays = dispc_overlays;
 }
 

@@ -535,7 +537,7 @@ void dss_init_overlays(struct platform_device *pdev)
 
 	num_overlays = 0;
 
-	for (i = 0; i < 3; ++i) {
+	for (i = 0; i < omap_dss_num_ovls(); ++i) {
 		struct omap_overlay *ovl;
 		ovl = kzalloc(sizeof(*ovl), GFP_KERNEL);
 

@@ -545,18 +547,12 @@ void dss_init_overlays(struct platform_device *pdev)
 		case 0:
 			ovl->name = "gfx";
 			ovl->id = OMAP_DSS_GFX;
-			ovl->supported_modes = cpu_is_omap34xx() ?
-				OMAP_DSS_COLOR_GFX_OMAP3 :
-				OMAP_DSS_COLOR_GFX_OMAP2;
 			ovl->caps = OMAP_DSS_OVL_CAP_DISPC;
 			ovl->info.global_alpha = 255;
 			break;
 		case 1:
 			ovl->name = "vid1";
 			ovl->id = OMAP_DSS_VIDEO1;
-			ovl->supported_modes = cpu_is_omap34xx() ?
-				OMAP_DSS_COLOR_VID1_OMAP3 :
-				OMAP_DSS_COLOR_VID_OMAP2;
 			ovl->caps = OMAP_DSS_OVL_CAP_SCALE |
 				OMAP_DSS_OVL_CAP_DISPC;
 			ovl->info.global_alpha = 255;

@@ -564,9 +560,6 @@ void dss_init_overlays(struct platform_device *pdev)
 		case 2:
 			ovl->name = "vid2";
 			ovl->id = OMAP_DSS_VIDEO2;
-			ovl->supported_modes = cpu_is_omap34xx() ?
-				OMAP_DSS_COLOR_VID2_OMAP3 :
-				OMAP_DSS_COLOR_VID_OMAP2;
 			ovl->caps = OMAP_DSS_OVL_CAP_SCALE |
 				OMAP_DSS_OVL_CAP_DISPC;
 			ovl->info.global_alpha = 255;

@@ -579,6 +572,9 @@ void dss_init_overlays(struct platform_device *pdev)
 		ovl->get_overlay_info = &dss_ovl_get_overlay_info;
 		ovl->wait_for_go = &dss_ovl_wait_for_go;
 
+		ovl->supported_modes =
+				omap_dss_supported_color_modes(ovl->id);
+
 		omap_dss_add_overlay(ovl);
 
 		r = kobject_init_and_add(&ovl->kobj, &overlay_ktype,

@@ -651,7 +647,7 @@ void dss_recheck_connections(struct omap_dss_device *dssdev, bool force)
 	}
 
 	if (mgr) {
-		for (i = 0; i < 3; i++) {
+		for (i = 0; i < omap_dss_num_ovls(); i++) {
 			struct omap_overlay *ovl;
 			ovl = omap_dss_get_overlay(i);
 			if (!ovl->manager || force) {
Signature · 1 line
1.7.0.4
author
#10 lore raw
In reply to Archit Taneja

Hi,

Could you comment on the dss_feature patches? I can start reworking the floating omap4 patches with this finalized :)

Thanks,

Archit

Quoted 36 lines · Archit

This is a simple approach to prevent scattered cpu_is_omapxxxx checks in DSS2 by bringing all omap version specific DSS features/values to one single place, initialize them and expose a set of functions to DSS2 driver files which return the value/existance of a feature.

This method provides the following:
-A set of functions which give the value of a feature. -Functions which add and return the start and end bits of a register field
which varies across omaps.
-A function which checks if a DSS2 feature exists or not on the present
omap (omap_dss_has_feature)

Whenever a new DSS feature is introduced, its value/existance is specified for all omaps in omap_dss_feature_init().

This patch also fills up a minimal list of features in omap_dss_features_init(). The second patch in the series makes use of dss_features to remove some of the existing omapxxxx checks.

Archit Taneja (2):
OMAP: DSS2: Introduce dss_features files
OMAP: DSS2: Use dss_features framework on DSS2 code

 arch/arm/plat-omap/include/plat/display.h |   31 -----
 drivers/video/omap2/dss/Makefile          |    2 +-
 drivers/video/omap2/dss/core.c            |    5 +
 drivers/video/omap2/dss/dispc.c           |   56 +++++----
 drivers/video/omap2/dss/dss_features.c    |  197
+++++++++++++++++++++++++++++
 drivers/video/omap2/dss/dss_features.h    |   48 +++++++
 drivers/video/omap2/dss/manager.c         |   33 +++---
 drivers/video/omap2/dss/overlay.c         |   24 ++--
 8 files changed, 308 insertions(+), 88 deletions(-)  create
mode 100644 drivers/video/omap2/dss/dss_features.c
 create mode 100644 drivers/video/omap2/dss/dss_features.h