On Sun, Sep 18, 2011 at 3:31 PM, Thomas Hellstrom <thomas@xxxxxxxxxxxx> wrote: > On 09/18/2011 10:15 PM, Rob Clark wrote: >> >> On Sun, Sep 18, 2011 at 3:00 PM, Thomas Hellstrom<thomas@xxxxxxxxxxxx> >> wrote: >> >>> >>> On 09/18/2011 09:50 PM, Rob Clark wrote: >>> >>>> >>>> On Sun, Sep 18, 2011 at 2:36 PM, Thomas Hellstrom<thomas@xxxxxxxxxxxx> >>>> wrote: >>>> >>>> >>>>> >>>>> On 09/17/2011 11:32 PM, Rob Clark wrote: >>>>> >>>>> >>>>>> >>>>>> From: Rob Clark<rob@xxxxxx> >>>>>> >>>>>> 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@xxxxxx> >>>>>> + * >>>>>> + * 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@xxxxxxxxxxxxxxxxxxxxx >>> http://lists.freedesktop.org/mailman/listinfo/dri-devel >>> >>> > > > > _______________________________________________ > dri-devel mailing list > dri-devel@xxxxxxxxxxxxxxxxxxxxx > http://lists.freedesktop.org/mailman/listinfo/dri-devel > _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
