Re: [PATCH] dma-buf: heap-helpers: Set dma-buf exporter name

2020-08-14 Thread Ezequiel Garcia
On Fri, 14 Aug 2020 at 23:20, John Stultz  wrote:
>
> On Fri, Aug 14, 2020 at 7:25 AM Ezequiel Garcia  
> wrote:
> >
> > Currently the heap helper uses DEFINE_DMA_BUF_EXPORT_INFO,
> > which uses KBUILD_MODNAME for the dma_buf_export_info.exp_name.
> >
> > This effectively makes all dma-bufs exported by the heap
> > helper as coming from "heap-helpers", instead of the actual heap name
> > (cma, system, etc).
> >
> > Fix this by adding a dma-heap name getter, and then setting
> > dma_buf_export_info.exp_name.
> >
> > Signed-off-by: Ezequiel Garcia 
>
> Untested, but looks sane to me.
>
> Acked-by: John Stultz 
>

Cool.

> On a slightly related note, I'm starting to regret the current
> heap-helpers approach (Andrew probably gets an "I told you so" there
> :). While it avoids a lot of duplication, it's really an all or
> nothing approach, and doesn't really compare well to other drm style
> helper functions.  I may eventually try to break the system and cma
> implementations out of the helper code and try to consider a different
> approach to avoid the duplication.
>

Fully agreed :-) It definitely looks too rigid right now.

Cheers,
Ezequiel
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel


[PATCH 0/2] Fixes to drm_sched_priority (v2)

2020-08-14 Thread Luben Tuikov
Some fixes to enum drm_sched_priority which I'd wanted to do
since last year.

For instance, renaming MAX to COUNT, as usually a maximum value
is a value which is part of the set of values, (e.g. a maxima of
a function), and thus assignable, whereby a count is the size of
a set (the enumeration in this case). It also makes it clearer
when used to define size of arrays.

Removing some redundant naming and perhaps better naming could be
had for PRIORITY_SW and PRIORITY_HW, maybe "moderate" and
"temperate" respectively. However, I've left them as "sw" and
"hw".

Also remove a macro which was used in only a single place.

In the second patch, remove priority INVALID, since it is not a
priority, e.g. a scheduler cannot be assigned and run at priority
"invalid". It seems to be more of a directive, a status, of user
input, and as such has no place in the enumeration of priority
levels.

Something else I'd like to do, is to eliminate priority
enumeration "UNSET", since it is not really a priority level,
but  a directive, instructing the code to "reset the priority
of a context to the initial priority".

At the moment, this functionality is overloaded to this priority
value, and it should be an IOCTL op, as opposed to a priority value.

Tested on my desktop system, which is running a kernel with
this patch applied.

v2: Some reverts; eliminate SW and HW for HIGH, and bring back
KERNEL--the highest!

Luben Tuikov (2):
  drm/scheduler: Scheduler priority fixes (v2)
  drm/scheduler: Remove priority macro INVALID (v2)

 drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c   |  9 +++--
 drivers/gpu/drm/amd/amdgpu/amdgpu_job.c   |  2 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c  |  2 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h  |  2 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c | 40 +--
 drivers/gpu/drm/amd/amdgpu/amdgpu_sched.h |  3 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c   |  2 +-
 drivers/gpu/drm/scheduler/sched_main.c|  4 +--
 include/drm/gpu_scheduler.h   | 13 
 9 files changed, 49 insertions(+), 28 deletions(-)

-- 
2.28.0.215.g878e727637

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel


[PATCH 1/2] drm/scheduler: Scheduler priority fixes (v2)

2020-08-14 Thread Luben Tuikov
Remove DRM_SCHED_PRIORITY_LOW, as it was used
in only one place.

Rename and separate by a line
DRM_SCHED_PRIORITY_MAX to DRM_SCHED_PRIORITY_COUNT
as it represents a (total) count of said
priorities and it is used as such in loops
throughout the code. (0-based indexing is the
the count number.)

Remove redundant word HIGH in priority names,
and rename *KERNEL* to *HIGH*, as it really
means that, high.

v2: Add back KERNEL and remove SW and HW,
in lieu of a single HIGH between NORMAL and KERNEL.

Signed-off-by: Luben Tuikov 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c   |  4 ++--
 drivers/gpu/drm/amd/amdgpu/amdgpu_job.c   |  2 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c  |  2 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h  |  2 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c |  6 +++---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c   |  2 +-
 drivers/gpu/drm/scheduler/sched_main.c|  4 ++--
 include/drm/gpu_scheduler.h   | 12 +++-
 8 files changed, 18 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
index d85d13f7a043..68eaa4f687a6 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
@@ -46,7 +46,7 @@ const unsigned int amdgpu_ctx_num_entities[AMDGPU_HW_IP_NUM] 
= {
 static int amdgpu_ctx_priority_permit(struct drm_file *filp,
  enum drm_sched_priority priority)
 {
-   if (priority < 0 || priority >= DRM_SCHED_PRIORITY_MAX)
+   if (priority < 0 || priority >= DRM_SCHED_PRIORITY_COUNT)
return -EINVAL;
 
/* NORMAL and below are accessible by everyone */
@@ -65,7 +65,7 @@ static int amdgpu_ctx_priority_permit(struct drm_file *filp,
 static enum gfx_pipe_priority amdgpu_ctx_sched_prio_to_compute_prio(enum 
drm_sched_priority prio)
 {
switch (prio) {
-   case DRM_SCHED_PRIORITY_HIGH_HW:
+   case DRM_SCHED_PRIORITY_HIGH:
case DRM_SCHED_PRIORITY_KERNEL:
return AMDGPU_GFX_PIPE_PRIO_HIGH;
default:
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
index 75d37dfb51aa..bb9e5481ff3c 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
@@ -251,7 +251,7 @@ void amdgpu_job_stop_all_jobs_on_sched(struct 
drm_gpu_scheduler *sched)
int i;
 
/* Signal all jobs not yet scheduled */
-   for (i = DRM_SCHED_PRIORITY_MAX - 1; i >= DRM_SCHED_PRIORITY_MIN; i--) {
+   for (i = DRM_SCHED_PRIORITY_COUNT - 1; i >= DRM_SCHED_PRIORITY_MIN; 
i--) {
struct drm_sched_rq *rq = >sched_rq[i];
 
if (!rq)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
index 13ea8ebc421c..6d4fc79bf84a 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
@@ -267,7 +267,7 @@ int amdgpu_ring_init(struct amdgpu_device *adev, struct 
amdgpu_ring *ring,
>sched;
}
 
-   for (i = 0; i < DRM_SCHED_PRIORITY_MAX; ++i)
+   for (i = DRM_SCHED_PRIORITY_MIN; i < DRM_SCHED_PRIORITY_COUNT; ++i)
atomic_set(>num_jobs[i], 0);
 
return 0;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
index da871d84b742..7112137689db 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
@@ -243,7 +243,7 @@ struct amdgpu_ring {
boolhas_compute_vm_bug;
boolno_scheduler;
 
-   atomic_tnum_jobs[DRM_SCHED_PRIORITY_MAX];
+   atomic_tnum_jobs[DRM_SCHED_PRIORITY_COUNT];
struct mutexpriority_mutex;
/* protected by priority_mutex */
int priority;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c
index c799691dfa84..17661ede9488 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c
@@ -36,14 +36,14 @@ enum drm_sched_priority amdgpu_to_sched_priority(int 
amdgpu_priority)
 {
switch (amdgpu_priority) {
case AMDGPU_CTX_PRIORITY_VERY_HIGH:
-   return DRM_SCHED_PRIORITY_HIGH_HW;
+   return DRM_SCHED_PRIORITY_HIGH;
case AMDGPU_CTX_PRIORITY_HIGH:
-   return DRM_SCHED_PRIORITY_HIGH_SW;
+   return DRM_SCHED_PRIORITY_HIGH;
case AMDGPU_CTX_PRIORITY_NORMAL:
return DRM_SCHED_PRIORITY_NORMAL;
case AMDGPU_CTX_PRIORITY_LOW:
case AMDGPU_CTX_PRIORITY_VERY_LOW:
-   return DRM_SCHED_PRIORITY_LOW;
+   return DRM_SCHED_PRIORITY_MIN;
case AMDGPU_CTX_PRIORITY_UNSET:
return DRM_SCHED_PRIORITY_UNSET;
default:
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c 

[PATCH 2/2] drm/scheduler: Remove priority macro INVALID (v2)

2020-08-14 Thread Luben Tuikov
Remove DRM_SCHED_PRIORITY_INVALID. We no longer
carry around an invalid priority and cut it off
at the source.

Backwards compatibility behaviour of AMDGPU CTX
IOCTL passing in garbage for context priority
from user space and then mapping that to
DRM_SCHED_PRIORITY_NORMAL is preserved.

v2: Revert "res"  --> "r" and
   "prio" --> "priority".

Signed-off-by: Luben Tuikov 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c   |  5 ++-
 drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c | 40 +--
 drivers/gpu/drm/amd/amdgpu/amdgpu_sched.h |  3 +-
 include/drm/gpu_scheduler.h   |  1 -
 4 files changed, 34 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
index 68eaa4f687a6..0a980f59cfd0 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
@@ -392,13 +392,12 @@ int amdgpu_ctx_ioctl(struct drm_device *dev, void *data,
struct amdgpu_device *adev = dev->dev_private;
struct amdgpu_fpriv *fpriv = filp->driver_priv;
 
-   r = 0;
id = args->in.ctx_id;
-   priority = amdgpu_to_sched_priority(args->in.priority);
+   r = amdgpu_to_sched_priority(args->in.priority, );
 
/* For backwards compatibility reasons, we need to accept
 * ioctls with garbage in the priority field */
-   if (priority == DRM_SCHED_PRIORITY_INVALID)
+   if (r == -EINVAL)
priority = DRM_SCHED_PRIORITY_NORMAL;
 
switch (args->in.op) {
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c
index 17661ede9488..9581283a4c78 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c
@@ -32,24 +32,32 @@
 
 #include "amdgpu_vm.h"
 
-enum drm_sched_priority amdgpu_to_sched_priority(int amdgpu_priority)
+int amdgpu_to_sched_priority(int amdgpu_priority,
+enum drm_sched_priority *prio)
 {
switch (amdgpu_priority) {
case AMDGPU_CTX_PRIORITY_VERY_HIGH:
-   return DRM_SCHED_PRIORITY_HIGH;
+   *prio = DRM_SCHED_PRIORITY_HIGH;
+   break;
case AMDGPU_CTX_PRIORITY_HIGH:
-   return DRM_SCHED_PRIORITY_HIGH;
+   *prio = DRM_SCHED_PRIORITY_HIGH;
+   break;
case AMDGPU_CTX_PRIORITY_NORMAL:
-   return DRM_SCHED_PRIORITY_NORMAL;
+   *prio = DRM_SCHED_PRIORITY_NORMAL;
+   break;
case AMDGPU_CTX_PRIORITY_LOW:
case AMDGPU_CTX_PRIORITY_VERY_LOW:
-   return DRM_SCHED_PRIORITY_MIN;
+   *prio = DRM_SCHED_PRIORITY_MIN;
+   break;
case AMDGPU_CTX_PRIORITY_UNSET:
-   return DRM_SCHED_PRIORITY_UNSET;
+   *prio = DRM_SCHED_PRIORITY_UNSET;
+   break;
default:
WARN(1, "Invalid context priority %d\n", amdgpu_priority);
-   return DRM_SCHED_PRIORITY_INVALID;
+   return -EINVAL;
}
+
+   return 0;
 }
 
 static int amdgpu_sched_process_priority_override(struct amdgpu_device *adev,
@@ -119,9 +127,20 @@ int amdgpu_sched_ioctl(struct drm_device *dev, void *data,
enum drm_sched_priority priority;
int r;
 
-   priority = amdgpu_to_sched_priority(args->in.priority);
-   if (priority == DRM_SCHED_PRIORITY_INVALID)
+   /* First check the op, then the op's argument.
+*/
+   switch (args->in.op) {
+   case AMDGPU_SCHED_OP_PROCESS_PRIORITY_OVERRIDE:
+   case AMDGPU_SCHED_OP_CONTEXT_PRIORITY_OVERRIDE:
+   break;
+   default:
+   DRM_ERROR("Invalid sched op specified: %d\n", args->in.op);
return -EINVAL;
+   }
+
+   r = amdgpu_to_sched_priority(args->in.priority, );
+   if (r)
+   return r;
 
switch (args->in.op) {
case AMDGPU_SCHED_OP_PROCESS_PRIORITY_OVERRIDE:
@@ -136,7 +155,8 @@ int amdgpu_sched_ioctl(struct drm_device *dev, void *data,
   priority);
break;
default:
-   DRM_ERROR("Invalid sched op specified: %d\n", args->in.op);
+   /* Impossible.
+*/
r = -EINVAL;
break;
}
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sched.h 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_sched.h
index 12299fd95691..67e5b2472f6a 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sched.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sched.h
@@ -30,7 +30,8 @@ enum drm_sched_priority;
 struct drm_device;
 struct drm_file;
 
-enum drm_sched_priority amdgpu_to_sched_priority(int amdgpu_priority);
+int amdgpu_to_sched_priority(int amdgpu_priority,
+enum drm_sched_priority *prio);
 int amdgpu_sched_ioctl(struct drm_device *dev, void *data,
   struct drm_file *filp);
 
diff --git 

Re: [PATCH] dma-buf: heap-helpers: Set dma-buf exporter name

2020-08-14 Thread John Stultz
On Fri, Aug 14, 2020 at 7:25 AM Ezequiel Garcia  wrote:
>
> Currently the heap helper uses DEFINE_DMA_BUF_EXPORT_INFO,
> which uses KBUILD_MODNAME for the dma_buf_export_info.exp_name.
>
> This effectively makes all dma-bufs exported by the heap
> helper as coming from "heap-helpers", instead of the actual heap name
> (cma, system, etc).
>
> Fix this by adding a dma-heap name getter, and then setting
> dma_buf_export_info.exp_name.
>
> Signed-off-by: Ezequiel Garcia 

Untested, but looks sane to me.

Acked-by: John Stultz 

On a slightly related note, I'm starting to regret the current
heap-helpers approach (Andrew probably gets an "I told you so" there
:). While it avoids a lot of duplication, it's really an all or
nothing approach, and doesn't really compare well to other drm style
helper functions.  I may eventually try to break the system and cma
implementations out of the helper code and try to consider a different
approach to avoid the duplication.

thanks
-john
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel


Re: [PATCH v10 3/5] drm/msm/dp: add support for DP PLL driver

2020-08-14 Thread Tanmay Shah

On 2020-08-14 10:05, Dmitry Baryshkov wrote:

On 12/08/2020 07:42, Tanmay Shah wrote:

From: Chandan Uddaraju 

Add the needed DP PLL specific files to support
display port interface on msm targets.


[skipped]

diff --git a/drivers/gpu/drm/msm/dp/dp_pll_private.h 
b/drivers/gpu/drm/msm/dp/dp_pll_private.h

new file mode 100644
index ..475ba6ed59ab
--- /dev/null
+++ b/drivers/gpu/drm/msm/dp/dp_pll_private.h
@@ -0,0 +1,98 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (c) 2016-2020, The Linux Foundation. All rights 
reserved.

+ */
+
+#ifndef __DP_PLL_10NM_H
+#define __DP_PLL_10NM_H
+
+#include "dp_pll.h"
+#include "dp_reg.h"
+
+#define DP_VCO_HSCLK_RATE_1620MHZDIV1000162UL
+#define DP_VCO_HSCLK_RATE_2700MHZDIV1000270UL
+#define DP_VCO_HSCLK_RATE_5400MHZDIV1000540UL
+#define DP_VCO_HSCLK_RATE_8100MHZDIV1000810UL
+
+#define NUM_DP_CLOCKS_MAX6
+
+#define DP_PHY_PLL_POLL_SLEEP_US500
+#define DP_PHY_PLL_POLL_TIMEOUT_US1
+
+#define DP_VCO_RATE_8100MHZDIV1000810UL
+#define DP_VCO_RATE_9720MHZDIV1000972UL
+#define DP_VCO_RATE_10800MHZDIV10001080UL
+
+struct dp_pll_vco_clk {
+struct clk_hw hw;
+unsigned longrate;/* current vco rate */
+u64min_rate;/* min vco rate */
+u64max_rate;/* max vco rate */
+void*priv;
+};
+
+struct dp_pll_db {


This struct should probably go into dp_pll_10nm.c. dp_pll_7nm.c, for
example, will use slightly different structure.



Sure, it sounds good. I will give it try. Thanks!


+struct msm_dp_pll *base;
+
+int id;
+struct platform_device *pdev;
+
+/* private clocks: */
+bool fixed_factor_clk[NUM_DP_CLOCKS_MAX];
+struct clk_hw *hws[NUM_DP_CLOCKS_MAX];


Then these two fields can use exact number of clocks rather than
NUM_DP_CLOCKS_MAX.



I didn't get this. I think NUM_DP_CLOCKS_MAX is doing same?


+u32 num_hws;
+
+/* lane and orientation settings */
+u8 lane_cnt;
+u8 orientation;
+
+/* COM PHY settings */
+u32 hsclk_sel;
+u32 dec_start_mode0;
+u32 div_frac_start1_mode0;
+u32 div_frac_start2_mode0;
+u32 div_frac_start3_mode0;
+u32 integloop_gain0_mode0;
+u32 integloop_gain1_mode0;
+u32 vco_tune_map;
+u32 lock_cmp1_mode0;
+u32 lock_cmp2_mode0;
+u32 lock_cmp3_mode0;
+u32 lock_cmp_en;
+
+/* PHY vco divider */
+u32 phy_vco_div;
+/*
+ * Certain pll's needs to update the same vco rate after resume 
in

+ * suspend/resume scenario. Cached the vco rate for such plls.
+ */
+unsigned longvco_cached_rate;
+u32cached_cfg0;
+u32cached_cfg1;
+u32cached_outdiv;
+
+uint32_t index;
+};
+
+static inline struct dp_pll_vco_clk *to_dp_vco_hw(struct clk_hw *hw)
+{
+return container_of(hw, struct dp_pll_vco_clk, hw);
+}
+
+#define to_msm_dp_pll(vco) ((struct msm_dp_pll *)vco->priv)
+
+#define to_dp_pll_db(x)((struct dp_pll_db *)x->priv)
+
+int dp_vco_set_rate_10nm(struct clk_hw *hw, unsigned long rate,
+unsigned long parent_rate);
+unsigned long dp_vco_recalc_rate_10nm(struct clk_hw *hw,
+unsigned long parent_rate);
+long dp_vco_round_rate_10nm(struct clk_hw *hw, unsigned long rate,
+unsigned long *parent_rate);
+int dp_vco_prepare_10nm(struct clk_hw *hw);
+void dp_vco_unprepare_10nm(struct clk_hw *hw);
+
+int msm_dp_pll_10nm_init(struct msm_dp_pll *dp_pll, int id);
+void msm_dp_pll_10nm_deinit(struct msm_dp_pll *dp_pll);


These functions don't seem to be used outside of dp_pll_10nm. What
about making them static?


I can't declare static to "init" and "deinit" as they are exported to 
dp_pll.c.

Rest of them I can move to dp_pll_10nm and then define static.

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel


Re: [PATCH] drm/dp_mst: Don't return error code when crtc is null

2020-08-14 Thread Lakha, Bhawanpreet
[AMD Official Use Only - Internal Distribution Only]

I took a closer look at this and there seems to be an issue in the function.

Crtc being null is a valid case here. The sequence that leads to this is, 
unplug -> disable crtc/release vcpi slots then hotplug. The issue is that 
pos->port is not guaranteed to be released in 
drm_dp_atomic_release_vcpi_slots() so list_for_each_entry(pos, 
_state->vcpis, next) {}  might still iterate thought it.

So, when a hotplug is done we still loop through the old port which has port! = 
null, crtc = null, and vpci = 0. I didn't find anything that I can use to 
remove the port from the list. So, a potential solution to this would be to add 
a check for vpci = 0 and skip that port.

Thoughts/Suggestions?

Bhawan




From: amd-gfx  on behalf of Lakha, 
Bhawanpreet 
Sent: August 14, 2020 2:52 PM
To: Ruhl, Michael J ; Lipski, Mikita 
; Kazlauskas, Nicholas ; 
Deucher, Alexander 
Cc: amd-...@lists.freedesktop.org ; 
dri-devel@lists.freedesktop.org 
Subject: Re: [PATCH] drm/dp_mst: Don't return error code when crtc is null


pos->port->connector?
This is checking the crtc not the connector. The crtc can be null if its 
disabled.

Since it is happening after a unplug->hotplug, I guess we are missing something 
in the disable sequence and the old connector is still in the list.

Bhawan

>>-Original Message-
>>From: dri-devel  On Behalf Of
>>Bhawanpreet Lakha
>>Sent: Friday, August 14, 2020 1:02 PM
>>To: mikita.lip...@amd.com; nicholas.kazlaus...@amd.com;
>>alexander.deuc...@amd.com
>>Cc: Bhawanpreet Lakha ; dri-
>>de...@lists.freedesktop.org; amd-...@lists.freedesktop.org
>>Subject: [PATCH] drm/dp_mst: Don't return error code when crtc is null
>>
>>[Why]
>>In certain cases the crtc can be NULL and returning -EINVAL causes
>>atomic check to fail when it shouln't. This leads to valid
>>configurations failing because atomic check fails.
>
>So is this a bug fix or an exception case, or an expected possibility?
>
>From my reading of the function comments, it is not clear that 
>pos->port->connector
>might be NULL for some reason.

>A better explanation of why this would occur would make this a much more
>useful commit message.
>

>My reading is that you ran into this issue an are masking it with this fix.
>
>Rather than this is a real possibility and this is the correct fix.
>
>Mike
>
>>[How]
>>Don't early return if crtc is null
>>
>>Signed-off-by: Bhawanpreet Lakha 
>>---
>> drivers/gpu/drm/drm_dp_mst_topology.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>>diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
>>b/drivers/gpu/drm/drm_dp_mst_topology.c
>>index 70c4b7afed12..bc90a1485699 100644
>>--- a/drivers/gpu/drm/drm_dp_mst_topology.c
>>+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
>>@@ -5037,8 +5037,8 @@ int drm_dp_mst_add_affected_dsc_crtcs(struct
>>drm_atomic_state *state, struct drm
>>
>>crtc = conn_state->crtc;
>>
>>-  if (WARN_ON(!crtc))
>>-  return -EINVAL;
>>+  if (!crtc)
>>+  continue;
>>
>>if (!drm_dp_mst_dsc_aux_for_port(pos->port))
>>continue;
>>--
>>2.17.1
>>
>>___
>>dri-devel mailing list
>>dri-devel@lists.freedesktop.org
>>https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-develdata=02%7C01%7CBhawanpreet.Lakha%40amd.com%7C0f5d55c551644fef3df908d840787b3e%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637330233520819407sdata=5N%2BBb0Qp3bd5zANfxovb%2BrVLAGnbP1sjyw3EeCHXj2w%3Dreserved=0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel


Re: [PATCH 2/2] drm/scheduler: Remove priority macro INVALID

2020-08-14 Thread Luben Tuikov
On 2020-08-14 3:28 p.m., Alex Deucher wrote:
> + dri-devel
> 
> 
> On Fri, Aug 14, 2020 at 3:14 PM Luben Tuikov  wrote:
>>
>> Remove DRM_SCHED_PRIORITY_INVALID. We no longer
>> carry around an invalid priority and cut it off
>> at the source.
>>
>> Backwards compatibility behaviour of AMDGPU CTX
>> IOCTL passing in garbage for context priority
>> from user space and then mapping that to
>> DRM_SCHED_PRIORITY_NORMAL is preserved.
>>
>> Signed-off-by: Luben Tuikov 
>> ---
>>  drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c   | 21 
>>  drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c | 62 +++
>>  drivers/gpu/drm/amd/amdgpu/amdgpu_sched.h |  3 +-
>>  include/drm/gpu_scheduler.h   |  1 -
>>  4 files changed, 53 insertions(+), 34 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
>> index fd97ac34277b..10d3bfa416c0 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
>> @@ -384,42 +384,41 @@ static int amdgpu_ctx_query2(struct amdgpu_device 
>> *adev,
>>  int amdgpu_ctx_ioctl(struct drm_device *dev, void *data,
>>  struct drm_file *filp)
>>  {
>> -   int r;
>> +   int res;
>> uint32_t id;
>> -   enum drm_sched_priority priority;
>> +   enum drm_sched_priority prio;
> 
> The variable renaming is not directly related to the functional change
> in the patch and should be split out as a separate patch is you think
> it should be applied.  I personally prefer the original naming.  The
> driver uses r or ret for return value checking pretty consistently.  I
> also prefer to spell things out unless the names are really long.
> Makes it more obvious what they are for.  Same comment on the
> functions below as well.

Sure, I can revert this. Personally, I don't like single letter
variables as they are very inexpressive and hard to search for.
I thought "prio" to be easier to type than "priority", but I can
revert this.

Regards,
Luben

> 
> Alex
> 
>>
>> union drm_amdgpu_ctx *args = data;
>> struct amdgpu_device *adev = dev->dev_private;
>> struct amdgpu_fpriv *fpriv = filp->driver_priv;
>>
>> -   r = 0;
>> id = args->in.ctx_id;
>> -   priority = amdgpu_to_sched_priority(args->in.priority);
>> +   res = amdgpu_to_sched_priority(args->in.priority, );
>>
>> /* For backwards compatibility reasons, we need to accept
>>  * ioctls with garbage in the priority field */
>> -   if (priority == DRM_SCHED_PRIORITY_INVALID)
>> -   priority = DRM_SCHED_PRIORITY_NORMAL;
>> +   if (res == -EINVAL)
>> +   prio = DRM_SCHED_PRIORITY_NORMAL;
>>
>> switch (args->in.op) {
>> case AMDGPU_CTX_OP_ALLOC_CTX:
>> -   r = amdgpu_ctx_alloc(adev, fpriv, filp, priority, );
>> +   res = amdgpu_ctx_alloc(adev, fpriv, filp, prio, );
>> args->out.alloc.ctx_id = id;
>> break;
>> case AMDGPU_CTX_OP_FREE_CTX:
>> -   r = amdgpu_ctx_free(fpriv, id);
>> +   res = amdgpu_ctx_free(fpriv, id);
>> break;
>> case AMDGPU_CTX_OP_QUERY_STATE:
>> -   r = amdgpu_ctx_query(adev, fpriv, id, >out);
>> +   res = amdgpu_ctx_query(adev, fpriv, id, >out);
>> break;
>> case AMDGPU_CTX_OP_QUERY_STATE2:
>> -   r = amdgpu_ctx_query2(adev, fpriv, id, >out);
>> +   res = amdgpu_ctx_query2(adev, fpriv, id, >out);
>> break;
>> default:
>> return -EINVAL;
>> }
>>
>> -   return r;
>> +   return res;
>>  }
>>
>>  struct amdgpu_ctx *amdgpu_ctx_get(struct amdgpu_fpriv *fpriv, uint32_t id)
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c
>> index e05bc22a0a49..2398eb646043 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c
>> @@ -32,24 +32,32 @@
>>
>>  #include "amdgpu_vm.h"
>>
>> -enum drm_sched_priority amdgpu_to_sched_priority(int amdgpu_priority)
>> +int amdgpu_to_sched_priority(int amdgpu_priority,
>> +enum drm_sched_priority *prio)
>>  {
>> switch (amdgpu_priority) {
>> case AMDGPU_CTX_PRIORITY_VERY_HIGH:
>> -   return DRM_SCHED_PRIORITY_HW;
>> +   *prio = DRM_SCHED_PRIORITY_HW;
>> +   break;
>> case AMDGPU_CTX_PRIORITY_HIGH:
>> -   return DRM_SCHED_PRIORITY_SW;
>> +   *prio = DRM_SCHED_PRIORITY_SW;
>> +   break;
>> case AMDGPU_CTX_PRIORITY_NORMAL:
>> -   return DRM_SCHED_PRIORITY_NORMAL;
>> +   *prio = DRM_SCHED_PRIORITY_NORMAL;
>> +   break;
>> case AMDGPU_CTX_PRIORITY_LOW:
>> case AMDGPU_CTX_PRIORITY_VERY_LOW:
>> -   return DRM_SCHED_PRIORITY_MIN;
>> 

Re: [Freedreno] [PATCH v10 2/5] drm/msm/dp: add displayPort driver support

2020-08-14 Thread Tanmay Shah

On 2020-08-14 10:56, Tanmay Shah wrote:

On 2020-08-14 10:12, Dmitry Baryshkov wrote:

Hello,

On 12/08/2020 07:42, Tanmay Shah wrote:

From: Chandan Uddaraju 


[skipped]


+   } else if ((dp_parser_check_prefix("ctrl", clk_name) ||
+  dp_parser_check_prefix("stream", clk_name))  &&
+  ctrl_clk_index < ctrl_clk_count) {
+   struct dss_clk *clk =
+   _power->clk_config[ctrl_clk_index];
+   strlcpy(clk->clk_name, clk_name, sizeof(clk->clk_name));
+   ctrl_clk_index++;
+
+   if (!strncmp(clk_name, "ctrl_link",
+   strlen("ctrl_link")) ||
+   !strncmp(clk_name, "stream_pixel",
+   strlen("ctrl_pixel")))


This should be "stream_pixel", I believe. I don't like macros, but
most probably it would help here. Also function/brace alignment could
be better (sorry, it really hides the issue here).



Thanks for reviews and good catch!! I completely missed it when I
renamed "ctrl_pixel".
Use of "stream_pixel" is very limited. So, instead of macros direct
name is used.
Fixing function and brace alignment sounds good idea insted.



Actually I will reuse dp_parser_check_prefix utility. It's already doing 
same Job.





+   clk->type = DSS_CLK_PCLK;
+   else
+   clk->type = DSS_CLK_AHB;
+   }
+   }

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel


Re: Role of DMA Heaps vs GEM in allocation

2020-08-14 Thread John Stultz
On Fri, Aug 14, 2020 at 3:59 AM Mikko Perttunen  wrote:
>
> I'm currently working on a new UAPI for Host1x/TegraDRM (see first draft
> in thread "[RFC] Host1x/TegraDRM UAPI"[1]). One question that has come
> up is regarding the buffer allocation mechanism. Traditionally, DRM
> drivers provide custom GEM allocation IOCTLs. However, we now have DMA
> Heaps, which would be sufficient for TegraDRM's needs, so we could skip
> implementing any GEM IOCTLs in the TegraDRM UAPI, and rely on importing
> DMA-BUFs. This would mean less code on TegraDRM's side.
>
> However, one complication with using DMA Heaps is that it only provides
> DMA-BUF FDs, so it is possible that a user application could run out of
> free file descriptors if it is not adjusting its soft FD limit. This
> would especially be a problem for existing applications that might have
> worked with the traditional GEM model and didn't need to adjust their FD
> limits, but would then fail in some situations with the increased FD
> usage of DMA-BUF FDs.

I'm not sure exactly if this would help, but I am working on some
exploratory tweaks to DMA BUF Heaps so that there could be an
in-kernel accessor that returns a struct dma_buf instead of a fd.

This is motivated as some folks want to use DMA BUF Heaps (if I
understand your approach) in a similar fashion, where the driver wants
to generate a DMA BUF but doesn't want to create their own DMA BUF
exporter which would duplicate one of the DMA BUF Heaps.

I'm a little mixed on this as part of the reason DMA BUF Heaps exists
as a userland interface is because its userland which knows the path
that a buffer will take, so userland is in the best position to
understand what type of buffer it needs to allocate. It seems to me
that drivers should instead import a buffer provided to them from
userland to fill, rather than allocating a buffer from a heap they
choose (which may constraint later use of that buffer). But, I also
grant that drivers implementing their own DMA BUF exporters that
duplicate existing code is silly, so having in-kernel allocation
interfaces may be reasonable.

However, the efforts are also somewhat blocked on having a public
in-kernel user of such an interface, so they are basically only
exploratory at the moment. So if you have an in-kernel user interested
in something like this, I'd be glad to get further input.

This probably doesn't help answer your question wrt to GEM, and I'd
defer to Daniel there. :)

thanks
-john
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel


Re: [RFC][PATCH v2 2/2] dma-heap: Add a system-uncached heap

2020-08-14 Thread John Stultz
On Fri, Aug 14, 2020 at 9:15 AM Ezequiel Garcia
 wrote:
> Thanks for the patch.
>
> On Fri, 14 Aug 2020 at 03:25, John Stultz  wrote:
> >
> > This adds a heap that allocates non-contiguous buffers that are
> > marked as writecombined, so they are not cached by the CPU.
> >
>
> What's the rationale for exposing the memory
> attribute as a new heap, instead of just introducing flags?
>
> I guess this calls for some guidelines on what situations
> call for a separate heap, and when it's just a modifier flag.

YES! :) A big part of this patch is to start a discussion and feel out
what properties of heaps are generic enough to be flags, and what
aspects are unique enough to deserve having their own heap
implementation.

ION used the ION_FLAG_CACHED bit for this and considered it a generic
property (though by default all buffers were uncached). This seemed to
cause enough friction in reviews that we dropped it and used cachable
buffers for the initial DMA BUF heaps.

Further, I want to make sure we avoid the custom flag abuse that ION
saw, especially with vendor heaps. So I think having each unique
behavior being a separate heap is a reasonable stance.

That said, we added the (currently unused) heap-flags field to the
interface as there may be some attributes or modalities that are truly
generic across heaps. So if we want to add an UNCACHED flag instead,
I'm open to that.. however I want to make sure it has clear general
meaning so that its behavior is consistent across all heaps and
architectures (or produces an error if it's not supported).

thanks
-john
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel


Re: [RFC][PATCH v2 2/2] dma-heap: Add a system-uncached heap

2020-08-14 Thread John Stultz
On Fri, Aug 14, 2020 at 2:17 AM Daniel Vetter  wrote:
>
> On Fri, Aug 14, 2020 at 06:24:58AM +, John Stultz wrote:
> > This adds a heap that allocates non-contiguous buffers that are
> > marked as writecombined, so they are not cached by the CPU.
> >
> > This is useful, as most graphics buffers are usually not touched
> > by the CPU or only written into once by the CPU. So when mapping
> > the buffer over and over between devices, we can skip the CPU
> > syncing, which saves a lot of cache management overhead, greatly
> > improving performance.
> >
> > For folk using ION, there was a ION_FLAG_CACHED flag, which
> > signaled if the returned buffer should be CPU cacheable or not.
> > With DMA-BUF heaps, we have no such flag, and by default the
> > current heaps (system and cma) produce CPU cachable buffers.
> > So for folks transitioning from ION to DMA-BUF Heaps, this fills
> > in some of that missing functionality.
> >
> > This does have a few "ugly" bits that were required to get
> > the buffer properly flushed out initially which I'd like to
> > improve. So feedback would be very welcome!
> >
> > Many thanks to Liam Mark for his help to get this working.
> >
> > Cc: Sumit Semwal 
> > Cc: Andrew F. Davis 
> > Cc: Benjamin Gaignard 
> > Cc: Liam Mark 
> > Cc: Laura Abbott 
> > Cc: Brian Starkey 
> > Cc: Hridya Valsaraju 
> > Cc: Robin Murphy 
> > Cc: linux-me...@vger.kernel.org
> > Cc: dri-devel@lists.freedesktop.org
> > Signed-off-by: John Stultz 
> > ---
> > v2:
> > * Fix build issue on sh reported-by: kernel test robot 
> > * Rework to use for_each_sgtable_sg(), dma_map_sgtable(), and
> >   for_each_sg_page() along with numerous other cleanups suggested
> >   by Robin Murphy
>
> Mildly annoying me, but where's the userspace for this? Do we have a
> gralloc somewhere that works with open driver stacks and uses this?

So this is still early RFC, but I've added support to the HiKey960
gralloc, and have pending patches (following DRM rules) I pushed here:
  https://android-review.googlesource.com/c/device/linaro/hikey/+/1399519

And avoiding the cache syncing overhead improves performance nicely on
that board.

There is also work in progress to change the codec2 implementation in
AOSP (which uses ion directly), to move over to DMA BUF heaps and as
it used the !ION_FLAG_CACHED case there this heap would be useful to
match ION's functionality.

The latest patches for that are here:
https://android-review.googlesource.com/c/platform/frameworks/av/+/1360640
(though I'm expecting a deeper rework on those)

thanks
-john
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel


Re: [PATCH] drm/modeset-lock: Take the modeset BKL for legacy drivers

2020-08-14 Thread Alex Deucher
On Fri, Aug 14, 2020 at 5:38 AM Daniel Vetter  wrote:
>
> This fell off in the conversion in
>
> commit 9bcaa3fe58ab7559e71df798bcff6e0795158695
> Author: Michal Orzel 
> Date:   Tue Apr 28 19:10:04 2020 +0200
>
> drm: Replace drm_modeset_lock/unlock_all with DRM_MODESET_LOCK_ALL_* 
> helpers
>
> but it's caught by the drm_warn_on_modeset_not_all_locked() that the
> legacy modeset code uses. Since this is the bkl and it's unclear
> what's all protected, play it safe and grab it again for legacy
> drivers.
>
> Unfortunately this means we need to sprinkle a few more #includes
> around.
>
> Also we need to add the drm_device as a parameter to the _END macro.
>
> Finally remove the mute_lock() from setcrtc, since that's now done by
> the macro.
>
> Cc: Alex Deucher 
> References: https://gitlab.freedesktop.org/drm/amd/-/issues/1224
> Fixes: 9bcaa3fe58ab ("drm: Replace drm_modeset_lock/unlock_all with 
> DRM_MODESET_LOCK_ALL_* helpers")
> Cc: Michal Orzel 
> Cc: Daniel Vetter 
> Cc: Maarten Lankhorst 
> Cc: Maxime Ripard 
> Cc: Thomas Zimmermann 
> Cc: David Airlie 
> Cc: Daniel Vetter 
> Cc: dri-devel@lists.freedesktop.org
> Cc:  # v5.8+
> Signed-off-by: Daniel Vetter 
> --
> Patch compiles but otherwise untested, and I'll go on vacations now
> for 2 weeks. Alex, can you pls take care of this?

Looks good to me.
Reviewed-by: Alex Deucher 

Also confirmed to fix the issue.  I'll push to drm-misc.

Thanks!

Alex

>
> Thanks, Daniel
> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 7 ---
>  drivers/gpu/drm/drm_color_mgmt.c| 2 +-
>  drivers/gpu/drm/drm_crtc.c  | 4 +---
>  drivers/gpu/drm/drm_mode_object.c   | 4 ++--
>  drivers/gpu/drm/drm_plane.c | 2 +-
>  include/drm/drm_modeset_lock.h  | 9 +++--
>  6 files changed, 16 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
> b/drivers/gpu/drm/drm_atomic_helper.c
> index f67ee513a7cc..7515a40b2056 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -34,6 +34,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -3109,7 +3110,7 @@ void drm_atomic_helper_shutdown(struct drm_device *dev)
> if (ret)
> DRM_ERROR("Disabling all crtc's during unload failed with 
> %i\n", ret);
>
> -   DRM_MODESET_LOCK_ALL_END(ctx, ret);
> +   DRM_MODESET_LOCK_ALL_END(dev, ctx, ret);
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_shutdown);
>
> @@ -3249,7 +3250,7 @@ struct drm_atomic_state 
> *drm_atomic_helper_suspend(struct drm_device *dev)
> }
>
>  unlock:
> -   DRM_MODESET_LOCK_ALL_END(ctx, err);
> +   DRM_MODESET_LOCK_ALL_END(dev, ctx, err);
> if (err)
> return ERR_PTR(err);
>
> @@ -3330,7 +3331,7 @@ int drm_atomic_helper_resume(struct drm_device *dev,
>
> err = drm_atomic_helper_commit_duplicated_state(state, );
>
> -   DRM_MODESET_LOCK_ALL_END(ctx, err);
> +   DRM_MODESET_LOCK_ALL_END(dev, ctx, err);
> drm_atomic_state_put(state);
>
> return err;
> diff --git a/drivers/gpu/drm/drm_color_mgmt.c 
> b/drivers/gpu/drm/drm_color_mgmt.c
> index c93123ff7c21..138ff34b31db 100644
> --- a/drivers/gpu/drm/drm_color_mgmt.c
> +++ b/drivers/gpu/drm/drm_color_mgmt.c
> @@ -294,7 +294,7 @@ int drm_mode_gamma_set_ioctl(struct drm_device *dev,
>  crtc->gamma_size, );
>
>  out:
> -   DRM_MODESET_LOCK_ALL_END(ctx, ret);
> +   DRM_MODESET_LOCK_ALL_END(dev, ctx, ret);
> return ret;
>
>  }
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 283bcc4362ca..aecdd7ea26dc 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -588,7 +588,6 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
> if (crtc_req->mode_valid && !drm_lease_held(file_priv, 
> plane->base.id))
> return -EACCES;
>
> -   mutex_lock(>dev->mode_config.mutex);
> DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx,
>DRM_MODESET_ACQUIRE_INTERRUPTIBLE, ret);
>
> @@ -756,8 +755,7 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
> fb = NULL;
> mode = NULL;
>
> -   DRM_MODESET_LOCK_ALL_END(ctx, ret);
> -   mutex_unlock(>dev->mode_config.mutex);
> +   DRM_MODESET_LOCK_ALL_END(dev, ctx, ret);
>
> return ret;
>  }
> diff --git a/drivers/gpu/drm/drm_mode_object.c 
> b/drivers/gpu/drm/drm_mode_object.c
> index 901b078abf40..db05f386a709 100644
> --- a/drivers/gpu/drm/drm_mode_object.c
> +++ b/drivers/gpu/drm/drm_mode_object.c
> @@ -428,7 +428,7 @@ int drm_mode_obj_get_properties_ioctl(struct drm_device 
> *dev, void *data,
>  out_unref:
> drm_mode_object_put(obj);
>  out:
> -   DRM_MODESET_LOCK_ALL_END(ctx, ret);
> +   DRM_MODESET_LOCK_ALL_END(dev, ctx, ret);
> return ret;
>  }
>
> @@ -470,7 +470,7 @@ static int set_property_legacy(struct drm_mode_object 
> *obj,
> 

[PATCH v2] drm/mcde: Retry DSI read/write transactions

2020-08-14 Thread Linus Walleij
The vendor driver makes a few retries on read DSI
transactions, something that is needed especially in
case of read (such as reading the panel MTP ID) while
the panel is running in video mode. This happens on
the Samsung s6e63m0 panel on the Golden device.

Retry reads and writes alike three times.

Cc: Stephan Gerhold 
Signed-off-by: Linus Walleij 
---
ChangeLog v1->v2:
- Retry three times.
- Only retry the actual command transmission like the vendor
  driver does, no need to set up all registers and do checks
  all over. Break out a part of the mcde_dsi_host_transfer()
  function to achieve this.
---
 drivers/gpu/drm/mcde/mcde_dsi.c | 158 +++-
 1 file changed, 92 insertions(+), 66 deletions(-)

diff --git a/drivers/gpu/drm/mcde/mcde_dsi.c b/drivers/gpu/drm/mcde/mcde_dsi.c
index 4ce8cc5f0be2..b3c5d3cbda92 100644
--- a/drivers/gpu/drm/mcde/mcde_dsi.c
+++ b/drivers/gpu/drm/mcde/mcde_dsi.c
@@ -208,79 +208,16 @@ static int mcde_dsi_host_detach(struct mipi_dsi_host 
*host,
 (type == MIPI_DSI_GENERIC_READ_REQUEST_2_PARAM) || \
 (type == MIPI_DSI_DCS_READ))
 
-static ssize_t mcde_dsi_host_transfer(struct mipi_dsi_host *host,
- const struct mipi_dsi_msg *msg)
+static int mcde_dsi_execute_transfer(struct mcde_dsi *d,
+const struct mipi_dsi_msg *msg)
 {
-   struct mcde_dsi *d = host_to_mcde_dsi(host);
const u32 loop_delay_us = 10; /* us */
-   const u8 *tx = msg->tx_buf;
u32 loop_counter;
size_t txlen = msg->tx_len;
size_t rxlen = msg->rx_len;
+   int i;
u32 val;
int ret;
-   int i;
-
-   if (txlen > 16) {
-   dev_err(d->dev,
-   "dunno how to write more than 16 bytes yet\n");
-   return -EIO;
-   }
-   if (rxlen > 4) {
-   dev_err(d->dev,
-   "dunno how to read more than 4 bytes yet\n");
-   return -EIO;
-   }
-
-   dev_dbg(d->dev,
-   "message to channel %d, write %zd bytes read %zd bytes\n",
-   msg->channel, txlen, rxlen);
-
-   /* Command "nature" */
-   if (MCDE_DSI_HOST_IS_READ(msg->type))
-   /* MCTL_MAIN_DATA_CTL already set up */
-   val = DSI_DIRECT_CMD_MAIN_SETTINGS_CMD_NAT_READ;
-   else
-   val = DSI_DIRECT_CMD_MAIN_SETTINGS_CMD_NAT_WRITE;
-   /*
-* More than 2 bytes will not fit in a single packet, so it's
-* time to set the "long not short" bit. One byte is used by
-* the MIPI DCS command leaving just one byte for the payload
-* in a short package.
-*/
-   if (mipi_dsi_packet_format_is_long(msg->type))
-   val |= DSI_DIRECT_CMD_MAIN_SETTINGS_CMD_LONGNOTSHORT;
-   val |= 0 << DSI_DIRECT_CMD_MAIN_SETTINGS_CMD_ID_SHIFT;
-   val |= txlen << DSI_DIRECT_CMD_MAIN_SETTINGS_CMD_SIZE_SHIFT;
-   val |= DSI_DIRECT_CMD_MAIN_SETTINGS_CMD_LP_EN;
-   val |= msg->type << DSI_DIRECT_CMD_MAIN_SETTINGS_CMD_HEAD_SHIFT;
-   writel(val, d->regs + DSI_DIRECT_CMD_MAIN_SETTINGS);
-
-   /* MIPI DCS command is part of the data */
-   if (txlen > 0) {
-   val = 0;
-   for (i = 0; i < 4 && i < txlen; i++)
-   val |= tx[i] << (i * 8);
-   }
-   writel(val, d->regs + DSI_DIRECT_CMD_WRDAT0);
-   if (txlen > 4) {
-   val = 0;
-   for (i = 0; i < 4 && (i + 4) < txlen; i++)
-   val |= tx[i + 4] << (i * 8);
-   writel(val, d->regs + DSI_DIRECT_CMD_WRDAT1);
-   }
-   if (txlen > 8) {
-   val = 0;
-   for (i = 0; i < 4 && (i + 8) < txlen; i++)
-   val |= tx[i + 8] << (i * 8);
-   writel(val, d->regs + DSI_DIRECT_CMD_WRDAT2);
-   }
-   if (txlen > 12) {
-   val = 0;
-   for (i = 0; i < 4 && (i + 12) < txlen; i++)
-   val |= tx[i + 12] << (i * 8);
-   writel(val, d->regs + DSI_DIRECT_CMD_WRDAT3);
-   }
 
writel(~0, d->regs + DSI_DIRECT_CMD_STS_CLR);
writel(~0, d->regs + DSI_CMD_MODE_STS_CLR);
@@ -297,6 +234,7 @@ static ssize_t mcde_dsi_host_transfer(struct mipi_dsi_host 
*host,
usleep_range(loop_delay_us, (loop_delay_us * 3) / 2);
if (!loop_counter) {
dev_err(d->dev, "DSI read timeout!\n");
+   /* Set exit code and retry */
return -ETIME;
}
} else {
@@ -307,6 +245,7 @@ static ssize_t mcde_dsi_host_transfer(struct mipi_dsi_host 
*host,
usleep_range(loop_delay_us, (loop_delay_us * 3) / 2);
 
if (!loop_counter) {
+   /* Set exit code and retry */
dev_err(d->dev, "DSI write timeout!\n");
return -ETIME;
   

Re: [PATCH 2/2] drm/scheduler: Remove priority macro INVALID

2020-08-14 Thread Alex Deucher
+ dri-devel


On Fri, Aug 14, 2020 at 3:14 PM Luben Tuikov  wrote:
>
> Remove DRM_SCHED_PRIORITY_INVALID. We no longer
> carry around an invalid priority and cut it off
> at the source.
>
> Backwards compatibility behaviour of AMDGPU CTX
> IOCTL passing in garbage for context priority
> from user space and then mapping that to
> DRM_SCHED_PRIORITY_NORMAL is preserved.
>
> Signed-off-by: Luben Tuikov 
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c   | 21 
>  drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c | 62 +++
>  drivers/gpu/drm/amd/amdgpu/amdgpu_sched.h |  3 +-
>  include/drm/gpu_scheduler.h   |  1 -
>  4 files changed, 53 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
> index fd97ac34277b..10d3bfa416c0 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
> @@ -384,42 +384,41 @@ static int amdgpu_ctx_query2(struct amdgpu_device *adev,
>  int amdgpu_ctx_ioctl(struct drm_device *dev, void *data,
>  struct drm_file *filp)
>  {
> -   int r;
> +   int res;
> uint32_t id;
> -   enum drm_sched_priority priority;
> +   enum drm_sched_priority prio;

The variable renaming is not directly related to the functional change
in the patch and should be split out as a separate patch is you think
it should be applied.  I personally prefer the original naming.  The
driver uses r or ret for return value checking pretty consistently.  I
also prefer to spell things out unless the names are really long.
Makes it more obvious what they are for.  Same comment on the
functions below as well.

Alex

>
> union drm_amdgpu_ctx *args = data;
> struct amdgpu_device *adev = dev->dev_private;
> struct amdgpu_fpriv *fpriv = filp->driver_priv;
>
> -   r = 0;
> id = args->in.ctx_id;
> -   priority = amdgpu_to_sched_priority(args->in.priority);
> +   res = amdgpu_to_sched_priority(args->in.priority, );
>
> /* For backwards compatibility reasons, we need to accept
>  * ioctls with garbage in the priority field */
> -   if (priority == DRM_SCHED_PRIORITY_INVALID)
> -   priority = DRM_SCHED_PRIORITY_NORMAL;
> +   if (res == -EINVAL)
> +   prio = DRM_SCHED_PRIORITY_NORMAL;
>
> switch (args->in.op) {
> case AMDGPU_CTX_OP_ALLOC_CTX:
> -   r = amdgpu_ctx_alloc(adev, fpriv, filp, priority, );
> +   res = amdgpu_ctx_alloc(adev, fpriv, filp, prio, );
> args->out.alloc.ctx_id = id;
> break;
> case AMDGPU_CTX_OP_FREE_CTX:
> -   r = amdgpu_ctx_free(fpriv, id);
> +   res = amdgpu_ctx_free(fpriv, id);
> break;
> case AMDGPU_CTX_OP_QUERY_STATE:
> -   r = amdgpu_ctx_query(adev, fpriv, id, >out);
> +   res = amdgpu_ctx_query(adev, fpriv, id, >out);
> break;
> case AMDGPU_CTX_OP_QUERY_STATE2:
> -   r = amdgpu_ctx_query2(adev, fpriv, id, >out);
> +   res = amdgpu_ctx_query2(adev, fpriv, id, >out);
> break;
> default:
> return -EINVAL;
> }
>
> -   return r;
> +   return res;
>  }
>
>  struct amdgpu_ctx *amdgpu_ctx_get(struct amdgpu_fpriv *fpriv, uint32_t id)
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c
> index e05bc22a0a49..2398eb646043 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c
> @@ -32,24 +32,32 @@
>
>  #include "amdgpu_vm.h"
>
> -enum drm_sched_priority amdgpu_to_sched_priority(int amdgpu_priority)
> +int amdgpu_to_sched_priority(int amdgpu_priority,
> +enum drm_sched_priority *prio)
>  {
> switch (amdgpu_priority) {
> case AMDGPU_CTX_PRIORITY_VERY_HIGH:
> -   return DRM_SCHED_PRIORITY_HW;
> +   *prio = DRM_SCHED_PRIORITY_HW;
> +   break;
> case AMDGPU_CTX_PRIORITY_HIGH:
> -   return DRM_SCHED_PRIORITY_SW;
> +   *prio = DRM_SCHED_PRIORITY_SW;
> +   break;
> case AMDGPU_CTX_PRIORITY_NORMAL:
> -   return DRM_SCHED_PRIORITY_NORMAL;
> +   *prio = DRM_SCHED_PRIORITY_NORMAL;
> +   break;
> case AMDGPU_CTX_PRIORITY_LOW:
> case AMDGPU_CTX_PRIORITY_VERY_LOW:
> -   return DRM_SCHED_PRIORITY_MIN;
> +   *prio = DRM_SCHED_PRIORITY_MIN;
> +   break;
> case AMDGPU_CTX_PRIORITY_UNSET:
> -   return DRM_SCHED_PRIORITY_UNSET;
> +   *prio = DRM_SCHED_PRIORITY_UNSET;
> +   break;
> default:
> WARN(1, "Invalid context priority %d\n", amdgpu_priority);
> -   return DRM_SCHED_PRIORITY_INVALID;
> +   

Re: [PATCH 0/2] Fixes to drm_sched_priority

2020-08-14 Thread Alex Deucher
+ dri-devel

On Fri, Aug 14, 2020 at 3:14 PM Luben Tuikov  wrote:
>
> Some fixes to enum drm_sched_priority which I'd wanted to do
> since last year.
>
> For instance, renaming MAX to COUNT, as usually a maximum value
> is a value which is part of the set of values, (e.g. a maxima of
> a function), and thus assignable, whereby a count is the size of
> a set (the enumeration in this case). It also makes it clearer
> when used to define size of arrays.
>
> Removing some redundant naming and perhaps better naming could be
> had for PRIORITY_SW and PRIORITY_HW, maybe "moderate" and
> "temperate" respectively. However, I've left them as "sw" and
> "hw".
>
> Also remove a macro which was used in only a single place.
>
> In the second patch, remove priority INVALID, since it is not a
> priority, e.g. a scheduler cannot be assigned and run at priority
> "invalid". It seems to be more of a directive, a status, of user
> input, and as such has no place in the enumeration of priority
> levels.
>
> Something else I'd like to do, is to eliminate priority
> enumeration "UNSET", since it is not really a priority level,
> but  a directive, instructing the code to "reset the priority
> of a context to the initial priority".
>
> At the moment, this functionality is overloaded to this priority
> value, and it should be an IOCTL op, as opposed to a priority value.
>
> Tested on my desktop system, which is running a kernel with
> this patch applied.
>
> Luben Tuikov (2):
>   drm/scheduler: Scheduler priority fixes
>   drm/scheduler: Remove priority macro INVALID
>
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c   | 27 +-
>  drivers/gpu/drm/amd/amdgpu/amdgpu_job.c   |  2 +-
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c  |  2 +-
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h  |  2 +-
>  drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c | 62 +++
>  drivers/gpu/drm/amd/amdgpu/amdgpu_sched.h |  3 +-
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c   |  2 +-
>  drivers/gpu/drm/scheduler/sched_main.c|  8 +--
>  include/drm/gpu_scheduler.h   | 16 +++---
>  9 files changed, 73 insertions(+), 51 deletions(-)
>
> --
> 2.28.0.215.g878e727637
>
> ___
> amd-gfx mailing list
> amd-...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel


Re: [PATCH] drm/dp_mst: Don't return error code when crtc is null

2020-08-14 Thread Lakha, Bhawanpreet
[AMD Official Use Only - Internal Distribution Only]


pos->port->connector?
This is checking the crtc not the connector. The crtc can be null if its 
disabled.

Since it is happening after a unplug->hotplug, I guess we are missing something 
in the disable sequence and the old connector is still in the list.

Bhawan

>>-Original Message-
>>From: dri-devel  On Behalf Of
>>Bhawanpreet Lakha
>>Sent: Friday, August 14, 2020 1:02 PM
>>To: mikita.lip...@amd.com; nicholas.kazlaus...@amd.com;
>>alexander.deuc...@amd.com
>>Cc: Bhawanpreet Lakha ; dri-
>>de...@lists.freedesktop.org; amd-...@lists.freedesktop.org
>>Subject: [PATCH] drm/dp_mst: Don't return error code when crtc is null
>>
>>[Why]
>>In certain cases the crtc can be NULL and returning -EINVAL causes
>>atomic check to fail when it shouln't. This leads to valid
>>configurations failing because atomic check fails.
>
>So is this a bug fix or an exception case, or an expected possibility?
>
>From my reading of the function comments, it is not clear that 
>pos->port->connector
>might be NULL for some reason.

>A better explanation of why this would occur would make this a much more
>useful commit message.
>

>My reading is that you ran into this issue an are masking it with this fix.
>
>Rather than this is a real possibility and this is the correct fix.
>
>Mike
>
>>[How]
>>Don't early return if crtc is null
>>
>>Signed-off-by: Bhawanpreet Lakha 
>>---
>> drivers/gpu/drm/drm_dp_mst_topology.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>>diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
>>b/drivers/gpu/drm/drm_dp_mst_topology.c
>>index 70c4b7afed12..bc90a1485699 100644
>>--- a/drivers/gpu/drm/drm_dp_mst_topology.c
>>+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
>>@@ -5037,8 +5037,8 @@ int drm_dp_mst_add_affected_dsc_crtcs(struct
>>drm_atomic_state *state, struct drm
>>
>>crtc = conn_state->crtc;
>>
>>-  if (WARN_ON(!crtc))
>>-  return -EINVAL;
>>+  if (!crtc)
>>+  continue;
>>
>>if (!drm_dp_mst_dsc_aux_for_port(pos->port))
>>continue;
>>--
>>2.17.1
>>
>>___
>>dri-devel mailing list
>>dri-devel@lists.freedesktop.org
>>https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-develdata=02%7C01%7CBhawanpreet.Lakha%40amd.com%7C0f5d55c551644fef3df908d840787b3e%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637330233520819407sdata=5N%2BBb0Qp3bd5zANfxovb%2BrVLAGnbP1sjyw3EeCHXj2w%3Dreserved=0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel


Re: [Freedreno] [PATCH v10 2/5] drm/msm/dp: add displayPort driver support

2020-08-14 Thread Tanmay Shah

On 2020-08-14 10:12, Dmitry Baryshkov wrote:

Hello,

On 12/08/2020 07:42, Tanmay Shah wrote:

From: Chandan Uddaraju 


[skipped]


+   } else if ((dp_parser_check_prefix("ctrl", clk_name) ||
+  dp_parser_check_prefix("stream", clk_name))  &&
+  ctrl_clk_index < ctrl_clk_count) {
+   struct dss_clk *clk =
+   _power->clk_config[ctrl_clk_index];
+   strlcpy(clk->clk_name, clk_name, sizeof(clk->clk_name));
+   ctrl_clk_index++;
+
+   if (!strncmp(clk_name, "ctrl_link",
+   strlen("ctrl_link")) ||
+   !strncmp(clk_name, "stream_pixel",
+   strlen("ctrl_pixel")))


This should be "stream_pixel", I believe. I don't like macros, but
most probably it would help here. Also function/brace alignment could
be better (sorry, it really hides the issue here).



Thanks for reviews and good catch!! I completely missed it when I 
renamed "ctrl_pixel".
Use of "stream_pixel" is very limited. So, instead of macros direct name 
is used.

Fixing function and brace alignment sounds good idea insted.






+   clk->type = DSS_CLK_PCLK;
+   else
+   clk->type = DSS_CLK_AHB;
+   }
+   }

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel


RE: [PATCH] drm/dp_mst: Don't return error code when crtc is null

2020-08-14 Thread Ruhl, Michael J
>-Original Message-
>From: dri-devel  On Behalf Of
>Bhawanpreet Lakha
>Sent: Friday, August 14, 2020 1:02 PM
>To: mikita.lip...@amd.com; nicholas.kazlaus...@amd.com;
>alexander.deuc...@amd.com
>Cc: Bhawanpreet Lakha ; dri-
>de...@lists.freedesktop.org; amd-...@lists.freedesktop.org
>Subject: [PATCH] drm/dp_mst: Don't return error code when crtc is null
>
>[Why]
>In certain cases the crtc can be NULL and returning -EINVAL causes
>atomic check to fail when it shouln't. This leads to valid
>configurations failing because atomic check fails.

So is this a bug fix or an exception case, or an expected possibility?

>From my reading of the function comments, it is not clear that 
>pos->port->connector
might be NULL for some reason.

A better explanation of why this would occur would make this a much more
useful commit message.

My reading is that you ran into this issue an are masking it with this fix.

Rather than this is a real possibility and this is the correct fix.

Mike

>[How]
>Don't early return if crtc is null
>
>Signed-off-by: Bhawanpreet Lakha 
>---
> drivers/gpu/drm/drm_dp_mst_topology.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
>b/drivers/gpu/drm/drm_dp_mst_topology.c
>index 70c4b7afed12..bc90a1485699 100644
>--- a/drivers/gpu/drm/drm_dp_mst_topology.c
>+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
>@@ -5037,8 +5037,8 @@ int drm_dp_mst_add_affected_dsc_crtcs(struct
>drm_atomic_state *state, struct drm
>
>   crtc = conn_state->crtc;
>
>-  if (WARN_ON(!crtc))
>-  return -EINVAL;
>+  if (!crtc)
>+  continue;
>
>   if (!drm_dp_mst_dsc_aux_for_port(pos->port))
>   continue;
>--
>2.17.1
>
>___
>dri-devel mailing list
>dri-devel@lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/dri-devel
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel


Re: [git pull] drm for 5.8-rc1

2020-08-14 Thread Daniel Vetter
On Fri, Aug 14, 2020 at 7:17 PM Daniel Stone  wrote:
>
> Hi,
>
> On Fri, 14 Aug 2020 at 17:22, Thierry Reding  wrote:
> > I suspect that the reason why this works in X but not in Wayland is
> > because X passes the right usage flags, whereas Weston may not. But I'll
> > have to investigate more in order to be sure.
>
> Weston allocates its own buffers for displaying the result of
> composition through GBM with USE_SCANOUT, which is definitely correct.
>
> Wayland clients (common to all compositors, in Mesa's
> src/egl/drivers/dri2/platform_wayland.c) allocate with USE_SHARED but
> _not_ USE_SCANOUT, which is correct in that they are guaranteed to be
> shared, but not guaranteed to be scanned out. The expectation is that
> non-scanout-compatible buffers would be rejected by gbm_bo_import if
> not drmModeAddFB2.
>
> One difference between Weston and all other compositors (GNOME Shell,
> KWin, Sway, etc) is that Weston uses KMS planes for composition when
> it can (i.e. when gbm_bo_import from dmabuf + drmModeAddFB2 from
> gbm_bo handle + atomic check succeed), but the other compositors only
> use the GPU. So if you have different assumptions about the layout of
> imported buffers between the GPU and KMS, that would explain a fair
> bit.

Yeah non-modifiered multi-gpu (of any kind) is pretty much hopeless I
think. I guess the only option is if the tegra mesa driver forces
linear and an extra copy on everything that's USE_SHARED or
USE_SCANOUT.

> > Perhaps we can go and release X 1.21.0 with that modifier enablement
> > patch and that'll motivate desktops to adopt it as well as the default?
>
> Unfortunately we don't really have a good way out of this one. They
> were disabled because the non-modifier path on Intel can be linear or
> X-tiled (row-major), whereas the modifier path enables Y-tiled
> (column-major) and compressed layouts. Y-tiled is the most efficient,
> but Intel could only spare about six transistors for the global FIFO
> shared between all their plane fetch engines, and Y-tiled blows
> straight through it. Both X and Shell would thus fail to enable high
> resolutions or many heads (2x 4K is enough even on modern platforms
> IIRC), so they just turned modifiers off.
>
> The best solution would be to do a global atomic_check across all
> outputs and just blacklist modifiers until you find one which works,
> but Shell doesn't yet have that code, and -modesetting ... well,
> no-one's volunteered to do that yet, or probably ever.

Yeah best bet for modifiered X is Xwayland on top of weston right now :-/
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel


Re: [git pull] drm for 5.8-rc1

2020-08-14 Thread Daniel Stone
Hi,

On Fri, 14 Aug 2020 at 17:22, Thierry Reding  wrote:
> I suspect that the reason why this works in X but not in Wayland is
> because X passes the right usage flags, whereas Weston may not. But I'll
> have to investigate more in order to be sure.

Weston allocates its own buffers for displaying the result of
composition through GBM with USE_SCANOUT, which is definitely correct.

Wayland clients (common to all compositors, in Mesa's
src/egl/drivers/dri2/platform_wayland.c) allocate with USE_SHARED but
_not_ USE_SCANOUT, which is correct in that they are guaranteed to be
shared, but not guaranteed to be scanned out. The expectation is that
non-scanout-compatible buffers would be rejected by gbm_bo_import if
not drmModeAddFB2.

One difference between Weston and all other compositors (GNOME Shell,
KWin, Sway, etc) is that Weston uses KMS planes for composition when
it can (i.e. when gbm_bo_import from dmabuf + drmModeAddFB2 from
gbm_bo handle + atomic check succeed), but the other compositors only
use the GPU. So if you have different assumptions about the layout of
imported buffers between the GPU and KMS, that would explain a fair
bit.

> Perhaps we can go and release X 1.21.0 with that modifier enablement
> patch and that'll motivate desktops to adopt it as well as the default?

Unfortunately we don't really have a good way out of this one. They
were disabled because the non-modifier path on Intel can be linear or
X-tiled (row-major), whereas the modifier path enables Y-tiled
(column-major) and compressed layouts. Y-tiled is the most efficient,
but Intel could only spare about six transistors for the global FIFO
shared between all their plane fetch engines, and Y-tiled blows
straight through it. Both X and Shell would thus fail to enable high
resolutions or many heads (2x 4K is enough even on modern platforms
IIRC), so they just turned modifiers off.

The best solution would be to do a global atomic_check across all
outputs and just blacklist modifiers until you find one which works,
but Shell doesn't yet have that code, and -modesetting ... well,
no-one's volunteered to do that yet, or probably ever.

Cheers,
Daniel
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel


[PATCH] drm/dp_mst: Don't return error code when crtc is null

2020-08-14 Thread Bhawanpreet Lakha
[Why]
In certain cases the crtc can be NULL and returning -EINVAL causes
atomic check to fail when it shouln't. This leads to valid
configurations failing because atomic check fails.

[How]
Don't early return if crtc is null

Signed-off-by: Bhawanpreet Lakha 
---
 drivers/gpu/drm/drm_dp_mst_topology.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c 
b/drivers/gpu/drm/drm_dp_mst_topology.c
index 70c4b7afed12..bc90a1485699 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -5037,8 +5037,8 @@ int drm_dp_mst_add_affected_dsc_crtcs(struct 
drm_atomic_state *state, struct drm
 
crtc = conn_state->crtc;
 
-   if (WARN_ON(!crtc))
-   return -EINVAL;
+   if (!crtc)
+   continue;
 
if (!drm_dp_mst_dsc_aux_for_port(pos->port))
continue;
-- 
2.17.1

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel


Re: [PATCH] dt-bindings: Whitespace clean-ups in schema files

2020-08-14 Thread Luca Ceresoli
Hi,

On 14/08/20 16:51, Rob Herring wrote:
> On Thu, Aug 13, 2020 at 4:31 AM Luca Ceresoli  wrote:
>>
>> Hi Rob,
>>
>> On 12/08/20 22:36, Rob Herring wrote:
>>> Clean-up incorrect indentation, extra spaces, long lines, and missing
>>> EOF newline in schema files. Most of the clean-ups are for list
>>> indentation which should always be 2 spaces more than the preceding
>>> keyword.
>>>
>>> Found with yamllint (which I plan to integrate into the checks).
>>
>> [...]
>>
>>> diff --git a/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml 
>>> b/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
>>> index 3d4e1685cc55..28c6461b9a9a 100644
>>> --- a/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
>>> +++ b/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
>>> @@ -95,10 +95,10 @@ allOf:
>>># Devices without builtin crystal
>>>properties:
>>>  clock-names:
>>> -minItems: 1
>>> -maxItems: 2
>>> -items:
>>> -  enum: [ xin, clkin ]
>>> +  minItems: 1
>>> +  maxItems: 2
>>> +  items:
>>> +enum: [ xin, clkin ]
>>>  clocks:
>>>minItems: 1
>>>maxItems: 2
>>
>> Thanks for noticing, LGTM.
>>
>> [...]
>>
>>> diff --git 
>>> a/Documentation/devicetree/bindings/input/touchscreen/touchscreen.yaml 
>>> b/Documentation/devicetree/bindings/input/touchscreen/touchscreen.yaml
>>> index d7dac16a3960..36dc7b56a453 100644
>>> --- a/Documentation/devicetree/bindings/input/touchscreen/touchscreen.yaml
>>> +++ b/Documentation/devicetree/bindings/input/touchscreen/touchscreen.yaml
>>> @@ -33,8 +33,8 @@ properties:
>>>  $ref: /schemas/types.yaml#/definitions/uint32
>>>
>>>touchscreen-min-pressure:
>>> -description: minimum pressure on the touchscreen to be achieved in 
>>> order for the
>>> - touchscreen driver to report a touch event.
>>> +description: minimum pressure on the touchscreen to be achieved in 
>>> order
>>> +  for the touchscreen driver to report a touch event.
>>
>> Out of personal taste, I find the original layout more pleasant and
>> readable. This third option is also good, especially for long descriptions:
>>
>>   description:
>> minimum pressure on the touchscreen to be achieved in order for the
>> touchscreen driver to report a touch event.
>>
>> At first glance yamllint seems to support exactly these two by default:
>>
>>> With indentation: {spaces: 4, check-multi-line-strings: true}
> 
> Turning on check-multi-line-strings results in 10K+ warnings, so no.
> 
> The other issue is the style ruamel.yaml wants to write out is as the
> patch does above. This matters when doing some scripted
> transformations where we read in the files and write them back out. I
> can somewhat work around that by first doing a pass with no changes
> and then another pass with the actual changes, but that's completely
> scriptable. Hopefully, ruamel learns to preserve the style better.

Kind of sad, but I understand the reason as far as my understanding of
the yaml world allows. Thanks for the explanation.

[For idt,versaclock5.yaml, plus an overview of whole patch]
Reviewed-by: Luca Ceresoli 

-- 
Luca
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel


Re: [git pull] drm for 5.8-rc1

2020-08-14 Thread Thierry Reding
On Fri, Aug 14, 2020 at 06:12:56PM +0200, Karol Herbst wrote:
> On Fri, Aug 14, 2020 at 6:06 PM Thierry Reding  
> wrote:
> >
> > On Fri, Aug 14, 2020 at 05:40:34PM +0200, Karol Herbst wrote:
> > > On Fri, Aug 14, 2020 at 5:35 PM Thierry Reding  
> > > wrote:
> > > >
> > > > On Fri, Aug 14, 2020 at 04:44:43PM +0200, Karol Herbst wrote:
> > > > > On Fri, Aug 14, 2020 at 4:05 PM Thierry Reding 
> > > > >  wrote:
> > > > > >
> > > > > > On Thu, Aug 13, 2020 at 10:45:34AM -0700, James Jones wrote:
> > > > > > > I'll defer to Thierry, but I think that may be by design.  Tegra 
> > > > > > > format
> > > > > > > modifiers were added to get things like this working in the first 
> > > > > > > place,
> > > > > > > right?  It's not a regression, is it?
> > > > > >
> > > > > > I recall that things used to work with or without modifiers at some
> > > > > > point. That was basically the point of the "pitch-linear by default"
> > > > > > patch, see:
> > > > > >
> > > > > > 
> > > > > > https://gitlab.freedesktop.org/mesa/mesa/-/commit/9603d81df05105857b676f20dff964ef3ab0ecff
> > > > > >
> > > > > > If we don't force pitch-linear for cases where we don't have 
> > > > > > modifiers,
> > > > > > there's no way we can properly display these as framebuffers 
> > > > > > because we
> > > > > > lack the modifier information at the kernel level.
> > > > > >
> > > > >
> > > > > I suspect that's related to
> > > > > https://gitlab.freedesktop.org/mesa/mesa/commit/c56fe4118a2dd991ff1b2a532c0f234eddd435e9
> > > >
> > > > But we're checking the the PIPE_USAGE_SCANOUT flag specifically for that
> > > > type of case, so this should work. On the other hand, it does sound like
> > > > that logic might actually work, but for some reason Nouveau then ends up
> > > > allocating a linear render buffer and a tiled depth buffer, and that's
> > > > the combination that doesn't work. Did I understand that correctly?
> > > >
> > > > So it sounds like there's a few options:
> > > >
> > > >   a) Make Nouveau render without a depth buffer in these cases (not sure
> > > >  if that's even possible).
> > > >
> > > >   b) Allocate a linear buffer *in addition* to the normal buffer that
> > > >  Nouveau allocates (with whatever it selects as the default layout)
> > > >  and then blit from the tiled buffer to the linear buffer everytime
> > > >  we need to. I think that'd be basically reproducing the code that
> > > >  is controlled by the DRI_PRIME environment variable.
> > > >
> > >
> > > yeah, I think Ilia suggested something like that a long time ago as well.
> > >
> > > >   c) Accept that modifiers are the way to go and rely on them for proper
> > > >  functionality. This is obviously a really bad solution because not
> > > >  everyone has transitioned to framebuffer modifiers yet. On the
> > > >  other hand, this is precisely the kind of use-case that framebuffer
> > > >  modifiers were meant to address, so it isn't all that far-fetched
> > > >  to make them a requirement.
> > > >
> > >
> > > the problem with that is, that by default we don't have modifier aware
> > > desktops. gnome-shell by default and no Xorg release.
> > >
> > > > None of those are really great options... does anyone have any other
> > > > ideas?
> > > >
> > >
> > > What I am wondering about is why with my patch it simply works in X,
> > > but that could be a mere coincidence.
> >
> > So I was testing your revert with Weston and that "works", but only as
> > in "it doesn't crash". As expected, since there's now a mismatch between
> > what layout Nouveau assumes and the pitch-linear buffer that modesetting
> > assumes it got, it'll now be completely scrambled. That said, even
> > without the revert I can't seem to be able to make Weston work without
> > modifiers support.
> >
> 
> yeah, that does seem to reproduce what I noticed with
> gnome-shell/wayland as well.

Oh, here's the patch I used to disable framebuffer modifiers in Weston,
in case anyone is interested:

--- >8 ---
diff --git a/libweston/backend-drm/kms.c b/libweston/backend-drm/kms.c
index f5215f20d694..889b2444b99f 100644
--- a/libweston/backend-drm/kms.c
+++ b/libweston/backend-drm/kms.c
@@ -1480,6 +1480,10 @@ init_kms_caps(struct drm_backend *b)
else
b->fb_modifiers = 0;
 
+   if (getenv("WESTON_DISABLE_FB_MODIFIERS")) {
+   b->fb_modifiers = 0;
+   }
+
/*
 * KMS support for hardware planes cannot properly synchronize
 * without nuclear page flip. Without nuclear/atomic, hw plane
--- >8 ---

I suspect that the reason why this works in X but not in Wayland is
because X passes the right usage flags, whereas Weston may not. But I'll
have to investigate more in order to be sure.

In either case, it does seem to me like b) above would be the best
solution for the legacy case (i.e. where we don't have modifiers). It's
not going to be great in terms of performance because of the extra copy,
but I thought I 

Re: [RFC][PATCH v2 2/2] dma-heap: Add a system-uncached heap

2020-08-14 Thread Ezequiel Garcia
Hi John,

Thanks for the patch.

On Fri, 14 Aug 2020 at 03:25, John Stultz  wrote:
>
> This adds a heap that allocates non-contiguous buffers that are
> marked as writecombined, so they are not cached by the CPU.
>

What's the rationale for exposing the memory
attribute as a new heap, instead of just introducing flags?

I guess this calls for some guidelines on what situations
call for a separate heap, and when it's just a modifier flag.

Thanks!
Ezequiel

> This is useful, as most graphics buffers are usually not touched
> by the CPU or only written into once by the CPU. So when mapping
> the buffer over and over between devices, we can skip the CPU
> syncing, which saves a lot of cache management overhead, greatly
> improving performance.
>
> For folk using ION, there was a ION_FLAG_CACHED flag, which
> signaled if the returned buffer should be CPU cacheable or not.
> With DMA-BUF heaps, we have no such flag, and by default the
> current heaps (system and cma) produce CPU cachable buffers.
> So for folks transitioning from ION to DMA-BUF Heaps, this fills
> in some of that missing functionality.
>
> This does have a few "ugly" bits that were required to get
> the buffer properly flushed out initially which I'd like to
> improve. So feedback would be very welcome!
>
> Many thanks to Liam Mark for his help to get this working.
>
> Cc: Sumit Semwal 
> Cc: Andrew F. Davis 
> Cc: Benjamin Gaignard 
> Cc: Liam Mark 
> Cc: Laura Abbott 
> Cc: Brian Starkey 
> Cc: Hridya Valsaraju 
> Cc: Robin Murphy 
> Cc: linux-me...@vger.kernel.org
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: John Stultz 
> ---
> v2:
> * Fix build issue on sh reported-by: kernel test robot 
> * Rework to use for_each_sgtable_sg(), dma_map_sgtable(), and
>   for_each_sg_page() along with numerous other cleanups suggested
>   by Robin Murphy
> ---
>  drivers/dma-buf/heaps/Kconfig|  10 +
>  drivers/dma-buf/heaps/Makefile   |   1 +
>  drivers/dma-buf/heaps/system_uncached_heap.c | 371 +++
>  3 files changed, 382 insertions(+)
>  create mode 100644 drivers/dma-buf/heaps/system_uncached_heap.c
>
> diff --git a/drivers/dma-buf/heaps/Kconfig b/drivers/dma-buf/heaps/Kconfig
> index a5eef06c4226..420b0ed0a512 100644
> --- a/drivers/dma-buf/heaps/Kconfig
> +++ b/drivers/dma-buf/heaps/Kconfig
> @@ -5,6 +5,16 @@ config DMABUF_HEAPS_SYSTEM
>   Choose this option to enable the system dmabuf heap. The system heap
>   is backed by pages from the buddy allocator. If in doubt, say Y.
>
> +config DMABUF_HEAPS_SYSTEM_UNCACHED
> +   bool "DMA-BUF Uncached System Heap"
> +   depends on DMABUF_HEAPS
> +   help
> + Choose this option to enable the uncached system dmabuf heap. This
> + heap is backed by pages from the buddy allocator, but pages are 
> setup
> + for write combining. This avoids cache management overhead, and can
> + be faster if pages are mostly untouched by the cpu.  If in doubt,
> + say Y.
> +
>  config DMABUF_HEAPS_CMA
> bool "DMA-BUF CMA Heap"
> depends on DMABUF_HEAPS && DMA_CMA
> diff --git a/drivers/dma-buf/heaps/Makefile b/drivers/dma-buf/heaps/Makefile
> index 6e54cdec3da0..085685ec478f 100644
> --- a/drivers/dma-buf/heaps/Makefile
> +++ b/drivers/dma-buf/heaps/Makefile
> @@ -1,4 +1,5 @@
>  # SPDX-License-Identifier: GPL-2.0
>  obj-y  += heap-helpers.o
>  obj-$(CONFIG_DMABUF_HEAPS_SYSTEM)  += system_heap.o
> +obj-$(CONFIG_DMABUF_HEAPS_SYSTEM_UNCACHED) += system_uncached_heap.o
>  obj-$(CONFIG_DMABUF_HEAPS_CMA) += cma_heap.o
> diff --git a/drivers/dma-buf/heaps/system_uncached_heap.c 
> b/drivers/dma-buf/heaps/system_uncached_heap.c
> new file mode 100644
> index ..3b8e699bcae7
> --- /dev/null
> +++ b/drivers/dma-buf/heaps/system_uncached_heap.c
> @@ -0,0 +1,371 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Uncached System DMA-Heap exporter
> + *
> + * Copyright (C) 2020 Linaro Ltd.
> + *
> + * Based off of Andrew Davis' SRAM heap:
> + * Copyright (C) 2019 Texas Instruments Incorporated - http://www.ti.com/
> + * Andrew F. Davis 
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +struct uncached_heap {
> +   struct dma_heap *heap;
> +};
> +
> +struct uncached_heap_buffer {
> +   struct dma_heap *heap;
> +   struct list_head attachments;
> +   struct mutex lock;
> +   unsigned long len;
> +   struct sg_table sg_table;
> +   int vmap_cnt;
> +   void *vaddr;
> +};
> +
> +struct dma_heap_attachment {
> +   struct device *dev;
> +   struct sg_table *table;
> +   struct list_head list;
> +};
> +
> +static struct sg_table *dup_sg_table(struct sg_table *table)
> +{
> +   struct sg_table *new_table;
> +   int ret, i;
> +   struct scatterlist *sg, *new_sg;
> +
> +   new_table = 

Re: [git pull] drm for 5.8-rc1

2020-08-14 Thread Karol Herbst
On Fri, Aug 14, 2020 at 6:06 PM Thierry Reding  wrote:
>
> On Fri, Aug 14, 2020 at 05:40:34PM +0200, Karol Herbst wrote:
> > On Fri, Aug 14, 2020 at 5:35 PM Thierry Reding  
> > wrote:
> > >
> > > On Fri, Aug 14, 2020 at 04:44:43PM +0200, Karol Herbst wrote:
> > > > On Fri, Aug 14, 2020 at 4:05 PM Thierry Reding 
> > > >  wrote:
> > > > >
> > > > > On Thu, Aug 13, 2020 at 10:45:34AM -0700, James Jones wrote:
> > > > > > I'll defer to Thierry, but I think that may be by design.  Tegra 
> > > > > > format
> > > > > > modifiers were added to get things like this working in the first 
> > > > > > place,
> > > > > > right?  It's not a regression, is it?
> > > > >
> > > > > I recall that things used to work with or without modifiers at some
> > > > > point. That was basically the point of the "pitch-linear by default"
> > > > > patch, see:
> > > > >
> > > > > 
> > > > > https://gitlab.freedesktop.org/mesa/mesa/-/commit/9603d81df05105857b676f20dff964ef3ab0ecff
> > > > >
> > > > > If we don't force pitch-linear for cases where we don't have 
> > > > > modifiers,
> > > > > there's no way we can properly display these as framebuffers because 
> > > > > we
> > > > > lack the modifier information at the kernel level.
> > > > >
> > > >
> > > > I suspect that's related to
> > > > https://gitlab.freedesktop.org/mesa/mesa/commit/c56fe4118a2dd991ff1b2a532c0f234eddd435e9
> > >
> > > But we're checking the the PIPE_USAGE_SCANOUT flag specifically for that
> > > type of case, so this should work. On the other hand, it does sound like
> > > that logic might actually work, but for some reason Nouveau then ends up
> > > allocating a linear render buffer and a tiled depth buffer, and that's
> > > the combination that doesn't work. Did I understand that correctly?
> > >
> > > So it sounds like there's a few options:
> > >
> > >   a) Make Nouveau render without a depth buffer in these cases (not sure
> > >  if that's even possible).
> > >
> > >   b) Allocate a linear buffer *in addition* to the normal buffer that
> > >  Nouveau allocates (with whatever it selects as the default layout)
> > >  and then blit from the tiled buffer to the linear buffer everytime
> > >  we need to. I think that'd be basically reproducing the code that
> > >  is controlled by the DRI_PRIME environment variable.
> > >
> >
> > yeah, I think Ilia suggested something like that a long time ago as well.
> >
> > >   c) Accept that modifiers are the way to go and rely on them for proper
> > >  functionality. This is obviously a really bad solution because not
> > >  everyone has transitioned to framebuffer modifiers yet. On the
> > >  other hand, this is precisely the kind of use-case that framebuffer
> > >  modifiers were meant to address, so it isn't all that far-fetched
> > >  to make them a requirement.
> > >
> >
> > the problem with that is, that by default we don't have modifier aware
> > desktops. gnome-shell by default and no Xorg release.
> >
> > > None of those are really great options... does anyone have any other
> > > ideas?
> > >
> >
> > What I am wondering about is why with my patch it simply works in X,
> > but that could be a mere coincidence.
>
> So I was testing your revert with Weston and that "works", but only as
> in "it doesn't crash". As expected, since there's now a mismatch between
> what layout Nouveau assumes and the pitch-linear buffer that modesetting
> assumes it got, it'll now be completely scrambled. That said, even
> without the revert I can't seem to be able to make Weston work without
> modifiers support.
>

yeah, that does seem to reproduce what I noticed with
gnome-shell/wayland as well.

> I'll have to investigate some more why that's not working. Perhaps it
> doesn't pass the correct usage flags?
>
> Thierry

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel


Re: [git pull] drm for 5.8-rc1

2020-08-14 Thread Thierry Reding
On Fri, Aug 14, 2020 at 05:40:34PM +0200, Karol Herbst wrote:
> On Fri, Aug 14, 2020 at 5:35 PM Thierry Reding  
> wrote:
> >
> > On Fri, Aug 14, 2020 at 04:44:43PM +0200, Karol Herbst wrote:
> > > On Fri, Aug 14, 2020 at 4:05 PM Thierry Reding  
> > > wrote:
> > > >
> > > > On Thu, Aug 13, 2020 at 10:45:34AM -0700, James Jones wrote:
> > > > > I'll defer to Thierry, but I think that may be by design.  Tegra 
> > > > > format
> > > > > modifiers were added to get things like this working in the first 
> > > > > place,
> > > > > right?  It's not a regression, is it?
> > > >
> > > > I recall that things used to work with or without modifiers at some
> > > > point. That was basically the point of the "pitch-linear by default"
> > > > patch, see:
> > > >
> > > > 
> > > > https://gitlab.freedesktop.org/mesa/mesa/-/commit/9603d81df05105857b676f20dff964ef3ab0ecff
> > > >
> > > > If we don't force pitch-linear for cases where we don't have modifiers,
> > > > there's no way we can properly display these as framebuffers because we
> > > > lack the modifier information at the kernel level.
> > > >
> > >
> > > I suspect that's related to
> > > https://gitlab.freedesktop.org/mesa/mesa/commit/c56fe4118a2dd991ff1b2a532c0f234eddd435e9
> >
> > But we're checking the the PIPE_USAGE_SCANOUT flag specifically for that
> > type of case, so this should work. On the other hand, it does sound like
> > that logic might actually work, but for some reason Nouveau then ends up
> > allocating a linear render buffer and a tiled depth buffer, and that's
> > the combination that doesn't work. Did I understand that correctly?
> >
> > So it sounds like there's a few options:
> >
> >   a) Make Nouveau render without a depth buffer in these cases (not sure
> >  if that's even possible).
> >
> >   b) Allocate a linear buffer *in addition* to the normal buffer that
> >  Nouveau allocates (with whatever it selects as the default layout)
> >  and then blit from the tiled buffer to the linear buffer everytime
> >  we need to. I think that'd be basically reproducing the code that
> >  is controlled by the DRI_PRIME environment variable.
> >
> 
> yeah, I think Ilia suggested something like that a long time ago as well.
> 
> >   c) Accept that modifiers are the way to go and rely on them for proper
> >  functionality. This is obviously a really bad solution because not
> >  everyone has transitioned to framebuffer modifiers yet. On the
> >  other hand, this is precisely the kind of use-case that framebuffer
> >  modifiers were meant to address, so it isn't all that far-fetched
> >  to make them a requirement.
> >
> 
> the problem with that is, that by default we don't have modifier aware
> desktops. gnome-shell by default and no Xorg release.
> 
> > None of those are really great options... does anyone have any other
> > ideas?
> >
> 
> What I am wondering about is why with my patch it simply works in X,
> but that could be a mere coincidence.

So I was testing your revert with Weston and that "works", but only as
in "it doesn't crash". As expected, since there's now a mismatch between
what layout Nouveau assumes and the pitch-linear buffer that modesetting
assumes it got, it'll now be completely scrambled. That said, even
without the revert I can't seem to be able to make Weston work without
modifiers support.

I'll have to investigate some more why that's not working. Perhaps it
doesn't pass the correct usage flags?

Thierry


signature.asc
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel


Re: [git pull] drm for 5.8-rc1

2020-08-14 Thread Karol Herbst
On Fri, Aug 14, 2020 at 5:24 PM Thierry Reding  wrote:
>
> On Fri, Aug 14, 2020 at 04:45:39PM +0200, Karol Herbst wrote:
> > On Fri, Aug 14, 2020 at 4:08 PM Thierry Reding  
> > wrote:
> > >
> > > On Thu, Aug 13, 2020 at 05:39:39PM +0200, Karol Herbst wrote:
> > > > btw, I just noticed that wayland with gnome-shell is totally busted.
> > > > With this MR it at least displays something, but without it doesn't
> > > > work at all.
> > >
> > > Interesting, one of my typical test cases is to run Weston with a couple
> > > of test programs (like weston-simple-egl). Those usually work. I'll go
> > > run a few more tests to see where we are.
> > >
> > > To clarify, is this gnome-shell/wayland issue happening with Mesa's
> > > mainline, or with James' patches already applied?
> > >
> >
> > mainline. It does work for me on weston, but that's because weston is
> > always modifier aware afaik. For gnome-shell/wayland we have to enable
> > it to make it work.
>
> For some reason I can't get my mouse to work in Weston and it seems like
> that's the only way to start a terminal... But sounds like that wouldn't
> be any good anyway since it's different from that use-case. Apart from
> building gnome-shell, which I recall has a large number of dependencies,
> are you aware of another use-case that would allow testing the code
> paths with no modifiers?
>

with the newest l4t releases you can just copy whatever distribution
provides filesystem tarballs and generate images yourself. But yeah..
this is still a bit messy to do and I don't have a script which just
works without having to go through other steps first :/ Don't know
what your system is based on, but the fedora 32 images should more or
less work (unless you need your own kernel and stuff)



> Sounds like perhaps that would be interesting to add to Weston as a knob
> to test these somewhat legacy paths.
>
> Thierry

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel


Re: [git pull] drm for 5.8-rc1

2020-08-14 Thread Karol Herbst
On Fri, Aug 14, 2020 at 5:35 PM Thierry Reding  wrote:
>
> On Fri, Aug 14, 2020 at 04:44:43PM +0200, Karol Herbst wrote:
> > On Fri, Aug 14, 2020 at 4:05 PM Thierry Reding  
> > wrote:
> > >
> > > On Thu, Aug 13, 2020 at 10:45:34AM -0700, James Jones wrote:
> > > > I'll defer to Thierry, but I think that may be by design.  Tegra format
> > > > modifiers were added to get things like this working in the first place,
> > > > right?  It's not a regression, is it?
> > >
> > > I recall that things used to work with or without modifiers at some
> > > point. That was basically the point of the "pitch-linear by default"
> > > patch, see:
> > >
> > > 
> > > https://gitlab.freedesktop.org/mesa/mesa/-/commit/9603d81df05105857b676f20dff964ef3ab0ecff
> > >
> > > If we don't force pitch-linear for cases where we don't have modifiers,
> > > there's no way we can properly display these as framebuffers because we
> > > lack the modifier information at the kernel level.
> > >
> >
> > I suspect that's related to
> > https://gitlab.freedesktop.org/mesa/mesa/commit/c56fe4118a2dd991ff1b2a532c0f234eddd435e9
>
> But we're checking the the PIPE_USAGE_SCANOUT flag specifically for that
> type of case, so this should work. On the other hand, it does sound like
> that logic might actually work, but for some reason Nouveau then ends up
> allocating a linear render buffer and a tiled depth buffer, and that's
> the combination that doesn't work. Did I understand that correctly?
>
> So it sounds like there's a few options:
>
>   a) Make Nouveau render without a depth buffer in these cases (not sure
>  if that's even possible).
>
>   b) Allocate a linear buffer *in addition* to the normal buffer that
>  Nouveau allocates (with whatever it selects as the default layout)
>  and then blit from the tiled buffer to the linear buffer everytime
>  we need to. I think that'd be basically reproducing the code that
>  is controlled by the DRI_PRIME environment variable.
>

yeah, I think Ilia suggested something like that a long time ago as well.

>   c) Accept that modifiers are the way to go and rely on them for proper
>  functionality. This is obviously a really bad solution because not
>  everyone has transitioned to framebuffer modifiers yet. On the
>  other hand, this is precisely the kind of use-case that framebuffer
>  modifiers were meant to address, so it isn't all that far-fetched
>  to make them a requirement.
>

the problem with that is, that by default we don't have modifier aware
desktops. gnome-shell by default and no Xorg release.

> None of those are really great options... does anyone have any other
> ideas?
>

What I am wondering about is why with my patch it simply works in X,
but that could be a mere coincidence.

> Thierry

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel


Re: [git pull] drm for 5.8-rc1

2020-08-14 Thread Thierry Reding
On Fri, Aug 14, 2020 at 04:44:43PM +0200, Karol Herbst wrote:
> On Fri, Aug 14, 2020 at 4:05 PM Thierry Reding  
> wrote:
> >
> > On Thu, Aug 13, 2020 at 10:45:34AM -0700, James Jones wrote:
> > > I'll defer to Thierry, but I think that may be by design.  Tegra format
> > > modifiers were added to get things like this working in the first place,
> > > right?  It's not a regression, is it?
> >
> > I recall that things used to work with or without modifiers at some
> > point. That was basically the point of the "pitch-linear by default"
> > patch, see:
> >
> > 
> > https://gitlab.freedesktop.org/mesa/mesa/-/commit/9603d81df05105857b676f20dff964ef3ab0ecff
> >
> > If we don't force pitch-linear for cases where we don't have modifiers,
> > there's no way we can properly display these as framebuffers because we
> > lack the modifier information at the kernel level.
> >
> 
> I suspect that's related to
> https://gitlab.freedesktop.org/mesa/mesa/commit/c56fe4118a2dd991ff1b2a532c0f234eddd435e9

But we're checking the the PIPE_USAGE_SCANOUT flag specifically for that
type of case, so this should work. On the other hand, it does sound like
that logic might actually work, but for some reason Nouveau then ends up
allocating a linear render buffer and a tiled depth buffer, and that's
the combination that doesn't work. Did I understand that correctly?

So it sounds like there's a few options:

  a) Make Nouveau render without a depth buffer in these cases (not sure
 if that's even possible).

  b) Allocate a linear buffer *in addition* to the normal buffer that
 Nouveau allocates (with whatever it selects as the default layout)
 and then blit from the tiled buffer to the linear buffer everytime
 we need to. I think that'd be basically reproducing the code that
 is controlled by the DRI_PRIME environment variable.

  c) Accept that modifiers are the way to go and rely on them for proper
 functionality. This is obviously a really bad solution because not
 everyone has transitioned to framebuffer modifiers yet. On the
 other hand, this is precisely the kind of use-case that framebuffer
 modifiers were meant to address, so it isn't all that far-fetched
 to make them a requirement.

None of those are really great options... does anyone have any other
ideas?

Thierry


signature.asc
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel


Re: [PATCH 2/4] drm/panel: s6e63m0: Add DSI transport

2020-08-14 Thread Stephan Gerhold
On Sun, Aug 09, 2020 at 11:51:02PM +0200, Linus Walleij wrote:
> This makes it possible to use the s6e63m0 panel with a
> DSI host, such as in the Samsung GT-I8190 (Golden) mobile
> phone.
> 
> Cc: Stephan Gerhold 
> Cc: Paweł Chmiel 
> Signed-off-by: Linus Walleij 
> ---
>  drivers/gpu/drm/panel/Kconfig |   8 ++
>  drivers/gpu/drm/panel/Makefile|   1 +
>  .../gpu/drm/panel/panel-samsung-s6e63m0-dsi.c | 128 ++
>  .../gpu/drm/panel/panel-samsung-s6e63m0-spi.c |   2 +-
>  drivers/gpu/drm/panel/panel-samsung-s6e63m0.c |   4 +-
>  drivers/gpu/drm/panel/panel-samsung-s6e63m0.h |   4 +-
>  6 files changed, 144 insertions(+), 3 deletions(-)
>  create mode 100644 drivers/gpu/drm/panel/panel-samsung-s6e63m0-dsi.c
> 
> diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
> index 96e1548e475f..731e84c5a13b 100644
> --- a/drivers/gpu/drm/panel/Kconfig
> +++ b/drivers/gpu/drm/panel/Kconfig
> @@ -343,6 +343,14 @@ config DRM_PANEL_SAMSUNG_S6E63M0_SPI
> Say Y here if you want to be able to access the Samsung
> S6E63M0 panel using SPI.
>  
> +config DRM_PANEL_SAMSUNG_S6E63M0_DSI
> + tristate "Samsung S6E63M0 RGB DSI interface"
> + depends on DRM_MIPI_DSI
> + depends on DRM_PANEL_SAMSUNG_S6E63M0
> + help
> +   Say Y here if you want to be able to access the Samsung
> +   S6E63M0 panel using DSI.
> +
>  config DRM_PANEL_SAMSUNG_S6E88A0_AMS452EF01
>   tristate "Samsung AMS452EF01 panel with S6E88A0 DSI video mode 
> controller"
>   depends on OF
> diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
> index 9cf71adfa794..14212cae3c29 100644
> --- a/drivers/gpu/drm/panel/Makefile
> +++ b/drivers/gpu/drm/panel/Makefile
> @@ -35,6 +35,7 @@ obj-$(CONFIG_DRM_PANEL_SAMSUNG_S6E3HA2) += 
> panel-samsung-s6e3ha2.o
>  obj-$(CONFIG_DRM_PANEL_SAMSUNG_S6E63J0X03) += panel-samsung-s6e63j0x03.o
>  obj-$(CONFIG_DRM_PANEL_SAMSUNG_S6E63M0) += panel-samsung-s6e63m0.o
>  obj-$(CONFIG_DRM_PANEL_SAMSUNG_S6E63M0_SPI) += panel-samsung-s6e63m0-spi.o
> +obj-$(CONFIG_DRM_PANEL_SAMSUNG_S6E63M0_DSI) += panel-samsung-s6e63m0-dsi.o
>  obj-$(CONFIG_DRM_PANEL_SAMSUNG_S6E88A0_AMS452EF01) += 
> panel-samsung-s6e88a0-ams452ef01.o
>  obj-$(CONFIG_DRM_PANEL_SAMSUNG_S6E8AA0) += panel-samsung-s6e8aa0.o
>  obj-$(CONFIG_DRM_PANEL_SEIKO_43WVF1G) += panel-seiko-43wvf1g.o
> diff --git a/drivers/gpu/drm/panel/panel-samsung-s6e63m0-dsi.c 
> b/drivers/gpu/drm/panel/panel-samsung-s6e63m0-dsi.c
> new file mode 100644
> index ..f4927a6ce26d
> --- /dev/null
> +++ b/drivers/gpu/drm/panel/panel-samsung-s6e63m0-dsi.c
> @@ -0,0 +1,128 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * DSI interface to the Samsung S6E63M0 panel.
> + * (C) 2019 Linus Walleij
> + */
> +
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +#include 
> +
> +#include "panel-samsung-s6e63m0.h"
> +
> +#define MCS_GLOBAL_PARAM 0xb0
> +#define S6E63M0_DSI_MAX_CHUNK15 /* CMD + 15 bytes max */
> +
> +static int s6e63m0_dsi_dcs_write(struct device *dev, const u8 *data, size_t 
> len)
> +{
> + struct mipi_dsi_device *dsi = to_mipi_dsi_device(dev);
> + const u8 *seqp = data;
> + u8 cmd;
> + u8 cmdwritten;
> + int remain;
> + int chunk;
> + int ret;
> +
> + DRM_DEV_INFO(dev, "DSI writing dcs seq: %*ph\n", (int)len, data);
> +

We should probably remove these or demote them to debug.
It's quite verbose.

> + /* Pick out and skip past the DCS command */
> + cmd = *seqp;
> + seqp++;
> + cmdwritten = 0;
> + remain = len - 1;
> + chunk = remain;
> +
> + /* Send max S6E63M0_DSI_MAX_CHUNK bytes at a time */
> + if (chunk > S6E63M0_DSI_MAX_CHUNK)
> + chunk = S6E63M0_DSI_MAX_CHUNK;
> + ret = mipi_dsi_dcs_write(dsi, cmd, seqp, chunk);
> + if (ret < 0) {
> + DRM_DEV_ERROR(dev,
> +   "error sending DCS command seq cmd %02x\n",
> +   cmd);
> + return ret;
> + }
> + cmdwritten += chunk;
> + seqp += chunk;
> +
> + while (cmdwritten < remain) {
> + chunk = remain - cmdwritten;
> + if (chunk > S6E63M0_DSI_MAX_CHUNK)
> + chunk = S6E63M0_DSI_MAX_CHUNK;
> + ret = mipi_dsi_dcs_write(dsi, MCS_GLOBAL_PARAM, , 1);
> + if (ret < 0) {
> + DRM_DEV_ERROR(dev,
> +   "error sending CMD %02x global param 
> %02x\n",
> +   cmd, cmdwritten);
> + return ret;
> + }
> + ret = mipi_dsi_dcs_write(dsi, cmd, seqp, chunk);
> + if (ret < 0) {
> + DRM_DEV_ERROR(dev,
> +   "error sending CMD %02x chunk\n",
> +   cmd);
> + return ret;
> + }
> + cmdwritten += chunk;
> + seqp 

Re: [git pull] drm for 5.8-rc1

2020-08-14 Thread Thierry Reding
On Fri, Aug 14, 2020 at 04:45:39PM +0200, Karol Herbst wrote:
> On Fri, Aug 14, 2020 at 4:08 PM Thierry Reding  
> wrote:
> >
> > On Thu, Aug 13, 2020 at 05:39:39PM +0200, Karol Herbst wrote:
> > > btw, I just noticed that wayland with gnome-shell is totally busted.
> > > With this MR it at least displays something, but without it doesn't
> > > work at all.
> >
> > Interesting, one of my typical test cases is to run Weston with a couple
> > of test programs (like weston-simple-egl). Those usually work. I'll go
> > run a few more tests to see where we are.
> >
> > To clarify, is this gnome-shell/wayland issue happening with Mesa's
> > mainline, or with James' patches already applied?
> >
> 
> mainline. It does work for me on weston, but that's because weston is
> always modifier aware afaik. For gnome-shell/wayland we have to enable
> it to make it work.

For some reason I can't get my mouse to work in Weston and it seems like
that's the only way to start a terminal... But sounds like that wouldn't
be any good anyway since it's different from that use-case. Apart from
building gnome-shell, which I recall has a large number of dependencies,
are you aware of another use-case that would allow testing the code
paths with no modifiers?

Sounds like perhaps that would be interesting to add to Weston as a knob
to test these somewhat legacy paths.

Thierry


signature.asc
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel


Re: [PATCH] drm/dp_mst: Add ddc i2c device links for DP MST connectors

2020-08-14 Thread Imre Deak
On Wed, Jul 29, 2020 at 04:15:28PM +1000, Sam McNally wrote:
> As of commit d8bd15b37d32 ("drm/dp_mst: Fix the DDC I2C device
> registration of an MST port"), DP MST DDC I2C devices are consistently
> parented to the underlying DRM device, making it challenging to
> associate the ddc i2c device with its connector from userspace.

I can't see how was it less challenging before the commit. There is no
guarantee for a CSN message which was the only way for the i2c device to
get reparented to the connector.

> Given the need for further refactoring before the i2c devices can be
> parented to their connectors, in the meantime follow the pattern of
> commit e1a29c6c5955 ("drm: Add ddc link in sysfs created by
> drm_connector"), creating sysfs ddc links to the associated i2c device
> for MST DP connectors.
> 
> If the connector is created and registered before the i2c device, create
> the link when registering the i2c device; otherwise, create the link
> during late connector registration.
> 
> Signed-off-by: Sam McNally 
> ---
> 
>  drivers/gpu/drm/drm_dp_mst_topology.c | 29 +--
>  1 file changed, 27 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c 
> b/drivers/gpu/drm/drm_dp_mst_topology.c
> index 1ac874e4e7a1..73a2299c2faa 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -2161,11 +2161,23 @@ static void build_mst_prop_path(const struct 
> drm_dp_mst_branch *mstb,
>  int drm_dp_mst_connector_late_register(struct drm_connector *connector,
>  struct drm_dp_mst_port *port)
>  {
> + int ret;
>   DRM_DEBUG_KMS("registering %s remote bus for %s\n",
> port->aux.name, connector->kdev->kobj.name);
>  
>   port->aux.dev = connector->kdev;
> - return drm_dp_aux_register_devnode(>aux);
> + ret = drm_dp_aux_register_devnode(>aux);
> + if (ret)
> + return ret;
> +
> + if (port->pdt != DP_PEER_DEVICE_NONE &&
> + drm_dp_mst_is_end_device(port->pdt, port->mcs)) {

How can we get here when drm_dp_mst_is_end_device(port) is not true?
AFAICS that's only case where we should create a connector and an i2c
device. (IOW we don't create them for branch ports.)

> + ret = sysfs_create_link(>connector->kdev->kobj,
> + >aux.ddc.dev.kobj, "ddc");
> + if (ret)
> + drm_dp_aux_unregister_devnode(>aux);
> + }
> + return ret;
>  }
>  EXPORT_SYMBOL(drm_dp_mst_connector_late_register);
>  
> @@ -5490,6 +5502,7 @@ static int drm_dp_mst_register_i2c_bus(struct 
> drm_dp_mst_port *port)
>  {
>   struct drm_dp_aux *aux = >aux;
>   struct device *parent_dev = port->mgr->dev->dev;
> + int ret;
>  
>   aux->ddc.algo = _dp_mst_i2c_algo;
>   aux->ddc.algo_data = aux;
> @@ -5504,7 +5517,17 @@ static int drm_dp_mst_register_i2c_bus(struct 
> drm_dp_mst_port *port)
>   strlcpy(aux->ddc.name, aux->name ? aux->name : dev_name(parent_dev),
>   sizeof(aux->ddc.name));
>  
> - return i2c_add_adapter(>ddc);
> + ret = i2c_add_adapter(>ddc);
> + if (ret)
> + return ret;
> +
> + if (port->connector && port->connector->kdev) {
> + ret = sysfs_create_link(>connector->kdev->kobj,
> + >aux.ddc.dev.kobj, "ddc");
> + if (ret)
> + i2c_del_adapter(>aux.ddc);
> + }
> + return ret;
>  }
>  
>  /**
> @@ -5513,6 +5536,8 @@ static int drm_dp_mst_register_i2c_bus(struct 
> drm_dp_mst_port *port)
>   */
>  static void drm_dp_mst_unregister_i2c_bus(struct drm_dp_mst_port *port)
>  {
> + if (port->connector && port->connector->kdev)
> + sysfs_remove_link(>connector->kdev->kobj, "ddc");
>   i2c_del_adapter(>aux.ddc);
>  }
>  
> -- 
> 2.28.0.rc0.142.g3c755180ce-goog
> 
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel


Re: [PATCH] dt-bindings: Whitespace clean-ups in schema files

2020-08-14 Thread Rob Herring
On Thu, Aug 13, 2020 at 4:31 AM Luca Ceresoli  wrote:
>
> Hi Rob,
>
> On 12/08/20 22:36, Rob Herring wrote:
> > Clean-up incorrect indentation, extra spaces, long lines, and missing
> > EOF newline in schema files. Most of the clean-ups are for list
> > indentation which should always be 2 spaces more than the preceding
> > keyword.
> >
> > Found with yamllint (which I plan to integrate into the checks).
>
> [...]
>
> > diff --git a/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml 
> > b/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
> > index 3d4e1685cc55..28c6461b9a9a 100644
> > --- a/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
> > +++ b/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
> > @@ -95,10 +95,10 @@ allOf:
> ># Devices without builtin crystal
> >properties:
> >  clock-names:
> > -minItems: 1
> > -maxItems: 2
> > -items:
> > -  enum: [ xin, clkin ]
> > +  minItems: 1
> > +  maxItems: 2
> > +  items:
> > +enum: [ xin, clkin ]
> >  clocks:
> >minItems: 1
> >maxItems: 2
>
> Thanks for noticing, LGTM.
>
> [...]
>
> > diff --git 
> > a/Documentation/devicetree/bindings/input/touchscreen/touchscreen.yaml 
> > b/Documentation/devicetree/bindings/input/touchscreen/touchscreen.yaml
> > index d7dac16a3960..36dc7b56a453 100644
> > --- a/Documentation/devicetree/bindings/input/touchscreen/touchscreen.yaml
> > +++ b/Documentation/devicetree/bindings/input/touchscreen/touchscreen.yaml
> > @@ -33,8 +33,8 @@ properties:
> >  $ref: /schemas/types.yaml#/definitions/uint32
> >
> >touchscreen-min-pressure:
> > -description: minimum pressure on the touchscreen to be achieved in 
> > order for the
> > - touchscreen driver to report a touch event.
> > +description: minimum pressure on the touchscreen to be achieved in 
> > order
> > +  for the touchscreen driver to report a touch event.
>
> Out of personal taste, I find the original layout more pleasant and
> readable. This third option is also good, especially for long descriptions:
>
>   description:
> minimum pressure on the touchscreen to be achieved in order for the
> touchscreen driver to report a touch event.
>
> At first glance yamllint seems to support exactly these two by default:
>
> > With indentation: {spaces: 4, check-multi-line-strings: true}

Turning on check-multi-line-strings results in 10K+ warnings, so no.

The other issue is the style ruamel.yaml wants to write out is as the
patch does above. This matters when doing some scripted
transformations where we read in the files and write them back out. I
can somewhat work around that by first doing a pass with no changes
and then another pass with the actual changes, but that's completely
scriptable. Hopefully, ruamel learns to preserve the style better.

Rob
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel


Re: [git pull] drm for 5.8-rc1

2020-08-14 Thread Karol Herbst
On Fri, Aug 14, 2020 at 4:08 PM Thierry Reding  wrote:
>
> On Thu, Aug 13, 2020 at 05:39:39PM +0200, Karol Herbst wrote:
> > btw, I just noticed that wayland with gnome-shell is totally busted.
> > With this MR it at least displays something, but without it doesn't
> > work at all.
>
> Interesting, one of my typical test cases is to run Weston with a couple
> of test programs (like weston-simple-egl). Those usually work. I'll go
> run a few more tests to see where we are.
>
> To clarify, is this gnome-shell/wayland issue happening with Mesa's
> mainline, or with James' patches already applied?
>

mainline. It does work for me on weston, but that's because weston is
always modifier aware afaik. For gnome-shell/wayland we have to enable
it to make it work.

> Thierry

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel


Re: [git pull] drm for 5.8-rc1

2020-08-14 Thread Karol Herbst
On Fri, Aug 14, 2020 at 4:05 PM Thierry Reding  wrote:
>
> On Thu, Aug 13, 2020 at 10:45:34AM -0700, James Jones wrote:
> > I'll defer to Thierry, but I think that may be by design.  Tegra format
> > modifiers were added to get things like this working in the first place,
> > right?  It's not a regression, is it?
>
> I recall that things used to work with or without modifiers at some
> point. That was basically the point of the "pitch-linear by default"
> patch, see:
>
> 
> https://gitlab.freedesktop.org/mesa/mesa/-/commit/9603d81df05105857b676f20dff964ef3ab0ecff
>
> If we don't force pitch-linear for cases where we don't have modifiers,
> there's no way we can properly display these as framebuffers because we
> lack the modifier information at the kernel level.
>

I suspect that's related to
https://gitlab.freedesktop.org/mesa/mesa/commit/c56fe4118a2dd991ff1b2a532c0f234eddd435e9

> Thierry

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel


Re: [git pull] drm for 5.8-rc1

2020-08-14 Thread Thierry Reding
On Fri, Aug 14, 2020 at 03:59:16PM +0200, Karol Herbst wrote:
> On Fri, Aug 14, 2020 at 3:57 PM Thierry Reding  
> wrote:
> >
> > On Thu, Aug 13, 2020 at 07:48:39PM +0200, Karol Herbst wrote:
> > > On Thu, Aug 13, 2020 at 7:45 PM James Jones  wrote:
> > > >
> > > > I'll defer to Thierry, but I think that may be by design.  Tegra format
> > > > modifiers were added to get things like this working in the first place,
> > > > right?  It's not a regression, is it?
> > > >
> > >
> > > That would be slightly annoying as this would mean by design it's
> > > broken by default :/ Also, we have no Xorg release supporting
> > > modifiers anyway and it does seem to work with X 1.20.8 (which doesn't
> > > enable modifier support). And I talked with Jonas (working on mutter)
> > > about it and there were no plans to turn on modifier support by
> > > default at this point.
> >
> > I thought you said earlier that 1.20.8 didn't work and was hitting the
> > assertion?
> >
> 
> uhm, I forgot to mention that it works with the patch I wrote:
> https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/6300

Okay, good to know. Like I said in another subthread, I think we need to
make sure that this doesn't break other use-cases. I vaguely recall that
there was one specific configuration that broke without that and I think
it was when glamor was disabled because that caused X to allocate
buffers without modifiers set. Let me try if I can reproduce such a
build somehow and check if that fails again with the revert.

Thierry


signature.asc
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel


Re: [git pull] drm for 5.8-rc1

2020-08-14 Thread Thierry Reding
On Thu, Aug 13, 2020 at 05:39:39PM +0200, Karol Herbst wrote:
> btw, I just noticed that wayland with gnome-shell is totally busted.
> With this MR it at least displays something, but without it doesn't
> work at all.

Interesting, one of my typical test cases is to run Weston with a couple
of test programs (like weston-simple-egl). Those usually work. I'll go
run a few more tests to see where we are.

To clarify, is this gnome-shell/wayland issue happening with Mesa's
mainline, or with James' patches already applied?

Thierry


signature.asc
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel


Re: [git pull] drm for 5.8-rc1

2020-08-14 Thread Thierry Reding
On Thu, Aug 13, 2020 at 10:45:34AM -0700, James Jones wrote:
> I'll defer to Thierry, but I think that may be by design.  Tegra format
> modifiers were added to get things like this working in the first place,
> right?  It's not a regression, is it?

I recall that things used to work with or without modifiers at some
point. That was basically the point of the "pitch-linear by default"
patch, see:


https://gitlab.freedesktop.org/mesa/mesa/-/commit/9603d81df05105857b676f20dff964ef3ab0ecff

If we don't force pitch-linear for cases where we don't have modifiers,
there's no way we can properly display these as framebuffers because we
lack the modifier information at the kernel level.

Thierry


signature.asc
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel


Re: [PATCH v4 2/2] xen: add helpers to allocate unpopulated memory

2020-08-14 Thread Jürgen Groß

On 14.08.20 15:35, Roger Pau Monné wrote:

On Fri, Aug 14, 2020 at 02:54:38PM +0200, Jürgen Groß wrote:

On 14.08.20 14:47, Roger Pau Monné wrote:

On Fri, Aug 14, 2020 at 12:27:32PM +0200, Jürgen Groß wrote:

On 14.08.20 11:56, Roger Pau Monné wrote:

On Fri, Aug 14, 2020 at 08:29:20AM +0100, Christoph Hellwig wrote:

On Thu, Aug 13, 2020 at 09:54:20AM +0200, Roger Pau Monn?? wrote:

On Thu, Aug 13, 2020 at 08:33:37AM +0100, Christoph Hellwig wrote:

On Tue, Aug 11, 2020 at 11:44:47AM +0200, Roger Pau Monne wrote:

If enabled (because ZONE_DEVICE is supported) the usage of the new
functionality untangles Xen balloon and RAM hotplug from the usage of
unpopulated physical memory ranges to map foreign pages, which is the
correct thing to do in order to avoid mappings of foreign pages depend
on memory hotplug.


So please just select ZONE_DEVICE if this is so much better rather
than maintaining two variants.


We still need to other variant for Arm at least, so both need to be
maintained anyway, even if we force ZONE_DEVICE on x86.


Well, it still really helps reproducability if you stick to one
implementation of x86.

The alternative would be an explicit config option to opt into it,
but just getting a different implementation based on a random
kernel option is strange.


Would adding something like the chunk below to the patch be OK?

---8<---
diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig
index 018020b91baa..5f321a1319e6 100644
--- a/drivers/xen/Kconfig
+++ b/drivers/xen/Kconfig
@@ -328,7 +328,14 @@ config XEN_FRONT_PGDIR_SHBUF
tristate
config XEN_UNPOPULATED_ALLOC
-   bool
-   default y if ZONE_DEVICE && !ARM && !ARM64
+   bool "Use unpopulated memory ranges for guest mappings"
+   depends on X86
+   select ZONE_DEVICE
+   default y


I'd rather use "default XEN_BACKEND" here, as mappings of other guest's
memory is rarely used for non-backend guests.


There's also the privcmd and gnt devices which make heavy use of this,
so I'm not sure only selecting by default on XEN_BACKEND is the best
option.


I just want to avoid that kernels built for running as Xen guest, but
not as dom0, will be forced to select ZONE_DEVICE.

As privcmd is dom0-only, this is no problem.


Oh, didn't know that, I somehow assumed privcmd would be available to
all Xen guests regardless of whether dom0 support was selected.


My remark above should have been more precise in this regard:

privcmd operations dealing with foreign mappings are normally limited
to dom0 (there might be special cases, like linux-based stubdoms, but
those will be configured carefully for that purpose, so they don't
need to be considered for selecting the default IMO).




In case you are worrying about gnt devices, I'd be fine to switch to

default XEN_BACKEND || XEN_GNTDEV


Sure. maybe even:

default XEN_BACKEND || XEN_GNTDEV || XEN_DOM0


Yes.



Do you want me to resend with this changed or are you happy to fixup
if there are no further comments?


I'd prefer a resend (maybe after Patch 1 has gained its missing Ack, and
then with Patch 1 sent to me, too).


Juergen
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel


Re: [git pull] drm for 5.8-rc1

2020-08-14 Thread Karol Herbst
On Fri, Aug 14, 2020 at 3:57 PM Thierry Reding  wrote:
>
> On Thu, Aug 13, 2020 at 07:48:39PM +0200, Karol Herbst wrote:
> > On Thu, Aug 13, 2020 at 7:45 PM James Jones  wrote:
> > >
> > > I'll defer to Thierry, but I think that may be by design.  Tegra format
> > > modifiers were added to get things like this working in the first place,
> > > right?  It's not a regression, is it?
> > >
> >
> > That would be slightly annoying as this would mean by design it's
> > broken by default :/ Also, we have no Xorg release supporting
> > modifiers anyway and it does seem to work with X 1.20.8 (which doesn't
> > enable modifier support). And I talked with Jonas (working on mutter)
> > about it and there were no plans to turn on modifier support by
> > default at this point.
>
> I thought you said earlier that 1.20.8 didn't work and was hitting the
> assertion?
>

uhm, I forgot to mention that it works with the patch I wrote:
https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/6300

> Thierry

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel


Re: [git pull] drm for 5.8-rc1

2020-08-14 Thread Thierry Reding
On Thu, Aug 13, 2020 at 07:48:39PM +0200, Karol Herbst wrote:
> On Thu, Aug 13, 2020 at 7:45 PM James Jones  wrote:
> >
> > I'll defer to Thierry, but I think that may be by design.  Tegra format
> > modifiers were added to get things like this working in the first place,
> > right?  It's not a regression, is it?
> >
> 
> That would be slightly annoying as this would mean by design it's
> broken by default :/ Also, we have no Xorg release supporting
> modifiers anyway and it does seem to work with X 1.20.8 (which doesn't
> enable modifier support). And I talked with Jonas (working on mutter)
> about it and there were no plans to turn on modifier support by
> default at this point.

I thought you said earlier that 1.20.8 didn't work and was hitting the
assertion?

Thierry


signature.asc
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel


Re: [git pull] drm for 5.8-rc1

2020-08-14 Thread Karol Herbst
On Fri, Aug 14, 2020 at 3:40 PM Thierry Reding  wrote:
>
> On Wed, Aug 12, 2020 at 10:03:46AM -0700, James Jones wrote:
> > On 8/12/20 5:37 AM, Ilia Mirkin wrote:
> > > On Wed, Aug 12, 2020 at 8:24 AM Karol Herbst  wrote:
> > > >
> > > > On Wed, Aug 12, 2020 at 12:43 PM Karol Herbst  
> > > > wrote:
> > > > >
> > > > > On Wed, Aug 12, 2020 at 12:27 PM Karol Herbst  
> > > > > wrote:
> > > > > >
> > > > > > On Wed, Aug 12, 2020 at 2:19 AM James Jones  
> > > > > > wrote:
> > > > > > >
> > > > > > > Sorry for the slow reply here as well.  I've been in the process 
> > > > > > > of
> > > > > > > rebasing and reworking the userspace patches.  I'm not clear my 
> > > > > > > changes
> > > > > > > will address the Jetson Nano issue, but if you'd like to try 
> > > > > > > them, the
> > > > > > > latest userspace changes are available here:
> > > > > > >
> > > > > > > https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/3724
> > > > > > >
> > > > > > > And the tegra-drm kernel patches are here:
> > > > > > >
> > > > > > >
> > > > > > > https://patchwork.ozlabs.org/project/linux-tegra/patch/20191217005205.2573-1-jajo...@nvidia.com/
> > > > > > >
> > > > > > > Those + the kernel changes addressed in this thread are 
> > > > > > > everything I had
> > > > > > > outstanding.
> > > > > > >
> > > > > >
> > > > > > I don't know if that's caused by your changes or not, but now the
> > > > > > assert I hit is a different one pointing out that
> > > > > > nvc0_miptree_select_best_modifier fails in a certain case and 
> > > > > > returns
> > > > > > MOD_INVALID... anyway, it seems like with your patches applied it's
> > > > > > now way easier to debug and figure out what's going wrong, so maybe 
> > > > > > I
> > > > > > can figure it out now :)
> > > > > >
> > > > >
> > > > > collected some information which might help to track it down.
> > > > >
> > > > > src/gallium/frontends/dri/dri2.c:648 is the assert hit: assert(*zsbuf)
> > > > >
> > > > > templ is {reference = {count = 0}, width0 = 300, height0 = 300, depth0
> > > > > = 1, array_size = 1, format = PIPE_FORMAT_Z24X8_UNORM, target =
> > > > > PIPE_TEXTURE_2D, last_level = 0, nr_samples = 0, nr_storage_samples =
> > > > > 0, usage = 0, bind = 1, flags = 0, next = 0x0, screen = 0x0}
> > > > >
> > > > > inside tegra_screen_resource_create modifier says
> > > > > DRM_FORMAT_MOD_INVALID as template->bind is 1
> > > > >
> > > > > and nvc0_miptree_select_best_modifier returns DRM_FORMAT_MOD_INVALID,
> > > > > so the call just returns NULL leading to the assert.
> > > > >
> > > > > Btw, this is on Xorg-1.20.8-1.fc32.aarch64 with glxgears.
> > > > >
> > > >
> > > > So I digged a bit deeper and here is what tripps it of:
> > > >
> > > > when the context gets made current, the normal framebuffer validation
> > > > and render buffer allocation is done, but we end up inside
> > > > tegra_screen_resource_create at some point with PIPE_BIND_SCANOUT set
> > > > in template->bind. Now the tegra driver forces the
> > > > DRM_FORMAT_MOD_LINEAR modifier and calls into
> > > > resource_create_with_modifiers.
> > > >
> > > > If it wouldn't do that, nouveau would allocate a tiled buffer, with
> > > > that it's linear and we at some point end up with an assert about a
> > > > depth_stencil buffer being there even though it shouldn't. If I always
> > > > use DRM_FORMAT_MOD_INVALID in tegra_screen_resource_create, things
> > > > just work.
> > > >
> > > > That's kind of the cause I pinpointed the issue down to. But I have no
> > > > idea what's supposed to happen and what the actual bug is.
> > >
> > > Yeah, the bug with tegra has always been "trying to render to linear
> > > color + tiled depth", which the hardware plain doesn't support. (And
> > > linear depth isn't a thing.)
> > >
> > > Question is whether what it's doing necessary. PIPE_BIND_SCANOUT
> > > (/linear) requirements are needed for DRI2 to work (well, maybe not in
> > > theory, but at least in practice the nouveau ddx expects linear
> > > buffers). However tegra operates on a more DRI3-like basis, so with
> > > "client" allocations, tiled should work OK as long as there's
> > > something in tegra to copy it to linear when necessary?
> >
> > I can confirm the above: Our hardware can't render to linear depth buffers,
> > nor can it mix linear color buffers with block linear depth buffers.
> >
> > I think there's a misunderstanding on expected behavior of
> > resource_create_with_modifiers() here too: tegra_screen_resource_create() is
> > passing DRM_FORMAT_MOD_INVALID as the only modifier in non-scanout cases.
> > Previously, I believe nouveau may have treated that as "no modifiers
> > specified.  Fall back to internal layout selection logic", but in my patches
> > I "fixed" it to match other drivers' behavior, in that allocation will fail
> > if that is the only modifier in the list, since it is equivalent to passing
> > in a list containing only unsupported modifiers.  To get fallback behavior,
> > 

Re: [git pull] drm for 5.8-rc1

2020-08-14 Thread Thierry Reding
On Thu, Aug 13, 2020 at 03:00:37PM +0200, Karol Herbst wrote:
> At least for now I've created an MR to revert the change:
> https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/6300
> 
> But it seems like there was probably a good reason why it got added?
> Happy to have better fixes, but that's the best we've got so far I
> think?
> 
> Thierry, what do you think?

I did find the following via my mailbox (finally collecting all those
emails is paying off =):

https://lists.freedesktop.org/archives/mesa-dev/2018-May/196026.html

Cc'ing Daniel on the off chance that he remembers what the use-case was.

I vaguely recall that Daniel was pointing out an issue on IRC that
caused a series of fixes, including this change. Given that this was all
done because of framebuffer modifiers, I think my earlier hunch about
this being related to the case of non-modifier userspace is correct and
we do run into issues with userspace that doesn't use framebuffer
modifiers at all. In that case we basically have to assume pitch-linear
because userspace will use DRM_IOCTL_MODE_ADDFB, which doesn't support
modifiers.

Thierry


signature.asc
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel


Re: [git pull] drm for 5.8-rc1

2020-08-14 Thread Thierry Reding
On Wed, Aug 12, 2020 at 10:03:46AM -0700, James Jones wrote:
> On 8/12/20 5:37 AM, Ilia Mirkin wrote:
> > On Wed, Aug 12, 2020 at 8:24 AM Karol Herbst  wrote:
> > > 
> > > On Wed, Aug 12, 2020 at 12:43 PM Karol Herbst  wrote:
> > > > 
> > > > On Wed, Aug 12, 2020 at 12:27 PM Karol Herbst  
> > > > wrote:
> > > > > 
> > > > > On Wed, Aug 12, 2020 at 2:19 AM James Jones  
> > > > > wrote:
> > > > > > 
> > > > > > Sorry for the slow reply here as well.  I've been in the process of
> > > > > > rebasing and reworking the userspace patches.  I'm not clear my 
> > > > > > changes
> > > > > > will address the Jetson Nano issue, but if you'd like to try them, 
> > > > > > the
> > > > > > latest userspace changes are available here:
> > > > > > 
> > > > > > https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/3724
> > > > > > 
> > > > > > And the tegra-drm kernel patches are here:
> > > > > > 
> > > > > > 
> > > > > > https://patchwork.ozlabs.org/project/linux-tegra/patch/20191217005205.2573-1-jajo...@nvidia.com/
> > > > > > 
> > > > > > Those + the kernel changes addressed in this thread are everything 
> > > > > > I had
> > > > > > outstanding.
> > > > > > 
> > > > > 
> > > > > I don't know if that's caused by your changes or not, but now the
> > > > > assert I hit is a different one pointing out that
> > > > > nvc0_miptree_select_best_modifier fails in a certain case and returns
> > > > > MOD_INVALID... anyway, it seems like with your patches applied it's
> > > > > now way easier to debug and figure out what's going wrong, so maybe I
> > > > > can figure it out now :)
> > > > > 
> > > > 
> > > > collected some information which might help to track it down.
> > > > 
> > > > src/gallium/frontends/dri/dri2.c:648 is the assert hit: assert(*zsbuf)
> > > > 
> > > > templ is {reference = {count = 0}, width0 = 300, height0 = 300, depth0
> > > > = 1, array_size = 1, format = PIPE_FORMAT_Z24X8_UNORM, target =
> > > > PIPE_TEXTURE_2D, last_level = 0, nr_samples = 0, nr_storage_samples =
> > > > 0, usage = 0, bind = 1, flags = 0, next = 0x0, screen = 0x0}
> > > > 
> > > > inside tegra_screen_resource_create modifier says
> > > > DRM_FORMAT_MOD_INVALID as template->bind is 1
> > > > 
> > > > and nvc0_miptree_select_best_modifier returns DRM_FORMAT_MOD_INVALID,
> > > > so the call just returns NULL leading to the assert.
> > > > 
> > > > Btw, this is on Xorg-1.20.8-1.fc32.aarch64 with glxgears.
> > > > 
> > > 
> > > So I digged a bit deeper and here is what tripps it of:
> > > 
> > > when the context gets made current, the normal framebuffer validation
> > > and render buffer allocation is done, but we end up inside
> > > tegra_screen_resource_create at some point with PIPE_BIND_SCANOUT set
> > > in template->bind. Now the tegra driver forces the
> > > DRM_FORMAT_MOD_LINEAR modifier and calls into
> > > resource_create_with_modifiers.
> > > 
> > > If it wouldn't do that, nouveau would allocate a tiled buffer, with
> > > that it's linear and we at some point end up with an assert about a
> > > depth_stencil buffer being there even though it shouldn't. If I always
> > > use DRM_FORMAT_MOD_INVALID in tegra_screen_resource_create, things
> > > just work.
> > > 
> > > That's kind of the cause I pinpointed the issue down to. But I have no
> > > idea what's supposed to happen and what the actual bug is.
> > 
> > Yeah, the bug with tegra has always been "trying to render to linear
> > color + tiled depth", which the hardware plain doesn't support. (And
> > linear depth isn't a thing.)
> > 
> > Question is whether what it's doing necessary. PIPE_BIND_SCANOUT
> > (/linear) requirements are needed for DRI2 to work (well, maybe not in
> > theory, but at least in practice the nouveau ddx expects linear
> > buffers). However tegra operates on a more DRI3-like basis, so with
> > "client" allocations, tiled should work OK as long as there's
> > something in tegra to copy it to linear when necessary?
> 
> I can confirm the above: Our hardware can't render to linear depth buffers,
> nor can it mix linear color buffers with block linear depth buffers.
> 
> I think there's a misunderstanding on expected behavior of
> resource_create_with_modifiers() here too: tegra_screen_resource_create() is
> passing DRM_FORMAT_MOD_INVALID as the only modifier in non-scanout cases.
> Previously, I believe nouveau may have treated that as "no modifiers
> specified.  Fall back to internal layout selection logic", but in my patches
> I "fixed" it to match other drivers' behavior, in that allocation will fail
> if that is the only modifier in the list, since it is equivalent to passing
> in a list containing only unsupported modifiers.  To get fallback behavior,
> tegra_screen_resource_create() should pass in (NULL, 0) for (modifiers,
> count), or just call resource_create() on the underlying screen instead.
> 
> Beyond that, I can only offer my thoughts based on analysis of the code
> referenced here so far:
> 
> While I've learned from the origins of 

[PATCH 0/3] drm/panel: Add panel driver for the Mantix MLAF057WE51-X DSI panel

2020-08-14 Thread Guido Günther
The panel uses a Focaltech FT8006p, the touch part is handled by the already
existing edt-ft5x06. It can be found in e.g. the Librem 5.

This series is against next-20200814.

Guido Günther (3):
  dt-bindings: vendor-prefixes: Add mantix vendor prefix
  dt-bindings: Add Mantix MLAF057WE51-X panel bindings
  drm/panel: Add panel driver for the Mantix MLAF057WE51-X DSI panel

 .../display/panel/mantix,mlaf057we51-x.yaml   |  73 
 .../devicetree/bindings/vendor-prefixes.yaml  |   2 +
 MAINTAINERS   |   7 +
 drivers/gpu/drm/panel/Kconfig |  11 +
 drivers/gpu/drm/panel/Makefile|   1 +
 .../gpu/drm/panel/panel-mantix-mlaf057we51.c  | 362 ++
 6 files changed, 456 insertions(+)
 create mode 100644 
Documentation/devicetree/bindings/display/panel/mantix,mlaf057we51-x.yaml
 create mode 100644 drivers/gpu/drm/panel/panel-mantix-mlaf057we51.c

-- 
2.26.2

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel


[PATCH 3/3] drm/panel: Add panel driver for the Mantix MLAF057WE51-X DSI panel

2020-08-14 Thread Guido Günther
The panel uses a Focaltech FT8006p, the touch part is handled by the
already existing edt-ft5x06.

Signed-off-by: Guido Günther 
---
 MAINTAINERS   |   7 +
 drivers/gpu/drm/panel/Kconfig |  11 +
 drivers/gpu/drm/panel/Makefile|   1 +
 .../gpu/drm/panel/panel-mantix-mlaf057we51.c  | 362 ++
 4 files changed, 381 insertions(+)
 create mode 100644 drivers/gpu/drm/panel/panel-mantix-mlaf057we51.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 83ba7b62651f7..7dfe4cc3d4ec8 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5474,6 +5474,13 @@ S:   Maintained
 F: drivers/gpu/drm/panel/panel-lvds.c
 F: Documentation/devicetree/bindings/display/panel/lvds.yaml
 
+DRM DRIVER FOR MANTIX MLAF057WE51 PANELS
+M: Guido Günther 
+R: Purism Kernel Team 
+S: Maintained
+F: 
Documentation/devicetree/bindings/display/panel/mantix,mlaf057we51-x.yaml
+F: drivers/gpu/drm/panel/panel-mantix-mlaf057we51.c
+
 DRM DRIVER FOR MATROX G200/G400 GRAPHICS CARDS
 S: Orphan / Obsolete
 F: drivers/gpu/drm/mga/
diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
index de2f2a452be55..8d97d07c58713 100644
--- a/drivers/gpu/drm/panel/Kconfig
+++ b/drivers/gpu/drm/panel/Kconfig
@@ -217,6 +217,17 @@ config DRM_PANEL_NOVATEK_NT39016
  Say Y here if you want to enable support for the panels built
  around the Novatek NT39016 display controller.
 
+config DRM_PANEL_MANTIX_MLAF057WE51
+   tristate "Mantix MLAF057WE51-X MIPI-DSI LCD panel"
+   depends on OF
+   depends on DRM_MIPI_DSI
+   depends on BACKLIGHT_CLASS_DEVICE
+   help
+ Say Y here if you want to enable support for the Mantix
+ MLAF057WE51-X MIPI DSI panel as e.g. used in the Librem 5. It
+ has a resolution of 720x1440 pixels, a built in backlight and touch
+ controller.
+
 config DRM_PANEL_OLIMEX_LCD_OLINUXINO
tristate "Olimex LCD-OLinuXino panel"
depends on OF
diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
index e45ceac6286fd..15a4e77529514 100644
--- a/drivers/gpu/drm/panel/Makefile
+++ b/drivers/gpu/drm/panel/Makefile
@@ -20,6 +20,7 @@ obj-$(CONFIG_DRM_PANEL_LG_LG4573) += panel-lg-lg4573.o
 obj-$(CONFIG_DRM_PANEL_NEC_NL8048HL11) += panel-nec-nl8048hl11.o
 obj-$(CONFIG_DRM_PANEL_NOVATEK_NT35510) += panel-novatek-nt35510.o
 obj-$(CONFIG_DRM_PANEL_NOVATEK_NT39016) += panel-novatek-nt39016.o
+obj-$(CONFIG_DRM_PANEL_MANTIX_MLAF057WE51) += panel-mantix-mlaf057we51.o
 obj-$(CONFIG_DRM_PANEL_OLIMEX_LCD_OLINUXINO) += panel-olimex-lcd-olinuxino.o
 obj-$(CONFIG_DRM_PANEL_ORISETECH_OTM8009A) += panel-orisetech-otm8009a.o
 obj-$(CONFIG_DRM_PANEL_OSD_OSD101T2587_53TS) += panel-osd-osd101t2587-53ts.o
diff --git a/drivers/gpu/drm/panel/panel-mantix-mlaf057we51.c 
b/drivers/gpu/drm/panel/panel-mantix-mlaf057we51.c
new file mode 100644
index 0..6c07bcdb75937
--- /dev/null
+++ b/drivers/gpu/drm/panel/panel-mantix-mlaf057we51.c
@@ -0,0 +1,362 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Mantix MLAF057WE51 5.7" MIPI-DSI panel driver
+ *
+ * Copyright (C) Purism SPC 2020
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+
+#include 
+#include 
+#include 
+#include 
+
+#define DRV_NAME "panel-mantix-mlaf057we51"
+
+/* Manufacturer specific Commands send via DSI */
+#define MANTIX_CMD_OTP_STOP_RELOAD_MIPI 0x41
+#define MANTIX_CMD_INT_CANCEL   0x4C
+
+struct mantix {
+   struct device *dev;
+   struct drm_panel panel;
+   struct gpio_desc *reset_gpio;
+
+   struct regulator *avdd;
+   struct regulator *avee;
+   struct regulator *vddi;
+};
+
+static inline struct mantix *panel_to_mantix(struct drm_panel *panel)
+{
+   return container_of(panel, struct mantix, panel);
+}
+
+#define dsi_generic_write_seq(dsi, seq...) do {
\
+   static const u8 d[] = { seq };  \
+   int ret;\
+   ret = mipi_dsi_generic_write(dsi, d, ARRAY_SIZE(d));\
+   if (ret < 0)\
+   return ret; \
+   } while (0)
+
+static int mantix_init_sequence(struct mantix *ctx)
+{
+   struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
+   struct device *dev = ctx->dev;
+
+   /*
+* Init sequence was supplied by the panel vendor.
+*/
+   dsi_generic_write_seq(dsi, MANTIX_CMD_OTP_STOP_RELOAD_MIPI, 0x5A);
+
+   dsi_generic_write_seq(dsi, MANTIX_CMD_INT_CANCEL, 0x03);
+   dsi_generic_write_seq(dsi, MANTIX_CMD_OTP_STOP_RELOAD_MIPI, 0x5A, 0x03);
+   dsi_generic_write_seq(dsi, 0x80, 0xA9, 0x00);
+
+   dsi_generic_write_seq(dsi, MANTIX_CMD_OTP_STOP_RELOAD_MIPI, 0x5A, 0x09);
+   dsi_generic_write_seq(dsi, 

[PATCH 1/3] dt-bindings: vendor-prefixes: Add mantix vendor prefix

2020-08-14 Thread Guido Günther
Add prefix for Mantix Display Technology Co.,Ltd.

Signed-off-by: Guido Günther 
---
 Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml 
b/Documentation/devicetree/bindings/vendor-prefixes.yaml
index 2baee2c817c1a..59d4c8b068c4d 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -611,6 +611,8 @@ patternProperties:
 description: Linux Automation GmbH
   "^macnica,.*":
 description: Macnica Americas
+  "^mantix,.*":
+description: Mantix Display Technology Co.,Ltd.
   "^mapleboard,.*":
 description: Mapleboard.org
   "^marvell,.*":
-- 
2.26.2

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel


[PATCH 2/3] dt-bindings: Add Mantix MLAF057WE51-X panel bindings

2020-08-14 Thread Guido Günther
The panel uses a Focaltech FT8006p, the touch part is handled by the
already existing edt-ft5x06.

Signed-off-by: Guido Günther 
---
 .../display/panel/mantix,mlaf057we51-x.yaml   | 73 +++
 1 file changed, 73 insertions(+)
 create mode 100644 
Documentation/devicetree/bindings/display/panel/mantix,mlaf057we51-x.yaml

diff --git 
a/Documentation/devicetree/bindings/display/panel/mantix,mlaf057we51-x.yaml 
b/Documentation/devicetree/bindings/display/panel/mantix,mlaf057we51-x.yaml
new file mode 100644
index 0..349f3380ac940
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/panel/mantix,mlaf057we51-x.yaml
@@ -0,0 +1,73 @@
+# SPDX-License-Identifier: (GPL-2.0-only or BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/panel/mantix,mlaf057we51-x.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Mantix MLAF057WE51-X 5.7" 720x1440 TFT LCD panel
+
+maintainers:
+  - Guido Günther 
+
+description: |
+ Mantix MLAF057WE51 X is a 720x1440 TFT LCD panel
+ connected using a MIPI-DSI video interface.
+
+allOf:
+  - $ref: panel-common.yaml#
+
+properties:
+  compatible:
+enum:
+  - mantix,mlaf057we51-x
+
+  port: true
+  reg:
+maxItems: 1
+description: DSI virtual channel
+
+  avdd-supply:
+description: Positive analog power supply
+
+  avee-supply:
+description: Negative analog power supply
+
+  vddi-supply:
+description: 1.8V I/O voltage supply
+
+  reset-gpios:
+description: GPIO used for the reset pin
+maxItems: 1
+
+  backlight:
+description: Backlight used by the panel
+$ref: "/schemas/types.yaml#/definitions/phandle"
+
+required:
+  - compatible
+  - reg
+  - avdd-supply
+  - avee-supply
+  - vddi-supply
+  - reset-gpios
+
+additionalProperties: false
+
+examples:
+  - |
+#include 
+
+dsi {
+  #address-cells = <1>;
+  #size-cells = <0>;
+  panel@0 {
+compatible = "mantix,mlaf057we51-x";
+reg = <0>;
+avdd-supply = <_avdd>;
+avee-supply = <_avee>;
+vddi-supply = <_1v8_p>;
+reset-gpios = < 29 GPIO_ACTIVE_LOW>;
+backlight = <>;
+  };
+};
+...
-- 
2.26.2

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel


Re: [PATCH v8 2/3] drm: bridge: Add support for Cadence MHDP DPI/DP bridge

2020-08-14 Thread Tomi Valkeinen
On 11/08/2020 05:36, Laurent Pinchart wrote:

>> +static int cdns_mhdp_mailbox_write(struct cdns_mhdp_device *mhdp, u8 val)
>> +{
>> +int ret, full;
>> +
>> +WARN_ON(!mutex_is_locked(>mbox_mutex));
>> +
>> +ret = readx_poll_timeout(readl, mhdp->regs + CDNS_MAILBOX_FULL,
>> + full, !full, MAILBOX_RETRY_US,
>> + MAILBOX_TIMEOUT_US);
>> +if (ret < 0)
>> +return ret;
>> +
>> +writel(val, mhdp->regs + CDNS_MAILBOX_TX_DATA);
>> +
>> +return 0;
>> +}
> 
> As commented previously, I think there's room for optimization here. Two
> options that I think should be investigated are using the mailbox
> interrupts, and only polling for the first byte of the message
> (depending on whether the firmware implementation can guarantee that
> when the first byte is available, the rest of the message will be
> immediately available too). This can be done on top of this patch
> though.

I made some tests on this.

I cannot see mailbox_write ever looping, mailbox is never full. So in this case 
the
readx_poll_timeout() call is there just for safety to catch the cases where 
something has gone
totally wrong or perhaps once in a while the mbox can be full for a tiny 
moment. But we always do
need to check CDNS_MAILBOX_FULL before each write to CDNS_MAILBOX_TX_DATA, so 
we can as well use
readx_poll_timeout for that to catch the odd cases (afaics, there's no real 
overhead if the exit
condition is true immediately).

mailbox_read polls sometimes. Most often it does not poll, as the data is ready 
in the mbox, and in
these cases the situation is the same as for mailbox_write.

The cases where it does poll are related to things where the fw has to wait for 
something. The
longest poll waits seemed to be EDID read (16 ms wait) and adjusting LT (1.7 ms 
wait). And afaics,
when the first byte of the received message is there, the rest of the bytes 
will be available
without wait.

For mailbox_write and for most mailbox_reads I think using interrupts makes no 
sense, as the
overhead would be big.

For those few long read operations, interrupts would make sense. I guess a 
simple way to handle this
would be to add a new function, wait_for_mbox_data() or such, which would use 
the interrupts to wait
for mbox not empty. This function could be used in selected functions (edid, 
LT) after
cdns_mhdp_mailbox_send().

Although I think it's not that bad currently, MAILBOX_RETRY_US is 1ms, so it's 
quite lazy polling,
so perhaps this can be considered TODO optimization.

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel


Re: [PATCH v4 2/2] xen: add helpers to allocate unpopulated memory

2020-08-14 Thread Jürgen Groß

On 14.08.20 14:47, Roger Pau Monné wrote:

On Fri, Aug 14, 2020 at 12:27:32PM +0200, Jürgen Groß wrote:

On 14.08.20 11:56, Roger Pau Monné wrote:

On Fri, Aug 14, 2020 at 08:29:20AM +0100, Christoph Hellwig wrote:

On Thu, Aug 13, 2020 at 09:54:20AM +0200, Roger Pau Monn?? wrote:

On Thu, Aug 13, 2020 at 08:33:37AM +0100, Christoph Hellwig wrote:

On Tue, Aug 11, 2020 at 11:44:47AM +0200, Roger Pau Monne wrote:

If enabled (because ZONE_DEVICE is supported) the usage of the new
functionality untangles Xen balloon and RAM hotplug from the usage of
unpopulated physical memory ranges to map foreign pages, which is the
correct thing to do in order to avoid mappings of foreign pages depend
on memory hotplug.


So please just select ZONE_DEVICE if this is so much better rather
than maintaining two variants.


We still need to other variant for Arm at least, so both need to be
maintained anyway, even if we force ZONE_DEVICE on x86.


Well, it still really helps reproducability if you stick to one
implementation of x86.

The alternative would be an explicit config option to opt into it,
but just getting a different implementation based on a random
kernel option is strange.


Would adding something like the chunk below to the patch be OK?

---8<---
diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig
index 018020b91baa..5f321a1319e6 100644
--- a/drivers/xen/Kconfig
+++ b/drivers/xen/Kconfig
@@ -328,7 +328,14 @@ config XEN_FRONT_PGDIR_SHBUF
tristate
   config XEN_UNPOPULATED_ALLOC
-   bool
-   default y if ZONE_DEVICE && !ARM && !ARM64
+   bool "Use unpopulated memory ranges for guest mappings"
+   depends on X86
+   select ZONE_DEVICE
+   default y


I'd rather use "default XEN_BACKEND" here, as mappings of other guest's
memory is rarely used for non-backend guests.


There's also the privcmd and gnt devices which make heavy use of this,
so I'm not sure only selecting by default on XEN_BACKEND is the best
option.


I just want to avoid that kernels built for running as Xen guest, but
not as dom0, will be forced to select ZONE_DEVICE.

As privcmd is dom0-only, this is no problem.

In case you are worrying about gnt devices, I'd be fine to switch to

default XEN_BACKEND || XEN_GNTDEV


Juergen

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel


Re: [PATCH v8 2/3] drm: bridge: Add support for Cadence MHDP DPI/DP bridge

2020-08-14 Thread Tomi Valkeinen
On 11/08/2020 05:36, Laurent Pinchart wrote:

>> +static int cdns_mhdp_connector_init(struct cdns_mhdp_device *mhdp)
>> +{
>> +u32 bus_format = MEDIA_BUS_FMT_RGB121212_1X36;
>> +struct drm_connector *conn = >connector;
>> +struct drm_bridge *bridge = >bridge;
>> +int ret;
>> +
>> +if (!bridge->encoder) {
>> +DRM_ERROR("Parent encoder object not found");
>> +return -ENODEV;
>> +}
>> +
>> +conn->polled = DRM_CONNECTOR_POLL_HPD;
>> +
>> +ret = drm_connector_init(bridge->dev, conn, _mhdp_conn_funcs,
>> + DRM_MODE_CONNECTOR_DisplayPort);
>> +if (ret) {
>> +DRM_ERROR("Failed to initialize connector with drm\n");
>> +return ret;
>> +}
>> +
>> +drm_connector_helper_add(conn, _mhdp_conn_helper_funcs);
>> +
>> +ret = drm_display_info_set_bus_formats(>display_info,
>> +   _format, 1);
>> +if (ret)
>> +return ret;
>> +
>> +conn->display_info.bus_flags = DRM_BUS_FLAG_DE_HIGH;
> 
> Aren't these supposed to be retrieved from the display ? Why do we need
> to override them here ?

DE_HIGH is meant for the display controller. I think this should be in 
bridge->timings->input_bus_flags

>> +static int cdns_mhdp_atomic_check(struct drm_bridge *bridge,
>> +  struct drm_bridge_state *bridge_state,
>> +  struct drm_crtc_state *crtc_state,
>> +  struct drm_connector_state *conn_state)
>> +{
>> +struct cdns_mhdp_device *mhdp = bridge_to_mhdp(bridge);
>> +const struct drm_display_mode *mode = _state->adjusted_mode;
>> +int ret;
>> +
>> +if (!mhdp->plugged)
>> +return 0;
>> +
>> +mutex_lock(>link_mutex);
>> +
>> +if (!mhdp->link_up) {
>> +ret = cdns_mhdp_link_up(mhdp);
>> +if (ret < 0)
>> +goto err_check;
>> +}
> 
> atomic_check isn't supposed to access the hardware. Is there a reason
> this is needed ?

We have been going back and forth with this. The basic problem is that to 
understand which
videomodes can be used, you need to do link training to see the bandwidth 
available.

I'm not sure if we strictly need to do LT in atomic check, though... It would 
then pass modes that
can't be used, but perhaps that's not a big issue.

I think the main point with doing LT in certain places is to filter the list of 
video modes passed
to userspace, as we can't pass the modes from EDID directly without filtering 
them based on the link
bandwidth.

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel


Re: Role of DMA Heaps vs GEM in allocation

2020-08-14 Thread Daniel Vetter
On Fri, Aug 14, 2020 at 1:34 PM Mikko Perttunen  wrote:
>
> Hi,
>
> I'm currently working on a new UAPI for Host1x/TegraDRM (see first draft
> in thread "[RFC] Host1x/TegraDRM UAPI"[1]). One question that has come
> up is regarding the buffer allocation mechanism. Traditionally, DRM
> drivers provide custom GEM allocation IOCTLs. However, we now have DMA
> Heaps, which would be sufficient for TegraDRM's needs, so we could skip
> implementing any GEM IOCTLs in the TegraDRM UAPI, and rely on importing
> DMA-BUFs. This would mean less code on TegraDRM's side.
>
> However, one complication with using DMA Heaps is that it only provides
> DMA-BUF FDs, so it is possible that a user application could run out of
> free file descriptors if it is not adjusting its soft FD limit. This
> would especially be a problem for existing applications that might have
> worked with the traditional GEM model and didn't need to adjust their FD
> limits, but would then fail in some situations with the increased FD
> usage of DMA-BUF FDs.
>
> My question is then: what is the role of DMA Heaps? If it is to be used
> as a central allocator, should the FD issue be left to the application,
> or addressed somehow? Should it be considered a potential alternative
> for GEM allocations?

Atm no one knows. What's for sure is that dma-buf fd are meant to
establish sharing, and then once imported everywhere, closed again.
dma-buf heaps might or might be useful for sharing the cross-device
memory allocator problem (the rough idea is that in sysfs every device
lists all the heaps it can use, and then you pick the common one that
works for all devices). But that's for shared buffers only.

For an acceleration driver you want drm gem ids, because yes fd
limits. Also constantly having to reimport dma-buf for every cs ioctl
doesn't sound like a bright idea either, there's a reason we have the
drm_prime cache and all that stuff.

I have also no idea why you wouldn't want to use the existing drm
infrastructure, it's all there.

Cheers, Daniel

>
> Thanks,
> Mikko
>
> [1] https://www.spinics.net/lists/dri-devel/msg262021.html
> ___
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel



-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel


Role of DMA Heaps vs GEM in allocation

2020-08-14 Thread Mikko Perttunen

Hi,

I'm currently working on a new UAPI for Host1x/TegraDRM (see first draft 
in thread "[RFC] Host1x/TegraDRM UAPI"[1]). One question that has come 
up is regarding the buffer allocation mechanism. Traditionally, DRM 
drivers provide custom GEM allocation IOCTLs. However, we now have DMA 
Heaps, which would be sufficient for TegraDRM's needs, so we could skip 
implementing any GEM IOCTLs in the TegraDRM UAPI, and rely on importing 
DMA-BUFs. This would mean less code on TegraDRM's side.


However, one complication with using DMA Heaps is that it only provides 
DMA-BUF FDs, so it is possible that a user application could run out of 
free file descriptors if it is not adjusting its soft FD limit. This 
would especially be a problem for existing applications that might have 
worked with the traditional GEM model and didn't need to adjust their FD 
limits, but would then fail in some situations with the increased FD 
usage of DMA-BUF FDs.


My question is then: what is the role of DMA Heaps? If it is to be used 
as a central allocator, should the FD issue be left to the application, 
or addressed somehow? Should it be considered a potential alternative 
for GEM allocations?


Thanks,
Mikko

[1] https://www.spinics.net/lists/dri-devel/msg262021.html
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel


Re: [PATCH v8 1/3] dt-bindings: drm/bridge: Document Cadence MHDP bridge bindings

2020-08-14 Thread Tomi Valkeinen
On 11/08/2020 03:36, Laurent Pinchart wrote:

> I've got a chance to study the J721E datasheet, and it shows the DP
> bridge has 4 inputs, to support MST. Shouldn't this already be reflected
> in the DT bindings ? I think it should be as simple as having 4 input
> ports (port@0 to port@3) and one output port (port@4).

I think this is a good point, mhdp has 4 input streams.

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel


Re: [PATCH v4 2/2] xen: add helpers to allocate unpopulated memory

2020-08-14 Thread Jürgen Groß

On 14.08.20 11:56, Roger Pau Monné wrote:

On Fri, Aug 14, 2020 at 08:29:20AM +0100, Christoph Hellwig wrote:

On Thu, Aug 13, 2020 at 09:54:20AM +0200, Roger Pau Monn?? wrote:

On Thu, Aug 13, 2020 at 08:33:37AM +0100, Christoph Hellwig wrote:

On Tue, Aug 11, 2020 at 11:44:47AM +0200, Roger Pau Monne wrote:

If enabled (because ZONE_DEVICE is supported) the usage of the new
functionality untangles Xen balloon and RAM hotplug from the usage of
unpopulated physical memory ranges to map foreign pages, which is the
correct thing to do in order to avoid mappings of foreign pages depend
on memory hotplug.


So please just select ZONE_DEVICE if this is so much better rather
than maintaining two variants.


We still need to other variant for Arm at least, so both need to be
maintained anyway, even if we force ZONE_DEVICE on x86.


Well, it still really helps reproducability if you stick to one
implementation of x86.

The alternative would be an explicit config option to opt into it,
but just getting a different implementation based on a random
kernel option is strange.


Would adding something like the chunk below to the patch be OK?

---8<---
diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig
index 018020b91baa..5f321a1319e6 100644
--- a/drivers/xen/Kconfig
+++ b/drivers/xen/Kconfig
@@ -328,7 +328,14 @@ config XEN_FRONT_PGDIR_SHBUF
tristate
  
  config XEN_UNPOPULATED_ALLOC

-   bool
-   default y if ZONE_DEVICE && !ARM && !ARM64
+   bool "Use unpopulated memory ranges for guest mappings"
+   depends on X86
+   select ZONE_DEVICE
+   default y


I'd rather use "default XEN_BACKEND" here, as mappings of other guest's
memory is rarely used for non-backend guests.


Juergen

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel


[PATCH] drm/modeset-lock: Take the modeset BKL for legacy drivers

2020-08-14 Thread Daniel Vetter
This fell off in the conversion in

commit 9bcaa3fe58ab7559e71df798bcff6e0795158695
Author: Michal Orzel 
Date:   Tue Apr 28 19:10:04 2020 +0200

drm: Replace drm_modeset_lock/unlock_all with DRM_MODESET_LOCK_ALL_* helpers

but it's caught by the drm_warn_on_modeset_not_all_locked() that the
legacy modeset code uses. Since this is the bkl and it's unclear
what's all protected, play it safe and grab it again for legacy
drivers.

Unfortunately this means we need to sprinkle a few more #includes
around.

Also we need to add the drm_device as a parameter to the _END macro.

Finally remove the mute_lock() from setcrtc, since that's now done by
the macro.

Cc: Alex Deucher 
References: https://gitlab.freedesktop.org/drm/amd/-/issues/1224
Fixes: 9bcaa3fe58ab ("drm: Replace drm_modeset_lock/unlock_all with 
DRM_MODESET_LOCK_ALL_* helpers")
Cc: Michal Orzel 
Cc: Daniel Vetter 
Cc: Maarten Lankhorst 
Cc: Maxime Ripard 
Cc: Thomas Zimmermann 
Cc: David Airlie 
Cc: Daniel Vetter 
Cc: dri-devel@lists.freedesktop.org
Cc:  # v5.8+
Signed-off-by: Daniel Vetter 
--
Patch compiles but otherwise untested, and I'll go on vacations now
for 2 weeks. Alex, can you pls take care of this?

Thanks, Daniel
---
 drivers/gpu/drm/drm_atomic_helper.c | 7 ---
 drivers/gpu/drm/drm_color_mgmt.c| 2 +-
 drivers/gpu/drm/drm_crtc.c  | 4 +---
 drivers/gpu/drm/drm_mode_object.c   | 4 ++--
 drivers/gpu/drm/drm_plane.c | 2 +-
 include/drm/drm_modeset_lock.h  | 9 +++--
 6 files changed, 16 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
b/drivers/gpu/drm/drm_atomic_helper.c
index f67ee513a7cc..7515a40b2056 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -34,6 +34,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -3109,7 +3110,7 @@ void drm_atomic_helper_shutdown(struct drm_device *dev)
if (ret)
DRM_ERROR("Disabling all crtc's during unload failed with 
%i\n", ret);
 
-   DRM_MODESET_LOCK_ALL_END(ctx, ret);
+   DRM_MODESET_LOCK_ALL_END(dev, ctx, ret);
 }
 EXPORT_SYMBOL(drm_atomic_helper_shutdown);
 
@@ -3249,7 +3250,7 @@ struct drm_atomic_state *drm_atomic_helper_suspend(struct 
drm_device *dev)
}
 
 unlock:
-   DRM_MODESET_LOCK_ALL_END(ctx, err);
+   DRM_MODESET_LOCK_ALL_END(dev, ctx, err);
if (err)
return ERR_PTR(err);
 
@@ -3330,7 +3331,7 @@ int drm_atomic_helper_resume(struct drm_device *dev,
 
err = drm_atomic_helper_commit_duplicated_state(state, );
 
-   DRM_MODESET_LOCK_ALL_END(ctx, err);
+   DRM_MODESET_LOCK_ALL_END(dev, ctx, err);
drm_atomic_state_put(state);
 
return err;
diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
index c93123ff7c21..138ff34b31db 100644
--- a/drivers/gpu/drm/drm_color_mgmt.c
+++ b/drivers/gpu/drm/drm_color_mgmt.c
@@ -294,7 +294,7 @@ int drm_mode_gamma_set_ioctl(struct drm_device *dev,
 crtc->gamma_size, );
 
 out:
-   DRM_MODESET_LOCK_ALL_END(ctx, ret);
+   DRM_MODESET_LOCK_ALL_END(dev, ctx, ret);
return ret;
 
 }
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 283bcc4362ca..aecdd7ea26dc 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -588,7 +588,6 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
if (crtc_req->mode_valid && !drm_lease_held(file_priv, plane->base.id))
return -EACCES;
 
-   mutex_lock(>dev->mode_config.mutex);
DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx,
   DRM_MODESET_ACQUIRE_INTERRUPTIBLE, ret);
 
@@ -756,8 +755,7 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
fb = NULL;
mode = NULL;
 
-   DRM_MODESET_LOCK_ALL_END(ctx, ret);
-   mutex_unlock(>dev->mode_config.mutex);
+   DRM_MODESET_LOCK_ALL_END(dev, ctx, ret);
 
return ret;
 }
diff --git a/drivers/gpu/drm/drm_mode_object.c 
b/drivers/gpu/drm/drm_mode_object.c
index 901b078abf40..db05f386a709 100644
--- a/drivers/gpu/drm/drm_mode_object.c
+++ b/drivers/gpu/drm/drm_mode_object.c
@@ -428,7 +428,7 @@ int drm_mode_obj_get_properties_ioctl(struct drm_device 
*dev, void *data,
 out_unref:
drm_mode_object_put(obj);
 out:
-   DRM_MODESET_LOCK_ALL_END(ctx, ret);
+   DRM_MODESET_LOCK_ALL_END(dev, ctx, ret);
return ret;
 }
 
@@ -470,7 +470,7 @@ static int set_property_legacy(struct drm_mode_object *obj,
break;
}
drm_property_change_valid_put(prop, ref);
-   DRM_MODESET_LOCK_ALL_END(ctx, ret);
+   DRM_MODESET_LOCK_ALL_END(dev, ctx, ret);
 
return ret;
 }
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index b7b90b3a2e38..affe1cfed009 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -792,7 +792,7 @@ static int setplane_internal(struct 

Re: [RFC][PATCH v2 2/2] dma-heap: Add a system-uncached heap

2020-08-14 Thread Daniel Vetter
On Fri, Aug 14, 2020 at 06:24:58AM +, John Stultz wrote:
> This adds a heap that allocates non-contiguous buffers that are
> marked as writecombined, so they are not cached by the CPU.
> 
> This is useful, as most graphics buffers are usually not touched
> by the CPU or only written into once by the CPU. So when mapping
> the buffer over and over between devices, we can skip the CPU
> syncing, which saves a lot of cache management overhead, greatly
> improving performance.
> 
> For folk using ION, there was a ION_FLAG_CACHED flag, which
> signaled if the returned buffer should be CPU cacheable or not.
> With DMA-BUF heaps, we have no such flag, and by default the
> current heaps (system and cma) produce CPU cachable buffers.
> So for folks transitioning from ION to DMA-BUF Heaps, this fills
> in some of that missing functionality.
> 
> This does have a few "ugly" bits that were required to get
> the buffer properly flushed out initially which I'd like to
> improve. So feedback would be very welcome!
> 
> Many thanks to Liam Mark for his help to get this working.
> 
> Cc: Sumit Semwal 
> Cc: Andrew F. Davis 
> Cc: Benjamin Gaignard 
> Cc: Liam Mark 
> Cc: Laura Abbott 
> Cc: Brian Starkey 
> Cc: Hridya Valsaraju 
> Cc: Robin Murphy 
> Cc: linux-me...@vger.kernel.org
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: John Stultz 
> ---
> v2:
> * Fix build issue on sh reported-by: kernel test robot 
> * Rework to use for_each_sgtable_sg(), dma_map_sgtable(), and
>   for_each_sg_page() along with numerous other cleanups suggested
>   by Robin Murphy

Mildly annoying me, but where's the userspace for this? Do we have a
gralloc somewhere that works with open driver stacks and uses this?

Without that I'm somewhat afraid we'll engineer ourselves something that
looks like it should work, but doesn't really work in reality.
-Daniel

> ---
>  drivers/dma-buf/heaps/Kconfig|  10 +
>  drivers/dma-buf/heaps/Makefile   |   1 +
>  drivers/dma-buf/heaps/system_uncached_heap.c | 371 +++
>  3 files changed, 382 insertions(+)
>  create mode 100644 drivers/dma-buf/heaps/system_uncached_heap.c
> 
> diff --git a/drivers/dma-buf/heaps/Kconfig b/drivers/dma-buf/heaps/Kconfig
> index a5eef06c4226..420b0ed0a512 100644
> --- a/drivers/dma-buf/heaps/Kconfig
> +++ b/drivers/dma-buf/heaps/Kconfig
> @@ -5,6 +5,16 @@ config DMABUF_HEAPS_SYSTEM
> Choose this option to enable the system dmabuf heap. The system heap
> is backed by pages from the buddy allocator. If in doubt, say Y.
>  
> +config DMABUF_HEAPS_SYSTEM_UNCACHED
> + bool "DMA-BUF Uncached System Heap"
> + depends on DMABUF_HEAPS
> + help
> +   Choose this option to enable the uncached system dmabuf heap. This
> +   heap is backed by pages from the buddy allocator, but pages are setup
> +   for write combining. This avoids cache management overhead, and can
> +   be faster if pages are mostly untouched by the cpu.  If in doubt,
> +   say Y.
> +
>  config DMABUF_HEAPS_CMA
>   bool "DMA-BUF CMA Heap"
>   depends on DMABUF_HEAPS && DMA_CMA
> diff --git a/drivers/dma-buf/heaps/Makefile b/drivers/dma-buf/heaps/Makefile
> index 6e54cdec3da0..085685ec478f 100644
> --- a/drivers/dma-buf/heaps/Makefile
> +++ b/drivers/dma-buf/heaps/Makefile
> @@ -1,4 +1,5 @@
>  # SPDX-License-Identifier: GPL-2.0
>  obj-y+= heap-helpers.o
>  obj-$(CONFIG_DMABUF_HEAPS_SYSTEM)+= system_heap.o
> +obj-$(CONFIG_DMABUF_HEAPS_SYSTEM_UNCACHED) += system_uncached_heap.o
>  obj-$(CONFIG_DMABUF_HEAPS_CMA)   += cma_heap.o
> diff --git a/drivers/dma-buf/heaps/system_uncached_heap.c 
> b/drivers/dma-buf/heaps/system_uncached_heap.c
> new file mode 100644
> index ..3b8e699bcae7
> --- /dev/null
> +++ b/drivers/dma-buf/heaps/system_uncached_heap.c
> @@ -0,0 +1,371 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Uncached System DMA-Heap exporter
> + *
> + * Copyright (C) 2020 Linaro Ltd.
> + *
> + * Based off of Andrew Davis' SRAM heap:
> + * Copyright (C) 2019 Texas Instruments Incorporated - http://www.ti.com/
> + *   Andrew F. Davis 
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +struct uncached_heap {
> + struct dma_heap *heap;
> +};
> +
> +struct uncached_heap_buffer {
> + struct dma_heap *heap;
> + struct list_head attachments;
> + struct mutex lock;
> + unsigned long len;
> + struct sg_table sg_table;
> + int vmap_cnt;
> + void *vaddr;
> +};
> +
> +struct dma_heap_attachment {
> + struct device *dev;
> + struct sg_table *table;
> + struct list_head list;
> +};
> +
> +static struct sg_table *dup_sg_table(struct sg_table *table)
> +{
> + struct sg_table *new_table;
> + int ret, i;
> + struct scatterlist *sg, *new_sg;
> +
> + new_table = kzalloc(sizeof(*new_table), GFP_KERNEL);
> +   

Re: Warnings with drm: Replace drm_modeset_lock/unlock_all with DRM_MODESET_LOCK_ALL_* helpers

2020-08-14 Thread Daniel Vetter
On Thu, Aug 13, 2020 at 11:17:11PM -0400, Alex Deucher wrote:
> The non-atomic modesetting code in amdgpu now spews warnings[1] with
> this patch applied.  I haven't really paged the legacy locking stuff
> into my head in quite a while.  Thoughts on how to proceed?

Uh that mess.

So the problem (and it's really the same for atomic as for legacy) is that
we have shared state between connector probe and modeset code. Generally
that's solved by just copying stuff (like drm_connector->display_info) and
hoping we don't race badly.

All the probe state is protected by dev->mode_config.mutex, while all the
modeset stuff is protected by the various drm_modeset_lock.

I think the simplest duct-tape is to reapply the old locking scheme to the
new macro, but only for non-atomic drivers. I'll type up something. But
I'll go on vacations for 2 weeks, so if it doesn't work out you have to
respin (and also merge).

Cheers, Daniel
> 
> Thanks,
> 
> Alex
> 
> commit 9bcaa3fe58ab7559e71df798bcff6e0795158695
> Author: Michal Orzel 
> Date:   Tue Apr 28 19:10:04 2020 +0200
> 
> drm: Replace drm_modeset_lock/unlock_all with DRM_MODESET_LOCK_ALL_* 
> helpers
> 
> As suggested by the TODO list for the kernel DRM subsystem, replace
> the deprecated functions that take/drop modeset locks with new helpers.
> 
> Signed-off-by: Michal Orzel 
> Signed-off-by: Daniel Vetter 
> Link: 
> https://patchwork.freedesktop.org/patch/msgid/1588093804-30446-1-git-send-email-michalorzel@gmail.com
> 
> 
> [1] https://gitlab.freedesktop.org/drm/amd/-/issues/1224
> ___
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel


[PATCH 2/4] drm/etnaviv: call perf_reg_read(..)

2020-08-14 Thread Christian Gmeiner
Replace the open coded access pattern with a function call.

Signed-off-by: Christian Gmeiner 
---
 drivers/gpu/drm/etnaviv/etnaviv_perfmon.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_perfmon.c 
b/drivers/gpu/drm/etnaviv/etnaviv_perfmon.c
index 1f0402f7a7de..782732e6ce72 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_perfmon.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_perfmon.c
@@ -58,8 +58,7 @@ static u32 pipe_perf_reg_read(struct etnaviv_gpu *gpu,
clock &= ~(VIVS_HI_CLOCK_CONTROL_DEBUG_PIXEL_PIPE__MASK);
clock |= VIVS_HI_CLOCK_CONTROL_DEBUG_PIXEL_PIPE(i);
gpu_write(gpu, VIVS_HI_CLOCK_CONTROL, clock);
-   gpu_write(gpu, domain->profile_config, signal->data);
-   value += gpu_read(gpu, domain->profile_read);
+   value += perf_reg_read(gpu, domain, signal);
}
 
/* switch back to pixel pipe 0 to prevent GPU hang */
-- 
2.26.2

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel


[PATCH 0/4] drm/etnaviv: add total hi bandwidth perf counters

2020-08-14 Thread Christian Gmeiner
This little patch set adds support for the total bandwidth used by HI. The
basic hi bandwidth read-out is quite simple but I needed to add some little
clean-ups to make it nice looking.

Christian Gmeiner (4):
  drm/etnaviv: rename pipe_reg_read(..)
  drm/etnaviv: call perf_reg_read(..)
  drm/etnaviv: add total hi bandwidth perfcounter
  drm/etnaviv: add pipe_select(..) helper

 drivers/gpu/drm/etnaviv/etnaviv_perfmon.c | 78 ---
 1 file changed, 55 insertions(+), 23 deletions(-)

-- 
2.26.2

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel


[PATCH 3/4] drm/etnaviv: add total hi bandwidth perfcounter

2020-08-14 Thread Christian Gmeiner
These two perf counters represent the total read and write
GPU bandwidth in terms of 64bits.

The used sequence was taken from Vivante kernel driver.

Signed-off-by: Christian Gmeiner 
---
 drivers/gpu/drm/etnaviv/etnaviv_perfmon.c | 35 ++-
 1 file changed, 34 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_perfmon.c 
b/drivers/gpu/drm/etnaviv/etnaviv_perfmon.c
index 782732e6ce72..b37459f022d7 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_perfmon.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_perfmon.c
@@ -69,6 +69,29 @@ static u32 pipe_perf_reg_read(struct etnaviv_gpu *gpu,
return value;
 }
 
+static u32 pipe_reg_read(struct etnaviv_gpu *gpu,
+   const struct etnaviv_pm_domain *domain,
+   const struct etnaviv_pm_signal *signal)
+{
+   u32 clock = gpu_read(gpu, VIVS_HI_CLOCK_CONTROL);
+   u32 value = 0;
+   unsigned i;
+
+   for (i = 0; i < gpu->identity.pixel_pipes; i++) {
+   clock &= ~(VIVS_HI_CLOCK_CONTROL_DEBUG_PIXEL_PIPE__MASK);
+   clock |= VIVS_HI_CLOCK_CONTROL_DEBUG_PIXEL_PIPE(i);
+   gpu_write(gpu, VIVS_HI_CLOCK_CONTROL, clock);
+   value += gpu_read(gpu, signal->data);
+   }
+
+   /* switch back to pixel pipe 0 to prevent GPU hang */
+   clock &= ~(VIVS_HI_CLOCK_CONTROL_DEBUG_PIXEL_PIPE__MASK);
+   clock |= VIVS_HI_CLOCK_CONTROL_DEBUG_PIXEL_PIPE(0);
+   gpu_write(gpu, VIVS_HI_CLOCK_CONTROL, clock);
+
+   return value;
+}
+
 static u32 hi_total_cycle_read(struct etnaviv_gpu *gpu,
const struct etnaviv_pm_domain *domain,
const struct etnaviv_pm_signal *signal)
@@ -102,8 +125,18 @@ static const struct etnaviv_pm_domain doms_3d[] = {
.name = "HI",
.profile_read = VIVS_MC_PROFILE_HI_READ,
.profile_config = VIVS_MC_PROFILE_CONFIG2,
-   .nr_signals = 5,
+   .nr_signals = 7,
.signal = (const struct etnaviv_pm_signal[]) {
+   {
+   "TOTAL_READ_BYTES8",
+   VIVS_HI_PROFILE_READ_BYTES8,
+   _reg_read,
+   },
+   {
+   "TOTAL_WRITE_BYTES8",
+   VIVS_HI_PROFILE_WRITE_BYTES8,
+   _reg_read,
+   },
{
"TOTAL_CYCLES",
0,
-- 
2.26.2

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel


[PATCH 1/4] drm/etnaviv: rename pipe_reg_read(..)

2020-08-14 Thread Christian Gmeiner
pipe_reg_read(..) iterates over all pixel pipes, selects a perf counter
register and sums the actual perf counter value. Rename the function
to reflect more what it is actual doing.

Signed-off-by: Christian Gmeiner 
---
 drivers/gpu/drm/etnaviv/etnaviv_perfmon.c | 30 +++
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_perfmon.c 
b/drivers/gpu/drm/etnaviv/etnaviv_perfmon.c
index 75f9db8f7bec..1f0402f7a7de 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_perfmon.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_perfmon.c
@@ -46,7 +46,7 @@ static u32 perf_reg_read(struct etnaviv_gpu *gpu,
return gpu_read(gpu, domain->profile_read);
 }
 
-static u32 pipe_reg_read(struct etnaviv_gpu *gpu,
+static u32 pipe_perf_reg_read(struct etnaviv_gpu *gpu,
const struct etnaviv_pm_domain *domain,
const struct etnaviv_pm_signal *signal)
 {
@@ -141,22 +141,22 @@ static const struct etnaviv_pm_domain doms_3d[] = {
{
"PIXEL_COUNT_KILLED_BY_COLOR_PIPE",

VIVS_MC_PROFILE_CONFIG0_PE_PIXEL_COUNT_KILLED_BY_COLOR_PIPE,
-   _reg_read
+   _perf_reg_read
},
{
"PIXEL_COUNT_KILLED_BY_DEPTH_PIPE",

VIVS_MC_PROFILE_CONFIG0_PE_PIXEL_COUNT_KILLED_BY_DEPTH_PIPE,
-   _reg_read
+   _perf_reg_read
},
{
"PIXEL_COUNT_DRAWN_BY_COLOR_PIPE",

VIVS_MC_PROFILE_CONFIG0_PE_PIXEL_COUNT_DRAWN_BY_COLOR_PIPE,
-   _reg_read
+   _perf_reg_read
},
{
"PIXEL_COUNT_DRAWN_BY_DEPTH_PIPE",

VIVS_MC_PROFILE_CONFIG0_PE_PIXEL_COUNT_DRAWN_BY_DEPTH_PIPE,
-   _reg_read
+   _perf_reg_read
}
}
},
@@ -184,32 +184,32 @@ static const struct etnaviv_pm_domain doms_3d[] = {
{
"VS_INST_COUNTER",
VIVS_MC_PROFILE_CONFIG0_SH_VS_INST_COUNTER,
-   _reg_read
+   _perf_reg_read
},
{
"RENDERED_VERTICE_COUNTER",

VIVS_MC_PROFILE_CONFIG0_SH_RENDERED_VERTICE_COUNTER,
-   _reg_read
+   _perf_reg_read
},
{
"VTX_BRANCH_INST_COUNTER",

VIVS_MC_PROFILE_CONFIG0_SH_VTX_BRANCH_INST_COUNTER,
-   _reg_read
+   _perf_reg_read
},
{
"VTX_TEXLD_INST_COUNTER",

VIVS_MC_PROFILE_CONFIG0_SH_VTX_TEXLD_INST_COUNTER,
-   _reg_read
+   _perf_reg_read
},
{
"PXL_BRANCH_INST_COUNTER",

VIVS_MC_PROFILE_CONFIG0_SH_PXL_BRANCH_INST_COUNTER,
-   _reg_read
+   _perf_reg_read
},
{
"PXL_TEXLD_INST_COUNTER",

VIVS_MC_PROFILE_CONFIG0_SH_PXL_TEXLD_INST_COUNTER,
-   _reg_read
+   _perf_reg_read
}
}
},
@@ -237,17 +237,17 @@ static const struct etnaviv_pm_domain doms_3d[] = {
{
"DEPTH_CLIPPED_COUNTER",

VIVS_MC_PROFILE_CONFIG1_PA_DEPTH_CLIPPED_COUNTER,
-   _reg_read
+   _perf_reg_read
},
{
"TRIVIAL_REJECTED_COUNTER",

VIVS_MC_PROFILE_CONFIG1_PA_TRIVIAL_REJECTED_COUNTER,
-   _reg_read
+   _perf_reg_read
},
{
"CULLED_COUNTER",
VIVS_MC_PROFILE_CONFIG1_PA_CULLED_COUNTER,
-   _reg_read
+   _perf_reg_read
}
}
},
@@ -400,7 +400,7 @@ static const 

[PATCH 4/4] drm/etnaviv: add pipe_select(..) helper

2020-08-14 Thread Christian Gmeiner
Replace the open coded pixel pipe selection pattern with a function.

Signed-off-by: Christian Gmeiner 
---
 drivers/gpu/drm/etnaviv/etnaviv_perfmon.c | 24 +++
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_perfmon.c 
b/drivers/gpu/drm/etnaviv/etnaviv_perfmon.c
index b37459f022d7..bafdfe49c1d8 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_perfmon.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_perfmon.c
@@ -46,6 +46,14 @@ static u32 perf_reg_read(struct etnaviv_gpu *gpu,
return gpu_read(gpu, domain->profile_read);
 }
 
+static inline void pipe_select(struct etnaviv_gpu *gpu, u32 clock, unsigned 
pipe)
+{
+   clock &= ~(VIVS_HI_CLOCK_CONTROL_DEBUG_PIXEL_PIPE__MASK);
+   clock |= VIVS_HI_CLOCK_CONTROL_DEBUG_PIXEL_PIPE(pipe);
+
+   gpu_write(gpu, VIVS_HI_CLOCK_CONTROL, clock);
+}
+
 static u32 pipe_perf_reg_read(struct etnaviv_gpu *gpu,
const struct etnaviv_pm_domain *domain,
const struct etnaviv_pm_signal *signal)
@@ -55,16 +63,12 @@ static u32 pipe_perf_reg_read(struct etnaviv_gpu *gpu,
unsigned i;
 
for (i = 0; i < gpu->identity.pixel_pipes; i++) {
-   clock &= ~(VIVS_HI_CLOCK_CONTROL_DEBUG_PIXEL_PIPE__MASK);
-   clock |= VIVS_HI_CLOCK_CONTROL_DEBUG_PIXEL_PIPE(i);
-   gpu_write(gpu, VIVS_HI_CLOCK_CONTROL, clock);
+   pipe_select(gpu, clock, i);
value += perf_reg_read(gpu, domain, signal);
}
 
/* switch back to pixel pipe 0 to prevent GPU hang */
-   clock &= ~(VIVS_HI_CLOCK_CONTROL_DEBUG_PIXEL_PIPE__MASK);
-   clock |= VIVS_HI_CLOCK_CONTROL_DEBUG_PIXEL_PIPE(0);
-   gpu_write(gpu, VIVS_HI_CLOCK_CONTROL, clock);
+   pipe_select(gpu, clock, 0);
 
return value;
 }
@@ -78,16 +82,12 @@ static u32 pipe_reg_read(struct etnaviv_gpu *gpu,
unsigned i;
 
for (i = 0; i < gpu->identity.pixel_pipes; i++) {
-   clock &= ~(VIVS_HI_CLOCK_CONTROL_DEBUG_PIXEL_PIPE__MASK);
-   clock |= VIVS_HI_CLOCK_CONTROL_DEBUG_PIXEL_PIPE(i);
-   gpu_write(gpu, VIVS_HI_CLOCK_CONTROL, clock);
+   pipe_select(gpu, clock, i);
value += gpu_read(gpu, signal->data);
}
 
/* switch back to pixel pipe 0 to prevent GPU hang */
-   clock &= ~(VIVS_HI_CLOCK_CONTROL_DEBUG_PIXEL_PIPE__MASK);
-   clock |= VIVS_HI_CLOCK_CONTROL_DEBUG_PIXEL_PIPE(0);
-   gpu_write(gpu, VIVS_HI_CLOCK_CONTROL, clock);
+   pipe_select(gpu, clock, 0);
 
return value;
 }
-- 
2.26.2

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel


Re: [PATCH v4 2/2] drm: panel: Add tianma nt36672a panel driver

2020-08-14 Thread Sumit Semwal
Hello Sam,

Thanks very much for the review.
On Fri, 14 Aug 2020 at 01:16, Sam Ravnborg  wrote:
>
> Hi Sumit.
>
> On Tue, Aug 11, 2020 at 11:51:07PM +0530, Sumit Semwal wrote:
> > Some Poco F1 phones have an LCD panel from Tianma, model nt36672a,
> > with a resolution of 1080x2246 that operates in DSI video mode.
> >
> > Add the drm panel driver for it.
> >
> > During testing, Benni Steini  helped us fix
> > the reset sequence timing (from 10ms to 20ms), to get the bootanimation
> > to work on Android.
> >
> > With current AOSP, we need to increase it to 200ms - this seems to be a
> > safe high value to avoid a white screen occasionally.
> >
> > Signed-off-by: Sumit Semwal 
> > Cc: Benni Steini 
>
> Checkpatch is not happy with this patch.
> Please fix relevant warnings.
>
> My checkpatch options:
> -q --emacs --strict --show-types
>
> A lot of details in the following.
> Sorry for not catching these before.
>
> Sam
>
> >
> > ---
> > v2: increase reset sequence timing to a safe 200ms
> > v4: Since "0425662fdf05: drm: Nuke mode->vrefresh", we have to calculate
> > vrefresh on demand. Update for it.
> > ---
> >  MAINTAINERS   |   7 +
> >  drivers/gpu/drm/panel/Kconfig |  11 +
> >  drivers/gpu/drm/panel/Makefile|   1 +
> >  drivers/gpu/drm/panel/panel-tianma-nt36672a.c | 858 ++
> >  4 files changed, 877 insertions(+)
> >  create mode 100644 drivers/gpu/drm/panel/panel-tianma-nt36672a.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index f77df02e4121..9e4bc8da9b2d 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -5611,6 +5611,13 @@ T: git git://anongit.freedesktop.org/drm/drm-misc
> >  F:   Documentation/devicetree/bindings/display/ste,mcde.txt
> >  F:   drivers/gpu/drm/mcde/
> >
> > +DRM DRIVER FOR TIANMA NT36672A PANELS
> > +M:   Sumit Semwal 
> > +S:   Maintained
> > +T:   git git://anongit.freedesktop.org/drm/drm-misc
> > +F:   
> > Documentation/devicetree/bindings/display/panel/tianma,nt36672a-panel.yaml
> > +F:   drivers/gpu/drm/panel/panel-tianma-nt36672a.c
> > +
> >  DRM DRIVER FOR TDFX VIDEO CARDS
> >  S:   Orphan / Obsolete
> >  F:   drivers/gpu/drm/tdfx/
> > diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
> > index de2f2a452be5..8108a682dcb0 100644
> > --- a/drivers/gpu/drm/panel/Kconfig
> > +++ b/drivers/gpu/drm/panel/Kconfig
> > @@ -437,6 +437,17 @@ config DRM_PANEL_TPO_TD043MTEA1
> > Say Y here if you want to enable support for TPO TD043MTEA1 800x480
> > 4.3" panel (found on the OMAP3 Pandora board).
> >
> > +config DRM_PANEL_TIANMA_FHD_NT36672A
> > + tristate "TIANMA NT36672A panel"
> > + depends on OF
> > + depends on DRM_MIPI_DSI
> > + depends on BACKLIGHT_CLASS_DEVICE
> > + help
> > +   Say Y here if you want to enable support for the Tianma NT36672A
> > +   panel. It is seen mostly in Xiaomi Poco F1 mobile phone.
> > +   The panel has a 1080x2246 resolution and uses 24 bit RGB per pixel.
> > +   It provides a MIPI DSI interface to the host.
> > +
> >  config DRM_PANEL_TPO_TPG110
> >   tristate "TPO TPG 800x400 panel"
> >   depends on OF && SPI && GPIOLIB
> > diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
> > index e45ceac6286f..472ae9ba8788 100644
> > --- a/drivers/gpu/drm/panel/Makefile
> > +++ b/drivers/gpu/drm/panel/Makefile
> > @@ -44,6 +44,7 @@ obj-$(CONFIG_DRM_PANEL_SITRONIX_ST7703) += 
> > panel-sitronix-st7703.o
> >  obj-$(CONFIG_DRM_PANEL_SITRONIX_ST7789V) += panel-sitronix-st7789v.o
> >  obj-$(CONFIG_DRM_PANEL_SONY_ACX424AKP) += panel-sony-acx424akp.o
> >  obj-$(CONFIG_DRM_PANEL_SONY_ACX565AKM) += panel-sony-acx565akm.o
> > +obj-$(CONFIG_DRM_PANEL_TIANMA_FHD_NT36672A) += panel-tianma-nt36672a.o
> >  obj-$(CONFIG_DRM_PANEL_TPO_TD028TTEC1) += panel-tpo-td028ttec1.o
> >  obj-$(CONFIG_DRM_PANEL_TPO_TD043MTEA1) += panel-tpo-td043mtea1.o
> >  obj-$(CONFIG_DRM_PANEL_TPO_TPG110) += panel-tpo-tpg110.o
> > diff --git a/drivers/gpu/drm/panel/panel-tianma-nt36672a.c 
> > b/drivers/gpu/drm/panel/panel-tianma-nt36672a.c
> > new file mode 100644
> > index ..2941975e039c
> > --- /dev/null
> > +++ b/drivers/gpu/drm/panel/panel-tianma-nt36672a.c
> > @@ -0,0 +1,858 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright (C) 2020 Linaro Ltd
> > + * Author: Sumit Semwal 
> > + */
> > +
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +
> > +#include 
> > +#include 
> > +#include 
> > +
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +
> > +#include 
> > +
> > +struct panel_cmd {
> > + size_t len;
> > + const char *data;
> > +};
> > +
> > +#define _INIT_CMD(...) { \
> > + .len = sizeof((char[]){__VA_ARGS__}), \
> > + .data = (char[]){__VA_ARGS__} }
> > +
> > +static const char * const regulator_names[] = {
> > + "vddio",
> > + "vddpos",
> > + "vddneg",
> > +};
> > 

Re: [BUG] drm/ttm: Warning during suspend/freeze

2020-08-14 Thread Christian König

Hi Thomas,

well I added a new assertion to catch cases when a driver tries to kmap 
a BO without grabbing the lock.


Looks like QXL is doing exactly that here.

Regards,
Christian.

Am 14.08.20 um 10:08 schrieb Thomas Gleixner:

Suspending or freezing a KVM guest triggers the following warning
reliably on current mainline:

[56691.550343] printk: Suspending console(s) (use no_console_suspend to debug)
[56691.578735] WARNING: CPU: 37 PID: 462 at drivers/gpu/drm/ttm/ttm_tt.c:51 
ttm_tt_create+0xb6/0xe0 [ttm]
[56692.795234] Modules linked in: snd_hda_codec_generic(E) qxl(E) 
drm_ttm_helper(E) ttm(E) snd_hda_intel(E) snd_intel_dspcfg(E) snd_hda_codec(E) 
drm_kms_helper(E) snd_hwdep(E) snd_hda_core(E) cec(E) snd_pcm(E) snd_timer(E) 
drm(E) joydev
(E) snd(E) pcspkr(E) sg(E) evdev(E) virtio_balloon(E) serio_raw(E) 
virtio_console(E) soundcore(E) button(E) virtio_rng(E) rng_core(E) ip_tables(E) 
x_tables(E) autofs4(E) ext4(E) crc32c_generic(E) crc16(E) mbcache(E) jbd2(E) 
hid_generic(E) usbhid(E) hid(E) virtio_net(E) net_failover(E) failover(E) 
uhci_hcd(E) virtio_blk(E) sr_mod(E) cdrom(E) ata_generic(E) ehci_pci(E) 
ata_piix(E) ehci_hcd(E) libata(E) virtio_pci(E) usbcore(E) psmouse(E) 
virtio_ring(E) scsi_mod(E) i2c_piix4(E) virtio(E) floppy(E)
[56691.578781] CPU: 37 PID: 462 Comm: kworker/37:1 Tainted: GE 
5.8.0+ #8217
[56691.578784] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 
1.12.0-1 04/01/2014
[56692.795348] Workqueue: events drm_fb_helper_dirty_work [drm_kms_helper]
[56691.578872] RIP: 0010:ttm_tt_create+0xb6/0xe0 [ttm]
[56691.578903] Call Trace:
[56691.578912]  ttm_bo_kmap+0x13c/0x260 [ttm]
[56691.578942]  qxl_bo_kmap+0x40/0x70 [qxl]
[56691.578947]  qxl_gem_prime_vmap+0x21/0x50 [qxl]
[56691.579060]  drm_gem_vmap+0x1f/0x50 [drm]
[56691.579073]  drm_client_buffer_vmap+0x1c/0x30 [drm]
[56691.579083]  drm_fb_helper_dirty_work+0xb2/0x1c0 [drm_kms_helper]
[56691.579091]  process_one_work+0x246/0x580
[56691.579099]  ? process_one_work+0x580/0x580
[56691.579101]  worker_thread+0x30/0x370
[56691.579104]  ? process_one_work+0x580/0x580
[56691.579107]  kthread+0x12a/0x140
[56691.579110]  ? kthread_park+0x80/0x80
[56691.579118]  ret_from_fork+0x22/0x30

Have not had time to figure out whether this is a regression or an older
issue. If you need further info please let me know.

Thanks,

 tglx


___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel


Re: [PATCH 4/6] drm/rockchip: dw_hdmi: Add vendor hdmi properties

2020-08-14 Thread Pekka Paalanen
On Thu, 13 Aug 2020 13:45:22 +0300
Laurent Pinchart  wrote:

> On Thu, Aug 13, 2020 at 10:42:28AM +0300, Pekka Paalanen wrote:
> > On Wed, 12 Aug 2020 16:30:17 +0300 Laurent Pinchart wrote:  
> > > On Wed, Aug 12, 2020 at 07:08:10PM +0800, crj wrote:  
> > > > 在 2020/8/12 17:33, Laurent Pinchart 写道:
> > > > > On Wed, Aug 12, 2020 at 04:35:43PM +0800, Algea Cao wrote:
> > > > >> Introduce struct dw_hdmi_property_ops in plat_data to support
> > > > >> vendor hdmi property.
> > > > >>
> > > > >> Implement hdmi vendor properties color_depth_property and
> > > > >> hdmi_output_property to config hdmi output color depth and
> > > > >> color format.
> > > > >>
> > > > >> The property "hdmi_output_format", the possible value
> > > > >> could be:
> > > > >>   - RGB
> > > > >>   - YCBCR 444
> > > > >>   - YCBCR 422
> > > > >>   - YCBCR 420
> > > > >>
> > > > >> Default value of the property is set to 0 = RGB, so no changes if you
> > > > >> don't set the property.
> > > > >>
> > > > >> The property "hdmi_output_depth" possible value could be
> > > > >>   - Automatic
> > > > >> This indicates prefer highest color depth, it is
> > > > >> 30bit on rockcip platform.
> > > > >>   - 24bit
> > > > >>   - 30bit
> > > > >> The default value of property is 24bit.
> > > > >>
> > > > >> Signed-off-by: Algea Cao 
> > > > >> ---
> > > > >>
> > > > >>   drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 174 
> > > > >> 
> > > > >>   include/drm/bridge/dw_hdmi.h|  22 +++
> > > > >>   2 files changed, 196 insertions(+)
> > > > >>
> > > > >> diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c 
> > > > >> b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> > > > >> index 23de359a1dec..8f22d9a566db 100644
> > > > >> --- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> > > > >> +++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> > > > >> @@ -52,6 +52,27 @@
> > > > >>   
> > > > >>   #define HIWORD_UPDATE(val, mask)   (val | (mask) << 16)
> > > > >>   
> > > > >> +/* HDMI output pixel format */
> > > > >> +enum drm_hdmi_output_type {
> > > > >> +DRM_HDMI_OUTPUT_DEFAULT_RGB, /* default RGB */
> > > > >> +DRM_HDMI_OUTPUT_YCBCR444, /* YCBCR 444 */
> > > > >> +DRM_HDMI_OUTPUT_YCBCR422, /* YCBCR 422 */
> > > > >> +DRM_HDMI_OUTPUT_YCBCR420, /* YCBCR 420 */
> > > > >> +DRM_HDMI_OUTPUT_YCBCR_HQ, /* Highest subsampled YUV */
> > > > >> +DRM_HDMI_OUTPUT_YCBCR_LQ, /* Lowest subsampled YUV */
> > > > >> +DRM_HDMI_OUTPUT_INVALID, /* Guess what ? */
> > > > >> +};
> > > > >
> > > > > Vendor-specific properties shouldn't use names starting with drm_ or
> > > > > DRM_, that's for the DRM core. But this doesn't seem specific to
> > > > > Rockchip at all, it should be a standard property. Additionally, new
> > > > > properties need to come with a userspace implementation showing their
> > > > > usage, in X.org, Weston, the Android DRM/KMS HW composer, or another
> > > > > relevant upstream project (a test tool is usually not enough).
> > > > 
> > > > We use these properties only in Android HW composer, But we can't 
> > > > upstream
> > > > 
> > > > our HW composer code right now.  Can we use this properties as private 
> > > > property
> > > > 
> > > > and do not upstream HW composer for the time being?
> > > 
> > > It's not my decision, it's a policy of the DRM subsystem to require an
> > > open implementation in userspace to validate all API additions.  
> > 
> > Also read
> > https://www.kernel.org/doc/html/latest/gpu/drm-uapi.html#open-source-userspace-requirements
> > very carefully: it calls for a FOSS userspace project's proper upstream
> > to have reviewed and accepted the patches to use the new UAPI, but
> > those patches must NOT be MERGED at that time yet.  
> 
> Correct. Many userspace projects wouldn't merge a patch before the
> kernel API is merged, so that would create a chicken and egg problem :-)

I wouldn't be so sure that absolutely everyone knows that rule. It only
takes just one userspace project to merge and release with it to
potentially require renaming everything if any change is needed after
the kernel review process.

Actually, if I remember right, I may have seen such merging happen.
After all, "accepted" is usually a synonym for "merged".


Thanks,
pq


pgp2FlATJ2zWy.pgp
Description: OpenPGP digital signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel


Re: [PATCH] MAINTAINERS: Change maintainer for hisilicon DRM driver

2020-08-14 Thread Thomas Zimmermann
Hi,

as a driver maintainer, you should have commit access to drm-misc. Head
over to

  https://drm.pages.freedesktop.org/maintainer-tools/commit-access.html

for a description of what that means. The account is requested at

  https://www.freedesktop.org/wiki/AccountRequests/

as described under 'Legacy SSH accounts'.

The first patch to commit would be your appointment as maintainer of
hisilicon. :)

Best regards
Thomas

Am 11.08.20 um 10:23 schrieb Tian Tao:
> Remove Rongrong Zou and change tiantao as hisilicon DRM maintainer.
> 
> Signed-off-by: Tian Tao 
> ---
>  MAINTAINERS | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index f12a868..f4e49e0 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -5694,7 +5694,7 @@ F:  drivers/gpu/drm/gma500/
>  
>  DRM DRIVERS FOR HISILICON
>  M:   Xinliang Liu 
> -M:   Rongrong Zou 
> +M:   Tian Tao  
>  R:   John Stultz 
>  R:   Xinwei Kong 
>  R:   Chen Feng 
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer



signature.asc
Description: OpenPGP digital signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel


Re: [RFC][PATCH v2 0/4] Support GEM object mappings from I/O memory

2020-08-14 Thread Thomas Zimmermann
cc'ing TTM maintainers for comments. We might want to use the I/O
pointer structure with TTM at some point.

Am 06.08.20 um 10:52 schrieb Thomas Zimmermann:
> DRM's fbdev console uses regular load and store operations to update
> framebuffer memory. The bochs driver on sparc64 requires the use of
> I/O-specific load and store operations. We have a workaround, but need
> a long-term solution to the problem. Previous attempts to resolve the
> issue returned an extra is_iomem flag from vmap(), or added a separate
> vmap_iomem() callback to GEM objects.
> 
> This patchset is yet another iteration with a different idea. Instead
> of a raw pointer, vmap() interfaces now return a structure that contains
> the buffer address in system or I/O memory, plus a flag that signals
> which location the address is in.
> 
> Patch #1 updates the vboxvideo driver to match the latest VRAM helpers.
> This simplifies the other patches and should be merged in any case.
> 
> Patch #2 adds struct drm_gem_membuf, which contains the pointer and flag,
> and converts the generic GEM interfaces to use it.
> 
> Patch #3 converts vmap/vunmap in GEM object functions and updates most
> GEM backends. A few drivers are still missing, but the patch should be
> acceptable for an RFC.
> 
> Patch #4 changes fbdev helpers to access framebuffer memory either with
> system or I/O memcpy functions.
> 
> Thomas Zimmermann (4):
>   drm/vboxvideo: Use drm_gem_vram_vmap() interfaces
>   drm/gem: Update client API to use struct drm_gem_membuf
>   drm/gem: Use struct drm_gem_membuf in vmap op and convert GEM backends
>   drm/fb_helper: Access framebuffer as I/O memory, if required
> 
>  drivers/gpu/drm/ast/ast_cursor.c   |  29 ++-
>  drivers/gpu/drm/ast/ast_drv.h  |   2 +-
>  drivers/gpu/drm/bochs/bochs_kms.c  |   1 -
>  drivers/gpu/drm/drm_client.c   |  25 ++-
>  drivers/gpu/drm/drm_fb_helper.c| 246 ++---
>  drivers/gpu/drm/drm_gem.c  |  28 +--
>  drivers/gpu/drm/drm_gem_cma_helper.c   |  15 +-
>  drivers/gpu/drm/drm_gem_shmem_helper.c |  31 ++--
>  drivers/gpu/drm/drm_gem_vram_helper.c  | 142 +-
>  drivers/gpu/drm/drm_internal.h |   5 +-
>  drivers/gpu/drm/drm_prime.c|  16 +-
>  drivers/gpu/drm/mgag200/mgag200_mode.c |  11 +-
>  drivers/gpu/drm/qxl/qxl_display.c  |  12 +-
>  drivers/gpu/drm/qxl/qxl_draw.c |  14 +-
>  drivers/gpu/drm/qxl/qxl_drv.h  |   6 +-
>  drivers/gpu/drm/qxl/qxl_object.c   |  19 +-
>  drivers/gpu/drm/qxl/qxl_object.h   |   2 +-
>  drivers/gpu/drm/qxl/qxl_prime.c|  12 +-
>  drivers/gpu/drm/tiny/cirrus.c  |  15 +-
>  drivers/gpu/drm/tiny/gm12u320.c|  12 +-
>  drivers/gpu/drm/udl/udl_modeset.c  |  10 +-
>  drivers/gpu/drm/vboxvideo/vbox_mode.c  |  17 +-
>  include/drm/drm_client.h   |   7 +-
>  include/drm/drm_device.h   |  26 +++
>  include/drm/drm_gem.h  |   5 +-
>  include/drm/drm_gem_cma_helper.h   |   4 +-
>  include/drm/drm_gem_shmem_helper.h |   4 +-
>  include/drm/drm_gem_vram_helper.h  |   9 +-
>  include/drm/drm_mode_config.h  |  12 --
>  29 files changed, 464 insertions(+), 273 deletions(-)
> 
> --
> 2.28.0
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer



signature.asc
Description: OpenPGP digital signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel


Re: [PATCH v2 2/4] drm/gem: Update client API to use struct drm_gem_membuf

2020-08-14 Thread Thomas Zimmermann
Hi

Am 13.08.20 um 12:26 schrieb Daniel Vetter:
> On Thu, Aug 06, 2020 at 10:52:37AM +0200, Thomas Zimmermann wrote:
>> GEM's vmap interface now wraps memory pointers in struct drm_gem_membuf.
>> The structure represents a pointer into the framebuffer, which is either
>> in I/O memory or in system memory. The structure contains a flag that
>> distinguishes these cases.
>>
>> Signed-off-by: Thomas Zimmermann 
>> ---
>>  drivers/gpu/drm/drm_client.c| 25 -
>>  drivers/gpu/drm/drm_fb_helper.c | 18 +-
>>  drivers/gpu/drm/drm_gem.c   | 19 +++
>>  drivers/gpu/drm/drm_internal.h  |  5 +++--
>>  drivers/gpu/drm/drm_prime.c | 16 ++--
>>  include/drm/drm_client.h|  7 ---
>>  include/drm/drm_device.h| 26 ++
>>  7 files changed, 75 insertions(+), 41 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c
>> index 495f47d23d87..0359b82928c1 100644
>> --- a/drivers/gpu/drm/drm_client.c
>> +++ b/drivers/gpu/drm/drm_client.c
>> @@ -234,7 +234,7 @@ static void drm_client_buffer_delete(struct 
>> drm_client_buffer *buffer)
>>  {
>>  struct drm_device *dev = buffer->client->dev;
>>  
>> -drm_gem_vunmap(buffer->gem, buffer->vaddr);
>> +drm_gem_vunmap(buffer->gem, >membuf);
>>  
>>  if (buffer->gem)
>>  drm_gem_object_put(buffer->gem);
>> @@ -302,12 +302,13 @@ drm_client_buffer_create(struct drm_client_dev 
>> *client, u32 width, u32 height, u
>>   * Returns:
>>   *  The mapped memory's address
>>   */
>> -void *drm_client_buffer_vmap(struct drm_client_buffer *buffer)
>> +const struct drm_gem_membuf *
>> +drm_client_buffer_vmap(struct drm_client_buffer *buffer)
>>  {
>> -void *vaddr;
>> +int ret;
>>  
>> -if (buffer->vaddr)
>> -return buffer->vaddr;
>> +if (buffer->membuf.vaddr)
>> +return >membuf;
>>  
>>  /*
>>   * FIXME: The dependency on GEM here isn't required, we could
>> @@ -317,13 +318,11 @@ void *drm_client_buffer_vmap(struct drm_client_buffer 
>> *buffer)
>>   * fd_install step out of the driver backend hooks, to make that
>>   * final step optional for internal users.
>>   */
>> -vaddr = drm_gem_vmap(buffer->gem);
>> -if (IS_ERR(vaddr))
>> -return vaddr;
>> -
>> -buffer->vaddr = vaddr;
>> +ret = drm_gem_vmap(buffer->gem, >membuf);
>> +if (ret)
>> +return ERR_PTR(ret);
>>  
>> -return vaddr;
>> +return >membuf;
>>  }
>>  EXPORT_SYMBOL(drm_client_buffer_vmap);
>>  
>> @@ -337,8 +336,8 @@ EXPORT_SYMBOL(drm_client_buffer_vmap);
>>   */
>>  void drm_client_buffer_vunmap(struct drm_client_buffer *buffer)
>>  {
>> -drm_gem_vunmap(buffer->gem, buffer->vaddr);
>> -buffer->vaddr = NULL;
>> +drm_gem_vunmap(buffer->gem, >membuf);
>> +buffer->membuf.vaddr = NULL;
>>  }
>>  EXPORT_SYMBOL(drm_client_buffer_vunmap);
>>  
>> diff --git a/drivers/gpu/drm/drm_fb_helper.c 
>> b/drivers/gpu/drm/drm_fb_helper.c
>> index 8697554ccd41..da24874247e7 100644
>> --- a/drivers/gpu/drm/drm_fb_helper.c
>> +++ b/drivers/gpu/drm/drm_fb_helper.c
>> @@ -394,7 +394,7 @@ static void drm_fb_helper_dirty_blit_real(struct 
>> drm_fb_helper *fb_helper,
>>  unsigned int cpp = fb->format->cpp[0];
>>  size_t offset = clip->y1 * fb->pitches[0] + clip->x1 * cpp;
>>  void *src = fb_helper->fbdev->screen_buffer + offset;
>> -void *dst = fb_helper->buffer->vaddr + offset;
>> +void *dst = fb_helper->buffer->membuf.vaddr + offset;
>>  size_t len = (clip->x2 - clip->x1) * cpp;
>>  unsigned int y;
>>  
>> @@ -416,7 +416,7 @@ static void drm_fb_helper_dirty_work(struct work_struct 
>> *work)
>>  struct drm_clip_rect *clip = >dirty_clip;
>>  struct drm_clip_rect clip_copy;
>>  unsigned long flags;
>> -void *vaddr;
>> +const struct drm_gem_membuf *buf;
>>  
>>  spin_lock_irqsave(>dirty_lock, flags);
>>  clip_copy = *clip;
>> @@ -429,8 +429,8 @@ static void drm_fb_helper_dirty_work(struct work_struct 
>> *work)
>>  
>>  /* Generic fbdev uses a shadow buffer */
>>  if (helper->buffer) {
>> -vaddr = drm_client_buffer_vmap(helper->buffer);
>> -if (IS_ERR(vaddr))
>> +buf = drm_client_buffer_vmap(helper->buffer);
>> +if (IS_ERR(buf))
>>  return;
>>  drm_fb_helper_dirty_blit_real(helper, _copy);
>>  }
>> @@ -2076,7 +2076,7 @@ static int drm_fb_helper_generic_probe(struct 
>> drm_fb_helper *fb_helper,
>>  struct drm_framebuffer *fb;
>>  struct fb_info *fbi;
>>  u32 format;
>> -void *vaddr;
>> +const struct drm_gem_membuf *membuf;
>>  
>>  drm_dbg_kms(dev, "surface width(%d), height(%d) and bpp(%d)\n",
>>  sizes->surface_width, sizes->surface_height,
>> @@ -2112,11 +2112,11 @@ static int 

[RFC][PATCH v2 2/2] dma-heap: Add a system-uncached heap

2020-08-14 Thread John Stultz
This adds a heap that allocates non-contiguous buffers that are
marked as writecombined, so they are not cached by the CPU.

This is useful, as most graphics buffers are usually not touched
by the CPU or only written into once by the CPU. So when mapping
the buffer over and over between devices, we can skip the CPU
syncing, which saves a lot of cache management overhead, greatly
improving performance.

For folk using ION, there was a ION_FLAG_CACHED flag, which
signaled if the returned buffer should be CPU cacheable or not.
With DMA-BUF heaps, we have no such flag, and by default the
current heaps (system and cma) produce CPU cachable buffers.
So for folks transitioning from ION to DMA-BUF Heaps, this fills
in some of that missing functionality.

This does have a few "ugly" bits that were required to get
the buffer properly flushed out initially which I'd like to
improve. So feedback would be very welcome!

Many thanks to Liam Mark for his help to get this working.

Cc: Sumit Semwal 
Cc: Andrew F. Davis 
Cc: Benjamin Gaignard 
Cc: Liam Mark 
Cc: Laura Abbott 
Cc: Brian Starkey 
Cc: Hridya Valsaraju 
Cc: Robin Murphy 
Cc: linux-me...@vger.kernel.org
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: John Stultz 
---
v2:
* Fix build issue on sh reported-by: kernel test robot 
* Rework to use for_each_sgtable_sg(), dma_map_sgtable(), and
  for_each_sg_page() along with numerous other cleanups suggested
  by Robin Murphy
---
 drivers/dma-buf/heaps/Kconfig|  10 +
 drivers/dma-buf/heaps/Makefile   |   1 +
 drivers/dma-buf/heaps/system_uncached_heap.c | 371 +++
 3 files changed, 382 insertions(+)
 create mode 100644 drivers/dma-buf/heaps/system_uncached_heap.c

diff --git a/drivers/dma-buf/heaps/Kconfig b/drivers/dma-buf/heaps/Kconfig
index a5eef06c4226..420b0ed0a512 100644
--- a/drivers/dma-buf/heaps/Kconfig
+++ b/drivers/dma-buf/heaps/Kconfig
@@ -5,6 +5,16 @@ config DMABUF_HEAPS_SYSTEM
  Choose this option to enable the system dmabuf heap. The system heap
  is backed by pages from the buddy allocator. If in doubt, say Y.
 
+config DMABUF_HEAPS_SYSTEM_UNCACHED
+   bool "DMA-BUF Uncached System Heap"
+   depends on DMABUF_HEAPS
+   help
+ Choose this option to enable the uncached system dmabuf heap. This
+ heap is backed by pages from the buddy allocator, but pages are setup
+ for write combining. This avoids cache management overhead, and can
+ be faster if pages are mostly untouched by the cpu.  If in doubt,
+ say Y.
+
 config DMABUF_HEAPS_CMA
bool "DMA-BUF CMA Heap"
depends on DMABUF_HEAPS && DMA_CMA
diff --git a/drivers/dma-buf/heaps/Makefile b/drivers/dma-buf/heaps/Makefile
index 6e54cdec3da0..085685ec478f 100644
--- a/drivers/dma-buf/heaps/Makefile
+++ b/drivers/dma-buf/heaps/Makefile
@@ -1,4 +1,5 @@
 # SPDX-License-Identifier: GPL-2.0
 obj-y  += heap-helpers.o
 obj-$(CONFIG_DMABUF_HEAPS_SYSTEM)  += system_heap.o
+obj-$(CONFIG_DMABUF_HEAPS_SYSTEM_UNCACHED) += system_uncached_heap.o
 obj-$(CONFIG_DMABUF_HEAPS_CMA) += cma_heap.o
diff --git a/drivers/dma-buf/heaps/system_uncached_heap.c 
b/drivers/dma-buf/heaps/system_uncached_heap.c
new file mode 100644
index ..3b8e699bcae7
--- /dev/null
+++ b/drivers/dma-buf/heaps/system_uncached_heap.c
@@ -0,0 +1,371 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Uncached System DMA-Heap exporter
+ *
+ * Copyright (C) 2020 Linaro Ltd.
+ *
+ * Based off of Andrew Davis' SRAM heap:
+ * Copyright (C) 2019 Texas Instruments Incorporated - http://www.ti.com/
+ * Andrew F. Davis 
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+struct uncached_heap {
+   struct dma_heap *heap;
+};
+
+struct uncached_heap_buffer {
+   struct dma_heap *heap;
+   struct list_head attachments;
+   struct mutex lock;
+   unsigned long len;
+   struct sg_table sg_table;
+   int vmap_cnt;
+   void *vaddr;
+};
+
+struct dma_heap_attachment {
+   struct device *dev;
+   struct sg_table *table;
+   struct list_head list;
+};
+
+static struct sg_table *dup_sg_table(struct sg_table *table)
+{
+   struct sg_table *new_table;
+   int ret, i;
+   struct scatterlist *sg, *new_sg;
+
+   new_table = kzalloc(sizeof(*new_table), GFP_KERNEL);
+   if (!new_table)
+   return ERR_PTR(-ENOMEM);
+
+   ret = sg_alloc_table(new_table, table->nents, GFP_KERNEL);
+   if (ret) {
+   kfree(new_table);
+   return ERR_PTR(-ENOMEM);
+   }
+
+   new_sg = new_table->sgl;
+   for_each_sgtable_sg(table, sg, i) {
+   sg_set_page(new_sg, sg_page(sg), sg->length, sg->offset);
+   new_sg = sg_next(new_sg);
+   }
+
+   return new_table;
+}
+
+static int dma_heap_attach(struct dma_buf *dmabuf,
+  struct 

[RFC][PATCH v2 1/2] dma-heap: Keep track of the heap device struct

2020-08-14 Thread John Stultz
Keep track of the heap device struct.

This will be useful for special DMA allocations
and actions.

Cc: Sumit Semwal 
Cc: Andrew F. Davis 
Cc: Benjamin Gaignard 
Cc: Liam Mark 
Cc: Laura Abbott 
Cc: Brian Starkey 
Cc: Hridya Valsaraju 
Cc: Robin Murphy 
Cc: linux-me...@vger.kernel.org
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: John Stultz 
---
 drivers/dma-buf/dma-heap.c | 33 +
 include/linux/dma-heap.h   |  9 +
 2 files changed, 34 insertions(+), 8 deletions(-)

diff --git a/drivers/dma-buf/dma-heap.c b/drivers/dma-buf/dma-heap.c
index afd22c9dbdcf..72c746755d89 100644
--- a/drivers/dma-buf/dma-heap.c
+++ b/drivers/dma-buf/dma-heap.c
@@ -30,6 +30,7 @@
  * @heap_devt  heap device node
  * @list   list head connecting to list of heaps
  * @heap_cdev  heap char device
+ * @heap_dev   heap device struct
  *
  * Represents a heap of memory from which buffers can be made.
  */
@@ -40,6 +41,7 @@ struct dma_heap {
dev_t heap_devt;
struct list_head list;
struct cdev heap_cdev;
+   struct device *heap_dev;
 };
 
 static LIST_HEAD(heap_list);
@@ -190,10 +192,21 @@ void *dma_heap_get_drvdata(struct dma_heap *heap)
return heap->priv;
 }
 
+/**
+ * dma_heap_get_dev() - get device struct for the heap
+ * @heap: DMA-Heap to retrieve device struct from
+ *
+ * Returns:
+ * The device struct for the heap.
+ */
+struct device *dma_heap_get_dev(struct dma_heap *heap)
+{
+   return heap->heap_dev;
+}
+
 struct dma_heap *dma_heap_add(const struct dma_heap_export_info *exp_info)
 {
struct dma_heap *heap, *h, *err_ret;
-   struct device *dev_ret;
unsigned int minor;
int ret;
 
@@ -247,16 +260,20 @@ struct dma_heap *dma_heap_add(const struct 
dma_heap_export_info *exp_info)
goto err1;
}
 
-   dev_ret = device_create(dma_heap_class,
-   NULL,
-   heap->heap_devt,
-   NULL,
-   heap->name);
-   if (IS_ERR(dev_ret)) {
+   heap->heap_dev = device_create(dma_heap_class,
+  NULL,
+  heap->heap_devt,
+  NULL,
+  heap->name);
+   if (IS_ERR(heap->heap_dev)) {
pr_err("dma_heap: Unable to create device\n");
-   err_ret = ERR_CAST(dev_ret);
+   err_ret = ERR_CAST(heap->heap_dev);
goto err2;
}
+
+   /* Make sure it doesn't disappear on us */
+   heap->heap_dev = get_device(heap->heap_dev);
+
/* Add heap to the list */
mutex_lock(_list_lock);
list_add(>list, _list);
diff --git a/include/linux/dma-heap.h b/include/linux/dma-heap.h
index 454e354d1ffb..82857e096910 100644
--- a/include/linux/dma-heap.h
+++ b/include/linux/dma-heap.h
@@ -50,6 +50,15 @@ struct dma_heap_export_info {
  */
 void *dma_heap_get_drvdata(struct dma_heap *heap);
 
+/**
+ * dma_heap_get_dev() - get device struct for the heap
+ * @heap: DMA-Heap to retrieve device struct from
+ *
+ * Returns:
+ * The device struct for the heap.
+ */
+struct device *dma_heap_get_dev(struct dma_heap *heap);
+
 /**
  * dma_heap_add - adds a heap to dmabuf heaps
  * @exp_info:  information needed to register this heap
-- 
2.17.1

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel


Re: [PATCH 18/24] drm/virtio: implement blob resources: hypercall interface

2020-08-14 Thread kernel test robot
Hi Gurchetan,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on drm-intel/for-linux-next]
[also build test WARNING on drm-tip/drm-tip linus/master next-20200814]
[cannot apply to tegra-drm/drm/tegra/for-next drm/drm-next 
drm-exynos/exynos-drm-next v5.8]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:
https://github.com/0day-ci/linux/commits/Gurchetan-Singh/Blob-prerequisites-blob-resources/20200814-104250
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
config: parisc-randconfig-s031-20200813 (attached as .config)
compiler: hppa-linux-gcc (GCC) 9.3.0
reproduce:
wget 
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O 
~/bin/make.cross
chmod +x ~/bin/make.cross
# apt-get install sparse
# sparse version: v0.6.2-168-g9554805c-dirty
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross C=1 
CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=parisc 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot 


sparse warnings: (new ones prefixed by >>)

   drivers/gpu/drm/virtio/virtgpu_vq.c:1210:23: sparse: sparse: incorrect type 
in assignment (different base types) @@ expected restricted __le64 
[usertype] offset @@ got unsigned long long [usertype] offset @@
   drivers/gpu/drm/virtio/virtgpu_vq.c:1210:23: sparse: expected restricted 
__le64 [usertype] offset
   drivers/gpu/drm/virtio/virtgpu_vq.c:1210:23: sparse: got unsigned long 
long [usertype] offset
>> drivers/gpu/drm/virtio/virtgpu_vq.c:1286:35: sparse: sparse: incorrect type 
>> in assignment (different base types) @@ expected restricted __le32 @@
>>  got restricted __le64 [usertype] @@
>> drivers/gpu/drm/virtio/virtgpu_vq.c:1286:35: sparse: expected restricted 
>> __le32
>> drivers/gpu/drm/virtio/virtgpu_vq.c:1286:35: sparse: got restricted 
>> __le64 [usertype]

vim +1286 drivers/gpu/drm/virtio/virtgpu_vq.c

  1260  
  1261  void virtio_gpu_cmd_set_scanout_blob(struct virtio_gpu_device *vgdev,
  1262   uint32_t scanout_id,
  1263   struct virtio_gpu_object *bo,
  1264   struct drm_framebuffer *fb,
  1265   uint32_t width, uint32_t height,
  1266   uint32_t x, uint32_t y)
  1267  {
  1268  uint32_t i;
  1269  struct virtio_gpu_set_scanout_blob *cmd_p;
  1270  struct virtio_gpu_vbuffer *vbuf;
  1271  uint32_t format = 
virtio_gpu_translate_format(fb->format->format);
  1272  
  1273  cmd_p = virtio_gpu_alloc_cmd(vgdev, , sizeof(*cmd_p));
  1274  memset(cmd_p, 0, sizeof(*cmd_p));
  1275  
  1276  cmd_p->hdr.type = cpu_to_le32(VIRTIO_GPU_CMD_SET_SCANOUT_BLOB);
  1277  cmd_p->resource_id = cpu_to_le32(bo->hw_res_handle);
  1278  cmd_p->scanout_id = cpu_to_le32(scanout_id);
  1279  
  1280  cmd_p->format = cpu_to_le32(format);
  1281  cmd_p->width  = cpu_to_le32(fb->width);
  1282  cmd_p->height = cpu_to_le32(fb->height);
  1283  
  1284  for (i = 0; i < 4; i++) {
  1285  cmd_p->strides[i] = cpu_to_le32(fb->pitches[i]);
> 1286  cmd_p->offsets[i] = cpu_to_le64(fb->offsets[i]);

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org


.config.gz
Description: application/gzip
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel