[Intel-gfx] [PATCH 1/8] drm/i915: Implement .get_format_info() hook for CCS

[Ville Syrjälä]

On Wed, Jun 07, 2017 at 12:44:47PM +0100, Daniel Stone wrote:
> Hi Vidya,
> 
> On 7 June 2017 at 12:40, Vidya Srinivas <vidya.srinivas at intel.com> wrote:
> > +static const struct drm_format_info ccs_formats[] = {
> > +       { .format = DRM_FORMAT_XRGB8888, .depth = 24, .num_planes = 2, .cpp = { 4, 1, }, .hsub = 16, .vsub = 8, },
> > +       { .format = DRM_FORMAT_XBGR8888, .depth = 24, .num_planes = 2, .cpp = { 4, 1, }, .hsub = 16, .vsub = 8, },
> > +       { .format = DRM_FORMAT_ARGB8888, .depth = 32, .num_planes = 2, .cpp = { 4, 1, }, .hsub = 16, .vsub = 8, },
> > +       { .format = DRM_FORMAT_ABGR8888, .depth = 32, .num_planes = 2, .cpp = { 4, 1, }, .hsub = 16, .vsub = 8, },
> > +};
> 
> I notice that here hsub/vsub are declared as 16x8. In Ville's tree
> which I pulled my submission from, this is 8x16, which aligns with
> this comment (missing from your submission):
> /*
>  * 1 byte of CCS actually corresponds to 16x8 pixels on the main
>  * surface, and the memory layout for the CCS tile us 64x64 bytes.
>  * But since we're pretending the CCS tile is 128 bytes wide we
>  * adjust hsub/vsub here accordingly to 8x16 so that the
>  * bytes<->x/y conversions come out correct.
>  */
> 
> If the hsub/vsub is inverted to match the comment, trying to add a
> 3200x1800 (for example) framebuffer will fail, because (1800 % 16) !=
> 0. This is true even if the allocation is correct (i.e. the buffer
> contains an even number of tiles for both width and height). Generic
> userspace cannot know that it should try to create a larger
> framebuffer (in this case 3200x1808) and only show a smaller region of
> that framebuffer.
> 
> This is the reason for the halign/valign patch mentioned earlier, as
> well as the additional part of the comment which was also present in
> my submission:
> /*
>  * We don't require any
>  * CCS block size alignment of the fb under the assumption that the
>  * hardware will handle things correctly of only a single pixel
>  * gets touched. The compression should be lossless so any garbage
>  * pixels as part of the same block shouldn't cause visual artifacts.
>  */

The alignment requirement is gone in upstream, hence my latest CCS
stuff doesn't have the valign/halign stuff anymore.

Anyways, I'll have to revisit the the offsets[] thing because people
didn't like my original linear offset idea, and it doesn't match what
userspace already does.

And I still need to convince myself that the ccs hash mode won't be
an issue, which I think I'm close to doing since I managed to trick
igt rendercopy to do ccs.

-- 
Ville Syrjälä
Intel OTC