On Wed, Jun 11, 2014 at 10:43:30AM -0700, Stéphane Marchesin wrote:
> On Tue, Jun 10, 2014 at 4:04 AM, Thierry Reding
> <thierry.reding@gmail.com> wrote:
> > From: Thierry Reding <treding@nvidia.com>
> >
> > Currently the tiling parameters of buffer objects can only be set at
> > allocation time, and only a single tiled mode is supported. This new
> > DRM_TEGRA_GEM_SET_TILING IOCTL allows more modes to be set and also
> > allows the tiling mode to be changed after the allocation. This will
> > enable the Tegra DRM driver to import buffers from a GPU and directly
> > scan them out by configuring the display controller appropriately.
> 
> I was wondering why not support the new tiling modes on creation as
> well, I guess because you don't have room for height in the creation
> ioctl?

Yes, back at the time when the GEM_CREATE IOCTL was added we only left
enough room for "flags", so it'll be difficult to add support for the
newer tiling modes.

One possibility that I see is to extend struct drm_tegra_gem_create in a
backwards-compatible way and make new fields dependent on some new flag,
somewhat like this:

	#define DRM_TEGRA_GEM_CREATE_BLOCK_LINEAR (1 << 3)

	struct drm_tegra_gem_create {
		__u64 size;
		__u32 flags;
		__u32 handle;
		__u32 height;
		__u32 pad;
	};

Then the DRM driver can parse the height field only if the BLOCK_LINEAR
flag is set.

To be honest, though, I'd prefer to deprecate the flags at creation if
these patches get merged so that we can have a standard way of setting
these rather than a few.

Do you see any particular advantages in specifying the tiling mode at
creation time?

> > diff --git a/include/uapi/drm/tegra_drm.h b/include/uapi/drm/tegra_drm.h
> > index b75482112428..0829f75eb986 100644
> > --- a/include/uapi/drm/tegra_drm.h
> > +++ b/include/uapi/drm/tegra_drm.h
> > @@ -129,6 +129,27 @@ struct drm_tegra_submit {
> >         __u32 reserved[5];      /* future expansion */
> >  };
> >
> > +#define DRM_TEGRA_GEM_TILING_MODE_PITCH 0
> > +#define DRM_TEGRA_GEM_TILING_MODE_TILED 1
> > +#define DRM_TEGRA_GEM_TILING_MODE_BLOCK 2
> > +
> > +struct drm_tegra_gem_set_tiling {
> > +       /* input */
> > +       __u32 handle;
> > +       __u32 mode;
> > +       __u32 value;
> 
> This seems to only be used for height, if so should it be called height?

The idea here was to have a generic name here so that it could be more
easily extended if/when newer tiling modes are introduced.

Thierry