[Intel-gfx] [PATCH 15/18] drm/i915: Add a new "remapped" gtt_view

[Chris Wilson]

Quoting Ville Syrjälä (2018-07-19 20:33:57)
> On Thu, Jul 19, 2018 at 07:59:33PM +0100, Chris Wilson wrote:
> > Quoting Ville Syrjala (2018-07-19 19:22:11)
> > > +static struct scatterlist *
> > > +remap_pages(const dma_addr_t *in, unsigned int offset,
> > > +           unsigned int width, unsigned int height,
> > > +           unsigned int stride,
> > > +           struct sg_table *st, struct scatterlist *sg)
> > > +{
> > > +       unsigned int column, row;
> > > +
> > > +       for (row = 0; row < height; row++) {
> > > +               for (column = 0; column < width; column++) {
> > > +                       st->nents++;
> > > +                       /* We don't need the pages, but need to initialize
> > > +                        * the entries so the sg list can be happily traversed.
> > > +                        * The only thing we need are DMA addresses.
> > > +                        */
> > > +                       sg_set_page(sg, NULL, PAGE_SIZE, 0);
> > > +                       sg_dma_address(sg) = in[offset + column];
> > > +                       sg_dma_len(sg) = PAGE_SIZE;
> > > +                       sg = sg_next(sg);
> > 
> > Ok. But should be I915_GTT_PAGE_SIZE?
> 
> I suppose. And now I wonder what would happen on gen2 with its
> 2KiB gtt pages. Probably nothing good.

Pffifle. We call it 4KiB. It's just about the semantics, and here we
should be splitting the dma addresses by GTT_PAGE_SIZE rather than
system page size.

> > > +struct intel_remapped_info {
> > > +       struct intel_remapped_plane_info {
> > > +               /* tiles */
> > > +               unsigned int width, height, stride, offset;
> > > +       } plane[2];
> > > +       unsigned int unused;
> > 
> > Tag it as mbz, since we do use it inside the compare. Hmm, I wonder if
> > it actually is better if it doesn't exist if it isn't used, then it
> > should be zero.. Hmm, not sure if that's defined at all, might have to
> > say memset and don't rely on {} zeroing?
> > 
> > Should work fine as a memcmp key for the rbtree.
> 
> This whole thing is a bit questionale the way I did it. When populating
> the gtt_view I just poke at view->rotated and rely on matching layout
> for view->remapped. To make it less magic maybe I should embed one
> inside the other?

Hmm. If it's intentionally the same layout, then we should just use the
same struct for both. remapped/remap_info is generic enough to cover
rotation as well.

> > > +} __packed;
> > > +
> > > +static inline void assert_intel_remapped_info_is_packed(void)
> > > +{
> > > +       BUILD_BUG_ON(sizeof(struct intel_remapped_info) != 10*sizeof(unsigned int));
> > > +}
> > > +
> > >  struct intel_rotation_info {
> > >         struct intel_rotation_plane_info {
> > >                 /* tiles */
> > > @@ -186,6 +199,7 @@ enum i915_ggtt_view_type {
> > >         I915_GGTT_VIEW_NORMAL = 0,
> > >         I915_GGTT_VIEW_ROTATED = sizeof(struct intel_rotation_info),
> > >         I915_GGTT_VIEW_PARTIAL = sizeof(struct intel_partial_info),
> > > +       I915_GGTT_VIEW_REMAPPED = sizeof(struct intel_remapped_info),

Oh, forgot about that trick. Yeah, they do need to differ in structs.

Hmm, so I think keep the remap_plane_info and reuse that?

struct intel_remapped_info {
       struct intel_remapped_plane_info {
               /* tiles */
               unsigned int width, height, stride, offset;
       } plane[2];
       int unused_mbz; 
};


struct intel_rotation_info {
	struct intel_remmaped_plane_info plane[2];
};

static inline void assert_intel_rotation_matches_remapped_info(void)
{
	/* Check that rotation/remapped shares offsets for simplicity */
	BUILD_BUG_ON(offsetof(struct intel_remapped_info, plane[0]) !=
	             offsetof(struct intel_rotation_info, plane[0]));
	BUILD_BUG_ON(offsetofend(struct intel_remapped_info, plane[1]) !=
	             offsetofend(struct intel_rotation_info, plane[1]));
}
-Chris