[PATCH] RFCv2: omapdrm DRM/KMS driver for TI OMAP platforms
Rob Clark
rob.clark at linaro.org
Sun Sep 18 14:03:52 PDT 2011
On Sun, Sep 18, 2011 at 3:31 PM, Thomas Hellstrom <thomas at shipmail.org> wrote:
> 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.
Ok, gotcha, thx for the explaination.. I'll add a couple of pad fields
where needed
BR,
-R
>
> /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
>>>
>>>
>
>
>
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>
More information about the dri-devel
mailing list