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

[Thomas Hellstrom <thomas@shipmail.org>]

On 09/18/2011 10:15 PM, Rob Clark wrote:
> On Sun, Sep 18, 2011 at 3:00 PM, Thomas Hellstrom<thomas@shipmail.org>  wrote:
>    
>> 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.
>>      
> So far this is true for 64bit scalars.. I'm using stdint types
> everywhere so there is no chance for fields having different sizes on
> 64b vs 32b.  And the only structs contained within ioctl structs so
> far are starting at offset==0.
>
> Is it necessary to ensure that the ioctl structs themselves (as
> opposed to structs within those structs) have sizes that are multiple
> of 64b?  The ioctl structs are copied
> (copy_from_user()/copy_to_user()), which I would have assumed would be
> sufficient?
>
>    

It depends. If a 64 bit kernel calculates the size as sizeof(struct ...) 
and
then tries to copy it to user-space using copy_to_user(), it might want 
to copy more
than the user-space structure size, causing -EFAULTs or overwritten 
user-space data. (If user-space is 32-bit.)

On x86, for example

struct  {
    int64_t a;
    int32_t b;
} x;

Is 96 byte in the 32 bit ABI, but 128 byte in the 64-bit ABI. So if you 
issue

copy_to_user(user_ptr, &x, sizeof(x))

It will try to copy 128 byte on a 64-bit kernel and will overwrite data 
or cause segfault with a 32-bit user-space.


However, IIRC the drm ioctl copy code uses the size encoded by 
user-space, which avoids that problem, but that's an implementation 
specific feature that shouldn't be relied upon.


/Thomas


>> 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...
>>      
> hmm, it might be nice to get some guidelines from ARM on this, since I
> really have no idea what a 64b ARM architecture would look like..
>
> BR,
> -R
>
>    
>> /Thomas
>>
>>
>>
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>>
>>