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

[Thomas Hellstrom <thomas@shipmail.org>]

On 09/18/2011 09:50 PM, Rob Clark wrote:
> On Sun, Sep 18, 2011 at 2:36 PM, Thomas Hellstrom<thomas@shipmail.org>  wrote:
>    
>> On 09/17/2011 11:32 PM, 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/
>>>
>>> History:
>>>
>>> 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
>>> ---
>>>   drivers/staging/Kconfig                  |    2 +
>>>   drivers/staging/Makefile                 |    1 +
>>>   drivers/staging/omapdrm/Kconfig          |   24 +
>>>   drivers/staging/omapdrm/Makefile         |    9 +
>>>   drivers/staging/omapdrm/TODO.txt         |   14 +
>>>   drivers/staging/omapdrm/omap_connector.c |  357 ++++++++++++++
>>>   drivers/staging/omapdrm/omap_crtc.c      |  332 +++++++++++++
>>>   drivers/staging/omapdrm/omap_drv.c       |  766
>>> ++++++++++++++++++++++++++++++
>>>   drivers/staging/omapdrm/omap_drv.h       |  126 +++++
>>>   drivers/staging/omapdrm/omap_encoder.c   |  188 ++++++++
>>>   drivers/staging/omapdrm/omap_fb.c        |  259 ++++++++++
>>>   drivers/staging/omapdrm/omap_fbdev.c     |  309 ++++++++++++
>>>   drivers/staging/omapdrm/omap_gem.c       |  720
>>> ++++++++++++++++++++++++++++
>>>   drivers/video/omap2/omapfb/Kconfig       |    2 +-
>>>   include/drm/Kbuild                       |    1 +
>>>   include/drm/omap_drm.h                   |  111 +++++
>>>   include/drm/omap_priv.h                  |   42 ++
>>>   17 files changed, 3262 insertions(+), 1 deletions(-)
>>>   create mode 100644 drivers/staging/omapdrm/Kconfig
>>>   create mode 100644 drivers/staging/omapdrm/Makefile
>>>   create mode 100644 drivers/staging/omapdrm/TODO.txt
>>>   create mode 100644 drivers/staging/omapdrm/omap_connector.c
>>>   create mode 100644 drivers/staging/omapdrm/omap_crtc.c
>>>   create mode 100644 drivers/staging/omapdrm/omap_drv.c
>>>   create mode 100644 drivers/staging/omapdrm/omap_drv.h
>>>   create mode 100644 drivers/staging/omapdrm/omap_encoder.c
>>>   create mode 100644 drivers/staging/omapdrm/omap_fb.c
>>>   create mode 100644 drivers/staging/omapdrm/omap_fbdev.c
>>>   create mode 100644 drivers/staging/omapdrm/omap_gem.c
>>>   create mode 100644 include/drm/omap_drm.h
>>>   create mode 100644 include/drm/omap_priv.h
>>>
>>>
>>>        
>> ...
>>
>>      
>>> diff --git a/include/drm/omap_drm.h b/include/drm/omap_drm.h
>>> new file mode 100644
>>> index 0000000..ea0ae8e
>>> --- /dev/null
>>> +++ b/include/drm/omap_drm.h
>>> @@ -0,0 +1,111 @@
>>> +/*
>>> + * linux/include/drm/omap_drm.h
>>> + *
>>> + * Copyright (C) 2011 Texas Instruments
>>> + * Author: Rob Clark<rob@ti.com>
>>> + *
>>> + * 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.
>>> + *
>>> + * This program is distributed in the hope that it will be useful, but
>>> WITHOUT
>>> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>>> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
>>> for
>>> + * more details.
>>> + *
>>> + * You should have received a copy of the GNU General Public License
>>> along with
>>> + * this program.  If not, see<http://www.gnu.org/licenses/>.
>>> + */
>>> +
>>> +#ifndef __OMAP_DRM_H__
>>> +#define __OMAP_DRM_H__
>>> +
>>> +#include "drm.h"
>>> +
>>> +/* Please note that modifications to all structs defined here are
>>> + * subject to backwards-compatibility constraints.
>>> + */
>>> +
>>> +#define OMAP_PARAM_CHIPSET_ID  1       /* ie. 0x3430, 0x4430, etc */
>>> +
>>> +struct drm_omap_param {
>>> +       uint64_t param;                 /* in */
>>> +       uint64_t value;                 /* in (set_param), out (get_param)
>>> */
>>> +};
>>> +
>>> +struct drm_omap_get_base {
>>> +       char plugin_name[64];           /* in */
>>> +       uint32_t ioctl_base;            /* out */
>>> +};
>>>
>>>        
>> What about  future ARM 64-bit vs 32-bit structure sizes? On x86 we always
>> take care to make structures appearing in the drm user-space interfaces
>> having sizes that are a multiple of 64-bits, to avoid having to maintain
>> compat code for  32-bit apps running on 64 bit kernels. For the same
>> reasons, structure members with 64 bit alignment requirements on 64-bit
>> systems need to be carefully places.
>>
>> I don't know whether there is or will be a 64-bit ARM, but it might be worth
>> taking into consideration.
>>      
> There isn't currently any 64-bit ARM, but it is a safe assumption that
> there will be some day..  I guess we'll have enough fun w/ various
> random 32b devices when LPAE arrives w/ the cortex-a15..
>
> I did try to make sure any uint64_t's were aligned to a 64bit offset,
> but beyond that I confess to not being an expert on how 64 vs 32b
> ioctl compat stuff is handled or what the issues going from 32->64b
> are.  If there are some additional considerations that should be taken
> care of, then now is the time.  So far I don't have any pointer fields
> in any of the ioctl structs.  Beyond that, I'm not entirely sure what
> else needs to be done, but would appreciate any pointers to docs about
> how the compat stuff works.
>
> BR,
> -R
>    

I've actually avoided writing compat ioctl code myself, by trying to 
make sure that structures look identical in the 64-bit and 32-bit x86 
ABIs, but the compat code is there to translate pointers and to overcome 
alignment incompatibilities.

A good way to at least try to avoid having to maintain compat code once 
the 64-bit ABI shows up is to make sure that 64-bit scalars and embedded 
structures are placed on 64-bit boundaries, padding if necessary, and to 
make sure (using padding) that struct sizes are always multiples of 64 bits.

But since there is no 64-bit ARM yet, it might be better to rely on 
using compat code in the future than on making guesses about alignment 
restrictions of the ABI...

/Thomas