Messages in this thread | | | From | Jason-JH Lin (林睿祥) <> | Subject | Re: [PATCH 1/2] drm/mediatek: Add ability to support dynamic connector selection | Date | Wed, 12 Apr 2023 12:26:11 +0000 |
| |
Hi Alexandre,
Thanks for the reviews.
On Wed, 2023-04-12 at 11:37 +0200, Alexandre Mergnat wrote: > External email : Please do not click links or open attachments until > you have verified the sender or the content. > > > Hi Jason, > > Can you give me the based git tree/branch for this series please ? > I want to do a non-regression test for mt8365-evk board. > Sorry for missing this information, I'll add this into [PATCH 0/2] at next version.
It based on mediatek-drm maintainer's tree / mediatek-drm-next branch :
https://git.kernel.org/pub/scm/linux/kernel/git/chunkuang.hu/linux.git/log/?h=mediatek-drm-next
> Feedback below: > > On 05/04/2023 16:51, Jason-JH.Lin wrote: > > 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. > > snip... > > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c > > b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c > > index d40142842f85..5ff20d0de29b 100644 > > --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c > > +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c > > @@ -60,8 +60,13 @@ struct mtk_drm_crtc { > > struct device *mmsys_dev; > > struct device *dma_dev; > > struct mtk_mutex *mutex; > > + > > Remove this extra line please. > OK, I'll remove it.
> > + unsigned int ddp_comp_nr_ori; > > + unsigned int max_ddp_comp_nr; > > unsigned int ddp_comp_nr; > > struct mtk_ddp_comp **ddp_comp; > > + unsigned int conn_route_nr; > > + const struct mtk_drm_route *conn_routes; > > > > /* lock for display hardware access */ > > struct mutex hw_lock; > > @@ -649,6 +654,85 @@ static void mtk_drm_crtc_disable_vblank(struct > > drm_crtc *crtc) > > mtk_ddp_comp_disable_vblank(comp); > > } > > > > +static unsigned int mtk_drm_crtc_max_num_route_comp(struct > > mtk_drm_crtc *mtk_crtc) > > +{ > > + unsigned int max_num = 0; > > + unsigned int i; > > + > > + if (!mtk_crtc->conn_route_nr) > > + return 0; > > + > > + for (i = 0; i < mtk_crtc->conn_route_nr; i++) > > + max_num = (max_num > mtk_crtc- > > >conn_routes[i].route_len) ? max_num : > > + mtk_crtc->conn_routes[i].route_len; > > if test is true, then do "max_num = max_num" > I'm not sure the compiler will optimize that. > Also, I think it more readable like that: > > if (max_num < mtk_crtc->conn_routes[i].route_len) { > max_num = mtk_crtc->conn_routes[i].route_len; > } > OK, I'll refine this.
> > + > > + return max_num; > > +} > > + > > +static int mtk_drm_crtc_update_output(struct drm_crtc *crtc, > > + struct drm_atomic_state *state) > > +{ > > + int crtc_index = drm_crtc_index(crtc); > > + struct drm_crtc_state *crtc_state = state- > > >crtcs[crtc_index].new_state; > > + unsigned int encoder_mask = crtc_state->encoder_mask; > > + struct mtk_drm_private *priv = crtc->dev->dev_private; > > + struct device *dev; > > + struct mtk_drm_crtc *mtk_crtc = to_mtk_crtc(crtc); > > + const struct mtk_drm_route *conn_routes; > > + unsigned int route_len = 0, route_index = 0; > > + unsigned int comp_id; > > + int i; > > Can you put them in alphabetical order please ? > OK, I'll put them in alphabetical order.
> > + > > + if (!mtk_crtc->conn_route_nr) > > + return 0; > > + > > snip... > > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.h > > b/drivers/gpu/drm/mediatek/mtk_drm_crtc.h > > index 3e9046993d09..96790f8f7a94 100644 > > --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.h > > +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.h > > @@ -7,6 +7,7 @@ > > #define MTK_DRM_CRTC_H > > > > #include <drm/drm_crtc.h> > > +#include "mtk_drm_drv.h" > > Alphabetical order please.
OK, I'll put them in alphabetical order.
Regards, Jason-JH.Lin > > > #include "mtk_drm_ddp_comp.h" > > #include "mtk_drm_plane.h" > > > > @@ -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 conn_routes_num); > > 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, > > > Regards, > Alexandre >
| |