[1/3] drm/i915: Fix command parser to validate multiple register access with the same command.

Commit Message

Francisco Jerez June 15, 2015, 11:18 a.m. UTC

Daniel Vetter <[e-mail]> writes:

> On Tue, Jun 02, 2015 at 05:36:26PM +0800, Zhigang Gong wrote:
>> The patchset LGTM and works well with beignet. The 80%+ performance regression issue in darktable also has been fixed
>> after this patchset applied and enable the atomic in L3 at beignet side. So,
>> 
>> Reviewed-by: Zhigang Gong <[e-mail]>
>
> All three merged.

Thanks Daniel.

> Aside: Dont we need an increment for the cmd parser version for
> userspace to be able to detect this?
>
Yeah, that would be a good idea, patch attached.

> And please follow up with a link to the beignet patches used to validate
> these kernel patches for references.
>
Zhigang, do you have a link to your Beignet patch?

> Thanks, Daniel
>
>> 
>> Thanks,
>> Zhigang Gong.
>>

Comments

Ville Syrjälä June 15, 2015, 11:26 a.m. UTC | #1

On Mon, Jun 15, 2015 at 02:18:01PM +0300, Francisco Jerez wrote:
> Daniel Vetter <[e-mail]> writes:
> 
> > On Tue, Jun 02, 2015 at 05:36:26PM +0800, Zhigang Gong wrote:
> >> The patchset LGTM and works well with beignet. The 80%+ performance regression issue in darktable also has been fixed
> >> after this patchset applied and enable the atomic in L3 at beignet side. So,
> >> 
> >> Reviewed-by: Zhigang Gong <[e-mail]>
> >
> > All three merged.
> 
> Thanks Daniel.
> 
> > Aside: Dont we need an increment for the cmd parser version for
> > userspace to be able to detect this?
> >
> Yeah, that would be a good idea, patch attached.

The old version alloweed userspace to write basically any register, the
new version allows only the whitelisted registers. I don't see how a
version number bump would help anyone.

> 
> > And please follow up with a link to the beignet patches used to validate
> > these kernel patches for references.
> >
> Zhigang, do you have a link to your Beignet patch?
> 
> > Thanks, Daniel
> >
> >> 
> >> Thanks,
> >> Zhigang Gong.
> >> 

> From 9f26beaf96473800252db35c4513933ae43e3c84 Mon Sep 17 00:00:00 2001
> From: Francisco Jerez <[e-mail]>
> Date: Mon, 15 Jun 2015 14:03:29 +0300
> Subject: [PATCH] drm/i915: Bump command parser version number.
> 
> Signed-off-by: Francisco Jerez <[e-mail]>
> ---
>  drivers/gpu/drm/i915/i915_cmd_parser.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c
> index 0146fe6..6722098 100644
> --- a/drivers/gpu/drm/i915/i915_cmd_parser.c
> +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
> @@ -1219,6 +1219,7 @@ int i915_cmd_parser_get_version(void)
>  	 * 2. Allow access to the MI_PREDICATE_SRC0 and
>  	 *    MI_PREDICATE_SRC1 registers.
>  	 * 3. Allow access to the GPGPU_THREADS_DISPATCHED register.
> +	 * 4. L3 atomic chicken bits of HSW_SCRATCH1 and HSW_ROW_CHICKEN3.
>  	 */
> -	return 3;
> +	return 4;
>  }
> -- 
> 2.4.3
> 




> _______________________________________________
> Intel-gfx mailing list
> [e-mail]
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Daniel Vetter June 15, 2015, 11:40 a.m. UTC | #2

On Mon, Jun 15, 2015 at 02:26:22PM +0300, Ville Syrjälä wrote:
> On Mon, Jun 15, 2015 at 02:18:01PM +0300, Francisco Jerez wrote:
> > Daniel Vetter <[e-mail]> writes:
> > 
> > > On Tue, Jun 02, 2015 at 05:36:26PM +0800, Zhigang Gong wrote:
> > >> The patchset LGTM and works well with beignet. The 80%+ performance regression issue in darktable also has been fixed
> > >> after this patchset applied and enable the atomic in L3 at beignet side. So,
> > >> 
> > >> Reviewed-by: Zhigang Gong <[e-mail]>
> > >
> > > All three merged.
> > 
> > Thanks Daniel.
> > 
> > > Aside: Dont we need an increment for the cmd parser version for
> > > userspace to be able to detect this?
> > >
> > Yeah, that would be a good idea, patch attached.
> 
> The old version alloweed userspace to write basically any register, the
> new version allows only the whitelisted registers. I don't see how a
> version number bump would help anyone.

