Re: [PATCH v10 0/4] drm/panfrost: Add support for mt8183 GPU

2021-01-12 Thread Tomeu Vizoso

On 1/13/21 7:06 AM, Nicolas Boichat wrote:

Hi!

Follow-up on the v5 [1], things have gotten significantly
better in the last 9 months, thanks to the efforts on Bifrost
support by the Collabora team (and probably others I'm not
aware of).

I've been testing this series on a MT8183/kukui device, with a
chromeos-5.10 kernel [2], and got basic Chromium OS UI up with
mesa 20.3.2 (lots of artifacts though).


Btw, don't know if you plan to retest with a newer Mesa, but a recent 
master should have pretty good ES 3.0 compliance on the Duet.


Cheers,

Tomeu


devfreq is currently not supported, as we'll need:
  - Clock core support for switching the GPU core clock (see 2/4).
  - Platform-specific handling of the 2-regulator (see 3/4).

Since the latter is easy to detect, patch 3/4 just disables
devfreq if the more than one regulator is specified in the
compatible matching table.

[1] 
https://patchwork.kernel.org/project/linux-mediatek/cover/20200306041345.259332-1-drink...@chromium.org/
[2] https://crrev.com/c/2608070

Changes in v10:
  - Fix the binding to make sure sram-supply property can be provided.

Changes in v9:
  - Explain why devfreq needs to be disabled for GPUs with >1
regulators.

Changes in v8:
  - Use DRM_DEV_INFO instead of ERROR

Changes in v7:
  - Fix GPU ID in commit message
  - Fix GPU ID in commit message

Changes in v6:
  - Rebased, actually tested with recent mesa driver.

Nicolas Boichat (4):
   dt-bindings: gpu: mali-bifrost: Add Mediatek MT8183
   arm64: dts: mt8183: Add node for the Mali GPU
   drm/panfrost: devfreq: Disable devfreq when num_supplies > 1
   drm/panfrost: Add mt8183-mali compatible string

  .../bindings/gpu/arm,mali-bifrost.yaml|  28 +
  arch/arm64/boot/dts/mediatek/mt8183-evb.dts   |   6 +
  .../arm64/boot/dts/mediatek/mt8183-kukui.dtsi |   6 +
  arch/arm64/boot/dts/mediatek/mt8183.dtsi  | 105 ++
  drivers/gpu/drm/panfrost/panfrost_devfreq.c   |   9 ++
  drivers/gpu/drm/panfrost/panfrost_drv.c   |  10 ++
  6 files changed, 164 insertions(+)



Re: [PATCH v7 3/4] drm/panfrost: devfreq: Disable devfreq when num_supplies > 1

2021-01-07 Thread Tomeu Vizoso

On 1/7/21 9:49 AM, Nicolas Boichat wrote:

On Thu, Jan 7, 2021 at 4:31 PM Tomeu Vizoso  wrote:


On 1/7/21 9:26 AM, Nicolas Boichat wrote:

GPUs with more than a single regulator (e.g. G72 on MT8183) will
require platform-specific handling, disable devfreq for now.

Signed-off-by: Nicolas Boichat 
---

Changes in v7:
   - Fix GPU ID in commit message

Changes in v6:
   - New change

   drivers/gpu/drm/panfrost/panfrost_devfreq.c | 9 +
   1 file changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c 
b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
index f44d28fad085..1f49043aae73 100644
--- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
+++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
@@ -92,6 +92,15 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev)
   struct thermal_cooling_device *cooling;
   struct panfrost_devfreq *pfdevfreq = &pfdev->pfdevfreq;

+ if (pfdev->comp->num_supplies > 1) {
+ /*
+  * GPUs with more than 1 supply require platform-specific 
handling:
+  * continue without devfreq
+  */
+ DRM_DEV_ERROR(dev, "More than 1 supply is not supported yet\n");


Should this be info instead?


Sure, will fix in v8.


Thanks. With that change, patches 3 and 4 are R-b.

Thanks again!

Tomeu



Regards,

Tomeu


+ return 0;
+ }
+
   opp_table = dev_pm_opp_set_regulators(dev, pfdev->comp->supply_names,
 pfdev->comp->num_supplies);
   if (IS_ERR(opp_table)) {



Re: [PATCH v7 3/4] drm/panfrost: devfreq: Disable devfreq when num_supplies > 1

2021-01-07 Thread Tomeu Vizoso

On 1/7/21 9:26 AM, Nicolas Boichat wrote:

GPUs with more than a single regulator (e.g. G72 on MT8183) will
require platform-specific handling, disable devfreq for now.

Signed-off-by: Nicolas Boichat 
---

Changes in v7:
  - Fix GPU ID in commit message

Changes in v6:
  - New change

  drivers/gpu/drm/panfrost/panfrost_devfreq.c | 9 +
  1 file changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c 
b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
index f44d28fad085..1f49043aae73 100644
--- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
+++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
@@ -92,6 +92,15 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev)
struct thermal_cooling_device *cooling;
struct panfrost_devfreq *pfdevfreq = &pfdev->pfdevfreq;
  
+	if (pfdev->comp->num_supplies > 1) {

+   /*
+* GPUs with more than 1 supply require platform-specific 
handling:
+* continue without devfreq
+*/
+   DRM_DEV_ERROR(dev, "More than 1 supply is not supported yet\n");


Should this be info instead?

Regards,

Tomeu


+   return 0;
+   }
+
opp_table = dev_pm_opp_set_regulators(dev, pfdev->comp->supply_names,
  pfdev->comp->num_supplies);
if (IS_ERR(opp_table)) {



devfreq and panfrost on Allwinner H6

2020-10-07 Thread Tomeu Vizoso
Hi Clément,

Have just noticed that my Pine H64 board hangs when I try to set the
performance governor for the GPU devfreq.

Is this a known bug?

Thanks,

Tomeu


Re: [PATCH 2/2] panfrost: Add compatible string for bifrost

2020-10-05 Thread Tomeu Vizoso
On Mon, 5 Oct 2020 at 08:44, Tomeu Vizoso  wrote:
>
> On Fri, 19 Jun 2020 at 11:00, Steven Price  wrote:
> >
> > On 11/06/2020 09:58, Tomeu Vizoso wrote:
> > > Mesa now supports some Bifrost devices, so enable it.
> > >
> > > Signed-off-by: Tomeu Vizoso 
> >
> > Reviewed-by: Steven Price 
> >
> > I've also dug out my Hikey960 (from the box it's been in since lock down
> > started), so I'll see if I can get things running on there, at the
> > moment I'm seeing some DATA_INVALID_FAULT regressions running my hacked
> > DDK :(
>
> Hi!
>
> Has this one fallen through the cracks?

Oops, sorry about the noise, I had an old checkout.

Cheers,

Tomeu

> Thanks,
>
> Tomeu
>
> >
> > Steve
> >
> > > ---
> > >   drivers/gpu/drm/panfrost/panfrost_drv.c | 1 +
> > >   1 file changed, 1 insertion(+)
> > >
> > > diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c 
> > > b/drivers/gpu/drm/panfrost/panfrost_drv.c
> > > index 882fecc33fdb..8ff8e140f91e 100644
> > > --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> > > +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> > > @@ -677,6 +677,7 @@ static const struct of_device_id dt_match[] = {
> > >   { .compatible = "arm,mali-t830", .data = &default_data, },
> > >   { .compatible = "arm,mali-t860", .data = &default_data, },
> > >   { .compatible = "arm,mali-t880", .data = &default_data, },
> > > + { .compatible = "arm,mali-bifrost", .data = &default_data, },
> > >   {}
> > >   };
> > >   MODULE_DEVICE_TABLE(of, dt_match);
> > >
> >
> > ___
> > dri-devel mailing list
> > dri-de...@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel


Re: [PATCH 2/2] panfrost: Add compatible string for bifrost

2020-10-04 Thread Tomeu Vizoso
On Fri, 19 Jun 2020 at 11:00, Steven Price  wrote:
>
> On 11/06/2020 09:58, Tomeu Vizoso wrote:
> > Mesa now supports some Bifrost devices, so enable it.
> >
> > Signed-off-by: Tomeu Vizoso 
>
> Reviewed-by: Steven Price 
>
> I've also dug out my Hikey960 (from the box it's been in since lock down
> started), so I'll see if I can get things running on there, at the
> moment I'm seeing some DATA_INVALID_FAULT regressions running my hacked
> DDK :(

Hi!

Has this one fallen through the cracks?

Thanks,

Tomeu

>
> Steve
>
> > ---
> >   drivers/gpu/drm/panfrost/panfrost_drv.c | 1 +
> >   1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c 
> > b/drivers/gpu/drm/panfrost/panfrost_drv.c
> > index 882fecc33fdb..8ff8e140f91e 100644
> > --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> > +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> > @@ -677,6 +677,7 @@ static const struct of_device_id dt_match[] = {
> >   { .compatible = "arm,mali-t830", .data = &default_data, },
> >   { .compatible = "arm,mali-t860", .data = &default_data, },
> >   { .compatible = "arm,mali-t880", .data = &default_data, },
> > + { .compatible = "arm,mali-bifrost", .data = &default_data, },
> >   {}
> >   };
> >   MODULE_DEVICE_TABLE(of, dt_match);
> >
>
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel


[PATCH 1/2] panfrost: Make sure GPU is powered on when reading GPU_LATEST_FLUSH_ID

2020-06-11 Thread Tomeu Vizoso
Bifrost devices do support the flush reduction feature, so on first job
submit we were trying to read the register while still powered off.

If the GPU is powered off, the feature doesn't bring any benefit, so
don't try to read.

Signed-off-by: Tomeu Vizoso 
---
 drivers/gpu/drm/panfrost/panfrost_gpu.c | 14 --
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_gpu.c 
b/drivers/gpu/drm/panfrost/panfrost_gpu.c
index f2c1ddc41a9b..e0f190e43813 100644
--- a/drivers/gpu/drm/panfrost/panfrost_gpu.c
+++ b/drivers/gpu/drm/panfrost/panfrost_gpu.c
@@ -10,6 +10,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include "panfrost_device.h"
 #include "panfrost_features.h"
@@ -368,7 +369,16 @@ void panfrost_gpu_fini(struct panfrost_device *pfdev)
 
 u32 panfrost_gpu_get_latest_flush_id(struct panfrost_device *pfdev)
 {
-   if (panfrost_has_hw_feature(pfdev, HW_FEATURE_FLUSH_REDUCTION))
-   return gpu_read(pfdev, GPU_LATEST_FLUSH_ID);
+   u32 flush_id;
+
+   if (panfrost_has_hw_feature(pfdev, HW_FEATURE_FLUSH_REDUCTION)) {
+   /* Flush reduction only makes sense when the GPU is kept 
powered on between jobs */
+   if (pm_runtime_get_if_in_use(pfdev->dev)) {
+   flush_id = gpu_read(pfdev, GPU_LATEST_FLUSH_ID);
+   pm_runtime_put(pfdev->dev);
+   return flush_id;
+   }
+   }
+
return 0;
 }
-- 
2.21.0



[PATCH 2/2] panfrost: Add compatible string for bifrost

2020-06-11 Thread Tomeu Vizoso
Mesa now supports some Bifrost devices, so enable it.

Signed-off-by: Tomeu Vizoso 
---
 drivers/gpu/drm/panfrost/panfrost_drv.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c 
b/drivers/gpu/drm/panfrost/panfrost_drv.c
index 882fecc33fdb..8ff8e140f91e 100644
--- a/drivers/gpu/drm/panfrost/panfrost_drv.c
+++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
@@ -677,6 +677,7 @@ static const struct of_device_id dt_match[] = {
{ .compatible = "arm,mali-t830", .data = &default_data, },
{ .compatible = "arm,mali-t860", .data = &default_data, },
{ .compatible = "arm,mali-t880", .data = &default_data, },
+   { .compatible = "arm,mali-bifrost", .data = &default_data, },
{}
 };
 MODULE_DEVICE_TABLE(of, dt_match);
-- 
2.21.0



Re: [PATCH 08/15] drm/panfrost: move devfreq_init()/fini() in device

2020-06-08 Thread Tomeu Vizoso

On 5/29/20 2:38 PM, Clément Péron wrote:

Hi Steven

On Thu, 28 May 2020 at 15:22, Steven Price  wrote:


On 10/05/2020 17:55, Clément Péron wrote:

Later we will introduce devfreq probing regulator if they
are present. As regulator should be probe only one time we
need to get this logic in the device_init().

panfrost_device is already taking care of devfreq_resume()
and devfreq_suspend(), so it's not totally illogic to move
the devfreq_init() and devfreq_fini() here.

Signed-off-by: Clément Péron 
---
   drivers/gpu/drm/panfrost/panfrost_device.c | 37 ++
   drivers/gpu/drm/panfrost/panfrost_drv.c| 15 ++---
   2 files changed, 25 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c 
b/drivers/gpu/drm/panfrost/panfrost_device.c
index 8136babd3ba9..f480127205d6 100644
--- a/drivers/gpu/drm/panfrost/panfrost_device.c
+++ b/drivers/gpu/drm/panfrost/panfrost_device.c
@@ -212,59 +212,67 @@ int panfrost_device_init(struct panfrost_device *pfdev)
   return err;
   }

+ err = panfrost_devfreq_init(pfdev);
+ if (err) {
+ dev_err(pfdev->dev, "devfreq init failed %d\n", err);
+ goto err_out0;
+ }
+
   err = panfrost_regulator_init(pfdev);
   if (err) {
   dev_err(pfdev->dev, "regulator init failed %d\n", err);
- goto err_out0;
+ goto err_out1;


NIT: Rather than just renumbering these can we give them sensible names
so we don't have this sort of refactoring in future?


Agree, I will change that in v2


FWIW, there's a sensible approach described in 
Documentation/process/coding-style.rst ("7) Centralized exiting of 
functions").


Regards,

Tomeu






   }

   err = panfrost_reset_init(pfdev);
   if (err) {
   dev_err(pfdev->dev, "reset init failed %d\n", err);
- goto err_out1;
+ goto err_out2;
   }

   err = panfrost_pm_domain_init(pfdev);
   if (err)
- goto err_out2;
+ goto err_out3;

   res = platform_get_resource(pfdev->pdev, IORESOURCE_MEM, 0);
   pfdev->iomem = devm_ioremap_resource(pfdev->dev, res);
   if (IS_ERR(pfdev->iomem)) {
   dev_err(pfdev->dev, "failed to ioremap iomem\n");
   err = PTR_ERR(pfdev->iomem);
- goto err_out3;
+ goto err_out4;
   }

   err = panfrost_gpu_init(pfdev);
   if (err)
- goto err_out3;
+ goto err_out4;

   err = panfrost_mmu_init(pfdev);
   if (err)
- goto err_out4;
+ goto err_out5;

   err = panfrost_job_init(pfdev);
   if (err)
- goto err_out5;
+ goto err_out6;

   err = panfrost_perfcnt_init(pfdev);
   if (err)
- goto err_out6;
+ goto err_out7;

   return 0;
-err_out6:
+err_out7:
   panfrost_job_fini(pfdev);
-err_out5:
+err_out6:
   panfrost_mmu_fini(pfdev);
-err_out4:
+err_out5:
   panfrost_gpu_fini(pfdev);
-err_out3:
+err_out4:
   panfrost_pm_domain_fini(pfdev);
-err_out2:
+err_out3:
   panfrost_reset_fini(pfdev);
-err_out1:
+err_out2:
   panfrost_regulator_fini(pfdev);
+err_out1:
+ panfrost_devfreq_fini(pfdev);
   err_out0:
   panfrost_clk_fini(pfdev);
   return err;
@@ -278,6 +286,7 @@ void panfrost_device_fini(struct panfrost_device *pfdev)
   panfrost_gpu_fini(pfdev);
   panfrost_pm_domain_fini(pfdev);
   panfrost_reset_fini(pfdev);
+ panfrost_devfreq_fini(pfdev);
   panfrost_regulator_fini(pfdev);
   panfrost_clk_fini(pfdev);
   }
diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c 
b/drivers/gpu/drm/panfrost/panfrost_drv.c
index 882fecc33fdb..4dda68689015 100644
--- a/drivers/gpu/drm/panfrost/panfrost_drv.c
+++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
@@ -14,7 +14,6 @@
   #include 

   #include "panfrost_device.h"
-#include "panfrost_devfreq.h"
   #include "panfrost_gem.h"
   #include "panfrost_mmu.h"
   #include "panfrost_job.h"
@@ -606,13 +605,6 @@ static int panfrost_probe(struct platform_device *pdev)
   goto err_out0;
   }

- err = panfrost_devfreq_init(pfdev);
- if (err) {
- if (err != -EPROBE_DEFER)
- dev_err(&pdev->dev, "Fatal error during devfreq init\n");


You seem to have lost the check for EPROBE_DEFER during the move.


Correct, sorry for that, I will fix it in v2.

Thanks for your review,
Clement




- goto err_out1;
- }
-
   pm_runtime_set_active(pfdev->dev);
   pm_runtime_mark_last_busy(pfdev->dev);
   pm_runtime_enable(pfdev->dev);
@@ -625,16 +617,14 @@ static int panfrost_probe(struct platform_device *pdev)
*/
   err = drm_dev_register(ddev, 0);
   if (err < 0)
- goto err_out2;
+ goto err_out1;

   panfrost_gem_shrinker_init(ddev);

   return 0;

