Re: [PATCH] drm/vc4: hdmi: Fix pointer dereference before check
On Wed, Nov 02, 2022 at 12:10:03PM +0100, José Expósito wrote:
> Hi Maxime,
>
> Thanks a lot for looking into the patch.
>
> On Wed, Nov 02, 2022 at 10:01:53AM +0100, Maxime Ripard wrote:
> > Hi,
> >
> > On Sat, Oct 29, 2022 at 11:34:13AM +0200, José Expósito wrote:
> > > Commit 6bed2ea3cb38 ("drm/vc4: hdmi: Reset link on hotplug") introduced
> > > the vc4_hdmi_reset_link() function. This function dereferences the
> > > "connector" pointer before checking whether it is NULL or not.
> > >
> > > Rework variable assignment to avoid this issue.
> > >
> > > Fixes: 6bed2ea3cb38 ("drm/vc4: hdmi: Reset link on hotplug")
> > > Signed-off-by: José Expósito
> > > ---
> > > drivers/gpu/drm/vc4/vc4_hdmi.c | 10 +++---
> > > 1 file changed, 7 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c
> > > b/drivers/gpu/drm/vc4/vc4_hdmi.c
> > > index 4a73fafca51b..07d058b6afb7 100644
> > > --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> > > +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> > > @@ -319,9 +319,9 @@ static int reset_pipe(struct drm_crtc *crtc,
> > > static int vc4_hdmi_reset_link(struct drm_connector *connector,
> > > struct drm_modeset_acquire_ctx *ctx)
> > > {
> > > - struct drm_device *drm = connector->dev;
> > > - struct vc4_hdmi *vc4_hdmi = connector_to_vc4_hdmi(connector);
> > > - struct drm_encoder *encoder = &vc4_hdmi->encoder.base;
> > > + struct drm_device *drm;
> > > + struct vc4_hdmi *vc4_hdmi;
> > > + struct drm_encoder *encoder;
> > > struct drm_connector_state *conn_state;
> > > struct drm_crtc_state *crtc_state;
> > > struct drm_crtc *crtc;
> > > @@ -332,6 +332,10 @@ static int vc4_hdmi_reset_link(struct drm_connector
> > > *connector,
> > > if (!connector)
> > > return 0;
> > >
> > > + drm = connector->dev;
> > > + vc4_hdmi = connector_to_vc4_hdmi(connector);
> > > + encoder = &vc4_hdmi->encoder.base;
> > > +
> >
> > I don't think that's right. Connector shouldn't be NULL to begin with,
> > how did you notice this?
> >
> > Maxime
>
> This issue was reported by Coverity. At the moment this function is not
> invoked with a NULL connector by any code path. However, since the NULL
> check is present, in my opinion, it makes sense to either remove it or
> make it usefull just in case the preconditions change in the future.
Yeah, it makes sense
I'd ask for a small cosmetic change then, could you add the assignments
where they are actually needed instead of at the top of the function?
Something like
if (!connector)
return 0;
+drm = connector->dev;
ret = drm_modeset_lock(&drm->mode_config.connection_mutex, ctx);
...
+vc4_hdmi = connector_to_vc4_hdmi(connector);
if (!vc4_hdmi_supports_scrambling(vc4_hdmi))
...
Changing the prototype of vc4_hdmi_supports_scrambling to take a struct
vc4_hdmi pointer would also help, it's much more convenient.
Maxime
Re: [PATCH v2 43/65] ASoC: tlv320aic32x4: Add a determine_rate hook
Hi Mark, On Fri, Nov 04, 2022 at 03:59:53PM +, Mark Brown wrote: > On Fri, Nov 04, 2022 at 04:51:23PM +0100, Maxime Ripard wrote: > > > Just filling determine_rate if it's missing with > > __clk_mux_determine_rate will possibly pick different parents, and I'm > > fairly certain that this have never been tested on most platforms, and > > will be completely broken. And I don't really want to play a game of > > whack-a-mole adding that flag everywhere it turns out it's broken. > > Well, hopefully everyone for whom it's an issue currently will be > objecting to this version of the change anyway so we'll either know > where to set the flag or we'll get the whack-a-mole with the series > being merged? I'm sorry, I'm not sure what you mean here. The only issue to fix at the moment is that determine_rate and set_parent aren't coupled, and it led to issues due to oversight. I initially added a warning but Stephen wanted to fix all users in that case and make that an error instead. If I filled __clk_mux_determine_rate into clocks that weren't using it before, I would change their behavior. With that flag set, on all users I add __clk_mux_determine_rate to, the behavior is the same than what we previously had, so the risk of regressions is minimal, and everything should keep going like it was? Maxime
[PATCH v6 00/20] drm/i915/vm_bind: Add VM_BIND functionality
DRM_I915_GEM_VM_BIND/UNBIND ioctls allows UMD to bind/unbind GEM buffer objects (BOs) or sections of a BOs at specified GPU virtual addresses on a specified address space (VM). Multiple mappings can map to the same physical pages of an object (aliasing). These mappings (also referred to as persistent mappings) will be persistent across multiple GPU submissions (execbuf calls) issued by the UMD, without user having to provide a list of all required mappings during each submission (as required by older execbuf mode). This patch series support VM_BIND version 1, as described by the param I915_PARAM_VM_BIND_VERSION. Add new execbuf3 ioctl (I915_GEM_EXECBUFFER3) which only works in vm_bind mode. The vm_bind mode only works with this new execbuf3 ioctl. The new execbuf3 ioctl will not have any execlist support and all the legacy support like relocations etc., are removed. NOTEs: * It is based on below VM_BIND design+uapi rfc. Documentation/gpu/rfc/i915_vm_bind.rst * The IGT RFC series is posted as, [PATCH i-g-t v5 0/12] vm_bind: Add VM_BIND validation support v2: Address various review comments v3: Address review comments and other fixes v4: Remove vm_unbind out fence uapi which is not supported yet, replace vm->vm_bind_mode check with i915_gem_vm_is_vm_bind_mode() v5: Render kernel-doc, use PIN_NOEVICT, limit vm_bind support to non-recoverable faults v6: Rebased, minor fixes, add reserved fields to drm_i915_gem_vm_bind, add new patch for async vm_unbind support Signed-off-by: Niranjana Vishwanathapura Niranjana Vishwanathapura (20): drm/i915/vm_bind: Expose vm lookup function drm/i915/vm_bind: Add __i915_sw_fence_await_reservation() drm/i915/vm_bind: Expose i915_gem_object_max_page_size() drm/i915/vm_bind: Add support to create persistent vma drm/i915/vm_bind: Implement bind and unbind of object drm/i915/vm_bind: Support for VM private BOs drm/i915/vm_bind: Add support to handle object evictions drm/i915/vm_bind: Support persistent vma activeness tracking drm/i915/vm_bind: Add out fence support drm/i915/vm_bind: Abstract out common execbuf functions drm/i915/vm_bind: Use common execbuf functions in execbuf path drm/i915/vm_bind: Implement I915_GEM_EXECBUFFER3 ioctl drm/i915/vm_bind: Update i915_vma_verify_bind_complete() drm/i915/vm_bind: Expose i915_request_await_bind() drm/i915/vm_bind: Handle persistent vmas in execbuf3 drm/i915/vm_bind: userptr dma-resv changes drm/i915/vm_bind: Limit vm_bind mode to non-recoverable contexts drm/i915/vm_bind: Add uapi for user to enable vm_bind_mode drm/i915/vm_bind: Render VM_BIND documentation drm/i915/vm_bind: Async vm_unbind support Documentation/gpu/i915.rst| 78 +- drivers/gpu/drm/i915/Makefile | 3 + drivers/gpu/drm/i915/gem/i915_gem_context.c | 43 +- drivers/gpu/drm/i915/gem/i915_gem_context.h | 17 + drivers/gpu/drm/i915/gem/i915_gem_create.c| 72 +- drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c| 6 + .../gpu/drm/i915/gem/i915_gem_execbuffer.c| 516 +-- .../gpu/drm/i915/gem/i915_gem_execbuffer3.c | 871 ++ .../drm/i915/gem/i915_gem_execbuffer_common.c | 666 + .../drm/i915/gem/i915_gem_execbuffer_common.h | 74 ++ drivers/gpu/drm/i915/gem/i915_gem_ioctls.h| 2 + drivers/gpu/drm/i915/gem/i915_gem_object.c| 3 + drivers/gpu/drm/i915/gem/i915_gem_object.h| 2 + .../gpu/drm/i915/gem/i915_gem_object_types.h | 6 + drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 19 + drivers/gpu/drm/i915/gem/i915_gem_vm_bind.h | 30 + .../drm/i915/gem/i915_gem_vm_bind_object.c| 449 + drivers/gpu/drm/i915/gt/intel_gtt.c | 17 + drivers/gpu/drm/i915/gt/intel_gtt.h | 21 + drivers/gpu/drm/i915/i915_driver.c| 4 + drivers/gpu/drm/i915/i915_drv.h | 2 + drivers/gpu/drm/i915/i915_gem_gtt.c | 39 + drivers/gpu/drm/i915/i915_gem_gtt.h | 3 + drivers/gpu/drm/i915/i915_getparam.c | 3 + drivers/gpu/drm/i915/i915_sw_fence.c | 28 +- drivers/gpu/drm/i915/i915_sw_fence.h | 23 +- drivers/gpu/drm/i915/i915_vma.c | 186 +++- drivers/gpu/drm/i915/i915_vma.h | 68 +- drivers/gpu/drm/i915/i915_vma_types.h | 39 + include/uapi/drm/i915_drm.h | 264 +- 30 files changed, 3008 insertions(+), 546 deletions(-) create mode 100644 drivers/gpu/drm/i915/gem/i915_gem_execbuffer3.c create mode 100644 drivers/gpu/drm/i915/gem/i915_gem_execbuffer_common.c create mode 100644 drivers/gpu/drm/i915/gem/i915_gem_execbuffer_common.h create mode 100644 drivers/gpu/drm/i915/gem/i915_gem_vm_bind.h create mode 100644 drivers/gpu/drm/i915/gem/i915_gem_vm_bind_object.c -- 2.21.0.rc0.32.g243a4c7e27
[PATCH v6 03/20] drm/i915/vm_bind: Expose i915_gem_object_max_page_size()
Expose i915_gem_object_max_page_size() function non-static
which will be used by the vm_bind feature.
Reviewed-by: Matthew Auld
Signed-off-by: Niranjana Vishwanathapura
Signed-off-by: Andi Shyti
---
drivers/gpu/drm/i915/gem/i915_gem_create.c | 18 +-
drivers/gpu/drm/i915/gem/i915_gem_object.h | 2 ++
2 files changed, 15 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_create.c
b/drivers/gpu/drm/i915/gem/i915_gem_create.c
index 33673fe7ee0a..5c6e396ab74d 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_create.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_create.c
@@ -15,10 +15,18 @@
#include "i915_trace.h"
#include "i915_user_extensions.h"
-static u32 object_max_page_size(struct intel_memory_region **placements,
- unsigned int n_placements)
+/**
+ * i915_gem_object_max_page_size() - max of min_page_size of the regions
+ * @placements: list of regions
+ * @n_placements: number of the placements
+ *
+ * Returns the largest of min_page_size of the @placements,
+ * or I915_GTT_PAGE_SIZE_4K if @n_placements is 0.
+ */
+u32 i915_gem_object_max_page_size(struct intel_memory_region **placements,
+ unsigned int n_placements)
{
- u32 max_page_size = 0;
+ u32 max_page_size = I915_GTT_PAGE_SIZE_4K;
int i;
for (i = 0; i < n_placements; i++) {
@@ -28,7 +36,6 @@ static u32 object_max_page_size(struct intel_memory_region
**placements,
max_page_size = max_t(u32, max_page_size, mr->min_page_size);
}
- GEM_BUG_ON(!max_page_size);
return max_page_size;
}
@@ -99,7 +106,8 @@ __i915_gem_object_create_user_ext(struct drm_i915_private
*i915, u64 size,
i915_gem_flush_free_objects(i915);
- size = round_up(size, object_max_page_size(placements, n_placements));
+ size = round_up(size, i915_gem_object_max_page_size(placements,
+ n_placements));
if (size == 0)
return ERR_PTR(-EINVAL);
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h
b/drivers/gpu/drm/i915/gem/i915_gem_object.h
index 6b9ecff42bb5..db3dd0e285c5 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h
@@ -47,6 +47,8 @@ static inline bool i915_gem_object_size_2big(u64 size)
}
void i915_gem_init__objects(struct drm_i915_private *i915);
+u32 i915_gem_object_max_page_size(struct intel_memory_region **placements,
+ unsigned int n_placements);
void i915_objects_module_exit(void);
int i915_objects_module_init(void);
--
2.21.0.rc0.32.g243a4c7e27
[PATCH v6 11/20] drm/i915/vm_bind: Use common execbuf functions in execbuf path
Update the execbuf path to use common execbuf functions to
reduce code duplication with the newer execbuf3 path.
Reviewed-by: Matthew Auld
Signed-off-by: Niranjana Vishwanathapura
---
.../gpu/drm/i915/gem/i915_gem_execbuffer.c| 507 ++
1 file changed, 38 insertions(+), 469 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
index 43f29acfbec9..749c3c80e02d 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
@@ -28,6 +28,7 @@
#include "i915_file_private.h"
#include "i915_gem_clflush.h"
#include "i915_gem_context.h"
+#include "i915_gem_execbuffer_common.h"
#include "i915_gem_evict.h"
#include "i915_gem_ioctls.h"
#include "i915_trace.h"
@@ -235,13 +236,6 @@ enum {
* the batchbuffer in trusted mode, otherwise the ioctl is rejected.
*/
-struct eb_fence {
- struct drm_syncobj *syncobj; /* Use with ptr_mask_bits() */
- struct dma_fence *dma_fence;
- u64 value;
- struct dma_fence_chain *chain_fence;
-};
-
struct i915_execbuffer {
struct drm_i915_private *i915; /** i915 backpointer */
struct drm_file *file; /** per-file lookup tables and limits */
@@ -2446,164 +2440,29 @@ static const enum intel_engine_id user_ring_map[] = {
[I915_EXEC_VEBOX] = VECS0
};
-static struct i915_request *eb_throttle(struct i915_execbuffer *eb, struct
intel_context *ce)
-{
- struct intel_ring *ring = ce->ring;
- struct intel_timeline *tl = ce->timeline;
- struct i915_request *rq;
-
- /*
-* Completely unscientific finger-in-the-air estimates for suitable
-* maximum user request size (to avoid blocking) and then backoff.
-*/
- if (intel_ring_update_space(ring) >= PAGE_SIZE)
- return NULL;
-
- /*
-* Find a request that after waiting upon, there will be at least half
-* the ring available. The hysteresis allows us to compete for the
-* shared ring and should mean that we sleep less often prior to
-* claiming our resources, but not so long that the ring completely
-* drains before we can submit our next request.
-*/
- list_for_each_entry(rq, &tl->requests, link) {
- if (rq->ring != ring)
- continue;
-
- if (__intel_ring_space(rq->postfix,
- ring->emit, ring->size) > ring->size / 2)
- break;
- }
- if (&rq->link == &tl->requests)
- return NULL; /* weird, we will check again later for real */
-
- return i915_request_get(rq);
-}
-
-static int eb_pin_timeline(struct i915_execbuffer *eb, struct intel_context
*ce,
- bool throttle)
-{
- struct intel_timeline *tl;
- struct i915_request *rq = NULL;
-
- /*
-* Take a local wakeref for preparing to dispatch the execbuf as
-* we expect to access the hardware fairly frequently in the
-* process, and require the engine to be kept awake between accesses.
-* Upon dispatch, we acquire another prolonged wakeref that we hold
-* until the timeline is idle, which in turn releases the wakeref
-* taken on the engine, and the parent device.
-*/
- tl = intel_context_timeline_lock(ce);
- if (IS_ERR(tl))
- return PTR_ERR(tl);
-
- intel_context_enter(ce);
- if (throttle)
- rq = eb_throttle(eb, ce);
- intel_context_timeline_unlock(tl);
-
- if (rq) {
- bool nonblock = eb->file->filp->f_flags & O_NONBLOCK;
- long timeout = nonblock ? 0 : MAX_SCHEDULE_TIMEOUT;
-
- if (i915_request_wait(rq, I915_WAIT_INTERRUPTIBLE,
- timeout) < 0) {
- i915_request_put(rq);
-
- /*
-* Error path, cannot use intel_context_timeline_lock as
-* that is user interruptable and this clean up step
-* must be done.
-*/
- mutex_lock(&ce->timeline->mutex);
- intel_context_exit(ce);
- mutex_unlock(&ce->timeline->mutex);
-
- if (nonblock)
- return -EWOULDBLOCK;
- else
- return -EINTR;
- }
- i915_request_put(rq);
- }
-
- return 0;
-}
-
static int eb_pin_engine(struct i915_execbuffer *eb, bool throttle)
{
- struct intel_context *ce = eb->context, *child;
int err;
- int i = 0, j = 0;
GEM_BUG_ON(eb->args->flags & __EXEC_ENGINE_PINNED);
- if (unlikely(intel_context_is_banned(ce)))
- return -EIO;
-
- /*
-* Pinning the contexts may
[PATCH v6 05/20] drm/i915/vm_bind: Implement bind and unbind of object
Add uapi and implement support for bind and unbind of an
object at the specified GPU virtual addresses.
The vm_bind mode is not supported in legacy execbuf2 ioctl.
It will be supported only in the newer execbuf3 ioctl.
v2: On older platforms ctx->vm is not set, check for it.
In vm_bind call, add vma to vm_bind_list.
Add more input validity checks.
Update some documentation.
v3: In vm_bind call, add vma to vm_bound_list as user can
request a fence and pass to execbuf3 as input fence.
Remove short term pinning with PIN_VALIDATE flag.
v4: Replace vm->vm_bind_mode check with i915_gem_vm_is_vm_bind_mode().
v5: Ensure all reserved fields are 0, use PIN_NOEVICT.
v6: Add reserved fields to drm_i915_gem_vm_bind.
Reviewed-by: Matthew Auld
Signed-off-by: Niranjana Vishwanathapura
Signed-off-by: Prathap Kumar Valsan
Signed-off-by: Andi Shyti
---
drivers/gpu/drm/i915/Makefile | 1 +
drivers/gpu/drm/i915/gem/i915_gem_context.h | 15 +
.../gpu/drm/i915/gem/i915_gem_execbuffer.c| 5 +
drivers/gpu/drm/i915/gem/i915_gem_vm_bind.h | 26 ++
.../drm/i915/gem/i915_gem_vm_bind_object.c| 324 ++
drivers/gpu/drm/i915/gt/intel_gtt.c | 10 +
drivers/gpu/drm/i915/gt/intel_gtt.h | 9 +
drivers/gpu/drm/i915/i915_driver.c| 3 +
drivers/gpu/drm/i915/i915_vma.c | 1 +
drivers/gpu/drm/i915/i915_vma_types.h | 14 +
include/uapi/drm/i915_drm.h | 99 ++
11 files changed, 507 insertions(+)
create mode 100644 drivers/gpu/drm/i915/gem/i915_gem_vm_bind.h
create mode 100644 drivers/gpu/drm/i915/gem/i915_gem_vm_bind_object.c
diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 51704b54317c..b731f3ac80da 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -166,6 +166,7 @@ gem-y += \
gem/i915_gem_ttm_move.o \
gem/i915_gem_ttm_pm.o \
gem/i915_gem_userptr.o \
+ gem/i915_gem_vm_bind_object.o \
gem/i915_gem_wait.o \
gem/i915_gemfs.o
i915-y += \
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.h
b/drivers/gpu/drm/i915/gem/i915_gem_context.h
index 899fa8f1e0fe..e8b41aa8f8c4 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.h
@@ -139,6 +139,21 @@ int i915_gem_context_setparam_ioctl(struct drm_device
*dev, void *data,
int i915_gem_context_reset_stats_ioctl(struct drm_device *dev, void *data,
struct drm_file *file);
+/**
+ * i915_gem_vm_is_vm_bind_mode() - Check if address space is in vm_bind mode
+ * @vm: the address space
+ *
+ * Returns:
+ * true: @vm is in vm_bind mode; allows only vm_bind method of binding.
+ * false: @vm is not in vm_bind mode; allows only legacy execbuff method
+ *of binding.
+ */
+static inline bool i915_gem_vm_is_vm_bind_mode(struct i915_address_space *vm)
+{
+ /* No support to enable vm_bind mode yet */
+ return false;
+}
+
struct i915_address_space *
i915_gem_vm_lookup(struct drm_i915_file_private *file_priv, u32 id);
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
index 1160723c9d2d..c5bc9f6e887f 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
@@ -781,6 +781,11 @@ static int eb_select_context(struct i915_execbuffer *eb)
if (unlikely(IS_ERR(ctx)))
return PTR_ERR(ctx);
+ if (ctx->vm && i915_gem_vm_is_vm_bind_mode(ctx->vm)) {
+ i915_gem_context_put(ctx);
+ return -EOPNOTSUPP;
+ }
+
eb->gem_context = ctx;
if (i915_gem_context_has_full_ppgtt(ctx))
eb->invalid_flags |= EXEC_OBJECT_NEEDS_GTT;
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_vm_bind.h
b/drivers/gpu/drm/i915/gem/i915_gem_vm_bind.h
new file mode 100644
index ..36262a6357b5
--- /dev/null
+++ b/drivers/gpu/drm/i915/gem/i915_gem_vm_bind.h
@@ -0,0 +1,26 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright © 2022 Intel Corporation
+ */
+
+#ifndef __I915_GEM_VM_BIND_H
+#define __I915_GEM_VM_BIND_H
+
+#include
+
+struct drm_device;
+struct drm_file;
+struct i915_address_space;
+struct i915_vma;
+
+struct i915_vma *
+i915_gem_vm_bind_lookup_vma(struct i915_address_space *vm, u64 va);
+
+int i915_gem_vm_bind_ioctl(struct drm_device *dev, void *data,
+ struct drm_file *file);
+int i915_gem_vm_unbind_ioctl(struct drm_device *dev, void *data,
+struct drm_file *file);
+
+void i915_gem_vm_unbind_all(struct i915_address_space *vm);
+
+#endif /* __I915_GEM_VM_BIND_H */
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_vm_bind_object.c
b/drivers/gpu/drm/i915/gem/i915_gem_vm_bind_object.c
new file mode 100644
index ..6f299806bee1
--- /dev/null
+++ b/drivers/gpu/drm/i915/gem/i915_gem_vm_bind_object.c
@
[PATCH v6 01/20] drm/i915/vm_bind: Expose vm lookup function
Make i915_gem_vm_lookup() function non-static as it will be
used by the vm_bind feature.
Reviewed-by: Matthew Auld
Signed-off-by: Niranjana Vishwanathapura
Signed-off-by: Andi Shyti
---
drivers/gpu/drm/i915/gem/i915_gem_context.c | 11 ++-
drivers/gpu/drm/i915/gem/i915_gem_context.h | 3 +++
2 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c
b/drivers/gpu/drm/i915/gem/i915_gem_context.c
index 01402f3c58f6..6bed0633f744 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
@@ -346,7 +346,16 @@ static int proto_context_register(struct
drm_i915_file_private *fpriv,
return ret;
}
-static struct i915_address_space *
+/**
+ * i915_gem_vm_lookup() - looks up for the VM reference given the vm id
+ * @file_priv: the private data associated with the user's file
+ * @id: the VM id
+ *
+ * Finds the VM reference associated to a specific id.
+ *
+ * Returns the VM pointer on success, NULL in case of failure.
+ */
+struct i915_address_space *
i915_gem_vm_lookup(struct drm_i915_file_private *file_priv, u32 id)
{
struct i915_address_space *vm;
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.h
b/drivers/gpu/drm/i915/gem/i915_gem_context.h
index e5b0f66ea1fe..899fa8f1e0fe 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.h
@@ -139,6 +139,9 @@ int i915_gem_context_setparam_ioctl(struct drm_device *dev,
void *data,
int i915_gem_context_reset_stats_ioctl(struct drm_device *dev, void *data,
struct drm_file *file);
+struct i915_address_space *
+i915_gem_vm_lookup(struct drm_i915_file_private *file_priv, u32 id);
+
struct i915_gem_context *
i915_gem_context_lookup(struct drm_i915_file_private *file_priv, u32 id);
--
2.21.0.rc0.32.g243a4c7e27
[PATCH v6 06/20] drm/i915/vm_bind: Support for VM private BOs
Each VM creates a root_obj and shares it with all of its private objects
to use it as dma_resv object. This has a performance advantage as it
requires a single dma_resv object update for all private BOs vs list of
dma_resv objects update for shared BOs, in the execbuf path.
VM private BOs can be only mapped on specified VM and cannot be dmabuf
exported. Also, they are supported only in vm_bind mode.
v2: Pad struct drm_i915_gem_create_ext_vm_private for 64bit alignment,
add input validity checks.
v3: Create root_obj only for ppgtt.
v4: Fix releasing of obj->priv_root. Do not create vm->root_obj yet.
Allow vm private object creation only in vm_bind mode.
Replace vm->vm_bind_mode check with i915_gem_vm_is_vm_bind_mode().
Reviewed-by: Matthew Auld
Signed-off-by: Niranjana Vishwanathapura
Signed-off-by: Andi Shyti
---
drivers/gpu/drm/i915/gem/i915_gem_context.c | 1 +
drivers/gpu/drm/i915/gem/i915_gem_create.c| 54 ++-
drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c| 6 +++
.../gpu/drm/i915/gem/i915_gem_execbuffer.c| 4 ++
drivers/gpu/drm/i915/gem/i915_gem_object.c| 3 ++
.../gpu/drm/i915/gem/i915_gem_object_types.h | 6 +++
.../drm/i915/gem/i915_gem_vm_bind_object.c| 9
drivers/gpu/drm/i915/gt/intel_gtt.c | 1 +
drivers/gpu/drm/i915/gt/intel_gtt.h | 4 ++
drivers/gpu/drm/i915/i915_vma.c | 1 +
drivers/gpu/drm/i915/i915_vma_types.h | 2 +
include/uapi/drm/i915_drm.h | 33
12 files changed, 122 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c
b/drivers/gpu/drm/i915/gem/i915_gem_context.c
index 6bed0633f744..1630a52f387d 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
@@ -83,6 +83,7 @@
#include "i915_file_private.h"
#include "i915_gem_context.h"
+#include "i915_gem_internal.h"
#include "i915_trace.h"
#include "i915_user_extensions.h"
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_create.c
b/drivers/gpu/drm/i915/gem/i915_gem_create.c
index 5c6e396ab74d..62648341780b 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_create.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_create.c
@@ -11,6 +11,7 @@
#include "pxp/intel_pxp.h"
#include "i915_drv.h"
+#include "i915_gem_context.h"
#include "i915_gem_create.h"
#include "i915_trace.h"
#include "i915_user_extensions.h"
@@ -251,6 +252,7 @@ struct create_ext {
unsigned int n_placements;
unsigned int placement_mask;
unsigned long flags;
+ u32 vm_id;
};
static void repr_placements(char *buf, size_t size,
@@ -400,9 +402,32 @@ static int ext_set_protected(struct i915_user_extension
__user *base, void *data
return 0;
}
+static int ext_set_vm_private(struct i915_user_extension __user *base,
+ void *data)
+{
+ struct drm_i915_gem_create_ext_vm_private ext;
+ struct create_ext *ext_data = data;
+
+ if (copy_from_user(&ext, base, sizeof(ext)))
+ return -EFAULT;
+
+ /* Reserved fields must be 0 */
+ if (ext.rsvd)
+ return -EINVAL;
+
+ /* vm_id 0 is reserved */
+ if (!ext.vm_id)
+ return -ENOENT;
+
+ ext_data->vm_id = ext.vm_id;
+
+ return 0;
+}
+
static const i915_user_extension_fn create_extensions[] = {
[I915_GEM_CREATE_EXT_MEMORY_REGIONS] = ext_set_placements,
[I915_GEM_CREATE_EXT_PROTECTED_CONTENT] = ext_set_protected,
+ [I915_GEM_CREATE_EXT_VM_PRIVATE] = ext_set_vm_private,
};
/**
@@ -418,6 +443,7 @@ i915_gem_create_ext_ioctl(struct drm_device *dev, void
*data,
struct drm_i915_private *i915 = to_i915(dev);
struct drm_i915_gem_create_ext *args = data;
struct create_ext ext_data = { .i915 = i915 };
+ struct i915_address_space *vm = NULL;
struct drm_i915_gem_object *obj;
int ret;
@@ -431,6 +457,17 @@ i915_gem_create_ext_ioctl(struct drm_device *dev, void
*data,
if (ret)
return ret;
+ if (ext_data.vm_id) {
+ vm = i915_gem_vm_lookup(file->driver_priv, ext_data.vm_id);
+ if (unlikely(!vm))
+ return -ENOENT;
+
+ if (!i915_gem_vm_is_vm_bind_mode(vm)) {
+ ret = -EINVAL;
+ goto vm_put;
+ }
+ }
+
if (!ext_data.n_placements) {
ext_data.placements[0] =
intel_memory_region_by_type(i915, INTEL_MEMORY_SYSTEM);
@@ -457,8 +494,21 @@ i915_gem_create_ext_ioctl(struct drm_device *dev, void
*data,
ext_data.placements,
ext_data.n_placements,
ext_data.flags);
- if (IS_ERR(obj))
- return PTR_ERR(obj);
+ if (IS_ERR(obj)) {
+ ret = PTR_E
[PATCH v6 13/20] drm/i915/vm_bind: Update i915_vma_verify_bind_complete()
Ensure i915_vma_verify_bind_complete() handles case where bind
is not initiated. Also make it non static, add documentation
and move it out of CONFIG_DRM_I915_DEBUG_GEM.
v2: Fix fence leak
Reviewed-by: Matthew Auld
Signed-off-by: Niranjana Vishwanathapura
Signed-off-by: Andi Shyti
---
drivers/gpu/drm/i915/i915_vma.c | 22 --
drivers/gpu/drm/i915/i915_vma.h | 1 +
2 files changed, 17 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
index eaa13e9ba966..aa4705246993 100644
--- a/drivers/gpu/drm/i915/i915_vma.c
+++ b/drivers/gpu/drm/i915/i915_vma.c
@@ -439,12 +439,25 @@ int i915_vma_sync(struct i915_vma *vma)
return i915_vm_sync(vma->vm);
}
-#if IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM)
-static int i915_vma_verify_bind_complete(struct i915_vma *vma)
+/**
+ * i915_vma_verify_bind_complete() - Check for the bind completion of the vma
+ * @vma: vma to check for bind completion
+ *
+ * As the fence reference is obtained under RCU, no locking is required by
+ * the caller.
+ *
+ * Returns: 0 if the vma bind is completed. Error code otherwise.
+ */
+int i915_vma_verify_bind_complete(struct i915_vma *vma)
{
- struct dma_fence *fence = i915_active_fence_get(&vma->active.excl);
+ struct dma_fence *fence;
int err;
+ /* Ensure vma bind is initiated */
+ if (!i915_vma_is_bound(vma, I915_VMA_BIND_MASK))
+ return -EINVAL;
+
+ fence = i915_active_fence_get(&vma->active.excl);
if (!fence)
return 0;
@@ -457,9 +470,6 @@ static int i915_vma_verify_bind_complete(struct i915_vma
*vma)
return err;
}
-#else
-#define i915_vma_verify_bind_complete(_vma) 0
-#endif
I915_SELFTEST_EXPORT void
i915_vma_resource_init_from_vma(struct i915_vma_resource *vma_res,
diff --git a/drivers/gpu/drm/i915/i915_vma.h b/drivers/gpu/drm/i915/i915_vma.h
index 1cadbf8fdedf..04770f8ba815 100644
--- a/drivers/gpu/drm/i915/i915_vma.h
+++ b/drivers/gpu/drm/i915/i915_vma.h
@@ -440,6 +440,7 @@ void i915_vma_make_purgeable(struct i915_vma *vma);
int i915_vma_wait_for_bind(struct i915_vma *vma);
int i915_vma_sync(struct i915_vma *vma);
+int i915_vma_verify_bind_complete(struct i915_vma *vma);
/**
* i915_vma_get_current_resource - Get the current resource of the vma
--
2.21.0.rc0.32.g243a4c7e27
[PATCH v6 12/20] drm/i915/vm_bind: Implement I915_GEM_EXECBUFFER3 ioctl
Implement new execbuf3 ioctl (I915_GEM_EXECBUFFER3) which only
works in vm_bind mode. The vm_bind mode only works with
this new execbuf3 ioctl.
The new execbuf3 ioctl will not have any list of objects to validate
bind as all required objects binding would have been requested by the
userspace before submitting the execbuf3.
Legacy features like relocations etc are not supported by execbuf3.
v2: Add more input validity checks.
v3: batch_address is a VA (not an array) if num_batches=1,
minor cleanup
v4: replace vm->vm_bind_mode check with i915_gem_vm_is_vm_bind_mode()
v5: Remove unwanted krealloc() and address other review comments.
Reviewed-by: Matthew Auld
Signed-off-by: Niranjana Vishwanathapura
Signed-off-by: Andi Shyti
---
drivers/gpu/drm/i915/Makefile | 1 +
.../gpu/drm/i915/gem/i915_gem_execbuffer3.c | 578 ++
drivers/gpu/drm/i915/gem/i915_gem_ioctls.h| 2 +
drivers/gpu/drm/i915/i915_driver.c| 1 +
include/uapi/drm/i915_drm.h | 61 ++
5 files changed, 643 insertions(+)
create mode 100644 drivers/gpu/drm/i915/gem/i915_gem_execbuffer3.c
diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 35636c6bf856..0fbdbb571709 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -150,6 +150,7 @@ gem-y += \
gem/i915_gem_domain.o \
gem/i915_gem_execbuffer_common.o \
gem/i915_gem_execbuffer.o \
+ gem/i915_gem_execbuffer3.o \
gem/i915_gem_internal.o \
gem/i915_gem_object.o \
gem/i915_gem_lmem.o \
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer3.c
b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer3.c
new file mode 100644
index ..64251dc4cf91
--- /dev/null
+++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer3.c
@@ -0,0 +1,578 @@
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright © 2022 Intel Corporation
+ */
+
+#include
+#include
+
+#include
+
+#include "gt/intel_context.h"
+#include "gt/intel_gpu_commands.h"
+#include "gt/intel_gt.h"
+
+#include "i915_drv.h"
+#include "i915_gem_context.h"
+#include "i915_gem_execbuffer_common.h"
+#include "i915_gem_ioctls.h"
+#include "i915_gem_vm_bind.h"
+#include "i915_trace.h"
+
+#define __EXEC3_ENGINE_PINNED BIT_ULL(32)
+#define __EXEC3_INTERNAL_FLAGS (~0ull << 32)
+
+/* Catch emission of unexpected errors for CI! */
+#if IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM)
+#undef EINVAL
+#define EINVAL ({ \
+ DRM_DEBUG_DRIVER("EINVAL at %s:%d\n", __func__, __LINE__); \
+ 22; \
+})
+#endif
+
+/**
+ * DOC: User command execution in vm_bind mode
+ *
+ * A VM in VM_BIND mode will not support older execbuf mode of binding.
+ * The execbuf ioctl handling in VM_BIND mode differs significantly from the
+ * older execbuf2 ioctl (See struct drm_i915_gem_execbuffer2).
+ * Hence, a new execbuf3 ioctl has been added to support VM_BIND mode. (See
+ * struct drm_i915_gem_execbuffer3). The execbuf3 ioctl will not accept any
+ * execlist. Hence, no support for implicit sync.
+ *
+ * The new execbuf3 ioctl only works in VM_BIND mode and the VM_BIND mode only
+ * works with execbuf3 ioctl for submission.
+ *
+ * The execbuf3 ioctl directly specifies the batch addresses instead of as
+ * object handles as in execbuf2 ioctl. The execbuf3 ioctl will also not
+ * support many of the older features like in/out/submit fences, fence array,
+ * default gem context etc. (See struct drm_i915_gem_execbuffer3).
+ *
+ * In VM_BIND mode, VA allocation is completely managed by the user instead of
+ * the i915 driver. Hence all VA assignment, eviction are not applicable in
+ * VM_BIND mode. Also, for determining object activeness, VM_BIND mode will not
+ * be using the i915_vma active reference tracking. It will instead check the
+ * dma-resv object's fence list for that.
+ *
+ * So, a lot of code supporting execbuf2 ioctl, like relocations, VA evictions,
+ * vma lookup table, implicit sync, vma active reference tracking etc., are not
+ * applicable for execbuf3 ioctl.
+ */
+
+/**
+ * struct i915_execbuffer - execbuf struct for execbuf3
+ * @i915: reference to the i915 instance we run on
+ * @file: drm file reference
+ * @args: execbuf3 ioctl structure
+ * @gt: reference to the gt instance ioctl submitted for
+ * @context: logical state for the request
+ * @gem_context: callers context
+ * @requests: requests to be build
+ * @composite_fence: used for excl fence in dma_resv objects when > 1 BB
submitted
+ * @ww: i915_gem_ww_ctx instance
+ * @num_batches: number of batches submitted
+ * @batch_addresses: addresses corresponds to the submitted batches
+ * @batches: references to the i915_vmas corresponding to the batches
+ * @fences: array of execbuf fences (See struct eb_fence)
+ * @num_fences: number of fences in @fences array
+ */
+struct i915_execbuffer {
+ struct drm_i915_private *i915;
+ struct drm_file *file;
+ struct drm_i915_gem_execbuffer3 *args;
+
+ str
[PATCH v6 20/20] drm/i915/vm_bind: Async vm_unbind support
Asynchronously unbind the vma upon vm_unbind call.
Fall back to synchronous unbind if backend doesn't support
async unbind or if async unbind fails.
No need for vm_unbind out fence support as i915 will internally
handle all sequencing and user need not try to sequence any
operation with the unbind completion.
Signed-off-by: Niranjana Vishwanathapura
---
drivers/gpu/drm/i915/i915_vma.c | 51 ++---
drivers/gpu/drm/i915/i915_vma.h | 1 +
2 files changed, 48 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
index 08218e3a2f12..03c966fad87b 100644
--- a/drivers/gpu/drm/i915/i915_vma.c
+++ b/drivers/gpu/drm/i915/i915_vma.c
@@ -42,6 +42,8 @@
#include "i915_vma.h"
#include "i915_vma_resource.h"
+static struct dma_fence *__i915_vma_unbind_async(struct i915_vma *vma);
+
static inline void assert_vma_held_evict(const struct i915_vma *vma)
{
/*
@@ -1711,7 +1713,7 @@ void i915_vma_reopen(struct i915_vma *vma)
spin_unlock_irq(>->closed_lock);
}
-static void force_unbind(struct i915_vma *vma)
+static void force_unbind(struct i915_vma *vma, bool async)
{
if (!drm_mm_node_allocated(&vma->node))
return;
@@ -1725,7 +1727,21 @@ static void force_unbind(struct i915_vma *vma)
i915_vma_set_purged(vma);
atomic_and(~I915_VMA_PIN_MASK, &vma->flags);
- WARN_ON(__i915_vma_unbind(vma));
+ if (async) {
+ struct dma_fence *fence;
+
+ fence = __i915_vma_unbind_async(vma);
+ if (IS_ERR_OR_NULL(fence)) {
+ async = false;
+ } else {
+ dma_resv_add_fence(vma->obj->base.resv, fence,
+ DMA_RESV_USAGE_READ);
+ dma_fence_put(fence);
+ }
+ }
+
+ if (!async)
+ WARN_ON(__i915_vma_unbind(vma));
GEM_BUG_ON(drm_mm_node_allocated(&vma->node));
}
@@ -1785,7 +1801,7 @@ void i915_vma_destroy_locked(struct i915_vma *vma)
{
lockdep_assert_held(&vma->vm->mutex);
- force_unbind(vma);
+ force_unbind(vma, false);
list_del_init(&vma->vm_link);
release_references(vma, vma->vm->gt, false);
}
@@ -1796,7 +1812,34 @@ void i915_vma_destroy(struct i915_vma *vma)
bool vm_ddestroy;
mutex_lock(&vma->vm->mutex);
- force_unbind(vma);
+ force_unbind(vma, false);
+ list_del_init(&vma->vm_link);
+ vm_ddestroy = vma->vm_ddestroy;
+ vma->vm_ddestroy = false;
+
+ /* vma->vm may be freed when releasing vma->vm->mutex. */
+ gt = vma->vm->gt;
+ mutex_unlock(&vma->vm->mutex);
+ release_references(vma, gt, vm_ddestroy);
+}
+
+void i915_vma_destroy_async(struct i915_vma *vma)
+{
+ bool vm_ddestroy, async = vma->obj->mm.rsgt;
+ struct intel_gt *gt;
+
+ if (dma_resv_reserve_fences(vma->obj->base.resv, 1))
+ async = false;
+
+ mutex_lock(&vma->vm->mutex);
+ /*
+* Ensure any asynchronous binding is complete while using
+* async unbind as we will be releasing the vma here.
+*/
+ if (async && i915_active_wait(&vma->active))
+ async = false;
+
+ force_unbind(vma, async);
list_del_init(&vma->vm_link);
vm_ddestroy = vma->vm_ddestroy;
vma->vm_ddestroy = false;
diff --git a/drivers/gpu/drm/i915/i915_vma.h b/drivers/gpu/drm/i915/i915_vma.h
index 737ef310d046..25f15965dab8 100644
--- a/drivers/gpu/drm/i915/i915_vma.h
+++ b/drivers/gpu/drm/i915/i915_vma.h
@@ -272,6 +272,7 @@ void i915_vma_reopen(struct i915_vma *vma);
void i915_vma_destroy_locked(struct i915_vma *vma);
void i915_vma_destroy(struct i915_vma *vma);
+void i915_vma_destroy_async(struct i915_vma *vma);
#define assert_vma_held(vma) dma_resv_assert_held((vma)->obj->base.resv)
--
2.21.0.rc0.32.g243a4c7e27
[PATCH v6 07/20] drm/i915/vm_bind: Add support to handle object evictions
Support eviction by maintaining a list of evicted persistent vmas
for rebinding during next submission. Ensure the list do not
include persistent vmas that are being purged.
v2: Remove unused I915_VMA_PURGED definition.
v3: Properly handle __i915_vma_unbind_async() case.
Reviewed-by: Matthew Auld
Signed-off-by: Niranjana Vishwanathapura
Signed-off-by: Andi Shyti
---
.../drm/i915/gem/i915_gem_vm_bind_object.c| 6
drivers/gpu/drm/i915/gt/intel_gtt.c | 2 ++
drivers/gpu/drm/i915/gt/intel_gtt.h | 4 +++
drivers/gpu/drm/i915/i915_vma.c | 31 +--
drivers/gpu/drm/i915/i915_vma.h | 10 ++
drivers/gpu/drm/i915/i915_vma_types.h | 8 +
6 files changed, 59 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_vm_bind_object.c
b/drivers/gpu/drm/i915/gem/i915_gem_vm_bind_object.c
index 19f29fa76c19..8cc78f954b97 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_vm_bind_object.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_vm_bind_object.c
@@ -86,6 +86,12 @@ static void i915_gem_vm_bind_remove(struct i915_vma *vma,
bool release_obj)
{
lockdep_assert_held(&vma->vm->vm_bind_lock);
+ spin_lock(&vma->vm->vm_rebind_lock);
+ if (!list_empty(&vma->vm_rebind_link))
+ list_del_init(&vma->vm_rebind_link);
+ i915_vma_set_purged(vma);
+ spin_unlock(&vma->vm->vm_rebind_lock);
+
list_del_init(&vma->vm_bind_link);
list_del_init(&vma->non_priv_vm_bind_link);
i915_vm_bind_it_remove(vma, &vma->vm->va);
diff --git a/drivers/gpu/drm/i915/gt/intel_gtt.c
b/drivers/gpu/drm/i915/gt/intel_gtt.c
index 74c3557e5bc4..ebf8fc3a4603 100644
--- a/drivers/gpu/drm/i915/gt/intel_gtt.c
+++ b/drivers/gpu/drm/i915/gt/intel_gtt.c
@@ -290,6 +290,8 @@ void i915_address_space_init(struct i915_address_space *vm,
int subclass)
INIT_LIST_HEAD(&vm->vm_bound_list);
mutex_init(&vm->vm_bind_lock);
INIT_LIST_HEAD(&vm->non_priv_vm_bind_list);
+ INIT_LIST_HEAD(&vm->vm_rebind_list);
+ spin_lock_init(&vm->vm_rebind_lock);
}
void *__px_vaddr(struct drm_i915_gem_object *p)
diff --git a/drivers/gpu/drm/i915/gt/intel_gtt.h
b/drivers/gpu/drm/i915/gt/intel_gtt.h
index 3d0a452567e4..b5a5b68adb32 100644
--- a/drivers/gpu/drm/i915/gt/intel_gtt.h
+++ b/drivers/gpu/drm/i915/gt/intel_gtt.h
@@ -266,6 +266,10 @@ struct i915_address_space {
struct list_head vm_bind_list;
/** @vm_bound_list: List of vm_binding completed */
struct list_head vm_bound_list;
+ /** @vm_rebind_list: list of vmas to be rebinded */
+ struct list_head vm_rebind_list;
+ /** @vm_rebind_lock: protects vm_rebound_list */
+ spinlock_t vm_rebind_lock;
/** @va: tree of persistent vmas */
struct rb_root_cached va;
/** @non_priv_vm_bind_list: list of non-private object mappings */
diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
index 0ffa24bc0954..249697ae1186 100644
--- a/drivers/gpu/drm/i915/i915_vma.c
+++ b/drivers/gpu/drm/i915/i915_vma.c
@@ -241,6 +241,7 @@ vma_create(struct drm_i915_gem_object *obj,
INIT_LIST_HEAD(&vma->vm_bind_link);
INIT_LIST_HEAD(&vma->non_priv_vm_bind_link);
+ INIT_LIST_HEAD(&vma->vm_rebind_link);
return vma;
err_unlock:
@@ -1681,6 +1682,14 @@ static void force_unbind(struct i915_vma *vma)
if (!drm_mm_node_allocated(&vma->node))
return;
+ /*
+* Persistent vma should have been purged by now.
+* If not, issue a warning and purge it.
+*/
+ if (GEM_WARN_ON(i915_vma_is_persistent(vma) &&
+ !i915_vma_is_purged(vma)))
+ i915_vma_set_purged(vma);
+
atomic_and(~I915_VMA_PIN_MASK, &vma->flags);
WARN_ON(__i915_vma_unbind(vma));
GEM_BUG_ON(drm_mm_node_allocated(&vma->node));
@@ -2042,6 +2051,16 @@ int __i915_vma_unbind(struct i915_vma *vma)
__i915_vma_evict(vma, false);
drm_mm_remove_node(&vma->node); /* pairs with i915_vma_release() */
+
+ if (i915_vma_is_persistent(vma)) {
+ spin_lock(&vma->vm->vm_rebind_lock);
+ if (list_empty(&vma->vm_rebind_link) &&
+ !i915_vma_is_purged(vma))
+ list_add_tail(&vma->vm_rebind_link,
+ &vma->vm->vm_rebind_list);
+ spin_unlock(&vma->vm->vm_rebind_lock);
+ }
+
return 0;
}
@@ -2054,8 +2073,7 @@ static struct dma_fence *__i915_vma_unbind_async(struct
i915_vma *vma)
if (!drm_mm_node_allocated(&vma->node))
return NULL;
- if (i915_vma_is_pinned(vma) ||
- &vma->obj->mm.rsgt->table != vma->resource->bi.pages)
+ if (i915_vma_is_pinned(vma))
return ERR_PTR(-EAGAIN);
/*
@@ -2077,6 +2095,15 @@ static struct dma_fence *__i915_vma_unbind_async(struct
i915_vma *vma)
[PATCH v6 08/20] drm/i915/vm_bind: Support persistent vma activeness tracking
Do not use i915_vma activeness tracking for persistent vmas.
As persistent vmas are part of working set for each execbuf
submission on that address space (VM), a persistent vma is
active if the VM active. As vm->root_obj->base.resv will be
updated for each submission on that VM, it correctly
represent whether the VM is active or not.
Add i915_vm_is_active() and i915_vm_sync() functions based
on vm->root_obj->base.resv with DMA_RESV_USAGE_BOOKKEEP
usage. dma-resv fence list will be updated with this usage
during each submission with this VM in the new execbuf3
ioctl path.
Update i915_vma_is_active(), i915_vma_sync() and the
__i915_vma_unbind_async() functions to properly handle
persistent vmas.
v2: Ensure lvalue of dma_resv_wait_timeout() call is long.
Reviewed-by: Andi Shyti
Signed-off-by: Niranjana Vishwanathapura
---
drivers/gpu/drm/i915/i915_gem_gtt.c | 39 +
drivers/gpu/drm/i915/i915_gem_gtt.h | 3 +++
drivers/gpu/drm/i915/i915_vma.c | 28 +
drivers/gpu/drm/i915/i915_vma.h | 25 +-
4 files changed, 83 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c
b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 7bd1861ddbdf..1d8506548d4a 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -25,6 +25,45 @@
#include "i915_trace.h"
#include "i915_vgpu.h"
+/**
+ * i915_vm_sync() - Wait until address space is not in use
+ * @vm: address space
+ *
+ * Waits until all requests using the address space are complete.
+ *
+ * Returns: 0 if success, -ve err code upon failure
+ */
+int i915_vm_sync(struct i915_address_space *vm)
+{
+ long ret;
+
+ /* Wait for all requests under this vm to finish */
+ ret = dma_resv_wait_timeout(vm->root_obj->base.resv,
+ DMA_RESV_USAGE_BOOKKEEP, false,
+ MAX_SCHEDULE_TIMEOUT);
+ if (ret < 0)
+ return ret;
+ else if (ret > 0)
+ return 0;
+ else
+ return -ETIMEDOUT;
+}
+
+/**
+ * i915_vm_is_active() - Check if address space is being used
+ * @vm: address space
+ *
+ * Check if any request using the specified address space is
+ * active.
+ *
+ * Returns: true if address space is active, false otherwise.
+ */
+bool i915_vm_is_active(const struct i915_address_space *vm)
+{
+ return !dma_resv_test_signaled(vm->root_obj->base.resv,
+ DMA_RESV_USAGE_BOOKKEEP);
+}
+
int i915_gem_gtt_prepare_pages(struct drm_i915_gem_object *obj,
struct sg_table *pages)
{
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h
b/drivers/gpu/drm/i915/i915_gem_gtt.h
index 8c2f57eb5dda..a5bbdc59d9df 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
@@ -51,4 +51,7 @@ int i915_gem_gtt_insert(struct i915_address_space *vm,
#define PIN_OFFSET_MASKI915_GTT_PAGE_MASK
+int i915_vm_sync(struct i915_address_space *vm);
+bool i915_vm_is_active(const struct i915_address_space *vm);
+
#endif
diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
index 249697ae1186..04abdb92c2b2 100644
--- a/drivers/gpu/drm/i915/i915_vma.c
+++ b/drivers/gpu/drm/i915/i915_vma.c
@@ -420,6 +420,24 @@ int i915_vma_wait_for_bind(struct i915_vma *vma)
return err;
}
+/**
+ * i915_vma_sync() - Wait for the vma to be idle
+ * @vma: vma to be tested
+ *
+ * Returns 0 on success and error code on failure
+ */
+int i915_vma_sync(struct i915_vma *vma)
+{
+ int ret;
+
+ /* Wait for the asynchronous bindings and pending GPU reads */
+ ret = i915_active_wait(&vma->active);
+ if (ret || !i915_vma_is_persistent(vma) || i915_vma_is_purged(vma))
+ return ret;
+
+ return i915_vm_sync(vma->vm);
+}
+
#if IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM)
static int i915_vma_verify_bind_complete(struct i915_vma *vma)
{
@@ -1882,6 +1900,8 @@ int _i915_vma_move_to_active(struct i915_vma *vma,
int err;
assert_object_held(obj);
+ if (i915_vma_is_persistent(vma))
+ return -EINVAL;
GEM_BUG_ON(!vma->pages);
@@ -2091,6 +2111,14 @@ static struct dma_fence *__i915_vma_unbind_async(struct
i915_vma *vma)
return ERR_PTR(-EBUSY);
}
+ if (i915_vma_is_persistent(vma) &&
+ __i915_sw_fence_await_reservation(&vma->resource->chain,
+ vma->vm->root_obj->base.resv,
+ DMA_RESV_USAGE_BOOKKEEP,
+ i915_fence_timeout(vma->vm->i915),
+ GFP_NOWAIT | __GFP_NOWARN) < 0)
+ return ERR_PTR(-EBUSY);
+
fence = __i915_vma_evict(vma, true);
drm_mm_remove_node(&vma->node); /* pairs with i915_vma_release() */
diff --git a/drivers
[PATCH v6 02/20] drm/i915/vm_bind: Add __i915_sw_fence_await_reservation()
Add function __i915_sw_fence_await_reservation() for
asynchronous wait on a dma-resv object with specified
dma_resv_usage. This is required for async vma unbind
with vm_bind.
Reviewed-by: Matthew Auld
Signed-off-by: Niranjana Vishwanathapura
---
drivers/gpu/drm/i915/i915_sw_fence.c | 28 +---
drivers/gpu/drm/i915/i915_sw_fence.h | 23 +--
2 files changed, 38 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_sw_fence.c
b/drivers/gpu/drm/i915/i915_sw_fence.c
index cc2a8821d22a..ae06d35db056 100644
--- a/drivers/gpu/drm/i915/i915_sw_fence.c
+++ b/drivers/gpu/drm/i915/i915_sw_fence.c
@@ -7,7 +7,6 @@
#include
#include
#include
-#include
#include "i915_sw_fence.h"
#include "i915_selftest.h"
@@ -569,11 +568,26 @@ int __i915_sw_fence_await_dma_fence(struct i915_sw_fence
*fence,
return ret;
}
-int i915_sw_fence_await_reservation(struct i915_sw_fence *fence,
- struct dma_resv *resv,
- bool write,
- unsigned long timeout,
- gfp_t gfp)
+/**
+ * __i915_sw_fence_await_reservation() - Setup a fence to wait on a dma-resv
+ * object with specified usage.
+ * @fence: the fence that needs to wait
+ * @resv: dma-resv object
+ * @usage: dma_resv_usage (See enum dma_resv_usage)
+ * @timeout: how long to wait in jiffies
+ * @gfp: allocation mode
+ *
+ * Setup the @fence to asynchronously wait on dma-resv object @resv for
+ * @usage to complete before signaling.
+ *
+ * Returns 0 if there is nothing to wait on, -ve error code upon error
+ * and >0 upon successfully setting up the wait.
+ */
+int __i915_sw_fence_await_reservation(struct i915_sw_fence *fence,
+ struct dma_resv *resv,
+ enum dma_resv_usage usage,
+ unsigned long timeout,
+ gfp_t gfp)
{
struct dma_resv_iter cursor;
struct dma_fence *f;
@@ -582,7 +596,7 @@ int i915_sw_fence_await_reservation(struct i915_sw_fence
*fence,
debug_fence_assert(fence);
might_sleep_if(gfpflags_allow_blocking(gfp));
- dma_resv_iter_begin(&cursor, resv, dma_resv_usage_rw(write));
+ dma_resv_iter_begin(&cursor, resv, usage);
dma_resv_for_each_fence_unlocked(&cursor, f) {
pending = i915_sw_fence_await_dma_fence(fence, f, timeout,
gfp);
diff --git a/drivers/gpu/drm/i915/i915_sw_fence.h
b/drivers/gpu/drm/i915/i915_sw_fence.h
index f752bfc7c6e1..9c4859dc4c0d 100644
--- a/drivers/gpu/drm/i915/i915_sw_fence.h
+++ b/drivers/gpu/drm/i915/i915_sw_fence.h
@@ -10,13 +10,13 @@
#define _I915_SW_FENCE_H_
#include
+#include
#include
#include
#include /* for NOTIFY_DONE */
#include
struct completion;
-struct dma_resv;
struct i915_sw_fence;
enum i915_sw_fence_notify {
@@ -89,11 +89,22 @@ int i915_sw_fence_await_dma_fence(struct i915_sw_fence
*fence,
unsigned long timeout,
gfp_t gfp);
-int i915_sw_fence_await_reservation(struct i915_sw_fence *fence,
- struct dma_resv *resv,
- bool write,
- unsigned long timeout,
- gfp_t gfp);
+int __i915_sw_fence_await_reservation(struct i915_sw_fence *fence,
+ struct dma_resv *resv,
+ enum dma_resv_usage usage,
+ unsigned long timeout,
+ gfp_t gfp);
+
+static inline int i915_sw_fence_await_reservation(struct i915_sw_fence *fence,
+ struct dma_resv *resv,
+ bool write,
+ unsigned long timeout,
+ gfp_t gfp)
+{
+ return __i915_sw_fence_await_reservation(fence, resv,
+dma_resv_usage_rw(write),
+timeout, gfp);
+}
bool i915_sw_fence_await(struct i915_sw_fence *fence);
void i915_sw_fence_complete(struct i915_sw_fence *fence);
--
2.21.0.rc0.32.g243a4c7e27
[PATCH v6 16/20] drm/i915/vm_bind: userptr dma-resv changes
For persistent (vm_bind) vmas of userptr BOs, handle the user
page pinning by using the i915_gem_object_userptr_submit_init()
/done() functions
v2: Do not double add vma to vm->userptr_invalidated_list
v3: Initialize vma->userptr_invalidated_link
Reviewed-by: Matthew Auld
Signed-off-by: Niranjana Vishwanathapura
Signed-off-by: Andi Shyti
---
.../gpu/drm/i915/gem/i915_gem_execbuffer3.c | 84 ++-
drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 19 +
.../drm/i915/gem/i915_gem_vm_bind_object.c| 15
drivers/gpu/drm/i915/gt/intel_gtt.c | 2 +
drivers/gpu/drm/i915/gt/intel_gtt.h | 4 +
drivers/gpu/drm/i915/i915_vma.c | 1 +
drivers/gpu/drm/i915/i915_vma_types.h | 2 +
7 files changed, 125 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer3.c
b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer3.c
index d91c2e96cd0f..895d4f0a2647 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer3.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer3.c
@@ -20,6 +20,7 @@
#include "i915_gem_vm_bind.h"
#include "i915_trace.h"
+#define __EXEC3_USERPTR_USED BIT_ULL(34)
#define __EXEC3_HAS_PINBIT_ULL(33)
#define __EXEC3_ENGINE_PINNED BIT_ULL(32)
#define __EXEC3_INTERNAL_FLAGS (~0ull << 32)
@@ -144,7 +145,22 @@ static void eb_scoop_unbound_vma_all(struct
i915_address_space *vm)
{
struct i915_vma *vma, *vn;
- /**
+#ifdef CONFIG_MMU_NOTIFIER
+ /*
+* Move all invalidated userptr vmas back into vm_bind_list so that
+* they are looked up and revalidated.
+*/
+ spin_lock(&vm->userptr_invalidated_lock);
+ list_for_each_entry_safe(vma, vn, &vm->userptr_invalidated_list,
+userptr_invalidated_link) {
+ list_del_init(&vma->userptr_invalidated_link);
+ if (!list_empty(&vma->vm_bind_link))
+ list_move_tail(&vma->vm_bind_link, &vm->vm_bind_list);
+ }
+ spin_unlock(&vm->userptr_invalidated_lock);
+#endif
+
+ /*
* Move all unbound vmas back into vm_bind_list so that they are
* revalidated.
*/
@@ -157,10 +173,47 @@ static void eb_scoop_unbound_vma_all(struct
i915_address_space *vm)
spin_unlock(&vm->vm_rebind_lock);
}
+static int eb_lookup_persistent_userptr_vmas(struct i915_execbuffer *eb)
+{
+ struct i915_address_space *vm = eb->context->vm;
+ struct i915_vma *last_vma = NULL;
+ struct i915_vma *vma;
+ int err;
+
+ lockdep_assert_held(&vm->vm_bind_lock);
+
+ list_for_each_entry(vma, &vm->vm_bind_list, vm_bind_link) {
+ if (!i915_gem_object_is_userptr(vma->obj))
+ continue;
+
+ err = i915_gem_object_userptr_submit_init(vma->obj);
+ if (err)
+ return err;
+
+ /*
+* The above submit_init() call does the object unbind and
+* hence adds vma into vm_rebind_list. Remove it from that
+* list as it is already scooped for revalidation.
+*/
+ spin_lock(&vm->vm_rebind_lock);
+ if (!list_empty(&vma->vm_rebind_link))
+ list_del_init(&vma->vm_rebind_link);
+ spin_unlock(&vm->vm_rebind_lock);
+
+ last_vma = vma;
+ }
+
+ if (last_vma)
+ eb->args->flags |= __EXEC3_USERPTR_USED;
+
+ return 0;
+}
+
static int eb_lookup_vma_all(struct i915_execbuffer *eb)
{
struct i915_vma *vma;
unsigned int i;
+ int err = 0;
for (i = 0; i < eb->num_batches; i++) {
vma = eb_find_vma(eb->context->vm, eb->batch_addresses[i]);
@@ -172,6 +225,10 @@ static int eb_lookup_vma_all(struct i915_execbuffer *eb)
eb_scoop_unbound_vma_all(eb->context->vm);
+ err = eb_lookup_persistent_userptr_vmas(eb);
+ if (err)
+ return err;
+
return 0;
}
@@ -344,6 +401,29 @@ static int eb_move_to_gpu(struct i915_execbuffer *eb)
}
}
+#ifdef CONFIG_MMU_NOTIFIER
+ /* Check for further userptr invalidations */
+ spin_lock(&vm->userptr_invalidated_lock);
+ if (!list_empty(&vm->userptr_invalidated_list))
+ err = -EAGAIN;
+ spin_unlock(&vm->userptr_invalidated_lock);
+
+ if (!err && (eb->args->flags & __EXEC3_USERPTR_USED)) {
+ read_lock(&eb->i915->mm.notifier_lock);
+ list_for_each_entry(vma, &vm->vm_bind_list, vm_bind_link) {
+ if (!i915_gem_object_is_userptr(vma->obj))
+ continue;
+
+ err = i915_gem_object_userptr_submit_done(vma->obj);
+ if (err)
+ break;
+ }
+ read_unlock(&eb->i915->mm.notifier_lock
[PATCH v6 09/20] drm/i915/vm_bind: Add out fence support
Add support for handling out fence for vm_bind call.
v2: Reset vma->vm_bind_fence.syncobj to NULL at the end
of vm_bind call.
v3: Remove vm_unbind out fence uapi which is not supported yet.
v4: Return error if I915_TIMELINE_FENCE_WAIT fence flag is set.
Wait for bind to complete iff I915_TIMELINE_FENCE_SIGNAL is
not specified.
v5: Ensure __I915_TIMELINE_FENCE_UNKNOWN_FLAGS are not set.
Reviewed-by: Matthew Auld
Signed-off-by: Niranjana Vishwanathapura
Signed-off-by: Andi Shyti
---
drivers/gpu/drm/i915/gem/i915_gem_vm_bind.h | 4 +
.../drm/i915/gem/i915_gem_vm_bind_object.c| 95 +++
drivers/gpu/drm/i915/i915_vma.c | 7 +-
drivers/gpu/drm/i915/i915_vma_types.h | 7 ++
include/uapi/drm/i915_drm.h | 49 +-
5 files changed, 159 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_vm_bind.h
b/drivers/gpu/drm/i915/gem/i915_gem_vm_bind.h
index 36262a6357b5..b70e900e35ab 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_vm_bind.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_vm_bind.h
@@ -8,6 +8,7 @@
#include
+struct dma_fence;
struct drm_device;
struct drm_file;
struct i915_address_space;
@@ -23,4 +24,7 @@ int i915_gem_vm_unbind_ioctl(struct drm_device *dev, void
*data,
void i915_gem_vm_unbind_all(struct i915_address_space *vm);
+void i915_vm_bind_signal_fence(struct i915_vma *vma,
+ struct dma_fence * const fence);
+
#endif /* __I915_GEM_VM_BIND_H */
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_vm_bind_object.c
b/drivers/gpu/drm/i915/gem/i915_gem_vm_bind_object.c
index 8cc78f954b97..6396fd1dc520 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_vm_bind_object.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_vm_bind_object.c
@@ -7,6 +7,8 @@
#include
+#include
+
#include "gem/i915_gem_context.h"
#include "gem/i915_gem_vm_bind.h"
@@ -101,6 +103,77 @@ static void i915_gem_vm_bind_remove(struct i915_vma *vma,
bool release_obj)
i915_gem_object_put(vma->obj);
}
+static int i915_vm_bind_add_fence(struct drm_file *file, struct i915_vma *vma,
+ u32 handle, u64 point)
+{
+ struct drm_syncobj *syncobj;
+
+ syncobj = drm_syncobj_find(file, handle);
+ if (!syncobj) {
+ drm_dbg(&vma->vm->i915->drm,
+ "Invalid syncobj handle provided\n");
+ return -ENOENT;
+ }
+
+ /*
+* For timeline syncobjs we need to preallocate chains for
+* later signaling.
+*/
+ if (point) {
+ vma->vm_bind_fence.chain_fence = dma_fence_chain_alloc();
+ if (!vma->vm_bind_fence.chain_fence) {
+ drm_syncobj_put(syncobj);
+ return -ENOMEM;
+ }
+ } else {
+ vma->vm_bind_fence.chain_fence = NULL;
+ }
+ vma->vm_bind_fence.syncobj = syncobj;
+ vma->vm_bind_fence.value = point;
+
+ return 0;
+}
+
+static void i915_vm_bind_put_fence(struct i915_vma *vma)
+{
+ if (!vma->vm_bind_fence.syncobj)
+ return;
+
+ drm_syncobj_put(vma->vm_bind_fence.syncobj);
+ dma_fence_chain_free(vma->vm_bind_fence.chain_fence);
+ vma->vm_bind_fence.syncobj = NULL;
+}
+
+/**
+ * i915_vm_bind_signal_fence() - Add fence to vm_bind syncobj
+ * @vma: vma mapping requiring signaling
+ * @fence: fence to be added
+ *
+ * Associate specified @fence with the @vma's syncobj to be
+ * signaled after the @fence work completes.
+ */
+void i915_vm_bind_signal_fence(struct i915_vma *vma,
+ struct dma_fence * const fence)
+{
+ struct drm_syncobj *syncobj = vma->vm_bind_fence.syncobj;
+
+ if (!syncobj)
+ return;
+
+ if (vma->vm_bind_fence.chain_fence) {
+ drm_syncobj_add_point(syncobj,
+ vma->vm_bind_fence.chain_fence,
+ fence, vma->vm_bind_fence.value);
+ /*
+* The chain's ownership is transferred to the
+* timeline.
+*/
+ vma->vm_bind_fence.chain_fence = NULL;
+ } else {
+ drm_syncobj_replace_fence(syncobj, fence);
+ }
+}
+
static int i915_gem_vm_unbind_vma(struct i915_address_space *vm,
struct drm_i915_gem_vm_unbind *va)
{
@@ -206,6 +279,11 @@ static int i915_gem_vm_bind_obj(struct i915_address_space
*vm,
if (!va->length || !IS_ALIGNED(va->start, I915_GTT_PAGE_SIZE))
ret = -EINVAL;
+ /* In fences are not supported */
+ if ((va->fence.flags & I915_TIMELINE_FENCE_WAIT) ||
+ (va->fence.flags & __I915_TIMELINE_FENCE_UNKNOWN_FLAGS))
+ ret = -EINVAL;
+
obj = i915_gem_object_lookup(file, va->handle);
if (!obj)
return -ENOENT;
@@ -238,6 +316,13 @@ static int i91
[PATCH v6 15/20] drm/i915/vm_bind: Handle persistent vmas in execbuf3
Handle persistent (VM_BIND) mappings during the request submission
in the execbuf3 path.
v2: Ensure requests wait for bindings to complete.
v3: Remove short term pinning with PIN_VALIDATE flag.
Individualize fences before adding to dma_resv obj.
v4: Fix bind completion check, use PIN_NOEVICT,
use proper lock while checking if vm_rebind_list is empty.
Reviewed-by: Matthew Auld
Signed-off-by: Niranjana Vishwanathapura
Signed-off-by: Andi Shyti
---
.../gpu/drm/i915/gem/i915_gem_execbuffer3.c | 215 +-
1 file changed, 214 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer3.c
b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer3.c
index 64251dc4cf91..d91c2e96cd0f 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer3.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer3.c
@@ -3,6 +3,7 @@
* Copyright © 2022 Intel Corporation
*/
+#include
#include
#include
@@ -19,6 +20,7 @@
#include "i915_gem_vm_bind.h"
#include "i915_trace.h"
+#define __EXEC3_HAS_PINBIT_ULL(33)
#define __EXEC3_ENGINE_PINNED BIT_ULL(32)
#define __EXEC3_INTERNAL_FLAGS (~0ull << 32)
@@ -42,7 +44,9 @@
* execlist. Hence, no support for implicit sync.
*
* The new execbuf3 ioctl only works in VM_BIND mode and the VM_BIND mode only
- * works with execbuf3 ioctl for submission.
+ * works with execbuf3 ioctl for submission. All BOs mapped on that VM (through
+ * VM_BIND call) at the time of execbuf3 call are deemed required for that
+ * submission.
*
* The execbuf3 ioctl directly specifies the batch addresses instead of as
* object handles as in execbuf2 ioctl. The execbuf3 ioctl will also not
@@ -58,6 +62,13 @@
* So, a lot of code supporting execbuf2 ioctl, like relocations, VA evictions,
* vma lookup table, implicit sync, vma active reference tracking etc., are not
* applicable for execbuf3 ioctl.
+ *
+ * During each execbuf submission, request fence is added to all VM_BIND mapped
+ * objects with DMA_RESV_USAGE_BOOKKEEP. The DMA_RESV_USAGE_BOOKKEEP usage will
+ * prevent over sync (See enum dma_resv_usage). Note that DRM_I915_GEM_WAIT and
+ * DRM_I915_GEM_BUSY ioctls do not check for DMA_RESV_USAGE_BOOKKEEP usage and
+ * hence should not be used for end of batch check. Instead, the execbuf3
+ * timeline out fence should be used for end of batch check.
*/
/**
@@ -129,6 +140,23 @@ eb_find_vma(struct i915_address_space *vm, u64 addr)
return i915_gem_vm_bind_lookup_vma(vm, va);
}
+static void eb_scoop_unbound_vma_all(struct i915_address_space *vm)
+{
+ struct i915_vma *vma, *vn;
+
+ /**
+* Move all unbound vmas back into vm_bind_list so that they are
+* revalidated.
+*/
+ spin_lock(&vm->vm_rebind_lock);
+ list_for_each_entry_safe(vma, vn, &vm->vm_rebind_list, vm_rebind_link) {
+ list_del_init(&vma->vm_rebind_link);
+ if (!list_empty(&vma->vm_bind_link))
+ list_move_tail(&vma->vm_bind_link, &vm->vm_bind_list);
+ }
+ spin_unlock(&vm->vm_rebind_lock);
+}
+
static int eb_lookup_vma_all(struct i915_execbuffer *eb)
{
struct i915_vma *vma;
@@ -142,14 +170,108 @@ static int eb_lookup_vma_all(struct i915_execbuffer *eb)
eb->batches[i] = vma;
}
+ eb_scoop_unbound_vma_all(eb->context->vm);
+
+ return 0;
+}
+
+static int eb_lock_vma_all(struct i915_execbuffer *eb)
+{
+ struct i915_address_space *vm = eb->context->vm;
+ struct i915_vma *vma;
+ int err;
+
+ err = i915_gem_object_lock(eb->context->vm->root_obj, &eb->ww);
+ if (err)
+ return err;
+
+ list_for_each_entry(vma, &vm->non_priv_vm_bind_list,
+ non_priv_vm_bind_link) {
+ err = i915_gem_object_lock(vma->obj, &eb->ww);
+ if (err)
+ return err;
+ }
+
return 0;
}
+static void eb_release_persistent_vma_all(struct i915_execbuffer *eb)
+{
+ struct i915_address_space *vm = eb->context->vm;
+ struct i915_vma *vma, *vn;
+
+ lockdep_assert_held(&vm->vm_bind_lock);
+
+ if (!(eb->args->flags & __EXEC3_HAS_PIN))
+ return;
+
+ assert_object_held(vm->root_obj);
+
+ list_for_each_entry_safe(vma, vn, &vm->vm_bind_list, vm_bind_link)
+ if (!i915_vma_verify_bind_complete(vma))
+ list_move_tail(&vma->vm_bind_link, &vm->vm_bound_list);
+
+ eb->args->flags &= ~__EXEC3_HAS_PIN;
+}
+
static void eb_release_vma_all(struct i915_execbuffer *eb)
{
+ eb_release_persistent_vma_all(eb);
eb_unpin_engine(eb);
}
+static int eb_reserve_fence_for_persistent_vma_all(struct i915_execbuffer *eb)
+{
+ struct i915_address_space *vm = eb->context->vm;
+ u64 num_fences = 1;
+ struct i915_vma *vma;
+ int ret;
+
+ /* Reserve enough slots to accommodate composite
[PATCH v6 18/20] drm/i915/vm_bind: Add uapi for user to enable vm_bind_mode
Add getparam support for VM_BIND capability version.
Add VM creation time flag to enable vm_bind_mode for the VM.
v2: update kernel-doc
v3: create vm->root_obj only upon I915_VM_CREATE_FLAGS_USE_VM_BIND
v4: replace vm->vm_bind_mode check with i915_gem_vm_is_vm_bind_mode()
Reviewed-by: Matthew Auld
Signed-off-by: Niranjana Vishwanathapura
Signed-off-by: Andi Shyti
---
drivers/gpu/drm/i915/gem/i915_gem_context.c | 25 ++--
drivers/gpu/drm/i915/gem/i915_gem_context.h | 3 +--
drivers/gpu/drm/i915/gt/intel_gtt.c | 2 ++
drivers/gpu/drm/i915/i915_drv.h | 2 ++
drivers/gpu/drm/i915/i915_getparam.c| 3 +++
include/uapi/drm/i915_drm.h | 26 -
6 files changed, 56 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c
b/drivers/gpu/drm/i915/gem/i915_gem_context.c
index 899079d602bc..56b60413bef9 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
@@ -1809,9 +1809,13 @@ int i915_gem_vm_create_ioctl(struct drm_device *dev,
void *data,
if (!HAS_FULL_PPGTT(i915))
return -ENODEV;
- if (args->flags)
+ if (args->flags & I915_VM_CREATE_FLAGS_UNKNOWN)
return -EINVAL;
+ if ((args->flags & I915_VM_CREATE_FLAGS_USE_VM_BIND) &&
+ !HAS_VM_BIND(i915))
+ return -EOPNOTSUPP;
+
ppgtt = i915_ppgtt_create(to_gt(i915), 0);
if (IS_ERR(ppgtt))
return PTR_ERR(ppgtt);
@@ -1824,15 +1828,32 @@ int i915_gem_vm_create_ioctl(struct drm_device *dev,
void *data,
goto err_put;
}
+ if (args->flags & I915_VM_CREATE_FLAGS_USE_VM_BIND) {
+ struct drm_i915_gem_object *obj;
+
+ obj = i915_gem_object_create_internal(i915, PAGE_SIZE);
+ if (IS_ERR(obj)) {
+ err = PTR_ERR(obj);
+ goto err_put;
+ }
+
+ ppgtt->vm.root_obj = obj;
+ }
+
err = xa_alloc(&file_priv->vm_xa, &id, &ppgtt->vm,
xa_limit_32b, GFP_KERNEL);
if (err)
- goto err_put;
+ goto err_root_obj_put;
GEM_BUG_ON(id == 0); /* reserved for invalid/unassigned ppgtt */
args->vm_id = id;
return 0;
+err_root_obj_put:
+ if (ppgtt->vm.root_obj) {
+ i915_gem_object_put(ppgtt->vm.root_obj);
+ ppgtt->vm.root_obj = NULL;
+ }
err_put:
i915_vm_put(&ppgtt->vm);
return err;
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.h
b/drivers/gpu/drm/i915/gem/i915_gem_context.h
index e8b41aa8f8c4..b53aef2853cb 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.h
@@ -150,8 +150,7 @@ int i915_gem_context_reset_stats_ioctl(struct drm_device
*dev, void *data,
*/
static inline bool i915_gem_vm_is_vm_bind_mode(struct i915_address_space *vm)
{
- /* No support to enable vm_bind mode yet */
- return false;
+ return !!vm->root_obj;
}
struct i915_address_space *
diff --git a/drivers/gpu/drm/i915/gt/intel_gtt.c
b/drivers/gpu/drm/i915/gt/intel_gtt.c
index 50648ab9214a..ae66fdd4bce9 100644
--- a/drivers/gpu/drm/i915/gt/intel_gtt.c
+++ b/drivers/gpu/drm/i915/gt/intel_gtt.c
@@ -178,6 +178,8 @@ int i915_vm_lock_objects(struct i915_address_space *vm,
void i915_address_space_fini(struct i915_address_space *vm)
{
drm_mm_takedown(&vm->mm);
+ if (vm->root_obj)
+ i915_gem_object_put(vm->root_obj);
GEM_BUG_ON(!RB_EMPTY_ROOT(&vm->va.rb_root));
mutex_destroy(&vm->vm_bind_lock);
}
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 05b3300cc4ed..a34d9a7dcd1c 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -978,6 +978,8 @@ IS_SUBPLATFORM(const struct drm_i915_private *i915,
#define HAS_LMEMBAR_SMEM_STOLEN(i915) (!HAS_LMEM(i915) && \
GRAPHICS_VER_FULL(i915) >= IP_VER(12,
70))
+#define HAS_VM_BIND(i915) (GRAPHICS_VER(i915) >= 12)
+
/* intel_device_info.c */
static inline struct intel_device_info *
mkwrite_device_info(struct drm_i915_private *dev_priv)
diff --git a/drivers/gpu/drm/i915/i915_getparam.c
b/drivers/gpu/drm/i915/i915_getparam.c
index 3047e80e1163..9f700c0c0fb0 100644
--- a/drivers/gpu/drm/i915/i915_getparam.c
+++ b/drivers/gpu/drm/i915/i915_getparam.c
@@ -178,6 +178,9 @@ int i915_getparam_ioctl(struct drm_device *dev, void *data,
case I915_PARAM_OA_TIMESTAMP_FREQUENCY:
value = i915_perf_oa_timestamp_frequency(i915);
break;
+ case I915_PARAM_VM_BIND_VERSION:
+ value = HAS_VM_BIND(i915);
+ break;
default:
DRM_DEBUG("Unknown parameter %d\n", param->param);
return -EINVAL;
diff --git a/i
[PATCH v6 10/20] drm/i915/vm_bind: Abstract out common execbuf functions
The new execbuf3 ioctl path and the legacy execbuf ioctl
paths have many common functionalities.
Abstract out the common execbuf functionalities into a
separate file where possible, thus allowing code sharing.
Reviewed-by: Andi Shyti
Reviewed-by: Matthew Auld
Signed-off-by: Niranjana Vishwanathapura
---
drivers/gpu/drm/i915/Makefile | 1 +
.../drm/i915/gem/i915_gem_execbuffer_common.c | 666 ++
.../drm/i915/gem/i915_gem_execbuffer_common.h | 74 ++
3 files changed, 741 insertions(+)
create mode 100644 drivers/gpu/drm/i915/gem/i915_gem_execbuffer_common.c
create mode 100644 drivers/gpu/drm/i915/gem/i915_gem_execbuffer_common.h
diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index b731f3ac80da..35636c6bf856 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -148,6 +148,7 @@ gem-y += \
gem/i915_gem_create.o \
gem/i915_gem_dmabuf.o \
gem/i915_gem_domain.o \
+ gem/i915_gem_execbuffer_common.o \
gem/i915_gem_execbuffer.o \
gem/i915_gem_internal.o \
gem/i915_gem_object.o \
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer_common.c
b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer_common.c
new file mode 100644
index ..4d1c9ce154b5
--- /dev/null
+++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer_common.c
@@ -0,0 +1,666 @@
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright © 2022 Intel Corporation
+ */
+
+#include
+
+#include
+
+#include "gt/intel_context.h"
+#include "gt/intel_gt.h"
+#include "gt/intel_gt_pm.h"
+#include "gt/intel_ring.h"
+
+#include "i915_gem_execbuffer_common.h"
+
+#define __EXEC_COMMON_FENCE_WAIT BIT(0)
+#define __EXEC_COMMON_FENCE_SIGNAL BIT(1)
+
+static struct i915_request *eb_throttle(struct intel_context *ce)
+{
+ struct intel_ring *ring = ce->ring;
+ struct intel_timeline *tl = ce->timeline;
+ struct i915_request *rq;
+
+ /*
+* Completely unscientific finger-in-the-air estimates for suitable
+* maximum user request size (to avoid blocking) and then backoff.
+*/
+ if (intel_ring_update_space(ring) >= PAGE_SIZE)
+ return NULL;
+
+ /*
+* Find a request that after waiting upon, there will be at least half
+* the ring available. The hysteresis allows us to compete for the
+* shared ring and should mean that we sleep less often prior to
+* claiming our resources, but not so long that the ring completely
+* drains before we can submit our next request.
+*/
+ list_for_each_entry(rq, &tl->requests, link) {
+ if (rq->ring != ring)
+ continue;
+
+ if (__intel_ring_space(rq->postfix,
+ ring->emit, ring->size) > ring->size / 2)
+ break;
+ }
+ if (&rq->link == &tl->requests)
+ return NULL; /* weird, we will check again later for real */
+
+ return i915_request_get(rq);
+}
+
+static int eb_pin_timeline(struct intel_context *ce, bool throttle,
+ bool nonblock)
+{
+ struct intel_timeline *tl;
+ struct i915_request *rq = NULL;
+
+ /*
+* Take a local wakeref for preparing to dispatch the execbuf as
+* we expect to access the hardware fairly frequently in the
+* process, and require the engine to be kept awake between accesses.
+* Upon dispatch, we acquire another prolonged wakeref that we hold
+* until the timeline is idle, which in turn releases the wakeref
+* taken on the engine, and the parent device.
+*/
+ tl = intel_context_timeline_lock(ce);
+ if (IS_ERR(tl))
+ return PTR_ERR(tl);
+
+ intel_context_enter(ce);
+ if (throttle)
+ rq = eb_throttle(ce);
+ intel_context_timeline_unlock(tl);
+
+ if (rq) {
+ long timeout = nonblock ? 0 : MAX_SCHEDULE_TIMEOUT;
+
+ if (i915_request_wait(rq, I915_WAIT_INTERRUPTIBLE,
+ timeout) < 0) {
+ i915_request_put(rq);
+
+ /*
+* Error path, cannot use intel_context_timeline_lock as
+* that is user interruptable and this clean up step
+* must be done.
+*/
+ mutex_lock(&ce->timeline->mutex);
+ intel_context_exit(ce);
+ mutex_unlock(&ce->timeline->mutex);
+
+ if (nonblock)
+ return -EWOULDBLOCK;
+ else
+ return -EINTR;
+ }
+ i915_request_put(rq);
+ }
+
+ return 0;
+}
+
+/**
+ * i915_eb_pin_engine() - Pin the engine
+ * @ce: the context
+ * @ww: optional locking contex
[PATCH v6 17/20] drm/i915/vm_bind: Limit vm_bind mode to non-recoverable contexts
Only support vm_bind mode with non-recoverable contexts.
With new vm_bind mode with eb3 submission path, we need not
support older recoverable contexts.
Reviewed-by: Matthew Auld
Signed-off-by: Niranjana Vishwanathapura
---
drivers/gpu/drm/i915/gem/i915_gem_context.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c
b/drivers/gpu/drm/i915/gem/i915_gem_context.c
index 1630a52f387d..899079d602bc 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
@@ -1617,6 +1617,12 @@ i915_gem_create_context(struct drm_i915_private *i915,
INIT_LIST_HEAD(&ctx->stale.engines);
if (pc->vm) {
+ /* Only non-recoverable contexts are allowed in vm_bind mode */
+ if (i915_gem_vm_is_vm_bind_mode(pc->vm) &&
+ (pc->user_flags & BIT(UCONTEXT_RECOVERABLE))) {
+ err = -EINVAL;
+ goto err_ctx;
+ }
vm = i915_vm_get(pc->vm);
} else if (HAS_FULL_PPGTT(i915)) {
struct i915_ppgtt *ppgtt;
--
2.21.0.rc0.32.g243a4c7e27
[PATCH v6 04/20] drm/i915/vm_bind: Add support to create persistent vma
Add i915_vma_instance_persistent() to create persistent vmas.
Persistent vmas will use i915_gtt_view to support partial binding.
vma_lookup is tied to segment of the object instead of section
of VA space. Hence, it do not support aliasing. ie., multiple
mappings (at different VA) point to the same gtt_view of object.
Skip vma_lookup for persistent vmas to support aliasing.
v2: Remove unused I915_VMA_PERSISTENT definition,
update validity check in i915_vma_compare(),
remove unwanted is_persistent check in release_references().
Reviewed-by: Matthew Auld
Signed-off-by: Niranjana Vishwanathapura
Signed-off-by: Andi Shyti
---
drivers/gpu/drm/i915/i915_vma.c | 36 +--
drivers/gpu/drm/i915/i915_vma.h | 17 -
drivers/gpu/drm/i915/i915_vma_types.h | 6 +
3 files changed, 56 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
index c39488eb9eeb..529d97318f00 100644
--- a/drivers/gpu/drm/i915/i915_vma.c
+++ b/drivers/gpu/drm/i915/i915_vma.c
@@ -109,7 +109,8 @@ static void __i915_vma_retire(struct i915_active *ref)
static struct i915_vma *
vma_create(struct drm_i915_gem_object *obj,
struct i915_address_space *vm,
- const struct i915_gtt_view *view)
+ const struct i915_gtt_view *view,
+ bool skip_lookup_cache)
{
struct i915_vma *pos = ERR_PTR(-E2BIG);
struct i915_vma *vma;
@@ -196,6 +197,9 @@ vma_create(struct drm_i915_gem_object *obj,
__set_bit(I915_VMA_GGTT_BIT, __i915_vma_flags(vma));
}
+ if (skip_lookup_cache)
+ goto skip_rb_insert;
+
rb = NULL;
p = &obj->vma.tree.rb_node;
while (*p) {
@@ -220,6 +224,7 @@ vma_create(struct drm_i915_gem_object *obj,
rb_link_node(&vma->obj_node, rb, p);
rb_insert_color(&vma->obj_node, &obj->vma.tree);
+skip_rb_insert:
if (i915_vma_is_ggtt(vma))
/*
* We put the GGTT vma at the start of the vma-list, followed
@@ -299,7 +304,34 @@ i915_vma_instance(struct drm_i915_gem_object *obj,
/* vma_create() will resolve the race if another creates the vma */
if (unlikely(!vma))
- vma = vma_create(obj, vm, view);
+ vma = vma_create(obj, vm, view, false);
+
+ GEM_BUG_ON(!IS_ERR(vma) && i915_vma_compare(vma, vm, view));
+ return vma;
+}
+
+/**
+ * i915_vma_create_persistent - create a persistent VMA
+ * @obj: parent &struct drm_i915_gem_object to be mapped
+ * @vm: address space in which the mapping is located
+ * @view: additional mapping requirements
+ *
+ * Creates a persistent vma.
+ *
+ * Returns the vma, or an error pointer.
+ */
+struct i915_vma *
+i915_vma_create_persistent(struct drm_i915_gem_object *obj,
+ struct i915_address_space *vm,
+ const struct i915_gtt_view *view)
+{
+ struct i915_vma *vma;
+
+ GEM_BUG_ON(!kref_read(&vm->ref));
+
+ vma = vma_create(obj, vm, view, true);
+ if (!IS_ERR(vma))
+ i915_vma_set_persistent(vma);
GEM_BUG_ON(!IS_ERR(vma) && i915_vma_compare(vma, vm, view));
return vma;
diff --git a/drivers/gpu/drm/i915/i915_vma.h b/drivers/gpu/drm/i915/i915_vma.h
index aecd9c64486b..c5378ec2f70a 100644
--- a/drivers/gpu/drm/i915/i915_vma.h
+++ b/drivers/gpu/drm/i915/i915_vma.h
@@ -44,6 +44,10 @@ struct i915_vma *
i915_vma_instance(struct drm_i915_gem_object *obj,
struct i915_address_space *vm,
const struct i915_gtt_view *view);
+struct i915_vma *
+i915_vma_create_persistent(struct drm_i915_gem_object *obj,
+ struct i915_address_space *vm,
+ const struct i915_gtt_view *view);
void i915_vma_unpin_and_release(struct i915_vma **p_vma, unsigned int flags);
#define I915_VMA_RELEASE_MAP BIT(0)
@@ -138,6 +142,16 @@ static inline u32 i915_ggtt_pin_bias(struct i915_vma *vma)
return i915_vm_to_ggtt(vma->vm)->pin_bias;
}
+static inline bool i915_vma_is_persistent(const struct i915_vma *vma)
+{
+ return test_bit(I915_VMA_PERSISTENT_BIT, __i915_vma_flags(vma));
+}
+
+static inline void i915_vma_set_persistent(struct i915_vma *vma)
+{
+ set_bit(I915_VMA_PERSISTENT_BIT, __i915_vma_flags(vma));
+}
+
static inline struct i915_vma *i915_vma_get(struct i915_vma *vma)
{
i915_gem_object_get(vma->obj);
@@ -164,7 +178,8 @@ i915_vma_compare(struct i915_vma *vma,
{
ptrdiff_t cmp;
- GEM_BUG_ON(view && !i915_is_ggtt_or_dpt(vm));
+ GEM_BUG_ON(view && !(i915_is_ggtt_or_dpt(vm) ||
+i915_vma_is_persistent(vma)));
cmp = ptrdiff(vma->vm, vm);
if (cmp)
diff --git a/drivers/gpu/drm/i915/i915_vma_types.h
b/drivers/gpu/drm/i915/i915_vma_types.h
index ec0f6c9f57d0..3144d71a0c3e 100644
--- a/drivers/gpu/drm/i915/i915_vma_types.h
+++ b/drivers/gpu/drm/i915/i9
Re: [PATCH] video: fbdev: pxafb: Remove unnecessary print function dev_err()
On 11/6/22 15:16, wangkail...@jari.cn wrote:
Eliminate the follow coccicheck warning:
./drivers/video/fbdev/pxafb.c:2330:2-9: line 2330 is redundant because
platform_get_irq() already prints an error
Signed-off-by: KaiLong Wang
applied.
Thanks!
Helge
---
drivers/video/fbdev/pxafb.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/video/fbdev/pxafb.c b/drivers/video/fbdev/pxafb.c
index 696ac5431180..c46ed78298ae 100644
--- a/drivers/video/fbdev/pxafb.c
+++ b/drivers/video/fbdev/pxafb.c
@@ -2327,7 +2327,6 @@ static int pxafb_probe(struct platform_device *dev)
irq = platform_get_irq(dev, 0);
if (irq < 0) {
- dev_err(&dev->dev, "no IRQ defined\n");
ret = -ENODEV;
goto failed_free_mem;
}
[PATCH v6 14/20] drm/i915/vm_bind: Expose i915_request_await_bind()
Rename __i915_request_await_bind() as i915_request_await_bind()
and make it non-static as it will be used in execbuf3 ioctl path.
v2: add documentation
Reviewed-by: Matthew Auld
Reviewed-by: Andi Shyti
Signed-off-by: Niranjana Vishwanathapura
---
drivers/gpu/drm/i915/i915_vma.c | 8 +---
drivers/gpu/drm/i915/i915_vma.h | 16
2 files changed, 17 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
index aa4705246993..f73955aef16a 100644
--- a/drivers/gpu/drm/i915/i915_vma.c
+++ b/drivers/gpu/drm/i915/i915_vma.c
@@ -1888,18 +1888,12 @@ void i915_vma_revoke_mmap(struct i915_vma *vma)
list_del(&vma->obj->userfault_link);
}
-static int
-__i915_request_await_bind(struct i915_request *rq, struct i915_vma *vma)
-{
- return __i915_request_await_exclusive(rq, &vma->active);
-}
-
static int __i915_vma_move_to_active(struct i915_vma *vma, struct i915_request
*rq)
{
int err;
/* Wait for the vma to be bound before we start! */
- err = __i915_request_await_bind(rq, vma);
+ err = i915_request_await_bind(rq, vma);
if (err)
return err;
diff --git a/drivers/gpu/drm/i915/i915_vma.h b/drivers/gpu/drm/i915/i915_vma.h
index 04770f8ba815..737ef310d046 100644
--- a/drivers/gpu/drm/i915/i915_vma.h
+++ b/drivers/gpu/drm/i915/i915_vma.h
@@ -54,6 +54,22 @@ void i915_vma_unpin_and_release(struct i915_vma **p_vma,
unsigned int flags);
/* do not reserve memory to prevent deadlocks */
#define __EXEC_OBJECT_NO_RESERVE BIT(31)
+/**
+ * i915_request_await_bind() - Setup request to wait for a vma bind completion
+ * @rq: the request which should wait
+ * @vma: vma whose binding @rq should wait to complete
+ *
+ * Setup the request @rq to asynchronously wait for @vma bind to complete
+ * before starting execution.
+ *
+ * Returns 0 on success, error code on failure.
+ */
+static inline int
+i915_request_await_bind(struct i915_request *rq, struct i915_vma *vma)
+{
+ return __i915_request_await_exclusive(rq, &vma->active);
+}
+
int __must_check _i915_vma_move_to_active(struct i915_vma *vma,
struct i915_request *rq,
struct dma_fence *fence,
--
2.21.0.rc0.32.g243a4c7e27
[PATCH v6 19/20] drm/i915/vm_bind: Render VM_BIND documentation
Update i915 documentation to include VM_BIND changes and render all VM_BIND related documentation. Reviewed-by: Matthew Auld Signed-off-by: Niranjana Vishwanathapura --- Documentation/gpu/i915.rst | 78 -- 1 file changed, 59 insertions(+), 19 deletions(-) diff --git a/Documentation/gpu/i915.rst b/Documentation/gpu/i915.rst index 4e59db1cfb00..5c55cbc980b1 100644 --- a/Documentation/gpu/i915.rst +++ b/Documentation/gpu/i915.rst @@ -283,15 +283,18 @@ An Intel GPU has multiple engines. There are several engine types. The Intel GPU family is a family of integrated GPU's using Unified Memory Access. For having the GPU "do work", user space will feed the -GPU batch buffers via one of the ioctls `DRM_IOCTL_I915_GEM_EXECBUFFER2` -or `DRM_IOCTL_I915_GEM_EXECBUFFER2_WR`. Most such batchbuffers will -instruct the GPU to perform work (for example rendering) and that work -needs memory from which to read and memory to which to write. All memory -is encapsulated within GEM buffer objects (usually created with the ioctl -`DRM_IOCTL_I915_GEM_CREATE`). An ioctl providing a batchbuffer for the GPU -to create will also list all GEM buffer objects that the batchbuffer reads -and/or writes. For implementation details of memory management see -`GEM BO Management Implementation Details`_. +GPU batch buffers via one of the ioctls `DRM_IOCTL_I915_GEM_EXECBUFFER2`, +`DRM_IOCTL_I915_GEM_EXECBUFFER2_WR` or `DRM_IOCTL_I915_GEM_EXECBUFFER3`. +Most such batchbuffers will instruct the GPU to perform work (for example +rendering) and that work needs memory from which to read and memory to +which to write. All memory is encapsulated within GEM buffer objects +(usually created with the ioctl `DRM_IOCTL_I915_GEM_CREATE`). In vm_bind mode +(see `VM_BIND mode`_), the batch buffer and all the GEM buffer objects that +it reads and/or writes should be bound with vm_bind ioctl before submitting +the batch buffer to GPU. In legacy (non-VM_BIND) mode, an ioctl providing a +batchbuffer for the GPU to create will also list all GEM buffer objects that +the batchbuffer reads and/or writes. For implementation details of memory +management see `GEM BO Management Implementation Details`_. The i915 driver allows user space to create a context via the ioctl `DRM_IOCTL_I915_GEM_CONTEXT_CREATE` which is identified by a 32-bit @@ -309,8 +312,9 @@ In addition to the ordering guarantees, the kernel will restore GPU state via HW context when commands are issued to a context, this saves user space the need to restore (most of atleast) the GPU state at the start of each batchbuffer. The non-deprecated ioctls to submit batchbuffer -work can pass that ID (in the lower bits of drm_i915_gem_execbuffer2::rsvd1) -to identify what context to use with the command. +work can pass that ID (drm_i915_gem_execbuffer3::ctx_id, or in the lower +bits of drm_i915_gem_execbuffer2::rsvd1) to identify what context to use +with the command. The GPU has its own memory management and address space. The kernel driver maintains the memory translation table for the GPU. For older @@ -318,14 +322,14 @@ GPUs (i.e. those before Gen8), there is a single global such translation table, a global Graphics Translation Table (GTT). For newer generation GPUs each context has its own translation table, called Per-Process Graphics Translation Table (PPGTT). Of important note, is that although -PPGTT is named per-process it is actually per context. When user space -submits a batchbuffer, the kernel walks the list of GEM buffer objects -used by the batchbuffer and guarantees that not only is the memory of -each such GEM buffer object resident but it is also present in the -(PP)GTT. If the GEM buffer object is not yet placed in the (PP)GTT, -then it is given an address. Two consequences of this are: the kernel -needs to edit the batchbuffer submitted to write the correct value of -the GPU address when a GEM BO is assigned a GPU address and the kernel +PPGTT is named per-process it is actually per context. In legacy +(non-vm_bind) mode, when user space submits a batchbuffer, the kernel walks +the list of GEM buffer objects used by the batchbuffer and guarantees that +not only is the memory of each such GEM buffer object resident but it is +also present in the (PP)GTT. If the GEM buffer object is not yet placed in +the (PP)GTT, then it is given an address. Two consequences of this are: the +kernel needs to edit the batchbuffer submitted to write the correct value +of the GPU address when a GEM BO is assigned a GPU address and the kernel might evict a different GEM BO from the (PP)GTT to make address room for another GEM BO. Consequently, the ioctls submitting a batchbuffer for execution also include a list of all locations within buffers that @@ -407,6 +411,15 @@ objects, which has the goal to make space in gpu virtual address spaces. .. kernel-doc:: drivers/gpu/drm/i915/gem/i915_gem_shrinker.c :internal: +VM_BIND mode + + +.
Re: [PATCH v2 56/65] clk: ingenic: cgu: Switch to determine_rate
Hi, On Fri, Nov 04, 2022 at 05:35:29PM +, Aidan MacDonald wrote: > > Maxime Ripard writes: > > > Hi Paul, > > > > On Fri, Nov 04, 2022 at 02:31:20PM +, Paul Cercueil wrote: > >> Le ven. 4 nov. 2022 à 14:18:13 +0100, Maxime Ripard a > >> écrit : > >> > The Ingenic CGU clocks implements a mux with a set_parent hook, but > >> > doesn't provide a determine_rate implementation. > >> > > >> > This is a bit odd, since set_parent() is there to, as its name implies, > >> > change the parent of a clock. However, the most likely candidate to > >> > trigger that parent change is a call to clk_set_rate(), with > >> > determine_rate() figuring out which parent is the best suited for a > >> > given rate. > >> > > >> > The other trigger would be a call to clk_set_parent(), but it's far less > >> > used, and it doesn't look like there's any obvious user for that clock. > >> > > >> > So, the set_parent hook is effectively unused, possibly because of an > >> > oversight. However, it could also be an explicit decision by the > >> > original author to avoid any reparenting but through an explicit call to > >> > clk_set_parent(). > >> > > >> > The driver does implement round_rate() though, which means that we can > >> > change the rate of the clock, but we will never get to change the > >> > parent. > >> > > >> > However, It's hard to tell whether it's been done on purpose or not. > >> > > >> > Since we'll start mandating a determine_rate() implementation, let's > >> > convert the round_rate() implementation to a determine_rate(), which > >> > will also make the current behavior explicit. And if it was an > >> > oversight, the clock behaviour can be adjusted later on. > >> > >> So it's partly on purpose, partly because I didn't know about > >> .determine_rate. > >> > >> There's nothing odd about having a lonely .set_parent callback; in my case > >> the clocks are parented from the device tree. > >> > >> Having the clocks driver trigger a parent change when requesting a rate > >> change sounds very dangerous, IMHO. My MMC controller can be parented to > >> the > >> external 48 MHz oscillator, and if the card requests 50 MHz, it could > >> switch > >> to one of the PLLs. That works as long as the PLLs don't change rate, but > >> if > >> one is configured as driving the CPU clock, it becomes messy. > >> The thing is, the clocks driver has no way to know whether or not it is > >> "safe" to use a designated parent. > >> > >> For that reason, in practice, I never actually want to have a clock > >> re-parented - it's almost always a bad idea vs. sticking to the parent > >> clock > >> configured in the DTS. > > > > Yeah, and this is totally fine. But we need to be explicit about it. The > > determine_rate implementation I did in all the patches is an exact > > equivalent to the round_rate one if there was one. We will never ask to > > change the parent. > > > > Given what you just said, I would suggest to set the > > CLK_SET_RATE_NO_REPARENT flag as well. > > Ideally there should be a way for drivers and the device tree to > say, "clock X must be driven by clock Y", but the clock framework > would be allowed to re-parent clocks freely as long as it doesn't > violate any DT or driver constraints. I'm not really sure what you mean there, sorry. Isn't it what assigned-clock-parents/clk_set_parent() at probe, plus a determine_rate implementation that would affect best_parent_hw would already provide? > That way allowing reparenting doesn't need to be an all-or-nothing > thing, and it doesn't need to be decided at the clock driver level > with special flags. Like I said, the default implementation is already working to what you suggested if I understood properly. However, this has never been tested for any of the drivers in that series so I don't want to introduce (and debug ;)) regressions in all those drivers that were not setting any constraint but never actually tested their reparenting code. So that series is strictly equivalent to what you had before, it's just explicit now. If you find that some other decision make sense for your driver in particular cases, feel free to change it. I barely know most of these platforms, so I won't be able to make that decision (and test it) unfortunately. Maxime
Re: [PATCH v3 1/2] dt-bindings: backlight: qcom-wled: Add PMI8950 compatible
On Tue, 01 Nov 2022, Luca Weiss wrote: > Document the compatible for the wled block found in PMI8950. > > Signed-off-by: Luca Weiss > --- > Changes since v2: > * New patch > > Documentation/devicetree/bindings/leds/backlight/qcom-wled.yaml | 1 + > 1 file changed, 1 insertion(+) Applied, thanks. -- Lee Jones [李琼斯]
dcn321_fpu.c:626: Array index check in wrong place ?
Hello there,
Static analyser cppcheck says:
linux-6.1-rc4/drivers/gpu/drm/amd/display/dc/dml/dcn321/dcn321_fpu.c:626:27:
style: Array index 'i' is used before limits check. [arrayIndexThenCheck]
Source code is
if (dcfclk_sta_targets[i] < optimal_dcfclk_for_uclk[j] && i <
num_dcfclk_sta_targets) {
It might be wise to move the limits check to before use.
Regards
David Binderman
Re: [PATCH 4/8] pwm: atmel-hlcdc: fix struct clk pointer comparing
[PATCH] driver: gpu: add failure check for ftell
From: shenyanfeng
add return-value check of ftell to improve robustness(and avoid abnormal
behavior)
Signed-off-by: SPeak
Signed-off-by: shenyanfeng
---
Receive "Undelivered Mail Returned to Sender", so send again
drivers/gpu/drm/radeon/mkregtable.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/radeon/mkregtable.c
b/drivers/gpu/drm/radeon/mkregtable.c
index 52a7246fe..c31c58e5f 100644
--- a/drivers/gpu/drm/radeon/mkregtable.c
+++ b/drivers/gpu/drm/radeon/mkregtable.c
@@ -193,6 +193,7 @@ static int parser_auth(struct table *t, const char
*filename)
regmatch_t match[4];
char buf[1024];
size_t end;
+ long pos;
int len;
int done = 0;
int r;
@@ -228,12 +229,12 @@ static int parser_auth(struct table *t, const char
*filename)
last_reg = strtol(last_reg_s, NULL, 16);
do {
- if (fgets(buf, 1024, file) == NULL) {
+ if ((fgets(buf, 1024, file) == NULL) || (pos = ftell(file)) <
0) {
fclose(file);
return -1;
}
len = strlen(buf);
- if (ftell(file) == end)
+ if (pos == end)
done = 1;
if (len) {
r = regexec(&mask_rex, buf, 4, match, 0);
--
2.25.1
Re: Must-Pass Test Suite for KMS drivers
On Thu, Oct 27, 2022 at 08:08:28AM -0700, Rob Clark wrote: > On Wed, Oct 26, 2022 at 1:17 AM wrote: > > > > Hi Rob, > > > > On Mon, Oct 24, 2022 at 08:48:15AM -0700, Rob Clark wrote: > > > On Mon, Oct 24, 2022 at 5:43 AM wrote: > > > > I've discussing the idea for the past year to add an IGT test suite that > > > > all well-behaved KMS drivers must pass. > > > > > > > > The main idea behind it comes from v4l2-compliance and cec-compliance, > > > > that are being used to validate that the drivers are sane. > > > > > > > > We should probably start building up the test list, and eventually > > > > mandate that all tests pass for all the new KMS drivers we would merge > > > > in the kernel, and be run by KCi or similar. > > > > > > Let's get https://patchwork.freedesktop.org/patch/502641/ merged > > > first, that already gives us a mechanism similar to what we use in > > > mesa to track pass/fail/flake > > > > I'm not sure it's a dependency per-se, and I believe both can (and > > should) happen separately. > > Basically my reasoning is that getting IGT green is a process that so > far is consisting of equal parts IGT test fixes, to clear out > lingering i915'isms, etc, and driver fixes. Yes, you could do this > manually but the drm/ci approach seems like it would make it easier to > track, so it is easier to see what tests are being run on which hw, > and what the pass/fail/flake status is. And the expectation files can > also be updated as we uprev the igt version being used in CI. > > I could be biased by how CI has been deployed (IMHO, successfully) in > mesa.. my experience there doesn't make me see any value in a > "mustpass" list. But does make me see value in automating and > tracking status. Obviously we want all the tests to pass, but getting > there is going to be a process. Tracking that progress is the thing > that is useful now. Yeah, I understand where you're coming from, and for CI I agree that your approach looks like the best one. It's not what I'm trying to address though. My issue is that, even though I have a bunch of KMS experience by now, every time I need to use IGT, I have exactly zero idea what test I need to run to check that a given driver behaves decently. I have no idea which tests I should run, which tests are supposed to be working but can't really because of some intel-specific behavior, which tests are skipped but shouldn't, which tests are broken but should be, etc. I don't want to have a nice table with everything green because there was no regression, I want to see which bugs I haven't found out are still lingering in my driver. I've been chasing bugs too many times where it turned out that there was a test for that in IGT somewhere, hidden in a 70k tests haystack with zero documentation. So, yeah, I get what you're saying, it makes sense, and please go forward with drm/ci. I still think we need to find a beginning of a solution for the issue I'm talking about. Maxime
Re: Must-Pass Test Suite for KMS drivers
Hi Thomas, On Fri, Oct 28, 2022 at 09:31:38AM +0200, Thomas Zimmermann wrote: > Am 24.10.22 um 14:43 schrieb max...@cerno.tech: > > I've discussing the idea for the past year to add an IGT test suite that > > all well-behaved KMS drivers must pass. > > > > The main idea behind it comes from v4l2-compliance and cec-compliance, > > that are being used to validate that the drivers are sane. > > > > We should probably start building up the test list, and eventually > > mandate that all tests pass for all the new KMS drivers we would merge > > in the kernel, and be run by KCi or similar. > > > > I did a first pass to create a draft of such a test-suite, which would > > contain: > > > > igt@core_auth@basic-auth > > igt@core_auth@getclient-master-drop > > igt@core_auth@getclient-simple > > igt@core_auth@many-magics > > igt@core_getclient > > igt@core_getstats > > igt@core_getversion > > igt@core_hotunplug@hotrebind-lateclose > > igt@core_hotunplug@hotunbind-rebind > > igt@core_hotunplug@unbind-rebind > > igt@core_setmaster > > igt@core_setmaster_vs_auth > > igt@device_reset@unbind-reset-rebind > > igt@drm_read > > igt@dumb_buffer > > igt@fbdev > > Maybe we make this test optional? At least sprd decided to not support fbdev > at all. I'm not sure we need to make that test optional, or at least, we should run it all the time, but skip it if there's no fbdev emulation, and not report it as a failure? Maxime
[PATCH] [next] drm/amdgpu: Replace 1-element array with flexible-array member
One-element arrays are deprecated, and we are replacing them with
flexible array members instead. So, replace one-element array with
flexible-array member in structs _ATOM_GPIO_PIN_ASSIGNMENT,
_ATOM_DISPLAY_OBJECT_PATH, _ATOM_DISPLAY_OBJECT_PATH_TABLE,
_ATOM_OBJECT_TABLE and refactor the rest of the code accordingly.
Important to mention is that doing a build before/after this patch results
in no functional binary output differences.
This helps with the ongoing efforts to tighten the FORTIFY_SOURCE
routines on memcpy() and help us make progress towards globally
enabling -fstrict-flex-arrays=3 [1].
Link: https://github.com/KSPP/linux/issues/79
Link: https://github.com/KSPP/linux/issues/238
Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101836 [1]
Signed-off-by: Paulo Miguel Almeida
---
Binary difference findings:
Some changes took more than a single line which changed the line
number parameter passed to the drm_dbg function (which leverages
kernel's dynamic debugging). Functionally-wise, nothing changed
after doing a before/after patch build.
Additional one-element arrays to be changed:
There are more instances of one-element arrays to be changed but
I will keep patches small so they are easy to review. [and I can
only dedicate a few hours per day on this :-) ]
---
.../gpu/drm/amd/display/dc/bios/bios_parser.c | 23 ---
drivers/gpu/drm/amd/include/atombios.h| 8 +++
2 files changed, 19 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/dc/bios/bios_parser.c
b/drivers/gpu/drm/amd/display/dc/bios/bios_parser.c
index 9b8ea6e9a2b9..39dd8b2dc254 100644
--- a/drivers/gpu/drm/amd/display/dc/bios/bios_parser.c
+++ b/drivers/gpu/drm/amd/display/dc/bios/bios_parser.c
@@ -138,7 +138,9 @@ static uint8_t get_number_of_objects(struct bios_parser
*bp, uint32_t offset)
uint32_t object_table_offset = bp->object_info_tbl_offset + offset;
- table = GET_IMAGE(ATOM_OBJECT_TABLE, object_table_offset);
+ table = ((ATOM_OBJECT_TABLE *) bios_get_image(&bp->base,
+ object_table_offset,
+ struct_size(table, asObjects, 1)));
if (!table)
return 0;
@@ -166,8 +168,9 @@ static struct graphics_object_id
bios_parser_get_connector_id(
uint32_t connector_table_offset = bp->object_info_tbl_offset
+
le16_to_cpu(bp->object_info_tbl.v1_1->usConnectorObjectTableOffset);
- ATOM_OBJECT_TABLE *tbl =
- GET_IMAGE(ATOM_OBJECT_TABLE, connector_table_offset);
+ ATOM_OBJECT_TABLE *tbl = ((ATOM_OBJECT_TABLE *)
bios_get_image(&bp->base,
+ connector_table_offset,
+ struct_size(tbl, asObjects, 1)));
if (!tbl) {
dm_error("Can't get connector table from atom bios.\n");
@@ -1789,11 +1792,13 @@ static enum bp_result bios_parser_get_gpio_pin_info(
if (!DATA_TABLES(GPIO_Pin_LUT))
return BP_RESULT_BADBIOSTABLE;
- header = GET_IMAGE(ATOM_GPIO_PIN_LUT, DATA_TABLES(GPIO_Pin_LUT));
+ header = ((ATOM_GPIO_PIN_LUT *) bios_get_image(&bp->base,
+ DATA_TABLES(GPIO_Pin_LUT),
+ struct_size(header, asGPIO_Pin, 1)));
if (!header)
return BP_RESULT_BADBIOSTABLE;
- if (sizeof(ATOM_COMMON_TABLE_HEADER) + sizeof(ATOM_GPIO_PIN_LUT)
+ if (sizeof(ATOM_COMMON_TABLE_HEADER) + struct_size(header, asGPIO_Pin,
1)
> le16_to_cpu(header->sHeader.usStructureSize))
return BP_RESULT_BADBIOSTABLE;
@@ -1978,7 +1983,8 @@ static ATOM_OBJECT *get_bios_object(struct bios_parser
*bp,
offset += bp->object_info_tbl_offset;
- tbl = GET_IMAGE(ATOM_OBJECT_TABLE, offset);
+ tbl = ((ATOM_OBJECT_TABLE *) bios_get_image(&bp->base, offset,
+ struct_size(tbl, asObjects, 1)));
if (!tbl)
return NULL;
@@ -2709,8 +2715,9 @@ static enum bp_result get_bracket_layout_record(
genericTableOffset = bp->object_info_tbl_offset +
bp->object_info_tbl.v1_3->usMiscObjectTableOffset;
- object_table = (ATOM_OBJECT_TABLE *)
- GET_IMAGE(ATOM_OBJECT_TABLE, genericTableOffset);
+ object_table = ((ATOM_OBJECT_TABLE *) bios_get_image(&bp->base,
+ genericTableOffset,
+ struct_size(object_table, asObjects, 1)));
if (!object_table)
return BP_RESULT_FAILURE;
diff --git a/drivers/gpu/drm/amd/include/atombios.h
b/drivers/gpu/drm/amd/include/atombios.h
index b5b1d073f8e2..55ae93c1e365 100644
--- a/drivers/gpu/drm/amd/include/atombios.h
+++ b/drivers/gpu/drm/amd/include/atombios.h
@@ -4386,7 +4386,7 @@ typedef struct _ATOM_GPIO_PIN_ASSIGNMENT
typedef struct _ATOM_GPIO_PIN_LUT
{
ATOM_
Re: [Intel-gfx] [PATCH 1/2] drm/i915/gt: Add GT oriented dmesg output
On 05/11/2022 01:03, Ceraolo Spurio, Daniele wrote:
On 11/4/2022 10:25 AM, john.c.harri...@intel.com wrote:
From: John Harrison
When trying to analyse bug reports from CI, customers, etc. it can be
difficult to work out exactly what is happening on which GT in a
multi-GT system. So add GT oriented debug/error message wrappers. If
used instead of the drm_ equivalents, you get the same output but with
a GT# prefix on it.
Signed-off-by: John Harrison
The only downside to this is that we'll print "GT0: " even on single-GT
devices. We could introduce a gt->info.name and print that, so we could
have it different per-platform, but IMO it's not worth the effort.
Reviewed-by: Daniele Ceraolo Spurio
I think it might be worth getting an ack from one of the maintainers to
make sure we're all aligned on transitioning to these new logging macro
for gt code.
Idea is I think a very good one. First I would suggest standardising to
lowercase GT in logs because:
$ grep "GT%" i915/ -r
$ grep "gt%" i915/ -r
i915/gt/intel_gt_sysfs.c:gt->i915->sysfs_gt,
"gt%d", gt->info.id))
i915/gt/intel_gt_sysfs.c:"failed to initialize gt%d sysfs root\n",
gt->info.id);
i915/gt/intel_gt_sysfs_pm.c: "failed to create gt%u RC6 sysfs
files (%pe)\n",
i915/gt/intel_gt_sysfs_pm.c: "failed to create gt%u RC6p
sysfs files (%pe)\n",
i915/gt/intel_gt_sysfs_pm.c: "failed to create gt%u RPS sysfs
files (%pe)",
i915/gt/intel_gt_sysfs_pm.c: "failed to create gt%u
punit_req_freq_mhz sysfs (%pe)",
i915/gt/intel_gt_sysfs_pm.c: "failed to create gt%u
throttle sysfs files (%pe)",
i915/gt/intel_gt_sysfs_pm.c: "failed to create gt%u
media_perf_power_attrs sysfs (%pe)\n",
i915/gt/intel_gt_sysfs_pm.c: "failed to add gt%u rps defaults
(%pe)\n",
i915/i915_driver.c: drm_err(>->i915->drm, "gt%d:
intel_pcode_init failed %d\n", id, ret);
i915/i915_hwmon.c: snprintf(ddat_gt->name, sizeof(ddat_gt->name),
"i915_gt%u", i);
Then there is a question of naming. Are we okay with GT_XXX or, do we want
intel_gt_, or something completely different. I don't have a strong opinion at
the moment so I'll add some more folks to Cc.
What I'd would like to see tried is to converting all of i915/gt within one
kernel release so we don't have a mish-mash of log formats.
Regards,
Tvrtko
---
drivers/gpu/drm/i915/gt/intel_gt.h | 15 +++
1 file changed, 15 insertions(+)
diff --git a/drivers/gpu/drm/i915/gt/intel_gt.h
b/drivers/gpu/drm/i915/gt/intel_gt.h
index e0365d5562484..1e016fb0117a4 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt.h
+++ b/drivers/gpu/drm/i915/gt/intel_gt.h
@@ -13,6 +13,21 @@
struct drm_i915_private;
struct drm_printer;
+#define GT_ERR(_gt, _fmt, ...) \
+ drm_err(&(_gt)->i915->drm, "GT%u: " _fmt, (_gt)->info.id,
##__VA_ARGS__)
+
+#define GT_WARN(_gt, _fmt, ...) \
+ drm_warn(&(_gt)->i915->drm, "GT%u: " _fmt, (_gt)->info.id,
##__VA_ARGS__)
+
+#define GT_NOTICE(_gt, _fmt, ...) \
+ drm_notice(&(_gt)->i915->drm, "GT%u: " _fmt, (_gt)->info.id,
##__VA_ARGS__)
+
+#define GT_INFO(_gt, _fmt, ...) \
+ drm_info(&(_gt)->i915->drm, "GT%u: " _fmt, (_gt)->info.id,
##__VA_ARGS__)
+
+#define GT_DBG(_gt, _fmt, ...) \
+ drm_dbg(&(_gt)->i915->drm, "GT%u: " _fmt, (_gt)->info.id,
##__VA_ARGS__)
+
#define GT_TRACE(gt, fmt, ...) do { \
const struct intel_gt *gt__ __maybe_unused = (gt); \
GEM_TRACE("%s " fmt, dev_name(gt__->i915->drm.dev), \
Re: Must-Pass Test Suite for KMS drivers
Hi Am 07.11.22 um 10:30 schrieb Maxime Ripard: Hi Thomas, On Fri, Oct 28, 2022 at 09:31:38AM +0200, Thomas Zimmermann wrote: Am 24.10.22 um 14:43 schrieb max...@cerno.tech: I've discussing the idea for the past year to add an IGT test suite that all well-behaved KMS drivers must pass. The main idea behind it comes from v4l2-compliance and cec-compliance, that are being used to validate that the drivers are sane. We should probably start building up the test list, and eventually mandate that all tests pass for all the new KMS drivers we would merge in the kernel, and be run by KCi or similar. I did a first pass to create a draft of such a test-suite, which would contain: igt@core_auth@basic-auth igt@core_auth@getclient-master-drop igt@core_auth@getclient-simple igt@core_auth@many-magics igt@core_getclient igt@core_getstats igt@core_getversion igt@core_hotunplug@hotrebind-lateclose igt@core_hotunplug@hotunbind-rebind igt@core_hotunplug@unbind-rebind igt@core_setmaster igt@core_setmaster_vs_auth igt@device_reset@unbind-reset-rebind igt@drm_read igt@dumb_buffer igt@fbdev Maybe we make this test optional? At least sprd decided to not support fbdev at all. I'm not sure we need to make that test optional, or at least, we should run it all the time, but skip it if there's no fbdev emulation, and not report it as a failure? Sure. I just meant that fbdev support shouldn't be a blocker. If there, it should work of course. Best regards Thomas Maxime -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Ivo Totev OpenPGP_signature Description: OpenPGP digital signature
Re: [PATCH v6 10/23] drm/modes: Fill drm_cmdline mode from named modes
On Sun, Nov 06, 2022 at 02:04:56PM +0100, Noralf Trønnes wrote: > > > Den 26.10.2022 17.33, skrev max...@cerno.tech: > > The current code to deal with named modes will only set the mode name, and > > then it's up to drivers to try to match that name to whatever mode or > > configuration they see fit. > > > > I couldn't find any driver that does that, all I could find that cares > about named modes are drm_client. Did I miss something here? sun4i at least does it: https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/sun4i/sun4i_tv.c#L292 I'm not aware of any other driver relying on named modes though. > Apart from that: > > Reviewed-by: Noralf Trønnes Thanks! Maxime signature.asc Description: PGP signature
Re: [PATCH v3 02/12] dt-bindings: display: mediatek: add MT8195 hdmi bindings
On 04/11/2022 15:09, Guillaume Ranquet wrote: > Add mt8195 SoC bindings for hdmi and hdmi-ddc > > On mt8195 the ddc i2c controller is part of the hdmi IP block and thus has no > specific register range, power domain or interrupt, making it simpler > than its the legacy "mediatek,hdmi-ddc" binding. > > Signed-off-by: Guillaume Ranquet > --- > .../bindings/display/mediatek/mediatek,hdmi.yaml | 61 > ++ > .../display/mediatek/mediatek,mt8195-hdmi-ddc.yaml | 51 ++ > 2 files changed, 101 insertions(+), 11 deletions(-) > > diff --git > a/Documentation/devicetree/bindings/display/mediatek/mediatek,hdmi.yaml > b/Documentation/devicetree/bindings/display/mediatek/mediatek,hdmi.yaml > index bdaf0b51e68c..9710b7b6e9bf 100644 > --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,hdmi.yaml > +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,hdmi.yaml > @@ -21,6 +21,7 @@ properties: >- mediatek,mt7623-hdmi >- mediatek,mt8167-hdmi >- mediatek,mt8173-hdmi > + - mediatek,mt8195-hdmi > >reg: > maxItems: 1 > @@ -29,18 +30,12 @@ properties: > maxItems: 1 > >clocks: > -items: > - - description: Pixel Clock > - - description: HDMI PLL > - - description: Bit Clock > - - description: S/PDIF Clock > +minItems: 4 Drop minItems, it's not needed when equal to maxItems. > +maxItems: 4 > >clock-names: > -items: > - - const: pixel > - - const: pll > - - const: bclk > - - const: spdif > +minItems: 4 Drop minItems, it's not needed when equal to maxItems. > +maxItems: 4 > >phys: > maxItems: 1 > @@ -58,6 +53,9 @@ properties: > description: | >phandle link and register offset to the system configuration registers. > > + power-domains: > +maxItems: 1 > + >ports: > $ref: /schemas/graph.yaml#/properties/ports > > @@ -86,9 +84,50 @@ required: >- clock-names >- phys >- phy-names > - - mediatek,syscon-hdmi >- ports > > +allOf: > + - if: > + properties: > +compatible: > + contains: > +const: mediatek,mt8195-hdmi > +then: > + properties: > +clocks: > + items: > +- description: APB > +- description: HDCP > +- description: HDCP 24M > +- description: Split HDMI > +clock-names: > + items: > +- const: hdmi_apb_sel > +- const: hdcp_sel > +- const: hdcp24_sel > +- const: split_hdmi > + > + required: > +- power-domains > +else: > + properties: > +clocks: > + items: > +- description: Pixel Clock > +- description: HDMI PLL > +- description: Bit Clock > +- description: S/PDIF Clock > + > +clock-names: > + items: > +- const: pixel > +- const: pll > +- const: bclk > +- const: spdif > + > + required: > +- mediatek,syscon-hdmi > + > additionalProperties: false > > examples: > diff --git > a/Documentation/devicetree/bindings/display/mediatek/mediatek,mt8195-hdmi-ddc.yaml > > b/Documentation/devicetree/bindings/display/mediatek/mediatek,mt8195-hdmi-ddc.yaml > new file mode 100644 > index ..2dc273689584 > --- /dev/null > +++ > b/Documentation/devicetree/bindings/display/mediatek/mediatek,mt8195-hdmi-ddc.yaml > @@ -0,0 +1,51 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: > http://devicetree.org/schemas/display/mediatek/mediatek,mt8195-hdmi-ddc.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Mediatek HDMI DDC for mt8195 > + > +maintainers: > + - CK Hu > + - Jitao shi > + > +description: | > + The HDMI DDC i2c controller is used to interface with the HDMI DDC pins. > + > +properties: > + compatible: > +enum: > + - mediatek,mt8195-hdmi-ddc > + > + clocks: > +maxItems: 1 > + > + clock-names: > +items: > + - const: ddc Unless you expect it to grow, I propose to drop clock-names. It's not useful when the name is the same as name of hardware. Best regards, Krzysztof
Re: [PATCH v6 16/23] drm/probe-helper: Provide a TV get_modes helper
Hi Noralf, On Sun, Nov 06, 2022 at 05:59:23PM +0100, Noralf Trønnes wrote: > > > Den 27.10.2022 00.02, skrev Mateusz Kwiatkowski: > > Hi Maxime, > > > > First of all, nice idea with the helper function that can be reused by > > different > > drivers. This is neat! > > > > But looking at this function, it feels a bit overcomplicated. You're > > creating > > the two modes, then checking which one is the default, then set the > > preferred > > one and possibly reorder them. Maybe it can be simplified somehow? > > > > Although when I tried to refactor it myself, I ended up with something > > that's > > not better at all. Maybe it needs to be complicated, after all :( > > > > I also thought that the function was complicated/difficult to read, in > particular the index stuff at the end, but I also failed in finding a > "better" solution, just a different one ;) I think I like yours better still :) Can I bring it into my series, with your authorship and SoB? Maxime signature.asc Description: PGP signature
Re: [PATCH v6 16/23] drm/probe-helper: Provide a TV get_modes helper
Hi Noralf, I'll leave aside your comments on the code, since we'll use your implementation. On Sun, Nov 06, 2022 at 05:33:48PM +0100, Noralf Trønnes wrote: > Den 26.10.2022 17.33, skrev max...@cerno.tech: > > + > > + if (cmdline->tv_mode_specified) > > + default_mode = cmdline->tv_mode; > > I realised that we don't verify tv_mode coming from the command line, > not here and not in the reset helper. Should we do that? A driver should > be programmed defensively to handle an illegal/unsupported value, but it > doesn't feel right to allow an illegal enum value coming through the > core/helpers. I don't think we can end up with an invalid value here if it's been specified. We parse the command line through drm_mode_parse_tv_mode() (introduced in patch 13 "drm/modes: Introduce the tv_mode property as a command-line option") that will pick the tv mode part of the command line, and call drm_get_tv_mode_from_name() using it. drm_get_tv_mode_from_name() will return a EINVAL if it's not a value we expect, and mode->tv_mode is only set on success. And AFAIK, there's no other path that will set tv_mode. Maxime signature.asc Description: PGP signature
Re: [igt-dev] Must-Pass Test Suite for KMS drivers
On Mon, 7 Nov 2022 at 10:43, Thomas Zimmermann wrote: > > Hi > > Am 07.11.22 um 10:30 schrieb Maxime Ripard: > > Hi Thomas, > > > > On Fri, Oct 28, 2022 at 09:31:38AM +0200, Thomas Zimmermann wrote: > >> Am 24.10.22 um 14:43 schrieb max...@cerno.tech: > >>> I've discussing the idea for the past year to add an IGT test suite that > >>> all well-behaved KMS drivers must pass. > >>> > >>> The main idea behind it comes from v4l2-compliance and cec-compliance, > >>> that are being used to validate that the drivers are sane. > >>> > >>> We should probably start building up the test list, and eventually > >>> mandate that all tests pass for all the new KMS drivers we would merge > >>> in the kernel, and be run by KCi or similar. > >>> > >>> I did a first pass to create a draft of such a test-suite, which would > >>> contain: > >>> > >>> igt@core_auth@basic-auth > >>> igt@core_auth@getclient-master-drop > >>> igt@core_auth@getclient-simple > >>> igt@core_auth@many-magics > >>> igt@core_getclient > >>> igt@core_getstats > >>> igt@core_getversion > >>> igt@core_hotunplug@hotrebind-lateclose > >>> igt@core_hotunplug@hotunbind-rebind > >>> igt@core_hotunplug@unbind-rebind > >>> igt@core_setmaster > >>> igt@core_setmaster_vs_auth > >>> igt@device_reset@unbind-reset-rebind > >>> igt@drm_read > >>> igt@dumb_buffer > >>> igt@fbdev > >> > >> Maybe we make this test optional? At least sprd decided to not support > >> fbdev > >> at all. > > > > I'm not sure we need to make that test optional, or at least, we should > > run it all the time, but skip it if there's no fbdev emulation, and not > > report it as a failure? > > Sure. I just meant that fbdev support shouldn't be a blocker. If there, > it should work of course. Not supporting fbdev looks more like an accident than intention here, and maybe a good reason to have these kind of must-past lists. Eventually, once the i915-ism is worked out of igt well enough for a set of tests at least. The drm/ci effort should help in building up that list (by essentially extracting the common set of tests that everyone passes and graduating that to the must-pass list for kms conformance or whatever we'll call it). -Daniel > > Best regards > Thomas > > > > > Maxime > > -- > Thomas Zimmermann > Graphics Driver Developer > SUSE Software Solutions Germany GmbH > Maxfeldstr. 5, 90409 Nürnberg, Germany > (HRB 36809, AG Nürnberg) > Geschäftsführer: Ivo Totev -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Re: (subset) [PATCH] drm/vc4: Fix missing platform_unregister_drivers() call in vc4_drm_register()
On Thu, 3 Nov 2022 01:47:05 +, Yuan Can wrote: > A problem about modprobe vc4 failed is triggered with the following log > given: > > [ 420.327987] Error: Driver 'vc4_hvs' is already registered, aborting... > [ 420.333904] failed to register platform driver vc4_hvs_driver [vc4]: -16 > modprobe: ERROR: could not insert 'vc4': Device or resource busy > > [...] Applied to drm/drm-misc (drm-misc-fixes). Thanks! Maxime
[PATCH] drm/msm/dpu1: Remove INTF4 IRQ from SDM845 IRQ mask
From: Konrad Dybcio SDM845 only has INTF0-3 and has no business caring about the INTF4 irq. Suggested-by: Dmitry Baryshkov Signed-off-by: Konrad Dybcio Signed-off-by: Konrad Dybcio --- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 1 - 1 file changed, 1 deletion(-) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c index 27f029fdc682..06897a497eb7 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c @@ -86,7 +86,6 @@ BIT(MDP_INTF1_INTR) | \ BIT(MDP_INTF2_INTR) | \ BIT(MDP_INTF3_INTR) | \ -BIT(MDP_INTF4_INTR) | \ BIT(MDP_AD4_0_INTR) | \ BIT(MDP_AD4_1_INTR)) -- 2.38.1
Re: [PATCH v1 3/5] arm64: dts: qcom: sm8450-hdk: enable display hardware
On 06/11/2022 05:30, Bjorn Andersson wrote:
On Fri, Nov 04, 2022 at 04:13:56PM +0300, Dmitry Baryshkov wrote:
Enable MDSS/DPU/DSI0 on SM8450-HDK device. Note, there is no panel
configuration (yet).
Signed-off-by: Dmitry Baryshkov
---
arch/arm64/boot/dts/qcom/sm8450-hdk.dts | 18 ++
1 file changed, 18 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/sm8450-hdk.dts
b/arch/arm64/boot/dts/qcom/sm8450-hdk.dts
index 38ccd44620d0..e1a4cf1ee51d 100644
--- a/arch/arm64/boot/dts/qcom/sm8450-hdk.dts
+++ b/arch/arm64/boot/dts/qcom/sm8450-hdk.dts
@@ -442,3 +442,21 @@ &usb_1_qmpphy {
vdda-phy-supply = <&vreg_l6b_1p2>;
vdda-pll-supply = <&vreg_l1b_0p91>;
};
+
+&mdss {
+ status = "okay";
+};
+
+&mdss_mdp {
+ status = "okay";
+};
+
+&dsi0 {
Please prefix the labels with "mdss_" so that you can keep them sorted
alphabetically.
Why such a change all of a sudden? Only downstream (and sc7280 upstream)
has mdss_ prefixes for dsi. Plain 'dsiN' is more generic.
Konrad
THanks,
Bjorn
+ status = "okay";
+ vdda-supply = <&vreg_l6b_1p2>;
+};
+
+&dsi0_phy {
+ status = "okay";
+ vdds-supply = <&vreg_l5b_0p88>;
+};
--
2.35.1
[PATCH 2/2] fbdev: Add support for the nomodeset kernel parameter
Support the kernel's nomodeset parameter for all PCI-based fbdev
drivers that use aperture helpers to remove other, hardware-agnostic
graphics drivers.
The parameter is a simple way of using the firmware-provided scanout
buffer if the hardware's native driver is broken. The same effect
could be achieved with per-driver options, but the importance of the
graphics output for many users makes a single, unified approach
worthwhile.
With nomodeset specified, the fbdev driver module will not load. This
unifies behavior with similar DRM drivers. In DRM helpers, modules
first check the nomodeset parameter before registering the PCI
driver. As fbdev has no such module helpers, we have to modify each
driver individually.
The name 'nomodeset' is slightly misleading, but has been chosen for
historical reasons. Several drivers implemented it before it became a
general option for DRM. So keeping the existing name was preferred over
introducing a new one.
Signed-off-by: Thomas Zimmermann
---
drivers/staging/sm750fb/Kconfig | 1 +
drivers/staging/sm750fb/sm750.c | 4 +++
drivers/video/fbdev/Kconfig | 37
drivers/video/fbdev/arkfb.c | 6
drivers/video/fbdev/asiliantfb.c | 5 +++
drivers/video/fbdev/aty/aty128fb.c | 6
drivers/video/fbdev/aty/atyfb_base.c | 6
drivers/video/fbdev/aty/radeon_base.c| 6
drivers/video/fbdev/carminefb.c | 5 +++
drivers/video/fbdev/chipsfb.c| 5 +++
drivers/video/fbdev/cirrusfb.c | 6
drivers/video/fbdev/cyber2000fb.c| 6
drivers/video/fbdev/geode/Kconfig| 3 ++
drivers/video/fbdev/geode/gx1fb_core.c | 7
drivers/video/fbdev/geode/gxfb_core.c| 7
drivers/video/fbdev/geode/lxfb_core.c| 7
drivers/video/fbdev/gxt4500.c| 5 +++
drivers/video/fbdev/hyperv_fb.c | 5 +++
drivers/video/fbdev/i740fb.c | 6
drivers/video/fbdev/i810/i810_main.c | 8 +
drivers/video/fbdev/imsttfb.c| 7
drivers/video/fbdev/intelfb/intelfbdrv.c | 5 +++
drivers/video/fbdev/kyro/fbdev.c | 6
drivers/video/fbdev/matrox/matroxfb_base.c | 5 +++
drivers/video/fbdev/mb862xx/mb862xxfbdrv.c | 5 +++
drivers/video/fbdev/neofb.c | 6
drivers/video/fbdev/nvidia/nvidia.c | 7
drivers/video/fbdev/pm2fb.c | 6
drivers/video/fbdev/pm3fb.c | 6
drivers/video/fbdev/pvr2fb.c | 7
drivers/video/fbdev/riva/fbdev.c | 7
drivers/video/fbdev/s3fb.c | 6
drivers/video/fbdev/savage/savagefb_driver.c | 5 +++
drivers/video/fbdev/sis/sis_main.c | 7
drivers/video/fbdev/skeletonfb.c | 7
drivers/video/fbdev/sm712fb.c| 5 +++
drivers/video/fbdev/sstfb.c | 4 +++
drivers/video/fbdev/sunxvr2500.c | 5 +++
drivers/video/fbdev/sunxvr500.c | 5 +++
drivers/video/fbdev/tdfxfb.c | 6
drivers/video/fbdev/tgafb.c | 6
drivers/video/fbdev/tridentfb.c | 6
drivers/video/fbdev/vermilion/vermilion.c| 7
drivers/video/fbdev/via/via-core.c | 5 +++
drivers/video/fbdev/vt8623fb.c | 6
45 files changed, 288 insertions(+)
diff --git a/drivers/staging/sm750fb/Kconfig b/drivers/staging/sm750fb/Kconfig
index 8c0d8a873d5b0..acb6c08d09dce 100644
--- a/drivers/staging/sm750fb/Kconfig
+++ b/drivers/staging/sm750fb/Kconfig
@@ -6,6 +6,7 @@ config FB_SM750
select FB_CFB_FILLRECT
select FB_CFB_COPYAREA
select FB_CFB_IMAGEBLIT
+ select VIDEO_NOMODESET
help
Frame buffer driver for the Silicon Motion SM750 chip
with 2D accelearion and dual head support.
diff --git a/drivers/staging/sm750fb/sm750.c b/drivers/staging/sm750fb/sm750.c
index 168ae2e9005d7..d6f89dd3f59ba 100644
--- a/drivers/staging/sm750fb/sm750.c
+++ b/drivers/staging/sm750fb/sm750.c
@@ -17,6 +17,7 @@
#include
#include
#include
+#include
#include "sm750.h"
#include "sm750_accel.h"
#include "sm750_cursor.h"
@@ -1168,6 +1169,9 @@ static int __init lynxfb_init(void)
{
char *option;
+ if (video_firmware_drivers_only())
+ return -ENODEV;
+
#ifdef MODULE
option = g_option;
#else
diff --git a/drivers/video/fbdev/Kconfig b/drivers/video/fbdev/Kconfig
index a98987aa27846..71019b167f8b0 100644
--- a/drivers/video/fbdev/Kconfig
+++ b/drivers/video/fbdev/Kconfig
@@ -227,6 +227,7 @@ config FB_CIRRUS
select FB_CFB_FILLRECT
select FB_CFB_COPYAREA
select FB_CFB_IMAGEBLIT
+ select VIDEO_NOMODESET
help
This enables support for Cirrus Logic GD542x/543x base
[PATCH 0/2] video/fbdev: Support 'nomodeset' in PCI drivers
Add support for the kernel's 'nomodeset' parameter to PCI-based
fbdev drivers. The option prevents DRM drivers from loading if they
could possibly displace a hardware-agnostic driver that runs on the
firmware framebuffer. It is a fallback for systems on which the
hardware's native driver does not work correctly.
After the work on the aperture helpers and their integration with
fbdev, it can happen that an fbdev driver replaces a hardware-
agnostic DRM driver. Supporting the nomodeset parameter unifies
the behavior among the graphics drivers and subsystems. An fbdev
driver will not replace any hardware-agnostic driver with nomodeset
given.
Thomas Zimmermann (2):
drm: Move nomodeset kernel parameter to drivers/video
fbdev: Add support for the nomodeset kernel parameter
.../admin-guide/kernel-parameters.txt | 2 +-
MAINTAINERS | 2 +
drivers/gpu/drm/Kconfig | 7 +---
drivers/gpu/drm/Makefile | 1 -
drivers/staging/sm750fb/Kconfig | 1 +
drivers/staging/sm750fb/sm750.c | 4 ++
drivers/video/Kconfig | 4 ++
drivers/video/Makefile| 1 +
drivers/video/fbdev/Kconfig | 37 +++
drivers/video/fbdev/arkfb.c | 6 +++
drivers/video/fbdev/asiliantfb.c | 5 +++
drivers/video/fbdev/aty/aty128fb.c| 6 +++
drivers/video/fbdev/aty/atyfb_base.c | 6 +++
drivers/video/fbdev/aty/radeon_base.c | 6 +++
drivers/video/fbdev/carminefb.c | 5 +++
drivers/video/fbdev/chipsfb.c | 5 +++
drivers/video/fbdev/cirrusfb.c| 6 +++
drivers/video/fbdev/cyber2000fb.c | 6 +++
drivers/video/fbdev/geode/Kconfig | 3 ++
drivers/video/fbdev/geode/gx1fb_core.c| 7
drivers/video/fbdev/geode/gxfb_core.c | 7
drivers/video/fbdev/geode/lxfb_core.c | 7
drivers/video/fbdev/gxt4500.c | 5 +++
drivers/video/fbdev/hyperv_fb.c | 5 +++
drivers/video/fbdev/i740fb.c | 6 +++
drivers/video/fbdev/i810/i810_main.c | 8
drivers/video/fbdev/imsttfb.c | 7
drivers/video/fbdev/intelfb/intelfbdrv.c | 5 +++
drivers/video/fbdev/kyro/fbdev.c | 6 +++
drivers/video/fbdev/matrox/matroxfb_base.c| 5 +++
drivers/video/fbdev/mb862xx/mb862xxfbdrv.c| 5 +++
drivers/video/fbdev/neofb.c | 6 +++
drivers/video/fbdev/nvidia/nvidia.c | 7
drivers/video/fbdev/pm2fb.c | 6 +++
drivers/video/fbdev/pm3fb.c | 6 +++
drivers/video/fbdev/pvr2fb.c | 7
drivers/video/fbdev/riva/fbdev.c | 7
drivers/video/fbdev/s3fb.c| 6 +++
drivers/video/fbdev/savage/savagefb_driver.c | 5 +++
drivers/video/fbdev/sis/sis_main.c| 7
drivers/video/fbdev/skeletonfb.c | 7
drivers/video/fbdev/sm712fb.c | 5 +++
drivers/video/fbdev/sstfb.c | 4 ++
drivers/video/fbdev/sunxvr2500.c | 5 +++
drivers/video/fbdev/sunxvr500.c | 5 +++
drivers/video/fbdev/tdfxfb.c | 6 +++
drivers/video/fbdev/tgafb.c | 6 +++
drivers/video/fbdev/tridentfb.c | 6 +++
drivers/video/fbdev/vermilion/vermilion.c | 7
drivers/video/fbdev/via/via-core.c| 5 +++
drivers/video/fbdev/vt8623fb.c| 6 +++
.../drm/drm_nomodeset.c => video/nomodeset.c} | 12 +++---
include/drm/drm_drv.h | 8 +++-
include/video/nomodeset.h | 8
54 files changed, 319 insertions(+), 14 deletions(-)
rename drivers/{gpu/drm/drm_nomodeset.c => video/nomodeset.c} (63%)
create mode 100644 include/video/nomodeset.h
base-commit: 3aa97a74d622aa26fe79cf4bd819b6a4fd176e90
--
2.38.0
[PATCH 1/2] drm: Move nomodeset kernel parameter to drivers/video
Move the nomodeset kernel parameter to drivers/video to make it
available to non-DRM drivers. Adapt the interface, but keep the DRM
interface drm_firmware_drivers_only() to avoid churn within DRM. The
function should later be inlined into callers.
The parameter disables any DRM graphics driver that would replace a
driver for firmware-provided scanout buffers. It is an option to easily
fallback to basic graphics output if the hardware's native driver is
broken. Moving it to a more prominent location wil make it available
to fbdev as well.
Signed-off-by: Thomas Zimmermann
---
Documentation/admin-guide/kernel-parameters.txt | 2 +-
MAINTAINERS | 2 ++
drivers/gpu/drm/Kconfig | 7 +--
drivers/gpu/drm/Makefile | 1 -
drivers/video/Kconfig| 4
drivers/video/Makefile | 1 +
.../{gpu/drm/drm_nomodeset.c => video/nomodeset.c} | 12 +++-
include/drm/drm_drv.h| 8 +++-
include/video/nomodeset.h| 8
9 files changed, 31 insertions(+), 14 deletions(-)
rename drivers/{gpu/drm/drm_nomodeset.c => video/nomodeset.c} (63%)
create mode 100644 include/video/nomodeset.h
diff --git a/Documentation/admin-guide/kernel-parameters.txt
b/Documentation/admin-guide/kernel-parameters.txt
index a465d5242774a..70178c5f53956 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -3777,7 +3777,7 @@
shutdown the other cpus. Instead use the REBOOT_VECTOR
irq.
- nomodeset Disable kernel modesetting. DRM drivers will not perform
+ nomodeset Disable kernel modesetting. Graphics drivers will not
perform
display-mode changes or accelerated rendering. Only the
system framebuffer will be available for use if this was
set-up by the firmware or boot loader.
diff --git a/MAINTAINERS b/MAINTAINERS
index 0f624e3ef6235..84b008f5aacbc 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6698,8 +6698,10 @@ F: drivers/gpu/drm/drm_aperture.c
F: drivers/gpu/drm/tiny/ofdrm.c
F: drivers/gpu/drm/tiny/simpledrm.c
F: drivers/video/aperture.c
+F: drivers/video/nomodeset.c
F: include/drm/drm_aperture.h
F: include/linux/aperture.h
+F: include/video/nomodeset.h
DRM DRIVER FOR SIS VIDEO CARDS
S: Orphan / Obsolete
diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index f30f99166531f..6b11614aecc5b 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -8,7 +8,6 @@
menuconfig DRM
tristate "Direct Rendering Manager (XFree86 4.1.0 and higher DRI
support)"
depends on (AGP || AGP=n) && !EMULATED_CMPXCHG && HAS_DMA
- select DRM_NOMODESET
select DRM_PANEL_ORIENTATION_QUIRKS
select HDMI
select FB_CMDLINE
@@ -19,6 +18,7 @@ menuconfig DRM
# gallium uses SYS_kcmp for os_same_file_description() to de-duplicate
# device and dmabuf fd. Let's make sure that is available for our userspace.
select KCMP
+ select VIDEO_NOMODESET
help
Kernel-level support for the Direct Rendering Infrastructure (DRI)
introduced in XFree86 4.0. If you say Y here, you need to select
@@ -515,11 +515,6 @@ config DRM_EXPORT_FOR_TESTS
config DRM_PANEL_ORIENTATION_QUIRKS
tristate
-# Separate option because nomodeset parameter is global and expected built-in
-config DRM_NOMODESET
- bool
- default n
-
config DRM_LIB_RANDOM
bool
default n
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index c44a54cadb618..f92cd78927110 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -72,7 +72,6 @@ drm-$(CONFIG_DRM_PRIVACY_SCREEN) += \
drm_privacy_screen_x86.o
obj-$(CONFIG_DRM) += drm.o
-obj-$(CONFIG_DRM_NOMODESET) += drm_nomodeset.o
obj-$(CONFIG_DRM_PANEL_ORIENTATION_QUIRKS) += drm_panel_orientation_quirks.o
#
diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
index 0587e21abad94..6d2fde6c5d11a 100644
--- a/drivers/video/Kconfig
+++ b/drivers/video/Kconfig
@@ -11,6 +11,10 @@ config APERTURE_HELPERS
Support tracking and hand-over of aperture ownership. Required
by graphics drivers for firmware-provided framebuffers.
+config VIDEO_NOMODESET
+ bool
+ default n
+
if HAS_IOMEM
config HAVE_FB_ATMEL
diff --git a/drivers/video/Makefile b/drivers/video/Makefile
index 5bb6b452cc83a..a50eb528ed3c3 100644
--- a/drivers/video/Makefile
+++ b/drivers/video/Makefile
@@ -2,6 +2,7 @@
obj-$(CONFIG_APERTURE_HELPERS)+= aperture.o
obj-$(CONFIG_VGASTATE)+= vgastate.o
+obj-$(CONFIG_VIDEO_NOMODESET) += nomodeset.o
obj-$(CONFIG_HDMI)+= hdmi.
Re: [igt-dev] Must-Pass Test Suite for KMS drivers
Hi Am 07.11.22 um 11:26 schrieb Daniel Vetter: On Mon, 7 Nov 2022 at 10:43, Thomas Zimmermann wrote: Hi Am 07.11.22 um 10:30 schrieb Maxime Ripard: Hi Thomas, On Fri, Oct 28, 2022 at 09:31:38AM +0200, Thomas Zimmermann wrote: Am 24.10.22 um 14:43 schrieb max...@cerno.tech: I've discussing the idea for the past year to add an IGT test suite that all well-behaved KMS drivers must pass. The main idea behind it comes from v4l2-compliance and cec-compliance, that are being used to validate that the drivers are sane. We should probably start building up the test list, and eventually mandate that all tests pass for all the new KMS drivers we would merge in the kernel, and be run by KCi or similar. I did a first pass to create a draft of such a test-suite, which would contain: igt@core_auth@basic-auth igt@core_auth@getclient-master-drop igt@core_auth@getclient-simple igt@core_auth@many-magics igt@core_getclient igt@core_getstats igt@core_getversion igt@core_hotunplug@hotrebind-lateclose igt@core_hotunplug@hotunbind-rebind igt@core_hotunplug@unbind-rebind igt@core_setmaster igt@core_setmaster_vs_auth igt@device_reset@unbind-reset-rebind igt@drm_read igt@dumb_buffer igt@fbdev Maybe we make this test optional? At least sprd decided to not support fbdev at all. I'm not sure we need to make that test optional, or at least, we should run it all the time, but skip it if there's no fbdev emulation, and not report it as a failure? Sure. I just meant that fbdev support shouldn't be a blocker. If there, it should work of course. Not supporting fbdev looks more like an accident than intention here, and maybe a good reason to have these kind of must-past lists. No. Back then, I specifically asked the developer, Kevin Tang IIRC, about fbdev/console support and he replied that he has no intention of adding it. It's the only driver without fbdev, but as we don't have such a requirements AFAIK, I left it at that. Best regards Thomas Eventually, once the i915-ism is worked out of igt well enough for a set of tests at least. The drm/ci effort should help in building up that list (by essentially extracting the common set of tests that everyone passes and graduating that to the must-pass list for kms conformance or whatever we'll call it). -Daniel Best regards Thomas Maxime -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Ivo Totev -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Ivo Totev OpenPGP_signature Description: OpenPGP digital signature
Re: [GIT PULL] treewide: timers: Use timer_shutdown*() before freeing timers
Linus, On Sun, Nov 06 2022 at 22:32, Steven Rostedt wrote: > As discussed here: > > https://lore.kernel.org/all/20221106212427.739928...@goodmis.org/ Please hold off. It's only nits, but tip has documented rules and random pull requests are not making them go away. Thanks, tglx
Re: [PATCH v2 20/65] clk: wm831x: clkout: Add a determine_rate hook
On Fri, Nov 04, 2022 at 02:17:37PM +0100, Maxime Ripard wrote: > The WM381x "clkout" clock implements a mux with a set_parent hook, > but doesn't provide a determine_rate implementation. > > This is a bit odd, since set_parent() is there to, as its name implies, > change the parent of a clock. However, the most likely candidate to > trigger that parent change is a call to clk_set_rate(), with > determine_rate() figuring out which parent is the best suited for a > given rate. > > The other trigger would be a call to clk_set_parent(), but it's far less > used, and it doesn't look like there's any obvious user for that clock. > > So, the set_parent hook is effectively unused, possibly because of an > oversight. However, it could also be an explicit decision by the > original author to avoid any reparenting but through an explicit call to > clk_set_parent(). > > The latter case would be equivalent to setting the flag > CLK_SET_RATE_NO_REPARENT, together with setting our determine_rate hook > to __clk_mux_determine_rate(). Indeed, if no determine_rate > implementation is provided, clk_round_rate() (through > clk_core_round_rate_nolock()) will call itself on the parent if > CLK_SET_RATE_PARENT is set, and will not change the clock rate > otherwise. __clk_mux_determine_rate() has the exact same behavior when > CLK_SET_RATE_NO_REPARENT is set. > > And if it was an oversight, then we are at least explicit about our > behavior now and it can be further refined down the line. > Yeah I don't think there would be anything wrong with this clock changing parents on a rate change, but as you say this can be refined down the line if someone needs the behaviour. It's an older part so probably better to stick roughly to the current behaviour for now. Acked-by: Charles Keepax Thanks, Charles
Re: [PATCH v3 04/12] drm/mediatek: extract common functions from the mtk hdmi driver
Il 04/11/22 15:09, Guillaume Ranquet ha scritto: Create a common "framework" that can be used to add support for different hdmi IPs within the mediatek range of products. Signed-off-by: Guillaume Ranquet --- drivers/gpu/drm/mediatek/Makefile | 3 +- drivers/gpu/drm/mediatek/mtk_hdmi.c| 620 ++--- drivers/gpu/drm/mediatek/mtk_hdmi.h| 16 + drivers/gpu/drm/mediatek/mtk_hdmi_common.c | 433 drivers/gpu/drm/mediatek/mtk_hdmi_common.h | 221 ++ 5 files changed, 704 insertions(+), 589 deletions(-) ..snip.. diff --git a/drivers/gpu/drm/mediatek/mtk_hdmi_common.c b/drivers/gpu/drm/mediatek/mtk_hdmi_common.c new file mode 100644 index ..3f08d37b1af0 --- /dev/null +++ b/drivers/gpu/drm/mediatek/mtk_hdmi_common.c ..snip.. + +int mtk_hdmi_setup_avi_infoframe(struct mtk_hdmi *hdmi, u8 *buffer, size_t bufsz, +struct drm_display_mode *mode) warning: no previous prototype for ‘mtk_hdmi_setup_avi_infoframe’ [-Wmissing-prototypes] Please fix. Thanks, Angelo
Re: [PATCH v3 06/12] drm/mediatek: hdmi: add frame_colorimetry flag
Il 04/11/22 15:09, Guillaume Ranquet ha scritto:
Add a flag to indicate support for frame colorimetry.
Signed-off-by: Guillaume Ranquet
---
drivers/gpu/drm/mediatek/mtk_hdmi_common.c | 11 +++
drivers/gpu/drm/mediatek/mtk_hdmi_common.h | 1 +
2 files changed, 12 insertions(+)
diff --git a/drivers/gpu/drm/mediatek/mtk_hdmi_common.c
b/drivers/gpu/drm/mediatek/mtk_hdmi_common.c
index 3635ca66817b..933c51b5f6d7 100644
--- a/drivers/gpu/drm/mediatek/mtk_hdmi_common.c
+++ b/drivers/gpu/drm/mediatek/mtk_hdmi_common.c
@@ -120,6 +120,17 @@ int mtk_hdmi_setup_avi_infoframe(struct mtk_hdmi *hdmi, u8
*buffer, size_t bufsz
return err;
}
+ if (hdmi->conf->has_frame_colorimetry) {
+ frame.colorimetry = hdmi->colorimtery;
Typo: s/colorimtery/colorimetry/g
...also, I don't see this being really used? In hdmi-v2 you're setting this
param
to HDMI_COLORIMETRY_NONE and quantization ranges to always the same default
value.
I wonder if this is really needed at this point.
Regards,
Angelo
Re: [PATCH v3 08/12] drm/mediatek: hdmi: v2: add audio support
Il 04/11/22 15:09, Guillaume Ranquet ha scritto:
Add HDMI audio support for v2
Signed-off-by: Guillaume Ranquet
---
drivers/gpu/drm/mediatek/mtk_hdmi_common.c | 1 +
drivers/gpu/drm/mediatek/mtk_hdmi_ddc_v2.c | 2 +-
drivers/gpu/drm/mediatek/mtk_hdmi_v2.c | 213 +
drivers/gpu/drm/mediatek/mtk_hdmi_v2.h | 2 +
4 files changed, 217 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/mediatek/mtk_hdmi_common.c
b/drivers/gpu/drm/mediatek/mtk_hdmi_common.c
index e43c938a9aa5..1ea91f8bb6c7 100644
--- a/drivers/gpu/drm/mediatek/mtk_hdmi_common.c
+++ b/drivers/gpu/drm/mediatek/mtk_hdmi_common.c
@@ -386,6 +386,7 @@ static const struct mtk_hdmi_conf mtk_hdmi_conf_v2 = {
.mtk_hdmi_output_init = mtk_hdmi_output_init_v2,
.mtk_hdmi_clk_disable = mtk_hdmi_clk_disable_v2,
.mtk_hdmi_clk_enable = mtk_hdmi_clk_enable_v2,
+ .set_hdmi_codec_pdata = set_hdmi_codec_pdata_v2,
Keep naming consistent please:
.mtk_hdmi_set_codec_pdata = mtk_hdmi_set_codec_pdata_v2,
.mtk_hdmi_clock_names = mtk_hdmi_clk_names_v2,
.num_clocks = MTK_HDMIV2_CLK_COUNT,
};
diff --git a/drivers/gpu/drm/mediatek/mtk_hdmi_ddc_v2.c
b/drivers/gpu/drm/mediatek/mtk_hdmi_ddc_v2.c
index 61696d255e51..26456802a5c4 100644
--- a/drivers/gpu/drm/mediatek/mtk_hdmi_ddc_v2.c
+++ b/drivers/gpu/drm/mediatek/mtk_hdmi_ddc_v2.c
@@ -309,7 +309,7 @@ static int mtk_hdmi_ddc_probe(struct platform_device *pdev)
ddc->regs = device_node_to_regmap(hdmi);
of_node_put(hdmi);
if (IS_ERR(ddc->regs))
- return dev_err_probe(dev, PTR_ERR(ddc->regs), "Unable to get
mt8195-hdmi syscon");
+ return dev_err_probe(dev, PTR_ERR(ddc->regs), "Unable to get hdmi
syscon");
You might as well do the rename in the commit that actually introduces this
file,
since you're doing that in the same series.
Please do so.
ddc->clk = devm_clk_get_enabled(dev, "ddc");
if (IS_ERR(ddc->clk))
diff --git a/drivers/gpu/drm/mediatek/mtk_hdmi_v2.c
b/drivers/gpu/drm/mediatek/mtk_hdmi_v2.c
index e8457429964d..b391b22fa9f5 100644
--- a/drivers/gpu/drm/mediatek/mtk_hdmi_v2.c
+++ b/drivers/gpu/drm/mediatek/mtk_hdmi_v2.c
@@ -211,6 +211,26 @@ static void mtk_hdmi_hw_vid_black(struct mtk_hdmi *hdmi,
bool black)
mtk_hdmi_mask(hdmi, TOP_VMUTE_CFG1, 0, REG_VMUTE_EN);
}
+static void mtk_hdmi_hw_aud_mute(struct mtk_hdmi *hdmi)
+{
+ u32 val;
+
+ val = mtk_hdmi_read(hdmi, AIP_CTRL, &val);
+
+ if (val & DSD_EN)
+ mtk_hdmi_mask(hdmi, AIP_TXCTRL,
+ DSD_MUTE_DATA | AUD_MUTE_FIFO_EN,
I think I already gave you some advice to shorten this in an earlier
series version.
Besides, you can also use the FIELD_PREP() macro here, and please use it.
P.S.:
val = FIELD_PREP(... something AUD_MUTE_FIFO_EN);
if (val & DSD_EN)
val |= FIELD_PREP( ... something DSD_MUTE_DATA)
mtk_hdmi_mask(hdmi, AIP_TXCTRL, val, val);
+ DSD_MUTE_DATA | AUD_MUTE_FIFO_EN);
+ else
+ mtk_hdmi_mask(hdmi, AIP_TXCTRL, AUD_MUTE_FIFO_EN,
+ AUD_MUTE_FIFO_EN);
+}
+
+static void mtk_hdmi_hw_aud_unmute(struct mtk_hdmi *hdmi)
+{
+ mtk_hdmi_mask(hdmi, AIP_TXCTRL, AUD_MUTE_DIS, AUD_MUTE_FIFO_EN);
+}
+
static void mtk_hdmi_hw_reset(struct mtk_hdmi *hdmi)
{
mtk_hdmi_mask(hdmi, HDMITX_CONFIG, 0x0, HDMITX_SW_RSTB);
@@ -889,6 +909,7 @@ static void mtk_hdmi_audio_reset(struct mtk_hdmi *hdmi,
bool rst)
..snip..
@@ -935,6 +957,28 @@ static void mtk_hdmi_reset_colorspace_setting(struct
mtk_hdmi *hdmi)
hdmi->ycc_quantization_range = HDMI_YCC_QUANTIZATION_RANGE_LIMITED;
}
+static void mtk_hdmi_audio_enable(struct mtk_hdmi *hdmi)
These two functions are used only in one function (each)... so you don't
need them. Just move the two lines in mtk_hdmi_audio_startup() ...
+{
+ mtk_hdmi_aud_enable_packet(hdmi, true);
+ hdmi->audio_enable = true;
+}
+
+static void mtk_hdmi_audio_disable(struct mtk_hdmi *hdmi)
+{
... and in mtk_hdmi_audio_shutdown().
+ mtk_hdmi_aud_enable_packet(hdmi, false);
+ hdmi->audio_enable = false;
+}
+
+static void mtk_hdmi_audio_set_param(struct mtk_hdmi *hdmi,
+struct hdmi_audio_param *param)
+{
+ if (!hdmi->audio_enable)
+ return;
+
+ memcpy(&hdmi->aud_param, param, sizeof(*param));
+ mtk_hdmi_aud_output_config(hdmi, &hdmi->mode);
+}
+
static void mtk_hdmi_change_video_resolution(struct mtk_hdmi *hdmi)
{
bool is_over_340M = false;
..snip..
+
+static int mtk_hdmi_audio_mute(struct device *dev, void *data, bool enable,
+ int direction)
+{
+ struct mtk_hdmi *hdmi = dev_get_drvdata(dev);
+
+ if (direction != SNDRV_PCM_STREAM_PLAYBACK)
Why aren't you returning an error here?
If this is really supposed to be like that, please add a
Re: [PATCH v2 65/65] clk: Warn if we register a mux without determine_rate
On Fri, Nov 04, 2022 at 02:18:22PM +0100, Maxime Ripard wrote: > The determine_rate hook allows to select the proper parent and its rate > for a given clock configuration. On another hand, set_parent is there to > change the parent of a mux. > > Some clocks provide a set_parent hook but don't implement > determine_rate. In such a case, set_parent is pretty much useless since > the clock framework will always assume the current parent is to be used, > and we will thus never change it. > > This situation can be solved in two ways: > - either we don't need to change the parent, and we thus shouldn't > implement set_parent; > - or we don't want to change the parent, in this case we should set > CLK_SET_RATE_NO_REPARENT; > - or we're missing a determine_rate implementation. > > The latter is probably just an oversight from the driver's author, and > we should thus raise their awareness about the fact that the current > state of the driver is confusing. > > It's not clear at this point how many drivers are affected though, so > let's make it a warning instead of an error for now. > Commit message could probably use updated to make the new state of the chain. Thanks, Charles
Re: [PATCH v6 16/23] drm/probe-helper: Provide a TV get_modes helper
Den 07.11.2022 11.07, skrev Maxime Ripard: > Hi Noralf, > > On Sun, Nov 06, 2022 at 05:59:23PM +0100, Noralf Trønnes wrote: >> >> >> Den 27.10.2022 00.02, skrev Mateusz Kwiatkowski: >>> Hi Maxime, >>> >>> First of all, nice idea with the helper function that can be reused by >>> different >>> drivers. This is neat! >>> >>> But looking at this function, it feels a bit overcomplicated. You're >>> creating >>> the two modes, then checking which one is the default, then set the >>> preferred >>> one and possibly reorder them. Maybe it can be simplified somehow? >>> >>> Although when I tried to refactor it myself, I ended up with something >>> that's >>> not better at all. Maybe it needs to be complicated, after all :( >>> >> >> I also thought that the function was complicated/difficult to read, in >> particular the index stuff at the end, but I also failed in finding a >> "better" solution, just a different one ;) > > I think I like yours better still :) > > Can I bring it into my series, with your authorship and SoB? > Sure, no problem. Noralf.
Re: [PATCH v3 12/12] drm/mediatek: dpi: Add mt8195 hdmi to DPI driver
Il 04/11/22 15:09, Guillaume Ranquet ha scritto:
Add the DPI1 hdmi path support in mtk dpi driver
Signed-off-by: Guillaume Ranquet
---
drivers/gpu/drm/mediatek/mtk_dpi.c | 143 ++--
drivers/gpu/drm/mediatek/mtk_dpi_regs.h | 5 ++
2 files changed, 141 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/mediatek/mtk_dpi.c
b/drivers/gpu/drm/mediatek/mtk_dpi.c
index 508a6d994e83..8052b47042b8 100644
--- a/drivers/gpu/drm/mediatek/mtk_dpi.c
+++ b/drivers/gpu/drm/mediatek/mtk_dpi.c
@@ -14,7 +14,10 @@
#include
#include
#include
+#include
#include
+#include
+#include
#include
@@ -65,10 +68,14 @@ struct mtk_dpi {
struct drm_bridge *next_bridge;
struct drm_connector *connector;
void __iomem *regs;
+ struct reset_control *reset_ctl;
struct device *dev;
struct clk *engine_clk;
+ struct clk *dpi_ck_cg;
struct clk *pixel_clk;
+ struct clk *dpi_sel_clk;
struct clk *tvd_clk;
+ struct clk *hdmi_cg;
I admit that I didn't really check these clocks, but judging by the names,
it is highly possible that one (or more) of them are supposed to be parents
of some others.
The first suspicious ones are dpi_ck_cg and dpi_sel_clk: please check.
I'm also not sure about the hdmi_cg, shouldn't the DPI have a HDMI port in
the graph that you'd declare in devicetree?
Besides... you're doing a lot of work to check if (is_internal_hdmi) for
power up/down paths, but seeing that you're introducing this change after
adding the HDMI driver makes me mostly sure that the internal hdmi that we're
talking about here is the one that is managed by the HDMIV2 driver... and
this means that you should really, really, really rely on connecting inputs
and outputs the right way in the devicetree, as that will most probably make
you able to write practically 0 code to manage power for the DPI... and may
also remove the need of adding the hdmi_cg clock here...
Regards,
Angelo
Re: [PATCH v3 03/12] drm/mediatek: hdmi: use a regmap instead of iomem
Il 04/11/22 15:09, Guillaume Ranquet ha scritto:
To prepare support for newer chips that need to share their address
range with a dedicated ddc driver, use a regmap.
Signed-off-by: Guillaume Ranquet
---
drivers/gpu/drm/mediatek/mtk_hdmi.c | 43 +++--
1 file changed, 13 insertions(+), 30 deletions(-)
diff --git a/drivers/gpu/drm/mediatek/mtk_hdmi.c
b/drivers/gpu/drm/mediatek/mtk_hdmi.c
index 4c80b6896dc3..9b02b30a193a 100644
--- a/drivers/gpu/drm/mediatek/mtk_hdmi.c
+++ b/drivers/gpu/drm/mediatek/mtk_hdmi.c
@@ -171,7 +171,7 @@ struct mtk_hdmi {
u32 ibias_up;
struct regmap *sys_regmap;
unsigned int sys_offset;
- void __iomem *regs;
+ struct regmap *regs;
enum hdmi_colorspace csp;
struct hdmi_audio_param aud_param;
bool audio_enable;
@@ -187,44 +187,29 @@ static inline struct mtk_hdmi
*hdmi_ctx_from_bridge(struct drm_bridge *b)
return container_of(b, struct mtk_hdmi, bridge);
}
-static u32 mtk_hdmi_read(struct mtk_hdmi *hdmi, u32 offset)
+static int mtk_hdmi_read(struct mtk_hdmi *hdmi, u32 offset, u32 *val)
{
- return readl(hdmi->regs + offset);
+ return regmap_read(hdmi->regs, offset, val);
}
static void mtk_hdmi_write(struct mtk_hdmi *hdmi, u32 offset, u32 val)
{
- writel(val, hdmi->regs + offset);
+ regmap_write(hdmi->regs, offset, val);
}
static void mtk_hdmi_clear_bits(struct mtk_hdmi *hdmi, u32 offset, u32 bits)
You don't need these functions anymore, as these are now simply wrapping
regmap calls, hence these don't contain any "real" logic anymore.
Please remove them and use the regmap API directly.
Thanks,
Angelo
Re: [PATCH v3 01/12] dt-bindings: phy: mediatek: hdmi-phy: Add mt8195 compatible
Il 04/11/22 15:09, Guillaume Ranquet ha scritto: Add a compatible for the HDMI PHY on MT8195 Acked-by: Krzysztof Kozlowski Signed-off-by: Guillaume Ranquet Ack and R-b tags go after your S-o-b. Apart from that: Reviewed-by: AngeloGioacchino Del Regno
Re: [PATCH v6 16/23] drm/probe-helper: Provide a TV get_modes helper
Den 07.11.2022 11.21, skrev Maxime Ripard: > Hi Noralf, > > I'll leave aside your comments on the code, since we'll use your > implementation. > > On Sun, Nov 06, 2022 at 05:33:48PM +0100, Noralf Trønnes wrote: >> Den 26.10.2022 17.33, skrev max...@cerno.tech: >>> + >>> + if (cmdline->tv_mode_specified) >>> + default_mode = cmdline->tv_mode; >> >> I realised that we don't verify tv_mode coming from the command line, >> not here and not in the reset helper. Should we do that? A driver should >> be programmed defensively to handle an illegal/unsupported value, but it >> doesn't feel right to allow an illegal enum value coming through the >> core/helpers. > > I don't think we can end up with an invalid value here if it's been > specified. > > We parse the command line through drm_mode_parse_tv_mode() (introduced > in patch 13 "drm/modes: Introduce the tv_mode property as a command-line > option") that will pick the tv mode part of the command line, and call > drm_get_tv_mode_from_name() using it. > > drm_get_tv_mode_from_name() will return a EINVAL if it's not a value we > expect, and mode->tv_mode is only set on success. And AFAIK, there's no > other path that will set tv_mode. > I see now that illegal was the wrong word, but if the driver only supports ntsc, the user can still set tv_mode=PAL right? And that's an unsupported value that the driver can't fulfill, so it errors out. But then again maybe that's just how it is, we can also set a display mode that the driver can't handle, so this is no different in that respect. Yeah, my argument lost some of its strength here :) Noralf.
Re: [PATCH v1 3/5] arm64: dts: qcom: sm8450-hdk: enable display hardware
On 07/11/2022 11:46, Konrad Dybcio wrote:
>
>
> On 06/11/2022 05:30, Bjorn Andersson wrote:
>> On Fri, Nov 04, 2022 at 04:13:56PM +0300, Dmitry Baryshkov wrote:
>>> Enable MDSS/DPU/DSI0 on SM8450-HDK device. Note, there is no panel
>>> configuration (yet).
>>>
>>> Signed-off-by: Dmitry Baryshkov
>>> ---
>>> arch/arm64/boot/dts/qcom/sm8450-hdk.dts | 18 ++
>>> 1 file changed, 18 insertions(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/qcom/sm8450-hdk.dts
>>> b/arch/arm64/boot/dts/qcom/sm8450-hdk.dts
>>> index 38ccd44620d0..e1a4cf1ee51d 100644
>>> --- a/arch/arm64/boot/dts/qcom/sm8450-hdk.dts
>>> +++ b/arch/arm64/boot/dts/qcom/sm8450-hdk.dts
>>> @@ -442,3 +442,21 @@ &usb_1_qmpphy {
>>> vdda-phy-supply = <&vreg_l6b_1p2>;
>>> vdda-pll-supply = <&vreg_l1b_0p91>;
>>> };
>>> +
>>> +&mdss {
>>> + status = "okay";
>>> +};
>>> +
>>> +&mdss_mdp {
>>> + status = "okay";
>>> +};
>>> +
>>> +&dsi0 {
>>
>> Please prefix the labels with "mdss_" so that you can keep them sorted
>> alphabetically.
> Why such a change all of a sudden? Only downstream (and sc7280 upstream)
> has mdss_ prefixes for dsi.
For keeping the nodes together - this makes review of code and patches
easier.
> Plain 'dsiN' is more generic.
And why the label should be generic? Label should be useful and
descriptive, although not too much, so mdss_dsi still fits in reasonable
choice.
Best regards,
Krzysztof
Re: [PATCH v1 3/5] arm64: dts: qcom: sm8450-hdk: enable display hardware
On 07/11/2022 12:36, Krzysztof Kozlowski wrote:
On 07/11/2022 11:46, Konrad Dybcio wrote:
On 06/11/2022 05:30, Bjorn Andersson wrote:
On Fri, Nov 04, 2022 at 04:13:56PM +0300, Dmitry Baryshkov wrote:
Enable MDSS/DPU/DSI0 on SM8450-HDK device. Note, there is no panel
configuration (yet).
Signed-off-by: Dmitry Baryshkov
---
arch/arm64/boot/dts/qcom/sm8450-hdk.dts | 18 ++
1 file changed, 18 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/sm8450-hdk.dts
b/arch/arm64/boot/dts/qcom/sm8450-hdk.dts
index 38ccd44620d0..e1a4cf1ee51d 100644
--- a/arch/arm64/boot/dts/qcom/sm8450-hdk.dts
+++ b/arch/arm64/boot/dts/qcom/sm8450-hdk.dts
@@ -442,3 +442,21 @@ &usb_1_qmpphy {
vdda-phy-supply = <&vreg_l6b_1p2>;
vdda-pll-supply = <&vreg_l1b_0p91>;
};
+
+&mdss {
+ status = "okay";
+};
+
+&mdss_mdp {
+ status = "okay";
+};
+
+&dsi0 {
Please prefix the labels with "mdss_" so that you can keep them sorted
alphabetically.
Why such a change all of a sudden? Only downstream (and sc7280 upstream)
has mdss_ prefixes for dsi.
For keeping the nodes together - this makes review of code and patches
easier.
Ok, I can see the reasoning.
Plain 'dsiN' is more generic.
And why the label should be generic? Label should be useful and
descriptive, although not too much, so mdss_dsi still fits in reasonable
choice.
I was under the impression that it should be. But you're right.
Konrad
Best regards,
Krzysztof
Re: [PATCH v5] media: mediatek: vcodec: Add to support VP9 inner racing mode
Hi Mingjia,
Thanks for your patch.
Some comments need to get your feedback.
On Tue, 2022-10-25 at 09:46 +0800, Mingjia Zhang wrote:
> Enable VP9 inner racing mode
> We send lat trans buffer to the core when trigger lat to work,
> instead of waiting for the lat decode done.
> It can be reduce decoder latency.
>
> Signed-off-by: Mingjia Zhang
> ---
> Changes from v3:
>
> - CTS/GTS test pass
> - Fluster result: Ran 275/303 tests successfully
>
> Changes from v2:
>
> - CTS/GTS test pass
> - Fluster result: Ran 240/303 tests successfully
>
> Changes from v1:
>
> - CTS/GTS test pass
> ---
> .../vcodec/vdec/vdec_vp9_req_lat_if.c | 85 ++---
> --
> 1 file changed, 47 insertions(+), 38 deletions(-)
>
> diff --git
> a/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c
> b/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c
> index 81de876d51267..1b39119c89951 100644
> ---
> a/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c
> +++
> b/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c
> @@ -436,6 +436,7 @@ struct vdec_vp9_slice_ref {
> * @frame_ctx: 4 frame context according to VP9 Spec
> * @frame_ctx_helper:4 frame context according to newest
> kernel spec
> * @dirty: state of each frame context
> + * @local_vsi: local instance vsi information
> * @init_vsi:vsi used for initialized VP9 instance
> * @vsi: vsi used for decoding/flush ...
> * @core_vsi:vsi used for Core stage
> @@ -482,6 +483,8 @@ struct vdec_vp9_slice_instance {
> struct v4l2_vp9_frame_context frame_ctx_helper;
> unsigned char dirty[4];
>
> + struct vdec_vp9_slice_vsi local_vsi;
> +
> /* MicroP vsi */
> union {
> struct vdec_vp9_slice_init_vsi *init_vsi;
> @@ -1616,16 +1619,10 @@ static int
> vdec_vp9_slice_update_single(struct vdec_vp9_slice_instance *instance
> }
>
> static int vdec_vp9_slice_update_lat(struct vdec_vp9_slice_instance
> *instance,
> - struct vdec_lat_buf *lat_buf,
> - struct vdec_vp9_slice_pfc *pfc)
> + struct vdec_vp9_slice_vsi *vsi)
> {
> - struct vdec_vp9_slice_vsi *vsi;
> -
> - vsi = &pfc->vsi;
> - memcpy(&pfc->state[0], &vsi->state, sizeof(vsi->state));
> -
> mtk_vcodec_debug(instance, "Frame %u LAT CRC 0x%08x %lx %lx\n",
> - pfc->seq, vsi->state.crc[0],
> + (instance->seq - 1), vsi->state.crc[0],
>(unsigned long)vsi->trans.dma_addr,
>(unsigned long)vsi->trans.dma_addr_end);
>
> @@ -2090,6 +2087,13 @@ static int vdec_vp9_slice_lat_decode(void
> *h_vdec, struct mtk_vcodec_mem *bs,
> return ret;
> }
>
> + if (IS_VDEC_INNER_RACING(ctx->dev->dec_capability)) {
> + vdec_vp9_slice_vsi_from_remote(vsi, instance->vsi, 0);
> + memcpy(&instance->local_vsi, vsi, sizeof(*vsi));
> + vdec_msg_queue_qbuf(&ctx->dev->msg_queue_core_ctx,
> lat_buf);
> + vsi = &instance->local_vsi;
> + }
> +
> if (instance->irq) {
> ret = mtk_vcodec_wait_for_done_ctx(ctx, MTK_INST_IRQ_
> RECEIVED,
> WAIT_INTR_TIMEOUT_MS
> , MTK_VDEC_LAT0);
> @@ -2102,22 +2106,25 @@ static int vdec_vp9_slice_lat_decode(void
> *h_vdec, struct mtk_vcodec_mem *bs,
> }
>
> vdec_vp9_slice_vsi_from_remote(vsi, instance->vsi, 0);
> - ret = vdec_vp9_slice_update_lat(instance, lat_buf, pfc);
> + ret = vdec_vp9_slice_update_lat(instance, vsi);
>
> - /* LAT trans full, no more UBE or decode timeout */
> - if (ret) {
> - mtk_vcodec_err(instance, "VP9 decode error: %d\n",
> ret);
> - return ret;
> - }
> + if (!IS_VDEC_INNER_RACING(ctx->dev->dec_capability))
> + /* LAT trans full, no more UBE or decode timeout */
> + if (ret) {
> + mtk_vcodec_err(instance, "frame[%d] decode
> error: %d\n",
> +ret, (instance->seq - 1));
> + return ret;
> + }
>
If inner racing decode error, how to do? Looks this error condition
only in non inner racing mode.
> - mtk_vcodec_debug(instance, "lat dma addr: 0x%lx 0x%lx\n",
> - (unsigned long)pfc->vsi.trans.dma_addr,
> - (unsigned long)pfc->vsi.trans.dma_addr_end);
>
> - vdec_msg_queue_update_ube_wptr(&ctx->msg_queue,
> -vsi->trans.dma_addr_end +
> -ctx-
> >msg_queue.wdma_addr.dma_addr);
> - vdec_msg_queue_qbuf(&ctx->dev->msg_queue_core_ctx, lat_buf);
> + vsi->trans.dma_addr_end += ctx->msg_queue.wdma_addr.dma_addr;
> + vdec_msg_queue_update_ube_wptr(&ctx->msg_queue, vsi-
>
Re: [PATCH] drm/vc4: hdmi: Fix pointer dereference before check
On Mon, Nov 07, 2022 at 09:26:30AM +0100, Maxime Ripard wrote:
> On Wed, Nov 02, 2022 at 12:10:03PM +0100, José Expósito wrote:
> > Hi Maxime,
> >
> > Thanks a lot for looking into the patch.
> >
> > On Wed, Nov 02, 2022 at 10:01:53AM +0100, Maxime Ripard wrote:
> > > Hi,
> > >
> > > On Sat, Oct 29, 2022 at 11:34:13AM +0200, José Expósito wrote:
> > > > Commit 6bed2ea3cb38 ("drm/vc4: hdmi: Reset link on hotplug") introduced
> > > > the vc4_hdmi_reset_link() function. This function dereferences the
> > > > "connector" pointer before checking whether it is NULL or not.
> > > >
> > > > Rework variable assignment to avoid this issue.
> > > >
> > > > Fixes: 6bed2ea3cb38 ("drm/vc4: hdmi: Reset link on hotplug")
> > > > Signed-off-by: José Expósito
> > > > ---
> > > > drivers/gpu/drm/vc4/vc4_hdmi.c | 10 +++---
> > > > 1 file changed, 7 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c
> > > > b/drivers/gpu/drm/vc4/vc4_hdmi.c
> > > > index 4a73fafca51b..07d058b6afb7 100644
> > > > --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> > > > +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> > > > @@ -319,9 +319,9 @@ static int reset_pipe(struct drm_crtc *crtc,
> > > > static int vc4_hdmi_reset_link(struct drm_connector *connector,
> > > >struct drm_modeset_acquire_ctx *ctx)
> > > > {
> > > > - struct drm_device *drm = connector->dev;
> > > > - struct vc4_hdmi *vc4_hdmi = connector_to_vc4_hdmi(connector);
> > > > - struct drm_encoder *encoder = &vc4_hdmi->encoder.base;
> > > > + struct drm_device *drm;
> > > > + struct vc4_hdmi *vc4_hdmi;
> > > > + struct drm_encoder *encoder;
> > > > struct drm_connector_state *conn_state;
> > > > struct drm_crtc_state *crtc_state;
> > > > struct drm_crtc *crtc;
> > > > @@ -332,6 +332,10 @@ static int vc4_hdmi_reset_link(struct
> > > > drm_connector *connector,
> > > > if (!connector)
> > > > return 0;
> > > >
> > > > + drm = connector->dev;
> > > > + vc4_hdmi = connector_to_vc4_hdmi(connector);
> > > > + encoder = &vc4_hdmi->encoder.base;
> > > > +
> > >
> > > I don't think that's right. Connector shouldn't be NULL to begin with,
> > > how did you notice this?
> > >
> > > Maxime
> >
> > This issue was reported by Coverity. At the moment this function is not
> > invoked with a NULL connector by any code path. However, since the NULL
> > check is present, in my opinion, it makes sense to either remove it or
> > make it usefull just in case the preconditions change in the future.
>
> Yeah, it makes sense
>
> I'd ask for a small cosmetic change then, could you add the assignments
> where they are actually needed instead of at the top of the function?
>
> Something like
>
> if (!connector)
> return 0;
Dunno why you want to keep around dead code like that.
I'd just nuke the bogus null check.
>
> +drm = connector->dev;
> ret = drm_modeset_lock(&drm->mode_config.connection_mutex, ctx);
>
> ...
>
> +vc4_hdmi = connector_to_vc4_hdmi(connector);
> if (!vc4_hdmi_supports_scrambling(vc4_hdmi))
>
> ...
>
> Changing the prototype of vc4_hdmi_supports_scrambling to take a struct
> vc4_hdmi pointer would also help, it's much more convenient.
>
> Maxime
--
Ville Syrjälä
Intel
Re: [PATCH v2 43/65] ASoC: tlv320aic32x4: Add a determine_rate hook
On Mon, Nov 07, 2022 at 09:43:22AM +0100, Maxime Ripard wrote: > On Fri, Nov 04, 2022 at 03:59:53PM +, Mark Brown wrote: > > Well, hopefully everyone for whom it's an issue currently will be > > objecting to this version of the change anyway so we'll either know > > where to set the flag or we'll get the whack-a-mole with the series > > being merged? > I'm sorry, I'm not sure what you mean here. The only issue to fix at the > moment is that determine_rate and set_parent aren't coupled, and it led > to issues due to oversight. > I initially added a warning but Stephen wanted to fix all users in that > case and make that an error instead. My suggestion is that instead of doing either of these things it'd be quicker and less error prone to just fix the core to provide the default implementation if nothing more specific is provided. Any issues that causes would already be present with your current series. > If I filled __clk_mux_determine_rate into clocks that weren't using it > before, I would change their behavior. With that flag set, on all users > I add __clk_mux_determine_rate to, the behavior is the same than what we > previously had, so the risk of regressions is minimal, and everything > should keep going like it was? The series does fill in __clk_mux_determine_rate for everything though - if it was just assumed by default the only thing that'd be needed would be adding the flag. signature.asc Description: PGP signature
Re: [PATCH v2 21/65] clk: davinci: da8xx-cfgchip: Add a determine_rate hook
Hi David, On Fri, Nov 04, 2022 at 11:45:17AM -0500, David Lechner wrote: > On 11/4/22 8:17 AM, Maxime Ripard wrote: > > The Davinci DA8xxx cfgchip mux clock implements a mux with a set_parent > > hook, but doesn't provide a determine_rate implementation. > > > > This is a bit odd, since set_parent() is there to, as its name implies, > > change the parent of a clock. However, the most likely candidate to > > trigger that parent change is a call to clk_set_rate(), with > > determine_rate() figuring out which parent is the best suited for a > > given rate. > > > > The other trigger would be a call to clk_set_parent(), but it's far less > > used, and it doesn't look like there's any obvious user for that clock. > > > > So, the set_parent hook is effectively unused, possibly because of an > > oversight. However, it could also be an explicit decision by the > > original author to avoid any reparenting but through an explicit call to > > clk_set_parent(). > > > The parent is defined in the device tree and is not expected to change > at runtime, so if I am understanding the patch correctly, setting the > CLK_SET_RATE_NO_REPARENT flag seems correct. Is that an acked-by/reviewed-by? Thanks! Maxime signature.asc Description: PGP signature
Re: [PATCH] drm: do not call detect for connectors which are forced on
On Fri, Sep 30, 2022 at 02:33:08AM +0300, Laurent Pinchart wrote:
> Hello Michael,
>
> Thank you for the patch. Sorry for the late reply, I wasn't on the CC
> list so I didn't notice it.
>
> On Fri, Aug 26, 2022 at 11:11:21AM +0200, Michael Rodin wrote:
> > "detect" should not be called and its return value shall not be used when a
> > connector is forced as hinted in the commit d50ba256b5f1 ("drm/kms: start
> > adding command line interface using fb.") and the commit 6fe14acd496e
> > ("drm: Document drm_connector_funcs"). One negative side effect of doing
> > this is observed on the RCar3 SoCs which use the dw-hdmi driver. It
> > continues executing drm_helper_hpd_irq_event even if its connector is
> > forced to ON. As consequence drm_helper_hpd_irq_event calls "detect" so the
> > connector status is updated to "disconnected":
> >
> > [ 420.201527] [drm:drm_helper_hpd_irq_event] [CONNECTOR:76:HDMI-A-1]
> > status updated from connected to disconnected
> >
> > This status is corrected by drm_helper_probe_single_connector_modes shortly
> > after this because this function checks if a connector is forced:
> >
> > [ 420.218703] [drm:drm_helper_probe_single_connector_modes]
> > [CONNECTOR:76:HDMI-A-1] status updated from disconnected to connected
> >
> > To avoid similar issues this commit adapts functions which call "detect"
> > so they check if a connector is forced and return the correct status.
> >
> > Fixes: 949f08862d66 ("drm: Make the connector .detect() callback optional")
>
> Is this really the right fixes tag ? The call to .detect() in
> drm_helper_hpd_irq_event() predates that commit.
>
> > Signed-off-by: Michael Rodin
> > ---
> > drivers/gpu/drm/drm_probe_helper.c | 16 ++--
> > 1 file changed, 14 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_probe_helper.c
> > b/drivers/gpu/drm/drm_probe_helper.c
> > index bb427c5a4f1f..1691047d0472 100644
> > --- a/drivers/gpu/drm/drm_probe_helper.c
> > +++ b/drivers/gpu/drm/drm_probe_helper.c
> > @@ -289,7 +289,12 @@ drm_helper_probe_detect_ctx(struct drm_connector
> > *connector, bool force)
> > retry:
> > ret = drm_modeset_lock(&connector->dev->mode_config.connection_mutex,
> > &ctx);
> > if (!ret) {
> > - if (funcs->detect_ctx)
> > + if (connector->force == DRM_FORCE_ON ||
> > + connector->force == DRM_FORCE_ON_DIGITAL)
> > + ret = connector_status_connected;
> > + else if (connector->force == DRM_FORCE_OFF)
> > + ret = connector_status_disconnected;
> > + else if (funcs->detect_ctx)
> > ret = funcs->detect_ctx(connector, &ctx, force);
> > else if (connector->funcs->detect)
> > ret = connector->funcs->detect(connector, force);
> > @@ -340,7 +345,14 @@ drm_helper_probe_detect(struct drm_connector
> > *connector,
> > if (ret)
> > return ret;
> >
> > - if (funcs->detect_ctx)
> > + if (connector->force == DRM_FORCE_ON ||
> > + connector->force == DRM_FORCE_ON_DIGITAL)
> > + ret = connector_status_connected;
> > + else if (connector->force == DRM_FORCE_OFF)
> > + ret = connector_status_disconnected;
> > + else if (funcs->detect_ctx)
> > + ret = funcs->detect_ctx(connector, ctx, force);
> > + else if (funcs->detect_ctx)
> > ret = funcs->detect_ctx(connector, ctx, force);
>
> Those two conditions are identical.
>
> > else if (connector->funcs->detect)
> > ret = connector->funcs->detect(connector, force);
>
> The same logic is used in two places in this patch. Could this be
> factored out to a separate function ? It may even be possible to
> refactor drm_helper_probe_detect() and drm_helper_probe_detect_ctx() to
> share more code between the two functions.
I just had a look, and it doesn't seem trivial. The obvious way would be
to make drm_helper_probe_detect_ctx allocate a context and call
drm_helper_probe_detect. The thing is, drm_helper_probe_detect will call
drm_helper_probe_detect_ctx if its context is NULL.
I guess we could have drm_helper_probe_detect allocate the context
itself if it's null, but handling differently the back-off and freeing
logic is probably going to add more complexity.
I'm not sure it's worth it for simple functions like this.
> This being said, I'm not sure this is the right fix. Beside the i915 and
> nouveau drivers, the only callers of drm_helper_probe_detect() are
> drm_helper_probe_single_connector_modes(), output_poll_execute() and
> check_connector_changed() in this file. The first two functions already
> check connector->force and skip the call to drm_helper_probe_detect() if
> the connector is forced. Only check_connector_changed() is missing that
> check. Wouldn't it be simpler to return false in that function if
> connector->force is set ?
I guess, but the drm_helper_probe_detect documentation states that it
"probe connector st
Re: [PATCH] drm: do not call detect for connectors which are forced on
On Fri, Aug 26, 2022 at 11:11:21AM +0200, Michael Rodin wrote:
> "detect" should not be called and its return value shall not be used when a
> connector is forced as hinted in the commit d50ba256b5f1 ("drm/kms: start
> adding command line interface using fb.") and the commit 6fe14acd496e
> ("drm: Document drm_connector_funcs"). One negative side effect of doing
> this is observed on the RCar3 SoCs which use the dw-hdmi driver. It
> continues executing drm_helper_hpd_irq_event even if its connector is
> forced to ON. As consequence drm_helper_hpd_irq_event calls "detect" so the
> connector status is updated to "disconnected":
>
> [ 420.201527] [drm:drm_helper_hpd_irq_event] [CONNECTOR:76:HDMI-A-1] status
> updated from connected to disconnected
>
> This status is corrected by drm_helper_probe_single_connector_modes shortly
> after this because this function checks if a connector is forced:
>
> [ 420.218703] [drm:drm_helper_probe_single_connector_modes]
> [CONNECTOR:76:HDMI-A-1] status updated from disconnected to connected
>
> To avoid similar issues this commit adapts functions which call "detect"
> so they check if a connector is forced and return the correct status.
>
> Fixes: 949f08862d66 ("drm: Make the connector .detect() callback optional")
> Signed-off-by: Michael Rodin
> ---
> drivers/gpu/drm/drm_probe_helper.c | 16 ++--
> 1 file changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_probe_helper.c
> b/drivers/gpu/drm/drm_probe_helper.c
> index bb427c5a4f1f..1691047d0472 100644
> --- a/drivers/gpu/drm/drm_probe_helper.c
> +++ b/drivers/gpu/drm/drm_probe_helper.c
> @@ -289,7 +289,12 @@ drm_helper_probe_detect_ctx(struct drm_connector
> *connector, bool force)
> retry:
> ret = drm_modeset_lock(&connector->dev->mode_config.connection_mutex,
> &ctx);
> if (!ret) {
> - if (funcs->detect_ctx)
> + if (connector->force == DRM_FORCE_ON ||
> + connector->force == DRM_FORCE_ON_DIGITAL)
> + ret = connector_status_connected;
> + else if (connector->force == DRM_FORCE_OFF)
> + ret = connector_status_disconnected;
> + else if (funcs->detect_ctx)
> ret = funcs->detect_ctx(connector, &ctx, force);
> else if (connector->funcs->detect)
> ret = connector->funcs->detect(connector, force);
Actually, I think this creates a regression on vc4 at least (and
possibly i915?), when the HDMI scrambling is on.
When a monitor is disconnected and then connected again, the mode won't
change by itself, but it will lose its scrambling status. The way we
dealt with it (and documented here:
https://lore.kernel.org/all/20220829134731.213478-9-max...@cerno.tech/)
is to have detect enabling the scrambler again if it detects a
disconnection/connection transition.
If we force the status on all the time, then we will never get called
and we won't have the opportunity to enable the scrambler again.
Maxime
signature.asc
Description: PGP signature
Re: [PATCH v6 16/23] drm/probe-helper: Provide a TV get_modes helper
On Mon, Nov 07, 2022 at 12:29:28PM +0100, Noralf Trønnes wrote: > > > Den 07.11.2022 11.21, skrev Maxime Ripard: > > Hi Noralf, > > > > I'll leave aside your comments on the code, since we'll use your > > implementation. > > > > On Sun, Nov 06, 2022 at 05:33:48PM +0100, Noralf Trønnes wrote: > >> Den 26.10.2022 17.33, skrev max...@cerno.tech: > >>> + > >>> + if (cmdline->tv_mode_specified) > >>> + default_mode = cmdline->tv_mode; > >> > >> I realised that we don't verify tv_mode coming from the command line, > >> not here and not in the reset helper. Should we do that? A driver should > >> be programmed defensively to handle an illegal/unsupported value, but it > >> doesn't feel right to allow an illegal enum value coming through the > >> core/helpers. > > > > I don't think we can end up with an invalid value here if it's been > > specified. > > > > We parse the command line through drm_mode_parse_tv_mode() (introduced > > in patch 13 "drm/modes: Introduce the tv_mode property as a command-line > > option") that will pick the tv mode part of the command line, and call > > drm_get_tv_mode_from_name() using it. > > > > drm_get_tv_mode_from_name() will return a EINVAL if it's not a value we > > expect, and mode->tv_mode is only set on success. And AFAIK, there's no > > other path that will set tv_mode. > > > > I see now that illegal was the wrong word, but if the driver only > supports ntsc, the user can still set tv_mode=PAL right? And that's an > unsupported value that the driver can't fulfill, so it errors out. But > then again maybe that's just how it is, we can also set a display mode > that the driver can't handle, so this is no different in that respect. > Yeah, my argument lost some of its strength here :) I don't think we can handle this better, really. Falling back to NTSC in that case would really be a stretch: it's a different mode, with a different TV mode, etc. It's an even bigger stretch than picking another mode I guess, and like you said we're not doing that if the mode isn't supported Maxime signature.asc Description: PGP signature
[PATCH 0/3] drm/fb-helper: Build fixes for cleanup
Cleaning up the fbdev code produced a number of simple build errors. Fix them. Thomas Zimmermann (3): drm/fbdev: Include drm/hisilicon/hibmc: Include for readl() and writel() drm/fb-helper: Document struct drm_fb_helper.hint_leak_smem_start drivers/gpu/drm/drm_fbdev_generic.c | 1 + drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c | 2 ++ include/drm/drm_fb_helper.h | 7 +++ 3 files changed, 10 insertions(+) -- 2.38.0
[PATCH 2/3] drm/hisilicon/hibmc: Include for readl() and writel()
Include to get readl() and writel() on S390. The error
message is shown below and a bug report is at [1].
drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c:75:15: error: implicit
declaration of function 'readl' [-Werror=implicit-function-declaration]
75 | reg = readl(priv->mmio + HIBMC_DISPLAY_CONTROL_HISILE);
| ^
drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c:80:9: error: implicit
declaration of function 'writel' [-Werror=implicit-function-declaration]
80 | writel(reg, priv->mmio + HIBMC_DISPLAY_CONTROL_HISILE);
| ^~
Reported-by: kernel test robot
Signed-off-by: Thomas Zimmermann
Fixes: 45b64fd9f7ae ("drm/fb-helper: Remove unnecessary include statements")
Cc: Thomas Zimmermann
Cc: Javier Martinez Canillas
Cc: Maarten Lankhorst
Cc: Maxime Ripard
Cc: David Airlie
Cc: Daniel Vetter
Cc: dri-devel@lists.freedesktop.org
Link:
https://lore.kernel.org/dri-devel/202211060608.qrtg8b2e-...@intel.com/T/#u # [1]
---
drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
index c228091fb0e6a..8c6d2ea2a4729 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
@@ -11,6 +11,8 @@
* Jianhua Li
*/
+#include
+
#include
#include
#include
--
2.38.0
[PATCH 3/3] drm/fb-helper: Document struct drm_fb_helper.hint_leak_smem_start
Document the new field smem_start in struct drm_fb_helper and avoid
a compile-time warning. An error message is shown below and the bug
report is at [1].
include/drm/drm_fb_helper.h:204: warning: Function parameter or member
'hint_leak_smem_start' not described in 'drm_fb_helper'
Reported-by: Stephen Rothwell
Signed-off-by: Thomas Zimmermann
Fixes: e7c5c29a9eb1 ("drm/fb-helper: Set flag in struct drm_fb_helper for
leaking physical addresses")
Cc: Thomas Zimmermann
Cc: Javier Martinez Canillas
Cc: Maarten Lankhorst
Cc: Maxime Ripard
Cc: David Airlie
Cc: Daniel Vetter
Cc: dri-devel@lists.freedesktop.org
Link:
https://lore.kernel.org/dri-devel/20221107143858.0253a...@canb.auug.org.au/T/#u
# [1]
---
include/drm/drm_fb_helper.h | 7 +++
1 file changed, 7 insertions(+)
diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
index ecfcd2c56d95a..b111dc7ada78d 100644
--- a/include/drm/drm_fb_helper.h
+++ b/include/drm/drm_fb_helper.h
@@ -200,6 +200,13 @@ struct drm_fb_helper {
*/
int preferred_bpp;
+ /**
+* @hint_leak_smem_start:
+*
+* Hint to the fbdev emulation to store the framebuffer's physical
+* address in struct &fb_info.fix.smem_start. If the hint is unset,
+* the smem_start field should always be cleared to zero.
+*/
bool hint_leak_smem_start;
};
--
2.38.0
[PATCH 1/3] drm/fbdev: Include
Include in fbdev emulation to get vzalloc() and
vfree() on MIPS. The error messages are shown below and bug reports
are at [1] and [2].
drivers/gpu/drm/drm_fbdev_generic.c: In function 'drm_fbdev_cleanup':
drivers/gpu/drm/drm_fbdev_generic.c:63:17: error: implicit declaration of
function 'vfree'; did you mean 'kvfree'? [-Werror=implicit-function-declaration]
63 | vfree(shadow);
| ^
| kvfree
drivers/gpu/drm/drm_fbdev_generic.c: In function 'drm_fbdev_fb_probe':
drivers/gpu/drm/drm_fbdev_generic.c:219:38: error: implicit declaration of
function 'vzalloc'; did you mean 'kvzalloc'?
[-Werror=implicit-function-declaration]
219 | fbi->screen_buffer = vzalloc(fbi->screen_size);
| ^~~
| kvzalloc
drivers/gpu/drm/drm_fbdev_generic.c:219:36: warning: assignment to 'char *'
from 'int' makes pointer from integer without a cast [-Wint-conversion]
219 | fbi->screen_buffer = vzalloc(fbi->screen_size);
Reported-by: kernel test robot
Signed-off-by: Thomas Zimmermann
Fixes: 8ab59da26bc0 ("drm/fb-helper: Move generic fbdev emulation into separate
source file")
Cc: Thomas Zimmermann
Cc: Javier Martinez Canillas
Cc: Maarten Lankhorst
Cc: Maxime Ripard
Cc: David Airlie
Cc: Daniel Vetter
Cc: dri-devel@lists.freedesktop.org
Link:
https://lore.kernel.org/dri-devel/202211060911.5u76gmte-...@intel.com/T/#u # [1]
Link:
https://lore.kernel.org/dri-devel/202211060331.1sod1tar-...@intel.com/T/#u # [2]
---
drivers/gpu/drm/drm_fbdev_generic.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/drm_fbdev_generic.c
b/drivers/gpu/drm/drm_fbdev_generic.c
index 2d6083ad2e3c7..ab86956692795 100644
--- a/drivers/gpu/drm/drm_fbdev_generic.c
+++ b/drivers/gpu/drm/drm_fbdev_generic.c
@@ -1,6 +1,7 @@
// SPDX-License-Identifier: MIT
#include
+#include
#include
#include
--
2.38.0
Re: [RFC PATCH v2 1/3] drivers/accel: define kconfig and register a new major
On Thu, Nov 03, 2022 at 10:39:36PM +0200, Oded Gabbay wrote: > On Thu, Nov 3, 2022 at 3:31 PM Oded Gabbay wrote: > > > > On Thu, Nov 3, 2022 at 2:31 AM Greg Kroah-Hartman > > wrote: > > > > > > On Wed, Nov 02, 2022 at 10:34:03PM +0200, Oded Gabbay wrote: > > > > --- /dev/null > > > > +++ b/drivers/accel/Kconfig > > > > @@ -0,0 +1,24 @@ > > > > +# SPDX-License-Identifier: GPL-2.0-only > > > > +# > > > > +# Compute Acceleration device configuration > > > > +# > > > > +# This framework provides support for compute acceleration devices, > > > > such > > > > +# as, but not limited to, Machine-Learning and Deep-Learning > > > > acceleration > > > > +# devices > > > > +# > > > > +menuconfig ACCEL > > > > + tristate "Compute Acceleration Framework" > > > > + depends on DRM > > > > + help > > > > + Framework for device drivers of compute acceleration devices, > > > > such > > > > + as, but not limited to, Machine-Learning and Deep-Learning > > > > + acceleration devices. > > > > + If you say Y here, you need to select the module that's right > > > > for > > > > + your acceleration device from the list below. > > > > + This framework is integrated with the DRM subsystem as compute > > > > + accelerators and GPUs share a lot in common and can use almost > > > > the > > > > + same infrastructure code. > > > > + Having said that, acceleration devices will have a different > > > > + major number than GPUs, and will be exposed to user-space using > > > > + different device files, called accel/accel* (in /dev, sysfs > > > > + and debugfs) > > > > > > Module name if "M" is chosen? > > Will add > So, unfortunately, the path of doing accel as a kernel module won't > work cleanly (Thanks stanislaw for pointing this out to me). > The reason is the circular dependency between drm and accel. drm calls > accel exported symbols during init and when devices are registering > (all the minor handling), and accel calls drm exported symbols because > I don't want to duplicate the entire drm core code. I really don't think this is the right way to integrate with DRM. Accel should be a layer over top of DRM, not have these wakky co-dependencies. The fact you are running into stuff like this already smells really bad. Jason
Re: [RFC PATCH v2 1/3] drivers/accel: define kconfig and register a new major
On Mon, Nov 7, 2022 at 2:56 PM Jason Gunthorpe wrote: > > On Thu, Nov 03, 2022 at 10:39:36PM +0200, Oded Gabbay wrote: > > On Thu, Nov 3, 2022 at 3:31 PM Oded Gabbay wrote: > > > > > > On Thu, Nov 3, 2022 at 2:31 AM Greg Kroah-Hartman > > > wrote: > > > > > > > > On Wed, Nov 02, 2022 at 10:34:03PM +0200, Oded Gabbay wrote: > > > > > --- /dev/null > > > > > +++ b/drivers/accel/Kconfig > > > > > @@ -0,0 +1,24 @@ > > > > > +# SPDX-License-Identifier: GPL-2.0-only > > > > > +# > > > > > +# Compute Acceleration device configuration > > > > > +# > > > > > +# This framework provides support for compute acceleration devices, > > > > > such > > > > > +# as, but not limited to, Machine-Learning and Deep-Learning > > > > > acceleration > > > > > +# devices > > > > > +# > > > > > +menuconfig ACCEL > > > > > + tristate "Compute Acceleration Framework" > > > > > + depends on DRM > > > > > + help > > > > > + Framework for device drivers of compute acceleration devices, > > > > > such > > > > > + as, but not limited to, Machine-Learning and Deep-Learning > > > > > + acceleration devices. > > > > > + If you say Y here, you need to select the module that's right > > > > > for > > > > > + your acceleration device from the list below. > > > > > + This framework is integrated with the DRM subsystem as compute > > > > > + accelerators and GPUs share a lot in common and can use > > > > > almost the > > > > > + same infrastructure code. > > > > > + Having said that, acceleration devices will have a different > > > > > + major number than GPUs, and will be exposed to user-space > > > > > using > > > > > + different device files, called accel/accel* (in /dev, sysfs > > > > > + and debugfs) > > > > > > > > Module name if "M" is chosen? > > > Will add > > So, unfortunately, the path of doing accel as a kernel module won't > > work cleanly (Thanks stanislaw for pointing this out to me). > > The reason is the circular dependency between drm and accel. drm calls > > accel exported symbols during init and when devices are registering > > (all the minor handling), and accel calls drm exported symbols because > > I don't want to duplicate the entire drm core code. > > I really don't think this is the right way to integrate with > DRM. Accel should be a layer over top of DRM, not have these wakky > co-dependencies. > > The fact you are running into stuff like this already smells really > bad. > > Jason I don't agree with your statement that it should be "a layer over top of DRM". Anything on top of DRM is a device driver. Accel is not a device driver, it is a new type of drm minor / drm driver. Please look at v3 of the patch-set. There I abandoned the idea of having accel as a separate module and instead it is part of drm.ko, as it should be because it is just a new drm minor. The only alternative imo to that is to abandon the idea of reusing drm, and just make an independant accel core code. Oded
Re: [RFC PATCH v2 1/3] drivers/accel: define kconfig and register a new major
On Mon, Nov 07, 2022 at 03:01:08PM +0200, Oded Gabbay wrote: > I don't agree with your statement that it should be "a layer over top of DRM". > Anything on top of DRM is a device driver. > Accel is not a device driver, it is a new type of drm minor / drm driver. Yeah, I still think this is not the right way, you are getting almost nothing from DRM and making everything more complicated in the process. > The only alternative imo to that is to abandon the idea of reusing > drm, and just make an independant accel core code. Not quite really, layer it properly and librarize parts of DRM into things accel can re-use so they are not intimately tied to the DRM struct device notion. IMHO this is much better, because accel has very little need of DRM to manage a struct device/cdev in the first place. Jason
Re: [PATCH v2 0/4] drm/sun4i: dsi: Support the A100/D1 controller variant
On Sun, 6 Nov 2022 23:35:48 -0600, Samuel Holland wrote: > This series adds support for the digital part of the DSI controller > found in the A100 and D1 SoCs (plus T7, which is not supported by > mainline Linux). There are two changes to the hardware integration: > 1) the module clock routes through the TCON TOP, and > 2) the separate I/O domain is removed. > > The actual register interface appears to be the same as before. The > register definitions in the D1 BSP exactly match the A64 BSP. > > [...] Applied to drm/drm-misc (drm-misc-next). Thanks! Maxime
Re: [PATCH v5] media: mediatek: vcodec: Add to support VP9 inner racing mode
Il 07/11/22 12:46, Yunfei Dong (董云飞) ha scritto:
Hi Mingjia,
Thanks for your patch.
Some comments need to get your feedback.
On Tue, 2022-10-25 at 09:46 +0800, Mingjia Zhang wrote:
Enable VP9 inner racing mode
We send lat trans buffer to the core when trigger lat to work,
instead of waiting for the lat decode done.
It can be reduce decoder latency.
Signed-off-by: Mingjia Zhang
---
Changes from v3:
- CTS/GTS test pass
- Fluster result: Ran 275/303 tests successfully
Changes from v2:
- CTS/GTS test pass
- Fluster result: Ran 240/303 tests successfully
Changes from v1:
- CTS/GTS test pass
---
.../vcodec/vdec/vdec_vp9_req_lat_if.c | 85 ++---
--
1 file changed, 47 insertions(+), 38 deletions(-)
diff --git
a/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c
b/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c
index 81de876d51267..1b39119c89951 100644
---
a/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c
+++
b/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c
@@ -436,6 +436,7 @@ struct vdec_vp9_slice_ref {
* @frame_ctx:4 frame context according to VP9 Spec
* @frame_ctx_helper: 4 frame context according to newest
kernel spec
* @dirty:state of each frame context
+ * @local_vsi: local instance vsi information
* @init_vsi: vsi used for initialized VP9 instance
* @vsi: vsi used for decoding/flush ...
* @core_vsi: vsi used for Core stage
@@ -482,6 +483,8 @@ struct vdec_vp9_slice_instance {
struct v4l2_vp9_frame_context frame_ctx_helper;
unsigned char dirty[4];
+ struct vdec_vp9_slice_vsi local_vsi;
+
/* MicroP vsi */
union {
struct vdec_vp9_slice_init_vsi *init_vsi;
@@ -1616,16 +1619,10 @@ static int
vdec_vp9_slice_update_single(struct vdec_vp9_slice_instance *instance
}
static int vdec_vp9_slice_update_lat(struct vdec_vp9_slice_instance
*instance,
-struct vdec_lat_buf *lat_buf,
-struct vdec_vp9_slice_pfc *pfc)
+struct vdec_vp9_slice_vsi *vsi)
{
- struct vdec_vp9_slice_vsi *vsi;
-
- vsi = &pfc->vsi;
- memcpy(&pfc->state[0], &vsi->state, sizeof(vsi->state));
-
mtk_vcodec_debug(instance, "Frame %u LAT CRC 0x%08x %lx %lx\n",
-pfc->seq, vsi->state.crc[0],
+(instance->seq - 1), vsi->state.crc[0],
(unsigned long)vsi->trans.dma_addr,
(unsigned long)vsi->trans.dma_addr_end);
@@ -2090,6 +2087,13 @@ static int vdec_vp9_slice_lat_decode(void
*h_vdec, struct mtk_vcodec_mem *bs,
return ret;
}
+ if (IS_VDEC_INNER_RACING(ctx->dev->dec_capability)) {
+ vdec_vp9_slice_vsi_from_remote(vsi, instance->vsi, 0);
+ memcpy(&instance->local_vsi, vsi, sizeof(*vsi));
+ vdec_msg_queue_qbuf(&ctx->dev->msg_queue_core_ctx,
lat_buf);
+ vsi = &instance->local_vsi;
+ }
+
if (instance->irq) {
ret = mtk_vcodec_wait_for_done_ctx(ctx, MTK_INST_IRQ_
RECEIVED,
WAIT_INTR_TIMEOUT_MS
, MTK_VDEC_LAT0);
@@ -2102,22 +2106,25 @@ static int vdec_vp9_slice_lat_decode(void
*h_vdec, struct mtk_vcodec_mem *bs,
}
vdec_vp9_slice_vsi_from_remote(vsi, instance->vsi, 0);
- ret = vdec_vp9_slice_update_lat(instance, lat_buf, pfc);
+ ret = vdec_vp9_slice_update_lat(instance, vsi);
- /* LAT trans full, no more UBE or decode timeout */
- if (ret) {
- mtk_vcodec_err(instance, "VP9 decode error: %d\n",
ret);
- return ret;
- }
+ if (!IS_VDEC_INNER_RACING(ctx->dev->dec_capability))
+ /* LAT trans full, no more UBE or decode timeout */
+ if (ret) {
+ mtk_vcodec_err(instance, "frame[%d] decode
error: %d\n",
+ ret, (instance->seq - 1));
+ return ret;
+ }
If inner racing decode error, how to do? Looks this error condition
only in non inner racing mode.
- mtk_vcodec_debug(instance, "lat dma addr: 0x%lx 0x%lx\n",
-(unsigned long)pfc->vsi.trans.dma_addr,
-(unsigned long)pfc->vsi.trans.dma_addr_end);
- vdec_msg_queue_update_ube_wptr(&ctx->msg_queue,
- vsi->trans.dma_addr_end +
- ctx-
msg_queue.wdma_addr.dma_addr);
- vdec_msg_queue_qbuf(&ctx->dev->msg_queue_core_ctx, lat_buf);
+ vsi->trans.dma_addr_end += ctx->msg_queue.wdma_addr.dma_addr;
+ vdec_msg_queue_update_ube_wptr(&ctx->msg_queue, vsi-
trans.dma_addr_end);
+ if (!IS_VDEC_INNER_RACING(ctx->dev->dec_capability))
+ vdec_msg_queue_q
Re: [PATCH 04/10] vfio: Move storage of allow_unsafe_interrupts to vfio_main.c
On Mon, Oct 31, 2022 at 04:45:26PM -0600, Alex Williamson wrote:
> > It is one idea, it depends how literal you want to be on "module
> > parameters are ABI". IMHO it is a weak form of ABI and the need of
> > this paramter in particular is not that common in modern times, AFAIK.
> >
> > So perhaps we just also expose it through vfio.ko and expect people to
> > migrate. That would give a window were both options are available.
>
> That might be best. Ultimately this is an opt-out of a feature that
> has security implications, so I'd rather error on the side of requiring
> the user to re-assert that opt-out. It seems the potential good in
> eliminating stale or unnecessary options outweighs any weak claims of
> preserving an ABI for a module that's no longer in service.
Ok, lets do this
--- a/drivers/vfio/vfio_main.c
+++ b/drivers/vfio/vfio_main.c
@@ -55,6 +55,11 @@ static struct vfio {
bool vfio_allow_unsafe_interrupts;
EXPORT_SYMBOL_GPL(vfio_allow_unsafe_interrupts);
+module_param_named(allow_unsafe_interrupts,
+ vfio_allow_unsafe_interrupts, bool, S_IRUGO | S_IWUSR);
+MODULE_PARM_DESC(allow_unsafe_interrupts,
+"Enable VFIO IOMMU support for on platforms without interrupt
remapping support.");
+
static DEFINE_XARRAY(vfio_device_set_xa);
static const struct file_operations vfio_group_fops;
> However, I'd question whether vfio is the right place for that new
> module option. As proposed, vfio is only passing it through to
> iommufd, where an error related to lack of the hardware feature is
> masked behind an -EPERM by the time it gets back to vfio, making any
> sort of advisory to the user about the module option convoluted. It
> seems like iommufd should own the option to opt-out universally, not
> just through the vfio use case. Thanks,
My thinking is this option shouldn't exist at all in other iommufd
users. eg I don't see value in VDPA supporting it.
So, let's wait and see if a need arises first. I'm reluctant to add
options to disable kernel security without really good reasons.
Jason
Re: [PATCH] drm/panfrost: Remove type name from internal struct again
Reviewed-by: Alyssa Rosenzweig
On Thu, Nov 03, 2022 at 11:40:36AM +, Steven Price wrote:
> Commit 72655fb942c1 ("drm/panfrost: replace endian-specific types with
> native ones") accidentally reverted part of the parent commit
> 7228d9d79248 ("drm/panfrost: Remove type name from internal structs")
> leading to the situation that the Panfrost UAPI header still doesn't
> compile correctly in C++.
>
> Revert the accidental revert and pass me a brown paper bag.
>
> Reported-by: Alyssa Rosenzweig
> Fixes: 72655fb942c1 ("drm/panfrost: replace endian-specific types with native
> ones")
> Signed-off-by: Steven Price
> ---
> include/uapi/drm/panfrost_drm.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/uapi/drm/panfrost_drm.h b/include/uapi/drm/panfrost_drm.h
> index 6f93c915cc88..9f231d40a146 100644
> --- a/include/uapi/drm/panfrost_drm.h
> +++ b/include/uapi/drm/panfrost_drm.h
> @@ -254,7 +254,7 @@ struct panfrost_dump_object_header {
> __u64 nbos;
> } reghdr;
>
> - struct pan_bomap_hdr {
> + struct {
> __u32 valid;
> __u64 iova;
> __u32 data[2];
> --
> 2.34.1
>
Re: [RFC PATCH v2 1/3] drivers/accel: define kconfig and register a new major
Hi On Mon, Nov 07, 2022 at 09:10:36AM -0400, Jason Gunthorpe wrote: > On Mon, Nov 07, 2022 at 03:01:08PM +0200, Oded Gabbay wrote: > > I don't agree with your statement that it should be "a layer over top of > > DRM". > > Anything on top of DRM is a device driver. > > Accel is not a device driver, it is a new type of drm minor / drm driver. > > Yeah, I still think this is not the right way, you are getting almost > nothing from DRM and making everything more complicated in the > process. > > > The only alternative imo to that is to abandon the idea of reusing > > drm, and just make an independant accel core code. > > Not quite really, layer it properly and librarize parts of DRM into > things accel can re-use so they are not intimately tied to the DRM > struct device notion. What do you mean by "struct device notion" ? struct drm_devce ? Regards Stanislaw
Re: [PATCH v6 14/23] drm/modes: Properly generate a drm_display_mode from a named mode
On Sat, Nov 05, 2022 at 06:50:30PM +0100, Noralf Trønnes wrote:
> Den 26.10.2022 17.33, skrev max...@cerno.tech:
> > The framework will get the drm_display_mode from the drm_cmdline_mode it
> > got by parsing the video command line argument by calling
> > drm_connector_pick_cmdline_mode().
> >
> > The heavy lifting will then be done by the
> > drm_mode_create_from_cmdline_mode()
> > function.
> >
> > In the case of the named modes though, there's no real code to make that
> > translation and we rely on the drivers to guess which actual display mode
> > we meant.
> >
> > Let's modify drm_mode_create_from_cmdline_mode() to properly generate the
> > drm_display_mode we mean when passing a named mode.
> >
> > Signed-off-by: Maxime Ripard
> >
> > ---
> > Changes in v6:
> > - Fix get_modes to return 0 instead of an error code
> > - Rename the tests to follow the DRM test naming convention
> >
> > Changes in v5:
> > - Switched to KUNIT_ASSERT_NOT_NULL
> > ---
> > drivers/gpu/drm/drm_modes.c | 34 ++-
> > drivers/gpu/drm/tests/drm_client_modeset_test.c | 77
> > -
> > 2 files changed, 109 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> > index dc037f7ceb37..85aa9898c229 100644
> > --- a/drivers/gpu/drm/drm_modes.c
> > +++ b/drivers/gpu/drm/drm_modes.c
> > @@ -2497,6 +2497,36 @@ bool drm_mode_parse_command_line_for_connector(const
> > char *mode_option,
> > }
> > EXPORT_SYMBOL(drm_mode_parse_command_line_for_connector);
> >
> > +static struct drm_display_mode *drm_named_mode(struct drm_device *dev,
> > + struct drm_cmdline_mode *cmd)
> > +{
> > + struct drm_display_mode *mode;
> > + unsigned int i;
> > +
> > + for (i = 0; i < ARRAY_SIZE(drm_named_modes); i++) {
> > + const struct drm_named_mode *named_mode = &drm_named_modes[i];
> > +
> > + if (strcmp(cmd->name, named_mode->name))
> > + continue;
> > +
> > + if (!named_mode->tv_mode)
> > + continue;
> > +
> > + mode = drm_analog_tv_mode(dev,
> > + named_mode->tv_mode,
> > + named_mode->pixel_clock_khz * 1000,
> > + named_mode->xres,
> > + named_mode->yres,
> > + named_mode->flags &
> > DRM_MODE_FLAG_INTERLACE);
> > + if (!mode)
> > + return NULL;
> > +
> > + return mode;
> > + }
> > +
> > + return NULL;
> > +}
> > +
> > /**
> > * drm_mode_create_from_cmdline_mode - convert a command line modeline
> > into a DRM display mode
> > * @dev: DRM device to create the new mode for
> > @@ -2514,7 +2544,9 @@ drm_mode_create_from_cmdline_mode(struct drm_device
> > *dev,
> > if (cmd->xres == 0 || cmd->yres == 0)
> > return NULL;
> >
> > - if (cmd->cvt)
> > + if (strlen(cmd->name))
> > + mode = drm_named_mode(dev, cmd);
>
> I'm trying to track how this generated mode fits into to it all and
> AFAICS if the connector already supports a mode with the same xres/yres
> as the named mode, the named mode will never be created because of the
> check at the beginning of drm_helper_probe_add_cmdline_mode(). It will
> just mark the existing mode with USERDEF and return.
Yep, you're right
> If the connector doesn't already support a mode with such a resolution
> it will be created, but should we do that? If the driver supported such
> a mode it would certainly already have added it to the mode list,
> wouldn't it? After all it's just 2 variants NTSC and PAL.
I wasn't so sure about this part. I think it's still benefitial because
some users (Geert at least has expressed that need) might want a smaller
mode than 480i/576i, whereas the driver is realistically only going to
register those two.
So creating that mode if it isn't declared seems to have value to some.
> We have this in drm_client_modeset.c:drm_connector_pick_cmdline_mode():
>
> list_for_each_entry(mode, &connector->modes, head) {
> /* Check (optional) mode name first */
> if (!strcmp(mode->name, cmdline_mode->name))
> return mode;
>
> Here it looks like the named mode thing is a way to choose a mode, not
> to add one.
>
> I couldn't find any documentation on how named modes is supposed to
> work, have you seen any?
Eh, I guess I'm to blame for that :)
Named modes are really only about the command-line name. The way it was
initially introduced was pretty much to only pass down the name to
drivers for them to figure it out, like we've been doing in sun4i:
https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/sun4i/sun4i_tv.c#L292
It wasn't really working, especially because the userspace pretty much
ignores it. One of the point of this series is to
Re: [PATCH 2/2] fbdev: Add support for the nomodeset kernel parameter
On 11/7/22 11:49, Thomas Zimmermann wrote:
Support the kernel's nomodeset parameter for all PCI-based fbdev
drivers that use aperture helpers to remove other, hardware-agnostic
graphics drivers.
The parameter is a simple way of using the firmware-provided scanout
buffer if the hardware's native driver is broken.
Nah... it's probably not broken, but you want it disabled in order
to use the DRM driver instead?
The same effect
could be achieved with per-driver options, but the importance of the
graphics output for many users makes a single, unified approach
worthwhile.
With nomodeset specified, the fbdev driver module will not load. This
unifies behavior with similar DRM drivers. In DRM helpers, modules
first check the nomodeset parameter before registering the PCI
driver. As fbdev has no such module helpers, we have to modify each
driver individually.
Ok.
The name 'nomodeset' is slightly misleading, but has been chosen for
historical reasons. Several drivers implemented it before it became a
general option for DRM. So keeping the existing name was preferred over
introducing a new one.
diff --git a/drivers/video/fbdev/aty/aty128fb.c
b/drivers/video/fbdev/aty/aty128fb.c
index 57e398fe7a81c..1a26ac2865d65 100644
--- a/drivers/video/fbdev/aty/aty128fb.c
+++ b/drivers/video/fbdev/aty/aty128fb.c
@@ -2503,7 +2504,12 @@ static int aty128fb_init(void)
{
#ifndef MODULE
char *option = NULL;
+#endif
+
+ if (video_firmware_drivers_only())
+ return -ENODEV;
I think it makes sense to give at least some info, why a specific
driver wasn't loaded, e.g. something like this kernel message:
aty128fb: Driver disabled due to "nomodeset" kernel parameter.
If you e.g. change the function video_firmware_drivers_only()
to become video_firmware_drivers_only(const char *drivername)
then you could print such a message in video_firmware_drivers_only().
And I don't like very much the name of function video_firmware_drivers_only(),
but don't have any other better idea right now either...
Helge
Re: [PATCH 10/10] iommufd: Allow iommufd to supply /dev/vfio/vfio
On Mon, Oct 31, 2022 at 04:53:11PM -0600, Alex Williamson wrote:
> On Fri, 28 Oct 2022 15:44:36 -0300
> Jason Gunthorpe wrote:
>
> > On Wed, Oct 26, 2022 at 03:31:33PM -0600, Alex Williamson wrote:
> > > On Tue, 25 Oct 2022 15:50:45 -0300
> > > Jason Gunthorpe wrote:
> > >
> > > > If the VFIO container is compiled out, give a kconfig option for iommufd
> > > > to provide the miscdev node with the same name and permissions as vfio
> > > > uses.
> > > >
> > > > The compatibility node supports the same ioctls as VFIO and
> > > > automatically
> > > > enables the VFIO compatible pinned page accounting mode.
> > >
> > > I think I'd like to see some sort of breadcrumb when /dev/vfio/vfio is
> > > provided by something other than the vfio container code. If we intend
> > > to include this before P2P is resolved, that breadcrumb
> >
> > I don't belive I can get P2P done soon enough. I plan to do it after
> > this is merged. Right now these two series are taking all my time.
> >
> > > (dmesg I'm guessing) might also list any known limitations of the
> > > compatibility to save time with debugging. Thanks,
> >
> > Yes, that makes sense.
> >
> > Do you want a dmesg at module load time, on every open, or a sysfs
> > something? What seems like it would make it into a bug report?
>
> I think dmesg at module load time should probably be ok, every open
> seems like harassment and sysfs would require updated support in
> various bug reporting tools. Users are often terrible about reporting
> full dmesg in bugs, but they do often filter it for "IOMMU" or "VFIO",
> so keep that in mind when crafting the log message. Thanks,
This seems like the right approach, the message comes out once when it
might be most useful:
@@ -176,8 +176,11 @@ static int iommufd_fops_open(struct inode *inode, struct
file *filp)
* For compatibility with VFIO when /dev/vfio/vfio is opened we default
* to the same rlimit accounting as vfio uses.
*/
- if (filp->private_data == &vfio_misc_dev)
+ if (IS_ENABLED(CONFIG_IOMMUFD_VFIO_CONTAINER) &&
+ filp->private_data == &vfio_misc_dev) {
ictx->account_mode = IOPT_PAGES_ACCOUNT_MM;
+ pr_info_once("IOMMUFD is providing /dev/vfio/vfio, not
VFIO.\n");
+ }
xa_init_flags(&ictx->objects, XA_FLAGS_ALLOC1 | XA_FLAGS_ACCOUNT);
ictx->file = filp;
Also this is needed:
@@ -446,6 +449,7 @@ module_exit(iommufd_exit);
#if IS_ENABLED(CONFIG_IOMMUFD_VFIO_CONTAINER)
MODULE_ALIAS_MISCDEV(VFIO_MINOR);
+MODULE_ALIAS("devname:vfio/vfio");
#endif
MODULE_DESCRIPTION("I/O Address Space Management for passthrough devices");
MODULE_LICENSE("GPL");
Thanks,
Jason
Re: [RFC PATCH v2 1/3] drivers/accel: define kconfig and register a new major
On Mon, Nov 7, 2022 at 3:10 PM Jason Gunthorpe wrote: > > On Mon, Nov 07, 2022 at 03:01:08PM +0200, Oded Gabbay wrote: > > I don't agree with your statement that it should be "a layer over top of > > DRM". > > Anything on top of DRM is a device driver. > > Accel is not a device driver, it is a new type of drm minor / drm driver. > > Yeah, I still think this is not the right way, you are getting almost > nothing from DRM and making everything more complicated in the > process. > > > The only alternative imo to that is to abandon the idea of reusing > > drm, and just make an independant accel core code. > > Not quite really, layer it properly and librarize parts of DRM into > things accel can re-use so they are not intimately tied to the DRM > struct device notion. > > IMHO this is much better, because accel has very little need of DRM to > manage a struct device/cdev in the first place. > > Jason I'm not following. How can an accel device be a new type of drm_minor, if it doesn't have access to all its functions and members ? How will accel device leverage, for example, the GEM code without being a drm_minor ? Librarizing parts of DRM sounds nice in theory but the reality is that everything there is interconnected, all the structures are interdependent. I would have to re-write the entire DRM library to make such a thing work. I don't think this was the intention. The current design makes the accel device an integral part of drm, with very minimal code duplication and without re-writing DRM. Oded
Re: [Intel-gfx] [PATCH] drm/i915: Don't wait forever in drop_caches
On 04/11/2022 17:45, John Harrison wrote: On 11/4/2022 03:01, Tvrtko Ursulin wrote: On 03/11/2022 19:16, John Harrison wrote: On 11/3/2022 02:38, Tvrtko Ursulin wrote: On 03/11/2022 09:18, Tvrtko Ursulin wrote: On 03/11/2022 01:33, John Harrison wrote: On 11/2/2022 07:20, Tvrtko Ursulin wrote: On 02/11/2022 12:12, Jani Nikula wrote: On Tue, 01 Nov 2022, john.c.harri...@intel.com wrote: From: John Harrison At the end of each test, IGT does a drop caches call via sysfs with sysfs? Sorry, that was meant to say debugfs. I've also been working on some sysfs IGT issues and evidently got my wires crossed! special flags set. One of the possible paths waits for idle with an infinite timeout. That causes problems for debugging issues when CI catches a "can't go idle" test failure. Best case, the CI system times out (after 90s), attempts a bunch of state dump actions and then reboots the system to recover it. Worst case, the CI system can't do anything at all and then times out (after 1000s) and simply reboots. Sometimes a serial port log of dmesg might be available, sometimes not. So rather than making life hard for ourselves, change the timeout to be 10s rather than infinite. Also, trigger the standard wedge/reset/recover sequence so that testing can continue with a working system (if possible). Signed-off-by: John Harrison --- drivers/gpu/drm/i915/i915_debugfs.c | 7 ++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index ae987e92251dd..9d916fbbfc27c 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -641,6 +641,9 @@ DEFINE_SIMPLE_ATTRIBUTE(i915_perf_noa_delay_fops, DROP_RESET_ACTIVE | \ DROP_RESET_SEQNO | \ DROP_RCU) + +#define DROP_IDLE_TIMEOUT (HZ * 10) I915_IDLE_ENGINES_TIMEOUT is defined in i915_drv.h. It's also only used here. So move here, dropping i915 prefix, next to the newly proposed one? Sure, can do that. I915_GEM_IDLE_TIMEOUT is defined in i915_gem.h. It's only used in gt/intel_gt.c. Move there and rename to GT_IDLE_TIMEOUT? I915_GT_SUSPEND_IDLE_TIMEOUT is defined and used only in intel_gt_pm.c. No action needed, maybe drop i915 prefix if wanted. These two are totally unrelated and in code not being touched by this change. I would rather not conflate changing random other things with fixing this specific issue. I915_IDLE_ENGINES_TIMEOUT is in ms, the rest are in jiffies. Add _MS suffix if wanted. My head spins. I follow and raise that the newly proposed DROP_IDLE_TIMEOUT applies to DROP_ACTIVE and not only DROP_IDLE. My original intention for the name was that is the 'drop caches timeout for intel_gt_wait_for_idle'. Which is quite the mouthful and hence abbreviated to DROP_IDLE_TIMEOUT. But yes, I realised later that name can be conflated with the DROP_IDLE flag. Will rename. Things get refactored, code moves around, bits get left behind, who knows. No reason to get too worked up. :) As long as people are taking a wider view when touching the code base, and are not afraid to send cleanups, things should be good. On the other hand, if every patch gets blocked in code review because someone points out some completely unrelated piece of code could be a bit better then nothing ever gets fixed. If you spot something that you think should be improved, isn't the general idea that you should post a patch yourself to improve it? There's two maintainers per branch and an order of magnitude or two more developers so it'd be nice if cleanups would just be incoming on self-initiative basis. ;) For the actual functional change at hand - it would be nice if code paths in question could handle SIGINT and then we could punt the decision on how long someone wants to wait purely to userspace. But it's probably hard and it's only debugfs so whatever. The code paths in question will already abort on a signal won't they? Both intel_gt_wait_for_idle() and intel_guc_wait_for_pending_msg(), which is where the uc_wait_for_idle eventually ends up, have an 'if(signal_pending) return -EINTR;' check. Beyond that, it sounds like what you are asking for is a change in the IGT libraries and/or CI framework to start sending signals after some specific timeout. That seems like a significantly more complex change (in terms of the number of entities affected and number of groups involved) and unnecessary. If you say so, I haven't looked at them all. But if the code path in question already aborts on signals then I am not sure what is the patch fixing? I assumed you are trying to avoid the write stuck in D forever, which then prevents driver unload and everything, requiring the test runner to eventually reboot. If you say SIGINT works then you can already recover from userspace, no? Whether or not 10s is enough CI will hopefully tell us. I'd probably
Re: [RFC PATCH v2 1/3] drivers/accel: define kconfig and register a new major
On Mon, Nov 07, 2022 at 04:02:01PM +0200, Oded Gabbay wrote: > On Mon, Nov 7, 2022 at 3:10 PM Jason Gunthorpe wrote: > > > > On Mon, Nov 07, 2022 at 03:01:08PM +0200, Oded Gabbay wrote: > > > I don't agree with your statement that it should be "a layer over top of > > > DRM". > > > Anything on top of DRM is a device driver. > > > Accel is not a device driver, it is a new type of drm minor / drm driver. > > > > Yeah, I still think this is not the right way, you are getting almost > > nothing from DRM and making everything more complicated in the > > process. > > > > > The only alternative imo to that is to abandon the idea of reusing > > > drm, and just make an independant accel core code. > > > > Not quite really, layer it properly and librarize parts of DRM into > > things accel can re-use so they are not intimately tied to the DRM > > struct device notion. > > > > IMHO this is much better, because accel has very little need of DRM to > > manage a struct device/cdev in the first place. > > > > Jason > I'm not following. How can an accel device be a new type of drm_minor, > if it doesn't have access to all its functions and members ? "drm_minor" is not necessary anymore. Strictly managing minor numbers lost its value years ago when /dev/ was reorganized. Just use dynamic minors fully. > How will accel device leverage, for example, the GEM code without > being a drm_minor ? Split GEM into a library so it doesn't require that. > Librarizing parts of DRM sounds nice in theory but the reality is that > everything there is interconnected, all the structures are > interdependent. Yes, the kernel is full of stuff that needs improving. Let's not take shortcuts. > I would have to re-write the entire DRM library to make such a thing > work. I don't think this was the intention. Not necessarily you, whoever someday needs GEM would have to do some work. > The current design makes the accel device an integral part of drm, > with very minimal code duplication and without re-writing DRM. And it smells bad, you can't even make it into a proper module. Who knows what other problems will come. Jason
Re: [PATCH v4a 30/38] timers: dma-buf: Use timer_shutdown_sync() for on stack timers
Am 05.11.22 um 07:00 schrieb Steven Rostedt: From: "Steven Rostedt (Google)" Before a timer is released, timer_shutdown_sync() must be called. Link: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fall%2F20221104054053.431922658%40goodmis.org%2F&data=05%7C01%7Cchristian.koenig%40amd.com%7Cfdadde8d801e48fecafa08dabef330f4%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638032248997820518%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=mEL3bFU7fZ7kSrz3vv96opdAyt7Ew4UeY2nNF%2BhDmfc%3D&reserved=0 Cc: Sumit Semwal Cc: "Christian König" Cc: linux-me...@vger.kernel.org Cc: dri-devel@lists.freedesktop.org Cc: linaro-mm-...@lists.linaro.org Signed-off-by: Steven Rostedt (Google) Reviewed-by: Christian König --- drivers/dma-buf/st-dma-fence.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/dma-buf/st-dma-fence.c b/drivers/dma-buf/st-dma-fence.c index fb6e0a6ae2c9..5d3e7b503501 100644 --- a/drivers/dma-buf/st-dma-fence.c +++ b/drivers/dma-buf/st-dma-fence.c @@ -412,7 +412,7 @@ static int test_wait_timeout(void *arg) err = 0; err_free: - del_timer_sync(&wt.timer); + timer_shutdown_sync(&wt.timer); destroy_timer_on_stack(&wt.timer); dma_fence_signal(wt.f); dma_fence_put(wt.f);
Re: [PATCH] drm/i915/mtl: Media GT and Render GT share common GGTT
On 31-10-2022 23:16, Matt Roper wrote:
> On Mon, Oct 31, 2022 at 06:01:11PM +0530, Aravind Iddamsetty wrote:
>> On XE_LPM+ platforms the media engines are carved out into a separate
>> GT but have a common GGTMMADR address range which essentially makes
>> the GGTT address space to be shared between media and render GT.
>
>>
>> int intel_gt_init_mmio(struct intel_gt *gt)
>> @@ -965,6 +973,9 @@ int intel_gt_tiles_init(struct drm_i915_private *i915)
>> int ret;
>>
>> for_each_gt(gt, i915, id) {
>> +if (GRAPHICS_VER(i915) >= 8)
>> +setup_private_pat(gt);
>> +
>
> Since the term "tile" is used for PVC-style remote tiles (which we have
> some framework for, but haven't enabled yet), it seems confusing to have
> the PAT setup for all GTs (including the standalone media GT) in a
> function called intel_gt_tiles_init(). Maybe we should also have a prep
> patch that renames this function if we're going to start doing non-tile
> things in here too?
But isn't GT and Tile used interchangeably. Also, Could you please
elaborate what do you mean by non tile related things here and as i
understand PAT are per GT registers and in case of SA Media the
gsi_offset get added.
>
>> ret = intel_gt_probe_lmem(gt);
>> if (ret)
>> return ret;
Thanks,
Aravind.
[PATCH v7 00/23] drm: Analog TV Improvements
Hi, Here's a series aiming at improving the command line named modes support, and more importantly how we deal with all the analog TV variants. The named modes support were initially introduced to allow to specify the analog TV mode to be used. However, this was causing multiple issues: * The mode name parsed on the command line was passed directly to the driver, which had to figure out which mode it was suppose to match; * Figuring that out wasn't really easy, since the video= argument or what the userspace might not even have a name in the first place, but instead could have passed a mode with the same timings; * The fallback to matching on the timings was mostly working as long as we were supporting one 525 lines (most likely NSTC) and one 625 lines (PAL), but couldn't differentiate between two modes with the same timings (NTSC vs PAL-M vs NSTC-J for example); * There was also some overlap with the tv mode property registered by drm_mode_create_tv_properties(), but named modes weren't interacting with that property at all. * Even though that property was generic, its possible values were specific to each drivers, which made some generic support difficult. Thus, I chose to tackle in multiple steps: * A new TV mode property was introduced, with generic values, each driver reporting through a bitmask what standard it supports to the userspace; * This option was added to the command line parsing code to be able to specify it on the kernel command line, and new atomic_check and reset helpers were created to integrate properly into atomic KMS; * The named mode parsing code is now creating a proper display mode for the given named mode, and the TV standard will thus be part of the connector state; * Two drivers were converted and tested for now (vc4 and sun4i), with some backward compatibility code to translate the old TV mode to the new TV mode; Unit tests were created along the way. One can switch from NTSC to PAL now using (on vc4) modetest -M vc4 -s 53:720x480i -w 53:'TV mode':1 # NTSC modetest -M vc4 -s 53:720x576i -w 53:'TV mode':4 # PAL Let me know what you think, Maxime To: David Airlie To: Daniel Vetter To: Maarten Lankhorst To: Maxime Ripard To: Thomas Zimmermann To: Emma Anholt To: Jani Nikula To: Joonas Lahtinen To: Rodrigo Vivi To: Tvrtko Ursulin To: Ben Skeggs To: Karol Herbst To: Lyude Paul To: Chen-Yu Tsai To: Jernej Skrabec To: Samuel Holland Cc: Geert Uytterhoeven Cc: Mateusz Kwiatkowski Cc: "Noralf Trønnes" Cc: Dave Stevenson Cc: Dom Cobley Cc: Phil Elwell Cc: Cc: linux-ker...@vger.kernel.org Cc: intel-...@lists.freedesktop.org Cc: nouv...@lists.freedesktop.org Cc: linux-arm-ker...@lists.infradead.org Cc: linux-su...@lists.linux.dev Cc: Hans de Goede Signed-off-by: Maxime Ripard --- Changes in v7: - Switch to another implementation of get_modes from Noralf - Made more checks in VEC's atomic_check - Fixed typo in a commit log - Checked for tv_mode_specified in drm_mode_parse_command_line_for_connector - Rebased on drm-misc-next-2022-11-03 - Link to v6: https://lore.kernel.org/r/20220728-rpi-analog-tv-properties-v6-0-e77927341...@cerno.tech Changes in v6: - Add and convert to a new get_modes helper to create the PAL and NTSC modes in the proper order, with the right preferred mode flag, depending on the driver capabilities and defaults. - Support PAL60 - Renamed tests to be consistent with DRM tests naming convention - Simplified a bit the named mode parsing code - Add a tv_mode_specified field - Return 0 in get_modes implementations instead of error codes - Link to v5: https://lore.kernel.org/r/20220728-rpi-analog-tv-properties-v5-0-d841cc64f...@cerno.tech Changes in v5: - Dropped TV Standard documentation removal - Switched the TV Mode documentation from CSV to actual documentation - Switched to kunit assertions where possible - Switched to KUNIT_ASSERT_NOT_NULL instead of KUNIT_ASSERT_PTR_NE(..., NULL) - Shuffled a bit the introduction of drm_client_modeset_connector_get_modes between patches - Renamed tv_mode_names to legacy_tv_mode_names - Removed the count variable in sun4i_tv_comp_get_modes - Rebased on top of current drm-misc-next - Link to v4: https://lore.kernel.org/r/20220728-rpi-analog-tv-properties-v4-0-60d38873f...@cerno.tech Changes in v4: - Removed the unused TV Standard property documentation - Added the TV Mode property documentation to kms-properties.csv - Fixed the documentation of drm_mode_create_tv_properties() - Removed DRM_MODE_TV_MODE_NONE - Reworded the line length check comment in drm_mode_analog_tv tests - Switched to HZ_PER_KHZ in drm_mode_analog_tv tests - Reworked drm_mode_analog_tv to fill our mode using the previously computed timings - Added the command-line option documentation to modedb.rst - Improved the Kunit helpers cleanup - Moved the subconnector documentation renaming to the proper patch - Added the various review
[PATCH v7 01/23] drm/tests: Add Kunit Helpers
As the number of kunit tests in KMS grows further, we start to have
multiple test suites that, for example, need to register a mock DRM
driver to interact with the KMS function they are supposed to test.
Let's add a file meant to provide those kind of helpers to avoid
duplication.
Reviewed-by: Noralf Trønnes
Signed-off-by: Maxime Ripard
---
Changes in v4:
- Simplified the DRM device cleanup patch using devm_drm_dev_alloc()
---
drivers/gpu/drm/tests/Makefile| 1 +
drivers/gpu/drm/tests/drm_kunit_helpers.c | 61 +++
drivers/gpu/drm/tests/drm_kunit_helpers.h | 9 +
3 files changed, 71 insertions(+)
diff --git a/drivers/gpu/drm/tests/Makefile b/drivers/gpu/drm/tests/Makefile
index 2d9f49b62ecb..b29ef1085cad 100644
--- a/drivers/gpu/drm/tests/Makefile
+++ b/drivers/gpu/drm/tests/Makefile
@@ -8,6 +8,7 @@ obj-$(CONFIG_DRM_KUNIT_TEST) += \
drm_format_helper_test.o \
drm_format_test.o \
drm_framebuffer_test.o \
+ drm_kunit_helpers.o \
drm_mm_test.o \
drm_plane_helper_test.o \
drm_rect_test.o
diff --git a/drivers/gpu/drm/tests/drm_kunit_helpers.c
b/drivers/gpu/drm/tests/drm_kunit_helpers.c
new file mode 100644
index ..3524d6a1fa9a
--- /dev/null
+++ b/drivers/gpu/drm/tests/drm_kunit_helpers.c
@@ -0,0 +1,61 @@
+#include
+#include
+
+#include
+
+#include
+
+struct kunit_dev {
+ struct drm_device base;
+};
+
+static const struct drm_mode_config_funcs drm_mode_config_funcs = {
+};
+
+static const struct drm_driver drm_mode_driver = {
+};
+
+static int dev_init(struct kunit_resource *res, void *ptr)
+{
+ char *name = ptr;
+ struct device *dev;
+
+ dev = root_device_register(name);
+ if (IS_ERR(dev))
+ return PTR_ERR(dev);
+
+ res->data = dev;
+ return 0;
+}
+
+static void dev_free(struct kunit_resource *res)
+{
+ struct device *dev = res->data;
+
+ root_device_unregister(dev);
+}
+
+struct drm_device *drm_kunit_device_init(struct kunit *test, char *name)
+{
+ struct kunit_dev *kdev;
+ struct drm_device *drm;
+ struct device *dev;
+ int ret;
+
+ dev = kunit_alloc_resource(test, dev_init, dev_free, GFP_KERNEL, name);
+ if (!dev)
+ return ERR_PTR(-ENOMEM);
+
+ kdev = devm_drm_dev_alloc(dev, &drm_mode_driver, struct kunit_dev,
base);
+ if (IS_ERR(kdev))
+ return ERR_CAST(kdev);
+
+ drm = &kdev->base;
+ drm->mode_config.funcs = &drm_mode_config_funcs;
+
+ ret = drmm_mode_config_init(drm);
+ if (ret)
+ return ERR_PTR(ret);
+
+ return drm;
+}
diff --git a/drivers/gpu/drm/tests/drm_kunit_helpers.h
b/drivers/gpu/drm/tests/drm_kunit_helpers.h
new file mode 100644
index ..a9354f9bda4e
--- /dev/null
+++ b/drivers/gpu/drm/tests/drm_kunit_helpers.h
@@ -0,0 +1,9 @@
+#ifndef DRM_KUNIT_HELPERS_H_
+#define DRM_KUNIT_HELPERS_H_
+
+struct drm_device;
+struct kunit;
+
+struct drm_device *drm_kunit_device_init(struct kunit *test, char *name);
+
+#endif // DRM_KUNIT_HELPERS_H_
--
b4 0.11.0-dev-99e3a
[PATCH printk v3 00/40] reduce console_lock scope
This is v3 of a series to prepare for threaded/atomic printing. v2 is here [0]. This series focuses on reducing the scope of the BKL console_lock. It achieves this by switching to SRCU and a dedicated mutex for console list iteration and modification, respectively. The console_lock will no longer offer this protection and is completely removed from (un)register_console() and console_stop/start() code. Also, during the review of v2 it came to our attention that many console drivers are checking CON_ENABLED to see if they are registered. Because this flag can change without unregistering and because this flag does not represent an atomic point when an (un)registration process is complete, a new console_is_registered() function is introduced. This function uses the console_list_lock to synchronize with the (un)registration process to provide a reliable status. All users of the console_lock for list iteration have been modified. For the call sites where the console_lock is still needed (because of other reasons), comments are added to explain exactly why the console_lock was needed. All users of CON_ENABLED for registration status have been modified to use console_is_registered(). Note that there are still users of CON_ENABLED, but this is for legitimate purposes about a registered console being able to print. The base commit for this series is from Paul McKenney's RCU tree and provides an NMI-safe SRCU implementation [1]. Without the NMI-safe SRCU implementation, this series is not less safe than mainline. But we will need the NMI-safe SRCU implementation for atomic consoles anyway, so we might as well get it in now. Especially since it _does_ increase the reliability for mainline in the panic path. Changes since v3: general: - introduce a synchronized console_is_registered() to query if a console is registered, meant to replace CON_ENABLED (mis)use for this purpose - directly read console->flags for registered consoles if it is race-free (and document that it is so) - replace uart_console_enabled() with a new uart_console_registered() based on console_is_registered() - change comments about why console_lock is used to synchronize console->device() by providing an example registration check fixups: - the following drivers were modified to use the new console_is_registered() instead of CON_ENABLED checks - arch/m68k/emu/nfcon.c - drivers/firmware/efi/earlycon.c - drivers/net/netconsole.c - drivers/tty/hvc/hvc_console.c - drivers/tty/serial/8250/8250_core.c - drivers/tty/serial/earlycon.c - drivers/tty/serial/pic32_uart.c - drivers/tty/serial/samsung_tty.c - drivers/tty/serial/serial_core.c - drivers/tty/serial/xilinx_uartps.c - drivers/usb/early/xhci-dbc.c um: kmsg_dumper: - change stdout dump criteria to match original intention kgdb/kdb: - in configure_kgdboc(), take console_list_lock to synchronize tty_find_polling_driver() against register_console() - add comments explaining why calling console->write() without locking might work tty: sh-sci: - use a setup() callback to setup the early console fbdev: xen: - implement a cleaner approach for console_force_preferred_locked() rcu: - implement debug_lockdep_rcu_enabled() for !CONFIG_DEBUG_LOCK_ALLOC printk: - check CONFIG_DEBUG_LOCK_ALLOC for srcu_read_lock_held() availability - for console_lock/_trylock/_unlock, replace "lock the console system" language with "block the console subsystem from printing" - use WRITE_ONCE() for updating console->flags of registered consoles - expand comments of synchronize_srcu() calls to explain why they are needed, and also expand comments to explain when it is not needed - change CON_BOOT consoles to always begin at earliest message - for non-BOOT/non-PRINTBUFFER consoles, initialize @seq to the minimal @seq of any of the enabled boot consoles - add comments and lockdep assertion to unregister_console_locked() because it is not clear from the name which lock is implied - dropped patches that caused unnecessary churn in the series John Ogness [0] https://lore.kernel.org/lkml/20221019145600.1282823-1-john.ogn...@linutronix.de [1] https://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git/log/?h=srcunmisafe.2022.10.21a John Ogness (38): rcu: implement lockdep_rcu_enabled for !CONFIG_DEBUG_LOCK_ALLOC printk: Prepare for SRCU console list protection printk: fix setting first seq for consoles um: kmsg_dump: only dump when no output console available console: introduce console_is_enabled() wrapper printk: use console_is_enabled() um: kmsg_dump: use console_is_enabled() kdb: kdb_io: use console_is_enabled() um: kmsg_dumper: use srcu console list iterator tty: serial: kgdboc: document console_lock usage tty: tty_io: document console_lock usage proc: consoles: document console_lock usage kdb: use srcu console list iterator printk: console_flush_all: use srcu console list iterator printk: c
[PATCH v7 02/23] drm/connector: Rename legacy TV property
The current tv_mode has driver-specific values that don't allow to
easily share code using it, either at the userspace or kernel level.
Since we're going to introduce a new, generic, property that fit the
same purpose, let's rename this one to legacy_tv_mode to make it
obvious we should move away from it.
Acked-by: Thomas Zimmermann
Reviewed-by: Lyude Paul # nouveau
Reviewed-by: Noralf Trønnes
Signed-off-by: Maxime Ripard
---
drivers/gpu/drm/drm_atomic_uapi.c | 8
drivers/gpu/drm/drm_connector.c | 6 +++---
drivers/gpu/drm/gud/gud_connector.c | 6 +++---
drivers/gpu/drm/i2c/ch7006_drv.c | 4 ++--
drivers/gpu/drm/i915/display/intel_tv.c | 3 ++-
drivers/gpu/drm/nouveau/dispnv04/tvnv17.c | 4 ++--
drivers/gpu/drm/vc4/vc4_vec.c | 8
include/drm/drm_connector.h | 4 ++--
include/drm/drm_mode_config.h | 6 --
9 files changed, 26 insertions(+), 23 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
b/drivers/gpu/drm/drm_atomic_uapi.c
index c06d0639d552..7f2b9a07fbdf 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -698,8 +698,8 @@ static int drm_atomic_connector_set_property(struct
drm_connector *connector,
state->tv.margins.top = val;
} else if (property == config->tv_bottom_margin_property) {
state->tv.margins.bottom = val;
- } else if (property == config->tv_mode_property) {
- state->tv.mode = val;
+ } else if (property == config->legacy_tv_mode_property) {
+ state->tv.legacy_mode = val;
} else if (property == config->tv_brightness_property) {
state->tv.brightness = val;
} else if (property == config->tv_contrast_property) {
@@ -808,8 +808,8 @@ drm_atomic_connector_get_property(struct drm_connector
*connector,
*val = state->tv.margins.top;
} else if (property == config->tv_bottom_margin_property) {
*val = state->tv.margins.bottom;
- } else if (property == config->tv_mode_property) {
- *val = state->tv.mode;
+ } else if (property == config->legacy_tv_mode_property) {
+ *val = state->tv.legacy_mode;
} else if (property == config->tv_brightness_property) {
*val = state->tv.brightness;
} else if (property == config->tv_contrast_property) {
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 223ff2666c3c..98bfbaf29273 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -1690,14 +1690,14 @@ int drm_mode_create_tv_properties(struct drm_device
*dev,
if (drm_mode_create_tv_margin_properties(dev))
goto nomem;
- dev->mode_config.tv_mode_property =
+ dev->mode_config.legacy_tv_mode_property =
drm_property_create(dev, DRM_MODE_PROP_ENUM,
"mode", num_modes);
- if (!dev->mode_config.tv_mode_property)
+ if (!dev->mode_config.legacy_tv_mode_property)
goto nomem;
for (i = 0; i < num_modes; i++)
- drm_property_add_enum(dev->mode_config.tv_mode_property,
+ drm_property_add_enum(dev->mode_config.legacy_tv_mode_property,
i, modes[i]);
dev->mode_config.tv_brightness_property =
diff --git a/drivers/gpu/drm/gud/gud_connector.c
b/drivers/gpu/drm/gud/gud_connector.c
index fa636206f232..86e992b2108b 100644
--- a/drivers/gpu/drm/gud/gud_connector.c
+++ b/drivers/gpu/drm/gud/gud_connector.c
@@ -303,7 +303,7 @@ static int gud_connector_atomic_check(struct drm_connector
*connector,
old_state->tv.margins.right != new_state->tv.margins.right ||
old_state->tv.margins.top != new_state->tv.margins.top ||
old_state->tv.margins.bottom != new_state->tv.margins.bottom ||
- old_state->tv.mode != new_state->tv.mode ||
+ old_state->tv.legacy_mode != new_state->tv.legacy_mode ||
old_state->tv.brightness != new_state->tv.brightness ||
old_state->tv.contrast != new_state->tv.contrast ||
old_state->tv.flicker_reduction != new_state->tv.flicker_reduction
||
@@ -424,7 +424,7 @@ gud_connector_property_lookup(struct drm_connector
*connector, u16 prop)
case GUD_PROPERTY_TV_BOTTOM_MARGIN:
return config->tv_bottom_margin_property;
case GUD_PROPERTY_TV_MODE:
- return config->tv_mode_property;
+ return config->legacy_tv_mode_property;
case GUD_PROPERTY_TV_BRIGHTNESS:
return config->tv_brightness_property;
case GUD_PROPERTY_TV_CONTRAST:
@@ -454,7 +454,7 @@ static unsigned int *gud_connector_tv_state_val(u16 prop,
struct drm_tv_connecto
case GUD_PROPERTY_TV_BOTTOM_MARGIN:
return &state->margins.bottom;
case GUD_
[PATCH v7 03/23] drm/connector: Only register TV mode property if present
The drm_create_tv_properties() will create the TV mode property
unconditionally.
However, since we'll gradually phase it out, let's register it only if we
have a list passed as an argument. This will make the transition easier.
Acked-by: Noralf Trønnes
Signed-off-by: Maxime Ripard
---
drivers/gpu/drm/drm_connector.c | 19 +++
1 file changed, 11 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 98bfbaf29273..6e9e2f96f3a8 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -1690,15 +1690,18 @@ int drm_mode_create_tv_properties(struct drm_device
*dev,
if (drm_mode_create_tv_margin_properties(dev))
goto nomem;
- dev->mode_config.legacy_tv_mode_property =
- drm_property_create(dev, DRM_MODE_PROP_ENUM,
- "mode", num_modes);
- if (!dev->mode_config.legacy_tv_mode_property)
- goto nomem;
- for (i = 0; i < num_modes; i++)
- drm_property_add_enum(dev->mode_config.legacy_tv_mode_property,
- i, modes[i]);
+ if (num_modes) {
+ dev->mode_config.legacy_tv_mode_property =
+ drm_property_create(dev, DRM_MODE_PROP_ENUM,
+ "mode", num_modes);
+ if (!dev->mode_config.legacy_tv_mode_property)
+ goto nomem;
+
+ for (i = 0; i < num_modes; i++)
+
drm_property_add_enum(dev->mode_config.legacy_tv_mode_property,
+ i, modes[i]);
+ }
dev->mode_config.tv_brightness_property =
drm_property_create_range(dev, 0, "brightness", 0, 100);
--
b4 0.11.0-dev-99e3a
[PATCH printk v3 33/40] printk, xen: fbfront: create/use safe function for forcing preferred
With commit 9e124fe16ff2("xen: Enable console tty by default in domU
if it's not a dummy") a hack was implemented to make sure that the
tty console remains the console behind the /dev/console device. The
main problem with the hack is that, after getting the console pointer
to the tty console, it is assumed the pointer is still valid after
releasing the console_sem. This assumption is incorrect and unsafe.
Make the hack safe by introducing a new function
console_force_preferred_locked() and perform the full operation
under the console_list_lock.
Signed-off-by: John Ogness
---
drivers/video/fbdev/xen-fbfront.c | 12 +++--
include/linux/console.h | 1 +
kernel/printk/printk.c| 44 ---
3 files changed, 46 insertions(+), 11 deletions(-)
diff --git a/drivers/video/fbdev/xen-fbfront.c
b/drivers/video/fbdev/xen-fbfront.c
index 4d2694d904aa..8752d389e382 100644
--- a/drivers/video/fbdev/xen-fbfront.c
+++ b/drivers/video/fbdev/xen-fbfront.c
@@ -504,18 +504,14 @@ static void xenfb_make_preferred_console(void)
if (console_set_on_cmdline)
return;
- console_lock();
+ console_list_lock();
for_each_console(c) {
if (!strcmp(c->name, "tty") && c->index == 0)
break;
}
- console_unlock();
- if (c) {
- unregister_console(c);
- c->flags |= CON_CONSDEV;
- c->flags &= ~CON_PRINTBUFFER; /* don't print again */
- register_console(c);
- }
+ if (c)
+ console_force_preferred_locked(c);
+ console_list_unlock();
}
static int xenfb_resume(struct xenbus_device *dev)
diff --git a/include/linux/console.h b/include/linux/console.h
index cdae70e27377..b6b5d796d15c 100644
--- a/include/linux/console.h
+++ b/include/linux/console.h
@@ -273,6 +273,7 @@ enum con_flush_mode {
};
extern int add_preferred_console(char *name, int idx, char *options);
+extern void console_force_preferred_locked(struct console *con);
extern void register_console(struct console *);
extern int unregister_console(struct console *);
extern void console_lock(void);
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index be40a9688403..d74e6e609f7d 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -247,9 +247,10 @@ int devkmsg_sysctl_set_loglvl(struct ctl_table *table, int
write,
void console_list_lock(void)
{
/*
-* In unregister_console(), synchronize_srcu() is called with the
-* console_list_lock held. Therefore it is not allowed that the
-* console_list_lock is taken with the srcu_lock held.
+* In unregister_console() and console_force_preferred_locked(),
+* synchronize_srcu() is called with the console_list_lock held.
+* Therefore it is not allowed that the console_list_lock is taken
+* with the srcu_lock held.
*
* Whether or not this context is in the read-side critical section
* can only be detected if the appropriate debug options are enabled.
@@ -3457,6 +3458,43 @@ int unregister_console(struct console *console)
}
EXPORT_SYMBOL(unregister_console);
+/**
+ * console_force_preferred_locked - force a registered console preferred
+ * @con: The registered console to force preferred.
+ *
+ * Must be called under console_list_lock().
+ */
+void console_force_preferred_locked(struct console *con)
+{
+ struct console *cur_pref_con;
+
+ if (!console_is_registered_locked(con))
+ return;
+
+ cur_pref_con = console_first();
+
+ /* Already preferred? */
+ if (cur_pref_con == con)
+ return;
+
+ hlist_del_init_rcu(&con->node);
+
+ /*
+* Ensure that all SRCU list walks have completed so that the console
+* can be added to the beginning of the console list and its forward
+* list pointer can be re-initialized.
+*/
+ synchronize_srcu(&console_srcu);
+
+ con->flags |= CON_CONSDEV;
+ WARN_ON(!con->device);
+
+ /* Only the new head can have CON_CONSDEV set. */
+ WRITE_ONCE(cur_pref_con->flags, cur_pref_con->flags & ~CON_CONSDEV);
+ hlist_add_behind_rcu(&con->node, console_list.first);
+}
+EXPORT_SYMBOL(console_force_preferred_locked);
+
/*
* Initialize the console device. This is called *early*, so
* we can't necessarily depend on lots of kernel help here.
--
2.30.2
[PATCH v7 04/23] drm/connector: Rename drm_mode_create_tv_properties
drm_mode_create_tv_properties(), among other things, will create the
"mode" property that stores the analog TV mode that connector is
supposed to output.
However, that property is getting deprecated, so let's rename that
function to mention it's deprecated. We'll introduce a new variant of
that function creating the property superseeding it in a later patch.
Reviewed-by: Lyude Paul # nouveau
Reviewed-by: Noralf Trønnes
Signed-off-by: Maxime Ripard
---
drivers/gpu/drm/drm_connector.c | 12 ++--
drivers/gpu/drm/gud/gud_connector.c | 4 ++--
drivers/gpu/drm/i2c/ch7006_drv.c | 2 +-
drivers/gpu/drm/i915/display/intel_tv.c | 2 +-
drivers/gpu/drm/nouveau/dispnv04/tvnv17.c | 2 +-
drivers/gpu/drm/vc4/vc4_vec.c | 5 +++--
include/drm/drm_connector.h | 6 +++---
7 files changed, 17 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 6e9e2f96f3a8..cd01850c66f7 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -1604,7 +1604,7 @@ EXPORT_SYMBOL(drm_connector_attach_tv_margin_properties);
* Called by a driver's HDMI connector initialization routine, this function
* creates the TV margin properties for a given device. No need to call this
* function for an SDTV connector, it's already called from
- * drm_mode_create_tv_properties().
+ * drm_mode_create_tv_properties_legacy().
*
* Returns:
* 0 on success or a negative error code on failure.
@@ -1639,7 +1639,7 @@ int drm_mode_create_tv_margin_properties(struct
drm_device *dev)
EXPORT_SYMBOL(drm_mode_create_tv_margin_properties);
/**
- * drm_mode_create_tv_properties - create TV specific connector properties
+ * drm_mode_create_tv_properties_legacy - create TV specific connector
properties
* @dev: DRM device
* @num_modes: number of different TV formats (modes) supported
* @modes: array of pointers to strings containing name of each format
@@ -1652,9 +1652,9 @@ EXPORT_SYMBOL(drm_mode_create_tv_margin_properties);
* Returns:
* 0 on success or a negative error code on failure.
*/
-int drm_mode_create_tv_properties(struct drm_device *dev,
- unsigned int num_modes,
- const char * const modes[])
+int drm_mode_create_tv_properties_legacy(struct drm_device *dev,
+unsigned int num_modes,
+const char * const modes[])
{
struct drm_property *tv_selector;
struct drm_property *tv_subconnector;
@@ -1737,7 +1737,7 @@ int drm_mode_create_tv_properties(struct drm_device *dev,
nomem:
return -ENOMEM;
}
-EXPORT_SYMBOL(drm_mode_create_tv_properties);
+EXPORT_SYMBOL(drm_mode_create_tv_properties_legacy);
/**
* drm_mode_create_scaling_mode_property - create scaling mode property
diff --git a/drivers/gpu/drm/gud/gud_connector.c
b/drivers/gpu/drm/gud/gud_connector.c
index 86e992b2108b..034e78360d4f 100644
--- a/drivers/gpu/drm/gud/gud_connector.c
+++ b/drivers/gpu/drm/gud/gud_connector.c
@@ -400,7 +400,7 @@ static int gud_connector_add_tv_mode(struct gud_device
*gdrm, struct drm_connect
for (i = 0; i < num_modes; i++)
modes[i] = &buf[i * GUD_CONNECTOR_TV_MODE_NAME_LEN];
- ret = drm_mode_create_tv_properties(connector->dev, num_modes, modes);
+ ret = drm_mode_create_tv_properties_legacy(connector->dev, num_modes,
modes);
free:
kfree(buf);
if (ret < 0)
@@ -539,7 +539,7 @@ static int gud_connector_add_properties(struct gud_device
*gdrm, struct gud_conn
fallthrough;
case GUD_PROPERTY_TV_HUE:
/* This is a no-op if already added. */
- ret = drm_mode_create_tv_properties(drm, 0, NULL);
+ ret = drm_mode_create_tv_properties_legacy(drm, 0,
NULL);
if (ret)
goto out;
break;
diff --git a/drivers/gpu/drm/i2c/ch7006_drv.c b/drivers/gpu/drm/i2c/ch7006_drv.c
index ef69f9bdeace..b63bad04b09d 100644
--- a/drivers/gpu/drm/i2c/ch7006_drv.c
+++ b/drivers/gpu/drm/i2c/ch7006_drv.c
@@ -250,7 +250,7 @@ static int ch7006_encoder_create_resources(struct
drm_encoder *encoder,
struct drm_device *dev = encoder->dev;
struct drm_mode_config *conf = &dev->mode_config;
- drm_mode_create_tv_properties(dev, NUM_TV_NORMS, ch7006_tv_norm_names);
+ drm_mode_create_tv_properties_legacy(dev, NUM_TV_NORMS,
ch7006_tv_norm_names);
priv->scale_property = drm_property_create_range(dev, 0, "scale", 0, 2);
if (!priv->scale_property)
diff --git a/drivers/gpu/drm/i915/display/intel_tv.c
b/drivers/gpu/drm/i915/display/intel_tv.c
index b2f42bf929e2..748821ebdf65 100644
--- a/drivers/gpu/drm/i915/display/intel_tv.c
+++ b/drivers/gpu/drm/i915/display/intel_tv.c
@@ -1995,7 +1995,7 @@ in
[PATCH v7 06/23] drm/modes: Add a function to generate analog display modes
Multiple drivers (meson, vc4, sun4i) define analog TV 525-lines and
625-lines modes in their drivers.
Since those modes are fairly standard, and that we'll need to use them
in more places in the future, it makes sense to move their definition
into the core framework.
However, analog display usually have fairly loose timings requirements,
the only discrete parameters being the total number of lines and pixel
clock frequency. Thus, we created a function that will create a display
mode from the standard, the pixel frequency and the active area.
Signed-off-by: Maxime Ripard
---
Changes in v6:
- Fix typo
Changes in v4:
- Reworded the line length check comment
- Switch to HZ_PER_KHZ in tests
- Use previous timing to fill our mode
- Move the number of lines check earlier
---
drivers/gpu/drm/drm_modes.c| 474 +
drivers/gpu/drm/tests/Makefile | 1 +
drivers/gpu/drm/tests/drm_modes_test.c | 144 ++
include/drm/drm_modes.h| 17 ++
4 files changed, 636 insertions(+)
diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index 5d4ac79381c4..71c050c3ee6b 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -116,6 +116,480 @@ void drm_mode_probed_add(struct drm_connector *connector,
}
EXPORT_SYMBOL(drm_mode_probed_add);
+enum drm_mode_analog {
+ DRM_MODE_ANALOG_NTSC, /* 525 lines, 60Hz */
+ DRM_MODE_ANALOG_PAL, /* 625 lines, 50Hz */
+};
+
+/*
+ * The timings come from:
+ * -
https://web.archive.org/web/20220406232708/http://www.kolumbus.fi/pami1/video/pal_ntsc.html
+ * -
https://web.archive.org/web/20220406124914/http://martin.hinner.info/vga/pal.html
+ * -
https://web.archive.org/web/20220609202433/http://www.batsocks.co.uk/readme/video_timing.htm
+ */
+#define NTSC_LINE_DURATION_NS 63556U
+#define NTSC_LINES_NUMBER 525
+
+#define NTSC_HBLK_DURATION_TYP_NS 10900U
+#define NTSC_HBLK_DURATION_MIN_NS (NTSC_HBLK_DURATION_TYP_NS - 200)
+#define NTSC_HBLK_DURATION_MAX_NS (NTSC_HBLK_DURATION_TYP_NS + 200)
+
+#define NTSC_HACT_DURATION_TYP_NS (NTSC_LINE_DURATION_NS -
NTSC_HBLK_DURATION_TYP_NS)
+#define NTSC_HACT_DURATION_MIN_NS (NTSC_LINE_DURATION_NS -
NTSC_HBLK_DURATION_MAX_NS)
+#define NTSC_HACT_DURATION_MAX_NS (NTSC_LINE_DURATION_NS -
NTSC_HBLK_DURATION_MIN_NS)
+
+#define NTSC_HFP_DURATION_TYP_NS 1500
+#define NTSC_HFP_DURATION_MIN_NS 1270
+#define NTSC_HFP_DURATION_MAX_NS 2220
+
+#define NTSC_HSLEN_DURATION_TYP_NS 4700
+#define NTSC_HSLEN_DURATION_MIN_NS (NTSC_HSLEN_DURATION_TYP_NS - 100)
+#define NTSC_HSLEN_DURATION_MAX_NS (NTSC_HSLEN_DURATION_TYP_NS + 100)
+
+#define NTSC_HBP_DURATION_TYP_NS 4700
+
+/*
+ * I couldn't find the actual tolerance for the back porch, so let's
+ * just reuse the sync length ones.
+ */
+#define NTSC_HBP_DURATION_MIN_NS (NTSC_HBP_DURATION_TYP_NS - 100)
+#define NTSC_HBP_DURATION_MAX_NS (NTSC_HBP_DURATION_TYP_NS + 100)
+
+#define PAL_LINE_DURATION_NS 64000U
+#define PAL_LINES_NUMBER 625
+
+#define PAL_HACT_DURATION_TYP_NS 51950U
+#define PAL_HACT_DURATION_MIN_NS (PAL_HACT_DURATION_TYP_NS - 100)
+#define PAL_HACT_DURATION_MAX_NS (PAL_HACT_DURATION_TYP_NS + 400)
+
+#define PAL_HBLK_DURATION_TYP_NS (PAL_LINE_DURATION_NS -
PAL_HACT_DURATION_TYP_NS)
+#define PAL_HBLK_DURATION_MIN_NS (PAL_LINE_DURATION_NS -
PAL_HACT_DURATION_MAX_NS)
+#define PAL_HBLK_DURATION_MAX_NS (PAL_LINE_DURATION_NS -
PAL_HACT_DURATION_MIN_NS)
+
+#define PAL_HFP_DURATION_TYP_NS1650
+#define PAL_HFP_DURATION_MIN_NS(PAL_HFP_DURATION_TYP_NS - 100)
+#define PAL_HFP_DURATION_MAX_NS(PAL_HFP_DURATION_TYP_NS + 400)
+
+#define PAL_HSLEN_DURATION_TYP_NS 4700
+#define PAL_HSLEN_DURATION_MIN_NS (PAL_HSLEN_DURATION_TYP_NS - 200)
+#define PAL_HSLEN_DURATION_MAX_NS (PAL_HSLEN_DURATION_TYP_NS + 200)
+
+#define PAL_HBP_DURATION_TYP_NS5700
+#define PAL_HBP_DURATION_MIN_NS(PAL_HBP_DURATION_TYP_NS - 200)
+#define PAL_HBP_DURATION_MAX_NS(PAL_HBP_DURATION_TYP_NS + 200)
+
+struct analog_param_field {
+ unsigned int even, odd;
+};
+
+#define PARAM_FIELD(_odd, _even) \
+ { .even = _even, .odd = _odd }
+
+struct analog_param_range {
+ unsigned intmin, typ, max;
+};
+
+#define PARAM_RANGE(_min, _typ, _max) \
+ { .min = _min, .typ = _typ, .max = _max }
+
+struct analog_parameters {
+ unsigned intnum_lines;
+ unsigned intline_duration_ns;
+
+ struct analog_param_range hact_ns;
+ struct analog_param_range hfp_ns;
+ struct analog_param_range hslen_ns;
+ struct analog_param_range hbp_ns;
+ struct analog_param_range hblk_ns;
+
+ unsigned intbt601_hfp;
+
+ s
[PATCH v7 05/23] drm/connector: Add TV standard property
The TV mode property has been around for a while now to select and get the
current TV mode output on an analog TV connector.
Despite that property name being generic, its content isn't and has been
driver-specific which makes it hard to build any generic behaviour on top
of it, both in kernel and user-space.
Let's create a new enum tv norm property, that can contain any of the
analog TV standards currently supported by kernel drivers. Each driver can
then pass in a bitmask of the modes it supports, and the property
creation function will filter out the modes not supported.
We'll then be able to phase out the older tv mode property.
Signed-off-by: Maxime Ripard
---
Changes in v5:
- Create an analog TV properties documentation section, and document TV
Mode there instead of the csv file
Changes in v4:
- Add property documentation to kms-properties.csv
- Fix documentation
---
Documentation/gpu/drm-kms.rst | 6 ++
drivers/gpu/drm/drm_atomic_uapi.c | 4 ++
drivers/gpu/drm/drm_connector.c | 122 +-
include/drm/drm_connector.h | 64
include/drm/drm_mode_config.h | 8 +++
5 files changed, 203 insertions(+), 1 deletion(-)
diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst
index b4377a545425..321f2f582c64 100644
--- a/Documentation/gpu/drm-kms.rst
+++ b/Documentation/gpu/drm-kms.rst
@@ -520,6 +520,12 @@ HDMI Specific Connector Properties
.. kernel-doc:: drivers/gpu/drm/drm_connector.c
:doc: HDMI connector properties
+Analog TV Specific Connector Properties
+--
+
+.. kernel-doc:: drivers/gpu/drm/drm_connector.c
+ :doc: Analog TV Connector Properties
+
Standard CRTC Properties
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
b/drivers/gpu/drm/drm_atomic_uapi.c
index 7f2b9a07fbdf..d867e7f9f2cd 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -700,6 +700,8 @@ static int drm_atomic_connector_set_property(struct
drm_connector *connector,
state->tv.margins.bottom = val;
} else if (property == config->legacy_tv_mode_property) {
state->tv.legacy_mode = val;
+ } else if (property == config->tv_mode_property) {
+ state->tv.mode = val;
} else if (property == config->tv_brightness_property) {
state->tv.brightness = val;
} else if (property == config->tv_contrast_property) {
@@ -810,6 +812,8 @@ drm_atomic_connector_get_property(struct drm_connector
*connector,
*val = state->tv.margins.bottom;
} else if (property == config->legacy_tv_mode_property) {
*val = state->tv.legacy_mode;
+ } else if (property == config->tv_mode_property) {
+ *val = state->tv.mode;
} else if (property == config->tv_brightness_property) {
*val = state->tv.brightness;
} else if (property == config->tv_contrast_property) {
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index cd01850c66f7..f9a6cb792bda 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -984,6 +984,17 @@ static const struct drm_prop_enum_list
drm_dvi_i_subconnector_enum_list[] = {
DRM_ENUM_NAME_FN(drm_get_dvi_i_subconnector_name,
drm_dvi_i_subconnector_enum_list)
+static const struct drm_prop_enum_list drm_tv_mode_enum_list[] = {
+ { DRM_MODE_TV_MODE_NTSC, "NTSC" },
+ { DRM_MODE_TV_MODE_NTSC_443, "NTSC-443" },
+ { DRM_MODE_TV_MODE_NTSC_J, "NTSC-J" },
+ { DRM_MODE_TV_MODE_PAL, "PAL" },
+ { DRM_MODE_TV_MODE_PAL_M, "PAL-M" },
+ { DRM_MODE_TV_MODE_PAL_N, "PAL-N" },
+ { DRM_MODE_TV_MODE_SECAM, "SECAM" },
+};
+DRM_ENUM_NAME_FN(drm_get_tv_mode_name, drm_tv_mode_enum_list)
+
static const struct drm_prop_enum_list drm_tv_select_enum_list[] = {
{ DRM_MODE_SUBCONNECTOR_Automatic, "Automatic" }, /* DVI-I and TV-out */
{ DRM_MODE_SUBCONNECTOR_Composite, "Composite" }, /* TV-out */
@@ -1552,6 +1563,71 @@
EXPORT_SYMBOL(drm_connector_attach_dp_subconnector_property);
* infoframe values is done through drm_hdmi_avi_infoframe_content_type().
*/
+/*
+ * TODO: Document the properties:
+ * - left margin
+ * - right margin
+ * - top margin
+ * - bottom margin
+ * - brightness
+ * - contrast
+ * - flicker reduction
+ * - hue
+ * - mode
+ * - overscan
+ * - saturation
+ * - select subconnector
+ * - subconnector
+ */
+/**
+ * DOC: Analog TV Connector Properties
+ *
+ * TV Mode:
+ * Indicates the TV Mode used on an analog TV connector. The value
+ * of this property can be one of the following:
+ *
+ * NTSC:
+ * TV Mode is CCIR System M (aka 525-lines) together with
+ * the NTSC Color Encoding.
+ *
+ * NTSC-443:
+ *
+ * TV Mode is CCIR System M (aka 525-lines) together with
+ *
[PATCH v7 07/23] drm/client: Add some tests for drm_connector_pick_cmdline_mode()
drm_connector_pick_cmdline_mode() is in charge of finding a proper
drm_display_mode from the definition we got in the video= command line
argument.
Let's add some unit tests to make sure we're not getting any regressions
there.
Acked-by: Noralf Trønnes
Signed-off-by: Maxime Ripard
---
Changes in v6:
- Rename tests to be consistent with DRM tests naming convention
Changes in v5:
- Removed useless (for now) count and modes intermediate variables in
get_modes
- Switched to kunit assertions in test init, and to KUNIT_ASSERT_NOT_NULL
instead of KUNIT_ASSERT_PTR_NE(..., NULL)
Changes in v4:
- Removed MODULE macros
---
drivers/gpu/drm/drm_client_modeset.c| 4 +
drivers/gpu/drm/tests/drm_client_modeset_test.c | 100
2 files changed, 104 insertions(+)
diff --git a/drivers/gpu/drm/drm_client_modeset.c
b/drivers/gpu/drm/drm_client_modeset.c
index bbc535cc50dd..d553e793e673 100644
--- a/drivers/gpu/drm/drm_client_modeset.c
+++ b/drivers/gpu/drm/drm_client_modeset.c
@@ -1237,3 +1237,7 @@ int drm_client_modeset_dpms(struct drm_client_dev
*client, int mode)
return ret;
}
EXPORT_SYMBOL(drm_client_modeset_dpms);
+
+#ifdef CONFIG_DRM_KUNIT_TEST
+#include "tests/drm_client_modeset_test.c"
+#endif
diff --git a/drivers/gpu/drm/tests/drm_client_modeset_test.c
b/drivers/gpu/drm/tests/drm_client_modeset_test.c
new file mode 100644
index ..3aa1acfe75df
--- /dev/null
+++ b/drivers/gpu/drm/tests/drm_client_modeset_test.c
@@ -0,0 +1,100 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2022 Maxime Ripard
+ */
+
+#include
+
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include "drm_kunit_helpers.h"
+
+struct drm_client_modeset_test_priv {
+ struct drm_device *drm;
+ struct drm_connector connector;
+};
+
+static int drm_client_modeset_connector_get_modes(struct drm_connector
*connector)
+{
+ return drm_add_modes_noedid(connector, 1920, 1200);
+}
+
+static const struct drm_connector_helper_funcs
drm_client_modeset_connector_helper_funcs = {
+ .get_modes = drm_client_modeset_connector_get_modes,
+};
+
+static const struct drm_connector_funcs drm_client_modeset_connector_funcs = {
+};
+
+static int drm_client_modeset_test_init(struct kunit *test)
+{
+ struct drm_client_modeset_test_priv *priv;
+ int ret;
+
+ priv = kunit_kzalloc(test, sizeof(*priv), GFP_KERNEL);
+ KUNIT_ASSERT_NOT_NULL(test, priv);
+
+ test->priv = priv;
+
+ priv->drm = drm_kunit_device_init(test, "drm-client-modeset-test");
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv->drm);
+
+ ret = drmm_connector_init(priv->drm, &priv->connector,
+ &drm_client_modeset_connector_funcs,
+ DRM_MODE_CONNECTOR_Unknown,
+ NULL);
+ KUNIT_ASSERT_EQ(test, ret, 0);
+
+ drm_connector_helper_add(&priv->connector,
&drm_client_modeset_connector_helper_funcs);
+
+ return 0;
+
+}
+
+static void drm_test_pick_cmdline_res_1920_1080_60(struct kunit *test)
+{
+ struct drm_client_modeset_test_priv *priv = test->priv;
+ struct drm_device *drm = priv->drm;
+ struct drm_connector *connector = &priv->connector;
+ struct drm_cmdline_mode *cmdline_mode = &connector->cmdline_mode;
+ struct drm_display_mode *expected_mode, *mode;
+ const char *cmdline = "1920x1080@60";
+ int ret;
+
+ expected_mode = drm_mode_find_dmt(priv->drm, 1920, 1080, 60, false);
+ KUNIT_ASSERT_NOT_NULL(test, expected_mode);
+
+ KUNIT_ASSERT_TRUE(test,
+ drm_mode_parse_command_line_for_connector(cmdline,
+ connector,
+
cmdline_mode));
+
+ mutex_lock(&drm->mode_config.mutex);
+ ret = drm_helper_probe_single_connector_modes(connector, 1920, 1080);
+ mutex_unlock(&drm->mode_config.mutex);
+ KUNIT_ASSERT_GT(test, ret, 0);
+
+ mode = drm_connector_pick_cmdline_mode(connector);
+ KUNIT_ASSERT_NOT_NULL(test, mode);
+
+ KUNIT_EXPECT_TRUE(test, drm_mode_equal(expected_mode, mode));
+}
+
+
+static struct kunit_case drm_test_pick_cmdline_tests[] = {
+ KUNIT_CASE(drm_test_pick_cmdline_res_1920_1080_60),
+ {}
+};
+
+static struct kunit_suite drm_test_pick_cmdline_test_suite = {
+ .name = "drm_test_pick_cmdline",
+ .init = drm_client_modeset_test_init,
+ .test_cases = drm_test_pick_cmdline_tests
+};
+
+kunit_test_suite(drm_test_pick_cmdline_test_suite);
--
b4 0.11.0-dev-99e3a
[PATCH v7 08/23] drm/modes: Move named modes parsing to a separate function
The current construction of the named mode parsing doesn't allow to extend
it easily. Let's move it to a separate function so we can add more
parameters and modes.
In order for the tests to still pass, some extra checks are needed, so
it's not a 1:1 move.
Reviewed-by: Noralf Trønnes
Signed-off-by: Maxime Ripard
---
Changes in v7:
- Add Noralf Reviewed-by
Changes in v6:
- Simplify the test for connection status extras
- Simplify the code path to call drm_mode_parse_cmdline_named_mode
Changes in v4:
- Fold down all the named mode patches that were split into a single
patch again to maintain bisectability
---
drivers/gpu/drm/drm_modes.c | 70 +
1 file changed, 58 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index 71c050c3ee6b..37542612912b 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -2229,6 +2229,51 @@ static const char * const drm_named_modes_whitelist[] = {
"PAL",
};
+static int drm_mode_parse_cmdline_named_mode(const char *name,
+unsigned int name_end,
+struct drm_cmdline_mode
*cmdline_mode)
+{
+ unsigned int i;
+
+ if (!name_end)
+ return 0;
+
+ /* If the name starts with a digit, it's not a named mode */
+ if (isdigit(name[0]))
+ return 0;
+
+ /*
+* If there's an equal sign in the name, the command-line
+* contains only an option and no mode.
+*/
+ if (strnchr(name, name_end, '='))
+ return 0;
+
+ /* The connection status extras can be set without a mode. */
+ if (name_end == 1 &&
+ (name[0] == 'd' || name[0] == 'D' || name[0] == 'e'))
+ return 0;
+
+ /*
+* We're sure we're a named mode at this point, iterate over the
+* list of modes we're aware of.
+*/
+ for (i = 0; i < ARRAY_SIZE(drm_named_modes_whitelist); i++) {
+ int ret;
+
+ ret = str_has_prefix(name, drm_named_modes_whitelist[i]);
+ if (ret != name_end)
+ continue;
+
+ strcpy(cmdline_mode->name, drm_named_modes_whitelist[i]);
+ cmdline_mode->specified = true;
+
+ return 1;
+ }
+
+ return -EINVAL;
+}
+
/**
* drm_mode_parse_command_line_for_connector - parse command line modeline for
connector
* @mode_option: optional per connector mode option
@@ -2265,7 +2310,7 @@ bool drm_mode_parse_command_line_for_connector(const char
*mode_option,
const char *bpp_ptr = NULL, *refresh_ptr = NULL, *extra_ptr = NULL;
const char *options_ptr = NULL;
char *bpp_end_ptr = NULL, *refresh_end_ptr = NULL;
- int i, len, ret;
+ int len, ret;
memset(mode, 0, sizeof(*mode));
mode->panel_orientation = DRM_MODE_PANEL_ORIENTATION_UNKNOWN;
@@ -2306,18 +2351,19 @@ bool drm_mode_parse_command_line_for_connector(const
char *mode_option,
parse_extras = true;
}
- /* First check for a named mode */
- for (i = 0; i < ARRAY_SIZE(drm_named_modes_whitelist); i++) {
- ret = str_has_prefix(name, drm_named_modes_whitelist[i]);
- if (ret == mode_end) {
- if (refresh_ptr)
- return false; /* named + refresh is invalid */
+ if (!mode_end)
+ return false;
- strcpy(mode->name, drm_named_modes_whitelist[i]);
- mode->specified = true;
- break;
- }
- }
+ ret = drm_mode_parse_cmdline_named_mode(name, mode_end, mode);
+ if (ret < 0)
+ return false;
+
+ /*
+* Having a mode that starts by a letter (and thus is named) and
+* an at-sign (used to specify a refresh rate) is disallowed.
+*/
+ if (ret && refresh_ptr)
+ return false;
/* No named mode? Check for a normal mode argument, e.g. 1024x768 */
if (!mode->specified && isdigit(name[0])) {
--
b4 0.11.0-dev-99e3a
[PATCH v7 09/23] drm/modes: Switch to named mode descriptors
The current named mode parsing relies only on the mode name, and doesn't
allow to specify any other parameter.
Let's convert that string list to an array of a custom structure that will
hold the name and some additional parameters in the future.
Reviewed-by: Noralf Trønnes
Signed-off-by: Maxime Ripard
---
Changes in v7:
- Fix typo in the commit log
- Add Noralf Reviewed-by
---
drivers/gpu/drm/drm_modes.c | 17 +++--
1 file changed, 11 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index 37542612912b..7594b657f86a 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -2224,9 +2224,13 @@ static int drm_mode_parse_cmdline_options(const char
*str,
return 0;
}
-static const char * const drm_named_modes_whitelist[] = {
- "NTSC",
- "PAL",
+struct drm_named_mode {
+ const char *name;
+};
+
+static const struct drm_named_mode drm_named_modes[] = {
+ { "NTSC", },
+ { "PAL", },
};
static int drm_mode_parse_cmdline_named_mode(const char *name,
@@ -2258,14 +2262,15 @@ static int drm_mode_parse_cmdline_named_mode(const char
*name,
* We're sure we're a named mode at this point, iterate over the
* list of modes we're aware of.
*/
- for (i = 0; i < ARRAY_SIZE(drm_named_modes_whitelist); i++) {
+ for (i = 0; i < ARRAY_SIZE(drm_named_modes); i++) {
+ const struct drm_named_mode *mode = &drm_named_modes[i];
int ret;
- ret = str_has_prefix(name, drm_named_modes_whitelist[i]);
+ ret = str_has_prefix(name, mode->name);
if (ret != name_end)
continue;
- strcpy(cmdline_mode->name, drm_named_modes_whitelist[i]);
+ strcpy(cmdline_mode->name, mode->name);
cmdline_mode->specified = true;
return 1;
--
b4 0.11.0-dev-99e3a
[PATCH v7 11/23] drm/connector: Add pixel clock to cmdline mode
We'll need to get the pixel clock to generate proper display modes for
all the current named modes. Let's add it to struct drm_cmdline_mode and
fill it when parsing the named mode.
Reviewed-by: Noralf Trønnes
Signed-off-by: Maxime Ripard
---
Changes in v7:
- Add Noralf Reviewed-by:w
---
drivers/gpu/drm/drm_modes.c | 9 ++---
include/drm/drm_connector.h | 7 +++
2 files changed, 13 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index acee23e1a8b7..c826f9583a1d 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -2226,22 +2226,24 @@ static int drm_mode_parse_cmdline_options(const char
*str,
struct drm_named_mode {
const char *name;
+ unsigned int pixel_clock_khz;
unsigned int xres;
unsigned int yres;
unsigned int flags;
};
-#define NAMED_MODE(_name, _x, _y, _flags) \
+#define NAMED_MODE(_name, _pclk, _x, _y, _flags) \
{ \
.name = _name, \
+ .pixel_clock_khz = _pclk, \
.xres = _x, \
.yres = _y, \
.flags = _flags,\
}
static const struct drm_named_mode drm_named_modes[] = {
- NAMED_MODE("NTSC", 720, 480, DRM_MODE_FLAG_INTERLACE),
- NAMED_MODE("PAL", 720, 576, DRM_MODE_FLAG_INTERLACE),
+ NAMED_MODE("NTSC", 13500, 720, 480, DRM_MODE_FLAG_INTERLACE),
+ NAMED_MODE("PAL", 13500, 720, 576, DRM_MODE_FLAG_INTERLACE),
};
static int drm_mode_parse_cmdline_named_mode(const char *name,
@@ -2282,6 +2284,7 @@ static int drm_mode_parse_cmdline_named_mode(const char
*name,
continue;
strcpy(cmdline_mode->name, mode->name);
+ cmdline_mode->pixel_clock = mode->pixel_clock_khz;
cmdline_mode->xres = mode->xres;
cmdline_mode->yres = mode->yres;
cmdline_mode->interlace = !!(mode->flags &
DRM_MODE_FLAG_INTERLACE);
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 9afc7956fdc6..4927dcb2573f 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -1273,6 +1273,13 @@ struct drm_cmdline_mode {
*/
bool bpp_specified;
+ /**
+* @pixel_clock:
+*
+* Pixel Clock in kHz. Optional.
+*/
+ unsigned int pixel_clock;
+
/**
* @xres:
*
--
b4 0.11.0-dev-99e3a
[PATCH v7 10/23] drm/modes: Fill drm_cmdline mode from named modes
The current code to deal with named modes will only set the mode name, and
then it's up to drivers to try to match that name to whatever mode or
configuration they see fit.
The plan is to remove that need and move the named mode handling out of
drivers and into the core, and only rely on modes and properties. Let's
start by properly filling drm_cmdline_mode from a named mode.
Reviewed-by: Noralf Trønnes
Signed-off-by: Maxime Ripard
---
Changes in v7:
- Add Noralf Reviewed-by:w
---
drivers/gpu/drm/drm_modes.c | 18 --
1 file changed, 16 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index 7594b657f86a..acee23e1a8b7 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -2226,11 +2226,22 @@ static int drm_mode_parse_cmdline_options(const char
*str,
struct drm_named_mode {
const char *name;
+ unsigned int xres;
+ unsigned int yres;
+ unsigned int flags;
};
+#define NAMED_MODE(_name, _x, _y, _flags) \
+ { \
+ .name = _name, \
+ .xres = _x, \
+ .yres = _y, \
+ .flags = _flags,\
+ }
+
static const struct drm_named_mode drm_named_modes[] = {
- { "NTSC", },
- { "PAL", },
+ NAMED_MODE("NTSC", 720, 480, DRM_MODE_FLAG_INTERLACE),
+ NAMED_MODE("PAL", 720, 576, DRM_MODE_FLAG_INTERLACE),
};
static int drm_mode_parse_cmdline_named_mode(const char *name,
@@ -2271,6 +2282,9 @@ static int drm_mode_parse_cmdline_named_mode(const char
*name,
continue;
strcpy(cmdline_mode->name, mode->name);
+ cmdline_mode->xres = mode->xres;
+ cmdline_mode->yres = mode->yres;
+ cmdline_mode->interlace = !!(mode->flags &
DRM_MODE_FLAG_INTERLACE);
cmdline_mode->specified = true;
return 1;
--
b4 0.11.0-dev-99e3a
