| Message ID | 20230901174557.3617839-2-mwalle@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3,1/2] drm/mediatek: fix kernel oops if no crtc is found | expand |
Il 01/09/23 19:45, Michael Walle ha scritto: > mtk_drm_find_possible_crtc_by_comp() assumed that the main path will > always have the CRTC with id 0, the ext id 1 and the third id 2. This > is only true if the paths are all available. But paths are optional (see > also comment in mtk_drm_kms_init()), e.g. the main path might not be > enabled or available at all. Then the CRTC IDs will shift one up, e.g. > ext will be 0 and the third path will be 1. > > To fix that, dynamically calculate the IDs by the presence of the paths. > > Fixes: 5aa8e7647676 ("drm/mediatek: dpi/dsi: Change the getting possible_crtc way") > Suggested-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> > Signed-off-by: Michael Walle <mwalle@kernel.org> > Reviewed-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> > Tested-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> > --- > v3: > - use data instead of priv_n->data > - fixed typos > - collected Rb and Tb tags > v2: > - iterate over all_drm_private[] to get any vdosys > - new check if a path is available > --- > drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c | 72 +++++++++++++++++---- > 1 file changed, 58 insertions(+), 14 deletions(-) > > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c > index 771f4e173353..c00f4669cc50 100644 > --- a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c > +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c > @@ -507,6 +507,27 @@ static bool mtk_drm_find_comp_in_ddp(struct device *dev, > return false; > } > > +static bool mtk_ddp_path_available(const unsigned int *path, > + unsigned int path_len, > + struct device_node **comp_node) > +{ > + unsigned int i; > + > + if (!path) > + return false; > + > + for (i = 0U; i < path_len; i++) { > + /* OVL_ADAPTOR doesn't have a device node */ > + if (path[i] == DDP_COMPONENT_DRM_OVL_ADAPTOR) > + continue; > + > + if (!comp_node[path[i]]) > + return false; > + } > + > + return true; > +} > + > int mtk_ddp_comp_get_id(struct device_node *node, > enum mtk_ddp_comp_type comp_type) > { > @@ -526,21 +547,44 @@ 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; > - > - if (mtk_drm_find_comp_in_ddp(dev, private->data->main_path, private->data->main_len, > - private->ddp_comp)) > - ret = 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); > - else if (mtk_drm_find_comp_in_ddp(dev, private->data->third_path, > - private->data->third_len, private->ddp_comp)) > - ret = BIT(2); > - else > - DRM_INFO("Failed to find comp in ddp table\n"); > + const struct mtk_mmsys_driver_data *data; > + struct mtk_drm_private *priv_n; > + int i = 0, j; > + > + for (j = 0; j < private->data->mmsys_dev_num; j++) { > + priv_n = private->all_drm_private[j]; > + data = priv_n->data; > + > + if (mtk_ddp_path_available(data->main_path, data->main_len, > + priv_n->comp_node)) { > + if (mtk_drm_find_comp_in_ddp(dev, data->main_path, > + data->main_len, > + priv_n->ddp_comp)) > + return BIT(i); > + i++; > + } > + > + if (mtk_ddp_path_available(data->ext_path, data->ext_len, > + priv_n->comp_node)) { > + if (mtk_drm_find_comp_in_ddp(dev, data->ext_path, > + data->ext_len, > + priv_n->ddp_comp)) > + return BIT(i); > + i++; > + } > + > + if (mtk_ddp_path_available(data->third_path, data->third_len, > + priv_n->comp_node)) { > + if (mtk_drm_find_comp_in_ddp(dev, data->third_path, > + data->third_len, > + priv_n->ddp_comp)) > + return BIT(i); > + i++; > + } > + } > > - return ret; > + DRM_INFO("Failed to find comp in ddp table\n"); > + return 0; At this point, I think that it would be sane to change this function to return a signed type, so that we can return -ENOENT if we couldn't find any DDP path (so, if we couldn't find any possible crtc). This function is called only twice, once in DPI, once in DSI - so the change should also be pretty straightforward to do. Cheers, Angelo > } > > int mtk_ddp_comp_init(struct device_node *node, struct mtk_ddp_comp *comp,
> At this point, I think that it would be sane to change this function to > return a signed type, so that we can return -ENOENT if we couldn't find > any DDP path (so, if we couldn't find any possible crtc). Fair enough, but should it be part of the fixes commit or a different one? -michael
Il 05/09/23 09:58, Michael Walle ha scritto: >> At this point, I think that it would be sane to change this function to >> return a signed type, so that we can return -ENOENT if we couldn't find >> any DDP path (so, if we couldn't find any possible crtc). > > Fair enough, but should it be part of the fixes commit or a different one? > > -michael I would say that this should be part of the Fixes commit: after all, you're fixing a case in which the possible_crtcs calculation *may fail*, so the error handling for this possible failure is, indeed, one part of the fix :-) Angelo
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c index 771f4e173353..c00f4669cc50 100644 --- a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c @@ -507,6 +507,27 @@ static bool mtk_drm_find_comp_in_ddp(struct device *dev, return false; } +static bool mtk_ddp_path_available(const unsigned int *path, + unsigned int path_len, + struct device_node **comp_node) +{ + unsigned int i; + + if (!path) + return false; + + for (i = 0U; i < path_len; i++) { + /* OVL_ADAPTOR doesn't have a device node */ + if (path[i] == DDP_COMPONENT_DRM_OVL_ADAPTOR) + continue; + + if (!comp_node[path[i]]) + return false; + } + + return true; +} + int mtk_ddp_comp_get_id(struct device_node *node, enum mtk_ddp_comp_type comp_type) { @@ -526,21 +547,44 @@ 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; - - if (mtk_drm_find_comp_in_ddp(dev, private->data->main_path, private->data->main_len, - private->ddp_comp)) - ret = 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); - else if (mtk_drm_find_comp_in_ddp(dev, private->data->third_path, - private->data->third_len, private->ddp_comp)) - ret = BIT(2); - else - DRM_INFO("Failed to find comp in ddp table\n"); + const struct mtk_mmsys_driver_data *data; + struct mtk_drm_private *priv_n; + int i = 0, j; + + for (j = 0; j < private->data->mmsys_dev_num; j++) { + priv_n = private->all_drm_private[j]; + data = priv_n->data; + + if (mtk_ddp_path_available(data->main_path, data->main_len, + priv_n->comp_node)) { + if (mtk_drm_find_comp_in_ddp(dev, data->main_path, + data->main_len, + priv_n->ddp_comp)) + return BIT(i); + i++; + } + + if (mtk_ddp_path_available(data->ext_path, data->ext_len, + priv_n->comp_node)) { + if (mtk_drm_find_comp_in_ddp(dev, data->ext_path, + data->ext_len, + priv_n->ddp_comp)) + return BIT(i); + i++; + } + + if (mtk_ddp_path_available(data->third_path, data->third_len, + priv_n->comp_node)) { + if (mtk_drm_find_comp_in_ddp(dev, data->third_path, + data->third_len, + priv_n->ddp_comp)) + return BIT(i); + i++; + } + } - return ret; + DRM_INFO("Failed to find comp in ddp table\n"); + return 0; } int mtk_ddp_comp_init(struct device_node *node, struct mtk_ddp_comp *comp,
mtk_drm_find_possible_crtc_by_comp() assumed that the main path will always have the CRTC with id 0, the ext id 1 and the third id 2. This is only true if the paths are all available. But paths are optional (see also comment in mtk_drm_kms_init()), e.g. the main path might not be enabled or available at all. Then the CRTC IDs will shift one up, e.g. ext will be 0 and the third path will be 1. To fix that, dynamically calculate the IDs by the presence of the paths. Fixes: 5aa8e7647676 ("drm/mediatek: dpi/dsi: Change the getting possible_crtc way") Suggested-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> Signed-off-by: Michael Walle <mwalle@kernel.org> Reviewed-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> Tested-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> --- v3: - use data instead of priv_n->data - fixed typos - collected Rb and Tb tags v2: - iterate over all_drm_private[] to get any vdosys - new check if a path is available --- drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c | 72 +++++++++++++++++---- 1 file changed, 58 insertions(+), 14 deletions(-)