-err_out2:
- pm_runtime_disable(pfdev->dev);
- panfrost_devfreq_fini(pfdev

Re: [PATCH 00/15][RFC] Add regulator devfreq support to Panfrost

2020-05-10 Thread Tomeu Vizoso

On 5/10/20 6:55 PM, Clément Péron wrote:

Hi,

This serie cleans and adds regulator support to Panfrost devfreq.
This is mostly based on comment for the freshly introduced lima
devfreq.

We need to add regulator support because on Allwinner the GPU OPP
table defines both frequencies and voltages.

First patches [01-08] should not change the actual behavior
and introduce a proper panfrost_devfreq struct.

Fatches after are WIP and add regulator support.

However I got several issues first we need to avoid getting regulator
if devfreq get by itself the regulator, but as of today the OPP
framework only get and don't enable the regulator...
An HACK for now is to add regulator-always-on in the device-tree.

Then when I enable devfreq I got several faults like.
I'm totally noob on GPU sched/fault and couldn't be helpfull with this.


Do you know at which frequencies do the faults happen? From what I can 
see, it's just the GPU behaving erratically, and the CPU reading random 
values from the GPU registers. Given the subject of this series, I guess 
the GPU isn't getting enough power.


There could be a problem with the OPP table, might be a good idea to see 
what levels are problematic and try with a more conservative table.


Besides that, there could be a problem with clock frequency changes, or 
voltage changes. It may take some time for the final state to be stable, 
depending how the regulation happens.


Thanks,

Tomeu





I got this running glmark2 on T720 (Allwinner H6) with Mesa 20.0.5.
# glmark2-es2-drm
===
 glmark2 2017.07
===
 OpenGL Information
 GL_VENDOR: Panfrost
 GL_RENDERER:   Mali T720 (Panfrost)
 GL_VERSION:OpenGL ES 2.0 Mesa 20.0.5
===

[   93.550063] panfrost 180.gpu: GPU Fault 0x0088 (UNKNOWN) at 
0x80117100
[   94.045401] panfrost 180.gpu: gpu sched timeout, js=0, config=0x3700, 
status=0x8, head=0x21d6c00, tail=0x21d6c00, sched_job=e3c2132f

[  328.871070] panfrost 180.gpu: Unhandled Page fault in AS0 at VA 
0x
[  328.871070] Reason: TODO
[  328.871070] raw fault status: 0xAA0003C2
[  328.871070] decoded fault status: SLAVE FAULT
[  328.871070] exception type 0xC2: TRANSLATION_FAULT_LEVEL2
[  328.871070] access type 0x3: WRITE
[  328.871070] source id 0xAA00
[  329.373327] panfrost 180.gpu: gpu sched timeout, js=1, config=0x3700, 
status=0x8, head=0xa1a4900, tail=0xa1a4900, sched_job=7ac31097
[  329.386527] panfrost 180.gpu: js fault, js=0, status=DATA_INVALID_FAULT, 
head=0xa1a4c00, tail=0xa1a4c00
[  329.396293] panfrost 180.gpu: gpu sched timeout, js=0, config=0x3700, 
status=0x58, head=0xa1a4c00, tail=0xa1a4c00, sched_job=04c90381
[  329.411521] panfrost 180.gpu: Unhandled Page fault in AS0 at VA 
0x
[  329.411521] Reason: TODO
[  329.411521] raw fault status: 0xAA0003C2
[  329.411521] decoded fault status: SLAVE FAULT
[  329.411521] exception type 0xC2: TRANSLATION_FAULT_LEVEL2
[  329.411521] access type 0x3: WRITE
[  329.411521] source id 0xAA00

Thanks for your reviews, help on this serie,
Clement

Clément Péron (15):
   drm/panfrost: avoid static declaration
   drm/panfrost: clean headers in devfreq
   drm/panfrost: don't use pfdevfreq.busy_count to know if hw is idle
   drm/panfrost: introduce panfrost_devfreq struct
   drm/panfrost: use spinlock instead of atomic
   drm/panfrost: properly handle error in probe
   drm/panfrost: use device_property_present to check for OPP
   drm/panfrost: move devfreq_init()/fini() in device
   drm/panfrost: dynamically alloc regulators
   drm/panfrost: add regulators to devfreq
   drm/panfrost: set devfreq clock name
   arm64: defconfig: Enable devfreq cooling device
   arm64: dts: allwinner: h6: Add cooling map for GPU
   [DO NOT MERGE] arm64: dts: allwinner: h6: Add GPU OPP table
   [DO NOT MERGE] arm64: dts: allwinner: force GPU regulator to be always

  .../dts/allwinner/sun50i-h6-beelink-gs1.dts   |   1 +
  arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi  | 102 ++
  arch/arm64/configs/defconfig  |   1 +
  drivers/gpu/drm/panfrost/panfrost_devfreq.c   | 190 --
  drivers/gpu/drm/panfrost/panfrost_devfreq.h   |  32 ++-
  drivers/gpu/drm/panfrost/panfrost_device.c|  56 --
  drivers/gpu/drm/panfrost/panfrost_device.h|  14 +-
  drivers/gpu/drm/panfrost/panfrost_drv.c   |  15 +-
  drivers/gpu/drm/panfrost/panfrost_job.c   |  10 +-
  9 files changed, 310 insertions(+), 111 deletions(-)



[PATCH 1/2] ARM: multi_v7_defconfig: add Panfrost driver

2019-06-04 Thread Tomeu Vizoso
With the goal of making it easier for CI services such as KernelCI to
run tests for it.

Signed-off-by: Tomeu Vizoso 
---
 arch/arm/configs/multi_v7_defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/configs/multi_v7_defconfig 
b/arch/arm/configs/multi_v7_defconfig
index 6b748f214eae..952dff9d39f2 100644
--- a/arch/arm/configs/multi_v7_defconfig
+++ b/arch/arm/configs/multi_v7_defconfig
@@ -656,6 +656,7 @@ CONFIG_DRM_VC4=m
 CONFIG_DRM_ETNAVIV=m
 CONFIG_DRM_MXSFB=m
 CONFIG_DRM_PL111=m
+CONFIG_DRM_PANFROST=m
 CONFIG_FB_EFI=y
 CONFIG_FB_WM8505=y
 CONFIG_FB_SH_MOBILE_LCDC=y
-- 
2.20.1



[PATCH 2/2] arm64: defconfig: add Panfrost driver

2019-06-04 Thread Tomeu Vizoso
With the goal of making it easier for CI services such as KernelCI to
run tests for it.

Signed-off-by: Tomeu Vizoso 
---
 arch/arm64/configs/defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index 4d583514258c..d588ceb9aa3c 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -505,6 +505,7 @@ CONFIG_DRM_HISI_HIBMC=m
 CONFIG_DRM_HISI_KIRIN=m
 CONFIG_DRM_MESON=m
 CONFIG_DRM_PL111=m
+CONFIG_DRM_PANFROST=m
 CONFIG_FB=y
 CONFIG_FB_MODE_HELPERS=y
 CONFIG_BACKLIGHT_GENERIC=m
-- 
2.20.1



Re: [PATCH v6 0/6] Allwinner H6 Mali GPU support

2019-05-31 Thread Tomeu Vizoso
On Wed, 29 May 2019 at 19:38, Robin Murphy  wrote:
>
> On 29/05/2019 16:09, Tomeu Vizoso wrote:
> > On Tue, 21 May 2019 at 18:11, Clément Péron  wrote:
> >>
> > [snip]
> >> [  345.204813] panfrost 180.gpu: mmu irq status=1
> >> [  345.209617] panfrost 180.gpu: Unhandled Page fault in AS0 at VA
> >> 0x02400400
> >
> >  From what I can see here, 0x02400400 points to the first byte
> > of the first submitted job descriptor.
> >
> > So mapping buffers for the GPU doesn't seem to be working at all on
> > 64-bit T-760.
> >
> > Steven, Robin, do you have any idea of why this could be?
>
> I tried rolling back to the old panfrost/nondrm shim, and it works fine
> with kbase, and I also found that T-820 falls over in the exact same
> manner, so the fact that it seemed to be common to the smaller 33-bit
> designs rather than anything to do with the other
> job_descriptor_size/v4/v5 complication turned out to be telling.

Is this complication something you can explain? I don't know what v4
and v5 are meant here.

> [ as an aside, are 64-bit jobs actually known not to work on v4 GPUs, or
> is it just that nobody's yet observed a 64-bit blob driving one? ]

I'm looking right now at getting Panfrost working on T720 with 64-bit
descriptors, with the ultimate goal of making Panfrost
64-bit-descriptor only so we can have a single build of Mesa in
distros.

> Long story short, it appears that 'Mali LPAE' is also lacking the start
> level notion of VMSA, and expects a full 4-level table even for <40 bits
> when level 0 effectively redundant. Thus walking the 3-level table that
> io-pgtable comes back with ends up going wildly wrong. The hack below
> seems to do the job for me; if Clément can confirm (on T-720 you'll
> still need the userspace hack to force 32-bit jobs as well) then I think
> I'll cook up a proper refactoring of the allocator to put things right.

Mmaps seem to work with this patch, thanks.

The main complication I'm facing right now seems to be that the SFBD
descriptor on T720 seems to be different from the one we already had
(tested on T6xx?).

Cheers,

Tomeu

> Robin.
>
>
> ->8-
> diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
> index 546968d8a349..f29da6e8dc08 100644
> --- a/drivers/iommu/io-pgtable-arm.c
> +++ b/drivers/iommu/io-pgtable-arm.c
> @@ -1023,12 +1023,14 @@ arm_mali_lpae_alloc_pgtable(struct
> io_pgtable_cfg *cfg, void *cookie)
> iop = arm_64_lpae_alloc_pgtable_s1(cfg, cookie);
> if (iop) {
> u64 mair, ttbr;
> +   struct arm_lpae_io_pgtable *data = 
> io_pgtable_ops_to_data(&iop->ops);
>
> +   data->levels = 4;
> /* Copy values as union fields overlap */
> mair = cfg->arm_lpae_s1_cfg.mair[0];
> ttbr = cfg->arm_lpae_s1_cfg.ttbr[0];
>
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel


Re: [PATCH] arm64: dts: allwinner: Add GPU operating points for H6

2019-05-29 Thread Tomeu Vizoso
On Wed, 29 May 2019 at 17:37, Clément Péron  wrote:
>
> Hi Tomeu,
>
> On Wed, 29 May 2019 at 17:33, Tomeu Vizoso  wrote:
> >
> > The GPU driver needs them to change the clock frequency and regulator
> > voltage depending on the load.
>
> As requested by Maxime, I have dropped these OPP table as It's taken
> from vendor and untested with Panfrost.
>
> https://lore.kernel.org/patchwork/patch/1060374/

Ok, guess this series should wait then until we can run Panfrost on it
and check how DVFS is working.

Thanks,

Tomeu

> Regards,
> Clément
>
> >
> > Signed-off-by: Tomeu Vizoso 
> > Cc: Clément Péron 
> >
> > ---
> >
> > Feel free to pick up this patch if you are going to keep pushing this
> > series forward.
> >
> > Thanks,
> >
> > Tomeu
> > ---
> >  arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi | 66 
> >  1 file changed, 66 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi 
> > b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
> > index 6aad06095c40..decf7b56e2df 100644
> > --- a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
> > +++ b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
> > @@ -157,6 +157,71 @@
> > allwinner,sram = <&ve_sram 1>;
> > };
> >
> > +   gpu_opp_table: opp-table2 {
> > +   compatible = "operating-points-v2";
> > +
> > +   opp00 {
> > +   opp-hz = /bits/ 64 <75600>;
> > +   opp-microvolt = <104>;
> > +   };
> > +   opp01 {
> > +   opp-hz = /bits/ 64 <62400>;
> > +   opp-microvolt = <95>;
> > +   };
> > +   opp02 {
> > +   opp-hz = /bits/ 64 <57600>;
> > +   opp-microvolt = <93>;
> > +   };
> > +   opp03 {
> > +   opp-hz = /bits/ 64 <54000>;
> > +   opp-microvolt = <91>;
> > +   };
> > +   opp04 {
> > +   opp-hz = /bits/ 64 <50400>;
> > +   opp-microvolt = <89>;
> > +   };
> > +   opp05 {
> > +   opp-hz = /bits/ 64 <45600>;
> > +   opp-microvolt = <87>;
> > +   };
> > +   opp06 {
> > +   opp-hz = /bits/ 64 <43200>;
> > +   opp-microvolt = <86>;
> > +   };
> > +   opp07 {
> > +   opp-hz = /bits/ 64 <42000>;
> > +   opp-microvolt = <85>;
> > +   };
> > +   opp08 {
> > +   opp-hz = /bits/ 64 <40800>;
> > +   opp-microvolt = <84>;
> > +   };
> > +   opp09 {
> > +   opp-hz = /bits/ 64 <38400>;
> > +   opp-microvolt = <83>;
> > +   };
> > +   opp10 {
> > +   opp-hz = /bits/ 64 <36000>;
> > +   opp-microvolt = <82>;
> > +   };
> > +   opp11 {
> > +   opp-hz = /bits/ 64 <33600>;
> > +   opp-microvolt = <81>;
> > +   };
> > +   opp12 {
> > +   opp-hz = /bits/ 64 <31200>;
> > +   opp-microvolt = <81>;
> > +   };
> > +   opp13 {
> > +   opp-hz = /bits/ 64 <26400>;
> > +   opp-microvolt = <81>;
> > +   };
> > +   opp14 {
> > +   opp-hz = /bits/ 64 <21600>;
> > +   opp-microvolt = <81>;
> > +   };
> > +   };
> > +
> > gpu: gpu@180 {
> > compatible = "allwinner,sun50i-h6-mali",
> >  "arm,mali-t720";
> > @@ -168,6 +233,7 @@
> > clocks = <&ccu CLK_GPU>, <&ccu CLK_BUS_GPU>;
> > clock-names = "core", "bus";
> > resets = <&ccu RST_BUS_GPU>;
> > +   operating-points-v2 = <&gpu_opp_table>;
> > status = "disabled";
> > };
> >
> > --
> > 2.20.1
> >


[PATCH] arm64: dts: allwinner: Add GPU operating points for H6

2019-05-29 Thread Tomeu Vizoso
The GPU driver needs them to change the clock frequency and regulator
voltage depending on the load.

Signed-off-by: Tomeu Vizoso 
Cc: Clément Péron 

---

Feel free to pick up this patch if you are going to keep pushing this
series forward.

Thanks,

Tomeu
---
 arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi | 66 
 1 file changed, 66 insertions(+)

diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi 
b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
index 6aad06095c40..decf7b56e2df 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
+++ b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
@@ -157,6 +157,71 @@
allwinner,sram = <&ve_sram 1>;
};
 
+   gpu_opp_table: opp-table2 {
+   compatible = "operating-points-v2";
+
+   opp00 {
+   opp-hz = /bits/ 64 <75600>;
+   opp-microvolt = <104>;
+   };
+   opp01 {
+   opp-hz = /bits/ 64 <62400>;
+   opp-microvolt = <95>;
+   };
+   opp02 {
+   opp-hz = /bits/ 64 <57600>;
+   opp-microvolt = <93>;
+   };
+   opp03 {
+   opp-hz = /bits/ 64 <54000>;
+   opp-microvolt = <91>;
+   };
+   opp04 {
+   opp-hz = /bits/ 64 <50400>;
+   opp-microvolt = <89>;
+   };
+   opp05 {
+   opp-hz = /bits/ 64 <45600>;
+   opp-microvolt = <87>;
+   };
+   opp06 {
+   opp-hz = /bits/ 64 <43200>;
+   opp-microvolt = <86>;
+   };
+   opp07 {
+   opp-hz = /bits/ 64 <42000>;
+   opp-microvolt = <85>;
+   };
+   opp08 {
+   opp-hz = /bits/ 64 <40800>;
+   opp-microvolt = <84>;
+   };
+   opp09 {
+   opp-hz = /bits/ 64 <38400>;
+   opp-microvolt = <83>;
+   };
+   opp10 {
+   opp-hz = /bits/ 64 <36000>;
+   opp-microvolt = <82>;
+   };
+   opp11 {
+   opp-hz = /bits/ 64 <33600>;
+   opp-microvolt = <81>;
+   };
+   opp12 {
+   opp-hz = /bits/ 64 <31200>;
+   opp-microvolt = <81>;
+   };
+   opp13 {
+   opp-hz = /bits/ 64 <26400>;
+   opp-microvolt = <81>;
+   };
+   opp14 {
+   opp-hz = /bits/ 64 <21600>;
+   opp-microvolt = <81>;
+   };
+   };
+
gpu: gpu@180 {
compatible = "allwinner,sun50i-h6-mali",
 "arm,mali-t720";
@@ -168,6 +233,7 @@
clocks = <&ccu CLK_GPU>, <&ccu CLK_BUS_GPU>;
clock-names = "core", "bus";
resets = <&ccu RST_BUS_GPU>;
+   operating-points-v2 = <&gpu_opp_table>;
status = "disabled";
};
 
-- 
2.20.1



Re: [GIT PULL] Thermal-SoC management changes for v5.2-rc1

2019-05-27 Thread Tomeu Vizoso
On Fri, 24 May 2019 at 15:54, Eduardo Valentin  wrote:
>
> Hello,
>
> On Fri, May 24, 2019 at 10:23:09AM +0200, Tomeu Vizoso wrote:
> > On Fri, 24 May 2019 at 04:40, Eduardo Valentin  wrote:
> > >
> > > On Thu, May 23, 2019 at 11:46:47AM +0200, Tomeu Vizoso wrote:
> > > > Hi Eduardo,
> > > >
> > > > I saw that for 5.1 [0] you included a kernelci boot report for your
> > > > tree, but not for 5.2. Have you found anything that should be improved
> > > > in KernelCI for it to be more useful to maintainers like you?
> > >
> > > Honestly, I take a couple of automated testing as input before sending
> > > my pulls to Linux: (a) my local test, (b) kernel-ci, and (c) 0-day.
> > >
> > > There was really no reason specifically for me to not add the report
> > > from kernelci, except..
> > > >
> > > > [0] 
> > > > https://lore.kernel.org/lkml/20190306161207.GA7365@localhost.localdomain/
> > > >
> > > > I found about this when trying to understand why the boot on the
> > > > veyron-jaq board has been broken in 5.2-rc1.
> > > >
> > >
> > > I remember a report saying this failed, but from what I could tell from
> > > the boot log, the board booted and hit terminal. But apparently, after
> > > all reports from developers, the veyron-jaq boards were in a hang state.
> > >
> > > That was hard for me to tell from your logs, as they looked like
> > > a regular boot that hits terminal.
> > >
> > > Maybe I should have looked for a specific output of a command you guys
> > > run, saying "successful boot" somewhere?
> >
> > I think what is easiest and clearest is to consider the bisection
> > reports as a very strong indication that something is quite wrong in
> > the branch.
>
> OK. I hear you.
>
> >
> > Because if a board stopped booting and the bisection found a
> > suspicious patch, and reverting it gets the board booting again, then
> > chances are very high that the patch in question broke that boot.
> >
>
>
> Yeah, for sure If I had understood the report properly I could have
> nacked the patch.
>
> > Do you think the wording could be improved to make it clearer? Or
> > maybe some other changes to make all this more useful to maintainers
> > like you?
> >
>
> Well, from my perspective, I need to judge if the failure on your report
> is really related to my changes. Many times, specially on build errors,
> we get failures that are unrelated. Build errors are more straight
> forward do judge. Similarly, we need to find out if a boot issue is
> caused by a change on the branch or something existing. On boot issues
> from kernelci reports, I think the false negatives I have been seeing
> is lab/boards failing to boot. Those can also be easy to spot as the
> in most cases the kernel wont even load.
>
> For this particular case, as I described before, the kernel would
> load and hit the shell command line, but in fact it was in hang state
> IIRC. That is probably why it has not straight forward to understand
> from the log. Maybe a successful boot message somewhere would have
> helped to spot the problem (or the opposite of it, something
> saying, I was expecting to execute a command and board was
> unresponsive).

Ah, I think I see now what you meant. In the boot log linked below,
near the end we have:

22:14:41.981401  ShellCommand command timed out.: Sending # in case of
corruption. Connection timeout 00:04:25, retry in 00:02:12
22:14:42.083594  #

The # character is sent by the LAVA machine that the DUT is connected
to, in the hope that a userspace shell would reply with something.

But the kernel failed to boot to userspace, so we have this at the end:

22:16:54.558087  depthcharge-retry failed: 1 of 1 attempts.
'auto-login-action timed out after 285 seconds'
22:16:54.560479  depthcharge-action failed: 1 of 1 attempts.
'auto-login-action timed out after 285 seconds'
22:16:54.855023  JobError: Your job cannot terminate cleanly.

https://storage.kernelci.org/evalenti/for-kernelci/v5.1-rc6-58-gbe827ffd38ea/arm/multi_v7_defconfig/gcc-8/lab-collabora/boot-rk3288-veyron-jaq.html

Do you think that's the source of the confusion?

Thanks,

Tomeu


Re: [GIT PULL] Thermal-SoC management changes for v5.2-rc1

2019-05-24 Thread Tomeu Vizoso
On Fri, 24 May 2019 at 04:40, Eduardo Valentin  wrote:
>
> On Thu, May 23, 2019 at 11:46:47AM +0200, Tomeu Vizoso wrote:
> > Hi Eduardo,
> >
> > I saw that for 5.1 [0] you included a kernelci boot report for your
> > tree, but not for 5.2. Have you found anything that should be improved
> > in KernelCI for it to be more useful to maintainers like you?
>
> Honestly, I take a couple of automated testing as input before sending
> my pulls to Linux: (a) my local test, (b) kernel-ci, and (c) 0-day.
>
> There was really no reason specifically for me to not add the report
> from kernelci, except..
> >
> > [0] 
> > https://lore.kernel.org/lkml/20190306161207.GA7365@localhost.localdomain/
> >
> > I found about this when trying to understand why the boot on the
> > veyron-jaq board has been broken in 5.2-rc1.
> >
>
> I remember a report saying this failed, but from what I could tell from
> the boot log, the board booted and hit terminal. But apparently, after
> all reports from developers, the veyron-jaq boards were in a hang state.
>
> That was hard for me to tell from your logs, as they looked like
> a regular boot that hits terminal.
>
> Maybe I should have looked for a specific output of a command you guys
> run, saying "successful boot" somewhere?

I think what is easiest and clearest is to consider the bisection
reports as a very strong indication that something is quite wrong in
the branch.

Because if a board stopped booting and the bisection found a
suspicious patch, and reverting it gets the board booting again, then
chances are very high that the patch in question broke that boot.

Do you think the wording could be improved to make it clearer? Or
maybe some other changes to make all this more useful to maintainers
like you?

Thanks,

Tomeu

> > Thanks,
> >
> > Tomeu
> >
> > On Thu, 16 May 2019 at 06:43, Eduardo Valentin  wrote:
> > >
> > > Hello Linus,
> > >
> > > Please consider the following thermal soc changes for v5.2-rc1.
> > >
> > > The following changes since commit 
> > > 37624b58542fb9f2d9a70e6ea006ef8a5f66c30b:
> > >
> > >   Linux 5.1-rc7 (2019-04-28 17:04:13 -0700)
> > >
> > > are available in the git repository at:
> > >
> > >   
> > > git://git.kernel.org/pub/scm/linux/kernel/git/evalenti/linux-soc-thermal 
> > > linus
> > >
> > > for you to fetch changes up to 37bcec5d9f71bd13142a97d2196b293c9ac23823:
> > >
> > >   hwmon: (pwm-fan) Use devm_thermal_of_cooling_device_register 
> > > (2019-05-14 07:00:47 -0700)
> > >
> > > Specifics:
> > > - thermal core has a new devm_* API for registering cooling devices, 
> > > thanks to Guenter R.
> > >   I took the entire series, that is why you see changes on drivers/hwmon 
> > > in this pull.
> > > - rockchip thermal driver gains support to PX30 SoC, thanks to Elaine Z.
> > > - the generic-adc thermal driver now considers the lookup table DT 
> > > property as optional,
> > >   thanks to Jean-Francois D.
> > > - Refactoring of tsens thermal driver, thanks to Amit K.
> > > - Cleanups on cpu cooling driver, thanks to Daniel L.
> > > - broadcom thermal driver dropped support to ACPI, thanks to Srinath M.
> > > - tegra thermal driver gains support to OC hw throttle and GPU throtle, 
> > > thanks to Wei Ni.
> > > - Fixes in several thermal drivers.
> > >
> > > BR,
> > >
> > > Eduardo Valentin
> > >
> > > 
> > > Amit Kucheria (21):
> > >   drivers: thermal: tsens: Document the data structures
> > >   drivers: thermal: tsens: Rename tsens_data
> > >   drivers: thermal: tsens: Rename tsens_device
> > >   drivers: thermal: tsens: Rename variable tmdev
> > >   drivers: thermal: tsens: Use consistent names for variables
> > >   drivers: thermal: tsens: Function prototypes should have argument 
> > > names
> > >   drivers: thermal: tsens: Rename tsens-8916 to prepare to merge with 
> > > tsens-8974
> > >   drivers: thermal: tsens: Rename constants to prepare to merge with 
> > > tsens-8974
> > >   drivers: thermal: tsens: Merge tsens-8974 into tsens-v0_1
> > >   drivers: thermal: tsens: Introduce reg_fields to deal with register 
> > > description
> > >   drivers: thermal: tsens: Save reference to the device pointer and 
> > > use it
> > >   drivers: t

Re: [GIT PULL] Thermal-SoC management changes for v5.2-rc1

2019-05-23 Thread Tomeu Vizoso
Hi Eduardo,

I saw that for 5.1 [0] you included a kernelci boot report for your
tree, but not for 5.2. Have you found anything that should be improved
in KernelCI for it to be more useful to maintainers like you?

[0] https://lore.kernel.org/lkml/20190306161207.GA7365@localhost.localdomain/

I found about this when trying to understand why the boot on the
veyron-jaq board has been broken in 5.2-rc1.

Thanks,

Tomeu

On Thu, 16 May 2019 at 06:43, Eduardo Valentin  wrote:
>
> Hello Linus,
>
> Please consider the following thermal soc changes for v5.2-rc1.
>
> The following changes since commit 37624b58542fb9f2d9a70e6ea006ef8a5f66c30b:
>
>   Linux 5.1-rc7 (2019-04-28 17:04:13 -0700)
>
> are available in the git repository at:
>
>   git://git.kernel.org/pub/scm/linux/kernel/git/evalenti/linux-soc-thermal 
> linus
>
> for you to fetch changes up to 37bcec5d9f71bd13142a97d2196b293c9ac23823:
>
>   hwmon: (pwm-fan) Use devm_thermal_of_cooling_device_register (2019-05-14 
> 07:00:47 -0700)
>
> Specifics:
> - thermal core has a new devm_* API for registering cooling devices, thanks 
> to Guenter R.
>   I took the entire series, that is why you see changes on drivers/hwmon in 
> this pull.
> - rockchip thermal driver gains support to PX30 SoC, thanks to Elaine Z.
> - the generic-adc thermal driver now considers the lookup table DT property 
> as optional,
>   thanks to Jean-Francois D.
> - Refactoring of tsens thermal driver, thanks to Amit K.
> - Cleanups on cpu cooling driver, thanks to Daniel L.
> - broadcom thermal driver dropped support to ACPI, thanks to Srinath M.
> - tegra thermal driver gains support to OC hw throttle and GPU throtle, 
> thanks to Wei Ni.
> - Fixes in several thermal drivers.
>
> BR,
>
> Eduardo Valentin
>
> 
> Amit Kucheria (21):
>   drivers: thermal: tsens: Document the data structures
>   drivers: thermal: tsens: Rename tsens_data
>   drivers: thermal: tsens: Rename tsens_device
>   drivers: thermal: tsens: Rename variable tmdev
>   drivers: thermal: tsens: Use consistent names for variables
>   drivers: thermal: tsens: Function prototypes should have argument names
>   drivers: thermal: tsens: Rename tsens-8916 to prepare to merge with 
> tsens-8974
>   drivers: thermal: tsens: Rename constants to prepare to merge with 
> tsens-8974
>   drivers: thermal: tsens: Merge tsens-8974 into tsens-v0_1
>   drivers: thermal: tsens: Introduce reg_fields to deal with register 
> description
>   drivers: thermal: tsens: Save reference to the device pointer and use it
>   drivers: thermal: tsens: Don't print error message on -EPROBE_DEFER
>   drivers: thermal: tsens: Add new operation to check if a sensor is 
> enabled
>   drivers: thermal: tsens: change data type for sensor IDs
>   drivers: thermal: tsens: Introduce IP-specific max_sensor count
>   drivers: thermal: tsens: simplify get_temp_tsens_v2 routine
>   drivers: thermal: tsens: Move get_temp_tsens_v2 to allow sharing
>   drivers: thermal: tsens: Common get_temp() learns to do ADC conversion
>   dt: thermal: tsens: Add bindings for qcs404
>   drivers: thermal: tsens: Add generic support for TSENS v1 IP
>   drivers: thermal: tsens: Move calibration constants to header file
>
> Andrey Smirnov (1):
>   thermal: qoriq: Remove unnecessary DT node is NULL check
>
> Daniel Lezcano (4):
>   thermal/drivers/cpu_cooling: Remove pointless test in power2state()
>   thermal/drivers/cpu_cooling: Fixup the header and copyright
>   thermal/drivers/cpu_cooling: Add Software Package Data Exchange (SPDX)
>   thermal/drivers/cpu_cooling: Remove pointless field
>
> Elaine Zhang (3):
>   thermal: rockchip: fix up the tsadc pinctrl setting error
>   dt-bindings: rockchip-thermal: Support the PX30 SoC compatible
>   thermal: rockchip: Support the PX30 SoC in thermal driver
>
> Enrico Weigelt, metux IT consult (1):
>   drivers: thermal: Kconfig: pedantic cleanups
>
> Guenter Roeck (6):
>   thermal: Introduce devm_thermal_of_cooling_device_register
>   hwmon: (aspeed-pwm-tacho) Use devm_thermal_of_cooling_device_register
>   hwmon: (gpio-fan) Use devm_thermal_of_cooling_device_register
>   hwmon: (mlxreg-fan) Use devm_thermal_of_cooling_device_register
>   hwmon: (npcm750-pwm-fan) Use devm_thermal_of_cooling_device_register
>   hwmon: (pwm-fan) Use devm_thermal_of_cooling_device_register
>
> Hoan Nguyen An (1):
>   thermal: rcar_gen3_thermal: Fix init value of IRQCTL register
>
> Jean-Francois Dagenais (2):
>   thermal: generic-adc: make lookup table optional
>   dt-bindings: thermal: generic-adc: make lookup-table optional
>
> Jiada Wang (3):
>   thermal: rcar_gen3_thermal: fix interrupt type
>   thermal: rcar_gen3_thermal: disable interrupt in .remove
>   thermal: rcar_gen3_thermal: Fix to show correct trip points number
>
> Matthias Kaeh

Re: [PATCH] drm/panfrost: Add sanity checks to submit IOCTL

2019-04-24 Thread Tomeu Vizoso
On Wed, 24 Apr 2019 at 15:20, Daniel Vetter  wrote:
>
> On Wed, Apr 24, 2019 at 03:13:53PM +0200, Tomeu Vizoso wrote:
> > So userspace can get feedback on any error conditions, instead of going
> > ahead and things breaking later.
> >
> > Signed-off-by: Tomeu Vizoso 
>
> Bonus: some igts to exercise these corner cases. It helps a lot with uapi
> review and testing.

I'm rebasing that igt branch as we speak :)

Tomeu

> -Daniel
>
> > ---
> >  drivers/gpu/drm/panfrost/panfrost_drv.c | 35 +
> >  1 file changed, 24 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c 
> > b/drivers/gpu/drm/panfrost/panfrost_drv.c
> > index c06af78ab833..0f2863cb8077 100644
> > --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> > +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> > @@ -172,13 +172,27 @@ static int panfrost_ioctl_submit(struct drm_device 
> > *dev, void *data,
> >  {
> >   struct panfrost_device *pfdev = dev->dev_private;
> >   struct drm_panfrost_submit *args = data;
> > - struct drm_syncobj *sync_out;
> > + struct drm_syncobj *sync_out = NULL;
> >   struct panfrost_job *job;
> >   int ret = 0;
> >
> > + if (!args->jc)
> > + return -EINVAL;
> > +
> > + if (args->requirements && args->requirements != PANFROST_JD_REQ_FS)
> > + return -EINVAL;
> > +
> > + if (args->out_sync > 0) {
> > + sync_out = drm_syncobj_find(file, args->out_sync);
> > + if (!sync_out)
> > + return -ENODEV;
> > + }
> > +
> >   job = kzalloc(sizeof(*job), GFP_KERNEL);
> > - if (!job)
> > - return -ENOMEM;
> > + if (!job) {
> > + ret = -ENOMEM;
> > + goto fail_out_sync;
> > + }
> >
> >   kref_init(&job->refcount);
> >
> > @@ -190,25 +204,24 @@ static int panfrost_ioctl_submit(struct drm_device 
> > *dev, void *data,
> >
> >   ret = panfrost_copy_in_sync(dev, file, args, job);
> >   if (ret)
> > - goto fail;
> > + goto fail_job;
> >
> >   ret = panfrost_lookup_bos(dev, file, args, job);
> >   if (ret)
> > - goto fail;
> > + goto fail_job;
> >
> >   ret = panfrost_job_push(job);
> >   if (ret)
> > - goto fail;
> > + goto fail_job;
> >
> >   /* Update the return sync object for the job */
> > - sync_out = drm_syncobj_find(file, args->out_sync);
> > - if (sync_out) {
> > + if (sync_out)
> >   drm_syncobj_replace_fence(sync_out, job->render_done_fence);
> > - drm_syncobj_put(sync_out);
> > - }
> >
> > -fail:
> > +fail_job:
> >   panfrost_job_put(job);
> > +fail_out_sync:
> > + drm_syncobj_put(sync_out);
> >
> >   return ret;
> >  }
> > --
> > 2.20.1
> >
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch


Re: [PATCH] PM / devfreq: Return -ENODEV from try_then_request_governor

2019-04-23 Thread Tomeu Vizoso
On Tue, 23 Apr 2019 at 11:56, Enric Balletbo i Serra
 wrote:
>
> Hi Tomeu,
>
> On 23/4/19 10:11, Tomeu Vizoso wrote:
> > Callers don't expect it to return NULL, but an error code.
> >
> > Fixes Oops such as the one below, when one tries to set a governor that
> > isn't available:
> >
> > Unable to handle kernel NULL pointer dereference at virtual address 0018
> >
> > [] (governor_store) from [] 
> > (kernfs_fop_write+0x100/0x1e0)
> > [] (kernfs_fop_write) from [] (__vfs_write+0x2c/0x17c)
> > [] (__vfs_write) from [] (vfs_write+0xa4/0x184)
> > [] (vfs_write) from [] (ksys_write+0x4c/0xac)
> > [] (ksys_write) from [] (ret_fast_syscall+0x0/0x28)
> >
> > Signed-off-by: Tomeu Vizoso 
> > Fixes: 23c7b54ca1cd ("PM / devfreq: Fix devfreq_add_device() when drivers 
> > are built as modules.")
> > Reported-by: Alyssa Rosenzweig 
> > Cc: Enric Balletbo i Serra 
> > ---
>
> There is already a fix for that. The fix was initially sent in October [2] but
> unfortunately it got lost. I resend and now is queued [1]. Hopefully the Fixes
> tag will help to pick the fix to the proper kernel releases.

Actually, Steve Price sent a third patch for this same issue.

Glad to read that it's being merged.

Thanks,

Tomeu



> Thanks,
>  Enric
>
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/mzx/devfreq.git/commit/?h=for-next&id=b53b0128052ffd687797d5f4deeb76327e7b5711
>
> [2] https://lkml.org/lkml/2018/10/16/744
>
> >  drivers/devfreq/devfreq.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> > index 0ae3de76833b..5539e9be718d 100644
> > --- a/drivers/devfreq/devfreq.c
> > +++ b/drivers/devfreq/devfreq.c
> > @@ -254,7 +254,7 @@ static struct devfreq_governor 
> > *try_then_request_governor(const char *name)
> >   /* Restore previous state before return */
> >   mutex_lock(&devfreq_list_lock);
> >   if (err)
> > - return NULL;
> > + return ERR_PTR(-ENODEV);
> >
> >   governor = find_devfreq_governor(name);
> >   }
> >


[PATCH] PM / devfreq: Return -ENODEV from try_then_request_governor

2019-04-23 Thread Tomeu Vizoso
Callers don't expect it to return NULL, but an error code.

Fixes Oops such as the one below, when one tries to set a governor that
isn't available:

Unable to handle kernel NULL pointer dereference at virtual address 0018

[] (governor_store) from [] (kernfs_fop_write+0x100/0x1e0)
[] (kernfs_fop_write) from [] (__vfs_write+0x2c/0x17c)
[] (__vfs_write) from [] (vfs_write+0xa4/0x184)
[] (vfs_write) from [] (ksys_write+0x4c/0xac)
[] (ksys_write) from [] (ret_fast_syscall+0x0/0x28)

Signed-off-by: Tomeu Vizoso 
Fixes: 23c7b54ca1cd ("PM / devfreq: Fix devfreq_add_device() when drivers are 
built as modules.")
Reported-by: Alyssa Rosenzweig 
Cc: Enric Balletbo i Serra 
---
 drivers/devfreq/devfreq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 0ae3de76833b..5539e9be718d 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -254,7 +254,7 @@ static struct devfreq_governor 
*try_then_request_governor(const char *name)
/* Restore previous state before return */
mutex_lock(&devfreq_list_lock);
if (err)
-   return NULL;
+   return ERR_PTR(-ENODEV);
 
governor = find_devfreq_governor(name);
}
-- 
2.20.1



[PATCH v4] arm64: dts: rockchip: Add DT for nanopc-t4

2018-11-27 Thread Tomeu Vizoso
This adds a device tree for the NanoPC-T4 SBC, which is based on the
Rockchip RK3399 SoC and marketed by FriendlyELEC.

Known working:

- Serial
- Ethernet
- HDMI
- USB 2.0

All of the interesting stuff is in a .dtsi because there are at least
two other boards that share most of it: NanoPi M4 and NanoPi NEO4.

Signed-off-by: Tomeu Vizoso 

---

v2: - Rename compatible from friendlyelec to friendlyarm, to match
  existing bindings
- Remove superfluous node spi1

v3: - Rewrite regulator tree to match the schematics (Heiko)
- Sort top-level nodes alphabetically (Heiko)
- Used defines for GPIO numbers (Heiko)
- Enabled rga (Heiko)
- Removed cdn_dp node (Heiko)
- Removed dependencies to fusb0 as extcon (Heiko)
- Removed superfluous properties (Heiko)

v4: - Replace underscores in node names (Heiko)
- Reorder entry in makefile (Heiko)
- Remove superfluous properties and nodes (Heiko and Shawn)
- Use xin32k as one of the clock outputs of the RK808 (Heiko)
---
 .../devicetree/bindings/arm/rockchip.txt  |   4 +
 arch/arm64/boot/dts/rockchip/Makefile |   1 +
 .../boot/dts/rockchip/rk3399-nanopc-t4.dts|  18 +
 .../boot/dts/rockchip/rk3399-nanopi4.dtsi | 732 ++
 4 files changed, 755 insertions(+)
 create mode 100644 arch/arm64/boot/dts/rockchip/rk3399-nanopc-t4.dts
 create mode 100644 arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi

diff --git a/Documentation/devicetree/bindings/arm/rockchip.txt 
b/Documentation/devicetree/bindings/arm/rockchip.txt
index 0cc71236d639..e907d309486e 100644
--- a/Documentation/devicetree/bindings/arm/rockchip.txt
+++ b/Documentation/devicetree/bindings/arm/rockchip.txt
@@ -71,6 +71,10 @@ Rockchip platforms device tree bindings
 Required root node properties:
   - compatible = "firefly,roc-rk3399-pc", "rockchip,rk3399";
 
+- FriendlyElec NanoPC-T4 board:
+Required root node properties:
+  - compatible = "friendlyarm,nanopc-t4", "rockchip,rk3399";
+
 - ChipSPARK PopMetal-RK3288 board:
 Required root node properties:
   - compatible = "chipspark,popmetal-rk3288", "rockchip,rk3288";
diff --git a/arch/arm64/boot/dts/rockchip/Makefile 
b/arch/arm64/boot/dts/rockchip/Makefile
index 49042c477870..19c129702e06 100644
--- a/arch/arm64/boot/dts/rockchip/Makefile
+++ b/arch/arm64/boot/dts/rockchip/Makefile
@@ -14,6 +14,7 @@ dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-ficus.dtb
 dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-firefly.dtb
 dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-gru-bob.dtb
 dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-gru-kevin.dtb
+dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-nanopc-t4.dtb
 dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-puma-haikou.dtb
 dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-roc-pc.dtb
 dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-rock960.dtb
diff --git a/arch/arm64/boot/dts/rockchip/rk3399-nanopc-t4.dts 
b/arch/arm64/boot/dts/rockchip/rk3399-nanopc-t4.dts
new file mode 100644
index ..0965712b4464
--- /dev/null
+++ b/arch/arm64/boot/dts/rockchip/rk3399-nanopc-t4.dts
@@ -0,0 +1,18 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * FriendlyElec NanoPC-T4 board device tree source
+ *
+ * Copyright (c) 2018 FriendlyElec Computer Tech. Co., Ltd.
+ * (http://www.friendlyarm.com)
+ *
+ * Copyright (c) 2018 Collabora Ltd.
+ */
+
+/dts-v1/;
+#include "rk3399-nanopi4.dtsi"
+
+/ {
+   model = "FriendlyElec NanoPC-T4";
+   compatible = "friendlyarm,nanopc-t4", "rockchip,rk3399";
+};
+
diff --git a/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi 
b/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi
new file mode 100644
index ..e10b98d637d3
--- /dev/null
+++ b/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi
@@ -0,0 +1,732 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * RK3399-based FriendlyElec boards device tree source
+ *
+ * Copyright (c) 2016 Fuzhou Rockchip Electronics Co., Ltd
+ *
+ * Copyright (c) 2018 FriendlyElec Computer Tech. Co., Ltd.
+ * (http://www.friendlyarm.com)
+ *
+ * Copyright (c) 2018 Collabora Ltd.
+ */
+
+/dts-v1/;
+#include 
+#include "rk3399.dtsi"
+#include "rk3399-opp.dtsi"
+
+/ {
+   chosen {
+   stdout-path = "serial2:150n8";
+   };
+
+   clkin_gmac: external-gmac-clock {
+   compatible = "fixed-clock";
+   clock-frequency = <12500>;
+   clock-output-names = "clkin_gmac";
+   #clock-cells = <0>;
+   };
+
+   vdd_5v: vdd-5v {
+   compatible = "regulator-fixed";
+   regulator-name = "vdd_5v";
+   regulator-always-on;
+   regulator-boot-on;
+   };
+
+   vcc5v0_core: vcc5v0-core {
+   compatible = "regulator-fixed";
+   regulator-name = "vcc5v0_core";
+   regulator-always-o

[PATCH v3] arm64: dts: rockchip: Add DT for nanopc-t4

2018-11-26 Thread Tomeu Vizoso
This adds a device tree for the NanoPC-T4 SBC, which is based on the
Rockchip RK3399 SoC and marketed by FriendlyELEC.

Known working:

- Serial
- Ethernet
- HDMI
- USB 2.0

All of the interesting stuff is in a .dtsi because there are at least
two other boards that share most of it: NanoPi M4 and NanoPi NEO4.

Signed-off-by: Tomeu Vizoso 

---

v2: - Rename compatible from friendlyelec to friendlyarm, to match
  existing bindings
- Remove superfluous node spi1

v3: - Rewrite regulator tree to match the schematics (Heiko)
- Sort top-level nodes alphabetically (Heiko)
- Used defines for GPIO numbers (Heiko)
- Enabled rga (Heiko)
- Removed cdn_dp node (Heiko)
- Removed dependencies to fusb0 as extcon (Heiko)
- Removed superfluous properties (Heiko)
---
 .../devicetree/bindings/arm/rockchip.txt  |   4 +
 arch/arm64/boot/dts/rockchip/Makefile |   1 +
 .../boot/dts/rockchip/rk3399-nanopc-t4.dts|  18 +
 .../boot/dts/rockchip/rk3399-nanopi4.dtsi | 740 ++
 4 files changed, 763 insertions(+)
 create mode 100644 arch/arm64/boot/dts/rockchip/rk3399-nanopc-t4.dts
 create mode 100644 arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi

diff --git a/Documentation/devicetree/bindings/arm/rockchip.txt 
b/Documentation/devicetree/bindings/arm/rockchip.txt
index 0cc71236d639..e907d309486e 100644
--- a/Documentation/devicetree/bindings/arm/rockchip.txt
+++ b/Documentation/devicetree/bindings/arm/rockchip.txt
@@ -71,6 +71,10 @@ Rockchip platforms device tree bindings
 Required root node properties:
   - compatible = "firefly,roc-rk3399-pc", "rockchip,rk3399";
 
+- FriendlyElec NanoPC-T4 board:
+Required root node properties:
+  - compatible = "friendlyarm,nanopc-t4", "rockchip,rk3399";
+
 - ChipSPARK PopMetal-RK3288 board:
 Required root node properties:
   - compatible = "chipspark,popmetal-rk3288", "rockchip,rk3288";
diff --git a/arch/arm64/boot/dts/rockchip/Makefile 
b/arch/arm64/boot/dts/rockchip/Makefile
index 49042c477870..4cbd2c461052 100644
--- a/arch/arm64/boot/dts/rockchip/Makefile
+++ b/arch/arm64/boot/dts/rockchip/Makefile
@@ -1,6 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0
 dtb-$(CONFIG_ARCH_ROCKCHIP) += px30-evb.dtb
 dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3328-evb.dtb
+dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-nanopc-t4.dtb
 dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3328-rock64.dtb
 dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3328-roc-cc.dtb
 dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3368-evb-act8846.dtb
diff --git a/arch/arm64/boot/dts/rockchip/rk3399-nanopc-t4.dts 
b/arch/arm64/boot/dts/rockchip/rk3399-nanopc-t4.dts
new file mode 100644
index ..0965712b4464
--- /dev/null
+++ b/arch/arm64/boot/dts/rockchip/rk3399-nanopc-t4.dts
@@ -0,0 +1,18 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * FriendlyElec NanoPC-T4 board device tree source
+ *
+ * Copyright (c) 2018 FriendlyElec Computer Tech. Co., Ltd.
+ * (http://www.friendlyarm.com)
+ *
+ * Copyright (c) 2018 Collabora Ltd.
+ */
+
+/dts-v1/;
+#include "rk3399-nanopi4.dtsi"
+
+/ {
+   model = "FriendlyElec NanoPC-T4";
+   compatible = "friendlyarm,nanopc-t4", "rockchip,rk3399";
+};
+
diff --git a/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi 
b/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi
new file mode 100644
index ..f102ff2317c3
--- /dev/null
+++ b/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi
@@ -0,0 +1,740 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * RK3399-based FriendlyElec boards device tree source
+ *
+ * Copyright (c) 2016 Fuzhou Rockchip Electronics Co., Ltd
+ *
+ * Copyright (c) 2018 FriendlyElec Computer Tech. Co., Ltd.
+ * (http://www.friendlyarm.com)
+ *
+ * Copyright (c) 2018 Collabora Ltd.
+ */
+
+/dts-v1/;
+#include 
+#include "rk3399.dtsi"
+#include "rk3399-opp.dtsi"
+
+/ {
+   chosen {
+   stdout-path = "serial2:150n8";
+   };
+
+   clkin_gmac: external-gmac-clock {
+   compatible = "fixed-clock";
+   clock-frequency = <12500>;
+   clock-output-names = "clkin_gmac";
+   #clock-cells = <0>;
+   };
+
+   vdd_5v: vdd_5v {
+   compatible = "regulator-fixed";
+   regulator-name = "vdd_5v";
+   regulator-always-on;
+   regulator-boot-on;
+   };
+
+   vcc5v0_core: vcc5v0_core {
+   compatible = "regulator-fixed";
+   regulator-name = "vcc5v0_core";
+   regulator-always-on;
+   regulator-boot-on;
+   vin-supply = <&vdd_5v>;
+   };
+
+   vcc3v3_sys: vcc3v3_sys {
+   compatible = "regulator-fixed";
+   regulator-name = "vcc3v3_sys";
+   regulator-always-o

[PATCH v2] arm64: dts: rockchip: Add DT for nanopc-t4

2018-11-22 Thread Tomeu Vizoso
This adds a device tree for the NanoPC-T4 SBC, which is based on the
Rockchip RK3399 SoC and marketed by FriendlyELEC.

Known working:

- Serial
- Ethernet
- HDMI
- USB 2.0

All of the interesting stuff is in a .dtsi because there are at least
two other boards that share most of it: NanoPi M4 and NanoPi NEO4.

Signed-off-by: Tomeu Vizoso 

---

v2: - Rename compatible from friendlyelec to friendlyarm, to match
  existing bindings
- Remove superfluous node spi1
---
 .../devicetree/bindings/arm/rockchip.txt  |   4 +
 arch/arm64/boot/dts/rockchip/Makefile |   1 +
 .../boot/dts/rockchip/rk3399-nanopc-t4.dts|  18 +
 .../boot/dts/rockchip/rk3399-nanopi4.dtsi | 770 ++
 4 files changed, 793 insertions(+)
 create mode 100644 arch/arm64/boot/dts/rockchip/rk3399-nanopc-t4.dts
 create mode 100644 arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi

diff --git a/Documentation/devicetree/bindings/arm/rockchip.txt 
b/Documentation/devicetree/bindings/arm/rockchip.txt
index 0cc71236d639..e907d309486e 100644
--- a/Documentation/devicetree/bindings/arm/rockchip.txt
+++ b/Documentation/devicetree/bindings/arm/rockchip.txt
@@ -71,6 +71,10 @@ Rockchip platforms device tree bindings
 Required root node properties:
   - compatible = "firefly,roc-rk3399-pc", "rockchip,rk3399";
 
+- FriendlyElec NanoPC-T4 board:
+Required root node properties:
+  - compatible = "friendlyarm,nanopc-t4", "rockchip,rk3399";
+
 - ChipSPARK PopMetal-RK3288 board:
 Required root node properties:
   - compatible = "chipspark,popmetal-rk3288", "rockchip,rk3288";
diff --git a/arch/arm64/boot/dts/rockchip/Makefile 
b/arch/arm64/boot/dts/rockchip/Makefile
index 49042c477870..ed90cd1e5a8b 100644
--- a/arch/arm64/boot/dts/rockchip/Makefile
+++ b/arch/arm64/boot/dts/rockchip/Makefile
@@ -20,3 +20,4 @@ dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-rock960.dtb
 dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-rockpro64.dtb
 dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-sapphire.dtb
 dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-sapphire-excavator.dtb
+dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-nanopc-t4.dtb
diff --git a/arch/arm64/boot/dts/rockchip/rk3399-nanopc-t4.dts 
b/arch/arm64/boot/dts/rockchip/rk3399-nanopc-t4.dts
new file mode 100644
index ..0965712b4464
--- /dev/null
+++ b/arch/arm64/boot/dts/rockchip/rk3399-nanopc-t4.dts
@@ -0,0 +1,18 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * FriendlyElec NanoPC-T4 board device tree source
+ *
+ * Copyright (c) 2018 FriendlyElec Computer Tech. Co., Ltd.
+ * (http://www.friendlyarm.com)
+ *
+ * Copyright (c) 2018 Collabora Ltd.
+ */
+
+/dts-v1/;
+#include "rk3399-nanopi4.dtsi"
+
+/ {
+   model = "FriendlyElec NanoPC-T4";
+   compatible = "friendlyarm,nanopc-t4", "rockchip,rk3399";
+};
+
diff --git a/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi 
b/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi
new file mode 100644
index ..148f85b4bd49
--- /dev/null
+++ b/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi
@@ -0,0 +1,770 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * RK3399-based FriendlyElec boards device tree source
+ *
+ * Copyright (c) 2016 Fuzhou Rockchip Electronics Co., Ltd
+ *
+ * Copyright (c) 2018 FriendlyElec Computer Tech. Co., Ltd.
+ * (http://www.friendlyarm.com)
+ *
+ * Copyright (c) 2018 Collabora Ltd.
+ */
+
+/dts-v1/;
+#include 
+#include "rk3399.dtsi"
+#include "rk3399-opp.dtsi"
+
+/ {
+   chosen {
+   stdout-path = "serial2:150n8";
+   };
+
+   clkin_gmac: external-gmac-clock {
+   compatible = "fixed-clock";
+   clock-frequency = <12500>;
+   clock-output-names = "clkin_gmac";
+   #clock-cells = <0>;
+   };
+
+   vcc3v3_sys: vcc3v3-sys {
+   compatible = "regulator-fixed";
+   regulator-name = "vcc3v3_sys";
+   regulator-always-on;
+   regulator-boot-on;
+   regulator-min-microvolt = <330>;
+   regulator-max-microvolt = <330>;
+   };
+
+   vcc5v0_host: vcc5v0-host-regulator {
+   compatible = "regulator-fixed";
+   regulator-name = "vcc5v0_host";
+   regulator-always-on;
+   regulator-boot-on;
+   regulator-min-microvolt = <500>;
+   regulator-max-microvolt = <500>;
+   };
+
+   vcc5v0_sys: vcc5v0-sys {
+   compatible = "regulator-fixed";
+   regulator-name = "vcc5v0_sys";
+   regulator-always-on;
+   regulator-boot-on;
+   regulator-min-microvolt = <500>;
+   regulator-max-microvolt = <500>;
+   };
+
+   vccadc_ref: 

[PATCH] arm64: dts: rockchip: Add DT for nanopc-t4

2018-11-22 Thread Tomeu Vizoso
This adds a device tree for the NanoPC-T4 SBC, which is based on the
Rockchip RK3399 SoC and marketed by FriendlyELEC.

Known working:

- Serial
- Ethernet
- HDMI
- USB 2.0

All of the interesting stuff is in a .dtsi because there are at least
two other boards that share most of it: NanoPi M4 and NanoPi NEO4.

Signed-off-by: Tomeu Vizoso 
---
 .../devicetree/bindings/arm/rockchip.txt  |   4 +
 arch/arm64/boot/dts/rockchip/Makefile |   1 +
 .../boot/dts/rockchip/rk3399-nanopc-t4.dts|  18 +
 .../boot/dts/rockchip/rk3399-nanopi4.dtsi | 782 ++
 4 files changed, 805 insertions(+)
 create mode 100644 arch/arm64/boot/dts/rockchip/rk3399-nanopc-t4.dts
 create mode 100644 arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi

diff --git a/Documentation/devicetree/bindings/arm/rockchip.txt 
b/Documentation/devicetree/bindings/arm/rockchip.txt
index 0cc71236d639..57e048dfec1f 100644
--- a/Documentation/devicetree/bindings/arm/rockchip.txt
+++ b/Documentation/devicetree/bindings/arm/rockchip.txt
@@ -71,6 +71,10 @@ Rockchip platforms device tree bindings
 Required root node properties:
   - compatible = "firefly,roc-rk3399-pc", "rockchip,rk3399";
 
+- FriendlyElec NanoPC-T4 board:
+Required root node properties:
+  - compatible = "friendlyelec,nanopc-t4", "rockchip,rk3399";
+
 - ChipSPARK PopMetal-RK3288 board:
 Required root node properties:
   - compatible = "chipspark,popmetal-rk3288", "rockchip,rk3288";
diff --git a/arch/arm64/boot/dts/rockchip/Makefile 
b/arch/arm64/boot/dts/rockchip/Makefile
index 49042c477870..ed90cd1e5a8b 100644
--- a/arch/arm64/boot/dts/rockchip/Makefile
+++ b/arch/arm64/boot/dts/rockchip/Makefile
@@ -20,3 +20,4 @@ dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-rock960.dtb
 dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-rockpro64.dtb
 dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-sapphire.dtb
 dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-sapphire-excavator.dtb
+dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-nanopc-t4.dtb
diff --git a/arch/arm64/boot/dts/rockchip/rk3399-nanopc-t4.dts 
b/arch/arm64/boot/dts/rockchip/rk3399-nanopc-t4.dts
new file mode 100644
index ..8e716c790d60
--- /dev/null
+++ b/arch/arm64/boot/dts/rockchip/rk3399-nanopc-t4.dts
@@ -0,0 +1,18 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * FriendlyElec NanoPC-T4 board device tree source
+ *
+ * Copyright (c) 2018 FriendlyElec Computer Tech. Co., Ltd.
+ * (http://www.friendlyarm.com)
+ *
+ * Copyright (c) 2018 Collabora Ltd.
+ */
+
+/dts-v1/;
+#include "rk3399-nanopi4.dtsi"
+
+/ {
+   model = "FriendlyElec NanoPC-T4";
+   compatible = "friendlyelec,nanopc-t4", "rockchip,rk3399";
+};
+
diff --git a/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi 
b/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi
new file mode 100644
index ..01be5cc57213
--- /dev/null
+++ b/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi
@@ -0,0 +1,782 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * RK3399-based FriendlyElec boards device tree source
+ *
+ * Copyright (c) 2016 Fuzhou Rockchip Electronics Co., Ltd
+ *
+ * Copyright (c) 2018 FriendlyElec Computer Tech. Co., Ltd.
+ * (http://www.friendlyarm.com)
+ *
+ * Copyright (c) 2018 Collabora Ltd.
+ */
+
+/dts-v1/;
+#include 
+#include "rk3399.dtsi"
+#include "rk3399-opp.dtsi"
+
+/ {
+   chosen {
+   stdout-path = "serial2:150n8";
+   };
+
+   clkin_gmac: external-gmac-clock {
+   compatible = "fixed-clock";
+   clock-frequency = <12500>;
+   clock-output-names = "clkin_gmac";
+   #clock-cells = <0>;
+   };
+
+   vcc3v3_sys: vcc3v3-sys {
+   compatible = "regulator-fixed";
+   regulator-name = "vcc3v3_sys";
+   regulator-always-on;
+   regulator-boot-on;
+   regulator-min-microvolt = <330>;
+   regulator-max-microvolt = <330>;
+   };
+
+   vcc5v0_host: vcc5v0-host-regulator {
+   compatible = "regulator-fixed";
+   regulator-name = "vcc5v0_host";
+   regulator-always-on;
+   regulator-boot-on;
+   regulator-min-microvolt = <500>;
+   regulator-max-microvolt = <500>;
+   };
+
+   vcc5v0_sys: vcc5v0-sys {
+   compatible = "regulator-fixed";
+   regulator-name = "vcc5v0_sys";
+   regulator-always-on;
+   regulator-boot-on;
+   regulator-min-microvolt = <500>;
+   regulator-max-microvolt = <500>;
+   };
+
+   vccadc_ref: vccadc-ref {
+   compatible = "regulator-fixed";
+   regulator-name = "v

Re: [PATCH v4] usb: dwc2: dwc2_vbus_supply_init: fix error check

2018-04-23 Thread Tomeu Vizoso
Hi,

could this patch be picked up, please? Or if for some reason it cannot
be, could the commit that introduced the regression be reverted?

It's causing some tests in KernelCI to fail:

https://storage.kernelci.org/next/master/next-20180423/arm/multi_v7_defconfig/lab-collabora/sleep-rk3288-veyron-jaq.html

Thanks,

Tomeu


On 11 April 2018 at 08:50, Minas Harutyunyan
 wrote:
> Hi Heiko,
>
> On 4/10/2018 7:37 PM, Heiko Stübner wrote:
>> Am Dienstag, 10. April 2018, 15:52:25 CEST schrieb Minas Harutyunyan:
>>> Hi Heiko,
>>>
>>> On 4/10/2018 4:28 PM, Heiko Stuebner wrote:
>>>> Am Montag, 26. März 2018, 11:00:01 CEST schrieb Tomeu Vizoso:
>>>>> devm_regulator_get_optional returns -ENODEV if the regulator isn't
>>>>> there, so if that's the case we have to make sure not to leave -ENODEV
>>>>> in the regulator pointer.
>>>>>
>>>>> Also, make sure we return 0 in that case, but correctly propagate any
>>>>> other errors. Also propagate the error from _dwc2_hcd_start.
>>>>>
>>>>> Fixes: 531ef5ebea96 ("usb: dwc2: add support for host mode external vbus
>>>>> supply") Cc: Amelie Delaunay 
>>>>> Signed-off-by: Tomeu Vizoso 
>>>>
>>>> The patch that gets fixed here also breaks display-output on dwc2-based
>>>> Rockchip devices (likely even more), probably due to making the regulator
>>>> framework hickup.
>>>
>>> Could you please elaborate what mean "breaks display-output".
>>> On which Kernel version you apply this patch?
>>
>> I think I may have written that poorly. _Without_ this patch I get
>> display breakage on the most recent torvalds/master (merge-window)
>> where "usb: dwc2: add support for host mode external vbus supply" is
>> applied and this patch fixes the issue.
>>
>> "breaks display output" means both hdmi + edp output are missing
>> also including the backlight staying off.
>>
>> The patch we're fixing here, causes a null-pointer dereference in the
>> regulator framework, which seems to also cause issues when other
>> regulators are enabled, which I think is what I'm seeing here.
>>
>>
> Thank you for clarification. Earlier you added "reviewed" tag, this
> feedback can assumed as "tested" tag.
>
> Thanks,
> Minas
>
>> Heiko
>>
>>>
>>> Thanks,
>>> Minas
>>>
>>>> With this patch applied, apart from not seeing the NULL-ptr, I also get
>>>> display output on my rk3288-veycron Chromebook again, so
>>>>
>>>> Tested-by: Heiko Stuebner 
>>>>
>>>>> v2: Only overwrite the error in the pointer after checking it (Heiko
>>>>>
>>>>>   Stübner )
>>>>>
>>>>> v3: Remove hunks that shouldn't be in this patch
>>>>> v4: Don't overwrite the error code before returning it (kbuild test
>>>>>
>>>>>   robot )
>>>>>
>>>>> ---
>>>>>
>>>>>drivers/usb/dwc2/hcd.c | 13 -
>>>>>1 file changed, 8 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
>>>>> index 190f95964000..c51b73b3e048 100644
>>>>> --- a/drivers/usb/dwc2/hcd.c
>>>>> +++ b/drivers/usb/dwc2/hcd.c
>>>>> @@ -358,9 +358,14 @@ static void dwc2_gusbcfg_init(struct dwc2_hsotg
>>>>> *hsotg)>>
>>>>>static int dwc2_vbus_supply_init(struct dwc2_hsotg *hsotg)
>>>>>{
>>>>>
>>>>> +  int ret;
>>>>> +
>>>>>
>>>>>hsotg->vbus_supply = devm_regulator_get_optional(hsotg->dev, 
>>>>> "vbus");
>>>>>
>>>>> -  if (IS_ERR(hsotg->vbus_supply))
>>>>> -  return 0;
>>>>> +  if (IS_ERR(hsotg->vbus_supply)) {
>>>>> +  ret = PTR_ERR(hsotg->vbus_supply);
>>>>> +  hsotg->vbus_supply = NULL;
>>>>> +  return ret == -ENODEV ? 0 : ret;
>>>>> +  }
>>>>>
>>>>>return regulator_enable(hsotg->vbus_supply);
>>>>>
>>>>>}
>>>>>
>>>>> @@ -4342,9 +4347,7 @@ static int _dwc2_hcd_start(struct usb_hcd *hcd)
>>>>>
>>>>>spin_unlock_irqrestore(&hsotg->lock, flags);
>>>>>
>>>>> -  dwc2_vbus_supply_init(hsotg);
>>>>> -
>>>>> -  return 0;
>>>>> +  return dwc2_vbus_supply_init(hsotg);
>>>>>
>>>>>}
>>>>>
>>>>>/*
>>
>>
>>
>


Re: [PATCH v8 07/14] ARM: dts: rockchip: add clocks in iommu nodes

2018-04-09 Thread Tomeu Vizoso
Hi there,

in today's linux-next, the DRM driver fails to probe because the iommu
driver fails to find the aclk. I need to apply this patch for things
to work again.

Thanks,

Tomeu

On 23 March 2018 at 08:38, Jeffy Chen  wrote:
> Add clocks in iommu nodes, since we are going to control clocks in
> rockchip iommu driver.
>
> Signed-off-by: Jeffy Chen 
> ---
>
> Changes in v8: None
> Changes in v7: None
> Changes in v6:
> Add clk names, and modify all iommu nodes in all existing rockchip dts
>
> Changes in v5:
> Remove clk names.
>
> Changes in v4: None
> Changes in v3: None
> Changes in v2: None
>
>  arch/arm/boot/dts/rk3036.dtsi|  2 ++
>  arch/arm/boot/dts/rk322x.dtsi|  8 
>  arch/arm/boot/dts/rk3288.dtsi| 12 
>  arch/arm64/boot/dts/rockchip/rk3328.dtsi | 10 ++
>  arch/arm64/boot/dts/rockchip/rk3368.dtsi | 10 ++
>  arch/arm64/boot/dts/rockchip/rk3399.dtsi | 14 --
>  6 files changed, 54 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/boot/dts/rk3036.dtsi b/arch/arm/boot/dts/rk3036.dtsi
> index a97458112ff6..567a6a725f9c 100644
> --- a/arch/arm/boot/dts/rk3036.dtsi
> +++ b/arch/arm/boot/dts/rk3036.dtsi
> @@ -197,6 +197,8 @@
> reg = <0x10118300 0x100>;
> interrupts = ;
> interrupt-names = "vop_mmu";
> +   clocks = <&cru ACLK_LCDC>, <&cru HCLK_LCDC>;
> +   clock-names = "aclk", "iface";
> #iommu-cells = <0>;
> status = "disabled";
> };
> diff --git a/arch/arm/boot/dts/rk322x.dtsi b/arch/arm/boot/dts/rk322x.dtsi
> index df1e47858675..be80e9a2c9af 100644
> --- a/arch/arm/boot/dts/rk322x.dtsi
> +++ b/arch/arm/boot/dts/rk322x.dtsi
> @@ -584,6 +584,8 @@
> reg = <0x20020800 0x100>;
> interrupts = ;
> interrupt-names = "vpu_mmu";
> +   clocks = <&cru ACLK_VPU>, <&cru HCLK_VPU>;
> +   clock-names = "aclk", "iface";
> iommu-cells = <0>;
> status = "disabled";
> };
> @@ -593,6 +595,8 @@
> reg = <0x20030480 0x40>, <0x200304c0 0x40>;
> interrupts = ;
> interrupt-names = "vdec_mmu";
> +   clocks = <&cru ACLK_RKVDEC>, <&cru HCLK_RKVDEC>;
> +   clock-names = "aclk", "iface";
> iommu-cells = <0>;
> status = "disabled";
> };
> @@ -602,6 +606,8 @@
> reg = <0x20053f00 0x100>;
> interrupts = ;
> interrupt-names = "vop_mmu";
> +   clocks = <&cru ACLK_VOP>, <&cru HCLK_VOP>;
> +   clock-names = "aclk", "iface";
> iommu-cells = <0>;
> status = "disabled";
> };
> @@ -611,6 +617,8 @@
> reg = <0x20070800 0x100>;
> interrupts = ;
> interrupt-names = "iep_mmu";
> +   clocks = <&cru ACLK_IEP>, <&cru HCLK_IEP>;
> +   clock-names = "aclk", "iface";
> iommu-cells = <0>;
> status = "disabled";
> };
> diff --git a/arch/arm/boot/dts/rk3288.dtsi b/arch/arm/boot/dts/rk3288.dtsi
> index be9acb6d28a1..d7e49d29ace5 100644
> --- a/arch/arm/boot/dts/rk3288.dtsi
> +++ b/arch/arm/boot/dts/rk3288.dtsi
> @@ -959,6 +959,8 @@
> reg = <0x0 0xff900800 0x0 0x40>;
> interrupts = ;
> interrupt-names = "iep_mmu";
> +   clocks = <&cru ACLK_IEP>, <&cru HCLK_IEP>;
> +   clock-names = "aclk", "iface";
> #iommu-cells = <0>;
> status = "disabled";
> };
> @@ -968,6 +970,8 @@
> reg = <0x0 0xff914000 0x0 0x100>, <0x0 0xff915000 0x0 0x100>;
> interrupts = ;
> interrupt-names = "isp_mmu";
> +   clocks = <&cru ACLK_ISP>, <&cru HCLK_ISP>;
> +   clock-names = "aclk", "iface";
> #iommu-cells = <0>;
> rockchip,disable-mmu-reset;
> status = "disabled";
> @@ -1027,6 +1031,8 @@
> reg = <0x0 0xff930300 0x0 0x100>;
> interrupts = ;
> interrupt-names = "vopb_mmu";
> +   clocks = <&cru ACLK_VOP0>, <&cru HCLK_VOP0>;
> +   clock-names = "aclk", "iface";
> power-domains = <&power RK3288_PD_VIO>;
> #iommu-cells = <0>;
> status = "disabled";
> @@ -1075,6 +1081,8 @@
> reg = <0x0 0xff940300 0x0 0x100>;
> interrupts = ;
> interrupt-names = "vopl_mmu";
> +   clocks = <&cru ACLK_VOP1>, <&cru HCLK_VOP1>;
> +   clock-names = "aclk", "iface";
> power-domains = <&power RK3288_PD_VIO>;
> #iommu-cells = <0>;
> status = "disabled";
> @@ -1206,6 +1214,8 @@
> reg = <0x0 0xff9a0800 0x0 0

Re: [PATCH v4] usb: dwc2: dwc2_vbus_supply_init: fix error check

2018-04-05 Thread Tomeu Vizoso

Hi Minas,

On 04/05/2018 09:54 AM, Minas Harutyunyan wrote:

Hi Tomeu,

On 3/26/2018 1:01 PM, Tomeu Vizoso wrote:

devm_regulator_get_optional returns -ENODEV if the regulator isn't
there, so if that's the case we have to make sure not to leave -ENODEV
in the regulator pointer.

Also, make sure we return 0 in that case, but correctly propagate any
other errors. Also propagate the error from _dwc2_hcd_start.

Fixes: 531ef5ebea96 ("usb: dwc2: add support for host mode external vbus 
supply")
Cc: Amelie Delaunay 
Signed-off-by: Tomeu Vizoso 

---

v2: Only overwrite the error in the pointer after checking it (Heiko
  Stübner )
v3: Remove hunks that shouldn't be in this patch
v4: Don't overwrite the error code before returning it (kbuild test
  robot )
---
   drivers/usb/dwc2/hcd.c | 13 -
   1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
index 190f95964000..c51b73b3e048 100644
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -358,9 +358,14 @@ static void dwc2_gusbcfg_init(struct dwc2_hsotg *hsotg)
   
   static int dwc2_vbus_supply_init(struct dwc2_hsotg *hsotg)

   {
+   int ret;
+
hsotg->vbus_supply = devm_regulator_get_optional(hsotg->dev, "vbus");
-   if (IS_ERR(hsotg->vbus_supply))
-   return 0;
+   if (IS_ERR(hsotg->vbus_supply)) {
+   ret = PTR_ERR(hsotg->vbus_supply);
+   hsotg->vbus_supply = NULL;
+   return ret == -ENODEV ? 0 : ret;
+   }
   
   	return regulator_enable(hsotg->vbus_supply);

   }
@@ -4342,9 +4347,7 @@ static int _dwc2_hcd_start(struct usb_hcd *hcd)
   
   	spin_unlock_irqrestore(&hsotg->lock, flags);
   
-	dwc2_vbus_supply_init(hsotg);

-
-   return 0;
+   return dwc2_vbus_supply_init(hsotg);
   }
   
   /*




Not able to apply patch. Please rebase to balbi/next


Are you sure? Just rebased and the resulting patch is identical to what 
was sent.


Thanks,

Tomeu


Re: [PATCH v4] usb: dwc2: dwc2_vbus_supply_init: fix error check

2018-04-04 Thread Tomeu Vizoso
Could this patch be picked up, please?

Thanks,

Tomeu

On 26 March 2018 at 13:51, Heiko Stübner  wrote:
> Am Montag, 26. März 2018, 11:00:01 CEST schrieb Tomeu Vizoso:
>> devm_regulator_get_optional returns -ENODEV if the regulator isn't
>> there, so if that's the case we have to make sure not to leave -ENODEV
>> in the regulator pointer.
>>
>> Also, make sure we return 0 in that case, but correctly propagate any
>> other errors. Also propagate the error from _dwc2_hcd_start.
>>
>> Fixes: 531ef5ebea96 ("usb: dwc2: add support for host mode external vbus
>> supply") Cc: Amelie Delaunay 
>> Signed-off-by: Tomeu Vizoso 
>
> Reviewed-by: Heiko Stuebner 


[PATCH v4] usb: dwc2: dwc2_vbus_supply_init: fix error check

2018-03-26 Thread Tomeu Vizoso
devm_regulator_get_optional returns -ENODEV if the regulator isn't
there, so if that's the case we have to make sure not to leave -ENODEV
in the regulator pointer.

Also, make sure we return 0 in that case, but correctly propagate any
other errors. Also propagate the error from _dwc2_hcd_start.

Fixes: 531ef5ebea96 ("usb: dwc2: add support for host mode external vbus 
supply")
Cc: Amelie Delaunay 
Signed-off-by: Tomeu Vizoso 

---

v2: Only overwrite the error in the pointer after checking it (Heiko
Stübner )
v3: Remove hunks that shouldn't be in this patch
v4: Don't overwrite the error code before returning it (kbuild test
robot )
---
 drivers/usb/dwc2/hcd.c | 13 -
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
index 190f95964000..c51b73b3e048 100644
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -358,9 +358,14 @@ static void dwc2_gusbcfg_init(struct dwc2_hsotg *hsotg)
 
 static int dwc2_vbus_supply_init(struct dwc2_hsotg *hsotg)
 {
+   int ret;
+
hsotg->vbus_supply = devm_regulator_get_optional(hsotg->dev, "vbus");
-   if (IS_ERR(hsotg->vbus_supply))
-   return 0;
+   if (IS_ERR(hsotg->vbus_supply)) {
+   ret = PTR_ERR(hsotg->vbus_supply);
+   hsotg->vbus_supply = NULL;
+   return ret == -ENODEV ? 0 : ret;
+   }
 
return regulator_enable(hsotg->vbus_supply);
 }
@@ -4342,9 +4347,7 @@ static int _dwc2_hcd_start(struct usb_hcd *hcd)
 
spin_unlock_irqrestore(&hsotg->lock, flags);
 
-   dwc2_vbus_supply_init(hsotg);
-
-   return 0;
+   return dwc2_vbus_supply_init(hsotg);
 }
 
 /*
-- 
2.14.3



[PATCH] usb: hub: Reduce warning to notice on power loss

2018-03-22 Thread Tomeu Vizoso
Currently we warn the user when the root hub lost power after resume,
but the user cannot do anything about it so it should probably be a
notice.

This will reduce the noise in the console during suspend and resume,
which is already quite significant in many systems.

Signed-off-by: Tomeu Vizoso 
---
 drivers/usb/core/hub.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index ac7bab772a3a..5964008003b4 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -3655,7 +3655,7 @@ static int hub_reset_resume(struct usb_interface *intf)
  */
 void usb_root_hub_lost_power(struct usb_device *rhdev)
 {
-   dev_warn(&rhdev->dev, "root hub lost power or was reset\n");
+   dev_notice(&rhdev->dev, "root hub lost power or was reset\n");
rhdev->reset_resume = 1;
 }
 EXPORT_SYMBOL_GPL(usb_root_hub_lost_power);
-- 
2.14.3



[PATCH v1] printk: change message to pr_info

2018-03-22 Thread Tomeu Vizoso
To allow userspace to prevent this message from appearing in the
console by changing the log priority.

This matches other informative messages that the power subsystem emits
when the system changes power states.

Signed-off-by: Tomeu Vizoso 
---
 kernel/printk/printk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 63f4c87f7a3d..704e55129c3a 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2161,7 +2161,7 @@ void suspend_console(void)
 {
if (!console_suspend_enabled)
return;
-   printk("Suspending console(s) (use no_console_suspend to debug)\n");
+   pr_info("Suspending console(s) (use no_console_suspend to debug)\n");
console_lock();
console_suspended = 1;
up_console_sem();
-- 
2.14.3



Re: [PATCH v3] usb: dwc2: dwc2_vbus_supply_init: fix error check

2018-03-22 Thread Tomeu Vizoso

On 03/22/2018 02:26 PM, Heiko Stübner wrote:

Am Donnerstag, 22. März 2018, 14:14:51 CET schrieb Tomeu Vizoso:

devm_regulator_get_optional returns -ENODEV if the regulator isn't
there, so if that's the case we have to make sure not to leave -ENODEV
in the regulator pointer.

Also, make sure we return 0 in that case, but correctly propagate any
other errors. Also propagate the error from _dwc2_hcd_start.

Fixes: 531ef5ebea96 ("usb: dwc2: add support for host mode external vbus
supply") Cc: Amelie Delaunay 
Signed-off-by: Tomeu Vizoso 

---

v2: Only overwrite the error in the pointer after checking it (Heiko
 Stübner )
v3: Remove hunks that shouldn't be in this patch
---
  drivers/usb/dwc2/hcd.c | 11 +++
  1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
index dcfda5eb4cac..863aed20517f 100644
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -359,8 +359,13 @@ static void dwc2_gusbcfg_init(struct dwc2_hsotg *hsotg)
static int dwc2_vbus_supply_init(struct dwc2_hsotg *hsotg)
  {
hsotg->vbus_supply = devm_regulator_get_optional(hsotg->dev, "vbus");
-   if (IS_ERR(hsotg->vbus_supply))
+   if (PTR_ERR(hsotg->vbus_supply) == -ENODEV) {
+   hsotg->vbus_supply = NULL;
return 0;
+   } else if (IS_ERR(hsotg->vbus_supply)) {
+   hsotg->vbus_supply = NULL;
+   return PTR_ERR(hsotg->vbus_supply);
+   }


my personal cluelessnes, but can you use PTR_ERR without checking for
IS_ERR first?


It's just a cast, so it should be fine.


I would've expected something along the line of

if (IS_ERR(hsotg->vbus_supply)) {
if (PTR_ERR(hsotg->vbus_supply) == -ENODEV) {
hsotg->vbus_supply = NULL;
return 0;
} else {
return PTR_ERR(hsotg->vbus_supply);
}
}


I kind of liked to avoid one indentation level. Also wanted to play safe 
and NULLify the pointer in both code paths.


Thanks,

Tomeu


[PATCH v3] usb: dwc2: dwc2_vbus_supply_init: fix error check

2018-03-22 Thread Tomeu Vizoso
devm_regulator_get_optional returns -ENODEV if the regulator isn't
there, so if that's the case we have to make sure not to leave -ENODEV
in the regulator pointer.

Also, make sure we return 0 in that case, but correctly propagate any
other errors. Also propagate the error from _dwc2_hcd_start.

Fixes: 531ef5ebea96 ("usb: dwc2: add support for host mode external vbus 
supply")
Cc: Amelie Delaunay 
Signed-off-by: Tomeu Vizoso 

---

v2: Only overwrite the error in the pointer after checking it (Heiko
Stübner )
v3: Remove hunks that shouldn't be in this patch
---
 drivers/usb/dwc2/hcd.c | 11 +++
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
index dcfda5eb4cac..863aed20517f 100644
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -359,8 +359,13 @@ static void dwc2_gusbcfg_init(struct dwc2_hsotg *hsotg)
 static int dwc2_vbus_supply_init(struct dwc2_hsotg *hsotg)
 {
hsotg->vbus_supply = devm_regulator_get_optional(hsotg->dev, "vbus");
-   if (IS_ERR(hsotg->vbus_supply))
+   if (PTR_ERR(hsotg->vbus_supply) == -ENODEV) {
+   hsotg->vbus_supply = NULL;
return 0;
+   } else if (IS_ERR(hsotg->vbus_supply)) {
+   hsotg->vbus_supply = NULL;
+   return PTR_ERR(hsotg->vbus_supply);
+   }
 
return regulator_enable(hsotg->vbus_supply);
 }
@@ -4342,9 +4347,7 @@ static int _dwc2_hcd_start(struct usb_hcd *hcd)
 
spin_unlock_irqrestore(&hsotg->lock, flags);
 
-   dwc2_vbus_supply_init(hsotg);
-
-   return 0;
+   return dwc2_vbus_supply_init(hsotg);
 }
 
 /*
-- 
2.14.3



[PATCH v2] usb: dwc2: dwc2_vbus_supply_init: fix error check

2018-03-22 Thread Tomeu Vizoso
devm_regulator_get_optional returns -ENODEV if the regulator isn't
there, so if that's the case we have to make sure not to leave -ENODEV
in the regulator pointer.

Also, make sure we return 0 in that case, but correctly propagate any
other errors. Also propagate the error from _dwc2_hcd_start.

Fixes: 531ef5ebea96 ("usb: dwc2: add support for host mode external vbus 
supply")
Cc: Amelie Delaunay 
Signed-off-by: Tomeu Vizoso 

---

v2: Only overwrite the error in the pointer after checking it (Heiko
Stübner )
---
 arch/arm/configs/multi_v7_defconfig |  3 +++
 drivers/usb/dwc2/hcd.c  | 11 +++
 scripts/setlocalversion |  9 -
 3 files changed, 10 insertions(+), 13 deletions(-)

diff --git a/arch/arm/configs/multi_v7_defconfig 
b/arch/arm/configs/multi_v7_defconfig
index 846ce7bb24bc..33148fcabd17 100644
--- a/arch/arm/configs/multi_v7_defconfig
+++ b/arch/arm/configs/multi_v7_defconfig
@@ -1029,3 +1029,6 @@ CONFIG_VIRTIO=y
 CONFIG_VIRTIO_PCI=y
 CONFIG_VIRTIO_PCI_LEGACY=y
 CONFIG_VIRTIO_MMIO=y
+CONFIG_LOCALVERSION_AUTO=n
+CONFIG_LOCALVERSION=""
+
diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
index dcfda5eb4cac..863aed20517f 100644
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -359,8 +359,13 @@ static void dwc2_gusbcfg_init(struct dwc2_hsotg *hsotg)
 static int dwc2_vbus_supply_init(struct dwc2_hsotg *hsotg)
 {
hsotg->vbus_supply = devm_regulator_get_optional(hsotg->dev, "vbus");
-   if (IS_ERR(hsotg->vbus_supply))
+   if (PTR_ERR(hsotg->vbus_supply) == -ENODEV) {
+   hsotg->vbus_supply = NULL;
return 0;
+   } else if (IS_ERR(hsotg->vbus_supply)) {
+   hsotg->vbus_supply = NULL;
+   return PTR_ERR(hsotg->vbus_supply);
+   }
 
return regulator_enable(hsotg->vbus_supply);
 }
@@ -4342,9 +4347,7 @@ static int _dwc2_hcd_start(struct usb_hcd *hcd)
 
spin_unlock_irqrestore(&hsotg->lock, flags);
 
-   dwc2_vbus_supply_init(hsotg);
-
-   return 0;
+   return dwc2_vbus_supply_init(hsotg);
 }
 
 /*
diff --git a/scripts/setlocalversion b/scripts/setlocalversion
index 71f39410691b..cbc36d3b4d0f 100755
--- a/scripts/setlocalversion
+++ b/scripts/setlocalversion
@@ -161,15 +161,6 @@ res="${res}${CONFIG_LOCALVERSION}${LOCALVERSION}"
 if test "$CONFIG_LOCALVERSION_AUTO" = "y"; then
# full scm version string
res="$res$(scm_version)"
-else
-   # append a plus sign if the repository is not in a clean
-   # annotated or signed tagged state (as git describe only
-   # looks at signed or annotated tags - git tag -a/-s) and
-   # LOCALVERSION= is not specified
-   if test "${LOCALVERSION+set}" != "set"; then
-   scm=$(scm_version --short)
-   res="$res${scm:++}"
-   fi
 fi
 
 echo "$res"
-- 
2.14.3



[PATCH] usb: dwc2: dwc2_vbus_supply_init: fix error check

2018-03-22 Thread Tomeu Vizoso
devm_regulator_get_optional returns -ENODEV if the regulator isn't
there, so if that's the case we have to make sure not to leave -ENODEV
in the regulator pointer.

Also, make sure we return 0 in that case, but correctly propagate any
other errors. Also propagate the error from _dwc2_hcd_start.

Fixes: 531ef5ebea96 ("usb: dwc2: add support for host mode external vbus 
supply")
Cc: Amelie Delaunay 
Signed-off-by: Tomeu Vizoso 
---
 drivers/usb/dwc2/hcd.c | 13 -
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
index dcfda5eb4cac..4ae211f65e85 100644
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -359,8 +359,13 @@ static void dwc2_gusbcfg_init(struct dwc2_hsotg *hsotg)
 static int dwc2_vbus_supply_init(struct dwc2_hsotg *hsotg)
 {
hsotg->vbus_supply = devm_regulator_get_optional(hsotg->dev, "vbus");
-   if (IS_ERR(hsotg->vbus_supply))
-   return 0;
+   if (IS_ERR(hsotg->vbus_supply)) {
+   hsotg->vbus_supply = NULL;
+   if (PTR_ERR(hsotg->vbus_supply) == -ENODEV)
+   return 0;
+   else
+   return PTR_ERR(hsotg->vbus_supply);
+   }
 
return regulator_enable(hsotg->vbus_supply);
 }
@@ -4342,9 +4347,7 @@ static int _dwc2_hcd_start(struct usb_hcd *hcd)
 
spin_unlock_irqrestore(&hsotg->lock, flags);
 
-   dwc2_vbus_supply_init(hsotg);
-
-   return 0;
+   return dwc2_vbus_supply_init(hsotg);
 }
 
 /*
-- 
2.14.3



Re: [PATCH v3 1/2] drm/virtio: Add window server support

2018-02-15 Thread Tomeu Vizoso
On 02/12/2018 12:45 PM, Gerd Hoffmann wrote: 4. QEMU pops 
data+buffers from the virtqueue, looks up shmem FD for each

 resource, sends data + FDs to the compositor with SCM_RIGHTS
>>>
>>> BTW: Is there a 1:1 relationship between buffers and shmem blocks?  Or
>>> does the wayland protocol allow for offsets in buffer meta data, so you
>>> can place multiple buffers in a single shmem block?
>>
>> The latter:
>> 
https://wayland.freedesktop.org/docs/html/apa.html#protocol-spec-wl_shm_pool

>
> Ah, good, that makes it alot easier.
>
> So, yes, using ivshmem would be one option.  Tricky part here is the
> buffer management though.  It's just a raw piece of memory.  The guest
> proxy could mmap the pci bar and manage it.  But then it is again either
> unmodified guest + copying the data, or modified client (which requests
> buffers from guest proxy) for zero-copy.

What if at VIRTIO_GPU_CMD_RESOURCE_CREATE_2D time we created a ivshmem 
device to back that resource. The ivshmem device would in turn be backed 
by a hostmem device that wraps a shmem FD.


The guest client can then export that resource/BO and pass the FD to the 
guest proxy. The guest proxy would import it and put the resource_id in 
the equivalent message in our protocol extension.


QEMU would get that resource id from vsock, look up which hostmem device 
is associated with that resource, and pass its FD to the compositor.


> We also need a solution for the keymap shmem block.  I guess the keymap
> doesn't change all that often, so maybe it is easiest to just copy it
> over (host proxy -> guest proxy) instead of trying to map the host shmem
> into the guest?

Not sure if that would be much simpler than creating a ivshmem+hostmem 
combo that wraps the incoming shmem FD and then having virtio-gpu create 
a BO that imports it.


Regards,

Tomeu


Re: [PATCH v3 1/2] drm/virtio: Add window server support

2018-02-13 Thread Tomeu Vizoso

On 02/12/2018 12:45 PM, Gerd Hoffmann wrote:

   Hi,


(a) software rendering: client allocates shared memory buffer, renders
into it, then passes a file handle for that shmem block together
with some meta data (size, format, ...) to the wayland server.

(b) gpu rendering: client opens a render node, allocates a buffer,
asks the cpu to renders into it, exports the buffer as dma-buf
(DRM_IOCTL_PRIME_HANDLE_TO_FD), passes this to the wayland server
(again including meta data of course).

Is that correct?


Both are correct descriptions of typical behaviors. But it isn't spec'ed
anywhere who has to do the buffer allocation.


Well, according to Pekka's reply it is spec'ed that way, for the
existing buffer types.  So for server allocated buffers you need
(a) a wayland protocol extension and (b) support for the extension
in the clients.


That's to say that if we cannot come up with a zero-copy solution for
unmodified clients, we should at least support zero-copy for cooperative
clients.


"cooperative clients" == "client which has support for the wayland
protocol extension", correct?


Guess it could be that, but I was rather thinking of clients that would 
allocate the buffer for wl_shm_pool with DRM_VIRTGPU_RESOURCE_CREATE or 
equivalent. Then that buffer would be exported and the fd passed using 
the standard wl_shm protocol.



4. QEMU maps that buffer to the guest's address space
(KVM_SET_USER_MEMORY_REGION), passes the guest PFN to the virtio driver


That part is problematic.  The host can't simply allocate something in
the physical address space, because most physical address space
management is done by the guest.  All pci bars are mapped by the guest
firmware for example (or by the guest OS in case of hotplug).


How can KVM_SET_USER_MEMORY_REGION ever be safely used then? I would have
expected that callers of that ioctl have enough knowledge to be able to
choose a physical address that won't conflict with the guest's kernel.


Depends on the kind of region.  Guest RAM is allocated and mapped by
qemu, guest firmware can query qemu about RAM mappings using a special
interface, then create a e820 memory map for the guest os.  PCI device
bars are mapped according to the pci config space registers, which in
turn are initialized by the guest firmware, so it is basically in the
guests hand where they show up.


I see that the ivshmem device in QEMU registers the memory region in BAR 2
of a PCI device instead. Would that be better in your opinion?


Yes.


Would it make sense for virtio-gpu to map buffers to the guest via PCI 
BARs? So we can use a single drm driver for both 2d and 3d.



4. QEMU pops data+buffers from the virtqueue, looks up shmem FD for each
resource, sends data + FDs to the compositor with SCM_RIGHTS


BTW: Is there a 1:1 relationship between buffers and shmem blocks?  Or
does the wayland protocol allow for offsets in buffer meta data, so you
can place multiple buffers in a single shmem block?


The latter:
https://wayland.freedesktop.org/docs/html/apa.html#protocol-spec-wl_shm_pool


Ah, good, that makes it alot easier.

So, yes, using ivshmem would be one option.  Tricky part here is the
buffer management though.  It's just a raw piece of memory.  The guest
proxy could mmap the pci bar and manage it.  But then it is again either
unmodified guest + copying the data, or modified client (which requests
buffers from guest proxy) for zero-copy.

Another idea would be extending stdvga.  Basically qemu would have to
use shmem as backing storage for vga memory instead of anonymous memory,
so it would be very  simliar to ivshmem on the host side.  But on the
guest side we have a drm driver for it (bochs-drm).  So clients can
allocate dumb drm buffers for software rendering, and the buffer would
already be backed by a host shmem segment.  Given that wayland already
supports drm buffers for 3d rendering that could work without extending
the wayland protocol.  The client proxy would have to translate the drm
buffer into an pci bar offset and pass it to the host side.  The host
proxy could register the pci bar as wl_shm_pool, then just pass through
the offset to reference the individual buffers.

Drawback of both approaches would be that software rendering and gpu
rendering would use quite different code paths.


Yeah, would be great if we could find a way to avoid that.


We also need a solution for the keymap shmem block.  I guess the keymap
doesn't change all that often, so maybe it is easiest to just copy it
over (host proxy -> guest proxy) instead of trying to map the host shmem
into the guest?


I think that should be fine for now. Something similar will have to 
happen for the clipboard, which currently uses pipes to exchange data.


Thanks,

Tomeu


Re: [PATCH v3 1/2] drm/virtio: Add window server support

2018-02-12 Thread Tomeu Vizoso

On 02/12/2018 03:27 PM, Gerd Hoffmann wrote:

On Mon, Feb 12, 2018 at 03:00:24PM +0100, Tomeu Vizoso wrote:

On 02/12/2018 12:52 PM, Gerd Hoffmann wrote:

Hi,


can we reach agreement on whether vsock should be involved in this?


I think the best approach would be to have guest proxy and host proxy
use vsock for the wayland protocol.  Use a wayland protocol extension to
reference the buffers in stdvga / ivshmem / virtio-gpu.  Only the two
proxies need to understand the extension, the client <=> guest proxy and
host proxy <=> server communication would be standard wayland protocol.


Thanks for the ideas. What I haven't understood yet is how you see the
actual passing of buffers via vsock. Are you thinking of using ancillary
data to pass FDs, or something else?


I was more thinking about a struct containing enough info to allow the
proxy on the host side find the buffer, something like:

struct {
   enum type { stdvga, virtio-cpu, ... }
   pcislot device;
   union {
  int stdvga_pcibar_offset;
  int virtio_gpu_resource_id;
   }
}

So when the guest proxy gets a message with a fd referencing a buffer it
would have to figure where the buffer is, rewrite the message into the
struct above for the host proxy.  The host proxy would rewrite the
message again for the server.


What I don't understand yet is how we can keep the buffer descriptions 
together with the protocol data that references them.


With SCM_RIGHTS, the FDs are placed in the ancillary data that "travels" 
together with the protocol data that references them.


With the present series, the DRM_IOCTL_VIRTGPU_WINSRV_TX ioctl struct has 
a field for the protocol data and an array of FDs.


How are you proposing to pass instances of that struct from above along 
the protocol data that refers to them?


Thanks,

Tomeu


Re: [PATCH v3 1/2] drm/virtio: Add window server support

2018-02-12 Thread Tomeu Vizoso

On 02/12/2018 12:52 PM, Gerd Hoffmann wrote:

   Hi,


can we reach agreement on whether vsock should be involved in this?


I think the best approach would be to have guest proxy and host proxy
use vsock for the wayland protocol.  Use a wayland protocol extension to
reference the buffers in stdvga / ivshmem / virtio-gpu.  Only the two
proxies need to understand the extension, the client <=> guest proxy and
host proxy <=> server communication would be standard wayland protocol.


Thanks for the ideas. What I haven't understood yet is how you see the 
actual passing of buffers via vsock. Are you thinking of using ancillary 
data to pass FDs, or something else?


Thanks,

Tomeu


Re: [PATCH v3 1/2] drm/virtio: Add window server support

2018-02-09 Thread Tomeu Vizoso

Hi Gerd and Stefan,

can we reach agreement on whether vsock should be involved in this?

Thanks,

Tomeu

On 02/07/2018 10:49 AM, Tomeu Vizoso wrote:

On 02/06/2018 03:23 PM, Gerd Hoffmann wrote:

   Hi,


Hmm?  I'm assuming the wayland client (in the guest) talks to the
wayland proxy, using the wayland protocol, like it would talk to a
wayland display server.  Buffers must be passed from client to
server/proxy somehow, probably using fd passing, so where is the
problem?

Or did I misunderstand the role of the proxy?


Hi Gerd,

it's starting to look to me that we're talking a bit past the other, so I
have pasted below a few words describing my current plan regarding the 
3 key

scenarios that I'm addressing.


You are describing the details, but I'm missing the big picture ...

So, virtualization aside, how do buffers work in wayland?  As far I know
it goes like this:

   (a) software rendering: client allocates shared memory buffer, renders
   into it, then passes a file handle for that shmem block together
   with some meta data (size, format, ...) to the wayland server.

   (b) gpu rendering: client opens a render node, allocates a buffer,
   asks the cpu to renders into it, exports the buffer as dma-buf
   (DRM_IOCTL_PRIME_HANDLE_TO_FD), passes this to the wayland server
   (again including meta data of course).

Is that correct?


Both are correct descriptions of typical behaviors. But it isn't spec'ed 
anywhere who has to do the buffer allocation.


In practical terms, the buffer allocation happens in either the 2D GUI 
toolkit (gtk+, for example), or the EGL implementation. Someone using 
this in a real product would most probably be interested in avoiding any 
extra copies and make sure that both allocate buffers via virtio-gpu, for 
example.


Depending on the use case, they could be also interested in supporting 
unmodified clients with an extra copy per buffer presentation.


That's to say that if we cannot come up with a zero-copy solution for 
unmodified clients, we should at least support zero-copy for cooperative 
clients.



Now, with virtualization added to the mix it becomes a bit more
complicated.  Client and server are unmodified.  The client talks to the
guest proxy (wayland protocol).  The guest proxy talks to the host proxy
(protocol to be defined). The host proxy talks to the server (wayland
protocol).

Buffers must be managed along the way, and we want avoid copying around
the buffers.  The host proxy could be implemented directly in qemu, or
as separate process which cooperates with qemu for buffer management.

Fine so far?


Yep.

I really think that whatever we come up with needs to support 3D 
clients as

well.


Lets start with 3d clients, I think these are easier.  They simply use
virtio-gpu for 3d rendering as usual.  When they are done the rendered
buffer already lives in a host drm buffer (because virgl runs the actual
rendering on the host gpu).  So the client passes the dma-buf to the
guest proxy, the guest proxy imports it to look up the resource-id,
passes the resource-id to the host proxy, the host proxy looks up the
drm buffer and exports it as dma-buf, then passes it to the server.
Done, without any extra data copies.


Yep.


Creation of shareable buffer by guest
-

1. Client requests virtio driver to create a buffer suitable for sharing
with host (DRM_VIRTGPU_RESOURCE_CREATE)


client or guest proxy?


As per the above, the GUI toolkit could have been modified so the client 
directly creates a shareable buffer, and renders directly to it without 
any extra copies.


If clients cannot be modified, then it's the guest proxy what has to 
create the shareable buffer and keep it in sync with the client's 
non-shareable buffer at the right times, by intercepting 
wl_surface.commit messages and copying buffer contents.



4. QEMU maps that buffer to the guest's address space
(KVM_SET_USER_MEMORY_REGION), passes the guest PFN to the virtio driver


That part is problematic.  The host can't simply allocate something in
the physical address space, because most physical address space
management is done by the guest.  All pci bars are mapped by the guest
firmware for example (or by the guest OS in case of hotplug).


How can KVM_SET_USER_MEMORY_REGION ever be safely used then? I would have 
expected that callers of that ioctl have enough knowledge to be able to 
choose a physical address that won't conflict with the guest's kernel.


I see that the ivshmem device in QEMU registers the memory region in BAR 
2 of a PCI device instead. Would that be better in your opinion?



4. QEMU pops data+buffers from the virtqueue, looks up shmem FD for each
resource, sends data + FDs to the compositor with SCM_RIGHTS


BTW: Is there a 1:1 relationship between buffers and shmem blocks?  Or
does the wayland protocol allow for offsets in buffer meta 

Re: [PATCH v3 1/2] drm/virtio: Add window server support

2018-02-07 Thread Tomeu Vizoso

On 02/06/2018 03:23 PM, Gerd Hoffmann wrote:

   Hi,


Hmm?  I'm assuming the wayland client (in the guest) talks to the
wayland proxy, using the wayland protocol, like it would talk to a
wayland display server.  Buffers must be passed from client to
server/proxy somehow, probably using fd passing, so where is the
problem?

Or did I misunderstand the role of the proxy?


Hi Gerd,

it's starting to look to me that we're talking a bit past the other, so I
have pasted below a few words describing my current plan regarding the 3 key
scenarios that I'm addressing.


You are describing the details, but I'm missing the big picture ...

So, virtualization aside, how do buffers work in wayland?  As far I know
it goes like this:

   (a) software rendering: client allocates shared memory buffer, renders
   into it, then passes a file handle for that shmem block together
   with some meta data (size, format, ...) to the wayland server.

   (b) gpu rendering: client opens a render node, allocates a buffer,
   asks the cpu to renders into it, exports the buffer as dma-buf
   (DRM_IOCTL_PRIME_HANDLE_TO_FD), passes this to the wayland server
   (again including meta data of course).

Is that correct?


Both are correct descriptions of typical behaviors. But it isn't spec'ed 
anywhere who has to do the buffer allocation.


In practical terms, the buffer allocation happens in either the 2D GUI 
toolkit (gtk+, for example), or the EGL implementation. Someone using 
this in a real product would most probably be interested in avoiding any 
extra copies and make sure that both allocate buffers via virtio-gpu, for 
example.


Depending on the use case, they could be also interested in supporting 
unmodified clients with an extra copy per buffer presentation.


That's to say that if we cannot come up with a zero-copy solution for 
unmodified clients, we should at least support zero-copy for cooperative 
clients.



Now, with virtualization added to the mix it becomes a bit more
complicated.  Client and server are unmodified.  The client talks to the
guest proxy (wayland protocol).  The guest proxy talks to the host proxy
(protocol to be defined). The host proxy talks to the server (wayland
protocol).

Buffers must be managed along the way, and we want avoid copying around
the buffers.  The host proxy could be implemented directly in qemu, or
as separate process which cooperates with qemu for buffer management.

Fine so far?


Yep.


I really think that whatever we come up with needs to support 3D clients as
well.


Lets start with 3d clients, I think these are easier.  They simply use
virtio-gpu for 3d rendering as usual.  When they are done the rendered
buffer already lives in a host drm buffer (because virgl runs the actual
rendering on the host gpu).  So the client passes the dma-buf to the
guest proxy, the guest proxy imports it to look up the resource-id,
passes the resource-id to the host proxy, the host proxy looks up the
drm buffer and exports it as dma-buf, then passes it to the server.
Done, without any extra data copies.


Yep.


Creation of shareable buffer by guest
-

1. Client requests virtio driver to create a buffer suitable for sharing
with host (DRM_VIRTGPU_RESOURCE_CREATE)


client or guest proxy?


As per the above, the GUI toolkit could have been modified so the client 
directly creates a shareable buffer, and renders directly to it without 
any extra copies.


If clients cannot be modified, then it's the guest proxy what has to 
create the shareable buffer and keep it in sync with the client's 
non-shareable buffer at the right times, by intercepting 
wl_surface.commit messages and copying buffer contents.



4. QEMU maps that buffer to the guest's address space
(KVM_SET_USER_MEMORY_REGION), passes the guest PFN to the virtio driver


That part is problematic.  The host can't simply allocate something in
the physical address space, because most physical address space
management is done by the guest.  All pci bars are mapped by the guest
firmware for example (or by the guest OS in case of hotplug).


How can KVM_SET_USER_MEMORY_REGION ever be safely used then? I would have 
expected that callers of that ioctl have enough knowledge to be able to 
choose a physical address that won't conflict with the guest's kernel.


I see that the ivshmem device in QEMU registers the memory region in BAR 
2 of a PCI device instead. Would that be better in your opinion?



4. QEMU pops data+buffers from the virtqueue, looks up shmem FD for each
resource, sends data + FDs to the compositor with SCM_RIGHTS


BTW: Is there a 1:1 relationship between buffers and shmem blocks?  Or
does the wayland protocol allow for offsets in buffer meta data, so you
can place multiple buffers in a single shmem block?


The latter: 
https://wayland.freedesktop.org/docs/html/apa.html#protocol-spec-wl_shm_pool


Regards,

Tomeu


Re: [PATCH v3 1/2] drm/virtio: Add window server support

2018-02-06 Thread Tomeu Vizoso

On 02/07/2018 02:09 AM, Michael S. Tsirkin wrote:

On Tue, Feb 06, 2018 at 03:23:02PM +0100, Gerd Hoffmann wrote:

Creation of shareable buffer by guest
-

1. Client requests virtio driver to create a buffer suitable for sharing
with host (DRM_VIRTGPU_RESOURCE_CREATE)


client or guest proxy?


4. QEMU maps that buffer to the guest's address space
(KVM_SET_USER_MEMORY_REGION), passes the guest PFN to the virtio driver


That part is problematic.  The host can't simply allocate something in
the physical address space, because most physical address space
management is done by the guest.  All pci bars are mapped by the guest
firmware for example (or by the guest OS in case of hotplug).


4. QEMU pops data+buffers from the virtqueue, looks up shmem FD for each
resource, sends data + FDs to the compositor with SCM_RIGHTS


If you squint hard, this sounds a bit like a use-case for vhost-user-gpu, does 
it not?


Can you extend on what makes you think that?

As an aside, crosvm runs the virtio-gpu device in a separate, jailed
process, among other virtual devices.

https://chromium.googlesource.com/chromiumos/platform/crosvm/

Regards,

Tomeu


Re: [PATCH v3 1/2] drm/virtio: Add window server support

2018-02-06 Thread Tomeu Vizoso

On 02/05/2018 05:03 PM, Gerd Hoffmann wrote:

On Mon, Feb 05, 2018 at 03:46:17PM +0100, Tomeu Vizoso wrote:

On 02/05/2018 01:20 PM, Gerd Hoffmann wrote:

Hi,


Why not use virtio-vsock to run the wayland protocol? I don't like
the idea to duplicate something with very simliar functionality in
virtio-gpu.


The reason for abandoning that approach was the type of objects that
could be shared via virtio-vsock would be extremely limited. Besides
that being potentially confusing to users, it would mean from the
implementation side that either virtio-vsock would gain a dependency on
the drm subsystem, or an appropriate abstraction for shareable buffers
would need to be added for little gain.


Well, no.  The idea is that virtio-vsock and virtio-gpu are used largely
as-is, without knowing about each other.  The guest wayland proxy which
does the buffer management talks to both devices.


Note that the proxy won't know anything about buffers if clients opt-in for
zero-copy support (they allocate the buffers in a way that allows for
sharing with the host).


Hmm?  I'm assuming the wayland client (in the guest) talks to the
wayland proxy, using the wayland protocol, like it would talk to a
wayland display server.  Buffers must be passed from client to
server/proxy somehow, probably using fd passing, so where is the
problem?

Or did I misunderstand the role of the proxy?


Hi Gerd,

it's starting to look to me that we're talking a bit past the other, so 
I have pasted below a few words describing my current plan regarding the 
3 key scenarios that I'm addressing.


I mention below KVM_SET_USER_MEMORY_REGION, but I guess we can discuss 
alternatives such as the one you are proposing using PCI BARs at a later 
stage.


I really think that whatever we come up with needs to support 3D clients 
as well.



Creation of shareable buffer by guest
-

1. Client requests virtio driver to create a buffer suitable for sharing 
with host (DRM_VIRTGPU_RESOURCE_CREATE)


2. Virtio driver creates a new resource ID and passes the request to 
QEMU (VIRTIO_GPU_CMD_RESOURCE_CREATE_2D)


3. QEMU creates a shmem file (for example with mkostemp), associates 
that FD with the ID of this resource


4. QEMU maps that buffer to the guest's address space 
(KVM_SET_USER_MEMORY_REGION), passes the guest PFN to the virtio driver


5. DRM_VIRTGPU_RESOURCE_CREATE returns the resource id just created

6. Client mmaps it with DRM_IOCTL_VIRTGPU_MAP+mmap and can render to it

7. Gets a FD with DRM_IOCTL_PRIME_HANDLE_TO_FD that can be sent around


Send of shareable buffer by guest
-

1. Client sends the host a message that refers to this buffer, passing 
the FD using SCM_RIGHTS


2. Guest proxy passes the message (serialized data + FDs) on to the 
virtio driver responsable for winsrv support


3. virtio driver puts the data and the resource ids corresponding to the 
FDs in a virtqueue, kicks it


4. QEMU pops data+buffers from the virtqueue, looks up shmem FD for each 
resource, sends data + FDs to the compositor with SCM_RIGHTS



Reception of buffer from the compositor
-

1. QEMU reads from the socket and gets a FD via SCM_RIGHTS

2. QEMU mmaps the FD and maps the resulting pointer to the guest via 
KVM_SET_USER_MEMORY_REGION


3. QEMU sends the guest PFN along the presentation data to the virtio 
driver (VIRTIO_GPU_CMD_WINSRV_RX)


4. Virtio driver wraps a FD around that PFN, puts it in a queue

5. Guest proxy calls DRM_IOCTL_VIRTGPU_WINSRV_RX and gets data plus that FD

6. Guest proxy sends that data + FD to the client via SCM_RIGHTS

7. Client gets FD, mmaps it and reads the data from the compositor



Thanks,

Tomeu



If you have a guest proxy anyway using virtio-sock for the protocol
stream and virtio-gpu for buffer sharing (and some day 3d rendering
too) should work fine I think.


If I understand correctly your proposal, virtio-gpu would be used for
creating buffers that could be shared across domains, but something
equivalent to SCM_RIGHTS would still be needed in virtio-vsock?


Yes, the proxy would send a reference to the buffer over virtio-vsock.
I was more thinking about a struct specifying something like
"ressource-id 42 on virtio-gpu-pci device in slot 1:23.0" instead of
using SCM_RIGHTS.


Can you extend on this? I'm having trouble figuring out how this could work
in a way that keeps protocol data together with the resources it refers to.


Don't know much about the wayland protocol.  Assuming you are passing
buffers as file handles, together with some information what kind of
buffer this is (sysv shm, dma-buf, ...).

We have a proxy on both ends.  One running in the guest, one on the host
(be it qemu or some external one).  So these two have to agree on how to
pass buffers from one to the other.  One way would be to have them talk
a

Re: [PATCH v3 1/2] drm/virtio: Add window server support

2018-02-05 Thread Tomeu Vizoso

On 02/05/2018 01:20 PM, Gerd Hoffmann wrote:

   Hi,


Why not use virtio-vsock to run the wayland protocol? I don't like
the idea to duplicate something with very simliar functionality in
virtio-gpu.


The reason for abandoning that approach was the type of objects that
could be shared via virtio-vsock would be extremely limited. Besides
that being potentially confusing to users, it would mean from the
implementation side that either virtio-vsock would gain a dependency on
the drm subsystem, or an appropriate abstraction for shareable buffers
would need to be added for little gain.


Well, no.  The idea is that virtio-vsock and virtio-gpu are used largely
as-is, without knowing about each other.  The guest wayland proxy which
does the buffer management talks to both devices.


Note that the proxy won't know anything about buffers if clients opt-in 
for zero-copy support (they allocate the buffers in a way that allows 
for sharing with the host).



If you have a guest proxy anyway using virtio-sock for the protocol
stream and virtio-gpu for buffer sharing (and some day 3d rendering
too) should work fine I think.


If I understand correctly your proposal, virtio-gpu would be used for
creating buffers that could be shared across domains, but something
equivalent to SCM_RIGHTS would still be needed in virtio-vsock?


Yes, the proxy would send a reference to the buffer over virtio-vsock.
I was more thinking about a struct specifying something like
"ressource-id 42 on virtio-gpu-pci device in slot 1:23.0" instead of
using SCM_RIGHTS.


Can you extend on this? I'm having trouble figuring out how this could 
work in a way that keeps protocol data together with the resources it 
refers to.



If the mechanics of passing presentation data were very complex, I think
this approach would have more merit. But as you can see from the code,
it isn't that bad.


Well, the devil is in the details.  If you have multiple connections you
don't want one being able to stall the others for example.  There are
reasons took quite a while to bring virtio-vsock to the state where it
is today.


Yes, but at the same time there are use cases that virtio-vsock has to 
support but aren't important in this scenario.



What is the plan for the host side? I see basically two options. Either
implement the host wayland proxy directly in qemu. Or
implement it as separate process, which then needs some help from
qemu to get access to the buffers. The later would allow qemu running
independant from the desktop session.


Regarding synchronizing buffers, this will stop becoming needed in
subsequent commits as all shared memory is allocated in the host and
mapped to the guest via KVM_SET_USER_MEMORY_REGION.


--verbose please.  The qemu patches linked from the cover letter not
exactly helpful in understanding how all this is supposed to work.


A client will allocate a buffer with DRM_VIRTGPU_RESOURCE_CREATE, export 
it and pass the FD to the compositor (via the proxy).


During resource creation, QEMU would allocate a shmem buffer and map it 
into the guest with KVM_SET_USER_MEMORY_REGION.


The client would mmap that resource and render to it. Because it's 
backed by host memory, the compositor would be able to read it without 
any further copies.



This is already the case for buffers passed from the compositor to the
clients (see patch 2/2), and I'm working on the equivalent for buffers
from the guest to the host (clients still have to create buffers with
DRM_VIRTGPU_RESOURCE_CREATE but they will be only backend by host memory
so no calls to DRM_VIRTGPU_TRANSFER_TO_HOST are needed).


Same here.  --verbose please.


When a FD comes from the compositor, QEMU mmaps it and maps that virtual 
address to the guest via KVM_SET_USER_MEMORY_REGION.


When the guest proxy reads from the winsrv socket, it will get a FD that 
wraps the buffer referenced above.


When the client reads from the guest proxy, it would get a FD that 
references that same buffer and would mmap it. At that point, the client 
is reading from the same physical pages where the compositor wrote to.


To be clear, I'm not against solving this via some form of restricted FD 
passing in virtio-vsock, but Stefan (added to CC) thought that it would 
be cleaner to do it all within virtio-gpu. This is the thread where it 
was discussed:


https://lkml.kernel.org/r/<2d73a3e1-af70-83a1-0e84-98b5932ea...@collabora.com>

Thanks,

Tomeu


Re: [PATCH v3 1/2] drm/virtio: Add window server support

2018-02-05 Thread Tomeu Vizoso

On 1 February 2018 at 17:36, Gerd Hoffmann  wrote:

Hi,

Sorry for joining the party late. Had a broken finger and was
offline for a bunch of weeks (and a buif backlog afterwards ...).


Hi, no problem, hope it's fine now.


This is to allow clients running within VMs to be able to
communicate with a compositor in the host. Clients will use the
communication protocol that the compositor supports, and virtio-gpu
will assist with making buffers available in both sides, and
copying content as needed.


Why not use virtio-vsock to run the wayland protocol? I don't like
the idea to duplicate something with very simliar functionality in 
virtio-gpu.


The reason for abandoning that approach was the type of objects that
could be shared via virtio-vsock would be extremely limited. Besides
that being potentially confusing to users, it would mean from the
implementation side that either virtio-vsock would gain a dependency on
the drm subsystem, or an appropriate abstraction for shareable buffers
would need to be added for little gain.

Another factor that was taken into account was that the complexity
required for implementing passing protocol data around was very small
when compared with the buffer sharing mechanism.

It is expected that a service in the guest will act as a proxy, 
interacting with virtio-gpu to support unmodified clients.


If you have a guest proxy anyway using virtio-sock for the protocol 
stream and virtio-gpu for buffer sharing (and some day 3d rendering

too) should work fine I think.


If I understand correctly your proposal, virtio-gpu would be used for
creating buffers that could be shared across domains, but something
equivalent to SCM_RIGHTS would still be needed in virtio-vsock?

If so, that's what was planned initially, with the concern being that we
would be adding a bunch of complexity to virtio-vsock that would be only
used in this specific use case. Then we would also need to figure out
how virtio-vsock would be able to work with buffers from virtio-gpu
(either direct dependency or a new abstraction).

If the mechanics of passing presentation data were very complex, I think
this approach would have more merit. But as you can see from the code,
it isn't that bad.


When the client notifies the compositor that it can read from that

buffer,

the proxy should copy the contents from the SHM region to the
virtio-gpu resource and call DRM_VIRTGPU_TRANSFER_TO_HOST.


What is the plan for the host side? I see basically two options. 
Either implement the host wayland proxy directly in qemu. Or

implement it as separate process, which then needs some help from
qemu to get access to the buffers. The later would allow qemu running
independant from the desktop session.


Regarding synchronizing buffers, this will stop becoming needed in
subsequent commits as all shared memory is allocated in the host and
mapped to the guest via KVM_SET_USER_MEMORY_REGION.

This is already the case for buffers passed from the compositor to the
clients (see patch 2/2), and I'm working on the equivalent for buffers
from the guest to the host (clients still have to create buffers with
DRM_VIRTGPU_RESOURCE_CREATE but they will be only backend by host memory
so no calls to DRM_VIRTGPU_TRANSFER_TO_HOST are needed).

But in the case that we still need a proxy for some reason on the host
side, I think it would be better to have it in the same process where
virtio-gpu is implemented. In crosvm's case it would be in a process
separate from the main VMM, as device processes are isolated from each
other with minijail (see
https://chromium.googlesource.com/chromiumos/platform/crosvm/ ).

Regards,

Tomeu


[PATCH v3 2/2] drm/virtio: Handle buffers from the compositor

2018-01-26 Thread Tomeu Vizoso
When retrieving queued messages from the compositor in the host for
clients in the guest, handle buffers that may be passed.

These buffers should have been mapped to the guest's address space, for
example via the KVM_SET_USER_MEMORY_REGION ioctl.

Signed-off-by: Tomeu Vizoso 
---

 drivers/gpu/drm/virtio/virtgpu_ioctl.c | 54 ++
 1 file changed, 54 insertions(+)

diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c 
b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
index d4230b1fa91d..57b1ad51d251 100644
--- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
+++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
@@ -545,14 +545,58 @@ static unsigned int winsrv_poll(struct file *filp,
return mask;
 }
 
+struct virtio_gpu_winsrv_region {
+   uint64_t pfn;
+   size_t size;
+};
+
+static int winsrv_fd_mmap(struct file *filp, struct vm_area_struct *vma)
+{
+   struct virtio_gpu_winsrv_region *region = filp->private_data;
+   unsigned long vm_size = vma->vm_end - vma->vm_start;
+   int ret = 0;
+
+   if (vm_size +
+   (vma->vm_pgoff << PAGE_SHIFT) > PAGE_ALIGN(region->size))
+   return -EINVAL;
+
+   ret = io_remap_pfn_range(vma, vma->vm_start, region->pfn, vm_size,
+vma->vm_page_prot);
+   if (ret)
+   return ret;
+
+   vma->vm_flags |= VM_PFNMAP | VM_IO | VM_DONTEXPAND | VM_DONTDUMP;
+
+   return ret;
+}
+
+static int winsrv_fd_release(struct inode *inodep, struct file *filp)
+{
+   struct virtio_gpu_winsrv_region *region = filp->private_data;
+
+   kfree(region);
+
+   return 0;
+}
+
+static const struct file_operations winsrv_fd_fops = {
+   .mmap = winsrv_fd_mmap,
+   .release = winsrv_fd_release,
+};
+
 static int winsrv_ioctl_rx(struct virtio_gpu_device *vgdev,
   struct virtio_gpu_winsrv_conn *conn,
   struct drm_virtgpu_winsrv *cmd)
 {
struct virtio_gpu_winsrv_rx_qentry *qentry, *tmp;
struct virtio_gpu_winsrv_rx *virtio_cmd;
+   struct virtio_gpu_winsrv_region *region;
int available_len = cmd->len;
int read_count = 0;
+   int i;
+
+   for (i = 0; i < VIRTGPU_WINSRV_MAX_ALLOCS; i++)
+   cmd->fds[i] = -1;
 
list_for_each_entry_safe(qentry, tmp, &conn->cmdq, next) {
virtio_cmd = qentry->cmd;
@@ -567,6 +611,16 @@ static int winsrv_ioctl_rx(struct virtio_gpu_device *vgdev,
return -EFAULT;
}
 
+   for (i = 0; virtio_cmd->pfns[i]; i++) {
+   region = kmalloc(sizeof(*region), GFP_KERNEL);
+   region->pfn = virtio_cmd->pfns[i];
+   region->size = virtio_cmd->lens[i];
+   cmd->fds[i] = anon_inode_getfd("[winsrv_fd]",
+  &winsrv_fd_fops,
+  region,
+  O_CLOEXEC | O_RDWR);
+   }
+
available_len -= virtio_cmd->len;
read_count += virtio_cmd->len;
 
-- 
2.14.3



[PATCH v3 1/2] drm/virtio: Add window server support

2018-01-26 Thread Tomeu Vizoso
This is to allow clients running within VMs to be able to communicate
with a compositor in the host. Clients will use the communication
protocol that the compositor supports, and virtio-gpu will assist with
making buffers available in both sides, and copying content as needed.

It is expected that a service in the guest will act as a proxy,
interacting with virtio-gpu to support unmodified clients. For some
features of the protocol, the hypervisor might have to intervene and
also parse the protocol data to properly bridge resources. The following
IOCTLs have been added to this effect:

*_WINSRV_CONNECT: Opens a connection to the compositor in the host,
returns a FD that represents this connection and on which the following
IOCTLs can be used. Callers are expected to poll this FD for new
messages from the compositor.

*_WINSRV_TX: Asks the hypervisor to forward a message to the compositor

*_WINSRV_RX: Returns all queued messages

Alongside protocol data that is opaque to the kernel, the client can
send file descriptors that reference GEM buffers allocated by
virtio-gpu. The guest proxy is expected to figure out when a client is
passing a FD that refers to shared memory in the guest and allocate a
virtio-gpu buffer of the same size with DRM_VIRTGPU_RESOURCE_CREATE.

When the client notifies the compositor that it can read from that buffer,
the proxy should copy the contents from the SHM region to the virtio-gpu
resource and call DRM_VIRTGPU_TRANSFER_TO_HOST.

This has been tested with Wayland clients that make use of wl_shm to
pass buffers to the compositor, but is expected to work similarly for X
clients that make use of MIT-SHM with FD passing.

v2: * Add padding to two virtio command structs
* Properly cast two __user pointers (kbuild test robot)

v3: * Handle absence of winsrv support in QEMU (Dave Airlie)

Signed-off-by: Tomeu Vizoso 
---

 drivers/gpu/drm/virtio/virtgpu_drv.c   |   1 +
 drivers/gpu/drm/virtio/virtgpu_drv.h   |  39 -
 drivers/gpu/drm/virtio/virtgpu_ioctl.c | 165 +++
 drivers/gpu/drm/virtio/virtgpu_kms.c   |  66 ++--
 drivers/gpu/drm/virtio/virtgpu_vq.c| 285 -
 include/uapi/drm/virtgpu_drm.h |  29 
 include/uapi/linux/virtio_gpu.h|  43 +
 7 files changed, 613 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.c 
b/drivers/gpu/drm/virtio/virtgpu_drv.c
index 49a3d8d5a249..a528ddedd09f 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.c
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.c
@@ -79,6 +79,7 @@ static unsigned int features[] = {
 */
VIRTIO_GPU_F_VIRGL,
 #endif
+   VIRTIO_GPU_F_WINSRV,
 };
 static struct virtio_driver virtio_gpu_driver = {
.feature_table = features,
diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h 
b/drivers/gpu/drm/virtio/virtgpu_drv.h
index da2fb585fea4..268b386e1232 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.h
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
@@ -178,6 +178,8 @@ struct virtio_gpu_device {
 
struct virtio_gpu_queue ctrlq;
struct virtio_gpu_queue cursorq;
+   struct virtio_gpu_queue winsrv_rxq;
+   struct virtio_gpu_queue winsrv_txq;
struct kmem_cache *vbufs;
bool vqs_ready;
 
@@ -205,10 +207,32 @@ struct virtio_gpu_device {
 
 struct virtio_gpu_fpriv {
uint32_t ctx_id;
+
+   struct list_head winsrv_conns; /* list of virtio_gpu_winsrv_conn */
+   spinlock_t winsrv_lock;
+};
+
+struct virtio_gpu_winsrv_rx_qentry {
+   struct virtio_gpu_winsrv_rx *cmd;
+   struct list_head next;
+};
+
+struct virtio_gpu_winsrv_conn {
+   struct virtio_gpu_device *vgdev;
+
+   spinlock_t lock;
+
+   int fd;
+   struct drm_file *drm_file;
+
+   struct list_head cmdq; /* queue of virtio_gpu_winsrv_rx_qentry */
+   wait_queue_head_t cmdwq;
+
+   struct list_head next;
 };
 
 /* virtio_ioctl.c */
-#define DRM_VIRTIO_NUM_IOCTLS 10
+#define DRM_VIRTIO_NUM_IOCTLS 11
 extern struct drm_ioctl_desc virtio_gpu_ioctls[DRM_VIRTIO_NUM_IOCTLS];
 
 /* virtio_kms.c */
@@ -318,9 +342,22 @@ virtio_gpu_cmd_resource_create_3d(struct virtio_gpu_device 
*vgdev,
 void virtio_gpu_ctrl_ack(struct virtqueue *vq);
 void virtio_gpu_cursor_ack(struct virtqueue *vq);
 void virtio_gpu_fence_ack(struct virtqueue *vq);
+void virtio_gpu_winsrv_tx_ack(struct virtqueue *vq);
+void virtio_gpu_winsrv_rx_read(struct virtqueue *vq);
 void virtio_gpu_dequeue_ctrl_func(struct work_struct *work);
 void virtio_gpu_dequeue_cursor_func(struct work_struct *work);
+void virtio_gpu_dequeue_winsrv_rx_func(struct work_struct *work);
+void virtio_gpu_dequeue_winsrv_tx_func(struct work_struct *work);
 void virtio_gpu_dequeue_fence_func(struct work_struct *work);
+void virtio_gpu_fill_winsrv_rx(struct virtio_gpu_device *vgdev);
+void virtio_gpu_queue_winsrv_rx_in(struct virtio_gpu_device *vgdev,
+  struct virtio_gpu_winsrv_rx *cmd);
+int

[PATCH v3 0/2] drm/virtio: Add window server support

2018-01-26 Thread Tomeu Vizoso
Hi,

this work is based on the virtio_wl driver in the ChromeOS kernel by
Zach Reizner, currently at:

https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-4.4/drivers/virtio/virtio_wl.c

There's one feature missing currently, which is letting clients write
directly to the host part of a resource, so the extra copy in
TRANSFER_TO_HOST isn't needed.

Have pushed the QEMU counterpart to this branch, though it isn't as
polished atm:

https://gitlab.collabora.com/tomeu/qemu/commits/winsrv-wip

Thanks,

Tomeu


Tomeu Vizoso (2):
  drm/virtio: Add window server support
  drm/virtio: Handle buffers from the compositor

 drivers/gpu/drm/virtio/virtgpu_drv.c   |   1 +
 drivers/gpu/drm/virtio/virtgpu_drv.h   |  39 -
 drivers/gpu/drm/virtio/virtgpu_ioctl.c | 219 +
 drivers/gpu/drm/virtio/virtgpu_kms.c   |  66 ++--
 drivers/gpu/drm/virtio/virtgpu_vq.c| 285 -
 include/uapi/drm/virtgpu_drm.h |  29 
 include/uapi/linux/virtio_gpu.h|  43 +
 7 files changed, 667 insertions(+), 15 deletions(-)

-- 
2.14.3



Re: [PATCH] drm/virtio: Add window server support

2018-01-12 Thread Tomeu Vizoso

On 01/12/2018 05:11 AM, Dave Airlie wrote:


this work is based on the virtio_wl driver in the ChromeOS kernel by
Zach Reizner, currently at:

https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-4.4/drivers/virtio/virtio_wl.c

There's two features missing in this patch when compared with virtio_wl:

* Allow the guest access directly host memory, without having to resort
to TRANSFER_TO_HOST

* Pass FDs from host to guest (Wayland specifies that the compositor
shares keyboard data with the guest via a shared buffer)

I plan to work on this next, but I would like to get some comments on
the general approach so I can better choose which patch to follow.


Shouldn't qemu expose some kind of capability to enable this so we know to
look for the extra vqs?


Sounds good. I'm unsure though on whether it should be done 
unconditionally if the hypervisor's code supports this, or if only if 
the user pass the -proxy-wayland switch and the hypervisor was able to 
open the socket to the compositor. I'm leaning towards the latter.



What happens if you run this on plain qemu, does it fallback correctly?


Will work on this.


Are there any scenarios where we don't want to expose this API because there
is nothing to back it.


I'm not sure what the overhead of the extra queues is, but I guess the 
ioctls could immediately return -ENODEV if the hypervisor doesn't have 
that capability.


Happy to see that there aren't any major concerns with the general approach.

Thanks,

Tomeu


Re: [PATCH] drm/virtio: Add window server support

2018-01-09 Thread Tomeu Vizoso
On 28 December 2017 at 12:53, Tomeu Vizoso  wrote:
> This is to allow clients running within VMs to be able to communicate
> with a compositor in the host. Clients will use the communication
> protocol that the compositor supports, and virtio-gpu will assist with
> making buffers available in both sides, and copying content as needed.

Here is the qemu side, a bit hackier atm:

https://gitlab.collabora.com/tomeu/qemu/commits/winsrv-wip

Regards,

Tomeu

> It is expected that a service in the guest will act as a proxy,
> interacting with virtio-gpu to support unmodified clients. For some
> features of the protocol, the hypervisor might have to intervene and
> also parse the protocol data to properly bridge resources. The following
> IOCTLs have been added to this effect:
>
> *_WINSRV_CONNECT: Opens a connection to the compositor in the host,
> returns a FD that represents this connection and on which the following
> IOCTLs can be used. Callers are expected to poll this FD for new
> messages from the compositor.
>
> *_WINSRV_TX: Asks the hypervisor to forward a message to the compositor
>
> *_WINSRV_RX: Returns all queued messages
>
> Alongside protocol data that is opaque to the kernel, the client can
> send file descriptors that reference GEM buffers allocated by
> virtio-gpu. The guest proxy is expected to figure out when a client is
> passing a FD that refers to shared memory in the guest and allocate a
> virtio-gpu buffer of the same size with DRM_VIRTGPU_RESOURCE_CREATE.
>
> When the client notifies the compositor that it can read from that buffer,
> the proxy should copy the contents from the SHM region to the virtio-gpu
> resource and call DRM_VIRTGPU_TRANSFER_TO_HOST.
>
> This has been tested with Wayland clients that make use of wl_shm to
> pass buffers to the compositor, but is expected to work similarly for X
> clients that make use of MIT-SHM with FD passing.
>
> v2: * Add padding to two virtio command structs
> * Properly cast two __user pointers (kbuild test robot)
>
> Signed-off-by: Tomeu Vizoso 
> Cc: Zach Reizner 
>
> ---
>
> Hi,
>
> this work is based on the virtio_wl driver in the ChromeOS kernel by
> Zach Reizner, currently at:
>
> https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-4.4/drivers/virtio/virtio_wl.c
>
> There's two features missing in this patch when compared with virtio_wl:
>
> * Allow the guest access directly host memory, without having to resort
> to TRANSFER_TO_HOST
>
> * Pass FDs from host to guest (Wayland specifies that the compositor
> shares keyboard data with the guest via a shared buffer)
>
> I plan to work on this next, but I would like to get some comments on
> the general approach so I can better choose which patch to follow.
>
> Thanks,
>
> Tomeu
> ---
>  drivers/gpu/drm/virtio/virtgpu_drv.h   |  39 -
>  drivers/gpu/drm/virtio/virtgpu_ioctl.c | 168 +++
>  drivers/gpu/drm/virtio/virtgpu_kms.c   |  58 +--
>  drivers/gpu/drm/virtio/virtgpu_vq.c| 285 
> -
>  include/uapi/drm/virtgpu_drm.h |  29 
>  include/uapi/linux/virtio_gpu.h|  41 +
>  6 files changed, 605 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h 
> b/drivers/gpu/drm/virtio/virtgpu_drv.h
> index da2fb585fea4..268b386e1232 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_drv.h
> +++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
> @@ -178,6 +178,8 @@ struct virtio_gpu_device {
>
> struct virtio_gpu_queue ctrlq;
> struct virtio_gpu_queue cursorq;
> +   struct virtio_gpu_queue winsrv_rxq;
> +   struct virtio_gpu_queue winsrv_txq;
> struct kmem_cache *vbufs;
> bool vqs_ready;
>
> @@ -205,10 +207,32 @@ struct virtio_gpu_device {
>
>  struct virtio_gpu_fpriv {
> uint32_t ctx_id;
> +
> +   struct list_head winsrv_conns; /* list of virtio_gpu_winsrv_conn */
> +   spinlock_t winsrv_lock;
> +};
> +
> +struct virtio_gpu_winsrv_rx_qentry {
> +   struct virtio_gpu_winsrv_rx *cmd;
> +   struct list_head next;
> +};
> +
> +struct virtio_gpu_winsrv_conn {
> +   struct virtio_gpu_device *vgdev;
> +
> +   spinlock_t lock;
> +
> +   int fd;
> +   struct drm_file *drm_file;
> +
> +   struct list_head cmdq; /* queue of virtio_gpu_winsrv_rx_qentry */
> +   wait_queue_head_t cmdwq;
> +
> +   struct list_head next;
>  };
>
>  /* virtio_ioctl.c */
> -#define DRM_VIRTIO_NUM_IOCTLS 10
> +#define DRM_VIRTIO_NUM_IOCTLS 11
>  extern struct drm_ioctl_desc virtio_gpu_ioctls[DRM_VIRTIO_NUM_IOCTLS];
>
>  /* virtio_kms.c */
>

Re: [RFC PATCH 01/60] hyper_dmabuf: initial working version of hyper_dmabuf drv

2017-12-29 Thread Tomeu Vizoso
On 26 December 2017 at 19:19, Matt Roper  wrote:
> On Wed, Dec 20, 2017 at 10:59:57AM +0100, Daniel Vetter wrote:
>> On Tue, Dec 19, 2017 at 03:27:31PM -0800, Dongwon Kim wrote:
>> > I forgot to include this brief information about this patch series.
>> >
>> > This patch series contains the implementation of a new device driver,
>> > hyper_dmabuf, which provides a method for DMA-BUF sharing across
>> > different OSes running on the same virtual OS platform powered by
>> > a hypervisor.
>> >
>> > Detailed information about this driver is described in a high-level doc
>> > added by the second patch of the series.
>> >
>> > [RFC PATCH 02/60] hyper_dmabuf: added a doc for hyper_dmabuf sharing
>> >
>> > I am attaching 'Overview' section here as a summary.
>> >
>> > --
>> > Section 1. Overview
>> > --
>> >
>> > Hyper_DMABUF driver is a Linux device driver running on multiple Virtual
>> > achines (VMs), which expands DMA-BUF sharing capability to the VM 
>> > environment
>> > where multiple different OS instances need to share same physical data 
>> > without
>> > data-copy across VMs.
>> >
>> > To share a DMA_BUF across VMs, an instance of the Hyper_DMABUF drv on the
>> > exporting VM (so called, “exporter”) imports a local DMA_BUF from the 
>> > original
>> > producer of the buffer, then re-exports it with an unique ID, 
>> > hyper_dmabuf_id
>> > for the buffer to the importing VM (so called, “importer”).
>> >
>> > Another instance of the Hyper_DMABUF driver on importer registers
>> > a hyper_dmabuf_id together with reference information for the shared 
>> > physical
>> > pages associated with the DMA_BUF to its database when the export happens.
>> >
>> > The actual mapping of the DMA_BUF on the importer’s side is done by
>> > the Hyper_DMABUF driver when user space issues the IOCTL command to access
>> > the shared DMA_BUF. The Hyper_DMABUF driver works as both an importing and
>> > exporting driver as is, that is, no special configuration is required.
>> > Consequently, only a single module per VM is needed to enable cross-VM 
>> > DMA_BUF
>> > exchange.
>>
>> So I know that most dma-buf implementations (especially lots of importers
>> in drivers/gpu) break this, but fundamentally only the original exporter
>> is allowed to know about the underlying pages. There's various scenarios
>> where a dma-buf isn't backed by anything like a struct page.
>>
>> So your first step of noodling the underlying struct page out from the
>> dma-buf is kinda breaking the abstraction, and I think it's not a good
>> idea to have that. Especially not for sharing across VMs.
>>
>> I think a better design would be if hyper-dmabuf would be the dma-buf
>> exporter in both of the VMs, and you'd import it everywhere you want to in
>> some gpu/video/whatever driver in the VMs. That way hyper-dmabuf is always
>> in control of the pages, and a lot of the troubling forwarding you
>> currently need to do disappears.
>
> I think one of the main driving use cases here is for a "local" graphics
> compositor inside the VM to accept client buffers from unmodified
> applications and then pass those buffers along to a "global" compositor
> running in the service domain.  This would allow the global compositor
> to composite applications running in different virtual machines (and
> possibly running under different operating systems).
>
> If we require that hyper-dmabuf always be the exporter, that complicates
> things a little bit since a buffer allocated via regular interfaces (GEM
> ioctls or whatever) wouldn't be directly transferrable to the global
> compositor.  For graphics use cases like this, we could probably hide a
> lot of the details by modifying/replacing the EGL implementation that
> handles the details of buffer allocation.  However if we have
> applications that are themselves just passing along externally-allocated
> buffers (e.g., images from a camera device), we'd probably need to
> modify those applications and/or the drivers they get their content
> from.

There's also non-GPU-rendering clients that pass SHM buffers to the compositor.

For now, a Wayland proxy in the guest is copying the client-provided
buffers to virtio-gpu resources at the appropriate times, which also
need to be copied once more to host memory. Would be great to reduce
the number of copies that that implies.

For more on this effort:

https://patchwork.kernel.org/patch/10134603/

Regards,

Tomeu

>
> Matt
>
>>
>> 2nd thing: This seems very much related to what's happening around gvt and
>> allowing at least the host (in a kvm based VM environment) to be able to
>> access some of the dma-buf (or well, framebuffers in general) that the
>> client is using. Adding some mailing lists for that.
>> -Daniel
>>
>> >
>> > --
>> >
>> > There is a

[PATCH] drm/virtio: Add window server support

2017-12-28 Thread Tomeu Vizoso
This is to allow clients running within VMs to be able to communicate
with a compositor in the host. Clients will use the communication
protocol that the compositor supports, and virtio-gpu will assist with
making buffers available in both sides, and copying content as needed.

It is expected that a service in the guest will act as a proxy,
interacting with virtio-gpu to support unmodified clients. For some
features of the protocol, the hypervisor might have to intervene and
also parse the protocol data to properly bridge resources. The following
IOCTLs have been added to this effect:

*_WINSRV_CONNECT: Opens a connection to the compositor in the host,
returns a FD that represents this connection and on which the following
IOCTLs can be used. Callers are expected to poll this FD for new
messages from the compositor.

*_WINSRV_TX: Asks the hypervisor to forward a message to the compositor

*_WINSRV_RX: Returns all queued messages

Alongside protocol data that is opaque to the kernel, the client can
send file descriptors that reference GEM buffers allocated by
virtio-gpu. The guest proxy is expected to figure out when a client is
passing a FD that refers to shared memory in the guest and allocate a
virtio-gpu buffer of the same size with DRM_VIRTGPU_RESOURCE_CREATE.

When the client notifies the compositor that it can read from that buffer,
the proxy should copy the contents from the SHM region to the virtio-gpu
resource and call DRM_VIRTGPU_TRANSFER_TO_HOST.

This has been tested with Wayland clients that make use of wl_shm to
pass buffers to the compositor, but is expected to work similarly for X
clients that make use of MIT-SHM with FD passing.

v2: * Add padding to two virtio command structs
* Properly cast two __user pointers (kbuild test robot)

Signed-off-by: Tomeu Vizoso 
Cc: Zach Reizner 

---

Hi,

this work is based on the virtio_wl driver in the ChromeOS kernel by
Zach Reizner, currently at:

https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-4.4/drivers/virtio/virtio_wl.c

There's two features missing in this patch when compared with virtio_wl:

* Allow the guest access directly host memory, without having to resort
to TRANSFER_TO_HOST

* Pass FDs from host to guest (Wayland specifies that the compositor
shares keyboard data with the guest via a shared buffer)

I plan to work on this next, but I would like to get some comments on
the general approach so I can better choose which patch to follow.

Thanks,

Tomeu
---
 drivers/gpu/drm/virtio/virtgpu_drv.h   |  39 -
 drivers/gpu/drm/virtio/virtgpu_ioctl.c | 168 +++
 drivers/gpu/drm/virtio/virtgpu_kms.c   |  58 +--
 drivers/gpu/drm/virtio/virtgpu_vq.c| 285 -
 include/uapi/drm/virtgpu_drm.h |  29 
 include/uapi/linux/virtio_gpu.h|  41 +
 6 files changed, 605 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h 
b/drivers/gpu/drm/virtio/virtgpu_drv.h
index da2fb585fea4..268b386e1232 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.h
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
@@ -178,6 +178,8 @@ struct virtio_gpu_device {
 
struct virtio_gpu_queue ctrlq;
struct virtio_gpu_queue cursorq;
+   struct virtio_gpu_queue winsrv_rxq;
+   struct virtio_gpu_queue winsrv_txq;
struct kmem_cache *vbufs;
bool vqs_ready;
 
@@ -205,10 +207,32 @@ struct virtio_gpu_device {
 
 struct virtio_gpu_fpriv {
uint32_t ctx_id;
+
+   struct list_head winsrv_conns; /* list of virtio_gpu_winsrv_conn */
+   spinlock_t winsrv_lock;
+};
+
+struct virtio_gpu_winsrv_rx_qentry {
+   struct virtio_gpu_winsrv_rx *cmd;
+   struct list_head next;
+};
+
+struct virtio_gpu_winsrv_conn {
+   struct virtio_gpu_device *vgdev;
+
+   spinlock_t lock;
+
+   int fd;
+   struct drm_file *drm_file;
+
+   struct list_head cmdq; /* queue of virtio_gpu_winsrv_rx_qentry */
+   wait_queue_head_t cmdwq;
+
+   struct list_head next;
 };
 
 /* virtio_ioctl.c */
-#define DRM_VIRTIO_NUM_IOCTLS 10
+#define DRM_VIRTIO_NUM_IOCTLS 11
 extern struct drm_ioctl_desc virtio_gpu_ioctls[DRM_VIRTIO_NUM_IOCTLS];
 
 /* virtio_kms.c */
@@ -318,9 +342,22 @@ virtio_gpu_cmd_resource_create_3d(struct virtio_gpu_device 
*vgdev,
 void virtio_gpu_ctrl_ack(struct virtqueue *vq);
 void virtio_gpu_cursor_ack(struct virtqueue *vq);
 void virtio_gpu_fence_ack(struct virtqueue *vq);
+void virtio_gpu_winsrv_tx_ack(struct virtqueue *vq);
+void virtio_gpu_winsrv_rx_read(struct virtqueue *vq);
 void virtio_gpu_dequeue_ctrl_func(struct work_struct *work);
 void virtio_gpu_dequeue_cursor_func(struct work_struct *work);
+void virtio_gpu_dequeue_winsrv_rx_func(struct work_struct *work);
+void virtio_gpu_dequeue_winsrv_tx_func(struct work_struct *work);
 void virtio_gpu_dequeue_fence_func(struct work_struct *work);
+void virtio_gpu_fill_winsrv_rx(struct virtio_gpu_device *vgdev);
+void virtio_gpu_queue_winsrv_

[PATCH] drm/virtio: Add window server support

2017-12-14 Thread Tomeu Vizoso
This is to allow clients running within VMs to be able to communicate
with a compositor in the host. Clients will use the communication
protocol that the compositor supports, and virtio-gpu will assist with
making buffers available in both sides, and copying content as needed.

It is expected that a service in the guest will act as a proxy,
interacting with virtio-gpu to support unmodified clients. For some
features of the protocol, the hypervisor might have to intervene and
also parse the protocol data to properly bridge resources. The following
IOCTLs have been added to this effect:

*_WINSRV_CONNECT: Opens a connection to the compositor in the host,
returns a FD that represents this connection and on which the following
IOCTLs can be used. Callers are expected to poll this FD for new
messages from the compositor.

*_WINSRV_TX: Asks the hypervisor to forward a message to the compositor

*_WINSRV_RX: Returns all queued messages

Alongside protocol data that is opaque to the kernel, the client can
send file descriptors that reference GEM buffers allocated by
virtio-gpu. The guest proxy is expected to figure out when a client is
passing a FD that refers to shared memory in the guest and allocate a
virtio-gpu buffer of the same size with DRM_VIRTGPU_RESOURCE_CREATE.

When the client notifies the compositor that it can read from that buffer,
the proxy should copy the contents from the SHM region to the virtio-gpu
resource and call DRM_VIRTGPU_TRANSFER_TO_HOST.

This has been tested with Wayland clients that make use of wl_shm to
pass buffers to the compositor, but is expected to work similarly for X
clients that make use of MIT-SHM with FD passing.

Signed-off-by: Tomeu Vizoso 
Cc: Zach Reizner 

---

Hi,

this work is based on the virtio_wl driver in the ChromeOS kernel by
Zach Reizner, currently at:

https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-4.4/drivers/virtio/virtio_wl.c

There's two features missing in this patch when compared with virtio_wl:

* Allow the guest access directly host memory, without having to resort
to TRANSFER_TO_HOST

* Pass FDs from host to guest (Wayland specifies that the compositor
shares keyboard data with the guest via a shared buffer)

I plan to work on this next, but I would like to get some comments on
the general approach so I can better choose which patch to follow.

Thanks,

Tomeu
---
 drivers/gpu/drm/virtio/virtgpu_drv.h   |  39 -
 drivers/gpu/drm/virtio/virtgpu_ioctl.c | 168 +++
 drivers/gpu/drm/virtio/virtgpu_kms.c   |  58 +--
 drivers/gpu/drm/virtio/virtgpu_vq.c| 283 +
 include/uapi/drm/virtgpu_drm.h |  29 
 include/uapi/linux/virtio_gpu.h|  39 +
 6 files changed, 602 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h 
b/drivers/gpu/drm/virtio/virtgpu_drv.h
index da2fb585fea4..268b386e1232 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.h
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
@@ -178,6 +178,8 @@ struct virtio_gpu_device {
 
struct virtio_gpu_queue ctrlq;
struct virtio_gpu_queue cursorq;
+   struct virtio_gpu_queue winsrv_rxq;
+   struct virtio_gpu_queue winsrv_txq;
struct kmem_cache *vbufs;
bool vqs_ready;
 
@@ -205,10 +207,32 @@ struct virtio_gpu_device {
 
 struct virtio_gpu_fpriv {
uint32_t ctx_id;
+
+   struct list_head winsrv_conns; /* list of virtio_gpu_winsrv_conn */
+   spinlock_t winsrv_lock;
+};
+
+struct virtio_gpu_winsrv_rx_qentry {
+   struct virtio_gpu_winsrv_rx *cmd;
+   struct list_head next;
+};
+
+struct virtio_gpu_winsrv_conn {
+   struct virtio_gpu_device *vgdev;
+
+   spinlock_t lock;
+
+   int fd;
+   struct drm_file *drm_file;
+
+   struct list_head cmdq; /* queue of virtio_gpu_winsrv_rx_qentry */
+   wait_queue_head_t cmdwq;
+
+   struct list_head next;
 };
 
 /* virtio_ioctl.c */
-#define DRM_VIRTIO_NUM_IOCTLS 10
+#define DRM_VIRTIO_NUM_IOCTLS 11
 extern struct drm_ioctl_desc virtio_gpu_ioctls[DRM_VIRTIO_NUM_IOCTLS];
 
 /* virtio_kms.c */
@@ -318,9 +342,22 @@ virtio_gpu_cmd_resource_create_3d(struct virtio_gpu_device 
*vgdev,
 void virtio_gpu_ctrl_ack(struct virtqueue *vq);
 void virtio_gpu_cursor_ack(struct virtqueue *vq);
 void virtio_gpu_fence_ack(struct virtqueue *vq);
+void virtio_gpu_winsrv_tx_ack(struct virtqueue *vq);
+void virtio_gpu_winsrv_rx_read(struct virtqueue *vq);
 void virtio_gpu_dequeue_ctrl_func(struct work_struct *work);
 void virtio_gpu_dequeue_cursor_func(struct work_struct *work);
+void virtio_gpu_dequeue_winsrv_rx_func(struct work_struct *work);
+void virtio_gpu_dequeue_winsrv_tx_func(struct work_struct *work);
 void virtio_gpu_dequeue_fence_func(struct work_struct *work);
+void virtio_gpu_fill_winsrv_rx(struct virtio_gpu_device *vgdev);
+void virtio_gpu_queue_winsrv_rx_in(struct virtio_gpu_device *vgdev,
+  struct virtio_gpu_winsrv_rx *cmd);

[PATCH] drm/virtio: Don't return invalid caps on timeout

2017-11-27 Thread Tomeu Vizoso
If the wait timeouts, the caps are probably invalid and we shouldn't be
passing them to userspace.

Signed-off-by: Tomeu Vizoso 
---
 drivers/gpu/drm/virtio/virtgpu_ioctl.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c 
b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
index b94bd5440e57..902120ad4a6d 100644
--- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
+++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
@@ -517,6 +517,8 @@ static int virtio_gpu_get_caps_ioctl(struct drm_device *dev,
 
ret = wait_event_timeout(vgdev->resp_wq,
 atomic_read(&cache_ent->is_valid), 5 * HZ);
+   if (!ret)
+   return -EBUSY;
 
ptr = cache_ent->caps_cache;
 
-- 
2.14.3



Re: [PATCH] x86/boot: Fix boot failure when SMP MP-table is based at 0

2017-11-16 Thread Tomeu Vizoso
And now with the correct email.

Sorry about that,

Tomeu

On 16 November 2017 at 10:16, Tomeu Vizoso  wrote:
> Adding regress...@leemhuis.info to CC so this regression is tracked.
>
> Regards,
>
> Tomeu
>
> On 8 November 2017 at 09:37, Tomeu Vizoso  wrote:
>> On 6 November 2017 at 23:01, Tom Lendacky  wrote:
>>> On 11/6/2017 3:41 PM, H. Peter Anvin wrote:
>>>>
>>>> On 11/06/17 12:17, Tom Lendacky wrote:
>>>>>
>>>>> When crosvm is used to boot a kernel as a VM, the SMP MP-table is found
>>>>> at physical address 0x0. This causes mpf_base to be set to 0 and a
>>>>> subsequent "if (!mpf_base)" check in default_get_smp_config() results in
>>>>> the MP-table not being parsed.  Further into the boot this results in an
>>>>> oops when attempting a read_apic_id().
>>>>>
>>>>> Add a boolean variable that is set to true when the MP-table is found.
>>>>> Use this variable for testing if the MP-table was found so that even a
>>>>> value of 0 for mpf_base will result in continued parsing of the MP-table.
>>>>>
>>>>> Reported-by: Tomeu Vizoso 
>>>>> Signed-off-by: Tom Lendacky 
>>>>
>>>>
>>>> Ahem... did anyone ever tell you that this is an epicly bad idea on your
>>>> part?  The low megabyte of physical memory has very special meaning on
>>>> x86, and deviating from the standard use of this memory is a *very*
>>>> dangerous thing to do, and imposing on the kernel a "fake null pointer"
>>>> requirement that exists only for the convenience of your particular
>>>> brokenness is not okay.
>>>>
>>>> -hpa
>>>
>>>
>>> That was my initial thought... what was something doing down at the start
>>> of memory.  But when I looked at default_find_smp_config() it specifically
>>> scans the bottom 1K for a an MP-table signature. I was hoping to get some
>>> feedback as to whether this would really be an acceptable thing to do. So
>>> I'm good with this patch being rejected, but the change I made in
>>>
>>> 5997efb96756 ("x86/boot: Use memremap() to map the MPF and MPC data")
>>>
>>> does break something that was working before.
>>
>> Do I understand correctly that the best we can do right now is
>> reverting 5997efb96756?
>>
>> Thanks,
>>
>> Tomeu


Re: [PATCH] x86/boot: Fix boot failure when SMP MP-table is based at 0

2017-11-16 Thread Tomeu Vizoso
Adding regress...@leemhuis.info to CC so this regression is tracked.

Regards,

Tomeu

On 8 November 2017 at 09:37, Tomeu Vizoso  wrote:
> On 6 November 2017 at 23:01, Tom Lendacky  wrote:
>> On 11/6/2017 3:41 PM, H. Peter Anvin wrote:
>>>
>>> On 11/06/17 12:17, Tom Lendacky wrote:
>>>>
>>>> When crosvm is used to boot a kernel as a VM, the SMP MP-table is found
>>>> at physical address 0x0. This causes mpf_base to be set to 0 and a
>>>> subsequent "if (!mpf_base)" check in default_get_smp_config() results in
>>>> the MP-table not being parsed.  Further into the boot this results in an
>>>> oops when attempting a read_apic_id().
>>>>
>>>> Add a boolean variable that is set to true when the MP-table is found.
>>>> Use this variable for testing if the MP-table was found so that even a
>>>> value of 0 for mpf_base will result in continued parsing of the MP-table.
>>>>
>>>> Reported-by: Tomeu Vizoso 
>>>> Signed-off-by: Tom Lendacky 
>>>
>>>
>>> Ahem... did anyone ever tell you that this is an epicly bad idea on your
>>> part?  The low megabyte of physical memory has very special meaning on
>>> x86, and deviating from the standard use of this memory is a *very*
>>> dangerous thing to do, and imposing on the kernel a "fake null pointer"
>>> requirement that exists only for the convenience of your particular
>>> brokenness is not okay.
>>>
>>> -hpa
>>
>>
>> That was my initial thought... what was something doing down at the start
>> of memory.  But when I looked at default_find_smp_config() it specifically
>> scans the bottom 1K for a an MP-table signature. I was hoping to get some
>> feedback as to whether this would really be an acceptable thing to do. So
>> I'm good with this patch being rejected, but the change I made in
>>
>> 5997efb96756 ("x86/boot: Use memremap() to map the MPF and MPC data")
>>
>> does break something that was working before.
>
> Do I understand correctly that the best we can do right now is
> reverting 5997efb96756?
>
> Thanks,
>
> Tomeu


Re: [PATCH] x86/boot: Fix boot failure when SMP MP-table is based at 0

2017-11-08 Thread Tomeu Vizoso
On 6 November 2017 at 23:01, Tom Lendacky  wrote:
> On 11/6/2017 3:41 PM, H. Peter Anvin wrote:
>>
>> On 11/06/17 12:17, Tom Lendacky wrote:
>>>
>>> When crosvm is used to boot a kernel as a VM, the SMP MP-table is found
>>> at physical address 0x0. This causes mpf_base to be set to 0 and a
>>> subsequent "if (!mpf_base)" check in default_get_smp_config() results in
>>> the MP-table not being parsed.  Further into the boot this results in an
>>> oops when attempting a read_apic_id().
>>>
>>> Add a boolean variable that is set to true when the MP-table is found.
>>> Use this variable for testing if the MP-table was found so that even a
>>> value of 0 for mpf_base will result in continued parsing of the MP-table.
>>>
>>> Reported-by: Tomeu Vizoso 
>>> Signed-off-by: Tom Lendacky 
>>
>>
>> Ahem... did anyone ever tell you that this is an epicly bad idea on your
>> part?  The low megabyte of physical memory has very special meaning on
>> x86, and deviating from the standard use of this memory is a *very*
>> dangerous thing to do, and imposing on the kernel a "fake null pointer"
>> requirement that exists only for the convenience of your particular
>> brokenness is not okay.
>>
>> -hpa
>
>
> That was my initial thought... what was something doing down at the start
> of memory.  But when I looked at default_find_smp_config() it specifically
> scans the bottom 1K for a an MP-table signature. I was hoping to get some
> feedback as to whether this would really be an acceptable thing to do. So
> I'm good with this patch being rejected, but the change I made in
>
> 5997efb96756 ("x86/boot: Use memremap() to map the MPF and MPC data")
>
> does break something that was working before.

Do I understand correctly that the best we can do right now is
reverting 5997efb96756?

Thanks,

Tomeu


Re: [PATCH v10 20/38] x86, mpparse: Use memremap to map the mpf and mpc data

2017-11-05 Thread Tomeu Vizoso
On 3 November 2017 at 16:31, Tom Lendacky  wrote:
> On 11/3/2017 10:12 AM, Tomeu Vizoso wrote:
>>
>> On 17 July 2017 at 23:10, Tom Lendacky  wrote:
>>>
>>> The SMP MP-table is built by UEFI and placed in memory in a decrypted
>>> state. These tables are accessed using a mix of early_memremap(),
>>> early_memunmap(), phys_to_virt() and virt_to_phys(). Change all accesses
>>> to use early_memremap()/early_memunmap(). This allows for proper setting
>>> of the encryption mask so that the data can be successfully accessed when
>>> SME is active.
>>>
>>> Reviewed-by: Borislav Petkov 
>>> Signed-off-by: Tom Lendacky 
>>> ---
>>>   arch/x86/kernel/mpparse.c | 98
>>> +--
>>>   1 file changed, 70 insertions(+), 28 deletions(-)
>>
>>
>> Hi there,
>>
>> today I played a bit with crosvm [0] and noticed that 4.14-rc7 doesn't
>> boot. git-bisect pointed to this patch, and reverting it indeed gets
>> things working again.
>>
>> Anybody has an idea of why this could be?
>
>
> If you send me your kernel config I'll see if I can reproduce the issue
> and debug it.

x86_64_defconfig should be enough. I have pasted my dev env
instructions here in case they help:

http://blog.tomeuvizoso.net/2017/11/experiments-with-crosvm_6.html

Thanks,

Tomeu


Re: [PATCH v10 20/38] x86, mpparse: Use memremap to map the mpf and mpc data

2017-11-03 Thread Tomeu Vizoso
On 17 July 2017 at 23:10, Tom Lendacky  wrote:
> The SMP MP-table is built by UEFI and placed in memory in a decrypted
> state. These tables are accessed using a mix of early_memremap(),
> early_memunmap(), phys_to_virt() and virt_to_phys(). Change all accesses
> to use early_memremap()/early_memunmap(). This allows for proper setting
> of the encryption mask so that the data can be successfully accessed when
> SME is active.
>
> Reviewed-by: Borislav Petkov 
> Signed-off-by: Tom Lendacky 
> ---
>  arch/x86/kernel/mpparse.c | 98 
> +--
>  1 file changed, 70 insertions(+), 28 deletions(-)

Hi there,

today I played a bit with crosvm [0] and noticed that 4.14-rc7 doesn't
boot. git-bisect pointed to this patch, and reverting it indeed gets
things working again.

Anybody has an idea of why this could be?

Thanks,

Tomeu

[0] https://chromium.googlesource.com/chromiumos/platform/crosvm

>
> diff --git a/arch/x86/kernel/mpparse.c b/arch/x86/kernel/mpparse.c
> index fd37f39..5cbb317 100644
> --- a/arch/x86/kernel/mpparse.c
> +++ b/arch/x86/kernel/mpparse.c
> @@ -429,7 +429,7 @@ static inline void __init 
> construct_default_ISA_mptable(int mpc_default_type)
> }
>  }
>
> -static struct mpf_intel *mpf_found;
> +static unsigned long mpf_base;
>
>  static unsigned long __init get_mpc_size(unsigned long physptr)
>  {
> @@ -451,6 +451,7 @@ static int __init check_physptr(struct mpf_intel *mpf, 
> unsigned int early)
>
> size = get_mpc_size(mpf->physptr);
> mpc = early_memremap(mpf->physptr, size);
> +
> /*
>  * Read the physical hardware table.  Anything here will
>  * override the defaults.
> @@ -497,12 +498,12 @@ static int __init check_physptr(struct mpf_intel *mpf, 
> unsigned int early)
>   */
>  void __init default_get_smp_config(unsigned int early)
>  {
> -   struct mpf_intel *mpf = mpf_found;
> +   struct mpf_intel *mpf;
>
> if (!smp_found_config)
> return;
>
> -   if (!mpf)
> +   if (!mpf_base)
> return;
>
> if (acpi_lapic && early)
> @@ -515,6 +516,12 @@ void __init default_get_smp_config(unsigned int early)
> if (acpi_lapic && acpi_ioapic)
> return;
>
> +   mpf = early_memremap(mpf_base, sizeof(*mpf));
> +   if (!mpf) {
> +   pr_err("MPTABLE: error mapping MP table\n");
> +   return;
> +   }
> +
> pr_info("Intel MultiProcessor Specification v1.%d\n",
> mpf->specification);
>  #if defined(CONFIG_X86_LOCAL_APIC) && defined(CONFIG_X86_32)
> @@ -529,7 +536,7 @@ void __init default_get_smp_config(unsigned int early)
> /*
>  * Now see if we need to read further.
>  */
> -   if (mpf->feature1 != 0) {
> +   if (mpf->feature1) {
> if (early) {
> /*
>  * local APIC has default address
> @@ -542,8 +549,10 @@ void __init default_get_smp_config(unsigned int early)
> construct_default_ISA_mptable(mpf->feature1);
>
> } else if (mpf->physptr) {
> -   if (check_physptr(mpf, early))
> +   if (check_physptr(mpf, early)) {
> +   early_memunmap(mpf, sizeof(*mpf));
> return;
> +   }
> } else
> BUG();
>
> @@ -552,6 +561,8 @@ void __init default_get_smp_config(unsigned int early)
> /*
>  * Only use the first configuration found.
>  */
> +
> +   early_memunmap(mpf, sizeof(*mpf));
>  }
>
>  static void __init smp_reserve_memory(struct mpf_intel *mpf)
> @@ -561,15 +572,16 @@ static void __init smp_reserve_memory(struct mpf_intel 
> *mpf)
>
>  static int __init smp_scan_config(unsigned long base, unsigned long length)
>  {
> -   unsigned int *bp = phys_to_virt(base);
> +   unsigned int *bp;
> struct mpf_intel *mpf;
> -   unsigned long mem;
> +   int ret = 0;
>
> apic_printk(APIC_VERBOSE, "Scan for SMP in [mem %#010lx-%#010lx]\n",
> base, base + length - 1);
> BUILD_BUG_ON(sizeof(*mpf) != 16);
>
> while (length > 0) {
> +   bp = early_memremap(base, length);
> mpf = (struct mpf_intel *)bp;
> if ((*bp == SMP_MAGIC_IDENT) &&
> (mpf->length == 1) &&
> @@ -579,24 +591,26 @@ static int __init smp_scan_config(unsigned long base, 
> unsigned long length)
>  #ifdef CONFIG_X86_LOCAL_APIC
> smp_found_config = 1;
>  #endif
> -   mpf_found = mpf;
> +   mpf_base = base;
>
> -   pr_info("found SMP MP-table at [mem 
> %#010llx-%#010llx] mapped at [%p]\n",
> -   (unsigned long long) virt_to_phys(mpf),
> -   (unsigned long long) virt_to_phys(mpf) +
> -   sizeof(*mpf) - 1, mpf)

Re: [PATCH] selinux: Fix SBLABEL_MNT for NFS mounts

2017-03-30 Thread Tomeu Vizoso
On 29 March 2017 at 23:34, J. Bruce Fields  wrote:
> On Wed, Mar 29, 2017 at 05:27:23PM +0200, Tomeu Vizoso wrote:
>> Labelling of files in a NFSv4.2 currently fails with ENOTSUPP because
>> the mount point doesn't have SBLABEL_MNT.
>>
>> Add specific condition for NFS4 filesystems so it gets correctly
>> labeled.
>
> Huh.  Looking at the code, I think this is meant to be handled by the
> SECURITY_FS_USE_NATIVE case--there was a similar failure fixed some time
> ago by 9fc2b4b436cf.  What kernel are you seeing this on?  Is it a
> recent regression (in which case, what's the latest kernel that worked
> for you)?

I have seen this on 4.11-rc4, but I never tried to get this working before.

I will try to find time to see why SECURITY_FS_USE_NATIVE isn't working here.

Thanks,

Tomeu

> --b.
>
>>
>> Signed-off-by: Tomeu Vizoso 
>> Cc: J. Bruce Fields 
>>
>> ---
>>
>> Hi,
>>
>> cannot remotely say that I currently understand how selinux is expected
>> to work within NFS mounts, but this change allowed me to fully boot AOSP
>> with its rootfs and ramdisk on a single NFS share.
>>
>> Thanks,
>>
>> Tomeu
>> ---
>>  security/selinux/hooks.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>> index 0c2ac318aa7f..71cd1d8c67c2 100644
>> --- a/security/selinux/hooks.c
>> +++ b/security/selinux/hooks.c
>> @@ -485,6 +485,7 @@ static int selinux_is_sblabel_mnt(struct super_block *sb)
>>   !strcmp(sb->s_type->name, "debugfs") ||
>>   !strcmp(sb->s_type->name, "tracefs") ||
>>   !strcmp(sb->s_type->name, "rootfs") ||
>> + !strcmp(sb->s_type->name, "nfs4") ||
>>   (selinux_policycap_cgroupseclabel &&
>>(!strcmp(sb->s_type->name, "cgroup") ||
>> !strcmp(sb->s_type->name, "cgroup2")));
>> --
>> 2.9.3
>>


[PATCH] selinux: Fix SBLABEL_MNT for NFS mounts

2017-03-29 Thread Tomeu Vizoso
Labelling of files in a NFSv4.2 currently fails with ENOTSUPP because
the mount point doesn't have SBLABEL_MNT.

Add specific condition for NFS4 filesystems so it gets correctly
labeled.

Signed-off-by: Tomeu Vizoso 
Cc: J. Bruce Fields 

---

Hi,

cannot remotely say that I currently understand how selinux is expected
to work within NFS mounts, but this change allowed me to fully boot AOSP
with its rootfs and ramdisk on a single NFS share.

Thanks,

Tomeu
---
 security/selinux/hooks.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 0c2ac318aa7f..71cd1d8c67c2 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -485,6 +485,7 @@ static int selinux_is_sblabel_mnt(struct super_block *sb)
!strcmp(sb->s_type->name, "debugfs") ||
!strcmp(sb->s_type->name, "tracefs") ||
!strcmp(sb->s_type->name, "rootfs") ||
+   !strcmp(sb->s_type->name, "nfs4") ||
(selinux_policycap_cgroupseclabel &&
 (!strcmp(sb->s_type->name, "cgroup") ||
  !strcmp(sb->s_type->name, "cgroup2")));
-- 
2.9.3



Re: RCU used on incoming CPU before rcu_cpu_starting() called

2017-03-20 Thread Tomeu Vizoso
On 9 March 2017 at 16:50, Paul E. McKenney  wrote:
>
> On Thu, Mar 09, 2017 at 07:29:26AM -0800, Paul E. McKenney wrote:
> > On Thu, Mar 09, 2017 at 04:12:55PM +0100, Peter Zijlstra wrote:
> > > On Thu, Mar 09, 2017 at 02:08:23PM +0100, Thomas Gleixner wrote:
> > > > On Wed, 8 Mar 2017, Paul E. McKenney wrote:
> > > > > [   30.694013]  lockdep_rcu_suspicious+0xe7/0x120
> > > > > [   30.694013]  get_work_pool+0x82/0x90
> > > > > [   30.694013]  __queue_work+0x70/0x5f0
> > > > > [   30.694013]  queue_work_on+0x33/0x70
> > > > > [   30.694013]  clear_sched_clock_stable+0x33/0x40
> > > > > [   30.694013]  early_init_intel+0xe7/0x2f0
> > > > > [   30.694013]  init_intel+0x11/0x350
> > > > > [   30.694013]  identify_cpu+0x344/0x5a0
> > > > > [   30.694013]  identify_secondary_cpu+0x18/0x80
> > > > > [   30.694013]  smp_store_cpu_info+0x39/0x40
> > > > > [   30.694013]  start_secondary+0x4e/0x100
> > > > > [   30.694013]  start_cpu+0x14/0x14
> > > > >
> > > > > Here is the relevant code from x86's smp_callin():
> > > > >
> > > > > /*
> > > > >  * Save our processor parameters. Note: this information
> > > > >  * is needed for clock calibration.
> > > > >  */
> > > > > smp_store_cpu_info(cpuid);
> > > > >
> > > > > The problem is that smp_store_cpu_info() indirectly invokes
> > > > > schedule_work(), which wants to use RCU.  But RCU isn't informed
> > > > > of the incoming CPU until the call to notify_cpu_starting(), which
> > > > > causes lockdep to complain bitterly about the use of RCU by the
> > > > > premature call to schedule_work().
> > > >
> > > > Right. And that want's to be fixed, not hacked around by silencing RCU.
> > > >
> > > > Peter
> > >
> > > I'm thinking this is hotplug? 30 seconds after boot is far too late for
> > > SMP bringup, or you have a stupid slow machine.
> >
> > And this certainly does qualify as "shortly", thank you!
> >
> > Yes, this only happens on hotplug with lockdep enabled, specifically
> > on rcutorture scenarios TASKS01 and TREE05.
> >
> > > Because it only calls schedule_work() after SMP-init. In which case
> > > there's then two cases, either:
> > >
> > >  - TSC was stable, hotplug wrecked it, TSC is now unstable, and we're
> > >screwed.
> > >
> > >  - TSC was unstable, hotplug triggers and we want to mark it unstable
> > >_again_.
> > >
> > > If this is the second, the below should fix it, if its the first, I've
> > > no idea yet on how to fix that properly :/
> >
> > I have applied this patch and started tests on TREE05 and TASKS01, should
> > get results shortly.
>
> And the below patch passed light rcutorture testing, so looking good!

I'm having trouble finding this patch in linux-next, has it been pushed already?

Thanks,

Tomeu


[PATCH] drm/dp: Add missing description to parameter

2017-03-07 Thread Tomeu Vizoso
Gabriel Krisman reported these warnings when building the documentation:

 ./drivers/gpu/drm/drm_dp_helper.c:1165: warning: No description found
for parameter 'crtc'
./drivers/gpu/drm/drm_dp_helper.c:1166: warning: No description found
for parameter 'crtc'

Signed-off-by: Tomeu Vizoso 
---
 drivers/gpu/drm/drm_dp_helper.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index c40cfe2e63ab..3e5f52110ea1 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -1158,6 +1158,7 @@ EXPORT_SYMBOL(drm_dp_psr_setup_time);
 /**
  * drm_dp_start_crc() - start capture of frame CRCs
  * @aux: DisplayPort AUX channel
+ * @crtc: CRTC displaying the frames whose CRCs are to be captured
  *
  * Returns 0 on success or a negative error code on failure.
  */
-- 
2.9.3



[PATCH v6 0/4] drm/dp: Implement CRC debugfs API

2017-03-03 Thread Tomeu Vizoso
Hi,

this series builds up on the API for exposing captured CRCs through
debugfs.

It adds new DP helpers for starting and stopping CRC capture and gets
the Rockchip driver to use it.

With these patches, tests in IGT such as kms_pipe_crc_basic and
kms_plane do pass on RK3288.

In this v6, the backpointer in drm_dp_aux becomes drm_crtc instead of
drm_connector, following discussion with Sean Paul.

Thanks,

Tomeu


Tomeu Vizoso (4):
  drm/dp: add crtc backpointer to drm_dp_aux
  drm/dp: add helpers for capture of frame CRCs
  drm/bridge: analogix_dp: add helpers for capture of frame CRCs
  drm/rockchip: Implement CRC debugfs API

 drivers/gpu/drm/bridge/analogix/analogix_dp_core.c |  22 
 drivers/gpu/drm/drm_dp_helper.c| 126 +
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c|  41 +++
 include/drm/bridge/analogix_dp.h   |   3 +
 include/drm/drm_dp_helper.h|   9 ++
 5 files changed, 201 insertions(+)

-- 
2.9.3



[PATCH v6 4/4] drm/rockchip: Implement CRC debugfs API

2017-03-03 Thread Tomeu Vizoso
Implement the .set_crc_source() callback and call the DP helpers
accordingly to start and stop CRC capture.

This is only done if this CRTC is currently using the eDP connector.

v3: Remove superfluous check on rockchip_crtc_state->output_type

v6: Remove superfluous variable

Signed-off-by: Tomeu Vizoso 
---

 drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 41 +
 1 file changed, 41 insertions(+)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c 
b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index 94d7b7327ff7..17ab16c4b922 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -19,6 +19,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 #include 
@@ -,6 +1112,45 @@ static void vop_crtc_destroy_state(struct drm_crtc *crtc,
kfree(s);
 }
 
+static struct drm_connector *vop_get_edp_connector(struct vop *vop)
+{
+   struct drm_crtc *crtc = &vop->crtc;
+   struct drm_connector *connector;
+
+   mutex_lock(&crtc->dev->mode_config.mutex);
+   drm_for_each_connector(connector, crtc->dev)
+   if (connector->connector_type == DRM_MODE_CONNECTOR_eDP) {
+   mutex_unlock(&crtc->dev->mode_config.mutex);
+   return connector;
+   }
+   mutex_unlock(&crtc->dev->mode_config.mutex);
+
+   return NULL;
+}
+
+static int vop_crtc_set_crc_source(struct drm_crtc *crtc,
+  const char *source_name, size_t *values_cnt)
+{
+   struct vop *vop = to_vop(crtc);
+   struct drm_connector *connector;
+   int ret;
+
+   connector = vop_get_edp_connector(vop);
+   if (!connector)
+   return -EINVAL;
+
+   *values_cnt = 3;
+
+   if (source_name && strcmp(source_name, "auto") == 0)
+   ret = analogix_dp_start_crc(connector);
+   else if (!source_name)
+   ret = analogix_dp_stop_crc(connector);
+   else
+   ret = -EINVAL;
+
+   return ret;
+}
+
 static const struct drm_crtc_funcs vop_crtc_funcs = {
.set_config = drm_atomic_helper_set_config,
.page_flip = drm_atomic_helper_page_flip,
@@ -1120,6 +1160,7 @@ static const struct drm_crtc_funcs vop_crtc_funcs = {
.atomic_destroy_state = vop_crtc_destroy_state,
.enable_vblank = vop_crtc_enable_vblank,
.disable_vblank = vop_crtc_disable_vblank,
+   .set_crc_source = vop_crtc_set_crc_source,
 };
 
 static void vop_fb_unref_worker(struct drm_flip_work *work, void *val)
-- 
2.9.3



[PATCH v6 2/4] drm/dp: add helpers for capture of frame CRCs

2017-03-03 Thread Tomeu Vizoso
Adds helpers for starting and stopping capture of frame CRCs through the
DPCD. When capture is on, a worker waits for vblanks and retrieves the
frame CRC to put it in the queue on the CRTC that is using the
eDP connector, so it's passed to userspace.

v2: Reuse drm_crtc_wait_one_vblank
Update locking, as drm_crtc_add_crc_entry now takes the lock

v3: Don't call wake_up_interruptible directly, that's now done in
drm_crtc_add_crc_entry.

v4: Style fixes (Sean Paul)
Reworked retry of CRC reads (Sean Paul)
Flush worker after stopping CRC generationa (Sean Paul)

v5: Move back to make the retry explicitly once

v6: Set and use the drm_crtc backpointer (Sean Paul)

Signed-off-by: Tomeu Vizoso 
---

 drivers/gpu/drm/drm_dp_helper.c | 126 
 include/drm/drm_dp_helper.h |   7 +++
 2 files changed, 133 insertions(+)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 68908c1d5ca1..c40cfe2e63ab 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -981,6 +981,78 @@ static const struct i2c_lock_operations 
drm_dp_i2c_lock_ops = {
.unlock_bus = unlock_bus,
 };
 
+static int drm_dp_aux_get_crc(struct drm_dp_aux *aux, u8 *crc)
+{
+   u8 buf, count;
+   int ret;
+
+   ret = drm_dp_dpcd_readb(aux, DP_TEST_SINK, &buf);
+   if (ret < 0)
+   return ret;
+
+   WARN_ON(!(buf & DP_TEST_SINK_START));
+
+   ret = drm_dp_dpcd_readb(aux, DP_TEST_SINK_MISC, &buf);
+   if (ret < 0)
+   return ret;
+
+   count = buf & DP_TEST_COUNT_MASK;
+   if (count == aux->crc_count)
+   return -EAGAIN; /* No CRC yet */
+
+   aux->crc_count = count;
+
+   /*
+* At DP_TEST_CRC_R_CR, there's 6 bytes containing CRC data, 2 bytes
+* per component (RGB or CrYCb).
+*/
+   ret = drm_dp_dpcd_read(aux, DP_TEST_CRC_R_CR, crc, 6);
+   if (ret < 0)
+   return ret;
+
+   return 0;
+}
+
+static void drm_dp_aux_crc_work(struct work_struct *work)
+{
+   struct drm_dp_aux *aux = container_of(work, struct drm_dp_aux,
+ crc_work);
+   struct drm_crtc *crtc;
+   u8 crc_bytes[6];
+   uint32_t crcs[3];
+   int ret;
+
+   if (WARN_ON(!aux->crtc))
+   return;
+
+   crtc = aux->crtc;
+   while (crtc->crc.opened) {
+   drm_crtc_wait_one_vblank(crtc);
+   if (!crtc->crc.opened)
+   break;
+
+   ret = drm_dp_aux_get_crc(aux, crc_bytes);
+   if (ret == -EAGAIN) {
+   usleep_range(1000, 2000);
+   ret = drm_dp_aux_get_crc(aux, crc_bytes);
+   }
+
+   if (ret == -EAGAIN) {
+   DRM_DEBUG_KMS("Get CRC failed after retrying: %d\n",
+ ret);
+   continue;
+   } else if (ret) {
+   DRM_DEBUG_KMS("Failed to get a CRC: %d\n", ret);
+   continue;
+   }
+
+   crcs[0] = crc_bytes[0] | crc_bytes[1] << 8;
+   crcs[1] = crc_bytes[2] | crc_bytes[3] << 8;
+   crcs[2] = crc_bytes[4] | crc_bytes[5] << 8;
+   drm_crtc_add_crc_entry(crtc, false, 0, crcs);
+   }
+}
+
 /**
  * drm_dp_aux_init() - minimally initialise an aux channel
  * @aux: DisplayPort AUX channel
@@ -993,6 +1065,7 @@ static const struct i2c_lock_operations 
drm_dp_i2c_lock_ops = {
 void drm_dp_aux_init(struct drm_dp_aux *aux)
 {
mutex_init(&aux->hw_mutex);
+   INIT_WORK(&aux->crc_work, drm_dp_aux_crc_work);
 
aux->ddc.algo = &drm_dp_i2c_algo;
aux->ddc.algo_data = aux;
@@ -1081,3 +1154,56 @@ int drm_dp_psr_setup_time(const u8 
psr_cap[EDP_PSR_RECEIVER_CAP_SIZE])
 EXPORT_SYMBOL(drm_dp_psr_setup_time);
 
 #undef PSR_SETUP_TIME
+
+/**
+ * drm_dp_start_crc() - start capture of frame CRCs
+ * @aux: DisplayPort AUX channel
+ *
+ * Returns 0 on success or a negative error code on failure.
+ */
+int drm_dp_start_crc(struct drm_dp_aux *aux, struct drm_crtc *crtc)
+{
+   u8 buf;
+   int ret;
+
+   ret = drm_dp_dpcd_readb(aux, DP_TEST_SINK, &buf);
+   if (ret < 0)
+   return ret;
+
+   ret = drm_dp_dpcd_writeb(aux, DP_TEST_SINK, buf | DP_TEST_SINK_START);
+   if (ret < 0)
+   return ret;
+
+   aux->crc_count = 0;
+   aux->crtc = crtc;
+   schedule_work(&aux->crc_work);
+
+   return 0;
+}
+EXPORT_SYMBOL(drm_dp_start_crc);
+
+/**
+ * drm_dp_stop_crc() - stop capture of frame CRCs
+ * @aux: DisplayPort AUX channel
+ *
+ * Returns 0 on success or a negative error code on failure.
+ */
+int drm_dp_stop_crc(struct drm_dp_aux *aux)
+{
+  

[PATCH v6 1/4] drm/dp: add crtc backpointer to drm_dp_aux

2017-03-03 Thread Tomeu Vizoso
This backpointer allows DP helpers to access the crtc it's currently
being used for.

v6: Have the backpointer be to drm_crtc (Sean Paul)

Signed-off-by: Tomeu Vizoso 
---

 include/drm/drm_dp_helper.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index ba89295c8651..a710e39b5f83 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -789,6 +789,7 @@ struct drm_dp_aux_msg {
  * @name: user-visible name of this AUX channel and the I2C-over-AUX adapter
  * @ddc: I2C adapter that can be used for I2C-over-AUX communication
  * @dev: pointer to struct device that is the parent for this AUX channel
+ * @crtc: backpointer to the crtc that is currently using this AUX channel
  * @hw_mutex: internal mutex used for locking transfers
  * @transfer: transfers a message representing a single AUX transaction
  *
@@ -825,6 +826,7 @@ struct drm_dp_aux {
const char *name;
struct i2c_adapter ddc;
struct device *dev;
+   struct drm_crtc *crtc;
struct mutex hw_mutex;
ssize_t (*transfer)(struct drm_dp_aux *aux,
struct drm_dp_aux_msg *msg);
-- 
2.9.3



[PATCH v6 3/4] drm/bridge: analogix_dp: add helpers for capture of frame CRCs

2017-03-03 Thread Tomeu Vizoso
Add two simple functions that just take the drm_dp_aux from our struct
and calls the corresponding DP helpers with it.

v6: Pass to the DP helper the drm_crtc of the current connector (Sean Paul)

Signed-off-by: Tomeu Vizoso 
---

 drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 22 ++
 include/drm/bridge/analogix_dp.h   |  3 +++
 2 files changed, 25 insertions(+)

diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c 
b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
index e7cd1056ff2d..c26997afd3cf 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
@@ -1488,6 +1488,28 @@ int analogix_dp_resume(struct device *dev)
 EXPORT_SYMBOL_GPL(analogix_dp_resume);
 #endif
 
+int analogix_dp_start_crc(struct drm_connector *connector)
+{
+   struct analogix_dp_device *dp = to_dp(connector);
+
+   if (!connector->state->crtc) {
+   DRM_ERROR("Connector %s doesn't currently have a CRTC.\n",
+ connector->name);
+   return -EINVAL;
+   }
+
+   return drm_dp_start_crc(&dp->aux, connector->state->crtc);
+}
+EXPORT_SYMBOL_GPL(analogix_dp_start_crc);
+
+int analogix_dp_stop_crc(struct drm_connector *connector)
+{
+   struct analogix_dp_device *dp = to_dp(connector);
+
+   return drm_dp_stop_crc(&dp->aux);
+}
+EXPORT_SYMBOL_GPL(analogix_dp_stop_crc);
+
 MODULE_AUTHOR("Jingoo Han ");
 MODULE_DESCRIPTION("Analogix DP Core Driver");
 MODULE_LICENSE("GPL v2");
diff --git a/include/drm/bridge/analogix_dp.h b/include/drm/bridge/analogix_dp.h
index f6f0c062205c..c99d6eaef1ac 100644
--- a/include/drm/bridge/analogix_dp.h
+++ b/include/drm/bridge/analogix_dp.h
@@ -49,4 +49,7 @@ int analogix_dp_bind(struct device *dev, struct drm_device 
*drm_dev,
 struct analogix_dp_plat_data *plat_data);
 void analogix_dp_unbind(struct device *dev, struct device *master, void *data);
 
+int analogix_dp_start_crc(struct drm_connector *connector);
+int analogix_dp_stop_crc(struct drm_connector *connector);
+
 #endif /* _ANALOGIX_DP_H_ */
-- 
2.9.3



[PATCH] drm/edid: Add EDID_QUIRK_FORCE_8BPC quirk for Rotel RSX-1058

2017-02-20 Thread Tomeu Vizoso
Rotel RSX-1058 is a receiver with 4 HDMI inputs and a HDMI output, all
1.1.

When a sink that supports deep color is connected to the output, the
receiver will send EDIDs that advertise this capability, even if it
isn't possible with HDMI versions earlier than 1.3.

Currently the kernel is assuming that deep color is possible and the
sink displays an error.

This quirk will make sure that deep color isn't used with this
particular receiver.

Fixes: 7a0baa623446 ("Revert "drm/i915: Disable 12bpc hdmi for now"")
References: https://bugs.freedesktop.org/show_bug.cgi?id=99869
Signed-off-by: Tomeu Vizoso 
---
 drivers/gpu/drm/drm_edid.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 24e7b282f16c..d994ccf94f88 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -148,6 +148,9 @@ static const struct edid_quirk {
 
/* Panel in Samsung NP700G7A-S01PL notebook reports 6bpc */
{ "SEC", 0xd033, EDID_QUIRK_FORCE_8BPC },
+
+   /* Rotel RSX-1058 forwards sink's EDID but only does HDMI 1.1*/
+   { "ETR", 13896, EDID_QUIRK_FORCE_8BPC },
 };
 
 /*
-- 
2.9.3



Re: [Intel-gfx] [RESEND PATCH v14 2/2] drm/i915: Put "cooked" vlank counters in frame CRC lines

2017-01-16 Thread Tomeu Vizoso
On 10 January 2017 at 17:31, Daniel Vetter  wrote:
> On Tue, Jan 10, 2017 at 05:54:57PM +0200, Ville Syrjälä wrote:
>> On Tue, Jan 10, 2017 at 02:43:05PM +0100, Tomeu Vizoso wrote:
>> > Use drm_accurate_vblank_count so we have the full 32 bit to represent
>> > the frame counter and userspace has a simpler way of knowing when the
>> > counter wraps around.
>> >
>> > Signed-off-by: Tomeu Vizoso 
>> > Reviewed-by: Emil Velikov 
>> > Reviewed-by: Robert Foss 
>> > ---
>> >
>> >  drivers/gpu/drm/i915/i915_irq.c | 6 +++---
>> >  1 file changed, 3 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/i915_irq.c 
>> > b/drivers/gpu/drm/i915/i915_irq.c
>> > index b9beb5955dae..75fb1f66cc0c 100644
>> > --- a/drivers/gpu/drm/i915/i915_irq.c
>> > +++ b/drivers/gpu/drm/i915/i915_irq.c
>> > @@ -1557,7 +1557,6 @@ static void display_pipe_crc_irq_handler(struct 
>> > drm_i915_private *dev_priv,
>> > struct drm_driver *driver = dev_priv->drm.driver;
>> > uint32_t crcs[5];
>> > int head, tail;
>> > -   u32 frame;
>> >
>> > spin_lock(&pipe_crc->lock);
>> > if (pipe_crc->source) {
>> > @@ -1612,8 +1611,9 @@ static void display_pipe_crc_irq_handler(struct 
>> > drm_i915_private *dev_priv,
>> > crcs[2] = crc2;
>> > crcs[3] = crc3;
>> > crcs[4] = crc4;
>> > -   frame = driver->get_vblank_counter(&dev_priv->drm, pipe);
>> > -   drm_crtc_add_crc_entry(&crtc->base, true, frame, crcs);
>> > +   drm_crtc_add_crc_entry(&crtc->base, true,
>> > +  drm_accurate_vblank_count(&crtc->base),
>>
>> My assumption would be that this gets called after the vblank irq
>> handler, so using the _accurate version seems a bit overkill.
>
> Since we're at like v15 of this I figured I'll pull this in, and we can
> polish this a bit more later. Tomeu, can you pls do that follow-up patch
> and get Ville to review+merge it.

At least on the SKL and SNB I have here, the -sequence subtests in
kms_pipe_crc_basic fail if I replace the call to
drm_accurate_vblank_count with drm_crtc_vblank_count.

Any ideas on why this could be?

Thanks,

Tomeu


Re: sysfs deferred_probe attribute

2017-01-12 Thread Tomeu Vizoso
On 01/12/2017 07:26 PM, Ben Hutchings wrote:
> On Thu, 2017-01-12 at 18:41 +0100, Greg Kroah-Hartman wrote:
>> On Thu, Jan 12, 2017 at 11:27:01AM -0600, Rob Herring wrote:
>>> I just noticed that we have a new device attribute 'deferred_probe'
>>> added in 4.10 with this commit:
>>>
>>> commit 6751667a29d6fd64afb9ce30567ad616b68ed789
>>> Author: Ben Hutchings 
>>> Date:   Tue Aug 16 14:34:18 2016 +0100
>>>
>>> driver core: Add deferred_probe attribute to devices in sysfs
>>>
>>> It is sometimes useful to know that a device is on the deferred probe
>>> list rather than, say, not having a driver available.  Expose this
>>> information to user-space.
>>>
>>> Signed-off-by: Ben Hutchings 
>>> Signed-off-by: Greg Kroah-Hartman 
>>>
>>>
>>> It seems like a bad idea to add an ABI for an internal kernel feature.
>>> When/if we replace deferred probe with something better based on
>>> functional dependencies are we going to keep this attr around? Or
>>> remove it and assume no userspace uses it?
> 
> It should be removed then (and replaced with some kind of representation
> of dependencies).
> 
>>> Perhaps it should be hidden
>>> behind CONFIG_DEBUG or just make a debugfs file that lists the
>>> deferred list. Then you wouldn't have to hunt for what got deferred.
>>
>> Ah, debugfs would be nice, I'd much prefer that.  I don't know how Ben
>> is using this, but I think that would make more sense to me.
> 
> I'm not using it any programmatic way, and don't intend to.  debugfs
> would be OK, but attaching it to devices was easy to do and seemed to
> make sense.

Russell King started work on printing those devices in the deferred
queue at late_initcall, not sure why it didn't land.

But note that without proper dependency information, you cannot know for
sure if a device deferred its probe just because a dependency doesn't
have a matching driver.

Regards,

Tomeu





[PATCH v5 0/4] drm/dp: Implement CRC debugfs API

2017-01-12 Thread Tomeu Vizoso
Hi,

this series builds up on the API for exposing captured CRCs through
debugfs.

It adds new DP helpers for starting and stopping CRC capture and gets
the Rockchip driver to use it.

Also had to add a connector backpointer to the drm_dp_aux struct so we could
wait for the right vblank and store the CRCs afterwards, I will be glad
to hear about better alternatives.

With these patches, tests in IGT such as kms_pipe_crc_basic and
kms_plane do pass on RK3288.

In this v5, "drm/dp: add helpers for capture of frame CRCs" has gone
back to the more explicit way of just retrying once.

Also, I have left the connector back pointer in the AUX structure, as on
IRC nor danvet nor me could find a good reason to change it.

Thanks,

Tomeu


Tomeu Vizoso (4):
  drm/bridge: analogix_dp: set connector to drm_dp_aux
  drm/dp: add helpers for capture of frame CRCs
  drm/bridge: analogix_dp: add helpers for capture of frame CRCs
  drm/rockchip: Implement CRC debugfs API

 drivers/gpu/drm/bridge/analogix/analogix_dp_core.c |  34 --
 drivers/gpu/drm/drm_dp_helper.c| 124 +
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c|  42 +++
 include/drm/bridge/analogix_dp.h   |   3 +
 include/drm/drm_dp_helper.h|   7 ++
 5 files changed, 202 insertions(+), 8 deletions(-)

-- 
2.9.3



[PATCH v5 4/4] drm/rockchip: Implement CRC debugfs API

2017-01-12 Thread Tomeu Vizoso
Implement the .set_crc_source() callback and call the DP helpers
accordingly to start and stop CRC capture.

This is only done if this CRTC is currently using the eDP connector.

v3: Remove superfluous check on rockchip_crtc_state->output_type

Signed-off-by: Tomeu Vizoso 
---

 drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 42 +
 1 file changed, 42 insertions(+)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c 
b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index fb5f001f51c3..6e5eb1aa182a 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -19,6 +19,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 #include 
@@ -1105,6 +1106,46 @@ static void vop_crtc_destroy_state(struct drm_crtc *crtc,
kfree(s);
 }
 
+static struct drm_connector *vop_get_edp_connector(struct vop *vop)
+{
+   struct drm_crtc *crtc = &vop->crtc;
+   struct drm_connector *connector;
+
+   mutex_lock(&crtc->dev->mode_config.mutex);
+   drm_for_each_connector(connector, crtc->dev)
+   if (connector->connector_type == DRM_MODE_CONNECTOR_eDP) {
+   mutex_unlock(&crtc->dev->mode_config.mutex);
+   return connector;
+   }
+   mutex_unlock(&crtc->dev->mode_config.mutex);
+
+   return NULL;
+}
+
+static int vop_crtc_set_crc_source(struct drm_crtc *crtc,
+  const char *source_name, size_t *values_cnt)
+{
+   struct vop *vop = to_vop(crtc);
+   struct rockchip_crtc_state *s = to_rockchip_crtc_state(crtc->state);
+   struct drm_connector *connector;
+   int ret;
+
+   connector = vop_get_edp_connector(vop);
+   if (!connector)
+   return -EINVAL;
+
+   *values_cnt = 3;
+
+   if (source_name && strcmp(source_name, "auto") == 0)
+   ret = analogix_dp_start_crc(connector);
+   else if (!source_name)
+   ret = analogix_dp_stop_crc(connector);
+   else
+   ret = -EINVAL;
+
+   return ret;
+}
+
 static const struct drm_crtc_funcs vop_crtc_funcs = {
.set_config = drm_atomic_helper_set_config,
.page_flip = drm_atomic_helper_page_flip,
@@ -1112,6 +1153,7 @@ static const struct drm_crtc_funcs vop_crtc_funcs = {
.reset = vop_crtc_reset,
.atomic_duplicate_state = vop_crtc_duplicate_state,
.atomic_destroy_state = vop_crtc_destroy_state,
+   .set_crc_source = vop_crtc_set_crc_source,
 };
 
 static void vop_fb_unref_worker(struct drm_flip_work *work, void *val)
-- 
2.9.3



[PATCH v5 3/4] drm/bridge: analogix_dp: add helpers for capture of frame CRCs

2017-01-12 Thread Tomeu Vizoso
Add two simple functions that just take the drm_dp_aux from our struct
and calls the corresponding DP helpers with it.

Signed-off-by: Tomeu Vizoso 
---

 drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 16 
 include/drm/bridge/analogix_dp.h   |  3 +++
 2 files changed, 19 insertions(+)

diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c 
b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
index 44c2b74bf771..f5e0379d9abe 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
@@ -1490,6 +1490,22 @@ int analogix_dp_resume(struct device *dev)
 EXPORT_SYMBOL_GPL(analogix_dp_resume);
 #endif
 
+int analogix_dp_start_crc(struct drm_connector *connector)
+{
+   struct analogix_dp_device *dp = to_dp(connector);
+
+   return drm_dp_start_crc(&dp->aux);
+}
+EXPORT_SYMBOL_GPL(analogix_dp_start_crc);
+
+int analogix_dp_stop_crc(struct drm_connector *connector)
+{
+   struct analogix_dp_device *dp = to_dp(connector);
+
+   return drm_dp_stop_crc(&dp->aux);
+}
+EXPORT_SYMBOL_GPL(analogix_dp_stop_crc);
+
 MODULE_AUTHOR("Jingoo Han ");
 MODULE_DESCRIPTION("Analogix DP Core Driver");
 MODULE_LICENSE("GPL v2");
diff --git a/include/drm/bridge/analogix_dp.h b/include/drm/bridge/analogix_dp.h
index f6f0c062205c..c99d6eaef1ac 100644
--- a/include/drm/bridge/analogix_dp.h
+++ b/include/drm/bridge/analogix_dp.h
@@ -49,4 +49,7 @@ int analogix_dp_bind(struct device *dev, struct drm_device 
*drm_dev,
 struct analogix_dp_plat_data *plat_data);
 void analogix_dp_unbind(struct device *dev, struct device *master, void *data);
 
+int analogix_dp_start_crc(struct drm_connector *connector);
+int analogix_dp_stop_crc(struct drm_connector *connector);
+
 #endif /* _ANALOGIX_DP_H_ */
-- 
2.9.3



[PATCH v5 2/4] drm/dp: add helpers for capture of frame CRCs

2017-01-12 Thread Tomeu Vizoso
Adds helpers for starting and stopping capture of frame CRCs through the
DPCD. When capture is on, a worker waits for vblanks and retrieves the
frame CRC to put it in the queue on the CRTC that is using the
eDP connector, so it's passed to userspace.

v2: Reuse drm_crtc_wait_one_vblank
Update locking, as drm_crtc_add_crc_entry now takes the lock

v3: Don't call wake_up_interruptible directly, that's now done in
drm_crtc_add_crc_entry.

v4: Style fixes (Sean Paul)
Reworked retry of CRC reads (Sean Paul)
Flush worker after stopping CRC generationa (Sean Paul)

v5: Move back to make the retry explicitly once

Signed-off-by: Tomeu Vizoso 
---

 drivers/gpu/drm/drm_dp_helper.c | 124 
 include/drm/drm_dp_helper.h |   7 +++
 2 files changed, 131 insertions(+)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 3e6fe82c6d64..6dd550c7a8bd 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -981,6 +981,78 @@ static const struct i2c_lock_operations 
drm_dp_i2c_lock_ops = {
.unlock_bus = unlock_bus,
 };
 
+static int drm_dp_aux_get_crc(struct drm_dp_aux *aux, u8 *crc)
+{
+   u8 buf, count;
+   int ret;
+
+   ret = drm_dp_dpcd_readb(aux, DP_TEST_SINK, &buf);
+   if (ret < 0)
+   return ret;
+
+   WARN_ON(!(buf & DP_TEST_SINK_START));
+
+   ret = drm_dp_dpcd_readb(aux, DP_TEST_SINK_MISC, &buf);
+   if (ret < 0)
+   return ret;
+
+   count = buf & DP_TEST_COUNT_MASK;
+   if (count == aux->crc_count)
+   return -EAGAIN; /* No CRC yet */
+
+   aux->crc_count = count;
+
+   /*
+* At DP_TEST_CRC_R_CR, there's 6 bytes containing CRC data, 2 bytes
+* per component (RGB or CrYCb).
+*/
+   ret = drm_dp_dpcd_read(aux, DP_TEST_CRC_R_CR, crc, 6);
+   if (ret < 0)
+   return ret;
+
+   return 0;
+}
+
+static void drm_dp_aux_crc_work(struct work_struct *work)
+{
+   struct drm_dp_aux *aux = container_of(work, struct drm_dp_aux,
+ crc_work);
+   struct drm_crtc *crtc;
+   u8 crc_bytes[6];
+   uint32_t crcs[3];
+   int ret;
+
+   if (WARN_ON(!aux->connector))
+   return;
+
+   crtc = aux->connector->state->crtc;
+   while (crtc->crc.opened) {
+   drm_crtc_wait_one_vblank(crtc);
+   if (!crtc->crc.opened)
+   break;
+
+   ret = drm_dp_aux_get_crc(aux, crc_bytes);
+   if (ret == -EAGAIN) {
+   usleep_range(1000, 2000);
+   ret = drm_dp_aux_get_crc(aux, crc_bytes);
+   }
+
+   if (ret == -EAGAIN) {
+   DRM_DEBUG_KMS("Get CRC failed after retrying: %d\n",
+ ret);
+   continue;
+   } else if (ret) {
+   DRM_DEBUG_KMS("Failed to get a CRC: %d\n", ret);
+   continue;
+   }
+
+   crcs[0] = crc_bytes[0] | crc_bytes[1] << 8;
+   crcs[1] = crc_bytes[2] | crc_bytes[3] << 8;
+   crcs[2] = crc_bytes[4] | crc_bytes[5] << 8;
+   drm_crtc_add_crc_entry(crtc, false, 0, crcs);
+   }
+}
+
 /**
  * drm_dp_aux_init() - minimally initialise an aux channel
  * @aux: DisplayPort AUX channel
@@ -993,6 +1065,7 @@ static const struct i2c_lock_operations 
drm_dp_i2c_lock_ops = {
 void drm_dp_aux_init(struct drm_dp_aux *aux)
 {
mutex_init(&aux->hw_mutex);
+   INIT_WORK(&aux->crc_work, drm_dp_aux_crc_work);
 
aux->ddc.algo = &drm_dp_i2c_algo;
aux->ddc.algo_data = aux;
@@ -1081,3 +1154,54 @@ int drm_dp_psr_setup_time(const u8 
psr_cap[EDP_PSR_RECEIVER_CAP_SIZE])
 EXPORT_SYMBOL(drm_dp_psr_setup_time);
 
 #undef PSR_SETUP_TIME
+
+/**
+ * drm_dp_start_crc() - start capture of frame CRCs
+ * @aux: DisplayPort AUX channel
+ *
+ * Returns 0 on success or a negative error code on failure.
+ */
+int drm_dp_start_crc(struct drm_dp_aux *aux)
+{
+   u8 buf;
+   int ret;
+
+   ret = drm_dp_dpcd_readb(aux, DP_TEST_SINK, &buf);
+   if (ret < 0)
+   return ret;
+
+   ret = drm_dp_dpcd_writeb(aux, DP_TEST_SINK, buf | DP_TEST_SINK_START);
+   if (ret < 0)
+   return ret;
+
+   aux->crc_count = 0;
+   schedule_work(&aux->crc_work);
+
+   return 0;
+}
+EXPORT_SYMBOL(drm_dp_start_crc);
+
+/**
+ * drm_dp_stop_crc() - stop capture of frame CRCs
+ * @aux: DisplayPort AUX channel
+ *
+ * Returns 0 on success or a negative error code on failure.
+ */
+int drm_dp_stop_crc(struct drm_dp_aux *aux)
+{
+   u8 buf;
+   int ret;
+
+   ret = drm_dp_dpcd_readb(aux, DP_TEST_SINK, &b

[PATCH v5 1/4] drm/bridge: analogix_dp: set connector to drm_dp_aux

2017-01-12 Thread Tomeu Vizoso
Set the backpointer so that the DP helpers are able to access the
connector that the drm_dp_aux is associated with.

Signed-off-by: Tomeu Vizoso 
---

 drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 18 ++
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c 
b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
index e7cd1056ff2d..44c2b74bf771 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
@@ -1403,26 +1403,28 @@ int analogix_dp_bind(struct device *dev, struct 
drm_device *drm_dev,
dp->drm_dev = drm_dev;
dp->encoder = dp->plat_data->encoder;
 
+   ret = analogix_dp_create_bridge(drm_dev, dp);
+   if (ret) {
+   DRM_ERROR("failed to create bridge (%d)\n", ret);
+   goto err_encoder_cleanup;
+   }
+
dp->aux.name = "DP-AUX";
dp->aux.transfer = analogix_dpaux_transfer;
dp->aux.dev = &pdev->dev;
+   dp->aux.connector = &dp->connector;
 
ret = drm_dp_aux_register(&dp->aux);
if (ret)
-   goto err_disable_pm_runtime;
-
-   ret = analogix_dp_create_bridge(drm_dev, dp);
-   if (ret) {
-   DRM_ERROR("failed to create bridge (%d)\n", ret);
-   drm_encoder_cleanup(dp->encoder);
-   goto err_disable_pm_runtime;
-   }
+   goto err_encoder_cleanup;
 
phy_power_off(dp->phy);
pm_runtime_put(dev);
 
return 0;
 
+err_encoder_cleanup:
+   drm_encoder_cleanup(dp->encoder);
 err_disable_pm_runtime:
 
phy_power_off(dp->phy);
-- 
2.9.3



[RESEND PATCH v14 1/2] drm/i915: Use new CRC debugfs API

2017-01-10 Thread Tomeu Vizoso
The core provides now an ABI to userspace for generation of frame CRCs,
so implement the ->set_crc_source() callback and reuse as much code as
possible with the previous ABI implementation.

When handling the pageflip interrupt, we skip 1 or 2 frames depending on
the HW because they contain wrong values. For the legacy ABI for
generating frame CRCs, this was done in userspace but now that we have a
generic ABI it's better if it's not exposed by the kernel.

v2:
- Leave the legacy implementation in place as the ABI implementation
  in the core is incompatible with it.
v3:
- Use the "cooked" vblank counter so we have a whole 32 bits.
- Make sure we don't mess with the state of the legacy CRC capture
  ABI implementation.
v4:
- Keep use of get_vblank_counter as in the legacy code, will be
  changed in a followup commit.

v5:
- Skip first frame or two as it's known that they contain wrong
  data.
- A few fixes suggested by Emil Velikov.

v6:
- Rework programming of the HW registers to preserve previous
  behavior.

v7:
- Address whitespace issue.
- Added a comment on why in the implementation of the new ABI we
  skip the 1st or 2nd frames.

v9:
- Add stub for intel_crtc_set_crc_source.

v12:
- Rebased.
- Remove stub for intel_crtc_set_crc_source and instead set the
  callback to NULL (Jani Nikula).

v15:
    - Rebased.

Signed-off-by: Tomeu Vizoso 
Reviewed-by: Emil Velikov 
Reviewed-by: Robert Foss 

irq

---

 drivers/gpu/drm/i915/i915_drv.h   |  1 +
 drivers/gpu/drm/i915/i915_irq.c   | 77 ++--
 drivers/gpu/drm/i915/intel_display.c  |  1 +
 drivers/gpu/drm/i915/intel_drv.h  |  6 +++
 drivers/gpu/drm/i915/intel_pipe_crc.c | 94 ++-
 5 files changed, 142 insertions(+), 37 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index ffeebf869e36..7a0eab675031 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1810,6 +1810,7 @@ struct intel_pipe_crc {
enum intel_pipe_crc_source source;
int head, tail;
wait_queue_head_t wq;
+   int skipped;
 };
 
 struct i915_frontbuffer_tracking {
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index a0e70f5b3aad..b9beb5955dae 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1553,41 +1553,68 @@ static void display_pipe_crc_irq_handler(struct 
drm_i915_private *dev_priv,
 {
struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[pipe];
struct intel_pipe_crc_entry *entry;
+   struct intel_crtc *crtc = intel_get_crtc_for_pipe(dev_priv, pipe);
+   struct drm_driver *driver = dev_priv->drm.driver;
+   uint32_t crcs[5];
int head, tail;
+   u32 frame;
 
spin_lock(&pipe_crc->lock);
+   if (pipe_crc->source) {
+   if (!pipe_crc->entries) {
+   spin_unlock(&pipe_crc->lock);
+   DRM_DEBUG_KMS("spurious interrupt\n");
+   return;
+   }
 
-   if (!pipe_crc->entries) {
-   spin_unlock(&pipe_crc->lock);
-   DRM_DEBUG_KMS("spurious interrupt\n");
-   return;
-   }
-
-   head = pipe_crc->head;
-   tail = pipe_crc->tail;
+   head = pipe_crc->head;
+   tail = pipe_crc->tail;
 
-   if (CIRC_SPACE(head, tail, INTEL_PIPE_CRC_ENTRIES_NR) < 1) {
-   spin_unlock(&pipe_crc->lock);
-   DRM_ERROR("CRC buffer overflowing\n");
-   return;
-   }
+   if (CIRC_SPACE(head, tail, INTEL_PIPE_CRC_ENTRIES_NR) < 1) {
+   spin_unlock(&pipe_crc->lock);
+   DRM_ERROR("CRC buffer overflowing\n");
+   return;
+   }
 
-   entry = &pipe_crc->entries[head];
+   entry = &pipe_crc->entries[head];
 
-   entry->frame = dev_priv->drm.driver->get_vblank_counter(&dev_priv->drm,
-pipe);
-   entry->crc[0] = crc0;
-   entry->crc[1] = crc1;
-   entry->crc[2] = crc2;
-   entry->crc[3] = crc3;
-   entry->crc[4] = crc4;
+   entry->frame = driver->get_vblank_counter(&dev_priv->drm, pipe);
+   entry->crc[0] = crc0;
+   entry->crc[1] = crc1;
+   entry->crc[2] = crc2;
+   entry->crc[3] = crc3;
+   entry->crc[4] = crc4;
 
-   head = (head + 1) & (INTEL_PIPE_CRC_ENTRIES_NR - 1);
-   pipe_crc->head = head;
+   head = (head + 1) & (INTEL_PIPE_CRC_ENTRIES_NR - 1);
+   

[RESEND PATCH v14 2/2] drm/i915: Put "cooked" vlank counters in frame CRC lines

2017-01-10 Thread Tomeu Vizoso
Use drm_accurate_vblank_count so we have the full 32 bit to represent
the frame counter and userspace has a simpler way of knowing when the
counter wraps around.

Signed-off-by: Tomeu Vizoso 
Reviewed-by: Emil Velikov 
Reviewed-by: Robert Foss 
---

 drivers/gpu/drm/i915/i915_irq.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index b9beb5955dae..75fb1f66cc0c 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1557,7 +1557,6 @@ static void display_pipe_crc_irq_handler(struct 
drm_i915_private *dev_priv,
struct drm_driver *driver = dev_priv->drm.driver;
uint32_t crcs[5];
int head, tail;
-   u32 frame;
 
spin_lock(&pipe_crc->lock);
if (pipe_crc->source) {
@@ -1612,8 +1611,9 @@ static void display_pipe_crc_irq_handler(struct 
drm_i915_private *dev_priv,
crcs[2] = crc2;
crcs[3] = crc3;
crcs[4] = crc4;
-   frame = driver->get_vblank_counter(&dev_priv->drm, pipe);
-   drm_crtc_add_crc_entry(&crtc->base, true, frame, crcs);
+   drm_crtc_add_crc_entry(&crtc->base, true,
+  drm_accurate_vblank_count(&crtc->base),
+  crcs);
}
 }
 #else
-- 
2.9.3



[RESEND PATCH v14 0/2] New debugfs API for capturing CRC of frames

2017-01-10 Thread Tomeu Vizoso
Hi,

here are the last two patches that remain to be merged in this series,
rebased on today's drm-tip.

Thanks,

Tomeu


Tomeu Vizoso (2):
  drm/i915: Use new CRC debugfs API
  drm/i915: Put "cooked" vlank counters in frame CRC lines

 drivers/gpu/drm/i915/i915_drv.h   |  1 +
 drivers/gpu/drm/i915/i915_irq.c   | 77 ++--
 drivers/gpu/drm/i915/intel_display.c  |  1 +
 drivers/gpu/drm/i915/intel_drv.h  |  6 +++
 drivers/gpu/drm/i915/intel_pipe_crc.c | 94 ++-
 5 files changed, 142 insertions(+), 37 deletions(-)

-- 
2.9.3



[PATCH v4 2/5] drm/bridge: analogix_dp: set connector to drm_dp_aux

2017-01-09 Thread Tomeu Vizoso
Set the backpointer so that the DP helpers are able to access the
connector that the drm_dp_aux is associated with.

Signed-off-by: Tomeu Vizoso 
---

 drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 18 ++
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c 
b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
index 02b97bf64ee4..7d45d3e4600a 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
@@ -1402,23 +1402,25 @@ int analogix_dp_bind(struct device *dev, struct 
drm_device *drm_dev,
dp->drm_dev = drm_dev;
dp->encoder = dp->plat_data->encoder;
 
+   ret = analogix_dp_create_bridge(drm_dev, dp);
+   if (ret) {
+   DRM_ERROR("failed to create bridge (%d)\n", ret);
+   goto err_encoder_cleanup;
+   }
+
dp->aux.name = "DP-AUX";
dp->aux.transfer = analogix_dpaux_transfer;
dp->aux.dev = &pdev->dev;
+   dp->aux.connector = &dp->connector;
 
ret = drm_dp_aux_register(&dp->aux);
if (ret)
-   goto err_disable_pm_runtime;
-
-   ret = analogix_dp_create_bridge(drm_dev, dp);
-   if (ret) {
-   DRM_ERROR("failed to create bridge (%d)\n", ret);
-   drm_encoder_cleanup(dp->encoder);
-   goto err_disable_pm_runtime;
-   }
+   goto err_encoder_cleanup;
 
return 0;
 
+err_encoder_cleanup:
+   drm_encoder_cleanup(dp->encoder);
 err_disable_pm_runtime:
pm_runtime_disable(dev);
 
-- 
2.9.3



[PATCH v4 0/5] drm/dp: Implement CRC debugfs API

2017-01-09 Thread Tomeu Vizoso
Hi,

this series builds up on the API for exposing captured CRCs through
debugfs.

It adds new DP helpers for starting and stopping CRC capture and gets
the Rockchip driver to use it.

Also had to add a connector backpointer to the drm_dp_aux struct so we could
wait for the right vblank and store the CRCs afterwards, I will be glad
to hear about better alternatives.

With these patches, tests in IGT such as kms_pipe_crc_basic and
kms_plane do pass on RK3288.

In this v4, "drm/dp: add helpers for capture of frame CRCs" has been
modified to address some of the comments from Sean Paul.

Thanks,

Tomeu


Tomeu Vizoso (5):
  drm/dp: add connector backpointer to drm_dp_aux
  drm/bridge: analogix_dp: set connector to drm_dp_aux
  drm/dp: add helpers for capture of frame CRCs
  drm/bridge: analogix_dp: add helpers for capture of frame CRCs
  drm/rockchip: Implement CRC debugfs API

 drivers/gpu/drm/bridge/analogix/analogix_dp_core.c |  34 --
 drivers/gpu/drm/drm_dp_helper.c| 129 +
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c|  42 +++
 include/drm/bridge/analogix_dp.h   |   3 +
 include/drm/drm_dp_helper.h|   9 ++
 5 files changed, 209 insertions(+), 8 deletions(-)

-- 
2.9.3



[PATCH v4 3/5] drm/dp: add helpers for capture of frame CRCs

2017-01-09 Thread Tomeu Vizoso
Adds helpers for starting and stopping capture of frame CRCs through the
DPCD. When capture is on, a worker waits for vblanks and retrieves the
frame CRC to put it in the queue on the CRTC that is using the
eDP connector, so it's passed to userspace.

v2: Reuse drm_crtc_wait_one_vblank
Update locking, as drm_crtc_add_crc_entry now takes the lock

v3: Don't call wake_up_interruptible directly, that's now done in
drm_crtc_add_crc_entry.

v4: Style fixes (Sean Paul)
Reworked retry of CRC reads (Sean Paul)
Flush worker after stopping CRC generationa (Sean Paul)

Signed-off-by: Tomeu Vizoso 
---

 drivers/gpu/drm/drm_dp_helper.c | 129 
 include/drm/drm_dp_helper.h |   7 +++
 2 files changed, 136 insertions(+)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 3e6fe82c6d64..f9018faf7e07 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -981,6 +981,83 @@ static const struct i2c_lock_operations 
drm_dp_i2c_lock_ops = {
.unlock_bus = unlock_bus,
 };
 
+static int drm_dp_aux_get_crc(struct drm_dp_aux *aux, u8 *crc)
+{
+   u8 buf, count;
+   int ret;
+
+   ret = drm_dp_dpcd_readb(aux, DP_TEST_SINK, &buf);
+   if (ret < 0)
+   return ret;
+
+   WARN_ON(!(buf & DP_TEST_SINK_START));
+
+   ret = drm_dp_dpcd_readb(aux, DP_TEST_SINK_MISC, &buf);
+   if (ret < 0)
+   return ret;
+
+   count = buf & DP_TEST_COUNT_MASK;
+   if (count == aux->crc_count)
+   return -EAGAIN; /* No CRC yet */
+
+   aux->crc_count = count;
+
+   /*
+* At DP_TEST_CRC_R_CR, there's 6 bytes containing CRC data, 2 bytes
+* per component (RGB or CrYCb).
+*/
+   ret = drm_dp_dpcd_read(aux, DP_TEST_CRC_R_CR, crc, 6);
+   if (ret < 0)
+   return ret;
+
+   return 0;
+}
+
+static void drm_dp_aux_crc_work(struct work_struct *work)
+{
+   struct drm_dp_aux *aux = container_of(work, struct drm_dp_aux,
+ crc_work);
+   struct drm_crtc *crtc;
+   u8 crc_bytes[6];
+   uint32_t crcs[3];
+   bool retry = false;
+   int ret;
+
+   if (WARN_ON(!aux->connector))
+   return;
+
+   crtc = aux->connector->state->crtc;
+   while (crtc->crc.opened) {
+   if (!retry) {
+   drm_crtc_wait_one_vblank(crtc);
+   if (!crtc->crc.opened)
+   break;
+   }
+
+   ret = drm_dp_aux_get_crc(aux, crc_bytes);
+   if (ret == -EAGAIN) {
+   if (retry)
+   DRM_DEBUG_KMS("Failed to get a CRC even after 
retrying: %d\n",
+ ret);
+   retry = !retry;
+   usleep_range(1000, 2000);
+   continue;
+   }
+
+   retry = false;
+   if (!ret) {
+   crcs[0] = crc_bytes[0] | crc_bytes[1] << 8;
+   crcs[1] = crc_bytes[2] | crc_bytes[3] << 8;
+   crcs[2] = crc_bytes[4] | crc_bytes[5] << 8;
+   ret = drm_crtc_add_crc_entry(crtc, false, 0, crcs);
+   if (ret)
+   DRM_DEBUG_KMS("Failed to add crc entry %d\n", 
ret);
+   } else {
+   DRM_DEBUG_KMS("Get CRC failed: %d\n", ret);
+   }
+   }
+}
+
 /**
  * drm_dp_aux_init() - minimally initialise an aux channel
  * @aux: DisplayPort AUX channel
@@ -993,6 +1070,7 @@ static const struct i2c_lock_operations 
drm_dp_i2c_lock_ops = {
 void drm_dp_aux_init(struct drm_dp_aux *aux)
 {
mutex_init(&aux->hw_mutex);
+   INIT_WORK(&aux->crc_work, drm_dp_aux_crc_work);
 
aux->ddc.algo = &drm_dp_i2c_algo;
aux->ddc.algo_data = aux;
@@ -1081,3 +1159,54 @@ int drm_dp_psr_setup_time(const u8 
psr_cap[EDP_PSR_RECEIVER_CAP_SIZE])
 EXPORT_SYMBOL(drm_dp_psr_setup_time);
 
 #undef PSR_SETUP_TIME
+
+/**
+ * drm_dp_start_crc() - start capture of frame CRCs
+ * @aux: DisplayPort AUX channel
+ *
+ * Returns 0 on success or a negative error code on failure.
+ */
+int drm_dp_start_crc(struct drm_dp_aux *aux)
+{
+   u8 buf;
+   int ret;
+
+   ret = drm_dp_dpcd_readb(aux, DP_TEST_SINK, &buf);
+   if (ret < 0)
+   return ret;
+
+   ret = drm_dp_dpcd_writeb(aux, DP_TEST_SINK, buf | DP_TEST_SINK_START);
+   if (ret < 0)
+   return ret;
+
+   aux->crc_count = 0;
+   schedule_work(&aux->crc_work);
+
+   return 0;
+}
+EXPORT_SYMBOL(drm_dp_start_crc);
+
+/**
+ * drm_dp_stop_crc() - stop capture of frame CRCs
+ * @aux: DisplayPort A

[PATCH v4 1/5] drm/dp: add connector backpointer to drm_dp_aux

2017-01-09 Thread Tomeu Vizoso
This backpointer allows DP helpers to access the connector it's being
used for.

Signed-off-by: Tomeu Vizoso 
---

 include/drm/drm_dp_helper.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index 55bbeb0ff594..4fa77b434594 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -721,6 +721,7 @@ struct drm_dp_aux_msg {
  * @name: user-visible name of this AUX channel and the I2C-over-AUX adapter
  * @ddc: I2C adapter that can be used for I2C-over-AUX communication
  * @dev: pointer to struct device that is the parent for this AUX channel
+ * @connector: backpointer to connector that uses this AUX channel
  * @hw_mutex: internal mutex used for locking transfers
  * @transfer: transfers a message representing a single AUX transaction
  *
@@ -757,6 +758,7 @@ struct drm_dp_aux {
const char *name;
struct i2c_adapter ddc;
struct device *dev;
+   struct drm_connector *connector;
struct mutex hw_mutex;
ssize_t (*transfer)(struct drm_dp_aux *aux,
struct drm_dp_aux_msg *msg);
-- 
2.9.3



[PATCH v4 4/5] drm/bridge: analogix_dp: add helpers for capture of frame CRCs

2017-01-09 Thread Tomeu Vizoso
Add two simple functions that just take the drm_dp_aux from our struct
and calls the corresponding DP helpers with it.

Signed-off-by: Tomeu Vizoso 
---

 drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 16 
 include/drm/bridge/analogix_dp.h   |  3 +++
 2 files changed, 19 insertions(+)

diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c 
b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
index 7d45d3e4600a..02f63eb1b887 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
@@ -1483,6 +1483,22 @@ int analogix_dp_resume(struct device *dev)
 EXPORT_SYMBOL_GPL(analogix_dp_resume);
 #endif
 
+int analogix_dp_start_crc(struct drm_connector *connector)
+{
+   struct analogix_dp_device *dp = to_dp(connector);
+
+   return drm_dp_start_crc(&dp->aux);
+}
+EXPORT_SYMBOL_GPL(analogix_dp_start_crc);
+
+int analogix_dp_stop_crc(struct drm_connector *connector)
+{
+   struct analogix_dp_device *dp = to_dp(connector);
+
+   return drm_dp_stop_crc(&dp->aux);
+}
+EXPORT_SYMBOL_GPL(analogix_dp_stop_crc);
+
 MODULE_AUTHOR("Jingoo Han ");
 MODULE_DESCRIPTION("Analogix DP Core Driver");
 MODULE_LICENSE("GPL v2");
diff --git a/include/drm/bridge/analogix_dp.h b/include/drm/bridge/analogix_dp.h
index f6f0c062205c..c99d6eaef1ac 100644
--- a/include/drm/bridge/analogix_dp.h
+++ b/include/drm/bridge/analogix_dp.h
@@ -49,4 +49,7 @@ int analogix_dp_bind(struct device *dev, struct drm_device 
*drm_dev,
 struct analogix_dp_plat_data *plat_data);
 void analogix_dp_unbind(struct device *dev, struct device *master, void *data);
 
+int analogix_dp_start_crc(struct drm_connector *connector);
+int analogix_dp_stop_crc(struct drm_connector *connector);
+
 #endif /* _ANALOGIX_DP_H_ */
-- 
2.9.3



[PATCH v4 5/5] drm/rockchip: Implement CRC debugfs API

2017-01-09 Thread Tomeu Vizoso
Implement the .set_crc_source() callback and call the DP helpers
accordingly to start and stop CRC capture.

This is only done if this CRTC is currently using the eDP connector.

v3: Remove superfluous check on rockchip_crtc_state->output_type

Signed-off-by: Tomeu Vizoso 
---

 drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 42 +
 1 file changed, 42 insertions(+)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c 
b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index fb5f001f51c3..6e5eb1aa182a 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -19,6 +19,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 #include 
@@ -1105,6 +1106,46 @@ static void vop_crtc_destroy_state(struct drm_crtc *crtc,
kfree(s);
 }
 
+static struct drm_connector *vop_get_edp_connector(struct vop *vop)
+{
+   struct drm_crtc *crtc = &vop->crtc;
+   struct drm_connector *connector;
+
+   mutex_lock(&crtc->dev->mode_config.mutex);
+   drm_for_each_connector(connector, crtc->dev)
+   if (connector->connector_type == DRM_MODE_CONNECTOR_eDP) {
+   mutex_unlock(&crtc->dev->mode_config.mutex);
+   return connector;
+   }
+   mutex_unlock(&crtc->dev->mode_config.mutex);
+
+   return NULL;
+}
+
+static int vop_crtc_set_crc_source(struct drm_crtc *crtc,
+  const char *source_name, size_t *values_cnt)
+{
+   struct vop *vop = to_vop(crtc);
+   struct rockchip_crtc_state *s = to_rockchip_crtc_state(crtc->state);
+   struct drm_connector *connector;
+   int ret;
+
+   connector = vop_get_edp_connector(vop);
+   if (!connector)
+   return -EINVAL;
+
+   *values_cnt = 3;
+
+   if (source_name && strcmp(source_name, "auto") == 0)
+   ret = analogix_dp_start_crc(connector);
+   else if (!source_name)
+   ret = analogix_dp_stop_crc(connector);
+   else
+   ret = -EINVAL;
+
+   return ret;
+}
+
 static const struct drm_crtc_funcs vop_crtc_funcs = {
.set_config = drm_atomic_helper_set_config,
.page_flip = drm_atomic_helper_page_flip,
@@ -1112,6 +1153,7 @@ static const struct drm_crtc_funcs vop_crtc_funcs = {
.reset = vop_crtc_reset,
.atomic_duplicate_state = vop_crtc_duplicate_state,
.atomic_destroy_state = vop_crtc_destroy_state,
+   .set_crc_source = vop_crtc_set_crc_source,
 };
 
 static void vop_fb_unref_worker(struct drm_flip_work *work, void *val)
-- 
2.9.3



Re: [PATCH v3 5/5] drm/rockchip: Implement CRC debugfs API

2017-01-09 Thread Tomeu Vizoso
On 6 January 2017 at 16:56, Sean Paul  wrote:
> On Fri, Jan 6, 2017 at 9:30 AM, Tomeu Vizoso  
> wrote:
>> Implement the .set_crc_source() callback and call the DP helpers
>> accordingly to start and stop CRC capture.
>>
>> This is only done if this CRTC is currently using the eDP connector.
>>
>> v3: Remove superfluous check on rockchip_crtc_state->output_type
>>
>> Signed-off-by: Tomeu Vizoso 
>> ---
>>
>>  drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 48 
>> +
>>  1 file changed, 48 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c 
>> b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>> index fb5f001f51c3..5e19bef6d5b4 100644
>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>> @@ -19,6 +19,7 @@
>>  #include 
>>  #include 
>>  #include 
>> +#include 
>>
>>  #include 
>>  #include 
>> @@ -1105,6 +1106,52 @@ static void vop_crtc_destroy_state(struct drm_crtc 
>> *crtc,
>> kfree(s);
>>  }
>>
>> +static struct drm_connector *vop_get_edp_connector(struct vop *vop)
>> +{
>> +   struct drm_crtc *crtc = &vop->crtc;
>> +   struct drm_connector *connector;
>> +
>> +   mutex_lock(&crtc->dev->mode_config.mutex);
>> +   drm_for_each_connector(connector, crtc->dev)
>> +   if (connector->connector_type == DRM_MODE_CONNECTOR_eDP) {
>> +   mutex_unlock(&crtc->dev->mode_config.mutex);
>> +   return connector;
>> +   }
>> +   mutex_unlock(&crtc->dev->mode_config.mutex);
>> +
>> +   return NULL;
>> +}
>> +
>> +static int vop_crtc_set_crc_source(struct drm_crtc *crtc,
>> +  const char *source_name, size_t 
>> *values_cnt)
>> +{
>> +   struct vop *vop = to_vop(crtc);
>> +   struct rockchip_crtc_state *s = to_rockchip_crtc_state(crtc->state);
>> +   struct drm_connector *connector;
>> +   int ret;
>> +
>> +   connector = vop_get_edp_connector(vop);
>> +   if (!connector)
>> +   return -EINVAL;
>> +
>> +   *values_cnt = 3;
>> +
>> +   if (source_name &&
>> +   strcmp(source_name, "auto") == 0) {
>> +
>> +   if (s->output_type != DRM_MODE_CONNECTOR_eDP)
>> +   return -EINVAL;
>> +
>> +   ret = analogix_dp_start_crc(connector);
>
> To pick up my thoughts from 1/5, you could do the following instead:
>
> analogix_dp_start_crc(drm_crtc_index(crtc));

Well, I still need the dpaux to access the dpcd, so that would have to be:

analogix_dp_start_crc(connector, drm_crtc_index(crtc));

Regards,

Tomeu

>> +   } else if (!source_name)
>> +
>> +   ret = analogix_dp_stop_crc(connector);
>> +   else
>> +   ret = -EINVAL;
>> +
>> +   return ret;
>> +}
>> +
>>  static const struct drm_crtc_funcs vop_crtc_funcs = {
>> .set_config = drm_atomic_helper_set_config,
>> .page_flip = drm_atomic_helper_page_flip,
>> @@ -1112,6 +1159,7 @@ static const struct drm_crtc_funcs vop_crtc_funcs = {
>> .reset = vop_crtc_reset,
>> .atomic_duplicate_state = vop_crtc_duplicate_state,
>> .atomic_destroy_state = vop_crtc_destroy_state,
>> +   .set_crc_source = vop_crtc_set_crc_source,
>>  };
>>
>>  static void vop_fb_unref_worker(struct drm_flip_work *work, void *val)
>> --
>> 2.9.3
>>
>>
>> ___
>> linux-arm-kernel mailing list
>> linux-arm-ker...@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
>
>
> --
> Sean Paul, Software Engineer, Google / Chromium OS


Re: [PATCH v3 3/5] drm/dp: add helpers for capture of frame CRCs

2017-01-09 Thread Tomeu Vizoso
On 6 January 2017 at 16:56, Sean Paul  wrote:
> On Fri, Jan 6, 2017 at 9:30 AM, Tomeu Vizoso  
> wrote:
>> Adds helpers for starting and stopping capture of frame CRCs through the
>> DPCD. When capture is on, a worker waits for vblanks and retrieves the
>> frame CRC to put it in the queue on the CRTC that is using the
>> eDP connector, so it's passed to userspace.
>>
>> v2: Reuse drm_crtc_wait_one_vblank
>> Update locking, as drm_crtc_add_crc_entry now takes the lock
>>
>> v3: Don't call wake_up_interruptible directly, that's now done in
>> drm_crtc_add_crc_entry.
>>
>> Signed-off-by: Tomeu Vizoso 
>> ---
>>
>>  drivers/gpu/drm/drm_dp_helper.c | 118 
>> 
>>  include/drm/drm_dp_helper.h |   7 +++
>>  2 files changed, 125 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_dp_helper.c 
>> b/drivers/gpu/drm/drm_dp_helper.c
>> index 3e6fe82c6d64..e531f1317268 100644
>> --- a/drivers/gpu/drm/drm_dp_helper.c
>> +++ b/drivers/gpu/drm/drm_dp_helper.c
>> @@ -981,6 +981,74 @@ static const struct i2c_lock_operations 
>> drm_dp_i2c_lock_ops = {
>> .unlock_bus = unlock_bus,
>>  };
>>
>> +static int drm_dp_aux_get_crc(struct drm_dp_aux *aux, u8 *crc)
>> +{
>> +   u8 buf, count;
>> +   int ret;
>> +
>> +   ret = drm_dp_dpcd_readb(aux, DP_TEST_SINK, &buf);
>> +   if (ret < 0)
>> +   return ret;
>> +
>> +   WARN_ON(!(buf & DP_TEST_SINK_START));
>> +
>> +   ret = drm_dp_dpcd_readb(aux, DP_TEST_SINK_MISC, &buf);
>> +   if (ret < 0)
>> +   return ret;
>> +
>> +   count = buf & DP_TEST_COUNT_MASK;
>> +   if (count == aux->crc_count)
>> +   return -EAGAIN; /* No CRC yet */
>> +
>> +   aux->crc_count = count;
>> +
>> +   /* At DP_TEST_CRC_R_CR, there's 6 bytes containing CRC data, 2 bytes
>> +* per component (RGB or CrYCb).
>
> nit: doesn't conform to CodingStyle
>
>> +*/
>> +   ret = drm_dp_dpcd_read(aux, DP_TEST_CRC_R_CR, crc, 6);
>> +   if (ret < 0)
>> +   return ret;
>> +
>> +   return 0;
>> +}
>> +
>> +static void drm_dp_aux_crc_work(struct work_struct *work)
>> +{
>> +   struct drm_dp_aux *aux = container_of(work, struct drm_dp_aux,
>> + crc_work);
>> +   struct drm_crtc *crtc;
>> +   u8 crc_bytes[6];
>> +   uint32_t crcs[3];
>> +   int ret;
>> +
>> +   if (WARN_ON(!aux->connector))
>> +   return;
>> +
>> +   crtc = aux->connector->state->crtc;
>> +   while (crtc->crc.opened) {
>> +   drm_crtc_wait_one_vblank(crtc);
>> +   if (!crtc->crc.opened)
>> +   break;
>> +
>> +   ret = drm_dp_aux_get_crc(aux, crc_bytes);
>> +   if (ret == -EAGAIN) {
>> +   usleep_range(1000, 2000);
>> +   ret = drm_dp_aux_get_crc(aux, crc_bytes);
>> +   }
>> +
>> +   if (ret) {
>> +   DRM_DEBUG_KMS("Failed to get a CRC even after 
>> retrying: %d\n",
>> + ret);
>> +   continue;
>> +   }
>
> I think it'd be better to restructure this to only call
> drm_dp_aux_get_crc in one place

I'm not completely sure, TBH, as I liked that it was very clear that
we only retry once. I'm about to send v4 with this change and you can
judge by yourself.

> (and differentiate between EAGAIN and
> other failures):

Good point.

> bool retry = false;
> while (...) {
> ...
>
> ret = drm_dp_aux_get_crc(aux, crc_bytes);
> if (ret == -EAGAIN) {
> if (retry)
> DRM_DEBUG_KMS("Get CRC failed after retrying: %d\n",
>   ret);
> retry = !retry;
> usleep_range(1000, 2000);
> continue;
> }
>
> retry = false;
> if (!ret) {
> crcs[0] = crc_bytes[0] | crc_bytes[1] << 8;
> crcs[1] = crc_bytes[2] | crc_bytes[3] << 8;
> crcs[2] = crc_bytes[4] | crc_bytes[5] << 8;
> ret = drm_crtc_add_crc_entry(crtc, false, 0, crcs);
> 

Re: [PATCH v3 1/5] drm/dp: add connector backpointer to drm_dp_aux

2017-01-09 Thread Tomeu Vizoso
On 6 January 2017 at 16:56, Sean Paul  wrote:
> On Fri, Jan 6, 2017 at 9:30 AM, Tomeu Vizoso  
> wrote:
>> This backpointer allows DP helpers to access the connector it's being
>> used for.
>>
>> Signed-off-by: Tomeu Vizoso 
>> ---
>>
>>  include/drm/drm_dp_helper.h | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
>> index 55bbeb0ff594..4fa77b434594 100644
>> --- a/include/drm/drm_dp_helper.h
>> +++ b/include/drm/drm_dp_helper.h
>> @@ -721,6 +721,7 @@ struct drm_dp_aux_msg {
>>   * @name: user-visible name of this AUX channel and the I2C-over-AUX adapter
>>   * @ddc: I2C adapter that can be used for I2C-over-AUX communication
>>   * @dev: pointer to struct device that is the parent for this AUX channel
>> + * @connector: backpointer to connector that uses this AUX channel
>>   * @hw_mutex: internal mutex used for locking transfers
>>   * @transfer: transfers a message representing a single AUX transaction
>>   *
>> @@ -757,6 +758,7 @@ struct drm_dp_aux {
>> const char *name;
>> struct i2c_adapter ddc;
>> struct device *dev;
>> +   struct drm_connector *connector;
>
> Perhaps I'm misreading the series, but it seems like you could instead
> add int crc_pipe along with the other crc fields. Then when you start
> the crc, set the pipe number. If you have the pipe in the crc worker,
> you can wait vblank on that pipe (no pointers needed).
>
> Reasonable?

I think that would work fine, but is it any better? I like that the
connector isn't going to change during the lifetime of the drm_dp_aux,
but crc_pipe could be changing between sampling sessions and any bugs
in keeping that field updated would be quite disconcerting and
cumbersome to debug.

crc_pipe's advantage is that we wouldn't need to update all callers of
drm_dp_aux_register, but I think it's better to have a connector field
that is mistakenly NULL, than a crc_pipe field with the wrong value.

Regards,

Tomeu

> Sean
>
>> struct mutex hw_mutex;
>> ssize_t (*transfer)(struct drm_dp_aux *aux,
>> struct drm_dp_aux_msg *msg);
>> --
>> 2.9.3
>>
>
>
>
> --
> Sean Paul, Software Engineer, Google / Chromium OS


[PATCH v3 5/5] drm/rockchip: Implement CRC debugfs API

2017-01-06 Thread Tomeu Vizoso
Implement the .set_crc_source() callback and call the DP helpers
accordingly to start and stop CRC capture.

This is only done if this CRTC is currently using the eDP connector.

v3: Remove superfluous check on rockchip_crtc_state->output_type

Signed-off-by: Tomeu Vizoso 
---

 drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 48 +
 1 file changed, 48 insertions(+)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c 
b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index fb5f001f51c3..5e19bef6d5b4 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -19,6 +19,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 #include 
@@ -1105,6 +1106,52 @@ static void vop_crtc_destroy_state(struct drm_crtc *crtc,
kfree(s);
 }
 
+static struct drm_connector *vop_get_edp_connector(struct vop *vop)
+{
+   struct drm_crtc *crtc = &vop->crtc;
+   struct drm_connector *connector;
+
+   mutex_lock(&crtc->dev->mode_config.mutex);
+   drm_for_each_connector(connector, crtc->dev)
+   if (connector->connector_type == DRM_MODE_CONNECTOR_eDP) {
+   mutex_unlock(&crtc->dev->mode_config.mutex);
+   return connector;
+   }
+   mutex_unlock(&crtc->dev->mode_config.mutex);
+
+   return NULL;
+}
+
+static int vop_crtc_set_crc_source(struct drm_crtc *crtc,
+  const char *source_name, size_t *values_cnt)
+{
+   struct vop *vop = to_vop(crtc);
+   struct rockchip_crtc_state *s = to_rockchip_crtc_state(crtc->state);
+   struct drm_connector *connector;
+   int ret;
+
+   connector = vop_get_edp_connector(vop);
+   if (!connector)
+   return -EINVAL;
+
+   *values_cnt = 3;
+
+   if (source_name &&
+   strcmp(source_name, "auto") == 0) {
+
+   if (s->output_type != DRM_MODE_CONNECTOR_eDP)
+   return -EINVAL;
+
+   ret = analogix_dp_start_crc(connector);
+   } else if (!source_name)
+
+   ret = analogix_dp_stop_crc(connector);
+   else
+   ret = -EINVAL;
+
+   return ret;
+}
+
 static const struct drm_crtc_funcs vop_crtc_funcs = {
.set_config = drm_atomic_helper_set_config,
.page_flip = drm_atomic_helper_page_flip,
@@ -1112,6 +1159,7 @@ static const struct drm_crtc_funcs vop_crtc_funcs = {
.reset = vop_crtc_reset,
.atomic_duplicate_state = vop_crtc_duplicate_state,
.atomic_destroy_state = vop_crtc_destroy_state,
+   .set_crc_source = vop_crtc_set_crc_source,
 };
 
 static void vop_fb_unref_worker(struct drm_flip_work *work, void *val)
-- 
2.9.3



[PATCH v3 4/5] drm/bridge: analogix_dp: add helpers for capture of frame CRCs

2017-01-06 Thread Tomeu Vizoso
Add two simple functions that just take the drm_dp_aux from our struct
and calls the corresponding DP helpers with it.

Signed-off-by: Tomeu Vizoso 
---

 drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 16 
 include/drm/bridge/analogix_dp.h   |  3 +++
 2 files changed, 19 insertions(+)

diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c 
b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
index 7d45d3e4600a..02f63eb1b887 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
@@ -1483,6 +1483,22 @@ int analogix_dp_resume(struct device *dev)
 EXPORT_SYMBOL_GPL(analogix_dp_resume);
 #endif
 
+int analogix_dp_start_crc(struct drm_connector *connector)
+{
+   struct analogix_dp_device *dp = to_dp(connector);
+
+   return drm_dp_start_crc(&dp->aux);
+}
+EXPORT_SYMBOL_GPL(analogix_dp_start_crc);
+
+int analogix_dp_stop_crc(struct drm_connector *connector)
+{
+   struct analogix_dp_device *dp = to_dp(connector);
+
+   return drm_dp_stop_crc(&dp->aux);
+}
+EXPORT_SYMBOL_GPL(analogix_dp_stop_crc);
+
 MODULE_AUTHOR("Jingoo Han ");
 MODULE_DESCRIPTION("Analogix DP Core Driver");
 MODULE_LICENSE("GPL v2");
diff --git a/include/drm/bridge/analogix_dp.h b/include/drm/bridge/analogix_dp.h
index f6f0c062205c..c99d6eaef1ac 100644
--- a/include/drm/bridge/analogix_dp.h
+++ b/include/drm/bridge/analogix_dp.h
@@ -49,4 +49,7 @@ int analogix_dp_bind(struct device *dev, struct drm_device 
*drm_dev,
 struct analogix_dp_plat_data *plat_data);
 void analogix_dp_unbind(struct device *dev, struct device *master, void *data);
 
+int analogix_dp_start_crc(struct drm_connector *connector);
+int analogix_dp_stop_crc(struct drm_connector *connector);
+
 #endif /* _ANALOGIX_DP_H_ */
-- 
2.9.3



[PATCH v3 0/5] drm/dp: Implement CRC debugfs API

2017-01-06 Thread Tomeu Vizoso
Hi,

this series builds up on the API for exposing captured CRCs through
debugfs.

It adds new DP helpers for starting and stopping CRC capture and gets
the Rockchip driver to use it.

Also had to add a connector backpointer to the drm_dp_aux struct so we could
wait for the right vblank and store the CRCs afterwards, I will be glad
to hear about better alternatives.

With these patches, tests in IGT such as kms_pipe_crc_basic and
kms_plane do pass on RK3288.

Thanks,

Tomeu


Tomeu Vizoso (5):
  drm/dp: add connector backpointer to drm_dp_aux
  drm/bridge: analogix_dp: set connector to drm_dp_aux
  drm/dp: add helpers for capture of frame CRCs
  drm/bridge: analogix_dp: add helpers for capture of frame CRCs
  drm/rockchip: Implement CRC debugfs API

 drivers/gpu/drm/bridge/analogix/analogix_dp_core.c |  34 --
 drivers/gpu/drm/drm_dp_helper.c| 118 +
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c|  48 +
 include/drm/bridge/analogix_dp.h   |   3 +
 include/drm/drm_dp_helper.h|   9 ++
 5 files changed, 204 insertions(+), 8 deletions(-)

-- 
2.9.3



[PATCH v3 1/5] drm/dp: add connector backpointer to drm_dp_aux

2017-01-06 Thread Tomeu Vizoso
This backpointer allows DP helpers to access the connector it's being
used for.

Signed-off-by: Tomeu Vizoso 
---

 include/drm/drm_dp_helper.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index 55bbeb0ff594..4fa77b434594 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -721,6 +721,7 @@ struct drm_dp_aux_msg {
  * @name: user-visible name of this AUX channel and the I2C-over-AUX adapter
  * @ddc: I2C adapter that can be used for I2C-over-AUX communication
  * @dev: pointer to struct device that is the parent for this AUX channel
+ * @connector: backpointer to connector that uses this AUX channel
  * @hw_mutex: internal mutex used for locking transfers
  * @transfer: transfers a message representing a single AUX transaction
  *
@@ -757,6 +758,7 @@ struct drm_dp_aux {
const char *name;
struct i2c_adapter ddc;
struct device *dev;
+   struct drm_connector *connector;
struct mutex hw_mutex;
ssize_t (*transfer)(struct drm_dp_aux *aux,
struct drm_dp_aux_msg *msg);
-- 
2.9.3



[PATCH v3 3/5] drm/dp: add helpers for capture of frame CRCs

2017-01-06 Thread Tomeu Vizoso
Adds helpers for starting and stopping capture of frame CRCs through the
DPCD. When capture is on, a worker waits for vblanks and retrieves the
frame CRC to put it in the queue on the CRTC that is using the
eDP connector, so it's passed to userspace.

v2: Reuse drm_crtc_wait_one_vblank
Update locking, as drm_crtc_add_crc_entry now takes the lock

v3: Don't call wake_up_interruptible directly, that's now done in
drm_crtc_add_crc_entry.

Signed-off-by: Tomeu Vizoso 
---

 drivers/gpu/drm/drm_dp_helper.c | 118 
 include/drm/drm_dp_helper.h |   7 +++
 2 files changed, 125 insertions(+)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 3e6fe82c6d64..e531f1317268 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -981,6 +981,74 @@ static const struct i2c_lock_operations 
drm_dp_i2c_lock_ops = {
.unlock_bus = unlock_bus,
 };
 
+static int drm_dp_aux_get_crc(struct drm_dp_aux *aux, u8 *crc)
+{
+   u8 buf, count;
+   int ret;
+
+   ret = drm_dp_dpcd_readb(aux, DP_TEST_SINK, &buf);
+   if (ret < 0)
+   return ret;
+
+   WARN_ON(!(buf & DP_TEST_SINK_START));
+
+   ret = drm_dp_dpcd_readb(aux, DP_TEST_SINK_MISC, &buf);
+   if (ret < 0)
+   return ret;
+
+   count = buf & DP_TEST_COUNT_MASK;
+   if (count == aux->crc_count)
+   return -EAGAIN; /* No CRC yet */
+
+   aux->crc_count = count;
+
+   /* At DP_TEST_CRC_R_CR, there's 6 bytes containing CRC data, 2 bytes
+* per component (RGB or CrYCb).
+*/
+   ret = drm_dp_dpcd_read(aux, DP_TEST_CRC_R_CR, crc, 6);
+   if (ret < 0)
+   return ret;
+
+   return 0;
+}
+
+static void drm_dp_aux_crc_work(struct work_struct *work)
+{
+   struct drm_dp_aux *aux = container_of(work, struct drm_dp_aux,
+ crc_work);
+   struct drm_crtc *crtc;
+   u8 crc_bytes[6];
+   uint32_t crcs[3];
+   int ret;
+
+   if (WARN_ON(!aux->connector))
+   return;
+
+   crtc = aux->connector->state->crtc;
+   while (crtc->crc.opened) {
+   drm_crtc_wait_one_vblank(crtc);
+   if (!crtc->crc.opened)
+   break;
+
+   ret = drm_dp_aux_get_crc(aux, crc_bytes);
+   if (ret == -EAGAIN) {
+   usleep_range(1000, 2000);
+   ret = drm_dp_aux_get_crc(aux, crc_bytes);
+   }
+
+   if (ret) {
+   DRM_DEBUG_KMS("Failed to get a CRC even after retrying: 
%d\n",
+ ret);
+   continue;
+   }
+
+   crcs[0] = crc_bytes[0] | crc_bytes[1] << 8;
+   crcs[1] = crc_bytes[2] | crc_bytes[3] << 8;
+   crcs[2] = crc_bytes[4] | crc_bytes[5] << 8;
+   ret = drm_crtc_add_crc_entry(crtc, false, 0, crcs);
+   }
+}
+
 /**
  * drm_dp_aux_init() - minimally initialise an aux channel
  * @aux: DisplayPort AUX channel
@@ -993,6 +1061,7 @@ static const struct i2c_lock_operations 
drm_dp_i2c_lock_ops = {
 void drm_dp_aux_init(struct drm_dp_aux *aux)
 {
mutex_init(&aux->hw_mutex);
+   INIT_WORK(&aux->crc_work, drm_dp_aux_crc_work);
 
aux->ddc.algo = &drm_dp_i2c_algo;
aux->ddc.algo_data = aux;
@@ -1081,3 +1150,52 @@ int drm_dp_psr_setup_time(const u8 
psr_cap[EDP_PSR_RECEIVER_CAP_SIZE])
 EXPORT_SYMBOL(drm_dp_psr_setup_time);
 
 #undef PSR_SETUP_TIME
+
+/**
+ * drm_dp_start_crc() - start capture of frame CRCs
+ * @aux: DisplayPort AUX channel
+ *
+ * Returns 0 on success or a negative error code on failure.
+ */
+int drm_dp_start_crc(struct drm_dp_aux *aux)
+{
+   u8 buf;
+   int ret;
+
+   ret = drm_dp_dpcd_readb(aux, DP_TEST_SINK, &buf);
+   if (ret < 0)
+   return ret;
+
+   ret = drm_dp_dpcd_writeb(aux, DP_TEST_SINK, buf | DP_TEST_SINK_START);
+   if (ret < 0)
+   return ret;
+
+   aux->crc_count = 0;
+   schedule_work(&aux->crc_work);
+
+   return 0;
+}
+EXPORT_SYMBOL(drm_dp_start_crc);
+
+/**
+ * drm_dp_stop_crc() - stop capture of frame CRCs
+ * @aux: DisplayPort AUX channel
+ *
+ * Returns 0 on success or a negative error code on failure.
+ */
+int drm_dp_stop_crc(struct drm_dp_aux *aux)
+{
+   u8 buf;
+   int ret;
+
+   ret = drm_dp_dpcd_readb(aux, DP_TEST_SINK, &buf);
+   if (ret < 0)
+   return ret;
+
+   ret = drm_dp_dpcd_writeb(aux, DP_TEST_SINK, buf & ~DP_TEST_SINK_START);
+   if (ret < 0)
+   return ret;
+
+   return 0;
+}
+EXPORT_SYMBOL(drm_dp_stop_crc);
diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index 4fa77b43459

[PATCH v3 2/5] drm/bridge: analogix_dp: set connector to drm_dp_aux

2017-01-06 Thread Tomeu Vizoso
Set the backpointer so that the DP helpers are able to access the
connector that the drm_dp_aux is associated with.

Signed-off-by: Tomeu Vizoso 
---

 drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 18 ++
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c 
b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
index 02b97bf64ee4..7d45d3e4600a 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
@@ -1402,23 +1402,25 @@ int analogix_dp_bind(struct device *dev, struct 
drm_device *drm_dev,
dp->drm_dev = drm_dev;
dp->encoder = dp->plat_data->encoder;
 
+   ret = analogix_dp_create_bridge(drm_dev, dp);
+   if (ret) {
+   DRM_ERROR("failed to create bridge (%d)\n", ret);
+   goto err_encoder_cleanup;
+   }
+
dp->aux.name = "DP-AUX";
dp->aux.transfer = analogix_dpaux_transfer;
dp->aux.dev = &pdev->dev;
+   dp->aux.connector = &dp->connector;
 
ret = drm_dp_aux_register(&dp->aux);
if (ret)
-   goto err_disable_pm_runtime;
-
-   ret = analogix_dp_create_bridge(drm_dev, dp);
-   if (ret) {
-   DRM_ERROR("failed to create bridge (%d)\n", ret);
-   drm_encoder_cleanup(dp->encoder);
-   goto err_disable_pm_runtime;
-   }
+   goto err_encoder_cleanup;
 
return 0;
 
+err_encoder_cleanup:
+   drm_encoder_cleanup(dp->encoder);
 err_disable_pm_runtime:
pm_runtime_disable(dev);
 
-- 
2.9.3



[PATCH v2 4/5] drm/bridge: analogix_dp: add helpers for capture of frame CRCs

2017-01-05 Thread Tomeu Vizoso
Add two simple functions that just take the drm_dp_aux from our struct
and calls the corresponding DP helpers with it.

Signed-off-by: Tomeu Vizoso 
---

 drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 16 
 include/drm/bridge/analogix_dp.h   |  3 +++
 2 files changed, 19 insertions(+)

diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c 
b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
index 7d45d3e4600a..02f63eb1b887 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
@@ -1483,6 +1483,22 @@ int analogix_dp_resume(struct device *dev)
 EXPORT_SYMBOL_GPL(analogix_dp_resume);
 #endif
 
+int analogix_dp_start_crc(struct drm_connector *connector)
+{
+   struct analogix_dp_device *dp = to_dp(connector);
+
+   return drm_dp_start_crc(&dp->aux);
+}
+EXPORT_SYMBOL_GPL(analogix_dp_start_crc);
+
+int analogix_dp_stop_crc(struct drm_connector *connector)
+{
+   struct analogix_dp_device *dp = to_dp(connector);
+
+   return drm_dp_stop_crc(&dp->aux);
+}
+EXPORT_SYMBOL_GPL(analogix_dp_stop_crc);
+
 MODULE_AUTHOR("Jingoo Han ");
 MODULE_DESCRIPTION("Analogix DP Core Driver");
 MODULE_LICENSE("GPL v2");
diff --git a/include/drm/bridge/analogix_dp.h b/include/drm/bridge/analogix_dp.h
index f6f0c062205c..c99d6eaef1ac 100644
--- a/include/drm/bridge/analogix_dp.h
+++ b/include/drm/bridge/analogix_dp.h
@@ -49,4 +49,7 @@ int analogix_dp_bind(struct device *dev, struct drm_device 
*drm_dev,
 struct analogix_dp_plat_data *plat_data);
 void analogix_dp_unbind(struct device *dev, struct device *master, void *data);
 
+int analogix_dp_start_crc(struct drm_connector *connector);
+int analogix_dp_stop_crc(struct drm_connector *connector);
+
 #endif /* _ANALOGIX_DP_H_ */
-- 
2.9.3



[PATCH v2 5/5] drm/rockchip: Implement CRC debugfs API

2017-01-05 Thread Tomeu Vizoso
Implement the .set_crc_source() callback and call the DP helpers
accordingly to start and stop CRC capture.

This is only done if this CRTC is currently using the eDP connector.

Signed-off-by: Tomeu Vizoso 
---

 drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 48 +
 1 file changed, 48 insertions(+)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c 
b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index fb5f001f51c3..5e19bef6d5b4 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -19,6 +19,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 #include 
@@ -1105,6 +1106,52 @@ static void vop_crtc_destroy_state(struct drm_crtc *crtc,
kfree(s);
 }
 
+static struct drm_connector *vop_get_edp_connector(struct vop *vop)
+{
+   struct drm_crtc *crtc = &vop->crtc;
+   struct drm_connector *connector;
+
+   mutex_lock(&crtc->dev->mode_config.mutex);
+   drm_for_each_connector(connector, crtc->dev)
+   if (connector->connector_type == DRM_MODE_CONNECTOR_eDP) {
+   mutex_unlock(&crtc->dev->mode_config.mutex);
+   return connector;
+   }
+   mutex_unlock(&crtc->dev->mode_config.mutex);
+
+   return NULL;
+}
+
+static int vop_crtc_set_crc_source(struct drm_crtc *crtc,
+  const char *source_name, size_t *values_cnt)
+{
+   struct vop *vop = to_vop(crtc);
+   struct rockchip_crtc_state *s = to_rockchip_crtc_state(crtc->state);
+   struct drm_connector *connector;
+   int ret;
+
+   connector = vop_get_edp_connector(vop);
+   if (!connector)
+   return -EINVAL;
+
+   *values_cnt = 3;
+
+   if (source_name &&
+   strcmp(source_name, "auto") == 0) {
+
+   if (s->output_type != DRM_MODE_CONNECTOR_eDP)
+   return -EINVAL;
+
+   ret = analogix_dp_start_crc(connector);
+   } else if (!source_name)
+
+   ret = analogix_dp_stop_crc(connector);
+   else
+   ret = -EINVAL;
+
+   return ret;
+}
+
 static const struct drm_crtc_funcs vop_crtc_funcs = {
.set_config = drm_atomic_helper_set_config,
.page_flip = drm_atomic_helper_page_flip,
@@ -1112,6 +1159,7 @@ static const struct drm_crtc_funcs vop_crtc_funcs = {
.reset = vop_crtc_reset,
.atomic_duplicate_state = vop_crtc_duplicate_state,
.atomic_destroy_state = vop_crtc_destroy_state,
+   .set_crc_source = vop_crtc_set_crc_source,
 };
 
 static void vop_fb_unref_worker(struct drm_flip_work *work, void *val)
-- 
2.9.3



[PATCH v2 3/5] drm/dp: add helpers for capture of frame CRCs

2017-01-05 Thread Tomeu Vizoso
Adds helpers for starting and stopping capture of frame CRCs through the
DPCD. When capture is on, a worker waits for vblanks and retrieves the
frame CRC to put it in the queue on the CRTC that is using the
eDP connector, so it's passed to userspace.

v2: Reuse drm_crtc_wait_one_vblank
Update locking, as drm_crtc_add_crc_entry now takes the lock

Signed-off-by: Tomeu Vizoso 
---

 drivers/gpu/drm/drm_dp_helper.c | 120 
 include/drm/drm_dp_helper.h |   7 +++
 2 files changed, 127 insertions(+)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 3e6fe82c6d64..4afb7ae307de 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -981,6 +981,76 @@ static const struct i2c_lock_operations 
drm_dp_i2c_lock_ops = {
.unlock_bus = unlock_bus,
 };
 
+static int drm_dp_aux_get_crc(struct drm_dp_aux *aux, u8 *crc)
+{
+   u8 buf, count;
+   int ret;
+
+   ret = drm_dp_dpcd_readb(aux, DP_TEST_SINK, &buf);
+   if (ret < 0)
+   return ret;
+
+   WARN_ON(!(buf & DP_TEST_SINK_START));
+
+   ret = drm_dp_dpcd_readb(aux, DP_TEST_SINK_MISC, &buf);
+   if (ret < 0)
+   return ret;
+
+   count = buf & DP_TEST_COUNT_MASK;
+   if (count == aux->crc_count)
+   return -EAGAIN; /* No CRC yet */
+
+   aux->crc_count = count;
+
+   /* At DP_TEST_CRC_R_CR, there's 6 bytes containing CRC data, 2 bytes
+* per component (RGB or CrYCb).
+*/
+   ret = drm_dp_dpcd_read(aux, DP_TEST_CRC_R_CR, crc, 6);
+   if (ret < 0)
+   return ret;
+
+   return 0;
+}
+
+static void drm_dp_aux_crc_work(struct work_struct *work)
+{
+   struct drm_dp_aux *aux = container_of(work, struct drm_dp_aux,
+ crc_work);
+   struct drm_crtc *crtc;
+   u8 crc_bytes[6];
+   uint32_t crcs[3];
+   int ret;
+
+   if (WARN_ON(!aux->connector))
+   return;
+
+   crtc = aux->connector->state->crtc;
+   while (crtc->crc.opened) {
+   drm_crtc_wait_one_vblank(crtc);
+   if (!crtc->crc.opened)
+   break;
+
+   ret = drm_dp_aux_get_crc(aux, crc_bytes);
+   if (ret == -EAGAIN) {
+   usleep_range(1000, 2000);
+   ret = drm_dp_aux_get_crc(aux, crc_bytes);
+   }
+
+   if (ret) {
+   DRM_DEBUG_KMS("Failed to get a CRC even after retrying: 
%d\n",
+ ret);
+   continue;
+   }
+
+   crcs[0] = crc_bytes[0] | crc_bytes[1] << 8;
+   crcs[1] = crc_bytes[2] | crc_bytes[3] << 8;
+   crcs[2] = crc_bytes[4] | crc_bytes[5] << 8;
+   ret = drm_crtc_add_crc_entry(crtc, false, 0, crcs);
+   if (!ret)
+   wake_up_interruptible_all(&crtc->crc.wq);
+   }
+}
+
 /**
  * drm_dp_aux_init() - minimally initialise an aux channel
  * @aux: DisplayPort AUX channel
@@ -993,6 +1063,7 @@ static const struct i2c_lock_operations 
drm_dp_i2c_lock_ops = {
 void drm_dp_aux_init(struct drm_dp_aux *aux)
 {
mutex_init(&aux->hw_mutex);
+   INIT_WORK(&aux->crc_work, drm_dp_aux_crc_work);
 
aux->ddc.algo = &drm_dp_i2c_algo;
aux->ddc.algo_data = aux;
@@ -1081,3 +1152,52 @@ int drm_dp_psr_setup_time(const u8 
psr_cap[EDP_PSR_RECEIVER_CAP_SIZE])
 EXPORT_SYMBOL(drm_dp_psr_setup_time);
 
 #undef PSR_SETUP_TIME
+
+/**
+ * drm_dp_start_crc() - start capture of frame CRCs
+ * @aux: DisplayPort AUX channel
+ *
+ * Returns 0 on success or a negative error code on failure.
+ */
+int drm_dp_start_crc(struct drm_dp_aux *aux)
+{
+   u8 buf;
+   int ret;
+
+   ret = drm_dp_dpcd_readb(aux, DP_TEST_SINK, &buf);
+   if (ret < 0)
+   return ret;
+
+   ret = drm_dp_dpcd_writeb(aux, DP_TEST_SINK, buf | DP_TEST_SINK_START);
+   if (ret < 0)
+   return ret;
+
+   aux->crc_count = 0;
+   schedule_work(&aux->crc_work);
+
+   return 0;
+}
+EXPORT_SYMBOL(drm_dp_start_crc);
+
+/**
+ * drm_dp_stop_crc() - stop capture of frame CRCs
+ * @aux: DisplayPort AUX channel
+ *
+ * Returns 0 on success or a negative error code on failure.
+ */
+int drm_dp_stop_crc(struct drm_dp_aux *aux)
+{
+   u8 buf;
+   int ret;
+
+   ret = drm_dp_dpcd_readb(aux, DP_TEST_SINK, &buf);
+   if (ret < 0)
+   return ret;
+
+   ret = drm_dp_dpcd_writeb(aux, DP_TEST_SINK, buf & ~DP_TEST_SINK_START);
+   if (ret < 0)
+   return ret;
+
+   return 0;
+}
+EXPORT_SYMBOL(drm_dp_stop_crc);
diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index 4fa77b434594..276e1ec

[PATCH v2 0/5] drm/dp: Implement CRC debugfs API

2017-01-05 Thread Tomeu Vizoso
Hi,

this series builds up on the API for exposing captured CRCs through
debugfs.

It adds new DP helpers for starting and stopping CRC capture and gets
the Rockchip driver to use it.

Also had to add a connector backpointer to the drm_dp_aux struct so we could
wait for the right vblank and store the CRCs afterwards, I will be glad
to hear about better alternatives.

Thanks,

Tomeu


Tomeu Vizoso (5):
  drm/dp: add connector backpointer to drm_dp_aux
  drm/bridge: analogix_dp: set connector to drm_dp_aux
  drm/dp: add helpers for capture of frame CRCs
  drm/bridge: analogix_dp: add helpers for capture of frame CRCs
  drm/rockchip: Implement CRC debugfs API

 drivers/gpu/drm/bridge/analogix/analogix_dp_core.c |  34 --
 drivers/gpu/drm/drm_dp_helper.c| 120 +
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c|  48 +
 include/drm/bridge/analogix_dp.h   |   3 +
 include/drm/drm_dp_helper.h|   9 ++
 5 files changed, 206 insertions(+), 8 deletions(-)

-- 
2.9.3



[PATCH v2 2/5] drm/bridge: analogix_dp: set connector to drm_dp_aux

2017-01-05 Thread Tomeu Vizoso
Set the backpointer so that the DP helpers are able to access the
connector that the drm_dp_aux is associated with.

Signed-off-by: Tomeu Vizoso 
---

 drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 18 ++
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c 
b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
index 02b97bf64ee4..7d45d3e4600a 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
@@ -1402,23 +1402,25 @@ int analogix_dp_bind(struct device *dev, struct 
drm_device *drm_dev,
dp->drm_dev = drm_dev;
dp->encoder = dp->plat_data->encoder;
 
+   ret = analogix_dp_create_bridge(drm_dev, dp);
+   if (ret) {
+   DRM_ERROR("failed to create bridge (%d)\n", ret);
+   goto err_encoder_cleanup;
+   }
+
dp->aux.name = "DP-AUX";
dp->aux.transfer = analogix_dpaux_transfer;
dp->aux.dev = &pdev->dev;
+   dp->aux.connector = &dp->connector;
 
ret = drm_dp_aux_register(&dp->aux);
if (ret)
-   goto err_disable_pm_runtime;
-
-   ret = analogix_dp_create_bridge(drm_dev, dp);
-   if (ret) {
-   DRM_ERROR("failed to create bridge (%d)\n", ret);
-   drm_encoder_cleanup(dp->encoder);
-   goto err_disable_pm_runtime;
-   }
+   goto err_encoder_cleanup;
 
return 0;
 
+err_encoder_cleanup:
+   drm_encoder_cleanup(dp->encoder);
 err_disable_pm_runtime:
pm_runtime_disable(dev);
 
-- 
2.9.3



  1   2   3   4   5   6   7   8   9   10   >