| Message ID | 20230215005419.2100887-4-umesh.nerlige.ramappa@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add OAM support for MTL | expand |
On Tue, 14 Feb 2023 16:54:13 -0800, Umesh Nerlige Ramappa wrote: > > Validate the OA sseu config after all params are parsed. Commit messages for all patches need to answer the "why" or the reason for the patch. In this case maybe an overkill but probably something like: Validate the OA sseu config after all params are parsed since the engine can be passed in as part of perf properties. > > Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com> > --- > drivers/gpu/drm/i915/i915_perf.c | 22 ++++++++++++---------- > 1 file changed, 12 insertions(+), 10 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c > index a879ae4bf8d7..0b2097ad000e 100644 > --- a/drivers/gpu/drm/i915/i915_perf.c > +++ b/drivers/gpu/drm/i915/i915_perf.c > @@ -3956,6 +3956,8 @@ static int read_properties_unlocked(struct i915_perf *perf, > u64 __user *uprop = uprops; > u32 i; > int ret; > + bool config_sseu = false; > + struct drm_i915_gem_context_param_sseu user_sseu; nit: longer lines above shorter lines > > memset(props, 0, sizeof(struct perf_open_properties)); > props->poll_oa_period = DEFAULT_POLL_PERIOD_NS; > @@ -4082,8 +4084,6 @@ static int read_properties_unlocked(struct i915_perf *perf, > props->hold_preemption = !!value; > break; > case DRM_I915_PERF_PROP_GLOBAL_SSEU: { > - struct drm_i915_gem_context_param_sseu user_sseu; > - > if (GRAPHICS_VER_FULL(perf->i915) >= IP_VER(12, 50)) { > drm_dbg(&perf->i915->drm, > "SSEU config not supported on gfx %x\n", > @@ -4098,14 +4098,7 @@ static int read_properties_unlocked(struct i915_perf *perf, > "Unable to copy global sseu parameter\n"); > return -EFAULT; > } > - > - ret = get_sseu_config(&props->sseu, props->engine, &user_sseu); > - if (ret) { > - drm_dbg(&perf->i915->drm, > - "Invalid SSEU configuration\n"); > - return ret; > - } > - props->has_sseu = true; > + config_sseu = true; > break; > } > case DRM_I915_PERF_PROP_POLL_OA_PERIOD: > @@ -4125,6 +4118,15 @@ static int read_properties_unlocked(struct i915_perf *perf, > uprop += 2; > } > > + if (config_sseu) { > + ret = get_sseu_config(&props->sseu, props->engine, &user_sseu); > + if (ret) { > + DRM_DEBUG("Invalid SSEU configuration\n"); drm_dbg? DRM_DEBUG is deprecated? > + return ret; > + } > + props->has_sseu = true; > + } > + > return 0; > } After addressing the above comments: Reviewed-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
On Wed, 15 Feb 2023 21:08:43 -0800, Dixit, Ashutosh wrote: > > On Tue, 14 Feb 2023 16:54:13 -0800, Umesh Nerlige Ramappa wrote: > > > > Validate the OA sseu config after all params are parsed. > > Commit messages for all patches need to answer the "why" or the reason for > the patch. In this case maybe an overkill but probably something like: > > Validate the OA sseu config after all params are parsed since the engine > can be passed in as part of perf properties. Also, if we do this the patch should probably be later in the series after the patch which introduces engine class/instance in the perf properties.
On Wed, 15 Feb 2023 21:36:50 -0800, Dixit, Ashutosh wrote: > > On Wed, 15 Feb 2023 21:08:43 -0800, Dixit, Ashutosh wrote: > > > > On Tue, 14 Feb 2023 16:54:13 -0800, Umesh Nerlige Ramappa wrote: > > > > > > Validate the OA sseu config after all params are parsed. > > > > Commit messages for all patches need to answer the "why" or the reason for > > the patch. In this case maybe an overkill but probably something like: > > > > Validate the OA sseu config after all params are parsed since the engine > > can be passed in as part of perf properties. > > Also, if we do this the patch should probably be later in the series after > the patch which introduces engine class/instance in the perf properties. General guidelines for submitting a patch series for review (for the future): 1. The commit message should explain "why" or reason for a patch 2. As far as possible patches should be in a logical order so the series should "tell a story" 3. The patches should be small (each patch being a single logical change if possible) So after we've done the hard part of figuring out the code and getting it to work, the above guidelines also make the review process easier. Thanks. -- Ashutosh
On Thu, Feb 16, 2023 at 08:31:21AM -0800, Dixit, Ashutosh wrote: >On Wed, 15 Feb 2023 21:36:50 -0800, Dixit, Ashutosh wrote: >> >> On Wed, 15 Feb 2023 21:08:43 -0800, Dixit, Ashutosh wrote: >> > >> > On Tue, 14 Feb 2023 16:54:13 -0800, Umesh Nerlige Ramappa wrote: >> > > >> > > Validate the OA sseu config after all params are parsed. >> > >> > Commit messages for all patches need to answer the "why" or the reason for >> > the patch. In this case maybe an overkill but probably something like: >> > >> > Validate the OA sseu config after all params are parsed since the engine >> > can be passed in as part of perf properties. >> >> Also, if we do this the patch should probably be later in the series after >> the patch which introduces engine class/instance in the perf properties. > >General guidelines for submitting a patch series for review (for the >future): > >1. The commit message should explain "why" or reason for a patch >2. As far as possible patches should be in a logical order so the series > should "tell a story" >3. The patches should be small (each patch being a single logical change if > possible) > >So after we've done the hard part of figuring out the code and getting it >to work, the above guidelines also make the review process easier. will do and post a new revision. Thanks, Umesh > >Thanks. >-- >Ashutosh
On Wed, Feb 15, 2023 at 09:36:50PM -0800, Dixit, Ashutosh wrote: >On Wed, 15 Feb 2023 21:08:43 -0800, Dixit, Ashutosh wrote: >> >> On Tue, 14 Feb 2023 16:54:13 -0800, Umesh Nerlige Ramappa wrote: >> > >> > Validate the OA sseu config after all params are parsed. >> >> Commit messages for all patches need to answer the "why" or the reason for >> the patch. In this case maybe an overkill but probably something like: >> >> Validate the OA sseu config after all params are parsed since the engine >> can be passed in as part of perf properties. > >Also, if we do this the patch should probably be later in the series after >the patch which introduces engine class/instance in the perf properties. Reg: order of this patch, I am thinking it still makes sense to have it here (before the class:instance patch). It's more like a refactor before enabling the feature so that once the feature is enabled, there are no corner cases. Thoughts? I would just add the same description in the commit message. Thanks, Umesh
diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c index a879ae4bf8d7..0b2097ad000e 100644 --- a/drivers/gpu/drm/i915/i915_perf.c +++ b/drivers/gpu/drm/i915/i915_perf.c @@ -3956,6 +3956,8 @@ static int read_properties_unlocked(struct i915_perf *perf, u64 __user *uprop = uprops; u32 i; int ret; + bool config_sseu = false; + struct drm_i915_gem_context_param_sseu user_sseu; memset(props, 0, sizeof(struct perf_open_properties)); props->poll_oa_period = DEFAULT_POLL_PERIOD_NS; @@ -4082,8 +4084,6 @@ static int read_properties_unlocked(struct i915_perf *perf, props->hold_preemption = !!value; break; case DRM_I915_PERF_PROP_GLOBAL_SSEU: { - struct drm_i915_gem_context_param_sseu user_sseu; - if (GRAPHICS_VER_FULL(perf->i915) >= IP_VER(12, 50)) { drm_dbg(&perf->i915->drm, "SSEU config not supported on gfx %x\n", @@ -4098,14 +4098,7 @@ static int read_properties_unlocked(struct i915_perf *perf, "Unable to copy global sseu parameter\n"); return -EFAULT; } - - ret = get_sseu_config(&props->sseu, props->engine, &user_sseu); - if (ret) { - drm_dbg(&perf->i915->drm, - "Invalid SSEU configuration\n"); - return ret; - } - props->has_sseu = true; + config_sseu = true; break; } case DRM_I915_PERF_PROP_POLL_OA_PERIOD: @@ -4125,6 +4118,15 @@ static int read_properties_unlocked(struct i915_perf *perf, uprop += 2; } + if (config_sseu) { + ret = get_sseu_config(&props->sseu, props->engine, &user_sseu); + if (ret) { + DRM_DEBUG("Invalid SSEU configuration\n"); + return ret; + } + props->has_sseu = true; + } + return 0; }
Validate the OA sseu config after all params are parsed. Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com> --- drivers/gpu/drm/i915/i915_perf.c | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-)