by Liviu Dudau

[permalink] [raw]

Subject: Re: [PATCH 2/4] drm: Add support for ARM's HDLCD controller.

On Mon, Aug 17, 2015 at 07:17:53PM +0100, Jon Medhurst (Tixy) wrote:
> I haven't reviewed the code in detail, just had one comment I alluded to
> in a private email the other day...
>
> On Wed, 2015-08-05 at 15:28 +0100, Liviu Dudau wrote:
>
> > diff --git a/drivers/gpu/drm/arm/hdlcd_crtc.c b/drivers/gpu/drm/arm/hdlcd_crtc.c
> [...]
> > +void hdlcd_set_scanout(struct hdlcd_drm_private *hdlcd)
> > +{
> > + struct drm_framebuffer *fb = hdlcd->crtc.primary->fb;
> > + struct drm_gem_cma_object *gem;
> > + unsigned int depth, bpp;
> > + dma_addr_t scanout_start;
> > +
> > + drm_fb_get_bpp_depth(fb->pixel_format, &depth, &bpp);
> > + gem = drm_fb_cma_get_gem_obj(fb, 0);
> > +
> > + scanout_start = gem->paddr + fb->offsets[0] +
> > + (hdlcd->crtc.y * fb->pitches[0]) + (hdlcd->crtc.x * bpp/8);
> > +
> > + hdlcd_write(hdlcd, HDLCD_REG_FB_BASE, scanout_start);
> > +}
> > +
>
> The above function accesses various pointers and values, which
> presumably all need to be valid and consistent. However...
>
> [...]
> > diff --git a/drivers/gpu/drm/arm/hdlcd_drv.c b/drivers/gpu/drm/arm/hdlcd_drv.c
> [...]
> > +static irqreturn_t hdlcd_irq(int irq, void *arg)
> > +{
> > + struct drm_device *dev = arg;
> > + struct hdlcd_drm_private *hdlcd = dev->dev_private;
> > + unsigned long irq_status;
> > +
> > + irq_status = hdlcd_read(hdlcd, HDLCD_REG_INT_STATUS);
> > +
> > +#ifdef CONFIG_DEBUG_FS
> > + if (irq_status & HDLCD_INTERRUPT_UNDERRUN) {
> > + atomic_inc(&hdlcd->buffer_underrun_count);
> > + }
> > + if (irq_status & HDLCD_INTERRUPT_DMA_END) {
> > + atomic_inc(&hdlcd->dma_end_count);
> > + }
> > + if (irq_status & HDLCD_INTERRUPT_BUS_ERROR) {
> > + atomic_inc(&hdlcd->bus_error_count);
> > + }
> > + if (irq_status & HDLCD_INTERRUPT_VSYNC) {
> > + atomic_inc(&hdlcd->vsync_count);
> > + }
> > +#endif
> > + if (irq_status & HDLCD_INTERRUPT_VSYNC) {
> > + struct drm_pending_vblank_event *event;
> > + unsigned long flags;
> > +
> > + hdlcd_set_scanout(hdlcd);
>
> hdlcd_set_scanout is being called on every vsync interrupt, can we
> guarantee that is safe? What if we're in the middle of a page flip or
> panning operation? Seems to me that there is at least scope for
> incorrect addresses being calculated leading to a nasty glitch on the
> screen for one frame. And in the worst case, possibly invalid pointer
> being dereferenced.

Agree, there is a risk of corruption here. I'm going to look at the atomic
mode setting which should hopefully resolve most of these issues.

>
> So, if scanout needs to be set on every vsync, would it not be safer
> (and more efficient) to have a single variable storing the value to use
> during interrupts, and recalculate that value outside of interrupts
> whenever things are changed?

hdlcd_set_scanout() function is merely a convenience function to calculate
the scanout_start variable. The interrupt handler probably doesn't need to
call that and it was mostly a shortcut to make sure the HDLCD_REG_FB_BASE
register was up-to-date when the vsync interrupt happened. I hope the
atomic modeset will cleanup things here.

Best regards,
Liviu

>
> --
> Tixy
>

--
====================
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---------------
¯\_(ツ)_/¯