Re: [PATCH 15/18] drm/i915: Add a new "remapped" gtt_view

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Jul 19, 2018 at 08:46:54PM +0100, Chris Wilson wrote:
> 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]));
> }

Yeah, something like that could work.

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx