| Message ID | 20230905084922.3908121-2-mwalle@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4,1/2] drm/mediatek: fix kernel oops if no crtc is found | expand |
Il 05/09/23 10:49, 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. > > While at it, make the return code a signed one and return -ENOENT if no > path is found and handle the error in the callers. > > 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> Perfect! Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> > --- > v4: > - return -ENOENT if mtk_drm_find_possible_crtc_by_comp() doesn't find > any path > 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_dpi.c | 5 +- > drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c | 75 ++++++++++++++++----- > drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h | 3 +- > drivers/gpu/drm/mediatek/mtk_dsi.c | 5 +- > 4 files changed, 68 insertions(+), 20 deletions(-) > > diff --git a/drivers/gpu/drm/mediatek/mtk_dpi.c b/drivers/gpu/drm/mediatek/mtk_dpi.c > index 2f931e4e2b60..f9250f7ee706 100644 > --- a/drivers/gpu/drm/mediatek/mtk_dpi.c > +++ b/drivers/gpu/drm/mediatek/mtk_dpi.c > @@ -796,7 +796,10 @@ static int mtk_dpi_bind(struct device *dev, struct device *master, void *data) > return ret; > } > > - dpi->encoder.possible_crtcs = mtk_drm_find_possible_crtc_by_comp(drm_dev, dpi->dev); > + ret = mtk_drm_find_possible_crtc_by_comp(drm_dev, dpi->dev); > + if (ret < 0) > + goto err_cleanup; > + dpi->encoder.possible_crtcs = ret; > > ret = drm_bridge_attach(&dpi->encoder, &dpi->bridge, NULL, > DRM_BRIDGE_ATTACH_NO_CONNECTOR); > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c > index 771f4e173353..83ae75ecd858 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) > { > @@ -522,25 +543,47 @@ int mtk_ddp_comp_get_id(struct device_node *node, > return -EINVAL; > } > > -unsigned int mtk_drm_find_possible_crtc_by_comp(struct drm_device *drm, > - struct device *dev) > +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 -ENOENT; > } > > int mtk_ddp_comp_init(struct device_node *node, struct mtk_ddp_comp *comp, > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h > index febcaeef16a1..6a95df72de0a 100644 > --- a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h > +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h > @@ -277,8 +277,7 @@ static inline bool mtk_ddp_comp_disconnect(struct mtk_ddp_comp *comp, struct dev > > int mtk_ddp_comp_get_id(struct device_node *node, > enum mtk_ddp_comp_type comp_type); > -unsigned int mtk_drm_find_possible_crtc_by_comp(struct drm_device *drm, > - struct device *dev); > +int mtk_drm_find_possible_crtc_by_comp(struct drm_device *drm, struct device *dev); > int mtk_ddp_comp_init(struct device_node *comp_node, struct mtk_ddp_comp *comp, > unsigned int comp_id); > enum mtk_ddp_comp_type mtk_ddp_comp_get_type(unsigned int comp_id); > diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c > index d8bfc2cce54d..d67e5c61a9b9 100644 > --- a/drivers/gpu/drm/mediatek/mtk_dsi.c > +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c > @@ -843,7 +843,10 @@ static int mtk_dsi_encoder_init(struct drm_device *drm, struct mtk_dsi *dsi) > return ret; > } > > - dsi->encoder.possible_crtcs = mtk_drm_find_possible_crtc_by_comp(drm, dsi->host.dev); > + ret = mtk_drm_find_possible_crtc_by_comp(drm, dsi->host.dev); > + if (ret < 0) > + goto err_cleanup_encoder; > + dsi->encoder.possible_crtcs = ret; > > ret = drm_bridge_attach(&dsi->encoder, &dsi->bridge, NULL, > DRM_BRIDGE_ATTACH_NO_CONNECTOR);
Hi, > 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. > > While at it, make the return code a signed one and return -ENOENT if no > path is found and handle the error in the callers. > > 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> Is there anything wrong with these two patches? They are now lingering around for more than two months. Unfortunately, patch 2/2 won't apply anymore because of commit 01389b324c97 ("drm/mediatek: Add connector dynamic selection capability). And I'm a bit puzzled for what the crtc_id is used there because I guess it will have the same problem this patch fixes. -michael > --- > v4: > - return -ENOENT if mtk_drm_find_possible_crtc_by_comp() doesn't find > any path > 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_dpi.c | 5 +- > drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c | 75 ++++++++++++++++----- > drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h | 3 +- > drivers/gpu/drm/mediatek/mtk_dsi.c | 5 +- > 4 files changed, 68 insertions(+), 20 deletions(-) > > diff --git a/drivers/gpu/drm/mediatek/mtk_dpi.c > b/drivers/gpu/drm/mediatek/mtk_dpi.c > index 2f931e4e2b60..f9250f7ee706 100644 > --- a/drivers/gpu/drm/mediatek/mtk_dpi.c > +++ b/drivers/gpu/drm/mediatek/mtk_dpi.c > @@ -796,7 +796,10 @@ static int mtk_dpi_bind(struct device *dev, struct > device *master, void *data) > return ret; > } > > - dpi->encoder.possible_crtcs = > mtk_drm_find_possible_crtc_by_comp(drm_dev, dpi->dev); > + ret = mtk_drm_find_possible_crtc_by_comp(drm_dev, dpi->dev); > + if (ret < 0) > + goto err_cleanup; > + dpi->encoder.possible_crtcs = ret; > > ret = drm_bridge_attach(&dpi->encoder, &dpi->bridge, NULL, > DRM_BRIDGE_ATTACH_NO_CONNECTOR); > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c > b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c > index 771f4e173353..83ae75ecd858 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) > { > @@ -522,25 +543,47 @@ int mtk_ddp_comp_get_id(struct device_node *node, > return -EINVAL; > } > > -unsigned int mtk_drm_find_possible_crtc_by_comp(struct drm_device > *drm, > - struct device *dev) > +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 -ENOENT; > } > > int mtk_ddp_comp_init(struct device_node *node, struct mtk_ddp_comp > *comp, > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h > b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h > index febcaeef16a1..6a95df72de0a 100644 > --- a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h > +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h > @@ -277,8 +277,7 @@ static inline bool mtk_ddp_comp_disconnect(struct > mtk_ddp_comp *comp, struct dev > > int mtk_ddp_comp_get_id(struct device_node *node, > enum mtk_ddp_comp_type comp_type); > -unsigned int mtk_drm_find_possible_crtc_by_comp(struct drm_device > *drm, > - struct device *dev); > +int mtk_drm_find_possible_crtc_by_comp(struct drm_device *drm, struct > device *dev); > int mtk_ddp_comp_init(struct device_node *comp_node, struct > mtk_ddp_comp *comp, > unsigned int comp_id); > enum mtk_ddp_comp_type mtk_ddp_comp_get_type(unsigned int comp_id); > diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c > b/drivers/gpu/drm/mediatek/mtk_dsi.c > index d8bfc2cce54d..d67e5c61a9b9 100644 > --- a/drivers/gpu/drm/mediatek/mtk_dsi.c > +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c > @@ -843,7 +843,10 @@ static int mtk_dsi_encoder_init(struct drm_device > *drm, struct mtk_dsi *dsi) > return ret; > } > > - dsi->encoder.possible_crtcs = mtk_drm_find_possible_crtc_by_comp(drm, > dsi->host.dev); > + ret = mtk_drm_find_possible_crtc_by_comp(drm, dsi->host.dev); > + if (ret < 0) > + goto err_cleanup_encoder; > + dsi->encoder.possible_crtcs = ret; > > ret = drm_bridge_attach(&dsi->encoder, &dsi->bridge, NULL, > DRM_BRIDGE_ATTACH_NO_CONNECTOR);
Hi, Michael: Michael Walle <mwalle@kernel.org> 於 2023年11月21日 週二 下午10:44寫道： > > Hi, > > > 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. > > > > While at it, make the return code a signed one and return -ENOENT if no > > path is found and handle the error in the callers. > > > > 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> > > Is there anything wrong with these two patches? They are now lingering > around for more than two months. > > Unfortunately, patch 2/2 won't apply anymore because of commit > 01389b324c97 ("drm/mediatek: Add connector dynamic selection > capability). And I'm a bit puzzled for what the crtc_id is used > there because I guess it will have the same problem this patch > fixes. Please base on the latest kernel version to fix. Regards, Chun-Kuang. > > -michael > > > --- > > v4: > > - return -ENOENT if mtk_drm_find_possible_crtc_by_comp() doesn't find > > any path > > 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_dpi.c | 5 +- > > drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c | 75 ++++++++++++++++----- > > drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h | 3 +- > > drivers/gpu/drm/mediatek/mtk_dsi.c | 5 +- > > 4 files changed, 68 insertions(+), 20 deletions(-) > > > > diff --git a/drivers/gpu/drm/mediatek/mtk_dpi.c > > b/drivers/gpu/drm/mediatek/mtk_dpi.c > > index 2f931e4e2b60..f9250f7ee706 100644 > > --- a/drivers/gpu/drm/mediatek/mtk_dpi.c > > +++ b/drivers/gpu/drm/mediatek/mtk_dpi.c > > @@ -796,7 +796,10 @@ static int mtk_dpi_bind(struct device *dev, struct > > device *master, void *data) > > return ret; > > } > > > > - dpi->encoder.possible_crtcs = > > mtk_drm_find_possible_crtc_by_comp(drm_dev, dpi->dev); > > + ret = mtk_drm_find_possible_crtc_by_comp(drm_dev, dpi->dev); > > + if (ret < 0) > > + goto err_cleanup; > > + dpi->encoder.possible_crtcs = ret; > > > > ret = drm_bridge_attach(&dpi->encoder, &dpi->bridge, NULL, > > DRM_BRIDGE_ATTACH_NO_CONNECTOR); > > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c > > b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c > > index 771f4e173353..83ae75ecd858 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) > > { > > @@ -522,25 +543,47 @@ int mtk_ddp_comp_get_id(struct device_node *node, > > return -EINVAL; > > } > > > > -unsigned int mtk_drm_find_possible_crtc_by_comp(struct drm_device > > *drm, > > - struct device *dev) > > +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 -ENOENT; > > } > > > > int mtk_ddp_comp_init(struct device_node *node, struct mtk_ddp_comp > > *comp, > > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h > > b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h > > index febcaeef16a1..6a95df72de0a 100644 > > --- a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h > > +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h > > @@ -277,8 +277,7 @@ static inline bool mtk_ddp_comp_disconnect(struct > > mtk_ddp_comp *comp, struct dev > > > > int mtk_ddp_comp_get_id(struct device_node *node, > > enum mtk_ddp_comp_type comp_type); > > -unsigned int mtk_drm_find_possible_crtc_by_comp(struct drm_device > > *drm, > > - struct device *dev); > > +int mtk_drm_find_possible_crtc_by_comp(struct drm_device *drm, struct > > device *dev); > > int mtk_ddp_comp_init(struct device_node *comp_node, struct > > mtk_ddp_comp *comp, > > unsigned int comp_id); > > enum mtk_ddp_comp_type mtk_ddp_comp_get_type(unsigned int comp_id); > > diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c > > b/drivers/gpu/drm/mediatek/mtk_dsi.c > > index d8bfc2cce54d..d67e5c61a9b9 100644 > > --- a/drivers/gpu/drm/mediatek/mtk_dsi.c > > +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c > > @@ -843,7 +843,10 @@ static int mtk_dsi_encoder_init(struct drm_device > > *drm, struct mtk_dsi *dsi) > > return ret; > > } > > > > - dsi->encoder.possible_crtcs = mtk_drm_find_possible_crtc_by_comp(drm, > > dsi->host.dev); > > + ret = mtk_drm_find_possible_crtc_by_comp(drm, dsi->host.dev); > > + if (ret < 0) > > + goto err_cleanup_encoder; > > + dsi->encoder.possible_crtcs = ret; > > > > ret = drm_bridge_attach(&dsi->encoder, &dsi->bridge, NULL, > > DRM_BRIDGE_ATTACH_NO_CONNECTOR);
Hi, >> > 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. >> > >> > While at it, make the return code a signed one and return -ENOENT if no >> > path is found and handle the error in the callers. >> > >> > 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> >> >> Is there anything wrong with these two patches? They are now lingering >> around for more than two months. >> >> Unfortunately, patch 2/2 won't apply anymore because of commit >> 01389b324c97 ("drm/mediatek: Add connector dynamic selection >> capability). And I'm a bit puzzled for what the crtc_id is used >> there because I guess it will have the same problem this patch >> fixes. > > Please base on the latest kernel version to fix. Of course, but the question is how to fix it. Maybe Jason-JH.Lin can help here. In any case, please pick patch 1/2, it's independent of the second patch and should still apply. -michael
diff --git a/drivers/gpu/drm/mediatek/mtk_dpi.c b/drivers/gpu/drm/mediatek/mtk_dpi.c index 2f931e4e2b60..f9250f7ee706 100644 --- a/drivers/gpu/drm/mediatek/mtk_dpi.c +++ b/drivers/gpu/drm/mediatek/mtk_dpi.c @@ -796,7 +796,10 @@ static int mtk_dpi_bind(struct device *dev, struct device *master, void *data) return ret; } - dpi->encoder.possible_crtcs = mtk_drm_find_possible_crtc_by_comp(drm_dev, dpi->dev); + ret = mtk_drm_find_possible_crtc_by_comp(drm_dev, dpi->dev); + if (ret < 0) + goto err_cleanup; + dpi->encoder.possible_crtcs = ret; ret = drm_bridge_attach(&dpi->encoder, &dpi->bridge, NULL, DRM_BRIDGE_ATTACH_NO_CONNECTOR); diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c index 771f4e173353..83ae75ecd858 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) { @@ -522,25 +543,47 @@ int mtk_ddp_comp_get_id(struct device_node *node, return -EINVAL; } -unsigned int mtk_drm_find_possible_crtc_by_comp(struct drm_device *drm, - struct device *dev) +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 -ENOENT; } int mtk_ddp_comp_init(struct device_node *node, struct mtk_ddp_comp *comp, diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h index febcaeef16a1..6a95df72de0a 100644 --- a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h @@ -277,8 +277,7 @@ static inline bool mtk_ddp_comp_disconnect(struct mtk_ddp_comp *comp, struct dev int mtk_ddp_comp_get_id(struct device_node *node, enum mtk_ddp_comp_type comp_type); -unsigned int mtk_drm_find_possible_crtc_by_comp(struct drm_device *drm, - struct device *dev); +int mtk_drm_find_possible_crtc_by_comp(struct drm_device *drm, struct device *dev); int mtk_ddp_comp_init(struct device_node *comp_node, struct mtk_ddp_comp *comp, unsigned int comp_id); enum mtk_ddp_comp_type mtk_ddp_comp_get_type(unsigned int comp_id); diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c index d8bfc2cce54d..d67e5c61a9b9 100644 --- a/drivers/gpu/drm/mediatek/mtk_dsi.c +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c @@ -843,7 +843,10 @@ static int mtk_dsi_encoder_init(struct drm_device *drm, struct mtk_dsi *dsi) return ret; } - dsi->encoder.possible_crtcs = mtk_drm_find_possible_crtc_by_comp(drm, dsi->host.dev); + ret = mtk_drm_find_possible_crtc_by_comp(drm, dsi->host.dev); + if (ret < 0) + goto err_cleanup_encoder; + dsi->encoder.possible_crtcs = ret; ret = drm_bridge_attach(&dsi->encoder, &dsi->bridge, NULL, DRM_BRIDGE_ATTACH_NO_CONNECTOR);
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. While at it, make the return code a signed one and return -ENOENT if no path is found and handle the error in the callers. 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> --- v4: - return -ENOENT if mtk_drm_find_possible_crtc_by_comp() doesn't find any path 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_dpi.c | 5 +- drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c | 75 ++++++++++++++++----- drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h | 3 +- drivers/gpu/drm/mediatek/mtk_dsi.c | 5 +- 4 files changed, 68 insertions(+), 20 deletions(-)