Re: [Intel-gfx] [RFC, drm-misc-next v4 3/9] drm/radeon: Implement .be_primary() callback
Am 04.09.23 um 21:57 schrieb Sui Jingfeng:
From: Sui Jingfeng
On a machine with multiple GPUs, a Linux user has no control over which one
is primary at boot time.
Question is why is that useful? Should we give users the ability to
control that?
I don't see an use case for this.
Regards,
Christian.
This patch tries to solve the mentioned problem by
implementing the .be_primary() callback. Pass radeon.modeset=10 on the
kernel cmd line if you really want the device bound by radeon to be the
primary video adapter, no matter what VGAARB say.
Cc: Alex Deucher
Cc: Christian Koenig
Signed-off-by: Sui Jingfeng
---
drivers/gpu/drm/radeon/radeon_device.c | 10 +-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/radeon/radeon_device.c
b/drivers/gpu/drm/radeon/radeon_device.c
index 71f2ff39d6a1..b661cd3a8dc2 100644
--- a/drivers/gpu/drm/radeon/radeon_device.c
+++ b/drivers/gpu/drm/radeon/radeon_device.c
@@ -1263,6 +1263,14 @@ static const struct vga_switcheroo_client_ops
radeon_switcheroo_ops = {
.can_switch = radeon_switcheroo_can_switch,
};
+static bool radeon_want_to_be_primary(struct pci_dev *pdev)
+{
+ if (radeon_modeset == 10)
+ return true;
+
+ return false;
+}
+
/**
* radeon_device_init - initialize the driver
*
@@ -1425,7 +1433,7 @@ int radeon_device_init(struct radeon_device *rdev,
/* if we have > 1 VGA cards, then disable the radeon VGA resources */
/* this will fail for cards that aren't VGA class devices, just
* ignore it */
- vga_client_register(rdev->pdev, radeon_vga_set_decode, NULL);
+ vga_client_register(rdev->pdev, radeon_vga_set_decode,
radeon_want_to_be_primary);
if (rdev->flags & RADEON_IS_PX)
runtime = true;
Re: [Intel-gfx] [PATCH v2 04/22] drm/i915/dp: Update the link bpp limits for DSC mode
On Mon, Sep 04, 2023 at 02:08:38PM +0300, Imre Deak wrote:
> On Mon, Sep 04, 2023 at 06:48:25AM +0300, Ville Syrjälä wrote:
> > On Thu, Aug 24, 2023 at 11:04:59AM +0300, Imre Deak wrote:
> > > In non-DSC mode the link bpp can be set in 2*3 bpp steps in the pipe bpp
> > > range, while in DSC mode it can be set in 1/16 bpp steps to any value
> > > up to the maximum pipe bpp. Update the limits accordingly in both modes
> > > to prepare for a follow-up patch which may need to reduce the max link
> > > bpp value and starts to check the link bpp limits in DSC mode as well.
> > >
> > > While at it add more detail to the link limit debug print and print it
> > > also for DSC mode.
> > >
> > > v2:
> > > - Add to_bpp_frac_dec() instead of open coding it. (Jani)
> > >
> > > Cc: Jani Nikula
> > > Signed-off-by: Imre Deak
> > > ---
> > > .../drm/i915/display/intel_display_types.h| 5 ++
> > > drivers/gpu/drm/i915/display/intel_dp.c | 89 +++
> > > drivers/gpu/drm/i915/display/intel_dp.h | 6 ++
> > > drivers/gpu/drm/i915/display/intel_dp_mst.c | 23 +++--
> > > 4 files changed, 101 insertions(+), 22 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > index 5875eff5012ce..a0a404967b5d2 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > @@ -2113,6 +2113,11 @@ static inline int to_bpp_int(int bpp_x16)
> > > return bpp_x16 >> 4;
> > > }
> > >
> > > +static inline int to_bpp_frac_dec(int bpp_x16)
> > > +{
> > > + return (bpp_x16 & 0xf) * 625;
> > > +}
> >
> > This gives me the impression that this would be somehow
> > generally useful, but I presume we only use it for the printk?
> > So maybe should just have some printk FMT+ARG macros for
> > this stuff?
>
> Yes, only used by printks. Make sense to define the FMT+ARG helpers at
> one place, can add these here.
>
> >
> > > +
> > > static inline int to_bpp_x16(int bpp)
> > > {
> > > return bpp << 4;
> > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> > > b/drivers/gpu/drm/i915/display/intel_dp.c
> > > index c580472c06b85..9ce861a7fd418 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > > @@ -2189,16 +2189,68 @@ int intel_dp_dsc_compute_config(struct intel_dp
> > > *intel_dp,
> > > return 0;
> > > }
> > >
> > > -static void
> > > +/**
> > > + * intel_dp_compute_config_link_bpp_limits - compute output link bpp
> > > limits
> > > + * @intel_dp: intel DP
> > > + * @crtc_state: crtc state
> > > + * @dsc: DSC compression mode
> > > + * @limits: link configuration limits
> > > + *
> > > + * Calculates the output link min, max bpp values in @limits based on the
> > > + * pipe bpp range, @crtc_state and @dsc mode.
> > > + *
> > > + * Returns %true in case of success.
> > > + */
> > > +bool
> > > +intel_dp_compute_config_link_bpp_limits(struct intel_dp *intel_dp,
> > > + const struct intel_crtc_state
> > > *crtc_state,
> > > + bool dsc,
> > > + struct link_config_limits *limits)
> > > +{
> > > + struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc->dev);
> > > + const struct drm_display_mode *adjusted_mode =
> > > + _state->hw.adjusted_mode;
> > > + const struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > > + const struct intel_encoder *encoder = _to_dig_port(intel_dp)->base;
> > > + int max_link_bpp_x16;
> > > +
> > > + max_link_bpp_x16 = to_bpp_x16(limits->pipe.max_bpp);
> > > +
> > > + if (!dsc) {
> > > + max_link_bpp_x16 = rounddown(max_link_bpp_x16, to_bpp_x16(2 *
> > > 3));
> > > +
> > > + if (max_link_bpp_x16 < to_bpp_x16(limits->pipe.min_bpp))
> > > + return false;
> >
> > Quite a few to_bpp_x16()'s in there. Seems like it would a bit simpler
> > to just do that once at the end.
>
> At the moment yes, but in a later patch max_link_bpp_x16 starts out as
> crtc_state->max_link_bpp_x16 limited value (with a non-zero fractional
> part).
>
> >
> > > +
> > > + limits->link.min_bpp_x16 = to_bpp_x16(limits->pipe.min_bpp);
> > > + } else {
> > > + limits->link.min_bpp_x16 = 0;
> >
> > Why is that zero? Don't we now have some helpers to fill
> > this stuff correctly?
>
> At the moment it's calculated only later in
> intel_edp_dsc_compute_pipe_bpp() / intel_dp_dsc_compute_pipe_bpp().
>
> It should be inited already here, but I wanted to do that only as a
> follow-up, since there's been other DSC changes from Ankit still under
> review. Is that ok, adding a TODO: here?
Sure.
--
Ville Syrjälä
Intel
[Intel-gfx] ✗ Fi.CI.IGT: failure for PCI/VGA: Allowing the user to select the primary video adapter at boot time
== Series Details ==
Series: PCI/VGA: Allowing the user to select the primary video adapter at boot
time
URL : https://patchwork.freedesktop.org/series/123258/
State : failure
== Summary ==
CI Bug Log - changes from CI_DRM_13594_full -> Patchwork_123258v1_full
Summary
---
**FAILURE**
Serious unknown changes coming with Patchwork_123258v1_full absolutely need
to be
verified manually.
If you think the reported changes have nothing to do with the changes
introduced in Patchwork_123258v1_full, please notify your bug team to allow
them
to document this new failure mode, which will reduce false positives in CI.
Participating hosts (9 -> 9)
--
No changes in participating hosts
Possible new issues
---
Here are the unknown changes that may have been introduced in
Patchwork_123258v1_full:
### IGT changes ###
Possible regressions
* igt@kms_flip@flip-vs-suspend@c-hdmi-a3:
- shard-dg2: NOTRUN -> [INCOMPLETE][1]
[1]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_123258v1/shard-dg2-5/igt@kms_flip@flip-vs-susp...@c-hdmi-a3.html
Suppressed
The following results come from untrusted machines, tests, or statuses.
They do not affect the overall result.
* {igt@kms_dirtyfb@dirtyfb-ioctl@drrs-hdmi-a-3}:
- shard-dg1: NOTRUN -> [SKIP][2] +1 similar issue
[2]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_123258v1/shard-dg1-12/igt@kms_dirtyfb@dirtyfb-io...@drrs-hdmi-a-3.html
New tests
-
New tests have been introduced between CI_DRM_13594_full and
Patchwork_123258v1_full:
### New IGT tests (1) ###
* igt@gen9_exec_parse:
- Statuses :
- Exec time: [None] s
Known issues
Here are the changes found in Patchwork_123258v1_full that come from known
issues:
### IGT changes ###
Issues hit
* igt@api_intel_bb@blit-reloc-purge-cache:
- shard-dg2: NOTRUN -> [SKIP][3] ([i915#8411])
[3]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_123258v1/shard-dg2-2/igt@api_intel...@blit-reloc-purge-cache.html
* igt@api_intel_bb@render-ccs:
- shard-dg2: NOTRUN -> [FAIL][4] ([i915#6122])
[4]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_123258v1/shard-dg2-11/igt@api_intel...@render-ccs.html
* igt@drm_fdinfo@virtual-busy-idle:
- shard-dg2: NOTRUN -> [SKIP][5] ([i915#8414])
[5]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_123258v1/shard-dg2-11/igt@drm_fdi...@virtual-busy-idle.html
* igt@feature_discovery@psr2:
- shard-dg2: NOTRUN -> [SKIP][6] ([i915#658]) +3 similar issues
[6]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_123258v1/shard-dg2-11/igt@feature_discov...@psr2.html
* igt@gem_ccs@suspend-resume:
- shard-mtlp: NOTRUN -> [SKIP][7] ([i915#5325])
[7]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_123258v1/shard-mtlp-6/igt@gem_...@suspend-resume.html
* igt@gem_ctx_persistence@hang:
- shard-dg2: NOTRUN -> [SKIP][8] ([i915#8555])
[8]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_123258v1/shard-dg2-10/igt@gem_ctx_persiste...@hang.html
* igt@gem_ctx_persistence@heartbeat-stop:
- shard-mtlp: NOTRUN -> [SKIP][9] ([i915#8555])
[9]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_123258v1/shard-mtlp-6/igt@gem_ctx_persiste...@heartbeat-stop.html
* igt@gem_ctx_persistence@legacy-engines-cleanup:
- shard-snb: NOTRUN -> [SKIP][10] ([fdo#109271] / [i915#1099]) +3
similar issues
[10]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_123258v1/shard-snb6/igt@gem_ctx_persiste...@legacy-engines-cleanup.html
* igt@gem_eio@hibernate:
- shard-dg2: NOTRUN -> [ABORT][11] ([i915#7975] / [i915#8213])
[11]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_123258v1/shard-dg2-2/igt@gem_...@hibernate.html
* igt@gem_eio@kms:
- shard-dg1: NOTRUN -> [FAIL][12] ([i915#5784])
[12]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_123258v1/shard-dg1-17/igt@gem_...@kms.html
* igt@gem_exec_balancer@bonded-semaphore:
- shard-mtlp: NOTRUN -> [SKIP][13] ([i915#4812])
[13]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_123258v1/shard-mtlp-6/igt@gem_exec_balan...@bonded-semaphore.html
* igt@gem_exec_fair@basic-deadline:
- shard-rkl: [PASS][14] -> [FAIL][15] ([i915#2846])
[14]:
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13594/shard-rkl-7/igt@gem_exec_f...@basic-deadline.html
[15]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_123258v1/shard-rkl-4/igt@gem_exec_f...@basic-deadline.html
* igt@gem_exec_fair@basic-pace@rcs0:
- shard-rkl: [PASS][16] -> [FAIL][17] ([i915#2842])
[16]:
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13594/shard-rkl-4/igt@gem_exec_fair@basic-p...@rcs0.html
[17]:
[Intel-gfx] ✓ Fi.CI.BAT: success for PCI/VGA: Allowing the user to select the primary video adapter at boot time
== Series Details == Series: PCI/VGA: Allowing the user to select the primary video adapter at boot time URL : https://patchwork.freedesktop.org/series/123258/ State : success == Summary == CI Bug Log - changes from CI_DRM_13594 -> Patchwork_123258v1 Summary --- **SUCCESS** No regressions found. External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_123258v1/index.html Participating hosts (22 -> 39) -- Additional (18): fi-kbl-7567u fi-rkl-11600 bat-adlp-11 bat-dg2-8 fi-skl-guc bat-adlm-1 fi-tgl-1115g4 bat-adlp-6 fi-kbl-guc fi-glk-j4005 fi-kbl-x1275 fi-ivb-3770 fi-elk-e7500 bat-jsl-3 bat-dg2-13 fi-blb-e6850 bat-jsl-1 bat-mtlp-6 Missing(1): fi-snb-2520m Known issues Here are the changes found in Patchwork_123258v1 that come from known issues: ### IGT changes ### Issues hit * igt@debugfs_test@basic-hwmon: - bat-adlp-6: NOTRUN -> [SKIP][1] ([i915#7456]) [1]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_123258v1/bat-adlp-6/igt@debugfs_t...@basic-hwmon.html - fi-rkl-11600: NOTRUN -> [SKIP][2] ([i915#7456]) [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_123258v1/fi-rkl-11600/igt@debugfs_t...@basic-hwmon.html - bat-jsl-3: NOTRUN -> [SKIP][3] ([i915#7456]) [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_123258v1/bat-jsl-3/igt@debugfs_t...@basic-hwmon.html - bat-adlp-11:NOTRUN -> [SKIP][4] ([i915#7456]) [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_123258v1/bat-adlp-11/igt@debugfs_t...@basic-hwmon.html - bat-adlm-1: NOTRUN -> [SKIP][5] ([i915#7456]) [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_123258v1/bat-adlm-1/igt@debugfs_t...@basic-hwmon.html - bat-jsl-1: NOTRUN -> [SKIP][6] ([i915#7456]) [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_123258v1/bat-jsl-1/igt@debugfs_t...@basic-hwmon.html - fi-tgl-1115g4: NOTRUN -> [SKIP][7] ([i915#7456]) [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_123258v1/fi-tgl-1115g4/igt@debugfs_t...@basic-hwmon.html - bat-mtlp-6: NOTRUN -> [SKIP][8] ([i915#7456]) [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_123258v1/bat-mtlp-6/igt@debugfs_t...@basic-hwmon.html * igt@debugfs_test@read_all_entries: - fi-kbl-7567u: NOTRUN -> [ABORT][9] ([i915#8913]) [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_123258v1/fi-kbl-7567u/igt@debugfs_test@read_all_entries.html * igt@fbdev@eof: - bat-adlm-1: NOTRUN -> [SKIP][10] ([i915#2582]) +3 similar issues [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_123258v1/bat-adlm-1/igt@fb...@eof.html * igt@fbdev@info: - fi-kbl-x1275: NOTRUN -> [SKIP][11] ([fdo#109271] / [i915#1849]) [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_123258v1/fi-kbl-x1275/igt@fb...@info.html - fi-kbl-guc: NOTRUN -> [SKIP][12] ([fdo#109271] / [i915#1849]) [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_123258v1/fi-kbl-guc/igt@fb...@info.html - bat-adlm-1: NOTRUN -> [SKIP][13] ([i915#1849] / [i915#2582]) [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_123258v1/bat-adlm-1/igt@fb...@info.html - bat-mtlp-6: NOTRUN -> [SKIP][14] ([i915#1849] / [i915#2582]) [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_123258v1/bat-mtlp-6/igt@fb...@info.html * igt@fbdev@write: - bat-mtlp-6: NOTRUN -> [SKIP][15] ([i915#2582]) +3 similar issues [15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_123258v1/bat-mtlp-6/igt@fb...@write.html * igt@gem_exec_suspend@basic-s0@smem: - bat-dg2-8: NOTRUN -> [INCOMPLETE][16] ([i915#6311]) [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_123258v1/bat-dg2-8/igt@gem_exec_suspend@basic...@smem.html * igt@gem_huc_copy@huc-copy: - fi-tgl-1115g4: NOTRUN -> [SKIP][17] ([i915#2190]) [17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_123258v1/fi-tgl-1115g4/igt@gem_huc_c...@huc-copy.html - bat-jsl-1: NOTRUN -> [SKIP][18] ([i915#2190]) [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_123258v1/bat-jsl-1/igt@gem_huc_c...@huc-copy.html - fi-rkl-11600: NOTRUN -> [SKIP][19] ([i915#2190]) [19]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_123258v1/fi-rkl-11600/igt@gem_huc_c...@huc-copy.html - bat-jsl-3: NOTRUN -> [SKIP][20] ([i915#2190]) [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_123258v1/bat-jsl-3/igt@gem_huc_c...@huc-copy.html - fi-glk-j4005: NOTRUN -> [SKIP][21] ([fdo#109271] / [i915#2190]) [21]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_123258v1/fi-glk-j4005/igt@gem_huc_c...@huc-copy.html - fi-kbl-x1275: NOTRUN -> [SKIP][22] ([fdo#109271] / [i915#2190]) [22]:
[Intel-gfx] ✗ Fi.CI.SPARSE: warning for PCI/VGA: Allowing the user to select the primary video adapter at boot time
== Series Details == Series: PCI/VGA: Allowing the user to select the primary video adapter at boot time URL : https://patchwork.freedesktop.org/series/123258/ State : warning == Summary == Error: dim sparse failed Sparse version: v0.6.2 Fast mode used, each commit won't be checked separately. +./arch/x86/include/asm/bitops.h:117:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:117:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:117:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:117:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:117:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:117:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:117:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:117:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:148:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:148:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:148:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:148:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:148:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:148:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:148:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:148:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:150:9: warning: unreplaced symbol 'oldbit' +./arch/x86/include/asm/bitops.h:150:9: warning: unreplaced symbol 'oldbit' +./arch/x86/include/asm/bitops.h:150:9: warning: unreplaced symbol 'oldbit' +./arch/x86/include/asm/bitops.h:150:9: warning: unreplaced symbol 'oldbit' +./arch/x86/include/asm/bitops.h:150:9: warning: unreplaced symbol 'oldbit' +./arch/x86/include/asm/bitops.h:150:9: warning: unreplaced symbol 'oldbit' +./arch/x86/include/asm/bitops.h:150:9: warning: unreplaced symbol 'oldbit' +./arch/x86/include/asm/bitops.h:150:9: warning: unreplaced symbol 'oldbit' +./arch/x86/include/asm/bitops.h:154:26: warning: unreplaced symbol 'oldbit' +./arch/x86/include/asm/bitops.h:154:26: warning: unreplaced symbol 'oldbit' +./arch/x86/include/asm/bitops.h:154:26: warning: unreplaced symbol 'oldbit' +./arch/x86/include/asm/bitops.h:154:26: warning: unreplaced symbol 'oldbit' +./arch/x86/include/asm/bitops.h:154:26: warning: unreplaced symbol 'oldbit' +./arch/x86/include/asm/bitops.h:154:26: warning: unreplaced symbol 'oldbit' +./arch/x86/include/asm/bitops.h:154:26: warning: unreplaced symbol 'oldbit' +./arch/x86/include/asm/bitops.h:154:26: warning: unreplaced symbol 'oldbit' +./arch/x86/include/asm/bitops.h:156:16: warning: unreplaced symbol 'oldbit' +./arch/x86/include/asm/bitops.h:156:16: warning: unreplaced symbol 'oldbit' +./arch/x86/include/asm/bitops.h:156:16: warning: unreplaced symbol 'oldbit' +./arch/x86/include/asm/bitops.h:156:16: warning: unreplaced symbol 'oldbit' +./arch/x86/include/asm/bitops.h:156:16: warning: unreplaced symbol 'oldbit' +./arch/x86/include/asm/bitops.h:156:16: warning: unreplaced symbol 'oldbit' +./arch/x86/include/asm/bitops.h:156:16: warning: unreplaced symbol 'oldbit' +./arch/x86/include/asm/bitops.h:156:16: warning: unreplaced symbol 'oldbit' +./arch/x86/include/asm/bitops.h:156:9: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:156:9: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:156:9: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:156:9: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:156:9: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:156:9: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:156:9: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:156:9: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:174:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:174:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:174:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:174:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:174:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:174:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:174:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:174:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:176:9: warning: unreplaced symbol 'oldbit' +./arch/x86/include/asm/bitops.h:176:9: warning: unreplaced symbol 'oldbit' +./arch/x86/include/asm/bitops.h:176:9: warning: unreplaced symbol 'oldbit' +./arch/x86/include/asm/bitops.h:176:9: warning: unreplaced symbol 'oldbit' +./arch/x86/include/asm/bitops.h:176:9: warning: unreplaced symbol 'oldbit' +./arch/x86/include/asm/bitops.h:176:9:
[Intel-gfx] [RFC, drm-misc-next v4 7/9] drm/ast: Register as a VGA client by calling vga_client_register()
From: Sui Jingfeng
Becasuse the display controller in the ASpeed BMC chip is a VGA-compatible
device, the software programming guide of AST2400 say that it is fully
IBM VGA compliant. Thus, it should also participate in the arbitration.
Cc: Thomas Zimmermann
Cc: Jocelyn Falempe
Signed-off-by: Sui Jingfeng
---
drivers/gpu/drm/ast/ast_drv.c | 31 +++
1 file changed, 31 insertions(+)
diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c
index e1224ef4ad83..1349f7bb5dfb 100644
--- a/drivers/gpu/drm/ast/ast_drv.c
+++ b/drivers/gpu/drm/ast/ast_drv.c
@@ -28,6 +28,7 @@
#include
#include
+#include
#include
#include
@@ -89,6 +90,34 @@ static const struct pci_device_id ast_pciidlist[] = {
MODULE_DEVICE_TABLE(pci, ast_pciidlist);
+static bool ast_want_to_be_primary(struct pci_dev *pdev)
+{
+ if (ast_modeset == 10)
+ return true;
+
+ return false;
+}
+
+static unsigned int ast_vga_set_decode(struct pci_dev *pdev, bool state)
+{
+ struct drm_device *drm = pci_get_drvdata(pdev);
+ struct ast_device *ast = to_ast_device(drm);
+ unsigned int decode;
+
+ if (state) {
+ /* Enable standard VGA decode and Enable normal VGA decode */
+ ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xa1, 0x04);
+
+ decode = VGA_RSRC_LEGACY_IO | VGA_RSRC_LEGACY_MEM |
+VGA_RSRC_NORMAL_IO | VGA_RSRC_NORMAL_MEM;
+ } else {
+ ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xa1, 0x07);
+ decode = VGA_RSRC_NORMAL_IO | VGA_RSRC_NORMAL_MEM;
+ }
+
+ return decode;
+}
+
static int ast_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
{
struct ast_device *ast;
@@ -112,6 +141,8 @@ static int ast_pci_probe(struct pci_dev *pdev, const struct
pci_device_id *ent)
if (ret)
return ret;
+ vga_client_register(pdev, ast_vga_set_decode, ast_want_to_be_primary);
+
drm_fbdev_generic_setup(dev, 32);
return 0;
--
2.34.1
[Intel-gfx] [RFC, drm-misc-next v4 3/9] drm/radeon: Implement .be_primary() callback
From: Sui Jingfeng
On a machine with multiple GPUs, a Linux user has no control over which one
is primary at boot time. This patch tries to solve the mentioned problem by
implementing the .be_primary() callback. Pass radeon.modeset=10 on the
kernel cmd line if you really want the device bound by radeon to be the
primary video adapter, no matter what VGAARB say.
Cc: Alex Deucher
Cc: Christian Koenig
Signed-off-by: Sui Jingfeng
---
drivers/gpu/drm/radeon/radeon_device.c | 10 +-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/radeon/radeon_device.c
b/drivers/gpu/drm/radeon/radeon_device.c
index 71f2ff39d6a1..b661cd3a8dc2 100644
--- a/drivers/gpu/drm/radeon/radeon_device.c
+++ b/drivers/gpu/drm/radeon/radeon_device.c
@@ -1263,6 +1263,14 @@ static const struct vga_switcheroo_client_ops
radeon_switcheroo_ops = {
.can_switch = radeon_switcheroo_can_switch,
};
+static bool radeon_want_to_be_primary(struct pci_dev *pdev)
+{
+ if (radeon_modeset == 10)
+ return true;
+
+ return false;
+}
+
/**
* radeon_device_init - initialize the driver
*
@@ -1425,7 +1433,7 @@ int radeon_device_init(struct radeon_device *rdev,
/* if we have > 1 VGA cards, then disable the radeon VGA resources */
/* this will fail for cards that aren't VGA class devices, just
* ignore it */
- vga_client_register(rdev->pdev, radeon_vga_set_decode, NULL);
+ vga_client_register(rdev->pdev, radeon_vga_set_decode,
radeon_want_to_be_primary);
if (rdev->flags & RADEON_IS_PX)
runtime = true;
--
2.34.1
[Intel-gfx] [RFC, drm-misc-next v4 0/9] PCI/VGA: Allowing the user to select the primary video adapter at boot time
From: Sui Jingfeng On a machine with multiple GPUs, a Linux user has no control over which one is primary at boot time. This series tries to solve above mentioned problem by introduced the ->be_primary() function stub. The specific device drivers can provide an implementation to hook up with this stub by calling the vga_client_register() function. Once the driver bound the device successfully, VGAARB will call back to the device driver. To query if the device drivers want to be primary or not. Device drivers can just pass NULL if have no such needs. Please note that: 1) The ARM64, Loongarch, Mips servers have a lot PCIe slot, and I would like to mount at least three video cards. 2) Typically, those non-86 machines don't have a good UEFI firmware support, which doesn't support select primary GPU as firmware stage. Even on x86, there are old UEFI firmwares which already made undesired decision for you. 3) This series is attempt to solve the remain problems at the driver level, while another series[1] of me is target to solve the majority of the problems at device level. Tested (limited) on x86 with four video card mounted, Intel UHD Graphics 630 is the default boot VGA, successfully override by ast2400 with ast.modeset=10 append at the kernel cmd line. $ lspci | grep VGA 00:02.0 VGA compatible controller: Intel Corporation CoffeeLake-S GT2 [UHD Graphics 630] 01:00.0 VGA compatible controller: Advanced Micro Devices, Inc. [AMD/ATI] Caicos XTX [Radeon HD 8490 / R5 235X OEM] 04:00.0 VGA compatible controller: ASPEED Technology, Inc. ASPEED Graphics Family (rev 30) 05:00.0 VGA compatible controller: NVIDIA Corporation GK208B [GeForce GT 720] (rev a1) $ sudo dmesg | grep vgaarb pci :00:02.0: vgaarb: setting as boot VGA device pci :00:02.0: vgaarb: VGA device added: decodes=io+mem,owns=io+mem,locks=none pci :01:00.0: vgaarb: VGA device added: decodes=io+mem,owns=none,locks=none pci :04:00.0: vgaarb: VGA device added: decodes=io+mem,owns=none,locks=none pci :05:00.0: vgaarb: VGA device added: decodes=io+mem,owns=none,locks=none vgaarb: loaded ast :04:00.0: vgaarb: Override as primary by driver i915 :00:02.0: vgaarb: changed VGA decodes: olddecodes=io+mem,decodes=none:owns=io+mem radeon :01:00.0: vgaarb: changed VGA decodes: olddecodes=io+mem,decodes=none:owns=none ast :04:00.0: vgaarb: changed VGA decodes: olddecodes=io+mem,decodes=none:owns=none v2: * Add a simple implemment for drm/i915 and drm/ast * Pick up all tags (Mario) v3: * Fix a mistake for drm/i915 implement * Fix patch can not be applied problem because of merge conflect. v4: * Focus on solve the real problem. v1,v2 at https://patchwork.freedesktop.org/series/120059/ v3 at https://patchwork.freedesktop.org/series/120562/ [1] https://patchwork.freedesktop.org/series/122845/ Sui Jingfeng (9): PCI/VGA: Allowing the user to select the primary video adapter at boot time drm/nouveau: Implement .be_primary() callback drm/radeon: Implement .be_primary() callback drm/amdgpu: Implement .be_primary() callback drm/i915: Implement .be_primary() callback drm/loongson: Implement .be_primary() callback drm/ast: Register as a VGA client by calling vga_client_register() drm/hibmc: Register as a VGA client by calling vga_client_register() drm/gma500: Register as a VGA client by calling vga_client_register() drivers/gpu/drm/amd/amdgpu/amdgpu_device.c| 11 +++- drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 13 - drivers/gpu/drm/ast/ast_drv.c | 31 ++ drivers/gpu/drm/gma500/psb_drv.c | 57 ++- .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c | 15 + drivers/gpu/drm/i915/display/intel_vga.c | 15 - drivers/gpu/drm/loongson/loongson_module.c| 2 +- drivers/gpu/drm/loongson/loongson_module.h| 1 + drivers/gpu/drm/loongson/lsdc_drv.c | 10 +++- drivers/gpu/drm/nouveau/nouveau_vga.c | 11 +++- drivers/gpu/drm/radeon/radeon_device.c| 10 +++- drivers/pci/vgaarb.c | 43 -- drivers/vfio/pci/vfio_pci_core.c | 2 +- include/linux/vgaarb.h| 8 ++- 14 files changed, 210 insertions(+), 19 deletions(-) -- 2.34.1
[Intel-gfx] [RFC, drm-misc-next v4 1/9] PCI/VGA: Allowing the user to select the primary video adapter at boot time
From: Sui Jingfeng
On a machine with multiple GPUs, a Linux user has no control over which
one is primary at boot time. This series tries to solve above mentioned
problem by introduced the ->be_primary() function stub. The specific
device drivers can provide an implementation to hook up with this stub by
calling the vga_client_register() function.
Once the driver bound the device successfully, VGAARB will call back to
the device driver. To query if the device drivers want to be primary or
not. Device drivers can just pass NULL if have no such needs.
Acked-by: Jani Nikula # i915
Reviewed-by: Lyude Paul # nouveau
Signed-off-by: Sui Jingfeng
---
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 2 +-
drivers/gpu/drm/i915/display/intel_vga.c | 3 +-
drivers/gpu/drm/loongson/lsdc_drv.c| 2 +-
drivers/gpu/drm/nouveau/nouveau_vga.c | 2 +-
drivers/gpu/drm/radeon/radeon_device.c | 2 +-
drivers/pci/vgaarb.c | 43 +++---
drivers/vfio/pci/vfio_pci_core.c | 2 +-
include/linux/vgaarb.h | 8 ++--
8 files changed, 49 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index e77f048c99d8..ecc4564ceac0 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -3916,7 +3916,7 @@ int amdgpu_device_init(struct amdgpu_device *adev,
* ignore it
*/
if ((adev->pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA)
- vga_client_register(adev->pdev, amdgpu_device_vga_set_decode);
+ vga_client_register(adev->pdev, amdgpu_device_vga_set_decode,
NULL);
px = amdgpu_device_supports_px(ddev);
diff --git a/drivers/gpu/drm/i915/display/intel_vga.c
b/drivers/gpu/drm/i915/display/intel_vga.c
index 286a0bdd28c6..98d7d4dffe9f 100644
--- a/drivers/gpu/drm/i915/display/intel_vga.c
+++ b/drivers/gpu/drm/i915/display/intel_vga.c
@@ -115,7 +115,6 @@ intel_vga_set_decode(struct pci_dev *pdev, bool
enable_decode)
int intel_vga_register(struct drm_i915_private *i915)
{
-
struct pci_dev *pdev = to_pci_dev(i915->drm.dev);
int ret;
@@ -127,7 +126,7 @@ int intel_vga_register(struct drm_i915_private *i915)
* then we do not take part in VGA arbitration and the
* vga_client_register() fails with -ENODEV.
*/
- ret = vga_client_register(pdev, intel_vga_set_decode);
+ ret = vga_client_register(pdev, intel_vga_set_decode, NULL);
if (ret && ret != -ENODEV)
return ret;
diff --git a/drivers/gpu/drm/loongson/lsdc_drv.c
b/drivers/gpu/drm/loongson/lsdc_drv.c
index 188ec82afcfb..d10a28c2c494 100644
--- a/drivers/gpu/drm/loongson/lsdc_drv.c
+++ b/drivers/gpu/drm/loongson/lsdc_drv.c
@@ -289,7 +289,7 @@ static int lsdc_pci_probe(struct pci_dev *pdev, const
struct pci_device_id *ent)
pci_set_drvdata(pdev, ddev);
- vga_client_register(pdev, lsdc_vga_set_decode);
+ vga_client_register(pdev, lsdc_vga_set_decode, NULL);
drm_kms_helper_poll_init(ddev);
diff --git a/drivers/gpu/drm/nouveau/nouveau_vga.c
b/drivers/gpu/drm/nouveau/nouveau_vga.c
index f8bf0ec26844..162b4f4676c7 100644
--- a/drivers/gpu/drm/nouveau/nouveau_vga.c
+++ b/drivers/gpu/drm/nouveau/nouveau_vga.c
@@ -92,7 +92,7 @@ nouveau_vga_init(struct nouveau_drm *drm)
return;
pdev = to_pci_dev(dev->dev);
- vga_client_register(pdev, nouveau_vga_set_decode);
+ vga_client_register(pdev, nouveau_vga_set_decode, NULL);
/* don't register Thunderbolt eGPU with vga_switcheroo */
if (pci_is_thunderbolt_attached(pdev))
diff --git a/drivers/gpu/drm/radeon/radeon_device.c
b/drivers/gpu/drm/radeon/radeon_device.c
index afbb3a80c0c6..71f2ff39d6a1 100644
--- a/drivers/gpu/drm/radeon/radeon_device.c
+++ b/drivers/gpu/drm/radeon/radeon_device.c
@@ -1425,7 +1425,7 @@ int radeon_device_init(struct radeon_device *rdev,
/* if we have > 1 VGA cards, then disable the radeon VGA resources */
/* this will fail for cards that aren't VGA class devices, just
* ignore it */
- vga_client_register(rdev->pdev, radeon_vga_set_decode);
+ vga_client_register(rdev->pdev, radeon_vga_set_decode, NULL);
if (rdev->flags & RADEON_IS_PX)
runtime = true;
diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
index 5a696078b382..552ac7df10ee 100644
--- a/drivers/pci/vgaarb.c
+++ b/drivers/pci/vgaarb.c
@@ -53,6 +53,7 @@ struct vga_device {
bool bridge_has_one_vga;
bool is_firmware_default; /* device selected by firmware */
unsigned int (*set_decode)(struct pci_dev *pdev, bool decode);
+ bool (*be_primary)(struct pci_dev *pdev);
};
static LIST_HEAD(vga_list);
@@ -956,6 +957,10 @@ EXPORT_SYMBOL(vga_set_legacy_decoding);
* @set_decode callback: If a client can disable its GPU VGA resource, it
*
[Intel-gfx] [RFC, drm-misc-next v4 4/9] drm/amdgpu: Implement .be_primary() callback
From: Sui Jingfeng
On a machine with multiple GPUs, a Linux user has no control over which one
is primary at boot time. This patch tries to solve the mentioned problem by
implementing the .be_primary() callback. Pass amdgpu.modeset=10 on the
kernel cmd line if you really want the device bound by amdgpu drm driver to
be the primary video adapter, no matter what VGAARB say.
Cc: Alex Deucher
Cc: Christian Konig
Signed-off-by: Sui Jingfeng
---
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 11 ++-
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c| 13 -
2 files changed, 22 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index ecc4564ceac0..59bde6972a8b 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -3507,6 +3507,14 @@ static void amdgpu_device_set_mcbp(struct amdgpu_device
*adev)
DRM_INFO("MCBP is enabled\n");
}
+static bool amdgpu_want_to_be_primary(struct pci_dev *pdev)
+{
+ if (amdgpu_modeset == 10)
+ return true;
+
+ return false;
+}
+
/**
* amdgpu_device_init - initialize the driver
*
@@ -3916,7 +3924,8 @@ int amdgpu_device_init(struct amdgpu_device *adev,
* ignore it
*/
if ((adev->pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA)
- vga_client_register(adev->pdev, amdgpu_device_vga_set_decode,
NULL);
+ vga_client_register(adev->pdev, amdgpu_device_vga_set_decode,
+ amdgpu_want_to_be_primary);
px = amdgpu_device_supports_px(ddev);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index 81edf66dbea8..2592e24ce62c 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -118,6 +118,7 @@
#define KMS_DRIVER_MINOR 54
#define KMS_DRIVER_PATCHLEVEL 0
+int amdgpu_modeset = -1;
unsigned int amdgpu_vram_limit = UINT_MAX;
int amdgpu_vis_vram_limit;
int amdgpu_gart_size = -1; /* auto */
@@ -223,6 +224,13 @@ struct amdgpu_watchdog_timer amdgpu_watchdog_timer = {
.period = 0x0, /* default to 0x0 (timeout disable) */
};
+/**
+ * DOC: modeset (int)
+ * Disable/Enable kernel modesetting (1 = enable, 0 = disable, -1 = auto
(default)).
+ */
+MODULE_PARM_DESC(modeset, "Disable/Enable kernel modesetting");
+module_param_named(modeset, amdgpu_modeset, int, 0600);
+
/**
* DOC: vramlimit (int)
* Restrict the total amount of VRAM in MiB for testing. The default is 0
(Use full VRAM).
@@ -2872,7 +2880,10 @@ static int __init amdgpu_init(void)
{
int r;
- if (drm_firmware_drivers_only())
+ if (drm_firmware_drivers_only() && amdgpu_modeset == -1)
+ return -EINVAL;
+
+ if (amdgpu_modeset == 0)
return -EINVAL;
r = amdgpu_sync_init();
--
2.34.1
[Intel-gfx] [RFC, drm-misc-next v4 6/9] drm/loongson: Implement .be_primary() callback
From: Sui Jingfeng
On a machine with multiple GPUs, a Linux user has no control over which one
is primary at boot time. This patch tries to solve the mentioned problem by
implementing the .be_primary() callback. Pass loongson.modeset=10 on the
kernel cmd line if you really want the device bound by loongson drm driver
to be the primary video adapter, no matter what VGAARB say.
Signed-off-by: Sui Jingfeng
---
drivers/gpu/drm/loongson/loongson_module.c | 2 +-
drivers/gpu/drm/loongson/loongson_module.h | 1 +
drivers/gpu/drm/loongson/lsdc_drv.c| 10 +-
3 files changed, 11 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/loongson/loongson_module.c
b/drivers/gpu/drm/loongson/loongson_module.c
index d2a51bd395f6..12f2a453adff 100644
--- a/drivers/gpu/drm/loongson/loongson_module.c
+++ b/drivers/gpu/drm/loongson/loongson_module.c
@@ -9,7 +9,7 @@
#include "loongson_module.h"
-static int loongson_modeset = -1;
+int loongson_modeset = -1;
MODULE_PARM_DESC(modeset, "Disable/Enable modesetting");
module_param_named(modeset, loongson_modeset, int, 0400);
diff --git a/drivers/gpu/drm/loongson/loongson_module.h
b/drivers/gpu/drm/loongson/loongson_module.h
index 931c17521bf0..afff51e7f34f 100644
--- a/drivers/gpu/drm/loongson/loongson_module.h
+++ b/drivers/gpu/drm/loongson/loongson_module.h
@@ -6,6 +6,7 @@
#ifndef __LOONGSON_MODULE_H__
#define __LOONGSON_MODULE_H__
+extern int loongson_modeset;
extern int loongson_vblank;
extern struct pci_driver lsdc_pci_driver;
diff --git a/drivers/gpu/drm/loongson/lsdc_drv.c
b/drivers/gpu/drm/loongson/lsdc_drv.c
index d10a28c2c494..7183b0666167 100644
--- a/drivers/gpu/drm/loongson/lsdc_drv.c
+++ b/drivers/gpu/drm/loongson/lsdc_drv.c
@@ -257,6 +257,14 @@ static unsigned int lsdc_vga_set_decode(struct pci_dev
*pdev, bool state)
return VGA_RSRC_NORMAL_IO | VGA_RSRC_NORMAL_MEM;
}
+static bool lsdc_want_to_be_primary(struct pci_dev *pdev)
+{
+ if (loongson_modeset == 10)
+ return true;
+
+ return false;
+}
+
static int lsdc_pci_probe(struct pci_dev *pdev, const struct pci_device_id
*ent)
{
const struct lsdc_desc *descp;
@@ -289,7 +297,7 @@ static int lsdc_pci_probe(struct pci_dev *pdev, const
struct pci_device_id *ent)
pci_set_drvdata(pdev, ddev);
- vga_client_register(pdev, lsdc_vga_set_decode, NULL);
+ vga_client_register(pdev, lsdc_vga_set_decode, lsdc_want_to_be_primary);
drm_kms_helper_poll_init(ddev);
--
2.34.1
[Intel-gfx] [RFC, drm-misc-next v4 5/9] drm/i915: Implement .be_primary() callback
From: Sui Jingfeng
On a machine with multiple GPUs, a Linux user has no control over which one
is primary at boot time. This patch tries to solve the mentioned problem by
implementing the .be_primary() callback. Pass i915.modeset=10 on the kernel
cmd line if you really want the device bound by i915 drm driver to be the
primary video adapter, no matter what VGAARB say.
Cc: Jani Nikula
Cc: David Airlie
Cc: Daniel Vetter
Signed-off-by: Sui Jingfeng
---
drivers/gpu/drm/i915/display/intel_vga.c | 14 +-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/display/intel_vga.c
b/drivers/gpu/drm/i915/display/intel_vga.c
index 98d7d4dffe9f..e3f78ba2668b 100644
--- a/drivers/gpu/drm/i915/display/intel_vga.c
+++ b/drivers/gpu/drm/i915/display/intel_vga.c
@@ -113,6 +113,17 @@ intel_vga_set_decode(struct pci_dev *pdev, bool
enable_decode)
return VGA_RSRC_NORMAL_IO | VGA_RSRC_NORMAL_MEM;
}
+static bool intel_want_to_be_primary(struct pci_dev *pdev)
+{
+ struct drm_i915_private *i915 = pdev_to_i915(pdev);
+ struct i915_params *params = >params;
+
+ if (params->modeset == 10)
+ return true;
+
+ return false;
+}
+
int intel_vga_register(struct drm_i915_private *i915)
{
struct pci_dev *pdev = to_pci_dev(i915->drm.dev);
@@ -126,7 +137,8 @@ int intel_vga_register(struct drm_i915_private *i915)
* then we do not take part in VGA arbitration and the
* vga_client_register() fails with -ENODEV.
*/
- ret = vga_client_register(pdev, intel_vga_set_decode, NULL);
+ ret = vga_client_register(pdev, intel_vga_set_decode,
+ intel_want_to_be_primary);
if (ret && ret != -ENODEV)
return ret;
--
2.34.1
[Intel-gfx] [RFC, drm-misc-next v4 8/9] drm/hibmc: Register as a VGA client by calling vga_client_register()
From: Sui Jingfeng
Because the display controller in the Hibmc chip is a VGA compatible
display controller. Because ARM64 doesn't need the VGA console. It does not
need to worry about the side effects that come with the VGA compatible.
However, the real problem is that some ARM64 PCs and servers do not have
good UEFI firmware support. At least, it is not as good as UEFI firmware
for x86. The Huawei KunPeng 920 PC and Taishan 100 server are examples.
When a discrete GPU is mounted on such machines, the UEFI firmware still
selects the integrated display controller (in the BMC) as the primary GPU.
It is hardcoded, no options are provided for selection. A Linux user has
no control at all.
Signed-off-by: Sui Jingfeng
---
drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c | 15 +++
1 file changed, 15 insertions(+)
diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
index 8a98fa276e8a..73a3f1cb109a 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
@@ -13,6 +13,7 @@
#include
#include
+#include
#include
#include
@@ -27,6 +28,10 @@
#include "hibmc_drm_drv.h"
#include "hibmc_drm_regs.h"
+static int hibmc_modeset = -1;
+MODULE_PARM_DESC(modeset, "Disable/Enable modesetting");
+module_param_named(modeset, hibmc_modeset, int, 0400);
+
DEFINE_DRM_GEM_FOPS(hibmc_fops);
static irqreturn_t hibmc_interrupt(int irq, void *arg)
@@ -299,6 +304,14 @@ static int hibmc_load(struct drm_device *dev)
return ret;
}
+static bool hibmc_want_to_be_primary(struct pci_dev *pdev)
+{
+ if (hibmc_modeset == 10)
+ return true;
+
+ return false;
+}
+
static int hibmc_pci_probe(struct pci_dev *pdev,
const struct pci_device_id *ent)
{
@@ -339,6 +352,8 @@ static int hibmc_pci_probe(struct pci_dev *pdev,
goto err_unload;
}
+ vga_client_register(pdev, NULL, hibmc_want_to_be_primary);
+
drm_fbdev_generic_setup(dev, 32);
return 0;
--
2.34.1
[Intel-gfx] [RFC, drm-misc-next v4 2/9] drm/nouveau: Implement .be_primary() callback
From: Sui Jingfeng
On a machine with multiple GPUs, a Linux user has no control over which one
is primary at boot time. This patch tries to solve the mentioned problem by
implementing the .be_primary() callback. VGAARB will call back to Nouveau
when the drm/nouveau gets loaded successfully.
Pass nouveau.modeset=10 on the kernel cmd line if you really want the
device bound by Nouveau to be the primary video adapter. This overrides
whatever boot device selected by VGAARB.
Signed-off-by: Sui Jingfeng
---
drivers/gpu/drm/nouveau/nouveau_vga.c | 11 ++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/nouveau/nouveau_vga.c
b/drivers/gpu/drm/nouveau/nouveau_vga.c
index 162b4f4676c7..4242188667e2 100644
--- a/drivers/gpu/drm/nouveau/nouveau_vga.c
+++ b/drivers/gpu/drm/nouveau/nouveau_vga.c
@@ -80,6 +80,15 @@ nouveau_switcheroo_ops = {
.can_switch = nouveau_switcheroo_can_switch,
};
+static bool
+nouveau_want_to_be_primary(struct pci_dev *pdev)
+{
+ if (nouveau_modeset == 10)
+ return true;
+
+ return false;
+}
+
void
nouveau_vga_init(struct nouveau_drm *drm)
{
@@ -92,7 +101,7 @@ nouveau_vga_init(struct nouveau_drm *drm)
return;
pdev = to_pci_dev(dev->dev);
- vga_client_register(pdev, nouveau_vga_set_decode, NULL);
+ vga_client_register(pdev, nouveau_vga_set_decode,
nouveau_want_to_be_primary);
/* don't register Thunderbolt eGPU with vga_switcheroo */
if (pci_is_thunderbolt_attached(pdev))
--
2.34.1
[Intel-gfx] [RFC, drm-misc-next v4 9/9] drm/gma500: Register as a VGA client by calling vga_client_register()
From: Sui Jingfeng
Because the display controller in N2000/D2000 series can be VGA-compatible,
so let's register gma500 as a VGA client, despite the firmware may alter
the PCI class code of IGD on a multiple GPU co-exist configuration. But
this commit no crime, because VGAARB only cares about VGA devices.
Noticed that the display controller in N2000/D2000 processor don't has a
valid VRAM BAR, the firmware put the EFI firmware framebuffer into the
stolen memory, so the commit <86fd887b7fe3> ("vgaarb: Don't default
exclusively to first video device with mem+io") is not effictive on such
a case. But the benefits of the stolen memory is that it will not suffer
from PCI resource relocation. Becase the stolen memory is carved out by
the firmware and reside in system RAM. Therefore, while at it, provided a
naive version of firmware framebuffer identification function and use the
new machanism just created.
Signed-off-by: Sui Jingfeng
---
drivers/gpu/drm/gma500/psb_drv.c | 57 ++--
1 file changed, 55 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/gma500/psb_drv.c b/drivers/gpu/drm/gma500/psb_drv.c
index 8b64f61ffaf9..eb95d030d981 100644
--- a/drivers/gpu/drm/gma500/psb_drv.c
+++ b/drivers/gpu/drm/gma500/psb_drv.c
@@ -14,7 +14,7 @@
#include
#include
#include
-
+#include
#include
#include
@@ -36,6 +36,11 @@
#include "psb_irq.h"
#include "psb_reg.h"
+static int gma500_modeset = -1;
+
+MODULE_PARM_DESC(modeset, "Disable/Enable modesetting");
+module_param_named(modeset, gma500_modeset, int, 0400);
+
static const struct drm_driver driver;
static int psb_pci_probe(struct pci_dev *pdev, const struct pci_device_id
*ent);
@@ -446,6 +451,49 @@ static int gma_remove_conflicting_framebuffers(struct
pci_dev *pdev,
return __aperture_remove_legacy_vga_devices(pdev);
}
+static bool gma_contain_firmware_fb(u64 ap_start, u64 ap_end)
+{
+ u64 fb_start;
+ u64 fb_size;
+ u64 fb_end;
+
+ if (screen_info.capabilities & VIDEO_CAPABILITY_64BIT_BASE)
+ fb_start = (u64)screen_info.ext_lfb_base << 32 |
screen_info.lfb_base;
+ else
+ fb_start = screen_info.lfb_base;
+
+ fb_size = screen_info.lfb_size;
+ fb_end = fb_start + fb_size - 1;
+
+ /* No firmware framebuffer support */
+ if (!fb_start || !fb_size)
+ return false;
+
+ if (fb_start >= ap_start && fb_end <= ap_end)
+ return true;
+
+ return false;
+}
+
+static bool gma_want_to_be_primary(struct pci_dev *pdev)
+{
+ struct drm_device *drm = pci_get_drvdata(pdev);
+ struct drm_psb_private *priv = to_drm_psb_private(drm);
+ u64 vram_base = priv->stolen_base;
+ u64 vram_size = priv->vram_stolen_size;
+
+ if (gma500_modeset == 10)
+ return true;
+
+ /* Stolen memory are not going to be moved */
+ if (gma_contain_firmware_fb(vram_base, vram_base + vram_size)) {
+ drm_dbg(drm, "Contains firmware FB in the stolen memory\n");
+ return true;
+ }
+
+ return false;
+}
+
static int psb_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
{
struct drm_psb_private *dev_priv;
@@ -475,6 +523,8 @@ static int psb_pci_probe(struct pci_dev *pdev, const struct
pci_device_id *ent)
if (ret)
return ret;
+ vga_client_register(pdev, NULL, gma_want_to_be_primary);
+
psb_fbdev_setup(dev_priv);
return 0;
@@ -526,7 +576,10 @@ static struct pci_driver psb_pci_driver = {
static int __init psb_init(void)
{
- if (drm_firmware_drivers_only())
+ if (drm_firmware_drivers_only() && (gma500_modeset == -1))
+ return -ENODEV;
+
+ if (!gma500_modeset)
return -ENODEV;
return pci_register_driver(_pci_driver);
--
2.34.1
[Intel-gfx] ✗ Fi.CI.IGT: failure for fbc on any planes (rev2)
== Series Details ==
Series: fbc on any planes (rev2)
URL : https://patchwork.freedesktop.org/series/123180/
State : failure
== Summary ==
CI Bug Log - changes from CI_DRM_13593_full -> Patchwork_123180v2_full
Summary
---
**FAILURE**
Serious unknown changes coming with Patchwork_123180v2_full absolutely need
to be
verified manually.
If you think the reported changes have nothing to do with the changes
introduced in Patchwork_123180v2_full, please notify your bug team to allow
them
to document this new failure mode, which will reduce false positives in CI.
Participating hosts (9 -> 10)
--
Additional (1): shard-rkl0
Possible new issues
---
Here are the unknown changes that may have been introduced in
Patchwork_123180v2_full:
### IGT changes ###
Possible regressions
* igt@kms_rotation_crc@multiplane-rotation-cropping-bottom:
- shard-rkl: [PASS][1] -> [INCOMPLETE][2]
[1]:
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13593/shard-rkl-2/igt@kms_rotation_...@multiplane-rotation-cropping-bottom.html
[2]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_123180v2/shard-rkl-4/igt@kms_rotation_...@multiplane-rotation-cropping-bottom.html
Suppressed
The following results come from untrusted machines, tests, or statuses.
They do not affect the overall result.
* {igt@kms_dirtyfb@dirtyfb-ioctl@psr-dp-4}:
- shard-dg2: NOTRUN -> [SKIP][3] +1 similar issue
[3]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_123180v2/shard-dg2-11/igt@kms_dirtyfb@dirtyfb-io...@psr-dp-4.html
Known issues
Here are the changes found in Patchwork_123180v2_full that come from known
issues:
### IGT changes ###
Issues hit
* igt@api_intel_bb@blit-reloc-purge-cache:
- shard-dg2: NOTRUN -> [SKIP][4] ([i915#8411]) +1 similar issue
[4]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_123180v2/shard-dg2-3/igt@api_intel...@blit-reloc-purge-cache.html
* igt@api_intel_bb@render-ccs:
- shard-dg2: NOTRUN -> [FAIL][5] ([i915#6122])
[5]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_123180v2/shard-dg2-3/igt@api_intel...@render-ccs.html
* igt@drm_buddy@drm_buddy_test:
- shard-snb: NOTRUN -> [SKIP][6] ([fdo#109271] / [i915#8661])
[6]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_123180v2/shard-snb4/igt@drm_buddy@drm_buddy_test.html
- shard-dg2: NOTRUN -> [SKIP][7] ([i915#8661])
[7]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_123180v2/shard-dg2-2/igt@drm_buddy@drm_buddy_test.html
* igt@drm_fdinfo@virtual-busy-idle-all:
- shard-dg2: NOTRUN -> [SKIP][8] ([i915#8414])
[8]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_123180v2/shard-dg2-2/igt@drm_fdi...@virtual-busy-idle-all.html
* igt@gem_ccs@suspend-resume@tile64-compressed-compfmt0-lmem0-lmem0:
- shard-dg2: [PASS][9] -> [INCOMPLETE][10] ([i915#6311] /
[i915#7297])
[9]:
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13593/shard-dg2-6/igt@gem_ccs@suspend-res...@tile64-compressed-compfmt0-lmem0-lmem0.html
[10]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_123180v2/shard-dg2-1/igt@gem_ccs@suspend-res...@tile64-compressed-compfmt0-lmem0-lmem0.html
* igt@gem_close_race@multigpu-basic-process:
- shard-dg2: NOTRUN -> [SKIP][11] ([i915#7697])
[11]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_123180v2/shard-dg2-2/igt@gem_close_r...@multigpu-basic-process.html
* igt@gem_ctx_persistence@hang:
- shard-dg2: NOTRUN -> [SKIP][12] ([i915#8555])
[12]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_123180v2/shard-dg2-3/igt@gem_ctx_persiste...@hang.html
* igt@gem_ctx_persistence@legacy-engines-mixed:
- shard-snb: NOTRUN -> [SKIP][13] ([fdo#109271] / [i915#1099])
[13]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_123180v2/shard-snb4/igt@gem_ctx_persiste...@legacy-engines-mixed.html
* igt@gem_eio@in-flight-contexts-10ms:
- shard-apl: [PASS][14] -> [TIMEOUT][15] ([i915#3063])
[14]:
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13593/shard-apl3/igt@gem_...@in-flight-contexts-10ms.html
[15]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_123180v2/shard-apl1/igt@gem_...@in-flight-contexts-10ms.html
* igt@gem_eio@unwedge-stress:
- shard-dg1: [PASS][16] -> [FAIL][17] ([i915#5784]) +1 similar
issue
[16]:
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13593/shard-dg1-16/igt@gem_...@unwedge-stress.html
[17]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_123180v2/shard-dg1-17/igt@gem_...@unwedge-stress.html
* igt@gem_exec_capture@pi@vecs0:
- shard-mtlp: [PASS][18] -> [FAIL][19] ([i915#4475] / [i915#7765])
[18]:
[Intel-gfx] ✓ Fi.CI.BAT: success for fbc on any planes (rev2)
== Series Details == Series: fbc on any planes (rev2) URL : https://patchwork.freedesktop.org/series/123180/ State : success == Summary == CI Bug Log - changes from CI_DRM_13593 -> Patchwork_123180v2 Summary --- **SUCCESS** No regressions found. External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_123180v2/index.html Participating hosts (40 -> 38) -- Missing(2): fi-kbl-soraka fi-snb-2520m Known issues Here are the changes found in Patchwork_123180v2 that come from known issues: ### IGT changes ### Issues hit * igt@i915_pm_backlight@basic-brightness: - bat-adlp-11:NOTRUN -> [ABORT][1] ([i915#8668]) [1]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_123180v2/bat-adlp-11/igt@i915_pm_backli...@basic-brightness.html * igt@kms_pipe_crc_basic@read-crc-frame-sequence@pipe-d-edp-1: - bat-rplp-1: [PASS][2] -> [ABORT][3] ([i915#8442] / [i915#8668]) [2]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13593/bat-rplp-1/igt@kms_pipe_crc_basic@read-crc-frame-seque...@pipe-d-edp-1.html [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_123180v2/bat-rplp-1/igt@kms_pipe_crc_basic@read-crc-frame-seque...@pipe-d-edp-1.html Possible fixes * igt@gem_exec_suspend@basic-s0@lmem0: - bat-dg2-9: [INCOMPLETE][4] ([i915#6311]) -> [PASS][5] [4]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13593/bat-dg2-9/igt@gem_exec_suspend@basic...@lmem0.html [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_123180v2/bat-dg2-9/igt@gem_exec_suspend@basic...@lmem0.html Warnings * igt@kms_setmode@basic-clone-single-crtc: - bat-adlp-11:[ABORT][6] ([i915#8260] / [i915#8668]) -> [SKIP][7] ([i915#3555]) [6]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13593/bat-adlp-11/igt@kms_setm...@basic-clone-single-crtc.html [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_123180v2/bat-adlp-11/igt@kms_setm...@basic-clone-single-crtc.html [i915#3555]: https://gitlab.freedesktop.org/drm/intel/issues/3555 [i915#6311]: https://gitlab.freedesktop.org/drm/intel/issues/6311 [i915#8260]: https://gitlab.freedesktop.org/drm/intel/issues/8260 [i915#8442]: https://gitlab.freedesktop.org/drm/intel/issues/8442 [i915#8668]: https://gitlab.freedesktop.org/drm/intel/issues/8668 Build changes - * Linux: CI_DRM_13593 -> Patchwork_123180v2 CI-20190529: 20190529 CI_DRM_13593: 70c5bfd28eab769b048876075fc3561c3f04a54a @ git://anongit.freedesktop.org/gfx-ci/linux IGT_7466: 7466 Patchwork_123180v2: 70c5bfd28eab769b048876075fc3561c3f04a54a @ git://anongit.freedesktop.org/gfx-ci/linux ### Linux commits b2038e767f51 drm/i915/lnl: FBC is supported with per pixel alpha bd8308ec9759 drm/i915/lnl: possibility to enable FBC on first three planes == Logs == For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_123180v2/index.html
Re: [Intel-gfx] [RFC 00/33] Add Support for Plane Color Pipeline
> -Original Message- > From: Sebastian Wick > Sent: Thursday, August 31, 2023 2:46 AM > To: Shankar, Uma > Cc: Harry Wentland ; intel- > g...@lists.freedesktop.org; dri-de...@lists.freedesktop.org; wayland- > de...@lists.freedesktop.org; Ville Syrjala ; > Pekka > Paalanen ; Simon Ser ; > Melissa Wen ; Jonas Ådahl ; Shashank > Sharma ; Alexander Goins ; > Naseer Ahmed ; Christopher Braga > > Subject: Re: [RFC 00/33] Add Support for Plane Color Pipeline > > On Wed, Aug 30, 2023 at 08:47:37AM +, Shankar, Uma wrote: > > > > > > > -Original Message- > > > From: Harry Wentland > > > Sent: Wednesday, August 30, 2023 12:56 AM > > > To: Shankar, Uma ; > > > intel-gfx@lists.freedesktop.org; dri- de...@lists.freedesktop.org > > > Cc: wayland-de...@lists.freedesktop.org; Ville Syrjala > > > ; Pekka Paalanen > > > ; Simon Ser ; > > > Melissa Wen ; Jonas Ådahl ; > > > Sebastian Wick ; Shashank Sharma > > > ; Alexander Goins ; > > > Naseer Ahmed ; Christopher Braga > > > > > > Subject: Re: [RFC 00/33] Add Support for Plane Color Pipeline > > > > > > +CC Naseer and Chris, FYI > > > > > > See https://patchwork.freedesktop.org/series/123024/ for whole series. > > > > > > On 2023-08-29 12:03, Uma Shankar wrote: > > > > Introduction > > > > > > > > > > > > Modern hardwares have various color processing capabilities both > > > > at pre-blending and post-blending phases in the color pipeline. > > > > The current drm implementation exposes only the post-blending > > > > color hardware blocks. Support for pre-blending hardware is missing. > > > > There are multiple use cases where pre-blending color hardware > > > > will be > > > > useful: > > > > a) Linearization of input buffers encoded in various transfer > > > >functions. > > > > b) Color Space conversion > > > > c) Tone mapping > > > > d) Frame buffer format conversion > > > > e) Non-linearization of buffer(apply transfer function) > > > > f) 3D Luts > > > > > > > > and other miscellaneous color operations. > > > > > > > > Hence, there is a need to expose the color capabilities of the > > > > hardware to user-space. This will help userspace/middleware to use > > > > display hardware for color processing and blending instead of > > > > doing it through GPU shaders. > > > > > > > > > > Thanks, Uma, for sending this. I've been working on something > > > similar but you beat me to it. :) > > > > Thanks Harry for the useful feedback and overall collaboration on this so > > far. > > > > > > > > > > Work done so far and relevant references > > > > > > > > > > > > Some implementation is done by Intel and AMD/Igalia to address the same. > > > > Broad consensus is there that we need a generic API at drm core to > > > > suffice the use case of various HW vendors. Below are the links > > > > capturing the discussion so far. > > > > > > > > Intel's Plane Color Implementation: > > > > https://patchwork.freedesktop.org/series/90825/ > > > > AMD's Plane Color Implementation: > > > > https://patchwork.freedesktop.org/series/116862/ > > > > > > > > > > > > Hackfest conclusions > > > > > > > > > > > > HDR/Color Hackfest was organised by Redhat to bring all the > > > > industry stakeholders together and converge on a common uapi > expectations. > > > > Participants from Intel, AMD, Nvidia, Collabora, Redhat, Igalia > > > > and other prominent user-space developers and maintainers. > > > > > > > > Discussions happened on the uapi expectations, opens, nature of > > > > hardware of multiple hardware vendors, challenges in generalizing > > > > the same and the path forward. Consensus was made that drm core > > > > should implement descriptive APIs and not go with prescriptive > > > > APIs. DRM core should just expose the hardware capabilities; > > > > enabling, customizing and programming the same should be done by > > > > the user-space. Driver should just > > > honor the user space request without doing any operations internally. > > > > > > > > Thanks to Simon Ser, for nicely documenting the design consensus > > > > and an UAPI RFC which can be referred to here: > > > > > > > > https://lore.kernel.org/dri-devel/QMers3awXvNCQlyhWdTtsPwkp5ie9bze > > > > _hD5 > > > > > > > > nAccFW7a_RXlWjYB7MoUW_8CKLT2bSQwIXVi5H6VULYIxCdgvryZoAoJnC5lZgyK1 > Q > > > Wn48 > > > > 8=@emersion.fr/ > > > > > > > > > > > > Design considerations > > > > = > > > > > > > > Following are the important aspects taken into account while > > > > designing the current RFC > > > > proposal: > > > > > > > > 1. Individual HW blocks can be muxed. (e.g. out of two HW blocks > > > > only one > > > can be used) > > > > 2. Position of the HW block in the pipeline can be programmable > > > > 3. LUTs can be one dimentional or three dimentional > > > > 4. Number of LUT entries can vary across platforms > > > > 5.
[Intel-gfx] ✗ Fi.CI.SPARSE: warning for fbc on any planes (rev2)
== Series Details == Series: fbc on any planes (rev2) URL : https://patchwork.freedesktop.org/series/123180/ State : warning == Summary == Error: dim sparse failed Sparse version: v0.6.2 Fast mode used, each commit won't be checked separately. +./arch/x86/include/asm/bitops.h:117:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:117:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:117:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:117:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:117:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:117:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:117:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:117:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:117:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:117:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:117:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:117:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:117:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:117:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:117:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:117:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:117:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:117:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:117:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:117:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:117:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:117:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:117:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:117:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:117:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:117:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:117:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:117:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:117:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:117:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:117:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:117:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:117:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:117:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:117:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:117:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:117:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:117:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:117:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:117:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:117:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:117:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:117:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:117:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:117:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:117:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:117:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:117:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:117:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:117:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:117:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:117:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:117:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:117:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:117:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:117:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:117:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:117:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:117:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:117:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:117:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:117:1: warning: unreplaced symbol 'return'
[Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for fbc on any planes (rev2)
== Series Details == Series: fbc on any planes (rev2) URL : https://patchwork.freedesktop.org/series/123180/ State : warning == Summary == Error: dim checkpatch failed e9836d242d2b drm/i915/lnl: possibility to enable FBC on first three planes -:74: WARNING:LONG_LINE: line length of 103 exceeds 100 columns #74: FILE: drivers/gpu/drm/i915/i915_reg.h:1331: +#define DPFC_CTL_PLANE_BINDING(plane_id) REG_FIELD_PREP(DPFC_CTL_PLANE_BINDING_MASK, (plane_id)) total: 0 errors, 1 warnings, 0 checks, 36 lines checked 460751b3aad5 drm/i915/lnl: FBC is supported with per pixel alpha
Re: [Intel-gfx] [RFC 02/33] drm: Add color operation structure
> -Original Message-
> From: dri-devel On Behalf Of Pekka
> Paalanen
> Sent: Wednesday, August 30, 2023 6:30 PM
> To: Shankar, Uma
> Cc: intel-gfx@lists.freedesktop.org; Borah, Chaitanya Kumar
> ; dri-de...@lists.freedesktop.org; wayland-
> de...@lists.freedesktop.org
> Subject: Re: [RFC 02/33] drm: Add color operation structure
>
> On Tue, 29 Aug 2023 21:33:51 +0530
> Uma Shankar wrote:
>
> > From: Chaitanya Kumar Borah
> >
> > Each Color Hardware block will be represented uniquely in the color
> > pipeline. Define the structure to represent the same.
> >
> > These color operations will form the building blocks of a color
> > pipeline which best represents the underlying Hardware. Color
> > operations can be re-arranged, substracted or added to create distinct
> > color pipelines to accurately describe the Hardware blocks present in
> > the display engine.
> >
> > Co-developed-by: Uma Shankar
> > Signed-off-by: Uma Shankar
> > Signed-off-by: Chaitanya Kumar Borah
> > ---
> > include/uapi/drm/drm_mode.h | 72
> > +
> > 1 file changed, 72 insertions(+)
> >
> > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> > index ea1b639bcb28..882479f41745 100644
> > --- a/include/uapi/drm/drm_mode.h
> > +++ b/include/uapi/drm/drm_mode.h
> > @@ -943,6 +943,78 @@ struct hdr_output_metadata {
> > };
> > };
> >
> > +/**
> > + * enum color_op_block
> > + *
> > + * Enums to identify hardware color blocks.
> > + *
> > + * @DRM_CB_PRE_CSC: LUT before the CTM unit
> > + * @DRM_CB_CSC: CTM hardware supporting 3x3 matrix
> > + * @DRM_CB_POST_CSC: LUT after the CTM unit
> > + * @DRM_CB_3D_LUT: LUT hardware with coefficients for all
> > + * color components
> > + * @DRM_CB_PRIVATE: Vendor specific hardware unit. Vendor
> > + * can expose a custom hardware by defining a
> > + * color operation block with this name as
> > + * identifier
>
> This naming scheme does not seem to work. It assumes a far too rigid pipeline,
> just like the old KMS property design. What if you have two other operations
> between PRE_CSC and CSC?
>
> What sense do PRE_CSC and POST_CSC make if you don't happen to have a CSC
> operation?
Sure, we can re-look at the naming. However, it will be good to define some
standard
operations common to all vendors and keep the rest as vendor private.
> What if a driver put POST_CSC before PRE_CSC in its pipeline?
>
> What if your CSC is actually a series of three independent operations, and in
> addition you have PRE_CSC and POST_CSC?
We should try to standardized the operations as much as possible and leave rest
as
vendor private. Current proposal allows us to do that.
> 3D_LUT is an operation category, not a name. The same could be said about
> private.
Sure, will fix this.
> Given that all these are also UAPI, do we also need protect old userspace from
> seeing values it does not understand?
For the values userspace doesn't understand, it can ignore the blocks. We
should ensure
that userspace always gets a clean state wrt color hardware state and no
baggage from
another client should be there. With that there is no burden of disabling that
particular
block will be there on an older userspace.
> > + */
> > +enum color_op_block {
> > + DRM_CB_INVAL = -1,
> > +
> > + DRM_CB_PRE_CSC = 0,
> > + DRM_CB_CSC,
> > + DRM_CB_POST_CSC,
> > + DRM_CB_3D_LUT,
> > +
> > + /* Any new generic hardware block can be updated here */
> > +
> > + /*
> > +* PRIVATE is kept at 255 to make it future proof and leave
> > +* scope for any new addition
> > +*/
> > + DRM_CB_PRIVATE = 255,
> > + DRM_CB_MAX = DRM_CB_PRIVATE,
> > +};
> > +
> > +/**
> > + * enum color_op_type
> > + *
> > + * These enums are to identify the mathematical operation that
> > + * a hardware block is capable of.
> > + * @CURVE_1D: It represents a one dimensional lookup table
> > + * @CURVE_3D: Represents lut value for each color component for 3d
> > +lut capable hardware
> > + * @MATRIX: It represents co-efficients for a CSC/CTM matrix hardware
> > + * @FIXED_FUNCTION: To enable and program any custom fixed function
> > +hardware unit */ enum color_op_type {
> > + CURVE_1D,
> > + CURVE_3D,
> > + MATRIX,
> > + FIXED_FUNCTION,
>
> My assumption was that a color_op_type would clearly and uniquely define the
> mathematical model of the operation and the UABI structure of the parameter
> blob. That means we need different values for uniform vs. exponentially vs.
> programmable distributed 1D LUT, etc.
In the hardware the LUTS are programmed as they are received from userspace.
So once the userspace gets to know the distribution of LUTS, segments,
precision,
Number of lut samples, it can create the lut values to be programmed.
This information on the distribution of luts in the hardware can be extracted
by the
drm_color_lut_range structure which is exposed as blob in the
Re: [Intel-gfx] [RFC 01/33] drm/doc/rfc: Add RFC document for proposed Plane Color Pipeline
> -Original Message- > From: dri-devel On Behalf Of Pekka > Paalanen > Sent: Wednesday, August 30, 2023 5:59 PM > To: Shankar, Uma > Cc: intel-gfx@lists.freedesktop.org; Borah, Chaitanya Kumar > ; dri-de...@lists.freedesktop.org; wayland- > de...@lists.freedesktop.org > Subject: Re: [RFC 01/33] drm/doc/rfc: Add RFC document for proposed Plane > Color Pipeline > > On Wed, 30 Aug 2023 08:59:36 + > "Shankar, Uma" wrote: > > > > -Original Message- > > > From: Harry Wentland > > > Sent: Wednesday, August 30, 2023 1:10 AM > > > To: Shankar, Uma ; > > > intel-gfx@lists.freedesktop.org; dri- de...@lists.freedesktop.org > > > Cc: Borah, Chaitanya Kumar ; > > > wayland- de...@lists.freedesktop.org > > > Subject: Re: [RFC 01/33] drm/doc/rfc: Add RFC document for proposed > > > Plane Color Pipeline > > > > > > > > > > > > On 2023-08-29 12:03, Uma Shankar wrote: > > > > Add the documentation for the new proposed Plane Color Pipeline. > > > > > > > > Co-developed-by: Chaitanya Kumar Borah > > > > > > > > Signed-off-by: Chaitanya Kumar Borah > > > > > > > > Signed-off-by: Uma Shankar > > > > --- > > > > .../gpu/rfc/plane_color_pipeline.rst | 394 ++ > > > > 1 file changed, 394 insertions(+) > > > > create mode 100644 > > > > Documentation/gpu/rfc/plane_color_pipeline.rst > > > > > > > > diff --git a/Documentation/gpu/rfc/plane_color_pipeline.rst > > > > b/Documentation/gpu/rfc/plane_color_pipeline.rst > > > > new file mode 100644 > > > > index ..60ce515b6ea7 > > > > --- /dev/null > > > > +++ b/Documentation/gpu/rfc/plane_color_pipeline.rst > > ... > > Hi Uma! Thanks Pekka for the feedback and useful inputs. > > > > +This color pipeline is then packaged within a blob for the user > > > > +space to retrieve it. Details can be found in the next section > > > > + > > > > > > Not sure I like blobs that contain other blob ids. > > > > It provides flexibility and helps with just one interface to > > userspace. Its easy to handle and manage once we get the hang of it . > > > > We can clearly define the steps of parsing and data structures to be > > used while interpreting and parsing the blobs. > > Don't forget extendability. Possibly every single struct will need some kind > of > versioning, and then it's not simple to parse anymore. Add to that new/old > kernel > vs. old/new userspace, and it seems a bit nightmarish to design. Structure to be used to interpret the blob should be defined as UAPI only and is not expected to change once agreed upon. It should be interpreted like a standard property. So structure to be used, say for 3dLut or 1dlut or CTM operations should be standardized and fixed. No versioning of structure should be done and same rules/restrictions as of UAPI property should be applied. New vs old userspace problem exists even today as you rightly highlighted in mail below, however we are planning to propose that we clean the hardware state once the userspace client switches or same client switches the pipeline. > Also since it's records inside a single blob, it's like a new file > format: every record needs a standard header that allows skipping it > appropriately if userspace does not understand it, or you need a standard > index > telling where everything is. Making all records the same size would waste > space, > and extendability requires variable size. The design currently implements 1 hardware block by a struct drm_color_op data structure. Multiple such blocks make the pipeline. So userspace just needs to get the pipeline and then parse blocks 1 by 1. For blocks which it doesn't understand it can just skip and move to the next one. Each block is differentiated by a unique "name" standardized by an enum which will be part of the UAPI. Thus we will have scope for variable size blob to represent the particular hardware pipeline, userspace can parse and implement whichever blocks it understands. Only rule defined by UAPI is the way the respective block is to be parsed and programmed. > I also would not assume that we can declare a standard set of blocks and that > nothing else will be needed. The existing hardware is too diverse for that > from > what I have understood. I assume that some hardware have blocks unique to > them, and they want to at least expose that functionality through a UAPI that > allows at least generic enumeration of functionality, even if it needs > specialized > userspace code to actually make use of. Yeah, this is right and for that reason we came up with an idea of DRM_CB_PRIVATE name for the hardware block. This will tell userspace that this is private hardware block for a particular hardware vendor. Generic userspace will ignore this block. Vendor specific HAL or compositor implementation can parse and use this block. To interpret the blob_id and assign to a respective data structure, private_flags will be used. These private_flags will be agreed upon by HAL and vendor
[Intel-gfx] [PATCH v4 1/2] drm/i915/lnl: possibility to enable FBC on first three planes
In LNL onwards, FBC can be associated to the first three planes.
FBC will be enabled on planes first come first served basis
until the userspace can select one of these FBC capable planes
explicitly.
v2:
- avoid fbc->state.plane check in intel_fbc_check_plane (Ville)
- simplify plane binding register writes (Matt)
- Update the subject to reflect that fbc can be enabled only in
the first three planes (Matt)
v3:
- use icl_is_hdr_plane(), use wrapper macro for plane binding
register access, comments update and patch split (Ville)
v4:
- update to the plane binding register access macro
Bspec: 69560
Signed-off-by: Vinod Govindapillai
---
drivers/gpu/drm/i915/display/intel_fbc.c | 3 +++
drivers/gpu/drm/i915/display/skl_universal_plane.c | 9 ++---
drivers/gpu/drm/i915/i915_reg.h| 2 ++
3 files changed, 11 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c
b/drivers/gpu/drm/i915/display/intel_fbc.c
index 66c8aed07bbc..a3999ad95a19 100644
--- a/drivers/gpu/drm/i915/display/intel_fbc.c
+++ b/drivers/gpu/drm/i915/display/intel_fbc.c
@@ -660,6 +660,9 @@ static u32 ivb_dpfc_ctl(struct intel_fbc *fbc)
if (IS_IVYBRIDGE(i915))
dpfc_ctl |= DPFC_CTL_PLANE_IVB(fbc_state->plane->i9xx_plane);
+ if (DISPLAY_VER(i915) >= 20)
+ dpfc_ctl |= DPFC_CTL_PLANE_BINDING(fbc_state->plane->id);
+
if (fbc_state->fence_id >= 0)
dpfc_ctl |= DPFC_CTL_FENCE_EN_IVB;
diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c
b/drivers/gpu/drm/i915/display/skl_universal_plane.c
index 4d01c7ae4485..8f946c5a2fd8 100644
--- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
+++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
@@ -1956,13 +1956,16 @@ static enum intel_fbc_id skl_fbc_id_for_pipe(enum pipe
pipe)
return pipe - PIPE_A + INTEL_FBC_A;
}
-static bool skl_plane_has_fbc(struct drm_i915_private *dev_priv,
+static bool skl_plane_has_fbc(struct drm_i915_private *i915,
enum intel_fbc_id fbc_id, enum plane_id plane_id)
{
- if ((DISPLAY_RUNTIME_INFO(dev_priv)->fbc_mask & BIT(fbc_id)) == 0)
+ if ((DISPLAY_RUNTIME_INFO(i915)->fbc_mask & BIT(fbc_id)) == 0)
return false;
- return plane_id == PLANE_PRIMARY;
+ if (DISPLAY_VER(i915) >= 20)
+ return icl_is_hdr_plane(i915, plane_id);
+ else
+ return plane_id == PLANE_PRIMARY;
}
static struct intel_fbc *skl_plane_fbc(struct drm_i915_private *dev_priv,
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index aefad14ab27a..d44ac6f1c052 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -1327,6 +1327,8 @@
#define DPFC_CTL_PLANE_IVB(i9xx_plane)
REG_FIELD_PREP(DPFC_CTL_PLANE_MASK_IVB, (i9xx_plane))
#define DPFC_CTL_FENCE_EN_IVBREG_BIT(28) /* ivb+ */
#define DPFC_CTL_PERSISTENT_MODE REG_BIT(25) /* g4x-snb */
+#define DPFC_CTL_PLANE_BINDING_MASK REG_GENMASK(12, 11) /* lnl+ */
+#define DPFC_CTL_PLANE_BINDING(plane_id)
REG_FIELD_PREP(DPFC_CTL_PLANE_BINDING_MASK, (plane_id))
#define DPFC_CTL_FALSE_COLOR REG_BIT(10) /* ivb+ */
#define DPFC_CTL_SR_EN REG_BIT(10) /* g4x only */
#define DPFC_CTL_SR_EXIT_DIS REG_BIT(9) /* g4x only */
--
2.34.1
[Intel-gfx] [PATCH v4 2/2] drm/i915/lnl: FBC is supported with per pixel alpha
For LNL onwards, FBC can be supported on planes with per
pixel alpha
Bspec: 69560
Signed-off-by: Vinod Govindapillai
---
drivers/gpu/drm/i915/display/intel_fbc.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c
b/drivers/gpu/drm/i915/display/intel_fbc.c
index a3999ad95a19..c0e4caec03ea 100644
--- a/drivers/gpu/drm/i915/display/intel_fbc.c
+++ b/drivers/gpu/drm/i915/display/intel_fbc.c
@@ -1209,7 +1209,8 @@ static int intel_fbc_check_plane(struct
intel_atomic_state *state,
return 0;
}
- if (plane_state->hw.pixel_blend_mode != DRM_MODE_BLEND_PIXEL_NONE &&
+ if (DISPLAY_VER(i915) < 20 &&
+ plane_state->hw.pixel_blend_mode != DRM_MODE_BLEND_PIXEL_NONE &&
fb->format->has_alpha) {
plane_state->no_fbc_reason = "per-pixel alpha not supported";
return 0;
--
2.34.1
[Intel-gfx] [PATCH v4 0/2] fbc on any planes
FBC can be supported in first three planes in lnl Vinod Govindapillai (2): drm/i915/lnl: possibility to enable FBC on first three planes drm/i915/lnl: FBC is supported with per pixel alpha drivers/gpu/drm/i915/display/intel_fbc.c | 6 +- drivers/gpu/drm/i915/display/skl_universal_plane.c | 9 ++--- drivers/gpu/drm/i915/i915_reg.h| 2 ++ 3 files changed, 13 insertions(+), 4 deletions(-) -- 2.34.1
Re: [Intel-gfx] [PATCH v2 04/22] drm/i915/dp: Update the link bpp limits for DSC mode
On Mon, Sep 04, 2023 at 06:48:25AM +0300, Ville Syrjälä wrote:
> On Thu, Aug 24, 2023 at 11:04:59AM +0300, Imre Deak wrote:
> > In non-DSC mode the link bpp can be set in 2*3 bpp steps in the pipe bpp
> > range, while in DSC mode it can be set in 1/16 bpp steps to any value
> > up to the maximum pipe bpp. Update the limits accordingly in both modes
> > to prepare for a follow-up patch which may need to reduce the max link
> > bpp value and starts to check the link bpp limits in DSC mode as well.
> >
> > While at it add more detail to the link limit debug print and print it
> > also for DSC mode.
> >
> > v2:
> > - Add to_bpp_frac_dec() instead of open coding it. (Jani)
> >
> > Cc: Jani Nikula
> > Signed-off-by: Imre Deak
> > ---
> > .../drm/i915/display/intel_display_types.h| 5 ++
> > drivers/gpu/drm/i915/display/intel_dp.c | 89 +++
> > drivers/gpu/drm/i915/display/intel_dp.h | 6 ++
> > drivers/gpu/drm/i915/display/intel_dp_mst.c | 23 +++--
> > 4 files changed, 101 insertions(+), 22 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
> > b/drivers/gpu/drm/i915/display/intel_display_types.h
> > index 5875eff5012ce..a0a404967b5d2 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > @@ -2113,6 +2113,11 @@ static inline int to_bpp_int(int bpp_x16)
> > return bpp_x16 >> 4;
> > }
> >
> > +static inline int to_bpp_frac_dec(int bpp_x16)
> > +{
> > + return (bpp_x16 & 0xf) * 625;
> > +}
>
> This gives me the impression that this would be somehow
> generally useful, but I presume we only use it for the printk?
> So maybe should just have some printk FMT+ARG macros for
> this stuff?
Yes, only used by printks. Make sense to define the FMT+ARG helpers at
one place, can add these here.
>
> > +
> > static inline int to_bpp_x16(int bpp)
> > {
> > return bpp << 4;
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> > b/drivers/gpu/drm/i915/display/intel_dp.c
> > index c580472c06b85..9ce861a7fd418 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > @@ -2189,16 +2189,68 @@ int intel_dp_dsc_compute_config(struct intel_dp
> > *intel_dp,
> > return 0;
> > }
> >
> > -static void
> > +/**
> > + * intel_dp_compute_config_link_bpp_limits - compute output link bpp limits
> > + * @intel_dp: intel DP
> > + * @crtc_state: crtc state
> > + * @dsc: DSC compression mode
> > + * @limits: link configuration limits
> > + *
> > + * Calculates the output link min, max bpp values in @limits based on the
> > + * pipe bpp range, @crtc_state and @dsc mode.
> > + *
> > + * Returns %true in case of success.
> > + */
> > +bool
> > +intel_dp_compute_config_link_bpp_limits(struct intel_dp *intel_dp,
> > + const struct intel_crtc_state
> > *crtc_state,
> > + bool dsc,
> > + struct link_config_limits *limits)
> > +{
> > + struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc->dev);
> > + const struct drm_display_mode *adjusted_mode =
> > + _state->hw.adjusted_mode;
> > + const struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > + const struct intel_encoder *encoder = _to_dig_port(intel_dp)->base;
> > + int max_link_bpp_x16;
> > +
> > + max_link_bpp_x16 = to_bpp_x16(limits->pipe.max_bpp);
> > +
> > + if (!dsc) {
> > + max_link_bpp_x16 = rounddown(max_link_bpp_x16, to_bpp_x16(2 *
> > 3));
> > +
> > + if (max_link_bpp_x16 < to_bpp_x16(limits->pipe.min_bpp))
> > + return false;
>
> Quite a few to_bpp_x16()'s in there. Seems like it would a bit simpler
> to just do that once at the end.
At the moment yes, but in a later patch max_link_bpp_x16 starts out as
crtc_state->max_link_bpp_x16 limited value (with a non-zero fractional
part).
>
> > +
> > + limits->link.min_bpp_x16 = to_bpp_x16(limits->pipe.min_bpp);
> > + } else {
> > + limits->link.min_bpp_x16 = 0;
>
> Why is that zero? Don't we now have some helpers to fill
> this stuff correctly?
At the moment it's calculated only later in
intel_edp_dsc_compute_pipe_bpp() / intel_dp_dsc_compute_pipe_bpp().
It should be inited already here, but I wanted to do that only as a
follow-up, since there's been other DSC changes from Ankit still under
review. Is that ok, adding a TODO: here?
>
> > + }
> > +
> > + limits->link.max_bpp_x16 = max_link_bpp_x16;
> > +
> > + drm_dbg_kms(>drm,
> > + "[ENCODER:%d:%s][CRTC:%d:%s] DP link limits: pixel clock %d
> > kHz DSC %s max lanes %d max rate %d max pipe_bpp %d max link_bpp %d.%04d\n",
> > + encoder->base.base.id, encoder->base.name,
> > + crtc->base.base.id, crtc->base.name,
> > + adjusted_mode->crtc_clock,
> > + dsc ? "on" : "off",
>
Re: [Intel-gfx] [PATCH 1/6] drm/edid: add drm_edid_is_digital()
On Fri, 01 Sep 2023, Alex Deucher wrote:
> On Thu, Aug 24, 2023 at 9:46 AM Jani Nikula wrote:
>>
>> Checking edid->input & DRM_EDID_INPUT_DIGITAL is common enough to
>> deserve a helper that also lets us abstract the raw EDID a bit better.
>>
>> Signed-off-by: Jani Nikula
>
> Reviewed-by: Alex Deucher
Thanks; I'm afraid this got merged already.
> Seems to be a few additional users of this that could be converted:
>
> drivers/gpu/drm/i915/display/intel_sdvo.c:if (edid &&
> edid->input & DRM_EDID_INPUT_DIGITAL)
> drivers/gpu/drm/i915/display/intel_sdvo.c:bool monitor_is_digital
> = !!(edid->input & DRM_EDID_INPUT_DIGITAL);
> drivers/gpu/drm/i915/display/intel_crt.c:bool is_digital =
> edid->input & DRM_EDID_INPUT_DIGITAL;
> drivers/gpu/drm/i915/display/intel_hdmi.c:if (edid && edid->input
> & DRM_EDID_INPUT_DIGITAL) {
The next patch takes care of these.
> drivers/gpu/drm/gma500/psb_intel_sdvo.c:if (edid->input &
> DRM_EDID_INPUT_DIGITAL) {
> drivers/gpu/drm/gma500/psb_intel_sdvo.c:if (edid->input &
> DRM_EDID_INPUT_DIGITAL)
> drivers/gpu/drm/gma500/psb_intel_sdvo.c:bool
> monitor_is_digital = !!(edid->input & DRM_EDID_INPUT_DIGITAL);
> drivers/gpu/drm/gma500/psb_intel_sdvo.c:if (edid != NULL &&
> edid->input & DRM_EDID_INPUT_DIGITAL)
> drivers/gpu/drm/gma500/cdv_intel_hdmi.c:if (edid->input &
> DRM_EDID_INPUT_DIGITAL) {
> drivers/gpu/drm/display/drm_dp_helper.c:edid->input &
> DRM_EDID_INPUT_DIGITAL &&
> drivers/gpu/drm/nouveau/nouveau_connector.c:if
> (nv_connector->edid->input & DRM_EDID_INPUT_DIGITAL)
> drivers/gpu/drm/radeon/radeon_connectors.c:
> !!(radeon_connector->edid->input & DRM_EDID_INPUT_DIGITAL);
> drivers/gpu/drm/radeon/radeon_connectors.c:
> !!(radeon_connector->edid->input & DRM_EDID_INPUT_DIGITAL);
> drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c:
> !!(amdgpu_connector->edid->input & DRM_EDID_INPUT_DIGITAL);
> drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c:
> !!(amdgpu_connector->edid->input & DRM_EDID_INPUT_DIGITAL);
drm_edid_is_digital() operates on struct drm_edid.
The drivers would first need to be converted to use struct drm_edid
instead of struct edid, and I'm not really taking that on.
IMO adding helpers to operate on struct edid would be
counter-productive.
BR,
Jani.
>
>
>
>
>> ---
>> drivers/gpu/drm/drm_edid.c | 17 +++--
>> include/drm/drm_edid.h | 1 +
>> 2 files changed, 16 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>> index 340da8257b51..1dbb15439468 100644
>> --- a/drivers/gpu/drm/drm_edid.c
>> +++ b/drivers/gpu/drm/drm_edid.c
>> @@ -3110,7 +3110,7 @@ drm_monitor_supports_rb(const struct drm_edid
>> *drm_edid)
>> return ret;
>> }
>>
>> - return ((drm_edid->edid->input & DRM_EDID_INPUT_DIGITAL) != 0);
>> + return drm_edid_is_digital(drm_edid);
>> }
>>
>> static void
>> @@ -6519,7 +6519,7 @@ static void update_display_info(struct drm_connector
>> *connector,
>> if (edid->revision < 3)
>> goto out;
>>
>> - if (!(edid->input & DRM_EDID_INPUT_DIGITAL))
>> + if (!drm_edid_is_digital(drm_edid))
>> goto out;
>>
>> info->color_formats |= DRM_COLOR_FORMAT_RGB444;
>> @@ -7335,3 +7335,16 @@ static void _drm_update_tile_info(struct
>> drm_connector *connector,
>> connector->tile_group = NULL;
>> }
>> }
>> +
>> +/**
>> + * drm_edid_is_digital - is digital?
>> + * @drm_edid: The EDID
>> + *
>> + * Return true if input is digital.
>> + */
>> +bool drm_edid_is_digital(const struct drm_edid *drm_edid)
>> +{
>> + return drm_edid && drm_edid->edid &&
>> + drm_edid->edid->input & DRM_EDID_INPUT_DIGITAL;
>> +}
>> +EXPORT_SYMBOL(drm_edid_is_digital);
>> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
>> index 48e93f909ef6..882d2638708e 100644
>> --- a/include/drm/drm_edid.h
>> +++ b/include/drm/drm_edid.h
>> @@ -612,6 +612,7 @@ const struct drm_edid *drm_edid_read_switcheroo(struct
>> drm_connector *connector,
>> int drm_edid_connector_update(struct drm_connector *connector,
>> const struct drm_edid *edid);
>> int drm_edid_connector_add_modes(struct drm_connector *connector);
>> +bool drm_edid_is_digital(const struct drm_edid *drm_edid);
>>
>> const u8 *drm_find_edid_extension(const struct drm_edid *drm_edid,
>> int ext_id, int *ext_index);
>> --
>> 2.39.2
>>
--
Jani Nikula, Intel Open Source Graphics Center
Re: [Intel-gfx] [PATCH v2 01/22] drm/i915/dp: Factor out helpers to compute the link limits
On Mon, Sep 04, 2023 at 06:19:04AM +0300, Ville Syrjälä wrote:
> On Thu, Aug 24, 2023 at 11:04:56AM +0300, Imre Deak wrote:
> > Factor out helpers that DP / DP_MST encoders can use to compute the link
> > rate/lane count and bpp limits. A follow-up patch will call these to
> > recalculate the limits if DSC compression is required.
> >
> > Signed-off-by: Imre Deak
> > ---
> > drivers/gpu/drm/i915/display/intel_dp.c | 61 +
> > drivers/gpu/drm/i915/display/intel_dp_mst.c | 52 ++
> > 2 files changed, 68 insertions(+), 45 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> > b/drivers/gpu/drm/i915/display/intel_dp.c
> > index 7067ee3a4bd36..53697f361f950 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > @@ -2187,29 +2187,25 @@ int intel_dp_dsc_compute_config(struct intel_dp
> > *intel_dp,
> > return 0;
> > }
> >
> > -static int
> > -intel_dp_compute_link_config(struct intel_encoder *encoder,
> > -struct intel_crtc_state *pipe_config,
> > -struct drm_connector_state *conn_state,
> > -bool respect_downstream_limits)
> > +static void
> > +intel_dp_compute_config_limits(struct intel_dp *intel_dp,
> > + struct intel_crtc_state *crtc_state,
> > + bool respect_downstream_limits,
> > + struct link_config_limits *limits)
> > {
> > - struct drm_i915_private *i915 = to_i915(encoder->base.dev);
> > - struct intel_crtc *crtc = to_intel_crtc(pipe_config->uapi.crtc);
> > + struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> > const struct drm_display_mode *adjusted_mode =
> > - _config->hw.adjusted_mode;
> > - struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> > - struct link_config_limits limits;
> > - bool joiner_needs_dsc = false;
> > - int ret;
> > + _state->hw.adjusted_mode;
> >
> > - limits.min_rate = intel_dp_common_rate(intel_dp, 0);
> > - limits.max_rate = intel_dp_max_link_rate(intel_dp);
> > + limits->min_rate = intel_dp_common_rate(intel_dp, 0);
> > + limits->max_rate = intel_dp_max_link_rate(intel_dp);
> >
> > - limits.min_lane_count = 1;
> > - limits.max_lane_count = intel_dp_max_lane_count(intel_dp);
> > + limits->min_lane_count = 1;
> > + limits->max_lane_count = intel_dp_max_lane_count(intel_dp);
> >
> > - limits.min_bpp = intel_dp_min_bpp(pipe_config->output_format);
> > - limits.max_bpp = intel_dp_max_bpp(intel_dp, pipe_config,
> > respect_downstream_limits);
> > + limits->min_bpp = intel_dp_min_bpp(crtc_state->output_format);
> > + limits->max_bpp = intel_dp_max_bpp(intel_dp, crtc_state,
> > + respect_downstream_limits);
> >
> > if (intel_dp->use_max_params) {
> > /*
> > @@ -2220,16 +2216,35 @@ intel_dp_compute_link_config(struct intel_encoder
> > *encoder,
> > * configuration, and typically on older panels these
> > * values correspond to the native resolution of the panel.
> > */
> > - limits.min_lane_count = limits.max_lane_count;
> > - limits.min_rate = limits.max_rate;
> > + limits->min_lane_count = limits->max_lane_count;
> > + limits->min_rate = limits->max_rate;
> > }
> >
> > - intel_dp_adjust_compliance_config(intel_dp, pipe_config, );
> > + intel_dp_adjust_compliance_config(intel_dp, crtc_state, limits);
>
> Annoying little bugger that mutates the crtc_state. Would be nice
> to relocate that small part somewhere else so that we could constify
> things more...
Yes, it could set dither_force_disable later separately.
> Anyways
> Reviewed-by: Ville Syrjälä
>
> >
> > drm_dbg_kms(>drm, "DP link computation with max lane count %i "
> > "max rate %d max bpp %d pixel clock %iKHz\n",
> > - limits.max_lane_count, limits.max_rate,
> > - limits.max_bpp, adjusted_mode->crtc_clock);
> > + limits->max_lane_count, limits->max_rate,
> > + limits->max_bpp, adjusted_mode->crtc_clock);
> > +}
> > +
> > +static int
> > +intel_dp_compute_link_config(struct intel_encoder *encoder,
> > +struct intel_crtc_state *pipe_config,
> > +struct drm_connector_state *conn_state,
> > +bool respect_downstream_limits)
> > +{
> > + struct drm_i915_private *i915 = to_i915(encoder->base.dev);
> > + struct intel_crtc *crtc = to_intel_crtc(pipe_config->uapi.crtc);
> > + const struct drm_display_mode *adjusted_mode =
> > + _config->hw.adjusted_mode;
> > + struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> > + struct link_config_limits limits;
> > + bool joiner_needs_dsc = false;
> > + int ret;
> > +
> > + intel_dp_compute_config_limits(intel_dp,
Re: [Intel-gfx] [PATCH v2 09/22] drm/dp_mst: Fix fractional bpp scaling in drm_dp_calc_pbn_mode()
On Mon, Sep 04, 2023 at 05:53:11AM +0300, Ville Syrjälä wrote:
> On Thu, Aug 24, 2023 at 11:05:04AM +0300, Imre Deak wrote:
> > For fractional bpp values passed to the function in a .4 fixed point
> > format, the fractional part is currently ignored due to scaling bpp too
> > early. Fix this by scaling the overhead factor instead and to avoid an
> > overflow multiplying bpp with the overhead factor instead of the clock
> > rate.
> >
> > While at it simplify the formula, and pass the expected fixed point bpp
> > values in the kunit tests.
> >
> > Cc: Lyude Paul
> > Cc: dri-de...@lists.freedesktop.org
> > Signed-off-by: Imre Deak
> > ---
> > drivers/gpu/drm/display/drm_dp_mst_topology.c | 7 ++-
> > drivers/gpu/drm/tests/drm_dp_mst_helper_test.c | 8
> > 2 files changed, 6 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > index ed96cfcfa3040..bd0f35a0ea5fb 100644
> > --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > @@ -4712,12 +4712,9 @@ int drm_dp_calc_pbn_mode(int clock, int bpp, bool
> > dsc)
> > * factor in the numerator rather than the denominator to avoid
> > * integer overflow
> > */
> > + u32 bpp_m = (dsc ? 64 / 16 : 64) * 1006 * bpp;
> >
> > - if (dsc)
> > - return DIV_ROUND_UP_ULL(mul_u32_u32(clock * (bpp / 16), 64 *
> > 1006),
> > - 8 * 54 * 1000 * 1000);
> > -
> > - return DIV_ROUND_UP_ULL(mul_u32_u32(clock * bpp, 64 * 1006),
> > + return DIV_ROUND_UP_ULL(mul_u32_u32(clock, bpp_m),
> > 8 * 54 * 1000 * 1000);
>
> I thought I sorted out this mess already...
> https://patchwork.freedesktop.org/patch/535005/?series=117201=3
> Apparently I forgot to push that.
Looks ok, can use that instead. I thought clock * bpp could overflow,
but probably not in practice.
The test cases below would still need to be fixed.
>
> > }
> > EXPORT_SYMBOL(drm_dp_calc_pbn_mode);
> > diff --git a/drivers/gpu/drm/tests/drm_dp_mst_helper_test.c
> > b/drivers/gpu/drm/tests/drm_dp_mst_helper_test.c
> > index 545beea33e8c7..ea2182815ebe8 100644
> > --- a/drivers/gpu/drm/tests/drm_dp_mst_helper_test.c
> > +++ b/drivers/gpu/drm/tests/drm_dp_mst_helper_test.c
> > @@ -40,15 +40,15 @@ static const struct drm_dp_mst_calc_pbn_mode_test
> > drm_dp_mst_calc_pbn_mode_cases
> > },
> > {
> > .clock = 332880,
> > - .bpp = 24,
> > + .bpp = 24 << 4,
> > .dsc = true,
> > - .expected = 50
> > + .expected = 1191
> > },
> > {
> > .clock = 324540,
> > - .bpp = 24,
> > + .bpp = 24 << 4,
> > .dsc = true,
> > - .expected = 49
> > + .expected = 1161
> > },
> > };
> >
> > --
> > 2.37.2
>
> --
> Ville Syrjälä
> Intel
Re: [Intel-gfx] [PATCH v4 0/4] Handle dma fences in dirtyfb ioctl
On Fri, 2023-09-01 at 14:09 +0300, Ville Syrjälä wrote: > On Fri, Sep 01, 2023 at 12:34:56PM +0300, Jouni Högander wrote: > > Currently i915 dirtyfb ioctl is not taking dma fences into > > account. This works with features like FBC, PSR, DRRS because our > > gem > > code is triggering flush again when rendering completes. We are > > targeting in getting rid of frontbuffer tracking code: Flusing hook > > from gem code will be removed as well. > > > > This patch set is adding dma fence handling into i915 dirtyfb > > ioctl. > > > > v4: > > - Move invalidate back being done before cb is added > > v3: > > - Check frontbuffer bits before adding any fence fb > > - Invalidate only when adding fence cb succeeds > > - Check schedule work success rather than work being pending > > - Init flush work when frontbuffer struct is initialized > > v2: > > - Clear fbc and psr busy bits on flip > > - Check if flush work is already pending > > - Use dma_resv_get_singleton > > > > Cc: Ville Syrjälä > > Cc: Maarten Lankhorst > > For the series: > Reviewed-by: Ville Syrjälä Thank you Ville and Luca for checking my patches. These are now merged. BR, Jouni Högander > > > > > Jouni Högander (4): > > drm/i915/fbc: Clear frontbuffer busy bits on flip > > drm/i915/psr: Clear frontbuffer busy bits on flip > > drm/i915: Add new frontbuffer tracking interface to queue flush > > drm/i915: Handle dma fences in dirtyfb callback > > > > drivers/gpu/drm/i915/display/intel_fb.c | 60 > > ++- > > drivers/gpu/drm/i915/display/intel_fbc.c | 6 +- > > .../gpu/drm/i915/display/intel_frontbuffer.c | 28 + > > .../gpu/drm/i915/display/intel_frontbuffer.h | 4 ++ > > drivers/gpu/drm/i915/display/intel_psr.c | 6 ++ > > 5 files changed, 97 insertions(+), 7 deletions(-) > > > > -- > > 2.34.1 >
Re: [Intel-gfx] [PATCH v4 1/4] drm/i915/fbc: Clear frontbuffer busy bits on flip
On Mon, 2023-09-04 at 08:40 +, Hogander, Jouni wrote: > On Mon, 2023-09-04 at 07:25 +, Coelho, Luciano wrote: > > Hi Jouni, > > > > On Fri, 2023-09-01 at 12:34 +0300, Jouni Högander wrote: > > > We are planning to move flush performed from work queue. This > > > means it is possible to have invalidate -> flip -> flush sequence. > > > Handle this by clearing possible busy bits on flip. > > > > > > Signed-off-by: Ville Syrjälä > > > Signed-off-by: Jouni Högander > > > --- > > > drivers/gpu/drm/i915/display/intel_fbc.c | 6 ++ > > > 1 file changed, 2 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c > > > b/drivers/gpu/drm/i915/display/intel_fbc.c > > > index 1c6d467cec26..817e5784660b 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_fbc.c > > > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c > > > @@ -1307,11 +1307,9 @@ static void __intel_fbc_post_update(struct > > > intel_fbc *fbc) > > > lockdep_assert_held(>lock); > > > > > > fbc->flip_pending = false; > > > + fbc->busy_bits = 0; > > > > > > - if (!fbc->busy_bits) > > > - intel_fbc_activate(fbc); > > > - else > > > - intel_fbc_deactivate(fbc, "frontbuffer write"); > > > + intel_fbc_activate(fbc); > > > > Can you explain why the call to intel_fbc_deactivate() is not needed > > here anymore? I think it would be a good idea to explain that in the > > commit message. Or, at least, an explanation about it here, so it's > > documented. ;) > > We are clearing fbc->busy_bits -> I.e. if(!fbc->busy_bits) is always > taken : > > Post plane update is called at the end of the flip. If you consider > case where busy_bits != 0 at this point: it means someone have > initiated frontbuffer write (invalidate) which is not yet completed > (flush from workqueue). That flush pending in workqueue is not valid > anymore as there was a flip and the buffer which was frontbuffer is not > a frontbuffer anymore. Even if the same buffer would be used when doing > a flip the atomic commit would take care of flushing the buffer towards > fbc. Also waiting for dma fences is take caren by the atomic commit > code. Thanks for the explanation! It makes sense. So you have my: Reviewed-by: Luca Coelho -- Cheers, Luca.
Re: [Intel-gfx] [PATCH] drm/i915/dsi: let HW maintain HS-TRAIL and CLK_POST
Thanks for the comment.
I will revise this patch, so the change is only removing POST overriding.
In addition, the patch for removing TRAIL was submitted as
https://patchwork.kernel.org/project/intel-gfx/patch/20211217100903.32599-1-william.ts...@intel.com/.
Can you help to review as well?
-Original Message-
From: Ville Syrjälä
Sent: Friday, September 1, 2023 6:53 PM
To: Tseng, William
Cc: intel-gfx@lists.freedesktop.org; Jani Nikula ;
Kulkarni, Vandita ; Kandpal, Suraj
; Lee, Shawn C
Subject: Re: [PATCH] drm/i915/dsi: let HW maintain HS-TRAIL and CLK_POST
On Fri, Sep 01, 2023 at 05:51:00PM +0800, William Tseng wrote:
> This change is to adjust TEOT timing and TCLK-POST timing so DSI
> signaling can meet CTS specification.
>
> For clock lane, the measured TEOT may be changed from 142.64 ns to
> 107.36 ns, which is less than (105 ns+12*UI) and is conformed to mipi
> D-PHY v1.2 CTS v1.0.
>
> As to TCLK-POST, it may be changed from 133.44 ns to 178.72 ns, which
> is greater than (60 ns+52*UI) and is conformed to the CTS standard.
>
> The computed UI is around 1.47 ns.
>
> Cc: Ville Syrjala
> Cc: Jani Nikula
> Cc: Vandita Kulkarni
> Cc: Suraj Kandpal
> Cc: Lee Shawn C
> Signed-off-by: William Tseng
> ---
> drivers/gpu/drm/i915/display/icl_dsi.c | 31
> --
> 1 file changed, 4 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/icl_dsi.c
> b/drivers/gpu/drm/i915/display/icl_dsi.c
> index ad6488e9c2b2..4a13f467ca46 100644
> --- a/drivers/gpu/drm/i915/display/icl_dsi.c
> +++ b/drivers/gpu/drm/i915/display/icl_dsi.c
> @@ -1819,10 +1819,10 @@ static void icl_dphy_param_init(struct intel_dsi
> *intel_dsi)
> struct intel_connector *connector = intel_dsi->attached_connector;
> struct mipi_config *mipi_config = connector->panel.vbt.dsi.config;
> u32 tlpx_ns;
> - u32 prepare_cnt, exit_zero_cnt, clk_zero_cnt, trail_cnt;
> - u32 ths_prepare_ns, tclk_trail_ns;
> + u32 prepare_cnt, exit_zero_cnt, clk_zero_cnt;
> + u32 ths_prepare_ns;
> u32 hs_zero_cnt;
> - u32 tclk_pre_cnt, tclk_post_cnt;
> + u32 tclk_pre_cnt;
>
> tlpx_ns = intel_dsi_tlpx_ns(intel_dsi);
>
> @@ -1853,14 +1853,6 @@ static void icl_dphy_param_init(struct intel_dsi
> *intel_dsi)
> clk_zero_cnt = ICL_CLK_ZERO_CNT_MAX;
> }
>
> - /* trail cnt in escape clocks*/
> - trail_cnt = DIV_ROUND_UP(tclk_trail_ns, tlpx_ns);
> - if (trail_cnt > ICL_TRAIL_CNT_MAX) {
> - drm_dbg_kms(_priv->drm, "trail_cnt out of range (%d)\n",
> - trail_cnt);
> - trail_cnt = ICL_TRAIL_CNT_MAX;
> - }
> -
> /* tclk pre count in escape clocks */
> tclk_pre_cnt = DIV_ROUND_UP(mipi_config->tclk_pre, tlpx_ns);
> if (tclk_pre_cnt > ICL_TCLK_PRE_CNT_MAX) { @@ -1869,15 +1861,6 @@
> static void icl_dphy_param_init(struct intel_dsi *intel_dsi)
> tclk_pre_cnt = ICL_TCLK_PRE_CNT_MAX;
> }
>
> - /* tclk post count in escape clocks */
> - tclk_post_cnt = DIV_ROUND_UP(mipi_config->tclk_post, tlpx_ns);
> - if (tclk_post_cnt > ICL_TCLK_POST_CNT_MAX) {
> - drm_dbg_kms(_priv->drm,
> - "tclk_post_cnt out of range (%d)\n",
> - tclk_post_cnt);
> - tclk_post_cnt = ICL_TCLK_POST_CNT_MAX;
> - }
> -
> /* hs zero cnt in escape clocks */
> hs_zero_cnt = DIV_ROUND_UP(mipi_config->ths_prepare_hszero -
> ths_prepare_ns, tlpx_ns);
> @@ -1902,19 +1885,13 @@ static void icl_dphy_param_init(struct intel_dsi
> *intel_dsi)
> CLK_ZERO_OVERRIDE |
> CLK_ZERO(clk_zero_cnt) |
> CLK_PRE_OVERRIDE |
> -CLK_PRE(tclk_pre_cnt) |
> -CLK_POST_OVERRIDE |
> -CLK_POST(tclk_post_cnt) |
> -CLK_TRAIL_OVERRIDE |
> -CLK_TRAIL(trail_cnt));
> +CLK_PRE(tclk_pre_cnt));
Windows seems set these overrides:
icl clk DPHY: PREPARE,ZERO
icl clk DSI: PREPARE,ZERO
icl data DPHY: PREPARE,ZERO,EXIT
icl data DSI: PREPARE,ZERO,EXIT
tgl clk DPHY: PREPARE,ZERO (?)
tgl clk DSI: PREPARE,ZERO,POST (?)
tgl data DPHY: PREPARE,ZERO,EXIT
tgl data DSI: PREPARE,ZERO,EXIT
adl clk DPHY: PREPARE,ZERO (?) (also PRE for 2.0-2.5 GHz data rate)
adl clk DSI: PREPARE,ZERO,POST (?) (also PRE for 2.0-2.5 GHz data rate)
adl data DPHY: PREPARE,ZERO,EXIT
adl data DSI : PREPARE,ZERO,EXIT
Didn't see any justification for the weird mismatch between DSI vs. DPHY POST
override on tgl+.
Anyways, looks like removing TRAIL is not particularly controversial since
Windows also never overrides it. So probably you should split that up into a
separate patch.
>
> /* data lanes dphy timings */
>
Re: [Intel-gfx] [PATCH v4 1/4] drm/i915/fbc: Clear frontbuffer busy bits on flip
On Mon, 2023-09-04 at 07:25 +, Coelho, Luciano wrote: > Hi Jouni, > > On Fri, 2023-09-01 at 12:34 +0300, Jouni Högander wrote: > > We are planning to move flush performed from work queue. This > > means it is possible to have invalidate -> flip -> flush sequence. > > Handle this by clearing possible busy bits on flip. > > > > Signed-off-by: Ville Syrjälä > > Signed-off-by: Jouni Högander > > --- > > drivers/gpu/drm/i915/display/intel_fbc.c | 6 ++ > > 1 file changed, 2 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c > > b/drivers/gpu/drm/i915/display/intel_fbc.c > > index 1c6d467cec26..817e5784660b 100644 > > --- a/drivers/gpu/drm/i915/display/intel_fbc.c > > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c > > @@ -1307,11 +1307,9 @@ static void __intel_fbc_post_update(struct > > intel_fbc *fbc) > > lockdep_assert_held(>lock); > > > > fbc->flip_pending = false; > > + fbc->busy_bits = 0; > > > > - if (!fbc->busy_bits) > > - intel_fbc_activate(fbc); > > - else > > - intel_fbc_deactivate(fbc, "frontbuffer write"); > > + intel_fbc_activate(fbc); > > Can you explain why the call to intel_fbc_deactivate() is not needed > here anymore? I think it would be a good idea to explain that in the > commit message. Or, at least, an explanation about it here, so it's > documented. ;) We are clearing fbc->busy_bits -> I.e. if(!fbc->busy_bits) is always taken : Post plane update is called at the end of the flip. If you consider case where busy_bits != 0 at this point: it means someone have initiated frontbuffer write (invalidate) which is not yet completed (flush from workqueue). That flush pending in workqueue is not valid anymore as there was a flip and the buffer which was frontbuffer is not a frontbuffer anymore. Even if the same buffer would be used when doing a flip the atomic commit would take care of flushing the buffer towards fbc. Also waiting for dma fences is take caren by the atomic commit code. BR, Jouni Högander > > -- > Cheers, > Luca.
Re: [Intel-gfx] [PATCH v15 17/23] drm/shmem-helper: Add and use drm_gem_shmem_resv_assert_held() helper
On Sat, 2 Sep 2023 22:43:02 +0300
Dmitry Osipenko wrote:
> On 8/29/23 10:29, Boris Brezillon wrote:
> > On Tue, 29 Aug 2023 05:34:23 +0300
> > Dmitry Osipenko wrote:
> >
> >> On 8/28/23 13:12, Boris Brezillon wrote:
> >>> On Sun, 27 Aug 2023 20:54:43 +0300
> >>> Dmitry Osipenko wrote:
> >>>
> In a preparation of adding drm-shmem memory shrinker, move all
> reservation
> locking lockdep checks to use new drm_gem_shmem_resv_assert_held() that
> will resolve spurious lockdep warning about wrong locking order vs
> fs_reclam code paths during freeing of shmem GEM, where lockdep isn't
> aware that it's impossible to have locking contention with the fs_reclam
> at this special time.
>
> Signed-off-by: Dmitry Osipenko
> ---
> drivers/gpu/drm/drm_gem_shmem_helper.c | 37 +-
> 1 file changed, 25 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c
> b/drivers/gpu/drm/drm_gem_shmem_helper.c
> index d96fee3d6166..ca5da976aafa 100644
> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
> @@ -128,6 +128,23 @@ struct drm_gem_shmem_object
> *drm_gem_shmem_create(struct drm_device *dev, size_t
> }
> EXPORT_SYMBOL_GPL(drm_gem_shmem_create);
>
> +static void drm_gem_shmem_resv_assert_held(struct drm_gem_shmem_object
> *shmem)
> +{
> +/*
> + * Destroying the object is a special case..
> drm_gem_shmem_free()
> + * calls many things that WARN_ON if the obj lock is not held.
> But
> + * acquiring the obj lock in drm_gem_shmem_free() can cause a
> locking
> + * order inversion between reservation_ww_class_mutex and
> fs_reclaim.
> + *
> + * This deadlock is not actually possible, because no one should
> + * be already holding the lock when drm_gem_shmem_free() is
> called.
> + * Unfortunately lockdep is not aware of this detail. So when
> the
> + * refcount drops to zero, we pretend it is already locked.
> + */
> +if (kref_read(>base.refcount))
> +drm_gem_shmem_resv_assert_held(shmem);
> +}
> +
> /**
> * drm_gem_shmem_free - Free resources associated with a shmem GEM
> object
> * @shmem: shmem GEM object to free
> @@ -142,8 +159,6 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object
> *shmem)
> if (obj->import_attach) {
> drm_prime_gem_destroy(obj, shmem->sgt);
> } else if (!shmem->imported_sgt) {
> -dma_resv_lock(shmem->base.resv, NULL);
> -
> drm_WARN_ON(obj->dev,
> kref_read(>vmap_use_count));
>
> if (shmem->sgt) {
> @@ -156,8 +171,6 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object
> *shmem)
> drm_gem_shmem_put_pages_locked(shmem);
> >>>
> >>> AFAICT, drm_gem_shmem_put_pages_locked() is the only function that's
> >>> called in the free path and would complain about resv-lock not being
> >>> held. I think I'd feel more comfortable if we were adding a
> >>> drm_gem_shmem_free_pages() function that did everything
> >>> drm_gem_shmem_put_pages_locked() does except for the lock_held() check
> >>> and the refcount dec, and have it called here (and in
> >>> drm_gem_shmem_put_pages_locked()). This way we can keep using
> >>> dma_resv_assert_held() instead of having our own variant.
> >>
> >> It's not only drm_gem_shmem_free_pages(), but any drm-shmem function
> >> that drivers may use in the GEM's freeing callback.
> >>
> >> For example, panfrost_gem_free_object() may unpin shmem BO and then do
> >> drm_gem_shmem_free().
> >
> > Is this really a valid use case? If the GEM refcount dropped to zero,
> > we should certainly not have pages_pin_count > 0 (thinking of vmap-ed
> > buffers that might disappear while kernel still has a pointer to the
> > CPU-mapped area). The only reason we have this
> > drm_gem_shmem_put_pages_locked() in drm_gem_shmem_free() is because of
> > this implicit ref hold by the sgt, and IMHO, we should be stricter and
> > check that pages_use_count == 1 when sgt != NULL and pages_use_count ==
> > 0 otherwise.
> >
> > I actually think it's a good thing to try and catch any attempt to call
> > functions trying lock the resv in a path they're not supposed to. At
> > least we can decide whether these actions are valid or not in this
> > context, and provide dedicated helpers for the free path if they are.
>
> To me it's a valid use-case. I was going to do it for the virtio-gpu
> driver for a specific BO type that should be permanently pinned in
> memory. So I made the BO pinned in the virto_gpu's bo_create() and
>
Re: [Intel-gfx] [PATCH v15 01/23] drm/shmem-helper: Fix UAF in error path when freeing SGT of imported GEM
On Sat, 2 Sep 2023 21:15:39 +0300
Dmitry Osipenko wrote:
> On 8/28/23 14:16, Boris Brezillon wrote:
> > On Sun, 27 Aug 2023 20:54:27 +0300
> > Dmitry Osipenko wrote:
> >
> >> Freeing drm-shmem GEM right after creating it using
> >> drm_gem_shmem_prime_import_sg_table() frees SGT of the imported dma-buf
> >> and then dma-buf frees this SGT second time.
> >>
> >> The v3d_prime_import_sg_table() is example of a error code path where
> >> dma-buf's SGT is freed by drm-shmem and then it's freed second time by
> >> dma_buf_unmap_attachment() in drm_gem_prime_import_dev().
> >>
> >> Add drm-shmem GEM flag telling that this is imported SGT shall not be
> >> treated as own SGT, fixing the use-after-free bug.
> >>
> >> Cc: sta...@vger.kernel.org
> >> Fixes: 2194a63a818d ("drm: Add library for shmem backed GEM objects")
> >> Signed-off-by: Dmitry Osipenko
> >> ---
> >> drivers/gpu/drm/drm_gem_shmem_helper.c | 3 ++-
> >> include/drm/drm_gem_shmem_helper.h | 7 +++
> >> 2 files changed, 9 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c
> >> b/drivers/gpu/drm/drm_gem_shmem_helper.c
> >> index a783d2245599..78d9cf2355a5 100644
> >> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
> >> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
> >> @@ -141,7 +141,7 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object
> >> *shmem)
> >>
> >>if (obj->import_attach) {
> >>drm_prime_gem_destroy(obj, shmem->sgt);
> >> - } else {
> >> + } else if (!shmem->imported_sgt) {
> >>dma_resv_lock(shmem->base.resv, NULL);
> >>
> >>drm_WARN_ON(obj->dev, shmem->vmap_use_count);
> >> @@ -758,6 +758,7 @@ drm_gem_shmem_prime_import_sg_table(struct drm_device
> >> *dev,
> >>return ERR_CAST(shmem);
> >>
> >>shmem->sgt = sgt;
> >> + shmem->imported_sgt = true;
> >
> >
> > I feel like adding more fields that can be used to do the is_imported()
> > check is going to be even more confusing. Can we instead have
> >
> > /* drm_gem_shmem_prime_import_sg_table() can be called from a
> > * driver specific ->import_sg_table() implementations that
> > * have extra failable initialization steps. Assign
> > * drm_gem_object::import_attach here (even though it's
> > * assigned in drm_gem_prime_import_dev()), so we don't end up
> > * with driver error paths calling drm_gem_shmem_free() with an
> > * imported sg_table assigned to drm_gem_shmem_object::sgt and
> > * drm_gem_object::import_attach left uninitialized.
> > */
> > shmem->base.import_attach = attach;
> >
> > here?
>
> AFAICT, this is not going to work because obj->import_attach will be
> released by drm_prime core by the time drm_gem_shmem_free() is invoked
> and drm_gem_shmem_free() uses obj->import_attach as well.
How can this happen? If something wrong happens in the driver-specific
->gem_prime_import_sg_table() implementation, drm_gem_shmem_free() will
be called before ->gem_prime_import_sg_table() returns, and the
attachment will only be released after that [1].
> I'll keep this
> patch around unless there will be other suggestions. To me the flag is
> good enough, I'll add a clarifying comment to the code in v16.
I really think this is a bad idea, for the same reasons I gave in my
reply to patch 2 (adding fields that need to be maintained when the
state can be inferred from other fields is error prone).
[1]https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_prime.c#L958
Re: [Intel-gfx] [PATCH 0/4] drm/amd/display: stop using drm_edid_override_connector_update()
On Fri, 1 Sept 2023 at 21:00, Alex Deucher wrote: > > On Thu, Aug 31, 2023 at 6:01 PM Alex Hung wrote: > > > > > > > > On 2023-08-30 01:29, Jani Nikula wrote: > > > On Tue, 29 Aug 2023, Alex Hung wrote: > > >> On 2023-08-29 11:03, Jani Nikula wrote: > > >>> On Tue, 29 Aug 2023, Jani Nikula wrote: > > On Tue, 29 Aug 2023, Alex Deucher wrote: > > > On Tue, Aug 29, 2023 at 6:48 AM Jani Nikula > > > wrote: > > >> > > >> On Wed, 23 Aug 2023, Jani Nikula wrote: > > >>> On Tue, 22 Aug 2023, Alex Hung wrote: > > On 2023-08-22 06:01, Jani Nikula wrote: > > > Over the past years I've been trying to unify the override and > > > firmware > > > EDID handling as well as EDID property updates. It won't work if > > > drivers > > > do their own random things. > > Let's check how to replace these references by appropriate ones or > > fork > > the function as reverting these patches causes regressions. > > >>> > > >>> I think the fundamental problem you have is conflating connector > > >>> forcing > > >>> with EDID override. They're orthogonal. The .force callback has no > > >>> business basing the decisions on connector->edid_override. Force is > > >>> force, override is override. > > >>> > > >>> The driver isn't even supposed to know or care if the EDID > > >>> originates > > >>> from the firmware loader or override EDID debugfs. drm_get_edid() > > >>> will > > >>> handle that for you transparently. It'll return the EDID, and you > > >>> shouldn't look at connector->edid_blob_ptr either. Using that will > > >>> make > > >>> future work in drm_edid.c harder. > > >>> > > >>> You can't fix that with minor tweaks. I think you'll be better off > > >>> starting from scratch. > > >>> > > >>> Also, connector->edid_override is debugfs. You actually can change > > >>> the > > >>> behaviour. If your userspace, whatever it is, has been written to > > >>> assume > > >>> connector forcing if EDID override is set, you *do* have to fix > > >>> that, > > >>> and set both. > > >> > > >> Any updates on fixing this, or shall we proceed with the reverts? > > >> > > >> There is a patch under internal reviews. It removes calls edid_override > > >> and drm_edid_override_connector_update as intended in this patchset but > > >> does not remove the functionality. > > > > > > While I am happy to hear there's progress, I'm somewhat baffled the > > > review is internal. The commits that I suggested to revert were also > > > only reviewed internally, as far as I can see... And that's kind of the > > > problem. > > > > > > Upstream code should be reviewed in public. > > > > Hi Jani, > > > > All patches are sent for public reviews, the progress is summarized as > > the followings: > > > > == internal == > > > > 1. a patch or patches are tested by CI. > > 2. internal technical and IP reviews are performed to ensure no concerns > > before patches are merged to internal branch. > > > > == public == > > > > 3. a regression test and IP reviews are performed by engineers before > > sending to public mailing lists. > > 4. the patchset is sent for public reviews ex. > > https://patchwork.freedesktop.org/series/122498/ > > 5. patches are merged to public repo. > > > > This sort of thing is fine for unreleased chips or new IP prior public > exposure, but for released hardware, you really need to do the reviews > on the mailing lists. Aye. Maybe with the clarification that if the embargoed code touches areas that are common code (or really should be handled in common code), then the cross-driver parts also need to be reviewed in public as upfront prep patches. If that's not possible (try to fix your process to make that possible please), at least ping stakeholders in private to give them a heads up, so that when the IP enabling gets published it's not going to be held up in the review for the necessary common changes. What's not good is if code that should be reviewed on dri-devel bypasses all that just because it's part of a hardware enabling series. Cheers, Sima > Alex > > > > > > > > > > > BR, > > > Jani. > > > > > > > > >> > > >> With the patch. both following git grep commands return nothing in > > >> amd-staging-drm-next. > > >> > > >> $ git grep drm_edid_override_connector_update -- drivers/gpu/drm/amd > > >> $ git grep edid_override -- drivers/gpu/drm/amd > > >> > > >> Best regards, > > >> Alex Hung > > >> > > > > > > What is the goal of the reverts? I don't disagree that we may be > > > using the interfaces wrong, but reverting them will regess > > > functionality in the driver. > > > > The commits are in v6.5-rc1, but not yet in a release. No user depends > > on them yet. I'd strongly prefer them not reaching v6.5 final and > > users. > > >>> > > >>> Sorry for confusion here, that's
Re: [Intel-gfx] [PATCH v15 02/23] drm/shmem-helper: Use flag for tracking page count bumped by get_pages_sgt()
On Sat, 2 Sep 2023 21:28:21 +0300
Dmitry Osipenko wrote:
> On 8/28/23 13:55, Boris Brezillon wrote:
> > On Sun, 27 Aug 2023 20:54:28 +0300
> > Dmitry Osipenko wrote:
> >
> >> Use separate flag for tracking page count bumped by shmem->sgt to avoid
> >> imbalanced page counter during of drm_gem_shmem_free() time. It's fragile
> >> to assume that populated shmem->pages at a freeing time means that the
> >> count was bumped by drm_gem_shmem_get_pages_sgt(), using a flag removes
> >> the ambiguity.
> >>
> >> Signed-off-by: Dmitry Osipenko
> >> ---
> >> drivers/gpu/drm/drm_gem_shmem_helper.c | 3 ++-
> >> drivers/gpu/drm/lima/lima_gem.c| 1 +
> >> include/drm/drm_gem_shmem_helper.h | 7 +++
> >> 3 files changed, 10 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c
> >> b/drivers/gpu/drm/drm_gem_shmem_helper.c
> >> index 78d9cf2355a5..db20b9123891 100644
> >> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
> >> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
> >> @@ -152,7 +152,7 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object
> >> *shmem)
> >>sg_free_table(shmem->sgt);
> >>kfree(shmem->sgt);
> >>}
> >> - if (shmem->pages)
> >> + if (shmem->got_sgt)
> >>drm_gem_shmem_put_pages(shmem);
> >
> > Can't we just move this drm_gem_shmem_put_pages() call in the
> > if (shmem->sgt) block?
>
> As you've seen in patch #1, the shmem->sgt may belong to imported dmabuf
> and pages aren't referenced in this case.
Unless I'm wrong, you're already in the if (!import_attach) branch
here, so shmem->sgt should not be a dmabuf sgt.
>
> I agree that the freeing code is confusing. The flags make it a better,
> not ideal. Though, the flags+comments solution is good enough to me.
But what's the point of adding a flag when you can just do an
if (!shmem->import_attach && shmem->sgt) check. At best, it just
confuses people as to what these fields mean/are used for (especially
when the field has such a generic name, when what you want is actually
something like ->got_sgt_for_non_imported_object). But the most
problematic aspect is that it adds fields to maintain, and those might
end up being inconsistent with the object state because
new/driver-specific code forgot to update them.
> Please let me know if you have more suggestions, otherwise I'll add
> comment to the code and keep this patch for v16.
I'd definitely prefer adding the following helper
static bool has_implicit_pages_ref(struct drm_gem_shmem_object *shmem)
{
return !shmem->import_attach && shmem->sgt;
}
which provides the same logic without adding a new field/flag.
>
> BTW, I realized that the new flag wasn't placed properly in the Lima
> driver, causing unbalanced page count in the error path. Will correct it
> in v16.
See, that's the sort of subtle bugs I'm talking about. If the state is
inferred from other fields that can't happen.
Re: [Intel-gfx] [PATCH v4 1/4] drm/i915/fbc: Clear frontbuffer busy bits on flip
Hi Jouni, On Fri, 2023-09-01 at 12:34 +0300, Jouni Högander wrote: > We are planning to move flush performed from work queue. This > means it is possible to have invalidate -> flip -> flush sequence. > Handle this by clearing possible busy bits on flip. > > Signed-off-by: Ville Syrjälä > Signed-off-by: Jouni Högander > --- > drivers/gpu/drm/i915/display/intel_fbc.c | 6 ++ > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c > b/drivers/gpu/drm/i915/display/intel_fbc.c > index 1c6d467cec26..817e5784660b 100644 > --- a/drivers/gpu/drm/i915/display/intel_fbc.c > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c > @@ -1307,11 +1307,9 @@ static void __intel_fbc_post_update(struct intel_fbc > *fbc) > lockdep_assert_held(>lock); > > fbc->flip_pending = false; > + fbc->busy_bits = 0; > > - if (!fbc->busy_bits) > - intel_fbc_activate(fbc); > - else > - intel_fbc_deactivate(fbc, "frontbuffer write"); > + intel_fbc_activate(fbc); Can you explain why the call to intel_fbc_deactivate() is not needed here anymore? I think it would be a good idea to explain that in the commit message. Or, at least, an explanation about it here, so it's documented. ;) -- Cheers, Luca.
[Intel-gfx] ✗ Fi.CI.BUILD: failure for drm/i915: Improve BW management on shared display links (rev3)
== Series Details == Series: drm/i915: Improve BW management on shared display links (rev3) URL : https://patchwork.freedesktop.org/series/122589/ State : failure == Summary == Error: patch https://patchwork.freedesktop.org/api/1.0/series/122589/revisions/3/mbox/ not applied Applying: drm/i915/dp: Factor out helpers to compute the link limits Using index info to reconstruct a base tree... M drivers/gpu/drm/i915/display/intel_dp.c M drivers/gpu/drm/i915/display/intel_dp_mst.c Falling back to patching base and 3-way merge... Auto-merging drivers/gpu/drm/i915/display/intel_dp_mst.c CONFLICT (content): Merge conflict in drivers/gpu/drm/i915/display/intel_dp_mst.c Auto-merging drivers/gpu/drm/i915/display/intel_dp.c error: Failed to merge in the changes. hint: Use 'git am --show-current-patch=diff' to see the failed patch Patch failed at 0001 drm/i915/dp: Factor out helpers to compute the link limits When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort". Build failed, no error log produced
Re: [Intel-gfx] [PATCH v2 3/6] drm_dbg: add trailing newlines to msgs
Hi Jim, On Sun, Sep 03, 2023 at 12:46:00PM -0600, Jim Cromie wrote: > By at least strong convention, a print-buffer's trailing newline says > "message complete, send it". The exception (no TNL, followed by a call > to pr_cont) proves the general rule. > > Most DRM.debug calls already comport with this: 207 DRM_DEV_DEBUG, > 1288 drm_dbg. Clean up the remainders, in maintainer sized chunks. > > No functional changes. > > Signed-off-by: Jim Cromie Reviewed-by: Andi Shyti Andi
