[PATCH/RFT v1 0/6] drm/via: drop use of deprecated headers drmP.h and drm_os_linux.h

[PATCH/RFT v1 0/6] drm/via: drop use of deprecated headers drmP.h and drm_os_linux.h

[Sam Ravnborg]

This is some janitorial updates to the via driver
that is required to get rid of deprecated headers
in the drm subsystem.

The first three patches are trivial, where
the dependencies on drmP.h and drm_os_linux are dropped.

The remaining three patches drop use of DRM_WAIT_ON().
They are replaced by wait_event_interruptible_timeout().
These patches could use a more critical review.

When replacing DRM_WAIT_ON() I took care to not change the uapi,
by continue to use the original return codes.

The following table was used for the transition:

               DRM_WAIT_ON   wait_event_interruptible_timeout
               -----------  ---------------------------------
condition OK:    ret                   >= 1
timeout:        -EBUSY                  0
interrupted:    -EINTR             -ERESTARTSYS

The changes has been build tested only.
Testing on a real device would be highly appreciated.

I had preferred that the via driver was replaced by the
openchrome driver, but until we have it then we need
to deal with the legacy via driver to remove old cruft
in the drm subsystem.

Note: A simpler approach had been to copy DRM_WAIT_ON
to via_drv.h, but then the actual solution is
presumeably better.

	Sam


Sam Ravnborg (6):
      drm/via: drop use of DRM(READ|WRITE) macros
      drm/via: make via_drv.h self-contained
      drm/via: drop use of drmP.h
      drm/via: drop DRM_WAIT_ON() in via_dmablit.c
      drm/via: drop DRM_WAIT_ON() in via_irq
      drm/via: drop DRM_WAIT_ON() in via_video

 drivers/gpu/drm/via/via_dma.c      |  9 +++++-
 drivers/gpu/drm/via/via_dmablit.c  | 66 +++++++++++++++++++++++++++-----------
 drivers/gpu/drm/via/via_drv.c      |  7 ++--
 drivers/gpu/drm/via/via_drv.h      | 21 +++++++++---
 drivers/gpu/drm/via/via_irq.c      | 37 +++++++++++++++------
 drivers/gpu/drm/via/via_map.c      |  6 +++-
 drivers/gpu/drm/via/via_mm.c       |  7 +++-
 drivers/gpu/drm/via/via_verifier.c | 10 +++---
 drivers/gpu/drm/via/via_video.c    | 23 ++++++++++---
 9 files changed, 137 insertions(+), 49 deletions(-)


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

[PATCH v1 1/6] drm/via: drop use of DRM(READ|WRITE) macros

[Sam Ravnborg]

The DRM_READ, DRM_WRITE macros comes from the deprecated drm_os_linux.h
header file. Remove their use to remove this dependency.

Replace the use of the macros with open coded variants.

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
Cc: Kevin Brace <kevinbrace@gmx.com>
Cc: Thomas Hellstrom <thellstrom@vmware.com>
Cc: "Gustavo A. R. Silva" <gustavo@embeddedor.com>
Cc: Mike Marshall <hubcap@omnibond.com>
Cc: Ira Weiny <ira.weiny@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Emil Velikov <emil.velikov@collabora.com>
---
 drivers/gpu/drm/via/via_drv.h | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/via/via_drv.h b/drivers/gpu/drm/via/via_drv.h
