Re: [PATCH 1/2] drm/i915/bdw: Implement a basic PM interrupt handler

Daniel Vetter
2014-04-16T07:47:36Z

On Tue, Apr 15, 2014 at 07:43:07PM -0700, Ben Widawsky wrote:
> On Mon, Apr 14, 2014 at 10:55:53PM +0300, Ville Syrjälä wrote:
> > On Mon, Apr 14, 2014 at 10:41:14PM +0530, deepak.s@intel.com wrote:
> > > From: Ben Widawsky <benjamin.widawsky@intel.com>
> > >
> > > Almost all of it is reusable from the existing code. The primary
> > > difference is we need to do even less in the interrupt handler, since
> > > interrupts are not shared in the same way.
> > >
> > > The patch is mostly a copy-paste of the existing snb+ code, with updates
> > > to the relevant parts requiring changes to the interrupt handling. As
> > > such it /should/ be relatively trivial. It's highly likely that I missed
> > > some places where I need a gen8 version of the PM interrupts, but it has
> > > become invisible to me by now.
> > >
> > > This patch could probably be split into adding the new functions,
> > > followed by actually handling the interrupts. Since the code is
> > > currently disabled (and broken) I think the patch stands better by
> > > itself.
> > >
> > > v2: Move the commit about not touching the ringbuffer interrupt to the
> > > snb_* function where it belongs (Rodrigo)
> > >
> > > v3: Rebased on Paulo's runtime PM changes
> > >
> > > v4: Not well validated, but rebase on
> > > commit 730488b2eddded4497f63f70867b1256cd9e117c
> > > Author: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > Date: Fri Mar 7 20:12:32 2014 -0300
> > >
> > > drm/i915: kill dev_priv->pm.regsave
> > >
> > > v5: Rebased on latest code base. (Deepak)
> > >
> > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > >
> > > Conflicts:
> > > drivers/gpu/drm/i915/i915_irq.c
> >
> > IIRC Daniel doesn't like these conflict markers. So should be dropped.
> >
>
> I like the conflict markers generally. Daniel can kill it if he likes,
> but thanks for the input. I've killed it this time around, but I don't
> plan on it for the future.

Imo leaving them in is just lazy. Either there was some real conflict that
required real work, and then you should rev the patch revision and take
note about the changes in the in-patch changelog.

Or the conflict was trivial, in which case the left-behind marker is
meaningless and doesn't add anything useful to the commit message.

So please remove them and if it makes sense augment the patch revision
log. See e.g. the rebase notes (where I mention the upstream changes that
required the rebase) I add for -internal patches.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch