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