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

[Jason-JH Lin (林睿祥)]

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@xxxxxxxxxxxx>
> > Signed-off-by: Nathan Lu <nathan.lu@xxxxxxxxxxxx>
> > Signed-off-by: Jason-JH.Lin <jason-jh.lin@xxxxxxxxxxxx>
> > ---
> >   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
> 
> 
> 
>