On 09/18/2011 10:15 PM, Rob Clark wrote:
> On Sun, Sep 18, 2011 at 3:00 PM, Thomas Hellstrom<thomas at 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 at shipmail.org>
>>> wrote:
>>>
>>>
>>>> On 09/17/2011 11:32 PM, Rob Clark wrote:
>>>>
>>>>
>>>>> From: Rob Clark<rob at 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 at 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 at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>>
>>
