| Message ID | 20240201125304.218467-2-angelogioacchino.delregno@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/mediatek: Fixes for DDP component search/destroy | expand |
Hi, Angelo: On Thu, 2024-02-01 at 13:53 +0100, AngeloGioacchino Del Regno wrote: > Finding a possible CRTC by DDP component is done by first checking > static routes in three paths (main, external, third/extra path) and > then, if not found, we check for dynamic connection on a per-route > basis because, for example, on some SoCs the main route may output > to either a DSI display or DisplayPort and this is finally done by > assigning a CRTC mask to `possible_crtcs`, found with function > mtk_drm_find_comp_in_ddp_conn_path(): being that a mask the possible > values are BIT(x) and, if no CRTC is possible, zero. > > Problem is, both mtk_drm_find_possible_crtc_by_comp() and the > aforementioned function are trying to return a negative error value > (but it's unsigned int!) if no CRTC was found, which is wrong for > multiple obvious reasons. I does not find anywhere to return negative value. So this patch just like a refine patch not bug fix. Regards, CK > > Cleanup both functions, so that: > - mtk_drm_find_comp_in_ddp_conn_path() returns a signed integer > with a negative number for error, or a bit/bitmask of the found > possible CRTC; and > - mtk_drm_find_possible_crtc_by_comp() always returns either a > bitmask of the possible CRTC, or zero if none available. > > Fixes: 01389b324c97 ("drm/mediatek: Add connector dynamic selection > capability") > Signed-off-by: AngeloGioacchino Del Regno < > angelogioacchino.delregno@collabora.com> > --- > drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c | 44 ++++++++++--------- > -- > 1 file changed, 21 insertions(+), 23 deletions(-) > > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c > b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c > index a9b5a21cde2d..c13359eeb3cd 100644 > --- a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c > +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c > @@ -513,29 +513,25 @@ static bool mtk_drm_find_comp_in_ddp(struct > device *dev, > return false; > } > > -static unsigned int mtk_drm_find_comp_in_ddp_conn_path(struct device > *dev, > - const struct > mtk_drm_route *routes, > - unsigned int > num_routes, > - struct > mtk_ddp_comp *ddp_comp) > +static int mtk_drm_find_comp_in_ddp_conn_path(struct device *dev, > + const struct > mtk_drm_route *routes, > + unsigned int num_routes, > + struct mtk_ddp_comp > *ddp_comp) > { > - int ret; > - unsigned int i; > + int i; > > - if (!routes) { > - ret = -EINVAL; > - goto err; > + if (!routes || !num_routes) { > + DRM_ERROR("No connection routes specified!\n"); > + return -EINVAL; > } > > for (i = 0; i < num_routes; i++) > if (dev == ddp_comp[routes[i].route_ddp].dev) > return BIT(routes[i].crtc_id); > > - ret = -ENODEV; > -err: > - > - DRM_INFO("Failed to find comp in ddp table, ret = %d\n", ret); > + DRM_ERROR("Failed to find component in ddp table\n"); > > - return 0; > + return -ENODEV; > } > > int mtk_ddp_comp_get_id(struct device_node *node, > @@ -557,22 +553,24 @@ unsigned int > mtk_drm_find_possible_crtc_by_comp(struct drm_device *drm, > struct device *dev) > { > struct mtk_drm_private *private = drm->dev_private; > - unsigned int ret = 0; > + int ret; > > if (mtk_drm_find_comp_in_ddp(dev, private->data->main_path, > private->data->main_len, > private->ddp_comp)) > - ret = BIT(0); > + return BIT(0); > else if (mtk_drm_find_comp_in_ddp(dev, private->data->ext_path, > private->data->ext_len, > private->ddp_comp)) > - ret = BIT(1); > + return BIT(1); > else if (mtk_drm_find_comp_in_ddp(dev, private->data- > >third_path, > private->data->third_len, > private->ddp_comp)) > - ret = BIT(2); > - else > - ret = mtk_drm_find_comp_in_ddp_conn_path(dev, > - private->data- > >conn_routes, > - private->data- > >num_conn_routes, > - private- > >ddp_comp); > + return BIT(2); > + > + ret = mtk_drm_find_comp_in_ddp_conn_path(dev, private->data- > >conn_routes, > + private->data- > >num_conn_routes, > + private->ddp_comp); > + /* No CRTC is available: return a zero mask */ > + if (ret < 0) > + return 0; > > return ret; > }
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c index a9b5a21cde2d..c13359eeb3cd 100644 --- a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c @@ -513,29 +513,25 @@ static bool mtk_drm_find_comp_in_ddp(struct device *dev, return false; } -static unsigned int mtk_drm_find_comp_in_ddp_conn_path(struct device *dev, - const struct mtk_drm_route *routes, - unsigned int num_routes, - struct mtk_ddp_comp *ddp_comp) +static int mtk_drm_find_comp_in_ddp_conn_path(struct device *dev, + const struct mtk_drm_route *routes, + unsigned int num_routes, + struct mtk_ddp_comp *ddp_comp) { - int ret; - unsigned int i; + int i; - if (!routes) { - ret = -EINVAL; - goto err; + if (!routes || !num_routes) { + DRM_ERROR("No connection routes specified!\n"); + return -EINVAL; } for (i = 0; i < num_routes; i++) if (dev == ddp_comp[routes[i].route_ddp].dev) return BIT(routes[i].crtc_id); - ret = -ENODEV; -err: - - DRM_INFO("Failed to find comp in ddp table, ret = %d\n", ret); + DRM_ERROR("Failed to find component in ddp table\n"); - return 0; + return -ENODEV; } int mtk_ddp_comp_get_id(struct device_node *node, @@ -557,22 +553,24 @@ unsigned int mtk_drm_find_possible_crtc_by_comp(struct drm_device *drm, struct device *dev) { struct mtk_drm_private *private = drm->dev_private; - unsigned int ret = 0; + int ret; if (mtk_drm_find_comp_in_ddp(dev, private->data->main_path, private->data->main_len, private->ddp_comp)) - ret = BIT(0); + return BIT(0); else if (mtk_drm_find_comp_in_ddp(dev, private->data->ext_path, private->data->ext_len, private->ddp_comp)) - ret = BIT(1); + return BIT(1); else if (mtk_drm_find_comp_in_ddp(dev, private->data->third_path, private->data->third_len, private->ddp_comp)) - ret = BIT(2); - else - ret = mtk_drm_find_comp_in_ddp_conn_path(dev, - private->data->conn_routes, - private->data->num_conn_routes, - private->ddp_comp); + return BIT(2); + + ret = mtk_drm_find_comp_in_ddp_conn_path(dev, private->data->conn_routes, + private->data->num_conn_routes, + private->ddp_comp); + /* No CRTC is available: return a zero mask */ + if (ret < 0) + return 0; return ret; }
Finding a possible CRTC by DDP component is done by first checking static routes in three paths (main, external, third/extra path) and then, if not found, we check for dynamic connection on a per-route basis because, for example, on some SoCs the main route may output to either a DSI display or DisplayPort and this is finally done by assigning a CRTC mask to `possible_crtcs`, found with function mtk_drm_find_comp_in_ddp_conn_path(): being that a mask the possible values are BIT(x) and, if no CRTC is possible, zero. Problem is, both mtk_drm_find_possible_crtc_by_comp() and the aforementioned function are trying to return a negative error value (but it's unsigned int!) if no CRTC was found, which is wrong for multiple obvious reasons. Cleanup both functions, so that: - mtk_drm_find_comp_in_ddp_conn_path() returns a signed integer with a negative number for error, or a bit/bitmask of the found possible CRTC; and - mtk_drm_find_possible_crtc_by_comp() always returns either a bitmask of the possible CRTC, or zero if none available. Fixes: 01389b324c97 ("drm/mediatek: Add connector dynamic selection capability") Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> --- drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c | 44 ++++++++++----------- 1 file changed, 21 insertions(+), 23 deletions(-)