mbox series

[git,pull] drm fixes for 5.12 final

Message ID CAPM=9tyKdGHyiRLDooKrMf=02GtNn8U4YfF4dJtXdabnVAGdXQ@mail.gmail.com (mailing list archive)
State New, archived
Headers show
Series [git,pull] drm fixes for 5.12 final | expand

Pull-request

git://anongit.freedesktop.org/drm/drm tags/drm-fixes-2021-04-23

Message

Dave Airlie April 23, 2021, 3:52 a.m. UTC
Hi Linus,

Just some small i915 and amdgpu fixes this week, Should be all until
you open the merge window.

Regards,
Dave.

drm-fixes-2021-04-23:
drm fixes for 5.12 final

amdgpu:
- Fix gpuvm page table update issue
- Modifier fixes
- Register fix for dimgrey cavefish

i915:
- GVT's BDW regression fix for cmd parser
- Fix modesetting in case of unexpected AUX timeouts
The following changes since commit bf05bf16c76bb44ab5156223e1e58e26dfe30a88:

  Linux 5.12-rc8 (2021-04-18 14:45:32 -0700)

are available in the Git repository at:

  git://anongit.freedesktop.org/drm/drm tags/drm-fixes-2021-04-23

for you to fetch changes up to aca38735ae624b93c71c055b68d5802b8f356ea5:

  Merge tag 'drm-intel-fixes-2021-04-22' of
git://anongit.freedesktop.org/drm/drm-intel into drm-fixes (2021-04-23
12:18:21 +1000)

----------------------------------------------------------------
drm fixes for 5.12 final

amdgpu:
- Fix gpuvm page table update issue
- Modifier fixes
- Register fix for dimgrey cavefish

i915:
- GVT's BDW regression fix for cmd parser
- Fix modesetting in case of unexpected AUX timeouts

----------------------------------------------------------------
Dave Airlie (2):
      Merge tag 'amd-drm-fixes-5.12-2021-04-21' of
https://gitlab.freedesktop.org/agd5f/linux into drm-fixes
      Merge tag 'drm-intel-fixes-2021-04-22' of
git://anongit.freedesktop.org/drm/drm-intel into drm-fixes

Imre Deak (1):
      drm/i915: Fix modesetting in case of unexpected AUX timeouts

Jiansong Chen (1):
      drm/amdgpu: fix GCR_GENERAL_CNTL offset for dimgrey_cavefish

Philip Yang (1):
      drm/amdgpu: reserve fence slot to update page table

Qingqing Zhuo (1):
      drm/amd/display: Update modifier list for gfx10_3

Rodrigo Vivi (1):
      Merge tag 'gvt-fixes-2021-04-20' of
https://github.com/intel/gvt-linux into drm-intel-fixes

Simon Ser (1):
      amd/display: allow non-linear multi-planar formats

Zhenyu Wang (1):
      drm/i915/gvt: Fix BDW command parser regression

 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c                | 10 ++++++++--
 drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c                |  2 +-
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c     | 15 ++++++---------
 drivers/gpu/drm/i915/display/intel_dp_link_training.c |  3 ++-
 drivers/gpu/drm/i915/gvt/cmd_parser.c                 | 19 +++++++++++++------
 5 files changed, 30 insertions(+), 19 deletions(-)

Comments

pr-tracker-bot@kernel.org April 23, 2021, 5:26 p.m. UTC | #1
The pull request you sent on Fri, 23 Apr 2021 13:52:29 +1000:

> git://anongit.freedesktop.org/drm/drm tags/drm-fixes-2021-04-23

has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/5bfc75d92efd494db37f5c4c173d3639d4772966

Thank you!
Linus Torvalds April 27, 2021, 11:43 p.m. UTC | #2
I've updated to Fedora 34 on one of my machines, and it causes a lot
of i915 warnings like

  drivers/gpu/drm/i915/intel_pm.c: In function ‘ilk_setup_wm_latency’:
  drivers/gpu/drm/i915/intel_pm.c:3059:9: note: referencing argument 3
of type ‘const u16 *’ {aka ‘const short unsigned int *’}
  drivers/gpu/drm/i915/intel_pm.c:2994:13: note: in a call to function
