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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux