Re: [PATCH v7 3/4] drm/mediatek: Add ability to support dynamic connector selection

Jason-JH Lin (林睿祥)
2023-07-31T09:37:31Z

Hi Angelo,

Thanks for the reviews.

On Fri, 2023-07-28 at 10:11 +0200, AngeloGioacchino Del Regno wrote:
> Il 27/07/23 18:41, Jason-JH.Lin ha scritto:
> > 1. Move output drm connector from each ddp_path array to connector
> > array.
> > 2. Add dynamic select available connector flow in crtc create and
> > enable.
> >
> > Signed-off-by: Nancy Lin <nancy.lin@mediatek.com>
> > Signed-off-by: Nathan Lu <nathan.lu@mediatek.com>
> > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> > ---
> > drivers/gpu/drm/mediatek/mtk_disp_drv.h | 1 +
> > drivers/gpu/drm/mediatek/mtk_dpi.c | 9 +++
> > drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 75
> > ++++++++++++++++++++-
> > drivers/gpu/drm/mediatek/mtk_drm_crtc.h | 5 +-
> > drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c | 26 +++++++
> > drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h | 8 +++
> > drivers/gpu/drm/mediatek/mtk_drm_drv.c | 20 ++++--
> > drivers/gpu/drm/mediatek/mtk_drm_drv.h | 7 ++
> > 8 files changed, 145 insertions(+), 6 deletions(-)
> >
>
> ..snip..
>
> > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
> > b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
> > index f114da4d36a9..bc7b0a0c20db 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
> > @@ -304,6 +304,7 @@ static const struct mtk_ddp_comp_funcs
> > ddp_dither = {
> > static const struct mtk_ddp_comp_funcs ddp_dpi = {
> > .start = mtk_dpi_start,
> > .stop = mtk_dpi_stop,
> > + .encoder_index = mtk_dpi_encoder_index,
> > };
> >
> > static const struct mtk_ddp_comp_funcs ddp_dsc = {
> > @@ -507,6 +508,25 @@ static bool mtk_drm_find_comp_in_ddp(struct
> > device *dev,
> > return false;
> > }
> >
> > +static int mtk_drm_find_comp_in_ddp_conn_path(struct device *dev,
> > + const struct
> > mtk_drm_route *routes,
> > + unsigned int routes_num,
>
> `num_routes` would be more readable.

OK, I'll change this naming.

>
> > + struct mtk_ddp_comp
> > *ddp_comp)
> > +{
> > + unsigned int i;
> > +
> > + if (!routes)
> > + return 0;
>
> if (!routes)
> return -EINVAL;

OK, I'll change it.

>
> > +
> > + for (i = 0; i < routes_num; i++)
> > + if (dev == ddp_comp[routes[i].route_ddp].dev)
> > + return BIT(routes[i].crtc_id);
> > +
> > + DRM_INFO("Failed to find comp in ddp connector table\n");
>
> This print is redundant.

OK, I'll remove this print.
>
> > +
> > + return 0;
>
> return -ENODEV;

OK, I'll change it.

>
> > +}
> > +
> > int mtk_ddp_comp_get_id(struct device_node *node,
> > enum mtk_ddp_comp_type comp_type)
> > {
> > @@ -538,6 +558,12 @@ unsigned int
> > mtk_drm_find_possible_crtc_by_comp(struct drm_device *drm,
> > 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);
> > +
> > + if (ret == 0)
>
> if (ret < 0)
>
> > DRM_INFO("Failed to find comp in ddp table\n");

The return value of mtk_drm_find_possible_crtc_by_comp() is `unsigned
int` and it will be assigned to `unsigned int ` dpi-
>encoder.possible_crtcs in mtk_dpi_bind() directly.

So we should reassign ret to 0 when ret < 0.
I would like to change to:

if (ret <= 0) {
DRM_INFO("Failed to find comp in ddp table, ret=%d\n", ret);
ret = 0;
}

Regards,
Jason-JH.Lin

> >
> > return ret;

>
> Regards,
> Angelo
>
>
>
>