[patch 1/1] drivers/gpu/drm/radeon/atom.c: fix warning
From: Andrew Morton
drivers/gpu/drm/radeon/atom.c: In function 'atom_op_delay':
drivers/gpu/drm/radeon/atom.c:653: warning: comparison is always false due to
limited range of data type
Cc: David Airlie
Cc: Alex Deucher
Cc: Matt Turner
Signed-off-by: Andrew Morton
---
drivers/gpu/drm/radeon/atom.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff -puN
drivers/gpu/drm/radeon/atom.c~drivers-gpu-drm-radeon-atomc-fix-warning
drivers/gpu/drm/radeon/atom.c
--- a/drivers/gpu/drm/radeon/atom.c~drivers-gpu-drm-radeon-atomc-fix-warning
+++ a/drivers/gpu/drm/radeon/atom.c
@@ -647,7 +647,7 @@ static void atom_op_compare(atom_exec_co
static void atom_op_delay(atom_exec_context *ctx, int *ptr, int arg)
{
- uint8_t count = U8((*ptr)++);
+ unsigned count = U8((*ptr)++);
SDEBUG(" count: %d\n", count);
if (arg == ATOM_UNIT_MICROSEC)
udelay(count);
_
[patch 1/1] drivers/gpu/drm/radeon/atom.c: fix warning
From: Andrew Morton
drivers/gpu/drm/radeon/atom.c: In function 'atom_op_delay':
drivers/gpu/drm/radeon/atom.c:653: warning: comparison is always false due to
limited range of data type
Cc: David Airlie
Cc: Alex Deucher
Cc: Matt Turner
Signed-off-by: Andrew Morton
---
drivers/gpu/drm/radeon/atom.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff -puN
drivers/gpu/drm/radeon/atom.c~drivers-gpu-drm-radeon-atomc-fix-warning
drivers/gpu/drm/radeon/atom.c
--- a/drivers/gpu/drm/radeon/atom.c~drivers-gpu-drm-radeon-atomc-fix-warning
+++ a/drivers/gpu/drm/radeon/atom.c
@@ -647,7 +647,7 @@ static void atom_op_compare(atom_exec_co
static void atom_op_delay(atom_exec_context *ctx, int *ptr, int arg)
{
- uint8_t count = U8((*ptr)++);
+ unsigned count = U8((*ptr)++);
SDEBUG(" count: %d\n", count);
if (arg == ATOM_UNIT_MICROSEC)
udelay(count);
_
[patch 2/2] include/linux/vgaarb.h: add missing part of include guard
From: Doug Goldstein vgaarb.h was missing the #define of the #ifndef at the top for the guard to prevent multiple #include's from causing re-define errors Signed-off-by: Doug Goldstein Cc: Dave Airlie Cc: Jesse Barnes Signed-off-by: Andrew Morton --- include/linux/vgaarb.h |1 + 1 file changed, 1 insertion(+) diff -puN include/linux/vgaarb.h~include-linux-vgaarbh-add-missing-part-of-include-guard include/linux/vgaarb.h --- a/include/linux/vgaarb.h~include-linux-vgaarbh-add-missing-part-of-include-guard +++ a/include/linux/vgaarb.h @@ -29,6 +29,7 @@ */ #ifndef LINUX_VGA_H +#define LINUX_VGA_H #include _
[patch 1/2] drivers/gpu/drm/i915: remove duplicate structure field initialization
From: Julia Lawall
In each case, is_mobile is defined twice to 1. Drop one initialization.
The semantic match that finds this problem is as follows:
(http://coccinelle.lip6.fr/)
//
@r@
identifier I, s, fld;
position p0,p;
expression E;
@@
struct I s =@p0 { ... .fld at p = E, ...};
@s@
identifier I, s, r.fld;
position r.p0,p;
expression E;
@@
struct I s =@p0 { ... .fld at p = E, ...};
@script:python@
p0 << r.p0;
fld << r.fld;
ps << s.p;
pr << r.p;
@@
if int(ps[0].line)
Signed-off-by: Julia Lawall
Signed-off-by: Andrew Morton
---
drivers/gpu/drm/i915/i915_drv.c |4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff -puN
drivers/gpu/drm/i915/i915_drv.c~drivers-gpu-drm-i915-remove-duplicate-structure-field-initialization
drivers/gpu/drm/i915/i915_drv.c
---
a/drivers/gpu/drm/i915/i915_drv.c~drivers-gpu-drm-i915-remove-duplicate-structure-field-initialization
+++ a/drivers/gpu/drm/i915/i915_drv.c
@@ -97,7 +97,7 @@ static const struct intel_device_info in
};
static const struct intel_device_info intel_i965gm_info = {
- .is_i965g = 1, .is_mobile = 1, .is_i965gm = 1, .is_i9xx = 1,
+ .is_i965g = 1, .is_i965gm = 1, .is_i9xx = 1,
.is_mobile = 1, .has_fbc = 1, .has_rc6 = 1,
.has_hotplug = 1,
};
@@ -114,7 +114,7 @@ static const struct intel_device_info in
};
static const struct intel_device_info intel_gm45_info = {
- .is_i965g = 1, .is_mobile = 1, .is_g4x = 1, .is_i9xx = 1,
+ .is_i965g = 1, .is_g4x = 1, .is_i9xx = 1,
.is_mobile = 1, .need_gfx_hws = 1, .has_fbc = 1, .has_rc6 = 1,
.has_pipe_cxsr = 1,
.has_hotplug = 1,
_
[patch 2/2] include/linux/vgaarb.h: add missing part of include guard
From: Doug Goldstein vgaarb.h was missing the #define of the #ifndef at the top for the guard to prevent multiple #include's from causing re-define errors Signed-off-by: Doug Goldstein Cc: Dave Airlie Cc: Jesse Barnes Signed-off-by: Andrew Morton --- include/linux/vgaarb.h |1 + 1 file changed, 1 insertion(+) diff -puN include/linux/vgaarb.h~include-linux-vgaarbh-add-missing-part-of-include-guard include/linux/vgaarb.h --- a/include/linux/vgaarb.h~include-linux-vgaarbh-add-missing-part-of-include-guard +++ a/include/linux/vgaarb.h @@ -29,6 +29,7 @@ */ #ifndef LINUX_VGA_H +#define LINUX_VGA_H #include _
[patch 1/2] drivers/gpu/drm/i915: remove duplicate structure field initialization
From: Julia Lawall
In each case, is_mobile is defined twice to 1. Drop one initialization.
The semantic match that finds this problem is as follows:
(http://coccinelle.lip6.fr/)
//
@r@
identifier I, s, fld;
position p0,p;
expression E;
@@
struct I s =@p0 { ... .fld at p = E, ...};
@s@
identifier I, s, r.fld;
position r.p0,p;
expression E;
@@
struct I s =@p0 { ... .fld at p = E, ...};
@script:python@
p0 << r.p0;
fld << r.fld;
ps << s.p;
pr << r.p;
@@
if int(ps[0].line)
Signed-off-by: Julia Lawall
Signed-off-by: Andrew Morton
---
drivers/gpu/drm/i915/i915_drv.c |4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff -puN
drivers/gpu/drm/i915/i915_drv.c~drivers-gpu-drm-i915-remove-duplicate-structure-field-initialization
drivers/gpu/drm/i915/i915_drv.c
---
a/drivers/gpu/drm/i915/i915_drv.c~drivers-gpu-drm-i915-remove-duplicate-structure-field-initialization
+++ a/drivers/gpu/drm/i915/i915_drv.c
@@ -97,7 +97,7 @@ static const struct intel_device_info in
};
static const struct intel_device_info intel_i965gm_info = {
- .is_i965g = 1, .is_mobile = 1, .is_i965gm = 1, .is_i9xx = 1,
+ .is_i965g = 1, .is_i965gm = 1, .is_i9xx = 1,
.is_mobile = 1, .has_fbc = 1, .has_rc6 = 1,
.has_hotplug = 1,
};
@@ -114,7 +114,7 @@ static const struct intel_device_info in
};
static const struct intel_device_info intel_gm45_info = {
- .is_i965g = 1, .is_mobile = 1, .is_g4x = 1, .is_i9xx = 1,
+ .is_i965g = 1, .is_g4x = 1, .is_i9xx = 1,
.is_mobile = 1, .need_gfx_hws = 1, .has_fbc = 1, .has_rc6 = 1,
.has_pipe_cxsr = 1,
.has_hotplug = 1,
_
[patch 1/1] drivers/gpu/drm/radeon/atom.c: fix warning
From: Andrew Morton
drivers/gpu/drm/radeon/atom.c: In function 'atom_op_delay':
drivers/gpu/drm/radeon/atom.c:653: warning: comparison is always false due to
limited range of data type
Cc: David Airlie
Cc: Alex Deucher
Cc: Matt Turner
Signed-off-by: Andrew Morton
---
drivers/gpu/drm/radeon/atom.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff -puN
drivers/gpu/drm/radeon/atom.c~drivers-gpu-drm-radeon-atomc-fix-warning
drivers/gpu/drm/radeon/atom.c
--- a/drivers/gpu/drm/radeon/atom.c~drivers-gpu-drm-radeon-atomc-fix-warning
+++ a/drivers/gpu/drm/radeon/atom.c
@@ -647,7 +647,7 @@ static void atom_op_compare(atom_exec_co
static void atom_op_delay(atom_exec_context *ctx, int *ptr, int arg)
{
- uint8_t count = U8((*ptr)++);
+ unsigned count = U8((*ptr)++);
SDEBUG(" count: %d\n", count);
if (arg == ATOM_UNIT_MICROSEC)
udelay(count);
_
[patch 1/1] drivers/gpu/drm/radeon/atom.c: fix warning
From: Andrew Morton
drivers/gpu/drm/radeon/atom.c: In function 'atom_op_delay':
drivers/gpu/drm/radeon/atom.c:653: warning: comparison is always false due to
limited range of data type
Cc: David Airlie
Cc: Alex Deucher
Cc: Matt Turner
Signed-off-by: Andrew Morton
---
drivers/gpu/drm/radeon/atom.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff -puN
drivers/gpu/drm/radeon/atom.c~drivers-gpu-drm-radeon-atomc-fix-warning
drivers/gpu/drm/radeon/atom.c
--- a/drivers/gpu/drm/radeon/atom.c~drivers-gpu-drm-radeon-atomc-fix-warning
+++ a/drivers/gpu/drm/radeon/atom.c
@@ -647,7 +647,7 @@ static void atom_op_compare(atom_exec_co
static void atom_op_delay(atom_exec_context *ctx, int *ptr, int arg)
{
- uint8_t count = U8((*ptr)++);
+ unsigned count = U8((*ptr)++);
SDEBUG(" count: %d\n", count);
if (arg == ATOM_UNIT_MICROSEC)
udelay(count);
_
[patch 2/2] include/linux/vgaarb.h: add missing part of include guard
From: Doug Goldstein vgaarb.h was missing the #define of the #ifndef at the top for the guard to prevent multiple #include's from causing re-define errors Signed-off-by: Doug Goldstein Cc: Dave Airlie Cc: Jesse Barnes Signed-off-by: Andrew Morton --- include/linux/vgaarb.h |1 + 1 file changed, 1 insertion(+) diff -puN include/linux/vgaarb.h~include-linux-vgaarbh-add-missing-part-of-include-guard include/linux/vgaarb.h --- a/include/linux/vgaarb.h~include-linux-vgaarbh-add-missing-part-of-include-guard +++ a/include/linux/vgaarb.h @@ -29,6 +29,7 @@ */ #ifndef LINUX_VGA_H +#define LINUX_VGA_H #include _
[patch 1/2] drivers/gpu/drm/i915: remove duplicate structure field initialization
From: Julia Lawall
In each case, is_mobile is defined twice to 1. Drop one initialization.
The semantic match that finds this problem is as follows:
(http://coccinelle.lip6.fr/)
//
@r@
identifier I, s, fld;
position p0,p;
expression E;
@@
struct I s =@p0 { ... .fld at p = E, ...};
@s@
identifier I, s, r.fld;
position r.p0,p;
expression E;
@@
struct I s =@p0 { ... .fld at p = E, ...};
@script:python@
p0 << r.p0;
fld << r.fld;
ps << s.p;
pr << r.p;
@@
if int(ps[0].line)
Signed-off-by: Julia Lawall
Signed-off-by: Andrew Morton
---
drivers/gpu/drm/i915/i915_drv.c |4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff -puN
drivers/gpu/drm/i915/i915_drv.c~drivers-gpu-drm-i915-remove-duplicate-structure-field-initialization
drivers/gpu/drm/i915/i915_drv.c
---
a/drivers/gpu/drm/i915/i915_drv.c~drivers-gpu-drm-i915-remove-duplicate-structure-field-initialization
+++ a/drivers/gpu/drm/i915/i915_drv.c
@@ -97,7 +97,7 @@ static const struct intel_device_info in
};
static const struct intel_device_info intel_i965gm_info = {
- .is_i965g = 1, .is_mobile = 1, .is_i965gm = 1, .is_i9xx = 1,
+ .is_i965g = 1, .is_i965gm = 1, .is_i9xx = 1,
.is_mobile = 1, .has_fbc = 1, .has_rc6 = 1,
.has_hotplug = 1,
};
@@ -114,7 +114,7 @@ static const struct intel_device_info in
};
static const struct intel_device_info intel_gm45_info = {
- .is_i965g = 1, .is_mobile = 1, .is_g4x = 1, .is_i9xx = 1,
+ .is_i965g = 1, .is_g4x = 1, .is_i9xx = 1,
.is_mobile = 1, .need_gfx_hws = 1, .has_fbc = 1, .has_rc6 = 1,
.has_pipe_cxsr = 1,
.has_hotplug = 1,
_
[patch 1/1] drivers/gpu/drm/radeon/atom.c: fix warning
From: Andrew Morton
drivers/gpu/drm/radeon/atom.c: In function 'atom_op_delay':
drivers/gpu/drm/radeon/atom.c:653: warning: comparison is always false due to
limited range of data type
Cc: David Airlie
Cc: Alex Deucher
Cc: Matt Turner
Signed-off-by: Andrew Morton
---
drivers/gpu/drm/radeon/atom.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff -puN
drivers/gpu/drm/radeon/atom.c~drivers-gpu-drm-radeon-atomc-fix-warning
drivers/gpu/drm/radeon/atom.c
--- a/drivers/gpu/drm/radeon/atom.c~drivers-gpu-drm-radeon-atomc-fix-warning
+++ a/drivers/gpu/drm/radeon/atom.c
@@ -647,7 +647,7 @@ static void atom_op_compare(atom_exec_co
static void atom_op_delay(atom_exec_context *ctx, int *ptr, int arg)
{
- uint8_t count = U8((*ptr)++);
+ unsigned count = U8((*ptr)++);
SDEBUG(" count: %d\n", count);
if (arg == ATOM_UNIT_MICROSEC)
udelay(count);
_
[patch 1/1] drivers/gpu/drm/radeon/atom.c: fix warning
From: Andrew Morton
drivers/gpu/drm/radeon/atom.c: In function 'atom_op_delay':
drivers/gpu/drm/radeon/atom.c:653: warning: comparison is always false due to
limited range of data type
Cc: David Airlie
Cc: Alex Deucher
Cc: Matt Turner
Signed-off-by: Andrew Morton
---
drivers/gpu/drm/radeon/atom.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff -puN
drivers/gpu/drm/radeon/atom.c~drivers-gpu-drm-radeon-atomc-fix-warning
drivers/gpu/drm/radeon/atom.c
--- a/drivers/gpu/drm/radeon/atom.c~drivers-gpu-drm-radeon-atomc-fix-warning
+++ a/drivers/gpu/drm/radeon/atom.c
@@ -647,7 +647,7 @@ static void atom_op_compare(atom_exec_co
static void atom_op_delay(atom_exec_context *ctx, int *ptr, int arg)
{
- uint8_t count = U8((*ptr)++);
+ unsigned count = U8((*ptr)++);
SDEBUG(" count: %d\n", count);
if (arg == ATOM_UNIT_MICROSEC)
udelay(count);
_
[patch 1/1] drivers/gpu/drm/radeon/atom.c: fix warning
From: Andrew Morton
drivers/gpu/drm/radeon/atom.c: In function 'atom_op_delay':
drivers/gpu/drm/radeon/atom.c:653: warning: comparison is always false due to
limited range of data type
Cc: David Airlie
Cc: Alex Deucher
Cc: Matt Turner
Signed-off-by: Andrew Morton
---
drivers/gpu/drm/radeon/atom.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff -puN
drivers/gpu/drm/radeon/atom.c~drivers-gpu-drm-radeon-atomc-fix-warning
drivers/gpu/drm/radeon/atom.c
--- a/drivers/gpu/drm/radeon/atom.c~drivers-gpu-drm-radeon-atomc-fix-warning
+++ a/drivers/gpu/drm/radeon/atom.c
@@ -647,7 +647,7 @@ static void atom_op_compare(atom_exec_co
static void atom_op_delay(atom_exec_context *ctx, int *ptr, int arg)
{
- uint8_t count = U8((*ptr)++);
+ unsigned count = U8((*ptr)++);
SDEBUG(" count: %d\n", count);
if (arg == ATOM_UNIT_MICROSEC)
udelay(count);
_
[patch 1/1] drivers/gpu/drm/radeon/atom.c: fix warning
From: Andrew Morton
drivers/gpu/drm/radeon/atom.c: In function 'atom_op_delay':
drivers/gpu/drm/radeon/atom.c:653: warning: comparison is always false due to
limited range of data type
Cc: David Airlie
Cc: Alex Deucher
Cc: Matt Turner
Signed-off-by: Andrew Morton
---
drivers/gpu/drm/radeon/atom.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff -puN
drivers/gpu/drm/radeon/atom.c~drivers-gpu-drm-radeon-atomc-fix-warning
drivers/gpu/drm/radeon/atom.c
--- a/drivers/gpu/drm/radeon/atom.c~drivers-gpu-drm-radeon-atomc-fix-warning
+++ a/drivers/gpu/drm/radeon/atom.c
@@ -647,7 +647,7 @@ static void atom_op_compare(atom_exec_co
static void atom_op_delay(atom_exec_context *ctx, int *ptr, int arg)
{
- uint8_t count = U8((*ptr)++);
+ unsigned count = U8((*ptr)++);
SDEBUG(" count: %d\n", count);
if (arg == ATOM_UNIT_MICROSEC)
udelay(count);
_
[patch 1/1] drivers/gpu/drm/radeon/atom.c: fix warning
From: Andrew Morton
drivers/gpu/drm/radeon/atom.c: In function 'atom_op_delay':
drivers/gpu/drm/radeon/atom.c:653: warning: comparison is always false due to
limited range of data type
Cc: David Airlie
Cc: Alex Deucher
Cc: Matt Turner
Signed-off-by: Andrew Morton
---
drivers/gpu/drm/radeon/atom.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff -puN
drivers/gpu/drm/radeon/atom.c~drivers-gpu-drm-radeon-atomc-fix-warning
drivers/gpu/drm/radeon/atom.c
--- a/drivers/gpu/drm/radeon/atom.c~drivers-gpu-drm-radeon-atomc-fix-warning
+++ a/drivers/gpu/drm/radeon/atom.c
@@ -647,7 +647,7 @@ static void atom_op_compare(atom_exec_co
static void atom_op_delay(atom_exec_context *ctx, int *ptr, int arg)
{
- uint8_t count = U8((*ptr)++);
+ unsigned count = U8((*ptr)++);
SDEBUG(" count: %d\n", count);
if (arg == ATOM_UNIT_MICROSEC)
udelay(count);
_
[patch 1/1] drivers/gpu/drm/radeon/atom.c: fix warning
From: Andrew Morton
drivers/gpu/drm/radeon/atom.c: In function 'atom_op_delay':
drivers/gpu/drm/radeon/atom.c:653: warning: comparison is always false due to
limited range of data type
Cc: David Airlie
Cc: Alex Deucher
Cc: Matt Turner
Signed-off-by: Andrew Morton
---
drivers/gpu/drm/radeon/atom.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff -puN
drivers/gpu/drm/radeon/atom.c~drivers-gpu-drm-radeon-atomc-fix-warning
drivers/gpu/drm/radeon/atom.c
--- a/drivers/gpu/drm/radeon/atom.c~drivers-gpu-drm-radeon-atomc-fix-warning
+++ a/drivers/gpu/drm/radeon/atom.c
@@ -647,7 +647,7 @@ static void atom_op_compare(atom_exec_co
static void atom_op_delay(atom_exec_context *ctx, int *ptr, int arg)
{
- uint8_t count = U8((*ptr)++);
+ unsigned count = U8((*ptr)++);
SDEBUG(" count: %d\n", count);
if (arg == ATOM_UNIT_MICROSEC)
udelay(count);
_
[patch 2/2] include/linux/vgaarb.h: add missing part of include guard
From: Doug Goldstein vgaarb.h was missing the #define of the #ifndef at the top for the guard to prevent multiple #include's from causing re-define errors Signed-off-by: Doug Goldstein Cc: Dave Airlie Cc: Jesse Barnes Signed-off-by: Andrew Morton --- include/linux/vgaarb.h |1 + 1 file changed, 1 insertion(+) diff -puN include/linux/vgaarb.h~include-linux-vgaarbh-add-missing-part-of-include-guard include/linux/vgaarb.h --- a/include/linux/vgaarb.h~include-linux-vgaarbh-add-missing-part-of-include-guard +++ a/include/linux/vgaarb.h @@ -29,6 +29,7 @@ */ #ifndef LINUX_VGA_H +#define LINUX_VGA_H #include _
[patch 1/2] drivers/gpu/drm/i915: remove duplicate structure field initialization
From: Julia Lawall
In each case, is_mobile is defined twice to 1. Drop one initialization.
The semantic match that finds this problem is as follows:
(http://coccinelle.lip6.fr/)
//
@r@
identifier I, s, fld;
position p0,p;
expression E;
@@
struct I s =@p0 { ... .fld at p = E, ...};
@s@
identifier I, s, r.fld;
position r.p0,p;
expression E;
@@
struct I s =@p0 { ... .fld at p = E, ...};
@script:python@
p0 << r.p0;
fld << r.fld;
ps << s.p;
pr << r.p;
@@
if int(ps[0].line)
Signed-off-by: Julia Lawall
Signed-off-by: Andrew Morton
---
drivers/gpu/drm/i915/i915_drv.c |4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff -puN
drivers/gpu/drm/i915/i915_drv.c~drivers-gpu-drm-i915-remove-duplicate-structure-field-initialization
drivers/gpu/drm/i915/i915_drv.c
---
a/drivers/gpu/drm/i915/i915_drv.c~drivers-gpu-drm-i915-remove-duplicate-structure-field-initialization
+++ a/drivers/gpu/drm/i915/i915_drv.c
@@ -97,7 +97,7 @@ static const struct intel_device_info in
};
static const struct intel_device_info intel_i965gm_info = {
- .is_i965g = 1, .is_mobile = 1, .is_i965gm = 1, .is_i9xx = 1,
+ .is_i965g = 1, .is_i965gm = 1, .is_i9xx = 1,
.is_mobile = 1, .has_fbc = 1, .has_rc6 = 1,
.has_hotplug = 1,
};
@@ -114,7 +114,7 @@ static const struct intel_device_info in
};
static const struct intel_device_info intel_gm45_info = {
- .is_i965g = 1, .is_mobile = 1, .is_g4x = 1, .is_i9xx = 1,
+ .is_i965g = 1, .is_g4x = 1, .is_i9xx = 1,
.is_mobile = 1, .need_gfx_hws = 1, .has_fbc = 1, .has_rc6 = 1,
.has_pipe_cxsr = 1,
.has_hotplug = 1,
_
[patch 2/2] include/linux/vgaarb.h: add missing part of include guard
From: Doug Goldstein vgaarb.h was missing the #define of the #ifndef at the top for the guard to prevent multiple #include's from causing re-define errors Signed-off-by: Doug Goldstein Cc: Dave Airlie Cc: Jesse Barnes Signed-off-by: Andrew Morton --- include/linux/vgaarb.h |1 + 1 file changed, 1 insertion(+) diff -puN include/linux/vgaarb.h~include-linux-vgaarbh-add-missing-part-of-include-guard include/linux/vgaarb.h --- a/include/linux/vgaarb.h~include-linux-vgaarbh-add-missing-part-of-include-guard +++ a/include/linux/vgaarb.h @@ -29,6 +29,7 @@ */ #ifndef LINUX_VGA_H +#define LINUX_VGA_H #include _
[patch 1/2] drivers/gpu/drm/i915: remove duplicate structure field initialization
From: Julia Lawall
In each case, is_mobile is defined twice to 1. Drop one initialization.
The semantic match that finds this problem is as follows:
(http://coccinelle.lip6.fr/)
//
@r@
identifier I, s, fld;
position p0,p;
expression E;
@@
struct I s =@p0 { ... .fld at p = E, ...};
@s@
identifier I, s, r.fld;
position r.p0,p;
expression E;
@@
struct I s =@p0 { ... .fld at p = E, ...};
@script:python@
p0 << r.p0;
fld << r.fld;
ps << s.p;
pr << r.p;
@@
if int(ps[0].line)
Signed-off-by: Julia Lawall
Signed-off-by: Andrew Morton
---
drivers/gpu/drm/i915/i915_drv.c |4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff -puN
drivers/gpu/drm/i915/i915_drv.c~drivers-gpu-drm-i915-remove-duplicate-structure-field-initialization
drivers/gpu/drm/i915/i915_drv.c
---
a/drivers/gpu/drm/i915/i915_drv.c~drivers-gpu-drm-i915-remove-duplicate-structure-field-initialization
+++ a/drivers/gpu/drm/i915/i915_drv.c
@@ -97,7 +97,7 @@ static const struct intel_device_info in
};
static const struct intel_device_info intel_i965gm_info = {
- .is_i965g = 1, .is_mobile = 1, .is_i965gm = 1, .is_i9xx = 1,
+ .is_i965g = 1, .is_i965gm = 1, .is_i9xx = 1,
.is_mobile = 1, .has_fbc = 1, .has_rc6 = 1,
.has_hotplug = 1,
};
@@ -114,7 +114,7 @@ static const struct intel_device_info in
};
static const struct intel_device_info intel_gm45_info = {
- .is_i965g = 1, .is_mobile = 1, .is_g4x = 1, .is_i9xx = 1,
+ .is_i965g = 1, .is_g4x = 1, .is_i9xx = 1,
.is_mobile = 1, .need_gfx_hws = 1, .has_fbc = 1, .has_rc6 = 1,
.has_pipe_cxsr = 1,
.has_hotplug = 1,
_
[patch 1/1] drivers/gpu/drm/radeon/atom.c: fix warning
From: Andrew Morton
drivers/gpu/drm/radeon/atom.c: In function 'atom_op_delay':
drivers/gpu/drm/radeon/atom.c:653: warning: comparison is always false due to
limited range of data type
Cc: David Airlie
Cc: Alex Deucher
Cc: Matt Turner
Signed-off-by: Andrew Morton
---
drivers/gpu/drm/radeon/atom.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff -puN
drivers/gpu/drm/radeon/atom.c~drivers-gpu-drm-radeon-atomc-fix-warning
drivers/gpu/drm/radeon/atom.c
--- a/drivers/gpu/drm/radeon/atom.c~drivers-gpu-drm-radeon-atomc-fix-warning
+++ a/drivers/gpu/drm/radeon/atom.c
@@ -647,7 +647,7 @@ static void atom_op_compare(atom_exec_co
static void atom_op_delay(atom_exec_context *ctx, int *ptr, int arg)
{
- uint8_t count = U8((*ptr)++);
+ unsigned count = U8((*ptr)++);
SDEBUG(" count: %d\n", count);
if (arg == ATOM_UNIT_MICROSEC)
udelay(count);
_
[patch 1/1] drivers/gpu/drm/radeon/atom.c: fix warning
From: Andrew Morton
drivers/gpu/drm/radeon/atom.c: In function 'atom_op_delay':
drivers/gpu/drm/radeon/atom.c:653: warning: comparison is always false due to
limited range of data type
Cc: David Airlie
Cc: Alex Deucher
Cc: Matt Turner
Signed-off-by: Andrew Morton
---
drivers/gpu/drm/radeon/atom.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff -puN
drivers/gpu/drm/radeon/atom.c~drivers-gpu-drm-radeon-atomc-fix-warning
drivers/gpu/drm/radeon/atom.c
--- a/drivers/gpu/drm/radeon/atom.c~drivers-gpu-drm-radeon-atomc-fix-warning
+++ a/drivers/gpu/drm/radeon/atom.c
@@ -647,7 +647,7 @@ static void atom_op_compare(atom_exec_co
static void atom_op_delay(atom_exec_context *ctx, int *ptr, int arg)
{
- uint8_t count = U8((*ptr)++);
+ unsigned count = U8((*ptr)++);
SDEBUG(" count: %d\n", count);
if (arg == ATOM_UNIT_MICROSEC)
udelay(count);
_
[patch 2/2] include/linux/vgaarb.h: add missing part of include guard
From: Doug Goldstein vgaarb.h was missing the #define of the #ifndef at the top for the guard to prevent multiple #include's from causing re-define errors Signed-off-by: Doug Goldstein Cc: Dave Airlie Cc: Jesse Barnes Signed-off-by: Andrew Morton --- include/linux/vgaarb.h |1 + 1 file changed, 1 insertion(+) diff -puN include/linux/vgaarb.h~include-linux-vgaarbh-add-missing-part-of-include-guard include/linux/vgaarb.h --- a/include/linux/vgaarb.h~include-linux-vgaarbh-add-missing-part-of-include-guard +++ a/include/linux/vgaarb.h @@ -29,6 +29,7 @@ */ #ifndef LINUX_VGA_H +#define LINUX_VGA_H #include _
[patch 1/2] drivers/gpu/drm/i915: remove duplicate structure field initialization
From: Julia Lawall
In each case, is_mobile is defined twice to 1. Drop one initialization.
The semantic match that finds this problem is as follows:
(http://coccinelle.lip6.fr/)
//
@r@
identifier I, s, fld;
position p0,p;
expression E;
@@
struct I s =@p0 { ... .fld at p = E, ...};
@s@
identifier I, s, r.fld;
position r.p0,p;
expression E;
@@
struct I s =@p0 { ... .fld at p = E, ...};
@script:python@
p0 << r.p0;
fld << r.fld;
ps << s.p;
pr << r.p;
@@
if int(ps[0].line)
Signed-off-by: Julia Lawall
Signed-off-by: Andrew Morton
---
drivers/gpu/drm/i915/i915_drv.c |4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff -puN
drivers/gpu/drm/i915/i915_drv.c~drivers-gpu-drm-i915-remove-duplicate-structure-field-initialization
drivers/gpu/drm/i915/i915_drv.c
---
a/drivers/gpu/drm/i915/i915_drv.c~drivers-gpu-drm-i915-remove-duplicate-structure-field-initialization
+++ a/drivers/gpu/drm/i915/i915_drv.c
@@ -97,7 +97,7 @@ static const struct intel_device_info in
};
static const struct intel_device_info intel_i965gm_info = {
- .is_i965g = 1, .is_mobile = 1, .is_i965gm = 1, .is_i9xx = 1,
+ .is_i965g = 1, .is_i965gm = 1, .is_i9xx = 1,
.is_mobile = 1, .has_fbc = 1, .has_rc6 = 1,
.has_hotplug = 1,
};
@@ -114,7 +114,7 @@ static const struct intel_device_info in
};
static const struct intel_device_info intel_gm45_info = {
- .is_i965g = 1, .is_mobile = 1, .is_g4x = 1, .is_i9xx = 1,
+ .is_i965g = 1, .is_g4x = 1, .is_i9xx = 1,
.is_mobile = 1, .need_gfx_hws = 1, .has_fbc = 1, .has_rc6 = 1,
.has_pipe_cxsr = 1,
.has_hotplug = 1,
_
Re: [PATCH] ARM: add get_user() support for 8 byte types
On Fri, Nov 09, 2012 at 03:17:33PM -0600, Rob Clark wrote:
> From: Rob Clark
>
> A new atomic modeset/pageflip ioctl being developed in DRM requires
> get_user() to work for 64bit types (in addition to just put_user()).
NAK.
(I did write a better email explaining all the ins and outs of why this
won't work and why 64-bit get_user isn't possible, but my editor crapped
out and lost all that well written message; I don't fancy typing it all
out again.)
Nevertheless,
int test_ptr(unsigned int **v, unsigned int **p)
{
return get_user(*v, p);
}
produces a warning, and you can't get away from that if you stick 64-bit
support into get_user().
Sorry, 64-bit get_user() is a no-no.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] ARM: add get_user() support for 8 byte types
On Mon, Nov 12, 2012 at 01:58:32PM -0600, Rob Clark wrote:
> On Mon, Nov 12, 2012 at 1:27 PM, Russell King - ARM Linux
> wrote:
> > On Fri, Nov 09, 2012 at 03:17:33PM -0600, Rob Clark wrote:
> >> From: Rob Clark
> >>
> >> A new atomic modeset/pageflip ioctl being developed in DRM requires
> >> get_user() to work for 64bit types (in addition to just put_user()).
> >
> > NAK.
> >
> > (I did write a better email explaining all the ins and outs of why this
> > won't work and why 64-bit get_user isn't possible, but my editor crapped
> > out and lost all that well written message; I don't fancy typing it all
> > out again.)
> >
> > Nevertheless,
> > int test_ptr(unsigned int **v, unsigned int **p)
> > {
> > return get_user(*v, p);
> > }
> >
> > produces a warning, and you can't get away from that if you stick 64-bit
> > support into get_user().
>
> Actually, it seems like using 'register typeof(x) __r2 asm("r2");'
> does avoid that warning..
That seems to pass the checks I've done on it so far, and seems rather
obvious (there's been a number of people looking at this, none of whom
have come up with that solution). Provided the final cast is kept
(which is there to ensure proper typechecking), it seems like it might
be a solution.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] ARM: add get_user() support for 8 byte types
On Mon, Nov 12, 2012 at 05:33:41PM -0600, Rob Clark wrote:
> I'm sort of thinking maybe we want to change 'switch (sizeof(*(__p)))'
> with 'switch (sizeof(typeof(x)))' in case someone ignores the compiler
> warning when they try something like:
Definitely not. Ttype of access is controlled by the pointer, not by the
size of what it's being assigned to. Switching that around is likely to
break stuff hugely. Consider this:
unsigned char __user *p;
int val;
get_user(val, p);
If the pointer type is used to determine the access size, a char will be
accessed. This is legal - because we end up assigning an unsigned character
to an int.
If the size of the destination was used, we'd access an int instead, which
is larger than the pointer, and probably the wrong thing to do anyway.
Think of get_user(a, b) as being a special accessor having the ultimate
semantics of: "a = *b;" but done in a safe way with error checking.
>uint32_t x;
>uint64_t *p = ...;
>get_user(x, p);
>
> that was my one concern about 'register typeof(x) __r2 ...', but I
> think just changing the switch condition is enough. But maybe good to
> have some eyes on in case there is something else I'm not thinking of.
And what should happen in the above is exactly the same as what happens
if you do:
x = *p;
with those types. For ARM, that would be a 64-bit access (if the
compiler decides not to optimize away the upper 32-bit access) followed
by a narrowing cast down to 32-bit. With get_user() of course, there's
no option not to optimize it away.
However, this _does_ reveal a bug with your approach. With sizeof(*p)
being 8, and the type of __r2 being a 32-bit quantity, the compiler will
choose the 64-bit accessor, which will corrupt r3 - and the compiler
won't know that r3 has been corrupted.
That's too unsafe.
I just tried a variant of your approach, but got lots of warnings again:
register unsigned long long __r2 asm("r2");
__get_user_x(__r2, __p, __e, 8, "lr");
x = (typeof(*(__p))) ((typeof(x))__r2);
because typeof(x) in the test_ptr() case ends up being a pointer itself.
So, back to the drawing board, and I think back to the original position
of "we don't support this".
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] ARM: add get_user() support for 8 byte types
On Mon, Nov 12, 2012 at 06:31:50PM -0600, Rob Clark wrote:
> right, that is what I was worried about.. but what about something
> along the lines of:
>
> case 8: { \
> if (sizeof(x) < 8) \
> __get_user_x(__r2, __p, __e, __l, 4); \
> else\
> __get_user_x(__r2, __p, __e, __l, 8); \
> break; \
> } \
>
> maybe we need a special variant of __get_user_8() instead to get the
> right 32bits on big vs little endian systems, but I think something
> roughly along these lines could work.
The problem with that is... big endian systems - where the 32-bit word we
want is the second one, so you can't just reduce that down to a 32-bit
access like that. You also need to add an offset to the pointer in the BE
case (which can be done.)
I'd suggest calling the 4-byte version in there __get_user_xb() and doing
the 4-byte offset for BE inside that (or aliasing it to __get_user_x for
LE systems).
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] ARM: add get_user() support for 8 byte types
On Tue, Nov 13, 2012 at 09:11:09AM +, Arnd Bergmann wrote:
> On Tuesday 13 November 2012, Rob Clark wrote:
> > right, that is what I was worried about.. but what about something
> > along the lines of:
> >
> > case 8: { \
> > if (sizeof(x) < 8) \
> > __get_user_x(__r2, __p, __e, __l, 4); \
> > else\
> > __get_user_x(__r2, __p, __e, __l, 8); \
> > break; \
> > } \
>
> I guess that's still broken if x is 8 or 16 bits wide.
Actually, it isn't - because if x is 8 or 16 bits wide, and we load
a 32-bit quantity, all that follows is a narrowing cast which is exactly
what happens today. We don't have a problem with register allocation
like we have in the 32-bit x vs 64-bit pointer target type, which is what
the above code works around.
> > maybe we need a special variant of __get_user_8() instead to get the
> > right 32bits on big vs little endian systems, but I think something
> > roughly along these lines could work.
> >
> > Or maybe in sizeof(x)<8 case, we just __get_user_bad().. I'm not 100%
> > sure on whether this is supposed to be treated as an error case at
> > compile time or not.
>
> We know that nobody is using that at the moment, so we could define
> it to be a compile-time error.
>
> But I still think this is a pointless exercise, a number of people have
> concluded independently that it's not worth trying to come up with a
> solution, whether one exists or not. Why can't you just use copy_from_user()
> anyway?
You're missing something; that is one of the greatest powers of open
source. The many eyes (and minds) effect. Someone out there probably
has a solution to whatever problem, the trick is to find that person. :)
I think we have a working solution for this for ARM. It won't be suitable
for every arch, where they have 8-bit and 16-bit registers able to be
allocated by the compiler, but for architectures where the minimum register
size is 32-bit, what we have below should work.
In other words, I don't think this will work for x86-32 where ax, al, ah
as well as eax are still available.
What I have currently in my test file, which appears to work correctly,
is (bear in mind this is based upon an older version of get_user() which
pre-dates Will's cleanups):
#define __get_user_x(__r2,__p,__e,__s,__i...) \
__asm__ __volatile__ ( \
"bl __get_user_" #__s \
: "=&r" (__e), "=r" (__r2) \
: "0" (__p) \
: __i, "cc")
#ifdef BIG_ENDIAN
#define __get_user_xb(__r2,__p,__e,__s,__i...) \
__get_user_x(__r2,(uintptr_t)__p + 4,__s,__i)
#else
#define __get_user_xb __get_user_x
#endif
#define get_user(x,p) \
({ \
register const typeof(*(p)) __user *__p asm("r0") = (p);\
register int __e asm("r0"); \
register typeof(x) __r2 asm("r2"); \
switch (sizeof(*(__p))) { \
case 1: \
__get_user_x(__r2, __p, __e, 1, "lr"); \
break; \
case 2: \
__get_user_x(__r2, __p, __e, 2, "r3", "lr");\
break; \
case 4: \
__get_user_x(__r2, __p, __e, 4, "lr"); \
break; \
case 8: \
if (sizeof((x)) < 8)\
__get_user_xb(__r2, __p, __e, 4, "lr"); \
else\
__get_user_x(__r2, __p, __e, 8, "lr"); \
break; \
default: __e = __get_user_bad(); break; \
} \
x = (typeof(*(__p))) __r2; \
__e
Re: [PATCH] ARM: add get_user() support for 8 byte types
On Mon, Nov 19, 2012 at 04:32:36PM +0200, Ville Syrjälä wrote: > On Thu, Nov 15, 2012 at 02:39:41PM +, Arnd Bergmann wrote: > > On Thursday 15 November 2012, Rob Clark wrote: > > > > I still haven't heard a conclusive argument why we need to use > > > > get_user() > > > > rather than copy_from_user() in the DRM code. Is this about a fast path > > > > where you want to shave off a few cycles for each call, or does this > > > > simplify the code structure, or something else? > > > > > > well, it is mostly because it seemed like a good idea to first try to > > > solve the root issue, rather than having to fix things up in each > > > driver when someone from x86-world introduces a 64b get_user().. > > > > As pointed out by hpa earlier, x86-32 doesn't have a 64b get_user > > either. I don't think we have a lot of drivers that are used only > > on 64-bit x86 and on 32-bit ARM but not on 32-bit x86. > > Ouch. I didn't realize that x86-32 doesn't have it. All the systems > where I've run the new code are 64bit so I never noticed the problem. > > I see there was a patch [1] posted a long time ago to implement 64bit > get_user() on x86-32. I wonder what happened to it? > > [1] https://lkml.org/lkml/2004/4/20/96 Wonderful lkml.org after four "Negotiating SSL connection..." messages gives me under elinks... Unable to retrieve https://lkml.org/lkml/2004/4/20/96: Error reading from socket and with wget: $ wget https://lkml.org/lkml/2004/4/20/96 --2012-11-19 14:47:16-- https://lkml.org/lkml/2004/4/20/96 Resolving lkml.org... 87.253.128.182 Connecting to lkml.org|87.253.128.182|:443... connected. HTTP request sent, awaiting response... No data received. Retrying. --2012-11-19 14:47:19-- (try: 2) https://lkml.org/lkml/2004/4/20/96 Connecting to lkml.org|87.253.128.182|:443... connected. HTTP request sent, awaiting response... No data received. Retrying. --2012-11-19 14:47:22-- (try: 3) https://lkml.org/lkml/2004/4/20/96 Connecting to lkml.org|87.253.128.182|:443... connected. HTTP request sent, awaiting response... No data received. Retrying. --2012-11-19 14:47:26-- (try: 4) https://lkml.org/lkml/2004/4/20/96 Connecting to lkml.org|87.253.128.182|:443... connected. HTTP request sent, awaiting response... No data received. Retrying. ... what a wonderful site... please choose another LKML archive, preferably one which works. Thanks. ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/4] drm/tilcdc: add TI LCD Controller DRM driver (v3)
On Wed, Jan 23, 2013 at 07:24:33AM -0600, Rob Clark wrote: > On Wed, Jan 23, 2013 at 3:42 AM, Jean-Francois Moine wrote: > > Hi Rob, > > > > As I wanted to re-use your nxp-tda998x driver for the Marvell Dove SoC, > > I had a look at your IT LCD driver. Comments below. > > Just fyi, you can re-use the nxp-tda998x part independently of tilcdc > (just in case that wasn't clear). Great, this chip is also used on the cubox too. The one thing I wonder is how you deal with the VSYNC/HSYNC polarities that are provided to the TDA9988x chip. On the cubox, I have to adjust the mode parameters such that the polarities are fixed up thusly: adjusted->flags &= ~(DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_PVSYNC | DRM_MODE_FLAG_NVSYNC | DRM_MODE_FLAG_PCSYNC | DRM_MODE_FLAG_NCSYNC); /* The TDA19988 always requires negative VSYNC? */ adjusted->flags |= DRM_MODE_FLAG_NVSYNC; /* The TDA19988 requires positive HSYNC on 1080p or 720p */ if ((adjusted->hdisplay == 1920 && adjusted->vdisplay == 1080) || (adjusted->hdisplay == 1280 && adjusted->vdisplay == 720)) adjusted->flags |= DRM_MODE_FLAG_PHSYNC; else adjusted->flags |= DRM_MODE_FLAG_NHSYNC; return true; for these resolutions to be displayed correctly. Also, when the only output is the HDMI device, reporting the display "disconnected" and without any modes seems to cause problems - I have to report "unknown" status when there's nothing connected, and also provide a default (720p) mode when no EDID is received. ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH V4 0/2] arm: samsung: Move FIMD headers to include/video/
On Tue, Aug 07, 2012 at 06:04:30PM +0530, Leela Krishna Amudala wrote: > arch/arm/plat-samsung/include/plat/regs-fb-v4.h| 159 > > drivers/gpu/drm/exynos/exynos_drm_fimd.c |2 +- > drivers/video/s3c-fb.c |2 +- > .../plat/regs-fb.h => include/video/samsung_fimd.h | 152 +-- Isn't include/video for framebuffer drivers? Isn't this more a DRM thing? Wouldn't include/drm therefore be more appropriate? ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] Consistently name interlaced modes
At the moment, there is an inconsistency in the way modes are named.
Modes with timings parsed from the EDID information will call
drm_mode_set_name(), which will name the mode using this form:
x
eg, 1920x1080i for an interlaced mode, or 1920x1080 for a progressive
mode.
However, timings parsed using the tables in drm_edid_modes.h do not
have the 'i' suffix. You are left to deduce that they're interlaced
from xrandr's output by the lower vertical refresh frequencies.
This patch changes the interlaced mode names in drm_edid_modes.h to
follow the style set by drm_mode_set_name(), which makes it clear
in xrandr which modes are interlaced and which are not (as xrandr
groups the refresh rates on a line according to the name field.)
Signed-off-by: Russell King
---
drivers/gpu/drm/drm_edid_modes.h | 42 +++
1 file changed, 21 insertions(+), 21 deletions(-)
diff --git a/drivers/gpu/drm/drm_edid_modes.h b/drivers/gpu/drm/drm_edid_modes.h
index ff98a7e..57459b3 100644
--- a/drivers/gpu/drm/drm_edid_modes.h
+++ b/drivers/gpu/drm/drm_edid_modes.h
@@ -89,7 +89,7 @@ static const struct drm_display_mode drm_dmt_modes[] = {
976, 1088, 0, 480, 486, 494, 517, 0,
DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC) },
/* 1024x768@43Hz, interlace */
- { DRM_MODE("1024x768", DRM_MODE_TYPE_DRIVER, 44900, 1024, 1032,
+ { DRM_MODE("1024x768i", DRM_MODE_TYPE_DRIVER, 44900, 1024, 1032,
1208, 1264, 0, 768, 768, 772, 817, 0,
DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC |
DRM_MODE_FLAG_INTERLACE) },
@@ -395,7 +395,7 @@ static const struct drm_display_mode edid_est_modes[] = {
{ DRM_MODE("1024x768", DRM_MODE_TYPE_DRIVER, 65000, 1024, 1048,
1184, 1344, 0, 768, 771, 777, 806, 0,
DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC) }, /*
1024x768@60Hz */
- { DRM_MODE("1024x768", DRM_MODE_TYPE_DRIVER,44900, 1024, 1032,
+ { DRM_MODE("1024x768i", DRM_MODE_TYPE_DRIVER,44900, 1024, 1032,
1208, 1264, 0, 768, 768, 776, 817, 0,
DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC |
DRM_MODE_FLAG_INTERLACE) }, /* 1024x768@43Hz */
{ DRM_MODE("832x624", DRM_MODE_TYPE_DRIVER, 57284, 832, 864,
@@ -506,17 +506,17 @@ static const struct drm_display_mode edid_cea_modes[] = {
1430, 1650, 0, 720, 725, 730, 750, 0,
DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC) },
/* 5 - 1920x1080i@60Hz */
- { DRM_MODE("1920x1080", DRM_MODE_TYPE_DRIVER, 74250, 1920, 2008,
+ { DRM_MODE("1920x1080i", DRM_MODE_TYPE_DRIVER, 74250, 1920, 2008,
2052, 2200, 0, 1080, 1084, 1094, 1125, 0,
DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC |
DRM_MODE_FLAG_INTERLACE) },
/* 6 - 1440x480i@60Hz */
- { DRM_MODE("1440x480", DRM_MODE_TYPE_DRIVER, 27000, 1440, 1478,
+ { DRM_MODE("1440x480i", DRM_MODE_TYPE_DRIVER, 27000, 1440, 1478,
1602, 1716, 0, 480, 488, 494, 525, 0,
DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC |
DRM_MODE_FLAG_INTERLACE | DRM_MODE_FLAG_DBLCLK) },
/* 7 - 1440x480i@60Hz */
- { DRM_MODE("1440x480", DRM_MODE_TYPE_DRIVER, 27000, 1440, 1478,
+ { DRM_MODE("1440x480i", DRM_MODE_TYPE_DRIVER, 27000, 1440, 1478,
1602, 1716, 0, 480, 488, 494, 525, 0,
DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC |
DRM_MODE_FLAG_INTERLACE | DRM_MODE_FLAG_DBLCLK) },
@@ -531,12 +531,12 @@ static const struct drm_display_mode edid_cea_modes[] = {
DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC |
DRM_MODE_FLAG_DBLCLK) },
/* 10 - 2880x480i@60Hz */
- { DRM_MODE("2880x480", DRM_MODE_TYPE_DRIVER, 54000, 2880, 2956,
+ { DRM_MODE("2880x480i", DRM_MODE_TYPE_DRIVER, 54000, 2880, 2956,
3204, 3432, 0, 480, 488, 494, 525, 0,
DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC |
DRM_MODE_FLAG_INTERLACE) },
/* 11 - 2880x480i@60Hz */
- { DRM_MODE("2880x480", DRM_MODE_TYPE_DRIVER, 54000, 2880, 2956,
+ { DRM_MODE("2880x480i", DRM_MODE_TYPE_DRIVER, 54000, 2880, 2956,
3204, 3432, 0, 480, 488, 494, 525, 0,
DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC |
DRM_MODE_FLAG_INTERLACE) },
@@ -573,17 +573,17 @@ static const struct drm_display_mode edid_cea_modes[] = {
1760, 1980, 0, 720, 725, 730, 750, 0,
DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC) },
/* 20 - 1920x1080i@50Hz */
- { DRM_MODE("1920x1080", DRM_MODE_TYPE_DRIVER, 74250, 1920, 2448,
+ { DRM_MODE("1920x1080i", DRM_MODE_TYPE_DRIVER, 74250, 1920, 2448,
2492, 2640, 0, 1080, 1084, 1094, 1125, 0
[BUG] EDID leaks kernel memory
Hi, While looking at the kernel DRM code, I've noticed that in many places we kmalloc() memory to store the raw EDID information, whether it be from a DDC adapter, or loaded from firmware. Nowhere can I find where this memory is freed. It seems in several places that we assign it into connector->display_info.raw_edid, and next time we obtain EDID information, we re-kmalloc and overwrite this pointer. Note that some drivers do kfree() this memory themselves after blindly setting connector->display_info.raw_edid to NULL... Can someone please point me to where this memory is freed? If not, I'll cook up a patch to add some kfree's. Thanks. ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[BUG] Intel xorg driver 2.20.2 overlay off-by-one bug
While reading through the Intel driver code, I spotted this in
I830SetPortAttributeOverlay:
} else if (attribute == xvPipe) {
xf86CrtcConfigPtr xf86_config = XF86_CRTC_CONFIG_PTR(scrn);
if ((value < -1) || (value > xf86_config->num_crtc))
return BadValue;
if (value < 0)
adaptor_priv->desired_crtc = NULL;
else
adaptor_priv->desired_crtc = xf86_config->crtc[value];
This allows value == xf86_config->num_crtc to be valid, which would be
the CRTC number _after_ the last one in the array. Presumably this is
not desired, and the test should be ">=".
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC 1/2] dma-buf: Introduce dma buffer sharing mechanismch
On Tue, Nov 08, 2011 at 06:42:27PM +0100, Daniel Vetter wrote: > Actually I think the importer should get a _mapped_ scatterlist when it > calls get_scatterlist. The simple reason is that for strange stuff like > memory remapped into e.g. omaps TILER doesn't have any sensible notion of > an address in physical memory. For the USB-example I think the right > approach is to attach the usb hci to the dma_buf, after all that is the > device that will read the data and move over the usb bus to the udl > device. Similar for any other device that sits behind a bus that can't do > dma (or it doesn't make sense to do dma). > > Imo if there's a use-case where the client needs to frob the sg_list > before calling dma_map_sg, we have an issue with the dma subsystem in > general. Let's clear something up about the DMA API, which I think is causing some misunderstanding here. For this purpose, I'm going to talk about dma_map_single(), but the same applies to the scatterlist and _page variants as well. dma = dma_map_single(dev, cpuaddr, size, dir); dev := the device _performing_ the DMA operation. You are quite correct that in the case of a USB peripheral device, the device is normally the USB HCI device. dma := dma address to be programmed into 'dev' which corresponds (by some means) with 'cpuaddr'. This may not be the physical address due to bus offset translations or mappings setup in IOMMUs. Therefore, it is wrong to talk about a 'physical address' when talking about the DMA API. We can take this one step further. Lets say that the USB HCI is not capable of performing memory accesses itself, but it is connected to a separate DMA engine device: mem <---> dma engine <---> usb hci <---> usb peripheral (such setups do exist, but despite having such implementations I've never tried to support it.) In this case, the dma engine, in response to control signals from the USB host controller, will generate the appropriate bus address to access memory and transfer the data into the USB HCI device. So, in this case, the struct device to be used for mapping memory for transfers to the usb peripheral is the DMA engine device, not the USB HCI device nor the USB peripheral device. ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/4] drm/i2c: nxp-tda998x (v3)
On Thu, Jan 31, 2013 at 08:23:53AM -0600, Rob Clark wrote: > On Wed, Jan 30, 2013 at 8:23 PM, Sebastian Hesselbarth > wrote: > > On 01/29/2013 06:23 PM, Rob Clark wrote: > >> > >> Driver for the NXP TDA998X i2c hdmi encoder slave. > > > > > > Rob, > > > > good to see a driver for TDA998x comming! I'd love to test > > it on CuBox (mach-dove) but there is no gpu driver I can hook up, > > yet. Anyway, I will make some comments how I think the driver > > should be improved. I will eventually pick this up and give it a whirl with my cubox DRM stuff. I've yet to receive my round tuit. ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v9 2/2] video: drm: exynos: Add pinctrl support to fimd
On Thu, Feb 28, 2013 at 11:03:57PM +0100, Sylwester Nawrocki wrote: > Please just use IS_ERR(), let's stop this IS_ERR_OR_NULL() insanity. Yes, indeed. On that topic (and off-topic for this thread, sorry) I've committed a set of patches to remove most users of IS_ERR_OR_NULL() from arch/arm. These are the last remaining ones there - and I don't want to see any more appearing: arch/arm/plat-samsung/clock.c: if (IS_ERR_OR_NULL(clk)) arch/arm/plat-samsung/clock.c: if (!IS_ERR_OR_NULL(clk) && clk->ops && clk->ops->round_rate) arch/arm/plat-samsung/clock.c: if (IS_ERR_OR_NULL(clk)) arch/arm/plat-samsung/clock.c: if (IS_ERR_OR_NULL(clk) || IS_ERR_OR_NULL(parent)) arch/arm/mach-imx/devices/platform-ipu-core.c: if (IS_ERR_OR_NULL(imx_ipu_coredev)) arch/arm/mach-imx/devices/platform-ipu-core.c: if (IS_ERR_OR_NULL(imx_ipu_coredev)) arch/arm/kernel/smp_twd.c: * We use IS_ERR_OR_NULL() here, because if the clock stubs arch/arm/kernel/smp_twd.c: if (!IS_ERR_OR_NULL(twd_clk)) They currently all legal uses of it - though I'm sure that the samsung clock uses can be reduced to just IS_ERR(). The IMX use looks "valid" in that imx_ipu_coredev really can be an error pointer (on failure) or NULL if the platform device hasn't yet been created. The TWD one is explained in the comments in the code (if people had to write explanations for using IS_ERR_OR_NULL(), we'd probably have it used correctly!) ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC 06/10] video: ARM CLCD: Add DT & CDF support
On Wed, Apr 17, 2013 at 04:17:18PM +0100, Pawel Moll wrote:
> +#if defined(CONFIG_OF)
> +static int clcdfb_of_get_tft_parallel_panel(struct clcd_panel *panel,
> + struct display_entity_interface_params *params)
> +{
> + int r = params->p.tft_parallel.r_bits;
> + int g = params->p.tft_parallel.g_bits;
> + int b = params->p.tft_parallel.b_bits;
> +
> + /* Bypass pixel clock divider, data output on the falling edge */
> + panel->tim2 = TIM2_BCD | TIM2_IPC;
> +
> + /* TFT display, vert. comp. interrupt at the start of the back porch */
> + panel->cntl |= CNTL_LCDTFT | CNTL_LCDVCOMP(1);
> +
> + if (params->p.tft_parallel.r_b_swapped)
> + panel->cntl |= CNTL_BGR;
NAK. Do not set this explicitly. Note the driver talks about this being
"the old way" - this should not be used with the panel capabilities - and
in fact it will be overwritten.
Instead, you need to encode this into the capabilities by masking the
below with CLCD_CAP_RGB or CLCD_CAP_BGR depending on the ordering.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC v2] video: ARM CLCD: Add DT & CDF support
On Thu, Apr 18, 2013 at 06:33:21PM +0100, Pawel Moll wrote: > This patch adds basic DT bindings for the PL11x CLCD cells > and make their fbdev driver use them, together with the > Common Display Framework. > > The DT provides information about the hardware configuration > and limitations (eg. the largest supported resolution) > but the video modes come exclusively from the Common > Display Framework drivers, referenced to by the standard CDF > binding. > > Signed-off-by: Pawel Moll Much better. I will point out though that there be all sorts of worms here when you come to the previous ARM evaluation boards (which is why the capabilities stuff got written in the first place) where there's a horrid mixture of BGR/RGB ordering at various levels of the system - some of which must be set correctly because the CLCD output isn't strictly used as R bits G bits and B bits (to support different formats from the CLCD's native formats.) ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] module: fix mutiple defined issue
On Mon, Apr 29, 2013 at 02:32:01PM +0900, Inki Dae wrote: > This patch fixes mutiple defined issue to MODULE_DEVICE_TABLE > > The issue could be induced when some framework which includes two > more sub drivers, is built as one moudle because those sub drivers > could have their own MODULE_DEVICE_TABLE. > > And 'struct of_device_id' isn't needed to be determined by type > argument because the definition of 'of_device_id' should be fixed. > So this patch makes 'of_devce_id' definition to be fixed and > only its instance name to be defined by type. > > Signed-off-by: Inki Dae > --- > include/linux/module.h |2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) > > diff --git a/include/linux/module.h b/include/linux/module.h > index 46f1ea0..ac5d79f 100644 > --- a/include/linux/module.h > +++ b/include/linux/module.h > @@ -84,7 +84,7 @@ void trim_init_extable(struct module *m); > > #ifdef MODULE > #define MODULE_GENERIC_TABLE(gtype,name) \ > -extern const struct gtype##_id __mod_##gtype##_table \ > +extern const struct of_device_id __mod_##gtype##_table \ >__attribute__ ((unused, alias(__stringify(name > > #else /* !MODULE */ This patch (a) looks wrong (why would a generic device table be limited to of_device_id when it could be ISAPNP or something else?) and (b) how does changing the type fix the "multiple defined issue" ? (c) include the errors that you're fixing in the commit log. ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[RFC 0/8] rmk's Dove DRM/TDA19988 Cubox driver
What follows is my DRM driver for Dove, which I've been working on with the Solid-run Cubox, which only offers HDMI output via the TDA19988 chip. Not everything is finished off perfectly in this driver; there's quite a number of ragged edges (particularly with page flipping, which is completely untested.) I have most of the Xorg driver complete - none of the DRI interfaces checked yet (partly because that involves quite an amount of work with Mesa). However, graphics, video overlay, interlacing, accelerated GPU via the original vivante support all works through this driver, with this driver providing the X pixmaps. ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/i2c: nxp-tda998x: fix abort of some i2c exchanges
On Fri, May 17, 2013 at 08:38:03AM +0200, Jean-Francois Moine wrote: > The signals SIGALRM (most often) and SIGPOLL (sometimes) may be raised > after irq or some parameter change and this aborts the i2c exchanges. > > The problem is solved by masking these signals. NAK. The real problem is in the I2C driver, which actually suffers from multiple problems. See the set of I2C patches I posted yesterday. ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC 0/8] rmk's Dove DRM/TDA19988 Cubox driver
On Fri, May 17, 2013 at 01:33:45PM +0200, Jean-Francois Moine wrote: > Hi Russell, > > I quickly compared your dove drm driver and ours (Sebastian and me): I already said - I don't support DT. I don't run any DT based ARM devices, so I have no experience with DT. What I care more about is a working cubox platform, which afaik DT still can't offer yet. > - CMA helper > > You don't use DRM_KMS_CMA_HELPER and DRM_GEM_CMA_HELPER which would > simplify some code. Possibly, but the biggest win for me is having cacheable gem objects. CMA will be a retrograde step for those. > - device tree > > Our driver depends on the DT and, by this way, it may be used for > various boards (Cubox, DB-MV88AP510 Development Board, CompuLab > CM-A510 Board..). Especially, in the Cubox, only a HDMI screen may be > connected, while the display controller permits connection of a smart > panel (port A) and a VGA screen (port B). I only have a Cubox, so I can't test anything but HDMI. Given that there's big questions about the polarity of signals coming out of the Armada 510 (with the invert bits 0, are the syncs positive or negative?) I wouldn't like to lead anyone up the path of thinking that this driver supports anything but the tested HDMI scenario yet. As that information is not in any of the Armada 510 TRMs, and can only be discovered by physically probing the signals with a scope... that's on my todo list when I feel like dismantling the Cubox and getting the soldering iron out to separate the two boards so I can get access to the signals. I've already tried to explain this to Sebastian on IRC, and kept getting nonsense back from him. "You need to program the whole chain". Yes, but that makes no sense to the question I was asking. The reason is this: +-+ ++ | Armada510 | | TDA19988 | | LCD controller ---> switch -> switch --> processing | | VSYNC/HSYNC | || +-+ ++ The issue is that each 'switch' point is capable of inverting the sync signal. If you program both identically then you end up with inversion following another inversion. Which means no inversion. That's why I've found Sebastian's answers to be much less than helpful on this point. What I have found with the NXP TDA19988 driver is that you need to get the input syncs to the TDA19988 set to the correct polarity for the TDA19988, which _isn't_ what is advertised in the EDID. The TDA19988 then has its own processing internally which places the appropriate sync codes onto the output data stream. In some cases, getting this wrong shifts the displayed image by a few pixels/lines. In other cases you get no output or invalid output which isn't recognised by the connected device. > Also, the driver could be used for different chips as the Armada 160 > which has quite the same LCD controller (but one LCD only and no > display controller - Sebastian's idea). That's no problem - you just provide my driver with only one IO space for the LCD controller, and it will only use one. If you have more than two, it will cope as well. > - module loading (si5351, tda998x) > > As our driver is loaded by the Cubox DT, and as the auxilliary drivers > (external clock, HDMI transmitter) may also be modules, a > synchronization mechanism (inspired by the tegra drm driver) permits > the driver to start when all the other drivers are also started. The lack of Si5351 already gets tested on every cubox boot I do. That causes probe deferal, which allows the driver to properly start when it becomes ready. Serial: 8250/16550 driver, 2 ports, IRQ sharing disabled serial8250.0: ttyS0 at MMIO 0xf1012000 (irq = 7) is a 16550A console [ttyS0] enabled serial8250.1: ttyS1 at MMIO 0xf1012100 (irq = 8) is a 16550A [drm] Initialized drm 1.1.0 20060810 [drm:dove_drm_load] *ERROR* failed to get clock platform dove-drm: Driver dove-drm requests probe deferral ... si5351 0-0060: registered si5351 i2c client si5351 0-0060: external clock setup : clkdev = d827f820 cubox_extclk_setup : add alias 'extclk/dovefb.0' to clkout0 w 0 external clock setup done ... dove-drm dove-drm: found TDA19988 [drm] Supports vblank timestamp caching Rev 1 (10.10.2010). [drm] No driver support for vblank timestamp query. dove-drm dove-drm: failed to read EDID Console: switching to colour frame buffer device 160x45 dove-drm dove-drm: fb0: dove-drmfb frame buffer device dove-drm dove-drm: registered panic notifier [drm] Initialized dove-drm 1.0.0 20120730 on minor 0 TDA998x is a different matter - I do need to propagate that error code, and then make a decision in the tda19988 connector whether to return -EPROBEDEFER. (The failure to read EDID is caused by my HDMI switch, which at boot doesn't have the cubox selected, and so doesn't provide EDID until the cubox is selected.) > - d
Re: [RFC 0/8] rmk's Dove DRM/TDA19988 Cubox driver
On Fri, May 17, 2013 at 07:40:23PM +0200, Jean-Francois Moine wrote: > On Fri, 17 May 2013 13:01:15 +0100 > Russell King - ARM Linux wrote: > > > - CMA helper > > > > > > You don't use DRM_KMS_CMA_HELPER and DRM_GEM_CMA_HELPER which would > > > simplify some code. > > > > Possibly, but the biggest win for me is having cacheable gem objects. > > CMA will be a retrograde step for those. > > I did not think yet about the possible problems which could be raised > when the output of the video decoding engine, vMeta, will be the LCD > overlay plane. Not talking about vMeta here, but the vivante GPU. vMeta has entirely different requirements because vMeta does not have any IOMMU, whereas the GPU has a built-in MMU. That alone allows us to deal with SHM-backed buffer objects in a sensible way. > > I've already tried to explain this to Sebastian on IRC, and kept getting > > nonsense back from him. "You need to program the whole chain". Yes, > > but that makes no sense to the question I was asking. > > > > The reason is this: > > +-+ ++ > > | Armada510 | | TDA19988 | > > | LCD controller ---> switch -> switch --> processing | > > | VSYNC/HSYNC | || > > +-+ ++ > > > > The issue is that each 'switch' point is capable of inverting the sync > > signal. If you program both identically then you end up with inversion > > following another inversion. Which means no inversion. That's why > > I've found Sebastian's answers to be much less than helpful on this > > point. > > > > What I have found with the NXP TDA19988 driver is that you need to get > > the input syncs to the TDA19988 set to the correct polarity for the > > TDA19988, which _isn't_ what is advertised in the EDID. The TDA19988 > > then has its own processing internally which places the appropriate > > sync codes onto the output data stream. > > > > In some cases, getting this wrong shifts the displayed image by a few > > pixels/lines. In other cases you get no output or invalid output which > > isn't recognised by the connected device. > > Right. But I found that using Rob's tda988x driver gives me a full > image on my TV set while with NXP's driver, I had more than 20 pixels > lost on each side (left, right, top and bottom). At the moment, Rob's driver has many problems, one of them is that it mis-programs the NPIX and NLINE registers in such a way that it causes my TV to complain about the input. Basically, the driver as it stands in mainline is totally unusable for me. The TV doesn't recognise any output. Yet, program the device with the CEA mode (where it uses its own internal settings) and lo and behold, picture. I tracked that down to NPIX and NLINE being completely wrong. What you're actually seeing is what's called "overscan" which is something that TVs do with broadcast images. The left and right pixels, top and bottom lines are always "off screen". However, our computer displays, we expect the display to be "underscanned" where the top left and bottom right most pixels correspond with the corners of the display. There are bits in the HDMI data stream which tell the TV whether the display should be overscanned or underscanned. Unfortunately, many TVs choose to ignore this data (they're permitted to do this by the standards), and instead decide that if it's a computer resolution, they will underscan, otherwise if it's a broadcast resolution, they will overscan. That's not a bug in the transmission. That's a bug in the display device. > > > - hardware cursor > > > > > > Our driver always proposes the HWC 32 (RGBA 64x64). > > > > Hardware cursor support is pretty useless. > > > > In RGBA mode, it's not 64x64 but you get the choice of either 64x32 or > > 32x64. This is useless for Xorg's purposes, where it really wants a > > 64x64 cursor. And the XF86 Xorg backend really wants RGBA for hardware > > cursor, not 2bpp. So my conclusion is that hardware cursor is not worth > > any effort and I've a good mind to strip that out from my driver (which > > will simplify a few places.) > > > > I've played with the idea of reducing a RGBA cursor down to 2bpp mode > > where we can fit the required size in, but that doesn't work very well. > > > > Just because the hardware does something doesn't
Re: [RFC 0/8] rmk's Dove DRM/TDA19988 Cubox driver
On Fri, May 17, 2013 at 07:00:29PM +0100, Russell King - ARM Linux wrote: > On Fri, May 17, 2013 at 07:40:23PM +0200, Jean-Francois Moine wrote: > > Maybe I did not explain correctly: the colored cursor maybe RGB888 + > > transparency (64x64) or full ARGB (64x32 or 32x64). I coded the first > > case. And, yes, I better like a hardware cursor: it asks for less > > computation, and I get it immediately at graphic starting time! > > Interesting. Where did you find the documentation for the transparency? > The FS lists HWC32_TRANS_CNTL but omits to specify where that gets used. Ignore that, I just found the reference (unfortunately wrapped so evince couldn't find it with a search.) ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC 4/4] DRM: tda998x: add missing include
On Sat, May 18, 2013 at 09:30:09PM +0200, Sebastian Hesselbarth wrote: > The device for tda998x yes, but not the driver. Anyway, Russel decided > to have tda998x probed by his drm_driver. For the simple reason that _that_ is how DRM slave encoders work. Sometimes, reading the code of the subsystem you're using is well worth the effort. If Jean-Francois would like to read drm_encoder_slave.c, then it will be found that in order to use the TDA998x driver, which is itself a DRM slave encoder, you must use drm_i2c_encoder_init(). In order to use that, you must provide the I2C adapter structure, and a board info structure. If you don't want to do that, your options are: (a) you don't use the existing TDA998x DRM slave encoder, and instead write your own TDA998x driver, which will likely be justifyable rejected, or (b) you propose a new DRM interface to allow DRM components to be registered independently, without reference to a core drm_device structure. > The connection of Dove LCD and tda998x is _not_ Cubox specific, it is > also on the D2Plug. To be precise, even "Dove LCD" is not Dove specific > as you can find the very same controller on other Marvell SoCs with > little differences. Well, to spoil the argument a little, actually, the interconnection between the two is in no way "standardized". There's many different ways to wire the two chips together and have it work - because the TDA998x chips have a set of input muxes and swaps which allow you to connect the red, green, blue high/low nibbles in various ways and still have a correctly working system. The TDA998x connectivity is _highly_ configuable. So, just because one board connects LCD_D0 (red bit 0) to a particular pin on the TDA998x does not mean that another board does it that way too. So Jean-Francois is quite correct that this data needs to be provided by the board in some manner. The question is - how to do that sensibly. One possible stop-gap solution is to provide a default set which just happens to match the cubox, and allow OF to override it. :) > There is so much to take care of like pixel format on lcd pins driving > an external encoder (_not_ only tda998x), what gpio pin is connected to > TDA interrupt line, one or two lcds, ... Luckily, drivers/gpu/drm/i2c/tda998x.c does not make use of the IRQ signal at present - it's fairly basic and it currently operates by polling. Eventually, this could change of course. :) I think people need to keep a sense of perspective here: this is all entirely "new" stuff which is still being actively developed. It is not fully polished. We've not had a true open source TDA998x driver before 3.9 (that's when it was introduced.) It has teething problems at the moment, but I'm working with the authors to resolve these issues. I'm also still working on the DRM driver. For example, I've been playing with the RGB888 cursor support today, which seems to be suffering from a one pixel error in the hotspot location. I've not got to the bottom of it, but that kind of error _is_ important to understand and resolve, because it means that things like drawing programmes become unusable. What I'm starting to suspect is a bug in the X server causing this and not either my DRM driver or Xorg driver. ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC 3/4] DRM: add OF support for Dove DRM driver
On Sat, May 18, 2013 at 07:45:02PM +0200, Jean-Francois Moine wrote: > It seems we are moving backwards: > - what about the display controller? > - how do you clone the lcd 0 output to the port B? > - what occurs when the si5351 and the tda998x are modules? I've no idea why you keep bringing that last point up. I've already told you what happens when the SI5351 is a module. It is already not a problem as I have already evidenced by my boot log. So please get that and stop repeating this same point which I've already answered, or I will start ranting at you and we will have a massive falling out. As for cloning output to the VGA port, that's just a matter of dealing with the display controller. That's _not_ difficult. > My driver had the same layout as Russell's when I proposed it to you > and when you insisted to handle the 2 LCDs and the 2 ports as one card. This seems to be a misrepresentation. So what you're saying is that your driver originally handled the two LCDs as two separate cards. That is _not_ the same as my driver. My driver handles the two LCDs as two separate CRTs of the same DRM device. This allows X to drive the two CRTs together in any manner it desires depending on the capabilities of the "connectors" associated with each CRTC and the user preferences. > I spent 2 months to have a nice design and you put it to garbage! > I am not happy... Stop that right now. If you want to start whinging about the amount of time you've spent on this, then I can tell you now that if you have only spent two months on this, you are a total newbie to this and your effort is utterly insignificant compared to mine. And you *have* touched a nerve here by making that statement. ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC 0/8] rmk's Dove DRM/TDA19988 Cubox driver
On Fri, May 17, 2013 at 07:40:23PM +0200, Jean-Francois Moine wrote: > Maybe I did not explain correctly: the colored cursor maybe RGB888 + > transparency (64x64) or full ARGB (64x32 or 32x64). I coded the first > case. And, yes, I better like a hardware cursor: it asks for less > computation, and I get it immediately at graphic starting time! Having looked at this now, using the RGB+transparency is less than ideal because we're having to reduce an alpha channel down to a simple on/off transparency. X cursors really are alphablended components! So, the options here are: (a) use "software" rendered cursor + correct and expected cursor size + correct rendering + possible to use the GPU (I believe mine does) - maybe time consuming as it has to be removed/replaced on the screen (b) use RGB+transparency for 64x64 hardware cursor + correct and expected cursor size + does not have to be removed/replaced when screen contents change - incorrect rendering due to reducing the alpha channel to a simple on/off transparency mask - has to be reloaded when the pointer is close to the edges of the screen which is CPU intensive - cursor image data passed into DRM is required to be ARGB (I've discussed this with David Airlie last night.) This means we have to do translation to RGB+T in the kernel which is *not* nice. (c) use ARGB 32x64 or 64x32 hardware cursor + does not have to be removed/replaced when screen contents change + correct rendering of cursor - unexpected cursor size; user clients do not expect to be restricted to 32 rows or 32 lines of cursor - can only select maximum cursor size on initialization of hardware cursor; can't dynamically switch between 32x64 and 64x32 sizes - has to be reloaded when the pointer is close to the edges of the screen which is CPU intensive While I would like to have hardware cursor support, I don't think it's worth the effort. The one which tips it for me is the need to reload the cursor near the edges - note the restriction in the documentation that the cursor position + cursor size can't be outside the active size. Also, cursors generally have 7 or so transparent pixels to the left of them (which of course changes depending on the cursor shape.) It's also less CPU intensive to (probably) allow the GPU to render it than it is for the CPU to deal with these hardware restrictions. I would've liked to see hardware cursor support, but I think what I'm going to be doing is stripping the cursor code out of the main driver into a separate optional patch which will ultimately be dropped. ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC 3/8] drm/i2c: nxp-tda998x: ensure VIP output mux is properly set
On Sat, May 18, 2013 at 08:56:59AM +0200, Jean-Francois Moine wrote:
> On Thu, 16 May 2013 20:26:18 +0100
> Russell King wrote:
>
> > When switching between various drivers for this device, it's possible
> > that some critical registers are left containing values which affect
> > the device operation. One such case encountered is the VIP output
> > mux register. This defaults to 0x24 on powerup, but other drivers may
> > set this to 0x12. This results in incorrect colours.
> >
> > Fix this by ensuring that the register is always set to the power on
> > default setting.
> >
> > Signed-off-by: Russell King
> > ---
> > drivers/gpu/drm/i2c/tda998x_drv.c |3 +++
> > 1 files changed, 3 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c
> > b/drivers/gpu/drm/i2c/tda998x_drv.c
> > index d71c408..4b4db95 100644
> > --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> > +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> > @@ -110,6 +110,7 @@ struct tda998x_priv {
> > #define REG_VIP_CNTRL_5 REG(0x00, 0x25) /* write */
> > # define VIP_CNTRL_5_CKCASE (1 << 0)
> > # define VIP_CNTRL_5_SP_CNT(x)(((x) & 3) << 1)
> > +#define REG_MUX_VP_VIP_OUTREG(0x00, 0x27) /* read/write */
> > #define REG_MAT_CONTRLREG(0x00, 0x80) /* write */
> > # define MAT_CONTRL_MAT_SC(x) (((x) & 3) << 0)
> > # define MAT_CONTRL_MAT_BP(1 << 2)
> > @@ -438,6 +439,8 @@ tda998x_encoder_dpms(struct drm_encoder *encoder, int
> > mode)
> >
> > switch (mode) {
> > case DRM_MODE_DPMS_ON:
> > + /* Write the default value MUX register */
> > + reg_write(encoder, REG_MUX_VP_VIP_OUT, 0x24);
> > /* enable audio and video ports */
> > reg_write(encoder, REG_ENA_AP, 0xff);
> > reg_write(encoder, REG_ENA_VP_0, 0xff);
>
> This register is never touched. Should not this setting better go at
> reset time (in tda998x_reset)?
Yes, you're right, it could be done there without any additional side
effects. I've just moved it there.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC 0/8] rmk's Dove DRM/TDA19988 Cubox driver
On Fri, May 17, 2013 at 01:33:45PM +0200, Jean-Francois Moine wrote: > I quickly compared your dove drm driver and ours (Sebastian and me): > > - CMA helper > > You don't use DRM_KMS_CMA_HELPER and DRM_GEM_CMA_HELPER which would > simplify some code. Looking at the CMA helper code in DRM, it makes some fundamental errors: 1. It assumes the returned DMA address is a physical address. This is not necessarily the case (there are a number of ARM platforms where this is most definitely not true.) So: cma_obj->vaddr = dma_alloc_writecombine(drm->dev, size, &cma_obj->paddr, GFP_KERNEL | __GFP_NOWARN); ... ret = remap_pfn_range(vma, vma->vm_start, cma_obj->paddr >> PAGE_SHIFT, vma->vm_end - vma->vm_start, vma->vm_page_prot); is extremely broken. 2. If you use the CMA helper, then all your GEM objects must be CMA objects. You can't combine different types of objects (eg, SHM-backed objects) with CMA objects. This will be a massive performance regression with GPUs which can handle scatter-gathered SHM objects such as the Vivante GPU on the Armada 510. So, for me, the last point especially is a very strong argument not to use the DRM CMA helper. Yes, I could switch to using the DMA coherent/ writecombine allocation API and thus make use of CMA, and that's something I will be looking at. ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
BUG: drm_mm head_node gets broken?
So, while tinkering with my Dove DRM driver, I tripped over a BUG_ON()
in drm_mm_hole_node_start(), which was called from drm_mm_dump_table():
int drm_mm_dump_table(struct seq_file *m, struct drm_mm *mm)
{
struct drm_mm_node *entry;
unsigned long total_used = 0, total_free = 0, total = 0;
unsigned long hole_start, hole_end, hole_size;
hole_start = drm_mm_hole_node_start(&mm->head_node);
hole_end = drm_mm_hole_node_end(&mm->head_node);
drm_mm_hole_node_start() does this:
static inline unsigned long drm_mm_hole_node_start(struct drm_mm_node
*hole_node)
{
BUG_ON(!hole_node->hole_follows);
return __drm_mm_hole_node_start(hole_node);
}
This appears to indicate that it is required that the head node should
always have a "hole" following it.
As this used to work, I dug in the git history, and found commit 6b9d89b4
(drm: Add colouring to the range allocator), in particular this change:
static void drm_mm_insert_helper(struct drm_mm_node *hole_node,
struct drm_mm_node *node,
-unsigned long size, unsigned alignment)
+unsigned long size, unsigned alignment,
+unsigned long color)
{
...
- if (!tmp) {
+ if (alignment) {
+ unsigned tmp = adj_start % alignment;
+ if (tmp)
+ adj_start += alignment - tmp;
+ }
+
+ if (adj_start == hole_start) {
hole_node->hole_follows = 0;
- list_del_init(&hole_node->hole_stack);
- } else
- wasted = alignment - tmp;
+ list_del(&hole_node->hole_stack);
+ }
Now, when we do an allocation, it is typically done as follows:
free = drm_mm_search_free(mm, size, align, 0);
if (free)
linear = drm_mm_get_block(free, size, align);
If nothing has been allocated in the mm, 'free' will be &mm->head_node.
The result is that drm_mm_get_block calls down into drm_mm_insert_helper
with hole_node == &mm->head_node. This I confirmed via:
diff --git a/drivers/gpu/drm/drm_mm.c b/drivers/gpu/drm/drm_mm.c
index db1e2d6..7241be8 100644
--- a/drivers/gpu/drm/drm_mm.c
+++ b/drivers/gpu/drm/drm_mm.c
@@ -125,6 +125,7 @@ static void drm_mm_insert_helper(struct drm_mm_node
*hole_node,
}
if (adj_start == hole_start) {
+ BUG_ON(hole_node == &mm->head_node);
hole_node->hole_follows = 0;
list_del(&hole_node->hole_stack);
}
which triggers during booting. Hence, we end up setting the head_node
to indicate no hole following - which violates the conditions of other
parts of the code in drm_mm.c.
It looks like this change was made quite a while ago - back in July
2012, so I assume that while the rest of the allocator is fine with this,
it's just the debugging code which is now broken. However, I suspect
that DRM people may wish to either back out the coloring change or
carefully review the change for correctness throughout the drm_mm
code.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC] drm: i2c: add irq handler for tda998x slave encoder
On Sun, May 19, 2013 at 06:49:22PM +0200, Sebastian Hesselbarth wrote:
> This adds an irq handler for HPD to the tda998x slave encoder driver
> to trigger HPD change instead of polling. The gpio connected to int
> pin of tda998x is passed through platform_data of the i2c client. As
> HPD will ultimately cause EDID read and that will raise an
> EDID_READ_DONE interrupt, the irq handling is done threaded with a
> workqueue to notify drm backend of HPD events.
>
> Signed-off-by: Sebastian Hesselbarth
Okay, I think I get this.. Some comments:
> +static irqreturn_t tda998x_irq_thread(int irq, void *data)
> +{
> + struct drm_encoder *encoder = data;
> + struct tda998x_priv *priv;
> + uint8_t sta, cec, hdmi, lev;
> +
> + if (!encoder)
> + return IRQ_HANDLED;
This won't ever be NULL. The IRQ layer stores the pointer you passed
in request_threaded_irq() and that pointer will continue to point at
that memory until the IRQ is freed. The only way it could be NULL is
if you register using a NULL pointer.
...
> + if (priv->irq < 0) {
> + for (i = 100; i > 0; i--) {
> + uint8_t val = reg_read(encoder, REG_INT_FLAGS_2);
IRQ 0 is the cross-arch "no interrupt" number. So just use !priv->irq
here and encourage anyone who uses -1 or NO_IRQ to fix their stuff. :)
> + struct tda998x_priv *priv = to_tda998x_priv(encoder);
> +
> + /* announce polling if no irq is available */
> + if (priv->irq < 0)
Same here.
> @@ -1122,7 +1197,9 @@ tda998x_encoder_init(struct i2c_client *client,
>
> priv->current_page = 0;
> priv->cec = i2c_new_dummy(client->adapter, 0x34);
> + priv->irq = -EINVAL;
And just initialize to zero. (it's allocated by kzalloc already right?
So that shouldn't be necessary.)
> diff --git a/include/drm/i2c/tda998x.h b/include/drm/i2c/tda998x.h
> index 41f799f..1838703 100644
> --- a/include/drm/i2c/tda998x.h
> +++ b/include/drm/i2c/tda998x.h
> @@ -20,4 +20,8 @@ struct tda998x_encoder_params {
> int swap_f, mirr_f;
> };
>
> +struct tda998x_platform_data {
> + int int_gpio;
> +};
> +
Should be combined with tda998x_encoder_params - the platform data is
really supposed to be passed to set_config - see this in drm_encoder_slave.c:
* If @info->platform_data is non-NULL it will be used as the initial
* slave config.
...
err = encoder_drv->encoder_init(client, dev, encoder);
if (err)
goto fail_unregister;
if (info->platform_data)
encoder->slave_funcs->set_config(&encoder->base,
info->platform_data);
So any platform data set will be passed into the set_config function...
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC 3/4] DRM: add OF support for Dove DRM driver
On Sat, May 18, 2013 at 09:18:46PM +0200, Jean-Francois Moine wrote: > The general hardware code of both drivers is close enough for merging > may be done starting from anyone. But the general layout is not: > > - my driver is DT driven and has one card with 2 CTRC's and 2 connectors. > > - Russell's is non-DT, so, with some extra code I am not aware of, with > any number of cards each one with one CRTC and one connector (no, I Utter shit, and I've told you before. I'm not going to repeat it again, and I warned you that we would have a falling out. As you seem to be immune to my comments provided in a sane manner, I'm now just going to ignore you in the future because you're clearly not bothering to read anything I'm telling you. That makes further discussion with you utterly pointless. > tried it, you cannot clone a connector of one card to the connector > of another card). > > So, for me, merging means enhance my code from Russell's, but I will > not go to a non-DT kernel. You seem to see DT setups vs non-DT setups as two entirely separate things. They are not. We have plenty of drivers which work in both setups just fine. So really all of your points above are utter trash to me - really. As to illustrate the point, my TDA19988 backend is now _just_ a slave encoder backend, it has nothing TDA19988 specific, nor does it have anything cubox specific there; the cubox specific part has been moved up into dove_drv.c as pure data, and that data can be constructed either from platform data or from OF properties parsed by dove_drv.c. But why did I just bother to type that. You're just going to ignore it and keep repeating your same old claptrap. ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC 0/8] rmk's Dove DRM/TDA19988 Cubox driver
On Mon, May 20, 2013 at 09:36:02AM -0400, Alex Deucher wrote: > You can tell the xserver what size cursor you support when you call > xf86_cursors_init() in the ddx. Just expose a 32x64 or 64x32 ARGB > cursor. Most apps don't use a 64x64 cursor anyway. I've used > hardware with non-64x64 cursors and haven't run into any problems yet. I guess the xf86 backend will fall back to software cursors if it gets a cursor larger than the hardware supports, so maybe 64x32 will be fine. However, my comments concerning the less-than-64x64 size come from David Airlie who said "X and userside programs expect 64x64 to work. so its possibly pointless supporting anything less, as in you'd write code but nobody would end up using it". Note also that the generic DRM KMS code assumes cursor support at 64x64, and there's no generic way for a driver at present (that I know of) to inform it of anything different. ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC PATCH v2 8/9] smp: refactor on_each_cpu to void returning func
On Sun, Jan 08, 2012 at 03:32:28PM +0200, Gilad Ben-Yossef wrote: > on_each_cpu returns the retunr value of smp_call_function > which is hard coded to 0. > > Refactor on_each_cpu to a void function and the few callers > that check the return value to save compares and branches. > > Signed-off-by: Gilad Ben-Yossef > Acked-by: Peter Zijlstra > Reviewed-by: Michal Nazarewicz > CC: David Airlie > CC: dri-devel@lists.freedesktop.org > CC: Benjamin Herrenschmidt > CC: Paul Mackerras > CC: Grant Likely > CC: Rob Herring > CC: linuxppc-...@lists.ozlabs.org > CC: devicetree-disc...@lists.ozlabs.org > CC: Richard Henderson > CC: Ivan Kokshaysky > CC: Matt Turner > CC: linux-al...@vger.kernel.org > CC: Thomas Gleixner > CC: Ingo Molnar > CC: "H. Peter Anvin" > CC: x...@kernel.org > CC: Tony Luck > CC: Fenghua Yu > CC: linux-i...@vger.kernel.org > CC: Will Deacon > CC: Peter Zijlstra > CC: Arnaldo Carvalho de Melo > CC: Russell King As there's only one place in the ARM code where we look at the return value, and you've patched that away in patch 1, this looks fine. I've not checked for users outside of arch/arm, so: Acked-by: Russell King Thanks. ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 09/10] MCDE: Add build files and bus
On Tue, Nov 30, 2010 at 04:21:47PM +0100, Arnd Bergmann wrote: > On Tuesday 30 November 2010, Linus Walleij wrote: > > 2010/11/26 Arnd Bergmann : > > > > > * When you say that the devices are static, I hope you do not mean > > > static in the C language sense. We used to allow devices to be > > > declared as "static struct" and registered using > > > platform_device_register (or other bus specific functions). This > > > is no longer valid and we are removing the existing users, do not > > > add new ones. When creating a platform device, use > > > platform_device_register_simple or platform_device_register_resndata. > > > > Is this part of the generic ARM runtime multi-platform kernel > > and device trees shebang? > > > > The Ux500 still isn't in that sector, it needs extensive rewriting > > of arch/arm/mach-ux500 to be done first, so as to support e.g. > > U8500 and U5500 with a single kernel image. > > > > Trying to skin that cat that as part of this review is a bit too > > much to ask IMO, I'd rather have the author of this driver > > adapt to whatever platform data registration mechanism is > > in place for the merge window. Else it needs fixing as part > > of a bigger endavour to root out compile-time platform > > configuration. > > The 'no static devices' rule is something that Greg brought up > at the embedded developer session during PlumbersConf this year, > I wasn't aware of the problem before that either. > > It is not related to the multi-platform kernel work and it's > not ARM specific. > > Maybe Greg can give a short explanation of the impact of this. > AFAIR, static device definitions still work, but there are > plans to remove that capability in the future. There's lots of static devices, not only platform devices, in the ARM tree. It's going to be a hell of a lot of work to fix this all up properly. I hope that the capability for static devices won't disappear until the huge pile of work on ARM has been completed. ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 09/10] MCDE: Add build files and bus
On Tue, Nov 30, 2010 at 10:48:34AM -0800, Greg KH wrote: > On Tue, Nov 30, 2010 at 06:40:49PM +, Russell King - ARM Linux wrote: > > There's lots of static devices, not only platform devices, in the ARM > > tree. It's going to be a hell of a lot of work to fix this all up > > properly. > > I agree, it's been abused for many years this way :( I don't agree that it is abuse - it was something explicitly allowed by the original device model design by Patrick, with the condition that such a device was never unregistered. That's exactly the way we treat these devices. What I'm slightly concerned about is that this is going to needlessly bloat the kernel - we're going to have to find some other way to store this information, and create devices from that - which means additional code to do the creation, and data structures for it to create these from. There will be additional wastage from kmalloc as kmalloc doesn't allocate just the size you ask for, but normally a power of two which will contain the size. That could potentially mean that as the device structure is 216 bytes, kmalloc will use the 256 byte allocation size, which means a wastage of 40 bytes per device structure. On top of that goes the size of resources with the allocation slop on top for that, and then there's another allocation for the platform data. Has anyone considered these implications before making this choice? ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 09/10] MCDE: Add build files and bus
On Tue, Nov 30, 2010 at 03:05:33PM -0800, Greg KH wrote: > On Tue, Nov 30, 2010 at 10:05:50PM +, Russell King - ARM Linux wrote: > > On Tue, Nov 30, 2010 at 10:48:34AM -0800, Greg KH wrote: > > > On Tue, Nov 30, 2010 at 06:40:49PM +, Russell King - ARM Linux wrote: > > > > There's lots of static devices, not only platform devices, in the ARM > > > > tree. It's going to be a hell of a lot of work to fix this all up > > > > properly. > > > > > > I agree, it's been abused for many years this way :( > > > > I don't agree that it is abuse - it was something explicitly allowed by > > the original device model design by Patrick, with the condition that > > such a device was never unregistered. That's exactly the way we treat > > these devices. > > I understand Pat allowed this, I just don't agree that it's the correct > thing to do :) > > -mm had a patch for a long time that would throw up warnings if you ever > did this for x86 so that arch should be clean of this issue by now. > > > What I'm slightly concerned about is that this is going to needlessly > > bloat the kernel - we're going to have to find some other way to store > > this information, and create devices from that - which means additional > > code to do the creation, and data structures for it to create these from. > > There will be additional wastage from kmalloc as kmalloc doesn't allocate > > just the size you ask for, but normally a power of two which will contain > > the size. > > > > That could potentially mean that as the device structure is 216 bytes, > > kmalloc will use the 256 byte allocation size, which means a wastage of > > 40 bytes per device structure. On top of that goes the size of > > resources with the allocation slop on top for that, and then there's > > another allocation for the platform data. > > > > Has anyone considered these implications before making this choice? > > Yes, I have, which is one reason I haven't done this type of change yet. > I need to figure out a way to not drasticly increase the size and still > make it easy and simple for the platform and driver write their code. > > It's a work in progress, but wherever possible, I encourage people to > not make 'struct device' static. Right, so saying to ARM developers that they can't submit code which adds new static device structures is rather problematical then, and effectively brings a section of kernel development to a complete standstill - it means no support for additional ARM platforms until this issue is resolved. (This "condition" was mentioned by Arnd earlier in this thread, and was put in such a way that it was now a hard and fast rule.) I feel it would be better to allow the current situation to continue. If we start telling people that they can't use statically declared devices without first having an alternative, we'll end up with people inventing their own individual - and different - solutions to this problem, which could actually make the problem harder to resolve in the longer term. ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 09/10] MCDE: Add build files and bus
On Wed, Dec 01, 2010 at 01:53:39PM +0100, Peter Stuge wrote: > Russell King - ARM Linux wrote: > > I feel it would be better to allow the current situation to continue. > > I think this misses the point, and is somewhat redundant; I think > everyone knows that it is easiest to never change anything. But > then nothing improves. > > > > If we start telling people that they can't use statically declared > > devices without first having an alternative, > > I would consider it early warning that the way things have been done > before will go away. And I would thus write drivers in a way that > demonstrates and includes that understanding. Clearly you haven't understood my point. If we go down the route you suggest, we're going to end up with everyone doing something different, which will then need to be cleaned up once the proper solution is in place. That could easily become much more work than just waiting for the proper solution. What is easier - to fix all instances which statically declare, or to fix all instances which statically declare _and_ all the bodged up alternative solutions? ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[RFC v2 0/8] rmk's Dove DRM/TDA19988 Cubox driver
This is version 2 of my DRM driver. Changes since the previous version: 1. It's now called Armada not Dove, because the "LCD controllers" aka CRTCs are found in Armada 16x as well as Armada 510 Dove. The Armada 16x is preliminary work. 2. Cursor support is now a separate patch. I've tried several formats, and 64x32 ARGB is the one I've settled on for the time being, though the driver will permit 32x64 as well. This, however, is not available with Armada 16x hardware. This patch will be a separate follow-up to this. The reason for this is it's in a separate branch which conflicts with this (and my NXP-TDA19988 output driver branch.) The value of this is still questionable due to the restrictions in the hardware meaning the entire cursor has to be reloaded near the edge of the screen. This also may interfere with the interlaced support; however the DRM i2c TDA998x driver seems incapable of supporting interlace at present. (It works with the CEA mode number programmed but not if you program with mode timings.) 3. A slave encoder output encoder which contains nothing specific to TDA19988 nor Cubox. This data is now in the top level driver, and can be provided using whatever method is desirable (eg, from DT.) What *can't* be done without a rewrite of the DRM slave encoder backend is to have the TDA998x I2C device provided by DT. There are mumblings about redoing the slave encoder API, hopefully this will be fixed there. 4. Finished off the page flip support, which now gets used on mode changes as well. Page flip as a whole still isn't tested. 5. Removal of default mode and reporting of unknown connector status. The kernel side will select 1024x768 if no connectors are connected - this will cause problems if your display reports that it supports 1024x768 but it doesn't actually. Xorg will do similar too. What is not in this patch set yet, but I have patches for: 1. Pulled the clock stuff out of the armada_crtc.c file into the top level. Armada 16x has different clocking options to Armada 510, and certain bits in the clock configuration register are different. This needs to be dealt with like we do in AMBA devices with "variant" support. So in a DT environment, we'd have: compatible = "marvell,armada510-lcd"; or compatible = "marvell,armada16x-lcd"; and the driver adjusts according to that. 2. Sebastian's additional patches ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC v2 0/8] rmk's Dove DRM/TDA19988 Cubox driver
On Sun, Jun 09, 2013 at 08:06:12PM +0100, Russell King - ARM Linux wrote: > This is version 2 of my DRM driver. Changes since the previous version: Okay, patch 1 got removed from this set, so some people won't see it (it shouldn't have been sent). Patch 2 is in moderation, which is probably the more interesting patch because that's got the interesting stuff in. ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH RFC 2/8] DRM: Armada: Add Armada DRM driver
t; > + * Odd frame, 563 total lines, VSYNC at line 543-548, pixel 1128.
> > + * Even frame, 562 total lines, VSYNC at line 542-547, pixel 2448.
> > + * Note: Vsync front porch remains constant!
> > + *
> > + * if (odd_frame) {
> > + * vtotal = mode->crtc_vtotal + 1;
> > + * vbackporch = mode->crtc_vsync_start - mode->crtc_vdisplay + 1;
> > + * vhorizpos = mode->crtc_hsync_start - mode->crtc_htotal / 2
> > + * } else {
> > + * vtotal = mode->crtc_vtotal;
> > + * vbackporch = mode->crtc_vsync_start - mode->crtc_vdisplay;
> > + * vhorizpos = mode->crtc_hsync_start;
> > + * }
> > + * vfrontporch = mode->crtc_vtotal - mode->crtc_vsync_end;
> > + *
> > + * So, we need to reprogram these registers on each vsync event:
> > + * LCD_SPU_V_PORCH, LCD_SPU_ADV_REG, LCD_SPUT_V_H_TOTAL
>
> ouch, proof that some hw designers must really hate driver writers ;-)
Yep - and interlace support is not something that can be done in a stable
way with the way the Linux kernel deals with interrupts (iow, with no
priority and strictly single-threaded.) The problem is to do this
properly, you need the LCD interrupt able to interrupt any other interrupt
handler so you can reprogram the LCD controller within a very tight
timeframe (less than one display field.)
It works, but occasionally you will get a hiccup (mainly that's the
fault of the USB stack taking 2.25ms over some of its keyboard/mouse
interrupts!)
> > +/* The mode_config.mutex will be held for this call */
> > +static int armada_drm_crtc_mode_set_base(struct drm_crtc *crtc, int x, int
> > y,
> > + struct drm_framebuffer *old_fb)
> > +{
> > + struct armada_crtc *dcrtc = drm_to_armada_crtc(crtc);
> > + struct armada_regs regs[4];
> > + unsigned i;
> > +
> > + i = armada_drm_crtc_calc_fb(crtc->fb, crtc->x, crtc->y, regs,
> > dcrtc->interlaced);
> > + armada_reg_queue_end(regs, i);
> > +
> > + /* Wait for pending flips to complete */
> > + wait_event(dcrtc->frame_wait, !dcrtc->frame_work);
> > +
> > + /* Take a reference to the new fb as we're using it */
> > + drm_gem_object_reference(&drm_fb_obj(crtc->fb)->obj);
>
> note that you probably want to ref/unref the fb (and let the fb hold a
> ref to the gem bo).. that will make life easier for planar formats too
> (as the fb should hold ref's to the bo for each plane)
I'll look into it, but it will take some time as the refcounting is far
from trivial or obvious in DRM...
>From what I can see there's no intrinsic refcounting between the fb
and the gem bo.
> > diff --git a/drivers/gpu/drm/armada/armada_drv.c
> > b/drivers/gpu/drm/armada/armada_drv.c
> [snip]
> > +
> > +static struct drm_ioctl_desc armada_ioctls[] = {
> > + DRM_IOCTL_DEF_DRV(ARMADA_GEM_CREATE, armada_gem_create_ioctl,
> > DRM_UNLOCKED),
> > + DRM_IOCTL_DEF_DRV(ARMADA_GEM_CREATE_PHYS,
> > armada_gem_create_phys_ioctl, DRM_UNLOCKED),
>
> you might want just a single ioctl for creating gem objects, with a
> 'scanout' flag.. perhaps some future version of the hw doesn't need
> phys contig for scanout, and this would probably be easier to handle
> with a single ioctl that has an 'if (flags & SCANOUT &&
> device_needs_phys_contig_scanout()) .. '
The purposes of these two ioctls are quite different. ARMADA_GEM_CREATE
is to create any gem buffer object that needs shmem backing - eg,
non-scanout pixmaps and the like.
ARMADA_GEM_CREATE_PHYS is to deal with creating a gem buffer object for
a chunk of physical memory allocated by other means (eg, the Vmeta stuff.)
This allows my X server to remain compatible with the XF86 Dove driver,
which permits the passing of the physical address of the video frame to
the X server (otherwise we're into rewriting a whole load more code than
is really desirable - and I really don't have time to rewrite bits of
gstreamer and other stuff.)
Lastly, the DUMB stuff is used for the graphics scanout buffer.
> > diff --git a/drivers/gpu/drm/armada/armada_gem.c
> > b/drivers/gpu/drm/armada/armada_gem.c
> [snip]
> > +int armada_gem_linear_back(struct drm_device *dev, struct
> > armada_gem_object *obj)
> > +{
> > + struct armada_private *priv = dev->dev_private;
> > + size_t size = obj->obj.size;
> > +
> > + if (obj->page || obj->linear)
> > + return 0;
> > +
> > + /* If it is a small allocation, try to get it from the system */
> > + if (size < 1048576) {
> > + unsigned int order = get_order(size
Re: [PATCH RFC 2/8] DRM: Armada: Add Armada DRM driver
On Mon, Jun 10, 2013 at 03:59:34PM -0400, Rob Clark wrote: > On Mon, Jun 10, 2013 at 1:06 PM, Russell King - ARM Linux > wrote: > > ARMADA_GEM_CREATE_PHYS is to deal with creating a gem buffer object for > > a chunk of physical memory allocated by other means (eg, the Vmeta stuff.) > > This allows my X server to remain compatible with the XF86 Dove driver, > > which permits the passing of the physical address of the video frame to > > the X server (otherwise we're into rewriting a whole load more code than > > is really desirable - and I really don't have time to rewrite bits of > > gstreamer and other stuff.) > > ahh, gotcha.. and, ugg, that is even worse.. > > I'm not hugely a fan of giving userspace a way to dma into arbitrary > phys addresses (ie. create_phys + put_image). But I don't need to > explain what you already know ;-) > > Is there a pre-defined carveout pool that you can at least bounds > check against? Otherwise put this ioctl inside a #if > CONFIG_CRAZY_SOC_VENDOR_HOLE_TO_DRIVE_A_TRUCK_THROUGH / #endif.. This driver is _not_ about supporting the GPU natively as part of the DRM, but providing the foundations for: (a) a properly functional interface to HDMI TVs (fbdev doesn't hack that) with hotplug (b) providing sufficient infrastructure to allow video playback to work. What this is not about is fixing the crappy userspace GPU libraries and video decoding so that it's "secure" - not only is that way beyond my abilities, it would also be a violation of the closed source license to reverse engineer them so that were possible. It is something that continues to be a problem and I'm making no claims that I'm fixing that. So no, I will not put such a config option around it for the simple reason that by doing so, turning such an option off renders userspace utterly useless and the driver might as well not exist. If that means you want to NACK the patch, then fine; I'll just maintain it out of tree. I did the driver mostly for my own personal benefit to get the Cubox to the point where I can boot it with or without the TV connected and have it work. But it would be a shame if that meant that others could not benefit from about 8 solid months of my work on this and have to put up with crappy a fbdev driver. ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH RFC 2/8] DRM: Armada: Add Armada DRM driver
On Mon, Jun 10, 2013 at 05:01:18PM -0400, Rob Clark wrote: > I would like to get at least some of the driver upstream. I'd hate > for it to have to live completely out of tree. I can think of a > couple possibilities. > > 1) the best would be, if there was some way for the drm driver to know > the upper/lower bounds of the carveout, then it could bounds-check > against this. And then in worst case, userspace can just screw up > other "graphics" buffers The bounds check does what? Check that the buffer object's physical address is within another driver's range? Fine, but all that it's doing is preventing it being associated with a buffer object which can _only_ be used for _read_ access by the LCD controller. There is no write access to the GEM objects by anything that this DRM driver talks to. So all it'll do is prevent you passing rogue addresses to the LCD controller for scanout. And how do we get that information into the driver from other random drivers? Hack in random inter-driver specific APIs to do it? Come up with yet another different way to try and identify whether a particular resource is a block of reserved memory for this driver's usage as a linear region or one of the others? How. Really, please think about the problem. The benefit vs the complexity involved really isn't worth it. ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH RFC 2/8] DRM: Armada: Add Armada DRM driver
On Mon, Jun 10, 2013 at 11:57:32AM -0400, Rob Clark wrote:
> On Sun, Jun 9, 2013 at 3:29 PM, Russell King
> wrote:
> > +/* The mode_config.mutex will be held for this call */
> > +static int armada_drm_crtc_mode_set_base(struct drm_crtc *crtc, int x, int
> > y,
> > + struct drm_framebuffer *old_fb)
> > +{
> > + struct armada_crtc *dcrtc = drm_to_armada_crtc(crtc);
> > + struct armada_regs regs[4];
> > + unsigned i;
> > +
> > + i = armada_drm_crtc_calc_fb(crtc->fb, crtc->x, crtc->y, regs,
> > dcrtc->interlaced);
> > + armada_reg_queue_end(regs, i);
> > +
> > + /* Wait for pending flips to complete */
> > + wait_event(dcrtc->frame_wait, !dcrtc->frame_work);
> > +
> > + /* Take a reference to the new fb as we're using it */
> > + drm_gem_object_reference(&drm_fb_obj(crtc->fb)->obj);
>
> note that you probably want to ref/unref the fb (and let the fb hold a
> ref to the gem bo).. that will make life easier for planar formats too
> (as the fb should hold ref's to the bo for each plane)
Now changed - and it looks from my debug of gem_linear that it's working
correctly (iow, not leaking).
> > +struct drm_armada_gem_create {
> > + uint32_t height;
> > + uint32_t width;
> > + uint32_t bpp;
>
> just fwiw, typically height/width/bpp are properties of the fb but not
> the bo.. (except in some cases where kernel needs to know this to
> setup GTT correctly for tiled buffers)
Also fixed.
> > +struct drm_armada_gem_pwrite {
> > + uint32_t handle;
> > + uint32_t offset;
> > + uint32_t size;
>
> probably want a uint32_t padding here, or move the uint64_t up in the
> struct to avoid 32 vs 64b alignment differences.
And also fixed.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH RFC 2/8] DRM: Armada: Add Armada DRM driver
On Mon, Jun 10, 2013 at 01:10:18PM +0200, Sebastian Hesselbarth wrote:
> On 06/09/13 21:29, Russell King wrote:
>> +/*
>> + * The spec is unclear about the polarities of the syncs.
>> + * We assume their non-inverted state is active high.
>> + */
>
> nit: "We confirmed their non-inverted state is active high."
Thanks, it's been a while since I looked at this so I had forgotten
to update the comment. Now done.
>> +if (resource_size(r) > SZ_4K)
>> +mem = r;
>
> nit again: The register address window of Dove LCD is 64k although you
> are right an no more than 512B are used. Also a comment would be nice,
> that everything above 4k (64k) is interpreted as gfx mem.
Fixed and comment added.
>> +/* Create all LCD controllers */
>> +for (n = 0; n < ARRAY_SIZE(priv->dcrtc); n++) {
>> +struct clk *clk;
>> +
>> +if (!res[n])
>> +break;
>> +
>> +clk = clk_get_sys("dovefb.0", "extclk");
>
> To be precise: the above should have the index at extclk as there
> is two extclk inputs that can be used for both lcdcs. So currently,
> as armada_crtc is hard-coding extclk0 input it should be
> "armadafb.%d", "extclk0".
>
> But I know, clocking in general will work-out with parent select for
> clk-mux and DT support.
I've sorted that out, but I'd forgotten there were three additional
patches on top of what I've posted sorting that stuff out - the
second one in particular:
4a5e9b7 DRM: armada: store kernel address for gem objects
f760c94 DRM: Dove: alternative variant based clocking
2e051fd DRM: Armada: convert hardware cursor support to 64x32 or 32x64 ARGB
Because they're scattered in other branches (the h/w cursor stuff
is separate) it's not trivial to generate a single series from git
for these.
>> +static const struct armada_output_type armada_drm_conn_slave = {
>> +.connector_type = DRM_MODE_CONNECTOR_HDMIA,
>
> For a rework of DRM slave encoder API, there should also be some way to
> get .connector_type and .encoder_type above from that slave encoder.
> IMHO it should be up to the slave encoder to determine connector and
> encoder type.
Encoder type - yes, but connector type doesn't seem sensible. It's
possible for the TDA998x to be connected to a DVI connector - how
would the slave encoder know that?
>> diff --git a/drivers/gpu/drm/armada/armada_slave.h
>> b/drivers/gpu/drm/armada/armada_slave.h
>> new file mode 100644
>> index 000..1b86696
>> --- /dev/null
>> +++ b/drivers/gpu/drm/armada/armada_slave.h
>> @@ -0,0 +1,26 @@
>> +/*
>> + * Copyright (C) 2012 Russell King
>> + *
>> + * 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.
>> + */
>> +#ifndef ARMADA_TDA19988_H
>> +#define ARMADA_TDA19988_H
>
> nit: ARMADA_SLAVE_H
Nobbled. :)
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH RFC 2/8] DRM: Armada: Add Armada DRM driver
On Tue, Jun 11, 2013 at 12:01:59AM +0200, Daniel Vetter wrote: > On Mon, Jun 10, 2013 at 9:59 PM, Rob Clark wrote: > > On Mon, Jun 10, 2013 at 1:06 PM, Russell King - ARM Linux > > wrote: > >> On Mon, Jun 10, 2013 at 11:57:32AM -0400, Rob Clark wrote: > >>> On Sun, Jun 9, 2013 at 3:29 PM, Russell King > >>> wrote: > >>> > This patch adds support for the pair of LCD controllers on the Marvell > >>> > Armada 510 SoCs. This driver supports: > >>> > - multiple contiguous scanout buffers for video and graphics > >>> > - shm backed cacheable buffer objects for X pixmaps for Vivante GPU > >>> > acceleration > >>> > - dual lcd0 and lcd1 crt operation > >>> > - video overlay on each LCD crt > >>> > >>> Any particular reason for not exposing the overlays as drm_plane's? > >>> That is probably something that should change before merging the > >>> driver. > >> > >> Only through not understanding much of DRM when I started this. > >> Provided DRM planes can do everything that I'm already doing with > >> the overlay support, then yes. Otherwise, I want to stick with this > >> which is modelled after the i915 overlay interface. > > Oh i915 overlays aren't a good reason ;-) I've done those way back > when drm didn't have any plane infrastructure and pretty much as my > very contribution. So totally lacked any clue. > > If I can scrap together a bit of time I want to port the legacy > overlay code over to drm planes (and remap the current ioctl interface > to the drm plane interface for backwards compat). But atm that's > crushed under a giant pile of other todo items. Aren't we all :( The amount of time I have to touch this has reduced substantially over the last couple of months, which is why there's been a few weeks between the RFC's for this. The issue here is that with the overlay interface, all I have to do with one of these pass-by-phys-address things which the X server gets is to: 1. associate the phys address with a gem object 2. pass it in via the overlay interface With the DRM plane stuff, this gets more complicated: 1. associate the phys address with a gem object 2. call another driver private ioctl to bind the gem object to a framebuffer (there is no support for this in the generic ioctl API afaics) which has to allocate and setup a framebuffer 3. use setplane to update the plane That all increases the expense of overlay, and adds further memory allocations to the path (and frees when the previous framebuffer is discarded.) That all makes for higher load due to more CPU expense. ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC v3 0/4] rmk's Dove DRM/TDA19988 Cubox driver
Okay, so the previous set didn't contain all the updates I wanted. That's partly because of the timespan between making those changes and re-posting the RFC. This time, the "Add Armada DRM driver" commit contains all the updates it should've had from last time! However, I'm posting a slightly different branch - it's rooted at the same "Add Armada DRM driver" commit so take patch 1 from this as an update to patch 2 of the previous series. This looks like: + DRM: Armada: Add Armada DRM driver |\ + | drm/i2c: nxp-tda998x patches + | DRM: Armada: add support for drm tda19988 driver + DRM: Armada: Add support for hardware cursors + DRM: Armada: convert Armada hardware cursor support to RGB+transparency + DRM: Armada: convert hardware cursor support to 64x32 or 32x64 ARGB Now, the three additional patches where incrementally add hardware cursor support; first my original version. This didn't work very well. Next is my try with the RGB+transparency suggested in the first RFC round. The cursor image looked silly against some windows because you can't sanely translate an alpha channel to transparency. The last version is what I'm using, and I _am_ using hardware cursors. This seems to be the best solution if we're going to have hardware cursors, even though it means that we don't support 64x64 cursors. For the first patch (the main large patch) the changes are as per my reply to the previous review comments confirming the updates, and a couple of other changes which should've been in the previous RFC: 1. if we map a gem object in kernel space, store its kernel space address. This avoids use of phys_to_virt() in the cursor code which really wasn't nice. 2. initial variant-based clocking support to support non-Dove implementations. ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH RFC 2/8] DRM: Armada: Add Armada DRM driver
On Mon, Jun 10, 2013 at 06:49:06PM -0400, Rob Clark wrote: > I guess in the short term, the best I can think is keep those phys > ioctls as a small patch on top of the upstream driver. It is ok to > leave place-holder ioctl #'s in the upstream driver so that the ioctl > #'s don't shift when you rebase. And then try to get the vendor to > add support for dmabuf so that the patch on top of upstream can > eventually be dropped. Maybe someone else has a better suggestion, > but I don't think we can merge those phys ioctls upstream, and I'd > really hate to 'throw the baby out with the bathwater' in this case > and not at least get the modesetting part of the driver merged. What you're saying is basically: 1. Throw out DRM_ARMADA_GEM_CREATE_PHYS, which cripples video playback on existing gstreamer, xbmc and other implementations without someone rewriting all that code. 2. Throw out DRM_ARMADA_GEM_PROP, which prevents any form of passing the GEM objects to the GPU libraries in userspace, thereby preventing any kind of GPU based acceleration. That makes the driver just be a dumb scanout only driver. Sorry, that *really* does not interest me one bit, because the CPU doing framebuffer accesses is pig slow. There's really no point in such a driver; people might as well just use the standard fbdev driver instead. ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH RFC 2/8] DRM: Armada: Add Armada DRM driver
On Tue, Jun 11, 2013 at 09:24:16AM +1000, Dave Airlie wrote: > I'd like to see all the ARM based drivers based on CMA if it can meet > their requirements > and using close to standard GEM/dma-buf interfaces. Otherwise it'll be > come an unmaintainable > nightmare for everyone, but mostly for me. I am *not* using the CMA layer - that layer is just plain broken in DRM. It forces every single gem object to be a CMA allocated object, which means I can't have cacheable pixmaps in X. And that makes X suck. Okay, I'm pulling this and I'm going to keep it in my private cubox tree; I'm not persuing pushing this driver or any other Armada 510 driver into mainline anymore. It's just too much fscking hastle dealing with people who don't like various stuff. I've done my best to clean a lot of the crap up, and the problem is that no matter how much I clean up, it remains unacceptable. Only the 100% perfect solution seems to be acceptable. That is unacceptable given that this stuff has already consumed something like 8 months solid of my time. So no, I'm not wasting any further time on this crap. ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH RFC 2/8] DRM: Armada: Add Armada DRM driver
On Mon, Jun 10, 2013 at 07:17:22PM -0400, Rob Clark wrote: > On Mon, Jun 10, 2013 at 6:56 PM, Russell King - ARM Linux > wrote: > > On Mon, Jun 10, 2013 at 06:49:06PM -0400, Rob Clark wrote: > >> I guess in the short term, the best I can think is keep those phys > >> ioctls as a small patch on top of the upstream driver. It is ok to > >> leave place-holder ioctl #'s in the upstream driver so that the ioctl > >> #'s don't shift when you rebase. And then try to get the vendor to > >> add support for dmabuf so that the patch on top of upstream can > >> eventually be dropped. Maybe someone else has a better suggestion, > >> but I don't think we can merge those phys ioctls upstream, and I'd > >> really hate to 'throw the baby out with the bathwater' in this case > >> and not at least get the modesetting part of the driver merged. > > > > What you're saying is basically: > > > > 1. Throw out DRM_ARMADA_GEM_CREATE_PHYS, which cripples video playback > >on existing gstreamer, xbmc and other implementations without someone > >rewriting all that code. > > > > 2. Throw out DRM_ARMADA_GEM_PROP, which prevents any form of passing > >the GEM objects to the GPU libraries in userspace, thereby preventing > >any kind of GPU based acceleration. > > > > That makes the driver just be a dumb scanout only driver. Sorry, > > that *really* does not interest me one bit, because the CPU doing > > framebuffer accesses is pig slow. > > Well, yes that is basically what I am saying, unless we can find a > different way (dmabuf? if there is some way to pass it through the > blob bits you don't have src code for?) > > The problem is that we really can't merge something upstream that lets > you dma to arbitrary physical address. Maybe in staging, perhaps? Or Which bit of "THIS DRIVER DOESN'T DMA _TO_ ANY ADDRESS" did you not yet? ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH RFC 2/8] DRM: Armada: Add Armada DRM driver
On Tue, Jun 11, 2013 at 09:48:57AM +1000, Dave Airlie wrote: > On Tue, Jun 11, 2013 at 9:36 AM, Russell King - ARM Linux > wrote: > > On Tue, Jun 11, 2013 at 09:24:16AM +1000, Dave Airlie wrote: > >> I'd like to see all the ARM based drivers based on CMA if it can meet > >> their requirements > >> and using close to standard GEM/dma-buf interfaces. Otherwise it'll be > >> come an unmaintainable > >> nightmare for everyone, but mostly for me. > > > > I am *not* using the CMA layer - that layer is just plain broken in > > DRM. It forces every single gem object to be a CMA allocated object, > > which means I can't have cacheable pixmaps in X. And that makes X > > suck. > > > > Okay, I'm pulling this and I'm going to keep it in my private cubox > > tree; I'm not persuing pushing this driver or any other Armada 510 > > driver into mainline anymore. It's just too much fscking hastle > > dealing with people who don't like various stuff. > > > > I've done my best to clean a lot of the crap up, and the problem is > > that no matter how much I clean up, it remains unacceptable. Only > > the 100% perfect solution seems to be acceptable. That is > > unacceptable given that this stuff has already consumed something > > like 8 months solid of my time. > > Russell, aren't you a kernel maintainer, because for fuck sake get real. > > I'm not merging bullshit into my tree that has a completely broken API that > has to be maintained for ever. You of all people should understand we > don't break Linux > userspace APIs, and adding a phys addr one is wrong, wrong, wrong, its not > cleanups, its just broken, and I'll never merge it. As I say, I'm no longer interested in pushing this into mainline. I've said my piece above, and I'm not changing that. I'm not spending years trying to sort this stuff out, by which time the platforms using it will be long since obsolete. The "8 months" is not an exaggeration. That's the time taken to sort out the kernel side, the libdrm stuff, the X server driver, and get video decode working on these platforms with vlc. It works for me, and that's really at this point all I care about. And yes, I'm a kernel maintainer. A FUCKING busy one right now at that. I haven't stopped working since 9am today yet, and it's now 1am. Do the maths. I really wish I had some time to myself now to think about some of these bigger issues which this has brought up, but I don't. And no, I *never* said that adding a phys address was a cleanup. Sheesh why don't you read my emails properly. Another reason why I won't be continuing this discussion much further. I said that _this_ amount of effort is a result of doing *LOTS* of cleanups. A DRM driver for this hardware *didn't* exist until I wrote it. There was a fbdev driver before, which was passed phys addresses there for the overlay, and passed phys addresses back out after the overlay frame has been replaced. ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH RFC 2/8] DRM: Armada: Add Armada DRM driver
On Tue, Jun 11, 2013 at 09:48:57AM +1000, Dave Airlie wrote: > On Tue, Jun 11, 2013 at 9:36 AM, Russell King - ARM Linux > wrote: > > On Tue, Jun 11, 2013 at 09:24:16AM +1000, Dave Airlie wrote: > >> I'd like to see all the ARM based drivers based on CMA if it can meet > >> their requirements > >> and using close to standard GEM/dma-buf interfaces. Otherwise it'll be > >> come an unmaintainable > >> nightmare for everyone, but mostly for me. > > > > I am *not* using the CMA layer - that layer is just plain broken in > > DRM. It forces every single gem object to be a CMA allocated object, > > which means I can't have cacheable pixmaps in X. And that makes X > > suck. > > > > Okay, I'm pulling this and I'm going to keep it in my private cubox > > tree; I'm not persuing pushing this driver or any other Armada 510 > > driver into mainline anymore. It's just too much fscking hastle > > dealing with people who don't like various stuff. > > > > I've done my best to clean a lot of the crap up, and the problem is > > that no matter how much I clean up, it remains unacceptable. Only > > the 100% perfect solution seems to be acceptable. That is > > unacceptable given that this stuff has already consumed something > > like 8 months solid of my time. > > Russell, aren't you a kernel maintainer, because for fuck sake get real. > > I'm not merging bullshit into my tree that has a completely broken API that > has to be maintained for ever. You of all people should understand we > don't break Linux > userspace APIs, and adding a phys addr one is wrong, wrong, wrong, its not > cleanups, its just broken, and I'll never merge it. And having thought about this driver, DRM some more, I'm now of the opinion that DRM is not suitable for driving hardware where the GPU is an entirely separate IP block from the display side. DRM is modelled after the PC setup where your "graphics card" does everything - it has the GPU, display and connectors all integrated together. This is not the case on embedded SoCs, which can be a collection of different IPs all integrated together. DRM is based on the assumption that you have a single card and everything is known about that card. Again, this is not the case with embedded SoCs, which is why Sebastian is having a hard time with the DRM slave encoder stuff. If DRM is going to be usable on SoCs, it needs to become more modular in nature, allowing the same scanout stuff to be used with different GPUs and providing _kernel_ side interfaces to allow different GPUs to be plugged in to a scanout implementation, or vice versa (the reverse is probably easier because the scanout interface is nicely abstracted). Or we go off and write an entirely new subsystem which *does* suit the needs of modular SoC implementations. ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH RFC 2/8] DRM: Armada: Add Armada DRM driver
On Wed, Jun 12, 2013 at 09:56:22AM -0400, Rob Clark wrote: > On Wed, Jun 12, 2013 at 9:48 AM, Russell King - ARM Linux > wrote: > > And having thought about this driver, DRM some more, I'm now of the > > opinion that DRM is not suitable for driving hardware where the GPU is > > an entirely separate IP block from the display side. > > > > DRM is modelled after the PC setup where your "graphics card" does > > everything - it has the GPU, display and connectors all integrated > > together. This is not the case on embedded SoCs, which can be a > > collection of different IPs all integrated together. > > actually it isn't even the case on desktop/laptop anymore, where you > can have one gpu with scanout and a second one without (or just with > display controller not hooked up to anything, etc, etc) > > That is the point of dmabuf and the upcoming fence/reservation stuff. Okay, but dmabuf really needs to be fixed, because as it stands this API is really quite broken wrt the DMA API. dma_map_sg() is (a) not supposed to have its return value ignored - mappings can fail, and (b) the returned number indicates how many entries are valid for the _mapped_ version of the scatterlist. Both these points are important if your DMA API implementation uses an IOMMU, which may coalesce the scatterlist array when creating mappings - much as described in Documentation/DMA-API.txt and Documentation/DMA-API-HOWTO.txt. That is not all DRMs fault - (a) is attributable to DRM's prime implementation: sgt = obj->dev->driver->gem_prime_get_sg_table(obj); if (!IS_ERR_OR_NULL(sgt)) dma_map_sg(attach->dev, sgt->sgl, sgt->nents, dir); and quite why it does the dma_map_sg() beneath the struct_mutex is beyond me - if the array of pages isn't safe without the mutex being held, then it isn't safe after the dma_map_sg() operation has completed and the mutex has been released. However, (b) is more a problem for dmabuf (which I just rather aptly mistyped as dmabug) because either dmabuf's .map_dma_buf method needs to return the value from dma_map_sg(), or it needs to stop requiring this of the .map_dma_buf method and have it done by the caller of this method so the caller can have access to that returned value. Added Sumit Semwal to this email for the dmabuf issue. Thankfully, this being correct isn't a requirement for me, but it's something which _should_ be fixed. ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH RFC 2/8] DRM: Armada: Add Armada DRM driver
On Wed, Jun 12, 2013 at 05:49:14PM +0100, Russell King - ARM Linux wrote: > On Wed, Jun 12, 2013 at 09:56:22AM -0400, Rob Clark wrote: > > On Wed, Jun 12, 2013 at 9:48 AM, Russell King - ARM Linux > > wrote: > > > And having thought about this driver, DRM some more, I'm now of the > > > opinion that DRM is not suitable for driving hardware where the GPU is > > > an entirely separate IP block from the display side. > > > > > > DRM is modelled after the PC setup where your "graphics card" does > > > everything - it has the GPU, display and connectors all integrated > > > together. This is not the case on embedded SoCs, which can be a > > > collection of different IPs all integrated together. > > > > actually it isn't even the case on desktop/laptop anymore, where you > > can have one gpu with scanout and a second one without (or just with > > display controller not hooked up to anything, etc, etc) > > > > That is the point of dmabuf and the upcoming fence/reservation stuff. > > Okay, but dmabuf really needs to be fixed, because as it stands this API > is really quite broken wrt the DMA API. dma_map_sg() is (a) not supposed > to have its return value ignored - mappings can fail, and (b) the returned > number indicates how many entries are valid for the _mapped_ version of > the scatterlist. > > Both these points are important if your DMA API implementation uses an > IOMMU, which may coalesce the scatterlist array when creating mappings - > much as described in Documentation/DMA-API.txt and > Documentation/DMA-API-HOWTO.txt. > > That is not all DRMs fault - (a) is attributable to DRM's prime > implementation: > > sgt = obj->dev->driver->gem_prime_get_sg_table(obj); > > if (!IS_ERR_OR_NULL(sgt)) > dma_map_sg(attach->dev, sgt->sgl, sgt->nents, dir); > > and quite why it does the dma_map_sg() beneath the struct_mutex is > beyond me - if the array of pages isn't safe without the mutex being > held, then it isn't safe after the dma_map_sg() operation has completed > and the mutex has been released. > > However, (b) is more a problem for dmabuf (which I just rather aptly > mistyped as dmabug) because either dmabuf's .map_dma_buf method needs > to return the value from dma_map_sg(), or it needs to stop requiring > this of the .map_dma_buf method and have it done by the caller of > this method so the caller can have access to that returned value. > > Added Sumit Semwal to this email for the dmabuf issue. > > Thankfully, this being correct isn't a requirement for me, but it's > something which _should_ be fixed. Okay, so Sumit Semwal has been a victim of TI's cuts, so don't expect dma_buf to get sorted by the original author... Now we come to this: drivers/gpu/drm/exynos/exynos_drm_dmabuf.c: return dma_buf_export(exynos_gem_obj, &exynos_dmabuf_ops, drivers/gpu/drm/exynos/exynos_drm_dmabuf.c- exynos_gem_obj->base.size, flags); drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c: return dma_buf_export(obj, &omap_dmabuf_ops, obj->size, flags); drivers/gpu/drm/drm_prime.c:return dma_buf_export(obj, &drm_gem_prime_dmabuf_ops, obj->size, drivers/gpu/drm/drm_prime.c- 0600); drivers/gpu/drm/i915/i915_gem_dmabuf.c: return dma_buf_export(obj, &i915_dmabuf_ops, obj->base.size, flags); Of the three implementations which don't use the generic version, they all pass 'flags' to dma_buf_export. drm_prime.c doesn't, it passes a fixed file mode. What's the correct version, or is flags | 0600 actually the right one (as flags only contains O_CLOEXEC)? ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH RFC 2/8] DRM: Armada: Add Armada DRM driver
On Wed, Jun 12, 2013 at 06:05:12PM +0100, Russell King - ARM Linux wrote:
> On Wed, Jun 12, 2013 at 05:49:14PM +0100, Russell King - ARM Linux wrote:
> > On Wed, Jun 12, 2013 at 09:56:22AM -0400, Rob Clark wrote:
> > > On Wed, Jun 12, 2013 at 9:48 AM, Russell King - ARM Linux
> > > wrote:
> > > > And having thought about this driver, DRM some more, I'm now of the
> > > > opinion that DRM is not suitable for driving hardware where the GPU is
> > > > an entirely separate IP block from the display side.
> > > >
> > > > DRM is modelled after the PC setup where your "graphics card" does
> > > > everything - it has the GPU, display and connectors all integrated
> > > > together. This is not the case on embedded SoCs, which can be a
> > > > collection of different IPs all integrated together.
> > >
> > > actually it isn't even the case on desktop/laptop anymore, where you
> > > can have one gpu with scanout and a second one without (or just with
> > > display controller not hooked up to anything, etc, etc)
> > >
> > > That is the point of dmabuf and the upcoming fence/reservation stuff.
> >
> > Okay, but dmabuf really needs to be fixed, because as it stands this API
> > is really quite broken wrt the DMA API. dma_map_sg() is (a) not supposed
> > to have its return value ignored - mappings can fail, and (b) the returned
> > number indicates how many entries are valid for the _mapped_ version of
> > the scatterlist.
> >
> > Both these points are important if your DMA API implementation uses an
> > IOMMU, which may coalesce the scatterlist array when creating mappings -
> > much as described in Documentation/DMA-API.txt and
> > Documentation/DMA-API-HOWTO.txt.
> >
> > That is not all DRMs fault - (a) is attributable to DRM's prime
> > implementation:
> >
> > sgt = obj->dev->driver->gem_prime_get_sg_table(obj);
> >
> > if (!IS_ERR_OR_NULL(sgt))
> > dma_map_sg(attach->dev, sgt->sgl, sgt->nents, dir);
> >
> > and quite why it does the dma_map_sg() beneath the struct_mutex is
> > beyond me - if the array of pages isn't safe without the mutex being
> > held, then it isn't safe after the dma_map_sg() operation has completed
> > and the mutex has been released.
> >
> > However, (b) is more a problem for dmabuf (which I just rather aptly
> > mistyped as dmabug) because either dmabuf's .map_dma_buf method needs
> > to return the value from dma_map_sg(), or it needs to stop requiring
> > this of the .map_dma_buf method and have it done by the caller of
> > this method so the caller can have access to that returned value.
> >
> > Added Sumit Semwal to this email for the dmabuf issue.
> >
> > Thankfully, this being correct isn't a requirement for me, but it's
> > something which _should_ be fixed.
>
> Okay, so Sumit Semwal has been a victim of TI's cuts, so don't expect
> dma_buf to get sorted by the original author...
And now the next muckup in dma-buf/drm_prime - though it's arguably the
fault of IS_ERR_OR_NULL existing in the first place:
drm_gem_prime_import()
{
sgt = dma_buf_map_attachment(attach, DMA_BIDIRECTIONAL);
if (IS_ERR_OR_NULL(sgt)) {
ret = PTR_ERR(sgt);
goto fail_detach;
}
fail_detach:
dma_buf_detach(dma_buf, attach);
return ERR_PTR(ret);
}
What happens if sgt is NULL here? What value does ret get? What does
drm_gem_prime_import() return? How is that interpreted by
drm_gem_prime_fd_to_handle() ?
I can probably oops a kernel running DRM through this if I can manipulate
a dma_buf_map_attachment() to return NULL (there probably is a driver
which is capable of doing so.)
APIs which use IS_ERR_OR_NULL() are problems waiting to happen; we've
already reached some concensus a while back to get rid of this helper,
if only it wasn't soo prolific (like its errors are.)
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH RFC 2/8] DRM: Armada: Add Armada DRM driver
And here's another one - look carefully at this path:
buf = dev->driver->gem_prime_export(dev, obj, flags);
if (IS_ERR(buf)) {
/* normally the created dma-buf takes ownership of the
ref,
* but if that fails then drop the ref
*/
drm_gem_object_unreference_unlocked(obj);
mutex_unlock(&file_priv->prime.lock);
return PTR_ERR(buf);
}
obj->export_dma_buf = buf;
*prime_fd = dma_buf_fd(buf, flags);
}
/* if we've exported this buffer the cheat and add it to the import list
* so we get the correct handle back
*/
ret = drm_prime_add_imported_buf_handle(&file_priv->prime,
obj->export_dma_buf, handle);
if (ret) {
drm_gem_object_unreference_unlocked(obj);
mutex_unlock(&file_priv->prime.lock);
return ret;
}
So in the event of drm_prime_add_imported_buf_handle() returning an error,
we return that error to our caller (which will eventually be userspace)
saying that we failed.
However, we've already setup the export and obtained an fd for it, which
we resultingly now leak in that situation.
Now, second problem here - consider what happens if you ask for the same
object to be exported a second (or more) times. Note that
obj->export_dma_buf will now be set, so we take a different path through
this code:
if (obj->export_dma_buf) {
get_dma_buf(obj->export_dma_buf);
*prime_fd = dma_buf_fd(obj->export_dma_buf, flags);
drm_gem_object_unreference_unlocked(obj);
} else {
}
/* if we've exported this buffer the cheat and add it to the import list
* so we get the correct handle back
*/
ret = drm_prime_add_imported_buf_handle(&file_priv->prime,
obj->export_dma_buf, handle);
if (ret) {
drm_gem_object_unreference_unlocked(obj);
mutex_unlock(&file_priv->prime.lock);
return ret;
}
Now, if I in a loop in userspace doing this:
do {
drmPrimeHandleToFD(..., &fd);
close(fd);
} while (1);
How long do you think it might take to exhaust the kmalloc() inside
drm_prime_add_imported_buf_handle() ?
It's not even trivially fixable, because drm_gem_dmabuf_release() doesn't
call drm_prime_remove_imported_buf_handle() because it has no knowledge
of the drm_prime_file_private structure to search for these things...
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH RFC 2/8] DRM: Armada: Add Armada DRM driver
And another issue...
What is drm_crtc_helper_set_mode() passed as the fb argument? Is it
the old fb, or the new fb?
bool drm_crtc_helper_set_mode(struct drm_crtc *crtc,
struct drm_display_mode *mode,
int x, int y,
struct drm_framebuffer *old_fb)
...
int drm_crtc_helper_set_config(struct drm_mode_set *set)
{
...
save_set.fb = set->crtc->fb;
...
old_fb = set->crtc->fb;
set->crtc->fb = set->fb;
if (!drm_crtc_helper_set_mode(set->crtc, set->mode,
set->x, set->y,
old_fb)) {
...
/* Try to restore the config */
if (mode_changed &&
!drm_crtc_helper_set_mode(save_set.crtc, save_set.mode, save_set.x,
save_set.y, save_set.fb))
}
...
int drm_helper_resume_force_mode(struct drm_device *dev)
{
...
ret = drm_crtc_helper_set_mode(crtc, &crtc->mode,
crtc->x, crtc->y, crtc->fb);
The function prototype implies it's the old fb, as does the first use.
The second and third uses of it clearly show it being the fb which we
wish to be displayed.
The deeper I look, the more bugs there seem to be in this DRM stuff,
and I'm continuing to look because I'm chasing a framebuffer refcount
bug.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH RFC 2/8] DRM: Armada: Add Armada DRM driver
On Thu, Jun 13, 2013 at 12:19:03PM +0100, Russell King - ARM Linux wrote:
> The deeper I look, the more bugs there seem to be in this DRM stuff,
> and I'm continuing to look because I'm chasing a framebuffer refcount
> bug.
So, this refcount bug - I think I've just found it. This is the flow of
references to the new fb on mode set:
drm_mode_setcrtc():
fb = drm_framebuffer_lookup(dev, crtc_req->fb_id);
set.fb = fb;
ret = drm_mode_set_config_internal(&set);
drm_mode_set_config_internal():
fb = set->fb;
ret = crtc->funcs->set_config(set);
drm_crtc_helper_set_config():
old_fb = set->crtc->fb;
set->crtc->fb = set->fb;
if (!drm_crtc_helper_set_mode(set->crtc, set->mode,
set->x, set->y,
old_fb)) {
drm_helper_disable_unused_functions(dev);
drm_helper_disable_unused_functions():
list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
crtc->enabled = drm_helper_crtc_in_use(crtc);
if (!crtc->enabled) {
crtc->fb = NULL;
}
}
back to drm_mode_set_config_internal():
if (ret == 0) {
if (fb)
drm_framebuffer_reference(fb);
back to drm_mode_setcrtc():
if (fb)
drm_framebuffer_unreference(fb);
Assuming success all the way through, what happens when a CRTC is unused
is:
1. We obtain a reference in drm_mode_setcrtc() via the lookup.
2. We set the mode
3. In trying to set the mode, we discover that all connectors for the CRTC
are in the disconnected state, and so we disable the CRTC
4. We set crtc->fb to NULL
5. back in drm_mode_set_config_internal(), we take a reference on the
framebuffer irrespective of this.
6. back in drm_mode_setcrtc(), we drop the original reference caused by
the lookup.
We now have a framebuffer with a reference count incremented by one but
no actual reference to it - the CRTC's reference is completely lost by
the action of drm_helper_disable_unused_functions().
You could argue that it's something the driver should deal with - fine,
but what if it only implements the DPMS method? Should it drop a
reference to the framebuffer when DPMS instructs it to turn off? Surely
not, because that means when DPMS turns stuff back on you're missing a
refcount.
Are drivers required to implement a disable function and cater for the
imbalance in the upper layers of code? If so, this is not a clean
design.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH RFC 2/8] DRM: Armada: Add Armada DRM driver
On Thu, Jun 13, 2013 at 12:50:16PM +0100, Russell King - ARM Linux wrote:
> On Thu, Jun 13, 2013 at 12:19:03PM +0100, Russell King - ARM Linux wrote:
> > The deeper I look, the more bugs there seem to be in this DRM stuff,
> > and I'm continuing to look because I'm chasing a framebuffer refcount
> > bug.
>
> So, this refcount bug - I think I've just found it. This is the flow of
> references to the new fb on mode set:
>
> drm_mode_setcrtc():
> fb = drm_framebuffer_lookup(dev, crtc_req->fb_id);
> set.fb = fb;
> ret = drm_mode_set_config_internal(&set);
> drm_mode_set_config_internal():
> fb = set->fb;
> ret = crtc->funcs->set_config(set);
> drm_crtc_helper_set_config():
> old_fb = set->crtc->fb;
> set->crtc->fb = set->fb;
> if (!drm_crtc_helper_set_mode(set->crtc, set->mode,
> set->x, set->y,
> old_fb)) {
> drm_helper_disable_unused_functions(dev);
> drm_helper_disable_unused_functions():
> list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
> crtc->enabled = drm_helper_crtc_in_use(crtc);
> if (!crtc->enabled) {
> crtc->fb = NULL;
> }
> }
> back to drm_mode_set_config_internal():
> if (ret == 0) {
> if (fb)
> drm_framebuffer_reference(fb);
> back to drm_mode_setcrtc():
> if (fb)
> drm_framebuffer_unreference(fb);
>
> Assuming success all the way through, what happens when a CRTC is unused
> is:
>
> 1. We obtain a reference in drm_mode_setcrtc() via the lookup.
> 2. We set the mode
> 3. In trying to set the mode, we discover that all connectors for the CRTC
>are in the disconnected state, and so we disable the CRTC
> 4. We set crtc->fb to NULL
> 5. back in drm_mode_set_config_internal(), we take a reference on the
>framebuffer irrespective of this.
> 6. back in drm_mode_setcrtc(), we drop the original reference caused by
>the lookup.
>
> We now have a framebuffer with a reference count incremented by one but
> no actual reference to it - the CRTC's reference is completely lost by
> the action of drm_helper_disable_unused_functions().
>
> You could argue that it's something the driver should deal with - fine,
> but what if it only implements the DPMS method? Should it drop a
> reference to the framebuffer when DPMS instructs it to turn off? Surely
> not, because that means when DPMS turns stuff back on you're missing a
> refcount.
>
> Are drivers required to implement a disable function and cater for the
> imbalance in the upper layers of code? If so, this is not a clean
> design.
There's a bigger issue here - if it's possible for drm_crtc_helper_set_config()
to be called with set->fb set but set->mode NULL, then we overwrite
set->fb to NULL. Again, that results in a lost reference.
For the time being, I'm using this patch, which solves my dropped
refcount problem, and marks the other possible dropped reference.
Either that check needs to be removed or it needs to properly drop
the refcount on the fb before 'losing' the reference to it.
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index dd64a06..774d7a6 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -2014,13 +2014,16 @@ int drm_mode_set_config_internal(struct drm_mode_set
*set)
old_fb = crtc->fb;
fb = set->fb;
+ if (fb)
+ drm_framebuffer_reference(fb);
ret = crtc->funcs->set_config(set);
if (ret == 0) {
if (old_fb)
drm_framebuffer_unreference(old_fb);
+ } else {
if (fb)
- drm_framebuffer_reference(fb);
+ drm_framebuffer_unreference(fb);
}
return ret;
diff --git a/drivers/gpu/drm/drm_crtc_helper.c
b/drivers/gpu/drm/drm_crtc_helper.c
index 7b2d378..0d18fb2 100644
--- a/drivers/gpu/drm/drm_crtc_helper.c
+++ b/drivers/gpu/drm/drm_crtc_helper.c
@@ -299,6 +299,8 @@ void drm_helper_disable_unused_functions(struct drm_device
*dev)
(*crtc_funcs->disable)(crtc);
else
(*crtc_funcs->dpms)(crtc, DRM_MODE_DPMS_OFF);
+ if (crtc->fb)
+ drm_framebuffer_unreference(crtc->fb);
crtc->fb = NULL
Re: [RFC PATCH] dmabuf-sync: Introduce buffer synchronization framework
On Thu, Jun 13, 2013 at 05:28:08PM +0900, Inki Dae wrote: > This patch adds a buffer synchronization framework based on DMA BUF[1] > and reservation[2] to use dma-buf resource, and based on ww-mutexes[3] > for lock mechanism. > > The purpose of this framework is not only to couple cache operations, > and buffer access control to CPU and DMA but also to provide easy-to-use > interfaces for device drivers and potentially user application > (not implemented for user applications, yet). And this framework can be > used for all dma devices using system memory as dma buffer, especially > for most ARM based SoCs. > > The mechanism of this framework has the following steps, > 1. Register dmabufs to a sync object - A task gets a new sync object and > can add one or more dmabufs that the task wants to access. > This registering should be performed when a device context or an event > context such as a page flip event is created or before CPU accesses a > shared > buffer. > > dma_buf_sync_get(a sync object, a dmabuf); > > 2. Lock a sync object - A task tries to lock all dmabufs added in its own > sync object. Basically, the lock mechanism uses ww-mutex[1] to avoid dead > lock issue and for race condition between CPU and CPU, CPU and DMA, and > DMA > and DMA. Taking a lock means that others cannot access all locked dmabufs > until the task that locked the corresponding dmabufs, unlocks all the > locked > dmabufs. > This locking should be performed before DMA or CPU accesses these dmabufs. > > dma_buf_sync_lock(a sync object); > > 3. Unlock a sync object - The task unlocks all dmabufs added in its own > sync > object. The unlock means that the DMA or CPU accesses to the dmabufs have > been completed so that others may access them. > This unlocking should be performed after DMA or CPU has completed accesses > to the dmabufs. > > dma_buf_sync_unlock(a sync object); > > 4. Unregister one or all dmabufs from a sync object - A task unregisters > the given dmabufs from the sync object. This means that the task dosen't > want to lock the dmabufs. > The unregistering should be performed after DMA or CPU has completed > accesses to the dmabufs or when dma_buf_sync_lock() is failed. > > dma_buf_sync_put(a sync object, a dmabuf); > dma_buf_sync_put_all(a sync object); > > The described steps may be summarized as: > get -> lock -> CPU or DMA access to a buffer/s -> unlock -> put > > This framework includes the following two features. > 1. read (shared) and write (exclusive) locks - A task is required to > declare > the access type when the task tries to register a dmabuf; > READ, WRITE, READ DMA, or WRITE DMA. > > The below is example codes, > struct dmabuf_sync *sync; > > sync = dmabuf_sync_init(NULL, "test sync"); > > dmabuf_sync_get(sync, dmabuf, DMA_BUF_ACCESS_READ); > ... > > And the below can be used as access types: > DMA_BUF_ACCESS_READ, > - CPU will access a buffer for read. > DMA_BUF_ACCESS_WRITE, > - CPU will access a buffer for read or write. > DMA_BUF_ACCESS_READ | DMA_BUF_ACCESS_DMA, > - DMA will access a buffer for read > DMA_BUF_ACCESS_WRITE | DMA_BUF_ACCESS_DMA, > - DMA will access a buffer for read or write. > > 2. Mandatory resource releasing - a task cannot hold a lock indefinitely. > A task may never try to unlock a buffer after taking a lock to the buffer. > In this case, a timer handler to the corresponding sync object is called > in five (default) seconds and then the timed-out buffer is unlocked by > work > queue handler to avoid lockups and to enforce resources of the buffer. > > The below is how to use: > 1. Allocate and Initialize a sync object: > struct dmabuf_sync *sync; > > sync = dmabuf_sync_init(NULL, "test sync"); > ... > > 2. Add a dmabuf to the sync object when setting up dma buffer relevant > registers: > dmabuf_sync_get(sync, dmabuf, DMA_BUF_ACCESS_READ); > ... > > 3. Lock all dmabufs of the sync object before DMA or CPU accesses > the dmabufs: > dmabuf_sync_lock(sync); > ... > > 4. Now CPU or DMA can access all dmabufs locked in step 3. > > 5. Unlock all dmabufs added in a sync object after DMA or CPU access > to these dmabufs is completed: > dmabu
Re: [PATCH RFC 2/8] DRM: Armada: Add Armada DRM driver
On Fri, Jun 14, 2013 at 03:53:41PM +0200, Daniel Vetter wrote:
> On Thu, Jun 13, 2013 at 12:50:16PM +0100, Russell King - ARM Linux wrote:
> > On Thu, Jun 13, 2013 at 12:19:03PM +0100, Russell King - ARM Linux wrote:
> > > The deeper I look, the more bugs there seem to be in this DRM stuff,
> > > and I'm continuing to look because I'm chasing a framebuffer refcount
> > > bug.
> >
> > So, this refcount bug - I think I've just found it. This is the flow of
> > references to the new fb on mode set:
> >
> > drm_mode_setcrtc():
> > fb = drm_framebuffer_lookup(dev, crtc_req->fb_id);
> > set.fb = fb;
> > ret = drm_mode_set_config_internal(&set);
> > drm_mode_set_config_internal():
> > fb = set->fb;
> > ret = crtc->funcs->set_config(set);
> > drm_crtc_helper_set_config():
> > old_fb = set->crtc->fb;
> > set->crtc->fb = set->fb;
> > if (!drm_crtc_helper_set_mode(set->crtc, set->mode,
> > set->x, set->y,
> > old_fb)) {
> > drm_helper_disable_unused_functions(dev);
> > drm_helper_disable_unused_functions():
> > list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
> > crtc->enabled = drm_helper_crtc_in_use(crtc);
> > if (!crtc->enabled) {
> > crtc->fb = NULL;
> > }
> > }
> > back to drm_mode_set_config_internal():
> > if (ret == 0) {
> > if (fb)
> > drm_framebuffer_reference(fb);
> > back to drm_mode_setcrtc():
> > if (fb)
> > drm_framebuffer_unreference(fb);
> >
> > Assuming success all the way through, what happens when a CRTC is unused
> > is:
> >
> > 1. We obtain a reference in drm_mode_setcrtc() via the lookup.
> > 2. We set the mode
> > 3. In trying to set the mode, we discover that all connectors for the CRTC
> >are in the disconnected state, and so we disable the CRTC
> > 4. We set crtc->fb to NULL
> > 5. back in drm_mode_set_config_internal(), we take a reference on the
> >framebuffer irrespective of this.
> > 6. back in drm_mode_setcrtc(), we drop the original reference caused by
> >the lookup.
> >
> > We now have a framebuffer with a reference count incremented by one but
> > no actual reference to it - the CRTC's reference is completely lost by
> > the action of drm_helper_disable_unused_functions().
> >
> > You could argue that it's something the driver should deal with - fine,
> > but what if it only implements the DPMS method? Should it drop a
> > reference to the framebuffer when DPMS instructs it to turn off? Surely
> > not, because that means when DPMS turns stuff back on you're missing a
> > refcount.
> >
> > Are drivers required to implement a disable function and cater for the
> > imbalance in the upper layers of code? If so, this is not a clean
> > design.
>
> Yep, if your driver grabs additional references (underlying gem object,
> pinning, whatever) you need to wire up your own ->disable hook to drop
> those. Note that for truly dumb kms drivers which only ever allocate an
> fb, the upper layer actually _does_ take care of all the refcounting.
Look again at what I said above - that is _not_ the case. My above
analysis is totally without considering any refcounting that the
driver itself may or may not do. If the driver does no refcounting on
the framebuffer object, then the leak is present.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH RFC 2/8] DRM: Armada: Add Armada DRM driver
On Fri, Jun 14, 2013 at 04:23:22PM +0200, Daniel Vetter wrote: > On Thu, Jun 13, 2013 at 3:03 PM, Russell King - ARM Linux > wrote: > > There's a bigger issue here - if it's possible for > > drm_crtc_helper_set_config() > > to be called with set->fb set but set->mode NULL, then we overwrite > > set->fb to NULL. Again, that results in a lost reference. > > > > For the time being, I'm using this patch, which solves my dropped > > refcount problem, and marks the other possible dropped reference. > > Either that check needs to be removed or it needs to properly drop > > the refcount on the fb before 'losing' the reference to it. > > Scrap my other mail, I see now where the leaking happens. One of them > is interface abuse which is now fixed (and i915 has a bunch of BUG_ONs > to enforce them). The other one is indeed a real case that eluded me > when I've done the refcountification for drm_framebuffers. I'll hack > up some patches, since this seems to be a good excuse to port some of > the i915 modeset improvements back to the crtc helpers. If you're happy with the patch I supplied, that's probably the minimal fix which should go to stable kernels (I'm using 3.9 here) - this also counts as a "user visible bug". It's something I've tripped over which causes exhausts memory and can prevent the X server from starting up. If you want me to package the patch up with a commit message and sign-off... ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH RFC 2/8] DRM: Armada: Add Armada DRM driver
On Fri, Jun 14, 2013 at 09:50:22PM +0200, Daniel Vetter wrote: > On Fri, Jun 14, 2013 at 4:42 PM, Russell King - ARM Linux > wrote: > > If you're happy with the patch I supplied, that's probably the minimal fix > > which should go to stable kernels (I'm using 3.9 here) - this also counts > > as a "user visible bug". It's something I've tripped over which causes > > exhausts memory and can prevent the X server from starting up. > > > > If you want me to package the patch up with a commit message and sign-off.. > > Your patch doesn't fix drm/i915 (since we don't use the crtc helpers > any more). And I don't think it's good to have the refcounting > partially in the drm core and partially in drivers. Let me check what you mean by that. I hope you mean that the drm core, drm helpers and drivers are individually responsible for dropping any refcount that they obtain on any object. In other words, if the drm core takes a refcount on object X, then the DRM core must be the code base which drops that refcount. ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC PATCH v2] dmabuf-sync: Introduce buffer synchronization framework
On Mon, Jun 17, 2013 at 10:04:45PM +0900, Inki Dae wrote: > It's just to implement a thin sync framework coupling cache operation. This > approach is based on dma-buf for more generic implementation against android > sync driver or KDS. > > The described steps may be summarized as: > lock -> cache operation -> CPU or DMA access to a buffer/s -> unlock > > I think that there is no need to get complicated for such approach at least > for most devices sharing system memory. Simple is best. But hang on, doesn't the dmabuf API already provide that? The dmabuf API already uses dma_map_sg() and dma_unmap_sg() by providers, and the rules around the DMA API are that: dma_map_sg() /* DMA _ONLY_ has access, CPU should not access */ dma_unmap_sg() /* DMA may not access, CPU can access */ It's a little more than that if you include the sync_sg_for_cpu and sync_sg_for_device APIs too - but the above is the general idea. What this means from the dmabuf API point of view is that once you attach to a dma_buf, and call dma_buf_map_attachment() to get the SG list, the CPU doesn't have ownership of the buffer and _must_ _not_ access it via any other means - including using the other dma_buf methods, until either the appropriate dma_sync_sg_for_cpu() call has been made or the DMA mapping has been removed via dma_buf_unmap_attachment(). So, the sequence should be: dma_buf_map_attachment() /* do DMA */ dma_buf_unmap_attachment() /* CPU can now access the buffer */ ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC PATCH v2] dmabuf-sync: Introduce buffer synchronization framework
On Tue, Jun 18, 2013 at 12:03:31AM +0900, Inki Dae wrote:
> 2013/6/17 Russell King - ARM Linux
> Exactly right. But that is not definitely my point. Could you please see
> the below simple example?:
> (Presume that CPU and DMA share a buffer and the buffer is mapped with user
> space as cachable)
>
> handle1 = drm_gem_fd_to_handle(a dmabuf fd); > 1
> ...
> va1 = drm_gem_mmap(handle1);
> va2 = drm_gem_mmap(handle2);
> va3 = malloc(size);
> ...
>
> while (conditions) {
> memcpy(va1, some data, size);
No!
Well, the first thing to say here is that under the requirements of the
DMA API, the above is immediately invalid, because you're writing to a
buffer which under the terms of the DMA API is currently owned by the
DMA agent, *not* by the CPU. You're supposed to call dma_sync_sg_for_cpu()
before you do that - but how is userspace supposed to know that requirement?
Why should userspace even _have_ to know these requirements of the DMA
API?
It's also entirely possible that drm_gem_fd_to_handle() (which indirectly
causes dma_map_sg() on the buffers scatterlist) followed by mmap'ing it
into userspace is a bug too, as it has the potential to touch caches or
stuff in ways that maybe the DMA or IOMMU may not expect - but I'm not
going to make too big a deal about that, because I don't think we have
anything that picky.
However, the first point above is the most important one, and exposing
the quirks of the DMA API to userland is certainly not a nice thing to be
doing. This needs to be fixed - we can't go and enforce an API which is
deeply embedded within the kernel all the way out to userland.
What we need is something along the lines of:
(a) dma_buf_map_attachment() _not_ to map the scatterlist for DMA.
or
(b) drm_gem_prime_import() not to call dma_buf_map_attachment() at all.
and for the scatterlist to be mapped for DMA at the point where the DMA
operation is initiated, and unmapped at the point where the DMA operation
is complete.
So no, the problem is not that we need more APIs and code - we need the
existing kernel API fixed so that we don't go exposing userspace to the
requirements of the DMA API. Unless we do that, we're going to end
up with a huge world of pain, where kernel architecture people need to
audit every damned DRM userspace implementation that happens to be run
on their platform, and that's not something arch people really can
afford to do.
Basically, I think the dma_buf stuff needs to be rewritten with the
requirements of the DMA API in the forefront of whosever mind is doing
the rewriting.
Note: the existing stuff does have the nice side effect of being able
to pass buffers which do not have a struct page * associated with them
through the dma_buf API - I think we can still preserve that by having
dma_buf provide a couple of new APIs to do the SG list map/sync/unmap,
but in any case we need to fix the existing API so that:
dma_buf_map_attachment() becomes dma_buf_get_sg()
dma_buf_unmap_attachment() becomes dma_buf_put_sg()
both getting rid of the DMA direction argument, and then we have four
new dma_buf calls:
dma_buf_map_sg()
dma_buf_unmap_sg()
dma_buf_sync_sg_for_cpu()
dma_buf_sync_sg_for_device()
which do the actual sg map/unmap via the DMA API *at the appropriate
time for DMA*.
So, the summary of this is - at the moment, I regard DRM Prime and dmabuf
to be utterly broken in design for architectures such as ARM where the
requirements of the DMA API have to be followed if you're going to have
a happy life.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC PATCH v2] dmabuf-sync: Introduce buffer synchronization framework
On Mon, Jun 17, 2013 at 04:42:37PM +0100, Russell King - ARM Linux wrote:
> On Tue, Jun 18, 2013 at 12:03:31AM +0900, Inki Dae wrote:
> > 2013/6/17 Russell King - ARM Linux
> > Exactly right. But that is not definitely my point. Could you please see
> > the below simple example?:
> > (Presume that CPU and DMA share a buffer and the buffer is mapped with user
> > space as cachable)
> >
> > handle1 = drm_gem_fd_to_handle(a dmabuf fd); > 1
> > ...
> > va1 = drm_gem_mmap(handle1);
> > va2 = drm_gem_mmap(handle2);
> > va3 = malloc(size);
> > ...
> >
> > while (conditions) {
> > memcpy(va1, some data, size);
>
> No!
>
> Well, the first thing to say here is that under the requirements of the
> DMA API, the above is immediately invalid, because you're writing to a
> buffer which under the terms of the DMA API is currently owned by the
> DMA agent, *not* by the CPU. You're supposed to call dma_sync_sg_for_cpu()
> before you do that - but how is userspace supposed to know that requirement?
> Why should userspace even _have_ to know these requirements of the DMA
> API?
>
> It's also entirely possible that drm_gem_fd_to_handle() (which indirectly
> causes dma_map_sg() on the buffers scatterlist) followed by mmap'ing it
> into userspace is a bug too, as it has the potential to touch caches or
> stuff in ways that maybe the DMA or IOMMU may not expect - but I'm not
> going to make too big a deal about that, because I don't think we have
> anything that picky.
>
> However, the first point above is the most important one, and exposing
> the quirks of the DMA API to userland is certainly not a nice thing to be
> doing. This needs to be fixed - we can't go and enforce an API which is
> deeply embedded within the kernel all the way out to userland.
>
> What we need is something along the lines of:
> (a) dma_buf_map_attachment() _not_ to map the scatterlist for DMA.
> or
> (b) drm_gem_prime_import() not to call dma_buf_map_attachment() at all.
>
> and for the scatterlist to be mapped for DMA at the point where the DMA
> operation is initiated, and unmapped at the point where the DMA operation
> is complete.
>
> So no, the problem is not that we need more APIs and code - we need the
> existing kernel API fixed so that we don't go exposing userspace to the
> requirements of the DMA API. Unless we do that, we're going to end
> up with a huge world of pain, where kernel architecture people need to
> audit every damned DRM userspace implementation that happens to be run
> on their platform, and that's not something arch people really can
> afford to do.
>
> Basically, I think the dma_buf stuff needs to be rewritten with the
> requirements of the DMA API in the forefront of whosever mind is doing
> the rewriting.
>
> Note: the existing stuff does have the nice side effect of being able
> to pass buffers which do not have a struct page * associated with them
> through the dma_buf API - I think we can still preserve that by having
> dma_buf provide a couple of new APIs to do the SG list map/sync/unmap,
> but in any case we need to fix the existing API so that:
>
> dma_buf_map_attachment() becomes dma_buf_get_sg()
> dma_buf_unmap_attachment() becomes dma_buf_put_sg()
>
> both getting rid of the DMA direction argument, and then we have four
> new dma_buf calls:
>
> dma_buf_map_sg()
> dma_buf_unmap_sg()
> dma_buf_sync_sg_for_cpu()
> dma_buf_sync_sg_for_device()
>
> which do the actual sg map/unmap via the DMA API *at the appropriate
> time for DMA*.
>
> So, the summary of this is - at the moment, I regard DRM Prime and dmabuf
> to be utterly broken in design for architectures such as ARM where the
> requirements of the DMA API have to be followed if you're going to have
> a happy life.
An addendum to the above:
I'll also point out that the whole thing of having random buffers mapped
into userspace, and doing DMA on them is _highly_ architecture specific.
I'm told that PARISC is one architecture where this does not work (because
DMA needs to have some kind of congruence factor programmed into it which
can be different between kernel and userspace mappings of the same
physical mappings.
I ran into this when trying to come up with a cross-arch DMA-API mmap API,
and PARISC ended up killing the idea off (the remains of that attempt is
the dma_mmap_* stuff in ARM's asm/dma-mapping.h.) However, this was for
the DMA coherent stuff, not the streaming API, which is what _this_
DMA prime/dma_buf stuff is about.
What I'm saying is think _very_ _carefully_ about userspace having
interleaved access to random DMA buffers. Arguably, DRM should _not_
allow this.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC PATCH v2] dmabuf-sync: Introduce buffer synchronization framework
On Tue, Jun 18, 2013 at 02:19:04AM +0900, Inki Dae wrote:
> It seems like that all pages of the scatterlist should be mapped with
> DMA every time DMA operation is started (or drm_xxx_set_src_dma_buffer
> function call), and the pages should be unmapped from DMA again every
> time DMA operation is completed: internally, including each cache
> operation.
Correct.
> Isn't that big overhead?
Yes, if you _have_ to do a cache operation to ensure that the DMA agent
can see the data the CPU has written.
> And If there is no problem, we should accept such overhead?
If there is no problem then why are we discussing this and why do we need
this API extension? :)
> Actually, drm_gem_fd_to_handle() includes to map a
> given dmabuf with iommu table (just logical data) of the DMA. And then, the
> device address (or iova) already mapped will be set to buffer register of
> the DMA with drm_xxx_set_src/dst_dma_buffer(handle1, ...) call.
Consider this with a PIPT cache:
dma_map_sg()- at this point, the kernel addresses of these
buffers are cleaned and invalidated for the DMA
userspace writes to the buffer, the data sits in the CPU cache
Because the cache is PIPT, this data becomes visible to the
kernel as well.
DMA is started, and it writes to the buffer
Now, at this point, which is the correct data? The data physically in the
RAM which the DMA has written, or the data in the CPU cache. It may
the answer is - they both are, and the loss of either can be a potential
data corruption issue - there is no way to tell which data should be
kept but the system is in an inconsistent state and _one_ of them will
have to be discarded.
dma_unmap_sg() - at this point, the kernel addresses of the
buffers are _invalidated_ and any data in those
cache lines is discarded
Which also means that the data in userspace will also be discarded with
PIPT caches.
This is precisely why we have buffer rules associated with the DMA API,
which are these:
dma_map_sg()
- the buffer transfers ownership from the CPU to the DMA agent.
- the CPU may not alter the buffer in any way.
while (cpu_wants_access) {
dma_sync_sg_for_cpu()
- the buffer transfers ownership from the DMA to the CPU.
- the DMA may not alter the buffer in any way.
dma_sync_sg_for_device()
- the buffer transfers ownership from the CPU to the DMA
- the CPU may not alter the buffer in any way.
}
dma_unmap_sg()
- the buffer transfers ownership from the DMA to the CPU.
- the DMA may not alter the buffer in any way.
Any violation of that is likely to lead to data corruption. Now, some
may say that the DMA API is only about the kernel mapping. Yes it is,
because it takes no regard what so ever about what happens with the
userspace mappings. This is absolutely true when you have VIVT or
aliasing VIPT caches.
Now consider that with a PIPT cache, or a non-aliasing VIPT cache (which
is exactly the same behaviourally from this aspect) any modification of
a userspace mapping is precisely the same as modifying the kernel space
mapping - and what you find is that the above rules end up _inherently_
applying to the userspace mappings as well.
In other words, userspace applications which have mapped the buffers
must _also_ respect these rules.
And there's no way in hell that I'd ever trust userspace to get this
anywhere near right, and I *really* do not like these kinds of internal
kernel API rules being exposed to userspace.
And so the idea that userspace should be allowed to setup DMA transfers
via "set source buffer", "set destination buffer" calls into an API is
just plain rediculous. No, userspace should be allowed to ask for
"please copy from buffer X to buffer Y using this transform".
Also remember that drm_xxx_set_src/dst_dma_buffer() would have to
deal with the DRM object IDs for the buffer, and not the actual buffer
information themselves - that is kept within the kernel, so the kernel
knows what's happening.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC PATCH v2] dmabuf-sync: Introduce buffer synchronization framework
On Tue, Jun 18, 2013 at 02:27:40PM +0900, Inki Dae wrote: > So I'd like to ask for other DRM maintainers. How do you think about it? it > seems like that Intel DRM (maintained by Daniel), OMAP DRM (maintained by > Rob) and GEM CMA helper also have same issue Russell pointed out. I think > not only the above approach but also the performance is very important. CMA uses coherent memory to back their buffers, though that might not be true of memory obtained from other drivers via dma_buf. Plus, there is no support in the CMA helper for exporting or importng these buffers. I guess Intel i915 is only used on x86, which is a coherent platform and requires no cache maintanence for DMA. OMAP DRM does not support importing non-DRM buffers buffers back into DRM. Moreover, it will suffer from the problems I described if any attempt is made to write to the buffer after it has been re-imported. Lastly, I should point out that the dma_buf stuff is really only useful when you need to export a dma buffer from one driver and import it into another driver - for example to pass data from a camera device driver to a display device driver. It shouldn't be used within a single driver as a means of passing buffers between userspace and kernel space. ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC PATCH v2] dmabuf-sync: Introduce buffer synchronization framework
On Tue, Jun 18, 2013 at 06:04:44PM +0900, Inki Dae wrote: > > > > -Original Message- > > From: Russell King - ARM Linux [mailto:li...@arm.linux.org.uk] > > Sent: Tuesday, June 18, 2013 5:43 PM > > To: Inki Dae > > Cc: 'Maarten Lankhorst'; 'linux-fbdev'; 'Kyungmin Park'; 'DRI mailing > > list'; 'Rob Clark'; 'myungjoo.ham'; 'YoungJun Cho'; 'Daniel Vetter'; > > linux-arm-ker...@lists.infradead.org; linux-me...@vger.kernel.org > > Subject: Re: [RFC PATCH v2] dmabuf-sync: Introduce buffer synchronization > > framework > > > > On Tue, Jun 18, 2013 at 02:27:40PM +0900, Inki Dae wrote: > > > So I'd like to ask for other DRM maintainers. How do you think about it? > > it > > > seems like that Intel DRM (maintained by Daniel), OMAP DRM (maintained > > by > > > Rob) and GEM CMA helper also have same issue Russell pointed out. I > > think > > > not only the above approach but also the performance is very important. > > > > CMA uses coherent memory to back their buffers, though that might not be > > true of memory obtained from other drivers via dma_buf. Plus, there is > > no support in the CMA helper for exporting or importng these buffers. > > > > It's not so. Please see Dave's drm next. recently dmabuf support for the CMA > helper has been merged to there. The point stands: CMA is DMA coherent memory. It doesn't need and must never be dma-map-sg'd or dma-sync'd or dma-unmap'd. ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC PATCH v2] dmabuf-sync: Introduce buffer synchronization framework
On Tue, Jun 18, 2013 at 09:00:16AM +0200, Daniel Vetter wrote: > On Mon, Jun 17, 2013 at 04:42:37PM +0100, Russell King - ARM Linux wrote: > > What we need is something along the lines of: > > (a) dma_buf_map_attachment() _not_ to map the scatterlist for DMA. > > or > > (b) drm_gem_prime_import() not to call dma_buf_map_attachment() at all. > > I strongly vote for (b) (plus adding a dma_buf_map_sync interface to allow > syncing to other devices/cpu whithout dropping the dma mappings). At least > that's been the idea behind things, but currently all (x86-based) drm > drivers cut corners here massively. > > Aside: The entire reason behind hiding the dma map step in the dma-buf > attachment is to allow drivers to expose crazy iommus (e.g. the tiler unit > on omap) to other devices. So imo (a) isn't the right choice. That's why I also talk below about adding the dma_buf_map/sync callbacks below, so that a dma_buf implementation can do whatever is necessary with the sg at the point of use. However, you are correct that this should be unnecessary, as DRM should only be calling dma_buf_map_attachment() when it needs to know about the memory behind the object. The problem is that people will cache that information - it also may be expensive to setup for the dma_buf - as it involves counting the number of pages, memory allocation, and maybe obtaining the set of pages from shmem. With (a) plus the callbacks below, it means that step is only performed once, and then the DMA API part is entirely separate - we can have our cake and eat it in that regard. > > Note: the existing stuff does have the nice side effect of being able > > to pass buffers which do not have a struct page * associated with them > > through the dma_buf API - I think we can still preserve that by having > > dma_buf provide a couple of new APIs to do the SG list map/sync/unmap, > > but in any case we need to fix the existing API so that: > > > > dma_buf_map_attachment() becomes dma_buf_get_sg() > > dma_buf_unmap_attachment() becomes dma_buf_put_sg() > > > > both getting rid of the DMA direction argument, and then we have four > > new dma_buf calls: > > > > dma_buf_map_sg() > > dma_buf_unmap_sg() > > dma_buf_sync_sg_for_cpu() > > dma_buf_sync_sg_for_device() > > > > which do the actual sg map/unmap via the DMA API *at the appropriate > > time for DMA*. > > Hm, my idea was to just add a dma_buf_sync_attchment for the device side > syncing, since the cpu access stuff is already bracketed with the > begin/end cpu access stuff. We might need a sync_for_cpu or so for mmap, > but imo mmap support for dma_buf is a bit insane anyway, so I don't care > too much about it. > > Since such dma mappings would be really longstanding in most cases anyway > drivers could just map with BIDIRECTIONAL and do all the real flushing > with the new sync stuff. Note that the DMA API debug doesn't allow you to change the direction argument on an existing mapping (neither should it, again this is documented in the DMA API stuff in Documentation/). This is where you would need the complete set of four functions I mention above which reflect the functionality of the DMA API. The dma_buf implementation doesn't _have_ to implement them if they are no-ops - for example, your dma_buf sg array contains DMA pointers, and the memory is already coherent in some way (for example, it's a separate chunk of memory which isn't part of system RAM.) ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC PATCH v2] dmabuf-sync: Introduce buffer synchronization framework
On Thu, Jun 20, 2013 at 12:10:04AM +0900, Inki Dae wrote: > On the other hand, the below shows how we could enhance the conventional > way with my approach (just example): > > CPU -> DMA, > ioctl(qbuf command) ioctl(streamon) > | | > | | > qbuf <- dma_buf_sync_get start streaming <- syncpoint > > dma_buf_sync_get just registers a sync buffer(dmabuf) to sync object. And > the syncpoint is performed by calling dma_buf_sync_lock(), and then DMA > accesses the sync buffer. > > And DMA -> CPU, > ioctl(dqbuf command) > | > | > dqbuf <- nothing to do > > Actual syncpoint is when DMA operation is completed (in interrupt handler): > the syncpoint is performed by calling dma_buf_sync_unlock(). > Hence, my approach is to move the syncpoints into just before dma access > as long as possible. What you've just described does *not* work on architectures such as ARMv7 which do speculative cache fetches from memory at any time that that memory is mapped with a cacheable status, and will lead to data corruption. ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC PATCH v2] dmabuf-sync: Introduce buffer synchronization framework
On Thu, Jun 20, 2013 at 09:47:07AM +0200, Lucas Stach wrote: > Am Donnerstag, den 20.06.2013, 15:43 +0900 schrieb Inki Dae: > > > > > -Original Message- > > > From: dri-devel-bounces+inki.dae=samsung@lists.freedesktop.org > > > [mailto:dri-devel-bounces+inki.dae=samsung@lists.freedesktop.org] On > > > Behalf Of Russell King - ARM Linux > > > Sent: Thursday, June 20, 2013 3:29 AM > > > To: Inki Dae > > > Cc: linux-fbdev; DRI mailing list; Kyungmin Park; myungjoo.ham; YoungJun > > > Cho; linux-me...@vger.kernel.org; linux-arm-ker...@lists.infradead.org > > > Subject: Re: [RFC PATCH v2] dmabuf-sync: Introduce buffer synchronization > > > framework > > > > > > On Thu, Jun 20, 2013 at 12:10:04AM +0900, Inki Dae wrote: > > > > On the other hand, the below shows how we could enhance the conventional > > > > way with my approach (just example): > > > > > > > > CPU -> DMA, > > > > ioctl(qbuf command) ioctl(streamon) > > > > | | > > > > | | > > > > qbuf <- dma_buf_sync_get start streaming <- syncpoint > > > > > > > > dma_buf_sync_get just registers a sync buffer(dmabuf) to sync object. > > > And > > > > the syncpoint is performed by calling dma_buf_sync_lock(), and then DMA > > > > accesses the sync buffer. > > > > > > > > And DMA -> CPU, > > > > ioctl(dqbuf command) > > > > | > > > > | > > > > dqbuf <- nothing to do > > > > > > > > Actual syncpoint is when DMA operation is completed (in interrupt > > > handler): > > > > the syncpoint is performed by calling dma_buf_sync_unlock(). > > > > Hence, my approach is to move the syncpoints into just before dma > > > access > > > > as long as possible. > > > > > > What you've just described does *not* work on architectures such as > > > ARMv7 which do speculative cache fetches from memory at any time that > > > that memory is mapped with a cacheable status, and will lead to data > > > corruption. > > > > I didn't explain that enough. Sorry about that. 'nothing to do' means that a > > dmabuf sync interface isn't called but existing functions are called. So > > this may be explained again: > > ioctl(dqbuf command) > > | > > | > > dqbuf <- 1. dma_unmap_sg > > 2. dma_buf_sync_unlock (syncpoint) > > > > The syncpoint I mentioned means lock mechanism; not doing cache operation. > > > > In addition, please see the below more detail examples. > > > > The conventional way (without dmabuf-sync) is: > > Task A > > > > 1. CPU accesses buf > > 2. Send the buf to Task B > > 3. Wait for the buf from Task B > > 4. go to 1 > > > > Task B > > --- > > 1. Wait for the buf from Task A > > 2. qbuf the buf > > 2.1 insert the buf to incoming queue > > 3. stream on > > 3.1 dma_map_sg if ready, and move the buf to ready queue > > 3.2 get the buf from ready queue, and dma start. > > 4. dqbuf > > 4.1 dma_unmap_sg after dma operation completion > > 4.2 move the buf to outgoing queue > > 5. back the buf to Task A > > 6. go to 1 > > > > In case that two tasks share buffers, and data flow goes from Task A to Task > > B, we would need IPC operation to send and receive buffers properly between > > those two tasks every time CPU or DMA access to buffers is started or > > completed. > > > > > > With dmabuf-sync: > > > > Task A > > > > 1. dma_buf_sync_lock <- synpoint (call by user side) > > 2. CPU accesses buf > > 3. dma_buf_sync_unlock <- syncpoint (call by user side) > > 4. Send the buf to Task B (just one time) > > 5. go to 1 > > > > > > Task B > > --- > > 1. Wait for the buf from Task A (just one time) > > 2. qbuf the buf > > 1.1 insert the buf to incoming queue > > 3. stream on > > 3.1 dma_buf_sync_lock <-
Re: [PATCH v4] drm/i2c: tda998x: fix sync generation and calculation
On Thu, Jun 20, 2013 at 09:46:03PM +0200, Sebastian Hesselbarth wrote: > + ref_pix = 3 + mode->hsync_start - mode->hdisplay; > + de_pix_s = mode->htotal - mode->hdisplay; > + de_pix_e = de_pix_s + mode->hdisplay; > + hs_pix_s = mode->hsync_start - mode->hdisplay; > + hs_pix_e = hs_pix_s + mode->hsync_end - mode->hsync_start; Correct, however, these can be simplified. For de_pix_e: de_pix_e = de_pix_s + mode->hdisplay; de_pix_s = mode->htotal - mode->hdisplay; Putting de_pix_s into de_pix_e's equation, you get: de_pix_e = mode->htotal - mode->hdisplay + mode->hdisplay; which ends up simply as: de_pix_e = mode->htotal; Now, doing the same for hs_pix_e: hs_pix_e = mode->hsync_start - mode->hdisplay + mode->hsync_end - mode->hsync_start; which ends up as: hs_pix_e = mode->hsync_end - mode->hdisplay; So overall these come out as: de_pix_e = mode->htotal; de_pix_s = mode->htotal - mode->hdisplay; hs_pix_e = mode->hsync_end - mode->hdisplay; hs_pix_s = mode->hsync_start - mode->hdisplay; I've listed them in reverse order because it makes more sense to me when thinking about it. What we're basically doing is rotating this by hdisplay pixel clocks to the left, so using abbreviations: ht=htotal hds=hdisplay start hde=hdisplay end hss=hsync_start hse=hsync_end 0ht hdshde hss hse | |---||---|| becomes: 0ht 0 hss hse hdshde ||---||---| and from that you can visualize taking hdisplay (being hde-hds) off each timing parameter modulo htotal gives you the above equations. These calculations are the same as what was originally in the driver: + de_start = mode->htotal - mode->hdisplay; + de_end = mode->htotal; + hs_start = mode->hsync_start - mode->hdisplay; + hs_end = mode->hsync_end - mode->hdisplay; when it was first committed. This is otherwise exactly what I came up with which gets my TV working again in HDMI mode. ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: Armada DRM driver on OLPC XO
On Wed, Jun 26, 2013 at 06:42:50PM +0200, Jean-Francois Moine wrote: > Do you know that there are 2 drm drivers for the Cubox? I posted mine > (http://lists.infradead.org/pipermail/linux-arm-kernel/2013-May/168732.html) > before Russell, but I got no return about it yet. > > As it uses the CMA helper (no handling of the Cubox GPU/VPU), my > driver is simpler and does not need any specific memory reservation. As I have previously covered, that's a *big* negative point - the big downside of that is that it will be *much* slower when it comes to using the GPU because the pixmap data will be accessed by the CPU via uncacheable mappings. Moreover, as you say you're not using the GPU, that means that every X operation which uses a DRM allocated pixmap will be copying between one uncached pixmap and another uncached pixmap. That is totally insane. > It has full DT support. The Cubox specific drivers are build as > loadable modules (dove-drm driver, tda998x hdmi slave encoder, si5351 > clock driver and kirkwood i2s/spdif audio driver). The synchronization > of module loading at startup time is done by EPROBE_DEFER. The DT > permits each CRTC to use any of up to 4 clocks. Via a horrid hack that doesn't really justify being in the kernel, and certainly isn't flexible because it makes use of global variables to detect when all the different parts of the DT representation have been registered. I have *serious* concerns about your approach to that problem, and I really violently detest your "solution" - which is more of a hack than a real solution. I've covered those points in my comments on your code when you first published it. ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC PATCH v2] dmabuf-sync: Introduce buffer synchronization framework
On Tue, Jun 25, 2013 at 11:23:21AM +0200, Daniel Vetter wrote: > Just a quick question on your assertion that we need all four > functions: Since we already have begin/end_cpu_access functions > (intention here was to allow the dma_buf exporter to ensure the memory > is pinned, e.g. for swapable gem objects, but also allow cpu cache > flushing if required) do we still need the sync_sg_for_cpu? Possibly not, but let's first look at that kind of scenario: - It attaches to a dma_buf using dma_buf_attach() - it maps the DMA buffer using dma_buf_map_attachment(). This does dma_map_sg(). This possibly incurs a cache flush depending on the coherence properties of the architecture. - it then calls dma_buf_begin_cpu_access(). This presumably does a dma_sync_sg_for_cpu(). This possibly incurs another cache flush depending on the coherence properties of the architecture. - then, presumably, it calls dma_buf_kmap_atomic() or dma_buf_kmap() to gain a pointer to the buffer. - at some point, dma_buf_kunmap_atomic() or dma_buf_kunmap() gets called. On certain cache architectures, kunmap_atomic() results in a cache flush. - dma_buf_end_cpu_access() gets called, calling through presumably to dma_sync_sg_for_device(). Possibly incurs a cache flush. - dma_buf_unmap_attachment()... another cache flush. Out of those all of those five cache flushes, the only cache flush which is really necessary out of all those would be the one in kunmap_atomic(). The rest of them are completely irrelevant to the CPU access provided that there is no DMA access by the device. What's more is that you can't say "well, the architecture should optimize them!" to which I'd respond "how does the architecture know at dma_map_sg() time that there isn't going to be a DMA access?" Now, if we say that it's not necessary to call dma_buf_map_attachment().. dma_buf_unmap_attachment() around the calls to dma_buf_begin_cpu_access().. dma_buf_end_cpu_access(), then how does dma_buf_begin_cpu_access() know whether the DMA buffer has been dma_map_sg()'d? It's invalid to call dma_sync_sg_for_cpu() on a buffer which has not been dma_map_sg()'d. Bear in mind that dma_buf_begin_cpu_access() is passed only the struct dma_buf and not the struct dma_buf_attachment *attach, there's no hope for the begin_cpu_access() method to know which user of the dma_buf is asking for this call, so you can't track any state there. Thank you for pointing this out, I'm less impressed with this dma_buf API now that I've looked deeper at those two interfaces, and even more convinced it needs to be redesigned! :P ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: Armada DRM driver on OLPC XO
On Tue, Jun 25, 2013 at 04:47:26PM -0400, Daniel Drake wrote: > Hi Russell, > > Thanks a lot for writing the Armada DRM driver. > > I have tested it on OLPC XO-1.75 (MMP2 aka Armada610) and OLPC XO-4 (MMP3 > aka PXA2128). After a bit of fighting, I have it running. Could you share your > X driver, or your methodology for testing hardware cursors? I'd like to test > your work there too. > > It's probably easiest to get your cubox driver merged before adding MMP2/MMP3 > complications into the mix. At that point, I will hopefully have time to > follow up developing MMP2/MMP3 support, which will involve the points > mentioned below. As a side note, you've looked at a previous generation of the patches - in that same thread, there are two versions of these patches. The original set was an older version that I mis-posted. The later version removes the clock handling for the LCDs out of armada_crtc.c into armada_drv.c. ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: Armada DRM driver on OLPC XO
On Fri, Jun 28, 2013 at 01:54:21PM -0600, Daniel Drake wrote: > On Wed, Jun 26, 2013 at 10:42 AM, Jean-Francois Moine wrote: > > Do you know that there are 2 drm drivers for the Cubox? I posted mine > > (http://lists.infradead.org/pipermail/linux-arm-kernel/2013-May/168732.html) > > before Russell, but I got no return about it yet. > > I thought there was some agreement that you would merge your efforts > into Russell's driver. Is that still the plan? > > Thanks for the links - I think we can learn something about timer > handling from your work. I'll send a patch shortly incorporating the > basis of something like this into armada_drm. Note that my driver has changed significantly since the last posting; it's now using drm planes and also dmabuf stuff to limited extents (because dmabuf is technically misdesigned for CPUs with noncoherent DMA.) ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: armada_drm clock selection
ps armada510_ops = {
> @@ -85,6 +88,19 @@ static const struct armada_ops armada510_ops = {
> .compute_crtc_clock = armada510_compute_crtc_clock,
> };
>
> +static void release_clocks(struct armada_private *priv)
> +{
> + int i;
> + for (i = 0; i < ARMADA_MAX_CLOCK; i++) {
> + struct clk *clk = priv->clk[i];
> + if (!clk)
> + continue;
> +
> + clk_put(clk);
> + priv->clk[i] = NULL;
Yuck, really - if you really really need to use of_clk_get(), of_clk_get()
needs to be fixed to also have a devm_* counterpart. I'm sure lots of
people will appreciate whoever steps up and adds that.
> + }
> +}
> +
> static int armada_drm_load(struct drm_device *dev, unsigned long flags)
> {
> struct armada_private *priv;
> @@ -129,7 +145,7 @@ static int armada_drm_load(struct drm_device *dev,
> unsigned long flags)
>
> ret = priv->ops->init(priv, dev->dev);
> if (ret)
> - return ret;
> + goto err_buf;
>
> /* Mode setting support */
> drm_mode_config_init(dev);
> @@ -176,6 +192,7 @@ static int armada_drm_load(struct drm_device *dev,
> unsigned long flags)
> err_irq:
> drm_irq_uninstall(dev);
> err_kms:
> + release_clocks(priv);
> drm_mode_config_cleanup(dev);
> drm_mm_takedown(&priv->linear);
> err_buf:
> @@ -193,6 +210,7 @@ static int armada_drm_unload(struct drm_device *dev)
> drm_irq_uninstall(dev);
> drm_mode_config_cleanup(dev);
> drm_mm_takedown(&priv->linear);
> + release_clocks(priv);
> dev->dev_private = NULL;
>
> return 0;
> diff --git a/drivers/gpu/drm/armada/armada_hw.h
> b/drivers/gpu/drm/armada/armada_hw.h
> index 58d2a20..df59b5a 100644
> --- a/drivers/gpu/drm/armada/armada_hw.h
> +++ b/drivers/gpu/drm/armada/armada_hw.h
> @@ -193,11 +193,12 @@ enum {
> };
>
> /* For LCD_CFG_SCLK_DIV */
> +#define SCLK_510_SHIFT 30
> enum {
> - SCLK_510_AXI= 0x0 << 30,/* Armada 510 */
> - SCLK_510_EXTCLK0= 0x1 << 30,/* Armada 510 */
> - SCLK_510_PLL= 0x2 << 30,/* Armada 510 */
> - SCLK_510_EXTCLK1= 0x3 << 30,/* Armada 510 */
> + SCLK_510_AXI= 0x0 << SCLK_510_SHIFT,/* Armada 510 */
> + SCLK_510_EXTCLK0= 0x1 << SCLK_510_SHIFT,/* Armada 510 */
> + SCLK_510_PLL= 0x2 << SCLK_510_SHIFT,/* Armada 510 */
> + SCLK_510_EXTCLK1= 0x3 << SCLK_510_SHIFT,/* Armada 510 */
> SCLK_DIV_CHANGE = 1 << 29,
> SCLK_16X_AHB= 0x0 << 28,/* Armada 16x */
> SCLK_16X_PCLK = 0x1 << 28,/* Armada 16x */
> --
> 1.8.1.4
>
> ___
> linux-arm-kernel mailing list
> linux-arm-ker...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
