On Sat, Jun 29, 2013 at 11:53:22PM +0100, Russell King wrote:
> 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 <rmk+kernel at arm.linux.org.uk>
Upfront disclaimer: I have no clue about ARM/DT integration issues, so I
don't have any opinion/comments on those.
I've spotted a few other things though, see below. With those addressed
this patch is
Reviewed-by: Daniel Vetter <daniel.vetter at ffwll.ch>
Cheers, Daniel
> ---
[snip]
> +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;
> + }
> +
> + /*
> + * 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).
> +
> + return val;
> +}
> +
[snip]
> +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) {
> + 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.
> +
> + 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]
> +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);
> + 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 ...
> +
> + 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;
> +}
[snip]
> +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) {
> + 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 <dmitry.v.rogozhkin at intel.com>
Date: Wed Oct 3 17:15:26 2012 +0300
drm/i915: EBUSY status handling added to i915_gem_fault().
> + return VM_FAULT_SIGBUS;
> + }
> +}
[snip]
> diff --git a/drivers/gpu/drm/armada/drm_helper.h
> b/drivers/gpu/drm/armada/drm_helper.h
> new file mode 100644
> index 0000000..d9f2e8d
> --- /dev/null
> +++ b/drivers/gpu/drm/armada/drm_helper.h
> @@ -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.
> + */
> +#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 ;-)
> +
> +#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
> --
> 1.7.4.4
>
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
