On Mon, Nov 07, 2022 at 03:24:12PM +0800, Nancy.Lin wrote:
> Add drm ovl_adaptor sub driver. Bring up ovl_adaptor sub driver if
> the component exists in the path.
>
> Signed-off-by: Nancy.Lin <[email protected]>
> Reviewed-by: AngeloGioacchino Del Regno <[email protected]>
> Reviewed-by: CK Hu <[email protected]>
> Tested-by: AngeloGioacchino Del Regno <[email protected]>
> Tested-by: Bo-Chen Chen <[email protected]>
> Tested-by: N?colas F. R. A. Prado <[email protected]>
> ---
[..]
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> index 30dcb65d8a5a..ce5617ad04cb 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
[..]
> @@ -897,22 +906,18 @@ int mtk_drm_crtc_create(struct drm_device *drm_dev,
> crtc_i++;
>
> for (i = 0; i < path_len; i++) {
> - enum mtk_ddp_comp_id comp_id = path[i];
> + unsigned int comp_id = path[i];
> struct device_node *node;
> - struct mtk_ddp_comp *comp;
>
> node = priv->comp_node[comp_id];
> - comp = &priv->ddp_comp[comp_id];
> -
> - if (!node) {
> - dev_info(dev,
> - "Not creating crtc %d because component %d is disabled or missing\n",
> - crtc_i, comp_id);
> - return 0;
> - }
>
> - if (!comp->dev) {
> - dev_err(dev, "Component %pOF not initialized\n", node);
> + /* Not all drm components have a DTS device node, such as ovl_adaptor,
> + * which is the drm bring up sub driver
> + */
> + if (!node && comp_id != DDP_COMPONENT_DRM_OVL_ADAPTOR) {
> + dev_err(dev,
> + "Not creating crtc %d because component %d is disabled, missing or not initialized\n",
> + crtc_i, comp_id);
> return -ENODEV;
Why do you change the behavior here? If !node, the return should be 0, because
there are two separate data streams, for internal and external display, and they
are optional. It should be possible to for example have the nodes for external
display disabled in DT and still have the driver probe and have a working
internal display. But with this change you're breaking that. Also, this message
should stay as dev_info and not mention "not initialized", so basically it
should stay the same as before the change.
Thanks,
N?colas
Dear Nicolas,
Thanks for the review.
On Fri, 2022-11-25 at 17:24 -0500, Nícolas F. R. A. Prado wrote:
> On Mon, Nov 07, 2022 at 03:24:12PM +0800, Nancy.Lin wrote:
> > Add drm ovl_adaptor sub driver. Bring up ovl_adaptor sub driver if
> > the component exists in the path.
> >
> > Signed-off-by: Nancy.Lin <[email protected]>
> > Reviewed-by: AngeloGioacchino Del Regno <
> > [email protected]>
> > Reviewed-by: CK Hu <[email protected]>
> > Tested-by: AngeloGioacchino Del Regno <
> > [email protected]>
> > Tested-by: Bo-Chen Chen <[email protected]>
> > Tested-by: Nícolas F. R. A. Prado <[email protected]>
> > ---
>
> [..]
> > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > index 30dcb65d8a5a..ce5617ad04cb 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
>
> [..]
> > @@ -897,22 +906,18 @@ int mtk_drm_crtc_create(struct drm_device
> > *drm_dev,
> > crtc_i++;
> >
> > for (i = 0; i < path_len; i++) {
> > - enum mtk_ddp_comp_id comp_id = path[i];
> > + unsigned int comp_id = path[i];
> > struct device_node *node;
> > - struct mtk_ddp_comp *comp;
> >
> > node = priv->comp_node[comp_id];
> > - comp = &priv->ddp_comp[comp_id];
> > -
> > - if (!node) {
> > - dev_info(dev,
> > - "Not creating crtc %d because
> > component %d is disabled or missing\n",
> > - crtc_i, comp_id);
> > - return 0;
> > - }
> >
> > - if (!comp->dev) {
> > - dev_err(dev, "Component %pOF not
> > initialized\n", node);
> > + /* Not all drm components have a DTS device node, such
> > as ovl_adaptor,
> > + * which is the drm bring up sub driver
> > + */
> > + if (!node && comp_id != DDP_COMPONENT_DRM_OVL_ADAPTOR)
> > {
> > + dev_err(dev,
> > + "Not creating crtc %d because component
> > %d is disabled, missing or not initialized\n",
> > + crtc_i, comp_id);
> > return -ENODEV;
>
> Why do you change the behavior here? If !node, the return should be
> 0, because
> there are two separate data streams, for internal and external
> display, and they
> are optional. It should be possible to for example have the nodes for
> external
> display disabled in DT and still have the driver probe and have a
> working
> internal display. But with this change you're breaking that. Also,
> this message
> should stay as dev_info and not mention "not initialized", so
> basically it
> should stay the same as before the change.
>
> Thanks,
> Nícolas
Yes, You are right. This is my mistake. I should not change this
behavior. I will fix it and modify as following for the ovl_adaptor sub
driver component, which don't have dts device node.
@@ -905,7 +914,10 @@ int mtk_drm_crtc_create(struct drm_device
*drm_dev,
node = priv->comp_node[comp_id];
comp = &priv->ddp_comp[comp_id];
- if (!node) {
+ /* Not all drm components have a DTS device node, such
as ovl_adaptor,
+ * which is the drm bring up sub driver
+ */
+ if (!node && comp_id != DDP_COMPONENT_DRM_OVL_ADAPTOR)
{
dev_info(dev,
"Not creating crtc %d because
component %d is disabled or missing\n",
crtc_i, comp_id);
@@ -938,7 +950,7 @@ int mtk_drm_crtc_create(struct drm_device *drm_dev,
}
Regards,
Nancy