All of lore.kernel.org
 help / color / mirror / Atom feed
From: xinlei.lee <xinlei.lee@mediatek.com>
To: Rex-BC Chen <rex-bc.chen@mediatek.com>, <chunkuang.hu@kernel.org>,
	<p.zabel@pengutronix.de>, <airlied@linux.ie>, <daniel@ffwll.ch>,
	<matthias.bgg@gmail.com>
Cc: jitao.shi@mediatek.com, linux-kernel@vger.kernel.org,
	dri-devel@lists.freedesktop.org,
	Project_Global_Chrome_Upstream_Group@mediatek.com,
	linux-mediatek@lists.infradead.org,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v3,2/4] drm/mediatek: Separate poweron/poweroff from enable/disable and define new funcs
Date: Thu, 7 Apr 2022 11:50:32 +0800	[thread overview]
Message-ID: <bdeeb164ad0144eafa6af2dc4ad1b6f7ce2a08e3.camel@mediatek.com> (raw)
In-Reply-To: <eca60f45ab518a53b1d281c078f562b8bc610060.camel@mediatek.com>

On Wed, 2022-03-23 at 19:46 +0800, Rex-BC Chen wrote:
> On Tue, 2022-03-22 at 17:23 +0800, xinlei.lee wrote:
> > On Thu, 2022-03-17 at 20:02 +0800, Rex-BC Chen wrote:
> > > Hello Xinlei,
> > > 
> > > On Thu, 2022-03-17 at 15:53 +0800, xinlei.lee@mediatek.com wrote:
> > > > From: Jitao Shi <jitao.shi@mediatek.com>
> > > > 
> > > > In order to match the changes of "Use the drm_panel_bridge
> > > > API",
> > > > the poweron/poweroff of dsi is extracted from enable/disable
> > > > and
> > > > defined as new funcs (pre_enable/post_disable).
> > > > 
> > > > Fixes: 2dd8075d2185 ("drm/mediatek: mtk_dsi: Use the
> > > > drm_panel_bridge
> > > > API")
> > > > 
> > > > Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
> > > > Signed-off-by: Xinlei Lee <xinlei.lee@mediatek.com>
> > > > ---
> > > >  drivers/gpu/drm/mediatek/mtk_dsi.c | 45 +++++++++++++++++-----
> > > > --
> > > > --
> > > > --
> > > > --
> > > >  1 file changed, 26 insertions(+), 19 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c
> > > > b/drivers/gpu/drm/mediatek/mtk_dsi.c
> > > > index 262c027d8c2f..e33caaca11a7 100644
> > > > --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> > > > +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> > > > @@ -679,16 +679,6 @@ static void mtk_dsi_poweroff(struct
> > > > mtk_dsi
> > > > *dsi)
> > > >  	if (--dsi->refcount != 0)
> > > >  		return;
> > > >  
> > > > -	/*
> > > > -	 * mtk_dsi_stop() and mtk_dsi_start() is asymmetric,
> > > > since
> > > > -	 * mtk_dsi_stop() should be called after
> > > > mtk_drm_crtc_atomic_disable(),
> > > > -	 * which needs irq for vblank, and mtk_dsi_stop() will
> > > > disable
> > > > irq.
> > > > -	 * mtk_dsi_start() needs to be called in
> > > > mtk_output_dsi_enable(),
> > > > -	 * after dsi is fully set.
> > > > -	 */
> > > > -	mtk_dsi_stop(dsi);
> > > > -
> > > > -	mtk_dsi_switch_to_cmd_mode(dsi, VM_DONE_INT_FLAG, 500);
> > > >  	mtk_dsi_reset_engine(dsi);
> > > >  	mtk_dsi_lane0_ulp_mode_enter(dsi);
> > > >  	mtk_dsi_clk_ulp_mode_enter(dsi);
> > > > @@ -703,17 +693,9 @@ static void mtk_dsi_poweroff(struct
> > > > mtk_dsi
> > > > *dsi)
> > > >  
> > > >  static void mtk_output_dsi_enable(struct mtk_dsi *dsi)
> > > >  {
> > > > -	int ret;
> > > > -
> > > >  	if (dsi->enabled)
> > > >  		return;
> > > >  
> > > > -	ret = mtk_dsi_poweron(dsi);
> > > > -	if (ret < 0) {
> > > > -		DRM_ERROR("failed to power on dsi\n");
> > > > -		return;
> > > > -	}
> > > > -
> > > >  	mtk_dsi_set_mode(dsi);
> > > >  	mtk_dsi_clk_hs_mode(dsi, 1);
> > > >  
> > > > @@ -727,7 +709,16 @@ static void mtk_output_dsi_disable(struct
> > > > mtk_dsi *dsi)
> > > >  	if (!dsi->enabled)
> > > >  		return;
> > > >  
> > > > -	mtk_dsi_poweroff(dsi);
> > > > +	/*
> > > > +	 * mtk_dsi_stop() and mtk_dsi_start() is asymmetric,
> > > > since
> > > 
> > > Why they are asymmetric?
> > > 
> > > > +	 * mtk_dsi_stop() should be called after
> > > > mtk_drm_crtc_atomic_disable(),
> > > > +	 * which needs irq for vblank, and mtk_dsi_stop() will
> > > > disable
> > > > irq.
> > > > +	 * mtk_dsi_start() needs to be called in
> > > > mtk_output_dsi_enable(),
> > > > +	 * after dsi is fully set.
> > > > +	 */
> > > > +	mtk_dsi_stop(dsi);
> > > > +
> > > > +	mtk_dsi_switch_to_cmd_mode(dsi, VM_DONE_INT_FLAG, 500);
> > > >  
> > > >  	dsi->enabled = false;
> > > >  }
> > > > @@ -765,10 +756,26 @@ static void mtk_dsi_bridge_enable(struct
> > > > drm_bridge *bridge)
> > > >  	mtk_output_dsi_enable(dsi);
> > > >  }
> > > >  
> > > > +static void mtk_dsi_bridge_pre_enable(struct drm_bridge
> > > > *bridge)
> > > > +{
> > > > +	struct mtk_dsi *dsi = bridge_to_dsi(bridge);
> > > > +
> > > > +	mtk_dsi_poweron(dsi);
> > > 
> > > Should you handle the error of mtk_dsi_poweron?
> > > If you failed to mtk_dsi_bridge_pre_enable and do
> > > mtk_dsi_bridge_enable,
> > > what will happend?
> > > 
> > > > +}
> > > > +
> > > > +static void mtk_dsi_bridge_post_disable(struct drm_bridge
> > > > *bridge)
> > > > +{
> > > > +	struct mtk_dsi *dsi = bridge_to_dsi(bridge);
> > > > +
> > > > +	mtk_dsi_poweroff(dsi);
> > > 
> > > If you failed to mtk_dsi_bridge_disable and you do
> > > mtk_dsi_bridge_post_disable,
> > > what will happend?
> > > Do you need to handle this?
> > > 
> > > BRs,
> > > Rex
> > > 
> > > > +}
> > > > +
> > > >  static const struct drm_bridge_funcs mtk_dsi_bridge_funcs = {
> > > >  	.attach = mtk_dsi_bridge_attach,
> > > >  	.disable = mtk_dsi_bridge_disable,
> > > >  	.enable = mtk_dsi_bridge_enable,
> > > > +	.pre_enable = mtk_dsi_bridge_pre_enable,
> > > > +	.post_disable = mtk_dsi_bridge_post_disable,
> > > >  	.mode_set = mtk_dsi_bridge_mode_set,
> > > >  };
> > > >  
> > > 
> > > 
> > 
> > Hi Rex:
> > 
> > Thanks for your review!
> > 
> > 1.Why they are asymmetric?
> > =>My understanding mtk_dsi_stop() and mtk_dsi_start() is to make
> > dsi
> > switch from LP11 and HS mode.DSI has two working modes:
> > If it is cmd mode, the data sent is sent by LP11, and dsi_start is
> > just
> > a signal. In this mode, dsi_stop is not required after sending cmd.
> > If it is video mode, because the data needs to be sent in HS mode,
> > dsi_start is required to make dsi enter HS mode from LP11. After
> > suspend, drm will call dsi_disable, and call dsi_stop to make dsi
> > return from HS mode to LP11 state.
> > Therefore mtk_dsi_stop() and mtk_dsi_start() are asymmetric.
> > For example, in the dsi_host_transfer function, only dsi_stop has
> > no
> > dsi_start operation.
> > 
> > 2.
> > Because the return type of pre_enable & post_disable in common code
> > is
> > void type. If there is an error, it will be processed in
> > poweron/poweroff, and the error message will be printed.
> > Do you mean that pre_enable & post_disable needs to accept the
> > poweron/poweroff error return value and then print the error log?
> > 
> > 3.
> > If pre_enable fails, there is only a problem with the dsi module,
> > and
> > it does not affect the execution of other modules and enable funcs
> > under drm. 
> > Same goes for post_disable & disable.
> > 
> > Best Regrds!
> > xinlei
> > 
> 
> Hello Xinlei,
> 
> about failure for mtk_dsi_poweron(), it is all becuase of error
> setting
> for clock.
> If we do not set clock correctly and not enable DSI, after set DSI
> start, will it cause any issue? (maybe bus hang or something.)
> 
> Because in original drivers, if mtk_dsi_poweron failed, the DSI will
> not start.
> 
> IMO, it also needs some error message for mtk_dsi_bridge_pre_enable()
> and mtk_dsi_bridge_post_disable().
> 
> BRs,
> Rex
> 

Hi Rex:

Thanks for your review.
In the original dsi_poweron, if (++dsi->refcount != 1) is used to
determine whether poweron has been repeatedly run.
The poweron failure to execute DSI Start you mentioned may indeed lead
to Bus hang in extreme cases, so I will add a poweron-like judgment
mechanism before the next version of enable.
And will add some error message for mtk_dsi_bridge_pre_enable() and
mtk_dsi_bridge_post_disable().

Best Regards!
xinlei


WARNING: multiple messages have this Message-ID (diff)
From: xinlei.lee <xinlei.lee@mediatek.com>
To: Rex-BC Chen <rex-bc.chen@mediatek.com>, <chunkuang.hu@kernel.org>,
	<p.zabel@pengutronix.de>, <airlied@linux.ie>, <daniel@ffwll.ch>,
	<matthias.bgg@gmail.com>
Cc: <dri-devel@lists.freedesktop.org>,
	<linux-mediatek@lists.infradead.org>,
	<linux-arm-kernel@lists.infradead.org>,
	<linux-kernel@vger.kernel.org>,
	<Project_Global_Chrome_Upstream_Group@mediatek.com>,
	<jitao.shi@mediatek.com>
Subject: Re: [PATCH v3,2/4] drm/mediatek: Separate poweron/poweroff from enable/disable and define new funcs
Date: Thu, 7 Apr 2022 11:50:32 +0800	[thread overview]
Message-ID: <bdeeb164ad0144eafa6af2dc4ad1b6f7ce2a08e3.camel@mediatek.com> (raw)
In-Reply-To: <eca60f45ab518a53b1d281c078f562b8bc610060.camel@mediatek.com>

On Wed, 2022-03-23 at 19:46 +0800, Rex-BC Chen wrote:
> On Tue, 2022-03-22 at 17:23 +0800, xinlei.lee wrote:
> > On Thu, 2022-03-17 at 20:02 +0800, Rex-BC Chen wrote:
> > > Hello Xinlei,
> > > 
> > > On Thu, 2022-03-17 at 15:53 +0800, xinlei.lee@mediatek.com wrote:
> > > > From: Jitao Shi <jitao.shi@mediatek.com>
> > > > 
> > > > In order to match the changes of "Use the drm_panel_bridge
> > > > API",
> > > > the poweron/poweroff of dsi is extracted from enable/disable
> > > > and
> > > > defined as new funcs (pre_enable/post_disable).
> > > > 
> > > > Fixes: 2dd8075d2185 ("drm/mediatek: mtk_dsi: Use the
> > > > drm_panel_bridge
> > > > API")
> > > > 
> > > > Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
> > > > Signed-off-by: Xinlei Lee <xinlei.lee@mediatek.com>
> > > > ---
> > > >  drivers/gpu/drm/mediatek/mtk_dsi.c | 45 +++++++++++++++++-----
> > > > --
> > > > --
> > > > --
> > > > --
> > > >  1 file changed, 26 insertions(+), 19 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c
> > > > b/drivers/gpu/drm/mediatek/mtk_dsi.c
> > > > index 262c027d8c2f..e33caaca11a7 100644
> > > > --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> > > > +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> > > > @@ -679,16 +679,6 @@ static void mtk_dsi_poweroff(struct
> > > > mtk_dsi
> > > > *dsi)
> > > >  	if (--dsi->refcount != 0)
> > > >  		return;
> > > >  
> > > > -	/*
> > > > -	 * mtk_dsi_stop() and mtk_dsi_start() is asymmetric,
> > > > since
> > > > -	 * mtk_dsi_stop() should be called after
> > > > mtk_drm_crtc_atomic_disable(),
> > > > -	 * which needs irq for vblank, and mtk_dsi_stop() will
> > > > disable
> > > > irq.
> > > > -	 * mtk_dsi_start() needs to be called in
> > > > mtk_output_dsi_enable(),
> > > > -	 * after dsi is fully set.
> > > > -	 */
> > > > -	mtk_dsi_stop(dsi);
> > > > -
> > > > -	mtk_dsi_switch_to_cmd_mode(dsi, VM_DONE_INT_FLAG, 500);
> > > >  	mtk_dsi_reset_engine(dsi);
> > > >  	mtk_dsi_lane0_ulp_mode_enter(dsi);
> > > >  	mtk_dsi_clk_ulp_mode_enter(dsi);
> > > > @@ -703,17 +693,9 @@ static void mtk_dsi_poweroff(struct
> > > > mtk_dsi
> > > > *dsi)
> > > >  
> > > >  static void mtk_output_dsi_enable(struct mtk_dsi *dsi)
> > > >  {
> > > > -	int ret;
> > > > -
> > > >  	if (dsi->enabled)
> > > >  		return;
> > > >  
> > > > -	ret = mtk_dsi_poweron(dsi);
> > > > -	if (ret < 0) {
> > > > -		DRM_ERROR("failed to power on dsi\n");
> > > > -		return;
> > > > -	}
> > > > -
> > > >  	mtk_dsi_set_mode(dsi);
> > > >  	mtk_dsi_clk_hs_mode(dsi, 1);
> > > >  
> > > > @@ -727,7 +709,16 @@ static void mtk_output_dsi_disable(struct
> > > > mtk_dsi *dsi)
> > > >  	if (!dsi->enabled)
> > > >  		return;
> > > >  
> > > > -	mtk_dsi_poweroff(dsi);
> > > > +	/*
> > > > +	 * mtk_dsi_stop() and mtk_dsi_start() is asymmetric,
> > > > since
> > > 
> > > Why they are asymmetric?
> > > 
> > > > +	 * mtk_dsi_stop() should be called after
> > > > mtk_drm_crtc_atomic_disable(),
> > > > +	 * which needs irq for vblank, and mtk_dsi_stop() will
> > > > disable
> > > > irq.
> > > > +	 * mtk_dsi_start() needs to be called in
> > > > mtk_output_dsi_enable(),
> > > > +	 * after dsi is fully set.
> > > > +	 */
> > > > +	mtk_dsi_stop(dsi);
> > > > +
> > > > +	mtk_dsi_switch_to_cmd_mode(dsi, VM_DONE_INT_FLAG, 500);
> > > >  
> > > >  	dsi->enabled = false;
> > > >  }
> > > > @@ -765,10 +756,26 @@ static void mtk_dsi_bridge_enable(struct
> > > > drm_bridge *bridge)
> > > >  	mtk_output_dsi_enable(dsi);
> > > >  }
> > > >  
> > > > +static void mtk_dsi_bridge_pre_enable(struct drm_bridge
> > > > *bridge)
> > > > +{
> > > > +	struct mtk_dsi *dsi = bridge_to_dsi(bridge);
> > > > +
> > > > +	mtk_dsi_poweron(dsi);
> > > 
> > > Should you handle the error of mtk_dsi_poweron?
> > > If you failed to mtk_dsi_bridge_pre_enable and do
> > > mtk_dsi_bridge_enable,
> > > what will happend?
> > > 
> > > > +}
> > > > +
> > > > +static void mtk_dsi_bridge_post_disable(struct drm_bridge
> > > > *bridge)
> > > > +{
> > > > +	struct mtk_dsi *dsi = bridge_to_dsi(bridge);
> > > > +
> > > > +	mtk_dsi_poweroff(dsi);
> > > 
> > > If you failed to mtk_dsi_bridge_disable and you do
> > > mtk_dsi_bridge_post_disable,
> > > what will happend?
> > > Do you need to handle this?
> > > 
> > > BRs,
> > > Rex
> > > 
> > > > +}
> > > > +
> > > >  static const struct drm_bridge_funcs mtk_dsi_bridge_funcs = {
> > > >  	.attach = mtk_dsi_bridge_attach,
> > > >  	.disable = mtk_dsi_bridge_disable,
> > > >  	.enable = mtk_dsi_bridge_enable,
> > > > +	.pre_enable = mtk_dsi_bridge_pre_enable,
> > > > +	.post_disable = mtk_dsi_bridge_post_disable,
> > > >  	.mode_set = mtk_dsi_bridge_mode_set,
> > > >  };
> > > >  
> > > 
> > > 
> > 
> > Hi Rex:
> > 
> > Thanks for your review!
> > 
> > 1.Why they are asymmetric?
> > =>My understanding mtk_dsi_stop() and mtk_dsi_start() is to make
> > dsi
> > switch from LP11 and HS mode.DSI has two working modes:
> > If it is cmd mode, the data sent is sent by LP11, and dsi_start is
> > just
> > a signal. In this mode, dsi_stop is not required after sending cmd.
> > If it is video mode, because the data needs to be sent in HS mode,
> > dsi_start is required to make dsi enter HS mode from LP11. After
> > suspend, drm will call dsi_disable, and call dsi_stop to make dsi
> > return from HS mode to LP11 state.
> > Therefore mtk_dsi_stop() and mtk_dsi_start() are asymmetric.
> > For example, in the dsi_host_transfer function, only dsi_stop has
> > no
> > dsi_start operation.
> > 
> > 2.
> > Because the return type of pre_enable & post_disable in common code
> > is
> > void type. If there is an error, it will be processed in
> > poweron/poweroff, and the error message will be printed.
> > Do you mean that pre_enable & post_disable needs to accept the
> > poweron/poweroff error return value and then print the error log?
> > 
> > 3.
> > If pre_enable fails, there is only a problem with the dsi module,
> > and
> > it does not affect the execution of other modules and enable funcs
> > under drm. 
> > Same goes for post_disable & disable.
> > 
> > Best Regrds!
> > xinlei
> > 
> 
> Hello Xinlei,
> 
> about failure for mtk_dsi_poweron(), it is all becuase of error
> setting
> for clock.
> If we do not set clock correctly and not enable DSI, after set DSI
> start, will it cause any issue? (maybe bus hang or something.)
> 
> Because in original drivers, if mtk_dsi_poweron failed, the DSI will
> not start.
> 
> IMO, it also needs some error message for mtk_dsi_bridge_pre_enable()
> and mtk_dsi_bridge_post_disable().
> 
> BRs,
> Rex
> 

Hi Rex:

Thanks for your review.
In the original dsi_poweron, if (++dsi->refcount != 1) is used to
determine whether poweron has been repeatedly run.
The poweron failure to execute DSI Start you mentioned may indeed lead
to Bus hang in extreme cases, so I will add a poweron-like judgment
mechanism before the next version of enable.
And will add some error message for mtk_dsi_bridge_pre_enable() and
mtk_dsi_bridge_post_disable().

Best Regards!
xinlei
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

WARNING: multiple messages have this Message-ID (diff)
From: xinlei.lee <xinlei.lee@mediatek.com>
To: Rex-BC Chen <rex-bc.chen@mediatek.com>, <chunkuang.hu@kernel.org>,
	<p.zabel@pengutronix.de>, <airlied@linux.ie>, <daniel@ffwll.ch>,
	<matthias.bgg@gmail.com>
Cc: <dri-devel@lists.freedesktop.org>,
	<linux-mediatek@lists.infradead.org>,
	<linux-arm-kernel@lists.infradead.org>,
	<linux-kernel@vger.kernel.org>,
	<Project_Global_Chrome_Upstream_Group@mediatek.com>,
	<jitao.shi@mediatek.com>
Subject: Re: [PATCH v3,2/4] drm/mediatek: Separate poweron/poweroff from enable/disable and define new funcs
Date: Thu, 7 Apr 2022 11:50:32 +0800	[thread overview]
Message-ID: <bdeeb164ad0144eafa6af2dc4ad1b6f7ce2a08e3.camel@mediatek.com> (raw)
In-Reply-To: <eca60f45ab518a53b1d281c078f562b8bc610060.camel@mediatek.com>

