Re: [Intel-gfx] [PATCH 2/2] drm/i915: Fail addfb ioctl if color and CCS buffers overlap

[Ville Syrjälä]

On Fri, Sep 22, 2017 at 02:25:19AM -0300, Gabriel Krisman Bertazi wrote:
> Ville Syrjälä  writes:
> 
> > On Tue, Sep 05, 2017 at 08:36:54AM +0100, Daniel Stone wrote:
> >> Hi Ville,
> >> 
> >> On 4 September 2017 at 17:37, Ville Syrjälä
> >>  wrote:
> >> > On Thu, Aug 31, 2017 at 04:52:15PM -0300, Gabriel Krisman Bertazi wrote:
> >> >> With this patch the new testcase igt@kms_ccs@pipe-X-invalid-ccs-offset
> >> >> succeeds.
> >> >
> >> > I don't think we actually want to reject overlap. I had a patch for that
> >> > years ago, but I decided to drop it because people might want to
> >> > interleave the planes in some interesting ways. Making the overlap
> >> > check accurate enough to allow that would be to total overkill. So IMO
> >> > it's perfectly fine to let the user shoot himself in the foot if they
> >> > mess up the offsets.
> >> 
> >> Is that actually supported by any hardware renderer? If not, maybe the
> >> check should only be enabled for generations who support it.
> >
> > Not sure I understand the question. You can program your offsets/strides
> > any which way you want,
> 
> Hi Ville,
> 
> Sorry for the delay.  Reviewing documentation at [1] I see the
> following, in page 177, regarding CCS structure:
> "The CCS is always placed after the main surface and is 4K page
> aligned".

The hardware doesn't really have such a requirement. The only thing it
cares about is that AUX_DIST must be positive which means the AUX
surface always starts >= plane SURF. That doesn't imply that the two
can't potentially overlap, and actually doesn't even imply that we
couldn't place the aux surface below the main surface (we can do that as
long as we are able to adjust the plane SURF and AUX_DIST appropriately).

But the main point is that the user can still ess up the AUX plane
offset in other ways, so trying to protect them against this one way
of messing up is rather pointless.

-- 
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/2] drm/i915: Fail addfb ioctl if color and CCS buffers overlap

[Gabriel Krisman Bertazi]

Ville Syrjälä  writes:

> On Tue, Sep 05, 2017 at 08:36:54AM +0100, Daniel Stone wrote:
>> Hi Ville,
>> 
>> On 4 September 2017 at 17:37, Ville Syrjälä
>>  wrote:
>> > On Thu, Aug 31, 2017 at 04:52:15PM -0300, Gabriel Krisman Bertazi wrote:
>> >> With this patch the new testcase igt@kms_ccs@pipe-X-invalid-ccs-offset
>> >> succeeds.
>> >
>> > I don't think we actually want to reject overlap. I had a patch for that
>> > years ago, but I decided to drop it because people might want to
>> > interleave the planes in some interesting ways. Making the overlap
>> > check accurate enough to allow that would be to total overkill. So IMO
>> > it's perfectly fine to let the user shoot himself in the foot if they
>> > mess up the offsets.
>> 
>> Is that actually supported by any hardware renderer? If not, maybe the
>> check should only be enabled for generations who support it.
>
> Not sure I understand the question. You can program your offsets/strides
> any which way you want,

Hi Ville,

Sorry for the delay.  Reviewing documentation at [1] I see the
following, in page 177, regarding CCS structure:
"The CCS is always placed after the main surface and is 4K page
aligned".  Sorry if I am missing something, but I believe this indicates
we can reject any overlapping between the main buffer and CCS. Can you
explain why not?

[1] 
https://01.org/sites/default/files/documentation/intel-gfx-prm-osrc-kbl-vol12-display.pdf

-- 
Gabriel Krisman Bertazi
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/2] drm/i915: Fail addfb ioctl if color and CCS buffers overlap

[Ville Syrjälä]

