* [PATCH 1/5] drm: Pass in new and old plane state to prepare_fb and cleanup_fb
2015-03-02 14:43 [PATCH 0/5] Skylake 90/270 display rotation Tvrtko Ursulin
@ 2015-03-02 14:43 ` Tvrtko Ursulin
2015-03-02 15:51 ` Daniel Vetter
2015-03-02 14:43 ` [PATCH 2/5] drm/i915/skl: Extract tile height code into a helper function Tvrtko Ursulin
` (3 subsequent siblings)
4 siblings, 1 reply; 11+ messages in thread
From: Tvrtko Ursulin @ 2015-03-02 14:43 UTC (permalink / raw)
To: Intel-gfx; +Cc: Daniel Vetter, dri-devel
From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Use cases like rotation require these hooks to have some context so they
know how to prepare and cleanup the frame buffer correctly.
For i915 specifically, object backing pages need to be mapped differently
for different rotation modes and the driver needs to know which mapping to
instantiate and which to tear down when transitioning between them.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: dri-devel@lists.freedesktop.org
---
drivers/gpu/drm/drm_atomic_helper.c | 13 ++++++++-----
drivers/gpu/drm/drm_plane_helper.c | 5 +++--
drivers/gpu/drm/i915/intel_display.c | 6 ++++--
drivers/gpu/drm/i915/intel_drv.h | 6 ++++--
drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c | 6 ++++--
drivers/gpu/drm/tegra/dc.c | 6 ++++--
include/drm/drm_plane_helper.h | 6 ++++--
7 files changed, 31 insertions(+), 17 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 3ce57f4..a745881 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1116,6 +1116,7 @@ int drm_atomic_helper_prepare_planes(struct drm_device *dev,
for (i = 0; i < nplanes; i++) {
struct drm_plane_helper_funcs *funcs;
struct drm_plane *plane = state->planes[i];
+ struct drm_plane_state *plane_state = state->plane_states[i];
struct drm_framebuffer *fb;
if (!plane)
@@ -1123,10 +1124,10 @@ int drm_atomic_helper_prepare_planes(struct drm_device *dev,
funcs = plane->helper_private;
- fb = state->plane_states[i]->fb;
+ fb = plane_state->fb;
if (fb && funcs->prepare_fb) {
- ret = funcs->prepare_fb(plane, fb);
+ ret = funcs->prepare_fb(plane, fb, plane_state);
if (ret)
goto fail;
}
@@ -1138,6 +1139,7 @@ fail:
for (i--; i >= 0; i--) {
struct drm_plane_helper_funcs *funcs;
struct drm_plane *plane = state->planes[i];
+ struct drm_plane_state *plane_state = state->plane_states[i];
struct drm_framebuffer *fb;
if (!plane)
@@ -1148,7 +1150,7 @@ fail:
fb = state->plane_states[i]->fb;
if (fb && funcs->cleanup_fb)
- funcs->cleanup_fb(plane, fb);
+ funcs->cleanup_fb(plane, fb, plane_state);
}
@@ -1254,6 +1256,7 @@ void drm_atomic_helper_cleanup_planes(struct drm_device *dev,
for (i = 0; i < nplanes; i++) {
struct drm_plane_helper_funcs *funcs;
struct drm_plane *plane = old_state->planes[i];
+ struct drm_plane_state *plane_state = old_state->plane_states[i];
struct drm_framebuffer *old_fb;
if (!plane)
@@ -1261,10 +1264,10 @@ void drm_atomic_helper_cleanup_planes(struct drm_device *dev,
funcs = plane->helper_private;
- old_fb = old_state->plane_states[i]->fb;
+ old_fb = plane_state->fb;
if (old_fb && funcs->cleanup_fb)
- funcs->cleanup_fb(plane, old_fb);
+ funcs->cleanup_fb(plane, old_fb, plane_state);
}
}
EXPORT_SYMBOL(drm_atomic_helper_cleanup_planes);
diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c
index 5ba5792..813a066 100644
--- a/drivers/gpu/drm/drm_plane_helper.c
+++ b/drivers/gpu/drm/drm_plane_helper.c
@@ -437,7 +437,8 @@ int drm_plane_helper_commit(struct drm_plane *plane,
if (plane_funcs->prepare_fb && plane_state->fb &&
plane_state->fb != old_fb) {
- ret = plane_funcs->prepare_fb(plane, plane_state->fb);
+ ret = plane_funcs->prepare_fb(plane, plane_state->fb,
+ plane_state);
if (ret)
goto out;
}
@@ -487,7 +488,7 @@ int drm_plane_helper_commit(struct drm_plane *plane,
}
if (plane_funcs->cleanup_fb && old_fb)
- plane_funcs->cleanup_fb(plane, old_fb);
+ plane_funcs->cleanup_fb(plane, old_fb, plane_state);
out:
if (plane_state) {
if (plane->funcs->atomic_destroy_state)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 3156d77..c54a6e9 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11891,7 +11891,8 @@ static void intel_shared_dpll_init(struct drm_device *dev)
*/
int
intel_prepare_plane_fb(struct drm_plane *plane,
- struct drm_framebuffer *fb)
+ struct drm_framebuffer *fb,
+ struct drm_plane_state *new_state)
{
struct drm_device *dev = plane->dev;
struct intel_plane *intel_plane = to_intel_plane(plane);
@@ -11945,7 +11946,8 @@ intel_prepare_plane_fb(struct drm_plane *plane,
*/
void
intel_cleanup_plane_fb(struct drm_plane *plane,
- struct drm_framebuffer *fb)
+ struct drm_framebuffer *fb,
+ struct drm_plane_state *old_state)
{
struct drm_device *dev = plane->dev;
struct drm_i915_gem_object *obj = intel_fb_obj(fb);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 632df1c..c466d43 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -965,9 +965,11 @@ void intel_finish_page_flip(struct drm_device *dev, int pipe);
void intel_finish_page_flip_plane(struct drm_device *dev, int plane);
void intel_check_page_flip(struct drm_device *dev, int pipe);
int intel_prepare_plane_fb(struct drm_plane *plane,
- struct drm_framebuffer *fb);
+ struct drm_framebuffer *fb,
+ struct drm_plane_state *new_state);
void intel_cleanup_plane_fb(struct drm_plane *plane,
- struct drm_framebuffer *fb);
+ struct drm_framebuffer *fb,
+ struct drm_plane_state *old_state);
int intel_plane_atomic_get_property(struct drm_plane *plane,
const struct drm_plane_state *state,
struct drm_property *property,
diff --git a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c
index cde2500..8105c46 100644
--- a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c
+++ b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c
@@ -83,7 +83,8 @@ static const struct drm_plane_funcs mdp4_plane_funcs = {
};
static int mdp4_plane_prepare_fb(struct drm_plane *plane,
- struct drm_framebuffer *fb)
+ struct drm_framebuffer *fb,
+ struct drm_plane_state *new_state)
{
struct mdp4_plane *mdp4_plane = to_mdp4_plane(plane);
struct mdp4_kms *mdp4_kms = get_kms(plane);
@@ -93,7 +94,8 @@ static int mdp4_plane_prepare_fb(struct drm_plane *plane,
}
static void mdp4_plane_cleanup_fb(struct drm_plane *plane,
- struct drm_framebuffer *fb)
+ struct drm_framebuffer *fb,
+ struct drm_plane_state *old_state)
{
struct mdp4_plane *mdp4_plane = to_mdp4_plane(plane);
struct mdp4_kms *mdp4_kms = get_kms(plane);
diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
index 1a52522..9f97d2f 100644
--- a/drivers/gpu/drm/tegra/dc.c
+++ b/drivers/gpu/drm/tegra/dc.c
@@ -472,13 +472,15 @@ static const struct drm_plane_funcs tegra_primary_plane_funcs = {
};
static int tegra_plane_prepare_fb(struct drm_plane *plane,
- struct drm_framebuffer *fb)
+ struct drm_framebuffer *fb,
+ struct drm_plane_state *new_state)
{
return 0;
}
static void tegra_plane_cleanup_fb(struct drm_plane *plane,
- struct drm_framebuffer *fb)
+ struct drm_framebuffer *fb,
+ struct drm_plane_state *old_fb)
{
}
diff --git a/include/drm/drm_plane_helper.h b/include/drm/drm_plane_helper.h
index 31c11d3..a0a72b2 100644
--- a/include/drm/drm_plane_helper.h
+++ b/include/drm/drm_plane_helper.h
@@ -59,9 +59,11 @@ extern int drm_crtc_init(struct drm_device *dev,
*/
struct drm_plane_helper_funcs {
int (*prepare_fb)(struct drm_plane *plane,
- struct drm_framebuffer *fb);
+ struct drm_framebuffer *fb,
+ struct drm_plane_state *new_state);
void (*cleanup_fb)(struct drm_plane *plane,
- struct drm_framebuffer *fb);
+ struct drm_framebuffer *fb,
+ struct drm_plane_state *old_state);
int (*atomic_check)(struct drm_plane *plane,
struct drm_plane_state *state);
--
2.3.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH 1/5] drm: Pass in new and old plane state to prepare_fb and cleanup_fb
2015-03-02 14:43 ` [PATCH 1/5] drm: Pass in new and old plane state to prepare_fb and cleanup_fb Tvrtko Ursulin
@ 2015-03-02 15:51 ` Daniel Vetter
0 siblings, 0 replies; 11+ messages in thread
From: Daniel Vetter @ 2015-03-02 15:51 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: Daniel Vetter, Intel-gfx, dri-devel
On Mon, Mar 02, 2015 at 02:43:48PM +0000, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> Use cases like rotation require these hooks to have some context so they
> know how to prepare and cleanup the frame buffer correctly.
>
> For i915 specifically, object backing pages need to be mapped differently
> for different rotation modes and the driver needs to know which mapping to
> instantiate and which to tear down when transitioning between them.
>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: dri-devel@lists.freedesktop.org
Since I'm still somewhat afraid that drivers will abuse this creatively
and do some state computations in prepare_fb (which they absolutely may
not) I think it would be good to make this new parameter const. I plan to
follow up with other patches to make state objects const in the commit
phase.
-Daniel
> ---
> drivers/gpu/drm/drm_atomic_helper.c | 13 ++++++++-----
> drivers/gpu/drm/drm_plane_helper.c | 5 +++--
> drivers/gpu/drm/i915/intel_display.c | 6 ++++--
> drivers/gpu/drm/i915/intel_drv.h | 6 ++++--
> drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c | 6 ++++--
> drivers/gpu/drm/tegra/dc.c | 6 ++++--
> include/drm/drm_plane_helper.h | 6 ++++--
> 7 files changed, 31 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 3ce57f4..a745881 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1116,6 +1116,7 @@ int drm_atomic_helper_prepare_planes(struct drm_device *dev,
> for (i = 0; i < nplanes; i++) {
> struct drm_plane_helper_funcs *funcs;
> struct drm_plane *plane = state->planes[i];
> + struct drm_plane_state *plane_state = state->plane_states[i];
> struct drm_framebuffer *fb;
>
> if (!plane)
> @@ -1123,10 +1124,10 @@ int drm_atomic_helper_prepare_planes(struct drm_device *dev,
>
> funcs = plane->helper_private;
>
> - fb = state->plane_states[i]->fb;
> + fb = plane_state->fb;
>
> if (fb && funcs->prepare_fb) {
> - ret = funcs->prepare_fb(plane, fb);
> + ret = funcs->prepare_fb(plane, fb, plane_state);
> if (ret)
> goto fail;
> }
> @@ -1138,6 +1139,7 @@ fail:
> for (i--; i >= 0; i--) {
> struct drm_plane_helper_funcs *funcs;
> struct drm_plane *plane = state->planes[i];
> + struct drm_plane_state *plane_state = state->plane_states[i];
> struct drm_framebuffer *fb;
>
> if (!plane)
> @@ -1148,7 +1150,7 @@ fail:
> fb = state->plane_states[i]->fb;
>
> if (fb && funcs->cleanup_fb)
> - funcs->cleanup_fb(plane, fb);
> + funcs->cleanup_fb(plane, fb, plane_state);
>
> }
>
> @@ -1254,6 +1256,7 @@ void drm_atomic_helper_cleanup_planes(struct drm_device *dev,
> for (i = 0; i < nplanes; i++) {
> struct drm_plane_helper_funcs *funcs;
> struct drm_plane *plane = old_state->planes[i];
> + struct drm_plane_state *plane_state = old_state->plane_states[i];
> struct drm_framebuffer *old_fb;
>
> if (!plane)
> @@ -1261,10 +1264,10 @@ void drm_atomic_helper_cleanup_planes(struct drm_device *dev,
>
> funcs = plane->helper_private;
>
> - old_fb = old_state->plane_states[i]->fb;
> + old_fb = plane_state->fb;
>
> if (old_fb && funcs->cleanup_fb)
> - funcs->cleanup_fb(plane, old_fb);
> + funcs->cleanup_fb(plane, old_fb, plane_state);
> }
> }
> EXPORT_SYMBOL(drm_atomic_helper_cleanup_planes);
> diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c
> index 5ba5792..813a066 100644
> --- a/drivers/gpu/drm/drm_plane_helper.c
> +++ b/drivers/gpu/drm/drm_plane_helper.c
> @@ -437,7 +437,8 @@ int drm_plane_helper_commit(struct drm_plane *plane,
>
> if (plane_funcs->prepare_fb && plane_state->fb &&
> plane_state->fb != old_fb) {
> - ret = plane_funcs->prepare_fb(plane, plane_state->fb);
> + ret = plane_funcs->prepare_fb(plane, plane_state->fb,
> + plane_state);
> if (ret)
> goto out;
> }
> @@ -487,7 +488,7 @@ int drm_plane_helper_commit(struct drm_plane *plane,
> }
>
> if (plane_funcs->cleanup_fb && old_fb)
> - plane_funcs->cleanup_fb(plane, old_fb);
> + plane_funcs->cleanup_fb(plane, old_fb, plane_state);
> out:
> if (plane_state) {
> if (plane->funcs->atomic_destroy_state)
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 3156d77..c54a6e9 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11891,7 +11891,8 @@ static void intel_shared_dpll_init(struct drm_device *dev)
> */
> int
> intel_prepare_plane_fb(struct drm_plane *plane,
> - struct drm_framebuffer *fb)
> + struct drm_framebuffer *fb,
> + struct drm_plane_state *new_state)
> {
> struct drm_device *dev = plane->dev;
> struct intel_plane *intel_plane = to_intel_plane(plane);
> @@ -11945,7 +11946,8 @@ intel_prepare_plane_fb(struct drm_plane *plane,
> */
> void
> intel_cleanup_plane_fb(struct drm_plane *plane,
> - struct drm_framebuffer *fb)
> + struct drm_framebuffer *fb,
> + struct drm_plane_state *old_state)
> {
> struct drm_device *dev = plane->dev;
> struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 632df1c..c466d43 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -965,9 +965,11 @@ void intel_finish_page_flip(struct drm_device *dev, int pipe);
> void intel_finish_page_flip_plane(struct drm_device *dev, int plane);
> void intel_check_page_flip(struct drm_device *dev, int pipe);
> int intel_prepare_plane_fb(struct drm_plane *plane,
> - struct drm_framebuffer *fb);
> + struct drm_framebuffer *fb,
> + struct drm_plane_state *new_state);
> void intel_cleanup_plane_fb(struct drm_plane *plane,
> - struct drm_framebuffer *fb);
> + struct drm_framebuffer *fb,
> + struct drm_plane_state *old_state);
> int intel_plane_atomic_get_property(struct drm_plane *plane,
> const struct drm_plane_state *state,
> struct drm_property *property,
> diff --git a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c
> index cde2500..8105c46 100644
> --- a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c
> +++ b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c
> @@ -83,7 +83,8 @@ static const struct drm_plane_funcs mdp4_plane_funcs = {
> };
>
> static int mdp4_plane_prepare_fb(struct drm_plane *plane,
> - struct drm_framebuffer *fb)
> + struct drm_framebuffer *fb,
> + struct drm_plane_state *new_state)
> {
> struct mdp4_plane *mdp4_plane = to_mdp4_plane(plane);
> struct mdp4_kms *mdp4_kms = get_kms(plane);
> @@ -93,7 +94,8 @@ static int mdp4_plane_prepare_fb(struct drm_plane *plane,
> }
>
> static void mdp4_plane_cleanup_fb(struct drm_plane *plane,
> - struct drm_framebuffer *fb)
> + struct drm_framebuffer *fb,
> + struct drm_plane_state *old_state)
> {
> struct mdp4_plane *mdp4_plane = to_mdp4_plane(plane);
> struct mdp4_kms *mdp4_kms = get_kms(plane);
> diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
> index 1a52522..9f97d2f 100644
> --- a/drivers/gpu/drm/tegra/dc.c
> +++ b/drivers/gpu/drm/tegra/dc.c
> @@ -472,13 +472,15 @@ static const struct drm_plane_funcs tegra_primary_plane_funcs = {
> };
>
> static int tegra_plane_prepare_fb(struct drm_plane *plane,
> - struct drm_framebuffer *fb)
> + struct drm_framebuffer *fb,
> + struct drm_plane_state *new_state)
> {
> return 0;
> }
>
> static void tegra_plane_cleanup_fb(struct drm_plane *plane,
> - struct drm_framebuffer *fb)
> + struct drm_framebuffer *fb,
> + struct drm_plane_state *old_fb)
> {
> }
>
> diff --git a/include/drm/drm_plane_helper.h b/include/drm/drm_plane_helper.h
> index 31c11d3..a0a72b2 100644
> --- a/include/drm/drm_plane_helper.h
> +++ b/include/drm/drm_plane_helper.h
> @@ -59,9 +59,11 @@ extern int drm_crtc_init(struct drm_device *dev,
> */
> struct drm_plane_helper_funcs {
> int (*prepare_fb)(struct drm_plane *plane,
> - struct drm_framebuffer *fb);
> + struct drm_framebuffer *fb,
> + struct drm_plane_state *new_state);
> void (*cleanup_fb)(struct drm_plane *plane,
> - struct drm_framebuffer *fb);
> + struct drm_framebuffer *fb,
> + struct drm_plane_state *old_state);
>
> int (*atomic_check)(struct drm_plane *plane,
> struct drm_plane_state *state);
> --
> 2.3.0
>
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/5] drm/i915/skl: Extract tile height code into a helper function
2015-03-02 14:43 [PATCH 0/5] Skylake 90/270 display rotation Tvrtko Ursulin
2015-03-02 14:43 ` [PATCH 1/5] drm: Pass in new and old plane state to prepare_fb and cleanup_fb Tvrtko Ursulin
@ 2015-03-02 14:43 ` Tvrtko Ursulin
2015-03-02 14:43 ` [PATCH 3/5] drm/i915/skl: Support secondary (rotated) frame buffer mapping Tvrtko Ursulin
` (2 subsequent siblings)
4 siblings, 0 replies; 11+ messages in thread
From: Tvrtko Ursulin @ 2015-03-02 14:43 UTC (permalink / raw)
To: Intel-gfx
From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
It will be used in a later patch.
v2: Rebased for fb modifiers.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Reviewed-by: Michel Thierry <michel.thierry@intel.com>
---
drivers/gpu/drm/i915/intel_display.c | 21 ++++++++++++++-------
1 file changed, 14 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index c54a6e9..7a5d0a7 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2189,13 +2189,11 @@ static bool need_vtd_wa(struct drm_device *dev)
return false;
}
-int
-intel_fb_align_height(struct drm_device *dev, int height,
- uint32_t pixel_format,
- uint64_t fb_format_modifier)
+static int
+intel_tile_height(struct drm_device *dev, uint32_t bits_per_pixel,
+ uint64_t fb_format_modifier)
{
int tile_height;
- uint32_t bits_per_pixel;
switch (fb_format_modifier) {
case DRM_FORMAT_MOD_NONE:
@@ -2208,7 +2206,6 @@ intel_fb_align_height(struct drm_device *dev, int height,
tile_height = 32;
break;
case I915_FORMAT_MOD_Yf_TILED:
- bits_per_pixel = drm_format_plane_cpp(pixel_format, 0) * 8;
switch (bits_per_pixel) {
default:
case 8:
@@ -2234,7 +2231,17 @@ intel_fb_align_height(struct drm_device *dev, int height,
break;
}
- return ALIGN(height, tile_height);
+ return tile_height;
+}
+
+int
+intel_fb_align_height(struct drm_device *dev, int height, uint32_t pixel_format,
+ uint64_t fb_format_modifier)
+{
+ uint32_t bits_per_pixel = drm_format_plane_cpp(pixel_format, 0) * 8;
+
+ return ALIGN(height, intel_fb_tile_height(dev, bits_per_pixel,
+ fb_format_modifier));
}
int
--
2.3.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH 3/5] drm/i915/skl: Support secondary (rotated) frame buffer mapping
2015-03-02 14:43 [PATCH 0/5] Skylake 90/270 display rotation Tvrtko Ursulin
2015-03-02 14:43 ` [PATCH 1/5] drm: Pass in new and old plane state to prepare_fb and cleanup_fb Tvrtko Ursulin
2015-03-02 14:43 ` [PATCH 2/5] drm/i915/skl: Extract tile height code into a helper function Tvrtko Ursulin
@ 2015-03-02 14:43 ` Tvrtko Ursulin
2015-03-02 18:21 ` Daniel Vetter
2015-03-02 14:43 ` [PATCH 4/5] drm/i915/skl: Query display address through a wrapper Tvrtko Ursulin
2015-03-02 14:43 ` [PATCH 5/5] drm/i915/skl: Take 90/270 rotation into account in watermark calculations Tvrtko Ursulin
4 siblings, 1 reply; 11+ messages in thread
From: Tvrtko Ursulin @ 2015-03-02 14:43 UTC (permalink / raw)
To: Intel-gfx
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 suchs 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)
For: VIZ-4726
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Reviewed-by: Michel Thierry <michel.thierry@intel.com> (v4)
---
drivers/gpu/drm/i915/i915_drv.h | 33 +++++-
drivers/gpu/drm/i915/i915_gem.c | 27 +++--
drivers/gpu/drm/i915/i915_gem_gtt.c | 14 ++-
drivers/gpu/drm/i915/i915_gem_gtt.h | 12 +++
drivers/gpu/drm/i915/intel_display.c | 204 +++++++++++++++++++++++++++++++----
drivers/gpu/drm/i915/intel_drv.h | 4 +
drivers/gpu/drm/i915/intel_fbdev.c | 2 +-
drivers/gpu/drm/i915/intel_overlay.c | 3 +-
8 files changed, 263 insertions(+), 36 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index e07a1cb..79d3f2c 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2743,8 +2743,10 @@ i915_gem_object_set_to_cpu_domain(struct drm_i915_gem_object *obj, bool write);
int __must_check
i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
u32 alignment,
- struct intel_engine_cs *pipelined);
-void i915_gem_object_unpin_from_display_plane(struct drm_i915_gem_object *obj);
+ struct intel_engine_cs *pipelined,
+ const struct i915_ggtt_view *view);
+void i915_gem_object_unpin_from_display_plane(struct drm_i915_gem_object *obj,
+ const struct i915_ggtt_view *view);
int i915_gem_object_attach_phys(struct drm_i915_gem_object *obj,
int align);
int i915_gem_open(struct drm_device *dev, struct drm_file *file);
@@ -2813,7 +2815,13 @@ i915_gem_obj_lookup_or_create_vma(struct drm_i915_gem_object *obj,
&i915_ggtt_view_normal);
}
-struct i915_vma *i915_gem_obj_to_ggtt(struct drm_i915_gem_object *obj);
+struct i915_vma *i915_gem_obj_to_ggtt_view(struct drm_i915_gem_object *obj,
+ enum i915_ggtt_view_type view);
+static inline
+struct i915_vma *i915_gem_obj_to_ggtt(struct drm_i915_gem_object *obj)
+{
+ return i915_gem_obj_to_ggtt_view(obj, I915_GGTT_VIEW_NORMAL);
+}
static inline bool i915_gem_obj_is_pinned(struct drm_i915_gem_object *obj) {
struct i915_vma *vma;
list_for_each_entry(vma, &obj->vma_list, vma_link)
@@ -2867,13 +2875,30 @@ i915_gem_obj_ggtt_pin(struct drm_i915_gem_object *obj,
alignment, flags | PIN_GLOBAL);
}
+static inline int __must_check
+i915_gem_obj_ggtt_pin_view(struct drm_i915_gem_object *obj,
+ uint32_t alignment,
+ unsigned flags,
+ const struct i915_ggtt_view *ggtt_view)
+{
+ return i915_gem_object_pin_view(obj, i915_obj_to_ggtt(obj),
+ alignment, flags | PIN_GLOBAL,
+ ggtt_view);
+}
+
static inline int
i915_gem_object_ggtt_unbind(struct drm_i915_gem_object *obj)
{
return i915_vma_unbind(i915_gem_obj_to_ggtt(obj));
}
-void i915_gem_object_ggtt_unpin(struct drm_i915_gem_object *obj);
+void i915_gem_object_ggtt_unpin_view(struct drm_i915_gem_object *obj,
+ enum i915_ggtt_view_type view);
+static inline void
+i915_gem_object_ggtt_unpin(struct drm_i915_gem_object *obj)
+{
+ i915_gem_object_ggtt_unpin_view(obj, I915_GGTT_VIEW_NORMAL);
+}
/* i915_gem_context.c */
int __must_check i915_gem_context_init(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 0107c2a..04c0cb1 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3938,7 +3938,8 @@ static bool is_pin_display(struct drm_i915_gem_object *obj)
int
i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
u32 alignment,
- struct intel_engine_cs *pipelined)
+ struct intel_engine_cs *pipelined,
+ const struct i915_ggtt_view *view)
{
u32 old_read_domains, old_write_domain;
bool was_pin_display;
@@ -3974,7 +3975,9 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
* (e.g. libkms for the bootup splash), we have to ensure that we
* always use map_and_fenceable for all scanout buffers.
*/
- ret = i915_gem_obj_ggtt_pin(obj, alignment, PIN_MAPPABLE);
+ ret = i915_gem_obj_ggtt_pin_view(obj, alignment,
+ view->type == I915_GGTT_VIEW_NORMAL ?
+ PIN_MAPPABLE : 0, view);
if (ret)
goto err_unpin_display;
@@ -4002,9 +4005,11 @@ err_unpin_display:
}
void
-i915_gem_object_unpin_from_display_plane(struct drm_i915_gem_object *obj)
+i915_gem_object_unpin_from_display_plane(struct drm_i915_gem_object *obj,
+ const struct i915_ggtt_view *view)
{
- i915_gem_object_ggtt_unpin(obj);
+ i915_gem_object_ggtt_unpin_view(obj, view->type);
+
obj->pin_display = is_pin_display(obj);
}
@@ -4241,15 +4246,16 @@ i915_gem_object_pin_view(struct drm_i915_gem_object *obj,
}
void
-i915_gem_object_ggtt_unpin(struct drm_i915_gem_object *obj)
+i915_gem_object_ggtt_unpin_view(struct drm_i915_gem_object *obj,
+ enum i915_ggtt_view_type view)
{
- struct i915_vma *vma = i915_gem_obj_to_ggtt(obj);
+ struct i915_vma *vma = i915_gem_obj_to_ggtt_view(obj, view);
BUG_ON(!vma);
BUG_ON(vma->pin_count == 0);
- BUG_ON(!i915_gem_obj_ggtt_bound(obj));
+ BUG_ON(!i915_gem_obj_bound_view(obj, i915_obj_to_ggtt(obj), view));
- if (--vma->pin_count == 0)
+ if (--vma->pin_count == 0 && view == I915_GGTT_VIEW_NORMAL)
obj->pin_mappable = false;
}
@@ -5306,14 +5312,15 @@ i915_gem_shrinker_oom(struct notifier_block *nb, unsigned long event, void *ptr)
return NOTIFY_DONE;
}
-struct i915_vma *i915_gem_obj_to_ggtt(struct drm_i915_gem_object *obj)
+struct i915_vma *i915_gem_obj_to_ggtt_view(struct drm_i915_gem_object *obj,
+ enum i915_ggtt_view_type view)
{
struct i915_address_space *ggtt = i915_obj_to_ggtt(obj);
struct i915_vma *vma;
list_for_each_entry(vma, &obj->vma_list, vma_link)
if (vma->vm == ggtt &&
- vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL)
+ vma->ggtt_view.type == view)
return vma;
return NULL;
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index bd95776..9336142 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -2375,11 +2375,16 @@ i915_gem_obj_lookup_or_create_vma_view(struct drm_i915_gem_object *obj,
static inline
int i915_get_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);
@@ -2387,10 +2392,15 @@ int i915_get_vma_pages(struct i915_vma *vma)
if (!vma->ggtt_view.pages) {
DRM_ERROR("Failed to get pages for VMA 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..56a2356 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 pixel_size;
+ unsigned int height;
+ unsigned int pitch;
+ 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 7a5d0a7..cb4dae8 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2244,18 +2244,151 @@ intel_fb_align_height(struct drm_device *dev, int height, uint32_t pixel_format,
fb_format_modifier));
}
-int
-intel_pin_and_fence_fb_obj(struct drm_plane *plane,
- struct drm_framebuffer *fb,
- struct intel_engine_cs *pipelined)
+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;
+ }
+ }
+}
+
+struct sg_table *intel_rotate_fb_obj_pages(struct i915_ggtt_view *ggtt_view,
+ struct drm_i915_gem_object *obj)
{
- struct drm_device *dev = fb->dev;
+ struct drm_device *dev = obj->base.dev;
struct drm_i915_private *dev_priv = dev->dev_private;
- struct drm_i915_gem_object *obj = intel_fb_obj(fb);
- u32 alignment;
- int ret;
+ struct i915_address_space *ggtt = &dev_priv->gtt.base;
+ 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_width, tile_height;
+ unsigned int width_pages, height_pages;
+ int ret = ENOMEM;
- WARN_ON(!mutex_is_locked(&dev->struct_mutex));
+ pages = i915_gem_obj_size(obj, ggtt) / PAGE_SIZE;
+
+ /* Calculate tiling geometry. */
+ tile_height = intel_tile_height(dev, rot_info->pixel_size * 8,
+ rot_info->fb_modifier);
+ tile_width = PAGE_SIZE / rot_info->pixel_size / tile_height;
+ width_pages = DIV_ROUND_UP(rot_info->pitch / rot_info->pixel_size,
+ tile_width);
+ 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_size=%u, %ux%u tiles, %lu pages).\n",
+ size, rot_info->pitch, rot_info->height,
+ rot_info->pixel_size, 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_size=%u, %ux%u tiles, %lu pages)\n",
+ size, ret, rot_info->pitch, rot_info->height,
+ rot_info->pixel_size, width_pages, height_pages,
+ rot_pages);
+ return ERR_PTR(ret);
+}
+
+static
+const struct i915_ggtt_view i915_rotated_ggtt_view_template = {
+ .type = I915_GGTT_VIEW_ROTATED,
+};
+
+static
+int intel_fb_ggtt_view(struct drm_plane_state *plane_state,
+ struct drm_framebuffer *fb,
+ struct i915_ggtt_view *view)
+{
+ struct intel_rotation_info *info = &view->rotation_info;
+
+ memcpy(view, &i915_ggtt_view_normal, sizeof(*view));
+
+ if (!plane_state)
+ return 0;
+
+ if (plane_state->rotation &
+ (BIT(DRM_ROTATE_90) | BIT(DRM_ROTATE_270))) == 0)
+ return 0;
+
+ memcpy(view, &i915_rotated_ggtt_view_template, sizeof(*view));
+
+ info->height = fb->height;
+ info->pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
+ 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;
+}
+
+static
+u32 intel_display_obj_alignment(struct drm_device *dev,
+ struct drm_framebuffer *fb)
+{
+ int alignment;
switch (fb->modifier[0]) {
case DRM_FORMAT_MOD_NONE:
@@ -2296,6 +2429,30 @@ intel_pin_and_fence_fb_obj(struct drm_plane *plane,
if (need_vtd_wa(dev) && alignment < 256 * 1024)
alignment = 256 * 1024;
+ return alignment;
+}
+
+int
+intel_pin_and_fence_fb_obj(struct drm_plane *plane,
+ struct drm_framebuffer *fb,
+ struct drm_plane_state *plane_state,
+ struct intel_engine_cs *pipelined)
+{
+ struct drm_device *dev = fb->dev;
+ struct drm_i915_private *dev_priv = dev->dev_private;
+ struct drm_i915_gem_object *obj = intel_fb_obj(fb);
+ struct i915_ggtt_view view;
+ u32 alignment;
+ int ret;
+
+ WARN_ON(!mutex_is_locked(&dev->struct_mutex));
+
+ alignment = intel_display_obj_alignment(dev, fb);
+
+ ret = intel_fb_ggtt_view(plane_state, fb, &view);
+ if (ret < 0)
+ return ret;
+
/*
* Global gtt pte registers are special registers which actually forward
* writes to a chunk of system memory. Which means that there is no risk
@@ -2306,7 +2463,8 @@ intel_pin_and_fence_fb_obj(struct drm_plane *plane,
intel_runtime_pm_get(dev_priv);
dev_priv->mm.interruptible = false;
- ret = i915_gem_object_pin_to_display_plane(obj, alignment, pipelined);
+ ret = i915_gem_object_pin_to_display_plane(obj, alignment, pipelined,
+ &view);
if (ret)
goto err_interruptible;
@@ -2326,19 +2484,27 @@ intel_pin_and_fence_fb_obj(struct drm_plane *plane,
return 0;
err_unpin:
- i915_gem_object_unpin_from_display_plane(obj);
+ i915_gem_object_unpin_from_display_plane(obj, &view);
err_interruptible:
dev_priv->mm.interruptible = true;
intel_runtime_pm_put(dev_priv);
return ret;
}
-static void intel_unpin_fb_obj(struct drm_i915_gem_object *obj)
+static void intel_unpin_fb_obj(struct drm_framebuffer *fb,
+ struct drm_plane_state *plane_state)
{
+ struct drm_i915_gem_object *obj = intel_fb_obj(fb);
+ struct i915_ggtt_view view;
+ int ret;
+
WARN_ON(!mutex_is_locked(&obj->base.dev->struct_mutex));
+ ret = intel_fb_ggtt_view(plane_state, fb, &view);
+ WARN_ONCE(ret < 0, "Invalid fb format and rotation combination!");
+
i915_gem_object_unpin_fence(obj);
- i915_gem_object_unpin_from_display_plane(obj);
+ i915_gem_object_unpin_from_display_plane(obj, &view);
}
/* Computes the linear offset to the base tile and adjusts x, y. bytes per pixel
@@ -9223,7 +9389,7 @@ static void intel_unpin_work_fn(struct work_struct *__work)
enum pipe pipe = to_intel_crtc(work->crtc)->pipe;
mutex_lock(&dev->struct_mutex);
- intel_unpin_fb_obj(intel_fb_obj(work->old_fb));
+ intel_unpin_fb_obj(work->old_fb, work->crtc->primary->state);
drm_gem_object_unreference(&work->pending_flip_obj->base);
drm_framebuffer_unreference(work->old_fb);
@@ -9931,7 +10097,8 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
ring = &dev_priv->ring[RCS];
}
- ret = intel_pin_and_fence_fb_obj(crtc->primary, fb, ring);
+ ret = intel_pin_and_fence_fb_obj(crtc->primary, fb,
+ crtc->primary->state, ring);
if (ret)
goto cleanup_pending;
@@ -9971,7 +10138,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
return 0;
cleanup_unpin:
- intel_unpin_fb_obj(obj);
+ intel_unpin_fb_obj(fb, crtc->primary->state);
cleanup_pending:
atomic_dec(&intel_crtc->unpin_work_count);
crtc->primary->fb = old_fb;
@@ -11933,7 +12100,7 @@ intel_prepare_plane_fb(struct drm_plane *plane,
if (ret)
DRM_DEBUG_KMS("failed to attach phys object\n");
} else {
- ret = intel_pin_and_fence_fb_obj(plane, fb, NULL);
+ ret = intel_pin_and_fence_fb_obj(plane, fb, new_state, NULL);
}
if (ret == 0)
@@ -11965,7 +12132,7 @@ intel_cleanup_plane_fb(struct drm_plane *plane,
if (plane->type != DRM_PLANE_TYPE_CURSOR ||
!INTEL_INFO(dev)->cursor_needs_physical) {
mutex_lock(&dev->struct_mutex);
- intel_unpin_fb_obj(obj);
+ intel_unpin_fb_obj(fb, old_state);
mutex_unlock(&dev->struct_mutex);
}
}
@@ -13862,6 +14029,7 @@ void intel_modeset_gem_init(struct drm_device *dev)
if (intel_pin_and_fence_fb_obj(c->primary,
c->primary->fb,
+ c->primary->state,
NULL)) {
DRM_ERROR("failed to pin boot fb on pipe %d\n",
to_intel_crtc(c)->pipe);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index c466d43..9c2145d 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -955,6 +955,7 @@ void intel_release_load_detect_pipe(struct drm_connector *connector,
struct intel_load_detect_pipe *old);
int intel_pin_and_fence_fb_obj(struct drm_plane *plane,
struct drm_framebuffer *fb,
+ struct drm_plane_state *plane_state,
struct intel_engine_cs *pipelined);
struct drm_framebuffer *
__intel_framebuffer_create(struct drm_device *dev,
@@ -979,6 +980,9 @@ int intel_plane_atomic_set_property(struct drm_plane *plane,
struct drm_property *property,
uint64_t val);
+struct sg_table *intel_rotate_fb_obj_pages(struct i915_ggtt_view *ggtt_view,
+ struct drm_i915_gem_object *obj);
+
/* 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,
diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index 234a699..d8204ae 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -126,7 +126,7 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
}
/* Flush everything out, we'll be doing GTT only from now on */
- ret = intel_pin_and_fence_fb_obj(NULL, fb, NULL);
+ ret = intel_pin_and_fence_fb_obj(NULL, fb, NULL, NULL);
if (ret) {
DRM_ERROR("failed to pin obj: %d\n", ret);
goto out_fb;
diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
index 823d1d9..dd92122 100644
--- a/drivers/gpu/drm/i915/intel_overlay.c
+++ b/drivers/gpu/drm/i915/intel_overlay.c
@@ -720,7 +720,8 @@ static int intel_overlay_do_put_image(struct intel_overlay *overlay,
if (ret != 0)
return ret;
- ret = i915_gem_object_pin_to_display_plane(new_bo, 0, NULL);
+ ret = i915_gem_object_pin_to_display_plane(new_bo, 0, NULL,
+ &i915_ggtt_view_normal);
if (ret != 0)
return ret;
--
2.3.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH 3/5] drm/i915/skl: Support secondary (rotated) frame buffer mapping
2015-03-02 14:43 ` [PATCH 3/5] drm/i915/skl: Support secondary (rotated) frame buffer mapping Tvrtko Ursulin
@ 2015-03-02 18:21 ` Daniel Vetter
2015-03-03 9:59 ` Tvrtko Ursulin
0 siblings, 1 reply; 11+ messages in thread
From: Daniel Vetter @ 2015-03-02 18:21 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: Intel-gfx
On Mon, Mar 02, 2015 at 02:43:50PM +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 suchs 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)
>
> For: VIZ-4726
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Reviewed-by: Michel Thierry <michel.thierry@intel.com> (v4)
Bunch of nitpicks below. Also I think it'd be good to split this patch
into the rote refactoring work to add view parameters all over the place
and the actual implementation.
Cheers, Daniel
> ---
> drivers/gpu/drm/i915/i915_drv.h | 33 +++++-
> drivers/gpu/drm/i915/i915_gem.c | 27 +++--
> drivers/gpu/drm/i915/i915_gem_gtt.c | 14 ++-
> drivers/gpu/drm/i915/i915_gem_gtt.h | 12 +++
> drivers/gpu/drm/i915/intel_display.c | 204 +++++++++++++++++++++++++++++++----
> drivers/gpu/drm/i915/intel_drv.h | 4 +
> drivers/gpu/drm/i915/intel_fbdev.c | 2 +-
> drivers/gpu/drm/i915/intel_overlay.c | 3 +-
> 8 files changed, 263 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index e07a1cb..79d3f2c 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2743,8 +2743,10 @@ i915_gem_object_set_to_cpu_domain(struct drm_i915_gem_object *obj, bool write);
> int __must_check
> i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
> u32 alignment,
> - struct intel_engine_cs *pipelined);
> -void i915_gem_object_unpin_from_display_plane(struct drm_i915_gem_object *obj);
> + struct intel_engine_cs *pipelined,
> + const struct i915_ggtt_view *view);
> +void i915_gem_object_unpin_from_display_plane(struct drm_i915_gem_object *obj,
> + const struct i915_ggtt_view *view);
> int i915_gem_object_attach_phys(struct drm_i915_gem_object *obj,
> int align);
> int i915_gem_open(struct drm_device *dev, struct drm_file *file);
> @@ -2813,7 +2815,13 @@ i915_gem_obj_lookup_or_create_vma(struct drm_i915_gem_object *obj,
> &i915_ggtt_view_normal);
> }
>
> -struct i915_vma *i915_gem_obj_to_ggtt(struct drm_i915_gem_object *obj);
> +struct i915_vma *i915_gem_obj_to_ggtt_view(struct drm_i915_gem_object *obj,
> + enum i915_ggtt_view_type view);
> +static inline
> +struct i915_vma *i915_gem_obj_to_ggtt(struct drm_i915_gem_object *obj)
> +{
> + return i915_gem_obj_to_ggtt_view(obj, I915_GGTT_VIEW_NORMAL);
> +}
> static inline bool i915_gem_obj_is_pinned(struct drm_i915_gem_object *obj) {
> struct i915_vma *vma;
> list_for_each_entry(vma, &obj->vma_list, vma_link)
> @@ -2867,13 +2875,30 @@ i915_gem_obj_ggtt_pin(struct drm_i915_gem_object *obj,
> alignment, flags | PIN_GLOBAL);
> }
>
> +static inline int __must_check
> +i915_gem_obj_ggtt_pin_view(struct drm_i915_gem_object *obj,
> + uint32_t alignment,
> + unsigned flags,
> + const struct i915_ggtt_view *ggtt_view)
> +{
> + return i915_gem_object_pin_view(obj, i915_obj_to_ggtt(obj),
> + alignment, flags | PIN_GLOBAL,
> + ggtt_view);
> +}
> +
> static inline int
> i915_gem_object_ggtt_unbind(struct drm_i915_gem_object *obj)
> {
> return i915_vma_unbind(i915_gem_obj_to_ggtt(obj));
> }
>
> -void i915_gem_object_ggtt_unpin(struct drm_i915_gem_object *obj);
> +void i915_gem_object_ggtt_unpin_view(struct drm_i915_gem_object *obj,
> + enum i915_ggtt_view_type view);
> +static inline void
> +i915_gem_object_ggtt_unpin(struct drm_i915_gem_object *obj)
> +{
> + i915_gem_object_ggtt_unpin_view(obj, I915_GGTT_VIEW_NORMAL);
> +}
>
> /* i915_gem_context.c */
> int __must_check i915_gem_context_init(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 0107c2a..04c0cb1 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3938,7 +3938,8 @@ static bool is_pin_display(struct drm_i915_gem_object *obj)
> int
> i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
> u32 alignment,
> - struct intel_engine_cs *pipelined)
> + struct intel_engine_cs *pipelined,
> + const struct i915_ggtt_view *view)
> {
> u32 old_read_domains, old_write_domain;
> bool was_pin_display;
> @@ -3974,7 +3975,9 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
> * (e.g. libkms for the bootup splash), we have to ensure that we
> * always use map_and_fenceable for all scanout buffers.
> */
> - ret = i915_gem_obj_ggtt_pin(obj, alignment, PIN_MAPPABLE);
> + ret = i915_gem_obj_ggtt_pin_view(obj, alignment,
> + view->type == I915_GGTT_VIEW_NORMAL ?
> + PIN_MAPPABLE : 0, view);
> if (ret)
> goto err_unpin_display;
>
> @@ -4002,9 +4005,11 @@ err_unpin_display:
> }
>
> void
> -i915_gem_object_unpin_from_display_plane(struct drm_i915_gem_object *obj)
> +i915_gem_object_unpin_from_display_plane(struct drm_i915_gem_object *obj,
> + const struct i915_ggtt_view *view)
> {
> - i915_gem_object_ggtt_unpin(obj);
> + i915_gem_object_ggtt_unpin_view(obj, view->type);
> +
> obj->pin_display = is_pin_display(obj);
> }
>
> @@ -4241,15 +4246,16 @@ i915_gem_object_pin_view(struct drm_i915_gem_object *obj,
> }
>
> void
> -i915_gem_object_ggtt_unpin(struct drm_i915_gem_object *obj)
> +i915_gem_object_ggtt_unpin_view(struct drm_i915_gem_object *obj,
> + enum i915_ggtt_view_type view)
> {
> - struct i915_vma *vma = i915_gem_obj_to_ggtt(obj);
> + struct i915_vma *vma = i915_gem_obj_to_ggtt_view(obj, view);
>
> BUG_ON(!vma);
> BUG_ON(vma->pin_count == 0);
> - BUG_ON(!i915_gem_obj_ggtt_bound(obj));
> + BUG_ON(!i915_gem_obj_bound_view(obj, i915_obj_to_ggtt(obj), view));
>
> - if (--vma->pin_count == 0)
> + if (--vma->pin_count == 0 && view == I915_GGTT_VIEW_NORMAL)
> obj->pin_mappable = false;
> }
>
> @@ -5306,14 +5312,15 @@ i915_gem_shrinker_oom(struct notifier_block *nb, unsigned long event, void *ptr)
> return NOTIFY_DONE;
> }
>
> -struct i915_vma *i915_gem_obj_to_ggtt(struct drm_i915_gem_object *obj)
> +struct i915_vma *i915_gem_obj_to_ggtt_view(struct drm_i915_gem_object *obj,
> + enum i915_ggtt_view_type view)
> {
> struct i915_address_space *ggtt = i915_obj_to_ggtt(obj);
> struct i915_vma *vma;
>
> list_for_each_entry(vma, &obj->vma_list, vma_link)
> if (vma->vm == ggtt &&
> - vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL)
> + vma->ggtt_view.type == view)
> return vma;
>
> return NULL;
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index bd95776..9336142 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -2375,11 +2375,16 @@ i915_gem_obj_lookup_or_create_vma_view(struct drm_i915_gem_object *obj,
> static inline
> int i915_get_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);
There's a bit a layering confusion going on, ggtt view code should be here
in the gem code with an appropriate prefix. Just moving the code is all
that's imo needed.
> else
> WARN_ONCE(1, "GGTT view %u not implemented!\n",
> vma->ggtt_view.type);
> @@ -2387,10 +2392,15 @@ int i915_get_vma_pages(struct i915_vma *vma)
> if (!vma->ggtt_view.pages) {
> DRM_ERROR("Failed to get pages for VMA 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..56a2356 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 pixel_size;
> + unsigned int height;
> + unsigned int pitch;
> + 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 7a5d0a7..cb4dae8 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2244,18 +2244,151 @@ intel_fb_align_height(struct drm_device *dev, int height, uint32_t pixel_format,
> fb_format_modifier));
> }
>
> -int
> -intel_pin_and_fence_fb_obj(struct drm_plane *plane,
> - struct drm_framebuffer *fb,
> - struct intel_engine_cs *pipelined)
> +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;
> + }
> + }
> +}
> +
> +struct sg_table *intel_rotate_fb_obj_pages(struct i915_ggtt_view *ggtt_view,
> + struct drm_i915_gem_object *obj)
> {
> - struct drm_device *dev = fb->dev;
> + struct drm_device *dev = obj->base.dev;
> struct drm_i915_private *dev_priv = dev->dev_private;
> - struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> - u32 alignment;
> - int ret;
> + struct i915_address_space *ggtt = &dev_priv->gtt.base;
> + 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_width, tile_height;
> + unsigned int width_pages, height_pages;
> + int ret = ENOMEM;
>
> - WARN_ON(!mutex_is_locked(&dev->struct_mutex));
> + pages = i915_gem_obj_size(obj, ggtt) / PAGE_SIZE;
> +
> + /* Calculate tiling geometry. */
> + tile_height = intel_tile_height(dev, rot_info->pixel_size * 8,
> + rot_info->fb_modifier);
> + tile_width = PAGE_SIZE / rot_info->pixel_size / tile_height;
> + width_pages = DIV_ROUND_UP(rot_info->pitch / rot_info->pixel_size,
> + tile_width);
> + 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_size=%u, %ux%u tiles, %lu pages).\n",
> + size, rot_info->pitch, rot_info->height,
> + rot_info->pixel_size, 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_size=%u, %ux%u tiles, %lu pages)\n",
> + size, ret, rot_info->pitch, rot_info->height,
> + rot_info->pixel_size, width_pages, height_pages,
> + rot_pages);
> + return ERR_PTR(ret);
> +}
> +
> +static
> +const struct i915_ggtt_view i915_rotated_ggtt_view_template = {
> + .type = I915_GGTT_VIEW_ROTATED,
> +};
Not really needed, you can just use a struct assignment instaed of the
memcpy below, i.e.
*view = {.type = I915_GGTT_VIEW_ROTATED};
gcc then clears everyhing else.
> +
> +static
> +int intel_fb_ggtt_view(struct drm_plane_state *plane_state,
> + struct drm_framebuffer *fb,
> + struct i915_ggtt_view *view)
I think intel_fill_fb_ggtt_view would be a clearer name for what this
does. Also generally we put the struct a function operations on first (to
fake OOP principles) and group later parameters by topic and then object
first and flags/data later. So here I'd go with (view, fb, plane_state).
> +{
> + struct intel_rotation_info *info = &view->rotation_info;
> +
> + memcpy(view, &i915_ggtt_view_normal, sizeof(*view));
> +
> + if (!plane_state)
> + return 0;
> +
> + if (plane_state->rotation &
> + (BIT(DRM_ROTATE_90) | BIT(DRM_ROTATE_270))) == 0)
> + return 0;
> +
> + memcpy(view, &i915_rotated_ggtt_view_template, sizeof(*view));
> +
> + info->height = fb->height;
> + info->pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
> + 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;
> +}
> +
> +static
> +u32 intel_display_obj_alignment(struct drm_device *dev,
> + struct drm_framebuffer *fb)
> +{
> + int alignment;
>
> switch (fb->modifier[0]) {
> case DRM_FORMAT_MOD_NONE:
> @@ -2296,6 +2429,30 @@ intel_pin_and_fence_fb_obj(struct drm_plane *plane,
> if (need_vtd_wa(dev) && alignment < 256 * 1024)
> alignment = 256 * 1024;
>
> + return alignment;
> +}
> +
> +int
> +intel_pin_and_fence_fb_obj(struct drm_plane *plane,
> + struct drm_framebuffer *fb,
> + struct drm_plane_state *plane_state,
> + struct intel_engine_cs *pipelined)
> +{
> + struct drm_device *dev = fb->dev;
> + struct drm_i915_private *dev_priv = dev->dev_private;
> + struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> + struct i915_ggtt_view view;
> + u32 alignment;
> + int ret;
> +
> + WARN_ON(!mutex_is_locked(&dev->struct_mutex));
> +
> + alignment = intel_display_obj_alignment(dev, fb);
> +
> + ret = intel_fb_ggtt_view(plane_state, fb, &view);
> + if (ret < 0)
> + return ret;
> +
> /*
> * Global gtt pte registers are special registers which actually forward
> * writes to a chunk of system memory. Which means that there is no risk
> @@ -2306,7 +2463,8 @@ intel_pin_and_fence_fb_obj(struct drm_plane *plane,
> intel_runtime_pm_get(dev_priv);
>
> dev_priv->mm.interruptible = false;
> - ret = i915_gem_object_pin_to_display_plane(obj, alignment, pipelined);
> + ret = i915_gem_object_pin_to_display_plane(obj, alignment, pipelined,
> + &view);
> if (ret)
> goto err_interruptible;
>
> @@ -2326,19 +2484,27 @@ intel_pin_and_fence_fb_obj(struct drm_plane *plane,
> return 0;
>
> err_unpin:
> - i915_gem_object_unpin_from_display_plane(obj);
> + i915_gem_object_unpin_from_display_plane(obj, &view);
> err_interruptible:
> dev_priv->mm.interruptible = true;
> intel_runtime_pm_put(dev_priv);
> return ret;
> }
>
> -static void intel_unpin_fb_obj(struct drm_i915_gem_object *obj)
> +static void intel_unpin_fb_obj(struct drm_framebuffer *fb,
> + struct drm_plane_state *plane_state)
> {
> + struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> + struct i915_ggtt_view view;
> + int ret;
> +
> WARN_ON(!mutex_is_locked(&obj->base.dev->struct_mutex));
>
> + ret = intel_fb_ggtt_view(plane_state, fb, &view);
> + WARN_ONCE(ret < 0, "Invalid fb format and rotation combination!");
> +
> i915_gem_object_unpin_fence(obj);
> - i915_gem_object_unpin_from_display_plane(obj);
> + i915_gem_object_unpin_from_display_plane(obj, &view);
> }
>
> /* Computes the linear offset to the base tile and adjusts x, y. bytes per pixel
> @@ -9223,7 +9389,7 @@ static void intel_unpin_work_fn(struct work_struct *__work)
> enum pipe pipe = to_intel_crtc(work->crtc)->pipe;
>
> mutex_lock(&dev->struct_mutex);
> - intel_unpin_fb_obj(intel_fb_obj(work->old_fb));
> + intel_unpin_fb_obj(work->old_fb, work->crtc->primary->state);
> drm_gem_object_unreference(&work->pending_flip_obj->base);
> drm_framebuffer_unreference(work->old_fb);
>
> @@ -9931,7 +10097,8 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
> ring = &dev_priv->ring[RCS];
> }
>
> - ret = intel_pin_and_fence_fb_obj(crtc->primary, fb, ring);
> + ret = intel_pin_and_fence_fb_obj(crtc->primary, fb,
> + crtc->primary->state, ring);
> if (ret)
> goto cleanup_pending;
>
> @@ -9971,7 +10138,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
> return 0;
>
> cleanup_unpin:
> - intel_unpin_fb_obj(obj);
> + intel_unpin_fb_obj(fb, crtc->primary->state);
> cleanup_pending:
> atomic_dec(&intel_crtc->unpin_work_count);
> crtc->primary->fb = old_fb;
> @@ -11933,7 +12100,7 @@ intel_prepare_plane_fb(struct drm_plane *plane,
> if (ret)
> DRM_DEBUG_KMS("failed to attach phys object\n");
> } else {
> - ret = intel_pin_and_fence_fb_obj(plane, fb, NULL);
> + ret = intel_pin_and_fence_fb_obj(plane, fb, new_state, NULL);
> }
>
> if (ret == 0)
> @@ -11965,7 +12132,7 @@ intel_cleanup_plane_fb(struct drm_plane *plane,
> if (plane->type != DRM_PLANE_TYPE_CURSOR ||
> !INTEL_INFO(dev)->cursor_needs_physical) {
> mutex_lock(&dev->struct_mutex);
> - intel_unpin_fb_obj(obj);
> + intel_unpin_fb_obj(fb, old_state);
> mutex_unlock(&dev->struct_mutex);
> }
> }
> @@ -13862,6 +14029,7 @@ void intel_modeset_gem_init(struct drm_device *dev)
>
> if (intel_pin_and_fence_fb_obj(c->primary,
> c->primary->fb,
> + c->primary->state,
> NULL)) {
> DRM_ERROR("failed to pin boot fb on pipe %d\n",
> to_intel_crtc(c)->pipe);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index c466d43..9c2145d 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -955,6 +955,7 @@ void intel_release_load_detect_pipe(struct drm_connector *connector,
> struct intel_load_detect_pipe *old);
> int intel_pin_and_fence_fb_obj(struct drm_plane *plane,
> struct drm_framebuffer *fb,
> + struct drm_plane_state *plane_state,
> struct intel_engine_cs *pipelined);
> struct drm_framebuffer *
> __intel_framebuffer_create(struct drm_device *dev,
> @@ -979,6 +980,9 @@ int intel_plane_atomic_set_property(struct drm_plane *plane,
> struct drm_property *property,
> uint64_t val);
>
> +struct sg_table *intel_rotate_fb_obj_pages(struct i915_ggtt_view *ggtt_view,
> + struct drm_i915_gem_object *obj);
> +
> /* 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,
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index 234a699..d8204ae 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -126,7 +126,7 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
> }
>
> /* Flush everything out, we'll be doing GTT only from now on */
> - ret = intel_pin_and_fence_fb_obj(NULL, fb, NULL);
> + ret = intel_pin_and_fence_fb_obj(NULL, fb, NULL, NULL);
> if (ret) {
> DRM_ERROR("failed to pin obj: %d\n", ret);
> goto out_fb;
> diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
> index 823d1d9..dd92122 100644
> --- a/drivers/gpu/drm/i915/intel_overlay.c
> +++ b/drivers/gpu/drm/i915/intel_overlay.c
> @@ -720,7 +720,8 @@ static int intel_overlay_do_put_image(struct intel_overlay *overlay,
> if (ret != 0)
> return ret;
>
> - ret = i915_gem_object_pin_to_display_plane(new_bo, 0, NULL);
> + ret = i915_gem_object_pin_to_display_plane(new_bo, 0, NULL,
> + &i915_ggtt_view_normal);
> if (ret != 0)
> return ret;
>
> --
> 2.3.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH 3/5] drm/i915/skl: Support secondary (rotated) frame buffer mapping
2015-03-02 18:21 ` Daniel Vetter
@ 2015-03-03 9:59 ` Tvrtko Ursulin
2015-03-04 14:43 ` Daniel Vetter
0 siblings, 1 reply; 11+ messages in thread
From: Tvrtko Ursulin @ 2015-03-03 9:59 UTC (permalink / raw)
To: Daniel Vetter; +Cc: Intel-gfx
On 03/02/2015 06:21 PM, Daniel Vetter wrote:
> On Mon, Mar 02, 2015 at 02:43:50PM +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 suchs 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)
>>
>> For: VIZ-4726
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Reviewed-by: Michel Thierry <michel.thierry@intel.com> (v4)
>
> Bunch of nitpicks below. Also I think it'd be good to split this patch
> into the rote refactoring work to add view parameters all over the place
> and the actual implementation.
Ok will split it. Rest below...
>> ---
>> drivers/gpu/drm/i915/i915_drv.h | 33 +++++-
>> drivers/gpu/drm/i915/i915_gem.c | 27 +++--
>> drivers/gpu/drm/i915/i915_gem_gtt.c | 14 ++-
>> drivers/gpu/drm/i915/i915_gem_gtt.h | 12 +++
>> drivers/gpu/drm/i915/intel_display.c | 204 +++++++++++++++++++++++++++++++----
>> drivers/gpu/drm/i915/intel_drv.h | 4 +
>> drivers/gpu/drm/i915/intel_fbdev.c | 2 +-
>> drivers/gpu/drm/i915/intel_overlay.c | 3 +-
>> 8 files changed, 263 insertions(+), 36 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index e07a1cb..79d3f2c 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -2743,8 +2743,10 @@ i915_gem_object_set_to_cpu_domain(struct drm_i915_gem_object *obj, bool write);
>> int __must_check
>> i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
>> u32 alignment,
>> - struct intel_engine_cs *pipelined);
>> -void i915_gem_object_unpin_from_display_plane(struct drm_i915_gem_object *obj);
>> + struct intel_engine_cs *pipelined,
>> + const struct i915_ggtt_view *view);
>> +void i915_gem_object_unpin_from_display_plane(struct drm_i915_gem_object *obj,
>> + const struct i915_ggtt_view *view);
>> int i915_gem_object_attach_phys(struct drm_i915_gem_object *obj,
>> int align);
>> int i915_gem_open(struct drm_device *dev, struct drm_file *file);
>> @@ -2813,7 +2815,13 @@ i915_gem_obj_lookup_or_create_vma(struct drm_i915_gem_object *obj,
>> &i915_ggtt_view_normal);
>> }
>>
>> -struct i915_vma *i915_gem_obj_to_ggtt(struct drm_i915_gem_object *obj);
>> +struct i915_vma *i915_gem_obj_to_ggtt_view(struct drm_i915_gem_object *obj,
>> + enum i915_ggtt_view_type view);
>> +static inline
>> +struct i915_vma *i915_gem_obj_to_ggtt(struct drm_i915_gem_object *obj)
>> +{
>> + return i915_gem_obj_to_ggtt_view(obj, I915_GGTT_VIEW_NORMAL);
>> +}
>> static inline bool i915_gem_obj_is_pinned(struct drm_i915_gem_object *obj) {
>> struct i915_vma *vma;
>> list_for_each_entry(vma, &obj->vma_list, vma_link)
>> @@ -2867,13 +2875,30 @@ i915_gem_obj_ggtt_pin(struct drm_i915_gem_object *obj,
>> alignment, flags | PIN_GLOBAL);
>> }
>>
>> +static inline int __must_check
>> +i915_gem_obj_ggtt_pin_view(struct drm_i915_gem_object *obj,
>> + uint32_t alignment,
>> + unsigned flags,
>> + const struct i915_ggtt_view *ggtt_view)
>> +{
>> + return i915_gem_object_pin_view(obj, i915_obj_to_ggtt(obj),
>> + alignment, flags | PIN_GLOBAL,
>> + ggtt_view);
>> +}
>> +
>> static inline int
>> i915_gem_object_ggtt_unbind(struct drm_i915_gem_object *obj)
>> {
>> return i915_vma_unbind(i915_gem_obj_to_ggtt(obj));
>> }
>>
>> -void i915_gem_object_ggtt_unpin(struct drm_i915_gem_object *obj);
>> +void i915_gem_object_ggtt_unpin_view(struct drm_i915_gem_object *obj,
>> + enum i915_ggtt_view_type view);
>> +static inline void
>> +i915_gem_object_ggtt_unpin(struct drm_i915_gem_object *obj)
>> +{
>> + i915_gem_object_ggtt_unpin_view(obj, I915_GGTT_VIEW_NORMAL);
>> +}
>>
>> /* i915_gem_context.c */
>> int __must_check i915_gem_context_init(struct drm_device *dev);
>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>> index 0107c2a..04c0cb1 100644
>> --- a/drivers/gpu/drm/i915/i915_gem.c
>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>> @@ -3938,7 +3938,8 @@ static bool is_pin_display(struct drm_i915_gem_object *obj)
>> int
>> i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
>> u32 alignment,
>> - struct intel_engine_cs *pipelined)
>> + struct intel_engine_cs *pipelined,
>> + const struct i915_ggtt_view *view)
>> {
>> u32 old_read_domains, old_write_domain;
>> bool was_pin_display;
>> @@ -3974,7 +3975,9 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
>> * (e.g. libkms for the bootup splash), we have to ensure that we
>> * always use map_and_fenceable for all scanout buffers.
>> */
>> - ret = i915_gem_obj_ggtt_pin(obj, alignment, PIN_MAPPABLE);
>> + ret = i915_gem_obj_ggtt_pin_view(obj, alignment,
>> + view->type == I915_GGTT_VIEW_NORMAL ?
>> + PIN_MAPPABLE : 0, view);
>> if (ret)
>> goto err_unpin_display;
>>
>> @@ -4002,9 +4005,11 @@ err_unpin_display:
>> }
>>
>> void
>> -i915_gem_object_unpin_from_display_plane(struct drm_i915_gem_object *obj)
>> +i915_gem_object_unpin_from_display_plane(struct drm_i915_gem_object *obj,
>> + const struct i915_ggtt_view *view)
>> {
>> - i915_gem_object_ggtt_unpin(obj);
>> + i915_gem_object_ggtt_unpin_view(obj, view->type);
>> +
>> obj->pin_display = is_pin_display(obj);
>> }
>>
>> @@ -4241,15 +4246,16 @@ i915_gem_object_pin_view(struct drm_i915_gem_object *obj,
>> }
>>
>> void
>> -i915_gem_object_ggtt_unpin(struct drm_i915_gem_object *obj)
>> +i915_gem_object_ggtt_unpin_view(struct drm_i915_gem_object *obj,
>> + enum i915_ggtt_view_type view)
>> {
>> - struct i915_vma *vma = i915_gem_obj_to_ggtt(obj);
>> + struct i915_vma *vma = i915_gem_obj_to_ggtt_view(obj, view);
>>
>> BUG_ON(!vma);
>> BUG_ON(vma->pin_count == 0);
>> - BUG_ON(!i915_gem_obj_ggtt_bound(obj));
>> + BUG_ON(!i915_gem_obj_bound_view(obj, i915_obj_to_ggtt(obj), view));
>>
>> - if (--vma->pin_count == 0)
>> + if (--vma->pin_count == 0 && view == I915_GGTT_VIEW_NORMAL)
>> obj->pin_mappable = false;
>> }
>>
>> @@ -5306,14 +5312,15 @@ i915_gem_shrinker_oom(struct notifier_block *nb, unsigned long event, void *ptr)
>> return NOTIFY_DONE;
>> }
>>
>> -struct i915_vma *i915_gem_obj_to_ggtt(struct drm_i915_gem_object *obj)
>> +struct i915_vma *i915_gem_obj_to_ggtt_view(struct drm_i915_gem_object *obj,
>> + enum i915_ggtt_view_type view)
>> {
>> struct i915_address_space *ggtt = i915_obj_to_ggtt(obj);
>> struct i915_vma *vma;
>>
>> list_for_each_entry(vma, &obj->vma_list, vma_link)
>> if (vma->vm == ggtt &&
>> - vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL)
>> + vma->ggtt_view.type == view)
>> return vma;
>>
>> return NULL;
>> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
>> index bd95776..9336142 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
>> @@ -2375,11 +2375,16 @@ i915_gem_obj_lookup_or_create_vma_view(struct drm_i915_gem_object *obj,
>> static inline
>> int i915_get_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);
>
> There's a bit a layering confusion going on, ggtt view code should be here
> in the gem code with an appropriate prefix. Just moving the code is all
> that's imo needed.
You would move intel_rotate_fb_obj_pages implementation into
i915_gem_gtt.c? Interesting, because I saw it differently - as a display
internal _implementation_ using the GEM GGTT view framework - and as
such does not belong in core GEM. Could you explain why you think so?
>> else
>> WARN_ONCE(1, "GGTT view %u not implemented!\n",
>> vma->ggtt_view.type);
>> @@ -2387,10 +2392,15 @@ int i915_get_vma_pages(struct i915_vma *vma)
>> if (!vma->ggtt_view.pages) {
>> DRM_ERROR("Failed to get pages for VMA 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..56a2356 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 pixel_size;
>> + unsigned int height;
>> + unsigned int pitch;
>> + 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 7a5d0a7..cb4dae8 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -2244,18 +2244,151 @@ intel_fb_align_height(struct drm_device *dev, int height, uint32_t pixel_format,
>> fb_format_modifier));
>> }
>>
>> -int
>> -intel_pin_and_fence_fb_obj(struct drm_plane *plane,
>> - struct drm_framebuffer *fb,
>> - struct intel_engine_cs *pipelined)
>> +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;
>> + }
>> + }
>> +}
>> +
>> +struct sg_table *intel_rotate_fb_obj_pages(struct i915_ggtt_view *ggtt_view,
>> + struct drm_i915_gem_object *obj)
>> {
>> - struct drm_device *dev = fb->dev;
>> + struct drm_device *dev = obj->base.dev;
>> struct drm_i915_private *dev_priv = dev->dev_private;
>> - struct drm_i915_gem_object *obj = intel_fb_obj(fb);
>> - u32 alignment;
>> - int ret;
>> + struct i915_address_space *ggtt = &dev_priv->gtt.base;
>> + 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_width, tile_height;
>> + unsigned int width_pages, height_pages;
>> + int ret = ENOMEM;
>>
>> - WARN_ON(!mutex_is_locked(&dev->struct_mutex));
>> + pages = i915_gem_obj_size(obj, ggtt) / PAGE_SIZE;
>> +
>> + /* Calculate tiling geometry. */
>> + tile_height = intel_tile_height(dev, rot_info->pixel_size * 8,
>> + rot_info->fb_modifier);
>> + tile_width = PAGE_SIZE / rot_info->pixel_size / tile_height;
>> + width_pages = DIV_ROUND_UP(rot_info->pitch / rot_info->pixel_size,
>> + tile_width);
>> + 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_size=%u, %ux%u tiles, %lu pages).\n",
>> + size, rot_info->pitch, rot_info->height,
>> + rot_info->pixel_size, 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_size=%u, %ux%u tiles, %lu pages)\n",
>> + size, ret, rot_info->pitch, rot_info->height,
>> + rot_info->pixel_size, width_pages, height_pages,
>> + rot_pages);
>> + return ERR_PTR(ret);
>> +}
>> +
>> +static
>> +const struct i915_ggtt_view i915_rotated_ggtt_view_template = {
>> + .type = I915_GGTT_VIEW_ROTATED,
>> +};
>
> Not really needed, you can just use a struct assignment instaed of the
> memcpy below, i.e.
>
> *view = {.type = I915_GGTT_VIEW_ROTATED};
>
> gcc then clears everyhing else.
Will change.
>> +
>> +static
>> +int intel_fb_ggtt_view(struct drm_plane_state *plane_state,
>> + struct drm_framebuffer *fb,
>> + struct i915_ggtt_view *view)
>
> I think intel_fill_fb_ggtt_view would be a clearer name for what this
> does. Also generally we put the struct a function operations on first (to
> fake OOP principles) and group later parameters by topic and then object
> first and flags/data later. So here I'd go with (view, fb, plane_state).
Makes sense, I just mindlessly expanded it in this version without
taking a step back to re-evalute the bigger picture.
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH 3/5] drm/i915/skl: Support secondary (rotated) frame buffer mapping
2015-03-03 9:59 ` Tvrtko Ursulin
@ 2015-03-04 14:43 ` Daniel Vetter
2015-03-04 15:04 ` Tvrtko Ursulin
0 siblings, 1 reply; 11+ messages in thread
From: Daniel Vetter @ 2015-03-04 14:43 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: Intel-gfx
On Tue, Mar 03, 2015 at 09:59:31AM +0000, Tvrtko Ursulin wrote:
>
> On 03/02/2015 06:21 PM, Daniel Vetter wrote:
> >On Mon, Mar 02, 2015 at 02:43:50PM +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 suchs 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)
> >>
> >>For: VIZ-4726
> >>Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>Reviewed-by: Michel Thierry <michel.thierry@intel.com> (v4)
> >
> >Bunch of nitpicks below. Also I think it'd be good to split this patch
> >into the rote refactoring work to add view parameters all over the place
> >and the actual implementation.
>
> Ok will split it. Rest below...
>
> >>---
> >> drivers/gpu/drm/i915/i915_drv.h | 33 +++++-
> >> drivers/gpu/drm/i915/i915_gem.c | 27 +++--
> >> drivers/gpu/drm/i915/i915_gem_gtt.c | 14 ++-
> >> drivers/gpu/drm/i915/i915_gem_gtt.h | 12 +++
> >> drivers/gpu/drm/i915/intel_display.c | 204 +++++++++++++++++++++++++++++++----
> >> drivers/gpu/drm/i915/intel_drv.h | 4 +
> >> drivers/gpu/drm/i915/intel_fbdev.c | 2 +-
> >> drivers/gpu/drm/i915/intel_overlay.c | 3 +-
> >> 8 files changed, 263 insertions(+), 36 deletions(-)
> >>
> >>diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> >>index e07a1cb..79d3f2c 100644
> >>--- a/drivers/gpu/drm/i915/i915_drv.h
> >>+++ b/drivers/gpu/drm/i915/i915_drv.h
> >>@@ -2743,8 +2743,10 @@ i915_gem_object_set_to_cpu_domain(struct drm_i915_gem_object *obj, bool write);
> >> int __must_check
> >> i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
> >> u32 alignment,
> >>- struct intel_engine_cs *pipelined);
> >>-void i915_gem_object_unpin_from_display_plane(struct drm_i915_gem_object *obj);
> >>+ struct intel_engine_cs *pipelined,
> >>+ const struct i915_ggtt_view *view);
> >>+void i915_gem_object_unpin_from_display_plane(struct drm_i915_gem_object *obj,
> >>+ const struct i915_ggtt_view *view);
> >> int i915_gem_object_attach_phys(struct drm_i915_gem_object *obj,
> >> int align);
> >> int i915_gem_open(struct drm_device *dev, struct drm_file *file);
> >>@@ -2813,7 +2815,13 @@ i915_gem_obj_lookup_or_create_vma(struct drm_i915_gem_object *obj,
> >> &i915_ggtt_view_normal);
> >> }
> >>
> >>-struct i915_vma *i915_gem_obj_to_ggtt(struct drm_i915_gem_object *obj);
> >>+struct i915_vma *i915_gem_obj_to_ggtt_view(struct drm_i915_gem_object *obj,
> >>+ enum i915_ggtt_view_type view);
> >>+static inline
> >>+struct i915_vma *i915_gem_obj_to_ggtt(struct drm_i915_gem_object *obj)
> >>+{
> >>+ return i915_gem_obj_to_ggtt_view(obj, I915_GGTT_VIEW_NORMAL);
> >>+}
> >> static inline bool i915_gem_obj_is_pinned(struct drm_i915_gem_object *obj) {
> >> struct i915_vma *vma;
> >> list_for_each_entry(vma, &obj->vma_list, vma_link)
> >>@@ -2867,13 +2875,30 @@ i915_gem_obj_ggtt_pin(struct drm_i915_gem_object *obj,
> >> alignment, flags | PIN_GLOBAL);
> >> }
> >>
> >>+static inline int __must_check
> >>+i915_gem_obj_ggtt_pin_view(struct drm_i915_gem_object *obj,
> >>+ uint32_t alignment,
> >>+ unsigned flags,
> >>+ const struct i915_ggtt_view *ggtt_view)
> >>+{
> >>+ return i915_gem_object_pin_view(obj, i915_obj_to_ggtt(obj),
> >>+ alignment, flags | PIN_GLOBAL,
> >>+ ggtt_view);
> >>+}
> >>+
> >> static inline int
> >> i915_gem_object_ggtt_unbind(struct drm_i915_gem_object *obj)
> >> {
> >> return i915_vma_unbind(i915_gem_obj_to_ggtt(obj));
> >> }
> >>
> >>-void i915_gem_object_ggtt_unpin(struct drm_i915_gem_object *obj);
> >>+void i915_gem_object_ggtt_unpin_view(struct drm_i915_gem_object *obj,
> >>+ enum i915_ggtt_view_type view);
> >>+static inline void
> >>+i915_gem_object_ggtt_unpin(struct drm_i915_gem_object *obj)
> >>+{
> >>+ i915_gem_object_ggtt_unpin_view(obj, I915_GGTT_VIEW_NORMAL);
> >>+}
> >>
> >> /* i915_gem_context.c */
> >> int __must_check i915_gem_context_init(struct drm_device *dev);
> >>diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> >>index 0107c2a..04c0cb1 100644
> >>--- a/drivers/gpu/drm/i915/i915_gem.c
> >>+++ b/drivers/gpu/drm/i915/i915_gem.c
> >>@@ -3938,7 +3938,8 @@ static bool is_pin_display(struct drm_i915_gem_object *obj)
> >> int
> >> i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
> >> u32 alignment,
> >>- struct intel_engine_cs *pipelined)
> >>+ struct intel_engine_cs *pipelined,
> >>+ const struct i915_ggtt_view *view)
> >> {
> >> u32 old_read_domains, old_write_domain;
> >> bool was_pin_display;
> >>@@ -3974,7 +3975,9 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
> >> * (e.g. libkms for the bootup splash), we have to ensure that we
> >> * always use map_and_fenceable for all scanout buffers.
> >> */
> >>- ret = i915_gem_obj_ggtt_pin(obj, alignment, PIN_MAPPABLE);
> >>+ ret = i915_gem_obj_ggtt_pin_view(obj, alignment,
> >>+ view->type == I915_GGTT_VIEW_NORMAL ?
> >>+ PIN_MAPPABLE : 0, view);
> >> if (ret)
> >> goto err_unpin_display;
> >>
> >>@@ -4002,9 +4005,11 @@ err_unpin_display:
> >> }
> >>
> >> void
> >>-i915_gem_object_unpin_from_display_plane(struct drm_i915_gem_object *obj)
> >>+i915_gem_object_unpin_from_display_plane(struct drm_i915_gem_object *obj,
> >>+ const struct i915_ggtt_view *view)
> >> {
> >>- i915_gem_object_ggtt_unpin(obj);
> >>+ i915_gem_object_ggtt_unpin_view(obj, view->type);
> >>+
> >> obj->pin_display = is_pin_display(obj);
> >> }
> >>
> >>@@ -4241,15 +4246,16 @@ i915_gem_object_pin_view(struct drm_i915_gem_object *obj,
> >> }
> >>
> >> void
> >>-i915_gem_object_ggtt_unpin(struct drm_i915_gem_object *obj)
> >>+i915_gem_object_ggtt_unpin_view(struct drm_i915_gem_object *obj,
> >>+ enum i915_ggtt_view_type view)
> >> {
> >>- struct i915_vma *vma = i915_gem_obj_to_ggtt(obj);
> >>+ struct i915_vma *vma = i915_gem_obj_to_ggtt_view(obj, view);
> >>
> >> BUG_ON(!vma);
> >> BUG_ON(vma->pin_count == 0);
> >>- BUG_ON(!i915_gem_obj_ggtt_bound(obj));
> >>+ BUG_ON(!i915_gem_obj_bound_view(obj, i915_obj_to_ggtt(obj), view));
> >>
> >>- if (--vma->pin_count == 0)
> >>+ if (--vma->pin_count == 0 && view == I915_GGTT_VIEW_NORMAL)
> >> obj->pin_mappable = false;
> >> }
> >>
> >>@@ -5306,14 +5312,15 @@ i915_gem_shrinker_oom(struct notifier_block *nb, unsigned long event, void *ptr)
> >> return NOTIFY_DONE;
> >> }
> >>
> >>-struct i915_vma *i915_gem_obj_to_ggtt(struct drm_i915_gem_object *obj)
> >>+struct i915_vma *i915_gem_obj_to_ggtt_view(struct drm_i915_gem_object *obj,
> >>+ enum i915_ggtt_view_type view)
> >> {
> >> struct i915_address_space *ggtt = i915_obj_to_ggtt(obj);
> >> struct i915_vma *vma;
> >>
> >> list_for_each_entry(vma, &obj->vma_list, vma_link)
> >> if (vma->vm == ggtt &&
> >>- vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL)
> >>+ vma->ggtt_view.type == view)
> >> return vma;
> >>
> >> return NULL;
> >>diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> >>index bd95776..9336142 100644
> >>--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> >>+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> >>@@ -2375,11 +2375,16 @@ i915_gem_obj_lookup_or_create_vma_view(struct drm_i915_gem_object *obj,
> >> static inline
> >> int i915_get_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);
> >
> >There's a bit a layering confusion going on, ggtt view code should be here
> >in the gem code with an appropriate prefix. Just moving the code is all
> >that's imo needed.
>
> You would move intel_rotate_fb_obj_pages implementation into i915_gem_gtt.c?
> Interesting, because I saw it differently - as a display internal
> _implementation_ using the GEM GGTT view framework - and as such does not
> belong in core GEM. Could you explain why you think so?
Mostly so that code in the same layer is in the same source files. We have
display code in intel_display which calls into the gem memory management
code exclusively through intel_pin_and_fence_fb_obj.
Manging of vmas and ptes and all that is gem territory and hence all the
different view imo should be there. We might want to eventually extract an
i915_gem_vma.c file and put all that stuff in there. Imo the benefit is
that if you need to make changes to a view they're all in the same spot
and adding another one you have all the examples locally.
There's nothing wrong with your approach, it's just how we organize
functions into files for i915. Maybe a clearer example would be basic gen
enabling. Atm it's all spread out over the different layers and functional
parts of the driver, but we could very well extract things and do files
per-gen (or maybe per hw block since they don't change in lockstep
everywhere). radeon is more organized like that as an example. But for
i915 we have all the gtt code in i915_gem_gtt.c, all the reset code in
i915_gpu_error.c, all the legacy ringuffer stuff in intel_ringbuffer.c.
And hence I also think we should put all the vma and view related things
together too.
I hope this explains the idea.
Cheers, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH 3/5] drm/i915/skl: Support secondary (rotated) frame buffer mapping
2015-03-04 14:43 ` Daniel Vetter
@ 2015-03-04 15:04 ` Tvrtko Ursulin
0 siblings, 0 replies; 11+ messages in thread
From: Tvrtko Ursulin @ 2015-03-04 15:04 UTC (permalink / raw)
To: Daniel Vetter; +Cc: Intel-gfx
On 03/04/2015 02:43 PM, Daniel Vetter wrote:
> On Tue, Mar 03, 2015 at 09:59:31AM +0000, Tvrtko Ursulin wrote:
>>
>> On 03/02/2015 06:21 PM, Daniel Vetter wrote:
>>> On Mon, Mar 02, 2015 at 02:43:50PM +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 suchs 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)
>>>>
>>>> For: VIZ-4726
>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>> Reviewed-by: Michel Thierry <michel.thierry@intel.com> (v4)
>>>
>>> Bunch of nitpicks below. Also I think it'd be good to split this patch
>>> into the rote refactoring work to add view parameters all over the place
>>> and the actual implementation.
>>
>> Ok will split it. Rest below...
>>
>>>> ---
>>>> drivers/gpu/drm/i915/i915_drv.h | 33 +++++-
>>>> drivers/gpu/drm/i915/i915_gem.c | 27 +++--
>>>> drivers/gpu/drm/i915/i915_gem_gtt.c | 14 ++-
>>>> drivers/gpu/drm/i915/i915_gem_gtt.h | 12 +++
>>>> drivers/gpu/drm/i915/intel_display.c | 204 +++++++++++++++++++++++++++++++----
>>>> drivers/gpu/drm/i915/intel_drv.h | 4 +
>>>> drivers/gpu/drm/i915/intel_fbdev.c | 2 +-
>>>> drivers/gpu/drm/i915/intel_overlay.c | 3 +-
>>>> 8 files changed, 263 insertions(+), 36 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>>>> index e07a1cb..79d3f2c 100644
>>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>>> @@ -2743,8 +2743,10 @@ i915_gem_object_set_to_cpu_domain(struct drm_i915_gem_object *obj, bool write);
>>>> int __must_check
>>>> i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
>>>> u32 alignment,
>>>> - struct intel_engine_cs *pipelined);
>>>> -void i915_gem_object_unpin_from_display_plane(struct drm_i915_gem_object *obj);
>>>> + struct intel_engine_cs *pipelined,
>>>> + const struct i915_ggtt_view *view);
>>>> +void i915_gem_object_unpin_from_display_plane(struct drm_i915_gem_object *obj,
>>>> + const struct i915_ggtt_view *view);
>>>> int i915_gem_object_attach_phys(struct drm_i915_gem_object *obj,
>>>> int align);
>>>> int i915_gem_open(struct drm_device *dev, struct drm_file *file);
>>>> @@ -2813,7 +2815,13 @@ i915_gem_obj_lookup_or_create_vma(struct drm_i915_gem_object *obj,
>>>> &i915_ggtt_view_normal);
>>>> }
>>>>
>>>> -struct i915_vma *i915_gem_obj_to_ggtt(struct drm_i915_gem_object *obj);
>>>> +struct i915_vma *i915_gem_obj_to_ggtt_view(struct drm_i915_gem_object *obj,
>>>> + enum i915_ggtt_view_type view);
>>>> +static inline
>>>> +struct i915_vma *i915_gem_obj_to_ggtt(struct drm_i915_gem_object *obj)
>>>> +{
>>>> + return i915_gem_obj_to_ggtt_view(obj, I915_GGTT_VIEW_NORMAL);
>>>> +}
>>>> static inline bool i915_gem_obj_is_pinned(struct drm_i915_gem_object *obj) {
>>>> struct i915_vma *vma;
>>>> list_for_each_entry(vma, &obj->vma_list, vma_link)
>>>> @@ -2867,13 +2875,30 @@ i915_gem_obj_ggtt_pin(struct drm_i915_gem_object *obj,
>>>> alignment, flags | PIN_GLOBAL);
>>>> }
>>>>
>>>> +static inline int __must_check
>>>> +i915_gem_obj_ggtt_pin_view(struct drm_i915_gem_object *obj,
>>>> + uint32_t alignment,
>>>> + unsigned flags,
>>>> + const struct i915_ggtt_view *ggtt_view)
>>>> +{
>>>> + return i915_gem_object_pin_view(obj, i915_obj_to_ggtt(obj),
>>>> + alignment, flags | PIN_GLOBAL,
>>>> + ggtt_view);
>>>> +}
>>>> +
>>>> static inline int
>>>> i915_gem_object_ggtt_unbind(struct drm_i915_gem_object *obj)
>>>> {
>>>> return i915_vma_unbind(i915_gem_obj_to_ggtt(obj));
>>>> }
>>>>
>>>> -void i915_gem_object_ggtt_unpin(struct drm_i915_gem_object *obj);
>>>> +void i915_gem_object_ggtt_unpin_view(struct drm_i915_gem_object *obj,
>>>> + enum i915_ggtt_view_type view);
>>>> +static inline void
>>>> +i915_gem_object_ggtt_unpin(struct drm_i915_gem_object *obj)
>>>> +{
>>>> + i915_gem_object_ggtt_unpin_view(obj, I915_GGTT_VIEW_NORMAL);
>>>> +}
>>>>
>>>> /* i915_gem_context.c */
>>>> int __must_check i915_gem_context_init(struct drm_device *dev);
>>>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>>>> index 0107c2a..04c0cb1 100644
>>>> --- a/drivers/gpu/drm/i915/i915_gem.c
>>>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>>>> @@ -3938,7 +3938,8 @@ static bool is_pin_display(struct drm_i915_gem_object *obj)
>>>> int
>>>> i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
>>>> u32 alignment,
>>>> - struct intel_engine_cs *pipelined)
>>>> + struct intel_engine_cs *pipelined,
>>>> + const struct i915_ggtt_view *view)
>>>> {
>>>> u32 old_read_domains, old_write_domain;
>>>> bool was_pin_display;
>>>> @@ -3974,7 +3975,9 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
>>>> * (e.g. libkms for the bootup splash), we have to ensure that we
>>>> * always use map_and_fenceable for all scanout buffers.
>>>> */
>>>> - ret = i915_gem_obj_ggtt_pin(obj, alignment, PIN_MAPPABLE);
>>>> + ret = i915_gem_obj_ggtt_pin_view(obj, alignment,
>>>> + view->type == I915_GGTT_VIEW_NORMAL ?
>>>> + PIN_MAPPABLE : 0, view);
>>>> if (ret)
>>>> goto err_unpin_display;
>>>>
>>>> @@ -4002,9 +4005,11 @@ err_unpin_display:
>>>> }
>>>>
>>>> void
>>>> -i915_gem_object_unpin_from_display_plane(struct drm_i915_gem_object *obj)
>>>> +i915_gem_object_unpin_from_display_plane(struct drm_i915_gem_object *obj,
>>>> + const struct i915_ggtt_view *view)
>>>> {
>>>> - i915_gem_object_ggtt_unpin(obj);
>>>> + i915_gem_object_ggtt_unpin_view(obj, view->type);
>>>> +
>>>> obj->pin_display = is_pin_display(obj);
>>>> }
>>>>
>>>> @@ -4241,15 +4246,16 @@ i915_gem_object_pin_view(struct drm_i915_gem_object *obj,
>>>> }
>>>>
>>>> void
>>>> -i915_gem_object_ggtt_unpin(struct drm_i915_gem_object *obj)
>>>> +i915_gem_object_ggtt_unpin_view(struct drm_i915_gem_object *obj,
>>>> + enum i915_ggtt_view_type view)
>>>> {
>>>> - struct i915_vma *vma = i915_gem_obj_to_ggtt(obj);
>>>> + struct i915_vma *vma = i915_gem_obj_to_ggtt_view(obj, view);
>>>>
>>>> BUG_ON(!vma);
>>>> BUG_ON(vma->pin_count == 0);
>>>> - BUG_ON(!i915_gem_obj_ggtt_bound(obj));
>>>> + BUG_ON(!i915_gem_obj_bound_view(obj, i915_obj_to_ggtt(obj), view));
>>>>
>>>> - if (--vma->pin_count == 0)
>>>> + if (--vma->pin_count == 0 && view == I915_GGTT_VIEW_NORMAL)
>>>> obj->pin_mappable = false;
>>>> }
>>>>
>>>> @@ -5306,14 +5312,15 @@ i915_gem_shrinker_oom(struct notifier_block *nb, unsigned long event, void *ptr)
>>>> return NOTIFY_DONE;
>>>> }
>>>>
>>>> -struct i915_vma *i915_gem_obj_to_ggtt(struct drm_i915_gem_object *obj)
>>>> +struct i915_vma *i915_gem_obj_to_ggtt_view(struct drm_i915_gem_object *obj,
>>>> + enum i915_ggtt_view_type view)
>>>> {
>>>> struct i915_address_space *ggtt = i915_obj_to_ggtt(obj);
>>>> struct i915_vma *vma;
>>>>
>>>> list_for_each_entry(vma, &obj->vma_list, vma_link)
>>>> if (vma->vm == ggtt &&
>>>> - vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL)
>>>> + vma->ggtt_view.type == view)
>>>> return vma;
>>>>
>>>> return NULL;
>>>> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
>>>> index bd95776..9336142 100644
>>>> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
>>>> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
>>>> @@ -2375,11 +2375,16 @@ i915_gem_obj_lookup_or_create_vma_view(struct drm_i915_gem_object *obj,
>>>> static inline
>>>> int i915_get_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);
>>>
>>> There's a bit a layering confusion going on, ggtt view code should be here
>>> in the gem code with an appropriate prefix. Just moving the code is all
>>> that's imo needed.
>>
>> You would move intel_rotate_fb_obj_pages implementation into i915_gem_gtt.c?
>> Interesting, because I saw it differently - as a display internal
>> _implementation_ using the GEM GGTT view framework - and as such does not
>> belong in core GEM. Could you explain why you think so?
>
> Mostly so that code in the same layer is in the same source files. We have
> display code in intel_display which calls into the gem memory management
> code exclusively through intel_pin_and_fence_fb_obj.
>
> Manging of vmas and ptes and all that is gem territory and hence all the
> different view imo should be there. We might want to eventually extract an
> i915_gem_vma.c file and put all that stuff in there. Imo the benefit is
> that if you need to make changes to a view they're all in the same spot
> and adding another one you have all the examples locally.
>
> There's nothing wrong with your approach, it's just how we organize
> functions into files for i915. Maybe a clearer example would be basic gen
> enabling. Atm it's all spread out over the different layers and functional
> parts of the driver, but we could very well extract things and do files
> per-gen (or maybe per hw block since they don't change in lockstep
> everywhere). radeon is more organized like that as an example. But for
> i915 we have all the gtt code in i915_gem_gtt.c, all the reset code in
> i915_gpu_error.c, all the legacy ringuffer stuff in intel_ringbuffer.c.
>
> And hence I also think we should put all the vma and view related things
> together too.
What about creating something like intel_display_rotation.c then to meet
half-way? :)
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 4/5] drm/i915/skl: Query display address through a wrapper
2015-03-02 14:43 [PATCH 0/5] Skylake 90/270 display rotation Tvrtko Ursulin
` (2 preceding siblings ...)
2015-03-02 14:43 ` [PATCH 3/5] drm/i915/skl: Support secondary (rotated) frame buffer mapping Tvrtko Ursulin
@ 2015-03-02 14:43 ` Tvrtko Ursulin
2015-03-02 14:43 ` [PATCH 5/5] drm/i915/skl: Take 90/270 rotation into account in watermark calculations Tvrtko Ursulin
4 siblings, 0 replies; 11+ messages in thread
From: Tvrtko Ursulin @ 2015-03-02 14:43 UTC (permalink / raw)
To: Intel-gfx
From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Need to do this in order to support 90/270 rotated display.
v2: Pass in drm_plane instead of plane index to intel_obj_display_address.
v3:
* Renamed intel_obj_display_address to intel_plane_obj_offset.
(Chris Wilson)
* Simplified rotation check to bitwise AND. (Chris Wilson)
v4:
* Extracted 90/270 rotation check into a helper function. (Michel Thierry)
For: VIZ-4545
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Reviewed-by: Michel Thierry <michel.thierry@intel.com>
---
drivers/gpu/drm/i915/i915_drv.h | 7 +++++++
drivers/gpu/drm/i915/i915_gem.c | 4 ++--
drivers/gpu/drm/i915/intel_display.c | 24 ++++++++++++++++++------
drivers/gpu/drm/i915/intel_drv.h | 8 ++++++++
drivers/gpu/drm/i915/intel_sprite.c | 5 ++++-
5 files changed, 39 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 79d3f2c..98a4ac5 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2861,6 +2861,13 @@ i915_gem_obj_ggtt_offset(struct drm_i915_gem_object *obj)
}
static inline unsigned long
+i915_gem_obj_ggtt_offset_view(struct drm_i915_gem_object *obj,
+ enum i915_ggtt_view_type view)
+{
+ return i915_gem_obj_offset_view(obj, i915_obj_to_ggtt(obj), view);
+}
+
+static inline unsigned long
i915_gem_obj_ggtt_size(struct drm_i915_gem_object *obj)
{
return i915_gem_obj_size(obj, i915_obj_to_ggtt(obj));
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 04c0cb1..f752c7f5a 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -5168,8 +5168,8 @@ unsigned long i915_gem_obj_offset_view(struct drm_i915_gem_object *o,
return vma->node.start;
}
- WARN(1, "%s vma for this object not found.\n",
- i915_is_ggtt(vm) ? "global" : "ppgtt");
+ WARN(1, "%s vma for this object not found. (view=%u)\n",
+ i915_is_ggtt(vm) ? "global" : "ppgtt", view);
return -1;
}
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index cb4dae8..15c28b1 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2363,8 +2363,7 @@ int intel_fb_ggtt_view(struct drm_plane_state *plane_state,
if (!plane_state)
return 0;
- if (plane_state->rotation &
- (BIT(DRM_ROTATE_90) | BIT(DRM_ROTATE_270))) == 0)
+ if (!intel_rotation_90_or_270(plane_state->rotation))
return 0;
memcpy(view, &i915_rotated_ggtt_view_template, sizeof(*view));
@@ -2980,6 +2979,17 @@ u32 intel_fb_stride_alignment(struct drm_device *dev, uint64_t fb_modifier,
}
}
+unsigned long intel_plane_obj_offset(struct intel_plane *intel_plane,
+ struct drm_i915_gem_object *obj)
+{
+ enum i915_ggtt_view_type view = I915_GGTT_VIEW_NORMAL;
+
+ if (intel_rotation_90_or_270(intel_plane->base.state->rotation))
+ view = I915_GGTT_VIEW_ROTATED;
+
+ return i915_gem_obj_ggtt_offset_view(obj, view);
+}
+
static void skylake_update_primary_plane(struct drm_crtc *crtc,
struct drm_framebuffer *fb,
int x, int y)
@@ -2990,6 +3000,7 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
struct drm_i915_gem_object *obj;
int pipe = intel_crtc->pipe;
u32 plane_ctl, stride_div;
+ unsigned long surf_addr;
if (!intel_crtc->primary_enabled) {
I915_WRITE(PLANE_CTL(pipe, 0), 0);
@@ -3056,11 +3067,12 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
obj = intel_fb_obj(fb);
stride_div = intel_fb_stride_alignment(dev, fb->modifier[0],
fb->pixel_format);
+ surf_addr = intel_plane_obj_offset(to_intel_plane(crtc->primary), obj);
I915_WRITE(PLANE_CTL(pipe, 0), plane_ctl);
DRM_DEBUG_KMS("Writing base %08lX %d,%d,%d,%d pitch=%d\n",
- i915_gem_obj_ggtt_offset(obj),
+ surf_addr,
x, y, fb->width, fb->height,
fb->pitches[0]);
@@ -3070,7 +3082,7 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
(intel_crtc->config->pipe_src_h - 1) << 16 |
(intel_crtc->config->pipe_src_w - 1));
I915_WRITE(PLANE_STRIDE(pipe, 0), fb->pitches[0] / stride_div);
- I915_WRITE(PLANE_SURF(pipe, 0), i915_gem_obj_ggtt_offset(obj));
+ I915_WRITE(PLANE_SURF(pipe, 0), surf_addr);
POSTING_READ(PLANE_SURF(pipe, 0));
}
@@ -10102,8 +10114,8 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
if (ret)
goto cleanup_pending;
- work->gtt_offset =
- i915_gem_obj_ggtt_offset(obj) + intel_crtc->dspaddr_offset;
+ work->gtt_offset = intel_plane_obj_offset(to_intel_plane(primary), obj)
+ + intel_crtc->dspaddr_offset;
if (use_mmio_flip(ring, obj)) {
ret = intel_queue_mmio_flip(dev, crtc, fb, obj, ring,
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 9c2145d..5690aa6 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -980,6 +980,11 @@ int intel_plane_atomic_set_property(struct drm_plane *plane,
struct drm_property *property,
uint64_t val);
+static inline bool
+intel_rotation_90_or_270(unsigned int rotation)
+{
+ return rotation & (BIT(DRM_ROTATE_90) | BIT(DRM_ROTATE_270));
+}
struct sg_table *intel_rotate_fb_obj_pages(struct i915_ggtt_view *ggtt_view,
struct drm_i915_gem_object *obj);
@@ -1037,6 +1042,9 @@ void intel_mode_from_pipe_config(struct drm_display_mode *mode,
void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc);
void intel_modeset_preclose(struct drm_device *dev, struct drm_file *file);
+unsigned long intel_plane_obj_offset(struct intel_plane *intel_plane,
+ struct drm_i915_gem_object *obj);
+
/* intel_dp.c */
void intel_dp_init(struct drm_device *dev, int output_reg, enum port port);
bool intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 7051da7..caa0bcc 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -191,6 +191,7 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
const int plane = intel_plane->plane + 1;
u32 plane_ctl, stride_div;
int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
+ unsigned long surf_addr;
plane_ctl = I915_READ(PLANE_CTL(pipe, plane));
@@ -280,12 +281,14 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
crtc_w--;
crtc_h--;
+ surf_addr = intel_plane_obj_offset(intel_plane, obj);
+
I915_WRITE(PLANE_OFFSET(pipe, plane), (y << 16) | x);
I915_WRITE(PLANE_STRIDE(pipe, plane), fb->pitches[0] / stride_div);
I915_WRITE(PLANE_POS(pipe, plane), (crtc_y << 16) | crtc_x);
I915_WRITE(PLANE_SIZE(pipe, plane), (crtc_h << 16) | crtc_w);
I915_WRITE(PLANE_CTL(pipe, plane), plane_ctl);
- I915_WRITE(PLANE_SURF(pipe, plane), i915_gem_obj_ggtt_offset(obj));
+ I915_WRITE(PLANE_SURF(pipe, plane), surf_addr);
POSTING_READ(PLANE_SURF(pipe, plane));
}
--
2.3.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH 5/5] drm/i915/skl: Take 90/270 rotation into account in watermark calculations
2015-03-02 14:43 [PATCH 0/5] Skylake 90/270 display rotation Tvrtko Ursulin
` (3 preceding siblings ...)
2015-03-02 14:43 ` [PATCH 4/5] drm/i915/skl: Query display address through a wrapper Tvrtko Ursulin
@ 2015-03-02 14:43 ` Tvrtko Ursulin
4 siblings, 0 replies; 11+ messages in thread
From: Tvrtko Ursulin @ 2015-03-02 14:43 UTC (permalink / raw)
To: Intel-gfx
From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
v2: Pass in rotation info to sprite plane updates as well.
v3: Use helper to determine 90/270 rotation. (Michel Thierry)
v4: Rebased for fb modifiers and atomic changes.
For: VIZ-4546
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Reviewed-by: Michel Thierry <michel.thierry@intel.com> (v3)
---
drivers/gpu/drm/i915/intel_display.c | 27 +++++++++++++++++++++++----
drivers/gpu/drm/i915/intel_drv.h | 4 ++++
drivers/gpu/drm/i915/intel_pm.c | 18 +++++++++++++++++-
drivers/gpu/drm/i915/intel_sprite.c | 5 +----
4 files changed, 45 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 15c28b1..fb07d3d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12064,6 +12064,28 @@ static void intel_shared_dpll_init(struct drm_device *dev)
}
/**
+ * intel_wm_need_update - Check whether watermarks need updating
+ * @plane: drm plane
+ * @state: new plane state
+ *
+ * Check current plane state versus the new one to determine whether
+ * watermarks need to be recalculated.
+ *
+ * Returns true or false.
+ */
+bool intel_wm_need_update(struct drm_plane *plane,
+ struct drm_plane_state *state)
+{
+ /* Update watermarks on tiling changes. */
+ if (!plane->state->fb || !state->fb ||
+ plane->state->fb->modifier[0] != state->fb->modifier[0] ||
+ plane->state->rotation != state->rotation)
+ return true;
+
+ return false;
+}
+
+/**
* intel_prepare_plane_fb - Prepare fb for usage on plane
* @plane: drm plane to prepare for
* @fb: framebuffer to prepare for presentation
@@ -12209,10 +12231,7 @@ intel_check_primary_plane(struct drm_plane *plane,
intel_crtc->atomic.update_fbc = true;
- /* Update watermarks on tiling changes. */
- if (!plane->state->fb || !state->base.fb ||
- plane->state->fb->modifier[0] !=
- state->base.fb->modifier[0])
+ if (intel_wm_need_update(plane, &state->base))
intel_crtc->atomic.update_wm = true;
}
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 5690aa6..715510a 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -501,6 +501,7 @@ struct intel_plane_wm_parameters {
bool enabled;
bool scaled;
u64 tiling;
+ unsigned int rotation;
};
struct intel_plane {
@@ -988,6 +989,9 @@ intel_rotation_90_or_270(unsigned int rotation)
struct sg_table *intel_rotate_fb_obj_pages(struct i915_ggtt_view *ggtt_view,
struct drm_i915_gem_object *obj);
+bool intel_wm_need_update(struct drm_plane *plane,
+ struct drm_plane_state *state);
+
/* 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,
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 0bf6767..bc90a14 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2712,6 +2712,7 @@ static void skl_compute_wm_pipe_parameters(struct drm_crtc *crtc,
*/
if (fb)
p->plane[0].tiling = fb->modifier[0];
+ p->plane[0].rotation = crtc->primary->state->rotation;
p->cursor.enabled = true;
p->cursor.bytes_per_pixel = 4;
@@ -2761,7 +2762,21 @@ static bool skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
if (p_params->tiling == I915_FORMAT_MOD_Y_TILED ||
p_params->tiling == I915_FORMAT_MOD_Yf_TILED) {
- uint32_t y_tile_minimum = plane_blocks_per_line * 4;
+ uint32_t min_scanlines = 4;
+ uint32_t y_tile_minimum;
+ if (intel_rotation_90_or_270(p_params->rotation)) {
+ switch (p_params->bytes_per_pixel) {
+ case 1:
+ min_scanlines = 16;
+ break;
+ case 2:
+ min_scanlines = 8;
+ break;
+ case 8:
+ WARN(1, "Unsupported pixel depth for rotation");
+ };
+ }
+ y_tile_minimum = plane_blocks_per_line * min_scanlines;
selected_result = max(method2, y_tile_minimum);
} else {
if ((ddb_allocation / plane_blocks_per_line) >= 1)
@@ -3222,6 +3237,7 @@ skl_update_sprite_wm(struct drm_plane *plane, struct drm_crtc *crtc,
*/
if (fb)
intel_plane->wm.tiling = fb->modifier[0];
+ intel_plane->wm.rotation = plane->state->rotation;
skl_update_wm(crtc);
}
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index caa0bcc..addc90e 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -1260,10 +1260,7 @@ finish:
if (!intel_crtc->primary_enabled && !state->hides_primary)
intel_crtc->atomic.post_enable_primary = true;
- /* Update watermarks on tiling changes. */
- if (!plane->state->fb || !state->base.fb ||
- plane->state->fb->modifier[0] !=
- state->base.fb->modifier[0])
+ if (intel_wm_need_update(plane, &state->base))
intel_crtc->atomic.update_wm = true;
}
--
2.3.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 11+ messages in thread