Oops, totally missed the context of patch 1. Jani I think that one's for
you too ...

Thanks for pointing this out.
-Daniel

Francisco Jerez June 15, 2015, 12:05 p.m. UTC | #3

Daniel Vetter <[e-mail]> writes:

> On Mon, Jun 15, 2015 at 02:26:22PM +0300, Ville Syrjälä wrote:
>> On Mon, Jun 15, 2015 at 02:18:01PM +0300, Francisco Jerez wrote:
>> > Daniel Vetter <[e-mail]> writes:
>> > 
>> > > On Tue, Jun 02, 2015 at 05:36:26PM +0800, Zhigang Gong wrote:
>> > >> The patchset LGTM and works well with beignet. The 80%+ performance regression issue in darktable also has been fixed
>> > >> after this patchset applied and enable the atomic in L3 at beignet side. So,
>> > >> 
>> > >> Reviewed-by: Zhigang Gong <[e-mail]>
>> > >
>> > > All three merged.
>> > 
>> > Thanks Daniel.
>> > 
>> > > Aside: Dont we need an increment for the cmd parser version for
>> > > userspace to be able to detect this?
>> > >
>> > Yeah, that would be a good idea, patch attached.
>> 
>> The old version alloweed userspace to write basically any register, the
>> new version allows only the whitelisted registers. I don't see how a
>> version number bump would help anyone.
>
> Oops, totally missed the context of patch 1. Jani I think that one's for
> you too ...
>
IMHO the version bump is still useful for userspace to find out whether
it can use plain LRIs to write the L3 atomic chicken bits.  It's true
that as Ville said it would have been possible for userspace to write
the same bits before this series by building a batch specifically
crafted to cheat the command parser, but I don't think we want userspace
to rely on a command parser bug (e.g. because we may want to back-port
the fix to earlier kernel versions).

> Thanks for pointing this out.
> -Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

Jani Nikula June 15, 2015, 1:15 p.m. UTC | #4

On Mon, 15 Jun 2015, Daniel Vetter <[e-mail]> wrote:
> On Mon, Jun 15, 2015 at 02:26:22PM +0300, Ville Syrjälä wrote:
>> On Mon, Jun 15, 2015 at 02:18:01PM +0300, Francisco Jerez wrote:
>> > Daniel Vetter <[e-mail]> writes:
>> > 
>> > > On Tue, Jun 02, 2015 at 05:36:26PM +0800, Zhigang Gong wrote:
>> > >> The patchset LGTM and works well with beignet. The 80%+ performance regression issue in darktable also has been fixed
>> > >> after this patchset applied and enable the atomic in L3 at beignet side. So,
>> > >> 
>> > >> Reviewed-by: Zhigang Gong <[e-mail]>
>> > >
>> > > All three merged.
>> > 
>> > Thanks Daniel.
>> > 
>> > > Aside: Dont we need an increment for the cmd parser version for
>> > > userspace to be able to detect this?
>> > >
>> > Yeah, that would be a good idea, patch attached.
>> 
>> The old version alloweed userspace to write basically any register, the
>> new version allows only the whitelisted registers. I don't see how a
>> version number bump would help anyone.
>
> Oops, totally missed the context of patch 1. Jani I think that one's for
> you too ...

Cherry-picked to drm-intel-next-fixes.

BR,
Jani.


>
> Thanks for pointing this out.
> -Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

Francisco Jerez Aug. 30, 2015, 10:19 p.m. UTC | #5

Francisco Jerez <[e-mail]> writes:

> Daniel Vetter <[e-mail]> writes:
>
>> On Mon, Jun 15, 2015 at 02:26:22PM +0300, Ville Syrjälä wrote:
>>> On Mon, Jun 15, 2015 at 02:18:01PM +0300, Francisco Jerez wrote:
>>> > Daniel Vetter <[e-mail]> writes:
>>> > 
>>> > > On Tue, Jun 02, 2015 at 05:36:26PM +0800, Zhigang Gong wrote:
>>> > >> The patchset LGTM and works well with beignet. The 80%+ performance regression issue in darktable also has been fixed
>>> > >> after this patchset applied and enable the atomic in L3 at beignet side. So,
>>> > >> 
>>> > >> Reviewed-by: Zhigang Gong <[e-mail]>
>>> > >
>>> > > All three merged.
>>> > 
>>> > Thanks Daniel.
>>> > 
>>> > > Aside: Dont we need an increment for the cmd parser version for
>>> > > userspace to be able to detect this?
>>> > >
>>> > Yeah, that would be a good idea, patch attached.
>>> 
>>> The old version alloweed userspace to write basically any register, the
>>> new version allows only the whitelisted registers. I don't see how a
>>> version number bump would help anyone.
>>
>> Oops, totally missed the context of patch 1. Jani I think that one's for
>> you too ...
>>
> IMHO the version bump is still useful for userspace to find out whether
> it can use plain LRIs to write the L3 atomic chicken bits.  It's true
> that as Ville said it would have been possible for userspace to write
> the same bits before this series by building a batch specifically
> crafted to cheat the command parser, but I don't think we want userspace
> to rely on a command parser bug (e.g. because we may want to back-port
> the fix to earlier kernel versions).
>
Ping.  I cannot use these registers from userspace until the command
parser version number is bumped.

