Re: Dynamically change enumeration list of DRM enumeration property
Inline.. On Mon, Jun 1, 2020 at 2:19 PM Pekka Paalanen wrote: > On Mon, 1 Jun 2020 09:22:27 +0530 > Yogish Kulkarni wrote: > > > Hi, > > > > For letting DRM clients to select output encoding: > > Sink can support certain display timings with high output bit-depths > using > > multiple output encodings, e.g. sink can support a particular timing with > > RGB 10-bit, YCbCr422 10-bit and YCbCr420 10-bit. So DRM client may want > to > > select YCbCr422 10-bit over RBG 10-bit output to reduce the link > bandwidth > > (and in turn reduce power/voltage). If DRM driver automatically selects > > output encoding then we are restricting DRM clients from making > appropriate > > choice. > > Hi, > > right, that seems to be another reason. > > > For selectable output color range: > > Certain applications (typically graphics) usually rendered in full range > > while some applications (typically video) have limited range content. > Since > > content can change dynamically, DRM driver does not have enough > information > > to choose correct quantization. Only DRM client can correctly select > which > > quantization to set (to preserve artist's intent). > > Now this is an interesting topic for me. As far as I know, there is no > window system protocol to tell the display server whether the > application provided content is using full or limited range. This means > that the display server cannot tell DRM about full vs. limited range > either. It also means that when not fullscreen, the display server > cannot show the limited range video content correctly, because it would > have to be converted to full-range (or vice versa). > > Right, but there could be DRM client which doesn't use window system (e.g. Gstreamer video sink) and wants to select between full/limited color range. I agree that there is no window system protocol yet but maybe Wayland protocol could be added/extended for this purpose once we finalize things that needs to be done in DRM. But why would an application produce limited range pixels anyway? Is it > common that hardware video decoders are unable to produce full-range > pixels? > The primary reason for why content producer masters video/gfx content as limited range is for compatibility with sinks which only support limited range, and not because video decoders are not capable of decoding full-range content. Also, certain cinema-related content (e.g., movies) may be better suited for limited range encoding due to the level of detail that they need to present/hide (see "Why does limited RGB even exist?" section in https://www.benq.com/en-us/knowledge-center/knowledge/full-rgb-vs-limited-rgb-is-there-a-difference.html#:~:text=Full%20RGB%20means%20the%20ability,less%20dark)%20than%20full%20RGB ). I am asking, because I have a request to add limited vs. full range > information to Wayland. > > What about video sinks, including monitors? Are there devices that > accept limited-range only, full-range only, or switchable? > Yes, there are sinks which support selectable quantization range and there are sinks which don't. If the quantization range is not selectable, then in general, sources should output full-range for IT timings, and output limited for CE timings. At a high-level, IT timings are part of a standard developed by VESA for computer monitor-like displays. CE (Consumer Electronics) timings are a separate standard for timings more applicable to sinks like consumer TVs, etc. > > Why not just always use full-range everywhere? > > Or if a sink supports only limited-range, have the display chip > automatically convert from full-range, so that software doesn't have to > convert in software. > I think it is ok to convert from limited range to full range in display HW pipeline. By "automatically" if you mean display HW or DRM driver should look at the content to figure out whether it is limited range content and then program display pipeline to do the conversion, I don't think that is a good idea since we would need to inspect each pixel. Also, there may be some post-processing done to full-range content that happens to cause the pixel component values to fall within the limited quantization range. How about adding a new DRM KMS plane property to let client convey the driver about input content range? More details on this below. > > If you actually have a DRM KMS property for the range, does it mean that: > - the sink is configured to accept that range, and the pixels in the > framebuffer need to comply, or > - the display chip converts to that range while framebuffer remains in > full-range? > I would imagine this as: (1) Add new read DRM KMS connector property which DRM client will read to know whether sink support selectable quantization range. (2) Add new read/write DRM KMS connector property which DRM client will write to set output quantization range and read to know the current output quantization range. (3) Add new read/write DRM KMS plane property which DRM client will write to set
[Bug 206987] [drm] [amdgpu] Whole system crashes when the driver is in mode_support_and_system_configuration
https://bugzilla.kernel.org/show_bug.cgi?id=206987 --- Comment #25 from Petteri Aimonen (j...@kernelbug.mail.kapsi.fi) --- Looks like there are two kinds of crash bugs here. Many of the amdgpu crashes have been fixed in 5.7.0, but the specific one that gives "simd exception" in dmesg is not. @Cyrax There is an experimental patch in https://bugzilla.kernel.org/show_bug.cgi?id=207979 if you want to try. Out of interest, are you possibly running a 32-bit operating system under virtualization on 64-bit host? That's what triggers the bug for me. -- You are receiving this mail because: You are watching the assignee of the bug. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/connector: notify userspace on hotplug after register complete
drm connector notifies userspace on hotplug event prematurely before late_register and mode_object register completes. This leads to a race between userspace and kernel on updating the IDR list. So, move the notification to end of connector register. Signed-off-by: Jeykumar Sankaran Signed-off-by: Steve Cohen --- drivers/gpu/drm/drm_connector.c | 5 + drivers/gpu/drm/drm_sysfs.c | 3 --- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c index b1099e1..d877ddc 100644 --- a/drivers/gpu/drm/drm_connector.c +++ b/drivers/gpu/drm/drm_connector.c @@ -27,6 +27,7 @@ #include #include #include +#include #include @@ -523,6 +524,10 @@ int drm_connector_register(struct drm_connector *connector) drm_mode_object_register(connector->dev, >base); connector->registration_state = DRM_CONNECTOR_REGISTERED; + + /* Let userspace know we have a new connector */ + drm_sysfs_hotplug_event(connector->dev); + goto unlock; err_debugfs: diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c index 939f003..f0336c8 100644 --- a/drivers/gpu/drm/drm_sysfs.c +++ b/drivers/gpu/drm/drm_sysfs.c @@ -291,9 +291,6 @@ int drm_sysfs_connector_add(struct drm_connector *connector) return PTR_ERR(connector->kdev); } - /* Let userspace know we have a new connector */ - drm_sysfs_hotplug_event(dev); - if (connector->ddc) return sysfs_create_link(>kdev->kobj, >ddc->dev.kobj, "ddc"); -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[Bug 206987] [drm] [amdgpu] Whole system crashes when the driver is in mode_support_and_system_configuration
https://bugzilla.kernel.org/show_bug.cgi?id=206987 --- Comment #24 from Cyrax (ev...@hotmail.com) --- (In reply to Petteri Aimonen from comment #16) > I hit the same issue, using Ubuntu 20.04. It happened when switching window > to Firefox. For me it only crashed Xorg, ssh to the machine still worked ok. > Killing Xorg didn't work and `shutdown -r now` hung up somewhere. > > Here is a bug report on the Ubuntu package: > https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1881134 > > Here is call trace decoded with the debug symbols: > [clip] Yeah, it happens when switching windows and/or to different workspace. And yes it will crash Xorg only, other things will continue work as usual and issuing reboot command via SSH won't - well - reboot it. Only REISUB brings machine back to usable state. -- You are receiving this mail because: You are watching the assignee of the bug. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[Bug 206987] [drm] [amdgpu] Whole system crashes when the driver is in mode_support_and_system_configuration
https://bugzilla.kernel.org/show_bug.cgi?id=206987 --- Comment #23 from Cyrax (ev...@hotmail.com) --- Created attachment 289483 --> https://bugzilla.kernel.org/attachment.cgi?id=289483=edit used decode_stacktrace.sh to previous dmesg log -- You are receiving this mail because: You are watching the assignee of the bug. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[Bug 206987] [drm] [amdgpu] Whole system crashes when the driver is in mode_support_and_system_configuration
https://bugzilla.kernel.org/show_bug.cgi?id=206987 Cyrax (ev...@hotmail.com) changed: What|Removed |Added Kernel Version|5.7.0-rc3 |5.7.0 -- You are receiving this mail because: You are watching the assignee of the bug. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[Bug 206987] [drm] [amdgpu] Whole system crashes when the driver is in mode_support_and_system_configuration
https://bugzilla.kernel.org/show_bug.cgi?id=206987 --- Comment #22 from Cyrax (ev...@hotmail.com) --- Created attachment 289481 --> https://bugzilla.kernel.org/attachment.cgi?id=289481=edit config file used to build kernel 5.7.0 with KASAN etc -- You are receiving this mail because: You are watching the assignee of the bug. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[Bug 206987] [drm] [amdgpu] Whole system crashes when the driver is in mode_support_and_system_configuration
https://bugzilla.kernel.org/show_bug.cgi?id=206987 --- Comment #21 from Cyrax (ev...@hotmail.com) --- Created attachment 289479 --> https://bugzilla.kernel.org/attachment.cgi?id=289479=edit dmesg output kernel 5.7.0 -- You are receiving this mail because: You are watching the assignee of the bug. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[pull v2] drm/msm: msm-next for 5.8
Hi Dave, v2 with the dpu clk and bw scaling patch that had issues on armv7 reverted. updated description of original pull req: Not too huge this time around, but a bunch of interesting new stuff: * new gpu support: a405, a640, a650 * dpu: color processing support * mdp5: support for msm8x36 (the thing with a405) * some prep work for per-context pagetables (ie the part that does not depend on in-flight iommu patches) * last but not least, UABI update for submit ioctl to support syncobj (from Bas) The UABI change has been on-list and reviewed for a while now. The only reason I didn't pull it in last cycle was that I ran out of time to review it myself at the time. But I'm happy with it. The MR for mesa (vulkan/turnip) support is here: https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/2769 The following changes since commit 2ef96a5bb12be62ef75b5828c0aab838ebb29cb8: Linux 5.7-rc5 (2020-05-10 15:16:58 -0700) are available in the Git repository at: https://gitlab.freedesktop.org/drm/msm.git for you to fetch changes up to 1cb2c4a2c89b2004a36399860c85a1af9b3fcba7: Revert "drm/msm/dpu: add support for clk and bw scaling for display" (2020-06-01 20:56:18 -0700) Bas Nieuwenhuizen (1): drm/msm: Add syncobj support. Bjorn Andersson (1): drm/msm: Fix undefined "rd_full" link error Christophe JAILLET (2): drm/msm/a6xx: Fix a typo in an error message drm/msm: Fix typo Hongbo Yao (1): drm/msm/dpu: Fix compile warnings Jonathan Marek (10): drm/msm: add msm_gem_get_and_pin_iova_range drm/msm: add internal MSM_BO_MAP_PRIV flag drm/msm/a6xx: use msm_gem for GMU memory objects drm/msm/a6xx: add A640/A650 to gpulist drm/msm/a6xx: HFI v2 for A640 and A650 drm/msm/a6xx: A640/A650 GMU firmware path drm/msm/a6xx: update pdc/rscc GMU registers for A640/A650 drm/msm/a6xx: enable GMU log drm/msm/a6xx: update a6xx_hw_init for A640 and A650 drm/msm/a6xx: skip HFI set freq if GMU is powered down Jordan Crouse (4): drm/msm: Check for powered down HW in the devfreq callbacks drm/msm: Attach the IOMMU device during initialization drm/msm: Refactor address space initialization drm/msm: Update the MMU helper function APIs Kalyan Thota (3): drm/msm/dpu: add support for color processing blocks in dpu driver drm/msm/dpu: add support for pcc color block in dpu driver drm/msm/dpu: add support for clk and bw scaling for display Konrad Dybcio (1): drm/msm/mdp5: Add MDP5 configuration for MSM8x36. Krishna Manikandan (1): drm/msm/dpu: update bandwidth threshold check Rob Clark (1): Revert "drm/msm/dpu: add support for clk and bw scaling for display" Roy Spliet (1): drm/msm/mdp5: Fix mdp5_init error path for failed mdp5_kms allocation Shawn Guo (2): drm/msm/a4xx: add adreno a405 support drm/msm/a4xx: add a405_registers for a405 device kbuild test robot (2): drm/msm/a6xx: a6xx_hfi_send_start() can be static drm/msm/dpu: dpu_setup_dspp_pcc() can be static drivers/gpu/drm/msm/Makefile | 1 + drivers/gpu/drm/msm/adreno/a2xx_gpu.c | 16 + drivers/gpu/drm/msm/adreno/a3xx_gpu.c | 1 + drivers/gpu/drm/msm/adreno/a4xx_gpu.c | 83 - drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 7 + drivers/gpu/drm/msm/adreno/a6xx.xml.h | 14 + drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 418 +++-- drivers/gpu/drm/msm/adreno/a6xx_gmu.h | 37 ++- drivers/gpu/drm/msm/adreno/a6xx_gmu.xml.h | 48 +-- drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 70 - drivers/gpu/drm/msm/adreno/a6xx_hfi.c | 123 +++- drivers/gpu/drm/msm/adreno/a6xx_hfi.h | 50 ++- drivers/gpu/drm/msm/adreno/adreno_device.c | 35 +++ drivers/gpu/drm/msm/adreno/adreno_gpu.c| 27 +- drivers/gpu/drm/msm/adreno/adreno_gpu.h| 23 ++ drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 23 +- drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 95 -- drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h | 2 + drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c| 12 +- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 48 ++- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 39 +++ drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c | 26 ++ drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h | 3 + drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dspp.c| 129 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dspp.h| 100 ++ drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h| 2 + drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c| 18 +- drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h| 1 + drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 58 +++- drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h | 2 + drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c | 18 +-
Re: [PATCH 1/3] drm/atomic-helper: reset vblank on crtc reset
Hi Daniel,
Thank you for the patch.
May I remind you about the -v option to git-format-patch ? :-) Seriously
speaking, it really helps review.
On Tue, Jun 02, 2020 at 11:51:38AM +0200, Daniel Vetter wrote:
> Only when vblanks are supported ofc.
>
> Some drivers do this already, but most unfortunately missed it. This
> opens up bugs after driver load, before the crtc is enabled for the
> first time. syzbot spotted this when loading vkms as a secondary
> output. Given how many drivers are buggy it's best to solve this once
> and for all in shared helper code.
>
> Aside from moving the few existing calls to drm_crtc_vblank_reset into
> helpers (i915 doesn't use helpers, so keeps its own) I think the
> regression risk is minimal: atomic helpers already rely on drivers
> calling drm_crtc_vblank_on/off correctly in their hooks when they
> support vblanks. And driver that's failing to handle vblanks after
> this is missing those calls already, and vblanks could only work by
> accident when enabling a CRTC for the first time right after boot.
>
> Big thanks to Tetsuo for helping track down what's going wrong here.
>
> There's only a few drivers which already had the necessary call and
> needed some updating:
> - komeda, atmel and tidss also needed to be changed to call
> __drm_atomic_helper_crtc_reset() intead of open coding it
> - tegra and msm even had it in the same place already, just code
> motion, and malidp already uses __drm_atomic_helper_crtc_reset().
>
> Only call left is in i915, which doesn't use drm_mode_config_reset,
> but has its own fastboot infrastructure. So that's the only case where
> we actually want this in the driver still.
>
> I've also reviewed all other drivers which set up vblank support with
> drm_vblank_init. After the previous patch fixing mxsfb all atomic
> drivers do call drm_crtc_vblank_on/off as they should, the remaining
> drivers are either legacy kms or legacy dri1 drivers, so not affected
> by this change to atomic helpers.
>
> v2: Use the drm_dev_has_vblank() helper.
>
> v3: Laurent pointed out that omap and rcar-du used drm_crtc_vblank_off
> instead of drm_crtc_vblank_reset. Adjust them too.
>
> Cc: Laurent Pinchart
> Reviewed-by: Laurent Pinchart
> Reviewed-by: Boris Brezillon
> Acked-by: Liviu Dudau
> Acked-by: Thierry Reding
> Link:
> https://syzkaller.appspot.com/bug?id=0ba17d70d062b2595e1f061231474800f076c7cb
> Reported-by: Tetsuo Handa
> Reported-by: syzbot+0871b14ca2e2fb64f...@syzkaller.appspotmail.com
> Cc: Tetsuo Handa
> Cc: "James (Qian) Wang"
> Cc: Liviu Dudau
> Cc: Mihail Atanassov
> Cc: Brian Starkey
> Cc: Sam Ravnborg
> Cc: Boris Brezillon
> Cc: Nicolas Ferre
> Cc: Alexandre Belloni
> Cc: Ludovic Desroches
> Cc: Maarten Lankhorst
> Cc: Maxime Ripard
> Cc: Thomas Zimmermann
> Cc: David Airlie
> Cc: Daniel Vetter
> Cc: Thierry Reding
> Cc: Jonathan Hunter
> Cc: Jyri Sarha
> Cc: Tomi Valkeinen
> Cc: Rob Clark
> Cc: Sean Paul
> Cc: Brian Masney
> Cc: Emil Velikov
> Cc: zhengbin
> Cc: Thomas Gleixner
> Cc: linux-te...@vger.kernel.org
> Signed-off-by: Daniel Vetter
> ---
> drivers/gpu/drm/arm/display/komeda/komeda_crtc.c | 7 ++-
> drivers/gpu/drm/arm/malidp_drv.c | 1 -
> drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 7 ++-
> drivers/gpu/drm/drm_atomic_state_helper.c| 4
> drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c| 2 --
> drivers/gpu/drm/omapdrm/omap_drv.c | 3 ---
> drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 3 ---
> drivers/gpu/drm/tegra/dc.c | 1 -
> drivers/gpu/drm/tidss/tidss_crtc.c | 3 +--
> drivers/gpu/drm/tidss/tidss_kms.c| 4
> 10 files changed, 9 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> index 56bd938961ee..f33418d6e1a0 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> @@ -492,10 +492,8 @@ static void komeda_crtc_reset(struct drm_crtc *crtc)
> crtc->state = NULL;
>
> state = kzalloc(sizeof(*state), GFP_KERNEL);
> - if (state) {
> - crtc->state = >base;
> - crtc->state->crtc = crtc;
> - }
> + if (state)
> + __drm_atomic_helper_crtc_reset(crtc, >base);
> }
>
> static struct drm_crtc_state *
> @@ -616,7 +614,6 @@ static int komeda_crtc_add(struct komeda_kms_dev *kms,
> return err;
>
> drm_crtc_helper_add(crtc, _crtc_helper_funcs);
> - drm_crtc_vblank_reset(crtc);
>
> crtc->port = kcrtc->master->of_output_port;
>
> diff --git a/drivers/gpu/drm/arm/malidp_drv.c
> b/drivers/gpu/drm/arm/malidp_drv.c
> index c2507b7d8512..02904392e370 100644
> --- a/drivers/gpu/drm/arm/malidp_drv.c
> +++ b/drivers/gpu/drm/arm/malidp_drv.c
> @@ -870,7 +870,6 @@ static int malidp_bind(struct device *dev)
>
[Bug 207383] [Regression] 5.7 amdgpu/polaris11 gpf: amdgpu_atomic_commit_tail
https://bugzilla.kernel.org/show_bug.cgi?id=207383 Duncan (1i5t5.dun...@cox.net) changed: What|Removed |Added Summary|[Regression] 5.7-rc:|[Regression] 5.7 |amdgpu/polaris11 gpf: |amdgpu/polaris11 gpf: |amdgpu_atomic_commit_tail |amdgpu_atomic_commit_tail --- Comment #14 from Duncan (1i5t5.dun...@cox.net) --- Unfortunately the bug's still there in 5.7 release. =:^( Not properly bisected yet as after the first failure I needed something reasonably stable for awhile as I had about a dozen live-git kde-plasma userspace bugs to track down and report, but kernel 5.6.0-07388-gf365ab31e has been exactly that, stable for me, for weeks now (built May 6), and the bug definitely triggered in 5.7-rc1, so it's gotta be between those. With the unrelated userspace side mostly fixed now, and this kernelspace bug now known to remain unfixed in the normal development cycle, maybe I can get back to bisecting it again. -- You are receiving this mail because: You are watching the assignee of the bug. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v8 04/10] drm: bridge: dw_mipi_dsi: allow bridge daisy chaining
Hi Adrian,
Thank you for the patch.
On Mon, Apr 27, 2020 at 11:19:46AM +0300, Adrian Ratiu wrote:
> Up until now the assumption was that the synopsis dsi bridge will
> directly connect to an encoder provided by the platform driver, but
> the current practice for drivers is to leave the encoder empty via
> the simple encoder API and add their logic to their own drm_bridge.
>
> Thus we need an ablility to connect the DSI bridge to another bridge
> provided by the platform driver, so we extend the dw_mipi_dsi bind()
> API with a new "previous bridge" arg instead of just hardcoding NULL.
>
> Cc: Laurent Pinchart
> Signed-off-by: Adrian Ratiu
> ---
> New in v8.
> ---
> drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 6 --
> drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c | 2 +-
> include/drm/bridge/dw_mipi_dsi.h| 5 -
> 3 files changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> index 16fd87055e7b7..140ff40fa1b62 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> @@ -1456,11 +1456,13 @@ EXPORT_SYMBOL_GPL(dw_mipi_dsi_remove);
> /*
> * Bind/unbind API, used from platforms based on the component framework.
> */
> -int dw_mipi_dsi_bind(struct dw_mipi_dsi *dsi, struct drm_encoder *encoder)
> +int dw_mipi_dsi_bind(struct dw_mipi_dsi *dsi,
> + struct drm_encoder *encoder,
> + struct drm_bridge *prev_bridge)
> {
> int ret;
>
> - ret = drm_bridge_attach(encoder, >bridge, NULL, 0);
> + ret = drm_bridge_attach(encoder, >bridge, prev_bridge, 0);
Please note that chaining of bridges doesn't work well if multiple
bridges in the chain try to create a connector. This is why a
DRM_BRIDGE_ATTACH_NO_CONNECTOR flag has been added, with a helper to
create a connector for a chain of bridges (drm_bridge_connector_init()).
This won't play well with the component framework. I would recommend
using the of_drm_find_bridge() instead in the rockchip driver, and
deprecating dw_mipi_dsi_bind().
> if (ret) {
> DRM_ERROR("Failed to initialize bridge with drm\n");
> return ret;
> diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c
> b/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c
> index 3feff0c45b3f7..83ef43be78135 100644
> --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c
> @@ -929,7 +929,7 @@ static int dw_mipi_dsi_rockchip_bind(struct device *dev,
> return ret;
> }
>
> - ret = dw_mipi_dsi_bind(dsi->dmd, >encoder);
> + ret = dw_mipi_dsi_bind(dsi->dmd, >encoder, NULL);
> if (ret) {
> DRM_DEV_ERROR(dev, "Failed to bind: %d\n", ret);
> return ret;
> diff --git a/include/drm/bridge/dw_mipi_dsi.h
> b/include/drm/bridge/dw_mipi_dsi.h
> index b0e390b3288e8..699b3531f5b36 100644
> --- a/include/drm/bridge/dw_mipi_dsi.h
> +++ b/include/drm/bridge/dw_mipi_dsi.h
> @@ -14,6 +14,7 @@
> #include
>
> struct drm_display_mode;
> +struct drm_bridge;
> struct drm_encoder;
> struct dw_mipi_dsi;
> struct mipi_dsi_device;
> @@ -62,7 +63,9 @@ struct dw_mipi_dsi *dw_mipi_dsi_probe(struct
> platform_device *pdev,
> const struct dw_mipi_dsi_plat_data
> *plat_data);
> void dw_mipi_dsi_remove(struct dw_mipi_dsi *dsi);
> -int dw_mipi_dsi_bind(struct dw_mipi_dsi *dsi, struct drm_encoder *encoder);
> +int dw_mipi_dsi_bind(struct dw_mipi_dsi *dsi,
> + struct drm_encoder *encoder,
> + struct drm_bridge *prev_bridge);
> void dw_mipi_dsi_unbind(struct dw_mipi_dsi *dsi);
> void dw_mipi_dsi_set_slave(struct dw_mipi_dsi *dsi, struct dw_mipi_dsi
> *slave);
>
--
Regards,
Laurent Pinchart
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 7/7] dt-bindings: display: Document Cadence MHDP HDMI/DP bindings
Hi Sandor, Thank you for the patch. On Mon, Jun 01, 2020 at 02:17:37PM +0800, sandor...@nxp.com wrote: > From: Sandor Yu > > Document the bindings used for the Cadence MHDP HDMI/DP bridge. > > Signed-off-by: Sandor Yu > --- > .../bindings/display/bridge/cdns,mhdp.yaml| 46 +++ > .../devicetree/bindings/display/imx/mhdp.yaml | 59 +++ Please split the patch in two. > 2 files changed, 105 insertions(+) > create mode 100644 > Documentation/devicetree/bindings/display/bridge/cdns,mhdp.yaml > create mode 100644 Documentation/devicetree/bindings/display/imx/mhdp.yaml > > diff --git a/Documentation/devicetree/bindings/display/bridge/cdns,mhdp.yaml > b/Documentation/devicetree/bindings/display/bridge/cdns,mhdp.yaml > new file mode 100644 > index ..aa23feba744a > --- /dev/null > +++ b/Documentation/devicetree/bindings/display/bridge/cdns,mhdp.yaml > @@ -0,0 +1,46 @@ > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/display/bridge/cdns,mhdp.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Cadence MHDP TX Encoder > + > +maintainers: > + - Sandor Yu > + > +description: | > + Cadence MHDP Controller supports one or more of the protocols, > + such as HDMI and DisplayPort. > + Each protocol requires a different FW binaries. > + > + This document defines device tree properties for the Cadence MHDP Encoder > + (CDNS MHDP TX). It doesn't constitue a device tree binding > + specification by itself but is meant to be referenced by platform-specific > + device tree bindings. > + > + When referenced from platform device tree bindings the properties defined > in > + this document are defined as follows. The platform device tree bindings are > + responsible for defining whether each property is required or optional. > + > +properties: > + reg: > +maxItems: 1 > +description: Memory mapped base address and length of the MHDP TX > registers. > + > + interrupts: > +maxItems: 2 > + > + interrupt-names: > +- const: plug_in > + description: Hotplug detect interrupter for cable plugin event. > +- const: plug_out > + description: Hotplug detect interrupter for cable plugout event. Does the IP core really have two different interrupt lines, one for hot-plug and one for hot-unplug ? That's a very unusual design. > + > + port: > +type: object > +description: | > + The connectivity of the MHDP TX with the rest of the system is > + expressed in using ports as specified in the device graph bindings > defined > + in Documentation/devicetree/bindings/graph.txt. The numbering of the > ports > + is platform-specific. > diff --git a/Documentation/devicetree/bindings/display/imx/mhdp.yaml > b/Documentation/devicetree/bindings/display/imx/mhdp.yaml > new file mode 100644 > index ..17850cfd1cb1 > --- /dev/null > +++ b/Documentation/devicetree/bindings/display/imx/mhdp.yaml > @@ -0,0 +1,59 @@ > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/display/bridge/mhdp.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Cadence MHDP Encoder > + > +maintainers: > + - Sandor Yu > + > +description: | > + The MHDP transmitter is a Cadence HD Display TX controller IP > + with a companion PHY IP. > + The MHDP supports one or more of the protocols, > + such as HDMI(1.4 & 2.0), DisplayPort(1.2). > + switching between the two modes (HDMI and DisplayPort) > + requires reloading the appropriate FW Does the IP core integrated in the imx8mp SoCs (as that is what this binding targets) support both HDMI and DP ? If not this should be reworded to be more specific to the SoC. > + > + These DT bindings follow the Cadence MHDP TX bindings defined in > + Documentation/devicetree/bindings/display/bridge/cdns,mhdp.yaml with the > + following device-specific properties. > + > +Properties: Have you tried validating this with make dt_binding_check ? See Documentation/devicetree/writing-schema.rst for more information. > + compatible: > +enum: > + - nxp,imx8mq-cdns-hdmi > + - nxp,imx8mq-cdns-dp > + > + reg: See cdns,mhdp.yaml. This isn't how bindings are referenced. You need to reference the parent binding with $ref, either globally, or on an individual property basis. > + > + interrupts: See cdns,mhdp.yaml. > + > + interrupt-names: See cdns,mhdp.yaml. That's it ? No clocks, no power domains, no resets, no PHYs (especially given that you mention a PHY companion IP above) ? > + > + ports: See cdns,mhdp.yaml. This isn't correct. Please soo of-graph.txt. If can have either one port node, or one ports node that contains one of more port subnodes. In this case you need at least two ports, one for the input to the HDMI encoder, and one for the HDMI output. The latter should be connected to a DT node representing the HDMI
Re: [PATCH 3/7] drm: bridge: cadence: initial support for MHDP DP bridge driver
Hi Sandor,
Thank you for the patch.
On Mon, Jun 01, 2020 at 02:17:33PM +0800, sandor...@nxp.com wrote:
> From: Sandor Yu
>
> This adds initial support for MHDP DP bridge driver.
> Basic DP functions are supported, that include:
> -Video mode set on-the-fly
> -Cable hotplug detect
> -MAX support resolution to 3096x2160@60fps
> -Support DP audio
> -EDID read via AUX
>
> Signed-off-by: Sandor Yu
> ---
> drivers/gpu/drm/bridge/cadence/Kconfig| 4 +
> drivers/gpu/drm/bridge/cadence/Makefile | 1 +
> drivers/gpu/drm/bridge/cadence/cdns-dp-core.c | 530 ++
> .../gpu/drm/bridge/cadence/cdns-mhdp-audio.c | 100
> .../gpu/drm/bridge/cadence/cdns-mhdp-common.c | 42 +-
> .../gpu/drm/bridge/cadence/cdns-mhdp-common.h | 3 +
> drivers/gpu/drm/bridge/cadence/cdns-mhdp-dp.c | 34 +-
> drivers/gpu/drm/rockchip/cdn-dp-core.c| 7 +-
> include/drm/bridge/cdns-mhdp.h| 52 +-
> 9 files changed, 740 insertions(+), 33 deletions(-)
> create mode 100644 drivers/gpu/drm/bridge/cadence/cdns-dp-core.c
>
> diff --git a/drivers/gpu/drm/bridge/cadence/Kconfig
> b/drivers/gpu/drm/bridge/cadence/Kconfig
> index 48c1b0f77dc6..b7b8d30b18b6 100644
> --- a/drivers/gpu/drm/bridge/cadence/Kconfig
> +++ b/drivers/gpu/drm/bridge/cadence/Kconfig
> @@ -5,3 +5,7 @@ config DRM_CDNS_MHDP
> depends on OF
> help
> Support Cadence MHDP API library.
> +
> +config DRM_CDNS_DP
> + tristate "Cadence DP DRM driver"
> + depends on DRM_CDNS_MHDP
> diff --git a/drivers/gpu/drm/bridge/cadence/Makefile
> b/drivers/gpu/drm/bridge/cadence/Makefile
> index ddb2ba4fb852..cb3c88311a64 100644
> --- a/drivers/gpu/drm/bridge/cadence/Makefile
> +++ b/drivers/gpu/drm/bridge/cadence/Makefile
> @@ -1,3 +1,4 @@
> # SPDX-License-Identifier: GPL-2.0-only
> cdns_mhdp_drmcore-y := cdns-mhdp-common.o cdns-mhdp-audio.o cdns-mhdp-dp.o
> +cdns_mhdp_drmcore-$(CONFIG_DRM_CDNS_DP) += cdns-dp-core.o
> obj-$(CONFIG_DRM_CDNS_MHDP) += cdns_mhdp_drmcore.o
> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dp-core.c
> b/drivers/gpu/drm/bridge/cadence/cdns-dp-core.c
> new file mode 100644
> index ..b2fe8fdc64ed
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/cadence/cdns-dp-core.c
> @@ -0,0 +1,530 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Cadence Display Port Interface (DP) driver
> + *
> + * Copyright (C) 2019-2020 NXP Semiconductor, Inc.
> + *
> + */
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include "cdns-mhdp-common.h"
> +
> +/*
> + * This function only implements native DPDC reads and writes
> + */
> +static ssize_t dp_aux_transfer(struct drm_dp_aux *aux,
> + struct drm_dp_aux_msg *msg)
> +{
> + struct cdns_mhdp_device *mhdp = dev_get_drvdata(aux->dev);
> + bool native = msg->request & (DP_AUX_NATIVE_WRITE & DP_AUX_NATIVE_READ);
> + int ret;
> +
> + /* Ignore address only message */
> + if ((msg->size == 0) || (msg->buffer == NULL)) {
> + msg->reply = native ?
> + DP_AUX_NATIVE_REPLY_ACK : DP_AUX_I2C_REPLY_ACK;
> + return msg->size;
> + }
> +
> + if (!native) {
> + dev_err(mhdp->dev, "%s: only native messages supported\n",
> __func__);
> + return -EINVAL;
> + }
> +
> + /* msg sanity check */
> + if (msg->size > DP_AUX_MAX_PAYLOAD_BYTES) {
> + dev_err(mhdp->dev, "%s: invalid msg: size(%zu), request(%x)\n",
> + __func__, msg->size, (unsigned
> int)msg->request);
> + return -EINVAL;
> + }
> +
> + if (msg->request == DP_AUX_NATIVE_WRITE) {
> + const u8 *buf = msg->buffer;
> + int i;
> + for (i = 0; i < msg->size; ++i) {
> + ret = cdns_mhdp_dpcd_write(mhdp,
> +msg->address + i, buf[i]);
> + if (!ret)
> + continue;
> +
> + DRM_DEV_ERROR(mhdp->dev, "Failed to write DPCD\n");
> +
> + return ret;
> + }
> + msg->reply = DP_AUX_NATIVE_REPLY_ACK;
> + return msg->size;
> + }
> +
> + if (msg->request == DP_AUX_NATIVE_READ) {
> + ret = cdns_mhdp_dpcd_read(mhdp, msg->address, msg->buffer,
> msg->size);
> + if (ret < 0)
> + return -EIO;
> + msg->reply = DP_AUX_NATIVE_REPLY_ACK;
> + return msg->size;
> + }
> + return 0;
> +}
> +
> +static int dp_aux_init(struct cdns_mhdp_device *mhdp,
> + struct device *dev)
> +{
> + int ret;
> +
> + mhdp->dp.aux.name = "imx_dp_aux";
> + mhdp->dp.aux.dev = dev;
> + mhdp->dp.aux.transfer = dp_aux_transfer;
> +
> + ret =
Re: [PATCH 1/7] drm/rockchip: prepare common code for cdns and rk dpi/dp driver
On Tue, Jun 02, 2020 at 02:55:52PM +0100, Emil Velikov wrote:
> On Mon, 1 Jun 2020 at 07:29, wrote:
> >
> > From: Sandor Yu
> >
> > - Extracted common fields from cdn_dp_device to a new cdns_mhdp_device
> > structure which will be used by two separate drivers later on.
> > - Moved some datatypes (audio_format, audio_info, vic_pxl_encoding_format,
> > video_info) from cdn-dp-core.c to cdn-dp-reg.h.
> > - Changed prefixes from cdn_dp to cdns_mhdp
> > cdn -> cdns to match the other Cadence's drivers
> > dp -> mhdp to distinguish it from a "just a DP" as the IP underneath
> > this registers map can be a HDMI (which is internally different,
> > but the interface for commands, events is pretty much the same).
> > - Modified cdn-dp-core.c to use the new driver structure and new function
> > names.
> > - writel and readl are replaced by cdns_mhdp_bus_write and
> > cdns_mhdp_bus_read.
> >
> The high-level idea is great - split, refactor and reuse the existing drivers.
>
> Although looking at the patches themselves - they seems to be doing
> multiple things at once.
> As indicated by the extensive list in the commit log.
>
> I would suggest splitting those up a bit, roughly in line of the
> itemisation as per the commit message.
>
> Here is one hand wavy way to chunk this patch:
> 1) use put_unalligned*
> 2) 'use local variable dev' style of changes (as seem in cdn_dp_clk_enable)
> 3) add writel/readl wrappers
> 4) hookup struct cdns_mhdp_device, keep dp->mhdp detail internal.
> The cdn-dp-reg.h function names/signatures will stay the same.
> 5) finalize the helpers - use mhdp directly, rename
I second this, otherwise review is very hard.
> Examples:
> 4)
> static int cdn_dp_mailbox_read(struct cdn_dp_device *dp)
> {
> +" struct cdns_mhdp_device *mhdp = dp->mhdp;
>int val, ret;
>
> - ret = readx_poll_timeout(readl, dp->regs + MAILBOX_EMPTY_ADDR,
> + ret = readx_poll_timeout(readl, mhdp->regs_base + MAILBOX_EMPTY_ADDR,
> ...
>return fancy_readl(dp, MAILBOX0_RD_DATA) & 0xff;
> }
>
> 5)
> -static int cdn_dp_mailbox_read(struct cdn_dp_device *dp)
> +static int mhdp_mailbox_read(struct cdns_mhdp_device *mhdp)
> {
> - struct cdns_mhdp_device *mhdp = dp->mhdp;
>int val, ret;
> ...
> - return fancy_readl(dp, MAILBOX0_RD_DATA) & 0xff;
> + return cdns_mhdp_bus_read(mhdp, MAILBOX0_RD_DATA) & 0xff;
> }
--
Regards,
Laurent Pinchart
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
linux-next: manual merge of the drm-intel-fixes tree with Linus' tree
Hi all,
Today's linux-next merge of the drm-intel-fixes tree got a conflict in:
drivers/gpu/drm/i915/gt/intel_lrc.c
between commit:
f53ae29c0ea1 ("drm/i915/gt: Include a few tracek for timeslicing")
from Linus' tree and commit:
00febf644648 ("drm/i915/gt: Incorporate the virtual engine into timeslicing")
from the drm-intel-fixes tree.
I fixed it up (see below) and can carry the fix as necessary. This
is now fixed as far as linux-next is concerned, but any non trivial
conflicts should be mentioned to your upstream maintainer when your tree
is submitted for merging. You may also want to consider cooperating
with the maintainer of the conflicting tree to minimise any particularly
complex conflicts.
--
Cheers,
Stephen Rothwell
diff --cc drivers/gpu/drm/i915/gt/intel_lrc.c
index 87e6c5bdd2dc,e77f89b43e5f..
--- a/drivers/gpu/drm/i915/gt/intel_lrc.c
+++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
@@@ -1971,20 -1853,12 +1990,19 @@@ static void set_timeslice(struct intel_
if (!intel_engine_has_timeslices(engine))
return;
- set_timer_ms(>execlists.timer, active_timeslice(engine));
+ duration = active_timeslice(engine);
+ ENGINE_TRACE(engine, "bump timeslicing, interval:%lu", duration);
+
+ set_timer_ms(>execlists.timer, duration);
}
- static void start_timeslice(struct intel_engine_cs *engine)
+ static void start_timeslice(struct intel_engine_cs *engine, int prio)
{
struct intel_engine_execlists *execlists = >execlists;
- const int prio = queue_prio(execlists);
+ unsigned long duration;
+
+ if (!intel_engine_has_timeslices(engine))
+ return;
WRITE_ONCE(execlists->switch_priority_hint, prio);
if (prio == INT_MIN)
@@@ -2140,13 -1994,8 +2158,13 @@@ static void execlists_dequeue(struct in
__unwind_incomplete_requests(engine);
last = NULL;
- } else if (need_timeslice(engine, last) &&
+ } else if (need_timeslice(engine, last, rb) &&
timeslice_expired(execlists, last)) {
+ if (i915_request_completed(last)) {
+ tasklet_hi_schedule(>tasklet);
+ return;
+ }
+
ENGINE_TRACE(engine,
"expired last=%llx:%lld, prio=%d, hint=%d,
yield?=%s\n",
last->fence.context,
pgp8LJ42VwoiO.pgp
Description: OpenPGP digital signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 00/15] forward MSIx vector enable error code in pci_alloc_irq_vectors_affinity
On Tue, Jun 02, 2020 at 11:16:17AM +0200, Piotr Stankiewicz wrote: > The primary objective of this patch series is to change the behaviour > of pci_alloc_irq_vectors_affinity such that it forwards the MSI-X enable > error code when appropriate. In the process, though, it was pointed out > that there are multiple places in the kernel which check/ask for message > signalled interrupts (MSI or MSI-X), which spawned the first patch adding > PCI_IRQ_MSI_TYPES. Finally the rest of the chain converts all users to > take advantage of PCI_IRQ_MSI_TYPES or PCI_IRQ_ALL_TYPES, as > appropriate. > > Piotr Stankiewicz (15): > PCI: add shorthand define for message signalled interrupt types > PCI/MSI: forward MSIx vector enable error code in > pci_alloc_irq_vectors_affinity > PCI: use PCI_IRQ_MSI_TYPES where appropriate > ahci: use PCI_IRQ_MSI_TYPES where appropriate > crypto: inside-secure - use PCI_IRQ_MSI_TYPES where appropriate > dmaengine: dw-edma: use PCI_IRQ_MSI_TYPES where appropriate > drm/amdgpu: use PCI_IRQ_MSI_TYPES where appropriate > IB/qib: Use PCI_IRQ_MSI_TYPES where appropriate > media: ddbridge: use PCI_IRQ_MSI_TYPES where appropriate > vmw_vmci: use PCI_IRQ_ALL_TYPES where appropriate > mmc: sdhci: use PCI_IRQ_MSI_TYPES where appropriate > amd-xgbe: use PCI_IRQ_MSI_TYPES where appropriate > aquantia: atlantic: use PCI_IRQ_ALL_TYPES where appropriate > net: hns3: use PCI_IRQ_MSI_TYPES where appropriate > scsi: use PCI_IRQ_MSI_TYPES and PCI_IRQ_ALL_TYPES where appropriate > > Documentation/PCI/msi-howto.rst | 5 +++-- > drivers/ata/ahci.c| 2 +- > drivers/crypto/inside-secure/safexcel.c | 2 +- > drivers/dma/dw-edma/dw-edma-pcie.c| 2 +- > drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c | 8 > drivers/infiniband/hw/qib/qib_pcie.c | 2 +- > drivers/media/pci/ddbridge/ddbridge-main.c| 2 +- > drivers/misc/vmw_vmci/vmci_guest.c| 3 +-- > drivers/mmc/host/sdhci-pci-gli.c | 3 +-- > drivers/mmc/host/sdhci-pci-o2micro.c | 3 +-- > drivers/net/ethernet/amd/xgbe/xgbe-pci.c | 2 +- > drivers/net/ethernet/aquantia/atlantic/aq_pci_func.c | 4 +--- > drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c | 3 +-- > drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c | 2 +- > drivers/pci/msi.c | 4 ++-- > drivers/pci/pcie/portdrv_core.c | 4 ++-- > drivers/pci/switch/switchtec.c| 3 +-- > drivers/scsi/ipr.c| 2 +- > drivers/scsi/vmw_pvscsi.c | 2 +- > include/linux/pci.h | 4 ++-- > 20 files changed, 28 insertions(+), 34 deletions(-) I think I'm OK with this, and since they all depend on the first PCI patch, it will probably be easiest to merge them all through the PCI tree. I'm happy to do that, but can you please: - Update the subject lines so they start with a capital letter to match the historical convention. - Use "MSI-X" instead of "MSIx" so it matches the spec and other usage in the kernel. - Add "()" after function names, e.g., "pci_alloc_irq_vectors_affinity()" instead of "pci_alloc_irq_vectors_affinity". - Reorder them so the actual fix (02/15) is first and the cleanups later. - Post them all to linux-pci (I only saw the drivers/pci patches). - If possible, post them with all the patches as replies to the cover letter. These all appear to be unrelated messages, which makes it a bit of a hassle to collect them all up. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [git pull] drm for 5.8-rc1
On Wed, 3 Jun 2020 at 08:14, Linus Torvalds wrote: > > On Mon, Jun 1, 2020 at 11:06 PM Dave Airlie wrote: > > > > I've pushed a merged by me tree here, which I think gets them all > > correct, but please let me know if you think different. > > https://cgit.freedesktop.org/~airlied/linux/log/?h=drm-5.8-merged > > Ok, I get the same result, except my resolution to the simple encoder > issue was slightly different. I removed the simple helper header > include too as part of basically undoing the whole simple encoder > conversion. Yes sounds like my experience. I spent time on the tides and it was a revert pretty much of the commit in next, I just missed the header include line. I also realised I'd likely mismerged earlier when fixing this up, I'm going to have to put more time into merge fixing up, I'm still not always happy with my methods of figuring out what the correct answer is. > But other than that we're identical, which is a good sign. Apparently > the drm mis-merge in the middle got fixed up. Cool, thanks for redoing it, since this was definitely one of the more conflicty ones I've had in a while. Dave. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 22/22] drm: mxsfb: Support the alpha plane
Hi Emil, On Sun, May 31, 2020 at 05:54:04PM +0100, Emil Velikov wrote: > HI Laurent, > > From a quick glance the series looks really good and neat. Thank you :-) > Then again, I don't know much about the hardware to provide meaningful > review. > > A couple of small ideas below. > > On Sat, 30 May 2020 at 04:11, Laurent Pinchart wrote: > > > +#define LCDC_AS_BUF0x220 > > +#define LCDC_AS_NEXT_BUF 0x230 > > s/LCDC_AS_BUF/LCDC_AS_CUR_BUF/ - to stay consistent with the primary > plane name scheme. The register names come directly from the datasheet (and yes, the datasheet uses CUR_BUF and NEXT_BUF for the primary plane, and BUF and NEXT_BUF for the overlay plane :-S). I'd thus rather keep them aligned with the documentation. > Would it make sense to store the above registers in mxsfb_devdata, > just like we do for the primary planes? The reason the register addresses are stored in mxsfb_devdata for the primary plane is because they differ between hardware revisions (don't they teach hardware engineers in school these days *not* to move registers around ? :-)). The overlay plane is only supported in the latest versions of the IP core, and are always located at the same address as far as I can tell. I don't think we need an extra level of indirection. -- Regards, Laurent Pinchart ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [git pull] drm for 5.8-rc1
The pull request you sent on Tue, 2 Jun 2020 16:06:32 +1000: > git://anongit.freedesktop.org/drm/drm tags/drm-next-2020-06-02 has been merged into torvalds/linux.git: https://git.kernel.org/torvalds/c/faa392181a0bd42c5478175cef601adeecdc91b6 Thank you! -- Deet-doot-dot, I am a bot. https://korg.wiki.kernel.org/userdoc/prtracker ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [git pull] drm for 5.8-rc1
On Mon, Jun 1, 2020 at 11:06 PM Dave Airlie wrote: > > I've pushed a merged by me tree here, which I think gets them all > correct, but please let me know if you think different. > https://cgit.freedesktop.org/~airlied/linux/log/?h=drm-5.8-merged Ok, I get the same result, except my resolution to the simple encoder issue was slightly different. I removed the simple helper header include too as part of basically undoing the whole simple encoder conversion. But other than that we're identical, which is a good sign. Apparently the drm mis-merge in the middle got fixed up. Linus ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [git pull] drm for 5.8-rc1
On Tue, Jun 2, 2020 at 2:21 PM Linus Torvalds
wrote:
>
> I'm still working through the rest of the merge, so far that was the
> only one that made me go "Whaa?".
Hmm. I'm also ending up effectively reverting the drm commit
b28ad7deb2f2 ("drm/tidss: Use simple encoder") because commit
9da67433f64e ("drm/tidss: fix crash related to accessing freed
memory") made the premise of that simply encoder commit no longer be
true.
If there is a better way to sort that out (ie something like "use
simple encoder but make it free things at destroy time"), I don't know
of it.
I'll let you guys fight it out (added people involved with those
commits to the participants,
Linus
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [git pull] drm for 5.8-rc1
On Tue, Jun 2, 2020 at 2:21 PM Linus Torvalds
wrote:
>
> Hmm. Some of them are due to your previous mis-merges.
>
> Your commit 937eea297e26 ("Merge tag 'amd-drm-next-5.8-2020-04-24' of
> git://people.freedesktop.org/~agd5f/linux into drm-next") seems to
> have mis-merged the CONFIG_DEBUG_FS thing in
> drivers/gpu/drm/amd/display/dc/core/dc_link_dp.c.
Sorry, wrong filename. That should have been
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c, I cut-and-pasted
the wrong path from the conflict list..
Linus
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [git pull] drm for 5.8-rc1
On Mon, Jun 1, 2020 at 11:06 PM Dave Airlie wrote:
>
> This tree is a bit conflicty, the i915 ones are probably the hairy
> ones, but amdgpu has a bunch as well, along with smattering of others.
Hmm. Some of them are due to your previous mis-merges.
Your commit 937eea297e26 ("Merge tag 'amd-drm-next-5.8-2020-04-24' of
git://people.freedesktop.org/~agd5f/linux into drm-next") seems to
have mis-merged the CONFIG_DEBUG_FS thing in
drivers/gpu/drm/amd/display/dc/core/dc_link_dp.c.
I'm still working through the rest of the merge, so far that was the
only one that made me go "Whaa?".
Linus
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/2] drm/panel: simple: Add support for KOE TX26D202VM0BWA panel
Hi Emil.
On Tue, Jun 02, 2020 at 01:46:19PM +0100, Emil Velikov wrote:
> On Tue, 2 Jun 2020 at 08:17, Liu Ying wrote:
> >
> > This patch adds support for Kaohsiung Opto-Electronics Inc.
> > 10.1" TX26D202VM0BWA WUXGA(1920x1200) TFT LCD panel with LVDS interface.
> > The panel has dual LVDS channels.
> >
> > My panel is manufactured by US Micro Products(USMP). There is a tag at
> > the back of the panel, which indicates the panel type is 'TX26D202VM0BWA'
> > and it's made by KOE in Taiwan.
> >
> > The panel spec from USMP can be found at:
> > https://www.usmicroproducts.com/sites/default/files/datasheets/USMP-T101-192120NDU-A0.pdf
> >
> > The below panel spec from KOE is basically the same to the one from USMP.
> > However, the panel type 'TX26D202VM0BAA' is a little bit different.
> > It looks that the two types of panel are compatible with each other.
> > http://www.koe.j-display.com/upload/product/TX26D202VM0BAA.pdf
> >
> > Cc: Thierry Reding
> > Cc: Sam Ravnborg
> > Signed-off-by: Liu Ying
> > ---
> > drivers/gpu/drm/panel/panel-simple.c | 34
> > ++
> > 1 file changed, 34 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/panel/panel-simple.c
> > b/drivers/gpu/drm/panel/panel-simple.c
> > index b6ecd15..7c222ec 100644
> > --- a/drivers/gpu/drm/panel/panel-simple.c
> > +++ b/drivers/gpu/drm/panel/panel-simple.c
> > @@ -2200,6 +2200,37 @@ static const struct panel_desc koe_tx14d24vm1bpa = {
> > },
> > };
> >
> > +static const struct display_timing koe_tx26d202vm0bwa_timing = {
> > + .pixelclock = { 15182, 15672, 15978 },
> > + .hactive = { 1920, 1920, 1920 },
> > + .hfront_porch = { 105, 130, 142 },
> > + .hback_porch = { 45, 70, 82 },
> > + .hsync_len = { 30, 30, 30 },
> > + .vactive = { 1200, 1200, 1200},
> > + .vfront_porch = { 3, 5, 10 },
> > + .vback_porch = { 2, 5, 10 },
> > + .vsync_len = { 5, 5, 5 },
> > +};
> > +
> > +static const struct panel_desc koe_tx26d202vm0bwa = {
> > + .timings = _tx26d202vm0bwa_timing,
> > + .num_timings = 1,
> > + .bpc = 8,
> > + .size = {
> > + .width = 217,
> > + .height = 136,
> > + },
> > + .delay = {
> > + .prepare = 1000,
> > + .enable = 1000,
> > + .unprepare = 1000,
> > + .disable = 1000,
> Ouch 1s for each delay is huge. Nevertheless it matches the specs so,
> the series is:
> Reviewed-by: Emil Velikov
>
> Sam, Thierry I assume you'll merge the series. Let me know if I should
> pick it up.
I am quite busy with non-linux stuff these days so fine if you can pick
them up. I like that simple panel patches are processed fast.
I expect to have some hours for linux work friday or saturday, but no
promises...
Sam
>
> -Emil
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] video: uvesafb: use true,false for bool variables
Hi Bartlomiej
On Mon, Jun 01, 2020 at 12:37:00PM +0200, Bartlomiej Zolnierkiewicz wrote:
>
> Hi,
>
> On 4/22/20 9:18 AM, Jason Yan wrote:
> > Fix the following coccicheck warning:
> >
> > drivers/video/fbdev/uvesafb.c:48:12-17: WARNING: Assignment of 0/1 to
> > bool variable
> > drivers/video/fbdev/uvesafb.c:1827:3-13: WARNING: Assignment of 0/1 to
> > bool variable
> > drivers/video/fbdev/uvesafb.c:1829:3-13: WARNING: Assignment of 0/1 to
> > bool variable
> > drivers/video/fbdev/uvesafb.c:1835:3-9: WARNING: Assignment of 0/1 to
> > bool variable
> > drivers/video/fbdev/uvesafb.c:1837:3-9: WARNING: Assignment of 0/1 to
> > bool variable
> > drivers/video/fbdev/uvesafb.c:1839:3-8: WARNING: Assignment of 0/1 to
> > bool variable
> >
> > Signed-off-by: Jason Yan
> > ---
> > drivers/video/fbdev/uvesafb.c | 12 ++--
> > 1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/video/fbdev/uvesafb.c b/drivers/video/fbdev/uvesafb.c
> > index 1b385cf76110..bee29aadc646 100644
> > --- a/drivers/video/fbdev/uvesafb.c
> > +++ b/drivers/video/fbdev/uvesafb.c
> > @@ -45,7 +45,7 @@ static const struct fb_fix_screeninfo uvesafb_fix = {
> > };
> >
> > static int mtrr= 3;/* enable mtrr by default */
> > -static bool blank = 1;/* enable blanking by default */
> > +static bool blank = true; /* enable blanking by default */
> > static int ypan= 1;/* 0: scroll, 1: ypan, 2: ywrap */
> > static bool pmi_setpal = true; /* use PMI for palette changes */
> > static bool nocrtc;/* ignore CRTC settings */
> > @@ -1824,19 +1824,19 @@ static int uvesafb_setup(char *options)
> > else if (!strcmp(this_opt, "ywrap"))
> > ypan = 2;
> > else if (!strcmp(this_opt, "vgapal"))
> > - pmi_setpal = 0;
> > + pmi_setpal = false;
> > else if (!strcmp(this_opt, "pmipal"))
> > - pmi_setpal = 1;
> > + pmi_setpal = true;
> > else if (!strncmp(this_opt, "mtrr:", 5))
> > mtrr = simple_strtoul(this_opt+5, NULL, 0);
> > else if (!strcmp(this_opt, "nomtrr"))
> > mtrr = 0;
> > else if (!strcmp(this_opt, "nocrtc"))
> > - nocrtc = 1;
> > + nocrtc = true;
> > else if (!strcmp(this_opt, "noedid"))
> > - noedid = 1;
> > + noedid = true;
> > else if (!strcmp(this_opt, "noblank"))
> > - blank = 0;
> > + blank = true;
>
> The above conversion is incorrect.
>
> The follow-up fix is included below (the original patch has been
> already applied).
Good spot, sorry for missing this when I applied the original patch.
>
> Best regards,
> --
> Bartlomiej Zolnierkiewicz
> Samsung R Institute Poland
> Samsung Electronics
>
>
> From: Bartlomiej Zolnierkiewicz
> Subject: [PATCH] video: fbdev: uvesafb: fix "noblank" option handling
>
> Fix the recent regression.
>
> Fixes: dbc7ece12a38 ("video: uvesafb: use true,false for bool variables")
> Cc: Jason Yan
> Cc: Sam Ravnborg
> Cc: Michal Januszewski
> Signed-off-by: Bartlomiej Zolnierkiewicz
Reviewed-by: Sam Ravnborg
> ---
> drivers/video/fbdev/uvesafb.c |2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> Index: b/drivers/video/fbdev/uvesafb.c
> ===
> --- a/drivers/video/fbdev/uvesafb.c
> +++ b/drivers/video/fbdev/uvesafb.c
> @@ -1836,7 +1836,7 @@ static int uvesafb_setup(char *options)
> else if (!strcmp(this_opt, "noedid"))
> noedid = true;
> else if (!strcmp(this_opt, "noblank"))
> - blank = true;
> + blank = false;
> else if (!strncmp(this_opt, "vtotal:", 7))
> vram_total = simple_strtoul(this_opt + 7, NULL, 0);
> else if (!strncmp(this_opt, "vremap:", 7))
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 000/105] drm/vc4: Support BCM2711 Display Pipeline
Hi Maxime, Am 27.05.20 um 17:47 schrieb Maxime Ripard: > Hi everyone, > > Here's a (pretty long) series to introduce support in the VC4 DRM driver > for the display pipeline found in the BCM2711 (and thus the RaspberryPi 4). > > The main differences are that there's two HDMI controllers and that there's > more pixelvalve now. Those pixelvalve come with a mux in the HVS that still > have only 3 FIFOs. Both of those differences are breaking a bunch of > expectations in the driver, so we first need a good bunch of cleanup and > reworks to introduce support for the new controllers. > > Similarly, the HDMI controller has all its registers shuffled and split in > multiple controllers now, so we need a bunch of changes to support this as > well. > > Only the HDMI support is enabled for now (even though the DPI output has > been tested too). > > This is based on the firmware clocks series sent separately: > https://lore.kernel.org/lkml/cover.662a8d401787ef33780d91252a352de91dc4be10.1590594293.git-series.max...@cerno.tech/ > > Let me know if you have any comments > Maxime > > Cc: bcm-kernel-feedback-l...@broadcom.com > Cc: devicet...@vger.kernel.org > Cc: Kamal Dasu > Cc: linux-...@vger.kernel.org > Cc: Michael Turquette > Cc: Philipp Zabel > Cc: Rob Herring > Cc: Stephen Boyd > > Changes from v2: > - Rebased on top of next-20200526 i assume this is the reason why this series doesn't completely apply against drm-misc-next. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 032/105] drm/vc4: crtc: Enable and disable the PV in atomic_enable / disable
Hi, Am 02.06.20 um 21:31 schrieb Eric Anholt: > On Tue, Jun 2, 2020 at 8:02 AM Dave Stevenson > wrote: >> Hi Maxime and Eric >> >> On Tue, 2 Jun 2020 at 15:12, Maxime Ripard wrote: >>> Hi Eric >>> >>> On Wed, May 27, 2020 at 09:54:44AM -0700, Eric Anholt wrote: On Wed, May 27, 2020 at 8:50 AM Maxime Ripard wrote: > The VIDEN bit in the pixelvalve currently being used to enable or disable > the pixelvalve seems to not be enough in some situations, which whill end > up with the pixelvalve stalling. > > In such a case, even re-enabling VIDEN doesn't bring it back and we need > to > clear the FIFO. This can only be done if the pixelvalve is disabled > though. > > In order to overcome this, we can configure the pixelvalve during > mode_set_no_fb, but only enable it in atomic_enable and flush the FIFO > there, and in atomic_disable disable the pixelvalve again. What displays has this been tested with? Getting this sequencing right is so painful, and things like DSI are tricky to get to light up. >>> That FIFO is between the HVS and the HDMI PVs, so this was obviously >>> tested against that. Dave also tested the DSI output IIRC, so we should >>> be covered here. >> DSI wasn't working on the first patch set that Maxime sent - I haven't >> tested this one as yet but will do so. >> DPI was working early on to both an Adafruit 800x480 DPI panel, and >> via a VGA666 as VGA. >> HDMI is obviously working. >> VEC is being ignored now. The clock structure is more restricted than >> earlier chips, so to get the required clocks for the VEC without using >> fractional divides it compromises the clock that other parts of the >> system can run at (IIRC including the ARM). That's why the VEC has to >> be explicitly enabled for the firmware to enable it as the only >> output. It's annoying, but that's just a restriction of the chip. > I'm more concerned with "make sure we don't regress pre-pi4 with this > series" than "pi4 displays all work from the beginning" unfortuntely i can confirm this. With this patch series (using Maxime's git repo with multi_v7_defconfig) my Raspberry Pi 3 B hangs up while starting X (screen stays black, heartbeat stops, no more output at the debug UART). AFAIR v2 didn't had this issue. Stefan > > ___ > linux-arm-kernel mailing list > linux-arm-ker...@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm: document how user-space should use link-status
On Tue, Jun 02, 2020 at 11:58:36AM +0200, Daniel Vetter wrote:
> On Tue, Jun 02, 2020 at 10:38:46AM +0300, Pekka Paalanen wrote:
> > On Mon, 01 Jun 2020 14:48:58 +
> > Simon Ser wrote:
> >
> > > Describe what a "BAD" link-status means for user-space and how it should
> > > handle it. The logic described has been implemented in igt [1].
> > >
> > > [1]:
> > > https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/commit/fbe61f529737191d0920521946a575bd55f00fbe
> > >
> > > Signed-off-by: Simon Ser
> > > Cc: Daniel Vetter
> > > Cc: Manasi Navare
> > > Cc: Pekka Paalanen
> > > ---
> > > drivers/gpu/drm/drm_connector.c | 6 ++
> > > 1 file changed, 6 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/drm_connector.c
> > > b/drivers/gpu/drm/drm_connector.c
> > > index f2b20fd66319..08ba84f9787a 100644
> > > --- a/drivers/gpu/drm/drm_connector.c
> > > +++ b/drivers/gpu/drm/drm_connector.c
> > > @@ -994,6 +994,12 @@ static const struct drm_prop_enum_list
> > > dp_colorspaces[] = {
> > > * after modeset, the kernel driver may set this to "BAD" and issue
> > > a
> > > * hotplug uevent. Drivers should update this value using
> > > * drm_connector_set_link_status_property().
> > > + *
> > > + * When user-space receives the hotplug uevent and detects a "BAD"
> > > + * link-status, the connector is no longer enabled. The list of
> > > available
>
> "no longer enabled" is kinda wrong, you can keep doing pageflip. It's just
> that the pixels aren't getting to the screen anymore.
>
> "enabled" wrt an output has a different meaning in kms, the internal
> drm_crtc_state->enabled state is very much still set. Including what that
> all means for the uapi.
Yes I was about to comment on that too. And here we should mention, that rather
when user space recieves the hotplug uevent and detects a BAD link status, that
means connector's physical link failed to train correctly hence no output across
the link and a black screen seen on the display. In this case the userspace
should respond to this uevent by reprobing the connector to get a modelist
now as per the new capabilities of the connector after the fallback in link
rate/lane count
and retry a full modeset resetting the link-status to GOOD
Also to answer Pekka's concern here about modelist being empty, this should not
happen
since the kernel fallsback the link capabilities until it reaches the lowest
RBR and 1 lane count
and with this most panels should be still able to do the lowest available mode
in their modelist.
And likely the link training will pass for this minimum resolution and minimum
capabilities.
Manasi
>
> Also I think we need some words here on what automatically happens when
> you do an atomic commit with the connector with the bad link status
> (auto-reset to good, which might make the atomic modeset fail). On irc we
> had some discussions that we should only do this if ALLOW_MODESET is set,
> but that's atm not the case.
> -Daniel
>
> > > + * modes may have changed. User-space is expected to pick a new
> > > mode if
> > > + * the current one has disappeared and perform a new modeset with
> > > + * link-status set to "GOOD" to re-enable the connector.
> > > * non_desktop:
> > > * Indicates the output should be ignored for purposes of
> > > displaying a
> > > * standard desktop environment or console. This is most likely
> > > because
> >
> > Hi,
> >
> > makes sense to me. Can it happen that there will be no modes left in
> > the list?
> >
> > What if userspace is driving two connectors from the same CRTC, and only
> > one connector gets link-status bad, what does it mean? Is the other
> > connector still running as normal, as if the failed connector didn't
> > even exist?
> >
> > That is mostly a question about what happens if userspace does not fix
> > up the link-status=bad connector and does not detach it from the CRTC,
> > but keeps on flipping or modesetting as if the failure never happened.
> > I guess I could ask it about both a CRTC that has another connector
> > still good, and a CRTC where the failed connector was the only one.
> >
> > Can I trust that if the other connector is in any way affected, it too
> > will get link-status bad?
> >
> >
> > Thanks,
> > pq
>
>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 032/105] drm/vc4: crtc: Enable and disable the PV in atomic_enable / disable
On Tue, Jun 2, 2020 at 8:02 AM Dave Stevenson wrote: > > Hi Maxime and Eric > > On Tue, 2 Jun 2020 at 15:12, Maxime Ripard wrote: > > > > Hi Eric > > > > On Wed, May 27, 2020 at 09:54:44AM -0700, Eric Anholt wrote: > > > On Wed, May 27, 2020 at 8:50 AM Maxime Ripard wrote: > > > > > > > > The VIDEN bit in the pixelvalve currently being used to enable or > > > > disable > > > > the pixelvalve seems to not be enough in some situations, which whill > > > > end > > > > up with the pixelvalve stalling. > > > > > > > > In such a case, even re-enabling VIDEN doesn't bring it back and we > > > > need to > > > > clear the FIFO. This can only be done if the pixelvalve is disabled > > > > though. > > > > > > > > In order to overcome this, we can configure the pixelvalve during > > > > mode_set_no_fb, but only enable it in atomic_enable and flush the FIFO > > > > there, and in atomic_disable disable the pixelvalve again. > > > > > > What displays has this been tested with? Getting this sequencing > > > right is so painful, and things like DSI are tricky to get to light > > > up. > > > > That FIFO is between the HVS and the HDMI PVs, so this was obviously > > tested against that. Dave also tested the DSI output IIRC, so we should > > be covered here. > > DSI wasn't working on the first patch set that Maxime sent - I haven't > tested this one as yet but will do so. > DPI was working early on to both an Adafruit 800x480 DPI panel, and > via a VGA666 as VGA. > HDMI is obviously working. > VEC is being ignored now. The clock structure is more restricted than > earlier chips, so to get the required clocks for the VEC without using > fractional divides it compromises the clock that other parts of the > system can run at (IIRC including the ARM). That's why the VEC has to > be explicitly enabled for the firmware to enable it as the only > output. It's annoying, but that's just a restriction of the chip. I'm more concerned with "make sure we don't regress pre-pi4 with this series" than "pi4 displays all work from the beginning" ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2] drm: document how user-space should use link-status
Describe what a "BAD" link-status means for user-space and how it should
handle it. The logic described has been implemented in igt [1].
v2:
- Change wording to avoid "enabled" (Daniel)
- Add paragraph about multiple connectors sharing the same CRTC (Pekka)
- Add paragraph about performing an atomic commit on a connector without
updating the link-status property (Daniel)
[1]:
https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/commit/fbe61f529737191d0920521946a575bd55f00fbe
Signed-off-by: Simon Ser
Cc: Daniel Vetter
Cc: Manasi Navare
Cc: Pekka Paalanen
---
drivers/gpu/drm/drm_connector.c | 15 +++
1 file changed, 15 insertions(+)
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index f2b20fd66319..829b21124048 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -994,6 +994,21 @@ static const struct drm_prop_enum_list dp_colorspaces[] = {
* after modeset, the kernel driver may set this to "BAD" and issue a
* hotplug uevent. Drivers should update this value using
* drm_connector_set_link_status_property().
+ *
+ * When user-space receives the hotplug uevent and detects a "BAD"
+ * link-status, the sink doesn't receive pixels anymore. The list of
+ * available modes may have changed. User-space is expected to pick a new
+ * mode if the current one has disappeared and perform a new modeset with
+ * link-status set to "GOOD" to re-enable the connector.
+ *
+ * If multiple connectors share the same CRTC and one of them gets a "BAD"
+ * link-status, the other are unaffected (ie. the sinks still continue to
+ * receive pixels).
+ *
+ * When user-space performs an atomic commit on a connector with a "BAD"
+ * link-status without resetting the property to "GOOD", it gets
+ * implicitly reset. This might make the atomic commit fail if the modeset
+ * is unsuccessful.
* non_desktop:
* Indicates the output should be ignored for purposes of displaying a
* standard desktop environment or console. This is most likely because
--
2.26.2
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 6/6] usb: gadget: function: Add Generic USB Display support
Hi,
Den 02.06.2020 19.05, skrev Felipe Balbi:
>
> Hi,
>
> I missed this completely until now.
> Noralf Trønnes writes:
>> This adds the gadget side support for the Generic USB Display. It presents
>> a DRM display device as a USB Display configured through configfs.
>>
>> The display is implemented as a vendor type USB interface with one bulk
>> out endpoint. The protocol is implemented using control requests.
>> lz4 compressed framebuffer data/pixels are sent over the bulk endpoint.
>>
>> The DRM part of the gadget is placed in the DRM subsystem since it reaches
>> into the DRM internals.
>
> First and foremost, could this be done in userspace using the raw gadget
> or f_fs?
>
An uncompressed 1920x1080-RGB565 frame is ~4MB. All frames can be
compressed (lz4) even if just a little, so this is decompressed into the
framebuffer of the attached DRM device. AFAIU I would need to be able to
mmap the received bulk buffer if I were to do this from userspace
without killing performance. So it doesn't look like I can use raw
gadget or f_fs.
>> diff --git a/drivers/usb/gadget/function/f_gud_drm.c
>> b/drivers/usb/gadget/function/f_gud_drm.c
>> new file mode 100644
>> index ..9a2d6bb9739f
>> --- /dev/null
>> +++ b/drivers/usb/gadget/function/f_gud_drm.c
>> @@ -0,0 +1,678 @@
>> +struct f_gud_drm {
>> +struct usb_function func;
>> +struct work_struct worker;
>
> why do you need a worker?
>
The gadget runs in interrupt context and I need to call into the DRM
subsystem which can sleep.
>> +size_t max_buffer_size;
>> +void *ctrl_req_buf;
>> +
>> +u8 interface_id;
>> +struct usb_request *ctrl_req;
>> +
>> +struct usb_ep *bulk_ep;
>> +struct usb_request *bulk_req;
>
> single request? Don't you want to amortize the latency of
> queue->complete by using a series of requests?
>
I use only one request per update or partial update.
I kmalloc the biggest buffer I can get (default 4MB) and tell the host
about this size. If a frame doesn't fit, the host splits it up into
partial updates. I already support partial updates so this is built in.
Userspace can tell the graphics driver which portion of the framebuffer
it has touched to avoid sending the full frame each time.
Having one continous buffer simplifies decompression.
There's a control request preceding the bulk request which tells the
area the update is for and whether it is compressed or not.
I did some testing to see if I should avoid the control request overhead
for split updates, but it turns out that the bottleneck is the
decompression. The control request is just 400-500us, while the total
time from control request to buffer is decompressed is 50-100ms
(depending on how well the frame compresses).
>> +struct gud_drm_gadget *gdg;
>> +
>> +spinlock_t lock; /* Protects the following members: */
>> +bool ctrl_pending;
>> +bool status_pending;
>> +bool bulk_pending;
>> +bool disable_pending;
>
> could this be a single u32 with #define'd flags? That would be atomic,
> right?
>
I have never grasped all the concurrency issues, but wouldn't I need
memory barriers as well?
>> +u8 errno;
>
> a global per-function error? Why?
>
This is the result of the previously request operation. The host will
request this value to see how it went. I might switch to using a bulk
endpoint for status following a discussion with Alan Stern in the host
driver thread in this patch series. If so I might not need this.
>> +u16 request;
>> +u16 value;
>
> also why? Looks odd
>
These values contains the operation (in addition to the payload) that
the worker shall perform following the control-OUT requests.
control-IN requests can run in interrupt context so in that case the
payload is queued up immediately.
>> +static void f_gud_drm_bulk_complete(struct usb_ep *ep, struct usb_request
>> *req)
>> +{
>> +struct f_gud_drm *fgd = req->context;
>> +unsigned long flags;
>> +
>> +if (req->status || req->actual != req->length)
>> +return;
>
> so, if we complete with an erroneous status or a short packet, you'll
> ignore it?
>
Hmm yeah. When I wrote this I thought that the bottleneck was the USB
transfers, so I didn't want the host to slow down performance by
requesting this status. Now I know it's the decompression that takes
time, so I could actually do a status check and retry the frame if the
device detects an error.
>> +spin_lock_irqsave(>lock, flags);
>> +fgd->bulk_pending = true;
>> +spin_unlock_irqrestore(>lock, flags);
>> +
>> +queue_work(system_long_wq, >worker);
>
> Do you need to offset this to a worker?
>
Yes, long running (one operation can be >100ms) and can sleep.
>> +static int f_gud_drm_ctrl_req_set_buffer(struct f_gud_drm *fgd, void *buf,
>> size_t len)
>> +{
>> +int ret;
>> +
>> +if (len != sizeof(struct gud_drm_req_set_buffer))
>> +return -EINVAL;
>> +
>> +ret = gud_drm_gadget_set_buffer(fgd->gdg, buf);
>> +
Re: [PATCH] drm: document how user-space should use link-status
On Tuesday, June 2, 2020 9:38 AM, Pekka Paalanen wrote: > Can it happen that there will be no modes left in > the list? Reading drm_helper_probe_single_connector_modes, this sounds unlikely but possible. This isn't specific to link-status though. This can be the case on a regular hotplug uevent too. > What if userspace is driving two connectors from the same CRTC, and only > one connector gets link-status bad, what does it mean? Is the other > connector still running as normal, as if the failed connector didn't > even exist? > > That is mostly a question about what happens if userspace does not fix > up the link-status=bad connector and does not detach it from the CRTC, > but keeps on flipping or modesetting as if the failure never happened. > I guess I could ask it about both a CRTC that has another connector > still good, and a CRTC where the failed connector was the only one. > > Can I trust that if the other connector is in any way affected, it too > will get link-status bad? link-status is about link maintenance (e.g. DP link training), so I think the other connector would be fine in this case. I'll add this to the next version and let Daniel/Manasi comment if that's incorrect. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [BUG][AMD tahiti xt][amdgpu] broken dpm
On Tue, Jun 2, 2020 at 1:13 PM wrote: > > bisected: commit 4dea25853a6c0c16e373665153bd9eb6edc6319e > > drm/[radeon|amdgpu]: Replace one-element array and use struct_size() helper > ... > Also, make use of the new struct_size() helper to properly calculate the > size of struct SISLANDS_SMC_SWSTATE. I've gone ahead and reverted the patch. Thanks for tracking this down. Alex > > > regards, > > -- > Sylvain ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 4/6] drm: Add Generic USB Display driver
Alan Stern wrote: > > > A gadget driver can STALL in response to a control-OUT data packet, > > > but only before it has seen the packet. > > > > How can it do that for OUT, and IN if possible there too? > > In the way described just above: The gadget driver's SETUP handler tells > the UDC to stall the data packet. > > > Is it related to f->setup() returning < 0 ? > > Yes; the composite core interprets such a value as meaning to STALL > endpoint 0. Thanks a lot for confirming this. > > The spec also allows NAK, but the gadget code seems to not (need to?) > > explicitly support that. Can you comment on this as well? > > If the gadget driver doesn't submit a usb_request then the UDC will > reply with NAK. And thanks for this as well. > > a status request so I can know the result of the operation the device has > > performed. .. > There are two reasonable approaches you could use. One is to have a > vendor-specific control request to get the result of the preceding > operation. .. > The other approach is to send the status data over a bulk-IN endpoint. I've tried to explain a third approach, which I think fits well because the status is only a "Ready" flag - ie. a memory barrier or flow control, to make the host not send data OUT. I'm proposing that the gadget should NAK on data OUT when it isn't Ready, and that the host driver shouldn't query status but simply send data when it has. Per Alans description the NAK happens automatically if the gadget driver has no usb_request pending while it is processing previously received data. On the host that NAK makes the host controller retry automatically until transfer success, timeout or fatal error. > It would have to be formatted in such a way that the host could > recognize it as a status packet and not some other sort of data packet. For host notification of status changes other than Ready I really like such an IN endpoint, but preferably an interrupt endpoint. To avoid the formatting problem each data packet could be one full status message. That way the host would always receive a known data structure. Interrupt packets can be max 64 byte. Noralf, do you think that's enough for everyone in the first version? //Peter ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 015/105] drm/vc4: hvs: Boost the core clock during modeset
On Tue, Jun 2, 2020 at 5:52 AM Maxime Ripard wrote: > > Hi Eric, > > On Wed, May 27, 2020 at 09:33:44AM -0700, Eric Anholt wrote: > > On Wed, May 27, 2020 at 8:49 AM Maxime Ripard wrote: > > > > > > In order to prevent timeouts and stalls in the pipeline, the core clock > > > needs to be maxed at 500MHz during a modeset on the BCM2711. > > > > Like, the whole system's core clock? > > Yep, unfortunately... > > > How is it reasonable for some device driver to crank the system's core > > clock up and back down to some fixed-in-the-driver frequency? Sounds > > like you need some sort of opp thing here. > > That frequency is the minimum rate of that clock. However, since other > devices have similar requirements (unicam in particular) with different > minimum requirements, we will switch to setting a minimum rate instead > of enforcing a particular rate, so that patch would be essentially > s/clk_set_rate/clk_set_min_rate/. clk_set_min_rate makes a lot more sense to me. r-b with that obvious change. Thanks! ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 02/21] drm: mxsfb: Use drm_panel_bridge
Hi Stefan, On Tue, Jun 02, 2020 at 04:34:07PM +0200, Stefan Agner wrote: > On 2020-06-02 15:12, Daniel Vetter wrote: > > On Sat, May 30, 2020 at 05:14:21AM +0300, Laurent Pinchart wrote: > >> On Mon, Mar 23, 2020 at 10:27:21PM +0100, Stefan Agner wrote: > >>> On 2020-03-09 20:51, Laurent Pinchart wrote: > Replace the manual connector implementation based on drm_panel with the > drm_panel_bridge helper. This simplifies the mxsfb driver by removing > connector-related code, and standardizing all pipeline control > operations on bridges. > > A hack is needed to get hold of the connector, as that's our only source > of bus flags and formats for now. As soon as the bridge API provides us > with that information this can be fixed. > >>> > >>> This seems like a nice simplification. > >>> > >>> I gave this a go applied on today's drm-misc-next using a Colibri iMX7 > >>> (imx7d-colibri-emmc-eval-v3.dts device tree). This device makes use of > >>> the simple panel driver. I get this when booting: > >>> > >>> [2.976698] [drm] Supports vblank timestamp caching Rev 2 > >>> (21.10.2013). > >>> [2.983526] [ cut here ] > >>> [2.988180] WARNING: CPU: 0 PID: 1 at > >>> drivers/gpu/drm/bridge/panel.c:267 devm_drm_panel_bridge_add+0x25/0x28 > >>> [2.998059] Modules linked in: > >>> [3.001159] CPU: 0 PID: 1 Comm: swapper/0 Not tainted > >>> 5.6.0-rc5-yocto-standard-01250-ga4ce5db7c9f1 #29 > >>> [3.010493] Hardware name: Freescale i.MX7 Dual (Device Tree) > >>> [3.016281] [] (unwind_backtrace) from [] > >>> (show_stack+0xb/0xc) > >>> [3.023887] [] (show_stack) from [] > >>> (dump_stack+0x63/0x70) > >>> [3.031144] [] (dump_stack) from [] > >>> (__warn+0x9d/0xb0) > >>> [3.038047] [] (__warn) from [] > >>> (warn_slowpath_fmt+0x6b/0x70) > >>> [3.045564] [] (warn_slowpath_fmt) from [] > >>> (devm_drm_panel_bridge_add+0x25/0x28) > >>> [3.054736] [] (devm_drm_panel_bridge_add) from > >>> [] (mxsfb_probe+0x197/0x2e0) > >>> [3.063559] [] (mxsfb_probe) from [] > >>> (platform_drv_probe+0x2d/0x60) > >>> [3.071598] [] (platform_drv_probe) from [] > >>> (really_probe+0x1dd/0x330) > >>> [3.079897] [] (really_probe) from [] > >>> (driver_probe_device+0x4f/0x154) > >>> [3.088195] [] (driver_probe_device) from [] > >>> (device_driver_attach+0x37/0x44) > >>> [3.097101] [] (device_driver_attach) from [] > >>> (__driver_attach+0x7b/0xec) > >>> [3.105660] [] (__driver_attach) from [] > >>> (bus_for_each_dev+0x3d/0x64) > >>> [3.113870] [] (bus_for_each_dev) from [] > >>> (bus_add_driver+0xef/0x160) > >>> [3.122081] [] (bus_add_driver) from [] > >>> (driver_register+0x35/0x9c) > >>> [3.130119] [] (driver_register) from [] > >>> (do_one_initcall+0x3d/0x1e4) > >>> [3.138333] [] (do_one_initcall) from [] > >>> (kernel_init_freeable+0x1b3/0x1fc) > >>> [3.147069] [] (kernel_init_freeable) from [] > >>> (kernel_init+0x7/0xd0) > >>> [3.155194] [] (kernel_init) from [] > >>> (ret_from_fork+0x11/0x38) > >>> [3.162789] Exception stack(0xec0e3fb0 to 0xec0e3ff8) > >>> [3.167862] 3fa0: > >>> > >>> [3.176071] 3fc0: > >>> > >>> [3.184278] 3fe0: 0013 > >>> > >>> [3.191029] ---[ end trace b69e1f44de470959 ]--- > >>> [3.195671] mxsfb 3073.lcdif: Cannot connect bridge: -19 > >>> > >>> Should we maybe use devm_drm_panel_bridge_add_typed? > >> > >> As Sam reported, this is caused by the panel not setting connector_type. > >> We could use devm_drm_panel_bridge_add_typed(), even if I like the > >> warning as it uncovers the problematic panels and gets them fixed. What > >> would be your preference ? > > > > Adding warnings everywhere is kinda uncool, at least my experience is that > > if there's too much you get warning overload and train people to ignore > > them. > > > > Imo either hide the wwarning for now, or if it's not too much work, review > > all the panel drivers and make sure they set the connector type somewhere. > > Warnings are kinda ok once you're pretty sure you got them all, and want > > to make sure newly added drivers get this all right. But not before we've > > reached that. > > I am with Daniel on this, too many warnings make users blind of them, so > we should save them when really attention is needed. > > I guess the only question which connector type we should default to. I > think DRM_MODE_CONNECTOR_DPI make sense for this IP. Yes, that makes sense. I'll fix this in v3, but will wait until you review the remaining patches from v2 before sending v3 out. > >>> Two more comments below. > >>> > Signed-off-by: Laurent Pinchart > --- > drivers/gpu/drm/mxsfb/Makefile| 2 +- > drivers/gpu/drm/mxsfb/mxsfb_drv.c | 105 ++ >
Re: [PATCH 02/21] drm: mxsfb: Use drm_panel_bridge
Hi Daniel, On Tue, Jun 02, 2020 at 03:12:37PM +0200, Daniel Vetter wrote: > On Sat, May 30, 2020 at 05:14:21AM +0300, Laurent Pinchart wrote: > > On Mon, Mar 23, 2020 at 10:27:21PM +0100, Stefan Agner wrote: > > > On 2020-03-09 20:51, Laurent Pinchart wrote: > > > > Replace the manual connector implementation based on drm_panel with the > > > > drm_panel_bridge helper. This simplifies the mxsfb driver by removing > > > > connector-related code, and standardizing all pipeline control > > > > operations on bridges. > > > > > > > > A hack is needed to get hold of the connector, as that's our only source > > > > of bus flags and formats for now. As soon as the bridge API provides us > > > > with that information this can be fixed. > > > > > > This seems like a nice simplification. > > > > > > I gave this a go applied on today's drm-misc-next using a Colibri iMX7 > > > (imx7d-colibri-emmc-eval-v3.dts device tree). This device makes use of > > > the simple panel driver. I get this when booting: > > > > > > [2.976698] [drm] Supports vblank timestamp caching Rev 2 > > > (21.10.2013). > > > [2.983526] [ cut here ] > > > [2.988180] WARNING: CPU: 0 PID: 1 at > > > drivers/gpu/drm/bridge/panel.c:267 devm_drm_panel_bridge_add+0x25/0x28 > > > [2.998059] Modules linked in: > > > [3.001159] CPU: 0 PID: 1 Comm: swapper/0 Not tainted > > > 5.6.0-rc5-yocto-standard-01250-ga4ce5db7c9f1 #29 > > > [3.010493] Hardware name: Freescale i.MX7 Dual (Device Tree) > > > [3.016281] [] (unwind_backtrace) from [] > > > (show_stack+0xb/0xc) > > > [3.023887] [] (show_stack) from [] > > > (dump_stack+0x63/0x70) > > > [3.031144] [] (dump_stack) from [] > > > (__warn+0x9d/0xb0) > > > [3.038047] [] (__warn) from [] > > > (warn_slowpath_fmt+0x6b/0x70) > > > [3.045564] [] (warn_slowpath_fmt) from [] > > > (devm_drm_panel_bridge_add+0x25/0x28) > > > [3.054736] [] (devm_drm_panel_bridge_add) from > > > [] (mxsfb_probe+0x197/0x2e0) > > > [3.063559] [] (mxsfb_probe) from [] > > > (platform_drv_probe+0x2d/0x60) > > > [3.071598] [] (platform_drv_probe) from [] > > > (really_probe+0x1dd/0x330) > > > [3.079897] [] (really_probe) from [] > > > (driver_probe_device+0x4f/0x154) > > > [3.088195] [] (driver_probe_device) from [] > > > (device_driver_attach+0x37/0x44) > > > [3.097101] [] (device_driver_attach) from [] > > > (__driver_attach+0x7b/0xec) > > > [3.105660] [] (__driver_attach) from [] > > > (bus_for_each_dev+0x3d/0x64) > > > [3.113870] [] (bus_for_each_dev) from [] > > > (bus_add_driver+0xef/0x160) > > > [3.122081] [] (bus_add_driver) from [] > > > (driver_register+0x35/0x9c) > > > [3.130119] [] (driver_register) from [] > > > (do_one_initcall+0x3d/0x1e4) > > > [3.138333] [] (do_one_initcall) from [] > > > (kernel_init_freeable+0x1b3/0x1fc) > > > [3.147069] [] (kernel_init_freeable) from [] > > > (kernel_init+0x7/0xd0) > > > [3.155194] [] (kernel_init) from [] > > > (ret_from_fork+0x11/0x38) > > > [3.162789] Exception stack(0xec0e3fb0 to 0xec0e3ff8) > > > [3.167862] 3fa0: > > > > > > [3.176071] 3fc0: > > > > > > [3.184278] 3fe0: 0013 > > > > > > [3.191029] ---[ end trace b69e1f44de470959 ]--- > > > [3.195671] mxsfb 3073.lcdif: Cannot connect bridge: -19 > > > > > > Should we maybe use devm_drm_panel_bridge_add_typed? > > > > As Sam reported, this is caused by the panel not setting connector_type. > > We could use devm_drm_panel_bridge_add_typed(), even if I like the > > warning as it uncovers the problematic panels and gets them fixed. What > > would be your preference ? > > Adding warnings everywhere is kinda uncool, at least my experience is that > if there's too much you get warning overload and train people to ignore > them. > > Imo either hide the wwarning for now, or if it's not too much work, review > all the panel drivers and make sure they set the connector type somewhere. All panel drivers but panel-simple set the connector type. For panel-simple, the type is stored in the panel_desc panel description, and out of the 123 supported panels, only 48 have a connector type. I've just sent a patch to fix this for the 7 DSI panels, so only 68 panels to go :-) This will require trying to find the corresponding datasheets, so that's a large effort for a single developer. That's why I was hoping a WARN_ON() could help distributing the work :-) I hear your concern though, and I'll set a default in this driver for the time being. > Warnings are kinda ok once you're pretty sure you got them all, and want > to make sure newly added drivers get this all right. But not before we've > reached that. > > > > Two more comments below. > > > > > > > Signed-off-by: Laurent Pinchart > > > > --- > > > >
[PATCH] drm/panel: simple: Set connector type for DSI panels
None of the DSI panels set the connector_type in their panel_desc
descriptor. As they are all guaranteed to be DSI panels, that's an easy
fix, set the connector type to DRM_MODE_CONNECTOR_DSI.
Signed-off-by: Laurent Pinchart
---
drivers/gpu/drm/panel/panel-simple.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/drivers/gpu/drm/panel/panel-simple.c
b/drivers/gpu/drm/panel/panel-simple.c
index b6ecd1552132..b86b52bfece7 100644
--- a/drivers/gpu/drm/panel/panel-simple.c
+++ b/drivers/gpu/drm/panel/panel-simple.c
@@ -4082,6 +4082,7 @@ static const struct panel_desc_dsi auo_b080uan01 = {
.width = 108,
.height = 272,
},
+ .connector_type = DRM_MODE_CONNECTOR_DSI,
},
.flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_CLOCK_NON_CONTINUOUS,
.format = MIPI_DSI_FMT_RGB888,
@@ -4110,6 +4111,7 @@ static const struct panel_desc_dsi boe_tv080wum_nl0 = {
.width = 107,
.height = 172,
},
+ .connector_type = DRM_MODE_CONNECTOR_DSI,
},
.flags = MIPI_DSI_MODE_VIDEO |
MIPI_DSI_MODE_VIDEO_BURST |
@@ -4140,6 +4142,7 @@ static const struct panel_desc_dsi lg_ld070wx3_sl01 = {
.width = 94,
.height = 151,
},
+ .connector_type = DRM_MODE_CONNECTOR_DSI,
},
.flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_CLOCK_NON_CONTINUOUS,
.format = MIPI_DSI_FMT_RGB888,
@@ -4168,6 +4171,7 @@ static const struct panel_desc_dsi lg_lh500wx1_sd03 = {
.width = 62,
.height = 110,
},
+ .connector_type = DRM_MODE_CONNECTOR_DSI,
},
.flags = MIPI_DSI_MODE_VIDEO,
.format = MIPI_DSI_FMT_RGB888,
@@ -4196,6 +4200,7 @@ static const struct panel_desc_dsi panasonic_vvx10f004b00
= {
.width = 217,
.height = 136,
},
+ .connector_type = DRM_MODE_CONNECTOR_DSI,
},
.flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_SYNC_PULSE |
MIPI_DSI_CLOCK_NON_CONTINUOUS,
@@ -4225,6 +4230,7 @@ static const struct panel_desc_dsi lg_acx467akm_7 = {
.width = 62,
.height = 110,
},
+ .connector_type = DRM_MODE_CONNECTOR_DSI,
},
.flags = 0,
.format = MIPI_DSI_FMT_RGB888,
@@ -4254,6 +4260,7 @@ static const struct panel_desc_dsi osd101t2045_53ts = {
.width = 217,
.height = 136,
},
+ .connector_type = DRM_MODE_CONNECTOR_DSI,
},
.flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_BURST |
MIPI_DSI_MODE_VIDEO_SYNC_PULSE |
--
Regards,
Laurent Pinchart
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [BUG][AMD tahiti xt][amdgpu] broken dpm
bisected: commit 4dea25853a6c0c16e373665153bd9eb6edc6319e drm/[radeon|amdgpu]: Replace one-element array and use struct_size() helper ... Also, make use of the new struct_size() helper to properly calculate the size of struct SISLANDS_SMC_SWSTATE. regards, -- Sylvain ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 6/6] usb: gadget: function: Add Generic USB Display support
Hi,
I missed this completely until now.
Noralf Trønnes writes:
> This adds the gadget side support for the Generic USB Display. It presents
> a DRM display device as a USB Display configured through configfs.
>
> The display is implemented as a vendor type USB interface with one bulk
> out endpoint. The protocol is implemented using control requests.
> lz4 compressed framebuffer data/pixels are sent over the bulk endpoint.
>
> The DRM part of the gadget is placed in the DRM subsystem since it reaches
> into the DRM internals.
First and foremost, could this be done in userspace using the raw gadget
or f_fs?
> diff --git a/drivers/usb/gadget/function/f_gud_drm.c
> b/drivers/usb/gadget/function/f_gud_drm.c
> new file mode 100644
> index ..9a2d6bb9739f
> --- /dev/null
> +++ b/drivers/usb/gadget/function/f_gud_drm.c
> @@ -0,0 +1,678 @@
> +struct f_gud_drm {
> + struct usb_function func;
> + struct work_struct worker;
why do you need a worker?
> + size_t max_buffer_size;
> + void *ctrl_req_buf;
> +
> + u8 interface_id;
> + struct usb_request *ctrl_req;
> +
> + struct usb_ep *bulk_ep;
> + struct usb_request *bulk_req;
single request? Don't you want to amortize the latency of
queue->complete by using a series of requests?
> + struct gud_drm_gadget *gdg;
> +
> + spinlock_t lock; /* Protects the following members: */
> + bool ctrl_pending;
> + bool status_pending;
> + bool bulk_pending;
> + bool disable_pending;
could this be a single u32 with #define'd flags? That would be atomic,
right?
> + u8 errno;
a global per-function error? Why?
> + u16 request;
> + u16 value;
also why? Looks odd
> +};
> +
> +static inline struct f_gud_drm *func_to_f_gud_drm(struct usb_function *f)
let the compiler inline for you
> +{
> + return container_of(f, struct f_gud_drm, func);
could be a macro, but okay.
> +static inline struct f_gud_drm_opts *fi_to_f_gud_drm_opts(const struct
> usb_function_instance *fi)
ditto
> +{
> + return container_of(fi, struct f_gud_drm_opts, func_inst);
ditto
> +}
> +
> +static inline struct f_gud_drm_opts *ci_to_f_gud_drm_opts(struct config_item
> *item)
ditto
> +{
> + return container_of(to_config_group(item), struct f_gud_drm_opts,
> + func_inst.group);
ditto
> +}
> +
> +#define F_MUD_DEFINE_BULK_ENDPOINT_DESCRIPTOR(name, addr, size) \
> + static struct usb_endpoint_descriptor name = { \
const? Also, please check all the others
> +static void f_gud_drm_bulk_complete(struct usb_ep *ep, struct usb_request
> *req)
> +{
> + struct f_gud_drm *fgd = req->context;
> + unsigned long flags;
> +
> + if (req->status || req->actual != req->length)
> + return;
so, if we complete with an erroneous status or a short packet, you'll
ignore it?
> + spin_lock_irqsave(>lock, flags);
> + fgd->bulk_pending = true;
> + spin_unlock_irqrestore(>lock, flags);
> +
> + queue_work(system_long_wq, >worker);
Do you need to offset this to a worker?
> +static int f_gud_drm_ctrl_req_set_buffer(struct f_gud_drm *fgd, void *buf,
> size_t len)
> +{
> + int ret;
> +
> + if (len != sizeof(struct gud_drm_req_set_buffer))
> + return -EINVAL;
> +
> + ret = gud_drm_gadget_set_buffer(fgd->gdg, buf);
> + if (ret < 0)
> + return ret;
> +
> + if (ret > fgd->max_buffer_size)
> + return -EOVERFLOW;
> +
> + fgd->bulk_req->length = ret;
> +
> + return usb_ep_queue(fgd->bulk_ep, fgd->bulk_req, GFP_KERNEL);
> +}
> +
> +static void f_gud_drm_worker(struct work_struct *work)
> +{
> + struct f_gud_drm *fgd = container_of(work, struct f_gud_drm, worker);
> + bool ctrl_pending, bulk_pending, disable_pending;
> + struct gud_drm_gadget *gdg = fgd->gdg;
> + unsigned long flags;
> + u16 request, value;
> + int ret;
> +
> + spin_lock_irqsave(>lock, flags);
> + request = fgd->request;
> + value = fgd->value;
> + ctrl_pending = fgd->ctrl_pending;
> + bulk_pending = fgd->bulk_pending;
> + disable_pending = fgd->disable_pending;
> + spin_unlock_irqrestore(>lock, flags);
> +
> + pr_debug("%s: bulk_pending=%u ctrl_pending=%u disable_pending=%u\n",
> + __func__, bulk_pending, ctrl_pending, disable_pending);
could we use dev_dbg() at least?
--
balbi
signature.asc
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [v2] drm/msm: add shutdown support for display platform_driver
On Tue, 2 Jun 2020 at 15:49, Sai Prakash Ranjan wrote: > > Hi Emil, > > On 2020-06-02 19:43, Emil Velikov wrote: > > Hi Krishna, > > > > On Tue, 2 Jun 2020 at 08:17, Krishna Manikandan > > wrote: > >> > >> Define shutdown callback for display drm driver, > >> so as to disable all the CRTCS when shutdown > >> notification is received by the driver. > >> > >> This change will turn off the timing engine so > >> that no display transactions are requested > >> while mmu translations are getting disabled > >> during reboot sequence. > >> > >> Signed-off-by: Krishna Manikandan > >> > > AFAICT atomics is setup in msm_drm_ops::bind and shutdown in > > msm_drm_ops::unbind. > > > > Are you saying that unbind never triggers? If so, then we should > > really fix that instead, since this patch seems more like a > > workaround. > > > > Which path do you suppose that the unbind should be called from, remove > callback? Here we are talking about the drivers which are builtin, where > remove callbacks are not called from the driver core during > reboot/shutdown, > instead shutdown callbacks are called which needs to be defined in order > to > trigger unbind. So AFAICS there is nothing to be fixed. > Interesting - in drm effectively only drm panels implement .shutdown. So my naive assumption was that .remove was used implicitly by core, as part of the shutdown process. Yet that's not the case, so it seems that many drivers could use some fixes. Then again, that's an existing problem which is irrelevant for msm. -Emil ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/nouveau/clk/gm20b: Fix memory leak in gm20b_clk_new()
On Tue, Jun 02, 2020 at 01:10:34PM +0200, Markus Elfring wrote: > > The original patch was basically fine. > > I propose to reconsider the interpretation of the software situation once > more. > > * Should the allocated clock object be kept usable even after > a successful return from this function? Heh. You're right. The patch is freeing "clk" on the success path so that doesn't work. regards, dan carpenter ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 4/6] drm: Add Generic USB Display driver
On Tue, Jun 02, 2020 at 05:21:50AM +, Peter Stuge wrote: > > The USB protocol forbids a device from sending a STALL response to a > > SETUP packet. The only valid response is ACK. Thus, there is no way > > to prevent the host from sending its DATA packet for a control-OUT > > transfer. > > Right; a STALL handshake only after a DATA packet, but a udc could silently > drop the first DATA packet if instructed to STALL during SETUP processing. > I don't know how common that is for the udc:s supported by gadget, but some > MCU:s behave like that. It happens from time to time, such as when the host sends a SETUP packet that the gadget driver doesn't understand. > > A gadget driver can STALL in response to a control-OUT data packet, > > but only before it has seen the packet. > > How can it do that for OUT, and IN if possible there too? In the way described just above: The gadget driver's SETUP handler tells the UDC to stall the data packet. > Is it related to f->setup() returning < 0 ? Yes; the composite core interprets such a value as meaning to STALL endpoint 0. > The spec also allows NAK, but the gadget code seems to not (need to?) > explicitly support that. Can you comment on this as well? If the gadget driver doesn't submit a usb_request then the UDC will reply with NAK. > > Once the driver knows what the data packet contains, the gadget API > > doesn't provide any way to STALL the status stage. > > Thanks. I think this particular gadget driver doesn't need to decide late. > > Ideally the control transfers can even be avoided. On Tue, Jun 02, 2020 at 01:46:38PM +0200, Noralf Trønnes wrote: > > A gadget driver can STALL in response to a control-OUT data packet, > > but only before it has seen the packet. Once the driver knows what > > the data packet contains, the gadget API doesn't provide any way to > > STALL the status stage. There has been a proposal to change the API > > to make this possible, but so far it hasn't gone forward. > > > > This confirms what I have seen in the kernel and the reason I added a > status request so I can know the result of the operation the device has > performed. Does this status request use ep0 or some other endpoint? > I have a problem that I've encountered with this status request. > In my first version the gadget would usb_ep_queue() the status value > when the operation was done and as long as this happended within the > host timeout (5s) everything worked fine. > > Then I hit a 10s timeout in the gadget when performing a display modeset > operation (wait on missing vblank). The result of this was that the host > timed out and moved on. The gadget however didn't know that the host > gave up, so it queued up the status value. The result of this was that > all further requests from the host would time out. > Do you know a solution to this? > My work around is to just poll on the status request, which returns a > value immediately, until there's a result. The udc driver I use is dwc2. It's hard to give a precise answer without knowing the details of how your driver works. There are two reasonable approaches you could use. One is to have a vendor-specific control request to get the result of the preceding operation. This works but it has high overhead -- which may not matter if it happens infrequently and isn't sensitive to latency. The other approach is to send the status data over a bulk-IN endpoint. It would have to be formatted in such a way that the host could recognize it as a status packet and not some other sort of data packet. That way, if the host received a stale status value, it could ignore it and move on. You also may want to give some thought to a "resynchronization" protocol, for use in situations where the host times out waiting for a response from the device while the device is waiting for something else (the host, a vblank, or whatever). This could be a special control request, or you could rely on the host doing a complete USB reset. Alan Stern ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 032/105] drm/vc4: crtc: Enable and disable the PV in atomic_enable / disable
Hi Maxime and Eric On Tue, 2 Jun 2020 at 15:12, Maxime Ripard wrote: > > Hi Eric > > On Wed, May 27, 2020 at 09:54:44AM -0700, Eric Anholt wrote: > > On Wed, May 27, 2020 at 8:50 AM Maxime Ripard wrote: > > > > > > The VIDEN bit in the pixelvalve currently being used to enable or disable > > > the pixelvalve seems to not be enough in some situations, which whill end > > > up with the pixelvalve stalling. > > > > > > In such a case, even re-enabling VIDEN doesn't bring it back and we need > > > to > > > clear the FIFO. This can only be done if the pixelvalve is disabled > > > though. > > > > > > In order to overcome this, we can configure the pixelvalve during > > > mode_set_no_fb, but only enable it in atomic_enable and flush the FIFO > > > there, and in atomic_disable disable the pixelvalve again. > > > > What displays has this been tested with? Getting this sequencing > > right is so painful, and things like DSI are tricky to get to light > > up. > > That FIFO is between the HVS and the HDMI PVs, so this was obviously > tested against that. Dave also tested the DSI output IIRC, so we should > be covered here. DSI wasn't working on the first patch set that Maxime sent - I haven't tested this one as yet but will do so. DPI was working early on to both an Adafruit 800x480 DPI panel, and via a VGA666 as VGA. HDMI is obviously working. VEC is being ignored now. The clock structure is more restricted than earlier chips, so to get the required clocks for the VEC without using fractional divides it compromises the clock that other parts of the system can run at (IIRC including the ARM). That's why the VEC has to be explicitly enabled for the firmware to enable it as the only output. It's annoying, but that's just a restriction of the chip. Dave ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v1 2/6] drm: panel-simple: add Seiko 70WVW2T 7" simple panel
On Tue, 2 Jun 2020 at 01:31, Doug Anderson wrote:
>
> Hi,
>
> On Mon, Jun 1, 2020 at 1:33 AM Sam Ravnborg wrote:
> >
> > The Seiko 70WVW2T is a discontinued product, but may be used somewhere.
> > Tested on a proprietary product.
> >
> > Signed-off-by: Sam Ravnborg
> > Cc: Søren Andersen
> > Cc: Thierry Reding
> > Cc: Sam Ravnborg
> > ---
> > drivers/gpu/drm/panel/panel-simple.c | 28
> > 1 file changed, 28 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/panel/panel-simple.c
> > b/drivers/gpu/drm/panel/panel-simple.c
> > index b067f66cea0e..8624bb80108c 100644
> > --- a/drivers/gpu/drm/panel/panel-simple.c
> > +++ b/drivers/gpu/drm/panel/panel-simple.c
> > @@ -3194,6 +3194,31 @@ static const struct panel_desc
> > shelly_sca07010_bfn_lnn = {
> > .bus_format = MEDIA_BUS_FMT_RGB666_1X18,
> > };
> >
> > +static const struct drm_display_mode sii_70wvw2t_mode = {
> > + .clock = 33000,
> > + .hdisplay = 800,
> > + .hsync_start = 800 + 256,
> > + .hsync_end = 800 + 256 + 0,
> > + .htotal = 800 + 256 + 0 + 0,
> > + .vdisplay = 480,
> > + .vsync_start = 480 + 0,
> > + .vsync_end = 480 + 0 + 0,
> > + .vtotal = 480 + 0 + 0 + 45,
>
> Important to have a "vrefresh"?
>
Ville posted a series (most of which already landed) getting removing
vrefresh all together. The overall idea is to compute it, in the rare
case it's needed.
>
> > + .flags = DRM_MODE_FLAG_NVSYNC | DRM_MODE_FLAG_NHSYNC,
> > +};
> > +
> > +static const struct panel_desc sii_70wvw2t = {
> > + .modes = _70wvw2t_mode,
> > + .num_modes = 1,
>
> Do we want "bpc = 6"?
>
The largest user of bpc is userspace - the data gets copied via the ioctls.
A secondary, and quite limited, user are drivers exposing the "max
bpc" connector property. From a quick look: amdgpu, the synopsys
dw-hdmi bridge and i915 do so. In case the data missing, atomics
assume a max 8 bpc.
>
> > + .size = {
> > + .width = 152,
> > + .height = 91,
> > + },
> > + .bus_format = MEDIA_BUS_FMT_RGB888_1X24,
>
> Should this be a 666 format? Random internet-found data sheet says
> 262K colors...
Good catch. Would be nice to have a spec sheet link (even if random)
in the commit message.
HTH
-Emil
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 07/15] drm/amdgpu: use PCI_IRQ_MSI_TYPES where appropriate
On Tue, Jun 2, 2020 at 10:35 AM Andy Shevchenko
wrote:
>
> On Tue, Jun 2, 2020 at 5:21 PM Alex Deucher wrote:
> > On Tue, Jun 2, 2020 at 10:00 AM Andy Shevchenko
> > wrote:
> > > On Tue, Jun 2, 2020 at 4:38 PM Ruhl, Michael J
> > > wrote:
> > > > >From: dri-devel On Behalf Of
> > > > >Piotr Stankiewicz
>
> > > > > int nvec = pci_msix_vec_count(adev->pdev);
> > > > > unsigned int flags;
> > > > >
> > > > >- if (nvec <= 0) {
> > > > >+ if (nvec > 0)
> > > > >+ flags = PCI_IRQ_MSI_TYPES;
> > > > >+ else
> > > > > flags = PCI_IRQ_MSI;
> > > > >- } else {
> > > > >- flags = PCI_IRQ_MSI | PCI_IRQ_MSIX;
> > > > >- }
> > > >
> > > > Minor nit:
> > > >
> > > > Is it really necessary to set do this check? Can flags just
> > > > be set?
> > > >
> > > > I.e.:
> > > > flags = PCI_IRQ_MSI_TYPES;
> > > >
> > > > pci_alloc_irq_vector() tries stuff in order. If MSIX is not available,
> > > > it will try MSI.
> > >
> > > That's also what I proposed earlier. But I suggested as well to wait
> > > for AMD people to confirm that neither pci_msix_vec_count() nor flags
> > > is needed and we can directly supply MSI_TYPES to the below call.
> > >
> >
> > I think it was leftover from debugging and just to be careful. We had
> > some issues when we originally enabled MSI-X on certain boards. The
> > fix was to just allocate a single vector (since that is all we use
> > anyway) and we were using the wrong irq (pdev->irq vs
> > pci_irq_vector(pdev, 0)).
>
> Do you agree that simple
>
> nvec = pci_alloc_irq_vectors(adev->pdev, 1, 1, PCI_IRQ_MSI_TYPES);
>
> will work and we can remove that leftover?
Yes, I believe so. Tom, can you give this a quick spin on raven just
in case if you get a chance? Something like this:
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
index 0cc4c67f95f7..c59111b57cc2 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
@@ -248,16 +248,10 @@ int amdgpu_irq_init(struct amdgpu_device *adev)
adev->irq.msi_enabled = false;
if (amdgpu_msi_ok(adev)) {
- int nvec = pci_msix_vec_count(adev->pdev);
- unsigned int flags;
+ int nvec;
- if (nvec <= 0) {
- flags = PCI_IRQ_MSI;
- } else {
- flags = PCI_IRQ_MSI | PCI_IRQ_MSIX;
- }
/* we only need one vector */
- nvec = pci_alloc_irq_vectors(adev->pdev, 1, 1, flags);
+ nvec = pci_alloc_irq_vectors(adev->pdev, 1, 1,
PCI_IRQ_MSI | PCI_IRQ_MSIX);
if (nvec > 0) {
adev->irq.msi_enabled = true;
dev_dbg(adev->dev, "using MSI/MSI-X.\n");
Thanks,
Alex
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 07/15] drm/amdgpu: use PCI_IRQ_MSI_TYPES where appropriate
On Tue, Jun 2, 2020 at 5:21 PM Alex Deucher wrote:
> On Tue, Jun 2, 2020 at 10:00 AM Andy Shevchenko
> wrote:
> > On Tue, Jun 2, 2020 at 4:38 PM Ruhl, Michael J
> > wrote:
> > > >From: dri-devel On Behalf Of
> > > >Piotr Stankiewicz
> > > > int nvec = pci_msix_vec_count(adev->pdev);
> > > > unsigned int flags;
> > > >
> > > >- if (nvec <= 0) {
> > > >+ if (nvec > 0)
> > > >+ flags = PCI_IRQ_MSI_TYPES;
> > > >+ else
> > > > flags = PCI_IRQ_MSI;
> > > >- } else {
> > > >- flags = PCI_IRQ_MSI | PCI_IRQ_MSIX;
> > > >- }
> > >
> > > Minor nit:
> > >
> > > Is it really necessary to set do this check? Can flags just
> > > be set?
> > >
> > > I.e.:
> > > flags = PCI_IRQ_MSI_TYPES;
> > >
> > > pci_alloc_irq_vector() tries stuff in order. If MSIX is not available,
> > > it will try MSI.
> >
> > That's also what I proposed earlier. But I suggested as well to wait
> > for AMD people to confirm that neither pci_msix_vec_count() nor flags
> > is needed and we can directly supply MSI_TYPES to the below call.
> >
>
> I think it was leftover from debugging and just to be careful. We had
> some issues when we originally enabled MSI-X on certain boards. The
> fix was to just allocate a single vector (since that is all we use
> anyway) and we were using the wrong irq (pdev->irq vs
> pci_irq_vector(pdev, 0)).
Do you agree that simple
nvec = pci_alloc_irq_vectors(adev->pdev, 1, 1, PCI_IRQ_MSI_TYPES);
will work and we can remove that leftover?
--
With Best Regards,
Andy Shevchenko
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 02/21] drm: mxsfb: Use drm_panel_bridge
On 2020-06-02 15:12, Daniel Vetter wrote: > On Sat, May 30, 2020 at 05:14:21AM +0300, Laurent Pinchart wrote: >> Hi Stefan, >> >> On Mon, Mar 23, 2020 at 10:27:21PM +0100, Stefan Agner wrote: >> > On 2020-03-09 20:51, Laurent Pinchart wrote: >> > > Replace the manual connector implementation based on drm_panel with the >> > > drm_panel_bridge helper. This simplifies the mxsfb driver by removing >> > > connector-related code, and standardizing all pipeline control >> > > operations on bridges. >> > > >> > > A hack is needed to get hold of the connector, as that's our only source >> > > of bus flags and formats for now. As soon as the bridge API provides us >> > > with that information this can be fixed. >> > >> > This seems like a nice simplification. >> > >> > I gave this a go applied on today's drm-misc-next using a Colibri iMX7 >> > (imx7d-colibri-emmc-eval-v3.dts device tree). This device makes use of >> > the simple panel driver. I get this when booting: >> > >> > [2.976698] [drm] Supports vblank timestamp caching Rev 2 >> > (21.10.2013). >> > [2.983526] [ cut here ] >> > [2.988180] WARNING: CPU: 0 PID: 1 at >> > drivers/gpu/drm/bridge/panel.c:267 devm_drm_panel_bridge_add+0x25/0x28 >> > [2.998059] Modules linked in: >> > [3.001159] CPU: 0 PID: 1 Comm: swapper/0 Not tainted >> > 5.6.0-rc5-yocto-standard-01250-ga4ce5db7c9f1 #29 >> > [3.010493] Hardware name: Freescale i.MX7 Dual (Device Tree) >> > [3.016281] [] (unwind_backtrace) from [] >> > (show_stack+0xb/0xc) >> > [3.023887] [] (show_stack) from [] >> > (dump_stack+0x63/0x70) >> > [3.031144] [] (dump_stack) from [] >> > (__warn+0x9d/0xb0) >> > [3.038047] [] (__warn) from [] >> > (warn_slowpath_fmt+0x6b/0x70) >> > [3.045564] [] (warn_slowpath_fmt) from [] >> > (devm_drm_panel_bridge_add+0x25/0x28) >> > [3.054736] [] (devm_drm_panel_bridge_add) from >> > [] (mxsfb_probe+0x197/0x2e0) >> > [3.063559] [] (mxsfb_probe) from [] >> > (platform_drv_probe+0x2d/0x60) >> > [3.071598] [] (platform_drv_probe) from [] >> > (really_probe+0x1dd/0x330) >> > [3.079897] [] (really_probe) from [] >> > (driver_probe_device+0x4f/0x154) >> > [3.088195] [] (driver_probe_device) from [] >> > (device_driver_attach+0x37/0x44) >> > [3.097101] [] (device_driver_attach) from [] >> > (__driver_attach+0x7b/0xec) >> > [3.105660] [] (__driver_attach) from [] >> > (bus_for_each_dev+0x3d/0x64) >> > [3.113870] [] (bus_for_each_dev) from [] >> > (bus_add_driver+0xef/0x160) >> > [3.122081] [] (bus_add_driver) from [] >> > (driver_register+0x35/0x9c) >> > [3.130119] [] (driver_register) from [] >> > (do_one_initcall+0x3d/0x1e4) >> > [3.138333] [] (do_one_initcall) from [] >> > (kernel_init_freeable+0x1b3/0x1fc) >> > [3.147069] [] (kernel_init_freeable) from [] >> > (kernel_init+0x7/0xd0) >> > [3.155194] [] (kernel_init) from [] >> > (ret_from_fork+0x11/0x38) >> > [3.162789] Exception stack(0xec0e3fb0 to 0xec0e3ff8) >> > [3.167862] 3fa0: >> > >> > [3.176071] 3fc0: >> > >> > [3.184278] 3fe0: 0013 >> > >> > [3.191029] ---[ end trace b69e1f44de470959 ]--- >> > [3.195671] mxsfb 3073.lcdif: Cannot connect bridge: -19 >> > >> > Should we maybe use devm_drm_panel_bridge_add_typed? >> >> As Sam reported, this is caused by the panel not setting connector_type. >> We could use devm_drm_panel_bridge_add_typed(), even if I like the >> warning as it uncovers the problematic panels and gets them fixed. What >> would be your preference ? > > Adding warnings everywhere is kinda uncool, at least my experience is that > if there's too much you get warning overload and train people to ignore > them. > > Imo either hide the wwarning for now, or if it's not too much work, review > all the panel drivers and make sure they set the connector type somewhere. > Warnings are kinda ok once you're pretty sure you got them all, and want > to make sure newly added drivers get this all right. But not before we've > reached that. I am with Daniel on this, too many warnings make users blind of them, so we should save them when really attention is needed. I guess the only question which connector type we should default to. I think DRM_MODE_CONNECTOR_DPI make sense for this IP. -- Stefan > > Cheers, Daniel > >> >> > Two more comments below. >> > >> > > Signed-off-by: Laurent Pinchart >> > > --- >> > > drivers/gpu/drm/mxsfb/Makefile| 2 +- >> > > drivers/gpu/drm/mxsfb/mxsfb_drv.c | 105 ++ >> > > drivers/gpu/drm/mxsfb/mxsfb_drv.h | 5 +- >> > > drivers/gpu/drm/mxsfb/mxsfb_out.c | 99 >> > > 4 files changed, 53 insertions(+), 158 deletions(-) >> > > delete mode 100644 drivers/gpu/drm/mxsfb/mxsfb_out.c >> > > >> > >
Re: [RFC PATCH 1/1] drm/mm: add ig_frag selftest
Am 02.06.20 um 16:13 schrieb Nirmoy:
Hi Christian,
On 6/2/20 2:47 PM, Christian König wrote:
Nirmoy please keep in mind that your current implementation doesn't
fully solve the issue the test case is exercising.
In other words what you have implement is fast skipping of fragmented
address space for bottom-up and top-down.
But what this test here exercises is the fast skipping of aligned
allocations. You should probably adjust the test case a bit.
Allocations with size=4k and aign = 8k is known to introduce
fragmentation,
Yes, but this fragmentation can't be avoided with what we already
implemented. For this we would need the extension with the alignment I
already explained.
do you mean I should only test bottom-up and top-down
for now ?
Yes and no.
What we need to test is the following:
1. Make tons of allocations with size=4k and align=0.
2. Free every other of those allocations.
3. Make tons of allocations with size=8k and align=0.
Previously bottom-up and top-down would have checked all the holes
created in step #2.
With your change they can immediately see that this doesn't make sense
and shortcut to the leftmost/rightmost leaf node in the tree with the
large free block.
That we can handle the alignment as well is the next step of that.
Regards,
Christian.
Regards,
Nirmoy
Regards,
Christian.
Am 29.05.20 um 23:01 schrieb Nirmoy:
On 5/29/20 5:52 PM, Chris Wilson wrote:
Quoting Nirmoy (2020-05-29 16:40:53)
This works correctly most of the times but sometimes
I have to take my word back. In another machine, 20k insertions in
best mode takes 6-9 times more than 10k insertions, all most all the
time.
evict, bottom-up and top-down modes remains in 2-5 times range.
If I reduce the insertions to 1k and 2k then scaling factor for best
mode stays below 4 most of the time.
evict, bottom-up and top-down modes remains in 2-3 times range.
I wonder if it makes sense to test with only 1k and 2k insertions
and tolerate more than error if the mode == best.
Regards,
Nirmoy
20k insertions can take more than 8 times of 10k insertion time.
The pressure is on to improve then :)
Regards,
Nirmoy
On 5/29/20 6:33 PM, Nirmoy Das wrote:
This patch introduces fragmentation in the address range
and measures time taken by 10k and 20k insertions. ig_frag()
will fail if time taken by 20k insertions takes more than 4 times
of 10k insertions as we know that insertions scale quadratically.
Also tolerate 10% error because of kernel scheduler's jitters.
Output:
[ 8092.653518] drm_mm: Testing DRM range manger (struct drm_mm),
with random_seed=0x9bfb4117 max_iterations=8192 max_prime=128
[ 8092.653520] drm_mm: igt_sanitycheck - ok!
[ 8092.653525] igt_debug 0x-0x0200:
512: free
[ 8092.653526] igt_debug 0x0200-0x0600:
1024: used
[ 8092.653527] igt_debug 0x0600-0x0a00:
1024: free
[ 8092.653528] igt_debug 0x0a00-0x0e00:
1024: used
[ 8092.653529] igt_debug 0x0e00-0x1000:
512: free
[ 8092.653529] igt_debug total: 4096, used 2048 free 2048
[ 8112.569813] drm_mm: best fragmented insert of 1 and 2
insertions took 504 and 1996 msecs
[ 8112.723254] drm_mm: bottom-up fragmented insert of 1 and
2 insertions took 44 and 108 msecs
[ 8112.813212] drm_mm: top-down fragmented insert of 1 and
2 insertions took 40 and 44 msecs
[ 8112.847733] drm_mm: evict fragmented insert of 1 and 2
insertions took 8 and 20 msecs
Signed-off-by: Nirmoy Das
---
drivers/gpu/drm/selftests/drm_mm_selftests.h | 1 +
drivers/gpu/drm/selftests/test-drm_mm.c | 73
2 files changed, 74 insertions(+)
diff --git a/drivers/gpu/drm/selftests/drm_mm_selftests.h
b/drivers/gpu/drm/selftests/drm_mm_selftests.h
index 6b943ea1c57d..8c87c964176b 100644
--- a/drivers/gpu/drm/selftests/drm_mm_selftests.h
+++ b/drivers/gpu/drm/selftests/drm_mm_selftests.h
@@ -14,6 +14,7 @@ selftest(insert, igt_insert)
selftest(replace, igt_replace)
selftest(insert_range, igt_insert_range)
selftest(align, igt_align)
+selftest(frag, igt_frag)
selftest(align32, igt_align32)
selftest(align64, igt_align64)
selftest(evict, igt_evict)
diff --git a/drivers/gpu/drm/selftests/test-drm_mm.c
b/drivers/gpu/drm/selftests/test-drm_mm.c
index 9aabe82dcd3a..05d8f3659b4d 100644
--- a/drivers/gpu/drm/selftests/test-drm_mm.c
+++ b/drivers/gpu/drm/selftests/test-drm_mm.c
@@ -1033,6 +1033,79 @@ static int igt_insert_range(void *ignored)
return 0;
}
+static int get_insert_time(unsigned int num_insert,
+ const struct insert_mode *mode)
+{
+ struct drm_mm mm;
+ struct drm_mm_node *nodes, *node, *next;
+ unsigned int size = 4096, align = 8192;
+ unsigned long start;
+ unsigned int i;
+ int ret = -EINVAL;
+
+ drm_mm_init(, 1, U64_MAX - 2);
+ nodes =
Re: [PATCH 07/15] drm/amdgpu: use PCI_IRQ_MSI_TYPES where appropriate
On Tue, Jun 2, 2020 at 10:00 AM Andy Shevchenko
wrote:
>
> On Tue, Jun 2, 2020 at 4:38 PM Ruhl, Michael J
> wrote:
> > >-Original Message-
> > >From: dri-devel On Behalf Of
> > >Piotr Stankiewicz
> > >Sent: Tuesday, June 2, 2020 5:21 AM
> > >To: Alex Deucher ; Christian König
> > >; David Zhou ; David
> > >Airlie ; Daniel Vetter
> > >Cc: Stankiewicz, Piotr ; dri-
> > >de...@lists.freedesktop.org; amd-...@lists.freedesktop.org; linux-
> > >ker...@vger.kernel.org
> > >Subject: [PATCH 07/15] drm/amdgpu: use PCI_IRQ_MSI_TYPES where
> > >appropriate
>
> ...
>
> > > int nvec = pci_msix_vec_count(adev->pdev);
> > > unsigned int flags;
> > >
> > >- if (nvec <= 0) {
> > >+ if (nvec > 0)
> > >+ flags = PCI_IRQ_MSI_TYPES;
> > >+ else
> > > flags = PCI_IRQ_MSI;
> > >- } else {
> > >- flags = PCI_IRQ_MSI | PCI_IRQ_MSIX;
> > >- }
> >
> > Minor nit:
> >
> > Is it really necessary to set do this check? Can flags just
> > be set?
> >
> > I.e.:
> > flags = PCI_IRQ_MSI_TYPES;
> >
> > pci_alloc_irq_vector() tries stuff in order. If MSIX is not available,
> > it will try MSI.
>
> That's also what I proposed earlier. But I suggested as well to wait
> for AMD people to confirm that neither pci_msix_vec_count() nor flags
> is needed and we can directly supply MSI_TYPES to the below call.
>
I think it was leftover from debugging and just to be careful. We had
some issues when we originally enabled MSI-X on certain boards. The
fix was to just allocate a single vector (since that is all we use
anyway) and we were using the wrong irq (pdev->irq vs
pci_irq_vector(pdev, 0)). For reference, the original patch to add
MSI-X:
commit bd660f4f61f60392dd02424c3a3d2240dc2f
Author: shaoyunl
Date: Tue Oct 1 15:52:31 2019 -0400
drm/amdgpu : enable msix for amdgpu driver
We might used out of the msi resources in some cloud project
which have a lot gpu devices(including PF and VF), msix can
provide enough resources from system level view
Signed-off-by: shaoyunl
Reviewed-by: Alex Deucher
Signed-off-by: Alex Deucher
And the fix:
commit 8a745c7ff2ddb8511ef760b4d9cb4cf56a15fc8d
Author: Alex Deucher
Date: Thu Oct 3 10:34:30 2019 -0500
drm/amdgpu: improve MSI-X handling (v3)
Check the number of supported vectors and fall back to MSI if
we return or error or 0 MSI-X vectors.
v2: only allocate one vector. We can't currently use more than
one anyway.
v3: install the irq on vector 0.
Tested-by: Tom St Denis
Reviewed-by: Shaoyun liu
Signed-off-by: Alex Deucher
Alex
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [v2] drm/msm: add shutdown support for display platform_driver
Hi Krishna, On Tue, 2 Jun 2020 at 08:17, Krishna Manikandan wrote: > > Define shutdown callback for display drm driver, > so as to disable all the CRTCS when shutdown > notification is received by the driver. > > This change will turn off the timing engine so > that no display transactions are requested > while mmu translations are getting disabled > during reboot sequence. > > Signed-off-by: Krishna Manikandan > AFAICT atomics is setup in msm_drm_ops::bind and shutdown in msm_drm_ops::unbind. Are you saying that unbind never triggers? If so, then we should really fix that instead, since this patch seems more like a workaround. -Emil ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC PATCH 1/1] drm/mm: add ig_frag selftest
Hi Christian,
On 6/2/20 2:47 PM, Christian König wrote:
Nirmoy please keep in mind that your current implementation doesn't
fully solve the issue the test case is exercising.
In other words what you have implement is fast skipping of fragmented
address space for bottom-up and top-down.
But what this test here exercises is the fast skipping of aligned
allocations. You should probably adjust the test case a bit.
Allocations with size=4k and aign = 8k is known to introduce
fragmentation, do you mean I should only test bottom-up and top-down
for now ?
Regards,
Nirmoy
Regards,
Christian.
Am 29.05.20 um 23:01 schrieb Nirmoy:
On 5/29/20 5:52 PM, Chris Wilson wrote:
Quoting Nirmoy (2020-05-29 16:40:53)
This works correctly most of the times but sometimes
I have to take my word back. In another machine, 20k insertions in
best mode takes 6-9 times more than 10k insertions, all most all the
time.
evict, bottom-up and top-down modes remains in 2-5 times range.
If I reduce the insertions to 1k and 2k then scaling factor for best
mode stays below 4 most of the time.
evict, bottom-up and top-down modes remains in 2-3 times range.
I wonder if it makes sense to test with only 1k and 2k insertions and
tolerate more than error if the mode == best.
Regards,
Nirmoy
20k insertions can take more than 8 times of 10k insertion time.
The pressure is on to improve then :)
Regards,
Nirmoy
On 5/29/20 6:33 PM, Nirmoy Das wrote:
This patch introduces fragmentation in the address range
and measures time taken by 10k and 20k insertions. ig_frag()
will fail if time taken by 20k insertions takes more than 4 times
of 10k insertions as we know that insertions scale quadratically.
Also tolerate 10% error because of kernel scheduler's jitters.
Output:
[ 8092.653518] drm_mm: Testing DRM range manger (struct drm_mm),
with random_seed=0x9bfb4117 max_iterations=8192 max_prime=128
[ 8092.653520] drm_mm: igt_sanitycheck - ok!
[ 8092.653525] igt_debug 0x-0x0200:
512: free
[ 8092.653526] igt_debug 0x0200-0x0600:
1024: used
[ 8092.653527] igt_debug 0x0600-0x0a00:
1024: free
[ 8092.653528] igt_debug 0x0a00-0x0e00:
1024: used
[ 8092.653529] igt_debug 0x0e00-0x1000:
512: free
[ 8092.653529] igt_debug total: 4096, used 2048 free 2048
[ 8112.569813] drm_mm: best fragmented insert of 1 and 2
insertions took 504 and 1996 msecs
[ 8112.723254] drm_mm: bottom-up fragmented insert of 1 and
2 insertions took 44 and 108 msecs
[ 8112.813212] drm_mm: top-down fragmented insert of 1 and
2 insertions took 40 and 44 msecs
[ 8112.847733] drm_mm: evict fragmented insert of 1 and 2
insertions took 8 and 20 msecs
Signed-off-by: Nirmoy Das
---
drivers/gpu/drm/selftests/drm_mm_selftests.h | 1 +
drivers/gpu/drm/selftests/test-drm_mm.c | 73
2 files changed, 74 insertions(+)
diff --git a/drivers/gpu/drm/selftests/drm_mm_selftests.h
b/drivers/gpu/drm/selftests/drm_mm_selftests.h
index 6b943ea1c57d..8c87c964176b 100644
--- a/drivers/gpu/drm/selftests/drm_mm_selftests.h
+++ b/drivers/gpu/drm/selftests/drm_mm_selftests.h
@@ -14,6 +14,7 @@ selftest(insert, igt_insert)
selftest(replace, igt_replace)
selftest(insert_range, igt_insert_range)
selftest(align, igt_align)
+selftest(frag, igt_frag)
selftest(align32, igt_align32)
selftest(align64, igt_align64)
selftest(evict, igt_evict)
diff --git a/drivers/gpu/drm/selftests/test-drm_mm.c
b/drivers/gpu/drm/selftests/test-drm_mm.c
index 9aabe82dcd3a..05d8f3659b4d 100644
--- a/drivers/gpu/drm/selftests/test-drm_mm.c
+++ b/drivers/gpu/drm/selftests/test-drm_mm.c
@@ -1033,6 +1033,79 @@ static int igt_insert_range(void *ignored)
return 0;
}
+static int get_insert_time(unsigned int num_insert,
+ const struct insert_mode *mode)
+{
+ struct drm_mm mm;
+ struct drm_mm_node *nodes, *node, *next;
+ unsigned int size = 4096, align = 8192;
+ unsigned long start;
+ unsigned int i;
+ int ret = -EINVAL;
+
+ drm_mm_init(, 1, U64_MAX - 2);
+ nodes = vzalloc(array_size(num_insert, sizeof(*nodes)));
+ if (!nodes)
+ goto err;
+
+ start = jiffies;
Use ktime_t start = ktime_now();
+ for (i = 0; i < num_insert; i++) {
+ if (!expect_insert(, [i], size, align, i,
mode)) {
+ pr_err("%s insert failed\n", mode->name);
+ goto out;
+ }
+ }
+
+ ret = jiffies_to_msecs(jiffies - start);
ret = ktime_sub(ktime_now(), start);
The downside to using ktime is remembering it is s64 and so requires
care
and attention in doing math.
+out:
+ drm_mm_for_each_node_safe(node, next, )
+ drm_mm_remove_node(node);
+ drm_mm_takedown();
+ vfree(nodes);
+err:
+ return ret;
+
+}
+
+static int igt_frag(void
Re: [PATCH v3 0/16] backlight updates
Hi Sam, On Mon, 1 Jun 2020 at 07:52, Sam Ravnborg wrote: > > v3: > - Dropped video patch that was reviewd and thus applied > - Updated kernel-doc so all fields now have a short intro > - Improved readability in a lot of places, thanks to review >feedback from Daniel - thanks! > - Added better intro to backlight > - Added acks > >Several other smaller changes documented in the >patches. >I left out patches to make functions static as >there are dependencies to drm-misc-next for these. >When this is landed I have a pile of follow-up patches waiting, >mostly introducing backlight_is_blank() all over. > What happened with my suggestion to use backlight_is_blank() in fbdev core itself? It effectively makes 13/13 and the above mentioned follow-up series obsolete. That said, series look spot on. With the documentation fixed (pointer by Daniel) patches 1-12 are: Reviewed-by: Emil Velikov -Emil ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 07/15] drm/amdgpu: use PCI_IRQ_MSI_TYPES where appropriate
On Tue, Jun 2, 2020 at 4:38 PM Ruhl, Michael J wrote:
> >-Original Message-
> >From: dri-devel On Behalf Of
> >Piotr Stankiewicz
> >Sent: Tuesday, June 2, 2020 5:21 AM
> >To: Alex Deucher ; Christian König
> >; David Zhou ; David
> >Airlie ; Daniel Vetter
> >Cc: Stankiewicz, Piotr ; dri-
> >de...@lists.freedesktop.org; amd-...@lists.freedesktop.org; linux-
> >ker...@vger.kernel.org
> >Subject: [PATCH 07/15] drm/amdgpu: use PCI_IRQ_MSI_TYPES where
> >appropriate
...
> > int nvec = pci_msix_vec_count(adev->pdev);
> > unsigned int flags;
> >
> >- if (nvec <= 0) {
> >+ if (nvec > 0)
> >+ flags = PCI_IRQ_MSI_TYPES;
> >+ else
> > flags = PCI_IRQ_MSI;
> >- } else {
> >- flags = PCI_IRQ_MSI | PCI_IRQ_MSIX;
> >- }
>
> Minor nit:
>
> Is it really necessary to set do this check? Can flags just
> be set?
>
> I.e.:
> flags = PCI_IRQ_MSI_TYPES;
>
> pci_alloc_irq_vector() tries stuff in order. If MSIX is not available,
> it will try MSI.
That's also what I proposed earlier. But I suggested as well to wait
for AMD people to confirm that neither pci_msix_vec_count() nor flags
is needed and we can directly supply MSI_TYPES to the below call.
> > /* we only need one vector */
> > nvec = pci_alloc_irq_vectors(adev->pdev, 1, 1, flags);
--
With Best Regards,
Andy Shevchenko
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3] drm/doc: device hot-unplug for userspace
On 6/1/20 10:32 AM, Pekka Paalanen wrote: From: Pekka Paalanen Set up the expectations on how hot-unplugging a DRM device should look like to userspace. Written by Daniel Vetter's request and largely based on his comments in IRC and from https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Farchives%2Fdri-devel%2F2020-May%2F265484.htmldata=02%7C01%7Candrey.grodzovsky%40amd.com%7Cfc4392da89ea4fc4281b08d806389835%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637266187434486889sdata=Mbsx6qBXN9MBnDDJi4shRZobf7tjcClvNUlUCPsiVtw%3Dreserved=0 . A related Wayland protocol change proposal is at https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.freedesktop.org%2Fwayland%2Fwayland-protocols%2F-%2Fmerge_requests%2F35data=02%7C01%7Candrey.grodzovsky%40amd.com%7Cfc4392da89ea4fc4281b08d806389835%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637266187434486889sdata=5j%2FNQFW0C1wvdnR%2BC0WgGU0JcNCb8fIYBPXFOmp36Ck%3Dreserved=0 Signed-off-by: Pekka Paalanen Cc: Daniel Vetter Cc: Andrey Grodzovsky Cc: Dave Airlie Cc: Sean Paul Cc: Simon Ser --- v3: - update ENODEV doc (Daniel) - clarify existing vs. new mmaps (Andrey) - split into KMS and render/cross sections (Andrey, Daniel) - open() returns ENXIO (open(2) man page) - ioctls may return ENODEV (Andrey, Daniel) - new wayland-protocols MR v2: - mmap reads/writes undefined (Daniel) - make render ioctl behaviour driver-specific (Daniel) - restructure the mmap paragraphs (Daniel) - chardev minor notes (Simon) - open behaviour (Daniel) - DRM leasing behaviour (Daniel) - added links Disclaimer: I am a userspace developer writing for other userspace developers. I took some liberties in defining what should happen without knowing what is actually possible or what existing drivers already implement. --- Documentation/gpu/drm-uapi.rst | 114 - 1 file changed, 113 insertions(+), 1 deletion(-) diff --git a/Documentation/gpu/drm-uapi.rst b/Documentation/gpu/drm-uapi.rst index 56fec6ed1ad8..db56c681b648 100644 --- a/Documentation/gpu/drm-uapi.rst +++ b/Documentation/gpu/drm-uapi.rst @@ -1,3 +1,5 @@ +.. Copyright 2020 DisplayLink (UK) Ltd. + === Userland interfaces === @@ -162,6 +164,116 @@ other hand, a driver requires shared state between clients which is visible to user-space and accessible beyond open-file boundaries, they cannot support render nodes. +Device Hot-Unplug += + +.. note:: + The following is the plan. Implementation is not there yet + (2020 May). + +Graphics devices (display and/or render) may be connected via USB (e.g. +display adapters or docking stations) or Thunderbolt (e.g. eGPU). An end +user is able to hot-unplug this kind of devices while they are being +used, and expects that the very least the machine does not crash. Any +damage from hot-unplugging a DRM device needs to be limited as much as +possible and userspace must be given the chance to handle it if it wants +to. Ideally, unplugging a DRM device still lets a desktop to continue +running, but that is going to need explicit support throughout the whole +graphics stack: from kernel and userspace drivers, through display +servers, via window system protocols, and in applications and libraries. + +Other scenarios that should lead to the same are: unrecoverable GPU +crash, PCI device disappearing off the bus, or forced unbind of a driver +from the physical device. + +In other words, from userspace perspective everything needs to keep on +working more or less, until userspace stops using the disappeared DRM +device and closes it completely. Userspace will learn of the device +disappearance from the device removed uevent, ioctls returning ENODEV +(or driver-specific ioctls returning driver-specific things), or open() +returning ENXIO. + +Only after userspace has closed all relevant DRM device and dmabuf file +descriptors and removed all mmaps, the DRM driver can tear down its +instance for the device that no longer exists. If the same physical +device somehow comes back in the mean time, it shall be a new DRM +device. + +Similar to PIDs, chardev minor numbers are not recycled immediately. A +new DRM device always picks the next free minor number compared to the +previous one allocated, and wraps around when minor numbers are +exhausted. + +The goal raises at least the following requirements for the kernel and +drivers. + +Requirements for KMS UAPI +- + +- KMS connectors must change their status to disconnected. + +- Legacy modesets and pageflips, and atomic commits, both real and + TEST_ONLY, and any other ioctls either fail with ENODEV or fake + success. + +- Pending non-blocking KMS operations deliver the DRM events userspace + is expecting. This applies also to ioctls that faked success. + +- open() on a device node whose underlying device has disappeared will + fail with ENXIO. + +- Attempting to create a
Re: [PATCH 1/7] drm/rockchip: prepare common code for cdns and rk dpi/dp driver
HI Sandor Yu
On Mon, 1 Jun 2020 at 07:29, wrote:
>
> From: Sandor Yu
>
> - Extracted common fields from cdn_dp_device to a new cdns_mhdp_device
> structure which will be used by two separate drivers later on.
> - Moved some datatypes (audio_format, audio_info, vic_pxl_encoding_format,
> video_info) from cdn-dp-core.c to cdn-dp-reg.h.
> - Changed prefixes from cdn_dp to cdns_mhdp
> cdn -> cdns to match the other Cadence's drivers
> dp -> mhdp to distinguish it from a "just a DP" as the IP underneath
> this registers map can be a HDMI (which is internally different,
> but the interface for commands, events is pretty much the same).
> - Modified cdn-dp-core.c to use the new driver structure and new function
> names.
> - writel and readl are replaced by cdns_mhdp_bus_write and
> cdns_mhdp_bus_read.
>
The high-level idea is great - split, refactor and reuse the existing drivers.
Although looking at the patches themselves - they seems to be doing
multiple things at once.
As indicated by the extensive list in the commit log.
I would suggest splitting those up a bit, roughly in line of the
itemisation as per the commit message.
Here is one hand wavy way to chunk this patch:
1) use put_unalligned*
2) 'use local variable dev' style of changes (as seem in cdn_dp_clk_enable)
3) add writel/readl wrappers
4) hookup struct cdns_mhdp_device, keep dp->mhdp detail internal.
The cdn-dp-reg.h function names/signatures will stay the same.
5) finalize the helpers - use mhdp directly, rename
HTH
Emil
Examples:
4)
static int cdn_dp_mailbox_read(struct cdn_dp_device *dp)
{
+" struct cdns_mhdp_device *mhdp = dp->mhdp;
int val, ret;
- ret = readx_poll_timeout(readl, dp->regs + MAILBOX_EMPTY_ADDR,
+ ret = readx_poll_timeout(readl, mhdp->regs_base + MAILBOX_EMPTY_ADDR,
...
return fancy_readl(dp, MAILBOX0_RD_DATA) & 0xff;
}
5)
-static int cdn_dp_mailbox_read(struct cdn_dp_device *dp)
+static int mhdp_mailbox_read(struct cdns_mhdp_device *mhdp)
{
- struct cdns_mhdp_device *mhdp = dp->mhdp;
int val, ret;
...
- return fancy_readl(dp, MAILBOX0_RD_DATA) & 0xff;
+ return cdns_mhdp_bus_read(mhdp, MAILBOX0_RD_DATA) & 0xff;
}
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
RE: [PATCH 07/15] drm/amdgpu: use PCI_IRQ_MSI_TYPES where appropriate
>-Original Message-
>From: dri-devel On Behalf Of
>Piotr Stankiewicz
>Sent: Tuesday, June 2, 2020 5:21 AM
>To: Alex Deucher ; Christian König
>; David Zhou ; David
>Airlie ; Daniel Vetter
>Cc: Stankiewicz, Piotr ; dri-
>de...@lists.freedesktop.org; amd-...@lists.freedesktop.org; linux-
>ker...@vger.kernel.org
>Subject: [PATCH 07/15] drm/amdgpu: use PCI_IRQ_MSI_TYPES where
>appropriate
>
>Seeing as there is shorthand available to use when asking for any type
>of interrupt, or any type of message signalled interrupt, leverage it.
>
>Signed-off-by: Piotr Stankiewicz
>Reviewed-by: Andy Shevchenko
>---
> drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c | 8
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
>diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
>b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
>index 5ed4227f304b..6dbe173a9fd4 100644
>--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
>+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
>@@ -251,11 +251,11 @@ int amdgpu_irq_init(struct amdgpu_device *adev)
> int nvec = pci_msix_vec_count(adev->pdev);
> unsigned int flags;
>
>- if (nvec <= 0) {
>+ if (nvec > 0)
>+ flags = PCI_IRQ_MSI_TYPES;
>+ else
> flags = PCI_IRQ_MSI;
>- } else {
>- flags = PCI_IRQ_MSI | PCI_IRQ_MSIX;
>- }
Minor nit:
Is it really necessary to set do this check? Can flags just
be set?
I.e.:
flags = PCI_IRQ_MSI_TYPES;
pci_alloc_irq_vector() tries stuff in order. If MSIX is not available,
it will try MSI.
M
>+
> /* we only need one vector */
> nvec = pci_alloc_irq_vectors(adev->pdev, 1, 1, flags);
> if (nvec > 0) {
>--
>2.17.2
>
>___
>dri-devel mailing list
>dri-devel@lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/dri-devel
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/fourcc: document modifier uniqueness requirements
On Mon, Jun 01, 2020 at 11:35:54AM +0100, Brian Starkey wrote:
> On Wed, May 27, 2020 at 10:55:34AM +0200, Daniel Vetter wrote:
> > On Wed, May 27, 2020 at 08:52:00AM +, Simon Ser wrote:
> > > There have suggestions to bake pitch alignment, address alignement,
> > > contiguous memory or other placement (hidden VRAM, GTT/BAR, etc)
> > > constraints into modifiers. Last time this was brought up it seemed
> > > like the consensus was to not allow this. Document this in drm_fourcc.h.
> > >
> > > There are several reasons for this.
> > >
> > > - Encoding all of these constraints in the modifiers would explode the
> > > search space pretty quickly (we only have 64 bits to work with).
> > > - Modifiers need to be unambiguous: a buffer can only have a single
> > > modifier.
> > > - Modifier users aren't expected to parse modifiers.
> > >
> > > Signed-off-by: Simon Ser
> > > Cc: Daniel Vetter
> > > Cc: Daniel Stone
> > > Cc: Bas Nieuwenhuizen
> > > Cc: Dave Airlie
> > > Cc: Marek Olšák
> > > ---
> > > include/uapi/drm/drm_fourcc.h | 11 +++
> > > 1 file changed, 11 insertions(+)
> > >
> > > diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> > > index 490143500a50..97eb0f1cf9f8 100644
> > > --- a/include/uapi/drm/drm_fourcc.h
> > > +++ b/include/uapi/drm/drm_fourcc.h
> > > @@ -58,6 +58,17 @@ extern "C" {
> > > * may preserve meaning - such as number of planes - from the fourcc
> > > code,
> > > * whereas others may not.
> > > *
> > > + * Modifiers must uniquely encode buffer layout. In other words, a
> > > buffer must
> > > + * match only a single modifier. A modifier must not be a subset of
> > > layouts of
> > > + * another modifier. For instance, it's incorrect to encode pitch
> > > alignment in
> > > + * a modifier: a buffer may match a 64-pixel aligned modifier and a
> > > 32-pixel
> > > + * aligned modifier. That said, modifiers can have implicit minimal
> > > + * requirements.
> >
> > I think we should also add the aliasing when the fourcc codes are
> > involved, with afbc as example. Maybe:
> >
> > For modifiers where the combination of fourcc code and modifier can alias,
> > a canonical pair needs to be defined and used by all drivers. An example
> > is afbc, where both argb and abgr have the exact same compressed layout.
>
> That's actually explicitly _not_ the case for AFBC, which was one of
> the things I was trying to document in afbc.rst.
I guess I wasn't clear: I wanted to highlight afbc as one modifier where
we discussed this aliasing problem, and worded the entire spec to make
sure the aliasing doesn't happen.
>
> >
> > With something like that added:
> >
> > Reviewed-by: Daniel Vetter
> >
> >
> > > + *
> > > + * Users see modifiers as opaque tokens they can check for equality and
> > > + * intersect. Users musn't need to know to reason about the modifier
> > > value
> > > + * (i.e. users are not expected to extract information out of the
> > > modifier).
> > > + *
>
> Doesn't this conflict with "implicit minimal requirements" above?
>
> Certainly for a bunch of different AFBC modifiers, the allocator would
> need to understand some fields in order to properly align-up the
> buffer size.
There's kinda two side to the modifier coin:
- one is how this all looks to the higher levels sitting on top of
egl/kms/... For those modifiers should be opaque things. And we really
don't care how much they alias, since worst case it's just a bunch more
modifiers to compare and pass around.
- the other side is the implement. For that side it very much matters that
modifiers don't alias badly, so that we can avoid cases where the hw
supports a common format, but the drivers use different aliases to
discribe it, preventing buffer sharing in an efficient format.
Finally we never let higher levels allocate the buffers, it's always just
some low-level allocator that does that (gbm, egl, ...). And those lower
levels obviously should understand the implementation and alignment
constraints of the modifiers involved.
Should we make this split a bit clearer of how this is supposed to work in
userspace?
-Daniel
>
> Thanks,
> -Brian
>
> > > * Vendors should document their modifier usage in as much detail as
> > > * possible, to ensure maximum compatibility across devices, drivers and
> > > * applications.
> > > --
> > > 2.26.2
> > >
> > >
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> > ___
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] video: fbdev: pxafb: Use correct return value for pxafb_probe()
Hi Bart,
On Mon, Jun 01, 2020 at 03:25:25PM +0200, Bartlomiej Zolnierkiewicz wrote:
>
> On 5/25/20 9:11 AM, Tiezhu Yang wrote:
> > When call function devm_platform_ioremap_resource(), we should use IS_ERR()
> > to check the return value and return PTR_ERR() if failed.
> >
> > Signed-off-by: Tiezhu Yang
>
> Applied to drm-misc-next tree (patch should show up in v5.9), thanks.
Thanks for going through all the backlog of patches in the fbdev area
every once in a while! That kind of housekeeping work is often
underappreciated, but rather important to keep the ship going.
Cheers, Daniel
PS: Of course also holds for everyone else doing this in other areas.
fbdev simply stuck out just now catching up on mails.
>
> Best regards,
> --
> Bartlomiej Zolnierkiewicz
> Samsung R Institute Poland
> Samsung Electronics
>
> > ---
> > drivers/video/fbdev/pxafb.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/video/fbdev/pxafb.c b/drivers/video/fbdev/pxafb.c
> > index 00b96a7..423331c 100644
> > --- a/drivers/video/fbdev/pxafb.c
> > +++ b/drivers/video/fbdev/pxafb.c
> > @@ -2305,7 +2305,7 @@ static int pxafb_probe(struct platform_device *dev)
> > fbi->mmio_base = devm_platform_ioremap_resource(dev, 0);
> > if (IS_ERR(fbi->mmio_base)) {
> > dev_err(>dev, "failed to get I/O memory\n");
> > - ret = -EBUSY;
> > + ret = PTR_ERR(fbi->mmio_base);
> > goto failed;
> > }
> >
> >
> ___
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 02/21] drm: mxsfb: Use drm_panel_bridge
On Sat, May 30, 2020 at 05:14:21AM +0300, Laurent Pinchart wrote: > Hi Stefan, > > On Mon, Mar 23, 2020 at 10:27:21PM +0100, Stefan Agner wrote: > > On 2020-03-09 20:51, Laurent Pinchart wrote: > > > Replace the manual connector implementation based on drm_panel with the > > > drm_panel_bridge helper. This simplifies the mxsfb driver by removing > > > connector-related code, and standardizing all pipeline control > > > operations on bridges. > > > > > > A hack is needed to get hold of the connector, as that's our only source > > > of bus flags and formats for now. As soon as the bridge API provides us > > > with that information this can be fixed. > > > > This seems like a nice simplification. > > > > I gave this a go applied on today's drm-misc-next using a Colibri iMX7 > > (imx7d-colibri-emmc-eval-v3.dts device tree). This device makes use of > > the simple panel driver. I get this when booting: > > > > [2.976698] [drm] Supports vblank timestamp caching Rev 2 > > (21.10.2013). > > [2.983526] [ cut here ] > > [2.988180] WARNING: CPU: 0 PID: 1 at > > drivers/gpu/drm/bridge/panel.c:267 devm_drm_panel_bridge_add+0x25/0x28 > > [2.998059] Modules linked in: > > [3.001159] CPU: 0 PID: 1 Comm: swapper/0 Not tainted > > 5.6.0-rc5-yocto-standard-01250-ga4ce5db7c9f1 #29 > > [3.010493] Hardware name: Freescale i.MX7 Dual (Device Tree) > > [3.016281] [] (unwind_backtrace) from [] > > (show_stack+0xb/0xc) > > [3.023887] [] (show_stack) from [] > > (dump_stack+0x63/0x70) > > [3.031144] [] (dump_stack) from [] > > (__warn+0x9d/0xb0) > > [3.038047] [] (__warn) from [] > > (warn_slowpath_fmt+0x6b/0x70) > > [3.045564] [] (warn_slowpath_fmt) from [] > > (devm_drm_panel_bridge_add+0x25/0x28) > > [3.054736] [] (devm_drm_panel_bridge_add) from > > [] (mxsfb_probe+0x197/0x2e0) > > [3.063559] [] (mxsfb_probe) from [] > > (platform_drv_probe+0x2d/0x60) > > [3.071598] [] (platform_drv_probe) from [] > > (really_probe+0x1dd/0x330) > > [3.079897] [] (really_probe) from [] > > (driver_probe_device+0x4f/0x154) > > [3.088195] [] (driver_probe_device) from [] > > (device_driver_attach+0x37/0x44) > > [3.097101] [] (device_driver_attach) from [] > > (__driver_attach+0x7b/0xec) > > [3.105660] [] (__driver_attach) from [] > > (bus_for_each_dev+0x3d/0x64) > > [3.113870] [] (bus_for_each_dev) from [] > > (bus_add_driver+0xef/0x160) > > [3.122081] [] (bus_add_driver) from [] > > (driver_register+0x35/0x9c) > > [3.130119] [] (driver_register) from [] > > (do_one_initcall+0x3d/0x1e4) > > [3.138333] [] (do_one_initcall) from [] > > (kernel_init_freeable+0x1b3/0x1fc) > > [3.147069] [] (kernel_init_freeable) from [] > > (kernel_init+0x7/0xd0) > > [3.155194] [] (kernel_init) from [] > > (ret_from_fork+0x11/0x38) > > [3.162789] Exception stack(0xec0e3fb0 to 0xec0e3ff8) > > [3.167862] 3fa0: > > > > [3.176071] 3fc0: > > > > [3.184278] 3fe0: 0013 > > > > [3.191029] ---[ end trace b69e1f44de470959 ]--- > > [3.195671] mxsfb 3073.lcdif: Cannot connect bridge: -19 > > > > Should we maybe use devm_drm_panel_bridge_add_typed? > > As Sam reported, this is caused by the panel not setting connector_type. > We could use devm_drm_panel_bridge_add_typed(), even if I like the > warning as it uncovers the problematic panels and gets them fixed. What > would be your preference ? Adding warnings everywhere is kinda uncool, at least my experience is that if there's too much you get warning overload and train people to ignore them. Imo either hide the wwarning for now, or if it's not too much work, review all the panel drivers and make sure they set the connector type somewhere. Warnings are kinda ok once you're pretty sure you got them all, and want to make sure newly added drivers get this all right. But not before we've reached that. Cheers, Daniel > > > Two more comments below. > > > > > Signed-off-by: Laurent Pinchart > > > --- > > > drivers/gpu/drm/mxsfb/Makefile| 2 +- > > > drivers/gpu/drm/mxsfb/mxsfb_drv.c | 105 ++ > > > drivers/gpu/drm/mxsfb/mxsfb_drv.h | 5 +- > > > drivers/gpu/drm/mxsfb/mxsfb_out.c | 99 > > > 4 files changed, 53 insertions(+), 158 deletions(-) > > > delete mode 100644 drivers/gpu/drm/mxsfb/mxsfb_out.c > > > > > > diff --git a/drivers/gpu/drm/mxsfb/Makefile > > > b/drivers/gpu/drm/mxsfb/Makefile > > > index ff6e358088fa..811584e54ad1 100644 > > > --- a/drivers/gpu/drm/mxsfb/Makefile > > > +++ b/drivers/gpu/drm/mxsfb/Makefile > > > @@ -1,3 +1,3 @@ > > > # SPDX-License-Identifier: GPL-2.0-only > > > -mxsfb-y := mxsfb_drv.o mxsfb_crtc.o mxsfb_out.o > > > +mxsfb-y := mxsfb_drv.o mxsfb_crtc.o > > > obj-$(CONFIG_DRM_MXSFB) +=
Re: [PATCH] drm/malidp: Don't call drm_crtc_vblank_off on unbind
Hi Daniel, On Tue, Jun 02, 2020 at 11:55:05AM +0200, Daniel Vetter wrote: > This is already done as part of the drm_atomic_helper_shutdown(), > and in that case only for the crtc which are actually on. > > v2: I overlooked that malidp also needs to have it's interrupt shut > down reordered. Got confused by the subject not having any version of the patch, so I've acked the other one, but this is the one I've meant to Ack. So, Acked-by: Liviu Dudau Best regards, Liviu > > Signed-off-by: Daniel Vetter > Cc: Liviu Dudau > Cc: Brian Starkey > --- > drivers/gpu/drm/arm/malidp_drv.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/arm/malidp_drv.c > b/drivers/gpu/drm/arm/malidp_drv.c > index 02904392e370..cdb817a7c611 100644 > --- a/drivers/gpu/drm/arm/malidp_drv.c > +++ b/drivers/gpu/drm/arm/malidp_drv.c > @@ -928,11 +928,10 @@ static void malidp_unbind(struct device *dev) > drm_dev_unregister(drm); > drm_kms_helper_poll_fini(drm); > pm_runtime_get_sync(dev); > - drm_crtc_vblank_off(>crtc); > + drm_atomic_helper_shutdown(drm); > malidp_se_irq_fini(hwdev); > malidp_de_irq_fini(hwdev); > drm->irq_enabled = false; > - drm_atomic_helper_shutdown(drm); > component_unbind_all(dev, drm); > of_node_put(malidp->crtc.port); > malidp->crtc.port = NULL; > -- > 2.26.2 > -- | I would like to | | fix the world, | | but they're not | | giving me the | \ source code! / --- ¯\_(ツ)_/¯ ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 3/3] drm/hdlcd: Don't call drm_crtc_vblank_off on unbind
On Tue, Jun 02, 2020 at 11:51:40AM +0200, Daniel Vetter wrote: > This is already taken care of by drm_atomic_helper_shutdown(), and > in that case only for the CRTC which are actually on. > > Only tricky bit here is that we kill the interrupt handling before we > shut down crtc, so need to reorder that. > > Signed-off-by: Daniel Vetter > Cc: Liviu Dudau Acked-by: Liviu Dudau Best regards, Liviu > Cc: Brian Starkey > Cc: > --- > drivers/gpu/drm/arm/hdlcd_drv.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/arm/hdlcd_drv.c b/drivers/gpu/drm/arm/hdlcd_drv.c > index 194419f47c5e..26bc5d7766f5 100644 > --- a/drivers/gpu/drm/arm/hdlcd_drv.c > +++ b/drivers/gpu/drm/arm/hdlcd_drv.c > @@ -347,9 +347,8 @@ static void hdlcd_drm_unbind(struct device *dev) > of_node_put(hdlcd->crtc.port); > hdlcd->crtc.port = NULL; > pm_runtime_get_sync(dev); > - drm_crtc_vblank_off(>crtc); > - drm_irq_uninstall(drm); > drm_atomic_helper_shutdown(drm); > + drm_irq_uninstall(drm); > pm_runtime_put(dev); > if (pm_runtime_enabled(dev)) > pm_runtime_disable(dev); > -- > 2.26.2 > -- | I would like to | | fix the world, | | but they're not | | giving me the | \ source code! / --- ¯\_(ツ)_/¯ ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/3] drm/malidp: Don't call drm_crtc_vblank_off on unbind
Hi Daniel,
On Tue, Jun 02, 2020 at 11:51:39AM +0200, Daniel Vetter wrote:
> This is already done as part of the drm_atomic_helper_shutdown(),
> and in that case only for the crtc which are actually on.
>
I'm pretty sure that it didn't used to be the case when I wrote the code
and I was hitting warnings from 84014b0a39eef6df ("drm/atomic-helper: check that
drivers call drm_crtc_vblank_off"), but I'm happy that things have now been
fixed.
> Signed-off-by: Daniel Vetter
> Cc: Liviu Dudau
Acked-by: Liviu Dudau
Best regards,
Liviu
> Cc: Brian Starkey
> Cc:
> ---
> drivers/gpu/drm/arm/malidp_drv.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/arm/malidp_drv.c
> b/drivers/gpu/drm/arm/malidp_drv.c
> index 02904392e370..db6ba5c78042 100644
> --- a/drivers/gpu/drm/arm/malidp_drv.c
> +++ b/drivers/gpu/drm/arm/malidp_drv.c
> @@ -928,7 +928,6 @@ static void malidp_unbind(struct device *dev)
> drm_dev_unregister(drm);
> drm_kms_helper_poll_fini(drm);
> pm_runtime_get_sync(dev);
> - drm_crtc_vblank_off(>crtc);
> malidp_se_irq_fini(hwdev);
> malidp_de_irq_fini(hwdev);
> drm->irq_enabled = false;
> --
> 2.26.2
>
--
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---
¯\_(ツ)_/¯
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Linux-stm32] [PATCH v8 08/10] drm: stm: dw-mipi-dsi: let the bridge handle the HW version check
Hi Adrian, On Mon, 1 Jun 2020 at 10:14, Adrian Ratiu wrote: > > On Fri, 29 May 2020, Philippe CORNU wrote: > > Hi Adrian, and thank you very much for the patchset. Thank you > > also for having tested it on STM32F769 and STM32MP1. Sorry for > > the late response, Yannick and I will review it as soon as > > possible and we will keep you posted. Note: Do not hesitate to > > put us in copy for the next version (philippe.co...@st.com, > > yannick.fer...@st.com) Regards, Philippe :-) > > Hi Philippe, > > Thank you very much for your previous and future STM testing, > really appreciate it! I've CC'd Yannick until now but I'll also CC > you sure :) > > It's been over a month since I posted v8 and I was just gearing up > to address all feedback, rebase & retest to prepare v9 but I'll > wait a little longer, no problem, it's no rush. > Small idea, pardon for joining so late: Might be a good idea to add inline comment, why the clocks are disabled so late. Effectively a 2 line version of the commit summary. Feel free to make that a separate/follow-up patch. -Emil ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/2] drm/panel: simple: Add support for KOE TX26D202VM0BWA panel
On Tue, 2 Jun 2020 at 08:17, Liu Ying wrote:
>
> This patch adds support for Kaohsiung Opto-Electronics Inc.
> 10.1" TX26D202VM0BWA WUXGA(1920x1200) TFT LCD panel with LVDS interface.
> The panel has dual LVDS channels.
>
> My panel is manufactured by US Micro Products(USMP). There is a tag at
> the back of the panel, which indicates the panel type is 'TX26D202VM0BWA'
> and it's made by KOE in Taiwan.
>
> The panel spec from USMP can be found at:
> https://www.usmicroproducts.com/sites/default/files/datasheets/USMP-T101-192120NDU-A0.pdf
>
> The below panel spec from KOE is basically the same to the one from USMP.
> However, the panel type 'TX26D202VM0BAA' is a little bit different.
> It looks that the two types of panel are compatible with each other.
> http://www.koe.j-display.com/upload/product/TX26D202VM0BAA.pdf
>
> Cc: Thierry Reding
> Cc: Sam Ravnborg
> Signed-off-by: Liu Ying
> ---
> drivers/gpu/drm/panel/panel-simple.c | 34 ++
> 1 file changed, 34 insertions(+)
>
> diff --git a/drivers/gpu/drm/panel/panel-simple.c
> b/drivers/gpu/drm/panel/panel-simple.c
> index b6ecd15..7c222ec 100644
> --- a/drivers/gpu/drm/panel/panel-simple.c
> +++ b/drivers/gpu/drm/panel/panel-simple.c
> @@ -2200,6 +2200,37 @@ static const struct panel_desc koe_tx14d24vm1bpa = {
> },
> };
>
> +static const struct display_timing koe_tx26d202vm0bwa_timing = {
> + .pixelclock = { 15182, 15672, 15978 },
> + .hactive = { 1920, 1920, 1920 },
> + .hfront_porch = { 105, 130, 142 },
> + .hback_porch = { 45, 70, 82 },
> + .hsync_len = { 30, 30, 30 },
> + .vactive = { 1200, 1200, 1200},
> + .vfront_porch = { 3, 5, 10 },
> + .vback_porch = { 2, 5, 10 },
> + .vsync_len = { 5, 5, 5 },
> +};
> +
> +static const struct panel_desc koe_tx26d202vm0bwa = {
> + .timings = _tx26d202vm0bwa_timing,
> + .num_timings = 1,
> + .bpc = 8,
> + .size = {
> + .width = 217,
> + .height = 136,
> + },
> + .delay = {
> + .prepare = 1000,
> + .enable = 1000,
> + .unprepare = 1000,
> + .disable = 1000,
Ouch 1s for each delay is huge. Nevertheless it matches the specs so,
the series is:
Reviewed-by: Emil Velikov
Sam, Thierry I assume you'll merge the series. Let me know if I should
pick it up.
-Emil
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC PATCH 1/1] drm/mm: add ig_frag selftest
Nirmoy please keep in mind that your current implementation doesn't
fully solve the issue the test case is exercising.
In other words what you have implement is fast skipping of fragmented
address space for bottom-up and top-down.
But what this test here exercises is the fast skipping of aligned
allocations. You should probably adjust the test case a bit.
Regards,
Christian.
Am 29.05.20 um 23:01 schrieb Nirmoy:
On 5/29/20 5:52 PM, Chris Wilson wrote:
Quoting Nirmoy (2020-05-29 16:40:53)
This works correctly most of the times but sometimes
I have to take my word back. In another machine, 20k insertions in
best mode takes 6-9 times more than 10k insertions, all most all the
time.
evict, bottom-up and top-down modes remains in 2-5 times range.
If I reduce the insertions to 1k and 2k then scaling factor for best
mode stays below 4 most of the time.
evict, bottom-up and top-down modes remains in 2-3 times range.
I wonder if it makes sense to test with only 1k and 2k insertions and
tolerate more than error if the mode == best.
Regards,
Nirmoy
20k insertions can take more than 8 times of 10k insertion time.
The pressure is on to improve then :)
Regards,
Nirmoy
On 5/29/20 6:33 PM, Nirmoy Das wrote:
This patch introduces fragmentation in the address range
and measures time taken by 10k and 20k insertions. ig_frag()
will fail if time taken by 20k insertions takes more than 4 times
of 10k insertions as we know that insertions scale quadratically.
Also tolerate 10% error because of kernel scheduler's jitters.
Output:
[ 8092.653518] drm_mm: Testing DRM range manger (struct drm_mm),
with random_seed=0x9bfb4117 max_iterations=8192 max_prime=128
[ 8092.653520] drm_mm: igt_sanitycheck - ok!
[ 8092.653525] igt_debug 0x-0x0200:
512: free
[ 8092.653526] igt_debug 0x0200-0x0600:
1024: used
[ 8092.653527] igt_debug 0x0600-0x0a00:
1024: free
[ 8092.653528] igt_debug 0x0a00-0x0e00:
1024: used
[ 8092.653529] igt_debug 0x0e00-0x1000:
512: free
[ 8092.653529] igt_debug total: 4096, used 2048 free 2048
[ 8112.569813] drm_mm: best fragmented insert of 1 and 2
insertions took 504 and 1996 msecs
[ 8112.723254] drm_mm: bottom-up fragmented insert of 1 and
2 insertions took 44 and 108 msecs
[ 8112.813212] drm_mm: top-down fragmented insert of 1 and
2 insertions took 40 and 44 msecs
[ 8112.847733] drm_mm: evict fragmented insert of 1 and 2
insertions took 8 and 20 msecs
Signed-off-by: Nirmoy Das
---
drivers/gpu/drm/selftests/drm_mm_selftests.h | 1 +
drivers/gpu/drm/selftests/test-drm_mm.c | 73
2 files changed, 74 insertions(+)
diff --git a/drivers/gpu/drm/selftests/drm_mm_selftests.h
b/drivers/gpu/drm/selftests/drm_mm_selftests.h
index 6b943ea1c57d..8c87c964176b 100644
--- a/drivers/gpu/drm/selftests/drm_mm_selftests.h
+++ b/drivers/gpu/drm/selftests/drm_mm_selftests.h
@@ -14,6 +14,7 @@ selftest(insert, igt_insert)
selftest(replace, igt_replace)
selftest(insert_range, igt_insert_range)
selftest(align, igt_align)
+selftest(frag, igt_frag)
selftest(align32, igt_align32)
selftest(align64, igt_align64)
selftest(evict, igt_evict)
diff --git a/drivers/gpu/drm/selftests/test-drm_mm.c
b/drivers/gpu/drm/selftests/test-drm_mm.c
index 9aabe82dcd3a..05d8f3659b4d 100644
--- a/drivers/gpu/drm/selftests/test-drm_mm.c
+++ b/drivers/gpu/drm/selftests/test-drm_mm.c
@@ -1033,6 +1033,79 @@ static int igt_insert_range(void *ignored)
return 0;
}
+static int get_insert_time(unsigned int num_insert,
+ const struct insert_mode *mode)
+{
+ struct drm_mm mm;
+ struct drm_mm_node *nodes, *node, *next;
+ unsigned int size = 4096, align = 8192;
+ unsigned long start;
+ unsigned int i;
+ int ret = -EINVAL;
+
+ drm_mm_init(, 1, U64_MAX - 2);
+ nodes = vzalloc(array_size(num_insert, sizeof(*nodes)));
+ if (!nodes)
+ goto err;
+
+ start = jiffies;
Use ktime_t start = ktime_now();
+ for (i = 0; i < num_insert; i++) {
+ if (!expect_insert(, [i], size, align, i,
mode)) {
+ pr_err("%s insert failed\n", mode->name);
+ goto out;
+ }
+ }
+
+ ret = jiffies_to_msecs(jiffies - start);
ret = ktime_sub(ktime_now(), start);
The downside to using ktime is remembering it is s64 and so requires
care
and attention in doing math.
+out:
+ drm_mm_for_each_node_safe(node, next, )
+ drm_mm_remove_node(node);
+ drm_mm_takedown();
+ vfree(nodes);
+err:
+ return ret;
+
+}
+
+static int igt_frag(void *ignored)
+{
+ const struct insert_mode *mode;
+ unsigned int insert_time1, insert_time2;
+ unsigned int insert_size = 1;
+ unsigned int scale_factor = 4;
+ /* tolerate 10% excess insertion duration */
+
Re: [PATCH] drm/vkms: Optimize compute_crc(), blend()
On Mon, 1 Jun 2020 at 01:25, Rodrigo Siqueira wrote: > > Hi, > > First of all, thanks a lot for all your patch. And thanks Emil for your > feedback. > > I have a suggestion here: > > Emil: > Could you give me your Acked-by or maybe Reviewed-by for the writeback > series? With that, I can finally apply the series. > Sure, once the issues highlighted are resolved. Just left you some more comprehensive feedback. -Emil P.S. Something something top posting sucks :-P ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH V4 3/3] drm/vkms: Add support for writeback
On Mon, 11 May 2020 at 12:55, Rodrigo Siqueira wrote:
> # SPDX-License-Identifier: GPL-2.0-only
> -vkms-y := vkms_drv.o vkms_plane.o vkms_output.o vkms_crtc.o vkms_gem.o
> vkms_composer.o
> +vkms-y := \
> + vkms_drv.o \
> + vkms_plane.o \
> + vkms_output.o \
> + vkms_crtc.o \
> + vkms_gem.o \
> + vkms_composer.o \
> + vkms_writeback.o
>
Nit: sort alphabetically
> @@ -191,9 +192,12 @@ void vkms_composer_worker(struct work_struct *work)
> if (!primary_composer)
> return;
>
> + if (wb_pending)
> + vaddr_out = crtc_state->active_writeback;
> +
> ret = compose_planes(_out, primary_composer, cursor_composer);
Forgot to mention earlier - with the allocation happening in the
caller, compose_planes() can take void *vaddr_out.
> if (ret) {
> - if (ret == -EINVAL)
> + if (ret == -EINVAL && !wb_pending)
> kfree(vaddr_out);
> return;
> }
> @@ -206,6 +210,14 @@ void vkms_composer_worker(struct work_struct *work)
> while (frame_start <= frame_end)
> drm_crtc_add_crc_entry(crtc, true, frame_start++, );
>
> + if (wb_pending) {
> + drm_writeback_signal_completion(>wb_connector, 0);
> + spin_lock_irq(>composer_lock);
> + crtc_state->wb_pending = false;
> + spin_unlock_irq(>composer_lock);
> + return;
> + }
> +
Just like the free() move this above the drm_crtc_add_crc_entry()
if (wb_pending) {
signal()
...
} else {
free()
}
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> index 1e8b2169d834..34dd74dc8eb0 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.c
> +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> @@ -39,6 +39,10 @@ bool enable_cursor = true;
> module_param_named(enable_cursor, enable_cursor, bool, 0444);
> MODULE_PARM_DESC(enable_cursor, "Enable/Disable cursor support");
>
> +bool enable_writeback;
> +module_param_named(enable_writeback, enable_writeback, bool, 0444);
> +MODULE_PARM_DESC(enable_writeback, "Enable/Disable writeback connector");
> +
Why is this a module parameter? Let's always enable it and leave it to
userspace whether to use the wb or not.
> static const struct file_operations vkms_driver_fops = {
> .owner = THIS_MODULE,
> .open = drm_open,
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> index f4036bb0b9a8..35f0b71413c9 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.h
> +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> @@ -8,6 +8,7 @@
> #include
> #include
> #include
> +#include
>
> #define XRES_MIN20
> #define YRES_MIN20
> @@ -19,6 +20,7 @@
> #define YRES_MAX 8192
>
> extern bool enable_cursor;
> +extern bool enable_writeback;
>
> struct vkms_composer {
> struct drm_framebuffer fb;
> @@ -52,9 +54,11 @@ struct vkms_crtc_state {
> int num_active_planes;
> /* stack of active planes for crc computation, should be in z order */
> struct vkms_plane_state **active_planes;
> + void *active_writeback;
>
> /* below three are protected by vkms_output.composer_lock */
Nit: s/below three/the following four/
> bool crc_pending;
> + bool wb_pending;
> u64 frame_start;
> u64 frame_end;
> };
> @@ -63,6 +67,7 @@ struct vkms_output {
> struct drm_crtc crtc;
> struct drm_encoder encoder;
> struct drm_connector connector;
> + struct drm_writeback_connector wb_connector;
> struct hrtimer vblank_hrtimer;
> ktime_t period_ns;
> struct drm_pending_vblank_event *event;
> @@ -144,4 +149,7 @@ int vkms_verify_crc_source(struct drm_crtc *crtc, const
> char *source_name,
> /* Composer Support */
> void vkms_composer_worker(struct work_struct *work);
>
> +/* Writeback */
> +int enable_writeback_connector(struct vkms_device *vkmsdev);
Don't forget appropriate name spacing - prefix the function with vkms.
> +
> #endif /* _VKMS_DRV_H_ */
> diff --git a/drivers/gpu/drm/vkms/vkms_output.c
> b/drivers/gpu/drm/vkms/vkms_output.c
> index 85afb77e97f0..19ffc67affec 100644
> --- a/drivers/gpu/drm/vkms/vkms_output.c
> +++ b/drivers/gpu/drm/vkms/vkms_output.c
> @@ -80,6 +80,16 @@ int vkms_output_init(struct vkms_device *vkmsdev, int
> index)
> goto err_attach;
> }
>
> + if (enable_writeback) {
> + ret = enable_writeback_connector(vkmsdev);
With wb connector always enabled, you can kill off the
vkms_output::composer_enabled all together. Plus the info/error
warnings (below) can go as well.
> + if (!ret) {
> + output->composer_enabled = true;
> + DRM_INFO("Writeback connector enabled");
> + } else {
> + DRM_ERROR("Failed to init writeback connector\n");
> + }
>
Re: [PATCH resend 0/2] dts: keystone-k2g-evm: Display support
Hi Santosh, On 14/02/2020 19:40, santosh.shilim...@oracle.com wrote: On 2/14/20 1:22 AM, Jyri Sarha wrote: Resend because the earlier recipient list was wrong. Now that drm/tidss is queued for mainline, lets add display support for k2g-evm. There is no hurry since tidss is out only in v5.7, but it should not harm to have the dts changes in place before that. Jyri Sarha (2): ARM: dts: keystone-k2g: Add DSS node ARM: dts: keystone-k2g-evm: add HDMI video support arch/arm/boot/dts/keystone-k2g-evm.dts | 101 + arch/arm/boot/dts/keystone-k2g.dtsi | 22 ++ 2 files changed, 123 insertions(+) Ok. Will add this to the next queue. What happened with this one? It used to be in linux-next, but now I don't see it anymore. Tomi -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 2/2] video: fbdev: amifb: add FIXMEs about {put,get}_user() failures
On Tue, Jun 2, 2020 at 1:52 PM Bartlomiej Zolnierkiewicz
wrote:
> Since we lack the hardware (or proper emulator setup) for
> testing needed changes add FIXMEs to document the issues
> (so at least they are not forgotten).
>
> Cc: Al Viro
> Cc: Geert Uytterhoeven
> Signed-off-by: Bartlomiej Zolnierkiewicz
Reviewed-by: Geert Uytterhoeven
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 1/2] video: fbdev: amifb: add FIXME about dead APUS support
On Tue, Jun 2, 2020 at 1:50 PM Bartlomiej Zolnierkiewicz
wrote:
> On 5/14/20 10:21 PM, Geert Uytterhoeven wrote:
> > These #ifdefs are relics from APUS (Amiga Power-Up System), which
> > added a PPC board. APUS support was killed off a long time ago,
> > when arch/ppc/ was still king, but these #ifdefs were missed, because
> > they didn't test for CONFIG_APUS.
>
> Add FIXME about using the C code variants (APUS ones) in the future.
>
> Reported-by: Al Viro
> Reported-by: Geert Uytterhoeven
> Signed-off-by: Bartlomiej Zolnierkiewicz
Reviewed-by: Geert Uytterhoeven
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 2/2] video: fbdev: amifb: add FIXMEs about {put,get}_user() failures
Since we lack the hardware (or proper emulator setup) for
testing needed changes add FIXMEs to document the issues
(so at least they are not forgotten).
Cc: Al Viro
Cc: Geert Uytterhoeven
Signed-off-by: Bartlomiej Zolnierkiewicz
---
v2:
- rebased on top of updated patch #1/2
drivers/video/fbdev/amifb.c |2 ++
1 file changed, 2 insertions(+)
Index: b/drivers/video/fbdev/amifb.c
===
--- a/drivers/video/fbdev/amifb.c
+++ b/drivers/video/fbdev/amifb.c
@@ -1892,6 +1892,7 @@ static int ami_get_var_cursorinfo(struct
| ((datawords >> 15) & 1));
datawords <<= 1;
#endif
+ /* FIXME: check the return value + test the change */
put_user(color, data++);
}
if (bits > 0) {
@@ -1962,6 +1963,7 @@ static int ami_set_var_cursorinfo(struct
bits = 16; words = delta; datawords = 0;
for (width = (short)var->width - 1; width >= 0; width--) {
unsigned long tdata = 0;
+ /* FIXME: check the return value + test the change */
get_user(tdata, data);
data++;
#ifdef __mc68000__
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 1/2] video: fbdev: amifb: add FIXME about dead APUS support
On 5/14/20 10:21 PM, Geert Uytterhoeven wrote:
> These #ifdefs are relics from APUS (Amiga Power-Up System), which
> added a PPC board. APUS support was killed off a long time ago,
> when arch/ppc/ was still king, but these #ifdefs were missed, because
> they didn't test for CONFIG_APUS.
Add FIXME about using the C code variants (APUS ones) in the future.
Reported-by: Al Viro
Reported-by: Geert Uytterhoeven
Signed-off-by: Bartlomiej Zolnierkiewicz
---
v2:
- added FIXME comment instead of removing the C code variants
drivers/video/fbdev/amifb.c |6 ++
1 file changed, 6 insertions(+)
Index: b/drivers/video/fbdev/amifb.c
===
--- a/drivers/video/fbdev/amifb.c
+++ b/drivers/video/fbdev/amifb.c
@@ -575,6 +575,12 @@ static u_short maxfmode, chipset;
#define downx(x, v)((v) & -(x))
#define modx(x, v) ((v) & ((x) - 1))
+/*
+ * FIXME: Use C variants of the code marked with #ifdef __mc68000__
+ * in the driver. It shouldn't negatively affect the performance and
+ * is required for APUS support (once it is re-added to the kernel).
+ * Needs to be tested on the hardware though..
+ */
/* if x1 is not a constant, this macro won't make real sense :-) */
#ifdef __mc68000__
#define DIVUL(x1, x2) ({int res; asm("divul %1,%2,%3": "=d" (res): \
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 4/6] drm: Add Generic USB Display driver
Den 02.06.2020 04.32, skrev Alan Stern: > On Tue, Jun 02, 2020 at 12:12:07AM +, Peter Stuge wrote: > > ... > >> The way I read composite_setup() after try_fun_setup: it calls f->setup() >> when available, and that can return < 0 to stall. >> >> I expect that composite_setup() and thus f->setup() run when the >> SETUP packet has arrived, thus before the data packet arrives, and if >> composite_setup() stalls then the device/function should never see the >> data packet. >> >> For an OUT transaction I think the host controller might still send >> the DATA packet, but the device controllers that I know don't make it >> visible to the application in that case. > > ... > > Are you guys interested in comments from other people who know more > about the kernel and how it works with USB? Absolutely, I want something thats works well in the kernel and doesn't look odd to kernel USB people. > Your messages have been > far too long to go into in any detail, but I will address this one issue. > > The USB protocol forbids a device from sending a STALL response to a > SETUP packet. The only valid response is ACK. Thus, there is no way > to prevent the host from sending its DATA packet for a control-OUT > transfer. > > A gadget driver can STALL in response to a control-OUT data packet, > but only before it has seen the packet. Once the driver knows what > the data packet contains, the gadget API doesn't provide any way to > STALL the status stage. There has been a proposal to change the API > to make this possible, but so far it hasn't gone forward. > This confirms what I have seen in the kernel and the reason I added a status request so I can know the result of the operation the device has performed. I have a problem that I've encountered with this status request. In my first version the gadget would usb_ep_queue() the status value when the operation was done and as long as this happended within the host timeout (5s) everything worked fine. Then I hit a 10s timeout in the gadget when performing a display modeset operation (wait on missing vblank). The result of this was that the host timed out and moved on. The gadget however didn't know that the host gave up, so it queued up the status value. The result of this was that all further requests from the host would time out. Do you know a solution to this? My work around is to just poll on the status request, which returns a value immediately, until there's a result. The udc driver I use is dwc2. Noralf. > Alan Stern > ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH V4 2/3] drm/vkms: Compute CRC without change input data
On Tue, 12 May 2020 at 12:34, Emil Velikov wrote:
>
> Hi Rodrigo,
>
> On Mon, 11 May 2020 at 12:55, Rodrigo Siqueira
> wrote:
> >
> > From: Rodrigo Siqueira
> >
> > The compute_crc() function is responsible for calculating the
> > framebuffer CRC value; due to the XRGB format, this function has to
> > ignore the alpha channel during the CRC computation. Therefore,
> > compute_crc() set zero to the alpha channel directly in the input
> > framebuffer, which is not a problem since this function receives a copy
> > of the original buffer. However, if we want to use this function in a
> > context without a buffer copy, it will change the initial value. This
> > patch makes compute_crc() calculate the CRC value without modifying the
> > input framebuffer.
> >
> > Signed-off-by: Rodrigo Siqueira
> > ---
> > drivers/gpu/drm/vkms/vkms_composer.c | 31 +---
> > 1 file changed, 19 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/vkms/vkms_composer.c
> > b/drivers/gpu/drm/vkms/vkms_composer.c
> > index 258e659ecfba..686d25e7b01d 100644
> > --- a/drivers/gpu/drm/vkms/vkms_composer.c
> > +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> > @@ -9,33 +9,40 @@
> >
> > #include "vkms_drv.h"
> >
> > +static u32 get_pixel_from_buffer(int x, int y, const u8 *buffer,
> > +const struct vkms_composer *composer)
> > +{
> > + int src_offset = composer->offset + (y * composer->pitch)
> > + + (x * composer->cpp);
> > +
> > + return *(u32 *)[src_offset];
> > +}
> > +
> > /**
> > * compute_crc - Compute CRC value on output frame
> > *
> > - * @vaddr_out: address to final framebuffer
> > + * @vaddr: address to final framebuffer
> > * @composer: framebuffer's metadata
> > *
> > * returns CRC value computed using crc32 on the visible portion of
> > * the final framebuffer at vaddr_out
> > */
> > -static uint32_t compute_crc(void *vaddr_out, struct vkms_composer
> > *composer)
> > +static uint32_t compute_crc(const u8 *vaddr,
> > + const struct vkms_composer *composer)
> > {
> > - int i, j, src_offset;
> > + int x, y;
> > int x_src = composer->src.x1 >> 16;
> > int y_src = composer->src.y1 >> 16;
> > int h_src = drm_rect_height(>src) >> 16;
> > int w_src = drm_rect_width(>src) >> 16;
> > - u32 crc = 0;
> > + u32 crc = 0, pixel = 0;
> >
> > - for (i = y_src; i < y_src + h_src; ++i) {
> > - for (j = x_src; j < x_src + w_src; ++j) {
> > - src_offset = composer->offset
> > -+ (i * composer->pitch)
> > -+ (j * composer->cpp);
> > + for (y = y_src; y < y_src + h_src; ++y) {
> > + for (x = x_src; x < x_src + w_src; ++x) {
> > /* XRGB format ignores Alpha channel */
> > - memset(vaddr_out + src_offset + 24, 0, 8);
> > - crc = crc32_le(crc, vaddr_out + src_offset,
> > - sizeof(u32));
> > + pixel = get_pixel_from_buffer(x, y, vaddr,
> > composer);
> > + bitmap_clear((void *), 0, 8);
> > + crc = crc32_le(crc, (void *), sizeof(u32));
> > }
> > }
> >
> IMHO using something like the following makes the code far simpler and
> clearer.
>
> offset = composer->offset + (y_src * composer->pitch) + (x_src *
> composer->cpp);
>
> for (i = 0; i < h_src; i++, offset += composer->pitch) {
>for (j = 0; j < w_src; j++, offset += composer->cpp) {
> pixel = get_pixel_from_buffer(vaddr, offset);
> crc = crc32_le(crc, , sizeof(u32); // cast should not be needed
>}
> }
>
> With the bitmap_clear() and related comment moved into
> get_pixel_from_buffer().
>
If you fold the bitmap_clear() in get_pixel_from_buffer(), and drop
the cast (unless I'm missing something and it's really needed) for
crc32_le() this patch is:
Reviewed-by: Emil Velikov
I would suggest (but it's not a requirement) that you simplify the
loop/offset calculation as separate patch in v5.
-Emil
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/2] video: fbdev: amifb: remove dead APUS support
On 6/2/20 1:07 PM, John Paul Adrian Glaubitz wrote: > Hi! > > On 6/2/20 1:04 PM, Geert Uytterhoeven wrote: >>> What do you mean with the sentence "when arch/ppc/ was still king"? >> >> Ah, Bartl copied that from my email ;-) >> >> There used to be APUS support under arch/ppc/. >> Later, 32-bit arch/ppc/ and 64-bit arch/ppc64/ were merged in a new\ >> architecture port under arch/powerpc/, and the old ones were dropped. >> APUS was never converted, and thus dropped. > > Ah, yes. Similar to the merge with x86. > >>> Does that mean - in the case we would re-add APUS support in the future, >>> that >>> these particular changes would not be necessary? >> >> They would still be necessary, as PowerPC doesn't grok m68k instructions. >> Alternatively, we could just drop the m68k inline asm, and retain the C >> version instead? I have no idea how big of a difference that would make >> on m68k, using a more modern compiler than when the code was written >> originally. > > Hmm, no idea. I would keep the assembly for the time being. This was just > a question out of curiosity. We could still consider such a change if > someone should consider working on APUS support again. > >> Note that all of this is used only for cursor handling, which I doubt is >> actually used by any user space application. The only exception is the >> DIVUL() macro, which is used once during initialization, thus also not >> performance critical. > I see, thanks. Since the code in question is not performance critical it indeed seems to be good idea to use C version. However it still would need be tested on the hardware (or emulator at least) so for the time being I think that we should just add another FIXME comment instead of doing real code changes.. Best regards, -- Bartlomiej Zolnierkiewicz Samsung R Institute Poland Samsung Electronics ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH V4 1/3] drm/vkms: Decouple crc operations from composer
Hi Rodrigo,
On Mon, 11 May 2020 at 12:55, Rodrigo Siqueira wrote:
> -static uint32_t _vkms_get_crc(struct vkms_composer *primary_composer,
> - struct vkms_composer *cursor_composer)
> +static int compose_planes(void **vaddr_out,
> + struct vkms_composer *primary_composer,
> + struct vkms_composer *cursor_composer)
> {
> struct drm_framebuffer *fb = _composer->fb;
> struct drm_gem_object *gem_obj = drm_gem_fb_get_obj(fb, 0);
> struct vkms_gem_object *vkms_obj = drm_gem_to_vkms_gem(gem_obj);
> - void *vaddr_out = kzalloc(vkms_obj->gem.size, GFP_KERNEL);
> - u32 crc = 0;
>
> - if (!vaddr_out) {
> - DRM_ERROR("Failed to allocate memory for output frame.");
> - return 0;
> + if (!*vaddr_out) {
> + *vaddr_out = kzalloc(vkms_obj->gem.size, GFP_KERNEL);
It would be clearer if you keep the to alloc (or not for wb) in the
caller. As-is it's very subtle and error prone.
> + if (!*vaddr_out) {
> + DRM_ERROR("Cannot allocate memory for output frame.");
> + return -ENOMEM;
> + }
> }
>
> - if (WARN_ON(!vkms_obj->vaddr)) {
> - kfree(vaddr_out);
> - return crc;
> - }
> + if (WARN_ON(!vkms_obj->vaddr))
> + return -EINVAL;
>
> - memcpy(vaddr_out, vkms_obj->vaddr, vkms_obj->gem.size);
> + memcpy(*vaddr_out, vkms_obj->vaddr, vkms_obj->gem.size);
>
> if (cursor_composer)
> - compose_cursor(cursor_composer, primary_composer, vaddr_out);
> + compose_cursor(cursor_composer, primary_composer, *vaddr_out);
>
> - crc = compute_crc(vaddr_out, primary_composer);
> -
> - kfree(vaddr_out);
> -
> - return crc;
> + return 0;
> }
>
> /**
> @@ -157,9 +153,11 @@ void vkms_composer_worker(struct work_struct *work)
> struct vkms_output *out = drm_crtc_to_vkms_output(crtc);
> struct vkms_composer *primary_composer = NULL;
> struct vkms_composer *cursor_composer = NULL;
> + void *vaddr_out = NULL;
> u32 crc32 = 0;
> u64 frame_start, frame_end;
> bool crc_pending;
> + int ret;
>
> spin_lock_irq(>composer_lock);
> frame_start = crtc_state->frame_start;
> @@ -183,14 +181,25 @@ void vkms_composer_worker(struct work_struct *work)
> if (crtc_state->num_active_planes == 2)
> cursor_composer = crtc_state->active_planes[1]->composer;
>
> - if (primary_composer)
> - crc32 = _vkms_get_crc(primary_composer, cursor_composer);
> + if (!primary_composer)
> + return;
> +
This early return changes the functionality. Namely the
drm_crtc_add_crc_entry( 0) is now missing. I don't recall much
about the crc to judge if that's a genuine bugfix, or newly introduced
bug.
In the former case, it should be a separate patch.
> + ret = compose_planes(_out, primary_composer, cursor_composer);
> + if (ret) {
> + if (ret == -EINVAL)
> + kfree(vaddr_out);
> + return;
> + }
> +
> + crc32 = compute_crc(vaddr_out, primary_composer);
>
> /*
> * The worker can fall behind the vblank hrtimer, make sure we catch
> up.
> */
> while (frame_start <= frame_end)
> drm_crtc_add_crc_entry(crtc, true, frame_start++, );
> +
> + kfree(vaddr_out);
Nit: move the free() just after compute_crc() - it's not needed for
the drm_crtc_add_crc_entry().
-Emil
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/2] video: fbdev: amifb: remove dead APUS support
Hi Adrian,
On Tue, Jun 2, 2020 at 12:41 PM John Paul Adrian Glaubitz
wrote:
> On 6/2/20 12:37 PM, Bartlomiej Zolnierkiewicz wrote:
> >> These #ifdefs are relics from APUS (Amiga Power-Up System), which
> >> added a PPC board. APUS support was killed off a long time ago,
> >> when arch/ppc/ was still king, but these #ifdefs were missed, because
> >> they didn't test for CONFIG_APUS.
> >
> > Reported-by: Al Viro
> > Reported-by: Geert Uytterhoeven
> > Signed-off-by: Bartlomiej Zolnierkiewicz
> > ---
> > drivers/video/fbdev/amifb.c | 63
> >
> > 1 file changed, 63 deletions(-)
>
> What do you mean with the sentence "when arch/ppc/ was still king"?
Ah, Bartl copied that from my email ;-)
There used to be APUS support under arch/ppc/.
Later, 32-bit arch/ppc/ and 64-bit arch/ppc64/ were merged in a new\
architecture port under arch/powerpc/, and the old ones were dropped.
APUS was never converted, and thus dropped.
> Does that mean - in the case we would re-add APUS support in the future, that
> these particular changes would not be necessary?
They would still be necessary, as PowerPC doesn't grok m68k instructions.
Alternatively, we could just drop the m68k inline asm, and retain the C
version instead? I have no idea how big of a difference that would make
on m68k, using a more modern compiler than when the code was written
originally.
Note that all of this is used only for cursor handling, which I doubt is
actually used by any user space application. The only exception is the
DIVUL() macro, which is used once during initialization, thus also not
performance critical.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 2/2] video: fbdev: amifb: add FIXMEs about {put,get}_user() failures
Since we lack the hardware (or proper emulator setup) for
testing needed changes add FIXMEs to document the issues
(so at least they are not forgotten).
Cc: Al Viro
Cc: Geert Uytterhoeven
Signed-off-by: Bartlomiej Zolnierkiewicz
---
drivers/video/fbdev/amifb.c |2 ++
1 file changed, 2 insertions(+)
Index: b/drivers/video/fbdev/amifb.c
===
--- a/drivers/video/fbdev/amifb.c
+++ b/drivers/video/fbdev/amifb.c
@@ -1866,6 +1866,7 @@ static int ami_get_var_cursorinfo(struct
"clrb %0 ; swap %1 ; lslw #1,%1 ; roxlb #1,%0 ;
"
"swap %1 ; lslw #1,%1 ; roxlb #1,%0"
: "=d" (color), "=d" (datawords) : "1"
(datawords));
+ /* FIXME: check the return value + test the change */
put_user(color, data++);
}
if (bits > 0) {
@@ -1923,6 +1924,7 @@ static int ami_set_var_cursorinfo(struct
bits = 16; words = delta; datawords = 0;
for (width = (short)var->width - 1; width >= 0; width--) {
unsigned long tdata = 0;
+ /* FIXME: check the return value + test the change */
get_user(tdata, data);
data++;
asm volatile (
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 1/2] video: fbdev: amifb: remove dead APUS support
On 5/14/20 10:21 PM, Geert Uytterhoeven wrote:
> These #ifdefs are relics from APUS (Amiga Power-Up System), which
> added a PPC board. APUS support was killed off a long time ago,
> when arch/ppc/ was still king, but these #ifdefs were missed, because
> they didn't test for CONFIG_APUS.
Reported-by: Al Viro
Reported-by: Geert Uytterhoeven
Signed-off-by: Bartlomiej Zolnierkiewicz
---
drivers/video/fbdev/amifb.c | 63
1 file changed, 63 deletions(-)
Index: b/drivers/video/fbdev/amifb.c
===
--- a/drivers/video/fbdev/amifb.c
+++ b/drivers/video/fbdev/amifb.c
@@ -576,14 +576,8 @@ static u_short maxfmode, chipset;
#define modx(x, v) ((v) & ((x) - 1))
/* if x1 is not a constant, this macro won't make real sense :-) */
-#ifdef __mc68000__
#define DIVUL(x1, x2) ({int res; asm("divul %1,%2,%3": "=d" (res): \
"d" (x2), "d" ((long)((x1) / 0x1ULL)), "0" ((long)(x1))); res;})
-#else
-/* We know a bit about the numbers, so we can do it this way */
-#define DIVUL(x1, x2) long)((unsigned long long)x1 >> 8) / x2) << 8) + \
- long)((unsigned long long)x1 >> 8) % x2) << 8) / x2))
-#endif
#define highw(x) ((u_long)(x)>>16 & 0x)
#define loww(x)((u_long)(x) & 0x)
@@ -1837,11 +1831,7 @@ static int ami_get_var_cursorinfo(struct
const struct amifb_par *par)
{
register u_short *lspr, *sspr;
-#ifdef __mc68000__
register u_long datawords asm ("d2");
-#else
- register u_long datawords;
-#endif
register short delta;
register u_char color;
short height, width, bits, words;
@@ -1868,24 +1858,14 @@ static int ami_get_var_cursorinfo(struct
for (width = (short)var->width - 1; width >= 0; width--) {
if (bits == 0) {
bits = 16; --words;
-#ifdef __mc68000__
asm volatile ("movew %1@(%3:w:2),%0 ; swap %0 ;
movew %1@+,%0"
: "=d" (datawords), "=a" (lspr) : "1"
(lspr), "d" (delta));
-#else
- datawords = (*(lspr + delta) << 16) | (*lspr++);
-#endif
}
--bits;
-#ifdef __mc68000__
asm volatile (
"clrb %0 ; swap %1 ; lslw #1,%1 ; roxlb #1,%0 ;
"
"swap %1 ; lslw #1,%1 ; roxlb #1,%0"
: "=d" (color), "=d" (datawords) : "1"
(datawords));
-#else
- color = (((datawords >> 30) & 2)
-| ((datawords >> 15) & 1));
- datawords <<= 1;
-#endif
put_user(color, data++);
}
if (bits > 0) {
@@ -1893,17 +1873,8 @@ static int ami_get_var_cursorinfo(struct
}
while (--words >= 0)
++lspr;
-#ifdef __mc68000__
asm volatile ("lea %0@(%4:w:2),%0 ; tstl %1 ; jeq 1f ; exg
%0,%1\n1:"
: "=a" (lspr), "=a" (sspr) : "0" (lspr), "1" (sspr),
"d" (delta));
-#else
- lspr += delta;
- if (sspr) {
- u_short *tmp = lspr;
- lspr = sspr;
- sspr = tmp;
- }
-#endif
}
return 0;
}
@@ -1912,11 +1883,7 @@ static int ami_set_var_cursorinfo(struct
u_char __user *data, struct amifb_par *par)
{
register u_short *lspr, *sspr;
-#ifdef __mc68000__
register u_long datawords asm ("d2");
-#else
- register u_long datawords;
-#endif
register short delta;
u_short fmode;
short height, width, bits, words;
@@ -1958,60 +1925,30 @@ static int ami_set_var_cursorinfo(struct
unsigned long tdata = 0;
get_user(tdata, data);
data++;
-#ifdef __mc68000__
asm volatile (
"lsrb #1,%2 ; roxlw #1,%0 ; swap %0 ; "
"lsrb #1,%2 ; roxlw #1,%0 ; swap %0"
: "=d" (datawords)
: "0" (datawords), "d" (tdata));
-#else
- datawords = ((datawords << 1) & 0xfffefffe);
- datawords |= tdata & 1;
- datawords |= (tdata & 2) << (16 - 1);
-#endif
if (--bits == 0) {
bits = 16; --words;
-#ifdef __mc68000__
asm volatile ("swap %2 ; movew %2,%0@(%3:w:2) ;
swap %2 ; movew %2,%0@+"
: "=a" (lspr) : "0" (lspr), "d"
(datawords), "d" (delta));
-#else
- *(lspr + delta) = (u_short)
Re: Re: [PATCH] drm/nouveau/clk/gm20b: Fix memory leak in gm20b_clk_new()
The original patch was basically fine. Just add a Fixes tag and resend. regards, dan carpenter ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v6] drm/ioctl: Add a ioctl to set and get a label on GEM objects
Hi Rohan,
url:
https://github.com/0day-ci/linux/commits/Rohan-Garg/drm-ioctl-Add-a-ioctl-to-set-and-get-a-label-on-GEM-objects/20200531-000134
base: https://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git
exynos-drm-next
config: i386-randconfig-m021-20200531 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-13) 9.3.0
If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot
Reported-by: Dan Carpenter
New smatch warnings:
drivers/gpu/drm/drm_gem.c:1004 drm_gem_get_label() warn: maybe return -EFAULT
instead of the bytes remaining?
Old smatch warnings:
drivers/gpu/drm/drm_gem.c:910 drm_gem_open_ioctl() warn: inconsistent returns
'dev->object_name_lock'.
#
https://github.com/0day-ci/linux/commit/174b10d2bdba06efe773aa0d09e682a57a00ec67
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout 174b10d2bdba06efe773aa0d09e682a57a00ec67
vim +1004 drivers/gpu/drm/drm_gem.c
174b10d2bdba06 Rohan Garg 2020-05-28 979 int drm_gem_get_label(struct
drm_device *dev, struct drm_file *file_priv,
174b10d2bdba06 Rohan Garg 2020-05-28 980struct
drm_handle_label *args)
174b10d2bdba06 Rohan Garg 2020-05-28 981 {
174b10d2bdba06 Rohan Garg 2020-05-28 982 struct drm_gem_object *gem_obj;
174b10d2bdba06 Rohan Garg 2020-05-28 983 int len, ret;
174b10d2bdba06 Rohan Garg 2020-05-28 984
174b10d2bdba06 Rohan Garg 2020-05-28 985 gem_obj =
drm_gem_object_lookup(file_priv, args->handle);
174b10d2bdba06 Rohan Garg 2020-05-28 986 if (!gem_obj) {
174b10d2bdba06 Rohan Garg 2020-05-28 987 DRM_DEBUG("Failed to
look up GEM BO %d\n", args->handle);
174b10d2bdba06 Rohan Garg 2020-05-28 988 return -ENOENT;
174b10d2bdba06 Rohan Garg 2020-05-28 989 }
174b10d2bdba06 Rohan Garg 2020-05-28 990
174b10d2bdba06 Rohan Garg 2020-05-28 991 if (!gem_obj->label) {
174b10d2bdba06 Rohan Garg 2020-05-28 992 args->label = NULL;
174b10d2bdba06 Rohan Garg 2020-05-28 993 args->len = 0;
174b10d2bdba06 Rohan Garg 2020-05-28 994 return 0;
174b10d2bdba06 Rohan Garg 2020-05-28 995 }
174b10d2bdba06 Rohan Garg 2020-05-28 996
174b10d2bdba06 Rohan Garg 2020-05-28 997 mutex_lock(_obj->bo_lock);
174b10d2bdba06 Rohan Garg 2020-05-28 998 len = strlen(gem_obj->label);
174b10d2bdba06 Rohan Garg 2020-05-28 999 ret =
copy_to_user(u64_to_user_ptr(args->label), gem_obj->label,
174b10d2bdba06 Rohan Garg 2020-05-28 1000
min(args->len, len));
copy_to_user() returns the number of bytes remaining to be copied but
this should be:
if (copy_to_user(u64_to_user_ptr(args->label), gem_obj->label,
min(args->len, len)))
ret = -EFAULT;
Don't forget to initialize "int ret = 0;" because GCC doesn't warn about
it these days... :/
174b10d2bdba06 Rohan Garg 2020-05-28 1001 mutex_unlock(_obj->bo_lock);
174b10d2bdba06 Rohan Garg 2020-05-28 1002 args->len = len;
174b10d2bdba06 Rohan Garg 2020-05-28 1003 drm_gem_object_put(gem_obj);
174b10d2bdba06 Rohan Garg 2020-05-28 @1004 return ret;
174b10d2bdba06 Rohan Garg 2020-05-28 1005 }
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org
.config.gz
Description: application/gzip
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 07/15] drm/amdgpu: use PCI_IRQ_MSI_TYPES where appropriate
On Tue, Jun 2, 2020 at 12:58 PM Stankiewicz, Piotr
wrote:
> > -Original Message-
> > From: Andy Shevchenko
> > Sent: Tuesday, June 2, 2020 11:49 AM
> > To: Stankiewicz, Piotr
> > Cc: Alex Deucher ; Christian König
> > ; David Zhou ; David
> > Airlie ; Daniel Vetter ; amd-
> > g...@lists.freedesktop.org; dri-devel ;
> > Linux
> > Kernel Mailing List
> > Subject: Re: [PATCH 07/15] drm/amdgpu: use PCI_IRQ_MSI_TYPES where
> > appropriate
> > On Tue, Jun 2, 2020 at 12:24 PM Piotr Stankiewicz
> > wrote:
...
> > > int nvec = pci_msix_vec_count(adev->pdev);
> > > unsigned int flags;
> > >
> > > - if (nvec <= 0) {
> > > + if (nvec > 0)
> > > + flags = PCI_IRQ_MSI_TYPES;
> > > + else
> > > flags = PCI_IRQ_MSI;
> > > - } else {
> > > - flags = PCI_IRQ_MSI | PCI_IRQ_MSIX;
> > > - }
> > > +
> > > /* we only need one vector */
> > > nvec = pci_alloc_irq_vectors(adev->pdev, 1, 1, flags);
> >
> > I'm not sure if you have seen my last comment internally about this patch.
> >
> > I don't understand why we need these pci_msix_vec_count() followed by
> > conditional at all.
> > Perhaps we may simple drop all these and supply flag directly?
> >
> > But OTOH, I don't know the initial motivation, so, the above patch is
> > non-intrusive and keeps original logic.
> >
>
> Sorry, I must have misunderstood or missed that comment. I am happy
> to do a V2 if dropping the conditional is preferable.
Let's wait for AMD people to confirm either.
--
With Best Regards,
Andy Shevchenko
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
RE: [PATCH 07/15] drm/amdgpu: use PCI_IRQ_MSI_TYPES where appropriate
> -Original Message-
> From: Andy Shevchenko
> Sent: Tuesday, June 2, 2020 11:49 AM
> To: Stankiewicz, Piotr
> Cc: Alex Deucher ; Christian König
> ; David Zhou ; David
> Airlie ; Daniel Vetter ; amd-
> g...@lists.freedesktop.org; dri-devel ; Linux
> Kernel Mailing List
> Subject: Re: [PATCH 07/15] drm/amdgpu: use PCI_IRQ_MSI_TYPES where
> appropriate
>
> On Tue, Jun 2, 2020 at 12:24 PM Piotr Stankiewicz
> wrote:
> >
> > Seeing as there is shorthand available to use when asking for any type
> > of interrupt, or any type of message signalled interrupt, leverage it.
> >
> > Signed-off-by: Piotr Stankiewicz
> > Reviewed-by: Andy Shevchenko
> > ---
> > drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c | 8
> > 1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
> > index 5ed4227f304b..6dbe173a9fd4 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
> > @@ -251,11 +251,11 @@ int amdgpu_irq_init(struct amdgpu_device *adev)
> > int nvec = pci_msix_vec_count(adev->pdev);
> > unsigned int flags;
> >
> > - if (nvec <= 0) {
> > + if (nvec > 0)
> > + flags = PCI_IRQ_MSI_TYPES;
> > + else
> > flags = PCI_IRQ_MSI;
> > - } else {
> > - flags = PCI_IRQ_MSI | PCI_IRQ_MSIX;
> > - }
> > +
> > /* we only need one vector */
> > nvec = pci_alloc_irq_vectors(adev->pdev, 1, 1, flags);
>
> I'm not sure if you have seen my last comment internally about this patch.
>
> I don't understand why we need these pci_msix_vec_count() followed by
> conditional at all.
> Perhaps we may simple drop all these and supply flag directly?
>
> But OTOH, I don't know the initial motivation, so, the above patch is
> non-intrusive and keeps original logic.
>
Sorry, I must have misunderstood or missed that comment. I am happy
to do a V2 if dropping the conditional is preferable.
> > if (nvec > 0) {
> > --
> > 2.17.2
> >
>
>
> --
> With Best Regards,
> Andy Shevchenko
BR,
Piotr Stankiewicz
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm: document how user-space should use link-status
On Tue, Jun 02, 2020 at 10:38:46AM +0300, Pekka Paalanen wrote:
> On Mon, 01 Jun 2020 14:48:58 +
> Simon Ser wrote:
>
> > Describe what a "BAD" link-status means for user-space and how it should
> > handle it. The logic described has been implemented in igt [1].
> >
> > [1]:
> > https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/commit/fbe61f529737191d0920521946a575bd55f00fbe
> >
> > Signed-off-by: Simon Ser
> > Cc: Daniel Vetter
> > Cc: Manasi Navare
> > Cc: Pekka Paalanen
> > ---
> > drivers/gpu/drm/drm_connector.c | 6 ++
> > 1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_connector.c
> > b/drivers/gpu/drm/drm_connector.c
> > index f2b20fd66319..08ba84f9787a 100644
> > --- a/drivers/gpu/drm/drm_connector.c
> > +++ b/drivers/gpu/drm/drm_connector.c
> > @@ -994,6 +994,12 @@ static const struct drm_prop_enum_list
> > dp_colorspaces[] = {
> > * after modeset, the kernel driver may set this to "BAD" and issue a
> > * hotplug uevent. Drivers should update this value using
> > * drm_connector_set_link_status_property().
> > + *
> > + * When user-space receives the hotplug uevent and detects a "BAD"
> > + * link-status, the connector is no longer enabled. The list of
> > available
"no longer enabled" is kinda wrong, you can keep doing pageflip. It's just
that the pixels aren't getting to the screen anymore.
"enabled" wrt an output has a different meaning in kms, the internal
drm_crtc_state->enabled state is very much still set. Including what that
all means for the uapi.
Also I think we need some words here on what automatically happens when
you do an atomic commit with the connector with the bad link status
(auto-reset to good, which might make the atomic modeset fail). On irc we
had some discussions that we should only do this if ALLOW_MODESET is set,
but that's atm not the case.
-Daniel
> > + * modes may have changed. User-space is expected to pick a new mode
> > if
> > + * the current one has disappeared and perform a new modeset with
> > + * link-status set to "GOOD" to re-enable the connector.
> > * non_desktop:
> > * Indicates the output should be ignored for purposes of
> > displaying a
> > * standard desktop environment or console. This is most likely
> > because
>
> Hi,
>
> makes sense to me. Can it happen that there will be no modes left in
> the list?
>
> What if userspace is driving two connectors from the same CRTC, and only
> one connector gets link-status bad, what does it mean? Is the other
> connector still running as normal, as if the failed connector didn't
> even exist?
>
> That is mostly a question about what happens if userspace does not fix
> up the link-status=bad connector and does not detach it from the CRTC,
> but keeps on flipping or modesetting as if the failure never happened.
> I guess I could ask it about both a CRTC that has another connector
> still good, and a CRTC where the failed connector was the only one.
>
> Can I trust that if the other connector is in any way affected, it too
> will get link-status bad?
>
>
> Thanks,
> pq
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/malidp: Don't call drm_crtc_vblank_off on unbind
This is already done as part of the drm_atomic_helper_shutdown(), and in that case only for the crtc which are actually on. v2: I overlooked that malidp also needs to have it's interrupt shut down reordered. Signed-off-by: Daniel Vetter Cc: Liviu Dudau Cc: Brian Starkey --- drivers/gpu/drm/arm/malidp_drv.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c index 02904392e370..cdb817a7c611 100644 --- a/drivers/gpu/drm/arm/malidp_drv.c +++ b/drivers/gpu/drm/arm/malidp_drv.c @@ -928,11 +928,10 @@ static void malidp_unbind(struct device *dev) drm_dev_unregister(drm); drm_kms_helper_poll_fini(drm); pm_runtime_get_sync(dev); - drm_crtc_vblank_off(>crtc); + drm_atomic_helper_shutdown(drm); malidp_se_irq_fini(hwdev); malidp_de_irq_fini(hwdev); drm->irq_enabled = false; - drm_atomic_helper_shutdown(drm); component_unbind_all(dev, drm); of_node_put(malidp->crtc.port); malidp->crtc.port = NULL; -- 2.26.2 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 3/3] drm/hdlcd: Don't call drm_crtc_vblank_off on unbind
This is already taken care of by drm_atomic_helper_shutdown(), and in that case only for the CRTC which are actually on. Only tricky bit here is that we kill the interrupt handling before we shut down crtc, so need to reorder that. Signed-off-by: Daniel Vetter Cc: Liviu Dudau Cc: Brian Starkey Cc: --- drivers/gpu/drm/arm/hdlcd_drv.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/gpu/drm/arm/hdlcd_drv.c b/drivers/gpu/drm/arm/hdlcd_drv.c index 194419f47c5e..26bc5d7766f5 100644 --- a/drivers/gpu/drm/arm/hdlcd_drv.c +++ b/drivers/gpu/drm/arm/hdlcd_drv.c @@ -347,9 +347,8 @@ static void hdlcd_drm_unbind(struct device *dev) of_node_put(hdlcd->crtc.port); hdlcd->crtc.port = NULL; pm_runtime_get_sync(dev); - drm_crtc_vblank_off(>crtc); - drm_irq_uninstall(drm); drm_atomic_helper_shutdown(drm); + drm_irq_uninstall(drm); pm_runtime_put(dev); if (pm_runtime_enabled(dev)) pm_runtime_disable(dev); -- 2.26.2 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 2/3] drm/malidp: Don't call drm_crtc_vblank_off on unbind
This is already done as part of the drm_atomic_helper_shutdown(), and in that case only for the crtc which are actually on. Signed-off-by: Daniel Vetter Cc: Liviu Dudau Cc: Brian Starkey Cc: --- drivers/gpu/drm/arm/malidp_drv.c | 1 - 1 file changed, 1 deletion(-) diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c index 02904392e370..db6ba5c78042 100644 --- a/drivers/gpu/drm/arm/malidp_drv.c +++ b/drivers/gpu/drm/arm/malidp_drv.c @@ -928,7 +928,6 @@ static void malidp_unbind(struct device *dev) drm_dev_unregister(drm); drm_kms_helper_poll_fini(drm); pm_runtime_get_sync(dev); - drm_crtc_vblank_off(>crtc); malidp_se_irq_fini(hwdev); malidp_de_irq_fini(hwdev); drm->irq_enabled = false; -- 2.26.2 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 1/3] drm/atomic-helper: reset vblank on crtc reset
Only when vblanks are supported ofc.
Some drivers do this already, but most unfortunately missed it. This
opens up bugs after driver load, before the crtc is enabled for the
first time. syzbot spotted this when loading vkms as a secondary
output. Given how many drivers are buggy it's best to solve this once
and for all in shared helper code.
Aside from moving the few existing calls to drm_crtc_vblank_reset into
helpers (i915 doesn't use helpers, so keeps its own) I think the
regression risk is minimal: atomic helpers already rely on drivers
calling drm_crtc_vblank_on/off correctly in their hooks when they
support vblanks. And driver that's failing to handle vblanks after
this is missing those calls already, and vblanks could only work by
accident when enabling a CRTC for the first time right after boot.
Big thanks to Tetsuo for helping track down what's going wrong here.
There's only a few drivers which already had the necessary call and
needed some updating:
- komeda, atmel and tidss also needed to be changed to call
__drm_atomic_helper_crtc_reset() intead of open coding it
- tegra and msm even had it in the same place already, just code
motion, and malidp already uses __drm_atomic_helper_crtc_reset().
Only call left is in i915, which doesn't use drm_mode_config_reset,
but has its own fastboot infrastructure. So that's the only case where
we actually want this in the driver still.
I've also reviewed all other drivers which set up vblank support with
drm_vblank_init. After the previous patch fixing mxsfb all atomic
drivers do call drm_crtc_vblank_on/off as they should, the remaining
drivers are either legacy kms or legacy dri1 drivers, so not affected
by this change to atomic helpers.
v2: Use the drm_dev_has_vblank() helper.
v3: Laurent pointed out that omap and rcar-du used drm_crtc_vblank_off
instead of drm_crtc_vblank_reset. Adjust them too.
Cc: Laurent Pinchart
Reviewed-by: Laurent Pinchart
Reviewed-by: Boris Brezillon
Acked-by: Liviu Dudau
Acked-by: Thierry Reding
Link:
https://syzkaller.appspot.com/bug?id=0ba17d70d062b2595e1f061231474800f076c7cb
Reported-by: Tetsuo Handa
Reported-by: syzbot+0871b14ca2e2fb64f...@syzkaller.appspotmail.com
Cc: Tetsuo Handa
Cc: "James (Qian) Wang"
Cc: Liviu Dudau
Cc: Mihail Atanassov
Cc: Brian Starkey
Cc: Sam Ravnborg
Cc: Boris Brezillon
Cc: Nicolas Ferre
Cc: Alexandre Belloni
Cc: Ludovic Desroches
Cc: Maarten Lankhorst
Cc: Maxime Ripard
Cc: Thomas Zimmermann
Cc: David Airlie
Cc: Daniel Vetter
Cc: Thierry Reding
Cc: Jonathan Hunter
Cc: Jyri Sarha
Cc: Tomi Valkeinen
Cc: Rob Clark
Cc: Sean Paul
Cc: Brian Masney
Cc: Emil Velikov
Cc: zhengbin
Cc: Thomas Gleixner
Cc: linux-te...@vger.kernel.org
Signed-off-by: Daniel Vetter
---
drivers/gpu/drm/arm/display/komeda/komeda_crtc.c | 7 ++-
drivers/gpu/drm/arm/malidp_drv.c | 1 -
drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 7 ++-
drivers/gpu/drm/drm_atomic_state_helper.c| 4
drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c| 2 --
drivers/gpu/drm/omapdrm/omap_drv.c | 3 ---
drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 3 ---
drivers/gpu/drm/tegra/dc.c | 1 -
drivers/gpu/drm/tidss/tidss_crtc.c | 3 +--
drivers/gpu/drm/tidss/tidss_kms.c| 4
10 files changed, 9 insertions(+), 26 deletions(-)
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
index 56bd938961ee..f33418d6e1a0 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
@@ -492,10 +492,8 @@ static void komeda_crtc_reset(struct drm_crtc *crtc)
crtc->state = NULL;
state = kzalloc(sizeof(*state), GFP_KERNEL);
- if (state) {
- crtc->state = >base;
- crtc->state->crtc = crtc;
- }
+ if (state)
+ __drm_atomic_helper_crtc_reset(crtc, >base);
}
static struct drm_crtc_state *
@@ -616,7 +614,6 @@ static int komeda_crtc_add(struct komeda_kms_dev *kms,
return err;
drm_crtc_helper_add(crtc, _crtc_helper_funcs);
- drm_crtc_vblank_reset(crtc);
crtc->port = kcrtc->master->of_output_port;
diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c
index c2507b7d8512..02904392e370 100644
--- a/drivers/gpu/drm/arm/malidp_drv.c
+++ b/drivers/gpu/drm/arm/malidp_drv.c
@@ -870,7 +870,6 @@ static int malidp_bind(struct device *dev)
drm->irq_enabled = true;
ret = drm_vblank_init(drm, drm->mode_config.num_crtc);
- drm_crtc_vblank_reset(>crtc);
if (ret < 0) {
DRM_ERROR("failed to initialise vblank\n");
goto vblank_fail;
diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
index 10985134ce0b..ce246b96330b 100644
---
Re: [PATCH 07/15] drm/amdgpu: use PCI_IRQ_MSI_TYPES where appropriate
On Tue, Jun 2, 2020 at 12:24 PM Piotr Stankiewicz
wrote:
>
> Seeing as there is shorthand available to use when asking for any type
> of interrupt, or any type of message signalled interrupt, leverage it.
>
> Signed-off-by: Piotr Stankiewicz
> Reviewed-by: Andy Shevchenko
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c | 8
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
> index 5ed4227f304b..6dbe173a9fd4 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
> @@ -251,11 +251,11 @@ int amdgpu_irq_init(struct amdgpu_device *adev)
> int nvec = pci_msix_vec_count(adev->pdev);
> unsigned int flags;
>
> - if (nvec <= 0) {
> + if (nvec > 0)
> + flags = PCI_IRQ_MSI_TYPES;
> + else
> flags = PCI_IRQ_MSI;
> - } else {
> - flags = PCI_IRQ_MSI | PCI_IRQ_MSIX;
> - }
> +
> /* we only need one vector */
> nvec = pci_alloc_irq_vectors(adev->pdev, 1, 1, flags);
I'm not sure if you have seen my last comment internally about this patch.
I don't understand why we need these pci_msix_vec_count() followed by
conditional at all.
Perhaps we may simple drop all these and supply flag directly?
But OTOH, I don't know the initial motivation, so, the above patch is
non-intrusive and keeps original logic.
> if (nvec > 0) {
> --
> 2.17.2
>
--
With Best Regards,
Andy Shevchenko
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC 01/17] dma-fence: add might_sleep annotation to _wait()
Op 12-05-2020 om 11:08 schreef Christian König: > Am 12.05.20 um 10:59 schrieb Daniel Vetter: >> But only for non-zero timeout, to avoid false positives. >> >> One question here is whether the might_sleep should be unconditional, >> or only for real timeouts. I'm not sure, so went with the more >> defensive option. But in the interest of locking down the cross-driver >> dma_fence rules we might want to be more aggressive. >> >> Cc: linux-me...@vger.kernel.org >> Cc: linaro-mm-...@lists.linaro.org >> Cc: linux-r...@vger.kernel.org >> Cc: amd-...@lists.freedesktop.org >> Cc: intel-...@lists.freedesktop.org >> Cc: Chris Wilson >> Cc: Maarten Lankhorst >> Cc: Christian König >> Signed-off-by: Daniel Vetter >> --- >> drivers/dma-buf/dma-fence.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c >> index 052a41e2451c..6802125349fb 100644 >> --- a/drivers/dma-buf/dma-fence.c >> +++ b/drivers/dma-buf/dma-fence.c >> @@ -208,6 +208,9 @@ dma_fence_wait_timeout(struct dma_fence *fence, bool >> intr, signed long timeout) >> if (WARN_ON(timeout < 0)) >> return -EINVAL; >> + if (timeout > 0) >> + might_sleep(); >> + > > I would rather like to see might_sleep() called here all the time even with > timeout==0. > > IIRC I removed the code in TTM abusing this in atomic context quite a while > ago, but could be that some leaked in again or it is called in atomic context > elsewhere as well. Same, glad I'm not the only one who wants it. :) ~Maarten ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/atomic-helper: reset vblank on crtc reset
On Sat, May 30, 2020 at 06:22:58AM +0300, Laurent Pinchart wrote:
> Hi Daniel,
>
> Thank you for the patch.
>
> On Wed, May 27, 2020 at 11:53:32AM +0200, Daniel Vetter wrote:
> > Only when vblanks are supported ofc.
> >
> > Some drivers do this already, but most unfortunately missed it. This
> > opens up bugs after driver load, before the crtc is enabled for the
> > first time. syzbot spotted this when loading vkms as a secondary
> > output. Given how many drivers are buggy it's best to solve this once
> > and for all in shared helper code.
> >
> > Aside from moving the few existing calls to drm_crtc_vblank_reset into
> > helpers (i915 doesn't use helpers, so keeps its own) I think the
> > regression risk is minimal: atomic helpers already rely on drivers
> > calling drm_crtc_vblank_on/off correctly in their hooks when they
> > support vblanks. And driver that's failing to handle vblanks after
> > this is missing those calls already, and vblanks could only work by
> > accident when enabling a CRTC for the first time right after boot.
> >
> > Big thanks to Tetsuo for helping track down what's going wrong here.
> >
> > There's only a few drivers which already had the necessary call and
> > needed some updating:
> > - komeda, atmel and tidss also needed to be changed to call
> > __drm_atomic_helper_crtc_reset() intead of open coding it
> > - tegra and msm even had it in the same place already, just code
> > motion, and malidp already uses __drm_atomic_helper_crtc_reset().
>
> Have you intentionally not touched drivers that use
> drm_crtc_vblank_off() at init time instead of drm_crtc_vblank_reset() ?
> I'm thinking about omapdrm and rcar-du that both call neither
> drm_crtc_vblank_reset() nor __drm_atomic_helper_crtc_reset() in their
> CRTC reset handler, and call drm_crtc_vblank_off() at init time. Should
> these (and likely other) drivers be updated ?
Good catch, I think we should remove those too with this patch.
I also used that opportunity to review all callers fo drm_crtc_vblank_off,
and I found two bogus callers in malidp and hdlcd. But only in the unload
code, so doesn't matter much.
I'll resend a new version.
-Daniel
> Other than that the patch looks good to me,
>
> Reviewed-by: Laurent Pinchart
>
> > Only call left is in i915, which doesn't use drm_mode_config_reset,
> > but has its own fastboot infrastructure. So that's the only case where
> > we actually want this in the driver still.
> >
> > I've also reviewed all other drivers which set up vblank support with
> > drm_vblank_init. After the previous patch fixing mxsfb all atomic
> > drivers do call drm_crtc_vblank_on/off as they should, the remaining
> > drivers are either legacy kms or legacy dri1 drivers, so not affected
> > by this change to atomic helpers.
> >
> > v2: Use the drm_dev_has_vblank() helper.
> >
> > Link:
> > https://syzkaller.appspot.com/bug?id=0ba17d70d062b2595e1f061231474800f076c7cb
> > Reported-by: Tetsuo Handa
> > Reported-by: syzbot+0871b14ca2e2fb64f...@syzkaller.appspotmail.com
> > Cc: Tetsuo Handa
> > Cc: "James (Qian) Wang"
> > Cc: Liviu Dudau
> > Cc: Mihail Atanassov
> > Cc: Brian Starkey
> > Cc: Sam Ravnborg
> > Cc: Boris Brezillon
> > Cc: Nicolas Ferre
> > Cc: Alexandre Belloni
> > Cc: Ludovic Desroches
> > Cc: Maarten Lankhorst
> > Cc: Maxime Ripard
> > Cc: Thomas Zimmermann
> > Cc: David Airlie
> > Cc: Daniel Vetter
> > Cc: Thierry Reding
> > Cc: Jonathan Hunter
> > Cc: Jyri Sarha
> > Cc: Tomi Valkeinen
> > Cc: Rob Clark
> > Cc: Sean Paul
> > Cc: Brian Masney
> > Cc: Emil Velikov
> > Cc: zhengbin
> > Cc: Thomas Gleixner
> > Cc: linux-te...@vger.kernel.org
> > Signed-off-by: Daniel Vetter
> > ---
> > drivers/gpu/drm/arm/display/komeda/komeda_crtc.c | 7 ++-
> > drivers/gpu/drm/arm/malidp_drv.c | 1 -
> > drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 7 ++-
> > drivers/gpu/drm/drm_atomic_state_helper.c| 4
> > drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c| 2 --
> > drivers/gpu/drm/tegra/dc.c | 1 -
> > drivers/gpu/drm/tidss/tidss_crtc.c | 3 +--
> > drivers/gpu/drm/tidss/tidss_kms.c| 4
> > 8 files changed, 9 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> > b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> > index 56bd938961ee..f33418d6e1a0 100644
> > --- a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> > @@ -492,10 +492,8 @@ static void komeda_crtc_reset(struct drm_crtc *crtc)
> > crtc->state = NULL;
> >
> > state = kzalloc(sizeof(*state), GFP_KERNEL);
> > - if (state) {
> > - crtc->state = >base;
> > - crtc->state->crtc = crtc;
> > - }
> > + if (state)
> > + __drm_atomic_helper_crtc_reset(crtc, >base);
> > }
> >
> > static struct drm_crtc_state *
> > @@ -616,7 +614,6 @@ static int
[PATCH 07/15] drm/amdgpu: use PCI_IRQ_MSI_TYPES where appropriate
Seeing as there is shorthand available to use when asking for any type
of interrupt, or any type of message signalled interrupt, leverage it.
Signed-off-by: Piotr Stankiewicz
Reviewed-by: Andy Shevchenko
---
drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
index 5ed4227f304b..6dbe173a9fd4 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
@@ -251,11 +251,11 @@ int amdgpu_irq_init(struct amdgpu_device *adev)
int nvec = pci_msix_vec_count(adev->pdev);
unsigned int flags;
- if (nvec <= 0) {
+ if (nvec > 0)
+ flags = PCI_IRQ_MSI_TYPES;
+ else
flags = PCI_IRQ_MSI;
- } else {
- flags = PCI_IRQ_MSI | PCI_IRQ_MSIX;
- }
+
/* we only need one vector */
nvec = pci_alloc_irq_vectors(adev->pdev, 1, 1, flags);
if (nvec > 0) {
--
2.17.2
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 00/15] forward MSIx vector enable error code in pci_alloc_irq_vectors_affinity
The primary objective of this patch series is to change the behaviour of pci_alloc_irq_vectors_affinity such that it forwards the MSI-X enable error code when appropriate. In the process, though, it was pointed out that there are multiple places in the kernel which check/ask for message signalled interrupts (MSI or MSI-X), which spawned the first patch adding PCI_IRQ_MSI_TYPES. Finally the rest of the chain converts all users to take advantage of PCI_IRQ_MSI_TYPES or PCI_IRQ_ALL_TYPES, as appropriate. Piotr Stankiewicz (15): PCI: add shorthand define for message signalled interrupt types PCI/MSI: forward MSIx vector enable error code in pci_alloc_irq_vectors_affinity PCI: use PCI_IRQ_MSI_TYPES where appropriate ahci: use PCI_IRQ_MSI_TYPES where appropriate crypto: inside-secure - use PCI_IRQ_MSI_TYPES where appropriate dmaengine: dw-edma: use PCI_IRQ_MSI_TYPES where appropriate drm/amdgpu: use PCI_IRQ_MSI_TYPES where appropriate IB/qib: Use PCI_IRQ_MSI_TYPES where appropriate media: ddbridge: use PCI_IRQ_MSI_TYPES where appropriate vmw_vmci: use PCI_IRQ_ALL_TYPES where appropriate mmc: sdhci: use PCI_IRQ_MSI_TYPES where appropriate amd-xgbe: use PCI_IRQ_MSI_TYPES where appropriate aquantia: atlantic: use PCI_IRQ_ALL_TYPES where appropriate net: hns3: use PCI_IRQ_MSI_TYPES where appropriate scsi: use PCI_IRQ_MSI_TYPES and PCI_IRQ_ALL_TYPES where appropriate Documentation/PCI/msi-howto.rst | 5 +++-- drivers/ata/ahci.c| 2 +- drivers/crypto/inside-secure/safexcel.c | 2 +- drivers/dma/dw-edma/dw-edma-pcie.c| 2 +- drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c | 8 drivers/infiniband/hw/qib/qib_pcie.c | 2 +- drivers/media/pci/ddbridge/ddbridge-main.c| 2 +- drivers/misc/vmw_vmci/vmci_guest.c| 3 +-- drivers/mmc/host/sdhci-pci-gli.c | 3 +-- drivers/mmc/host/sdhci-pci-o2micro.c | 3 +-- drivers/net/ethernet/amd/xgbe/xgbe-pci.c | 2 +- drivers/net/ethernet/aquantia/atlantic/aq_pci_func.c | 4 +--- drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c | 3 +-- drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c | 2 +- drivers/pci/msi.c | 4 ++-- drivers/pci/pcie/portdrv_core.c | 4 ++-- drivers/pci/switch/switchtec.c| 3 +-- drivers/scsi/ipr.c| 2 +- drivers/scsi/vmw_pvscsi.c | 2 +- include/linux/pci.h | 4 ++-- 20 files changed, 28 insertions(+), 34 deletions(-) -- 2.17.2 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC PATCH v5 2/6] interconnect: Add generic interconnect driver for Exynos SoCs
On Fri, May 29, 2020 at 06:31:56PM +0200, Sylwester Nawrocki wrote:
> This patch adds a generic interconnect driver for Exynos SoCs in order
> to provide interconnect functionality for each "samsung,exynos-bus"
> compatible device.
>
> The SoC topology is a graph (or more specifically, a tree) and its
> edges are specified using the 'samsung,interconnect-parent' in the
> DT. Due to unspecified relative probing order, -EPROBE_DEFER may be
> propagated to ensure that the parent is probed before its children.
>
> Each bus is now an interconnect provider and an interconnect node as
> well (cf. Documentation/interconnect/interconnect.rst), i.e. every bus
> registers itself as a node. Node IDs are not hardcoded but rather
> assigned dynamically at runtime. This approach allows for using this
> driver with various Exynos SoCs.
>
> Frequencies requested via the interconnect API for a given node are
> propagated to devfreq using dev_pm_qos_update_request(). Please note
> that it is not an error when CONFIG_INTERCONNECT is 'n', in which
> case all interconnect API functions are no-op.
>
> Signed-off-by: Artur Świgoń
> Signed-off-by: Sylwester Nawrocki
>
> Changes for v5:
> - adjust to renamed exynos,interconnect-parent-node property,
> - use automatically generated platform device id as the interconect
>node id instead of a now unavailable devfreq->id field,
> - add icc_ prefix to some variables to make the code more self-commenting,
> - use icc_nodes_remove() instead of icc_node_del() + icc_node_destroy(),
> - adjust to exynos,interconnect-parent-node property rename to
>samsung,interconnect-parent,
> - converted to a separate platform driver in drivers/interconnect.
> ---
> drivers/interconnect/Kconfig | 1 +
> drivers/interconnect/Makefile| 1 +
> drivers/interconnect/exynos/Kconfig | 6 ++
> drivers/interconnect/exynos/Makefile | 4 +
> drivers/interconnect/exynos/exynos.c | 185
> +++
> 5 files changed, 197 insertions(+)
> create mode 100644 drivers/interconnect/exynos/Kconfig
> create mode 100644 drivers/interconnect/exynos/Makefile
> create mode 100644 drivers/interconnect/exynos/exynos.c
>
> diff --git a/drivers/interconnect/Kconfig b/drivers/interconnect/Kconfig
> index 5b7204e..eca6eda 100644
> --- a/drivers/interconnect/Kconfig
> +++ b/drivers/interconnect/Kconfig
> @@ -11,6 +11,7 @@ menuconfig INTERCONNECT
>
> if INTERCONNECT
>
> +source "drivers/interconnect/exynos/Kconfig"
> source "drivers/interconnect/imx/Kconfig"
> source "drivers/interconnect/qcom/Kconfig"
>
> diff --git a/drivers/interconnect/Makefile b/drivers/interconnect/Makefile
> index 4825c28..2ba1de6 100644
> --- a/drivers/interconnect/Makefile
> +++ b/drivers/interconnect/Makefile
> @@ -4,5 +4,6 @@ CFLAGS_core.o := -I$(src)
> icc-core-objs:= core.o
>
> obj-$(CONFIG_INTERCONNECT) += icc-core.o
> +obj-$(CONFIG_INTERCONNECT_EXYNOS)+= exynos/
> obj-$(CONFIG_INTERCONNECT_IMX) += imx/
> obj-$(CONFIG_INTERCONNECT_QCOM) += qcom/
> diff --git a/drivers/interconnect/exynos/Kconfig
> b/drivers/interconnect/exynos/Kconfig
> new file mode 100644
> index 000..e51e52e
> --- /dev/null
> +++ b/drivers/interconnect/exynos/Kconfig
> @@ -0,0 +1,6 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +config INTERCONNECT_EXYNOS
> + tristate "Exynos generic interconnect driver"
> + depends on ARCH_EXYNOS || COMPILE_TEST
> + help
> + Generic interconnect driver for Exynos SoCs.
> diff --git a/drivers/interconnect/exynos/Makefile
> b/drivers/interconnect/exynos/Makefile
> new file mode 100644
> index 000..e19d1df
> --- /dev/null
> +++ b/drivers/interconnect/exynos/Makefile
> @@ -0,0 +1,4 @@
> +# SPDX-License-Identifier: GPL-2.0
> +exynos-interconnect-objs := exynos.o
> +
> +obj-$(CONFIG_INTERCONNECT_EXYNOS)+= exynos-interconnect.o
> diff --git a/drivers/interconnect/exynos/exynos.c
> b/drivers/interconnect/exynos/exynos.c
> new file mode 100644
> index 000..8278194
> --- /dev/null
> +++ b/drivers/interconnect/exynos/exynos.c
> @@ -0,0 +1,185 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Exynos generic interconnect provider driver
> + *
> + * Copyright (c) 2020 Samsung Electronics Co., Ltd.
> + *
> + * Authors: Artur Świgoń
> + * Sylwester Nawrocki
> + */
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#define kbps_to_khz(x) ((x) / 8)
> +
> +struct exynos_icc_priv {
> + struct device *dev;
> +
> + /* One interconnect node per provider */
> + struct icc_provider provider;
> + struct icc_node *node;
> +
> + struct dev_pm_qos_request qos_req;
> +};
> +
> +static struct icc_node *exynos_icc_get_parent(struct device_node *np)
> +{
> + struct of_phandle_args args;
> + int num, ret;
> +
> + num = of_count_phandle_with_args(np,
Re: [RFC PATCH v5 1/6] dt-bindings: exynos-bus: Add documentation for interconnect properties
On Fri, May 29, 2020 at 06:31:55PM +0200, Sylwester Nawrocki wrote: > Add documentation for new optional properties in the exynos bus nodes: > samsung,interconnect-parent, #interconnect-cells. > These properties allow to specify the SoC interconnect structure which > then allows the interconnect consumer devices to request specific > bandwidth requirements. > > Signed-off-by: Artur Świgoń > Signed-off-by: Sylwester Nawrocki > --- > Changes for v5: > - exynos,interconnect-parent-node renamed to samsung,interconnect-parent > --- > Documentation/devicetree/bindings/devfreq/exynos-bus.txt | 15 +-- > 1 file changed, 13 insertions(+), 2 deletions(-) Acked-by: Krzysztof Kozlowski Best regards, Krzysztof ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[Bug 205827] drm:drm_atomic_helper_wait_for_dependencies - drm_kms_helper - flip_done timed out
https://bugzilla.kernel.org/show_bug.cgi?id=205827 sander44 (ionut_n2...@yahoo.com) changed: What|Removed |Added Resolution|CODE_FIX|UNREPRODUCIBLE -- You are receiving this mail because: You are watching the assignee of the bug. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[Bug 205827] drm:drm_atomic_helper_wait_for_dependencies - drm_kms_helper - flip_done timed out
https://bugzilla.kernel.org/show_bug.cgi?id=205827 sander44 (ionut_n2...@yahoo.com) changed: What|Removed |Added Status|NEW |RESOLVED Resolution|--- |CODE_FIX --- Comment #2 from sander44 (ionut_n2...@yahoo.com) --- Hi Kernel Team, I tried with the new kernel version, and the problem no longer appears. Issue seems to be gone. I used the new kernel version: 5.7.0 It is ok to close this ticket. Thank you for solving it. A good day. -- You are receiving this mail because: You are watching the assignee of the bug. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm: document how user-space should use link-status
On Mon, 01 Jun 2020 14:48:58 +
Simon Ser wrote:
> Describe what a "BAD" link-status means for user-space and how it should
> handle it. The logic described has been implemented in igt [1].
>
> [1]:
> https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/commit/fbe61f529737191d0920521946a575bd55f00fbe
>
> Signed-off-by: Simon Ser
> Cc: Daniel Vetter
> Cc: Manasi Navare
> Cc: Pekka Paalanen
> ---
> drivers/gpu/drm/drm_connector.c | 6 ++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index f2b20fd66319..08ba84f9787a 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -994,6 +994,12 @@ static const struct drm_prop_enum_list dp_colorspaces[]
> = {
> * after modeset, the kernel driver may set this to "BAD" and issue a
> * hotplug uevent. Drivers should update this value using
> * drm_connector_set_link_status_property().
> + *
> + * When user-space receives the hotplug uevent and detects a "BAD"
> + * link-status, the connector is no longer enabled. The list of
> available
> + * modes may have changed. User-space is expected to pick a new mode if
> + * the current one has disappeared and perform a new modeset with
> + * link-status set to "GOOD" to re-enable the connector.
> * non_desktop:
> * Indicates the output should be ignored for purposes of displaying a
> * standard desktop environment or console. This is most likely because
Hi,
makes sense to me. Can it happen that there will be no modes left in
the list?
What if userspace is driving two connectors from the same CRTC, and only
one connector gets link-status bad, what does it mean? Is the other
connector still running as normal, as if the failed connector didn't
even exist?
That is mostly a question about what happens if userspace does not fix
up the link-status=bad connector and does not detach it from the CRTC,
but keeps on flipping or modesetting as if the failure never happened.
I guess I could ask it about both a CRTC that has another connector
still good, and a CRTC where the failed connector was the only one.
Can I trust that if the other connector is in any way affected, it too
will get link-status bad?
Thanks,
pq
pgpjDGjFU5JZ3.pgp
Description: OpenPGP digital signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
