Paul Kocialkowski <paul.kocialkowski@bootlin.com> writes:

> Hi Eric,
>
> On Wed, 2019-01-23 at 10:47 -0800, Eric Anholt wrote:
>> Paul Kocialkowski <paul.kocialkowski@bootlin.com> writes:
>> > +void vc4_hvs_mask_underrun(struct drm_device *dev)
>> > +{
>> > +	struct vc4_dev *vc4 = to_vc4_dev(dev);
>> > +	u32 dispctrl = HVS_READ(SCALER_DISPCTRL);
>> > +
>> > +	dispctrl &= ~(SCALER_DISPCTRL_DSPEISLUR(0) |
>> > +		      SCALER_DISPCTRL_DSPEISLUR(1) |
>> > +		      SCALER_DISPCTRL_DSPEISLUR(2));
>> > +
>> > +	HVS_WRITE(SCALER_DISPCTRL, dispctrl);
>> > +}
>> > +
>> > +void vc4_hvs_unmask_underrun(struct drm_device *dev)
>> > +{
>> > +	struct vc4_dev *vc4 = to_vc4_dev(dev);
>> > +	u32 dispctrl = HVS_READ(SCALER_DISPCTRL);
>> > +
>> > +	dispctrl |= SCALER_DISPCTRL_DSPEISLUR(0) |
>> > +		    SCALER_DISPCTRL_DSPEISLUR(1) |
>> > +		    SCALER_DISPCTRL_DSPEISLUR(2);
>> > +
>> > +	HVS_WRITE(SCALER_DISPSTAT,
>> > +		  SCALER_DISPSTAT_EUFLOW(0) |
>> > +		  SCALER_DISPSTAT_EUFLOW(1) |
>> > +		  SCALER_DISPSTAT_EUFLOW(2));
>> > +	HVS_WRITE(SCALER_DISPCTRL, dispctrl);
>> > +}
>> > +
>> > +static void vc4_hvs_report_underrun(struct drm_device *dev)
>> > +{
>> > +	struct vc4_dev *vc4 = to_vc4_dev(dev);
>> > +
>> > +	atomic_inc(&vc4->underrun);
>> > +	DRM_DEV_ERROR(dev->dev, "HVS underrun\n");
>> > +}
>> > +
>> > +static irqreturn_t vc4_hvs_irq_handler(int irq, void *data)
>> > +{
>> > +	struct drm_device *dev = data;
>> > +	struct vc4_dev *vc4 = to_vc4_dev(dev);
>> > +	u32 status;
>> > +
>> > +	status = HVS_READ(SCALER_DISPSTAT);
>> > +
>> > +	if (status &
>> > +	    (SCALER_DISPSTAT_EUFLOW(0) | SCALER_DISPSTAT_EUFLOW(1) |
>> > +	     SCALER_DISPSTAT_EUFLOW(2))) {
>> > +		vc4_hvs_mask_underrun(dev);
>> > +		vc4_hvs_report_underrun(dev);
>> > +	}
>> > +
>> > +	HVS_WRITE(SCALER_DISPSTAT, status);
>> > +
>> > +	return status ? IRQ_HANDLED : IRQ_NONE;
>> > +}
>> 
>> So, if UFLOW is set then we incremented the counter and disabled the
>> interrupt, otherwise we acked this specific interrupt and return?  Given
>> that a short-line error (the other potential cause of EISLUR) would be
>> likely to persist, we should probably either vc4_hvs_mask_underrun() in
>> that case too, or only return IRQ_HANDLED for the case we actually
>> handled.
>
> I see, there is definitely an inconsistency here. I don't think we
> should be disabling the interrupt if we get a short line indication,
> just in case the interrupt gets triggered later for a legitimate
> underrun (before the next commit).
>
> So I think we should just totally ignore the short line status bit for
> the IRQ return (although it certainly doesn't hurt to clear it as
> well). What do you think?

You just have to make sure that you return UNHANDLED for short line, so
an IRQ storm doesn't take down the machine.