Armada DRM driver

This patch adds support for the pair of LCD controllers on the Marvell Armada 510 SoCs. This driver supports:
- multiple contiguous scanout buffers for video and graphics
- shm backed cacheable buffer objects for X pixmaps for Vivante GPU acceleration
- dual lcd0 and lcd1 crt operation
- video overlay on each LCD crt
- page flipping of the main scanout buffers

Included in this commit is the core driver with no output support; output support is platform and encoder driver dependent.

Signed-off-by Russell King <[email protected]>

---

drivers/gpu/drm/armada/armada_crtc.c new file +22 -0

+static uint32_t armada_drm_crtc_calculate_csc(struct armada_crtc *dcrtc)
+{
+	struct drm_display_mode *adj = &dcrtc->crtc.mode;
+	uint32_t val = 0;
+
+	if (dcrtc->csc_yuv_mode == CSC_YUV_CCIR709)
+		val |= CFG_CSC_YUV_CCIR709;
+	if (dcrtc->csc_rgb_mode == CSC_RGB_STUDIO)
+		val |= CFG_CSC_RGB_STUDIO;
+
+	/*
+	 * In auto mode, set the colorimetry, based upon the HDMI spec.
+	 * 1280x720p, 1920x1080p and 1920x1080i use ITU709, others use
+	 * ITU601.  It may be more appropriate to set this depending on
+	 * the source - but what if the graphic frame is YUV and the
+	 * video frame is RGB?
+	 */
+	if ((adj->hdisplay == 1280 && adj->vdisplay == 720 &&
+	     !(adj->flags & DRM_MODE_FLAG_INTERLACE)) ||
+	    (adj->hdisplay == 1920 && adj->vdisplay == 1080)) {
+		if (dcrtc->csc_yuv_mode == CSC_AUTO)
+			val |= CFG_CSC_YUV_CCIR709;

drivers/gpu/drm/armada/armada_crtc.c new file +4 -0

+
+	return val;
+}
+

drivers/gpu/drm/armada/armada_fb.c new file +41 -0

+struct armada_framebuffer *armada_framebuffer_create(struct drm_device *dev,
+	struct drm_mode_fb_cmd2 *mode, struct armada_gem_object *obj)
+{
+	struct armada_framebuffer *dfb;
+	uint8_t format, config;
+	int ret;
+
+	switch (mode->pixel_format) {
+#define FMT(drm, fmt, mod)		\
+	case DRM_FORMAT_##drm:		\
+		format = CFG_##fmt;	\
+		config = mod;		\
+		break
+	FMT(RGB565,	565,		CFG_SWAPRB);
+	FMT(BGR565,	565,		0);
+	FMT(ARGB1555,	1555,		CFG_SWAPRB);
+	FMT(ABGR1555,	1555,		0);
+	FMT(RGB888,	888PACK,	CFG_SWAPRB);
+	FMT(BGR888,	888PACK,	0);
+	FMT(XRGB8888,	X888,		CFG_SWAPRB);
+	FMT(XBGR8888,	X888,		0);
+	FMT(ARGB8888,	8888,		CFG_SWAPRB);
+	FMT(ABGR8888,	8888,		0);
+	FMT(YUYV,	422PACK,	CFG_YUV2RGB | CFG_SWAPYU | CFG_SWAPUV);
+	FMT(UYVY,	422PACK,	CFG_YUV2RGB);
+	FMT(VYUY,	422PACK,	CFG_YUV2RGB | CFG_SWAPUV);
+	FMT(YVYU,	422PACK,	CFG_YUV2RGB | CFG_SWAPYU);
+	FMT(YUV422,	422,		CFG_YUV2RGB | CFG_SWAPUV);
+	FMT(YVU422,	422,		CFG_YUV2RGB);
+	FMT(YUV420,	420,		CFG_YUV2RGB | CFG_SWAPUV);
+	FMT(YVU420,	420,		CFG_YUV2RGB);
+	FMT(C8,		PSEUDO8,	0);
+#undef FMT
+	default:
+		return ERR_PTR(-EINVAL);
+	}
+
+	dfb = kzalloc(sizeof(*dfb), GFP_KERNEL);
+	if (!dfb) {
+		DRM_ERROR("failed to allocate Armada fb object\n");
+		return ERR_PTR(-ENOMEM);

drivers/gpu/drm/armada/armada_fb.c new file +12 -0

+
+	drm_helper_mode_fill_fb_struct(&dfb->fb, mode);
+
+	/*
+	 * Take a reference on our object - the caller is expected
+	 * to drop their reference to it.
+	 */
+	drm_gem_object_reference(&obj->obj);
+	dfb->obj = obj;
+
+	return dfb;
+}

drivers/gpu/drm/armada/armada_fbdev.c new file +34 -0

+static int armada_fb_create(struct drm_fb_helper *fbh,
+	struct drm_fb_helper_surface_size *sizes)
+{
+	struct drm_device *dev = fbh->dev;
+	struct drm_mode_fb_cmd2 mode;
+	struct armada_framebuffer *dfb;
+	struct armada_gem_object *obj;
+	struct fb_info *info;
+	int size, ret;
+	void *ptr;
+
+	memset(&mode, 0, sizeof(mode));
+	mode.width = sizes->surface_width;
+	mode.height = sizes->surface_height;
+	mode.pitches[0] = armada_pitch(mode.width, sizes->surface_bpp);
+	mode.pixel_format = drm_mode_legacy_fb_format(sizes->surface_bpp,
+					sizes->surface_depth);
+
+	size = mode.pitches[0] * mode.height;
+	obj = armada_gem_alloc_private_object(dev, size);
+	if (!obj) {
+		DRM_ERROR("failed to allocate fb memory\n");
+		return -ENOMEM;
+	}
+
+	ret = armada_gem_linear_back(dev, obj);
+	if (ret) {
+		drm_gem_object_unreference_unlocked(&obj->obj);
+		return ret;
+	}
+
+	ptr = armada_gem_map_object(dev, obj);
+	if (!ptr) {
+		drm_gem_object_unreference_unlocked(&obj->obj);

drivers/gpu/drm/armada/armada_fbdev.c new file +45 -0

+
+	info = framebuffer_alloc(0, dev->dev);
+	if (!info) {
+		ret = -ENOMEM;
+		goto err_fballoc;
+	}
+
+	ret = fb_alloc_cmap(&info->cmap, 256, 0);
+	if (ret) {
+		ret = -ENOMEM;
+		goto err_fbcmap;
+	}
+
+	strlcpy(info->fix.id, "armada-drmfb", sizeof(info->fix.id));
+	info->par = fbh;
+	info->flags = FBINFO_DEFAULT | FBINFO_CAN_FORCE_OUTPUT;
+	info->fbops = &armada_fb_ops;
+	info->fix.smem_start = obj->phys_addr;
+	info->fix.smem_len = obj->obj.size;
+	info->screen_size = obj->obj.size;
+	info->screen_base = ptr;
+	fbh->fb = &dfb->fb;
+	fbh->fbdev = info;
+	drm_fb_helper_fill_fix(info, dfb->fb.pitches[0], dfb->fb.depth);
+	drm_fb_helper_fill_var(info, fbh, sizes->fb_width, sizes->fb_height);
+
+	DRM_DEBUG_KMS("allocated %dx%d %dbpp fb: 0x%08x\n",
+		dfb->fb.width, dfb->fb.height,
+		dfb->fb.bits_per_pixel, obj->phys_addr);
+
+	/* Reference is now held by the framebuffer object */
+	drm_gem_object_unreference(&obj->obj);
+	mutex_unlock(&dev->struct_mutex);
+
+	return 0;
+
+ err_fbcmap:
+	framebuffer_release(info);
+ err_fballoc:
+	mutex_unlock(&dev->struct_mutex);
+	dfb->fb.funcs->destroy(&dfb->fb);
+ err_fbcreate:
+	drm_gem_object_unreference_unlocked(&obj->obj);
+	return ret;
+}

drivers/gpu/drm/armada/armada_gem.c new file +11 -0

+static int armada_gem_vm_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
+{
+	struct armada_gem_object *obj = drm_to_armada_gem(vma->vm_private_data);
+	unsigned long addr = (unsigned long)vmf->virtual_address;
+	unsigned long pfn = obj->phys_addr >> PAGE_SHIFT;
+	int ret;
+
+	pfn += (addr - vma->vm_start) >> PAGE_SHIFT;
+	ret = vm_insert_pfn(vma, addr, pfn);
+
+	switch (ret) {

drivers/gpu/drm/armada/armada_gem.c new file +3 -0

+		return VM_FAULT_SIGBUS;
+	}
+}

drivers/gpu/drm/armada/drm_helper.h new file +1 -0

new file mode 100644
index 0000000..d9f2e8d
@@ -0,0 +1,31 @@
+/*

drivers/gpu/drm/armada/drm_helper.h new file +20 -0

+
+#include <drm/drmP.h>
+
+static inline struct drm_crtc *drm_crtc_find(struct drm_device *dev,
+	uint32_t id)
+{
+	struct drm_mode_object *mo;
+	mo = drm_mode_object_find(dev, id, DRM_MODE_OBJECT_CRTC);
+	return mo ? obj_to_crtc(mo) : NULL;
+}
+
+static inline struct drm_encoder *drm_encoder_find(struct drm_device *dev,
+	uint32_t id)
+{
+	struct drm_mode_object *mo;
+	mo = drm_mode_object_find(dev, id, DRM_MODE_OBJECT_ENCODER);
+	return mo ? obj_to_encoder(mo) : NULL;
+}
+
+#endif

Signature · 1 line

1.7.4.4

Quoted 22 lines · Russell King

drivers/gpu/drm/armada/armada_crtc.c new file +22 -0

+static uint32_t armada_drm_crtc_calculate_csc(struct armada_crtc *dcrtc)
+{
+	struct drm_display_mode *adj = &dcrtc->crtc.mode;
+	uint32_t val = 0;
+
+	if (dcrtc->csc_yuv_mode == CSC_YUV_CCIR709)
+		val |= CFG_CSC_YUV_CCIR709;
+	if (dcrtc->csc_rgb_mode == CSC_RGB_STUDIO)
+		val |= CFG_CSC_RGB_STUDIO;
+
+	/*
+	 * In auto mode, set the colorimetry, based upon the HDMI spec.
+	 * 1280x720p, 1920x1080p and 1920x1080i use ITU709, others use
+	 * ITU601.  It may be more appropriate to set this depending on
+	 * the source - but what if the graphic frame is YUV and the
+	 * video frame is RGB?
+	 */
+	if ((adj->hdisplay == 1280 && adj->vdisplay == 720 &&
+	     !(adj->flags & DRM_MODE_FLAG_INTERLACE)) ||
+	    (adj->hdisplay == 1920 && adj->vdisplay == 1080)) {
+		if (dcrtc->csc_yuv_mode == CSC_AUTO)
+			val |= CFG_CSC_YUV_CCIR709;

drivers/gpu/drm/armada/armada_crtc.c line 450

Quoted 10 lines

drivers/gpu/drm/armada/armada_crtc.c new file +10 -0

+	}
+
+	/*
+	 * We assume we're connected to a TV-like device, so the YUV->RGB
+	 * conversion should produce a limited range.  We should set this
+	 * depending on the connectors attached to this CRTC, and what
+	 * kind of device they report being connected.
+	 */
+	if (dcrtc->csc_rgb_mode == CSC_AUTO)
+		val |= CFG_CSC_RGB_STUDIO;

In the intel driver we check whether it's an cea mode with drm_match_cea_mode, e.g. in intel_hdmi.c:

	if (intel_hdmi->color_range_auto) {
		/* See CEA-861-E - 5.1 Default Encoding Parameters */
		if (intel_hdmi->has_hdmi_sink &&
		    drm_match_cea_mode(adjusted_mode) > 1)
			intel_hdmi->color_range = HDMI_COLOR_RANGE_16_235;
		else
			intel_hdmi->color_range = 0;
	}

(The color_range gets then propageted to the right place since different platforms have different ways to do that in intel hw).

Quoted 48 lines

drivers/gpu/drm/armada/armada_fb.c new file +48 -0

+struct armada_framebuffer *armada_framebuffer_create(struct drm_device *dev,
+	struct drm_mode_fb_cmd2 *mode, struct armada_gem_object *obj)
+{
+	struct armada_framebuffer *dfb;
+	uint8_t format, config;
+	int ret;
+
+	switch (mode->pixel_format) {
+#define FMT(drm, fmt, mod)		\
+	case DRM_FORMAT_##drm:		\
+		format = CFG_##fmt;	\
+		config = mod;		\
+		break
+	FMT(RGB565,	565,		CFG_SWAPRB);
+	FMT(BGR565,	565,		0);
+	FMT(ARGB1555,	1555,		CFG_SWAPRB);
+	FMT(ABGR1555,	1555,		0);
+	FMT(RGB888,	888PACK,	CFG_SWAPRB);
+	FMT(BGR888,	888PACK,	0);
+	FMT(XRGB8888,	X888,		CFG_SWAPRB);
+	FMT(XBGR8888,	X888,		0);
+	FMT(ARGB8888,	8888,		CFG_SWAPRB);
+	FMT(ABGR8888,	8888,		0);
+	FMT(YUYV,	422PACK,	CFG_YUV2RGB | CFG_SWAPYU | CFG_SWAPUV);
+	FMT(UYVY,	422PACK,	CFG_YUV2RGB);
+	FMT(VYUY,	422PACK,	CFG_YUV2RGB | CFG_SWAPUV);
+	FMT(YVYU,	422PACK,	CFG_YUV2RGB | CFG_SWAPYU);
+	FMT(YUV422,	422,		CFG_YUV2RGB | CFG_SWAPUV);
+	FMT(YVU422,	422,		CFG_YUV2RGB);
+	FMT(YUV420,	420,		CFG_YUV2RGB | CFG_SWAPUV);
+	FMT(YVU420,	420,		CFG_YUV2RGB);
+	FMT(C8,		PSEUDO8,	0);
+#undef FMT
+	default:
+		return ERR_PTR(-EINVAL);
+	}
+
+	dfb = kzalloc(sizeof(*dfb), GFP_KERNEL);
+	if (!dfb) {
+		DRM_ERROR("failed to allocate Armada fb object\n");
+		return ERR_PTR(-ENOMEM);
+	}
+
+	dfb->fmt = format;
+	dfb->mod = config;
+
+	ret = drm_framebuffer_init(dev, &dfb->fb, &armada_fb_funcs);
+	if (ret) {

drivers/gpu/drm/armada/armada_fb.c line 87

Quoted 3 lines

drivers/gpu/drm/armada/armada_fb.c new file +3 -0

+		kfree(dfb);
+		return ERR_PTR(ret);
+	}

drm_framebuffer_init publishes the fb object to the world, hence initialization of all invariant state _must_ be done beforehand. This is a new requirement since the kms locking rework. So all the below should be moved above.

Quoted 12 lines

drivers/gpu/drm/armada/armada_fb.c new file +12 -0

+
+	drm_helper_mode_fill_fb_struct(&dfb->fb, mode);
+
+	/*
+	 * Take a reference on our object - the caller is expected
+	 * to drop their reference to it.
+	 */
+	drm_gem_object_reference(&obj->obj);
+	dfb->obj = obj;
+
+	return dfb;
+}

[snip]

Quoted 34 lines

drivers/gpu/drm/armada/armada_fbdev.c new file +34 -0

+static int armada_fb_create(struct drm_fb_helper *fbh,
+	struct drm_fb_helper_surface_size *sizes)
+{
+	struct drm_device *dev = fbh->dev;
+	struct drm_mode_fb_cmd2 mode;
+	struct armada_framebuffer *dfb;
+	struct armada_gem_object *obj;
+	struct fb_info *info;
+	int size, ret;
+	void *ptr;
+
+	memset(&mode, 0, sizeof(mode));
+	mode.width = sizes->surface_width;
+	mode.height = sizes->surface_height;
+	mode.pitches[0] = armada_pitch(mode.width, sizes->surface_bpp);
+	mode.pixel_format = drm_mode_legacy_fb_format(sizes->surface_bpp,
+					sizes->surface_depth);
+
+	size = mode.pitches[0] * mode.height;
+	obj = armada_gem_alloc_private_object(dev, size);
+	if (!obj) {
+		DRM_ERROR("failed to allocate fb memory\n");
+		return -ENOMEM;
+	}
+
+	ret = armada_gem_linear_back(dev, obj);
+	if (ret) {
+		drm_gem_object_unreference_unlocked(&obj->obj);
+		return ret;
+	}
+
+	ptr = armada_gem_map_object(dev, obj);
+	if (!ptr) {
+		drm_gem_object_unreference_unlocked(&obj->obj);

drivers/gpu/drm/armada/armada_fbdev.c line 78

Quoted 10 lines

drivers/gpu/drm/armada/armada_fbdev.c new file +10 -0

+		return -ENOMEM;
+	}
+
+	dfb = armada_framebuffer_create(dev, &mode, obj);
+	if (IS_ERR(dfb)) {
+		ret = PTR_ERR(dfb);
+		goto err_fbcreate;
+	}
+
+	mutex_lock(&dev->struct_mutex);

I don't understand what exactly the dev->struct_mutex protects here, I think it can be dropped.

I'm just trying to reduce proliferation of that lock as much as possible, since it's deeply interwoven with the legacy drm dragons ...

Quoted 11 lines

drivers/gpu/drm/armada/armada_gem.c new file +11 -0

+static int armada_gem_vm_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
+{
+	struct armada_gem_object *obj = drm_to_armada_gem(vma->vm_private_data);
+	unsigned long addr = (unsigned long)vmf->virtual_address;
+	unsigned long pfn = obj->phys_addr >> PAGE_SHIFT;
+	int ret;
+
+	pfn += (addr - vma->vm_start) >> PAGE_SHIFT;
+	ret = vm_insert_pfn(vma, addr, pfn);
+
+	switch (ret) {

drivers/gpu/drm/armada/armada_gem.c line 37

Quoted 10 lines

drivers/gpu/drm/armada/armada_gem.c new file +10 -0

+	case -EIO:
+	case -EAGAIN:
+		set_need_resched();
+	case 0:
+	case -ERESTARTSYS:
+	case -EINTR:
+		return VM_FAULT_NOPAGE;
+	case -ENOMEM:
+		return VM_FAULT_OOM;
+	default:

You don't handle -EBUSY from vm_insert_pfn here. This can happen when mulitple threads concurrently fault on the same address. See

commit e79e0fe380847493266fba557217e2773c61bd1b

Author: Dmitry Rogozhkin <[email protected]>
Date:   Wed Oct 3 17:15:26 2012 +0300

drm/i915: EBUSY status handling added to i915_gem_fault().

Quoted 4 lines

drivers/gpu/drm/armada/armada_gem.c new file +3 -0

+		return VM_FAULT_SIGBUS;
+	}
+}

[snip]

Quoted 13 lines

drivers/gpu/drm/armada/drm_helper.h new file +7 -0

new file mode 100644
index 0000000..d9f2e8d
@@ -0,0 +1,31 @@
+/*
+ * Copyright (C) 2012 Russell King
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */

drivers/gpu/drm/armada/drm_helper.h line 11

Quoted 4 lines

drivers/gpu/drm/armada/drm_helper.h new file +4 -0

+#ifndef DRM_HELPER_H
+#define DRM_HELPER_H
+
+/* Useful helpers I wish the DRM core would provide */

With the addition of variants for connectors/planes and rolling it out in drm_crtc.c this sounds like a nice up-front patch. I agree that this doesn't really belong in drivers ;-)

Quoted 46 lines · Daniel Vetter

Quoted 22 lines · Russell King

drivers/gpu/drm/armada/armada_crtc.c new file +22 -0

+static uint32_t armada_drm_crtc_calculate_csc(struct armada_crtc *dcrtc)
+{
+   struct drm_display_mode *adj = &dcrtc->crtc.mode;
+   uint32_t val = 0;
+
+   if (dcrtc->csc_yuv_mode == CSC_YUV_CCIR709)
+           val |= CFG_CSC_YUV_CCIR709;
+   if (dcrtc->csc_rgb_mode == CSC_RGB_STUDIO)
+           val |= CFG_CSC_RGB_STUDIO;
+
+   /*
+    * In auto mode, set the colorimetry, based upon the HDMI spec.
+    * 1280x720p, 1920x1080p and 1920x1080i use ITU709, others use
+    * ITU601.  It may be more appropriate to set this depending on
+    * the source - but what if the graphic frame is YUV and the
+    * video frame is RGB?
+    */
+   if ((adj->hdisplay == 1280 && adj->vdisplay == 720 &&
+        !(adj->flags & DRM_MODE_FLAG_INTERLACE)) ||
+       (adj->hdisplay == 1920 && adj->vdisplay == 1080)) {
+           if (dcrtc->csc_yuv_mode == CSC_AUTO)
+                   val |= CFG_CSC_YUV_CCIR709;

drivers/gpu/drm/armada/armada_crtc.c line 450

Quoted 10 lines

drivers/gpu/drm/armada/armada_crtc.c new file +10 -0

+   }
+
+   /*
+    * We assume we're connected to a TV-like device, so the YUV->RGB
+    * conversion should produce a limited range.  We should set this
+    * depending on the connectors attached to this CRTC, and what
+    * kind of device they report being connected.
+    */
+   if (dcrtc->csc_rgb_mode == CSC_AUTO)
+           val |= CFG_CSC_RGB_STUDIO;

In the intel driver we check whether it's an cea mode with drm_match_cea_mode, e.g. in intel_hdmi.c:

      if (intel_hdmi->color_range_auto) {
              /* See CEA-861-E - 5.1 Default Encoding Parameters */
              if (intel_hdmi->has_hdmi_sink &&
                  drm_match_cea_mode(adjusted_mode) > 1)
                      intel_hdmi->color_range = HDMI_COLOR_RANGE_16_235;
              else
                      intel_hdmi->color_range = 0;
      }

Quoted 2 lines

(The color_range gets then propageted to the right place since different platforms have different ways to do that in intel hw).

Unfortunately, this disagrees with the HDMI v1.3a specification:

| Default Colorimetry
|
| ...
|
| 480p, 480i, 576p, 576i, 240p and 288p
|
| The default colorimetry for all 480-line, 576-line, 240-line, and 288-line
| video formats described in CEA-861-D is based on SMPTE 170M.
|
| 1080i, 1080p and 720p
|
| The default colorimetry for high-definition video formats (1080i, 1080p and
| 720p) described in CEA-861-D is based on ITU-R BT.709-5.

As the HDMI spec refers to the CEA-861 specs, the HDMI spec overrides CEA when dealing with HDMI specifics.

So, according to the HDMI specification, the default is that it's only 1080i, 1080p and 720p which are 709, the others are 601. This does not correspond with "drm_match_cea_mode(adjusted_mode) > 1".

Unfortunately, given DRM's structure, there's no way for the CRTC layer to really know what its driving, and this setting is at the CRTC layer, not the output layer. It may be that you have the CRTC duplicated between two different display devices with different characteristics, which means you have to "choose" one - or just not bother. I've chosen the latter because it's a simpler solution, and is in no way any more correct than any other solution.

This is also why these are exported to userspace via properties, so if userspace knows better, it can deal with those issues.

Quoted 48 lines

Quoted 48 lines

drivers/gpu/drm/armada/armada_fb.c new file +48 -0

+struct armada_framebuffer *armada_framebuffer_create(struct drm_device *dev,
+   struct drm_mode_fb_cmd2 *mode, struct armada_gem_object *obj)
+{
+   struct armada_framebuffer *dfb;
+   uint8_t format, config;
+   int ret;
+
+   switch (mode->pixel_format) {
+#define FMT(drm, fmt, mod)         \
+   case DRM_FORMAT_##drm:          \
+           format = CFG_##fmt;     \
+           config = mod;           \
+           break
+   FMT(RGB565,     565,            CFG_SWAPRB);
+   FMT(BGR565,     565,            0);
+   FMT(ARGB1555,   1555,           CFG_SWAPRB);
+   FMT(ABGR1555,   1555,           0);
+   FMT(RGB888,     888PACK,        CFG_SWAPRB);
+   FMT(BGR888,     888PACK,        0);
+   FMT(XRGB8888,   X888,           CFG_SWAPRB);
+   FMT(XBGR8888,   X888,           0);
+   FMT(ARGB8888,   8888,           CFG_SWAPRB);
+   FMT(ABGR8888,   8888,           0);
+   FMT(YUYV,       422PACK,        CFG_YUV2RGB | CFG_SWAPYU | CFG_SWAPUV);
+   FMT(UYVY,       422PACK,        CFG_YUV2RGB);
+   FMT(VYUY,       422PACK,        CFG_YUV2RGB | CFG_SWAPUV);
+   FMT(YVYU,       422PACK,        CFG_YUV2RGB | CFG_SWAPYU);
+   FMT(YUV422,     422,            CFG_YUV2RGB | CFG_SWAPUV);
+   FMT(YVU422,     422,            CFG_YUV2RGB);
+   FMT(YUV420,     420,            CFG_YUV2RGB | CFG_SWAPUV);
+   FMT(YVU420,     420,            CFG_YUV2RGB);
+   FMT(C8,         PSEUDO8,        0);
+#undef FMT
+   default:
+           return ERR_PTR(-EINVAL);
+   }
+
+   dfb = kzalloc(sizeof(*dfb), GFP_KERNEL);
+   if (!dfb) {
+           DRM_ERROR("failed to allocate Armada fb object\n");
+           return ERR_PTR(-ENOMEM);
+   }
+
+   dfb->fmt = format;
+   dfb->mod = config;
+
+   ret = drm_framebuffer_init(dev, &dfb->fb, &armada_fb_funcs);
+   if (ret) {

Quoted 8 lines

drivers/gpu/drm/armada/armada_fb.c line 87

Quoted 3 lines

drivers/gpu/drm/armada/armada_fb.c new file +3 -0

+           kfree(dfb);
+           return ERR_PTR(ret);
+   }

drm_framebuffer_init publishes the fb object to the world, hence initialization of all invariant state _must_ be done beforehand. This is a new requirement since the kms locking rework. So all the below should be moved above.

The constant rewriting of DRM is a right pain in the arse. I'm dealing with v3.9 here, and v3.9 only... However, it doesn't harm moving the below so I'll do that. When v3.10 comes along, that's the kernel I'll care about, and not v3.10-rcwhatever.

Quoted 63 lines

Quoted 12 lines

drivers/gpu/drm/armada/armada_fb.c new file +12 -0

+
+   drm_helper_mode_fill_fb_struct(&dfb->fb, mode);
+
+   /*
+    * Take a reference on our object - the caller is expected
+    * to drop their reference to it.
+    */
+   drm_gem_object_reference(&obj->obj);
+   dfb->obj = obj;
+
+   return dfb;
+}

[snip]

Quoted 34 lines

drivers/gpu/drm/armada/armada_fbdev.c new file +34 -0

+static int armada_fb_create(struct drm_fb_helper *fbh,
+   struct drm_fb_helper_surface_size *sizes)
+{
+   struct drm_device *dev = fbh->dev;
+   struct drm_mode_fb_cmd2 mode;
+   struct armada_framebuffer *dfb;
+   struct armada_gem_object *obj;
+   struct fb_info *info;
+   int size, ret;
+   void *ptr;
+
+   memset(&mode, 0, sizeof(mode));
+   mode.width = sizes->surface_width;
+   mode.height = sizes->surface_height;
+   mode.pitches[0] = armada_pitch(mode.width, sizes->surface_bpp);
+   mode.pixel_format = drm_mode_legacy_fb_format(sizes->surface_bpp,
+                                   sizes->surface_depth);
+
+   size = mode.pitches[0] * mode.height;
+   obj = armada_gem_alloc_private_object(dev, size);
+   if (!obj) {
+           DRM_ERROR("failed to allocate fb memory\n");
+           return -ENOMEM;
+   }
+
+   ret = armada_gem_linear_back(dev, obj);
+   if (ret) {
+           drm_gem_object_unreference_unlocked(&obj->obj);
+           return ret;
+   }
+
+   ptr = armada_gem_map_object(dev, obj);
+   if (!ptr) {
+           drm_gem_object_unreference_unlocked(&obj->obj);

drivers/gpu/drm/armada/armada_fbdev.c line 78

Quoted 10 lines

drivers/gpu/drm/armada/armada_fbdev.c new file +10 -0

+           return -ENOMEM;
+   }
+
+   dfb = armada_framebuffer_create(dev, &mode, obj);
+   if (IS_ERR(dfb)) {
+           ret = PTR_ERR(dfb);
+           goto err_fbcreate;
+   }
+
+   mutex_lock(&dev->struct_mutex);

I don't understand what exactly the dev->struct_mutex protects here, I think it can be dropped.

Quoted 2 lines

I'm just trying to reduce proliferation of that lock as much as possible, since it's deeply interwoven with the legacy drm dragons ...

Bear in mind much of this was written when I had very little clue as to what DRM required, so there's lots of stuff which was worked out from the i915 driver - if the i915 driver did something like that, then I initially copied that pattern.

You may wish to ask the exact same question of the i915 driver which still takes that lock over a similar section of code.

Quoted 25 lines

Quoted 11 lines

drivers/gpu/drm/armada/armada_gem.c new file +11 -0

+static int armada_gem_vm_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
+{
+   struct armada_gem_object *obj = drm_to_armada_gem(vma->vm_private_data);
+   unsigned long addr = (unsigned long)vmf->virtual_address;
+   unsigned long pfn = obj->phys_addr >> PAGE_SHIFT;
+   int ret;
+
+   pfn += (addr - vma->vm_start) >> PAGE_SHIFT;
+   ret = vm_insert_pfn(vma, addr, pfn);
+
+   switch (ret) {

drivers/gpu/drm/armada/armada_gem.c line 37

Quoted 10 lines

drivers/gpu/drm/armada/armada_gem.c new file +10 -0

+   case -EIO:
+   case -EAGAIN:
+           set_need_resched();
+   case 0:
+   case -ERESTARTSYS:
+   case -EINTR:
+           return VM_FAULT_NOPAGE;
+   case -ENOMEM:
+           return VM_FAULT_OOM;
+   default:

You don't handle -EBUSY from vm_insert_pfn here. This can happen when mulitple threads concurrently fault on the same address. See

Quoted 5 lines

commit e79e0fe380847493266fba557217e2773c61bd1b

Author: Dmitry Rogozhkin <[email protected]>
Date:   Wed Oct 3 17:15:26 2012 +0300

drm/i915: EBUSY status handling added to i915_gem_fault().

Ok.

Quoted 20 lines

Quoted 4 lines

drivers/gpu/drm/armada/armada_gem.c new file +3 -0

+           return VM_FAULT_SIGBUS;
+   }
+}

[snip]

Quoted 13 lines

drivers/gpu/drm/armada/drm_helper.h new file +7 -0

new file mode 100644
index 0000000..d9f2e8d
@@ -0,0 +1,31 @@
+/*
+ * Copyright (C) 2012 Russell King
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */

Quoted 8 lines

drivers/gpu/drm/armada/drm_helper.h line 11

Quoted 4 lines

drivers/gpu/drm/armada/drm_helper.h new file +4 -0

+#ifndef DRM_HELPER_H
+#define DRM_HELPER_H
+
+/* Useful helpers I wish the DRM core would provide */

With the addition of variants for connectors/planes and rolling it out in drm_crtc.c this sounds like a nice up-front patch. I agree that this doesn't really belong in drivers ;-)

If DRM people want to take this and do that, then that's fine, but otherwise I have more than enough on my plate that moving those and then fixing up all the DRM drivers is way beyond what I'm prepared to do. (Consider that it has been more than two weeks between this posting and the previous posting...)

In any case, I disagree with the idea that they should find their way into drm_crtc.c - they're small enough to go into a header file as inline functions as I've shown in this patch. Do we really need the overhead of function calls for this, especially when these may be used in various fast paths?

However, at least I've taken the time to _think_ about what I'm doing and realise that there _is_ scope here for the DRM core to improve, rather than burying this stuff deep inside my driver like everyone else has. That's no reason to penalise patches from the "good guys" who think about what they're doing while accepting drivers from people who don't (like everyone else who's submitted a DRM driver recently without doing this.)

Quoted 52 lines · Russell King - ARM Linux

Quoted 46 lines · Daniel Vetter

Quoted 22 lines · Russell King

drivers/gpu/drm/armada/armada_crtc.c new file +22 -0

+static uint32_t armada_drm_crtc_calculate_csc(struct armada_crtc *dcrtc)
+{
+   struct drm_display_mode *adj = &dcrtc->crtc.mode;
+   uint32_t val = 0;
+
+   if (dcrtc->csc_yuv_mode == CSC_YUV_CCIR709)
+           val |= CFG_CSC_YUV_CCIR709;
+   if (dcrtc->csc_rgb_mode == CSC_RGB_STUDIO)
+           val |= CFG_CSC_RGB_STUDIO;
+
+   /*
+    * In auto mode, set the colorimetry, based upon the HDMI spec.
+    * 1280x720p, 1920x1080p and 1920x1080i use ITU709, others use
+    * ITU601.  It may be more appropriate to set this depending on
+    * the source - but what if the graphic frame is YUV and the
+    * video frame is RGB?
+    */
+   if ((adj->hdisplay == 1280 && adj->vdisplay == 720 &&
+        !(adj->flags & DRM_MODE_FLAG_INTERLACE)) ||
+       (adj->hdisplay == 1920 && adj->vdisplay == 1080)) {
+           if (dcrtc->csc_yuv_mode == CSC_AUTO)
+                   val |= CFG_CSC_YUV_CCIR709;

drivers/gpu/drm/armada/armada_crtc.c line 450

Quoted 10 lines

drivers/gpu/drm/armada/armada_crtc.c new file +10 -0

+   }
+
+   /*
+    * We assume we're connected to a TV-like device, so the YUV->RGB
+    * conversion should produce a limited range.  We should set this
+    * depending on the connectors attached to this CRTC, and what
+    * kind of device they report being connected.
+    */
+   if (dcrtc->csc_rgb_mode == CSC_AUTO)
+           val |= CFG_CSC_RGB_STUDIO;

In the intel driver we check whether it's an cea mode with drm_match_cea_mode, e.g. in intel_hdmi.c:

      if (intel_hdmi->color_range_auto) {
              /* See CEA-861-E - 5.1 Default Encoding Parameters */
              if (intel_hdmi->has_hdmi_sink &&
                  drm_match_cea_mode(adjusted_mode) > 1)
                      intel_hdmi->color_range = HDMI_COLOR_RANGE_16_235;
              else
                      intel_hdmi->color_range = 0;
      }

Quoted 2 lines

(The color_range gets then propageted to the right place since different platforms have different ways to do that in intel hw).

Unfortunately, this disagrees with the HDMI v1.3a specification:

Quoted 5 lines

| Default Colorimetry
|
| ...
|
| 480p, 480i, 576p, 576i, 240p and 288p

Quoted 8 lines

|
| The default colorimetry for all 480-line, 576-line, 240-line, and 288-line
| video formats described in CEA-861-D is based on SMPTE 170M.
|
| 1080i, 1080p and 720p
|
| The default colorimetry for high-definition video formats (1080i, 1080p and
| 720p) described in CEA-861-D is based on ITU-R BT.709-5.

Quoted 6 lines

As the HDMI spec refers to the CEA-861 specs, the HDMI spec overrides CEA when dealing with HDMI specifics.

So, according to the HDMI specification, the default is that it's only 1080i, 1080p and 720p which are 709, the others are 601. This does not correspond with "drm_match_cea_mode(adjusted_mode) > 1".

Hm, sounds like we need to improve stuff a bit, maybe add a common cea helper to the drm core with a bool is_hdmi to decide whether it's palin CEA or HDMI-CEA. Ville (who's written the current code and the match cea mode helper) can you please take a look?

Quoted 8 lines

Unfortunately, given DRM's structure, there's no way for the CRTC layer to really know what its driving, and this setting is at the CRTC layer, not the output layer. It may be that you have the CRTC duplicated between two different display devices with different characteristics, which means you have to "choose" one - or just not bother. I've chosen the latter because it's a simpler solution, and is in no way any more correct than any other solution.

Quoted 2 lines

This is also why these are exported to userspace via properties, so if userspace knows better, it can deal with those issues.

Yeah, allowing userspace to overwrite the default selection makes lots of sense, we expose similar properties.

Quoted 15 lines · Daniel Vetter

Quoted 5 lines · Russell King - ARM Linux

| Default Colorimetry
|
| ...
|
| 480p, 480i, 576p, 576i, 240p and 288p

Quoted 8 lines

|
| The default colorimetry for all 480-line, 576-line, 240-line, and 288-line
| video formats described in CEA-861-D is based on SMPTE 170M.
|
| 1080i, 1080p and 720p
|
| The default colorimetry for high-definition video formats (1080i, 1080p and
| 720p) described in CEA-861-D is based on ITU-R BT.709-5.

I think this was pretty much copy pasted from CEA-861-D which is very vague.

CEA-861-E is a bit better, and more clearly states that if the sink can receive YCbCr, then the source should use it by default for CE formats, and the default colorimetry depends on whether it's SD or HD. It also states that when transmitting IT or CE formats as RGB, the color space is the one defined in the EDID. CEA-861-D only made that statement for IT formats, and left the RGB CE format case out in the cold.

Oh and the other thing someone should do is fix the intel Xv code to use BT.709 CSC matrix for HD content. I believe that code is hardcoded for BT.601 currently, which may explain the last weirdness reported in that CEA bug or ours.

Quoted 20 lines · Ville Syrjälä

Quoted 15 lines · Daniel Vetter

Quoted 5 lines · Russell King - ARM Linux

| Default Colorimetry
|
| ...
|
| 480p, 480i, 576p, 576i, 240p and 288p

Quoted 8 lines

|
| The default colorimetry for all 480-line, 576-line, 240-line, and 288-line
| video formats described in CEA-861-D is based on SMPTE 170M.
|
| 1080i, 1080p and 720p
|
| The default colorimetry for high-definition video formats (1080i, 1080p and
| 720p) described in CEA-861-D is based on ITU-R BT.709-5.

I think this was pretty much copy pasted from CEA-861-D which is very vague.

Quoted 6 lines

CEA-861-E is a bit better, and more clearly states that if the sink can receive YCbCr, then the source should use it by default for CE formats, and the default colorimetry depends on whether it's SD or HD. It also states that when transmitting IT or CE formats as RGB, the color space is the one defined in the EDID. CEA-861-D only made that statement for IT formats, and left the RGB CE format case out in the cold.

Actually, what I'm doing there is probably wrong when you consider what is going on:

Overlay (YUV) -> YUV->RGB colorspace conversion

                              |
                              v

Graphic (RGB) -----------(colorkey)--------------------> HDMI

These bits control the YUV->RGB colorspace conversion. The "is it 601 or 709 colorspace" question applies more to the colorspace of the overlay image. As far as I can tell, that is unspecified within our normal video playback programs - there's provision to communicate that information (they certainly don't seem to look for any kind of Xv attribute).

The "is it computer or studio RGB" question (I think - I can't say because the documentation is hellishly poor, and you now have as much information on this as I do) refers to the colorspace of the RGB side.

So, maybe I should move the YUV colorspace setting to be a drm_plane property? But then how do we know what format it is supposed to be? Do we just pick one and hope it's right? Do we try to autodetect it from the size of the drm_plane framebuffer? What if something downscales a HD YUV framebuffer to something smaller because the display is smaller?

What I can say is that I've watched many hours of content with my driver and at 720p output resolution, I prefer it converting the YUV between 709 to studio RGB - otherwise the blacks are too black and I find that I have to adjust the brightness/contrast to bring the black levels up compared to a standard TV broadcast.

Quoted 4 lines

Oh and the other thing someone should do is fix the intel Xv code to use BT.709 CSC matrix for HD content. I believe that code is hardcoded for BT.601 currently, which may explain the last weirdness reported in that CEA bug or ours.

How do you propose to switch between the two?

Quoted 46 lines · Daniel Vetter

Quoted 22 lines · Russell King

drivers/gpu/drm/armada/armada_crtc.c new file +22 -0

+static uint32_t armada_drm_crtc_calculate_csc(struct armada_crtc *dcrtc)
+{
+   struct drm_display_mode *adj = &dcrtc->crtc.mode;
+   uint32_t val = 0;
+
+   if (dcrtc->csc_yuv_mode == CSC_YUV_CCIR709)
+           val |= CFG_CSC_YUV_CCIR709;
+   if (dcrtc->csc_rgb_mode == CSC_RGB_STUDIO)
+           val |= CFG_CSC_RGB_STUDIO;
+
+   /*
+    * In auto mode, set the colorimetry, based upon the HDMI spec.
+    * 1280x720p, 1920x1080p and 1920x1080i use ITU709, others use
+    * ITU601.  It may be more appropriate to set this depending on
+    * the source - but what if the graphic frame is YUV and the
+    * video frame is RGB?
+    */
+   if ((adj->hdisplay == 1280 && adj->vdisplay == 720 &&
+        !(adj->flags & DRM_MODE_FLAG_INTERLACE)) ||
+       (adj->hdisplay == 1920 && adj->vdisplay == 1080)) {
+           if (dcrtc->csc_yuv_mode == CSC_AUTO)
+                   val |= CFG_CSC_YUV_CCIR709;

drivers/gpu/drm/armada/armada_crtc.c line 450

Quoted 10 lines

drivers/gpu/drm/armada/armada_crtc.c new file +10 -0

+   }
+
+   /*
+    * We assume we're connected to a TV-like device, so the YUV->RGB
+    * conversion should produce a limited range.  We should set this
+    * depending on the connectors attached to this CRTC, and what
+    * kind of device they report being connected.
+    */
+   if (dcrtc->csc_rgb_mode == CSC_AUTO)
+           val |= CFG_CSC_RGB_STUDIO;

In the intel driver we check whether it's an cea mode with drm_match_cea_mode, e.g. in intel_hdmi.c:

      if (intel_hdmi->color_range_auto) {
              /* See CEA-861-E - 5.1 Default Encoding Parameters */
              if (intel_hdmi->has_hdmi_sink &&
                  drm_match_cea_mode(adjusted_mode) > 1)
                      intel_hdmi->color_range = HDMI_COLOR_RANGE_16_235;
              else
                      intel_hdmi->color_range = 0;
      }

Quoted 2 lines

(The color_range gets then propageted to the right place since different platforms have different ways to do that in intel hw).

Unfortunately, this disagrees with the HDMI v1.3a specification:

| Default Colorimetry
|
| ...
|
| 480p, 480i, 576p, 576i, 240p and 288p
|
| The default colorimetry for all 480-line, 576-line, 240-line, and 288-line
| video formats described in CEA-861-D is based on SMPTE 170M.
|
| 1080i, 1080p and 720p
|
| The default colorimetry for high-definition video formats (1080i, 1080p and
| 720p) described in CEA-861-D is based on ITU-R BT.709-5.

As the HDMI spec refers to the CEA-861 specs, the HDMI spec overrides CEA when dealing with HDMI specifics.

So, according to the HDMI specification, the default is that it's only 1080i, 1080p and 720p which are 709, the others are 601. This does not correspond with "drm_match_cea_mode(adjusted_mode) > 1".

Unfortunately, given DRM's structure, there's no way for the CRTC layer to really know what its driving, and this setting is at the CRTC layer, not the output layer. It may be that you have the CRTC duplicated between two different display devices with different characteristics, which means you have to "choose" one - or just not bother. I've chosen the latter because it's a simpler solution, and is in no way any more correct than any other solution.

This is also why these are exported to userspace via properties, so if userspace knows better, it can deal with those issues.

Quoted 48 lines

Quoted 48 lines

drivers/gpu/drm/armada/armada_fb.c new file +48 -0

+struct armada_framebuffer *armada_framebuffer_create(struct drm_device *dev,
+   struct drm_mode_fb_cmd2 *mode, struct armada_gem_object *obj)
+{
+   struct armada_framebuffer *dfb;
+   uint8_t format, config;
+   int ret;
+
+   switch (mode->pixel_format) {
+#define FMT(drm, fmt, mod)         \
+   case DRM_FORMAT_##drm:          \
+           format = CFG_##fmt;     \
+           config = mod;           \
+           break
+   FMT(RGB565,     565,            CFG_SWAPRB);
+   FMT(BGR565,     565,            0);
+   FMT(ARGB1555,   1555,           CFG_SWAPRB);
+   FMT(ABGR1555,   1555,           0);
+   FMT(RGB888,     888PACK,        CFG_SWAPRB);
+   FMT(BGR888,     888PACK,        0);
+   FMT(XRGB8888,   X888,           CFG_SWAPRB);
+   FMT(XBGR8888,   X888,           0);
+   FMT(ARGB8888,   8888,           CFG_SWAPRB);
+   FMT(ABGR8888,   8888,           0);
+   FMT(YUYV,       422PACK,        CFG_YUV2RGB | CFG_SWAPYU | CFG_SWAPUV);
+   FMT(UYVY,       422PACK,        CFG_YUV2RGB);
+   FMT(VYUY,       422PACK,        CFG_YUV2RGB | CFG_SWAPUV);
+   FMT(YVYU,       422PACK,        CFG_YUV2RGB | CFG_SWAPYU);
+   FMT(YUV422,     422,            CFG_YUV2RGB | CFG_SWAPUV);
+   FMT(YVU422,     422,            CFG_YUV2RGB);
+   FMT(YUV420,     420,            CFG_YUV2RGB | CFG_SWAPUV);
+   FMT(YVU420,     420,            CFG_YUV2RGB);
+   FMT(C8,         PSEUDO8,        0);
+#undef FMT
+   default:
+           return ERR_PTR(-EINVAL);
+   }
+
+   dfb = kzalloc(sizeof(*dfb), GFP_KERNEL);
+   if (!dfb) {
+           DRM_ERROR("failed to allocate Armada fb object\n");
+           return ERR_PTR(-ENOMEM);
+   }
+
+   dfb->fmt = format;
+   dfb->mod = config;
+
+   ret = drm_framebuffer_init(dev, &dfb->fb, &armada_fb_funcs);
+   if (ret) {

Quoted 8 lines

drivers/gpu/drm/armada/armada_fb.c line 87

Quoted 3 lines

drivers/gpu/drm/armada/armada_fb.c new file +3 -0

+           kfree(dfb);
+           return ERR_PTR(ret);
+   }

drm_framebuffer_init publishes the fb object to the world, hence initialization of all invariant state _must_ be done beforehand. This is a new requirement since the kms locking rework. So all the below should be moved above.

The constant rewriting of DRM is a right pain in the arse. I'm dealing with v3.9 here, and v3.9 only... However, it doesn't harm moving the below so I'll do that. When v3.10 comes along, that's the kernel I'll care about, and not v3.10-rcwhatever.

OMG I'm working in a subsystem where stuff is being developed, with only a few resources! I know my full time job isn't maintaining a 500,000 line subsystem,
and the sub maintainers and developers do a great job refactoring where required.

lots of other driver authors have made substantial changes to the drm core as they've written drivers, you spot one bit of refactoring and its a major shortcoming
of the entire subsystem and development community.

how about instead of writing:
"However, at least I've taken the time to _think_ about what I'm doing and realise that there _is_ scope here for the DRM core to improve, rather than burying this stuff deep inside my driver like everyone else has. That's no reason to penalise patches from the "good guys" who think"

you go with
"I noticed this piece of functionality could be refactored, here is a patch adding them to
the core, does anyone think its a good idea?"

As Daniel pointed out every driver submitted has refactored things, even exynos did a
lot of work to be the first ARM driver we shipped, the DRM core doesn't write itself and
I fully expect driver authors to be the ones that tell me what needs refactoring, since
they are on the pointy end, but to state that you are the only person ever to think about
things is frankly being a dick.

Lets try for less dick, and more productive in future.

Quoted 32 lines · Dave Airlie

OMG I'm working in a subsystem where stuff is being developed, with only a few resources! I know my full time job isn't maintaining a 500,000 line subsystem,
and the sub maintainers and developers do a great job refactoring where required.

lots of other driver authors have made substantial changes to the drm core as they've written drivers, you spot one bit of refactoring and its a major shortcoming
of the entire subsystem and development community.

how about instead of writing:
"However, at least I've taken the time to _think_ about what I'm doing and realise that there _is_ scope here for the DRM core to improve, rather than burying this stuff deep inside my driver like everyone else has. That's no reason to penalise patches from the "good guys" who think"

you go with
"I noticed this piece of functionality could be refactored, here is a patch adding them to
the core, does anyone think its a good idea?"

As Daniel pointed out every driver submitted has refactored things, even exynos did a
lot of work to be the first ARM driver we shipped, the DRM core doesn't write itself and
I fully expect driver authors to be the ones that tell me what needs refactoring, since
they are on the pointy end, but to state that you are the only person ever to think about
things is frankly being a dick.

Quoted 1 line

Lets try for less dick, and more productive in future.

And you can stick your dick back where you got it from David. Really, your response is totally uncalled for.

Let's try realising that I only have very limited time to put into this and unlike the other submitters who have been _paid_ to get their code sorted, this is being done purely for free - which means it's really low priority. As you should already realise because I've stated already that I *really* don't care whether it gets into mainline or not.

If you want stuff to be refactored in DRM, you need to find someone with more time to do it.

how about instead of writing:
"However, at least I've taken the time to_think_ about what I'm doing and realise that there_is_ scope here for the DRM core to improve, rather than burying this stuff deep inside my driver like everyone else has. That's no reason to penalise patches from the "good guys" who think"

you go with
"I noticed this piece of functionality could be refactored, here is a patch adding them to
the core, does anyone think its a good idea?"

Quoted 10 lines · Dave Airlie

how about instead of writing:
"However, at least I've taken the time to_think_ about what I'm doing and realise that there_is_ scope here for the DRM core to improve, rather than burying this stuff deep inside my driver like everyone else has. That's no reason to penalise patches from the "good guys" who think"

you go with
"I noticed this piece of functionality could be refactored, here is a patch adding them to
the core, does anyone think its a good idea?"

Dave,

at least on this point I do share Russell's impression. I've sent bunch of patches improving TDA998x and DRM+DT:
- TDA998x irq handling - ignored
- TDA998x sync fix - ignored
- Fix drm I2C slave encoder probing

I am aware that this is not an easy job nor one you get much appreciation for. But, back when TDA998x driver was published, all my comments were basically answered with "Oh, I know. Maybe someday somebody will fix it".

I am not being paid for any of this, but have a strong intrinsic motivation here. But I am loosing interest in sending fixes for DRM stuff because my (personal) impression is the same Russell
has: Depending on who sends patches, they get merged independent of how broken they are - others are discussed to death.

Quoted 10 lines · Sebastian Hesselbarth

at least on this point I do share Russell's impression. I've sent bunch of patches improving TDA998x and DRM+DT:
- TDA998x irq handling - ignored
- TDA998x sync fix - ignored
- Fix drm I2C slave encoder probing

I am aware that this is not an easy job nor one you get much appreciation for. But, back when TDA998x driver was published, all my comments were basically answered with "Oh, I know. Maybe someday somebody will fix it".

I guess part of the problem here is that in the arm world we don't (yet) have many full-blown drivers and not many people to fix up stuff or chime in with good review. And sometimes that just means that someone puts down his foot and says "this is how we do it" - at least for drm/i915 I sometimes have to do that to unblock a massive bikeshed-fest.

Quoted 5 lines

I am not being paid for any of this, but have a strong intrinsic motivation here. But I am loosing interest in sending fixes for DRM stuff because my (personal) impression is the same Russell
has: Depending on who sends patches, they get merged independent of how broken they are - others are discussed to death.

Hm, we run fairly extensive QA for drm/i915, and thus far the drm core stuff hasn't really blown up badly for us. So can you please point at examples where crap was merged and shouldn't have been?

Wrt to bikeshed to death I know that drm folks are a bit prone to that (me included), but recently I haven't spotted a case where this happened. ARM stuff excluded ofc since I don't follow that too closely. There's also that Dave is sometimes a bit swamped, but pinging him on irc about lost patches works well (at least for stuff I care about).

Of course, I share the idea of true, full-blown of_drm_something helpers. But the DT patch, is not an improvement but a real fix as in "make DRM not break on some platforms". From that on, I can start digging into DRM API and improve DT support here and there.

So for the three patches I mentioned above, they are all minor improvements of the API. Minor, because I cannot ever sent _the_ single perfect patch just because I don't know the API well enough, yet.

Just based on my personal experience: TDA998x driver got merged the way it is _although_ I addressed some concerns - the fixes are not merged _although_ I provided experimental results. This is of course disappointing for me, but I am sure it will work out soon and I will get back to happily send improvements for the platforms I can test on.

Quoted 9 lines

Of course, I share the idea of true, full-blown of_drm_something helpers. But the DT patch, is not an improvement but a real fix as in "make DRM not break on some platforms". From that on, I can start digging into DRM API and improve DT support here and there.

So for the three patches I mentioned above, they are all minor improvements of the API. Minor, because I cannot ever sent _the_ single perfect patch just because I don't know the API well enough, yet.

Quoted 5 lines

Just based on my personal experience: TDA998x driver got merged the way it is _although_ I addressed some concerns - the fixes are not merged _although_ I provided experimental results. This is of course disappointing for me, but I am sure it will work out soon and I will get back to happily send improvements for the platforms I can test on.

To be honest I've no idea what those tda998x patches could fix or break, so they go on my no ideas list and I hope if they get reposted someone will tell me what they do.

I could probably be more motivated towards poking other people to review stuff, but I mostly hope in vain that the ARM people will cross review things for other ARM chips, I had a bit of that happening for a while at the start, but it seems to have died off. Now I'm mostly relying on Rob to have some clue what I'm merging is sane.

My current priority for merging stuff is:
core patches that affect all platforms,
core patches that affect x86
drivers that I maintain by default
core patches that affect ARM
misc ARM drivers that fall through cracks.

Maybe I can persuade Rob to become a sub maintainer for all of the SoC drivers but I suspect he'd try and hurt me in real life.

I'll take another look at the tda patches but I may still have no idea what they are doing.

Dave.
_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Quoted 5 lines · Dave Airlie

how about instead of writing:
"However, at least I've taken the time to_think_ about what I'm doing and realise that there_is_ scope here for the DRM core to improve,

Quoted 7 lines

rather than burying this stuff deep inside my driver like everyone else has. That's no reason to penalise patches from the "good guys" who think"

you go with
"I noticed this piece of functionality could be refactored, here is a patch adding them to
the core, does anyone think its a good idea?"

Dave,

at least on this point I do share Russell's impression. I've sent bunch of patches improving TDA998x and DRM+DT:
- TDA998x irq handling - ignored
- TDA998x sync fix - ignored

- Fix drm I2C slave encoder probing

I am aware that this is not an easy job nor one you get much appreciation for. But, back when TDA998x driver was published, all my comments were basically answered with "Oh, I know. Maybe someday somebody will fix it".

I am not being paid for any of this, but have a strong intrinsic motivation here. But I am loosing interest in sending fixes for DRM stuff because my (personal) impression is the same Russell
has: Depending on who sends patches, they get merged independent of how broken they are - others are discussed to death.

Sebastian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Quoted 2 lines · Sebastian Hesselbarth

- TDA998x irq handling - ignored
- TDA998x sync fix - ignored

At least the sync fix, looks like I missed it (it probably is a good idea to CC me if you want me to look at it). Looks like there was some follow-up discussion on both patches, unless I missed seeing a newer version of those patches.

Sometimes if you think a patch has been ignored/forgotten, it doesn't hurt to ping on mailing list or #dri-devel.. a lot of us are working not just on kernel (the relatively small part in the whole linux graphics stack), but also mesa and/or x11. Some times things end up several pages down in the mail folder. It's not because we are all sitting on a beach drinking margaritas, or because we don't like you. It is just because we are busy and missed it.

Last few months I've been pretty buried in r/e + gallium driver for new gpu, so I wasn't always checking dri-devel list every day. At least now I am in drm-driver mode again ;-)

Quoted 2 lines

- Fix drm I2C slave encoder probing

If you have a better idea about how to make the slave encoder probing better (and/or more generic to support stuff other than i2c), please send RFC patch. (And if you already did this, please send updated version, see previous point about sometimes missing patches.)

This patch adds support for the pair of LCD controllers on the Marvell Armada 510 SoCs. This driver supports:
- multiple contiguous scanout buffers for video and graphics
- shm backed cacheable buffer objects for X pixmaps for Vivante GPU acceleration
- dual lcd0 and lcd1 crt operation
- video overlay on each LCD crt
- page flipping of the main scanout buffers

Included in this commit is the core driver with no output support; output support is platform and encoder driver dependent.

Signed-off-by Russell King <[email protected]>

drivers/gpu/drm/armada/armada_gem.c new file +54 -0

+int
+armada_gem_linear_back(struct drm_device *dev, struct armada_gem_object *obj)
+{
+	struct armada_private *priv = dev->dev_private;
+	size_t size = obj->obj.size;
+
+	if (obj->page || obj->linear)
+		return 0;
+
+	/*
+	 * If it is a small allocation (typically cursor, which will
+	 * be 32x64 or 64x32 ARGB pixels) try to get it from the system.
+	 * Framebuffers will never be this small (our minimum size for
+	 * framebuffers is larger than this anyway.)  Such objects are
+	 * only accessed by the CPU so we don't need any special handing
+	 * here.
+	 */
+	if (size <= 8192) {
+		unsigned int order = get_order(size);
+		struct page *p = alloc_pages(GFP_KERNEL, order);
+
+		if (p) {
+			obj->addr = page_address(p);
+			obj->phys_addr = page_to_phys(p);
+			obj->page = p;
+
+			memset(obj->addr, 0, PAGE_ALIGN(size));
+		}
+	}
+
+	/*
+	 * We could grab something from CMA if it's enabled, but that
+	 * involves building in a problem:
+	 *
+	 * CMA's interface uses dma_alloc_coherent(), which provides us
+	 * with an CPU virtual address and a device address.
+	 *
+	 * The CPU virtual address may be either an address in the kernel
+	 * direct mapped region (for example, as it would be on x86) or
+	 * it may be remapped into another part of kernel memory space
+	 * (eg, as it would be on ARM.)  This means virt_to_phys() on the
+	 * returned virtual address is invalid depending on the architecture
+	 * implementation.
+	 *
+	 * The device address may also not be a physical address; it may
+	 * be that there is some kind of remapping between the device and
+	 * system RAM, which makes the use of the device address also
+	 * unsafe to re-use as a physical address.
+	 *
+	 * This makes DRM usage of dma_alloc_coherent() in a generic way
+	 * at best very questionable and unsafe.
+	 */
+
+	/* Otherwise, grab it from our linear allocation */

drivers/gpu/drm/armada/armada_gem.c line 142

Quoted 5 lines · Russell King

drivers/gpu/drm/armada/armada_gem.c new file +5 -0

+		mutex_lock(&dev->struct_mutex);
+		free = drm_mm_search_free(&priv->linear, size, align, 0);
+		if (free)
+			obj->linear = drm_mm_get_block(free, size, align);
+		mutex_unlock(&dev->struct_mutex);

The two-stage search_free+get_block drm_mm interface is something I'd like to remove since it' complicates the interface for no gain. And the preallocation trick for atomic contexts is pretty racy as-is. Can you please convert this over to the insert_node interfaces which take a preallocate drm_mm_node?

Quoted 12 lines · Daniel Vetter

drivers/gpu/drm/armada/armada_gem.c line 142

Quoted 5 lines · Russell King

drivers/gpu/drm/armada/armada_gem.c new file +5 -0

+		mutex_lock(&dev->struct_mutex);
+		free = drm_mm_search_free(&priv->linear, size, align, 0);
+		if (free)
+			obj->linear = drm_mm_get_block(free, size, align);
+		mutex_unlock(&dev->struct_mutex);

The two-stage search_free+get_block drm_mm interface is something I'd like to remove since it' complicates the interface for no gain. And the preallocation trick for atomic contexts is pretty racy as-is. Can you please convert this over to the insert_node interfaces which take a preallocate drm_mm_node?

Yes and no. This is only racy if it is callable from atomic contexts, which the above isn't, because it takes a mutex, and the above is the only place which allocations against that pool are done. Mutexes can't be taken in atomic contexts. Plus it's using the non-atomic drm_mm_* interfaces.

However, the insert_node interfaces appear not to solve any kind of race. All they do is allow the kmalloc to be moved out of this region into the user of this code sequence, while offering no additional locking or safety. So the mutex lock around a call to drm_mm_insert_node*() is still required.

As the kmalloc() happens beneath the mutex lock, another thread can't race with an allocation in the above code anyway. The *only* reason I'll change this is that it is nicer to the system not to hold locks while allocating memory.

Quoted 26 lines · Russell King - ARM Linux

Quoted 7 lines · Daniel Vetter

drivers/gpu/drm/armada/armada_gem.c line 142

Quoted 5 lines · Russell King

drivers/gpu/drm/armada/armada_gem.c new file +5 -0

+         mutex_lock(&dev->struct_mutex);
+         free = drm_mm_search_free(&priv->linear, size, align, 0);
+         if (free)
+                 obj->linear = drm_mm_get_block(free, size, align);
+         mutex_unlock(&dev->struct_mutex);

Quoted 5 lines

The two-stage search_free+get_block drm_mm interface is something I'd like to remove since it' complicates the interface for no gain. And the preallocation trick for atomic contexts is pretty racy as-is. Can you please convert this over to the insert_node interfaces which take a preallocate drm_mm_node?

Yes and no. This is only racy if it is callable from atomic contexts, which the above isn't, because it takes a mutex, and the above is the only place which allocations against that pool are done. Mutexes can't be taken in atomic contexts. Plus it's using the non-atomic drm_mm_* interfaces.

However, the insert_node interfaces appear not to solve any kind of race. All they do is allow the kmalloc to be moved out of this region into the user of this code sequence, while offering no additional locking or safety. So the mutex lock around a call to drm_mm_insert_node*() is still required.

Quoted 4 lines

As the kmalloc() happens beneath the mutex lock, another thread can't race with an allocation in the above code anyway. The *only* reason I'll change this is that it is nicer to the system not to hold locks while allocating memory.

Err. There's bugs here in the other drivers such as i915 which follow your suggestion. The problem is this:

Every time they allocate using drm_mm_insert_node(), they kmalloc a new struct drm_mm_node for this function to fill in and attach.

When they've done with the allocation, they call drm_mm_put_block(). This places the node onto the mm's unused_nodes list provided we don't already have MM_UNUSED_TARGET nodes on that list.

So, we end up filling up the unused nodes list to MM_UNUSED_TARGET, at which point we then start freeing any further nodes - and with no way to pull these off that list, they all just sit there not being used.

The only function which takes nodes off that list is drm_mm_kmalloc(), which is declared statically, and so isn't available to drivers.

Are drivers which use the drm_mm_insert_node() function expected to also use drm_mm_remove_node(), kfreeing the node after that call, rather than using drm_mm_put_block() ?

Either way, I think someone needs to fix the other drivers and Cc those patches to Stable...