Re: [Nouveau] [PATCH] drm/nouveau: Add fine-grain temperature reporting
On Thu, 10 Sep 2020 at 00:07, Jeremy Cline wrote:
>
> On Wed, Sep 09, 2020 at 10:22:14AM +0200, Karol Herbst wrote:
> > On Wed, Sep 9, 2020 at 6:06 AM Ben Skeggs wrote:
> > >
> > > On Thu, 13 Aug 2020 at 06:50, Jeremy Cline wrote:
> > > >
> > > > Commit d32656373857 ("drm/nouveau/therm/gp100: initial implementation of
> > > > new gp1xx temperature sensor") added support for reading finer-grain
> > > > temperatures, but continued to report temperatures in 1 degree Celsius
> > > > increments via nvkm_therm_temp_get().
> > > >
> > > > Rather than altering nvkm_therm_temp_get() to report finer-grain
> > > > temperatures, which would be inconvenient for other users of the
> > > > function, a second interface has been added to line up with hwmon's
> > > > native unit of temperature.
> > > Hey Jeremy,
> > >
> > > Sorry this slipped past me until now. I'm OK with adding support for
> > > millidegree temperature reporting, but don't think we need to keep
> > > both interfaces around and would rather see the existing code
> > > converted to return millidegrees (even on GPUs that don't support it)
> > > instead of degrees.
> > >
> > > Thanks!
> > > Ben.
> > >
> >
> > just a note as I was trying something like that in the past: we have a
> > lot of code which fetches the temperature and there are a lot of
> > places where we would then do a divide by 1000, so having a wrapper
> > function at least makes sense. If we want to keep the func pointers?
> > well.. I guess the increased CPU overhead wouldn't be as bad if all
> > sub classes do this static * 1000 as well either. I just think we
> > should have both interfaces in subdev/therm.h so we can just keep most
> > of the current code as is.
> >
>
> Indeed. My initial plan was to change the meaning of temp_get() and
> adjust all the users, but there's around a dozen of them and based on my
> understanding of the users none of them cared much about such accuracy.
>
> However, I do find having one way to do things appealing, so if there's
> a strong preference for it, I'm happy to produce a somewhat more
> invasive patch where all users expect millidegrees.
Yeah, I do have a strong preference for not having to keep multiple
interfaces around unnecessarily. It'd be somewhat different if we had
external interactions, but this is all stuff within the module and we
should be able to make these kinds of changes without too much pain.
Ben.
>
> - Jeremy
>
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau
Re: [Nouveau] [trivial PATCH] treewide: Convert switch/case fallthrough; to break;
On Wed, Sep 09, 2020 at 01:06:39PM -0700, Joe Perches wrote:
> diff --git a/crypto/tcrypt.c b/crypto/tcrypt.c
> index eea0f453cfb6..8aac5bc60f4c 100644
> --- a/crypto/tcrypt.c
> +++ b/crypto/tcrypt.c
> @@ -2464,7 +2464,7 @@ static int do_test(const char *alg, u32 type, u32 mask,
> int m, u32 num_mb)
> test_hash_speed("streebog512", sec,
> generic_hash_speed_template);
> if (mode > 300 && mode < 400) break;
> - fallthrough;
> + break;
> case 399:
> break;
Just imho, this change makes the preceding 'if' look even more
pointless. Maybe the fallthrough was a deliberate choice? Not that my
opinion matters here as I don't know this module, but it looked a bit
odd to me.
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau
Re: [Nouveau] [PATCH 4/5] drm_dp_cec: add plumbing in preparation for MST support
On Wed, 2 Sep 2020 at 04:12, Lyude Paul wrote:
>
> Super minor nitpicks:
>
> On Tue, 2020-09-01 at 16:22 +1000, Sam McNally wrote:
> > From: Hans Verkuil
> >
> > Signed-off-by: Hans Verkuil
> > [sa...@chromium.org:
> > - rebased
> > - removed polling-related changes
> > - moved the calls to drm_dp_cec_(un)set_edid() into the next patch
> > ]
> > Signed-off-by: Sam McNally
> > ---
> >
> > .../display/amdgpu_dm/amdgpu_dm_mst_types.c | 2 +-
> > drivers/gpu/drm/drm_dp_cec.c | 22 ++-
> > drivers/gpu/drm/i915/display/intel_dp.c | 2 +-
> > drivers/gpu/drm/nouveau/nouveau_connector.c | 2 +-
> > include/drm/drm_dp_helper.h | 6 +++--
> > 5 files changed, 19 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> > index 461fa4da0a34..6e7075893ec9 100644
> > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> > @@ -419,7 +419,7 @@ void amdgpu_dm_initialize_dp_connector(struct
> > amdgpu_display_manager *dm,
> >
> > drm_dp_aux_init(>dm_dp_aux.aux);
> > drm_dp_cec_register_connector(>dm_dp_aux.aux,
> > - >base);
> > + >base, false);
> >
> > if (aconnector->base.connector_type == DRM_MODE_CONNECTOR_eDP)
> > return;
> > diff --git a/drivers/gpu/drm/drm_dp_cec.c b/drivers/gpu/drm/drm_dp_cec.c
> > index 3ab2609f9ec7..04ab7b88055c 100644
> > --- a/drivers/gpu/drm/drm_dp_cec.c
> > +++ b/drivers/gpu/drm/drm_dp_cec.c
> > @@ -14,6 +14,7 @@
> > #include
> > #include
> > #include
> > +#include
> >
> > /*
> > * Unfortunately it turns out that we have a chicken-and-egg situation
> > @@ -338,8 +339,6 @@ void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const
> > struct edid *edid)
> > if (aux->cec.adap) {
> > if (aux->cec.adap->capabilities == cec_caps &&
> > aux->cec.adap->available_log_addrs == num_las) {
> > - /* Unchanged, so just set the phys addr */
> > - cec_s_phys_addr_from_edid(aux->cec.adap, edid);
> > goto unlock;
> > }
>
> May as well drop the braces here
>
> > /*
> > @@ -364,15 +363,16 @@ void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const
> > struct edid *edid)
> > if (cec_register_adapter(aux->cec.adap, connector->dev->dev)) {
> > cec_delete_adapter(aux->cec.adap);
> > aux->cec.adap = NULL;
> > - } else {
> > - /*
> > - * Update the phys addr for the new CEC adapter. When called
> > - * from drm_dp_cec_register_connector() edid == NULL, so in
> > - * that case the phys addr is just invalidated.
> > - */
> > - cec_s_phys_addr_from_edid(aux->cec.adap, edid);
> > }
> > unlock:
> > + /*
> > + * Update the phys addr for the new CEC adapter. When called
> > + * from drm_dp_cec_register_connector() edid == NULL, so in
> > + * that case the phys addr is just invalidated.
> > + */
> > + if (aux->cec.adap && edid) {
> > + cec_s_phys_addr_from_edid(aux->cec.adap, edid);
> > + }
>
> And here
>
> > mutex_unlock(>cec.lock);
> > }
> > EXPORT_SYMBOL(drm_dp_cec_set_edid);
> > @@ -418,6 +418,7 @@ EXPORT_SYMBOL(drm_dp_cec_unset_edid);
> > * drm_dp_cec_register_connector() - register a new connector
> > * @aux: DisplayPort AUX channel
> > * @connector: drm connector
> > + * @is_mst: set to true if this is an MST branch
> > *
> > * A new connector was registered with associated CEC adapter name and
> > * CEC adapter parent device. After registering the name and parent
> > @@ -425,12 +426,13 @@ EXPORT_SYMBOL(drm_dp_cec_unset_edid);
> > * CEC and to register a CEC adapter if that is the case.
> > */
> > void drm_dp_cec_register_connector(struct drm_dp_aux *aux,
> > -struct drm_connector *connector)
> > +struct drm_connector *connector, bool
> > is_mst)
> > {
> > WARN_ON(aux->cec.adap);
> > if (WARN_ON(!aux->transfer))
> > return;
> > aux->cec.connector = connector;
> > + aux->cec.is_mst = is_mst;
>
> Also JFYI, you can also check aux->is_remote, but maybe you've got another
> reason for copying this here
>
I think this was just an artefact of this patch originally being
written before aux->is_remote was added. Switching to it mostly
removes the need for this patch, and leaving drm_dp_cec_set_edid()
unchanged, as Hans suggests, removes the rest.
> Either way:
>
> Reviewed-by: Lyude Paul
>
> ...Also, maybe this is just a coincidence - but do I know your name from
> somewhere? Perhaps an IRC community from long ago?
>
Not that I can
Re: [Nouveau] [trivial PATCH] treewide: Convert switch/case fallthrough; to break;
On Wed, 2020-09-09 at 19:36 -0300, Jason Gunthorpe wrote:
> On Wed, Sep 09, 2020 at 01:06:39PM -0700, Joe Perches wrote:
> > fallthrough to a separate case/default label break; isn't very readable.
> >
> > Convert pseudo-keyword fallthrough; statements to a simple break; when
> > the next label is case or default and the only statement in the next
> > label block is break;
> >
> > Found using:
> >
> > $ grep-2.5.4 -rP --include=*.[ch] -n
> > "fallthrough;(\s*(case\s+\w+|default)\s*:\s*){1,7}break;" *
> >
> > Miscellanea:
> >
> > o Move or coalesce a couple label blocks above a default: block.
> >
> > Signed-off-by: Joe Perches
> > ---
> >
> > Compiled allyesconfig x86-64 only.
> > A few files for other arches were not compiled.
>
> IB part looks OK, I prefer it like this
>
> You could do the same for continue as well, I saw a few of those..
I saw some continue uses as well but wasn't sure
and didn't look to see if the switch/case with
continue was in a for/while loop.
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau
Re: [Nouveau] [PATCH 4/5] drm_dp_cec: add plumbing in preparation for MST support
On Tue, 8 Sep 2020 at 18:41, Hans Verkuil wrote:
>
> On 01/09/2020 08:22, Sam McNally wrote:
> > From: Hans Verkuil
> >
> > Signed-off-by: Hans Verkuil
> > [sa...@chromium.org:
> > - rebased
> > - removed polling-related changes
> > - moved the calls to drm_dp_cec_(un)set_edid() into the next patch
> > ]
> > Signed-off-by: Sam McNally
> > ---
> >
> > .../display/amdgpu_dm/amdgpu_dm_mst_types.c | 2 +-
> > drivers/gpu/drm/drm_dp_cec.c | 22 ++-
> > drivers/gpu/drm/i915/display/intel_dp.c | 2 +-
> > drivers/gpu/drm/nouveau/nouveau_connector.c | 2 +-
> > include/drm/drm_dp_helper.h | 6 +++--
> > 5 files changed, 19 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> > index 461fa4da0a34..6e7075893ec9 100644
> > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> > @@ -419,7 +419,7 @@ void amdgpu_dm_initialize_dp_connector(struct
> > amdgpu_display_manager *dm,
> >
> > drm_dp_aux_init(>dm_dp_aux.aux);
> > drm_dp_cec_register_connector(>dm_dp_aux.aux,
> > - >base);
> > + >base, false);
> >
> > if (aconnector->base.connector_type == DRM_MODE_CONNECTOR_eDP)
> > return;
> > diff --git a/drivers/gpu/drm/drm_dp_cec.c b/drivers/gpu/drm/drm_dp_cec.c
> > index 3ab2609f9ec7..04ab7b88055c 100644
> > --- a/drivers/gpu/drm/drm_dp_cec.c
> > +++ b/drivers/gpu/drm/drm_dp_cec.c
> > @@ -14,6 +14,7 @@
> > #include
> > #include
> > #include
> > +#include
> >
> > /*
> > * Unfortunately it turns out that we have a chicken-and-egg situation
> > @@ -338,8 +339,6 @@ void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const
> > struct edid *edid)
> > if (aux->cec.adap) {
> > if (aux->cec.adap->capabilities == cec_caps &&
> > aux->cec.adap->available_log_addrs == num_las) {
> > - /* Unchanged, so just set the phys addr */
> > - cec_s_phys_addr_from_edid(aux->cec.adap, edid);
> > goto unlock;
> > }
> > /*
> > @@ -364,15 +363,16 @@ void drm_dp_cec_set_edid(struct drm_dp_aux *aux,
> > const struct edid *edid)
> > if (cec_register_adapter(aux->cec.adap, connector->dev->dev)) {
> > cec_delete_adapter(aux->cec.adap);
> > aux->cec.adap = NULL;
> > - } else {
> > - /*
> > - * Update the phys addr for the new CEC adapter. When called
> > - * from drm_dp_cec_register_connector() edid == NULL, so in
> > - * that case the phys addr is just invalidated.
> > - */
> > - cec_s_phys_addr_from_edid(aux->cec.adap, edid);
> > }
> > unlock:
> > + /*
> > + * Update the phys addr for the new CEC adapter. When called
> > + * from drm_dp_cec_register_connector() edid == NULL, so in
> > + * that case the phys addr is just invalidated.
> > + */
>
> The comment is no longer in sync with the code: if EDID == NULL, then
> nothing is done due to the edid check in the 'if' below.
>
> > + if (aux->cec.adap && edid) {
>
> I think this should just be: if (aux->cec.adap)
>
> Also, the {} aren't necessary here.
>
> > + cec_s_phys_addr_from_edid(aux->cec.adap, edid);
> > + }
> > mutex_unlock(>cec.lock);
> > }
> > EXPORT_SYMBOL(drm_dp_cec_set_edid);
>
> Frankly, the changes to this function should be dropped completely, from
> what I can see they are not necessary. It was done in my original patch
> because of the way I handled mst, but you did it differently (and I think
> better), so these changes are no longer needed.
>
> I know I am actually commenting on my old patch, but that patch was from a
> work-in-progress git branch and was never meant as a 'proper' patch.
>
> However, what complicates matters is that after digging a bit more I
> discovered
> that commit 732300154980 ("drm: Do not call drm_dp_cec_set_edid() while
> registering
> DP connectors") changed drm_dp_cec_register_connector() so that it no longer
> calls drm_dp_cec_set_edid(), but the comments there and in this function were
> not updated. It would be nice if you can add a patch fixing these outdated
> comments.
>
> Regardless of that change in commit 732300154980, the edid pointer can still
> be
> NULL and the existing behavior should be kept (i.e. create a CEC device, but
> with
> an invalid physical address since there is no EDID for some reason).
>
> Regards,
>
> Hans
>
Thanks. Leaving drm_dp_cec_set_edid() unchanged combined with Lyude's
suggestion to use aux->is_remote removes the need for this patch
entirely.
> > @@ -418,6 +418,7 @@ EXPORT_SYMBOL(drm_dp_cec_unset_edid);
> > *
Re: [Nouveau] [trivial PATCH] treewide: Convert switch/case fallthrough; to break;
On Wed, Sep 09, 2020 at 01:06:39PM -0700, Joe Perches wrote:
> fallthrough to a separate case/default label break; isn't very readable.
>
> Convert pseudo-keyword fallthrough; statements to a simple break; when
> the next label is case or default and the only statement in the next
> label block is break;
>
> Found using:
>
> $ grep-2.5.4 -rP --include=*.[ch] -n
> "fallthrough;(\s*(case\s+\w+|default)\s*:\s*){1,7}break;" *
>
> Miscellanea:
>
> o Move or coalesce a couple label blocks above a default: block.
>
> Signed-off-by: Joe Perches
> ---
>
> Compiled allyesconfig x86-64 only.
> A few files for other arches were not compiled.
IB part looks OK, I prefer it like this
You could do the same for continue as well, I saw a few of those..
Thanks,
Jason
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau
Re: [Nouveau] [trivial PATCH] treewide: Convert switch/case fallthrough; to break;
On 9/9/20 15:06, Joe Perches wrote:
> fallthrough to a separate case/default label break; isn't very readable.
>
> Convert pseudo-keyword fallthrough; statements to a simple break; when
> the next label is case or default and the only statement in the next
> label block is break;
>
> Found using:
>
> $ grep-2.5.4 -rP --include=*.[ch] -n
> "fallthrough;(\s*(case\s+\w+|default)\s*:\s*){1,7}break;" *
>
> Miscellanea:
>
> o Move or coalesce a couple label blocks above a default: block.
>
> Signed-off-by: Joe Perches
Acked-by: Gustavo A. R. Silva
Thanks
--
Gustavo
> ---
>
> Compiled allyesconfig x86-64 only.
> A few files for other arches were not compiled.
>
> arch/arm/mach-mmp/pm-pxa910.c | 2 +-
> arch/arm64/kvm/handle_exit.c | 2 +-
> arch/mips/kernel/cpu-probe.c | 2 +-
> arch/mips/math-emu/cp1emu.c | 2 +-
> arch/s390/pci/pci.c | 2 +-
> crypto/tcrypt.c | 4 ++--
> drivers/ata/sata_mv.c | 2 +-
> drivers/atm/lanai.c | 2 +-
> drivers/gpu/drm/i915/display/intel_sprite.c | 2 +-
> drivers/gpu/drm/nouveau/nvkm/engine/disp/hdmi.c | 2 +-
> drivers/hid/wacom_wac.c | 2 +-
> drivers/i2c/busses/i2c-i801.c | 2 +-
> drivers/infiniband/ulp/rtrs/rtrs-clt.c| 14 +++---
> drivers/infiniband/ulp/rtrs/rtrs-srv.c| 6 +++---
> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 2 +-
> drivers/irqchip/irq-vic.c | 4 ++--
> drivers/md/dm.c | 2 +-
> drivers/media/dvb-frontends/drxd_hard.c | 2 +-
> drivers/media/i2c/ov5640.c| 2 +-
> drivers/media/i2c/ov6650.c| 5 ++---
> drivers/media/i2c/smiapp/smiapp-core.c| 2 +-
> drivers/media/i2c/tvp5150.c | 2 +-
> drivers/media/pci/ddbridge/ddbridge-core.c| 2 +-
> drivers/media/usb/cpia2/cpia2_core.c | 2 +-
> drivers/mfd/iqs62x.c | 3 +--
> drivers/mmc/host/atmel-mci.c | 2 +-
> drivers/mtd/nand/raw/nandsim.c| 2 +-
> drivers/net/ethernet/intel/e1000e/phy.c | 2 +-
> drivers/net/ethernet/intel/fm10k/fm10k_pf.c | 2 +-
> drivers/net/ethernet/intel/i40e/i40e_adminq.c | 2 +-
> drivers/net/ethernet/intel/i40e/i40e_txrx.c | 2 +-
> drivers/net/ethernet/intel/iavf/iavf_txrx.c | 2 +-
> drivers/net/ethernet/intel/igb/e1000_phy.c| 2 +-
> drivers/net/ethernet/intel/ixgbe/ixgbe_82599.c| 2 +-
> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 2 +-
> drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c| 2 +-
> drivers/net/ethernet/intel/ixgbevf/vf.c | 2 +-
> drivers/net/ethernet/netronome/nfp/nfpcore/nfp6000_pcie.c | 2 +-
> drivers/net/ethernet/qlogic/qed/qed_mcp.c | 2 +-
> drivers/net/ethernet/sfc/falcon/farch.c | 2 +-
> drivers/net/ethernet/sfc/farch.c | 2 +-
> drivers/net/phy/adin.c| 3 +--
> drivers/net/usb/pegasus.c | 4 ++--
> drivers/net/usb/usbnet.c | 2 +-
> drivers/net/wireless/ath/ath5k/eeprom.c | 2 +-
> drivers/net/wireless/mediatek/mt7601u/dma.c | 8
> drivers/nvme/host/core.c | 12 ++--
> drivers/pcmcia/db1xxx_ss.c| 4 ++--
> drivers/power/supply/abx500_chargalg.c| 2 +-
> drivers/power/supply/charger-manager.c| 2 +-
> drivers/rtc/rtc-pcf85063.c| 2 +-
> drivers/s390/scsi/zfcp_fsf.c | 2 +-
> drivers/scsi/aic7xxx/aic79xx_core.c | 4 ++--
> drivers/scsi/aic94xx/aic94xx_tmf.c| 2 +-
> drivers/scsi/lpfc/lpfc_sli.c | 2 +-
> drivers/scsi/smartpqi/smartpqi_init.c | 2 +-
> drivers/scsi/sr.c | 2 +-
> drivers/tty/serial/sunsu.c| 2 +-
> drivers/tty/serial/sunzilog.c | 2 +-
> drivers/tty/vt/vt_ioctl.c | 2 +-
> drivers/usb/dwc3/core.c | 2 +-
> drivers/usb/gadget/legacy/inode.c |
[Nouveau] [trivial PATCH] treewide: Convert switch/case fallthrough; to break;
fallthrough to a separate case/default label break; isn't very readable.
Convert pseudo-keyword fallthrough; statements to a simple break; when
the next label is case or default and the only statement in the next
label block is break;
Found using:
$ grep-2.5.4 -rP --include=*.[ch] -n
"fallthrough;(\s*(case\s+\w+|default)\s*:\s*){1,7}break;" *
Miscellanea:
o Move or coalesce a couple label blocks above a default: block.
Signed-off-by: Joe Perches
---
Compiled allyesconfig x86-64 only.
A few files for other arches were not compiled.
arch/arm/mach-mmp/pm-pxa910.c | 2 +-
arch/arm64/kvm/handle_exit.c | 2 +-
arch/mips/kernel/cpu-probe.c | 2 +-
arch/mips/math-emu/cp1emu.c | 2 +-
arch/s390/pci/pci.c | 2 +-
crypto/tcrypt.c | 4 ++--
drivers/ata/sata_mv.c | 2 +-
drivers/atm/lanai.c | 2 +-
drivers/gpu/drm/i915/display/intel_sprite.c | 2 +-
drivers/gpu/drm/nouveau/nvkm/engine/disp/hdmi.c | 2 +-
drivers/hid/wacom_wac.c | 2 +-
drivers/i2c/busses/i2c-i801.c | 2 +-
drivers/infiniband/ulp/rtrs/rtrs-clt.c| 14 +++---
drivers/infiniband/ulp/rtrs/rtrs-srv.c| 6 +++---
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 2 +-
drivers/irqchip/irq-vic.c | 4 ++--
drivers/md/dm.c | 2 +-
drivers/media/dvb-frontends/drxd_hard.c | 2 +-
drivers/media/i2c/ov5640.c| 2 +-
drivers/media/i2c/ov6650.c| 5 ++---
drivers/media/i2c/smiapp/smiapp-core.c| 2 +-
drivers/media/i2c/tvp5150.c | 2 +-
drivers/media/pci/ddbridge/ddbridge-core.c| 2 +-
drivers/media/usb/cpia2/cpia2_core.c | 2 +-
drivers/mfd/iqs62x.c | 3 +--
drivers/mmc/host/atmel-mci.c | 2 +-
drivers/mtd/nand/raw/nandsim.c| 2 +-
drivers/net/ethernet/intel/e1000e/phy.c | 2 +-
drivers/net/ethernet/intel/fm10k/fm10k_pf.c | 2 +-
drivers/net/ethernet/intel/i40e/i40e_adminq.c | 2 +-
drivers/net/ethernet/intel/i40e/i40e_txrx.c | 2 +-
drivers/net/ethernet/intel/iavf/iavf_txrx.c | 2 +-
drivers/net/ethernet/intel/igb/e1000_phy.c| 2 +-
drivers/net/ethernet/intel/ixgbe/ixgbe_82599.c| 2 +-
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 2 +-
drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c| 2 +-
drivers/net/ethernet/intel/ixgbevf/vf.c | 2 +-
drivers/net/ethernet/netronome/nfp/nfpcore/nfp6000_pcie.c | 2 +-
drivers/net/ethernet/qlogic/qed/qed_mcp.c | 2 +-
drivers/net/ethernet/sfc/falcon/farch.c | 2 +-
drivers/net/ethernet/sfc/farch.c | 2 +-
drivers/net/phy/adin.c| 3 +--
drivers/net/usb/pegasus.c | 4 ++--
drivers/net/usb/usbnet.c | 2 +-
drivers/net/wireless/ath/ath5k/eeprom.c | 2 +-
drivers/net/wireless/mediatek/mt7601u/dma.c | 8
drivers/nvme/host/core.c | 12 ++--
drivers/pcmcia/db1xxx_ss.c| 4 ++--
drivers/power/supply/abx500_chargalg.c| 2 +-
drivers/power/supply/charger-manager.c| 2 +-
drivers/rtc/rtc-pcf85063.c| 2 +-
drivers/s390/scsi/zfcp_fsf.c | 2 +-
drivers/scsi/aic7xxx/aic79xx_core.c | 4 ++--
drivers/scsi/aic94xx/aic94xx_tmf.c| 2 +-
drivers/scsi/lpfc/lpfc_sli.c | 2 +-
drivers/scsi/smartpqi/smartpqi_init.c | 2 +-
drivers/scsi/sr.c | 2 +-
drivers/tty/serial/sunsu.c| 2 +-
drivers/tty/serial/sunzilog.c | 2 +-
drivers/tty/vt/vt_ioctl.c | 2 +-
drivers/usb/dwc3/core.c | 2 +-
drivers/usb/gadget/legacy/inode.c | 2 +-
drivers/usb/gadget/udc/pxa25x_udc.c | 4 ++--
drivers/usb/host/ohci-hcd.c | 2 +-
drivers/usb/isp1760/isp1760-hcd.c | 2 +-
drivers/usb/musb/cppi_dma.c
Re: [Nouveau] [PATCH] drm/nouveau: Add fine-grain temperature reporting
On Wed, Sep 09, 2020 at 10:22:14AM +0200, Karol Herbst wrote:
> On Wed, Sep 9, 2020 at 6:06 AM Ben Skeggs wrote:
> >
> > On Thu, 13 Aug 2020 at 06:50, Jeremy Cline wrote:
> > >
> > > Commit d32656373857 ("drm/nouveau/therm/gp100: initial implementation of
> > > new gp1xx temperature sensor") added support for reading finer-grain
> > > temperatures, but continued to report temperatures in 1 degree Celsius
> > > increments via nvkm_therm_temp_get().
> > >
> > > Rather than altering nvkm_therm_temp_get() to report finer-grain
> > > temperatures, which would be inconvenient for other users of the
> > > function, a second interface has been added to line up with hwmon's
> > > native unit of temperature.
> > Hey Jeremy,
> >
> > Sorry this slipped past me until now. I'm OK with adding support for
> > millidegree temperature reporting, but don't think we need to keep
> > both interfaces around and would rather see the existing code
> > converted to return millidegrees (even on GPUs that don't support it)
> > instead of degrees.
> >
> > Thanks!
> > Ben.
> >
>
> just a note as I was trying something like that in the past: we have a
> lot of code which fetches the temperature and there are a lot of
> places where we would then do a divide by 1000, so having a wrapper
> function at least makes sense. If we want to keep the func pointers?
> well.. I guess the increased CPU overhead wouldn't be as bad if all
> sub classes do this static * 1000 as well either. I just think we
> should have both interfaces in subdev/therm.h so we can just keep most
> of the current code as is.
>
Indeed. My initial plan was to change the meaning of temp_get() and
adjust all the users, but there's around a dozen of them and based on my
understanding of the users none of them cared much about such accuracy.
However, I do find having one way to do things appealing, so if there's
a strong preference for it, I'm happy to produce a somewhat more
invasive patch where all users expect millidegrees.
- Jeremy
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau
[Nouveau] nouveau: BUG: Invalid wait context
Greetings,
Box is an aging generic i4790 + GTX-980 desktop.
[ 1143.133663] =
[ 1143.133666] [ BUG: Invalid wait context ]
[ 1143.133671] 5.9.0.g34d4ddd-preempt #2 Tainted: G S E
[ 1143.133675] -
[ 1143.133678] X/2015 is trying to lock:
[ 1143.133682] 8d3e9efd63d8 (>lock){..-.}-{3:3}, at:
get_page_from_freelist+0x6ed/0x1e10
[ 1143.133694] other info that might help us debug this:
[ 1143.133697] context-{5:5}
[ 1143.133700] 4 locks held by X/2015:
[ 1143.133703] #0: 8d3e562d30c0 (>mutex){+.+.}-{4:4}, at:
nouveau_abi16_get+0x2c/0x60 [nouveau]
[ 1143.133756] #1: a9a6c0c57d30
(reservation_ww_class_acquire){+.+.}-{0:0}, at: drm_ioctl_kernel+0x91/0xe0 [drm]
[ 1143.133785] #2: 8d3e3dcef1a0 (reservation_ww_class_mutex){+.+.}-{4:4},
at: nouveau_gem_ioctl_pushbuf+0x63b/0x1cb0 [nouveau]
[ 1143.133834] #3: 8d3e9ec9ea10 (krc.lock){-.-.}-{2:2}, at:
kvfree_call_rcu+0x65/0x210
[ 1143.133845] stack backtrace:
[ 1143.133850] CPU: 2 PID: 2015 Comm: X Kdump: loaded Tainted: G S E
5.9.0.g34d4ddd-preempt #2
[ 1143.133856] Hardware name: MEDION MS-7848/MS-7848, BIOS M7848W08.20C
09/23/2013
[ 1143.133862] Call Trace:
[ 1143.133872] dump_stack+0x77/0x9b
[ 1143.133879] __lock_acquire+0x629/0xbd0
[ 1143.133887] ? try_to_wake_up+0x250/0x860
[ 1143.133894] lock_acquire+0x92/0x390
[ 1143.133900] ? get_page_from_freelist+0x6ed/0x1e10
[ 1143.133908] ? __mutex_unlock_slowpath+0x124/0x280
[ 1143.133915] _raw_spin_lock+0x2f/0x40
[ 1143.133921] ? get_page_from_freelist+0x6ed/0x1e10
[ 1143.133927] get_page_from_freelist+0x6ed/0x1e10
[ 1143.133937] ? lock_acquire+0x92/0x390
[ 1143.133943] ? kvfree_call_rcu+0x65/0x210
[ 1143.133949] __alloc_pages_nodemask+0x173/0x3e0
[ 1143.133957] __get_free_pages+0xd/0x40
[ 1143.133962] kvfree_call_rcu+0x135/0x210
[ 1143.134002] nouveau_fence_unref+0x36/0x50 [nouveau]
[ 1143.134045] validate_fini_no_ticket.isra.8+0x138/0x240 [nouveau]
[ 1143.134090] nouveau_gem_ioctl_pushbuf+0x10c8/0x1cb0 [nouveau]
[ 1143.134136] ? nouveau_gem_ioctl_new+0xc0/0xc0 [nouveau]
[ 1143.134159] ? drm_ioctl_kernel+0x91/0xe0 [drm]
[ 1143.134170] drm_ioctl_kernel+0x91/0xe0 [drm]
[ 1143.134182] drm_ioctl+0x2db/0x380 [drm]
[ 1143.134211] ? nouveau_gem_ioctl_new+0xc0/0xc0 [nouveau]
[ 1143.134217] ? _raw_spin_unlock_irqrestore+0x47/0x60
[ 1143.134222] ? lockdep_hardirqs_on+0x78/0x100
[ 1143.134226] ? _raw_spin_unlock_irqrestore+0x34/0x60
[ 1143.134257] nouveau_drm_ioctl+0x56/0xb0 [nouveau]
[ 1143.134263] __x64_sys_ioctl+0x8e/0xd0
[ 1143.134267] ? lockdep_hardirqs_on+0x78/0x100
[ 1143.134271] do_syscall_64+0x33/0x40
[ 1143.134276] entry_SYSCALL_64_after_hwframe+0x44/0xa9
[ 1143.134280] RIP: 0033:0x7f02e2a84ac7
[ 1143.134284] Code: b3 66 90 48 8b 05 d1 13 2c 00 64 c7 00 26 00 00 00 48 c7
c0 ff ff ff ff c3 66 2e 0f 1f 84 00 00 00 00 00 b8 10 00 00 00 0f 05 <48> 3d 01
f0 ff ff 73 01 c3 48 8b 0d a1 13 2c 00 f7 d8 64 89 01 48
[ 1143.134293] RSP: 002b:7ffe2c2739e8 EFLAGS: 0246 ORIG_RAX:
0010
[ 1143.134298] RAX: ffda RBX: 560c3f86ec08 RCX: 7f02e2a84ac7
[ 1143.134302] RDX: 7ffe2c273a50 RSI: c0406481 RDI: 000e
[ 1143.134306] RBP: 7ffe2c273a50 R08: 560c3f846820 R09: 560c3f87fc08
[ 1143.134310] R10: 560c401dbb38 R11: 0246 R12: c0406481
[ 1143.134314] R13: 000e R14: 560c3f846820 R15: 560c3f84a010
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau
Re: [Nouveau] [bug report] drm/nouveau: move io_reserve_lru handling into the driver v5
Hi Dan,
Am 09.09.20 um 13:49 schrieb Dan Carpenter:
Hello Christian König,
The patch 141b15e59175: "drm/nouveau: move io_reserve_lru handling
into the driver v5" from Aug 21, 2020, leads to the following static
checker warning:
drivers/gpu/drm/nouveau/nouveau_ttm.c:148 nouveau_ttm_fault()
warn: inconsistent returns '*bo->base.resv'.
drivers/gpu/drm/nouveau/nouveau_ttm.c
126 static vm_fault_t nouveau_ttm_fault(struct vm_fault *vmf)
127 {
128 struct vm_area_struct *vma = vmf->vma;
129 struct ttm_buffer_object *bo = vma->vm_private_data;
130 pgprot_t prot;
131 vm_fault_t ret;
132
133 ret = ttm_bo_vm_reserve(bo, vmf);
134 if (ret)
135 return ret;
136
137 nouveau_bo_del_io_reserve_lru(bo);
138
139 prot = vm_get_page_prot(vma->vm_flags);
140 ret = ttm_bo_vm_fault_reserved(vmf, prot,
TTM_BO_VM_NUM_PREFAULT, 1);
141 if (ret == VM_FAULT_RETRY && !(vmf->flags &
FAULT_FLAG_RETRY_NOWAIT))
142 return ret;
^^
Call dma_resv_unlock() before returning?
No, this is correct. In the case of returning VM_FAULT_RETRY the
function ttm_bo_vm_fault_reserved() has already dropped the lock for us.
Regards,
Christian.
143
144 nouveau_bo_add_io_reserve_lru(bo);
145
146 dma_resv_unlock(bo->base.resv);
147
148 return ret;
149 }
regards,
dan carpenter
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau
[Nouveau] [bug report] drm/nouveau: move io_reserve_lru handling into the driver v5
Hello Christian König,
The patch 141b15e59175: "drm/nouveau: move io_reserve_lru handling
into the driver v5" from Aug 21, 2020, leads to the following static
checker warning:
drivers/gpu/drm/nouveau/nouveau_ttm.c:148 nouveau_ttm_fault()
warn: inconsistent returns '*bo->base.resv'.
drivers/gpu/drm/nouveau/nouveau_ttm.c
126 static vm_fault_t nouveau_ttm_fault(struct vm_fault *vmf)
127 {
128 struct vm_area_struct *vma = vmf->vma;
129 struct ttm_buffer_object *bo = vma->vm_private_data;
130 pgprot_t prot;
131 vm_fault_t ret;
132
133 ret = ttm_bo_vm_reserve(bo, vmf);
134 if (ret)
135 return ret;
136
137 nouveau_bo_del_io_reserve_lru(bo);
138
139 prot = vm_get_page_prot(vma->vm_flags);
140 ret = ttm_bo_vm_fault_reserved(vmf, prot,
TTM_BO_VM_NUM_PREFAULT, 1);
141 if (ret == VM_FAULT_RETRY && !(vmf->flags &
FAULT_FLAG_RETRY_NOWAIT))
142 return ret;
^^
Call dma_resv_unlock() before returning?
143
144 nouveau_bo_add_io_reserve_lru(bo);
145
146 dma_resv_unlock(bo->base.resv);
147
148 return ret;
149 }
regards,
dan carpenter
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau
Re: [Nouveau] [PATCH] drm/nouveau: Add fine-grain temperature reporting
On Wed, Sep 9, 2020 at 6:06 AM Ben Skeggs wrote:
>
> On Thu, 13 Aug 2020 at 06:50, Jeremy Cline wrote:
> >
> > Commit d32656373857 ("drm/nouveau/therm/gp100: initial implementation of
> > new gp1xx temperature sensor") added support for reading finer-grain
> > temperatures, but continued to report temperatures in 1 degree Celsius
> > increments via nvkm_therm_temp_get().
> >
> > Rather than altering nvkm_therm_temp_get() to report finer-grain
> > temperatures, which would be inconvenient for other users of the
> > function, a second interface has been added to line up with hwmon's
> > native unit of temperature.
> Hey Jeremy,
>
> Sorry this slipped past me until now. I'm OK with adding support for
> millidegree temperature reporting, but don't think we need to keep
> both interfaces around and would rather see the existing code
> converted to return millidegrees (even on GPUs that don't support it)
> instead of degrees.
>
> Thanks!
> Ben.
>
just a note as I was trying something like that in the past: we have a
lot of code which fetches the temperature and there are a lot of
places where we would then do a divide by 1000, so having a wrapper
function at least makes sense. If we want to keep the func pointers?
well.. I guess the increased CPU overhead wouldn't be as bad if all
sub classes do this static * 1000 as well either. I just think we
should have both interfaces in subdev/therm.h so we can just keep most
of the current code as is.
> >
> > Signed-off-by: Jeremy Cline
> > ---
> > .../drm/nouveau/include/nvkm/subdev/therm.h | 18 +
> > drivers/gpu/drm/nouveau/nouveau_hwmon.c | 4 +--
> > .../gpu/drm/nouveau/nvkm/subdev/therm/base.c | 16
> > .../gpu/drm/nouveau/nvkm/subdev/therm/gp100.c | 25 +--
> > .../gpu/drm/nouveau/nvkm/subdev/therm/priv.h | 1 +
> > 5 files changed, 60 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/nouveau/include/nvkm/subdev/therm.h
> > b/drivers/gpu/drm/nouveau/include/nvkm/subdev/therm.h
> > index 62c34f98c930..7b9928dd001c 100644
> > --- a/drivers/gpu/drm/nouveau/include/nvkm/subdev/therm.h
> > +++ b/drivers/gpu/drm/nouveau/include/nvkm/subdev/therm.h
> > @@ -100,6 +100,24 @@ struct nvkm_therm {
> > };
> >
> > int nvkm_therm_temp_get(struct nvkm_therm *);
> > +
> > +/**
> > + * nvkm_therm_temp_millidegree_get() - get the temperature in millidegrees
> > + * @therm: The thermal device to read from.
> > + *
> > + * This interface reports temperatures in units of millidegree Celsius to
> > + * align with the hwmon API. Some cards may only be capable of reporting in
> > + * units of Celsius, and those that report finer grain temperatures may
> > not be
> > + * capable of millidegree Celsius accuracy,
> > + *
> > + * For cases where millidegree temperature is too fine-grain, the
> > + * nvkm_therm_temp_get() interface reports temperatures in one degree
> > Celsius
> > + * increments.
> > + *
> > + * Return: The temperature in millidegrees Celsius, or -ENODEV if
> > temperature
> > + * reporting is not supported.
> > + */
> > +int nvkm_therm_temp_millidegree_get(struct nvkm_therm *therm);
> > int nvkm_therm_fan_sense(struct nvkm_therm *);
> > int nvkm_therm_cstate(struct nvkm_therm *, int, int);
> > void nvkm_therm_clkgate_init(struct nvkm_therm *,
> > diff --git a/drivers/gpu/drm/nouveau/nouveau_hwmon.c
> > b/drivers/gpu/drm/nouveau/nouveau_hwmon.c
> > index 1c3104d20571..e96355f93ce5 100644
> > --- a/drivers/gpu/drm/nouveau/nouveau_hwmon.c
> > +++ b/drivers/gpu/drm/nouveau/nouveau_hwmon.c
> > @@ -428,8 +428,8 @@ nouveau_temp_read(struct device *dev, u32 attr, int
> > channel, long *val)
> > case hwmon_temp_input:
> > if (drm_dev->switch_power_state != DRM_SWITCH_POWER_ON)
> > return -EINVAL;
> > - ret = nvkm_therm_temp_get(therm);
> > - *val = ret < 0 ? ret : (ret * 1000);
> > + ret = nvkm_therm_temp_millidegree_get(therm);
> > + *val = ret;
> > break;
> > case hwmon_temp_max:
> > *val = therm->attr_get(therm, NVKM_THERM_ATTR_THRS_DOWN_CLK)
> > diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/therm/base.c
> > b/drivers/gpu/drm/nouveau/nvkm/subdev/therm/base.c
> > index 4a4d1e224126..e655b32c78b8 100644
> > --- a/drivers/gpu/drm/nouveau/nvkm/subdev/therm/base.c
> > +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/therm/base.c
> > @@ -34,6 +34,22 @@ nvkm_therm_temp_get(struct nvkm_therm *therm)
> > return -ENODEV;
> > }
> >
> > +int
> > +nvkm_therm_temp_millidegree_get(struct nvkm_therm *therm)
> > +{
> > + int ret = -ENODEV;
> > +
> > + if (therm->func->temp_millidegree_get)
> > + return therm->func->temp_millidegree_get(therm);
> > +
> > + if (therm->func->temp_get) {
> > + ret = therm->func->temp_get(therm);
> > + if (ret > 0)
> > + ret *= 1000;
> > + }