index 6d1ae834484c..d5a2b1ffd8c1 100644
--- a/drivers/gpu/drm/via/via_drv.h
+++ b/drivers/gpu/drm/via/via_drv.h
@@ -115,10 +115,17 @@ enum via_family {
 /* VIA MMIO register access */
 #define VIA_BASE ((dev_priv->mmio))
 
-#define VIA_READ(reg)		DRM_READ32(VIA_BASE, reg)
-#define VIA_WRITE(reg, val)	DRM_WRITE32(VIA_BASE, reg, val)
-#define VIA_READ8(reg)		DRM_READ8(VIA_BASE, reg)
-#define VIA_WRITE8(reg, val)	DRM_WRITE8(VIA_BASE, reg, val)
+#define VIA_READ(reg) \
+	readl(((void __iomem *)VIA_BASE->handle) + (reg))
+
+#define VIA_WRITE(reg, val) \
+	writel(val, ((void __iomem *)VIA_BASE->handle) + (reg))
+
+#define VIA_READ8(reg) \
+	readb(((void __iomem *)VIA_BASE->handle) + (reg))
+
+#define VIA_WRITE8(reg, val) \
+	writeb(val, ((void __iomem *)VIA_BASE->handle) + (reg))
 
 extern const struct drm_ioctl_desc via_ioctls[];
 extern int via_max_ioctl;
-- 
2.20.1

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

[PATCH v1 2/6] drm/via: make via_drv.h self-contained

[Sam Ravnborg]

Added the necessary header files to make this header file
self-contained.

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
Cc: Kevin Brace <kevinbrace@gmx.com>
Cc: Thomas Hellstrom <thellstrom@vmware.com>
Cc: "Gustavo A. R. Silva" <gustavo@embeddedor.com>
Cc: Mike Marshall <hubcap@omnibond.com>
Cc: Ira Weiny <ira.weiny@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Emil Velikov <emil.velikov@collabora.com>
---
 drivers/gpu/drm/via/via_drv.h | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/via/via_drv.h b/drivers/gpu/drm/via/via_drv.h
index d5a2b1ffd8c1..f36ac1134424 100644
--- a/drivers/gpu/drm/via/via_drv.h
+++ b/drivers/gpu/drm/via/via_drv.h
@@ -24,8 +24,12 @@
 #ifndef _VIA_DRV_H_
 #define _VIA_DRV_H_
 
-#include <drm/drm_mm.h>
+#include <linux/irqreturn.h>
+
+#include <drm/drm_ioctl.h>
 #include <drm/drm_legacy.h>
+#include <drm/drm_mm.h>
+#include <drm/via_drm.h>
 
 #define DRIVER_AUTHOR	"Various"
 
-- 
2.20.1

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

[PATCH v1 3/6] drm/via: drop use of drmP.h

[Sam Ravnborg]

Drop use of the deprecated drmP.h header.
While touching the files divide include files in blocks
and sort the files alphabetically.

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
Cc: Kevin Brace <kevinbrace@gmx.com>
Cc: Thomas Hellstrom <thellstrom@vmware.com>
Cc: "Gustavo A. R. Silva" <gustavo@embeddedor.com>
Cc: Mike Marshall <hubcap@omnibond.com>
Cc: Ira Weiny <ira.weiny@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Emil Velikov <emil.velikov@collabora.com>
---
 drivers/gpu/drm/via/via_dma.c      |  9 ++++++++-
 drivers/gpu/drm/via/via_dmablit.c  | 14 +++++++++-----
 drivers/gpu/drm/via/via_drv.c      |  7 +++++--
 drivers/gpu/drm/via/via_irq.c      |  5 ++++-
 drivers/gpu/drm/via/via_map.c      |  6 +++++-
 drivers/gpu/drm/via/via_mm.c       |  7 ++++++-
 drivers/gpu/drm/via/via_verifier.c | 10 +++++-----
 drivers/gpu/drm/via/via_video.c    |  4 +++-
 8 files changed, 45 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/via/via_dma.c b/drivers/gpu/drm/via/via_dma.c
index d17d8f245c1a..4e50834dd222 100644
--- a/drivers/gpu/drm/via/via_dma.c
+++ b/drivers/gpu/drm/via/via_dma.c
@@ -34,8 +34,15 @@
  *    Thomas Hellstrom.
  */
 
-#include <drm/drmP.h>
+#include <linux/delay.h>
+#include <linux/uaccess.h>
+
+#include <drm/drm.h>
+#include <drm/drm_agpsupport.h>
+#include <drm/drm_device.h>
+#include <drm/drm_file.h>
 #include <drm/via_drm.h>
+
 #include "via_drv.h"
 #include "via_3d_reg.h"
 
diff --git a/drivers/gpu/drm/via/via_dmablit.c b/drivers/gpu/drm/via/via_dmablit.c
index 062067438f1d..87d88bdd20c6 100644
--- a/drivers/gpu/drm/via/via_dmablit.c
+++ b/drivers/gpu/drm/via/via_dmablit.c
@@ -34,13 +34,17 @@
  * the same DMA mappings?
  */
 
-#include <drm/drmP.h>
-#include <drm/via_drm.h>
-#include "via_drv.h"
-#include "via_dmablit.h"
-
 #include <linux/pagemap.h>
 #include <linux/slab.h>
+#include <linux/vmalloc.h>
+
+#include <drm/drm_device.h>
+#include <drm/drm_os_linux.h>
+#include <drm/drm_pci.h>
+#include <drm/via_drm.h>
+
+#include "via_dmablit.h"
+#include "via_drv.h"
 
 #define VIA_PGDN(x)	     (((unsigned long)(x)) & PAGE_MASK)
 #define VIA_PGOFF(x)	    (((unsigned long)(x)) & ~PAGE_MASK)
diff --git a/drivers/gpu/drm/via/via_drv.c b/drivers/gpu/drm/via/via_drv.c
index af6a12d3c058..666a16de84f9 100644
--- a/drivers/gpu/drm/via/via_drv.c
+++ b/drivers/gpu/drm/via/via_drv.c
@@ -24,11 +24,14 @@
 
 #include <linux/module.h>
 
-#include <drm/drmP.h>
+#include <drm/drm_drv.h>
+#include <drm/drm_file.h>
+#include <drm/drm_pci.h>
+#include <drm/drm_pciids.h>
 #include <drm/via_drm.h>
+
 #include "via_drv.h"
 
-#include <drm/drm_pciids.h>
 
 static int via_driver_open(struct drm_device *dev, struct drm_file *file)
 {
diff --git a/drivers/gpu/drm/via/via_irq.c b/drivers/gpu/drm/via/via_irq.c
index c96830ccc0ec..9d47feb367d8 100644
--- a/drivers/gpu/drm/via/via_irq.c
+++ b/drivers/gpu/drm/via/via_irq.c
@@ -35,8 +35,11 @@
  * The refresh rate is also calculated for video playback sync purposes.
  */
 
-#include <drm/drmP.h>
+#include <drm/drm_device.h>
+#include <drm/drm_os_linux.h>
+#include <drm/drm_vblank.h>
 #include <drm/via_drm.h>
+
 #include "via_drv.h"
 
 #define VIA_REG_INTERRUPT       0x200
diff --git a/drivers/gpu/drm/via/via_map.c b/drivers/gpu/drm/via/via_map.c
index 2ad865870372..431c150df014 100644
--- a/drivers/gpu/drm/via/via_map.c
+++ b/drivers/gpu/drm/via/via_map.c
@@ -21,8 +21,12 @@
  * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
  * DEALINGS IN THE SOFTWARE.
  */
-#include <drm/drmP.h>
+
+#include <drm/drm_device.h>
+#include <drm/drm_pci.h>
+#include <drm/drm_vblank.h>
 #include <drm/via_drm.h>
+
 #include "via_drv.h"
 
 static int via_do_init_map(struct drm_device *dev, drm_via_init_t *init)
diff --git a/drivers/gpu/drm/via/via_mm.c b/drivers/gpu/drm/via/via_mm.c
index 4217d66a5cc6..45cc9e900260 100644
--- a/drivers/gpu/drm/via/via_mm.c
+++ b/drivers/gpu/drm/via/via_mm.c
@@ -25,8 +25,13 @@
  * Authors: Thomas Hellström <thomas-at-tungstengraphics-dot-com>
  */
 
-#include <drm/drmP.h>
+#include <linux/slab.h>
+
+#include <drm/drm_device.h>
+#include <drm/drm_file.h>
+#include <drm/drm_irq.h>
 #include <drm/via_drm.h>
+
 #include "via_drv.h"
 
 #define VIA_MM_ALIGN_SHIFT 4
diff --git a/drivers/gpu/drm/via/via_verifier.c b/drivers/gpu/drm/via/via_verifier.c
index fb2609434df7..361a450058f2 100644
--- a/drivers/gpu/drm/via/via_verifier.c
+++ b/drivers/gpu/drm/via/via_verifier.c
@@ -28,13 +28,13 @@
  * be very slow.
  */
 
-#include "via_3d_reg.h"
-#include <drm/drmP.h>
-#include <drm/via_drm.h>
+#include <drm/drm_device.h>
 #include <drm/drm_legacy.h>
-#include "via_verifier.h"
+#include <drm/via_drm.h>
+
+#include "via_3d_reg.h"
 #include "via_drv.h"
-#include <linux/kernel.h>
+#include "via_verifier.h"
 
 typedef enum {
 	state_command,
diff --git a/drivers/gpu/drm/via/via_video.c b/drivers/gpu/drm/via/via_video.c
index a9ffbad1cfdd..4e165b1b0b18 100644
--- a/drivers/gpu/drm/via/via_video.c
+++ b/drivers/gpu/drm/via/via_video.c
@@ -25,8 +25,10 @@
  * Video and XvMC related functions.
  */
 
-#include <drm/drmP.h>
+#include <drm/drm_device.h>
+#include <drm/drm_os_linux.h>
 #include <drm/via_drm.h>
+
 #include "via_drv.h"
 
 void via_init_futex(drm_via_private_t *dev_priv)
-- 
2.20.1

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

[PATCH v1 4/6] drm/via: drop DRM_WAIT_ON() in via_dmablit.c

[Sam Ravnborg]

DRM_WAIT_ON() is a reliec from the past and is discouraged.
Use the standard wait_event_*() as replacement.

Be carefull to keep the same return values.

via_dma_blit_sync() changed -EINTR to -EAGAIN.
Moved this to via_dmablit_sync() so return value is
adjusted only once.

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
Cc: Kevin Brace <kevinbrace@gmx.com>
Cc: Thomas Hellstrom <thellstrom@vmware.com>
Cc: "Gustavo A. R. Silva" <gustavo@embeddedor.com>
Cc: Mike Marshall <hubcap@omnibond.com>
Cc: Ira Weiny <ira.weiny@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Emil Velikov <emil.velikov@collabora.com>
---
 drivers/gpu/drm/via/via_dmablit.c | 54 ++++++++++++++++++++++---------
 1 file changed, 39 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/via/via_dmablit.c b/drivers/gpu/drm/via/via_dmablit.c
index 87d88bdd20c6..27e2a6411502 100644
--- a/drivers/gpu/drm/via/via_dmablit.c
+++ b/drivers/gpu/drm/via/via_dmablit.c
@@ -39,7 +39,6 @@
 #include <linux/vmalloc.h>
 
 #include <drm/drm_device.h>
-#include <drm/drm_os_linux.h>
 #include <drm/drm_pci.h>
 #include <drm/via_drm.h>
 
@@ -428,8 +427,12 @@ via_dmablit_active(drm_via_blitq_t *blitq, int engine, uint32_t handle, wait_que
 
 /*
  * Sync. Wait for at least three seconds for the blit to be performed.
+ *
+ * Returns:
+ * 0 if blit was performed within the three seconds period
+ * -EBUSY if timeout occured
+ * -EAGAIN if a signal interrupted the wait
  */
-
 static int
 via_dmablit_sync(struct drm_device *dev, uint32_t handle, int engine)
 {
@@ -437,16 +440,26 @@ via_dmablit_sync(struct drm_device *dev, uint32_t handle, int engine)
 	drm_via_private_t *dev_priv = (drm_via_private_t *)dev->dev_private;
 	drm_via_blitq_t *blitq = dev_priv->blit_queues + engine;
 	wait_queue_head_t *queue;
-	int ret = 0;
+	int ret = 1;
 
 	if (via_dmablit_active(blitq, engine, handle, &queue)) {
-		DRM_WAIT_ON(ret, *queue, 3 * HZ,
-			    !via_dmablit_active(blitq, engine, handle, NULL));
+		ret = wait_event_interruptible_timeout(*queue,
+			!via_dmablit_active(blitq, engine, handle, NULL),
+			msecs_to_jiffies(3 * 1000));
 	}
 	DRM_DEBUG("DMA blit sync handle 0x%x engine %d returned %d\n",
 		  handle, engine, ret);
 
-	return ret;
+	switch (ret) {
+	case 0:
+		/* timeout */
+		return -EBUSY;
+	case -ERESTARTSYS:
+		/* interrupted by signal */
+		return -EAGAIN;
+	default:
+		return 0;
+	}
 }
 
 
@@ -677,13 +690,17 @@ via_build_sg_info(struct drm_device *dev, drm_via_sg_info_t *vsg, drm_via_dmabli
 
 /*
  * Reserve one free slot in the blit queue. Will wait for one second for one
- * to become available. Otherwise -EBUSY is returned.
+ * to become available.
+ *
+ * Returns:
+ * 0 if slot was reserved
+ * -EBUSY if timeout while waiting for free slot
+ * -EAGAIN if interrupted by a signal
  */
-
 static int
 via_dmablit_grab_slot(drm_via_blitq_t *blitq, int engine)
 {
-	int ret = 0;
+	int ret;
 	unsigned long irqsave;
 
 	DRM_DEBUG("Num free is %d\n", blitq->num_free);
@@ -691,9 +708,19 @@ via_dmablit_grab_slot(drm_via_blitq_t *blitq, int engine)
 	while (blitq->num_free == 0) {
 		spin_unlock_irqrestore(&blitq->blit_lock, irqsave);
 
-		DRM_WAIT_ON(ret, blitq->busy_queue, HZ, blitq->num_free > 0);
-		if (ret)
-			return (-EINTR == ret) ? -EAGAIN : ret;
+		ret = wait_event_interruptible_timeout(blitq->busy_queue,
+						       blitq->num_free > 0,
+						       msecs_to_jiffies(1000));
+		switch (ret) {
+		case 0:
+			/* timeout */
+			return -EBUSY;
+		case -ERESTARTSYS:
+			/* interrupted by signal */
+			return -EAGAIN;
+		default:
+			return 0;
+		}
 
 		spin_lock_irqsave(&blitq->blit_lock, irqsave);
 	}
@@ -786,9 +813,6 @@ via_dma_blit_sync(struct drm_device *dev, void *data, struct drm_file *file_priv
 
 	err = via_dmablit_sync(dev, sync->sync_handle, sync->engine);
 
-	if (-EINTR == err)
-		err = -EAGAIN;
-
 	return err;
 }
 
-- 
2.20.1

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

[PATCH v1 5/6] drm/via: drop DRM_WAIT_ON() in via_irq

[Sam Ravnborg]

Replace DRM_WAIT_ON() with wait_event_interruptible().
While replacing be careful to keep same return value semantics.

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
Cc: Kevin Brace <kevinbrace@gmx.com>
Cc: Thomas Hellstrom <thellstrom@vmware.com>
Cc: "Gustavo A. R. Silva" <gustavo@embeddedor.com>
Cc: Mike Marshall <hubcap@omnibond.com>
Cc: Ira Weiny <ira.weiny@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Emil Velikov <emil.velikov@collabora.com>
---
 drivers/gpu/drm/via/via_irq.c | 34 ++++++++++++++++++++++++----------
 1 file changed, 24 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/via/via_irq.c b/drivers/gpu/drm/via/via_irq.c
index 9d47feb367d8..6de15065a3c0 100644
--- a/drivers/gpu/drm/via/via_irq.c
+++ b/drivers/gpu/drm/via/via_irq.c
@@ -36,7 +36,6 @@
  */
 
 #include <drm/drm_device.h>
-#include <drm/drm_os_linux.h>
 #include <drm/drm_vblank.h>
 #include <drm/via_drm.h>
 
@@ -201,6 +200,12 @@ void via_disable_vblank(struct drm_device *dev, unsigned int pipe)
 		DRM_ERROR("%s:  bad crtc %u\n", __func__, pipe);
 }
 
+/*
+ * Returns:
+ * 0 if interrupt occured within 3 secs
+ * -EBUSY if timeout happended
+ * -EINTR if interrupted by a signal
+ */
 static int
 via_driver_irq_wait(struct drm_device *dev, unsigned int irq, int force_sequence,
 		    unsigned int *sequence)
@@ -208,7 +213,7 @@ via_driver_irq_wait(struct drm_device *dev, unsigned int irq, int force_sequence
 	drm_via_private_t *dev_priv = (drm_via_private_t *) dev->dev_private;
 	unsigned int cur_irq_sequence;
 	drm_via_irq_t *cur_irq;
-	int ret = 0;
+	int ret;
 	maskarray_t *masks;
 	int real_irq;
 
@@ -236,18 +241,27 @@ via_driver_irq_wait(struct drm_device *dev, unsigned int irq, int force_sequence
 	cur_irq = dev_priv->via_irqs + real_irq;
 
 	if (masks[real_irq][2] && !force_sequence) {
-		DRM_WAIT_ON(ret, cur_irq->irq_queue, 3 * HZ,
-			    ((VIA_READ(masks[irq][2]) & masks[irq][3]) ==
-			     masks[irq][4]));
+		ret = wait_event_interruptible_timeout(cur_irq->irq_queue,
+			((VIA_READ(masks[irq][2]) & masks[irq][3]) == masks[irq][4]),
+			msecs_to_jiffies(3000));
 		cur_irq_sequence = atomic_read(&cur_irq->irq_received);
 	} else {
-		DRM_WAIT_ON(ret, cur_irq->irq_queue, 3 * HZ,
-			    (((cur_irq_sequence =
-			       atomic_read(&cur_irq->irq_received)) -
-			      *sequence) <= (1 << 23)));
+		ret = wait_event_interruptible_timeout(cur_irq->irq_queue,
+			(((cur_irq_sequence = atomic_read(&cur_irq->irq_received)) - *sequence) <= (1 << 23)),
+			msecs_to_jiffies(3000));
 	}
 	*sequence = cur_irq_sequence;
-	return ret;
+
+	switch (ret) {
+	case 0:
+		/* timeout */
+		return -EBUSY;
+	case -ERESTARTSYS:
+		/* interrupted by signal */
+		return -EINTR;
+	default:
+		return 0;
+	}
 }
 
 
-- 
2.20.1

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

[PATCH v1 6/6] drm/via: drop DRM_WAIT_ON() in via_video

[Sam Ravnborg]

Replace DRM_WAIT_ON() with wait_event_interruptible().
Be careful to keep same return value semantics

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
Cc: Kevin Brace <kevinbrace@gmx.com>
Cc: Thomas Hellstrom <thellstrom@vmware.com>
Cc: "Gustavo A. R. Silva" <gustavo@embeddedor.com>
Cc: Mike Marshall <hubcap@omnibond.com>
Cc: Ira Weiny <ira.weiny@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Emil Velikov <emil.velikov@collabora.com>
---
 drivers/gpu/drm/via/via_video.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/via/via_video.c b/drivers/gpu/drm/via/via_video.c
index 4e165b1b0b18..1f88180affef 100644
--- a/drivers/gpu/drm/via/via_video.c
+++ b/drivers/gpu/drm/via/via_video.c
@@ -26,7 +26,6 @@
  */
 
 #include <drm/drm_device.h>
-#include <drm/drm_os_linux.h>
 #include <drm/via_drm.h>
 
 #include "via_drv.h"
@@ -73,7 +72,7 @@ int via_decoder_futex(struct drm_device *dev, void *data, struct drm_file *file_
 	volatile int *lock;
 	drm_via_private_t *dev_priv = (drm_via_private_t *) dev->dev_private;
 	drm_via_sarea_t *sAPriv = dev_priv->sarea_priv;
-	int ret = 0;
+	int ret;
 
 	DRM_DEBUG("\n");
 
@@ -84,9 +83,21 @@ int via_decoder_futex(struct drm_device *dev, void *data, struct drm_file *file_
 
 	switch (fx->func) {
 	case VIA_FUTEX_WAIT:
-		DRM_WAIT_ON(ret, dev_priv->decoder_queue[fx->lock],
-			    (fx->ms / 10) * (HZ / 100), *lock != fx->val);
-		return ret;
+		ret = wait_event_interruptible_timeout(
+				dev_priv->decoder_queue[fx->lock],
+				*lock != fx->val,
+				msecs_to_jiffies(fx->ms));
+		switch (ret) {
+		case 0:
+			/* timeout */
+			return -EBUSY;
+		case -ERESTARTSYS:
+			/* interrupted by signal */
+			return -EINTR;
+		default:
+			return 0;
+		}
+
 	case VIA_FUTEX_WAKE:
 		wake_up(&(dev_priv->decoder_queue[fx->lock]));
 		return 0;
-- 
2.20.1

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

Re: [PATCH/RFT v1 0/6] drm/via: drop use of deprecated headers drmP.h and drm_os_linux.h

[Sam Ravnborg]

On Thu, Jul 18, 2019 at 05:37:31PM +0200, Sam Ravnborg wrote:
> This is some janitorial updates to the via driver
> that is required to get rid of deprecated headers
> in the drm subsystem.
> 
> The first three patches are trivial, where
> the dependencies on drmP.h and drm_os_linux are dropped.
> 
> The remaining three patches drop use of DRM_WAIT_ON().
> They are replaced by wait_event_interruptible_timeout().
> These patches could use a more critical review.

The differences between DRM_WAIT_ON() and
wait_event_interruptible_timeout() are bigger than anticipated.

The conversion I did for drm_vblank.c is bogus thus I expect
the conversion done for via is also bogus.
I will post a v2.

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

Re: [PATCH/RFT v1 0/6] drm/via: drop use of deprecated headers drmP.h and drm_os_linux.h

[Michel Dänzer]

On 2019-07-19 8:07 a.m., Sam Ravnborg wrote:
> On Thu, Jul 18, 2019 at 05:37:31PM +0200, Sam Ravnborg wrote:
>> This is some janitorial updates to the via driver
>> that is required to get rid of deprecated headers
>> in the drm subsystem.
>>
>> The first three patches are trivial, where
>> the dependencies on drmP.h and drm_os_linux are dropped.
>>
>> The remaining three patches drop use of DRM_WAIT_ON().
>> They are replaced by wait_event_interruptible_timeout().
>> These patches could use a more critical review.
> 
> The differences between DRM_WAIT_ON() and
> wait_event_interruptible_timeout() are bigger than anticipated.
> 
> The conversion I did for drm_vblank.c is bogus thus I expect
> the conversion done for via is also bogus.

What exactly is the problem though? Can you share information about the
failures you're seeing?

There was some discussion about DRM_WAIT_ON() "polling" on IRC. I assume
that refers to it only sleeping for up to 0.01s before checking the
condition again. In contrast, wait_event_interruptible_timeout() checks
the condition once, then sleeps up to the full timeout before checking
it again.

If that makes a difference for drm_wait_vblank_ioctl, it indicates that
some other code which updates the vblank count or clears vblank->enabled
doesn't wake up the vblank->queue.


-- 
Earthling Michel Dänzer               |              https://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH/RFT v1 0/6] drm/via: drop use of deprecated headers drmP.h and drm_os_linux.h

[Daniel Vetter]

On Fri, Jul 19, 2019 at 11:05:44AM +0200, Michel Dänzer wrote:
> On 2019-07-19 8:07 a.m., Sam Ravnborg wrote:
> > On Thu, Jul 18, 2019 at 05:37:31PM +0200, Sam Ravnborg wrote:
> >> This is some janitorial updates to the via driver
> >> that is required to get rid of deprecated headers
> >> in the drm subsystem.
> >>
> >> The first three patches are trivial, where
> >> the dependencies on drmP.h and drm_os_linux are dropped.
> >>
> >> The remaining three patches drop use of DRM_WAIT_ON().
> >> They are replaced by wait_event_interruptible_timeout().
> >> These patches could use a more critical review.
> > 
> > The differences between DRM_WAIT_ON() and
> > wait_event_interruptible_timeout() are bigger than anticipated.
> > 
> > The conversion I did for drm_vblank.c is bogus thus I expect
> > the conversion done for via is also bogus.
> 
> What exactly is the problem though? Can you share information about the
> failures you're seeing?
> 
> There was some discussion about DRM_WAIT_ON() "polling" on IRC. I assume
> that refers to it only sleeping for up to 0.01s before checking the
> condition again. In contrast, wait_event_interruptible_timeout() checks
> the condition once, then sleeps up to the full timeout before checking
> it again.
> 
> If that makes a difference for drm_wait_vblank_ioctl, it indicates that
> some other code which updates the vblank count or clears vblank->enabled
> doesn't wake up the vblank->queue.

The return values differ, latest patch from Sam passes CI:

https://patchwork.freedesktop.org/patch/318909/

I have vague memories from exactly this fun when I ripped out DRM_WAIT_ON
from i915.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH/RFT v1 0/6] drm/via: drop use of deprecated headers drmP.h and drm_os_linux.h

[Sam Ravnborg]

Hi Michael.

On Fri, Jul 19, 2019 at 11:05:44AM +0200, Michel Dänzer wrote:
> On 2019-07-19 8:07 a.m., Sam Ravnborg wrote:
> > On Thu, Jul 18, 2019 at 05:37:31PM +0200, Sam Ravnborg wrote:
> >> This is some janitorial updates to the via driver
> >> that is required to get rid of deprecated headers
> >> in the drm subsystem.
> >>
> >> The first three patches are trivial, where
> >> the dependencies on drmP.h and drm_os_linux are dropped.
> >>
> >> The remaining three patches drop use of DRM_WAIT_ON().
> >> They are replaced by wait_event_interruptible_timeout().
> >> These patches could use a more critical review.
> > 
> > The differences between DRM_WAIT_ON() and
> > wait_event_interruptible_timeout() are bigger than anticipated.
> > 
> > The conversion I did for drm_vblank.c is bogus thus I expect
> > the conversion done for via is also bogus.
> 
> What exactly is the problem though? Can you share information about the
> failures you're seeing?
> 
> There was some discussion about DRM_WAIT_ON() "polling" on IRC. I assume
> that refers to it only sleeping for up to 0.01s before checking the
> condition again. In contrast, wait_event_interruptible_timeout() checks
> the condition once, then sleeps up to the full timeout before checking
> it again.
Correct - it was based on the feedback on irc from airlied and ickle
that made me conclude that the via part may not be good.
I cannot say if the polling versus timeout is properly dealt with in the
via driver and I am inclided to just move DRM_WAIT_ON() to via_drv.h and
name it VIA_WAIT_ON().
Then the changes to this legacy driver is minimal and it will not
prevent us from gettting rid of drm_os_linux.h

> 
> If that makes a difference for drm_wait_vblank_ioctl, it indicates that
> some other code which updates the vblank count or clears vblank->enabled
> doesn't wake up the vblank->queue.
Let me analyse a little...

In drm_handle_vblank() there is a call to wake_up(&vblank->queue);
And this is called from an interrupt - OK.

drm_vblank_enable() is called outside an interrupt - no need for
wake_up()

drm_crtc_accurate_vblank_count() is called outside interrupt - no need
for wake_up()

drm_vblank_disable_and_save() is called outside interrupt - no need for
wake_up()'

That is all functions I could dig up that updates the vblank counter.

So based on this short analysis I actually start to think that
I can use the variant that uses wait_event_interruptible_timeout()
anyway.
I will post a v3 and await feedback on that version.

As for via - introducing VIA_WAIT_ON() is the simple solution.
So I will post a v2 of this series using VIA_WAIT_ON().

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

Re: [PATCH/RFT v1 0/6] drm/via: drop use of deprecated headers drmP.h and drm_os_linux.h

[Daniel Vetter]

On Fri, Jul 19, 2019 at 1:32 PM Sam Ravnborg <sam@ravnborg.org> wrote:
>
> Hi Michael.
>
> On Fri, Jul 19, 2019 at 11:05:44AM +0200, Michel Dänzer wrote:
> > On 2019-07-19 8:07 a.m., Sam Ravnborg wrote:
> > > On Thu, Jul 18, 2019 at 05:37:31PM +0200, Sam Ravnborg wrote:
> > >> This is some janitorial updates to the via driver
> > >> that is required to get rid of deprecated headers
> > >> in the drm subsystem.
> > >>
> > >> The first three patches are trivial, where
> > >> the dependencies on drmP.h and drm_os_linux are dropped.
> > >>
> > >> The remaining three patches drop use of DRM_WAIT_ON().
> > >> They are replaced by wait_event_interruptible_timeout().
> > >> These patches could use a more critical review.
> > >
> > > The differences between DRM_WAIT_ON() and
> > > wait_event_interruptible_timeout() are bigger than anticipated.
> > >
> > > The conversion I did for drm_vblank.c is bogus thus I expect
> > > the conversion done for via is also bogus.
> >
> > What exactly is the problem though? Can you share information about the
> > failures you're seeing?
> >
> > There was some discussion about DRM_WAIT_ON() "polling" on IRC. I assume
> > that refers to it only sleeping for up to 0.01s before checking the
> > condition again. In contrast, wait_event_interruptible_timeout() checks
> > the condition once, then sleeps up to the full timeout before checking
> > it again.
> Correct - it was based on the feedback on irc from airlied and ickle
> that made me conclude that the via part may not be good.
> I cannot say if the polling versus timeout is properly dealt with in the
> via driver and I am inclided to just move DRM_WAIT_ON() to via_drv.h and
> name it VIA_WAIT_ON().
> Then the changes to this legacy driver is minimal and it will not
> prevent us from gettting rid of drm_os_linux.h
>
> >
> > If that makes a difference for drm_wait_vblank_ioctl, it indicates that
> > some other code which updates the vblank count or clears vblank->enabled
> > doesn't wake up the vblank->queue.
> Let me analyse a little...
>
> In drm_handle_vblank() there is a call to wake_up(&vblank->queue);
> And this is called from an interrupt - OK.
>
> drm_vblank_enable() is called outside an interrupt - no need for
> wake_up()
>
> drm_crtc_accurate_vblank_count() is called outside interrupt - no need
> for wake_up()
>
> drm_vblank_disable_and_save() is called outside interrupt - no need for
> wake_up()'
>
> That is all functions I could dig up that updates the vblank counter.
>
> So based on this short analysis I actually start to think that
> I can use the variant that uses wait_event_interruptible_timeout()
> anyway.
> I will post a v3 and await feedback on that version.

Furthermore the new vblank sequence ioctls that Keith added don't use
DRM_WAIT_ON(), so maybe that ship sailed already.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH/RFT v1 0/6] drm/via: drop use of deprecated headers drmP.h and drm_os_linux.h

[Michel Dänzer]

On 2019-07-19 2:37 p.m., Daniel Vetter wrote:
> On Fri, Jul 19, 2019 at 1:32 PM Sam Ravnborg <sam@ravnborg.org> wrote:
>> On Fri, Jul 19, 2019 at 11:05:44AM +0200, Michel Dänzer wrote:
>>> On 2019-07-19 8:07 a.m., Sam Ravnborg wrote:
>>>> On Thu, Jul 18, 2019 at 05:37:31PM +0200, Sam Ravnborg wrote:
>>>>> This is some janitorial updates to the via driver
>>>>> that is required to get rid of deprecated headers
>>>>> in the drm subsystem.
>>>>>
>>>>> The first three patches are trivial, where
>>>>> the dependencies on drmP.h and drm_os_linux are dropped.
>>>>>
>>>>> The remaining three patches drop use of DRM_WAIT_ON().
>>>>> They are replaced by wait_event_interruptible_timeout().
>>>>> These patches could use a more critical review.
>>>>
>>>> The differences between DRM_WAIT_ON() and
>>>> wait_event_interruptible_timeout() are bigger than anticipated.
>>>>
>>>> The conversion I did for drm_vblank.c is bogus thus I expect
>>>> the conversion done for via is also bogus.
>>>
>>> What exactly is the problem though? Can you share information about the
>>> failures you're seeing?
>>>
>>> There was some discussion about DRM_WAIT_ON() "polling" on IRC. I assume
>>> that refers to it only sleeping for up to 0.01s before checking the
>>> condition again. In contrast, wait_event_interruptible_timeout() checks
>>> the condition once, then sleeps up to the full timeout before checking
>>> it again.
>> Correct - it was based on the feedback on irc from airlied and ickle
>> that made me conclude that the via part may not be good.
>> I cannot say if the polling versus timeout is properly dealt with in the
>> via driver and I am inclided to just move DRM_WAIT_ON() to via_drv.h and
>> name it VIA_WAIT_ON().
>> Then the changes to this legacy driver is minimal and it will not
>> prevent us from gettting rid of drm_os_linux.h
>>
>>>
>>> If that makes a difference for drm_wait_vblank_ioctl, it indicates that
>>> some other code which updates the vblank count or clears vblank->enabled
>>> doesn't wake up the vblank->queue.
>> Let me analyse a little...
>>
>> In drm_handle_vblank() there is a call to wake_up(&vblank->queue);
>> And this is called from an interrupt - OK.

I'm not sure why it's relevant whether or not a function can be called
from an interrupt handler.


>> drm_vblank_enable() is called outside an interrupt - no need for
>> wake_up()

There is no need here because there can be no sleeping waiters in the
queue, because vblank->enabled == false immediately terminates any waits.


>> drm_crtc_accurate_vblank_count() is called outside interrupt - no need
>> for wake_up()

This is called from interrupt handlers, at least from
amdgpu_dm.c:dm_pflip_high_irq(). Not sure it needs to wake up the queue
though, the driver should call drm_(crtc_)_handle_vblank anyway.


>> drm_vblank_disable_and_save() is called outside interrupt - no need for
>> wake_up()'

It can be called from an interrupt, via drm_handle_vblank ->
vblank_disable_fn. However, the only place where
drm_vblank_disable_and_save can be called with sleeping waiters in the
queue is in drm_crtc_vblank_off, which wakes up the queue afterwards
(which terminates all waits, because vblank->enabled == false at this
point).


>> That is all functions I could dig up that updates the vblank counter.

I agree, this should also cover everything which clears vblank->enabled.


So, are there still failures with v2 of the drm_wait_vblank_ioctl patch
(which I haven't seen after all BTW)? If yes, can you share information
about them? If not, why do you want to send a v3?


-- 
Earthling Michel Dänzer               |              https://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel