Skip to contentSkip to conversation

drm: Add checks for NULL drm_*_helper_funcs

PATCH Superseded Haneen Mohammed 2 participants 4 messages May 25, 2018 – May 31, 2018

Subsystems (2)
DRM drivers DRM misc

v1 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Haneen Mohammed

author
#0 lore raw

PATCH 1/1 drm: Add checks for NULL drm_*_helper_funcs

This patch add checks for NULL drm_[connector/crtc/plane]_helper_funcs pointers before derefrencing the variable to avoid NULL pointer dereference and make the helper functions as optional as possible.

Signed-off-by Haneen Mohammed <[email protected]>
---
 drivers/gpu/drm/drm_atomic_helper.c | 42 +++++++++++++++--------------
 drivers/gpu/drm/drm_probe_helper.c  | 11 ++++----
 2 files changed, 28 insertions(+), 25 deletions(-)
drivers/gpu/drm/drm_atomic_helper.c +22 -20
index c35654591c12..52092deb741d 100644
@@ -112,9 +112,9 @@ static int handle_conflicting_encoders(struct drm_atomic_state *state,
 		if (!new_conn_state->crtc)
 			continue;
 
-		if (funcs->atomic_best_encoder)
+		if (funcs && funcs->atomic_best_encoder)
 			new_encoder = funcs->atomic_best_encoder(connector, new_conn_state);
-		else if (funcs->best_encoder)
+		else if (funcs && funcs->best_encoder)
 			new_encoder = funcs->best_encoder(connector);
 		else
 			new_encoder = drm_atomic_helper_best_encoder(connector);

@@ -308,10 +308,10 @@ update_connector_routing(struct drm_atomic_state *state,
 
 	funcs = connector->helper_private;
 
-	if (funcs->atomic_best_encoder)
+	if (funcs && funcs->atomic_best_encoder)
 		new_encoder = funcs->atomic_best_encoder(connector,
 							 new_connector_state);
-	else if (funcs->best_encoder)
+	else if (funcs && funcs->best_encoder)
 		new_encoder = funcs->best_encoder(connector);
 	else
 		new_encoder = drm_atomic_helper_best_encoder(connector);

@@ -438,7 +438,7 @@ mode_fixup(struct drm_atomic_state *state)
 			continue;
 
 		funcs = crtc->helper_private;
-		if (!funcs->mode_fixup)
+		if (!funcs || !funcs->mode_fixup)
 			continue;
 
 		ret = funcs->mode_fixup(crtc, &new_crtc_state->mode,

@@ -639,7 +639,7 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
 				new_crtc_state->connectors_changed = true;
 		}
 
-		if (funcs->atomic_check)
+		if (funcs && funcs->atomic_check)
 			ret = funcs->atomic_check(connector, new_connector_state);
 		if (ret)
 			return ret;

@@ -681,7 +681,7 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
 		if (connectors_mask & BIT(i))
 			continue;
 
-		if (funcs->atomic_check)
+		if (funcs && funcs->atomic_check)
 			ret = funcs->atomic_check(connector, new_connector_state);
 		if (ret)
 			return ret;

@@ -972,14 +972,16 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
 
 
 		/* Right function depends upon target state. */
-		if (new_crtc_state->enable && funcs->prepare)
-			funcs->prepare(crtc);
-		else if (funcs->atomic_disable)
-			funcs->atomic_disable(crtc, old_crtc_state);
-		else if (funcs->disable)
-			funcs->disable(crtc);
-		else
-			funcs->dpms(crtc, DRM_MODE_DPMS_OFF);
+		if (funcs) {
+			if (new_crtc_state->enable && funcs->prepare)
+				funcs->prepare(crtc);
+			else if (funcs->atomic_disable)
+				funcs->atomic_disable(crtc, old_crtc_state);
+			else if (funcs->disable)
+				funcs->disable(crtc);
+			else
+				funcs->dpms(crtc, DRM_MODE_DPMS_OFF);
+		}
 
 		if (!(dev->irq_enabled && dev->num_crtcs))
 			continue;

@@ -1093,7 +1095,7 @@ crtc_set_mode(struct drm_device *dev, struct drm_atomic_state *old_state)
 
 		funcs = crtc->helper_private;
 
-		if (new_crtc_state->enable && funcs->mode_set_nofb) {
+		if (new_crtc_state->enable && funcs && funcs->mode_set_nofb) {
 			DRM_DEBUG_ATOMIC("modeset on [CRTC:%d:%s]\n",
 					 crtc->base.id, crtc->name);
 

@@ -1197,7 +1199,7 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
 
 		funcs = crtc->helper_private;
 
-		if (new_crtc_state->enable) {
+		if (funcs && new_crtc_state->enable) {
 			DRM_DEBUG_ATOMIC("enabling [CRTC:%d:%s]\n",
 					 crtc->base.id, crtc->name);
 

@@ -2117,7 +2119,7 @@ int drm_atomic_helper_prepare_planes(struct drm_device *dev,
 
 		funcs = plane->helper_private;
 
-		if (funcs->prepare_fb) {
+		if (funcs && funcs->prepare_fb) {
 			ret = funcs->prepare_fb(plane, new_plane_state);
 			if (ret)
 				goto fail;

@@ -2135,7 +2137,7 @@ int drm_atomic_helper_prepare_planes(struct drm_device *dev,
 
 		funcs = plane->helper_private;
 
-		if (funcs->cleanup_fb)
+		if (funcs && funcs->cleanup_fb)
 			funcs->cleanup_fb(plane, new_plane_state);
 	}
 

@@ -2412,7 +2414,7 @@ void drm_atomic_helper_cleanup_planes(struct drm_device *dev,
 
 		funcs = plane->helper_private;
 
-		if (funcs->cleanup_fb)
+		if (funcs && funcs->cleanup_fb)
 			funcs->cleanup_fb(plane, plane_state);
 	}
 }
drivers/gpu/drm/drm_probe_helper.c +6 -5
index 527743394150..42507aa7f763 100644
@@ -272,9 +272,9 @@ drm_helper_probe_detect_ctx(struct drm_connector *connector, bool force)
 retry:
 	ret = drm_modeset_lock(&connector->dev->mode_config.connection_mutex, &ctx);
 	if (!ret) {
-		if (funcs->detect_ctx)
+		if (funcs && funcs->detect_ctx)
 			ret = funcs->detect_ctx(connector, &ctx, force);
-		else if (connector->funcs->detect)
+		else if (connector->funcs && connector->funcs->detect)
 			ret = connector->funcs->detect(connector, force);
 		else
 			ret = connector_status_connected;

@@ -320,9 +320,9 @@ drm_helper_probe_detect(struct drm_connector *connector,
 	if (ret)
 		return ret;
 
-	if (funcs->detect_ctx)
+	if (funcs && funcs->detect_ctx)
 		return funcs->detect_ctx(connector, ctx, force);
-	else if (connector->funcs->detect)
+	else if (connector->funcs && connector->funcs->detect)
 		return connector->funcs->detect(connector, force);
 	else
 		return connector_status_connected;

@@ -480,7 +480,8 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
 		goto prune;
 	}
 
-	count = (*connector_funcs->get_modes)(connector);
+	if (connector_funcs && connector_funcs->get_modes)
+		count = (*connector_funcs->get_modes)(connector);
 
 	if (count == 0 && connector->status == connector_status_connected)
 		count = drm_add_modes_noedid(connector, 1024, 768);
Signature · 6 lines
2.17.0

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
#1 lore raw
Quoted 5 lines · Haneen Mohammed

This patch add checks for NULL drm_[connector/crtc/plane]_helper_funcs pointers before derefrencing the variable to avoid NULL pointer dereference and make the helper functions as optional as possible.

Signed-off-by Haneen Mohammed <[email protected]>

I started reviewing this, and then realized it's a bit a can of worms. E.g. connector->funcs->detect shouldn't be there, it's a helper callback really (but placed in the wrong function table). So connector->funcs isn't optional, but connector->funcs->detect maybe should be optional.

So I'm not sure anymore whether doing this holesale is a good idea. Do we still need this for vkms? If yes, then a more focused patch to just make the things optional that vkms does not (yet) provide might be better. Including an explanation of what exactly blows up in vkms.

Thanks, Daniel

Quoted 172 lines
---
 drivers/gpu/drm/drm_atomic_helper.c | 42 +++++++++++++++--------------
 drivers/gpu/drm/drm_probe_helper.c  | 11 ++++----
 2 files changed, 28 insertions(+), 25 deletions(-)
drivers/gpu/drm/drm_atomic_helper.c +22 -20
index c35654591c12..52092deb741d 100644
@@ -112,9 +112,9 @@ static int handle_conflicting_encoders(struct drm_atomic_state *state,
 		if (!new_conn_state->crtc)
 			continue;
 
-		if (funcs->atomic_best_encoder)
+		if (funcs && funcs->atomic_best_encoder)
 			new_encoder = funcs->atomic_best_encoder(connector, new_conn_state);
-		else if (funcs->best_encoder)
+		else if (funcs && funcs->best_encoder)
 			new_encoder = funcs->best_encoder(connector);
 		else
 			new_encoder = drm_atomic_helper_best_encoder(connector);

@@ -308,10 +308,10 @@ update_connector_routing(struct drm_atomic_state *state,
 
 	funcs = connector->helper_private;
 
-	if (funcs->atomic_best_encoder)
+	if (funcs && funcs->atomic_best_encoder)
 		new_encoder = funcs->atomic_best_encoder(connector,
 							 new_connector_state);
-	else if (funcs->best_encoder)
+	else if (funcs && funcs->best_encoder)
 		new_encoder = funcs->best_encoder(connector);
 	else
 		new_encoder = drm_atomic_helper_best_encoder(connector);

@@ -438,7 +438,7 @@ mode_fixup(struct drm_atomic_state *state)
 			continue;
 
 		funcs = crtc->helper_private;
-		if (!funcs->mode_fixup)
+		if (!funcs || !funcs->mode_fixup)
 			continue;
 
 		ret = funcs->mode_fixup(crtc, &new_crtc_state->mode,

@@ -639,7 +639,7 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
 				new_crtc_state->connectors_changed = true;
 		}
 
-		if (funcs->atomic_check)
+		if (funcs && funcs->atomic_check)
 			ret = funcs->atomic_check(connector, new_connector_state);
 		if (ret)
 			return ret;

@@ -681,7 +681,7 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
 		if (connectors_mask & BIT(i))
 			continue;
 
-		if (funcs->atomic_check)
+		if (funcs && funcs->atomic_check)
 			ret = funcs->atomic_check(connector, new_connector_state);
 		if (ret)
 			return ret;

@@ -972,14 +972,16 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
 
 
 		/* Right function depends upon target state. */
-		if (new_crtc_state->enable && funcs->prepare)
-			funcs->prepare(crtc);
-		else if (funcs->atomic_disable)
-			funcs->atomic_disable(crtc, old_crtc_state);
-		else if (funcs->disable)
-			funcs->disable(crtc);
-		else
-			funcs->dpms(crtc, DRM_MODE_DPMS_OFF);
+		if (funcs) {
+			if (new_crtc_state->enable && funcs->prepare)
+				funcs->prepare(crtc);
+			else if (funcs->atomic_disable)
+				funcs->atomic_disable(crtc, old_crtc_state);
+			else if (funcs->disable)
+				funcs->disable(crtc);
+			else
+				funcs->dpms(crtc, DRM_MODE_DPMS_OFF);
+		}
 
 		if (!(dev->irq_enabled && dev->num_crtcs))
 			continue;

@@ -1093,7 +1095,7 @@ crtc_set_mode(struct drm_device *dev, struct drm_atomic_state *old_state)
 
 		funcs = crtc->helper_private;
 
-		if (new_crtc_state->enable && funcs->mode_set_nofb) {
+		if (new_crtc_state->enable && funcs && funcs->mode_set_nofb) {
 			DRM_DEBUG_ATOMIC("modeset on [CRTC:%d:%s]\n",
 					 crtc->base.id, crtc->name);
 

@@ -1197,7 +1199,7 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
 
 		funcs = crtc->helper_private;
 
-		if (new_crtc_state->enable) {
+		if (funcs && new_crtc_state->enable) {
 			DRM_DEBUG_ATOMIC("enabling [CRTC:%d:%s]\n",
 					 crtc->base.id, crtc->name);
 

@@ -2117,7 +2119,7 @@ int drm_atomic_helper_prepare_planes(struct drm_device *dev,
 
 		funcs = plane->helper_private;
 
-		if (funcs->prepare_fb) {
+		if (funcs && funcs->prepare_fb) {
 			ret = funcs->prepare_fb(plane, new_plane_state);
 			if (ret)
 				goto fail;

@@ -2135,7 +2137,7 @@ int drm_atomic_helper_prepare_planes(struct drm_device *dev,
 
 		funcs = plane->helper_private;
 
-		if (funcs->cleanup_fb)
+		if (funcs && funcs->cleanup_fb)
 			funcs->cleanup_fb(plane, new_plane_state);
 	}
 

@@ -2412,7 +2414,7 @@ void drm_atomic_helper_cleanup_planes(struct drm_device *dev,
 
 		funcs = plane->helper_private;
 
-		if (funcs->cleanup_fb)
+		if (funcs && funcs->cleanup_fb)
 			funcs->cleanup_fb(plane, plane_state);
 	}
 }
drivers/gpu/drm/drm_probe_helper.c +6 -5
index 527743394150..42507aa7f763 100644
@@ -272,9 +272,9 @@ drm_helper_probe_detect_ctx(struct drm_connector *connector, bool force)
 retry:
 	ret = drm_modeset_lock(&connector->dev->mode_config.connection_mutex, &ctx);
 	if (!ret) {
-		if (funcs->detect_ctx)
+		if (funcs && funcs->detect_ctx)
 			ret = funcs->detect_ctx(connector, &ctx, force);
-		else if (connector->funcs->detect)
+		else if (connector->funcs && connector->funcs->detect)
 			ret = connector->funcs->detect(connector, force);
 		else
 			ret = connector_status_connected;

@@ -320,9 +320,9 @@ drm_helper_probe_detect(struct drm_connector *connector,
 	if (ret)
 		return ret;
 
-	if (funcs->detect_ctx)
+	if (funcs && funcs->detect_ctx)
 		return funcs->detect_ctx(connector, ctx, force);
-	else if (connector->funcs->detect)
+	else if (connector->funcs && connector->funcs->detect)
 		return connector->funcs->detect(connector, force);
 	else
 		return connector_status_connected;

@@ -480,7 +480,8 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
 		goto prune;
 	}
 
-	count = (*connector_funcs->get_modes)(connector);
+	if (connector_funcs && connector_funcs->get_modes)
+		count = (*connector_funcs->get_modes)(connector);
 
 	if (count == 0 && connector->status == connector_status_connected)
 		count = drm_add_modes_noedid(connector, 1024, 768);
Signature · 1 line
2.17.0
Signature · 7 lines
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
author
#2 lore raw
Quoted 12 lines · Daniel Vetter
Quoted 5 lines · Haneen Mohammed

This patch add checks for NULL drm_[connector/crtc/plane]_helper_funcs pointers before derefrencing the variable to avoid NULL pointer dereference and make the helper functions as optional as possible.

Signed-off-by Haneen Mohammed <[email protected]>

I started reviewing this, and then realized it's a bit a can of worms. E.g. connector->funcs->detect shouldn't be there, it's a helper callback really (but placed in the wrong function table). So connector->funcs isn't optional, but connector->funcs->detect maybe should be optional.

Quoted 5 lines

So I'm not sure anymore whether doing this holesale is a good idea. Do we still need this for vkms? If yes, then a more focused patch to just make the things optional that vkms does not (yet) provide might be better. Including an explanation of what exactly blows up in vkms.

hm only for running the igt tests & modetest in libdrm.

It was easier to add check for all the funcs, but I will check again the least amount of checks needed for vkms.

Thank you!
Haneen

Quoted 168 lines

Thanks, Daniel

Quoted 167 lines
---
 drivers/gpu/drm/drm_atomic_helper.c | 42 +++++++++++++++--------------
 drivers/gpu/drm/drm_probe_helper.c  | 11 ++++----
 2 files changed, 28 insertions(+), 25 deletions(-)
drivers/gpu/drm/drm_atomic_helper.c +22 -20
index c35654591c12..52092deb741d 100644
@@ -112,9 +112,9 @@ static int handle_conflicting_encoders(struct drm_atomic_state *state,
 		if (!new_conn_state->crtc)
 			continue;
 
-		if (funcs->atomic_best_encoder)
+		if (funcs && funcs->atomic_best_encoder)
 			new_encoder = funcs->atomic_best_encoder(connector, new_conn_state);
-		else if (funcs->best_encoder)
+		else if (funcs && funcs->best_encoder)
 			new_encoder = funcs->best_encoder(connector);
 		else
 			new_encoder = drm_atomic_helper_best_encoder(connector);

@@ -308,10 +308,10 @@ update_connector_routing(struct drm_atomic_state *state,
 
 	funcs = connector->helper_private;
 
-	if (funcs->atomic_best_encoder)
+	if (funcs && funcs->atomic_best_encoder)
 		new_encoder = funcs->atomic_best_encoder(connector,
 							 new_connector_state);
-	else if (funcs->best_encoder)
+	else if (funcs && funcs->best_encoder)
 		new_encoder = funcs->best_encoder(connector);
 	else
 		new_encoder = drm_atomic_helper_best_encoder(connector);

@@ -438,7 +438,7 @@ mode_fixup(struct drm_atomic_state *state)
 			continue;
 
 		funcs = crtc->helper_private;
-		if (!funcs->mode_fixup)
+		if (!funcs || !funcs->mode_fixup)
 			continue;
 
 		ret = funcs->mode_fixup(crtc, &new_crtc_state->mode,

@@ -639,7 +639,7 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
 				new_crtc_state->connectors_changed = true;
 		}
 
-		if (funcs->atomic_check)
+		if (funcs && funcs->atomic_check)
 			ret = funcs->atomic_check(connector, new_connector_state);
 		if (ret)
 			return ret;

@@ -681,7 +681,7 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
 		if (connectors_mask & BIT(i))
 			continue;
 
-		if (funcs->atomic_check)
+		if (funcs && funcs->atomic_check)
 			ret = funcs->atomic_check(connector, new_connector_state);
 		if (ret)
 			return ret;

@@ -972,14 +972,16 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
 
 
 		/* Right function depends upon target state. */
-		if (new_crtc_state->enable && funcs->prepare)
-			funcs->prepare(crtc);
-		else if (funcs->atomic_disable)
-			funcs->atomic_disable(crtc, old_crtc_state);
-		else if (funcs->disable)
-			funcs->disable(crtc);
-		else
-			funcs->dpms(crtc, DRM_MODE_DPMS_OFF);
+		if (funcs) {
+			if (new_crtc_state->enable && funcs->prepare)
+				funcs->prepare(crtc);
+			else if (funcs->atomic_disable)
+				funcs->atomic_disable(crtc, old_crtc_state);
+			else if (funcs->disable)
+				funcs->disable(crtc);
+			else
+				funcs->dpms(crtc, DRM_MODE_DPMS_OFF);
+		}
 
 		if (!(dev->irq_enabled && dev->num_crtcs))
 			continue;

@@ -1093,7 +1095,7 @@ crtc_set_mode(struct drm_device *dev, struct drm_atomic_state *old_state)
 
 		funcs = crtc->helper_private;
 
-		if (new_crtc_state->enable && funcs->mode_set_nofb) {
+		if (new_crtc_state->enable && funcs && funcs->mode_set_nofb) {
 			DRM_DEBUG_ATOMIC("modeset on [CRTC:%d:%s]\n",
 					 crtc->base.id, crtc->name);
 

@@ -1197,7 +1199,7 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
 
 		funcs = crtc->helper_private;
 
-		if (new_crtc_state->enable) {
+		if (funcs && new_crtc_state->enable) {
 			DRM_DEBUG_ATOMIC("enabling [CRTC:%d:%s]\n",
 					 crtc->base.id, crtc->name);
 

@@ -2117,7 +2119,7 @@ int drm_atomic_helper_prepare_planes(struct drm_device *dev,
 
 		funcs = plane->helper_private;
 
-		if (funcs->prepare_fb) {
+		if (funcs && funcs->prepare_fb) {
 			ret = funcs->prepare_fb(plane, new_plane_state);
 			if (ret)
 				goto fail;

@@ -2135,7 +2137,7 @@ int drm_atomic_helper_prepare_planes(struct drm_device *dev,
 
 		funcs = plane->helper_private;
 
-		if (funcs->cleanup_fb)
+		if (funcs && funcs->cleanup_fb)
 			funcs->cleanup_fb(plane, new_plane_state);
 	}
 

@@ -2412,7 +2414,7 @@ void drm_atomic_helper_cleanup_planes(struct drm_device *dev,
 
 		funcs = plane->helper_private;
 
-		if (funcs->cleanup_fb)
+		if (funcs && funcs->cleanup_fb)
 			funcs->cleanup_fb(plane, plane_state);
 	}
 }
drivers/gpu/drm/drm_probe_helper.c +6 -5
index 527743394150..42507aa7f763 100644
@@ -272,9 +272,9 @@ drm_helper_probe_detect_ctx(struct drm_connector *connector, bool force)
 retry:
 	ret = drm_modeset_lock(&connector->dev->mode_config.connection_mutex, &ctx);
 	if (!ret) {
-		if (funcs->detect_ctx)
+		if (funcs && funcs->detect_ctx)
 			ret = funcs->detect_ctx(connector, &ctx, force);
-		else if (connector->funcs->detect)
+		else if (connector->funcs && connector->funcs->detect)
 			ret = connector->funcs->detect(connector, force);
 		else
 			ret = connector_status_connected;

@@ -320,9 +320,9 @@ drm_helper_probe_detect(struct drm_connector *connector,
 	if (ret)
 		return ret;
 
-	if (funcs->detect_ctx)
+	if (funcs && funcs->detect_ctx)
 		return funcs->detect_ctx(connector, ctx, force);
-	else if (connector->funcs->detect)
+	else if (connector->funcs && connector->funcs->detect)
 		return connector->funcs->detect(connector, force);
 	else
 		return connector_status_connected;

@@ -480,7 +480,8 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
 		goto prune;
 	}
 
-	count = (*connector_funcs->get_modes)(connector);
+	if (connector_funcs && connector_funcs->get_modes)
+		count = (*connector_funcs->get_modes)(connector);
Quoted 10 lines
Quoted 5 lines
drivers/gpu/drm/drm_probe_helper.c +0 -0
 	if (count == 0 && connector->status == connector_status_connected)
 		count = drm_add_modes_noedid(connector, 1024, 768);
Signature · 1 line
2.17.0
Signature · 3 lines
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

#3 lore raw
Quoted 19 lines · Haneen Mohammed
Quoted 12 lines · Daniel Vetter
Quoted 5 lines · Haneen Mohammed

This patch add checks for NULL drm_[connector/crtc/plane]_helper_funcs pointers before derefrencing the variable to avoid NULL pointer dereference and make the helper functions as optional as possible.

Signed-off-by Haneen Mohammed <[email protected]>

I started reviewing this, and then realized it's a bit a can of worms. E.g. connector->funcs->detect shouldn't be there, it's a helper callback really (but placed in the wrong function table). So connector->funcs isn't optional, but connector->funcs->detect maybe should be optional.

Quoted 5 lines

So I'm not sure anymore whether doing this holesale is a good idea. Do we still need this for vkms? If yes, then a more focused patch to just make the things optional that vkms does not (yet) provide might be better. Including an explanation of what exactly blows up in vkms.

Quoted 4 lines

hm only for running the igt tests & modetest in libdrm.

It was easier to add check for all the funcs, but I will check again the least amount of checks needed for vkms.

To clarify what I'm looking for: I think a more specific approach like "vkms doesn have funcs for $object, and that makes sense because $reason." I think making some of the funcs optional might not make any sense at all, since any working driver needs them. vkms atm is special, since it just absolutely nothing right now (but that will change).

The goal here is to make driver writing easier by only requiring what's really needed, not mislead them by not requiring stuff that really should be there for a working driver :-)
-Daniel

Quoted 182 lines

Thank you!
Haneen

Quoted 178 lines

Thanks, Daniel

Quoted 172 lines
---
 drivers/gpu/drm/drm_atomic_helper.c | 42 +++++++++++++++--------------
 drivers/gpu/drm/drm_probe_helper.c  | 11 ++++----
 2 files changed, 28 insertions(+), 25 deletions(-)
drivers/gpu/drm/drm_atomic_helper.c +22 -20
index c35654591c12..52092deb741d 100644
@@ -112,9 +112,9 @@ static int handle_conflicting_encoders(struct drm_atomic_state *state,
 		if (!new_conn_state->crtc)
 			continue;
 
-		if (funcs->atomic_best_encoder)
+		if (funcs && funcs->atomic_best_encoder)
 			new_encoder = funcs->atomic_best_encoder(connector, new_conn_state);
-		else if (funcs->best_encoder)
+		else if (funcs && funcs->best_encoder)
 			new_encoder = funcs->best_encoder(connector);
 		else
 			new_encoder = drm_atomic_helper_best_encoder(connector);

@@ -308,10 +308,10 @@ update_connector_routing(struct drm_atomic_state *state,
 
 	funcs = connector->helper_private;
 
-	if (funcs->atomic_best_encoder)
+	if (funcs && funcs->atomic_best_encoder)
 		new_encoder = funcs->atomic_best_encoder(connector,
 							 new_connector_state);
-	else if (funcs->best_encoder)
+	else if (funcs && funcs->best_encoder)
 		new_encoder = funcs->best_encoder(connector);
 	else
 		new_encoder = drm_atomic_helper_best_encoder(connector);

@@ -438,7 +438,7 @@ mode_fixup(struct drm_atomic_state *state)
 			continue;
 
 		funcs = crtc->helper_private;
-		if (!funcs->mode_fixup)
+		if (!funcs || !funcs->mode_fixup)
 			continue;
 
 		ret = funcs->mode_fixup(crtc, &new_crtc_state->mode,

@@ -639,7 +639,7 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
 				new_crtc_state->connectors_changed = true;
 		}
 
-		if (funcs->atomic_check)
+		if (funcs && funcs->atomic_check)
 			ret = funcs->atomic_check(connector, new_connector_state);
 		if (ret)
 			return ret;

@@ -681,7 +681,7 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
 		if (connectors_mask & BIT(i))
 			continue;
 
-		if (funcs->atomic_check)
+		if (funcs && funcs->atomic_check)
 			ret = funcs->atomic_check(connector, new_connector_state);
 		if (ret)
 			return ret;

@@ -972,14 +972,16 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
 
 
 		/* Right function depends upon target state. */
-		if (new_crtc_state->enable && funcs->prepare)
-			funcs->prepare(crtc);
-		else if (funcs->atomic_disable)
-			funcs->atomic_disable(crtc, old_crtc_state);
-		else if (funcs->disable)
-			funcs->disable(crtc);
-		else
-			funcs->dpms(crtc, DRM_MODE_DPMS_OFF);
+		if (funcs) {
+			if (new_crtc_state->enable && funcs->prepare)
+				funcs->prepare(crtc);
+			else if (funcs->atomic_disable)
+				funcs->atomic_disable(crtc, old_crtc_state);
+			else if (funcs->disable)
+				funcs->disable(crtc);
+			else
+				funcs->dpms(crtc, DRM_MODE_DPMS_OFF);
+		}
 
 		if (!(dev->irq_enabled && dev->num_crtcs))
 			continue;

@@ -1093,7 +1095,7 @@ crtc_set_mode(struct drm_device *dev, struct drm_atomic_state *old_state)
 
 		funcs = crtc->helper_private;
 
-		if (new_crtc_state->enable && funcs->mode_set_nofb) {
+		if (new_crtc_state->enable && funcs && funcs->mode_set_nofb) {
 			DRM_DEBUG_ATOMIC("modeset on [CRTC:%d:%s]\n",
 					 crtc->base.id, crtc->name);
 

@@ -1197,7 +1199,7 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
 
 		funcs = crtc->helper_private;
 
-		if (new_crtc_state->enable) {
+		if (funcs && new_crtc_state->enable) {
 			DRM_DEBUG_ATOMIC("enabling [CRTC:%d:%s]\n",
 					 crtc->base.id, crtc->name);
 

@@ -2117,7 +2119,7 @@ int drm_atomic_helper_prepare_planes(struct drm_device *dev,
 
 		funcs = plane->helper_private;
 
-		if (funcs->prepare_fb) {
+		if (funcs && funcs->prepare_fb) {
 			ret = funcs->prepare_fb(plane, new_plane_state);
 			if (ret)
 				goto fail;

@@ -2135,7 +2137,7 @@ int drm_atomic_helper_prepare_planes(struct drm_device *dev,
 
 		funcs = plane->helper_private;
 
-		if (funcs->cleanup_fb)
+		if (funcs && funcs->cleanup_fb)
 			funcs->cleanup_fb(plane, new_plane_state);
 	}
 

@@ -2412,7 +2414,7 @@ void drm_atomic_helper_cleanup_planes(struct drm_device *dev,
 
 		funcs = plane->helper_private;
 
-		if (funcs->cleanup_fb)
+		if (funcs && funcs->cleanup_fb)
 			funcs->cleanup_fb(plane, plane_state);
 	}
 }
drivers/gpu/drm/drm_probe_helper.c +6 -5
index 527743394150..42507aa7f763 100644
@@ -272,9 +272,9 @@ drm_helper_probe_detect_ctx(struct drm_connector *connector, bool force)
 retry:
 	ret = drm_modeset_lock(&connector->dev->mode_config.connection_mutex, &ctx);
 	if (!ret) {
-		if (funcs->detect_ctx)
+		if (funcs && funcs->detect_ctx)
 			ret = funcs->detect_ctx(connector, &ctx, force);
-		else if (connector->funcs->detect)
+		else if (connector->funcs && connector->funcs->detect)
 			ret = connector->funcs->detect(connector, force);
 		else
 			ret = connector_status_connected;

@@ -320,9 +320,9 @@ drm_helper_probe_detect(struct drm_connector *connector,
 	if (ret)
 		return ret;
 
-	if (funcs->detect_ctx)
+	if (funcs && funcs->detect_ctx)
 		return funcs->detect_ctx(connector, ctx, force);
-	else if (connector->funcs->detect)
+	else if (connector->funcs && connector->funcs->detect)
 		return connector->funcs->detect(connector, force);
 	else
 		return connector_status_connected;

@@ -480,7 +480,8 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
 		goto prune;
 	}
 
-	count = (*connector_funcs->get_modes)(connector);
+	if (connector_funcs && connector_funcs->get_modes)
+		count = (*connector_funcs->get_modes)(connector);
 
 	if (count == 0 && connector->status == connector_status_connected)
 		count = drm_add_modes_noedid(connector, 1024, 768);
Signature · 1 line
2.17.0
Signature · 3 lines
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
Signature · 7 lines
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel