| Message ID | 20200424063551.14336-1-bernard@vivo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] drm/arm: fixes pixel clock enabled with wrong format | expand |
Hi Bernard, On Fri, 24 Apr 2020 at 08:15, Bernard Zhao <bernard@vivo.com> wrote: > > The pixel clock is still enabled when the format is wrong. > no error branch handle, and also some register is not set > in this case, e.g: HDLCD_REG_<color>_SELECT. Maybe we > should disable this clock and throw an warn message when > this happened. > With this change, the code maybe a bit more readable. > > Signed-off-by: Bernard Zhao <bernard@vivo.com> > > Changes since V1: > *add format error handle, if format is not correct, throw > an warning message and disable this clock. > > Link for V1: > *https://lore.kernel.org/patchwork/patch/1228501/ > --- > drivers/gpu/drm/arm/hdlcd_crtc.c | 13 +++++++++---- > 1 file changed, 9 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/arm/hdlcd_crtc.c b/drivers/gpu/drm/arm/hdlcd_crtc.c > index af67fefed38d..f3945dee2b7d 100644 > --- a/drivers/gpu/drm/arm/hdlcd_crtc.c > +++ b/drivers/gpu/drm/arm/hdlcd_crtc.c > @@ -96,7 +96,7 @@ static int hdlcd_set_pxl_fmt(struct drm_crtc *crtc) > } > > if (WARN_ON(!format)) > - return 0; > + return -EINVAL; > > /* HDLCD uses 'bytes per pixel', zero means 1 byte */ > btpp = (format->bits_per_pixel + 7) / 8; > @@ -125,7 +125,7 @@ static int hdlcd_set_pxl_fmt(struct drm_crtc *crtc) > return 0; > } > > -static void hdlcd_crtc_mode_set_nofb(struct drm_crtc *crtc) > +static int hdlcd_crtc_mode_set_nofb(struct drm_crtc *crtc) > { > struct hdlcd_drm_private *hdlcd = crtc_to_hdlcd_priv(crtc); > struct drm_display_mode *m = &crtc->state->adjusted_mode; > @@ -162,9 +162,10 @@ static void hdlcd_crtc_mode_set_nofb(struct drm_crtc *crtc) > > err = hdlcd_set_pxl_fmt(crtc); > if (err) > - return; > + return err; > > clk_set_rate(hdlcd->clk, m->crtc_clock * 1000); > + return 0; > } > > static void hdlcd_crtc_atomic_enable(struct drm_crtc *crtc, > @@ -173,7 +174,11 @@ static void hdlcd_crtc_atomic_enable(struct drm_crtc *crtc, > struct hdlcd_drm_private *hdlcd = crtc_to_hdlcd_priv(crtc); > > clk_prepare_enable(hdlcd->clk); > - hdlcd_crtc_mode_set_nofb(crtc); > + if (hdlcd_crtc_mode_set_nofb(crtc)) { > + DRM_DEBUG_KMS("Invalid format, pixel clock enable failed!\n"); > + clk_disable_unprepare(hdlcd->clk); > + return; > + } This doesn't seem right. As far as I can tell, the state must be checked in the .atomic_check since .atomic_enable, intentionally, "cannot" fail. HTH Emil > hdlcd_write(hdlcd, HDLCD_REG_COMMAND, 1); > drm_crtc_vblank_on(crtc); > } > -- > 2.26.2 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi Bernand, On Thu, Apr 23, 2020 at 11:35:51PM -0700, Bernard Zhao wrote: > The pixel clock is still enabled when the format is wrong. > no error branch handle, and also some register is not set > in this case, e.g: HDLCD_REG_<color>_SELECT. Maybe we > should disable this clock and throw an warn message when > this happened. > With this change, the code maybe a bit more readable. > > Signed-off-by: Bernard Zhao <bernard@vivo.com> > > Changes since V1: > *add format error handle, if format is not correct, throw > an warning message and disable this clock. > > Link for V1: > *https://lore.kernel.org/patchwork/patch/1228501/ > --- > drivers/gpu/drm/arm/hdlcd_crtc.c | 13 +++++++++---- > 1 file changed, 9 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/arm/hdlcd_crtc.c b/drivers/gpu/drm/arm/hdlcd_crtc.c > index af67fefed38d..f3945dee2b7d 100644 > --- a/drivers/gpu/drm/arm/hdlcd_crtc.c > +++ b/drivers/gpu/drm/arm/hdlcd_crtc.c > @@ -96,7 +96,7 @@ static int hdlcd_set_pxl_fmt(struct drm_crtc *crtc) > } > > if (WARN_ON(!format)) > - return 0; > + return -EINVAL; That is the right fix! > > /* HDLCD uses 'bytes per pixel', zero means 1 byte */ > btpp = (format->bits_per_pixel + 7) / 8; > @@ -125,7 +125,7 @@ static int hdlcd_set_pxl_fmt(struct drm_crtc *crtc) > return 0; > } > > -static void hdlcd_crtc_mode_set_nofb(struct drm_crtc *crtc) > +static int hdlcd_crtc_mode_set_nofb(struct drm_crtc *crtc) But this is not. We don't need to propagate the error further, just .... > { > struct hdlcd_drm_private *hdlcd = crtc_to_hdlcd_priv(crtc); > struct drm_display_mode *m = &crtc->state->adjusted_mode; > @@ -162,9 +162,10 @@ static void hdlcd_crtc_mode_set_nofb(struct drm_crtc *crtc) > > err = hdlcd_set_pxl_fmt(crtc); > if (err) > - return; ... return here so that we don't call clk_set_rate(); Best regards, Liviu > + return err; > > clk_set_rate(hdlcd->clk, m->crtc_clock * 1000); > + return 0; > } > > static void hdlcd_crtc_atomic_enable(struct drm_crtc *crtc, > @@ -173,7 +174,11 @@ static void hdlcd_crtc_atomic_enable(struct drm_crtc *crtc, > struct hdlcd_drm_private *hdlcd = crtc_to_hdlcd_priv(crtc); > > clk_prepare_enable(hdlcd->clk); > - hdlcd_crtc_mode_set_nofb(crtc); > + if (hdlcd_crtc_mode_set_nofb(crtc)) { > + DRM_DEBUG_KMS("Invalid format, pixel clock enable failed!\n"); > + clk_disable_unprepare(hdlcd->clk); > + return; > + } > hdlcd_write(hdlcd, HDLCD_REG_COMMAND, 1); > drm_crtc_vblank_on(crtc); > } > -- > 2.26.2 >
From: Liviu Dudau <liviu.dudau@arm.com> Date: 2020-04-24 19:09:50 To: Bernard Zhao <bernard@vivo.com> Cc: Brian Starkey <brian.starkey@arm.com>,David Airlie <airlied@linux.ie>,Daniel Vetter <daniel@ffwll.ch>,dri-devel@lists.freedesktop.org,linux-kernel@vger.kernel.org,opensource.kernel@vivo.com Subject: Re: [PATCH v2] drm/arm: fixes pixel clock enabled with wrong format>Hi Bernand, > >On Thu, Apr 23, 2020 at 11:35:51PM -0700, Bernard Zhao wrote: >> The pixel clock is still enabled when the format is wrong. >> no error branch handle, and also some register is not set >> in this case, e.g: HDLCD_REG_<color>_SELECT. Maybe we >> should disable this clock and throw an warn message when >> this happened. >> With this change, the code maybe a bit more readable. >> >> Signed-off-by: Bernard Zhao <bernard@vivo.com> >> >> Changes since V1: >> *add format error handle, if format is not correct, throw >> an warning message and disable this clock. >> >> Link for V1: >> *https://lore.kernel.org/patchwork/patch/1228501/ >> --- >> drivers/gpu/drm/arm/hdlcd_crtc.c | 13 +++++++++---- >> 1 file changed, 9 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/gpu/drm/arm/hdlcd_crtc.c b/drivers/gpu/drm/arm/hdlcd_crtc.c >> index af67fefed38d..f3945dee2b7d 100644 >> --- a/drivers/gpu/drm/arm/hdlcd_crtc.c >> +++ b/drivers/gpu/drm/arm/hdlcd_crtc.c >> @@ -96,7 +96,7 @@ static int hdlcd_set_pxl_fmt(struct drm_crtc *crtc) >> } >> >> if (WARN_ON(!format)) >> - return 0; >> + return -EINVAL; > >That is the right fix! > >> >> /* HDLCD uses 'bytes per pixel', zero means 1 byte */ >> btpp = (format->bits_per_pixel + 7) / 8; >> @@ -125,7 +125,7 @@ static int hdlcd_set_pxl_fmt(struct drm_crtc *crtc) >> return 0; >> } >> >> -static void hdlcd_crtc_mode_set_nofb(struct drm_crtc *crtc) >> +static int hdlcd_crtc_mode_set_nofb(struct drm_crtc *crtc) > >But this is not. We don't need to propagate the error further, just .... > >> { >> struct hdlcd_drm_private *hdlcd = crtc_to_hdlcd_priv(crtc); >> struct drm_display_mode *m = &crtc->state->adjusted_mode; >> @@ -162,9 +162,10 @@ static void hdlcd_crtc_mode_set_nofb(struct drm_crtc *crtc) >> >> err = hdlcd_set_pxl_fmt(crtc); >> if (err) >> - return; >My previous understanding was that when such an exception occurred, it was caught in the atomic_enable interface, and then disable pixel clock. I am not sure is this ok or i have to do more register clean operaction. >... return here so that we don't call clk_set_rate(); And for this part, i am a little confused : 1 clk_set_rate must be set even if format is wrong? 2 The original code logic shows that If format is not correct, we will not set registers HDLCD_REG_PIXEL_FORMAT & HDLCD_REG_<color>_SELECT, will this bring in any problems? 3 if 1 the rate must set & 2 registers above doesn`t matter, then maybe there is no need to disable pixel clock. Regards, Bernard >> + return err; >> >> clk_set_rate(hdlcd->clk, m->crtc_clock * 1000); >> + return 0; >> } >> >> static void hdlcd_crtc_atomic_enable(struct drm_crtc *crtc, >> @@ -173,7 +174,11 @@ static void hdlcd_crtc_atomic_enable(struct drm_crtc *crtc, >> struct hdlcd_drm_private *hdlcd = crtc_to_hdlcd_priv(crtc); >> >> clk_prepare_enable(hdlcd->clk); >> - hdlcd_crtc_mode_set_nofb(crtc); >> + if (hdlcd_crtc_mode_set_nofb(crtc)) { >> + DRM_DEBUG_KMS("Invalid format, pixel clock enable failed!\n"); >> + clk_disable_unprepare(hdlcd->clk); >> + return; >> + } >> hdlcd_write(hdlcd, HDLCD_REG_COMMAND, 1); >> drm_crtc_vblank_on(crtc); >> } >> -- >> 2.26.2 >> > >-- >==================== >| I would like to | >| fix the world, | >| but they're not | >| giving me the | > \ source code! / > --------------- > ¯\_(ツ)_/¯
From: "赵军奎" <bernard@vivo.com> Date: 2020-04-24 19:37:36 To: Liviu Dudau <liviu.dudau@arm.com> Cc: Brian Starkey <brian.starkey@arm.com>,David Airlie <airlied@linux.ie>,Daniel Vetter <daniel@ffwll.ch>,dri-devel@lists.freedesktop.org,linux-kernel@vger.kernel.org,opensource.kernel@vivo.com Subject: Re:Re: [PATCH v2] drm/arm: fixes pixel clock enabled with wrong format From: Liviu Dudau <liviu.dudau@arm.com> Date: 2020-04-24 19:09:50 To: Bernard Zhao <bernard@vivo.com> Cc: Brian Starkey <brian.starkey@arm.com>,David Airlie <airlied@linux.ie>,Daniel Vetter <daniel@ffwll.ch>,dri-devel@lists.freedesktop.org,linux-kernel@vger.kernel.org,opensource.kernel@vivo.com Subject: Re: [PATCH v2] drm/arm: fixes pixel clock enabled with wrong format>Hi Bernand, > >On Thu, Apr 23, 2020 at 11:35:51PM -0700, Bernard Zhao wrote: >> The pixel clock is still enabled when the format is wrong. >> no error branch handle, and also some register is not set >> in this case, e.g: HDLCD_REG_<color>_SELECT. Maybe we >> should disable this clock and throw an warn message when >> this happened. >> With this change, the code maybe a bit more readable. >> >> Signed-off-by: Bernard Zhao <bernard@vivo.com> >> >> Changes since V1: >> *add format error handle, if format is not correct, throw >> an warning message and disable this clock. >> >> Link for V1: >> *https://lore.kernel.org/patchwork/patch/1228501/ >> --- >> drivers/gpu/drm/arm/hdlcd_crtc.c | 13 +++++++++---- >> 1 file changed, 9 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/gpu/drm/arm/hdlcd_crtc.c b/drivers/gpu/drm/arm/hdlcd_crtc.c >> index af67fefed38d..f3945dee2b7d 100644 >> --- a/drivers/gpu/drm/arm/hdlcd_crtc.c >> +++ b/drivers/gpu/drm/arm/hdlcd_crtc.c >> @@ -96,7 +96,7 @@ static int hdlcd_set_pxl_fmt(struct drm_crtc *crtc) >> } >> >> if (WARN_ON(!format)) >> - return 0; >> + return -EINVAL; > >That is the right fix! > >> >> /* HDLCD uses 'bytes per pixel', zero means 1 byte */ >> btpp = (format->bits_per_pixel + 7) / 8; >> @@ -125,7 +125,7 @@ static int hdlcd_set_pxl_fmt(struct drm_crtc *crtc) >> return 0; >> } >> >> -static void hdlcd_crtc_mode_set_nofb(struct drm_crtc *crtc) >> +static int hdlcd_crtc_mode_set_nofb(struct drm_crtc *crtc) > >But this is not. We don't need to propagate the error further, just .... > >> { >> struct hdlcd_drm_private *hdlcd = crtc_to_hdlcd_priv(crtc); >> struct drm_display_mode *m = &crtc->state->adjusted_mode; >> @@ -162,9 +162,10 @@ static void hdlcd_crtc_mode_set_nofb(struct drm_crtc *crtc) >> >> err = hdlcd_set_pxl_fmt(crtc); >> if (err) >> - return; > My previous understanding was that when such an exception occurred, it was caught in the atomic_enable interface, and then disable pixel clock. I am not sure is this ok or i have to do more register clean operaction. >... return here so that we don't call clk_set_rate(); And for this part, i am a little confused : 1 clk_set_rate must be set even if format is wrong? 2 The original code logic shows that If format is not correct, we will not set registers HDLCD_REG_PIXEL_FORMAT & HDLCD_REG_<color>_SELECT, will this bring in any problems? 3 if 1 the rate must set & 2 registers above doesn`t matter, then maybe there is no need to disable pixel clock. Am i misunderstanding Regards, Bernard >> + return err; >> >> clk_set_rate(hdlcd->clk, m->crtc_clock * 1000); >> + return 0; >> } >> >> static void hdlcd_crtc_atomic_enable(struct drm_crtc *crtc, >> @@ -173,7 +174,11 @@ static void hdlcd_crtc_atomic_enable(struct drm_crtc *crtc, >> struct hdlcd_drm_private *hdlcd = crtc_to_hdlcd_priv(crtc); >> >> clk_prepare_enable(hdlcd->clk); >> - hdlcd_crtc_mode_set_nofb(crtc); >> + if (hdlcd_crtc_mode_set_nofb(crtc)) { >> + DRM_DEBUG_KMS("Invalid format, pixel clock enable failed!\n"); >> + clk_disable_unprepare(hdlcd->clk); >> + return; >> + } >> hdlcd_write(hdlcd, HDLCD_REG_COMMAND, 1); >> drm_crtc_vblank_on(crtc); >> } >> -- >> 2.26.2 >> > >-- >==================== >| I would like to | >| fix the world, | >| but they're not | >| giving me the | > \ source code! / > --------------- > ¯\_(ツ)_/¯
Hi Bernard, On Fri, May 08, 2020 at 04:47:17PM +0800, Bernard wrote: > From: "赵军奎" <bernard@vivo.com> > Date: 2020-04-24 19:37:36 > To: Liviu Dudau <liviu.dudau@arm.com> > Cc: Brian Starkey <brian.starkey@arm.com>,David Airlie <airlied@linux.ie>,Daniel Vetter <daniel@ffwll.ch>,dri-devel@lists.freedesktop.org,linux-kernel@vger.kernel.org,opensource.kernel@vivo.com > Subject: Re:Re: [PATCH v2] drm/arm: fixes pixel clock enabled with wrong format > > > > > From: Liviu Dudau <liviu.dudau@arm.com> > Date: 2020-04-24 19:09:50 > To: Bernard Zhao <bernard@vivo.com> > Cc: Brian Starkey <brian.starkey@arm.com>,David Airlie <airlied@linux.ie>,Daniel Vetter <daniel@ffwll.ch>,dri-devel@lists.freedesktop.org,linux-kernel@vger.kernel.org,opensource.kernel@vivo.com > Subject: Re: [PATCH v2] drm/arm: fixes pixel clock enabled with wrong format>Hi Bernand, > > > >On Thu, Apr 23, 2020 at 11:35:51PM -0700, Bernard Zhao wrote: > >> The pixel clock is still enabled when the format is wrong. > >> no error branch handle, and also some register is not set > >> in this case, e.g: HDLCD_REG_<color>_SELECT. Maybe we > >> should disable this clock and throw an warn message when > >> this happened. > >> With this change, the code maybe a bit more readable. > >> > >> Signed-off-by: Bernard Zhao <bernard@vivo.com> > >> > >> Changes since V1: > >> *add format error handle, if format is not correct, throw > >> an warning message and disable this clock. > >> > >> Link for V1: > >> *https://lore.kernel.org/patchwork/patch/1228501/ > >> --- > >> drivers/gpu/drm/arm/hdlcd_crtc.c | 13 +++++++++---- > >> 1 file changed, 9 insertions(+), 4 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/arm/hdlcd_crtc.c b/drivers/gpu/drm/arm/hdlcd_crtc.c > >> index af67fefed38d..f3945dee2b7d 100644 > >> --- a/drivers/gpu/drm/arm/hdlcd_crtc.c > >> +++ b/drivers/gpu/drm/arm/hdlcd_crtc.c > >> @@ -96,7 +96,7 @@ static int hdlcd_set_pxl_fmt(struct drm_crtc *crtc) > >> } > >> > >> if (WARN_ON(!format)) > >> - return 0; > >> + return -EINVAL; > > > >That is the right fix! > > > >> > >> /* HDLCD uses 'bytes per pixel', zero means 1 byte */ > >> btpp = (format->bits_per_pixel + 7) / 8; > >> @@ -125,7 +125,7 @@ static int hdlcd_set_pxl_fmt(struct drm_crtc *crtc) > >> return 0; > >> } > >> > >> -static void hdlcd_crtc_mode_set_nofb(struct drm_crtc *crtc) > >> +static int hdlcd_crtc_mode_set_nofb(struct drm_crtc *crtc) > > > >But this is not. We don't need to propagate the error further, just .... > > > >> { > >> struct hdlcd_drm_private *hdlcd = crtc_to_hdlcd_priv(crtc); > >> struct drm_display_mode *m = &crtc->state->adjusted_mode; > >> @@ -162,9 +162,10 @@ static void hdlcd_crtc_mode_set_nofb(struct drm_crtc *crtc) > >> > >> err = hdlcd_set_pxl_fmt(crtc); > >> if (err) > >> - return; > > > > My previous understanding was that when such an exception occurred, it was caught > in the atomic_enable interface, and then disable pixel clock. I am not sure is this ok or > i have to do more register clean operaction. > > >... return here so that we don't call clk_set_rate(); > And for this part, i am a little confused : > 1 clk_set_rate must be set even if format is wrong? > 2 The original code logic shows that If format is not correct, we will not set registers > HDLCD_REG_PIXEL_FORMAT & HDLCD_REG_<color>_SELECT, will this bring in > any problems? > 3 if 1 the rate must set & 2 registers above doesn`t matter, then maybe there is no > need to disable pixel clock. > Am i misunderstanding You are right, the pixel format check should not happen inside hdlcd_crtc_atomic_enable() hook, it should be moved into a separate function hdlcd_crtc_atomic_check() and that needs to be hooked into .atomic_check() for hdlcd_crtc_helper_funcs(). If you want to have another go I'll be happy to review and Ack your patch. Best regards, Liviu > > Regards, > Bernard > > >> + return err; > >> > >> clk_set_rate(hdlcd->clk, m->crtc_clock * 1000); > >> + return 0; > >> } > >> > >> static void hdlcd_crtc_atomic_enable(struct drm_crtc *crtc, > >> @@ -173,7 +174,11 @@ static void hdlcd_crtc_atomic_enable(struct drm_crtc *crtc, > >> struct hdlcd_drm_private *hdlcd = crtc_to_hdlcd_priv(crtc); > >> > >> clk_prepare_enable(hdlcd->clk); > >> - hdlcd_crtc_mode_set_nofb(crtc); > >> + if (hdlcd_crtc_mode_set_nofb(crtc)) { > >> + DRM_DEBUG_KMS("Invalid format, pixel clock enable failed!\n"); > >> + clk_disable_unprepare(hdlcd->clk); > >> + return; > >> + } > >> hdlcd_write(hdlcd, HDLCD_REG_COMMAND, 1); > >> drm_crtc_vblank_on(crtc); > >> } > >> -- > >> 2.26.2 > >> > > > >-- > >==================== > >| I would like to | > >| fix the world, | > >| but they're not | > >| giving me the | > > \ source code! / > > --------------- > > ¯\_(ツ)_/¯ > > >
发件人：Liviu Dudau <liviu.dudau@arm.com> 发送日期：2020-05-15 22:41:49 收件人：Bernard <bernard@vivo.com> 抄送人：Brian Starkey <brian.starkey@arm.com>,David Airlie <airlied@linux.ie>,Daniel Vetter <daniel@ffwll.ch>,dri-devel@lists.freedesktop.org,linux-kernel@vger.kernel.org,opensource.kernel@vivo.com 主题：Re: Re:Re: [PATCH v2] drm/arm: fixes pixel clock enabled with wrong format>Hi Bernard, > >On Fri, May 08, 2020 at 04:47:17PM +0800, Bernard wrote: >> From: "赵军奎" <bernard@vivo.com> >> Date: 2020-04-24 19:37:36 >> To: Liviu Dudau <liviu.dudau@arm.com> >> Cc: Brian Starkey <brian.starkey@arm.com>,David Airlie <airlied@linux.ie>,Daniel Vetter <daniel@ffwll.ch>,dri-devel@lists.freedesktop.org,linux-kernel@vger.kernel.org,opensource.kernel@vivo.com >> Subject: Re:Re: [PATCH v2] drm/arm: fixes pixel clock enabled with wrong format >> >> >> >> >> From: Liviu Dudau <liviu.dudau@arm.com> >> Date: 2020-04-24 19:09:50 >> To: Bernard Zhao <bernard@vivo.com> >> Cc: Brian Starkey <brian.starkey@arm.com>,David Airlie <airlied@linux.ie>,Daniel Vetter <daniel@ffwll.ch>,dri-devel@lists.freedesktop.org,linux-kernel@vger.kernel.org,opensource.kernel@vivo.com >> Subject: Re: [PATCH v2] drm/arm: fixes pixel clock enabled with wrong format>Hi Bernand, >> > >> >On Thu, Apr 23, 2020 at 11:35:51PM -0700, Bernard Zhao wrote: >> >> The pixel clock is still enabled when the format is wrong. >> >> no error branch handle, and also some register is not set >> >> in this case, e.g: HDLCD_REG_<color>_SELECT. Maybe we >> >> should disable this clock and throw an warn message when >> >> this happened. >> >> With this change, the code maybe a bit more readable. >> >> >> >> Signed-off-by: Bernard Zhao <bernard@vivo.com> >> >> >> >> Changes since V1: >> >> *add format error handle, if format is not correct, throw >> >> an warning message and disable this clock. >> >> >> >> Link for V1: >> >> *https://lore.kernel.org/patchwork/patch/1228501/ >> >> --- >> >> drivers/gpu/drm/arm/hdlcd_crtc.c | 13 +++++++++---- >> >> 1 file changed, 9 insertions(+), 4 deletions(-) >> >> >> >> diff --git a/drivers/gpu/drm/arm/hdlcd_crtc.c b/drivers/gpu/drm/arm/hdlcd_crtc.c >> >> index af67fefed38d..f3945dee2b7d 100644 >> >> --- a/drivers/gpu/drm/arm/hdlcd_crtc.c >> >> +++ b/drivers/gpu/drm/arm/hdlcd_crtc.c >> >> @@ -96,7 +96,7 @@ static int hdlcd_set_pxl_fmt(struct drm_crtc *crtc) >> >> } >> >> >> >> if (WARN_ON(!format)) >> >> - return 0; >> >> + return -EINVAL; >> > >> >That is the right fix! >> > >> >> >> >> /* HDLCD uses 'bytes per pixel', zero means 1 byte */ >> >> btpp = (format->bits_per_pixel + 7) / 8; >> >> @@ -125,7 +125,7 @@ static int hdlcd_set_pxl_fmt(struct drm_crtc *crtc) >> >> return 0; >> >> } >> >> >> >> -static void hdlcd_crtc_mode_set_nofb(struct drm_crtc *crtc) >> >> +static int hdlcd_crtc_mode_set_nofb(struct drm_crtc *crtc) >> > >> >But this is not. We don't need to propagate the error further, just .... >> > >> >> { >> >> struct hdlcd_drm_private *hdlcd = crtc_to_hdlcd_priv(crtc); >> >> struct drm_display_mode *m = &crtc->state->adjusted_mode; >> >> @@ -162,9 +162,10 @@ static void hdlcd_crtc_mode_set_nofb(struct drm_crtc *crtc) >> >> >> >> err = hdlcd_set_pxl_fmt(crtc); >> >> if (err) >> >> - return; >> > >> >> My previous understanding was that when such an exception occurred, it was caught >> in the atomic_enable interface, and then disable pixel clock. I am not sure is this ok or >> i have to do more register clean operaction. >> >> >... return here so that we don't call clk_set_rate(); >> And for this part, i am a little confused : >> 1 clk_set_rate must be set even if format is wrong? >> 2 The original code logic shows that If format is not correct, we will not set registers >> HDLCD_REG_PIXEL_FORMAT & HDLCD_REG_<color>_SELECT, will this bring in >> any problems? >> 3 if 1 the rate must set & 2 registers above doesn`t matter, then maybe there is no >> need to disable pixel clock. >> Am i misunderstanding > >You are right, the pixel format check should not happen inside hdlcd_crtc_atomic_enable() >hook, it should be moved into a separate function hdlcd_crtc_atomic_check() and that needs >to be hooked into .atomic_check() for hdlcd_crtc_helper_funcs(). > >If you want to have another go I'll be happy to review and Ack your patch. > >Best regards, >Liviu > Hi Sure, i will check this and re-subbmit one patch. Regards, Bernard >> >> Regards, >> Bernard >> >> >> + return err; >> >> >> >> clk_set_rate(hdlcd->clk, m->crtc_clock * 1000); >> >> + return 0; >> >> } >> >> >> >> static void hdlcd_crtc_atomic_enable(struct drm_crtc *crtc, >> >> @@ -173,7 +174,11 @@ static void hdlcd_crtc_atomic_enable(struct drm_crtc *crtc, >> >> struct hdlcd_drm_private *hdlcd = crtc_to_hdlcd_priv(crtc); >> >> >> >> clk_prepare_enable(hdlcd->clk); >> >> - hdlcd_crtc_mode_set_nofb(crtc); >> >> + if (hdlcd_crtc_mode_set_nofb(crtc)) { >> >> + DRM_DEBUG_KMS("Invalid format, pixel clock enable failed!\n"); >> >> + clk_disable_unprepare(hdlcd->clk); >> >> + return; >> >> + } >> >> hdlcd_write(hdlcd, HDLCD_REG_COMMAND, 1); >> >> drm_crtc_vblank_on(crtc); >> >> } >> >> -- >> >> 2.26.2 >> >> >> > >> >-- >> >==================== >> >| I would like to | >> >| fix the world, | >> >| but they're not | >> >| giving me the | >> > \ source code! / >> > --------------- >> > ¯\_(ツ)_/¯ >> >> >> > >-- >==================== >| I would like to | >| fix the world, | >| but they're not | >| giving me the | > \ source code! / > --------------- > ¯\_(ツ)_/¯
diff --git a/drivers/gpu/drm/arm/hdlcd_crtc.c b/drivers/gpu/drm/arm/hdlcd_crtc.c index af67fefed38d..f3945dee2b7d 100644 --- a/drivers/gpu/drm/arm/hdlcd_crtc.c +++ b/drivers/gpu/drm/arm/hdlcd_crtc.c @@ -96,7 +96,7 @@ static int hdlcd_set_pxl_fmt(struct drm_crtc *crtc) } if (WARN_ON(!format)) - return 0; + return -EINVAL; /* HDLCD uses 'bytes per pixel', zero means 1 byte */ btpp = (format->bits_per_pixel + 7) / 8; @@ -125,7 +125,7 @@ static int hdlcd_set_pxl_fmt(struct drm_crtc *crtc) return 0; } -static void hdlcd_crtc_mode_set_nofb(struct drm_crtc *crtc) +static int hdlcd_crtc_mode_set_nofb(struct drm_crtc *crtc) { struct hdlcd_drm_private *hdlcd = crtc_to_hdlcd_priv(crtc); struct drm_display_mode *m = &crtc->state->adjusted_mode; @@ -162,9 +162,10 @@ static void hdlcd_crtc_mode_set_nofb(struct drm_crtc *crtc) err = hdlcd_set_pxl_fmt(crtc); if (err) - return; + return err; clk_set_rate(hdlcd->clk, m->crtc_clock * 1000); + return 0; } static void hdlcd_crtc_atomic_enable(struct drm_crtc *crtc, @@ -173,7 +174,11 @@ static void hdlcd_crtc_atomic_enable(struct drm_crtc *crtc, struct hdlcd_drm_private *hdlcd = crtc_to_hdlcd_priv(crtc); clk_prepare_enable(hdlcd->clk); - hdlcd_crtc_mode_set_nofb(crtc); + if (hdlcd_crtc_mode_set_nofb(crtc)) { + DRM_DEBUG_KMS("Invalid format, pixel clock enable failed!\n"); + clk_disable_unprepare(hdlcd->clk); + return; + } hdlcd_write(hdlcd, HDLCD_REG_COMMAND, 1); drm_crtc_vblank_on(crtc); }
The pixel clock is still enabled when the format is wrong. no error branch handle, and also some register is not set in this case, e.g: HDLCD_REG_<color>_SELECT. Maybe we should disable this clock and throw an warn message when this happened. With this change, the code maybe a bit more readable. Signed-off-by: Bernard Zhao <bernard@vivo.com> Changes since V1: *add format error handle, if format is not correct, throw an warning message and disable this clock. Link for V1: *https://lore.kernel.org/patchwork/patch/1228501/ --- drivers/gpu/drm/arm/hdlcd_crtc.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)