| Message ID | 1478625682-8955-1-git-send-email-paulo.r.zanoni@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Nov 08, 2016 at 03:21:22PM -0200, Paulo Zanoni wrote: > One of the memsets was added by 5a920b85f2c6 ("drm/i915/gen9: fix DDB > partitioning for multi-screen cases"), and the other was added by > 01c72d6c17 ("drm/i915/gen9: fix DDB partitioning for multi-screen > cases"). I'm confused and I'll let the maintainers find out what went > wrong here. > > Cc: Jani Nikula <jani.nikula@intel.com> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> I think this is just an artifact of having the patch in both -next and -fixes. The context probably changed later in -next causing a conflict when the two were merged, and then the merge resolution accidentally duplicated this hunk. The merge commit ac4139ed7 is where we wound up with the two copies: diff --cc drivers/gpu/drm/i915/intel_pm.c index db24f89,cc9e0c0..88e28c9 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@@ -3361,11 -3404,13 +3404,17 @@@ skl_allocate_pipe_ddb(struct intel_crtc unsigned int total_data_rate; int num_active; int id, i; + unsigned plane_data_rate[I915_MAX_PLANES] = {}; + unsigned plane_y_data_rate[I915_MAX_PLANES] = {}; + + /* Clear the partitioning for disabled planes. */ + memset(ddb->plane[pipe], 0, sizeof(ddb->plane[pipe])); + memset(ddb->y_plane[pipe], 0, sizeof(ddb->y_plane[pipe])); + /* Clear the partitioning for disabled planes. */ + memset(ddb->plane[pipe], 0, sizeof(ddb->plane[pipe])); + memset(ddb->y_plane[pipe], 0, sizeof(ddb->y_plane[pipe])); + if (WARN_ON(!state)) return 0; Dropping one looks good to me. Reviewed-by: Matt Roper <matthew.d.roper@intel.com> > --- > drivers/gpu/drm/i915/intel_pm.c | 4 ---- > 1 file changed, 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index 88e28c9..cc9e0c0 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -3411,10 +3411,6 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate, > memset(ddb->plane[pipe], 0, sizeof(ddb->plane[pipe])); > memset(ddb->y_plane[pipe], 0, sizeof(ddb->y_plane[pipe])); > > - /* Clear the partitioning for disabled planes. */ > - memset(ddb->plane[pipe], 0, sizeof(ddb->plane[pipe])); > - memset(ddb->y_plane[pipe], 0, sizeof(ddb->y_plane[pipe])); > - > if (WARN_ON(!state)) > return 0; > > -- > 2.7.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, 09 Nov 2016, Matt Roper <matthew.d.roper@intel.com> wrote: > On Tue, Nov 08, 2016 at 03:21:22PM -0200, Paulo Zanoni wrote: >> One of the memsets was added by 5a920b85f2c6 ("drm/i915/gen9: fix DDB >> partitioning for multi-screen cases"), and the other was added by >> 01c72d6c17 ("drm/i915/gen9: fix DDB partitioning for multi-screen >> cases"). I'm confused and I'll let the maintainers find out what went >> wrong here. >> >> Cc: Jani Nikula <jani.nikula@intel.com> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > I think this is just an artifact of having the patch in both -next and > -fixes. The context probably changed later in -next causing a conflict > when the two were merged, and then the merge resolution accidentally > duplicated this hunk. The merge commit ac4139ed7 is where we wound up > with the two copies: I don't have that merge commit. Is that a nightly rebuild? I don't think we even have this issue in anywhere other than nightly. There isn't a branch we could apply this fix to. BR, Jani. > > diff --cc drivers/gpu/drm/i915/intel_pm.c > index db24f89,cc9e0c0..88e28c9 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@@ -3361,11 -3404,13 +3404,17 @@@ skl_allocate_pipe_ddb(struct intel_crtc > unsigned int total_data_rate; > int num_active; > int id, i; > + unsigned plane_data_rate[I915_MAX_PLANES] = {}; > + unsigned plane_y_data_rate[I915_MAX_PLANES] = {}; > + > + /* Clear the partitioning for disabled planes. */ > + memset(ddb->plane[pipe], 0, sizeof(ddb->plane[pipe])); > + memset(ddb->y_plane[pipe], 0, sizeof(ddb->y_plane[pipe])); > > + /* Clear the partitioning for disabled planes. */ > + memset(ddb->plane[pipe], 0, sizeof(ddb->plane[pipe])); > + memset(ddb->y_plane[pipe], 0, sizeof(ddb->y_plane[pipe])); > + > if (WARN_ON(!state)) > return 0; > > Dropping one looks good to me. > > Reviewed-by: Matt Roper <matthew.d.roper@intel.com> > >> --- >> drivers/gpu/drm/i915/intel_pm.c | 4 ---- >> 1 file changed, 4 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c >> index 88e28c9..cc9e0c0 100644 >> --- a/drivers/gpu/drm/i915/intel_pm.c >> +++ b/drivers/gpu/drm/i915/intel_pm.c >> @@ -3411,10 +3411,6 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate, >> memset(ddb->plane[pipe], 0, sizeof(ddb->plane[pipe])); >> memset(ddb->y_plane[pipe], 0, sizeof(ddb->y_plane[pipe])); >> >> - /* Clear the partitioning for disabled planes. */ >> - memset(ddb->plane[pipe], 0, sizeof(ddb->plane[pipe])); >> - memset(ddb->y_plane[pipe], 0, sizeof(ddb->y_plane[pipe])); >> - >> if (WARN_ON(!state)) >> return 0; >> >> -- >> 2.7.4 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, Nov 09, 2016 at 11:27:37AM +0200, Jani Nikula wrote: > On Wed, 09 Nov 2016, Matt Roper <matthew.d.roper@intel.com> wrote: > > On Tue, Nov 08, 2016 at 03:21:22PM -0200, Paulo Zanoni wrote: > >> One of the memsets was added by 5a920b85f2c6 ("drm/i915/gen9: fix DDB > >> partitioning for multi-screen cases"), and the other was added by > >> 01c72d6c17 ("drm/i915/gen9: fix DDB partitioning for multi-screen > >> cases"). I'm confused and I'll let the maintainers find out what went > >> wrong here. > >> > >> Cc: Jani Nikula <jani.nikula@intel.com> > >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > > I think this is just an artifact of having the patch in both -next and > > -fixes. The context probably changed later in -next causing a conflict > > when the two were merged, and then the merge resolution accidentally > > duplicated this hunk. The merge commit ac4139ed7 is where we wound up > > with the two copies: > > I don't have that merge commit. Is that a nightly rebuild? Yeah, it's part of a nightly rebuild. Looks like 82282577545 is the equivalent now. > I don't think we even have this issue in anywhere other than > nightly. There isn't a branch we could apply this fix to. Yeah, the patch can't apply anywhere directly, but I thought we could make rerere forget its resolution of intel_pm.c for that merge and then fix it by hand so that future merges behave correctly? Matt > > BR, > Jani. > > > > > > diff --cc drivers/gpu/drm/i915/intel_pm.c > > index db24f89,cc9e0c0..88e28c9 > > --- a/drivers/gpu/drm/i915/intel_pm.c > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > @@@ -3361,11 -3404,13 +3404,17 @@@ skl_allocate_pipe_ddb(struct intel_crtc > > unsigned int total_data_rate; > > int num_active; > > int id, i; > > + unsigned plane_data_rate[I915_MAX_PLANES] = {}; > > + unsigned plane_y_data_rate[I915_MAX_PLANES] = {}; > > + > > + /* Clear the partitioning for disabled planes. */ > > + memset(ddb->plane[pipe], 0, sizeof(ddb->plane[pipe])); > > + memset(ddb->y_plane[pipe], 0, sizeof(ddb->y_plane[pipe])); > > > > + /* Clear the partitioning for disabled planes. */ > > + memset(ddb->plane[pipe], 0, sizeof(ddb->plane[pipe])); > > + memset(ddb->y_plane[pipe], 0, sizeof(ddb->y_plane[pipe])); > > + > > if (WARN_ON(!state)) > > return 0; > > > > Dropping one looks good to me. > > > > Reviewed-by: Matt Roper <matthew.d.roper@intel.com> > > > >> --- > >> drivers/gpu/drm/i915/intel_pm.c | 4 ---- > >> 1 file changed, 4 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > >> index 88e28c9..cc9e0c0 100644 > >> --- a/drivers/gpu/drm/i915/intel_pm.c > >> +++ b/drivers/gpu/drm/i915/intel_pm.c > >> @@ -3411,10 +3411,6 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate, > >> memset(ddb->plane[pipe], 0, sizeof(ddb->plane[pipe])); > >> memset(ddb->y_plane[pipe], 0, sizeof(ddb->y_plane[pipe])); > >> > >> - /* Clear the partitioning for disabled planes. */ > >> - memset(ddb->plane[pipe], 0, sizeof(ddb->plane[pipe])); > >> - memset(ddb->y_plane[pipe], 0, sizeof(ddb->y_plane[pipe])); > >> - > >> if (WARN_ON(!state)) > >> return 0; > >> > >> -- > >> 2.7.4 > >> > >> _______________________________________________ > >> Intel-gfx mailing list > >> Intel-gfx@lists.freedesktop.org > >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Jani Nikula, Intel Open Source Technology Center
On Wed, 09 Nov 2016, Matt Roper <matthew.d.roper@intel.com> wrote: > On Wed, Nov 09, 2016 at 11:27:37AM +0200, Jani Nikula wrote: >> On Wed, 09 Nov 2016, Matt Roper <matthew.d.roper@intel.com> wrote: >> > On Tue, Nov 08, 2016 at 03:21:22PM -0200, Paulo Zanoni wrote: >> >> One of the memsets was added by 5a920b85f2c6 ("drm/i915/gen9: fix DDB >> >> partitioning for multi-screen cases"), and the other was added by >> >> 01c72d6c17 ("drm/i915/gen9: fix DDB partitioning for multi-screen >> >> cases"). I'm confused and I'll let the maintainers find out what went >> >> wrong here. >> >> >> >> Cc: Jani Nikula <jani.nikula@intel.com> >> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> >> > >> > I think this is just an artifact of having the patch in both -next and >> > -fixes. The context probably changed later in -next causing a conflict >> > when the two were merged, and then the merge resolution accidentally >> > duplicated this hunk. The merge commit ac4139ed7 is where we wound up >> > with the two copies: >> >> I don't have that merge commit. Is that a nightly rebuild? > > Yeah, it's part of a nightly rebuild. Looks like > 82282577545 is the equivalent now. > >> I don't think we even have this issue in anywhere other than >> nightly. There isn't a branch we could apply this fix to. > > Yeah, the patch can't apply anywhere directly, but I thought we could > make rerere forget its resolution of intel_pm.c for that merge and then > fix it by hand so that future merges behave correctly? Only if it was a manual fixup that went wrong. For silent conflicts, we need to add a fixup patch for the merge. BR, Jani. > > > Matt > > >> >> BR, >> Jani. >> >> >> > >> > diff --cc drivers/gpu/drm/i915/intel_pm.c >> > index db24f89,cc9e0c0..88e28c9 >> > --- a/drivers/gpu/drm/i915/intel_pm.c >> > +++ b/drivers/gpu/drm/i915/intel_pm.c >> > @@@ -3361,11 -3404,13 +3404,17 @@@ skl_allocate_pipe_ddb(struct intel_crtc >> > unsigned int total_data_rate; >> > int num_active; >> > int id, i; >> > + unsigned plane_data_rate[I915_MAX_PLANES] = {}; >> > + unsigned plane_y_data_rate[I915_MAX_PLANES] = {}; >> > + >> > + /* Clear the partitioning for disabled planes. */ >> > + memset(ddb->plane[pipe], 0, sizeof(ddb->plane[pipe])); >> > + memset(ddb->y_plane[pipe], 0, sizeof(ddb->y_plane[pipe])); >> > >> > + /* Clear the partitioning for disabled planes. */ >> > + memset(ddb->plane[pipe], 0, sizeof(ddb->plane[pipe])); >> > + memset(ddb->y_plane[pipe], 0, sizeof(ddb->y_plane[pipe])); >> > + >> > if (WARN_ON(!state)) >> > return 0; >> > >> > Dropping one looks good to me. >> > >> > Reviewed-by: Matt Roper <matthew.d.roper@intel.com> >> > >> >> --- >> >> drivers/gpu/drm/i915/intel_pm.c | 4 ---- >> >> 1 file changed, 4 deletions(-) >> >> >> >> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c >> >> index 88e28c9..cc9e0c0 100644 >> >> --- a/drivers/gpu/drm/i915/intel_pm.c >> >> +++ b/drivers/gpu/drm/i915/intel_pm.c >> >> @@ -3411,10 +3411,6 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate, >> >> memset(ddb->plane[pipe], 0, sizeof(ddb->plane[pipe])); >> >> memset(ddb->y_plane[pipe], 0, sizeof(ddb->y_plane[pipe])); >> >> >> >> - /* Clear the partitioning for disabled planes. */ >> >> - memset(ddb->plane[pipe], 0, sizeof(ddb->plane[pipe])); >> >> - memset(ddb->y_plane[pipe], 0, sizeof(ddb->y_plane[pipe])); >> >> - >> >> if (WARN_ON(!state)) >> >> return 0; >> >> >> >> -- >> >> 2.7.4 >> >> >> >> _______________________________________________ >> >> Intel-gfx mailing list >> >> Intel-gfx@lists.freedesktop.org >> >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx >> >> -- >> Jani Nikula, Intel Open Source Technology Center
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 88e28c9..cc9e0c0 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -3411,10 +3411,6 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate, memset(ddb->plane[pipe], 0, sizeof(ddb->plane[pipe])); memset(ddb->y_plane[pipe], 0, sizeof(ddb->y_plane[pipe])); - /* Clear the partitioning for disabled planes. */ - memset(ddb->plane[pipe], 0, sizeof(ddb->plane[pipe])); - memset(ddb->y_plane[pipe], 0, sizeof(ddb->y_plane[pipe])); - if (WARN_ON(!state)) return 0;
One of the memsets was added by 5a920b85f2c6 ("drm/i915/gen9: fix DDB partitioning for multi-screen cases"), and the other was added by 01c72d6c17 ("drm/i915/gen9: fix DDB partitioning for multi-screen cases"). I'm confused and I'll let the maintainers find out what went wrong here. Cc: Jani Nikula <jani.nikula@intel.com> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> --- drivers/gpu/drm/i915/intel_pm.c | 4 ---- 1 file changed, 4 deletions(-)