RE: [Intel-gfx] [PATCH 2/6] drm/eld: replace uint8_t with u8
> -Original Message-
> From: Intel-gfx On Behalf Of Jani
> Nikula
> Sent: Thursday, September 7, 2023 2:58 PM
> To: dri-devel@lists.freedesktop.org
> Cc: Nikula, Jani ; intel-...@lists.freedesktop.org
> Subject: [Intel-gfx] [PATCH 2/6] drm/eld: replace uint8_t with u8
>
> Unify on kernel types.
>
> Cc: Mitul Golani
> Signed-off-by: Jani Nikula
LGTM
Reviewed-by: Chaitanya Kumar Borah
> ---
> include/drm/drm_eld.h | 14 +++---
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/include/drm/drm_eld.h b/include/drm/drm_eld.h index
> 9bde89bd96ea..7b674256b9aa 100644
> --- a/include/drm/drm_eld.h
> +++ b/include/drm/drm_eld.h
> @@ -70,7 +70,7 @@
> * drm_eld_mnl - Get ELD monitor name length in bytes.
> * @eld: pointer to an eld memory structure with mnl set
> */
> -static inline int drm_eld_mnl(const uint8_t *eld)
> +static inline int drm_eld_mnl(const u8 *eld)
> {
> return (eld[DRM_ELD_CEA_EDID_VER_MNL] &
> DRM_ELD_MNL_MASK) >> DRM_ELD_MNL_SHIFT; } @@ -79,7 +79,7 @@
> static inline int drm_eld_mnl(const uint8_t *eld)
> * drm_eld_sad - Get ELD SAD structures.
> * @eld: pointer to an eld memory structure with sad_count set
> */
> -static inline const uint8_t *drm_eld_sad(const uint8_t *eld)
> +static inline const u8 *drm_eld_sad(const u8 *eld)
> {
> unsigned int ver, mnl;
>
> @@ -98,7 +98,7 @@ static inline const uint8_t *drm_eld_sad(const uint8_t
> *eld)
> * drm_eld_sad_count - Get ELD SAD count.
> * @eld: pointer to an eld memory structure with sad_count set
> */
> -static inline int drm_eld_sad_count(const uint8_t *eld)
> +static inline int drm_eld_sad_count(const u8 *eld)
> {
> return (eld[DRM_ELD_SAD_COUNT_CONN_TYPE] &
> DRM_ELD_SAD_COUNT_MASK) >>
> DRM_ELD_SAD_COUNT_SHIFT;
> @@ -111,7 +111,7 @@ static inline int drm_eld_sad_count(const uint8_t *eld)
> * This is a helper for determining the payload size of the baseline block,
> in
> * bytes, for e.g. setting the Baseline_ELD_Len field in the ELD header
> block.
> */
> -static inline int drm_eld_calc_baseline_block_size(const uint8_t *eld)
> +static inline int drm_eld_calc_baseline_block_size(const u8 *eld)
> {
> return DRM_ELD_MONITOR_NAME_STRING -
> DRM_ELD_HEADER_BLOCK_SIZE +
> drm_eld_mnl(eld) + drm_eld_sad_count(eld) * 3; @@ -127,7
> +127,7 @@ static inline int drm_eld_calc_baseline_block_size(const uint8_t
> *eld)
> *
> * The returned value is guaranteed to be a multiple of 4.
> */
> -static inline int drm_eld_size(const uint8_t *eld)
> +static inline int drm_eld_size(const u8 *eld)
> {
> return DRM_ELD_HEADER_BLOCK_SIZE +
> eld[DRM_ELD_BASELINE_ELD_LEN] * 4; } @@ -139,7 +139,7 @@ static inline
> int drm_eld_size(const uint8_t *eld)
> * The returned value is the speakers mask. User has to use
> %DRM_ELD_SPEAKER
> * field definitions to identify speakers.
> */
> -static inline u8 drm_eld_get_spk_alloc(const uint8_t *eld)
> +static inline u8 drm_eld_get_spk_alloc(const u8 *eld)
> {
> return eld[DRM_ELD_SPEAKER] & DRM_ELD_SPEAKER_MASK; } @@ -
> 151,7 +151,7 @@ static inline u8 drm_eld_get_spk_alloc(const uint8_t *eld)
> * The caller need to use %DRM_ELD_CONN_TYPE_HDMI or
> %DRM_ELD_CONN_TYPE_DP to
> * identify the display type connected.
> */
> -static inline u8 drm_eld_get_conn_type(const uint8_t *eld)
> +static inline u8 drm_eld_get_conn_type(const u8 *eld)
> {
> return eld[DRM_ELD_SAD_COUNT_CONN_TYPE] &
> DRM_ELD_CONN_TYPE_MASK; }
> --
> 2.39.2
[PATCH 5/5] drm/msm/mdp5: drop global_state_lock
Since the commit b962a12050a3 ("drm/atomic: integrate modeset lock with
private objects") the DRM framework no longer requires the external
lock for private objects. Drop the lock, letting the DRM to manage
private object locking.
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c | 8
drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.h | 1 -
2 files changed, 9 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
index 4d51a736d1d0..c15fa9dafe55 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
@@ -84,11 +84,6 @@ struct mdp5_global_state *mdp5_get_global_state(struct
drm_atomic_state *s)
struct msm_drm_private *priv = s->dev->dev_private;
struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(priv->kms));
struct drm_private_state *priv_state;
- int ret;
-
- ret = drm_modeset_lock(_kms->glob_state_lock, s->acquire_ctx);
- if (ret)
- return ERR_PTR(ret);
priv_state = drm_atomic_get_private_obj_state(s, _kms->glob_state);
if (IS_ERR(priv_state))
@@ -138,8 +133,6 @@ static int mdp5_global_obj_init(struct mdp5_kms *mdp5_kms)
{
struct mdp5_global_state *state;
- drm_modeset_lock_init(_kms->glob_state_lock);
-
state = kzalloc(sizeof(*state), GFP_KERNEL);
if (!state)
return -ENOMEM;
@@ -626,7 +619,6 @@ static void mdp5_destroy(struct mdp5_kms *mdp5_kms)
pm_runtime_disable(_kms->pdev->dev);
drm_atomic_private_obj_fini(_kms->glob_state);
- drm_modeset_lock_fini(_kms->glob_state_lock);
}
static int construct_pipes(struct mdp5_kms *mdp5_kms, int cnt,
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.h
b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.h
index 29bf11f08601..70fdc0b6c7c5 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.h
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.h
@@ -40,7 +40,6 @@ struct mdp5_kms {
* Global private object state, Do not access directly, use
* mdp5_global_get_state()
*/
- struct drm_modeset_lock glob_state_lock;
struct drm_private_obj glob_state;
struct mdp5_smp *smp;
--
2.39.2
[PATCH 4/5] drm/msm/mdp5: migrate SMP dumping to using atomic_print_state
The Shared Memory Pool (SMP) state is a part of the MDP5's private
object state. Use existing infrastructure, atomic_print_state()
callback, to dump SMP state (which also makes it included into
debugfs/dri/N/state). This allows us to drop the custom debugfs file
too.
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/disp/mdp5/mdp5_irq.c | 2 --
drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c | 46 ++--
drivers/gpu/drm/msm/disp/mdp5/mdp5_smp.c | 12 ++-
drivers/gpu/drm/msm/disp/mdp5/mdp5_smp.h | 4 ++-
4 files changed, 15 insertions(+), 49 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_irq.c
b/drivers/gpu/drm/msm/disp/mdp5/mdp5_irq.c
index 43443a435d59..b40ed3a847c8 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_irq.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_irq.c
@@ -31,8 +31,6 @@ static void mdp5_irq_error_handler(struct mdp_irq *irq,
uint32_t irqstatus)
if (dumpstate && __ratelimit()) {
struct drm_printer p = drm_info_printer(mdp5_kms->dev->dev);
drm_state_dump(mdp5_kms->dev, );
- if (mdp5_kms->smp)
- mdp5_smp_dump(mdp5_kms->smp, );
}
}
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
index 92bf9d949d09..4d51a736d1d0 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
@@ -119,9 +119,19 @@ static void mdp5_global_destroy_state(struct
drm_private_obj *obj,
kfree(mdp5_state);
}
+static void mdp5_global_print_state(struct drm_printer *p,
+ const struct drm_private_state *state)
+{
+ struct mdp5_global_state *mdp5_state = to_mdp5_global_state(state);
+
+ if (mdp5_state->mdp5_kms->smp)
+ mdp5_smp_dump(mdp5_state->mdp5_kms->smp, p, mdp5_state);
+}
+
static const struct drm_private_state_funcs mdp5_global_state_funcs = {
.atomic_duplicate_state = mdp5_global_duplicate_state,
.atomic_destroy_state = mdp5_global_destroy_state,
+ .atomic_print_state = mdp5_global_print_state,
};
static int mdp5_global_obj_init(struct mdp5_kms *mdp5_kms)
@@ -226,39 +236,6 @@ static void mdp5_kms_destroy(struct msm_kms *kms)
mdp5_destroy(mdp5_kms);
}
-#ifdef CONFIG_DEBUG_FS
-static int smp_show(struct seq_file *m, void *arg)
-{
- struct drm_info_node *node = m->private;
- struct drm_device *dev = node->minor->dev;
- struct msm_drm_private *priv = dev->dev_private;
- struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(priv->kms));
- struct drm_printer p = drm_seq_file_printer(m);
-
- if (!mdp5_kms->smp) {
- drm_printf(, "no SMP pool\n");
- return 0;
- }
-
- mdp5_smp_dump(mdp5_kms->smp, );
-
- return 0;
-}
-
-static struct drm_info_list mdp5_debugfs_list[] = {
- {"smp", smp_show },
-};
-
-static int mdp5_kms_debugfs_init(struct msm_kms *kms, struct drm_minor *minor)
-{
- drm_debugfs_create_files(mdp5_debugfs_list,
-ARRAY_SIZE(mdp5_debugfs_list),
-minor->debugfs_root, minor);
-
- return 0;
-}
-#endif
-
static const struct mdp_kms_funcs kms_funcs = {
.base = {
.hw_init = mdp5_hw_init,
@@ -277,9 +254,6 @@ static const struct mdp_kms_funcs kms_funcs = {
.get_format = mdp_get_format,
.set_split_display = mdp5_set_split_display,
.destroy = mdp5_kms_destroy,
-#ifdef CONFIG_DEBUG_FS
- .debugfs_init= mdp5_kms_debugfs_init,
-#endif
},
.set_irqmask = mdp5_set_irqmask,
};
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_smp.c
b/drivers/gpu/drm/msm/disp/mdp5/mdp5_smp.c
index 56a3063545ec..9a51fbf93cc4 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_smp.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_smp.c
@@ -325,22 +325,17 @@ void mdp5_smp_complete_commit(struct mdp5_smp *smp,
struct mdp5_smp_state *state
state->released = 0;
}
-void mdp5_smp_dump(struct mdp5_smp *smp, struct drm_printer *p)
+void mdp5_smp_dump(struct mdp5_smp *smp, struct drm_printer *p,
+ struct mdp5_global_state *global_state)
{
struct mdp5_kms *mdp5_kms = get_kms(smp);
struct mdp5_hw_pipe_state *hwpstate;
struct mdp5_smp_state *state;
- struct mdp5_global_state *global_state;
int total = 0, i, j;
drm_printf(p, "name\tinuse\tplane\n");
drm_printf(p, "\t-\t-\n");
- if (drm_can_sleep())
- drm_modeset_lock(_kms->glob_state_lock, NULL);
-
- global_state = mdp5_get_existing_global_state(mdp5_kms);
-
/* grab these *after* we hold the state_lock */
hwpstate = _state->hwpipe;
state = _state->smp;
@@ -365,9 +360,6 @@ void mdp5_smp_dump(struct mdp5_smp *smp, struct drm_printer
*p)
drm_printf(p,
[PATCH 0/5] drm/msm: cleanup private obj handling
While debugging one of the features in DRM/MSM I noticed that MSM
subdrivers still wrap private object access with manual modeset locking.
Since commit b962a12050a3 ("drm/atomic: integrate modeset lock with
private objects") this is no longer required, as the DRM framework
handles private objects internally. Drop these custom locks, while also
cleaning up the surrounding code.
Dmitry Baryshkov (5):
drm/atomic: add private obj state to state dump
drm/msm/dpu: finalise global state object
drm/msm/dpu: drop global_state_lock
drm/msm/mdp5: migrate SMP dumping to using atomic_print_state
drm/msm/mdp5: drop global_state_lock
drivers/gpu/drm/drm_atomic.c | 9
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 14 +++---
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 1 -
drivers/gpu/drm/msm/disp/mdp5/mdp5_irq.c | 2 -
drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c | 54 +---
drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.h | 1 -
drivers/gpu/drm/msm/disp/mdp5/mdp5_smp.c | 12 +-
drivers/gpu/drm/msm/disp/mdp5/mdp5_smp.h | 4 +-
8 files changed, 31 insertions(+), 66 deletions(-)
--
2.39.2
[PATCH 2/5] drm/msm/dpu: finalise global state object
Add calls to finalise global state object and corresponding lock.
Fixes: de3916c70a24 ("drm/msm/dpu: Track resources in global state")
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 8
1 file changed, 8 insertions(+)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index aa6ba2cf4b84..fba2294e329f 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -385,6 +385,12 @@ static int dpu_kms_global_obj_init(struct dpu_kms *dpu_kms)
return 0;
}
+static void dpu_kms_global_obj_fini(struct dpu_kms *dpu_kms)
+{
+ drm_atomic_private_obj_fini(_kms->global_state);
+ drm_modeset_lock_fini(_kms->global_state_lock);
+}
+
static int dpu_kms_parse_data_bus_icc_path(struct dpu_kms *dpu_kms)
{
struct icc_path *path0;
@@ -827,6 +833,8 @@ static void _dpu_kms_hw_destroy(struct dpu_kms *dpu_kms)
dpu_rm_destroy(_kms->rm);
dpu_kms->rm_init = false;
+ dpu_kms_global_obj_fini(dpu_kms);
+
dpu_kms->catalog = NULL;
if (dpu_kms->vbif[VBIF_NRT])
--
2.39.2
[PATCH 3/5] drm/msm/dpu: drop global_state_lock
Since the commit b962a12050a3 ("drm/atomic: integrate modeset lock with
private objects") the DRM framework no longer requires the external
lock for private objects. Drop the lock, letting the DRM to manage
private object locking.
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 8
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 1 -
2 files changed, 9 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index fba2294e329f..ee84160592ce 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -328,11 +328,6 @@ struct dpu_global_state *dpu_kms_get_global_state(struct
drm_atomic_state *s)
struct msm_drm_private *priv = s->dev->dev_private;
struct dpu_kms *dpu_kms = to_dpu_kms(priv->kms);
struct drm_private_state *priv_state;
- int ret;
-
- ret = drm_modeset_lock(_kms->global_state_lock, s->acquire_ctx);
- if (ret)
- return ERR_PTR(ret);
priv_state = drm_atomic_get_private_obj_state(s,
_kms->global_state);
@@ -373,8 +368,6 @@ static int dpu_kms_global_obj_init(struct dpu_kms *dpu_kms)
{
struct dpu_global_state *state;
- drm_modeset_lock_init(_kms->global_state_lock);
-
state = kzalloc(sizeof(*state), GFP_KERNEL);
if (!state)
return -ENOMEM;
@@ -388,7 +381,6 @@ static int dpu_kms_global_obj_init(struct dpu_kms *dpu_kms)
static void dpu_kms_global_obj_fini(struct dpu_kms *dpu_kms)
{
drm_atomic_private_obj_fini(_kms->global_state);
- drm_modeset_lock_fini(_kms->global_state_lock);
}
static int dpu_kms_parse_data_bus_icc_path(struct dpu_kms *dpu_kms)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
index b6f53ca6e962..ed549f0f7c65 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
@@ -84,7 +84,6 @@ struct dpu_kms {
* Global private object state, Do not access directly, use
* dpu_kms_global_get_state()
*/
- struct drm_modeset_lock global_state_lock;
struct drm_private_obj global_state;
struct dpu_rm rm;
--
2.39.2
[PATCH 1/5] drm/atomic: add private obj state to state dump
The drm_atomic_print_new_state() already prints private object state via
drm_atomic_private_obj_print_state(). Add private object state dumping
to __drm_state_dump(), so that it is also included into drm_state_dump()
output and into debugfs/dri/N/state file.
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/drm_atomic.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index c277b198fa3f..9543e284dc15 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1773,6 +1773,7 @@ static void __drm_state_dump(struct drm_device *dev,
struct drm_printer *p,
struct drm_crtc *crtc;
struct drm_connector *connector;
struct drm_connector_list_iter conn_iter;
+ struct drm_private_obj *obj;
if (!drm_drv_uses_atomic_modeset(dev))
return;
@@ -1801,6 +1802,14 @@ static void __drm_state_dump(struct drm_device *dev,
struct drm_printer *p,
if (take_locks)
drm_modeset_unlock(>mode_config.connection_mutex);
drm_connector_list_iter_end(_iter);
+
+ list_for_each_entry(obj, >privobj_list, head) {
+ if (take_locks)
+ drm_modeset_lock(>lock, NULL);
+ drm_atomic_private_obj_print_state(p, obj->state);
+ if (take_locks)
+ drm_modeset_unlock(>lock);
+ }
}
/**
--
2.39.2
Re: [git pull] drm fixes for 6.6-rc1
The pull request you sent on Fri, 8 Sep 2023 12:45:13 +1000: > git://anongit.freedesktop.org/drm/drm tags/drm-next-2023-09-08 has been merged into torvalds/linux.git: https://git.kernel.org/torvalds/c/a48fa7efaf1161c1c898931fe4c7f0070964233a Thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/prtracker.html
Re: [git pull] drm fixes for 6.6-rc1
On Thu, 7 Sept 2023 at 19:45, Dave Airlie wrote: > > Just a poke about the outstanding drm CI support pull request since I > haven't see any movement on that in the week, hopefully it's not as > difficult a problem as bcachefs :-) I was assuming that it wouldn't interfere with anything else... and that I could just ignore it until I have all my "real" pulls done. I didn't want to even look at it until I was "done". Yes, it's past the mid-way point of the second week of the merge window, and I mostly _should_ be "done". But I still keep finding new pull requests in my inbox that aren't just fixes and updates for previous main pull requests. Linus
[git pull] drm fixes for 6.6-rc1
Hey Linus, Regular rounds of rc1 fixes, a large bunch for amdgpu since it's 3 weeks in one go, one i915, one nouveau and one ivpu. I think there might be a few more fixes in misc that I haven't pulled in yet, but we should get them all for rc2. Just a poke about the outstanding drm CI support pull request since I haven't see any movement on that in the week, hopefully it's not as difficult a problem as bcachefs :-) Thanks, Dave. drm-next-2023-09-08: drm fixes for 6.6-rc1 amdgpu: - Display replay fixes - Fixes for headless boards - Fix documentation breakage - RAS fixes - Handle newer IP discovery tables - SMU 13.0.6 fixes - SR-IOV fixes - Display vstartup fixes - NBIO 7.9 fixes - Display scaling mode fixes - Debugfs power reporting fix - GC 9.4.3 fixes - Dirty framebuffer fixes for fbcon - eDP fixes - DCN 3.1.5 fix - Display ODM fixes - GPU core dump fix - Re-enable zops property now that IGT test is fixed - Fix possible UAF in CS code - Cursor degamma fix amdkfd: - HMM fixes - Interrupt masking fix - GFX11 MQD fixes i915: - Mark requests for GuC virtual engines to avoid use-after-free nouveau: - Fix fence state in nouveau_fence_emit() ivpu: - replace strncpy The following changes since commit 3698a75f5a98d0a6599e2878ab25d30a82dd836a: Merge tag 'drm-intel-next-fixes-2023-08-24' of git://anongit.freedesktop.org/drm/drm-intel into drm-next (2023-08-25 12:55:55 +1000) are available in the Git repository at: git://anongit.freedesktop.org/drm/drm tags/drm-next-2023-09-08 for you to fetch changes up to 43ffcd6fa1635f479ad73145dfbba59edc2b3b28: Merge tag 'amd-drm-fixes-6.6-2023-09-06' of https://gitlab.freedesktop.org/agd5f/linux into drm-next (2023-09-08 10:44:07 +1000) drm fixes for 6.6-rc1 amdgpu: - Display replay fixes - Fixes for headless boards - Fix documentation breakage - RAS fixes - Handle newer IP discovery tables - SMU 13.0.6 fixes - SR-IOV fixes - Display vstartup fixes - NBIO 7.9 fixes - Display scaling mode fixes - Debugfs power reporting fix - GC 9.4.3 fixes - Dirty framebuffer fixes for fbcon - eDP fixes - DCN 3.1.5 fix - Display ODM fixes - GPU core dump fix - Re-enable zops property now that IGT test is fixed - Fix possible UAF in CS code - Cursor degamma fix amdkfd: - HMM fixes - Interrupt masking fix - GFX11 MQD fixes i915: - Mark requests for GuC virtual engines to avoid use-after-free nouveau: - Fix fence state in nouveau_fence_emit() ivpu: - replace strncpy Alex Deucher (1): drm/amd/pm: fix debugfs pm_info output Alex Sierra (2): drm/amdkfd: retry after EBUSY is returned from hmm_ranges_get_pages drm/amdkfd: use mask to get v9 interrupt sq data bits correctly Andrzej Hajda (1): drm/i915: mark requests for GuC virtual engines to avoid use-after-free André Almeida (1): drm/amdgpu: Allocate coredump memory in a nonblocking way Asad Kamal (3): drm/amd/pm: Update SMUv13.0.6 PMFW headers drm/amd/pm: Add critical temp for GC v9.4.3 drm/amd/pm: Fix critical temp unit of SMU v13.0.6 Bhawanpreet Lakha (1): drm/amd/display: Enable Replay for static screen use cases Bokun Zhang (1): drm/amdgpu/pm: Add notification for no DC support Candice Li (1): drm/amdgpu: Only support RAS EEPROM on dGPU platform Christian König (1): drm/amdgpu: fix amdgpu_cs_p1_user_fence ChunTao Tso (1): drm/amd/display: set minimum of VBlank_nom Danilo Krummrich (1): drm/nouveau: fence: fix undefined fence state after emit Dave Airlie (3): Merge tag 'drm-misc-next-fixes-2023-09-01' of git://anongit.freedesktop.org/drm/drm-misc into drm-next Merge tag 'drm-intel-next-fixes-2023-08-31' of git://anongit.freedesktop.org/drm/drm-intel into drm-next Merge tag 'amd-drm-fixes-6.6-2023-09-06' of https://gitlab.freedesktop.org/agd5f/linux into drm-next Fudong Wang (1): drm/amd/display: Add smu write msg id fail retry process Gabe Teeger (1): drm/amd/display: Remove wait while locked Hamza Mahfooz (7): drm/amd/display: fix mode scaling (RMX_.*) drm/amdgpu: register a dirty framebuffer callback for fbcon drm/amd/display: register edp_backlight_control() for DCN301 Revert "Revert "drm/amd/display: Implement zpos property"" Revert "drm/amd/display: Remove v_startup workaround for dcn3+" drm/amd/display: limit the v_startup workaround to ASICs older than DCN3.1 drm/amd/display: prevent potential division by zero errors Hawking Zhang (4): drm/amdgpu: Fix the return for gpu mode1_reset drm/amdgpu: Add umc_info v4_0 structure drm/amdgpu: Support query ecc cap for aqua_vanjaram drm/amdgpu: Free ras cmd input buffer properly Horace Chen (1): drm/amdkfd: use correct method to get clock under SRIOV Jay Cornwall (1): drm/amdkfd: Add missing gfx11 MQD manager callbacks Justin Stitt (1):
Re: [RFC PATCH v2 06/11] page-pool: add device memory support
On 19/08/2023 13:24, Mina Almasry wrote: > On Sat, Aug 19, 2023 at 8:22 AM Jesper Dangaard Brouer > wrote: >> >> >> >> On 19/08/2023 16.08, Willem de Bruijn wrote: >>> On Sat, Aug 19, 2023 at 5:51 AM Jesper Dangaard Brouer >>> wrote: On 10/08/2023 03.57, Mina Almasry wrote: > Overload the LSB of struct page* to indicate that it's a page_pool_iov. > > Refactor mm calls on struct page * into helpers, and add page_pool_iov > handling on those helpers. Modify callers of these mm APIs with calls to > these helpers instead. > I don't like of this approach. This is adding code to the PP (page_pool) fast-path in multiple places. I've not had time to run my usual benchmarks, which are here: https://github.com/netoptimizer/prototype-kernel/blob/master/kernel/lib/bench_page_pool_simple.c > > Thank you for linking that, I'll try to run these against the next submission. > But I'm sure it will affect performance. Regardless of performance, this approach is using ptr-LSB-bits, to hide that page-pointer are not really struct-pages, feels like force feeding a solution just to use the page_pool APIs. > In areas where struct page* is dereferenced, add a check for special > handling of page_pool_iov. > > The memory providers producing page_pool_iov can set the LSB on the > struct page* returned to the page pool. > > Note that instead of overloading the LSB of page pointers, we can > instead define a new union between struct page & struct page_pool_iov and > compact it in a new type. However, we'd need to implement the code churn > to modify the page_pool & drivers to use this new type. For this POC > that is not implemented (feedback welcome). > I've said before, that I prefer multiplexing on page->pp_magic. For your page_pool_iov the layout would have to match the offset of pp_magic, to do this. (And if insisting on using PP infra the refcnt would also need to align). >>> >>> Perhaps I misunderstand, but this suggests continuing to using >>> struct page to demultiplex memory type? >>> >> >> (Perhaps we are misunderstanding each-other and my use of the words >> multiplexing and demultiplex are wrong, I'm sorry, as English isn't my >> native language.) >> >> I do see the problem of depending on having a struct page, as the >> page_pool_iov isn't related to struct page. Having "page" in the name >> of "page_pool_iov" is also confusing (hardest problem is CS is naming, >> as we all know). >> >> To support more allocator types, perhaps skb->pp_recycle bit need to >> grow another bit (and be renamed skb->recycle), so we can tell allocator >> types apart, those that are page based and those whom are not. >> >> >>> I think the feedback has been strong to not multiplex yet another >>> memory type into that struct, that is not a real page. Which is why >>> we went into this direction. This latest series limits the impact largely >>> to networking structures and code. >>> >> >> Some what related what I'm objecting to: the "page_pool_iov" is not a >> real page, but this getting recycled into something called "page_pool", >> which funny enough deals with struct-pages internally and depend on the >> struct-page-refcnt. >> >> Given the approach changed way from using struct page, then I also don't >> see the connection with the page_pool. Sorry. >> >>> One way or another, there will be a branch and multiplexing. Whether >>> that is in struct page, the page pool or a new netdev mem type as you >>> propose. >>> >> >> I'm asking to have this branch/multiplexing done a the call sites. >> >> (IMHO not changing the drivers is a pipe-dream.) >> > > I think I understand what Jesper is saying. I think Jesper wants the > page_pool to remain unchanged, and another layer on top of it to do > the multiplexing, i.e.: > > driver ---> new_layer (does multiplexing) ---> page_pool ---> mm layer > \--> > devmem_pool --> dma-buf layer > > But, I think, Jakub wants the page_pool to be the front end, and for > the multiplexing to happen in the page pool (I think, Jakub did not > write this in an email, but this is how I interpret his comments from > [1], and his memory provider RFC). So I think Jakub wants: > > driver --> page_pool ---> memory_provider (does multiplexing) ---> > basic_provider ---> mm layer > > \> devmem_provider --> dma-buf > layer > > That is the approach in this RFC. > > I think proper naming that makes sense can be figured out, and is not > a huge issue. I think in both cases we can minimize the changes to the > drivers, maybe. In the first approach the driver will need to use the > APIs created by the new layer. In the second approach, the driver > continues to use page_pool APIs. > > I think we need to converge on a path between
Re: [PATCH v2] drm/msm/dpu: change _dpu_plane_calc_bw() to use u64 to avoid overflow
On 08/09/2023 04:26, Abhinav Kumar wrote:
_dpu_plane_calc_bw() uses integer variables to calculate the bandwidth
used during plane bandwidth calculations. However for high resolution
displays this overflows easily and leads to below errors
[dpu error]crtc83 failed performance check -7
Promote the intermediate variables to u64 to avoid overflow.
changes in v2:
- change to u64 where actually needed in the math
Fixes: c33b7c0389e1 ("drm/msm/dpu: add support for clk and bw scaling for
display")
Closes: https://gitlab.freedesktop.org/drm/msm/-/issues/32
Signed-off-by: Abhinav Kumar
---
drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
Reviewed-by: Dmitry Baryshkov
--
With best wishes
Dmitry
[PATCH v2] drm/msm/dpu: change _dpu_plane_calc_bw() to use u64 to avoid overflow
_dpu_plane_calc_bw() uses integer variables to calculate the bandwidth
used during plane bandwidth calculations. However for high resolution
displays this overflows easily and leads to below errors
[dpu error]crtc83 failed performance check -7
Promote the intermediate variables to u64 to avoid overflow.
changes in v2:
- change to u64 where actually needed in the math
Fixes: c33b7c0389e1 ("drm/msm/dpu: add support for clk and bw scaling for
display")
Closes: https://gitlab.freedesktop.org/drm/msm/-/issues/32
Signed-off-by: Abhinav Kumar
---
drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
index c2aaaded07ed..98c1b22e9bca 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
@@ -119,6 +119,7 @@ static u64 _dpu_plane_calc_bw(const struct dpu_mdss_cfg
*catalog,
struct dpu_sw_pipe_cfg *pipe_cfg)
{
int src_width, src_height, dst_height, fps;
+ u64 plane_pixel_rate, plane_bit_rate;
u64 plane_prefill_bw;
u64 plane_bw;
u32 hw_latency_lines;
@@ -136,13 +137,12 @@ static u64 _dpu_plane_calc_bw(const struct dpu_mdss_cfg
*catalog,
scale_factor = src_height > dst_height ?
mult_frac(src_height, 1, dst_height) : 1;
- plane_bw =
- src_width * mode->vtotal * fps * fmt->bpp *
- scale_factor;
+ plane_pixel_rate = src_width * mode->vtotal * fps;
+ plane_bit_rate = plane_pixel_rate * fmt->bpp;
- plane_prefill_bw =
- src_width * hw_latency_lines * fps * fmt->bpp *
- scale_factor * mode->vtotal;
+ plane_bw = plane_bit_rate * scale_factor;
+
+ plane_prefill_bw = plane_bw * hw_latency_lines;
if ((vbp+vpw) > hw_latency_lines)
do_div(plane_prefill_bw, (vbp+vpw));
--
2.40.1
Re: [PATCH] drm/msm/dpu: change _dpu_plane_calc_bw() to use u64 to avoid overflow
On 08/09/2023 03:52, Abhinav Kumar wrote:
On 9/7/2023 5:35 PM, Dmitry Baryshkov wrote:
On 08/09/2023 03:32, Abhinav Kumar wrote:
_dpu_plane_calc_bw() uses integer variables to calculate the bandwidth
used during plane bandwidth calculations. However for high resolution
displays this overflows easily and leads to below errors
Can we move the u64 conversion closer to the place where we actually
need it? Having u64 source width looks very strange.
Its this math
https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c#L139 which overflows.
So its not just one variable but I can even change this to u32 as that
also fixes the issue. Let me know if u32 works better.
Perhaps I went too far to go from int to u64.
I'd prefer to have the u64 conversion around the actual calculations -
so that we emphasise the issue, not the size of the width / etc.
[dpu error]crtc83 failed performance check -7
Promote the intermediate variables to u64 to avoid overflow.
Fixes: c33b7c0389e1 ("drm/msm/dpu: add support for clk and bw scaling
for display")
Closes: https://gitlab.freedesktop.org/drm/msm/-/issues/32
Signed-off-by: Abhinav Kumar
---
drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
index ae970af1154f..c6193131beec 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
@@ -118,7 +118,7 @@ static u64 _dpu_plane_calc_bw(const struct
dpu_mdss_cfg *catalog,
const struct drm_display_mode *mode,
struct dpu_sw_pipe_cfg *pipe_cfg)
{
- int src_width, src_height, dst_height, fps;
+ u64 src_width, src_height, dst_height, fps;
u64 plane_prefill_bw;
u64 plane_bw;
u32 hw_latency_lines;
--
With best wishes
Dmitry
Re: [PATCH] drm/msm/dpu: change _dpu_plane_calc_bw() to use u64 to avoid overflow
On 9/7/2023 5:35 PM, Dmitry Baryshkov wrote:
On 08/09/2023 03:32, Abhinav Kumar wrote:
_dpu_plane_calc_bw() uses integer variables to calculate the bandwidth
used during plane bandwidth calculations. However for high resolution
displays this overflows easily and leads to below errors
Can we move the u64 conversion closer to the place where we actually
need it? Having u64 source width looks very strange.
Its this math
https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c#L139
which overflows.
So its not just one variable but I can even change this to u32 as that
also fixes the issue. Let me know if u32 works better.
Perhaps I went too far to go from int to u64.
[dpu error]crtc83 failed performance check -7
Promote the intermediate variables to u64 to avoid overflow.
Fixes: c33b7c0389e1 ("drm/msm/dpu: add support for clk and bw scaling
for display")
Closes: https://gitlab.freedesktop.org/drm/msm/-/issues/32
Signed-off-by: Abhinav Kumar
---
drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
index ae970af1154f..c6193131beec 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
@@ -118,7 +118,7 @@ static u64 _dpu_plane_calc_bw(const struct
dpu_mdss_cfg *catalog,
const struct drm_display_mode *mode,
struct dpu_sw_pipe_cfg *pipe_cfg)
{
- int src_width, src_height, dst_height, fps;
+ u64 src_width, src_height, dst_height, fps;
u64 plane_prefill_bw;
u64 plane_bw;
u32 hw_latency_lines;
Re: [RFC PATCH v2 02/11] netdev: implement netlink api to bind dma-buf to netdevice
On 09/08/2023 18:57, Mina Almasry wrote:
> Add a netdev_dmabuf_binding struct which represents the
> dma-buf-to-netdevice binding. The netlink API will bind the dma-buf to
> an rx queue on the netdevice. On the binding, the dma_buf_attach
> & dma_buf_map_attachment will occur. The entries in the sg_table from
> mapping will be inserted into a genpool to make it ready
> for allocation.
>
> The chunks in the genpool are owned by a dmabuf_chunk_owner struct which
> holds the dma-buf offset of the base of the chunk and the dma_addr of
> the chunk. Both are needed to use allocations that come from this chunk.
>
> We create a new type that represents an allocation from the genpool:
> page_pool_iov. We setup the page_pool_iov allocation size in the
> genpool to PAGE_SIZE for simplicity: to match the PAGE_SIZE normally
> allocated by the page pool and given to the drivers.
>
> The user can unbind the dmabuf from the netdevice by closing the netlink
> socket that established the binding. We do this so that the binding is
> automatically unbound even if the userspace process crashes.
>
> The binding and unbinding leaves an indicator in struct netdev_rx_queue
> that the given queue is bound, but the binding doesn't take effect until
> the driver actually reconfigures its queues, and re-initializes its page
> pool. This issue/weirdness is highlighted in the memory provider
> proposal[1], and I'm hoping that some generic solution for all
> memory providers will be discussed; this patch doesn't address that
> weirdness again.
>
> The netdev_dmabuf_binding struct is refcounted, and releases its
> resources only when all the refs are released.
>
> [1] https://lore.kernel.org/netdev/20230707183935.997267-1-k...@kernel.org/
>
> Signed-off-by: Willem de Bruijn
> Signed-off-by: Kaiyuan Zhang
>
> Signed-off-by: Mina Almasry
> ---
> include/linux/netdevice.h | 57
> include/net/page_pool.h | 27 ++
> net/core/dev.c| 178 ++
> net/core/netdev-genl.c| 101 -
> 4 files changed, 361 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 3800d0479698..1b7c5966d2ca 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -53,6 +53,8 @@
> #include
> #include
> #include
> +#include
> +#include
>
> struct netpoll_info;
> struct device;
> @@ -782,6 +784,55 @@ bool rps_may_expire_flow(struct net_device *dev, u16
> rxq_index, u32 flow_id,
> #endif
> #endif /* CONFIG_RPS */
>
> +struct netdev_dmabuf_binding {
> + struct dma_buf *dmabuf;
> + struct dma_buf_attachment *attachment;
> + struct sg_table *sgt;
> + struct net_device *dev;
> + struct gen_pool *chunk_pool;
> +
> + /* The user holds a ref (via the netlink API) for as long as they want
> + * the binding to remain alive. Each page pool using this binding holds
> + * a ref to keep the binding alive. Each allocated page_pool_iov holds a
> + * ref.
> + *
> + * The binding undos itself and unmaps the underlying dmabuf once all
> + * those refs are dropped and the binding is no longer desired or in
> + * use.
> + */
> + refcount_t ref;
> +
> + /* The portid of the user that owns this binding. Used for netlink to
> + * notify us of the user dropping the bind.
> + */
> + u32 owner_nlportid;
> +
> + /* The list of bindings currently active. Used for netlink to notify us
> + * of the user dropping the bind.
> + */
> + struct list_head list;
> +
> + /* rxq's this binding is active on. */
> + struct xarray bound_rxq_list;
> +};
> +
> +void __netdev_devmem_binding_free(struct netdev_dmabuf_binding *binding);
> +
> +static inline void
> +netdev_devmem_binding_get(struct netdev_dmabuf_binding *binding)
> +{
> + refcount_inc(>ref);
> +}
> +
> +static inline void
> +netdev_devmem_binding_put(struct netdev_dmabuf_binding *binding)
> +{
> + if (!refcount_dec_and_test(>ref))
> + return;
> +
> + __netdev_devmem_binding_free(binding);
> +}
> +
> /* This structure contains an instance of an RX queue. */
> struct netdev_rx_queue {
> struct xdp_rxq_info xdp_rxq;
> @@ -796,6 +847,7 @@ struct netdev_rx_queue {
> #ifdef CONFIG_XDP_SOCKETS
> struct xsk_buff_pool*pool;
> #endif
> + struct netdev_dmabuf_binding*binding;
> } cacheline_aligned_in_smp;
>
> /*
> @@ -5026,6 +5078,11 @@ void netif_set_tso_max_segs(struct net_device *dev,
> unsigned int segs);
> void netif_inherit_tso_max(struct net_device *to,
> const struct net_device *from);
>
> +void netdev_unbind_dmabuf_to_queue(struct netdev_dmabuf_binding *binding);
> +int netdev_bind_dmabuf_to_queue(struct net_device *dev, unsigned int
> dmabuf_fd,
> + u32 rxq_idx,
> + struct netdev_dmabuf_binding
Re: [PATCH] drm/msm/dpu: change _dpu_plane_calc_bw() to use u64 to avoid overflow
On 08/09/2023 03:32, Abhinav Kumar wrote:
_dpu_plane_calc_bw() uses integer variables to calculate the bandwidth
used during plane bandwidth calculations. However for high resolution
displays this overflows easily and leads to below errors
Can we move the u64 conversion closer to the place where we actually
need it? Having u64 source width looks very strange.
[dpu error]crtc83 failed performance check -7
Promote the intermediate variables to u64 to avoid overflow.
Fixes: c33b7c0389e1 ("drm/msm/dpu: add support for clk and bw scaling for
display")
Closes: https://gitlab.freedesktop.org/drm/msm/-/issues/32
Signed-off-by: Abhinav Kumar
---
drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
index ae970af1154f..c6193131beec 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
@@ -118,7 +118,7 @@ static u64 _dpu_plane_calc_bw(const struct dpu_mdss_cfg
*catalog,
const struct drm_display_mode *mode,
struct dpu_sw_pipe_cfg *pipe_cfg)
{
- int src_width, src_height, dst_height, fps;
+ u64 src_width, src_height, dst_height, fps;
u64 plane_prefill_bw;
u64 plane_bw;
u32 hw_latency_lines;
--
With best wishes
Dmitry
[PATCH] drm/msm/dpu: change _dpu_plane_calc_bw() to use u64 to avoid overflow
_dpu_plane_calc_bw() uses integer variables to calculate the bandwidth
used during plane bandwidth calculations. However for high resolution
displays this overflows easily and leads to below errors
[dpu error]crtc83 failed performance check -7
Promote the intermediate variables to u64 to avoid overflow.
Fixes: c33b7c0389e1 ("drm/msm/dpu: add support for clk and bw scaling for
display")
Closes: https://gitlab.freedesktop.org/drm/msm/-/issues/32
Signed-off-by: Abhinav Kumar
---
drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
index ae970af1154f..c6193131beec 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
@@ -118,7 +118,7 @@ static u64 _dpu_plane_calc_bw(const struct dpu_mdss_cfg
*catalog,
const struct drm_display_mode *mode,
struct dpu_sw_pipe_cfg *pipe_cfg)
{
- int src_width, src_height, dst_height, fps;
+ u64 src_width, src_height, dst_height, fps;
u64 plane_prefill_bw;
u64 plane_bw;
u32 hw_latency_lines;
--
2.40.1
Re: [PATCH] drm/msm/dp: support setting the DP subconnector type
Quoting Dmitry Baryshkov (2023-09-07 14:48:54) > On 08/09/2023 00:34, Stephen Boyd wrote: > > Quoting Dmitry Baryshkov (2023-09-03 15:24:32) > >> diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c > >> b/drivers/gpu/drm/msm/dp/dp_panel.c > >> index 97ba41593820..1cb54f26f5aa 100644 > >> --- a/drivers/gpu/drm/msm/dp/dp_panel.c > >> +++ b/drivers/gpu/drm/msm/dp/dp_panel.c > >> @@ -162,6 +162,11 @@ int dp_panel_read_sink_caps(struct dp_panel *dp_panel, > >> } > >> } > >> > >> + rc = drm_dp_read_downstream_info(panel->aux, dp_panel->dpcd, > >> +dp_panel->downstream_ports); > >> + if (rc) > >> + return rc; > > > > I haven't been able to test it yet, but I think with an apple dongle > > we'll never populate the 'downstream_ports' member if the HDMI cable is > > not connected when this runs. That's because this function bails out > > early before trying to read the downstream ports when there isn't a > > sink. Perhaps we need to read it again when an hpd_irq comes in, or we > > need to read it before bailing out from here? > > I don't have an Apple dongle here. But I'll run a check with first > connecting the dongle and plugging the HDMI afterwards. > > However my expectation based on my previous tests is that we only get > here when the actual display is connected. > We get here when HPD is high. With an apple dongle, hpd is high when just the dongle is plugged in. That calls dp_display_process_hpd_high() which calls dp_panel_read_sink_caps(), but that returns with an error (-ENOTCONN) and then we wait for something to change. When the HDMI cable is plugged in (i.e. an actual display) we get an irq_hpd. That causes dp_irq_hpd_handle() to call dp_display_usbpd_attention_cb() which calls dp_link_process_request() that sees 'sink_request & DS_PORT_STATUS_CHANGED' and thus calls dp_display_handle_port_ststus_changed() (that has a typo right?) which hits the else condition and calls dp_display_process_hpd_high(). So yes? We will eventually call dp_panel_read_sink_caps() again, and this time not bail out early. It's probably fine.
Re: [PATCH] drm/msm/dp: support setting the DP subconnector type
On 08/09/2023 00:34, Stephen Boyd wrote: Quoting Dmitry Baryshkov (2023-09-03 15:24:32) diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c b/drivers/gpu/drm/msm/dp/dp_panel.c index 97ba41593820..1cb54f26f5aa 100644 --- a/drivers/gpu/drm/msm/dp/dp_panel.c +++ b/drivers/gpu/drm/msm/dp/dp_panel.c @@ -162,6 +162,11 @@ int dp_panel_read_sink_caps(struct dp_panel *dp_panel, } } + rc = drm_dp_read_downstream_info(panel->aux, dp_panel->dpcd, +dp_panel->downstream_ports); + if (rc) + return rc; I haven't been able to test it yet, but I think with an apple dongle we'll never populate the 'downstream_ports' member if the HDMI cable is not connected when this runs. That's because this function bails out early before trying to read the downstream ports when there isn't a sink. Perhaps we need to read it again when an hpd_irq comes in, or we need to read it before bailing out from here? I don't have an Apple dongle here. But I'll run a check with first connecting the dongle and plugging the HDMI afterwards. However my expectation based on my previous tests is that we only get here when the actual display is connected. -- With best wishes Dmitry
Re: [PATCH] drm/msm/dp: skip validity check for DP CTS EDID checksum
Quoting Jani Nikula (2023-09-01 07:20:34)
> The DP CTS test for EDID last block checksum expects the checksum for
> the last block, invalid or not. Skip the validity check.
>
> For the most part (*), the EDIDs returned by drm_get_edid() will be
> valid anyway, and there's the CTS workaround to get the checksum for
> completely invalid EDIDs. See commit 7948fe12d47a ("drm/msm/dp: return
> correct edid checksum after corrupted edid checksum read").
>
> This lets us remove one user of drm_edid_block_valid() with hopes the
> function can be removed altogether in the future.
>
> (*) drm_get_edid() ignores checksum errors on CTA extensions.
>
> Cc: Abhinav Kumar
> Cc: Dmitry Baryshkov
> Cc: Kuogee Hsieh
> Cc: Marijn Suijten
> Cc: Rob Clark
> Cc: Sean Paul
> Cc: Stephen Boyd
> Cc: linux-arm-...@vger.kernel.org
> Cc: freedr...@lists.freedesktop.org
> Signed-off-by: Jani Nikula
> ---
Reviewed-by: Stephen Boyd
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c
> b/drivers/gpu/drm/msm/dp/dp_panel.c
> index 42d52510ffd4..86a8e06c7a60 100644
> --- a/drivers/gpu/drm/msm/dp/dp_panel.c
> +++ b/drivers/gpu/drm/msm/dp/dp_panel.c
> @@ -289,26 +289,9 @@ int dp_panel_get_modes(struct dp_panel *dp_panel,
>
> static u8 dp_panel_get_edid_checksum(struct edid *edid)
It would be nice to make 'edid' const here in another patch.
Re: [PATCH] drm/msm/dp: support setting the DP subconnector type
Quoting Dmitry Baryshkov (2023-09-03 15:24:32) > diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c > b/drivers/gpu/drm/msm/dp/dp_panel.c > index 97ba41593820..1cb54f26f5aa 100644 > --- a/drivers/gpu/drm/msm/dp/dp_panel.c > +++ b/drivers/gpu/drm/msm/dp/dp_panel.c > @@ -162,6 +162,11 @@ int dp_panel_read_sink_caps(struct dp_panel *dp_panel, > } > } > > + rc = drm_dp_read_downstream_info(panel->aux, dp_panel->dpcd, > +dp_panel->downstream_ports); > + if (rc) > + return rc; I haven't been able to test it yet, but I think with an apple dongle we'll never populate the 'downstream_ports' member if the HDMI cable is not connected when this runs. That's because this function bails out early before trying to read the downstream ports when there isn't a sink. Perhaps we need to read it again when an hpd_irq comes in, or we need to read it before bailing out from here?
Re: [Intel-gfx] [PATCH v2] drm/i915/huc: silence injected failure in the load via GSC path
On 8/16/2023 16:13, Daniele Ceraolo Spurio wrote:
If we can't load the HuC due to an injected failure, we don't want
to throw and error and trip CI. Using the gt_probe_error macro for
logging ensure that the error is only printed if it wasn't explicitly
injected.
v2: keep the line to less than 100 characters (checkpatch).
Link: https://gitlab.freedesktop.org/drm/intel/-/issues/7061
Signed-off-by: Daniele Ceraolo Spurio
Reviewed-by: Andi Shyti #v1
Reviewed-by: John Harrison
Although aren't we supposed to be using %pe / PTR_ERR(ret) these days?
Not a blocker but for future reference.
John.
---
drivers/gpu/drm/i915/pxp/intel_pxp_tee.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
index f89a1f80f50e..bb58fa9579b8 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
@@ -9,6 +9,7 @@
#include
#include "gem/i915_gem_lmem.h"
+#include "gt/intel_gt_print.h"
#include "i915_drv.h"
#include "gt/intel_gt.h"
@@ -156,7 +157,8 @@ static int i915_pxp_tee_component_bind(struct device
*i915_kdev,
{
struct drm_i915_private *i915 = kdev_to_i915(i915_kdev);
struct intel_pxp *pxp = i915->pxp;
- struct intel_uc *uc = >ctrl_gt->uc;
+ struct intel_gt *gt = pxp->ctrl_gt;
+ struct intel_uc *uc = >uc;
intel_wakeref_t wakeref;
int ret = 0;
@@ -176,7 +178,7 @@ static int i915_pxp_tee_component_bind(struct device *i915_kdev,
/* load huc via pxp */
ret = intel_huc_fw_load_and_auth_via_gsc(>huc);
if (ret < 0)
- drm_err(>drm, "failed to load huc via gsc
%d\n", ret);
+ gt_probe_error(gt, "failed to load huc via gsc
%d\n", ret);
}
}
Re: [RFC 01/33] drm/doc/rfc: Add RFC document for proposed Plane Color Pipeline
On 8/29/2023 12:03 PM, Uma Shankar wrote: Add the documentation for the new proposed Plane Color Pipeline. Co-developed-by: Chaitanya Kumar Borah Signed-off-by: Chaitanya Kumar Borah Signed-off-by: Uma Shankar --- .../gpu/rfc/plane_color_pipeline.rst | 394 ++ 1 file changed, 394 insertions(+) create mode 100644 Documentation/gpu/rfc/plane_color_pipeline.rst diff --git a/Documentation/gpu/rfc/plane_color_pipeline.rst b/Documentation/gpu/rfc/plane_color_pipeline.rst new file mode 100644 index ..60ce515b6ea7 --- /dev/null +++ b/Documentation/gpu/rfc/plane_color_pipeline.rst @@ -0,0 +1,394 @@ +=== + Plane Color Pipeline: A UAPI proposal +=== + +To build the proposal on, lets take the premise of a color pipeline as shown +below. + Hi Uma, Thanks for posting this. A few comments, with some echoing the sentiments of other commenters on the patch set. + +---+ + |RAM| + | +--++-++-+ | + | | FB 1 || FB 2 || FB N| | + | +--++-++-+ | + +---+ + | Plane Color Hardware Block | + ++ + | +---v-+ +---v---+ +---v--+ | + | | Plane A | | Plane B | | Plane N | | + | | Pre-CSC | | Pre-CSC | | Pre-CSC | | + | +---+-+ +---+---+ +---+--+ | + | | | || + | +---v-+ +---v---+ +---v--+ | + | |Plane A | | Plane B | | Plane N | | + | |CSC/CTM | | CSC/CTM | | CSC/CTM | | + | +---+-+ ++--+ ++-+ | + | | | | | + | +---v-+ +v--+ +v-+ | + | | Plane A | | Plane B | | Plane N | | + | |Post-CSC | | Post-CSC | | Post-CSC | | + | +---+-+ ++--+ ++-+ | + | | | | | + ++ ++--v--v---v---| +|| || +|| Pipe Blender|| ++++ +||| +|+---v--+ | +|| Pipe Pre-CSC| | +|| | | +|+---+--+ | +||Pipe Color | +|+---v--+ Hardware| +|| Pipe CSC/CTM| | +|| | | +|+---+--+ | +||| +|+---v--+ | +|| Pipe Post-CSC | | +|| | | +|+---+--+ | +||| ++-+ + | + v +Pipe Output + +Each plane consists of the following color blocks + * Pre-CSC : This block can used to linearize the input frame buffer data. + The linear data then can be further acted on by the following + color hardware blocks in the display hardware pipeline + + * CSC/CTM: Used to program color transformation matrix, this block is used +to perform color space conversions like BT2020 to BT709 or BT601 +etc. This block acts on the linearized data coming from the +Pre-CSC HW block. + + * Post-CSC: This HW block can be used to non-linearize frame buffer data to + match the sink. Another use case of it could be to perform Tone + mapping for HDR use-cases. + +Data from multiple planes will then be fed to pipe/crtc where it will get blended. +There is a similar set of HW blocks available at pipe/crtc level which acts on +this blended data. + +Below is a sample usecase fo video playback with sub-titles and playback +controls + +┌┐┌─┐ ┌─┐┌─┐ +│FB1 ││PRE-CSC │ │ CTM Matrix ││ POST-CSC│ +│├───►│Linearize├►│ BT709 to├───►│ SDR to HDR │ +│BT709 SDR ││ │ │ BT2020 ││ Tone Mapping├─┐ +└┘└─┘ └─┘└─┘ │ +(subtitles) │ + │ +┌┐┌─┐ ┌─┐┌─┐ │ +│FB2 ││PRE-CSC │ │ CTM Matrix ││ POST-CSC│ │ +│├───►│Linearize
[Bug 205089] amdgpu : drm:amdgpu_cs_ioctl : Failed to initialize parser -125
https://bugzilla.kernel.org/show_bug.cgi?id=205089 Alexey Kuznetsov (a...@me.com) changed: What|Removed |Added CC||a...@me.com --- Comment #58 from Alexey Kuznetsov (a...@me.com) --- 6.1.0-10-amd64 (debian 12) *ERROR* Failed to initialize parser -125 https://linux-hardware.org/?probe=72516e7752 -- You may reply to this email to add a comment. You are receiving this mail because: You are watching the assignee of the bug.
Re: [Nouveau] [RFC, drm-misc-next v4 0/9] PCI/VGA: Allowing the user to select the primary video adapter at boot time
Hi, On 2023/9/7 17:08, Christian König wrote: I strongly suggest that you just completely drop this here Drop this is OK, no problem. Then I will go to develop something else. This version is not intended to merge originally, as it's a RFC. Also, the core mechanism already finished, it is the first patch in this series. Things left are just policy (how to specify one and parse the kernel CMD line) and nothing interesting left. It is actually to fulfill my promise at V3 which is to give some examples as usage cases. and go into the AST driver and try to fix it. Well, someone tell me that this is well defined behavior yesterday, which imply that it is not a bug. I'm not going to fix a non-bug. But if thomas ask me to fix it, then I probably have to try to fix. But I suggest if things not broken, don't fix it. Otherwise this may incur more big trouble. For server's single display use case, it is good enough. Thanks.
Re: [Nouveau] [RFC, drm-misc-next v4 0/9] PCI/VGA: Allowing the user to select the primary video adapter at boot time
Am 07.09.23 um 17:26 schrieb suijingfeng: [SNIP] Then, I'll give you another example, see below for elaborate description. I have one AMD BC160 GPU, see[1] to get what it looks like. The GPU don't has a display connector interface exported. It actually can be seen as a render-only GPU or compute class GPU for bitcoin. But the firmware of it still acclaim this GPU as VGA compatible. When mount this GPU onto motherboard, the system always select this GPU as primary. But this GPU can't be able to connect with a monitor. Under such a situation, modprobe.blacklist=amdgpu don't works either, because vgaarb always select this GPU as primary, this is a device-level decision. It's not VGAARB which makes this selection, it's the BIOS. VGAARB just detects what the BIOS has decided. $ dmesg | grep vgaarb: [ 3.541405] pci :0c:00.0: vgaarb: BAR 0: [mem 0xa000-0xafff 64bit pref] contains firmware FB [0xa000-0xa02f] [ 3.901448] pci :05:00.0: vgaarb: setting as boot VGA device [ 3.905375] pci :05:00.0: vgaarb: VGA device added: decodes=io+mem,owns=none,locks=none [ 3.905382] pci :0c:00.0: vgaarb: setting as boot VGA device (overriding previous) [ 3.909375] pci :0c:00.0: vgaarb: VGA device added: decodes=io+mem,owns=io+mem,locks=none [ 3.913375] pci :0d:00.0: vgaarb: VGA device added: decodes=io+mem,owns=none,locks=none [ 3.913377] vgaarb: loaded [ 13.513760] amdgpu :0c:00.0: vgaarb: deactivate vga console [ 19.020992] amdgpu :0c:00.0: vgaarb: changed VGA decodes: olddecodes=io+mem,decodes=none:owns=io+mem I'm using ubuntu 22.04 system, with ast.modeset=10 passed on the cmd line, I still be able to enter the graphics system. And views this GPU as a render-only GPU. Probably continue to examine what's wrong, except this, drm/amdgpu report " *ERROR* IB test failed on sdma0 (-110)" to me. Does this count as problem? No, again that is perfectly expected behavior. Some BIOSes (or maybe most by modern standard) allows to override this, but if you later override this by the OS you run the hardware outside what's validated. When you put a VGA device into a board with an integrated VGA device the integrated one gets disabled. This is even part of some PCIe specification IIRC. So the problems you run into here are perfectly expected. Regards, Christian. Before I could find solution, I have keep this de-fact render only GPU mounted. Because I need recompile kennel module, install the kernel module and testing. All I need is a 2D video card to display something, ast drm is OK, despite simple. It suit the need for my daily usage with VIM, that's enough for me. Now, the real questions that I want ask is: 1) Does the fact that when the kernel driver module got blocked (by modprobe.blacklist=amdgpu), while the vgaarb still select it as primary which leave the X server crash there (because no kennel space driver loaded) count as a problem? 2) Does my approach that mounting another GPU as the primary display adapter, while its real purpose is to solving bugs and development for another GPU, count as a use case? $ cat demsg.txt | grep drm [ 10.099888] ACPI: bus type drm_connector registered [ 11.083920] etnaviv :0d:00.0: [drm] bind etnaviv-display, master name: :0d:00.0 [ 11.084106] [drm] Initialized etnaviv 1.3.0 20151214 for :0d:00.0 on minor 0 [ 13.301702] [drm] amdgpu kernel modesetting enabled. [ 13.359820] [drm] initializing kernel modesetting (NAVI12 0x1002:0x7360 0x1002:0x0A34 0xC7). [ 13.368246] [drm] register mmio base: 0xEB10 [ 13.372861] [drm] register mmio size: 524288 [ 13.380788] [drm] add ip block number 0 [ 13.385661] [drm] add ip block number 1 [ 13.390531] [drm] add ip block number 2 [ 13.395405] [drm] add ip block number 3 [ 13.399760] [drm] add ip block number 4 [ 13.404111] [drm] add ip block number 5 [ 13.408378] [drm] add ip block number 6 [ 13.413249] [drm] add ip block number 7 [ 13.433546] [drm] add ip block number 8 [ 13.433547] [drm] add ip block number 9 [ 13.497757] [drm] VCN decode is enabled in VM mode [ 13.502540] [drm] VCN encode is enabled in VM mode [ 13.508785] [drm] JPEG decode is enabled in VM mode [ 13.529596] [drm] vm size is 262144 GB, 4 levels, block size is 9-bit, fragment size is 9-bit [ 13.564762] [drm] Detected VRAM RAM=8176M, BAR=256M [ 13.569628] [drm] RAM width 2048bits HBM [ 13.574167] [drm] amdgpu: 8176M of VRAM memory ready [ 13.579125] [drm] amdgpu: 15998M of GTT memory ready. [ 13.584184] [drm] GART: num cpu pages 131072, num gpu pages 131072 [ 13.590505] [drm] PCIE GART of 512M enabled (table at 0x00800030). [ 13.598749] [drm] Found VCN firmware Version ENC: 1.16 DEC: 5 VEP: 0 Revision: 4 [ 13.671786] [drm] reserve 0xe0 from 0x81fd00 for PSP TMR [ 13.801235] [drm] Display Core v3.2.247 initialized on DCN 2.0 [ 13.807061] [drm]
Re: [Nouveau] [RFC, drm-misc-next v4 0/9] PCI/VGA: Allowing the user to select the primary video adapter at boot time
Hi, On 2023/9/7 20:43, Christian König wrote: Am 07.09.23 um 14:32 schrieb suijingfeng: Hi, On 2023/9/7 17:08, Christian König wrote: Well, I have over 25 years of experience with display hardware and what you describe here was never an issue. I want to give you an example to let you know more. I have a ASRock AD2550B-ITX board[1], When another discrete video card is mounted into it mini PCIe slot or PCI slot, The IGD cannot be the primary display adapter anymore. The display is totally black. I have try to draft a few trivial patch to help fix this[2]. And I want to use the IGD as primary, does this count as an issue? No, this is completely expected behavior and a limitation of the hardware design. As far as I know both AMD and Intel GPUs work the same here. Regards, Christian. [1] https://www.asrock.com/mb/Intel/AD2550-ITX/ [2] https://patchwork.freedesktop.org/series/123073/ Then, I'll give you another example, see below for elaborate description. I have one AMD BC160 GPU, see[1] to get what it looks like. The GPU don't has a display connector interface exported. It actually can be seen as a render-only GPU or compute class GPU for bitcoin. But the firmware of it still acclaim this GPU as VGA compatible. When mount this GPU onto motherboard, the system always select this GPU as primary. But this GPU can't be able to connect with a monitor. Under such a situation, modprobe.blacklist=amdgpu don't works either, because vgaarb always select this GPU as primary, this is a device-level decision. $ dmesg | grep vgaarb: [3.541405] pci :0c:00.0: vgaarb: BAR 0: [mem 0xa000-0xafff 64bit pref] contains firmware FB [0xa000-0xa02f] [3.901448] pci :05:00.0: vgaarb: setting as boot VGA device [3.905375] pci :05:00.0: vgaarb: VGA device added: decodes=io+mem,owns=none,locks=none [3.905382] pci :0c:00.0: vgaarb: setting as boot VGA device (overriding previous) [3.909375] pci :0c:00.0: vgaarb: VGA device added: decodes=io+mem,owns=io+mem,locks=none [3.913375] pci :0d:00.0: vgaarb: VGA device added: decodes=io+mem,owns=none,locks=none [3.913377] vgaarb: loaded [ 13.513760] amdgpu :0c:00.0: vgaarb: deactivate vga console [ 19.020992] amdgpu :0c:00.0: vgaarb: changed VGA decodes: olddecodes=io+mem,decodes=none:owns=io+mem I'm using ubuntu 22.04 system, with ast.modeset=10 passed on the cmd line, I still be able to enter the graphics system. And views this GPU as a render-only GPU. Probably continue to examine what's wrong, except this, drm/amdgpu report " *ERROR* IB test failed on sdma0 (-110)" to me. Does this count as problem? Before I could find solution, I have keep this de-fact render only GPU mounted. Because I need recompile kennel module, install the kernel module and testing. All I need is a 2D video card to display something, ast drm is OK, despite simple. It suit the need for my daily usage with VIM, that's enough for me. Now, the real questions that I want ask is: 1) Does the fact that when the kernel driver module got blocked (by modprobe.blacklist=amdgpu), while the vgaarb still select it as primary which leave the X server crash there (because no kennel space driver loaded) count as a problem? 2) Does my approach that mounting another GPU as the primary display adapter, while its real purpose is to solving bugs and development for another GPU, count as a use case? $ cat demsg.txt | grep drm [ 10.099888] ACPI: bus type drm_connector registered [ 11.083920] etnaviv :0d:00.0: [drm] bind etnaviv-display, master name: :0d:00.0 [ 11.084106] [drm] Initialized etnaviv 1.3.0 20151214 for :0d:00.0 on minor 0 [ 13.301702] [drm] amdgpu kernel modesetting enabled. [ 13.359820] [drm] initializing kernel modesetting (NAVI12 0x1002:0x7360 0x1002:0x0A34 0xC7). [ 13.368246] [drm] register mmio base: 0xEB10 [ 13.372861] [drm] register mmio size: 524288 [ 13.380788] [drm] add ip block number 0 [ 13.385661] [drm] add ip block number 1 [ 13.390531] [drm] add ip block number 2 [ 13.395405] [drm] add ip block number 3 [ 13.399760] [drm] add ip block number 4 [ 13.404111] [drm] add ip block number 5 [ 13.408378] [drm] add ip block number 6 [ 13.413249] [drm] add ip block number 7 [ 13.433546] [drm] add ip block number 8 [ 13.433547] [drm] add ip block number 9 [ 13.497757] [drm] VCN decode is enabled in VM mode [ 13.502540] [drm] VCN encode is enabled in VM mode [ 13.508785] [drm] JPEG decode is enabled in VM mode [ 13.529596] [drm] vm size is 262144 GB, 4 levels, block size is 9-bit, fragment size is 9-bit [ 13.564762] [drm] Detected VRAM RAM=8176M, BAR=256M [ 13.569628] [drm] RAM width 2048bits HBM [ 13.574167] [drm] amdgpu: 8176M of VRAM memory ready [ 13.579125] [drm] amdgpu: 15998M of GTT memory ready. [ 13.584184] [drm] GART: num cpu pages 131072, num gpu pages 131072 [ 13.590505] [drm] PCIE GART of 512M
Re: [PATCH v3 2/2] drm/tests/drm_exec: Add a test for object freeing within drm_exec_fini()
Hi Thomas,
On Thu, Sep 07, 2023 at 03:53:39PM +0200, Thomas Hellström wrote:
> Check that object freeing from within drm_exec_fini() works as expected
> and is unlikely to generate any warnings.
>
> v3:
> - Condition the test on CONFIG_DEBUG_LOCK_ALLOC
> - Make the test fail if the situation that generates the lockdep
> warning occurs. (Maxime Ripard)
>
> Cc: Maxime Ripard
> Cc: Christian König
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: Thomas Hellström
> ---
> drivers/gpu/drm/tests/drm_exec_test.c | 82 +++
> 1 file changed, 82 insertions(+)
>
> diff --git a/drivers/gpu/drm/tests/drm_exec_test.c
> b/drivers/gpu/drm/tests/drm_exec_test.c
> index 563949d777dd..83fddc6fe1ae 100644
> --- a/drivers/gpu/drm/tests/drm_exec_test.c
> +++ b/drivers/gpu/drm/tests/drm_exec_test.c
> @@ -21,6 +21,9 @@
> struct drm_exec_priv {
> struct device *dev;
> struct drm_device *drm;
> +#ifdef CONFIG_DEBUG_LOCK_ALLOC
> + struct drm_exec *exec;
> +#endif
> };
>
> static int drm_exec_test_init(struct kunit *test)
> @@ -170,6 +173,82 @@ static void test_prepare_array(struct kunit *test)
> drm_gem_private_object_fini();
> }
>
> +#ifdef CONFIG_DEBUG_LOCK_ALLOC
> +static void drm_exec_test_obj_free(struct drm_gem_object *gem)
> +{
> + struct kunit *test = current->kunit_test;
> + struct drm_exec_priv *priv = test->priv;
> + bool resv_class_held;
> + bool first_object_locked;
> +
> + /*
> + * The lock alloc tracking code may warn if the dma_resv lock
> + * class is still held, and we're freeing the first object we
> + * locked.
> + */
> + resv_class_held = (lockdep_is_held(>resv->lock.base) ==
> +LOCK_STATE_HELD);
> + first_object_locked = (gem == priv->exec->objects[0]);
> + KUNIT_EXPECT_FALSE(current->kunit_test,
> +resv_class_held && first_object_locked);
> +
> + dma_resv_fini(gem->resv);
> + kfree(gem);
> +}
> +
> +static const struct drm_gem_object_funcs put_funcs = {
> + .free = drm_exec_test_obj_free,
> +};
> +
> +/*
> + * Check that freeing objects from within drm_exec_fini()
> + * doesn't trigger a false lock alloc warning due to
> + * the dma_resv lock *class* still being held and we're
> + * freeing the first object locked, which *might* be
> + * registered as the address of the held lock of that
> + * lock class.
> + */
> +static void test_early_put(struct kunit *test)
> +{
> + struct drm_exec_priv *priv = test->priv;
> + struct drm_gem_object *gobj1;
> + struct drm_gem_object *gobj2;
> + struct drm_gem_object *array[2];
> + struct drm_exec exec;
> + int ret;
> +
> + priv->exec =
> +
> + gobj1 = kzalloc(sizeof(*gobj1), GFP_KERNEL);
> + KUNIT_EXPECT_NOT_NULL(test, gobj1);
> + if (!gobj1)
> + return;
> +
> + gobj2 = kzalloc(sizeof(*gobj2), GFP_KERNEL);
> + KUNIT_EXPECT_NOT_NULL(test, gobj2);
> + if (!gobj2) {
> + kfree(gobj1);
> + return;
> + }
> +
> + gobj1->funcs = _funcs;
> + gobj2->funcs = _funcs;
> + drm_gem_private_object_init(priv->drm, gobj1, PAGE_SIZE);
> + drm_gem_private_object_init(priv->drm, gobj2, PAGE_SIZE);
> + array[0] = gobj1;
> + array[1] = gobj2;
> +
> + drm_exec_init(, DRM_EXEC_INTERRUPTIBLE_WAIT);
> + drm_exec_until_all_locked()
> + ret = drm_exec_prepare_array(, array, ARRAY_SIZE(array),
> + 1);
> + KUNIT_EXPECT_EQ(test, ret, 0);
> + drm_gem_object_put(gobj1);
> + drm_gem_object_put(gobj2);
> + drm_exec_fini();
> +}
> +#endif
We might want to revisit this later depending on the answer from the
kunit maintainers, but for now
Acked-by: Maxime Ripard
Thanks!
Maxime
signature.asc
Description: PGP signature
Re: [PATCH v3 1/2] drm/tests: helpers: Avoid a driver uaf
On Thu, 7 Sep 2023 15:53:38 +0200, Thomas Hellström wrote: > when using __drm_kunit_helper_alloc_drm_device() the driver may be > dereferenced by device-managed resources up until the device is > freed, which is typically later than the kunit-managed resource code > frees it. Fix this by simply make the driver device-managed as well. > > > [ ... ] Acked-by: Maxime Ripard Thanks! Maxime
Re: [PATCH v3 0/2] drm/tests: Fix for UAF and a test for drm_exec lock alloc tracking warning
Am 07.09.23 um 16:47 schrieb Thomas Hellström: Hi, On 9/7/23 16:37, Christian König wrote: Am 07.09.23 um 15:53 schrieb Thomas Hellström: While trying to replicate a weird drm_exec lock alloc tracking warning using the drm_exec kunit test, the warning was shadowed by a UAF warning from KASAN due to a bug in the drm kunit helpers. Patch 1 fixes that drm kunit UAF. Patch 2 introduces a drm_exec kunit subtest that fails if the conditions for the weird warning are met. The series previously also had a patch with a drm_exec workaround for the warning but that patch has already been commited to drm_misc_next_fixes. Thinking more about this what happens when somebody calls drm_exec_unlock_obj() on the first locked object? Essentially the same thing. I've been thinking of the best way to handle that, but not sure what's the best one. Well what does lockdep store in that object in the first place? Could we fix that somehow? Christian. /Thomas Christian. v2: - Rewording of commit messages - Add some commit message tags v3: - Remove an already committed patch - Rework the test to not require dmesg inspection (Maxime Ripard) - Condition the test on CONFIG_LOCK_ALLOC - Update code comments and commit messages (Maxime Ripard) Cc: Maxime Ripard Cc: Christian König Thomas Hellström (2): drm/tests: helpers: Avoid a driver uaf drm/tests/drm_exec: Add a test for object freeing within drm_exec_fini() drivers/gpu/drm/tests/drm_exec_test.c | 82 +++ include/drm/drm_kunit_helpers.h | 4 +- 2 files changed, 85 insertions(+), 1 deletion(-)
Re: [PATCH v3 0/2] drm/tests: Fix for UAF and a test for drm_exec lock alloc tracking warning
Hi, On 9/7/23 16:37, Christian König wrote: Am 07.09.23 um 15:53 schrieb Thomas Hellström: While trying to replicate a weird drm_exec lock alloc tracking warning using the drm_exec kunit test, the warning was shadowed by a UAF warning from KASAN due to a bug in the drm kunit helpers. Patch 1 fixes that drm kunit UAF. Patch 2 introduces a drm_exec kunit subtest that fails if the conditions for the weird warning are met. The series previously also had a patch with a drm_exec workaround for the warning but that patch has already been commited to drm_misc_next_fixes. Thinking more about this what happens when somebody calls drm_exec_unlock_obj() on the first locked object? Essentially the same thing. I've been thinking of the best way to handle that, but not sure what's the best one. /Thomas Christian. v2: - Rewording of commit messages - Add some commit message tags v3: - Remove an already committed patch - Rework the test to not require dmesg inspection (Maxime Ripard) - Condition the test on CONFIG_LOCK_ALLOC - Update code comments and commit messages (Maxime Ripard) Cc: Maxime Ripard Cc: Christian König Thomas Hellström (2): drm/tests: helpers: Avoid a driver uaf drm/tests/drm_exec: Add a test for object freeing within drm_exec_fini() drivers/gpu/drm/tests/drm_exec_test.c | 82 +++ include/drm/drm_kunit_helpers.h | 4 +- 2 files changed, 85 insertions(+), 1 deletion(-)
Re: [PATCH v3 0/2] drm/tests: Fix for UAF and a test for drm_exec lock alloc tracking warning
Am 07.09.23 um 15:53 schrieb Thomas Hellström: While trying to replicate a weird drm_exec lock alloc tracking warning using the drm_exec kunit test, the warning was shadowed by a UAF warning from KASAN due to a bug in the drm kunit helpers. Patch 1 fixes that drm kunit UAF. Patch 2 introduces a drm_exec kunit subtest that fails if the conditions for the weird warning are met. The series previously also had a patch with a drm_exec workaround for the warning but that patch has already been commited to drm_misc_next_fixes. Thinking more about this what happens when somebody calls drm_exec_unlock_obj() on the first locked object? Christian. v2: - Rewording of commit messages - Add some commit message tags v3: - Remove an already committed patch - Rework the test to not require dmesg inspection (Maxime Ripard) - Condition the test on CONFIG_LOCK_ALLOC - Update code comments and commit messages (Maxime Ripard) Cc: Maxime Ripard Cc: Christian König Thomas Hellström (2): drm/tests: helpers: Avoid a driver uaf drm/tests/drm_exec: Add a test for object freeing within drm_exec_fini() drivers/gpu/drm/tests/drm_exec_test.c | 82 +++ include/drm/drm_kunit_helpers.h | 4 +- 2 files changed, 85 insertions(+), 1 deletion(-)
Re: (subset) [PATCH v2] drm/connector: document DRM_MODE_COLORIMETRY_COUNT
On Wed, 06 Sep 2023 22:47:38 +0200, Javier Carrasco wrote: > The drm_colorspace enum member DRM_MODE_COLORIMETRY_COUNT has been > properly documented by moving the description out of the enum to the > member description list to get rid of an additional warning and improve > documentation clarity. > > Applied to drm/drm-misc (drm-misc-next). Thanks! Maxime
Re: [PATCH v3] drm/i915: Run relevant bits of debugfs drop_caches per GT
On 9/7/2023 2:58 PM, Andi Shyti wrote:
From: Tvrtko Ursulin
Walk all GTs when doing the respective bits of drop_caches work.
Signed-off-by: Tvrtko Ursulin
Signed-off-by: Andi Shyti
Reviewed-by: Nirmoy Das
---
Hi,
I'm proposing this new version of the series I sent here[*].
Patch 1 from that series is not necessary so taht I'm going to
propose the original version proposed by Tvrtko when we were
young.
Andi
Changelog
=
v2 -> v3:
- fix the "for_each_gt()" parameter order.
v1 -> v2:
- drop the gt idling and the cache flushing decoupling and stick
to the original version.
[*] https://patchwork.freedesktop.org/series/123301/
drivers/gpu/drm/i915/i915_debugfs.c | 10 +++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
b/drivers/gpu/drm/i915/i915_debugfs.c
index 7a90a2e32c9f1..e9b79c2c37d84 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -740,15 +740,19 @@ static int
i915_drop_caches_set(void *data, u64 val)
{
struct drm_i915_private *i915 = data;
+ struct intel_gt *gt;
unsigned int flags;
+ unsigned int i;
int ret;
drm_dbg(>drm, "Dropping caches: 0x%08llx [0x%08llx]\n",
val, val & DROP_ALL);
- ret = gt_drop_caches(to_gt(i915), val);
- if (ret)
- return ret;
+ for_each_gt(gt, i915, i) {
+ ret = gt_drop_caches(gt, val);
+ if (ret)
+ return ret;
+ }
fs_reclaim_acquire(GFP_KERNEL);
flags = memalloc_noreclaim_save();
Re: [RFC PATCH 04/10] drm/panel_helper: Introduce drm_panel_helper
On Tue, Sep 05, 2023 at 12:12:58PM -0700, Doug Anderson wrote: > Hi, > > On Tue, Sep 5, 2023 at 9:45 AM Doug Anderson wrote: > > > > As per our discussion, in V2 we will make drm_panel_remove() actually > > unprepare / disable a panel if it was left enabled. This would > > essentially fold in the drm_panel_helper_shutdown() from my RFC patch. > > This would make tdo_tl070wsh30_panel_remove() behave the same as it > > did before. Ugh, though I may have to think about this more when I get > > to implementation since I don't think there's a guarantee of the > > ordering of shutdown calls between the DRM driver and the panel. > > Anyway, something to discuss later. > > Ugh, ignore the above paragraph. I managed to confuse myself and was > thinking about shutdown but talking about remove. Sigh. :( Instead, > pretend the above paragraph said: > > As per our discussion, in V2 we will make drm_panel_remove() actually > unprepare / disable a panel (and print a warning) if it was left > enabled. This would essentially fold in the > drm_panel_helper_shutdown() from my RFC patch (but add a warning). > This would make tdo_tl070wsh30_panel_remove() behave the same as it > did before with the addition of a warning if someone tries to remove a > currently powered panel. Ok, that works for me :) Maxime signature.asc Description: PGP signature
Re: [RFC PATCH 04/10] drm/panel_helper: Introduce drm_panel_helper
On Tue, Sep 05, 2023 at 09:45:49AM -0700, Doug Anderson wrote:
> > > In any case, I don't think there's any need to switch this to
> > > refcounting as part of this effort. Someone could, in theory, do it as
> > > a separate patch series.
> >
> > I'm sorry, but I'll insist on getting a solution that will warn panels
> > that call drm_atomic_helper_shutdown or drm_panel_disable/unprepare by
> > hand. It doesn't have to be refcounting though if you have a better idea
> > in mind.
>
> As per above, I think this already happens with the boolean? Won't you
> see an error message like this:
>
> dev_warn(panel->dev, "Skipping unprepare of already unprepared panel\n");
>
> ...from drm_panel_unprepare()
Urgh. I missed that part, sorry for dragging you into that refcounting
discussion then.
> > > > > The above solves the problem with panels wanting to power sequence
> > > > > themselves at remove() time, but not at shutdown() time. Thus we'd
> > > > > still have a dependency on having all drivers use
> > > > > drm_atomic_helper_shutdown() so that work becomes a dependency.
> > > >
> > > > Does it? I think it can be done in parallel?
> > >
> > > I don't think it can be in parallel. While it makes sense for panels
> > > to call drm_panel_remove() at remove time, it doesn't make sense for
> > > them to call it at shutdown time. That means that the trick of having
> > > the panel get powered off in drm_panel_remove() won't help for
> > > shutdown. For shutdown, which IMO is the more important case, we need
> > > to wait until all drm drivers call drm_atomic_helper_shutdown()
> > > properly.
> >
> > Right, my point was more that drivers that already don't disable the
> > panel in their shutdown implementation will still not do it. And drivers
> > that do will still do it, so there's no regression.
> >
> > We obviously want to tend to having all drivers call
> > drm_atomic_helper_shutdown(), but not having it will not introduce any
> > regression.
>
> I'm still confused here too. The next patch in this patch series here
> that we're talking about is:
>
> https://lore.kernel.org/dri-devel/20230804140605.RFC.5.Icc3238e91bc726d4b04c51a4acf67f001ec453d7@changeid/
>
> Let's look at an arbitrary concrete part of that patch: the
> modification to "panel-tdo-tl070wsh30.c". For that panel, my RFC patch
> removed the tracking of "prepared" and and then did this:
>
> @@ -220,16 +209,14 @@ static void tdo_tl070wsh30_panel_remove(struct
> mipi_dsi_device *dsi)
> dev_err(>dev, "failed to detach from DSI host: %d\n", err);
>
> drm_panel_remove(_tl070wsh30->base);
> - drm_panel_disable(_tl070wsh30->base);
> - drm_panel_unprepare(_tl070wsh30->base);
> + drm_panel_helper_shutdown(_tl070wsh30->base);
> }
>
> static void tdo_tl070wsh30_panel_shutdown(struct mipi_dsi_device *dsi)
> {
> struct tdo_tl070wsh30_panel *tdo_tl070wsh30 = mipi_dsi_get_drvdata(dsi);
>
> - drm_panel_disable(_tl070wsh30->base);
> - drm_panel_unprepare(_tl070wsh30->base);
> + drm_panel_helper_shutdown(_tl070wsh30->base);
> }
>
> As per our discussion, the plan is to send a V2 of my patch series
> where I _don't_ create the call drm_panel_helper_shutdown() and thus I
> won't call it. That means that the V2 of the patch for
> "panel-tdo-tl070wsh30.c" will remove the calls to drm_panel_disable()
> and drm_panel_unprepare() and not replace them with anything.
Right, that's what we should do.
> As per our discussion, in V2 we will make drm_panel_remove() actually
> unprepare / disable a panel if it was left enabled. This would
> essentially fold in the drm_panel_helper_shutdown() from my RFC patch.
> This would make tdo_tl070wsh30_panel_remove() behave the same as it
> did before.
Not really? You would get a warning from drm_panel_remove(), but our
discussion very much implied still disabling it...
> Ugh, though I may have to think about this more when I get to
> implementation since I don't think there's a guarantee of the ordering
> of shutdown calls between the DRM driver and the panel. Anyway,
> something to discuss later.
>
> However... tdo_tl070wsh30_panel_shutdown() will no longer power the
> panel off properly _unless_ the main DRM driver properly calls
> drm_atomic_helper_shutdown().
... with the expectation that all KMS drivers should call
drm_atomic_helper_shutdown() anyway (thanks to your other series) and
thus we wouldn't trigger that remove warning anyway.
> Did I get anything above incorrect? Assuming I got it correct, that
> means that our choices are:
>
> a) Block landing the change to "panel-tdo-tl070wsh30.c" until after
> all drivers properly call drm_atomic_helper_shutdown().
I don't think we care about all drivers here. Only the driver it's
enabled with would be a blocker. Like, let's say sun4i hasn't been
converted but that panel is only used with rockchip anyway, we don't
really care that sun4i shutdown patch hasn't been merged (yet).
> b) Add a hacky call to drm_panel_remove() in
> tdo_tl070wsh30_panel_shutdown() to
Re: [PATCH v2 07/34] drm/amd/display: explicitly define EOTF and inverse EOTF
On 2023-09-07 03:49, Pekka Paalanen wrote:
> On Wed, 6 Sep 2023 16:15:10 -0400
> Harry Wentland wrote:
>
>> On 2023-08-25 10:18, Melissa Wen wrote:
>>> On 08/22, Pekka Paalanen wrote:
On Thu, 10 Aug 2023 15:02:47 -0100
Melissa Wen wrote:
> Instead of relying on color block names to get the transfer function
> intention regarding encoding pixel's luminance, define supported
> Electro-Optical Transfer Functions (EOTFs) and inverse EOTFs, that
> includes pure gamma or standardized transfer functions.
>
> Suggested-by: Harry Wentland
> Signed-off-by: Melissa Wen
> ---
> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 19 +++--
> .../amd/display/amdgpu_dm/amdgpu_dm_color.c | 69 +++
> 2 files changed, 67 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> index c749c9cb3d94..f6251ed89684 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> @@ -718,14 +718,21 @@ extern const struct amdgpu_ip_block_version
> dm_ip_block;
>
> enum amdgpu_transfer_function {
> AMDGPU_TRANSFER_FUNCTION_DEFAULT,
> - AMDGPU_TRANSFER_FUNCTION_SRGB,
> - AMDGPU_TRANSFER_FUNCTION_BT709,
> - AMDGPU_TRANSFER_FUNCTION_PQ,
> + AMDGPU_TRANSFER_FUNCTION_SRGB_EOTF,
> + AMDGPU_TRANSFER_FUNCTION_BT709_EOTF,
> + AMDGPU_TRANSFER_FUNCTION_PQ_EOTF,
> AMDGPU_TRANSFER_FUNCTION_LINEAR,
> AMDGPU_TRANSFER_FUNCTION_UNITY,
> - AMDGPU_TRANSFER_FUNCTION_GAMMA22,
> - AMDGPU_TRANSFER_FUNCTION_GAMMA24,
> - AMDGPU_TRANSFER_FUNCTION_GAMMA26,
> + AMDGPU_TRANSFER_FUNCTION_GAMMA22_EOTF,
> + AMDGPU_TRANSFER_FUNCTION_GAMMA24_EOTF,
> + AMDGPU_TRANSFER_FUNCTION_GAMMA26_EOTF,
> + AMDGPU_TRANSFER_FUNCTION_SRGB_INV_EOTF,
> + AMDGPU_TRANSFER_FUNCTION_BT709_INV_EOTF,
> + AMDGPU_TRANSFER_FUNCTION_PQ_INV_EOTF,
> + AMDGPU_TRANSFER_FUNCTION_GAMMA22_INV_EOTF,
> + AMDGPU_TRANSFER_FUNCTION_GAMMA24_INV_EOTF,
> + AMDGPU_TRANSFER_FUNCTION_GAMMA26_INV_EOTF,
> +AMDGPU_TRANSFER_FUNCTION_COUNT
> };
>
> struct dm_plane_state {
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
> index 56ce008b9095..cc2187c0879a 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
> @@ -85,18 +85,59 @@ void amdgpu_dm_init_color_mod(void)
> }
>
> #ifdef AMD_PRIVATE_COLOR
> -static const struct drm_prop_enum_list
> amdgpu_transfer_function_enum_list[] = {
> - { AMDGPU_TRANSFER_FUNCTION_DEFAULT, "Default" },
> - { AMDGPU_TRANSFER_FUNCTION_SRGB, "sRGB" },
> - { AMDGPU_TRANSFER_FUNCTION_BT709, "BT.709" },
> - { AMDGPU_TRANSFER_FUNCTION_PQ, "PQ (Perceptual Quantizer)" },
> - { AMDGPU_TRANSFER_FUNCTION_LINEAR, "Linear" },
> - { AMDGPU_TRANSFER_FUNCTION_UNITY, "Unity" },
> - { AMDGPU_TRANSFER_FUNCTION_GAMMA22, "Gamma 2.2" },
> - { AMDGPU_TRANSFER_FUNCTION_GAMMA24, "Gamma 2.4" },
> - { AMDGPU_TRANSFER_FUNCTION_GAMMA26, "Gamma 2.6" },
> +static const char * const
> +amdgpu_transfer_function_names[] = {
> + [AMDGPU_TRANSFER_FUNCTION_DEFAULT] = "Default",
> + [AMDGPU_TRANSFER_FUNCTION_LINEAR] = "Linear",
Hi,
if the below is identity, then what is linear? Is there a coefficient
(multiplier) somewhere? Offset?
> + [AMDGPU_TRANSFER_FUNCTION_UNITY]= "Unity",
Should "Unity" be called "Identity"?
>>>
>>> AFAIU, AMD treats Linear and Unity as the same: Identity. So, IIUC,
>>> indeed merging both as identity sounds the best approach.
>>
>> Agreed.
>>
Doesn't unity mean that the output is always 1.0 regardless of input?
> + [AMDGPU_TRANSFER_FUNCTION_SRGB_EOTF]= "sRGB EOTF",
> + [AMDGPU_TRANSFER_FUNCTION_BT709_EOTF] = "BT.709 EOTF",
BT.709 says about "Overall opto-electronic transfer characteristics at
source":
In typical production practice the encoding function of image
sources is adjusted so that the final picture has the desired
look, as viewed on a reference monitor having the reference
decoding function of Recommendation ITU-R BT.1886, in the
reference viewing environment defined in Recommendation ITU-R
BT.2035.
IOW, typically people tweak the encoding function instead of using
BT.709 OETF as is, which means that inverting the BT.709 OETF produces
something slightly unknown. The note about BT.1886 means that that
something is also not quite how it's supposed to be turned into light.
Should this enum
[PATCH v3 1/2] drm/tests: helpers: Avoid a driver uaf
when using __drm_kunit_helper_alloc_drm_device() the driver may be dereferenced by device-managed resources up until the device is freed, which is typically later than the kunit-managed resource code frees it. Fix this by simply make the driver device-managed as well. In short, the sequence leading to the UAF is as follows: INIT: Code allocates a struct device as a kunit-managed resource. Code allocates a drm driver as a kunit-managed resource. Code allocates a drm device as a device-managed resource. EXIT: Kunit resource cleanup frees the drm driver Kunit resource cleanup puts the struct device, which starts a device-managed resource cleanup device-managed cleanup calls drm_dev_put() drm_dev_put() dereferences the (now freed) drm driver -> Boom. Related KASAN message: [55272.551542] == [55272.551551] BUG: KASAN: slab-use-after-free in drm_dev_put.part.0+0xd4/0xe0 [drm] [55272.551603] Read of size 8 at addr 888127502828 by task kunit_try_catch/10353 [55272.551612] CPU: 4 PID: 10353 Comm: kunit_try_catch Tainted: G U N 6.5.0-rc7+ #155 [55272.551620] Hardware name: ASUS System Product Name/PRIME B560M-A AC, BIOS 0403 01/26/2021 [55272.551626] Call Trace: [55272.551629] [55272.551633] dump_stack_lvl+0x57/0x90 [55272.551639] print_report+0xcf/0x630 [55272.551645] ? _raw_spin_lock_irqsave+0x5f/0x70 [55272.551652] ? drm_dev_put.part.0+0xd4/0xe0 [drm] [55272.551694] kasan_report+0xd7/0x110 [55272.551699] ? drm_dev_put.part.0+0xd4/0xe0 [drm] [55272.551742] drm_dev_put.part.0+0xd4/0xe0 [drm] [55272.551783] devres_release_all+0x15d/0x1f0 [55272.551790] ? __pfx_devres_release_all+0x10/0x10 [55272.551797] device_unbind_cleanup+0x16/0x1a0 [55272.551802] device_release_driver_internal+0x3e5/0x540 [55272.551808] ? kobject_put+0x5d/0x4b0 [55272.551814] bus_remove_device+0x1f1/0x3f0 [55272.551819] device_del+0x342/0x910 [55272.551826] ? __pfx_device_del+0x10/0x10 [55272.551830] ? lock_release+0x339/0x5e0 [55272.551836] ? kunit_remove_resource+0x128/0x290 [kunit] [55272.551845] ? __pfx_lock_release+0x10/0x10 [55272.551851] platform_device_del.part.0+0x1f/0x1e0 [55272.551856] ? _raw_spin_unlock_irqrestore+0x30/0x60 [55272.551863] kunit_remove_resource+0x195/0x290 [kunit] [55272.551871] ? _raw_spin_unlock_irqrestore+0x30/0x60 [55272.551877] kunit_cleanup+0x78/0x120 [kunit] [55272.551885] ? __kthread_parkme+0xc1/0x1f0 [55272.551891] ? __pfx_kunit_try_run_case_cleanup+0x10/0x10 [kunit] [55272.551900] ? __pfx_kunit_generic_run_threadfn_adapter+0x10/0x10 [kunit] [55272.551909] kunit_generic_run_threadfn_adapter+0x4a/0x90 [kunit] [55272.551919] kthread+0x2e7/0x3c0 [55272.551924] ? __pfx_kthread+0x10/0x10 [55272.551929] ret_from_fork+0x2d/0x70 [55272.551935] ? __pfx_kthread+0x10/0x10 [55272.551940] ret_from_fork_asm+0x1b/0x30 [55272.551948] [55272.551953] Allocated by task 10351: [55272.551956] kasan_save_stack+0x1c/0x40 [55272.551962] kasan_set_track+0x21/0x30 [55272.551966] __kasan_kmalloc+0x8b/0x90 [55272.551970] __kmalloc+0x5e/0x160 [55272.551976] kunit_kmalloc_array+0x1c/0x50 [kunit] [55272.551984] drm_exec_test_init+0xfa/0x2c0 [drm_exec_test] [55272.551991] kunit_try_run_case+0xdd/0x250 [kunit] [55272.551999] kunit_generic_run_threadfn_adapter+0x4a/0x90 [kunit] [55272.552008] kthread+0x2e7/0x3c0 [55272.552012] ret_from_fork+0x2d/0x70 [55272.552017] ret_from_fork_asm+0x1b/0x30 [55272.552024] Freed by task 10353: [55272.552027] kasan_save_stack+0x1c/0x40 [55272.552032] kasan_set_track+0x21/0x30 [55272.552036] kasan_save_free_info+0x27/0x40 [55272.552041] __kasan_slab_free+0x106/0x180 [55272.552046] slab_free_freelist_hook+0xb3/0x160 [55272.552051] __kmem_cache_free+0xb2/0x290 [55272.552056] kunit_remove_resource+0x195/0x290 [kunit] [55272.552064] kunit_cleanup+0x78/0x120 [kunit] [55272.552072] kunit_generic_run_threadfn_adapter+0x4a/0x90 [kunit] [55272.552080] kthread+0x2e7/0x3c0 [55272.552085] ret_from_fork+0x2d/0x70 [55272.552089] ret_from_fork_asm+0x1b/0x30 [55272.552096] The buggy address belongs to the object at 888127502800 which belongs to the cache kmalloc-512 of size 512 [55272.552105] The buggy address is located 40 bytes inside of freed 512-byte region [888127502800, 888127502a00) [55272.552115] The buggy address belongs to the physical page: [55272.552119] page:af6c70ff refcount:1 mapcount:0 mapping: index:0x0 pfn:0x127500 [55272.552127] head:af6c70ff order:3 entire_mapcount:0 nr_pages_mapped:0 pincount:0 [55272.552133] anon flags: 0x17c0010200(slab|head|node=0|zone=2|lastcpupid=0x1f) [55272.552141] page_type: 0x() [55272.552145] raw: 0017c0010200 888100042c80 dead0001 [55272.552152] raw: 80200020 0001 [55272.552157] page dumped because: kasan: bad access detected
[PATCH v3 2/2] drm/tests/drm_exec: Add a test for object freeing within drm_exec_fini()
Check that object freeing from within drm_exec_fini() works as expected
and is unlikely to generate any warnings.
v3:
- Condition the test on CONFIG_DEBUG_LOCK_ALLOC
- Make the test fail if the situation that generates the lockdep
warning occurs. (Maxime Ripard)
Cc: Maxime Ripard
Cc: Christian König
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: Thomas Hellström
---
drivers/gpu/drm/tests/drm_exec_test.c | 82 +++
1 file changed, 82 insertions(+)
diff --git a/drivers/gpu/drm/tests/drm_exec_test.c
b/drivers/gpu/drm/tests/drm_exec_test.c
index 563949d777dd..83fddc6fe1ae 100644
--- a/drivers/gpu/drm/tests/drm_exec_test.c
+++ b/drivers/gpu/drm/tests/drm_exec_test.c
@@ -21,6 +21,9 @@
struct drm_exec_priv {
struct device *dev;
struct drm_device *drm;
+#ifdef CONFIG_DEBUG_LOCK_ALLOC
+ struct drm_exec *exec;
+#endif
};
static int drm_exec_test_init(struct kunit *test)
@@ -170,6 +173,82 @@ static void test_prepare_array(struct kunit *test)
drm_gem_private_object_fini();
}
+#ifdef CONFIG_DEBUG_LOCK_ALLOC
+static void drm_exec_test_obj_free(struct drm_gem_object *gem)
+{
+ struct kunit *test = current->kunit_test;
+ struct drm_exec_priv *priv = test->priv;
+ bool resv_class_held;
+ bool first_object_locked;
+
+ /*
+* The lock alloc tracking code may warn if the dma_resv lock
+* class is still held, and we're freeing the first object we
+* locked.
+*/
+ resv_class_held = (lockdep_is_held(>resv->lock.base) ==
+ LOCK_STATE_HELD);
+ first_object_locked = (gem == priv->exec->objects[0]);
+ KUNIT_EXPECT_FALSE(current->kunit_test,
+ resv_class_held && first_object_locked);
+
+ dma_resv_fini(gem->resv);
+ kfree(gem);
+}
+
+static const struct drm_gem_object_funcs put_funcs = {
+ .free = drm_exec_test_obj_free,
+};
+
+/*
+ * Check that freeing objects from within drm_exec_fini()
+ * doesn't trigger a false lock alloc warning due to
+ * the dma_resv lock *class* still being held and we're
+ * freeing the first object locked, which *might* be
+ * registered as the address of the held lock of that
+ * lock class.
+ */
+static void test_early_put(struct kunit *test)
+{
+ struct drm_exec_priv *priv = test->priv;
+ struct drm_gem_object *gobj1;
+ struct drm_gem_object *gobj2;
+ struct drm_gem_object *array[2];
+ struct drm_exec exec;
+ int ret;
+
+ priv->exec =
+
+ gobj1 = kzalloc(sizeof(*gobj1), GFP_KERNEL);
+ KUNIT_EXPECT_NOT_NULL(test, gobj1);
+ if (!gobj1)
+ return;
+
+ gobj2 = kzalloc(sizeof(*gobj2), GFP_KERNEL);
+ KUNIT_EXPECT_NOT_NULL(test, gobj2);
+ if (!gobj2) {
+ kfree(gobj1);
+ return;
+ }
+
+ gobj1->funcs = _funcs;
+ gobj2->funcs = _funcs;
+ drm_gem_private_object_init(priv->drm, gobj1, PAGE_SIZE);
+ drm_gem_private_object_init(priv->drm, gobj2, PAGE_SIZE);
+ array[0] = gobj1;
+ array[1] = gobj2;
+
+ drm_exec_init(, DRM_EXEC_INTERRUPTIBLE_WAIT);
+ drm_exec_until_all_locked()
+ ret = drm_exec_prepare_array(, array, ARRAY_SIZE(array),
+1);
+ KUNIT_EXPECT_EQ(test, ret, 0);
+ drm_gem_object_put(gobj1);
+ drm_gem_object_put(gobj2);
+ drm_exec_fini();
+}
+#endif
+
static void test_multiple_loops(struct kunit *test)
{
struct drm_exec exec;
@@ -198,6 +277,9 @@ static struct kunit_case drm_exec_tests[] = {
KUNIT_CASE(test_prepare),
KUNIT_CASE(test_prepare_array),
KUNIT_CASE(test_multiple_loops),
+#ifdef CONFIG_DEBUG_LOCK_ALLOC
+ KUNIT_CASE(test_early_put),
+#endif
{}
};
--
2.41.0
[PATCH v3 0/2] drm/tests: Fix for UAF and a test for drm_exec lock alloc tracking warning
While trying to replicate a weird drm_exec lock alloc tracking warning using the drm_exec kunit test, the warning was shadowed by a UAF warning from KASAN due to a bug in the drm kunit helpers. Patch 1 fixes that drm kunit UAF. Patch 2 introduces a drm_exec kunit subtest that fails if the conditions for the weird warning are met. The series previously also had a patch with a drm_exec workaround for the warning but that patch has already been commited to drm_misc_next_fixes. v2: - Rewording of commit messages - Add some commit message tags v3: - Remove an already committed patch - Rework the test to not require dmesg inspection (Maxime Ripard) - Condition the test on CONFIG_LOCK_ALLOC - Update code comments and commit messages (Maxime Ripard) Cc: Maxime Ripard Cc: Christian König Thomas Hellström (2): drm/tests: helpers: Avoid a driver uaf drm/tests/drm_exec: Add a test for object freeing within drm_exec_fini() drivers/gpu/drm/tests/drm_exec_test.c | 82 +++ include/drm/drm_kunit_helpers.h | 4 +- 2 files changed, 85 insertions(+), 1 deletion(-) -- 2.41.0
Re: [PATCH v3] drm/i915: Run relevant bits of debugfs drop_caches per GT
On Thu, Sep 07, 2023 at 02:58:08PM +0200, Andi Shyti wrote:
> From: Tvrtko Ursulin
>
> Walk all GTs when doing the respective bits of drop_caches work.
>
> Signed-off-by: Tvrtko Ursulin
> Signed-off-by: Andi Shyti
Reviewed-by: Rodrigo Vivi
> ---
> Hi,
>
> I'm proposing this new version of the series I sent here[*].
> Patch 1 from that series is not necessary so taht I'm going to
> propose the original version proposed by Tvrtko when we were
> young.
>
> Andi
>
> Changelog
> =
> v2 -> v3:
> - fix the "for_each_gt()" parameter order.
> v1 -> v2:
> - drop the gt idling and the cache flushing decoupling and stick
>to the original version.
>
> [*] https://patchwork.freedesktop.org/series/123301/
>
> drivers/gpu/drm/i915/i915_debugfs.c | 10 +++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index 7a90a2e32c9f1..e9b79c2c37d84 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -740,15 +740,19 @@ static int
> i915_drop_caches_set(void *data, u64 val)
> {
> struct drm_i915_private *i915 = data;
> + struct intel_gt *gt;
> unsigned int flags;
> + unsigned int i;
> int ret;
>
> drm_dbg(>drm, "Dropping caches: 0x%08llx [0x%08llx]\n",
> val, val & DROP_ALL);
>
> - ret = gt_drop_caches(to_gt(i915), val);
> - if (ret)
> - return ret;
> + for_each_gt(gt, i915, i) {
> + ret = gt_drop_caches(gt, val);
> + if (ret)
> + return ret;
> + }
>
> fs_reclaim_acquire(GFP_KERNEL);
> flags = memalloc_noreclaim_save();
> --
> 2.40.1
>
Re: [PATCH 5/6] drm/edid: add helpers to get/set struct cea_sad from/to 3-byte sad
Hi Jani,
kernel test robot noticed the following build warnings:
[auto build test WARNING on drm-tip/drm-tip]
url:
https://github.com/intel-lab-lkp/linux/commits/Jani-Nikula/drm-edid-split-out-drm_eld-h-from-drm_edid-h/20230907-173011
base: git://anongit.freedesktop.org/drm/drm-tip drm-tip
patch link:
https://lore.kernel.org/r/eba53a0904126fb904a5190750002695350f44eb.1694078430.git.jani.nikula%40intel.com
patch subject: [PATCH 5/6] drm/edid: add helpers to get/set struct cea_sad
from/to 3-byte sad
config: i386-randconfig-013-20230907
(https://download.01.org/0day-ci/archive/20230907/202309072156.id0etpd1-...@intel.com/config)
compiler: gcc-7 (Ubuntu 7.5.0-6ubuntu2) 7.5.0
reproduce (this is a W=1 build):
(https://download.01.org/0day-ci/archive/20230907/202309072156.id0etpd1-...@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot
| Closes:
https://lore.kernel.org/oe-kbuild-all/202309072156.id0etpd1-...@intel.com/
All warnings (new ones prefixed by >>):
>> drivers/gpu/drm/drm_edid.c:5505:6: warning: no previous declaration for
>> 'drm_edid_cta_sad_get' [-Wmissing-declarations]
void drm_edid_cta_sad_get(const struct cea_sad *cta_sad, u8 *sad)
^~~~
>> drivers/gpu/drm/drm_edid.c:5515:6: warning: no previous declaration for
>> 'drm_edid_cta_sad_set' [-Wmissing-declarations]
void drm_edid_cta_sad_set(struct cea_sad *cta_sad, const u8 *sad)
^~~~
vim +/drm_edid_cta_sad_get +5505 drivers/gpu/drm/drm_edid.c
5501
5502 /*
5503 * Get 3-byte SAD buf from struct cea_sad.
5504 */
> 5505 void drm_edid_cta_sad_get(const struct cea_sad *cta_sad, u8 *sad)
5506 {
5507 sad[0] = cta_sad->format << 3 | cta_sad->channels;
5508 sad[1] = cta_sad->freq;
5509 sad[2] = cta_sad->byte2;
5510 }
5511
5512 /*
5513 * Set struct cea_sad from 3-byte SAD buf.
5514 */
> 5515 void drm_edid_cta_sad_set(struct cea_sad *cta_sad, const u8 *sad)
5516 {
5517 cta_sad->format = (sad[0] & 0x78) >> 3;
5518 cta_sad->channels = sad[0] & 0x07;
5519 cta_sad->freq = sad[1] & 0x7f;
5520 cta_sad->byte2 = sad[2];
5521 }
5522
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Re: [PATCH v6 02/20] drm/gpuva_mgr: Helper to get range of unmap from a remap op.
On Thu, 07 Sep 2023, Donald Robson wrote:
> On Thu, 2023-09-07 at 15:14 +0300, Jani Nikula wrote:
>> On Wed, 06 Sep 2023, Sarah Walker wrote:
>> > From: Donald Robson
>> >
>> > Signed-off-by: Donald Robson
>> > ---
>> > include/drm/drm_gpuva_mgr.h | 27 +++
>> > 1 file changed, 27 insertions(+)
>> >
>> > diff --git a/include/drm/drm_gpuva_mgr.h b/include/drm/drm_gpuva_mgr.h
>> > index ed8d50200cc3..be7b3a6d7e67 100644
>> > --- a/include/drm/drm_gpuva_mgr.h
>> > +++ b/include/drm/drm_gpuva_mgr.h
>> > @@ -703,4 +703,31 @@ void drm_gpuva_remap(struct drm_gpuva *prev,
>> >
>> > void drm_gpuva_unmap(struct drm_gpuva_op_unmap *op);
>> >
>> > +/**
>> > + * drm_gpuva_op_remap_get_unmap_range() - Helper to get the start and
>> > range of
>> > + * the unmap stage of a remap op.
>> > + * @op: Remap op.
>> > + * @start_addr: Output pointer for the start of the required unmap.
>> > + * @range: Output pointer for the length of the required unmap.
>> > + *
>> > + * These parameters can then be used by the caller to unmap memory pages
>> > that
>> > + * are no longer required.
>> > + */
>> > +static __always_inline void
>>
>> IMO __always_inline *always* requires a justification in the commit
>> message.
>>
>> BR,
>> Jani.
>
> Hi Jani,
> I went with __always_inline because I can't see this being used more than
> once per driver.
> I can add that to the commit message, but is that suitable justification? I
> could move
> it to the source file or make it a macro if you prefer.
My personal opinion is that static inlines in general should always have
a performance justification. If there isn't one, it should be a regular
function. Static inlines leak the abstractions and often make the header
dependencies worse.
Not everyone agrees, of course.
More than anything I was looking for justification on __always_inline
rather than just inline, though.
BR,
Jani.
> Thanks,
> Donald
>>
>>
>> > +drm_gpuva_op_remap_get_unmap_range(const struct drm_gpuva_op_remap *op,
>> > + u64 *start_addr, u64 *range)
>> > +{
>> > + const u64 va_start = op->prev ?
>> > + op->prev->va.addr + op->prev->va.range :
>> > + op->unmap->va->va.addr;
>> > + const u64 va_end = op->next ?
>> > + op->next->va.addr :
>> > + op->unmap->va->va.addr + op->unmap->va->va.range;
>> > +
>> > + if (start_addr)
>> > + *start_addr = va_start;
>> > + if (range)
>> > + *range = va_end - va_start;
>> > +}
>> > +
>> > #endif /* __DRM_GPUVA_MGR_H__ */
--
Jani Nikula, Intel Open Source Graphics Center
Re: [PATCH v6 02/20] drm/gpuva_mgr: Helper to get range of unmap from a remap op.
On Thu, 2023-09-07 at 15:14 +0300, Jani Nikula wrote:
> On Wed, 06 Sep 2023, Sarah Walker wrote:
> > From: Donald Robson
> >
> > Signed-off-by: Donald Robson
> > ---
> > include/drm/drm_gpuva_mgr.h | 27 +++
> > 1 file changed, 27 insertions(+)
> >
> > diff --git a/include/drm/drm_gpuva_mgr.h b/include/drm/drm_gpuva_mgr.h
> > index ed8d50200cc3..be7b3a6d7e67 100644
> > --- a/include/drm/drm_gpuva_mgr.h
> > +++ b/include/drm/drm_gpuva_mgr.h
> > @@ -703,4 +703,31 @@ void drm_gpuva_remap(struct drm_gpuva *prev,
> >
> > void drm_gpuva_unmap(struct drm_gpuva_op_unmap *op);
> >
> > +/**
> > + * drm_gpuva_op_remap_get_unmap_range() - Helper to get the start and
> > range of
> > + * the unmap stage of a remap op.
> > + * @op: Remap op.
> > + * @start_addr: Output pointer for the start of the required unmap.
> > + * @range: Output pointer for the length of the required unmap.
> > + *
> > + * These parameters can then be used by the caller to unmap memory pages
> > that
> > + * are no longer required.
> > + */
> > +static __always_inline void
>
> IMO __always_inline *always* requires a justification in the commit
> message.
>
> BR,
> Jani.
Hi Jani,
I went with __always_inline because I can't see this being used more than once
per driver.
I can add that to the commit message, but is that suitable justification? I
could move
it to the source file or make it a macro if you prefer.
Thanks,
Donald
>
>
> > +drm_gpuva_op_remap_get_unmap_range(const struct drm_gpuva_op_remap *op,
> > + u64 *start_addr, u64 *range)
> > +{
> > + const u64 va_start = op->prev ?
> > +op->prev->va.addr + op->prev->va.range :
> > +op->unmap->va->va.addr;
> > + const u64 va_end = op->next ?
> > + op->next->va.addr :
> > + op->unmap->va->va.addr + op->unmap->va->va.range;
> > +
> > + if (start_addr)
> > + *start_addr = va_start;
> > + if (range)
> > + *range = va_end - va_start;
> > +}
> > +
> > #endif /* __DRM_GPUVA_MGR_H__ */
[PATCH v3] drm/i915: Run relevant bits of debugfs drop_caches per GT
From: Tvrtko Ursulin
Walk all GTs when doing the respective bits of drop_caches work.
Signed-off-by: Tvrtko Ursulin
Signed-off-by: Andi Shyti
---
Hi,
I'm proposing this new version of the series I sent here[*].
Patch 1 from that series is not necessary so taht I'm going to
propose the original version proposed by Tvrtko when we were
young.
Andi
Changelog
=
v2 -> v3:
- fix the "for_each_gt()" parameter order.
v1 -> v2:
- drop the gt idling and the cache flushing decoupling and stick
to the original version.
[*] https://patchwork.freedesktop.org/series/123301/
drivers/gpu/drm/i915/i915_debugfs.c | 10 +++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
b/drivers/gpu/drm/i915/i915_debugfs.c
index 7a90a2e32c9f1..e9b79c2c37d84 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -740,15 +740,19 @@ static int
i915_drop_caches_set(void *data, u64 val)
{
struct drm_i915_private *i915 = data;
+ struct intel_gt *gt;
unsigned int flags;
+ unsigned int i;
int ret;
drm_dbg(>drm, "Dropping caches: 0x%08llx [0x%08llx]\n",
val, val & DROP_ALL);
- ret = gt_drop_caches(to_gt(i915), val);
- if (ret)
- return ret;
+ for_each_gt(gt, i915, i) {
+ ret = gt_drop_caches(gt, val);
+ if (ret)
+ return ret;
+ }
fs_reclaim_acquire(GFP_KERNEL);
flags = memalloc_noreclaim_save();
--
2.40.1
Re: [Nouveau] [RFC, drm-misc-next v4 0/9] PCI/VGA: Allowing the user to select the primary video adapter at boot time
Am 07.09.23 um 14:32 schrieb suijingfeng: Hi, On 2023/9/7 17:08, Christian König wrote: Well, I have over 25 years of experience with display hardware and what you describe here was never an issue. I want to give you an example to let you know more. I have a ASRock AD2550B-ITX board[1], When another discrete video card is mounted into it mini PCIe slot or PCI slot, The IGD cannot be the primary display adapter anymore. The display is totally black. I have try to draft a few trivial patch to help fix this[2]. And I want to use the IGD as primary, does this count as an issue? No, this is completely expected behavior and a limitation of the hardware design. As far as I know both AMD and Intel GPUs work the same here. Regards, Christian. [1] https://www.asrock.com/mb/Intel/AD2550-ITX/ [2] https://patchwork.freedesktop.org/series/123073/
Re: [Nouveau] [RFC, drm-misc-next v4 0/9] PCI/VGA: Allowing the user to select the primary video adapter at boot time
Hi, On 2023/9/7 17:08, Christian König wrote: Well, I have over 25 years of experience with display hardware and what you describe here was never an issue. I want to give you an example to let you know more. I have a ASRock AD2550B-ITX board[1], When another discrete video card is mounted into it mini PCIe slot or PCI slot, The IGD cannot be the primary display adapter anymore. The display is totally black. I have try to draft a few trivial patch to help fix this[2]. And I want to use the IGD as primary, does this count as an issue? [1] https://www.asrock.com/mb/Intel/AD2550-ITX/ [2] https://patchwork.freedesktop.org/series/123073/
RE: [RFC 01/33] drm/doc/rfc: Add RFC document for proposed Plane Color Pipeline
> -Original Message- > From: Pekka Paalanen > Sent: Tuesday, September 5, 2023 5:03 PM > To: Shankar, Uma > Cc: intel-...@lists.freedesktop.org; Borah, Chaitanya Kumar > ; dri-devel@lists.freedesktop.org; wayland- > de...@lists.freedesktop.org; Harry Wentland ; > Sebastian Wick ; ville.syrj...@linux.intel.com; > Jonas Adahl > Subject: Re: [RFC 01/33] drm/doc/rfc: Add RFC document for proposed Plane > Color Pipeline > > On Mon, 4 Sep 2023 13:44:49 + > "Shankar, Uma" wrote: > > > > -Original Message- > > > From: dri-devel On Behalf > > > Of Pekka Paalanen > > > Sent: Wednesday, August 30, 2023 5:59 PM > > > To: Shankar, Uma > > > Cc: intel-...@lists.freedesktop.org; Borah, Chaitanya Kumar > > > ; dri-devel@lists.freedesktop.org; > > > wayland- de...@lists.freedesktop.org > > > Subject: Re: [RFC 01/33] drm/doc/rfc: Add RFC document for proposed > > > Plane Color Pipeline > > > > > > On Wed, 30 Aug 2023 08:59:36 + > > > "Shankar, Uma" wrote: > > > > > > > > -Original Message- > > > > > From: Harry Wentland > > > > > Sent: Wednesday, August 30, 2023 1:10 AM > > > > > To: Shankar, Uma ; > > > > > intel-...@lists.freedesktop.org; dri- > > > > > de...@lists.freedesktop.org > > > > > Cc: Borah, Chaitanya Kumar ; > > > > > wayland- de...@lists.freedesktop.org > > > > > Subject: Re: [RFC 01/33] drm/doc/rfc: Add RFC document for > > > > > proposed Plane Color Pipeline > > > > > > > > > > > > > > > > > > > > On 2023-08-29 12:03, Uma Shankar wrote: > > > > > > Add the documentation for the new proposed Plane Color Pipeline. > > > > > > > > > > > > Co-developed-by: Chaitanya Kumar Borah > > > > > > > > > > > > Signed-off-by: Chaitanya Kumar Borah > > > > > > > > > > > > Signed-off-by: Uma Shankar > > > > > > --- > > > > > > .../gpu/rfc/plane_color_pipeline.rst | 394 > > > > > > ++ > > > > > > 1 file changed, 394 insertions(+) > > > > > > create mode 100644 > > > > > > Documentation/gpu/rfc/plane_color_pipeline.rst > > > > > > > > > > > > diff --git a/Documentation/gpu/rfc/plane_color_pipeline.rst > > > > > > b/Documentation/gpu/rfc/plane_color_pipeline.rst > > > > > > new file mode 100644 > > > > > > index ..60ce515b6ea7 > > > > > > --- /dev/null > > > > > > +++ b/Documentation/gpu/rfc/plane_color_pipeline.rst > > > > > > ... > > > > > > Hi Uma! > > > > Thanks Pekka for the feedback and useful inputs. > > Hi Uma, > > sorry to say, but the overall feeling I get from this proposal is that it is > just the > current color related KMS properties wrapped in a pipeline blob. That is not > the > re-design I believe we are looking for, and the feeling is based on several > details > that are just copied from the current property design. Also the "private" > stuff has > to go. Hi Pekka, Ok, got the concerns in general. We will try to evaluate in deeper detail the property based design and come back if there are some issues or inputs. At Intel we don't need private as of now, but we thought of having an option to enable any custom hardware or vendor. But we can drop the same for now if community doesn't feel the need for it. > All the varying LUT entries, varying LUT precision, 1D/3D LUTs, varying LUT > tap > distribution, and parametrized curves are good development, but right now we > are looking at things one step higher level: the overall color pipeline > design and > how to represent any operation. Most of this series is considering details > below > the current attention level, hence I'm paying attention only to the first few > patches. We will need to precisely describe the hardware in userspace. Number of luts, precision, segments etc.., we can't just pass EOTF's as enum from userspace and let driver put hardcoded values to LUT. This will be nothing but an extension of descriptive behaviour. This will be needed as there are multiple colorspaces possible and even LUTS can be used to perform tone mapping. So, we need userspace to be able to program luts directly. This is something we must expose to userspace. We will check if this can be fitted in property based approach. Thanks for all the feedback and comments on the design. We will work on it and get back. Regards, Uma Shankar > More below. > > > > > > > +This color pipeline is then packaged within a blob for the > > > > > > +user space to retrieve it. Details can be found in the next > > > > > > +section > > > > > > + > > > > > > > > > > Not sure I like blobs that contain other blob ids. > > > > > > > > It provides flexibility and helps with just one interface to > > > > userspace. Its easy to handle and manage once we get the hang of it . > > > > > > > > We can clearly define the steps of parsing and data structures to > > > > be used while interpreting and parsing the blobs. > > > > > > Don't forget extendability. Possibly every single struct will need > > > some kind of versioning, and then it's not simple to parse anymore. > > > Add to that new/old
Re: [PATCH] MAINTAINERS: Update DRM DRIVERS FOR FREESCALE IMX entry
On Mi, 2023-09-06 at 07:28 -0700, Douglas Anderson wrote: > As per the discussion on the lists [1], changes to this driver > generally flow through drm-misc. If they need to be coordinated with > v4l2 they sometimes go through Philipp Zabel's tree instead. List both > trees in MAINTAINERS. Also update the title of this driver to specify > that it's just for IMX 5/6 since, as per Philipp "There are a lot more > i.MX that do not use IPUv3 than those that do." > > [1] > https://lore.kernel.org/r/d56dfb568711b4b932edc9601010feda020c2c22.ca...@pengutronix.de > > Signed-off-by: Douglas Anderson Thank you, Acked-by: Philipp Zabel regards Philipp
Re: [PATCH v6 02/20] drm/gpuva_mgr: Helper to get range of unmap from a remap op.
On Wed, 06 Sep 2023, Sarah Walker wrote:
> From: Donald Robson
>
> Signed-off-by: Donald Robson
> ---
> include/drm/drm_gpuva_mgr.h | 27 +++
> 1 file changed, 27 insertions(+)
>
> diff --git a/include/drm/drm_gpuva_mgr.h b/include/drm/drm_gpuva_mgr.h
> index ed8d50200cc3..be7b3a6d7e67 100644
> --- a/include/drm/drm_gpuva_mgr.h
> +++ b/include/drm/drm_gpuva_mgr.h
> @@ -703,4 +703,31 @@ void drm_gpuva_remap(struct drm_gpuva *prev,
>
> void drm_gpuva_unmap(struct drm_gpuva_op_unmap *op);
>
> +/**
> + * drm_gpuva_op_remap_get_unmap_range() - Helper to get the start and range
> of
> + * the unmap stage of a remap op.
> + * @op: Remap op.
> + * @start_addr: Output pointer for the start of the required unmap.
> + * @range: Output pointer for the length of the required unmap.
> + *
> + * These parameters can then be used by the caller to unmap memory pages that
> + * are no longer required.
> + */
> +static __always_inline void
IMO __always_inline *always* requires a justification in the commit
message.
BR,
Jani.
> +drm_gpuva_op_remap_get_unmap_range(const struct drm_gpuva_op_remap *op,
> +u64 *start_addr, u64 *range)
> +{
> + const u64 va_start = op->prev ?
> + op->prev->va.addr + op->prev->va.range :
> + op->unmap->va->va.addr;
> + const u64 va_end = op->next ?
> +op->next->va.addr :
> +op->unmap->va->va.addr + op->unmap->va->va.range;
> +
> + if (start_addr)
> + *start_addr = va_start;
> + if (range)
> + *range = va_end - va_start;
> +}
> +
> #endif /* __DRM_GPUVA_MGR_H__ */
--
Jani Nikula, Intel Open Source Graphics Center
Re: [PATCH 5/6] drm/edid: add helpers to get/set struct cea_sad from/to 3-byte sad
Hi Jani,
kernel test robot noticed the following build warnings:
[auto build test WARNING on drm-tip/drm-tip]
url:
https://github.com/intel-lab-lkp/linux/commits/Jani-Nikula/drm-edid-split-out-drm_eld-h-from-drm_edid-h/20230907-173011
base: git://anongit.freedesktop.org/drm/drm-tip drm-tip
patch link:
https://lore.kernel.org/r/eba53a0904126fb904a5190750002695350f44eb.1694078430.git.jani.nikula%40intel.com
patch subject: [PATCH 5/6] drm/edid: add helpers to get/set struct cea_sad
from/to 3-byte sad
config: arm-defconfig
(https://download.01.org/0day-ci/archive/20230907/202309071934.azntzcvc-...@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 13.2.0
reproduce (this is a W=1 build):
(https://download.01.org/0day-ci/archive/20230907/202309071934.azntzcvc-...@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot
| Closes:
https://lore.kernel.org/oe-kbuild-all/202309071934.azntzcvc-...@intel.com/
All warnings (new ones prefixed by >>):
>> drivers/gpu/drm/drm_edid.c:5505:6: warning: no previous prototype for
>> 'drm_edid_cta_sad_get' [-Wmissing-prototypes]
5505 | void drm_edid_cta_sad_get(const struct cea_sad *cta_sad, u8 *sad)
| ^~~~
>> drivers/gpu/drm/drm_edid.c:5515:6: warning: no previous prototype for
>> 'drm_edid_cta_sad_set' [-Wmissing-prototypes]
5515 | void drm_edid_cta_sad_set(struct cea_sad *cta_sad, const u8 *sad)
| ^~~~
vim +/drm_edid_cta_sad_get +5505 drivers/gpu/drm/drm_edid.c
5501
5502 /*
5503 * Get 3-byte SAD buf from struct cea_sad.
5504 */
> 5505 void drm_edid_cta_sad_get(const struct cea_sad *cta_sad, u8 *sad)
5506 {
5507 sad[0] = cta_sad->format << 3 | cta_sad->channels;
5508 sad[1] = cta_sad->freq;
5509 sad[2] = cta_sad->byte2;
5510 }
5511
5512 /*
5513 * Set struct cea_sad from 3-byte SAD buf.
5514 */
> 5515 void drm_edid_cta_sad_set(struct cea_sad *cta_sad, const u8 *sad)
5516 {
5517 cta_sad->format = (sad[0] & 0x78) >> 3;
5518 cta_sad->channels = sad[0] & 0x07;
5519 cta_sad->freq = sad[1] & 0x7f;
5520 cta_sad->byte2 = sad[2];
5521 }
5522
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Re: [PATCH v11] drm: Add initial ci/ subdirectory
Hi, On 04/09/2023 09:54, Daniel Vetter wrote: On Wed, 30 Aug 2023 at 17:14, Helen Koike > wrote: >> >> On 30/08/2023 11:57, Maxime Ripard wrote: >>> >>> I agree that we need a baseline, but that baseline should be >>> defined by the tests own merits, not their outcome on a >>> particular platform. >>> >>> In other words, I want all drivers to follow that baseline, and >>> if they don't it's a bug we should fix, and we should be vocal >>> about it. We shouldn't ignore the test because it's broken. >>> >>> Going back to the example I used previously, >>> kms_hdmi_inject@inject-4k shouldn't fail on mt8173, ever. That's >>> a bug. Ignoring it and reporting that "all tests are good" isn't >>> ok. There's something wrong with that driver and we should fix >>> it. >>> >>> Or at the very least, explain in much details what is the >>> breakage, how we noticed it, why we can't fix it, and how to >>> reproduce it. >>> >>> Because in its current state, there's no chance we'll ever go >>> over that test list and remove some of them. Or even know if, if >>> we ever fix a bug somewhere, we should remove a flaky or failing >>> test. >>> >>> [...] >>> we need to have a clear view about which tests are not corresponding to it, so we can start fixing. First we need to be aware of the issues so we can start fixing them, otherwise we will stay in the "no tests no failures" ground :) >>> >>> I think we have somewhat contradicting goals. You want to make >>> regression testing, so whatever test used to work in the past >>> should keep working. That's fine, but it's different from >>> "expectations about what the DRM drivers are supposed to pass in >>> the IGT test suite" which is about validation, ie "all KMS >>> drivers must behave this way". >> >> [...] >> >> >> We could have some policy: if you want to enable a certain device >> in the CI, you need to make sure it passes all tests first to force >> people to go fix the issues, but maybe it would be a big barrier. >> >> I'm afraid that, if a test fail (and it is a clear bug), people >> would just say "work for most of the cases, this is not a priority >> to fix" and just start ignoring the CI, this is why I think >> regression tests is a good way to start with. > > I think eventually we need to get to both goals, but currently > driver and test quality just isn't remotely there. > > I think a good approach would be if CI work focuses on the pure sw > tests first, so kunit and running igt against vgem/vkms. And then we > could use that to polish a set of must-pass igt testcases, which > also drivers in general are supposed to pass. Plus ideally weed out > the bad igts that aren't reliable enough or have bad assumptions. > > For hardware I think it will take a very long time until we get to a > point where CI can work without a test result list, we're nowhere > close to that. But for virtual driver this really should be > achievable, albeit with a huge amount of effort required to get > there I think. Yeah, this is what our experience with Mesa (in particular) has taught us. Having 100% of the tests pass 100% of the time on 100% of the platforms is a great goal that everyone should aim for. But it will also never happen. Firstly, we're just not there yet today. Every single GPU-side DRM driver has userspace-triggerable faults which cause occasional errors in GL/Vulkan tests. Every single one. We deal with these in Mesa by retrying; if we didn't retry, across the breadth of hardware we test, I'd expect 99% of should-succeed merges to fail because of these intermittent bugs in the DRM drivers. We don't have the same figure for KMS - because we don't test it - but I'd be willing to bet no driver is 100% if you run tests often enough. Secondly, we will never be there. If we could pause for five years and sit down making all the current usecases for all the current hardware on the current kernel run perfectly, we'd probably get there. But we can't: there's new hardware, new userspace, and hundreds of new kernel trees. Even without the first two, what happens when the Arm SMMU maintainers (choosing a random target to pick on, sorry Robin) introduce subtle breakage which makes a lot of tests fail some of the time? Do we refuse to backmerge Linus into DRM until it's fixed, or do we disable all testing on Arm until it's fixed? When we've done that, what happens when we re-enable testing, and discover that a bunch of tests get broken because we haven't been testing? Thirdly, hardware is capricious. 'This board doesn't make it to u-boot' is a clear infrastructure error, but if you test at sufficient scale, cold solder or failing caps surface way more often than you might think. And you can't really pick those out by any other means than running at scale, dealing with non-binary results, and looking at the trends over time. (Again this is something we do in Mesa - we graph test failures per
Re: [PATCH drm-misc-next v2 5/7] drm/gpuvm: add an abstraction for a VM / BO combination
On Thu, 7 Sep 2023 11:41:11 +0200
Danilo Krummrich wrote:
> On 9/7/23 10:42, Boris Brezillon wrote:
> > On Wed, 6 Sep 2023 23:47:13 +0200
> > Danilo Krummrich wrote:
> >
> >> +void drm_gpuvm_bo_destroy(struct kref *kref);
> >
> > I usually keep kref's release functions private so people are not
> > tempted to use them.
>
> I think I did that because drm_gpuvm_bo_put() needs it.
Yeah, but you can move the drm_gpuvm_bo_put() implementation to the C
file and make this one private.
>
> >
> >> +
> >> +/**
> >> + * drm_gpuvm_bo_get() - acquire a struct drm_gpuvm_bo reference
> >> + * @vm_bo: the _gpuvm_bo to acquire the reference of
> >> + *
> >> + * This function acquires an additional reference to @vm_bo. It is
> >> illegal to
> >> + * call this without already holding a reference. No locks required.
> >> + */
> >> +static inline struct drm_gpuvm_bo *
> >> +drm_gpuvm_bo_get(struct drm_gpuvm_bo *vm_bo)
> >> +{
> >> + kref_get(_bo->kref);
> >> + return vm_bo;
> >> +}
> >> +
> >> +/**
> >> + * drm_gpuvm_bo_put() - drop a struct drm_gpuvm_bo reference
> >> + * @vm_bo: the _gpuvm_bo to release the reference of
> >> + *
> >> + * This releases a reference to @vm_bo.
> >> + */
> >> +static inline void
> >> +drm_gpuvm_bo_put(struct drm_gpuvm_bo *vm_bo)
> >> +{
> >> + kref_put(_bo->kref, drm_gpuvm_bo_destroy);
> >
> > nit: can we have
> >
> > if (vm_bo)
> > kref_put(_bo->kref, drm_gpuvm_bo_destroy);
> >
> > so we don't have to bother testing the vm_bo value in the error/cleanup
> > paths?
> >
> >> +}
> >> +
> >
>
Re: [PATCH 4/6] drm: ci: Enable configs to fix mt8173 boot hang issue
Il 25/08/23 14:24, Vignesh Raman ha scritto: Enable regulator Enable MT6397 RTC driver Signed-off-by: Vignesh Raman --- drivers/gpu/drm/ci/arm64.config | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/gpu/drm/ci/arm64.config b/drivers/gpu/drm/ci/arm64.config index 817e18ddfd4f..ea7a6cceff40 100644 --- a/drivers/gpu/drm/ci/arm64.config +++ b/drivers/gpu/drm/ci/arm64.config @@ -184,6 +184,8 @@ CONFIG_HW_RANDOM_MTK=y CONFIG_MTK_DEVAPC=y CONFIG_PWM_MTK_DISP=y CONFIG_MTK_CMDQ=y +CONFIG_REGULATOR_DA9211=y +CONFIG_RTC_DRV_MT6397=y I wonder if it'd be a better idea to simply add those to the defconfig instead as CONFIG_REGULATOR_DA9211=m CONFIG_RTC_DRV_MT6397=m Any opinion on this? Matthias? Anyone else? Cheers, Angelo # For nouveau. Note that DRM must be a module so that it's loaded after NFS is up to provide the firmware. CONFIG_ARCH_TEGRA=y
Re: [Intel-xe] [PATCH 1/3] drm/kunit: Avoid a driver uaf
Hi, Maxime, On 9/6/23 12:08, Maxime Ripard wrote: On Tue, Sep 05, 2023 at 02:43:00PM +0200, Thomas Hellström wrote: Hi maxime, On 9/5/23 14:06, Maxime Ripard wrote: On Tue, Sep 05, 2023 at 10:58:30AM +0200, Thomas Hellström wrote: when using __drm_kunit_helper_alloc_drm_device() the driver may be dereferenced by device-managed resources up until the device is freed, which is typically later than the kunit-managed resource code frees it. I'd like to have a bit more context on how a driver can end up in that situation? I interpret the attached traces as follows. INIT: Code allocates a struct device as a kunit-managed resource. Code allocates a drm driver as a kunit-managed resource. Code allocates a drm device as a device-managed resource. EXIT: Kunit resource cleanup frees the drm driver Kunit resource cleanup frees the struct device, which starts a device-managed resource cleanup device-managed cleanup calls drm_dev_put() drm_dev_put() dereferences the (now freed) drm driver -> Boom. It should be sufficient to enable KASAN and run the drm_exec_test kunit test to trigger this. Ack. Can you put this into your commit log? Thanks! Maxime Thanks for reviewing. I'll update this and the other patch with your comments. Thanks, Thomas
Re: [PATCH v6 03/20] dt-bindings: gpu: Add Imagination Technologies PowerVR/IMG GPU
Hey,
On Wed, Sep 06, 2023 at 10:55:25AM +0100, Sarah Walker wrote:
> +examples:
> + - |
> +#include
> +#include
> +#include
> +
> +gpu: gpu@fd0 {
This "gpu" label isn't used and can be dropped if you respin.
Otherwise, this seems fine to me,
Reviewed-by: Conor Dooley
Thanks,
Conor.
> +compatible = "ti,am62-gpu", "img,img-axe";
> +reg = <0x0fd0 0x2>;
> +clocks = <_clks 187 0>;
> +clock-names = "core";
> +interrupts = ;
> +power-domains = <_pds 187 TI_SCI_PD_EXCLUSIVE>;
> +};
signature.asc
Description: PGP signature
Re: [PATCH v16 15/20] drm/shmem-helper: Add memory shrinker
Hi Dmitry,
kernel test robot noticed the following build warnings:
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url:
https://github.com/intel-lab-lkp/linux/commits/Dmitry-Osipenko/drm-shmem-helper-Fix-UAF-in-error-path-when-freeing-SGT-of-imported-GEM/20230904-011037
base: linus/master
patch link:
https://lore.kernel.org/r/20230903170736.513347-16-dmitry.osipenko%40collabora.com
patch subject: [PATCH v16 15/20] drm/shmem-helper: Add memory shrinker
config: x86_64-randconfig-161-20230907
(https://download.01.org/0day-ci/archive/20230907/202309070814.jygjojzy-...@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce:
(https://download.01.org/0day-ci/archive/20230907/202309070814.jygjojzy-...@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot
| Reported-by: Dan Carpenter
| Closes: https://lore.kernel.org/r/202309070814.jygjojzy-...@intel.com/
smatch warnings:
drivers/gpu/drm/drm_gem_shmem_helper.c:733 drm_gem_shmem_fault() error: we
previously assumed 'shmem->pages' could be null (see line 724)
vim +733 drivers/gpu/drm/drm_gem_shmem_helper.c
2194a63a818db7 Noralf Trønnes 2019-03-12 702 static vm_fault_t
drm_gem_shmem_fault(struct vm_fault *vmf)
2194a63a818db7 Noralf Trønnes 2019-03-12 703 {
2194a63a818db7 Noralf Trønnes 2019-03-12 704 struct vm_area_struct
*vma = vmf->vma;
2194a63a818db7 Noralf Trønnes 2019-03-12 705 struct drm_gem_object
*obj = vma->vm_private_data;
2194a63a818db7 Noralf Trønnes 2019-03-12 706 struct
drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj);
2194a63a818db7 Noralf Trønnes 2019-03-12 707 loff_t num_pages =
obj->size >> PAGE_SHIFT;
d611b4a0907cec Neil Roberts2021-02-23 708 vm_fault_t ret;
2194a63a818db7 Noralf Trønnes 2019-03-12 709 struct page *page;
11d5a4745e00e7 Neil Roberts2021-02-23 710 pgoff_t page_offset;
2c607edf57db6a Dmitry Osipenko 2023-09-03 711 bool pages_unpinned;
2c607edf57db6a Dmitry Osipenko 2023-09-03 712 int err;
11d5a4745e00e7 Neil Roberts2021-02-23 713
11d5a4745e00e7 Neil Roberts2021-02-23 714 /* We don't use
vmf->pgoff since that has the fake offset */
11d5a4745e00e7 Neil Roberts2021-02-23 715 page_offset =
(vmf->address - vma->vm_start) >> PAGE_SHIFT;
2194a63a818db7 Noralf Trønnes 2019-03-12 716
21aa27ddc58269 Dmitry Osipenko 2023-05-30 717
dma_resv_lock(shmem->base.resv, NULL);
2194a63a818db7 Noralf Trønnes 2019-03-12 718
2c607edf57db6a Dmitry Osipenko 2023-09-03 719 /* Sanity-check that we
have the pages pointer when it should present */
2c607edf57db6a Dmitry Osipenko 2023-09-03 720 pages_unpinned =
(shmem->evicted || shmem->madv < 0 ||
2c607edf57db6a Dmitry Osipenko 2023-09-03 721
!refcount_read(>pages_use_count));
2c607edf57db6a Dmitry Osipenko 2023-09-03 722
drm_WARN_ON_ONCE(obj->dev, !shmem->pages ^ pages_unpinned);
2c607edf57db6a Dmitry Osipenko 2023-09-03 723
2c607edf57db6a Dmitry Osipenko 2023-09-03 @724 if (page_offset >=
num_pages || (!shmem->pages && !shmem->evicted)) {
^^^
Should this be || instead of &&? (The other thing that people do is
add "!shmem->evicted" for readability even though it doesn't need to be
checked. So maybe that's the issue, but the checker assumes it needs to
be checked).
d611b4a0907cec Neil Roberts2021-02-23 725 ret =
VM_FAULT_SIGBUS;
d611b4a0907cec Neil Roberts2021-02-23 726 } else {
2c607edf57db6a Dmitry Osipenko 2023-09-03 727 err =
drm_gem_shmem_swapin_locked(shmem);
Or maybe it's because the kbuild bot can't use the cross function db
and shmem->pages is assigned here?
2c607edf57db6a Dmitry Osipenko 2023-09-03 728 if (err) {
2c607edf57db6a Dmitry Osipenko 2023-09-03 729 ret =
VM_FAULT_OOM;
2c607edf57db6a Dmitry Osipenko 2023-09-03 730 goto
unlock;
2c607edf57db6a Dmitry Osipenko 2023-09-03 731 }
2c607edf57db6a Dmitry Osipenko 2023-09-03 732
11d5a4745e00e7 Neil Roberts2021-02-23 @733 page =
shmem->pages[page_offset];
Unchecked dereference
2194a63a818db7 Noralf Trønnes 2019-03-12 734
8b93d1d7dbd578 Daniel Vetter 2021-08-12 735 ret =
vmf_insert_pfn(vma, vmf->address, page_to_pfn(page));
d611b4a0907cec Neil Roberts2021-02-23 736 }
d611b4a0907cec Neil Roberts2021-02-23 737
2
[PATCH 09/11] drm/mediatek: add missing of_node_put
for_each_child_of_node performs an of_node_get on each
iteration, so a break out of the loop requires an
of_node_put.
This was done using the Coccinelle semantic patch
iterators/for_each_child.cocci
Signed-off-by: Julia Lawall
---
drivers/gpu/drm/mediatek/mtk_disp_ovl_adaptor.c |4 +++-
drivers/gpu/drm/mediatek/mtk_drm_drv.c |4 +++-
2 files changed, 6 insertions(+), 2 deletions(-)
diff -u -p a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
--- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
@@ -377,8 +377,10 @@ static bool mtk_drm_get_all_drm_priv(str
if (all_drm_priv[cnt] && all_drm_priv[cnt]->mtk_drm_bound)
cnt++;
- if (cnt == MAX_CRTC)
+ if (cnt == MAX_CRTC) {
+ of_node_put(node);
break;
+ }
}
if (drm_priv->data->mmsys_dev_num == cnt) {
diff -u -p a/drivers/gpu/drm/mediatek/mtk_disp_ovl_adaptor.c
b/drivers/gpu/drm/mediatek/mtk_disp_ovl_adaptor.c
--- a/drivers/gpu/drm/mediatek/mtk_disp_ovl_adaptor.c
+++ b/drivers/gpu/drm/mediatek/mtk_disp_ovl_adaptor.c
@@ -436,8 +436,10 @@ static int ovl_adaptor_comp_init(struct
}
comp_pdev = of_find_device_by_node(node);
- if (!comp_pdev)
+ if (!comp_pdev) {
+ of_node_put(node);
return -EPROBE_DEFER;
+ }
priv->ovl_adaptor_comp[id] = _pdev->dev;
[PATCH 00/11] add missing of_node_put
Add of_node_put on a break out of an of_node loop. --- arch/powerpc/kexec/file_load_64.c|8 ++-- arch/powerpc/platforms/powermac/low_i2c.c|4 +++- arch/powerpc/platforms/powermac/smp.c|4 +++- drivers/bus/arm-cci.c|4 +++- drivers/genpd/ti/ti_sci_pm_domains.c |8 ++-- drivers/gpu/drm/mediatek/mtk_disp_ovl_adaptor.c |4 +++- drivers/gpu/drm/mediatek/mtk_drm_drv.c |4 +++- drivers/media/platform/mediatek/mdp3/mtk-mdp3-comp.c |1 + drivers/mmc/host/atmel-mci.c |8 ++-- drivers/net/ethernet/broadcom/asp2/bcmasp.c |1 + drivers/soc/dove/pmu.c |5 - drivers/thermal/thermal_of.c |8 ++-- sound/soc/sh/rcar/core.c |1 + 13 files changed, 46 insertions(+), 14 deletions(-)
Re: [Nouveau] [RFC, drm-misc-next v4 0/9] PCI/VGA: Allowing the user to select the primary video adapter at boot time
On Wed, 06 Sep 2023, suijingfeng wrote: > Another limitation of the 'nomodeset' parameter is that > it is only available on recent upstream kernel. Low version > downstream kernel don't has this parameter supported yet. > So this create inconstant developing experience. I believe that > there always some people need do back-port and upstream work > for various reasons. While that may be true, it's not an argument in favour of adding new module parameters or special values to existing module parameters. They would have to be backported just as well. BR, Jani. -- Jani Nikula, Intel Open Source Graphics Center
Re: [PATCH drm-misc-next v2 5/7] drm/gpuvm: add an abstraction for a VM / BO combination
On 9/7/23 10:42, Boris Brezillon wrote:
On Wed, 6 Sep 2023 23:47:13 +0200
Danilo Krummrich wrote:
+void drm_gpuvm_bo_destroy(struct kref *kref);
I usually keep kref's release functions private so people are not
tempted to use them.
I think I did that because drm_gpuvm_bo_put() needs it.
+
+/**
+ * drm_gpuvm_bo_get() - acquire a struct drm_gpuvm_bo reference
+ * @vm_bo: the _gpuvm_bo to acquire the reference of
+ *
+ * This function acquires an additional reference to @vm_bo. It is illegal to
+ * call this without already holding a reference. No locks required.
+ */
+static inline struct drm_gpuvm_bo *
+drm_gpuvm_bo_get(struct drm_gpuvm_bo *vm_bo)
+{
+ kref_get(_bo->kref);
+ return vm_bo;
+}
+
+/**
+ * drm_gpuvm_bo_put() - drop a struct drm_gpuvm_bo reference
+ * @vm_bo: the _gpuvm_bo to release the reference of
+ *
+ * This releases a reference to @vm_bo.
+ */
+static inline void
+drm_gpuvm_bo_put(struct drm_gpuvm_bo *vm_bo)
+{
+ kref_put(_bo->kref, drm_gpuvm_bo_destroy);
nit: can we have
if (vm_bo)
kref_put(_bo->kref, drm_gpuvm_bo_destroy);
so we don't have to bother testing the vm_bo value in the error/cleanup
paths?
+}
+
Re: [PATCH 2/3] drm: Add Wrapper Functions for ELD SAD Extraction
On Mon, 21 Aug 2023, Mitul Golani wrote:
> Add wrapper functions to facilitate extracting Short Audio
> Descriptor (SAD) information from EDID-Like Data (ELD) pointers
> with different constness requirements.
>
> 1. `drm_eld_sad`: This function returns a constant `uint8_t`
> pointer and wraps the main extraction function, allowing access
> to SAD information while maintaining const correctness.
>
> 2. `drm_extract_sad_from_eld`: This function returns a mutable
> `uint8_t` pointer and implements the core logic for extracting
> SAD from the provided ELD pointer. It performs version and
> maximum channel checks to ensure proper extraction.
>
> These wrapper functions provide flexibility to the codebase,
> allowing users to access SAD information while adhering to
> const correctness when needed and modifying the pointer when
> required.
I've considered this, and in the end I think it's better to *not* make
it easier for drivers to modify the ELD buffer directly.
To that end, I wrote some helpers to modify the SADs using the existing
struct cea_sad abstraction [1]. Please have a look. It should make your
changes better focus on what you need to do here, instead of getting
distracted with ELD parsing details.
BR,
Jani.
[1] https://patchwork.freedesktop.org/series/123384/
>
> Signed-off-by: Mitul Golani
> ---
> include/drm/drm_edid.h | 15 ++-
> 1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> index 48e93f909ef6..626bc0d542eb 100644
> --- a/include/drm/drm_edid.h
> +++ b/include/drm/drm_edid.h
> @@ -418,11 +418,7 @@ static inline int drm_eld_mnl(const uint8_t *eld)
> return (eld[DRM_ELD_CEA_EDID_VER_MNL] & DRM_ELD_MNL_MASK) >>
> DRM_ELD_MNL_SHIFT;
> }
>
> -/**
> - * drm_eld_sad - Get ELD SAD structures.
> - * @eld: pointer to an eld memory structure with sad_count set
> - */
> -static inline const uint8_t *drm_eld_sad(const uint8_t *eld)
> +static uint8_t *drm_extract_sad_from_eld(uint8_t *eld)
> {
> unsigned int ver, mnl;
>
> @@ -437,6 +433,15 @@ static inline const uint8_t *drm_eld_sad(const uint8_t
> *eld)
> return eld + DRM_ELD_CEA_SAD(mnl, 0);
> }
>
> +/**
> + * drm_eld_sad - Get ELD SAD structures.
> + * @eld: pointer to an eld memory structure with sad_count set
> + */
> +static inline const uint8_t *drm_eld_sad(const uint8_t *eld)
> +{
> + return drm_extract_sad_from_eld((uint8_t *)eld);
> +}
> +
> /**
> * drm_eld_sad_count - Get ELD SAD count.
> * @eld: pointer to an eld memory structure with sad_count set
--
Jani Nikula, Intel Open Source Graphics Center
[PATCH 6/6] drm/eld: add helpers to modify the SADs of an ELD
Occasionally it's necessary for drivers to modify the SADs of an ELD,
but it's not so cool to have drivers poke at the ELD buffer directly.
Using the helpers to translate between 3-byte SAD and struct cea_sad,
add ELD helpers to get/set the SADs from/to an ELD.
Cc: Mitul Golani
Signed-off-by: Jani Nikula
---
Documentation/gpu/drm-kms-helpers.rst | 3 ++
drivers/gpu/drm/Makefile | 1 +
drivers/gpu/drm/drm_eld.c | 55 +++
include/drm/drm_eld.h | 5 +++
4 files changed, 64 insertions(+)
create mode 100644 drivers/gpu/drm/drm_eld.c
diff --git a/Documentation/gpu/drm-kms-helpers.rst
b/Documentation/gpu/drm-kms-helpers.rst
index f0f93aa62545..df91b7cd992e 100644
--- a/Documentation/gpu/drm-kms-helpers.rst
+++ b/Documentation/gpu/drm-kms-helpers.rst
@@ -366,6 +366,9 @@ EDID Helper Functions Reference
.. kernel-doc:: include/drm/drm_eld.h
:internal:
+.. kernel-doc:: drivers/gpu/drm/drm_eld.c
+ :export:
+
SCDC Helper Functions Reference
===
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 215e78e79125..632e74d823e8 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -22,6 +22,7 @@ drm-y := \
drm_drv.o \
drm_dumb_buffers.o \
drm_edid.o \
+ drm_eld.o \
drm_encoder.o \
drm_file.o \
drm_fourcc.o \
diff --git a/drivers/gpu/drm/drm_eld.c b/drivers/gpu/drm/drm_eld.c
new file mode 100644
index ..34e0d71c3550
--- /dev/null
+++ b/drivers/gpu/drm/drm_eld.c
@@ -0,0 +1,55 @@
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright © 2023 Intel Corporation
+ */
+
+#include
+#include
+
+#include "drm_internal.h"
+
+/**
+ * drm_eld_sad_get - get SAD from ELD to struct cea_sad
+ * @eld: ELD buffer
+ * @i: SAD number
+ * @cta_sad: destination struct cea_sad
+ *
+ * @return: 0 on success, or negative on errors
+ */
+int drm_eld_sad_get(const u8 *eld, int i, struct cea_sad *cta_sad)
+{
+ const u8 *sad;
+
+ if (i >= drm_eld_sad_count(eld))
+ return -EINVAL;
+
+ sad = eld + DRM_ELD_CEA_SAD(drm_eld_mnl(eld), i);
+
+ drm_edid_cta_sad_set(cta_sad, sad);
+
+ return 0;
+}
+EXPORT_SYMBOL(drm_eld_sad_get);
+
+/**
+ * drm_eld_sad_set - set SAD to ELD from struct cea_sad
+ * @eld: ELD buffer
+ * @i: SAD number
+ * @cta_sad: source struct cea_sad
+ *
+ * @return: 0 on success, or negative on errors
+ */
+int drm_eld_sad_set(u8 *eld, int i, const struct cea_sad *cta_sad)
+{
+ u8 *sad;
+
+ if (i >= drm_eld_sad_count(eld))
+ return -EINVAL;
+
+ sad = eld + DRM_ELD_CEA_SAD(drm_eld_mnl(eld), i);
+
+ drm_edid_cta_sad_get(cta_sad, sad);
+
+ return 0;
+}
+EXPORT_SYMBOL(drm_eld_sad_set);
diff --git a/include/drm/drm_eld.h b/include/drm/drm_eld.h
index 7b674256b9aa..5b320157684c 100644
--- a/include/drm/drm_eld.h
+++ b/include/drm/drm_eld.h
@@ -8,6 +8,8 @@
#include
+struct cea_sad;
+
/* ELD Header Block */
#define DRM_ELD_HEADER_BLOCK_SIZE 4
@@ -75,6 +77,9 @@ static inline int drm_eld_mnl(const u8 *eld)
return (eld[DRM_ELD_CEA_EDID_VER_MNL] & DRM_ELD_MNL_MASK) >>
DRM_ELD_MNL_SHIFT;
}
+int drm_eld_sad_get(const u8 *eld, int i, struct cea_sad *cta_sad);
+int drm_eld_sad_set(u8 *eld, int i, const struct cea_sad *cta_sad);
+
/**
* drm_eld_sad - Get ELD SAD structures.
* @eld: pointer to an eld memory structure with sad_count set
--
2.39.2
[PATCH 5/6] drm/edid: add helpers to get/set struct cea_sad from/to 3-byte sad
Add helpers to pack/unpack SADs. Both ways and non-static, as follow-up
work needs them.
Cc: Mitul Golani
Signed-off-by: Jani Nikula
---
drivers/gpu/drm/drm_edid.c | 33 -
drivers/gpu/drm/drm_internal.h | 6 ++
2 files changed, 30 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index fcdc2c314cde..1260e2688bd7 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -5499,6 +5499,27 @@ static void clear_eld(struct drm_connector *connector)
connector->audio_latency[1] = 0;
}
+/*
+ * Get 3-byte SAD buf from struct cea_sad.
+ */
+void drm_edid_cta_sad_get(const struct cea_sad *cta_sad, u8 *sad)
+{
+ sad[0] = cta_sad->format << 3 | cta_sad->channels;
+ sad[1] = cta_sad->freq;
+ sad[2] = cta_sad->byte2;
+}
+
+/*
+ * Set struct cea_sad from 3-byte SAD buf.
+ */
+void drm_edid_cta_sad_set(struct cea_sad *cta_sad, const u8 *sad)
+{
+ cta_sad->format = (sad[0] & 0x78) >> 3;
+ cta_sad->channels = sad[0] & 0x07;
+ cta_sad->freq = sad[1] & 0x7f;
+ cta_sad->byte2 = sad[2];
+}
+
/*
* drm_edid_to_eld - build ELD from EDID
* @connector: connector corresponding to the HDMI/DP sink
@@ -5593,21 +5614,15 @@ static int _drm_edid_to_sad(const struct drm_edid
*drm_edid,
cea_db_iter_for_each(db, ) {
if (cea_db_tag(db) == CTA_DB_AUDIO) {
struct cea_sad *sads;
- int j;
+ int i;
count = cea_db_payload_len(db) / 3; /* SAD is 3B */
sads = kcalloc(count, sizeof(*sads), GFP_KERNEL);
*psads = sads;
if (!sads)
return -ENOMEM;
- for (j = 0; j < count; j++) {
- const u8 *sad = >data[j * 3];
-
- sads[j].format = (sad[0] & 0x78) >> 3;
- sads[j].channels = sad[0] & 0x7;
- sads[j].freq = sad[1] & 0x7F;
- sads[j].byte2 = sad[2];
- }
+ for (i = 0; i < count; i++)
+ drm_edid_cta_sad_set([i], >data[i *
3]);
break;
}
}
diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
index ab9a472f4a47..5b7c661da459 100644
--- a/drivers/gpu/drm/drm_internal.h
+++ b/drivers/gpu/drm/drm_internal.h
@@ -22,6 +22,7 @@
*/
#include
+#include
#include
#include
@@ -31,6 +32,7 @@
#define DRM_IF_VERSION(maj, min) (maj << 16 | min)
+struct cea_sad;
struct dentry;
struct dma_buf;
struct iosys_map;
@@ -265,3 +267,7 @@ int drm_syncobj_query_ioctl(struct drm_device *dev, void
*data,
void drm_framebuffer_print_info(struct drm_printer *p, unsigned int indent,
const struct drm_framebuffer *fb);
void drm_framebuffer_debugfs_init(struct drm_device *dev);
+
+/* drm_edid.c */
+void drm_edid_cta_sad_get(const struct cea_sad *cta_sad, u8 *sad);
+void drm_edid_cta_sad_set(struct cea_sad *cta_sad, const u8 *sad);
--
2.39.2
[PATCH 2/6] drm/eld: replace uint8_t with u8
Unify on kernel types.
Cc: Mitul Golani
Signed-off-by: Jani Nikula
---
include/drm/drm_eld.h | 14 +++---
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/include/drm/drm_eld.h b/include/drm/drm_eld.h
index 9bde89bd96ea..7b674256b9aa 100644
--- a/include/drm/drm_eld.h
+++ b/include/drm/drm_eld.h
@@ -70,7 +70,7 @@
* drm_eld_mnl - Get ELD monitor name length in bytes.
* @eld: pointer to an eld memory structure with mnl set
*/
-static inline int drm_eld_mnl(const uint8_t *eld)
+static inline int drm_eld_mnl(const u8 *eld)
{
return (eld[DRM_ELD_CEA_EDID_VER_MNL] & DRM_ELD_MNL_MASK) >>
DRM_ELD_MNL_SHIFT;
}
@@ -79,7 +79,7 @@ static inline int drm_eld_mnl(const uint8_t *eld)
* drm_eld_sad - Get ELD SAD structures.
* @eld: pointer to an eld memory structure with sad_count set
*/
-static inline const uint8_t *drm_eld_sad(const uint8_t *eld)
+static inline const u8 *drm_eld_sad(const u8 *eld)
{
unsigned int ver, mnl;
@@ -98,7 +98,7 @@ static inline const uint8_t *drm_eld_sad(const uint8_t *eld)
* drm_eld_sad_count - Get ELD SAD count.
* @eld: pointer to an eld memory structure with sad_count set
*/
-static inline int drm_eld_sad_count(const uint8_t *eld)
+static inline int drm_eld_sad_count(const u8 *eld)
{
return (eld[DRM_ELD_SAD_COUNT_CONN_TYPE] & DRM_ELD_SAD_COUNT_MASK) >>
DRM_ELD_SAD_COUNT_SHIFT;
@@ -111,7 +111,7 @@ static inline int drm_eld_sad_count(const uint8_t *eld)
* This is a helper for determining the payload size of the baseline block, in
* bytes, for e.g. setting the Baseline_ELD_Len field in the ELD header block.
*/
-static inline int drm_eld_calc_baseline_block_size(const uint8_t *eld)
+static inline int drm_eld_calc_baseline_block_size(const u8 *eld)
{
return DRM_ELD_MONITOR_NAME_STRING - DRM_ELD_HEADER_BLOCK_SIZE +
drm_eld_mnl(eld) + drm_eld_sad_count(eld) * 3;
@@ -127,7 +127,7 @@ static inline int drm_eld_calc_baseline_block_size(const
uint8_t *eld)
*
* The returned value is guaranteed to be a multiple of 4.
*/
-static inline int drm_eld_size(const uint8_t *eld)
+static inline int drm_eld_size(const u8 *eld)
{
return DRM_ELD_HEADER_BLOCK_SIZE + eld[DRM_ELD_BASELINE_ELD_LEN] * 4;
}
@@ -139,7 +139,7 @@ static inline int drm_eld_size(const uint8_t *eld)
* The returned value is the speakers mask. User has to use %DRM_ELD_SPEAKER
* field definitions to identify speakers.
*/
-static inline u8 drm_eld_get_spk_alloc(const uint8_t *eld)
+static inline u8 drm_eld_get_spk_alloc(const u8 *eld)
{
return eld[DRM_ELD_SPEAKER] & DRM_ELD_SPEAKER_MASK;
}
@@ -151,7 +151,7 @@ static inline u8 drm_eld_get_spk_alloc(const uint8_t *eld)
* The caller need to use %DRM_ELD_CONN_TYPE_HDMI or %DRM_ELD_CONN_TYPE_DP to
* identify the display type connected.
*/
-static inline u8 drm_eld_get_conn_type(const uint8_t *eld)
+static inline u8 drm_eld_get_conn_type(const u8 *eld)
{
return eld[DRM_ELD_SAD_COUNT_CONN_TYPE] & DRM_ELD_CONN_TYPE_MASK;
}
--
2.39.2
[PATCH 4/6] drm/edid: use a temp variable for sads to drop one level of dereferences
It's arguably easier on the eyes, and drops a set of parenthesis too.
Cc: Mitul Golani
Signed-off-by: Jani Nikula
---
drivers/gpu/drm/drm_edid.c | 16 +---
1 file changed, 9 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 2025970816c9..fcdc2c314cde 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -5583,7 +5583,7 @@ static void drm_edid_to_eld(struct drm_connector
*connector,
}
static int _drm_edid_to_sad(const struct drm_edid *drm_edid,
- struct cea_sad **sads)
+ struct cea_sad **psads)
{
const struct cea_db *db;
struct cea_db_iter iter;
@@ -5592,19 +5592,21 @@ static int _drm_edid_to_sad(const struct drm_edid
*drm_edid,
cea_db_iter_edid_begin(drm_edid, );
cea_db_iter_for_each(db, ) {
if (cea_db_tag(db) == CTA_DB_AUDIO) {
+ struct cea_sad *sads;
int j;
count = cea_db_payload_len(db) / 3; /* SAD is 3B */
- *sads = kcalloc(count, sizeof(**sads), GFP_KERNEL);
- if (!*sads)
+ sads = kcalloc(count, sizeof(*sads), GFP_KERNEL);
+ *psads = sads;
+ if (!sads)
return -ENOMEM;
for (j = 0; j < count; j++) {
const u8 *sad = >data[j * 3];
- (*sads)[j].format = (sad[0] & 0x78) >> 3;
- (*sads)[j].channels = sad[0] & 0x7;
- (*sads)[j].freq = sad[1] & 0x7F;
- (*sads)[j].byte2 = sad[2];
+ sads[j].format = (sad[0] & 0x78) >> 3;
+ sads[j].channels = sad[0] & 0x7;
+ sads[j].freq = sad[1] & 0x7F;
+ sads[j].byte2 = sad[2];
}
break;
}
--
2.39.2
[PATCH 3/6] drm/edid: include drm_eld.h only where required
Reduce the dependencies on drm_eld.h. Some files might be able to drop the dependency on drm_edid.h too with the direct inclusion of drm_eld.h. Cc: Mitul Golani Signed-off-by: Jani Nikula --- drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c| 1 + drivers/gpu/drm/drm_edid.c | 1 + drivers/gpu/drm/i915/display/intel_audio.c | 1 + drivers/gpu/drm/i915/display/intel_crtc_state_dump.c | 1 + drivers/gpu/drm/i915/display/intel_sdvo.c| 1 + drivers/gpu/drm/nouveau/dispnv50/disp.c | 1 + drivers/gpu/drm/radeon/radeon_audio.c| 1 + drivers/gpu/drm/tegra/hdmi.c | 1 + drivers/gpu/drm/tegra/sor.c | 1 + include/drm/drm_edid.h | 1 - sound/core/pcm_drm_eld.c | 1 + sound/soc/codecs/hdac_hdmi.c | 1 + sound/soc/codecs/hdmi-codec.c| 1 + sound/x86/intel_hdmi_audio.c | 1 + 14 files changed, 13 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c index 268cb99a4c4b..fe7e307ae7f9 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -86,6 +86,7 @@ #include #include #include +#include #include #include #include diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 39dd3f694544..2025970816c9 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -41,6 +41,7 @@ #include #include #include +#include #include #include diff --git a/drivers/gpu/drm/i915/display/intel_audio.c b/drivers/gpu/drm/i915/display/intel_audio.c index 19605264a35c..39f5b698e08a 100644 --- a/drivers/gpu/drm/i915/display/intel_audio.c +++ b/drivers/gpu/drm/i915/display/intel_audio.c @@ -25,6 +25,7 @@ #include #include +#include #include #include "i915_drv.h" diff --git a/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c b/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c index 8d4640d0fd34..fcddd6d81768 100644 --- a/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c +++ b/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c @@ -4,6 +4,7 @@ */ #include +#include #include "i915_drv.h" #include "intel_crtc_state_dump.h" diff --git a/drivers/gpu/drm/i915/display/intel_sdvo.c b/drivers/gpu/drm/i915/display/intel_sdvo.c index 135a2527fd1b..6abae283998e 100644 --- a/drivers/gpu/drm/i915/display/intel_sdvo.c +++ b/drivers/gpu/drm/i915/display/intel_sdvo.c @@ -35,6 +35,7 @@ #include #include #include +#include #include "i915_drv.h" #include "i915_reg.h" diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c index 4e7c9c353c51..9332aa633867 100644 --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c @@ -38,6 +38,7 @@ #include #include #include +#include #include #include #include diff --git a/drivers/gpu/drm/radeon/radeon_audio.c b/drivers/gpu/drm/radeon/radeon_audio.c index d6ccaf24ee0c..279bf130a18c 100644 --- a/drivers/gpu/drm/radeon/radeon_audio.c +++ b/drivers/gpu/drm/radeon/radeon_audio.c @@ -26,6 +26,7 @@ #include #include +#include #include "dce6_afmt.h" #include "evergreen_hdmi.h" #include "radeon.h" diff --git a/drivers/gpu/drm/tegra/hdmi.c b/drivers/gpu/drm/tegra/hdmi.c index 0ba3ca3ac509..a1fcee665023 100644 --- a/drivers/gpu/drm/tegra/hdmi.c +++ b/drivers/gpu/drm/tegra/hdmi.c @@ -24,6 +24,7 @@ #include #include #include +#include #include #include #include diff --git a/drivers/gpu/drm/tegra/sor.c b/drivers/gpu/drm/tegra/sor.c index d5a3d3f4fece..83341576630d 100644 --- a/drivers/gpu/drm/tegra/sor.c +++ b/drivers/gpu/drm/tegra/sor.c @@ -20,6 +20,7 @@ #include #include #include +#include #include #include #include diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h index 1ff52f57ab9c..e98aa6818700 100644 --- a/include/drm/drm_edid.h +++ b/include/drm/drm_edid.h @@ -25,7 +25,6 @@ #include #include -#include /* FIXME: remove this, include directly where needed */ #include struct drm_device; diff --git a/sound/core/pcm_drm_eld.c b/sound/core/pcm_drm_eld.c index 07075071972d..1cdca4d4fc9c 100644 --- a/sound/core/pcm_drm_eld.c +++ b/sound/core/pcm_drm_eld.c @@ -6,6 +6,7 @@ #include #include #include +#include #include #include diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c index 8b6b76029694..d1b53fc1efb6 100644 --- a/sound/soc/codecs/hdac_hdmi.c +++ b/sound/soc/codecs/hdac_hdmi.c @@ -16,6 +16,7 @@ #include #include #include +#include #include #include #include diff --git a/sound/soc/codecs/hdmi-codec.c b/sound/soc/codecs/hdmi-codec.c index d21f69f05342..9b01d060f7cc 100644 --- a/sound/soc/codecs/hdmi-codec.c +++
[PATCH 1/6] drm/edid: split out drm_eld.h from drm_edid.h
The drm_edid.[ch] files are starting to be a bit crowded, and with plans
to add more ELD related functionality, it's perhaps cleanest to split
the ELD code out to a header of its own.
Include drm_eld.h from drm_edid.h for starters, and leave it to
follow-up work to only include drm_eld.h where needed.
Cc: Mitul Golani
Signed-off-by: Jani Nikula
---
Documentation/gpu/drm-kms-helpers.rst | 3 +
include/drm/drm_edid.h| 149 +---
include/drm/drm_eld.h | 159 ++
3 files changed, 163 insertions(+), 148 deletions(-)
create mode 100644 include/drm/drm_eld.h
diff --git a/Documentation/gpu/drm-kms-helpers.rst
b/Documentation/gpu/drm-kms-helpers.rst
index b8ab05e42dbb..f0f93aa62545 100644
--- a/Documentation/gpu/drm-kms-helpers.rst
+++ b/Documentation/gpu/drm-kms-helpers.rst
@@ -363,6 +363,9 @@ EDID Helper Functions Reference
.. kernel-doc:: drivers/gpu/drm/drm_edid.c
:export:
+.. kernel-doc:: include/drm/drm_eld.h
+ :internal:
+
SCDC Helper Functions Reference
===
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index 882d2638708e..1ff52f57ab9c 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -25,6 +25,7 @@
#include
#include
+#include /* FIXME: remove this, include directly where needed
*/
#include
struct drm_device;
@@ -269,64 +270,6 @@ struct detailed_timing {
#define DRM_EDID_DSC_MAX_SLICES0xf
#define DRM_EDID_DSC_TOTAL_CHUNK_KBYTES0x3f
-/* ELD Header Block */
-#define DRM_ELD_HEADER_BLOCK_SIZE 4
-
-#define DRM_ELD_VER0
-# define DRM_ELD_VER_SHIFT 3
-# define DRM_ELD_VER_MASK (0x1f << 3)
-# define DRM_ELD_VER_CEA861D (2 << 3) /* supports 861D or below */
-# define DRM_ELD_VER_CANNED(0x1f << 3)
-
-#define DRM_ELD_BASELINE_ELD_LEN 2 /* in dwords! */
-
-/* ELD Baseline Block for ELD_Ver == 2 */
-#define DRM_ELD_CEA_EDID_VER_MNL 4
-# define DRM_ELD_CEA_EDID_VER_SHIFT5
-# define DRM_ELD_CEA_EDID_VER_MASK (7 << 5)
-# define DRM_ELD_CEA_EDID_VER_NONE (0 << 5)
-# define DRM_ELD_CEA_EDID_VER_CEA861 (1 << 5)
-# define DRM_ELD_CEA_EDID_VER_CEA861A (2 << 5)
-# define DRM_ELD_CEA_EDID_VER_CEA861BCD(3 << 5)
-# define DRM_ELD_MNL_SHIFT 0
-# define DRM_ELD_MNL_MASK (0x1f << 0)
-
-#define DRM_ELD_SAD_COUNT_CONN_TYPE5
-# define DRM_ELD_SAD_COUNT_SHIFT 4
-# define DRM_ELD_SAD_COUNT_MASK(0xf << 4)
-# define DRM_ELD_CONN_TYPE_SHIFT 2
-# define DRM_ELD_CONN_TYPE_MASK(3 << 2)
-# define DRM_ELD_CONN_TYPE_HDMI(0 << 2)
-# define DRM_ELD_CONN_TYPE_DP (1 << 2)
-# define DRM_ELD_SUPPORTS_AI (1 << 1)
-# define DRM_ELD_SUPPORTS_HDCP (1 << 0)
-
-#define DRM_ELD_AUD_SYNCH_DELAY6 /* in units of 2 ms */
-# define DRM_ELD_AUD_SYNCH_DELAY_MAX 0xfa/* 500 ms */
-
-#define DRM_ELD_SPEAKER7
-# define DRM_ELD_SPEAKER_MASK 0x7f
-# define DRM_ELD_SPEAKER_RLRC (1 << 6)
-# define DRM_ELD_SPEAKER_FLRC (1 << 5)
-# define DRM_ELD_SPEAKER_RC(1 << 4)
-# define DRM_ELD_SPEAKER_RLR (1 << 3)
-# define DRM_ELD_SPEAKER_FC(1 << 2)
-# define DRM_ELD_SPEAKER_LFE (1 << 1)
-# define DRM_ELD_SPEAKER_FLR (1 << 0)
-
-#define DRM_ELD_PORT_ID8 /* offsets 8..15
inclusive */
-# define DRM_ELD_PORT_ID_LEN 8
-
-#define DRM_ELD_MANUFACTURER_NAME0 16
-#define DRM_ELD_MANUFACTURER_NAME1 17
-
-#define DRM_ELD_PRODUCT_CODE0 18
-#define DRM_ELD_PRODUCT_CODE1 19
-
-#define DRM_ELD_MONITOR_NAME_STRING20 /* offsets 20..(20+mnl-1)
inclusive */
-
-#define DRM_ELD_CEA_SAD(mnl, sad) (20 + (mnl) + 3 * (sad))
-
struct edid {
u8 header[8];
/* Vendor & product info */
@@ -409,96 +352,6 @@ drm_hdmi_avi_infoframe_quant_range(struct
hdmi_avi_infoframe *frame,
const struct drm_display_mode *mode,
enum hdmi_quantization_range
rgb_quant_range);
-/**
- * drm_eld_mnl - Get ELD monitor name length in bytes.
- * @eld: pointer to an eld memory structure with mnl set
- */
-static inline int drm_eld_mnl(const uint8_t *eld)
-{
- return (eld[DRM_ELD_CEA_EDID_VER_MNL] & DRM_ELD_MNL_MASK) >>
DRM_ELD_MNL_SHIFT;
-}
-
-/**
- * drm_eld_sad - Get ELD SAD structures.
- * @eld: pointer to an eld memory structure with sad_count set
- */
-static inline const uint8_t *drm_eld_sad(const uint8_t *eld)
-{
- unsigned int ver, mnl;
-
- ver = (eld[DRM_ELD_VER] & DRM_ELD_VER_MASK) >> DRM_ELD_VER_SHIFT;
- if (ver != 2 && ver != 31)
- return NULL;
-
- mnl = drm_eld_mnl(eld);
- if (mnl > 16)
- return NULL;
-
- return eld +
[PATCH 0/6] drm/edid: split out drm_eld.[ch], add some SAD helpers
Split out drm_eld.[ch] from drm_edid.h, add some helpers to convert and
modify SADs.
This should make it easier and more robust to implement things like [1],
with ELD parsing details hidden in drm_eld.[ch].
for (i = 0; i < drm_eld_sad_count(eld); i++) {
struct cea_sad sad;
drm_eld_sad_get(eld, i, );
/* do stuff with sad */
drm_eld_sad_set(eld, i, );
}
struct cea_sad provides an easier way to manipulate CTA SADs.
** UNTESTED ***
Cc: Mitul Golani
[1]
https://patchwork.freedesktop.org/patch/msgid/20230821160004.2821445-4-mitulkumar.ajitkumar.gol...@intel.com
Jani Nikula (6):
drm/edid: split out drm_eld.h from drm_edid.h
drm/eld: replace uint8_t with u8
drm/edid: include drm_eld.h only where required
drm/edid: use a temp variable for sads to drop one level of
dereferences
drm/edid: add helpers to get/set struct cea_sad from/to 3-byte sad
drm/eld: add helpers to modify the SADs of an ELD
Documentation/gpu/drm-kms-helpers.rst | 6 +
drivers/gpu/drm/Makefile | 1 +
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 1 +
drivers/gpu/drm/drm_edid.c| 42 +++--
drivers/gpu/drm/drm_eld.c | 55 ++
drivers/gpu/drm/drm_internal.h| 6 +
drivers/gpu/drm/i915/display/intel_audio.c| 1 +
.../drm/i915/display/intel_crtc_state_dump.c | 1 +
drivers/gpu/drm/i915/display/intel_sdvo.c | 1 +
drivers/gpu/drm/nouveau/dispnv50/disp.c | 1 +
drivers/gpu/drm/radeon/radeon_audio.c | 1 +
drivers/gpu/drm/tegra/hdmi.c | 1 +
drivers/gpu/drm/tegra/sor.c | 1 +
include/drm/drm_edid.h| 148
include/drm/drm_eld.h | 164 ++
sound/core/pcm_drm_eld.c | 1 +
sound/soc/codecs/hdac_hdmi.c | 1 +
sound/soc/codecs/hdmi-codec.c | 1 +
sound/x86/intel_hdmi_audio.c | 1 +
19 files changed, 274 insertions(+), 160 deletions(-)
create mode 100644 drivers/gpu/drm/drm_eld.c
create mode 100644 include/drm/drm_eld.h
--
2.39.2
[PATCH v5] drm/mediatek: Fix coverity issue with unintentional integer overflow
1. Instead of multiplying 2 variable of different types. Change to
assign a value of one variable and then multiply the other variable.
2. Add a int variable for multiplier calculation instead of calculating
different types multiplier with dma_addr_t variable directly.
Fixes: 1a64a7aff8da ("drm/mediatek: Fix cursor plane no update")
Signed-off-by: Jason-JH.Lin
Reviewed-by: Alexandre Mergnat
Reviewed-by: AngeloGioacchino Del Regno
---
Change in v5:
Add 'coverity issue' in title and code comments.
---
drivers/gpu/drm/mediatek/mtk_drm_gem.c | 9 +-
drivers/gpu/drm/mediatek/mtk_drm_plane.c | 39 ++--
2 files changed, 38 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_gem.c
b/drivers/gpu/drm/mediatek/mtk_drm_gem.c
index 9f364df52478..f6632a0fe509 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_gem.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_gem.c
@@ -121,7 +121,14 @@ int mtk_drm_gem_dumb_create(struct drm_file *file_priv,
struct drm_device *dev,
int ret;
args->pitch = DIV_ROUND_UP(args->width * args->bpp, 8);
- args->size = args->pitch * args->height;
+
+ /*
+* Multiply 2 variables of different types,
+* for example: args->size = args->spacing * args->height;
+* may cause coverity issue with unintentional overflow.
+*/
+ args->size = args->pitch;
+ args->size *= args->height;
mtk_gem = mtk_drm_gem_create(dev, args->size, false);
if (IS_ERR(mtk_gem))
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_plane.c
b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
index db2f70ae060d..5acb03b7c6fe 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_plane.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
@@ -141,6 +141,7 @@ static void mtk_plane_update_new_state(struct
drm_plane_state *new_state,
dma_addr_t addr;
dma_addr_t hdr_addr = 0;
unsigned int hdr_pitch = 0;
+ int offset;
gem = fb->obj[0];
mtk_gem = to_mtk_gem_obj(gem);
@@ -150,8 +151,15 @@ static void mtk_plane_update_new_state(struct
drm_plane_state *new_state,
modifier = fb->modifier;
if (modifier == DRM_FORMAT_MOD_LINEAR) {
- addr += (new_state->src.x1 >> 16) * fb->format->cpp[0];
- addr += (new_state->src.y1 >> 16) * pitch;
+ /*
+* Using dma_addr_t variable to calculate with multiplier of
different types,
+* for example: addr += (new_state->src.x1 >> 16) *
fb->format->cpp[0];
+* may cause coverity issue with unintentional overflow.
+*/
+ offset = (new_state->src.x1 >> 16) * fb->format->cpp[0];
+ addr += offset;
+ offset = (new_state->src.y1 >> 16) * pitch;
+ addr += offset;
} else {
int width_in_blocks = ALIGN(fb->width, AFBC_DATA_BLOCK_WIDTH)
/ AFBC_DATA_BLOCK_WIDTH;
@@ -159,21 +167,34 @@ static void mtk_plane_update_new_state(struct
drm_plane_state *new_state,
/ AFBC_DATA_BLOCK_HEIGHT;
int x_offset_in_blocks = (new_state->src.x1 >> 16) /
AFBC_DATA_BLOCK_WIDTH;
int y_offset_in_blocks = (new_state->src.y1 >> 16) /
AFBC_DATA_BLOCK_HEIGHT;
- int hdr_size;
+ int hdr_size, hdr_offset;
hdr_pitch = width_in_blocks * AFBC_HEADER_BLOCK_SIZE;
pitch = width_in_blocks * AFBC_DATA_BLOCK_WIDTH *
AFBC_DATA_BLOCK_HEIGHT * fb->format->cpp[0];
hdr_size = ALIGN(hdr_pitch * height_in_blocks,
AFBC_HEADER_ALIGNMENT);
+ hdr_offset = hdr_pitch * y_offset_in_blocks +
+ AFBC_HEADER_BLOCK_SIZE * x_offset_in_blocks;
+
+ /*
+* Using dma_addr_t variable to calculate with multiplier of
different types,
+* for example: addr += hdr_pitch * y_offset_in_blocks;
+* may cause coverity issue with unintentional overflow.
+*/
+ hdr_addr = addr + hdr_offset;
- hdr_addr = addr + hdr_pitch * y_offset_in_blocks +
- AFBC_HEADER_BLOCK_SIZE * x_offset_in_blocks;
/* The data plane is offset by 1 additional block. */
- addr = addr + hdr_size +
- pitch * y_offset_in_blocks +
- AFBC_DATA_BLOCK_WIDTH * AFBC_DATA_BLOCK_HEIGHT *
- fb->format->cpp[0] * (x_offset_in_blocks + 1);
+ offset = pitch * y_offset_in_blocks +
+AFBC_DATA_BLOCK_WIDTH * AFBC_DATA_BLOCK_HEIGHT *
+fb->format->cpp[0] * (x_offset_in_blocks + 1);
+
+ /*
+* Using dma_addr_t variable to calculate with multiplier of
different types,
+* for example: addr += pitch * y_offset_in_blocks;
+
Re: [PATCH drm-misc-next v2 5/7] drm/gpuvm: add an abstraction for a VM / BO combination
On 9/7/23 10:16, Boris Brezillon wrote:
On Wed, 6 Sep 2023 23:47:13 +0200
Danilo Krummrich wrote:
@@ -812,15 +967,20 @@ EXPORT_SYMBOL_GPL(drm_gpuva_remove);
/**
* drm_gpuva_link() - link a _gpuva
* @va: the _gpuva to link
+ * @vm_bo: the _gpuvm_bo to add the _gpuva to
*
- * This adds the given to the GPU VA list of the _gem_object it is
- * associated with.
+ * This adds the given to the GPU VA list of the _gpuvm_bo and the
+ * _gpuvm_bo to the _gem_object it is associated with.
+ *
+ * For every _gpuva entry added to the _gpuvm_bo an additional
+ * reference of the latter is taken.
*
* This function expects the caller to protect the GEM's GPUVA list against
- * concurrent access using the GEMs dma_resv lock.
+ * concurrent access using either the GEMs dma_resv lock or a driver specific
+ * lock set through drm_gem_gpuva_set_lock().
*/
void
-drm_gpuva_link(struct drm_gpuva *va)
+drm_gpuva_link(struct drm_gpuva *va, struct drm_gpuvm_bo *vm_bo)
{
struct drm_gem_object *obj = va->gem.obj;
@@ -829,7 +989,10 @@ drm_gpuva_link(struct drm_gpuva *va)
drm_gem_gpuva_assert_lock_held(obj);
- list_add_tail(>gem.entry, >gpuva.list);
+ drm_gpuvm_bo_get(vm_bo);
Guess we should WARN if vm_obj->obj == obj, at least.
+ list_add_tail(>gem.entry, _bo->list.gpuva);
+ if (list_empty(_bo->list.entry.gem))
+ list_add_tail(_bo->list.entry.gem, >gpuva.list);
}
EXPORT_SYMBOL_GPL(drm_gpuva_link);
@@ -840,20 +1003,40 @@ EXPORT_SYMBOL_GPL(drm_gpuva_link);
* This removes the given from the GPU VA list of the _gem_object it
is
* associated with.
*
+ * This removes the given from the GPU VA list of the _gpuvm_bo and
+ * the _gpuvm_bo from the _gem_object it is associated with in case
+ * this call unlinks the last _gpuva from the _gpuvm_bo.
+ *
+ * For every _gpuva entry removed from the _gpuvm_bo a reference of
+ * the latter is dropped.
+ *
* This function expects the caller to protect the GEM's GPUVA list against
- * concurrent access using the GEMs dma_resv lock.
+ * concurrent access using either the GEMs dma_resv lock or a driver specific
+ * lock set through drm_gem_gpuva_set_lock().
*/
void
drm_gpuva_unlink(struct drm_gpuva *va)
{
struct drm_gem_object *obj = va->gem.obj;
+ struct drm_gpuvm_bo *vm_bo;
if (unlikely(!obj))
return;
drm_gem_gpuva_assert_lock_held(obj);
+ vm_bo = __drm_gpuvm_bo_find(va->vm, obj);
Could we add a drm_gpuva::vm_bo field so we don't have to search the
vm_bo here, and maybe drop the drm_gpuva::vm and drm_gpuva::obj fields,
since drm_gpuvm_bo contains both the vm and the GEM object. I know that
means adding an extra indirection + allocation for drivers that don't
want to use drm_gpuva_[un]link(), but I wonder if it's not preferable
over having the information duplicated (with potential mismatch)
I was considering that and I think we can add a drm_gpuva::vm_bo field and
get rid of drm_gpuva::obj. However, I think we need to keep drm_gpuva::vm,
since it is valid for ::obj to be NULL, hence it must be valid for ::vm_bo too.
Null objects are used for sparse mappings / userptr.
+ if (WARN(!vm_bo, "GPUVA doesn't seem to be linked.\n"))
+ return;
+
list_del_init(>gem.entry);
+
+ /* This is the last mapping being unlinked for this GEM object, hence
+* also remove the VM_BO from the GEM's gpuva list.
+*/
+ if (list_empty(_bo->list.gpuva))
+ list_del_init(_bo->list.entry.gem);
+ drm_gpuvm_bo_put(vm_bo);
}
EXPORT_SYMBOL_GPL(drm_gpuva_unlink);
Re: [Nouveau] [RFC, drm-misc-next v4 0/9] PCI/VGA: Allowing the user to select the primary video adapter at boot time
Am 07.09.23 um 04:30 schrieb Sui Jingfeng: Hi, On 2023/9/6 17:40, Christian König wrote: Am 06.09.23 um 11:08 schrieb suijingfeng: Well, welcome to correct me if I'm wrong. You seem to have some very basic misunderstandings here. The term framebuffer describes some VRAM memory used for scanout. This framebuffer is exposed to userspace through some framebuffer driver, on UEFI platforms that is usually efifb but can be quite a bunch of different drivers. When the DRM drivers load they remove the previous drivers using drm_aperture_remove_conflicting_pci_framebuffers() (or similar function), but this does not mean that the framebuffer or scanout parameters are modified in any way. It just means that the framebuffer is just no longer exposed through this driver. Take over is the perfectly right description here because that's exactly what's happening. The framebuffer configuration including the VRAM memory as well as the parameters for scanout are exposed by the newly loaded DRM driver. In other words userspace can query through the DRM interfaces which monitors already driven by the hardware and so in your terminology figure out which is the primary one. I'm a little bit of not convinced about this idea, you might be correct. Well I can point you to the code if you don't believe me. But there cases where three are multiple monitors and each video card connect one. Yeah, but this is irrelevant. The key point is the configuration is taken over when the driver loads. So whatever is there before as setup (one monitor showing console, three monitors mirrored, whatever) should be there after loading the driver as well. This configuration is just immediately overwritten because nobody cares about it. It also quite common that no monitors is connected, let the machine boot first, then find a monitors to connect to a random display output. See which will display. I don't expect the primary shake with. The primary one have to be determined as early as possible, because of the VGA console and the framebuffer console may directly output the primary. Well that is simply not correct. There is not concept of "primary" display, it can just be that a monitor was brought up by the BIOS or bootloader and we take over this configuration. Get the DDC and/or HPD involved may necessary complicated the problem. There are ASpeed BMC who add a virtual connector in order to able display remotely. There are also have commands to force a connector to be connected status. It's just that as Thomas explained as well that this completely irrelevant to any modern desktop. Both X and Wayland both iterate the available devices and start rendering to them which one was used during boot doesn't really matter to them. You may be correct, but I'm still not sure. I probably need more times to investigate. Me and my colleagues are mainly using X server, the version varies from 1.20.4 and 1.21.1.4. Even this is true, the problems still exist for non-modern desktops. Well, I have over 25 years of experience with display hardware and what you describe here was never an issue. What you have is simply a broken display driver which for some reason can't handle your use case. I strongly suggest that you just completely drop this here and go into the AST driver and try to fix it. Regards, Christian. Apart from that ranting like this and trying to explain stuff to people who obviously have much better background in the topic is not going to help your patches getting upstream. Thanks for you tell me so much knowledge, I'm realized where are the problems now. I will try to resolve the concerns at the next version. Regards, Christian.
Re: [PATCH 3/3] drm/drm_exec: Work around a WW mutex lockdep oddity
Hi,
On 9/6/23 10:34, Christian König wrote:
Am 05.09.23 um 16:29 schrieb Thomas Hellström:
Hi, Christian
On 9/5/23 15:14, Christian König wrote:
Am 05.09.23 um 10:58 schrieb Thomas Hellström:
If *any* object of a certain WW mutex class is locked, lockdep will
consider *all* mutexes of that class as locked. Also the lock
allocation
tracking code will apparently register only the address of the first
mutex locked in a sequence.
This has the odd consequence that if that first mutex is unlocked and
its memory then freed, the lock alloc tracking code will assume
that memory
is freed with a held lock in there.
For now, work around that for drm_exec by releasing the first grabbed
object lock last.
Related lock alloc tracking warning:
[ 322.660067] =
[ 322.660070] WARNING: held lock freed!
[ 322.660074] 6.5.0-rc7+ #155 Tainted: G U N
[ 322.660078] -
[ 322.660081] kunit_try_catch/4981 is freeing memory
888112adc000-888112adc3ff, with a lock still held there!
[ 322.660089] 888112adc1a0
(reservation_ww_class_mutex){+.+.}-{3:3}, at:
drm_exec_lock_obj+0x11a/0x600 [drm_exec]
[ 322.660104] 2 locks held by kunit_try_catch/4981:
[ 322.660108] #0: c9000343fe18
(reservation_ww_class_acquire){+.+.}-{0:0}, at:
test_early_put+0x22f/0x490 [drm_exec_test]
[ 322.660123] #1: 888112adc1a0
(reservation_ww_class_mutex){+.+.}-{3:3}, at:
drm_exec_lock_obj+0x11a/0x600 [drm_exec]
[ 322.660135]
stack backtrace:
[ 322.660139] CPU: 7 PID: 4981 Comm: kunit_try_catch Tainted:
G U N 6.5.0-rc7+ #155
[ 322.660146] Hardware name: ASUS System Product Name/PRIME
B560M-A AC, BIOS 0403 01/26/2021
[ 322.660152] Call Trace:
[ 322.660155]
[ 322.660158] dump_stack_lvl+0x57/0x90
[ 322.660164] debug_check_no_locks_freed+0x20b/0x2b0
[ 322.660172] slab_free_freelist_hook+0xa1/0x160
[ 322.660179] ? drm_exec_unlock_all+0x168/0x2a0 [drm_exec]
[ 322.660186] __kmem_cache_free+0xb2/0x290
[ 322.660192] drm_exec_unlock_all+0x168/0x2a0 [drm_exec]
[ 322.660200] drm_exec_fini+0xf/0x1c0 [drm_exec]
[ 322.660206] test_early_put+0x289/0x490 [drm_exec_test]
[ 322.660215] ? __pfx_test_early_put+0x10/0x10 [drm_exec_test]
[ 322.660222] ? __kasan_check_byte+0xf/0x40
[ 322.660227] ? __ksize+0x63/0x140
[ 322.660233] ? drmm_add_final_kfree+0x3e/0xa0 [drm]
[ 322.660289] ? _raw_spin_unlock_irqrestore+0x30/0x60
[ 322.660294] ? lockdep_hardirqs_on+0x7d/0x100
[ 322.660301] ? __pfx_kunit_try_run_case+0x10/0x10 [kunit]
[ 322.660310] ?
__pfx_kunit_generic_run_threadfn_adapter+0x10/0x10 [kunit]
[ 322.660319] kunit_generic_run_threadfn_adapter+0x4a/0x90 [kunit]
[ 322.660328] kthread+0x2e7/0x3c0
[ 322.660334] ? __pfx_kthread+0x10/0x10
[ 322.660339] ret_from_fork+0x2d/0x70
[ 322.660345] ? __pfx_kthread+0x10/0x10
[ 322.660349] ret_from_fork_asm+0x1b/0x30
[ 322.660358]
[ 322.660818] ok 8 test_early_put
Cc: Christian König
Cc: Boris Brezillon
Cc: Danilo Krummrich
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: Thomas Hellström
---
drivers/gpu/drm/drm_exec.c | 2 +-
include/drm/drm_exec.h | 35 +++
2 files changed, 32 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/drm_exec.c b/drivers/gpu/drm/drm_exec.c
index ff69cf0fb42a..5d2809de4517 100644
--- a/drivers/gpu/drm/drm_exec.c
+++ b/drivers/gpu/drm/drm_exec.c
@@ -56,7 +56,7 @@ static void drm_exec_unlock_all(struct drm_exec
*exec)
struct drm_gem_object *obj;
unsigned long index;
- drm_exec_for_each_locked_object(exec, index, obj) {
+ drm_exec_for_each_locked_object_reverse(exec, index, obj) {
Well that's a really good catch, just one more additional thought
below.
dma_resv_unlock(obj->resv);
drm_gem_object_put(obj);
}
diff --git a/include/drm/drm_exec.h b/include/drm/drm_exec.h
index e0462361adf9..55764cf7c374 100644
--- a/include/drm/drm_exec.h
+++ b/include/drm/drm_exec.h
@@ -51,6 +51,20 @@ struct drm_exec {
struct drm_gem_object *prelocked;
};
+/**
+ * drm_exec_obj() - Return the object for a give drm_exec index
+ * @exec: Pointer to the drm_exec context
+ * @index: The index.
+ *
+ * Return: Pointer to the locked object corresponding to @index if
+ * index is within the number of locked objects. NULL otherwise.
+ */
+static inline struct drm_gem_object *
+drm_exec_obj(struct drm_exec *exec, unsigned long index)
+{
+ return index < exec->num_objects ? exec->objects[index] : NULL;
+}
+
/**
* drm_exec_for_each_locked_object - iterate over all the locked
objects
* @exec: drm_exec object
@@ -59,10 +73,23 @@ struct drm_exec {
*
* Iterate over all the locked GEM objects inside the drm_exec
object.
*/
-#define drm_exec_for_each_locked_object(exec, index, obj) \
- for (index = 0, obj = (exec)->objects[0]; \
- index < (exec)->num_objects; \
- ++index, obj =
Re: [PATCH drm-misc-next v2 2/7] drm/gpuvm: rename struct drm_gpuva_manager to struct drm_gpuvm
On 9/7/23 09:56, Boris Brezillon wrote: On Wed, 6 Sep 2023 23:47:10 +0200 Danilo Krummrich wrote: Rename struct drm_gpuva_manager to struct drm_gpuvm including corresponding functions. This way the GPUVA manager's structures align very well with the documentation of VM_BIND [1] and VM_BIND locking [2]. It also provides a better foundation for the naming of data structures and functions introduced for implementing a common dma-resv per GPU-VM including tracking of external and evicted objects in subsequent patches. I'm fine with this rename, but I feel like some bits are missing in this patch. I see a few functions taking a drm_gpuvm object and still being prefixed with drm_gpuva_ (I'm not talking about functions that only manipulate a drm_gpuva object, but those updating the the VM state, like the sm_map/unmap helpers), and I think it'd be preferable to rename the source files and the Kconfig option too. I was a bit hesitant to also rename the source files in the first place, but I think that makes sense.
[PATCH v2 3/7] fbdev/core: Fix style of code for boot-up logo
Fix a number of warnings from checkpatch.pl in this code before
moving it into a separate file. This includes
* Prefer 'unsigned int' to bare use of 'unsigned'
* space required after that ',' (ctx:VxV)
* space prohibited after that open parenthesis '('
* suspect code indent for conditional statements (16, 32)
* braces {} are not necessary for single statement blocks
No functional changes.
Signed-off-by: Thomas Zimmermann
Acked-by: Javier Martinez Canillas
---
drivers/video/fbdev/core/fbmem.c | 13 +++--
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index ee44a46a66be..98e1847e4287 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -186,7 +186,7 @@ EXPORT_SYMBOL(fb_get_buffer_offset);
#ifdef CONFIG_LOGO
-static inline unsigned safe_shift(unsigned d, int n)
+static inline unsigned int safe_shift(unsigned int d, int n)
{
return n < 0 ? d >> -n : d << n;
}
@@ -229,7 +229,9 @@ static void fb_set_logo_truepalette(struct fb_info *info,
const struct linux_logo *logo,
u32 *palette)
{
- static const unsigned char mask[] = {
0,0x80,0xc0,0xe0,0xf0,0xf8,0xfc,0xfe,0xff };
+ static const unsigned char mask[] = {
+ 0, 0x80, 0xc0, 0xe0, 0xf0, 0xf8, 0xfc, 0xfe, 0xff
+ };
unsigned char redmask, greenmask, bluemask;
int redshift, greenshift, blueshift;
int i;
@@ -247,7 +249,7 @@ static void fb_set_logo_truepalette(struct fb_info *info,
greenshift = info->var.green.offset - (8 - info->var.green.length);
blueshift = info->var.blue.offset - (8 - info->var.blue.length);
- for ( i = 0; i < logo->clutsize; i++) {
+ for (i = 0; i < logo->clutsize; i++) {
palette[i+32] = (safe_shift((clut[0] & redmask), redshift) |
safe_shift((clut[1] & greenmask), greenshift) |
safe_shift((clut[2] & bluemask), blueshift));
@@ -371,7 +373,7 @@ static void fb_rotate_logo_cw(const u8 *in, u8 *out, u32
width, u32 height)
for (i = 0; i < height; i++)
for (j = 0; j < width; j++)
- out[height * j + h - i] = *in++;
+ out[height * j + h - i] = *in++;
}
static void fb_rotate_logo_ccw(const u8 *in, u8 *out, u32 width, u32 height)
@@ -636,9 +638,8 @@ int fb_prepare_logo(struct fb_info *info, int rotate)
/* Return if no suitable logo was found */
fb_logo.logo = fb_find_logo(depth);
- if (!fb_logo.logo) {
+ if (!fb_logo.logo)
return 0;
- }
if (rotate == FB_ROTATE_UR || rotate == FB_ROTATE_UD)
yres = info->var.yres;
--
2.42.0
[PATCH v2 7/7] fbdev/core: Clean up include statements in fbmem.c
Remove all unnecessary include statements from fbmem.c. Most of them were for functionality that has meanwhile been moved into other files. Signed-off-by: Thomas Zimmermann Acked-by: Javier Martinez Canillas --- drivers/video/fbdev/core/fbmem.c | 19 +-- 1 file changed, 1 insertion(+), 18 deletions(-) diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c index 1a662a606ba6..fc206755f5f6 100644 --- a/drivers/video/fbdev/core/fbmem.c +++ b/drivers/video/fbdev/core/fbmem.c @@ -11,29 +11,12 @@ * for more details. */ -#include - -#include -#include -#include -#include -#include -#include -#include -#include -#include #include -#include -#include -#include -#include +#include #include #include -#include -#include #include -#include #include "fb_internal.h" -- 2.42.0
[PATCH v2 4/7] fbdev/core: Unexport logo helpers
The interfaces for the fbdev logo are not used outside of the fbdev
module. Hence declare the fbdev logo functions in the internal header
file and remove their symbol exports. Only build the functions if
CONFIG_LOGO has been selected.
Signed-off-by: Thomas Zimmermann
Acked-by: Javier Martinez Canillas
---
drivers/video/fbdev/core/fb_internal.h | 16
drivers/video/fbdev/core/fbmem.c | 5 -
include/linux/fb.h | 5 -
3 files changed, 16 insertions(+), 10 deletions(-)
diff --git a/drivers/video/fbdev/core/fb_internal.h
b/drivers/video/fbdev/core/fb_internal.h
index 4c8d509a0026..1116faefa034 100644
--- a/drivers/video/fbdev/core/fb_internal.h
+++ b/drivers/video/fbdev/core/fb_internal.h
@@ -21,6 +21,22 @@ static inline void fb_unregister_chrdev(void)
#endif
/* fbmem.c */
+#if defined(CONFIG_LOGO)
+extern bool fb_center_logo;
+extern int fb_logo_count;
+int fb_prepare_logo(struct fb_info *fb_info, int rotate);
+int fb_show_logo(struct fb_info *fb_info, int rotate);
+#else
+static inline int fb_prepare_logo(struct fb_info *info, int rotate)
+{
+ return 0;
+}
+static inline int fb_show_logo(struct fb_info *info, int rotate)
+{
+ return 0;
+}
+#endif /* CONFIG_LOGO */
+
extern struct class *fb_class;
extern struct mutex registration_lock;
extern struct fb_info *registered_fb[FB_MAX];
diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index 98e1847e4287..ee25ac38737d 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -696,12 +696,7 @@ int fb_show_logo(struct fb_info *info, int rotate)
return y;
}
-#else
-int fb_prepare_logo(struct fb_info *info, int rotate) { return 0; }
-int fb_show_logo(struct fb_info *info, int rotate) { return 0; }
#endif /* CONFIG_LOGO */
-EXPORT_SYMBOL(fb_prepare_logo);
-EXPORT_SYMBOL(fb_show_logo);
int
fb_pan_display(struct fb_info *info, struct fb_var_screeninfo *var)
diff --git a/include/linux/fb.h b/include/linux/fb.h
index 16c3e6d6c55d..d110676c9c83 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -591,8 +591,6 @@ extern ssize_t fb_sys_write(struct fb_info *info, const
char __user *buf,
/* drivers/video/fbmem.c */
extern int register_framebuffer(struct fb_info *fb_info);
extern void unregister_framebuffer(struct fb_info *fb_info);
-extern int fb_prepare_logo(struct fb_info *fb_info, int rotate);
-extern int fb_show_logo(struct fb_info *fb_info, int rotate);
extern char* fb_get_buffer_offset(struct fb_info *info, struct fb_pixmap *buf,
u32 size);
extern void fb_pad_unaligned_buffer(u8 *dst, u32 d_pitch, u8 *src, u32 idx,
u32 height, u32 shift_high, u32 shift_low, u32
mod);
@@ -603,9 +601,6 @@ extern int fb_get_color_depth(struct fb_var_screeninfo *var,
extern int fb_get_options(const char *name, char **option);
extern int fb_new_modelist(struct fb_info *info);
-extern bool fb_center_logo;
-extern int fb_logo_count;
-
static inline void lock_fb_info(struct fb_info *info)
{
mutex_lock(>lock);
--
2.42.0
[PATCH v2 6/7] fbdev/core: Remove empty internal helpers from fb_logo.c
Remove the two empty helpers for the case the CONFIG_FB_LOGO_EXTRA
has not been set. They are internal functions and only called once.
Providing empty replacements seems like overkill. Instead protect
the call sites with a test for CONFIG_FB_LOGO_EXTRA.
Signed-off-by: Thomas Zimmermann
Acked-by: Javier Martinez Canillas
---
drivers/video/fbdev/core/fb_logo.c | 22 ++
1 file changed, 6 insertions(+), 16 deletions(-)
diff --git a/drivers/video/fbdev/core/fb_logo.c
b/drivers/video/fbdev/core/fb_logo.c
index 6897f7a898fc..0bab8352b684 100644
--- a/drivers/video/fbdev/core/fb_logo.c
+++ b/drivers/video/fbdev/core/fb_logo.c
@@ -413,21 +413,6 @@ static int fb_show_extra_logos(struct fb_info *info, int
y, int rotate)
return y;
}
-
-#else /* !CONFIG_FB_LOGO_EXTRA */
-
-static inline int fb_prepare_extra_logos(struct fb_info *info,
-unsigned int height,
-unsigned int yres)
-{
- return height;
-}
-
-static inline int fb_show_extra_logos(struct fb_info *info, int y, int rotate)
-{
- return y;
-}
-
#endif /* CONFIG_FB_LOGO_EXTRA */
int fb_prepare_logo(struct fb_info *info, int rotate)
@@ -498,8 +483,11 @@ int fb_prepare_logo(struct fb_info *info, int rotate)
height = fb_logo.logo->height;
if (fb_center_logo)
height += (yres - fb_logo.logo->height) / 2;
+#ifdef CONFIG_FB_LOGO_EXTRA
+ height = fb_prepare_extra_logos(info, height, yres);
+#endif
- return fb_prepare_extra_logos(info, height, yres);
+ return height;
}
int fb_show_logo(struct fb_info *info, int rotate)
@@ -512,7 +500,9 @@ int fb_show_logo(struct fb_info *info, int rotate)
count = fb_logo_count < 0 ? num_online_cpus() : fb_logo_count;
y = fb_show_logo_line(info, rotate, fb_logo.logo, 0, count);
+#ifdef CONFIG_FB_LOGO_EXTRA
y = fb_show_extra_logos(info, y, rotate);
+#endif
return y;
}
--
2.42.0
[PATCH v2 5/7] fbdev/core: Move logo functions into separate source file
Move the fbdev function for displaying boot-up logos into their
own file fb_logo.c. Only build fb_logo.c if CONFIG_LOGO has been
selected. No functional changes.
v2:
* include fb_internal.h (kernel test robot)
* simplify option-parsing ifdefs
* build fb_logo.o iff CONFIG_LOGO has been set
Signed-off-by: Thomas Zimmermann
Acked-by: Javier Martinez Canillas
---
drivers/video/fbdev/core/Makefile | 2 +
drivers/video/fbdev/core/fb_internal.h | 3 +-
drivers/video/fbdev/core/fb_logo.c | 518
drivers/video/fbdev/core/fbcon.c | 2 +
drivers/video/fbdev/core/fbmem.c | 519 -
5 files changed, 524 insertions(+), 520 deletions(-)
create mode 100644 drivers/video/fbdev/core/fb_logo.c
diff --git a/drivers/video/fbdev/core/Makefile
b/drivers/video/fbdev/core/Makefile
index edfde2948e5c..36d3156dc759 100644
--- a/drivers/video/fbdev/core/Makefile
+++ b/drivers/video/fbdev/core/Makefile
@@ -23,6 +23,8 @@ fb-y+= fbcon_rotate.o fbcon_cw.o
fbcon_ud.o \
endif
endif
+fb-$(CONFIG_LOGO)+= fb_logo.o
+
obj-$(CONFIG_FB_CFB_FILLRECT) += cfbfillrect.o
obj-$(CONFIG_FB_CFB_COPYAREA) += cfbcopyarea.o
obj-$(CONFIG_FB_CFB_IMAGEBLIT) += cfbimgblt.o
diff --git a/drivers/video/fbdev/core/fb_internal.h
b/drivers/video/fbdev/core/fb_internal.h
index 1116faefa034..613832d335fe 100644
--- a/drivers/video/fbdev/core/fb_internal.h
+++ b/drivers/video/fbdev/core/fb_internal.h
@@ -20,7 +20,7 @@ static inline void fb_unregister_chrdev(void)
{ }
#endif
-/* fbmem.c */
+/* fb_logo.c */
#if defined(CONFIG_LOGO)
extern bool fb_center_logo;
extern int fb_logo_count;
@@ -37,6 +37,7 @@ static inline int fb_show_logo(struct fb_info *info, int
rotate)
}
#endif /* CONFIG_LOGO */
+/* fbmem.c */
extern struct class *fb_class;
extern struct mutex registration_lock;
extern struct fb_info *registered_fb[FB_MAX];
diff --git a/drivers/video/fbdev/core/fb_logo.c
b/drivers/video/fbdev/core/fb_logo.c
new file mode 100644
index ..6897f7a898fc
--- /dev/null
+++ b/drivers/video/fbdev/core/fb_logo.c
@@ -0,0 +1,518 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include
+#include
+
+#include "fb_internal.h"
+
+bool fb_center_logo __read_mostly;
+int fb_logo_count __read_mostly = -1;
+
+static inline unsigned int safe_shift(unsigned int d, int n)
+{
+ return n < 0 ? d >> -n : d << n;
+}
+
+static void fb_set_logocmap(struct fb_info *info,
+ const struct linux_logo *logo)
+{
+ struct fb_cmap palette_cmap;
+ u16 palette_green[16];
+ u16 palette_blue[16];
+ u16 palette_red[16];
+ int i, j, n;
+ const unsigned char *clut = logo->clut;
+
+ palette_cmap.start = 0;
+ palette_cmap.len = 16;
+ palette_cmap.red = palette_red;
+ palette_cmap.green = palette_green;
+ palette_cmap.blue = palette_blue;
+ palette_cmap.transp = NULL;
+
+ for (i = 0; i < logo->clutsize; i += n) {
+ n = logo->clutsize - i;
+ /* palette_cmap provides space for only 16 colors at once */
+ if (n > 16)
+ n = 16;
+ palette_cmap.start = 32 + i;
+ palette_cmap.len = n;
+ for (j = 0; j < n; ++j) {
+ palette_cmap.red[j] = clut[0] << 8 | clut[0];
+ palette_cmap.green[j] = clut[1] << 8 | clut[1];
+ palette_cmap.blue[j] = clut[2] << 8 | clut[2];
+ clut += 3;
+ }
+ fb_set_cmap(_cmap, info);
+ }
+}
+
+static void fb_set_logo_truepalette(struct fb_info *info,
+ const struct linux_logo *logo,
+ u32 *palette)
+{
+ static const unsigned char mask[] = {
+ 0, 0x80, 0xc0, 0xe0, 0xf0, 0xf8, 0xfc, 0xfe, 0xff
+ };
+ unsigned char redmask, greenmask, bluemask;
+ int redshift, greenshift, blueshift;
+ int i;
+ const unsigned char *clut = logo->clut;
+
+ /*
+* We have to create a temporary palette since console palette is only
+* 16 colors long.
+*/
+ /* Bug: Doesn't obey msb_right ... (who needs that?) */
+ redmask = mask[info->var.red.length < 8 ? info->var.red.length :
8];
+ greenmask = mask[info->var.green.length < 8 ? info->var.green.length :
8];
+ bluemask = mask[info->var.blue.length < 8 ? info->var.blue.length :
8];
+ redshift = info->var.red.offset - (8 - info->var.red.length);
+ greenshift = info->var.green.offset - (8 - info->var.green.length);
+ blueshift = info->var.blue.offset - (8 - info->var.blue.length);
+
+ for (i = 0; i < logo->clutsize; i++) {
+ palette[i+32] = (safe_shift((clut[0] & redmask), redshift) |
+
[PATCH v2 1/7] fbdev/au1200fb: Do not display boot-up logo
The fbcon module takes care of displaying the logo, if any. Remove
the code form au1200fb. If we want to display the logo without fbcon,
we should implement this in the fbdev core code.
Signed-off-by: Thomas Zimmermann
Acked-by: Javier Martinez Canillas
---
drivers/video/fbdev/au1200fb.c | 9 -
1 file changed, 9 deletions(-)
diff --git a/drivers/video/fbdev/au1200fb.c b/drivers/video/fbdev/au1200fb.c
index c137d6afe484..98afd385c49c 100644
--- a/drivers/video/fbdev/au1200fb.c
+++ b/drivers/video/fbdev/au1200fb.c
@@ -1719,15 +1719,6 @@ static int au1200fb_drv_probe(struct platform_device
*dev)
}
au1200fb_fb_set_par(fbi);
-
-#if !defined(CONFIG_FRAMEBUFFER_CONSOLE) && defined(CONFIG_LOGO)
- if (plane == 0)
- if (fb_prepare_logo(fbi, FB_ROTATE_UR)) {
- /* Start display and show logo on boot */
- fb_set_cmap(>cmap, fbi);
- fb_show_logo(fbi, FB_ROTATE_UR);
- }
-#endif
}
/* Now hook interrupt too */
--
2.42.0
[PATCH v2 2/7] fbdev/mmp/mmpfb: Do not display boot-up logo
The fbcon module takes care of displaying the logo, if any. Remove
the code form mmpfb. It is probably no tworking as expected, as it
interferes with the framebuffer console. If we want to display the
logo without fbcon, we should implement this in the fbdev core code.
v2:
* add a note on fbcon interference (Javier)
Signed-off-by: Thomas Zimmermann
Acked-by: Javier Martinez Canillas
---
drivers/video/fbdev/mmp/fb/mmpfb.c | 7 ---
1 file changed, 7 deletions(-)
diff --git a/drivers/video/fbdev/mmp/fb/mmpfb.c
b/drivers/video/fbdev/mmp/fb/mmpfb.c
index 42a87474bcea..2d9797c6fb3e 100644
--- a/drivers/video/fbdev/mmp/fb/mmpfb.c
+++ b/drivers/video/fbdev/mmp/fb/mmpfb.c
@@ -628,13 +628,6 @@ static int mmpfb_probe(struct platform_device *pdev)
dev_info(fbi->dev, "loaded to /dev/fb%d <%s>.\n",
info->node, info->fix.id);
-#ifdef CONFIG_LOGO
- if (fbi->fb_start) {
- fb_prepare_logo(info, 0);
- fb_show_logo(info, 0);
- }
-#endif
-
return 0;
failed_clear_info:
--
2.42.0
[PATCH v2 0/7] fbdev: Split off code for boot-up logo
The boot-up logo is a feature of the fbcon console; with only a few external callers. Move it from the core fbdev code into its own file. Patches 1 and 2 remove the logo setup from fbdev drivers. The logo requires a configured output, which is provided by the framebuffer console. Drivers should not implement their own logo. Patches 3 to 6 move the code for the boot-up logo into its own file and add a number of simple cleanups. It's now separate from the core fbdev code that maintains the display framebuffers. Patch 7 then removes a number of unecessary include statements from fbmem.c. v2: * unexport helpers in a separate patch * squash patches that set up fb_logo.c (Javier) Thomas Zimmermann (7): fbdev/au1200fb: Do not display boot-up logo fbdev/mmp/mmpfb: Do not display boot-up logo fbdev/core: Fix style of code for boot-up logo fbdev/core: Unexport logo helpers fbdev/core: Move logo functions into separate source file fbdev/core: Remove empty internal helpers from fb_logo.c fbdev/core: Clean up include statements in fbmem.c drivers/video/fbdev/au1200fb.c | 9 - drivers/video/fbdev/core/Makefile | 2 + drivers/video/fbdev/core/fb_internal.h | 17 + drivers/video/fbdev/core/fb_logo.c | 508 +++ drivers/video/fbdev/core/fbcon.c | 2 + drivers/video/fbdev/core/fbmem.c | 542 + drivers/video/fbdev/mmp/fb/mmpfb.c | 7 - include/linux/fb.h | 5 - 8 files changed, 530 insertions(+), 562 deletions(-) create mode 100644 drivers/video/fbdev/core/fb_logo.c -- 2.42.0
Re: [PATCH drm-misc-next v2 2/7] drm/gpuvm: rename struct drm_gpuva_manager to struct drm_gpuvm
Hi Danilo,
kernel test robot noticed the following build warnings:
[auto build test WARNING on 6bd3d8da51ca1ec97c724016466606aec7739b9f]
url:
https://github.com/intel-lab-lkp/linux/commits/Danilo-Krummrich/drm-gpuva_mgr-allow-building-as-module/20230907-054931
base: 6bd3d8da51ca1ec97c724016466606aec7739b9f
patch link:https://lore.kernel.org/r/20230906214723.4393-3-dakr%40redhat.com
patch subject: [PATCH drm-misc-next v2 2/7] drm/gpuvm: rename struct
drm_gpuva_manager to struct drm_gpuvm
reproduce:
(https://download.01.org/0day-ci/archive/20230907/202309071613.s6ztmeyu-...@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot
| Closes:
https://lore.kernel.org/oe-kbuild-all/202309071613.s6ztmeyu-...@intel.com/
All warnings (new ones prefixed by >>):
>> ./include/drm/drm_gpuva_mgr.h:138: warning: Function parameter or member
>> 'vm' not described in 'drm_gpuva'
vim +138 ./include/drm/drm_gpuva_mgr.h
e6303f323b1ad9 Danilo Krummrich 2023-07-20 60
e6303f323b1ad9 Danilo Krummrich 2023-07-20 61 /**
e6303f323b1ad9 Danilo Krummrich 2023-07-20 62 * struct drm_gpuva -
structure to track a GPU VA mapping
e6303f323b1ad9 Danilo Krummrich 2023-07-20 63 *
e6303f323b1ad9 Danilo Krummrich 2023-07-20 64 * This structure represents a
GPU VA mapping and is associated with a
3142f8b7e68331 Danilo Krummrich 2023-09-06 65 * _gpuvm.
e6303f323b1ad9 Danilo Krummrich 2023-07-20 66 *
e6303f323b1ad9 Danilo Krummrich 2023-07-20 67 * Typically, this structure
is embedded in bigger driver structures.
e6303f323b1ad9 Danilo Krummrich 2023-07-20 68 */
e6303f323b1ad9 Danilo Krummrich 2023-07-20 69 struct drm_gpuva {
e6303f323b1ad9 Danilo Krummrich 2023-07-20 70 /**
3142f8b7e68331 Danilo Krummrich 2023-09-06 71 * @gpuvm: the
_gpuvm this object is associated with
e6303f323b1ad9 Danilo Krummrich 2023-07-20 72 */
3142f8b7e68331 Danilo Krummrich 2023-09-06 73 struct drm_gpuvm *vm;
e6303f323b1ad9 Danilo Krummrich 2023-07-20 74
e6303f323b1ad9 Danilo Krummrich 2023-07-20 75 /**
e6303f323b1ad9 Danilo Krummrich 2023-07-20 76 * @flags: the
_gpuva_flags for this mapping
e6303f323b1ad9 Danilo Krummrich 2023-07-20 77 */
e6303f323b1ad9 Danilo Krummrich 2023-07-20 78 enum drm_gpuva_flags
flags;
e6303f323b1ad9 Danilo Krummrich 2023-07-20 79
e6303f323b1ad9 Danilo Krummrich 2023-07-20 80 /**
e6303f323b1ad9 Danilo Krummrich 2023-07-20 81 * @va: structure
containing the address and range of the _gpuva
e6303f323b1ad9 Danilo Krummrich 2023-07-20 82 */
e6303f323b1ad9 Danilo Krummrich 2023-07-20 83 struct {
e6303f323b1ad9 Danilo Krummrich 2023-07-20 84 /**
e6303f323b1ad9 Danilo Krummrich 2023-07-20 85 * @addr: the
start address
e6303f323b1ad9 Danilo Krummrich 2023-07-20 86 */
e6303f323b1ad9 Danilo Krummrich 2023-07-20 87 u64 addr;
e6303f323b1ad9 Danilo Krummrich 2023-07-20 88
e6303f323b1ad9 Danilo Krummrich 2023-07-20 89 /*
e6303f323b1ad9 Danilo Krummrich 2023-07-20 90 * @range: the
range
e6303f323b1ad9 Danilo Krummrich 2023-07-20 91 */
e6303f323b1ad9 Danilo Krummrich 2023-07-20 92 u64 range;
e6303f323b1ad9 Danilo Krummrich 2023-07-20 93 } va;
e6303f323b1ad9 Danilo Krummrich 2023-07-20 94
e6303f323b1ad9 Danilo Krummrich 2023-07-20 95 /**
e6303f323b1ad9 Danilo Krummrich 2023-07-20 96 * @gem: structure
containing the _gem_object and it's offset
e6303f323b1ad9 Danilo Krummrich 2023-07-20 97 */
e6303f323b1ad9 Danilo Krummrich 2023-07-20 98 struct {
e6303f323b1ad9 Danilo Krummrich 2023-07-20 99 /**
e6303f323b1ad9 Danilo Krummrich 2023-07-20 100 * @offset: the
offset within the _gem_object
e6303f323b1ad9 Danilo Krummrich 2023-07-20 101 */
e6303f323b1ad9 Danilo Krummrich 2023-07-20 102 u64 offset;
e6303f323b1ad9 Danilo Krummrich 2023-07-20 103
e6303f323b1ad9 Danilo Krummrich 2023-07-20 104 /**
e6303f323b1ad9 Danilo Krummrich 2023-07-20 105 * @obj: the
mapped _gem_object
e6303f323b1ad9 Danilo Krummrich 2023-07-20 106 */
e6303f323b1ad9 Danilo Krummrich 2023-07-20 107 struct
drm_gem_object *obj;
e6303f323b1ad9 Danilo Krummrich 2023-07-20 108
e6303f323b1ad9 Danilo Krummrich 2023-07-20 109 /**
e6303f323b1ad9 Danilo Krummrich 2023-07-20 110 * @entry: the
_head to attach this object to a _gem_object
e6303f323b1ad9 Danilo Krummrich 2023-07-20 111 */
e6303f323b1ad9 Danilo Krummrich 2023-07-20 112 struc
Re: [PATCH] drm/tegra: Remove existing framebuffer only if we support display
Thomas Zimmermann writes: [...] >> That's a good point. However I recall from earlier attempts at doing >> something like this in Nouveau (although this is now very long ago) that >> it's not very easy. The problem, as I recall, is that the driver is a >> singleton, so we would essentially be supporting either modesetting or >> not, for any device in the system. > > Take a look at struct drm_device.driver_features. It let's you clear the > flags that your device doesn't support. > > https://elixir.bootlin.com/linux/v6.5/source/include/drm/drm_device.h#L128 > That sounds indeed as the best approach and I see that at least i915 does it: https://elixir.bootlin.com/linux/v6.5/source/drivers/gpu/drm/i915/intel_device_info.c#L418 > Best regards > Thomas > -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH drm-misc-next v2 5/7] drm/gpuvm: add an abstraction for a VM / BO combination
On Wed, 6 Sep 2023 23:47:13 +0200
Danilo Krummrich wrote:
> +void drm_gpuvm_bo_destroy(struct kref *kref);
I usually keep kref's release functions private so people are not
tempted to use them.
> +
> +/**
> + * drm_gpuvm_bo_get() - acquire a struct drm_gpuvm_bo reference
> + * @vm_bo: the _gpuvm_bo to acquire the reference of
> + *
> + * This function acquires an additional reference to @vm_bo. It is illegal to
> + * call this without already holding a reference. No locks required.
> + */
> +static inline struct drm_gpuvm_bo *
> +drm_gpuvm_bo_get(struct drm_gpuvm_bo *vm_bo)
> +{
> + kref_get(_bo->kref);
> + return vm_bo;
> +}
> +
> +/**
> + * drm_gpuvm_bo_put() - drop a struct drm_gpuvm_bo reference
> + * @vm_bo: the _gpuvm_bo to release the reference of
> + *
> + * This releases a reference to @vm_bo.
> + */
> +static inline void
> +drm_gpuvm_bo_put(struct drm_gpuvm_bo *vm_bo)
> +{
> + kref_put(_bo->kref, drm_gpuvm_bo_destroy);
nit: can we have
if (vm_bo)
kref_put(_bo->kref, drm_gpuvm_bo_destroy);
so we don't have to bother testing the vm_bo value in the error/cleanup
paths?
> +}
> +
Re: [PATCH] drm/tegra: Remove existing framebuffer only if we support display
Hi
Am 07.09.23 um 10:03 schrieb Thierry Reding:
On Thu, Aug 31, 2023 at 10:04:29AM +0200, Daniel Vetter wrote:
On Thu, 31 Aug 2023 at 08:33, Mikko Perttunen wrote:
On 8/30/23 13:19, Thomas Zimmermann wrote:
Hi
Am 25.08.23 um 15:22 schrieb Thierry Reding:
From: Thierry Reding
Tegra DRM doesn't support display on Tegra234 and later, so make sure
not to remove any existing framebuffers in that case.
Signed-off-by: Thierry Reding
---
drivers/gpu/drm/tegra/drm.c | 8 +---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
index b1e1a78e30c6..7a38dadbc264 100644
--- a/drivers/gpu/drm/tegra/drm.c
+++ b/drivers/gpu/drm/tegra/drm.c
@@ -1220,9 +1220,11 @@ static int host1x_drm_probe(struct
host1x_device *dev)
drm_mode_config_reset(drm);
-err = drm_aperture_remove_framebuffers(_drm_driver);
-if (err < 0)
-goto hub;
+if (drm->mode_config.num_crtc > 0) {
If you don't support the hardware, wouldn't it be better to return
-ENODEV if !num_crtc?
While display is not supported through TegraDRM on Tegra234+, certain
multimedia accelerators are supported, so we need to finish probe for those.
Ideally you also register the tegra driver without DRIVER_MODESET |
DRIVER_ATOMIC in that case, to avoid unecessary userspace confusion.
Most userspace can cope with a display driver without any crtc, but I
think xorg-modesettting actually falls over. Or at least I've seen
some hacks that the agx people added to make sure X doesn't
accidentally open the wrong driver.
That's a good point. However I recall from earlier attempts at doing
something like this in Nouveau (although this is now very long ago) that
it's not very easy. The problem, as I recall, is that the driver is a
singleton, so we would essentially be supporting either modesetting or
not, for any device in the system.
Take a look at struct drm_device.driver_features. It let's you clear the
flags that your device doesn't support.
https://elixir.bootlin.com/linux/v6.5/source/include/drm/drm_device.h#L128
Best regards
Thomas
Now, it's unlikely that we'd have a mix of one Tegra DRM driver with
display support and another without, but it's something that I recall
back at the time with Nouveau was problematic because you could have the
Tegra integrated graphics (without display support) and a PCI-connected
discrete GPU (with display support) within the same system.
I need to look into it a bit more to see if I can come up with something
good to account for this.
Thierry
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)
OpenPGP_signature
Description: OpenPGP digital signature
Re: [PATCH 0/5] drm/bridge: samsung-dsim: fix various modes with ADV7535 bridge
Hi Michael, On 06.09.23 11:56, Michael Tretter wrote: > Hi Frieder, > > On Wed, 06 Sep 2023 11:31:45 +0200, Frieder Schrempf wrote: >> On 04.09.23 16:02, Frieder Schrempf wrote: >>> On 28.08.23 17:59, Michael Tretter wrote: I tested the i.MX8M Nano EVK with the NXP supplied MIPI-DSI adapter, which uses an ADV7535 MIPI-DSI to HDMI converter. I found that a few modes were working, but in many modes my monitor stayed dark. This series fixes the Samsung DSIM bridge driver to bring up a few more modes: The driver read the rate of the PLL ref clock only during probe. However, if the clock is re-parented to the VIDEO_PLL, changes to the pixel clock have an effect on the PLL ref clock. Therefore, the driver must read and potentially update the PLL ref clock on every modeset. I also found that the rounding mode of the porches and active area has an effect on the working modes. If the driver rounds up instead of rounding down and be calculates them in Hz instead of kHz, more modes start to work. The following table shows the modes that were working in my test without this patch set and the modes that are working now: |Mode | Before | Now | | 1920x1080-60.00 | X | X | | 1920x1080-59.94 || X | | 1920x1080-50.00 || X | | 1920x1080-30.00 || X | | 1920x1080-29.97 || X | | 1920x1080-25.00 || X | | 1920x1080-24.00 || | | 1920x1080-23.98 || | | 1680x1050-59.88 || X | | 1280x1024-75.03 | X | X | | 1280x1024-60.02 | X | X | | 1200x960-59.99 || X | | 1152x864-75.00 | X | X | | 1280x720-60.00 || | | 1280x720-59.94 || | | 1280x720-50.00 || X | | 1024x768-75.03 || X | | 1024x768-60.00 || X | | 800x600-75.00 | X | X | | 800x600-60.32 | X | X | | 720x576-50.00 | X | X | | 720x480-60.00 || | | 720x480-59.94 | X | | | 640x480-75.00 | X | X | | 640x480-60.00 || X | | 640x480-59.94 || X | | 720x400-70.08 || | Interestingly, the 720x480-59.94 mode stopped working. However, I am able to bring up the 720x480 modes by manually hacking the active area (hsa) to 40 and carefully adjusting the clocks, but something still seems to be off. Unfortunately, a few more modes are still not working at all. The NXP downstream kernel has some quirks to handle some of the modes especially wrt. to the porches, but I cannot figure out, what the driver should actually do in these cases. Maybe there is still an error in the calculation of the porches and someone at NXP can chime in. >>> >>> Thanks for working on this! We tested these patches with our Kontron BL >>> i.MX8MM board and a "10.1inch HDMI LCD (E)" display from Waveshare [1]. >>> >>> Without this series we don't get an image with the default mode of the >>> display (1024x600). With this series applied, it's now working. >> >> Minor correction: The display does work, but there is some flickering >> and occasional black screens if you let it run for some time. So there >> is still some sync issue. > > What is the parent of the dsi_phy_ref clock in your test case? It's the IMX8MM_CLK_24M default from imx8mm.dtsi. > Make sure that > the lcdif_pixel and dsi_phy_ref clocks are driven by the same PLL. > > I saw occasional black screens, if the lcdif_pixel and dsi_phy_ref clock had > different parents, and fixed it by changing the assigned-parent of > IMX8MN_CLK_DSI_PHY_REF to IMX8MN_VIDEO_PLL1_OUT in the device tree. If the > clocks are not in sync, it seems that the ADV3575 has issues correctly > reconstructing the pixel clock and HDMI monitors loose sync. > > This is something that must be configured in the board device tree, though. Ok, I tried to set the parent of IMX8MM_CLK_DSI_PHY_REF to IMX8MM_VIDEO_PLL1_OUT. I also noticed, that I forgot to remove the samsung,pll-clock-frequency from the mipi_dsi node in imx8mm.dtsi. This is necessary to make use of your PLL input clock improvements. Otherwise I get 23.76 MHz for the DSIM PLL input (derived from IMX8MM_VIDEO_PLL1_OUT) which results in a HS clock of 300.96 MHz while the display requests 301.5 MHz (50.25 MHz pixel clock). This is too low and the display doesn't work at all. Unfortunately all of this doesn't result in a more stable image on the 10" Waveshare display. It seems like the display turns black whenever the Qt app does a lot of framebuffer updates, e. g. during animations, etc. Anyway, thanks for the help! Best regards Frieder
Re: [PATCH drm-misc-next v2 5/7] drm/gpuvm: add an abstraction for a VM / BO combination
On Wed, 6 Sep 2023 23:47:13 +0200
Danilo Krummrich wrote:
> @@ -812,15 +967,20 @@ EXPORT_SYMBOL_GPL(drm_gpuva_remove);
> /**
> * drm_gpuva_link() - link a _gpuva
> * @va: the _gpuva to link
> + * @vm_bo: the _gpuvm_bo to add the _gpuva to
> *
> - * This adds the given to the GPU VA list of the _gem_object it is
> - * associated with.
> + * This adds the given to the GPU VA list of the _gpuvm_bo and the
> + * _gpuvm_bo to the _gem_object it is associated with.
> + *
> + * For every _gpuva entry added to the _gpuvm_bo an additional
> + * reference of the latter is taken.
> *
> * This function expects the caller to protect the GEM's GPUVA list against
> - * concurrent access using the GEMs dma_resv lock.
> + * concurrent access using either the GEMs dma_resv lock or a driver specific
> + * lock set through drm_gem_gpuva_set_lock().
> */
> void
> -drm_gpuva_link(struct drm_gpuva *va)
> +drm_gpuva_link(struct drm_gpuva *va, struct drm_gpuvm_bo *vm_bo)
> {
> struct drm_gem_object *obj = va->gem.obj;
>
> @@ -829,7 +989,10 @@ drm_gpuva_link(struct drm_gpuva *va)
>
> drm_gem_gpuva_assert_lock_held(obj);
>
> - list_add_tail(>gem.entry, >gpuva.list);
> + drm_gpuvm_bo_get(vm_bo);
Guess we should WARN if vm_obj->obj == obj, at least.
> + list_add_tail(>gem.entry, _bo->list.gpuva);
> + if (list_empty(_bo->list.entry.gem))
> + list_add_tail(_bo->list.entry.gem, >gpuva.list);
> }
> EXPORT_SYMBOL_GPL(drm_gpuva_link);
>
> @@ -840,20 +1003,40 @@ EXPORT_SYMBOL_GPL(drm_gpuva_link);
> * This removes the given from the GPU VA list of the _gem_object it
> is
> * associated with.
> *
> + * This removes the given from the GPU VA list of the _gpuvm_bo and
> + * the _gpuvm_bo from the _gem_object it is associated with in case
> + * this call unlinks the last _gpuva from the _gpuvm_bo.
> + *
> + * For every _gpuva entry removed from the _gpuvm_bo a reference of
> + * the latter is dropped.
> + *
> * This function expects the caller to protect the GEM's GPUVA list against
> - * concurrent access using the GEMs dma_resv lock.
> + * concurrent access using either the GEMs dma_resv lock or a driver specific
> + * lock set through drm_gem_gpuva_set_lock().
> */
> void
> drm_gpuva_unlink(struct drm_gpuva *va)
> {
> struct drm_gem_object *obj = va->gem.obj;
> + struct drm_gpuvm_bo *vm_bo;
>
> if (unlikely(!obj))
> return;
>
> drm_gem_gpuva_assert_lock_held(obj);
>
> + vm_bo = __drm_gpuvm_bo_find(va->vm, obj);
Could we add a drm_gpuva::vm_bo field so we don't have to search the
vm_bo here, and maybe drop the drm_gpuva::vm and drm_gpuva::obj fields,
since drm_gpuvm_bo contains both the vm and the GEM object. I know that
means adding an extra indirection + allocation for drivers that don't
want to use drm_gpuva_[un]link(), but I wonder if it's not preferable
over having the information duplicated (with potential mismatch)
> + if (WARN(!vm_bo, "GPUVA doesn't seem to be linked.\n"))
> + return;
> +
> list_del_init(>gem.entry);
> +
> + /* This is the last mapping being unlinked for this GEM object, hence
> + * also remove the VM_BO from the GEM's gpuva list.
> + */
> + if (list_empty(_bo->list.gpuva))
> + list_del_init(_bo->list.entry.gem);
> + drm_gpuvm_bo_put(vm_bo);
> }
> EXPORT_SYMBOL_GPL(drm_gpuva_unlink);
Re: [PATCH] drm/tegra: Remove existing framebuffer only if we support display
Thierry Reding writes: Hello Thierry, > On Wed, Aug 30, 2023 at 08:13:04AM +0200, Javier Martinez Canillas wrote: [...] >> I also wonder if is worth to move the drm_num_crtcs() function from >> drivers/gpu/drm/drm_crtc.c to include/drm/drm_crtc.h and use that helper >> instead? > > I've been looking at this, there's a few things that come to mind. It > seems like we have a couple of different ways to get the number of CRTCs > for a device. We have struct drm_device's num_crtcs, which is set during > drm_vblank_init(), then we have struct drm_mode_config's num_crtc, which > is incremented every time a new CRTC is added (and decremented when a > CRTC is removed), and finally we've got the drm_num_crtcs() which > "computes" the number of CRTCs registered by iterating over all CRTCs > that have been registered. > > Are there any cases where these three can yield different values? Would > it not make sense to consolidate these into a single variable? > I als was confused by that when looked at the implementation of the mentioned helpers and couldn't find a reason why there are different ways to calculate the number of CRTCs. Maybe Sima or someone else can shed some light? > Thierry -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH 5/7] fbdev/core: Build fb_logo iff CONFIG_LOGO has been selected
Am 06.09.23 um 12:12 schrieb Javier Martinez Canillas: Thomas Zimmermann writes: Only build fb_logo.c if CONFIG_LOGO has been selected. Otherwise provide empty implementations of the contained interfaces and avoid using the exported variables. Signed-off-by: Thomas Zimmermann Ah! You are doing in this patch exactly what I mentioned in my previous email :) I would just squash this patch with #4, but up to you. I'll refactor these patches slightly: The unexporting of the helpers goes into a new patch and the rest of patches 4 and 5 will be squashed. Acked-by: Javier Martinez Canillas -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Frankenstrasse 146, 90461 Nuernberg, Germany GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman HRB 36809 (AG Nuernberg) OpenPGP_signature Description: OpenPGP digital signature
Re: [PATCH 5/7] fbdev/core: Build fb_logo iff CONFIG_LOGO has been selected
Am 04.09.23 um 09:08 schrieb Thomas Zimmermann:
Hi
Am 01.09.23 um 10:22 schrieb Helge Deller:
On 8/29/23 16:15, Thomas Zimmermann wrote:
Only build fb_logo.c if CONFIG_LOGO has been selected. Otherwise
provide empty implementations of the contained interfaces and avoid
using the exported variables.
Signed-off-by: Thomas Zimmermann
...
diff --git a/drivers/video/fbdev/core/fbcon.c
b/drivers/video/fbdev/core/fbcon.c
index f157a5a1dffc..24b038510a71 100644
--- a/drivers/video/fbdev/core/fbcon.c
+++ b/drivers/video/fbdev/core/fbcon.c
@@ -474,15 +474,19 @@ static int __init fb_console_setup(char *this_opt)
if (!strncmp(options, "logo-pos:", 9)) {
options += 9;
+#ifdef CONFIG_LOGO
if (!strcmp(options, "center"))
fb_center_logo = true;
+#endif
IMHO, *sometimes* it makes sense to not use #ifdef and code it instead
like this:
if (IS_ENABLED(CONFIG_LOGO) &&
!strcmp(options, "center"))
...
That way the compiler will optimize that code away as well, but in
addition it will compile-check that you have correct coding independend
if CONFIG_LOGO is set or not.
Good idea. I'll change it. The IS_ENABLED code is also easier to read IMHO.
I'll keep the current approach, but in a simplified form.
Best regards
Thomas
continue;
}
if (!strncmp(options, "logo-count:", 11)) {
options += 11;
+#ifdef CONFIG_LOGO
if (*options)
fb_logo_count = simple_strtol(options, , 0);
+#endif
same here.
Helge
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)
OpenPGP_signature
Description: OpenPGP digital signature
Re: [PATCH] drm/tegra: Remove existing framebuffer only if we support display
On Thu, Aug 31, 2023 at 10:04:29AM +0200, Daniel Vetter wrote:
> On Thu, 31 Aug 2023 at 08:33, Mikko Perttunen wrote:
> >
> > On 8/30/23 13:19, Thomas Zimmermann wrote:
> > > Hi
> > >
> > > Am 25.08.23 um 15:22 schrieb Thierry Reding:
> > >> From: Thierry Reding
> > >>
> > >> Tegra DRM doesn't support display on Tegra234 and later, so make sure
> > >> not to remove any existing framebuffers in that case.
> > >>
> > >> Signed-off-by: Thierry Reding
> > >> ---
> > >> drivers/gpu/drm/tegra/drm.c | 8 +---
> > >> 1 file changed, 5 insertions(+), 3 deletions(-)
> > >>
> > >> diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
> > >> index b1e1a78e30c6..7a38dadbc264 100644
> > >> --- a/drivers/gpu/drm/tegra/drm.c
> > >> +++ b/drivers/gpu/drm/tegra/drm.c
> > >> @@ -1220,9 +1220,11 @@ static int host1x_drm_probe(struct
> > >> host1x_device *dev)
> > >> drm_mode_config_reset(drm);
> > >> -err = drm_aperture_remove_framebuffers(_drm_driver);
> > >> -if (err < 0)
> > >> -goto hub;
> > >> +if (drm->mode_config.num_crtc > 0) {
> > >
> > > If you don't support the hardware, wouldn't it be better to return
> > > -ENODEV if !num_crtc?
> >
> > While display is not supported through TegraDRM on Tegra234+, certain
> > multimedia accelerators are supported, so we need to finish probe for those.
>
> Ideally you also register the tegra driver without DRIVER_MODESET |
> DRIVER_ATOMIC in that case, to avoid unecessary userspace confusion.
> Most userspace can cope with a display driver without any crtc, but I
> think xorg-modesettting actually falls over. Or at least I've seen
> some hacks that the agx people added to make sure X doesn't
> accidentally open the wrong driver.
That's a good point. However I recall from earlier attempts at doing
something like this in Nouveau (although this is now very long ago) that
it's not very easy. The problem, as I recall, is that the driver is a
singleton, so we would essentially be supporting either modesetting or
not, for any device in the system.
Now, it's unlikely that we'd have a mix of one Tegra DRM driver with
display support and another without, but it's something that I recall
back at the time with Nouveau was problematic because you could have the
Tegra integrated graphics (without display support) and a PCI-connected
discrete GPU (with display support) within the same system.
I need to look into it a bit more to see if I can come up with something
good to account for this.
Thierry
signature.asc
Description: PGP signature
Re: [PATCH] drm/tegra: Remove existing framebuffer only if we support display
On Wed, Aug 30, 2023 at 08:13:04AM +0200, Javier Martinez Canillas wrote:
> Thierry Reding writes:
>
> Hello Thierry,
>
> > From: Thierry Reding
> >
> > Tegra DRM doesn't support display on Tegra234 and later, so make sure
> > not to remove any existing framebuffers in that case.
> >
>
> I see, this makes sense to me.
>
> Acked-by: Javier Martinez Canillas
>
> A couple of comments below though:
>
> > Signed-off-by: Thierry Reding
> > ---
> > drivers/gpu/drm/tegra/drm.c | 8 +---
> > 1 file changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
> > index b1e1a78e30c6..7a38dadbc264 100644
> > --- a/drivers/gpu/drm/tegra/drm.c
> > +++ b/drivers/gpu/drm/tegra/drm.c
> > @@ -1220,9 +1220,11 @@ static int host1x_drm_probe(struct host1x_device
> > *dev)
> >
> > drm_mode_config_reset(drm);
> >
> > - err = drm_aperture_remove_framebuffers(_drm_driver);
> > - if (err < 0)
> > - goto hub;
> > + if (drm->mode_config.num_crtc > 0) {
>
> Maybe you can add a comment here explaining why the check is needed?
Sure, will do.
> I also wonder if is worth to move the drm_num_crtcs() function from
> drivers/gpu/drm/drm_crtc.c to include/drm/drm_crtc.h and use that helper
> instead?
I've been looking at this, there's a few things that come to mind. It
seems like we have a couple of different ways to get the number of CRTCs
for a device. We have struct drm_device's num_crtcs, which is set during
drm_vblank_init(), then we have struct drm_mode_config's num_crtc, which
is incremented every time a new CRTC is added (and decremented when a
CRTC is removed), and finally we've got the drm_num_crtcs() which
"computes" the number of CRTCs registered by iterating over all CRTCs
that have been registered.
Are there any cases where these three can yield different values? Would
it not make sense to consolidate these into a single variable?
Thierry
signature.asc
Description: PGP signature
Re: [PATCH v2 10/34] drm/amd/display: add plane 3D LUT driver-specific properties
On Wed, 6 Sep 2023 15:30:04 -0400
Harry Wentland wrote:
> On 2023-08-10 12:02, Melissa Wen wrote:
> > Add 3D LUT property for plane gamma correction using a 3D lookup table.
> > Since a 3D LUT has a limited number of entries in each dimension we want
> > to use them in an optimal fashion. This means using the 3D LUT in a
> > colorspace that is optimized for human vision, such as sRGB, PQ, or
> > another non-linear space. Therefore, userpace may need one 1D LUT
> > (shaper) before it to delinearize content and another 1D LUT after 3D
> > LUT (blend) to linearize content again for blending. The next patches
> > add these 1D LUTs to the plane color mgmt pipeline.
> >
> > Signed-off-by: Melissa Wen
> > ---
> > drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h | 10
> > .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 9
> > .../amd/display/amdgpu_dm/amdgpu_dm_color.c | 14 +++
> > .../amd/display/amdgpu_dm/amdgpu_dm_plane.c | 23 +++
> > 4 files changed, 56 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
> > index 66bae0eed80c..730a88236501 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
> > @@ -363,6 +363,16 @@ struct amdgpu_mode_info {
> > * @plane_hdr_mult_property:
> > */
> > struct drm_property *plane_hdr_mult_property;
> > + /**
> > +* @plane_lut3d_property: Plane property for gamma correction using a
> > +* 3D LUT (pre-blending).
> > +*/
>
> I think we'll want to describe how the 3DLUT entries are laid out.
> Something that describes how userspace should fill it, like
> gamescope does for example:
> https://github.com/ValveSoftware/gamescope/blob/7108880ed80b68c21750369e2ac9b7315fecf264/src/color_helpers.cpp#L302
>
> Something like: a three-dimensional array, with each dimension
> having a size of the cubed root of lut3d_size, blue being the
> outermost dimension, red the innermost.
>
Here is an example of how we defined a 3D LUT layout in Weston:
https://gitlab.freedesktop.org/wayland/weston/-/blob/68e2a606c056c8453c770263f41f34cd68bdc9d0/libweston/color.h#L114-152
I think that is the most clear definition it can be, without needing to
understand specific terminology.
Thanks,
pq
>
> > + struct drm_property *plane_lut3d_property;
> > + /**
> > +* @plane_degamma_lut_size_property: Plane property to define the max
> > +* size of 3D LUT as supported by the driver (read-only).
> > +*/
>
> We should probably document that the size of the 3DLUT should
> be the size of one dimension cubed, or that the cubed root of
> the LUT size gives the size per dimension.
>
> Harry
pgpDWoXYEGZ85.pgp
Description: OpenPGP digital signature
Re: [PATCH drm-misc-next v2 2/7] drm/gpuvm: rename struct drm_gpuva_manager to struct drm_gpuvm
On Wed, 6 Sep 2023 23:47:10 +0200 Danilo Krummrich wrote: > Rename struct drm_gpuva_manager to struct drm_gpuvm including > corresponding functions. This way the GPUVA manager's structures align > very well with the documentation of VM_BIND [1] and VM_BIND locking [2]. > > It also provides a better foundation for the naming of data structures > and functions introduced for implementing a common dma-resv per GPU-VM > including tracking of external and evicted objects in subsequent > patches. I'm fine with this rename, but I feel like some bits are missing in this patch. I see a few functions taking a drm_gpuvm object and still being prefixed with drm_gpuva_ (I'm not talking about functions that only manipulate a drm_gpuva object, but those updating the the VM state, like the sm_map/unmap helpers), and I think it'd be preferable to rename the source files and the Kconfig option too.
Re: [PATCH v2 07/34] drm/amd/display: explicitly define EOTF and inverse EOTF
On Wed, 6 Sep 2023 16:15:10 -0400
Harry Wentland wrote:
> On 2023-08-25 10:18, Melissa Wen wrote:
> > On 08/22, Pekka Paalanen wrote:
> >> On Thu, 10 Aug 2023 15:02:47 -0100
> >> Melissa Wen wrote:
> >>
> >>> Instead of relying on color block names to get the transfer function
> >>> intention regarding encoding pixel's luminance, define supported
> >>> Electro-Optical Transfer Functions (EOTFs) and inverse EOTFs, that
> >>> includes pure gamma or standardized transfer functions.
> >>>
> >>> Suggested-by: Harry Wentland
> >>> Signed-off-by: Melissa Wen
> >>> ---
> >>> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 19 +++--
> >>> .../amd/display/amdgpu_dm/amdgpu_dm_color.c | 69 +++
> >>> 2 files changed, 67 insertions(+), 21 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> >>> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> >>> index c749c9cb3d94..f6251ed89684 100644
> >>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> >>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> >>> @@ -718,14 +718,21 @@ extern const struct amdgpu_ip_block_version
> >>> dm_ip_block;
> >>>
> >>> enum amdgpu_transfer_function {
> >>> AMDGPU_TRANSFER_FUNCTION_DEFAULT,
> >>> - AMDGPU_TRANSFER_FUNCTION_SRGB,
> >>> - AMDGPU_TRANSFER_FUNCTION_BT709,
> >>> - AMDGPU_TRANSFER_FUNCTION_PQ,
> >>> + AMDGPU_TRANSFER_FUNCTION_SRGB_EOTF,
> >>> + AMDGPU_TRANSFER_FUNCTION_BT709_EOTF,
> >>> + AMDGPU_TRANSFER_FUNCTION_PQ_EOTF,
> >>> AMDGPU_TRANSFER_FUNCTION_LINEAR,
> >>> AMDGPU_TRANSFER_FUNCTION_UNITY,
> >>> - AMDGPU_TRANSFER_FUNCTION_GAMMA22,
> >>> - AMDGPU_TRANSFER_FUNCTION_GAMMA24,
> >>> - AMDGPU_TRANSFER_FUNCTION_GAMMA26,
> >>> + AMDGPU_TRANSFER_FUNCTION_GAMMA22_EOTF,
> >>> + AMDGPU_TRANSFER_FUNCTION_GAMMA24_EOTF,
> >>> + AMDGPU_TRANSFER_FUNCTION_GAMMA26_EOTF,
> >>> + AMDGPU_TRANSFER_FUNCTION_SRGB_INV_EOTF,
> >>> + AMDGPU_TRANSFER_FUNCTION_BT709_INV_EOTF,
> >>> + AMDGPU_TRANSFER_FUNCTION_PQ_INV_EOTF,
> >>> + AMDGPU_TRANSFER_FUNCTION_GAMMA22_INV_EOTF,
> >>> + AMDGPU_TRANSFER_FUNCTION_GAMMA24_INV_EOTF,
> >>> + AMDGPU_TRANSFER_FUNCTION_GAMMA26_INV_EOTF,
> >>> +AMDGPU_TRANSFER_FUNCTION_COUNT
> >>> };
> >>>
> >>> struct dm_plane_state {
> >>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
> >>> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
> >>> index 56ce008b9095..cc2187c0879a 100644
> >>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
> >>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
> >>> @@ -85,18 +85,59 @@ void amdgpu_dm_init_color_mod(void)
> >>> }
> >>>
> >>> #ifdef AMD_PRIVATE_COLOR
> >>> -static const struct drm_prop_enum_list
> >>> amdgpu_transfer_function_enum_list[] = {
> >>> - { AMDGPU_TRANSFER_FUNCTION_DEFAULT, "Default" },
> >>> - { AMDGPU_TRANSFER_FUNCTION_SRGB, "sRGB" },
> >>> - { AMDGPU_TRANSFER_FUNCTION_BT709, "BT.709" },
> >>> - { AMDGPU_TRANSFER_FUNCTION_PQ, "PQ (Perceptual Quantizer)" },
> >>> - { AMDGPU_TRANSFER_FUNCTION_LINEAR, "Linear" },
> >>> - { AMDGPU_TRANSFER_FUNCTION_UNITY, "Unity" },
> >>> - { AMDGPU_TRANSFER_FUNCTION_GAMMA22, "Gamma 2.2" },
> >>> - { AMDGPU_TRANSFER_FUNCTION_GAMMA24, "Gamma 2.4" },
> >>> - { AMDGPU_TRANSFER_FUNCTION_GAMMA26, "Gamma 2.6" },
> >>> +static const char * const
> >>> +amdgpu_transfer_function_names[] = {
> >>> + [AMDGPU_TRANSFER_FUNCTION_DEFAULT] = "Default",
> >>> + [AMDGPU_TRANSFER_FUNCTION_LINEAR] = "Linear",
> >>
> >> Hi,
> >>
> >> if the below is identity, then what is linear? Is there a coefficient
> >> (multiplier) somewhere? Offset?
> >>
> >>> + [AMDGPU_TRANSFER_FUNCTION_UNITY]= "Unity",
> >>
> >> Should "Unity" be called "Identity"?
> >
> > AFAIU, AMD treats Linear and Unity as the same: Identity. So, IIUC,
> > indeed merging both as identity sounds the best approach.
>
> Agreed.
>
> >>
> >> Doesn't unity mean that the output is always 1.0 regardless of input?
> >>
> >>> + [AMDGPU_TRANSFER_FUNCTION_SRGB_EOTF]= "sRGB EOTF",
> >>> + [AMDGPU_TRANSFER_FUNCTION_BT709_EOTF] = "BT.709 EOTF",
> >>
> >> BT.709 says about "Overall opto-electronic transfer characteristics at
> >> source":
> >>
> >>In typical production practice the encoding function of image
> >>sources is adjusted so that the final picture has the desired
> >>look, as viewed on a reference monitor having the reference
> >>decoding function of Recommendation ITU-R BT.1886, in the
> >>reference viewing environment defined in Recommendation ITU-R
> >>BT.2035.
> >>
> >> IOW, typically people tweak the encoding function instead of using
> >> BT.709 OETF as is, which means that inverting the BT.709 OETF produces
> >> something slightly unknown. The note about BT.1886 means that that
> >> something is also not quite how it's supposed to be turned into light.
> >>
> >> Should this enum item be "BT.709 inverse OETF" and respectively below
[PULL] drm-misc-fixes
Hi, Here's this week drm-misc-fixes PR Maxime drm-misc-fixes-2023-09-07: One doc fix for drm/connector, one fix for amdgpu for an crash when VRAM usage is high, and one fix in gm12u320 to fix the timeout units in the code The following changes since commit f9e96bf1905479f18e83a3a4c314a8dfa56ede2c: drm/vmwgfx: Fix possible invalid drm gem put calls (2023-08-23 13:20:04 -0400) are available in the Git repository at: git://anongit.freedesktop.org/drm/drm-misc tags/drm-misc-fixes-2023-09-07 for you to fetch changes up to 7583028d359db3cd0072badcc576b4f9455fd27a: drm: gm12u320: Fix the timeout usage for usb_bulk_msg() (2023-09-04 10:00:57 +0200) One doc fix for drm/connector, one fix for amdgpu for an crash when VRAM usage is high, and one fix in gm12u320 to fix the timeout units in the code Jinjie Ruan (1): drm: gm12u320: Fix the timeout usage for usb_bulk_msg() Lee Jones (1): drm/drm_connector: Provide short description of param 'supported_colorspaces' Simon Pilkington (1): drm/amd: Make fence wait in suballocator uninterruptible drivers/gpu/drm/amd/amdgpu/amdgpu_sa.c | 2 +- drivers/gpu/drm/drm_connector.c| 2 ++ drivers/gpu/drm/tiny/gm12u320.c| 10 +- 3 files changed, 8 insertions(+), 6 deletions(-) signature.asc Description: PGP signature
[PATCH 2/2] accel/ivpu: Compile ivpu_debugfs.c conditionally
Only compile ivpu_debugfs.c file with CONFIG_DEBUG_FS.
Signed-off-by: Stanislaw Gruszka
---
drivers/accel/ivpu/Makefile | 3 ++-
drivers/accel/ivpu/ivpu_debugfs.h | 4
drivers/accel/ivpu/ivpu_drv.c | 2 --
3 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/accel/ivpu/Makefile b/drivers/accel/ivpu/Makefile
index e4328b430564..95ff7ad16338 100644
--- a/drivers/accel/ivpu/Makefile
+++ b/drivers/accel/ivpu/Makefile
@@ -2,7 +2,6 @@
# Copyright (C) 2023 Intel Corporation
intel_vpu-y := \
- ivpu_debugfs.o \
ivpu_drv.o \
ivpu_fw.o \
ivpu_fw_log.o \
@@ -16,4 +15,6 @@ intel_vpu-y := \
ivpu_mmu_context.o \
ivpu_pm.o
+intel_vpu-$(CONFIG_DEBUG_FS) += ivpu_debugfs.o
+
obj-$(CONFIG_DRM_ACCEL_IVPU) += intel_vpu.o
diff --git a/drivers/accel/ivpu/ivpu_debugfs.h
b/drivers/accel/ivpu/ivpu_debugfs.h
index 76dbce139772..49ae9ea78287 100644
--- a/drivers/accel/ivpu/ivpu_debugfs.h
+++ b/drivers/accel/ivpu/ivpu_debugfs.h
@@ -8,6 +8,10 @@
struct ivpu_device;
+#if defined(CONFIG_DEBUG_FS)
void ivpu_debugfs_init(struct ivpu_device *vdev);
+#else
+static inline void ivpu_debugfs_init(struct ivpu_device *vdev) { }
+#endif
#endif /* __IVPU_DEBUGFS_H__ */
diff --git a/drivers/accel/ivpu/ivpu_drv.c b/drivers/accel/ivpu/ivpu_drv.c
index b6aaf9811355..7851ff7773ca 100644
--- a/drivers/accel/ivpu/ivpu_drv.c
+++ b/drivers/accel/ivpu/ivpu_drv.c
@@ -627,9 +627,7 @@ static int ivpu_probe(struct pci_dev *pdev, const struct
pci_device_id *id)
if (ret)
return ret;
-#if defined(CONFIG_DEBUG_FS)
ivpu_debugfs_init(vdev);
-#endif
ret = drm_dev_register(>drm, 0);
if (ret) {
--
2.25.1
[PATCH 1/2] accel/ivpu: Update debugfs to latest changes in DRM
Use new drm debugfs helpers. This is needed after changes from
commit 78346ebf9f94 ("drm/debugfs: drop debugfs_init() for the render
and accel node v2").
Signed-off-by: Stanislaw Gruszka
---
drivers/accel/ivpu/ivpu_debugfs.c | 50 +++
drivers/accel/ivpu/ivpu_debugfs.h | 4 +--
drivers/accel/ivpu/ivpu_drv.c | 8 ++---
3 files changed, 30 insertions(+), 32 deletions(-)
diff --git a/drivers/accel/ivpu/ivpu_debugfs.c
b/drivers/accel/ivpu/ivpu_debugfs.c
index 5e5996fd4f9f..9163bc6bd3ef 100644
--- a/drivers/accel/ivpu/ivpu_debugfs.c
+++ b/drivers/accel/ivpu/ivpu_debugfs.c
@@ -17,20 +17,26 @@
#include "ivpu_jsm_msg.h"
#include "ivpu_pm.h"
+static inline struct ivpu_device *seq_to_ivpu(struct seq_file *s)
+{
+ struct drm_debugfs_entry *entry = s->private;
+
+ return to_ivpu_device(entry->dev);
+}
+
static int bo_list_show(struct seq_file *s, void *v)
{
- struct drm_info_node *node = (struct drm_info_node *)s->private;
struct drm_printer p = drm_seq_file_printer(s);
+ struct ivpu_device *vdev = seq_to_ivpu(s);
- ivpu_bo_list(node->minor->dev, );
+ ivpu_bo_list(>drm, );
return 0;
}
static int fw_name_show(struct seq_file *s, void *v)
{
- struct drm_info_node *node = (struct drm_info_node *)s->private;
- struct ivpu_device *vdev = to_ivpu_device(node->minor->dev);
+ struct ivpu_device *vdev = seq_to_ivpu(s);
seq_printf(s, "%s\n", vdev->fw->name);
return 0;
@@ -38,8 +44,7 @@ static int fw_name_show(struct seq_file *s, void *v)
static int fw_trace_capability_show(struct seq_file *s, void *v)
{
- struct drm_info_node *node = (struct drm_info_node *)s->private;
- struct ivpu_device *vdev = to_ivpu_device(node->minor->dev);
+ struct ivpu_device *vdev = seq_to_ivpu(s);
u64 trace_hw_component_mask;
u32 trace_destination_mask;
int ret;
@@ -57,8 +62,7 @@ static int fw_trace_capability_show(struct seq_file *s, void
*v)
static int fw_trace_config_show(struct seq_file *s, void *v)
{
- struct drm_info_node *node = (struct drm_info_node *)s->private;
- struct ivpu_device *vdev = to_ivpu_device(node->minor->dev);
+ struct ivpu_device *vdev = seq_to_ivpu(s);
/**
* WA: VPU_JSM_MSG_TRACE_GET_CONFIG command is not working yet,
* so we use values from vdev->fw instead of calling
ivpu_jsm_trace_get_config()
@@ -78,8 +82,7 @@ static int fw_trace_config_show(struct seq_file *s, void *v)
static int last_bootmode_show(struct seq_file *s, void *v)
{
- struct drm_info_node *node = (struct drm_info_node *)s->private;
- struct ivpu_device *vdev = to_ivpu_device(node->minor->dev);
+ struct ivpu_device *vdev = seq_to_ivpu(s);
seq_printf(s, "%s\n", (vdev->pm->is_warmboot) ? "warmboot" :
"coldboot");
@@ -88,8 +91,7 @@ static int last_bootmode_show(struct seq_file *s, void *v)
static int reset_counter_show(struct seq_file *s, void *v)
{
- struct drm_info_node *node = (struct drm_info_node *)s->private;
- struct ivpu_device *vdev = to_ivpu_device(node->minor->dev);
+ struct ivpu_device *vdev = seq_to_ivpu(s);
seq_printf(s, "%d\n", atomic_read(>pm->reset_counter));
return 0;
@@ -97,14 +99,13 @@ static int reset_counter_show(struct seq_file *s, void *v)
static int reset_pending_show(struct seq_file *s, void *v)
{
- struct drm_info_node *node = (struct drm_info_node *)s->private;
- struct ivpu_device *vdev = to_ivpu_device(node->minor->dev);
+ struct ivpu_device *vdev = seq_to_ivpu(s);
seq_printf(s, "%d\n", atomic_read(>pm->in_reset));
return 0;
}
-static const struct drm_info_list vdev_debugfs_list[] = {
+static const struct drm_debugfs_info vdev_debugfs_list[] = {
{"bo_list", bo_list_show, 0},
{"fw_name", fw_name_show, 0},
{"fw_trace_capability", fw_trace_capability_show, 0},
@@ -270,25 +271,22 @@ static const struct file_operations
ivpu_reset_engine_fops = {
.write = ivpu_reset_engine_fn,
};
-void ivpu_debugfs_init(struct drm_minor *minor)
+void ivpu_debugfs_init(struct ivpu_device *vdev)
{
- struct ivpu_device *vdev = to_ivpu_device(minor->dev);
-
- drm_debugfs_create_files(vdev_debugfs_list,
ARRAY_SIZE(vdev_debugfs_list),
-minor->debugfs_root, minor);
+ drm_debugfs_add_files(>drm, vdev_debugfs_list,
ARRAY_SIZE(vdev_debugfs_list));
- debugfs_create_file("force_recovery", 0200, minor->debugfs_root, vdev,
+ debugfs_create_file("force_recovery", 0200, vdev->drm.debugfs_root,
vdev,
_force_recovery_fops);
- debugfs_create_file("fw_log", 0644, minor->debugfs_root, vdev,
+ debugfs_create_file("fw_log", 0644, vdev->drm.debugfs_root, vdev,
_log_fops);
- debugfs_create_file("fw_trace_destination_mask", 0200,
minor->debugfs_root,
