发件人：Liviu Dudau <liviu.dudau@xxxxxxx> 发送日期：2020-05-15 22:41:49 收件人：Bernard <bernard@xxxxxxxx> 抄送人：Brian Starkey <brian.starkey@xxxxxxx>,David Airlie <airlied@xxxxxxxx>,Daniel Vetter <daniel@xxxxxxxx>,dri-devel@xxxxxxxxxxxxxxxxxxxxx,linux-kernel@xxxxxxxxxxxxxxx,opensource.kernel@xxxxxxxx 主题：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@xxxxxxxx> >> Date: 2020-04-24 19:37:36 >> To: Liviu Dudau <liviu.dudau@xxxxxxx> >> Cc: Brian Starkey <brian.starkey@xxxxxxx>,David Airlie <airlied@xxxxxxxx>,Daniel Vetter <daniel@xxxxxxxx>,dri-devel@xxxxxxxxxxxxxxxxxxxxx,linux-kernel@xxxxxxxxxxxxxxx,opensource.kernel@xxxxxxxx >> Subject: Re:Re: [PATCH v2] drm/arm: fixes pixel clock enabled with wrong format >> >> >> >> >> From: Liviu Dudau <liviu.dudau@xxxxxxx> >> Date: 2020-04-24 19:09:50 >> To: Bernard Zhao <bernard@xxxxxxxx> >> Cc: Brian Starkey <brian.starkey@xxxxxxx>,David Airlie <airlied@xxxxxxxx>,Daniel Vetter <daniel@xxxxxxxx>,dri-devel@xxxxxxxxxxxxxxxxxxxxx,linux-kernel@xxxxxxxxxxxxxxx,opensource.kernel@xxxxxxxx >> 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@xxxxxxxx> >> >> >> >> 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! / > --------------- > ¯\_(ツ)_/¯ _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
