from:"Daniel Drake"

Re: [PATCH v2 00/91] drm/vc4: Support BCM2711 Display Pipelin

2020-05-27 Thread Daniel Drake

On Wed, May 27, 2020 at 5:13 PM Maxime Ripard  wrote:
> I'm about to send a v3 today or tomorrow, I can Cc you (and Jian-Hong) if you
> want.

That would be great, although given the potentially inconsistent
results we've been seeing so far it would be great if you could
additionally push a git branch somewhere.
That way we can have higher confidence that we are applying exactly
the same patches to the same base etc.

Thanks
Daniel
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 00/91] drm/vc4: Support BCM2711 Display Pipelin

2020-05-26 Thread Daniel Drake

Hi Maxime,

On Tue, May 26, 2020 at 6:20 PM Maxime Ripard  wrote:
> I gave it a try with U-Boot with my latest work and couldn't reproduce it, so 
> it
> seems that I fixed it along the way

Is your latest work available in a git branch anywhere that we could
test directly?

Thanks
Daniel
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] i915: disable framebuffer compression on GeminiLake

2019-05-09 Thread Daniel Drake

Hi,


On Thu, Apr 25, 2019 at 4:27 AM Paulo Zanoni  wrote:
>
> Em qua, 2019-04-24 às 20:58 +0100, Chris Wilson escreveu:
> > Quoting Jian-Hong Pan (2019-04-23 10:28:10)
> > > From: Daniel Drake 
> > >
> > > On many (all?) the Gemini Lake systems we work with, there is frequent
> > > momentary graphical corruption at the top of the screen, and it seems
> > > that disabling framebuffer compression can avoid this.
> > >
> > > The ticket was reported 6 months ago and has already affected a
> > > multitude of users, without any real progress being made. So, lets
> > > disable framebuffer compression on GeminiLake until a solution is found.
> > >
> > > Buglink: https://bugs.freedesktop.org/show_bug.cgi?id=108085
> > > Signed-off-by: Daniel Drake 
> > > Signed-off-by: Jian-Hong Pan 
> >
> > Fixes: fd7d6c5c8f3e ("drm/i915: enable FBC on gen9+ too") ?
> > Cc: Paulo Zanoni 
> > Cc: Daniel Vetter 
> > Cc: Jani Nikula 
> > Cc:  # v4.11+
> >
> > glk landed 1 month before, so that seems the earliest broken point.
> >
>
> The bug is well reported, the bug author is helpful and it even has a
> description of "steps to reproduce" that looks very easy (although I
> didn't try it). Everything suggests this is a bug the display team
> could actually solve with not-so-many hours of debugging.
>
> In the meantime, unbreak the systems:
> Reviewed-by: Paulo Zanoni 

Quick ping here. Any further comments on this patch? Can it be applied?

Thanks
Daniel
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

freedreno header uses not installed xf86atomic.h

2019-02-15 Thread Daniel Drake

Hi,

Using libdrm-2.4.97, mesa fails to build on ARM with:

[  456s] In file included from
../../../../../src/gallium/drivers/freedreno/freedreno_util.h:33,
[  456s]  from
../../../../../src/gallium/drivers/freedreno/freedreno_batch.h:34,
[  456s]  from
../../../../../src/gallium/drivers/freedreno/freedreno_context.h:39,
[  456s]  from
../../../../../src/gallium/drivers/freedreno/freedreno_program.c:33:
[  456s] /usr/include/freedreno/freedreno_ringbuffer.h:32:10: fatal
error: xf86atomic.h: No such file or directory

The freedreno headers were recently modified to use xf86atomic.h:
https://gitlab.freedesktop.org/mesa/drm/commit/b541d21a0a908bf98d44375720f4430297720743

However xf86atomic.h is not within the set of files installed by
libdrm. Should it be?

Thanks
Daniel
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: Gemini Lake graphics corruption at top of screen

2018-10-14 Thread Daniel Drake

Hi,

On Mon, Oct 8, 2018 at 1:48 PM Daniel Drake  wrote:
> I recently filed a bug report regarding graphics corruption seen on
> Gemini Lake platforms:
> https://bugs.freedesktop.org/show_bug.cgi?id=108085
>
> This has been reproduced on multiple distros on products from at least
> 4 vendors. It seems to apply to every GeminiLake product that we have
> seen.
>
> The graphics corruption is quite promiment when using these platforms
> for daily use.

Ping... how can we help diagnose this issue?

If you provide a shipping address we can send a sample to Intel, with
the issue easily reproducible and ready-to-go.

Thanks
Daniel
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Gemini Lake graphics corruption at top of screen

2018-10-07 Thread Daniel Drake

Hi,

I recently filed a bug report regarding graphics corruption seen on
Gemini Lake platforms:
https://bugs.freedesktop.org/show_bug.cgi?id=108085

This has been reproduced on multiple distros on products from at least
4 vendors. It seems to apply to every GeminiLake product that we have
seen.

The graphics corruption is quite promiment when using these platforms
for daily use. I would love to see more attention here, how can we
help advance the diagnosis of this issue?

Thanks
Daniel
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: amdgpu hangs on boot or shutdown on AMD Raven Ridge CPU (Engineer Sample)

2018-05-08 Thread Daniel Drake

WHi Alex,

On Thu, Apr 19, 2018 at 4:13 PM, Alex Deucher  wrote:
 https://bugs.freedesktop.org/show_bug.cgi?id=105684
>>>
>>> No progress made on that bug report so far.
>>> What can we do to help this advance?
>>
>> Ping, any news here? How can we help advance on this bug?
>
> Can you try one of these branches?
> https://cgit.freedesktop.org/~agd5f/linux/log/?h=amd-staging-drm-next
> https://cgit.freedesktop.org/~agd5f/linux/log/?h=drm-next-4.18-wip
> do they work any better?

It's been over 3 months since we reported this bug by email, over 6
weeks since we reported it on bugzilla, and still there has been no
meaningful diagnostics help from AMD. This follows a similar pattern
to what we have seen with other issues prior to this one.

What can we do so that this bug gets some attention from your team?

Secondarily https://bugs.freedesktop.org/show_bug.cgi?id=106228 is
another bug that needs attention. We have a growing number of consumer
platforms affected by this. When booted, the amdgpu screen brightness
value is incorrectly read back as 0, which systemd will then store on
shutdown. On next boot, it restores the very low brightness level.
This can reproduce out of the box on Fedora, Ubuntu, etc.

Thanks,
Daniel
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: amdgpu hangs on boot or shutdown on AMD Raven Ridge CPU (Engineer Sample)

2018-04-03 Thread Daniel Drake

On Thu, Mar 22, 2018 at 3:09 AM, Daniel Drake <dr...@endlessm.com> wrote:
> On Tue, Feb 20, 2018 at 10:18 PM, Alex Deucher <alexdeuc...@gmail.com> wrote:
>>> It seems that we are not alone seeing amdgpu-induced stability
>>> problems on multiple Raven Ridge platforms.
>>> https://www.phoronix.com/scan.php?page=news_item=AMD-Raven-Ridge-Mobo-Linux
>>>
>>> AMD, what can we do to help?
>>
>> Please file bugs:
>> https://bugs.freedesktop.org
>
> Sorry for the delayed response. We're still seeing serious instability
> here even on the latest kernel. Filed
> https://bugs.freedesktop.org/show_bug.cgi?id=105684

No progress made on that bug report so far.
What can we do to help this advance?

Thanks,
Daniel
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: amdgpu hangs on boot or shutdown on AMD Raven Ridge CPU (Engineer Sample)

2018-03-22 Thread Daniel Drake

Hi Alex,

On Tue, Feb 20, 2018 at 10:18 PM, Alex Deucher  wrote:
>> It seems that we are not alone seeing amdgpu-induced stability
>> problems on multiple Raven Ridge platforms.
>> https://www.phoronix.com/scan.php?page=news_item=AMD-Raven-Ridge-Mobo-Linux
>>
>> AMD, what can we do to help?
>
> Please file bugs:
> https://bugs.freedesktop.org

Sorry for the delayed response. We're still seeing serious instability
here even on the latest kernel. Filed
https://bugs.freedesktop.org/show_bug.cgi?id=105684

Thanks,
Daniel
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: amdgpu hangs on boot or shutdown on AMD Raven Ridge CPU (Engineer Sample)

2018-02-19 Thread Daniel Drake

Hi,

> >>> We are working with new laptops that have the AMD Ravenl Ridge
> >>> chipset with this `/proc/cpuinfo`
> >>> https://gist.github.com/mschiu77/b06dba574e89b9a30cf4c450eaec49bc
> >>>
> >>> With the latest kernel 4.15, there're lots of different
> >>> panics/oops during boot so no chance to get into X. It also happens
> >>> during shutdown. Then I tried to build kernel from
> >>> git://people.freedesktop.org/~agd5f/linux on branch
> >>> amd-staging-drm-next with head on commit "drm: Fix trailing semicolon"
> >>> and update the linux-firmware. Things seem to get better, only 1 oops
> >>> observed. Here's the oops
> >>> https://gist.github.com/mschiu77/1a68f27272b24775b2040acdb474cdd3.

It seems that we are not alone seeing amdgpu-induced stability
problems on multiple Raven Ridge platforms.
https://www.phoronix.com/scan.php?page=news_item=AMD-Raven-Ridge-Mobo-Linux

AMD, what can we do to help?

Thanks!
Daniel
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 4.15] drm/amd/display: call set csc_default if enable adjustment is false

2017-12-29 Thread Daniel Drake

From: Yue Hin Lau <yuehin@amd.com>

Signed-off-by: Yue Hin Lau <yuehin@amd.com>
Reviewed-by: Eric Bernstein <eric.bernst...@amd.com>
Acked-by: Harry Wentland <harry.wentl...@amd.com>
Signed-off-by: Alex Deucher <alexander.deuc...@amd.com>
[dr...@endlessm.com: backport to 4.15]
Signed-off-by: Daniel Drake <dr...@endlessm.com>
---
 drivers/gpu/drm/amd/display/dc/dcn10/dcn10_dpp.h  | 2 +-
 drivers/gpu/drm/amd/display/dc/dcn10/dcn10_dpp_cm.c   | 6 ++
 drivers/gpu/drm/amd/display/dc/dcn10/dcn10_hw_sequencer.c | 2 ++
 drivers/gpu/drm/amd/display/dc/inc/hw/dpp.h   | 2 +-
 4 files changed, 6 insertions(+), 6 deletions(-)

Testing Acer Aspire TC-380 engineering sample (Raven Ridge), the display
comes up with an excessively green tint. This patch (from amd-staging-drm-next)
solves the issue. Can it be included in Linux 4.15?

