Re: [PATCH] DRM: omapdrm DRM/KMS driver for TI OMAP platforms

[Daniel Vetter <daniel@ffwll.ch>]

On Sun, Oct 23, 2011 at 05:35:28PM -0500, Rob Clark wrote:
> From: Rob Clark <rob@ti.com>
> 
> A DRM display driver for TI OMAP platform.  Similar to omapfb (fbdev)
> and omap_vout (v4l2 display) drivers in the past, this driver uses the
> DSS2 driver to access the display hardware, including support for
> HDMI, DVI, and various types of LCD panels.  And it implements GEM
> support for buffer allocation (for KMS as well as offscreen buffers
> used by the xf86-video-omap userspace xorg driver).
> 
> The driver maps CRTCs to overlays, encoders to overlay-managers, and
> connectors to dssdev's.  Note that this arrangement might change slightly
> when support for drm_plane overlays is added.
> 
> For GEM support, non-scanout buffers are using the shmem backed pages
> provided by GEM core (In drm_gem_object_init()).  In the case of scanout
> buffers, which need to be physically contiguous, those are allocated
> with CMA and use drm_gem_private_object_init().
> 
> See userspace xorg driver:
> git://github.com/robclark/xf86-video-omap.git
> 
> Refer to this link for CMA (Continuous Memory Allocator):
> http://lkml.org/lkml/2011/8/19/302
> 
> Links to previous versions of the patch:
> v1: http://lwn.net/Articles/458137/
> v2: http://lwn.net/Articles/459397/
> 
> History:
> 
> v4: bit of rework of encoder/connector _dpms() code, modeset_init()
>     rework to not use nested functions, update TODO.txt
> v3: minor cleanups, improved error handling for dev_load(), some minor
>     API changes that will be needed later for tiled buffer support
> v2: replace omap_vram with CMA for scanout buffer allocation, remove
>     unneeded functions, use dma_addr_t for physical addresses, error
>     handling cleanup, refactor attach/detach pages into common drm
>     functions, split non-userspace-facing API into omap_priv.h, remove
>     plugin API
> 
> v1: original
> 
> Signed-off-by: Rob Clark <rob@ti.com>

I think this has already seen way too many iterations for a simple
-staging merge ... ;-)

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> (for -staging)

> +	/* if we couldn't find another connected connector, lets start
> +	 * looking at the unconnected connectors:
> +	 *
> +	 * note: it might not be immediately apparent, but thanks to
> +	 * the !mgr check in both this loop and the one above, the only
> +	 * way to enter this loop is with *j == priv->num_connectors,
> +	 * so idx can never go negative.
> +	 */
> +	while (*j < 2 * priv->num_connectors && !mgr) {

This loop here is still pretty ... strange. Might want to clean that up as
the first patch when merged. Why no just a simple?
	for (j = 0; j < num_connectors; j++)
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48