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

[PATCH v2 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 prepare for the removal of drmP.h.
The last patch remove use of drmP.h and replace with necessary
include files to fix build.

Build tested with various configs and various architectures.

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 when removing old cruft
in the drm subsystem.

v2:
- Add a copy of DRM_WAIT_ON to the via driver, keeping
  the changes to this legacy driver to a minimum.
  This also gives much more confidence that the
  driver continues to work as there is no changes
  in logic. Therefore dropped "RFT".
- Added Cc: Michel Dänzer <michel@daenzer.net> to all
  patches, as Michael have commented on the series.

	Sam

Sam Ravnborg (4):
      drm/via: drop use of DRM(READ|WRITE) macros
      drm/via: add VIA_WAIT_ON()
      drm/via: make via_drv.h self-contained
      drm/via: drop use of drmP.h

 drivers/gpu/drm/via/via_dma.c      |  9 +++++-
 drivers/gpu/drm/via/via_dmablit.c  | 17 +++++++-----
 drivers/gpu/drm/via/via_drv.c      |  7 +++--
 drivers/gpu/drm/via/via_drv.h      | 57 ++++++++++++++++++++++++++++++++++----
 drivers/gpu/drm/via/via_irq.c      |  8 ++++--
 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    |  5 ++--
 9 files changed, 99 insertions(+), 27 deletions(-)


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

[PATCH v2 1/4] 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>
Cc: Michel Dänzer <michel@daenzer.net>
---
 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 v2 2/4] drm/via: add VIA_WAIT_ON()

[Sam Ravnborg]

VIA_WAIT_ON() is a direct copy of DRM_WAIT_ON() from
drm_os_linux.h.
The copy is made so we can avoid the dependency on the legacy header.
A more involved approach had been to introduce wait_event_* but for this
legacy driver the simpler and more safe approach with a copy of the
macro was selected.
Added the relevant header files for the functions used in VIA_WAIT_ON.

Users of the macro will come in a follow-up patch.

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>
Cc: Michel Dänzer <michel@daenzer.net>
---
 drivers/gpu/drm/via/via_drv.h | 42 ++++++++++++++++++++++++++++++++++-
 1 file changed, 41 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/via/via_drv.h b/drivers/gpu/drm/via/via_drv.h
index d5a2b1ffd8c1..664b7f8a20c4 100644
--- a/drivers/gpu/drm/via/via_drv.h
+++ b/drivers/gpu/drm/via/via_drv.h
@@ -24,8 +24,13 @@
 #ifndef _VIA_DRV_H_
 #define _VIA_DRV_H_
 
-#include <drm/drm_mm.h>
+#include <linux/jiffies.h>
+#include <linux/sched.h>
+#include <linux/sched/signal.h>
+#include <linux/wait.h>
+
 #include <drm/drm_legacy.h>
+#include <drm/drm_mm.h>
 
 #define DRIVER_AUTHOR	"Various"
 
@@ -127,6 +132,41 @@ enum via_family {
 #define VIA_WRITE8(reg, val) \
 	writeb(val, ((void __iomem *)VIA_BASE->handle) + (reg))
 
+/*
+ * Poll in a loop waiting for 'contidition' to be true.
+ * Note: A direct replacement with wait_event_interruptible_timeout()
+ *       will not work unless driver is updated to emit wake_up()
+ *       in relevant places that can impact the 'condition'
+ *
+ * Returns:
+ *   ret keeps current value if 'condition' becomes true
+ *   ret = -BUSY if timeout happens
+ *   ret = -EINTR if a signal interrupted the waiting period
+ */
+#define VIA_WAIT_ON( ret, queue, timeout, condition )		\
+do {								\
+	DECLARE_WAITQUEUE(entry, current);			\
+	unsigned long end = jiffies + (timeout);		\
+	add_wait_queue(&(queue), &entry);			\
+								\
+	for (;;) {						\
+		__set_current_state(TASK_INTERRUPTIBLE);	\
+		if (condition)					\
+			break;					\
+		if (time_after_eq(jiffies, end)) {		\
+			ret = -EBUSY;				\
+			break;					\
+		}						\
+		schedule_timeout((HZ/100 > 1) ? HZ/100 : 1);	\
+		if (signal_pending(current)) {			\
+			ret = -EINTR;				\
+			break;					\
+		}						\
+	}							\
+	__set_current_state(TASK_RUNNING);			\
+	remove_wait_queue(&(queue), &entry);			\
+} while (0)
+
 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 v2 3/4] drm/via: make via_drv.h self-contained

[Sam Ravnborg]

Added include of header file 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>
Cc: Michel Dänzer <michel@daenzer.net>
---
 drivers/gpu/drm/via/via_drv.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/via/via_drv.h b/drivers/gpu/drm/via/via_drv.h
index 664b7f8a20c4..a58af0ad5108 100644
--- a/drivers/gpu/drm/via/via_drv.h
+++ b/drivers/gpu/drm/via/via_drv.h
@@ -24,13 +24,16 @@
 #ifndef _VIA_DRV_H_
 #define _VIA_DRV_H_
 
+#include <linux/irqreturn.h>
 #include <linux/jiffies.h>
 #include <linux/sched.h>
 #include <linux/sched/signal.h>
 #include <linux/wait.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 v2 4/4] 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.

v2:
- Replace all uses of DRM_WAIT_ON() with VIA_WAIT_ON()
  and thus avoiding to pull in drm_os_linux.h

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>
Cc: Michel Dänzer <michel@daenzer.net>
---
 drivers/gpu/drm/via/via_dma.c      |  9 ++++++++-
 drivers/gpu/drm/via/via_dmablit.c  | 17 ++++++++++-------
 drivers/gpu/drm/via/via_drv.c      |  7 +++++--
 drivers/gpu/drm/via/via_irq.c      |  8 +++++---
 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    |  5 +++--
 8 files changed, 47 insertions(+), 22 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..9fbedc488e62 100644
--- a/drivers/gpu/drm/via/via_dmablit.c
+++ b/drivers/gpu/drm/via/via_dmablit.c
@@ -34,13 +34,16 @@
  * 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_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)
@@ -436,7 +439,7 @@ via_dmablit_sync(struct drm_device *dev, uint32_t handle, int engine)
 	int ret = 0;
 
 	if (via_dmablit_active(blitq, engine, handle, &queue)) {
-		DRM_WAIT_ON(ret, *queue, 3 * HZ,
+		VIA_WAIT_ON(ret, *queue, 3 * HZ,
 			    !via_dmablit_active(blitq, engine, handle, NULL));
 	}
 	DRM_DEBUG("DMA blit sync handle 0x%x engine %d returned %d\n",
@@ -687,7 +690,7 @@ 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);
+		VIA_WAIT_ON(ret, blitq->busy_queue, HZ, blitq->num_free > 0);
 		if (ret)
 			return (-EINTR == ret) ? -EAGAIN : ret;
 
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..138c3f8d0af2 100644
--- a/drivers/gpu/drm/via/via_irq.c
+++ b/drivers/gpu/drm/via/via_irq.c
@@ -35,8 +35,10 @@
  * The refresh rate is also calculated for video playback sync purposes.
  */
 
-#include <drm/drmP.h>
+#include <drm/drm_device.h>
+#include <drm/drm_vblank.h>
 #include <drm/via_drm.h>
+
 #include "via_drv.h"
 
 #define VIA_REG_INTERRUPT       0x200
@@ -233,12 +235,12 @@ 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_WAIT_ON(ret, cur_irq->irq_queue, 3 * HZ,
 			    ((VIA_READ(masks[irq][2]) & masks[irq][3]) ==
 			     masks[irq][4]));
 		cur_irq_sequence = atomic_read(&cur_irq->irq_received);
 	} else {
-		DRM_WAIT_ON(ret, cur_irq->irq_queue, 3 * HZ,
+		VIA_WAIT_ON(ret, cur_irq->irq_queue, 3 * HZ,
 			    (((cur_irq_sequence =
 			       atomic_read(&cur_irq->irq_received)) -
 			      *sequence) <= (1 << 23)));
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..53b1f58f99b4 100644
--- a/drivers/gpu/drm/via/via_video.c
+++ b/drivers/gpu/drm/via/via_video.c
@@ -25,8 +25,9 @@
  * Video and XvMC related functions.
  */
 
-#include <drm/drmP.h>
+#include <drm/drm_device.h>
 #include <drm/via_drm.h>
+
 #include "via_drv.h"
 
 void via_init_futex(drm_via_private_t *dev_priv)
@@ -82,7 +83,7 @@ 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],
+		VIA_WAIT_ON(ret, dev_priv->decoder_queue[fx->lock],
 			    (fx->ms / 10) * (HZ / 100), *lock != fx->val);
 		return ret;
 	case VIA_FUTEX_WAKE:
-- 
2.20.1

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

Re: [PATCH v2 1/4] drm/via: drop use of DRM(READ|WRITE) macros

[Emil Velikov]

On Sat, 20 Jul 2019 at 09:46, Sam Ravnborg <sam@ravnborg.org> wrote:
>
> 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>
> Cc: Michel Dänzer <michel@daenzer.net>
> ---
>  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))
>
IMHO a far better idea is to expand these macros as static inline functions.
The extra bonus here is that the pseudo-magical VIA_BASE will also disappear.

Since all the VIA_READ8 are used for masking, one might as well drop
them in favour of via_mask().

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

Re: [PATCH v2 2/4] drm/via: add VIA_WAIT_ON()

[Emil Velikov]

On Sat, 20 Jul 2019 at 09:46, Sam Ravnborg <sam@ravnborg.org> wrote:
>
> VIA_WAIT_ON() is a direct copy of DRM_WAIT_ON() from
> drm_os_linux.h.
> The copy is made so we can avoid the dependency on the legacy header.
> A more involved approach had been to introduce wait_event_* but for this
> legacy driver the simpler and more safe approach with a copy of the
> macro was selected.
> Added the relevant header files for the functions used in VIA_WAIT_ON.
>
> Users of the macro will come in a follow-up patch.
>
Since nothing "new" is added here I would change the summary to
"drm/via: copy DRM_WAIT_ON as VIA_WAIT_ON and use it"
IMHO there's little point in splitting introduction and usage.

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

Re: [PATCH v2 1/4] drm/via: drop use of DRM(READ|WRITE) macros

[Sam Ravnborg]

Hi Emil.

On Mon, Jul 22, 2019 at 04:38:39PM +0100, Emil Velikov wrote:
> On Sat, 20 Jul 2019 at 09:46, Sam Ravnborg <sam@ravnborg.org> wrote:
> >
> > 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>
> > Cc: Michel Dänzer <michel@daenzer.net>
> > ---
> >  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))
> >
> IMHO a far better idea is to expand these macros as static inline functions.
> The extra bonus here is that the pseudo-magical VIA_BASE will also disappear.
> 
> Since all the VIA_READ8 are used for masking, one might as well drop
> them in favour of via_mask().
> 
> WDYT?

As this is a legacy driver I like the steps to be small.
So we keep it like this but maybe address it in a follow-up patch?

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

Re: [PATCH v2 2/4] drm/via: add VIA_WAIT_ON()

[Sam Ravnborg]

Hi Email.
On Mon, Jul 22, 2019 at 04:46:08PM +0100, Emil Velikov wrote:
> On Sat, 20 Jul 2019 at 09:46, Sam Ravnborg <sam@ravnborg.org> wrote:
> >
> > VIA_WAIT_ON() is a direct copy of DRM_WAIT_ON() from
> > drm_os_linux.h.
> > The copy is made so we can avoid the dependency on the legacy header.
> > A more involved approach had been to introduce wait_event_* but for this
> > legacy driver the simpler and more safe approach with a copy of the
> > macro was selected.
> > Added the relevant header files for the functions used in VIA_WAIT_ON.
> >
> > Users of the macro will come in a follow-up patch.
> >
> Since nothing "new" is added here I would change the summary to
> "drm/via: copy DRM_WAIT_ON as VIA_WAIT_ON and use it"
> IMHO there's little point in splitting introduction and usage.
Makes sense, I will do a v3 where the usage is included in this patch.

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

Re: [PATCH v2 3/4] drm/via: make via_drv.h self-contained

[Emil Velikov]

On Sat, 20 Jul 2019 at 09:45, Sam Ravnborg <sam@ravnborg.org> wrote:
>
> Added include of header file 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>
> Cc: Michel Dänzer <michel@daenzer.net>
> ---
>  drivers/gpu/drm/via/via_drv.h | 3 +++
>  1 file changed, 3 insertions(+)
>
Reviewed-by: Emil Velikov <emil.velikov@collabora.com>

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

Re: [PATCH v2 4/4] drm/via: drop use of drmP.h

[Emil Velikov]

On Sat, 20 Jul 2019 at 09:46, Sam Ravnborg <sam@ravnborg.org> wrote:
>
> Drop use of the deprecated drmP.h header.
> While touching the files divide include files in blocks
> and sort the files alphabetically.
>
> v2:
> - Replace all uses of DRM_WAIT_ON() with VIA_WAIT_ON()
>   and thus avoiding to pull in drm_os_linux.h
>
> 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>
> Cc: Michel Dänzer <michel@daenzer.net>
> ---
>  drivers/gpu/drm/via/via_dma.c      |  9 ++++++++-
>  drivers/gpu/drm/via/via_dmablit.c  | 17 ++++++++++-------
>  drivers/gpu/drm/via/via_drv.c      |  7 +++++--
>  drivers/gpu/drm/via/via_irq.c      |  8 +++++---
>  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    |  5 +++--
>  8 files changed, 47 insertions(+), 22 deletions(-)
>
With the s/DRM_WAIT_ON/VIA_WAIT_ON/ bits moved to 2/4, this patch is

Reviewed-by: Emil Velikov <emil.velikov@collabora.com>

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

Re: [PATCH v2 1/4] drm/via: drop use of DRM(READ|WRITE) macros

[Emil Velikov]

On Mon, 22 Jul 2019 at 17:17, Sam Ravnborg <sam@ravnborg.org> wrote:
>
> Hi Emil.
>
> On Mon, Jul 22, 2019 at 04:38:39PM +0100, Emil Velikov wrote:
> > On Sat, 20 Jul 2019 at 09:46, Sam Ravnborg <sam@ravnborg.org> wrote:
> > >
> > > 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>
> > > Cc: Michel Dänzer <michel@daenzer.net>
> > > ---
> > >  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))
> > >
> > IMHO a far better idea is to expand these macros as static inline functions.
> > The extra bonus here is that the pseudo-magical VIA_BASE will also disappear.
> >
> > Since all the VIA_READ8 are used for masking, one might as well drop
> > them in favour of via_mask().
> >
> > WDYT?
>
> As this is a legacy driver I like the steps to be small.
> So we keep it like this but maybe address it in a follow-up patch?
>
Sounds like unnecessary churn BTH.

Looking from another angle - machines with such GPUs are likely to be
slow at building the kernel.
So people testing this, then another series (or two?) which does the
above polish would be time consuming.
Perhaps even a bit annoying.

That said, I don't have a strong preference.

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

Re: [PATCH v2 1/4] drm/via: drop use of DRM(READ|WRITE) macros

[Sam Ravnborg]

Hi Email.

> > > IMHO a far better idea is to expand these macros as static inline functions.
> > > The extra bonus here is that the pseudo-magical VIA_BASE will also disappear.
> > >
> > > Since all the VIA_READ8 are used for masking, one might as well drop
> > > them in favour of via_mask().

Like this:
static inline u32 via_read(struct drm_via_private *dev_priv, u32 reg)
{
        return readl((void __iomem *)(dev_priv->mmio->handle + reg));
}

static inline void via_write(struct drm_via_private *dev_priv, u32 reg,
                             u32 val)
{
        writel(val, (void __iomem *)(dev_priv->mmio->handle + reg));
}

static inline void via_write8(struct drm_via_private *dev_priv, u32 reg,
                              u32 val)
{
        writeb(val, (void __iomem *)(dev_priv->mmio->handle + reg));
}

static inline void via_write8_mask_or(struct drm_via_private *dev_priv,
                                      u32 reg, u32 mask)
{
        u32 val;

        val = readb((void __iomem *)(dev_priv->mmio->handle + reg));
        writeb(val | mask, (void __iomem *)(dev_priv->mmio->handle + reg));
}

