Wolfgang Rohdewald wrote:
> On Dienstag 07 Juni 2005 17:44, Johannes Stezenbach wrote:
>
> > Why don't you just fix the problem with the b0rked error
> > handling in Load/BlitBitmap and be done with it?
>
> OK, here comes my first try. I hope this propagates all ERESTARTSYS,
> not only in OSDSetBlock
>
> yesterday I said these problems do not happen with a non preempting kernel.
> Wrong, they do happen, but only rarely.
>
> about OSDSetBlock: If it is interrupted, it starts all over again
> until it can finish. I don't know how big the risk of endless trying is.
> Actually I think it can be neglected. Otherwise, possible solutions:
>
> 1. remember how much has been done: add a counter to struct av7110
> which is incremented in relevant places. On next try, dry-run these
> places (i.e. do not pass anything to the card).
Doesn't work. Userspace has the option not to restart interrupted
syscalls.
> 2. at the second time, use wait_event instead of wait_event_interruptible
I hope this isn't necessary...
> If OSDSetBlock times out, -ETIMEDOUT is returned. Most other timeouts
> will return -1. Could this be made more consistent? Also, the return codes
> in <dvb/osd.h> don't match what the driver does. I don't know what should
> be done.
We better ask Klaus Schmidinger about this.
To me your patch looks mostly fine.
A few minor nits:
> - SetColor_(av7110, av7110->osdwin, bpp2pal[av7110->osdbpp[av7110->osdwin]],
> + ret = SetColor_(av7110, av7110->osdwin, bpp2pal[av7110->osdbpp[av7110->osdwin]],
> color, ch, cl);
Broken indentation (with vim you can just ^F in insert mode to
line up the second line properly).
> - SetBlend_(av7110, av7110->osdwin, bpp2pal[av7110->osdbpp[av7110->osdwin]],
> + if (!ret)
> + ret = SetBlend_(av7110, av7110->osdwin, bpp2pal[av7110->osdbpp[av7110->osdwin]],
> color, ((blend >> 4) & 0x0f));
same here (repeats)
> + if (av7110->bmp_state == BMP_LOADING)
> + {
> + /* possible if syscall is repeated by -ERESTARTSYS */
> + rc = WaitUntilBmpLoaded(av7110);
> + if (rc)
> + return rc;
> + /* restart since we don't know where we were interrupted */
> + ReleaseBitmap(av7110);
> + }
Hm, I think it would be better to call ReleaseBitmap() before
aborting the OSDSetBlock ioctl. The firmware doesn't accept
other OSD ioctls before the bitmap upload is terminated.
Or, you could move this test outside the big switch in av7110_osd_cmd().
(Usually the syscall will be restarted so this isn't a problem,
except e.g. if you kill -9 vdr.)
> - ret = down_interruptible(&av7110->osd_sema);
> - if (ret)
> + if (down_interruptible(&av7110->osd_sema))
> return -ERESTARTSYS;
...
> case OSD_SetTrans:
> + ret = 0;
> goto out;
Hm, I don't like this ret = 0 everyhwere, why no set ret = 0 by default?
> @@ -1153,6 +1177,12 @@
>
> out:
> up(&av7110->osd_sema);
> +
This line adds trailing whitespace.
Thanks,
Johannes
