Re: [PATCH v2 5/8] drm/amdgpu: Refactor sysfs removal
On 6/24/20 2:11 AM, Greg KH wrote: On Tue, Jun 23, 2020 at 11:04:30PM -0400, Andrey Grodzovsky wrote: On 6/23/20 2:05 AM, Greg KH wrote: On Tue, Jun 23, 2020 at 12:51:00AM -0400, Andrey Grodzovsky wrote: On 6/22/20 12:45 PM, Greg KH wrote: On Mon, Jun 22, 2020 at 12:07:25PM -0400, Andrey Grodzovsky wrote: On 6/22/20 7:21 AM, Greg KH wrote: On Mon, Jun 22, 2020 at 11:51:24AM +0200, Daniel Vetter wrote: On Sun, Jun 21, 2020 at 02:03:05AM -0400, Andrey Grodzovsky wrote: Track sysfs files in a list so they all can be removed during pci remove since otherwise their removal after that causes crash because parent folder was already removed during pci remove. Huh? That should not happen, do you have a backtrace of that crash? 2 examples in the attached trace. Odd, how did you trigger these? By manually triggering PCI remove from sysfs cd /sys/bus/pci/devices/\:05\:00.0 && echo 1 > remove For some reason, I didn't think that video/drm devices could handle hot-remove like this. The "old" PCI hotplug specification explicitly said that video devices were not supported, has that changed? And this whole issue is probably tied to the larger issue that Daniel was asking me about, when it came to device lifetimes and the drm layer, so odds are we need to fix that up first before worrying about trying to support this crazy request, right? :) [ 925.738225 <0.188086>] BUG: kernel NULL pointer dereference, address: 0090 [ 925.738232 <0.07>] #PF: supervisor read access in kernel mode [ 925.738236 <0.04>] #PF: error_code(0x) - not-present page [ 925.738240 <0.04>] PGD 0 P4D 0 [ 925.738245 <0.05>] Oops: [#1] SMP PTI [ 925.738249 <0.04>] CPU: 7 PID: 2547 Comm: amdgpu_test Tainted: G W OE 5.5.0-rc7-dev-kfd+ #50 [ 925.738256 <0.07>] Hardware name: System manufacturer System Product Name/RAMPAGE IV FORMULA, BIOS 4804 12/30/2013 [ 925.738266 <0.10>] RIP: 0010:kernfs_find_ns+0x18/0x110 [ 925.738270 <0.04>] Code: a6 cf ff 0f 1f 44 00 00 66 2e 0f 1f 84 00 00 00 00 00 66 66 66 66 90 41 57 41 56 49 89 f6 41 55 41 54 49 89 fd 55 53 49 89 d4 <0f> b7 af 90 00 00 00 8b 05 8f ee 6b 01 48 8b 5f 68 66 83 e5 20 41 [ 925.738282 <0.12>] RSP: 0018:ad6d0118fb00 EFLAGS: 00010246 [ 925.738287 <0.05>] RAX: RBX: RCX: 2098a12076864b7e [ 925.738292 <0.05>] RDX: RSI: b6606b31 RDI: [ 925.738297 <0.05>] RBP: b6606b31 R08: b5379d10 R09: [ 925.738302 <0.05>] R10: ad6d0118fb38 R11: 9a75f64820a8 R12: [ 925.738307 <0.05>] R13: R14: b6606b31 R15: 9a7612b06130 [ 925.738313 <0.06>] FS: 7f3eca4e8700() GS:9a763dbc() knlGS: [ 925.738319 <0.06>] CS: 0010 DS: ES: CR0: 80050033 [ 925.738323 <0.04>] CR2: 0090 CR3: 35e5a005 CR4: 000606e0 [ 925.738329 <0.06>] Call Trace: [ 925.738334 <0.05>] kernfs_find_and_get_ns+0x2e/0x50 [ 925.738339 <0.05>] sysfs_remove_group+0x25/0x80 [ 925.738344 <0.05>] sysfs_remove_groups+0x29/0x40 [ 925.738350 <0.06>] free_msi_irqs+0xf5/0x190 [ 925.738354 <0.04>] pci_disable_msi+0xe9/0x120 So the PCI core is trying to clean up attributes that it had registered, which is fine. But we can't seem to find the attributes? Were they already removed somewhere else? that's odd. Yes, as i pointed above i am emulating device remove from sysfs and this triggers pci device remove sequence and as part of that my specific device folder (05:00.0) is removed from the sysfs tree. But why are things being removed twice? Not sure I understand what removed twice ? I remove only once per sysfs attribute. This code path shows that the kernel is trying to remove a file that is not present, so someone removed it already... thanks, gre k-h That a mystery for me too... Andrey ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC PATCH 1/2] drm/hyperv: Add DRM driver for hyperv synthetic video device
> > > Thanks for the review. Unfortunately only the first vmbus message > > > take > > > effect and subsequent calls are ignored. I originally implemented > > > using > > > vram helpers but I figured out calling this vmbus message again > > > won't > > > change the vram location. > > /me notices there also is user_ctx. What is this? Not sure, I will try to get in touch with hyper-v folks internally to see if page-flip can be used. > > > I think that needs a very big comment. Maybe even enforce that with > > a > > "vram_gpa_set" boolean in the device structure, and complain if we > > try to > > do that twice. > > > > Also I guess congrats to microsoft for defining things like that :- > > / > > I would be kind of surprised if the virtual device doesn't support > pageflips. Maybe setting vram_gpa just isn't the correct way to do > it. Is there a specification available? > > There are a number of microsoft folks in Cc: Anyone willing to > comment? > > thanks, > Gerd > > PS: And, yes, in case pageflips really don't work going with shmem > helpers + blits is reasonable. > ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 0/2] virtio: modernize DMA quirks
Use generic names for the quirks to make it clear it is not just about the IOMMU, it's about DMA access in general. changes from v1: added patch 2 Michael S. Tsirkin (2): virtio: VIRTIO_F_IOMMU_PLATFORM -> VIRTIO_F_ACCESS_PLATFORM virtio: virtio_has_iommu_quirk -> virtio_has_dma_quirk arch/um/drivers/virtio_uml.c| 2 +- drivers/gpu/drm/virtio/virtgpu_object.c | 2 +- drivers/gpu/drm/virtio/virtgpu_vq.c | 4 ++-- drivers/vdpa/ifcvf/ifcvf_base.h | 2 +- drivers/vdpa/vdpa_sim/vdpa_sim.c| 4 ++-- drivers/vhost/net.c | 4 ++-- drivers/vhost/vdpa.c| 2 +- drivers/virtio/virtio_balloon.c | 2 +- drivers/virtio/virtio_ring.c| 4 ++-- include/linux/virtio_config.h | 6 +++--- include/uapi/linux/virtio_config.h | 10 +++--- tools/virtio/linux/virtio_config.h | 6 +++--- 12 files changed, 26 insertions(+), 22 deletions(-) -- MST ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 2/2] virtio: virtio_has_iommu_quirk -> virtio_has_dma_quirk
Now that the corresponding feature bit has been renamed,
rename the quirk too - it's about special ways to
do DMA, not necessarily about the IOMMU.
Signed-off-by: Michael S. Tsirkin
---
drivers/gpu/drm/virtio/virtgpu_object.c | 2 +-
drivers/gpu/drm/virtio/virtgpu_vq.c | 4 ++--
drivers/virtio/virtio_ring.c| 2 +-
include/linux/virtio_config.h | 4 ++--
tools/virtio/linux/virtio_config.h | 4 ++--
5 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/virtio/virtgpu_object.c
b/drivers/gpu/drm/virtio/virtgpu_object.c
index 6ccbd01cd888..e8799ab0c753 100644
--- a/drivers/gpu/drm/virtio/virtgpu_object.c
+++ b/drivers/gpu/drm/virtio/virtgpu_object.c
@@ -141,7 +141,7 @@ static int virtio_gpu_object_shmem_init(struct
virtio_gpu_device *vgdev,
struct virtio_gpu_mem_entry **ents,
unsigned int *nents)
{
- bool use_dma_api = !virtio_has_iommu_quirk(vgdev->vdev);
+ bool use_dma_api = !virtio_has_dma_quirk(vgdev->vdev);
struct virtio_gpu_object_shmem *shmem = to_virtio_gpu_shmem(bo);
struct scatterlist *sg;
int si, ret;
diff --git a/drivers/gpu/drm/virtio/virtgpu_vq.c
b/drivers/gpu/drm/virtio/virtgpu_vq.c
index 9e663a5d9952..53af60d484a4 100644
--- a/drivers/gpu/drm/virtio/virtgpu_vq.c
+++ b/drivers/gpu/drm/virtio/virtgpu_vq.c
@@ -599,7 +599,7 @@ void virtio_gpu_cmd_transfer_to_host_2d(struct
virtio_gpu_device *vgdev,
struct virtio_gpu_object *bo = gem_to_virtio_gpu_obj(objs->objs[0]);
struct virtio_gpu_transfer_to_host_2d *cmd_p;
struct virtio_gpu_vbuffer *vbuf;
- bool use_dma_api = !virtio_has_iommu_quirk(vgdev->vdev);
+ bool use_dma_api = !virtio_has_dma_quirk(vgdev->vdev);
struct virtio_gpu_object_shmem *shmem = to_virtio_gpu_shmem(bo);
if (use_dma_api)
@@ -1015,7 +1015,7 @@ void virtio_gpu_cmd_transfer_to_host_3d(struct
virtio_gpu_device *vgdev,
struct virtio_gpu_object *bo = gem_to_virtio_gpu_obj(objs->objs[0]);
struct virtio_gpu_transfer_host_3d *cmd_p;
struct virtio_gpu_vbuffer *vbuf;
- bool use_dma_api = !virtio_has_iommu_quirk(vgdev->vdev);
+ bool use_dma_api = !virtio_has_dma_quirk(vgdev->vdev);
struct virtio_gpu_object_shmem *shmem = to_virtio_gpu_shmem(bo);
if (use_dma_api)
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index a1a5c2a91426..34253cb69cb8 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -240,7 +240,7 @@ static inline bool virtqueue_use_indirect(struct virtqueue
*_vq,
static bool vring_use_dma_api(struct virtio_device *vdev)
{
- if (!virtio_has_iommu_quirk(vdev))
+ if (!virtio_has_dma_quirk(vdev))
return true;
/* Otherwise, we are left to guess. */
diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
index f2cc2a0df174..3b4eae5ac5e3 100644
--- a/include/linux/virtio_config.h
+++ b/include/linux/virtio_config.h
@@ -162,10 +162,10 @@ static inline bool virtio_has_feature(const struct
virtio_device *vdev,
}
/**
- * virtio_has_iommu_quirk - determine whether this device has the iommu quirk
+ * virtio_has_dma_quirk - determine whether this device has the DMA quirk
* @vdev: the device
*/
-static inline bool virtio_has_iommu_quirk(const struct virtio_device *vdev)
+static inline bool virtio_has_dma_quirk(const struct virtio_device *vdev)
{
/*
* Note the reverse polarity of the quirk feature (compared to most
diff --git a/tools/virtio/linux/virtio_config.h
b/tools/virtio/linux/virtio_config.h
index f99ae42668e0..f2640e505c4e 100644
--- a/tools/virtio/linux/virtio_config.h
+++ b/tools/virtio/linux/virtio_config.h
@@ -42,10 +42,10 @@ static inline void __virtio_clear_bit(struct virtio_device
*vdev,
(__virtio_test_bit((dev), feature))
/**
- * virtio_has_iommu_quirk - determine whether this device has the iommu quirk
+ * virtio_has_dma_quirk - determine whether this device has the DMA quirk
* @vdev: the device
*/
-static inline bool virtio_has_iommu_quirk(const struct virtio_device *vdev)
+static inline bool virtio_has_dma_quirk(const struct virtio_device *vdev)
{
/*
* Note the reverse polarity of the quirk feature (compared to most
--
MST
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[RFC v7 11/11] drm/nouveau/kms/nvd9-: Add CRC support
This introduces support for CRC readback on gf119+, using the documentation generously provided to us by Nvidia: https://github.com/NVIDIA/open-gpu-doc/blob/master/Display-CRC/display-crc.txt We expose all available CRC sources. SF, SOR, PIOR, and DAC are exposed through a single set of "outp" sources: outp-active/auto for a CRC of the scanout region, outp-complete for a CRC of both the scanout and blanking/sync region combined, and outp-inactive for a CRC of only the blanking/sync region. For each source, nouveau selects the appropriate tap point based on the output path in use. We also expose an "rg" source, which allows for capturing CRCs of the scanout raster before it's encoded into a video signal in the output path. This tap point is referred to as the raster generator. Note that while there's some other neat features that can be used with CRC capture on nvidia hardware, like capturing from two CRC sources simultaneously, I couldn't see any usecase for them and did not implement them. Nvidia only allows for accessing CRCs through a shared DMA region that we program through the core EVO/NvDisplay channel which is referred to as the notifier context. The notifier context is limited to either 255 (for Fermi-Pascal) or 2047 (Volta+) entries to store CRCs in, and unfortunately the hardware simply drops CRCs and reports an overflow once all available entries in the notifier context are filled. Since the DRM CRC API and igt-gpu-tools don't expect there to be a limit on how many CRCs can be captured, we work around this in nouveau by allocating two separate notifier contexts for each head instead of one. We schedule a vblank worker ahead of time so that once we start getting close to filling up all of the available entries in the notifier context, we can swap the currently used notifier context out with another pre-prepared notifier context in a manner similar to page flipping. Unfortunately, the hardware only allows us to this by flushing two separate updates on the core channel: one to release the current notifier context handle, and one to program the next notifier context's handle. When the hardware processes the first update, the CRC for the current frame is lost. However, the second update can be flushed immediately without waiting for the first to complete so that CRC generation resumes on the next frame. According to Nvidia's hardware engineers, there isn't any cleaner way of flipping notifier contexts that would avoid this. Since using vblank workers to swap out the notifier context will ensure we can usually flush both updates to hardware within the timespan of a single frame, we can also ensure that there will only be exactly one frame lost between the first and second update being executed by the hardware. This gives us the guarantee that we're always correctly matching each CRC entry with it's respective frame even after a context flip. And since IGT will retrieve the CRC entry for a frame by waiting until it receives a CRC for any subsequent frames, this doesn't cause an issue with any tests and is much simpler than trying to change the current DRM API to accommodate. In order to facilitate testing of correct handling of this limitation, we also expose a debugfs interface to manually control the threshold for when we start trying to flip the notifier context. We will use this in igt to trigger a context flip for testing purposes without needing to wait for the notifier to completely fill up. This threshold is reset to the default value set by nouveau after each capture, and is exposed in a separate folder within each CRTC's debugfs directory labelled "nv_crc". Changes since v1: * Forgot to finish saving crc.h before saving, whoops. This just adds some corrections to the empty function declarations that we use if CONFIG_DEBUG_FS isn't enabled. Changes since v2: * Don't check return code from debugfs_create_dir() or debugfs_create_file() - Greg K-H Changes since v3: (no functional changes) * Fix SPDX license identifiers (checkpatch) * s/uint32_t/u32/ (checkpatch) * Fix indenting in switch cases (checkpatch) Changes since v4: * Remove unneeded param changes with nv50_head_flush_clr/set * Rebase Changes since v5: * Remove set but unused variable (outp) in nv50_crc_atomic_check() - Kbuild bot Signed-off-by: Lyude Paul --- drivers/gpu/drm/nouveau/dispnv04/crtc.c | 25 +- drivers/gpu/drm/nouveau/dispnv50/Kbuild | 4 + drivers/gpu/drm/nouveau/dispnv50/atom.h | 20 + drivers/gpu/drm/nouveau/dispnv50/core.h | 4 + drivers/gpu/drm/nouveau/dispnv50/core907d.c | 3 + drivers/gpu/drm/nouveau/dispnv50/core917d.c | 3 + drivers/gpu/drm/nouveau/dispnv50/corec37d.c | 3 + drivers/gpu/drm/nouveau/dispnv50/corec57d.c | 3 + drivers/gpu/drm/nouveau/dispnv50/crc.c | 714 drivers/gpu/drm/nouveau/dispnv50/crc.h | 125 drivers/gpu/drm/nouveau/dispnv50/crc907d.c | 139 drivers/gpu/drm/nouveau/dispnv50/crcc37d.c | 153 + dr
[RFC v7 10/11] drm/nouveau/kms/nv50-: Move hard-coded object handles into header
While most of the functionality on Nvidia GPUs doesn't require using an
explicit handle instead of the main VRAM handle + offset, there are a
couple of places that do require explicit handles, such as CRC
functionality. Since this means we're about to add another
nouveau-chosen handle, let's just go ahead and move any hard-coded
handles into a single header. This is just to keep things slightly
organized, and to make it a little bit easier if we need to add more
handles in the future.
This patch should contain no functional changes.
Changes since v3:
* Correct SPDX license identifier (checkpatch)
Signed-off-by: Lyude Paul
---
drivers/gpu/drm/nouveau/dispnv50/disp.c| 7 +--
drivers/gpu/drm/nouveau/dispnv50/handles.h | 15 +++
drivers/gpu/drm/nouveau/dispnv50/wndw.c| 3 ++-
3 files changed, 22 insertions(+), 3 deletions(-)
create mode 100644 drivers/gpu/drm/nouveau/dispnv50/handles.h
diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c
b/drivers/gpu/drm/nouveau/dispnv50/disp.c
index 090882794f7d6..bf7ba1e1c0f74 100644
--- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
+++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
@@ -26,6 +26,7 @@
#include "core.h"
#include "head.h"
#include "wndw.h"
+#include "handles.h"
#include
#include
@@ -154,7 +155,8 @@ nv50_dmac_create(struct nvif_device *device, struct
nvif_object *disp,
if (!syncbuf)
return 0;
- ret = nvif_object_init(&dmac->base.user, 0xf000, NV_DMA_IN_MEMORY,
+ ret = nvif_object_init(&dmac->base.user, NV50_DISP_HANDLE_SYNCBUF,
+ NV_DMA_IN_MEMORY,
&(struct nv_dma_v0) {
.target = NV_DMA_V0_TARGET_VRAM,
.access = NV_DMA_V0_ACCESS_RDWR,
@@ -165,7 +167,8 @@ nv50_dmac_create(struct nvif_device *device, struct
nvif_object *disp,
if (ret)
return ret;
- ret = nvif_object_init(&dmac->base.user, 0xf001, NV_DMA_IN_MEMORY,
+ ret = nvif_object_init(&dmac->base.user, NV50_DISP_HANDLE_VRAM,
+ NV_DMA_IN_MEMORY,
&(struct nv_dma_v0) {
.target = NV_DMA_V0_TARGET_VRAM,
.access = NV_DMA_V0_ACCESS_RDWR,
diff --git a/drivers/gpu/drm/nouveau/dispnv50/handles.h
b/drivers/gpu/drm/nouveau/dispnv50/handles.h
new file mode 100644
index 0..d1beeb9a444db
--- /dev/null
+++ b/drivers/gpu/drm/nouveau/dispnv50/handles.h
@@ -0,0 +1,15 @@
+// SPDX-License-Identifier: MIT
+#ifndef __NV50_KMS_HANDLES_H__
+#define __NV50_KMS_HANDLES_H__
+
+/*
+ * Various hard-coded object handles that nouveau uses. These are made-up by
+ * nouveau developers, not Nvidia. The only significance of the handles chosen
+ * is that they must all be unique.
+ */
+#define NV50_DISP_HANDLE_SYNCBUF
0xf000
+#define NV50_DISP_HANDLE_VRAM
0xf001
+
+#define NV50_DISP_HANDLE_WNDW_CTX(kind)(0xfb00 |
kind)
+
+#endif /* !__NV50_KMS_HANDLES_H__ */
diff --git a/drivers/gpu/drm/nouveau/dispnv50/wndw.c
b/drivers/gpu/drm/nouveau/dispnv50/wndw.c
index cfee61f14aa49..9d963ecdd34e8 100644
--- a/drivers/gpu/drm/nouveau/dispnv50/wndw.c
+++ b/drivers/gpu/drm/nouveau/dispnv50/wndw.c
@@ -21,6 +21,7 @@
*/
#include "wndw.h"
#include "wimm.h"
+#include "handles.h"
#include
#include
@@ -59,7 +60,7 @@ nv50_wndw_ctxdma_new(struct nv50_wndw *wndw, struct
drm_framebuffer *fb)
int ret;
nouveau_framebuffer_get_layout(fb, &unused, &kind);
- handle = 0xfb00 | kind;
+ handle = NV50_DISP_HANDLE_WNDW_CTX(kind);
list_for_each_entry(ctxdma, &wndw->ctxdma.list, head) {
if (ctxdma->object.handle == handle)
--
2.26.2
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[RFC v7 06/11] drm/nouveau/kms/nv50-: Fix disabling dithering
While we expose the ability to turn off hardware dithering for nouveau,
we actually make the mistake of turning it on anyway, due to
dithering_depth containing a non-zero value if our dithering depth isn't
also set to 6 bpc.
So, fix it by never enabling dithering when it's disabled.
Signed-off-by: Lyude Paul
---
drivers/gpu/drm/nouveau/dispnv50/head.c | 24 +---
1 file changed, 13 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/dispnv50/head.c
b/drivers/gpu/drm/nouveau/dispnv50/head.c
index e29ea40e7c334..72bc3bce396a7 100644
--- a/drivers/gpu/drm/nouveau/dispnv50/head.c
+++ b/drivers/gpu/drm/nouveau/dispnv50/head.c
@@ -84,18 +84,20 @@ nv50_head_atomic_check_dither(struct nv50_head_atom *armh,
{
u32 mode = 0x00;
- if (asyc->dither.mode == DITHERING_MODE_AUTO) {
- if (asyh->base.depth > asyh->or.bpc * 3)
- mode = DITHERING_MODE_DYNAMIC2X2;
- } else {
- mode = asyc->dither.mode;
- }
+ if (asyc->dither.mode) {
+ if (asyc->dither.mode == DITHERING_MODE_AUTO) {
+ if (asyh->base.depth > asyh->or.bpc * 3)
+ mode = DITHERING_MODE_DYNAMIC2X2;
+ } else {
+ mode = asyc->dither.mode;
+ }
- if (asyc->dither.depth == DITHERING_DEPTH_AUTO) {
- if (asyh->or.bpc >= 8)
- mode |= DITHERING_DEPTH_8BPC;
- } else {
- mode |= asyc->dither.depth;
+ if (asyc->dither.depth == DITHERING_DEPTH_AUTO) {
+ if (asyh->or.bpc >= 8)
+ mode |= DITHERING_DEPTH_8BPC;
+ } else {
+ mode |= asyc->dither.depth;
+ }
}
asyh->dither.enable = mode;
--
2.26.2
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[RFC v7 05/11] drm/nouveau/kms/nv140-: Don't modify depth in state during atomic commit
Currently, we modify the depth value stored in the atomic state when
performing a commit in order to workaround the fact we haven't
implemented support for depths higher then 10 yet. This isn't idempotent
though, as it will happen every atomic commit where we modify the OR
state even if the head's depth in the atomic state hasn't been modified.
Normally this wouldn't matter, since we don't modify OR state outside of
modesets, but since the CRC capture region is implemented as part of the
OR state in hardware we'll want to make sure all commits modifying OR
state are idempotent so as to avoid changing the depth unexpectedly.
So, fix this by simply not writing the reduced depth value we come up
with to the atomic state.
Signed-off-by: Lyude Paul
---
drivers/gpu/drm/nouveau/dispnv50/headc37d.c | 11 +++
drivers/gpu/drm/nouveau/dispnv50/headc57d.c | 11 +++
2 files changed, 14 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/dispnv50/headc37d.c
b/drivers/gpu/drm/nouveau/dispnv50/headc37d.c
index 4a9a32b89f746..9ef3c603fc43e 100644
--- a/drivers/gpu/drm/nouveau/dispnv50/headc37d.c
+++ b/drivers/gpu/drm/nouveau/dispnv50/headc37d.c
@@ -27,17 +27,20 @@ static void
headc37d_or(struct nv50_head *head, struct nv50_head_atom *asyh)
{
struct nv50_dmac *core = &nv50_disp(head->base.base.dev)->core->chan;
+ u8 depth;
u32 *push;
+
if ((push = evo_wait(core, 2))) {
/*XXX: This is a dirty hack until OR depth handling is
* improved later for deep colour etc.
*/
switch (asyh->or.depth) {
- case 6: asyh->or.depth = 5; break;
- case 5: asyh->or.depth = 4; break;
- case 2: asyh->or.depth = 1; break;
- case 0: asyh->or.depth = 4; break;
+ case 6: depth = 5; break;
+ case 5: depth = 4; break;
+ case 2: depth = 1; break;
+ case 0: depth = 4; break;
default:
+ depth = asyh->or.depth;
WARN_ON(1);
break;
}
diff --git a/drivers/gpu/drm/nouveau/dispnv50/headc57d.c
b/drivers/gpu/drm/nouveau/dispnv50/headc57d.c
index 859131a8bc3c8..97141eb8e75ab 100644
--- a/drivers/gpu/drm/nouveau/dispnv50/headc57d.c
+++ b/drivers/gpu/drm/nouveau/dispnv50/headc57d.c
@@ -27,17 +27,20 @@ static void
headc57d_or(struct nv50_head *head, struct nv50_head_atom *asyh)
{
struct nv50_dmac *core = &nv50_disp(head->base.base.dev)->core->chan;
+ u8 depth;
u32 *push;
+
if ((push = evo_wait(core, 2))) {
/*XXX: This is a dirty hack until OR depth handling is
* improved later for deep colour etc.
*/
switch (asyh->or.depth) {
- case 6: asyh->or.depth = 5; break;
- case 5: asyh->or.depth = 4; break;
- case 2: asyh->or.depth = 1; break;
- case 0: asyh->or.depth = 4; break;
+ case 6: depth = 5; break;
+ case 5: depth = 4; break;
+ case 2: depth = 1; break;
+ case 0: depth = 4; break;
default:
+ depth = asyh->or.depth;
WARN_ON(1);
break;
}
--
2.26.2
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[RFC v7 02/11] drm/vblank: Use spin_(un)lock_irq() in drm_crtc_vblank_off()
This got me confused for a bit while looking over this code: I had been planning on adding some blocking function calls into this function, but seeing the irqsave/irqrestore variants of spin_(un)lock() didn't make it very clear whether or not that would actually be safe. So I went ahead and reviewed every single driver in the kernel that uses this function, and they all fall into three categories: * Driver probe code * ->atomic_disable() callbacks * Legacy modesetting callbacks All of these will be guaranteed to have IRQs enabled, which means it's perfectly safe to block here. Just to make things a little less confusing to others in the future, let's switch over to spin_lock_irq()/spin_unlock_irq() to make that fact a little more obvious. Signed-off-by: Lyude Paul Cc: Daniel Vetter Cc: Ville Syrjälä --- drivers/gpu/drm/drm_vblank.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c index ce5c1e1d29963..e895f5331fdb4 100644 --- a/drivers/gpu/drm/drm_vblank.c +++ b/drivers/gpu/drm/drm_vblank.c @@ -1283,13 +1283,12 @@ void drm_crtc_vblank_off(struct drm_crtc *crtc) struct drm_pending_vblank_event *e, *t; ktime_t now; - unsigned long irqflags; u64 seq; if (drm_WARN_ON(dev, pipe >= dev->num_crtcs)) return; - spin_lock_irqsave(&dev->event_lock, irqflags); + spin_lock_irq(&dev->event_lock); spin_lock(&dev->vbl_lock); drm_dbg_vbl(dev, "crtc %d, vblank enabled %d, inmodeset %d\n", @@ -1325,7 +1324,7 @@ void drm_crtc_vblank_off(struct drm_crtc *crtc) drm_vblank_put(dev, pipe); send_vblank_event(dev, e, seq, now); } - spin_unlock_irqrestore(&dev->event_lock, irqflags); + spin_unlock_irq(&dev->event_lock); /* Will be reset by the modeset helpers when re-enabling the crtc by * calling drm_calc_timestamping_constants(). */ -- 2.26.2 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[RFC v7 04/11] drm/nouveau/kms/nv50-: Unroll error cleanup in nv50_head_create()
We'll be rolling back more things in this function, and the way it's
structured is a bit confusing. So, let's clean this up a bit and just
unroll in the event of failure.
Signed-off-by: Lyude Paul
---
drivers/gpu/drm/nouveau/dispnv50/head.c | 33 +
1 file changed, 23 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/dispnv50/head.c
b/drivers/gpu/drm/nouveau/dispnv50/head.c
index 8f6455697ba72..e29ea40e7c334 100644
--- a/drivers/gpu/drm/nouveau/dispnv50/head.c
+++ b/drivers/gpu/drm/nouveau/dispnv50/head.c
@@ -507,20 +507,28 @@ nv50_head_create(struct drm_device *dev, int index)
if (disp->disp->object.oclass < GV100_DISP) {
ret = nv50_base_new(drm, head->base.index, &base);
+ if (ret)
+ goto fail_free;
+
ret = nv50_ovly_new(drm, head->base.index, &ovly);
+ if (ret)
+ goto fail_free;
} else {
ret = nv50_wndw_new(drm, DRM_PLANE_TYPE_PRIMARY,
head->base.index * 2 + 0, &base);
+ if (ret)
+ goto fail_free;
+
ret = nv50_wndw_new(drm, DRM_PLANE_TYPE_OVERLAY,
head->base.index * 2 + 1, &ovly);
- }
- if (ret == 0)
- ret = nv50_curs_new(drm, head->base.index, &curs);
- if (ret) {
- kfree(head);
- return ERR_PTR(ret);
+ if (ret)
+ goto fail_free;
}
+ ret = nv50_curs_new(drm, head->base.index, &curs);
+ if (ret)
+ goto fail_free;
+
crtc = &head->base.base;
drm_crtc_init_with_planes(dev, crtc, &base->plane, &curs->plane,
&nv50_head_func, "head-%d", head->base.index);
@@ -533,11 +541,16 @@ nv50_head_create(struct drm_device *dev, int index)
if (head->func->olut_set) {
ret = nv50_lut_init(disp, &drm->client.mmu, &head->olut);
- if (ret) {
- nv50_head_destroy(crtc);
- return ERR_PTR(ret);
- }
+ if (ret)
+ goto fail_crtc_cleanup;
}
return head;
+
+fail_crtc_cleanup:
+ drm_crtc_cleanup(crtc);
+fail_free:
+ kfree(head);
+
+ return ERR_PTR(ret);
}
--
2.26.2
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[RFC v7 01/11] drm/vblank: Register drmm cleanup action once per drm_vblank_crtc
Since we'll be allocating resources for kthread_create_worker() in the
next commit (which could fail and require us to clean up the mess),
let's simplify the cleanup process a bit by registering a
drm_vblank_init_release() action for each drm_vblank_crtc so they're
still cleaned up if we fail to initialize one of them.
Changes since v3:
* Use drmm_add_action_or_reset() - Daniel Vetter
Cc: Daniel Vetter
Cc: Ville Syrjälä
Cc: dri-devel@lists.freedesktop.org
Cc: nouv...@lists.freedesktop.org
Signed-off-by: Lyude Paul
---
drivers/gpu/drm/drm_vblank.c | 23 ++-
1 file changed, 10 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
index 85e5f2db16085..ce5c1e1d29963 100644
--- a/drivers/gpu/drm/drm_vblank.c
+++ b/drivers/gpu/drm/drm_vblank.c
@@ -492,16 +492,12 @@ static void vblank_disable_fn(struct timer_list *t)
static void drm_vblank_init_release(struct drm_device *dev, void *ptr)
{
- unsigned int pipe;
-
- for (pipe = 0; pipe < dev->num_crtcs; pipe++) {
- struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
+ struct drm_vblank_crtc *vblank = ptr;
- drm_WARN_ON(dev, READ_ONCE(vblank->enabled) &&
- drm_core_check_feature(dev, DRIVER_MODESET));
+ drm_WARN_ON(dev, READ_ONCE(vblank->enabled) &&
+ drm_core_check_feature(dev, DRIVER_MODESET));
- del_timer_sync(&vblank->disable_timer);
- }
+ del_timer_sync(&vblank->disable_timer);
}
/**
@@ -511,7 +507,7 @@ static void drm_vblank_init_release(struct drm_device *dev,
void *ptr)
*
* This function initializes vblank support for @num_crtcs display pipelines.
* Cleanup is handled automatically through a cleanup function added with
- * drmm_add_action().
+ * drmm_add_action_or_reset().
*
* Returns:
* Zero on success or a negative error code on failure.
@@ -530,10 +526,6 @@ int drm_vblank_init(struct drm_device *dev, unsigned int
num_crtcs)
dev->num_crtcs = num_crtcs;
- ret = drmm_add_action(dev, drm_vblank_init_release, NULL);
- if (ret)
- return ret;
-
for (i = 0; i < num_crtcs; i++) {
struct drm_vblank_crtc *vblank = &dev->vblank[i];
@@ -542,6 +534,11 @@ int drm_vblank_init(struct drm_device *dev, unsigned int
num_crtcs)
init_waitqueue_head(&vblank->queue);
timer_setup(&vblank->disable_timer, vblank_disable_fn, 0);
seqlock_init(&vblank->seqlock);
+
+ ret = drmm_add_action_or_reset(dev, drm_vblank_init_release,
+ vblank);
+ if (ret)
+ return ret;
}
return 0;
--
2.26.2
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[RFC v7 09/11] drm/nouveau/kms/nv50-: Expose nv50_outp_atom in disp.h
In order to make sure that we flush disable updates at the right time
when disabling CRCs, we'll need to be able to look at the outp state to
see if we're changing it at the same time that we're disabling CRCs.
So, expose the struct in disp.h.
Signed-off-by: Lyude Paul
---
drivers/gpu/drm/nouveau/dispnv50/disp.c | 18 --
drivers/gpu/drm/nouveau/dispnv50/disp.h | 14 ++
2 files changed, 14 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c
b/drivers/gpu/drm/nouveau/dispnv50/disp.c
index 368069a5b181a..090882794f7d6 100644
--- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
+++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
@@ -57,24 +57,6 @@
#include
-/**
- * Atomic state
- */
-
-struct nv50_outp_atom {
- struct list_head head;
-
- struct drm_encoder *encoder;
- bool flush_disable;
-
- union nv50_outp_atom_mask {
- struct {
- bool ctrl:1;
- };
- u8 mask;
- } set, clr;
-};
-
/**
* EVO channel
*/
diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.h
b/drivers/gpu/drm/nouveau/dispnv50/disp.h
index 696e70a6b98b6..c7b72fa850995 100644
--- a/drivers/gpu/drm/nouveau/dispnv50/disp.h
+++ b/drivers/gpu/drm/nouveau/dispnv50/disp.h
@@ -71,6 +71,20 @@ struct nv50_dmac {
struct mutex lock;
};
+struct nv50_outp_atom {
+ struct list_head head;
+
+ struct drm_encoder *encoder;
+ bool flush_disable;
+
+ union nv50_outp_atom_mask {
+ struct {
+ bool ctrl:1;
+ };
+ u8 mask;
+ } set, clr;
+};
+
int nv50_dmac_create(struct nvif_device *device, struct nvif_object *disp,
const s32 *oclass, u8 head, void *data, u32 size,
u64 syncbuf, struct nv50_dmac *dmac);
--
2.26.2
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[RFC v7 07/11] drm/nouveau/kms/nv50-: s/harm/armh/g
We refer to the armed hardware assembly as armh elsewhere in nouveau, so
fix the naming here to make it consistent.
This patch contains no functional changes.
Signed-off-by: Lyude Paul
---
drivers/gpu/drm/nouveau/dispnv50/wndw.c | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/dispnv50/wndw.c
b/drivers/gpu/drm/nouveau/dispnv50/wndw.c
index 99b9b681736da..cfee61f14aa49 100644
--- a/drivers/gpu/drm/nouveau/dispnv50/wndw.c
+++ b/drivers/gpu/drm/nouveau/dispnv50/wndw.c
@@ -408,7 +408,7 @@ nv50_wndw_atomic_check(struct drm_plane *plane, struct
drm_plane_state *state)
struct nv50_wndw *wndw = nv50_wndw(plane);
struct nv50_wndw_atom *armw = nv50_wndw_atom(wndw->plane.state);
struct nv50_wndw_atom *asyw = nv50_wndw_atom(state);
- struct nv50_head_atom *harm = NULL, *asyh = NULL;
+ struct nv50_head_atom *armh = NULL, *asyh = NULL;
bool modeset = false;
int ret;
@@ -429,9 +429,9 @@ nv50_wndw_atomic_check(struct drm_plane *plane, struct
drm_plane_state *state)
/* Fetch assembly state for the head the window used to belong to. */
if (armw->state.crtc) {
- harm = nv50_head_atom_get(asyw->state.state, armw->state.crtc);
- if (IS_ERR(harm))
- return PTR_ERR(harm);
+ armh = nv50_head_atom_get(asyw->state.state, armw->state.crtc);
+ if (IS_ERR(armh))
+ return PTR_ERR(armh);
}
/* LUT configuration can potentially cause the window to be disabled. */
@@ -455,8 +455,8 @@ nv50_wndw_atomic_check(struct drm_plane *plane, struct
drm_plane_state *state)
asyh->wndw.mask |= BIT(wndw->id);
} else
if (armw->visible) {
- nv50_wndw_atomic_check_release(wndw, asyw, harm);
- harm->wndw.mask &= ~BIT(wndw->id);
+ nv50_wndw_atomic_check_release(wndw, asyw, armh);
+ armh->wndw.mask &= ~BIT(wndw->id);
} else {
return 0;
}
--
2.26.2
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[RFC v7 03/11] drm/vblank: Add vblank works
Add some kind of vblank workers. The interface is similar to regular delayed works, and is mostly based off kthread_work. It allows for scheduling delayed works that execute once a particular vblank sequence has passed. It also allows for accurate flushing of scheduled vblank works - in that flushing waits for both the vblank sequence and job execution to complete, or for the work to get cancelled - whichever comes first. Whatever hardware programming we do in the work must be fast (must at least complete during the vblank or scanout period, sometimes during the first few scanlines of the vblank). As such we use a high-priority per-CRTC thread to accomplish this. Changes since v6: * Get rid of ->pending and seqcounts, and implement flushing through simpler means - danvet * Get rid of work_lock, just use drm_device->event_lock * Move drm_vblank_work item cleanup into drm_crtc_vblank_off() so that we ensure that all vblank work has finished before disabling vblanks * Add checks into drm_crtc_vblank_reset() so we yell if it gets called while there's vblank workers active * Grab event_lock in both drm_crtc_vblank_on()/drm_crtc_vblank_off(), the main reason for this is so that other threads calling drm_vblank_work_schedule() are blocked from attempting to schedule while we're in the middle of enabling/disabling vblanks. * Move drm_handle_vblank_works() call below drm_handle_vblank_events() * Simplify drm_vblank_work_cancel_sync() * Fix drm_vblank_work_cancel_sync() documentation * Move wake_up_all() calls out of spinlock where we can. The only one I left was the call to wake_up_all() in drm_vblank_handle_works() as this seemed like it made more sense just living in that function (which is all technically under lock) * Move drm_vblank_work related functions into their own source files * Add drm_vblank_internal.h so we can export some functions we don't want drivers using, but that we do need to use in drm_vblank_work.c * Add a bunch of documentation Changes since v4: * Get rid of kthread interfaces we tried adding and move all of the locking into drm_vblank.c. For implementing drm_vblank_work_flush(), we now use a wait_queue and sequence counters in order to differentiate between multiple work item executions. * Get rid of drm_vblank_work_cancel() - this would have been pretty difficult to actually reimplement and it occurred to me that neither nouveau or i915 are even planning to use this function. Since there's also no async cancel function for most of the work interfaces in the kernel, it seems a bit unnecessary anyway. * Get rid of to_drm_vblank_work() since we now are also able to just pass the struct drm_vblank_work to work item callbacks anyway Changes since v3: * Use our own spinlocks, don't integrate so tightly with kthread_works Changes since v2: * Use kthread_workers instead of reinventing the wheel. Cc: Daniel Vetter Cc: Tejun Heo Cc: dri-devel@lists.freedesktop.org Cc: nouv...@lists.freedesktop.org Co-developed-by: Ville Syrjälä Signed-off-by: Lyude Paul --- Documentation/gpu/drm-kms.rst | 15 ++ drivers/gpu/drm/Makefile | 2 +- drivers/gpu/drm/drm_vblank.c | 55 +++-- drivers/gpu/drm/drm_vblank_internal.h | 19 ++ drivers/gpu/drm/drm_vblank_work.c | 259 + drivers/gpu/drm/drm_vblank_work_internal.h | 24 ++ include/drm/drm_vblank.h | 20 ++ include/drm/drm_vblank_work.h | 71 ++ 8 files changed, 447 insertions(+), 18 deletions(-) create mode 100644 drivers/gpu/drm/drm_vblank_internal.h create mode 100644 drivers/gpu/drm/drm_vblank_work.c create mode 100644 drivers/gpu/drm/drm_vblank_work_internal.h create mode 100644 include/drm/drm_vblank_work.h diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst index 975cfeb8a3532..3c5ae4f6dfd23 100644 --- a/Documentation/gpu/drm-kms.rst +++ b/Documentation/gpu/drm-kms.rst @@ -543,3 +543,18 @@ Vertical Blanking and Interrupt Handling Functions Reference .. kernel-doc:: drivers/gpu/drm/drm_vblank.c :export: + +Vertical Blank Work +=== + +.. kernel-doc:: drivers/gpu/drm/drm_vblank_work.c + :doc: vblank works + +Vertical Blank Work Functions Reference +--- + +.. kernel-doc:: include/drm/drm_vblank_work.h + :internal: + +.. kernel-doc:: drivers/gpu/drm/drm_vblank_work.c + :export: diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile index 2c0e5a7e59536..02ee5faf1a925 100644 --- a/drivers/gpu/drm/Makefile +++ b/drivers/gpu/drm/Makefile @@ -18,7 +18,7 @@ drm-y :=drm_auth.o drm_cache.o \ drm_dumb_buffers.o drm_mode_config.o drm_vblank.o \ drm_syncobj.o drm_lease.o drm_writeback.o drm_client.o \ drm_client_modeset.o drm_atomic_uapi.o drm_hdcp.o \ - drm_managed.o + drm_managed.o drm_vblank_work.o drm-$(CONFIG_DRM
[RFC v7 08/11] drm/nouveau/kms/nv140-: Track wndw mappings in nv50_head_atom
While we're not quite ready yet to add support for flexible wndw
mappings, we are going to need to at least keep track of the static wndw
mappings we're currently using in each head's atomic state. We'll likely
use this in the future to implement real flexible window mapping, but
the primary reason we'll need this is for CRC support.
See: on nvidia hardware, each CRC entry in the CRC notifier dma context
has a "tag". This tag corresponds to the nth update on a specific
EVO/NvDisplay channel, which itself is referred to as the "controlling
channel". For gf119+ this can be the core channel, ovly channel, or base
channel. Since we don't expose CRC entry tags to userspace, we simply
ignore this feature and always use the core channel as the controlling
channel. Simple.
Things get a little bit more complicated on gv100+ though. GV100+ only
lets us set the controlling channel to a specific wndw channel, and that
wndw must be owned by the head that we're grabbing CRCs when we enable
CRC generation. Thus, we always need to make sure that each atomic head
state has at least one wndw that is mapped to the head, which will be
used as the controlling channel.
Note that since we don't have flexible wndw mappings yet, we don't
expect to run into any scenarios yet where we'd have a head with no
mapped wndws. When we do add support for flexible wndw mappings however,
we'll need to make sure that we handle reprogramming CRC capture if our
controlling wndw is moved to another head (and potentially reject the
new head state entirely if we can't find another available wndw to
replace it).
With that being said, nouveau currently tracks wndw visibility on heads.
It does not keep track of the actual ownership mappings, which are
(currently) statically programmed. To fix this, we introduce another
bitmask into nv50_head_atom.wndw to keep track of ownership separately
from visibility. We then introduce a nv50_head callback to handle
populating the wndw ownership map, and call it during the atomic check
phase when core->assign_windows is set to true.
Signed-off-by: Lyude Paul
---
drivers/gpu/drm/nouveau/dispnv50/atom.h | 1 +
drivers/gpu/drm/nouveau/dispnv50/disp.c | 16
drivers/gpu/drm/nouveau/dispnv50/head.h | 2 ++
drivers/gpu/drm/nouveau/dispnv50/headc37d.c | 10 ++
drivers/gpu/drm/nouveau/dispnv50/headc57d.c | 2 ++
5 files changed, 31 insertions(+)
diff --git a/drivers/gpu/drm/nouveau/dispnv50/atom.h
b/drivers/gpu/drm/nouveau/dispnv50/atom.h
index 24f7700768dab..62faaf60f47a5 100644
--- a/drivers/gpu/drm/nouveau/dispnv50/atom.h
+++ b/drivers/gpu/drm/nouveau/dispnv50/atom.h
@@ -18,6 +18,7 @@ struct nv50_head_atom {
struct {
u32 mask;
+ u32 owned;
u32 olut;
} wndw;
diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c
b/drivers/gpu/drm/nouveau/dispnv50/disp.c
index d472942102f50..368069a5b181a 100644
--- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
+++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
@@ -2287,12 +2287,28 @@ static int
nv50_disp_atomic_check(struct drm_device *dev, struct drm_atomic_state *state)
{
struct nv50_atom *atom = nv50_atom(state);
+ struct nv50_core *core = nv50_disp(dev)->core;
struct drm_connector_state *old_connector_state, *new_connector_state;
struct drm_connector *connector;
struct drm_crtc_state *new_crtc_state;
struct drm_crtc *crtc;
+ struct nv50_head *head;
+ struct nv50_head_atom *asyh;
int ret, i;
+ if (core->assign_windows && core->func->head->static_wndw_map) {
+ drm_for_each_crtc(crtc, dev) {
+ new_crtc_state = drm_atomic_get_crtc_state(state,
+ crtc);
+ if (IS_ERR(new_crtc_state))
+ return PTR_ERR(new_crtc_state);
+
+ head = nv50_head(crtc);
+ asyh = nv50_head_atom(new_crtc_state);
+ core->func->head->static_wndw_map(head, asyh);
+ }
+ }
+
/* We need to handle colour management on a per-plane basis. */
for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
if (new_crtc_state->color_mgmt_changed) {
diff --git a/drivers/gpu/drm/nouveau/dispnv50/head.h
b/drivers/gpu/drm/nouveau/dispnv50/head.h
index c32b27cdaefc9..c05bbba9e247c 100644
--- a/drivers/gpu/drm/nouveau/dispnv50/head.h
+++ b/drivers/gpu/drm/nouveau/dispnv50/head.h
@@ -40,6 +40,7 @@ struct nv50_head_func {
void (*dither)(struct nv50_head *, struct nv50_head_atom *);
void (*procamp)(struct nv50_head *, struct nv50_head_atom *);
void (*or)(struct nv50_head *, struct nv50_head_atom *);
+ void (*static_wndw_map)(struct nv50_head *, struct nv50_head_atom *);
};
extern const struct nv50_head_func head507d;
@@ -86,6 +87,7 @@ int headc37d_curs_format(str
[RFC v7 00/11] drm/nouveau: Introduce CRC support for gf119+
Nvidia released some documentation on how CRC support works on their GPUs, hooray! So: this patch series implements said CRC support in nouveau, along with adding some special debugfs interfaces for some relevant igt-gpu-tools tests (already on the ML). First - we add some new functionality to kthread_work in the kernel, and then use this to add a new feature to DRM that Ville Syrjälä came up with: vblank workers. Basically, this is just a generic DRM interface that allows for scheduling high-priority workers that start on a given vblank interrupt. Note that while we're currently only using this in nouveau, Intel has plans to use this for i915 as well (hence why they came up with it!). And finally: in order to implement the last feature, we expose some new functions in the kernel's kthread_worker infrastructure so that we can de-complicate our implementation of this. Anyway-welcome to the future! :) Major changes since v6: * Move vblank_work related functions into their own files * Write documentation * Simplify work flushing and cancellation by getting rid of seqcounts and ->pending Major changes since v4: * Remove the interfaces we tried adding to kthread_worker and use a wait queue + seqcount in order to implement flushing vblank workers. * Rebase Major changes since v3: * Style fixes on nouveau patches from checkpatch, no functional changes * Don't integrate so tightly with kthread_work (and use our own lock), instead introduce some new functions for doing simple async flushing and cancelling. I think this interface looks a lot more acceptable then what I was previously trying. * Apply some changes requested by danvet Major changes since v2: * Use kthread_worker instead of kthreadd for vblank workers * Don't check debugfs return values Lyude Paul (11): drm/vblank: Register drmm cleanup action once per drm_vblank_crtc drm/vblank: Use spin_(un)lock_irq() in drm_crtc_vblank_off() drm/vblank: Add vblank works drm/nouveau/kms/nv50-: Unroll error cleanup in nv50_head_create() drm/nouveau/kms/nv140-: Don't modify depth in state during atomic commit drm/nouveau/kms/nv50-: Fix disabling dithering drm/nouveau/kms/nv50-: s/harm/armh/g drm/nouveau/kms/nv140-: Track wndw mappings in nv50_head_atom drm/nouveau/kms/nv50-: Expose nv50_outp_atom in disp.h drm/nouveau/kms/nv50-: Move hard-coded object handles into header drm/nouveau/kms/nvd9-: Add CRC support Documentation/gpu/drm-kms.rst | 15 + drivers/gpu/drm/Makefile| 2 +- drivers/gpu/drm/drm_vblank.c| 83 ++- drivers/gpu/drm/drm_vblank_internal.h | 19 + drivers/gpu/drm/drm_vblank_work.c | 259 +++ drivers/gpu/drm/drm_vblank_work_internal.h | 24 + drivers/gpu/drm/nouveau/dispnv04/crtc.c | 25 +- drivers/gpu/drm/nouveau/dispnv50/Kbuild | 4 + drivers/gpu/drm/nouveau/dispnv50/atom.h | 21 + drivers/gpu/drm/nouveau/dispnv50/core.h | 4 + drivers/gpu/drm/nouveau/dispnv50/core907d.c | 3 + drivers/gpu/drm/nouveau/dispnv50/core917d.c | 3 + drivers/gpu/drm/nouveau/dispnv50/corec37d.c | 3 + drivers/gpu/drm/nouveau/dispnv50/corec57d.c | 3 + drivers/gpu/drm/nouveau/dispnv50/crc.c | 714 drivers/gpu/drm/nouveau/dispnv50/crc.h | 125 drivers/gpu/drm/nouveau/dispnv50/crc907d.c | 139 drivers/gpu/drm/nouveau/dispnv50/crcc37d.c | 153 + drivers/gpu/drm/nouveau/dispnv50/disp.c | 58 +- drivers/gpu/drm/nouveau/dispnv50/disp.h | 24 + drivers/gpu/drm/nouveau/dispnv50/handles.h | 16 + drivers/gpu/drm/nouveau/dispnv50/head.c | 134 +++- drivers/gpu/drm/nouveau/dispnv50/head.h | 12 +- drivers/gpu/drm/nouveau/dispnv50/head907d.c | 14 +- drivers/gpu/drm/nouveau/dispnv50/headc37d.c | 27 +- drivers/gpu/drm/nouveau/dispnv50/headc57d.c | 20 +- drivers/gpu/drm/nouveau/dispnv50/wndw.c | 15 +- drivers/gpu/drm/nouveau/nouveau_display.c | 60 +- include/drm/drm_vblank.h| 20 + include/drm/drm_vblank_work.h | 71 ++ 30 files changed, 1910 insertions(+), 160 deletions(-) create mode 100644 drivers/gpu/drm/drm_vblank_internal.h create mode 100644 drivers/gpu/drm/drm_vblank_work.c create mode 100644 drivers/gpu/drm/drm_vblank_work_internal.h create mode 100644 drivers/gpu/drm/nouveau/dispnv50/crc.c create mode 100644 drivers/gpu/drm/nouveau/dispnv50/crc.h create mode 100644 drivers/gpu/drm/nouveau/dispnv50/crc907d.c create mode 100644 drivers/gpu/drm/nouveau/dispnv50/crcc37d.c create mode 100644 drivers/gpu/drm/nouveau/dispnv50/handles.h create mode 100644 include/drm/drm_vblank_work.h -- 2.26.2 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 27/27] drm: Add default modes for connectors in unknown state
Hi Daniel,
On Wed, Jun 24, 2020 at 09:23:04AM +0200, Daniel Vetter wrote:
> On Wed, Jun 24, 2020 at 04:12:09AM +0300, Laurent Pinchart wrote:
> > On Sun, Jun 21, 2020 at 10:40:00AM +0200, Sam Ravnborg wrote:
> > > On Tue, May 26, 2020 at 04:15:05AM +0300, Laurent Pinchart wrote:
> > > > The DRM CRTC helpers add default modes to connectors in the connected
> > > > state if no mode can be retrieved from the connector. This behaviour is
> > > > useful for VGA or DVI outputs that have no connected DDC bus. However,
> > > > in such cases, the status of the output usually can't be retrieved and
> > > > is reported as connector_status_unknown.
> > > >
> > > > Extend the addition of default modes to connectors in an unknown state
> > > > to support outputs that can retrieve neither the modes nor the
> > > > connection status.
> > > >
> > > > Signed-off-by: Laurent Pinchart
> > > >
> > >
> > > From your description sounds like an OK approach.
> > > But this is not something I feel too familiar with.
> > > Acked-by: Sam Ravnborg
> >
> > Thanks for the ack. I'd like to have Daniel's (CC'ed) feedback on this
> > too.
>
> Makes sense, and at least pre-coffee me can't immediately think of a
> scenario where we're going to regret this. _unknown status is pretty much
> limited to old VGA and similar things where load detect somehow isn't well
> supported by the hw.
>
> Reviewed-by: Daniel Vetter
>
> > > > ---
> > > > drivers/gpu/drm/drm_probe_helper.c | 3 ++-
> > > > include/drm/drm_modeset_helper_vtables.h | 8 +++-
> > > > 2 files changed, 9 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/drm_probe_helper.c
> > > > b/drivers/gpu/drm/drm_probe_helper.c
> > > > index f5d141e0400f..9055d9573c90 100644
> > > > --- a/drivers/gpu/drm/drm_probe_helper.c
> > > > +++ b/drivers/gpu/drm/drm_probe_helper.c
> > > > @@ -491,7 +491,8 @@ int drm_helper_probe_single_connector_modes(struct
> > > > drm_connector *connector,
> > > > if (count == 0 && connector->status ==
> > > > connector_status_connected)
> > > > count = drm_add_override_edid_modes(connector);
> > > >
> > > > - if (count == 0 && connector->status ==
> > > > connector_status_connected)
> > > > + if (count == 0 && (connector->status ==
> > > > connector_status_connected ||
> > > > + connector->status ==
> > > > connector_status_unknown))
> > > > count = drm_add_modes_noedid(connector, 1024, 768);
> > > > count += drm_helper_probe_add_cmdline_mode(connector);
> > > > if (count == 0)
> > > > diff --git a/include/drm/drm_modeset_helper_vtables.h
> > > > b/include/drm/drm_modeset_helper_vtables.h
> > > > index 421a30f08463..afe55e2e93d2 100644
> > > > --- a/include/drm/drm_modeset_helper_vtables.h
> > > > +++ b/include/drm/drm_modeset_helper_vtables.h
> > > > @@ -876,13 +876,19 @@ struct drm_connector_helper_funcs {
> > > > * The usual way to implement this is to cache the EDID
> > > > retrieved in the
> > > > * probe callback somewhere in the driver-private connector
> > > > structure.
> > > > * In this function drivers then parse the modes in the EDID
> > > > and add
> > > > -* them by calling drm_add_edid_modes(). But connectors that
> > > > driver a
> > > > +* them by calling drm_add_edid_modes(). But connectors that
> > > > drive a
> > > > * fixed panel can also manually add specific modes using
> > > > * drm_mode_probed_add(). Drivers which manually add modes
> > > > should also
> > > > * make sure that the &drm_connector.display_info,
> > > > * &drm_connector.width_mm and &drm_connector.height_mm fields
> > > > are
> > > > * filled in.
> > > > *
> > > > +* Note that the caller function will automatically add
> > > > standard VESA
> > > > +* DMT modes up to 1024x768 if the .get_modes() helper
> > > > operation returns
> > > > +* no mode and if the connector status is
> > > > connector_status_connected or
> > > > +* connector_status_unknown. There is no need to call
> > > > +* drm_add_edid_modes() manually in that case.
>
> Hm calling drm_add_edid_modes if you have no edid is a bit a funny idea
> ... Personally I'd just leave out the last sentence, I think that only
> confuses readers. Or I'm not grasphing what you're trying to tell here.
Sorry, I meant drm_add_modes_noedid(). Is that clearer ?
> r-b with or without this change since imo super tiny nit.
>
> > > > +*
> > > > * Virtual drivers that just want some standard VESA mode with
> > > > a given
> > > > * resolution can call drm_add_modes_noedid(), and mark the
> > > > preferred
> > > > * one using drm_set_preferred_mode().
--
Regards,
Laurent Pinchart
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.
[PATCH v2 3/3] Revert "drm/amd/display: Expose connector VRR range via debugfs"
From: Bhanuprakash Modem
v2:
* Rebase (Manasi)
As both VRR min and max are already part of drm_display_info,
drm can expose this VRR range for each connector.
Hence this logic should move to core DRM.
This reverts commit 727962f030c23422a01e8b22d0f463815fb15ec4.
Signed-off-by: Bhanuprakash Modem
Cc: Nicholas Kazlauskas
Cc: Harry Wentland
Cc: Alex Deucher
Cc: Manasi Navare
Cc: AMD gfx
Reviewed-by: Nicholas Kazlauskas
---
.../amd/display/amdgpu_dm/amdgpu_dm_debugfs.c | 20 ---
1 file changed, 20 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_debugfs.c
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_debugfs.c
index 076af267b488..71387d2af2ed 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_debugfs.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_debugfs.c
@@ -820,24 +820,6 @@ static int output_bpc_show(struct seq_file *m, void *data)
return res;
}
-/*
- * Returns the min and max vrr vfreq through the connector's debugfs file.
- * Example usage: cat /sys/kernel/debug/dri/0/DP-1/vrr_range
- */
-static int vrr_range_show(struct seq_file *m, void *data)
-{
- struct drm_connector *connector = m->private;
- struct amdgpu_dm_connector *aconnector =
to_amdgpu_dm_connector(connector);
-
- if (connector->status != connector_status_connected)
- return -ENODEV;
-
- seq_printf(m, "Min: %u\n", (unsigned int)aconnector->min_vfreq);
- seq_printf(m, "Max: %u\n", (unsigned int)aconnector->max_vfreq);
-
- return 0;
-}
-
#ifdef CONFIG_DRM_AMD_DC_HDCP
/*
* Returns the HDCP capability of the Display (1.4 for now).
@@ -1001,7 +983,6 @@ static ssize_t dp_dpcd_data_read(struct file *f, char
__user *buf,
DEFINE_SHOW_ATTRIBUTE(dmub_fw_state);
DEFINE_SHOW_ATTRIBUTE(dmub_tracebuffer);
DEFINE_SHOW_ATTRIBUTE(output_bpc);
-DEFINE_SHOW_ATTRIBUTE(vrr_range);
#ifdef CONFIG_DRM_AMD_DC_HDCP
DEFINE_SHOW_ATTRIBUTE(hdcp_sink_capability);
#endif
@@ -1059,7 +1040,6 @@ static const struct {
{"phy_settings", &dp_phy_settings_debugfs_fop},
{"test_pattern", &dp_phy_test_pattern_fops},
{"output_bpc", &output_bpc_fops},
- {"vrr_range", &vrr_range_fops},
#ifdef CONFIG_DRM_AMD_DC_HDCP
{"hdcp_sink_capability", &hdcp_sink_capability_fops},
#endif
--
2.19.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[pull] amdgpu, amdkfd, radeon drm-fixes-5.8
Hi Dave, Daniel, Fixes for 5.8. The following changes since commit 48778464bb7d346b47157d21ffde2af6b2d39110: Linux 5.8-rc2 (2020-06-21 15:45:29 -0700) are available in the Git repository at: git://people.freedesktop.org/~agd5f/linux tags/amd-drm-fixes-5.8-2020-06-24 for you to fetch changes up to b5b78a6c8d8cb9c307bc6b16a754603424459d6e: drm/amd: fix potential memleak in err branch (2020-06-24 18:03:16 -0400) amd-drm-fixes-5.8-2020-06-24: amdgpu: - Fix missed mutex unlock in DC error path - Fix firmware leak for sdma5 - DC bpc property fixes amdkfd: - Fix memleak in an error path radeon: - Fix copy paste typo in NI DPM spll validation Bernard Zhao (1): drm/amd: fix potential memleak in err branch Denis Efremov (1): drm/radeon: fix fb_div check in ni_init_smc_spll_table() John van der Kamp (1): drm/amdgpu/display: Unlock mutex on error Stylon Wang (2): drm/amd/display: Enable output_bpc property on all outputs drm/amd/display: Fix ineffective setting of max bpc property Wenhui Sheng (1): drm/amdgpu: add fw release for sdma v5_0 drivers/gpu/drm/amd/amdgpu/sdma_v5_0.c| 6 +- drivers/gpu/drm/amd/amdkfd/kfd_process.c | 1 + drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 3 ++- drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_debugfs.c | 4 +++- drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_hdcp.c| 6 -- drivers/gpu/drm/radeon/ni_dpm.c | 2 +- 6 files changed, 16 insertions(+), 6 deletions(-) ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RESEND PATCH v5 5/5] drm/bridge: lvds-codec: simplify error handling code
Hi Andrzej,
On Wed, Jun 24, 2020 at 04:03:30PM +0200, Andrzej Hajda wrote:
> On 24.06.2020 15:33, Laurent Pinchart wrote:
> > On Wed, Jun 24, 2020 at 01:41:27PM +0200, Andrzej Hajda wrote:
> >> Using probe_err code has following advantages:
> >> - shorter code,
> >> - recorded defer probe reason for debugging,
> >> - uniform error code logging.
> >>
> >> Signed-off-by: Andrzej Hajda
> >> ---
> >> drivers/gpu/drm/bridge/lvds-codec.c | 9 ++---
> >> 1 file changed, 2 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/bridge/lvds-codec.c
> >> b/drivers/gpu/drm/bridge/lvds-codec.c
> >> index 24fb1befdfa2..c76fa0239e39 100644
> >> --- a/drivers/gpu/drm/bridge/lvds-codec.c
> >> +++ b/drivers/gpu/drm/bridge/lvds-codec.c
> >> @@ -71,13 +71,8 @@ static int lvds_codec_probe(struct platform_device
> >> *pdev)
> >>lvds_codec->connector_type = (uintptr_t)of_device_get_match_data(dev);
> >>lvds_codec->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown",
> >> GPIOD_OUT_HIGH);
> >> - if (IS_ERR(lvds_codec->powerdown_gpio)) {
> >> - int err = PTR_ERR(lvds_codec->powerdown_gpio);
> >> -
> >> - if (err != -EPROBE_DEFER)
> >> - dev_err(dev, "powerdown GPIO failure: %d\n", err);
> >> - return err;
> >> - }
> >> + if (IS_ERR(lvds_codec->powerdown_gpio))
> >> + return probe_err(dev, lvds_codec->powerdown_gpio, "powerdown
> >> GPIO failure\n");
> >
> > Line wrap please.
>
> I hoped that with latest checkpatch line length limit increase from 80
> to 100 it is acceptable :) but apparently not [1].
I'm all for using longer lines when that improves readability, but in
this case I think it's easy enough to split the line. A longer line
limit doesn't mean we're forced to generate longer lines :-)
On a side note, I've been working on a C++ userspace project where we
had to decide on a coding style. Line length was one parameter, and we
went for a soft limit of 80 columns, and a hard limit of 120 columns.
This works quite well so far. The only pain point is that clang-format
(we use it, wrapped in a python script, to detect coding style
violations) doesn't understand soft and hard limits for line lengths.
> [1]:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=bdc48fa11e46f867ea4d75fa59ee87a7f48be144
>
> > It bothers me that the common pattern of writing the error code at the
> > end of the message isn't possible anymore. Maybe I'll get used to it,
> > but it removes some flexibility.
>
> Yes, but it gives uniformity :) and now with %pe printk format it
> changes anyway.
>
> >>/* Locate the panel DT node. */
> >>panel_node = of_graph_get_remote_node(dev->of_node, 1, 0);
--
Regards,
Laurent Pinchart
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[radeon-alex:drm-next 333/496] arch/arc/include/asm/bug.h:22:2: error: implicit declaration of function 'pr_warn'; did you mean
Hi Evan,
FYI, the error/warning still remains.
tree: git://people.freedesktop.org/~agd5f/linux.git drm-next
head: d86eab497ead0df7157704c0ed540652d6ba7ddc
commit: 5872ef0b03247fe659226973998ff28e835afbe4 [333/496] drm/amd/powerplay:
forbid to use pr_err/warn/info/debug
config: arc-allyesconfig (attached as .config)
compiler: arc-elf-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
~/bin/make.cross
chmod +x ~/bin/make.cross
git checkout 5872ef0b03247fe659226973998ff28e835afbe4
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=arc
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot
All errors (new ones prefixed by >>):
In file included from include/linux/bug.h:5,
from include/linux/mmdebug.h:5,
from include/linux/gfp.h:5,
from include/linux/firmware.h:7,
from drivers/gpu/drm/amd/amdgpu/../powerplay/smu_v11_0.c:23:
drivers/gpu/drm/amd/amdgpu/../powerplay/smu_v11_0.c: In function
'smu_v11_0_init_microcode':
>> arch/arc/include/asm/bug.h:22:2: error: implicit declaration of function
>> 'pr_warn'; did you mean 'pci_warn'? [-Werror=implicit-function-declaration]
22 | pr_warn("BUG: failure at %s:%d/%s()!\n", __FILE__, __LINE__,
__func__); \
| ^~~
drivers/gpu/drm/amd/amdgpu/../powerplay/smu_v11_0.c:176:3: note: in
expansion of macro 'BUG'
176 | BUG();
| ^~~
In file included from drivers/gpu/drm/amd/amdgpu/../powerplay/smu_v11_0.c:30:
At top level:
drivers/gpu/drm/amd/amdgpu/../amdgpu/amdgpu.h:190:18: warning:
'sched_policy' defined but not used [-Wunused-const-variable=]
190 | static const int sched_policy = KFD_SCHED_POLICY_HWS;
| ^~~~
In file included from drivers/gpu/drm/amd/amdgpu/../display/dc/dc_types.h:33,
from
drivers/gpu/drm/amd/amdgpu/../display/dc/dm_services_types.h:30,
from
drivers/gpu/drm/amd/amdgpu/../include/dm_pp_interface.h:26,
from drivers/gpu/drm/amd/amdgpu/../amdgpu/amdgpu.h:65,
from drivers/gpu/drm/amd/amdgpu/../powerplay/smu_v11_0.c:30:
drivers/gpu/drm/amd/amdgpu/../display/include/fixed31_32.h:76:32: warning:
'dc_fixpt_ln2_div_2' defined but not used [-Wunused-const-variable=]
76 | static const struct fixed31_32 dc_fixpt_ln2_div_2 = { 1488522236LL };
|^~
drivers/gpu/drm/amd/amdgpu/../display/include/fixed31_32.h:75:32: warning:
'dc_fixpt_ln2' defined but not used [-Wunused-const-variable=]
75 | static const struct fixed31_32 dc_fixpt_ln2 = { 2977044471LL };
|^~~~
drivers/gpu/drm/amd/amdgpu/../display/include/fixed31_32.h:74:32: warning:
'dc_fixpt_e' defined but not used [-Wunused-const-variable=]
74 | static const struct fixed31_32 dc_fixpt_e = { 11674931555LL };
|^~
drivers/gpu/drm/amd/amdgpu/../display/include/fixed31_32.h:73:32: warning:
'dc_fixpt_two_pi' defined but not used [-Wunused-const-variable=]
73 | static const struct fixed31_32 dc_fixpt_two_pi = { 26986075409LL };
|^~~
drivers/gpu/drm/amd/amdgpu/../display/include/fixed31_32.h:72:32: warning:
'dc_fixpt_pi' defined but not used [-Wunused-const-variable=]
72 | static const struct fixed31_32 dc_fixpt_pi = { 13493037705LL };
|^~~
drivers/gpu/drm/amd/amdgpu/../display/include/fixed31_32.h:67:32: warning:
'dc_fixpt_zero' defined but not used [-Wunused-const-variable=]
67 | static const struct fixed31_32 dc_fixpt_zero = { 0 };
|^
cc1: some warnings being treated as errors
vim +22 arch/arc/include/asm/bug.h
3be80aaef861a6 Vineet Gupta 2013-01-18 20
3be80aaef861a6 Vineet Gupta 2013-01-18 21 #define BUG() do {
\
3872d05299b5ab Vineet Gupta 2014-09-24 @22 pr_warn("BUG: failure at
%s:%d/%s()!\n", __FILE__, __LINE__, __func__); \
173a3efd3edb2e Arnd Bergmann 2018-02-21 23 barrier_before_unreachable();
\
173a3efd3edb2e Arnd Bergmann 2018-02-21 24 __builtin_trap();
\
3be80aaef861a6 Vineet Gupta 2013-01-18 25 } while (0)
3be80aaef861a6 Vineet Gupta 2013-01-18 26
:: The code at line 22 was first introduced by commit
:: 3872d05299b5ab58446f484df18f71cab4628c50 ARC: BUG() dumps stack after
@msg (@msg now same as in generic BUG))
:: TO: Vineet Gupta
:: CC: Vineet Gupta
--
[Bug 206475] amdgpu under load drop signal to monitor until hard reset
https://bugzilla.kernel.org/show_bug.cgi?id=206475 Alex Deucher (alexdeuc...@gmail.com) changed: What|Removed |Added CC||alexdeuc...@gmail.com --- Comment #16 from Alex Deucher (alexdeuc...@gmail.com) --- (In reply to Andrew Ammerlaan from comment #15) > However, now that the iGPU is default, I can still see the system monitor > that I usually run on the other monitor when this issue occurs. Every single > time the thermal sensor of the GPU would show a ridiculous value (e.g. 511 > degrees Celsius). When the GPU is in reset all reads to the MMIO BAR return 1s so you are just getting all ones until the reset succeeds. 511 is just all ones. This patch will fix that issue: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=9271dfd9e0f79e2969dcbe28568bce0fdc4f8f73 -- 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 206475] amdgpu under load drop signal to monitor until hard reset
https://bugzilla.kernel.org/show_bug.cgi?id=206475 --- Comment #15 from Andrew Ammerlaan (andrewammerl...@riseup.net) --- So today it was *really* hot, and I had this issue occur a couple of times. (The solution with the extra fans was nice and all, but not enough to prevent it entirely) However, now that the iGPU is default, I can still see the system monitor that I usually run on the other monitor when this issue occurs. Every single time the thermal sensor of the GPU would show a ridiculous value (e.g. 511 degrees Celsius). Now, this could explain why the GPU does a reset. If the thermal sensor would all of a sudden return a value of e.g. 511, then of course the GPU will shut itself down. As it is clearly impossible for the temperature of the GPU to jump from being somewhere between 80 to 90, to over 500 within a couple of milliseconds. I conclude that there is something wrong, either physically with the thermal sensor, or with the way the firmware/driver handles the temperature reporting from the sensor. Also, if the GPU would have actually reached a temperature of 511 it would be broken now, as the melting temperature of tin is about 230 degrees Celsius. I happen to work with thermometers quite a lot, and I have seen temperature readings do stuff like this. Usually the cause is either a broken, or shorted sensor (which is unlikely in this case, cause it works normally most of the time), or a wrong/incomplete calibration curve. (Usually thermal sensors are only calibrated within the range they are expected to operate, but the high limit of this calibration curve might be too low.) Anyway, either the GPU reset is caused by the incorrect temperature readings, or the incorrect temperature readings are caused by the GPU reset (which is also possible I guess). In any case, it would be great if AMD could look into this soon. Because clearly something is wrong. -- 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/msm/dpu: add support for dither block in display
On Wed, Jun 24, 2020 at 4:57 AM Kalyan Thota wrote:
>
> This change enables dither block for primary interface
> in display.
>
> Enabled for 6bpc in the current version.
>
> Signed-off-by: Kalyan Thota
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 45 +
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c | 66
> +
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.h | 28 +++
> 3 files changed, 130 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 63976dc..26e870a 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -208,6 +208,42 @@ struct dpu_encoder_virt {
>
> #define to_dpu_encoder_virt(x) container_of(x, struct dpu_encoder_virt, base)
>
> +static u32 dither_matrix[DITHER_MATRIX_SZ] = {
> + 15, 7, 13, 5, 3, 11, 1, 9, 12, 4, 14, 6, 0, 8, 2, 10
> +};
> +
> +static void _dpu_encoder_setup_dither(struct dpu_encoder_phys *phys)
> +{
> + struct dpu_hw_dither_cfg dither_cfg = { 0 };
> + struct drm_display_info *info;
> +
> + if (!phys || !phys->connector || !phys->hw_pp ||
> + !phys->hw_pp->ops.setup_dither)
> + return;
it looks like other than phys->hw_pp->ops.setup_dither, you shouldn't
need to check most of these conditions.
> +
> + info = &phys->connector->display_info;
> + if (!info)
and definitely not this one
> + return;
> +
> + switch (phys->connector->display_info.bpc) {
> + case 6:
> + dither_cfg.c0_bitdepth = 6;
> + dither_cfg.c1_bitdepth = 6;
> + dither_cfg.c2_bitdepth = 6;
> + dither_cfg.c3_bitdepth = 6;
> + dither_cfg.temporal_en = 0;
> + break;
> + default:
> + phys->hw_pp->ops.setup_dither(phys->hw_pp, NULL);
> + return;
> + }
> +
> + memcpy(&dither_cfg.matrix, dither_matrix,
> + sizeof(u32) * DITHER_MATRIX_SZ);
> +
> + phys->hw_pp->ops.setup_dither(phys->hw_pp, &dither_cfg);
> +}
> +
> void dpu_encoder_helper_report_irq_timeout(struct dpu_encoder_phys *phys_enc,
> enum dpu_intr_idx intr_idx)
> {
> @@ -1082,6 +1118,7 @@ static void _dpu_encoder_virt_enable_helper(struct
> drm_encoder *drm_enc)
> struct dpu_encoder_virt *dpu_enc = NULL;
> struct msm_drm_private *priv;
> struct dpu_kms *dpu_kms;
> + int i;
>
> if (!drm_enc || !drm_enc->dev) {
> DPU_ERROR("invalid parameters\n");
> @@ -1104,6 +1141,14 @@ static void _dpu_encoder_virt_enable_helper(struct
> drm_encoder *drm_enc)
> dpu_kms->catalog);
>
> _dpu_encoder_update_vsync_source(dpu_enc, &dpu_enc->disp_info);
> +
> + if (dpu_enc->disp_info.intf_type == DRM_MODE_ENCODER_DSI) {
> + for (i = 0; i < dpu_enc->num_phys_encs; i++) {
> + struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
> +
> + _dpu_encoder_setup_dither(phys);
> + }
> + }
> }
>
> void dpu_encoder_virt_runtime_resume(struct drm_encoder *drm_enc)
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
> index d110a40..cf7603d 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
> @@ -28,6 +28,16 @@
> #define PP_FBC_BUDGET_CTL 0x038
> #define PP_FBC_LOSSY_MODE 0x03C
>
> +#define PP_DITHER_EN 0x000
> +#define PP_DITHER_BITDEPTH 0x004
> +#define PP_DITHER_MATRIX 0x008
> +
> +#define DITHER_DEPTH_MAP_INDEX 9
> +
> +static u32 dither_depth_map[DITHER_DEPTH_MAP_INDEX] = {
> + 0, 0, 0, 0, 0, 0, 0, 1, 2
> +};
> +
> static const struct dpu_pingpong_cfg *_pingpong_offset(enum dpu_pingpong pp,
> const struct dpu_mdss_cfg *m,
> void __iomem *addr,
> @@ -49,6 +59,40 @@ static const struct dpu_pingpong_cfg
> *_pingpong_offset(enum dpu_pingpong pp,
> return ERR_PTR(-EINVAL);
> }
>
> +static void dpu_hw_pp_setup_dither(struct dpu_hw_pingpong *pp,
> + struct dpu_hw_dither_cfg *cfg)
> +{
> + struct dpu_hw_blk_reg_map *c;
> + u32 i, base, data = 0;
> +
> + if (!pp)
> + return;
can this ever be NULL.. at least currently you are checking this both
here and in _dpu_encoder_setup_dither()
BR,
-R
> +
> + c = &pp->hw;
> + base = pp->caps->sblk->dither.base;
> + if (!cfg) {
> + DPU_REG_WRITE(c, base + PP_DITHER_EN, 0);
> + return;
> + }
> +
> + data = dither_depth_map[cfg->c0_bitdepth] & REG_MASK(2);
> + data |= (dither_depth_map[cfg->c1_bitdepth] & REG_MASK(2)) << 2;
Re: [PATCH 27/27] drm: Add default modes for connectors in unknown state
On Wed, Jun 24, 2020 at 3:31 PM Daniel Vetter wrote:
>
> On Wed, Jun 24, 2020 at 5:24 PM Alex Deucher wrote:
> >
> > On Wed, Jun 24, 2020 at 3:23 AM Daniel Vetter wrote:
> > >
> > > On Wed, Jun 24, 2020 at 04:12:09AM +0300, Laurent Pinchart wrote:
> > > > Hi Sam,
> > > >
> > > > On Sun, Jun 21, 2020 at 10:40:00AM +0200, Sam Ravnborg wrote:
> > > > > On Tue, May 26, 2020 at 04:15:05AM +0300, Laurent Pinchart wrote:
> > > > > > The DRM CRTC helpers add default modes to connectors in the
> > > > > > connected
> > > > > > state if no mode can be retrieved from the connector. This
> > > > > > behaviour is
> > > > > > useful for VGA or DVI outputs that have no connected DDC bus.
> > > > > > However,
> > > > > > in such cases, the status of the output usually can't be retrieved
> > > > > > and
> > > > > > is reported as connector_status_unknown.
> > > > > >
> > > > > > Extend the addition of default modes to connectors in an unknown
> > > > > > state
> > > > > > to support outputs that can retrieve neither the modes nor the
> > > > > > connection status.
> > > > > >
> > > > > > Signed-off-by: Laurent Pinchart
> > > > > >
> > > > >
> > > > > From your description sounds like an OK approach.
> > > > > But this is not something I feel too familiar with.
> > > > > Acked-by: Sam Ravnborg
> > > >
> > > > Thanks for the ack. I'd like to have Daniel's (CC'ed) feedback on this
> > > > too.
> > >
> > > Makes sense, and at least pre-coffee me can't immediately think of a
> > > scenario where we're going to regret this. _unknown status is pretty much
> > > limited to old VGA and similar things where load detect somehow isn't well
> > > supported by the hw.
> > >
> > > Reviewed-by: Daniel Vetter
> > >
> > > >
> > > > > > ---
> > > > > > drivers/gpu/drm/drm_probe_helper.c | 3 ++-
> > > > > > include/drm/drm_modeset_helper_vtables.h | 8 +++-
> > > > > > 2 files changed, 9 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/drm_probe_helper.c
> > > > > > b/drivers/gpu/drm/drm_probe_helper.c
> > > > > > index f5d141e0400f..9055d9573c90 100644
> > > > > > --- a/drivers/gpu/drm/drm_probe_helper.c
> > > > > > +++ b/drivers/gpu/drm/drm_probe_helper.c
> > > > > > @@ -491,7 +491,8 @@ int
> > > > > > drm_helper_probe_single_connector_modes(struct drm_connector
> > > > > > *connector,
> > > > > > if (count == 0 && connector->status == connector_status_connected)
> > > > > > count = drm_add_override_edid_modes(connector);
> > > > > >
> > > > > > - if (count == 0 && connector->status == connector_status_connected)
> > > > > > + if (count == 0 && (connector->status ==
> > > > > > connector_status_connected ||
> > > > > > +connector->status == connector_status_unknown))
> > > > > > count = drm_add_modes_noedid(connector, 1024, 768);
> > > > > > count += drm_helper_probe_add_cmdline_mode(connector);
> > > > > > if (count == 0)
> > > > > > diff --git a/include/drm/drm_modeset_helper_vtables.h
> > > > > > b/include/drm/drm_modeset_helper_vtables.h
> > > > > > index 421a30f08463..afe55e2e93d2 100644
> > > > > > --- a/include/drm/drm_modeset_helper_vtables.h
> > > > > > +++ b/include/drm/drm_modeset_helper_vtables.h
> > > > > > @@ -876,13 +876,19 @@ struct drm_connector_helper_funcs {
> > > > > >* The usual way to implement this is to cache the EDID retrieved
> > > > > > in the
> > > > > >* probe callback somewhere in the driver-private connector
> > > > > > structure.
> > > > > >* In this function drivers then parse the modes in the EDID and
> > > > > > add
> > > > > > - * them by calling drm_add_edid_modes(). But connectors that
> > > > > > driver a
> > > > > > + * them by calling drm_add_edid_modes(). But connectors that
> > > > > > drive a
> > > > > >* fixed panel can also manually add specific modes using
> > > > > >* drm_mode_probed_add(). Drivers which manually add modes should
> > > > > > also
> > > > > >* make sure that the &drm_connector.display_info,
> > > > > >* &drm_connector.width_mm and &drm_connector.height_mm fields are
> > > > > >* filled in.
> > > > > >*
> > > > > > + * Note that the caller function will automatically add standard
> > > > > > VESA
> > > > > > + * DMT modes up to 1024x768 if the .get_modes() helper operation
> > > > > > returns
> > > > > > + * no mode and if the connector status is
> > > > > > connector_status_connected or
> > > > > > + * connector_status_unknown. There is no need to call
> > > > > > + * drm_add_edid_modes() manually in that case.
> > >
> > > Hm calling drm_add_edid_modes if you have no edid is a bit a funny idea
> > > ... Personally I'd just leave out the last sentence, I think that only
> > > confuses readers. Or I'm not grasphing what you're trying to tell here.
> >
> > IIRC, some drivers used and desktop environments expected unknown
> > rather than off for LVDS/eDP panels when the lid was shut or if the
> > mux was switched to another
Re: [RESEND PATCH v5 3/5] drivers core: allow probe_err accept integer and pointer types
On 24.06.2020 17:16, Robin Murphy wrote: > On 2020-06-24 16:04, Mark Brown wrote: >> On Wed, Jun 24, 2020 at 03:25:33PM +0100, Robin Murphy wrote: >> >>> And yeah, anyone who pipes up suggesting that places where an >>> ERR_PTR value >>> could be passed to probe_err() could simply refactor IS_ERR() checks >>> with >>> more uses of the god-awful PTR_ERR_OR_ZERO() obfuscator gets a long >>> stare of >>> disapproval... >> >> We could also have a probe_err_ptr() or something that took an ERR_PTR() >> instead if there really were an issue with explicitly doing this. > > Yeah, for all my lyrical objection, a static inline _ptr_err() > helper to wrap _err() with sensible type checking might actually > be an OK compromise if people really feel strongly for having that > utility. I have proposed such thing in my previous iteration[1], except it was macro because of variadic arguments. With current version we save 8 chars and hacky macro, with the old version we save only 4 chars and more clear construct - less tempting solution for me. Personally I prefer the current version - it does not seems to me more dangerous than all these PTR_ERR, IS_ERR,ERR_PTR helpers, but can prevent expression split across multiple lines due to 80char limit. Probably the simplest solution is to drop this patch, I will do it then. [1]: https://lwn.net/ml/linux-kernel/20181220102247.4911-4-a.ha...@samsung.com/ Regards Andrzej > > (and then we can debate whether it should also convert NULL to -ENOMEM > and !IS_ERR to 0... :D) > > Robin. > ___ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://protect2.fireeye.com/url?k=074420c0-5ada8e5a-0745ab8f-0cc47a336fae-bba8bb4caf96e14d&q=1&u=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel > > > ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] dma-buf: minor doc touch-ups
On Fri, Jun 12, 2020 at 09:05:35AM +0200, Daniel Vetter wrote: > Just some tiny edits: > - fix link to struct dma_fence > - give slightly more meaningful title - the polling here is about > implicit fences, explicit fences (in sync_file or drm_syncobj) also > have their own polling > > v2: I misplaced the .rst include change corresponding to this patch. > > Reviewed-by: Thomas Hellstrom > Signed-off-by: Daniel Vetter I went ahead and merged this one, shouldn't be the controversial part of the series :-) -Daniel > --- > Documentation/driver-api/dma-buf.rst | 6 +++--- > drivers/dma-buf/dma-buf.c| 6 +++--- > 2 files changed, 6 insertions(+), 6 deletions(-) > > diff --git a/Documentation/driver-api/dma-buf.rst > b/Documentation/driver-api/dma-buf.rst > index 63dec76d1d8d..7fb7b661febd 100644 > --- a/Documentation/driver-api/dma-buf.rst > +++ b/Documentation/driver-api/dma-buf.rst > @@ -100,11 +100,11 @@ CPU Access to DMA Buffer Objects > .. kernel-doc:: drivers/dma-buf/dma-buf.c > :doc: cpu access > > -Fence Poll Support > -~~ > +Implicit Fence Poll Support > +~~~ > > .. kernel-doc:: drivers/dma-buf/dma-buf.c > - :doc: fence polling > + :doc: implicit fence polling > > Kernel Functions and Structures Reference > ~ > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c > index 01ce125f8e8d..e018ef80451e 100644 > --- a/drivers/dma-buf/dma-buf.c > +++ b/drivers/dma-buf/dma-buf.c > @@ -161,11 +161,11 @@ static loff_t dma_buf_llseek(struct file *file, loff_t > offset, int whence) > } > > /** > - * DOC: fence polling > + * DOC: implicit fence polling > * > * To support cross-device and cross-driver synchronization of buffer access > - * implicit fences (represented internally in the kernel with &struct fence) > can > - * be attached to a &dma_buf. The glue for that and a few related things are > + * implicit fences (represented internally in the kernel with &struct > dma_fence) > + * can be attached to a &dma_buf. The glue for that and a few related things > are > * provided in the &dma_resv structure. > * > * Userspace can query the state of these implicitly tracked fences using > poll() > -- > 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
Re: [PATCH 27/27] drm: Add default modes for connectors in unknown state
On Wed, Jun 24, 2020 at 5:24 PM Alex Deucher wrote:
>
> On Wed, Jun 24, 2020 at 3:23 AM Daniel Vetter wrote:
> >
> > On Wed, Jun 24, 2020 at 04:12:09AM +0300, Laurent Pinchart wrote:
> > > Hi Sam,
> > >
> > > On Sun, Jun 21, 2020 at 10:40:00AM +0200, Sam Ravnborg wrote:
> > > > On Tue, May 26, 2020 at 04:15:05AM +0300, Laurent Pinchart wrote:
> > > > > The DRM CRTC helpers add default modes to connectors in the connected
> > > > > state if no mode can be retrieved from the connector. This behaviour
> > > > > is
> > > > > useful for VGA or DVI outputs that have no connected DDC bus. However,
> > > > > in such cases, the status of the output usually can't be retrieved and
> > > > > is reported as connector_status_unknown.
> > > > >
> > > > > Extend the addition of default modes to connectors in an unknown state
> > > > > to support outputs that can retrieve neither the modes nor the
> > > > > connection status.
> > > > >
> > > > > Signed-off-by: Laurent Pinchart
> > > > >
> > > >
> > > > From your description sounds like an OK approach.
> > > > But this is not something I feel too familiar with.
> > > > Acked-by: Sam Ravnborg
> > >
> > > Thanks for the ack. I'd like to have Daniel's (CC'ed) feedback on this
> > > too.
> >
> > Makes sense, and at least pre-coffee me can't immediately think of a
> > scenario where we're going to regret this. _unknown status is pretty much
> > limited to old VGA and similar things where load detect somehow isn't well
> > supported by the hw.
> >
> > Reviewed-by: Daniel Vetter
> >
> > >
> > > > > ---
> > > > > drivers/gpu/drm/drm_probe_helper.c | 3 ++-
> > > > > include/drm/drm_modeset_helper_vtables.h | 8 +++-
> > > > > 2 files changed, 9 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/drm_probe_helper.c
> > > > > b/drivers/gpu/drm/drm_probe_helper.c
> > > > > index f5d141e0400f..9055d9573c90 100644
> > > > > --- a/drivers/gpu/drm/drm_probe_helper.c
> > > > > +++ b/drivers/gpu/drm/drm_probe_helper.c
> > > > > @@ -491,7 +491,8 @@ int
> > > > > drm_helper_probe_single_connector_modes(struct drm_connector
> > > > > *connector,
> > > > > if (count == 0 && connector->status == connector_status_connected)
> > > > > count = drm_add_override_edid_modes(connector);
> > > > >
> > > > > - if (count == 0 && connector->status == connector_status_connected)
> > > > > + if (count == 0 && (connector->status == connector_status_connected
> > > > > ||
> > > > > +connector->status == connector_status_unknown))
> > > > > count = drm_add_modes_noedid(connector, 1024, 768);
> > > > > count += drm_helper_probe_add_cmdline_mode(connector);
> > > > > if (count == 0)
> > > > > diff --git a/include/drm/drm_modeset_helper_vtables.h
> > > > > b/include/drm/drm_modeset_helper_vtables.h
> > > > > index 421a30f08463..afe55e2e93d2 100644
> > > > > --- a/include/drm/drm_modeset_helper_vtables.h
> > > > > +++ b/include/drm/drm_modeset_helper_vtables.h
> > > > > @@ -876,13 +876,19 @@ struct drm_connector_helper_funcs {
> > > > >* The usual way to implement this is to cache the EDID retrieved
> > > > > in the
> > > > >* probe callback somewhere in the driver-private connector
> > > > > structure.
> > > > >* In this function drivers then parse the modes in the EDID and add
> > > > > - * them by calling drm_add_edid_modes(). But connectors that driver
> > > > > a
> > > > > + * them by calling drm_add_edid_modes(). But connectors that drive a
> > > > >* fixed panel can also manually add specific modes using
> > > > >* drm_mode_probed_add(). Drivers which manually add modes should
> > > > > also
> > > > >* make sure that the &drm_connector.display_info,
> > > > >* &drm_connector.width_mm and &drm_connector.height_mm fields are
> > > > >* filled in.
> > > > >*
> > > > > + * Note that the caller function will automatically add standard
> > > > > VESA
> > > > > + * DMT modes up to 1024x768 if the .get_modes() helper operation
> > > > > returns
> > > > > + * no mode and if the connector status is
> > > > > connector_status_connected or
> > > > > + * connector_status_unknown. There is no need to call
> > > > > + * drm_add_edid_modes() manually in that case.
> >
> > Hm calling drm_add_edid_modes if you have no edid is a bit a funny idea
> > ... Personally I'd just leave out the last sentence, I think that only
> > confuses readers. Or I'm not grasphing what you're trying to tell here.
>
> IIRC, some drivers used and desktop environments expected unknown
> rather than off for LVDS/eDP panels when the lid was shut or if the
> mux was switched to another device in the case of hybrid laptops.
We seem to have totally ditched that in
commit 05c72e77ccda89ff624108b1b59a0fc43843f343
Author: Ville Syrjälä
Date: Tue Jul 17 20:42:14 2018 +0300
drm/i915: Nuke the LVDS lid notifier
No screaming yet.
But I'm also a bit confused, for a panel there's generally an edid
around, or
[Bug 204241] amdgpu fails to resume from suspend
https://bugzilla.kernel.org/show_bug.cgi?id=204241 poinck (an...@poinck.de) changed: What|Removed |Added CC||an...@poinck.de --- Comment #62 from poinck (an...@poinck.de) --- I am having the same issue with: Platform: Linux-5.4.38-gentoo-x86_64-Intel-R-_Core-TM-i5-3570K_CPU@_3.40GHz-with-gentoo-2.6, 64bit Graphics: 01:00.0 VGA compatible controller: Advanced Micro Devices, Inc. [AMD/ATI] Baffin [Radeon RX 550 640SP / RX 560/560X] (rev cf) DE: Gnome 3.34.4 Steps to repoduce: - start qutebrowser (uses Qt-5.14.2) under Gnome - hibernate - system freezes immediatly (or just blank screen and remotely still available) or eventually blanks after resume. - restart or hard reset neccessary Workarround: - stop qutebrowser before hibernating - resume works and I can login normally and resume the session -- 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: [bug report] drm/vc4: dsi: Fix bridge chain handling
On Wed, Jun 24, 2020 at 08:30:25PM +0200, Boris Brezillon wrote:
> Hello Dan,
>
> On Wed, 24 Jun 2020 20:58:06 +0300
> Dan Carpenter wrote:
>
> > Hello Boris Brezillon,
> >
> > The patch 033bfe7538a1: "drm/vc4: dsi: Fix bridge chain handling"
> > from Dec 27, 2019, leads to the following static checker warning:
> >
> > drivers/gpu/drm/vc4/vc4_dsi.c:758 vc4_dsi_encoder_disable()
> > warn: iterator used outside loop: 'iter'
> >
> > drivers/gpu/drm/vc4/vc4_dsi.c
> >743 static void vc4_dsi_encoder_disable(struct drm_encoder *encoder)
> >744 {
> >745 struct vc4_dsi_encoder *vc4_encoder =
> > to_vc4_dsi_encoder(encoder);
> >746 struct vc4_dsi *dsi = vc4_encoder->dsi;
> >747 struct device *dev = &dsi->pdev->dev;
> >748 struct drm_bridge *iter;
> >749
> >750 list_for_each_entry_reverse(iter, &dsi->bridge_chain,
> > chain_node) {
> >751 if (iter->funcs->disable)
> >752 iter->funcs->disable(iter);
> >753 }
> >
> > This loops backwards until iter is parked on the list head.
> >
> >754
> >755 vc4_dsi_ulps(dsi, true);
> >756
> >757 list_for_each_entry_from(iter, &dsi->bridge_chain,
> > chain_node) {
> >
> > But then this "continues" until the iter is parked on the list head.
> > Since we ended with the iterator already on the list head then we never
> > enter this loop and it is a no-op.
> >
> > Am I missing something?
>
> It should definitely be list_for_each_entry() here. Thanks for
> this report. Would you mind sending a patch?
Yeah. I can do that tomorrow.
I'm working on a new Smatch check and so I wasn't super familiar with
some of these list_for_each() loops. Thanks!
regards,
dan carpenter
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [bug report] drm/vc4: dsi: Fix bridge chain handling
Hello Dan,
On Wed, 24 Jun 2020 20:58:06 +0300
Dan Carpenter wrote:
> Hello Boris Brezillon,
>
> The patch 033bfe7538a1: "drm/vc4: dsi: Fix bridge chain handling"
> from Dec 27, 2019, leads to the following static checker warning:
>
> drivers/gpu/drm/vc4/vc4_dsi.c:758 vc4_dsi_encoder_disable()
> warn: iterator used outside loop: 'iter'
>
> drivers/gpu/drm/vc4/vc4_dsi.c
>743 static void vc4_dsi_encoder_disable(struct drm_encoder *encoder)
>744 {
>745 struct vc4_dsi_encoder *vc4_encoder =
> to_vc4_dsi_encoder(encoder);
>746 struct vc4_dsi *dsi = vc4_encoder->dsi;
>747 struct device *dev = &dsi->pdev->dev;
>748 struct drm_bridge *iter;
>749
>750 list_for_each_entry_reverse(iter, &dsi->bridge_chain,
> chain_node) {
>751 if (iter->funcs->disable)
>752 iter->funcs->disable(iter);
>753 }
>
> This loops backwards until iter is parked on the list head.
>
>754
>755 vc4_dsi_ulps(dsi, true);
>756
>757 list_for_each_entry_from(iter, &dsi->bridge_chain,
> chain_node) {
>
> But then this "continues" until the iter is parked on the list head.
> Since we ended with the iterator already on the list head then we never
> enter this loop and it is a no-op.
>
> Am I missing something?
It should definitely be list_for_each_entry() here. Thanks for
this report. Would you mind sending a patch?
Regards,
Boris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[bug report] drm/vc4: dsi: Fix bridge chain handling
Hello Boris Brezillon,
The patch 033bfe7538a1: "drm/vc4: dsi: Fix bridge chain handling"
from Dec 27, 2019, leads to the following static checker warning:
drivers/gpu/drm/vc4/vc4_dsi.c:758 vc4_dsi_encoder_disable()
warn: iterator used outside loop: 'iter'
drivers/gpu/drm/vc4/vc4_dsi.c
743 static void vc4_dsi_encoder_disable(struct drm_encoder *encoder)
744 {
745 struct vc4_dsi_encoder *vc4_encoder =
to_vc4_dsi_encoder(encoder);
746 struct vc4_dsi *dsi = vc4_encoder->dsi;
747 struct device *dev = &dsi->pdev->dev;
748 struct drm_bridge *iter;
749
750 list_for_each_entry_reverse(iter, &dsi->bridge_chain,
chain_node) {
751 if (iter->funcs->disable)
752 iter->funcs->disable(iter);
753 }
This loops backwards until iter is parked on the list head.
754
755 vc4_dsi_ulps(dsi, true);
756
757 list_for_each_entry_from(iter, &dsi->bridge_chain, chain_node) {
But then this "continues" until the iter is parked on the list head.
Since we ended with the iterator already on the list head then we never
enter this loop and it is a no-op.
Am I missing something?
758 if (iter->funcs->post_disable)
759 iter->funcs->post_disable(iter);
760 }
761
762 clk_disable_unprepare(dsi->pll_phy_clock);
763 clk_disable_unprepare(dsi->escape_clock);
764 clk_disable_unprepare(dsi->pixel_clock);
765
766 pm_runtime_put(dev);
767 }
regards,
dan carpenter
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 2/6] drm: msm: a6xx: send opp instead of a frequency
Hi,
On Thu, Jun 18, 2020 at 10:52:09AM -0700, Rob Clark wrote:
> On Fri, Jun 5, 2020 at 9:26 PM Sharat Masetty wrote:
> >
> > This patch changes the plumbing to send the devfreq recommended opp rather
> > than the frequency. Also consolidate and rearrange the code in a6xx to set
> > the GPU frequency and the icc vote in preparation for the upcoming
> > changes for GPU->DDR scaling votes.
> >
> > Signed-off-by: Sharat Masetty
> > ---
> > drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 62
> > +++
> > drivers/gpu/drm/msm/adreno/a6xx_gpu.h | 2 +-
> > drivers/gpu/drm/msm/msm_gpu.c | 3 +-
> > drivers/gpu/drm/msm/msm_gpu.h | 3 +-
> > 4 files changed, 38 insertions(+), 32 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> > b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> > index 748cd37..2d8124b 100644
> > --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> > +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> > @@ -100,17 +100,30 @@ bool a6xx_gmu_gx_is_on(struct a6xx_gmu *gmu)
> > A6XX_GMU_SPTPRAC_PWR_CLK_STATUS_GX_HM_CLK_OFF));
> > }
> >
> > -static void __a6xx_gmu_set_freq(struct a6xx_gmu *gmu, int index)
> > +void a6xx_gmu_set_freq(struct msm_gpu *gpu, struct dev_pm_opp *opp)
> > {
> > - struct a6xx_gpu *a6xx_gpu = container_of(gmu, struct a6xx_gpu, gmu);
> > - struct adreno_gpu *adreno_gpu = &a6xx_gpu->base;
> > - struct msm_gpu *gpu = &adreno_gpu->base;
> > - int ret;
> > + struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
> > + struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu);
> > + struct a6xx_gmu *gmu = &a6xx_gpu->gmu;
> > + u32 perf_index;
> > + unsigned long gpu_freq;
> > + int ret = 0;
> > +
> > + gpu_freq = dev_pm_opp_get_freq(opp);
> > +
> > + if (gpu_freq == gmu->freq)
> > + return;
> > +
> > + for (perf_index = 0; perf_index < gmu->nr_gpu_freqs - 1;
> > perf_index++)
> > + if (gpu_freq == gmu->gpu_freqs[perf_index])
> > + break;
> > +
> > + gmu->current_perf_index = perf_index;
> >
> > gmu_write(gmu, REG_A6XX_GMU_DCVS_ACK_OPTION, 0);
> >
> > gmu_write(gmu, REG_A6XX_GMU_DCVS_PERF_SETTING,
> > - ((3 & 0xf) << 28) | index);
> > + ((3 & 0xf) << 28) | perf_index);
> >
> > /*
> > * Send an invalid index as a vote for the bus bandwidth and let the
> > @@ -126,7 +139,7 @@ static void __a6xx_gmu_set_freq(struct a6xx_gmu *gmu,
> > int index)
> > if (ret)
> > dev_err(gmu->dev, "GMU set GPU frequency error: %d\n", ret);
> >
> > - gmu->freq = gmu->gpu_freqs[index];
> > + gmu->freq = gmu->gpu_freqs[perf_index];
> >
> > /*
> > * Eventually we will want to scale the path vote with the
> > frequency but
> > @@ -135,25 +148,6 @@ static void __a6xx_gmu_set_freq(struct a6xx_gmu *gmu,
> > int index)
> > icc_set_bw(gpu->icc_path, 0, MBps_to_icc(7216));
> > }
> >
> > -void a6xx_gmu_set_freq(struct msm_gpu *gpu, unsigned long freq)
> > -{
> > - struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
> > - struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu);
> > - struct a6xx_gmu *gmu = &a6xx_gpu->gmu;
> > - u32 perf_index = 0;
> > -
> > - if (freq == gmu->freq)
> > - return;
> > -
> > - for (perf_index = 0; perf_index < gmu->nr_gpu_freqs - 1;
> > perf_index++)
> > - if (freq == gmu->gpu_freqs[perf_index])
> > - break;
> > -
> > - gmu->current_perf_index = perf_index;
> > -
> > - __a6xx_gmu_set_freq(gmu, perf_index);
> > -}
>
> this does end up conflicting a bit with some of the newer stuff that
> landed this cycle, in particular "drm/msm/a6xx: HFI v2 for A640 and
> A650"
>
> Adding Jonathan on CC since I think he will want to test this on
> a650/a640 as well..
Sharat, please send an updated version that is rebased on the latest drm-msm.
Thanks
Matthias
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[GIT PULL] drm/tegra: Fixes for v5.8-rc3
Hi Dave, The following changes since commit b3a9e3b9622ae10064826dccb4f7a52bd88c7407: Linux 5.8-rc1 (2020-06-14 12:45:04 -0700) are available in the Git repository at: git://anongit.freedesktop.org/tegra/linux tags/drm/tegra/for-5.8-rc3 for you to fetch changes up to fce3a51d9b31312aa12ecb72ffabfc4c7b40bdc6: drm/tegra: Add zpos property for cursor planes (2020-06-16 19:03:25 +0200) Thanks, Thierry drm/tegra: Fixes for v5.8-rc3 This contains a fairly random assortment of fixes for various minor issues. Christophe JAILLET (1): gpu: host1x: Clean up debugfs in error handling path Colton Lewis (1): gpu: host1x: Correct trivial kernel-doc inconsistencies Nicolin Chen (1): drm/tegra: hub: Do not enable orphaned window group Thierry Reding (4): gpu: host1x: Register child devices drm/tegra: hub: Register child devices gpu: host1x: Detach driver on unregister drm/tegra: Add zpos property for cursor planes drivers/gpu/drm/tegra/dc.c | 1 + drivers/gpu/drm/tegra/hub.c | 17 +++-- drivers/gpu/host1x/bus.c| 9 + drivers/gpu/host1x/dev.c| 11 +-- include/linux/host1x.h | 3 +++ 5 files changed, 37 insertions(+), 4 deletions(-) ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 0/8] Fix a bunch of W=1 warnings in Backlight
Hi Lee. On Wed, Jun 24, 2020 at 04:43:21PM +0100, Lee Jones wrote: > On Wed, 24 Jun 2020, Sam Ravnborg wrote: > > > Hi Lee. > > > > On Wed, Jun 24, 2020 at 03:57:13PM +0100, Lee Jones wrote: > > > Attempting to clean-up W=1 kernel builds, which are currently > > > overwhelmingly riddled with niggly little warnings. > > > > > > Lee Jones (8): > > > backlight: lms501kf03: Remove unused const variables > > > backlight: lcd: Add missing kerneldoc entry for 'struct device parent' > > > > > > > backlight: ili922x: Add missing kerneldoc descriptions for > > > CHECK_FREQ_REG() args > > > backlight: ili922x: Remove invalid use of kerneldoc syntax > > > backlight: ili922x: Add missing kerneldoc description for > > > ili922x_reg_dump()'s arg > > I wonder why these warnings show up as nothing pulls in this .c file. > > Anyway I would suggest to drop using kerneldoc syntax for single drivers > > like this - and the benefit here is low. > > Now they are typed, otherwise this ahd been fine in a single patch. > > What do you mean by 'nothing pulls it in'? There are no .rst files that includes any: .. kernel-doc:: drivers/video/backlight/ili922x.c so I do not see how the kernel-doc comments will be used by any of the generated kernel-docs. Sam > > > > backlight: backlight: Supply description for function args in existing > > > Kerneldocs > > > backlight: lm3630a_bl: Remove invalid checks for unsigned int < 0 > > > backlight: qcom-wled: Remove unused configs for LED3 and LED4 > > > > The other fixes looks good. > > They are all: > > Acked-by: Sam Ravnborg > > Thanks (although this should be Reviewed-by). > > > > drivers/video/backlight/backlight.c | 2 ++ > > > drivers/video/backlight/ili922x.c| 8 ++-- > > > drivers/video/backlight/lcd.c| 1 + > > > drivers/video/backlight/lm3630a_bl.c | 4 ++-- > > > drivers/video/backlight/lms501kf03.c | 8 > > > drivers/video/backlight/qcom-wled.c | 8 > > > 6 files changed, 11 insertions(+), 20 deletions(-) > > > > > -- > Lee Jones [李琼斯] > Senior Technical Lead - Developer Services > Linaro.org │ Open source software for Arm SoCs > Follow Linaro: Facebook | Twitter | Blog ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 0/8] Fix a bunch of W=1 warnings in Backlight
On Wed, 24 Jun 2020, Sam Ravnborg wrote: > Hi Lee. > > On Wed, Jun 24, 2020 at 03:57:13PM +0100, Lee Jones wrote: > > Attempting to clean-up W=1 kernel builds, which are currently > > overwhelmingly riddled with niggly little warnings. > > > > Lee Jones (8): > > backlight: lms501kf03: Remove unused const variables > > backlight: lcd: Add missing kerneldoc entry for 'struct device parent' > > > > backlight: ili922x: Add missing kerneldoc descriptions for > > CHECK_FREQ_REG() args > > backlight: ili922x: Remove invalid use of kerneldoc syntax > > backlight: ili922x: Add missing kerneldoc description for > > ili922x_reg_dump()'s arg > I wonder why these warnings show up as nothing pulls in this .c file. > Anyway I would suggest to drop using kerneldoc syntax for single drivers > like this - and the benefit here is low. > Now they are typed, otherwise this ahd been fine in a single patch. What do you mean by 'nothing pulls it in'? > > backlight: backlight: Supply description for function args in existing > > Kerneldocs > > backlight: lm3630a_bl: Remove invalid checks for unsigned int < 0 > > backlight: qcom-wled: Remove unused configs for LED3 and LED4 > > The other fixes looks good. > They are all: > Acked-by: Sam Ravnborg Thanks (although this should be Reviewed-by). > > drivers/video/backlight/backlight.c | 2 ++ > > drivers/video/backlight/ili922x.c| 8 ++-- > > drivers/video/backlight/lcd.c| 1 + > > drivers/video/backlight/lm3630a_bl.c | 4 ++-- > > drivers/video/backlight/lms501kf03.c | 8 > > drivers/video/backlight/qcom-wled.c | 8 > > 6 files changed, 11 insertions(+), 20 deletions(-) > > -- Lee Jones [李琼斯] Senior Technical Lead - Developer Services Linaro.org │ Open source software for Arm SoCs Follow Linaro: Facebook | Twitter | Blog ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2] drm/bridge: dw-mipi-dsi.c: Add VPG runtime config through debugfs
Hello Angelo,
thanks for the patch.
Tested-by: Yannick Fertre
Tested OK on STM32MP1-DISCO, DSI v1.31
Best regards
On 4/6/20 3:49 PM, Angelo Ribeiro wrote:
> Add support for the video pattern generator (VPG) BER pattern mode and
> configuration in runtime.
>
> This enables using the debugfs interface to manipulate the VPG after
> the pipeline is set.
> Also, enables the usage of the VPG BER pattern.
>
> Changes in v2:
>- Added VID_MODE_VPG_MODE
>- Solved incompatible return type on __get and __set
>
> Reported-by: kbuild test robot
> Reported-by: Adrian Pop
> Cc: Gustavo Pimentel
> Cc: Joao Pinto
> Cc: Jose Abreu
> Signed-off-by: Angelo Ribeiro
> ---
> drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 98
> ---
> 1 file changed, 90 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> index b18351b..9de3645 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> @@ -91,6 +91,7 @@
> #define VID_MODE_TYPE_BURST 0x2
> #define VID_MODE_TYPE_MASK 0x3
> #define VID_MODE_VPG_ENABLE BIT(16)
> +#define VID_MODE_VPG_MODEBIT(20)
> #define VID_MODE_VPG_HORIZONTAL BIT(24)
>
> #define DSI_VID_PKT_SIZE0x3c
> @@ -221,6 +222,21 @@
> #define PHY_STATUS_TIMEOUT_US 1
> #define CMD_PKT_STATUS_TIMEOUT_US 2
>
> +#ifdef CONFIG_DEBUG_FS
> +#define VPG_DEFS(name, dsi) \
> + ((void __force *)&((*dsi).vpg_defs.name))
> +
> +#define REGISTER(name, mask, dsi) \
> + { #name, VPG_DEFS(name, dsi), mask, dsi }
> +
> +struct debugfs_entries {
> + const char *name;
> + bool*reg;
> + u32 mask;
> + struct dw_mipi_dsi *dsi;
> +};
> +#endif /* CONFIG_DEBUG_FS */
> +
> struct dw_mipi_dsi {
> struct drm_bridge bridge;
> struct mipi_dsi_host dsi_host;
> @@ -238,9 +254,12 @@ struct dw_mipi_dsi {
>
> #ifdef CONFIG_DEBUG_FS
> struct dentry *debugfs;
> -
> - bool vpg;
> - bool vpg_horizontal;
> + struct debugfs_entries *debugfs_vpg;
> + struct {
> + bool vpg;
> + bool vpg_horizontal;
> + bool vpg_ber_pattern;
> + } vpg_defs;
> #endif /* CONFIG_DEBUG_FS */
>
> struct dw_mipi_dsi *master; /* dual-dsi master ptr */
> @@ -530,9 +549,11 @@ static void dw_mipi_dsi_video_mode_config(struct
> dw_mipi_dsi *dsi)
> val |= VID_MODE_TYPE_NON_BURST_SYNC_EVENTS;
>
> #ifdef CONFIG_DEBUG_FS
> - if (dsi->vpg) {
> + if (dsi->vpg_defs.vpg) {
> val |= VID_MODE_VPG_ENABLE;
> - val |= dsi->vpg_horizontal ? VID_MODE_VPG_HORIZONTAL : 0;
> + val |= dsi->vpg_defs.vpg_horizontal ?
> +VID_MODE_VPG_HORIZONTAL : 0;
> + val |= dsi->vpg_defs.vpg_ber_pattern ? VID_MODE_VPG_MODE : 0;
> }
> #endif /* CONFIG_DEBUG_FS */
>
> @@ -961,6 +982,68 @@ static const struct drm_bridge_funcs
> dw_mipi_dsi_bridge_funcs = {
>
> #ifdef CONFIG_DEBUG_FS
>
> +int dw_mipi_dsi_debugfs_write(void *data, u64 val)
> +{
> + struct debugfs_entries *vpg = data;
> + struct dw_mipi_dsi *dsi;
> + u32 mode_cfg;
> +
> + if (!vpg)
> + return -ENODEV;
> +
> + dsi = vpg->dsi;
> +
> + *vpg->reg = (bool)val;
> +
> + mode_cfg = dsi_read(dsi, DSI_VID_MODE_CFG);
> +
> + if (*vpg->reg)
> + mode_cfg |= vpg->mask;
> + else
> + mode_cfg &= ~vpg->mask;
> +
> + dsi_write(dsi, DSI_VID_MODE_CFG, mode_cfg);
> +
> + return 0;
> +}
> +
> +int dw_mipi_dsi_debugfs_show(void *data, u64 *val)
> +{
> + struct debugfs_entries *vpg = data;
> +
> + if (!vpg)
> + return -ENODEV;
> +
> + *val = *vpg->reg;
> +
> + return 0;
> +}
> +
> +DEFINE_DEBUGFS_ATTRIBUTE(fops_x32, dw_mipi_dsi_debugfs_show,
> + dw_mipi_dsi_debugfs_write, "%llu\n");
> +
> +static void debugfs_create_files(void *data)
> +{
> + struct dw_mipi_dsi *dsi = data;
> + struct debugfs_entries debugfs[] = {
> + REGISTER(vpg, VID_MODE_VPG_ENABLE, dsi),
> + REGISTER(vpg_horizontal, VID_MODE_VPG_HORIZONTAL, dsi),
> + REGISTER(vpg_ber_pattern, VID_MODE_VPG_MODE, dsi),
> + };
> + int i;
> +
> + dsi->debugfs_vpg = kmalloc(sizeof(debugfs), GFP_KERNEL);
> + if (!dsi->debugfs_vpg)
> + return;
> +
> + memcpy(dsi->debugfs_vpg, debugfs, sizeof(debugfs));
> +
> + for (i = 0; i < ARRAY_SIZE(debugfs); i++)
> + debugfs_create_file(dsi->debugfs_vpg[i].name, 0644,
> + dsi->debugfs, &dsi->debugfs_vpg[i],
> + &fops_x32);
> +}
> +
> static void dw_mipi_dsi_debug
Re: [PATCH 0/8] Fix a bunch of W=1 warnings in Backlight
Hi Lee. On Wed, Jun 24, 2020 at 03:57:13PM +0100, Lee Jones wrote: > Attempting to clean-up W=1 kernel builds, which are currently > overwhelmingly riddled with niggly little warnings. > > Lee Jones (8): > backlight: lms501kf03: Remove unused const variables > backlight: lcd: Add missing kerneldoc entry for 'struct device parent' > backlight: ili922x: Add missing kerneldoc descriptions for > CHECK_FREQ_REG() args > backlight: ili922x: Remove invalid use of kerneldoc syntax > backlight: ili922x: Add missing kerneldoc description for > ili922x_reg_dump()'s arg I wonder why these warnings show up as nothing pulls in this .c file. Anyway I would suggest to drop using kerneldoc syntax for single drivers like this - and the benefit here is low. Now they are typed, otherwise this ahd been fine in a single patch. > backlight: backlight: Supply description for function args in existing > Kerneldocs > backlight: lm3630a_bl: Remove invalid checks for unsigned int < 0 > backlight: qcom-wled: Remove unused configs for LED3 and LED4 The other fixes looks good. They are all: Acked-by: Sam Ravnborg Sam > drivers/video/backlight/backlight.c | 2 ++ > drivers/video/backlight/ili922x.c| 8 ++-- > drivers/video/backlight/lcd.c| 1 + > drivers/video/backlight/lm3630a_bl.c | 4 ++-- > drivers/video/backlight/lms501kf03.c | 8 > drivers/video/backlight/qcom-wled.c | 8 > 6 files changed, 11 insertions(+), 20 deletions(-) > > -- > 2.25.1 > > ___ > 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: [RESEND PATCH v5 1/5] driver core: add probe_err log helper
On Wed, Jun 24, 2020 at 04:00:34PM +0100, Robin Murphy wrote: > Be thankful... And count me in as one of those miserable users; here's one > of mine being bad enough without even printing any specific messages about > deferring ;) > [robin@weasel-cheese ~]$ dmesg | grep dwmmc > [3.046297] dwmmc_rockchip ff0c.mmc: IDMAC supports 32-bit address > mode. > [3.054312] dwmmc_rockchip ff0c.mmc: Using internal DMA controller. > [3.061774] dwmmc_rockchip ff0c.mmc: Version ID is 270a > [3.068101] dwmmc_rockchip ff0c.mmc: DW MMC controller at irq 30,32 > bit host data width,256 deep fifo Looking at that driver it's going to have lots of problems whatever happens - it's printing out a huge amount of stuff before it's finished acquiring resources. signature.asc Description: PGP signature ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 27/27] drm: Add default modes for connectors in unknown state
On Wed, Jun 24, 2020 at 3:23 AM Daniel Vetter wrote:
>
> On Wed, Jun 24, 2020 at 04:12:09AM +0300, Laurent Pinchart wrote:
> > Hi Sam,
> >
> > On Sun, Jun 21, 2020 at 10:40:00AM +0200, Sam Ravnborg wrote:
> > > On Tue, May 26, 2020 at 04:15:05AM +0300, Laurent Pinchart wrote:
> > > > The DRM CRTC helpers add default modes to connectors in the connected
> > > > state if no mode can be retrieved from the connector. This behaviour is
> > > > useful for VGA or DVI outputs that have no connected DDC bus. However,
> > > > in such cases, the status of the output usually can't be retrieved and
> > > > is reported as connector_status_unknown.
> > > >
> > > > Extend the addition of default modes to connectors in an unknown state
> > > > to support outputs that can retrieve neither the modes nor the
> > > > connection status.
> > > >
> > > > Signed-off-by: Laurent Pinchart
> > > >
> > >
> > > From your description sounds like an OK approach.
> > > But this is not something I feel too familiar with.
> > > Acked-by: Sam Ravnborg
> >
> > Thanks for the ack. I'd like to have Daniel's (CC'ed) feedback on this
> > too.
>
> Makes sense, and at least pre-coffee me can't immediately think of a
> scenario where we're going to regret this. _unknown status is pretty much
> limited to old VGA and similar things where load detect somehow isn't well
> supported by the hw.
>
> Reviewed-by: Daniel Vetter
>
> >
> > > > ---
> > > > drivers/gpu/drm/drm_probe_helper.c | 3 ++-
> > > > include/drm/drm_modeset_helper_vtables.h | 8 +++-
> > > > 2 files changed, 9 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/drm_probe_helper.c
> > > > b/drivers/gpu/drm/drm_probe_helper.c
> > > > index f5d141e0400f..9055d9573c90 100644
> > > > --- a/drivers/gpu/drm/drm_probe_helper.c
> > > > +++ b/drivers/gpu/drm/drm_probe_helper.c
> > > > @@ -491,7 +491,8 @@ int drm_helper_probe_single_connector_modes(struct
> > > > drm_connector *connector,
> > > > if (count == 0 && connector->status == connector_status_connected)
> > > > count = drm_add_override_edid_modes(connector);
> > > >
> > > > - if (count == 0 && connector->status == connector_status_connected)
> > > > + if (count == 0 && (connector->status == connector_status_connected ||
> > > > +connector->status == connector_status_unknown))
> > > > count = drm_add_modes_noedid(connector, 1024, 768);
> > > > count += drm_helper_probe_add_cmdline_mode(connector);
> > > > if (count == 0)
> > > > diff --git a/include/drm/drm_modeset_helper_vtables.h
> > > > b/include/drm/drm_modeset_helper_vtables.h
> > > > index 421a30f08463..afe55e2e93d2 100644
> > > > --- a/include/drm/drm_modeset_helper_vtables.h
> > > > +++ b/include/drm/drm_modeset_helper_vtables.h
> > > > @@ -876,13 +876,19 @@ struct drm_connector_helper_funcs {
> > > >* The usual way to implement this is to cache the EDID retrieved in
> > > > the
> > > >* probe callback somewhere in the driver-private connector structure.
> > > >* In this function drivers then parse the modes in the EDID and add
> > > > - * them by calling drm_add_edid_modes(). But connectors that driver a
> > > > + * them by calling drm_add_edid_modes(). But connectors that drive a
> > > >* fixed panel can also manually add specific modes using
> > > >* drm_mode_probed_add(). Drivers which manually add modes should also
> > > >* make sure that the &drm_connector.display_info,
> > > >* &drm_connector.width_mm and &drm_connector.height_mm fields are
> > > >* filled in.
> > > >*
> > > > + * Note that the caller function will automatically add standard VESA
> > > > + * DMT modes up to 1024x768 if the .get_modes() helper operation
> > > > returns
> > > > + * no mode and if the connector status is connector_status_connected
> > > > or
> > > > + * connector_status_unknown. There is no need to call
> > > > + * drm_add_edid_modes() manually in that case.
>
> Hm calling drm_add_edid_modes if you have no edid is a bit a funny idea
> ... Personally I'd just leave out the last sentence, I think that only
> confuses readers. Or I'm not grasphing what you're trying to tell here.
IIRC, some drivers used and desktop environments expected unknown
rather than off for LVDS/eDP panels when the lid was shut or if the
mux was switched to another device in the case of hybrid laptops.
Alex
>
> r-b with or without this change since imo super tiny nit.
>
> Cheers, Daniel
>
> > > > + *
> > > >* Virtual drivers that just want some standard VESA mode with a given
> > > >* resolution can call drm_add_modes_noedid(), and mark the preferred
> > > >* one using drm_set_preferred_mode().
> >
> > --
> > Regards,
> >
> > Laurent Pinchart
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> ___
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/lis
Re: [RESEND][PATCH v8 4/8] PM / EM: add support for other devices than CPUs in Energy Model
On Wed, Jun 10, 2020 at 12:12 PM Lukasz Luba wrote:
>
> Add support for other devices than CPUs. The registration function
> does not require a valid cpumask pointer and is ready to handle new
> devices. Some of the internal structures has been reorganized in order to
> keep consistent view (like removing per_cpu pd pointers).
>
> Signed-off-by: Lukasz Luba
> ---
> Hi all,
>
> This is just a small change compared to v8 addressing Rafael's
> comments an Dan's static analyzes.
> Here are the changes:
> - added comment about mutex usage in the unregister function
> - changed 'dev' into @dev in the kerneldoc comments
> - removed 'else' statement from em_create_pd() to calm down static analizers
I've applied the series as 5.9 material with patch [4/8] replaced with this one.
Sorry for the delays in handling this!
Thanks!
> include/linux/device.h | 5 +
> include/linux/energy_model.h | 29 -
> kernel/power/energy_model.c | 244 ---
> 3 files changed, 194 insertions(+), 84 deletions(-)
>
> diff --git a/include/linux/device.h b/include/linux/device.h
> index ac8e37cd716a..7023d3ea189b 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -13,6 +13,7 @@
> #define _DEVICE_H_
>
> #include
> +#include
> #include
> #include
> #include
> @@ -559,6 +560,10 @@ struct device {
> struct dev_pm_info power;
> struct dev_pm_domain*pm_domain;
>
> +#ifdef CONFIG_ENERGY_MODEL
> + struct em_perf_domain *em_pd;
> +#endif
> +
> #ifdef CONFIG_GENERIC_MSI_IRQ_DOMAIN
> struct irq_domain *msi_domain;
> #endif
> diff --git a/include/linux/energy_model.h b/include/linux/energy_model.h
> index 7076cb22b247..2d4689964029 100644
> --- a/include/linux/energy_model.h
> +++ b/include/linux/energy_model.h
> @@ -12,8 +12,10 @@
>
> /**
> * em_perf_state - Performance state of a performance domain
> - * @frequency: The CPU frequency in KHz, for consistency with CPUFreq
> - * @power: The power consumed by 1 CPU at this level, in milli-watts
> + * @frequency: The frequency in KHz, for consistency with CPUFreq
> + * @power: The power consumed at this level, in milli-watts (by 1 CPU or
> + by a registered device). It can be a total power: static and
> + dynamic.
> * @cost: The cost coefficient associated with this level, used during
> * energy calculation. Equal to: power * max_frequency /
> frequency
> */
> @@ -27,12 +29,16 @@ struct em_perf_state {
> * em_perf_domain - Performance domain
> * @table: List of performance states, in ascending order
> * @nr_perf_states:Number of performance states
> - * @cpus: Cpumask covering the CPUs of the domain
> + * @cpus: Cpumask covering the CPUs of the domain. It's here
> + * for performance reasons to avoid potential cache
> + * misses during energy calculations in the scheduler
> + * and simplifies allocating/freeing that memory region.
> *
> - * A "performance domain" represents a group of CPUs whose performance is
> - * scaled together. All CPUs of a performance domain must have the same
> - * micro-architecture. Performance domains often have a 1-to-1 mapping with
> - * CPUFreq policies.
> + * In case of CPU device, a "performance domain" represents a group of CPUs
> + * whose performance is scaled together. All CPUs of a performance domain
> + * must have the same micro-architecture. Performance domains often have
> + * a 1-to-1 mapping with CPUFreq policies. In case of other devices the @cpus
> + * field is unused.
> */
> struct em_perf_domain {
> struct em_perf_state *table;
> @@ -71,10 +77,12 @@ struct em_data_callback {
> #define EM_DATA_CB(_active_power_cb) { .active_power = &_active_power_cb }
>
> struct em_perf_domain *em_cpu_get(int cpu);
> +struct em_perf_domain *em_pd_get(struct device *dev);
> int em_register_perf_domain(cpumask_t *span, unsigned int nr_states,
> struct em_data_callback *cb);
> int em_dev_register_perf_domain(struct device *dev, unsigned int nr_states,
> struct em_data_callback *cb, cpumask_t *span);
> +void em_dev_unregister_perf_domain(struct device *dev);
>
> /**
> * em_pd_energy() - Estimates the energy consumed by the CPUs of a perf.
> domain
> @@ -184,10 +192,17 @@ int em_dev_register_perf_domain(struct device *dev,
> unsigned int nr_states,
> {
> return -EINVAL;
> }
> +static inline void em_dev_unregister_perf_domain(struct device *dev)
> +{
> +}
> static inline struct em_perf_domain *em_cpu_get(int cpu)
> {
> return NULL;
> }
> +static inline struct em_perf_domain *em_pd_get(struct device *dev)
> +{
> + return NULL;
> +}
> static inline unsigned long em_pd_energy(struct em_perf_domain *pd,
> unsigned long max_util, unsigne
Re: [PATCH] drm/radeon: fix array out-of-bounds read and write issues
Applied. Thanks!
Alex
On Wed, Jun 24, 2020 at 8:07 AM Colin King wrote:
>
> From: Colin Ian King
>
> There is an off-by-one bounds check on the index into arrays
> table->mc_reg_address and table->mc_reg_table_entry[k].mc_data[j] that
> can lead to reads and writes outside of arrays. Fix the bound checking
> off-by-one error.
>
> Addresses-Coverity: ("Out-of-bounds read/write")
> Fixes: cc8dbbb4f62a ("drm/radeon: add dpm support for CI dGPUs (v2)")
> Signed-off-by: Colin Ian King
> ---
> drivers/gpu/drm/radeon/ci_dpm.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/radeon/ci_dpm.c b/drivers/gpu/drm/radeon/ci_dpm.c
> index 134aa2b01f90..86ac032275bb 100644
> --- a/drivers/gpu/drm/radeon/ci_dpm.c
> +++ b/drivers/gpu/drm/radeon/ci_dpm.c
> @@ -4351,7 +4351,7 @@ static int ci_set_mc_special_registers(struct
> radeon_device *rdev,
>
> table->mc_reg_table_entry[k].mc_data[j] |= 0x100;
> }
> j++;
> - if (j > SMU7_DISCRETE_MC_REGISTER_ARRAY_SIZE)
> + if (j >= SMU7_DISCRETE_MC_REGISTER_ARRAY_SIZE)
> return -EINVAL;
>
> if (!pi->mem_gddr5) {
> --
> 2.27.0
>
> ___
> 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][next] drm/amdgpu: ensure 0 is returned for success in jpeg_v2_5_wait_for_idle
On Wed, Jun 24, 2020 at 10:54 AM Colin King wrote:
>
> From: Colin Ian King
>
> In the cases where adev->jpeg.num_jpeg_inst is zero or the condition
> adev->jpeg.harvest_config & (1 << i) is always non-zero the variable
> ret is never set to an error condition and the function returns
> an uninitialized value in ret. Since the only exit condition at
> the end if the function is a success then explicitly return
> 0 rather than a potentially uninitialized value in ret.
We should actually never hit this condition in practice because the
driver won't initialize this module if all of the instances are
harvested, but better safe than sorry. Applied.
Thanks,
Alex
>
> Addresses-Coverity: ("Uninitialized scalar variable")
> Fixes: 14f43e8f88c5 ("drm/amdgpu: move JPEG2.5 out from VCN2.5")
> Signed-off-by: Colin Ian King
> ---
> drivers/gpu/drm/amd/amdgpu/jpeg_v2_5.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/jpeg_v2_5.c
> b/drivers/gpu/drm/amd/amdgpu/jpeg_v2_5.c
> index f74262a22a16..7a51c615d22d 100644
> --- a/drivers/gpu/drm/amd/amdgpu/jpeg_v2_5.c
> +++ b/drivers/gpu/drm/amd/amdgpu/jpeg_v2_5.c
> @@ -462,7 +462,7 @@ static int jpeg_v2_5_wait_for_idle(void *handle)
> return ret;
> }
>
> - return ret;
> + return 0;
> }
>
> static int jpeg_v2_5_set_clockgating_state(void *handle,
> --
> 2.27.0
>
> ___
> 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: [RESEND PATCH v5 3/5] drivers core: allow probe_err accept integer and pointer types
On 2020-06-24 16:04, Mark Brown wrote: On Wed, Jun 24, 2020 at 03:25:33PM +0100, Robin Murphy wrote: And yeah, anyone who pipes up suggesting that places where an ERR_PTR value could be passed to probe_err() could simply refactor IS_ERR() checks with more uses of the god-awful PTR_ERR_OR_ZERO() obfuscator gets a long stare of disapproval... We could also have a probe_err_ptr() or something that took an ERR_PTR() instead if there really were an issue with explicitly doing this. Yeah, for all my lyrical objection, a static inline _ptr_err() helper to wrap _err() with sensible type checking might actually be an OK compromise if people really feel strongly for having that utility. (and then we can debate whether it should also convert NULL to -ENOMEM and !IS_ERR to 0... :D) Robin. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH][next] drm: amdgpu: fix premature goto because of missing braces
Applied. Thanks!
Alex
On Wed, Jun 24, 2020 at 10:32 AM Nirmoy wrote:
>
> Acked-by: Nirmoy Das
>
>
> Thanks,
>
> Nirmoy
>
> On 6/24/20 4:14 PM, Colin King wrote:
> > From: Colin Ian King
> >
> > Currently the goto statement is skipping over a lot of setup code
> > because it is outside of an if-block and should be inside it. Fix
> > this by adding missing if statement braces.
> >
> > Addresses-Coverity: ("Structurally dead code")
> > Fixes: fd151ca5396d ("drm amdgpu: SI UVD v3_1")
> > Signed-off-by: Colin Ian King
> > ---
> > drivers/gpu/drm/amd/amdgpu/uvd_v3_1.c | 5 +++--
> > 1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/uvd_v3_1.c
> > b/drivers/gpu/drm/amd/amdgpu/uvd_v3_1.c
> > index 599719e89c31..7cf4b11a65c5 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/uvd_v3_1.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/uvd_v3_1.c
> > @@ -642,9 +642,10 @@ static int uvd_v3_1_hw_init(void *handle)
> > uvd_v3_1_start(adev);
> >
> > r = amdgpu_ring_test_helper(ring);
> > - if (r)
> > + if (r) {
> > DRM_ERROR("amdgpu: UVD ring test fail (%d).\n", r);
> > - goto done;
> > + goto done;
> > + }
> >
> > r = amdgpu_ring_alloc(ring, 10);
> > if (r) {
> ___
> amd-gfx mailing list
> amd-...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/bridge: dw-mipi-dsi.c: remove unused header file
Hello Angelo, thank for patch. Reviewed-by: Yannick Fertre On 4/3/20 3:30 PM, Angelo Ribeiro wrote: > dw-mipi-dsi does not use any definition from drm_probe_helper. > > Coverity output: > Event unnecessary_header: > Including .../include/drm/drm_probe_helper.h does not provide any > needed symbols. > > Cc: Gustavo Pimentel > Cc: Joao Pinto > Cc: Jose Abreu > Signed-off-by: Angelo Ribeiro > --- > drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c > b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c > index 024acad..582635d 100644 > --- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c > +++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c > @@ -27,7 +27,6 @@ > #include > #include > #include > -#include > > #define HWVER_131 0x31333100 /* IP version 1.31 */ > > ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 8/8] backlight: qcom-wled: Remove unused configs for LED3 and LED4
Fixes W=1 warnings:
drivers/video/backlight/qcom-wled.c:1294:34: warning: ‘wled4_string_cfg’
defined but not used [-Wunused-const-variable=]
1294 | static const struct wled_var_cfg wled4_string_cfg = {
| ^~~~
drivers/video/backlight/qcom-wled.c:1290:34: warning: ‘wled3_string_cfg’
defined but not used [-Wunused-const-variable=]
1290 | static const struct wled_var_cfg wled3_string_cfg = {
| ^~~~
Cc:
Cc: Andy Gross
Cc: Bjorn Andersson
Cc: Bartlomiej Zolnierkiewicz
Cc: linux-arm-...@vger.kernel.org
Signed-off-by: Lee Jones
---
drivers/video/backlight/qcom-wled.c | 8
1 file changed, 8 deletions(-)
diff --git a/drivers/video/backlight/qcom-wled.c
b/drivers/video/backlight/qcom-wled.c
index 4c8c34b994414..c25c31199952c 100644
--- a/drivers/video/backlight/qcom-wled.c
+++ b/drivers/video/backlight/qcom-wled.c
@@ -1287,14 +1287,6 @@ static const struct wled_var_cfg
wled4_string_i_limit_cfg = {
.size = ARRAY_SIZE(wled4_string_i_limit_values),
};
-static const struct wled_var_cfg wled3_string_cfg = {
- .size = 8,
-};
-
-static const struct wled_var_cfg wled4_string_cfg = {
- .size = 16,
-};
-
static const struct wled_var_cfg wled5_mod_sel_cfg = {
.size = 2,
};
--
2.25.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 7/8] backlight: lm3630a_bl: Remove invalid checks for unsigned int < 0
unsigned ints 'sources' and 'bank' cannot be less than LM3630A_SINK_0 (0)
and LM3630A_BANK_0 (0) respecitively, so change the logic to only check
for thier two possible valid values.
Fixes W=1 warnings:
drivers/video/backlight/lm3630a_bl.c: In function ‘lm3630a_parse_led_sources’:
drivers/video/backlight/lm3630a_bl.c:394:18: warning: comparison of unsigned
expression < 0 is always false [-Wtype-limits]
394 | if (sources[i] < LM3630A_SINK_0 || sources[i] > LM3630A_SINK_1)
| ^
drivers/video/backlight/lm3630a_bl.c: In function ‘lm3630a_parse_bank’:
drivers/video/backlight/lm3630a_bl.c:415:11: warning: comparison of unsigned
expression < 0 is always false [-Wtype-limits]
415 | if (bank < LM3630A_BANK_0 || bank > LM3630A_BANK_1)
| ^
Cc:
Cc: Bartlomiej Zolnierkiewicz
Cc: Daniel Jeong
Cc: LDD MLP
Signed-off-by: Lee Jones
---
drivers/video/backlight/lm3630a_bl.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/video/backlight/lm3630a_bl.c
b/drivers/video/backlight/lm3630a_bl.c
index ee320883b7108..e88a2b0e59046 100644
--- a/drivers/video/backlight/lm3630a_bl.c
+++ b/drivers/video/backlight/lm3630a_bl.c
@@ -391,7 +391,7 @@ static int lm3630a_parse_led_sources(struct fwnode_handle
*node,
return ret;
for (i = 0; i < num_sources; i++) {
- if (sources[i] < LM3630A_SINK_0 || sources[i] > LM3630A_SINK_1)
+ if (sources[i] != LM3630A_SINK_0 && sources[i] !=
LM3630A_SINK_1)
return -EINVAL;
ret |= BIT(sources[i]);
@@ -412,7 +412,7 @@ static int lm3630a_parse_bank(struct lm3630a_platform_data
*pdata,
if (ret)
return ret;
- if (bank < LM3630A_BANK_0 || bank > LM3630A_BANK_1)
+ if (bank != LM3630A_BANK_0 && bank != LM3630A_BANK_1)
return -EINVAL;
led_sources = lm3630a_parse_led_sources(node, BIT(bank));
--
2.25.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 3/8] backlight: ili922x: Add missing kerneldoc descriptions for CHECK_FREQ_REG() args
Kerneldoc syntax is used, but not complete. Descriptions required.
Prevents warnings like:
drivers/video/backlight/ili922x.c:116: warning: Function parameter or member
's' not described in 'CHECK_FREQ_REG'
drivers/video/backlight/ili922x.c:116: warning: Function parameter or member
'x' not described in 'CHECK_FREQ_REG'
Cc:
Cc: Bartlomiej Zolnierkiewicz
Cc: Software Engineering
Signed-off-by: Lee Jones
---
drivers/video/backlight/ili922x.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/video/backlight/ili922x.c
b/drivers/video/backlight/ili922x.c
index 9c5aa3fbb2842..8cb4b9d3c3bba 100644
--- a/drivers/video/backlight/ili922x.c
+++ b/drivers/video/backlight/ili922x.c
@@ -107,6 +107,8 @@
* lower frequency when the registers are read/written.
* The macro sets the frequency in the spi_transfer structure if
* the frequency exceeds the maximum value.
+ * @s: pointer to controller side proxy for an SPI slave device
+ * @x: pointer to the read/write buffer pair
*/
#define CHECK_FREQ_REG(s, x) \
do {\
--
2.25.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 1/8] backlight: lms501kf03: Remove unused const variables
W=1 kernel build reports:
drivers/video/backlight/lms501kf03.c:96:28: warning: ‘seq_sleep_in’ defined
but not used [-Wunused-const-variable=]
96 | static const unsigned char seq_sleep_in[] = {
| ^~~~
drivers/video/backlight/lms501kf03.c:92:28: warning: ‘seq_up_dn’ defined but
not used [-Wunused-const-variable=]
92 | static const unsigned char seq_up_dn[] = {
| ^
Either 'seq_sleep_in' nor 'seq_up_dn' have been used since the
driver first landed in 2013.
Cc:
Cc: Bartlomiej Zolnierkiewicz
Signed-off-by: Lee Jones
---
drivers/video/backlight/lms501kf03.c | 8
1 file changed, 8 deletions(-)
diff --git a/drivers/video/backlight/lms501kf03.c
b/drivers/video/backlight/lms501kf03.c
index 8ae32e3573c1a..c1bd02bb8b2ee 100644
--- a/drivers/video/backlight/lms501kf03.c
+++ b/drivers/video/backlight/lms501kf03.c
@@ -89,14 +89,6 @@ static const unsigned char seq_rgb_gamma[] = {
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
};
-static const unsigned char seq_up_dn[] = {
- 0x36, 0x10,
-};
-
-static const unsigned char seq_sleep_in[] = {
- 0x10,
-};
-
static const unsigned char seq_sleep_out[] = {
0x11,
};
--
2.25.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 0/8] Fix a bunch of W=1 warnings in Backlight
Attempting to clean-up W=1 kernel builds, which are currently overwhelmingly riddled with niggly little warnings. Lee Jones (8): backlight: lms501kf03: Remove unused const variables backlight: lcd: Add missing kerneldoc entry for 'struct device parent' backlight: ili922x: Add missing kerneldoc descriptions for CHECK_FREQ_REG() args backlight: ili922x: Remove invalid use of kerneldoc syntax backlight: ili922x: Add missing kerneldoc description for ili922x_reg_dump()'s arg backlight: backlight: Supply description for function args in existing Kerneldocs backlight: lm3630a_bl: Remove invalid checks for unsigned int < 0 backlight: qcom-wled: Remove unused configs for LED3 and LED4 drivers/video/backlight/backlight.c | 2 ++ drivers/video/backlight/ili922x.c| 8 ++-- drivers/video/backlight/lcd.c| 1 + drivers/video/backlight/lm3630a_bl.c | 4 ++-- drivers/video/backlight/lms501kf03.c | 8 drivers/video/backlight/qcom-wled.c | 8 6 files changed, 11 insertions(+), 20 deletions(-) -- 2.25.1 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 4/8] backlight: ili922x: Remove invalid use of kerneldoc syntax
Kerneldoc is for documenting function arguments and return values.
Prevents warnings like:
drivers/video/backlight/ili922x.c:127: warning: cannot understand function
prototype: 'int ili922x_id = 1; '
drivers/video/backlight/ili922x.c:136: warning: cannot understand function
prototype: 'struct ili922x '
Cc:
Cc: Bartlomiej Zolnierkiewicz
Cc: Software Engineering
Signed-off-by: Lee Jones
---
drivers/video/backlight/ili922x.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/video/backlight/ili922x.c
b/drivers/video/backlight/ili922x.c
index 8cb4b9d3c3bba..cd41433b87aeb 100644
--- a/drivers/video/backlight/ili922x.c
+++ b/drivers/video/backlight/ili922x.c
@@ -123,7 +123,7 @@
#define set_tx_byte(b) (tx_invert ? ~(b) : b)
-/**
+/*
* ili922x_id - id as set by manufacturer
*/
static int ili922x_id = 1;
@@ -132,7 +132,7 @@ module_param(ili922x_id, int, 0);
static int tx_invert;
module_param(tx_invert, int, 0);
-/**
+/*
* driver's private structure
*/
struct ili922x {
--
2.25.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 6/8] backlight: backlight: Supply description for function args in existing Kerneldocs
Kerneldoc syntax is used, but not complete. Descriptions required. Prevents warnings like: drivers/video/backlight/backlight.c:329: warning: Function parameter or member 'reason' not described in 'backlight_force_update' drivers/video/backlight/backlight.c:354: warning: Function parameter or member 'props' not described in 'backlight_device_register' Cc: Cc: Bartlomiej Zolnierkiewicz Cc: Jamey Hicks Cc: Andrew Zabolotny Signed-off-by: Lee Jones --- drivers/video/backlight/backlight.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c index 92d80aa0c0ef1..744ba58488e01 100644 --- a/drivers/video/backlight/backlight.c +++ b/drivers/video/backlight/backlight.c @@ -320,6 +320,7 @@ ATTRIBUTE_GROUPS(bl_device); * backlight_force_update - tell the backlight subsystem that hardware state * has changed * @bd: the backlight device to update + * @reason: reason for update * * Updates the internal state of the backlight in response to a hardware event, * and generate a uevent to notify userspace @@ -344,6 +345,7 @@ EXPORT_SYMBOL(backlight_force_update); * @devdata: an optional pointer to be stored for private driver use. The * methods may retrieve it by using bl_get_data(bd). * @ops: the backlight operations structure. + * @props: pointer to backlight's properties structure. * * Creates and registers new backlight device. Returns either an * ERR_PTR() or a pointer to the newly allocated device. -- 2.25.1 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 2/8] backlight: lcd: Add missing kerneldoc entry for 'struct device parent'
This has been missing since the conversion to 'struct device' in 2007. Cc: Cc: Bartlomiej Zolnierkiewicz Cc: Jamey Hicks Cc: Andrew Zabolotny Signed-off-by: Lee Jones --- drivers/video/backlight/lcd.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/video/backlight/lcd.c b/drivers/video/backlight/lcd.c index 78b0333586258..db56e465aaff3 100644 --- a/drivers/video/backlight/lcd.c +++ b/drivers/video/backlight/lcd.c @@ -179,6 +179,7 @@ ATTRIBUTE_GROUPS(lcd_device); * lcd_device_register - register a new object of lcd_device class. * @name: the name of the new object(must be the same as the name of the * respective framebuffer device). + * @parent: pointer to the parent's struct device . * @devdata: an optional pointer to be stored in the device. The * methods may retrieve it by using lcd_get_data(ld). * @ops: the lcd operations structure. -- 2.25.1 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 5/8] backlight: ili922x: Add missing kerneldoc description for ili922x_reg_dump()'s arg
Kerneldoc syntax is used, but not complete. Descriptions required.
Prevents warnings like:
drivers/video/backlight/ili922x.c:298: warning: Function parameter or member
'spi' not described in 'ili922x_reg_dump'
Cc:
Cc: Bartlomiej Zolnierkiewicz
Cc: Software Engineering
Signed-off-by: Lee Jones
---
drivers/video/backlight/ili922x.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/video/backlight/ili922x.c
b/drivers/video/backlight/ili922x.c
index cd41433b87aeb..26193f38234e7 100644
--- a/drivers/video/backlight/ili922x.c
+++ b/drivers/video/backlight/ili922x.c
@@ -295,6 +295,8 @@ static int ili922x_write(struct spi_device *spi, u8 reg,
u16 value)
#ifdef DEBUG
/**
* ili922x_reg_dump - dump all registers
+ *
+ * @spi: pointer to the controller side proxy for an SPI slave device
*/
static void ili922x_reg_dump(struct spi_device *spi)
{
--
2.25.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RESEND PATCH v5 3/5] drivers core: allow probe_err accept integer and pointer types
On Wed, Jun 24, 2020 at 03:25:33PM +0100, Robin Murphy wrote: > And yeah, anyone who pipes up suggesting that places where an ERR_PTR value > could be passed to probe_err() could simply refactor IS_ERR() checks with > more uses of the god-awful PTR_ERR_OR_ZERO() obfuscator gets a long stare of > disapproval... We could also have a probe_err_ptr() or something that took an ERR_PTR() instead if there really were an issue with explicitly doing this. signature.asc Description: PGP signature ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RESEND PATCH v5 1/5] driver core: add probe_err log helper
On 2020-06-24 15:02, Mark Brown wrote: > On Wed, Jun 24, 2020 at 04:45:28PM +0300, Andy Shevchenko wrote: >> On Wed, Jun 24, 2020 at 4:27 PM Mark Brown wrote: > >>> As I said down the thread that's not a great pattern since it means that >>> probe deferral errors never get displayed and users have a hard time >>> figuring out why their driver isn't instantiating. > >> Don't we have a file in the debugfs to list deferred drivers? > > Part of what this patch series aims to solve is that that list is not > very useful since it doesn't provide any information on how things got > deferred which means it's of no use in trying to figure out any > problems. > >> In the case of deferred probes the errors out of it makes users more >> miserable in order to look through tons of spam and lose really useful >> data in the logs. > > I seem to never manage to end up using any of the systems which generate > excessive deferrals. Be thankful... And count me in as one of those miserable users; here's one of mine being bad enough without even printing any specific messages about deferring ;) Robin. - [robin@weasel-cheese ~]$ dmesg | grep dwmmc [3.046297] dwmmc_rockchip ff0c.mmc: IDMAC supports 32-bit address mode. [3.054312] dwmmc_rockchip ff0c.mmc: Using internal DMA controller. [3.061774] dwmmc_rockchip ff0c.mmc: Version ID is 270a [3.068101] dwmmc_rockchip ff0c.mmc: DW MMC controller at irq 30,32 bit host data width,256 deep fifo [3.079638] dwmmc_rockchip ff0d.mmc: IDMAC supports 32-bit address mode. [3.087678] dwmmc_rockchip ff0d.mmc: Using internal DMA controller. [3.095134] dwmmc_rockchip ff0d.mmc: Version ID is 270a [3.101480] dwmmc_rockchip ff0d.mmc: DW MMC controller at irq 31,32 bit host data width,256 deep fifo [3.113071] dwmmc_rockchip ff0f.mmc: IDMAC supports 32-bit address mode. [3.121110] dwmmc_rockchip ff0f.mmc: Using internal DMA controller. [3.128565] dwmmc_rockchip ff0f.mmc: Version ID is 270a [3.134886] dwmmc_rockchip ff0f.mmc: DW MMC controller at irq 32,32 bit host data width,256 deep fifo [3.948510] dwmmc_rockchip ff0c.mmc: IDMAC supports 32-bit address mode. [3.956475] dwmmc_rockchip ff0c.mmc: Using internal DMA controller. [3.963884] dwmmc_rockchip ff0c.mmc: Version ID is 270a [3.970133] dwmmc_rockchip ff0c.mmc: DW MMC controller at irq 30,32 bit host data width,256 deep fifo [4.141231] dwmmc_rockchip ff0d.mmc: IDMAC supports 32-bit address mode. [4.149178] dwmmc_rockchip ff0d.mmc: Using internal DMA controller. [4.156582] dwmmc_rockchip ff0d.mmc: Version ID is 270a [4.162823] dwmmc_rockchip ff0d.mmc: DW MMC controller at irq 31,32 bit host data width,256 deep fifo [4.175606] dwmmc_rockchip ff0f.mmc: IDMAC supports 32-bit address mode. [4.183540] dwmmc_rockchip ff0f.mmc: Using internal DMA controller. [4.190946] dwmmc_rockchip ff0f.mmc: Version ID is 270a [4.197196] dwmmc_rockchip ff0f.mmc: DW MMC controller at irq 32,32 bit host data width,256 deep fifo [4.250758] dwmmc_rockchip ff0d.mmc: IDMAC supports 32-bit address mode. [4.258688] dwmmc_rockchip ff0d.mmc: Using internal DMA controller. [4.266104] dwmmc_rockchip ff0d.mmc: Version ID is 270a [4.272358] dwmmc_rockchip ff0d.mmc: DW MMC controller at irq 31,32 bit host data width,256 deep fifo [4.285390] dwmmc_rockchip ff0d.mmc: IDMAC supports 32-bit address mode. [4.29] dwmmc_rockchip ff0d.mmc: Using internal DMA controller. [4.300750] dwmmc_rockchip ff0d.mmc: Version ID is 270a [4.307005] dwmmc_rockchip ff0d.mmc: DW MMC controller at irq 31,32 bit host data width,256 deep fifo [4.971373] dwmmc_rockchip ff0f.mmc: Successfully tuned phase to 134 [5.027225] dwmmc_rockchip ff0d.mmc: IDMAC supports 32-bit address mode. [5.035339] dwmmc_rockchip ff0d.mmc: Using internal DMA controller. [5.042769] dwmmc_rockchip ff0d.mmc: Version ID is 270a [5.049050] dwmmc_rockchip ff0d.mmc: DW MMC controller at irq 31,32 bit host data width,256 deep fifo [ 24.727583] dwmmc_rockchip ff0d.mmc: IDMAC supports 32-bit address mode. [ 24.745541] dwmmc_rockchip ff0d.mmc: Using internal DMA controller. [ 24.753003] dwmmc_rockchip ff0d.mmc: Version ID is 270a [ 24.763289] dwmmc_rockchip ff0d.mmc: DW MMC controller at irq 31,32 bit host data width,256 deep fifo [ 25.589620] dwmmc_rockchip ff0d.mmc: IDMAC supports 32-bit address mode. [ 25.603066] dwmmc_rockchip ff0d.mmc: Using internal DMA controller. [ 25.615283] dwmmc_rockchip ff0d.mmc: Version ID is 270a [ 25.627911] dwmmc_rockchip ff0d.mmc: DW MMC controller at irq 31,32 bit host data width,256 deep fifo [ 25.643469] dwmmc_rockchip ff0d.mmc: IDMAC supports 32-bit address mode. [ 25.651532] dwmmc_rockchip ff0d.mmc: Using internal DMA controller. [
Re: [RESEND PATCH v5 3/5] drivers core: allow probe_err accept integer and pointer types
On Wed, Jun 24, 2020 at 4:44 PM Andrzej Hajda wrote: > > > On 24.06.2020 14:14, Rafael J. Wysocki wrote: > > On Wed, Jun 24, 2020 at 1:41 PM Andrzej Hajda wrote: > >> Many resource acquisition functions return error value encapsulated in > >> pointer instead of integer value. To simplify coding we can use macro > >> which will accept both types of error. > >> With this patch user can use: > >> probe_err(dev, ptr, ...) > >> instead of: > >> probe_err(dev, PTR_ERR(ptr), ...) > >> Without loosing old functionality: > >> probe_err(dev, err, ...) > >> > >> Signed-off-by: Andrzej Hajda > > The separation of this change from patch [1/5] looks kind of artificial to > > me. > > > > You are introducing a new function anyway, so why not to make it what > > you want right away? > > > Two reasons: > > 1.This patch is my recent idea, I didn't want to mix it with already > reviewed code. > > 2. This patch could be treated hacky by some devs due to macro > definition and type-casting. Fair enough. There is some opposition against the $subject one, so I guess it may be dropped even. Thanks! ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH][next] drm/amdgpu: ensure 0 is returned for success in jpeg_v2_5_wait_for_idle
From: Colin Ian King
In the cases where adev->jpeg.num_jpeg_inst is zero or the condition
adev->jpeg.harvest_config & (1 << i) is always non-zero the variable
ret is never set to an error condition and the function returns
an uninitialized value in ret. Since the only exit condition at
the end if the function is a success then explicitly return
0 rather than a potentially uninitialized value in ret.
Addresses-Coverity: ("Uninitialized scalar variable")
Fixes: 14f43e8f88c5 ("drm/amdgpu: move JPEG2.5 out from VCN2.5")
Signed-off-by: Colin Ian King
---
drivers/gpu/drm/amd/amdgpu/jpeg_v2_5.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/jpeg_v2_5.c
b/drivers/gpu/drm/amd/amdgpu/jpeg_v2_5.c
index f74262a22a16..7a51c615d22d 100644
--- a/drivers/gpu/drm/amd/amdgpu/jpeg_v2_5.c
+++ b/drivers/gpu/drm/amd/amdgpu/jpeg_v2_5.c
@@ -462,7 +462,7 @@ static int jpeg_v2_5_wait_for_idle(void *handle)
return ret;
}
- return ret;
+ return 0;
}
static int jpeg_v2_5_set_clockgating_state(void *handle,
--
2.27.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RESEND PATCH v5 3/5] drivers core: allow probe_err accept integer and pointer types
On 24.06.2020 14:30, Greg Kroah-Hartman wrote:
> On Wed, Jun 24, 2020 at 01:41:25PM +0200, Andrzej Hajda wrote:
>> Many resource acquisition functions return error value encapsulated in
>> pointer instead of integer value. To simplify coding we can use macro
>> which will accept both types of error.
>> With this patch user can use:
>> probe_err(dev, ptr, ...)
>> instead of:
>> probe_err(dev, PTR_ERR(ptr), ...)
>> Without loosing old functionality:
>> probe_err(dev, err, ...)
>>
>> Signed-off-by: Andrzej Hajda
>> ---
>> drivers/base/core.c| 25 ++---
>> include/linux/device.h | 25 -
>> 2 files changed, 26 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/base/core.c b/drivers/base/core.c
>> index 2a96954d5460..df283c62d9c0 100644
>> --- a/drivers/base/core.c
>> +++ b/drivers/base/core.c
>> @@ -3953,28 +3953,7 @@ define_dev_printk_level(_dev_info, KERN_INFO);
>>
>> #endif
>>
>> -/**
>> - * probe_err - probe error check and log helper
>> - * @dev: the pointer to the struct device
>> - * @err: error value to test
>> - * @fmt: printf-style format string
>> - * @...: arguments as specified in the format string
>> - *
>> - * This helper implements common pattern present in probe functions for
>> error
>> - * checking: print message if the error is not -EPROBE_DEFER and propagate
>> it.
>> - * In case of -EPROBE_DEFER it sets defer probe reason, which can be checked
>> - * later by reading devices_deferred debugfs attribute.
>> - * It replaces code sequence:
>> - * if (err != -EPROBE_DEFER)
>> - * dev_err(dev, ...);
>> - * return err;
>> - * with
>> - * return probe_err(dev, err, ...);
>> - *
>> - * Returns @err.
>> - *
>> - */
>> -int probe_err(const struct device *dev, int err, const char *fmt, ...)
>> +int __probe_err(const struct device *dev, int err, const char *fmt, ...)
>> {
>> struct va_format vaf;
>> va_list args;
>> @@ -3992,7 +3971,7 @@ int probe_err(const struct device *dev, int err, const
>> char *fmt, ...)
>>
>> return err;
>> }
>> -EXPORT_SYMBOL_GPL(probe_err);
>> +EXPORT_SYMBOL_GPL(__probe_err);
>>
>> static inline bool fwnode_is_primary(struct fwnode_handle *fwnode)
>> {
>> diff --git a/include/linux/device.h b/include/linux/device.h
>> index 40a90d9bf799..22d3c3d4f461 100644
>> --- a/include/linux/device.h
>> +++ b/include/linux/device.h
>> @@ -965,7 +965,30 @@ void device_links_supplier_sync_state_pause(void);
>> void device_links_supplier_sync_state_resume(void);
>>
>> extern __printf(3, 4)
>> -int probe_err(const struct device *dev, int err, const char *fmt, ...);
>> +int __probe_err(const struct device *dev, int err, const char *fmt, ...);
>> +
>> +/**
>> + * probe_err - probe error check and log helper
>> + * @dev: the pointer to the struct device
>> + * @err: error value to test, can be integer or pointer type
>> + * @fmt: printf-style format string
>> + * @...: arguments as specified in the format string
>> + *
>> + * This helper implements common pattern present in probe functions for
>> error
>> + * checking: print message if the error is not -EPROBE_DEFER and propagate
>> it.
>> + * In case of -EPROBE_DEFER it sets defer probe reason, which can be checked
>> + * later by reading devices_deferred debugfs attribute.
>> + * It replaces code sequence:
>> + * if (err != -EPROBE_DEFER)
>> + * dev_err(dev, ...);
>> + * return err;
>> + * with
>> + * return probe_err(dev, err, ...);
>> + *
>> + * Returns @err.
>> + *
>> + */
>> +#define probe_err(dev, err, args...) __probe_err(dev, (long)(err), args)
> Shouldn't that be "unsigned long" instead of "long"? That's what we put
> pointers in last I looked...
Unless we know this is error inside pointer, in such case we follow
practice from PTR_ERR function.
Regards
Andrzej
>
> thanks,
>
> greg k-h
> ___
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://protect2.fireeye.com/url?k=75712e41-28bf2f92-7570a50e-000babff317b-a5a76e98e30aecc2&q=1&u=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel
>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RESEND PATCH v5 3/5] drivers core: allow probe_err accept integer and pointer types
On 24.06.2020 14:14, Rafael J. Wysocki wrote:
> On Wed, Jun 24, 2020 at 1:41 PM Andrzej Hajda wrote:
>> Many resource acquisition functions return error value encapsulated in
>> pointer instead of integer value. To simplify coding we can use macro
>> which will accept both types of error.
>> With this patch user can use:
>> probe_err(dev, ptr, ...)
>> instead of:
>> probe_err(dev, PTR_ERR(ptr), ...)
>> Without loosing old functionality:
>> probe_err(dev, err, ...)
>>
>> Signed-off-by: Andrzej Hajda
> The separation of this change from patch [1/5] looks kind of artificial to me.
>
> You are introducing a new function anyway, so why not to make it what
> you want right away?
Two reasons:
1.This patch is my recent idea, I didn't want to mix it with already
reviewed code.
2. This patch could be treated hacky by some devs due to macro
definition and type-casting.
If both patches are OK I can merge them of course into one.
Regards
Andrzej
>
>> ---
>> drivers/base/core.c| 25 ++---
>> include/linux/device.h | 25 -
>> 2 files changed, 26 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/base/core.c b/drivers/base/core.c
>> index 2a96954d5460..df283c62d9c0 100644
>> --- a/drivers/base/core.c
>> +++ b/drivers/base/core.c
>> @@ -3953,28 +3953,7 @@ define_dev_printk_level(_dev_info, KERN_INFO);
>>
>> #endif
>>
>> -/**
>> - * probe_err - probe error check and log helper
>> - * @dev: the pointer to the struct device
>> - * @err: error value to test
>> - * @fmt: printf-style format string
>> - * @...: arguments as specified in the format string
>> - *
>> - * This helper implements common pattern present in probe functions for
>> error
>> - * checking: print message if the error is not -EPROBE_DEFER and propagate
>> it.
>> - * In case of -EPROBE_DEFER it sets defer probe reason, which can be checked
>> - * later by reading devices_deferred debugfs attribute.
>> - * It replaces code sequence:
>> - * if (err != -EPROBE_DEFER)
>> - * dev_err(dev, ...);
>> - * return err;
>> - * with
>> - * return probe_err(dev, err, ...);
>> - *
>> - * Returns @err.
>> - *
>> - */
>> -int probe_err(const struct device *dev, int err, const char *fmt, ...)
>> +int __probe_err(const struct device *dev, int err, const char *fmt, ...)
>> {
>> struct va_format vaf;
>> va_list args;
>> @@ -3992,7 +3971,7 @@ int probe_err(const struct device *dev, int err, const
>> char *fmt, ...)
>>
>> return err;
>> }
>> -EXPORT_SYMBOL_GPL(probe_err);
>> +EXPORT_SYMBOL_GPL(__probe_err);
>>
>> static inline bool fwnode_is_primary(struct fwnode_handle *fwnode)
>> {
>> diff --git a/include/linux/device.h b/include/linux/device.h
>> index 40a90d9bf799..22d3c3d4f461 100644
>> --- a/include/linux/device.h
>> +++ b/include/linux/device.h
>> @@ -965,7 +965,30 @@ void device_links_supplier_sync_state_pause(void);
>> void device_links_supplier_sync_state_resume(void);
>>
>> extern __printf(3, 4)
>> -int probe_err(const struct device *dev, int err, const char *fmt, ...);
>> +int __probe_err(const struct device *dev, int err, const char *fmt, ...);
>> +
>> +/**
>> + * probe_err - probe error check and log helper
>> + * @dev: the pointer to the struct device
>> + * @err: error value to test, can be integer or pointer type
>> + * @fmt: printf-style format string
>> + * @...: arguments as specified in the format string
>> + *
>> + * This helper implements common pattern present in probe functions for
>> error
>> + * checking: print message if the error is not -EPROBE_DEFER and propagate
>> it.
>> + * In case of -EPROBE_DEFER it sets defer probe reason, which can be checked
>> + * later by reading devices_deferred debugfs attribute.
>> + * It replaces code sequence:
>> + * if (err != -EPROBE_DEFER)
>> + * dev_err(dev, ...);
>> + * return err;
>> + * with
>> + * return probe_err(dev, err, ...);
>> + *
>> + * Returns @err.
>> + *
>> + */
>> +#define probe_err(dev, err, args...) __probe_err(dev, (long)(err), args)
>>
>> /* Create alias, so I can be autoloaded. */
>> #define MODULE_ALIAS_CHARDEV(major,minor) \
>> --
>> 2.17.1
>>
> ___
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://protect2.fireeye.com/url?k=fe383567-a3a29cc4-fe39be28-002590f5b904-1faeb9cf68e31587&q=1&u=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel
>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH][next] drm: amdgpu: fix premature goto because of missing braces
Acked-by: Nirmoy Das
Thanks,
Nirmoy
On 6/24/20 4:14 PM, Colin King wrote:
From: Colin Ian King
Currently the goto statement is skipping over a lot of setup code
because it is outside of an if-block and should be inside it. Fix
this by adding missing if statement braces.
Addresses-Coverity: ("Structurally dead code")
Fixes: fd151ca5396d ("drm amdgpu: SI UVD v3_1")
Signed-off-by: Colin Ian King
---
drivers/gpu/drm/amd/amdgpu/uvd_v3_1.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/uvd_v3_1.c
b/drivers/gpu/drm/amd/amdgpu/uvd_v3_1.c
index 599719e89c31..7cf4b11a65c5 100644
--- a/drivers/gpu/drm/amd/amdgpu/uvd_v3_1.c
+++ b/drivers/gpu/drm/amd/amdgpu/uvd_v3_1.c
@@ -642,9 +642,10 @@ static int uvd_v3_1_hw_init(void *handle)
uvd_v3_1_start(adev);
r = amdgpu_ring_test_helper(ring);
- if (r)
+ if (r) {
DRM_ERROR("amdgpu: UVD ring test fail (%d).\n", r);
- goto done;
+ goto done;
+ }
r = amdgpu_ring_alloc(ring, 10);
if (r) {
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/fb-helper: Fix vt restore
On Wed, Jun 24, 2020 at 5:29 AM Daniel Vetter wrote:
>
> In the past we had a pile of hacks to orchestrate access between fbdev
> emulation and native kms clients. We've tried to streamline this, by
> always preferring the kms side above fbdev calls when a drm master
> exists, because drm master controls access to the display resources.
>
> Unfortunately this breaks existing userspace, specifically Xorg. When
> exiting Xorg first restores the console to text mode using the KDSET
> ioctl on the vt. This does nothing, because a drm master is still
> around. Then it drops the drm master status, which again does nothing,
> because logind is keeping additional drm fd open to be able to
> orchestrate vt switches. In the past this is the point where fbdev was
> restored, as part of the ->lastclose hook on the drm side.
>
> Now to fix this regression we don't want to go back to letting fbdev
> restore things whenever it feels like, or to the pile of hacks we've
> had before. Instead try and go with a minimal exception to make the
> KDSET case work again, and nothing else.
>
> This means that if userspace does a KDSET call when switching between
> graphical compositors, there will be some flickering with fbcon
> showing up for a bit. But a) that's not a regression and b) userspace
> can fix it by improving the vt switching dance - logind should have
> all the information it needs.
>
> While pondering all this I'm also wondering wheter we should have a
> SWITCH_MASTER ioctl to allow race-free master status handover. But
> that's for another day.
>
> v2: Somehow forgot to cc all the fbdev people.
>
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=208179
> Cc: shl...@fastmail.com
> Reported-and-Tested-by: shl...@fastmail.com
> Cc: Michel Dänzer
> Fixes: 64914da24ea9 ("drm/fbdev-helper: don't force restores")
> Cc: Noralf Trønnes
> Cc: Thomas Zimmermann
> Cc: Daniel Vetter
> Cc: Maarten Lankhorst
> Cc: Maxime Ripard
> Cc: David Airlie
> Cc: Daniel Vetter
> Cc: dri-devel@lists.freedesktop.org
> Cc: # v5.7+
> Cc: Bartlomiej Zolnierkiewicz
> Cc: Geert Uytterhoeven
> Cc: Nathan Chancellor
> Cc: Qiujun Huang
> Cc: Peter Rosin
> Cc: linux-fb...@vger.kernel.org
> Signed-off-by: Daniel Vetter
> ---
> drivers/gpu/drm/drm_fb_helper.c | 63 +---
> drivers/video/fbdev/core/fbcon.c | 3 +-
> include/uapi/linux/fb.h | 1 +
> 3 files changed, 52 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 170aa7689110..ae69bf8e9bcc 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -227,18 +227,9 @@ int drm_fb_helper_debug_leave(struct fb_info *info)
> }
> EXPORT_SYMBOL(drm_fb_helper_debug_leave);
>
> -/**
> - * drm_fb_helper_restore_fbdev_mode_unlocked - restore fbdev configuration
> - * @fb_helper: driver-allocated fbdev helper, can be NULL
> - *
> - * This should be called from driver's drm &drm_driver.lastclose callback
> - * when implementing an fbcon on top of kms using this helper. This ensures
> that
> - * the user isn't greeted with a black screen when e.g. X dies.
> - *
> - * RETURNS:
> - * Zero if everything went ok, negative error code otherwise.
> - */
> -int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper
> *fb_helper)
> +static int
> +__drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper,
> + bool force)
> {
> bool do_delayed;
> int ret;
> @@ -250,7 +241,16 @@ int drm_fb_helper_restore_fbdev_mode_unlocked(struct
> drm_fb_helper *fb_helper)
> return 0;
>
> mutex_lock(&fb_helper->lock);
> - ret = drm_client_modeset_commit(&fb_helper->client);
> + if (force) {
> + /*
> +* Yes this is the _locked version which expects the master
> lock
> +* to be held. But for forced restores we're intentionally
> +* racing here, see drm_fb_helper_set_par().
> +*/
> + ret = drm_client_modeset_commit_locked(&fb_helper->client);
> + } else {
> + ret = drm_client_modeset_commit(&fb_helper->client);
> + }
>
> do_delayed = fb_helper->delayed_hotplug;
> if (do_delayed)
> @@ -262,6 +262,22 @@ int drm_fb_helper_restore_fbdev_mode_unlocked(struct
> drm_fb_helper *fb_helper)
>
> return ret;
> }
> +
> +/**
> + * drm_fb_helper_restore_fbdev_mode_unlocked - restore fbdev configuration
> + * @fb_helper: driver-allocated fbdev helper, can be NULL
> + *
> + * This should be called from driver's drm &drm_driver.lastclose callback
> + * when implementing an fbcon on top of kms using this helper. This ensures
> that
> + * the user isn't greeted with a black screen when e.g. X dies.
> + *
> + * RETURNS:
> + * Zero if everything went ok, negative error code otherwise.
> + */
> +int drm_fb_helper_restore_fbdev_mode_unloc
Re: [RESEND PATCH v5 3/5] drivers core: allow probe_err accept integer and pointer types
On 2020-06-24 13:55, Andy Shevchenko wrote: On Wed, Jun 24, 2020 at 3:37 PM Robin Murphy wrote: On 2020-06-24 12:41, Andrzej Hajda wrote: Many resource acquisition functions return error value encapsulated in pointer instead of integer value. To simplify coding we can use macro which will accept both types of error. With this patch user can use: probe_err(dev, ptr, ...) instead of: probe_err(dev, PTR_ERR(ptr), ...) Without loosing old functionality: probe_err(dev, err, ...) Personally I'm not convinced that simplification has much value, and I'd say it *does* have a significant downside. This: if (IS_ERR(x)) do_something_with(PTR_ERR(x)); is a familiar and expected pattern when reading/reviewing code, and at a glance is almost certainly doing the right thing. If I see this, on the other hand: if (IS_ERR(x)) do_something_with(x); I don't consider your arguments strong enough. You are appealing to one pattern vs. new coming *pattern* just with a different name and actually much less characters to parse. We have a lot of clean ups like this, have you protested against them? JFYI: they are now *established patterns* and saved a ton of LOCs in some of which even were typos. I'm not against probe_err() itself, I think that stands to be a wonderfully helpful thing that will eliminate a hell of a lot of ugly mess from drivers. It's only the specific elision of 9 characters worth of "PTR_ERR()" in certain cases that I'm questioning. I mean, we've already got +20 characters leeway now that checkpatch has acknowledged all that blank space on the right-hand side of all my editor windows. Sure, there's not necessarily anything bad about introducing a new pattern in itself, but it's not going to *replace* the existing pattern of "IS_ERR() pairs with PTR_ERR()", because IS_ERR() is used for more than driver probe error handling. Instead, everybody now has to bear in mind that the new pattern is "IS_ERR() pairs with PTR_ERR(), except sometimes when it doesn't - last time I looked only probe_err() was special, but maybe something's changed, I'll have to go check...", and that's just adding cognitive load for the sake of not even saving a linewrap any more. First, the wave of Sparse errors from the build bots hits because it turns out casting arbitrary pointers appropriately is hard. As it washes past, the reality of authors' and maintainers' personal preferences comes to bear... some inevitably prefer to keep spelling out PTR_ERR() in probe_err() calls for the sake of clarity, bikeshedding ensues, and the checkpatch and Coccinelle armies mobilise to send unwanted patches changing things back and forth. Eventually, in all the confusion, a synapse misfires in a muddled developer's mind, an ERR_PTR value passed to kfree() "because kfree() is robust" slips through, and a bug is born. And there's the thing: inconsistency breeds bugs. Sometimes, of course, there are really good reasons to be inconsistent. Is 9 characters per line for a few hundred lines across the kernel tree really a really good reason? The tersest code is not always the most maintainable. Consider that we could also save many more "tons of LoC" by rewriting an entire category of small if/else statements with ternaries. Would the overall effect on maintainability be positive? I don't think so. And yeah, anyone who pipes up suggesting that places where an ERR_PTR value could be passed to probe_err() could simply refactor IS_ERR() checks with more uses of the god-awful PTR_ERR_OR_ZERO() obfuscator gets a long stare of disapproval... Robin. my immediate instinct is to be suspicious, and now I've got to go off and double-check that if do_something_with() really expects a pointer it's also robust against PTR_ERR values. Off-hand I can't think of any APIs that work that way in the areas with which I'm familiar, so it would be a pretty unusual and non-obvious thing. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH][next] drm: amdgpu: fix premature goto because of missing braces
From: Colin Ian King
Currently the goto statement is skipping over a lot of setup code
because it is outside of an if-block and should be inside it. Fix
this by adding missing if statement braces.
Addresses-Coverity: ("Structurally dead code")
Fixes: fd151ca5396d ("drm amdgpu: SI UVD v3_1")
Signed-off-by: Colin Ian King
---
drivers/gpu/drm/amd/amdgpu/uvd_v3_1.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/uvd_v3_1.c
b/drivers/gpu/drm/amd/amdgpu/uvd_v3_1.c
index 599719e89c31..7cf4b11a65c5 100644
--- a/drivers/gpu/drm/amd/amdgpu/uvd_v3_1.c
+++ b/drivers/gpu/drm/amd/amdgpu/uvd_v3_1.c
@@ -642,9 +642,10 @@ static int uvd_v3_1_hw_init(void *handle)
uvd_v3_1_start(adev);
r = amdgpu_ring_test_helper(ring);
- if (r)
+ if (r) {
DRM_ERROR("amdgpu: UVD ring test fail (%d).\n", r);
- goto done;
+ goto done;
+ }
r = amdgpu_ring_alloc(ring, 10);
if (r) {
--
2.27.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RESEND PATCH v5 4/5] drm/bridge/sii8620: fix resource acquisition error handling
On Wed, Jun 24, 2020 at 03:43:10PM +0200, Andrzej Hajda wrote: > > On 24.06.2020 15:25, Mark Brown wrote: > > If we silently ignore all deferred probe errors we make it hard for > > anyone who is experiencing issues with deferred probe to figure out what > > they're missing. We should at least be logging problems at debug level > > so there's something for people to go on without having to hack the > > kernel source. > But you can always do: > cat /sys/kernel/debug/devices_deferred > And you will find there deferred probe reason (thanks to patch 2/5). Right, my point is more that we shouldn't be promoting discarding the diagnostics entirely but rather saying that we want to redirect those somewhere else. > Eventually if you want it in dmesg anyway, one can adjust probe_err function > to log probe error on debug level as well. That would most likely be very useful as a boot option for problems that occur before we get a console up. signature.asc Description: PGP signature ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RESEND PATCH v5 1/5] driver core: add probe_err log helper
On 24.06.2020 15:23, Laurent Pinchart wrote:
> On Wed, Jun 24, 2020 at 02:31:40PM +0200, Greg Kroah-Hartman wrote:
>> On Wed, Jun 24, 2020 at 01:41:23PM +0200, Andrzej Hajda wrote:
>>> During probe every time driver gets resource it should usually check for
>>> error
>>> printk some message if it is not -EPROBE_DEFER and return the error. This
>>> pattern is simple but requires adding few lines after any resource
>>> acquisition
>>> code, as a result it is often omited or implemented only partially.
>>> probe_err helps to replace such code sequences with simple call, so code:
>>> if (err != -EPROBE_DEFER)
>>> dev_err(dev, ...);
>>> return err;
>>> becomes:
>>> return probe_err(dev, err, ...);
>>>
>>> Signed-off-by: Andrzej Hajda
>>> Reviewed-by: Javier Martinez Canillas
>>> Reviewed-by: Mark Brown
>>> Reviewed-by: Andy Shevchenko
>>> ---
>>> drivers/base/core.c| 39 +++
>>> include/linux/device.h | 3 +++
>>> 2 files changed, 42 insertions(+)
>>>
>>> diff --git a/drivers/base/core.c b/drivers/base/core.c
>>> index 67d39a90b45c..ee9da66bff1b 100644
>>> --- a/drivers/base/core.c
>>> +++ b/drivers/base/core.c
>>> @@ -3953,6 +3953,45 @@ define_dev_printk_level(_dev_info, KERN_INFO);
>>>
>>> #endif
>>>
>>> +/**
>>> + * probe_err - probe error check and log helper
>>> + * @dev: the pointer to the struct device
>>> + * @err: error value to test
>>> + * @fmt: printf-style format string
>>> + * @...: arguments as specified in the format string
>>> + *
>>> + * This helper implements common pattern present in probe functions for
>>> error
>>> + * checking: print message if the error is not -EPROBE_DEFER and propagate
>>> it.
>>> + * It replaces code sequence:
>>> + * if (err != -EPROBE_DEFER)
>>> + * dev_err(dev, ...);
>>> + * return err;
>>> + * with
>>> + * return probe_err(dev, err, ...);
>>> + *
>>> + * Returns @err.
>>> + *
>>> + */
>>> +int probe_err(const struct device *dev, int err, const char *fmt, ...)
>>> +{
>>> + struct va_format vaf;
>>> + va_list args;
>>> +
>>> + if (err == -EPROBE_DEFER)
>>> + return err;
>>> +
>>> + va_start(args, fmt);
>>> + vaf.fmt = fmt;
>>> + vaf.va = &args;
>>> +
>>> + dev_err(dev, "error %d: %pV", err, &vaf);
>>> +
>>> + va_end(args);
>>> +
>>> + return err;
>>> +}
>>> +EXPORT_SYMBOL_GPL(probe_err);
>> Please be specific in global symbols, how about "driver_probe_error()"?
> Or dev_err_probe() to match the existing dev_* functions ?
OK.
Regards
Andrzej
>
>> And merge the other patch into this one, as Raphael said, otherwise this
>> just looks odd to add something and then fix it up later.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RESEND PATCH v5 5/5] drm/bridge: lvds-codec: simplify error handling code
On 24.06.2020 15:33, Laurent Pinchart wrote:
> Hi Andrzej,
>
> On Wed, Jun 24, 2020 at 01:41:27PM +0200, Andrzej Hajda wrote:
>> Using probe_err code has following advantages:
>> - shorter code,
>> - recorded defer probe reason for debugging,
>> - uniform error code logging.
>>
>> Signed-off-by: Andrzej Hajda
>> ---
>> drivers/gpu/drm/bridge/lvds-codec.c | 9 ++---
>> 1 file changed, 2 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/lvds-codec.c
>> b/drivers/gpu/drm/bridge/lvds-codec.c
>> index 24fb1befdfa2..c76fa0239e39 100644
>> --- a/drivers/gpu/drm/bridge/lvds-codec.c
>> +++ b/drivers/gpu/drm/bridge/lvds-codec.c
>> @@ -71,13 +71,8 @@ static int lvds_codec_probe(struct platform_device *pdev)
>> lvds_codec->connector_type = (uintptr_t)of_device_get_match_data(dev);
>> lvds_codec->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown",
>> GPIOD_OUT_HIGH);
>> -if (IS_ERR(lvds_codec->powerdown_gpio)) {
>> -int err = PTR_ERR(lvds_codec->powerdown_gpio);
>> -
>> -if (err != -EPROBE_DEFER)
>> -dev_err(dev, "powerdown GPIO failure: %d\n", err);
>> -return err;
>> -}
>> +if (IS_ERR(lvds_codec->powerdown_gpio))
>> +return probe_err(dev, lvds_codec->powerdown_gpio, "powerdown
>> GPIO failure\n");
> Line wrap please.
I hoped that with latest checkpatch line length limit increase from 80
to 100 it is acceptable :) but apparently not [1].
[1]:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=bdc48fa11e46f867ea4d75fa59ee87a7f48be144
>
> It bothers me that the common pattern of writing the error code at the
> end of the message isn't possible anymore. Maybe I'll get used to it,
> but it removes some flexibility.
Yes, but it gives uniformity :) and now with %pe printk format it
changes anyway.
Regards
Andrzej
>
>>
>> /* Locate the panel DT node. */
>> panel_node = of_graph_get_remote_node(dev->of_node, 1, 0);
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RESEND PATCH v5 1/5] driver core: add probe_err log helper
On Wed, Jun 24, 2020 at 04:45:28PM +0300, Andy Shevchenko wrote: > On Wed, Jun 24, 2020 at 4:27 PM Mark Brown wrote: > > As I said down the thread that's not a great pattern since it means that > > probe deferral errors never get displayed and users have a hard time > > figuring out why their driver isn't instantiating. > Don't we have a file in the debugfs to list deferred drivers? Part of what this patch series aims to solve is that that list is not very useful since it doesn't provide any information on how things got deferred which means it's of no use in trying to figure out any problems. > In the case of deferred probes the errors out of it makes users more > miserable in order to look through tons of spam and lose really useful > data in the logs. I seem to never manage to end up using any of the systems which generate excessive deferrals. signature.asc Description: PGP signature ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RESEND PATCH v5 1/5] driver core: add probe_err log helper
On Wed, Jun 24, 2020 at 4:27 PM Mark Brown wrote: > > On Wed, Jun 24, 2020 at 01:41:23PM +0200, Andrzej Hajda wrote: > > During probe every time driver gets resource it should usually check for > > error > > printk some message if it is not -EPROBE_DEFER and return the error. This > > As I said down the thread that's not a great pattern since it means that > probe deferral errors never get displayed and users have a hard time > figuring out why their driver isn't instantiating. Don't we have a file in the debugfs to list deferred drivers? In the case of deferred probes the errors out of it makes users more miserable in order to look through tons of spam and lose really useful data in the logs. -- With Best Regards, Andy Shevchenko ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RESEND PATCH v5 4/5] drm/bridge/sii8620: fix resource acquisition error handling
On 24.06.2020 15:25, Mark Brown wrote: On Wed, Jun 24, 2020 at 01:41:26PM +0200, Andrzej Hajda wrote: In case of error during resource acquisition driver should print error message only in case it is not deferred probe, using probe_err helper solves the issue. Moreover it records defer probe reason for debugging. If we silently ignore all deferred probe errors we make it hard for anyone who is experiencing issues with deferred probe to figure out what they're missing. We should at least be logging problems at debug level so there's something for people to go on without having to hack the kernel source. But you can always do: cat /sys/kernel/debug/devices_deferred And you will find there deferred probe reason (thanks to patch 2/5). Eventually if you want it in dmesg anyway, one can adjust probe_err function to log probe error on debug level as well. Regards Andrzej ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://protect2.fireeye.com/url?k=ef34cf7b-b2ff4845-ef354434-0cc47a31309a-305676031d9eb553&q=1&u=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 2/2] drm/ttm: make TT creation purely optional
We only need the page array when the BO is about to be accessed.
So not only populate, but also create it on demand.
Signed-off-by: Christian König
---
drivers/gpu/drm/ttm/ttm_bo.c | 26 --
drivers/gpu/drm/ttm/ttm_bo_util.c | 9 +++--
drivers/gpu/drm/ttm/ttm_bo_vm.c | 5 +
3 files changed, 16 insertions(+), 24 deletions(-)
diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index 15f9b19fa00d..0e0a9dadf3ed 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -667,13 +667,8 @@ static int ttm_bo_evict(struct ttm_buffer_object *bo,
placement.num_busy_placement = 0;
bdev->driver->evict_flags(bo, &placement);
- if (!placement.num_placement && !placement.num_busy_placement) {
- ret = ttm_bo_pipeline_gutting(bo);
- if (ret)
- return ret;
-
- return ttm_tt_create(bo, false);
- }
+ if (!placement.num_placement && !placement.num_busy_placement)
+ return ttm_bo_pipeline_gutting(bo);
evict_mem = bo->mem;
evict_mem.mm_node = NULL;
@@ -1200,13 +1195,8 @@ int ttm_bo_validate(struct ttm_buffer_object *bo,
/*
* Remove the backing store if no placement is given.
*/
- if (!placement->num_placement && !placement->num_busy_placement) {
- ret = ttm_bo_pipeline_gutting(bo);
- if (ret)
- return ret;
-
- return ttm_tt_create(bo, false);
- }
+ if (!placement->num_placement && !placement->num_busy_placement)
+ return ttm_bo_pipeline_gutting(bo);
/*
* Check whether we need to move buffer.
@@ -1223,14 +1213,6 @@ int ttm_bo_validate(struct ttm_buffer_object *bo,
ttm_flag_masked(&bo->mem.placement, new_flags,
~TTM_PL_MASK_MEMTYPE);
}
- /*
-* We might need to add a TTM.
-*/
- if (bo->mem.mem_type == TTM_PL_SYSTEM && bo->ttm == NULL) {
- ret = ttm_tt_create(bo, true);
- if (ret)
- return ret;
- }
return 0;
}
EXPORT_SYMBOL(ttm_bo_validate);
diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c
b/drivers/gpu/drm/ttm/ttm_bo_util.c
index 52d2b71f1588..f8414f820350 100644
--- a/drivers/gpu/drm/ttm/ttm_bo_util.c
+++ b/drivers/gpu/drm/ttm/ttm_bo_util.c
@@ -580,12 +580,17 @@ static int ttm_bo_kmap_ttm(struct ttm_buffer_object *bo,
.interruptible = false,
.no_wait_gpu = false
};
- struct ttm_tt *ttm = bo->ttm;
+ struct ttm_tt *ttm;
pgprot_t prot;
int ret;
- BUG_ON(!ttm);
+ if (!bo->ttm) {
+ ret = ttm_tt_create(bo, true);
+ if (ret)
+ return ret;
+ }
+ ttm = bo->ttm;
ret = ttm_tt_populate(ttm, &ctx);
if (ret)
return ret;
diff --git a/drivers/gpu/drm/ttm/ttm_bo_vm.c b/drivers/gpu/drm/ttm/ttm_bo_vm.c
index 0ad30b112982..bdfed6725d6f 100644
--- a/drivers/gpu/drm/ttm/ttm_bo_vm.c
+++ b/drivers/gpu/drm/ttm/ttm_bo_vm.c
@@ -349,6 +349,11 @@ vm_fault_t ttm_bo_vm_fault_reserved(struct vm_fault *vmf,
};
+ if (!bo->ttm && ttm_tt_create(bo, true)) {
+ ret = VM_FAULT_OOM;
+ goto out_io_unlock;
+ }
+
ttm = bo->ttm;
if (ttm_tt_populate(bo->ttm, &ctx)) {
ret = VM_FAULT_OOM;
--
2.17.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 1/2] drm/ttm: cleanup ttm_mem_type_manager_func.get_node interface
Instead of signaling failure by setting the node pointer to
NULL do so by returning -ENOSPC.
Signed-off-by: Christian König
---
drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c | 4 +---
drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c | 5 ++---
drivers/gpu/drm/nouveau/nouveau_ttm.c | 8
drivers/gpu/drm/ttm/ttm_bo.c | 11 +--
drivers/gpu/drm/ttm/ttm_bo_manager.c | 2 +-
drivers/gpu/drm/vmwgfx/vmwgfx_gmrid_manager.c | 4 +---
6 files changed, 10 insertions(+), 24 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c
index 627104401e84..2baa80224fa4 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c
@@ -229,7 +229,7 @@ static int amdgpu_gtt_mgr_new(struct ttm_mem_type_manager
*man,
if ((&tbo->mem == mem || tbo->mem.mem_type != TTM_PL_TT) &&
atomic64_read(&mgr->available) < mem->num_pages) {
spin_unlock(&mgr->lock);
- return 0;
+ return -ENOSPC;
}
atomic64_sub(mem->num_pages, &mgr->available);
spin_unlock(&mgr->lock);
@@ -249,8 +249,6 @@ static int amdgpu_gtt_mgr_new(struct ttm_mem_type_manager
*man,
r = amdgpu_gtt_mgr_alloc(man, tbo, place, mem);
if (unlikely(r)) {
kfree(node);
- mem->mm_node = NULL;
- r = 0;
goto err_out;
}
} else {
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
index 128a667ed8fa..e8d1dd564006 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
@@ -336,8 +336,7 @@ static int amdgpu_vram_mgr_new(struct ttm_mem_type_manager
*man,
mem_bytes = (u64)mem->num_pages << PAGE_SHIFT;
if (atomic64_add_return(mem_bytes, &mgr->usage) > max_bytes) {
atomic64_sub(mem_bytes, &mgr->usage);
- mem->mm_node = NULL;
- return 0;
+ return -ENOSPC;
}
if (place->flags & TTM_PL_FLAG_CONTIGUOUS) {
@@ -417,7 +416,7 @@ static int amdgpu_vram_mgr_new(struct ttm_mem_type_manager
*man,
atomic64_sub(mem->num_pages << PAGE_SHIFT, &mgr->usage);
kvfree(nodes);
- return r == -ENOSPC ? 0 : r;
+ return r;
}
/**
diff --git a/drivers/gpu/drm/nouveau/nouveau_ttm.c
b/drivers/gpu/drm/nouveau/nouveau_ttm.c
index 7ca0a2498532..e89ea052cf71 100644
--- a/drivers/gpu/drm/nouveau/nouveau_ttm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_ttm.c
@@ -75,10 +75,6 @@ nouveau_vram_manager_new(struct ttm_mem_type_manager *man,
ret = nouveau_mem_vram(reg, nvbo->contig, nvbo->page);
if (ret) {
nouveau_mem_del(reg);
- if (ret == -ENOSPC) {
- reg->mm_node = NULL;
- return 0;
- }
return ret;
}
@@ -139,10 +135,6 @@ nv04_gart_manager_new(struct ttm_mem_type_manager *man,
reg->num_pages << PAGE_SHIFT, &mem->vma[0]);
if (ret) {
nouveau_mem_del(reg);
- if (ret == -ENOSPC) {
- reg->mm_node = NULL;
- return 0;
- }
return ret;
}
diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index f73b81c2576e..15f9b19fa00d 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -916,10 +916,10 @@ static int ttm_bo_mem_force_space(struct
ttm_buffer_object *bo,
ticket = dma_resv_locking_ctx(bo->base.resv);
do {
ret = (*man->func->get_node)(man, bo, place, mem);
- if (unlikely(ret != 0))
- return ret;
- if (mem->mm_node)
+ if (likely(!ret))
break;
+ if (unlikely(ret != -ENOSPC))
+ return ret;
ret = ttm_mem_evict_first(bdev, mem->mem_type, place, ctx,
ticket);
if (unlikely(ret != 0))
@@ -1063,12 +1063,11 @@ int ttm_bo_mem_space(struct ttm_buffer_object *bo,
man = &bdev->man[mem->mem_type];
ret = (*man->func->get_node)(man, bo, place, mem);
+ if (ret == -ENOSPC)
+ continue;
if (unlikely(ret))
goto error;
- if (!mem->mm_node)
- continue;
-
ret = ttm_bo_add_move_fence(bo, man, mem, ctx->no_wait_gpu);
if (unlikely(ret)) {
(*man->func->put_node)(man, mem);
diff --git a/drivers/gpu/drm/ttm/ttm_bo_manager.c
b/drivers/gpu/drm/ttm/ttm_bo_manager.c
index 18d3debcc949..facd3049c3aa 100644
--- a/driver
Re: [RESEND PATCH v5 5/5] drm/bridge: lvds-codec: simplify error handling code
Hi Andrzej,
On Wed, Jun 24, 2020 at 01:41:27PM +0200, Andrzej Hajda wrote:
> Using probe_err code has following advantages:
> - shorter code,
> - recorded defer probe reason for debugging,
> - uniform error code logging.
>
> Signed-off-by: Andrzej Hajda
> ---
> drivers/gpu/drm/bridge/lvds-codec.c | 9 ++---
> 1 file changed, 2 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/lvds-codec.c
> b/drivers/gpu/drm/bridge/lvds-codec.c
> index 24fb1befdfa2..c76fa0239e39 100644
> --- a/drivers/gpu/drm/bridge/lvds-codec.c
> +++ b/drivers/gpu/drm/bridge/lvds-codec.c
> @@ -71,13 +71,8 @@ static int lvds_codec_probe(struct platform_device *pdev)
> lvds_codec->connector_type = (uintptr_t)of_device_get_match_data(dev);
> lvds_codec->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown",
>GPIOD_OUT_HIGH);
> - if (IS_ERR(lvds_codec->powerdown_gpio)) {
> - int err = PTR_ERR(lvds_codec->powerdown_gpio);
> -
> - if (err != -EPROBE_DEFER)
> - dev_err(dev, "powerdown GPIO failure: %d\n", err);
> - return err;
> - }
> + if (IS_ERR(lvds_codec->powerdown_gpio))
> + return probe_err(dev, lvds_codec->powerdown_gpio, "powerdown
> GPIO failure\n");
Line wrap please.
It bothers me that the common pattern of writing the error code at the
end of the message isn't possible anymore. Maybe I'll get used to it,
but it removes some flexibility.
>
> /* Locate the panel DT node. */
> panel_node = of_graph_get_remote_node(dev->of_node, 1, 0);
--
Regards,
Laurent Pinchart
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RESEND PATCH v5 3/5] drivers core: allow probe_err accept integer and pointer types
On Wed, Jun 24, 2020 at 01:37:52PM +0100, Robin Murphy wrote:
> On 2020-06-24 12:41, Andrzej Hajda wrote:
> > Many resource acquisition functions return error value encapsulated in
> > pointer instead of integer value. To simplify coding we can use macro
> > which will accept both types of error.
> > With this patch user can use:
> > probe_err(dev, ptr, ...)
> > instead of:
> > probe_err(dev, PTR_ERR(ptr), ...)
> > Without loosing old functionality:
> > probe_err(dev, err, ...)
>
> Personally I'm not convinced that simplification has much value, and I'd
> say it *does* have a significant downside. This:
>
> if (IS_ERR(x))
> do_something_with(PTR_ERR(x));
>
> is a familiar and expected pattern when reading/reviewing code, and at a
> glance is almost certainly doing the right thing. If I see this, on the
> other hand:
>
> if (IS_ERR(x))
> do_something_with(x);
>
> my immediate instinct is to be suspicious, and now I've got to go off
> and double-check that if do_something_with() really expects a pointer
> it's also robust against PTR_ERR values. Off-hand I can't think of any
> APIs that work that way in the areas with which I'm familiar, so it
> would be a pretty unusual and non-obvious thing.
I second this. Furthermore, the hidden cast to long means that we'll
leak pointer values if one happens to pass a real pointer to this
function.
> Furthermore, an error helper that explicitly claims to accept "pointer
> type" values seems like it could easily lead to misunderstandings like this:
>
> int init_my_buffer(struct my_device *d)
> {
> d->buffer = kzalloc(d->buffer_size, GFP_KERNEL);
> return probe_err(d->dev, d->buffer, "failed to init buffer\n");
> }
>
> and allowing that to compile without any hint of an error seems a
> little... unfair.
>
> Robin.
>
> > Signed-off-by: Andrzej Hajda
> > ---
> > drivers/base/core.c| 25 ++---
> > include/linux/device.h | 25 -
> > 2 files changed, 26 insertions(+), 24 deletions(-)
> >
> > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > index 2a96954d5460..df283c62d9c0 100644
> > --- a/drivers/base/core.c
> > +++ b/drivers/base/core.c
> > @@ -3953,28 +3953,7 @@ define_dev_printk_level(_dev_info, KERN_INFO);
> >
> > #endif
> >
> > -/**
> > - * probe_err - probe error check and log helper
> > - * @dev: the pointer to the struct device
> > - * @err: error value to test
> > - * @fmt: printf-style format string
> > - * @...: arguments as specified in the format string
> > - *
> > - * This helper implements common pattern present in probe functions for
> > error
> > - * checking: print message if the error is not -EPROBE_DEFER and propagate
> > it.
> > - * In case of -EPROBE_DEFER it sets defer probe reason, which can be
> > checked
> > - * later by reading devices_deferred debugfs attribute.
> > - * It replaces code sequence:
> > - * if (err != -EPROBE_DEFER)
> > - * dev_err(dev, ...);
> > - * return err;
> > - * with
> > - * return probe_err(dev, err, ...);
> > - *
> > - * Returns @err.
> > - *
> > - */
> > -int probe_err(const struct device *dev, int err, const char *fmt, ...)
> > +int __probe_err(const struct device *dev, int err, const char *fmt, ...)
> > {
> > struct va_format vaf;
> > va_list args;
> > @@ -3992,7 +3971,7 @@ int probe_err(const struct device *dev, int err,
> > const char *fmt, ...)
> >
> > return err;
> > }
> > -EXPORT_SYMBOL_GPL(probe_err);
> > +EXPORT_SYMBOL_GPL(__probe_err);
> >
> > static inline bool fwnode_is_primary(struct fwnode_handle *fwnode)
> > {
> > diff --git a/include/linux/device.h b/include/linux/device.h
> > index 40a90d9bf799..22d3c3d4f461 100644
> > --- a/include/linux/device.h
> > +++ b/include/linux/device.h
> > @@ -965,7 +965,30 @@ void device_links_supplier_sync_state_pause(void);
> > void device_links_supplier_sync_state_resume(void);
> >
> > extern __printf(3, 4)
> > -int probe_err(const struct device *dev, int err, const char *fmt, ...);
> > +int __probe_err(const struct device *dev, int err, const char *fmt, ...);
> > +
> > +/**
> > + * probe_err - probe error check and log helper
> > + * @dev: the pointer to the struct device
> > + * @err: error value to test, can be integer or pointer type
> > + * @fmt: printf-style format string
> > + * @...: arguments as specified in the format string
> > + *
> > + * This helper implements common pattern present in probe functions for
> > error
> > + * checking: print message if the error is not -EPROBE_DEFER and propagate
> > it.
> > + * In case of -EPROBE_DEFER it sets defer probe reason, which can be
> > checked
> > + * later by reading devices_deferred debugfs attribute.
> > + * It replaces code sequence:
> > + * if (err != -EPROBE_DEFER)
> > + * dev_err(dev, ...);
> > + * return err;
> > + * w
Re: [RESEND PATCH v5 2/5] driver core: add deferring probe reason to devices_deferred property
On 24.06.2020 14:11, Rafael J. Wysocki wrote:
> On Wed, Jun 24, 2020 at 1:41 PM Andrzej Hajda wrote:
>> /sys/kernel/debug/devices_deferred property contains list of deferred
>> devices.
>> This list does not contain reason why the driver deferred probe, the patch
>> improves it.
>> The natural place to set the reason is probe_err function introduced
>> recently,
>> ie. if probe_err will be called with -EPROBE_DEFER instead of printk the
>> message
>> will be attached to deferred device and printed when user read
>> devices_deferred
>> property.
>>
>> Signed-off-by: Andrzej Hajda
>> Reviewed-by: Mark Brown
>> Reviewed-by: Javier Martinez Canillas
>> Reviewed-by: Andy Shevchenko
>> ---
>> drivers/base/base.h | 3 +++
>> drivers/base/core.c | 10 ++
>> drivers/base/dd.c | 21 -
>> 3 files changed, 29 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/base/base.h b/drivers/base/base.h
>> index 95c22c0f9036..93ef1c2f4c1f 100644
>> --- a/drivers/base/base.h
>> +++ b/drivers/base/base.h
>> @@ -93,6 +93,7 @@ struct device_private {
>> struct klist_node knode_class;
>> struct list_head deferred_probe;
>> struct device_driver *async_driver;
>> + char *deferred_probe_msg;
> What about calling this deferred_probe_reason?
>
>> struct device *device;
>> u8 dead:1;
>> };
>> @@ -134,6 +135,8 @@ extern void device_release_driver_internal(struct device
>> *dev,
>> extern void driver_detach(struct device_driver *drv);
>> extern int driver_probe_device(struct device_driver *drv, struct device
>> *dev);
>> extern void driver_deferred_probe_del(struct device *dev);
>> +extern void __deferred_probe_set_msg(const struct device *dev,
>> +struct va_format *vaf);
> I'd call this device_set_deferred_probe_reson() to follow the naming
> convention for the function names in this header file.
>
>> static inline int driver_match_device(struct device_driver *drv,
>>struct device *dev)
>> {
>> diff --git a/drivers/base/core.c b/drivers/base/core.c
>> index ee9da66bff1b..2a96954d5460 100644
>> --- a/drivers/base/core.c
>> +++ b/drivers/base/core.c
>> @@ -3962,6 +3962,8 @@ define_dev_printk_level(_dev_info, KERN_INFO);
>>*
>>* This helper implements common pattern present in probe functions for
>> error
>>* checking: print message if the error is not -EPROBE_DEFER and propagate
>> it.
>> + * In case of -EPROBE_DEFER it sets defer probe reason, which can be checked
>> + * later by reading devices_deferred debugfs attribute.
>>* It replaces code sequence:
>>* if (err != -EPROBE_DEFER)
>>* dev_err(dev, ...);
>> @@ -3977,14 +3979,14 @@ int probe_err(const struct device *dev, int err,
>> const char *fmt, ...)
>> struct va_format vaf;
>> va_list args;
>>
>> - if (err == -EPROBE_DEFER)
>> - return err;
>> -
>> va_start(args, fmt);
>> vaf.fmt = fmt;
>> vaf.va = &args;
>>
>> - dev_err(dev, "error %d: %pV", err, &vaf);
>> + if (err == -EPROBE_DEFER)
>> + __deferred_probe_set_msg(dev, &vaf);
>> + else
>> + dev_err(dev, "error %d: %pV", err, &vaf);
>>
>> va_end(args);
>>
>> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
>> index 9a1d940342ac..f44d26454b6a 100644
>> --- a/drivers/base/dd.c
>> +++ b/drivers/base/dd.c
>> @@ -27,6 +27,7 @@
>> #include
>> #include
>> #include
>> +#include
>>
>> #include "base.h"
>> #include "power/power.h"
>> @@ -136,6 +137,8 @@ void driver_deferred_probe_del(struct device *dev)
>> if (!list_empty(&dev->p->deferred_probe)) {
>> dev_dbg(dev, "Removed from deferred list\n");
>> list_del_init(&dev->p->deferred_probe);
>> + kfree(dev->p->deferred_probe_msg);
>> + dev->p->deferred_probe_msg = NULL;
>> }
>> mutex_unlock(&deferred_probe_mutex);
>> }
>> @@ -211,6 +214,21 @@ void device_unblock_probing(void)
>> driver_deferred_probe_trigger();
>> }
>>
>> +/*
>> + * __deferred_probe_set_msg() - Set defer probe reason message for device
> I'd change this into a full kerneldoc comment.
OK I will apply all changes in next version. Thx for review.
Regards
Andrzej
>
>> + */
>> +void __deferred_probe_set_msg(const struct device *dev, struct va_format
>> *vaf)
>> +{
>> + const char *drv = dev_driver_string(dev);
>> +
>> + mutex_lock(&deferred_probe_mutex);
>> +
>> + kfree(dev->p->deferred_probe_msg);
>> + dev->p->deferred_probe_msg = kasprintf(GFP_KERNEL, "%s: %pV", drv,
>> vaf);
>> +
>> + mutex_unlock(&deferred_probe_mutex);
>> +}
>> +
>> /*
>>* deferred_devs_show() - Show the devices in the deferred probe pending
>> list.
>>*/
>> @@ -221,7 +239,8 @@ static int deferred_devs_show(struct seq_file *s, void
>> *data)
>>
Re: [RESEND PATCH v5 1/5] driver core: add probe_err log helper
On Wed, Jun 24, 2020 at 01:41:23PM +0200, Andrzej Hajda wrote: > During probe every time driver gets resource it should usually check for error > printk some message if it is not -EPROBE_DEFER and return the error. This As I said down the thread that's not a great pattern since it means that probe deferral errors never get displayed and users have a hard time figuring out why their driver isn't instantiating. signature.asc Description: PGP signature ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v6] drm/fourcc: document modifier uniqueness requirements
Looks good now, thanks!
On Wed, Jun 24, 2020 at 01:01:31PM +, 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 (except drivers).
>
> v2: add paragraph about aliases (Daniel)
>
> v3: fix unrelated changes sent with the patch
>
> v4: disambiguate users between driver and higher-level programs (Brian,
> Daniel)
>
> v5: fix AFBC example (Brian, Daniel)
>
> v6: remove duplicated paragraph (Daniel)
>
> Signed-off-by: Simon Ser
> Reviewed-by: Daniel Vetter
> Cc: Daniel Stone
> Cc: Bas Nieuwenhuizen
> Cc: Dave Airlie
> Cc: Marek Olšák
> Cc: Alex Deucher
> Cc: Neil Armstrong
> Cc: Michel Dänzer
> Cc: Brian Starkey
> ---
> include/uapi/drm/drm_fourcc.h | 24
> 1 file changed, 24 insertions(+)
>
> diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> index 490143500a50..8eaa158fef81 100644
> --- a/include/uapi/drm/drm_fourcc.h
> +++ b/include/uapi/drm/drm_fourcc.h
> @@ -58,6 +58,30 @@ 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.
> + *
> + * For modifiers where the combination of fourcc code and modifier can alias,
> + * a canonical pair needs to be defined and used by all drivers. Preferred
> + * combinations are also encouraged where all combinations might lead to
> + * confusion and unnecessarily reduced interoperability. An example for the
> + * latter is AFBC, where the ABGR layouts are preferred over ARGB layouts.
> + *
> + * There are two kinds of modifier users:
> + *
> + * - Kernel and user-space drivers: for drivers it's important that modifiers
> + * don't alias, otherwise two drivers might support the same format but use
> + * different aliases, preventing them from sharing buffers in an efficient
> + * format.
> + * - Higher-level programs interfacing with KMS/GBM/EGL/Vulkan/etc: these
> users
> + * see modifiers as opaque tokens they can check for equality and
> intersect.
> + * These users musn't need to know to reason about the modifier value
> + * (i.e. they are not expected to extract information out of the modifier).
> + *
> * Vendors should document their modifier usage in as much detail as
> * possible, to ensure maximum compatibility across devices, drivers and
> * applications.
> --
> 2.27.0
>
>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RESEND PATCH v5 2/5] driver core: add deferring probe reason to devices_deferred property
On 24.06.2020 14:34, Greg Kroah-Hartman wrote:
> On Wed, Jun 24, 2020 at 01:41:24PM +0200, Andrzej Hajda wrote:
>> /sys/kernel/debug/devices_deferred property contains list of deferred
>> devices.
>> This list does not contain reason why the driver deferred probe, the patch
>> improves it.
>> The natural place to set the reason is probe_err function introduced
>> recently,
>> ie. if probe_err will be called with -EPROBE_DEFER instead of printk the
>> message
>> will be attached to deferred device and printed when user read
>> devices_deferred
>> property.
>>
>> Signed-off-by: Andrzej Hajda
>> Reviewed-by: Mark Brown
>> Reviewed-by: Javier Martinez Canillas
>> Reviewed-by: Andy Shevchenko
>> ---
>> drivers/base/base.h | 3 +++
>> drivers/base/core.c | 10 ++
>> drivers/base/dd.c | 21 -
>> 3 files changed, 29 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/base/base.h b/drivers/base/base.h
>> index 95c22c0f9036..93ef1c2f4c1f 100644
>> --- a/drivers/base/base.h
>> +++ b/drivers/base/base.h
>> @@ -93,6 +93,7 @@ struct device_private {
>> struct klist_node knode_class;
>> struct list_head deferred_probe;
>> struct device_driver *async_driver;
>> +char *deferred_probe_msg;
>> struct device *device;
>> u8 dead:1;
>> };
>> @@ -134,6 +135,8 @@ extern void device_release_driver_internal(struct device
>> *dev,
>> extern void driver_detach(struct device_driver *drv);
>> extern int driver_probe_device(struct device_driver *drv, struct device
>> *dev);
>> extern void driver_deferred_probe_del(struct device *dev);
>> +extern void __deferred_probe_set_msg(const struct device *dev,
>> + struct va_format *vaf);
>> static inline int driver_match_device(struct device_driver *drv,
>>struct device *dev)
>> {
>> diff --git a/drivers/base/core.c b/drivers/base/core.c
>> index ee9da66bff1b..2a96954d5460 100644
>> --- a/drivers/base/core.c
>> +++ b/drivers/base/core.c
>> @@ -3962,6 +3962,8 @@ define_dev_printk_level(_dev_info, KERN_INFO);
>>*
>>* This helper implements common pattern present in probe functions for
>> error
>>* checking: print message if the error is not -EPROBE_DEFER and propagate
>> it.
>> + * In case of -EPROBE_DEFER it sets defer probe reason, which can be checked
>> + * later by reading devices_deferred debugfs attribute.
>>* It replaces code sequence:
>>* if (err != -EPROBE_DEFER)
>>* dev_err(dev, ...);
>> @@ -3977,14 +3979,14 @@ int probe_err(const struct device *dev, int err,
>> const char *fmt, ...)
>> struct va_format vaf;
>> va_list args;
>>
>> -if (err == -EPROBE_DEFER)
>> -return err;
>> -
>> va_start(args, fmt);
>> vaf.fmt = fmt;
>> vaf.va = &args;
>>
>> -dev_err(dev, "error %d: %pV", err, &vaf);
>> +if (err == -EPROBE_DEFER)
>> +__deferred_probe_set_msg(dev, &vaf);
>> +else
>> +dev_err(dev, "error %d: %pV", err, &vaf);
>>
>> va_end(args);
>>
>> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
>> index 9a1d940342ac..f44d26454b6a 100644
>> --- a/drivers/base/dd.c
>> +++ b/drivers/base/dd.c
>> @@ -27,6 +27,7 @@
>> #include
>> #include
>> #include
>> +#include
>>
>> #include "base.h"
>> #include "power/power.h"
>> @@ -136,6 +137,8 @@ void driver_deferred_probe_del(struct device *dev)
>> if (!list_empty(&dev->p->deferred_probe)) {
>> dev_dbg(dev, "Removed from deferred list\n");
>> list_del_init(&dev->p->deferred_probe);
>> +kfree(dev->p->deferred_probe_msg);
>> +dev->p->deferred_probe_msg = NULL;
>> }
>> mutex_unlock(&deferred_probe_mutex);
>> }
>> @@ -211,6 +214,21 @@ void device_unblock_probing(void)
>> driver_deferred_probe_trigger();
>> }
>>
>> +/*
>> + * __deferred_probe_set_msg() - Set defer probe reason message for device
>> + */
>> +void __deferred_probe_set_msg(const struct device *dev, struct va_format
>> *vaf)
>> +{
>> +const char *drv = dev_driver_string(dev);
>> +
>> +mutex_lock(&deferred_probe_mutex);
>> +
>> +kfree(dev->p->deferred_probe_msg);
>> +dev->p->deferred_probe_msg = kasprintf(GFP_KERNEL, "%s: %pV", drv, vaf);
> What about the device name? Don't you also want that?
deferred_devs_show prints it already, deferred_probe_msg is appended if
not null.
Regards
Andrzej
>
> You want the same format that __dev_printk() outputs, please use that
> to be consistant with all other messages that drivers are spitting out.
>
> thanks,
>
> greg k-h
> ___
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://protect2.fireeye.com/url?k=28daee95-7508f5cd-28db65da-0cc47a31c8b4-b3e8d1affcda9c08&q=1&u=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel
>
___
dri
Re: [RESEND PATCH v5 4/5] drm/bridge/sii8620: fix resource acquisition error handling
On Wed, Jun 24, 2020 at 01:41:26PM +0200, Andrzej Hajda wrote: > In case of error during resource acquisition driver should print error > message only in case it is not deferred probe, using probe_err helper > solves the issue. Moreover it records defer probe reason for debugging. If we silently ignore all deferred probe errors we make it hard for anyone who is experiencing issues with deferred probe to figure out what they're missing. We should at least be logging problems at debug level so there's something for people to go on without having to hack the kernel source. signature.asc Description: PGP signature ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RESEND PATCH v5 1/5] driver core: add probe_err log helper
On Wed, Jun 24, 2020 at 02:31:40PM +0200, Greg Kroah-Hartman wrote:
> On Wed, Jun 24, 2020 at 01:41:23PM +0200, Andrzej Hajda wrote:
> > During probe every time driver gets resource it should usually check for
> > error
> > printk some message if it is not -EPROBE_DEFER and return the error. This
> > pattern is simple but requires adding few lines after any resource
> > acquisition
> > code, as a result it is often omited or implemented only partially.
> > probe_err helps to replace such code sequences with simple call, so code:
> > if (err != -EPROBE_DEFER)
> > dev_err(dev, ...);
> > return err;
> > becomes:
> > return probe_err(dev, err, ...);
> >
> > Signed-off-by: Andrzej Hajda
> > Reviewed-by: Javier Martinez Canillas
> > Reviewed-by: Mark Brown
> > Reviewed-by: Andy Shevchenko
> > ---
> > drivers/base/core.c| 39 +++
> > include/linux/device.h | 3 +++
> > 2 files changed, 42 insertions(+)
> >
> > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > index 67d39a90b45c..ee9da66bff1b 100644
> > --- a/drivers/base/core.c
> > +++ b/drivers/base/core.c
> > @@ -3953,6 +3953,45 @@ define_dev_printk_level(_dev_info, KERN_INFO);
> >
> > #endif
> >
> > +/**
> > + * probe_err - probe error check and log helper
> > + * @dev: the pointer to the struct device
> > + * @err: error value to test
> > + * @fmt: printf-style format string
> > + * @...: arguments as specified in the format string
> > + *
> > + * This helper implements common pattern present in probe functions for
> > error
> > + * checking: print message if the error is not -EPROBE_DEFER and propagate
> > it.
> > + * It replaces code sequence:
> > + * if (err != -EPROBE_DEFER)
> > + * dev_err(dev, ...);
> > + * return err;
> > + * with
> > + * return probe_err(dev, err, ...);
> > + *
> > + * Returns @err.
> > + *
> > + */
> > +int probe_err(const struct device *dev, int err, const char *fmt, ...)
> > +{
> > + struct va_format vaf;
> > + va_list args;
> > +
> > + if (err == -EPROBE_DEFER)
> > + return err;
> > +
> > + va_start(args, fmt);
> > + vaf.fmt = fmt;
> > + vaf.va = &args;
> > +
> > + dev_err(dev, "error %d: %pV", err, &vaf);
> > +
> > + va_end(args);
> > +
> > + return err;
> > +}
> > +EXPORT_SYMBOL_GPL(probe_err);
>
> Please be specific in global symbols, how about "driver_probe_error()"?
Or dev_err_probe() to match the existing dev_* functions ?
> And merge the other patch into this one, as Raphael said, otherwise this
> just looks odd to add something and then fix it up later.
--
Regards,
Laurent Pinchart
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RESEND PATCH v5 3/5] drivers core: allow probe_err accept integer and pointer types
On 24.06.2020 14:53, Andy Shevchenko wrote: > On Wed, Jun 24, 2020 at 2:41 PM Andrzej Hajda wrote: >> Many resource acquisition functions return error value encapsulated in >> pointer instead of integer value. To simplify coding we can use macro >> which will accept both types of error. >> With this patch user can use: >> probe_err(dev, ptr, ...) >> instead of: >> probe_err(dev, PTR_ERR(ptr), ...) >> Without loosing old functionality: >> probe_err(dev, err, ...) > ... > >> + * This helper implements common pattern present in probe functions for >> error >> + * checking: print message if the error is not -EPROBE_DEFER and propagate >> it. >> + * In case of -EPROBE_DEFER it sets defer probe reason, which can be checked >> + * later by reading devices_deferred debugfs attribute. >> + * It replaces code sequence: >> + * if (err != -EPROBE_DEFER) >> + * dev_err(dev, ...); > Btw, we have now %pe. Can you consider to use it? OK, I haven't noticed it finally appeared. > >> + * return err; >> + * with >> + * return probe_err(dev, err, ...); >> + * >> + * Returns @err. >> + * >> + */ >> +#define probe_err(dev, err, args...) __probe_err(dev, (long)(err), args) > Can't we use PTR_ERR() here? Nope, I want to accept both types: int and pointer. Regards Andrzej > ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 37/43] drm/tilcdc: Use GEM CMA object functions
On 05/06/2020 10:32, Thomas Zimmermann wrote:
> Create GEM objects with drm_gem_cma_create_object_default_funcs(), which
> allocates the object and sets CMA's default object functions. Corresponding
> callbacks in struct drm_driver are cleared. No functional changes are made.
>
> Driver and object-function instances use the same callback functions, with
> the exception of vunmap. The implementation of vunmap is empty and left out
> in CMA's default object functions.
>
> v3:
> * convert to DRIVER_OPS macro in a separate patch
>
> Signed-off-by: Thomas Zimmermann
> Acked-by: Emil Velikov
Reviewed-by: Jyri Sarha
Tested-by: Jyri Sarha
> ---
> drivers/gpu/drm/tilcdc/tilcdc_drv.c | 7 +--
> 1 file changed, 1 insertion(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> index a5e9ee4c7fbf4..a6582325651bd 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> @@ -496,17 +496,12 @@ DEFINE_DRM_GEM_CMA_FOPS(fops);
> static struct drm_driver tilcdc_driver = {
> .driver_features= DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC,
> .irq_handler= tilcdc_irq,
> - .gem_free_object_unlocked = drm_gem_cma_free_object,
> - .gem_print_info = drm_gem_cma_print_info,
> - .gem_vm_ops = &drm_gem_cma_vm_ops,
> + .gem_create_object = drm_gem_cma_create_object_default_funcs,
> .dumb_create= drm_gem_cma_dumb_create,
>
> .prime_handle_to_fd = drm_gem_prime_handle_to_fd,
> .prime_fd_to_handle = drm_gem_prime_fd_to_handle,
> - .gem_prime_get_sg_table = drm_gem_cma_prime_get_sg_table,
> .gem_prime_import_sg_table = drm_gem_cma_prime_import_sg_table,
> - .gem_prime_vmap = drm_gem_cma_prime_vmap,
> - .gem_prime_vunmap = drm_gem_cma_prime_vunmap,
> .gem_prime_mmap = drm_gem_cma_prime_mmap,
> #ifdef CONFIG_DEBUG_FS
> .debugfs_init = tilcdc_debugfs_init,
>
--
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 v3 38/43] drm/tilcdc: Set GEM CMA functions with DRM_GEM_CMA_DRIVER_OPS
On 05/06/2020 10:32, Thomas Zimmermann wrote:
> DRM_GEM_CMA_DRIVER_OPS sets the functions in struct drm_driver
> to their defaults. No functional changes are made.
>
> Signed-off-by: Thomas Zimmermann
> Acked-by: Emil Velikov
Reviewed-by: Jyri Sarha
Tested-by: Jyri Sarha
> ---
> drivers/gpu/drm/tilcdc/tilcdc_drv.c | 8 +---
> 1 file changed, 1 insertion(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> index a6582325651bd..0d74a64432633 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> @@ -496,13 +496,7 @@ DEFINE_DRM_GEM_CMA_FOPS(fops);
> static struct drm_driver tilcdc_driver = {
> .driver_features= DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC,
> .irq_handler= tilcdc_irq,
> - .gem_create_object = drm_gem_cma_create_object_default_funcs,
> - .dumb_create= drm_gem_cma_dumb_create,
> -
> - .prime_handle_to_fd = drm_gem_prime_handle_to_fd,
> - .prime_fd_to_handle = drm_gem_prime_fd_to_handle,
> - .gem_prime_import_sg_table = drm_gem_cma_prime_import_sg_table,
> - .gem_prime_mmap = drm_gem_cma_prime_mmap,
> + DRM_GEM_CMA_DRIVER_OPS,
> #ifdef CONFIG_DEBUG_FS
> .debugfs_init = tilcdc_debugfs_init,
> #endif
>
--
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
[PATCH v6] drm/fourcc: document modifier uniqueness requirements
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 (except drivers).
v2: add paragraph about aliases (Daniel)
v3: fix unrelated changes sent with the patch
v4: disambiguate users between driver and higher-level programs (Brian,
Daniel)
v5: fix AFBC example (Brian, Daniel)
v6: remove duplicated paragraph (Daniel)
Signed-off-by: Simon Ser
Reviewed-by: Daniel Vetter
Cc: Daniel Stone
Cc: Bas Nieuwenhuizen
Cc: Dave Airlie
Cc: Marek Olšák
Cc: Alex Deucher
Cc: Neil Armstrong
Cc: Michel Dänzer
Cc: Brian Starkey
---
include/uapi/drm/drm_fourcc.h | 24
1 file changed, 24 insertions(+)
diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
index 490143500a50..8eaa158fef81 100644
--- a/include/uapi/drm/drm_fourcc.h
+++ b/include/uapi/drm/drm_fourcc.h
@@ -58,6 +58,30 @@ 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.
+ *
+ * For modifiers where the combination of fourcc code and modifier can alias,
+ * a canonical pair needs to be defined and used by all drivers. Preferred
+ * combinations are also encouraged where all combinations might lead to
+ * confusion and unnecessarily reduced interoperability. An example for the
+ * latter is AFBC, where the ABGR layouts are preferred over ARGB layouts.
+ *
+ * There are two kinds of modifier users:
+ *
+ * - Kernel and user-space drivers: for drivers it's important that modifiers
+ * don't alias, otherwise two drivers might support the same format but use
+ * different aliases, preventing them from sharing buffers in an efficient
+ * format.
+ * - Higher-level programs interfacing with KMS/GBM/EGL/Vulkan/etc: these users
+ * see modifiers as opaque tokens they can check for equality and intersect.
+ * These users musn't need to know to reason about the modifier value
+ * (i.e. they are not expected to extract information out of the modifier).
+ *
* Vendors should document their modifier usage in as much detail as
* possible, to ensure maximum compatibility across devices, drivers and
* applications.
--
2.27.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v5] drm/fourcc: document modifier uniqueness requirements
> > The new paragraph below looks good, but this sentence from the end of > > the paragraph above still needs to be removed: > > An example is AFBC, where both ARGB and ABGR have the exact same compressed > > layout. > > I think that entire paragraph was meant to be deleted, the replacement > is the new one below. Otherwise we have 2x the exact same sentence :-) Err, indeed, sorry about that! Sending a fixed version. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v5] drm/fourcc: document modifier uniqueness requirements
On Wed, Jun 24, 2020 at 1:08 PM Brian Starkey wrote:
>
> Hi,
>
> On Tue, Jun 23, 2020 at 03:25:08PM +, 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 (except drivers).
> >
> > v2: add paragraph about aliases (Daniel)
> >
> > v3: fix unrelated changes sent with the patch
> >
> > v4: disambiguate users between driver and higher-level programs (Brian,
> > Daniel)
> >
> > v5: fix AFBC example (Brian, Daniel)
> >
> > Signed-off-by: Simon Ser
> > Reviewed-by: Daniel Vetter
> > Cc: Daniel Stone
> > Cc: Bas Nieuwenhuizen
> > Cc: Dave Airlie
> > Cc: Marek Olšák
> > Cc: Alex Deucher
> > Cc: Neil Armstrong
> > Cc: Michel Dänzer
> > Cc: Brian Starkey
> > ---
> > include/uapi/drm/drm_fourcc.h | 28
> > 1 file changed, 28 insertions(+)
> >
> > diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> > index 490143500a50..8296197189bf 100644
> > --- a/include/uapi/drm/drm_fourcc.h
> > +++ b/include/uapi/drm/drm_fourcc.h
> > @@ -58,6 +58,34 @@ 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.
> > + *
> > + * 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.
>
> The new paragraph below looks good, but this sentence from the end of
> the paragraph above still needs to be removed:
>
> An example is AFBC, where both ARGB and ABGR have the exact same compressed
> layout.
I think that entire paragraph was meant to be deleted, the replacement
is the new one below. Otherwise we have 2x the exact same sentence :-)
-Daniel
>
> With that fixed:
>
> Reviewed-by: Brian Starkey
>
> Thanks,
> -Brian
>
> > + *
> > + * For modifiers where the combination of fourcc code and modifier can
> > alias,
> > + * a canonical pair needs to be defined and used by all drivers. Preferred
> > + * combinations are also encouraged where all combinations might lead to
> > + * confusion and unnecessarily reduced interoperability. An example for the
> > + * latter is AFBC, where the ABGR layouts are preferred over ARGB layouts.
> > + *
> > + * There are two kinds of modifier users:
> > + *
> > + * - Kernel and user-space drivers: for drivers it's important that
> > modifiers
> > + * don't alias, otherwise two drivers might support the same format but
> > use
> > + * different aliases, preventing them from sharing buffers in an
> > efficient
> > + * format.
> > + * - Higher-level programs interfacing with KMS/GBM/EGL/Vulkan/etc: these
> > users
> > + * see modifiers as opaque tokens they can check for equality and
> > intersect.
> > + * These users musn't need to know to reason about the modifier value
> > + * (i.e. they are not expected to extract information out of the
> > modifier).
> > + *
> > * Vendors should document their modifier usage in as much detail as
> > * possible, to ensure maximum compatibility across devices, drivers and
> > * applications.
> > --
> > 2.27.0
> >
> >
--
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: [RESEND PATCH v5 3/5] drivers core: allow probe_err accept integer and pointer types
On Wed, Jun 24, 2020 at 3:37 PM Robin Murphy wrote: > On 2020-06-24 12:41, Andrzej Hajda wrote: > > Many resource acquisition functions return error value encapsulated in > > pointer instead of integer value. To simplify coding we can use macro > > which will accept both types of error. > > With this patch user can use: > > probe_err(dev, ptr, ...) > > instead of: > > probe_err(dev, PTR_ERR(ptr), ...) > > Without loosing old functionality: > > probe_err(dev, err, ...) > > Personally I'm not convinced that simplification has much value, and I'd > say it *does* have a significant downside. This: > > if (IS_ERR(x)) > do_something_with(PTR_ERR(x)); > > is a familiar and expected pattern when reading/reviewing code, and at a > glance is almost certainly doing the right thing. If I see this, on the > other hand: > > if (IS_ERR(x)) > do_something_with(x); I don't consider your arguments strong enough. You are appealing to one pattern vs. new coming *pattern* just with a different name and actually much less characters to parse. We have a lot of clean ups like this, have you protested against them? JFYI: they are now *established patterns* and saved a ton of LOCs in some of which even were typos. > my immediate instinct is to be suspicious, and now I've got to go off > and double-check that if do_something_with() really expects a pointer > it's also robust against PTR_ERR values. Off-hand I can't think of any > APIs that work that way in the areas with which I'm familiar, so it > would be a pretty unusual and non-obvious thing. -- With Best Regards, Andy Shevchenko ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RESEND PATCH v5 3/5] drivers core: allow probe_err accept integer and pointer types
On Wed, Jun 24, 2020 at 2:41 PM Andrzej Hajda wrote: > > Many resource acquisition functions return error value encapsulated in > pointer instead of integer value. To simplify coding we can use macro > which will accept both types of error. > With this patch user can use: > probe_err(dev, ptr, ...) > instead of: > probe_err(dev, PTR_ERR(ptr), ...) > Without loosing old functionality: > probe_err(dev, err, ...) ... > + * This helper implements common pattern present in probe functions for error > + * checking: print message if the error is not -EPROBE_DEFER and propagate > it. > + * In case of -EPROBE_DEFER it sets defer probe reason, which can be checked > + * later by reading devices_deferred debugfs attribute. > + * It replaces code sequence: > + * if (err != -EPROBE_DEFER) > + * dev_err(dev, ...); Btw, we have now %pe. Can you consider to use it? > + * return err; > + * with > + * return probe_err(dev, err, ...); > + * > + * Returns @err. > + * > + */ > +#define probe_err(dev, err, args...) __probe_err(dev, (long)(err), args) Can't we use PTR_ERR() here? -- With Best Regards, Andy Shevchenko ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RESEND PATCH v5 3/5] drivers core: allow probe_err accept integer and pointer types
On 2020-06-24 12:41, Andrzej Hajda wrote:
Many resource acquisition functions return error value encapsulated in
pointer instead of integer value. To simplify coding we can use macro
which will accept both types of error.
With this patch user can use:
probe_err(dev, ptr, ...)
instead of:
probe_err(dev, PTR_ERR(ptr), ...)
Without loosing old functionality:
probe_err(dev, err, ...)
Personally I'm not convinced that simplification has much value, and I'd
say it *does* have a significant downside. This:
if (IS_ERR(x))
do_something_with(PTR_ERR(x));
is a familiar and expected pattern when reading/reviewing code, and at a
glance is almost certainly doing the right thing. If I see this, on the
other hand:
if (IS_ERR(x))
do_something_with(x);
my immediate instinct is to be suspicious, and now I've got to go off
and double-check that if do_something_with() really expects a pointer
it's also robust against PTR_ERR values. Off-hand I can't think of any
APIs that work that way in the areas with which I'm familiar, so it
would be a pretty unusual and non-obvious thing.
Furthermore, an error helper that explicitly claims to accept "pointer
type" values seems like it could easily lead to misunderstandings like this:
int init_my_buffer(struct my_device *d)
{
d->buffer = kzalloc(d->buffer_size, GFP_KERNEL);
return probe_err(d->dev, d->buffer, "failed to init buffer\n");
}
and allowing that to compile without any hint of an error seems a
little... unfair.
Robin.
Signed-off-by: Andrzej Hajda
---
drivers/base/core.c| 25 ++---
include/linux/device.h | 25 -
2 files changed, 26 insertions(+), 24 deletions(-)
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 2a96954d5460..df283c62d9c0 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -3953,28 +3953,7 @@ define_dev_printk_level(_dev_info, KERN_INFO);
#endif
-/**
- * probe_err - probe error check and log helper
- * @dev: the pointer to the struct device
- * @err: error value to test
- * @fmt: printf-style format string
- * @...: arguments as specified in the format string
- *
- * This helper implements common pattern present in probe functions for error
- * checking: print message if the error is not -EPROBE_DEFER and propagate it.
- * In case of -EPROBE_DEFER it sets defer probe reason, which can be checked
- * later by reading devices_deferred debugfs attribute.
- * It replaces code sequence:
- * if (err != -EPROBE_DEFER)
- * dev_err(dev, ...);
- * return err;
- * with
- * return probe_err(dev, err, ...);
- *
- * Returns @err.
- *
- */
-int probe_err(const struct device *dev, int err, const char *fmt, ...)
+int __probe_err(const struct device *dev, int err, const char *fmt, ...)
{
struct va_format vaf;
va_list args;
@@ -3992,7 +3971,7 @@ int probe_err(const struct device *dev, int err, const
char *fmt, ...)
return err;
}
-EXPORT_SYMBOL_GPL(probe_err);
+EXPORT_SYMBOL_GPL(__probe_err);
static inline bool fwnode_is_primary(struct fwnode_handle *fwnode)
{
diff --git a/include/linux/device.h b/include/linux/device.h
index 40a90d9bf799..22d3c3d4f461 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -965,7 +965,30 @@ void device_links_supplier_sync_state_pause(void);
void device_links_supplier_sync_state_resume(void);
extern __printf(3, 4)
-int probe_err(const struct device *dev, int err, const char *fmt, ...);
+int __probe_err(const struct device *dev, int err, const char *fmt, ...);
+
+/**
+ * probe_err - probe error check and log helper
+ * @dev: the pointer to the struct device
+ * @err: error value to test, can be integer or pointer type
+ * @fmt: printf-style format string
+ * @...: arguments as specified in the format string
+ *
+ * This helper implements common pattern present in probe functions for error
+ * checking: print message if the error is not -EPROBE_DEFER and propagate it.
+ * In case of -EPROBE_DEFER it sets defer probe reason, which can be checked
+ * later by reading devices_deferred debugfs attribute.
+ * It replaces code sequence:
+ * if (err != -EPROBE_DEFER)
+ * dev_err(dev, ...);
+ * return err;
+ * with
+ * return probe_err(dev, err, ...);
+ *
+ * Returns @err.
+ *
+ */
+#define probe_err(dev, err, args...) __probe_err(dev, (long)(err), args)
/* Create alias, so I can be autoloaded. */
#define MODULE_ALIAS_CHARDEV(major,minor) \
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RESEND PATCH v5 2/5] driver core: add deferring probe reason to devices_deferred property
On Wed, Jun 24, 2020 at 01:41:24PM +0200, Andrzej Hajda wrote:
> /sys/kernel/debug/devices_deferred property contains list of deferred devices.
> This list does not contain reason why the driver deferred probe, the patch
> improves it.
> The natural place to set the reason is probe_err function introduced recently,
> ie. if probe_err will be called with -EPROBE_DEFER instead of printk the
> message
> will be attached to deferred device and printed when user read
> devices_deferred
> property.
>
> Signed-off-by: Andrzej Hajda
> Reviewed-by: Mark Brown
> Reviewed-by: Javier Martinez Canillas
> Reviewed-by: Andy Shevchenko
> ---
> drivers/base/base.h | 3 +++
> drivers/base/core.c | 10 ++
> drivers/base/dd.c | 21 -
> 3 files changed, 29 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/base/base.h b/drivers/base/base.h
> index 95c22c0f9036..93ef1c2f4c1f 100644
> --- a/drivers/base/base.h
> +++ b/drivers/base/base.h
> @@ -93,6 +93,7 @@ struct device_private {
> struct klist_node knode_class;
> struct list_head deferred_probe;
> struct device_driver *async_driver;
> + char *deferred_probe_msg;
> struct device *device;
> u8 dead:1;
> };
> @@ -134,6 +135,8 @@ extern void device_release_driver_internal(struct device
> *dev,
> extern void driver_detach(struct device_driver *drv);
> extern int driver_probe_device(struct device_driver *drv, struct device
> *dev);
> extern void driver_deferred_probe_del(struct device *dev);
> +extern void __deferred_probe_set_msg(const struct device *dev,
> + struct va_format *vaf);
> static inline int driver_match_device(struct device_driver *drv,
> struct device *dev)
> {
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index ee9da66bff1b..2a96954d5460 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -3962,6 +3962,8 @@ define_dev_printk_level(_dev_info, KERN_INFO);
> *
> * This helper implements common pattern present in probe functions for error
> * checking: print message if the error is not -EPROBE_DEFER and propagate
> it.
> + * In case of -EPROBE_DEFER it sets defer probe reason, which can be checked
> + * later by reading devices_deferred debugfs attribute.
> * It replaces code sequence:
> * if (err != -EPROBE_DEFER)
> * dev_err(dev, ...);
> @@ -3977,14 +3979,14 @@ int probe_err(const struct device *dev, int err,
> const char *fmt, ...)
> struct va_format vaf;
> va_list args;
>
> - if (err == -EPROBE_DEFER)
> - return err;
> -
> va_start(args, fmt);
> vaf.fmt = fmt;
> vaf.va = &args;
>
> - dev_err(dev, "error %d: %pV", err, &vaf);
> + if (err == -EPROBE_DEFER)
> + __deferred_probe_set_msg(dev, &vaf);
> + else
> + dev_err(dev, "error %d: %pV", err, &vaf);
>
> va_end(args);
>
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index 9a1d940342ac..f44d26454b6a 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -27,6 +27,7 @@
> #include
> #include
> #include
> +#include
>
> #include "base.h"
> #include "power/power.h"
> @@ -136,6 +137,8 @@ void driver_deferred_probe_del(struct device *dev)
> if (!list_empty(&dev->p->deferred_probe)) {
> dev_dbg(dev, "Removed from deferred list\n");
> list_del_init(&dev->p->deferred_probe);
> + kfree(dev->p->deferred_probe_msg);
> + dev->p->deferred_probe_msg = NULL;
> }
> mutex_unlock(&deferred_probe_mutex);
> }
> @@ -211,6 +214,21 @@ void device_unblock_probing(void)
> driver_deferred_probe_trigger();
> }
>
> +/*
> + * __deferred_probe_set_msg() - Set defer probe reason message for device
> + */
> +void __deferred_probe_set_msg(const struct device *dev, struct va_format
> *vaf)
> +{
> + const char *drv = dev_driver_string(dev);
> +
> + mutex_lock(&deferred_probe_mutex);
> +
> + kfree(dev->p->deferred_probe_msg);
> + dev->p->deferred_probe_msg = kasprintf(GFP_KERNEL, "%s: %pV", drv, vaf);
What about the device name? Don't you also want that?
You want the same format that __dev_printk() outputs, please use that
to be consistant with all other messages that drivers are spitting out.
thanks,
greg k-h
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RESEND PATCH v5 1/5] driver core: add probe_err log helper
On Wed, Jun 24, 2020 at 01:41:23PM +0200, Andrzej Hajda wrote:
> During probe every time driver gets resource it should usually check for error
> printk some message if it is not -EPROBE_DEFER and return the error. This
> pattern is simple but requires adding few lines after any resource acquisition
> code, as a result it is often omited or implemented only partially.
> probe_err helps to replace such code sequences with simple call, so code:
> if (err != -EPROBE_DEFER)
> dev_err(dev, ...);
> return err;
> becomes:
> return probe_err(dev, err, ...);
>
> Signed-off-by: Andrzej Hajda
> Reviewed-by: Javier Martinez Canillas
> Reviewed-by: Mark Brown
> Reviewed-by: Andy Shevchenko
> ---
> drivers/base/core.c| 39 +++
> include/linux/device.h | 3 +++
> 2 files changed, 42 insertions(+)
>
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 67d39a90b45c..ee9da66bff1b 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -3953,6 +3953,45 @@ define_dev_printk_level(_dev_info, KERN_INFO);
>
> #endif
>
> +/**
> + * probe_err - probe error check and log helper
> + * @dev: the pointer to the struct device
> + * @err: error value to test
> + * @fmt: printf-style format string
> + * @...: arguments as specified in the format string
> + *
> + * This helper implements common pattern present in probe functions for error
> + * checking: print message if the error is not -EPROBE_DEFER and propagate
> it.
> + * It replaces code sequence:
> + * if (err != -EPROBE_DEFER)
> + * dev_err(dev, ...);
> + * return err;
> + * with
> + * return probe_err(dev, err, ...);
> + *
> + * Returns @err.
> + *
> + */
> +int probe_err(const struct device *dev, int err, const char *fmt, ...)
> +{
> + struct va_format vaf;
> + va_list args;
> +
> + if (err == -EPROBE_DEFER)
> + return err;
> +
> + va_start(args, fmt);
> + vaf.fmt = fmt;
> + vaf.va = &args;
> +
> + dev_err(dev, "error %d: %pV", err, &vaf);
> +
> + va_end(args);
> +
> + return err;
> +}
> +EXPORT_SYMBOL_GPL(probe_err);
Please be specific in global symbols, how about "driver_probe_error()"?
And merge the other patch into this one, as Raphael said, otherwise this
just looks odd to add something and then fix it up later.
thanks,
greg k-h
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RESEND PATCH v5 3/5] drivers core: allow probe_err accept integer and pointer types
On Wed, Jun 24, 2020 at 01:41:25PM +0200, Andrzej Hajda wrote:
> Many resource acquisition functions return error value encapsulated in
> pointer instead of integer value. To simplify coding we can use macro
> which will accept both types of error.
> With this patch user can use:
> probe_err(dev, ptr, ...)
> instead of:
> probe_err(dev, PTR_ERR(ptr), ...)
> Without loosing old functionality:
> probe_err(dev, err, ...)
>
> Signed-off-by: Andrzej Hajda
> ---
> drivers/base/core.c| 25 ++---
> include/linux/device.h | 25 -
> 2 files changed, 26 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 2a96954d5460..df283c62d9c0 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -3953,28 +3953,7 @@ define_dev_printk_level(_dev_info, KERN_INFO);
>
> #endif
>
> -/**
> - * probe_err - probe error check and log helper
> - * @dev: the pointer to the struct device
> - * @err: error value to test
> - * @fmt: printf-style format string
> - * @...: arguments as specified in the format string
> - *
> - * This helper implements common pattern present in probe functions for error
> - * checking: print message if the error is not -EPROBE_DEFER and propagate
> it.
> - * In case of -EPROBE_DEFER it sets defer probe reason, which can be checked
> - * later by reading devices_deferred debugfs attribute.
> - * It replaces code sequence:
> - * if (err != -EPROBE_DEFER)
> - * dev_err(dev, ...);
> - * return err;
> - * with
> - * return probe_err(dev, err, ...);
> - *
> - * Returns @err.
> - *
> - */
> -int probe_err(const struct device *dev, int err, const char *fmt, ...)
> +int __probe_err(const struct device *dev, int err, const char *fmt, ...)
> {
> struct va_format vaf;
> va_list args;
> @@ -3992,7 +3971,7 @@ int probe_err(const struct device *dev, int err, const
> char *fmt, ...)
>
> return err;
> }
> -EXPORT_SYMBOL_GPL(probe_err);
> +EXPORT_SYMBOL_GPL(__probe_err);
>
> static inline bool fwnode_is_primary(struct fwnode_handle *fwnode)
> {
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 40a90d9bf799..22d3c3d4f461 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -965,7 +965,30 @@ void device_links_supplier_sync_state_pause(void);
> void device_links_supplier_sync_state_resume(void);
>
> extern __printf(3, 4)
> -int probe_err(const struct device *dev, int err, const char *fmt, ...);
> +int __probe_err(const struct device *dev, int err, const char *fmt, ...);
> +
> +/**
> + * probe_err - probe error check and log helper
> + * @dev: the pointer to the struct device
> + * @err: error value to test, can be integer or pointer type
> + * @fmt: printf-style format string
> + * @...: arguments as specified in the format string
> + *
> + * This helper implements common pattern present in probe functions for error
> + * checking: print message if the error is not -EPROBE_DEFER and propagate
> it.
> + * In case of -EPROBE_DEFER it sets defer probe reason, which can be checked
> + * later by reading devices_deferred debugfs attribute.
> + * It replaces code sequence:
> + * if (err != -EPROBE_DEFER)
> + * dev_err(dev, ...);
> + * return err;
> + * with
> + * return probe_err(dev, err, ...);
> + *
> + * Returns @err.
> + *
> + */
> +#define probe_err(dev, err, args...) __probe_err(dev, (long)(err), args)
Shouldn't that be "unsigned long" instead of "long"? That's what we put
pointers in last I looked...
thanks,
greg k-h
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RESEND PATCH v5 3/5] drivers core: allow probe_err accept integer and pointer types
On Wed, Jun 24, 2020 at 1:41 PM Andrzej Hajda wrote:
>
> Many resource acquisition functions return error value encapsulated in
> pointer instead of integer value. To simplify coding we can use macro
> which will accept both types of error.
> With this patch user can use:
> probe_err(dev, ptr, ...)
> instead of:
> probe_err(dev, PTR_ERR(ptr), ...)
> Without loosing old functionality:
> probe_err(dev, err, ...)
>
> Signed-off-by: Andrzej Hajda
The separation of this change from patch [1/5] looks kind of artificial to me.
You are introducing a new function anyway, so why not to make it what
you want right away?
> ---
> drivers/base/core.c| 25 ++---
> include/linux/device.h | 25 -
> 2 files changed, 26 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 2a96954d5460..df283c62d9c0 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -3953,28 +3953,7 @@ define_dev_printk_level(_dev_info, KERN_INFO);
>
> #endif
>
> -/**
> - * probe_err - probe error check and log helper
> - * @dev: the pointer to the struct device
> - * @err: error value to test
> - * @fmt: printf-style format string
> - * @...: arguments as specified in the format string
> - *
> - * This helper implements common pattern present in probe functions for error
> - * checking: print message if the error is not -EPROBE_DEFER and propagate
> it.
> - * In case of -EPROBE_DEFER it sets defer probe reason, which can be checked
> - * later by reading devices_deferred debugfs attribute.
> - * It replaces code sequence:
> - * if (err != -EPROBE_DEFER)
> - * dev_err(dev, ...);
> - * return err;
> - * with
> - * return probe_err(dev, err, ...);
> - *
> - * Returns @err.
> - *
> - */
> -int probe_err(const struct device *dev, int err, const char *fmt, ...)
> +int __probe_err(const struct device *dev, int err, const char *fmt, ...)
> {
> struct va_format vaf;
> va_list args;
> @@ -3992,7 +3971,7 @@ int probe_err(const struct device *dev, int err, const
> char *fmt, ...)
>
> return err;
> }
> -EXPORT_SYMBOL_GPL(probe_err);
> +EXPORT_SYMBOL_GPL(__probe_err);
>
> static inline bool fwnode_is_primary(struct fwnode_handle *fwnode)
> {
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 40a90d9bf799..22d3c3d4f461 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -965,7 +965,30 @@ void device_links_supplier_sync_state_pause(void);
> void device_links_supplier_sync_state_resume(void);
>
> extern __printf(3, 4)
> -int probe_err(const struct device *dev, int err, const char *fmt, ...);
> +int __probe_err(const struct device *dev, int err, const char *fmt, ...);
> +
> +/**
> + * probe_err - probe error check and log helper
> + * @dev: the pointer to the struct device
> + * @err: error value to test, can be integer or pointer type
> + * @fmt: printf-style format string
> + * @...: arguments as specified in the format string
> + *
> + * This helper implements common pattern present in probe functions for error
> + * checking: print message if the error is not -EPROBE_DEFER and propagate
> it.
> + * In case of -EPROBE_DEFER it sets defer probe reason, which can be checked
> + * later by reading devices_deferred debugfs attribute.
> + * It replaces code sequence:
> + * if (err != -EPROBE_DEFER)
> + * dev_err(dev, ...);
> + * return err;
> + * with
> + * return probe_err(dev, err, ...);
> + *
> + * Returns @err.
> + *
> + */
> +#define probe_err(dev, err, args...) __probe_err(dev, (long)(err), args)
>
> /* Create alias, so I can be autoloaded. */
> #define MODULE_ALIAS_CHARDEV(major,minor) \
> --
> 2.17.1
>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RESEND PATCH v5 2/5] driver core: add deferring probe reason to devices_deferred property
On Wed, Jun 24, 2020 at 1:41 PM Andrzej Hajda wrote:
>
> /sys/kernel/debug/devices_deferred property contains list of deferred devices.
> This list does not contain reason why the driver deferred probe, the patch
> improves it.
> The natural place to set the reason is probe_err function introduced recently,
> ie. if probe_err will be called with -EPROBE_DEFER instead of printk the
> message
> will be attached to deferred device and printed when user read
> devices_deferred
> property.
>
> Signed-off-by: Andrzej Hajda
> Reviewed-by: Mark Brown
> Reviewed-by: Javier Martinez Canillas
> Reviewed-by: Andy Shevchenko
> ---
> drivers/base/base.h | 3 +++
> drivers/base/core.c | 10 ++
> drivers/base/dd.c | 21 -
> 3 files changed, 29 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/base/base.h b/drivers/base/base.h
> index 95c22c0f9036..93ef1c2f4c1f 100644
> --- a/drivers/base/base.h
> +++ b/drivers/base/base.h
> @@ -93,6 +93,7 @@ struct device_private {
> struct klist_node knode_class;
> struct list_head deferred_probe;
> struct device_driver *async_driver;
> + char *deferred_probe_msg;
What about calling this deferred_probe_reason?
> struct device *device;
> u8 dead:1;
> };
> @@ -134,6 +135,8 @@ extern void device_release_driver_internal(struct device
> *dev,
> extern void driver_detach(struct device_driver *drv);
> extern int driver_probe_device(struct device_driver *drv, struct device
> *dev);
> extern void driver_deferred_probe_del(struct device *dev);
> +extern void __deferred_probe_set_msg(const struct device *dev,
> +struct va_format *vaf);
I'd call this device_set_deferred_probe_reson() to follow the naming
convention for the function names in this header file.
> static inline int driver_match_device(struct device_driver *drv,
> struct device *dev)
> {
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index ee9da66bff1b..2a96954d5460 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -3962,6 +3962,8 @@ define_dev_printk_level(_dev_info, KERN_INFO);
> *
> * This helper implements common pattern present in probe functions for error
> * checking: print message if the error is not -EPROBE_DEFER and propagate
> it.
> + * In case of -EPROBE_DEFER it sets defer probe reason, which can be checked
> + * later by reading devices_deferred debugfs attribute.
> * It replaces code sequence:
> * if (err != -EPROBE_DEFER)
> * dev_err(dev, ...);
> @@ -3977,14 +3979,14 @@ int probe_err(const struct device *dev, int err,
> const char *fmt, ...)
> struct va_format vaf;
> va_list args;
>
> - if (err == -EPROBE_DEFER)
> - return err;
> -
> va_start(args, fmt);
> vaf.fmt = fmt;
> vaf.va = &args;
>
> - dev_err(dev, "error %d: %pV", err, &vaf);
> + if (err == -EPROBE_DEFER)
> + __deferred_probe_set_msg(dev, &vaf);
> + else
> + dev_err(dev, "error %d: %pV", err, &vaf);
>
> va_end(args);
>
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index 9a1d940342ac..f44d26454b6a 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -27,6 +27,7 @@
> #include
> #include
> #include
> +#include
>
> #include "base.h"
> #include "power/power.h"
> @@ -136,6 +137,8 @@ void driver_deferred_probe_del(struct device *dev)
> if (!list_empty(&dev->p->deferred_probe)) {
> dev_dbg(dev, "Removed from deferred list\n");
> list_del_init(&dev->p->deferred_probe);
> + kfree(dev->p->deferred_probe_msg);
> + dev->p->deferred_probe_msg = NULL;
> }
> mutex_unlock(&deferred_probe_mutex);
> }
> @@ -211,6 +214,21 @@ void device_unblock_probing(void)
> driver_deferred_probe_trigger();
> }
>
> +/*
> + * __deferred_probe_set_msg() - Set defer probe reason message for device
I'd change this into a full kerneldoc comment.
> + */
> +void __deferred_probe_set_msg(const struct device *dev, struct va_format
> *vaf)
> +{
> + const char *drv = dev_driver_string(dev);
> +
> + mutex_lock(&deferred_probe_mutex);
> +
> + kfree(dev->p->deferred_probe_msg);
> + dev->p->deferred_probe_msg = kasprintf(GFP_KERNEL, "%s: %pV", drv,
> vaf);
> +
> + mutex_unlock(&deferred_probe_mutex);
> +}
> +
> /*
> * deferred_devs_show() - Show the devices in the deferred probe pending
> list.
> */
> @@ -221,7 +239,8 @@ static int deferred_devs_show(struct seq_file *s, void
> *data)
> mutex_lock(&deferred_probe_mutex);
>
> list_for_each_entry(curr, &deferred_probe_pending_list,
> deferred_probe)
> - seq_printf(s, "%s\n", dev_name(curr->device));
> + seq_printf(s, "%s\t%s", dev_name(curr->device),
> + curr->device->p->deferred_probe_msg ?
[PATCH] drm/radeon: fix array out-of-bounds read and write issues
From: Colin Ian King
There is an off-by-one bounds check on the index into arrays
table->mc_reg_address and table->mc_reg_table_entry[k].mc_data[j] that
can lead to reads and writes outside of arrays. Fix the bound checking
off-by-one error.
Addresses-Coverity: ("Out-of-bounds read/write")
Fixes: cc8dbbb4f62a ("drm/radeon: add dpm support for CI dGPUs (v2)")
Signed-off-by: Colin Ian King
---
drivers/gpu/drm/radeon/ci_dpm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/radeon/ci_dpm.c b/drivers/gpu/drm/radeon/ci_dpm.c
index 134aa2b01f90..86ac032275bb 100644
--- a/drivers/gpu/drm/radeon/ci_dpm.c
+++ b/drivers/gpu/drm/radeon/ci_dpm.c
@@ -4351,7 +4351,7 @@ static int ci_set_mc_special_registers(struct
radeon_device *rdev,
table->mc_reg_table_entry[k].mc_data[j]
|= 0x100;
}
j++;
- if (j > SMU7_DISCRETE_MC_REGISTER_ARRAY_SIZE)
+ if (j >= SMU7_DISCRETE_MC_REGISTER_ARRAY_SIZE)
return -EINVAL;
if (!pi->mem_gddr5) {
--
2.27.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RESEND PATCH v5 1/5] driver core: add probe_err log helper
On Wed, Jun 24, 2020 at 1:41 PM Andrzej Hajda wrote:
>
> During probe every time driver gets resource it should usually check for error
> printk some message if it is not -EPROBE_DEFER and return the error. This
> pattern is simple but requires adding few lines after any resource acquisition
> code, as a result it is often omited or implemented only partially.
> probe_err helps to replace such code sequences with simple call, so code:
> if (err != -EPROBE_DEFER)
> dev_err(dev, ...);
> return err;
> becomes:
> return probe_err(dev, err, ...);
>
> Signed-off-by: Andrzej Hajda
> Reviewed-by: Javier Martinez Canillas
> Reviewed-by: Mark Brown
> Reviewed-by: Andy Shevchenko
Reviewed-by Rafael J. Wysocki
> ---
> drivers/base/core.c| 39 +++
> include/linux/device.h | 3 +++
> 2 files changed, 42 insertions(+)
>
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 67d39a90b45c..ee9da66bff1b 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -3953,6 +3953,45 @@ define_dev_printk_level(_dev_info, KERN_INFO);
>
> #endif
>
> +/**
> + * probe_err - probe error check and log helper
> + * @dev: the pointer to the struct device
> + * @err: error value to test
> + * @fmt: printf-style format string
> + * @...: arguments as specified in the format string
> + *
> + * This helper implements common pattern present in probe functions for error
> + * checking: print message if the error is not -EPROBE_DEFER and propagate
> it.
> + * It replaces code sequence:
> + * if (err != -EPROBE_DEFER)
> + * dev_err(dev, ...);
> + * return err;
> + * with
> + * return probe_err(dev, err, ...);
> + *
> + * Returns @err.
> + *
> + */
> +int probe_err(const struct device *dev, int err, const char *fmt, ...)
> +{
> + struct va_format vaf;
> + va_list args;
> +
> + if (err == -EPROBE_DEFER)
> + return err;
> +
> + va_start(args, fmt);
> + vaf.fmt = fmt;
> + vaf.va = &args;
> +
> + dev_err(dev, "error %d: %pV", err, &vaf);
> +
> + va_end(args);
> +
> + return err;
> +}
> +EXPORT_SYMBOL_GPL(probe_err);
> +
> static inline bool fwnode_is_primary(struct fwnode_handle *fwnode)
> {
> return fwnode && !IS_ERR(fwnode->secondary);
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 15460a5ac024..40a90d9bf799 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -964,6 +964,9 @@ void device_link_remove(void *consumer, struct device
> *supplier);
> void device_links_supplier_sync_state_pause(void);
> void device_links_supplier_sync_state_resume(void);
>
> +extern __printf(3, 4)
> +int probe_err(const struct device *dev, int err, const char *fmt, ...);
> +
> /* Create alias, so I can be autoloaded. */
> #define MODULE_ALIAS_CHARDEV(major,minor) \
> MODULE_ALIAS("char-major-" __stringify(major) "-" __stringify(minor))
> --
> 2.17.1
>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[RESEND PATCH v5 1/5] driver core: add probe_err log helper
During probe every time driver gets resource it should usually check for error
printk some message if it is not -EPROBE_DEFER and return the error. This
pattern is simple but requires adding few lines after any resource acquisition
code, as a result it is often omited or implemented only partially.
probe_err helps to replace such code sequences with simple call, so code:
if (err != -EPROBE_DEFER)
dev_err(dev, ...);
return err;
becomes:
return probe_err(dev, err, ...);
Signed-off-by: Andrzej Hajda
Reviewed-by: Javier Martinez Canillas
Reviewed-by: Mark Brown
Reviewed-by: Andy Shevchenko
---
drivers/base/core.c| 39 +++
include/linux/device.h | 3 +++
2 files changed, 42 insertions(+)
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 67d39a90b45c..ee9da66bff1b 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -3953,6 +3953,45 @@ define_dev_printk_level(_dev_info, KERN_INFO);
#endif
+/**
+ * probe_err - probe error check and log helper
+ * @dev: the pointer to the struct device
+ * @err: error value to test
+ * @fmt: printf-style format string
+ * @...: arguments as specified in the format string
+ *
+ * This helper implements common pattern present in probe functions for error
+ * checking: print message if the error is not -EPROBE_DEFER and propagate it.
+ * It replaces code sequence:
+ * if (err != -EPROBE_DEFER)
+ * dev_err(dev, ...);
+ * return err;
+ * with
+ * return probe_err(dev, err, ...);
+ *
+ * Returns @err.
+ *
+ */
+int probe_err(const struct device *dev, int err, const char *fmt, ...)
+{
+ struct va_format vaf;
+ va_list args;
+
+ if (err == -EPROBE_DEFER)
+ return err;
+
+ va_start(args, fmt);
+ vaf.fmt = fmt;
+ vaf.va = &args;
+
+ dev_err(dev, "error %d: %pV", err, &vaf);
+
+ va_end(args);
+
+ return err;
+}
+EXPORT_SYMBOL_GPL(probe_err);
+
static inline bool fwnode_is_primary(struct fwnode_handle *fwnode)
{
return fwnode && !IS_ERR(fwnode->secondary);
diff --git a/include/linux/device.h b/include/linux/device.h
index 15460a5ac024..40a90d9bf799 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -964,6 +964,9 @@ void device_link_remove(void *consumer, struct device
*supplier);
void device_links_supplier_sync_state_pause(void);
void device_links_supplier_sync_state_resume(void);
+extern __printf(3, 4)
+int probe_err(const struct device *dev, int err, const char *fmt, ...);
+
/* Create alias, so I can be autoloaded. */
#define MODULE_ALIAS_CHARDEV(major,minor) \
MODULE_ALIAS("char-major-" __stringify(major) "-" __stringify(minor))
--
2.17.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[RESEND PATCH v5 4/5] drm/bridge/sii8620: fix resource acquisition error handling
In case of error during resource acquisition driver should print error
message only in case it is not deferred probe, using probe_err helper
solves the issue. Moreover it records defer probe reason for debugging.
Signed-off-by: Andrzej Hajda
---
drivers/gpu/drm/bridge/sil-sii8620.c | 18 ++
1 file changed, 6 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c
b/drivers/gpu/drm/bridge/sil-sii8620.c
index 92acd336aa89..2f825b2d0098 100644
--- a/drivers/gpu/drm/bridge/sil-sii8620.c
+++ b/drivers/gpu/drm/bridge/sil-sii8620.c
@@ -2299,10 +2299,8 @@ static int sii8620_probe(struct i2c_client *client,
INIT_LIST_HEAD(&ctx->mt_queue);
ctx->clk_xtal = devm_clk_get(dev, "xtal");
- if (IS_ERR(ctx->clk_xtal)) {
- dev_err(dev, "failed to get xtal clock from DT\n");
- return PTR_ERR(ctx->clk_xtal);
- }
+ if (IS_ERR(ctx->clk_xtal))
+ return probe_err(dev, ctx->clk_xtal, "failed to get xtal clock
from DT\n");
if (!client->irq) {
dev_err(dev, "no irq provided\n");
@@ -2313,16 +2311,12 @@ static int sii8620_probe(struct i2c_client *client,
sii8620_irq_thread,
IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
"sii8620", ctx);
- if (ret < 0) {
- dev_err(dev, "failed to install IRQ handler\n");
- return ret;
- }
+ if (ret < 0)
+ return probe_err(dev, ret, "failed to install IRQ handler\n");
ctx->gpio_reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
- if (IS_ERR(ctx->gpio_reset)) {
- dev_err(dev, "failed to get reset gpio from DT\n");
- return PTR_ERR(ctx->gpio_reset);
- }
+ if (IS_ERR(ctx->gpio_reset))
+ return probe_err(dev, ctx->gpio_reset, "failed to get reset
gpio from DT\n");
ctx->supplies[0].supply = "cvcc10";
ctx->supplies[1].supply = "iovcc18";
--
2.17.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[RESEND PATCH v5 3/5] drivers core: allow probe_err accept integer and pointer types
Many resource acquisition functions return error value encapsulated in
pointer instead of integer value. To simplify coding we can use macro
which will accept both types of error.
With this patch user can use:
probe_err(dev, ptr, ...)
instead of:
probe_err(dev, PTR_ERR(ptr), ...)
Without loosing old functionality:
probe_err(dev, err, ...)
Signed-off-by: Andrzej Hajda
---
drivers/base/core.c| 25 ++---
include/linux/device.h | 25 -
2 files changed, 26 insertions(+), 24 deletions(-)
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 2a96954d5460..df283c62d9c0 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -3953,28 +3953,7 @@ define_dev_printk_level(_dev_info, KERN_INFO);
#endif
-/**
- * probe_err - probe error check and log helper
- * @dev: the pointer to the struct device
- * @err: error value to test
- * @fmt: printf-style format string
- * @...: arguments as specified in the format string
- *
- * This helper implements common pattern present in probe functions for error
- * checking: print message if the error is not -EPROBE_DEFER and propagate it.
- * In case of -EPROBE_DEFER it sets defer probe reason, which can be checked
- * later by reading devices_deferred debugfs attribute.
- * It replaces code sequence:
- * if (err != -EPROBE_DEFER)
- * dev_err(dev, ...);
- * return err;
- * with
- * return probe_err(dev, err, ...);
- *
- * Returns @err.
- *
- */
-int probe_err(const struct device *dev, int err, const char *fmt, ...)
+int __probe_err(const struct device *dev, int err, const char *fmt, ...)
{
struct va_format vaf;
va_list args;
@@ -3992,7 +3971,7 @@ int probe_err(const struct device *dev, int err, const
char *fmt, ...)
return err;
}
-EXPORT_SYMBOL_GPL(probe_err);
+EXPORT_SYMBOL_GPL(__probe_err);
static inline bool fwnode_is_primary(struct fwnode_handle *fwnode)
{
diff --git a/include/linux/device.h b/include/linux/device.h
index 40a90d9bf799..22d3c3d4f461 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -965,7 +965,30 @@ void device_links_supplier_sync_state_pause(void);
void device_links_supplier_sync_state_resume(void);
extern __printf(3, 4)
-int probe_err(const struct device *dev, int err, const char *fmt, ...);
+int __probe_err(const struct device *dev, int err, const char *fmt, ...);
+
+/**
+ * probe_err - probe error check and log helper
+ * @dev: the pointer to the struct device
+ * @err: error value to test, can be integer or pointer type
+ * @fmt: printf-style format string
+ * @...: arguments as specified in the format string
+ *
+ * This helper implements common pattern present in probe functions for error
+ * checking: print message if the error is not -EPROBE_DEFER and propagate it.
+ * In case of -EPROBE_DEFER it sets defer probe reason, which can be checked
+ * later by reading devices_deferred debugfs attribute.
+ * It replaces code sequence:
+ * if (err != -EPROBE_DEFER)
+ * dev_err(dev, ...);
+ * return err;
+ * with
+ * return probe_err(dev, err, ...);
+ *
+ * Returns @err.
+ *
+ */
+#define probe_err(dev, err, args...) __probe_err(dev, (long)(err), args)
/* Create alias, so I can be autoloaded. */
#define MODULE_ALIAS_CHARDEV(major,minor) \
--
2.17.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
