Paul Kocialkowski <paul.kocialkowski@xxxxxxxxxxx> writes:
> Hi Eric,
>
> On Wed, 2019-01-23 at 10:47 -0800, Eric Anholt wrote:
>> Paul Kocialkowski <paul.kocialkowski@xxxxxxxxxxx> 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.
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