‘intel_print_wm_latency’

and the reason is that gcc now seems to look at the argument array
size more, and notices that

 (a) intel_print_wm_latency() takes a "const u16 wm[8]" argument

but

 (b) most of the arrays passed in tend to look like 'u16 pri_latency[5]'

I think I will make the argument type to intel_print_wm_latency() be
just "const u16 wm[]" for now, just to avoid seeing a ton of silly
warnings.

I'm not sure if there is a better solution (like making all of those
latency arrays be 8 entries in size), so I'm just letting you know
about my change in this area in case anybody has a better idea.

             Linus
Linus Torvalds April 28, 2021, 12:26 a.m. UTC | #3
On Tue, Apr 27, 2021 at 4:43 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> I think I will make the argument type to intel_print_wm_latency() be
> just "const u16 wm[]" for now, just to avoid seeing a ton of silly
> warnings.

After fixing the trivial ones, this one remains:

  drivers/gpu/drm/i915/display/intel_dp.c: In function
‘intel_dp_check_mst_status’:
  drivers/gpu/drm/i915/display/intel_dp.c:4554:22: warning:
‘drm_dp_channel_eq_ok’ reading 6 bytes from a region of size 4
[-Wstringop-overread]
   4554 |                     !drm_dp_channel_eq_ok(&esi[10],
intel_dp->lane_count)) {
        |
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  drivers/gpu/drm/i915/display/intel_dp.c:4554:22: note: referencing
argument 1 of type ‘const u8 *’ {aka ‘const unsigned char *’}
  In file included from drivers/gpu/drm/i915/display/intel_dp.c:38:
  ./include/drm/drm_dp_helper.h:1459:6: note: in a call to function
‘drm_dp_channel_eq_ok’
   1459 | bool drm_dp_channel_eq_ok(const u8 link_status[DP_LINK_STATUS_SIZE],
        |      ^~~~~~~~~~~~~~~~~~~~

and I'm not fixing that one, because it actually looks like a valid
warning, and doesn't have an obvious fix.

That "esi[]" array is 14 bytes in size (DP_DPRX_ESI_LEN). So when it
does that "&esi[10]" and passes it in as an argument, then only 4
bytes remain of the array.

And drm_dp_channel_eq_ok() supposedly takes a "const u8
link_status[DP_LINK_STATUS_SIZE]", which is 6 bytes.

There may be some reason this is ok, but it does look a bit fishy, and
the compiler warning is appropriate.

            Linus
Jani Nikula April 28, 2021, 7:27 a.m. UTC | #4
On Tue, 27 Apr 2021, Linus Torvalds <torvalds@linux-foundation.org> wrote:
> I've updated to Fedora 34 on one of my machines, and it causes a lot
> of i915 warnings like
>
>   drivers/gpu/drm/i915/intel_pm.c: In function ‘ilk_setup_wm_latency’:
>   drivers/gpu/drm/i915/intel_pm.c:3059:9: note: referencing argument 3
> of type ‘const u16 *’ {aka ‘const short unsigned int *’}
>   drivers/gpu/drm/i915/intel_pm.c:2994:13: note: in a call to function
> ‘intel_print_wm_latency’
>
> and the reason is that gcc now seems to look at the argument array
> size more, and notices that

Arnd Bergmann reported some of these a while back. I think we have some
of them fixed in our -next already, but not all. Thanks for the
reminder.

BR,
Jani.
Linus Torvalds May 8, 2021, 6:51 p.m. UTC | #5
I have heard nothing about this, and it remains the only warning from
my allmodconfig build (I have another one for drm compiled with clang,
but there I at least heard back that a fix exists).

Since I am going to release rc1 tomorrow, and I don't want to release
it with an ugly compiler warning, I took it upon myself to just fix
the code:

  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=fec4d42724a1bf3dcba52307e55375fdb967b852

HOWEVER.

That commit fixes the warning, and is at worst harmless. At best it
fixes an access to random stack memory. But it does smell like
somebody who actually knows how these arrays work should look at that
code.

IOW, maybe the code should actually have read 16 bytes from the Event
Status Indicator? Maybe offset 10 was wrong? Maybe
drm_dp_channel_eq_ok() should never have taken six bytes to begin
with?

It's a mystery, and I haven't heard anything otherwise, so there it is.

              Linus

On Wed, Apr 28, 2021 at 12:27 AM Jani Nikula <jani.nikula@intel.com> wrote:
>
> On Tue, 27 Apr 2021, Linus Torvalds <torvalds@linux-foundation.org> wrote:
> > I've updated to Fedora 34 on one of my machines, and it causes a lot
> > of i915 warnings like
> >
> >   drivers/gpu/drm/i915/intel_pm.c: In function ‘ilk_setup_wm_latency’:
> >   drivers/gpu/drm/i915/intel_pm.c:3059:9: note: referencing argument 3
> > of type ‘const u16 *’ {aka ‘const short unsigned int *’}
> >   drivers/gpu/drm/i915/intel_pm.c:2994:13: note: in a call to function
> > ‘intel_print_wm_latency’
> >
> > and the reason is that gcc now seems to look at the argument array
> > size more, and notices that
>
> Arnd Bergmann reported some of these a while back. I think we have some
> of them fixed in our -next already, but not all. Thanks for the
> reminder.
Jani Nikula May 10, 2021, 9:20 a.m. UTC | #6
On Sat, 08 May 2021, Linus Torvalds <torvalds@linux-foundation.org> wrote:
> I have heard nothing about this, and it remains the only warning from
> my allmodconfig build (I have another one for drm compiled with clang,
> but there I at least heard back that a fix exists).
>
> Since I am going to release rc1 tomorrow, and I don't want to release
> it with an ugly compiler warning, I took it upon myself to just fix
> the code:
>
>   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=fec4d42724a1bf3dcba52307e55375fdb967b852
>
> HOWEVER.
>
> That commit fixes the warning, and is at worst harmless. At best it
> fixes an access to random stack memory. But it does smell like
> somebody who actually knows how these arrays work should look at that
> code.
>
> IOW, maybe the code should actually have read 16 bytes from the Event
> Status Indicator? Maybe offset 10 was wrong? Maybe
> drm_dp_channel_eq_ok() should never have taken six bytes to begin
> with?
>
> It's a mystery, and I haven't heard anything otherwise, so there it is.

Fair enough. My bad for not getting this fixed.

The fix is harmless. drm_dp_channel_eq_ok() only ever accesses 3 bytes
instead of 6. I figure the DP_LINK_STATUS_SIZE (=6) is there because in
the normal case you'd read that much, and use a family of functions on
that data, some of which do access the full 6 bytes, some don't.

In our case, we use drm_dp_channel_eq_ok() to check 3 bytes of similarly
encoded data elsewhere in the DPCD address space, and the
DP_LINK_STATUS_SIZE is meaningless there.

The straightforward fix would be to replace
link_status[DP_LINK_STATUS_SIZE] with link_status[3], and that likely
needs changes in dp_link_status() and dp_get_lane_status() as well.


BR,
Jani.


>
>               Linus
>
> On Wed, Apr 28, 2021 at 12:27 AM Jani Nikula <jani.nikula@intel.com> wrote:
>>
>> On Tue, 27 Apr 2021, Linus Torvalds <torvalds@linux-foundation.org> wrote:
>> > I've updated to Fedora 34 on one of my machines, and it causes a lot
>> > of i915 warnings like
>> >
>> >   drivers/gpu/drm/i915/intel_pm.c: In function ‘ilk_setup_wm_latency’:
>> >   drivers/gpu/drm/i915/intel_pm.c:3059:9: note: referencing argument 3
>> > of type ‘const u16 *’ {aka ‘const short unsigned int *’}
>> >   drivers/gpu/drm/i915/intel_pm.c:2994:13: note: in a call to function
>> > ‘intel_print_wm_latency’
>> >
>> > and the reason is that gcc now seems to look at the argument array
>> > size more, and notices that
>>
>> Arnd Bergmann reported some of these a while back. I think we have some
>> of them fixed in our -next already, but not all. Thanks for the
>> reminder.