<pre>
Hi CK,

Thanks for the reviews.

On Mon, 2023-09-18 at 07:06 +0000, CK Hu (胡俊光) wrote:
> Hi, Jason:
>
> On Thu, 2023-08-10 at 02:15 +0800, Jason-JH.Lin wrote:
> > Add dynamic select available connector flow in
> > mtk_drm_crtc_create()
> > and mtk_drm_crtc_atomic_enable().
> >
> > In mtk_drm_crtc_create(), if there is a connector routes array in
> > drm
> > driver data, all components definded in the connector routes array
> > will
> > be checked and their encoder_index will be set.
> >
> > In mtk_drm_crtc_atomic_enable(), crtc will check its encoder_index
> > to
> > identify which componet in the connector routes array should
> > append.
> >
> > Move DDP_COMPONENT_DP_INTF0 from mt8188_mtk_ddp_main array to a
> > connector routes array called mt8188_mtk_ddp_main_routes to support
> > dynamic selection capability for mt8188.
> >
> > 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_drm_crtc.c | 81
> > ++++++++++++++++++++-
> > drivers/gpu/drm/mediatek/mtk_drm_crtc.h | 5 +-
> > drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c | 27 ++++++-
> > drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h | 7 ++
> > drivers/gpu/drm/mediatek/mtk_drm_drv.c | 20 ++++-
> > drivers/gpu/drm/mediatek/mtk_drm_drv.h | 7 ++
> > 6 files changed, 140 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > index d40142842f85..c57012f9c0c8 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > @@ -62,6 +62,8 @@ struct mtk_drm_crtc {
> > struct mtk_mutex*mutex;
> > unsigned intddp_comp_nr;
> > struct mtk_ddp_comp**ddp_comp;
> > +unsigned intnum_conn_routes;
> > +const struct mtk_drm_route*conn_routes;
> >
> > /* lock for display hardware access */
> > struct mutexhw_lock;
> > @@ -649,6 +651,47 @@ static void mtk_drm_crtc_disable_vblank(struct
> > drm_crtc *crtc)
> > mtk_ddp_comp_disable_vblank(comp);
> > }
> >
> > +static void mtk_drm_crtc_update_output(struct drm_crtc *crtc,
> > + struct drm_atomic_state *state)
> > +{
> > +int crtc_index = drm_crtc_index(crtc);
> > +int i;
> > +struct device *dev;
> > +struct drm_crtc_state *crtc_state = state-
> > > crtcs[crtc_index].new_state;
> >
> > +struct mtk_drm_crtc *mtk_crtc = to_mtk_crtc(crtc);
> > +struct mtk_drm_private *priv = crtc->dev->dev_private;
> > +unsigned int comp_id;
> > +unsigned int encoder_mask = crtc_state->encoder_mask;
> > +
> > +if (!crtc_state->connectors_changed)
> > +return;
> > +
> > +if (!mtk_crtc->num_conn_routes)
> > +return;
> > +
> > +priv = priv->all_drm_private[crtc_index];
> > +dev = priv->dev;
> > +
> > +dev_dbg(dev, "connector change:%d, encoder mask:0x%x for
> > crtc:%d\n",
> > +crtc_state->connectors_changed, encoder_mask,
> > crtc_index);
> > +
> > +for (i = 0; i < mtk_crtc->num_conn_routes; i++) {
> > +struct mtk_ddp_comp *comp;
> > +
> > +comp_id = mtk_crtc->conn_routes[i].route_ddp;
> > +comp = &priv->ddp_comp[comp_id];
> > +if (comp->encoder_index >= 0 &&
> > + encoder_mask & BIT(comp->encoder_index)) {
> > +mtk_crtc->ddp_comp[mtk_crtc->ddp_comp_nr - 1] =
> > comp;
> > +dev_dbg(dev, "Add comp_id: %d at path index
> > %d\n",
> > +comp->id, mtk_crtc->ddp_comp_nr - 1);
> > +break;
> > +}
> > +}
> > +
> > +dev_dbg(dev, "Update total comp num:%d", mtk_crtc-
> > > ddp_comp_nr);
>
> Why print this? You does not update comp num.

OK, I'll remove this message.

>
> > +}
> > +
> > int mtk_drm_crtc_plane_check(struct drm_crtc *crtc, struct
> > drm_plane
> > *plane,
> > struct mtk_plane_state *state)
> > {
> > @@ -681,6 +724,8 @@ static void mtk_drm_crtc_atomic_enable(struct
> > drm_crtc *crtc,
> >
> > DRM_DEBUG_DRIVER("%s %d\n", __func__, crtc->base.id);
> >
> > +mtk_drm_crtc_update_output(crtc, state);
> > +
> > ret = pm_runtime_resume_and_get(comp->dev);
> > if (ret < 0) {
> > DRM_DEV_ERROR(comp->dev, "Failed to enable power
> > domain: %d\n", ret);
> > @@ -886,7 +931,8 @@ struct device *mtk_drm_crtc_dma_dev_get(struct
> > drm_crtc *crtc)
> >
> > int mtk_drm_crtc_create(struct drm_device *drm_dev,
> > const unsigned int *path, unsigned int
> > path_len,
> > -int priv_data_index)
> > +int priv_data_index, const struct mtk_drm_route
> > *conn_routes,
> > +unsigned int num_conn_routes)
> > {
> > struct mtk_drm_private *priv = drm_dev->dev_private;
> > struct device *dev = drm_dev->dev;
> > @@ -1040,5 +1086,38 @@ int mtk_drm_crtc_create(struct drm_device
> > *drm_dev,
> > init_waitqueue_head(&mtk_crtc->cb_blocking_queue);
> > }
> > #endif
> > +
> > +if (conn_routes) {
> > +struct device_node *node;
> > +struct mtk_ddp_comp *comp;
> > +unsigned int comp_id;
> > +
> > +for (i = 0; i < num_conn_routes; i++) {
> > +comp_id = conn_routes[i].route_ddp;
> > +node = priv->comp_node[comp_id];
> > +comp = &priv->ddp_comp[comp_id];
> > +
> > +if (!comp->dev) {
> > +dev_dbg(dev, "comp_id:%d, Component
> > %pOF not initialized\n",
>
> I would like add comment that we allow route component device is not
> enabled make set comp->encoder_index to -1 to mark this.

OK, I'll add the comment for that.

>
> > +comp_id, node);
> > +comp->encoder_index = -1;
> > +continue;
> > +}
> > +
> > +mtk_ddp_comp_encoder_index_set(&priv-
> > > ddp_comp[comp_id]);
> >
> > +}
> > +
> > +mtk_crtc->num_conn_routes = num_conn_routes;
> > +mtk_crtc->conn_routes = conn_routes;
> > +
> > +/* append the last ddp_comp and ddp_comp_nr at the end
> > of mtk_drm_crtc_create */
> > +mtk_crtc->ddp_comp[mtk_crtc->ddp_comp_nr] =
> > +devm_kmalloc(dev, sizeof(*mtk_crtc->ddp_comp),
> > GFP_KERNEL);
>
> mtk_crtc->ddp_comp[mtk_crtc->ddp_comp_nr] is not valid here. You may
> increase mtk_crtc->ddp_comp_nr before mtk_crtc->ddp_comp is
> allocated.
>

OK, I'll calculate the max array size first, then allocate the array.

> > +if (!mtk_crtc->ddp_comp[mtk_crtc->ddp_comp_nr])
> > +return -ENOMEM;
> > +
> > +mtk_crtc->ddp_comp_nr++;
> > +}
> > +
> > return 0;
> > }
> > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.h
> > b/drivers/gpu/drm/mediatek/mtk_drm_crtc.h
> > index 3e9046993d09..3c224595fa71 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.h
> > +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.h
> > @@ -8,6 +8,7 @@
> >
> > #include <drm/drm_crtc.h>
> > #include "mtk_drm_ddp_comp.h"
> > +#include "mtk_drm_drv.h"
> > #include "mtk_drm_plane.h"
> >
> > #define MTK_LUT_SIZE512
> > @@ -18,7 +19,9 @@ void mtk_drm_crtc_commit(struct drm_crtc *crtc);
> > int mtk_drm_crtc_create(struct drm_device *drm_dev,
> > const unsigned int *path,
> > unsigned int path_len,
> > -int priv_data_index);
> > +int priv_data_index,
> > +const struct mtk_drm_route *conn_routes,
> > +unsigned int num_conn_routes);
> > int mtk_drm_crtc_plane_check(struct drm_crtc *crtc, struct
> > drm_plane
> > *plane,
> > struct mtk_plane_state *state);
> > void mtk_drm_crtc_async_update(struct drm_crtc *crtc, struct
> > drm_plane *plane,
> > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
> > b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
> > index effaaa769b46..c1ea112e6be8 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
> > @@ -508,6 +508,23 @@ 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 num_routes,
> > + struct mtk_ddp_comp
> > *ddp_comp)
> > +{
> > +unsigned int i;
> > +
> > +if (!routes)
> > +return -EINVAL;
> > +
> > +for (i = 0; i < num_routes; i++)
> > +if (dev == ddp_comp[routes[i].route_ddp].dev)
> > +return BIT(routes[i].crtc_id);
> > +
> > +return -ENODEV;
> > +}
> > +
> > int mtk_ddp_comp_get_id(struct device_node *node,
> > enum mtk_ddp_comp_type comp_type)
> > {
> > @@ -539,7 +556,15 @@ unsigned int
> > mtk_drm_find_possible_crtc_by_comp(struct drm_device *drm,
> > private->data->third_len,
> > private->ddp_comp))
> > ret = BIT(2);
> > else
> > -DRM_INFO("Failed to find comp in ddp table\n");
> > +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) {
> > +DRM_INFO("Failed to find comp in ddp table, ret =%d\n",
> > ret);
> > +ret = 0;
> > +}
> >
> > return ret;
> > }
> > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h
> > b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h
> > index 326cb57a18c9..d16c9cb20199 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h
> > +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h
> > @@ -87,6 +87,7 @@ struct mtk_ddp_comp {
> > struct device *dev;
> > int irq;
> > unsigned int id;
> > +unsigned int encoder_index;
> > const struct mtk_ddp_comp_funcs *funcs;
> > };
> >
> > @@ -276,6 +277,12 @@ static inline bool
> > mtk_ddp_comp_disconnect(struct mtk_ddp_comp *comp, struct dev
> > return false;
> > }
> >
> > +static inline void mtk_ddp_comp_encoder_index_set(struct
> > mtk_ddp_comp *comp)
> > +{
> > +if (comp->funcs && comp->funcs->encoder_index)
> > +comp->encoder_index = comp->funcs->encoder_index(comp-
> > > dev);
> >
> > +}
>
> I would like this to be a separate patch of adding ddp comp encoder
> index interface. Part of [4/7] would be that patch also.

OK I'll separate it.

>
> > +
> > 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,
> > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> > b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> > index c12886f31e54..9900007667a2 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> > @@ -185,7 +185,10 @@ static const unsigned int
> > mt8188_mtk_ddp_main[]
> > = {
> > DDP_COMPONENT_GAMMA,
> > DDP_COMPONENT_POSTMASK0,
> > DDP_COMPONENT_DITHER0,
> > -DDP_COMPONENT_DP_INTF0,
> > +};
> > +
> > +static const struct mtk_drm_route mt8188_mtk_ddp_main_routes[] = {
> > +{0, DDP_COMPONENT_DP_INTF0},
> > };
>
> I would like separate mt8188 modification to another patch and make
> this patch independent of each SoC.

OK, I'll separate this to another patch.

Regards,
Jason-JH.Lin
>
> >
> > static const unsigned int mt8192_mtk_ddp_main[] = {
> > @@ -287,6 +290,8 @@ static const struct mtk_mmsys_driver_data
> > mt8186_mmsys_driver_data = {
> > static const struct mtk_mmsys_driver_data
> > mt8188_vdosys0_driver_data
> > = {
> > .main_path = mt8188_mtk_ddp_main,
> > .main_len = ARRAY_SIZE(mt8188_mtk_ddp_main),
> > +.conn_routes = mt8188_mtk_ddp_main_routes,
> > +.num_conn_routes = ARRAY_SIZE(mt8188_mtk_ddp_main_routes),
> > .mmsys_dev_num = 1,
> > };
> >
> > @@ -419,6 +424,11 @@ static bool mtk_drm_find_mmsys_comp(struct
> > mtk_drm_private *private, int comp_id
> > if (drv_data->third_path[i] == comp_id)
> > return true;
> >
> > +if (drv_data->num_conn_routes)
> > +for (i = 0; i < drv_data->num_conn_routes; i++)
> > +if (drv_data->conn_routes[i].route_ddp ==
> > comp_id)
> > +return true;
> > +
> > return false;
> > }
> >
> > @@ -477,21 +487,23 @@ static int mtk_drm_kms_init(struct drm_device
> > *drm)
> >
> > if (i == CRTC_MAIN && priv_n->data->main_len) {
> > ret = mtk_drm_crtc_create(drm, priv_n-
> > > data->main_path,
> >
> > - priv_n->data-
> > > main_len, j);
> >
> > + priv_n->data-
> > > main_len, j,
> >
> > + priv_n->data-
> > > conn_routes,
> >
> > + priv_n->data-
> > > num_conn_routes);
> >
> > if (ret)
> > goto err_component_unbind;
> >
> > continue;
> > } else if (i == CRTC_EXT && priv_n->data-
> > > ext_len) {
> >
> > ret = mtk_drm_crtc_create(drm, priv_n-
> > > data->ext_path,
> >
> > - priv_n->data-
> > > ext_len, j);
> >
> > + priv_n->data-
> > > ext_len, j, NULL, 0);
> >
> > if (ret)
> > goto err_component_unbind;
> >
> > continue;
> > } else if (i == CRTC_THIRD && priv_n->data-
> > > third_len) {
> >
> > ret = mtk_drm_crtc_create(drm, priv_n-
> > > data->third_path,
> >
> > - priv_n->data-
> > > third_len, j);
> >
> > + priv_n->data-
> > > third_len, j, NULL, 0);
> >
> > if (ret)
> > goto err_component_unbind;
> >
> > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.h
> > b/drivers/gpu/drm/mediatek/mtk_drm_drv.h
> > index f4de8bb27685..6f98fff4f1a4 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.h
> > +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.h
> > @@ -28,6 +28,11 @@ struct drm_fb_helper;
> > struct drm_property;
> > struct regmap;
> >
> > +struct mtk_drm_route {
> > +const unsigned int crtc_id;
> > +const unsigned int route_ddp;
> > +};
> > +
> > struct mtk_mmsys_driver_data {
> > const unsigned int *main_path;
> > unsigned int main_len;
> > @@ -35,6 +40,8 @@ struct mtk_mmsys_driver_data {
> > unsigned int ext_len;
> > const unsigned int *third_path;
> > unsigned int third_len;
> > +const struct mtk_drm_route *conn_routes;
> > +unsigned int num_conn_routes;
> >
> > bool shadow_register;
> > unsigned int mmsys_id;

</pre><!--type:text--><!--{--><pre>************* MEDIATEK Confidentiality Notice ********************
The information contained in this e-mail message (including any 
attachments) may be confidential, proprietary, privileged, or otherwise
exempt from disclosure under applicable laws. It is intended to be 
conveyed only to the designated recipient(s). Any use, dissemination, 
distribution, printing, retaining or copying of this e-mail (including its 
attachments) by unintended recipient(s) is strictly prohibited and may 
be unlawful. If you are not an intended recipient of this e-mail, or believe 
that you have received this e-mail in error, please notify the sender 
immediately (by replying to this e-mail), delete any and all copies of 
this e-mail (including any attachments) from your system, and do not
disclose the content of this e-mail to any other person. Thank you!
</pre><!--}-->