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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
