| Message ID | 20150728111419.38b36358@pc1 (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jul 28, 2015 at 11:14:19AM -0700, Hanno Böck wrote: > Hi, > > On Tue, 28 Jul 2015 10:14:51 +0200 > Daniel Vetter <daniel@ffwll.ch> wrote: > > > > Indeed, nice catch. Could you please read > > > Documentation/SubmittingPatches and apply your Signed-off-by and > > > then we can accept this patch under your authorship. > > > > > > Preferrably this is two patches, (a) fix the tables, (b) fix the > > > validator. That way we can delay enabling the validator if we need > > > to fix the tables for others. > > I think I have checked all tables, not just the ones used on my gpu, > they should be fine. But I've splittet the patch. > > > Also can you please add signed-off-by lines to your patch when > > resubmitting? See Documentation/SubmittingPatches for all the details. > > The patch already had a "Signed-off-by" line. > > The checkpatch script complains that it doesn't like the formatting of > the CMD command. However I won't change that in this patch, as this is > how the whole file is formatted. If this is wanted I can submit a patch > changing the formatting afterwards, but I think this is an unrelated > change. > > Please apply. > > -- > Hanno Böck > http://hboeck.de/ > > mail/jabber: hanno@hboeck.de > GPG: BBB51E42 > Properly sort cmd tables. drm/i915: Properly sort MI coomand table In the future, we may want to speed up command/register searching using a bisection and so we require them to be in ascending order respectively by command value or register address. However, this was not true for one pair in the MI table; make it so. > Signed-off-by: Hanno Boeck <hanno@hboeck.de> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> > diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c > index 306d9e4..5fdd8c8 100644 > --- a/drivers/gpu/drm/i915/i915_cmd_parser.c > +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c > @@ -151,8 +151,8 @@ static const struct drm_i915_cmd_descriptor render_cmds[] = { > CMD( MI_ARB_ON_OFF, SMI, F, 1, R ), > CMD( MI_PREDICATE, SMI, F, 1, S ), > CMD( MI_TOPOLOGY_FILTER, SMI, F, 1, S ), > - CMD( MI_DISPLAY_FLIP, SMI, !F, 0xFF, R ), > CMD( MI_SET_APPID, SMI, F, 1, S ), > + CMD( MI_DISPLAY_FLIP, SMI, !F, 0xFF, R ), > CMD( MI_SET_CONTEXT, SMI, !F, 0xFF, R ), > CMD( MI_URB_CLEAR, SMI, !F, 0xFF, S ), > CMD( MI_STORE_DWORD_IMM, SMI, !F, 0x3F, B, > Fix loop checking cmd tables. drm/i915: Fix command parser table validator As we may like to use a bisection search on the tables in future, we need them to be ordered. For convenience we expect the compiled tables to be order and check on initialisation. However, the validator used the wrong iterators failed to spot the misordered MI tables and instead walked off into the unknown (as spotted by kasan). > Signed-off-by: Hanno Boeck <hanno@hboeck.de> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> > diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c > index 306d9e4..3a53bf3 100644 > --- a/drivers/gpu/drm/i915/i915_cmd_parser.c > +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c > @@ -564,7 +564,7 @@ static bool validate_cmds_sorted(struct intel_engine_cs *ring, > > for (j = 0; j < table->count; j++) { > const struct drm_i915_cmd_descriptor *desc = > - &table->table[i]; > + &table->table[j]; > u32 curr = desc->cmd.value & desc->cmd.mask; > > if (curr < previous) {
On Tue, Jul 28, 2015 at 08:36:18PM +0100, Chris Wilson wrote: > On Tue, Jul 28, 2015 at 11:14:19AM -0700, Hanno Böck wrote: > > Hi, > > > > On Tue, 28 Jul 2015 10:14:51 +0200 > > Daniel Vetter <daniel@ffwll.ch> wrote: > > > > > > Indeed, nice catch. Could you please read > > > > Documentation/SubmittingPatches and apply your Signed-off-by and > > > > then we can accept this patch under your authorship. > > > > > > > > Preferrably this is two patches, (a) fix the tables, (b) fix the > > > > validator. That way we can delay enabling the validator if we need > > > > to fix the tables for others. > > > > I think I have checked all tables, not just the ones used on my gpu, > > they should be fine. But I've splittet the patch. > > > > > Also can you please add signed-off-by lines to your patch when > > > resubmitting? See Documentation/SubmittingPatches for all the details. > > > > The patch already had a "Signed-off-by" line. > > > > The checkpatch script complains that it doesn't like the formatting of > > the CMD command. However I won't change that in this patch, as this is > > how the whole file is formatted. If this is wanted I can submit a patch > > changing the formatting afterwards, but I think this is an unrelated > > change. > > > > Please apply. > > > > -- > > Hanno Böck > > http://hboeck.de/ > > > > mail/jabber: hanno@hboeck.de > > GPG: BBB51E42 > > > Properly sort cmd tables. > > drm/i915: Properly sort MI coomand table > > In the future, we may want to speed up command/register searching using > a bisection and so we require them to be in ascending order respectively > by command value or register address. However, this was not true for one > pair in the MI table; make it so. > > > Signed-off-by: Hanno Boeck <hanno@hboeck.de> > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> > > > diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c > > index 306d9e4..5fdd8c8 100644 > > --- a/drivers/gpu/drm/i915/i915_cmd_parser.c > > +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c > > @@ -151,8 +151,8 @@ static const struct drm_i915_cmd_descriptor render_cmds[] = { > > CMD( MI_ARB_ON_OFF, SMI, F, 1, R ), > > CMD( MI_PREDICATE, SMI, F, 1, S ), > > CMD( MI_TOPOLOGY_FILTER, SMI, F, 1, S ), > > - CMD( MI_DISPLAY_FLIP, SMI, !F, 0xFF, R ), > > CMD( MI_SET_APPID, SMI, F, 1, S ), > > + CMD( MI_DISPLAY_FLIP, SMI, !F, 0xFF, R ), > > CMD( MI_SET_CONTEXT, SMI, !F, 0xFF, R ), > > CMD( MI_URB_CLEAR, SMI, !F, 0xFF, S ), > > CMD( MI_STORE_DWORD_IMM, SMI, !F, 0x3F, B, > > > Fix loop checking cmd tables. > > drm/i915: Fix command parser table validator > > As we may like to use a bisection search on the tables in future, we > need them to be ordered. For convenience we expect the compiled tables > to be order and check on initialisation. However, the validator used the > wrong iterators failed to spot the misordered MI tables and instead > walked off into the unknown (as spotted by kasan). > > > Signed-off-by: Hanno Boeck <hanno@hboeck.de> > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> Ok I hand-assembled the patches since it's your first, but please followo Documentation/Submitting patches next time around, i.e. 1 mail per patch, no attachments, diff included at the bottom. Otherwise the tools can't just pick up patches on my side. git send-email will do all that formatting for you correctly, so that's what I recommend to use. Anyway pachtes applied to drm-intel-next-queued, thanks a lot for doing them. -Daniel > > > diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c > > index 306d9e4..3a53bf3 100644 > > --- a/drivers/gpu/drm/i915/i915_cmd_parser.c > > +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c > > @@ -564,7 +564,7 @@ static bool validate_cmds_sorted(struct intel_engine_cs *ring, > > > > for (j = 0; j < table->count; j++) { > > const struct drm_i915_cmd_descriptor *desc = > > - &table->table[i]; > > + &table->table[j]; > > u32 curr = desc->cmd.value & desc->cmd.mask; > > > > if (curr < previous) { > > -- > Chris Wilson, Intel Open Source Technology Centre
Fix loop checking cmd tables. Signed-off-by: Hanno Boeck <hanno@hboeck.de> diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c index 306d9e4..3a53bf3 100644 --- a/drivers/gpu/drm/i915/i915_cmd_parser.c +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c @@ -564,7 +564,7 @@ static bool validate_cmds_sorted(struct intel_engine_cs *ring, for (j = 0; j < table->count; j++) { const struct drm_i915_cmd_descriptor *desc = - &table->table[i]; + &table->table[j]; u32 curr = desc->cmd.value & desc->cmd.mask; if (curr < previous) {