date:20220603


Any one has any comments?

Thanks,

On 5/27/2022 2:32 PM, Kuogee Hsieh wrote:

During display resolution changes display have to be disabled first
followed by display enabling with new resolution. Display disable
will turn off both pixel clock and main link clock so that main link
have to be re trained during display enable to have new video stream
flow again. At current implementation, display enable function manually
kicks up irq_hpd_handle which will read panel link status and start link
training if link status is not in sync state. However, there is rare
case that a particular panel links status keep staying in sync for
some period of time after main link had been shut down previously at
display disabled. Main link retraining will not be executed by
irq_hdp_handle() if the link status read from pane shows it is in
sync state. If this was happen, then video stream of newer display
resolution will fail to be transmitted to panel due to main link is
not in sync between host and panel. This patch force main link always
be retrained during display enable procedure to prevent this rare
failed case from happening. Also this implementation are more
efficient than manual kicking off irq_hpd_handle function.

Changes in v2:
-- set force_link_train flag on DP only (is_edp == false)

Changes in v3:
-- revise commit  text
-- add Fixes tag

Changes in v4:
-- revise commit  text

Fixes: 62671d2ef24b ("drm/msm/dp: fixes wrong connection state caused by failure of 
link train")
Signed-off-by: Kuogee Hsieh 
---
  drivers/gpu/drm/msm/dp/dp_ctrl.c|  6 +++---
  drivers/gpu/drm/msm/dp/dp_ctrl.h|  2 +-
  drivers/gpu/drm/msm/dp/dp_display.c | 15 ---
  3 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
index af7a80c..bea93eb 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
@@ -1551,7 +1551,7 @@ static int dp_ctrl_process_phy_test_request(struct 
dp_ctrl_private *ctrl)
  
  	ret = dp_ctrl_on_link(>dp_ctrl);

if (!ret)
-   ret = dp_ctrl_on_stream(>dp_ctrl);
+   ret = dp_ctrl_on_stream(>dp_ctrl, false);
else
DRM_ERROR("failed to enable DP link controller\n");
  
@@ -1807,7 +1807,7 @@ static int dp_ctrl_link_retrain(struct dp_ctrl_private *ctrl)

return dp_ctrl_setup_main_link(ctrl, _step);
  }
  
-int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl)

+int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl, bool force_link_train)
  {
int ret = 0;
bool mainlink_ready = false;
@@ -1848,7 +1848,7 @@ int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl)
return 0;
}
  
-	if (!dp_ctrl_channel_eq_ok(ctrl))

+   if (force_link_train || !dp_ctrl_channel_eq_ok(ctrl))
dp_ctrl_link_retrain(ctrl);
  
  	/* stop txing train pattern to end link training */

diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.h b/drivers/gpu/drm/msm/dp/dp_ctrl.h
index 0745fde..b563e2e 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.h
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.h
@@ -21,7 +21,7 @@ struct dp_ctrl {
  };
  
  int dp_ctrl_on_link(struct dp_ctrl *dp_ctrl);

-int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl);
+int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl, bool force_link_train);
  int dp_ctrl_off_link_stream(struct dp_ctrl *dp_ctrl);
  int dp_ctrl_off_link(struct dp_ctrl *dp_ctrl);
  int dp_ctrl_off(struct dp_ctrl *dp_ctrl);
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c 
b/drivers/gpu/drm/msm/dp/dp_display.c
index c388323..370348d 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -872,7 +872,7 @@ static int dp_display_enable(struct dp_display_private *dp, 
u32 data)
return 0;
}
  
-	rc = dp_ctrl_on_stream(dp->ctrl);

+   rc = dp_ctrl_on_stream(dp->ctrl, data);
if (!rc)
dp_display->power_on = true;
  
@@ -1654,6 +1654,7 @@ void dp_bridge_enable(struct drm_bridge *drm_bridge)

int rc = 0;
struct dp_display_private *dp_display;
u32 state;
+   bool force_link_train = false;
  
  	dp_display = container_of(dp, struct dp_display_private, dp_display);

if (!dp_display->dp_mode.drm_mode.clock) {
@@ -1688,10 +1689,14 @@ void dp_bridge_enable(struct drm_bridge *drm_bridge)
  
  	state =  dp_display->hpd_state;
  
-	if (state == ST_DISPLAY_OFF)

+   if (state == ST_DISPLAY_OFF) {
dp_display_host_phy_init(dp_display);
  
-	dp_display_enable(dp_display, 0);

+   if (!dp->is_edp)
+   force_link_train = true;
+   }
+
+   dp_display_enable(dp_display, force_link_train);
  
  	rc = dp_display_post_enable(dp);

if (rc) {
@@ -1700,10 +1705,6 @@ void dp_bridge_enable(struct drm_bridge *drm_bridge)
dp_display_unprepare(dp);
}
  
-	/* manual kick off plug event to train link */

-   if (state == ST_DISPLAY_OFF)
-

[PATCH v1 2/2] drm/msm/dp: check core_initialized before disable interrupts at dp_display_unbind()

During msm initialize phase, dp_display_unbind() will be called to undo
initializations had been done by dp_display_bind() previously if there is
error happen at msm_drm_bind. In this case, core_initialized flag had to
be check to make sure clocks is on before update DP controller register
to disable HPD interrupts. Otherwise system will crash due to below NOC
fatal error.

QTISECLIB [01f01a7ad]CNOC2 ERROR: ERRLOG0_LOW = 0x00061007
QTISECLIB [01f01a7ad]GEM_NOC ERROR: ERRLOG0_LOW = 0x1007
QTISECLIB [01f0371a0]CNOC2 ERROR: ERRLOG0_HIGH = 0x0003
QTISECLIB [01f055297]GEM_NOC ERROR: ERRLOG0_HIGH = 0x0003
QTISECLIB [01f072beb]CNOC2 ERROR: ERRLOG1_LOW = 0x0024
QTISECLIB [01f0914b8]GEM_NOC ERROR: ERRLOG1_LOW = 0x0042
QTISECLIB [01f0ae639]CNOC2 ERROR: ERRLOG1_HIGH = 0x4002
QTISECLIB [01f0cc73f]GEM_NOC ERROR: ERRLOG1_HIGH = 0x4002
QTISECLIB [01f0ea092]CNOC2 ERROR: ERRLOG2_LOW = 0x0009020c
QTISECLIB [01f10895f]GEM_NOC ERROR: ERRLOG2_LOW = 0x0ae9020c
QTISECLIB [01f125ae1]CNOC2 ERROR: ERRLOG2_HIGH = 0x
QTISECLIB [01f143be7]GEM_NOC ERROR: ERRLOG2_HIGH = 0x
QTISECLIB [01f16153a]CNOC2 ERROR: ERRLOG3_LOW = 0x
QTISECLIB [01f17fe07]GEM_NOC ERROR: ERRLOG3_LOW = 0x
QTISECLIB [01f19cf89]CNOC2 ERROR: ERRLOG3_HIGH = 0x
QTISECLIB [01f1bb08e]GEM_NOC ERROR: ERRLOG3_HIGH = 0x
QTISECLIB [01f1d8a31]CNOC2 ERROR: SBM1 FAULTINSTATUS0_LOW = 0x0002
QTISECLIB [01f1f72a4]GEM_NOC ERROR: SBM0 FAULTINSTATUS0_LOW = 0x0001
QTISECLIB [01f21a217]CNOC3 ERROR: ERRLOG0_LOW = 0x0006
QTISECLIB [01f23dfd3]NOC error fatal

Signed-off-by: Kuogee Hsieh 
---
 drivers/gpu/drm/msm/dp/dp_display.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c 
b/drivers/gpu/drm/msm/dp/dp_display.c
index 69afb25..af233c9 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -309,7 +309,8 @@ static void dp_display_unbind(struct device *dev, struct 
device *master,
struct msm_drm_private *priv = dev_get_drvdata(master);
 
/* disable all HPD interrupts */
-   dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_INT_MASK, false);
+   if (dp->core_initialized)
+   dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_INT_MASK, 
false);
 
kthread_stop(dp->ev_tsk);
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v1 1/2] drm/msm: enable msm irq after all initializations are done successfully at msm_drm_init()

At msm initialize phase, msm_drm_init() is called to initialize modules
sequentially. It will call msm_drm_uninit() to clean up initialized phase
if any module initialization return failed. This patch move msm_irq_install()
to the last step of msm_drm_init() after all modules are initialized 
successfully
so that no msm_irq_unistall() required at msm_drm_uninit() if any module
initialization failed happen at msm_drm_init().

Signed-off-by: Kuogee Hsieh 
---
 drivers/gpu/drm/msm/msm_drv.c | 29 ++---
 1 file changed, 6 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 6665c5a..ab42e9a 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -132,15 +132,6 @@ static int msm_irq_install(struct drm_device *dev, 
unsigned int irq)
return 0;
 }
 
-static void msm_irq_uninstall(struct drm_device *dev)
-{
-   struct msm_drm_private *priv = dev->dev_private;
-   struct msm_kms *kms = priv->kms;
-
-   kms->funcs->irq_uninstall(kms);
-   free_irq(kms->irq, dev);
-}
-
 struct msm_vblank_work {
struct work_struct work;
int crtc_id;
@@ -232,10 +223,6 @@ static int msm_drm_uninit(struct device *dev)
 
drm_mode_config_cleanup(ddev);
 
-   pm_runtime_get_sync(dev);
-   msm_irq_uninstall(ddev);
-   pm_runtime_put_sync(dev);
-
if (kms && kms->funcs)
kms->funcs->destroy(kms);
 
@@ -437,16 +424,6 @@ static int msm_drm_init(struct device *dev, const struct 
drm_driver *drv)
goto err_msm_uninit;
}
 
-   if (kms) {
-   pm_runtime_get_sync(dev);
-   ret = msm_irq_install(ddev, kms->irq);
-   pm_runtime_put_sync(dev);
-   if (ret < 0) {
-   DRM_DEV_ERROR(dev, "failed to install IRQ handler\n");
-   goto err_msm_uninit;
-   }
-   }
-
ret = drm_dev_register(ddev, 0);
if (ret)
goto err_msm_uninit;
@@ -467,6 +444,12 @@ static int msm_drm_init(struct device *dev, const struct 
drm_driver *drv)
if (ret)
goto err_msm_uninit;
 
+   if (kms) {
+   pm_runtime_get_sync(dev);
+   msm_irq_install(ddev, kms->irq);
+   pm_runtime_put_sync(dev);
+   }
+
drm_kms_helper_poll_init(ddev);
 
return 0;
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v1 0/2] Fix NOC fatal error if msm_drm_init() failed

system crash due to NOC fatal error if any module initialization faield
during msm_drm_init()

Kuogee Hsieh (2):
  drm/msm: enable msm irq after all initializations are done
successfully at msm_drm_init()
  drm/msm/dp: check core_initialized before disable interrupts at
dp_display_unbind()

 drivers/gpu/drm/msm/dp/dp_display.c |  3 ++-
 drivers/gpu/drm/msm/msm_drv.c   | 29 ++---
 2 files changed, 8 insertions(+), 24 deletions(-)

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v2 2/2] drm/virtio: fence created per cursor/plane update

2022-06-03 Thread Dongwon Kim

Having one fence for a vgfb would cause conflict in case there are
multiple planes referencing the same vgfb (e.g. Xorg screen covering
two displays in extended mode) being flushed simultaneously. So it makes
sence to use a separated fence for each plane update to prevent this.

vgfb->fence is not required anymore with the suggested code change so
both prepare_fb and cleanup_fb are removed since only fence creation/
freeing are done in there.

v2: - use the fence always as long as guest_blob is enabled on the
  scanout object
- obj and fence initialized as NULL ptrs to avoid uninitialzed
  ptr problem (Reported by Dan Carpenter/kernel-test-robot)

Reported-by: kernel test robot 
Reported-by: Dan Carpenter 
Cc: Gurchetan Singh 
Cc: Gerd Hoffmann 
Cc: Vivek Kasireddy 
Signed-off-by: Dongwon Kim 
---
 drivers/gpu/drm/virtio/virtgpu_drv.h   |   1 -
 drivers/gpu/drm/virtio/virtgpu_plane.c | 103 ++---
 2 files changed, 39 insertions(+), 65 deletions(-)

diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h 
b/drivers/gpu/drm/virtio/virtgpu_drv.h
index 0a194aaad419..4c59c1e67ca5 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.h
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
@@ -186,7 +186,6 @@ struct virtio_gpu_output {
 
 struct virtio_gpu_framebuffer {
struct drm_framebuffer base;
-   struct virtio_gpu_fence *fence;
 };
 #define to_virtio_gpu_framebuffer(x) \
container_of(x, struct virtio_gpu_framebuffer, base)
diff --git a/drivers/gpu/drm/virtio/virtgpu_plane.c 
b/drivers/gpu/drm/virtio/virtgpu_plane.c
index 6d3cc9e238a4..821023b7d57d 100644
--- a/drivers/gpu/drm/virtio/virtgpu_plane.c
+++ b/drivers/gpu/drm/virtio/virtgpu_plane.c
@@ -137,29 +137,37 @@ static void virtio_gpu_resource_flush(struct drm_plane 
*plane,
struct virtio_gpu_device *vgdev = dev->dev_private;
struct virtio_gpu_framebuffer *vgfb;
struct virtio_gpu_object *bo;
+   struct virtio_gpu_object_array *objs = NULL;
+   struct virtio_gpu_fence *fence = NULL;
 
vgfb = to_virtio_gpu_framebuffer(plane->state->fb);
bo = gem_to_virtio_gpu_obj(vgfb->base.obj[0]);
-   if (vgfb->fence) {
-   struct virtio_gpu_object_array *objs;
 
+   if (!bo)
+   return;
+
+   if (bo->dumb && bo->guest_blob)
+   fence = virtio_gpu_fence_alloc(vgdev, vgdev->fence_drv.context,
+  0);
+
+   if (fence) {
objs = virtio_gpu_array_alloc(1);
-   if (!objs)
+   if (!objs) {
+   kfree(fence);
return;
+   }
virtio_gpu_array_add_obj(objs, vgfb->base.obj[0]);
virtio_gpu_array_lock_resv(objs);
-   virtio_gpu_cmd_resource_flush(vgdev, bo->hw_res_handle, x, y,
- width, height, objs, vgfb->fence);
-   virtio_gpu_notify(vgdev);
+   }
+
+   virtio_gpu_cmd_resource_flush(vgdev, bo->hw_res_handle, x, y,
+ width, height, objs, fence);
+   virtio_gpu_notify(vgdev);
 
-   dma_fence_wait_timeout(>fence->f, true,
+   if (fence) {
+   dma_fence_wait_timeout(>f, true,
   msecs_to_jiffies(50));
-   dma_fence_put(>fence->f);
-   vgfb->fence = NULL;
-   } else {
-   virtio_gpu_cmd_resource_flush(vgdev, bo->hw_res_handle, x, y,
- width, height, NULL, NULL);
-   virtio_gpu_notify(vgdev);
+   dma_fence_put(>f);
}
 }
 
@@ -239,47 +247,6 @@ static void virtio_gpu_primary_plane_update(struct 
drm_plane *plane,
  rect.y2 - rect.y1);
 }
 
-static int virtio_gpu_plane_prepare_fb(struct drm_plane *plane,
-  struct drm_plane_state *new_state)
-{
-   struct drm_device *dev = plane->dev;
-   struct virtio_gpu_device *vgdev = dev->dev_private;
-   struct virtio_gpu_framebuffer *vgfb;
-   struct virtio_gpu_object *bo;
-
-   if (!new_state->fb)
-   return 0;
-
-   vgfb = to_virtio_gpu_framebuffer(new_state->fb);
-   bo = gem_to_virtio_gpu_obj(vgfb->base.obj[0]);
-   if (!bo || (plane->type == DRM_PLANE_TYPE_PRIMARY && !bo->guest_blob))
-   return 0;
-
-   if (bo->dumb && (plane->state->fb != new_state->fb)) {
-   vgfb->fence = virtio_gpu_fence_alloc(vgdev, 
vgdev->fence_drv.context,
-0);
-   if (!vgfb->fence)
-   return -ENOMEM;
-   }
-
-   return 0;
-}
-
-static void virtio_gpu_plane_cleanup_fb(struct drm_plane *plane,
-   struct drm_plane_state *old_state)
-{
-   struct virtio_gpu_framebuffer *vgfb;
-
-   if (!plane->state->fb)
-

[PATCH v2 1/2] drm/virtio: .release ops for virtgpu fence release

2022-06-03 Thread Dongwon Kim

virtio_gpu_fence_release is added to free virtio-gpu-fence
upon release of dma_fence.

Cc: Gurchetan Singh 
Cc: Gerd Hoffmann 
Cc: Vivek Kasireddy 
Signed-off-by: Dongwon Kim 
---
 drivers/gpu/drm/virtio/virtgpu_fence.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/virtio/virtgpu_fence.c 
b/drivers/gpu/drm/virtio/virtgpu_fence.c
index f28357dbde35..ba659ac2a51d 100644
--- a/drivers/gpu/drm/virtio/virtgpu_fence.c
+++ b/drivers/gpu/drm/virtio/virtgpu_fence.c
@@ -63,12 +63,20 @@ static void virtio_gpu_timeline_value_str(struct dma_fence 
*f, char *str,
 (u64)atomic64_read(>drv->last_fence_id));
 }
 
+static void virtio_gpu_fence_release(struct dma_fence *f)
+{
+   struct virtio_gpu_fence *fence = to_virtio_gpu_fence(f);
+
+   kfree(fence);
+}
+
 static const struct dma_fence_ops virtio_gpu_fence_ops = {
.get_driver_name = virtio_gpu_get_driver_name,
.get_timeline_name   = virtio_gpu_get_timeline_name,
.signaled= virtio_gpu_fence_signaled,
.fence_value_str = virtio_gpu_fence_value_str,
.timeline_value_str  = virtio_gpu_timeline_value_str,
+   .release = virtio_gpu_fence_release,
 };
 
 struct virtio_gpu_fence *virtio_gpu_fence_alloc(struct virtio_gpu_device 
*vgdev,
-- 
2.20.1

[PATCH v2 0/2] drm/virtio: fence handling in case of multi scanouts

2022-06-03 Thread Dongwon Kim

Current primary plane update flow when blob is enabled (for zero copy
display sharing) shows fence synchronization problems when multi planes
are referencing a same single large FB (i.e. multi displays in extended
mode). This is because there is only one fence bound to the FB and this
single fence is re-used asynchronously when flushing all associated
planes.

The way to prevent this is to assign the fence for each plane so that
flushing one plane won't be affecting or affected by other plane's flush
operation.

The 1st patch "drm/virtio: .release ops for virtgpu fence release" which
adds device specific release ops is for making the virtio_gpu fence freed
upon the last dma_fence_put call.

The 2nd patch "drm/virtio: fence created per cursor/plane update" contains
the main implementation of per-plane fence.

Dongwon Kim (2):
  drm/virtio: .release ops for virtgpu fence release
  drm/virtio: fence created per cursor/plane update

 drivers/gpu/drm/virtio/virtgpu_drv.h   |   1 -
 drivers/gpu/drm/virtio/virtgpu_fence.c |   8 ++
 drivers/gpu/drm/virtio/virtgpu_plane.c | 103 ++---
 3 files changed, 47 insertions(+), 65 deletions(-)

-- 
2.20.1

[PATCH] dt-bindings: msm: update maintainers list with proper id

Use quic id instead of codeaurora id in maintainers list
for display devicetree bindings.

Signed-off-by: Kuogee Hsieh 
---
 Documentation/devicetree/bindings/display/msm/dp-controller.yaml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml 
b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
index cd05cfd..c950710 100644
--- a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
+++ b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
@@ -7,7 +7,7 @@ $schema: http://devicetree.org/meta-schemas/core.yaml#
 title: MSM Display Port Controller
 
 maintainers:
-  - Kuogee Hsieh 
+  - Kuogee Hsieh 
 
 description: |
   Device tree bindings for DisplayPort host controller for MSM targets
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

XDC 2022: Registration & Call for Presentations still open!

2022-06-03 Thread Lyude Paul

Hello! This is just a reminder that the CFP for XDC in 2022 is still open!

The 2022 X.Org Developers Conference is being held in conjunction with
the 2022 Wine Developers Conference.  This is a meeting to bring
together developers working on all things open graphics (Linux kernel,
Mesa, DRM, Wayland, X11, etc.) as well as developers for the Wine
Project, a key consumer of open graphics.

Registration & Call for Proposals are now open for both XDC 2022 and
WineConf 2022, which will take place on October 4-6, 2022 in
Minneapolis, Minnesota, USA. 

https://xdc2022.x.org
 
As usual, the conference is free of charge and open to the general
public. If you plan on attending, please make sure to register as early
as possible!
 
In order to register as attendee, you will therefore need to do it via
the XDC website:
 
https://indico.freedesktop.org/event/2/registrations/2/
 
In addition to registration, the CfP is now open for talks, workshops
and demos at XDC 2022. While any serious proposal will be gratefully
considered, topics of interest to X.Org and freedesktop.org developers
are encouraged. The program focus is on new development, ongoing
challenges and anything else that will spark discussions among
attendees in the hallway track.
 
We are open to talks across all layers of the graphics stack, from the
kernel to desktop environments / graphical applications and about how
to make things better for the developers who build them. Head to the
CfP page to learn more: 
 
https://indico.freedesktop.org/event/2/abstracts/
 
The deadline for submissions is Monday July 4th, 2022.
 
Check out our Reimbursement Policy to accept speaker
expenses for X.Org events like XDC 2022:
 
https://www.x.org/wiki/XorgFoundation/Policies/Reimbursement/

If you have any questions, please send me an email to
x...@codeweavers.com, adding on CC the X.org board (board
at foundation.x.org).
 
And don't forget, you can follow us on Twitter for all the latest
updates and to stay connected:
 
https://twitter.com/XOrgDevConf

Best regards,
Lyude Paul, on behalf of X.org


-- 
Cheers,
 Lyude Paul (she/her)
 Software Engineer at Red Hat

XDC 2022: Registration & Call for Presentations still open!

2022-06-03 Thread Lyude Paul

Hello! This is just a reminder that the CFP for XDC in 2022 is still open!

The 2022 X.Org Developers Conference is being held in conjunction with
the 2022 Wine Developers Conference.  This is a meeting to bring
together developers working on all things open graphics (Linux kernel,
Mesa, DRM, Wayland, X11, etc.) as well as developers for the Wine
Project, a key consumer of open graphics.

Registration & Call for Proposals are now open for both XDC 2022 and
WineConf 2022, which will take place on October 4-6, 2022 in
Minneapolis, Minnesota, USA. 

https://xdc2022.x.org
 
As usual, the conference is free of charge and open to the general
public. If you plan on attending, please make sure to register as early
as possible!
 
In order to register as attendee, you will therefore need to do it via
the XDC website:
 
https://indico.freedesktop.org/event/2/registrations/2/
 
In addition to registration, the CfP is now open for talks, workshops
and demos at XDC 2022. While any serious proposal will be gratefully
considered, topics of interest to X.Org and freedesktop.org developers
are encouraged. The program focus is on new development, ongoing
challenges and anything else that will spark discussions among
attendees in the hallway track.
 
We are open to talks across all layers of the graphics stack, from the
kernel to desktop environments / graphical applications and about how
to make things better for the developers who build them. Head to the
CfP page to learn more: 
 
https://indico.freedesktop.org/event/2/abstracts/
 
The deadline for submissions is Monday July 4th, 2022.
 
Check out our Reimbursement Policy to accept speaker
expenses for X.Org events like XDC 2022:
 
https://www.x.org/wiki/XorgFoundation/Policies/Reimbursement/

If you have any questions, please send me an email to
x...@codeweavers.com, adding on CC the X.org board (board
at foundation.x.org).
 
And don't forget, you can follow us on Twitter for all the latest
updates and to stay connected:
 
https://twitter.com/XOrgDevConf

Best regards,
Lyude Paul, on behalf of X.org


-- 
Cheers,
 Lyude Paul (she/her)
 Software Engineer at Red Hat

Re: [PATCH] drm/ttm: fix missing NULL check in ttm_device_swapout

2022-06-03 Thread Felix Kuehling




On 2022-06-03 06:46, Christian König wrote:

Resources about to be destructed are not tied to BOs any more.
I've been seeing a backtrace in that area with a patch series I'm 
working on, but didn't have enough time to track it down yet. I'll try 
if this patch fixes it.


Regards,
  Felix




Signed-off-by: Christian König 
---
  drivers/gpu/drm/ttm/ttm_device.c | 6 +-
  1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/ttm/ttm_device.c b/drivers/gpu/drm/ttm/ttm_device.c
index a0562ab386f5..e7147e304637 100644
--- a/drivers/gpu/drm/ttm/ttm_device.c
+++ b/drivers/gpu/drm/ttm/ttm_device.c
@@ -156,8 +156,12 @@ int ttm_device_swapout(struct ttm_device *bdev, struct 
ttm_operation_ctx *ctx,
  
  		ttm_resource_manager_for_each_res(man, , res) {

struct ttm_buffer_object *bo = res->bo;
-   uint32_t num_pages = PFN_UP(bo->base.size);
+   uint32_t num_pages;
  
+			if (!bo)

+   continue;
+
+   num_pages = PFN_UP(bo->base.size);
ret = ttm_bo_swapout(bo, ctx, gfp_flags);
/* ttm_bo_swapout has dropped the lru_lock */
if (!ret)

Re: [RFC PATCH 3/5] drm/amdgpu: Allow explicit sync for VM ops.

On Fri, Jun 3, 2022 at 8:41 PM Christian König  wrote:
>
> Am 03.06.22 um 19:50 schrieb Bas Nieuwenhuizen:
> > [SNIP]
>  Yeah, but that's exactly the bubble we try to avoid. Isn't it?
> >>> For this series, not really.  To clarify there are two sides for
> >>> getting GPU bubbles and no overlap:
> >>>
> >>> (1) VM operations implicitly wait for earlier CS submissions
> >>> (2) CS submissions implicitly wait for earlier VM operations
> >>>
> >>> Together, these combine to ensure that you get a (potentially small)
> >>> bubble any time VM work happens.
> >>>
> >>> Your series (and further ideas) tackles (2), and is a worthwhile thing
> >>> to do. However, while writing the userspace for this I noticed this
> >>> isn't enough to get rid of all our GPU bubbles. In particular when
> >>> doing a non-sparse map of a new BO, that tends to need to be waited on
> >>> for the next CS anyway for API semantics. Due to VM operations
> >>> happening on a single timeline that means this high priority map can
> >>> end up being blocked by earlier sparse maps and hence the bubble in
> >>> that case still exists.
> >>>
> >>> So in this series I try to tackle (1) instead. Since GPU work
> >>> typically lags behind CPU submissions and VM operations aren't that
> >>> slow, we can typically execute VM operations early enough that any
> >>> implicit syncs from (2) are less/no issue.
> >> Ok, once more since you don't seem to understand what I want to say: It
> >> isn't possible to fix #1 before you have fixed #2.
> >>
> >> The VM unmap operation here is a barrier which divides the CS operations
> >> in a before and after. This is intentional design.
> > Why is that barrier needed? The two barriers I got and understood and
> > I think we can deal with:
> >
> > 1) the VM unmap is a barrier between prior CS and later memory free.
> > 2) The TLB flush need to happen between a VM unmap and later CS.
> >
> > But why do we need the VM unmap to be a strict barrier between prior
> > CS and later CS?
>
> Exactly because of the two reasons you mentioned.

This is the part I'm not seeing. I get that removing #2 is a
nightmare, which is why I did something that doesn't violate that
constraint.

Like if an explicit CS that was running before the VM operation  runs
till after the VM operation (and hence possibly till after the TLB
flush, or otherwise have the TLB flush not apply due to lack of async
TLB flush support), that is not an issue. It might see the state from
before the unmap, or after the unmap, or some intermediate state and
all of those would be okay.

We still get the constraint that the TLB flush happens between the VM
unmap and later CS and hence the unmap is certainly visible to them.

>
> #1 Is rather easy to fix, you just need to copy all dma_fences from the
> page table dma_resv object over to the BOs dma_resv object in the gem
> close handler. E.g. exactly what you suggested with the dma_resv_copy
> function.
>
> #2 is a nightmare.
>
> We can't move the TLB flush at the end of the unmap operation because on
> async TLB flushes are either a bit complicated (double flushes etc..) or
> don't even work at all because of hw bugs. So to have a reliable TLB
> flush we must make sure that nothing else is ongoing and that means
> CS->VM->CS barrier.
>
> We try very hard to circumvent that already on maps by (for example)
> using a completely new VMID for CS after the VM map operation.
>
> But for the unmap operation we would need some kind special dma_fence
> implementation which would not only wait for all existing dma_fence but
> also for the one added until the unmap operation is completed. Cause
> otherwise our operation we do at #1 would simply not catch all
> dma_fences which have access to the memory.
>
> That's certainly doable, but I think just using the drm_exec stuff I
> already came up with is easier.
>
> When we can grab locks for all the BOs involved amdgpu_vm_clear_freed()
> goes away and we can keep track of the unmap operations in the bo_va
> structure.
>
> With that done you can make the explicit sync you noted in the bo_va
> structure and implicit sync when the bo_va structure goes away.
>
> Then the only reason I can see why we would need a CS->VM dependency is
> implicit synchronization, and that's what we are trying to avoid here in
> the first place.
>
> Regards,
> Christian.
>
> >
> >> To get rid of this barrier you must first fix the part where CS
> >> submissions wait for the VM operation to complete, e.g. the necessity of
> >> the barrier.
> >>
> >> I'm working on this for a couple of years now and I'm really running out
> >> of idea how to explain this restriction.
> >>
> >> Regards,
> >> Christian.
> >>
>

Re: [git pull] drm for 5.19-rc1 part2/fixes

2022-06-03 Thread pr-tracker-bot

The pull request you sent on Fri, 3 Jun 2022 13:49:48 +1000:

> git://anongit.freedesktop.org/drm/drm tags/drm-next-2022-06-03-1

has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/ab18b7b36a82b1900687c5718f7d46f0d8e77d86

Thank you!

-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/prtracker.html

Re: [RFC PATCH 3/5] drm/amdgpu: Allow explicit sync for VM ops.


Am 03.06.22 um 19:50 schrieb Bas Nieuwenhuizen:

[SNIP]

Yeah, but that's exactly the bubble we try to avoid. Isn't it?

For this series, not really.  To clarify there are two sides for
getting GPU bubbles and no overlap:

(1) VM operations implicitly wait for earlier CS submissions
(2) CS submissions implicitly wait for earlier VM operations

Together, these combine to ensure that you get a (potentially small)
bubble any time VM work happens.

Your series (and further ideas) tackles (2), and is a worthwhile thing
to do. However, while writing the userspace for this I noticed this
isn't enough to get rid of all our GPU bubbles. In particular when
doing a non-sparse map of a new BO, that tends to need to be waited on
for the next CS anyway for API semantics. Due to VM operations
happening on a single timeline that means this high priority map can
end up being blocked by earlier sparse maps and hence the bubble in
that case still exists.

So in this series I try to tackle (1) instead. Since GPU work
typically lags behind CPU submissions and VM operations aren't that
slow, we can typically execute VM operations early enough that any
implicit syncs from (2) are less/no issue.

Ok, once more since you don't seem to understand what I want to say: It
isn't possible to fix #1 before you have fixed #2.

The VM unmap operation here is a barrier which divides the CS operations
in a before and after. This is intentional design.

Why is that barrier needed? The two barriers I got and understood and
I think we can deal with:

1) the VM unmap is a barrier between prior CS and later memory free.
2) The TLB flush need to happen between a VM unmap and later CS.

But why do we need the VM unmap to be a strict barrier between prior
CS and later CS?


Exactly because of the two reasons you mentioned.

#1 Is rather easy to fix, you just need to copy all dma_fences from the 
page table dma_resv object over to the BOs dma_resv object in the gem 
close handler. E.g. exactly what you suggested with the dma_resv_copy 
function.


#2 is a nightmare.

We can't move the TLB flush at the end of the unmap operation because on 
async TLB flushes are either a bit complicated (double flushes etc..) or 
don't even work at all because of hw bugs. So to have a reliable TLB 
flush we must make sure that nothing else is ongoing and that means 
CS->VM->CS barrier.


We try very hard to circumvent that already on maps by (for example) 
using a completely new VMID for CS after the VM map operation.


But for the unmap operation we would need some kind special dma_fence 
implementation which would not only wait for all existing dma_fence but 
also for the one added until the unmap operation is completed. Cause 
otherwise our operation we do at #1 would simply not catch all 
dma_fences which have access to the memory.


That's certainly doable, but I think just using the drm_exec stuff I 
already came up with is easier.


When we can grab locks for all the BOs involved amdgpu_vm_clear_freed() 
goes away and we can keep track of the unmap operations in the bo_va 
structure.


With that done you can make the explicit sync you noted in the bo_va 
structure and implicit sync when the bo_va structure goes away.


Then the only reason I can see why we would need a CS->VM dependency is 
implicit synchronization, and that's what we are trying to avoid here in 
the first place.


Regards,
Christian.




To get rid of this barrier you must first fix the part where CS
submissions wait for the VM operation to complete, e.g. the necessity of
the barrier.

I'm working on this for a couple of years now and I'm really running out
of idea how to explain this restriction.

Regards,
Christian.

Re: [PATCH 0/6] drm: Add mouse cursor hotspot support to atomic KMS

On Jun 3, 2022, at 11:49 AM, Simon Ser
mailto:cont...@emersion.fr>> wrote:

⚠ External Email

On Friday, June 3rd, 2022 at 17:32, Zack Rusin
mailto:za...@vmware.com>> wrote:

On Jun 3, 2022, at 11:22 AM, Simon Ser
mailto:cont...@emersion.fr>> wrote:
⚠ External Email

On Friday, June 3rd, 2022 at 17:17, Zack Rusin
mailto:za...@vmware.com>> wrote:

On Jun 3, 2022, at 10:56 AM, Simon Ser
mailto:cont...@emersion.fr>> wrote:
⚠ External Email

On Friday, June 3rd, 2022 at 16:38, Zack Rusin
mailto:za...@vmware.com>> wrote:

On Jun 3, 2022, at 10:32 AM, Simon Ser
mailto:cont...@emersion.fr>> wrote:

⚠ External Email

On Friday, June 3rd, 2022 at 16:27, Zack Rusin
mailto:za...@vmware.com>> wrote:

In particular: since the driver will ignore the KMS cursor plane
position set by user-space, I don't think it's okay to just expose
without opt-in from user-space (e.g. with a DRM_CLIENT_CAP).

cc wayland-devel and Pekka for user-space feedback.

I think Thomas expressed our concerns and reasons why those wouldn’t
work for us back then. Is there something else you’d like to add?

I disagreed, and I don't understand Thomas' reasoning.

KMS clients will need an update to work correctly. Adding a new prop
without a cap leaves existing KMS clients broken. Adding a cap allows
both existing KMS clients and updated KMS clients to work correctly.

I’m not sure what you mean here. They are broken right now. That’s what we’re
fixing. That’s the reason why the virtualized drivers are on deny-lists for
all atomic kms. So nothing needs to be updated. If you have a kms client that
was using virtualized drivers with atomic kms then mouse clicks never worked
correctly.

So, yes, clients need to set cursor hotspot if they want mouse to work
correctly with virtualized drivers.

My proposal was:

- Introduce DRM_CLIENT_CAP_CURSOR_PLANE_NO_POSITION (or a better name). Only
user-space which supports the hotspot props will enable it.
- By default, don't expose a cursor plane, because current user-space doesn't
support it (Weston might put a window in the cursor plane, and nobody can
report hotspot).
- If the KMS client enables the cap, advertise the cursor
plane, and make it so the plane doesn't have the CRTC_X/CRTC_Y properties
since the driver will pick the position.

That way both old and new user-space are fixed.

I don’t think I see how that fixes anything. In particular I don’t see a way
of fixing the old user space at all. We require hotspot info, old user space
doesn’t set it because there’s no way of setting it on atomic kms.

Old atomic user-space is fixed by removing the cursor plane. Then old
atomic user-space will fallback to drawing the cursor itself, e.g. via
OpenGL.

But it’s not fixed, because the driver is still on a deny-list and
nothing implements this. You’re saying you could potentially “fix” by
implementing client side cursor handling in all kms clients? That’s a
hard sell if the user space can just put the virtualized driver on
deny-lists and fallback to use old kms which supports hotspots.

What deny-list are you referring to?

All compositors I know of implement a fallback when no cursor plane is
usable.

I put the links in the first email in the series:
https://gitlab.gnome.org/GNOME/mutter/-/blob/main/src/backends/native/meta-kms-impl-device-atomic.c#L1188
https://invent.kde.org/plasma/kwin/-/blob/master/src/backends/drm/drm_gpu.cpp#L156

Also, let me point this out because it also seems to be a fundamental
misunderstanding, user space implementing software cursor doesn’t fix anything.
Just leaves everything broken in different ways. The reason virtualized drivers
went away from software cursors is because it makes it impossible to make it
work with a bunch of interesting and desirable scenarios, e.g. unity mode where
the guest might have a terminal and browser open and then the virtual machine
software creates windows out of those, so you don’t have the entire desktop of
the guest but instead native looking windows with the apps. In that case guest
has no way of knowing when the cursor leaves the window, so to make seemless
cursors work you’d need to implement irc or backdoors that send that
information from the host to the guest, then have virtualized drivers create
some sort of uevent, to send to the userspace to let it know to hide the cursor
because it actually left the window. That’s a trivial scenario and there’s a
lot more with mice that are remoted themselves, guests with singular or maybe
many apps exported, possibly overlapping on the host but all within a desktop
in the guest, etc.

New user-space supports setting the hotspot prop, and is aware that it can't
set the cursor plane position, so the cursor plane can be exposed again when
the cap is enabled.

But we still use cursor plane position. Hotspots are offsets from
cursor plane positions. Both are required.

No, VM drivers don't need and disregard the cursor position AFAIK. They
replace it with the host cursor's

Re: [RFC PATCH 3/5] drm/amdgpu: Allow explicit sync for VM ops.

On Fri, Jun 3, 2022 at 7:42 PM Christian König  wrote:
>
> Am 03.06.22 um 15:23 schrieb Bas Nieuwenhuizen:
> > On Fri, Jun 3, 2022 at 2:49 PM Christian König  
> > wrote:
> >> Am 03.06.22 um 14:39 schrieb Bas Nieuwenhuizen:
> >>> On Fri, Jun 3, 2022 at 2:08 PM Christian König  
> >>> wrote:
>  Am 03.06.22 um 13:07 schrieb Bas Nieuwenhuizen:
> > On Fri, Jun 3, 2022 at 12:16 PM Christian König
> >  wrote:
> >> Am 03.06.22 um 12:08 schrieb Bas Nieuwenhuizen:
> >>> [SNIP]
> > I do have to fix some stuff indeed, especially for the GEM close but
> > with that we should be able to keep the same basic approach?
>  Nope, not even remotely.
> 
>  What we need is the following:
>  1. Rolling out my drm_exec patch set, so that we can lock buffers as 
>  needed.
>  2. When we get a VM operation we not only lock the VM page tables, 
>  but
>  also all buffers we potentially need to unmap.
>  3. Nuking the freed list in the amdgpu_vm structure by updating freed
>  areas directly when they are unmapped.
>  4. Tracking those updates inside the bo_va structure for the BO+VM
>  combination.
>  5. When the bo_va structure is destroy because of closing the handle
>  move the last clear operation over to the VM as implicit sync.
> 
> >>> Hi Christian, isn't that a different problem though (that we're also
> >>> trying to solve, but in your series)?
> >>>
> >>> What this patch tries to achieve:
> >>>
> >>> (t+0) CS submission setting BOOKKEEP fences (i.e. no implicit sync)
> >>> (t+1) a VM operation on a BO/VM accessed by the CS.
> >>>
> >>> to run concurrently. What it *doesn't* try is
> >>>
> >>> (t+0) a VM operation on a BO/VM accessed by the CS.
> >>> (t+1) CS submission setting BOOKKEEP fences (i.e. no implicit sync)
> >>>
> >>> to run concurrently. When you write
> >>>
>  Only when all this is done we then can resolve the dependency that 
>  the
>  CS currently must wait for any clear operation on the VM.
> >>> isn't that all about the second problem?
> >> No, it's the same.
> >>
> >> See what we do in the VM code is to artificially insert a bubble so 
> >> that
> >> all VM clear operations wait for all CS operations and then use the
> >> clear fence to indicate when the backing store of the BO can be freed.
> > Isn't that remediated with something like the code below? At least the
> > gem_close case should be handled with this, and the move case was
> > already handled by the copy operation.
>  That is one necessary puzzle piece, yes. But you need more than that.
> 
>  Especially the explicit unmap operation needs to be converted into an
>  implicit unmap to get the TLB flush right.
> >>> This doesn't change anything about the TLB flush though? Since all
> >>> unmap -> later jobs dependencies are still implicit.
> >>>
> >>> So the worst what could happen (i.f. e.g. userspace gets the
> >>> waits/dependencies wrong) is
> >>>
> >>> 1) non-implicit CS gets submitted that touches a BO
> >>> 2)  VM unmap on that BO happens
> >>> 2.5) the CS from 1 is still active due to missing dependencies
> >>> 2.6) but any CS submission after 2 will trigger a TLB flush
> >> Yeah, but that's exactly the bubble we try to avoid. Isn't it?
> > For this series, not really.  To clarify there are two sides for
> > getting GPU bubbles and no overlap:
> >
> > (1) VM operations implicitly wait for earlier CS submissions
> > (2) CS submissions implicitly wait for earlier VM operations
> >
> > Together, these combine to ensure that you get a (potentially small)
> > bubble any time VM work happens.
> >
> > Your series (and further ideas) tackles (2), and is a worthwhile thing
> > to do. However, while writing the userspace for this I noticed this
> > isn't enough to get rid of all our GPU bubbles. In particular when
> > doing a non-sparse map of a new BO, that tends to need to be waited on
> > for the next CS anyway for API semantics. Due to VM operations
> > happening on a single timeline that means this high priority map can
> > end up being blocked by earlier sparse maps and hence the bubble in
> > that case still exists.
> >
> > So in this series I try to tackle (1) instead. Since GPU work
> > typically lags behind CPU submissions and VM operations aren't that
> > slow, we can typically execute VM operations early enough that any
> > implicit syncs from (2) are less/no issue.
>
> Ok, once more since you don't seem to understand what I want to say: It
> isn't possible to fix #1 before you have fixed #2.
>
> The VM unmap operation here is a barrier which divides the CS operations
> in a before and after. This is intentional design.

