| Message ID | 20231017092837.32428-5-andrealmeid@igalia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: Add support for atomic async page-flip | expand |
Reviewed-by: Simon Ser <contact@emersion.fr>
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/202310172311.kgvIGcqy-lkp@intel.com/config) compiler: m68k-linux-gcc (GCC) 13.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231017/202310172311.kgvIGcqy-lkp@intel.com/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 <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202310172311.kgvIGcqy-lkp@intel.com/ 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
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?
On Sunday, October 22nd, 2023 at 12:12, Michel Dänzer <michel.daenzer@mailbox.org> 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 <michel.daenzer@mailbox.org> 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.
On Monday, October 23rd, 2023 at 10:42, Michel Dänzer <michel.daenzer@mailbox.org> wrote: > On 10/23/23 10:27, Simon Ser wrote: > > > On Sunday, October 22nd, 2023 at 12:12, Michel Dänzer michel.daenzer@mailbox.org 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.
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);
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 <andrealmeid@igalia.com> --- 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(-)