On Tue, Sep 05, 2017 at 08:36:54AM +0100, Daniel Stone wrote:
> Hi Ville,
> 
> On 4 September 2017 at 17:37, Ville Syrjälä
>  wrote:
> > On Thu, Aug 31, 2017 at 04:52:15PM -0300, Gabriel Krisman Bertazi wrote:
> >> With this patch the new testcase igt@kms_ccs@pipe-X-invalid-ccs-offset
> >> succeeds.
> >
> > I don't think we actually want to reject overlap. I had a patch for that
> > years ago, but I decided to drop it because people might want to
> > interleave the planes in some interesting ways. Making the overlap
> > check accurate enough to allow that would be to total overkill. So IMO
> > it's perfectly fine to let the user shoot himself in the foot if they
> > mess up the offsets.
> 
> Is that actually supported by any hardware renderer? If not, maybe the
> check should only be enabled for generations who support it.

Not sure I understand the question. You can program your offsets/strides
any which way you want,

-- 
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/2] drm/i915: Fail addfb ioctl if color and CCS buffers overlap

[Daniel Stone]

Hi Ville,

On 4 September 2017 at 17:37, Ville Syrjälä
 wrote:
> On Thu, Aug 31, 2017 at 04:52:15PM -0300, Gabriel Krisman Bertazi wrote:
>> With this patch the new testcase igt@kms_ccs@pipe-X-invalid-ccs-offset
>> succeeds.
>
> I don't think we actually want to reject overlap. I had a patch for that
> years ago, but I decided to drop it because people might want to
> interleave the planes in some interesting ways. Making the overlap
> check accurate enough to allow that would be to total overkill. So IMO
> it's perfectly fine to let the user shoot himself in the foot if they
> mess up the offsets.

Is that actually supported by any hardware renderer? If not, maybe the
check should only be enabled for generations who support it.

Cheers,
Daniel
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/2] drm/i915: Fail addfb ioctl if color and CCS buffers overlap

[Ville Syrjälä]

On Thu, Aug 31, 2017 at 04:52:15PM -0300, Gabriel Krisman Bertazi wrote:
> With this patch the new testcase igt@kms_ccs@pipe-X-invalid-ccs-offset
> succeeds.

I don't think we actually want to reject overlap. I had a patch for that
years ago, but I decided to drop it because people might want to
interleave the planes in some interesting ways. Making the overlap
check accurate enough to allow that would be to total overkill. So IMO
it's perfectly fine to let the user shoot himself in the foot if they
mess up the offsets.

> 
> Signed-off-by: Gabriel Krisman Bertazi 
> ---
>  drivers/gpu/drm/i915/intel_display.c | 5 +
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c 
> b/drivers/gpu/drm/i915/intel_display.c
> index b28f076f98bc..ff1ed67a9eff 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13989,6 +13989,11 @@ static int intel_framebuffer_init(struct 
> intel_framebuffer *intel_fb,
>   DRM_DEBUG_KMS("RC supported only with RGB 
> formats\n");
>   goto err;
>   }
> +
> + if (mode_cmd->offsets[1] < mode_cmd->pitches[0]) {
> + DRM_DEBUG_KMS("CCS and color buffers overlap\n");
> + return -EINVAL;
> + }
>   /* fall through */
>   case I915_FORMAT_MOD_Y_TILED:
>   case I915_FORMAT_MOD_Yf_TILED:
> -- 
> 2.11.0
> 
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/2] drm/i915: Fail addfb ioctl if color and CCS buffers overlap

[Daniel Vetter]

On Thu, Aug 31, 2017 at 04:52:15PM -0300, Gabriel Krisman Bertazi wrote:
> With this patch the new testcase igt@kms_ccs@pipe-X-invalid-ccs-offset
> succeeds.
> 
> Signed-off-by: Gabriel Krisman Bertazi 

Do we have igts for this? If so, please add a Testcase: line.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_display.c | 5 +
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c 
> b/drivers/gpu/drm/i915/intel_display.c
> index b28f076f98bc..ff1ed67a9eff 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13989,6 +13989,11 @@ static int intel_framebuffer_init(struct 
> intel_framebuffer *intel_fb,
>   DRM_DEBUG_KMS("RC supported only with RGB 
> formats\n");
>   goto err;
>   }
> +
> + if (mode_cmd->offsets[1] < mode_cmd->pitches[0]) {
> + DRM_DEBUG_KMS("CCS and color buffers overlap\n");
> + return -EINVAL;
> + }
>   /* fall through */
>   case I915_FORMAT_MOD_Y_TILED:
>   case I915_FORMAT_MOD_Yf_TILED:
> -- 
> 2.11.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/2] drm/i915: Fail addfb ioctl if color and CCS buffers overlap

[Gabriel Krisman Bertazi]

Ben Widawsky  writes:

> On 17-08-31 16:52:15, Gabriel Krisman Bertazi wrote:
>>With this patch the new testcase igt@kms_ccs@pipe-X-invalid-ccs-offset
>>succeeds.
>>
>>Signed-off-by: Gabriel Krisman Bertazi 
>>---
>> drivers/gpu/drm/i915/intel_display.c | 5 +
>> 1 file changed, 5 insertions(+)
>>
>>diff --git a/drivers/gpu/drm/i915/intel_display.c 
>>b/drivers/gpu/drm/i915/intel_display.c
>>index b28f076f98bc..ff1ed67a9eff 100644
>>--- a/drivers/gpu/drm/i915/intel_display.c
>>+++ b/drivers/gpu/drm/i915/intel_display.c
>>@@ -13989,6 +13989,11 @@ static int intel_framebuffer_init(struct 
>>intel_framebuffer *intel_fb,
>>  DRM_DEBUG_KMS("RC supported only with RGB 
>> formats\n");
>>  goto err;
>>  }
>>+
>>+ if (mode_cmd->offsets[1] < mode_cmd->pitches[0]) {
>>+ DRM_DEBUG_KMS("CCS and color buffers overlap\n");
>>+ return -EINVAL;
>>+ }
>
> This check doesn't look nearly strict enough to determine overlap. All it's
> checking is that the aux buffer isn't in the first row of the main buffer.
> Second of all, while today our requirement is that the aux buffer always come
> after the main buffer, that may not be the case forever.

Right.  Thanks for catching it.  I'll re-spin with a more strict (and
correct) test for overlapping.

-- 
Gabriel Krisman Bertazi
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/2] drm/i915: Fail addfb ioctl if color and CCS buffers overlap

[Ben Widawsky]

On 17-08-31 16:52:15, Gabriel Krisman Bertazi wrote:

With this patch the new testcase igt@kms_ccs@pipe-X-invalid-ccs-offset
succeeds.

Signed-off-by: Gabriel Krisman Bertazi 
---
drivers/gpu/drm/i915/intel_display.c | 5 +
1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index b28f076f98bc..ff1ed67a9eff 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13989,6 +13989,11 @@ static int intel_framebuffer_init(struct 
intel_framebuffer *intel_fb,
DRM_DEBUG_KMS("RC supported only with RGB 
formats\n");
goto err;
}
+
+   if (mode_cmd->offsets[1] < mode_cmd->pitches[0]) {
+   DRM_DEBUG_KMS("CCS and color buffers overlap\n");
+   return -EINVAL;
+   }


This check doesn't look nearly strict enough to determine overlap. All it's
checking is that the aux buffer isn't in the first row of the main buffer.
Second of all, while today our requirement is that the aux buffer always come
after the main buffer, that may not be the case forever.


/* fall through */
case I915_FORMAT_MOD_Y_TILED:
case I915_FORMAT_MOD_Yf_TILED:
--
2.11.0


___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 2/2] drm/i915: Fail addfb ioctl if color and CCS buffers overlap

[Gabriel Krisman Bertazi]

With this patch the new testcase igt@kms_ccs@pipe-X-invalid-ccs-offset
succeeds.

Signed-off-by: Gabriel Krisman Bertazi 
---
 drivers/gpu/drm/i915/intel_display.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index b28f076f98bc..ff1ed67a9eff 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13989,6 +13989,11 @@ static int intel_framebuffer_init(struct 
intel_framebuffer *intel_fb,
DRM_DEBUG_KMS("RC supported only with RGB 
formats\n");
goto err;
}
+
+   if (mode_cmd->offsets[1] < mode_cmd->pitches[0]) {
+   DRM_DEBUG_KMS("CCS and color buffers overlap\n");
+   return -EINVAL;
+   }
/* fall through */
case I915_FORMAT_MOD_Y_TILED:
case I915_FORMAT_MOD_Yf_TILED:
-- 
2.11.0

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx