Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for Raspberry Pi hardware cursor #39

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

JamesH65
Copy link

@JamesH65 JamesH65 commented Jul 8, 2014

Requires firmware changes but will detect at
run time if the firmware supports the HW cursor
and enabled/disable as necessary.

The required firmware has been in the wild for some
time so most will already have it.

@ssvb
Copy link
Owner

ssvb commented Sep 20, 2014

Overall, this looks quite a bit bigger than I would have expected. The fact that some functions seem to be unused is apparently contributing to this.

The commit(s) should have Signed-off-by tag(s). The coding style should preferably match the rest of the sources (indentation level, curly braces, line length limit), unless the code is imported from some other project.

I don't mind adding the hardware cursor support if the review comments are addressed. However, practically speaking, having some bigger feature like X11 EGL support or XV video overlays relying on it would definitely make it more attractive.

@JamesH65
Copy link
Author

I can try and fix up the other 'issues' (Although I have never agreed with having a line length limitation given we now use monitors that are bigger than teletypes and punch cards), but not sure what you mean by X11 EGL and XV overlays. This is a performance enhancing HW cursor implementation for the Raspberry Pi, and unrelated to any other features. Testing has shown noticeable improvements in performance over the SW implementation - that's the reason for it.

@ssvb
Copy link
Owner

ssvb commented Sep 22, 2014

Although I have never agreed with having a line length limitation given we now use
monitors that are bigger than teletypes and punch cards)

The github patch view page appears to have line length ~113 characters in my browser. Some of the lines from your patch do not fit. This is a hindrance for review.

not sure what you mean by X11 EGL and XV overlays.

I mean that X11 EGL or XV overlays would bring really visible major features to the end users. They depend on working hardware cursor and are a natural next step.

Testing has shown noticeable improvements in performance over the SW
implementation - that's the reason for it.

The benchmark numbers would be very nice to see in the commit message.

@JamesH65
Copy link
Author

Agreed about the line length of github - that's rather shortsighted of github, but will adjust lines accordingly.

I won't be doing anything on X11 EGL or overlays - out of my field, this was specifically a low hanging fruit optimisation. As you say, it should give someone else the ability to implement those features. I suspect, given this needed firmware changes, EGL and XV overlays may also need them.

I don't have specific benchmarks, just anecdotal from my own testing, that of the bitscope people who tested with their software oscilloscope code, and some testing done by the community. I'm not really sure of a way of getting general performance figures out of it. IIRC, I was seeing between 2 and 5% improvement on the bitscope application.

Requires firmware changes but will detect at
run time if the firmware supports the HW cursor
and enabled/disable as necessary.

The required firmware has been in the wild for some
time so most will already have it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants