Re: [PATCH 3/3] drm: Make drm_local_map use a resource_size_t offset

[Eric Anholt <eric@anholt.net>]

[-- Attachment #1: Type: text/plain, Size: 2805 bytes --]

On Mon, 2009-02-02 at 16:55 +1100, Benjamin Herrenschmidt wrote:
> This changes drm_local_map to use a resource_size for its "offset"
> member instead of an unsigned long, thus allowing 32-bit machines
> with a >32-bit physical address space to be able to store there
> their register or framebuffer addresses when those are above 4G,
> such as when using a PCI video card on a recent AMCC 440 SoC.
> 
> This patch isn't as "trivial" as it sounds: A few functions needed
> to have some unsigned long/int changed to resource_size_t and a few
> printk's had to be adjusted.
> 
> But also, because userspace isn't capable of passing such offsets,
> I had to modify drm_find_matching_map() to ignore the offset passed
> in for maps of type _DRM_FRAMEBUFFER or _DRM_REGISTERS.
> 
> If we ever support multiple _DRM_FRAMEBUFFER or _DRM_REGISTERS maps
> for a given device, we might have to change that trick, but I don't
> think that happens on any current driver.
> 
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>

Just one little comment I think then I'd be happy:

> ---
> 
>  drivers/gpu/drm/drm_bufs.c         |   33 ++++++++++++++++++++++++---------
>  drivers/gpu/drm/drm_proc.c         |    4 ++--
>  drivers/gpu/drm/drm_vm.c           |   22 ++++++++++++----------
>  drivers/gpu/drm/mga/mga_dma.c      |   17 +++++++++--------
>  drivers/gpu/drm/mga/mga_drv.h      |    4 ++--
>  drivers/gpu/drm/r128/r128_cce.c    |    7 ++++---
>  drivers/gpu/drm/radeon/radeon_cp.c |    9 +++++----
>  include/drm/drmP.h                 |   12 ++++++------
>  8 files changed, 64 insertions(+), 44 deletions(-)
> 
> --- linux-work.orig/drivers/gpu/drm/drm_bufs.c	2009-02-02 16:29:54.000000000 +1100
> +++ linux-work/drivers/gpu/drm/drm_bufs.c	2009-02-02 16:29:54.000000000 +1100
> @@ -54,11 +54,25 @@ static struct drm_map_list *drm_find_mat
>  {
>  	struct drm_map_list *entry;
>  	list_for_each_entry(entry, &dev->maplist, head) {
> -		if (entry->map && (entry->master == dev->primary->master) && (map->type == entry->map->type) &&
> -		    ((entry->map->offset == map->offset) ||
> -		     ((map->type == _DRM_SHM) && (map->flags&_DRM_CONTAINS_LOCK)))) {
> +		/* Due to userspace API breakage, we ignore the map offset
> +		 * for maps of type _DRM_FRAMEBUFFER or _DRM_REGISTERS
> +		 */

Could this comment instead be maybe:

Because the kernel-userspace ABI is fixed at a 32-bit offset, while PCI
resources may live above that, we ignore the map offset for maps of type
_DRM_FRAMEBUFFER or _DRM_REGISTERS.  It is assumed that each driver will
have only one resource of each type.

(I want to remember later what exact ABI problem was in question)

-- 
Eric Anholt
eric@anholt.net                         eric.anholt@intel.com



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 197 bytes --]