diff --git a/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_dpp.h 
b/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_dpp.h
index a9782b1aba47..34daf895f848 100644
--- a/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_dpp.h
+++ b/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_dpp.h
@@ -1360,7 +1360,7 @@ void dpp1_cm_set_output_csc_adjustment(
 
 void dpp1_cm_set_output_csc_default(
struct dpp *dpp_base,
-   const struct default_adjustment *default_adjust);
+   enum dc_color_space colorspace);
 
 void dpp1_cm_set_gamut_remap(
struct dpp *dpp,
diff --git a/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_dpp_cm.c 
b/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_dpp_cm.c
index 40627c244bf5..ed1216b53465 100644
--- a/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_dpp_cm.c
+++ b/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_dpp_cm.c
@@ -225,14 +225,13 @@ void dpp1_cm_set_gamut_remap(
 
 void dpp1_cm_set_output_csc_default(
struct dpp *dpp_base,
-   const struct default_adjustment *default_adjust)
+   enum dc_color_space colorspace)
 {
 
struct dcn10_dpp *dpp = TO_DCN10_DPP(dpp_base);
uint32_t ocsc_mode = 0;
 
-   if (default_adjust != NULL) {
-   switch (default_adjust->out_color_space) {
+   switch (colorspace) {
case COLOR_SPACE_SRGB:
case COLOR_SPACE_2020_RGB_FULLRANGE:
ocsc_mode = 0;
@@ -253,7 +252,6 @@ void dpp1_cm_set_output_csc_default(
case COLOR_SPACE_UNKNOWN:
default:
break;
-   }
}
 
REG_SET(CM_OCSC_CONTROL, 0, CM_OCSC_MODE, ocsc_mode);
diff --git a/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_hw_sequencer.c 
b/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_hw_sequencer.c
index 961ad5c3b454..05dc01e54531 100644
--- a/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_hw_sequencer.c
+++ b/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_hw_sequencer.c
@@ -2097,6 +2097,8 @@ static void program_csc_matrix(struct pipe_ctx *pipe_ctx,
tbl_entry.color_space = color_space;
//tbl_entry.regval = matrix;

pipe_ctx->plane_res.dpp->funcs->opp_set_csc_adjustment(pipe_ctx->plane_res.dpp, 
_entry);
+   } else {
+   
pipe_ctx->plane_res.dpp->funcs->opp_set_csc_default(pipe_ctx->plane_res.dpp, 
colorspace);
}
 }
 static bool is_lower_pipe_tree_visible(struct pipe_ctx *pipe_ctx)
diff --git a/drivers/gpu/drm/amd/display/dc/inc/hw/dpp.h 
b/drivers/gpu/drm/amd/display/dc/inc/hw/dpp.h
index 83a68460edcd..9420dfb94d39 100644
--- a/drivers/gpu/drm/amd/display/dc/inc/hw/dpp.h
+++ b/drivers/gpu/drm/amd/display/dc/inc/hw/dpp.h
@@ -64,7 +64,7 @@ struct dpp_funcs {
 
void (*opp_set_csc_default)(
struct dpp *dpp,
-   const struct default_adjustment *default_adjust);
+   enum dc_color_space colorspace);
 
void (*opp_set_csc_adjustment)(
struct dpp *dpp,
-- 
2.14.1

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

AMD DC test results

2017-12-21 Thread Daniel Drake

Hi,

Thanks for the hard work on AMD DC development! Here are some new test
results - hope they are interesting/useful.

CONTEXT

We have been tracking DC for a while as we work with multiple products
where amdgpu previously was not able to support the HDMI audio output.
We are hoping to ship the new driver as soon as possible to fix this
Linux compatibility issue.

TEST SETUP

With an eye to shipping this ASAP, we backported DC to the current
stable release, Linux 4.14. To do this we took the amdgpu/DC commits
from:
https://cgit.freedesktop.org/~agd5f/linux/log/?h=upstream-4.14-drm-next-amd-dc-staging-chrome

And added these two pull requests:
https://www.spinics.net/lists/dri-devel/msg156828.html
https://lists.freedesktop.org/archives/dri-devel/2017-November/157687.html

At this point we diff the resultant amdgpu directories with Linus tree
as of commit f6705bf959efac87bca76d40050d342f1d212587 (when he
accepted the DC driver) and the diff is zero. This is published on the
master branch of https://github.com/endlessm/linux

There have been more AMD DC changes merged into Linus tree since then,
however we tried and the backporting seems like loads of work, so we
have not gone beyond the initial merge into Linus tree for now.

We then asked our QA team to run our standard tests with this kernel.
They tested 3 platforms:
Acer Aspire E5-553G (AMD FX-9800P RADEON R7)
Acer Aspire E5-523G (AMD E2-9010 RADEON R2)
Acer Aspire A315-21 (AMD A4-9120 RADEON R3)

CONFIG_DRM_AMD_DC_PRE_VEGA was set in order to have AMD DC support
this hardware.

TEST RESULTS

HDMI audio works now, however we have encountered some regressions. In
all cases the 3 platforms behaved the same (either all 3 pass the
individual tests, or all 3 show the same problem). The regressions
are:

1. System hangs on S3 resume.
Bug can't be reproduced when booting with amdgpu.dc=0
Also reproduced with DC on Linus master (4.15-rc), but appears fixed
on agd5f/amd-staging-drm-next
https://bugs.freedesktop.org/show_bug.cgi?id=104281

2. Display corruption when using multiple displays
Bug can't be reproduced when booting with amdgpu.dc=0
Also reproduced with DC on Linus master (4.15-rc), but appears fixed
on agd5f/amd-staging-drm-next
https://bugs.freedesktop.org/show_bug.cgi?id=104319

3. HDMI audio device still shown as present and active even after
disconnecting HDMI cable
Bug can't be reproduced when booting with amdgpu.dc=0
Appears fixed with DC on Linus master (4.15-rc).

Full test results spreadsheet (including tests passed): http://goo.gl/oqnMDx
Thanks to my colleagues: Carlo Caione for doing the backport and
Vanessa Chang for managing the testing.

NEXT STEPS

Unfortunately we can't ship the driver in current state with these
regressions, but on the positive side it looks like it will be in
better shape for Linux 4.16.

We will now focus on making 4.15 regression-free before revisiting the
idea of backporting DC to earlier kernels.

After the holiday break we will try to identify the relevant fixes in
amd-staging-drm-next and see if they can be included in Linux 4.15.
We'll send these backports upstream as soon as we find them.

Daniel
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: Kernel panic on nouveau during boot on NVIDIA NV118 (GM108)

2017-06-02 Thread Daniel Drake

On Fri, Jun 2, 2017 at 4:01 AM, Chris Chiu  wrote:
> We are working with new desktop that have the NVIDIA NV118
> chipset.
>
> During boot, the display becomes unusable at the point where the
> nouveau driver loads. We have reproduced on 4.8, 4.11 and linux
> master (4.12-rc3).

To save digging into the attachment, here is the crash:

nouveau :01:00.0: pci: failed to adjust cap speed
nouveau :01:00.0: pci: failed to adjust lnkctl speed
nouveau :01:00.0: fb: 0 MiB DDR3
divide error:  [#1] SMP
Modules linked in: hid_generic usbmouse usbkbd usbhid i915 nouveau(+)
mxm_wmi i2c_algo_bit drm_kms_helper sdhci_pci syscopyarea sysfillrect
sysimgblt fb_sys_fops ttm sdhci drm ahci libahci wmi i2c_hid hid video
CPU: 3 PID: 200 Comm: systemd-udevd Not tainted 4.11.0-2-generic
#7+dev143.9f9ecd2beos3.2.2-Endless
Hardware name: Acer Aspire Z20-730/IPMAL-BR3, BIOS D01 07/07/2016
task: 8a3070288000 task.stack: a3eb4103c000
RIP: 0010:gf100_ltc_oneinit_tag_ram+0xba/0x100 [nouveau]
RSP: 0018:a3eb4103f6b8 EFLAGS: 00010206
RAX: 1000ffefdfff RBX: 8a306f915000 RCX: 8a3075570030
RDX:  RSI: dead0200 RDI: 8a307fd9b700
RBP: a3eb4103f6d0 R08: 0001e980 R09: 8a3077003900
R10: a3eb40cdbda0 R11: 8a307fd986a4 R12: 
R13: 00015fff R14: 8a306fa2e400 R15: 8a306f914e00
FS:  7f456d052900() GS:8a307fd8() knlGS:
CS:  0010 DS:  ES:  CR0: 80050033
CR2: 7fefceb1c020 CR3: 00026fbc5000 CR4: 003406e0
Call Trace:
 gm107_ltc_oneinit+0x7c/0x90 [nouveau]
 nvkm_ltc_oneinit+0x13/0x20 [nouveau]
 nvkm_subdev_init+0x50/0x210 [nouveau]
 nvkm_device_init+0x151/0x270 [nouveau]
 nvkm_udevice_init+0x48/0x60 [nouveau]
 nvkm_object_init+0x40/0x190 [nouveau]
 nvkm_ioctl_new+0x179/0x290 [nouveau]
 ? nvkm_client_notify+0x30/0x30 [nouveau]
 ? nvkm_udevice_rd08+0x30/0x30 [nouveau]
 nvkm_ioctl+0x168/0x240 [nouveau]
 ? nvif_client_init+0x42/0x110 [nouveau]
 nvkm_client_ioctl+0x12/0x20 [nouveau]
 nvif_object_ioctl+0x42/0x50 [nouveau]
 nvif_object_init+0xc2/0x130 [nouveau]
 nvif_device_init+0x12/0x30 [nouveau]
 nouveau_cli_init+0x15e/0x1d0 [nouveau]
 nouveau_drm_load+0x67/0x8b0 [nouveau]
 ? sysfs_do_create_link_sd.isra.2+0x70/0xb0
 drm_dev_register+0x148/0x1e0 [drm]
 drm_get_pci_dev+0xa0/0x160 [drm]
 nouveau_drm_probe+0x1d9/0x260 [nouveau]

Daniel
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: i915 4.9 regression: DP AUX CH sanitization no longer working on Asus desktops

2017-05-24 Thread Daniel Drake

On Fri, May 5, 2017 at 4:29 AM, Ville Syrjälä
<ville.syrj...@linux.intel.com> wrote:
> On Thu, May 04, 2017 at 02:52:09PM -0600, Daniel Drake wrote:
>> On Thu, May 4, 2017 at 2:37 PM, Ville Syrjälä
>> <ville.syrj...@linux.intel.com> wrote:
>> > Please check if commit bb1d132935c2 ("drm/i915/vbt: split out defaults
>> > that are set when there is no VBT") fixes things for you.
>>
>> I think this is not going to help. This would only make a difference
>> when there is no VBT at all at which point we would see this message
>> in the logs:
>>
>> DRM_INFO("Failed to find VBIOS tables (VBT)\n");
>
> No, the patch will in fact make a difference only when there is a VBT.

We confirmed the mentioned patch fixes the issue. Apologies for doubting you :)

Thanks!
Daniel
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

amdgpu display corruption and hang on AMD A10-9620P

2017-05-09 Thread Daniel Drake

Hi,

We are working with new laptops that have the AMD Bristol Ridge
chipset with this SoC:

AMD A10-9620P RADEON R5, 10 COMPUTE CORES 4C+6G

I think this is the Bristol Ridge chipset.

During boot, the display becomes unusable at the point where the
amdgpu driver loads. You can see at least two horizontal lines of
garbage at this point. We have reproduced on 4.8, 4.10 and linus
master (early 4.12).

Photo: http://pasteboard.co/qrC9mh4p.jpg

Getting logs is tricky because the system appears to freeze at that point.

Is this a known issue? Anything we can do to help diagnosis?

Thanks
Daniel
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: i915 4.9 regression: DP AUX CH sanitization no longer working on Asus desktops

2017-05-04 Thread Daniel Drake

On Thu, May 4, 2017 at 2:37 PM, Ville Syrjälä
 wrote:
> Please check if commit bb1d132935c2 ("drm/i915/vbt: split out defaults
> that are set when there is no VBT") fixes things for you.

I think this is not going to help. This would only make a difference
when there is no VBT at all at which point we would see this message
in the logs:

DRM_INFO("Failed to find VBIOS tables (VBT)\n");

but in this case we have a VBT for ports B, C and E.

 [drm:intel_bios_init [i915]] Port B VBT info: DP:1 HDMI:1 DVI:1 EDP:0 CRT:0
 [drm:intel_bios_init [i915]] VBT HDMI level shift for port B: 8
 [drm:intel_bios_init [i915]] Port C VBT info: DP:0 HDMI:1 DVI:1 EDP:0 CRT:0
 [drm:intel_bios_init [i915]] VBT HDMI level shift for port C: 8
 [drm:intel_bios_init [i915]] Port E VBT info: DP:1 HDMI:0 DVI:0 EDP:0 CRT:0
 [drm:intel_bios_init [i915]] VBT HDMI level shift for port E: 0

Let me know if I'm missing something and we will test it anyway

Thanks
Daniel
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

i915 4.9 regression: DP AUX CH sanitization no longer working on Asus desktops

2017-05-04 Thread Daniel Drake

Hi,

Numerous Asus desktops and All-in-one computers (e.g. D520MT) have a
regression on Linux 4.9 where the VGA output is shown all-white.

This is a regression caused by:

commit 0ce140d45a8398b501934ac289aef0eb7f47c596
Author: Ville Syrjälä 
Date:   Tue Oct 11 20:52:47 2016 +0300

drm/i915: Clean up DDI DDC/AUX CH sanitation


On these platforms, the VGA output is detected as DP (presumably
theres a DP-to-VGA converter on the motherboard). The sanitization
done by the code that was removed here was correctly realising that
port E's DP aux channel was DP_AUX_A, so it disabled DP output on port
A, also showing this message:

   [drm:intel_ddi_init] VBT says port A is not DVI/HDMI/DP compatible,
respect it

But after this cleanup commit, both port A and port E are activated
and the screen shows all-white. Reverting the commit restores usable
VGA display output.

The reason the new implementation doesn't catch the duplicate
configuration is because the new code only considers ports that are
present in the VBT where parse_ddi_port() has run on them (in order to
set that port's info->alternate_aux_channel).

In this case, port A is not present in the VBT so it will not have
info->alternate_aux_channel set, and the new sanitize_aux_ch will run
on port E but will not consider any overlap with port A.

debug logs from an affected kernel:
https://gist.github.com/dsd/7e56c9bca7b2345b678cfacdab30ec55

Should we modify sanitize_aux_ch to look at all aux channels, not only
for the ports specified in the VBT?

Thanks
Daniel
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/nouveau/core: recognise GP107 chipset

2017-02-14 Thread Daniel Drake

From: Chris Chiu <c...@endlessm.com>

This new graphics card was failing to initialize with nouveau due to
an "unknown chipset" error.

Copy the GP106 configuration and rename for GP107/NV137. We don't
know for certain that this is fully correct, but brief desktop testing
suggests this is working fine.

Signed-off-by: Chris Chiu <c...@endlessm.com>
Signed-off-by: Daniel Drake <dr...@endlessm.com>
---
 drivers/gpu/drm/nouveau/nvkm/engine/device/base.c | 29 +++
 1 file changed, 29 insertions(+)

diff --git a/drivers/gpu/drm/nouveau/nvkm/engine/device/base.c 
b/drivers/gpu/drm/nouveau/nvkm/engine/device/base.c
index fea30d6..d242431 100644
--- a/drivers/gpu/drm/nouveau/nvkm/engine/device/base.c
+++ b/drivers/gpu/drm/nouveau/nvkm/engine/device/base.c
@@ -2237,6 +2237,34 @@ nv136_chipset = {
.fifo = gp100_fifo_new,
 };
 
+static const struct nvkm_device_chip
+nv137_chipset = {
+   .name = "GP107",
+   .bar = gf100_bar_new,
+   .bios = nvkm_bios_new,
+   .bus = gf100_bus_new,
+   .devinit = gm200_devinit_new,
+   .fb = gp104_fb_new,
+   .fuse = gm107_fuse_new,
+   .gpio = gk104_gpio_new,
+   .i2c = gm200_i2c_new,
+   .ibus = gm200_ibus_new,
+   .imem = nv50_instmem_new,
+   .ltc = gp100_ltc_new,
+   .mc = gp100_mc_new,
+   .mmu = gf100_mmu_new,
+   .pci = gp100_pci_new,
+   .timer = gk20a_timer_new,
+   .top = gk104_top_new,
+   .ce[0] = gp104_ce_new,
+   .ce[1] = gp104_ce_new,
+   .ce[2] = gp104_ce_new,
+   .ce[3] = gp104_ce_new,
+   .disp = gp104_disp_new,
+   .dma = gf119_dma_new,
+   .fifo = gp100_fifo_new,
+};
+
 static int
 nvkm_device_event_ctor(struct nvkm_object *object, void *data, u32 size,
   struct nvkm_notify *notify)
@@ -2673,6 +2701,7 @@ nvkm_device_ctor(const struct nvkm_device_func *func,
case 0x130: device->chip = _chipset; break;
case 0x134: device->chip = _chipset; break;
case 0x136: device->chip = _chipset; break;
+   case 0x137: device->chip = _chipset; break;
default:
nvdev_error(device, "unknown chipset (%08x)\n", boot0);
goto done;
-- 
2.9.3

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2] drm/exynos: switch to universal plane API

2014-09-19 Thread Daniel Drake

On Fri, Sep 19, 2014 at 6:58 AM, Andrzej Hajda  wrote:
> The patch replaces legacy functions
> drm_plane_init() / drm_crtc_init() with
> drm_universal_plane_init() and drm_crtc_init_with_planes().
> It allows to replace fake primary plane with the real one.
> Additionally the patch leaves cleanup of crtcs to core,
> this way planes and crtcs are cleaned in correct order.
>
> Signed-off-by: Andrzej Hajda 

Thanks Andrzej! With your patch the crashes I was seeing are gone.

Daniel

[PATCH] drm/exynos: fix plane-framebuffer linkage

2014-09-18 Thread Daniel Drake

On Thu, Sep 18, 2014 at 12:39 AM, Daniel Vetter  wrote:
> On Wed, Sep 17, 2014 at 6:41 PM, Daniel Drake  wrote:
>> 2. drm_mode_rmfb then calls drm_framebuffer_remove, which calls
>> drm_mode_set_config_internal() in order to turn off the CRTC, dropping
>> another reference in the process.
>> if (tmp->old_fb)
>> drm_framebuffer_unreference(tmp->old_fb);
>>
>> 3. drm_framebuffer_remove calls drm_plane_force_disable() which drops
>> another reference:
>> /* disconnect the plane from the fb and crtc: */
>> __drm_framebuffer_unreference(old_fb);
>
> If 3. here is about the primary plane then this won't happen, since
> the primary plane pointer has already been cleared in step
> 2.

I just checked - as Joonyoung suspects, the plane being force disabled
in step 3 is the private exynos-drm plane. So thats an issue - but at
least now I have a complete understanding of the problem.

Sounds like that will also be fixed by moving to universal planes.
I'll wait for Andrzej's patch.

Thanks!
Daniel

[PATCH] drm/exynos: fix plane-framebuffer linkage

2014-09-17 Thread Daniel Drake

On Wed, Sep 17, 2014 at 7:45 AM, Daniel Vetter  wrote:
> I think fb refcounting in exynos is just plain busted. If you look at
> other drivers the only place the refcount framebuffers or backing
> storage objects is for pageflips to make sure the memory doesn't go
> away while the hw is still scanning out the old framebuffer. If you
> refcount anywhere else you either do something really crazy or your
> driver is broken.

With my patch actually the behaviour is much more similar to omapdrm,
which also doesn't quite match your description of "other drivers".
See omap_plane.c.

There is a fb reference taken for "pinning" in update_pin() which
presumably is what you describe - avoid destroying the fb while it is
being scanned out. (Maybe exynos should have something equivalent too,
but thats a separate issue)

However there is *another* fb reference taken in
omap_plane_mode_set(). And my patch is modelled to do the same in
exynos-drm.

I believe this is necessary under the current model. At least, when
drm_mode_rmfb() is running for the last user of the active
framebuffer, it expects to drop 3 references from the framebuffer
before dropping the 4th causes the object to be destroyed, as follows:

1. drm_mode_rmfb explicitly drops a reference - it calls
__drm_framebuffer_unregister which then calls
__drm_framebuffer_unreference
/* Mark fb as reaped, we still have a ref from fpriv->fbs. */
__drm_framebuffer_unregister(dev, fb);

2. drm_mode_rmfb then calls drm_framebuffer_remove, which calls
drm_mode_set_config_internal() in order to turn off the CRTC, dropping
another reference in the process.
if (tmp->old_fb)
drm_framebuffer_unreference(tmp->old_fb);

3. drm_framebuffer_remove calls drm_plane_force_disable() which drops
another reference:
/* disconnect the plane from the fb and crtc: */
__drm_framebuffer_unreference(old_fb);

4. drm_framebuffer drops the final reference itself, to cause freeing
of the object:
drm_framebuffer_unreference(fb);

So ordinarily, after a fb is created by drm core (with refcnt at 1),
there would have to be 3 references added to it by the time it is the
primary fb so that when we do rmfb, it has a refcnt of 4, and gets
freed correctly.
(The second bug I was seeing with pageflips was that refcnt was 3,
which means that the final reference was dropped in (3) above, but
__drm_framebuffer_unreference doesn't like that at all - it calls
drm_framebuffer_free_bug)

Not being overly familiar with DRM internals I tried to go backwards
to find out where these 3 references would be created during normal
operation. 2 are clear:

1. drm_framebuffer_init() explicitly grabs one:
/* Grab the idr reference. */
drm_framebuffer_reference(fb)

2. drm_mode_set_config_internal() takes one:
if (tmp->primary->fb)
drm_framebuffer_reference(tmp->primary->fb);

Where should the 3rd one be created? I don't know, but looking at
previous exynos commit 25c8b5c304 and omapdrm, I assumed that the drm
driver should take one, both on crtc mode set and crtc page flip.

>> However, I'll be happy if universal planes means the driver does not
>> have to care about this any more. Andrej, please go ahead if you are
>> interested, I'll be happy to test your results.
>
> universal planes will fix up the mess with 2 drm plane objects
> (primary plane + exonys internal primary). So should help to untangle
> this not, but it will not magically fix the refcounting bugs itself.

So even when we move to universal planes (fixing 1 of the issues), its
good that we're having this refcount discussion (which we need to
understand to confidently solve the 2nd issue). Thanks for your input!

Daniel

[PATCH] drm/exynos: fix plane-framebuffer linkage

2014-09-17 Thread Daniel Drake

On Wed, Sep 17, 2014 at 1:44 AM, Joonyoung Shim  
wrote:
> It's problem to add this from commit 25c8b5c3048cb6c98d402ca8d4735ccf910f727c.

My patch moves that drm_framebuffer_reference() call to the plane
function which is called from crtc_mode_set context (and also called
in crtc pageflip path), so there should be no problem here.

> Chip specific drm driver internally doesn't have to care fb reference count if
> there is no special case. We should have switched to universal plane at that
> time.

To me it seems like the chip-specific DRM drivers do need to add a
reference in the crtc_mode_set and crtc page flip paths otherwise
framebuffer removal crashes (expecting to remove 3 references), as
noted by my testing and also in commit 25c8b5c304.

However, I'll be happy if universal planes means the driver does not
have to care about this any more. Andrej, please go ahead if you are
interested, I'll be happy to test your results.

Thanks
Daniel

[PATCH] drm/exynos: fix plane-framebuffer linkage

2014-09-15 Thread Daniel Drake

Pageflipping currently causes some inconsistencies that lead to
crashes. Just run an app that causes a CRTC pageflip in a raw X session
and check that it exits cleanly and can be restarted - you'll see
crashes like:
 Unable to handle kernel NULL pointer dereference at virtual address 0334
 PC is at exynos_drm_crtc_plane_commit+0x20/0x40
 LR is at exynos_drm_crtc_plane_commit+0x20/0x40
 [] (exynos_drm_crtc_plane_commit) from [] 
(exynos_drm_crtc_commit+0x44/0x70)
 [] (exynos_drm_crtc_commit) from [] 
(exynos_drm_crtc_mode_set_commit.isra.2+0xb4/0xc4)
 [] (exynos_drm_crtc_mode_set_commit.isra.2) from [] 
(exynos_drm_crtc_page_flip+0x140/0x1a8)
 [] (exynos_drm_crtc_page_flip) from [] 
(drm_mode_page_flip_ioctl+0x224/0x2dc)
 [] (drm_mode_page_flip_ioctl) from [] 
(drm_ioctl+0x338/0x4fc)

These crashes happen because drm_plane_force_disable has previously set
plane->crtc to NULL.

When drm_mode_page_flip_ioctl() is used to flip another framebuffer
onto the primary plane, crtc->primary->fb is correctly updated (this is
a virtual plane created by plane_helper), but plane->fb is not (this
plane is the real one, created by exynos_drm_crtc_create).

We then come to handle rmfb of the backbuffer, which the "real" primary
plane is incorrectly pointing at. So drm_framebuffer_remove() decides that
the buffer is actually active on a plane and force-disables the plane.

Ensuring that plane->fb is kept up-to-date solves that issue, but
exposes a reference counting problem. Now we see crashes when rmfb is
called on the front-buffer, because the rmfb code expects to drop 3
references here, and there are only 2.

That can be fixed by adopting the reference management found in omapdrm:
Framebuffer references are not taken directly in crtc mode_set context,
but rather in the context of updating the plane, which also covers
flips. Like omapdrm we also unreference the old framebuffer here.

Signed-off-by: Daniel Drake 
---
 drivers/gpu/drm/exynos/exynos_drm_crtc.c  | 12 ++--
 drivers/gpu/drm/exynos/exynos_drm_plane.c |  8 
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c 
b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
index b68e58f..7aa9dee 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
@@ -140,16 +140,8 @@ exynos_drm_crtc_mode_set(struct drm_crtc *crtc, struct 
drm_display_mode *mode,
if (manager->ops->mode_set)
manager->ops->mode_set(manager, >mode);

-   ret = exynos_plane_mode_set(plane, crtc, crtc->primary->fb, 0, 0, 
crtc_w, crtc_h,
-   x, y, crtc_w, crtc_h);
-   if (ret)
-   return ret;
-
-   plane->crtc = crtc;
-   plane->fb = crtc->primary->fb;
-   drm_framebuffer_reference(plane->fb);
-
-   return 0;
+   return exynos_plane_mode_set(plane, crtc, crtc->primary->fb, 0, 0,
+crtc_w, crtc_h, x, y, crtc_w, crtc_h);
 }

 static int exynos_drm_crtc_mode_set_commit(struct drm_crtc *crtc, int x, int y,
diff --git a/drivers/gpu/drm/exynos/exynos_drm_plane.c 
b/drivers/gpu/drm/exynos/exynos_drm_plane.c
index 8371cbd..df27e35 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_plane.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_plane.c
@@ -139,6 +139,14 @@ int exynos_plane_mode_set(struct drm_plane *plane, struct 
drm_crtc *crtc,
overlay->crtc_x, overlay->crtc_y,
overlay->crtc_width, overlay->crtc_height);

+   if (plane->fb)
+   drm_framebuffer_unreference(plane->fb);
+
+   drm_framebuffer_reference(fb);
+
+   plane->fb = fb;
+   plane->crtc = crtc;
+
exynos_drm_crtc_plane_mode_set(crtc, overlay);

return 0;
-- 
1.9.1

mixer_check_mode drops Exynos5 display modes

2014-07-08 Thread Daniel Drake

Hi Sean,

While looking at the following commit I noticed something:

commit f041b257a8997c8472a1013e9f252c3e2a1d879e
Author: Sean Paul 
Date:   Thu Jan 30 16:19:15 2014 -0500

drm/exynos: Remove exynos_drm_hdmi shim


This commit changes how mixer_check_mode() is used. It used to just
exclude certain modes on old mixer versions, but now it seems to be
called unconditionally, for all mixer versions. I guess the effect
here is that some modes on exynos5 setups are dropped whereas they
weren't before. Is that intentional?

Thanks
Daniel

exynos-drm HDMI PHY configuration values

2014-03-25 Thread Daniel Drake

Hi Sean,

In your commit "drm/exynos: hdmi: support extra resolutions using
drm_display_mode timings"  you added several more HDMI PHY configs to
exynos-drm. Thanks for that.

Can you explain where these magic numbers came from?

I'm interested in adding 85.5MHz for 1366x768 support.

Thanks,
Daniel

[PATCH] drm/edid: request HDMI underscan by default

2014-02-27 Thread Daniel Drake

Working with HDMI TVs is a real pain as they tend to overscan by
default, meaning that the pixels around the edge of the framebuffer
are not displayed. This is well explained here:
http://mjg59.dreamwidth.org/8705.html

There is a bit in the HDMI info frame that can request that the
remote display shows the full pixel data ("underscan"). For the
remote display, the HDMI spec states that this is optional - it
doesn't have to listen. That means that most TVs will probably ignore
this.

But, maybe there are a handful of TVs for which this would help
the situation. As we live in a digital world, ask the remote
display not to overscan by default.

Signed-off-by: Daniel Drake 
---
 drivers/gpu/drm/drm_edid.c | 1 +
 1 file changed, 1 insertion(+)

Replaces the patch titled "video: hdmi: request underscan by default"
This version moves the change to the DRM layer, as requested by
Ville Syrj?l?.

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index b924306..f8d8a1d 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -3599,6 +3599,7 @@ drm_hdmi_avi_infoframe_from_display_mode(struct 
hdmi_avi_infoframe *frame,

frame->picture_aspect = HDMI_PICTURE_ASPECT_NONE;
frame->active_aspect = HDMI_ACTIVE_ASPECT_PICTURE;
+   frame->scan_mode = HDMI_SCAN_MODE_UNDERSCAN;

return 0;
 }
-- 
1.8.3.2

[PATCH] video: hdmi: request underscan by default

2014-02-24 Thread Daniel Drake

Working with HDMI TVs is a real pain as they tend to overscan by
default, meaning that the pixels around the edge of the framebuffer
are not displayed. This is well explained here:
http://mjg59.dreamwidth.org/8705.html

There is a bit in the HDMI info frame that can request that the
remote display shows the full pixel data ("underscan"). For the
remote display, the HDMI spec states that this is optional - it
doesn't have to listen. That means that most TVs will probably ignore
this.

But, maybe there are a handful of TVs for which this would help
the situation. As we live in a digital world, ask the remote
display not to overscan by default.

Signed-off-by: Daniel Drake 
---
 drivers/video/hdmi.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c
index 9e758a8..6c2d924 100644
--- a/drivers/video/hdmi.c
+++ b/drivers/video/hdmi.c
@@ -54,6 +54,7 @@ int hdmi_avi_infoframe_init(struct hdmi_avi_infoframe *frame)
frame->type = HDMI_INFOFRAME_TYPE_AVI;
frame->version = 2;
frame->length = HDMI_AVI_INFOFRAME_SIZE;
+   frame->scan_mode = HDMI_SCAN_MODE_UNDERSCAN;

return 0;
 }
-- 
1.8.3.2

[PATCH] drm/exynos: power up HDMI before mixer

2014-02-21 Thread Daniel Drake

Testing on Exynos4412, when changing screen resolution under GNOME/X11,
first DPMS is set to OFF via drm_mode_obj_set_property_ioctl.

This is done via drm_hdmi_dpms which powers down the mixer then the
HDMI component.

Then the mode change happens. We then see this call chain, powering things
back on:
 exynos_drm_crtc_commit
  exynos_drm_crtc_dpms
   exynos_drm_encoder_crtc_dpms
drm_hdmi_dpms

And at this point, drm_hdmi_dpms first powers on the mixer, then the
HDMI component.

Strangely enough, this works fine on the first resolution change, but
on the second it hangs in mixer_poweron() in:
mixer_reg_write(res, MXR_INT_EN, ctx->int_en);

Through some experiments I determined that this register write will
hang the machine here unless the hdmi_resources.hdmi clock is running.
I can't explain why there is a difference between the first and second
time; I did check that the underlying clock gating register has the
same value in both cases.

Anyway, the mixer clearly has some kind of dependency on the HDMI
component, so lets make sure we power that up first.

Signed-off-by: Daniel Drake 
---
 drivers/gpu/drm/exynos/exynos_drm_hdmi.c | 9 -
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_hdmi.c 
b/drivers/gpu/drm/exynos/exynos_drm_hdmi.c
index 8548b97..3bfd9d6 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_hdmi.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_hdmi.c
@@ -261,10 +261,17 @@ static void drm_hdmi_dpms(struct device *subdrv_dev, int 
mode)
 {
struct drm_hdmi_context *ctx = to_context(subdrv_dev);

+   /* When powering up, we must first power up the HDMI component, as
+* otherwise mixer register accesses will sometimes hang.
+* When powering down, we do the opposite: mixer off, HDMI off. */
+
+   if (mode == DRM_MODE_DPMS_ON && hdmi_ops && hdmi_ops->dpms)
+   hdmi_ops->dpms(ctx->hdmi_ctx->ctx, mode);
+
if (mixer_ops && mixer_ops->dpms)
mixer_ops->dpms(ctx->mixer_ctx->ctx, mode);

-   if (hdmi_ops && hdmi_ops->dpms)
+   if (mode != DRM_MODE_DPMS_ON && hdmi_ops && hdmi_ops->dpms)
hdmi_ops->dpms(ctx->hdmi_ctx->ctx, mode);
 }

-- 
1.8.3.2

drm_do_probe_ddc_edid ENXIO check too aggressive?

2013-12-18 Thread Daniel Drake

On Wed, Dec 18, 2013 at 2:18 PM, Daniel Drake  wrote:
> Yes, this looks very similar to the approach I tried earlier. I guess
> the patch was written for the same reasons as well.
> Sean, any objections to me taking your patch and sending it upstream?
>
> http://git.chromium.org/gitweb/?p=chromiumos/third_party/kernel-next.git;a=commitdiff;h=77aed71acf4b84e722ead24c820d8ad059121e5b

Seems easier said than done. The patch looks innocent but it touches
on other stuff that has changed a lot.

For example, it adds cancel_delayed_work_sync() in the hdmi_poweroff()
path. With my naive backport to vanilla-3.8, hdmi_poweroff() gets
called within the context of that work item, and a work item trying to
cancel_delayed_work_sync() on itself hangs. Reproduced just by
unplugging the display once.

I see that things have changed substantially in the ChromiumOS tree,
for example this patch probably has some effect:

commit 29ae0c6096395a96a597453271946fc7d8442b6e
Author: Sean Paul 
Date:   Thu Jun 20 17:24:12 2013 -0400

drm/exynos: Consolidate suspend/resume in drm_drv

As this job seems bigger than anticipated and I don't have any Exynos
hardware that can boot mainline, I think I might have to stop here,
and hope that ChromiumOS guys upstream all this soon?

Daniel

drm_do_probe_ddc_edid ENXIO check too aggressive?

2013-12-18 Thread Daniel Drake

On Wed, Dec 18, 2013 at 1:58 PM, Daniel Kurtz  wrote:
> +seanpaul
>
> I think the problem is that the hdmi irq is really just an undebounced
> gpio interrupt, and thus, it is firing way too soon.
> The chromium kernel adds an excplicit 1.1 second timer to debounce hpd
> between the hdmi hpd-gpio irq and taking any action (ie reading the
> edid).  I believe this will eventually make its way upstream, but is
> still pending some other patches:
>
> http://git.chromium.org/gitweb/?p=chromiumos/third_party/kernel-next.git;a=blob;f=drivers/gpu/drm/exynos/exynos_hdmi.c;h=1258c67e87f360c846c64bb3f04436a68018b4fe;hb=refs/heads/chromeos-3.8#l2647

Yes, this looks very similar to the approach I tried earlier. I guess
the patch was written for the same reasons as well.
Sean, any objections to me taking your patch and sending it upstream?

http://git.chromium.org/gitweb/?p=chromiumos/third_party/kernel-next.git;a=commitdiff;h=77aed71acf4b84e722ead24c820d8ad059121e5b

Thanks
Daniel

drm_do_probe_ddc_edid ENXIO check too aggressive?

2013-12-18 Thread Daniel Drake

On Wed, Dec 18, 2013 at 2:43 AM, Daniel Vetter  wrote:
> I think we can do it simpler. When you get a hpd interrupt you eventually
> call drm_helper_hpd_irq_event which will update all the state and poke
> connectors. I'd just create a delay_work which you launch from
> hdmi_irq_thread with a 1 second delay which again calls
> drm_helper_hpd_irq_event. That way you won't miss the EDID (presuming the
> user isn't _really_ careful with pluggin in the cable slowly).

OK, I like this more simplistic option.

However, one more pesky detail, if I am understanding your suggestion
correctly. You are hoping for:

1. I connect the display, causing HPD interrupt
2. hpd irq handler calls drm_helper_hpd_irq_event() directly. Fails to
read EDID, no big deal.
3. hpd irq handles schedules a work item to call
drm_helper_hpd_irq_event() a second time, 1 second later. Reads EDID
sucessfully, image gets output to display.

That doesn't quite work. drm_helper_hpd_irq_event() in step 2 notices
and records the state change (disconnected --> connected). And after
drm_helper_probe_single_connector_modes() fails to read the EDID, it
starts outputting an image at 1024x768. When we come to step 3, we
call drm_helper_hpd_irq_event again, but that just returns without
doing anything as it does not detect any new state change.

If we skip step 2, i.e. always delay the call to
drm_helper_hpd_irq_event by 1 second, the problem is solved. As a
user, the 1 second delay is not noticable. What do you think about
just doing this?

Daniel

drm_do_probe_ddc_edid ENXIO check too aggressive?

2013-12-17 Thread Daniel Drake

On Mon, Dec 16, 2013 at 5:40 PM, Daniel Vetter  wrote:
> Have a bit of logic in the exynos ->detect function to re-try a 2nd
> round of edid probing after each hdp interrupt if the first one
> returns an -ENXIO. Only tricky part is to be careful with edge
> detection so that userspace gets all the hotplug events still.
> Presuming you don't have any funkiness with reprobing causing yet
> another hpd interrupt and stuff like that (seen it all), as long as
> you're using the helpers in drm_crtc_helper.c it should all be working
> correctly. So you want a work item which just grabs all modeset locks
> and then calls drm_helper_probe_single_connector_modes or something on
> the right connector.

Thanks for the tips. Having trouble sticking to those details though.
exynos_drm_connector_detect() is actually a long way away from EDID
probing so it is hard to detect the ENXIO case there.

What happens here is:
exynos hdmi_irq_thread() calls drm_helper_hpd_irq_event()
That then calls exynos_drm_connector_detect(), which returns a simple
"yes, hpd detection says we are connected"
Then it calls drm_kms_helper_hotplug_event() for which the call chain is:

drm_kms_helper_hotplug_event
exynos_drm_output_poll_changed
drm_fb_helper_hotplug_event
drm_fb_helper_probe_connector_mode
exynos_drm_connector_fill_modes
drm_helper_probe_single_connector_modes
exynos_drm_connector_get_modes
drm_get_edid
drm_do_get_edid
drm_do_probe_ddc_edid <-- ENXIO in here

drm_do_probe_ddc_edid actually swallows the ENXIO code and just
returns -1, so that particular error is indistinguishable from others.

Trying to follow your suggestions, the closest would seem to be something like:

1. Modify exynos_drm_connector_detect to read and cache the EDID right
away. If EDID read fails for any reason, report "no connection" and
schedule a work item to retry the EDID read. If the later EDID read
succeeds, call drm_kms_helper_hotplug_event()

2. Modify exynos_drm_connector_get_modes to return cached EDID

Does that sound sensible?

Thanks
Daniel

drm_do_probe_ddc_edid ENXIO check too aggressive?

2013-12-16 Thread Daniel Drake

On Mon, Dec 16, 2013 at 4:19 PM, Daniel Vetter  wrote:
> This usually happens if the hpd isn't properly recessed and we start
> the i2c transaction while the physical connection hasn't been
> established properly yet. If you're _really_ slow and careful you can
> probably even break your current delay (presuming this theory is
> correct).

Hmm yes, I think you are probably right. Without touching the HDMI
cable I disconnect and reconnect the power cable of my TV. Presumably
that plug is more "atomic" :)

When I do that, it detects the resolution fine.

Do you have any suggestions on how to fix this then? Anything nicer
than e.g. a 1 second delay before processing hpd events? That would
still fail in the "slow connect" case but might be the best option?

Daniel

drm_do_probe_ddc_edid ENXIO check too aggressive?

2013-12-16 Thread Daniel Drake

Resend with correct addresses for Eugeni and Chris...

On Mon, Dec 16, 2013 at 3:47 PM, Daniel Drake  wrote:
> Hi,
>
> I'm using exynos_drm on Exynos4412 to output to a Sony HDMI TV.
>
> When I disconnect and then re-plug the TV, Exynos detects this event
> and tries to read the EDID from the DDC over I2C.
>
> The DDC does not provide an ACK at this point, so the i2c-s3c2410
> driver reports ENXIO, which seems to agree with the documentation:
>
> ENXIO
> Returned by I2C adapters to indicate that the address phase
> of a transfer didn't get an ACK.  While it might just mean
> an I2C device was temporarily not responding, usually it
> means there's nothing listening at that address.
>
> As of commit 9292f37e1f5c79400254dca46f83313488093825, DRM treats
> ENXIO as "no device, bail":
>
> Author: Eugeni Dodonov 
> Date:   Thu Jan 5 09:34:28 2012 -0200
>
> drm: give up on edid retries when i2c bus is not responding
>
> But in the case of my Sony TV it seems that we hit the "temporarily
> not responding" case, because if I insert a delay, the message gets
> acked and the EDID gets read successfully. Similarly, if I revert the
> patch so that we attempt the query a few times times, it succeeds on
> second retry.
>
> Any suggested solutions to this issue?
>
> Thanks,
> Daniel

drm_do_probe_ddc_edid ENXIO check too aggressive?

2013-12-16 Thread Daniel Drake

Hi,

I'm using exynos_drm on Exynos4412 to output to a Sony HDMI TV.

When I disconnect and then re-plug the TV, Exynos detects this event
and tries to read the EDID from the DDC over I2C.

The DDC does not provide an ACK at this point, so the i2c-s3c2410
driver reports ENXIO, which seems to agree with the documentation:

ENXIO
Returned by I2C adapters to indicate that the address phase
of a transfer didn't get an ACK.  While it might just mean
an I2C device was temporarily not responding, usually it
means there's nothing listening at that address.

As of commit 9292f37e1f5c79400254dca46f83313488093825, DRM treats
ENXIO as "no device, bail":

Author: Eugeni Dodonov 
Date:   Thu Jan 5 09:34:28 2012 -0200

drm: give up on edid retries when i2c bus is not responding

But in the case of my Sony TV it seems that we hit the "temporarily
not responding" case, because if I insert a delay, the message gets
acked and the EDID gets read successfully. Similarly, if I revert the
patch so that we attempt the query a few times times, it succeeds on
second retry.

Any suggested solutions to this issue?

Thanks,
Daniel

exynos-drm 1024x768 HDMI output

2013-12-13 Thread Daniel Drake

On Fri, Dec 13, 2013 at 9:46 AM, Daniel Drake  wrote:
> Lets just accept the fact that the first line of the output image is
> rendered badly. Specifically, it has 257 black pixels added onto the
> end of it. The following rows do not exhibit that problem.
>
> To accept and ignore this oddity, I want to make the device start
> outputting the image exactly 1024+257 pixels too early. i.e. I want
> 1281 junk pixels before the image starts.
>
> Then, I want to adjust the timing parameters appropriately so that
> those junk pixels do not appear on the display. I want those 1281 junk
> pixels to be sent to the display during the horizontal scans that
> happen before the top left visible pixel is clocked. I think I'm
> saying: I want to adjust the field of view so that those 1281 junk
> pixels are rendered inside the vertical back porch.

Nearly got something working along these lines:

tg->vact_st -= 1
tg->vact_sz += 1

Now the 257 bad pixels start at the top left of my display (rather
than on the second row of visible pixels), and that causes the
annoying horizontal wrap that I wrote about in my first mail.
So add from earlier:
tg->hact_st -= 257
tg->hact_sz + 257

Now the displayed image looks good.

But looking closer, I am missing the first horizontal row of pixels,
and the rightmost vertical column of pixels.

Daniel

exynos-drm 1024x768 HDMI output

2013-12-13 Thread Daniel Drake

On Thu, Dec 12, 2013 at 4:46 PM, Daniel Drake  wrote:
> What I feel we are missing here is an explanation for why the first
> row of pixels is treated differently from the rest. Every value that I
> tweak seems to have an effect on the first line (which was rendered
> OK) as well as all the others (which were bad). If it were just a
> matter of tweaking e.g. h_fsz then I would expect *all* rows of pixels
> to have been rendered badly in the first place.
>
> Nevertheless, I'm happy to continue throwing values at the variables
> if that is our best option... We need to find a way of making a change
> that effects the only first line, or all the lines except the first.

I have an idea, but my understanding of CRT timings isn't quite strong
enough to tell me exactly how to implement it. Maybe you can help
suggest what variables to tweak in order to try this experiment:

Lets just accept the fact that the first line of the output image is
rendered badly. Specifically, it has 257 black pixels added onto the
end of it. The following rows do not exhibit that problem.

To accept and ignore this oddity, I want to make the device start
outputting the image exactly 1024+257 pixels too early. i.e. I want
1281 junk pixels before the image starts.

Then, I want to adjust the timing parameters appropriately so that
those junk pixels do not appear on the display. I want those 1281 junk
pixels to be sent to the display during the horizontal scans that
happen before the top left visible pixel is clocked. I think I'm
saying: I want to adjust the field of view so that those 1281 junk
pixels are rendered inside the vertical back porch.

Does that make any sense?

Daniel

exynos-drm 1024x768 HDMI output

2013-12-12 Thread Daniel Drake

On Wed, Dec 4, 2013 at 12:14 PM, Sean Paul  wrote:
> I assume this is the "1024x768 at 60Hz" mode in drm_edid.c?
>
> hdisplay = 1024
> hsync_start = 1048
> hsync_end = 1184
> htotal = 1344
> vdisplay = 768
> vsync_start = 771
> vsync_end = 777
> vtotal = 806

That's the one.

> I don't have any documentation for the 4412, so I'm flying blind as well.

What about documentation for exynos5, which runs from the same driver?

>> Specifically, I have not yet found an explanation for why the first
>> row gets rendered right, and I haven't found a way to reduce the size
>> of region X, although I have figured out how to move it around:
>>
>> The standard timings for 1024x768 are:
>> hdisplay=1024
>> hsync_start=1048
>> hsync_end=1185
>> htotal=1344
>>
>> Using these numbers, exynos-drm writes:
>> tg->hact_st = 320
>> tg->hact_sz = 1024
>>
>> If I subtract and add 257 (the size of the black region X)  respectively, 
>> i.e.
>> tg->hact_st = 63
>> tg->hact_sz = 1288
>>
>> Then I get the following:
>>
>> X
>> B
>> C
>
> Have you tried:
>
> hact_st = 320
> h_fsz += 257?
>
> hact_st specifies where to start scanning out the buffer, so it's not
> surprising that you're missing the first 257 pixels in this case.
> Maybe bumping up the full size of the h_line will help?

If I do:
hact_st = 320 (no change)
hact_sz += 257
h_fsz = h_line = 1344 (no change)
then I don't see any change in behaviour to the base case, where the
second line is 'indented' by 257 black pixels.

If I change h_fsz alone, or h_line alone, I get no display at all on
the TV. I can only change them in harmony.

If I do:
hact_st = 320 (no change)
hact_sz = 1024 (no change)
h_fsz += 257
h_line += 257
then my TV thinks it is outputting at 1281x768. To the left and right
of the image, where I would expect black side bars (its a widescreen
TV, so it will pad the sides with black to fit a 4:3 resolution on), I
now get visual corruption (basically one of the pixels from the screen
repeated forming a horizontal line).

The first 257 pixels of the first row of the image output are not
shown anywhere.
The second row of pixels (and the ones that follow) start roughly in
the right place but the final 257 pixels are not displayed anywhere.

What I feel we are missing here is an explanation for why the first
row of pixels is treated differently from the rest. Every value that I
tweak seems to have an effect on the first line (which was rendered
OK) as well as all the others (which were bad). If it were just a
matter of tweaking e.g. h_fsz then I would expect *all* rows of pixels
to have been rendered badly in the first place.

Nevertheless, I'm happy to continue throwing values at the variables
if that is our best option... We need to find a way of making a change
that effects the only first line, or all the lines except the first.

Daniel

exynos-drm 1024x768 HDMI output

2013-12-03 Thread Daniel Drake

Hi,

Thanks a lot for this exynos-drm commit:

commit 62747fe0ad5169a767987d9009e3398466555cde
Author: Sean Paul 
Date:   Tue Jan 15 08:11:08 2013 -0500

drm/exynos: hdmi: support extra resolutions using drm_display_mode timings

As you probably know, many people had written off the Exynos4's
capability to output to non-TV resolutions over HDMI. Now, with your
patch, 640x480 works perfectly, and other resolutions are hopefully
within reach.

I would like to get 1024x768 working on Exynos4412. I'm using a Sony
TV which has an EDID that offers 1024x768 in its established timing
bitmap - i.e. it supports 1024x768 via the standard/ancient VESA
timings. I have tested the TV with other devices, it seems to work
fine at this res.

However, on Exynos4412 with exynos-drm, running at this resolution is
not quite right. The very first row of pixels is rendered perfectly,
but all off the others are offset and wrapped. I'll try to explain
with some ASCII art:

A
X
B

Imagine that is the complete first 3 rows of pixels shown on my display.
The first row, with pixels A, is complete, and starts and finishes
where it should do on the physical display.

However, the second row on the display starts with a series of
always-black pixels, this is the region marked X, which occupies 257
pixels. Immediately after, the second row of pixels of the output
image starts (B), then when it hits the end of the screen, it wraps
around onto the next row of pixels on the screen. Then the third row
of the image pixels starts (C) and so on.

Sounds like a hsync issue, right? I have been playing with the
register writes in hdmi_v14_mode_set() but I can't quite get a perfect
output.

Is there any documentation available for these registers? Why do we
have to program the numbers twice (once in HDMI regs, once in timing
generator regs)?

Specifically, I have not yet found an explanation for why the first
row gets rendered right, and I haven't found a way to reduce the size
of region X, although I have figured out how to move it around:

The standard timings for 1024x768 are:
hdisplay=1024
hsync_start=1048
hsync_end=1185
htotal=1344

Using these numbers, exynos-drm writes:
tg->hact_st = 320
tg->hact_sz = 1024

If I subtract and add 257 (the size of the black region X)  respectively, i.e.
tg->hact_st = 63
tg->hact_sz = 1288

Then I get the following:

X
B
C

i.e. all rows of displayed output are now fine, except for the first.
On the first row, the leftmost 257 pixels of output are no longer
visible anywhere, the rest of them start in the wrong place (top left,
rather than 257 pixels in), and the black region X is now positioned
at the right hand side of the first row.

That is the closest I have found so far.

Any thoughts or tips much appreciated.

Thanks
Daniel

DT binding review for Armada display subsystem

2013-07-17 Thread Daniel Drake

On Mon, Jul 15, 2013 at 3:09 PM, Sascha Hauer  wrote:
> You don't have to call drm_irq_install(). Both the exynos and i.MX
> driver do without it and at least the i.MX driver uses multiple irqs per
> drm_device.

Good point, thanks. That unblocks that item.

>> Secondly, devm. I understand from the "best practices" thread that we
>> want to have exactly one platform_device which corresponds to the
>> "super node", and all of the individual display controller components
>> will be represented by DT nodes but *without* their own
>> platform_device.
>
> A super node approach doesn't mean that there's only one platform
> device. You can still have a platform device for each component.

Yes. As noted, this was simply a preference that seemed to emerge in
the previous discussion. If in practice it causes too many headaches,
maybe we will change our minds. For now devm is the only issue I have
seen; I am avoiding devm where it is no longer possible under this
design.

The goal of getting this driver working properly on OLPC XO seems to
be a twisty path that presents a new issue at every turn. In the above
work I am trying to first implement the DT binding for Armada
510/cubox, as that is what the current code is aimed at.

I am now facing a problem with i2c/TDA998x which Russell already noted:

http://lists.freedesktop.org/archives/dri-devel/2013-June/039632.html
   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.

This is because the existing DRM/encoder system works something like this:
1. i2c driver (e.g. tda998x_drv) registers as an i2c_driver via
drm_i2c_encoder_register()
2. The drm driver (i.e. armada) later has to know that it is expecting
a tda998x instance. It calls drm_i2c_encoder_init() to set this up.

drm_i2c_encoder init requires:
 1. The i2c_adapter in question. In a DT world, we could get this from
finding the parent node of the tda998x node (this is the i2c bus
master), and then using i2c_for_each_dev to find the first i2c adapter
instance that has the same of_node.
 2. i2c_board_info - basically the address of the device on the i2c
bus. This is basically the way of instantiating i2c devices from
platform data. Not the DT way :(

In a DT world the i2c driver would be instantiated from a node in the
DT, which already includes the info that would come in i2c_board_info.
Then later it would have to be linked to a DRM slave/encoder, perhaps
when the DRM device finds the of_node corresponding to it.

So I think the next step is fixing up DRM, as Russell hinted. Unless
someone sees another acceptable option that doesn't break our DT
design.

I am going away for 3-4 weeks now, so you won't be hearing for me for
a while. Just in case someone else wants to pick up the task in the
mean time, here is my work in progress. I'm still reading and
learning, so please don't do any detailed reviews yet :)

http://dev.laptop.org/~dsd/20130717/0001-dove-clk-dt-wip.patch

It includes the previous clock selection patch as this stuff is quite
closely bound with DT support.

Thanks
Daniel

Re: DT binding review for Armada display subsystem

2013-07-17 Thread Daniel Drake

On Mon, Jul 15, 2013 at 3:09 PM, Sascha Hauer s.ha...@pengutronix.de wrote:
 You don't have to call drm_irq_install(). Both the exynos and i.MX
 driver do without it and at least the i.MX driver uses multiple irqs per
 drm_device.

Good point, thanks. That unblocks that item.

 Secondly, devm. I understand from the best practices thread that we
 want to have exactly one platform_device which corresponds to the
 super node, and all of the individual display controller components
 will be represented by DT nodes but *without* their own
 platform_device.

 A super node approach doesn't mean that there's only one platform
 device. You can still have a platform device for each component.

Yes. As noted, this was simply a preference that seemed to emerge in
the previous discussion. If in practice it causes too many headaches,
maybe we will change our minds. For now devm is the only issue I have
seen; I am avoiding devm where it is no longer possible under this
design.

The goal of getting this driver working properly on OLPC XO seems to
be a twisty path that presents a new issue at every turn. In the above
work I am trying to first implement the DT binding for Armada
510/cubox, as that is what the current code is aimed at.

I am now facing a problem with i2c/TDA998x which Russell already noted:

http://lists.freedesktop.org/archives/dri-devel/2013-June/039632.html
   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.

This is because the existing DRM/encoder system works something like this:
1. i2c driver (e.g. tda998x_drv) registers as an i2c_driver via
drm_i2c_encoder_register()
2. The drm driver (i.e. armada) later has to know that it is expecting
a tda998x instance. It calls drm_i2c_encoder_init() to set this up.

drm_i2c_encoder init requires:
 1. The i2c_adapter in question. In a DT world, we could get this from
finding the parent node of the tda998x node (this is the i2c bus
master), and then using i2c_for_each_dev to find the first i2c adapter
instance that has the same of_node.
 2. i2c_board_info - basically the address of the device on the i2c
bus. This is basically the way of instantiating i2c devices from
platform data. Not the DT way :(

In a DT world the i2c driver would be instantiated from a node in the
DT, which already includes the info that would come in i2c_board_info.
Then later it would have to be linked to a DRM slave/encoder, perhaps
when the DRM device finds the of_node corresponding to it.

So I think the next step is fixing up DRM, as Russell hinted. Unless
someone sees another acceptable option that doesn't break our DT
design.

I am going away for 3-4 weeks now, so you won't be hearing for me for
a while. Just in case someone else wants to pick up the task in the
mean time, here is my work in progress. I'm still reading and
learning, so please don't do any detailed reviews yet :)

http://dev.laptop.org/~dsd/20130717/0001-dove-clk-dt-wip.patch

It includes the previous clock selection patch as this stuff is quite
closely bound with DT support.

Thanks
Daniel
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

DT binding review for Armada display subsystem

2013-07-15 Thread Daniel Drake

On Fri, Jul 12, 2013 at 10:44 AM, Daniel Drake  wrote:
> Based on the outcomes of the "Best practice device tree design for display
> subsystems" discussion I have drafted a DT binding. Comments much appreciated.
>
> At a high level, it uses a "super node" as something for the driver to bind
> to, needed because there is no clear one device that controls all the
> others, and also to distinguish between the Armada 510 possible use cases
> of having one video card with two outputs, or two independent video cards.
> It uses node-to-node linking beyond that point, V4L2 style.

As this hasn't been shot down very far, I've started to implement the
driver side of this binding. I have already run into a couple of
little problems.

First, interrupts. In the dt binding, each "lcd controller" node
defines which interrupt it listens to, and in the armada 510 case
there are indeed 2 interrupts (47 and 46, one for each LCD
controller). And remember that the drm driver itself binds to the
super-node.

Looking at drm_irq_install(), it looks like DRM only supports 1
interrupt line per drm_device. As we have no known users who will want
these two LCD controllers represented as 2 CRTCs on 1 DRM device
(which would require management of 2 interrupt lines), I figured this
might be a reason to ignore that use case for now (from the coding
standpoint), meaning that in all cases we are left with each DRM
device having 1 CRTC, corresponding to 1 LCD controller, which has
exactly 1 interrupt line.

That still doesn't solve the problem though. drm_irq_install calls
into drm_platform to figure out which IRQ number to use, and that code
looks at the platform_device (i.e. super node). In this case we don't
have the irq info there, we have it in the "lcd controller" node.

Do I invent our own "DRM bus" implementation so that we can override
get_irq()? Start to standardise our DT design as a generic drm_dt.c
bus implementation? Any thoughts?

Secondly, devm. I understand from the "best practices" thread that we
want to have exactly one platform_device which corresponds to the
"super node", and all of the individual display controller components
will be represented by DT nodes but *without* their own
platform_device. As we now put things (clocks, interrupts) into DT
nodes that don't have a corresponding platform_device, we lose the
ability to use devm. Russell wasn't too pleased last time I posted a
patch moving away from devm, admittedly last time the patch was bad
and it wasn't necessary, but this time it looks like it might be.

Finally, just curious, do we still want to support non-DT platform
data type setups on this driver? That adds a bit of coding effort. Not
a problem, just wanted to check.

Daniel

DT binding review for Armada display subsystem

2013-07-13 Thread Daniel Drake

On Sat, Jul 13, 2013 at 2:35 AM, Jean-Francois Moine  wrote:
> I use my Cubox for daily jobs as a desktop computer. My kernel is a DT
> driven 3.10.0. The dove-drm, tda998x and si5351 (clock) are kernel
> modules. I set 3 clocks in the DT for the LCD0: lcdclk, axi and extclk0
> (si5351). Normally, the external clock is used, but, sometimes, the
> si5351 module is not yet initialized when the drm driver starts. So,
> for 1920x1080p, it uses the lcdclk which sets the LCD clock to 13
> (40/3) instead of 148500. As a result, display and sound still work
> correctly on my TV set thru HDMI.
>
> So, it would be nice to have 2 usable clocks per LCD, until we find a
> good way to initialize the modules in the right order at startup time.

Having multiple usable clocks is implemented in the patch I referred
you to. However it doesn't solve the "better clock turns up after
initializing the driver" problem, which seems like a tricky one.

Maybe the best solution is to defer probe until all DT-defined clocks
become available. That means that whoever compiles the kernel must
take care to not forget to build the clock drivers for all the clocks
referenced in this part of the DT, but perhaps that is a reasonable
thing to require.

On the other hand, this problem acts in support of a simpler approach
mentioned by Sebastian: developers figure out what the best clock is
for each CRTC on each board, and the DT only specifies that one clock.
The driver uses that clock if it is available and defers probe if it
is not.

> Back to the specific case of the Cubox with new ideas.
>
> The Cubox is based on the Armada 510 (Dove), so, all the hardware must
> be described in dove.dtsi. This includes the LCDs and DCON/IRE, the
> possible clocks and the (static) v4l2 links:

As a sidenote, I think we have concluded that we are not going to
interact with the armada 510 DCON in any way at the moment. The driver
will not have code that touches it, and the DT will not mention it.
The first person who actually needs to use the DCON will have the
responsibility for doing that work. Nevertheless it makes sense for us
to pick a DT design where we know that the DCON could be slotted in
later.

> lcd0: lcd-controller at 82 {
> compatible = "marvell,dove-lcd0";
> reg = <0x82 0x1c8>;
> interrupts = <47>;
> clocks = <_clk 3>, <>;
> clock-names = "axi", "lcdclk";
> marvell-output = < 0>;
> status = "disabled";
> };
>
> lcd1: lcd-controller at 81 {
> compatible = "marvell,dove-lcd1";
> reg = <0x81 0x1c8>;
> interrupts = <46>;
> clocks = <_clk 3>, <>;
> clock-names = "axi", "lcdclk";
> marvell-output = < 1>;
> status = "disabled";
> };
>
> /* display controller and image rotation engine */
> dcon: display-controller at 83 {
> compatible = "marvell,dove-dcon";
> reg = <0x83 0xc4>,  /* DCON */
>   <0x831000 0x8c>;  /* IRE */
> interrupts = <45>;
> marvell-input = <>, <>;
> status = "disabled";
> };

I guess the IRE falls into the same category as the DCON - we won't
implement it for now, but knowing where it might fit in is useful.

Why would you put it in the same node as the DCON though? It has its
own separate register space and additionally it is a component shared
with the MMP boards (whereas the DCON is not).

> The specific Cubox hardware (tda998x and si5351) is described in
> dove-cubox.dts, with the new clocks and the v4l2 link dcon0 <--> tda998x.
>
>  {
> si5351: clock-generator {
> ...
> };
> tda998x: hdmi-encoder {
> compatible = "nxp,tda998x";
> reg = <0x70>;
> interrupt-parent = <>;
> interrupts = <27 2>;/* falling edge */
> marvell-input = < 0>;
> };
> };
>  {
> status = "okay";
> clocks = < 0>;
> clock-names = "extclk0";
> };
>  {
> status = "okay";
> marvell-output = <>, 0; /* no connector on 
> port B */
> };

So now you are taking an approach equivalent to the v4l2 standard
"video interfaces" binding, which is the concept of representing a
connection graph via phandles. This means that our two proposals are
equivalent yet I proposed using a standard interface and you proposed
inventing your own, again without explaining why you don't like the
standard interface.

> Then, about the software, the drm driver may not need to know anything
> more (it scans

DT binding review for Armada display subsystem

2013-07-13 Thread Daniel Drake

Hi,

Based on the outcomes of the Best practice device tree design for display
subsystems discussion I have drafted a DT binding. Comments much appreciated.

At a high level, it uses a super node as something for the driver to bind
to, needed because there is no clear one device that controls all the
others, and also to distinguish between the Armada 510 possible use cases
of having one video card with two outputs, or two independent video cards.
It uses node-to-node linking beyond that point, V4L2 style.

I have some questions still:

1. What names does the Armada 510 datasheet give to the components? Below
   I renamed the LCD controllers (which don't control LCDs on any of the
   current target hardware) to display controllers. For what we were
   previously referring to as DCON, I renamed to output selector. I would
   like to match the docs.

   Actually the output selector is not mentioned as per Sebastian's
   suggestion, but I believe it would fit in the below design once the first
   user comes along (e.g. it would slot in the graph between dcon0 and tda99x).

2. On that topic, what names does the Armada 510 datasheet give to the
   available clocks? Lets make them match the docs.

3. What is the remote property in the video interfaces binding? Doesn't
   seem to be documented. But I think I copied its usage style below.





Marvell Armada Display Subsystem

Design considerations
-

The display device that emerges from this subsystem is quite heavily
componentized and the formation of the composite device will vary by SoC and
by board. The DT binding is designed with this flexibility in mind.

For example, there are 2 display controllers on the Armada 510.
They could legitametely be used to form two independent video cards, or
they could be combined into a single video card with two outputs, depending
on board wiring and use case.

As there is no clear component that controls the other components, especially
when we wish to combine the two display controllers into one virtual device, we
define a top-level super node that describes the basic composition of the
overall display device. That node uses phandles to define the start of the
graph of display components.

In the bindings for the individual display components, phandle properties
are used to represent direct, fixed links to other components. We use the
port/endpoint abstraction from bindings/media/video-interfaces.txt to
represent these links.


display super node
--

Required properties:
 - compatible: marvell,armada-display
 - marvell,video-devices : List of phandles to the display controllers that
   form this composite device.

Optional properties;
 - reg: Memory address and size of a RAM allocation designated as graphics
   memory
 - linux,video-memory-size: If reg is not provided, this property defines
   how much memory to cut out of regular available RAM to be used as graphics
   memory.


display-controller node
---

Required properties:
 - compatible: marvell,armada-510-display, marvell,mmp2-display or
  marvell,mmp3-display
 - reg: Register space for this display controller
 - clocks: List of clocks available to this device. From common clock binding.
 - clock-names: Names of the given clocks (see below)
 - ports: From video interface binding

Valid clock names for Armada 510: extclk0 extclk1 axi pll

Valid clock names for MMP2: lcd1 lcd2 axi hdmi

Valid clock names for MMP3: lcd1 lcd2 axi hdmi dsi lvds


Example:

display {
Â  Â    compatible = marvell,armada-display;
Â  Â  Â  reg = 0 0x3f00 0x100; /* video-mem hole */
Â  Â  Â  marvell,video-devices = dcon0;
Â  Â  };

dcon0: display-controller@2 {
  compatible = marvell,armada-510-display-controller;
Â  Â  Â  reg = 0x2 0x1000;
Â  Â  Â  interrupts = 47;
Â  Â  Â  clocks = ext_clk0;
Â  Â  Â  clock-names = extclk0;

  port {
dcon0_1: endpoint {
  remote = tda998x;
};
  };
};

i2c0 {
Â  Â    tda998x: hdmi-transmitter@60 {
Â  Â  Â    compatible = nxp,tda19988;
Â  Â  Â  Â  reg = 0x60;
port {
  tda998x_1: endpoint {
remote-endpoint = dcon0_1;
  };
};
Â  Â  Â  };
};
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: DT binding review for Armada display subsystem

2013-07-13 Thread Daniel Drake

On Sat, Jul 13, 2013 at 2:35 AM, Jean-Francois Moine moin...@free.fr wrote:
 I use my Cubox for daily jobs as a desktop computer. My kernel is a DT
 driven 3.10.0. The dove-drm, tda998x and si5351 (clock) are kernel
 modules. I set 3 clocks in the DT for the LCD0: lcdclk, axi and extclk0
 (si5351). Normally, the external clock is used, but, sometimes, the
 si5351 module is not yet initialized when the drm driver starts. So,
 for 1920x1080p, it uses the lcdclk which sets the LCD clock to 13
 (40/3) instead of 148500. As a result, display and sound still work
 correctly on my TV set thru HDMI.

 So, it would be nice to have 2 usable clocks per LCD, until we find a
 good way to initialize the modules in the right order at startup time.

Having multiple usable clocks is implemented in the patch I referred
you to. However it doesn't solve the better clock turns up after
initializing the driver problem, which seems like a tricky one.

Maybe the best solution is to defer probe until all DT-defined clocks
become available. That means that whoever compiles the kernel must
take care to not forget to build the clock drivers for all the clocks
referenced in this part of the DT, but perhaps that is a reasonable
thing to require.

On the other hand, this problem acts in support of a simpler approach
mentioned by Sebastian: developers figure out what the best clock is
for each CRTC on each board, and the DT only specifies that one clock.
The driver uses that clock if it is available and defers probe if it
is not.

 Back to the specific case of the Cubox with new ideas.

 The Cubox is based on the Armada 510 (Dove), so, all the hardware must
 be described in dove.dtsi. This includes the LCDs and DCON/IRE, the
 possible clocks and the (static) v4l2 links:

As a sidenote, I think we have concluded that we are not going to
interact with the armada 510 DCON in any way at the moment. The driver
will not have code that touches it, and the DT will not mention it.
The first person who actually needs to use the DCON will have the
responsibility for doing that work. Nevertheless it makes sense for us
to pick a DT design where we know that the DCON could be slotted in
later.

 lcd0: lcd-controller@82 {
 compatible = marvell,dove-lcd0;
 reg = 0x82 0x1c8;
 interrupts = 47;
 clocks = core_clk 3, lcdclk;
 clock-names = axi, lcdclk;
 marvell-output = dcon 0;
 status = disabled;
 };

 lcd1: lcd-controller@81 {
 compatible = marvell,dove-lcd1;
 reg = 0x81 0x1c8;
 interrupts = 46;
 clocks = core_clk 3, lcdclk;
 clock-names = axi, lcdclk;
 marvell-output = dcon 1;
 status = disabled;
 };

 /* display controller and image rotation engine */
 dcon: display-controller@83 {
 compatible = marvell,dove-dcon;
 reg = 0x83 0xc4,  /* DCON */
   0x831000 0x8c;  /* IRE */
 interrupts = 45;
 marvell-input = lcd0, lcd1;
 status = disabled;
 };

I guess the IRE falls into the same category as the DCON - we won't
implement it for now, but knowing where it might fit in is useful.

Why would you put it in the same node as the DCON though? It has its
own separate register space and additionally it is a component shared
with the MMP boards (whereas the DCON is not).

 The specific Cubox hardware (tda998x and si5351) is described in
 dove-cubox.dts, with the new clocks and the v4l2 link dcon0 -- tda998x.

 i2c0 {
 si5351: clock-generator {
 ...
 };
 tda998x: hdmi-encoder {
 compatible = nxp,tda998x;
 reg = 0x70;
 interrupt-parent = gpio0;
 interrupts = 27 2;/* falling edge */
 marvell-input = dcon 0;
 };
 };
 lcd0 {
 status = okay;
 clocks = si5351 0;
 clock-names = extclk0;
 };
 dcon {
 status = okay;
 marvell-output = tda998x, 0; /* no connector on 
 port B */
 };

So now you are taking an approach equivalent to the v4l2 standard
video interfaces binding, which is the concept of representing a
connection graph via phandles. This means that our two proposals are
equivalent yet I proposed using a standard interface and you proposed
inventing your own, again without explaining why you don't like the
standard interface.

 Then, about the software, the drm driver may not need to know anything
 more (it scans the DT for the LCDs and gets all the subdevices thanks
 to the v4l2

DT binding review for Armada display subsystem

2013-07-12 Thread Daniel Drake

On Fri, Jul 12, 2013 at 12:39 PM, Jean-Francois Moine  
wrote:
> - I think it is better to keep the names 'lcd' for the memory to dumb panel
>   sub-devices and 'dcon' for the dumb panel to LCD/VGA sub-device, as
>   named in the spec.

I agree it is worth keeping the spec-defined names, if they do not
cause excessive confusion when mixed with the spec-defined names for
MMP2/MMP3. I'll check the spec you pointed out, thanks.

> - the phandles to the clocks does not tell how the clock may be set by
>   the driver (it is an array index in the 510).

In the other threads on clock selection, we decided that that exact
information on how to select a clock (i.e. register bits/values) must
be in the driver, not in the DT. What was suggested there is a
well-documented scheme for clock naming, so the driver knows which
clock is which. That is defined in the proposed DT binding though the
"valid clock names" part. For an example driver implementation of this
you can see my patch "armada_drm clock selection - try 2".

> - I don't see the use of the phandles in the leaves (dcon and tda998x).

That is defined by the video interfaces common binding. I'm not
immediately aware of the use, but the ability to easily traverse the
graph in both directions seems like a requirement that could easily
emerge from somewhere.

> Well, here is how I feel the DTs:
>
> - for the Cubox (one LCD, the DCON is not used, TDA998x for HDMI/DVI
>   output):

That is the same as my proposal except you have decided to use direct
phandle properties instead of using the common video interfaces
binding (which is just a slightly more detailed way of describing such
links). In the "best practices" thread, the suggestion was raised
multiple times of doing what v4l2 does, so thats why I decided to
explore this here.

> - for some tablet based on the Armada 510 with a LCD and a VGA connector:
>
> video {
> compatible = "marvell,armada-video";
> marvell,video-devices = <>, <>, <>
> };

This proposal is slightly different because it does not describe the
relationship between the LCD controllers and the DCON. Focusing
specifically on LCD1, which would be connected to a DAC via a phandle
property in your proposal. The interconnectivity between the
components represented as a graph (and in the v4l2 way, which includes
my proposal) would be:

LCD1 --- DCON --- DAC

However your phandle properties would "suggest" that LCD1 is directly
connected to the DAC. The driver would have to have special knowledge
that the DCON sits right in the middle. Maybe that is not an issue, as
it is obviously OK for the driver to have *some* knowledge about how
the hardware works :)

I don't think we have a full consensus on whether we want to go the
"v4l2 way" here. But I figured that would be a good thing to propose
in a first iteration to have a clearer idea of what the result would
look like. It sounds like you are not overly keen on it, I would be
interested to hear exactly why.

Thanks,
Daniel

DT binding review for Armada display subsystem

2013-07-12 Thread Daniel Drake

Hi,

Based on the outcomes of the "Best practice device tree design for display
subsystems" discussion I have drafted a DT binding. Comments much appreciated.

At a high level, it uses a "super node" as something for the driver to bind
to, needed because there is no clear one device that controls all the
others, and also to distinguish between the Armada 510 possible use cases
of having one video card with two outputs, or two independent video cards.
It uses node-to-node linking beyond that point, V4L2 style.

I have some questions still:

1. What names does the Armada 510 datasheet give to the components? Below
   I renamed the "LCD controllers" (which don't control LCDs on any of the
   current target hardware) to "display controllers". For what we were
   previously referring to as DCON, I renamed to "output selector". I would
   like to match the docs.

   Actually the output selector is not mentioned as per Sebastian's
   suggestion, but I believe it would fit in the below design once the first
   user comes along (e.g. it would slot in the graph between dcon0 and tda99x).

2. On that topic, what names does the Armada 510 datasheet give to the
   available clocks? Lets make them match the docs.

3. What is the "remote" property in the video interfaces binding? Doesn't
   seem to be documented. But I think I copied its usage style below.





Marvell Armada Display Subsystem

Design considerations
-

The display device that emerges from this subsystem is quite heavily
componentized and the formation of the composite device will vary by SoC and
by board. The DT binding is designed with this flexibility in mind.

For example, there are 2 display controllers on the Armada 510.
They could legitametely be used to form two independent video cards, or
they could be combined into a single video card with two outputs, depending
on board wiring and use case.

As there is no clear component that controls the other components, especially
when we wish to combine the two display controllers into one virtual device, we
define a top-level "super node" that describes the basic composition of the
overall display device. That node uses phandles to define the start of the
graph of display components.

In the bindings for the individual display components, phandle properties
are used to represent direct, fixed links to other components. We use the
port/endpoint abstraction from bindings/media/video-interfaces.txt to
represent these links.


display super node
--

Required properties:
 - compatible: "marvell,armada-display"
 - marvell,video-devices : List of phandles to the display controllers that
   form this composite device.

Optional properties;
 - reg: Memory address and size of a RAM allocation designated as graphics
   memory
 - linux,video-memory-size: If reg is not provided, this property defines
   how much memory to cut out of regular available RAM to be used as graphics
   memory.


display-controller node
---

Required properties:
 - compatible: "marvell,armada-510-display", "marvell,mmp2-display" or
  "marvell,mmp3-display"
 - reg: Register space for this display controller
 - clocks: List of clocks available to this device. From common clock binding.
 - clock-names: Names of the given clocks (see below)
 - ports: From video interface binding

Valid clock names for Armada 510: extclk0 extclk1 axi pll

Valid clock names for MMP2: lcd1 lcd2 axi hdmi

Valid clock names for MMP3: lcd1 lcd2 axi hdmi dsi lvds


Example:

display {
?? ??   compatible = "marvell,armada-display";
?? ?? ?? reg = <0 0x3f00 0x100>; /* video-mem hole */
?? ?? ?? marvell,video-devices = <>;
?? ?? };

dcon0: display-controller at 2 {
  compatible = "marvell,armada-510-display-controller";
?? ?? ?? reg = <0x2 0x1000>;
?? ?? ?? interrupts = <47>;
?? ?? ?? clocks = <_clk0>;
?? ?? ?? clock-names = "extclk0";

  port {
dcon0_1: endpoint {
  remote = <>;
};
  };
};

 {
?? ??   tda998x: hdmi-transmitter at 60 {
?? ?? ??   compatible = "nxp,tda19988";
?? ?? ?? ?? reg = <0x60>;
port {
  tda998x_1: endpoint {
remote-endpoint = <_1>;
  };
};
?? ?? ?? };
};

armada_drm clock selection - try 2

2013-07-03 Thread Daniel Drake

Hi Russell,

Here is a new patch which should incorporate all your previous feedback.
Now each variant passes clock info to the main driver via a new
armada_clk_info structure.

A helper function in the core lets each variant find the best clock.
As you suggested we first try external (dedicated) clocks, where we can
change the rate to find an exact match. We fall back on looking at the rates
of the other clocks and we return the clock that provides us with the closest
match after integer division (rejecting those that don't bring us within 1%).

There is still the possibility that two CRTCs on the same device end up using
the same clock, so I added a usage counter to each clock to prevent the rate
from being changed by another CRTC.

This is compile-tested only - but after your feedback I will add the
remaining required hacks and test it on XO.

diff --git a/drivers/gpu/drm/armada/armada_510.c 
b/drivers/gpu/drm/armada/armada_510.c
index a016888..7dff2dc 100644
--- a/drivers/gpu/drm/armada/armada_510.c
+++ b/drivers/gpu/drm/armada/armada_510.c
@@ -17,12 +17,29 @@
 
 static int armada510_init(struct armada_private *priv, struct device *dev)
 {
-   priv-extclk[0] = devm_clk_get(dev, ext_ref_clk_1);
+   struct armada_clk_info *clk_info = devm_kzalloc(dev,
+   sizeof(struct armada_clk_info) * 4, GFP_KERNEL);
 
-   if (IS_ERR(priv-extclk[0])  PTR_ERR(priv-extclk[0]) == -ENOENT)
-   priv-extclk[0] = ERR_PTR(-EPROBE_DEFER);
+   if (!clk_info)
+   return -ENOMEM;
 
-   return PTR_RET(priv-extclk[0]);
+   /* External clocks */
+   clk_info[0].clk = devm_clk_get(dev, ext_ref_clk_0);
+   clk_info[0].is_dedicated = true;
+   clk_info[0].sclk = SCLK_510_EXTCLK0;
+   clk_info[1].clk = devm_clk_get(dev, ext_ref_clk_1);
+   clk_info[1].is_dedicated = true;
+   clk_info[1].sclk = SCLK_510_EXTCLK1;
+
+   /* Internal/shared clocks */
+   clk_info[2].clk = devm_clk_get(dev, axi);
+   clk_info[2].sclk = SCLK_510_AXI;
+   clk_info[3].clk = devm_clk_get(dev, pll);
+   clk_info[3].sclk = SCLK_510_PLL;
+
+   priv-num_clks = 4;
+   priv-clk_info = clk_info;
+   return 0;
 }
 
 static int armada510_crtc_init(struct armada_crtc *dcrtc)
@@ -38,42 +55,25 @@ static int armada510_crtc_init(struct armada_crtc *dcrtc)
  * supportable, and again with sclk != NULL to set the clocks up for
  * that.  The former can return an error, but the latter is expected
  * not to.
- *
- * We currently are pretty rudimentary here, always selecting
- * EXT_REF_CLK_1 for LCD0 and erroring LCD1.  This needs improvement!
  */
 static int armada510_crtc_compute_clock(struct armada_crtc *dcrtc,
const struct drm_display_mode *mode, uint32_t *sclk)
 {
struct armada_private *priv = dcrtc-crtc.dev-dev_private;
-   struct clk *clk = priv-extclk[0];
-   int ret;
-
-   if (dcrtc-num == 1)
-   return -EINVAL;
-
-   if (IS_ERR(clk))
-   return PTR_ERR(clk);
-
-   if (dcrtc-clk != clk) {
-   ret = clk_prepare_enable(clk);
-   if (ret)
-   return ret;
-   dcrtc-clk = clk;
-   }
+   struct armada_clk_info *clk_info;
+   int err;
+   int divider;
 
-   if (sclk) {
-   uint32_t rate, ref, div;
+   clk_info = armada_drm_find_best_clock(priv, mode-clock * 1000, 
divider);
+   if (!clk_info)
+   return -ENOENT;
 
-   rate = mode-clock * 1000;
-   ref = clk_round_rate(clk, rate);
-   div = DIV_ROUND_UP(ref, rate);
-   if (div  1)
-   div = 1;
+   err = armada_drm_crtc_switch_clock(dcrtc, clk_info);
+   if (err)
+   return err;
 
-   clk_set_rate(clk, ref);
-   *sclk = div | SCLK_510_EXTCLK1;
-   }
+   if (sclk)
+   *sclk = divider | clk_info-sclk;
 
return 0;
 }
diff --git a/drivers/gpu/drm/armada/armada_crtc.c 
b/drivers/gpu/drm/armada/armada_crtc.c
index f489157..8202be2 100644
--- a/drivers/gpu/drm/armada/armada_crtc.c
+++ b/drivers/gpu/drm/armada/armada_crtc.c
@@ -83,6 +83,34 @@ enum csc_mode {
  * porch, which we're reprogramming.)
  */
 
+/* Switch to a new clock source after disabling and unpreparing the current
+ * one. If non-NULL, the new clock source is expected to be prepared before
+ * this function is called. */
+int armada_drm_crtc_switch_clock(struct armada_crtc *dcrtc,
+   struct armada_clk_info *clk_info)
+{
+   int err;
+
+   if (dcrtc-active_clk == clk_info)
+   return 0;
+
+   if (dcrtc-active_clk) {
+   clk_disable_unprepare(dcrtc-active_clk-clk);
+   dcrtc-active_clk-usage_count--;
+   dcrtc-active_clk = NULL;
+   }
+
+   if (clk_info) {
+   err = clk_enable(clk_info-clk);
+   if (err)
+   return err;
+

Re: armada_drm clock selection - try 2

2013-07-03 Thread Daniel Drake

On Mon, Jul 1, 2013 at 3:48 PM, Sebastian Hesselbarth
sebastian.hesselba...@gmail.com wrote:
 I guess extclk0 and extclk1 should be sufficient for clock names.
 Also, they are not dedicated as you can have CRTC0 and CRTC1 use e.g.
 extclk0 simultaneously. See below for .is_dedicated in general.

Maybe we can find better terminology, or just document it a bit
better, but having two CRTCs share the same clock falls within the
scheme designed and implemented there. Dedicated simply means a
clock that is dedicated to the display controller, it is not shared by
other devices.

 +struct armada_clk_info {
 +   struct clk *clk;
 +
 +   /* If this clock is dedicated to us, we can change its rate
 without
 +* worrying about any other users in other parts of the system. */
 +   bool is_dedicated;
 +
 +   /* However, we cannot share the same dedicated clock between two
 CRTCs
 +* if each CRTC wants a different rate. Track the number of users.
 */
 +   int usage_count;


 You can share the same clock between two CRTCs. Just consider CRTC1
 using a mode with half pixel clock as CRTC0. Not that I think this will
 ever happen, but it is possible.

And my implementation already lets that happen - its just that I
didn't document it in enough detail.

 I prefer not to try to find the best clock (source) at all. Let the
 user pass the clock name by e.g. platform_data (or DT) and just try to
 get the requested pixclk or a integer multiple of it. You will never be
 able to find the best clock for all use cases.

 Just figure out, if integer division is sufficient to get requested
 pixclk and clk_set_rate otherwise. This may be useful for current mode
 running at 148.5MHz (1080p50) and new mode at 74.25MHz (1080i50, 720p).

I am not opposed to this approach, it is nice and simple, but as
Russell points out we do additionally need to distinguish between
clocks that are ours to play with, vs those that are shared with
other devices. It would be a bad idea to try call clk_set_rate() on
the AXI bus clock, for example, changing the clock for a whole bunch
of other devices. This is what the is_dedicated flag is about. However
such a flag could be easily added to the DT/platform data definition
that you propose.

Daniel
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Best practice device tree design for display subsystems/DRM

2013-07-03 Thread Daniel Drake

Hi,

I'm looking into implementing devicetree support in armada_drm and
would like to better understand the best practice here.

Adding DT support for a DRM driver seems to be complicated by the fact
that DRM is not hotpluggable - it is not possible for the drm_device
to be initialised without an output, with the output connector/encoder
appearing at some later moment. Instead, the connectors/encoders must
be fully loaded before the drm_driver load function returns. This
means that we cannot drive the individual display components through
individual, separate modules - we need a way to control their load
order.

Looking at existing DRM drivers:

tilcdc_drm takes an approach that each of the components in the
display subsystem (panel, framebuffer, encoder, display controllers)
are separate DT nodes and do not have any DT-level linkage.
It implements just a single module, and that module carefully
initialises things in this order:
 1. Register platform drivers for output components
 2. Register main drm_driver

When the output component's platform drivers get loaded, probes for
such drivers immediately run as they match things in the device tree.
At this point, there is no drm_device available for the outputs to
bind to, so instead, references to these platform devices just get
saved in some global structures.

Later, when the drm_driver gets registered and loaded, the global
structures are consulted to find all of the output devices at the
right moment.


exynos seems to take a the same approach. Components are separate in
the device tree, and each component is implemented as a platform
driver or i2c driver. However all the drivers are built together in
the same module, and the module_init sequence is careful to initialise
all of the output component drivers before loading the DRM driver. The
output component driver store their findings in global structures.



Russell King suggested an alternative design for armada_drm. If all
display components are represented within the same display
super-node, we can examine the DT during drm_device initialisation and
initialise the appropriate output components. In this case, the output
components would not be registered as platform drivers.

The end result would be something similar to exynos/tilcdc (i.e.
drm_driver figuring out its output in the appropriate moment), however
the hackyness of using global storage to store output devices before
drm_driver is ready is avoided. And there is the obvious difference in
devicetree layout, which would look something like:

display {
  compatible = marvell,armada-510-display;
  clocks = ext_clk0, lcd_pll_clk;
  lcd0 {
compatible = marvell,armada-510-lcd;
reg = 0xf182 0x1000;
interrupts = 47;
  };
  lcd1 {
compatible = marvell,armada-510-lcd;
reg = 0xf181 0x1000;
interrupts = 46;
  };
  dcon {
compatible = marvell,armada-510-dcon;
reg = ...;
  };
};

Any thoughts/comments?

Thanks
Daniel
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: Best practice device tree design for display subsystems/DRM

2013-07-03 Thread Daniel Drake

On Tue, Jul 2, 2013 at 12:43 PM, Russell King r...@arm.linux.org.uk wrote:
 I will point out that relying on driver probing orders has already been
 stated by driver model people to be unsafe.  This is why I will not
 adopt such a solution for my driver; it is a bad design.

Just to clarify, what you're objecting to is effectively the
following? Because it is not guaranteed in the future that the probe
order will be the same as the platform_driver_register() calls?

static int __init exynos_drm_init(void)
{
ret = platform_driver_register(hdmi_driver);
if (ret  0)
goto out_hdmi;
ret = platform_driver_register(mixer_driver);
if (ret  0)
goto out_mixer;
ret = platform_driver_register(exynos_drm_common_hdmi_driver);
if (ret  0)
goto out_common_hdmi;
ret = platform_driver_register(exynos_drm_platform_driver);
if (ret  0)
goto out_drm;

(exynos_drm_platform_driver is the driver that creates the drm_device)

Thanks
Daniel
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: Best practice device tree design for display subsystems/DRM

2013-07-03 Thread Daniel Drake

On Tue, Jul 2, 2013 at 1:57 PM, Sebastian Hesselbarth
sebastian.hesselba...@gmail.com wrote:
 I am against a super node which contains lcd and dcon/ire nodes. You can
 enable those devices on a per board basis. We add them to dove.dtsi but
 disable them by default (status = disabled).

 The DRM driver itself should get a video-card node outside of
 soc/internal-regs where you can put e.g. video memory hole (or video
 mem size if it will be taken from RAM later)

For completeness of the discussion, and my understanding too, can you
explain your objections to the display super-node in a bit more
detail?

Thanks
Daniel
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Best practice device tree design for display subsystems/DRM

2013-07-02 Thread Daniel Drake

On Tue, Jul 2, 2013 at 1:57 PM, Sebastian Hesselbarth
 wrote:
> I am against a super node which contains lcd and dcon/ire nodes. You can
> enable those devices on a per board basis. We add them to dove.dtsi but
> disable them by default (status = "disabled").
>
> The DRM driver itself should get a video-card node outside of
> soc/internal-regs where you can put e.g. video memory hole (or video
> mem size if it will be taken from RAM later)

For completeness of the discussion, and my understanding too, can you
explain your objections to the display super-node in a bit more
detail?

Thanks
Daniel

Best practice device tree design for display subsystems/DRM

2013-07-02 Thread Daniel Drake

On Tue, Jul 2, 2013 at 12:43 PM, Russell King  wrote:
> I will point out that relying on driver probing orders has already been
> stated by driver model people to be unsafe.  This is why I will not
> adopt such a solution for my driver; it is a bad design.

Just to clarify, what you're objecting to is effectively the
following? Because it is not guaranteed in the future that the probe
order will be the same as the platform_driver_register() calls?

static int __init exynos_drm_init(void)
{
ret = platform_driver_register(_driver);
if (ret < 0)
goto out_hdmi;
ret = platform_driver_register(_driver);
if (ret < 0)
goto out_mixer;
ret = platform_driver_register(_drm_common_hdmi_driver);
if (ret < 0)
goto out_common_hdmi;
ret = platform_driver_register(_drm_platform_driver);
if (ret < 0)
goto out_drm;

(exynos_drm_platform_driver is the driver that creates the drm_device)

Thanks
Daniel

Best practice device tree design for display subsystems/DRM

2013-07-02 Thread Daniel Drake

Hi,

I'm looking into implementing devicetree support in armada_drm and
would like to better understand the best practice here.

Adding DT support for a DRM driver seems to be complicated by the fact
that DRM is not "hotpluggable" - it is not possible for the drm_device
to be initialised without an output, with the output connector/encoder
appearing at some later moment. Instead, the connectors/encoders must
be fully loaded before the drm_driver load function returns. This
means that we cannot drive the individual display components through
individual, separate modules - we need a way to control their load
order.

Looking at existing DRM drivers:

tilcdc_drm takes an approach that each of the components in the
display subsystem (panel, framebuffer, encoder, display controllers)
are separate DT nodes and do not have any DT-level linkage.
It implements just a single module, and that module carefully
initialises things in this order:
 1. Register platform drivers for output components
 2. Register main drm_driver

When the output component's platform drivers get loaded, probes for
such drivers immediately run as they match things in the device tree.
At this point, there is no drm_device available for the outputs to
bind to, so instead, references to these platform devices just get
saved in some global structures.

Later, when the drm_driver gets registered and loaded, the global
structures are consulted to find all of the output devices at the
right moment.


exynos seems to take a the same approach. Components are separate in
the device tree, and each component is implemented as a platform
driver or i2c driver. However all the drivers are built together in
the same module, and the module_init sequence is careful to initialise
all of the output component drivers before loading the DRM driver. The
output component driver store their findings in global structures.



Russell King suggested an alternative design for armada_drm. If all
display components are represented within the same "display"
super-node, we can examine the DT during drm_device initialisation and
initialise the appropriate output components. In this case, the output
components would not be registered as platform drivers.

The end result would be something similar to exynos/tilcdc (i.e.
drm_driver figuring out its output in the appropriate moment), however
the hackyness of using global storage to store output devices before
drm_driver is ready is avoided. And there is the obvious difference in
devicetree layout, which would look something like:

display {
  compatible = "marvell,armada-510-display";
  clocks = <_clk0>, <_pll_clk>;
  lcd0 {
compatible = "marvell,armada-510-lcd";
reg = <0xf182 0x1000>;
interrupts = <47>;
  };
  lcd1 {
compatible = "marvell,armada-510-lcd";
reg = <0xf181 0x1000>;
interrupts = <46>;
  };
  dcon {
compatible = "marvell,armada-510-dcon";
reg = <...>;
  };
};

Any thoughts/comments?

Thanks
Daniel

armada_drm clock selection - try 2

2013-07-01 Thread Daniel Drake

On Mon, Jul 1, 2013 at 3:48 PM, Sebastian Hesselbarth
 wrote:
> I guess "extclk0" and "extclk1" should be sufficient for clock names.
> Also, they are not dedicated as you can have CRTC0 and CRTC1 use e.g.
> extclk0 simultaneously. See below for .is_dedicated in general.

Maybe we can find better terminology, or just document it a bit
better, but having two CRTCs share the same clock falls within the
scheme designed and implemented there. "Dedicated" simply means a
clock that is dedicated to the display controller, it is not shared by
other devices.

>> +struct armada_clk_info {
>> +   struct clk *clk;
>> +
>> +   /* If this clock is dedicated to us, we can change its rate
>> without
>> +* worrying about any other users in other parts of the system. */
>> +   bool is_dedicated;
>> +
>> +   /* However, we cannot share the same dedicated clock between two
>> CRTCs
>> +* if each CRTC wants a different rate. Track the number of users.
>> */
>> +   int usage_count;
>
>
> You can share the same clock between two CRTCs. Just consider CRTC1
> using a mode with half pixel clock as CRTC0. Not that I think this will
> ever happen, but it is possible.

And my implementation already lets that happen - its just that I
didn't document it in enough detail.

> I prefer not to try to find the best clock (source) at all. Let the
> user pass the clock name by e.g. platform_data (or DT) and just try to
> get the requested pixclk or a integer multiple of it. You will never be
> able to find the best clock for all use cases.
>
> Just figure out, if integer division is sufficient to get requested
> pixclk and clk_set_rate otherwise. This may be useful for current mode
> running at 148.5MHz (1080p50) and new mode at 74.25MHz (1080i50, 720p).

I am not opposed to this approach, it is nice and simple, but as
Russell points out we do additionally need to distinguish between
clocks that are "ours" to play with, vs those that are shared with
other devices. It would be a bad idea to try call clk_set_rate() on
the AXI bus clock, for example, changing the clock for a whole bunch
of other devices. This is what the is_dedicated flag is about. However
such a flag could be easily added to the DT/platform data definition
that you propose.

Daniel

armada_drm clock selection - try 2

2013-07-01 Thread Daniel Drake

Hi Russell,

Here is a new patch which should incorporate all your previous feedback.
Now each variant passes clock info to the main driver via a new
armada_clk_info structure.

A helper function in the core lets each variant find the best clock.
As you suggested we first try external ("dedicated") clocks, where we can
change the rate to find an exact match. We fall back on looking at the rates
of the other clocks and we return the clock that provides us with the closest
match after integer division (rejecting those that don't bring us within 1%).

There is still the possibility that two CRTCs on the same device end up using
the same clock, so I added a usage counter to each clock to prevent the rate
from being changed by another CRTC.

This is compile-tested only - but after your feedback I will add the
remaining required hacks and test it on XO.

diff --git a/drivers/gpu/drm/armada/armada_510.c 
b/drivers/gpu/drm/armada/armada_510.c
index a016888..7dff2dc 100644
--- a/drivers/gpu/drm/armada/armada_510.c
+++ b/drivers/gpu/drm/armada/armada_510.c
@@ -17,12 +17,29 @@

 static int armada510_init(struct armada_private *priv, struct device *dev)
 {
-   priv->extclk[0] = devm_clk_get(dev, "ext_ref_clk_1");
+   struct armada_clk_info *clk_info = devm_kzalloc(dev,
+   sizeof(struct armada_clk_info) * 4, GFP_KERNEL);

-   if (IS_ERR(priv->extclk[0]) && PTR_ERR(priv->extclk[0]) == -ENOENT)
-   priv->extclk[0] = ERR_PTR(-EPROBE_DEFER);
+   if (!clk_info)
+   return -ENOMEM;

-   return PTR_RET(priv->extclk[0]);
+   /* External clocks */
+   clk_info[0].clk = devm_clk_get(dev, "ext_ref_clk_0");
+   clk_info[0].is_dedicated = true;
+   clk_info[0].sclk = SCLK_510_EXTCLK0;
+   clk_info[1].clk = devm_clk_get(dev, "ext_ref_clk_1");
+   clk_info[1].is_dedicated = true;
+   clk_info[1].sclk = SCLK_510_EXTCLK1;
+
+   /* Internal/shared clocks */
+   clk_info[2].clk = devm_clk_get(dev, "axi");
+   clk_info[2].sclk = SCLK_510_AXI;
+   clk_info[3].clk = devm_clk_get(dev, "pll");
+   clk_info[3].sclk = SCLK_510_PLL;
+
+   priv->num_clks = 4;
+   priv->clk_info = clk_info;
+   return 0;
 }

 static int armada510_crtc_init(struct armada_crtc *dcrtc)
@@ -38,42 +55,25 @@ static int armada510_crtc_init(struct armada_crtc *dcrtc)
  * supportable, and again with sclk != NULL to set the clocks up for
  * that.  The former can return an error, but the latter is expected
  * not to.
- *
- * We currently are pretty rudimentary here, always selecting
- * EXT_REF_CLK_1 for LCD0 and erroring LCD1.  This needs improvement!
  */
 static int armada510_crtc_compute_clock(struct armada_crtc *dcrtc,
const struct drm_display_mode *mode, uint32_t *sclk)
 {
struct armada_private *priv = dcrtc->crtc.dev->dev_private;
-   struct clk *clk = priv->extclk[0];
-   int ret;
-
-   if (dcrtc->num == 1)
-   return -EINVAL;
-
-   if (IS_ERR(clk))
-   return PTR_ERR(clk);
-
-   if (dcrtc->clk != clk) {
-   ret = clk_prepare_enable(clk);
-   if (ret)
-   return ret;
-   dcrtc->clk = clk;
-   }
+   struct armada_clk_info *clk_info;
+   int err;
+   int divider;

-   if (sclk) {
-   uint32_t rate, ref, div;
+   clk_info = armada_drm_find_best_clock(priv, mode->clock * 1000, 
);
+   if (!clk_info)
+   return -ENOENT;

-   rate = mode->clock * 1000;
-   ref = clk_round_rate(clk, rate);
-   div = DIV_ROUND_UP(ref, rate);
-   if (div < 1)
-   div = 1;
+   err = armada_drm_crtc_switch_clock(dcrtc, clk_info);
+   if (err)
+   return err;

-   clk_set_rate(clk, ref);
-   *sclk = div | SCLK_510_EXTCLK1;
-   }
+   if (sclk)
+   *sclk = divider | clk_info->sclk;

return 0;
 }
diff --git a/drivers/gpu/drm/armada/armada_crtc.c 
b/drivers/gpu/drm/armada/armada_crtc.c
index f489157..8202be2 100644
--- a/drivers/gpu/drm/armada/armada_crtc.c
+++ b/drivers/gpu/drm/armada/armada_crtc.c
@@ -83,6 +83,34 @@ enum csc_mode {
  * porch, which we're reprogramming.)
  */

+/* Switch to a new clock source after disabling and unpreparing the current
+ * one. If non-NULL, the new clock source is expected to be prepared before
+ * this function is called. */
+int armada_drm_crtc_switch_clock(struct armada_crtc *dcrtc,
+   struct armada_clk_info *clk_info)
+{
+   int err;
+
+   if (dcrtc->active_clk == clk_info)
+   return 0;
+
+   if (dcrtc->active_clk) {
+   clk_disable_unprepare(dcrtc->active_clk->clk);
+   dcrtc->active_clk->usage_count--;
+   dcrtc->active_clk = NULL;
+   }
+
+   if (clk_info) {
+   err = clk_enable(clk_info->clk);
+   if (err)
+   return err;

armada_drm clock selection

2013-06-29 Thread Daniel Drake

On Sat, Jun 29, 2013 at 1:26 PM, Russell King - ARM Linux
 wrote:
> So, I'd suggest that an initial approach would be something along the
> lines of:
> - if there is an external clock, can it generate the desired rate?
>   if yes, use it.
> - otherwise, get the clock rate from the internal clocks and calculate
>   the appropriate divider.  Use which ever one gives you the best
>   approximation that's better than (eg) 1%.  If not, fail the mode set.

This sounds sane to me.

According to your earlier mail, armada 510 is the only current target
platform that has external clocks. For the fallback on internal
clocks, we would not try to change the rate of any of them, we would
just look at how close they can bring us to what is needed, as you
describe.

If any clocks are broken or useless on a specific platform, then they
can be excluded from the DT or platform data. So this is really not
far off from the ideas from Sebastian and me where we would simply
pass in the information about the "interesting" clocks and be done
with it.

I agree that we need the DT have a way of not only providing the
clock, but also indicating which clock in the SCLK register it refers
to, and I also think the way to do that is by having a fixed,
documented clock naming scheme. So that should not be a problem. I'll
avoid putting register values in the DT.

I think that resolves all the open questions that I had, so I will
work on this early next week.

> This now brings me on to another important subject with DT vs DRM.
> The way DRM is setup, it expects all resources for a particular
> implementation to be known at 'drm_device' creation time.  You can't
> "hot-plug" additional parts of the "drm system" together.  What this
> means is that at driver load time (to use DRM terms) all parts must
> be known.

Its unfortunate that we can't hotplug the DRM bits, but at least that
is no longer an open question.

The super-device approach sounds sane and would seem to reflect on
other parts of the DT that I have worked with. It seems reasonable to
take the viewpoint that the LCD is a child connected to the display
controller parent, which is what the DT layout suggests.

I wonder what other DRM drivers do DT-wise? Maybe I will look into
that after we've progressed with the clocks.

Daniel

armada_drm clock selection

2013-06-29 Thread Daniel Drake

Hi,

Thanks for all the clear comments and explanations - I'll address all
of that in the next patch.

On Fri, Jun 28, 2013 at 3:18 PM, Russell King - ARM Linux
 wrote:
> Sure... lets add some background info first: the big problem here is the
> completely different register layouts for the clock register:
>
> On Armada 510:
> 31:30 - select the clock input from 4 possible sources:
> 0 - AXI, 1 - EXT0, 2 - PLL, 3 - EXT1
> 29- loads the divider values
> 27:16 - (broken) fractional divider - you never want to use this
> 15:0  - integer divider
>
> Armada 16x:
> 31:28 - select clock input from 4 possible sources using bit patterns.
> 0 - AHB, 1 - PCLK, 2 - AXI, 3 - PLL
> 27:16 - (broken) fractional divider
> 15:0  - integer divider
>
> From drivers/video/mmp driver (MMP2/MMP3, aka Armada 610?):
> 31- clock select
> 28- disable bit
> 27:16 - fractional divider
> 15:12 - apparantly not used
> 11:8  - DSI divider
> 7:0   - integer divider

MMP2 (Armada 610) and MMP3 (PXA2128, no Armada name) is even a bit
more complex than that.
On MMP2 the selectable clocks are written in bits 31:30 and are:
0 - AXI, 1 - LCD1, 2 - LCD2, 3 - HDMI

On MMP3 the selectable clocks are written in bits 31:29 and are:
0 - AXI or LVDS (depending on bit 12)
1 - LCD1
2 - LCD2
3 - HDMI
7 - DSI

When clock 0 is selected, bit 12 comes into effect for the final say
on the clock selection. 0 = AXI, 1 = LVDS

As you note, handling all this stuff is not trivial. And I also
understand the complications that we do not want to change some
clocks, if they are in use by another CRTC or are shared with other
parts of the system.

With your clear explanations I think I can come up with an
implementation that takes all these factors into account, offering
access to a great deal of the available functionality. Should not be
too difficult now that we have had this discussion, and I'd be happy
to do so. But before I do that, a question popped up into my mind: is
this complexity really worth it?

For OLPC, we only ever use LCD1 with divider 2 for our panel, which
only has 1 resolution. I think we use the fast AXI clock for HDMI
path, which has a separate SCLK register. For armada 510, it looks
like you are quite happy with the EXT1 clock, and you mentioned that
on the 16x most of the clocks are broken, which might mean there is
again just one clock of preference?

So, could we just specify per-CRTC clock preference in platform data,
DT, or something like that? e.g. we could just specify which SCLK bits
to set and clear, and which Linux-level struct clk is engaged as a
result. It adds a little burden for the platform implementor, but it
would avoid all of the complications that you have mentioned. Or do
you have a preference for the added flexibility?

Thanks
Daniel

Re: Armada DRM driver on OLPC XO

2013-06-29 Thread Daniel Drake

On Fri, Jun 28, 2013 at 3:27 PM, Russell King - ARM Linux
li...@arm.linux.org.uk wrote:
 On Tue, Jun 25, 2013 at 04:47:26PM -0400, Daniel Drake wrote:
 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.

 BTW... a point on this.  As you don't have the LCD_SPU_ADV_REG register,
 you probably don't have support for ARGB cursors.  DRM only supports ARGB
 cursors.  You can't down-convert an ARGB cursor to something which looks
 reasonable in the kernel, so I'd suggest forgetting hardware cursors.
 Even converting ARGB to transparency + RGB looks rubbish and wrong.

Interesting. Yes, a previous developer battled unsuccessfully with
hardware cursors and in the end we ended up using low color depth ones
which don't look great. I was wondering if you had found something
new, but it sounds like that we really are limited by the hardware.

Thanks
Daniel
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: armada_drm clock selection

2013-06-29 Thread Daniel Drake

Hi,

Thanks for all the clear comments and explanations - I'll address all
of that in the next patch.

On Fri, Jun 28, 2013 at 3:18 PM, Russell King - ARM Linux
li...@arm.linux.org.uk wrote:
 Sure... lets add some background info first: the big problem here is the
 completely different register layouts for the clock register:

 On Armada 510:
 31:30 - select the clock input from 4 possible sources:
 0 - AXI, 1 - EXT0, 2 - PLL, 3 - EXT1
 29- loads the divider values
 27:16 - (broken) fractional divider - you never want to use this
 15:0  - integer divider

 Armada 16x:
 31:28 - select clock input from 4 possible sources using bit patterns.
 0 - AHB, 1 - PCLK, 2 - AXI, 3 - PLL
 27:16 - (broken) fractional divider
 15:0  - integer divider

 From drivers/video/mmp driver (MMP2/MMP3, aka Armada 610?):
 31- clock select
 28- disable bit
 27:16 - fractional divider
 15:12 - apparantly not used
 11:8  - DSI divider
 7:0   - integer divider

MMP2 (Armada 610) and MMP3 (PXA2128, no Armada name) is even a bit
more complex than that.
On MMP2 the selectable clocks are written in bits 31:30 and are:
0 - AXI, 1 - LCD1, 2 - LCD2, 3 - HDMI

On MMP3 the selectable clocks are written in bits 31:29 and are:
0 - AXI or LVDS (depending on bit 12)
1 - LCD1
2 - LCD2
3 - HDMI
7 - DSI

When clock 0 is selected, bit 12 comes into effect for the final say
on the clock selection. 0 = AXI, 1 = LVDS

As you note, handling all this stuff is not trivial. And I also
understand the complications that we do not want to change some
clocks, if they are in use by another CRTC or are shared with other
parts of the system.

With your clear explanations I think I can come up with an
implementation that takes all these factors into account, offering
access to a great deal of the available functionality. Should not be
too difficult now that we have had this discussion, and I'd be happy
to do so. But before I do that, a question popped up into my mind: is
this complexity really worth it?

For OLPC, we only ever use LCD1 with divider 2 for our panel, which
only has 1 resolution. I think we use the fast AXI clock for HDMI
path, which has a separate SCLK register. For armada 510, it looks
like you are quite happy with the EXT1 clock, and you mentioned that
on the 16x most of the clocks are broken, which might mean there is
again just one clock of preference?

So, could we just specify per-CRTC clock preference in platform data,
DT, or something like that? e.g. we could just specify which SCLK bits
to set and clear, and which Linux-level struct clk is engaged as a
result. It adds a little burden for the platform implementor, but it
would avoid all of the complications that you have mentioned. Or do
you have a preference for the added flexibility?

Thanks
Daniel
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: armada_drm clock selection

2013-06-29 Thread Daniel Drake

On Sat, Jun 29, 2013 at 1:26 PM, Russell King - ARM Linux
li...@arm.linux.org.uk wrote:
 So, I'd suggest that an initial approach would be something along the
 lines of:
 - if there is an external clock, can it generate the desired rate?
   if yes, use it.
 - otherwise, get the clock rate from the internal clocks and calculate
   the appropriate divider.  Use which ever one gives you the best
   approximation that's better than (eg) 1%.  If not, fail the mode set.

This sounds sane to me.

According to your earlier mail, armada 510 is the only current target
platform that has external clocks. For the fallback on internal
clocks, we would not try to change the rate of any of them, we would
just look at how close they can bring us to what is needed, as you
describe.

If any clocks are broken or useless on a specific platform, then they
can be excluded from the DT or platform data. So this is really not
far off from the ideas from Sebastian and me where we would simply
pass in the information about the interesting clocks and be done
with it.

I agree that we need the DT have a way of not only providing the
clock, but also indicating which clock in the SCLK register it refers
to, and I also think the way to do that is by having a fixed,
documented clock naming scheme. So that should not be a problem. I'll
avoid putting register values in the DT.

I think that resolves all the open questions that I had, so I will
work on this early next week.

 This now brings me on to another important subject with DT vs DRM.
 The way DRM is setup, it expects all resources for a particular
 implementation to be known at 'drm_device' creation time.  You can't
 hot-plug additional parts of the drm system together.  What this
 means is that at driver load time (to use DRM terms) all parts must
 be known.

Its unfortunate that we can't hotplug the DRM bits, but at least that
is no longer an open question.

The super-device approach sounds sane and would seem to reflect on
other parts of the DT that I have worked with. It seems reasonable to
take the viewpoint that the LCD is a child connected to the display
controller parent, which is what the DT layout suggests.

I wonder what other DRM drivers do DT-wise? Maybe I will look into
that after we've progressed with the clocks.

Daniel
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

armada_drm clock selection

2013-06-28 Thread Daniel Drake

Hi Russell,

Thanks for pointing me to the most recent version of your driver.
Can you comment on the below patch for improved clock handling?
It is based on the approach in Jean-Francois Moine's driver, and would
serve for the basis of having clock info come from the DT. If you have
something else in mind, maybe you can explain quickly, and I'll try to
implement it.

armada_priv now has a clk array, the indice of each member of the array
corresponds to the id in the SCLK register (so extclk1 goes at clk[3]).
When we need to set up SCLK for a CRTC, we try to find a clock that can
meet the requested rate exactly. If we don't find one, we fall back on the
first clock in the array.

armada510_compute_crtc_clock had a fair amount of stuff that is in danger
of being repeated in a MMP2-equivalent function, and then again for MMP3.
So I took out the clock-hunting code and made that into a function that
would be shared by the 3 platforms.

devm can't be used for clocks as for DT we will use of_clk_get() with an index
and there is no devm equivalent.

Comments appreciated. Compile tested only.
---
 drivers/gpu/drm/armada/armada_crtc.c |  31 ---
 drivers/gpu/drm/armada/armada_drm.h  |  15 +++--
 drivers/gpu/drm/armada/armada_drv.c  | 104 ---
 drivers/gpu/drm/armada/armada_hw.h   |   9 +--
 4 files changed, 100 insertions(+), 59 deletions(-)

diff --git a/drivers/gpu/drm/armada/armada_crtc.c 
b/drivers/gpu/drm/armada/armada_crtc.c
index dadaf63..9705466 100644
--- a/drivers/gpu/drm/armada/armada_crtc.c
+++ b/drivers/gpu/drm/armada/armada_crtc.c
@@ -413,16 +413,31 @@ static int armada_drm_crtc_mode_set(struct drm_crtc *crtc,
struct armada_private *priv = crtc->dev->dev_private;
struct armada_crtc *dcrtc = drm_to_armada_crtc(crtc);
struct armada_regs regs[16];
+   struct clk *clk;
uint32_t lm, rm, tm, bm, val, sclk;
unsigned long flags;
unsigned i;
bool interlaced;
+   int clk_idx;
int ret;

-   /* First, check that this will succeed. */
-   ret = priv->ops->compute_crtc_clock(dcrtc, adj, NULL);
-   if (ret)
-   return ret;
+   /* First, check that a suitable clock is available. */
+   clk_idx = armada_drm_find_best_clock(priv, adj->clock * 1000);
+   if (clk_idx < 0)
+   return clk_idx;
+
+   clk = priv->clk[clk_idx];
+   if (dcrtc->clk != clk) {
+   if (dcrtc->clk) {
+   clk_disable_unprepare(clk);
+   dcrtc->clk = NULL;
+   }
+
+   ret = clk_prepare_enable(clk);
+   if (ret)
+   return ret;
+   dcrtc->clk = clk;
+   }

drm_framebuffer_reference(crtc->fb);

@@ -459,8 +474,8 @@ static int armada_drm_crtc_mode_set(struct drm_crtc *crtc,
writel_relaxed(val, dcrtc->base + LCD_SPU_DUMB_CTRL);
}

-   /* Now compute the divider for real */
-   priv->ops->compute_crtc_clock(dcrtc, adj, );
+   /* Now compute the divider */
+   sclk = priv->ops->compute_crtc_clock(dcrtc, adj->clock * 1000, clk_idx);

armada_reg_queue_set(regs, i, sclk, LCD_CFG_SCLK_DIV);

@@ -634,7 +649,7 @@ static void armada_drm_crtc_destroy(struct drm_crtc *crtc)
priv->dcrtc[dcrtc->num] = NULL;
drm_crtc_cleanup(>crtc);

-   if (!IS_ERR(dcrtc->clk))
+   if (dcrtc->clk)
clk_disable_unprepare(dcrtc->clk);

kfree(dcrtc);
@@ -734,7 +749,7 @@ int armada_drm_crtc_create(struct drm_device *dev, unsigned 
num,

dcrtc->base = base;
dcrtc->num = num;
-   dcrtc->clk = ERR_PTR(-EINVAL);
+   dcrtc->clk = NULL;
dcrtc->cfg_dma_ctrl0 = CFG_GRA_ENA | CFG_GRA_HSMOOTH;
dcrtc->cfg_dumb_ctrl = DUMB24_RGB888_0;
spin_lock_init(>irq_lock);
diff --git a/drivers/gpu/drm/armada/armada_drm.h 
b/drivers/gpu/drm/armada/armada_drm.h
index c2dcde5..b41d77c 100644
--- a/drivers/gpu/drm/armada/armada_drm.h
+++ b/drivers/gpu/drm/armada/armada_drm.h
@@ -41,18 +41,23 @@ struct armada_private;

 struct armada_ops {
int (*init)(struct armada_private *, struct device *);
-   int (*compute_crtc_clock)(struct armada_crtc *,
- const struct drm_display_mode *,
- uint32_t *);
+   uint32_t (*compute_crtc_clock)(struct armada_crtc *,
+  long rate, int clk_idx);
 };

+#define ARMADA_MAX_CLOCK 4
+
 struct armada_private {
const struct armada_ops *ops;
struct drm_fb_helper*fbdev;
struct armada_crtc  *dcrtc[2];
struct armada_overlay   *overlay;
struct drm_mm   linear;
-   struct clk  *extclk[2];
+
+   /* The index of clocks in this array line up with their ID
+* into the SCLK clock selection register. */
+   struct clk  *clk[ARMADA_MAX_CLOCK];
+
 #ifdef

Armada DRM driver on OLPC XO

2013-06-28 Thread Daniel Drake

On Fri, Jun 28, 2013 at 3:27 PM, Russell King - ARM Linux
 wrote:
> On Tue, Jun 25, 2013 at 04:47:26PM -0400, Daniel Drake wrote:
>> 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.
>
> BTW... a point on this.  As you don't have the LCD_SPU_ADV_REG register,
> you probably don't have support for ARGB cursors.  DRM only supports ARGB
> cursors.  You can't down-convert an ARGB cursor to something which looks
> reasonable in the kernel, so I'd suggest forgetting hardware cursors.
> Even converting ARGB to transparency + RGB looks rubbish and wrong.

Interesting. Yes, a previous developer battled unsuccessfully with
hardware cursors and in the end we ended up using low color depth ones
which don't look great. I was wondering if you had found something
new, but it sounds like that we really are limited by the hardware.

Thanks
Daniel

Armada DRM driver on OLPC XO

2013-06-28 Thread Daniel Drake

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.

Daniel

Armada DRM driver on OLPC XO

2013-06-26 Thread Daniel Drake

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.

A hacky patch is also included below, which makes the driver run on this
platform. I'm prepared to do the heavy lifting in implementing these changes
properly, but any high level guidance would be appreciated, especially as I
am new to the world of graphics.

Ordered roughly from highest to lowest importance:


1. Device tree support
The OLPC XO boots entirely from the device tree, all clocks and things are
defined there. Your current display controller driver is close to
DT-compatibility, the only tricky bit is:
Â  Â  Â  Â  Â  Â  Â  Â clk = clk_get_sys(dovefb.0, extclk);

Not sure how that would translate to DT, or if we can transform that into
something that works for both DT and platform devices. The norm in DT is
that a clock is associated to a specific device, so we could just pull it
off the platform device itself.


2. Panel support.
From my reading of your patches, on the cubox you drive the hardware as if it
is connected to a panel, but actually it is connected to an encoder chip which
outputs a HDMI signal?
In the OLPC case, we actually have a dumb panel connected to the panel
interface, so we need some driver support there.

The panel definition should come from the device tree, but I already hit a
small headache there. The display controller driver (armada_drv) gets probed
before my panel driver, and armada_drm_load() is not happy if it completes
without a connector/encoder registered. We will either have to force a probe
order somehow, or make the driver be happy to be loaded without a
connector/encoder which would then appear later.


3. Register space conflicts
Already found a couple of register conflicts between your dove and the MMP
platforms. Your LCD_SPU_ADV_REG is something completely different here.

The high bits of the DMA_CTRL0 register is used to select a clock. Â In the
dove and MMP2 case these bits are 31:30 but on MMP3 this is 31:29. Also, OLPC
uses this field to select the LCD clock as a clock source, but your driver
chooses another clock for cubox. So we need ways to represent all of these
differences.


4. Video memory
The driver at the moment requires an area of RAM as video memory, but this
must actually be memory that Linux does not regard as normal available RAM,
since ioremap() is called on it. I guess in your platform code you look at
how much RAM is available and cut out a chunk for graphics memory. Then when
communicating to the MM core how much RAM is available, you do not tell it
about the graphics memory?

I realise I'm talking to the ARM MM guru here, but... can we do better? The
decision to have to cut out the memory as above would have to be made during
early boot, before we know if we even have a graphics driver to come along and
make use of that memory. In my case I have similarly hacked our firmware to do
the cut out operation when finalizing the DT before booting the kernel.

I would have hoped in a world with CMA and things like that we could now do a
bit better. I tried creating a coherent DMA allocation in armada_drm_load() to
be used as video memory, but this failed later when armada_gem_linear_back()
tried to ioremap it (you probably don't need reminding that ioremap on memory
otherwise available as RAM fails, http://lwn.net/Articles/409689/).

I realise that I can avoid that particular ioremap since we already have the
virtual address available, but I am left wondering how this memory is
accessed by DRM/GEM in other contexts (e.g. when it wants to write image data
in there). If that uses ioremap() as well, then we are in trouble.



5. Output paths

This is something we'll have to address for HDMI output support on OLPC, which
is the lowest priority item on this list, lets get the inbuilt panel going
first!

The way I read your code is that up to 2 CRTC addresses can be defined in
platform data. Each one then gets passed to armada_drm_crtc_create() and the
address is used as a base for register accesses.

Does that mean that the register list in the big armada_hw.h enum is
essentially duplicated exactly? Almost as if the system has 2 separate display
controllers?

Your register list essentially starts at offset 0xc0. What can be found at
offsets below that address?

On MMP2/MMP3 the situation is a bit different. 3 output paths are supported
- two panel paths, and one TV path (which is HDMI - direct output from the
SoC, no separate encoder chip necessary).

These paths are closely related, probably not as far

Armada DRM driver on OLPC XO

2013-06-25 Thread Daniel Drake

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.

A hacky patch is also included below, which makes the driver run on this
platform. I'm prepared to do the heavy lifting in implementing these changes
properly, but any high level guidance would be appreciated, especially as I
am new to the world of graphics.

Ordered roughly from highest to lowest importance:


1. Device tree support
The OLPC XO boots entirely from the device tree, all clocks and things are
defined there. Your current display controller driver is close to
DT-compatibility, the only tricky bit is:
?? ?? ?? ?? ?? ?? ?? ??clk = clk_get_sys("dovefb.0", "extclk");

Not sure how that would translate to DT, or if we can transform that into
something that works for both DT and platform devices. The norm in DT is
that a clock is associated to a specific device, so we could just pull it
off the platform device itself.


2. Panel support.
>From my reading of your patches, on the cubox you drive the hardware as if it
is connected to a panel, but actually it is connected to an encoder chip which
outputs a HDMI signal?
In the OLPC case, we actually have a dumb panel connected to the panel
interface, so we need some driver support there.

The panel definition should come from the device tree, but I already hit a
small headache there. The display controller driver (armada_drv) gets probed
before my panel driver, and armada_drm_load() is not happy if it completes
without a connector/encoder registered. We will either have to force a probe
order somehow, or make the driver be happy to be loaded without a
connector/encoder which would then appear later.


3. Register space conflicts
Already found a couple of register conflicts between your dove and the MMP
platforms. Your LCD_SPU_ADV_REG is something completely different here.

The high bits of the DMA_CTRL0 register is used to select a clock. ??In the
dove and MMP2 case these bits are 31:30 but on MMP3 this is 31:29. Also, OLPC
uses this field to select the LCD clock as a clock source, but your driver
chooses another clock for cubox. So we need ways to represent all of these
differences.


4. Video memory
The driver at the moment requires an area of RAM as video memory, but this
must actually be memory that Linux does not regard as normal available RAM,
since ioremap() is called on it. I guess in your platform code you look at
how much RAM is available and cut out a chunk for graphics memory. Then when
communicating to the MM core how much RAM is available, you do not tell it
about the graphics memory?

I realise I'm talking to the ARM MM guru here, but... can we do better? The
decision to have to "cut out" the memory as above would have to be made during
early boot, before we know if we even have a graphics driver to come along and
make use of that memory. In my case I have similarly hacked our firmware to do
the "cut out" operation when finalizing the DT before booting the kernel.

I would have hoped in a world with CMA and things like that we could now do a
bit better. I tried creating a coherent DMA allocation in armada_drm_load() to
be used as video memory, but this failed later when armada_gem_linear_back()
tried to ioremap it (you probably don't need reminding that ioremap on memory
otherwise available as RAM fails, http://lwn.net/Articles/409689/).

I realise that I can avoid that particular ioremap since we already have the
virtual address available, but I am left wondering how this memory is
accessed by DRM/GEM in other contexts (e.g. when it wants to write image data
in there). If that uses ioremap() as well, then we are in trouble.



5. Output paths

This is something we'll have to address for HDMI output support on OLPC, which
is the lowest priority item on this list, lets get the inbuilt panel going
first!

The way I read your code is that up to 2 CRTC addresses can be defined in
platform data. Each one then gets passed to armada_drm_crtc_create() and the
address is used as a base for register accesses.

Does that mean that the register list in the big armada_hw.h enum is
essentially duplicated exactly? Almost as if the system has 2 separate display
controllers?

Your register list essentially starts at offset 0xc0. What can be found at
offsets below that address?

On MMP2/MMP3 the situation is a bit different. 3 output paths are supported
- two panel paths, and one TV path (which is HDMI - direct output from the
SoC, no separate encoder chip necessary).

These paths are closely related, probably

67 matches

Mail list logo