[v7,4/6] drm: Refuse to async flip with atomic prop changes

  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(-)
  

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.