| Message ID | 1507887353-30360-1-git-send-email-jsarha@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki On 13/10/17 12:35, Jyri Sarha wrote: > We need the total frame refresh time to check if we are too close to > vertical sync when updating the two framebuffer DMA registers and risk > a collision. This new method is more accurate that the previous that > based on mode's vrefresh value, which itself is inaccurate or may not > even be initialized. > > Reported-by: Kevin Hao <kexin.hao@windriver.com> > Fixes: 11abbc9f39e0 ("drm/tilcdc: Set framebuffer DMA address to HW only if CRTC is enabled") > Cc: <stable@vger.kernel.org> # v4.11+ > Signed-off-by: Jyri Sarha <jsarha@ti.com> > --- > Since first version: > > Change frametime* variable names to hvtotal and use 64-bit division > instead of dynamcally scaled 32-bit division as suggested by Tomi > Valkeinen. > > drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 17 ++++++++++++++++- > 1 file changed, 16 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c > index 704db24..a8b22aa 100644 > --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c > +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c > @@ -24,6 +24,7 @@ > #include <linux/completion.h> > #include <linux/dma-mapping.h> > #include <linux/of_graph.h> > +#include <linux/math64.h> > > #include "tilcdc_drv.h" > #include "tilcdc_regs.h" > @@ -48,6 +49,7 @@ struct tilcdc_crtc { > unsigned int lcd_fck_rate; > > ktime_t last_vblank; > + unsigned int hvtotal_us; > > struct drm_framebuffer *curr_fb; > struct drm_framebuffer *next_fb; > @@ -292,6 +294,16 @@ static void tilcdc_crtc_set_clk(struct drm_crtc *crtc) > LCDC_V2_CORE_CLK_EN); > } > > +uint tilcdc_mode_hvtotal(const struct drm_display_mode *mode) > +{ > + uint ret; > + > + ret = (uint) div_u64(1000llu * mode->htotal * mode->vtotal, > + mode->clock); > + > + return ret; > +} I don't think "uint" is recommended. Just use u32. And drop the ret variable, just one-line return statement should be enough. Otherwise: Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ti.com> Tomi
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki On 10/13/17 14:04, Tomi Valkeinen wrote: >> +uint tilcdc_mode_hvtotal(const struct drm_display_mode *mode) >> +{ >> + uint ret; >> + >> + ret = (uint) div_u64(1000llu * mode->htotal * mode->vtotal, >> + mode->clock); >> + >> + return ret; >> +} > I don't think "uint" is recommended. Just use u32. And drop the ret > variable, just one-line return statement should be enough. > The ret variable can of course be dropped, but how u32 is better than uint? If the driver would ever be used in 64bit architecture (highly unlikely), then it would automatically use the higher precision, and on 32-bit architecture there is no difference. Also the data member in the private struct is uint and so are the other similar data members like lcd_fck_rate. So why change? Best regards, Jyri > Otherwise: > > Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ti.com> > > Tomi
This message contains a digitally signed email which can be read by opening the attachment. Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki On 13/10/17 15:00, Jyri Sarha wrote: > On 10/13/17 14:04, Tomi Valkeinen wrote: >>> +uint tilcdc_mode_hvtotal(const struct drm_display_mode *mode) >>> +{ >>> + uint ret; >>> + >>> + ret = (uint) div_u64(1000llu * mode->htotal * mode->vtotal, >>> + mode->clock); >>> + >>> + return ret; >>> +} >> I don't think "uint" is recommended. Just use u32. And drop the ret >> variable, just one-line return statement should be enough. >> > > The ret variable can of course be dropped, but how u32 is better than > uint? If the driver would ever be used in 64bit architecture (highly > unlikely), then it would automatically use the higher precision, and on > 32-bit architecture there is no difference. I think int is normally 32 bit on 64 bit platforms too. But that depends on compiler flags. Where does "uint" come from anyway? I don't think it's part of a C standard. > Also the data member in the private struct is uint and so are the other > similar data members like lcd_fck_rate. Ok. Well, it's good to stick to one way of doing things, so maybe uint is fine. For any new code, I would not recommend using it. Tomi
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
On 10/13/17 15:32, Tomi Valkeinen wrote:
> Where does "uint" come from anyway? I don't think it's part of a C standard.
AFAIK, it is just a Linux commodity typedef for unsigned int, and
because of that a defacto typedef in many other environments too.
Best regards,
Jyri
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c index 704db24..a8b22aa 100644 --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c @@ -24,6 +24,7 @@ #include <linux/completion.h> #include <linux/dma-mapping.h> #include <linux/of_graph.h> +#include <linux/math64.h> #include "tilcdc_drv.h" #include "tilcdc_regs.h" @@ -48,6 +49,7 @@ struct tilcdc_crtc { unsigned int lcd_fck_rate; ktime_t last_vblank; + unsigned int hvtotal_us; struct drm_framebuffer *curr_fb; struct drm_framebuffer *next_fb; @@ -292,6 +294,16 @@ static void tilcdc_crtc_set_clk(struct drm_crtc *crtc) LCDC_V2_CORE_CLK_EN); } +uint tilcdc_mode_hvtotal(const struct drm_display_mode *mode) +{ + uint ret; + + ret = (uint) div_u64(1000llu * mode->htotal * mode->vtotal, + mode->clock); + + return ret; +} + static void tilcdc_crtc_set_mode(struct drm_crtc *crtc) { struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc); @@ -459,6 +471,9 @@ static void tilcdc_crtc_set_mode(struct drm_crtc *crtc) drm_framebuffer_get(fb); crtc->hwmode = crtc->state->adjusted_mode; + + tilcdc_crtc->hvtotal_us = + tilcdc_mode_hvtotal(&crtc->hwmode); } static void tilcdc_crtc_enable(struct drm_crtc *crtc) @@ -642,7 +657,7 @@ int tilcdc_crtc_update_fb(struct drm_crtc *crtc, spin_lock_irqsave(&tilcdc_crtc->irq_lock, flags); next_vblank = ktime_add_us(tilcdc_crtc->last_vblank, - 1000000 / crtc->hwmode.vrefresh); + tilcdc_crtc->hvtotal_us); tdiff = ktime_to_us(ktime_sub(next_vblank, ktime_get())); if (tdiff < TILCDC_VBLANK_SAFETY_THRESHOLD_US)
We need the total frame refresh time to check if we are too close to vertical sync when updating the two framebuffer DMA registers and risk a collision. This new method is more accurate that the previous that based on mode's vrefresh value, which itself is inaccurate or may not even be initialized. Reported-by: Kevin Hao <kexin.hao@windriver.com> Fixes: 11abbc9f39e0 ("drm/tilcdc: Set framebuffer DMA address to HW only if CRTC is enabled") Cc: <stable@vger.kernel.org> # v4.11+ Signed-off-by: Jyri Sarha <jsarha@ti.com> --- Since first version: Change frametime* variable names to hvtotal and use 64-bit division instead of dynamcally scaled 32-bit division as suggested by Tomi Valkeinen. drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-)