On Wed, 2022-03-23 at 19:46 +0800, Rex-BC Chen wrote:
> On Tue, 2022-03-22 at 17:23 +0800, xinlei.lee wrote:
> > On Thu, 2022-03-17 at 20:02 +0800, Rex-BC Chen wrote:
> > > Hello Xinlei,
> > > 
> > > On Thu, 2022-03-17 at 15:53 +0800, xinlei.lee@mediatek.com wrote:
> > > > From: Jitao Shi <jitao.shi@mediatek.com>
> > > > 
> > > > In order to match the changes of "Use the drm_panel_bridge
> > > > API",
> > > > the poweron/poweroff of dsi is extracted from enable/disable
> > > > and
> > > > defined as new funcs (pre_enable/post_disable).
> > > > 
> > > > Fixes: 2dd8075d2185 ("drm/mediatek: mtk_dsi: Use the
> > > > drm_panel_bridge
> > > > API")
> > > > 
> > > > Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
> > > > Signed-off-by: Xinlei Lee <xinlei.lee@mediatek.com>
> > > > ---
> > > >  drivers/gpu/drm/mediatek/mtk_dsi.c | 45 +++++++++++++++++-----
> > > > --
> > > > --
> > > > --
> > > > --
> > > >  1 file changed, 26 insertions(+), 19 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c
> > > > b/drivers/gpu/drm/mediatek/mtk_dsi.c
> > > > index 262c027d8c2f..e33caaca11a7 100644
> > > > --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> > > > +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> > > > @@ -679,16 +679,6 @@ static void mtk_dsi_poweroff(struct
> > > > mtk_dsi
> > > > *dsi)
> > > >  	if (--dsi->refcount != 0)
> > > >  		return;
> > > >  
> > > > -	/*
> > > > -	 * mtk_dsi_stop() and mtk_dsi_start() is asymmetric,
> > > > since
> > > > -	 * mtk_dsi_stop() should be called after
> > > > mtk_drm_crtc_atomic_disable(),
> > > > -	 * which needs irq for vblank, and mtk_dsi_stop() will
> > > > disable
> > > > irq.
> > > > -	 * mtk_dsi_start() needs to be called in
> > > > mtk_output_dsi_enable(),
> > > > -	 * after dsi is fully set.
> > > > -	 */
> > > > -	mtk_dsi_stop(dsi);
> > > > -
> > > > -	mtk_dsi_switch_to_cmd_mode(dsi, VM_DONE_INT_FLAG, 500);
> > > >  	mtk_dsi_reset_engine(dsi);
> > > >  	mtk_dsi_lane0_ulp_mode_enter(dsi);
> > > >  	mtk_dsi_clk_ulp_mode_enter(dsi);
> > > > @@ -703,17 +693,9 @@ static void mtk_dsi_poweroff(struct
> > > > mtk_dsi
> > > > *dsi)
> > > >  
> > > >  static void mtk_output_dsi_enable(struct mtk_dsi *dsi)
> > > >  {
> > > > -	int ret;
> > > > -
> > > >  	if (dsi->enabled)
> > > >  		return;
> > > >  
> > > > -	ret = mtk_dsi_poweron(dsi);
> > > > -	if (ret < 0) {
> > > > -		DRM_ERROR("failed to power on dsi\n");
> > > > -		return;
> > > > -	}
> > > > -
> > > >  	mtk_dsi_set_mode(dsi);
> > > >  	mtk_dsi_clk_hs_mode(dsi, 1);
> > > >  
> > > > @@ -727,7 +709,16 @@ static void mtk_output_dsi_disable(struct
> > > > mtk_dsi *dsi)
> > > >  	if (!dsi->enabled)
> > > >  		return;
> > > >  
> > > > -	mtk_dsi_poweroff(dsi);
> > > > +	/*
> > > > +	 * mtk_dsi_stop() and mtk_dsi_start() is asymmetric,
> > > > since
> > > 
> > > Why they are asymmetric?
> > > 
> > > > +	 * mtk_dsi_stop() should be called after
> > > > mtk_drm_crtc_atomic_disable(),
> > > > +	 * which needs irq for vblank, and mtk_dsi_stop() will
> > > > disable
> > > > irq.
> > > > +	 * mtk_dsi_start() needs to be called in
> > > > mtk_output_dsi_enable(),
> > > > +	 * after dsi is fully set.
> > > > +	 */
> > > > +	mtk_dsi_stop(dsi);
> > > > +
> > > > +	mtk_dsi_switch_to_cmd_mode(dsi, VM_DONE_INT_FLAG, 500);
> > > >  
> > > >  	dsi->enabled = false;
> > > >  }
> > > > @@ -765,10 +756,26 @@ static void mtk_dsi_bridge_enable(struct
> > > > drm_bridge *bridge)
> > > >  	mtk_output_dsi_enable(dsi);
> > > >  }
> > > >  
> > > > +static void mtk_dsi_bridge_pre_enable(struct drm_bridge
> > > > *bridge)
> > > > +{
> > > > +	struct mtk_dsi *dsi = bridge_to_dsi(bridge);
> > > > +
> > > > +	mtk_dsi_poweron(dsi);
> > > 
> > > Should you handle the error of mtk_dsi_poweron?
> > > If you failed to mtk_dsi_bridge_pre_enable and do
> > > mtk_dsi_bridge_enable,
> > > what will happend?
> > > 
> > > > +}
> > > > +
> > > > +static void mtk_dsi_bridge_post_disable(struct drm_bridge
> > > > *bridge)
> > > > +{
> > > > +	struct mtk_dsi *dsi = bridge_to_dsi(bridge);
> > > > +
> > > > +	mtk_dsi_poweroff(dsi);
> > > 
> > > If you failed to mtk_dsi_bridge_disable and you do
> > > mtk_dsi_bridge_post_disable,
> > > what will happend?
> > > Do you need to handle this?
> > > 
> > > BRs,
> > > Rex
> > > 
> > > > +}
> > > > +
> > > >  static const struct drm_bridge_funcs mtk_dsi_bridge_funcs = {
> > > >  	.attach = mtk_dsi_bridge_attach,
> > > >  	.disable = mtk_dsi_bridge_disable,
> > > >  	.enable = mtk_dsi_bridge_enable,
> > > > +	.pre_enable = mtk_dsi_bridge_pre_enable,
> > > > +	.post_disable = mtk_dsi_bridge_post_disable,
> > > >  	.mode_set = mtk_dsi_bridge_mode_set,
> > > >  };
> > > >  
> > > 
> > > 
> > 
> > Hi Rex:
> > 
> > Thanks for your review!
> > 
> > 1.Why they are asymmetric?
> > =>My understanding mtk_dsi_stop() and mtk_dsi_start() is to make
> > dsi
> > switch from LP11 and HS mode.DSI has two working modes:
> > If it is cmd mode, the data sent is sent by LP11, and dsi_start is
> > just
> > a signal. In this mode, dsi_stop is not required after sending cmd.
> > If it is video mode, because the data needs to be sent in HS mode,
> > dsi_start is required to make dsi enter HS mode from LP11. After
> > suspend, drm will call dsi_disable, and call dsi_stop to make dsi
> > return from HS mode to LP11 state.
> > Therefore mtk_dsi_stop() and mtk_dsi_start() are asymmetric.
> > For example, in the dsi_host_transfer function, only dsi_stop has
> > no
> > dsi_start operation.
> > 
> > 2.
> > Because the return type of pre_enable & post_disable in common code
> > is
> > void type. If there is an error, it will be processed in
> > poweron/poweroff, and the error message will be printed.
> > Do you mean that pre_enable & post_disable needs to accept the
> > poweron/poweroff error return value and then print the error log?
> > 
> > 3.
> > If pre_enable fails, there is only a problem with the dsi module,
> > and
> > it does not affect the execution of other modules and enable funcs
> > under drm. 
> > Same goes for post_disable & disable.
> > 
> > Best Regrds!
> > xinlei
> > 
> 
> Hello Xinlei,
> 
> about failure for mtk_dsi_poweron(), it is all becuase of error
> setting
> for clock.
> If we do not set clock correctly and not enable DSI, after set DSI
> start, will it cause any issue? (maybe bus hang or something.)
> 
> Because in original drivers, if mtk_dsi_poweron failed, the DSI will
> not start.
> 
> IMO, it also needs some error message for mtk_dsi_bridge_pre_enable()
> and mtk_dsi_bridge_post_disable().
> 
> BRs,
> Rex
> 

