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 >
