Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <cover.8ec406bf8f4f097e9dc909d5aac466556822f592.1555487650.git-series.maxime.ripard@bootlin.com>
 <20190417154121.GJ13337@phenom.ffwll.local> <20190418062229.eyog4i62eg4pr6uf@flea>
 <CAKMK7uHwSjqXwWGt+wQ6oMFWoPqmBxYHL7r+vTOXdYt9KMCYLQ@mail.gmail.com>
 <20190418090221.e57dogn4yx5nwdni@flea> <CAKMK7uHN6QfYyzx4DjRT+7VLRi6N9DorQtw2GoDiNGTgie=DXA@mail.gmail.com>
 <20190420225904.GZ4964@pendragon.ideasonboard.com> <20190423072554.GW13337@phenom.ffwll.local>
In-Reply-To: <20190423072554.GW13337@phenom.ffwll.local>
From:   Daniel Stone <daniel@fooishbar.org>
Date:   Tue, 23 Apr 2019 09:59:37 +0100
Message-ID: <CAPj87rMWdjEvTJWRvdMLTZyw7pZpo5PpPr0FWcALNRe_D8cYqw@mail.gmail.com>
Subject: Re: [PATCH 00/20] drm: Split out the formats API and move it to a
 common place
To:     Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
        Maxime Ripard <maxime.ripard@bootlin.com>,
        Daniel Vetter <daniel.vetter@intel.com>,
        David Airlie <airlied@linux.ie>,
        Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
        Sean Paul <seanpaul@chromium.org>,
        Mauro Carvalho Chehab <mchehab@kernel.org>,
        Sakari Ailus <sakari.ailus@linux.intel.com>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        dri-devel <dri-devel@lists.freedesktop.org>,
        Paul Kocialkowski <paul.kocialkowski@bootlin.com>,
        Hans Verkuil <hans.verkuil@cisco.com>,
        Thomas Petazzoni <thomas.petazzoni@bootlin.com>,
        "open list:DMA BUFFER SHARING FRAMEWORK" 
        <linux-media@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi,

On Tue, 23 Apr 2019 at 08:26, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Sun, Apr 21, 2019 at 01:59:04AM +0300, Laurent Pinchart wrote:
> > > > > - drm fourcc code doesn't actually define the drm_format_info
> > > > >   uniquely, drivers can override that (that's an explicit design
> > > > >   intent of modifiers, to allow drivers to add another plane for
> > > > >   e.g. compression information). You'd need to pull that driver
> > > > >   knowledge into your format library.
> >
> > That's a mistake in my opinion. We tried that in V4L2 to store metadata
> > in a separate plane, and had to go another route eventually as it
> > created a very bad mess.
>
> Just quick clarification in the middle here: This is how the hw works.
> It's not metadata that sw ever touches (in general, testcases to make sure
> we display these correctly excepted).
>
> There has been some talking to add maybe a bit more mixed metadata, for
> fast-clear colors (which isn't used by any display engine afaik yet). That
> would generally be written by the cpu (in the gl stack), but again read by
> the hw (loaded as indirect state packet most likely, or something like
> that). So again hw specific layout, because the hw needs to read it.
>
> Pure metadata only of interest for the cpu/sw stack has been shot down
> completely on the drm side too.

Totally. Let's take DRM_FORMAT_XRGB8888 + I915_FORMAT_MOD_Y_TILED as
an example. Here, there is one colour plane which is laid out in a
documented tiled format, containing normal XRGB8888 pixels once you do
the maths to get the correct pixel location. So that's fine.

I915_FORMAT_MOD_Y_TILED_CCS has a base colour plane as above, but adds
an auxiliary plane which has a few bits describing the state of every
(differently-sized) tile. Before reading the tile from the colour
plane, you look at the corresponding location in the auxiliary plane:
if you read 0x55 from the auxiliary plane, then the entire cacheline
is the value of the first pixel, i.e. a solid fill. Hardware takes
advantage of this to only write out the first pixel: if you try to
read the colour plane as Y_TILED then for solid-filled regions, only
the first pixel of every tile will show correctly, and the rest will
be garbage.

The auxiliary plane has its own layout and placement requirements, so
we need to carry around an offset and a stride for the auxiliary data.
We already have this for multiple planes; stuffing it into the base
plane would require us to reinvent the same for auxiliary data within
a single plane.

I understand at least one of the Tegra colour-compression layouts (for
Tegra 1xx?) is similar to this.

It would be good to understand what you had in mind when you said that
using multiple planes created a mess. I haven't touched media
encode/decode units at a low level for quite a while (hooray for
gst-v4l2!), but I remember that they often used padding areas around
the buffer for scratch space - maybe motion vectors or similar? That
case is quite different to something like CCS, since the data is only
meaningful to the media engine and must be ignored (but preserved) by
everyone else. Using multiple planes in that case isn't appropriate,
since it's very specific to how that hardware unit deals with that
buffer, instead of something that every consumer needs to understand
in order to use it.

Cheers,
Daniel