| Message ID | 1426607117-10626-6-git-send-email-tvrtko.ursulin@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On ti, 2015-03-17 at 15:45 +0000, Tvrtko Ursulin wrote: > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > 90/270 rotated scanout needs a rotated GTT view of the framebuffer. > > This is put in a separate VMA with a dedicated ggtt view and wired such that > it is created when a framebuffer is pinned to a 90/270 rotated plane. > > Rotation is only possible with Yb/Yf buffers and error is propagated to > user space in case of a mismatch. > > Special rotated page view is constructed at the VMA creation time by > borrowing the DMA addresses from obj->pages. > > v2: > * Do not bother with pages for rotated sg list, just populate the DMA > addresses. (Daniel Vetter) > * Checkpatch cleanup. > > v3: > * Rebased on top of new plane handling (create rotated mapping when > setting the rotation property). > * Unpin rotated VMA on unpinning from display plane. > * Simplify rotation check using bitwise AND. (Chris Wilson) > > v4: > * Fix unpinning of optional rotated mapping so it is really considered > to be optional. > > v5: > * Rebased for fb modifier changes. > * Rebased for atomic commit. > * Only pin needed view for display. (Ville Syrjälä, Daniel Vetter) > > v6: > * Rebased after preparatory work has been extracted out. (Daniel Vetter) > > v7: > * Slightly simplified tiling geometry calculation. > * Moved rotated GGTT view implementation into i915_gem_gtt.c (Daniel Vetter) > > v8: > * Do not use i915_gem_obj_size to get object size since that actually > returns the size of an VMA which may not exist. > * Rebased for ggtt view changes. > > For: VIZ-4726 > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Reviewed-by: Michel Thierry <michel.thierry@intel.com> (v4) > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > --- > drivers/gpu/drm/i915/i915_gem_gtt.c | 113 ++++++++++++++++++++++++++++++++++- > drivers/gpu/drm/i915/i915_gem_gtt.h | 12 ++++ > drivers/gpu/drm/i915/intel_display.c | 29 ++++++++- > drivers/gpu/drm/i915/intel_drv.h | 4 ++ > 4 files changed, 154 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c > index f1b9ea6..5381ebf 100644 > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c > @@ -2413,15 +2413,119 @@ i915_gem_obj_lookup_or_create_ggtt_vma(struct drm_i915_gem_object *obj, > > } > > +static > +void rotate_pages(dma_addr_t *in, unsigned int width, unsigned int height, > + struct sg_table *st) > +{ > + unsigned int column, row; > + unsigned int src_idx; > + struct scatterlist *sg = st->sgl; > + > + st->nents = 0; > + > + for (column = 0; column < width; column++) { > + src_idx = width * (height - 1) + column; > + for (row = 0; row < height; row++) { > + 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[src_idx]; > + sg_dma_len(sg) = PAGE_SIZE; > + sg = sg_next(sg); > + src_idx -= width; > + } > + } > +} > + > +static > +struct sg_table *intel_rotate_fb_obj_pages(struct i915_ggtt_view *ggtt_view, > + struct drm_i915_gem_object *obj) > +{ > + struct drm_device *dev = obj->base.dev; > + struct intel_rotation_info *rot_info = &ggtt_view->rotation_info; > + unsigned long size, pages, rot_pages; > + struct sg_page_iter sg_iter; > + unsigned long i; > + dma_addr_t *page_addr_list; > + struct sg_table *st; > + unsigned int tile_pitch, tile_height; > + unsigned int width_pages, height_pages; > + int ret = ENOMEM; > + > + pages = obj->base.size / PAGE_SIZE; > + > + /* Calculate tiling geometry. */ > + tile_height = intel_tile_height(dev, rot_info->pixel_format, > + rot_info->fb_modifier); > + tile_pitch = PAGE_SIZE / tile_height; > + width_pages = DIV_ROUND_UP(rot_info->pitch, tile_pitch); > + height_pages = DIV_ROUND_UP(rot_info->height, tile_height); > + rot_pages = width_pages * height_pages; > + size = rot_pages * PAGE_SIZE; > + > + /* Allocate a temporary list of source pages for random access. */ > + page_addr_list = drm_malloc_ab(pages, sizeof(dma_addr_t)); > + if (!page_addr_list) > + return ERR_PTR(ret); > + > + /* Allocate target SG list. */ > + st = kmalloc(sizeof(*st), GFP_KERNEL); > + if (!st) > + goto err_st_alloc; > + > + ret = sg_alloc_table(st, rot_pages, GFP_KERNEL); > + if (ret) > + goto err_sg_alloc; > + > + /* Populate source page list from the object. */ > + i = 0; > + for_each_sg_page(obj->pages->sgl, &sg_iter, obj->pages->nents, 0) { > + page_addr_list[i] = sg_page_iter_dma_address(&sg_iter); > + i++; > + } > + > + /* Rotate the pages. */ > + rotate_pages(page_addr_list, width_pages, height_pages, st); > + > + DRM_DEBUG_KMS( > + "Created rotated page mapping for object size %lu (pitch=%u, height=%u, pixel_format=0x%x, %ux%u tiles, %lu pages).\n", > + size, rot_info->pitch, rot_info->height, > + rot_info->pixel_format, width_pages, height_pages, > + rot_pages); > + > + drm_free_large(page_addr_list); > + > + return st; > + > +err_sg_alloc: > + kfree(st); > +err_st_alloc: > + drm_free_large(page_addr_list); > + > + DRM_DEBUG_KMS( > + "Failed to create rotated mapping for object size %lu! (%d) (pitch=%u, height=%u, pixel_format=0x%x, %ux%u tiles, %lu pages)\n", > + size, ret, rot_info->pitch, rot_info->height, > + rot_info->pixel_format, width_pages, height_pages, > + rot_pages); > + return ERR_PTR(ret); > +} > > static inline > int i915_get_ggtt_vma_pages(struct i915_vma *vma) Same rant about function signatures as on earlier patch, put all on the same line like most of new the code has it. > { > + int ret = 0; > + > if (vma->ggtt_view.pages) > return 0; > > if (vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL) > vma->ggtt_view.pages = vma->obj->pages; > + else if (vma->ggtt_view.type == I915_GGTT_VIEW_ROTATED) > + vma->ggtt_view.pages = > + intel_rotate_fb_obj_pages(&vma->ggtt_view, vma->obj); > else > WARN_ONCE(1, "GGTT view %u not implemented!\n", > vma->ggtt_view.type); > @@ -2429,10 +2533,15 @@ int i915_get_ggtt_vma_pages(struct i915_vma *vma) > if (!vma->ggtt_view.pages) { > DRM_ERROR("Failed to get pages for GGTT view type %u!\n", > vma->ggtt_view.type); > - return -EINVAL; > + ret = -EINVAL; > + } else if (IS_ERR(vma->ggtt_view.pages)) { > + ret = PTR_ERR(vma->ggtt_view.pages); > + vma->ggtt_view.pages = NULL; > + DRM_ERROR("Failed to get pages for VMA view type %u (%d)!\n", > + vma->ggtt_view.type, ret); > } > > - return 0; > + return ret; > } > > /** > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h > index c9e93f5..a61e8dd 100644 > --- a/drivers/gpu/drm/i915/i915_gem_gtt.h > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h > @@ -111,12 +111,24 @@ typedef gen8_gtt_pte_t gen8_ppgtt_pde_t; > > enum i915_ggtt_view_type { > I915_GGTT_VIEW_NORMAL = 0, > + I915_GGTT_VIEW_ROTATED > +}; > + > +struct intel_rotation_info { > + unsigned int height; > + unsigned int pitch; > + uint32_t pixel_format; > + uint64_t fb_modifier; > }; > > struct i915_ggtt_view { > enum i915_ggtt_view_type type; > > struct sg_table *pages; > + > + union { > + struct intel_rotation_info rotation_info; > + }; In preparation for the memcmp way of comparing views, I would move this be before the variable struct parts (namely sg_table *pages), and also wrap it once more so the result would be like this: [snip] enum i915_ggtt_view_type type; union { struct { struct intel_rotation_info info; } rotated; struct { ... } partial; }; // private bits go here, to be wrapped in their struct with view // type comparing patches struct sg_table *pages; [snip] That way it's clear which view owns what. > }; > > extern const struct i915_ggtt_view i915_ggtt_view_normal; > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index fe11e99..c2c3a76 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -2194,7 +2194,7 @@ static bool need_vtd_wa(struct drm_device *dev) > return false; > } > > -static unsigned int > +unsigned int > intel_tile_height(struct drm_device *dev, uint32_t pixel_format, > uint64_t fb_format_modifier) > { > @@ -2254,9 +2254,34 @@ int intel_fill_fb_ggtt_view(struct i915_ggtt_view *view, > struct drm_framebuffer *fb, > const struct drm_plane_state *plane_state) > { > + struct intel_rotation_info *info = &view->rotation_info; > + static const struct i915_ggtt_view rotated_view = > + { .type = I915_GGTT_VIEW_ROTATED }; > + > *view = i915_ggtt_view_normal; > > - return 0; > + if (!plane_state) > + return 0; > + > + if (!(plane_state->rotation & > + (BIT(DRM_ROTATE_90) | BIT(DRM_ROTATE_270)))) > + return 0; Should the rotation checking helper introduced in previous patch be used here too? > + > + *view = rotated_view; > + > + info->height = fb->height; > + info->pixel_format = fb->pixel_format; > + info->pitch = fb->pitches[0]; > + info->fb_modifier = fb->modifier[0]; > + > + if (!(info->fb_modifier == I915_FORMAT_MOD_Y_TILED || > + info->fb_modifier == I915_FORMAT_MOD_Yf_TILED)) { > + DRM_DEBUG_KMS( > + "Y or Yf tiling is needed for 90/270 rotation!\n"); > + return -EINVAL; > + } > + > + return 1; > } > > int > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 3721878..53a1372 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -987,6 +987,10 @@ int intel_plane_atomic_set_property(struct drm_plane *plane, > struct drm_property *property, > uint64_t val); > > +unsigned int > +intel_tile_height(struct drm_device *dev, uint32_t pixel_format, > + uint64_t fb_format_modifier); > + > /* shared dpll functions */ > struct intel_shared_dpll *intel_crtc_to_shared_dpll(struct intel_crtc *crtc); > void assert_shared_dpll(struct drm_i915_private *dev_priv,
Hi, On 03/19/2015 01:02 PM, Joonas Lahtinen wrote: >> static inline >> int i915_get_ggtt_vma_pages(struct i915_vma *vma) > > Same rant about function signatures as on earlier patch, put all on the > same line like most of new the code has it. Ok. >> struct i915_ggtt_view { >> enum i915_ggtt_view_type type; >> >> struct sg_table *pages; >> + >> + union { >> + struct intel_rotation_info rotation_info; >> + }; > > In preparation for the memcmp way of comparing views, I would move this > be before the variable struct parts (namely sg_table *pages), and also > wrap it once more so the result would be like this: > > [snip] > enum i915_ggtt_view_type type; > > union { > struct { > struct intel_rotation_info info; > } rotated; > struct { > ... > } partial; > }; > > // private bits go here, to be wrapped in their struct with view > // type comparing patches > > struct sg_table *pages; > [snip] > > That way it's clear which view owns what. Hm, rotation info is not considered in comparing views, it is just a bucket of data passed around between layers. So I suppose private data under your design. Since there is no private union yet, maybe do this later? >> extern const struct i915_ggtt_view i915_ggtt_view_normal; >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >> index fe11e99..c2c3a76 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -2194,7 +2194,7 @@ static bool need_vtd_wa(struct drm_device *dev) >> return false; >> } >> >> -static unsigned int >> +unsigned int >> intel_tile_height(struct drm_device *dev, uint32_t pixel_format, >> uint64_t fb_format_modifier) >> { >> @@ -2254,9 +2254,34 @@ int intel_fill_fb_ggtt_view(struct i915_ggtt_view *view, >> struct drm_framebuffer *fb, >> const struct drm_plane_state *plane_state) >> { >> + struct intel_rotation_info *info = &view->rotation_info; >> + static const struct i915_ggtt_view rotated_view = >> + { .type = I915_GGTT_VIEW_ROTATED }; >> + >> *view = i915_ggtt_view_normal; >> >> - return 0; >> + if (!plane_state) >> + return 0; >> + >> + if (!(plane_state->rotation & >> + (BIT(DRM_ROTATE_90) | BIT(DRM_ROTATE_270)))) >> + return 0; > > Should the rotation checking helper introduced in previous patch be used > here too? It's the other way round, following patch adds the helper and replaces this with a call to it. Regards, Tvrtko
On to, 2015-03-19 at 15:07 +0000, Tvrtko Ursulin wrote: > Hi, > > On 03/19/2015 01:02 PM, Joonas Lahtinen wrote: > >> static inline > >> int i915_get_ggtt_vma_pages(struct i915_vma *vma) > > > > Same rant about function signatures as on earlier patch, put all on the > > same line like most of new the code has it. > > Ok. > > >> struct i915_ggtt_view { > >> enum i915_ggtt_view_type type; > >> > >> struct sg_table *pages; > >> + > >> + union { > >> + struct intel_rotation_info rotation_info; > >> + }; > > > > In preparation for the memcmp way of comparing views, I would move this > > be before the variable struct parts (namely sg_table *pages), and also > > wrap it once more so the result would be like this: > > > > [snip] > > enum i915_ggtt_view_type type; > > > > union { > > struct { > > struct intel_rotation_info info; > > } rotated; > > struct { > > ... > > } partial; > > }; > > > > // private bits go here, to be wrapped in their struct with view > > // type comparing patches > > > > struct sg_table *pages; > > [snip] > > > > That way it's clear which view owns what. > > Hm, rotation info is not considered in comparing views, it is just a > bucket of data passed around between layers. So I suppose private data > under your design. Since there is no private union yet, maybe do this later? Why not? Isn't a 270 degree rotated view substantially different from a 90 degree rotated view (even when the difference technically is just some bit flip somewhere else). At least I would be pretty upset if I was returned the address for 90 degree rotated view when I wanted 270 rotated. If multiple rotated views are not possible, then it is again an implicit thing. There are quite a lot of hardware constraints like this that appear in the code implicitly, which IMHO makes the code hard to follow at times. So I'd try to make it more explicit that the views are not the same, there just can be one rotated view at a time (if that is the case). > > >> extern const struct i915_ggtt_view i915_ggtt_view_normal; > >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > >> index fe11e99..c2c3a76 100644 > >> --- a/drivers/gpu/drm/i915/intel_display.c > >> +++ b/drivers/gpu/drm/i915/intel_display.c > >> @@ -2194,7 +2194,7 @@ static bool need_vtd_wa(struct drm_device *dev) > >> return false; > >> } > >> > >> -static unsigned int > >> +unsigned int > >> intel_tile_height(struct drm_device *dev, uint32_t pixel_format, > >> uint64_t fb_format_modifier) > >> { > >> @@ -2254,9 +2254,34 @@ int intel_fill_fb_ggtt_view(struct i915_ggtt_view *view, > >> struct drm_framebuffer *fb, > >> const struct drm_plane_state *plane_state) > >> { > >> + struct intel_rotation_info *info = &view->rotation_info; > >> + static const struct i915_ggtt_view rotated_view = > >> + { .type = I915_GGTT_VIEW_ROTATED }; > >> + > >> *view = i915_ggtt_view_normal; > >> > >> - return 0; > >> + if (!plane_state) > >> + return 0; > >> + > >> + if (!(plane_state->rotation & > >> + (BIT(DRM_ROTATE_90) | BIT(DRM_ROTATE_270)))) > >> + return 0; > > > > Should the rotation checking helper introduced in previous patch be used > > here too? > > It's the other way round, following patch adds the helper and replaces > this with a call to it. > Yes, my bad, I got:) Regards, Joonas > Regards, > > Tvrtko
On 03/20/2015 12:01 PM, Joonas Lahtinen wrote: > On to, 2015-03-19 at 15:07 +0000, Tvrtko Ursulin wrote: >> Hi, >> >> On 03/19/2015 01:02 PM, Joonas Lahtinen wrote: >>>> static inline >>>> int i915_get_ggtt_vma_pages(struct i915_vma *vma) >>> >>> Same rant about function signatures as on earlier patch, put all on the >>> same line like most of new the code has it. >> >> Ok. >> >>>> struct i915_ggtt_view { >>>> enum i915_ggtt_view_type type; >>>> >>>> struct sg_table *pages; >>>> + >>>> + union { >>>> + struct intel_rotation_info rotation_info; >>>> + }; >>> >>> In preparation for the memcmp way of comparing views, I would move this >>> be before the variable struct parts (namely sg_table *pages), and also >>> wrap it once more so the result would be like this: >>> >>> [snip] >>> enum i915_ggtt_view_type type; >>> >>> union { >>> struct { >>> struct intel_rotation_info info; >>> } rotated; >>> struct { >>> ... >>> } partial; >>> }; >>> >>> // private bits go here, to be wrapped in their struct with view >>> // type comparing patches >>> >>> struct sg_table *pages; >>> [snip] >>> >>> That way it's clear which view owns what. >> >> Hm, rotation info is not considered in comparing views, it is just a >> bucket of data passed around between layers. So I suppose private data >> under your design. Since there is no private union yet, maybe do this later? > > Why not? Isn't a 270 degree rotated view substantially different from a > 90 degree rotated view (even when the difference technically is just > some bit flip somewhere else). > > At least I would be pretty upset if I was returned the address for 90 > degree rotated view when I wanted 270 rotated. If multiple rotated views > are not possible, then it is again an implicit thing. > > There are quite a lot of hardware constraints like this that appear in > the code implicitly, which IMHO makes the code hard to follow at times. > So I'd try to make it more explicit that the views are not the same, > there just can be one rotated view at a time (if that is the case). 90 and 270 views are indeed the same page layout - same address for scanout. And there can only be one such VMA for an object at a time. But how this mapping needs to look like is determined by more than the object itself - framebuffer geometry defines it. The private data in the view is used to transfer that meta-data so the GTT core can build the appropriate view. That was my argument in fact for not putting the page shuffling bit in i915_gem_gtt.c since it is really display engine ownership. Regards, Tvrtko
Hi, On pe, 2015-03-20 at 12:11 +0000, Tvrtko Ursulin wrote: > On 03/20/2015 12:01 PM, Joonas Lahtinen wrote: > > On to, 2015-03-19 at 15:07 +0000, Tvrtko Ursulin wrote: > >> Hi, > >> > >> On 03/19/2015 01:02 PM, Joonas Lahtinen wrote: > >>>> static inline > >>>> int i915_get_ggtt_vma_pages(struct i915_vma *vma) > >>> > >>> Same rant about function signatures as on earlier patch, put all on the > >>> same line like most of new the code has it. > >> > >> Ok. > >> > >>>> struct i915_ggtt_view { > >>>> enum i915_ggtt_view_type type; > >>>> > >>>> struct sg_table *pages; > >>>> + > >>>> + union { > >>>> + struct intel_rotation_info rotation_info; > >>>> + }; > >>> > >>> In preparation for the memcmp way of comparing views, I would move this > >>> be before the variable struct parts (namely sg_table *pages), and also > >>> wrap it once more so the result would be like this: > >>> > >>> [snip] > >>> enum i915_ggtt_view_type type; > >>> > >>> union { > >>> struct { > >>> struct intel_rotation_info info; > >>> } rotated; > >>> struct { > >>> ... > >>> } partial; > >>> }; > >>> > >>> // private bits go here, to be wrapped in their struct with view > >>> // type comparing patches > >>> > >>> struct sg_table *pages; > >>> [snip] > >>> > >>> That way it's clear which view owns what. > >> > >> Hm, rotation info is not considered in comparing views, it is just a > >> bucket of data passed around between layers. So I suppose private data > >> under your design. Since there is no private union yet, maybe do this later? > > > > Why not? Isn't a 270 degree rotated view substantially different from a > > 90 degree rotated view (even when the difference technically is just > > some bit flip somewhere else). > > > > At least I would be pretty upset if I was returned the address for 90 > > degree rotated view when I wanted 270 rotated. If multiple rotated views > > are not possible, then it is again an implicit thing. > > > > There are quite a lot of hardware constraints like this that appear in > > the code implicitly, which IMHO makes the code hard to follow at times. > > So I'd try to make it more explicit that the views are not the same, > > there just can be one rotated view at a time (if that is the case). > > 90 and 270 views are indeed the same page layout - same address for > scanout. And there can only be one such VMA for an object at a time. > > But how this mapping needs to look like is determined by more than the > object itself - framebuffer geometry defines it. The private data in the > view is used to transfer that meta-data so the GTT core can build the > appropriate view. > Right, I think I understand your viewpoint now. I would still prefer it to be even more explicit like I915_GGTT_VIEW_Yf_SCANOUT, because what it really does is rearranges the pages to layout suitable for rotated scanout, not making the view rotated in itself. And in that case it would not be in the memcmp range, like it is not currently. Regards, Joonas > That was my argument in fact for not putting the page shuffling bit in > i915_gem_gtt.c since it is really display engine ownership. > > Regards, > > Tvrtko
On 03/20/2015 01:31 PM, Joonas Lahtinen wrote: [snip] >>>> Hm, rotation info is not considered in comparing views, it is just a >>>> bucket of data passed around between layers. So I suppose private data >>>> under your design. Since there is no private union yet, maybe do this later? >>> >>> Why not? Isn't a 270 degree rotated view substantially different from a >>> 90 degree rotated view (even when the difference technically is just >>> some bit flip somewhere else). >>> >>> At least I would be pretty upset if I was returned the address for 90 >>> degree rotated view when I wanted 270 rotated. If multiple rotated views >>> are not possible, then it is again an implicit thing. >>> >>> There are quite a lot of hardware constraints like this that appear in >>> the code implicitly, which IMHO makes the code hard to follow at times. >>> So I'd try to make it more explicit that the views are not the same, >>> there just can be one rotated view at a time (if that is the case). >> >> 90 and 270 views are indeed the same page layout - same address for >> scanout. And there can only be one such VMA for an object at a time. >> >> But how this mapping needs to look like is determined by more than the >> object itself - framebuffer geometry defines it. The private data in the >> view is used to transfer that meta-data so the GTT core can build the >> appropriate view. >> > > Right, I think I understand your viewpoint now. I would still prefer it > to be even more explicit like I915_GGTT_VIEW_Yf_SCANOUT, because what it > really does is rearranges the pages to layout suitable for rotated > scanout, not making the view rotated in itself. Ha, not sure what you mean, but Yf is definitely not the right name for it since it is a different thing altogether. It is rotated in a way that tiles (==pages) are rotated by 90 degrees. But content within the tile is not (display engine handles that bit). In your mind that would maybe be I915_GGTT_VIEW_ROTATED_PAGES_FOR_ROTATED_SCANOUT? :) I am guessing only, but I still think I915_GGTT_VIEW_ROTATED is fine. Regards, Tvrtko
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index f1b9ea6..5381ebf 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -2413,15 +2413,119 @@ i915_gem_obj_lookup_or_create_ggtt_vma(struct drm_i915_gem_object *obj, } +static +void rotate_pages(dma_addr_t *in, unsigned int width, unsigned int height, + struct sg_table *st) +{ + unsigned int column, row; + unsigned int src_idx; + struct scatterlist *sg = st->sgl; + + st->nents = 0; + + for (column = 0; column < width; column++) { + src_idx = width * (height - 1) + column; + for (row = 0; row < height; row++) { + 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[src_idx]; + sg_dma_len(sg) = PAGE_SIZE; + sg = sg_next(sg); + src_idx -= width; + } + } +} + +static +struct sg_table *intel_rotate_fb_obj_pages(struct i915_ggtt_view *ggtt_view, + struct drm_i915_gem_object *obj) +{ + struct drm_device *dev = obj->base.dev; + struct intel_rotation_info *rot_info = &ggtt_view->rotation_info; + unsigned long size, pages, rot_pages; + struct sg_page_iter sg_iter; + unsigned long i; + dma_addr_t *page_addr_list; + struct sg_table *st; + unsigned int tile_pitch, tile_height; + unsigned int width_pages, height_pages; + int ret = ENOMEM; + + pages = obj->base.size / PAGE_SIZE; + + /* Calculate tiling geometry. */ + tile_height = intel_tile_height(dev, rot_info->pixel_format, + rot_info->fb_modifier); + tile_pitch = PAGE_SIZE / tile_height; + width_pages = DIV_ROUND_UP(rot_info->pitch, tile_pitch); + height_pages = DIV_ROUND_UP(rot_info->height, tile_height); + rot_pages = width_pages * height_pages; + size = rot_pages * PAGE_SIZE; + + /* Allocate a temporary list of source pages for random access. */ + page_addr_list = drm_malloc_ab(pages, sizeof(dma_addr_t)); + if (!page_addr_list) + return ERR_PTR(ret); + + /* Allocate target SG list. */ + st = kmalloc(sizeof(*st), GFP_KERNEL); + if (!st) + goto err_st_alloc; + + ret = sg_alloc_table(st, rot_pages, GFP_KERNEL); + if (ret) + goto err_sg_alloc; + + /* Populate source page list from the object. */ + i = 0; + for_each_sg_page(obj->pages->sgl, &sg_iter, obj->pages->nents, 0) { + page_addr_list[i] = sg_page_iter_dma_address(&sg_iter); + i++; + } + + /* Rotate the pages. */ + rotate_pages(page_addr_list, width_pages, height_pages, st); + + DRM_DEBUG_KMS( + "Created rotated page mapping for object size %lu (pitch=%u, height=%u, pixel_format=0x%x, %ux%u tiles, %lu pages).\n", + size, rot_info->pitch, rot_info->height, + rot_info->pixel_format, width_pages, height_pages, + rot_pages); + + drm_free_large(page_addr_list); + + return st; + +err_sg_alloc: + kfree(st); +err_st_alloc: + drm_free_large(page_addr_list); + + DRM_DEBUG_KMS( + "Failed to create rotated mapping for object size %lu! (%d) (pitch=%u, height=%u, pixel_format=0x%x, %ux%u tiles, %lu pages)\n", + size, ret, rot_info->pitch, rot_info->height, + rot_info->pixel_format, width_pages, height_pages, + rot_pages); + return ERR_PTR(ret); +} static inline int i915_get_ggtt_vma_pages(struct i915_vma *vma) { + int ret = 0; + if (vma->ggtt_view.pages) return 0; if (vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL) vma->ggtt_view.pages = vma->obj->pages; + else if (vma->ggtt_view.type == I915_GGTT_VIEW_ROTATED) + vma->ggtt_view.pages = + intel_rotate_fb_obj_pages(&vma->ggtt_view, vma->obj); else WARN_ONCE(1, "GGTT view %u not implemented!\n", vma->ggtt_view.type); @@ -2429,10 +2533,15 @@ int i915_get_ggtt_vma_pages(struct i915_vma *vma) if (!vma->ggtt_view.pages) { DRM_ERROR("Failed to get pages for GGTT view type %u!\n", vma->ggtt_view.type); - return -EINVAL; + ret = -EINVAL; + } else if (IS_ERR(vma->ggtt_view.pages)) { + ret = PTR_ERR(vma->ggtt_view.pages); + vma->ggtt_view.pages = NULL; + DRM_ERROR("Failed to get pages for VMA view type %u (%d)!\n", + vma->ggtt_view.type, ret); } - return 0; + return ret; } /** diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h index c9e93f5..a61e8dd 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.h +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h @@ -111,12 +111,24 @@ typedef gen8_gtt_pte_t gen8_ppgtt_pde_t; enum i915_ggtt_view_type { I915_GGTT_VIEW_NORMAL = 0, + I915_GGTT_VIEW_ROTATED +}; + +struct intel_rotation_info { + unsigned int height; + unsigned int pitch; + uint32_t pixel_format; + uint64_t fb_modifier; }; struct i915_ggtt_view { enum i915_ggtt_view_type type; struct sg_table *pages; + + union { + struct intel_rotation_info rotation_info; + }; }; extern const struct i915_ggtt_view i915_ggtt_view_normal; diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index fe11e99..c2c3a76 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -2194,7 +2194,7 @@ static bool need_vtd_wa(struct drm_device *dev) return false; } -static unsigned int +unsigned int intel_tile_height(struct drm_device *dev, uint32_t pixel_format, uint64_t fb_format_modifier) { @@ -2254,9 +2254,34 @@ int intel_fill_fb_ggtt_view(struct i915_ggtt_view *view, struct drm_framebuffer *fb, const struct drm_plane_state *plane_state) { + struct intel_rotation_info *info = &view->rotation_info; + static const struct i915_ggtt_view rotated_view = + { .type = I915_GGTT_VIEW_ROTATED }; + *view = i915_ggtt_view_normal; - return 0; + if (!plane_state) + return 0; + + if (!(plane_state->rotation & + (BIT(DRM_ROTATE_90) | BIT(DRM_ROTATE_270)))) + return 0; + + *view = rotated_view; + + info->height = fb->height; + info->pixel_format = fb->pixel_format; + info->pitch = fb->pitches[0]; + info->fb_modifier = fb->modifier[0]; + + if (!(info->fb_modifier == I915_FORMAT_MOD_Y_TILED || + info->fb_modifier == I915_FORMAT_MOD_Yf_TILED)) { + DRM_DEBUG_KMS( + "Y or Yf tiling is needed for 90/270 rotation!\n"); + return -EINVAL; + } + + return 1; } int diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 3721878..53a1372 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -987,6 +987,10 @@ int intel_plane_atomic_set_property(struct drm_plane *plane, struct drm_property *property, uint64_t val); +unsigned int +intel_tile_height(struct drm_device *dev, uint32_t pixel_format, + uint64_t fb_format_modifier); + /* shared dpll functions */ struct intel_shared_dpll *intel_crtc_to_shared_dpll(struct intel_crtc *crtc); void assert_shared_dpll(struct drm_i915_private *dev_priv,