Given that prop changes may lead to modesetting, which would defeat the
fast path of the async flip, refuse any atomic prop change for async
flips in atomic API. The only exceptions are the framebuffer ID to flip
to and the mode ID, that could be referring to an identical mode.
Signed-off-by: André Almeida <[email protected]>
---
v7: drop the mode_id exception for prop changes
---
drivers/gpu/drm/drm_atomic_uapi.c | 47 +++++++++++++++++++++++++++--
drivers/gpu/drm/drm_crtc_internal.h | 2 +-
drivers/gpu/drm/drm_mode_object.c | 2 +-
3 files changed, 46 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index a15121e75a0a..b358de1bf4e7 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -1006,13 +1006,28 @@ int drm_atomic_connector_commit_dpms(struct drm_atomic_state *state,
return ret;
}
+static int drm_atomic_check_prop_changes(int ret, uint64_t old_val, uint64_t prop_value,
+ struct drm_property *prop)
+{
+ if (ret != 0 || old_val != prop_value) {
+ drm_dbg_atomic(prop->dev,
+ "[PROP:%d:%s] No prop can be changed during async flip\n",
+ prop->base.id, prop->name);
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
int drm_atomic_set_property(struct drm_atomic_state *state,
struct drm_file *file_priv,
struct drm_mode_object *obj,
struct drm_property *prop,
- uint64_t prop_value)
+ uint64_t prop_value,
+ bool async_flip)
{
struct drm_mode_object *ref;
+ uint64_t old_val;
int ret;
if (!drm_property_change_valid_get(prop, prop_value, &ref))
@@ -1029,6 +1044,13 @@ int drm_atomic_set_property(struct drm_atomic_state *state,
break;
}
+ if (async_flip) {
+ ret = drm_atomic_connector_get_property(connector, connector_state,
+ prop, &old_val);
+ ret = drm_atomic_check_prop_changes(ret, old_val, prop_value, prop);
+ break;
+ }
+
ret = drm_atomic_connector_set_property(connector,
connector_state, file_priv,
prop, prop_value);
@@ -1037,6 +1059,7 @@ int drm_atomic_set_property(struct drm_atomic_state *state,
case DRM_MODE_OBJECT_CRTC: {
struct drm_crtc *crtc = obj_to_crtc(obj);
struct drm_crtc_state *crtc_state;
+ struct drm_mode_config *config = &crtc->dev->mode_config;
crtc_state = drm_atomic_get_crtc_state(state, crtc);
if (IS_ERR(crtc_state)) {
@@ -1044,6 +1067,13 @@ int drm_atomic_set_property(struct drm_atomic_state *state,
break;
}
+ if (async_flip) {
+ ret = drm_atomic_crtc_get_property(crtc, crtc_state,
+ prop, &old_val);
+ ret = drm_atomic_check_prop_changes(ret, old_val, prop_value, prop);
+ break;
+ }
+
ret = drm_atomic_crtc_set_property(crtc,
crtc_state, prop, prop_value);
break;
@@ -1051,6 +1081,7 @@ int drm_atomic_set_property(struct drm_atomic_state *state,
case DRM_MODE_OBJECT_PLANE: {
struct drm_plane *plane = obj_to_plane(obj);
struct drm_plane_state *plane_state;
+ struct drm_mode_config *config = &plane->dev->mode_config;
plane_state = drm_atomic_get_plane_state(state, plane);
if (IS_ERR(plane_state)) {
@@ -1058,6 +1089,13 @@ int drm_atomic_set_property(struct drm_atomic_state *state,
break;
}
+ if (async_flip && prop != config->prop_fb_id) {
+ ret = drm_atomic_plane_get_property(plane, plane_state,
+ prop, &old_val);
+ ret = drm_atomic_check_prop_changes(ret, old_val, prop_value, prop);
+ break;
+ }
+
ret = drm_atomic_plane_set_property(plane,
plane_state, file_priv,
prop, prop_value);
@@ -1349,6 +1387,7 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
struct drm_out_fence_state *fence_state;
int ret = 0;
unsigned int i, j, num_fences;
+ bool async_flip = false;
/* disallow for drivers not supporting atomic: */
if (!drm_core_check_feature(dev, DRIVER_ATOMIC))
@@ -1385,6 +1424,8 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
"commit failed: DRM_MODE_PAGE_FLIP_ASYNC not supported with atomic\n");
return -EINVAL;
}
+
+ async_flip = true;
}
/* can't test and expect an event at the same time. */
@@ -1469,8 +1510,8 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
goto out;
}
- ret = drm_atomic_set_property(state, file_priv,
- obj, prop, prop_value);
+ ret = drm_atomic_set_property(state, file_priv, obj,
+ prop, prop_value, async_flip);
if (ret) {
drm_mode_object_put(obj);
goto out;
diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h
index 8556c3b3ff88..a4c2ea33b1ef 100644
--- a/drivers/gpu/drm/drm_crtc_internal.h
+++ b/drivers/gpu/drm/drm_crtc_internal.h
@@ -251,7 +251,7 @@ int drm_atomic_set_property(struct drm_atomic_state *state,
struct drm_file *file_priv,
struct drm_mode_object *obj,
struct drm_property *prop,
- uint64_t prop_value);
+ uint64_t prop_value, bool async_flip);
int drm_atomic_get_property(struct drm_mode_object *obj,
struct drm_property *property, uint64_t *val);
diff --git a/drivers/gpu/drm/drm_mode_object.c b/drivers/gpu/drm/drm_mode_object.c
index ac0d2ce3f870..0e8355063eee 100644
--- a/drivers/gpu/drm/drm_mode_object.c
+++ b/drivers/gpu/drm/drm_mode_object.c
@@ -538,7 +538,7 @@ static int set_property_atomic(struct drm_mode_object *obj,
obj_to_connector(obj),
prop_value);
} else {
- ret = drm_atomic_set_property(state, file_priv, obj, prop, prop_value);
+ ret = drm_atomic_set_property(state, file_priv, obj, prop, prop_value, false);
if (ret)
goto out;
ret = drm_atomic_commit(state);
--
2.42.0
Reviewed-by: Simon Ser <[email protected]>
After discussing with André it seems like we missed a plane type check
here. We need to make sure FB_ID changes are only allowed on primary
planes.
Hi Andr?,
kernel test robot noticed the following build warnings:
[auto build test WARNING on drm-misc/drm-misc-next]
[also build test WARNING on drm-intel/for-linux-next drm-intel/for-linux-next-fixes drm/drm-next linus/master v6.6-rc6 next-20231017]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Andr-Almeida/drm-allow-DRM_MODE_PAGE_FLIP_ASYNC-for-atomic-commits/20231017-173047
base: git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
patch link: https://lore.kernel.org/r/20231017092837.32428-5-andrealmeid%40igalia.com
patch subject: [PATCH v7 4/6] drm: Refuse to async flip with atomic prop changes
config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20231017/[email protected]/config)
compiler: m68k-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231017/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All warnings (new ones prefixed by >>):
drivers/gpu/drm/drm_atomic_uapi.c: In function 'drm_atomic_set_property':
>> drivers/gpu/drm/drm_atomic_uapi.c:1062:41: warning: unused variable 'config' [-Wunused-variable]
1062 | struct drm_mode_config *config = &crtc->dev->mode_config;
| ^~~~~~
vim +/config +1062 drivers/gpu/drm/drm_atomic_uapi.c
1021
1022 int drm_atomic_set_property(struct drm_atomic_state *state,
1023 struct drm_file *file_priv,
1024 struct drm_mode_object *obj,
1025 struct drm_property *prop,
1026 uint64_t prop_value,
1027 bool async_flip)
1028 {
1029 struct drm_mode_object *ref;
1030 uint64_t old_val;
1031 int ret;
1032
1033 if (!drm_property_change_valid_get(prop, prop_value, &ref))
1034 return -EINVAL;
1035
1036 switch (obj->type) {
1037 case DRM_MODE_OBJECT_CONNECTOR: {
1038 struct drm_connector *connector = obj_to_connector(obj);
1039 struct drm_connector_state *connector_state;
1040
1041 connector_state = drm_atomic_get_connector_state(state, connector);
1042 if (IS_ERR(connector_state)) {
1043 ret = PTR_ERR(connector_state);
1044 break;
1045 }
1046
1047 if (async_flip) {
1048 ret = drm_atomic_connector_get_property(connector, connector_state,
1049 prop, &old_val);
1050 ret = drm_atomic_check_prop_changes(ret, old_val, prop_value, prop);
1051 break;
1052 }
1053
1054 ret = drm_atomic_connector_set_property(connector,
1055 connector_state, file_priv,
1056 prop, prop_value);
1057 break;
1058 }
1059 case DRM_MODE_OBJECT_CRTC: {
1060 struct drm_crtc *crtc = obj_to_crtc(obj);
1061 struct drm_crtc_state *crtc_state;
> 1062 struct drm_mode_config *config = &crtc->dev->mode_config;
1063
1064 crtc_state = drm_atomic_get_crtc_state(state, crtc);
1065 if (IS_ERR(crtc_state)) {
1066 ret = PTR_ERR(crtc_state);
1067 break;
1068 }
1069
1070 if (async_flip) {
1071 ret = drm_atomic_crtc_get_property(crtc, crtc_state,
1072 prop, &old_val);
1073 ret = drm_atomic_check_prop_changes(ret, old_val, prop_value, prop);
1074 break;
1075 }
1076
1077 ret = drm_atomic_crtc_set_property(crtc,
1078 crtc_state, prop, prop_value);
1079 break;
1080 }
1081 case DRM_MODE_OBJECT_PLANE: {
1082 struct drm_plane *plane = obj_to_plane(obj);
1083 struct drm_plane_state *plane_state;
1084 struct drm_mode_config *config = &plane->dev->mode_config;
1085
1086 plane_state = drm_atomic_get_plane_state(state, plane);
1087 if (IS_ERR(plane_state)) {
1088 ret = PTR_ERR(plane_state);
1089 break;
1090 }
1091
1092 if (async_flip && prop != config->prop_fb_id) {
1093 ret = drm_atomic_plane_get_property(plane, plane_state,
1094 prop, &old_val);
1095 ret = drm_atomic_check_prop_changes(ret, old_val, prop_value, prop);
1096 break;
1097 }
1098
1099 ret = drm_atomic_plane_set_property(plane,
1100 plane_state, file_priv,
1101 prop, prop_value);
1102 break;
1103 }
1104 default:
1105 drm_dbg_atomic(prop->dev, "[OBJECT:%d] has no properties\n", obj->id);
1106 ret = -EINVAL;
1107 break;
1108 }
1109
1110 drm_property_change_valid_put(prop, ref);
1111 return ret;
1112 }
1113
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
On 10/17/23 14:16, Simon Ser wrote:
> After discussing with André it seems like we missed a plane type check
> here. We need to make sure FB_ID changes are only allowed on primary
> planes.
Can you elaborate why that's needed?
--
Earthling Michel Dänzer | https://redhat.com
Libre software enthusiast | Mesa and Xwayland developer
On Sunday, October 22nd, 2023 at 12:12, Michel Dänzer <[email protected]> wrote:
> On 10/17/23 14:16, Simon Ser wrote:
>
> > After discussing with André it seems like we missed a plane type check
> > here. We need to make sure FB_ID changes are only allowed on primary
> > planes.
>
> Can you elaborate why that's needed?
Current drivers are in general not prepared to perform async page-flips
on planes other than primary. For instance I don't think i915 has logic
to perform async page-flip on an overlay plane FB_ID change.
On 10/23/23 10:27, Simon Ser wrote:
> On Sunday, October 22nd, 2023 at 12:12, Michel Dänzer <[email protected]> wrote:
>> On 10/17/23 14:16, Simon Ser wrote:
>>
>>> After discussing with André it seems like we missed a plane type check
>>> here. We need to make sure FB_ID changes are only allowed on primary
>>> planes.
>>
>> Can you elaborate why that's needed?
>
> Current drivers are in general not prepared to perform async page-flips
> on planes other than primary. For instance I don't think i915 has logic
> to perform async page-flip on an overlay plane FB_ID change.
That should be handled in the driver's atomic_check then?
Async flips of overlay planes would be useful e.g. for presenting a windowed application with tearing, while the rest of the desktop is tear-free.
--
Earthling Michel Dänzer | https://redhat.com
Libre software enthusiast | Mesa and Xwayland developer
On Monday, October 23rd, 2023 at 10:42, Michel Dänzer <[email protected]> wrote:
> On 10/23/23 10:27, Simon Ser wrote:
>
> > On Sunday, October 22nd, 2023 at 12:12, Michel Dänzer [email protected] wrote:
> >
> > > On 10/17/23 14:16, Simon Ser wrote:
> > >
> > > > After discussing with André it seems like we missed a plane type check
> > > > here. We need to make sure FB_ID changes are only allowed on primary
> > > > planes.
> > >
> > > Can you elaborate why that's needed?
> >
> > Current drivers are in general not prepared to perform async page-flips
> > on planes other than primary. For instance I don't think i915 has logic
> > to perform async page-flip on an overlay plane FB_ID change.
>
>
> That should be handled in the driver's atomic_check then?
>
> Async flips of overlay planes would be useful e.g. for presenting a windowed application with tearing, while the rest of the desktop is tear-free.
Yes, that would be useful, but requires more work. Small steps: first
expose what the legacy uAPI can do in atomic, then later extend that in
some drivers.