On Thu, Apr 02, 2015 at 10:24:02AM +0530, Jindal, Sonika wrote:
>
>
> On 4/1/2015 11:52 PM, Matt Roper wrote:
> >On Mon, Mar 30, 2015 at 02:04:57PM +0530, Sonika Jindal wrote:
> >>v2: Moving creation of property in a function, checking for 90/270
> >>rotation simultaneously (Chris)
> >>Letting primary plane to be positioned
> >>v3: Adding if/else for 90/270 and rest params programming, adding check for
> >>pixel_format, some cleanup (review comments)
> >>v4: Adding right pixel_formats, using src_* params instead of crtc_* for offset
> >>and size programming (Ville)
> >>v5: Rebased on -nightly and Tvrtko's series for gtt remapping.
> >>v6: Rebased on -nightly (Tvrtko's series merged)
> >>
> >>Signed-off-by: Sonika Jindal <sonika.jindal@xxxxxxxxx>
> >>---
> >> drivers/gpu/drm/i915/i915_reg.h | 2 +
> >> drivers/gpu/drm/i915/intel_display.c | 103 +++++++++++++++++++++++++++-------
> >> drivers/gpu/drm/i915/intel_drv.h | 6 ++
> >> drivers/gpu/drm/i915/intel_sprite.c | 52 ++++++++++++-----
> >> 4 files changed, 129 insertions(+), 34 deletions(-)
> >>
> >>diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> >>index b522eb6..564bbd5 100644
> >>--- a/drivers/gpu/drm/i915/i915_reg.h
> >>+++ b/drivers/gpu/drm/i915/i915_reg.h
> >>@@ -4854,7 +4854,9 @@ enum skl_disp_power_wells {
> >> #define PLANE_CTL_ALPHA_HW_PREMULTIPLY ( 3 << 4)
> >> #define PLANE_CTL_ROTATE_MASK 0x3
> >> #define PLANE_CTL_ROTATE_0 0x0
> >>+#define PLANE_CTL_ROTATE_90 0x1
> >> #define PLANE_CTL_ROTATE_180 0x2
> >>+#define PLANE_CTL_ROTATE_270 0x3
> >> #define _PLANE_STRIDE_1_A 0x70188
> >> #define _PLANE_STRIDE_2_A 0x70288
> >> #define _PLANE_STRIDE_3_A 0x70388
> >>diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >>index f0bbc22..86ee0f0 100644
> >>--- a/drivers/gpu/drm/i915/intel_display.c
> >>+++ b/drivers/gpu/drm/i915/intel_display.c
> >>@@ -2318,6 +2318,28 @@ intel_fill_fb_ggtt_view(struct i915_ggtt_view *view, struct drm_framebuffer *fb,
> >> return -EINVAL;
> >> }
> >>
> >>+ switch (fb->pixel_format) {
> >>+ case DRM_FORMAT_XRGB8888:
> >>+ case DRM_FORMAT_ARGB8888:
> >>+ case DRM_FORMAT_XBGR8888:
> >>+ case DRM_FORMAT_ABGR8888:
> >>+ case DRM_FORMAT_XRGB2101010:
> >>+ case DRM_FORMAT_ARGB2101010:
> >>+ case DRM_FORMAT_XBGR2101010:
> >>+ case DRM_FORMAT_ABGR2101010:
> >>+ case DRM_FORMAT_YUYV:
> >>+ case DRM_FORMAT_YVYU:
> >>+ case DRM_FORMAT_UYVY:
> >>+ case DRM_FORMAT_VYUY:
> >>+ case DRM_FORMAT_NV12:
> >>+ break;
> >>+
> >>+ default:
> >>+ DRM_DEBUG_KMS("Unsupported pixel format:%d for 90/270 rotation!\n",
> >>+ fb->pixel_format);
> >>+ return -EINVAL;
> >>+ }
> >
> >Shouldn't we be matching against the list of formats the plane supports
> >(which may vary by platform, or by specific plane) rather than this
> >generic list? We already specified what formats the plane can handle at
> >plane init time, so it seems like what you'd really want is just a call
> >to
> >
> > drm_plane_check_pixel_format(plane_state->plane, fb->pixel_format)
> >
> >then follow that up with explicit checks to exclude any formats that we
> >can handle in 0/180, but not in 90/270.
> >
> I am not sure how it will help. drm_plane_check_pixel_format should
> be used to check the pixel format of the fb which we should be doing
> in some -check functions (I don't think we do that right now?)
> against what is supported by the plane.
> But to check for the formats which are allowed for 90/270, we would
> need this kind of explicit check.
Right, I guess there are two aspects here. First, we need to properly
test for acceptable pixel formats for the plane in general; at the
moment the DRM core setplane() tests this, but if we use the atomic
ioctl it never gets checked (which is a bug). So as you say, we need a
test in a _check() function to verify this. We probably also need to
add an i-g-t test for it too.
Once we know that the pixel format is valid in general, it makes sense
to have a simpler test to reject some subset of those formats iff we
notice we're doing 90/270 rotation. Maybe it's not really a big deal,
but it seems like that's a little easier to understand and verify than
having two completely separate lists, especially when future platforms
may support different formats, or even different planes of the same
platform have varying pixel format capabilities.
Matt
> >I'd also move this check to intel_plane_atomic_check(), since the
> >'check' code path is where I'd usually go looking for these types of
> >checks; the function you've got it in at the moment gets called from the
> >'prepare' step which works as well, but seems a bit less obvious.
> >
> Yes, I agree, but this is on top of Tvrtko's patch for secondary
> buffer mapping where based upon tiling and pixel format we are
> allowing the rotated gtt.
>
> Tvrtko,
> Can these be moved to the intel_plane_atomic_check()
>
> >>+
> >> return 0;
> >> }
> >>
> >>@@ -2919,8 +2941,12 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
> >> struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> >> struct drm_i915_gem_object *obj;
> >> int pipe = intel_crtc->pipe;
> >>- u32 plane_ctl, stride_div;
> >>+ u32 plane_ctl, stride_div, stride;
> >>+ u32 tile_height, plane_offset, plane_size;
> >>+ unsigned int rotation;
> >>+ int x_offset, y_offset;
> >> unsigned long surf_addr;
> >>+ struct drm_plane *plane;
> >>
> >> if (!intel_crtc->primary_enabled) {
> >> I915_WRITE(PLANE_CTL(pipe, 0), 0);
> >>@@ -2981,21 +3007,51 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
> >> }
> >>
> >> plane_ctl |= PLANE_CTL_PLANE_GAMMA_DISABLE;
> >>- if (crtc->primary->state->rotation == BIT(DRM_ROTATE_180))
> >>+
> >>+ plane = crtc->primary;
> >>+ rotation = plane->state->rotation;
> >>+ switch (rotation) {
> >>+ case BIT(DRM_ROTATE_90):
> >>+ plane_ctl |= PLANE_CTL_ROTATE_90;
> >>+ break;
> >>+
> >>+ case BIT(DRM_ROTATE_180):
> >> plane_ctl |= PLANE_CTL_ROTATE_180;
> >>+ break;
> >>+
> >>+ case BIT(DRM_ROTATE_270):
> >>+ plane_ctl |= PLANE_CTL_ROTATE_270;
> >>+ break;
> >>+ }
> >>
> >> obj = intel_fb_obj(fb);
> >> stride_div = intel_fb_stride_alignment(dev, fb->modifier[0],
> >> fb->pixel_format);
> >
> >Minor nit; can we move this down inside the 'else' branch to make it
> >apparent how this serves a parallel role to 'tile_height' from the 'if'
> >branch.
> >
> Yes sure.
> >
> >>- surf_addr = intel_plane_obj_offset(to_intel_plane(crtc->primary), obj);
> >>+ surf_addr = intel_plane_obj_offset(to_intel_plane(plane), obj);
> >>+
> >>+ if (rotation & (BIT(DRM_ROTATE_90) | BIT(DRM_ROTATE_270))) {
> >
> >Might as well use the intel_rotation_90_or_270() function since we have
> >it. Same comment for the sprite path farther down.
> >
> Sure.
>
> Regards,
> Sonika
> >>+ /* stride = Surface height in tiles */
> >>+ tile_height = intel_tile_height(dev, fb->bits_per_pixel,
> >>+ fb->modifier[0]);
> >>+ stride = DIV_ROUND_UP(fb->height, tile_height);
> >>+ x_offset = stride * tile_height - y - (plane->state->src_h >> 16);
> >>+ y_offset = x;
> >>+ plane_size = ((plane->state->src_w >> 16) - 1) << 16 |
> >>+ ((plane->state->src_h >> 16) - 1);
> >>+ } else {
> >>+ stride = fb->pitches[0] / stride_div;
> >>+ x_offset = x;
> >>+ y_offset = y;
> >>+ plane_size = ((plane->state->src_h >> 16) - 1) << 16 |
> >>+ ((plane->state->src_w >> 16) - 1);
> >>+ }
> >>+ plane_offset = y_offset << 16 | x_offset;
> >>
> >> I915_WRITE(PLANE_CTL(pipe, 0), plane_ctl);
> >> I915_WRITE(PLANE_POS(pipe, 0), 0);
> >>- I915_WRITE(PLANE_OFFSET(pipe, 0), (y << 16) | x);
> >>- I915_WRITE(PLANE_SIZE(pipe, 0),
> >>- (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_OFFSET(pipe, 0), plane_offset);
> >>+ I915_WRITE(PLANE_SIZE(pipe, 0), plane_size);
> >>+ I915_WRITE(PLANE_STRIDE(pipe, 0), stride);
> >> I915_WRITE(PLANE_SURF(pipe, 0), surf_addr);
> >>
> >> POSTING_READ(PLANE_SURF(pipe, 0));
> >>@@ -12406,23 +12462,32 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
> >> intel_primary_formats, num_formats,
> >> DRM_PLANE_TYPE_PRIMARY);
> >>
> >>- if (INTEL_INFO(dev)->gen >= 4) {
> >>- if (!dev->mode_config.rotation_property)
> >>- dev->mode_config.rotation_property =
> >>- drm_mode_create_rotation_property(dev,
> >>- BIT(DRM_ROTATE_0) |
> >>- BIT(DRM_ROTATE_180));
> >>- if (dev->mode_config.rotation_property)
> >>- drm_object_attach_property(&primary->base.base,
> >>- dev->mode_config.rotation_property,
> >>- state->base.rotation);
> >>- }
> >>+ if (INTEL_INFO(dev)->gen >= 4)
> >>+ intel_create_rotation_property(dev, primary);
> >>
> >> drm_plane_helper_add(&primary->base, &intel_plane_helper_funcs);
> >>
> >> return &primary->base;
> >> }
> >>
> >>+void intel_create_rotation_property(struct drm_device *dev, struct intel_plane *plane)
> >>+{
> >>+ if (!dev->mode_config.rotation_property) {
> >>+ unsigned long flags = BIT(DRM_ROTATE_0) |
> >>+ BIT(DRM_ROTATE_180);
> >>+
> >>+ if (INTEL_INFO(dev)->gen >= 9)
> >>+ flags |= BIT(DRM_ROTATE_90) | BIT(DRM_ROTATE_270);
> >>+
> >>+ dev->mode_config.rotation_property =
> >>+ drm_mode_create_rotation_property(dev, flags);
> >>+ }
> >>+ if (dev->mode_config.rotation_property)
> >>+ drm_object_attach_property(&plane->base.base,
> >>+ dev->mode_config.rotation_property,
> >>+ plane->base.state->rotation);
> >>+}
> >>+
> >> static int
> >> intel_check_cursor_plane(struct drm_plane *plane,
> >> struct intel_plane_state *state)
> >>diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> >>index 811a1db..d32025a 100644
> >>--- a/drivers/gpu/drm/i915/intel_drv.h
> >>+++ b/drivers/gpu/drm/i915/intel_drv.h
> >>@@ -995,6 +995,12 @@ intel_rotation_90_or_270(unsigned int rotation)
> >> return rotation & (BIT(DRM_ROTATE_90) | BIT(DRM_ROTATE_270));
> >> }
> >>
> >>+unsigned int
> >>+intel_tile_height(struct drm_device *dev, uint32_t bits_per_pixel,
> >>+ uint64_t fb_modifier);
> >>+void intel_create_rotation_property(struct drm_device *dev,
> >>+ struct intel_plane *plane);
> >>+
> >> bool intel_wm_need_update(struct drm_plane *plane,
> >> struct drm_plane_state *state);
> >>
> >>diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> >>index f41e872..65eb147 100644
> >>--- a/drivers/gpu/drm/i915/intel_sprite.c
> >>+++ b/drivers/gpu/drm/i915/intel_sprite.c
> >>@@ -190,10 +190,13 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
> >> struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> >> const int pipe = intel_plane->pipe;
> >> const int plane = intel_plane->plane + 1;
> >>- u32 plane_ctl, stride_div;
> >>+ u32 plane_ctl, stride_div, stride;
> >> int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
> >> const struct drm_intel_sprite_colorkey *key = &intel_plane->ckey;
> >> unsigned long surf_addr;
> >>+ u32 tile_height, plane_offset, plane_size;
> >>+ unsigned int rotation;
> >>+ int x_offset, y_offset;
> >>
> >> plane_ctl = PLANE_CTL_ENABLE |
> >> PLANE_CTL_PIPE_CSC_ENABLE;
> >>@@ -254,8 +257,20 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
> >> MISSING_CASE(fb->modifier[0]);
> >> }
> >>
> >>- if (drm_plane->state->rotation == BIT(DRM_ROTATE_180))
> >>+ rotation = drm_plane->state->rotation;
> >>+ switch (rotation) {
> >>+ case BIT(DRM_ROTATE_90):
> >>+ plane_ctl |= PLANE_CTL_ROTATE_90;
> >>+ break;
> >>+
> >>+ case BIT(DRM_ROTATE_180):
> >> plane_ctl |= PLANE_CTL_ROTATE_180;
> >>+ break;
> >>+
> >>+ case BIT(DRM_ROTATE_270):
> >>+ plane_ctl |= PLANE_CTL_ROTATE_270;
> >>+ break;
> >>+ }
> >>
> >> intel_update_sprite_watermarks(drm_plane, crtc, src_w, src_h,
> >> pixel_size, true,
> >>@@ -283,10 +298,26 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
> >>
> >> 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);
> >>+ if (rotation & (BIT(DRM_ROTATE_90) | BIT(DRM_ROTATE_270))) {
> >>+ /* stride: Surface height in tiles */
> >>+ tile_height = intel_tile_height(dev, fb->bits_per_pixel,
> >>+ fb->modifier[0]);
> >>+ stride = DIV_ROUND_UP(fb->height, tile_height);
> >>+ plane_size = (src_w << 16) | src_h;
> >>+ x_offset = stride * tile_height - y - (src_h + 1);
> >>+ y_offset = x;
> >>+ } else {
> >>+ stride = fb->pitches[0] / stride_div;
> >>+ plane_size = (src_h << 16) | src_w;
> >>+ x_offset = x;
> >>+ y_offset = y;
> >>+ }
> >>+ plane_offset = y_offset << 16 | x_offset;
> >>+
> >>+ I915_WRITE(PLANE_OFFSET(pipe, plane), plane_offset);
> >>+ I915_WRITE(PLANE_STRIDE(pipe, plane), stride);
> >> 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_SIZE(pipe, plane), plane_size);
> >> I915_WRITE(PLANE_CTL(pipe, plane), plane_ctl);
> >> I915_WRITE(PLANE_SURF(pipe, plane), surf_addr);
> >> POSTING_READ(PLANE_SURF(pipe, plane));
> >>@@ -1310,16 +1341,7 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
> >> goto out;
> >> }
> >>
> >>- if (!dev->mode_config.rotation_property)
> >>- dev->mode_config.rotation_property =
> >>- drm_mode_create_rotation_property(dev,
> >>- BIT(DRM_ROTATE_0) |
> >>- BIT(DRM_ROTATE_180));
> >>-
> >>- if (dev->mode_config.rotation_property)
> >>- drm_object_attach_property(&intel_plane->base.base,
> >>- dev->mode_config.rotation_property,
> >>- state->base.rotation);
> >>+ intel_create_rotation_property(dev, intel_plane);
> >>
> >> drm_plane_helper_add(&intel_plane->base, &intel_plane_helper_funcs);
> >>
> >>--
> >>1.7.10.4
> >>
> >>_______________________________________________
> >>Intel-gfx mailing list
> >>Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> >>http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
--
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
