Re: [PATCH v10 0/4] drm/panfrost: Add support for mt8183 GPU
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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