[RFC 1/6] fbdev: simplefb: add init through platform_data
On 06/24/2013 04:27 PM, David Herrmann wrote:
> If we create proper platform-devices in x86 boot-code, we can use simplefb
> for VBE or EFI framebuffers, too. However, there is normally no OF support
> so we introduce a platform_data object so x86 boot-code can pass the
> paramaters via plain old platform-data.
>
> This also removes the OF dependency as it is not needed. The headers
> provide proper dummies for the case OF is disabled.
>
> Furthermore, we move the FORMAT-definitions to the common platform header
> so initialization code can use it to transform "struct screen_info" to
> the right format-name.
> diff --git a/include/linux/platform_data/simplefb.h
> b/include/linux/platform_data/simplefb.h
> +/* the framebuffer size and location is available as IORESOURCE_MEM */
> +struct simplefb_platform_data {
> + u32 width;
> + u32 height;
> + u32 stride;
> + char format[64];
> +};
Any reason not to make format:
const char *format;
You should be able to initialize that just as easily in platform code,
either as static data or at runtime, I think.
[RFC 2/6] x86: provide platform-devices for boot-framebuffers
On 06/24/2013 04:27 PM, David Herrmann wrote:
> The current situation regarding boot-framebuffers (VGA, VESA/VBE, EFI) on
> x86 causes troubles when loading multiple fbdev drivers. The global
> "struct screen_info" does not provide any state-tracking about which
> drivers use the FBs. request_mem_region() theoretically works, but
> unfortunately vesafb/efifb ignore it due to quirks for broken boards.
>
> Avoid this by creating a "platform-framebuffer" device with a pointer
> to the "struct screen_info" as platform-data. Drivers can now create
> platform-drivers and the driver-core will refuse multiple drivers being
> active simultaneously.
>
> We keep the screen_info available for backwards-compatibility. Drivers
> can be converted in follow-up patches.
>
> Apart from "platform-framebuffer" devices, this also introduces a
> compatibility option for "simple-framebuffer" drivers which recently got
> introduced for OF based systems. If CONFIG_X86_SYSFB is selected, we
> try to match the screen_info against a simple-framebuffer supported
> format. If we succeed, we create a "simple-framebuffer" device instead
> of a platform-framebuffer.
> This allows to reuse the simplefb.c driver across architectures and also
> to introduce a SimpleDRM driver. There is no need to have vesafb.c,
> efifb.c, simplefb.c and more just to have architecture specific quirks
> in their setup-routines.
>
> Instead, we now move the architecture specific quirks into x86-setup and
> provide a generic simple-framebuffer. For backwards-compatibility (if
> strange formats are used), we still allow vesafb/efifb to be loaded
> simultaneously and pick up all remaining devices.
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> +config X86_SYSFB
> + bool "Mark VGA/VBE/EFI FB as generic system framebuffer"
> + help
> + Firmwares often provide initial graphics framebuffers so the BIOS,
> + bootloader or kernel can show basic video-output during boot for
> + user-guidance and debugging. Historically, x86 used the VESA BIOS
> + Extensions and EFI-framebuffers for this, which are mostly limited
> + to x86. However, a generic system-framebuffer initialization emerged
> + recently on some non-x86 architectures.
After this patch has been in the kernel a while, that very last won't
really be true; simplefb won't have been introduced recently. Perhaps
just delete that one sentence?
> + This option, if enabled, marks VGA/VBE/EFI framebuffers as generic
> + framebuffers so the new generic system-framebuffer drivers can be
> + used on x86.
> +
> + This breaks any x86-only driver like efifb, vesafb, uvesafb, which
> + will not work if this is selected.
Doesn't that imply that some form of conflicts or depends ! statement
should be added here?
> diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
> +obj-y+= sysfb.o
I suspect that should be obj-$(CONFIG_X86_SYSFB) += sysfb.o.
> diff --git a/arch/x86/kernel/sysfb.c b/arch/x86/kernel/sysfb.c
> +#ifdef CONFIG_X86_SYSFB
Rather than ifdef'ing the body of this file, why not create a dummy
static inline version of add_sysfb() and put that into a header file
that users include. There should be a header file to prototype the
function anyway. That way, you avoid all of the ifdefs and static inline
functions in this file.
> +static bool parse_mode(const struct screen_info *si,
> +struct simplefb_platform_data *mode)
> + strlcpy(mode->format, f->name, sizeof(mode->format));
Per my comments about the type of mode->format, I think that could just be:
mode->format = f->name;
... since formats[] (i.e. f) isn't initdata.
> +#else /* CONFIG_X86_SYSFB */
> +
> +static bool parse_mode(const struct screen_info *si,
> +struct simplefb_platform_data *mode)
> +{
> + return false;
> +}
> +
> +static int create_simplefb(const struct screen_info *si,
> +const struct simplefb_platform_data *mode)
> +{
> + return -EINVAL;
> +}
> +
> +#endif /* CONFIG_X86_SYSFB */
Following on from my ifdef comment above, I believe those versions of
those functions will always cause add_sysfb() to return -ENODEV, so you
may as well provide a static inline for add_sysfb() instead.
[RFC 3/6] drm: add SimpleDRM driver
On 06/24/2013 04:27 PM, David Herrmann wrote: > The SimpleDRM driver binds to simple-framebuffer devices and provides a > DRM/KMS API. It provides only a single CRTC+encoder+connector combination > plus one initial mode. > > Userspace can create one dumb-buffer and attach it to the CRTC. Only if > the buffer is destroyed, a new buffer can be created. The buffer is > directly mapped into user-space, so we have only resources for a single > buffer. Otherwise, shadow buffers plus damage-request would be needed. > diff --git a/drivers/gpu/drm/simpledrm/Kconfig > b/drivers/gpu/drm/simpledrm/Kconfig > +config DRM_SIMPLEDRM > + tristate "Simple firmware framebuffer DRM driver" > + depends on DRM && !FB_SIMPLE > + help > + SimpleDRM can run on all systems with pre-initialized graphics > + hardware. It uses a framebuffer that was initialized during > + firmware boot. No page-flipping, modesetting or other advanced > + features are available. However, other DRM drivers can be loaded > + later and take over from SimpleDRM if they provide real hardware > + support. > + > + SimpleDRM supports: "simple-framebuffer" DeviceTree objects, x86 VESA > + BIOS Extensions (VBE), EFI framebuffers DT objects, yes. I'm not sure it's quite true to say it actually directly supports VBE or EFI FBs; it's more the code in patch 2/6 that supports those. I guess this is a bit nit-picky of a distinction though. > diff --git a/drivers/gpu/drm/simpledrm/simpledrm_drv.c > b/drivers/gpu/drm/simpledrm/simpledrm_drv.c > +static int parse_dt(struct platform_device *pdev, > + struct simplefb_platform_data *mode) > + strlcpy(mode->format, format, sizeof(mode->format)); Even here, I believe the DT data sticks around so just copying the pointer should be safe. It'd be worth validating that for sure though. I didn't review the DRM stuff here since I'm not at all familiar with DRM.
[RFC 4/6] drm: simpledrm: add fbdev fallback support
On 06/24/2013 04:27 PM, David Herrmann wrote: > Create a simple fbdev device during SimpleDRM setup so legacy user-space > and fbcon can use it. > > fbdev deletion is quite buggy. A unregister_framebuffer() call followed by > a printk() causes NULL-derefs in hide_cursor() and other places in the VT > layer. Hence, we leak the fbdev device currently to make the VT layer > happy. This needs to be fixed soon! Otherwise, we need a "depends !VT" > line for SimpleDRM. > diff --git a/drivers/gpu/drm/simpledrm/Makefile > b/drivers/gpu/drm/simpledrm/Makefile > simpledrm-y := simpledrm_drv.o simpledrm_main.o simpledrm_mem.o > > +ifdef CONFIG_DRM_SIMPLEDRM_FBDEV > + simpledrm-y += simpledrm_fbdev.o > +endif I think that's: + simpledrm-$(CONFIG_DRM_SIMPLEDRM_FBDEV) += simpledrm_fbdev.o
[RFC 0/6] SimpleDRM Driver (was: dvbe driver)
On 06/24/2013 04:27 PM, David Herrmann wrote:
> Hi
>
> This is my second revision of the dvbe driver. I renamed it to SimpleDRM to
> show the resemblence with the recently introduced simplefb.c fbdev driver. The
> driver is supposed to be the most basic DRM driver similar to efifb.c,
> vesafb.c,
> offb.c, simplefb.c, ...
> It provides a single virtual CRTC+encoder+connector and allows user-space to
> create one dumb-buffer at a time and attach it.
>
> The setup changed slightly. It no longer uses shadow buffers but instead maps
> the framebuffer directly into userspace. Furthermore, a new infrastructure is
> used to unload firmware drivers during real hardware drivers probe cycles.
> Only
> nouveau was changed to use it, yet.
>
> I still have an odd problem when unloading DRM drivers (not just SimpleDRM)
> with
> an fbdev fallback. If I call printk() directly after unregister_framebufer(),
> I
> get a NULL-deref somewhere in the VT layer (most times hide_cursor()). I
> haven't
> figured out exactly where that happens, but I am also very reluctant to spend
> more time debugging the VT layer.
I tested this on a Tegra ARM system, and it basically worked.
I have one question: With the simplefb driver, and console=tty1 on the
kernel command-line, I see both the penguins logo and Linux's boot
messages on the LCD panel that's hooked up through simplefb. However,
with simpledrm, I only see the penguins logo, but no boot messages. Is
that expected? How would I solve that if so?
Note: I needed to apply the following patch to get it to compile:
diff --git a/drivers/gpu/drm/simpledrm/simpledrm_fbdev.c
b/drivers/gpu/drm/simpledrm/simpledrm_fbdev.c
index 40a2696..39885c8 100644
--- a/drivers/gpu/drm/simpledrm/simpledrm_fbdev.c
+++ b/drivers/gpu/drm/simpledrm/simpledrm_fbdev.c
@@ -159,7 +159,7 @@ void sdrm_fbdev_cleanup(struct sdrm_device *sdrm)
{
struct fb_info *info;
- if (!sdrm->info)
+ if (!sdrm->fbdev)
return;
dev_info(sdrm->ddev->dev, "fbdev cleanup\n");
nouveau lockdep splat
On 03/06/2013 12:14 PM, Marcin Slusarz wrote: > On Wed, Mar 06, 2013 at 01:04:29AM +0100, Borislav Petkov wrote: >> On Tue, Mar 05, 2013 at 05:30:52PM +0100, Lucas Stach wrote: >>> Dropping Tegra ML, it's not the place where Nouveau mails should go. >> >> $ ./scripts/get_maintainer.pl -f drivers/gpu/drm/nouveau/nv50_display.c >> ... >> linux-tegra at vger.kernel.org (open list:TEGRA SUPPORT) >> >> Maybe get_maintainer.pl patterns need correction... > > That's new feature (introduced in commit eb90d0855b75f8 "get_maintainer: allow > keywords to match filenames") of get_maintainer.pl which now can look at file > contents... get_maintainer.pl could always look at file contents IIRC. The change was that I added keyword "tegra" to the Tegra section that now matches this file's contents. ./scripts/get_maintainer.pl -f drivers/gpu/drm/nouveau ... might be a better invocation, although perhaps I should add an explicit exclusion for "nouveau" to the Tegra section in MAINTAINERS.
[PATCH 1/2] get_maintainer: create filename-only regex match type
From: Stephen Warren <swar...@nvidia.com>
Create a new N: entry type in MAINTAINERS which performs a regex match
against filenames; either those extracted from patch +++ or --- lines,
or those specified on the command-line using the -f option.
This provides the same benefits as using a K: regex option to match a
set of filenames (see commit eb90d08 "get_maintainer: allow keywords to
match filenames"), but without the disadvantage that "random" file
content, such as comments, will ever match the regex.
Suggested-by: Joe Perches
Signed-off-by: Stephen Warren
---
MAINTAINERS |3 +++
scripts/get_maintainer.pl |2 +-
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/MAINTAINERS b/MAINTAINERS
index 9561658..1671842 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -90,6 +90,9 @@ Descriptions of section entries:
F: drivers/net/* all files in drivers/net, but not below
F: */net/* all files in "any top level directory"/net
One pattern per line. Multiple F: lines acceptable.
+ N: Files and directories with regex patterns.
+ K: [^a-z]tegra all files whose patch contains the word tegra
+ One pattern per line. Multiple N: lines acceptable.
X: Files and directories that are NOT maintained, same rules as F:
Files exclusions are tested before file matches.
Can be useful for excluding a specific subdirectory, for instance:
diff --git a/scripts/get_maintainer.pl b/scripts/get_maintainer.pl
index ce4cc83..27dc5cb 100755
--- a/scripts/get_maintainer.pl
+++ b/scripts/get_maintainer.pl
@@ -611,7 +611,7 @@ sub get_maintainers {
$hash{$tvi} = $value_pd;
}
}
- } elsif ($type eq 'K') {
+ } elsif ($type eq 'K' || $type eq 'N') {
if ($file =~ m/$value/x) {
$hash{$tvi} = 0;
}
--
1.7.10.4
[PATCH 2/2] MAINTAINERS: tegra: match related files using N: not K:
From: Stephen Warren <swar...@nvidia.com> This causes the regex to be applied to filenames only, and not patch or file content (such as comments). This should prevent e.g. drivers/gpu/drm/nouveau/nv50_display.c from matching this entry. Reported-by: Marcin Slusarz Signed-off-by: Stephen Warren --- MAINTAINERS |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/MAINTAINERS b/MAINTAINERS index 1671842..8b986e2 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -7851,7 +7851,7 @@ L:linux-tegra at vger.kernel.org Q: http://patchwork.ozlabs.org/project/linux-tegra/list/ T: git git://git.kernel.org/pub/scm/linux/kernel/git/swarren/linux-tegra.git S: Supported -K: (?i)[^a-z]tegra +N: [^a-z]tegra TEHUTI ETHERNET DRIVER M: Andy Gospodarek -- 1.7.10.4
[PATCH V2 1/3] get_maintainer: create filename-only regex match type
From: Stephen Warren <swar...@nvidia.com>
Create a new N: entry type in MAINTAINERS which performs a regex match
against filenames; either those extracted from patch +++ or --- lines,
or those specified on the command-line using the -f option.
This provides the same benefits as using a K: regex option to match a
set of filenames (see commit eb90d08 "get_maintainer: allow keywords to
match filenames"), but without the disadvantage that "random" file
content, such as comments, will ever match the regex.
Suggested-by: Joe Perches
Signed-off-by: Stephen Warren
---
v2: Corrected typo in MAINTAINERS documentation
---
MAINTAINERS |3 +++
scripts/get_maintainer.pl |2 +-
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/MAINTAINERS b/MAINTAINERS
index 9561658..c9b1e37 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -90,6 +90,9 @@ Descriptions of section entries:
F: drivers/net/* all files in drivers/net, but not below
F: */net/* all files in "any top level directory"/net
One pattern per line. Multiple F: lines acceptable.
+ N: Files and directories with regex patterns.
+ N: [^a-z]tegra all files whose patch contains the word tegra
+ One pattern per line. Multiple N: lines acceptable.
X: Files and directories that are NOT maintained, same rules as F:
Files exclusions are tested before file matches.
Can be useful for excluding a specific subdirectory, for instance:
diff --git a/scripts/get_maintainer.pl b/scripts/get_maintainer.pl
index ce4cc83..27dc5cb 100755
--- a/scripts/get_maintainer.pl
+++ b/scripts/get_maintainer.pl
@@ -611,7 +611,7 @@ sub get_maintainers {
$hash{$tvi} = $value_pd;
}
}
- } elsif ($type eq 'K') {
+ } elsif ($type eq 'K' || $type eq 'N') {
if ($file =~ m/$value/x) {
$hash{$tvi} = 0;
}
--
1.7.10.4
[PATCH V2 2/3] MAINTAINERS: tegra: match related files using N: not K:
From: Stephen Warren <swar...@nvidia.com> This causes the regex to be applied to filenames only, and not patch or file content (such as comments). This should prevent e.g. drivers/gpu/drm/nouveau/nv50_display.c from matching this entry. Reported-by: Marcin Slusarz Signed-off-by: Stephen Warren --- MAINTAINERS |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/MAINTAINERS b/MAINTAINERS index c9b1e37..2d02ab5 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -7851,7 +7851,7 @@ L:linux-tegra at vger.kernel.org Q: http://patchwork.ozlabs.org/project/linux-tegra/list/ T: git git://git.kernel.org/pub/scm/linux/kernel/git/swarren/linux-tegra.git S: Supported -K: (?i)[^a-z]tegra +N: [^a-z]tegra TEHUTI ETHERNET DRIVER M: Andy Gospodarek -- 1.7.10.4
[PATCH V2 3/3] get_maintainer: prevent keywords from matching filenames
From: Stephen Warren <swar...@nvidia.com>
This reverts most of eb90d08 "get_maintainer: allow keywords to match
filenames"; all except the parts that are required to implement the new
N: entry type.
The rationale is that it's better to have K: match just patch or file
content as it previously did, and N: match just filenames, rather than
have K: math all three cases. This gives more explicit control, and
removes the temptation to use K: for filenames, and then have those
keywords accidentally match unexpected patch or file content.
Signed-off-by: Stephen Warren
---
v2: New patch.
---
MAINTAINERS |9 -
scripts/get_maintainer.pl |2 +-
2 files changed, 5 insertions(+), 6 deletions(-)
diff --git a/MAINTAINERS b/MAINTAINERS
index 2d02ab5..e68a07a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -100,13 +100,12 @@ Descriptions of section entries:
X: net/ipv6/
matches all files in and below net excluding net/ipv6/
K: Keyword perl extended regex pattern to match content in a
- patch or file, or an affected filename. For instance:
+ patch or file. For instance:
K: of_get_profile
- matches patch or file content, or filenames, that contain
- "of_get_profile"
+ matches patches or files that contain "of_get_profile"
K: \b(printk|pr_(info|err))\b
- matches patch or file content, or filenames, that contain one or
- more of the words printk, pr_info or pr_err
+ matches patches or files that contain one or more of the words
+ printk, pr_info or pr_err
One regex pattern per line. Multiple K: lines acceptable.
Note: For the hard of thinking, this list is meant to remain in alphabetical
diff --git a/scripts/get_maintainer.pl b/scripts/get_maintainer.pl
index 27dc5cb..5e4fb14 100755
--- a/scripts/get_maintainer.pl
+++ b/scripts/get_maintainer.pl
@@ -611,7 +611,7 @@ sub get_maintainers {
$hash{$tvi} = $value_pd;
}
}
- } elsif ($type eq 'K' || $type eq 'N') {
+ } elsif ($type eq 'N') {
if ($file =~ m/$value/x) {
$hash{$tvi} = 0;
}
--
1.7.10.4
[PATCH V2 3/3] get_maintainer: prevent keywords from matching filenames
On 03/06/2013 05:30 PM, Joe Perches wrote: > On Wed, 2013-03-06 at 17:29 -0700, Stephen Warren wrote: >> From: Stephen Warren >> >> This reverts most of eb90d08 "get_maintainer: allow keywords to match >> filenames"; all except the parts that are required to implement the new >> N: entry type. > > Just combine patches 1 and 3 into a single patch. That would break bisectability; using MAINTAINERS after applying a squashed 1+3 but without patch 2 applied would not operate as desired.
[PATCH V2 3/3] get_maintainer: prevent keywords from matching filenames
On 03/06/2013 05:40 PM, Joe Perches wrote: > On Wed, 2013-03-06 at 17:34 -0700, Stephen Warren wrote: >> On 03/06/2013 05:30 PM, Joe Perches wrote: >>> On Wed, 2013-03-06 at 17:29 -0700, Stephen Warren wrote: >>>> From: Stephen Warren >>>> >>>> This reverts most of eb90d08 "get_maintainer: allow keywords to match >>>> filenames"; all except the parts that are required to implement the new >>>> N: entry type. >>> >>> Just combine patches 1 and 3 into a single patch. >> >> That would break bisectability; using MAINTAINERS after applying a >> squashed 1+3 but without patch 2 applied would not operate as desired. > > > > Which is why I showed the whole thing in a single patch. > No worries if it's simply to increase the patch count... I'm not sure what showed refers to? I guess if I squashed /everything/ into a single commit (i.e. the change to the Tegra section in MAINTAINERS too) then there wouldn't be any bisect issues. I really don't care about patch count. The reason for >1 patch is that there's often push-back if doing logically separate things (adding N: feature, modifying a MAINTAINERS entry to use it, removing part of K: feature) in a single patch...
[PATCH V3] get_maintainer: use filename-only regex match for Tegra
From: Stephen Warren <swar...@nvidia.com>
Create a new N: entry type in MAINTAINERS which performs a regex match
against filenames; either those extracted from patch +++ or --- lines,
or those specified on the command-line using the -f option.
This provides the same benefits as using a K: regex option to match a
set of filenames (see commit eb90d08 "get_maintainer: allow keywords to
match filenames"), but without the disadvantage that "random" file
content, such as comments, will ever match the regex. Hence, revert most
of that commit.
Switch the Tegra entry from using K: to N:
Reported-by: Marcin Slusarz
Suggested-by: Joe Perches
Signed-off-by: Stephen Warren
---
v2: Corrected typo in MAINTAINERS documentation
v3: Squash 3 patches into one.
---
MAINTAINERS | 14 --
scripts/get_maintainer.pl |2 +-
2 files changed, 9 insertions(+), 7 deletions(-)
diff --git a/MAINTAINERS b/MAINTAINERS
index 9561658..e68a07a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -90,6 +90,9 @@ Descriptions of section entries:
F: drivers/net/* all files in drivers/net, but not below
F: */net/* all files in "any top level directory"/net
One pattern per line. Multiple F: lines acceptable.
+ N: Files and directories with regex patterns.
+ N: [^a-z]tegra all files whose patch contains the word tegra
+ One pattern per line. Multiple N: lines acceptable.
X: Files and directories that are NOT maintained, same rules as F:
Files exclusions are tested before file matches.
Can be useful for excluding a specific subdirectory, for instance:
@@ -97,13 +100,12 @@ Descriptions of section entries:
X: net/ipv6/
matches all files in and below net excluding net/ipv6/
K: Keyword perl extended regex pattern to match content in a
- patch or file, or an affected filename. For instance:
+ patch or file. For instance:
K: of_get_profile
- matches patch or file content, or filenames, that contain
- "of_get_profile"
+ matches patches or files that contain "of_get_profile"
K: \b(printk|pr_(info|err))\b
- matches patch or file content, or filenames, that contain one or
- more of the words printk, pr_info or pr_err
+ matches patches or files that contain one or more of the words
+ printk, pr_info or pr_err
One regex pattern per line. Multiple K: lines acceptable.
Note: For the hard of thinking, this list is meant to remain in alphabetical
@@ -7848,7 +7850,7 @@ L:linux-tegra at vger.kernel.org
Q: http://patchwork.ozlabs.org/project/linux-tegra/list/
T: git
git://git.kernel.org/pub/scm/linux/kernel/git/swarren/linux-tegra.git
S: Supported
-K: (?i)[^a-z]tegra
+N: [^a-z]tegra
TEHUTI ETHERNET DRIVER
M: Andy Gospodarek
diff --git a/scripts/get_maintainer.pl b/scripts/get_maintainer.pl
index ce4cc83..5e4fb14 100755
--- a/scripts/get_maintainer.pl
+++ b/scripts/get_maintainer.pl
@@ -611,7 +611,7 @@ sub get_maintainers {
$hash{$tvi} = $value_pd;
}
}
- } elsif ($type eq 'K') {
+ } elsif ($type eq 'N') {
if ($file =~ m/$value/x) {
$hash{$tvi} = 0;
}
--
1.7.10.4
[PATCH V3] get_maintainer: use filename-only regex match for Tegra
On 03/11/2013 03:36 PM, Marcin ?lusarz wrote: > > 11 mar 2013 21:19, "Stephen Warren" <mailto:swarren at wwwdotorg.org>> napisa?(a): >> Create a new N: entry type in MAINTAINERS which performs a regex match >> against filenames; either those extracted from patch +++ or --- lines, >> or those specified on the command-line using the -f option. >> diff --git a/MAINTAINERS b/MAINTAINERS >> + N: Files and directories with regex patterns. >> + N: [^a-z]tegra all files whose patch contains the word tegra > > s/patch/path/ ? It looks like Andrew has fixed this up himself. Thanks very much!
[PATCH REPOST] drm: tegra: don't depend on OF
From: Stephen Warren <swar...@nvidia.com> ARCH_TEGRA always enabled OF, so there's no need for any driver to depend on it. Signed-off-by: Stephen Warren --- drivers/gpu/drm/tegra/Kconfig |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/tegra/Kconfig b/drivers/gpu/drm/tegra/Kconfig index c92955d..965c48c 100644 --- a/drivers/gpu/drm/tegra/Kconfig +++ b/drivers/gpu/drm/tegra/Kconfig @@ -1,6 +1,6 @@ config DRM_TEGRA tristate "NVIDIA Tegra DRM" - depends on DRM && OF && ARCH_TEGRA + depends on DRM && ARCH_TEGRA select DRM_KMS_HELPER select DRM_GEM_CMA_HELPER select DRM_KMS_CMA_HELPER -- 1.7.10.4
bcm2835 (Raspberry Pi) KMS driver
On 10/11/2015 06:39 AM, Stefan Wahren wrote: > Am 09.10.2015 um 23:27 schrieb Eric Anholt: >> This is a respin of the Raspberry Pi KMS series. Now that we've got a >> real clock driver, I can actually set new video modes. Also in this >> version, most of the custom DT stuff from before is gone, thanks to >> finding exynos's platform_driver component matching code (I have sent >> separate patches to drivers/base to make helpers for doing it). >> >> https://github.com/anholt/linux/tree/vc4-kms-squash-2 > > I want to point out that git format-patch could prepare a nice cover > letter and usually the changelog should go there. Well, I guess you could put it there, but that wouldn't remove the need to put the changelog in the individual patches too, so that reviewers don't have to switch back and forth between different messages just to find out what changed in each patch. +1 on sending the cover letter using git format-patch/send-email thoughl; the threading here is a little odd.
[PATCH v5 00/11] Improvements to Tegra-based Chromebook support
On 02/12/2015 01:50 AM, Tomeu Vizoso wrote: > Hello, > > this series adds support for the Tegra-based HP Chromebook 14 (aka nyan > blaze), which is very similar to the Acer Chromebook 13 (aka nyan big). > Because they both include tegra124-nyan.dtsi, some improvements to Blaze > support have also benefitted the Big. I have tested that USB2, the panels, > HDMI, the trackpad, Wifi and sound work on both. > > The leaf DTs contain the whole pinmux configuration as generated by > tegra-pinmux-scripts. I chose to not put the common configuration in the > common dtsi so we can paste the output as is and be sure that the kernel > doesn't diverge from the canonical data. At a quick glance this series looks OK, Acked-by: Stephen Warren
[alsa-devel] [RFC] ASoC: snd_soc_jack for HDMI audio: does it make sense?
On 08/23/2012 07:44 PM, Ricardo Neri wrote: > On 08/22/2012 02:55 AM, Takashi Iwai wrote: >> At Tue, 21 Aug 2012 19:58:02 -0500, >> Ricardo Neri wrote: ... >>> Maybe the lack of audio support in drm is because the audio users should >>> not talk to drm directly but to a lower level component (omapdrm, >>> omapdss?). However, today there exists video technology supports audio >>> as well, such as DisplayPort or HDMI. Could it make more sense now to >>> provide audio support? >> >> The reason is that the audio and video handling are already separated >> in the hardware level, at least, for desktop graphics. > > Separated in what sense? Do they have separate register banks in For NVIDIA desktop GPUs, this is certainly true, and I think so for any Intel Azalia/HDA controller. The separate register banks are in different PCI functions on the chip. The Intel Azalia/HDA spec also architects specific ways that the audio and video parts interact (i.e. ELD representation of EDID data, unsolicited response messages when the video state changes, etc.) Oh, I see Takashi mentioned this below. > independent power domains? Most likely yes. > Can audio an video work with complete > independence. What happens if someone decides to power off video. Is the > audio able to continue if required? I believe audio DMA isn't affect by the video state, but I'm not 100% sure of that. >> The audio infoframe is passed via ELD to the audio controller part >> upon plug/unplugging via HD-audio unsolicited event, and the audio >> driver sets up the stuff according to the given ELD. Thus no extra >> interface between drm and ALSA was required in the kernel API level, >> so far. > > I see that the unsolicited event is used only to parse the EDID, > correct? It also notifies about the jack status. Hence, if there the > cable is disconnected the application will know and act accordingly. Is > this understanding correct? The kernel will know, and then passes the information on to user-space.
[RFC v2 1/8] video: tegra: Add nvhost driver
On 11/30/2012 03:38 AM, Thierry Reding wrote: > On Fri, Nov 30, 2012 at 10:56:39AM +0200, Terje Bergstr?m wrote: >> On 29.11.2012 13:47, Thierry Reding wrote: >>> On Thu, Nov 29, 2012 at 12:21:04PM +0200, Terje Bergstr?m >>> wrote: Tegra20 and Tegra30 are compatible, but future chips are not. I was hoping we would be ready in upstream kernel for future chips. >>> >>> I think we should ignore that problem for now. Generally >>> planning for any possible combination of incompatibilities >>> leads to overgeneralized designs that require precisely these >>> kinds of indirections. >>> >>> Once some documentation for Tegra 40 materializes we can start >>> thinking about how to encapsulate the incompatible code. >> >> I think here our perspectives differ a lot. That is natural >> considering the company I work for and company you work for, so >> let's try to sync the perspective. >> >> In my reality, whatever is in market is old news and I barely >> work on them anymore. Upstreaming activity is the exception. 90% >> of my time is spent dealing with future chips which I know cannot >> be handled without this split to logical and physical driver >> parts. >> >> For you, Tegra2 and Tegra3 are the reality. > > To be fair, Tegra2 and Tegra3 are the reality for *everyone* > *outside* NVIDIA. > > It's great that you spend most of your time working with future > chips, but unless you submit the code for inclusion or review > nobody upstream needs to be concerned about the implications. Most > people don't have time to waste so we naturally try to keep the > maintenance burden to a minimum. > > The above implies that when you submit code it shouldn't contain > pieces that prepare for possible future extensions which may or may > not be submitted (the exception being if such changes are part of a > series where subsequent patches actually use them). The outcome is > that the amount of cruft in the mainline kernel is kept to a > minimum. And that's a very good thing. I think there's room for letting Terje's complete knowledge of future chips guide the design of the current code that's sent upstream. Certainly we shouldn't add a ton of unnecessary abstraction layers right now that aren't needed for Tegra20/30, but if there's some decision that doesn't affect the bloat, opaqueness, ... of the current code but one choice is better for future development without serious negatives for the current code, it's pretty reasonable to make that decision rather than the other. (That all said, I haven't really followed the details of this particular point, so I can't say how my comment applies to any decisions being made right now - just that we shouldn't blanket reject future knowledge when making decisions) After all, making the right decision now will reduce the number/size of patches later, and hence reduce code churn and reviewer load.
[RFC v2 1/8] video: tegra: Add nvhost driver
On 12/01/2012 07:58 AM, Thierry Reding wrote:
> On Sat, Dec 01, 2012 at 01:31:02PM +0200, Terje Bergstr?m wrote:
...
>> I was thinking of definitions like this:
>>
>> static inline u32 host1x_sync_cfpeek_ctrl_cfpeek_addr_f(u32 v) {
>> return (v & 0x1ff) << 0; }
>>
>> versus
>>
>> #define host1x_sync_cfpeek_ctrl_cfpeek_addr_f(v) ((v) >> 16) &
>> 0x3ff
>>
>> Both of these produce the same machine code and have same usage,
>> but the latter has type checking and code coverage analysis and
>> the former is (in my eyes) clearer. In both of these cases the
>> usage is like this:
>>
>> writel(host1x_sync_cfpeek_ctrl_cfpeek_ena_f(1) |
>> host1x_sync_cfpeek_ctrl_cfpeek_channr_f(chid) |
>> host1x_sync_cfpeek_ctrl_cfpeek_addr_f(rd_ptr), m->sync_aperture +
>> host1x_sync_cfpeek_ctrl_r());
>
> Again there's no precedent for doing this with static inline
> functions. You can do the same with macros. Type checking isn't an
> issue in these cases since we're talking about bitfields for which
> no proper type exists.
I suspect the inline functions could encode signed-vs-unsigned fields,
perhaps catch u8 variables when they should have been u32, etc.?
[PATCH] drm: tegra: Use framebuffer pitch as line stride
On 12/03/2012 08:00 PM, Mark Zhang wrote: > On 11/28/2012 02:37 PM, Mark Zhang wrote: >> On 11/28/2012 05:39 AM, Stephen Warren wrote: >>> On 11/27/2012 11:17 AM, Stephen Warren wrote: >>>> On 11/26/2012 08:16 PM, Mark Zhang wrote: >>>>> On 11/27/2012 06:37 AM, Stephen Warren wrote: >>>>>> On 11/22/2012 12:37 PM, Thierry Reding wrote: >>>>>>> Instead of using the stride derived from the display mode, use the pitch >>>>>>> associated with the currently active framebuffer. This fixes a bug where >>>>>>> the LCD display content would be skewed when enabling HDMI with a video >>>>>>> mode different from that of the LCD. >>>>>> >>>>>> This patch certainly doesn't cause any additional issues for me, so: >>>>>> >>>>>> Tested-by: Stephen Warren >>>>>> >>>>>> Howwever, it still doesn't allow both Cardhu's LCD panel and external >>>>>> HDMI port (1080p) to be active at once. If I boot with both enabled, or >>>>>> boot with just the LCD enabled and hot-plug HDMI, as soon as both heads >>>>>> are active, then some kind of display corruption starts; it looks like a >>>>>> clocking issue or perhaps memory underflow. >>>>> >>>>> I haven't observed this issue. What kind of display corruption you mean? >>>>> Did it recover after some seconds or the display in LVDS panel was >>>>> always corrupted? >>>>> >>>>> During my testing, I connected HDMI while booting cardhu and I can see >>>>> the LVDS and HDMI working with no corruptions. >>>> >>>> For your viewing pleasure (and playing with my new phone) :-) >>>> http://www.youtube.com/watch?v=ZJxJnONz7DA >>>> >>>> The external monitor is 1920x1200 I believe. >>> >>> Jon Mayo says the corruption in the video is display (memory fetch) >>> underflow. Perhaps this is because (IIRC) the BCT I'm using on Cardhu >>> programs the memory controller at a slow rate, and the bootloader and/or >>> kernel is supposed to bump up the rate to the max, but that's not >>> implemented anywhere yet upstream. If you're testing with "fastboot" >>> instead of U-Boot, that might be re-programming the memory frequencies, >>> and hence avoiding this. >>> >> >> All right, I just test the framebuffer console and "xinit", I didn't >> install the whole ubuntu. >> >> I'll install the ubuntu in my cardhu and see whether I have this kind of >> issues. > > Hi swarren, I installed ubuntu 12.04 in l4t and didn't observe the issue > you described. The display worked with no corruptions. I can show you > the video if you want. > > What I used for testing is a cardhu board with our downstream U-Boot. > > But the HDMI didn't work. The HDMI monitor showed this: "CANNOT DISPLAY > THIS VIDEO MODE, CHANGE COMPUTER DISPLAY INPUT TO 1920x1080 at 60HZ". So > sounds like the clock setting has some problems... I'll have a look at this. Oh, I thought I'd followed up on this - Jon Mayo said it was display underflow due to lack of memory bandwidth. IIRC, this may be due to the BCT programming the memory controller for conservative settings that don't require non-default voltages from the PMIC, with the expectation that the bootloader or kernel will reprogram everything for correct performance.
First version of host1x intro
On 12/06/2012 01:13 AM, Mark Zhang wrote: > On 12/06/2012 04:00 PM, Lucas Stach wrote: >> Am Donnerstag, den 06.12.2012, 15:49 +0800 schrieb Mark Zhang: > [...] >>> >>> OK. So these relocation addresses are used to let userspace tells kernel >>> which buffers mentioned in the command should be relocated to addresses >>> which host1x clients able to reach. >>> >> Yep, preferably all buffers referenced by a command stream should >> already be set up in such a position (CMA with Tegra2) or the relocation >> should be nothing more than setting up IOMMU page tables (Tegra3). >> >>> I'm also wondering that, if our driver understands the stuffs in the >>> commands, maybe we can find out all addresses in the command, in that >>> way, we will not need userspace tells us which are the addresses need to >>> be relocated, right? >> >> No. How will the kernel ever know which buffer gets referenced in a >> command stream? All the kernel sees is is a command stream with >> something like "blit data to address 0xADDR" in it. The only info that >> you can gather from that is that there must be some buffer to blit into. >> Neither do you know which buffer the stuff should be going to, nor can >> you know if you blit to offset zero in this buffer. It's perfectly valid >> to only use a subregion of a buffer. >> >> Or maybe I'm misunderstanding you and you mean it the other way around. >> You don't let userspace dictate the addresses, the relocation >> information just tells the kernel to find the addresses of the >> referenced buffers for you and insert them, instead of the dummy >> information, into the command stream. > > Yes, I think this is what I mean. No dummy information in the command > stream, userspace just fills the address which it uses(actually this is > cpu address of the buffer) in the command stream, and our driver must > have a HashTable or something which contains the buffer address pair -- > (cpu address, dma address), so our driver can find the dma addresses for > every buffer then modify the addresses in the command stream. Typically there would be no CPU address; there's no need in most cases to ever map most buffers to the CPU. Automatically parsing the buffer sounds like an interesting idea, but then the kernel relocation code would have to know the meaning of every single register or command-stream "method" in order to know which of them take a buffer address as an argument. I am not familiar with this HW specifically, so perhaps it's much more regular than I think and it's actually easy to do that, but I imagine it'd be a PITA to implement that (although perhaps we have to for the command-stream validation stuff anyway?). Also, it'd be a lot quicker at least for larger command buffers to go straight to the few locations in the command stream where a buffer is referenced (i.e. use the side-band metadata for relocation) rather than having the CPU re-read the entire command stream in the kernel to parse it.
[RFC v2 6/8] gpu: drm: tegra: Remove redundant host1x
On 12/13/2012 01:57 AM, Thierry Reding wrote: > On Thu, Dec 13, 2012 at 10:48:55AM +0200, Terje Bergstr?m wrote: >> On 12.12.2012 18:08, Thierry Reding wrote: >>> I've briefly discussed this with Stephen on IRC because I >>> thought I had remembered him objecting to the idea of adding a >>> dummy device just for this purpose. It turns out, however, that >>> what he didn't like was to add a dummy node to the DT just to >>> make this happen, but he has no (strong) objections to a dummy >>> platform device. >>> >>> While I'm not very happy about that solution, I've been going >>> over it for a week now and haven't come up with any better >>> alternative that doesn't have its own disadvantages. So perhaps >>> we should go ahead and implement that. For the host1x driver >>> this really just means creating a platform device and adding it >>> to the system, with some of the fields tweaked to make things >>> work. >> >> Even the virtual device is not too beautiful. The problem is that >> the virtual device is not physical parent for DC, HDMI, etc, so >> dev_get_drvdata(pdev->dev.parent) returns the data from host1x >> device, not the virtual device. >> >> We'll post with something that goes around this, but it's not >> going to be too pretty. Let's try to find the solution once we >> get the code out. > > After some more discussion with Stephen on IRC we came to the > conclusion that the easiest might be to have tegra-drm call into > host1x with something like: > > void host1x_set_drm_device(struct host1x *host1x, struct device > *dev); If host1x is registering the dummy device that causes tegradrm to be instantiated, then presumably there's no need for the API above, since host1x will already have the struct device * for tegradrm, since it created it? > Once the dummy device has been properly set up and have each client > use > > struct device *host1x_get_drm_device(struct host1x *host1x); > > to obtain a pointer to the dummy device, such that it can receive > the driver-private data using dev_get_drvdata() on the returned > device. As long as tegra-drm hasn't registered with host1x yet, the > above function could return ERR_PTR(-EPROBE_DEFER), so that > dependencies are automatically handled. This is required because, > tegra-drm not being the parent of the child devices, it can be > guaranteed that it is probed before any of the children. > > That way we should be able to get around any circular > dependencies, since we only call into host1x from tegra-drm but not > the other way around.
[RFC v2 6/8] gpu: drm: tegra: Remove redundant host1x
On 12/13/2012 11:09 PM, Terje Bergstr?m wrote: > On 13.12.2012 19:58, Stephen Warren wrote: >> On 12/13/2012 01:57 AM, Thierry Reding wrote: >>> After some more discussion with Stephen on IRC we came to the >>> conclusion that the easiest might be to have tegra-drm call into >>> host1x with something like: >>> >>> void host1x_set_drm_device(struct host1x *host1x, struct device >>> *dev); >> >> If host1x is registering the dummy device that causes tegradrm to be >> instantiated, then presumably there's no need for the API above, since >> host1x will already have the struct device * for tegradrm, since it >> created it? > > I didn't add the dummy device in my latest patch set. I first set out to > add it, and moved the drm global data to be drvdata of that device. Then > I noticed that it doesn't actually help at all. > > The critical accesses to the global data are from probes of DC, HDMI, > etc. OK > They want to get the global data by getting drvdata of their parent. There's no reason that /has/ to be so; they can get any information from wherever it is, rather than trying to require it to be somewhere it isn't. > The dummy device is not their parent - host1x is. There's no > connection between the dummy and the real client devices. It's quite possible for the client devices to ask their actual parent (host1x) for the tegradrm struct device *, by calling a host1x API, and have host1x implement that API by returning the dummy/virtual device that it created. That should be just 1-2 lines of code to implement in host1x, plus maybe a couple more to correctly return -EPROBE_DEFERRED when appropriate.
[RFC v2 6/8] gpu: drm: tegra: Remove redundant host1x
On 12/16/2012 09:37 AM, Terje Bergstr?m wrote: ... > ... Sure we could tell DC to ask its parent > (host1x), and call host1x driver with platform_device pointer found that > way, and host1x would return a pointer to tegradrm's data. Hanging the > data onto host1x driver is just a more complicated way of implementing > global data No it's not; at that point, the data is no longer global, but specific to the driver instance.
[RFC v2 6/8] gpu: drm: tegra: Remove redundant host1x
On 12/20/2012 02:17 AM, Terje Bergstr?m wrote: > On 16.12.2012 14:16, Thierry Reding wrote: >> Okay, so we're back on the topic of using globals. I need to assert >> again that this is not an option. If we were to use globals, then we >> could just as well leave out the dummy device and just do all of that in >> the tegra-drm driver's initialization function. > > I found a way of dropping the global in a straightforward way, and > introduce dummy device for drm_platform_init(). > > I added dummy device and driver, and moved the tegradrm global > (previously called struct host1x *host1x) allocation to happen in the > probe. In addition, probe calls device tree node traversal to do the > tegra_drm_add_client() calls. The dummy device is owner for this global. > > I changed the device tree node traversal so that it goes actually > through each host1x child, checks if it's supported by tegradrm, and if > so, sets its drvdata to point to the tegradrm data. I'm not sure that sounds right. drvdata is something that a driver should manage itself. What's wrong with just having each device ask the host1x (its parent) for a pointer to the (dummy) tegradrm device. That seems extremely simple, and doesn't require abusing existing stuff like drvdata for non-standard purposes.
[RFC v2 6/8] gpu: drm: tegra: Remove redundant host1x
On 12/20/2012 10:46 AM, Terje Bergstr?m wrote: > On 20.12.2012 19:14, Stephen Warren wrote: >> I'm not sure that sounds right. drvdata is something that a driver >> should manage itself. >> >> What's wrong with just having each device ask the host1x (its parent) >> for a pointer to the (dummy) tegradrm device. That seems extremely >> simple, and doesn't require abusing existing stuff like drvdata for >> non-standard purposes. > > This is tegradrm's own data, and it's tegradrm which accesses the > pointer. So it's entirely something that the driver takes care of itself. So it's fine for the tegradrm driver to manipulate the tegradrm (virtual) device's drvdata pointer. It's not fine for tegradrm to manipulate the drvdata pointer of other devices; the host1x children. The drvdata pointer for other devices is reserved for those devices' driver's use only.
[RFC v2 6/8] gpu: drm: tegra: Remove redundant host1x
On 12/20/2012 02:34 PM, Terje Bergstr?m wrote: > On 20.12.2012 22:30, Thierry Reding wrote: >> The problem with your proposed solution is that, even any of Stephen's >> valid objections aside, it won't work. Once the tegra-drm module is >> unloaded, the driver's data will be left in the current state and the >> link to the dummy device will be lost. > > The dummy device is created by tegradrm's module init, because it's used No, the tegradrm driver object might be registered by tegradrm's module init, but the dummy tegradrm platform device would need to be created/registered by host1x's probe. Otherwise, the device would get created even if there was no host1x/... in the system (disabled for some reason, multi-SoC kernel, ...)
[RFC v2 6/8] gpu: drm: tegra: Remove redundant host1x
On 12/20/2012 02:50 PM, Thierry Reding wrote: > On Thu, Dec 20, 2012 at 11:34:26PM +0200, Terje Bergstr?m wrote: >> On 20.12.2012 22:30, Thierry Reding wrote: >>> The problem with your proposed solution is that, even any of >>> Stephen's valid objections aside, it won't work. Once the >>> tegra-drm module is unloaded, the driver's data will be left in >>> the current state and the link to the dummy device will be >>> lost. >> >> The dummy device is created by tegradrm's module init, because >> it's used only by tegradrm. When tegradrm is unloaded, all the >> tegradrm devices and drivers are unloaded and freed, including >> the dummy one. > > No, the children platform devices of host1x are not freed, so > their driver data will still contain the data set by the previous > call to their driver's .probe() function. > > But reading what you said again, you were proposing to set the > children driver data from the tegra-drm dummy device's .probe(). In > that case it would probably work. Many things would work, but since tegradrm isn't (or at least should not be) the driver for the platform devices which are host1x's children, it shouldn't be randomly screwing around with those devices' drvdata fields.
[RFC v2 6/8] gpu: drm: tegra: Remove redundant host1x
On 12/21/2012 01:57 AM, Arto Merilainen wrote: > On 12/20/2012 07:14 PM, Stephen Warren wrote: >> >> What's wrong with just having each device ask the host1x (its parent) >> for a pointer to the (dummy) tegradrm device. That seems extremely >> > > We are talking about creating a dummy device because: > - we need to give something for drm_platform_init(), > - drm related data should be stored somewhere, Yes to those too, I believe. > - some data must be passed to all driver probe() functions. This is not > hw-related data, but just some lists that are used to ensure that all > drivers have been initialised before calling drm_platform_init(). I haven't really thought through /when/ host1x would create the dummy device. I guess if tegradrm really must initialize after all the devices that it uses (rather than using something like deferred probe) then the above may be true. > All these issues are purely tegra-drm related and solving them elsewhere > (in host1x) would be just wrong! host1x would not even use the dummy > device for anything so creating it there seems bizarre. I see the situation more like: * There's host1x hardware. * There's a low-level driver just for host1x itself; the host1x driver. * There's a high-level driver for the entire host1x complex of devices. That is tegradrm. There may be more high-level drivers in the future (e.g. v4l2 camera driver if it needs to aggregate a bunch of host1x sub-devices liek tegradrm does). * The presence of the host1x DT node logically implies that both the two drivers in the previous two points should be instantiated. * Linux instantiates a single device per DT node. * So, it's host1x's responsibility to instantiate the other device(s). I think it's reasonable for the host1x driver to know exactly what features the host1x HW complex supports; raw host1x access being one, and the higher level concept of a tegradrm being another. So, it's reasonable for host1x to trigger the instantiation of tegradrm. * If DRM core didn't stomp on the device object's drvdata (or whichever field it is), I would recommend not creating a dummy device at all, but rather having the host1x driver directly implement multiple driver interfaces. There are many examples like this already in the kernel, e.g. combined GPIO+IRQ driver, combined GPIO+IRQ+pinctrl driver, etc.
[PATCHv4 5/8] drm: tegra: Remove redundant host1x
On 12/21/2012 11:50 PM, Terje Bergstr?m wrote:
> On 21.12.2012 16:36, Thierry Reding wrote:
>> On Fri, Dec 21, 2012 at 01:39:21PM +0200, Terje Bergstrom wrote:
>>> +static struct platform_driver tegra_drm_platform_driver = {
>>> + .driver = {
>>> + .name = "tegradrm",
>>
>> This should be "tegra-drm" to match the module name.
>
> We've actually created two problems.
>
> First is that the device name should match driver name which should
> match module name. But host1x doesn't know the module name of tegradrm.
There's no hard requirement for the device/driver name to match the
module name. It's good thing to do, but nothing will blow up if it don't
(modules can use MODULE_ALIAS() to declare which drivers they expose).
But, what's the problem with host1x knowing the driver name; the host1x
driver and tegradrm driver are both part of the same code-base, so this
seems trivial to achieve.
> Second problem is that host1x driver creates tegradrm device even if
> tegradrm isn't loaded to system.
That's fine. If there's no driver, the device simply won't be probe()d.
That's just like a device node existing in device tree, but the driver
for it not being enabled in the kernel, or the relevant module not being
inserted.
> These mean that the device has to be created in tegra-drm module to have
I definitely disagree here.
[RFC 2/4] iommu: tegra/gart: Add device tree support
On 04/11/2012 06:10 AM, Thierry Reding wrote: > This commit adds device tree support for the GART hardware available on > NVIDIA Tegra 20 SoCs. > > Signed-off-by: Thierry Reding > --- > arch/arm/boot/dts/tegra20.dtsi |6 ++ > arch/arm/mach-tegra/board-dt-tegra20.c |1 + > drivers/iommu/tegra-gart.c | 10 ++ > 3 files changed, 17 insertions(+) I think I'd prefer at least the tegra20.dtsi change split out into a separate patch so I can queue it in a "dt" topic branch in the Tegra repo. It might be a good idea to split this into two on an arch/arm vs. drivers/iommu boundary. Looking at Olof's branches for 3.4, that split is what happened there. Finally, there should be a binding documentation file in Documentation/devicetree/bindings in order to specify the number of reg property entries needed, and their meaning, since there's more than 1 (even though you did comment it nicely in the .dtsi file)
[RFC 3/4] drm: fixed: Add dfixed_frac() macro
On 04/11/2012 06:10 AM, Thierry Reding wrote: > This commit is taken from the Chromium tree and was originally written > by Robert Morell. Maybe just cherry-pick it from there? That way, the git authorship will show up as Robert.
[RFC 4/4] drm: Add NVIDIA Tegra support
On 04/11/2012 06:10 AM, Thierry Reding wrote:
> This commit adds a very basic DRM driver for NVIDIA Tegra SoCs. It
> currently has rudimentary GEM support and can run a console on the
> framebuffer as well as X using the xf86-video-modesetting driver.
> Only the RGB output is supported. Quite a lot of things still need
> to be worked out and there is a lot of room for cleanup.
I'll let Jon Mayo comment on the actual driver implementation, since
he's a lot more familiar with Tegra's display hardware. However, I have
some general comments below.
> .../devicetree/bindings/gpu/drm/tegra.txt | 24 +
> arch/arm/mach-tegra/board-dt-tegra20.c |3 +
> arch/arm/mach-tegra/tegra2_clocks.c|8 +-
> drivers/gpu/drm/Kconfig|2 +
> drivers/gpu/drm/Makefile |1 +
> drivers/gpu/drm/tegra/Kconfig | 10 +
> drivers/gpu/drm/tegra/Makefile |5 +
> drivers/gpu/drm/tegra/tegra_drv.c | 2241
>
> drivers/gpu/drm/tegra/tegra_drv.h | 184 ++
> include/drm/tegra_drm.h| 44 +
Splitting this patch into two, between arch/arm and drivers/gpu would be
a good idea.
> diff --git a/Documentation/devicetree/bindings/gpu/drm/tegra.txt
> b/Documentation/devicetree/bindings/gpu/drm/tegra.txt
> + drm at 5420 {
> + compatible = "nvidia,tegra20-drm";
This doesn't seem right; there isn't a "DRM" hardware module on Tegra,
since "DRM" is a Linux/software-specific term.
I'd at least expect to see this compatible flag be renamed to something
more like "nvidia,tegra20-dc" (dc==display controller).
Since Tegra has two display controller modules (I believe identical?),
and numerous other independent(?) blocks, I'd expect to see multiple
nodes in device tree, one per hardware block, such that each block gets
its own device and driver. That said, I'm not familiar enough with
Tegra's display and graphics HW to know if this makes sense. Jon, what's
your take here? The clock change below, and in particular the original
code there that we use downstream, lends weight to my argument.
> + reg = < 0x5420 0x0004/* display A */
> + 0x5424 0x0004/* display B */
> + 0x5800 0x0200 >; /* GART aperture */
> + interrupts = < 0 73 0x04/* display A */
> +0 74 0x04 >; /* display B */
> +
> + lvds {
> + type = "rgb";
These sub-nodes probably want a "compatible" property rather than a
"type" property.
> + size = <345 194>;
> +
> + default-mode {
> + pixel-clock = <61715000>;
> + vertical-refresh = <50>;
> + resolution = <1366 768>;
> + bits-per-pixel = <16>;
> + horizontal-timings = <4 136 2 36>;
> + vertical-timings = <2 4 21 10>;
> + };
> + };
I imagine that quite a bit of thought needs to be put into the output
part of the binding in order to:
* Model the outputs/connectors separately from display controllers.
* Make sure that the basic infra-structure for representing an output is
general enough to be extensible to all the kinds of outputs we support,
not just the LVDS output.
* We were wondering about putting an EDID into the DT to represent the
display modes, so that all outputs had EDIDs rather than "real" monitors
having EDIDs, and fixed internal displays having some other
representation of capabilities.
I'm hoping that Jon will drive this.
> diff --git a/arch/arm/mach-tegra/tegra2_clocks.c
> b/arch/arm/mach-tegra/tegra2_clocks.c
> - PERIPH_CLK("disp1", "tegradc.0",NULL, 27, 0x138,
> 6, mux_pllp_plld_pllc_clkm, MUX), /* scales with voltage and
> process_id */
> - PERIPH_CLK("disp2", "tegradc.1",NULL, 26, 0x13c,
> 6, mux_pllp_plld_pllc_clkm, MUX), /* scales with voltage and
> process_id */
> + PERIPH_CLK("disp1", "tegra-drm",NULL, 27, 0x138,
> 6, mux_pllp_plld_pllc_clkm, MUX), /* scales with voltage and
> process_id */
> + PERIPH_CLK("disp2", "tegra-drm",NULL, 26, 0x13c,
> 6, mux_pllp_plld_pllc_clkm, MUX), /* scales with voltage and
> process_id */
This doesn't seem right, and couples back to my assertion above that the
two display controller modules probably deserve separate device objects,
named e.g. tegradc.*.
> diff --git a/drivers/gpu/drm/tegra/Kconfig b/drivers/gpu/drm/tegra/Kconfig
> new file mode 100644
> index 000..f3382c9
> --- /dev/null
> +++ b/drivers/gpu/drm/tegra/Kconfig
> @@ -0,0 +1,10 @@
> +config DRM_TEGRA
> +
[RFC 4/4] drm: Add NVIDIA Tegra support
On 04/12/2012 12:50 AM, Thierry Reding wrote:
> * Stephen Warren wrote:
>> On 04/11/2012 06:10 AM, Thierry Reding wrote:
>>> This commit adds a very basic DRM driver for NVIDIA Tegra SoCs. It
>>> currently has rudimentary GEM support and can run a console on the
>>> framebuffer as well as X using the xf86-video-modesetting driver.
>>> Only the RGB output is supported. Quite a lot of things still need
>>> to be worked out and there is a lot of room for cleanup.
...
>>> diff --git a/Documentation/devicetree/bindings/gpu/drm/tegra.txt
>>> b/Documentation/devicetree/bindings/gpu/drm/tegra.txt
...
>> This doesn't seem right, and couples back to my assertion above that the
>> two display controller modules probably deserve separate device objects,
>> named e.g. tegradc.*.
>
> I think I understand where you're going with this. Does the following look
> more correct?
>
> disp1 : dc at 5420 {
> compatible = "nvidia,tegra20-dc";
> reg = <0x5420, 0x0004>;
> interrupts = <0 73 0x04>;
> };
>
> disp2 : dc at 5424 {
> compatible = "nvidia,tegra20-dc";
> reg = <0x5424, 0x0004>;
> interrupts = <0 74 0x04>;
> };
Those look good.
> drm {
> compatible = "nvidia,tegra20-drm";
I'm don't think having an explicit "drm" node is the right approach; drm
is after all a SW term and the DT should be describing HW. Having some
kind of top-level node almost certainly makes sense, but naming it
something related to "tegra display" than "drm" would be appropriate.
> lvds {
> compatible = "...";
> dc = <>;
> };
Aren't the outputs separate HW blocks too, such that they would have
their own compatible/reg properties and their own drivers, and be
outside the top-level drm/display node?
I believe the mapping between the output this node represents and the
display controller ("dc" above) that it uses is not static; the
connectivity should be set up at runtime, and possibly dynamically
alterable via xrandr or equivalent.
> hdmi {
> compatible = "...";
> dc = <>;
> };
> };
>>> +static int tegra_drm_parse_dt(struct platform_device *pdev)
>>> +{
>> ...
>>> + pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
>>> + if (!pdata)
>>> + return -ENOMEM;
>> ...
>>> + dev->platform_data = pdata;
>>
>> I don't think you should assign to dev->platform_data. If you do, then I
>> think the following could happen:
>>
>> * During first probe, the assignment above happens
>> * Module is removed, hence device removed, hence dev->platform_data
>> freed, but not zero'd out
>> * Module is re-inserted, finds that dev->platform_data!=NULL and
>> proceeds to use it.
>
> Actually the code does zero out platform_data in tegra_drm_remove(). In fact
> I did test module unloading and reloading and it works properly. But it
> should probably be zeroed in case drm_platform_init() fails as well.
>
>> Instead, the active platform data should probably be stored in a
>> tegra_drm struct that's stored in the dev's private data.
>> tegra_drm_probe() might then look more like:
>>
>> struct tegra_drm *tdev;
>>
>> tdev = devm_kzalloc();
>> tdev->pdata = pdev->dev.platform_data;
>> if (!tdev->pdata)
>> tdev->pdata = tegra_drm_parse_dt();
>> if (!tdev->pdata)
>> return -EINVAL;
>>
>> dev_set_drvdata(dev, tdev);
>>
>> This is safe, since probe() will never assume that dev_get_drvdata()
>> might contain something valid before probe() sets it.
>
> I prefer my approach over storing the data in an extra field because the
> device platform_data field is where everybody would expect it. Furthermore
> this wouldn't be relevant if we decided not to support non-DT setups.
Drivers are expected to use pre-existing platform data, if provided.
This might happen in order to work around bugs in device tree content.
[RFC 4/4] drm: Add NVIDIA Tegra support
On 04/12/2012 11:44 AM, Thierry Reding wrote:
> * Stephen Warren wrote:
>> On 04/12/2012 12:50 AM, Thierry Reding wrote:
>>> drm {
>>> compatible = "nvidia,tegra20-drm";
>>
>> I'm don't think having an explicit "drm" node is the right approach; drm
>> is after all a SW term and the DT should be describing HW. Having some
>> kind of top-level node almost certainly makes sense, but naming it
>> something related to "tegra display" than "drm" would be appropriate.
>
> In this case there really isn't a HW device that can be represented. But in
> the end it's still the DRM driver that needs to bind to the device. However
> the other graphics devices (MPE, VI/CSI, EPP, GR2D and GR3D) probably need
> to be bound against.
Well, everything graphics-related appears to be grouped within some
container. In the memory map, there's the range 0x5400-547f, and
in the Tegra20 TRM, the first diagram in section 29 "Display Controller"
again lumps all the modules together into one box.
I'd say it's perfectly legitimate to create a device tree node to
represent this aggregate display/graphics-related engine. So, I think
that yes there is a real HW device to represent.
And given that, I don't think we should name the node after some
OS-specific software concept. Device tree is intended to model hardware.
> Would it be possible for someone at NVIDIA to provide some more details about
> what those other devices are? GR2D and GR3D seem obvious, MPE might be video
> decoding, VI/CSI video input and camera interface? As to EPP I have no idea.
MPE is something video encode/decode related. VI/CSI are indeed camera
related. I'm afraid I don't personally know any more details than that,
and even if I did, I'm only allowed to discuss what's in the TRM:-(
I don't think those modules should have much impact on the DRM driver
design if that's what you're worried about. I /believe/ they all just
interact directly with memory rather than the other components,
irrespective of what that first diagram in section 29 might imply.
> Maybe one solution would be to have a top-level DRM device with a register
> map from 0x5400 to 0x547f, which the TRM designates as "host
> registers". Then subnodes could be used for the subdevices.
Ah yes, just what I was thinking above:-)
I don't think you'd /have/ to make the nodes sub-nodes; you could use
phandles to refer from the top-level node to the other components. One
might end up having to use phandles anyway to represent the routing from
DCA or DCB to MIPI or HDMI anyway, so it's possible that using phandles
everywhere might be simpler and more consistent than parent/child
relationships for some things and phandles for other things.
>>> lvds {
>>> compatible = "...";
>>> dc = <>;
>>> };
>>
>> Aren't the outputs separate HW blocks too, such that they would have
>> their own compatible/reg properties and their own drivers, and be
>> outside the top-level drm/display node?
>
> The RGB output is programmed via the display controller registers. For HDMI,
> TVO and DSI there are indeed separate sets of registers in addition to the
> display controller's. So perhaps for those more nodes would be required:
>
> hdmi : hdmi at 5428 {
> compatible = "nvidia,tegra20-hdmi";
> reg = <0x5428 0x0004>;
> };
Yes, looks reasonable.
> And hook that up with the HDMI output node of the "DRM" node:
>
> drm {
> hdmi {
> compatible = "...";
> connector = <>;
> dc = <>;
> };
> };
>
> Maybe with this setup we no longer need the "compatible" property since it
> will already be inherent in the "connector" property. There will have to be
> special handling for the RGB output, which could be the default if the
> "connector" property is missing.
I suspect you'd have something more like:
tegra-graphics {
output-resources = < ... >;
display-controllers = < >;
};
i.e. just a list of all extant devices. Each should provide some common
API so you could just map from phandle to of_node to device object, and
call the appropriate APIs on it.
Finally note that although I didn't really represent it correct above,
there are at least 3 levels of object/hierarchy here:
Display controllers reads from RAM and form a set of pixels to display.
I believe things like cursors, overlays, pan-scan, etc. are resolved
entirely at this level.
Output resources drive certain pins on Tegra in some specif
[RFC 4/4] drm: Add NVIDIA Tegra support
On 04/13/2012 03:14 AM, Thierry Reding wrote:
> * Stephen Warren wrote:
>> On 04/12/2012 11:44 AM, Thierry Reding wrote:
> [...]
>> And given that, I don't think we should name the node after some
>> OS-specific software concept. Device tree is intended to model hardware.
> [...]
>>> Maybe one solution would be to have a top-level DRM device with a register
>>> map from 0x5400 to 0x547f, which the TRM designates as "host
>>> registers". Then subnodes could be used for the subdevices.
>>
>> Ah yes, just what I was thinking above:-)
>
> I came up with the following:
>
> /* host1x */
> host1x : host1x at 5000 {
> reg = <0x5000 0x00024000>;
> interrupts = <0 64 0x04 /* cop syncpt */
> 0 65 0x04 /* mpcore syncpt */
> 0 66 0x04 /* cop general */
> 0 67 0x04>; /* mpcore general */
> };
>
> /* graphics host */
> graphics at 5400 {
> compatible = "nvidia,tegra20-graphics";
>
> #address-cells = <1>;
> #size-cells = <1>;
> ranges = <0 0x5400 0x0800>;
>
> host1x = <>;
>
> /* video-encoding/decoding */
> mpe at 5404 {
> reg = <0x5404 0x0004>;
> interrupts = <0 68 0x04>;
> };
... [a bunch of nodes for graphics-related HW modules]
That all looks reasonable.
> display-controllers = < >;
> outputs = < >;
I don't think you need both the child nodes and those two properties.
In other words, I think you either want:
graphics at 5400 {
... a bunch of child nodes
};
or you want:
disp1 : dc at 5420 {
...
};
disp2 : dc at 5424 {
...
};
... all the other graphics nodes
graphics at 5400 {
display-controllers = < >;
outputs = < >;
};
In the former case, presumably the drivers for the child nodes would
make some API call into the parent node and just register themselves
directly as a certain type of driver, so avoiding the
display-controllers/outputs properties.
> /* initial configuration */
> configuration {
> lvds {
> display-controller = <>;
> output = <>;
> };
>
> hdmi {
> display-controller = <>;
> output = <>;
> };
> };
> };
>
> I added an additional node for the initial configuration so that the driver
> knows which mapping to setup at boot.
Isn't that kind of thing usually set up by the video= KMS-related kernel
command-line option? See Documentation/fb/modedb.txt. Again here, I
think the actual display controllers would be allocated to whichever
outputs get used on a first-come first-serve based, so no need for the
display-controller property above either way.
> What I don't quite see yet is where to
> attach EDID data or pass the phandle to the I2C controller for DDC/EDID
> probing.
I think there's a third node type.
1) Nodes for the display controllers in Tegra (Also known as CRTCs I
believe)
2) Nodes for the video outputs from the Tegra chips (what you have as
outputs above)
3) Connectors, which aren't represented in your DT above yet.
I think you need to add additional nodes for (3). These are where you'd
represent all the EDID/I2C/... stuff. Then, the nodes for the outputs
will point at the nodes for the connectors using phandles, or vice-versa.
(IIRC, although I didn't look at them in detail, the sdrm patches
recently mentioned something about splitting up the various object types
in DRM and allowing them to be represented separately, and I vaguely
recall something along the lines of the types I mention above. I might
expect to have a 1:1 mapping between the DRM object types and DT nodes.)
> The initial configuration is certainly not the right place. Perhaps
> the outputs property should be made a node instead:
>
> outputs {
> lvds_out {
> output = <>;
> edid = <>;
> };
>
> hdmi_out {
> output = <>;
> ddc = <>;
> };
> };
>
> But then "outputs" should probably become something like "connectors"
> instead and the initial configuration refers to the "_out" phandles.
[RFC 4/4] drm: Add NVIDIA Tegra support
On 04/15/2012 02:39 AM, Thierry Reding wrote:
> * Stephen Warren wrote:
>> On 04/13/2012 03:14 AM, Thierry Reding wrote:
>>> display-controllers = < >;
>>> outputs = < >;
>>
>> I don't think you need both the child nodes and those two properties.
>>
>> In other words, I think you either want:
>>
>> graphics at 5400 {
>> ... a bunch of child nodes
>> };
>>
>> or you want:
>>
>> disp1 : dc at 5420 {
>> ...
>> };
>> disp2 : dc at 5424 {
>> ...
>> };
>> ... all the other graphics nodes
>>
>> graphics at 5400 {
>> display-controllers = < >;
>> outputs = < >;
>> };
>>
>> In the former case, presumably the drivers for the child nodes would
>> make some API call into the parent node and just register themselves
>> directly as a certain type of driver, so avoiding the
>> display-controllers/outputs properties.
>
> I think I like the former better. The way I understand it the children of the
> graphics node will have to be registered explicitly by the DRM driver because
> of_platform_populate() doesn't work recursively. That would ensure that the
> DRM driver can setup the CRTC and output registries before the other devices
> call back into the DRM to register themselves.
Yes, with the first way, the DRM driver will have to call
of_platform_populate() recursively to make this work.
The thing here is that the device tree should model hardware, not be
designed purely to match the device registration needs of the DRM
driver. I'm not sure that it's correct to model all those devices as
children of the top-level graphics object; I /think/ all the devices are
flat on a single bus, and hence not children of each-other. That all
said, I guess having the nodes as children isn't too far off how the HW
is designed (even if the register accesses aren't on a child bus, the
modules at least logically are grouped together in an umbrella
situation), so I wouldn't push back on the first option above that you
prefer.
>>> /* initial configuration */
>>> configuration {
>>> lvds {
>>> display-controller = <>;
>>> output = <>;
>>> };
>>>
>>> hdmi {
>>> display-controller = <>;
>>> output = <>;
>>> };
>>> };
>>> };
>>>
>>> I added an additional node for the initial configuration so that the driver
>>> knows which mapping to setup at boot.
>>
>> Isn't that kind of thing usually set up by the video= KMS-related kernel
>> command-line option? See Documentation/fb/modedb.txt. Again here, I
>> think the actual display controllers would be allocated to whichever
>> outputs get used on a first-come first-serve based, so no need for the
>> display-controller property above either way.
>
> Boards should still be able to boot and display a console on the "standard"
> output even if the user doesn't provide a video= option. Shouldn't there be a
> way for a board DTS to specify what the default (or even allowed) connections
> are?
Why wouldn't the default be to light up all outputs that have an
attached display, or an algorithm something like:
* If internal LCD is present, use that
* Else, if HDMI display plugged in, use that
...
> Evaluation hardware like the Harmony might have LVDS, HDMI and VGA connectors
> to provide for a wide range of use cases. The Plutux for instance has only an
> HDMI connector and the Medcom has only LVDS. For the Medcom it would be quite
> confusing for people to suddenly see an HDMI-1 connector pop up f.e. in
> xrandr. It would be equally useless for the Plutux to show up as supporting
> an LVDS or VGA connector.
So the device tree for those devices would disable (or not include) the
connectors that were not present on the board.
...
> I see. Maybe this could be used for board-specific configuration? For
> example, the Plutux could have something like this:
>
> connectors {
> hdmi {
> output = <>;
> ddc = <>;
> };
> };
>
> The Medcom could have:
>
> connectors {
> lvds {
> output = <>;
> edid = <>;
> };
>
[RFC 4/4] drm: Add NVIDIA Tegra support
On 04/16/2012 12:48 PM, Thierry Reding wrote: > * Stephen Warren wrote: ... >>> Has there been any discussion as to how EDID data would best be represented >>> in DT? Should it just be a binary blob or rather some textual >>> representation? >> >> I think a binary blob makes sense - that's the exact same format it'd >> have if read over the DDC I2C bus. > > DTC has /incbin/ for that. Is arch/arm/boot/dts still the correct place for > EDID blobs? I could add tegra-medcom.edid if that's okay. As far as I know, yes. Perhaps we'll want to start putting stuff in SoC-specific sub-directories given the number of files we'll end up with here (irrespective of EDID etc.), but I haven't seen any move towards that yet.
[RFC 4/4] drm: Add NVIDIA Tegra support
On 04/16/2012 01:03 PM, Thierry Reding wrote: ... > I've been looking about for tools to generate EDID data but didn't find > anything useful. Does anyone know of any tool that's more convenient than > manually filling a struct edid and writing that to a file? Sorry, no.
[RFC v2 3/5] i2c: Add of_i2c_get_adapter() function
On 04/25/2012 03:45 AM, Thierry Reding wrote: > This function resolves an OF device node to an I2C adapter registered > with the I2C core. I think this is doing the same thing as a patch I posted recently: http://www.spinics.net/lists/linux-i2c/msg07808.html What's the advantage of one way over the other?
[PATCH v2] of: Add videomode helper
On 07/04/2012 01:56 AM, Sascha Hauer wrote:
> This patch adds a helper function for parsing videomodes from the devicetree.
> The videomode can be either converted to a struct drm_display_mode or a
> struct fb_videomode.
> diff --git a/Documentation/devicetree/bindings/video/displaymode
> b/Documentation/devicetree/bindings/video/displaymode
> +Required properties:
> + - xres, yres: Display resolution
> + - left-margin, right-margin, hsync-len: Horizontal Display timing parameters
> + in pixels
> + upper-margin, lower-margin, vsync-len: Vertical display timing parameters
> in
> + lines
Perhaps bike-shedding, but...
For the margin property names, wouldn't it be better to use terminology
more commonly used for video timings rather than Linux FB naming. In
other words naming like:
hactive, hsync-len, hfront-porch, hback-porch?
> + - clock: displayclock in Hz
> +
> +Optional properties:
> + - width-mm, height-mm: Display dimensions in mm
> + - hsync-active-high (bool): Hsync pulse is active high
> + - vsync-active-high (bool): Vsync pulse is active high
> + - interlaced (bool): This is an interlaced mode
> + - doublescan (bool): This is a doublescan mode
> +
> +There are different ways of describing a display mode. The devicetree
> representation
> +corresponds to the one used by the Linux Framebuffer framework described
> here in
> +Documentation/fb/framebuffer.txt. This representation has been chosen
> because it's
> +the only format which does not allow for inconsistent parameters.Unlike the
> Framebuffer
> +framework the devicetree has the clock in Hz instead of ps.
As Rob mentioned, I think there shouldn't be any mention of Linux FB here.
> +
> +Example:
> +
> + display at 0 {
This node appears to describe a video mode, not a display, hence the
node name seems wrong.
Many displays can support multiple different video modes. As mentioned
elsewhere, properties like display width/height are properties of the
display not the mode.
So, I might expect something more like the following (various overhead
properties like reg/#address-cells etc. elided for simplicity):
disp: display {
width-mm = <...>;
height-mm = <...>;
modes {
mode at 0 {
/* 1920x1080p24 */
clock = <5200>;
xres = <1920>;
yres = <1080>;
left-margin = <25>;
right-margin = <25>;
hsync-len = <25>;
lower-margin = <2>;
upper-margin = <2>;
vsync-len = <2>;
hsync-active-high;
};
mode at 1 {
};
};
};
display-connector {
display = <>;
// interface-specific properties such as pixel format here
};
Finally, have you considered just using an EDID instead of creating
something custom? I know that creating an EDID is harder than writing a
few simple properties into a DT, but EDIDs have the following advantages:
a) They're already standardized and very common.
b) They allow other information such as a display's HDMI audio
capabilities to be represented, which can then feed into an ALSA driver.
c) The few LCD panel datasheets I've seen actually quote their
specification as an EDID already, so deriving the EDID may actually be easy.
d) People familiar with displays are almost certainly familiar with
EDID's mode representation. There are many ways of representing display
modes (sync position vs. porch widths, htotal specified rather than
specifying all the components and hence htotal being calculated etc.).
Not everyone will be familiar with all representations. Conversion
errors are less likely if the target is EDID's familiar format.
e) You'll end up with exactly the same data as if you have a DDC-based
external monitor rather than an internal panel, so you end up getting to
a common path in display handling code much more quickly.
[PATCH v2] of: Add videomode helper
On 07/05/2012 08:51 AM, Rob Herring wrote:
> On 07/04/2012 02:56 AM, Sascha Hauer wrote:
>> This patch adds a helper function for parsing videomodes from the devicetree.
>> The videomode can be either converted to a struct drm_display_mode or a
>> struct fb_videomode.
>> diff --git a/Documentation/devicetree/bindings/video/displaymode
>> b/Documentation/devicetree/bindings/video/displaymode
>> +Example:
>> +
>> + display at 0 {
>> + /* 1920x1080p24 */
>> + clock = <5200>;
>
> Should this use the clock binding? You probably need both constraints
> and clock binding though.
I don't think the clock binding is appropriate here. This binding
specifies a desired video mode, and should specify just the expected
clock frequency for that mode. Perhaps any tolerance imposed by the
specification used to calculate the mode timing could be specified too,
but the allowed tolerance (for a mode to be recognized by the dispaly)
is more driven by the receiving device than the transmitting device.
The clock bindings should come into play in the display controller that
sends a signal in that display mode. Something like:
mode: hd1080p {
clock = <5200>;
xres = <1920>;
yres = <1080>;
};
display-controller {
pixel-clock-source = < ...>; // common clock bindings here
default-mode = <>;
> Often you don't know the frequency up front and/or have limited control
> of the frequency (i.e. integer dividers). Then you have to adjust the
> margins to get the desired refresh rate. To do that, you need to know
> the ranges of values a panel can support. Perhaps you just assume you
> can increase the right-margin and lower-margins as I think you will hit
> pixel clock frequency max before any limit on margins.
I think this is more usually dealt with in HW design and matching
components.
The PLL/... feeding the display controller is going to have some known
specifications that imply which pixel clocks it can generate. HW
designers will pick a panel that accepts a clock the display controller
can generate. The driver for the display controller will just generate
as near of a pixel clock as it can to the rate specified in the mode
definition. I believe it'd be unusual for a display driver to start
fiddling with front-back porch (or margin) widths to munge the timing;
that kind of thing probably worked OK with analog displays, but with
digital displays where each pixel is clocked even in the margins, I
think that would cause more problems than it solved.
Similarly for external displays, the display controller will just pick
the nearest clock it can. If it can't generate a close enough clock, it
will just refuse to set the mode. In fact, a display controller driver
would typically validate this when the set of legal modes was enumerated
when initially detecting the display, so user-space typically wouldn't
even request invalid modes.
[PATCH v2] of: Add videomode helper
On 08/03/2012 01:38 AM, Sascha Hauer wrote:
> Hi Stephen,
>
> On Thu, Aug 02, 2012 at 01:35:40PM -0600, Stephen Warren wrote:
>> On 07/04/2012 01:56 AM, Sascha Hauer wrote:
>>> This patch adds a helper function for parsing videomodes from the
>>> devicetree.
>>> The videomode can be either converted to a struct drm_display_mode or a
>>> struct fb_videomode.
>>
>>> diff --git a/Documentation/devicetree/bindings/video/displaymode
>>> b/Documentation/devicetree/bindings/video/displaymode
>>
>>> +Required properties:
>>> + - xres, yres: Display resolution
>>> + - left-margin, right-margin, hsync-len: Horizontal Display timing
>>> parameters
>>> + in pixels
>>> + upper-margin, lower-margin, vsync-len: Vertical display timing
>>> parameters in
>>> + lines
>>
>> Perhaps bike-shedding, but...
>>
>> For the margin property names, wouldn't it be better to use terminology
>> more commonly used for video timings rather than Linux FB naming. In
>> other words naming like:
>>
>> hactive, hsync-len, hfront-porch, hback-porch?
>
> Can do. Just to make sure:
>
> hactive == xres
> hsync-len == hsync-len
> hfront-porch == right-margin
> hback-porch == left-margin
I believe so yes.
>> a) They're already standardized and very common.
>
> Indeed, that's a big plus for EDID. I have no intention of removing EDID
> data from the devicetree. There are situations where EDID is handy, for
> example when you get EDID data provided by your vendor.
>
>>
>> b) They allow other information such as a display's HDMI audio
>> capabilities to be represented, which can then feed into an ALSA driver.
>>
>> c) The few LCD panel datasheets I've seen actually quote their
>> specification as an EDID already, so deriving the EDID may actually be easy.
>>
>> d) People familiar with displays are almost certainly familiar with
>> EDID's mode representation. There are many ways of representing display
>> modes (sync position vs. porch widths, htotal specified rather than
>> specifying all the components and hence htotal being calculated etc.).
>> Not everyone will be familiar with all representations. Conversion
>> errors are less likely if the target is EDID's familiar format.
>
> You seem to think about a different class of displays for which EDID
> indeed is a better way to handle. What I have to deal with here mostly
> are dumb displays which:
>
> - can only handle their native resolution
> - Have no audio capabilities at all
> - come with a datasheet which specify a min/typ/max triplet for
> xres,hsync,..., often enough they are scanned to pdf from some previously
> printed paper.
>
> These displays are very common on embedded devices, probably that's the
> reason I did not even think about the possibility that a single display
> might have different modes.
That's true, but as I mentioned, there are at least some dumb panels
(both I've seen recently) whose specification provides the EDID. I don't
know how common that is though, I must admit.
>> e) You'll end up with exactly the same data as if you have a DDC-based
>> external monitor rather than an internal panel, so you end up getting to
>> a common path in display handling code much more quickly.
>
> All we have in our display driver currently is:
>
> edidp = of_get_property(np, "edid", >edid_len);
> if (edidp) {
> imxpd->edid = kmemdup(edidp, imxpd->edid_len, GFP_KERNEL);
> } else {
> ret = of_get_video_mode(np, >mode, NULL);
> if (!ret)
> imxpd->mode_valid = 1;
> }
Presumably there's more to it though; something later checks
imxpd->mode_valid and if false, parses the EDID and sets up imxpd->mode,
etc.
Tegra DRM device tree bindings
On 07/05/2012 06:15 AM, Thierry Reding wrote: > Here's a new proposal that should address all issues collected > during the discussion of the previous one: > > From tegra20.dtsi: ... At a quick glance, that all seems pretty reasonable now. > One problem I've come across when trying to get some rudimentary > code working with this is that there's no longer a device which the > DRM driver can bind to, because the top-level device (host1x) now > has a separate driver. Can't you just have the host1x driver trigger the instantiation of the DRM driver? In other words, the host1x node is both the plain host1x driver and the DRM driver. So, after initializing anything for host1x itself, just end host1x's probe() with something a call to drm_platform_init(), passing in host1x's own pdev. After all, it seems like the whole point of representing host1x in the DT is to expose the graphics HW, so you'd need the DRM device in that case.
[alsa-devel] [PATCH v3] pass ELD to HDMI/DP audio driver
Wu Fengguang wrote at Friday, August 05, 2011 6:50 AM: > On Fri, Aug 05, 2011 at 02:03:41AM +0800, Keith Packard wrote: > > On Thu, 4 Aug 2011 17:40:24 +0800, Wu Fengguang > > wrote: ... > > > You may wonder why the mode parameter is needed in intel_write_eld(). > > > This is because the ELD field aud_synch_delay (ie. A/V sync delay) may > > > have different values in progressive/interleaved display modes. > > > > Ok, so you can't write ELD data until the display is active, which > > happens at mode_set time. > > > > Do you need to provide ELD when the display is inactive? Is this only to > > enable audio output when the display is not on? In that case, we will > > Good questions! In general the audio functionalities should not > depend on the display activeness. There are even audio-only HDMI > devices. So I'll need to make intel_write_eld() work even without > information about the current display mode. Is there such a thing as an audio-only HDMI signal though; the audio data is transferred inside the blanking region of the HDMI video signal, which kinda implies that there needs to be some video content (even if the pixel data is e.g. hard-coded black rather than being scanned out from a frame- buffer) so as to define where those blanking regions are within the signal. -- nvpublic
