[PATCH v5 1/4] drm/i915/display: add support for DMC wakelocks

[Luca Coelho]

On Mon, 2024-04-15 at 05:05 +0000, Shankar, Uma wrote:
> 
> > -----Original Message-----
> > From: Luca Coelho <luca at coelho.fi>
> > Sent: Friday, April 12, 2024 5:57 PM
> > To: Shankar, Uma <uma.shankar at intel.com>; Coelho, Luciano
> > <luciano.coelho at intel.com>; intel-gfx at lists.freedesktop.org
> > Cc: intel-xe at lists.freedesktop.org; ville.syrjala at linux.intel.com; Nikula, Jani
> > <jani.nikula at intel.com>
> > Subject: Re: [PATCH v5 1/4] drm/i915/display: add support for DMC wakelocks
> > 
> > On Fri, 2024-04-12 at 10:30 +0000, Shankar, Uma wrote:
> > > 
> > > > -----Original Message-----
> > > > From: Coelho, Luciano <luciano.coelho at intel.com>
> > > > Sent: Friday, April 12, 2024 3:12 PM
> > > > To: intel-gfx at lists.freedesktop.org
> > > > Cc: intel-xe at lists.freedesktop.org; Shankar, Uma
> > > > <uma.shankar at intel.com>; ville.syrjala at linux.intel.com; Nikula, Jani
> > > > <jani.nikula at intel.com>
> > > > Subject: [PATCH v5 1/4] drm/i915/display: add support for DMC
> > > > wakelocks
> > > > 
> > > > In order to reduce the DC5->DC2 restore time, wakelocks have been
> > > > introduced in DMC so the driver can tell it when registers and other
> > > > memory areas are going to be accessed and keep their respective blocks
> > awake.
> > > > 
> > > > Implement this in the driver by adding the concept of DMC wakelocks.
> > > > When the driver needs to access memory which lies inside pre-defined
> > > > ranges, it will tell DMC to set the wakelock, access the memory,
> > > > then wait for a while and clear the wakelock.
> > > > 
> > > > The wakelock state is protected in the driver with spinlocks to
> > > > prevent concurrency issues.
> > > 
> > > Hi Luca,
> > > Seems you missed to add the version history.
> > 
> > I've been sending the version history in the cover letter, because I don't think it
> > adds any information after it gets to the mainline kernel.  The history is lost
> > anyway, so the mailing list is a better place to store it (it's unique and meaningful
> > there).
> 
> Its matter of preference, but being part of the patch's commit message it stays with it
> and can be checked with a git show. Cover letter details gets lost though as it doesn't
> end up in the tree.

Yes, but the change history in the commit message doesn't tell the full
story.  If I write, for instance, "In v2: refactor intel_foo_bar()",
there's no way to know what it looked like before.  That's why I think
that, if we want to keep the version history accessible from the git
tree, we should have a link to the mailing list discussions, as
apparently we do in most cases anyway.


> > Bur as I said to someone else before, I can add it to the commit message if you
> > think that it's needed.
> 
> Not needed Luca, it was a simple nitpick 😊. You can skip that.

Thanks for pointing out, anyway! 😉 I don't want to keep flaming about
this, so I'll just try to remember to add it next time, so it doesn't
come up again.


> > > 
> > > Anyways, changes look good to me.
> > > Reviewed-by: Uma Shankar <uma.shankar at intel.com>
> > 
> > Thanks a lot!
> > 
> > Though you didn't review patch 3/4, the one about the module parameter.
> > Was that intentional or did you just miss it?
> 
> I think I have reviewed and RB'ed it. The entire series is RB'ed now.

Oh, right.  "Someone" was not paying attention.  I even already had the
r-b in the commit message itself.  Silly me.

Thanks a lot for your reviews! Now I just need to get someone to merge
this series, since I don't have commit rights to the repo yet.

--
Cheers,
Luca.