| Message ID | 1441989858-4511-1-git-send-email-chandra.konduru@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Sep 11, 2015 at 09:44:18AM -0700, Chandra Konduru wrote: > This patch adds NV12 as supported format to > intel_framebuffer_init and performs various checks. > > v2: > -Fix an issue in checks added (me) > > v3: > -cosmetic update, split checks into two (Ville) > > v4: > -Add stride alignment and modifier checks for UV subplane (Ville) > > v5: > -Make modifier check general (Ville) > -Check tile-y uv start alignment from begining of page (Ville) > > Signed-off-by: Chandra Konduru <chandra.konduru@intel.com> > Testcase: igt/kms_nv12 > --- > drivers/gpu/drm/i915/intel_display.c | 66 +++++++++++++++++++++++++++------- > drivers/gpu/drm/i915/intel_drv.h | 2 +- > drivers/gpu/drm/i915/intel_sprite.c | 2 +- > 3 files changed, 55 insertions(+), 15 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 2a5170e..af28ca9 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -2914,9 +2914,9 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc, > } > > u32 intel_fb_stride_alignment(struct drm_device *dev, uint64_t fb_modifier, > - uint32_t pixel_format) > + uint32_t pixel_format, int plane) > { > - u32 bits_per_pixel = drm_format_plane_cpp(pixel_format, 0) * 8; > + u32 bits_per_pixel = drm_format_plane_cpp(pixel_format, plane) * 8; > > /* > * The stride is either expressed as a multiple of 64 bytes > @@ -3125,7 +3125,7 @@ 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); > + fb->pixel_format, 0); > surf_addr = intel_plane_obj_offset(to_intel_plane(plane), obj, 0); > > /* > @@ -9104,7 +9104,7 @@ skylake_get_initial_plane_config(struct intel_crtc *crtc, > > val = I915_READ(PLANE_STRIDE(pipe, 0)); > stride_mult = intel_fb_stride_alignment(dev, fb->modifier[0], > - fb->pixel_format); > + fb->pixel_format, 0); > fb->pitches[0] = (val & 0x3ff) * stride_mult; > > aligned_height = intel_fb_align_height(dev, fb->height, > @@ -11175,7 +11175,7 @@ static void skl_do_mmio_flip(struct intel_crtc *intel_crtc) > */ > stride = fb->pitches[0] / > intel_fb_stride_alignment(dev, fb->modifier[0], > - fb->pixel_format); > + fb->pixel_format, 0); > > /* > * Both PLANE_CTL and PLANE_STRIDE are not updated on vblank but on > @@ -14241,6 +14241,7 @@ static int intel_framebuffer_init(struct drm_device *dev, > { > unsigned int aligned_height; > int ret; > + int i; > u32 pitch_limit, stride_alignment; > > WARN_ON(!mutex_is_locked(&dev->struct_mutex)); > @@ -14255,7 +14256,8 @@ static int intel_framebuffer_init(struct drm_device *dev, > } > } else { > if (obj->tiling_mode == I915_TILING_X) > - mode_cmd->modifier[0] = I915_FORMAT_MOD_X_TILED; > + for (i = 0; i < drm_format_num_planes(mode_cmd->pixel_format); i++) > + mode_cmd->modifier[i] = I915_FORMAT_MOD_X_TILED; The other branch needs updating too so that it will reject the operation if the modifier disagrees with the obj tiling mode. > else if (obj->tiling_mode == I915_TILING_Y) { > DRM_DEBUG("No Y tiling for legacy addfb\n"); > return -EINVAL; > @@ -14280,12 +14282,15 @@ static int intel_framebuffer_init(struct drm_device *dev, > return -EINVAL; > } > > - stride_alignment = intel_fb_stride_alignment(dev, mode_cmd->modifier[0], > - mode_cmd->pixel_format); > - if (mode_cmd->pitches[0] & (stride_alignment - 1)) { > - DRM_DEBUG("pitch (%d) must be at least %u byte aligned\n", > - mode_cmd->pitches[0], stride_alignment); > - return -EINVAL; > + /* check stride alignment for sub-planes */ > + for (i = 0; i < drm_format_num_planes(mode_cmd->pixel_format); i++) { > + stride_alignment = intel_fb_stride_alignment(dev, mode_cmd->modifier[i], > + mode_cmd->pixel_format, i); > + if (mode_cmd->pitches[i] & (stride_alignment - 1)) { > + DRM_DEBUG("subplane %d pitch (%d) must be at least %u bytes " > + "aligned\n", i, mode_cmd->pitches[i], stride_alignment); > + return -EINVAL; > + } > } > > pitch_limit = intel_fb_pitch_limit(dev, mode_cmd->modifier[0], > @@ -14352,9 +14357,44 @@ static int intel_framebuffer_init(struct drm_device *dev, > return -EINVAL; > } > break; > + case DRM_FORMAT_NV12: > + if (INTEL_INFO(dev)->gen < 9) { > + DRM_DEBUG("unsupported pixel format: %s\n", > + drm_get_format_name(mode_cmd->pixel_format)); > + return -EINVAL; > + } > + if (!mode_cmd->offsets[1]) { > + DRM_DEBUG("uv start offset not set\n"); > + return -EINVAL; > + } > + if (mode_cmd->pitches[0] != mode_cmd->pitches[1]) { > + DRM_DEBUG("y and uv subplanes have different pitches\n"); > + return -EINVAL; > + } > + if (mode_cmd->handles[0] != mode_cmd->handles[1]) { > + DRM_DEBUG("y and uv subplanes have different handles\n"); > + return -EINVAL; > + } > + if (mode_cmd->modifier[0] != mode_cmd->modifier[1]) { > + DRM_DEBUG("y and uv subplanes have different modifiers\n"); > + return -EINVAL; > + } > + if (mode_cmd->modifier[1] == I915_FORMAT_MOD_Yf_TILED && > + (mode_cmd->offsets[1] & 0xFFF)) { I've been trying to solicit ideas on how we should define the offsets[]; raw byte offset, or linear offset. I didn't get many opinions yet. So we need to figure it out and document it somewhere before we expose it to the world. In the meantime we could just reject non tile row aligned offsets regardless of the tiling mode. > + DRM_DEBUG("tile-Yf uv offset 0x%x isn't starting on new tile-row\n", > + mode_cmd->offsets[1]); > + return -EINVAL; > + } > + if (mode_cmd->modifier[1] == I915_FORMAT_MOD_Y_TILED && > + (((mode_cmd->offsets[1] & 0xFFF) / mode_cmd->pitches[1]) % 4)) { > + DRM_DEBUG("tile-Y uv offset 0x%x isn't 4-line aligned\n", > + mode_cmd->offsets[1]); > + return -EINVAL; > + } > + break; > default: > DRM_DEBUG("unsupported pixel format: %s\n", > - drm_get_format_name(mode_cmd->pixel_format)); > + drm_get_format_name(mode_cmd->pixel_format)); > return -EINVAL; > } > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index d50b8cb..62d2a11 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -980,7 +980,7 @@ unsigned int intel_fb_align_height(struct drm_device *dev, > void intel_fb_obj_flush(struct drm_i915_gem_object *obj, bool retire, > enum fb_op_origin origin); > u32 intel_fb_stride_alignment(struct drm_device *dev, uint64_t fb_modifier, > - uint32_t pixel_format); > + uint32_t pixel_format, int plane); > > /* intel_audio.c */ > void intel_init_audio(struct drm_device *dev); > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c > index 797594e..49feae0 100644 > --- a/drivers/gpu/drm/i915/intel_sprite.c > +++ b/drivers/gpu/drm/i915/intel_sprite.c > @@ -203,7 +203,7 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc, > src_w != crtc_w || src_h != crtc_h); > > stride_div = intel_fb_stride_alignment(dev, fb->modifier[0], > - fb->pixel_format); > + fb->pixel_format, 0); > > scaler_id = to_intel_plane_state(drm_plane->state)->scaler_id; > > -- > 1.7.9.5 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > @@ -14241,6 +14241,7 @@ static int intel_framebuffer_init(struct > drm_device *dev, > > { > > unsigned int aligned_height; > > int ret; > > + int i; > > u32 pitch_limit, stride_alignment; > > > > WARN_ON(!mutex_is_locked(&dev->struct_mutex)); > > @@ -14255,7 +14256,8 @@ static int intel_framebuffer_init(struct > drm_device *dev, > > } > > } else { > > if (obj->tiling_mode == I915_TILING_X) > > - mode_cmd->modifier[0] = > I915_FORMAT_MOD_X_TILED; > > + for (i = 0; i < drm_format_num_planes(mode_cmd- > >pixel_format); i++) > > + mode_cmd->modifier[i] = > I915_FORMAT_MOD_X_TILED; > > The other branch needs updating too so that it will reject the operation > if the modifier disagrees with the obj tiling mode. Is below something you meant? @@ -14223,10 +14223,12 @@ static int intel_framebuffer_init(struct drm_device *d if (mode_cmd->flags & DRM_MODE_FB_MODIFIERS) { /* Enforce that fb modifier and tiling mode match, but only for * X-tiled. This is needed for FBC. */ - if (!!(obj->tiling_mode == I915_TILING_X) != - !!(mode_cmd->modifier[0] == I915_FORMAT_MOD_X_TILED)) { - DRM_DEBUG("tiling_mode doesn't match fb modifier\n"); - return -EINVAL; + for (i = 0; i < drm_format_num_planes(mode_cmd->pixel_format); i + if (!!(obj->tiling_mode == I915_TILING_X) != + !!(mode_cmd->modifier[i] == I915_FORMAT_MOD_X_TILED)) { + DRM_DEBUG("tiling_mode doesn't match fb modifier + return -EINVAL; + } } } else { > > + if (mode_cmd->modifier[1] == I915_FORMAT_MOD_Yf_TILED > && > > + (mode_cmd->offsets[1] & 0xFFF)) { > > I've been trying to solicit ideas on how we should define the offsets[]; > raw byte offset, or linear offset. I didn't get many opinions yet. So we > need to figure it out and document it somewhere before we expose it to > the world. In the meantime we could just reject non tile row aligned > offsets regardless of the tiling mode. Above check is simply making sure tile Yf, uv offset starts on a new page. Is there any issue with above check?
On Wed, Sep 30, 2015 at 10:58:07PM +0000, Konduru, Chandra wrote: > > > @@ -14241,6 +14241,7 @@ static int intel_framebuffer_init(struct > > drm_device *dev, > > > { > > > unsigned int aligned_height; > > > int ret; > > > + int i; > > > u32 pitch_limit, stride_alignment; > > > > > > WARN_ON(!mutex_is_locked(&dev->struct_mutex)); > > > @@ -14255,7 +14256,8 @@ static int intel_framebuffer_init(struct > > drm_device *dev, > > > } > > > } else { > > > if (obj->tiling_mode == I915_TILING_X) > > > - mode_cmd->modifier[0] = > > I915_FORMAT_MOD_X_TILED; > > > + for (i = 0; i < drm_format_num_planes(mode_cmd- > > >pixel_format); i++) > > > + mode_cmd->modifier[i] = > > I915_FORMAT_MOD_X_TILED; > > > > The other branch needs updating too so that it will reject the operation > > if the modifier disagrees with the obj tiling mode. > > Is below something you meant? > > @@ -14223,10 +14223,12 @@ static int intel_framebuffer_init(struct drm_device *d > if (mode_cmd->flags & DRM_MODE_FB_MODIFIERS) { > /* Enforce that fb modifier and tiling mode match, but only for > * X-tiled. This is needed for FBC. */ > - if (!!(obj->tiling_mode == I915_TILING_X) != > - !!(mode_cmd->modifier[0] == I915_FORMAT_MOD_X_TILED)) { > - DRM_DEBUG("tiling_mode doesn't match fb modifier\n"); > - return -EINVAL; > + for (i = 0; i < drm_format_num_planes(mode_cmd->pixel_format); i > + if (!!(obj->tiling_mode == I915_TILING_X) != > + !!(mode_cmd->modifier[i] == I915_FORMAT_MOD_X_TILED)) { > + DRM_DEBUG("tiling_mode doesn't match fb modifier > + return -EINVAL; > + } > } Yep. > } else { > > > > + if (mode_cmd->modifier[1] == I915_FORMAT_MOD_Yf_TILED > > && > > > + (mode_cmd->offsets[1] & 0xFFF)) { > > > > I've been trying to solicit ideas on how we should define the offsets[]; > > raw byte offset, or linear offset. I didn't get many opinions yet. So we > > need to figure it out and document it somewhere before we expose it to > > the world. In the meantime we could just reject non tile row aligned > > offsets regardless of the tiling mode. > > Above check is simply making sure tile Yf, uv offset starts on a new page. > Is there any issue with above check? It won't necessarily be a page boundary if we interpret offsets[] as a linear offset. Eg. let's assume 4x4 tile size, stride=8, and offset=16. If interpret the offset as a linear offset we would land at 'x', but interpreted as a raw byte offset (not sure that's a good name, maybe untiled offset?) we'd land at 'y'. ----------- | |y | | | | |x | | | | | ----------- |z | | | | | | | | | | | ----------- If we had offset=32, then of course we would land at 'z' for both cases, which is why I suggested that if we haven't made up our mind about what offsets[] is, we could require it to be tile row aligned so that there would be no difference between the two interpretations.
On Thu, Oct 01, 2015 at 02:37:27PM +0300, Ville Syrjälä wrote: > On Wed, Sep 30, 2015 at 10:58:07PM +0000, Konduru, Chandra wrote: > > > > @@ -14241,6 +14241,7 @@ static int intel_framebuffer_init(struct > > > drm_device *dev, > > > > { > > > > unsigned int aligned_height; > > > > int ret; > > > > + int i; > > > > u32 pitch_limit, stride_alignment; > > > > > > > > WARN_ON(!mutex_is_locked(&dev->struct_mutex)); > > > > @@ -14255,7 +14256,8 @@ static int intel_framebuffer_init(struct > > > drm_device *dev, > > > > } > > > > } else { > > > > if (obj->tiling_mode == I915_TILING_X) > > > > - mode_cmd->modifier[0] = > > > I915_FORMAT_MOD_X_TILED; > > > > + for (i = 0; i < drm_format_num_planes(mode_cmd- > > > >pixel_format); i++) > > > > + mode_cmd->modifier[i] = > > > I915_FORMAT_MOD_X_TILED; > > > > > > The other branch needs updating too so that it will reject the operation > > > if the modifier disagrees with the obj tiling mode. > > > > Is below something you meant? > > > > @@ -14223,10 +14223,12 @@ static int intel_framebuffer_init(struct drm_device *d > > if (mode_cmd->flags & DRM_MODE_FB_MODIFIERS) { > > /* Enforce that fb modifier and tiling mode match, but only for > > * X-tiled. This is needed for FBC. */ > > - if (!!(obj->tiling_mode == I915_TILING_X) != > > - !!(mode_cmd->modifier[0] == I915_FORMAT_MOD_X_TILED)) { > > - DRM_DEBUG("tiling_mode doesn't match fb modifier\n"); > > - return -EINVAL; > > + for (i = 0; i < drm_format_num_planes(mode_cmd->pixel_format); i > > + if (!!(obj->tiling_mode == I915_TILING_X) != > > + !!(mode_cmd->modifier[i] == I915_FORMAT_MOD_X_TILED)) { > > + DRM_DEBUG("tiling_mode doesn't match fb modifier > > + return -EINVAL; > > + } > > } > > Yep. > > > } else { > > > > > > + if (mode_cmd->modifier[1] == I915_FORMAT_MOD_Yf_TILED > > > && > > > > + (mode_cmd->offsets[1] & 0xFFF)) { > > > > > > I've been trying to solicit ideas on how we should define the offsets[]; > > > raw byte offset, or linear offset. I didn't get many opinions yet. So we > > > need to figure it out and document it somewhere before we expose it to > > > the world. In the meantime we could just reject non tile row aligned > > > offsets regardless of the tiling mode. > > > > Above check is simply making sure tile Yf, uv offset starts on a new page. > > Is there any issue with above check? > > It won't necessarily be a page boundary if we interpret offsets[] as a linear > offset. > > Eg. let's assume 4x4 tile size, stride=8, and offset=16. If interpret > the offset as a linear offset we would land at 'x', but interpreted as > a raw byte offset (not sure that's a good name, maybe untiled offset?) > we'd land at 'y'. > > ----------- > | |y | > | | | > |x | | > | | | > ----------- > |z | | > | | | > | | | > | | | > ----------- > > If we had offset=32, then of course we would land at 'z' for both cases, > which is why I suggested that if we haven't made up our mind about what > offsets[] is, we could require it to be tile row aligned so that there > would be no difference between the two interpretations. Oh and I just figured out that linear offset would probably be better because that also isolates us from having to think about the internal tile layout, as in which way the bytes/owords/whatver are walked within the tile.
> -----Original Message----- > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] > Sent: Thursday, October 01, 2015 4:41 AM > To: Konduru, Chandra > Cc: intel-gfx@lists.freedesktop.org; Vetter, Daniel; Syrjala, Ville > Subject: Re: [Intel-gfx] [PATCH 09/15] drm/i915: Add NV12 support to > intel_framebuffer_init > > On Thu, Oct 01, 2015 at 02:37:27PM +0300, Ville Syrjälä wrote: > > On Wed, Sep 30, 2015 at 10:58:07PM +0000, Konduru, Chandra wrote: > > > > > @@ -14241,6 +14241,7 @@ static int intel_framebuffer_init(struct > > > > drm_device *dev, > > > > > { > > > > > unsigned int aligned_height; > > > > > int ret; > > > > > + int i; > > > > > u32 pitch_limit, stride_alignment; > > > > > > > > > > WARN_ON(!mutex_is_locked(&dev->struct_mutex)); > > > > > @@ -14255,7 +14256,8 @@ static int intel_framebuffer_init(struct > > > > drm_device *dev, > > > > > } > > > > > } else { > > > > > if (obj->tiling_mode == I915_TILING_X) > > > > > - mode_cmd->modifier[0] = > > > > I915_FORMAT_MOD_X_TILED; > > > > > + for (i = 0; i < > drm_format_num_planes(mode_cmd- > > > > >pixel_format); i++) > > > > > + mode_cmd->modifier[i] = > > > > I915_FORMAT_MOD_X_TILED; > > > > > > > > The other branch needs updating too so that it will reject the operation > > > > if the modifier disagrees with the obj tiling mode. > > > > > > Is below something you meant? > > > > > > @@ -14223,10 +14223,12 @@ static int intel_framebuffer_init(struct > drm_device *d > > > if (mode_cmd->flags & DRM_MODE_FB_MODIFIERS) { > > > /* Enforce that fb modifier and tiling mode match, but only for > > > * X-tiled. This is needed for FBC. */ > > > - if (!!(obj->tiling_mode == I915_TILING_X) != > > > - !!(mode_cmd->modifier[0] == I915_FORMAT_MOD_X_TILED)) { > > > - DRM_DEBUG("tiling_mode doesn't match fb modifier\n"); > > > - return -EINVAL; > > > + for (i = 0; i < drm_format_num_planes(mode_cmd->pixel_format); i > > > + if (!!(obj->tiling_mode == I915_TILING_X) != > > > + !!(mode_cmd->modifier[i] == I915_FORMAT_MOD_X_TILED)) { > > > + DRM_DEBUG("tiling_mode doesn't match fb modifier > > > + return -EINVAL; > > > + } > > > } > > > > Yep. > > > > > } else { > > > > > > > > + if (mode_cmd->modifier[1] == > I915_FORMAT_MOD_Yf_TILED > > > > && > > > > > + (mode_cmd->offsets[1] & 0xFFF)) { > > > > > > > > I've been trying to solicit ideas on how we should define the offsets[]; > > > > raw byte offset, or linear offset. I didn't get many opinions yet. So we > > > > need to figure it out and document it somewhere before we expose it to > > > > the world. In the meantime we could just reject non tile row aligned > > > > offsets regardless of the tiling mode. > > > > > > Above check is simply making sure tile Yf, uv offset starts on a new page. > > > Is there any issue with above check? > > > > It won't necessarily be a page boundary if we interpret offsets[] as a linear > > offset. > > > > Eg. let's assume 4x4 tile size, stride=8, and offset=16. If interpret > > the offset as a linear offset we would land at 'x', but interpreted as > > a raw byte offset (not sure that's a good name, maybe untiled offset?) > > we'd land at 'y'. > > > > ----------- > > | |y | > > | | | > > |x | | > > | | | > > ----------- > > |z | | > > | | | > > | | | > > | | | > > ----------- > > > > If we had offset=32, then of course we would land at 'z' for both cases, > > which is why I suggested that if we haven't made up our mind about what > > offsets[] is, we could require it to be tile row aligned so that there > > would be no difference between the two interpretations. > > Oh and I just figured out that linear offset would probably be better > because that also isolates us from having to think about the internal > tile layout, as in which way the bytes/owords/whatver are walked within > the tile. What is the issue with current offsets the patch is using? Also I think linear offset doesn't work for hw (it is actually confusing to say linear offset for a tiling format to start with). Requires a conversion to tiling format which requires i915 addfb logic to know tiling limitations/restrictions from media and/or other engines. Userlands know where the UV start is for every tiling format if they are using the tiling format and giving the UV offset to i915. If someone didn't set offset right, above check is helping to point that out.
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 2a5170e..af28ca9 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -2914,9 +2914,9 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc, } u32 intel_fb_stride_alignment(struct drm_device *dev, uint64_t fb_modifier, - uint32_t pixel_format) + uint32_t pixel_format, int plane) { - u32 bits_per_pixel = drm_format_plane_cpp(pixel_format, 0) * 8; + u32 bits_per_pixel = drm_format_plane_cpp(pixel_format, plane) * 8; /* * The stride is either expressed as a multiple of 64 bytes @@ -3125,7 +3125,7 @@ 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); + fb->pixel_format, 0); surf_addr = intel_plane_obj_offset(to_intel_plane(plane), obj, 0); /* @@ -9104,7 +9104,7 @@ skylake_get_initial_plane_config(struct intel_crtc *crtc, val = I915_READ(PLANE_STRIDE(pipe, 0)); stride_mult = intel_fb_stride_alignment(dev, fb->modifier[0], - fb->pixel_format); + fb->pixel_format, 0); fb->pitches[0] = (val & 0x3ff) * stride_mult; aligned_height = intel_fb_align_height(dev, fb->height, @@ -11175,7 +11175,7 @@ static void skl_do_mmio_flip(struct intel_crtc *intel_crtc) */ stride = fb->pitches[0] / intel_fb_stride_alignment(dev, fb->modifier[0], - fb->pixel_format); + fb->pixel_format, 0); /* * Both PLANE_CTL and PLANE_STRIDE are not updated on vblank but on @@ -14241,6 +14241,7 @@ static int intel_framebuffer_init(struct drm_device *dev, { unsigned int aligned_height; int ret; + int i; u32 pitch_limit, stride_alignment; WARN_ON(!mutex_is_locked(&dev->struct_mutex)); @@ -14255,7 +14256,8 @@ static int intel_framebuffer_init(struct drm_device *dev, } } else { if (obj->tiling_mode == I915_TILING_X) - mode_cmd->modifier[0] = I915_FORMAT_MOD_X_TILED; + for (i = 0; i < drm_format_num_planes(mode_cmd->pixel_format); i++) + mode_cmd->modifier[i] = I915_FORMAT_MOD_X_TILED; else if (obj->tiling_mode == I915_TILING_Y) { DRM_DEBUG("No Y tiling for legacy addfb\n"); return -EINVAL; @@ -14280,12 +14282,15 @@ static int intel_framebuffer_init(struct drm_device *dev, return -EINVAL; } - stride_alignment = intel_fb_stride_alignment(dev, mode_cmd->modifier[0], - mode_cmd->pixel_format); - if (mode_cmd->pitches[0] & (stride_alignment - 1)) { - DRM_DEBUG("pitch (%d) must be at least %u byte aligned\n", - mode_cmd->pitches[0], stride_alignment); - return -EINVAL; + /* check stride alignment for sub-planes */ + for (i = 0; i < drm_format_num_planes(mode_cmd->pixel_format); i++) { + stride_alignment = intel_fb_stride_alignment(dev, mode_cmd->modifier[i], + mode_cmd->pixel_format, i); + if (mode_cmd->pitches[i] & (stride_alignment - 1)) { + DRM_DEBUG("subplane %d pitch (%d) must be at least %u bytes " + "aligned\n", i, mode_cmd->pitches[i], stride_alignment); + return -EINVAL; + } } pitch_limit = intel_fb_pitch_limit(dev, mode_cmd->modifier[0], @@ -14352,9 +14357,44 @@ static int intel_framebuffer_init(struct drm_device *dev, return -EINVAL; } break; + case DRM_FORMAT_NV12: + if (INTEL_INFO(dev)->gen < 9) { + DRM_DEBUG("unsupported pixel format: %s\n", + drm_get_format_name(mode_cmd->pixel_format)); + return -EINVAL; + } + if (!mode_cmd->offsets[1]) { + DRM_DEBUG("uv start offset not set\n"); + return -EINVAL; + } + if (mode_cmd->pitches[0] != mode_cmd->pitches[1]) { + DRM_DEBUG("y and uv subplanes have different pitches\n"); + return -EINVAL; + } + if (mode_cmd->handles[0] != mode_cmd->handles[1]) { + DRM_DEBUG("y and uv subplanes have different handles\n"); + return -EINVAL; + } + if (mode_cmd->modifier[0] != mode_cmd->modifier[1]) { + DRM_DEBUG("y and uv subplanes have different modifiers\n"); + return -EINVAL; + } + if (mode_cmd->modifier[1] == I915_FORMAT_MOD_Yf_TILED && + (mode_cmd->offsets[1] & 0xFFF)) { + DRM_DEBUG("tile-Yf uv offset 0x%x isn't starting on new tile-row\n", + mode_cmd->offsets[1]); + return -EINVAL; + } + if (mode_cmd->modifier[1] == I915_FORMAT_MOD_Y_TILED && + (((mode_cmd->offsets[1] & 0xFFF) / mode_cmd->pitches[1]) % 4)) { + DRM_DEBUG("tile-Y uv offset 0x%x isn't 4-line aligned\n", + mode_cmd->offsets[1]); + return -EINVAL; + } + break; default: DRM_DEBUG("unsupported pixel format: %s\n", - drm_get_format_name(mode_cmd->pixel_format)); + drm_get_format_name(mode_cmd->pixel_format)); return -EINVAL; } diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index d50b8cb..62d2a11 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -980,7 +980,7 @@ unsigned int intel_fb_align_height(struct drm_device *dev, void intel_fb_obj_flush(struct drm_i915_gem_object *obj, bool retire, enum fb_op_origin origin); u32 intel_fb_stride_alignment(struct drm_device *dev, uint64_t fb_modifier, - uint32_t pixel_format); + uint32_t pixel_format, int plane); /* intel_audio.c */ void intel_init_audio(struct drm_device *dev); diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c index 797594e..49feae0 100644 --- a/drivers/gpu/drm/i915/intel_sprite.c +++ b/drivers/gpu/drm/i915/intel_sprite.c @@ -203,7 +203,7 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc, src_w != crtc_w || src_h != crtc_h); stride_div = intel_fb_stride_alignment(dev, fb->modifier[0], - fb->pixel_format); + fb->pixel_format, 0); scaler_id = to_intel_plane_state(drm_plane->state)->scaler_id;
This patch adds NV12 as supported format to intel_framebuffer_init and performs various checks. v2: -Fix an issue in checks added (me) v3: -cosmetic update, split checks into two (Ville) v4: -Add stride alignment and modifier checks for UV subplane (Ville) v5: -Make modifier check general (Ville) -Check tile-y uv start alignment from begining of page (Ville) Signed-off-by: Chandra Konduru <chandra.konduru@intel.com> Testcase: igt/kms_nv12 --- drivers/gpu/drm/i915/intel_display.c | 66 +++++++++++++++++++++++++++------- drivers/gpu/drm/i915/intel_drv.h | 2 +- drivers/gpu/drm/i915/intel_sprite.c | 2 +- 3 files changed, 55 insertions(+), 15 deletions(-)