Dear Wolf,
I finally tested your EDID code on my other machines and it works
everywhere as expected.
I also went through the code in your vesafb branch. Here are some
comments and suggestions what you should do during your final week of
coding:
* I believe that you are severely behind the schedule. I see that many
of the important building blocks are prototyped successfully, but the
whole functionality is not well integrated. According to your
proposal, the final week of coding should be dedicated to code
cleanup, comments and documentation. But unfortunately you have much
more pushing tasks to do.
* From the user experience point of view I see two major problems:
- The vesafb driver does not set the optimal mode (according to EDID)
yet, it only reuses the kfb mode.
- Without the kernel framebuffer activated there is no way how to
activate the compositor and vesafb (i.e. how to switch from the
text mode console to the compositor).
AI: Catch up with the schedule and deliver an integrated user
experience, i.e. vesafb setting optimal mode using EDID, possibly
also without kernel framebuffer present in the kernel at all.
* Similar issues appear also on the code level. While x86emu is nicely
isolated into a library, other pieces of "borrowed" code (drm_edid,
drm_modes, etc.) are used directly in the vesafb driver. You should
have really written the EDID code from scratch, the "DRM" code is
alien to HelenOS.
AI: Rewrite the "DRM" code from scratch.
* What is even more wrong is the use of the source files linuxlist.h,
linuxlist_sort.{c|h}, linuxpoison.h and linuxtypes.h in vesafb. If I
am not mistaken, these files are distributed under GPL, thus forcing
(at least) the entire vesafb to be also licensed under GPL.
AI: Remove/rewrite the Linux lists.
* In sys_debug_console() (kernel/generic/src/console.c) you access the
data structure supplied from user space without any checks. The
approach is not only vulnerable, moreover it works only on x86 by
sheer coincidence.
AI: Use copy_from_uspace() to access the user space supplied
structure.
* It is a pity that you have failed to follow previous advice from me
and others [1][2]. The command line interface of compctl is still
unfixed and frankly, using compctl is a pain in the ass currently.
There are also still two nodes for controlling the compositor
registered by the location service. And vesafb is still creating a
thread for unknown reasons.
After several repeated requests, could you finally make your branch
compile without the need to remove the -Werror option and merge the
mainline changes (this should be rather straightforward)?
AI: Improve the command line interface of compctl. As there is almost
universally just one compositor in the system, make it the default
choice for compctl. Or at least make compctl print the list of
compositors running by default.
AI: Get rid of the two compositor nodes.
AI: Get rid of the thread in vesafb.
AI: Get rid of the compilation warnings.
AI: Merge with mainline.
* Beyond that, here are some extra action items:
AI: Clean up the code, write and update documentation comments (at
least for all public and major functions).
AI: Write at least some design and user documentation on our wiki.
AI: I still don't like how the kfb and vesafb "coexist". Your
solution requires that kfb is completely absent from the system on
ia32 and amd64. But the original suggestion was a bit different [3].
It would be nice if both kfb and vesafb could coexist in the system.
AI: If I am not mistaken, the real-mode IVT area does not necessarily
need to be mappable in a read/write mode. It would be nice if the DDI
would only allow to map it in a read-only mode.
Finally, there is the issue we have already spoken about privately. I
really hate to open it in the public, but it somehow relates to all the
previous points. Even if I over-estimate your coding effort, I am able
to count only some 2000 (maybe 2500) lines of code written or modified
by you.
No surprise you are behind the schedule. 2500 SLOC is about 1/2 of the
expected productivity of a GSoC student, even considering the context of
the topic. I hope that I don't have to explain the rationale behind
this, I have already written too many private warnings to you.
Your diligent effort in your final week and tackling all the remaining
action items can save you for the final evaluation. The final
deliverable has to be something that could be integrated into the
HelenOS mainline branch and work reasonably.
Good luck!
[1] http://lists.modry.cz/private/helenos-devel/2014-July/007287.html
[2] http://lists.modry.cz/private/helenos-devel/2014-July/007289.html
[3] http://lists.modry.cz/private/helenos-devel/2014-July/007293.html
M.D.
_______________________________________________
HelenOS-devel mailing list
[email protected]
http://lists.modry.cz/listinfo/helenos-devel