Hi Rex:

Thanks for your review.
In the original dsi_poweron, if (++dsi->refcount != 1) is used to
determine whether poweron has been repeatedly run.
The poweron failure to execute DSI Start you mentioned may indeed lead
to Bus hang in extreme cases, so I will add a poweron-like judgment
mechanism before the next version of enable.
And will add some error message for mtk_dsi_bridge_pre_enable() and
mtk_dsi_bridge_post_disable().

Best Regards!
xinlei
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

  reply	other threads:[~2022-04-07  3:50 UTC|newest]

Thread overview: 64+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-03-17  7:53 [PATCH v3,0/4] Cooperate with DSI RX devices to modify dsi funcs and delay mipi high to cooperate with panel sequence xinlei.lee
2022-03-17  7:53 ` [PATCH v3, 0/4] " xinlei.lee
2022-03-17  7:53 ` xinlei.lee
2022-03-17  7:53 ` xinlei.lee
2022-03-17  7:53 ` [PATCH v3,1/4] drm/mediatek: Adjust the timing of mipi signal from LP00 to LP11 xinlei.lee
2022-03-17  7:53   ` [PATCH v3, 1/4] " xinlei.lee
2022-03-17  7:53   ` xinlei.lee
2022-03-17  7:53   ` xinlei.lee
2022-03-17 11:13   ` [PATCH v3,1/4] " Rex-BC Chen
2022-03-17 11:13     ` Rex-BC Chen
2022-03-17 11:13     ` Rex-BC Chen
2022-03-17 11:13     ` Rex-BC Chen
2022-03-22  5:59     ` xinlei.lee
2022-03-22  5:59       ` xinlei.lee
2022-03-22  5:59       ` xinlei.lee
2022-03-21  9:36   ` [PATCH v3, 1/4] " CK Hu
2022-03-21  9:36     ` CK Hu
2022-03-21  9:36     ` CK Hu
2022-03-21  9:36     ` CK Hu
2022-03-22  6:16     ` xinlei.lee
2022-03-22  6:16       ` xinlei.lee
2022-03-22  6:16       ` xinlei.lee
2022-03-17  7:53 ` [PATCH v3,2/4] drm/mediatek: Separate poweron/poweroff from enable/disable and define new funcs xinlei.lee
2022-03-17  7:53   ` [PATCH v3, 2/4] " xinlei.lee
2022-03-17  7:53   ` xinlei.lee
2022-03-17  7:53   ` xinlei.lee
2022-03-17 12:02   ` [PATCH v3,2/4] " Rex-BC Chen
2022-03-17 12:02     ` Rex-BC Chen
2022-03-17 12:02     ` Rex-BC Chen
2022-03-17 12:02     ` Rex-BC Chen
2022-03-22  9:23     ` xinlei.lee
2022-03-22  9:23       ` xinlei.lee
2022-03-22  9:23       ` xinlei.lee
2022-03-23 11:46       ` Rex-BC Chen
2022-03-23 11:46         ` Rex-BC Chen
2022-03-23 11:46         ` Rex-BC Chen
2022-04-07  3:50         ` xinlei.lee [this message]
2022-04-07  3:50           ` xinlei.lee
2022-04-07  3:50           ` xinlei.lee
2022-03-17  7:53 ` [PATCH v3,3/4] drm/mediatek: keep dsi as LP00 before dcs cmds transfer xinlei.lee
2022-03-17  7:53   ` [PATCH v3, 3/4] " xinlei.lee
2022-03-17  7:53   ` xinlei.lee
2022-03-17  7:53   ` xinlei.lee
2022-03-17 12:06   ` [PATCH v3,3/4] " Rex-BC Chen
2022-03-17 12:06     ` Rex-BC Chen
2022-03-17 12:06     ` Rex-BC Chen
2022-03-17 12:06     ` Rex-BC Chen
2022-03-22  9:46     ` xinlei.lee
2022-03-22  9:46       ` xinlei.lee
2022-03-22  9:46       ` xinlei.lee
2022-03-17  7:53 ` [PATCH v3,4/4] drm/mediatek: Add pull-down MIPI operation in mtk_dsi_poweroff function xinlei.lee
2022-03-17  7:53   ` [PATCH v3, 4/4] " xinlei.lee
2022-03-17  7:53   ` xinlei.lee
2022-03-17  7:53   ` xinlei.lee
2022-03-17 12:20   ` [PATCH v3,4/4] " Rex-BC Chen
2022-03-17 12:20     ` Rex-BC Chen
2022-03-17 12:20     ` Rex-BC Chen
2022-03-17 12:20     ` Rex-BC Chen
2022-03-22 10:00     ` xinlei.lee
2022-03-22 10:00       ` xinlei.lee
2022-03-22 10:00       ` xinlei.lee
2022-03-23 11:54       ` Rex-BC Chen
2022-03-23 11:54         ` Rex-BC Chen
2022-03-23 11:54         ` Rex-BC Chen

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=bdeeb164ad0144eafa6af2dc4ad1b6f7ce2a08e3.camel@mediatek.com \
    --to=xinlei.lee@mediatek.com \
    --cc=Project_Global_Chrome_Upstream_Group@mediatek.com \
    --cc=airlied@linux.ie \
    --cc=chunkuang.hu@kernel.org \
    --cc=daniel@ffwll.ch \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=jitao.shi@mediatek.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mediatek@lists.infradead.org \
    --cc=matthias.bgg@gmail.com \
    --cc=p.zabel@pengutronix.de \
    --cc=rex-bc.chen@mediatek.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.