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

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

 



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.


/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


[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