[PATCH v3 2/2] drm/bridge: it6505: Add pre_enable/post_disable callback
Add atomic_pre_enable and atomic_post_disable callback to make sure the
bridge is not powered off until atomic_post_disable is called. This
prevents a power leakage when it6505 is powered off, but the upstream
DRM bridge is still sending display signals.
Fixes: b5c84a9edcd4 ("drm/bridge: add it6505 driver")
Signed-off-by: Pin-yen Lin
Reviewed-by: AngeloGioacchino Del Regno
---
Changes in v3:
- Collect review tag
drivers/gpu/drm/bridge/ite-it6505.c | 24
1 file changed, 24 insertions(+)
diff --git a/drivers/gpu/drm/bridge/ite-it6505.c
b/drivers/gpu/drm/bridge/ite-it6505.c
index 050f3be9adbc..922e1f2b7a35 100644
--- a/drivers/gpu/drm/bridge/ite-it6505.c
+++ b/drivers/gpu/drm/bridge/ite-it6505.c
@@ -2991,6 +2991,28 @@ static void it6505_bridge_atomic_disable(struct
drm_bridge *bridge,
}
}
+static void it6505_bridge_atomic_pre_enable(struct drm_bridge *bridge,
+ struct drm_bridge_state *old_state)
+{
+ struct it6505 *it6505 = bridge_to_it6505(bridge);
+ struct device *dev = >client->dev;
+
+ DRM_DEV_DEBUG_DRIVER(dev, "start");
+
+ pm_runtime_get_sync(dev);
+}
+
+static void it6505_bridge_atomic_post_disable(struct drm_bridge *bridge,
+ struct drm_bridge_state
*old_state)
+{
+ struct it6505 *it6505 = bridge_to_it6505(bridge);
+ struct device *dev = >client->dev;
+
+ DRM_DEV_DEBUG_DRIVER(dev, "start");
+
+ pm_runtime_put_sync(dev);
+}
+
static enum drm_connector_status
it6505_bridge_detect(struct drm_bridge *bridge)
{
@@ -3025,6 +3047,8 @@ static const struct drm_bridge_funcs it6505_bridge_funcs
= {
.mode_valid = it6505_bridge_mode_valid,
.atomic_enable = it6505_bridge_atomic_enable,
.atomic_disable = it6505_bridge_atomic_disable,
+ .atomic_pre_enable = it6505_bridge_atomic_pre_enable,
+ .atomic_post_disable = it6505_bridge_atomic_post_disable,
.detect = it6505_bridge_detect,
.get_edid = it6505_bridge_get_edid,
};
--
2.38.0.rc1.362.ged0d419d3c-goog
[PATCH v3 1/2] drm/bridge: it6505: Adapt runtime power management framework
Use pm_runtime_(get|put)_sync to control the bridge power, and add
SET_SYSTEM_SLEEP_PM_OPS with pm_runtime_force_(suspend|resume) to it6505
driver. Without SET_SYSTEM_SLEEP_PM_OPS, the bridge will be powered on
unnecessarily when no external display is connected.
Fixes: b5c84a9edcd4 ("drm/bridge: add it6505 driver")
Signed-off-by: Pin-yen Lin
---
Changes in v3:
- Handle the error from extcon_get_state
Changes in v2:
- Handle the error from pm_runtime_get_sync in it6505_extcon_work
drivers/gpu/drm/bridge/ite-it6505.c | 34 -
1 file changed, 24 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ite-it6505.c
b/drivers/gpu/drm/bridge/ite-it6505.c
index 2bb957cffd94..050f3be9adbc 100644
--- a/drivers/gpu/drm/bridge/ite-it6505.c
+++ b/drivers/gpu/drm/bridge/ite-it6505.c
@@ -421,6 +421,7 @@ struct it6505 {
struct notifier_block event_nb;
struct extcon_dev *extcon;
struct work_struct extcon_wq;
+ int extcon_state;
enum drm_connector_status connector_status;
enum link_train_status link_state;
struct work_struct link_works;
@@ -2685,31 +2686,41 @@ static void it6505_extcon_work(struct work_struct *work)
{
struct it6505 *it6505 = container_of(work, struct it6505, extcon_wq);
struct device *dev = >client->dev;
- int state = extcon_get_state(it6505->extcon, EXTCON_DISP_DP);
- unsigned int pwroffretry = 0;
+ int state, ret;
if (it6505->enable_drv_hold)
return;
mutex_lock(>extcon_lock);
+ state = extcon_get_state(it6505->extcon, EXTCON_DISP_DP);
DRM_DEV_DEBUG_DRIVER(dev, "EXTCON_DISP_DP = 0x%02x", state);
- if (state > 0) {
+
+ if (state == it6505->extcon_state || unlikely(state < 0))
+ goto unlock;
+ it6505->extcon_state = state;
+ if (state) {
DRM_DEV_DEBUG_DRIVER(dev, "start to power on");
msleep(100);
- it6505_poweron(it6505);
+ ret = pm_runtime_get_sync(dev);
+
+ /*
+* On system resume, extcon_work can be triggered before
+* pm_runtime_force_resume re-enables runtime power management.
+* Handling the error here to make sure the bridge is powered
on.
+*/
+ if (ret)
+ it6505_poweron(it6505);
} else {
DRM_DEV_DEBUG_DRIVER(dev, "start to power off");
- while (it6505_poweroff(it6505) && pwroffretry++ < 5) {
- DRM_DEV_DEBUG_DRIVER(dev, "power off fail %d times",
-pwroffretry);
- }
+ pm_runtime_put_sync(dev);
drm_helper_hpd_irq_event(it6505->bridge.dev);
memset(it6505->dpcd, 0, sizeof(it6505->dpcd));
DRM_DEV_DEBUG_DRIVER(dev, "power off it6505 success!");
}
+unlock:
mutex_unlock(>extcon_lock);
}
@@ -3032,8 +3043,10 @@ static __maybe_unused int it6505_bridge_suspend(struct
device *dev)
return it6505_poweroff(it6505);
}
-static SIMPLE_DEV_PM_OPS(it6505_bridge_pm_ops, it6505_bridge_suspend,
-it6505_bridge_resume);
+static const struct dev_pm_ops it6505_bridge_pm_ops = {
+ SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
pm_runtime_force_resume)
+ SET_RUNTIME_PM_OPS(it6505_bridge_suspend, it6505_bridge_resume, NULL)
+};
static int it6505_init_pdata(struct it6505 *it6505)
{
@@ -3315,6 +3328,7 @@ static int it6505_i2c_probe(struct i2c_client *client,
DRM_DEV_DEBUG_DRIVER(dev, "it6505 device name: %s", dev_name(dev));
debugfs_init(it6505);
+ pm_runtime_enable(dev);
it6505->bridge.funcs = _bridge_funcs;
it6505->bridge.type = DRM_MODE_CONNECTOR_DisplayPort;
--
2.38.0.rc1.362.ged0d419d3c-goog
[PATCH v3 0/2] drm/bridge: it6505: Power management fixes for it6505 bridge
This series contains 2 fixes related to it6505 power management. Changes in v3: - Handle the error from extcon_get_state - Collect review tag Changes in v2: - Handle the error from pm_runtime_get_sync in it6505_extcon_work Pin-yen Lin (2): drm/bridge: it6505: Adapt runtime power management framework drm/bridge: it6505: Add pre_enable/post_disable callback drivers/gpu/drm/bridge/ite-it6505.c | 58 - 1 file changed, 48 insertions(+), 10 deletions(-) -- 2.38.0.rc1.362.ged0d419d3c-goog
Re: [PATCH 0/5] drm: Fix math issues in MSM DSC implementation
On 01-10-22, 21:08, Marijn Suijten wrote: > Various removals of complex yet unnecessary math, fixing all uses of > drm_dsc_config::bits_per_pixel to deal with the fact that this field > includes four fractional bits, and finally an approach for dealing with > dsi_host setting negative values in range_bpg_offset, resulting in > overflow inside drm_dsc_pps_payload_pack(). > > Note that updating the static bpg_offset array to limit the size of > these negative values to 6 bits changes what would be written to the DPU > hardware at register(s) DSC_RANGE_BPG_OFFSET, hence the choice has been > made to cover up for this while packing the value into a smaller field > instead. Thanks for fixing these. I dont have my pixel3 availble but changes lgtm Reviewed-by: Vinod Koul > Altogether this series is responsible for solving _all_ Display Stream > Compression issues and artifacts on the Sony Tama (sdm845) Akatsuki > smartphone (2880x1440p). Does it need two dsi lanes? -- ~Vinod
Re: linux-next: build failure after merge of the drm tree
Hi Dave,
On Tue, 4 Oct 2022 12:24:37 +1000 David Airlie wrote:
>
> On Tue, Oct 4, 2022 at 12:21 PM Stephen Rothwell
> wrote:
> >
> > On Fri, 30 Sep 2022 11:54:34 +0100 broo...@kernel.org wrote:
> > >
> > > After merging the drm tree, today's linux-next build (x86_64
> > > allmodconfig) failed like this:
> > >
> > > /tmp/next/build/drivers/gpu/drm/amd/amdgpu/../display/dc/core/dc_stream.c:
> > > In function 'dc_stream_remove_writeback':
> > > /tmp/next/build/drivers/gpu/drm/amd/amdgpu/../display/dc/core/dc_stream.c:527:55:
> > > error: array subscript [0, 0] is outside array bounds of 'struct
> > > dc_writeback_info[1]' [-Werror=array-bounds]
> > > 527 | stream->writeback_info[j] = stream->writeback_info[i];
> > > | ~~^~~
> > > cc1: all warnings being treated as errors
> > >
> > > Caused by
> > >
> > > 5d8c3e836fc224 ("drm/amd/display: fix array-bounds error in
> > > dc_stream_remove_writeback()")
> > >
> > > I have reverted that commit for today.
> >
> > I am still getting this failure. The full error is:
> >
> > drivers/gpu/drm/amd/amdgpu/../display/dc/core/dc_stream.c: In function
> > 'dc_stream_remove_writeback':
> > drivers/gpu/drm/amd/amdgpu/../display/dc/core/dc_stream.c:527:83: error:
> > array subscript [0, 0] is outside array bounds of 'struct
> > dc_writeback_info[1]' [-Werror=array-bounds]
> > 527 | stream->writeback_info[j] =
> > stream->writeback_info[i];
> > |
> > ~~^~~
> > In file included from drivers/gpu/drm/amd/amdgpu/../display/dc/dc.h:1269,
> > from
> > drivers/gpu/drm/amd/amdgpu/../display/dc/inc/core_types.h:29,
> > from
> > drivers/gpu/drm/amd/amdgpu/../display/dc/basics/dc_common.h:29,
> > from
> > drivers/gpu/drm/amd/amdgpu/../display/dc/core/dc_stream.c:27:
> > drivers/gpu/drm/amd/amdgpu/../display/dc/dc_stream.h:241:34: note: while
> > referencing 'writeback_info'
> > 241 | struct dc_writeback_info writeback_info[MAX_DWB_PIPES];
> > | ^~
>
> I'm not seeing it here, what gcc is this with?
I am using an x86_64 cross compiler hosted on ppc64le - gcc v11.2.0 (on
Debian).
--
Cheers,
Stephen Rothwell
pgp3GArTvG4G9.pgp
Description: OpenPGP digital signature
linux-next: manual merge of the fbdev tree with the drm tree
Hi all,
Today's linux-next merge of the fbdev tree got a conflict in:
drivers/video/fbdev/tridentfb.c
between commit:
145eed48de27 ("fbdev: Remove conflicting devices on PCI bus")
from the drm tree and commit:
d738bf0123d6 ("fbdev: tridentfb: Fix missing pci_disable_device() in probe
and remove")
from the fbdev tree.
I fixed it up (see below) and can carry the fix as necessary. This
is now fixed as far as linux-next is concerned, but any non trivial
conflicts should be mentioned to your upstream maintainer when your tree
is submitted for merging. You may also want to consider cooperating
with the maintainer of the conflicting tree to minimise any particularly
complex conflicts.
--
Cheers,
Stephen Rothwell
diff --cc drivers/video/fbdev/tridentfb.c
index f9c3b1d38fc2,4d08f4489a0a..
--- a/drivers/video/fbdev/tridentfb.c
+++ b/drivers/video/fbdev/tridentfb.c
@@@ -1471,11 -1465,7 +1466,11 @@@ static int trident_pci_probe(struct pci
int chip_id;
bool found = false;
+ err = aperture_remove_conflicting_pci_devices(dev, "tridentfb");
+ if (err)
+ return err;
+
- err = pci_enable_device(dev);
+ err = pcim_enable_device(dev);
if (err)
return err;
pgpJomFe73YBw.pgp
Description: OpenPGP digital signature
Re: linux-next: build failure after merge of the drm tree
On Tue, Oct 4, 2022 at 12:21 PM Stephen Rothwell wrote:
>
> Hi broo...@kernel.org,
>
> On Fri, 30 Sep 2022 11:54:34 +0100 broo...@kernel.org wrote:
> >
> > After merging the drm tree, today's linux-next build (x86_64
> > allmodconfig) failed like this:
> >
> > /tmp/next/build/drivers/gpu/drm/amd/amdgpu/../display/dc/core/dc_stream.c:
> > In function 'dc_stream_remove_writeback':
> > /tmp/next/build/drivers/gpu/drm/amd/amdgpu/../display/dc/core/dc_stream.c:527:55:
> > error: array subscript [0, 0] is outside array bounds of 'struct
> > dc_writeback_info[1]' [-Werror=array-bounds]
> > 527 | stream->writeback_info[j] = stream->writeback_info[i];
> > | ~~^~~
> > cc1: all warnings being treated as errors
> >
> > Caused by
> >
> > 5d8c3e836fc224 ("drm/amd/display: fix array-bounds error in
> > dc_stream_remove_writeback()")
> >
> > I have reverted that commit for today.
>
> I am still getting this failure. The full error is:
>
> drivers/gpu/drm/amd/amdgpu/../display/dc/core/dc_stream.c: In function
> 'dc_stream_remove_writeback':
> drivers/gpu/drm/amd/amdgpu/../display/dc/core/dc_stream.c:527:83: error:
> array subscript [0, 0] is outside array bounds of 'struct
> dc_writeback_info[1]' [-Werror=array-bounds]
> 527 | stream->writeback_info[j] =
> stream->writeback_info[i];
> |
> ~~^~~
> In file included from drivers/gpu/drm/amd/amdgpu/../display/dc/dc.h:1269,
> from
> drivers/gpu/drm/amd/amdgpu/../display/dc/inc/core_types.h:29,
> from
> drivers/gpu/drm/amd/amdgpu/../display/dc/basics/dc_common.h:29,
> from
> drivers/gpu/drm/amd/amdgpu/../display/dc/core/dc_stream.c:27:
> drivers/gpu/drm/amd/amdgpu/../display/dc/dc_stream.h:241:34: note: while
> referencing 'writeback_info'
> 241 | struct dc_writeback_info writeback_info[MAX_DWB_PIPES];
> | ^~
I'm not seeing it here, what gcc is this with?
Dave.
Re: linux-next: build failure after merge of the drm tree
Hi broo...@kernel.org,
On Fri, 30 Sep 2022 11:54:34 +0100 broo...@kernel.org wrote:
>
> After merging the drm tree, today's linux-next build (x86_64
> allmodconfig) failed like this:
>
> /tmp/next/build/drivers/gpu/drm/amd/amdgpu/../display/dc/core/dc_stream.c: In
> function 'dc_stream_remove_writeback':
> /tmp/next/build/drivers/gpu/drm/amd/amdgpu/../display/dc/core/dc_stream.c:527:55:
> error: array subscript [0, 0] is outside array bounds of 'struct
> dc_writeback_info[1]' [-Werror=array-bounds]
> 527 | stream->writeback_info[j] = stream->writeback_info[i];
> | ~~^~~
> cc1: all warnings being treated as errors
>
> Caused by
>
> 5d8c3e836fc224 ("drm/amd/display: fix array-bounds error in
> dc_stream_remove_writeback()")
>
> I have reverted that commit for today.
I am still getting this failure. The full error is:
drivers/gpu/drm/amd/amdgpu/../display/dc/core/dc_stream.c: In function
'dc_stream_remove_writeback':
drivers/gpu/drm/amd/amdgpu/../display/dc/core/dc_stream.c:527:83: error: array
subscript [0, 0] is outside array bounds of 'struct dc_writeback_info[1]'
[-Werror=array-bounds]
527 | stream->writeback_info[j] =
stream->writeback_info[i];
|
~~^~~
In file included from drivers/gpu/drm/amd/amdgpu/../display/dc/dc.h:1269,
from
drivers/gpu/drm/amd/amdgpu/../display/dc/inc/core_types.h:29,
from
drivers/gpu/drm/amd/amdgpu/../display/dc/basics/dc_common.h:29,
from
drivers/gpu/drm/amd/amdgpu/../display/dc/core/dc_stream.c:27:
drivers/gpu/drm/amd/amdgpu/../display/dc/dc_stream.h:241:34: note: while
referencing 'writeback_info'
241 | struct dc_writeback_info writeback_info[MAX_DWB_PIPES];
| ^~
I have reverted that commit again today.
--
Cheers,
Stephen Rothwell
pgpc97jrOp9N7.pgp
Description: OpenPGP digital signature
Re: [PATCH] Updated the orientation quirks to support the many variations of ONEXPLAYER and AokZoe devices.
Has anyone been able to review this? If possible I'd like to see this
pulled into 6.1
On Fri, Sep 16, 2022, 8:50 AM Matthew wrote:
> Signed-off-by: Matthew Anderson
> ---
> .../gpu/drm/drm_panel_orientation_quirks.c| 86 ++-
> 1 file changed, 85 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_panel_orientation_quirks.c
> b/drivers/gpu/drm/drm_panel_orientation_quirks.c
> index fc1728d46ac2..15203c134717 100644
> --- a/drivers/gpu/drm/drm_panel_orientation_quirks.c
> +++ b/drivers/gpu/drm/drm_panel_orientation_quirks.c
> @@ -115,6 +115,18 @@ static const struct drm_dmi_panel_orientation_data
> lcd1280x1920_rightside_up = {
> .orientation = DRM_MODE_PANEL_ORIENTATION_RIGHT_UP,
> };
>
> +static const struct drm_dmi_panel_orientation_data
> lcd800x1280_leftside_up = {
> + .width = 800,
> + .height = 1280,
> + .orientation = DRM_MODE_PANEL_ORIENTATION_LEFT_UP,
> +};
> +
> +static const struct drm_dmi_panel_orientation_data
> lcd1200x1920_leftside_up = {
> + .width = 1200,
> + .height = 1920,
> + .orientation = DRM_MODE_PANEL_ORIENTATION_LEFT_UP,
> +};
> +
> static const struct drm_dmi_panel_orientation_data
> lcd1600x2560_leftside_up = {
> .width = 1600,
> .height = 2560,
> @@ -128,6 +140,12 @@ static const struct dmi_system_id orientation_data[]
> = {
> DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "One S1003"),
> },
> .driver_data = (void *)_rightside_up,
> + }, {/* AOKZOE A1 AR07 */
> + .matches = {
> + DMI_EXACT_MATCH(DMI_SYS_VENDOR, "AOKZOE"),
> + DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "AOKZOE A1 AR07"),
> + },
> + .driver_data = (void *)_leftside_up,
> }, {/* Asus T100HA */
> .matches = {
> DMI_EXACT_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
> @@ -308,12 +326,78 @@ static const struct dmi_system_id orientation_data[]
> = {
> DMI_EXACT_MATCH(DMI_PRODUCT_VERSION, "Default string"),
> },
> .driver_data = (void *)_pro,
> - }, {/* OneXPlayer */
> + }, {/* OneXPlayer 800P Original DMI Values */
> + .matches = {
> + DMI_EXACT_MATCH(DMI_SYS_VENDOR, "ONE-NETBOOK TECHNOLOGY
> CO., LTD."),
> + DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "ONE XPLAYER"),
> + },
> + .driver_data = (void *)_leftside_up,
> + }, {/* OneXPlayer 1200P Original DMI Values */
> + .matches = {
> + DMI_EXACT_MATCH(DMI_SYS_VENDOR, "ONE-NETBOOK TECHNOLOGY
> CO., LTD."),
> + DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "ONE XPLAYER"),
> + },
> + .driver_data = (void *)_leftside_up,
> + }, {/* OneXPlayer 1600P Original DMI Values */
> .matches = {
> DMI_EXACT_MATCH(DMI_SYS_VENDOR, "ONE-NETBOOK TECHNOLOGY
> CO., LTD."),
> DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "ONE XPLAYER"),
> },
> .driver_data = (void *)_leftside_up,
> + }, {/* OneXPlayer Gundam Edition Bios Updated */
> + .matches = {
> + DMI_EXACT_MATCH(DMI_SYS_VENDOR, "ONE-NETBOOK"),
> + DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "ONEXPLAYER GUNDAM
> GA72"),
> + },
> + .driver_data = (void *)_leftside_up,
> + }, {/* ONEXPLAYER mini A07 800P Bios Updated */
> + .matches = {
> + DMI_EXACT_MATCH(DMI_SYS_VENDOR, "ONE-NETBOOK"),
> + DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "ONEXPLAYER mini A07"),
> + },
> + .driver_data = (void *)_leftside_up,
> + }, {/* ONEXPLAYER mini A07 1200P Bios Updated*/
> + .matches = {
> + DMI_EXACT_MATCH(DMI_SYS_VENDOR, "ONE-NETBOOK"),
> + DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "ONEXPLAYER mini A07"),
> + },
> + .driver_data = (void *)_leftside_up,
> + }, {/* ONEXPLAYER mini GA72 800P Bios Updated*/
> + .matches = {
> + DMI_EXACT_MATCH(DMI_SYS_VENDOR, "ONE-NETBOOK"),
> + DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "ONEXPLAYER mini
> GA72"),
> + },
> + .driver_data = (void *)_leftside_up,
> + }, {/* ONEXPLAYER mini GA72 1200P Bios Updated*/
> + .matches = {
> + DMI_EXACT_MATCH(DMI_SYS_VENDOR, "ONE-NETBOOK"),
> + DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "ONEXPLAYER mini
> GA72"),
> + },
> + .driver_data = (void *)_leftside_up,
> + }, {/* ONEXPLAYER mini GT72 800P Bios Updated*/
> + .matches = {
> + DMI_EXACT_MATCH(DMI_SYS_VENDOR, "ONE-NETBOOK"),
> + DMI_EXACT_MATCH(DMI_PRODUCT_NAME,
Re: [PATCH 2/5] drm/msm/dsi: Remove repeated calculation of slice_per_intf
On Sat, Oct 01, 2022 at 09:08:04PM +0200, Marijn Suijten wrote:
> slice_per_intf is already computed for intf_width, which holds the same
> value as hdisplay.
>
> Fixes: 08802f515c3c ("drm/msm/dsi: Add support for DSC configuration")
> Signed-off-by: Marijn Suijten
Reviewed-by: Bjorn Andersson
> ---
> drivers/gpu/drm/msm/dsi/dsi_host.c | 6 ++
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c
> b/drivers/gpu/drm/msm/dsi/dsi_host.c
> index e05bae647431..cb6f2fa11f58 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -842,7 +842,7 @@ static void dsi_ctrl_config(struct msm_dsi_host
> *msm_host, bool enable,
> static void dsi_update_dsc_timing(struct msm_dsi_host *msm_host, bool
> is_cmd_mode, u32 hdisplay)
> {
> struct drm_dsc_config *dsc = msm_host->dsc;
> - u32 reg, intf_width, reg_ctrl, reg_ctrl2;
> + u32 reg, reg_ctrl, reg_ctrl2;
> u32 slice_per_intf, total_bytes_per_intf;
> u32 pkt_per_line;
> u32 bytes_in_slice;
> @@ -851,8 +851,7 @@ static void dsi_update_dsc_timing(struct msm_dsi_host
> *msm_host, bool is_cmd_mod
> /* first calculate dsc parameters and then program
>* compress mode registers
>*/
> - intf_width = hdisplay;
> - slice_per_intf = DIV_ROUND_UP(intf_width, dsc->slice_width);
> + slice_per_intf = DIV_ROUND_UP(hdisplay, dsc->slice_width);
>
> /* If slice_per_pkt is greater than slice_per_intf
>* then default to 1. This can happen during partial
> @@ -861,7 +860,6 @@ static void dsi_update_dsc_timing(struct msm_dsi_host
> *msm_host, bool is_cmd_mod
> if (slice_per_intf > dsc->slice_count)
> dsc->slice_count = 1;
>
> - slice_per_intf = DIV_ROUND_UP(hdisplay, dsc->slice_width);
> bytes_in_slice = DIV_ROUND_UP(dsc->slice_width * dsc->bits_per_pixel,
> 8);
>
> dsc->slice_chunk_size = bytes_in_slice;
> --
> 2.37.3
>
Re: [PATCH 1/5] drm/msm/dsi: Remove useless math in DSC calculation
On Sat, Oct 01, 2022 at 09:08:03PM +0200, Marijn Suijten wrote:
> Multiplying a value by 2 and adding 1 to it always results in a value
> that is uneven, and that 1 gets truncated immediately when performing
> integer division by 2 again. There is no "rounding" possible here.
>
> Fixes: b9080324d6ca ("drm/msm/dsi: add support for dsc data")
> Signed-off-by: Marijn Suijten
Reviewed-by: Bjorn Andersson
> ---
> drivers/gpu/drm/msm/dsi/dsi_host.c | 7 +--
> 1 file changed, 1 insertion(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c
> b/drivers/gpu/drm/msm/dsi/dsi_host.c
> index 8e4bc586c262..e05bae647431 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -1864,12 +1864,7 @@ static int dsi_populate_dsc_params(struct
> drm_dsc_config *dsc)
> data = 2048 * (dsc->rc_model_size - dsc->initial_offset +
> num_extra_mux_bits);
> dsc->slice_bpg_offset = DIV_ROUND_UP(data, groups_total);
>
> - /* bpp * 16 + 0.5 */
> - data = dsc->bits_per_pixel * 16;
> - data *= 2;
> - data++;
> - data /= 2;
> - target_bpp_x16 = data;
> + target_bpp_x16 = dsc->bits_per_pixel * 16;
>
> data = (dsc->initial_xmit_delay * target_bpp_x16) / 16;
> final_value = dsc->rc_model_size - data + num_extra_mux_bits;
> --
> 2.37.3
>
Re: [PATCH v2 10/17] drm/i915/vm_bind: Abstract out common execbuf functions
Hi Niranjana,
On Mon, Oct 03, 2022 at 02:06:18PM -0700, Niranjana Vishwanathapura wrote:
> On Mon, Oct 03, 2022 at 05:53:37PM +0200, Andi Shyti wrote:
> > Hi Niranjana,
> >
> > [...]
> >
> > > + for_each_child(ce, child) {
> > > + err = intel_context_pin_ww(child, ww);
> > > + GEM_BUG_ON(err);/* perma-pinned should incr a counter */
> > > + }
> > > +
> > > + for_each_child(ce, child) {
> > > + err = eb_pin_timeline(child, throttle, nonblock);
> > > + if (err)
> > > + goto unwind;
> > > + ++i;
> > > + }
> >
> > any reason for having two separate for_each_child here?
> >
>
> This part is ported as is from i915_gem_execbuffer.c.
> Probably the author found it easy to unwind in case of error.
yes... yes... I know... but these hard copies are also a good
occasion to do some refactoring on the original code... but
anyway, let's keep this simple...
I forgot earlier:
Reviewed-by: Andi Shyti
Andi
[PATCH 2/2] drm/i915/slpc: Update the frequency debugfs
Read the values stored in the SLPC structures. Remove the
fields that are no longer valid (like RPS interrupts) as
well.
Signed-off-by: Vinay Belgaumkar
---
drivers/gpu/drm/i915/gt/intel_rps.c | 37 +
1 file changed, 37 insertions(+)
diff --git a/drivers/gpu/drm/i915/gt/intel_rps.c
b/drivers/gpu/drm/i915/gt/intel_rps.c
index 7b0f6b4cfe78..92ab9c36a504 100644
--- a/drivers/gpu/drm/i915/gt/intel_rps.c
+++ b/drivers/gpu/drm/i915/gt/intel_rps.c
@@ -2382,10 +2382,47 @@ static void rps_frequency_dump(struct intel_rps *rps,
struct drm_printer *p)
intel_gpu_freq(rps, rps->efficient_freq));
}
+static void slpc_frequency_dump(struct intel_rps *rps, struct drm_printer *p)
+{
+ struct intel_gt *gt = rps_to_gt(rps);
+ struct intel_uncore *uncore = gt->uncore;
+ struct intel_rps_freq_caps caps;
+ u32 pm_mask;
+
+ gen6_rps_get_freq_caps(rps, );
+ pm_mask = intel_uncore_read(uncore, GEN6_PMINTRMSK);
+
+ drm_printf(p, "PM MASK=0x%08x\n", pm_mask);
+ drm_printf(p, "pm_intrmsk_mbz: 0x%08x\n",
+ rps->pm_intrmsk_mbz);
+ drm_printf(p, "RPSTAT1: 0x%08x\n", intel_uncore_read(uncore,
GEN6_RPSTAT1));
+ drm_printf(p, "RPNSWREQ: %dMHz\n",
intel_rps_get_requested_frequency(rps));
+ drm_printf(p, "Lowest (RPN) frequency: %dMHz\n",
+ intel_gpu_freq(rps, caps.min_freq));
+ drm_printf(p, "Nominal (RP1) frequency: %dMHz\n",
+ intel_gpu_freq(rps, caps.rp1_freq));
+ drm_printf(p, "Max non-overclocked (RP0) frequency: %dMHz\n",
+ intel_gpu_freq(rps, caps.rp0_freq));
+ drm_printf(p, "Current freq: %d MHz\n",
+ intel_rps_get_requested_frequency(rps));
+ drm_printf(p, "Actual freq: %d MHz\n",
+ intel_rps_read_actual_frequency(rps));
+ drm_printf(p, "Min freq: %d MHz\n",
+ intel_rps_get_min_frequency(rps));
+ drm_printf(p, "Boost freq: %d MHz\n",
+ intel_rps_get_boost_frequency(rps));
+ drm_printf(p, "Max freq: %d MHz\n",
+ intel_rps_get_max_frequency(rps));
+ drm_printf(p,
+ "efficient (RPe) frequency: %d MHz\n",
+ intel_gpu_freq(rps, caps.rp1_freq));
+}
void gen6_rps_frequency_dump(struct intel_rps *rps, struct drm_printer *p)
{
if (!rps_uses_slpc(rps))
return rps_frequency_dump(rps, p);
+ else
+ return slpc_frequency_dump(rps, p);
}
static int set_max_freq(struct intel_rps *rps, u32 val)
--
2.35.1
[PATCH 1/2] drm/i915: Add a wrapper for frequency debugfs
Move it to the RPS source file.
Signed-off-by: Vinay Belgaumkar
---
drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c | 157 +---
drivers/gpu/drm/i915/gt/intel_rps.c | 169 ++
drivers/gpu/drm/i915/gt/intel_rps.h | 3 +
3 files changed, 173 insertions(+), 156 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c
b/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c
index 9fd4d9255a97..4319d6cdafe2 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c
@@ -344,162 +344,7 @@ void intel_gt_pm_frequency_dump(struct intel_gt *gt,
struct drm_printer *p)
drm_printf(p, "efficient (RPe) frequency: %d MHz\n",
intel_gpu_freq(rps, rps->efficient_freq));
} else if (GRAPHICS_VER(i915) >= 6) {
- u32 rp_state_limits;
- u32 gt_perf_status;
- struct intel_rps_freq_caps caps;
- u32 rpmodectl, rpinclimit, rpdeclimit;
- u32 rpstat, cagf, reqf;
- u32 rpcurupei, rpcurup, rpprevup;
- u32 rpcurdownei, rpcurdown, rpprevdown;
- u32 rpupei, rpupt, rpdownei, rpdownt;
- u32 pm_ier, pm_imr, pm_isr, pm_iir, pm_mask;
-
- rp_state_limits = intel_uncore_read(uncore,
GEN6_RP_STATE_LIMITS);
- gen6_rps_get_freq_caps(rps, );
- if (IS_GEN9_LP(i915))
- gt_perf_status = intel_uncore_read(uncore,
BXT_GT_PERF_STATUS);
- else
- gt_perf_status = intel_uncore_read(uncore,
GEN6_GT_PERF_STATUS);
-
- /* RPSTAT1 is in the GT power well */
- intel_uncore_forcewake_get(uncore, FORCEWAKE_ALL);
-
- reqf = intel_uncore_read(uncore, GEN6_RPNSWREQ);
- if (GRAPHICS_VER(i915) >= 9) {
- reqf >>= 23;
- } else {
- reqf &= ~GEN6_TURBO_DISABLE;
- if (IS_HASWELL(i915) || IS_BROADWELL(i915))
- reqf >>= 24;
- else
- reqf >>= 25;
- }
- reqf = intel_gpu_freq(rps, reqf);
-
- rpmodectl = intel_uncore_read(uncore, GEN6_RP_CONTROL);
- rpinclimit = intel_uncore_read(uncore, GEN6_RP_UP_THRESHOLD);
- rpdeclimit = intel_uncore_read(uncore, GEN6_RP_DOWN_THRESHOLD);
-
- rpstat = intel_uncore_read(uncore, GEN6_RPSTAT1);
- rpcurupei = intel_uncore_read(uncore, GEN6_RP_CUR_UP_EI) &
GEN6_CURICONT_MASK;
- rpcurup = intel_uncore_read(uncore, GEN6_RP_CUR_UP) &
GEN6_CURBSYTAVG_MASK;
- rpprevup = intel_uncore_read(uncore, GEN6_RP_PREV_UP) &
GEN6_CURBSYTAVG_MASK;
- rpcurdownei = intel_uncore_read(uncore, GEN6_RP_CUR_DOWN_EI) &
GEN6_CURIAVG_MASK;
- rpcurdown = intel_uncore_read(uncore, GEN6_RP_CUR_DOWN) &
GEN6_CURBSYTAVG_MASK;
- rpprevdown = intel_uncore_read(uncore, GEN6_RP_PREV_DOWN) &
GEN6_CURBSYTAVG_MASK;
-
- rpupei = intel_uncore_read(uncore, GEN6_RP_UP_EI);
- rpupt = intel_uncore_read(uncore, GEN6_RP_UP_THRESHOLD);
-
- rpdownei = intel_uncore_read(uncore, GEN6_RP_DOWN_EI);
- rpdownt = intel_uncore_read(uncore, GEN6_RP_DOWN_THRESHOLD);
-
- cagf = intel_rps_read_actual_frequency(rps);
-
- intel_uncore_forcewake_put(uncore, FORCEWAKE_ALL);
-
- if (GRAPHICS_VER(i915) >= 11) {
- pm_ier = intel_uncore_read(uncore,
GEN11_GPM_WGBOXPERF_INTR_ENABLE);
- pm_imr = intel_uncore_read(uncore,
GEN11_GPM_WGBOXPERF_INTR_MASK);
- /*
-* The equivalent to the PM ISR & IIR cannot be read
-* without affecting the current state of the system
-*/
- pm_isr = 0;
- pm_iir = 0;
- } else if (GRAPHICS_VER(i915) >= 8) {
- pm_ier = intel_uncore_read(uncore, GEN8_GT_IER(2));
- pm_imr = intel_uncore_read(uncore, GEN8_GT_IMR(2));
- pm_isr = intel_uncore_read(uncore, GEN8_GT_ISR(2));
- pm_iir = intel_uncore_read(uncore, GEN8_GT_IIR(2));
- } else {
- pm_ier = intel_uncore_read(uncore, GEN6_PMIER);
- pm_imr = intel_uncore_read(uncore, GEN6_PMIMR);
- pm_isr = intel_uncore_read(uncore, GEN6_PMISR);
- pm_iir = intel_uncore_read(uncore, GEN6_PMIIR);
- }
- pm_mask = intel_uncore_read(uncore, GEN6_PMINTRMSK);
-
- drm_printf(p, "Video Turbo Mode: %s\n",
- str_yes_no(rpmodectl & GEN6_RP_MEDIA_TURBO));
-
[PATCH 0/2] drm/i915/slpc: Update frequency debugfs for SLPC
Remove the RPS related information that is not valid when SLPC is enabled. Signed-off-by: Vinay Belgaumkar Vinay Belgaumkar (2): drm/i915: Add a wrapper for frequency debugfs drm/i915/slpc: Update the frequency debugfs drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c | 157 + drivers/gpu/drm/i915/gt/intel_rps.c | 206 ++ drivers/gpu/drm/i915/gt/intel_rps.h | 3 + 3 files changed, 210 insertions(+), 156 deletions(-) -- 2.35.1
Re: [Intel-gfx] [PATCH 7/7] drm/i915/hwmon: Extend power/energy for XEHPSDV
On Tue, Sep 27, 2022 at 11:20:20AM +0530, Badal Nilawar wrote: > From: Dale B Stimson > > Extend hwmon power/energy for XEHPSDV especially per gt level energy > usage. > > v2: Update to latest HWMON spec (Ashutosh) > v3: Fix review comments (Ashutosh) > v4: Fix review comments (Anshuman) > v5: s/hwmon_device_register_with_info/ > devm_hwmon_device_register_with_info/ (Ashutosh) > > Signed-off-by: Ashutosh Dixit > Signed-off-by: Dale B Stimson > Signed-off-by: Badal Nilawar > Acked-by: Guenter Roeck > Reviewed-by: Ashutosh Dixit > Reviewed-by: Anshuman Gupta This last patch is making me thing that the hwmon should have been under gt rather than under i915. We leave it to a later refactoring. Reviewed-by: Andi Shyti Thanks, Andi
Re: [Intel-gfx] [PATCH 6/7] drm/i915/hwmon: Expose power1_max_interval
Hi Badal,
> diff --git a/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
> b/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
> index f9d6d3b08bba..19b9fe3ef237 100644
> --- a/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
> +++ b/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
> @@ -26,6 +26,15 @@ Description: RO. Card default power limit (default
> TDP setting).
>
> Only supported for particular Intel i915 graphics platforms.
>
> +What:/sys/devices/.../hwmon/hwmon/power1_max_interval
> +Date:February 2023
> +KernelVersion: 6.2
> +Contact: dri-devel@lists.freedesktop.org
same question here.
> +Description: RW. Sustained power limit interval (Tau in PL1/Tau) in
> + milliseconds over which sustained power is averaged.
> +
> + Only supported for particular Intel i915 graphics platforms.
> +
> What:/sys/devices/.../hwmon/hwmon/power1_crit
> Date:February 2023
> KernelVersion: 6.2
> diff --git a/drivers/gpu/drm/i915/i915_hwmon.c
> b/drivers/gpu/drm/i915/i915_hwmon.c
> index 2394fa789793..641143956c45 100644
> --- a/drivers/gpu/drm/i915/i915_hwmon.c
> +++ b/drivers/gpu/drm/i915/i915_hwmon.c
> @@ -20,11 +20,13 @@
> * - power - microwatts
> * - curr - milliamperes
> * - energy - microjoules
> + * - time - milliseconds
> */
> #define SF_VOLTAGE 1000
> #define SF_POWER 100
> #define SF_CURR 1000
> #define SF_ENERGY100
> +#define SF_TIME 1000
>
> struct hwm_reg {
> i915_reg_t gt_perf_status;
> @@ -53,6 +55,7 @@ struct i915_hwmon {
> struct hwm_reg rg;
> int scl_shift_power;
> int scl_shift_energy;
> + int scl_shift_time;
> };
>
> static void
> @@ -161,6 +164,115 @@ hwm_energy(struct hwm_drvdata *ddat, long *energy)
> return 0;
> }
>
> +static ssize_t
> +hwm_power1_max_interval_show(struct device *dev, struct device_attribute
> *attr,
> + char *buf)
> +{
> + struct hwm_drvdata *ddat = dev_get_drvdata(dev);
> + struct i915_hwmon *hwmon = ddat->hwmon;
> + intel_wakeref_t wakeref;
> + u32 r, x, y, x_w = 2; /* 2 bits */
> + u64 tau4, out;
> +
> + with_intel_runtime_pm(ddat->uncore->rpm, wakeref)
> + r = intel_uncore_read(ddat->uncore, hwmon->rg.pkg_rapl_limit);
> +
> + x = REG_FIELD_GET(PKG_PWR_LIM_1_TIME_X, r);
> + y = REG_FIELD_GET(PKG_PWR_LIM_1_TIME_Y, r);
> + /*
> + * tau = 1.x * power(2,y), x = bits(23:22), y = bits(21:17)
> + * = (4 | x) << (y - 2)
> + * where (y - 2) ensures a 1.x fixed point representation of 1.x
> + * However because y can be < 2, we compute
> + * tau4 = (4 | x) << y
> + * but add 2 when doing the final right shift to account for units
> + */
> + tau4 = ((1 << x_w) | x) << y;
> + /* val in hwmon interface units (millisec) */
> + out = mul_u64_u32_shr(tau4, SF_TIME, hwmon->scl_shift_time + x_w);
> +
> + return sysfs_emit(buf, "%llu\n", out);
> +}
> +
> +static ssize_t
> +hwm_power1_max_interval_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct hwm_drvdata *ddat = dev_get_drvdata(dev);
> + struct i915_hwmon *hwmon = ddat->hwmon;
> + long val, max_win, ret;
you have some type mismatch here:
- val should be unsigned long
- max_win should be u64
- ret should be int
> + u32 x, y, rxy, x_w = 2; /* 2 bits */
> + u64 tau4, r;
> +
> +#define PKG_MAX_WIN_DEFAULT 0x12ull
could you please add a comment here?
> +
> + ret = kstrtoul(buf, 0, );
> + if (ret)
> + return ret;
> +
> + /*
> + * val must be < max in hwmon interface units. The steps below are
> + * explained in i915_power1_max_interval_show()
> + */
> + r = FIELD_PREP(PKG_MAX_WIN, PKG_MAX_WIN_DEFAULT);
> +
> + x = REG_FIELD_GET(PKG_MAX_WIN_X, r);
> + y = REG_FIELD_GET(PKG_MAX_WIN_Y, r);
> + tau4 = ((1 << x_w) | x) << y;
> + max_win = mul_u64_u32_shr(tau4, SF_TIME, hwmon->scl_shift_time + x_w);
> +
> + if (val > max_win)
> + return -EINVAL;
> +
> + /* val in hw units */
> + val = DIV_ROUND_CLOSEST_ULL((u64)val << hwmon->scl_shift_time, SF_TIME);
> + /* Convert to 1.x * power(2,y) */
> + if (!val)
> + return -EINVAL;
> + y = ilog2(val);
> + /* x = (val - (1 << y)) >> (y - 2); */
some leftover
> + x = (val - (1ul << y)) << x_w >> y;
> +
> + rxy = REG_FIELD_PREP(PKG_PWR_LIM_1_TIME_X, x) |
> REG_FIELD_PREP(PKG_PWR_LIM_1_TIME_Y, y);
> +
> + hwm_locked_with_pm_intel_uncore_rmw(ddat, hwmon->rg.pkg_rapl_limit,
> + PKG_PWR_LIM_1_TIME, rxy);
> + return count;
> +}
> +
> +static SENSOR_DEVICE_ATTR(power1_max_interval, 0664,
> +
Re: [Intel-gfx] [PATCH 5/7] drm/i915/hwmon: Expose card reactive critical power
Hi Badal, [...] > diff --git a/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon > b/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon > index 7525db243d74..f9d6d3b08bba 100644 > --- a/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon > +++ b/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon > @@ -26,6 +26,32 @@ Description: RO. Card default power limit (default > TDP setting). > > Only supported for particular Intel i915 graphics platforms. > > +What:/sys/devices/.../hwmon/hwmon/power1_crit > +Date:February 2023 > +KernelVersion: 6.2 > +Contact: dri-devel@lists.freedesktop.org > +Description: RW. Card reactive critical (I1) power limit in microwatts. > + > + Card reactive critical (I1) power limit in microwatts is exposed > + for client products. The power controller will throttle the > + operating frequency if the power averaged over a window exceeds > + this limit. > + > + Only supported for particular Intel i915 graphics platforms. > + > +What:/sys/devices/.../hwmon/hwmon/curr1_crit > +Date:February 2023 > +KernelVersion: 6.2 > +Contact: dri-devel@lists.freedesktop.org same question here, why dri-devel and not intel-gfx? Andi > +Description: RW. Card reactive critical (I1) power limit in milliamperes. > + > + Card reactive critical (I1) power limit in milliamperes is > + exposed for server products. The power controller will throttle > + the operating frequency if the power averaged over a window > + exceeds this limit. > + > + Only supported for particular Intel i915 graphics platforms. > + > What:/sys/devices/.../hwmon/hwmon/energy1_input > Date:February 2023 > KernelVersion: 6.2
Re: [Intel-gfx] [PATCH 4/7] drm/i915/hwmon: Show device level energy usage
Hi Badal,
[...]
> > diff --git a/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
> > b/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
> > index 16e697b1db3d..7525db243d74 100644
> > --- a/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
> > +++ b/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
> > @@ -25,3 +25,11 @@ Contact: dri-devel@lists.freedesktop.org
> > Description: RO. Card default power limit (default TDP setting).
> >
> > Only supported for particular Intel i915 graphics platforms.
> > +
> > +What: /sys/devices/.../hwmon/hwmon/energy1_input
> > +Date: February 2023
> > +KernelVersion: 6.2
> > +Contact: dri-devel@lists.freedesktop.org
>
> I'm sorry for being late on the review here, and I know that others
> already looked at the date and other details here in this doc.
> So I'm curious why we have decided for the dri-devel mailing list
> and not for the intel-gfx since intel-gfx is the only one we have
> listed for i915 dir in the MAINTAINERS file:
> L: intel-...@lists.freedesktop.org
same question here.
> > +Description: RO. Energy input of device in microjoules.
> > +
> > + Only supported for particular Intel i915 graphics platforms.
[...]
> > +/*
> > + * hwm_energy - Obtain energy value
> > + *
> > + * The underlying energy hardware register is 32-bits and is subject to
> > + * overflow. How long before overflow? For example, with an example
> > + * scaling bit shift of 14 bits (see register *PACKAGE_POWER_SKU_UNIT) and
> > + * a power draw of 1000 watts, the 32-bit counter will overflow in
> > + * approximately 4.36 minutes.
> > + *
> > + * Examples:
> > + *1 watt: (2^32 >> 14) /1 W / (60 * 60 * 24) secs/day -> 3 days
> > + * 1000 watts: (2^32 >> 14) / 1000 W / 60 secs/min -> 4.36
> > minutes
> > + *
> > + * The function significantly increases overflow duration (from 4.36
> > + * minutes) by accumulating the energy register into a 'long' as allowed by
> > + * the hwmon API. Using x86_64 128 bit arithmetic (see mul_u64_u32_shr()),
> > + * a 'long' of 63 bits, SF_ENERGY of 1e6 (~20 bits) and
> > + * hwmon->scl_shift_energy of 14 bits we have 57 (63 - 20 + 14) bits before
> > + * energy1_input overflows. This at 1000 W is an overflow duration of 278
> > years.
> > + */
> > +static int
> > +hwm_energy(struct hwm_drvdata *ddat, long *energy)
This function can just be void.
Andi
> > +{
[...]
Re: [Intel-gfx] [PATCH 12/16] drm/i915/vm_bind: Implement I915_GEM_EXECBUFFER3 ioctl
On Thu, Sep 29, 2022 at 09:02:01AM -0700, Niranjana Vishwanathapura wrote:
On Thu, Sep 29, 2022 at 04:00:47PM +0100, Matthew Auld wrote:
On 28/09/2022 07:19, Niranjana Vishwanathapura wrote:
Implement new execbuf3 ioctl (I915_GEM_EXECBUFFER3) which only
works in vm_bind mode. The vm_bind mode only works with
this new execbuf3 ioctl.
The new execbuf3 ioctl will not have any list of objects to validate
bind as all required objects binding would have been requested by the
userspace before submitting the execbuf3.
Legacy features like relocations etc are not supported by execbuf3.
Signed-off-by: Niranjana Vishwanathapura
Signed-off-by: Andi Shyti
---
drivers/gpu/drm/i915/Makefile | 1 +
.../gpu/drm/i915/gem/i915_gem_execbuffer3.c | 571 ++
drivers/gpu/drm/i915/gem/i915_gem_ioctls.h| 2 +
drivers/gpu/drm/i915/i915_driver.c| 1 +
include/uapi/drm/i915_drm.h | 61 ++
5 files changed, 636 insertions(+)
create mode 100644 drivers/gpu/drm/i915/gem/i915_gem_execbuffer3.c
diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index bf952f478555..3473ee5825bb 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -150,6 +150,7 @@ gem-y += \
gem/i915_gem_domain.o \
gem/i915_gem_execbuffer_common.o \
gem/i915_gem_execbuffer.o \
+ gem/i915_gem_execbuffer3.o \
gem/i915_gem_internal.o \
gem/i915_gem_object.o \
gem/i915_gem_lmem.o \
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer3.c
b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer3.c
new file mode 100644
index ..92af88bc8deb
--- /dev/null
+++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer3.c
@@ -0,0 +1,571 @@
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright © 2022 Intel Corporation
+ */
+
+#include
+#include
+
+#include
+
+#include "gt/intel_context.h"
+#include "gt/intel_gpu_commands.h"
+#include "gt/intel_gt.h"
+
+#include "i915_drv.h"
+#include "i915_gem_context.h"
+#include "i915_gem_execbuffer_common.h"
+#include "i915_gem_ioctls.h"
+#include "i915_gem_vm_bind.h"
+#include "i915_trace.h"
+
+#define __EXEC3_ENGINE_PINNED BIT_ULL(32)
+#define __EXEC3_INTERNAL_FLAGS (~0ull << 32)
+
+/* Catch emission of unexpected errors for CI! */
+#if IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM)
+#undef EINVAL
+#define EINVAL ({ \
+ DRM_DEBUG_DRIVER("EINVAL at %s:%d\n", __func__, __LINE__); \
+ 22; \
+})
+#endif
+
+/**
+ * DOC: User command execution with execbuf3 ioctl
+ *
+ * A VM in VM_BIND mode will not support older execbuf mode of binding.
+ * The execbuf ioctl handling in VM_BIND mode differs significantly from the
+ * older execbuf2 ioctl (See struct drm_i915_gem_execbuffer2).
+ * Hence, a new execbuf3 ioctl has been added to support VM_BIND mode. (See
+ * struct drm_i915_gem_execbuffer3). The execbuf3 ioctl will not accept any
+ * execlist. Hence, no support for implicit sync.
+ *
+ * The new execbuf3 ioctl only works in VM_BIND mode and the VM_BIND mode only
+ * works with execbuf3 ioctl for submission.
+ *
+ * The execbuf3 ioctl directly specifies the batch addresses instead of as
+ * object handles as in execbuf2 ioctl. The execbuf3 ioctl will also not
+ * support many of the older features like in/out/submit fences, fence array,
+ * default gem context etc. (See struct drm_i915_gem_execbuffer3).
+ *
+ * In VM_BIND mode, VA allocation is completely managed by the user instead of
+ * the i915 driver. Hence all VA assignment, eviction are not applicable in
+ * VM_BIND mode. Also, for determining object activeness, VM_BIND mode will not
+ * be using the i915_vma active reference tracking. It will instead check the
+ * dma-resv object's fence list for that.
+ *
+ * So, a lot of code supporting execbuf2 ioctl, like relocations, VA evictions,
+ * vma lookup table, implicit sync, vma active reference tracking etc., are not
+ * applicable for execbuf3 ioctl.
+ */
+
+/**
+ * struct i915_execbuffer - execbuf struct for execbuf3
+ * @i915: reference to the i915 instance we run on
+ * @file: drm file reference
+ * args: execbuf3 ioctl structure
+ * @gt: reference to the gt instance ioctl submitted for
+ * @context: logical state for the request
+ * @gem_context: callers context
+ * @requests: requests to be build
+ * @composite_fence: used for excl fence in dma_resv objects when > 1 BB
submitted
+ * @ww: i915_gem_ww_ctx instance
+ * @num_batches: number of batches submitted
+ * @batch_addresses: addresses corresponds to the submitted batches
+ * @batches: references to the i915_vmas corresponding to the batches
+ */
+struct i915_execbuffer {
+ struct drm_i915_private *i915;
+ struct drm_file *file;
+ struct drm_i915_gem_execbuffer3 *args;
+
+ struct intel_gt *gt;
+ struct intel_context *context;
+ struct i915_gem_context *gem_context;
+
+ struct i915_request *requests[MAX_ENGINE_INSTANCE + 1];
+ struct dma_fence
Re: [PATCH v2 10/17] drm/i915/vm_bind: Abstract out common execbuf functions
On Mon, Oct 03, 2022 at 05:53:37PM +0200, Andi Shyti wrote:
Hi Niranjana,
[...]
+ for_each_child(ce, child) {
+ err = intel_context_pin_ww(child, ww);
+ GEM_BUG_ON(err);/* perma-pinned should incr a counter */
+ }
+
+ for_each_child(ce, child) {
+ err = eb_pin_timeline(child, throttle, nonblock);
+ if (err)
+ goto unwind;
+ ++i;
+ }
any reason for having two separate for_each_child here?
This part is ported as is from i915_gem_execbuffer.c.
Probably the author found it easy to unwind in case of error.
Regards,
Niranjana
Andi
+ err = eb_pin_timeline(ce, throttle, nonblock);
+ if (err)
+ goto unwind;
+
+ return 0;
+
+unwind:
+ for_each_child(ce, child) {
+ if (j++ < i) {
+ mutex_lock(>timeline->mutex);
+ intel_context_exit(child);
+ mutex_unlock(>timeline->mutex);
+ }
+ }
+ for_each_child(ce, child)
+ intel_context_unpin(child);
[...]
Re: [Intel-gfx] [PATCH 3/7] drm/i915/hwmon: Power PL1 limit and TDP setting
Hi Badal,
[...]
> hwm_get_preregistration_info(struct drm_i915_private *i915)
> {
> struct i915_hwmon *hwmon = i915->hwmon;
> + struct intel_uncore *uncore = >uncore;
> + intel_wakeref_t wakeref;
> + u32 val_sku_unit;
>
> - if (IS_DG1(i915) || IS_DG2(i915))
> + if (IS_DG1(i915) || IS_DG2(i915)) {
> hwmon->rg.gt_perf_status = GEN12_RPSTAT1;
> - else
> + hwmon->rg.pkg_power_sku_unit = PCU_PACKAGE_POWER_SKU_UNIT;
> + hwmon->rg.pkg_power_sku = PCU_PACKAGE_POWER_SKU;
> + hwmon->rg.pkg_rapl_limit = PCU_PACKAGE_RAPL_LIMIT;
> + } else {
> hwmon->rg.gt_perf_status = INVALID_MMIO_REG;
> + hwmon->rg.pkg_power_sku_unit = INVALID_MMIO_REG;
> + hwmon->rg.pkg_power_sku = INVALID_MMIO_REG;
> + hwmon->rg.pkg_rapl_limit = INVALID_MMIO_REG;
> + }
> +
> + with_intel_runtime_pm(uncore->rpm, wakeref) {
> + /*
> + * The contents of register hwmon->rg.pkg_power_sku_unit do not
> change,
> + * so read it once and store the shift values.
> + */
> + if (i915_mmio_reg_valid(hwmon->rg.pkg_power_sku_unit)) {
> + val_sku_unit = intel_uncore_read(uncore,
> +
> hwmon->rg.pkg_power_sku_unit);
> + } else {
> + val_sku_unit = 0;
> + }
please remove the brackets here and, just a small nitpick:
move val_sky_unit inside the "with_intel_runtime_pm()" and
initialize it to '0', you will save the else statement.
Other than that:
Reviewed-by: Andi Shyti
Thanks,
Andi
Re: [Intel-gfx] [PATCH 2/7] drm/i915/hwmon: Add HWMON current voltage support
Hi Badal,
[...]
> static void
> hwm_get_preregistration_info(struct drm_i915_private *i915)
> {
> + struct i915_hwmon *hwmon = i915->hwmon;
> +
> + if (IS_DG1(i915) || IS_DG2(i915))
why not GRAPHICS_VER(i915) >= 12 here?
Andi
> + hwmon->rg.gt_perf_status = GEN12_RPSTAT1;
> + else
> + hwmon->rg.gt_perf_status = INVALID_MMIO_REG;
> }
>
> void i915_hwmon_register(struct drm_i915_private *i915)
> --
> 2.25.1
Re: [Intel-gfx] [PATCH 1/7] drm/i915/hwmon: Add HWMON infrastructure
Hi Badal, On Tue, Sep 27, 2022 at 11:20:14AM +0530, Badal Nilawar wrote: > From: Dale B Stimson > > The i915 HWMON module will be used to expose voltage, power and energy > values for dGfx. Here we set up i915 hwmon infrastructure including i915 > hwmon registration, basic data structures and functions. > > v2: > - Create HWMON infra patch (Ashutosh) > - Fixed review comments (Jani) > - Remove "select HWMON" from i915/Kconfig (Jani) > v3: Use hwm_ prefix for static functions (Ashutosh) > v4: s/#ifdef CONFIG_HWMON/#if IS_REACHABLE(CONFIG_HWMON)/ since the former > doesn't work if hwmon is compiled as a module (Guenter) > v5: Fixed review comments (Jani) > v6: s/kzalloc/devm_kzalloc/ (Andi) > v7: s/hwmon_device_register_with_info/ > devm_hwmon_device_register_with_info/ (Ashutosh) > > Cc: Guenter Roeck > Signed-off-by: Dale B Stimson > Signed-off-by: Ashutosh Dixit > Signed-off-by: Riana Tauro > Signed-off-by: Badal Nilawar > Acked-by: Guenter Roeck > Reviewed-by: Ashutosh Dixit > Reviewed-by: Anshuman Gupta Reviewed-by: Andi Shyti Andi
Re: [PATCH] drm/panel: ws2401: Silent no spi_device_id warning
On Thu, Sep 15, 2022 at 6:17 PM Wei Yongjun wrote: > From: Wei Yongjun > > Add spi_device_id entries to silent following SPI warning: > > SPI driver ws2401-panel has no spi_device_id for samsung,lms380kf01 > > Signed-off-by: Wei Yongjun Patch applied. Yours, Linus Walleij
Re: [PATCH] drm/panel: tpg110: Silent no spi_device_id warning
On Thu, Sep 15, 2022 at 6:17 PM Wei Yongjun wrote: > From: Wei Yongjun > > Add spi_device_id entries to silent following SPI warning: > > SPI driver tpo-tpg110-panel has no spi_device_id for tpo,tpg110 > > Signed-off-by: Wei Yongjun Patch applied. Yours, Linus Walleij
Re: [PATCH] drm/panel: db7430: Silent no spi_device_id warning
On Thu, Sep 15, 2022 at 6:14 PM Wei Yongjun wrote: > From: Wei Yongjun > > Add spi_device_id entries to silent following SPI warning: > > SPI driver db7430-panel has no spi_device_id for samsung,lms397kf04 > > Signed-off-by: Wei Yongjun Patch applied. Yours, Linus Walleij
Re: [Intel-gfx] [PATCH v2 14/14] drm/i915/mtl: Add multicast steering for media GT
On Mon, Oct 03, 2022 at 09:56:18AM +0100, Tvrtko Ursulin wrote:
>
> Hi Matt,
>
> On 01/10/2022 01:45, Matt Roper wrote:
> > MTL's media GT only has a single type of steering ("OAADDRM") which
> > selects between media slice 0 and media slice 1. We'll always steer to
> > media slice 0 unless it is fused off (which is the case when VD0, VE0,
> > and SFC0 are all reported as unavailable).
> >
> > Bspec: 67789
> > Signed-off-by: Matt Roper
> > ---
> > drivers/gpu/drm/i915/gt/intel_gt_mcr.c | 19 +--
> > drivers/gpu/drm/i915/gt/intel_gt_types.h| 1 +
> > drivers/gpu/drm/i915/gt/intel_workarounds.c | 18 +-
> > 3 files changed, 35 insertions(+), 3 deletions(-)
>
> [snip]
>
> > +static void
> > +mtl_media_gt_workarounds_init(struct intel_gt *gt, struct i915_wa_list
> > *wal)
> > +{
> > + /*
> > +* Unlike older platforms, we no longer setup implicit steering here;
> > +* all MCR accesses are explicitly steered.
> > +*/
> > + if (drm_debug_enabled(DRM_UT_DRIVER)) {
> > + struct drm_printer p = drm_debug_printer("MCR Steering:");
> > +
> > + intel_gt_mcr_report_steering(, gt, false);
> > + }
> > +}
> > +
> > static void
> > gt_init_workarounds(struct intel_gt *gt, struct i915_wa_list *wal)
> > {
> > struct drm_i915_private *i915 = gt->i915;
> > - if (IS_METEORLAKE(i915) && gt->type == GT_PRIMARY)
> > + if (IS_METEORLAKE(i915) && gt->type == GT_MEDIA)
> > + mtl_media_gt_workarounds_init(gt, wal);
> > + else if (IS_METEORLAKE(i915) && gt->type == GT_PRIMARY)
> > mtl_3d_gt_workarounds_init(gt, wal);
> > else if (IS_PONTEVECCHIO(i915))
> > pvc_gt_workarounds_init(gt, wal);
>
> Casually reading only - wouldn't it be nicer if the if-ladder in here
> (gt_init_workarounds) would have a single case per platform, and then you'd
> fork further (3d vs media) in MTL specific function?
Actually, that reminds me that we probably need to change this in a
different direction --- starting with MTL, we should stop tying
workarounds to the platform (IS_METEORLAKE) but rather tie them to the
IP version (i.e., GRAPHICS_VER or MEDIA_VER) since in the future the
same chiplets can potentially be re-used on multiple platforms.
Conversely, you could also potentially have variants of the same
"platform" (e.g., MTL) that incorporate chiplets with different IP
versions (and thus need distinct lists of workarounds and such).
At the moment MTL is the only platform we have with the standalone media
design so there's no potential for mix-and-match of chiplets yet, and
IS_METEORLAKE works fine in the short term, but we do need to start
planning ahead and moving off of platform checks in areas of the driver
like this.
>
> Also, series ends up with mtl_media_gt_workarounds_init and
> mtl_3d_gt_workarounds_init apparently 100% identical. You will need two
> copies in the future?
Yes, the two GTs are expected to end up with completely different sets
of workarounds once the platform is enabled. We've just been delaying
on actually sending the new MTL workarounds upstream to give the
workaround database a bit more time to settle.
Matt
>
> Regards,
>
> Tvrtko
--
Matt Roper
Graphics Software Engineer
VTT-OSGC Platform Enablement
Intel Corporation
Re: [PATCH] drm: amd: clean up dcn32_fpu.c kernel-doc
On 2022-10-01 00:33, Randy Dunlap wrote: Rectify multiple kernel-doc warnings in dcn32_fpu.c. E.g.: drivers/gpu/drm/amd/amdgpu/../display/dc/dml/dcn32/dcn32_fpu.c:247: warning: This comment starts with '/**', but isn't a kernel-doc comment. Refer Documentation/doc-guide/kernel-doc.rst * Finds dummy_latency_index when MCLK switching using firmware based drivers/gpu/drm/amd/amdgpu/../display/dc/dml/dcn32/dcn32_fpu.c:484: warning: Function parameter or member 'phantom_stream' not described in 'dcn32_set_phantom_stream_timing' drivers/gpu/drm/amd/amdgpu/../display/dc/dml/dcn32/dcn32_fpu.c:601: warning: Function parameter or member 'dc' not described in 'dcn32_assign_subvp_pipe' drivers/gpu/drm/amd/amdgpu/../display/dc/dml/dcn32/dcn32_fpu.c:601: warning: Function parameter or member 'context' not described in 'dcn32_assign_subvp_pipe' drivers/gpu/drm/amd/amdgpu/../display/dc/dml/dcn32/dcn32_fpu.c:601: warning: Function parameter or member 'index' not described in 'dcn32_assign_subvp_pipe' drivers/gpu/drm/amd/amdgpu/../display/dc/dml/dcn32/dcn32_fpu.c:2140: warning: Function parameter or member 'dc' not described in 'dcn32_update_bw_bounding_box_fpu' drivers/gpu/drm/amd/amdgpu/../display/dc/dml/dcn32/dcn32_fpu.c:2140: warning: Function parameter or member 'bw_params' not described in 'dcn32_update_bw_bounding_box_fpu' drivers/gpu/drm/amd/amdgpu/../display/dc/dml/dcn32/dcn32_fpu.c:2140: warning: expecting prototype for dcn32_update_bw_bounding_box(). Prototype was for dcn32_update_bw_bounding_box_fpu() instead Reported-by: kernel test robot Signed-off-by: Randy Dunlap Cc: George Shen Cc: Alvin Lee Cc: Nevenko Stupar Cc: Harry Wentland Cc: Leo Li Cc: Rodrigo Siqueira Cc: amd-...@lists.freedesktop.org Cc: dri-devel@lists.freedesktop.org Cc: Alex Deucher Cc: Christian König Cc: "Pan, Xinhui" --- drivers/gpu/drm/amd/display/dc/dml/dcn32/dcn32_fpu.c | 116 -- 1 file changed, 49 insertions(+), 67 deletions(-) --- a/drivers/gpu/drm/amd/display/dc/dml/dcn32/dcn32_fpu.c +++ b/drivers/gpu/drm/amd/display/dc/dml/dcn32/dcn32_fpu.c @@ -243,7 +243,7 @@ void dcn32_build_wm_range_table_fpu(stru clk_mgr->base.bw_params->wm_table.nv_entries[WM_D].pmfw_breakdown.max_uclk = 0x; } -/** +/* * Finds dummy_latency_index when MCLK switching using firmware based * vblank stretch is enabled. This function will iterate through the * table of dummy pstate latencies until the lowest value that allows @@ -290,15 +290,14 @@ int dcn32_find_dummy_latency_index_for_f /** * dcn32_helper_populate_phantom_dlg_params - Get DLG params for phantom pipes * and populate pipe_ctx with those params. - * - * This function must be called AFTER the phantom pipes are added to context - * and run through DML (so that the DLG params for the phantom pipes can be - * populated), and BEFORE we program the timing for the phantom pipes. - * * @dc: [in] current dc state * @context: [in] new dc state * @pipes: [in] DML pipe params array * @pipe_cnt: [in] DML pipe count + * + * This function must be called AFTER the phantom pipes are added to context + * and run through DML (so that the DLG params for the phantom pipes can be + * populated), and BEFORE we program the timing for the phantom pipes. */ void dcn32_helper_populate_phantom_dlg_params(struct dc *dc, struct dc_state *context, @@ -331,8 +330,9 @@ void dcn32_helper_populate_phantom_dlg_p } /** - * *** - * dcn32_predict_pipe_split: Predict if pipe split will occur for a given DML pipe + * dcn32_predict_pipe_split - Predict if pipe split will occur for a given DML pipe + * @context: [in] New DC state to be programmed + * @pipe_e2e: [in] DML pipe end to end context * * This function takes in a DML pipe (pipe_e2e) and predicts if pipe split is required (both * ODM and MPC). For pipe split, ODM combine is determined by the ODM mode, and MPC combine is @@ -343,12 +343,7 @@ void dcn32_helper_populate_phantom_dlg_p * - MPC combine is only chosen if there is no ODM combine requirements / policy in place, and * MPC is required * - * @param [in]: context: New DC state to be programmed - * @param [in]: pipe_e2e: DML pipe end to end context - * - * @return: Number of splits expected (1 for 2:1 split, 3 for 4:1 split, 0 for no splits). - * - * *** + * Return: Number of splits expected (1 for 2:1 split, 3 for 4:1 split, 0 for no splits). */ uint8_t dcn32_predict_pipe_split(struct dc_state *context, display_e2e_pipe_params_st *pipe_e2e) @@ -504,7 +499,14 @@ void insert_entry_into_table_sorted(stru } /** - * dcn32_set_phantom_stream_timing: Set timing params for the phantom stream + * dcn32_set_phantom_stream_timing - Set
[PATCH] drm/i915/pmu: Match frequencies reported by PMU and sysfs
PMU and sysfs use different wakeref's to "interpret" zero freq. Sysfs uses
runtime PM wakeref (see intel_rps_read_punit_req and
intel_rps_read_actual_frequency). PMU uses the GT parked/unparked
wakeref. In general the GT wakeref is held for less time that the runtime
PM wakeref which causes PMU to report a lower average freq than the average
freq obtained from sampling sysfs.
To resolve this, use the same freq functions (and wakeref's) in PMU as
those used in sysfs.
Bug: https://gitlab.freedesktop.org/drm/intel/-/issues/7025
Reported-by: Ashwin Kumar Kulkarni
Cc: Tvrtko Ursulin
Signed-off-by: Ashutosh Dixit
---
drivers/gpu/drm/i915/i915_pmu.c | 27 ++-
1 file changed, 2 insertions(+), 25 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
index 958b37123bf1..eda03f264792 100644
--- a/drivers/gpu/drm/i915/i915_pmu.c
+++ b/drivers/gpu/drm/i915/i915_pmu.c
@@ -371,37 +371,16 @@ static void
frequency_sample(struct intel_gt *gt, unsigned int period_ns)
{
struct drm_i915_private *i915 = gt->i915;
- struct intel_uncore *uncore = gt->uncore;
struct i915_pmu *pmu = >pmu;
struct intel_rps *rps = >rps;
if (!frequency_sampling_enabled(pmu))
return;
- /* Report 0/0 (actual/requested) frequency while parked. */
- if (!intel_gt_pm_get_if_awake(gt))
- return;
-
if (pmu->enable & config_mask(I915_PMU_ACTUAL_FREQUENCY)) {
- u32 val;
-
- /*
-* We take a quick peek here without using forcewake
-* so that we don't perturb the system under observation
-* (forcewake => !rc6 => increased power use). We expect
-* that if the read fails because it is outside of the
-* mmio power well, then it will return 0 -- in which
-* case we assume the system is running at the intended
-* frequency. Fortunately, the read should rarely fail!
-*/
- val = intel_uncore_read_fw(uncore, GEN6_RPSTAT1);
- if (val)
- val = intel_rps_get_cagf(rps, val);
- else
- val = rps->cur_freq;
-
add_sample_mult(>sample[__I915_SAMPLE_FREQ_ACT],
- intel_gpu_freq(rps, val), period_ns / 1000);
+ intel_rps_read_actual_frequency(rps),
+ period_ns / 1000);
}
if (pmu->enable & config_mask(I915_PMU_REQUESTED_FREQUENCY)) {
@@ -409,8 +388,6 @@ frequency_sample(struct intel_gt *gt, unsigned int
period_ns)
intel_rps_get_requested_frequency(rps),
period_ns / 1000);
}
-
- intel_gt_pm_put_async(gt);
}
static enum hrtimer_restart i915_sample(struct hrtimer *hrtimer)
--
2.34.1
Re: [PATCH] drm/tve200: Use drm_* variants for logging
On Wed, Sep 28, 2022 at 11:31 AM Khalid Masum wrote: > We have routines like drm_info/warn/err for logging. Use them instead > of dev_* variants to get drm-formatted log messages. > > Signed-off-by: Khalid Masum So is this the new thing? We've been through: - DRM_INFO_ETC BIG CAPITAL LETTER MACROS - Just use dev_info() & friends like everyone else - Now drm_info() & co Note I don't wanna bikeshed about this, just tell me there is consensus now so we know what to do. Yours, Linus Walleij
Re: [PATCH 2/4] drm/tegra: dsi: Clear enable register if powered by bootloader
On Fri, Sep 30, 2022 at 01:11:10PM +0200, Thierry Reding wrote:
> On Thu, Sep 29, 2022 at 06:05:00PM +0100, Diogo Ivo wrote:
> > +
> > err = tegra_dsi_prepare(dsi);
> > if (err < 0) {
> > dev_err(dsi->dev, "failed to prepare: %d\n", err);
> > @@ -1573,6 +1600,8 @@ static int tegra_dsi_probe(struct platform_device
> > *pdev)
> >
> > dsi->output.connector.polled = DRM_CONNECTOR_POLL_HPD;
> >
> > + /* Check if the DSI module was left on by bootloader. */
> > + dsi->enabled = of_property_read_bool(pdev->dev.of_node,
> > "nvidia,boot-on");
>
> The isn't a documented property. But before you go and add this, are
> there no alternative ways to detect that the DSI controller is active?
> Could we not read one of the registers to find out?
Hello, thank you for your feedback.
You are correct, it is possible to simply read a register to obtain
this information, and this property is not needed.
> DRM/KMS has built-in mechanisms to read back hardware state on boot, so
> I wonder if we can hook that up. It'd make the most sense if all sub-
> drivers did this, because then we could eventually inherit the
> bootloader configuration and transition to the kernel display driver
> seamlessly, but doing this in DSI first may help prepare for that more
> extended use-case.
I have only recently started digging in the DRM/KMS subsystem, could
you point out what those mechanisms are? That end goal seems like
something worth pursuing.
> A slightly simpler alternative would be to add the reset code to the
> encoder's or connector's ->reset() implementation. This is called at the
> right time (i.e. when the mode configuration is first reset), so you can
> run the workaround from tegra_dsi_encoder_enable() there. That's better
> than having this guarded by the dsi->enabled flag so that it is run only
> once.
>
> Thierry
Regarding the placement of the workaround, I placed it in encoder_enable()
since my attempts of placing it in other functions (such as the connector's
->reset() method) resulted in a kernel hang, and I have no solution for this.
I'm assuming this is due to some part of the DSI hardware not being fully
initialized, but I haven't been able to confirm this.
Best regards,
Diogo
Re: [Intel-gfx] [PATCH] drm/i915: Fix CFI violations in gt_sysfs
Hi Andrzej, On Thu, Sep 29, 2022 at 03:44:40PM -0700, Nathan Chancellor wrote: > On Fri, Sep 30, 2022 at 12:34:41AM +0200, Andrzej Hajda wrote: > > On 22.09.2022 21:51, Nathan Chancellor wrote: > > > When booting with clang's kernel control flow integrity series [1], > > > there are numerous violations when accessing the files under > > > /sys/devices/pci:00/:00:02.0/drm/card0/gt/gt0: > > > > > >$ cd /sys/devices/pci:00/:00:02.0/drm/card0/gt/gt0 > > > > > >$ grep . * > > >id:0 > > >punit_req_freq_mhz:350 > > >rc6_enable:1 > > >rc6_residency_ms:214934 > > >rps_act_freq_mhz:1300 > > >rps_boost_freq_mhz:1300 > > >rps_cur_freq_mhz:350 > > >rps_max_freq_mhz:1300 > > >rps_min_freq_mhz:350 > > >rps_RP0_freq_mhz:1300 > > >rps_RP1_freq_mhz:350 > > >rps_RPn_freq_mhz:350 > > >throttle_reason_pl1:0 > > >throttle_reason_pl2:0 > > >throttle_reason_pl4:0 > > >throttle_reason_prochot:0 > > >throttle_reason_ratl:0 > > >throttle_reason_status:0 > > >throttle_reason_thermal:0 > > >throttle_reason_vr_tdc:0 > > >throttle_reason_vr_thermalert:0 > > > > > >$ sudo dmesg &| grep "CFI failure at" > > >[ 214.595903] CFI failure at kobj_attr_show+0x19/0x30 (target: > > > id_show+0x0/0x70 [i915]; expected type: 0xc527b809) > > >[ 214.596064] CFI failure at kobj_attr_show+0x19/0x30 (target: > > > punit_req_freq_mhz_show+0x0/0x40 [i915]; expected type: 0xc527b809) > > >[ 214.596407] CFI failure at kobj_attr_show+0x19/0x30 (target: > > > rc6_enable_show+0x0/0x40 [i915]; expected type: 0xc527b809) > > >[ 214.596528] CFI failure at kobj_attr_show+0x19/0x30 (target: > > > rc6_residency_ms_show+0x0/0x270 [i915]; expected type: 0xc527b809) > > >[ 214.596682] CFI failure at kobj_attr_show+0x19/0x30 (target: > > > act_freq_mhz_show+0x0/0xe0 [i915]; expected type: 0xc527b809) > > >[ 214.596792] CFI failure at kobj_attr_show+0x19/0x30 (target: > > > boost_freq_mhz_show+0x0/0xe0 [i915]; expected type: 0xc527b809) > > >[ 214.596893] CFI failure at kobj_attr_show+0x19/0x30 (target: > > > cur_freq_mhz_show+0x0/0xe0 [i915]; expected type: 0xc527b809) > > >[ 214.596996] CFI failure at kobj_attr_show+0x19/0x30 (target: > > > max_freq_mhz_show+0x0/0xe0 [i915]; expected type: 0xc527b809) > > >[ 214.597099] CFI failure at kobj_attr_show+0x19/0x30 (target: > > > min_freq_mhz_show+0x0/0xe0 [i915]; expected type: 0xc527b809) > > >[ 214.597198] CFI failure at kobj_attr_show+0x19/0x30 (target: > > > RP0_freq_mhz_show+0x0/0xe0 [i915]; expected type: 0xc527b809) > > >[ 214.597301] CFI failure at kobj_attr_show+0x19/0x30 (target: > > > RP1_freq_mhz_show+0x0/0xe0 [i915]; expected type: 0xc527b809) > > >[ 214.597405] CFI failure at kobj_attr_show+0x19/0x30 (target: > > > RPn_freq_mhz_show+0x0/0xe0 [i915]; expected type: 0xc527b809) > > >[ 214.597538] CFI failure at kobj_attr_show+0x19/0x30 (target: > > > throttle_reason_bool_show+0x0/0x50 [i915]; expected type: 0xc527b809) > > >[ 214.597701] CFI failure at kobj_attr_show+0x19/0x30 (target: > > > throttle_reason_bool_show+0x0/0x50 [i915]; expected type: 0xc527b809) > > >[ 214.597836] CFI failure at kobj_attr_show+0x19/0x30 (target: > > > throttle_reason_bool_show+0x0/0x50 [i915]; expected type: 0xc527b809) > > >[ 214.597952] CFI failure at kobj_attr_show+0x19/0x30 (target: > > > throttle_reason_bool_show+0x0/0x50 [i915]; expected type: 0xc527b809) > > >[ 214.598071] CFI failure at kobj_attr_show+0x19/0x30 (target: > > > throttle_reason_bool_show+0x0/0x50 [i915]; expected type: 0xc527b809) > > >[ 214.598177] CFI failure at kobj_attr_show+0x19/0x30 (target: > > > throttle_reason_bool_show+0x0/0x50 [i915]; expected type: 0xc527b809) > > >[ 214.598307] CFI failure at kobj_attr_show+0x19/0x30 (target: > > > throttle_reason_bool_show+0x0/0x50 [i915]; expected type: 0xc527b809) > > >[ 214.598439] CFI failure at kobj_attr_show+0x19/0x30 (target: > > > throttle_reason_bool_show+0x0/0x50 [i915]; expected type: 0xc527b809) > > >[ 214.598542] CFI failure at kobj_attr_show+0x19/0x30 (target: > > > throttle_reason_bool_show+0x0/0x50 [i915]; expected type: 0xc527b809) > > > > > > With kCFI, indirect calls are validated against their expected type > > > versus actual type and failures occur when the two types do not match. > > > > Have you tried this tool with drm subsytem, IIRC there are also similar > > cases with callbacks expecting ptr to different struct than actually passed. > > Yes, I have also run a kCFI kernel on an AMD system that I have and I > have not seen any failures from them. I only have AMD and Intel systems > with graphics so there could be other problems lurking in other drivers. > > > > The ultimate issue is that these sysfs functions are expecting to be > > > called via dev_attr_show() but they may also be called via > > > kobj_attr_show(), as certain files are created
Re: [PATCH v2 1/8] mm/memory.c: Fix race when faulting a device private page
Am 2022-10-02 um 20:53 schrieb Alistair Popple:
Felix Kuehling writes:
On 2022-09-28 08:01, Alistair Popple wrote:
When the CPU tries to access a device private page the migrate_to_ram()
callback associated with the pgmap for the page is called. However no
reference is taken on the faulting page. Therefore a concurrent
migration of the device private page can free the page and possibly the
underlying pgmap. This results in a race which can crash the kernel due
to the migrate_to_ram() function pointer becoming invalid. It also means
drivers can't reliably read the zone_device_data field because the page
may have been freed with memunmap_pages().
Close the race by getting a reference on the page while holding the ptl
to ensure it has not been freed. Unfortunately the elevated reference
count will cause the migration required to handle the fault to fail. To
avoid this failure pass the faulting page into the migrate_vma functions
so that if an elevated reference count is found it can be checked to see
if it's expected or not.
Do we really have to drag the fault_page all the way into the migrate structure?
Is the elevated refcount still needed at that time? Maybe it would be easier to
drop the refcount early in the ops->migrage_to_ram callbacks, so we won't have
to deal with it in all the migration code.
That would also work. Honestly I don't really like either solution :-)
Then we agree. :)
I didn't like having to plumb it all through the migration code
but I ended up going this way because I felt it was easier to explain
the life time of vmf->page for the migrate_to_ram() callback. This way
vmf->page is guaranteed to be valid for the duration of the
migrate_to_ram() callbak.
As you suggest we could instead have drivers call put_page(vmf->page)
somewhere in migrate_to_ram() before calling migrate_vma_setup(). The
reason I didn't go this way is IMHO it's more subtle because in general
the page will remain valid after that put_page() anyway. So it would be
easy for drivers to introduce a bug assuming the vmf->page is still
valid and not notice because most of the time it is.
I guess I'm biased because my migrate_to_ram implementation doesn't use
the vmf->page at all. I agree that dropping the refcount in the callback
is subtle. But handling an elevated refcount for just one special page
in the migration code also looks a bit fragile to me.
It's not my call to make. But my preference is very clear. Either way,
if the decision is to go with your solution, then you have my
Acked-by: Felix Kuehling
Let me know if you disagree with my reasoning though - would appreciate
any review here.
Regards,
Felix
Signed-off-by: Alistair Popple
Cc: Jason Gunthorpe
Cc: John Hubbard
Cc: Ralph Campbell
Cc: Michael Ellerman
Cc: Felix Kuehling
Cc: Lyude Paul
---
arch/powerpc/kvm/book3s_hv_uvmem.c | 15 ++-
drivers/gpu/drm/amd/amdkfd/kfd_migrate.c | 17 +++--
drivers/gpu/drm/amd/amdkfd/kfd_migrate.h | 2 +-
drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 11 +---
include/linux/migrate.h | 8 ++-
lib/test_hmm.c | 7 ++---
mm/memory.c | 16 +++-
mm/migrate.c | 34 ++---
mm/migrate_device.c | 18 +
9 files changed, 87 insertions(+), 41 deletions(-)
diff --git a/arch/powerpc/kvm/book3s_hv_uvmem.c
b/arch/powerpc/kvm/book3s_hv_uvmem.c
index 5980063..d4eacf4 100644
--- a/arch/powerpc/kvm/book3s_hv_uvmem.c
+++ b/arch/powerpc/kvm/book3s_hv_uvmem.c
@@ -508,10 +508,10 @@ unsigned long kvmppc_h_svm_init_start(struct kvm *kvm)
static int __kvmppc_svm_page_out(struct vm_area_struct *vma,
unsigned long start,
unsigned long end, unsigned long page_shift,
- struct kvm *kvm, unsigned long gpa)
+ struct kvm *kvm, unsigned long gpa, struct page *fault_page)
{
unsigned long src_pfn, dst_pfn = 0;
- struct migrate_vma mig;
+ struct migrate_vma mig = { 0 };
struct page *dpage, *spage;
struct kvmppc_uvmem_page_pvt *pvt;
unsigned long pfn;
@@ -525,6 +525,7 @@ static int __kvmppc_svm_page_out(struct vm_area_struct *vma,
mig.dst = _pfn;
mig.pgmap_owner = _uvmem_pgmap;
mig.flags = MIGRATE_VMA_SELECT_DEVICE_PRIVATE;
+ mig.fault_page = fault_page;
/* The requested page is already paged-out, nothing to do */
if (!kvmppc_gfn_is_uvmem_pfn(gpa >> page_shift, kvm, NULL))
@@ -580,12 +581,14 @@ static int __kvmppc_svm_page_out(struct vm_area_struct
*vma,
static inline int kvmppc_svm_page_out(struct vm_area_struct *vma,
unsigned long start, unsigned long end,
unsigned long page_shift,
- struct kvm *kvm, unsigned long gpa)
+
Re: [Intel-gfx] [PATCH 1/1] drm/i915: Use GEN12 RPSTAT register
On Tue, Sep 27, 2022 at 06:47:28PM -0700, Dixit, Ashutosh wrote:
> On Tue, 27 Sep 2022 04:35:29 -0700, Badal Nilawar wrote:
> >
> > From: Don Hiatt
> >
> > On GEN12 and above use GEN12_RPSTAT register to get Current
> > Actual Graphics Frequency of GT
>
> I think even for the purposes of reviewing this it would be good to mention
> in the commit message that:
>
> a. GEN12_RPSTAT register doesn't require a forcewake to be read (it doesn't
>belong to a forcewake domain)
> b. Will result in a 0 frequency if the GT is in RC6
perhaps also in a comment... (continue)
>
> Thanks.
> --
> Ashutosh
>
> > v2:
> > - Fixed review comments(Ashutosh)
> > - Added function intel_rps_read_rpstat_fw to read RPSTAT without
> > forcewake, required especially for GEN6_RPSTAT1 (Ashutosh, Tvrtko)
> >
> > Cc: Don Hiatt
> > Cc: Andi Shyti
> > Signed-off-by: Don Hiatt
> > Signed-off-by: Badal Nilawar
> > ---
> > drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c | 2 +-
> > drivers/gpu/drm/i915/gt/intel_gt_regs.h | 4 +++
> > drivers/gpu/drm/i915/gt/intel_rps.c | 32 +--
> > drivers/gpu/drm/i915/gt/intel_rps.h | 2 ++
> > drivers/gpu/drm/i915/i915_pmu.c | 3 +-
> > 5 files changed, 38 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c
> > b/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c
> > index 10f680dbd7b6..b9b47052b26d 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c
> > @@ -380,7 +380,7 @@ void intel_gt_pm_frequency_dump(struct intel_gt *gt,
> > struct drm_printer *p)
> > rpinclimit = intel_uncore_read(uncore, GEN6_RP_UP_THRESHOLD);
> > rpdeclimit = intel_uncore_read(uncore, GEN6_RP_DOWN_THRESHOLD);
> >
> > - rpstat = intel_uncore_read(uncore, GEN6_RPSTAT1);
> > + rpstat = intel_rps_read_rpstat(rps);
> > rpcurupei = intel_uncore_read(uncore, GEN6_RP_CUR_UP_EI) &
> > GEN6_CURICONT_MASK;
> > rpcurup = intel_uncore_read(uncore, GEN6_RP_CUR_UP) &
> > GEN6_CURBSYTAVG_MASK;
> > rpprevup = intel_uncore_read(uncore, GEN6_RP_PREV_UP) &
> > GEN6_CURBSYTAVG_MASK;
> > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> > b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> > index 7f79bbf97828..1f1e90acc1ab 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> > +++ b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> > @@ -1519,6 +1519,10 @@
> > #define VLV_RENDER_C0_COUNT_MMIO(0x138118)
> > #define VLV_MEDIA_C0_COUNT _MMIO(0x13811c)
> >
> > +#define GEN12_RPSTAT1 _MMIO(0x1381b4)
> > +#define GEN12_CAGF_SHIFT 11
> > +#define GEN12_CAGF_MASK REG_GENMASK(19, 11)
> > +
> > #define GEN11_GT_INTR_DW(x)_MMIO(0x190018 + ((x) *
> > 4))
> > #define GEN11_CSME (31)
> > #define GEN11_GUNIT (28)
> > diff --git a/drivers/gpu/drm/i915/gt/intel_rps.c
> > b/drivers/gpu/drm/i915/gt/intel_rps.c
> > index 17b40b625e31..5a15a630b1c6 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_rps.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_rps.c
> > @@ -2068,12 +2068,40 @@ void intel_rps_sanitize(struct intel_rps *rps)
> > rps_disable_interrupts(rps);
> > }
... here!
> > +u32 intel_rps_read_rpstat_fw(struct intel_rps *rps)
> > +{
> > + struct drm_i915_private *i915 = rps_to_i915(rps);
> > + i915_reg_t rpstat;
> > +
> > + if (GRAPHICS_VER(i915) >= 12)
> > + rpstat = GEN12_RPSTAT1;
> > + else
> > + rpstat = GEN6_RPSTAT1;
> > +
> > + return intel_uncore_read_fw(rps_to_gt(rps)->uncore, rpstat);
> > +}
> > +
> > +u32 intel_rps_read_rpstat(struct intel_rps *rps)
> > +{
> > + struct drm_i915_private *i915 = rps_to_i915(rps);
> > + i915_reg_t rpstat;
> > +
> > + if (GRAPHICS_VER(i915) >= 12)
> > + rpstat = GEN12_RPSTAT1;
> > + else
> > + rpstat = GEN6_RPSTAT1;
> > +
> > + return intel_uncore_read(rps_to_gt(rps)->uncore, rpstat);
> > +}
perhaps this can be simplified a bit more to avoid some code
duplication, but I'm not going to push on this.
I see that CI got stuck somewhere, but the failure doesn't seem
to be related to this patch.
Otherwise it all looks good, with the improved git comment as
Ashutosh asked and the comment above:
Reviewed-by: Andi Shyti
Andi
> > u32 intel_rps_get_cagf(struct intel_rps *rps, u32 rpstat)
> > {
> > struct drm_i915_private *i915 = rps_to_i915(rps);
> > u32 cagf;
> >
> > - if (IS_VALLEYVIEW(i915) || IS_CHERRYVIEW(i915))
> > + if (GRAPHICS_VER(i915) >= 12)
> > + cagf = (rpstat & GEN12_CAGF_MASK) >> GEN12_CAGF_SHIFT;
> > + else if (IS_VALLEYVIEW(i915) || IS_CHERRYVIEW(i915))
> > cagf = (rpstat >> 8) & 0xff;
> > else if (GRAPHICS_VER(i915) >= 9)
> > cagf =
Re: [PATCH 1/4] dt-bindings: display: Add bindings for JDI LPM102A188A
On Fri, Sep 30, 2022 at 12:49:31PM +0200, Krzysztof Kozlowski wrote: > > + ts-reset-gpios: > > +maxItems: 1 > > +description: | > > + Specifier for a GPIO connected to the touchscreen reset control > > signal. > > + The reset signal is active low. > > Isn't touchscreen a separate (input) device? Hello, thank you for the feedback. According to the downstream kernel's log, it seems like the panel and the touchscreen controller are considered to be embedded in the same unit (for example in [1]), with the touch input being transmitted via HID-over-I2C, and since I did not find any reset gpio handling in that driver I opted to include this reset here, unless there is a better way of going about this. Best regards, Diogo [1]: https://android.googlesource.com/kernel/tegra/+/bca61c34db9f72113af058f53eeb9fbd5e69a1d0
Re: [PATCH v3 3/8] dt-bindings: Add bindings for Tegra234 NVDEC
On Tue, 20 Sep 2022 11:11:58 +0300, Mikko Perttunen wrote: > From: Mikko Perttunen > > Update NVDEC bindings for Tegra234. This new engine version only has > two memory clients, but now requires three clocks, and as a bigger > change the engine loads firmware from a secure carveout configured by > the bootloader. > > For the latter, we need to add a phandle to the memory controller > to query the location of this carveout, and several other properties > containing offsets into the firmware inside the carveout. This > carveout is not accessible by the CPU, but is needed by NVDEC, > so we need this information to be relayed from the bootloader. > > As the binding was getting large with many conditional properties, > also split the Tegra234 version out into a separate file. > > Signed-off-by: Mikko Perttunen > --- > v3: > - Adjusted descriptions for firmware-related DT properties > as requested. > - Small update to commit message. > v2: > - Split out into separate file to avoid complexity with > conditionals etc. > --- > .../gpu/host1x/nvidia,tegra234-nvdec.yaml | 156 ++ > 1 file changed, 156 insertions(+) > create mode 100644 > Documentation/devicetree/bindings/gpu/host1x/nvidia,tegra234-nvdec.yaml > Reviewed-by: Rob Herring
Re: [PATCH v2 14/17] drm/i915/vm_bind: Expose i915_request_await_bind()
Hi Niranjana, On Sun, Oct 02, 2022 at 11:12:42PM -0700, Niranjana Vishwanathapura wrote: > Rename __i915_request_await_bind() as i915_request_await_bind() > and make it non-static as it will be used in execbuf3 ioctl path. > > Signed-off-by: Niranjana Vishwanathapura Reviewed-by: Andi Shyti Andi
Re: [PATCH v2 10/17] drm/i915/vm_bind: Abstract out common execbuf functions
Hi Niranjana,
[...]
> + for_each_child(ce, child) {
> + err = intel_context_pin_ww(child, ww);
> + GEM_BUG_ON(err);/* perma-pinned should incr a counter */
> + }
> +
> + for_each_child(ce, child) {
> + err = eb_pin_timeline(child, throttle, nonblock);
> + if (err)
> + goto unwind;
> + ++i;
> + }
any reason for having two separate for_each_child here?
Andi
> + err = eb_pin_timeline(ce, throttle, nonblock);
> + if (err)
> + goto unwind;
> +
> + return 0;
> +
> +unwind:
> + for_each_child(ce, child) {
> + if (j++ < i) {
> + mutex_lock(>timeline->mutex);
> + intel_context_exit(child);
> + mutex_unlock(>timeline->mutex);
> + }
> + }
> + for_each_child(ce, child)
> + intel_context_unpin(child);
[...]
Re: [PATCH v1 09/17] drm/mediatek: hdmi: add connector flag
On Tue, 20 Sep 2022 12:38, AngeloGioacchino Del Regno
wrote:
>Il 19/09/22 18:56, Guillaume Ranquet ha scritto:
>> Add a flag to indicate support for an external connector
>>
>> Signed-off-by: Guillaume Ranquet
>>
>> diff --git a/drivers/gpu/drm/mediatek/mtk_hdmi_common.c
>> b/drivers/gpu/drm/mediatek/mtk_hdmi_common.c
>> index 86653ebaacfd..30407603d693 100644
>> --- a/drivers/gpu/drm/mediatek/mtk_hdmi_common.c
>> +++ b/drivers/gpu/drm/mediatek/mtk_hdmi_common.c
>> @@ -199,20 +199,22 @@ int mtk_hdmi_dt_parse_pdata(struct mtk_hdmi *hdmi,
>> struct platform_device *pdev,
>> goto put_device;
>> }
>>
>> -remote = of_graph_get_remote_node(np, 1, 0);
>> -if (!remote) {
>> -ret = -EINVAL;
>> -goto put_device;
>> -}
>> -
>> -if (!of_device_is_compatible(remote, "hdmi-connector")) {
>> -hdmi->next_bridge = of_drm_find_bridge(remote);
>> -if (!hdmi->next_bridge) {
>> -dev_err(dev, "Waiting for external bridge\n");
>> -of_node_put(remote);
>> -ret = -EPROBE_DEFER;
>> +if (hdmi->conf->has_connector) {
>
>If MT8195's DPI uses the internal HDMI->DP converter, I say that the external
>DP has HDMI input and DP output.
>Logically, you can't have a HDMI port that's connected to nothing.
>
>Please, rethink this change.
>
>Regards,
>Angelo
Hi Angelo,
Sorry for the late answer.
I have reworked this for V2, to use an hdmi connector device node to "bind"
both the hdmi and hdmi-ddc driver together as with "legacy" code.
So this patch is dropped in V2 (hopefully ready soon).
Just to make things clear, the hardware path on mt8195 is:
DPI1 -> HDMI Tx -> HDMI Phy
DP Intf1 -> DP Tx -> USB Type C Mux -> DP over USB-C
So there's no HDMI->DP converter involved.
Thx,
Guillaume.
RE: [PATCH v4.1] drm/i915/mtl: Define engine context layouts
> -Original Message- > From: De Marchi, Lucas > Sent: Thursday, September 29, 2022 5:11 PM > To: Sripada, Radhakrishna > Cc: intel-...@lists.freedesktop.org; dri-devel@lists.freedesktop.org > Subject: Re: [PATCH v4.1] drm/i915/mtl: Define engine context layouts > > On Wed, Sep 28, 2022 at 08:55:11AM -0700, Radhakrishna Sripada wrote: > >From: Matt Roper > > > >The part of the media and blitter engine contexts that we care about for > >setting up an initial state on MTL are nearly similar to DG2 (and PVC). > >The difference being PRT_BB_STATE being replaced with NOP. > > > >For render/compute engines, the part of the context images are nearly > >the same, although the layout had a very slight change --- one POSH > >register was removed and the placement of some LRI/noops adjusted > >slightly to compensate. > > > >v2: > > - Dg2, mtl xcs offsets slightly vary. Use a separate offsets array(Bala) > > - Add missing nop in xcs offsets(Bala) > >v3: > > - Fix the spacing for nop in xcs offset(MattR) > >v4: > > - Fix rcs register offset(MattR) > >v4.1: > > - Fix commit message(Lucas) > > > >Bspec: 46261, 46260, 45585 > >Cc: Balasubramani Vivekanandan > >Cc: Licas De Marchi > >Signed-off-by: Matt Roper > >Signed-off-by: Radhakrishna Sripada > > > Reviewed-by: Lucas De Marchi Pushed the patch, Thanks for the review. -Radhakrishna Sripada > > Lucas De Marchi
Re: [PATCH v1 17/17] drm/mediatek: Add mt8195-dpi support to drm_drv
On Tue, 27 Sep 2022 16:28, Krzysztof Kozlowski
wrote:
>On 27/09/2022 15:04, Guillaume Ranquet wrote:
>> On Thu, 22 Sep 2022 09:20, Krzysztof Kozlowski
>> wrote:
>>> On 19/09/2022 18:56, Guillaume Ranquet wrote:
Add dpi support to enable the HDMI path.
Signed-off-by: Guillaume Ranquet
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
index 72049a530ae1..27f029ca760b 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
@@ -820,6 +820,8 @@ static const struct of_device_id mtk_ddp_comp_dt_ids[]
= {
.data = (void *)MTK_DPI },
{ .compatible = "mediatek,mt8192-dpi",
.data = (void *)MTK_DPI },
+ { .compatible = "mediatek,mt8195-dpi",
+.data = (void *)MTK_DPI },
>>>
>>> It's compatible with the others. You don't need more compatibles.
>>
>> Hi Krzysztof,
>>
>> It's a bit confusing, because this compatible is used in both
>> mtk_drm_drv.c and in mtk_dpi.c
>>
>> Albeit it's entirely the same thing regarding the mtk_drm_drv module,
>> it's pretty different
>> regarding the mtk_dpi module.
>
>Sure, but this does not explain why do you need these entries here in
>mtk_drm_drv.
>
>Best regards,
>Krzysztof
>
Hi Krzysztof,
Sorry for the late answer.
The mtk_drm_drv is the component master of the full mediatek drm stack.
it "binds" all of the crtc/dpi/ovl/mutex/merge... components of the stack.
That mtk_ddp_comp_dt_ids array is iterated over to find all of the components
from the device tree.
Hope this clarifies things?
Thx,
Guillaume.
Re: [PATCH -next resend] backlight: ktd253: Switch to use dev_err_probe() helper
On Mon, Sep 26, 2022 at 4:14 PM Yang Yingliang wrote: > In the probe path, dev_err() can be replaced with dev_err_probe() > which will check if error code is -EPROBE_DEFER and prints the > error name. It also sets the defer probe reason which can be > checked later through debugfs. It's more simple in error path. > > Signed-off-by: Yang Yingliang > Reviewed-by: Daniel Thompson Reviewed-by: Linus Walleij Yours, Linus Walleij
Re: [RFC/PATCH] backlight: hx8357: prepare to conversion to gpiod API
On Wed, Sep 28, 2022 at 11:33:52AM -0700, Dmitry Torokhov wrote: > On Wed, Sep 28, 2022 at 12:00:51PM +0100, Daniel Thompson wrote: > > On Tue, Sep 27, 2022 at 03:32:35PM -0700, Dmitry Torokhov wrote: > > > Properties describing GPIOs should be named as "-gpios" or > > > "-gpio", and that is what gpiod API expects, however the > > > driver uses non-standard "gpios-reset" name. Let's adjust this, and also > > > note that the reset line is active low as that is also important to > > > gpiod API. > > > > No objections to the goal but... > > > > > > > Signed-off-by: Dmitry Torokhov > > > --- > > > > > > Another option is to add another quirk into gpiolib-of.c, but we > > > may end up with a ton of them once we convert everything away from > > > of_get_named_gpio() to gpiod API, so I'd prefer not doing that. > > > > ... it is unusual to permit backwards incompatible changes to the DT > > bindings[1]: creating "flag days" where hardware stops functioning if > > you boot an new kernel with an old DT is a known annoyance to users. > > > > I usually favour quirks tables or similar[2] rather than break legacy > > DTs. Very occasionally I accept (believable) arguments that no legacy > > DTs actually exist but that can very difficult to verify. > > > > Overall I'd like to solicit views from both GPIO and DT maintainers > > before rejecting quirks tables as a way to help smooth these sort of > > changes (or links to ML archives if this has already been discussed). > > I believe I was able to convince Rob once or twice that keeping > compatibility was not worth it (not in general but in a couple of > concrete cases), at least while device tree bindings are part of the > kernel. Can't find the emails though... > > I think we should consider several options: I have to note that these are *non-exclusive* options > 1. DTS/DTB is in firmware. In this case absolutely, we need to keep > binary compatibility as we can not expect users to reflash firmware > (there might not even be a new firmware). This situation matches what we > have with ACPI systems where we are trying to work around issues > > 2. DTS is shipped with the kernel: > 2a. DTS is present in upstream kernel - awesome, we can patch it > as needed and have one less thing to worry about. I don't think the presence of a DT within the kernel can be the basis for any useful reasoning. a. "Better" firmware projects aimed are likely to consume a DT that is shipped with the kernel and pin it (meaning the kernel cannot solve version skew problems by updating it's copies of the DT). I think tow-boot to be a specific example of this. b. The fact there are are consumers of the binding shipped with the kernel isn't sufficient to show that *all* consumers of the binding are shipped with the kernel. On other words I don't think the presence of a DT in the kernel is especially useful to showing that neither #1 nor #3 apply. > 2b. DTS is not upstream. Vendor did not bother sending it. In > this case it is extremely unlikely that an upstream kernel > will work on such system out of the box, and updating the > kernel is a large engineering task where you pull down new > kernel, rework and apply non-upstream patches, rework kernel > config (new Kconfig options can be introduced, old options > can be renamed, etc). And then spend several weeks > stabilizing the system and tracking down regressions (in > general, not DTS-related ones) > > 3. DTS is not in firmware and not in kernel. Are there such systems? DT overlays strike me are an example of this case. I'm particularly thinking of daughterboard/expansion card examples here where the DT overlay could be any several different places: firmware, an add on boards I2C FLASH, daughterboard documentation, blog posts, etc. That is especially relevant to this specific patch since HX8357 is found on several widely available add-on boards. > So my opinion is that while device trees are part of kernel code and > have not been split into a separate project they are a fair game. If the > change can be handled in the driver without much effort (something like > "wakeup-source" vs "linux,wakeup" vs "linux,keypad-wakeup") - fine, we > can just put a tiny quirk in the driver, but if we need something more > substantial we need to think long and hard if we should implement a > fallback and how much effort there is to maintain/test it so it does not > bitrot. To be honest my original thoughts were that for simple renames, a rename quirk table shared by all renames needed to introduce libgpiod would probably have a lower impact than all the "tiny" per-driver quirks (because it could share code across multiple names). > Anyway, I hope Rob, Linux and Krzysztof to chime in on this exciting > topic once again ;) I'm especially interested in a gpiod point of view! I have invested quite a few characters in this thread. That is
Re: [PATCH] drm: rcar-du: Fix Kconfig dependency between RCAR_DU and RCAR_MIPI_DSI
Hi Tomi,
On Mon, Oct 03, 2022 at 10:31:24AM +0300, Tomi Valkeinen wrote:
> On 03/10/2022 10:01, Geert Uytterhoeven wrote:
> > On Mon, Oct 3, 2022 at 8:56 AM Tomi Valkeinen wrote:
> >> On 02/10/2022 01:03, Laurent Pinchart wrote:
> >>> When the R-Car MIPI DSI driver was added, it was a standalone encoder
> >>> driver without any dependency to or from the R-Car DU driver. Commit
> >>> 957fe62d7d15 ("drm: rcar-du: Fix DSI enable & disable sequence") then
> >>> added a direct call from the DU driver to the MIPI DSI driver, without
> >>> updating Kconfig to take the new dependency into account. Fix it the
> >>> same way that the LVDS encoder is handled.
> >>>
> >>> Fixes: 957fe62d7d15 ("drm: rcar-du: Fix DSI enable & disable sequence")
> >>> Reported-by: kernel test robot
> >>> Signed-off-by: Laurent Pinchart
> >>>
> >>> ---
> >>>drivers/gpu/drm/rcar-du/Kconfig | 13 +
> >>>1 file changed, 9 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/rcar-du/Kconfig
> >>> b/drivers/gpu/drm/rcar-du/Kconfig
> >>> index c959e8c6be7d..fd2c2eaee26b 100644
> >>> --- a/drivers/gpu/drm/rcar-du/Kconfig
> >>> +++ b/drivers/gpu/drm/rcar-du/Kconfig
> >>> @@ -44,12 +44,17 @@ config DRM_RCAR_LVDS
> >>>select OF_FLATTREE
> >>>select OF_OVERLAY
> >>>
> >>> +config DRM_RCAR_USE_MIPI_DSI
> >>> + bool "R-Car DU MIPI DSI Encoder Support"
> >>> + depends on DRM_BRIDGE && OF
> >>> + default DRM_RCAR_DU
> >>> + help
> >>> + Enable support for the R-Car Display Unit embedded MIPI DSI
> >>> encoders.
> >>> +
> >>>config DRM_RCAR_MIPI_DSI
> >>> - tristate "R-Car DU MIPI DSI Encoder Support"
> >>> - depends on DRM && DRM_BRIDGE && OF
> >>> + def_tristate DRM_RCAR_DU
> >>> + depends on DRM_RCAR_USE_MIPI_DSI
> >>>select DRM_MIPI_DSI
> >>> - help
> >>> - Enable support for the R-Car Display Unit embedded MIPI DSI
> >>> encoders.
> >>>
> >>>config DRM_RCAR_VSP
> >>>bool "R-Car DU VSP Compositor Support" if ARM
> >>>
> >>> base-commit: 7860d720a84c74b2761c6b7995392a798ab0a3cb
> >>
> >> Interesting dependency issue. Took me a while to understand it =).
> >>
> >> But is there a reason to not have "depends on DRM_RCAR_DU" for
> >> DRM_RCAR_USE_MIPI_DSI and DRM_RCAR_USE_LVDS? Now the menu items are
> >> available even if RCAR_DU is n. That's also the case for
> >> DRM_RCAR_DW_HDMI, but I'm not sure if that's supposed to be usable even
> >> without RCAR_DU.
> >
> > See
> > https://lore.kernel.org/linux-renesas-soc/cover.1639559338.git.geert+rene...@glider.be/
> >
> > Didn't get to implement the suggested improvements yet...
>
> Ok, looks good.
>
> But I started to wonder, for LVDS and DSI (maybe for HDMI too), what's
> the point of modules here...
>
> If RCAR_DU, LVDS and DSI are compiled as modules, you can load either
> LVDS or DSI module, but those won't do anything alone. So in practice
> you always need to load RCAR_DU module too. But if LVDS or DSI are
> enabled in the kconfig, you _have_ to load those before RCAR_DU.
>
> In other words, you can never, e.g. load RCAR_DU and LVDS, but not DSI,
> if all three are enabled.
>
> So why not just compile LVDS and DSI into the RCAR_DU module,
> simplifying the dependencies, removing this issue altogether?
Patches are welcome :-) I'd do that for v6.2 though, not as a v6.1 fix.
--
Regards,
Laurent Pinchart
Re: [PATCH v2 15/17] drm/i915/vm_bind: Handle persistent vmas in execbuf3
On 03/10/2022 07:12, Niranjana Vishwanathapura wrote:
Handle persistent (VM_BIND) mappings during the request submission
in the execbuf3 path.
v2: Ensure requests wait for bindings to complete.
Signed-off-by: Niranjana Vishwanathapura
Signed-off-by: Andi Shyti
---
.../gpu/drm/i915/gem/i915_gem_execbuffer3.c | 214 +-
1 file changed, 213 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer3.c
b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer3.c
index 66b723842e45..5cece16949d8 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer3.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer3.c
@@ -19,6 +19,7 @@
#include "i915_gem_vm_bind.h"
#include "i915_trace.h"
+#define __EXEC3_HAS_PIN BIT_ULL(33)
#define __EXEC3_ENGINE_PINNED BIT_ULL(32)
#define __EXEC3_INTERNAL_FLAGS(~0ull << 32)
@@ -42,7 +43,9 @@
* execlist. Hence, no support for implicit sync.
*
* The new execbuf3 ioctl only works in VM_BIND mode and the VM_BIND mode only
- * works with execbuf3 ioctl for submission.
+ * works with execbuf3 ioctl for submission. All BOs mapped on that VM (through
+ * VM_BIND call) at the time of execbuf3 call are deemed required for that
+ * submission.
*
* The execbuf3 ioctl directly specifies the batch addresses instead of as
* object handles as in execbuf2 ioctl. The execbuf3 ioctl will also not
@@ -58,6 +61,13 @@
* So, a lot of code supporting execbuf2 ioctl, like relocations, VA
evictions,
* vma lookup table, implicit sync, vma active reference tracking etc., are
not
* applicable for execbuf3 ioctl.
+ *
+ * During each execbuf submission, request fence is added to all VM_BIND mapped
+ * objects with DMA_RESV_USAGE_BOOKKEEP. The DMA_RESV_USAGE_BOOKKEEP usage will
+ * prevent over sync (See enum dma_resv_usage). Note that DRM_I915_GEM_WAIT and
+ * DRM_I915_GEM_BUSY ioctls do not check for DMA_RESV_USAGE_BOOKKEEP usage and
+ * hence should not be used for end of batch check. Instead, the execbuf3
+ * timeline out fence should be used for end of batch check.
*/
/**
@@ -127,6 +137,23 @@ eb_find_vma(struct i915_address_space *vm, u64 addr)
return i915_gem_vm_bind_lookup_vma(vm, va);
}
+static void eb_scoop_unbound_vma_all(struct i915_address_space *vm)
+{
+ struct i915_vma *vma, *vn;
+
+ /**
+* Move all unbound vmas back into vm_bind_list so that they are
+* revalidated.
+*/
+ spin_lock(>vm_rebind_lock);
+ list_for_each_entry_safe(vma, vn, >vm_rebind_list, vm_rebind_link) {
+ list_del_init(>vm_rebind_link);
+ if (!list_empty(>vm_bind_link))
+ list_move_tail(>vm_bind_link, >vm_bind_list);
+ }
+ spin_unlock(>vm_rebind_lock);
+}
+
static int eb_lookup_vma_all(struct i915_execbuffer *eb)
{
unsigned int i, current_batch = 0;
@@ -141,14 +168,121 @@ static int eb_lookup_vma_all(struct i915_execbuffer *eb)
++current_batch;
}
+ eb_scoop_unbound_vma_all(eb->context->vm);
+
return 0;
}
+static int eb_lock_vma_all(struct i915_execbuffer *eb)
+{
+ struct i915_address_space *vm = eb->context->vm;
+ struct i915_vma *vma;
+ int err;
+
+ err = i915_gem_object_lock(eb->context->vm->root_obj, >ww);
+ if (err)
+ return err;
+
+ list_for_each_entry(vma, >non_priv_vm_bind_list,
+ non_priv_vm_bind_link) {
+ err = i915_gem_object_lock(vma->obj, >ww);
+ if (err)
+ return err;
+ }
+
+ return 0;
+}
+
+static void eb_release_persistent_vma_all(struct i915_execbuffer *eb,
+ bool final)
+{
+ struct i915_address_space *vm = eb->context->vm;
+ struct i915_vma *vma, *vn;
+
+ lockdep_assert_held(>vm_bind_lock);
+
+ if (!(eb->args->flags & __EXEC3_HAS_PIN))
+ return;
+
+ assert_object_held(vm->root_obj);
+
+ list_for_each_entry(vma, >vm_bind_list, vm_bind_link)
+ __i915_vma_unpin(vma);
+
+ eb->args->flags &= ~__EXEC3_HAS_PIN;
+ if (!final)
+ return;
+
+ list_for_each_entry_safe(vma, vn, >vm_bind_list, vm_bind_link)
+ if (i915_vma_verify_bind_complete(vma))
+ list_move_tail(>vm_bind_link, >vm_bound_list);
+}
+
static void eb_release_vma_all(struct i915_execbuffer *eb, bool final)
{
+ eb_release_persistent_vma_all(eb, final);
eb_unpin_engine(eb);
}
+static int eb_reserve_fence_for_persistent_vma_all(struct i915_execbuffer *eb)
+{
+ struct i915_address_space *vm = eb->context->vm;
+ struct i915_vma *vma;
+ int ret;
+
+ ret = dma_resv_reserve_fences(vm->root_obj->base.resv, 1);
+ if (ret)
+ return ret;
+
+ list_for_each_entry(vma, >non_priv_vm_bind_list,
+
[PATCH v2] drm/i915/guc: Fix revocation of non-persistent contexts
From: Tvrtko Ursulin
Patch which added graceful exit for non-persistent contexts missed the
fact it is not enough to set the exiting flag on a context and let the
backend handle it from there.
GuC backend cannot handle it because it runs independently in the
firmware and driver might not see the requests ever again. Patch also
missed the fact some usages of intel_context_is_banned in the GuC backend
needed replacing with newly introduced intel_context_is_schedulable.
Fix the first issue by calling into backend revoke when we know this is
the last chance to do it. Fix the second issue by replacing
intel_context_is_banned with intel_context_is_schedulable, which should
always be safe since latter is a superset of the former.
v2:
* Just call ce->ops->revoke unconditionally. (Andrzej)
Signed-off-by: Tvrtko Ursulin
Fixes: 45c64ecf97ee ("drm/i915: Improve user experience and driver robustness
under SIGINT or similar")
Cc: Andrzej Hajda
Cc: John Harrison
Cc: Daniele Ceraolo Spurio
Cc: # v6.0+
Reviewed-by: Andrzej Hajda
---
drivers/gpu/drm/i915/gem/i915_gem_context.c | 8 +-
drivers/gpu/drm/i915/gt/intel_context.c | 5 ++--
drivers/gpu/drm/i915/gt/intel_context.h | 3 +--
.../gpu/drm/i915/gt/uc/intel_guc_submission.c | 26 +--
4 files changed, 17 insertions(+), 25 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c
b/drivers/gpu/drm/i915/gem/i915_gem_context.c
index 0bcde53c50c6..1e29b1e6d186 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
@@ -1387,14 +1387,8 @@ kill_engines(struct i915_gem_engines *engines, bool
exit, bool persistent)
*/
for_each_gem_engine(ce, engines, it) {
struct intel_engine_cs *engine;
- bool skip = false;
- if (exit)
- skip = intel_context_set_exiting(ce);
- else if (!persistent)
- skip = intel_context_exit_nonpersistent(ce, NULL);
-
- if (skip)
+ if ((exit || !persistent) && intel_context_revoke(ce))
continue; /* Already marked. */
/*
diff --git a/drivers/gpu/drm/i915/gt/intel_context.c
b/drivers/gpu/drm/i915/gt/intel_context.c
index 654a092ed3d6..e94365b08f1e 100644
--- a/drivers/gpu/drm/i915/gt/intel_context.c
+++ b/drivers/gpu/drm/i915/gt/intel_context.c
@@ -614,13 +614,12 @@ bool intel_context_ban(struct intel_context *ce, struct
i915_request *rq)
return ret;
}
-bool intel_context_exit_nonpersistent(struct intel_context *ce,
- struct i915_request *rq)
+bool intel_context_revoke(struct intel_context *ce)
{
bool ret = intel_context_set_exiting(ce);
if (ce->ops->revoke)
- ce->ops->revoke(ce, rq, ce->engine->props.preempt_timeout_ms);
+ ce->ops->revoke(ce, NULL, ce->engine->props.preempt_timeout_ms);
return ret;
}
diff --git a/drivers/gpu/drm/i915/gt/intel_context.h
b/drivers/gpu/drm/i915/gt/intel_context.h
index 8e2d70630c49..be09fb2e883a 100644
--- a/drivers/gpu/drm/i915/gt/intel_context.h
+++ b/drivers/gpu/drm/i915/gt/intel_context.h
@@ -329,8 +329,7 @@ static inline bool intel_context_set_exiting(struct
intel_context *ce)
return test_and_set_bit(CONTEXT_EXITING, >flags);
}
-bool intel_context_exit_nonpersistent(struct intel_context *ce,
- struct i915_request *rq);
+bool intel_context_revoke(struct intel_context *ce);
static inline bool
intel_context_force_single_submission(const struct intel_context *ce)
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
index 0ef295a94060..88a4476b8e92 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
@@ -685,7 +685,7 @@ static int __guc_add_request(struct intel_guc *guc, struct
i915_request *rq)
* Corner case where requests were sitting in the priority list or a
* request resubmitted after the context was banned.
*/
- if (unlikely(intel_context_is_banned(ce))) {
+ if (unlikely(!intel_context_is_schedulable(ce))) {
i915_request_put(i915_request_mark_eio(rq));
intel_engine_signal_breadcrumbs(ce->engine);
return 0;
@@ -871,15 +871,15 @@ static int guc_wq_item_append(struct intel_guc *guc,
struct i915_request *rq)
{
struct intel_context *ce = request_to_scheduling_context(rq);
- int ret = 0;
+ int ret;
- if (likely(!intel_context_is_banned(ce))) {
- ret = __guc_wq_item_append(rq);
+ if (unlikely(!intel_context_is_schedulable(ce)))
+ return 0;
- if (unlikely(ret == -EBUSY)) {
- guc->stalled_request = rq;
-
Re: [Intel-gfx] [PATCH v4 3/4] drm/i915: Make the heartbeat play nice with long pre-emption timeouts
On 03/10/2022 08:53, Tvrtko Ursulin wrote:
On 30/09/2022 18:44, John Harrison wrote:
On 9/30/2022 02:22, Tvrtko Ursulin wrote:
On 29/09/2022 17:21, John Harrison wrote:
On 9/29/2022 00:42, Tvrtko Ursulin wrote:
On 29/09/2022 03:18, john.c.harri...@intel.com wrote:
From: John Harrison
Compute workloads are inherently not pre-emptible for long periods on
current hardware. As a workaround for this, the pre-emption timeout
for compute capable engines was disabled. This is undesirable with
GuC
submission as it prevents per engine reset of hung contexts. Hence
the
next patch will re-enable the timeout but bumped up by an order of
magnitude.
However, the heartbeat might not respect that. Depending upon current
activity, a pre-emption to the heartbeat pulse might not even be
attempted until the last heartbeat period. Which means that only one
period is granted for the pre-emption to occur. With the aforesaid
bump, the pre-emption timeout could be significantly larger than this
heartbeat period.
So adjust the heartbeat code to take the pre-emption timeout into
account. When it reaches the final (high priority) period, it now
ensures the delay before hitting reset is bigger than the pre-emption
timeout.
v2: Fix for selftests which adjust the heartbeat period manually.
Signed-off-by: John Harrison
---
.../gpu/drm/i915/gt/intel_engine_heartbeat.c | 19
+++
1 file changed, 19 insertions(+)
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c
b/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c
index a3698f611f457..823a790a0e2ae 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c
+++ b/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c
@@ -22,9 +22,28 @@
static bool next_heartbeat(struct intel_engine_cs *engine)
{
+ struct i915_request *rq;
long delay;
delay = READ_ONCE(engine->props.heartbeat_interval_ms);
+
+ rq = engine->heartbeat.systole;
+
+ if (rq && rq->sched.attr.priority >= I915_PRIORITY_BARRIER &&
+ delay == engine->defaults.heartbeat_interval_ms) {
Maybe I forgot but what is the reason for the check against the
default heartbeat interval?
That's the 'v2: fix for selftests that manually adjust the
heartbeat'. If something (or someone) has explicitly set an override
of the heartbeat then it has to be assumed that they know what they
are doing, and if things don't work any more that's their problem.
But if we don't respect their override then they won't get the
timings they expect and the selftest will fail.
Isn't this a bit too strict for the non-selftest case? If the new
concept is extending the last pulse to guarantee preemption, then I
think we could allow tweaking of the heartbeat period. Like what if
user wants 1s, or 10s instead of 2.5s - why would that need to break
the improvement from this patch?
Then the user is back to where they were before this patch.
In what ways selftests fail? Are they trying to guess time to reset
based on the hearbeat period set? If so perhaps add a helper to query
it based on the last pulse extension.
I don't recall. It was six months ago when I was actually working on
this. And right now I do not have the time to go back and re-run all
the testing and re-write a bunch of self tests with whole new helpers
and algorithms and whatever else might be necessary to polish this to
perfection. And in the meantime, all the existing issues are still
present - there is no range checking on any of this stuff, it is very
possible for a driver with default settings to break a legal workload
because the heartbeat and pre-emption are fighting with each other, we
don't even have per engine resets enabled, etc.
Maybe it could be even better with a follow up patch. Feel free to do
that. But as it stands, this patch set significantly improves the
situation without making anything worse.
As we seem to be in agreement that the check against default heartbeat
is a hack with only purpose to work around assumptions made by
selftests, then please file a Jira about removing it (this hack). Then
work can be assigned to someone to clean it up. With that done I would
agree the series is indeed an improvement and it would have my ack.
One more thing - put a comment in the code along the lines of
"FIXME/HACK: Work around selftests assumptions by only extending the
last heartbeat if the period is at default value". The the Jira can
associate to that comment.
Until that is resolve it may also be worth emitting a drm_notice if
heartbeat is changed via sysfs? Informing users the things will not work
as expected if they fiddle with it. Whether as a blanket warning or
checking first the 3-4x heartbeat vs preempt timeout value. That message
should then go away once the follow up work to fix the selftests is
done. See what the other reviewers will think.
Regards,
Tvrtko
Re: [PATCH v2 1/2] drm/bridge: it6505: Adapt runtime power management framework
Il 03/10/22 07:03, Pin-yen Lin ha scritto:
Use pm_runtime_(get|put)_sync to control the bridge power, and add
SET_SYSTEM_SLEEP_PM_OPS with pm_runtime_force_(suspend|resume) to it6505
driver. Without SET_SYSTEM_SLEEP_PM_OPS, the bridge will be powered on
unnecessarily when no external display is connected.
Fixes: b5c84a9edcd4 ("drm/bridge: add it6505 driver")
Signed-off-by: Pin-yen Lin
---
Changes in v2:
- Handle the error from pm_runtime_get_sync in it6505_extcon_work
drivers/gpu/drm/bridge/ite-it6505.c | 33 +
1 file changed, 24 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ite-it6505.c
b/drivers/gpu/drm/bridge/ite-it6505.c
index 2bb957cffd94..685d8e750b12 100644
--- a/drivers/gpu/drm/bridge/ite-it6505.c
+++ b/drivers/gpu/drm/bridge/ite-it6505.c
@@ -421,6 +421,7 @@ struct it6505 {
struct notifier_block event_nb;
struct extcon_dev *extcon;
struct work_struct extcon_wq;
+ int extcon_state;
enum drm_connector_status connector_status;
enum link_train_status link_state;
struct work_struct link_works;
@@ -2685,31 +2686,42 @@ static void it6505_extcon_work(struct work_struct *work)
{
struct it6505 *it6505 = container_of(work, struct it6505, extcon_wq);
struct device *dev = >client->dev;
- int state = extcon_get_state(it6505->extcon, EXTCON_DISP_DP);
- unsigned int pwroffretry = 0;
+ int state, ret;
if (it6505->enable_drv_hold)
return;
mutex_lock(>extcon_lock);
+ state = extcon_get_state(it6505->extcon, EXTCON_DISP_DP);
DRM_DEV_DEBUG_DRIVER(dev, "EXTCON_DISP_DP = 0x%02x", state);
+
+ if (state == it6505->extcon_state)
+ goto unlock;
Even if it's unlikely for anything bad to happen, please add error handling,
or we might end up with unbalanced pm_runtime calls.
if (state == it6505->extcon_state || unlikely(state < 0))
goto unlock;
it6505->extcon_state = state;
if (state) {
} else {
}
Regards,
Angelo
Re: [PATCH v2 2/2] drm/bridge: it6505: Add pre_enable/post_disable callback
Il 03/10/22 07:03, Pin-yen Lin ha scritto:
Add atomic_pre_enable and atomic_post_disable callback to make sure the
bridge is not powered off until atomic_post_disable is called. This
prevents a power leakage when it6505 is powered off, but the upstream
DRM bridge is still sending display signals.
Fixes: b5c84a9edcd4 ("drm/bridge: add it6505 driver")
Signed-off-by: Pin-yen Lin
Reviewed-by: AngeloGioacchino Del Regno
Re: [PATCH] drm/i915/slpc: Update frequency debugfs for SLPC
On Fri, 30 Sep 2022, Vinay Belgaumkar wrote:
> Create a wrapper to print out the frequency debugfs for
> SLPC and non-SLPC cases. Most of the RPS related information
> is no longer valid when SLPC is enabled.
Please split code movement and functional changes to separate patches.
BR,
Jani.
>
> Signed-off-by: Vinay Belgaumkar
> ---
> drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c | 157 +-
> drivers/gpu/drm/i915/gt/intel_rps.c | 205 ++
> drivers/gpu/drm/i915/gt/intel_rps.h | 2 +
> 3 files changed, 208 insertions(+), 156 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c
> b/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c
> index 9fd4d9255a97..d5afa08c54e3 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c
> @@ -344,162 +344,7 @@ void intel_gt_pm_frequency_dump(struct intel_gt *gt,
> struct drm_printer *p)
> drm_printf(p, "efficient (RPe) frequency: %d MHz\n",
> intel_gpu_freq(rps, rps->efficient_freq));
> } else if (GRAPHICS_VER(i915) >= 6) {
> - u32 rp_state_limits;
> - u32 gt_perf_status;
> - struct intel_rps_freq_caps caps;
> - u32 rpmodectl, rpinclimit, rpdeclimit;
> - u32 rpstat, cagf, reqf;
> - u32 rpcurupei, rpcurup, rpprevup;
> - u32 rpcurdownei, rpcurdown, rpprevdown;
> - u32 rpupei, rpupt, rpdownei, rpdownt;
> - u32 pm_ier, pm_imr, pm_isr, pm_iir, pm_mask;
> -
> - rp_state_limits = intel_uncore_read(uncore,
> GEN6_RP_STATE_LIMITS);
> - gen6_rps_get_freq_caps(rps, );
> - if (IS_GEN9_LP(i915))
> - gt_perf_status = intel_uncore_read(uncore,
> BXT_GT_PERF_STATUS);
> - else
> - gt_perf_status = intel_uncore_read(uncore,
> GEN6_GT_PERF_STATUS);
> -
> - /* RPSTAT1 is in the GT power well */
> - intel_uncore_forcewake_get(uncore, FORCEWAKE_ALL);
> -
> - reqf = intel_uncore_read(uncore, GEN6_RPNSWREQ);
> - if (GRAPHICS_VER(i915) >= 9) {
> - reqf >>= 23;
> - } else {
> - reqf &= ~GEN6_TURBO_DISABLE;
> - if (IS_HASWELL(i915) || IS_BROADWELL(i915))
> - reqf >>= 24;
> - else
> - reqf >>= 25;
> - }
> - reqf = intel_gpu_freq(rps, reqf);
> -
> - rpmodectl = intel_uncore_read(uncore, GEN6_RP_CONTROL);
> - rpinclimit = intel_uncore_read(uncore, GEN6_RP_UP_THRESHOLD);
> - rpdeclimit = intel_uncore_read(uncore, GEN6_RP_DOWN_THRESHOLD);
> -
> - rpstat = intel_uncore_read(uncore, GEN6_RPSTAT1);
> - rpcurupei = intel_uncore_read(uncore, GEN6_RP_CUR_UP_EI) &
> GEN6_CURICONT_MASK;
> - rpcurup = intel_uncore_read(uncore, GEN6_RP_CUR_UP) &
> GEN6_CURBSYTAVG_MASK;
> - rpprevup = intel_uncore_read(uncore, GEN6_RP_PREV_UP) &
> GEN6_CURBSYTAVG_MASK;
> - rpcurdownei = intel_uncore_read(uncore, GEN6_RP_CUR_DOWN_EI) &
> GEN6_CURIAVG_MASK;
> - rpcurdown = intel_uncore_read(uncore, GEN6_RP_CUR_DOWN) &
> GEN6_CURBSYTAVG_MASK;
> - rpprevdown = intel_uncore_read(uncore, GEN6_RP_PREV_DOWN) &
> GEN6_CURBSYTAVG_MASK;
> -
> - rpupei = intel_uncore_read(uncore, GEN6_RP_UP_EI);
> - rpupt = intel_uncore_read(uncore, GEN6_RP_UP_THRESHOLD);
> -
> - rpdownei = intel_uncore_read(uncore, GEN6_RP_DOWN_EI);
> - rpdownt = intel_uncore_read(uncore, GEN6_RP_DOWN_THRESHOLD);
> -
> - cagf = intel_rps_read_actual_frequency(rps);
> -
> - intel_uncore_forcewake_put(uncore, FORCEWAKE_ALL);
> -
> - if (GRAPHICS_VER(i915) >= 11) {
> - pm_ier = intel_uncore_read(uncore,
> GEN11_GPM_WGBOXPERF_INTR_ENABLE);
> - pm_imr = intel_uncore_read(uncore,
> GEN11_GPM_WGBOXPERF_INTR_MASK);
> - /*
> - * The equivalent to the PM ISR & IIR cannot be read
> - * without affecting the current state of the system
> - */
> - pm_isr = 0;
> - pm_iir = 0;
> - } else if (GRAPHICS_VER(i915) >= 8) {
> - pm_ier = intel_uncore_read(uncore, GEN8_GT_IER(2));
> - pm_imr = intel_uncore_read(uncore, GEN8_GT_IMR(2));
> - pm_isr = intel_uncore_read(uncore, GEN8_GT_ISR(2));
> - pm_iir = intel_uncore_read(uncore, GEN8_GT_IIR(2));
> - } else {
> - pm_ier = intel_uncore_read(uncore, GEN6_PMIER);
> - pm_imr = intel_uncore_read(uncore, GEN6_PMIMR);
> - pm_isr =
Re: [RFC v2] drm/kms: control display brightness through drm_connector properties
Hi, On 10/3/22 12:32, Pekka Paalanen wrote: > On Mon, 3 Oct 2022 11:44:56 +0200 > Hans de Goede wrote: > >> One example mentioned here is that laptops with Intel integrated >> graphics may have some "perceived brightness" mapping table >> in their Video BIOS Tables (VBT) it would be interesting to use >> this and to export the curve coming from that to userspace as >> extra information (including allow userspace to write it). >> Since in this case (unlike many other cases) at least this >> curve is done in the kernel I guess we can then also add a boolean >> property to disable the curve (making it linear) in case that >> is helpful for HDR scenarios. > > Hi Hans, > > what if you would export that curve to userspace and not apply it in > the kernel, aside from the min-luminance=0 offset? > > I'm not sure if that was your intention, I didn't see it clearly said. > Of course this can be only about curves that are supposed to be applied > by the OS and not applied in firmware or hardware. Call it "software > curve"? > > Let userspace apply that curve if it happens to exist. Then, if we get > some other definition replacing that curve on some hardware, the kernel > could just expose the other thing as a new thing, and the old curve API > would not be interfering. Userspace that does not understand the new > thing (and the old curve property is not populated) would still be able > to control the brightness, just not in as pleasant way. > > It would also make using a custom curve a completely userspace thing with > no need for the kernel to support overwriting it. Userspace comes in many forms, which is why my preference for "software curve" support would be for it to be applied by the kernel by default (if present in the VBT) but then with a "software curve enable" flag to allow advanced userspace to disable it and then apply a curve of userspace's own choosing itself. This allows a simple implementation for desktop-environments which are unlikely to implement all this themselves, giving everyone the benefit of the nicer behavior of the VBT provided curve while allowing advanced desktop environments (likely mainly KDE + GNOME/mutter) to do something more advanced. Either way, this is all future extensions. First lets get the basic brightness control with just brightness + brightness-max attributes in place. Regards, Hans
Re: [RFC v2] drm/kms: control display brightness through drm_connector properties
On Mon, 3 Oct 2022 11:44:56 +0200 Hans de Goede wrote: > One example mentioned here is that laptops with Intel integrated > graphics may have some "perceived brightness" mapping table > in their Video BIOS Tables (VBT) it would be interesting to use > this and to export the curve coming from that to userspace as > extra information (including allow userspace to write it). > Since in this case (unlike many other cases) at least this > curve is done in the kernel I guess we can then also add a boolean > property to disable the curve (making it linear) in case that > is helpful for HDR scenarios. Hi Hans, what if you would export that curve to userspace and not apply it in the kernel, aside from the min-luminance=0 offset? I'm not sure if that was your intention, I didn't see it clearly said. Of course this can be only about curves that are supposed to be applied by the OS and not applied in firmware or hardware. Call it "software curve"? Let userspace apply that curve if it happens to exist. Then, if we get some other definition replacing that curve on some hardware, the kernel could just expose the other thing as a new thing, and the old curve API would not be interfering. Userspace that does not understand the new thing (and the old curve property is not populated) would still be able to control the brightness, just not in as pleasant way. It would also make using a custom curve a completely userspace thing with no need for the kernel to support overwriting it. Thanks, pq pgp69AHOPFZPI.pgp Description: OpenPGP digital signature
Re: [RFC v2] drm/kms: control display brightness through drm_connector properties
On Mon, 3 Oct 2022 12:29:01 +0300 Ville Syrjälä wrote: > On Mon, Oct 03, 2022 at 11:37:50AM +0300, Pekka Paalanen wrote: > > On Fri, 30 Sep 2022 18:17:39 +0200 > > Sebastian Wick wrote: > > > > > On Fri, Sep 30, 2022 at 5:27 PM Pekka Paalanen > > > wrote: > > > > > > > > On Fri, 30 Sep 2022 17:44:17 +0300 > > > > Ville Syrjälä wrote: > > > > > > > > > On Fri, Sep 30, 2022 at 04:20:29PM +0200, Sebastian Wick wrote: > > > > > > On Fri, Sep 30, 2022 at 9:40 AM Pekka Paalanen > > > > > > wrote: > > > > > > > > > > > > > > On Thu, 29 Sep 2022 20:06:50 +0200 > > > > > > > Sebastian Wick wrote: > > > > > > > > > > > > > > > If it is supposed to be a non-linear luminance curve, which one > > > > > > > > is it? > > > > > > > > It would be much clearer if user space can control linear > > > > > > > > luminance > > > > > > > > and use whatever definition of perceived brightness it wants. > > > > > > > > The > > > > > > > > obvious downside of it is that it requires bits to encode > > > > > > > > changes that > > > > > > > > users can't perceive. What about backlights which only have a > > > > > > > > few > > > > > > > > predefined luminance levels? How would user space differentiate > > > > > > > > between the continuous and discrete backlight? What about > > > > > > > > self-emitting displays? They usually increase the dynamic range > > > > > > > > when > > > > > > > > they increase in brightness because the black level doesn't > > > > > > > > rise. They > > > > > > > > also probably employ some tonemapping to adjust for that. What > > > > > > > > about > > > > > > > > the range of the backlight? What about the absolute luminance > > > > > > > > of the > > > > > > > > backlight? I want to know about that all in user space. > > > > > > > > > > > > > > > > I understand that most of the time the kernel doesn't have > > > > > > > > answers to > > > > > > > > those questions right now but the API should account for all of > > > > > > > > that. ... > > I suppose the firmware may expose some tables that may allow mapping > > raw hardware values into something more pleasant to use. Like something > > where each step is more or less a visible change. That does not have to > > imply anything about linearity in any space, they may just be "good > > values" for e.g. keyboard-based changing of backlight levels with no > > mathematical or physical basis. > > > > Ville, what kind of tables do you know about? What do they actually > > tell? > > We just get a set of points (up to 20 originally, and I think up to > 32 in later spec revisions) that map input brightness (in %) to > output duty cycle (0-0xff in old spec, 0-0x in new spec). > And I *think* we're supposed to just linearly inteprolate between > the entries, though the spec doesn't really state that in super > clear terms. > > There is some mention in the spec about this being more or less > designed for Windows Vista, so a look through eg. > https://learn.microsoft.com/en-us/windows-hardware/drivers/display/supporting-brightness-controls-on-integrated-display-panels > might be a good idea here. That's a nice link. Quote: "Brightness levels are represented as single-byte values in the range from zero to 100 where zero is off and 100 is the maximum brightness that a laptop computer supports. Every laptop computer must report a maximum brightness level of 100; however, a laptop computer is not required to support a level of zero. The only requirement for values from zero to 100 is that larger values must represent higher brightness levels. The increment between levels is not required to be uniform, and a laptop computer can support any number of distinct values up to the maximum of 101 levels. You must decide how to map hardware levels to the range of brightness level values. However, a call to the display miniport driver's DxgkDdiGetPossibleBrightness function should not report more brightness level values than the hardware supports." Sounds like "good values" to me, and that each value must be distinguishable from any another (at least electrically). Thanks, pq pgpwsFeSNM432.pgp Description: OpenPGP digital signature
Re: [RFC v2] drm/kms: control display brightness through drm_connector properties
Hi, On 9/30/22 18:17, Sebastian Wick wrote: > On Fri, Sep 30, 2022 at 5:27 PM Pekka Paalanen wrote: >> >> On Fri, 30 Sep 2022 17:44:17 +0300 >> Ville Syrjälä wrote: >> >>> On Fri, Sep 30, 2022 at 04:20:29PM +0200, Sebastian Wick wrote: On Fri, Sep 30, 2022 at 9:40 AM Pekka Paalanen wrote: > > On Thu, 29 Sep 2022 20:06:50 +0200 > Sebastian Wick wrote: > >> If it is supposed to be a non-linear luminance curve, which one is it? >> It would be much clearer if user space can control linear luminance >> and use whatever definition of perceived brightness it wants. The >> obvious downside of it is that it requires bits to encode changes that >> users can't perceive. What about backlights which only have a few >> predefined luminance levels? How would user space differentiate >> between the continuous and discrete backlight? What about >> self-emitting displays? They usually increase the dynamic range when >> they increase in brightness because the black level doesn't rise. They >> also probably employ some tonemapping to adjust for that. What about >> the range of the backlight? What about the absolute luminance of the >> backlight? I want to know about that all in user space. >> >> I understand that most of the time the kernel doesn't have answers to >> those questions right now but the API should account for all of that. > > Hi, > > if the API accounts for all that, and the kernel doesn't know, then how > can the API not lie? If the API sometimes lies, how could we ever trust > it at all? Make it possible for the API to say "I don't know". I'd much rather have an API tell me explicitly what it does and doesn't know instead of having to guess what data I can actually rely on. For example if the kernel knows the luminance is linear on one display and doesn't know anything about the other display and it exposes them both in the same way I can not possibly write any code which relies on exact control over the luminance for either display. > > Personally I have the feeling that if we can even get to the level of > "each step in the value is a more or less perceivable change", that > would be good enough. Think of UI, e.g. hotkeys to change brightness. > You'd expect almost every press to change it a bit. The nice thing is that you can have that even if you have no further information about the brightness control and it might be good enough for some use cases but it isn't for others. > If an end user wants defined and controlled luminance, I'd suggest they > need to profile (physically measure) the response of the display at > hand. This is no different from color profiling displays, but you need > a measurement device that produces absolute measurements if absolute > control is what they want. If that's the kind of user experience you're after, good for you. I certainly want things to work out of the box which makes this just a big no-go. >>> >>> I think if we have the information to make the default behaviour >>> better then we should do that. Ie. if the firmaware gives us a >>> table to remap the values for a more linear response we should >>> make use of that by default. >> >> But that's only like 20% of what Sebastian is asking for. >> >> What's "linear"? Radiometric or perceptual? >> >> Radiometric linear control would make a terrible UX, so if the control >> is radiometric, userspace needs to remap it. That might be a good >> thing, but it's also complicated, because the relationship between >> brightness and luminance is somewhere between a power curve and >> exponential curve. You need to make sure that the userspace remapping >> works for different backlights with different luminance ranges. That's >> not obvious to me. >> >>> We can of course provide a way for the user to plug in their own >>> actually measured data later. But IMO that doesn't even have to >>> happen in the initial implementation. Just need to avoid painting >>> ourselves totally in the corner in a way that would prevent later >>> additions like that. >> >> For userspace delivering its own curve, you need to define the units. >> Absolute or relative? Radiometric or perceptual? Otherwise the >> resulting control is not portable between window systems. >> >>> I just hate the current limbo where we're somehow too afraid to >>> change the current behaviour to do the remapping by default. >>> I see no upsides in the current behaviour of just blindly >>> exposing the raw hardware register values more or less. They >>> mean absolutely nothing to any user. >> >> I never argued like that. >> >> I'm saying that what looks realistic to me is somewhere *between* >> status quo and what Sebastian is asking for. Whatever you mean by "linear >> remapping" is probably a realistic goal, because you know you have
Re: [Intel-gfx] [PATCH] drm/i915/guc: Fix revocation of non-persistent contexts
On 03.10.2022 09:59, Tvrtko Ursulin wrote:
On 30/09/2022 15:52, Andrzej Hajda wrote:
On 30.09.2022 11:47, Tvrtko Ursulin wrote:
From: Tvrtko Ursulin
Patch which added graceful exit for non-persistent contexts missed the
fact it is not enough to set the exiting flag on a context and let the
backend handle it from there.
GuC backend cannot handle it because it runs independently in the
firmware and driver might not see the requests ever again. Patch also
missed the fact some usages of intel_context_is_banned in the GuC
backend
needed replacing with newly introduced intel_context_is_schedulable.
Fix the first issue by calling into backend revoke when we know this is
the last chance to do it. Fix the second issue by replacing
intel_context_is_banned with intel_context_is_schedulable, which should
always be safe since latter is a superset of the former.
negation of the latter is a ...?
I did not get what you meant here.
Signed-off-by: Tvrtko Ursulin
Fixes: 45c64ecf97ee ("drm/i915: Improve user experience and driver
robustness under SIGINT or similar")
Cc: Andrzej Hajda
Cc: John Harrison
Cc: Daniele Ceraolo Spurio
---
drivers/gpu/drm/i915/gem/i915_gem_context.c | 8 +-
drivers/gpu/drm/i915/gt/intel_context.c | 14 +++---
drivers/gpu/drm/i915/gt/intel_context.h | 8 +-
.../gpu/drm/i915/gt/uc/intel_guc_submission.c | 26 +--
4 files changed, 25 insertions(+), 31 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c
b/drivers/gpu/drm/i915/gem/i915_gem_context.c
index 0bcde53c50c6..1e29b1e6d186 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
@@ -1387,14 +1387,8 @@ kill_engines(struct i915_gem_engines *engines,
bool exit, bool persistent)
*/
for_each_gem_engine(ce, engines, it) {
struct intel_engine_cs *engine;
- bool skip = false;
- if (exit)
- skip = intel_context_set_exiting(ce);
- else if (!persistent)
- skip = intel_context_exit_nonpersistent(ce, NULL); > -
- if (skip)
+ if ((exit || !persistent) && intel_context_revoke(ce))
continue; /* Already marked. */
/*
diff --git a/drivers/gpu/drm/i915/gt/intel_context.c
b/drivers/gpu/drm/i915/gt/intel_context.c
index 654a092ed3d6..398b2a9eed61 100644
--- a/drivers/gpu/drm/i915/gt/intel_context.c
+++ b/drivers/gpu/drm/i915/gt/intel_context.c
@@ -614,13 +614,19 @@ bool intel_context_ban(struct intel_context
*ce, struct i915_request *rq)
return ret;
}
-bool intel_context_exit_nonpersistent(struct intel_context *ce,
- struct i915_request *rq)
+bool intel_context_revoke(struct intel_context *ce)
{
bool ret = intel_context_set_exiting(ce);
- if (ce->ops->revoke)
- ce->ops->revoke(ce, rq, ce->engine->props.preempt_timeout_ms);
+ if (!ret && intel_engine_uses_guc(ce->engine)) {
+ /*
+ * With GuC backend we have to notify it of revocation as soon
+ * as the exiting flag is set.
+ */
+ if (ce->ops->revoke)
+ ce->ops->revoke(ce, NULL,
+ ce->engine->props.preempt_timeout_ms);
+ }
Now revoke is called only with GuC, previously it was called also for
other backends in case non-exiting/non-persistent, is it OK?
It is okay (execlists has no revoke vfunc, ringbuffer has it but only
works if target request is known), but agreed it is a bit ugly. I was in
two minds which way to go. Perhaps it would indeed be cleaner to go
unconditional. I will resend with that change, copying stable this time
round (since 6.0 is out), and can keep your r-b?
Yes, please keep r-b.
Regards
Andrzej
Regards,
Tvrtko
return ret;
}
diff --git a/drivers/gpu/drm/i915/gt/intel_context.h
b/drivers/gpu/drm/i915/gt/intel_context.h
index 8e2d70630c49..40f8809d14ea 100644
--- a/drivers/gpu/drm/i915/gt/intel_context.h
+++ b/drivers/gpu/drm/i915/gt/intel_context.h
@@ -319,18 +319,12 @@ static inline bool
intel_context_is_schedulable(const struct intel_context *ce)
!test_bit(CONTEXT_BANNED, >flags);
}
-static inline bool intel_context_is_exiting(const struct
intel_context *ce)
-{
- return test_bit(CONTEXT_EXITING, >flags);
-}
-
static inline bool intel_context_set_exiting(struct intel_context *ce)
{
return test_and_set_bit(CONTEXT_EXITING, >flags);
}
-bool intel_context_exit_nonpersistent(struct intel_context *ce,
- struct i915_request *rq);
+bool intel_context_revoke(struct intel_context *ce);
static inline bool
intel_context_force_single_submission(const struct intel_context *ce)
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
index 0ef295a94060..88a4476b8e92 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
@@ -685,7 +685,7
Re: [RFC v2] drm/kms: control display brightness through drm_connector properties
On Mon, Oct 03, 2022 at 11:37:50AM +0300, Pekka Paalanen wrote: > On Fri, 30 Sep 2022 18:17:39 +0200 > Sebastian Wick wrote: > > > On Fri, Sep 30, 2022 at 5:27 PM Pekka Paalanen wrote: > > > > > > On Fri, 30 Sep 2022 17:44:17 +0300 > > > Ville Syrjälä wrote: > > > > > > > On Fri, Sep 30, 2022 at 04:20:29PM +0200, Sebastian Wick wrote: > > > > > On Fri, Sep 30, 2022 at 9:40 AM Pekka Paalanen > > > > > wrote: > > > > > > > > > > > > On Thu, 29 Sep 2022 20:06:50 +0200 > > > > > > Sebastian Wick wrote: > > > > > > > > > > > > > If it is supposed to be a non-linear luminance curve, which one > > > > > > > is it? > > > > > > > It would be much clearer if user space can control linear > > > > > > > luminance > > > > > > > and use whatever definition of perceived brightness it wants. The > > > > > > > obvious downside of it is that it requires bits to encode changes > > > > > > > that > > > > > > > users can't perceive. What about backlights which only have a few > > > > > > > predefined luminance levels? How would user space differentiate > > > > > > > between the continuous and discrete backlight? What about > > > > > > > self-emitting displays? They usually increase the dynamic range > > > > > > > when > > > > > > > they increase in brightness because the black level doesn't rise. > > > > > > > They > > > > > > > also probably employ some tonemapping to adjust for that. What > > > > > > > about > > > > > > > the range of the backlight? What about the absolute luminance of > > > > > > > the > > > > > > > backlight? I want to know about that all in user space. > > > > > > > > > > > > > > I understand that most of the time the kernel doesn't have > > > > > > > answers to > > > > > > > those questions right now but the API should account for all of > > > > > > > that. > > ... > > > > I'm saying that what looks realistic to me is somewhere *between* > > > status quo and what Sebastian is asking for. Whatever you mean by "linear > > > remapping" is probably a realistic goal, because you know you have some > > > hardware/firmware delivering that information already. > > > > > > OTOH, designing UAPI for information that exists only in our dreams > > > is... well. > > > > I also didn't say that we should design an UAPI for what doesn't > > exist, just that we should design the UAPI we actually need in a way > > that when we get more information we can properly expose that. So if > > the UAPI exposes anything other than the luminance (e.g. "each step is > > a perceptual step in brightness", "linear brightness", ..) we have to > > put some human perception model into the kernel which is ridiculous > > and it makes it impossible to expose luminance to user space if the > > kernel has that information. > > You don't need a human perception model in the kernel. You also cannot > have one, because I would expect most or all backlight and their > metadata to not define luminance at all. But that is just a guess. > > I suppose the firmware may expose some tables that may allow mapping > raw hardware values into something more pleasant to use. Like something > where each step is more or less a visible change. That does not have to > imply anything about linearity in any space, they may just be "good > values" for e.g. keyboard-based changing of backlight levels with no > mathematical or physical basis. > > Ville, what kind of tables do you know about? What do they actually > tell? We just get a set of points (up to 20 originally, and I think up to 32 in later spec revisions) that map input brightness (in %) to output duty cycle (0-0xff in old spec, 0-0x in new spec). And I *think* we're supposed to just linearly inteprolate between the entries, though the spec doesn't really state that in super clear terms. There is some mention in the spec about this being more or less designed for Windows Vista, so a look through eg. https://learn.microsoft.com/en-us/windows-hardware/drivers/display/supporting-brightness-controls-on-integrated-display-panels might be a good idea here. -- Ville Syrjälä Intel
Re: KMS atomic state sets, full vs. minimal (Re: [PATCH v3 0/6] Add support for atomic async page-flips)
On Mon, Oct 03, 2022 at 11:48:49AM +0300, Pekka Paalanen wrote: > On Fri, 30 Sep 2022 18:45:09 +0300 > Ville Syrjälä wrote: > > > On Fri, Sep 30, 2022 at 06:37:00PM +0300, Pekka Paalanen wrote: > > > On Fri, 30 Sep 2022 18:09:55 +0300 > > > Ville Syrjälä wrote: > > > > > > > That would actively discourage people from even attempting the > > > > "just dump all the state into the ioctl" approach with async flips > > > > since even the props whose value isn't even changing would be rejected. > > > > > > > > > > About that. > > > > > > To me it looks like just a classic case of broken communication. > > > > > > The kernel developers assume that of course userspace will minimize the > > > state set to only those properties that change, because...? > > > > > > Userspace developers think that of course the kernel will optimize the > > > state set into minimal changes, because the kernel actually has the > > > authoritative knowledge of what the current state is, and the driver > > > actually knows which properties are costly and need to be optimized and > > > which ones don't matter. It has never even occurred to me that the > > > kernel would not compare next state to current state. > > > > > > No-one ever documented any expectations, did they? > > > > Do you really want that for async flips? Async flips can't be > > done atomically with anything else, so why are you even asking > > the kernel to do that? > > I'm not talking about async flips only. > > I'm talking about atomic commits in general. I don't think it's a good > idea to make async atomic commits behave fundamentally different from > sync atomic commits wrt. what non-changing state you are allowed to > list in your state set or not. > > Isn't there common DRM code to convert an atomic commit state set into > state objects? It probably starts by copying the current state, and > then playing through the commit state set, setting all listed > properties to their new values? Why wouldn't that loop maintain the > knowledge of what actually changed? Any such book keeping is entirely ad-hoc atm. It's also not super obvious how much of it is actually useful. You have to do a real commit on the crtc anyway if the crtc (or on any of its associated objects) is listed in the commit, so there's not necessarily much to be gained by tracking chages in all properties. And that behaviour again enters very muddy waters when combined with the async flip flag for the entire commit. The current approach being proposed seems to suggest that we can instead short circuit async commits when nothing has changed. That is not at all how sync atomic commits work. > When you copy the current data, reset all changed-flags to false. When > you apply each property from the commit set, compare the value and set > the changed-flag only if the value changes. > > This manufacturing of the new tentative state set happens at atomic > commit ioctl time before the ioctl returns, right? So the current state > is well-defined: any previous atomic sync or async commit can be assumed to > have succeeded even if it hasn't applied in hardware yet if the commit > ioctl for it succeeded, right? Yes. We could certainly try to fully track all changes in properties, but no has measured if there's any real benefit of doing so. -- Ville Syrjälä Intel
Re: [RFC v2] drm/kms: control display brightness through drm_connector properties
Hi, On 9/28/22 12:57, Ville Syrjälä wrote: > On Wed, Sep 28, 2022 at 01:04:01PM +0300, Jani Nikula wrote: >> On Fri, 09 Sep 2022, Hans de Goede wrote: >>> Hi all, >>> >>> Here is v2 of my "drm/kms: control display brightness through drm_connector >>> properties" RFC: >>> >>> Changes from version 1: >>> - Drop bl_brightness_0_is_min_brightness from list of new connector >>> properties. >>> - Clearly define that 0 is always min-brightness when setting the brightness >>> through the connector properties. >>> - Drop bl_brightness_control_method from list of new connector >>> properties. >>> - Phase 1 of the plan has been completed >>> >>> As discussed already several times in the past: >>> https://www.x.org/wiki/Events/XDC2014/XDC2014GoedeBacklight/ >>> >>> https://lore.kernel.org/all/4b17ba08-39f3-57dd-5aad-d37d844b0...@linux.intel.com/ >>> >>> The current userspace API for brightness control offered by >>> /sys/class/backlight devices has various issues: >>> >>> 1. There is no way to map the backlight device to a specific >>>display-output / panel (1) >>> 2. Controlling the brightness requires root-rights requiring >>>desktop-environments to use suid-root helpers for this. >>> 3. The meaning of 0 is not clearly defined, it can be either off, >>>or minimum brightness at which the display is still readable >>>(in a low light environment) >>> 4. It's not possible to change both the gamma and the brightness in the >>>same KMS atomic commit. You'd want to be able to reduce brightness to >>>conserve power, and counter the effects of that by changing gamma to >>>reach a visually similar image. And you'd want to have the changes take >>>effect at the same time instead of reducing brightness at some frame and >>>change gamma at some other frame. This is pretty much impossible to do >>>via the sysfs interface. >>> >>> As already discussed on various conference's hallway tracks >>> and as has been proposed on the dri-devel list once before (2), >>> it seems that there is consensus that the best way to to solve these >>> 2 issues is to add support for controlling a video-output's brightness >>> through properties on the drm_connector. >>> >>> This RFC outlines my plan to try and actually implement this, >>> which has 3 phases: >>> >>> >>> Phase 1: Stop registering multiple /sys/class/backlight devs for a single >>> display >>> = >>> >>> On x86 there can be multiple firmware + direct-hw-access methods >>> for controlling the backlight and in some cases the kernel registers >>> multiple backlight-devices for a single internal laptop LCD panel. >>> >>> A plan to fix this was posted here: >>> https://lore.kernel.org/dri-devel/98519ba0-7f18-201a-ea34-652f50343...@redhat.com/ >>> And a pull-req actually implementing this plan has been send out this week: >>> https://lore.kernel.org/dri-devel/261afe3d-7790-e945-adf6-a2c96c9b1...@redhat.com/ >>> >>> >>> Phase 2: Add drm_connector properties mirroring the matching backlight >>> device >>> = >>> >>> The plan is to add a drm_connector helper function, which optionally takes >>> a pointer to the backlight device for the GPU's native backlight device, >>> which will then mirror the backlight settings from the backlight device >>> in a set of read/write brightness* properties on the connector. >>> >>> This function can then be called by GPU drivers for the drm_connector for >>> the internal panel and it will then take care of everything. When there >>> is no native GPU backlight device, or when it should not be used then >>> (on x86) the helper will use the acpi_video_get_backlight_type() to >>> determine which backlight-device should be used instead and it will find >>> + mirror that one. >>> >>> >>> Phase 3: Deprecate /sys/class/backlight uAPI >>> >>> >>> Once most userspace has moved over to using the new drm_connector >>> brightness props, a Kconfig option can be added to stop exporting >>> the backlight-devices under /sys/class/backlight. The plan is to >>> just disable the sysfs interface and keep the existing backlight-device >>> internal kernel abstraction as is, since some abstraction for (non GPU >>> native) backlight devices will be necessary regardless. >>> >>> It is unsure if we will ever be able to do this. For example people using >>> non fully integrated desktop environments like e.g. sway often use custom >>> scripts binded to hotkeys to get functionality like the brightness >>> up/down keyboard hotkeys changing the brightness. This typically involves >>> e.g. the xbacklight utility. >>> >>> Even if the xbacklight utility is ported to use kms with the new connector >>> object brightness properties then this still will not work because >>> changing the properties will require drm-master rights and e.g. sway will >>>
Re: [Intel-gfx] [PATCH v2 14/14] drm/i915/mtl: Add multicast steering for media GT
Hi Matt,
On 01/10/2022 01:45, Matt Roper wrote:
MTL's media GT only has a single type of steering ("OAADDRM") which
selects between media slice 0 and media slice 1. We'll always steer to
media slice 0 unless it is fused off (which is the case when VD0, VE0,
and SFC0 are all reported as unavailable).
Bspec: 67789
Signed-off-by: Matt Roper
---
drivers/gpu/drm/i915/gt/intel_gt_mcr.c | 19 +--
drivers/gpu/drm/i915/gt/intel_gt_types.h| 1 +
drivers/gpu/drm/i915/gt/intel_workarounds.c | 18 +-
3 files changed, 35 insertions(+), 3 deletions(-)
[snip]
+static void
+mtl_media_gt_workarounds_init(struct intel_gt *gt, struct i915_wa_list *wal)
+{
+ /*
+* Unlike older platforms, we no longer setup implicit steering here;
+* all MCR accesses are explicitly steered.
+*/
+ if (drm_debug_enabled(DRM_UT_DRIVER)) {
+ struct drm_printer p = drm_debug_printer("MCR Steering:");
+
+ intel_gt_mcr_report_steering(, gt, false);
+ }
+}
+
static void
gt_init_workarounds(struct intel_gt *gt, struct i915_wa_list *wal)
{
struct drm_i915_private *i915 = gt->i915;
- if (IS_METEORLAKE(i915) && gt->type == GT_PRIMARY)
+ if (IS_METEORLAKE(i915) && gt->type == GT_MEDIA)
+ mtl_media_gt_workarounds_init(gt, wal);
+ else if (IS_METEORLAKE(i915) && gt->type == GT_PRIMARY)
mtl_3d_gt_workarounds_init(gt, wal);
else if (IS_PONTEVECCHIO(i915))
pvc_gt_workarounds_init(gt, wal);
Casually reading only - wouldn't it be nicer if the if-ladder in here
(gt_init_workarounds) would have a single case per platform, and then
you'd fork further (3d vs media) in MTL specific function?
Also, series ends up with mtl_media_gt_workarounds_init and
mtl_3d_gt_workarounds_init apparently 100% identical. You will need two
copies in the future?
Regards,
Tvrtko
Re: [RFC v2] drm/kms: control display brightness through drm_connector properties
Hi, On 9/28/22 12:04, Jani Nikula wrote: > On Fri, 09 Sep 2022, Hans de Goede wrote: >> Hi all, >> >> Here is v2 of my "drm/kms: control display brightness through drm_connector >> properties" RFC: >> >> Changes from version 1: >> - Drop bl_brightness_0_is_min_brightness from list of new connector >> properties. >> - Clearly define that 0 is always min-brightness when setting the brightness >> through the connector properties. >> - Drop bl_brightness_control_method from list of new connector >> properties. >> - Phase 1 of the plan has been completed >> >> As discussed already several times in the past: >> https://www.x.org/wiki/Events/XDC2014/XDC2014GoedeBacklight/ >> >> https://lore.kernel.org/all/4b17ba08-39f3-57dd-5aad-d37d844b0...@linux.intel.com/ >> >> The current userspace API for brightness control offered by >> /sys/class/backlight devices has various issues: >> >> 1. There is no way to map the backlight device to a specific >>display-output / panel (1) >> 2. Controlling the brightness requires root-rights requiring >>desktop-environments to use suid-root helpers for this. >> 3. The meaning of 0 is not clearly defined, it can be either off, >>or minimum brightness at which the display is still readable >>(in a low light environment) >> 4. It's not possible to change both the gamma and the brightness in the >>same KMS atomic commit. You'd want to be able to reduce brightness to >>conserve power, and counter the effects of that by changing gamma to >>reach a visually similar image. And you'd want to have the changes take >>effect at the same time instead of reducing brightness at some frame and >>change gamma at some other frame. This is pretty much impossible to do >>via the sysfs interface. >> >> As already discussed on various conference's hallway tracks >> and as has been proposed on the dri-devel list once before (2), >> it seems that there is consensus that the best way to to solve these >> 2 issues is to add support for controlling a video-output's brightness >> through properties on the drm_connector. >> >> This RFC outlines my plan to try and actually implement this, >> which has 3 phases: >> >> >> Phase 1: Stop registering multiple /sys/class/backlight devs for a single >> display >> = >> >> On x86 there can be multiple firmware + direct-hw-access methods >> for controlling the backlight and in some cases the kernel registers >> multiple backlight-devices for a single internal laptop LCD panel. >> >> A plan to fix this was posted here: >> https://lore.kernel.org/dri-devel/98519ba0-7f18-201a-ea34-652f50343...@redhat.com/ >> And a pull-req actually implementing this plan has been send out this week: >> https://lore.kernel.org/dri-devel/261afe3d-7790-e945-adf6-a2c96c9b1...@redhat.com/ >> >> >> Phase 2: Add drm_connector properties mirroring the matching backlight device >> = >> >> The plan is to add a drm_connector helper function, which optionally takes >> a pointer to the backlight device for the GPU's native backlight device, >> which will then mirror the backlight settings from the backlight device >> in a set of read/write brightness* properties on the connector. >> >> This function can then be called by GPU drivers for the drm_connector for >> the internal panel and it will then take care of everything. When there >> is no native GPU backlight device, or when it should not be used then >> (on x86) the helper will use the acpi_video_get_backlight_type() to >> determine which backlight-device should be used instead and it will find >> + mirror that one. >> >> >> Phase 3: Deprecate /sys/class/backlight uAPI >> >> >> Once most userspace has moved over to using the new drm_connector >> brightness props, a Kconfig option can be added to stop exporting >> the backlight-devices under /sys/class/backlight. The plan is to >> just disable the sysfs interface and keep the existing backlight-device >> internal kernel abstraction as is, since some abstraction for (non GPU >> native) backlight devices will be necessary regardless. >> >> It is unsure if we will ever be able to do this. For example people using >> non fully integrated desktop environments like e.g. sway often use custom >> scripts binded to hotkeys to get functionality like the brightness >> up/down keyboard hotkeys changing the brightness. This typically involves >> e.g. the xbacklight utility. >> >> Even if the xbacklight utility is ported to use kms with the new connector >> object brightness properties then this still will not work because >> changing the properties will require drm-master rights and e.g. sway will >> already hold those. >> >> >> The drm_connector brightness properties >> === >> >> The new uAPI for this consists of 2 properties: >>
Re: KMS atomic state sets, full vs. minimal (Re: [PATCH v3 0/6] Add support for atomic async page-flips)
On Fri, 30 Sep 2022 18:45:09 +0300 Ville Syrjälä wrote: > On Fri, Sep 30, 2022 at 06:37:00PM +0300, Pekka Paalanen wrote: > > On Fri, 30 Sep 2022 18:09:55 +0300 > > Ville Syrjälä wrote: > > > > > That would actively discourage people from even attempting the > > > "just dump all the state into the ioctl" approach with async flips > > > since even the props whose value isn't even changing would be rejected. > > > > About that. > > > > To me it looks like just a classic case of broken communication. > > > > The kernel developers assume that of course userspace will minimize the > > state set to only those properties that change, because...? > > > > Userspace developers think that of course the kernel will optimize the > > state set into minimal changes, because the kernel actually has the > > authoritative knowledge of what the current state is, and the driver > > actually knows which properties are costly and need to be optimized and > > which ones don't matter. It has never even occurred to me that the > > kernel would not compare next state to current state. > > > > No-one ever documented any expectations, did they? > > Do you really want that for async flips? Async flips can't be > done atomically with anything else, so why are you even asking > the kernel to do that? I'm not talking about async flips only. I'm talking about atomic commits in general. I don't think it's a good idea to make async atomic commits behave fundamentally different from sync atomic commits wrt. what non-changing state you are allowed to list in your state set or not. Isn't there common DRM code to convert an atomic commit state set into state objects? It probably starts by copying the current state, and then playing through the commit state set, setting all listed properties to their new values? Why wouldn't that loop maintain the knowledge of what actually changed? When you copy the current data, reset all changed-flags to false. When you apply each property from the commit set, compare the value and set the changed-flag only if the value changes. This manufacturing of the new tentative state set happens at atomic commit ioctl time before the ioctl returns, right? So the current state is well-defined: any previous atomic sync or async commit can be assumed to have succeeded even if it hasn't applied in hardware yet if the commit ioctl for it succeeded, right? Thanks, pq pgpjf_7P6u4Dq.pgp Description: OpenPGP digital signature
Re: [RFC v2] drm/kms: control display brightness through drm_connector properties
On Fri, 30 Sep 2022 18:17:39 +0200 Sebastian Wick wrote: > On Fri, Sep 30, 2022 at 5:27 PM Pekka Paalanen wrote: > > > > On Fri, 30 Sep 2022 17:44:17 +0300 > > Ville Syrjälä wrote: > > > > > On Fri, Sep 30, 2022 at 04:20:29PM +0200, Sebastian Wick wrote: > > > > On Fri, Sep 30, 2022 at 9:40 AM Pekka Paalanen > > > > wrote: > > > > > > > > > > On Thu, 29 Sep 2022 20:06:50 +0200 > > > > > Sebastian Wick wrote: > > > > > > > > > > > If it is supposed to be a non-linear luminance curve, which one is > > > > > > it? > > > > > > It would be much clearer if user space can control linear luminance > > > > > > and use whatever definition of perceived brightness it wants. The > > > > > > obvious downside of it is that it requires bits to encode changes > > > > > > that > > > > > > users can't perceive. What about backlights which only have a few > > > > > > predefined luminance levels? How would user space differentiate > > > > > > between the continuous and discrete backlight? What about > > > > > > self-emitting displays? They usually increase the dynamic range when > > > > > > they increase in brightness because the black level doesn't rise. > > > > > > They > > > > > > also probably employ some tonemapping to adjust for that. What about > > > > > > the range of the backlight? What about the absolute luminance of the > > > > > > backlight? I want to know about that all in user space. > > > > > > > > > > > > I understand that most of the time the kernel doesn't have answers > > > > > > to > > > > > > those questions right now but the API should account for all of > > > > > > that. ... > > I'm saying that what looks realistic to me is somewhere *between* > > status quo and what Sebastian is asking for. Whatever you mean by "linear > > remapping" is probably a realistic goal, because you know you have some > > hardware/firmware delivering that information already. > > > > OTOH, designing UAPI for information that exists only in our dreams > > is... well. > > I also didn't say that we should design an UAPI for what doesn't > exist, just that we should design the UAPI we actually need in a way > that when we get more information we can properly expose that. So if > the UAPI exposes anything other than the luminance (e.g. "each step is > a perceptual step in brightness", "linear brightness", ..) we have to > put some human perception model into the kernel which is ridiculous > and it makes it impossible to expose luminance to user space if the > kernel has that information. You don't need a human perception model in the kernel. You also cannot have one, because I would expect most or all backlight and their metadata to not define luminance at all. But that is just a guess. I suppose the firmware may expose some tables that may allow mapping raw hardware values into something more pleasant to use. Like something where each step is more or less a visible change. That does not have to imply anything about linearity in any space, they may just be "good values" for e.g. keyboard-based changing of backlight levels with no mathematical or physical basis. Ville, what kind of tables do you know about? What do they actually tell? Let's say we have these first properties defined as "reasonable steps for manual backlight control": one integer for the value, another integer for the max. If we later see that we can actually get more precise or high-level information, we can add new optional properties, e.g. a blob with table that maps the integers into some better defined quantity. Then we know what the values mean, but the steps may be quite coarse, much coarser than what the raw control value allows. That's the next problem: if we want as fine control as hardware is capable, how do you expose that? Should the answer be, that the exposed integer is actually a raw value for hardware, merely offsetted so that zero maps to minimum but visible backlight level, and then add another property from the start with a table for the "good values" for keyboard control? And the "good values" would be literally just that, no implication of linearity of anything. Thanks, pq pgpFr5nNsdCX0.pgp Description: OpenPGP digital signature
Re: [PATCH 14/16] drm/i915/vm_bind: Handle persistent vmas in execbuf3
On 02/10/2022 07:28, Niranjana Vishwanathapura wrote:
On Fri, Sep 30, 2022 at 10:47:48AM +0100, Matthew Auld wrote:
On 28/09/2022 07:19, Niranjana Vishwanathapura wrote:
Handle persistent (VM_BIND) mappings during the request submission
in the execbuf3 path.
Signed-off-by: Niranjana Vishwanathapura
Signed-off-by: Andi Shyti
---
.../gpu/drm/i915/gem/i915_gem_execbuffer3.c | 188 +-
1 file changed, 187 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer3.c
b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer3.c
index 92af88bc8deb..1aeeff5e8540 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer3.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer3.c
@@ -19,6 +19,7 @@
#include "i915_gem_vm_bind.h"
#include "i915_trace.h"
+#define __EXEC3_HAS_PIN BIT_ULL(33)
#define __EXEC3_ENGINE_PINNED BIT_ULL(32)
#define __EXEC3_INTERNAL_FLAGS (~0ull << 32)
@@ -42,7 +43,9 @@
* execlist. Hence, no support for implicit sync.
*
* The new execbuf3 ioctl only works in VM_BIND mode and the VM_BIND
mode only
- * works with execbuf3 ioctl for submission.
+ * works with execbuf3 ioctl for submission. All BOs mapped on that
VM (through
+ * VM_BIND call) at the time of execbuf3 call are deemed required
for that
+ * submission.
*
* The execbuf3 ioctl directly specifies the batch addresses instead
of as
* object handles as in execbuf2 ioctl. The execbuf3 ioctl will also
not
@@ -58,6 +61,13 @@
* So, a lot of code supporting execbuf2 ioctl, like relocations, VA
evictions,
* vma lookup table, implicit sync, vma active reference tracking
etc., are not
* applicable for execbuf3 ioctl.
+ *
+ * During each execbuf submission, request fence is added to all
VM_BIND mapped
+ * objects with DMA_RESV_USAGE_BOOKKEEP. The DMA_RESV_USAGE_BOOKKEEP
usage will
+ * prevent over sync (See enum dma_resv_usage). Note that
DRM_I915_GEM_WAIT and
+ * DRM_I915_GEM_BUSY ioctls do not check for DMA_RESV_USAGE_BOOKKEEP
usage and
+ * hence should not be used for end of batch check. Instead, the
execbuf3
+ * timeline out fence should be used for end of batch check.
*/
/**
@@ -127,6 +137,23 @@ eb_find_vma(struct i915_address_space *vm, u64
addr)
return i915_gem_vm_bind_lookup_vma(vm, va);
}
+static void eb_scoop_unbound_vma_all(struct i915_address_space *vm)
+{
+ struct i915_vma *vma, *vn;
+
+ /**
+ * Move all unbound vmas back into vm_bind_list so that they are
+ * revalidated.
+ */
+ spin_lock(>vm_rebind_lock);
+ list_for_each_entry_safe(vma, vn, >vm_rebind_list,
vm_rebind_link) {
+ list_del_init(>vm_rebind_link);
+ if (!list_empty(>vm_bind_link))
+ list_move_tail(>vm_bind_link, >vm_bind_list);
+ }
+ spin_unlock(>vm_rebind_lock);
+}
+
static int eb_lookup_vma_all(struct i915_execbuffer *eb)
{
unsigned int i, current_batch = 0;
@@ -141,14 +168,121 @@ static int eb_lookup_vma_all(struct
i915_execbuffer *eb)
++current_batch;
}
+ eb_scoop_unbound_vma_all(eb->context->vm);
+
+ return 0;
+}
+
+static int eb_lock_vma_all(struct i915_execbuffer *eb)
+{
+ struct i915_address_space *vm = eb->context->vm;
+ struct i915_vma *vma;
+ int err;
+
+ err = i915_gem_object_lock(eb->context->vm->root_obj, >ww);
+ if (err)
+ return err;
+
+ list_for_each_entry(vma, >non_priv_vm_bind_list,
+ non_priv_vm_bind_link) {
+ err = i915_gem_object_lock(vma->obj, >ww);
+ if (err)
+ return err;
+ }
+
return 0;
}
+static void eb_release_persistent_vma_all(struct i915_execbuffer *eb,
+ bool final)
+{
+ struct i915_address_space *vm = eb->context->vm;
+ struct i915_vma *vma, *vn;
+
+ lockdep_assert_held(>vm_bind_lock);
+
+ if (!(eb->args->flags & __EXEC3_HAS_PIN))
+ return;
+
+ assert_object_held(vm->root_obj);
+
+ list_for_each_entry(vma, >vm_bind_list, vm_bind_link)
+ __i915_vma_unpin(vma);
+
+ eb->args->flags &= ~__EXEC3_HAS_PIN;
+ if (!final)
+ return;
+
+ list_for_each_entry_safe(vma, vn, >vm_bind_list, vm_bind_link)
+ if (i915_vma_verify_bind_complete(vma))
+ list_move_tail(>vm_bind_link, >vm_bound_list);
+}
+
static void eb_release_vma_all(struct i915_execbuffer *eb, bool final)
{
+ eb_release_persistent_vma_all(eb, final);
eb_unpin_engine(eb);
}
+static int eb_reserve_fence_for_persistent_vma_all(struct
i915_execbuffer *eb)
+{
+ struct i915_address_space *vm = eb->context->vm;
+ struct i915_vma *vma;
+ int ret;
+
+ ret = dma_resv_reserve_fences(vm->root_obj->base.resv, 1);
+ if (ret)
+ return ret;
+
+ list_for_each_entry(vma, >non_priv_vm_bind_list,
+ non_priv_vm_bind_link) {
+ ret = dma_resv_reserve_fences(vma->obj->base.resv, 1);
+ if (ret)
+ return ret;
+ }
+
+ return 0;
+}
+
+static int eb_validate_persistent_vma_all(struct
Re: [PATCH v3 2/2] drm/tests: Split drm_test_dp_mst_sideband_msg_req_decode into parameterized tests
On Sat, Oct 01, 2022 at 07:34:22PM -0300, Maíra Canal wrote: > The drm_test_dp_mst_sideband_msg_req_decode repeats the same test > structure with different parameters. This could be better represented > by parameterized tests, provided by KUnit. > > In addition to the parameterization of the tests, the test case for the > client ID was changed: instead of using get_random_bytes to generate > the client ID, the client ID is now hardcoded in the test case. This > doesn't affect the assertively of the tests, as this test case only compare > the data going in with the data going out and it doesn't transform the data > itself in any way. > > So, convert drm_test_dp_mst_sideband_msg_req_decode into parameterized > tests and make the tests' allocations and prints completely managed by KUnit. > > Signed-off-by: Maíra Canal Reviewed-by: Michał Winiarski Thanks! -Michał > --- > v1 -> v2: > https://lore.kernel.org/dri-devel/20220925222719.345424-1-mca...@igalia.com/T/#m056610a23a63109484afeafefb5846178c4d36b2 > - Mention on the commit message the change on the test case for the client ID > (Michał Winiarski). > > v2 -> v3: > https://lore.kernel.org/dri-devel/20220927221206.55930-1-mca...@igalia.com/T/#m2dc961da2d4921566cd0f9a8ed9d2d33a1cf4416 > - Mention on the commit message that the "random" usage is not incompatible > with parameterized tests (Michał Winiarski). > --- > .../gpu/drm/tests/drm_dp_mst_helper_test.c| 370 -- > 1 file changed, 243 insertions(+), 127 deletions(-)
[PATCH] drm/ttm: fix bo->resource check in vm_access
Touching bo->resource looks like it should require first locking the object, since this state is dynamic and could potentially change from under us. It looks we can just use obj->base.size here, which avoids any issues with locking, since this is immutable state. Signed-off-by: Matthew Auld Cc: Christian König Reviewed-by: Christian König --- drivers/gpu/drm/ttm/ttm_bo_vm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/ttm/ttm_bo_vm.c b/drivers/gpu/drm/ttm/ttm_bo_vm.c index 38119311284d..e0f73f0ac086 100644 --- a/drivers/gpu/drm/ttm/ttm_bo_vm.c +++ b/drivers/gpu/drm/ttm/ttm_bo_vm.c @@ -412,7 +412,7 @@ int ttm_bo_vm_access(struct vm_area_struct *vma, unsigned long addr, << PAGE_SHIFT); int ret; - if (len < 1 || (offset + len) >> PAGE_SHIFT > bo->resource->num_pages) + if (len < 1 || (offset + len) > bo->base.size) return -EIO; ret = ttm_bo_reserve(bo, true, false, NULL); -- 2.37.3
Re: [Intel-gfx] [PATCH] drm/i915/guc: Fix revocation of non-persistent contexts
On 30/09/2022 15:52, Andrzej Hajda wrote:
On 30.09.2022 11:47, Tvrtko Ursulin wrote:
From: Tvrtko Ursulin
Patch which added graceful exit for non-persistent contexts missed the
fact it is not enough to set the exiting flag on a context and let the
backend handle it from there.
GuC backend cannot handle it because it runs independently in the
firmware and driver might not see the requests ever again. Patch also
missed the fact some usages of intel_context_is_banned in the GuC backend
needed replacing with newly introduced intel_context_is_schedulable.
Fix the first issue by calling into backend revoke when we know this is
the last chance to do it. Fix the second issue by replacing
intel_context_is_banned with intel_context_is_schedulable, which should
always be safe since latter is a superset of the former.
negation of the latter is a ...?
I did not get what you meant here.
Signed-off-by: Tvrtko Ursulin
Fixes: 45c64ecf97ee ("drm/i915: Improve user experience and driver
robustness under SIGINT or similar")
Cc: Andrzej Hajda
Cc: John Harrison
Cc: Daniele Ceraolo Spurio
---
drivers/gpu/drm/i915/gem/i915_gem_context.c | 8 +-
drivers/gpu/drm/i915/gt/intel_context.c | 14 +++---
drivers/gpu/drm/i915/gt/intel_context.h | 8 +-
.../gpu/drm/i915/gt/uc/intel_guc_submission.c | 26 +--
4 files changed, 25 insertions(+), 31 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c
b/drivers/gpu/drm/i915/gem/i915_gem_context.c
index 0bcde53c50c6..1e29b1e6d186 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
@@ -1387,14 +1387,8 @@ kill_engines(struct i915_gem_engines *engines,
bool exit, bool persistent)
*/
for_each_gem_engine(ce, engines, it) {
struct intel_engine_cs *engine;
- bool skip = false;
- if (exit)
- skip = intel_context_set_exiting(ce);
- else if (!persistent)
- skip = intel_context_exit_nonpersistent(ce, NULL); > -
- if (skip)
+ if ((exit || !persistent) && intel_context_revoke(ce))
continue; /* Already marked. */
/*
diff --git a/drivers/gpu/drm/i915/gt/intel_context.c
b/drivers/gpu/drm/i915/gt/intel_context.c
index 654a092ed3d6..398b2a9eed61 100644
--- a/drivers/gpu/drm/i915/gt/intel_context.c
+++ b/drivers/gpu/drm/i915/gt/intel_context.c
@@ -614,13 +614,19 @@ bool intel_context_ban(struct intel_context *ce,
struct i915_request *rq)
return ret;
}
-bool intel_context_exit_nonpersistent(struct intel_context *ce,
- struct i915_request *rq)
+bool intel_context_revoke(struct intel_context *ce)
{
bool ret = intel_context_set_exiting(ce);
- if (ce->ops->revoke)
- ce->ops->revoke(ce, rq, ce->engine->props.preempt_timeout_ms);
+ if (!ret && intel_engine_uses_guc(ce->engine)) {
+ /*
+ * With GuC backend we have to notify it of revocation as soon
+ * as the exiting flag is set.
+ */
+ if (ce->ops->revoke)
+ ce->ops->revoke(ce, NULL,
+ ce->engine->props.preempt_timeout_ms);
+ }
Now revoke is called only with GuC, previously it was called also for
other backends in case non-exiting/non-persistent, is it OK?
It is okay (execlists has no revoke vfunc, ringbuffer has it but only
works if target request is known), but agreed it is a bit ugly. I was in
two minds which way to go. Perhaps it would indeed be cleaner to go
unconditional. I will resend with that change, copying stable this time
round (since 6.0 is out), and can keep your r-b?
Regards,
Tvrtko
return ret;
}
diff --git a/drivers/gpu/drm/i915/gt/intel_context.h
b/drivers/gpu/drm/i915/gt/intel_context.h
index 8e2d70630c49..40f8809d14ea 100644
--- a/drivers/gpu/drm/i915/gt/intel_context.h
+++ b/drivers/gpu/drm/i915/gt/intel_context.h
@@ -319,18 +319,12 @@ static inline bool
intel_context_is_schedulable(const struct intel_context *ce)
!test_bit(CONTEXT_BANNED, >flags);
}
-static inline bool intel_context_is_exiting(const struct
intel_context *ce)
-{
- return test_bit(CONTEXT_EXITING, >flags);
-}
-
static inline bool intel_context_set_exiting(struct intel_context *ce)
{
return test_and_set_bit(CONTEXT_EXITING, >flags);
}
-bool intel_context_exit_nonpersistent(struct intel_context *ce,
- struct i915_request *rq);
+bool intel_context_revoke(struct intel_context *ce);
static inline bool
intel_context_force_single_submission(const struct intel_context *ce)
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
index 0ef295a94060..88a4476b8e92 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
@@ -685,7 +685,7 @@ static int __guc_add_request(struct intel_guc
*guc, struct i915_request *rq)
Re: [PATCH v7 05/12] dt-bindings: display/msm: move common MDSS properties to mdss-common.yaml
On 23/09/2022 22:32, Dmitry Baryshkov wrote: > On 22/09/2022 15:28, Krzysztof Kozlowski wrote: >> On 22/09/2022 13:46, Dmitry Baryshkov wrote: > - ranges: true > +maxItems: 2 > > interconnects: > -items: > - - description: Interconnect path from mdp0 port to the data bus > - - description: Interconnect path from mdp1 port to the data bus > +maxItems: 2 I think this is not equivalent now, because you have in total minItems:1 and maxItems:2, while in past minItems was 2. >>> >>> This means that I should have minItems:2, maxItems:2, which, if I got it >>> right, is frowned upon. Let me doublecheck though if it works as expected. >> >> It is frowned upon only if it is alone, because for missing minItems, >> maxItems implies minItems. Here you have minItems in other schema, so >> there is no such case > > Well, I just checked, the schema will throw an error if I put a single > interconnects or iommus entry. If I understand correctly these two > clauses are evaluated separately. So, the dpu-common's clause tells > minItems:1, maxItems:2. The platform schema file contains just > maxItems:2, which implicitly adds minItems:2 to _this_ clause. > > Thus I think I'll leave this part as is. Thanks for checking. It's good then. Best regards, Krzysztof
Re: [Intel-gfx] [PATCH v4 3/4] drm/i915: Make the heartbeat play nice with long pre-emption timeouts
On 30/09/2022 18:44, John Harrison wrote:
On 9/30/2022 02:22, Tvrtko Ursulin wrote:
On 29/09/2022 17:21, John Harrison wrote:
On 9/29/2022 00:42, Tvrtko Ursulin wrote:
On 29/09/2022 03:18, john.c.harri...@intel.com wrote:
From: John Harrison
Compute workloads are inherently not pre-emptible for long periods on
current hardware. As a workaround for this, the pre-emption timeout
for compute capable engines was disabled. This is undesirable with GuC
submission as it prevents per engine reset of hung contexts. Hence the
next patch will re-enable the timeout but bumped up by an order of
magnitude.
However, the heartbeat might not respect that. Depending upon current
activity, a pre-emption to the heartbeat pulse might not even be
attempted until the last heartbeat period. Which means that only one
period is granted for the pre-emption to occur. With the aforesaid
bump, the pre-emption timeout could be significantly larger than this
heartbeat period.
So adjust the heartbeat code to take the pre-emption timeout into
account. When it reaches the final (high priority) period, it now
ensures the delay before hitting reset is bigger than the pre-emption
timeout.
v2: Fix for selftests which adjust the heartbeat period manually.
Signed-off-by: John Harrison
---
.../gpu/drm/i915/gt/intel_engine_heartbeat.c | 19
+++
1 file changed, 19 insertions(+)
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c
b/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c
index a3698f611f457..823a790a0e2ae 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c
+++ b/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c
@@ -22,9 +22,28 @@
static bool next_heartbeat(struct intel_engine_cs *engine)
{
+ struct i915_request *rq;
long delay;
delay = READ_ONCE(engine->props.heartbeat_interval_ms);
+
+ rq = engine->heartbeat.systole;
+
+ if (rq && rq->sched.attr.priority >= I915_PRIORITY_BARRIER &&
+ delay == engine->defaults.heartbeat_interval_ms) {
Maybe I forgot but what is the reason for the check against the
default heartbeat interval?
That's the 'v2: fix for selftests that manually adjust the
heartbeat'. If something (or someone) has explicitly set an override
of the heartbeat then it has to be assumed that they know what they
are doing, and if things don't work any more that's their problem.
But if we don't respect their override then they won't get the
timings they expect and the selftest will fail.
Isn't this a bit too strict for the non-selftest case? If the new
concept is extending the last pulse to guarantee preemption, then I
think we could allow tweaking of the heartbeat period. Like what if
user wants 1s, or 10s instead of 2.5s - why would that need to break
the improvement from this patch?
Then the user is back to where they were before this patch.
In what ways selftests fail? Are they trying to guess time to reset
based on the hearbeat period set? If so perhaps add a helper to query
it based on the last pulse extension.
I don't recall. It was six months ago when I was actually working on
this. And right now I do not have the time to go back and re-run all the
testing and re-write a bunch of self tests with whole new helpers and
algorithms and whatever else might be necessary to polish this to
perfection. And in the meantime, all the existing issues are still
present - there is no range checking on any of this stuff, it is very
possible for a driver with default settings to break a legal workload
because the heartbeat and pre-emption are fighting with each other, we
don't even have per engine resets enabled, etc.
Maybe it could be even better with a follow up patch. Feel free to do
that. But as it stands, this patch set significantly improves the
situation without making anything worse.
As we seem to be in agreement that the check against default heartbeat
is a hack with only purpose to work around assumptions made by
selftests, then please file a Jira about removing it (this hack). Then
work can be assigned to someone to clean it up. With that done I would
agree the series is indeed an improvement and it would have my ack.
Regards,
Tvrtko
Re: [PATCH v5 06/11] drm: bridge: samsung-dsim: Add atomic_check
Hi Jagan,
On 16.09.2022 20:17, Jagan Teki wrote:
> Look like an explicit fixing up of mode_flags is required for DSIM IP
> present in i.MX8M Mini/Nano SoCs.
>
> At least the LCDIF + DSIM needs active low sync polarities in order
> to correlate the correct sync flags of the surrounding components in
> the chain to make sure the whole pipeline can work properly.
>
> On the other hand the i.MX 8M Mini Applications Processor Reference Manual,
> Rev. 3, 11/2020 says.
> "13.6.3.5.2 RGB interface
> Vsync, Hsync, and VDEN are active high signals."
>
> No clear evidence about whether it can be documentation issues or
> something, so added a comment FIXME for this and updated the active low
> sync polarities using SAMSUNG_DSIM_TYPE_IMX8MM hw_type.
>
> v5:
> * rebase based new bridge changes [mszyprow]
> * remove DSIM_QUIRK_FIXUP_SYNC_POL
> * add hw_type check for sync polarities change.
>
> v4:
> * none
>
> v3:
> * add DSIM_QUIRK_FIXUP_SYNC_POL to handle mode_flasg fixup
>
> v2:
> * none
>
> v1:
> * fix mode flags in atomic_check instead of mode_fixup
>
> Signed-off-by: Jagan Teki
> ---
> drivers/gpu/drm/bridge/samsung-dsim.c | 26 ++
> include/drm/bridge/samsung-dsim.h | 1 +
> 2 files changed, 27 insertions(+)
>
> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c
> b/drivers/gpu/drm/bridge/samsung-dsim.c
> index 6d524338d4ff..8abf89326424 100644
> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> @@ -1315,6 +1315,31 @@ static void samsung_dsim_atomic_post_disable(struct
> drm_bridge *bridge,
> pm_runtime_put_sync(dsi->dev);
> }
>
> +static int samsung_dsim_atomic_check(struct drm_bridge *bridge,
> + struct drm_bridge_state *bridge_state,
> + struct drm_crtc_state *crtc_state,
> + struct drm_connector_state *conn_state)
> +{
> + struct samsung_dsim *dsi = bridge_to_dsi(bridge);
> + struct drm_display_mode *adjusted_mode = _state->adjusted_mode;
> +
> + if (dsi->plat_data->hw_type & SAMSUNG_DSIM_TYPE_IMX8MM) {
Again the above should be 'if (dsi->plat_data->hw_type ==
SAMSUNG_DSIM_TYPE_IMX8MM)', hw_type is not a bitmask.
> + /**
> + * FIXME:
> + * At least LCDIF + DSIM needs active low sync,
> + * but i.MX 8M Mini Applications Processor Reference Manual,
> + * Rev. 3, 11/2020 says
> + *
> + * 13.6.3.5.2 RGB interface
> + * Vsync, Hsync, and VDEN are active high signals.
> + */
> + adjusted_mode->flags |= (DRM_MODE_FLAG_NHSYNC |
> DRM_MODE_FLAG_NVSYNC);
> + adjusted_mode->flags &= ~(DRM_MODE_FLAG_PHSYNC |
> DRM_MODE_FLAG_PVSYNC);
> + }
> +
> + return 0;
> +}
> +
> static void samsung_dsim_mode_set(struct drm_bridge *bridge,
> const struct drm_display_mode *mode,
> const struct drm_display_mode *adjusted_mode)
> @@ -1337,6 +1362,7 @@ static const struct drm_bridge_funcs
> samsung_dsim_bridge_funcs = {
> .atomic_duplicate_state =
> drm_atomic_helper_bridge_duplicate_state,
> .atomic_destroy_state =
> drm_atomic_helper_bridge_destroy_state,
> .atomic_reset = drm_atomic_helper_bridge_reset,
> + .atomic_check = samsung_dsim_atomic_check,
> .atomic_pre_enable = samsung_dsim_atomic_pre_enable,
> .atomic_enable = samsung_dsim_atomic_enable,
> .atomic_disable = samsung_dsim_atomic_disable,
> diff --git a/include/drm/bridge/samsung-dsim.h
> b/include/drm/bridge/samsung-dsim.h
> index 57b27d75369e..0c5a905f3de7 100644
> --- a/include/drm/bridge/samsung-dsim.h
> +++ b/include/drm/bridge/samsung-dsim.h
> @@ -27,6 +27,7 @@ enum samsung_dsim_type {
> SAMSUNG_DSIM_TYPE_EXYNOS5410,
> SAMSUNG_DSIM_TYPE_EXYNOS5422,
> SAMSUNG_DSIM_TYPE_EXYNOS5433,
> + SAMSUNG_DSIM_TYPE_IMX8MM,
> SAMSUNG_DSIM_TYPE_COUNT,
> };
>
Best regards
--
Marek Szyprowski, PhD
Samsung R Institute Poland
Re: [PATCH v6 01/10] drm: bridge: Add Samsung DSIM bridge driver
Hi Jagan,
On 01.10.2022 10:06, Jagan Teki wrote:
> Samsung MIPI DSIM controller is common DSI IP that can be used in various
> SoCs like Exynos, i.MX8M Mini/Nano.
>
> In order to access this DSI controller between various platform SoCs,
> the ideal way to incorporate this in the drm stack is via the drm bridge
> driver.
>
> This patch is trying to differentiate platform-specific and bridge driver
> code by maintaining exynos platform glue code in exynos_drm_dsi.c driver
> and common bridge driver code in samsung-dsim.c providing that the new
> platform-specific glue should be supported in the bridge driver, unlike
> exynos platform drm drivers.
>
> - Add samsung_dsim_plat_data for keeping platform-specific attributes like
>host_ops, irq_ops, and hw_type.
>
> - Initialize the plat_data hooks for exynos platform in exynos_drm_dsi.c.
>
> - samsung_dsim_probe is the common probe call across exynos_drm_dsi.c and
>samsung-dsim.c.
>
> - plat_data hooks like host_ops and irq_ops are invoked during the
>respective bridge call chains.
>
> v6:
> * handle previous bridge pointer for exynos dsi
There are still issues, see my comments below.
> v5:
> * [mszyprow] reworked driver initialization using the same approach as in
>drivers/gpu/drm/{exynos/exynos_dp.c, bridge/analogix/analogix_dp_core.c},
>removed weak functions, moved exynos_dsi_driver back to exynos_drm_dsi.c
>and restored integration with exynos_drm custom initialization.
> * updated the commit message [Jagan]
>
> v4:
> * include Inki Dae in MAINTAINERS
> * remove dsi_driver probe in exynos_drm_drv to support multi-arch build
>
> v3:
> * restore gpio related fixes
> * restore proper bridge chain
> * rework initialization issue
> * fix header includes in proper way
>
> v2:
> * fixed exynos dsi driver conversion (Marek Szyprowski)
> * updated commit message
> * updated MAINTAINERS file
>
> v1:
> * don't maintain component_ops in bridge driver
> * don't maintain platform glue code in bridge driver
> * add platform-specific glue code and make a common bridge
>
> Signed-off-by: Marek Szyprowski
> Signed-off-by: Jagan Teki
> ---
> MAINTAINERS |9 +
> drivers/gpu/drm/bridge/Kconfig | 12 +
> drivers/gpu/drm/bridge/Makefile |1 +
> drivers/gpu/drm/bridge/samsung-dsim.c | 1703 ++
> drivers/gpu/drm/exynos/Kconfig |1 +
> drivers/gpu/drm/exynos/exynos_drm_dsi.c | 1769 ++-
> include/drm/bridge/samsung-dsim.h | 113 ++
> 7 files changed, 1955 insertions(+), 1653 deletions(-)
> create mode 100644 drivers/gpu/drm/bridge/samsung-dsim.c
> create mode 100644 include/drm/bridge/samsung-dsim.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 9ae989b32ebb..ba7a6721443c 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -6624,6 +6624,15 @@ T: git git://anongit.freedesktop.org/drm/drm-misc
> F: Documentation/devicetree/bindings/display/panel/samsung,lms397kf04.yaml
> F: drivers/gpu/drm/panel/panel-samsung-db7430.c
>
> +DRM DRIVER FOR SAMSUNG MIPI DSIM BRIDGE
> +M: Jagan Teki
> +M: Marek Szyprowski
> +M: Inki Dae +S: Maintained
> +T: git git://anongit.freedesktop.org/drm/drm-misc
> +F: drivers/gpu/drm/bridge/samsung-dsim.c
> +F: include/drm/bridge/samsung-dsim.h
> +
> DRM DRIVER FOR SAMSUNG S6D27A1 PANELS
> M: Markuss Broks
> S: Maintained
> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> index 57946d80b02d..8e85dac9f53e 100644
> --- a/drivers/gpu/drm/bridge/Kconfig
> +++ b/drivers/gpu/drm/bridge/Kconfig
> @@ -231,6 +231,18 @@ config DRM_PARADE_PS8640
> The PS8640 is a high-performance and low-power
> MIPI DSI to eDP converter
>
> +config DRM_SAMSUNG_DSIM
> + tristate "Samsung MIPI DSIM bridge driver"
> + depends on COMMON_CLK
> + depends on OF && HAS_IOMEM
> + select DRM_KMS_HELPER
> + select DRM_MIPI_DSI
> + select DRM_PANEL_BRIDGE
> + help
> + The Samsung MIPI DSIM bridge controller driver.
> + This MIPI DSIM bridge can be found it on Exynos SoCs and
> + NXP's i.MX8M Mini/Nano.
> +
> config DRM_SIL_SII8620
> tristate "Silicon Image SII8620 HDMI/MHL bridge"
> depends on OF
> diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
> index 1884803c6860..dae843723991 100644
> --- a/drivers/gpu/drm/bridge/Makefile
> +++ b/drivers/gpu/drm/bridge/Makefile
> @@ -15,6 +15,7 @@ obj-$(CONFIG_DRM_MEGACHIPS_STDP_GE_B850V3_FW) +=
> megachips-stdp-ge-b850v
> obj-$(CONFIG_DRM_NXP_PTN3460) += nxp-ptn3460.o
> obj-$(CONFIG_DRM_PARADE_PS8622) += parade-ps8622.o
> obj-$(CONFIG_DRM_PARADE_PS8640) += parade-ps8640.o
> +obj-$(CONFIG_DRM_SAMSUNG_DSIM) += samsung-dsim.o
> obj-$(CONFIG_DRM_SIL_SII8620) += sil-sii8620.o
> obj-$(CONFIG_DRM_SII902X) += sii902x.o
> obj-$(CONFIG_DRM_SII9234) += sii9234.o
> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c
>
Re: [PATCH] drm: rcar-du: Fix Kconfig dependency between RCAR_DU and RCAR_MIPI_DSI
On 03/10/2022 10:01, Geert Uytterhoeven wrote:
Hi Tomi,
On Mon, Oct 3, 2022 at 8:56 AM Tomi Valkeinen
wrote:
On 02/10/2022 01:03, Laurent Pinchart wrote:
When the R-Car MIPI DSI driver was added, it was a standalone encoder
driver without any dependency to or from the R-Car DU driver. Commit
957fe62d7d15 ("drm: rcar-du: Fix DSI enable & disable sequence") then
added a direct call from the DU driver to the MIPI DSI driver, without
updating Kconfig to take the new dependency into account. Fix it the
same way that the LVDS encoder is handled.
Fixes: 957fe62d7d15 ("drm: rcar-du: Fix DSI enable & disable sequence")
Reported-by: kernel test robot
Signed-off-by: Laurent Pinchart
---
drivers/gpu/drm/rcar-du/Kconfig | 13 +
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/rcar-du/Kconfig b/drivers/gpu/drm/rcar-du/Kconfig
index c959e8c6be7d..fd2c2eaee26b 100644
--- a/drivers/gpu/drm/rcar-du/Kconfig
+++ b/drivers/gpu/drm/rcar-du/Kconfig
@@ -44,12 +44,17 @@ config DRM_RCAR_LVDS
select OF_FLATTREE
select OF_OVERLAY
+config DRM_RCAR_USE_MIPI_DSI
+ bool "R-Car DU MIPI DSI Encoder Support"
+ depends on DRM_BRIDGE && OF
+ default DRM_RCAR_DU
+ help
+ Enable support for the R-Car Display Unit embedded MIPI DSI encoders.
+
config DRM_RCAR_MIPI_DSI
- tristate "R-Car DU MIPI DSI Encoder Support"
- depends on DRM && DRM_BRIDGE && OF
+ def_tristate DRM_RCAR_DU
+ depends on DRM_RCAR_USE_MIPI_DSI
select DRM_MIPI_DSI
- help
- Enable support for the R-Car Display Unit embedded MIPI DSI encoders.
config DRM_RCAR_VSP
bool "R-Car DU VSP Compositor Support" if ARM
base-commit: 7860d720a84c74b2761c6b7995392a798ab0a3cb
Interesting dependency issue. Took me a while to understand it =).
But is there a reason to not have "depends on DRM_RCAR_DU" for
DRM_RCAR_USE_MIPI_DSI and DRM_RCAR_USE_LVDS? Now the menu items are
available even if RCAR_DU is n. That's also the case for
DRM_RCAR_DW_HDMI, but I'm not sure if that's supposed to be usable even
without RCAR_DU.
See
https://lore.kernel.org/linux-renesas-soc/cover.1639559338.git.geert+rene...@glider.be/
Didn't get to implement the suggested improvements yet...
Ok, looks good.
But I started to wonder, for LVDS and DSI (maybe for HDMI too), what's
the point of modules here...
If RCAR_DU, LVDS and DSI are compiled as modules, you can load either
LVDS or DSI module, but those won't do anything alone. So in practice
you always need to load RCAR_DU module too. But if LVDS or DSI are
enabled in the kconfig, you _have_ to load those before RCAR_DU.
In other words, you can never, e.g. load RCAR_DU and LVDS, but not DSI,
if all three are enabled.
So why not just compile LVDS and DSI into the RCAR_DU module,
simplifying the dependencies, removing this issue altogether?
Tomi
Re: Nested AVIC design (was:Re: [RFC PATCH v3 04/19] KVM: x86: mmu: allow to enable write tracking externally)
On Thu, 2022-09-29 at 22:38 +, Sean Christopherson wrote: > On Mon, Aug 08, 2022, Maxim Levitsky wrote: > > Hi Sean, Paolo, and everyone else who wants to review my nested AVIC work. > > Before we dive deep into design details, I think we should first decide > whether > or not nested AVIC is worth pursing/supporting. > > - Rome has a ucode/silicon bug with no known workaround and no anticipated > fix[*]; > AMD's recommended "workaround" is to disable AVIC. > - AVIC is not available in Milan, which may or may not be related to the > aforementioned bug. > - AVIC is making a comeback on Zen4, but Zen4 comes with x2AVIC. > - x2APIC is likely going to become ubiquitous, e.g. Intel is effectively > requiring x2APIC to fudge around xAPIC bugs. > - It's actually quite realistic to effectively force the guest to use > x2APIC, > at least if it's a Linux guest. E.g. turn x2APIC on in BIOS, which is > often > (always?) controlled by the host, and Linux will use x2APIC. > > In other words, given that AVIC is well on its way to becoming a "legacy" > feature, > IMO there needs to be a fairly strong use case to justify taking on this much > code > and complexity. ~1500 lines of code to support a feature that has > historically > been buggy _without_ nested support is going to require a non-trivial amount > of > effort to review, stabilize, and maintain. > > [*] 1235 "Guest With AVIC (Advanced Virtual Interrupt Controller) Enabled May > Fail > to Process IPI (Inter-Processor Interrupt) Until Guest Is Re-Scheduled" in > https://www.amd.com/system/files/TechDocs/56323-PUB_1.00.pdf > I am afraid that you mixed things up: You mistake is that x2avic is just a minor addition to AVIC. It is still for all practical purposes the same feature. 1. The AVIC is indeed kind of broken on Zen2 (but AFAIK for all practical purposes, including nested it works fine, the errata only shows up in a unit test and/or under very specific workloads (most of the time a delayed wakeup doesn't cause a hang). Yet, I agree that for production Zen2 should not have AVIC enabled. 2. Zen3 does indeed have AVIC soft disabled in CPUID. AFAIK it works just fine, but I understand that customers won't use it against AMD's guidance. 3. On Zen4, AVIC is fully enabled and also extended to support x2apic mode. The fact that AVIC was extended to support X2apic mode also shows that AMD is committed to supporting it. My nested AVIC code technically doesn't expose x2avic to the guest, but it is pretty much trivial to add (I am only waiting to get my hands on Zen4 machine to do it), and also even in its current form it would work just fine if the host uses normal AVIC . (or even doesn't use AVIC at all - the nested AVIC code works just fine even if the host has its AVIC inhibited for some reason). Adding nested x2avic support is literally about not passing through that MMIO address, Enabling the x2avic bit in int_ctl, and opening up the access to x2apic msrs. Plus I need to do some minor changes in unaccelerated IPI handler, dealing With read-only logical ID and such. Physid tables, apic backing pages, doorbell emulation, everything is pretty much unchanged. So AVIC is nothing but a legacy feature, and my nested AVIC code will support both nested AVIC and nested X2AVIC. Best regards, Maxim Levitsky
Re: [PATCH] drm: rcar-du: Fix Kconfig dependency between RCAR_DU and RCAR_MIPI_DSI
Hi Tomi,
On Mon, Oct 3, 2022 at 8:56 AM Tomi Valkeinen
wrote:
> On 02/10/2022 01:03, Laurent Pinchart wrote:
> > When the R-Car MIPI DSI driver was added, it was a standalone encoder
> > driver without any dependency to or from the R-Car DU driver. Commit
> > 957fe62d7d15 ("drm: rcar-du: Fix DSI enable & disable sequence") then
> > added a direct call from the DU driver to the MIPI DSI driver, without
> > updating Kconfig to take the new dependency into account. Fix it the
> > same way that the LVDS encoder is handled.
> >
> > Fixes: 957fe62d7d15 ("drm: rcar-du: Fix DSI enable & disable sequence")
> > Reported-by: kernel test robot
> > Signed-off-by: Laurent Pinchart
> > ---
> > drivers/gpu/drm/rcar-du/Kconfig | 13 +
> > 1 file changed, 9 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/rcar-du/Kconfig
> > b/drivers/gpu/drm/rcar-du/Kconfig
> > index c959e8c6be7d..fd2c2eaee26b 100644
> > --- a/drivers/gpu/drm/rcar-du/Kconfig
> > +++ b/drivers/gpu/drm/rcar-du/Kconfig
> > @@ -44,12 +44,17 @@ config DRM_RCAR_LVDS
> > select OF_FLATTREE
> > select OF_OVERLAY
> >
> > +config DRM_RCAR_USE_MIPI_DSI
> > + bool "R-Car DU MIPI DSI Encoder Support"
> > + depends on DRM_BRIDGE && OF
> > + default DRM_RCAR_DU
> > + help
> > + Enable support for the R-Car Display Unit embedded MIPI DSI
> > encoders.
> > +
> > config DRM_RCAR_MIPI_DSI
> > - tristate "R-Car DU MIPI DSI Encoder Support"
> > - depends on DRM && DRM_BRIDGE && OF
> > + def_tristate DRM_RCAR_DU
> > + depends on DRM_RCAR_USE_MIPI_DSI
> > select DRM_MIPI_DSI
> > - help
> > - Enable support for the R-Car Display Unit embedded MIPI DSI
> > encoders.
> >
> > config DRM_RCAR_VSP
> > bool "R-Car DU VSP Compositor Support" if ARM
> >
> > base-commit: 7860d720a84c74b2761c6b7995392a798ab0a3cb
>
> Interesting dependency issue. Took me a while to understand it =).
>
> But is there a reason to not have "depends on DRM_RCAR_DU" for
> DRM_RCAR_USE_MIPI_DSI and DRM_RCAR_USE_LVDS? Now the menu items are
> available even if RCAR_DU is n. That's also the case for
> DRM_RCAR_DW_HDMI, but I'm not sure if that's supposed to be usable even
> without RCAR_DU.
See
https://lore.kernel.org/linux-renesas-soc/cover.1639559338.git.geert+rene...@glider.be/
Didn't get to implement the suggested improvements yet...
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [PATCH] drm: rcar-du: Fix Kconfig dependency between RCAR_DU and RCAR_MIPI_DSI
Hi,
On 02/10/2022 01:03, Laurent Pinchart wrote:
When the R-Car MIPI DSI driver was added, it was a standalone encoder
driver without any dependency to or from the R-Car DU driver. Commit
957fe62d7d15 ("drm: rcar-du: Fix DSI enable & disable sequence") then
added a direct call from the DU driver to the MIPI DSI driver, without
updating Kconfig to take the new dependency into account. Fix it the
same way that the LVDS encoder is handled.
Fixes: 957fe62d7d15 ("drm: rcar-du: Fix DSI enable & disable sequence")
Reported-by: kernel test robot
Signed-off-by: Laurent Pinchart
---
drivers/gpu/drm/rcar-du/Kconfig | 13 +
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/rcar-du/Kconfig b/drivers/gpu/drm/rcar-du/Kconfig
index c959e8c6be7d..fd2c2eaee26b 100644
--- a/drivers/gpu/drm/rcar-du/Kconfig
+++ b/drivers/gpu/drm/rcar-du/Kconfig
@@ -44,12 +44,17 @@ config DRM_RCAR_LVDS
select OF_FLATTREE
select OF_OVERLAY
+config DRM_RCAR_USE_MIPI_DSI
+ bool "R-Car DU MIPI DSI Encoder Support"
+ depends on DRM_BRIDGE && OF
+ default DRM_RCAR_DU
+ help
+ Enable support for the R-Car Display Unit embedded MIPI DSI encoders.
+
config DRM_RCAR_MIPI_DSI
- tristate "R-Car DU MIPI DSI Encoder Support"
- depends on DRM && DRM_BRIDGE && OF
+ def_tristate DRM_RCAR_DU
+ depends on DRM_RCAR_USE_MIPI_DSI
select DRM_MIPI_DSI
- help
- Enable support for the R-Car Display Unit embedded MIPI DSI encoders.
config DRM_RCAR_VSP
bool "R-Car DU VSP Compositor Support" if ARM
base-commit: 7860d720a84c74b2761c6b7995392a798ab0a3cb
Interesting dependency issue. Took me a while to understand it =).
But is there a reason to not have "depends on DRM_RCAR_DU" for
DRM_RCAR_USE_MIPI_DSI and DRM_RCAR_USE_LVDS? Now the menu items are
available even if RCAR_DU is n. That's also the case for
DRM_RCAR_DW_HDMI, but I'm not sure if that's supposed to be usable even
without RCAR_DU.
Reviewed-by: Tomi Valkeinen
Tomi
[PATCH v2 17/17] drm/i915/vm_bind: Add uapi for user to enable vm_bind_mode
Add getparam support for VM_BIND capability version.
Add VM creation time flag to enable vm_bind_mode for the VM.
v2: update kernel-doc
Acked-by: Matthew Auld
Signed-off-by: Niranjana Vishwanathapura
Signed-off-by: Andi Shyti
---
drivers/gpu/drm/i915/gem/i915_gem_context.c | 9 -
drivers/gpu/drm/i915/i915_drv.h | 2 ++
drivers/gpu/drm/i915/i915_getparam.c| 3 +++
include/uapi/drm/i915_drm.h | 22 -
4 files changed, 34 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c
b/drivers/gpu/drm/i915/gem/i915_gem_context.c
index f4e648ec01ed..c20bd6e8aaf8 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
@@ -1808,9 +1808,13 @@ int i915_gem_vm_create_ioctl(struct drm_device *dev,
void *data,
if (!HAS_FULL_PPGTT(i915))
return -ENODEV;
- if (args->flags)
+ if (args->flags & I915_VM_CREATE_FLAGS_UNKNOWN)
return -EINVAL;
+ if ((args->flags & I915_VM_CREATE_FLAGS_USE_VM_BIND) &&
+ !HAS_VM_BIND(i915))
+ return -EOPNOTSUPP;
+
ppgtt = i915_ppgtt_create(to_gt(i915), 0);
if (IS_ERR(ppgtt))
return PTR_ERR(ppgtt);
@@ -1828,6 +1832,9 @@ int i915_gem_vm_create_ioctl(struct drm_device *dev, void
*data,
if (err)
goto err_put;
+ if (args->flags & I915_VM_CREATE_FLAGS_USE_VM_BIND)
+ ppgtt->vm.vm_bind_mode = true;
+
GEM_BUG_ON(id == 0); /* reserved for invalid/unassigned ppgtt */
args->vm_id = id;
return 0;
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 90ed8e6db2fe..826b0f90879f 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -977,6 +977,8 @@ IS_SUBPLATFORM(const struct drm_i915_private *i915,
#define HAS_BAR2_SMEM_STOLEN(i915) (!HAS_LMEM(i915) && \
GRAPHICS_VER_FULL(i915) >= IP_VER(12, 70))
+#define HAS_VM_BIND(i915) (GRAPHICS_VER(i915) >= 12)
+
/* intel_device_info.c */
static inline struct intel_device_info *
mkwrite_device_info(struct drm_i915_private *dev_priv)
diff --git a/drivers/gpu/drm/i915/i915_getparam.c
b/drivers/gpu/drm/i915/i915_getparam.c
index 342c8ca6414e..f45b3c684bcf 100644
--- a/drivers/gpu/drm/i915/i915_getparam.c
+++ b/drivers/gpu/drm/i915/i915_getparam.c
@@ -175,6 +175,9 @@ int i915_getparam_ioctl(struct drm_device *dev, void *data,
case I915_PARAM_PERF_REVISION:
value = i915_perf_ioctl_version();
break;
+ case I915_PARAM_VM_BIND_VERSION:
+ value = HAS_VM_BIND(i915);
+ break;
default:
DRM_DEBUG("Unknown parameter %d\n", param->param);
return -EINVAL;
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index 6e0f6b8d66a3..4ed170b8bae7 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -755,6 +755,22 @@ typedef struct drm_i915_irq_wait {
/* Query if the kernel supports the I915_USERPTR_PROBE flag. */
#define I915_PARAM_HAS_USERPTR_PROBE 56
+/*
+ * VM_BIND feature version supported.
+ *
+ * The following versions of VM_BIND have been defined:
+ *
+ * 0: No VM_BIND support.
+ *
+ * 1: In VM_UNBIND calls, the UMD must specify the exact mappings created
+ *previously with VM_BIND, the ioctl will not support unbinding multiple
+ *mappings or splitting them. Similarly, VM_BIND calls will not replace
+ *any existing mappings.
+ *
+ * See struct drm_i915_gem_vm_bind and struct drm_i915_gem_vm_unbind.
+ */
+#define I915_PARAM_VM_BIND_VERSION 57
+
/* Must be kept compact -- no holes and well documented */
/**
@@ -2606,6 +2622,9 @@ struct drm_i915_gem_context_destroy {
* on the same file. Extensions can be provided to configure exactly how the
* address space is setup upon creation.
*
+ * If I915_VM_CREATE_FLAGS_USE_VM_BIND flag is set, VM created will work in
+ * VM_BIND mode.
+ *
* The id of new VM (bound to the fd) for use with I915_CONTEXT_PARAM_VM is
* returned in the outparam @id.
*
@@ -2622,7 +2641,8 @@ struct drm_i915_gem_vm_control {
/** @extensions: Zero-terminated chain of extensions. */
__u64 extensions;
- /** @flags: reserved for future usage, currently MBZ */
+#define I915_VM_CREATE_FLAGS_USE_VM_BIND (1u << 0)
+#define I915_VM_CREATE_FLAGS_UNKNOWN (-(I915_VM_CREATE_FLAGS_USE_VM_BIND <<
1))
__u32 flags;
/** @vm_id: Id of the VM created or to be destroyed */
--
2.21.0.rc0.32.g243a4c7e27
[PATCH v2 12/17] drm/i915/vm_bind: Implement I915_GEM_EXECBUFFER3 ioctl
Implement new execbuf3 ioctl (I915_GEM_EXECBUFFER3) which only
works in vm_bind mode. The vm_bind mode only works with
this new execbuf3 ioctl.
The new execbuf3 ioctl will not have any list of objects to validate
bind as all required objects binding would have been requested by the
userspace before submitting the execbuf3.
Legacy features like relocations etc are not supported by execbuf3.
v2: Add more input validity checks.
Signed-off-by: Niranjana Vishwanathapura
Signed-off-by: Andi Shyti
---
drivers/gpu/drm/i915/Makefile | 1 +
.../gpu/drm/i915/gem/i915_gem_execbuffer3.c | 575 ++
drivers/gpu/drm/i915/gem/i915_gem_ioctls.h| 2 +
drivers/gpu/drm/i915/i915_driver.c| 1 +
include/uapi/drm/i915_drm.h | 61 ++
5 files changed, 640 insertions(+)
create mode 100644 drivers/gpu/drm/i915/gem/i915_gem_execbuffer3.c
diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index bf952f478555..3473ee5825bb 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -150,6 +150,7 @@ gem-y += \
gem/i915_gem_domain.o \
gem/i915_gem_execbuffer_common.o \
gem/i915_gem_execbuffer.o \
+ gem/i915_gem_execbuffer3.o \
gem/i915_gem_internal.o \
gem/i915_gem_object.o \
gem/i915_gem_lmem.o \
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer3.c
b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer3.c
new file mode 100644
index ..66b723842e45
--- /dev/null
+++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer3.c
@@ -0,0 +1,575 @@
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright © 2022 Intel Corporation
+ */
+
+#include
+#include
+
+#include
+
+#include "gt/intel_context.h"
+#include "gt/intel_gpu_commands.h"
+#include "gt/intel_gt.h"
+
+#include "i915_drv.h"
+#include "i915_gem_context.h"
+#include "i915_gem_execbuffer_common.h"
+#include "i915_gem_ioctls.h"
+#include "i915_gem_vm_bind.h"
+#include "i915_trace.h"
+
+#define __EXEC3_ENGINE_PINNED BIT_ULL(32)
+#define __EXEC3_INTERNAL_FLAGS (~0ull << 32)
+
+/* Catch emission of unexpected errors for CI! */
+#if IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM)
+#undef EINVAL
+#define EINVAL ({ \
+ DRM_DEBUG_DRIVER("EINVAL at %s:%d\n", __func__, __LINE__); \
+ 22; \
+})
+#endif
+
+/**
+ * DOC: User command execution with execbuf3 ioctl
+ *
+ * A VM in VM_BIND mode will not support older execbuf mode of binding.
+ * The execbuf ioctl handling in VM_BIND mode differs significantly from the
+ * older execbuf2 ioctl (See struct drm_i915_gem_execbuffer2).
+ * Hence, a new execbuf3 ioctl has been added to support VM_BIND mode. (See
+ * struct drm_i915_gem_execbuffer3). The execbuf3 ioctl will not accept any
+ * execlist. Hence, no support for implicit sync.
+ *
+ * The new execbuf3 ioctl only works in VM_BIND mode and the VM_BIND mode only
+ * works with execbuf3 ioctl for submission.
+ *
+ * The execbuf3 ioctl directly specifies the batch addresses instead of as
+ * object handles as in execbuf2 ioctl. The execbuf3 ioctl will also not
+ * support many of the older features like in/out/submit fences, fence array,
+ * default gem context etc. (See struct drm_i915_gem_execbuffer3).
+ *
+ * In VM_BIND mode, VA allocation is completely managed by the user instead of
+ * the i915 driver. Hence all VA assignment, eviction are not applicable in
+ * VM_BIND mode. Also, for determining object activeness, VM_BIND mode will not
+ * be using the i915_vma active reference tracking. It will instead check the
+ * dma-resv object's fence list for that.
+ *
+ * So, a lot of code supporting execbuf2 ioctl, like relocations, VA evictions,
+ * vma lookup table, implicit sync, vma active reference tracking etc., are not
+ * applicable for execbuf3 ioctl.
+ */
+
+/**
+ * struct i915_execbuffer - execbuf struct for execbuf3
+ * @i915: reference to the i915 instance we run on
+ * @file: drm file reference
+ * args: execbuf3 ioctl structure
+ * @gt: reference to the gt instance ioctl submitted for
+ * @context: logical state for the request
+ * @gem_context: callers context
+ * @requests: requests to be build
+ * @composite_fence: used for excl fence in dma_resv objects when > 1 BB
submitted
+ * @ww: i915_gem_ww_ctx instance
+ * @num_batches: number of batches submitted
+ * @batch_addresses: addresses corresponds to the submitted batches
+ * @batches: references to the i915_vmas corresponding to the batches
+ */
+struct i915_execbuffer {
+ struct drm_i915_private *i915;
+ struct drm_file *file;
+ struct drm_i915_gem_execbuffer3 *args;
+
+ struct intel_gt *gt;
+ struct intel_context *context;
+ struct i915_gem_context *gem_context;
+
+ struct i915_request *requests[MAX_ENGINE_INSTANCE + 1];
+ struct dma_fence *composite_fence;
+
+ struct i915_gem_ww_ctx ww;
+
+ unsigned int num_batches;
+ u64 batch_addresses[MAX_ENGINE_INSTANCE + 1];
+
[PATCH v2 13/17] drm/i915/vm_bind: Update i915_vma_verify_bind_complete()
Ensure i915_vma_verify_bind_complete() handles case where bind
is not initiated. Also make it non static, add documentation
and move it out of CONFIG_DRM_I915_DEBUG_GEM.
Signed-off-by: Niranjana Vishwanathapura
Signed-off-by: Andi Shyti
---
drivers/gpu/drm/i915/i915_vma.c | 16 +++-
drivers/gpu/drm/i915/i915_vma.h | 1 +
2 files changed, 12 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
index e62d769d8a55..492a00d09cf3 100644
--- a/drivers/gpu/drm/i915/i915_vma.c
+++ b/drivers/gpu/drm/i915/i915_vma.c
@@ -439,12 +439,21 @@ int i915_vma_sync(struct i915_vma *vma)
return i915_vm_sync(vma->vm);
}
-#if IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM)
-static int i915_vma_verify_bind_complete(struct i915_vma *vma)
+/**
+ * i915_vma_verify_bind_complete() - Check for the bind completion of the vma
+ * @vma: vma to check for bind completion
+ *
+ * Returns: 0 if the vma bind is completed. Error code otherwise.
+ */
+int i915_vma_verify_bind_complete(struct i915_vma *vma)
{
struct dma_fence *fence = i915_active_fence_get(>active.excl);
int err;
+ /* Ensure vma bind is initiated */
+ if (!i915_vma_is_bound(vma, I915_VMA_BIND_MASK))
+ return -EINVAL;
+
if (!fence)
return 0;
@@ -457,9 +466,6 @@ static int i915_vma_verify_bind_complete(struct i915_vma
*vma)
return err;
}
-#else
-#define i915_vma_verify_bind_complete(_vma) 0
-#endif
I915_SELFTEST_EXPORT void
i915_vma_resource_init_from_vma(struct i915_vma_resource *vma_res,
diff --git a/drivers/gpu/drm/i915/i915_vma.h b/drivers/gpu/drm/i915/i915_vma.h
index 1cadbf8fdedf..04770f8ba815 100644
--- a/drivers/gpu/drm/i915/i915_vma.h
+++ b/drivers/gpu/drm/i915/i915_vma.h
@@ -440,6 +440,7 @@ void i915_vma_make_purgeable(struct i915_vma *vma);
int i915_vma_wait_for_bind(struct i915_vma *vma);
int i915_vma_sync(struct i915_vma *vma);
+int i915_vma_verify_bind_complete(struct i915_vma *vma);
/**
* i915_vma_get_current_resource - Get the current resource of the vma
--
2.21.0.rc0.32.g243a4c7e27
[PATCH v2 11/17] drm/i915/vm_bind: Use common execbuf functions in execbuf path
Update the execbuf path to use common execbuf functions to
reduce code duplication with the newer execbuf3 path.
Acked-by: Matthew Auld
Signed-off-by: Niranjana Vishwanathapura
---
.../gpu/drm/i915/gem/i915_gem_execbuffer.c| 507 ++
1 file changed, 38 insertions(+), 469 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
index 4673e0812277..eb8491bd32c1 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
@@ -28,6 +28,7 @@
#include "i915_file_private.h"
#include "i915_gem_clflush.h"
#include "i915_gem_context.h"
+#include "i915_gem_execbuffer_common.h"
#include "i915_gem_evict.h"
#include "i915_gem_ioctls.h"
#include "i915_trace.h"
@@ -235,13 +236,6 @@ enum {
* the batchbuffer in trusted mode, otherwise the ioctl is rejected.
*/
-struct eb_fence {
- struct drm_syncobj *syncobj; /* Use with ptr_mask_bits() */
- struct dma_fence *dma_fence;
- u64 value;
- struct dma_fence_chain *chain_fence;
-};
-
struct i915_execbuffer {
struct drm_i915_private *i915; /** i915 backpointer */
struct drm_file *file; /** per-file lookup tables and limits */
@@ -2446,164 +2440,29 @@ static const enum intel_engine_id user_ring_map[] = {
[I915_EXEC_VEBOX] = VECS0
};
-static struct i915_request *eb_throttle(struct i915_execbuffer *eb, struct
intel_context *ce)
-{
- struct intel_ring *ring = ce->ring;
- struct intel_timeline *tl = ce->timeline;
- struct i915_request *rq;
-
- /*
-* Completely unscientific finger-in-the-air estimates for suitable
-* maximum user request size (to avoid blocking) and then backoff.
-*/
- if (intel_ring_update_space(ring) >= PAGE_SIZE)
- return NULL;
-
- /*
-* Find a request that after waiting upon, there will be at least half
-* the ring available. The hysteresis allows us to compete for the
-* shared ring and should mean that we sleep less often prior to
-* claiming our resources, but not so long that the ring completely
-* drains before we can submit our next request.
-*/
- list_for_each_entry(rq, >requests, link) {
- if (rq->ring != ring)
- continue;
-
- if (__intel_ring_space(rq->postfix,
- ring->emit, ring->size) > ring->size / 2)
- break;
- }
- if (>link == >requests)
- return NULL; /* weird, we will check again later for real */
-
- return i915_request_get(rq);
-}
-
-static int eb_pin_timeline(struct i915_execbuffer *eb, struct intel_context
*ce,
- bool throttle)
-{
- struct intel_timeline *tl;
- struct i915_request *rq = NULL;
-
- /*
-* Take a local wakeref for preparing to dispatch the execbuf as
-* we expect to access the hardware fairly frequently in the
-* process, and require the engine to be kept awake between accesses.
-* Upon dispatch, we acquire another prolonged wakeref that we hold
-* until the timeline is idle, which in turn releases the wakeref
-* taken on the engine, and the parent device.
-*/
- tl = intel_context_timeline_lock(ce);
- if (IS_ERR(tl))
- return PTR_ERR(tl);
-
- intel_context_enter(ce);
- if (throttle)
- rq = eb_throttle(eb, ce);
- intel_context_timeline_unlock(tl);
-
- if (rq) {
- bool nonblock = eb->file->filp->f_flags & O_NONBLOCK;
- long timeout = nonblock ? 0 : MAX_SCHEDULE_TIMEOUT;
-
- if (i915_request_wait(rq, I915_WAIT_INTERRUPTIBLE,
- timeout) < 0) {
- i915_request_put(rq);
-
- /*
-* Error path, cannot use intel_context_timeline_lock as
-* that is user interruptable and this clean up step
-* must be done.
-*/
- mutex_lock(>timeline->mutex);
- intel_context_exit(ce);
- mutex_unlock(>timeline->mutex);
-
- if (nonblock)
- return -EWOULDBLOCK;
- else
- return -EINTR;
- }
- i915_request_put(rq);
- }
-
- return 0;
-}
-
static int eb_pin_engine(struct i915_execbuffer *eb, bool throttle)
{
- struct intel_context *ce = eb->context, *child;
int err;
- int i = 0, j = 0;
GEM_BUG_ON(eb->args->flags & __EXEC_ENGINE_PINNED);
- if (unlikely(intel_context_is_banned(ce)))
- return -EIO;
-
- /*
-* Pinning the contexts may generate requests in
[PATCH v2 06/17] drm/i915/vm_bind: Support for VM private BOs
Each VM creates a root_obj and shares it with all of its private objects
to use it as dma_resv object. This has a performance advantage as it
requires a single dma_resv object update for all private BOs vs list of
dma_resv objects update for shared BOs, in the execbuf path.
VM private BOs can be only mapped on specified VM and cannot be dmabuf
exported. Also, they are supported only in vm_bind mode.
v2: Pad struct drm_i915_gem_create_ext_vm_private for 64bit alignment,
add input validity checks.
Signed-off-by: Niranjana Vishwanathapura
Signed-off-by: Andi Shyti
---
drivers/gpu/drm/i915/gem/i915_gem_create.c| 49 ++-
drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c| 6 +++
.../gpu/drm/i915/gem/i915_gem_execbuffer.c| 4 ++
drivers/gpu/drm/i915/gem/i915_gem_object.c| 3 ++
.../gpu/drm/i915/gem/i915_gem_object_types.h | 3 ++
drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 3 ++
.../drm/i915/gem/i915_gem_vm_bind_object.c| 9
drivers/gpu/drm/i915/gt/intel_gtt.c | 4 ++
drivers/gpu/drm/i915/gt/intel_gtt.h | 2 +
drivers/gpu/drm/i915/i915_vma.c | 1 +
drivers/gpu/drm/i915/i915_vma_types.h | 2 +
include/uapi/drm/i915_drm.h | 33 +
12 files changed, 117 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_create.c
b/drivers/gpu/drm/i915/gem/i915_gem_create.c
index 5c6e396ab74d..694d4638ac8b 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_create.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_create.c
@@ -11,6 +11,7 @@
#include "pxp/intel_pxp.h"
#include "i915_drv.h"
+#include "i915_gem_context.h"
#include "i915_gem_create.h"
#include "i915_trace.h"
#include "i915_user_extensions.h"
@@ -251,6 +252,7 @@ struct create_ext {
unsigned int n_placements;
unsigned int placement_mask;
unsigned long flags;
+ u32 vm_id;
};
static void repr_placements(char *buf, size_t size,
@@ -400,9 +402,32 @@ static int ext_set_protected(struct i915_user_extension
__user *base, void *data
return 0;
}
+static int ext_set_vm_private(struct i915_user_extension __user *base,
+ void *data)
+{
+ struct drm_i915_gem_create_ext_vm_private ext;
+ struct create_ext *ext_data = data;
+
+ if (copy_from_user(, base, sizeof(ext)))
+ return -EFAULT;
+
+ /* Reserved fields must be 0 */
+ if (ext.rsvd)
+ return -EINVAL;
+
+ /* vm_id 0 is reserved */
+ if (!ext.vm_id)
+ return -ENOENT;
+
+ ext_data->vm_id = ext.vm_id;
+
+ return 0;
+}
+
static const i915_user_extension_fn create_extensions[] = {
[I915_GEM_CREATE_EXT_MEMORY_REGIONS] = ext_set_placements,
[I915_GEM_CREATE_EXT_PROTECTED_CONTENT] = ext_set_protected,
+ [I915_GEM_CREATE_EXT_VM_PRIVATE] = ext_set_vm_private,
};
/**
@@ -418,6 +443,7 @@ i915_gem_create_ext_ioctl(struct drm_device *dev, void
*data,
struct drm_i915_private *i915 = to_i915(dev);
struct drm_i915_gem_create_ext *args = data;
struct create_ext ext_data = { .i915 = i915 };
+ struct i915_address_space *vm = NULL;
struct drm_i915_gem_object *obj;
int ret;
@@ -431,6 +457,12 @@ i915_gem_create_ext_ioctl(struct drm_device *dev, void
*data,
if (ret)
return ret;
+ if (ext_data.vm_id) {
+ vm = i915_gem_vm_lookup(file->driver_priv, ext_data.vm_id);
+ if (unlikely(!vm))
+ return -ENOENT;
+ }
+
if (!ext_data.n_placements) {
ext_data.placements[0] =
intel_memory_region_by_type(i915, INTEL_MEMORY_SYSTEM);
@@ -457,8 +489,21 @@ i915_gem_create_ext_ioctl(struct drm_device *dev, void
*data,
ext_data.placements,
ext_data.n_placements,
ext_data.flags);
- if (IS_ERR(obj))
- return PTR_ERR(obj);
+ if (IS_ERR(obj)) {
+ ret = PTR_ERR(obj);
+ goto vm_put;
+ }
+
+ if (vm) {
+ obj->base.resv = vm->root_obj->base.resv;
+ obj->priv_root = i915_gem_object_get(vm->root_obj);
+ i915_vm_put(vm);
+ }
return i915_gem_publish(obj, file, >size, >handle);
+vm_put:
+ if (vm)
+ i915_vm_put(vm);
+
+ return ret;
}
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
index f5062d0c6333..6433173c3e84 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
@@ -218,6 +218,12 @@ struct dma_buf *i915_gem_prime_export(struct
drm_gem_object *gem_obj, int flags)
struct drm_i915_gem_object *obj = to_intel_bo(gem_obj);
DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
[PATCH v2 14/17] drm/i915/vm_bind: Expose i915_request_await_bind()
Rename __i915_request_await_bind() as i915_request_await_bind()
and make it non-static as it will be used in execbuf3 ioctl path.
Signed-off-by: Niranjana Vishwanathapura
---
drivers/gpu/drm/i915/i915_vma.c | 8 +---
drivers/gpu/drm/i915/i915_vma.h | 6 ++
2 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
index 492a00d09cf3..9e8f30022721 100644
--- a/drivers/gpu/drm/i915/i915_vma.c
+++ b/drivers/gpu/drm/i915/i915_vma.c
@@ -1889,18 +1889,12 @@ void i915_vma_revoke_mmap(struct i915_vma *vma)
list_del(>obj->userfault_link);
}
-static int
-__i915_request_await_bind(struct i915_request *rq, struct i915_vma *vma)
-{
- return __i915_request_await_exclusive(rq, >active);
-}
-
static int __i915_vma_move_to_active(struct i915_vma *vma, struct i915_request
*rq)
{
int err;
/* Wait for the vma to be bound before we start! */
- err = __i915_request_await_bind(rq, vma);
+ err = i915_request_await_bind(rq, vma);
if (err)
return err;
diff --git a/drivers/gpu/drm/i915/i915_vma.h b/drivers/gpu/drm/i915/i915_vma.h
index 04770f8ba815..19e57e12b956 100644
--- a/drivers/gpu/drm/i915/i915_vma.h
+++ b/drivers/gpu/drm/i915/i915_vma.h
@@ -54,6 +54,12 @@ void i915_vma_unpin_and_release(struct i915_vma **p_vma,
unsigned int flags);
/* do not reserve memory to prevent deadlocks */
#define __EXEC_OBJECT_NO_RESERVE BIT(31)
+static inline int
+i915_request_await_bind(struct i915_request *rq, struct i915_vma *vma)
+{
+ return __i915_request_await_exclusive(rq, >active);
+}
+
int __must_check _i915_vma_move_to_active(struct i915_vma *vma,
struct i915_request *rq,
struct dma_fence *fence,
--
2.21.0.rc0.32.g243a4c7e27
[PATCH v2 15/17] drm/i915/vm_bind: Handle persistent vmas in execbuf3
Handle persistent (VM_BIND) mappings during the request submission
in the execbuf3 path.
v2: Ensure requests wait for bindings to complete.
Signed-off-by: Niranjana Vishwanathapura
Signed-off-by: Andi Shyti
---
.../gpu/drm/i915/gem/i915_gem_execbuffer3.c | 214 +-
1 file changed, 213 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer3.c
b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer3.c
index 66b723842e45..5cece16949d8 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer3.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer3.c
@@ -19,6 +19,7 @@
#include "i915_gem_vm_bind.h"
#include "i915_trace.h"
+#define __EXEC3_HAS_PINBIT_ULL(33)
#define __EXEC3_ENGINE_PINNED BIT_ULL(32)
#define __EXEC3_INTERNAL_FLAGS (~0ull << 32)
@@ -42,7 +43,9 @@
* execlist. Hence, no support for implicit sync.
*
* The new execbuf3 ioctl only works in VM_BIND mode and the VM_BIND mode only
- * works with execbuf3 ioctl for submission.
+ * works with execbuf3 ioctl for submission. All BOs mapped on that VM (through
+ * VM_BIND call) at the time of execbuf3 call are deemed required for that
+ * submission.
*
* The execbuf3 ioctl directly specifies the batch addresses instead of as
* object handles as in execbuf2 ioctl. The execbuf3 ioctl will also not
@@ -58,6 +61,13 @@
* So, a lot of code supporting execbuf2 ioctl, like relocations, VA evictions,
* vma lookup table, implicit sync, vma active reference tracking etc., are not
* applicable for execbuf3 ioctl.
+ *
+ * During each execbuf submission, request fence is added to all VM_BIND mapped
+ * objects with DMA_RESV_USAGE_BOOKKEEP. The DMA_RESV_USAGE_BOOKKEEP usage will
+ * prevent over sync (See enum dma_resv_usage). Note that DRM_I915_GEM_WAIT and
+ * DRM_I915_GEM_BUSY ioctls do not check for DMA_RESV_USAGE_BOOKKEEP usage and
+ * hence should not be used for end of batch check. Instead, the execbuf3
+ * timeline out fence should be used for end of batch check.
*/
/**
@@ -127,6 +137,23 @@ eb_find_vma(struct i915_address_space *vm, u64 addr)
return i915_gem_vm_bind_lookup_vma(vm, va);
}
+static void eb_scoop_unbound_vma_all(struct i915_address_space *vm)
+{
+ struct i915_vma *vma, *vn;
+
+ /**
+* Move all unbound vmas back into vm_bind_list so that they are
+* revalidated.
+*/
+ spin_lock(>vm_rebind_lock);
+ list_for_each_entry_safe(vma, vn, >vm_rebind_list, vm_rebind_link) {
+ list_del_init(>vm_rebind_link);
+ if (!list_empty(>vm_bind_link))
+ list_move_tail(>vm_bind_link, >vm_bind_list);
+ }
+ spin_unlock(>vm_rebind_lock);
+}
+
static int eb_lookup_vma_all(struct i915_execbuffer *eb)
{
unsigned int i, current_batch = 0;
@@ -141,14 +168,121 @@ static int eb_lookup_vma_all(struct i915_execbuffer *eb)
++current_batch;
}
+ eb_scoop_unbound_vma_all(eb->context->vm);
+
return 0;
}
+static int eb_lock_vma_all(struct i915_execbuffer *eb)
+{
+ struct i915_address_space *vm = eb->context->vm;
+ struct i915_vma *vma;
+ int err;
+
+ err = i915_gem_object_lock(eb->context->vm->root_obj, >ww);
+ if (err)
+ return err;
+
+ list_for_each_entry(vma, >non_priv_vm_bind_list,
+ non_priv_vm_bind_link) {
+ err = i915_gem_object_lock(vma->obj, >ww);
+ if (err)
+ return err;
+ }
+
+ return 0;
+}
+
+static void eb_release_persistent_vma_all(struct i915_execbuffer *eb,
+ bool final)
+{
+ struct i915_address_space *vm = eb->context->vm;
+ struct i915_vma *vma, *vn;
+
+ lockdep_assert_held(>vm_bind_lock);
+
+ if (!(eb->args->flags & __EXEC3_HAS_PIN))
+ return;
+
+ assert_object_held(vm->root_obj);
+
+ list_for_each_entry(vma, >vm_bind_list, vm_bind_link)
+ __i915_vma_unpin(vma);
+
+ eb->args->flags &= ~__EXEC3_HAS_PIN;
+ if (!final)
+ return;
+
+ list_for_each_entry_safe(vma, vn, >vm_bind_list, vm_bind_link)
+ if (i915_vma_verify_bind_complete(vma))
+ list_move_tail(>vm_bind_link, >vm_bound_list);
+}
+
static void eb_release_vma_all(struct i915_execbuffer *eb, bool final)
{
+ eb_release_persistent_vma_all(eb, final);
eb_unpin_engine(eb);
}
+static int eb_reserve_fence_for_persistent_vma_all(struct i915_execbuffer *eb)
+{
+ struct i915_address_space *vm = eb->context->vm;
+ struct i915_vma *vma;
+ int ret;
+
+ ret = dma_resv_reserve_fences(vm->root_obj->base.resv, 1);
+ if (ret)
+ return ret;
+
+ list_for_each_entry(vma, >non_priv_vm_bind_list,
+ non_priv_vm_bind_link) {
+ ret =
[PATCH v2 08/17] drm/i915/vm_bind: Support persistent vma activeness tracking
Do not use i915_vma activeness tracking for persistent vmas.
As persistent vmas are part of working set for each execbuf
submission on that address space (VM), a persistent vma is
active if the VM active. As vm->root_obj->base.resv will be
updated for each submission on that VM, it correctly
represent whether the VM is active or not.
Add i915_vm_is_active() and i915_vm_sync() functions based
on vm->root_obj->base.resv with DMA_RESV_USAGE_BOOKKEEP
usage. dma-resv fence list will be updated with this usage
during each submission with this VM in the new execbuf3
ioctl path.
Update i915_vma_is_active(), i915_vma_sync() and the
__i915_vma_unbind_async() functions to properly handle
persistent vmas.
Reviewed-by: Andi Shyti
Signed-off-by: Niranjana Vishwanathapura
---
drivers/gpu/drm/i915/i915_gem_gtt.c | 39 +
drivers/gpu/drm/i915/i915_gem_gtt.h | 3 +++
drivers/gpu/drm/i915/i915_vma.c | 28 +
drivers/gpu/drm/i915/i915_vma.h | 25 +-
4 files changed, 83 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c
b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 329ff75b80b9..b7d0844de561 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -25,6 +25,45 @@
#include "i915_trace.h"
#include "i915_vgpu.h"
+/**
+ * i915_vm_sync() - Wait until address space is not in use
+ * @vm: address space
+ *
+ * Waits until all requests using the address space are complete.
+ *
+ * Returns: 0 if success, -ve err code upon failure
+ */
+int i915_vm_sync(struct i915_address_space *vm)
+{
+ int ret;
+
+ /* Wait for all requests under this vm to finish */
+ ret = dma_resv_wait_timeout(vm->root_obj->base.resv,
+ DMA_RESV_USAGE_BOOKKEEP, false,
+ MAX_SCHEDULE_TIMEOUT);
+ if (ret < 0)
+ return ret;
+ else if (ret > 0)
+ return 0;
+ else
+ return -ETIMEDOUT;
+}
+
+/**
+ * i915_vm_is_active() - Check if address space is being used
+ * @vm: address space
+ *
+ * Check if any request using the specified address space is
+ * active.
+ *
+ * Returns: true if address space is active, false otherwise.
+ */
+bool i915_vm_is_active(const struct i915_address_space *vm)
+{
+ return !dma_resv_test_signaled(vm->root_obj->base.resv,
+ DMA_RESV_USAGE_BOOKKEEP);
+}
+
int i915_gem_gtt_prepare_pages(struct drm_i915_gem_object *obj,
struct sg_table *pages)
{
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h
b/drivers/gpu/drm/i915/i915_gem_gtt.h
index 8c2f57eb5dda..a5bbdc59d9df 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
@@ -51,4 +51,7 @@ int i915_gem_gtt_insert(struct i915_address_space *vm,
#define PIN_OFFSET_MASKI915_GTT_PAGE_MASK
+int i915_vm_sync(struct i915_address_space *vm);
+bool i915_vm_is_active(const struct i915_address_space *vm);
+
#endif
diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
index 4c0e933a7ea5..abf9ca4267cf 100644
--- a/drivers/gpu/drm/i915/i915_vma.c
+++ b/drivers/gpu/drm/i915/i915_vma.c
@@ -420,6 +420,24 @@ int i915_vma_wait_for_bind(struct i915_vma *vma)
return err;
}
+/**
+ * i915_vma_sync() - Wait for the vma to be idle
+ * @vma: vma to be tested
+ *
+ * Returns 0 on success and error code on failure
+ */
+int i915_vma_sync(struct i915_vma *vma)
+{
+ int ret;
+
+ /* Wait for the asynchronous bindings and pending GPU reads */
+ ret = i915_active_wait(>active);
+ if (ret || !i915_vma_is_persistent(vma) || i915_vma_is_purged(vma))
+ return ret;
+
+ return i915_vm_sync(vma->vm);
+}
+
#if IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM)
static int i915_vma_verify_bind_complete(struct i915_vma *vma)
{
@@ -1887,6 +1905,8 @@ int _i915_vma_move_to_active(struct i915_vma *vma,
int err;
assert_object_held(obj);
+ if (i915_vma_is_persistent(vma))
+ return -EINVAL;
GEM_BUG_ON(!vma->pages);
@@ -2097,6 +2117,14 @@ static struct dma_fence *__i915_vma_unbind_async(struct
i915_vma *vma)
return ERR_PTR(-EBUSY);
}
+ if (i915_vma_is_persistent(vma) &&
+ __i915_sw_fence_await_reservation(>resource->chain,
+ vma->vm->root_obj->base.resv,
+ DMA_RESV_USAGE_BOOKKEEP,
+ i915_fence_timeout(vma->vm->i915),
+ GFP_NOWAIT | __GFP_NOWARN) < 0)
+ return ERR_PTR(-EBUSY);
+
fence = __i915_vma_evict(vma, true);
drm_mm_remove_node(>node); /* pairs with i915_vma_release() */
diff --git a/drivers/gpu/drm/i915/i915_vma.h b/drivers/gpu/drm/i915/i915_vma.h
index
[PATCH v2 07/17] drm/i915/vm_bind: Add support to handle object evictions
Support eviction by maintaining a list of evicted persistent vmas
for rebinding during next submission. Ensure the list do not
include persistent vmas that are being purged.
v2: Remove unused I915_VMA_PURGED definition.
Acked-by: Matthew Auld
Signed-off-by: Niranjana Vishwanathapura
Signed-off-by: Andi Shyti
---
.../drm/i915/gem/i915_gem_vm_bind_object.c| 6 ++
drivers/gpu/drm/i915/gt/intel_gtt.c | 2 ++
drivers/gpu/drm/i915/gt/intel_gtt.h | 4
drivers/gpu/drm/i915/i915_vma.c | 19 +++
drivers/gpu/drm/i915/i915_vma.h | 10 ++
drivers/gpu/drm/i915/i915_vma_types.h | 8
6 files changed, 49 insertions(+)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_vm_bind_object.c
b/drivers/gpu/drm/i915/gem/i915_gem_vm_bind_object.c
index 1ade9a5847fa..b51f8088d914 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_vm_bind_object.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_vm_bind_object.c
@@ -85,6 +85,12 @@ static void i915_gem_vm_bind_remove(struct i915_vma *vma,
bool release_obj)
{
lockdep_assert_held(>vm->vm_bind_lock);
+ spin_lock(>vm->vm_rebind_lock);
+ if (!list_empty(>vm_rebind_link))
+ list_del_init(>vm_rebind_link);
+ i915_vma_set_purged(vma);
+ spin_unlock(>vm->vm_rebind_lock);
+
list_del_init(>vm_bind_link);
list_del_init(>non_priv_vm_bind_link);
i915_vm_bind_it_remove(vma, >vm->va);
diff --git a/drivers/gpu/drm/i915/gt/intel_gtt.c
b/drivers/gpu/drm/i915/gt/intel_gtt.c
index 65276da137b8..fe5902ea4065 100644
--- a/drivers/gpu/drm/i915/gt/intel_gtt.c
+++ b/drivers/gpu/drm/i915/gt/intel_gtt.c
@@ -296,6 +296,8 @@ void i915_address_space_init(struct i915_address_space *vm,
int subclass)
INIT_LIST_HEAD(>non_priv_vm_bind_list);
vm->root_obj = i915_gem_object_create_internal(vm->i915, PAGE_SIZE);
GEM_BUG_ON(IS_ERR(vm->root_obj));
+ INIT_LIST_HEAD(>vm_rebind_list);
+ spin_lock_init(>vm_rebind_lock);
}
void *__px_vaddr(struct drm_i915_gem_object *p)
diff --git a/drivers/gpu/drm/i915/gt/intel_gtt.h
b/drivers/gpu/drm/i915/gt/intel_gtt.h
index 4ae5734f7d6b..443d1918ad4e 100644
--- a/drivers/gpu/drm/i915/gt/intel_gtt.h
+++ b/drivers/gpu/drm/i915/gt/intel_gtt.h
@@ -265,6 +265,10 @@ struct i915_address_space {
struct list_head vm_bind_list;
/** @vm_bound_list: List of vm_binding completed */
struct list_head vm_bound_list;
+ /* @vm_rebind_list: list of vmas to be rebinded */
+ struct list_head vm_rebind_list;
+ /* @vm_rebind_lock: protects vm_rebound_list */
+ spinlock_t vm_rebind_lock;
/* @va: tree of persistent vmas */
struct rb_root_cached va;
struct list_head non_priv_vm_bind_list;
diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
index 5d3d67a4bf47..4c0e933a7ea5 100644
--- a/drivers/gpu/drm/i915/i915_vma.c
+++ b/drivers/gpu/drm/i915/i915_vma.c
@@ -241,6 +241,7 @@ vma_create(struct drm_i915_gem_object *obj,
INIT_LIST_HEAD(>vm_bind_link);
INIT_LIST_HEAD(>non_priv_vm_bind_link);
+ INIT_LIST_HEAD(>vm_rebind_link);
return vma;
err_unlock:
@@ -1686,6 +1687,14 @@ static void force_unbind(struct i915_vma *vma)
if (!drm_mm_node_allocated(>node))
return;
+ /*
+* Persistent vma should have been purged by now.
+* If not, issue a warning and purge it.
+*/
+ if (GEM_WARN_ON(i915_vma_is_persistent(vma) &&
+ !i915_vma_is_purged(vma)))
+ i915_vma_set_purged(vma);
+
atomic_and(~I915_VMA_PIN_MASK, >flags);
WARN_ON(__i915_vma_unbind(vma));
GEM_BUG_ON(drm_mm_node_allocated(>node));
@@ -2047,6 +2056,16 @@ int __i915_vma_unbind(struct i915_vma *vma)
__i915_vma_evict(vma, false);
drm_mm_remove_node(>node); /* pairs with i915_vma_release() */
+
+ if (i915_vma_is_persistent(vma)) {
+ spin_lock(>vm->vm_rebind_lock);
+ if (list_empty(>vm_rebind_link) &&
+ !i915_vma_is_purged(vma))
+ list_add_tail(>vm_rebind_link,
+ >vm->vm_rebind_list);
+ spin_unlock(>vm->vm_rebind_lock);
+ }
+
return 0;
}
diff --git a/drivers/gpu/drm/i915/i915_vma.h b/drivers/gpu/drm/i915/i915_vma.h
index c5378ec2f70a..9a4a7a8dfe5b 100644
--- a/drivers/gpu/drm/i915/i915_vma.h
+++ b/drivers/gpu/drm/i915/i915_vma.h
@@ -152,6 +152,16 @@ static inline void i915_vma_set_persistent(struct i915_vma
*vma)
set_bit(I915_VMA_PERSISTENT_BIT, __i915_vma_flags(vma));
}
+static inline bool i915_vma_is_purged(const struct i915_vma *vma)
+{
+ return test_bit(I915_VMA_PURGED_BIT, __i915_vma_flags(vma));
+}
+
+static inline void i915_vma_set_purged(struct i915_vma *vma)
+{
+ set_bit(I915_VMA_PURGED_BIT, __i915_vma_flags(vma));
+}
+
static
[PATCH v2 09/17] drm/i915/vm_bind: Add out fence support
Add support for handling out fence for vm_bind call.
Signed-off-by: Niranjana Vishwanathapura
Signed-off-by: Andi Shyti
---
drivers/gpu/drm/i915/gem/i915_gem_vm_bind.h | 4 +
.../drm/i915/gem/i915_gem_vm_bind_object.c| 81 +++
drivers/gpu/drm/i915/i915_vma.c | 7 +-
drivers/gpu/drm/i915/i915_vma_types.h | 7 ++
include/uapi/drm/i915_drm.h | 63 ++-
5 files changed, 157 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_vm_bind.h
b/drivers/gpu/drm/i915/gem/i915_gem_vm_bind.h
index 36262a6357b5..b70e900e35ab 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_vm_bind.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_vm_bind.h
@@ -8,6 +8,7 @@
#include
+struct dma_fence;
struct drm_device;
struct drm_file;
struct i915_address_space;
@@ -23,4 +24,7 @@ int i915_gem_vm_unbind_ioctl(struct drm_device *dev, void
*data,
void i915_gem_vm_unbind_all(struct i915_address_space *vm);
+void i915_vm_bind_signal_fence(struct i915_vma *vma,
+ struct dma_fence * const fence);
+
#endif /* __I915_GEM_VM_BIND_H */
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_vm_bind_object.c
b/drivers/gpu/drm/i915/gem/i915_gem_vm_bind_object.c
index b51f8088d914..2df7d529276f 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_vm_bind_object.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_vm_bind_object.c
@@ -7,6 +7,8 @@
#include
+#include
+
#include "gem/i915_gem_context.h"
#include "gem/i915_gem_vm_bind.h"
@@ -100,6 +102,75 @@ static void i915_gem_vm_bind_remove(struct i915_vma *vma,
bool release_obj)
i915_gem_object_put(vma->obj);
}
+static int i915_vm_bind_add_fence(struct drm_file *file, struct i915_vma *vma,
+ u32 handle, u64 point)
+{
+ struct drm_syncobj *syncobj;
+
+ syncobj = drm_syncobj_find(file, handle);
+ if (!syncobj) {
+ DRM_DEBUG("Invalid syncobj handle provided\n");
+ return -ENOENT;
+ }
+
+ /*
+* For timeline syncobjs we need to preallocate chains for
+* later signaling.
+*/
+ if (point) {
+ vma->vm_bind_fence.chain_fence = dma_fence_chain_alloc();
+ if (!vma->vm_bind_fence.chain_fence) {
+ drm_syncobj_put(syncobj);
+ return -ENOMEM;
+ }
+ } else {
+ vma->vm_bind_fence.chain_fence = NULL;
+ }
+ vma->vm_bind_fence.syncobj = syncobj;
+ vma->vm_bind_fence.value = point;
+
+ return 0;
+}
+
+static void i915_vm_bind_put_fence(struct i915_vma *vma)
+{
+ if (!vma->vm_bind_fence.syncobj)
+ return;
+
+ drm_syncobj_put(vma->vm_bind_fence.syncobj);
+ dma_fence_chain_free(vma->vm_bind_fence.chain_fence);
+}
+
+/**
+ * i915_vm_bind_signal_fence() - Add fence to vm_bind syncobj
+ * @vma: vma mapping requiring signaling
+ * @fence: fence to be added
+ *
+ * Associate specified @fence with the @vma's syncobj to be
+ * signaled after the @fence work completes.
+ */
+void i915_vm_bind_signal_fence(struct i915_vma *vma,
+ struct dma_fence * const fence)
+{
+ struct drm_syncobj *syncobj = vma->vm_bind_fence.syncobj;
+
+ if (!syncobj)
+ return;
+
+ if (vma->vm_bind_fence.chain_fence) {
+ drm_syncobj_add_point(syncobj,
+ vma->vm_bind_fence.chain_fence,
+ fence, vma->vm_bind_fence.value);
+ /*
+* The chain's ownership is transferred to the
+* timeline.
+*/
+ vma->vm_bind_fence.chain_fence = NULL;
+ } else {
+ drm_syncobj_replace_fence(syncobj, fence);
+ }
+}
+
static int i915_gem_vm_unbind_vma(struct i915_address_space *vm,
struct drm_i915_gem_vm_unbind *va)
{
@@ -237,6 +308,13 @@ static int i915_gem_vm_bind_obj(struct i915_address_space
*vm,
goto unlock_vm;
}
+ if (va->fence.flags & I915_TIMELINE_FENCE_SIGNAL) {
+ ret = i915_vm_bind_add_fence(file, vma, va->fence.handle,
+va->fence.value);
+ if (ret)
+ goto put_vma;
+ }
+
pin_flags = va->start | PIN_OFFSET_FIXED | PIN_USER;
for_i915_gem_ww(, ret, true) {
@@ -265,6 +343,9 @@ static int i915_gem_vm_bind_obj(struct i915_address_space
*vm,
i915_gem_object_get(vma->obj);
}
+ if (va->fence.flags & I915_TIMELINE_FENCE_SIGNAL)
+ i915_vm_bind_put_fence(vma);
+put_vma:
if (ret)
i915_vma_destroy(vma);
unlock_vm:
diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
index abf9ca4267cf..e62d769d8a55 100644
--- a/drivers/gpu/drm/i915/i915_vma.c
+++
[PATCH v2 10/17] drm/i915/vm_bind: Abstract out common execbuf functions
The new execbuf3 ioctl path and the legacy execbuf ioctl
paths have many common functionalities.
Abstract out the common execbuf functionalities into a
separate file where possible, thus allowing code sharing.
Acked-by: Matthew Auld
Signed-off-by: Niranjana Vishwanathapura
---
drivers/gpu/drm/i915/Makefile | 1 +
.../drm/i915/gem/i915_gem_execbuffer_common.c | 666 ++
.../drm/i915/gem/i915_gem_execbuffer_common.h | 74 ++
3 files changed, 741 insertions(+)
create mode 100644 drivers/gpu/drm/i915/gem/i915_gem_execbuffer_common.c
create mode 100644 drivers/gpu/drm/i915/gem/i915_gem_execbuffer_common.h
diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 9bf939ef18ea..bf952f478555 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -148,6 +148,7 @@ gem-y += \
gem/i915_gem_create.o \
gem/i915_gem_dmabuf.o \
gem/i915_gem_domain.o \
+ gem/i915_gem_execbuffer_common.o \
gem/i915_gem_execbuffer.o \
gem/i915_gem_internal.o \
gem/i915_gem_object.o \
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer_common.c
b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer_common.c
new file mode 100644
index ..4d1c9ce154b5
--- /dev/null
+++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer_common.c
@@ -0,0 +1,666 @@
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright © 2022 Intel Corporation
+ */
+
+#include
+
+#include
+
+#include "gt/intel_context.h"
+#include "gt/intel_gt.h"
+#include "gt/intel_gt_pm.h"
+#include "gt/intel_ring.h"
+
+#include "i915_gem_execbuffer_common.h"
+
+#define __EXEC_COMMON_FENCE_WAIT BIT(0)
+#define __EXEC_COMMON_FENCE_SIGNAL BIT(1)
+
+static struct i915_request *eb_throttle(struct intel_context *ce)
+{
+ struct intel_ring *ring = ce->ring;
+ struct intel_timeline *tl = ce->timeline;
+ struct i915_request *rq;
+
+ /*
+* Completely unscientific finger-in-the-air estimates for suitable
+* maximum user request size (to avoid blocking) and then backoff.
+*/
+ if (intel_ring_update_space(ring) >= PAGE_SIZE)
+ return NULL;
+
+ /*
+* Find a request that after waiting upon, there will be at least half
+* the ring available. The hysteresis allows us to compete for the
+* shared ring and should mean that we sleep less often prior to
+* claiming our resources, but not so long that the ring completely
+* drains before we can submit our next request.
+*/
+ list_for_each_entry(rq, >requests, link) {
+ if (rq->ring != ring)
+ continue;
+
+ if (__intel_ring_space(rq->postfix,
+ ring->emit, ring->size) > ring->size / 2)
+ break;
+ }
+ if (>link == >requests)
+ return NULL; /* weird, we will check again later for real */
+
+ return i915_request_get(rq);
+}
+
+static int eb_pin_timeline(struct intel_context *ce, bool throttle,
+ bool nonblock)
+{
+ struct intel_timeline *tl;
+ struct i915_request *rq = NULL;
+
+ /*
+* Take a local wakeref for preparing to dispatch the execbuf as
+* we expect to access the hardware fairly frequently in the
+* process, and require the engine to be kept awake between accesses.
+* Upon dispatch, we acquire another prolonged wakeref that we hold
+* until the timeline is idle, which in turn releases the wakeref
+* taken on the engine, and the parent device.
+*/
+ tl = intel_context_timeline_lock(ce);
+ if (IS_ERR(tl))
+ return PTR_ERR(tl);
+
+ intel_context_enter(ce);
+ if (throttle)
+ rq = eb_throttle(ce);
+ intel_context_timeline_unlock(tl);
+
+ if (rq) {
+ long timeout = nonblock ? 0 : MAX_SCHEDULE_TIMEOUT;
+
+ if (i915_request_wait(rq, I915_WAIT_INTERRUPTIBLE,
+ timeout) < 0) {
+ i915_request_put(rq);
+
+ /*
+* Error path, cannot use intel_context_timeline_lock as
+* that is user interruptable and this clean up step
+* must be done.
+*/
+ mutex_lock(>timeline->mutex);
+ intel_context_exit(ce);
+ mutex_unlock(>timeline->mutex);
+
+ if (nonblock)
+ return -EWOULDBLOCK;
+ else
+ return -EINTR;
+ }
+ i915_request_put(rq);
+ }
+
+ return 0;
+}
+
+/**
+ * i915_eb_pin_engine() - Pin the engine
+ * @ce: the context
+ * @ww: optional locking context or NULL
+ * @throttle: throttle to ensure
[PATCH v2 16/17] drm/i915/vm_bind: userptr dma-resv changes
For persistent (vm_bind) vmas of userptr BOs, handle the user
page pinning by using the i915_gem_object_userptr_submit_init()
/done() functions
Signed-off-by: Niranjana Vishwanathapura
Signed-off-by: Andi Shyti
---
.../gpu/drm/i915/gem/i915_gem_execbuffer3.c | 80 +++
drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 17
.../drm/i915/gem/i915_gem_vm_bind_object.c| 15
drivers/gpu/drm/i915/gt/intel_gtt.c | 2 +
drivers/gpu/drm/i915/gt/intel_gtt.h | 4 +
drivers/gpu/drm/i915/i915_vma_types.h | 2 +
6 files changed, 120 insertions(+)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer3.c
b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer3.c
index 5cece16949d8..e5f8eacc0de6 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer3.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer3.c
@@ -19,6 +19,7 @@
#include "i915_gem_vm_bind.h"
#include "i915_trace.h"
+#define __EXEC3_USERPTR_USED BIT_ULL(34)
#define __EXEC3_HAS_PINBIT_ULL(33)
#define __EXEC3_ENGINE_PINNED BIT_ULL(32)
#define __EXEC3_INTERNAL_FLAGS (~0ull << 32)
@@ -141,6 +142,21 @@ static void eb_scoop_unbound_vma_all(struct
i915_address_space *vm)
{
struct i915_vma *vma, *vn;
+#ifdef CONFIG_MMU_NOTIFIER
+ /**
+* Move all invalidated userptr vmas back into vm_bind_list so that
+* they are looked up and revalidated.
+*/
+ spin_lock(>userptr_invalidated_lock);
+ list_for_each_entry_safe(vma, vn, >userptr_invalidated_list,
+userptr_invalidated_link) {
+ list_del_init(>userptr_invalidated_link);
+ if (!list_empty(>vm_bind_link))
+ list_move_tail(>vm_bind_link, >vm_bind_list);
+ }
+ spin_unlock(>userptr_invalidated_lock);
+#endif
+
/**
* Move all unbound vmas back into vm_bind_list so that they are
* revalidated.
@@ -154,10 +170,47 @@ static void eb_scoop_unbound_vma_all(struct
i915_address_space *vm)
spin_unlock(>vm_rebind_lock);
}
+static int eb_lookup_persistent_userptr_vmas(struct i915_execbuffer *eb)
+{
+ struct i915_address_space *vm = eb->context->vm;
+ struct i915_vma *last_vma = NULL;
+ struct i915_vma *vma;
+ int err;
+
+ lockdep_assert_held(>vm_bind_lock);
+
+ list_for_each_entry(vma, >vm_bind_list, vm_bind_link) {
+ if (!i915_gem_object_is_userptr(vma->obj))
+ continue;
+
+ err = i915_gem_object_userptr_submit_init(vma->obj);
+ if (err)
+ return err;
+
+ /**
+* The above submit_init() call does the object unbind and
+* hence adds vma into vm_rebind_list. Remove it from that
+* list as it is already scooped for revalidation.
+*/
+ spin_lock(>vm_rebind_lock);
+ if (!list_empty(>vm_rebind_link))
+ list_del_init(>vm_rebind_link);
+ spin_unlock(>vm_rebind_lock);
+
+ last_vma = vma;
+ }
+
+ if (last_vma)
+ eb->args->flags |= __EXEC3_USERPTR_USED;
+
+ return 0;
+}
+
static int eb_lookup_vma_all(struct i915_execbuffer *eb)
{
unsigned int i, current_batch = 0;
struct i915_vma *vma;
+ int err = 0;
for (i = 0; i < eb->num_batches; i++) {
vma = eb_find_vma(eb->context->vm, eb->batch_addresses[i]);
@@ -170,6 +223,10 @@ static int eb_lookup_vma_all(struct i915_execbuffer *eb)
eb_scoop_unbound_vma_all(eb->context->vm);
+ err = eb_lookup_persistent_userptr_vmas(eb);
+ if (err)
+ return err;
+
return 0;
}
@@ -349,6 +406,29 @@ static int eb_move_to_gpu(struct i915_execbuffer *eb)
}
}
+#ifdef CONFIG_MMU_NOTIFIER
+ /* Check for further userptr invalidations */
+ spin_lock(>userptr_invalidated_lock);
+ if (!list_empty(>userptr_invalidated_list))
+ err = -EAGAIN;
+ spin_unlock(>userptr_invalidated_lock);
+
+ if (!err && (eb->args->flags & __EXEC3_USERPTR_USED)) {
+ read_lock(>i915->mm.notifier_lock);
+ list_for_each_entry(vma, >vm_bind_list, vm_bind_link) {
+ if (!i915_gem_object_is_userptr(vma->obj))
+ continue;
+
+ err = i915_gem_object_userptr_submit_done(vma->obj);
+ if (err)
+ break;
+ }
+ read_unlock(>i915->mm.notifier_lock);
+ }
+#endif
+ if (unlikely(err))
+ goto err_skip;
+
/* Unconditionally flush any chipset caches (for streaming writes). */
intel_gt_chipset_flush(eb->gt);
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
[PATCH v2 04/17] drm/i915/vm_bind: Add support to create persistent vma
Add i915_vma_instance_persistent() to create persistent vmas.
Persistent vmas will use i915_gtt_view to support partial binding.
vma_lookup is tied to segment of the object instead of section
of VA space. Hence, it do not support aliasing. ie., multiple
mappings (at different VA) point to the same gtt_view of object.
Skip vma_lookup for persistent vmas to support aliasing.
v2: Remove unused I915_VMA_PERSISTENT definition,
update validity check in i915_vma_compare(),
remove unwanted is_persistent check in release_references().
Acked-by: Matthew Auld
Signed-off-by: Niranjana Vishwanathapura
Signed-off-by: Andi Shyti
---
drivers/gpu/drm/i915/i915_vma.c | 36 +--
drivers/gpu/drm/i915/i915_vma.h | 17 -
drivers/gpu/drm/i915/i915_vma_types.h | 6 +
3 files changed, 56 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
index f17c09ead7d7..f9da8010021f 100644
--- a/drivers/gpu/drm/i915/i915_vma.c
+++ b/drivers/gpu/drm/i915/i915_vma.c
@@ -109,7 +109,8 @@ static void __i915_vma_retire(struct i915_active *ref)
static struct i915_vma *
vma_create(struct drm_i915_gem_object *obj,
struct i915_address_space *vm,
- const struct i915_gtt_view *view)
+ const struct i915_gtt_view *view,
+ bool skip_lookup_cache)
{
struct i915_vma *pos = ERR_PTR(-E2BIG);
struct i915_vma *vma;
@@ -196,6 +197,9 @@ vma_create(struct drm_i915_gem_object *obj,
__set_bit(I915_VMA_GGTT_BIT, __i915_vma_flags(vma));
}
+ if (skip_lookup_cache)
+ goto skip_rb_insert;
+
rb = NULL;
p = >vma.tree.rb_node;
while (*p) {
@@ -220,6 +224,7 @@ vma_create(struct drm_i915_gem_object *obj,
rb_link_node(>obj_node, rb, p);
rb_insert_color(>obj_node, >vma.tree);
+skip_rb_insert:
if (i915_vma_is_ggtt(vma))
/*
* We put the GGTT vma at the start of the vma-list, followed
@@ -299,7 +304,34 @@ i915_vma_instance(struct drm_i915_gem_object *obj,
/* vma_create() will resolve the race if another creates the vma */
if (unlikely(!vma))
- vma = vma_create(obj, vm, view);
+ vma = vma_create(obj, vm, view, false);
+
+ GEM_BUG_ON(!IS_ERR(vma) && i915_vma_compare(vma, vm, view));
+ return vma;
+}
+
+/**
+ * i915_vma_create_persistent - create a persistent VMA
+ * @obj: parent drm_i915_gem_object to be mapped
+ * @vm: address space in which the mapping is located
+ * @view: additional mapping requirements
+ *
+ * Creates a persistent vma.
+ *
+ * Returns the vma, or an error pointer.
+ */
+struct i915_vma *
+i915_vma_create_persistent(struct drm_i915_gem_object *obj,
+ struct i915_address_space *vm,
+ const struct i915_gtt_view *view)
+{
+ struct i915_vma *vma;
+
+ GEM_BUG_ON(!kref_read(>ref));
+
+ vma = vma_create(obj, vm, view, true);
+ if (!IS_ERR(vma))
+ i915_vma_set_persistent(vma);
GEM_BUG_ON(!IS_ERR(vma) && i915_vma_compare(vma, vm, view));
return vma;
diff --git a/drivers/gpu/drm/i915/i915_vma.h b/drivers/gpu/drm/i915/i915_vma.h
index aecd9c64486b..c5378ec2f70a 100644
--- a/drivers/gpu/drm/i915/i915_vma.h
+++ b/drivers/gpu/drm/i915/i915_vma.h
@@ -44,6 +44,10 @@ struct i915_vma *
i915_vma_instance(struct drm_i915_gem_object *obj,
struct i915_address_space *vm,
const struct i915_gtt_view *view);
+struct i915_vma *
+i915_vma_create_persistent(struct drm_i915_gem_object *obj,
+ struct i915_address_space *vm,
+ const struct i915_gtt_view *view);
void i915_vma_unpin_and_release(struct i915_vma **p_vma, unsigned int flags);
#define I915_VMA_RELEASE_MAP BIT(0)
@@ -138,6 +142,16 @@ static inline u32 i915_ggtt_pin_bias(struct i915_vma *vma)
return i915_vm_to_ggtt(vma->vm)->pin_bias;
}
+static inline bool i915_vma_is_persistent(const struct i915_vma *vma)
+{
+ return test_bit(I915_VMA_PERSISTENT_BIT, __i915_vma_flags(vma));
+}
+
+static inline void i915_vma_set_persistent(struct i915_vma *vma)
+{
+ set_bit(I915_VMA_PERSISTENT_BIT, __i915_vma_flags(vma));
+}
+
static inline struct i915_vma *i915_vma_get(struct i915_vma *vma)
{
i915_gem_object_get(vma->obj);
@@ -164,7 +178,8 @@ i915_vma_compare(struct i915_vma *vma,
{
ptrdiff_t cmp;
- GEM_BUG_ON(view && !i915_is_ggtt_or_dpt(vm));
+ GEM_BUG_ON(view && !(i915_is_ggtt_or_dpt(vm) ||
+i915_vma_is_persistent(vma)));
cmp = ptrdiff(vma->vm, vm);
if (cmp)
diff --git a/drivers/gpu/drm/i915/i915_vma_types.h
b/drivers/gpu/drm/i915/i915_vma_types.h
index ec0f6c9f57d0..3144d71a0c3e 100644
--- a/drivers/gpu/drm/i915/i915_vma_types.h
+++ b/drivers/gpu/drm/i915/i915_vma_types.h
@@ -264,6 +264,12
[PATCH v2 05/17] drm/i915/vm_bind: Implement bind and unbind of object
Add uapi and implement support for bind and unbind of an
object at the specified GPU virtual addresses.
The vm_bind mode is not supported in legacy execbuf2 ioctl.
It will be supported only in the newer execbuf3 ioctl.
v2: On older platforms ctx->vm is not set, check for it.
In vm_bind call, add vma to vm_bind_list.
Add more input validity checks.
Update some documentation.
Signed-off-by: Niranjana Vishwanathapura
Signed-off-by: Prathap Kumar Valsan
Signed-off-by: Andi Shyti
---
drivers/gpu/drm/i915/Makefile | 1 +
.../gpu/drm/i915/gem/i915_gem_execbuffer.c| 5 +
drivers/gpu/drm/i915/gem/i915_gem_vm_bind.h | 26 ++
.../drm/i915/gem/i915_gem_vm_bind_object.c| 328 ++
drivers/gpu/drm/i915/gt/intel_gtt.c | 10 +
drivers/gpu/drm/i915/gt/intel_gtt.h | 17 +
drivers/gpu/drm/i915/i915_driver.c| 3 +
drivers/gpu/drm/i915/i915_vma.c | 1 +
drivers/gpu/drm/i915/i915_vma_types.h | 14 +
include/uapi/drm/i915_drm.h | 110 ++
10 files changed, 515 insertions(+)
create mode 100644 drivers/gpu/drm/i915/gem/i915_gem_vm_bind.h
create mode 100644 drivers/gpu/drm/i915/gem/i915_gem_vm_bind_object.c
diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index a26edcdadc21..9bf939ef18ea 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -166,6 +166,7 @@ gem-y += \
gem/i915_gem_ttm_move.o \
gem/i915_gem_ttm_pm.o \
gem/i915_gem_userptr.o \
+ gem/i915_gem_vm_bind_object.o \
gem/i915_gem_wait.o \
gem/i915_gemfs.o
i915-y += \
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
index 8f5796cf9c9c..9fb9f6faafd8 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
@@ -781,6 +781,11 @@ static int eb_select_context(struct i915_execbuffer *eb)
if (unlikely(IS_ERR(ctx)))
return PTR_ERR(ctx);
+ if (ctx->vm && ctx->vm->vm_bind_mode) {
+ i915_gem_context_put(ctx);
+ return -EOPNOTSUPP;
+ }
+
eb->gem_context = ctx;
if (i915_gem_context_has_full_ppgtt(ctx))
eb->invalid_flags |= EXEC_OBJECT_NEEDS_GTT;
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_vm_bind.h
b/drivers/gpu/drm/i915/gem/i915_gem_vm_bind.h
new file mode 100644
index ..36262a6357b5
--- /dev/null
+++ b/drivers/gpu/drm/i915/gem/i915_gem_vm_bind.h
@@ -0,0 +1,26 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright © 2022 Intel Corporation
+ */
+
+#ifndef __I915_GEM_VM_BIND_H
+#define __I915_GEM_VM_BIND_H
+
+#include
+
+struct drm_device;
+struct drm_file;
+struct i915_address_space;
+struct i915_vma;
+
+struct i915_vma *
+i915_gem_vm_bind_lookup_vma(struct i915_address_space *vm, u64 va);
+
+int i915_gem_vm_bind_ioctl(struct drm_device *dev, void *data,
+ struct drm_file *file);
+int i915_gem_vm_unbind_ioctl(struct drm_device *dev, void *data,
+struct drm_file *file);
+
+void i915_gem_vm_unbind_all(struct i915_address_space *vm);
+
+#endif /* __I915_GEM_VM_BIND_H */
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_vm_bind_object.c
b/drivers/gpu/drm/i915/gem/i915_gem_vm_bind_object.c
new file mode 100644
index ..88314e3a99d1
--- /dev/null
+++ b/drivers/gpu/drm/i915/gem/i915_gem_vm_bind_object.c
@@ -0,0 +1,328 @@
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright © 2022 Intel Corporation
+ */
+
+#include
+
+#include
+
+#include "gem/i915_gem_context.h"
+#include "gem/i915_gem_vm_bind.h"
+
+#include "gt/intel_gpu_commands.h"
+
+#define START(node) ((node)->start)
+#define LAST(node) ((node)->last)
+
+INTERVAL_TREE_DEFINE(struct i915_vma, rb, u64, __subtree_last,
+START, LAST, static inline, i915_vm_bind_it)
+
+#undef START
+#undef LAST
+
+/**
+ * DOC: VM_BIND/UNBIND ioctls
+ *
+ * DRM_I915_GEM_VM_BIND/UNBIND ioctls allows UMD to bind/unbind GEM buffer
+ * objects (BOs) or sections of a BOs at specified GPU virtual addresses on a
+ * specified address space (VM). Multiple mappings can map to the same physical
+ * pages of an object (aliasing). These mappings (also referred to as
persistent
+ * mappings) will be persistent across multiple GPU submissions (execbuf calls)
+ * issued by the UMD, without user having to provide a list of all required
+ * mappings during each submission (as required by older execbuf mode).
+ *
+ * The VM_BIND/UNBIND calls allow UMDs to request a timeline out fence for
+ * signaling the completion of bind/unbind operation.
+ *
+ * VM_BIND feature is advertised to user via I915_PARAM_VM_BIND_VERSION.
+ * User has to opt-in for VM_BIND mode of binding for an address space (VM)
+ * during VM creation time via I915_VM_CREATE_FLAGS_USE_VM_BIND extension.
+ *
+ * VM_BIND/UNBIND ioctl calls executed on different
[PATCH v2 03/17] drm/i915/vm_bind: Expose i915_gem_object_max_page_size()
Expose i915_gem_object_max_page_size() function non-static
which will be used by the vm_bind feature.
Acked-by: Matthew Auld
Signed-off-by: Niranjana Vishwanathapura
Signed-off-by: Andi Shyti
---
drivers/gpu/drm/i915/gem/i915_gem_create.c | 18 +-
drivers/gpu/drm/i915/gem/i915_gem_object.h | 2 ++
2 files changed, 15 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_create.c
b/drivers/gpu/drm/i915/gem/i915_gem_create.c
index 33673fe7ee0a..5c6e396ab74d 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_create.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_create.c
@@ -15,10 +15,18 @@
#include "i915_trace.h"
#include "i915_user_extensions.h"
-static u32 object_max_page_size(struct intel_memory_region **placements,
- unsigned int n_placements)
+/**
+ * i915_gem_object_max_page_size() - max of min_page_size of the regions
+ * @placements: list of regions
+ * @n_placements: number of the placements
+ *
+ * Returns the largest of min_page_size of the @placements,
+ * or I915_GTT_PAGE_SIZE_4K if @n_placements is 0.
+ */
+u32 i915_gem_object_max_page_size(struct intel_memory_region **placements,
+ unsigned int n_placements)
{
- u32 max_page_size = 0;
+ u32 max_page_size = I915_GTT_PAGE_SIZE_4K;
int i;
for (i = 0; i < n_placements; i++) {
@@ -28,7 +36,6 @@ static u32 object_max_page_size(struct intel_memory_region
**placements,
max_page_size = max_t(u32, max_page_size, mr->min_page_size);
}
- GEM_BUG_ON(!max_page_size);
return max_page_size;
}
@@ -99,7 +106,8 @@ __i915_gem_object_create_user_ext(struct drm_i915_private
*i915, u64 size,
i915_gem_flush_free_objects(i915);
- size = round_up(size, object_max_page_size(placements, n_placements));
+ size = round_up(size, i915_gem_object_max_page_size(placements,
+ n_placements));
if (size == 0)
return ERR_PTR(-EINVAL);
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h
b/drivers/gpu/drm/i915/gem/i915_gem_object.h
index a3b7551a57fc..d53d01b1860a 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h
@@ -47,6 +47,8 @@ static inline bool i915_gem_object_size_2big(u64 size)
}
void i915_gem_init__objects(struct drm_i915_private *i915);
+u32 i915_gem_object_max_page_size(struct intel_memory_region **placements,
+ unsigned int n_placements);
void i915_objects_module_exit(void);
int i915_objects_module_init(void);
--
2.21.0.rc0.32.g243a4c7e27
[PATCH v2 01/17] drm/i915/vm_bind: Expose vm lookup function
Make i915_gem_vm_lookup() function non-static as it will be
used by the vm_bind feature.
Acked-by: Matthew Auld
Signed-off-by: Niranjana Vishwanathapura
Signed-off-by: Andi Shyti
---
drivers/gpu/drm/i915/gem/i915_gem_context.c | 11 ++-
drivers/gpu/drm/i915/gem/i915_gem_context.h | 3 +++
2 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c
b/drivers/gpu/drm/i915/gem/i915_gem_context.c
index 0bcde53c50c6..f4e648ec01ed 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
@@ -346,7 +346,16 @@ static int proto_context_register(struct
drm_i915_file_private *fpriv,
return ret;
}
-static struct i915_address_space *
+/**
+ * i915_gem_vm_lookup() - looks up for the VM reference given the vm id
+ * @file_priv: the private data associated with the user's file
+ * @id: the VM id
+ *
+ * Finds the VM reference associated to a specific id.
+ *
+ * Returns the VM pointer on success, NULL in case of failure.
+ */
+struct i915_address_space *
i915_gem_vm_lookup(struct drm_i915_file_private *file_priv, u32 id)
{
struct i915_address_space *vm;
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.h
b/drivers/gpu/drm/i915/gem/i915_gem_context.h
index e5b0f66ea1fe..899fa8f1e0fe 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.h
@@ -139,6 +139,9 @@ int i915_gem_context_setparam_ioctl(struct drm_device *dev,
void *data,
int i915_gem_context_reset_stats_ioctl(struct drm_device *dev, void *data,
struct drm_file *file);
+struct i915_address_space *
+i915_gem_vm_lookup(struct drm_i915_file_private *file_priv, u32 id);
+
struct i915_gem_context *
i915_gem_context_lookup(struct drm_i915_file_private *file_priv, u32 id);
--
2.21.0.rc0.32.g243a4c7e27
[PATCH v2 00/17] drm/i915/vm_bind: Add VM_BIND functionality
DRM_I915_GEM_VM_BIND/UNBIND ioctls allows UMD to bind/unbind GEM buffer objects (BOs) or sections of a BOs at specified GPU virtual addresses on a specified address space (VM). Multiple mappings can map to the same physical pages of an object (aliasing). These mappings (also referred to as persistent mappings) will be persistent across multiple GPU submissions (execbuf calls) issued by the UMD, without user having to provide a list of all required mappings during each submission (as required by older execbuf mode). This patch series support VM_BIND version 1, as described by the param I915_PARAM_VM_BIND_VERSION. Add new execbuf3 ioctl (I915_GEM_EXECBUFFER3) which only works in vm_bind mode. The vm_bind mode only works with this new execbuf3 ioctl. The new execbuf3 ioctl will not have any execlist support and all the legacy support like relocations etc., are removed. TODOs: * Support out fence for VM_UNBIND ioctl. * Async VM_UNBIND support. * Optimizations. NOTEs: * It is based on below VM_BIND design+uapi rfc. Documentation/gpu/rfc/i915_vm_bind.rst * The IGT RFC series is posted as, [PATCH i-g-t v2 0/8] vm_bind: Add VM_BIND validation support v2: Address various review comments Signed-off-by: Niranjana Vishwanathapura Niranjana Vishwanathapura (17): drm/i915/vm_bind: Expose vm lookup function drm/i915/vm_bind: Add __i915_sw_fence_await_reservation() drm/i915/vm_bind: Expose i915_gem_object_max_page_size() drm/i915/vm_bind: Add support to create persistent vma drm/i915/vm_bind: Implement bind and unbind of object drm/i915/vm_bind: Support for VM private BOs drm/i915/vm_bind: Add support to handle object evictions drm/i915/vm_bind: Support persistent vma activeness tracking drm/i915/vm_bind: Add out fence support drm/i915/vm_bind: Abstract out common execbuf functions drm/i915/vm_bind: Use common execbuf functions in execbuf path drm/i915/vm_bind: Implement I915_GEM_EXECBUFFER3 ioctl drm/i915/vm_bind: Update i915_vma_verify_bind_complete() drm/i915/vm_bind: Expose i915_request_await_bind() drm/i915/vm_bind: Handle persistent vmas in execbuf3 drm/i915/vm_bind: userptr dma-resv changes drm/i915/vm_bind: Add uapi for user to enable vm_bind_mode drivers/gpu/drm/i915/Makefile | 3 + drivers/gpu/drm/i915/gem/i915_gem_context.c | 20 +- drivers/gpu/drm/i915/gem/i915_gem_context.h | 3 + drivers/gpu/drm/i915/gem/i915_gem_create.c| 67 +- drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c| 6 + .../gpu/drm/i915/gem/i915_gem_execbuffer.c| 516 +-- .../gpu/drm/i915/gem/i915_gem_execbuffer3.c | 867 ++ .../drm/i915/gem/i915_gem_execbuffer_common.c | 666 ++ .../drm/i915/gem/i915_gem_execbuffer_common.h | 74 ++ drivers/gpu/drm/i915/gem/i915_gem_ioctls.h| 2 + drivers/gpu/drm/i915/gem/i915_gem_object.c| 3 + drivers/gpu/drm/i915/gem/i915_gem_object.h| 2 + .../gpu/drm/i915/gem/i915_gem_object_types.h | 3 + drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 3 + drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 17 + drivers/gpu/drm/i915/gem/i915_gem_vm_bind.h | 30 + .../drm/i915/gem/i915_gem_vm_bind_object.c| 439 + drivers/gpu/drm/i915/gt/intel_gtt.c | 18 + drivers/gpu/drm/i915/gt/intel_gtt.h | 27 + drivers/gpu/drm/i915/i915_driver.c| 4 + drivers/gpu/drm/i915/i915_drv.h | 2 + drivers/gpu/drm/i915/i915_gem_gtt.c | 39 + drivers/gpu/drm/i915/i915_gem_gtt.h | 3 + drivers/gpu/drm/i915/i915_getparam.c | 3 + drivers/gpu/drm/i915/i915_sw_fence.c | 28 +- drivers/gpu/drm/i915/i915_sw_fence.h | 23 +- drivers/gpu/drm/i915/i915_vma.c | 116 ++- drivers/gpu/drm/i915/i915_vma.h | 57 +- drivers/gpu/drm/i915/i915_vma_types.h | 39 + include/uapi/drm/i915_drm.h | 281 +- 30 files changed, 2842 insertions(+), 519 deletions(-) create mode 100644 drivers/gpu/drm/i915/gem/i915_gem_execbuffer3.c create mode 100644 drivers/gpu/drm/i915/gem/i915_gem_execbuffer_common.c create mode 100644 drivers/gpu/drm/i915/gem/i915_gem_execbuffer_common.h create mode 100644 drivers/gpu/drm/i915/gem/i915_gem_vm_bind.h create mode 100644 drivers/gpu/drm/i915/gem/i915_gem_vm_bind_object.c -- 2.21.0.rc0.32.g243a4c7e27
[PATCH v2 02/17] drm/i915/vm_bind: Add __i915_sw_fence_await_reservation()
Add function __i915_sw_fence_await_reservation() for
asynchronous wait on a dma-resv object with specified
dma_resv_usage. This is required for async vma unbind
with vm_bind.
Acked-by: Matthew Auld
Signed-off-by: Niranjana Vishwanathapura
---
drivers/gpu/drm/i915/i915_sw_fence.c | 28 +---
drivers/gpu/drm/i915/i915_sw_fence.h | 23 +--
2 files changed, 38 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_sw_fence.c
b/drivers/gpu/drm/i915/i915_sw_fence.c
index cc2a8821d22a..ae06d35db056 100644
--- a/drivers/gpu/drm/i915/i915_sw_fence.c
+++ b/drivers/gpu/drm/i915/i915_sw_fence.c
@@ -7,7 +7,6 @@
#include
#include
#include
-#include
#include "i915_sw_fence.h"
#include "i915_selftest.h"
@@ -569,11 +568,26 @@ int __i915_sw_fence_await_dma_fence(struct i915_sw_fence
*fence,
return ret;
}
-int i915_sw_fence_await_reservation(struct i915_sw_fence *fence,
- struct dma_resv *resv,
- bool write,
- unsigned long timeout,
- gfp_t gfp)
+/**
+ * __i915_sw_fence_await_reservation() - Setup a fence to wait on a dma-resv
+ * object with specified usage.
+ * @fence: the fence that needs to wait
+ * @resv: dma-resv object
+ * @usage: dma_resv_usage (See enum dma_resv_usage)
+ * @timeout: how long to wait in jiffies
+ * @gfp: allocation mode
+ *
+ * Setup the @fence to asynchronously wait on dma-resv object @resv for
+ * @usage to complete before signaling.
+ *
+ * Returns 0 if there is nothing to wait on, -ve error code upon error
+ * and >0 upon successfully setting up the wait.
+ */
+int __i915_sw_fence_await_reservation(struct i915_sw_fence *fence,
+ struct dma_resv *resv,
+ enum dma_resv_usage usage,
+ unsigned long timeout,
+ gfp_t gfp)
{
struct dma_resv_iter cursor;
struct dma_fence *f;
@@ -582,7 +596,7 @@ int i915_sw_fence_await_reservation(struct i915_sw_fence
*fence,
debug_fence_assert(fence);
might_sleep_if(gfpflags_allow_blocking(gfp));
- dma_resv_iter_begin(, resv, dma_resv_usage_rw(write));
+ dma_resv_iter_begin(, resv, usage);
dma_resv_for_each_fence_unlocked(, f) {
pending = i915_sw_fence_await_dma_fence(fence, f, timeout,
gfp);
diff --git a/drivers/gpu/drm/i915/i915_sw_fence.h
b/drivers/gpu/drm/i915/i915_sw_fence.h
index f752bfc7c6e1..9c4859dc4c0d 100644
--- a/drivers/gpu/drm/i915/i915_sw_fence.h
+++ b/drivers/gpu/drm/i915/i915_sw_fence.h
@@ -10,13 +10,13 @@
#define _I915_SW_FENCE_H_
#include
+#include
#include
#include
#include /* for NOTIFY_DONE */
#include
struct completion;
-struct dma_resv;
struct i915_sw_fence;
enum i915_sw_fence_notify {
@@ -89,11 +89,22 @@ int i915_sw_fence_await_dma_fence(struct i915_sw_fence
*fence,
unsigned long timeout,
gfp_t gfp);
-int i915_sw_fence_await_reservation(struct i915_sw_fence *fence,
- struct dma_resv *resv,
- bool write,
- unsigned long timeout,
- gfp_t gfp);
+int __i915_sw_fence_await_reservation(struct i915_sw_fence *fence,
+ struct dma_resv *resv,
+ enum dma_resv_usage usage,
+ unsigned long timeout,
+ gfp_t gfp);
+
+static inline int i915_sw_fence_await_reservation(struct i915_sw_fence *fence,
+ struct dma_resv *resv,
+ bool write,
+ unsigned long timeout,
+ gfp_t gfp)
+{
+ return __i915_sw_fence_await_reservation(fence, resv,
+dma_resv_usage_rw(write),
+timeout, gfp);
+}
bool i915_sw_fence_await(struct i915_sw_fence *fence);
void i915_sw_fence_complete(struct i915_sw_fence *fence);
--
2.21.0.rc0.32.g243a4c7e27