>> Thanks for pointing this out.
>> -Daniel
>> -- 
>> Daniel Vetter
>> Software Engineer, Intel Corporation
>> http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> [e-mail]
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Daniel Vetter Sept. 1, 2015, 9:41 a.m. UTC | #6

On Mon, Aug 31, 2015 at 01:19:47AM +0300, Francisco Jerez wrote:
> Francisco Jerez <[e-mail]> writes:
> 
> > Daniel Vetter <[e-mail]> writes:
> >
> >> On Mon, Jun 15, 2015 at 02:26:22PM +0300, Ville Syrjälä wrote:
> >>> On Mon, Jun 15, 2015 at 02:18:01PM +0300, Francisco Jerez wrote:
> >>> > Daniel Vetter <[e-mail]> writes:
> >>> > 
> >>> > > On Tue, Jun 02, 2015 at 05:36:26PM +0800, Zhigang Gong wrote:
> >>> > >> The patchset LGTM and works well with beignet. The 80%+ performance regression issue in darktable also has been fixed
> >>> > >> after this patchset applied and enable the atomic in L3 at beignet side. So,
> >>> > >> 
> >>> > >> Reviewed-by: Zhigang Gong <[e-mail]>
> >>> > >
> >>> > > All three merged.
> >>> > 
> >>> > Thanks Daniel.
> >>> > 
> >>> > > Aside: Dont we need an increment for the cmd parser version for
> >>> > > userspace to be able to detect this?
> >>> > >
> >>> > Yeah, that would be a good idea, patch attached.
> >>> 
> >>> The old version alloweed userspace to write basically any register, the
> >>> new version allows only the whitelisted registers. I don't see how a
> >>> version number bump would help anyone.
> >>
> >> Oops, totally missed the context of patch 1. Jani I think that one's for
> >> you too ...
> >>
> > IMHO the version bump is still useful for userspace to find out whether
> > it can use plain LRIs to write the L3 atomic chicken bits.  It's true
> > that as Ville said it would have been possible for userspace to write
> > the same bits before this series by building a batch specifically
> > crafted to cheat the command parser, but I don't think we want userspace
> > to rely on a command parser bug (e.g. because we may want to back-port
> > the fix to earlier kernel versions).
> >
> Ping.  I cannot use these registers from userspace until the command
> parser version number is bumped.

Queued for -next, thanks for the patch.
-Daniel

> 
> >> Thanks for pointing this out.
> >> -Daniel
> >> -- 
> >> Daniel Vetter
> >> Software Engineer, Intel Corporation
> >> http://blog.ffwll.ch
> > _______________________________________________
> > Intel-gfx mailing list
> > [e-mail]
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Patch

From 9f26beaf96473800252db35c4513933ae43e3c84 Mon Sep 17 00:00:00 2001
From: Francisco Jerez <[e-mail]>
Date: Mon, 15 Jun 2015 14:03:29 +0300
Subject: [PATCH] drm/i915: Bump command parser version number.

Signed-off-by: Francisco Jerez <[e-mail]>
---
 drivers/gpu/drm/i915/i915_cmd_parser.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c
index 0146fe6..6722098 100644
--- a/drivers/gpu/drm/i915/i915_cmd_parser.c
+++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
@@ -1219,6 +1219,7 @@  int i915_cmd_parser_get_version(void)
 	 * 2. Allow access to the MI_PREDICATE_SRC0 and
 	 *    MI_PREDICATE_SRC1 registers.
 	 * 3. Allow access to the GPGPU_THREADS_DISPATCHED register.
+	 * 4. L3 atomic chicken bits of HSW_SCRATCH1 and HSW_ROW_CHICKEN3.
 	 */
-	return 3;
+	return 4;
 }
-- 
2.4.3