[08/12] drm/i915: Add NV12 support to intel_framebuffer_init
diff mbox

Message ID 1431925865-7637-9-git-send-email-chandra.konduru@intel.com
State New
Headers show

Commit Message

Chandra Konduru May 18, 2015, 5:11 a.m. UTC
This patch adds NV12 as supported format to
intel_framebuffer_init and performs various checks.

Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
Testcase: igt/kms_nv12
---
 drivers/gpu/drm/i915/intel_display.c |   27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

Comments

Daniel Vetter May 19, 2015, 8:24 a.m. UTC | #1
On Sun, May 17, 2015 at 10:11:01PM -0700, Chandra Konduru wrote:
> This patch adds NV12 as supported format to
> intel_framebuffer_init and performs various checks.
> 
> Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
> Testcase: igt/kms_nv12
> ---
>  drivers/gpu/drm/i915/intel_display.c |   27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 42924a6..41cd26f 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -14043,6 +14043,33 @@ 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;
> +		}

Nope. It's perfectly ok to have NV12 with a 0 offset for the uv plane, if
it's e.g. in a separate buffer object. Which is the part this series seems
to be completely missing - there's no code at all to look up (and store in
intel_framebuffer the 2nd i915_bo pointer) the 2nd buffer handle afaics.

You should also change your igts to use 2 separate buffers, just for test
coverage.
-Daniel

> +		if (mode_cmd->pitches[0] != mode_cmd->pitches[1] ||
> +			mode_cmd->handles[0] != mode_cmd->handles[1]) {
> +			DRM_DEBUG("y and uv subplanes have different parameters\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] % mode_cmd->pitches[1]) % 4)) {
> +			DRM_DEBUG("tile-Y uv offset 0x%x isn't 4-line aligned\n",
> +				mode_cmd->offsets[1]);
> +		}
> +		break;
>  	default:
>  		DRM_DEBUG("unsupported pixel format: %s\n",
>  			  drm_get_format_name(mode_cmd->pixel_format));
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Chandra Konduru May 19, 2015, 4:31 p.m. UTC | #2
> -----Original Message-----
> From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter
> Sent: Tuesday, May 19, 2015 1:24 AM
> To: Konduru, Chandra
> Cc: intel-gfx@lists.freedesktop.org; Vetter, Daniel; Syrjala, Ville
> Subject: Re: [Intel-gfx] [PATCH 08/12] drm/i915: Add NV12 support to
> intel_framebuffer_init
> 
> On Sun, May 17, 2015 at 10:11:01PM -0700, Chandra Konduru wrote:
> > This patch adds NV12 as supported format to
> > intel_framebuffer_init and performs various checks.
> >
> > Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
> > Testcase: igt/kms_nv12
> > ---
> >  drivers/gpu/drm/i915/intel_display.c |   27 +++++++++++++++++++++++++++
> >  1 file changed, 27 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> > index 42924a6..41cd26f 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -14043,6 +14043,33 @@ 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;
> > +		}
> 
> Nope. It's perfectly ok to have NV12 with a 0 offset for the uv plane, if
> it's e.g. in a separate buffer object. Which is the part this series seems
> to be completely missing - there's no code at all to look up (and store in
> intel_framebuffer the 2nd i915_bo pointer) the 2nd buffer handle afaics.
> 
> You should also change your igts to use 2 separate buffers, just for test
> coverage.
> -Daniel

Hi Daniel,
Agree, in general that is very well ok. But as skl hw requires uv to be after 
y in gtt. This can be guaranteed by having a single bo and y and uv offsets 
into it. Above sanity checks in i915 specific fb init call are for that reason.
There are definitely ways to guarantee uv to be after y even with two 
separate bos (by uv remapping), but I see that is unnecessary 
complication and not sure value by allowing that. Or am I missing 
something here?

> 
> > +		if (mode_cmd->pitches[0] != mode_cmd->pitches[1] ||
> > +			mode_cmd->handles[0] != mode_cmd->handles[1]) {
> > +			DRM_DEBUG("y and uv subplanes have different
> parameters\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] % mode_cmd->pitches[1]) %
> 4)) {
> > +			DRM_DEBUG("tile-Y uv offset 0x%x isn't 4-line
> aligned\n",
> > +				mode_cmd->offsets[1]);
> > +		}
> > +		break;
> >  	default:
> >  		DRM_DEBUG("unsupported pixel format: %s\n",
> >  			  drm_get_format_name(mode_cmd->pixel_format));
> > --
> > 1.7.9.5
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
Daniel Vetter May 20, 2015, 7:36 a.m. UTC | #3
On Tue, May 19, 2015 at 04:31:54PM +0000, Konduru, Chandra wrote:
> 
> 
> > -----Original Message-----
> > From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter
> > Sent: Tuesday, May 19, 2015 1:24 AM
> > To: Konduru, Chandra
> > Cc: intel-gfx@lists.freedesktop.org; Vetter, Daniel; Syrjala, Ville
> > Subject: Re: [Intel-gfx] [PATCH 08/12] drm/i915: Add NV12 support to
> > intel_framebuffer_init
> > 
> > On Sun, May 17, 2015 at 10:11:01PM -0700, Chandra Konduru wrote:
> > > This patch adds NV12 as supported format to
> > > intel_framebuffer_init and performs various checks.
> > >
> > > Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
> > > Testcase: igt/kms_nv12
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c |   27 +++++++++++++++++++++++++++
> > >  1 file changed, 27 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > > index 42924a6..41cd26f 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -14043,6 +14043,33 @@ 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;
> > > +		}
> > 
> > Nope. It's perfectly ok to have NV12 with a 0 offset for the uv plane, if
> > it's e.g. in a separate buffer object. Which is the part this series seems
> > to be completely missing - there's no code at all to look up (and store in
> > intel_framebuffer the 2nd i915_bo pointer) the 2nd buffer handle afaics.
> > 
> > You should also change your igts to use 2 separate buffers, just for test
> > coverage.
> > -Daniel
> 
> Hi Daniel,
> Agree, in general that is very well ok. But as skl hw requires uv to be after 
> y in gtt. This can be guaranteed by having a single bo and y and uv offsets 
> into it. Above sanity checks in i915 specific fb init call are for that reason.
> There are definitely ways to guarantee uv to be after y even with two 
> separate bos (by uv remapping), but I see that is unnecessary 
> complication and not sure value by allowing that. Or am I missing 
> something here?

For a start you don't reject multiplane stuff where this isn't the case.
And if we indeed have the hw requirement that the gtt address for y must
be before the gtt address for uv (sounds strange to me, definitely need a
bspec reference for that) then we need to check that throughroughly:
Currently you could place Y after UV even in a single BO.

Also we need igts to make sure we catch userspace throwing invalid stuff
at us.
-Daniel
Chandra Konduru May 21, 2015, 12:31 a.m. UTC | #4
> -----Original Message-----
> From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter
> Sent: Wednesday, May 20, 2015 12:37 AM
> To: Konduru, Chandra
> Cc: Daniel Vetter; intel-gfx@lists.freedesktop.org; Vetter, Daniel; Syrjala, Ville
> Subject: Re: [Intel-gfx] [PATCH 08/12] drm/i915: Add NV12 support to
> intel_framebuffer_init
> 
> On Tue, May 19, 2015 at 04:31:54PM +0000, Konduru, Chandra wrote:
> >
> >
> > > -----Original Message-----
> > > From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel
> Vetter
> > > Sent: Tuesday, May 19, 2015 1:24 AM
> > > To: Konduru, Chandra
> > > Cc: intel-gfx@lists.freedesktop.org; Vetter, Daniel; Syrjala, Ville
> > > Subject: Re: [Intel-gfx] [PATCH 08/12] drm/i915: Add NV12 support to
> > > intel_framebuffer_init
> > >
> > > On Sun, May 17, 2015 at 10:11:01PM -0700, Chandra Konduru wrote:
> > > > This patch adds NV12 as supported format to
> > > > intel_framebuffer_init and performs various checks.
> > > >
> > > > Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
> > > > Testcase: igt/kms_nv12
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_display.c |   27
> +++++++++++++++++++++++++++
> > > >  1 file changed, 27 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > b/drivers/gpu/drm/i915/intel_display.c
> > > > index 42924a6..41cd26f 100644
> > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > @@ -14043,6 +14043,33 @@ 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;
> > > > +		}
> > >
> > > Nope. It's perfectly ok to have NV12 with a 0 offset for the uv plane, if
> > > it's e.g. in a separate buffer object. Which is the part this series seems
> > > to be completely missing - there's no code at all to look up (and store in
> > > intel_framebuffer the 2nd i915_bo pointer) the 2nd buffer handle afaics.
> > >
> > > You should also change your igts to use 2 separate buffers, just for test
> > > coverage.
> > > -Daniel
> >
> > Hi Daniel,
> > Agree, in general that is very well ok. But as skl hw requires uv to be after
> > y in gtt. This can be guaranteed by having a single bo and y and uv offsets
> > into it. Above sanity checks in i915 specific fb init call are for that reason.
> > There are definitely ways to guarantee uv to be after y even with two
> > separate bos (by uv remapping), but I see that is unnecessary
> > complication and not sure value by allowing that. Or am I missing
> > something here?
> 
> For a start you don't reject multiplane stuff where this isn't the case.
> And if we indeed have the hw requirement that the gtt address for y must
> be before the gtt address for uv (sounds strange to me, definitely need a
> bspec reference for that) then we need to check that throughroughly:
> Currently you could place Y after UV even in a single BO.

Hi Daniel,
NV12 programming is documented in bspec under display planes 
"Plane Planar YUV programming". There it talks about aux_dist
which is the distance between y and uv planes expecting uv to be
after y.

> 
> Also we need igts to make sure we catch userspace throwing invalid stuff
> at us.

Added a sub-test to kms_nv12 and will send out the updated igt patch.

> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
Daniel Vetter May 21, 2015, 9:33 a.m. UTC | #5
On Thu, May 21, 2015 at 12:31:49AM +0000, Konduru, Chandra wrote:
> 
> 
> > -----Original Message-----
> > From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter
> > Sent: Wednesday, May 20, 2015 12:37 AM
> > To: Konduru, Chandra
> > Cc: Daniel Vetter; intel-gfx@lists.freedesktop.org; Vetter, Daniel; Syrjala, Ville
> > Subject: Re: [Intel-gfx] [PATCH 08/12] drm/i915: Add NV12 support to
> > intel_framebuffer_init
> > 
> > On Tue, May 19, 2015 at 04:31:54PM +0000, Konduru, Chandra wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel
> > Vetter
> > > > Sent: Tuesday, May 19, 2015 1:24 AM
> > > > To: Konduru, Chandra
> > > > Cc: intel-gfx@lists.freedesktop.org; Vetter, Daniel; Syrjala, Ville
> > > > Subject: Re: [Intel-gfx] [PATCH 08/12] drm/i915: Add NV12 support to
> > > > intel_framebuffer_init
> > > >
> > > > On Sun, May 17, 2015 at 10:11:01PM -0700, Chandra Konduru wrote:
> > > > > This patch adds NV12 as supported format to
> > > > > intel_framebuffer_init and performs various checks.
> > > > >
> > > > > Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
> > > > > Testcase: igt/kms_nv12
> > > > > ---
> > > > >  drivers/gpu/drm/i915/intel_display.c |   27
> > +++++++++++++++++++++++++++
> > > > >  1 file changed, 27 insertions(+)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > > b/drivers/gpu/drm/i915/intel_display.c
> > > > > index 42924a6..41cd26f 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > > @@ -14043,6 +14043,33 @@ 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;
> > > > > +		}
> > > >
> > > > Nope. It's perfectly ok to have NV12 with a 0 offset for the uv plane, if
> > > > it's e.g. in a separate buffer object. Which is the part this series seems
> > > > to be completely missing - there's no code at all to look up (and store in
> > > > intel_framebuffer the 2nd i915_bo pointer) the 2nd buffer handle afaics.
> > > >
> > > > You should also change your igts to use 2 separate buffers, just for test
> > > > coverage.
> > > > -Daniel
> > >
> > > Hi Daniel,
> > > Agree, in general that is very well ok. But as skl hw requires uv to be after
> > > y in gtt. This can be guaranteed by having a single bo and y and uv offsets
> > > into it. Above sanity checks in i915 specific fb init call are for that reason.
> > > There are definitely ways to guarantee uv to be after y even with two
> > > separate bos (by uv remapping), but I see that is unnecessary
> > > complication and not sure value by allowing that. Or am I missing
> > > something here?
> > 
> > For a start you don't reject multiplane stuff where this isn't the case.
> > And if we indeed have the hw requirement that the gtt address for y must
> > be before the gtt address for uv (sounds strange to me, definitely need a
> > bspec reference for that) then we need to check that throughroughly:
> > Currently you could place Y after UV even in a single BO.
> 
> Hi Daniel,
> NV12 programming is documented in bspec under display planes 
> "Plane Planar YUV programming". There it talks about aux_dist
> which is the distance between y and uv planes expecting uv to be
> after y.

Bspec talks about wrap-around, which at least indicates that this might be
possible and the hw doesn't have a real restriction here. Art, can you
please double-check whether we could wrape-around PLANE_AUX_DIST with a
2s complement to avoid placement restrictions on the aux buffers?

Bspec doesn't say anything about this being required to be positive ...
-Daniel
Chandra Konduru May 21, 2015, 4:11 p.m. UTC | #6
> > > > > > This patch adds NV12 as supported format to
> > > > > > intel_framebuffer_init and performs various checks.
> > > > > >
> > > > > > Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
> > > > > > Testcase: igt/kms_nv12
> > > > > > ---
> > > > > >  drivers/gpu/drm/i915/intel_display.c |   27
> > > +++++++++++++++++++++++++++
> > > > > >  1 file changed, 27 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > > > b/drivers/gpu/drm/i915/intel_display.c
> > > > > > index 42924a6..41cd26f 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > > > @@ -14043,6 +14043,33 @@ 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;
> > > > > > +		}
> > > > >
> > > > > Nope. It's perfectly ok to have NV12 with a 0 offset for the uv
> > > > > plane, if it's e.g. in a separate buffer object. Which is the
> > > > > part this series seems to be completely missing - there's no
> > > > > code at all to look up (and store in intel_framebuffer the 2nd i915_bo
> pointer) the 2nd buffer handle afaics.
> > > > >
> > > > > You should also change your igts to use 2 separate buffers, just
> > > > > for test coverage.
> > > > > -Daniel
> > > >
> > > > Hi Daniel,
> > > > Agree, in general that is very well ok. But as skl hw requires uv
> > > > to be after y in gtt. This can be guaranteed by having a single bo
> > > > and y and uv offsets into it. Above sanity checks in i915 specific fb init call
> are for that reason.
> > > > There are definitely ways to guarantee uv to be after y even with
> > > > two separate bos (by uv remapping), but I see that is unnecessary
> > > > complication and not sure value by allowing that. Or am I missing
> > > > something here?
> > >
> > > For a start you don't reject multiplane stuff where this isn't the case.
> > > And if we indeed have the hw requirement that the gtt address for y
> > > must be before the gtt address for uv (sounds strange to me,
> > > definitely need a bspec reference for that) then we need to check that
> throughroughly:
> > > Currently you could place Y after UV even in a single BO.
> >
> > Hi Daniel,
> > NV12 programming is documented in bspec under display planes "Plane
> > Planar YUV programming". There it talks about aux_dist which is the
> > distance between y and uv planes expecting uv to be after y.
> 
> Bspec talks about wrap-around, which at least indicates that this might be
> possible and the hw doesn't have a real restriction here. Art, can you please
> double-check whether we could wrape-around PLANE_AUX_DIST with a 2s
> complement to avoid placement restrictions on the aux buffers?
> 
> Bspec doesn't say anything about this being required to be positive ...
> -Daniel

Yeah, it isn't explicit, so to be sure a while ago checked with hw team about
uv plane positioning relative to y-plane. It was confirmed that aux_dist to be 
positive. Certainly Art can double confirm for you.

> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
Runyan, Arthur J May 21, 2015, 5:34 p.m. UTC | #7
>From: Konduru, Chandra
>
>> >
>> > Hi Daniel,
>> > NV12 programming is documented in bspec under display planes "Plane
>> > Planar YUV programming". There it talks about aux_dist which is the
>> > distance between y and uv planes expecting uv to be after y.
>>
>> Bspec talks about wrap-around, which at least indicates that this might be
>> possible and the hw doesn't have a real restriction here. Art, can you please
>> double-check whether we could wrape-around PLANE_AUX_DIST with a 2s
>> complement to avoid placement restrictions on the aux buffers?
>>
>> Bspec doesn't say anything about this being required to be positive ...
>> -Daniel
>
>Yeah, it isn't explicit, so to be sure a while ago checked with hw team about
>uv plane positioning relative to y-plane. It was confirmed that aux_dist to be
>positive. Certainly Art can double confirm for you.
>

I'll check on it.
Runyan, Arthur J May 22, 2015, 7:06 p.m. UTC | #8
>From: Konduru, Chandra
........
>> >
>> > Hi Daniel,
>> > NV12 programming is documented in bspec under display planes "Plane
>> > Planar YUV programming". There it talks about aux_dist which is the
>> > distance between y and uv planes expecting uv to be after y.
>>
>> Bspec talks about wrap-around, which at least indicates that this might be
>> possible and the hw doesn't have a real restriction here. Art, can you please
>> double-check whether we could wrape-around PLANE_AUX_DIST with a 2s
>> complement to avoid placement restrictions on the aux buffers?
>>
>> Bspec doesn't say anything about this being required to be positive ...
>> -Daniel
>
>Yeah, it isn't explicit, so to be sure a while ago checked with hw team about
>uv plane positioning relative to y-plane. It was confirmed that aux_dist to be
>positive. Certainly Art can double confirm for you.
>
Positive is correct.  We'll update the bspec to make it clear.

Patch
diff mbox

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 42924a6..41cd26f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14043,6 +14043,33 @@  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] ||
+			mode_cmd->handles[0] != mode_cmd->handles[1]) {
+			DRM_DEBUG("y and uv subplanes have different parameters\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] % mode_cmd->pitches[1]) % 4)) {
+			DRM_DEBUG("tile-Y uv offset 0x%x isn't 4-line aligned\n",
+				mode_cmd->offsets[1]);
+		}
+		break;
 	default:
 		DRM_DEBUG("unsupported pixel format: %s\n",
 			  drm_get_format_name(mode_cmd->pixel_format));