static inline void via_write8_mask_and(struct drm_via_private *dev_priv,
                                       u32 reg, u32 mask)
{
        u32 val;

        val = readb((void __iomem *)(dev_priv->mmio->handle + reg));
        writeb(val & mask, (void __iomem *)(dev_priv->mmio->handle + reg));
}

Patches are almost ready, but if there is any quick feedback let me
know.

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

Re: [PATCH v2 1/4] drm/via: drop use of DRM(READ|WRITE) macros

[Emil Velikov]

On 2019/07/22, Sam Ravnborg wrote:
> Hi Email.
> 
> > > > IMHO a far better idea is to expand these macros as static inline functions.
> > > > The extra bonus here is that the pseudo-magical VIA_BASE will also disappear.
> > > >
> > > > Since all the VIA_READ8 are used for masking, one might as well drop
> > > > them in favour of via_mask().
> 
> Like this:
> static inline u32 via_read(struct drm_via_private *dev_priv, u32 reg)
> {
>         return readl((void __iomem *)(dev_priv->mmio->handle + reg));
> }
> 
> static inline void via_write(struct drm_via_private *dev_priv, u32 reg,
>                              u32 val)
> {
>         writel(val, (void __iomem *)(dev_priv->mmio->handle + reg));
> }
> 
> static inline void via_write8(struct drm_via_private *dev_priv, u32 reg,
>                               u32 val)
> {
>         writeb(val, (void __iomem *)(dev_priv->mmio->handle + reg));
> }
> 
> static inline void via_write8_mask_or(struct drm_via_private *dev_priv,
>                                       u32 reg, u32 mask)
> {
>         u32 val;
> 
>         val = readb((void __iomem *)(dev_priv->mmio->handle + reg));
>         writeb(val | mask, (void __iomem *)(dev_priv->mmio->handle + reg));
> }
> 
> static inline void via_write8_mask_and(struct drm_via_private *dev_priv,
>                                        u32 reg, u32 mask)
> {
>         u32 val;
> 
>         val = readb((void __iomem *)(dev_priv->mmio->handle + reg));
>         writeb(val & mask, (void __iomem *)(dev_priv->mmio->handle + reg));
> }
> 
> Patches are almost ready, but if there is any quick feedback let me
> know.
> 

Don't think I've seen any "mask_and" "mask_or" API in DRM. The common
theme seems to be:

mtk_cec_mask(driver_priv, offset, value, mask)
malidp_write32_mask(driver_priv, offset, mask, value)
nvif_mask(driver_priv, address, mask, value)

HTH
Emil
_______________________________________________
openchrome-devel mailing list
openchrome-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/openchrome-devel

Re: [PATCH v2 1/4] drm/via: drop use of DRM(READ|WRITE) macros

[Sam Ravnborg]

Hi Emil.

> > 
> > Like this:
> > 
> > static inline void via_write8_mask_or(struct drm_via_private *dev_priv,
> >                                       u32 reg, u32 mask)
> > {
> >         u32 val;
> > 
> >         val = readb((void __iomem *)(dev_priv->mmio->handle + reg));
> >         writeb(val | mask, (void __iomem *)(dev_priv->mmio->handle + reg));
> > }
> > 
> > static inline void via_write8_mask_and(struct drm_via_private *dev_priv,
> >                                        u32 reg, u32 mask)
> > {
> >         u32 val;
> > 
> >         val = readb((void __iomem *)(dev_priv->mmio->handle + reg));
> >         writeb(val & mask, (void __iomem *)(dev_priv->mmio->handle + reg));
> > }
> > 
> > Patches are almost ready, but if there is any quick feedback let me
> > know.
> > 
> 
> Don't think I've seen any "mask_and" "mask_or" API in DRM. The common
> theme seems to be:
> 
> mtk_cec_mask(driver_priv, offset, value, mask)
> malidp_write32_mask(driver_priv, offset, mask, value)
> nvif_mask(driver_priv, address, mask, value)
Yep, this is better. Will send out an updated version.

Hmm, some inconsistency in order of parameters.
Decided for mask, value - seems a little bit more logical to me.

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