Why is that barrier needed? The two barriers I got and understood and
I think we can deal with:

1) the VM unmap is a barrier

Re: [RFC PATCH 3/5] drm/amdgpu: Allow explicit sync for VM ops.


Am 03.06.22 um 15:23 schrieb Bas Nieuwenhuizen:

On Fri, Jun 3, 2022 at 2:49 PM Christian König  wrote:

Am 03.06.22 um 14:39 schrieb Bas Nieuwenhuizen:

On Fri, Jun 3, 2022 at 2:08 PM Christian König  wrote:

Am 03.06.22 um 13:07 schrieb Bas Nieuwenhuizen:

On Fri, Jun 3, 2022 at 12:16 PM Christian König
 wrote:

Am 03.06.22 um 12:08 schrieb Bas Nieuwenhuizen:

[SNIP]

I do have to fix some stuff indeed, especially for the GEM close but
with that we should be able to keep the same basic approach?

Nope, not even remotely.

What we need is the following:
1. Rolling out my drm_exec patch set, so that we can lock buffers as needed.
2. When we get a VM operation we not only lock the VM page tables, but
also all buffers we potentially need to unmap.
3. Nuking the freed list in the amdgpu_vm structure by updating freed
areas directly when they are unmapped.
4. Tracking those updates inside the bo_va structure for the BO+VM
combination.
5. When the bo_va structure is destroy because of closing the handle
move the last clear operation over to the VM as implicit sync.


Hi Christian, isn't that a different problem though (that we're also
trying to solve, but in your series)?

What this patch tries to achieve:

(t+0) CS submission setting BOOKKEEP fences (i.e. no implicit sync)
(t+1) a VM operation on a BO/VM accessed by the CS.

to run concurrently. What it *doesn't* try is

(t+0) a VM operation on a BO/VM accessed by the CS.
(t+1) CS submission setting BOOKKEEP fences (i.e. no implicit sync)

to run concurrently. When you write


Only when all this is done we then can resolve the dependency that the
CS currently must wait for any clear operation on the VM.

isn't that all about the second problem?

No, it's the same.

See what we do in the VM code is to artificially insert a bubble so that
all VM clear operations wait for all CS operations and then use the
clear fence to indicate when the backing store of the BO can be freed.

Isn't that remediated with something like the code below? At least the
gem_close case should be handled with this, and the move case was
already handled by the copy operation.

That is one necessary puzzle piece, yes. But you need more than that.

Especially the explicit unmap operation needs to be converted into an
implicit unmap to get the TLB flush right.

This doesn't change anything about the TLB flush though? Since all
unmap -> later jobs dependencies are still implicit.

So the worst what could happen (i.f. e.g. userspace gets the
waits/dependencies wrong) is

1) non-implicit CS gets submitted that touches a BO
2)  VM unmap on that BO happens
2.5) the CS from 1 is still active due to missing dependencies
2.6) but any CS submission after 2 will trigger a TLB flush

Yeah, but that's exactly the bubble we try to avoid. Isn't it?

For this series, not really.  To clarify there are two sides for
getting GPU bubbles and no overlap:

(1) VM operations implicitly wait for earlier CS submissions
(2) CS submissions implicitly wait for earlier VM operations

Together, these combine to ensure that you get a (potentially small)
bubble any time VM work happens.

Your series (and further ideas) tackles (2), and is a worthwhile thing
to do. However, while writing the userspace for this I noticed this
isn't enough to get rid of all our GPU bubbles. In particular when
doing a non-sparse map of a new BO, that tends to need to be waited on
for the next CS anyway for API semantics. Due to VM operations
happening on a single timeline that means this high priority map can
end up being blocked by earlier sparse maps and hence the bubble in
that case still exists.

So in this series I try to tackle (1) instead. Since GPU work
typically lags behind CPU submissions and VM operations aren't that
slow, we can typically execute VM operations early enough that any
implicit syncs from (2) are less/no issue.


Ok, once more since you don't seem to understand what I want to say: It 
isn't possible to fix #1 before you have fixed #2.


The VM unmap operation here is a barrier which divides the CS operations 
in a before and after. This is intentional design.


To get rid of this barrier you must first fix the part where CS 
submissions wait for the VM operation to complete, e.g. the necessity of 
the barrier.


I'm working on this for a couple of years now and I'm really running out 
of idea how to explain this restriction.


Regards,
Christian.

Re: [PATCH v1 11/13] drm/edid: add HF-EEODB support to EDID read and allocation

2022-06-03 Thread Ville Syrjälä

On Tue, May 24, 2022 at 01:39:33PM +0300, Jani Nikula wrote:
> HDMI 2.1 section 10.3.6 defines an HDMI Forum EDID Extension Override
> Data Block, which may contain a different extension count than the base
> block claims. Add support for reading more EDID data if available. The
> extra blocks aren't parsed yet, though.
> 
> Signed-off-by: Jani Nikula 
> ---
>  drivers/gpu/drm/drm_edid.c | 81 --
>  1 file changed, 78 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 5e0a91da565e..ba0c880dc133 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -1581,6 +1581,15 @@ static bool version_greater(const struct drm_edid 
> *drm_edid,
>   (edid->version == version && edid->revision > revision);
>  }
>  
> +static int edid_hfeeodb_extension_block_count(const struct edid *edid);
> +
> +static int edid_hfeeodb_block_count(const struct edid *edid)
> +{
> + int eeodb = edid_hfeeodb_extension_block_count(edid);
> +
> + return eeodb ? eeodb + 1 : 0;
> +}
> +
>  static int edid_extension_block_count(const struct edid *edid)
>  {
>   return edid->extensions;
> @@ -2026,6 +2035,11 @@ static struct edid *edid_filter_invalid_blocks(struct 
> edid *edid,
>   struct edid *new;
>   int i, valid_blocks = 0;
>  
> + /*
> +  * Note: If the EDID uses HF-EEODB, but has invalid blocks, we'll revert
> +  * back to regular extension count here. We don't want to start
> +  * modifying the HF-EEODB extension too.
> +  */
>   for (i = 0; i < edid_block_count(edid); i++) {
>   const void *src_block = edid_block_data(edid, i);
>  
> @@ -2235,7 +2249,7 @@ static struct edid *_drm_do_get_edid(struct 
> drm_connector *connector,
>size_t *size)
>  {
>   enum edid_block_status status;
> - int i, invalid_blocks = 0;
> + int i, num_blocks, invalid_blocks = 0;
>   struct edid *edid, *new;
>   size_t alloc_size = EDID_LENGTH;
>  
> @@ -2277,7 +2291,8 @@ static struct edid *_drm_do_get_edid(struct 
> drm_connector *connector,
>   goto fail;
>   edid = new;
>  
> - for (i = 1; i < edid_block_count(edid); i++) {
> + num_blocks = edid_block_count(edid);
> + for (i = 1; i < num_blocks; i++) {
>   void *block = (void *)edid_block_data(edid, i);
>  
>   status = edid_block_read(block, i, read_block, context);
> @@ -2288,11 +2303,31 @@ static struct edid *_drm_do_get_edid(struct 
> drm_connector *connector,
>   if (status == EDID_BLOCK_READ_FAIL)
>   goto fail;
>   invalid_blocks++;
> + } else if (i == 1) {
> + /*
> +  * If the first EDID extension is a CTA extension, and
> +  * the first Data Block is HF-EEODB, override the
> +  * extension block count.
> +  *
> +  * Note: HF-EEODB could specify a smaller extension
> +  * count too, but we can't risk allocating a smaller
> +  * amount.
> +  */
> + int eeodb = edid_hfeeodb_block_count(edid);
> +
> + if (eeodb > num_blocks) {
> + num_blocks = eeodb;
> + alloc_size = edid_size_by_blocks(num_blocks);
> + new = krealloc(edid, alloc_size, GFP_KERNEL);
> + if (!new)
> + goto fail;
> + edid = new;
> + }
>   }
>   }
>  
>   if (invalid_blocks) {
> - connector_bad_edid(connector, edid, edid_block_count(edid));
> + connector_bad_edid(connector, edid, num_blocks);
>  
>   edid = edid_filter_invalid_blocks(edid, _size);
>   }
> @@ -3825,6 +3860,7 @@ static int add_detailed_modes(struct drm_connector 
> *connector,
>  #define CTA_EXT_DB_HDR_STATIC_METADATA   6
>  #define CTA_EXT_DB_420_VIDEO_DATA14
>  #define CTA_EXT_DB_420_VIDEO_CAP_MAP 15
> +#define CTA_EXT_DB_HF_EEODB  0x78
>  #define CTA_EXT_DB_HF_SCDB   0x79
>  
>  #define EDID_BASIC_AUDIO (1 << 6)
> @@ -4868,6 +4904,12 @@ static bool cea_db_is_hdmi_forum_vsdb(const struct 
> cea_db *db)
>   cea_db_payload_len(db) >= 7;
>  }
>  
> +static bool cea_db_is_hdmi_forum_eeodb(const void *db)
> +{
> + return cea_db_is_extended_tag(db, CTA_EXT_DB_HF_EEODB) &&
> + cea_db_payload_len(db) >= 2;
> +}
> +
>  static bool cea_db_is_microsoft_vsdb(const struct cea_db *db)
>  {
>   return cea_db_is_vendor(db, MICROSOFT_IEEE_OUI) &&
> @@ -4902,6 +4944,39 @@ static bool cea_db_is_hdmi_hdr_metadata_block(const 
> struct cea_db *db)
>   cea_db_payload_len(db) >= 3;
>  }
>

Re: [RESEND 10/14] iio: adc: mt6370: Add Mediatek MT6370 support

2022-06-03 Thread Jonathan Cameron

> >  
> > > +
> > > +#define MT6370_AICR_400MA0x6
> > > +#define MT6370_ICHG_500MA0x4
> > > +#define MT6370_ICHG_900MA0x8
> > > +
> > > +#define ADC_CONV_TIME_US 35000
> > > +#define ADC_CONV_POLLING_TIME1000
> > > +
> > > +struct mt6370_adc_data {
> > > + struct device *dev;
> > > + struct regmap *regmap;
> > > + struct mutex lock;  
> >
> > All locks need documentation.  What is the scope of the lock?
> > Looks like it protects device state when doing setup, wait for read, read
> > cycles.  
> 
> This mutex lock is for preventing the different adc channel from being
> read at the same time.
> So, if I just change its name to adc_chan_lock or adc_lock and add the
> comment for this mutex lock, does this change meet your requirement

Yes

> 
> >  
> > > +};
> > > +
> > > +static int mt6370_adc_read_scale(struct mt6370_adc_data *priv,
> > > +  int chan, int *val1, int *val2)
> > > +{
> > > + unsigned int reg_val;
> > > + int ret;
> > > +
> > > + switch (chan) {
> > > + case MT6370_CHAN_VBAT:
> > > + case MT6370_CHAN_VSYS:
> > > + case MT6370_CHAN_CHG_VDDP:
> > > + *val1 = 5000;  
> >
> > This seems very large.  Voltages are in millivolts
> > as per Documentation/ABI/testing/sysfs-bus-iio
> > and this means each step is 5 volts.
> >
> > So value in mv is currently 5 * _raw
> >  
> 
> OK, I got it. Also, I will add the ABI file in the next version. Thanks!
Only add ABI documentation for anything non-standard.

The documentation scripts really don't like repeats!

> 
> > > +static const char * const mt6370_channel_labels[MT6370_CHAN_MAX] = {  
> >
> > Perhaps define an enum with which to index this and the chan spec
> > and hence ensure they end up matching.
> >  [vbusdiv5] = "vbusdiv5", etc
> >  
> 
> Do you mean that I can refine this const char array to the following array??
> 
> static const char * const mt6370_channel_labels[MT6370_CHAN_MAX] = {
> [MT6370_CHAN_VBUSDIV5] =  "vbusdiv5",
> [MT6370_CHAN_VBUSDIV2] =  "vbusdiv2",
> ...
> ...
> [MT6370_CHAN_TEMP_JC] =  "temp_jc",
> };

Yes

thanks,

Jonathan

Re: How should "max bpc" and "Colorspace" KMS property work?

2022-06-03 Thread Ville Syrjälä

On Fri, Jun 03, 2022 at 10:19:09AM +0300, Pekka Paalanen wrote:
> On Thu, 2 Jun 2022 19:40:01 +0300
> Ville Syrjälä  wrote:
> 
> > On Thu, Jun 02, 2022 at 10:47:59AM +0300, Pekka Paalanen wrote:
> > > On Wed, 1 Jun 2022 17:06:25 +0300
> > > Ville Syrjälä  wrote:
> > >   
> 
> ...
> 
> > > > The "Colorspace" property just changes what we report to the display
> > > > via infoframes/MSA/SDP. It does not cause the display hardware to do
> > > > any colorspace conversion/etc.  
> > > 
> > > Good.
> > >   
> > > > To actually force the display hardware to do the desired conversion
> > > > we need some new properties. ATM the only automagic conversion that
> > > > can happen (at least with i915) is the RGB 4:4:4->YCbCr 4:2:0 fallback,
> > > > which is needed on some displays to get 4k+ modes to work at all.  
> > > 
> > > When "Colorspace" says RGB, and the automatic fallback would kick in to
> > > create a conflict, what happens?  
> > 
> > I would put that in the "Doctor, it hurts when I..." category.
> 
> Hi Ville,
> 
> I meant specifically the case where the driver chooses to use YCbCr
> 4:2:0 even though userspace is setting absolutely everything to RGB.
> 
> So yeah, that is a problem, like you and Sebastian agreed later.
> 
> Am I safe from that automatic driver fallback if I don't use 4k or
> bigger video modes? What about e.g. 1080p@120 or something? Can I
> calculate when the fallback will happen and choose my "Colorspace"
> accordingly? Which also requires me to set up the RGB->YCbCR color
> model conversion myself?

Porbably not something you can trivially compute unless you
replicate the exact logic from the driver.

> 
> What about failing the modeset instead if userspace sets "Colorspace"
> explicitly to RGB, and the driver would need to fall back to YCbCR
> 4:2:0?
> 
> That would make the most sense to me, as then the driver would not
> silently do a buggy thing.

I'm not sure there's much point in polishing that turd too much.
Should just add the explicit props and make userspace that actually
cares about color management set them exactly as it likes.

> > > I thought we agreed that "max bpc" means limiting link bpc to at most
> > > that value, but the driver will automatically pick a lower value if
> > > that makes the requested video mode work (and in lack of new KMS
> > > properties).
> > > 
> > > I have no desire that change that. I also think the Plymouth issue is
> > > not fully fixable without some new KMS property, and until then the
> > > only thing Plymouth could do is to smash "max bpc" always to 8 - which
> > > mostly stops being a problem once display servers learn to handle "max
> > > bpc".  
> > 
> > There's no real differene between the kernel automagically clamping
> > "max bpc" to the current BIOS programmed value vs. plymouth doing it.
> > Either approach will break deep color support for current userspace
> > which doesn't reset "max bpc" back to the max.
> 
> There is one big difference: if Plymouth does it, then people cannot
> scream "kernel regression".

But they'll just report the bugs against i915 anyway. I'd rather
not have to deal with those without at least being able to point
them at an existing fix, at least for the more common wayland
compositors+Xorg.

> People can point fingers at Plymouth, but I
> would imagine the actual fixes will come as patches to other KMS clients
> and not Plymouth.

I'm worried that we'll be stuck herding these bug reports
for quite some time.

-- 
Ville Syrjälä
Intel

[PATCH] drm/stm: ltdc: remove error message about scaling

Remove error message about scaling & replace it by a debug
message to avoid too much error.

Signed-off-by: Yannick Fertre 
---
 drivers/gpu/drm/stm/ltdc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/stm/ltdc.c b/drivers/gpu/drm/stm/ltdc.c
index eeefc3260c07..a4098aaff243 100644
--- a/drivers/gpu/drm/stm/ltdc.c
+++ b/drivers/gpu/drm/stm/ltdc.c
@@ -1215,7 +1215,8 @@ static int ltdc_plane_atomic_check(struct drm_plane 
*plane,
 
/* Reject scaling */
if (src_w != new_plane_state->crtc_w || src_h != 
new_plane_state->crtc_h) {
-   DRM_ERROR("Scaling is not supported");
+   DRM_DEBUG_DRIVER("Scaling is not supported");
+
return -EINVAL;
}
 
-- 
2.25.1

[PATCH] drm/stm: ltdc: add support of the dynamic z-order

Zpos property is immutable for all hardware versions except the last
version (0x40100) which support the blending order feature
(dynamic z-order).

Signed-off-by: Yannick Fertre 
---
 drivers/gpu/drm/stm/drv.c  |  1 +
 drivers/gpu/drm/stm/ltdc.c | 23 ---
 drivers/gpu/drm/stm/ltdc.h |  1 +
 3 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/stm/drv.c b/drivers/gpu/drm/stm/drv.c
index 0da7cce2a1a2..c63945dc2260 100644
--- a/drivers/gpu/drm/stm/drv.c
+++ b/drivers/gpu/drm/stm/drv.c
@@ -95,6 +95,7 @@ static int drv_load(struct drm_device *ddev)
ddev->mode_config.max_width = STM_MAX_FB_WIDTH;
ddev->mode_config.max_height = STM_MAX_FB_HEIGHT;
ddev->mode_config.funcs = _mode_config_funcs;
+   ddev->mode_config.normalize_zpos = true;
 
ret = ltdc_load(ddev);
if (ret)
diff --git a/drivers/gpu/drm/stm/ltdc.c b/drivers/gpu/drm/stm/ltdc.c
index 6a9f613839b5..00a6bc1b1d7c 100644
--- a/drivers/gpu/drm/stm/ltdc.c
+++ b/drivers/gpu/drm/stm/ltdc.c
@@ -194,6 +194,7 @@
 
 #define LXBFCR_BF2 GENMASK(2, 0)   /* Blending Factor 2 */
 #define LXBFCR_BF1 GENMASK(10, 8)  /* Blending Factor 1 */
+#define LXBFCR_BOR GENMASK(18, 16) /* Blending ORder */
 
 #define LXCFBLR_CFBLL  GENMASK(12, 0)  /* Color Frame Buffer Line Length */
 #define LXCFBLR_CFBP   GENMASK(28, 16) /* Color Frame Buffer Pitch in bytes */
@@ -1309,7 +1310,14 @@ static void ltdc_plane_atomic_update(struct drm_plane 
*plane,
plane->type != DRM_PLANE_TYPE_PRIMARY)
val = BF1_PAXCA | BF2_1PAXCA;
 
-   regmap_write_bits(ldev->regmap, LTDC_L1BFCR + lofs, LXBFCR_BF2 | 
LXBFCR_BF1, val);
+   if (ldev->caps.dynamic_zorder) {
+   val |= (newstate->normalized_zpos << 16);
+   regmap_write_bits(ldev->regmap, LTDC_L1BFCR + lofs,
+ LXBFCR_BF2 | LXBFCR_BF1 | LXBFCR_BOR, val);
+   } else {
+   regmap_write_bits(ldev->regmap, LTDC_L1BFCR + lofs,
+ LXBFCR_BF2 | LXBFCR_BF1, val);
+   }
 
/* Configures the frame buffer line number */
line_number = y1 - y0 + 1;
@@ -1578,7 +1586,10 @@ static int ltdc_crtc_init(struct drm_device *ddev, 
struct drm_crtc *crtc)
return -EINVAL;
}
 
-   drm_plane_create_zpos_immutable_property(primary, 0);
+   if (ldev->caps.dynamic_zorder)
+   drm_plane_create_zpos_property(primary, 0, 0, 
ldev->caps.nb_layers - 1);
+   else
+   drm_plane_create_zpos_immutable_property(primary, 0);
 
/* Init CRTC according to its hardware features */
if (ldev->caps.crc)
@@ -1607,7 +1618,10 @@ static int ltdc_crtc_init(struct drm_device *ddev, 
struct drm_crtc *crtc)
DRM_ERROR("Can not create overlay plane %d\n", i);
goto cleanup;
}
-   drm_plane_create_zpos_immutable_property(overlay, i);
+   if (ldev->caps.dynamic_zorder)
+   drm_plane_create_zpos_property(overlay, i, 0, 
ldev->caps.nb_layers - 1);
+   else
+   drm_plane_create_zpos_immutable_property(overlay, i);
}
 
return 0;
@@ -1737,6 +1751,7 @@ static int ltdc_get_caps(struct drm_device *ddev)
ldev->caps.ycbcr_output = false;
ldev->caps.plane_reg_shadow = false;
ldev->caps.crc = false;
+   ldev->caps.dynamic_zorder = false;
break;
case HWVER_20101:
ldev->caps.layer_ofs = LAY_OFS_0;
@@ -1752,6 +1767,7 @@ static int ltdc_get_caps(struct drm_device *ddev)
ldev->caps.ycbcr_output = false;
ldev->caps.plane_reg_shadow = false;
ldev->caps.crc = false;
+   ldev->caps.dynamic_zorder = false;
break;
case HWVER_40100:
ldev->caps.layer_ofs = LAY_OFS_1;
@@ -1767,6 +1783,7 @@ static int ltdc_get_caps(struct drm_device *ddev)
ldev->caps.ycbcr_output = true;
ldev->caps.plane_reg_shadow = true;
ldev->caps.crc = true;
+   ldev->caps.dynamic_zorder = true;
break;
default:
return -ENODEV;
diff --git a/drivers/gpu/drm/stm/ltdc.h b/drivers/gpu/drm/stm/ltdc.h
index 59fc5d1bbbab..4855898bd4c0 100644
--- a/drivers/gpu/drm/stm/ltdc.h
+++ b/drivers/gpu/drm/stm/ltdc.h
@@ -28,6 +28,7 @@ struct ltdc_caps {
bool ycbcr_output;  /* ycbcr output converter supported */
bool plane_reg_shadow;  /* plane shadow registers ability */
bool crc;   /* cyclic redundancy check supported */
+   bool dynamic_zorder;/* dynamic z-order */
 };
 
 #define LTDC_MAX_LAYER 4
-- 
2.25.1

Re: [PATCH 0/6] drm: Add mouse cursor hotspot support to atomic KMS



On Friday, June 3rd, 2022 at 17:32, Zack Rusin  wrote:

> > On Jun 3, 2022, at 11:22 AM, Simon Ser  wrote:
> > ⚠ External Email
> >
> > On Friday, June 3rd, 2022 at 17:17, Zack Rusin  wrote:
> >
> >
> > >
> > >
> > > > On Jun 3, 2022, at 10:56 AM, Simon Ser  wrote:
> > > > ⚠ External Email
> > > >
> > > > On Friday, June 3rd, 2022 at 16:38, Zack Rusin  wrote:
> > > >
> > > >
> > > > > > On Jun 3, 2022, at 10:32 AM, Simon Ser  wrote:
> > > > > >
> > > > > > ⚠ External Email
> > > > > >
> > > > > > On Friday, June 3rd, 2022 at 16:27, Zack Rusin  
> > > > > > wrote:
> > > > > >
> > > > > >
> > > > > > > > In particular: since the driver will ignore the KMS cursor plane
> > > > > > > > position set by user-space, I don't think it's okay to just 
> > > > > > > > expose
> > > > > > > > without opt-in from user-space (e.g. with a DRM_CLIENT_CAP).
> > > > > > > >
> > > > > > > > cc wayland-devel and Pekka for user-space feedback.
> > > > > > >
> > > > > > >
> > > > > > > I think Thomas expressed our concerns and reasons why those 
> > > > > > > wouldn’t
> > > > > > > work for us back then. Is there something else you’d like to add?
> > > > > >
> > > > > >
> > > > > > I disagreed, and I don't understand Thomas' reasoning.
> > > > > >
> > > > > > KMS clients will need an update to work correctly. Adding a new prop
> > > > > > without a cap leaves existing KMS clients broken. Adding a cap 
> > > > > > allows
> > > > > > both existing KMS clients and updated KMS clients to work correctly.
> > > > >
> > > > >
> > > > > I’m not sure what you mean here. They are broken right now. That’s 
> > > > > what we’re
> > > > > fixing. That’s the reason why the virtualized drivers are on 
> > > > > deny-lists for
> > > > > all atomic kms. So nothing needs to be updated. If you have a kms 
> > > > > client that
> > > > > was using virtualized drivers with atomic kms then mouse clicks never 
> > > > > worked
> > > > > correctly.
> > > > >
> > > > > So, yes, clients need to set cursor hotspot if they want mouse to work
> > > > > correctly with virtualized drivers.
> > > >
> > > >
> > > > My proposal was:
> > > >
> > > > - Introduce DRM_CLIENT_CAP_CURSOR_PLANE_NO_POSITION (or a better name). 
> > > > Only
> > > > user-space which supports the hotspot props will enable it.
> > > > - By default, don't expose a cursor plane, because current user-space 
> > > > doesn't
> > > > support it (Weston might put a window in the cursor plane, and nobody 
> > > > can
> > > > report hotspot).
> > > > - If the KMS client enables the cap, advertise the cursor
> > > > plane, and make it so the plane doesn't have the CRTC_X/CRTC_Y 
> > > > properties
> > > > since the driver will pick the position.
> > > >
> > > > That way both old and new user-space are fixed.
> > >
> > >
> > > I don’t think I see how that fixes anything. In particular I don’t see a 
> > > way
> > > of fixing the old user space at all. We require hotspot info, old user 
> > > space
> > > doesn’t set it because there’s no way of setting it on atomic kms.
> >
> >
> > Old atomic user-space is fixed by removing the cursor plane. Then old
> > atomic user-space will fallback to drawing the cursor itself, e.g. via
> > OpenGL.
>
> But it’s not fixed, because the driver is still on a deny-list and
> nothing implements this. You’re saying you could potentially “fix” by
> implementing client side cursor handling in all kms clients? That’s a
> hard sell if the user space can just put the virtualized driver on
> deny-lists and fallback to use old kms which supports hotspots.

What deny-list are you referring to?

All compositors I know of implement a fallback when no cursor plane is
usable.

> > New user-space supports setting the hotspot prop, and is aware that it can't
> > set the cursor plane position, so the cursor plane can be exposed again when
> > the cap is enabled.
>
> But we still use cursor plane position. Hotspots are offsets from
> cursor plane positions. Both are required.

No, VM drivers don't need and disregard the cursor position AFAIK. They
replace it with the host cursor's position.

This is what breaks Weston, breaks uAPI expectations, and IMHO is
unacceptable without KMS client opt-in.

Re: [PATCH 0/6] drm: Add mouse cursor hotspot support to atomic KMS



On Jun 3, 2022, at 11:22 AM, Simon Ser 
mailto:cont...@emersion.fr>> wrote:

⚠ External Email

On Friday, June 3rd, 2022 at 17:17, Zack Rusin 
mailto:za...@vmware.com>> wrote:


On Jun 3, 2022, at 10:56 AM, Simon Ser 
mailto:cont...@emersion.fr>> wrote:
⚠ External Email

On Friday, June 3rd, 2022 at 16:38, Zack Rusin 
mailto:za...@vmware.com>> wrote:

On Jun 3, 2022, at 10:32 AM, Simon Ser 
mailto:cont...@emersion.fr>> wrote:

⚠ External Email

On Friday, June 3rd, 2022 at 16:27, Zack Rusin 
mailto:za...@vmware.com>> wrote:

In particular: since the driver will ignore the KMS cursor plane
position set by user-space, I don't think it's okay to just expose
without opt-in from user-space (e.g. with a DRM_CLIENT_CAP).

cc wayland-devel and Pekka for user-space feedback.

I think Thomas expressed our concerns and reasons why those wouldn’t
work for us back then. Is there something else you’d like to add?

I disagreed, and I don't understand Thomas' reasoning.

KMS clients will need an update to work correctly. Adding a new prop
without a cap leaves existing KMS clients broken. Adding a cap allows
both existing KMS clients and updated KMS clients to work correctly.

I’m not sure what you mean here. They are broken right now. That’s what we’re
fixing. That’s the reason why the virtualized drivers are on deny-lists for
all atomic kms. So nothing needs to be updated. If you have a kms client that
was using virtualized drivers with atomic kms then mouse clicks never worked
correctly.

So, yes, clients need to set cursor hotspot if they want mouse to work
correctly with virtualized drivers.

My proposal was:

- Introduce DRM_CLIENT_CAP_CURSOR_PLANE_NO_POSITION (or a better name). Only
user-space which supports the hotspot props will enable it.
- By default, don't expose a cursor plane, because current user-space doesn't
support it (Weston might put a window in the cursor plane, and nobody can
report hotspot).
- If the KMS client enables the cap, advertise the cursor
plane, and make it so the plane doesn't have the CRTC_X/CRTC_Y properties
since the driver will pick the position.

That way both old and new user-space are fixed.

I don’t think I see how that fixes anything. In particular I don’t see a way
of fixing the old user space at all. We require hotspot info, old user space
doesn’t set it because there’s no way of setting it on atomic kms.

Old atomic user-space is fixed by removing the cursor plane. Then old
atomic user-space will fallback to drawing the cursor itself, e.g. via
OpenGL.

But it’s not fixed, because the driver is still on a deny-list and nothing 
implements this. You’re saying you could potentially “fix” by implementing 
client side cursor handling in all kms clients? That’s a hard sell if the user 
space can just put the virtualized driver on deny-lists and fallback to use old 
kms which supports hotspots.

New user-space supports setting the hotspot prop, and is aware that it can't
set the cursor plane position, so the cursor plane can be exposed again when
the cap is enabled.

But we still use cursor plane position. Hotspots are offsets from cursor plane 
positions. Both are required.

z

Re: [PATCH 0/6] drm: Add mouse cursor hotspot support to atomic KMS

On Friday, June 3rd, 2022 at 17:17, Zack Rusin  wrote:

>
> > On Jun 3, 2022, at 10:56 AM, Simon Ser  wrote:
> > ⚠ External Email
> >
> > On Friday, June 3rd, 2022 at 16:38, Zack Rusin  wrote:
> >
> > > > On Jun 3, 2022, at 10:32 AM, Simon Ser  wrote:
> > > >
> > > > ⚠ External Email
> > > >
> > > > On Friday, June 3rd, 2022 at 16:27, Zack Rusin  wrote:
> > > >
> > > > > > In particular: since the driver will ignore the KMS cursor plane
> > > > > > position set by user-space, I don't think it's okay to just expose
> > > > > > without opt-in from user-space (e.g. with a DRM_CLIENT_CAP).
> > > > > >
> > > > > > cc wayland-devel and Pekka for user-space feedback.
> > > > >
> > > > > I think Thomas expressed our concerns and reasons why those wouldn’t
> > > > > work for us back then. Is there something else you’d like to add?
> > > >
> > > > I disagreed, and I don't understand Thomas' reasoning.
> > > >
> > > > KMS clients will need an update to work correctly. Adding a new prop
> > > > without a cap leaves existing KMS clients broken. Adding a cap allows
> > > > both existing KMS clients and updated KMS clients to work correctly.
> > >
> > > I’m not sure what you mean here. They are broken right now. That’s what 
> > > we’re
> > > fixing. That’s the reason why the virtualized drivers are on deny-lists 
> > > for
> > > all atomic kms. So nothing needs to be updated. If you have a kms client 
> > > that
> > > was using virtualized drivers with atomic kms then mouse clicks never 
> > > worked
> > > correctly.
> > >
> > > So, yes, clients need to set cursor hotspot if they want mouse to work
> > > correctly with virtualized drivers.
> >
> > My proposal was:
> >
> > - Introduce DRM_CLIENT_CAP_CURSOR_PLANE_NO_POSITION (or a better name). Only
> > user-space which supports the hotspot props will enable it.
> > - By default, don't expose a cursor plane, because current user-space 
> > doesn't
> > support it (Weston might put a window in the cursor plane, and nobody can
> > report hotspot).
> > - If the KMS client enables the cap, advertise the cursor
> > plane, and make it so the plane doesn't have the CRTC_X/CRTC_Y properties
> > since the driver will pick the position.
> >
> > That way both old and new user-space are fixed.
>
> I don’t think I see how that fixes anything. In particular I don’t see a way
> of fixing the old user space at all. We require hotspot info, old user space
> doesn’t set it because there’s no way of setting it on atomic kms.

Old atomic user-space is fixed by removing the cursor plane. Then old
atomic user-space will fallback to drawing the cursor itself, e.g. via
OpenGL.

New user-space supports setting the hotspot prop, and is aware that it can't
set the cursor plane position, so the cursor plane can be exposed again when
the cap is enabled.

[PATCH] drm/stm: ltdc: disable all layers before crtc shutdown

All plans must be disabled before the CRTC shutdown helping
the crtc to restart from a clean situation (without unwanted
planes already enable).

Signed-off-by: Yannick Fertre 
---
 drivers/gpu/drm/stm/ltdc.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/stm/ltdc.c b/drivers/gpu/drm/stm/ltdc.c
index 6bd45df8f5a7..eeefc3260c07 100644
--- a/drivers/gpu/drm/stm/ltdc.c
+++ b/drivers/gpu/drm/stm/ltdc.c
@@ -787,11 +787,17 @@ static void ltdc_crtc_atomic_disable(struct drm_crtc 
*crtc,
 {
struct ltdc_device *ldev = crtc_to_ltdc(crtc);
struct drm_device *ddev = crtc->dev;
+   int layer_index = 0;
 
DRM_DEBUG_DRIVER("\n");
 
drm_crtc_vblank_off(crtc);
 
+   /* Disable all layers */
+   for (layer_index = 0; layer_index < ldev->caps.nb_layers; layer_index++)
+   regmap_write_bits(ldev->regmap, LTDC_L1CR + layer_index * 
LAY_OFS,
+ LXCR_CLUTEN | LXCR_LEN, 0);
+
/* disable IRQ */
regmap_clear_bits(ldev->regmap, LTDC_IER, IER_RRIE | IER_FUIE | 
IER_TERRIE);
 
-- 
2.25.1

Re: [PATCH v2 0/2] drm/bridge: analogix_dp: Self-refresh state machine fixes

2022-06-03 Thread Doug Anderson

Hi,

On Fri, Jun 3, 2022 at 8:11 AM Sean Paul  wrote:
>
> On Mon, May 23, 2022 at 5:51 PM Brian Norris  wrote:
> >
> > On Thu, Mar 10, 2022 at 3:50 PM Brian Norris  
> > wrote:
> > > On Mon, Feb 28, 2022 at 12:25 PM Brian Norris  
> > > wrote:
> >
> > > Ping for review? Sean, perhaps? (You already reviewed this on the
> > > Chromium tracker.)
> >
> > Ping
>
> Apologies for the delay. Please in future ping on irc/chat if you're
> waiting for review from me, my inbox is often neglected.
>
> The set still looks good to me,
>
> Reviewed-by: Sean Paul 

Unless someone yells, I'll plan to apply both patches to
drm-misc-fixes early next week, possibly Monday. Seems like if someone
was going to object to these they've had plenty of time up until now.

-Doug

Re: [PATCH 0/6] drm: Add mouse cursor hotspot support to atomic KMS



On Jun 3, 2022, at 10:56 AM, Simon Ser 
mailto:cont...@emersion.fr>> wrote:

⚠ External Email

On Friday, June 3rd, 2022 at 16:38, Zack Rusin 
mailto:za...@vmware.com>> wrote:

On Jun 3, 2022, at 10:32 AM, Simon Ser 
mailto:cont...@emersion.fr>> wrote:

⚠ External Email

On Friday, June 3rd, 2022 at 16:27, Zack Rusin 
mailto:za...@vmware.com>> wrote:

In particular: since the driver will ignore the KMS cursor plane
position set by user-space, I don't think it's okay to just expose
without opt-in from user-space (e.g. with a DRM_CLIENT_CAP).

cc wayland-devel and Pekka for user-space feedback.

I think Thomas expressed our concerns and reasons why those wouldn’t
work for us back then. Is there something else you’d like to add?

I disagreed, and I don't understand Thomas' reasoning.

KMS clients will need an update to work correctly. Adding a new prop
without a cap leaves existing KMS clients broken. Adding a cap allows
both existing KMS clients and updated KMS clients to work correctly.

I’m not sure what you mean here. They are broken right now. That’s what we’re
fixing. That’s the reason why the virtualized drivers are on deny-lists for
all atomic kms. So nothing needs to be updated. If you have a kms client that
was using virtualized drivers with atomic kms then mouse clicks never worked
correctly.

So, yes, clients need to set cursor hotspot if they want mouse to work
correctly with virtualized drivers.

My proposal was:

- Introduce DRM_CLIENT_CAP_CURSOR_PLANE_NO_POSITION (or a better name). Only
user-space which supports the hotspot props will enable it.
- By default, don't expose a cursor plane, because current user-space doesn't
support it (Weston might put a window in the cursor plane, and nobody can
report hotspot).
- If the KMS client enables the cap, advertise the cursor
plane, and make it so the plane doesn't have the CRTC_X/CRTC_Y properties
since the driver will pick the position.

That way both old and new user-space are fixed.

I don’t think I see how that fixes anything. In particular I don’t see a way of 
fixing the old user space at all. We require hotspot info, old user space 
doesn’t set it because there’s no way of setting it on atomic kms. Whether we 
expose cursor plane or not doesn’t change the fact that we still require the 
hotspot info. I don’t know… Unless the small-print in your proposal is “rewrite 
mouse cursor support in all hypervisors” (which I don’t see how you could make 
work without hotspot info) then I don’t think this solves anything, old or new.

Or did you have some magical way of extracting the hotspot data without the kms 
clients providing it? e.g. in your proposal in virtgpu_plane.c how is 
output->cursor.hot_x and output->cursor.hot_y set without a cursor plane and 
with it?

z

[PATCH] drm/stm: ltdc: fix various coding-style warnings

Fix issues reported by checkpatch.pl:
- Braces {} should be used on all arms
- Blank lines

Signed-off-by: Yannick Fertre 
---
 drivers/gpu/drm/stm/ltdc.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/stm/ltdc.c b/drivers/gpu/drm/stm/ltdc.c
index a4098aaff243..6a9f613839b5 100644
--- a/drivers/gpu/drm/stm/ltdc.c
+++ b/drivers/gpu/drm/stm/ltdc.c
@@ -908,9 +908,9 @@ static void ltdc_crtc_mode_set_nofb(struct drm_crtc *crtc)
drm_connector_list_iter_end();
}
 
-   if (bridge && bridge->timings)
+   if (bridge && bridge->timings) {
bus_flags = bridge->timings->input_bus_flags;
-   else if (connector) {
+   } else if (connector) {
bus_flags = connector->display_info.bus_flags;
if (connector->display_info.num_bus_formats)
bus_formats = connector->display_info.bus_formats[0];
@@ -1917,7 +1917,6 @@ int ltdc_load(struct drm_device *ddev)
DRM_ERROR("Failed to register LTDC interrupt\n");
goto err;
}
-
}
 
crtc = devm_kzalloc(dev, sizeof(*crtc), GFP_KERNEL);
-- 
2.25.1

Re: [PATCH v2 0/2] drm/bridge: analogix_dp: Self-refresh state machine fixes

2022-06-03 Thread Sean Paul

On Mon, May 23, 2022 at 5:51 PM Brian Norris  wrote:
>
> On Thu, Mar 10, 2022 at 3:50 PM Brian Norris  wrote:
> > On Mon, Feb 28, 2022 at 12:25 PM Brian Norris  
> > wrote:
>
> > Ping for review? Sean, perhaps? (You already reviewed this on the
> > Chromium tracker.)
>
> Ping

Apologies for the delay. Please in future ping on irc/chat if you're
waiting for review from me, my inbox is often neglected.

The set still looks good to me,

Reviewed-by: Sean Paul 

Sean

[PATCH] drm/stm: ltdc: add support of horizontal & vertical mirroring

Support of vertical & horizontal mirroring features thanks to
the plane rotation property.

Signed-off-by: Yannick Fertre 
---
 drivers/gpu/drm/stm/ltdc.c | 163 -
 drivers/gpu/drm/stm/ltdc.h |   1 +
 2 files changed, 108 insertions(+), 56 deletions(-)

diff --git a/drivers/gpu/drm/stm/ltdc.c b/drivers/gpu/drm/stm/ltdc.c
index 00a6bc1b1d7c..ff2075dd9474 100644
--- a/drivers/gpu/drm/stm/ltdc.c
+++ b/drivers/gpu/drm/stm/ltdc.c
@@ -180,6 +180,7 @@
 #define LXCR_LEN   BIT(0)  /* Layer ENable */
 #define LXCR_COLKENBIT(1)  /* Color Keying Enable */
 #define LXCR_CLUTENBIT(4)  /* Color Look-Up Table ENable */
+#define LXCR_HMEN  BIT(8)  /* Horizontal Mirroring ENable */
 
 #define LXWHPCR_WHSTPOSGENMASK(11, 0)  /* Window Horizontal StarT 
POSition */
 #define LXWHPCR_WHSPPOSGENMASK(27, 16) /* Window Horizontal StoP 
POSition */
@@ -197,7 +198,7 @@
 #define LXBFCR_BOR GENMASK(18, 16) /* Blending ORder */
 
 #define LXCFBLR_CFBLL  GENMASK(12, 0)  /* Color Frame Buffer Line Length */
-#define LXCFBLR_CFBP   GENMASK(28, 16) /* Color Frame Buffer Pitch in bytes */
+#define LXCFBLR_CFBP   GENMASK(31, 16) /* Color Frame Buffer Pitch in bytes */
 
 #define LXCFBLNR_CFBLN GENMASK(10, 0)  /* Color Frame Buffer Line Number */
 
@@ -1237,7 +1238,8 @@ static void ltdc_plane_atomic_update(struct drm_plane 
*plane,
u32 y0 = newstate->crtc_y;
u32 y1 = newstate->crtc_y + newstate->crtc_h - 1;
u32 src_x, src_y, src_w, src_h;
-   u32 val, pitch_in_bytes, line_length, line_number, paddr, ahbp, avbp, 
bpcr;
+   u32 val, pitch_in_bytes, line_length, line_number, ahbp, avbp, bpcr;
+   u32 paddr, paddr1, paddr2;
enum ltdc_pix_fmt pf;
 
if (!newstate->crtc || !fb) {
@@ -1289,13 +1291,6 @@ static void ltdc_plane_atomic_update(struct drm_plane 
*plane,
}
regmap_write_bits(ldev->regmap, LTDC_L1PFCR + lofs, LXPFCR_PF, val);
 
-   /* Configures the color frame buffer pitch in bytes & line length */
-   pitch_in_bytes = fb->pitches[0];
-   line_length = fb->format->cpp[0] *
- (x1 - x0 + 1) + (ldev->caps.bus_width >> 3) - 1;
-   val = ((pitch_in_bytes << 16) | line_length);
-   regmap_write_bits(ldev->regmap, LTDC_L1CFBLR + lofs, LXCFBLR_CFBLL | 
LXCFBLR_CFBP, val);
-
/* Specifies the constant alpha value */
val = newstate->alpha >> 8;
regmap_write_bits(ldev->regmap, LTDC_L1CACR + lofs, LXCACR_CONSTA, val);
@@ -1319,76 +1314,115 @@ static void ltdc_plane_atomic_update(struct drm_plane 
*plane,
  LXBFCR_BF2 | LXBFCR_BF1, val);
}
 
-   /* Configures the frame buffer line number */
-   line_number = y1 - y0 + 1;
-   regmap_write_bits(ldev->regmap, LTDC_L1CFBLNR + lofs, LXCFBLNR_CFBLN, 
line_number);
-
/* Sets the FB address */
paddr = (u32)drm_fb_cma_get_gem_addr(fb, newstate, 0);
 
+   if (newstate->rotation & DRM_MODE_REFLECT_X)
+   paddr += (fb->format->cpp[0] * (x1 - x0 + 1)) - 1;
+
+   if (newstate->rotation & DRM_MODE_REFLECT_Y)
+   paddr += (fb->pitches[0] * (y1 - y0));
+
DRM_DEBUG_DRIVER("fb: phys 0x%08x", paddr);
regmap_write(ldev->regmap, LTDC_L1CFBAR + lofs, paddr);
 
+   /* Configures the color frame buffer pitch in bytes & line length */
+   line_length = fb->format->cpp[0] *
+ (x1 - x0 + 1) + (ldev->caps.bus_width >> 3) - 1;
+
+   if (newstate->rotation & DRM_MODE_REFLECT_Y)
+   /* Compute negative value (signed on 16 bits) for the picth */
+   pitch_in_bytes = 0x1 - fb->pitches[0];
+   else
+   pitch_in_bytes = fb->pitches[0];
+
+   val = (pitch_in_bytes << 16) | line_length;
+   regmap_write_bits(ldev->regmap, LTDC_L1CFBLR + lofs, LXCFBLR_CFBLL | 
LXCFBLR_CFBP, val);
+
+   /* Configures the frame buffer line number */
+   line_number = y1 - y0 + 1;
+   regmap_write_bits(ldev->regmap, LTDC_L1CFBLNR + lofs, LXCFBLNR_CFBLN, 
line_number);
+
if (ldev->caps.ycbcr_input) {
if (fb->format->is_yuv) {
switch (fb->format->format) {
case DRM_FORMAT_NV12:
case DRM_FORMAT_NV21:
-   /* Configure the auxiliary frame buffer address 0 & 1 */
-   paddr = (u32)drm_fb_cma_get_gem_addr(fb, newstate, 1);
-   regmap_write(ldev->regmap, LTDC_L1AFBA0R + lofs, paddr);
-   regmap_write(ldev->regmap, LTDC_L1AFBA1R + lofs, paddr 
+ 1);
+   /* Configure the auxiliary frame buffer address 0 */
+   paddr1 = (u32)drm_fb_cma_get_gem_addr(fb, newstate, 1);
+
+   if (newstate->rotation & DRM_MODE_REFLECT_X)
+   paddr1 += ((fb->format->cpp[1] * (x1 - x0 + 1)) 
>> 1) -

Re: linux-next: Fixes tag needs some work in the drm tree

2022-06-03 Thread Rob Clark

On Thu, Jun 2, 2022 at 10:16 PM Stephen Rothwell  wrote:
>
> Hi all,
>
> In commit
>
>   8caad14e7224 ("drm/msm/dpu: Fix pointer dereferenced before checking")
>
> Fixes tag
>
>   Fixes: d7d0e73f7de33 ("drm/msm/dpu: introduce the dpu_encoder_phys_* for
>
> has these problem(s):
>
>   - Subject has leading but no trailing parentheses
>   - Subject has leading but no trailing quotes
>
> Fixes tags should not be truncated or split across more than 1 line.   So:
>
> Fixes: d7d0e73f7de3 ("drm/msm/dpu: introduce the dpu_encoder_phys_* for 
> writeback")

Hmm, checkpatch seems to not catch this (unless it was a recent
addition that landed after what msm-fixes is based on.

will the truncated subject confuse the scripts that look for patches
to backport to stable, ie. do we *really* have to rewrite history to
fix this?

BR,
-R

Re: [PATCH 0/6] drm: Add mouse cursor hotspot support to atomic KMS

On Friday, June 3rd, 2022 at 16:38, Zack Rusin  wrote:

> > On Jun 3, 2022, at 10:32 AM, Simon Ser  wrote:
> >
> > ⚠ External Email
> >
> > On Friday, June 3rd, 2022 at 16:27, Zack Rusin  wrote:
> >
> >>> In particular: since the driver will ignore the KMS cursor plane
> >>> position set by user-space, I don't think it's okay to just expose
> >>> without opt-in from user-space (e.g. with a DRM_CLIENT_CAP).
> >>>
> >>> cc wayland-devel and Pekka for user-space feedback.
> >>
> >> I think Thomas expressed our concerns and reasons why those wouldn’t
> >> work for us back then. Is there something else you’d like to add?
> >
> > I disagreed, and I don't understand Thomas' reasoning.
> >
> > KMS clients will need an update to work correctly. Adding a new prop
> > without a cap leaves existing KMS clients broken. Adding a cap allows
> > both existing KMS clients and updated KMS clients to work correctly.
>
> I’m not sure what you mean here. They are broken right now. That’s what we’re
> fixing. That’s the reason why the virtualized drivers are on deny-lists for
> all atomic kms. So nothing needs to be updated. If you have a kms client that
> was using virtualized drivers with atomic kms then mouse clicks never worked
> correctly.
>
> So, yes, clients need to set cursor hotspot if they want mouse to work
> correctly with virtualized drivers.

My proposal was:

- Introduce DRM_CLIENT_CAP_CURSOR_PLANE_NO_POSITION (or a better name). Only
  user-space which supports the hotspot props will enable it.
- By default, don't expose a cursor plane, because current user-space doesn't
  support it (Weston might put a window in the cursor plane, and nobody can
  report hotspot).
- If the KMS client enables the cap, advertise the cursor
  plane, and make it so the plane doesn't have the CRTC_X/CRTC_Y properties
  since the driver will pick the position.

That way both old and new user-space are fixed.

> >>> On Thursday, June 2nd, 2022 at 17:42, Zack Rusin z...@kde.org wrote:
> >>>
>  - all userspace code needs to hardcore a list of drivers which require
>  hotspots because there's no way to query from drm "does this driver
>  require hotspot"
> >>>
> >>> Can you elaborate? I'm not sure I understand what you mean here.
> >>
> >> Only some drivers require informations about cursor hotspot, user space
> >> currently has no way of figuring out which ones are those, i.e. amdgpu
> >> doesn’t care about hotspots, qxl does so when running on qxl without
> >> properly set cursor hotspot atomic kms will result in a desktop where
> >> mouse clicks have incorrect coordinates.
> >>
> >> There’s no way to differentiate between drivers that do not care about
> >> cursor hotspots and ones that absolutely require it.
> >
> > Only VM drivers should expose the hotspot properties. Real drivers like
> > amdgpu must not expose it.
>
> Yes, that’s what I said. There’s no way to differentiate between amdgpu that
> doesn’t have those properties and virtio_gpu driver from a kernel before
> hotspot properties were added.

See cap proposal above.

Re: [PATCH v3 3/4] drm/bridge: Add devm_drm_bridge_add()

2022-06-03 Thread Doug Anderson

Hi,

On Fri, Jun 3, 2022 at 7:14 AM Maxime Ripard  wrote:
>
> On Fri, Jun 03, 2022 at 01:19:16PM +0300, Dmitry Baryshkov wrote:
> > On Fri, 3 Jun 2022 at 11:21, Maxime Ripard  wrote:
> > >
> > > On Tue, May 31, 2022 at 02:06:34PM -0700, Doug Anderson wrote:
> > > > On Mon, May 23, 2022 at 10:00 AM Doug Anderson  
> > > > wrote:
> > > > > On Sat, May 21, 2022 at 2:17 AM Maxime Ripard  
> > > > > wrote:
> > > > > > On Tue, May 10, 2022 at 12:29:43PM -0700, Douglas Anderson wrote:
> > > > > > > This adds a devm managed version of drm_bridge_add(). Like other
> > > > > > > "devm" function listed in drm_bridge.h, this function takes an
> > > > > > > explicit "dev" to use for the lifetime management. A few notes:
> > > > > > > * In general we have a "struct device" for bridges that makes a 
> > > > > > > good
> > > > > > >   candidate for where the lifetime matches exactly what we want.
> > > > > > > * The "bridge->dev->dev" device appears to be the encoder
> > > > > > >   device. That's not the right device to use for lifetime 
> > > > > > > management.
> > > > > > >
> > > > > > > Suggested-by: Dmitry Baryshkov 
> > > > > > > Signed-off-by: Douglas Anderson 
> > > > > >
> > > > > > If we are to introduce more managed helpers, I think it'd be wiser 
> > > > > > to
> > > > > > introduce them as DRM-managed, and not device managed.
> > > > > >
> > > > > > Otherwise, you'll end up in a weird state when a device has been 
> > > > > > removed
> > > > > > but the DRM device is still around.
> > > > >
> > > > > I'm kinda confused. In this case there is no DRM device for the bridge
> > > > > and, as per my CL description, "bridge-dev->dev" appears to be the
> > > > > encoder device. I wasn't personally involved in discussions about it,
> > > > > but I was under the impression that this was expected / normal. Thus
> > > > > we can't make this DRM-managed.
> > > >
> > > > Since I didn't hear a reply,
> > >
> > > Gah, I replied but it looks like somehow it never reached the ML...
> > >
> > > Here was my original reply:
> > >
> > > > > > This adds a devm managed version of drm_bridge_add(). Like other
> > > > > > "devm" function listed in drm_bridge.h, this function takes an
> > > > > > explicit "dev" to use for the lifetime management. A few notes:
> > > > > > * In general we have a "struct device" for bridges that makes a good
> > > > > >   candidate for where the lifetime matches exactly what we want.
> > > > > > * The "bridge->dev->dev" device appears to be the encoder
> > > > > >   device. That's not the right device to use for lifetime 
> > > > > > management.
> > > > > >
> > > > > > Suggested-by: Dmitry Baryshkov 
> > > > > > Signed-off-by: Douglas Anderson 
> > > > >
> > > > > If we are to introduce more managed helpers, I think it'd be wiser to
> > > > > introduce them as DRM-managed, and not device managed.
> > > > >
> > > > > Otherwise, you'll end up in a weird state when a device has been 
> > > > > removed
> > > > > but the DRM device is still around.
> > > >=20
> > > > I'm kinda confused. In this case there is no DRM device for the bridge
> > > > and, as per my CL description, "bridge-dev->dev" appears to be the
> > > > encoder device.
> > >
> > > bridge->dev seems right though?
> > >
> > > > I wasn't personally involved in discussions about it, but I was under
> > > > the impression that this was expected / normal. Thus we can't make
> > > > this DRM-managed.
> > >
> > > Still, I don't think devm is the right solution to this either.
> > >
> > > The underlying issue is two-fold:
> > >
> > >   - Encoders can have a pointer to a bridge through of_drm_find_bridge
> > > or similar. However, bridges are traditionally tied to their device
> > > lifetime (by calling drm_bridge_add in probe, and drm_bridge_remove
> > > in remove). Encoders will typically be tied to the DRM device
> > > however, and that one sticks around until the last application
> > > closes it. We can thus very easily end up with a dangling pointer,
> > > and a use-after-free.
> > >
> > >   - It's not the case yet, but it doesn't seem far fetch to expose
> > > properties of bridges to the userspace. In that case, the userspace
> > > would be likely to still hold references to objects that aren't
> > > there anymore when the bridge is gone.
> > >
> > > The first is obviously a larger concern, but if we can find a solution
> > > that would accomodate the second it would be great.
> > >
> > > As far as I can see, we should fix in two steps:
> > >
> > >   - in drm_bridge_attach, we should add a device-managed call that will
> > > unregister the main DRM device. We don't allow to probe the main DRM
> > > device when the bridge isn't there yet in most case, so it makes
> > > sense to remove it once the bridge is no longer there as well.
> >
> > The problem is that I do not see a good way to unregister the main DRM
> > device outside of it's driver code.
>
> That's what drmm helpers are doing though:

Re: [PATCH] BACKPORT: drm/amdgpu/disply: set num_crtc earlier

2022-06-03 Thread Greg KH

A: http://en.wikipedia.org/wiki/Top_post
Q: Were do I find info about this thing called top-posting?
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?

A: No.
Q: Should I include quotations after my reply?

http://daringfireball.net/2007/07/on_top

On Wed, Jun 01, 2022 at 04:04:45AM +, Lin, Tsung-hua (Ryan) wrote:
> [AMD Official Use Only - General]
> 
> Hi Greg,
> 
> Thanks for your advice. I have modified the commit and submitted it to Gerrit 
> and it's under code review now.

This makes no sense to me, we do not use gerrit for kernel development.

confused,

greg k-h

Re: [PATCH 0/6] drm: Add mouse cursor hotspot support to atomic KMS



> On Jun 3, 2022, at 6:28 AM, Gerd Hoffmann  wrote:
> 
> ⚠ External Email
> 
>  Hi,
> 
>> the legacy kms to atomic. This left virtualized drivers, all which
>> are atomic, in a weird spot because all userspace compositors put
>> those drivers on deny-lists for atomic kms due to the fact that mouse
>> clicks were incorrectly routed, e.g:
> 
>> - all userspace code needs to hardcore a list of drivers which require
>> hotspots because there's no way to query from drm "does this driver
>> require hotspot"
> 
> So drivers move from one list to another.  Not ideal, but also not worse
> than it was before, so:

Yea, that problem was always there because we never had a way of checking 
whether a particular driver requires hotspot info. Unfortunately I don’t think 
this can not be solved in the kernel anymore. Technically the hotspot 
properties solve it: does the driver expose hotspot properties on cursor 
planes? If yes, then it requires hotspot info. But we can’t differentiate 
between:
- driver that doesn’t require hotspot info
- driver that requires hotspot data but we’re running on a kernel before those 
properties were implemented
We’d have to backport those properties to every kernel between now and when 
atomic kms was implemented. Same with any other check we could add to the 
kernel. Likely libdrm would be the place for this atm but I’m not sure in what 
form.

z

Re: [PATCH 0/6] drm: Add mouse cursor hotspot support to atomic KMS



> On Jun 3, 2022, at 10:32 AM, Simon Ser  wrote:
> 
> ⚠ External Email
> 
> On Friday, June 3rd, 2022 at 16:27, Zack Rusin  wrote:
> 
>>> In particular: since the driver will ignore the KMS cursor plane
>>> position set by user-space, I don't think it's okay to just expose
>>> without opt-in from user-space (e.g. with a DRM_CLIENT_CAP).
>>> 
>>> cc wayland-devel and Pekka for user-space feedback.
>> 
>> I think Thomas expressed our concerns and reasons why those wouldn’t
>> work for us back then. Is there something else you’d like to add?
> 
> I disagreed, and I don't understand Thomas' reasoning.
> 
> KMS clients will need an update to work correctly. Adding a new prop
> without a cap leaves existing KMS clients broken. Adding a cap allows
> both existing KMS clients and updated KMS clients to work correctly.

I’m not sure what you mean here. They are broken right now. That’s what we’re 
fixing. That’s the reason why the virtualized drivers are on deny-lists for all 
atomic kms. So nothing needs to be updated. If you have a kms client that was 
using virtualized drivers with atomic kms then mouse clicks never worked 
correctly.
So, yes, clients need to set cursor hotspot if they want mouse to work 
correctly with virtualized drivers.


>>> On Thursday, June 2nd, 2022 at 17:42, Zack Rusin z...@kde.org wrote:
>>> 
 - all userspace code needs to hardcore a list of drivers which require
 hotspots because there's no way to query from drm "does this driver
 require hotspot"
>>> 
>>> Can you elaborate? I'm not sure I understand what you mean here.
>> 
>> Only some drivers require informations about cursor hotspot, user space
>> currently has no way of figuring out which ones are those, i.e. amdgpu
>> doesn’t care about hotspots, qxl does so when running on qxl without
>> properly set cursor hotspot atomic kms will result in a desktop where
>> mouse clicks have incorrect coordinates.
>> 
>> There’s no way to differentiate between drivers that do not care about
>> cursor hotspots and ones that absolutely require it.
> 
> Only VM drivers should expose the hotspot properties. Real drivers like
> amdgpu must not expose it.

Yes, that’s what I said. There’s no way to differentiate between amdgpu that 
doesn’t have those properties and virtio_gpu driver from a kernel before 
hotspot properties were added.

z

Re: [PATCH v3 2/2] drm: bridge: Add TI DLPC3433 DSI to DMD bridge

2022-06-03 Thread Robert Foss

On Fri, 3 Jun 2022 at 16:04, Jagan Teki  wrote:
>
> TI DLPC3433 is a MIPI DSI based display controller bridge
> for processing high resolution DMD based projectors.
>
> It has a flexible configuration of MIPI DSI and DPI signal
> input that produces a DMD output in RGB565, RGB666, RGB888
> formats.
>
> It supports upto 720p resolution with 60 and 120 Hz refresh
> rates.
>
> Add bridge driver for it.
>
> Signed-off-by: Christopher Vollo 
> Signed-off-by: Jagan Teki 
> ---
> Changes for v3:
> - none
> Changes for v2:
> - fixed license
> - filled display size buffer
> - fixed power off
> - fixed dev_err_probe
>
>  MAINTAINERS  |   1 +
>  drivers/gpu/drm/bridge/Kconfig   |  16 +
>  drivers/gpu/drm/bridge/Makefile  |   1 +
>  drivers/gpu/drm/bridge/ti-dlpc3433.c | 417 +++
>  4 files changed, 435 insertions(+)
>  create mode 100644 drivers/gpu/drm/bridge/ti-dlpc3433.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index aea0fe5156af..ede21cc48708 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -6435,6 +6435,7 @@ DRM DRIVER FOR TI DLPC3433 MIPI DSI TO DMD BRIDGE
>  M: Jagan Teki 
>  S: Maintained
>  F: Documentation/devicetree/bindings/display/bridge/ti,dlpc3433.yaml
> +F: drivers/gpu/drm/bridge/ti-dlpc3433.c
>
>  DRM DRIVER FOR TI SN65DSI86 BRIDGE CHIP
>  R: Douglas Anderson 
> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> index 146ab069838f..4b28f939fff6 100644
> --- a/drivers/gpu/drm/bridge/Kconfig
> +++ b/drivers/gpu/drm/bridge/Kconfig
> @@ -321,6 +321,22 @@ config DRM_TOSHIBA_TC358775
> help
>   Toshiba TC358775 DSI/LVDS bridge chip driver.
>
> +config DRM_TI_DLPC3433
> +   tristate "TI DLPC3433 Display controller"
> +   depends on DRM && DRM_PANEL
> +   depends on OF
> +   select DRM_MIPI_DSI
> +   help
> + TI DLPC3433 is a MIPI DSI based display controller bridge
> + for processing high resolution DMD based projectors.
> +
> + It has a flexible configuration of MIPI DSI and DPI signal
> + input that produces a DMD output in RGB565, RGB666, RGB888
> + formats.
> +
> + It supports upto 720p resolution with 60 and 120 Hz refresh
> + rates.
> +
>  config DRM_TI_TFP410
> tristate "TI TFP410 DVI/HDMI bridge"
> depends on OF
> diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
> index f6c0a95de549..043b499545e3 100644
> --- a/drivers/gpu/drm/bridge/Makefile
> +++ b/drivers/gpu/drm/bridge/Makefile
> @@ -26,6 +26,7 @@ obj-$(CONFIG_DRM_TOSHIBA_TC358767) += tc358767.o
>  obj-$(CONFIG_DRM_TOSHIBA_TC358768) += tc358768.o
>  obj-$(CONFIG_DRM_TOSHIBA_TC358775) += tc358775.o
>  obj-$(CONFIG_DRM_I2C_ADV7511) += adv7511/
> +obj-$(CONFIG_DRM_TI_DLPC3433) += ti-dlpc3433.o
>  obj-$(CONFIG_DRM_TI_SN65DSI83) += ti-sn65dsi83.o
>  obj-$(CONFIG_DRM_TI_SN65DSI86) += ti-sn65dsi86.o
>  obj-$(CONFIG_DRM_TI_TFP410) += ti-tfp410.o
> diff --git a/drivers/gpu/drm/bridge/ti-dlpc3433.c 
> b/drivers/gpu/drm/bridge/ti-dlpc3433.c
> new file mode 100644
> index ..06e519798ac5
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/ti-dlpc3433.c
> @@ -0,0 +1,417 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2021 RenewOutReach
> + * Copyright (C) 2021 Amarula Solutions(India)
> + *
> + * Author:
> + * Jagan Teki 
> + * Christopher Vollo 
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +enum cmd_registers {
> +   WR_INPUT_SOURCE = 0x05, /* Write Input Source Select */
> +   WR_EXT_SOURCE_FMT   = 0x07, /* Write External Video Source Format 
> */
> +   WR_IMAGE_CROP   = 0x10, /* Write Image Crop */
> +   WR_DISPLAY_SIZE = 0x12, /* Write Display Size */
> +   WR_IMAGE_FREEZE = 0x1A, /* Write Image Freeze */
> +   WR_INPUT_IMAGE_SIZE = 0x2E, /* Write External Input Image Size */
> +   WR_RGB_LED_EN   = 0x52, /* Write RGB LED Enable */
> +   WR_RGB_LED_CURRENT  = 0x54, /* Write RGB LED Current */
> +   WR_RGB_LED_MAX_CURRENT  = 0x5C, /* Write RGB LED Max Current */
> +   WR_DSI_HS_CLK   = 0xBD, /* Write DSI HS Clock */
> +   RD_DEVICE_ID= 0xD4, /* Read Controller Device ID */
> +   WR_DSI_PORT_EN  = 0xD7, /* Write DSI Port Enable */
> +};
> +
> +enum input_source {
> +   INPUT_EXTERNAL_VIDEO= 0,
> +   INPUT_TEST_PATTERN,
> +   INPUT_SPLASH_SCREEN,
> +};
> +
> +#define DEV_ID_MASKGENMASK(3, 0)
> +#define IMAGE_FREESE_ENBIT(0)
> +#define DSI_PORT_EN0
> +#define EXT_SOURCE_FMT_DSI 0
> +#define RED_LED_EN BIT(0)
> +#define GREEN_LED_EN   BIT(1)
> +#define BLUE_LED_ENBIT(2)
> +#define LED_MASK   GENMASK(2, 0)
> +#define MAX_BYTE_SIZE  8
> +
> +struct

Re: [PATCH 0/6] drm: Add mouse cursor hotspot support to atomic KMS

On Friday, June 3rd, 2022 at 16:27, Zack Rusin  wrote:

> > In particular: since the driver will ignore the KMS cursor plane
> > position set by user-space, I don't think it's okay to just expose
> > without opt-in from user-space (e.g. with a DRM_CLIENT_CAP).
> >
> > cc wayland-devel and Pekka for user-space feedback.
>
> I think Thomas expressed our concerns and reasons why those wouldn’t
> work for us back then. Is there something else you’d like to add?

I disagreed, and I don't understand Thomas' reasoning.

KMS clients will need an update to work correctly. Adding a new prop
without a cap leaves existing KMS clients broken. Adding a cap allows
both existing KMS clients and updated KMS clients to work correctly.

> > On Thursday, June 2nd, 2022 at 17:42, Zack Rusin z...@kde.org wrote:
> >
> > > - all userspace code needs to hardcore a list of drivers which require
> > > hotspots because there's no way to query from drm "does this driver
> > > require hotspot"
> >
> > Can you elaborate? I'm not sure I understand what you mean here.
>
> Only some drivers require informations about cursor hotspot, user space
> currently has no way of figuring out which ones are those, i.e. amdgpu
> doesn’t care about hotspots, qxl does so when running on qxl without
> properly set cursor hotspot atomic kms will result in a desktop where
> mouse clicks have incorrect coordinates.
>
> There’s no way to differentiate between drivers that do not care about
> cursor hotspots and ones that absolutely require it.

Only VM drivers should expose the hotspot properties. Real drivers like
amdgpu must not expose it.

Re: [PATCH 0/6] drm: Add mouse cursor hotspot support to atomic KMS

> On Jun 3, 2022, at 10:14 AM, Simon Ser  wrote:
> 
> Hi,
> 
> Please, read this thread:
> https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Farchives%2Fdri-devel%2F2020-March%2Fthread.html%23259615data=05%7C01%7Czackr%40vmware.com%7C05141cb812004e947f0408da456b76e2%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637898625140237028%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7Csdata=%2BZUG0OFC5SXC8%2Bm3D93AliT0VNJHbc1AEwnhVAnw8WQ%3Dreserved=0
> 
> It has a lot of information about the pitfalls of cursor hotspot and
> other things done by VM software.
> 
> In particular: since the driver will ignore the KMS cursor plane
> position set by user-space, I don't think it's okay to just expose
> without opt-in from user-space (e.g. with a DRM_CLIENT_CAP).
> 
> cc wayland-devel and Pekka for user-space feedback.

I think Thomas expressed our concerns and reasons why those wouldn’t work for 
us back then. Is there something else you’d like to add?

> On Thursday, June 2nd, 2022 at 17:42, Zack Rusin  wrote:
> 
>> - all userspace code needs to hardcore a list of drivers which require
>> hotspots because there's no way to query from drm "does this driver
>> require hotspot"
> 
> Can you elaborate? I'm not sure I understand what you mean here.

Only some drivers require informations about cursor hotspot, user space 
currently has no way of figuring out which ones are those, i.e. amdgpu doesn’t 
care about hotspots, qxl does so when running on qxl without properly set 
cursor hotspot atomic kms will result in a desktop where mouse clicks have 
incorrect coordinates. 
There’s no way to differentiate between drivers that do not care about cursor 
hotspots and ones that absolutely require it.

z

Re: [PATCH] drm/i915/pvc: GuC depriv applies to PVC

2022-06-03 Thread Souza, Jose

On Thu, 2022-06-02 at 16:30 -0700, Matt Roper wrote:
> We missed this setting in the initial device info patch's definition of
> XE_HPC_FEATURES.

Reviewed-by: José Roberto de Souza 

> 
> Signed-off-by: Matt Roper 
> ---
>  drivers/gpu/drm/i915/i915_pci.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c
> index 047a6e326031..a5a1a7647320 100644
> --- a/drivers/gpu/drm/i915/i915_pci.c
> +++ b/drivers/gpu/drm/i915/i915_pci.c
> @@ -1089,6 +1089,7 @@ static const struct intel_device_info ats_m_info = {
>   XE_HP_FEATURES, \
>   .dma_mask_size = 52, \
>   .has_3d_pipeline = 0, \
> + .has_guc_deprivilege = 1, \
>   .has_l3_ccs_read = 1, \
>   .has_one_eu_per_fuse_bit = 1
>

Re: [PATCH] drm/sun4i: sun8i: Add the ability to keep scaler enabled for VI layer

On Fri, Jun 03, 2022 at 11:57:35AM +0300, Roman Stratiienko wrote:
> Hi Maxime,
> 
> пт, 3 июн. 2022 г. в 11:24, Maxime Ripard :
> >
> > Hi,
> >
> > On Thu, Jun 02, 2022 at 06:01:18PM +, Roman Stratiienko wrote:
> > > According to DE2.0/DE3.0 manual VI scaler enable register is double
> > > buffered, but de facto it doesn't, or the hardware has the shadow
> > > register latching issues which causes single-frame picture corruption
> > > after changing the state of scaler enable register.
> > >
> > > Allow the user to keep the scaler always enabled, preventing the UI
> > > glitches on the transition from scaled to unscaled state.
> > >
> > > NOTE:
> > > UI layer scaler has more registers with double-buffering issue and can't
> > > be workarounded in the same manner.
> > >
> > > You may find a python test and a demo video for this issue at [1]
> > >
> > > [1]: https://github.com/GloDroid/glodroid_tests/issues/4
> >
> > Please describe the issue entirely here. The commit log must be 
> > self-sufficient.
> 
> Commit message already states "single-frame picture corruption after
> changing the state of scaler enable register", therefore I find it
> already self-sufficient
> 
> Also I find demo videos and link to tests useful for the followers to
> go further with investigation.

Until a couple of years, where that URL isn't valid anymore and it's just 
useless.

> If you have something specific in mind when asking to enhance the
> commit message please say it.

What sequence of events trigger the issue, what issue it triggers and
why your solution addresses it would be nice

> > > Signed-off-by: Roman Stratiienko 
> > > ---
> > >  drivers/gpu/drm/sun4i/sun8i_mixer.c| 12 
> > >  drivers/gpu/drm/sun4i/sun8i_vi_layer.c |  4 +++-
> > >  2 files changed, 15 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c 
> > > b/drivers/gpu/drm/sun4i/sun8i_mixer.c
> > > index 71ab0a00b4de..15cad0330f66 100644
> > > --- a/drivers/gpu/drm/sun4i/sun8i_mixer.c
> > > +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c
> > > @@ -27,6 +27,18 @@
> > >  #include "sun8i_vi_layer.h"
> > >  #include "sunxi_engine.h"
> > >
> > > +/* According to DE2.0/DE3.0 manual VI scaler enable register is double
> > > + * buffered, but de facto it doesn't, or the hardware has the shadow
> > > + * register latching issues which causes single-frame picture corruption
> > > + * after changing the state of scaler enable register.
> > > + * Allow the user to keep the scaler always enabled, preventing the UI
> > > + * glitches on the transition from scaled to unscaled state.
> > > + */
> > > +int sun8i_vi_keep_scaler_enabled;
> > > +MODULE_PARM_DESC(keep_vi_scaler_enabled,
> > > +  "Keep VI scaler enabled (1 = enabled, 0 = disabled 
> > > (default))");
> > > +module_param_named(keep_vi_scaler_enabled, sun8i_vi_keep_scaler_enabled, 
> > > int, 0644);
> > > +
> >
> > It's not clear to me why we would want to make that a parameter?
> >
> >1 If it never works, we should fix it once and for all and not allow a 
> >broken setup at all.
> 
> It's a hardware issue and can be fixed only within the hardware.
> 
> Current patch is a workaround that if enabled can cause increased
> power consumption for existing users. Therefore I think it is better
> to give existing distro-maintainers a chance to test it prior to
> delivery.

Except distro-maintainers are likely never going to notice that flag
exists in the first place, won't be using it and thus the issue will
remain.

Maxime

Re: [PATCH v3 3/4] drm/bridge: Add devm_drm_bridge_add()

On Fri, Jun 03, 2022 at 06:52:05AM -0700, Doug Anderson wrote:
> On Fri, Jun 3, 2022 at 3:19 AM Dmitry Baryshkov
>  wrote:
> > On Fri, 3 Jun 2022 at 11:21, Maxime Ripard  wrote:
> > >
> > > On Tue, May 31, 2022 at 02:06:34PM -0700, Doug Anderson wrote:
> > > > On Mon, May 23, 2022 at 10:00 AM Doug Anderson  
> > > > wrote:
> > > > > On Sat, May 21, 2022 at 2:17 AM Maxime Ripard  
> > > > > wrote:
> > > > > > On Tue, May 10, 2022 at 12:29:43PM -0700, Douglas Anderson wrote:
> > > > > > > This adds a devm managed version of drm_bridge_add(). Like other
> > > > > > > "devm" function listed in drm_bridge.h, this function takes an
> > > > > > > explicit "dev" to use for the lifetime management. A few notes:
> > > > > > > * In general we have a "struct device" for bridges that makes a 
> > > > > > > good
> > > > > > >   candidate for where the lifetime matches exactly what we want.
> > > > > > > * The "bridge->dev->dev" device appears to be the encoder
> > > > > > >   device. That's not the right device to use for lifetime 
> > > > > > > management.
> > > > > > >
> > > > > > > Suggested-by: Dmitry Baryshkov 
> > > > > > > Signed-off-by: Douglas Anderson 
> > > > > >
> > > > > > If we are to introduce more managed helpers, I think it'd be wiser 
> > > > > > to
> > > > > > introduce them as DRM-managed, and not device managed.
> > > > > >
> > > > > > Otherwise, you'll end up in a weird state when a device has been 
> > > > > > removed
> > > > > > but the DRM device is still around.
> > > > >
> > > > > I'm kinda confused. In this case there is no DRM device for the bridge
> > > > > and, as per my CL description, "bridge-dev->dev" appears to be the
> > > > > encoder device. I wasn't personally involved in discussions about it,
> > > > > but I was under the impression that this was expected / normal. Thus
> > > > > we can't make this DRM-managed.
> > > >
> > > > Since I didn't hear a reply,
> > >
> > > Gah, I replied but it looks like somehow it never reached the ML...
> > >
> > > Here was my original reply:
> > >
> > > > > > This adds a devm managed version of drm_bridge_add(). Like other
> > > > > > "devm" function listed in drm_bridge.h, this function takes an
> > > > > > explicit "dev" to use for the lifetime management. A few notes:
> > > > > > * In general we have a "struct device" for bridges that makes a good
> > > > > >   candidate for where the lifetime matches exactly what we want.
> > > > > > * The "bridge->dev->dev" device appears to be the encoder
> > > > > >   device. That's not the right device to use for lifetime 
> > > > > > management.
> > > > > >
> > > > > > Suggested-by: Dmitry Baryshkov 
> > > > > > Signed-off-by: Douglas Anderson 
> > > > >
> > > > > If we are to introduce more managed helpers, I think it'd be wiser to
> > > > > introduce them as DRM-managed, and not device managed.
> > > > >
> > > > > Otherwise, you'll end up in a weird state when a device has been 
> > > > > removed
> > > > > but the DRM device is still around.
> > > >=20
> > > > I'm kinda confused. In this case there is no DRM device for the bridge
> > > > and, as per my CL description, "bridge-dev->dev" appears to be the
> > > > encoder device.
> > >
> > > bridge->dev seems right though?
> > >
> > > > I wasn't personally involved in discussions about it, but I was under
> > > > the impression that this was expected / normal. Thus we can't make
> > > > this DRM-managed.
> > >
> > > Still, I don't think devm is the right solution to this either.
> > >
> > > The underlying issue is two-fold:
> > >
> > >   - Encoders can have a pointer to a bridge through of_drm_find_bridge
> > > or similar. However, bridges are traditionally tied to their device
> > > lifetime (by calling drm_bridge_add in probe, and drm_bridge_remove
> > > in remove). Encoders will typically be tied to the DRM device
> > > however, and that one sticks around until the last application
> > > closes it. We can thus very easily end up with a dangling pointer,
> > > and a use-after-free.
> > >
> > >   - It's not the case yet, but it doesn't seem far fetch to expose
> > > properties of bridges to the userspace. In that case, the userspace
> > > would be likely to still hold references to objects that aren't
> > > there anymore when the bridge is gone.
> > >
> > > The first is obviously a larger concern, but if we can find a solution
> > > that would accomodate the second it would be great.
> > >
> > > As far as I can see, we should fix in two steps:
> > >
> > >   - in drm_bridge_attach, we should add a device-managed call that will
> > > unregister the main DRM device. We don't allow to probe the main DRM
> > > device when the bridge isn't there yet in most case, so it makes
> > > sense to remove it once the bridge is no longer there as well.
> >
> > The problem is that I do not see a good way to unregister the main DRM
> > device outside of it's driver code.
> >
> > >
> > >   - When the DRM device is removed, have

Re: [PATCH 0/6] drm: Add mouse cursor hotspot support to atomic KMS

Hi,

Please, read this thread:
https://lists.freedesktop.org/archives/dri-devel/2020-March/thread.html#259615

It has a lot of information about the pitfalls of cursor hotspot and
other things done by VM software.

In particular: since the driver will ignore the KMS cursor plane
position set by user-space, I don't think it's okay to just expose
without opt-in from user-space (e.g. with a DRM_CLIENT_CAP).

cc wayland-devel and Pekka for user-space feedback.

On Thursday, June 2nd, 2022 at 17:42, Zack Rusin  wrote:

> - all userspace code needs to hardcore a list of drivers which require
> hotspots because there's no way to query from drm "does this driver
> require hotspot"

Can you elaborate? I'm not sure I understand what you mean here.

Thanks,

Simon

Re: [PATCH v3 3/4] drm/bridge: Add devm_drm_bridge_add()

On Fri, Jun 03, 2022 at 01:19:16PM +0300, Dmitry Baryshkov wrote:
> On Fri, 3 Jun 2022 at 11:21, Maxime Ripard  wrote:
> >
> > On Tue, May 31, 2022 at 02:06:34PM -0700, Doug Anderson wrote:
> > > On Mon, May 23, 2022 at 10:00 AM Doug Anderson  
> > > wrote:
> > > > On Sat, May 21, 2022 at 2:17 AM Maxime Ripard  wrote:
> > > > > On Tue, May 10, 2022 at 12:29:43PM -0700, Douglas Anderson wrote:
> > > > > > This adds a devm managed version of drm_bridge_add(). Like other
> > > > > > "devm" function listed in drm_bridge.h, this function takes an
> > > > > > explicit "dev" to use for the lifetime management. A few notes:
> > > > > > * In general we have a "struct device" for bridges that makes a good
> > > > > >   candidate for where the lifetime matches exactly what we want.
> > > > > > * The "bridge->dev->dev" device appears to be the encoder
> > > > > >   device. That's not the right device to use for lifetime 
> > > > > > management.
> > > > > >
> > > > > > Suggested-by: Dmitry Baryshkov 
> > > > > > Signed-off-by: Douglas Anderson 
> > > > >
> > > > > If we are to introduce more managed helpers, I think it'd be wiser to
> > > > > introduce them as DRM-managed, and not device managed.
> > > > >
> > > > > Otherwise, you'll end up in a weird state when a device has been 
> > > > > removed
> > > > > but the DRM device is still around.
> > > >
> > > > I'm kinda confused. In this case there is no DRM device for the bridge
> > > > and, as per my CL description, "bridge-dev->dev" appears to be the
> > > > encoder device. I wasn't personally involved in discussions about it,
> > > > but I was under the impression that this was expected / normal. Thus
> > > > we can't make this DRM-managed.
> > >
> > > Since I didn't hear a reply,
> >
> > Gah, I replied but it looks like somehow it never reached the ML...
> >
> > Here was my original reply:
> >
> > > > > This adds a devm managed version of drm_bridge_add(). Like other
> > > > > "devm" function listed in drm_bridge.h, this function takes an
> > > > > explicit "dev" to use for the lifetime management. A few notes:
> > > > > * In general we have a "struct device" for bridges that makes a good
> > > > >   candidate for where the lifetime matches exactly what we want.
> > > > > * The "bridge->dev->dev" device appears to be the encoder
> > > > >   device. That's not the right device to use for lifetime management.
> > > > >
> > > > > Suggested-by: Dmitry Baryshkov 
> > > > > Signed-off-by: Douglas Anderson 
> > > >
> > > > If we are to introduce more managed helpers, I think it'd be wiser to
> > > > introduce them as DRM-managed, and not device managed.
> > > >
> > > > Otherwise, you'll end up in a weird state when a device has been removed
> > > > but the DRM device is still around.
> > >=20
> > > I'm kinda confused. In this case there is no DRM device for the bridge
> > > and, as per my CL description, "bridge-dev->dev" appears to be the
> > > encoder device.
> >
> > bridge->dev seems right though?
> >
> > > I wasn't personally involved in discussions about it, but I was under
> > > the impression that this was expected / normal. Thus we can't make
> > > this DRM-managed.
> >
> > Still, I don't think devm is the right solution to this either.
> >
> > The underlying issue is two-fold:
> >
> >   - Encoders can have a pointer to a bridge through of_drm_find_bridge
> > or similar. However, bridges are traditionally tied to their device
> > lifetime (by calling drm_bridge_add in probe, and drm_bridge_remove
> > in remove). Encoders will typically be tied to the DRM device
> > however, and that one sticks around until the last application
> > closes it. We can thus very easily end up with a dangling pointer,
> > and a use-after-free.
> >
> >   - It's not the case yet, but it doesn't seem far fetch to expose
> > properties of bridges to the userspace. In that case, the userspace
> > would be likely to still hold references to objects that aren't
> > there anymore when the bridge is gone.
> >
> > The first is obviously a larger concern, but if we can find a solution
> > that would accomodate the second it would be great.
> >
> > As far as I can see, we should fix in two steps:
> >
> >   - in drm_bridge_attach, we should add a device-managed call that will
> > unregister the main DRM device. We don't allow to probe the main DRM
> > device when the bridge isn't there yet in most case, so it makes
> > sense to remove it once the bridge is no longer there as well.
> 
> The problem is that I do not see a good way to unregister the main DRM
> device outside of it's driver code.

That's what drmm helpers are doing though: they'll defer the cleanup
until the last user has closed its fd.

> >   - When the DRM device is removed, have the core cleanup any bridge
> > registered. That will remove the need to have drm_bridge_remove in
> > the first place.
> >
> > > I'll assume that my response addressed your concerns. Assuming I get
>

[PATCH v3 2/2] drm: bridge: Add TI DLPC3433 DSI to DMD bridge

2022-06-03 Thread Jagan Teki

TI DLPC3433 is a MIPI DSI based display controller bridge
for processing high resolution DMD based projectors.

It has a flexible configuration of MIPI DSI and DPI signal
input that produces a DMD output in RGB565, RGB666, RGB888
formats.

It supports upto 720p resolution with 60 and 120 Hz refresh
rates.

Add bridge driver for it.

Signed-off-by: Christopher Vollo 
Signed-off-by: Jagan Teki 
---
Changes for v3:
- none
Changes for v2:
- fixed license
- filled display size buffer
- fixed power off
- fixed dev_err_probe

 MAINTAINERS  |   1 +
 drivers/gpu/drm/bridge/Kconfig   |  16 +
 drivers/gpu/drm/bridge/Makefile  |   1 +
 drivers/gpu/drm/bridge/ti-dlpc3433.c | 417 +++
 4 files changed, 435 insertions(+)
 create mode 100644 drivers/gpu/drm/bridge/ti-dlpc3433.c

diff --git a/MAINTAINERS b/MAINTAINERS
index aea0fe5156af..ede21cc48708 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6435,6 +6435,7 @@ DRM DRIVER FOR TI DLPC3433 MIPI DSI TO DMD BRIDGE
 M: Jagan Teki 
 S: Maintained
 F: Documentation/devicetree/bindings/display/bridge/ti,dlpc3433.yaml
+F: drivers/gpu/drm/bridge/ti-dlpc3433.c
 
 DRM DRIVER FOR TI SN65DSI86 BRIDGE CHIP
 R: Douglas Anderson 
diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
index 146ab069838f..4b28f939fff6 100644
--- a/drivers/gpu/drm/bridge/Kconfig
+++ b/drivers/gpu/drm/bridge/Kconfig
@@ -321,6 +321,22 @@ config DRM_TOSHIBA_TC358775
help
  Toshiba TC358775 DSI/LVDS bridge chip driver.
 
+config DRM_TI_DLPC3433
+   tristate "TI DLPC3433 Display controller"
+   depends on DRM && DRM_PANEL
+   depends on OF
+   select DRM_MIPI_DSI
+   help
+ TI DLPC3433 is a MIPI DSI based display controller bridge
+ for processing high resolution DMD based projectors.
+
+ It has a flexible configuration of MIPI DSI and DPI signal
+ input that produces a DMD output in RGB565, RGB666, RGB888
+ formats.
+
+ It supports upto 720p resolution with 60 and 120 Hz refresh
+ rates.
+
 config DRM_TI_TFP410
tristate "TI TFP410 DVI/HDMI bridge"
depends on OF
diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
index f6c0a95de549..043b499545e3 100644
--- a/drivers/gpu/drm/bridge/Makefile
+++ b/drivers/gpu/drm/bridge/Makefile
@@ -26,6 +26,7 @@ obj-$(CONFIG_DRM_TOSHIBA_TC358767) += tc358767.o
 obj-$(CONFIG_DRM_TOSHIBA_TC358768) += tc358768.o
 obj-$(CONFIG_DRM_TOSHIBA_TC358775) += tc358775.o
 obj-$(CONFIG_DRM_I2C_ADV7511) += adv7511/
+obj-$(CONFIG_DRM_TI_DLPC3433) += ti-dlpc3433.o
 obj-$(CONFIG_DRM_TI_SN65DSI83) += ti-sn65dsi83.o
 obj-$(CONFIG_DRM_TI_SN65DSI86) += ti-sn65dsi86.o
 obj-$(CONFIG_DRM_TI_TFP410) += ti-tfp410.o
diff --git a/drivers/gpu/drm/bridge/ti-dlpc3433.c 
b/drivers/gpu/drm/bridge/ti-dlpc3433.c
new file mode 100644
index ..06e519798ac5
--- /dev/null
+++ b/drivers/gpu/drm/bridge/ti-dlpc3433.c
@@ -0,0 +1,417 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2021 RenewOutReach
+ * Copyright (C) 2021 Amarula Solutions(India)
+ *
+ * Author:
+ * Jagan Teki 
+ * Christopher Vollo 
+ */
+
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+enum cmd_registers {
+   WR_INPUT_SOURCE = 0x05, /* Write Input Source Select */
+   WR_EXT_SOURCE_FMT   = 0x07, /* Write External Video Source Format */
+   WR_IMAGE_CROP   = 0x10, /* Write Image Crop */
+   WR_DISPLAY_SIZE = 0x12, /* Write Display Size */
+   WR_IMAGE_FREEZE = 0x1A, /* Write Image Freeze */
+   WR_INPUT_IMAGE_SIZE = 0x2E, /* Write External Input Image Size */
+   WR_RGB_LED_EN   = 0x52, /* Write RGB LED Enable */
+   WR_RGB_LED_CURRENT  = 0x54, /* Write RGB LED Current */
+   WR_RGB_LED_MAX_CURRENT  = 0x5C, /* Write RGB LED Max Current */
+   WR_DSI_HS_CLK   = 0xBD, /* Write DSI HS Clock */
+   RD_DEVICE_ID= 0xD4, /* Read Controller Device ID */
+   WR_DSI_PORT_EN  = 0xD7, /* Write DSI Port Enable */
+};
+
+enum input_source {
+   INPUT_EXTERNAL_VIDEO= 0,
+   INPUT_TEST_PATTERN,
+   INPUT_SPLASH_SCREEN,
+};
+
+#define DEV_ID_MASKGENMASK(3, 0)
+#define IMAGE_FREESE_ENBIT(0)
+#define DSI_PORT_EN0
+#define EXT_SOURCE_FMT_DSI 0
+#define RED_LED_EN BIT(0)
+#define GREEN_LED_EN   BIT(1)
+#define BLUE_LED_ENBIT(2)
+#define LED_MASK   GENMASK(2, 0)
+#define MAX_BYTE_SIZE  8
+
+struct dlpc {
+   struct device   *dev;
+   struct drm_bridge   bridge;
+   struct drm_bridge   *next_bridge;
+   struct device_node  *host_node;
+   struct mipi_dsi_device  *dsi;
+   struct drm_display_mode mode;
+
+   struct gpio_desc*enable_gpio;
+   struct regulator

[PATCH v3 1/2] dt-bindings: display: bridge: Add TI DLPC3433 DSI to DMD

2022-06-03 Thread Jagan Teki

TI DLPC3433 is a MIPI DSI based display controller bridge
for processing high resolution DMD based projectors.

It has a flexible configuration of MIPI DSI and DPI signal
input that produces a DMD output in RGB565, RGB666, RGB888
formats.

It supports upto 720p resolution with 60 and 120 Hz refresh
rates.

Add dt-bingings for it.

Signed-off-by: Christopher Vollo 
Signed-off-by: Jagan Teki 
Reviewed-by: Rob Herring 
---
Changes for v3:
- collect Rob r-b
Changes for v2:
- fix compatible
- drop reg description
- fix enable_gpio description
- fix port@2

 .../bindings/display/bridge/ti,dlpc3433.yaml  | 117 ++
 MAINTAINERS   |   5 +
 2 files changed, 122 insertions(+)
 create mode 100644 
Documentation/devicetree/bindings/display/bridge/ti,dlpc3433.yaml

diff --git a/Documentation/devicetree/bindings/display/bridge/ti,dlpc3433.yaml 
b/Documentation/devicetree/bindings/display/bridge/ti,dlpc3433.yaml
new file mode 100644
index ..542193d77cdf
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/bridge/ti,dlpc3433.yaml
@@ -0,0 +1,117 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/bridge/ti,dlpc3433.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: TI DLPC3433 MIPI DSI to DMD bridge
+
+maintainers:
+  - Jagan Teki 
+  - Christopher Vollo 
+
+description: |
+  TI DLPC3433 is a MIPI DSI based display controller bridge
+  for processing high resolution DMD based projectors.
+
+  It has a flexible configuration of MIPI DSI and DPI signal
+  input that produces a DMD output in RGB565, RGB666, RGB888
+  formats.
+
+  It supports upto 720p resolution with 60 and 120 Hz refresh
+  rates.
+
+properties:
+  compatible:
+const: ti,dlpc3433
+
+  reg:
+enum:
+  - 0x1b
+  - 0x1d
+
+  enable-gpios:
+description: PROJ_ON pin, chip powers up PROJ_ON is high.
+
+  vcc_intf-supply:
+description: A 1.8V/3.3V supply that power the Host I/O.
+
+  vcc_flsh-supply:
+description: A 1.8V/3.3V supply that power the Flash I/O.
+
+  ports:
+$ref: /schemas/graph.yaml#/properties/ports
+
+properties:
+  port@0:
+$ref: /schemas/graph.yaml#/$defs/port-base
+unevaluatedProperties: false
+description: Video port for MIPI DSI input.
+
+properties:
+  endpoint:
+$ref: /schemas/media/video-interfaces.yaml#
+unevaluatedProperties: false
+
+properties:
+  data-lanes:
+description: array of physical DSI data lane indexes.
+minItems: 1
+items:
+  - const: 1
+  - const: 2
+  - const: 3
+  - const: 4
+
+  port@1:
+$ref: /schemas/graph.yaml#/properties/port
+description: Video port for DMD output.
+
+required:
+  - port@0
+  - port@1
+
+required:
+  - compatible
+  - reg
+  - enable-gpios
+  - ports
+
+additionalProperties: false
+
+examples:
+  - |
+#include 
+
+i2c1 {
+#address-cells = <1>;
+#size-cells = <0>;
+
+bridge@1b {
+compatible = "ti,dlpc3433";
+reg = <0x1b>;
+enable-gpios = < 1 GPIO_ACTIVE_HIGH>;
+
+ports {
+#address-cells = <1>;
+#size-cells = <0>;
+
+port@0 {
+reg = <0>;
+
+bridge_in_dsi: endpoint {
+remote-endpoint = <_out_bridge>;
+data-lanes = <1 2 3 4>;
+};
+};
+
+port@1 {
+reg = <1>;
+
+bridge_out_panel: endpoint {
+remote-endpoint = <_out_bridge>;
+};
+};
+};
+};
+};
diff --git a/MAINTAINERS b/MAINTAINERS
index 11da16bfa123..aea0fe5156af 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6431,6 +6431,11 @@ DRM DRIVER FOR TDFX VIDEO CARDS
 S: Orphan / Obsolete
 F: drivers/gpu/drm/tdfx/
 
+DRM DRIVER FOR TI DLPC3433 MIPI DSI TO DMD BRIDGE
+M: Jagan Teki 
+S: Maintained
+F: Documentation/devicetree/bindings/display/bridge/ti,dlpc3433.yaml
+
 DRM DRIVER FOR TI SN65DSI86 BRIDGE CHIP
 R: Douglas Anderson 
 F: Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml
-- 
2.25.1

Re: [PATCH v2] drm/tegra: dc: rgb: Fix refcount leak in tegra_dc_rgb_probe

2022-06-03 Thread Dmitry Osipenko

On 6/3/22 16:27, Miaoqian Lin wrote:
> of_get_child_by_name() returns a node pointer with refcount
> incremented, we should use of_node_put() on it when not need anymore.
> So add of_node_put() in error paths.
> 
> Fixes: d8f4a9eda006 ("drm: Add NVIDIA Tegra20 support")
> Signed-off-by: Miaoqian Lin 
> ---
> changes in v2:
> - update Fixes tag.
> v1 Link: https://lore.kernel.org/r/20220602155615.43277-1-linmq...@gmail.com
> ---
>  drivers/gpu/drm/tegra/rgb.c | 31 +--
>  1 file changed, 21 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tegra/rgb.c b/drivers/gpu/drm/tegra/rgb.c
> index ff8fce36d2aa..cef2b1b72385 100644
> --- a/drivers/gpu/drm/tegra/rgb.c
> +++ b/drivers/gpu/drm/tegra/rgb.c
> @@ -196,12 +196,16 @@ int tegra_dc_rgb_probe(struct tegra_dc *dc)
>   int err;
>  
>   np = of_get_child_by_name(dc->dev->of_node, "rgb");
> - if (!np || !of_device_is_available(np))
> - return -ENODEV;
> + if (!np || !of_device_is_available(np)) {
> + err = -ENODEV;
> + goto err_put_node;
> + }
>  
>   rgb = devm_kzalloc(dc->dev, sizeof(*rgb), GFP_KERNEL);
> - if (!rgb)
> - return -ENOMEM;
> + if (!rgb) {
> + err = -ENOMEM;
> + goto err_put_node;
> + }
>  
>   rgb->output.dev = dc->dev;
>   rgb->output.of_node = np;
> @@ -209,31 +213,34 @@ int tegra_dc_rgb_probe(struct tegra_dc *dc)
>  
>   err = tegra_output_probe(>output);
>   if (err < 0)
> - return err;
> + goto err_put_node;
> +
>  
>   rgb->clk = devm_clk_get(dc->dev, NULL);
>   if (IS_ERR(rgb->clk)) {
>   dev_err(dc->dev, "failed to get clock\n");
> - return PTR_ERR(rgb->clk);
> + err =  PTR_ERR(rgb->clk);
> + goto err_put_node;
>   }
>  
>   rgb->clk_parent = devm_clk_get(dc->dev, "parent");
>   if (IS_ERR(rgb->clk_parent)) {
>   dev_err(dc->dev, "failed to get parent clock\n");
> - return PTR_ERR(rgb->clk_parent);
> + err = PTR_ERR(rgb->clk_parent);
> + goto err_put_node;
>   }
>  
>   err = clk_set_parent(rgb->clk, rgb->clk_parent);
>   if (err < 0) {
>   dev_err(dc->dev, "failed to set parent clock: %d\n", err);
> - return err;
> + goto err_put_node;
>   }
>  
>   rgb->pll_d_out0 = clk_get_sys(NULL, "pll_d_out0");
>   if (IS_ERR(rgb->pll_d_out0)) {
>   err = PTR_ERR(rgb->pll_d_out0);
>   dev_err(dc->dev, "failed to get pll_d_out0: %d\n", err);
> - return err;
> + goto err_put_node;
>   }
>  
>   if (dc->soc->has_pll_d2_out0) {
> @@ -241,13 +248,17 @@ int tegra_dc_rgb_probe(struct tegra_dc *dc)
>   if (IS_ERR(rgb->pll_d2_out0)) {
>   err = PTR_ERR(rgb->pll_d2_out0);
>   dev_err(dc->dev, "failed to get pll_d2_out0: %d\n", 
> err);
> - return err;
> + goto err_put_node;
>   }
>   }
>  
>   dc->rgb = >output;
>  
>   return 0;
> +
> +err_put_node:
> + of_node_put(np);
> + return err;
>  }
>  
>  int tegra_dc_rgb_remove(struct tegra_dc *dc)

Doesn't look like the node is put on driver removal either. Hence you
should make it resource-managed.

-- 
Best regards,
Dmitry

[PATCH] drm/stm: ltdc: update hardware error management

The latest hardware version (0x40100) supports a hardware threshold
register (aka FUTR) to trigger a fifo underrun interrupt.
A software threshold has been implemented for other hardware versions.
The threshold is set to 128 by default.

Signed-off-by: Yannick Fertre 
---
 drivers/gpu/drm/stm/ltdc.c | 90 ++
 drivers/gpu/drm/stm/ltdc.h |  6 ++-
 2 files changed, 77 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/stm/ltdc.c b/drivers/gpu/drm/stm/ltdc.c
index ff2075dd9474..42a3bd515477 100644
--- a/drivers/gpu/drm/stm/ltdc.c
+++ b/drivers/gpu/drm/stm/ltdc.c
@@ -162,16 +162,20 @@
 #define BCCR_BCWHITE   GENMASK(23, 0)  /* Background Color WHITE */
 
 #define IER_LIEBIT(0)  /* Line Interrupt Enable */
-#define IER_FUIE   BIT(1)  /* Fifo Underrun Interrupt Enable */
+#define IER_FUWIE  BIT(1)  /* Fifo Underrun Warning Interrupt 
Enable */
 #define IER_TERRIE BIT(2)  /* Transfer ERRor Interrupt Enable */
-#define IER_RRIE   BIT(3)  /* Register Reload Interrupt enable */
+#define IER_RRIE   BIT(3)  /* Register Reload Interrupt Enable */
+#define IER_FUEIE  BIT(6)  /* Fifo Underrun Error Interrupt Enable 
*/
+#define IER_CRCIE  BIT(7)  /* CRC Error Interrupt Enable */
 
 #define CPSR_CYPOS GENMASK(15, 0)  /* Current Y position */
 
 #define ISR_LIFBIT(0)  /* Line Interrupt Flag */
-#define ISR_FUIF   BIT(1)  /* Fifo Underrun Interrupt Flag */
+#define ISR_FUWIF  BIT(1)  /* Fifo Underrun Warning Interrupt Flag 
*/
 #define ISR_TERRIF BIT(2)  /* Transfer ERRor Interrupt Flag */
 #define ISR_RRIF   BIT(3)  /* Register Reload Interrupt Flag */
+#define ISR_FUEIF  BIT(6)  /* Fifo Underrun Error Interrupt Flag */
+#define ISR_CRCIF  BIT(7)  /* CRC Error Interrupt Flag */
 
 #define EDCR_OCYEN BIT(25) /* Output Conversion to YCbCr 422: 
ENable */
 #define EDCR_OCYSELBIT(26) /* Output Conversion to YCbCr 422: 
SELection of the CCIR */
@@ -231,6 +235,8 @@
 
 #define NB_PF  8   /* Max nb of HW pixel format */
 
+#define FUT_DFT128 /* Default value of fifo 
underrun threshold */
+
 /*
  * Skip the first value and the second in case CRC was enabled during
  * the thread irq. This is to be sure CRC value is relevant for the
@@ -711,12 +717,13 @@ static irqreturn_t ltdc_irq_thread(int irq, void *arg)
ltdc_irq_crc_handle(ldev, crtc);
}
 
-   /* Save FIFO Underrun & Transfer Error status */
mutex_lock(>err_lock);
-   if (ldev->irq_status & ISR_FUIF)
-   ldev->error_status |= ISR_FUIF;
if (ldev->irq_status & ISR_TERRIF)
-   ldev->error_status |= ISR_TERRIF;
+   ldev->transfer_err++;
+   if (ldev->irq_status & ISR_FUEIF)
+   ldev->fifo_err++;
+   if (ldev->irq_status & ISR_FUWIF)
+   ldev->fifo_warn++;
mutex_unlock(>err_lock);
 
return IRQ_HANDLED;
@@ -775,7 +782,7 @@ static void ltdc_crtc_atomic_enable(struct drm_crtc *crtc,
regmap_write(ldev->regmap, LTDC_BCCR, BCCR_BCBLACK);
 
/* Enable IRQ */
-   regmap_set_bits(ldev->regmap, LTDC_IER, IER_RRIE | IER_FUIE | 
IER_TERRIE);
+   regmap_set_bits(ldev->regmap, LTDC_IER, IER_FUWIE | IER_FUEIE | 
IER_RRIE | IER_TERRIE);
 
/* Commit shadow registers = update planes at next vblank */
if (!ldev->caps.plane_reg_shadow)
@@ -801,13 +808,20 @@ static void ltdc_crtc_atomic_disable(struct drm_crtc 
*crtc,
  LXCR_CLUTEN | LXCR_LEN, 0);
 
/* disable IRQ */
-   regmap_clear_bits(ldev->regmap, LTDC_IER, IER_RRIE | IER_FUIE | 
IER_TERRIE);
+   regmap_clear_bits(ldev->regmap, LTDC_IER, IER_FUWIE | IER_FUEIE | 
IER_RRIE | IER_TERRIE);
 
/* immediately commit disable of layers before switching off LTDC */
if (!ldev->caps.plane_reg_shadow)
regmap_set_bits(ldev->regmap, LTDC_SRCR, SRCR_IMR);
 
pm_runtime_put_sync(ddev->dev);
+
+   /*  clear interrupt error counters */
+   mutex_lock(>err_lock);
+   ldev->transfer_err = 0;
+   ldev->fifo_err = 0;
+   ldev->fifo_warn = 0;
+   mutex_unlock(>err_lock);
 }
 
 #define CLK_TOLERANCE_HZ 50
@@ -1168,6 +1182,18 @@ static int ltdc_crtc_verify_crc_source(struct drm_crtc 
*crtc,
return 0;
 }
 
+static void ltdc_crtc_atomic_print_state(struct drm_printer *p,
+const struct drm_crtc_state *state)
+{
+   struct drm_crtc *crtc = state->crtc;
+   struct ltdc_device *ldev = crtc_to_ltdc(crtc);
+
+   drm_printf(p, "\ttransfer_error=%d\n", ldev->transfer_err);
+   drm_printf(p, "\tfifo_underrun_error=%d\n", ldev->fifo_err);
+   drm_printf(p, "\tfifo_underrun_warning=%d\n", ldev->fifo_warn);
+

Re: [PATCH v3 3/4] drm/bridge: Add devm_drm_bridge_add()

2022-06-03 Thread Doug Anderson

Hi,

On Fri, Jun 3, 2022 at 3:19 AM Dmitry Baryshkov
 wrote:
>
> On Fri, 3 Jun 2022 at 11:21, Maxime Ripard  wrote:
> >
> > On Tue, May 31, 2022 at 02:06:34PM -0700, Doug Anderson wrote:
> > > On Mon, May 23, 2022 at 10:00 AM Doug Anderson  
> > > wrote:
> > > > On Sat, May 21, 2022 at 2:17 AM Maxime Ripard  wrote:
> > > > > On Tue, May 10, 2022 at 12:29:43PM -0700, Douglas Anderson wrote:
> > > > > > This adds a devm managed version of drm_bridge_add(). Like other
> > > > > > "devm" function listed in drm_bridge.h, this function takes an
> > > > > > explicit "dev" to use for the lifetime management. A few notes:
> > > > > > * In general we have a "struct device" for bridges that makes a good
> > > > > >   candidate for where the lifetime matches exactly what we want.
> > > > > > * The "bridge->dev->dev" device appears to be the encoder
> > > > > >   device. That's not the right device to use for lifetime 
> > > > > > management.
> > > > > >
> > > > > > Suggested-by: Dmitry Baryshkov 
> > > > > > Signed-off-by: Douglas Anderson 
> > > > >
> > > > > If we are to introduce more managed helpers, I think it'd be wiser to
> > > > > introduce them as DRM-managed, and not device managed.
> > > > >
> > > > > Otherwise, you'll end up in a weird state when a device has been 
> > > > > removed
> > > > > but the DRM device is still around.
> > > >
> > > > I'm kinda confused. In this case there is no DRM device for the bridge
> > > > and, as per my CL description, "bridge-dev->dev" appears to be the
> > > > encoder device. I wasn't personally involved in discussions about it,
> > > > but I was under the impression that this was expected / normal. Thus
> > > > we can't make this DRM-managed.
> > >
> > > Since I didn't hear a reply,
> >
> > Gah, I replied but it looks like somehow it never reached the ML...
> >
> > Here was my original reply:
> >
> > > > > This adds a devm managed version of drm_bridge_add(). Like other
> > > > > "devm" function listed in drm_bridge.h, this function takes an
> > > > > explicit "dev" to use for the lifetime management. A few notes:
> > > > > * In general we have a "struct device" for bridges that makes a good
> > > > >   candidate for where the lifetime matches exactly what we want.
> > > > > * The "bridge->dev->dev" device appears to be the encoder
> > > > >   device. That's not the right device to use for lifetime management.
> > > > >
> > > > > Suggested-by: Dmitry Baryshkov 
> > > > > Signed-off-by: Douglas Anderson 
> > > >
> > > > If we are to introduce more managed helpers, I think it'd be wiser to
> > > > introduce them as DRM-managed, and not device managed.
> > > >
> > > > Otherwise, you'll end up in a weird state when a device has been removed
> > > > but the DRM device is still around.
> > >=20
> > > I'm kinda confused. In this case there is no DRM device for the bridge
> > > and, as per my CL description, "bridge-dev->dev" appears to be the
> > > encoder device.
> >
> > bridge->dev seems right though?
> >
> > > I wasn't personally involved in discussions about it, but I was under
> > > the impression that this was expected / normal. Thus we can't make
> > > this DRM-managed.
> >
> > Still, I don't think devm is the right solution to this either.
> >
> > The underlying issue is two-fold:
> >
> >   - Encoders can have a pointer to a bridge through of_drm_find_bridge
> > or similar. However, bridges are traditionally tied to their device
> > lifetime (by calling drm_bridge_add in probe, and drm_bridge_remove
> > in remove). Encoders will typically be tied to the DRM device
> > however, and that one sticks around until the last application
> > closes it. We can thus very easily end up with a dangling pointer,
> > and a use-after-free.
> >
> >   - It's not the case yet, but it doesn't seem far fetch to expose
> > properties of bridges to the userspace. In that case, the userspace
> > would be likely to still hold references to objects that aren't
> > there anymore when the bridge is gone.
> >
> > The first is obviously a larger concern, but if we can find a solution
> > that would accomodate the second it would be great.
> >
> > As far as I can see, we should fix in two steps:
> >
> >   - in drm_bridge_attach, we should add a device-managed call that will
> > unregister the main DRM device. We don't allow to probe the main DRM
> > device when the bridge isn't there yet in most case, so it makes
> > sense to remove it once the bridge is no longer there as well.
>
> The problem is that I do not see a good way to unregister the main DRM
> device outside of it's driver code.
>
> >
> >   - When the DRM device is removed, have the core cleanup any bridge
> > registered. That will remove the need to have drm_bridge_remove in
> > the first place.
> >
> > > I'll assume that my response addressed your concerns. Assuming I get
> > > reviews for the other two patches in this series I'll plan to land
> > > this with Dmitry's review.
> >

[PATCH v2] drm/tegra: dc: rgb: Fix refcount leak in tegra_dc_rgb_probe

2022-06-03 Thread Miaoqian Lin

of_get_child_by_name() returns a node pointer with refcount
incremented, we should use of_node_put() on it when not need anymore.
So add of_node_put() in error paths.

Fixes: d8f4a9eda006 ("drm: Add NVIDIA Tegra20 support")
Signed-off-by: Miaoqian Lin 
---
changes in v2:
- update Fixes tag.
v1 Link: https://lore.kernel.org/r/20220602155615.43277-1-linmq...@gmail.com
---
 drivers/gpu/drm/tegra/rgb.c | 31 +--
 1 file changed, 21 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/tegra/rgb.c b/drivers/gpu/drm/tegra/rgb.c
index ff8fce36d2aa..cef2b1b72385 100644
--- a/drivers/gpu/drm/tegra/rgb.c
+++ b/drivers/gpu/drm/tegra/rgb.c
@@ -196,12 +196,16 @@ int tegra_dc_rgb_probe(struct tegra_dc *dc)
int err;
 
np = of_get_child_by_name(dc->dev->of_node, "rgb");
-   if (!np || !of_device_is_available(np))
-   return -ENODEV;
+   if (!np || !of_device_is_available(np)) {
+   err = -ENODEV;
+   goto err_put_node;
+   }
 
rgb = devm_kzalloc(dc->dev, sizeof(*rgb), GFP_KERNEL);
-   if (!rgb)
-   return -ENOMEM;
+   if (!rgb) {
+   err = -ENOMEM;
+   goto err_put_node;
+   }
 
rgb->output.dev = dc->dev;
rgb->output.of_node = np;
@@ -209,31 +213,34 @@ int tegra_dc_rgb_probe(struct tegra_dc *dc)
 
err = tegra_output_probe(>output);
if (err < 0)
-   return err;
+   goto err_put_node;
+
 
rgb->clk = devm_clk_get(dc->dev, NULL);
if (IS_ERR(rgb->clk)) {
dev_err(dc->dev, "failed to get clock\n");
-   return PTR_ERR(rgb->clk);
+   err =  PTR_ERR(rgb->clk);
+   goto err_put_node;
}
 
rgb->clk_parent = devm_clk_get(dc->dev, "parent");
if (IS_ERR(rgb->clk_parent)) {
dev_err(dc->dev, "failed to get parent clock\n");
-   return PTR_ERR(rgb->clk_parent);
+   err = PTR_ERR(rgb->clk_parent);
+   goto err_put_node;
}
 
err = clk_set_parent(rgb->clk, rgb->clk_parent);
if (err < 0) {
dev_err(dc->dev, "failed to set parent clock: %d\n", err);
-   return err;
+   goto err_put_node;
}
 
rgb->pll_d_out0 = clk_get_sys(NULL, "pll_d_out0");
if (IS_ERR(rgb->pll_d_out0)) {
err = PTR_ERR(rgb->pll_d_out0);
dev_err(dc->dev, "failed to get pll_d_out0: %d\n", err);
-   return err;
+   goto err_put_node;
}
 
if (dc->soc->has_pll_d2_out0) {
@@ -241,13 +248,17 @@ int tegra_dc_rgb_probe(struct tegra_dc *dc)
if (IS_ERR(rgb->pll_d2_out0)) {
err = PTR_ERR(rgb->pll_d2_out0);
dev_err(dc->dev, "failed to get pll_d2_out0: %d\n", 
err);
-   return err;
+   goto err_put_node;
}
}
 
dc->rgb = >output;
 
return 0;
+
+err_put_node:
+   of_node_put(np);
+   return err;
 }
 
 int tegra_dc_rgb_remove(struct tegra_dc *dc)
-- 
2.25.1

Re: [RFC PATCH 3/5] drm/amdgpu: Allow explicit sync for VM ops.

On Fri, Jun 3, 2022 at 2:49 PM Christian König  wrote:
>
> Am 03.06.22 um 14:39 schrieb Bas Nieuwenhuizen:
> > On Fri, Jun 3, 2022 at 2:08 PM Christian König  
> > wrote:
> >> Am 03.06.22 um 13:07 schrieb Bas Nieuwenhuizen:
> >>> On Fri, Jun 3, 2022 at 12:16 PM Christian König
> >>>  wrote:
>  Am 03.06.22 um 12:08 schrieb Bas Nieuwenhuizen:
> > [SNIP]
> >>> I do have to fix some stuff indeed, especially for the GEM close but
> >>> with that we should be able to keep the same basic approach?
> >> Nope, not even remotely.
> >>
> >> What we need is the following:
> >> 1. Rolling out my drm_exec patch set, so that we can lock buffers as 
> >> needed.
> >> 2. When we get a VM operation we not only lock the VM page tables, but
> >> also all buffers we potentially need to unmap.
> >> 3. Nuking the freed list in the amdgpu_vm structure by updating freed
> >> areas directly when they are unmapped.
> >> 4. Tracking those updates inside the bo_va structure for the BO+VM
> >> combination.
> >> 5. When the bo_va structure is destroy because of closing the handle
> >> move the last clear operation over to the VM as implicit sync.
> >>
> > Hi Christian, isn't that a different problem though (that we're also
> > trying to solve, but in your series)?
> >
> > What this patch tries to achieve:
> >
> > (t+0) CS submission setting BOOKKEEP fences (i.e. no implicit sync)
> > (t+1) a VM operation on a BO/VM accessed by the CS.
> >
> > to run concurrently. What it *doesn't* try is
> >
> > (t+0) a VM operation on a BO/VM accessed by the CS.
> > (t+1) CS submission setting BOOKKEEP fences (i.e. no implicit sync)
> >
> > to run concurrently. When you write
> >
> >> Only when all this is done we then can resolve the dependency that the
> >> CS currently must wait for any clear operation on the VM.
> > isn't that all about the second problem?
>  No, it's the same.
> 
>  See what we do in the VM code is to artificially insert a bubble so that
>  all VM clear operations wait for all CS operations and then use the
>  clear fence to indicate when the backing store of the BO can be freed.
> >>> Isn't that remediated with something like the code below? At least the
> >>> gem_close case should be handled with this, and the move case was
> >>> already handled by the copy operation.
> >> That is one necessary puzzle piece, yes. But you need more than that.
> >>
> >> Especially the explicit unmap operation needs to be converted into an
> >> implicit unmap to get the TLB flush right.
> > This doesn't change anything about the TLB flush though? Since all
> > unmap -> later jobs dependencies are still implicit.
> >
> > So the worst what could happen (i.f. e.g. userspace gets the
> > waits/dependencies wrong) is
> >
> > 1) non-implicit CS gets submitted that touches a BO
> > 2)  VM unmap on that BO happens
> > 2.5) the CS from 1 is still active due to missing dependencies
> > 2.6) but any CS submission after 2 will trigger a TLB flush
>
> Yeah, but that's exactly the bubble we try to avoid. Isn't it?

For this series, not really.  To clarify there are two sides for
getting GPU bubbles and no overlap:

(1) VM operations implicitly wait for earlier CS submissions
(2) CS submissions implicitly wait for earlier VM operations

Together, these combine to ensure that you get a (potentially small)
bubble any time VM work happens.

Your series (and further ideas) tackles (2), and is a worthwhile thing
to do. However, while writing the userspace for this I noticed this
isn't enough to get rid of all our GPU bubbles. In particular when
doing a non-sparse map of a new BO, that tends to need to be waited on
for the next CS anyway for API semantics. Due to VM operations
happening on a single timeline that means this high priority map can
end up being blocked by earlier sparse maps and hence the bubble in
that case still exists.

So in this series I try to tackle (1) instead. Since GPU work
typically lags behind CPU submissions and VM operations aren't that
slow, we can typically execute VM operations early enough that any
implicit syncs from (2) are less/no issue. In particular, by doing all
dependency waits in userspace, we can make almost all VM operations
start pretty much immediately (with a bunch of exceptions, like other
VM work that takes time, radeonsi still submitting implicitly synced
stuff etc.).

So I think (2) is valuable, just not what this series tries to focus
on or touch at all.

(And then the cherry on top would be having two timelines for VM
operations, a high priority one for non-sparse bindings and a low
priority one for sparse bindings, but that is very complex and not
super high value on top of eliminating (1) + (2), so I'd punt that for
"maybe later". See e.g. the discussion wrt Intel at
https://patchwork.freedesktop.org/patch/486604/#comment_879193)

>
> When we want to do a

Re: [PATCH 2/6] drm/vmwgfx: Create mouse hotspot properties on cursor planes

2022-06-03 Thread Martin Krastev (VMware)


From: Martin Krastev 

On 2.06.22 г. 18:42 ч., Zack Rusin wrote:

From: Zack Rusin 

Atomic modesetting got support for mouse hotspots via the hotspot
properties. Drivers need to create those properties on cursor planes
which require the mouse hotspot coordinates.

Add the code creating hotspot properties and port away from old legacy
hotspot API. The legacy hotspot paths have an implementation that works
with new atomic properties so there's no reason to keep them and it
makes sense to unify both paths.

Signed-off-by: Zack Rusin 
Cc: Martin Krastev 
Cc: Maaz Mombasawala 
---
  drivers/gpu/drm/vmwgfx/vmwgfx_kms.c  | 11 ++-
  drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c  |  2 ++
  drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c |  1 +
  drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c |  2 ++
  4 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c 
b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
index 693028c31b6b..a4cd312fee46 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
@@ -652,13 +652,8 @@ vmw_du_cursor_plane_atomic_update(struct drm_plane *plane,
struct vmw_plane_state *vps = vmw_plane_state_to_vps(new_state);
s32 hotspot_x, hotspot_y;
  
-	hotspot_x = du->hotspot_x;

-   hotspot_y = du->hotspot_y;
-
-   if (new_state->fb) {
-   hotspot_x += new_state->fb->hot_x;
-   hotspot_y += new_state->fb->hot_y;
-   }
+   hotspot_x = du->hotspot_x + new_state->hotspot_x;
+   hotspot_y = du->hotspot_y + new_state->hotspot_y;
  
  	du->cursor_surface = vps->surf;

du->cursor_bo = vps->bo;
@@ -2270,8 +2265,6 @@ int vmw_du_crtc_gamma_set(struct drm_crtc *crtc,
int i;
  
  	for (i = 0; i < size; i++) {

-   DRM_DEBUG("%d r/g/b = 0x%04x / 0x%04x / 0x%04x\n", i,
- r[i], g[i], b[i]);
vmw_write(dev_priv, SVGA_PALETTE_BASE + i * 3 + 0, r[i] >> 8);
vmw_write(dev_priv, SVGA_PALETTE_BASE + i * 3 + 1, g[i] >> 8);
vmw_write(dev_priv, SVGA_PALETTE_BASE + i * 3 + 2, b[i] >> 8);
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c 
b/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c
index e4347faccee0..43e89c6755b2 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c
@@ -474,6 +474,8 @@ static int vmw_ldu_init(struct vmw_private *dev_priv, 
unsigned unit)
(>base,
 dev_priv->implicit_placement_property,
 1);
+   if (vmw_cmd_supported(dev_priv))
+   drm_plane_create_hotspot_properties(>base);
  
  	return 0;
  
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c b/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c

index c89ad3a2d141..8d46b0cbe640 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
@@ -932,6 +932,7 @@ static int vmw_sou_init(struct vmw_private *dev_priv, 
unsigned unit)
   dev->mode_config.suggested_x_property, 0);
drm_object_attach_property(>base,
   dev->mode_config.suggested_y_property, 0);
+   drm_plane_create_hotspot_properties(>base);
return 0;
  
  err_free_unregister:

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c 
b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
index eb014b97d156..d940b9a525e7 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
@@ -1822,6 +1822,8 @@ static int vmw_stdu_init(struct vmw_private *dev_priv, 
unsigned unit)
   dev->mode_config.suggested_x_property, 0);
drm_object_attach_property(>base,
   dev->mode_config.suggested_y_property, 0);
+   drm_plane_create_hotspot_properties(>base);
+
return 0;
  
  err_free_unregister:



LGTM!

Reviewed-by: Martin Krastev 


Regards,

Martin

Re: [RFC PATCH 3/5] drm/amdgpu: Allow explicit sync for VM ops.


Am 03.06.22 um 14:39 schrieb Bas Nieuwenhuizen:

On Fri, Jun 3, 2022 at 2:08 PM Christian König  wrote:

Am 03.06.22 um 13:07 schrieb Bas Nieuwenhuizen:

On Fri, Jun 3, 2022 at 12:16 PM Christian König
 wrote:

Am 03.06.22 um 12:08 schrieb Bas Nieuwenhuizen:

[SNIP]

I do have to fix some stuff indeed, especially for the GEM close but
with that we should be able to keep the same basic approach?

Nope, not even remotely.

What we need is the following:
1. Rolling out my drm_exec patch set, so that we can lock buffers as needed.
2. When we get a VM operation we not only lock the VM page tables, but
also all buffers we potentially need to unmap.
3. Nuking the freed list in the amdgpu_vm structure by updating freed
areas directly when they are unmapped.
4. Tracking those updates inside the bo_va structure for the BO+VM
combination.
5. When the bo_va structure is destroy because of closing the handle
move the last clear operation over to the VM as implicit sync.


Hi Christian, isn't that a different problem though (that we're also
trying to solve, but in your series)?

What this patch tries to achieve:

(t+0) CS submission setting BOOKKEEP fences (i.e. no implicit sync)
(t+1) a VM operation on a BO/VM accessed by the CS.

to run concurrently. What it *doesn't* try is

(t+0) a VM operation on a BO/VM accessed by the CS.
(t+1) CS submission setting BOOKKEEP fences (i.e. no implicit sync)

to run concurrently. When you write


Only when all this is done we then can resolve the dependency that the
CS currently must wait for any clear operation on the VM.

isn't that all about the second problem?

No, it's the same.

See what we do in the VM code is to artificially insert a bubble so that
all VM clear operations wait for all CS operations and then use the
clear fence to indicate when the backing store of the BO can be freed.

Isn't that remediated with something like the code below? At least the
gem_close case should be handled with this, and the move case was
already handled by the copy operation.

That is one necessary puzzle piece, yes. But you need more than that.

Especially the explicit unmap operation needs to be converted into an
implicit unmap to get the TLB flush right.

This doesn't change anything about the TLB flush though? Since all
unmap -> later jobs dependencies are still implicit.

So the worst what could happen (i.f. e.g. userspace gets the
waits/dependencies wrong) is

1) non-implicit CS gets submitted that touches a BO
2)  VM unmap on that BO happens
2.5) the CS from 1 is still active due to missing dependencies
2.6) but any CS submission after 2 will trigger a TLB flush


Yeah, but that's exactly the bubble we try to avoid. Isn't it?

When we want to do a TLB flush the unmap operation must already be 
completed. Otherwise the flush is rather pointless since any access 
could reloads the not yet updated PTEs.


And this means that we need to artificially add a dependency on every 
command submission after 2 to wait until the unmap operation is completed.


Christian.


3) A TLB flush happens for a new CS
4) All CS submissions here see the TLB flush and hence the unmap

So the main problem would be the CS from step 1, but (a) if that
VMFaults that is the apps own fault and (b) because we don't free the
memory until (1) finishes it is not a security issue kernel-wise.


I think I know all the necessary steps now, it's just tons of work to do.

Regards,
Christian.



--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
@@ -187,6 +187,39 @@ static int amdgpu_gem_object_open(struct
drm_gem_object *obj,
 return 0;
}

+static void dma_resv_copy(struct dma_resv *src, struct dma_resv *dst)
+{
+   struct dma_resv_iter cursor;
+   struct dma_fence *f;
+   int r;
+   unsigned num_fences = 0;
+
+   if (src == dst)
+   return;
+
+   /* We assume the later loops get the same fences as the caller should
+* lock the resv. */
+   dma_resv_for_each_fence(, src, DMA_RESV_USAGE_BOOKKEEP, f) {
+   ++num_fences;
+   dma_fence_put(f);
+   }
+
+   r = dma_resv_reserve_fences(dst, num_fences);
+   if (r) {
+   /* As last resort on OOM we block for the fence */
+   dma_resv_for_each_fence(, src,
DMA_RESV_USAGE_BOOKKEEP, f) {
+   dma_fence_wait(f, false);
+   dma_fence_put(f);
+   }
+   }
+
+   dma_resv_for_each_fence(, src, DMA_RESV_USAGE_BOOKKEEP, f) {
+   dma_resv_add_fence(dst, f, dma_resv_iter_usage());
+   dma_fence_put(f);
+   }
+}
+
+
static void amdgpu_gem_object_close(struct drm_gem_object *obj,
 struct drm_file *file_priv)
{
@@ -233,6 +266,8 @@ static void amdgpu_gem_object_close(struct
drm_gem_object *obj,
 amdgpu_bo_fence(bo, fence, true);
 dma_fence_put(fence);

+   dma_resv_copy(vm->root.bo->tbo.base.resv,

Re: [RFC PATCH 3/5] drm/amdgpu: Allow explicit sync for VM ops.

On Fri, Jun 3, 2022 at 2:08 PM Christian König  wrote:
>
> Am 03.06.22 um 13:07 schrieb Bas Nieuwenhuizen:
> > On Fri, Jun 3, 2022 at 12:16 PM Christian König
> >  wrote:
> >> Am 03.06.22 um 12:08 schrieb Bas Nieuwenhuizen:
> >>> [SNIP]
> > I do have to fix some stuff indeed, especially for the GEM close but
> > with that we should be able to keep the same basic approach?
>  Nope, not even remotely.
> 
>  What we need is the following:
>  1. Rolling out my drm_exec patch set, so that we can lock buffers as 
>  needed.
>  2. When we get a VM operation we not only lock the VM page tables, but
>  also all buffers we potentially need to unmap.
>  3. Nuking the freed list in the amdgpu_vm structure by updating freed
>  areas directly when they are unmapped.
>  4. Tracking those updates inside the bo_va structure for the BO+VM
>  combination.
>  5. When the bo_va structure is destroy because of closing the handle
>  move the last clear operation over to the VM as implicit sync.
> 
> >>> Hi Christian, isn't that a different problem though (that we're also
> >>> trying to solve, but in your series)?
> >>>
> >>> What this patch tries to achieve:
> >>>
> >>> (t+0) CS submission setting BOOKKEEP fences (i.e. no implicit sync)
> >>> (t+1) a VM operation on a BO/VM accessed by the CS.
> >>>
> >>> to run concurrently. What it *doesn't* try is
> >>>
> >>> (t+0) a VM operation on a BO/VM accessed by the CS.
> >>> (t+1) CS submission setting BOOKKEEP fences (i.e. no implicit sync)
> >>>
> >>> to run concurrently. When you write
> >>>
>  Only when all this is done we then can resolve the dependency that the
>  CS currently must wait for any clear operation on the VM.
> >>> isn't that all about the second problem?
> >> No, it's the same.
> >>
> >> See what we do in the VM code is to artificially insert a bubble so that
> >> all VM clear operations wait for all CS operations and then use the
> >> clear fence to indicate when the backing store of the BO can be freed.
> > Isn't that remediated with something like the code below? At least the
> > gem_close case should be handled with this, and the move case was
> > already handled by the copy operation.
>
> That is one necessary puzzle piece, yes. But you need more than that.
>
> Especially the explicit unmap operation needs to be converted into an
> implicit unmap to get the TLB flush right.

This doesn't change anything about the TLB flush though? Since all
unmap -> later jobs dependencies are still implicit.

So the worst what could happen (i.f. e.g. userspace gets the
waits/dependencies wrong) is

1) non-implicit CS gets submitted that touches a BO
2)  VM unmap on that BO happens
2.5) the CS from 1 is still active due to missing dependencies
2.6) but any CS submission after 2 will trigger a TLB flush
3) A TLB flush happens for a new CS
4) All CS submissions here see the TLB flush and hence the unmap

So the main problem would be the CS from step 1, but (a) if that
VMFaults that is the apps own fault and (b) because we don't free the
memory until (1) finishes it is not a security issue kernel-wise.

>
> I think I know all the necessary steps now, it's just tons of work to do.
>
> Regards,
> Christian.
>
> >
> >
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> > @@ -187,6 +187,39 @@ static int amdgpu_gem_object_open(struct
> > drm_gem_object *obj,
> > return 0;
> > }
> >
> > +static void dma_resv_copy(struct dma_resv *src, struct dma_resv *dst)
> > +{
> > +   struct dma_resv_iter cursor;
> > +   struct dma_fence *f;
> > +   int r;
> > +   unsigned num_fences = 0;
> > +
> > +   if (src == dst)
> > +   return;
> > +
> > +   /* We assume the later loops get the same fences as the caller 
> > should
> > +* lock the resv. */
> > +   dma_resv_for_each_fence(, src, DMA_RESV_USAGE_BOOKKEEP, f) {
> > +   ++num_fences;
> > +   dma_fence_put(f);
> > +   }
> > +
> > +   r = dma_resv_reserve_fences(dst, num_fences);
> > +   if (r) {
> > +   /* As last resort on OOM we block for the fence */
> > +   dma_resv_for_each_fence(, src,
> > DMA_RESV_USAGE_BOOKKEEP, f) {
> > +   dma_fence_wait(f, false);
> > +   dma_fence_put(f);
> > +   }
> > +   }
> > +
> > +   dma_resv_for_each_fence(, src, DMA_RESV_USAGE_BOOKKEEP, f) {
> > +   dma_resv_add_fence(dst, f, dma_resv_iter_usage());
> > +   dma_fence_put(f);
> > +   }
> > +}
> > +
> > +
> > static void amdgpu_gem_object_close(struct drm_gem_object *obj,
> > struct drm_file *file_priv)
> > {
> > @@ -233,6 +266,8 @@ static void amdgpu_gem_object_close(struct
> > drm_gem_object *obj,
> > amdgpu_bo_fence(bo, fence, true);
> > dma_fence_put(fence);
> >
> > +

[PATCH] drm/etnaviv: print offender task information on hangcheck recovery

2022-06-03 Thread Christian Gmeiner

Track the pid per submit, so we can print the name and cmdline of
the task which submitted the batch that caused the gpu to hang.

Signed-off-by: Christian Gmeiner 
---
 drivers/gpu/drm/etnaviv/etnaviv_gem.h|  1 +
 drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c |  6 ++
 drivers/gpu/drm/etnaviv/etnaviv_gpu.c| 18 +-
 drivers/gpu/drm/etnaviv/etnaviv_gpu.h|  2 +-
 drivers/gpu/drm/etnaviv/etnaviv_sched.c  |  2 +-
 5 files changed, 26 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.h 
b/drivers/gpu/drm/etnaviv/etnaviv_gem.h
index 63688e6e4580..baa81cbf701a 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gem.h
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.h
@@ -96,6 +96,7 @@ struct etnaviv_gem_submit {
int out_fence_id;
struct list_head node; /* GPU active submit list */
struct etnaviv_cmdbuf cmdbuf;
+   struct pid *pid;   /* submitting process */
bool runtime_resumed;
u32 exec_state;
u32 flags;
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c 
b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
index 1ac916b24891..1491159d0d20 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
@@ -399,6 +399,9 @@ static void submit_cleanup(struct kref *kref)
mutex_unlock(>gpu->fence_lock);
dma_fence_put(submit->out_fence);
}
+
+   put_pid(submit->pid);
+
kfree(submit->pmrs);
kfree(submit);
 }
@@ -422,6 +425,7 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void 
*data,
struct sync_file *sync_file = NULL;
struct ww_acquire_ctx ticket;
int out_fence_fd = -1;
+   struct pid *pid = get_pid(task_pid(current));
void *stream;
int ret;
 
@@ -519,6 +523,8 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void 
*data,
goto err_submit_ww_acquire;
}
 
+   submit->pid = pid;
+
ret = etnaviv_cmdbuf_init(priv->cmdbuf_suballoc, >cmdbuf,
  ALIGN(args->stream_size, 8) + 8);
if (ret)
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c 
b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
index 37018bc55810..7d9bf4673e2d 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
@@ -1045,12 +1045,28 @@ int etnaviv_gpu_debugfs(struct etnaviv_gpu *gpu, struct 
seq_file *m)
 }
 #endif
 
-void etnaviv_gpu_recover_hang(struct etnaviv_gpu *gpu)
+void etnaviv_gpu_recover_hang(struct etnaviv_gem_submit *submit)
 {
+   struct etnaviv_gpu *gpu = submit->gpu;
+   char *comm = NULL, *cmd = NULL;
+   struct task_struct *task;
unsigned int i;
 
dev_err(gpu->dev, "recover hung GPU!\n");
 
+   task = get_pid_task(submit->pid, PIDTYPE_PID);
+   if (task) {
+   comm = kstrdup(task->comm, GFP_KERNEL);
+   cmd = kstrdup_quotable_cmdline(task, GFP_KERNEL);
+   put_task_struct(task);
+   }
+
+   if (comm && cmd)
+   dev_err(gpu->dev, "offending task: %s (%s)\n", comm, cmd);
+
+   kfree(cmd);
+   kfree(comm);
+
if (pm_runtime_get_sync(gpu->dev) < 0)
goto pm_put;
 
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.h 
b/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
index 85eddd492774..b3a0941d56fd 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
@@ -168,7 +168,7 @@ bool etnaviv_fill_identity_from_hwdb(struct etnaviv_gpu 
*gpu);
 int etnaviv_gpu_debugfs(struct etnaviv_gpu *gpu, struct seq_file *m);
 #endif
 
-void etnaviv_gpu_recover_hang(struct etnaviv_gpu *gpu);
+void etnaviv_gpu_recover_hang(struct etnaviv_gem_submit *submit);
 void etnaviv_gpu_retire(struct etnaviv_gpu *gpu);
 int etnaviv_gpu_wait_fence_interruptible(struct etnaviv_gpu *gpu,
u32 fence, struct drm_etnaviv_timespec *timeout);
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_sched.c 
b/drivers/gpu/drm/etnaviv/etnaviv_sched.c
index 72e2553fbc98..d29f467eee13 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_sched.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_sched.c
@@ -67,7 +67,7 @@ static enum drm_gpu_sched_stat 
etnaviv_sched_timedout_job(struct drm_sched_job
 
/* get the GPU back into the init state */
etnaviv_core_dump(submit);
-   etnaviv_gpu_recover_hang(gpu);
+   etnaviv_gpu_recover_hang(submit);
 
drm_sched_resubmit_jobs(>sched);
 
-- 
2.36.1

Re: [RFC PATCH 3/5] drm/amdgpu: Allow explicit sync for VM ops.


Am 03.06.22 um 13:07 schrieb Bas Nieuwenhuizen:

On Fri, Jun 3, 2022 at 12:16 PM Christian König
 wrote:

Am 03.06.22 um 12:08 schrieb Bas Nieuwenhuizen:

[SNIP]

I do have to fix some stuff indeed, especially for the GEM close but
with that we should be able to keep the same basic approach?

Nope, not even remotely.

What we need is the following:
1. Rolling out my drm_exec patch set, so that we can lock buffers as needed.
2. When we get a VM operation we not only lock the VM page tables, but
also all buffers we potentially need to unmap.
3. Nuking the freed list in the amdgpu_vm structure by updating freed
areas directly when they are unmapped.
4. Tracking those updates inside the bo_va structure for the BO+VM
combination.
5. When the bo_va structure is destroy because of closing the handle
move the last clear operation over to the VM as implicit sync.


Hi Christian, isn't that a different problem though (that we're also
trying to solve, but in your series)?

What this patch tries to achieve:

(t+0) CS submission setting BOOKKEEP fences (i.e. no implicit sync)
(t+1) a VM operation on a BO/VM accessed by the CS.

to run concurrently. What it *doesn't* try is

(t+0) a VM operation on a BO/VM accessed by the CS.
(t+1) CS submission setting BOOKKEEP fences (i.e. no implicit sync)

to run concurrently. When you write


Only when all this is done we then can resolve the dependency that the
CS currently must wait for any clear operation on the VM.

isn't that all about the second problem?

No, it's the same.

See what we do in the VM code is to artificially insert a bubble so that
all VM clear operations wait for all CS operations and then use the
clear fence to indicate when the backing store of the BO can be freed.

Isn't that remediated with something like the code below? At least the
gem_close case should be handled with this, and the move case was
already handled by the copy operation.


That is one necessary puzzle piece, yes. But you need more than that.

Especially the explicit unmap operation needs to be converted into an 
implicit unmap to get the TLB flush right.


I think I know all the necessary steps now, it's just tons of work to do.

Regards,
Christian.




--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
@@ -187,6 +187,39 @@ static int amdgpu_gem_object_open(struct
drm_gem_object *obj,
return 0;
}

+static void dma_resv_copy(struct dma_resv *src, struct dma_resv *dst)
+{
+   struct dma_resv_iter cursor;
+   struct dma_fence *f;
+   int r;
+   unsigned num_fences = 0;
+
+   if (src == dst)
+   return;
+
+   /* We assume the later loops get the same fences as the caller should
+* lock the resv. */
+   dma_resv_for_each_fence(, src, DMA_RESV_USAGE_BOOKKEEP, f) {
+   ++num_fences;
+   dma_fence_put(f);
+   }
+
+   r = dma_resv_reserve_fences(dst, num_fences);
+   if (r) {
+   /* As last resort on OOM we block for the fence */
+   dma_resv_for_each_fence(, src,
DMA_RESV_USAGE_BOOKKEEP, f) {
+   dma_fence_wait(f, false);
+   dma_fence_put(f);
+   }
+   }
+
+   dma_resv_for_each_fence(, src, DMA_RESV_USAGE_BOOKKEEP, f) {
+   dma_resv_add_fence(dst, f, dma_resv_iter_usage());
+   dma_fence_put(f);
+   }
+}
+
+
static void amdgpu_gem_object_close(struct drm_gem_object *obj,
struct drm_file *file_priv)
{
@@ -233,6 +266,8 @@ static void amdgpu_gem_object_close(struct
drm_gem_object *obj,
amdgpu_bo_fence(bo, fence, true);
dma_fence_put(fence);

+   dma_resv_copy(vm->root.bo->tbo.base.resv, bo->tbo.base.resv);
+
out_unlock:
if (unlikely(r < 0))
dev_err(adev->dev, "failed to clear page "


When you want to remove this bubble (which is certainly a good idea) you
need to first come up with a different approach to handle the clear
operations.

Regards,
Christian.




Regards,
Christian.

Re: [PATCH v6 2/6] drm/v3d: Get rid of pm code

2022-06-03 Thread Melissa Wen

On 06/03, Peter Robinson wrote:
> Runtime PM doesn't seem to work correctly on this driver. On top of
> that, commit 8b6864e3e138 ("drm/v3d/v3d_drv: Remove unused static
> variable 'v3d_v3d_pm_ops'") hints that it most likely never did as the
> driver's PM ops were not hooked-up.
> 
> So, in order to support regular operation with V3D on BCM2711 (Raspberry
> Pi 4), get rid of the PM code. PM will be reinstated once we figure out
> the underlying issues.

I've spent some time trying to get PM working properly on RPi4, but I
haven´t gotten good results yet, and all attempts ended up disabling the
auto suspend option somehow. Keeping in mind how long this has been
unresolved, I agree to clean it now as we continue to investigate the
issue.

Acked-by: Melissa Wen 
> 
> Signed-off-by: Nicolas Saenz Julienne 
> Signed-off-by: Peter Robinson 
> Reviewed-by: Javier Martinez Canillas 
> ---
> Changes since v4:
> - Rebase
> 
> Changes since v3:
> - Minor updates for rebase
> 
>  drivers/gpu/drm/v3d/v3d_debugfs.c | 18 +-
>  drivers/gpu/drm/v3d/v3d_drv.c | 11 ---
>  drivers/gpu/drm/v3d/v3d_gem.c | 12 +---
>  3 files changed, 2 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/gpu/drm/v3d/v3d_debugfs.c 
> b/drivers/gpu/drm/v3d/v3d_debugfs.c
> index 29fd13109e43..efbde124c296 100644
> --- a/drivers/gpu/drm/v3d/v3d_debugfs.c
> +++ b/drivers/gpu/drm/v3d/v3d_debugfs.c
> @@ -4,7 +4,6 @@
>  #include 
>  #include 
>  #include 
> -#include 
>  #include 
>  #include 
>  
> @@ -131,11 +130,7 @@ static int v3d_v3d_debugfs_ident(struct seq_file *m, 
> void *unused)
>   struct drm_device *dev = node->minor->dev;
>   struct v3d_dev *v3d = to_v3d_dev(dev);
>   u32 ident0, ident1, ident2, ident3, cores;
> - int ret, core;
> -
> - ret = pm_runtime_get_sync(v3d->drm.dev);
> - if (ret < 0)
> - return ret;
> + int core;
>  
>   ident0 = V3D_READ(V3D_HUB_IDENT0);
>   ident1 = V3D_READ(V3D_HUB_IDENT1);
> @@ -188,9 +183,6 @@ static int v3d_v3d_debugfs_ident(struct seq_file *m, void 
> *unused)
>  (misccfg & V3D_MISCCFG_OVRTMUOUT) != 0);
>   }
>  
> - pm_runtime_mark_last_busy(v3d->drm.dev);
> - pm_runtime_put_autosuspend(v3d->drm.dev);
> -
>   return 0;
>  }
>  
> @@ -218,11 +210,6 @@ static int v3d_measure_clock(struct seq_file *m, void 
> *unused)
>   uint32_t cycles;
>   int core = 0;
>   int measure_ms = 1000;
> - int ret;
> -
> - ret = pm_runtime_get_sync(v3d->drm.dev);
> - if (ret < 0)
> - return ret;
>  
>   if (v3d->ver >= 40) {
>   V3D_CORE_WRITE(core, V3D_V4_PCTR_0_SRC_0_3,
> @@ -246,9 +233,6 @@ static int v3d_measure_clock(struct seq_file *m, void 
> *unused)
>  cycles / (measure_ms * 1000),
>  (cycles / (measure_ms * 100)) % 10);
>  
> - pm_runtime_mark_last_busy(v3d->drm.dev);
> - pm_runtime_put_autosuspend(v3d->drm.dev);
> -
>   return 0;
>  }
>  
> diff --git a/drivers/gpu/drm/v3d/v3d_drv.c b/drivers/gpu/drm/v3d/v3d_drv.c
> index 1afcd54fbbd5..56d5f831e48b 100644
> --- a/drivers/gpu/drm/v3d/v3d_drv.c
> +++ b/drivers/gpu/drm/v3d/v3d_drv.c
> @@ -19,7 +19,6 @@
>  #include 
>  #include 
>  #include 
> -#include 
>  #include 
>  
>  #include 
> @@ -43,7 +42,6 @@ static int v3d_get_param_ioctl(struct drm_device *dev, void 
> *data,
>  {
>   struct v3d_dev *v3d = to_v3d_dev(dev);
>   struct drm_v3d_get_param *args = data;
> - int ret;
>   static const u32 reg_map[] = {
>   [DRM_V3D_PARAM_V3D_UIFCFG] = V3D_HUB_UIFCFG,
>   [DRM_V3D_PARAM_V3D_HUB_IDENT1] = V3D_HUB_IDENT1,
> @@ -69,17 +67,12 @@ static int v3d_get_param_ioctl(struct drm_device *dev, 
> void *data,
>   if (args->value != 0)
>   return -EINVAL;
>  
> - ret = pm_runtime_get_sync(v3d->drm.dev);
> - if (ret < 0)
> - return ret;
>   if (args->param >= DRM_V3D_PARAM_V3D_CORE0_IDENT0 &&
>   args->param <= DRM_V3D_PARAM_V3D_CORE0_IDENT2) {
>   args->value = V3D_CORE_READ(0, offset);
>   } else {
>   args->value = V3D_READ(offset);
>   }
> - pm_runtime_mark_last_busy(v3d->drm.dev);
> - pm_runtime_put_autosuspend(v3d->drm.dev);
>   return 0;
>   }
>  
> @@ -280,10 +273,6 @@ static int v3d_platform_drm_probe(struct platform_device 
> *pdev)
>   return -ENOMEM;
>   }
>  
> - pm_runtime_use_autosuspend(dev);
> - pm_runtime_set_autosuspend_delay(dev, 50);
> - pm_runtime_enable(dev);
> -
>   ret = v3d_gem_init(drm);
>   if (ret)
>   goto dma_free;
> diff --git a/drivers/gpu/drm/v3d/v3d_gem.c b/drivers/gpu/drm/v3d/v3d_gem.c
> index 92bc0faee84f..7026214a09f0 100644
> --- a/drivers/gpu/drm/v3d/v3d_gem.c
> +++ b/drivers/gpu/drm/v3d/v3d_gem.c
> @@ -6,7 +6,6 @@
>  #include

Re: [RFC PATCH 3/5] drm/amdgpu: Allow explicit sync for VM ops.

On Fri, Jun 3, 2022 at 12:16 PM Christian König
 wrote:
>
> Am 03.06.22 um 12:08 schrieb Bas Nieuwenhuizen:
> > [SNIP]
> >>> I do have to fix some stuff indeed, especially for the GEM close but
> >>> with that we should be able to keep the same basic approach?
> >> Nope, not even remotely.
> >>
> >> What we need is the following:
> >> 1. Rolling out my drm_exec patch set, so that we can lock buffers as 
> >> needed.
> >> 2. When we get a VM operation we not only lock the VM page tables, but
> >> also all buffers we potentially need to unmap.
> >> 3. Nuking the freed list in the amdgpu_vm structure by updating freed
> >> areas directly when they are unmapped.
> >> 4. Tracking those updates inside the bo_va structure for the BO+VM
> >> combination.
> >> 5. When the bo_va structure is destroy because of closing the handle
> >> move the last clear operation over to the VM as implicit sync.
> >>
> > Hi Christian, isn't that a different problem though (that we're also
> > trying to solve, but in your series)?
> >
> > What this patch tries to achieve:
> >
> > (t+0) CS submission setting BOOKKEEP fences (i.e. no implicit sync)
> > (t+1) a VM operation on a BO/VM accessed by the CS.
> >
> > to run concurrently. What it *doesn't* try is
> >
> > (t+0) a VM operation on a BO/VM accessed by the CS.
> > (t+1) CS submission setting BOOKKEEP fences (i.e. no implicit sync)
> >
> > to run concurrently. When you write
> >
> >> Only when all this is done we then can resolve the dependency that the
> >> CS currently must wait for any clear operation on the VM.
> > isn't that all about the second problem?
>
> No, it's the same.
>
> See what we do in the VM code is to artificially insert a bubble so that
> all VM clear operations wait for all CS operations and then use the
> clear fence to indicate when the backing store of the BO can be freed.

Isn't that remediated with something like the code below? At least the
gem_close case should be handled with this, and the move case was
already handled by the copy operation.


--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
@@ -187,6 +187,39 @@ static int amdgpu_gem_object_open(struct
drm_gem_object *obj,
   return 0;
}

+static void dma_resv_copy(struct dma_resv *src, struct dma_resv *dst)
+{
+   struct dma_resv_iter cursor;
+   struct dma_fence *f;
+   int r;
+   unsigned num_fences = 0;
+
+   if (src == dst)
+   return;
+
+   /* We assume the later loops get the same fences as the caller should
+* lock the resv. */
+   dma_resv_for_each_fence(, src, DMA_RESV_USAGE_BOOKKEEP, f) {
+   ++num_fences;
+   dma_fence_put(f);
+   }
+
+   r = dma_resv_reserve_fences(dst, num_fences);
+   if (r) {
+   /* As last resort on OOM we block for the fence */
+   dma_resv_for_each_fence(, src,
DMA_RESV_USAGE_BOOKKEEP, f) {
+   dma_fence_wait(f, false);
+   dma_fence_put(f);
+   }
+   }
+
+   dma_resv_for_each_fence(, src, DMA_RESV_USAGE_BOOKKEEP, f) {
+   dma_resv_add_fence(dst, f, dma_resv_iter_usage());
+   dma_fence_put(f);
+   }
+}
+
+
static void amdgpu_gem_object_close(struct drm_gem_object *obj,
   struct drm_file *file_priv)
{
@@ -233,6 +266,8 @@ static void amdgpu_gem_object_close(struct
drm_gem_object *obj,
   amdgpu_bo_fence(bo, fence, true);
   dma_fence_put(fence);

+   dma_resv_copy(vm->root.bo->tbo.base.resv, bo->tbo.base.resv);
+
out_unlock:
   if (unlikely(r < 0))
   dev_err(adev->dev, "failed to clear page "

>
> When you want to remove this bubble (which is certainly a good idea) you
> need to first come up with a different approach to handle the clear
> operations.
>
> Regards,
> Christian.
>
> >
> >
> >> Regards,
> >> Christian.
> >>
> >>
>

RE: [v1] drm/msm: add null checks for drm device to avoid crash during probe defer



> -Original Message-
> From: Dmitry Baryshkov 
> Sent: Friday, June 3, 2022 3:07 PM
> To: Vinod Polimera (QUIC) ; dri-
> de...@lists.freedesktop.org; linux-arm-...@vger.kernel.org;
> freedr...@lists.freedesktop.org; devicet...@vger.kernel.org
> Cc: linux-ker...@vger.kernel.org; robdcl...@gmail.com;
> diand...@chromium.org; vpoli...@quicinc.com; swb...@chromium.org;
> kaly...@quicinc.com
> Subject: Re: [v1] drm/msm: add null checks for drm device to avoid crash
> during probe defer
> 
> WARNING: This email originated from outside of Qualcomm. Please be wary
> of any links or attachments, and do not enable macros.
> 
> On 03/06/2022 12:22, Vinod Polimera wrote:
> > During probe defer, drm device is not initialized and an external
> > trigger to shutdown is trying to clean up drm device leading to crash.
> > Add checks to avoid drm device cleanup in such cases.
> >
> > BUG: unable to handle kernel NULL pointer dereference at virtual
> > address 00b8
> >
> > Call trace:
> >
> > drm_atomic_helper_shutdown+0x44/0x144
> > msm_pdev_shutdown+0x2c/0x38
> > platform_shutdown+0x2c/0x38
> > device_shutdown+0x158/0x210
> > kernel_restart_prepare+0x40/0x4c
> > kernel_restart+0x20/0x6c
> > __arm64_sys_reboot+0x194/0x23c
> > invoke_syscall+0x50/0x13c
> > el0_svc_common+0xa0/0x17c
> > do_el0_svc_compat+0x28/0x34
> > el0_svc_compat+0x20/0x70
> > el0t_32_sync_handler+0xa8/0xcc
> > el0t_32_sync+0x1a8/0x1ac
> >
> > Signed-off-by: Vinod Polimera 
> 
> Fixes ?
- Added fixes tag in v2.
> 
> > ---
> >   drivers/gpu/drm/msm/msm_drv.c | 6 +-
> >   1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/msm/msm_drv.c
> b/drivers/gpu/drm/msm/msm_drv.c
> > index 4448536..d62ac66 100644
> > --- a/drivers/gpu/drm/msm/msm_drv.c
> > +++ b/drivers/gpu/drm/msm/msm_drv.c
> > @@ -142,6 +142,9 @@ static void msm_irq_uninstall(struct drm_device
> *dev)
> >   struct msm_drm_private *priv = dev->dev_private;
> >   struct msm_kms *kms = priv->kms;
> >
> > + if (!irq_has_action(kms->irq))
> > + return;
> > +
> 
> Is this part required with
> https://patchwork.freedesktop.org/patch/485422/?series=103702=1?
Yes, I feel like this is a better approach than maintaining a new variable. I 
see a couple of drivers following similar approach to safeguard uninstall 
without being install called.
> 
> >   kms->funcs->irq_uninstall(kms);
> >   if (kms->irq_requested)
> >   free_irq(kms->irq, dev);
> > @@ -259,6 +262,7 @@ static int msm_drm_uninit(struct device *dev)
> >
> >   ddev->dev_private = NULL;
> >   drm_dev_put(ddev);
> > + priv->dev = NULL;
> 
> What are you trying to protect here?
If we get a shutdown call after probe defer, there can be stale pointer in 
priv->dev which is invalid that needs to be cleared.
> 
> >
> >   destroy_workqueue(priv->wq);
> >
> > @@ -1167,7 +1171,7 @@ void msm_drv_shutdown(struct platform_device
> *pdev)
> >   struct msm_drm_private *priv = platform_get_drvdata(pdev);
> >   struct drm_device *drm = priv ? priv->dev : NULL;
> >
> > - if (!priv || !priv->kms)
> > + if (!priv || !priv->kms || !drm)
> >   return;
> >
> >   drm_atomic_helper_shutdown(drm);
> 
> 
> --
> With best wishes
> Dmitry

[PATCH] drm/ttm: fix missing NULL check in ttm_device_swapout

Resources about to be destructed are not tied to BOs any more.

Signed-off-by: Christian König 
---
 drivers/gpu/drm/ttm/ttm_device.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/ttm/ttm_device.c b/drivers/gpu/drm/ttm/ttm_device.c
index a0562ab386f5..e7147e304637 100644
--- a/drivers/gpu/drm/ttm/ttm_device.c
+++ b/drivers/gpu/drm/ttm/ttm_device.c
@@ -156,8 +156,12 @@ int ttm_device_swapout(struct ttm_device *bdev, struct 
ttm_operation_ctx *ctx,
 
ttm_resource_manager_for_each_res(man, , res) {
struct ttm_buffer_object *bo = res->bo;
-   uint32_t num_pages = PFN_UP(bo->base.size);
+   uint32_t num_pages;
 
+   if (!bo)
+   continue;
+
+   num_pages = PFN_UP(bo->base.size);
ret = ttm_bo_swapout(bo, ctx, gfp_flags);
/* ttm_bo_swapout has dropped the lru_lock */
if (!ret)
-- 
2.25.1

Re: [PATCH 0/6] drm: Add mouse cursor hotspot support to atomic KMS

2022-06-03 Thread Gerd Hoffmann

  Hi,

> the legacy kms to atomic. This left virtualized drivers, all which
> are atomic, in a weird spot because all userspace compositors put
> those drivers on deny-lists for atomic kms due to the fact that mouse
> clicks were incorrectly routed, e.g:

> - all userspace code needs to hardcore a list of drivers which require
> hotspots because there's no way to query from drm "does this driver
> require hotspot"

So drivers move from one list to another.  Not ideal, but also not worse
than it was before, so:

Acked-by: Gerd Hoffmann 

for the qxl and virtio driver changes.

take care,
  Gerd

Re: [PATCH v3 3/4] drm/bridge: Add devm_drm_bridge_add()

On Fri, 3 Jun 2022 at 11:21, Maxime Ripard  wrote:
>
> On Tue, May 31, 2022 at 02:06:34PM -0700, Doug Anderson wrote:
> > On Mon, May 23, 2022 at 10:00 AM Doug Anderson  
> > wrote:
> > > On Sat, May 21, 2022 at 2:17 AM Maxime Ripard  wrote:
> > > > On Tue, May 10, 2022 at 12:29:43PM -0700, Douglas Anderson wrote:
> > > > > This adds a devm managed version of drm_bridge_add(). Like other
> > > > > "devm" function listed in drm_bridge.h, this function takes an
> > > > > explicit "dev" to use for the lifetime management. A few notes:
> > > > > * In general we have a "struct device" for bridges that makes a good
> > > > >   candidate for where the lifetime matches exactly what we want.
> > > > > * The "bridge->dev->dev" device appears to be the encoder
> > > > >   device. That's not the right device to use for lifetime management.
> > > > >
> > > > > Suggested-by: Dmitry Baryshkov 
> > > > > Signed-off-by: Douglas Anderson 
> > > >
> > > > If we are to introduce more managed helpers, I think it'd be wiser to
> > > > introduce them as DRM-managed, and not device managed.
> > > >
> > > > Otherwise, you'll end up in a weird state when a device has been removed
> > > > but the DRM device is still around.
> > >
> > > I'm kinda confused. In this case there is no DRM device for the bridge
> > > and, as per my CL description, "bridge-dev->dev" appears to be the
> > > encoder device. I wasn't personally involved in discussions about it,
> > > but I was under the impression that this was expected / normal. Thus
> > > we can't make this DRM-managed.
> >
> > Since I didn't hear a reply,
>
> Gah, I replied but it looks like somehow it never reached the ML...
>
> Here was my original reply:
>
> > > > This adds a devm managed version of drm_bridge_add(). Like other
> > > > "devm" function listed in drm_bridge.h, this function takes an
> > > > explicit "dev" to use for the lifetime management. A few notes:
> > > > * In general we have a "struct device" for bridges that makes a good
> > > >   candidate for where the lifetime matches exactly what we want.
> > > > * The "bridge->dev->dev" device appears to be the encoder
> > > >   device. That's not the right device to use for lifetime management.
> > > >
> > > > Suggested-by: Dmitry Baryshkov 
> > > > Signed-off-by: Douglas Anderson 
> > >
> > > If we are to introduce more managed helpers, I think it'd be wiser to
> > > introduce them as DRM-managed, and not device managed.
> > >
> > > Otherwise, you'll end up in a weird state when a device has been removed
> > > but the DRM device is still around.
> >=20
> > I'm kinda confused. In this case there is no DRM device for the bridge
> > and, as per my CL description, "bridge-dev->dev" appears to be the
> > encoder device.
>
> bridge->dev seems right though?
>
> > I wasn't personally involved in discussions about it, but I was under
> > the impression that this was expected / normal. Thus we can't make
> > this DRM-managed.
>
> Still, I don't think devm is the right solution to this either.
>
> The underlying issue is two-fold:
>
>   - Encoders can have a pointer to a bridge through of_drm_find_bridge
> or similar. However, bridges are traditionally tied to their device
> lifetime (by calling drm_bridge_add in probe, and drm_bridge_remove
> in remove). Encoders will typically be tied to the DRM device
> however, and that one sticks around until the last application
> closes it. We can thus very easily end up with a dangling pointer,
> and a use-after-free.
>
>   - It's not the case yet, but it doesn't seem far fetch to expose
> properties of bridges to the userspace. In that case, the userspace
> would be likely to still hold references to objects that aren't
> there anymore when the bridge is gone.
>
> The first is obviously a larger concern, but if we can find a solution
> that would accomodate the second it would be great.
>
> As far as I can see, we should fix in two steps:
>
>   - in drm_bridge_attach, we should add a device-managed call that will
> unregister the main DRM device. We don't allow to probe the main DRM
> device when the bridge isn't there yet in most case, so it makes
> sense to remove it once the bridge is no longer there as well.

The problem is that I do not see a good way to unregister the main DRM
device outside of it's driver code.

>
>   - When the DRM device is removed, have the core cleanup any bridge
> registered. That will remove the need to have drm_bridge_remove in
> the first place.
>
> > I'll assume that my response addressed your concerns. Assuming I get
> > reviews for the other two patches in this series I'll plan to land
> > this with Dmitry's review.
>
> I still don't think it's a good idea to merge it. It gives an illusion
> of being safe, but it's really far from it.

It is more of removing the boilerplate code spread over all the
drivers rather than about particular safety.

I'd propose to land devm_drm_bridge_add (and deprecate

Re: [RFC PATCH 3/5] drm/amdgpu: Allow explicit sync for VM ops.


Am 03.06.22 um 12:08 schrieb Bas Nieuwenhuizen:

[SNIP]

I do have to fix some stuff indeed, especially for the GEM close but
with that we should be able to keep the same basic approach?

Nope, not even remotely.

What we need is the following:
1. Rolling out my drm_exec patch set, so that we can lock buffers as needed.
2. When we get a VM operation we not only lock the VM page tables, but
also all buffers we potentially need to unmap.
3. Nuking the freed list in the amdgpu_vm structure by updating freed
areas directly when they are unmapped.
4. Tracking those updates inside the bo_va structure for the BO+VM
combination.
5. When the bo_va structure is destroy because of closing the handle
move the last clear operation over to the VM as implicit sync.


Hi Christian, isn't that a different problem though (that we're also
trying to solve, but in your series)?

What this patch tries to achieve:

(t+0) CS submission setting BOOKKEEP fences (i.e. no implicit sync)
(t+1) a VM operation on a BO/VM accessed by the CS.

to run concurrently. What it *doesn't* try is

(t+0) a VM operation on a BO/VM accessed by the CS.
(t+1) CS submission setting BOOKKEEP fences (i.e. no implicit sync)

to run concurrently. When you write


Only when all this is done we then can resolve the dependency that the
CS currently must wait for any clear operation on the VM.

isn't that all about the second problem?


No, it's the same.

See what we do in the VM code is to artificially insert a bubble so that 
all VM clear operations wait for all CS operations and then use the 
clear fence to indicate when the backing store of the BO can be freed.


When you want to remove this bubble (which is certainly a good idea) you 
need to first come up with a different approach to handle the clear 
operations.


Regards,
Christian.





Regards,
Christian.

Re: [RFC PATCH 3/5] drm/amdgpu: Allow explicit sync for VM ops.

On Fri, Jun 3, 2022 at 10:11 AM Christian König
 wrote:
>
> Am 03.06.22 um 03:21 schrieb Bas Nieuwenhuizen:
> > [SNIP]
> >> The problem is we need to wait on fences *not* added to the buffer object.
> > What fences wouldn't be added to the buffer object that we need here?
>
> Basically all still running submissions from the VM which could
> potentially access the BO.
>
> That's why we have the AMDGPU_SYNC_EQ_OWNER in amdgpu_vm_update_range().
>
> >> E.g. what we currently do here while freeing memory is:
> >> 1. Update the PTEs and make that update wait for everything!
> >> 2. Add the fence of that update to the freed up BO so that this BO isn't
> >> freed before the next CS.
> >>
> >> We might be able to fix this by adding the fences to the BO before
> >> freeing it manually, but I'm not 100% sure we can actually allocate
> >> memory for the fences in that moment.
> > I think we don't need to be able to. We're already adding the unmap
> > fence to the BO in the gem close ioctl, and that has the fallback that
> > if we can't allocate space for the fence in the BO, we wait on the
> > fence manually on the CPU. I think that is a reasonable fallback for
> > this as well?
>
> Yes, just blocking might work in an OOM situation as well.
>
> > For the TTM move path amdgpu_copy_buffer will wait on the BO resv and
> > then following submissions will trigger VM updates that will wait on
> > the amdgpu_copy_buffer jobs (and hence transitively) will wait on the
> > work.  AFAICT the amdgpu_bo_move does not trigger any VM updates by
> > itself, and the amdgpu_bo_move_notify is way after the move (and after
> > the ttm_bo_move_accel_cleanup which would free the old resource), so
> > any VM changes triggered by that would see the TTM copy and sync to
> > it.
> >
> > I do have to fix some stuff indeed, especially for the GEM close but
> > with that we should be able to keep the same basic approach?
>
> Nope, not even remotely.
>
> What we need is the following:
> 1. Rolling out my drm_exec patch set, so that we can lock buffers as needed.
> 2. When we get a VM operation we not only lock the VM page tables, but
> also all buffers we potentially need to unmap.
> 3. Nuking the freed list in the amdgpu_vm structure by updating freed
> areas directly when they are unmapped.
> 4. Tracking those updates inside the bo_va structure for the BO+VM
> combination.
> 5. When the bo_va structure is destroy because of closing the handle
> move the last clear operation over to the VM as implicit sync.
>

Hi Christian, isn't that a different problem though (that we're also
trying to solve, but in your series)?

What this patch tries to achieve:

(t+0) CS submission setting BOOKKEEP fences (i.e. no implicit sync)
(t+1) a VM operation on a BO/VM accessed by the CS.

to run concurrently. What it *doesn't* try is

(t+0) a VM operation on a BO/VM accessed by the CS.
(t+1) CS submission setting BOOKKEEP fences (i.e. no implicit sync)

to run concurrently. When you write

> Only when all this is done we then can resolve the dependency that the
> CS currently must wait for any clear operation on the VM.

isn't that all about the second problem?


>
> Regards,
> Christian.
>
>

Re: [v2] drm/msm: add null checks for drm device to avoid crash during probe defer


On 03/06/2022 12:42, Vinod Polimera wrote:

During probe defer, drm device is not initialized and an external
trigger to shutdown is trying to clean up drm device leading to crash.
Add checks to avoid drm device cleanup in such cases.

BUG: unable to handle kernel NULL pointer dereference at virtual
address 00b8

Call trace:

drm_atomic_helper_shutdown+0x44/0x144
msm_pdev_shutdown+0x2c/0x38
platform_shutdown+0x2c/0x38
device_shutdown+0x158/0x210
kernel_restart_prepare+0x40/0x4c
kernel_restart+0x20/0x6c
__arm64_sys_reboot+0x194/0x23c
invoke_syscall+0x50/0x13c
el0_svc_common+0xa0/0x17c
do_el0_svc_compat+0x28/0x34
el0_svc_compat+0x20/0x70
el0t_32_sync_handler+0xa8/0xcc
el0t_32_sync+0x1a8/0x1ac

Changes in v2:
- Add fixes tag.

Fixes: 623f279c778 ("drm/msm: fix shutdown hook in case GPU components failed to 
bind")
Signed-off-by: Vinod Polimera 


Also please remove bouncing quicinc.com emails from cc list


---
  drivers/gpu/drm/msm/msm_drv.c | 6 +-
  1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 4448536..d62ac66 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -142,6 +142,9 @@ static void msm_irq_uninstall(struct drm_device *dev)
struct msm_drm_private *priv = dev->dev_private;
struct msm_kms *kms = priv->kms;
  
+	if (!irq_has_action(kms->irq))

+   return;
+
kms->funcs->irq_uninstall(kms);
if (kms->irq_requested)
free_irq(kms->irq, dev);
@@ -259,6 +262,7 @@ static int msm_drm_uninit(struct device *dev)
  
  	ddev->dev_private = NULL;

drm_dev_put(ddev);
+   priv->dev = NULL;
  
  	destroy_workqueue(priv->wq);
  
@@ -1167,7 +1171,7 @@ void msm_drv_shutdown(struct platform_device *pdev)

struct msm_drm_private *priv = platform_get_drvdata(pdev);
struct drm_device *drm = priv ? priv->dev : NULL;
  
-	if (!priv || !priv->kms)

+   if (!priv || !priv->kms || !drm)
return;
  
  	drm_atomic_helper_shutdown(drm);



--
With best wishes
Dmitry

Re: [v2] drm/msm: add null checks for drm device to avoid crash during probe defer


On 03/06/2022 12:42, Vinod Polimera wrote:

During probe defer, drm device is not initialized and an external
trigger to shutdown is trying to clean up drm device leading to crash.
Add checks to avoid drm device cleanup in such cases.

BUG: unable to handle kernel NULL pointer dereference at virtual
address 00b8

Call trace:

drm_atomic_helper_shutdown+0x44/0x144
msm_pdev_shutdown+0x2c/0x38
platform_shutdown+0x2c/0x38
device_shutdown+0x158/0x210
kernel_restart_prepare+0x40/0x4c
kernel_restart+0x20/0x6c
__arm64_sys_reboot+0x194/0x23c
invoke_syscall+0x50/0x13c
el0_svc_common+0xa0/0x17c
do_el0_svc_compat+0x28/0x34
el0_svc_compat+0x20/0x70
el0t_32_sync_handler+0xa8/0xcc
el0t_32_sync+0x1a8/0x1ac

Changes in v2:
- Add fixes tag.


I'm still waiting for an answer to the questions raised in v1 review.



Fixes: 623f279c778 ("drm/msm: fix shutdown hook in case GPU components failed to 
bind")
Signed-off-by: Vinod Polimera 
---
  drivers/gpu/drm/msm/msm_drv.c | 6 +-
  1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 4448536..d62ac66 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -142,6 +142,9 @@ static void msm_irq_uninstall(struct drm_device *dev)
struct msm_drm_private *priv = dev->dev_private;
struct msm_kms *kms = priv->kms;
  
+	if (!irq_has_action(kms->irq))

+   return;
+
kms->funcs->irq_uninstall(kms);
if (kms->irq_requested)
free_irq(kms->irq, dev);
@@ -259,6 +262,7 @@ static int msm_drm_uninit(struct device *dev)
  
  	ddev->dev_private = NULL;

drm_dev_put(ddev);
+   priv->dev = NULL;
  
  	destroy_workqueue(priv->wq);
  
@@ -1167,7 +1171,7 @@ void msm_drv_shutdown(struct platform_device *pdev)

struct msm_drm_private *priv = platform_get_drvdata(pdev);
struct drm_device *drm = priv ? priv->dev : NULL;
  
-	if (!priv || !priv->kms)

+   if (!priv || !priv->kms || !drm)
return;
  
  	drm_atomic_helper_shutdown(drm);



--
With best wishes
Dmitry

[v2] drm/msm: add null checks for drm device to avoid crash during probe defer

During probe defer, drm device is not initialized and an external
trigger to shutdown is trying to clean up drm device leading to crash.
Add checks to avoid drm device cleanup in such cases.

BUG: unable to handle kernel NULL pointer dereference at virtual
address 00b8

Call trace:

drm_atomic_helper_shutdown+0x44/0x144
msm_pdev_shutdown+0x2c/0x38
platform_shutdown+0x2c/0x38
device_shutdown+0x158/0x210
kernel_restart_prepare+0x40/0x4c
kernel_restart+0x20/0x6c
__arm64_sys_reboot+0x194/0x23c
invoke_syscall+0x50/0x13c
el0_svc_common+0xa0/0x17c
do_el0_svc_compat+0x28/0x34
el0_svc_compat+0x20/0x70
el0t_32_sync_handler+0xa8/0xcc
el0t_32_sync+0x1a8/0x1ac

Changes in v2:
- Add fixes tag.

Fixes: 623f279c778 ("drm/msm: fix shutdown hook in case GPU components failed 
to bind")
Signed-off-by: Vinod Polimera 
---
 drivers/gpu/drm/msm/msm_drv.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 4448536..d62ac66 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -142,6 +142,9 @@ static void msm_irq_uninstall(struct drm_device *dev)
struct msm_drm_private *priv = dev->dev_private;
struct msm_kms *kms = priv->kms;
 
+   if (!irq_has_action(kms->irq))
+   return;
+
kms->funcs->irq_uninstall(kms);
if (kms->irq_requested)
free_irq(kms->irq, dev);
@@ -259,6 +262,7 @@ static int msm_drm_uninit(struct device *dev)
 
ddev->dev_private = NULL;
drm_dev_put(ddev);
+   priv->dev = NULL;
 
destroy_workqueue(priv->wq);
 
@@ -1167,7 +1171,7 @@ void msm_drv_shutdown(struct platform_device *pdev)
struct msm_drm_private *priv = platform_get_drvdata(pdev);
struct drm_device *drm = priv ? priv->dev : NULL;
 
-   if (!priv || !priv->kms)
+   if (!priv || !priv->kms || !drm)
return;
 
drm_atomic_helper_shutdown(drm);
-- 
2.7.4

Re: [PATCH v1 00/13] Host1x support on Tegra234

2022-06-03 Thread Dmitry Osipenko

On 5/16/22 13:02, cyn...@kapsi.fi wrote:
> Hi all,
> 
> This series adds support for Host1x and VIC on the recently released
> Tegra234 (Orin) SoC. It's split into the following parts:
> 
> * Device tree binding updates
> * Cleanup in host1x driver
> * Add programming of new registers and old registers that now need to
>   be programmed to a non-reset value
> * Tegra234 device data and headers
> * Rewrite of the job opcode sequence, and related patches to
>   support MLOCKs on Tegra186+.
> 
> The rewrite of the job opcode sequence brings Tegra186, Tegra194 and
> Tegra234 support to a 'full-featured' status that is necessary to
> support all host1x features in the future. This should not have any
> impact on older SoCs.
> 
> This series should be applied on top of the Host1x context isolation
> series.
> 
> Tested on Jetson AGX Xavier and Jetson AGX Orin.

The code looks okay at a quick glance. Please rebase the patches on top
of latest -next. Perhaps won't hurt to merge all the related patchsets
into a single series for 5.20.

-- 
Best regards,
Dmitry

Re: [v1] drm/msm: add null checks for drm device to avoid crash during probe defer


On 03/06/2022 12:22, Vinod Polimera wrote:

During probe defer, drm device is not initialized and an external
trigger to shutdown is trying to clean up drm device leading to crash.
Add checks to avoid drm device cleanup in such cases.

BUG: unable to handle kernel NULL pointer dereference at virtual
address 00b8

Call trace:

drm_atomic_helper_shutdown+0x44/0x144
msm_pdev_shutdown+0x2c/0x38
platform_shutdown+0x2c/0x38
device_shutdown+0x158/0x210
kernel_restart_prepare+0x40/0x4c
kernel_restart+0x20/0x6c
__arm64_sys_reboot+0x194/0x23c
invoke_syscall+0x50/0x13c
el0_svc_common+0xa0/0x17c
do_el0_svc_compat+0x28/0x34
el0_svc_compat+0x20/0x70
el0t_32_sync_handler+0xa8/0xcc
el0t_32_sync+0x1a8/0x1ac

Signed-off-by: Vinod Polimera 


Fixes ?


---
  drivers/gpu/drm/msm/msm_drv.c | 6 +-
  1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 4448536..d62ac66 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -142,6 +142,9 @@ static void msm_irq_uninstall(struct drm_device *dev)
struct msm_drm_private *priv = dev->dev_private;
struct msm_kms *kms = priv->kms;
  
+	if (!irq_has_action(kms->irq))

+   return;
+


Is this part required with 
https://patchwork.freedesktop.org/patch/485422/?series=103702=1?



kms->funcs->irq_uninstall(kms);
if (kms->irq_requested)
free_irq(kms->irq, dev);
@@ -259,6 +262,7 @@ static int msm_drm_uninit(struct device *dev)
  
  	ddev->dev_private = NULL;

drm_dev_put(ddev);
+   priv->dev = NULL;


What are you trying to protect here?

  
  	destroy_workqueue(priv->wq);
  
@@ -1167,7 +1171,7 @@ void msm_drv_shutdown(struct platform_device *pdev)

struct msm_drm_private *priv = platform_get_drvdata(pdev);
struct drm_device *drm = priv ? priv->dev : NULL;
  
-	if (!priv || !priv->kms)

+   if (!priv || !priv->kms || !drm)
return;
  
  	drm_atomic_helper_shutdown(drm);



--
With best wishes
Dmitry

[v3] drm/msm/disp/dpu1: avoid clearing hw interrupts if hw_intr is null during drm uninit

If dp modeset init is failed due to panel being not ready and
probe defers during drm bind, avoid clearing irqs and dereference
hw_intr when hw_intr is null.

BUG: Unable to handle kernel NULL pointer dereference at virtual address 


Call trace:
 dpu_core_irq_uninstall+0x50/0xb0
 dpu_irq_uninstall+0x18/0x24
 msm_drm_uninit+0xd8/0x16c
 msm_drm_bind+0x580/0x5fc
 try_to_bring_up_master+0x168/0x1c0
 __component_add+0xb4/0x178
 component_add+0x1c/0x28
 dp_display_probe+0x38c/0x400
 platform_probe+0xb0/0xd0
 really_probe+0xcc/0x2c8
 __driver_probe_device+0xbc/0xe8
 driver_probe_device+0x48/0xf0
 __device_attach_driver+0xa0/0xc8
 bus_for_each_drv+0x8c/0xd8
 __device_attach+0xc4/0x150
 device_initial_probe+0x1c/0x28

Changes in V2:
- Update commit message and correct fixes tag.

Changes in V3:
- Misc changes in commit text.

Fixes: f25f656608e3 ("drm/msm/dpu: merge struct dpu_irq into struct 
dpu_hw_intr")
Signed-off-by: Vinod Polimera 
Reviewed-by: Stephen Boyd 
Reviewed-by: Dmitry Baryshkov 
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
index c515b7c..ab28577 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
@@ -599,6 +599,9 @@ void dpu_core_irq_uninstall(struct dpu_kms *dpu_kms)
 {
int i;
 
+   if (!dpu_kms->hw_intr)
+   return;
+
pm_runtime_get_sync(_kms->pdev->dev);
for (i = 0; i < dpu_kms->hw_intr->total_irqs; i++)
if (!list_empty(_kms->hw_intr->irq_cb_tbl[i]))
-- 
2.7.4

[PATCH v6 5/6] ARM: configs: Enable DRM_V3D

BCM2711, the SoC used on the Raspberry Pi 4 has a different 3D
render GPU IP than its predecessors. Enable it it on multi v7
and bcm2835 configs.

Signed-off-by: Nicolas Saenz Julienne 
Signed-off-by: Peter Robinson 
Reviewed-by: Stefan Wahren 
Reviewed-by: Javier Martinez Canillas 
---
Changes since v4:
- Added to bcm2835_defconfig

 arch/arm/configs/bcm2835_defconfig  | 1 +
 arch/arm/configs/multi_v7_defconfig | 1 +
 2 files changed, 2 insertions(+)

diff --git a/arch/arm/configs/bcm2835_defconfig 
b/arch/arm/configs/bcm2835_defconfig
index a9ed79b7f871..9270512c14ea 100644
--- a/arch/arm/configs/bcm2835_defconfig
+++ b/arch/arm/configs/bcm2835_defconfig
@@ -106,6 +106,7 @@ CONFIG_REGULATOR_GPIO=y
 CONFIG_MEDIA_SUPPORT=y
 CONFIG_MEDIA_CAMERA_SUPPORT=y
 CONFIG_DRM=y
+CONFIG_DRM_V3D=y
 CONFIG_DRM_VC4=y
 CONFIG_FB_SIMPLE=y
 CONFIG_FRAMEBUFFER_CONSOLE=y
diff --git a/arch/arm/configs/multi_v7_defconfig 
b/arch/arm/configs/multi_v7_defconfig
index d6a6811f0539..e2db5cdc66b7 100644
--- a/arch/arm/configs/multi_v7_defconfig
+++ b/arch/arm/configs/multi_v7_defconfig
@@ -731,6 +731,7 @@ CONFIG_DRM_IMX_PARALLEL_DISPLAY=m
 CONFIG_DRM_IMX_TVE=m
 CONFIG_DRM_IMX_LDB=m
 CONFIG_DRM_IMX_HDMI=m
+CONFIG_DRM_V3D=m
 CONFIG_DRM_VC4=m
 CONFIG_DRM_ETNAVIV=m
 CONFIG_DRM_MXSFB=m
-- 
2.36.1

[PATCH v6 4/6] ARM: dts: bcm2711: Enable V3D

This adds the entry for V3D for bcm2711 (used in the Raspberry Pi 4)
and the associated firmware clock entry.

Signed-off-by: Nicolas Saenz Julienne 
Signed-off-by: Peter Robinson 
Reviewed-by: Javier Martinez Canillas 
---
Changes since v5:
- Update the compatible to match the other updated ones

Changes since v4:
- Move the firmware clock to bcm2711-rpi.dtsi

 arch/arm/boot/dts/bcm2711-rpi.dtsi |  4 
 arch/arm/boot/dts/bcm2711.dtsi | 11 +++
 2 files changed, 15 insertions(+)

diff --git a/arch/arm/boot/dts/bcm2711-rpi.dtsi 
b/arch/arm/boot/dts/bcm2711-rpi.dtsi
index ca266c5d9f9b..98817a6675b9 100644
--- a/arch/arm/boot/dts/bcm2711-rpi.dtsi
+++ b/arch/arm/boot/dts/bcm2711-rpi.dtsi
@@ -69,6 +69,10 @@ blconfig: nvram@0 {
};
 };
 
+ {
+   clocks = <_clocks 5>;
+};
+
  {
interrupts = ;
 };
diff --git a/arch/arm/boot/dts/bcm2711.dtsi b/arch/arm/boot/dts/bcm2711.dtsi
index 89af57482bc8..20e6771e8b1f 100644
--- a/arch/arm/boot/dts/bcm2711.dtsi
+++ b/arch/arm/boot/dts/bcm2711.dtsi
@@ -601,6 +601,17 @@ genet_mdio: mdio@e14 {
#size-cells = <0x0>;
};
};
+
+   v3d: gpu@7ec0 {
+   compatible = "brcm,2711-v3d";
+   reg = <0x0 0x7ec0 0x4000>,
+ <0x0 0x7ec04000 0x4000>;
+   reg-names = "hub", "core0";
+
+   power-domains = < BCM2835_POWER_DOMAIN_GRAFX_V3D>;
+   resets = < BCM2835_RESET_V3D>;
+   interrupts = ;
+   };
};
 };
 
-- 
2.36.1

[PATCH v6 6/6] arm64: config: Enable DRM_V3D

From: Nicolas Saenz Julienne 

BCM2711, the SoC used on the Raspberry Pi 4 has a different GPU than its
predecessors. Enable it.

Signed-off-by: Nicolas Saenz Julienne 
Signed-off-by: Peter Robinson 
Reviewed-by: Stefan Wahren 
Reviewed-by: Javier Martinez Canillas 
---
 arch/arm64/configs/defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index 50aa3d75ab4f..446bac1ef774 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -738,6 +738,7 @@ CONFIG_DRM_I2C_ADV7511_AUDIO=y
 CONFIG_DRM_DW_HDMI_AHB_AUDIO=m
 CONFIG_DRM_DW_HDMI_CEC=m
 CONFIG_DRM_IMX_DCSS=m
+CONFIG_DRM_V3D=m
 CONFIG_DRM_VC4=m
 CONFIG_DRM_ETNAVIV=m
 CONFIG_DRM_HISI_HIBMC=m
-- 
2.36.1

[PATCH v6 3/6] drm/v3d: Add support for bcm2711

Add compatible string and Kconfig options and help for bcm2711.

Signed-off-by: Nicolas Saenz Julienne 
Signed-off-by: Peter Robinson 
Reviewed-by: Stefan Wahren 
Reviewed-by: Javier Martinez Canillas 
---
Changes since v5:
- Update help text to cover all supported SoCs

Changes since v4:
- Change compatible to align downstream and other HW, reorder to suit

 drivers/gpu/drm/v3d/Kconfig   | 5 +++--
 drivers/gpu/drm/v3d/v3d_drv.c | 1 +
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/v3d/Kconfig b/drivers/gpu/drm/v3d/Kconfig
index e973ec487484..ce62c5908e1d 100644
--- a/drivers/gpu/drm/v3d/Kconfig
+++ b/drivers/gpu/drm/v3d/Kconfig
@@ -1,7 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0-only
 config DRM_V3D
tristate "Broadcom V3D 3.x and newer"
-   depends on ARCH_BCM || ARCH_BRCMSTB || COMPILE_TEST
+   depends on ARCH_BCM || ARCH_BRCMSTB || ARCH_BCM2835 || COMPILE_TEST
depends on DRM
depends on COMMON_CLK
depends on MMU
@@ -9,4 +9,5 @@ config DRM_V3D
select DRM_GEM_SHMEM_HELPER
help
  Choose this option if you have a system that has a Broadcom
- V3D 3.x or newer GPU, such as BCM7268.
+ V3D 3.x or newer GPUs. SoCs supported include the BCM2711,
+ BCM7268 and BCM7278.
diff --git a/drivers/gpu/drm/v3d/v3d_drv.c b/drivers/gpu/drm/v3d/v3d_drv.c
index 56d5f831e48b..8c7f910daa28 100644
--- a/drivers/gpu/drm/v3d/v3d_drv.c
+++ b/drivers/gpu/drm/v3d/v3d_drv.c
@@ -191,6 +191,7 @@ static const struct drm_driver v3d_drm_driver = {
 };
 
 static const struct of_device_id v3d_of_match[] = {
+   { .compatible = "brcm,2711-v3d" },
{ .compatible = "brcm,7268-v3d" },
{ .compatible = "brcm,7278-v3d" },
{},
-- 
2.36.1

[PATCH v6 1/6] dt-bindings: gpu: v3d: Add BCM2711's compatible

BCM2711, Raspberry Pi 4's SoC, contains a V3D core. So add its specific
compatible to the bindings.

Signed-off-by: Nicolas Saenz Julienne 
Signed-off-by: Peter Robinson 
Reviewed-by: Stefan Wahren 
Reviewed-by: Javier Martinez Canillas 
Acked-by: Rob Herring 
---
Changes since v4:
- Change compatible to align downstream and other HW, reorder to suit

 Documentation/devicetree/bindings/gpu/brcm,bcm-v3d.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/gpu/brcm,bcm-v3d.yaml 
b/Documentation/devicetree/bindings/gpu/brcm,bcm-v3d.yaml
index e6485f7b046f..217c42874f41 100644
--- a/Documentation/devicetree/bindings/gpu/brcm,bcm-v3d.yaml
+++ b/Documentation/devicetree/bindings/gpu/brcm,bcm-v3d.yaml
@@ -16,6 +16,7 @@ properties:
 
   compatible:
 enum:
+  - brcm,2711-v3d
   - brcm,7268-v3d
   - brcm,7278-v3d
 
-- 
2.36.1

[PATCH v6 2/6] drm/v3d: Get rid of pm code

Runtime PM doesn't seem to work correctly on this driver. On top of
that, commit 8b6864e3e138 ("drm/v3d/v3d_drv: Remove unused static
variable 'v3d_v3d_pm_ops'") hints that it most likely never did as the
driver's PM ops were not hooked-up.

So, in order to support regular operation with V3D on BCM2711 (Raspberry
Pi 4), get rid of the PM code. PM will be reinstated once we figure out
the underlying issues.

Signed-off-by: Nicolas Saenz Julienne 
Signed-off-by: Peter Robinson 
Reviewed-by: Javier Martinez Canillas 
---
Changes since v4:
- Rebase

Changes since v3:
- Minor updates for rebase

 drivers/gpu/drm/v3d/v3d_debugfs.c | 18 +-
 drivers/gpu/drm/v3d/v3d_drv.c | 11 ---
 drivers/gpu/drm/v3d/v3d_gem.c | 12 +---
 3 files changed, 2 insertions(+), 39 deletions(-)

diff --git a/drivers/gpu/drm/v3d/v3d_debugfs.c 
b/drivers/gpu/drm/v3d/v3d_debugfs.c
index 29fd13109e43..efbde124c296 100644
--- a/drivers/gpu/drm/v3d/v3d_debugfs.c
+++ b/drivers/gpu/drm/v3d/v3d_debugfs.c
@@ -4,7 +4,6 @@
 #include 
 #include 
 #include 
-#include 
 #include 
 #include 
 
@@ -131,11 +130,7 @@ static int v3d_v3d_debugfs_ident(struct seq_file *m, void 
*unused)
struct drm_device *dev = node->minor->dev;
struct v3d_dev *v3d = to_v3d_dev(dev);
u32 ident0, ident1, ident2, ident3, cores;
-   int ret, core;
-
-   ret = pm_runtime_get_sync(v3d->drm.dev);
-   if (ret < 0)
-   return ret;
+   int core;
 
ident0 = V3D_READ(V3D_HUB_IDENT0);
ident1 = V3D_READ(V3D_HUB_IDENT1);
@@ -188,9 +183,6 @@ static int v3d_v3d_debugfs_ident(struct seq_file *m, void 
*unused)
   (misccfg & V3D_MISCCFG_OVRTMUOUT) != 0);
}
 
-   pm_runtime_mark_last_busy(v3d->drm.dev);
-   pm_runtime_put_autosuspend(v3d->drm.dev);
-
return 0;
 }
 
@@ -218,11 +210,6 @@ static int v3d_measure_clock(struct seq_file *m, void 
*unused)
uint32_t cycles;
int core = 0;
int measure_ms = 1000;
-   int ret;
-
-   ret = pm_runtime_get_sync(v3d->drm.dev);
-   if (ret < 0)
-   return ret;
 
if (v3d->ver >= 40) {
V3D_CORE_WRITE(core, V3D_V4_PCTR_0_SRC_0_3,
@@ -246,9 +233,6 @@ static int v3d_measure_clock(struct seq_file *m, void 
*unused)
   cycles / (measure_ms * 1000),
   (cycles / (measure_ms * 100)) % 10);
 
-   pm_runtime_mark_last_busy(v3d->drm.dev);
-   pm_runtime_put_autosuspend(v3d->drm.dev);
-
return 0;
 }
 
diff --git a/drivers/gpu/drm/v3d/v3d_drv.c b/drivers/gpu/drm/v3d/v3d_drv.c
index 1afcd54fbbd5..56d5f831e48b 100644
--- a/drivers/gpu/drm/v3d/v3d_drv.c
+++ b/drivers/gpu/drm/v3d/v3d_drv.c
@@ -19,7 +19,6 @@
 #include 
 #include 
 #include 
-#include 
 #include 
 
 #include 
@@ -43,7 +42,6 @@ static int v3d_get_param_ioctl(struct drm_device *dev, void 
*data,
 {
struct v3d_dev *v3d = to_v3d_dev(dev);
struct drm_v3d_get_param *args = data;
-   int ret;
static const u32 reg_map[] = {
[DRM_V3D_PARAM_V3D_UIFCFG] = V3D_HUB_UIFCFG,
[DRM_V3D_PARAM_V3D_HUB_IDENT1] = V3D_HUB_IDENT1,
@@ -69,17 +67,12 @@ static int v3d_get_param_ioctl(struct drm_device *dev, void 
*data,
if (args->value != 0)
return -EINVAL;
 
-   ret = pm_runtime_get_sync(v3d->drm.dev);
-   if (ret < 0)
-   return ret;
if (args->param >= DRM_V3D_PARAM_V3D_CORE0_IDENT0 &&
args->param <= DRM_V3D_PARAM_V3D_CORE0_IDENT2) {
args->value = V3D_CORE_READ(0, offset);
} else {
args->value = V3D_READ(offset);
}
-   pm_runtime_mark_last_busy(v3d->drm.dev);
-   pm_runtime_put_autosuspend(v3d->drm.dev);
return 0;
}
 
@@ -280,10 +273,6 @@ static int v3d_platform_drm_probe(struct platform_device 
*pdev)
return -ENOMEM;
}
 
-   pm_runtime_use_autosuspend(dev);
-   pm_runtime_set_autosuspend_delay(dev, 50);
-   pm_runtime_enable(dev);
-
ret = v3d_gem_init(drm);
if (ret)
goto dma_free;
diff --git a/drivers/gpu/drm/v3d/v3d_gem.c b/drivers/gpu/drm/v3d/v3d_gem.c
index 92bc0faee84f..7026214a09f0 100644
--- a/drivers/gpu/drm/v3d/v3d_gem.c
+++ b/drivers/gpu/drm/v3d/v3d_gem.c
@@ -6,7 +6,6 @@
 #include 
 #include 
 #include 
-#include 
 #include 
 #include 
 #include 
@@ -367,9 +366,6 @@ v3d_job_free(struct kref *ref)
dma_fence_put(job->irq_fence);
dma_fence_put(job->done_fence);
 
-   pm_runtime_mark_last_busy(job->v3d->drm.dev);
-   pm_runtime_put_autosuspend(job->v3d->drm.dev);
-
if (job->perfmon)
v3d_perfmon_put(job->perfmon);
 
@@ -471,14 +467,10 @@ v3d_job_init(struct v3d_dev *v3d, struct drm_file 
*file_priv,
job->v3d = v3d;
job->free =

[PATCH v6 0/6] Raspberry PI 4 V3D enablement

This is a follow up from my v4 patchset. The power management pieces have
been split out to a separate independent set of patches by Stefan [1]. This
version 5 of the DRM patches are independent and given the V3D driver has
been upstream for some time the two patches to enable it in defconfigs can 
be taken at anytime independent of the enablement for the Raspberry Pi 4.

I've tested this using mesa 22.0.x and Wayland/Gnome on Fedora 36, it's 
more or less stable with basic testing.

Changes since v5:
- Update the DT compatible to match the others that were updated
- Adjust the Kconfig help text
- Add review tags

Changes since v4:
- Fixes for device tree and bindings
- Split out the power management changes into an independent set
- Rebase to 5.18
- Individual changes in patches

[1] https://www.spinics.net/lists/arm-kernel/msg980342.html

Nicolas Saenz Julienne (1):
  arm64: config: Enable DRM_V3D

Peter Robinson (5):
  dt-bindings: gpu: v3d: Add BCM2711's compatible
  drm/v3d: Get rid of pm code
  drm/v3d: Add support for bcm2711
  ARM: dts: bcm2711: Enable V3D
  ARM: configs: Enable DRM_V3D

 .../devicetree/bindings/gpu/brcm,bcm-v3d.yaml  |  1 +
 arch/arm/boot/dts/bcm2711-rpi.dtsi |  4 
 arch/arm/boot/dts/bcm2711.dtsi | 11 +++
 arch/arm/configs/bcm2835_defconfig |  1 +
 arch/arm/configs/multi_v7_defconfig|  1 +
 arch/arm64/configs/defconfig   |  1 +
 drivers/gpu/drm/v3d/Kconfig|  5 +++--
 drivers/gpu/drm/v3d/v3d_debugfs.c  | 18 +-
 drivers/gpu/drm/v3d/v3d_drv.c  | 12 +---
 drivers/gpu/drm/v3d/v3d_gem.c  | 12 +---
 10 files changed, 25 insertions(+), 41 deletions(-)

-- 
2.36.1

[v1] drm/msm: add null checks for drm device to avoid crash during probe defer

During probe defer, drm device is not initialized and an external
trigger to shutdown is trying to clean up drm device leading to crash.
Add checks to avoid drm device cleanup in such cases.

BUG: unable to handle kernel NULL pointer dereference at virtual
address 00b8

Call trace:

drm_atomic_helper_shutdown+0x44/0x144
msm_pdev_shutdown+0x2c/0x38
platform_shutdown+0x2c/0x38
device_shutdown+0x158/0x210
kernel_restart_prepare+0x40/0x4c
kernel_restart+0x20/0x6c
__arm64_sys_reboot+0x194/0x23c
invoke_syscall+0x50/0x13c
el0_svc_common+0xa0/0x17c
do_el0_svc_compat+0x28/0x34
el0_svc_compat+0x20/0x70
el0t_32_sync_handler+0xa8/0xcc
el0t_32_sync+0x1a8/0x1ac

Signed-off-by: Vinod Polimera 
---
 drivers/gpu/drm/msm/msm_drv.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 4448536..d62ac66 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -142,6 +142,9 @@ static void msm_irq_uninstall(struct drm_device *dev)
struct msm_drm_private *priv = dev->dev_private;
struct msm_kms *kms = priv->kms;
 
+   if (!irq_has_action(kms->irq))
+   return;
+
kms->funcs->irq_uninstall(kms);
if (kms->irq_requested)
free_irq(kms->irq, dev);
@@ -259,6 +262,7 @@ static int msm_drm_uninit(struct device *dev)
 
ddev->dev_private = NULL;
drm_dev_put(ddev);
+   priv->dev = NULL;
 
destroy_workqueue(priv->wq);
 
@@ -1167,7 +1171,7 @@ void msm_drv_shutdown(struct platform_device *pdev)
struct msm_drm_private *priv = platform_get_drvdata(pdev);
struct drm_device *drm = priv ? priv->dev : NULL;
 
-   if (!priv || !priv->kms)
+   if (!priv || !priv->kms || !drm)
return;
 
drm_atomic_helper_shutdown(drm);
-- 
2.7.4

Re: [PATCH] drm/sun4i: sun8i: Add the ability to keep scaler enabled for VI layer

2022-06-03 Thread Roman Stratiienko

Hi Maxime,

пт, 3 июн. 2022 г. в 11:24, Maxime Ripard :
>
> Hi,
>
> On Thu, Jun 02, 2022 at 06:01:18PM +, Roman Stratiienko wrote:
> > According to DE2.0/DE3.0 manual VI scaler enable register is double
> > buffered, but de facto it doesn't, or the hardware has the shadow
> > register latching issues which causes single-frame picture corruption
> > after changing the state of scaler enable register.
> >
> > Allow the user to keep the scaler always enabled, preventing the UI
> > glitches on the transition from scaled to unscaled state.
> >
> > NOTE:
> > UI layer scaler has more registers with double-buffering issue and can't
> > be workarounded in the same manner.
> >
> > You may find a python test and a demo video for this issue at [1]
> >
> > [1]: https://github.com/GloDroid/glodroid_tests/issues/4
>
> Please describe the issue entirely here. The commit log must be 
> self-sufficient.

Commit message already states "single-frame picture corruption after
changing the state of scaler enable register", therefore I find it
already self-sufficient

Also I find demo videos and link to tests useful for the followers to
go further with investigation.

If you have something specific in mind when asking to enhance the
commit message please say it.

>
> > Signed-off-by: Roman Stratiienko 
> > ---
> >  drivers/gpu/drm/sun4i/sun8i_mixer.c| 12 
> >  drivers/gpu/drm/sun4i/sun8i_vi_layer.c |  4 +++-
> >  2 files changed, 15 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c 
> > b/drivers/gpu/drm/sun4i/sun8i_mixer.c
> > index 71ab0a00b4de..15cad0330f66 100644
> > --- a/drivers/gpu/drm/sun4i/sun8i_mixer.c
> > +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c
> > @@ -27,6 +27,18 @@
> >  #include "sun8i_vi_layer.h"
> >  #include "sunxi_engine.h"
> >
> > +/* According to DE2.0/DE3.0 manual VI scaler enable register is double
> > + * buffered, but de facto it doesn't, or the hardware has the shadow
> > + * register latching issues which causes single-frame picture corruption
> > + * after changing the state of scaler enable register.
> > + * Allow the user to keep the scaler always enabled, preventing the UI
> > + * glitches on the transition from scaled to unscaled state.
> > + */
> > +int sun8i_vi_keep_scaler_enabled;
> > +MODULE_PARM_DESC(keep_vi_scaler_enabled,
> > +  "Keep VI scaler enabled (1 = enabled, 0 = disabled 
> > (default))");
> > +module_param_named(keep_vi_scaler_enabled, sun8i_vi_keep_scaler_enabled, 
> > int, 0644);
> > +
>
> It's not clear to me why we would want to make that a parameter?
>
>1 If it never works, we should fix it once and for all and not allow a broken 
>setup at all.

It's a hardware issue and can be fixed only within the hardware.

Current patch is a workaround that if enabled can cause increased
power consumption
for existing users. Therefore I think it is better to give existing
distro-maintainers a chance to test it prior to delivery.

Also I do not have all the sunxi SOCs to test it. My tests were
limited only by A64 and H6 SOCs.

> Maxime

Best regards,
Roman

Re: [PATCH] drm/sun4i: sun8i: Add the ability to keep scaler enabled for VI layer

Hi,

On Thu, Jun 02, 2022 at 06:01:18PM +, Roman Stratiienko wrote:
> According to DE2.0/DE3.0 manual VI scaler enable register is double
> buffered, but de facto it doesn't, or the hardware has the shadow
> register latching issues which causes single-frame picture corruption
> after changing the state of scaler enable register.
> 
> Allow the user to keep the scaler always enabled, preventing the UI
> glitches on the transition from scaled to unscaled state.
> 
> NOTE:
> UI layer scaler has more registers with double-buffering issue and can't
> be workarounded in the same manner.
> 
> You may find a python test and a demo video for this issue at [1]
> 
> [1]: https://github.com/GloDroid/glodroid_tests/issues/4

Please describe the issue entirely here. The commit log must be self-sufficient.

> Signed-off-by: Roman Stratiienko 
> ---
>  drivers/gpu/drm/sun4i/sun8i_mixer.c| 12 
>  drivers/gpu/drm/sun4i/sun8i_vi_layer.c |  4 +++-
>  2 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c 
> b/drivers/gpu/drm/sun4i/sun8i_mixer.c
> index 71ab0a00b4de..15cad0330f66 100644
> --- a/drivers/gpu/drm/sun4i/sun8i_mixer.c
> +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c
> @@ -27,6 +27,18 @@
>  #include "sun8i_vi_layer.h"
>  #include "sunxi_engine.h"
>  
> +/* According to DE2.0/DE3.0 manual VI scaler enable register is double
> + * buffered, but de facto it doesn't, or the hardware has the shadow
> + * register latching issues which causes single-frame picture corruption
> + * after changing the state of scaler enable register.
> + * Allow the user to keep the scaler always enabled, preventing the UI
> + * glitches on the transition from scaled to unscaled state.
> + */
> +int sun8i_vi_keep_scaler_enabled;
> +MODULE_PARM_DESC(keep_vi_scaler_enabled,
> +  "Keep VI scaler enabled (1 = enabled, 0 = disabled 
> (default))");
> +module_param_named(keep_vi_scaler_enabled, sun8i_vi_keep_scaler_enabled, 
> int, 0644);
> +

It's not clear to me why we would want to make that a parameter?

If it never works, we should fix it once and for all and not allow a broken 
setup at all.

Maxime

Re: [PATCH v3 3/4] drm/bridge: Add devm_drm_bridge_add()

On Tue, May 31, 2022 at 02:06:34PM -0700, Doug Anderson wrote:
> On Mon, May 23, 2022 at 10:00 AM Doug Anderson  wrote:
> > On Sat, May 21, 2022 at 2:17 AM Maxime Ripard  wrote:
> > > On Tue, May 10, 2022 at 12:29:43PM -0700, Douglas Anderson wrote:
> > > > This adds a devm managed version of drm_bridge_add(). Like other
> > > > "devm" function listed in drm_bridge.h, this function takes an
> > > > explicit "dev" to use for the lifetime management. A few notes:
> > > > * In general we have a "struct device" for bridges that makes a good
> > > >   candidate for where the lifetime matches exactly what we want.
> > > > * The "bridge->dev->dev" device appears to be the encoder
> > > >   device. That's not the right device to use for lifetime management.
> > > >
> > > > Suggested-by: Dmitry Baryshkov 
> > > > Signed-off-by: Douglas Anderson 
> > >
> > > If we are to introduce more managed helpers, I think it'd be wiser to
> > > introduce them as DRM-managed, and not device managed.
> > >
> > > Otherwise, you'll end up in a weird state when a device has been removed
> > > but the DRM device is still around.
> >
> > I'm kinda confused. In this case there is no DRM device for the bridge
> > and, as per my CL description, "bridge-dev->dev" appears to be the
> > encoder device. I wasn't personally involved in discussions about it,
> > but I was under the impression that this was expected / normal. Thus
> > we can't make this DRM-managed.
> 
> Since I didn't hear a reply,

Gah, I replied but it looks like somehow it never reached the ML...

Here was my original reply:

> > > This adds a devm managed version of drm_bridge_add(). Like other
> > > "devm" function listed in drm_bridge.h, this function takes an
> > > explicit "dev" to use for the lifetime management. A few notes:
> > > * In general we have a "struct device" for bridges that makes a good
> > >   candidate for where the lifetime matches exactly what we want.
> > > * The "bridge->dev->dev" device appears to be the encoder
> > >   device. That's not the right device to use for lifetime management.
> > >
> > > Suggested-by: Dmitry Baryshkov 
> > > Signed-off-by: Douglas Anderson 
> >
> > If we are to introduce more managed helpers, I think it'd be wiser to
> > introduce them as DRM-managed, and not device managed.
> >
> > Otherwise, you'll end up in a weird state when a device has been removed
> > but the DRM device is still around.
>=20
> I'm kinda confused. In this case there is no DRM device for the bridge
> and, as per my CL description, "bridge-dev->dev" appears to be the
> encoder device.

bridge->dev seems right though?

> I wasn't personally involved in discussions about it, but I was under
> the impression that this was expected / normal. Thus we can't make
> this DRM-managed.

Still, I don't think devm is the right solution to this either.

The underlying issue is two-fold:

  - Encoders can have a pointer to a bridge through of_drm_find_bridge
or similar. However, bridges are traditionally tied to their device
lifetime (by calling drm_bridge_add in probe, and drm_bridge_remove
in remove). Encoders will typically be tied to the DRM device
however, and that one sticks around until the last application
closes it. We can thus very easily end up with a dangling pointer,
and a use-after-free.

  - It's not the case yet, but it doesn't seem far fetch to expose
properties of bridges to the userspace. In that case, the userspace
would be likely to still hold references to objects that aren't
there anymore when the bridge is gone.

The first is obviously a larger concern, but if we can find a solution
that would accomodate the second it would be great.

As far as I can see, we should fix in two steps:

  - in drm_bridge_attach, we should add a device-managed call that will
unregister the main DRM device. We don't allow to probe the main DRM
device when the bridge isn't there yet in most case, so it makes
sense to remove it once the bridge is no longer there as well.

  - When the DRM device is removed, have the core cleanup any bridge
registered. That will remove the need to have drm_bridge_remove in
the first place.

> I'll assume that my response addressed your concerns. Assuming I get
> reviews for the other two patches in this series I'll plan to land
> this with Dmitry's review.

I still don't think it's a good idea to merge it. It gives an illusion
of being safe, but it's really far from it.

Maxime

Re: [RFC PATCH 3/5] drm/amdgpu: Allow explicit sync for VM ops.


Am 03.06.22 um 03:21 schrieb Bas Nieuwenhuizen:

[SNIP]

The problem is we need to wait on fences *not* added to the buffer object.

What fences wouldn't be added to the buffer object that we need here?


Basically all still running submissions from the VM which could 
potentially access the BO.


That's why we have the AMDGPU_SYNC_EQ_OWNER in amdgpu_vm_update_range().


E.g. what we currently do here while freeing memory is:
1. Update the PTEs and make that update wait for everything!
2. Add the fence of that update to the freed up BO so that this BO isn't
freed before the next CS.

We might be able to fix this by adding the fences to the BO before
freeing it manually, but I'm not 100% sure we can actually allocate
memory for the fences in that moment.

I think we don't need to be able to. We're already adding the unmap
fence to the BO in the gem close ioctl, and that has the fallback that
if we can't allocate space for the fence in the BO, we wait on the
fence manually on the CPU. I think that is a reasonable fallback for
this as well?


Yes, just blocking might work in an OOM situation as well.


For the TTM move path amdgpu_copy_buffer will wait on the BO resv and
then following submissions will trigger VM updates that will wait on
the amdgpu_copy_buffer jobs (and hence transitively) will wait on the
work.  AFAICT the amdgpu_bo_move does not trigger any VM updates by
itself, and the amdgpu_bo_move_notify is way after the move (and after
the ttm_bo_move_accel_cleanup which would free the old resource), so
any VM changes triggered by that would see the TTM copy and sync to
it.

I do have to fix some stuff indeed, especially for the GEM close but
with that we should be able to keep the same basic approach?


Nope, not even remotely.

What we need is the following:
1. Rolling out my drm_exec patch set, so that we can lock buffers as needed.
2. When we get a VM operation we not only lock the VM page tables, but 
also all buffers we potentially need to unmap.
3. Nuking the freed list in the amdgpu_vm structure by updating freed 
areas directly when they are unmapped.
4. Tracking those updates inside the bo_va structure for the BO+VM 
combination.
5. When the bo_va structure is destroy because of closing the handle 
move the last clear operation over to the VM as implicit sync.


Only when all this is done we then can resolve the dependency that the 
CS currently must wait for any clear operation on the VM.


Regards,
Christian.

[PATCH V15 06/24] LoongArch: Add writecombine support for drm

2022-06-03 Thread Huacai Chen

LoongArch maintains cache coherency in hardware, but its WUC attribute
(Weak-ordered UnCached, which is similar to WC) is out of the scope of
cache coherency machanism. This means WUC can only used for write-only
memory regions.

Cc: Daniel Vetter 
Cc: dri-devel@lists.freedesktop.org
Reviewed-by: WANG Xuerui 
Reviewed-by: Jiaxun Yang 
Signed-off-by: Huacai Chen 
---
 drivers/gpu/drm/drm_vm.c | 2 +-
 drivers/gpu/drm/ttm/ttm_module.c | 2 +-
 include/drm/drm_cache.h  | 8 
 3 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_vm.c b/drivers/gpu/drm/drm_vm.c
index e957d4851dc0..f024dc93939e 100644
--- a/drivers/gpu/drm/drm_vm.c
+++ b/drivers/gpu/drm/drm_vm.c
@@ -69,7 +69,7 @@ static pgprot_t drm_io_prot(struct drm_local_map *map,
pgprot_t tmp = vm_get_page_prot(vma->vm_flags);
 
 #if defined(__i386__) || defined(__x86_64__) || defined(__powerpc__) || \
-defined(__mips__)
+defined(__mips__) || defined(__loongarch__)
if (map->type == _DRM_REGISTERS && !(map->flags & _DRM_WRITE_COMBINING))
tmp = pgprot_noncached(tmp);
else
diff --git a/drivers/gpu/drm/ttm/ttm_module.c b/drivers/gpu/drm/ttm/ttm_module.c
index a3ad7c9736ec..b3fffe7b5062 100644
--- a/drivers/gpu/drm/ttm/ttm_module.c
+++ b/drivers/gpu/drm/ttm/ttm_module.c
@@ -74,7 +74,7 @@ pgprot_t ttm_prot_from_caching(enum ttm_caching caching, 
pgprot_t tmp)
 #endif /* CONFIG_UML */
 #endif /* __i386__ || __x86_64__ */
 #if defined(__ia64__) || defined(__arm__) || defined(__aarch64__) || \
-   defined(__powerpc__) || defined(__mips__)
+   defined(__powerpc__) || defined(__mips__) || defined(__loongarch__)
if (caching == ttm_write_combined)
tmp = pgprot_writecombine(tmp);
else
diff --git a/include/drm/drm_cache.h b/include/drm/drm_cache.h
index 22deb216b59c..08e0e3ffad13 100644
--- a/include/drm/drm_cache.h
+++ b/include/drm/drm_cache.h
@@ -67,6 +67,14 @@ static inline bool drm_arch_can_wc_memory(void)
 * optimization entirely for ARM and arm64.
 */
return false;
+#elif defined(CONFIG_LOONGARCH)
+   /*
+* LoongArch maintains cache coherency in hardware, but its WUC 
attribute
+* (Weak-ordered UnCached, which is similar to WC) is out of the scope 
of
+* cache coherency machanism. This means WUC can only used for 
write-only
+* memory regions.
+*/
+   return false;
 #else
return true;
 #endif
-- 
2.27.0

Re: [Intel-gfx] [RFC v3 1/3] drm/doc/rfc: VM_BIND feature design document

2022-06-03 Thread Lionel Landwerlin

On 02/06/2022 23:35, Jason Ekstrand wrote:
On Thu, Jun 2, 2022 at 3:11 PM Niranjana Vishwanathapura 
 wrote:

On Wed, Jun 01, 2022 at 01:28:36PM -0700, Matthew Brost wrote:
>On Wed, Jun 01, 2022 at 05:25:49PM +0300, Lionel Landwerlin wrote:
>> On 17/05/2022 21:32, Niranjana Vishwanathapura wrote:
>> > +VM_BIND/UNBIND ioctl will immediately start
binding/unbinding the mapping in an
>> > +async worker. The binding and unbinding will work like a
special GPU engine.
>> > +The binding and unbinding operations are serialized and will
wait on specified
>> > +input fences before the operation and will signal the output
fences upon the
>> > +completion of the operation. Due to serialization,
completion of an operation
>> > +will also indicate that all previous operations are also
complete.
>>
>> I guess we should avoid saying "will immediately start
binding/unbinding" if
>> there are fences involved.
>>
>> And the fact that it's happening in an async worker seem to
imply it's not
>> immediate.
>>

Ok, will fix.
This was added because in earlier design binding was deferred
until next execbuff.
But now it is non-deferred (immediate in that sense). But yah,
this is confusing
and will fix it.

>>
>> I have a question on the behavior of the bind operation when no
input fence
>> is provided. Let say I do :
>>
>> VM_BIND (out_fence=fence1)
>>
>> VM_BIND (out_fence=fence2)
>>
>> VM_BIND (out_fence=fence3)
>>
>>
>> In what order are the fences going to be signaled?
>>
>> In the order of VM_BIND ioctls? Or out of order?
>>
>> Because you wrote "serialized I assume it's : in order
>>

Yes, in the order of VM_BIND/UNBIND ioctls. Note that bind and
unbind will use
the same queue and hence are ordered.

>>
>> One thing I didn't realize is that because we only get one
"VM_BIND" engine,
>> there is a disconnect from the Vulkan specification.
>>
>> In Vulkan VM_BIND operations are serialized but per engine.
>>
>> So you could have something like this :
>>
>> VM_BIND (engine=rcs0, in_fence=fence1, out_fence=fence2)
>>
>> VM_BIND (engine=ccs0, in_fence=fence3, out_fence=fence4)
>>
>>
>> fence1 is not signaled
>>
>> fence3 is signaled
>>
>> So the second VM_BIND will proceed before the first VM_BIND.
>>
>>
>> I guess we can deal with that scenario in userspace by doing
the wait
>> ourselves in one thread per engines.
>>
>> But then it makes the VM_BIND input fences useless.
>>
>>
>> Daniel : what do you think? Should be rework this or just deal
with wait
>> fences in userspace?
>>
>
>My opinion is rework this but make the ordering via an engine
param optional.
>
>e.g. A VM can be configured so all binds are ordered within the VM
>
>e.g. A VM can be configured so all binds accept an engine
argument (in
>the case of the i915 likely this is a gem context handle) and binds
>ordered with respect to that engine.
>
>This gives UMDs options as the later likely consumes more KMD
resources
>so if a different UMD can live with binds being ordered within the VM
>they can use a mode consuming less resources.
>

I think we need to be careful here if we are looking for some out of
(submission) order completion of vm_bind/unbind.
In-order completion means, in a batch of binds and unbinds to be
completed in-order, user only needs to specify in-fence for the
first bind/unbind call and the our-fence for the last bind/unbind
call. Also, the VA released by an unbind call can be re-used by
any subsequent bind call in that in-order batch.

These things will break if binding/unbinding were to be allowed to
go out of order (of submission) and user need to be extra careful
not to run into pre-mature triggereing of out-fence and bind failing
as VA is still in use etc.

Also, VM_BIND binds the provided mapping on the specified address
space
(VM). So, the uapi is not engine/context specific.

We can however add a 'queue' to the uapi which can be one from the
pre-defined queues,
I915_VM_BIND_QUEUE_0
I915_VM_BIND_QUEUE_1
...
I915_VM_BIND_QUEUE_(N-1)

KMD will spawn an async work queue for each queue which will only
bind the mappings on that queue in the order of submission.
User can assign the queue to per engine or anything like that.

But again here, user need to be careful and not deadlock these
queues with circular dependency of fences.

I prefer adding this later an as extension based on whether it
is really helping with the implementation.

I can tell you right now that having everything on a single in-order 
queue will not get us the perf we want. What vulkan

Re: How should "max bpc" and "Colorspace" KMS property work?

2022-06-03 Thread Pekka Paalanen

On Thu, 2 Jun 2022 19:40:01 +0300
Ville Syrjälä  wrote:

> On Thu, Jun 02, 2022 at 10:47:59AM +0300, Pekka Paalanen wrote:
> > On Wed, 1 Jun 2022 17:06:25 +0300
> > Ville Syrjälä  wrote:
> >   

...

> > > The "Colorspace" property just changes what we report to the display
> > > via infoframes/MSA/SDP. It does not cause the display hardware to do
> > > any colorspace conversion/etc.  
> > 
> > Good.
> >   
> > > To actually force the display hardware to do the desired conversion
> > > we need some new properties. ATM the only automagic conversion that
> > > can happen (at least with i915) is the RGB 4:4:4->YCbCr 4:2:0 fallback,
> > > which is needed on some displays to get 4k+ modes to work at all.  
> > 
> > When "Colorspace" says RGB, and the automatic fallback would kick in to
> > create a conflict, what happens?  
> 
> I would put that in the "Doctor, it hurts when I..." category.

Hi Ville,

I meant specifically the case where the driver chooses to use YCbCr
4:2:0 even though userspace is setting absolutely everything to RGB.

So yeah, that is a problem, like you and Sebastian agreed later.

Am I safe from that automatic driver fallback if I don't use 4k or
bigger video modes? What about e.g. 1080p@120 or something? Can I
calculate when the fallback will happen and choose my "Colorspace"
accordingly? Which also requires me to set up the RGB->YCbCR color
model conversion myself?

What about failing the modeset instead if userspace sets "Colorspace"
explicitly to RGB, and the driver would need to fall back to YCbCR
4:2:0?

That would make the most sense to me, as then the driver would not
silently do a buggy thing.

> > I thought we agreed that "max bpc" means limiting link bpc to at most
> > that value, but the driver will automatically pick a lower value if
> > that makes the requested video mode work (and in lack of new KMS
> > properties).
> > 
> > I have no desire that change that. I also think the Plymouth issue is
> > not fully fixable without some new KMS property, and until then the
> > only thing Plymouth could do is to smash "max bpc" always to 8 - which
> > mostly stops being a problem once display servers learn to handle "max
> > bpc".  
> 
> There's no real differene between the kernel automagically clamping
> "max bpc" to the current BIOS programmed value vs. plymouth doing it.
> Either approach will break deep color support for current userspace
> which doesn't reset "max bpc" back to the max.

There is one big difference: if Plymouth does it, then people cannot
scream "kernel regression". People can point fingers at Plymouth, but I
would imagine the actual fixes will come as patches to other KMS clients
and not Plymouth.

Thanks,
pq

pgpdkWgccarOE.pgp
Description: OpenPGP digital signature

Re: [PATCH] drm/sun4i: sun8i: Add the ability to keep scaler enabled for VI layer

2022-06-03 Thread Roman Stratiienko

Hi Ondrej,

пт, 3 июн. 2022 г. в 00:55, Ondřej Jirman :
>
> Hi Roman,
>
> On Thu, Jun 02, 2022 at 06:01:18PM +, Roman Stratiienko wrote:
> > According to DE2.0/DE3.0 manual VI scaler enable register is double
> > buffered, but de facto it doesn't, or the hardware has the shadow
> > register latching issues which causes single-frame picture corruption
> > after changing the state of scaler enable register.
> >
> > Allow the user to keep the scaler always enabled, preventing the UI
> > glitches on the transition from scaled to unscaled state.
> >
> > NOTE:
> > UI layer scaler has more registers with double-buffering issue and can't
> > be workarounded in the same manner.
> >
> > You may find a python test and a demo video for this issue at [1]
>
> Isn't this an issue with kernel driver not waiting for DE2 FINISH IRQ, but
> for VBLANK IRQ from TCON instead, before allowing to write new set of register
> values?

No, DE2 FINISH IRQ is triggered just some micro or even nanoseconds
earlier from vblank IRQ
(I have checked it using tracing).

I can guess MISSION FINISH IRQ triggered when the last line is being
sent to the line buffer cache (in between of the mixer output and tcon
input).
But VBLANG IRQ triggers when the last line is being sent to the
display + hfront porch time + hsync time.

I have also tried scheduling the register updates that have
double-buffering issues at VBLANK irq. And such a solution fixes the
test for both VI and UI scalers.
But under high loads there are still glitches happening. This patch
solves the issue for both test and high load conditions. (but for VI
only)
I'll post my solution with a deferred scaling register update soon.

>
> https://megous.com/dl/tmp/4fe35b3fc72ee7de.png
>
> I haven't checked if FINISH flag is set at time of VBLANK interrupt, so maybe
> this is not the issue.
>
> regards,
> o.
>
> > [1]: https://github.com/GloDroid/glodroid_tests/issues/4
> > Signed-off-by: Roman Stratiienko 
> > ---
> >  drivers/gpu/drm/sun4i/sun8i_mixer.c| 12 
> >  drivers/gpu/drm/sun4i/sun8i_vi_layer.c |  4 +++-
> >  2 files changed, 15 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c 
> > b/drivers/gpu/drm/sun4i/sun8i_mixer.c
> > index 71ab0a00b4de..15cad0330f66 100644
> > --- a/drivers/gpu/drm/sun4i/sun8i_mixer.c
> > +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c
> > @@ -27,6 +27,18 @@
> >  #include "sun8i_vi_layer.h"
> >  #include "sunxi_engine.h"
> >
> > +/* According to DE2.0/DE3.0 manual VI scaler enable register is double
> > + * buffered, but de facto it doesn't, or the hardware has the shadow
> > + * register latching issues which causes single-frame picture corruption
> > + * after changing the state of scaler enable register.
> > + * Allow the user to keep the scaler always enabled, preventing the UI
> > + * glitches on the transition from scaled to unscaled state.
> > + */
> > +int sun8i_vi_keep_scaler_enabled;
> > +MODULE_PARM_DESC(keep_vi_scaler_enabled,
> > +  "Keep VI scaler enabled (1 = enabled, 0 = disabled 
> > (default))");
> > +module_param_named(keep_vi_scaler_enabled, sun8i_vi_keep_scaler_enabled, 
> > int, 0644);
> > +
> >  struct de2_fmt_info {
> >   u32 drm_fmt;
> >   u32 de2_fmt;
> > diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c 
> > b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> > index 662ba1018cc4..f005ab883503 100644
> > --- a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> > +++ b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> > @@ -17,6 +17,8 @@
> >  #include "sun8i_vi_layer.h"
> >  #include "sun8i_vi_scaler.h"
> >
> > +extern int sun8i_vi_keep_scaler_enabled;
> > +
> >  static void sun8i_vi_layer_enable(struct sun8i_mixer *mixer, int channel,
> > int overlay, bool enable, unsigned int zpos)
> >  {
> > @@ -149,7 +151,7 @@ static int sun8i_vi_layer_update_coord(struct 
> > sun8i_mixer *mixer, int channel,
> >*/
> >   subsampled = format->hsub > 1 || format->vsub > 1;
> >
> > - if (insize != outsize || subsampled || hphase || vphase) {
> > + if (insize != outsize || subsampled || hphase || vphase || 
> > sun8i_vi_keep_scaler_enabled) {
> >   unsigned int scanline, required;
> >   struct drm_display_mode *mode;
> >   u32 hscale, vscale, fps;
> > --
> > 2.30.2
> >

Re: [PATCH v5 2/6] drm/v3d: Get rid of pm code

2022-06-03 Thread nicolas saenz julienne

On Wed, 2022-06-01 at 12:02 +0100, Peter Robinson wrote:
> Runtime PM doesn't seem to work correctly on this driver. On top of
> that, commit 8b6864e3e138 ("drm/v3d/v3d_drv: Remove unused static
> variable 'v3d_v3d_pm_ops'") hints that it most likely never did as the
> driver's PM ops were not hooked-up.
> 
> So, in order to support regular operation with V3D on BCM2711 (Raspberry
> Pi 4), get rid of the PM code. PM will be reinstated once we figure out
> the underlying issues.
> 
> Signed-off-by: Nicolas Saenz Julienne 
> Signed-off-by: Peter Robinson 
> ---

Hi Florian, any thoughts on this?
Are brcm,7268-v3d and brcm,7278-v3d using runtime PM downstream?

Nicolas

Re: [PATCH 2/3] drm/msm/dpu: Add MISR register support for interface

On Fri, 3 Jun 2022 at 04:00, Jessica Zhang  wrote:
>
>
>
> On 6/2/2022 3:31 PM, Dmitry Baryshkov wrote:
> > On 27/05/2022 23:11, Jessica Zhang wrote:
> >>
> >>
> >> On 5/27/2022 12:38 PM, Dmitry Baryshkov wrote:
> >>> On 27/05/2022 21:54, Jessica Zhang wrote:
>  Add support for setting MISR registers within the interface
> 
>  Signed-off-by: Jessica Zhang 
>  ---
>    drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 55
>  -
>    drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h |  8 ++-
>    2 files changed, 61 insertions(+), 2 deletions(-)
> 
>  diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>  b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>  index 3f4d2c6e1b45..29aaeff9eacd 100644
>  --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>  +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>  @@ -1,5 +1,7 @@
>    // SPDX-License-Identifier: GPL-2.0-only
>  -/* Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.
>  +/*
>  + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights
>  reserved.
>  + * Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.
> */
>    #include "dpu_hwio.h"
>  @@ -67,6 +69,14 @@
>    #define INTF_CFG2_DATABUS_WIDENBIT(0)
>    #define INTF_CFG2_DATA_HCTL_ENBIT(4)
>  +#define INTF_MISR_CTRL0x180
>  +#define INTF_MISR_SIGNATURE0x184
>  +#define INTF_MISR_FRAME_COUNT_MASK0xFF
>  +#define INTF_MISR_CTRL_ENABLEBIT(8)
>  +#define INTF_MISR_CTRL_STATUSBIT(9)
>  +#define INTF_MISR_CTRL_STATUS_CLEARBIT(10)
>  +#define INTF_MISR_CTRL_FREE_RUN_MASKBIT(31)
> >>>
> >>> I'm tempted to ask to move these bits to some common header. Is there
> >>> any other hardware block which uses the same bitfields to control MISR?
> >>
> >> dpu_hw_lm.c has similar macros here [1] for _ENABLE, _STATUS,
> >> _STATUS_CLEAR, and _FREE_RUN_MASK
> >>
> >> [1]
> >> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c#L31
> >
> >
> > Please move bit definitions to dpu_hw_util.h. According to what I
> > observe, this might be useful.
>
> Noted.
>
> >
>  +
>    static const struct dpu_intf_cfg *_intf_offset(enum dpu_intf intf,
>    const struct dpu_mdss_cfg *m,
>    void __iomem *addr,
>  @@ -319,6 +329,47 @@ static u32 dpu_hw_intf_get_line_count(struct
>  dpu_hw_intf *intf)
>    return DPU_REG_READ(c, INTF_LINE_COUNT);
>    }
>  +static void dpu_hw_intf_setup_misr(struct dpu_hw_intf *intf, bool
>  enable, u32 frame_count)
>  +{
>  +struct dpu_hw_blk_reg_map *c = >hw;
>  +u32 config = 0;
>  +
>  +DPU_REG_WRITE(c, INTF_MISR_CTRL, INTF_MISR_CTRL_STATUS_CLEAR);
>  +
>  +/* Clear old MISR value (in case it's read before a new value
>  is calculated)*/
>  +wmb();
>  +
>  +if (enable) {
>  +config = (frame_count & INTF_MISR_FRAME_COUNT_MASK) |
>  +INTF_MISR_CTRL_ENABLE | INTF_MISR_CTRL_FREE_RUN_MASK;
>  +
>  +DPU_REG_WRITE(c, INTF_MISR_CTRL, config);
>  +} else {
>  +DPU_REG_WRITE(c, INTF_MISR_CTRL, 0);
>  +}
> >
> > This should also be abstracted. Please merge these functions with LM's
> > and add corresponding helpers to dpu_hw_util.c
>
> Good idea.
>
> >
>  +}
>  +
>  +static int dpu_hw_intf_collect_misr(struct dpu_hw_intf *intf, u32
>  *misr_value)
>  +{
>  +struct dpu_hw_blk_reg_map *c = >hw;
>  +u32 ctrl = 0;
>  +
>  +if (!misr_value)
>  +return -EINVAL;
>  +
>  +ctrl = DPU_REG_READ(c, INTF_MISR_CTRL);
>  +
>  +if (!(ctrl & INTF_MISR_CTRL_ENABLE))
>  +return -ENODATA;
> >
> > As the users of collect_misr() are going to ignore the -ENODATA, I think
> > it would be worth changing this to set *misr_value to 0 and return 0
> > here. It would reduce the amount of boilerplate code.
>
> 0 might be a valid misr_value so it might be better to not write it to
> the debugfs. IMO we should also avoid writing to the debugfs if the misr
> is disabled -- that way we won't be spamming the debugfs with
> meaningless entries.

Ack, true. Then I'd ask to change encoder code to skip crcs entries
corresponding to the phys_encs with no hw_intf bound.


-- 
With best wishes
Dmitry

Re: [Freedreno] [PATCH 3/3] drm/msm/dpu: Add interface support for CRC debugfs