On Tue, May 19, 2015 at 1:09 PM, Jan Dolezal <dolez...@fel.cvut.cz> wrote:
> Hello Gedare,
> thank you for the swift response..
>
>
> On 19.5.2015 17:38, Gedare Bloom wrote:
>>
>> On Tue, May 19, 2015 at 10:34 AM, Jan Dolezal <dolez...@fel.cvut.cz>
>> wrote:
>>>
>>> driver is not initialized by default
>>> initialization is possible through multiboot command line option or
>>> through string variable set in user's module allowing the driver
>>> to evaluate this variable after the two modules are linked together
>>> ---
>>> c/src/lib/libbsp/i386/pc386/console/fb_vesa_rm.c | 152
>>> +++++++++++++++++++----
>>> 1 file changed, 127 insertions(+), 25 deletions(-)
>>>
>>> diff --git a/c/src/lib/libbsp/i386/pc386/console/fb_vesa_rm.c
>>> b/c/src/lib/libbsp/i386/pc386/console/fb_vesa_rm.c
>>> index d6fd174..5c4d8bf 100644
>>> --- a/c/src/lib/libbsp/i386/pc386/console/fb_vesa_rm.c
>>> +++ b/c/src/lib/libbsp/i386/pc386/console/fb_vesa_rm.c
>>> @@ -19,8 +19,14 @@
>>> *
>>> * Driver reads parameter from multiboot command line to setup video:
>>> * "--video=<resX>x<resY>[-<bpp>]"
>>> - * If cmdline parameter is not specified an attempt for obtaining
>>> - * resolution from display attached is made.
>>> + * "--video=auto" - try EDID to find mode that fits the display attached
>>> best
>>> + * "--video=none" / "--video=off" - do not initialize the driver
>>> + * If cmdline parameter is not specified the rtems_console_vbe_default
>>> + * variable content is tested (see doc below).
>>> + * Command line option has higher priority. rtems_console_vbe_default is
>>> probed
>>> + * only if cmdline "--video=" is not specified at all.
>>> + *
>>> + * If neither of the above options is specified the driver is not
>>> initialized.
>>> */
>>>
>>> /*
>>> @@ -31,7 +37,7 @@
>>> * found in the file LICENSE in this distribution or at
>>> * http://www.rtems.org/license/LICENSE.
>>> *
>>> - * The code for rtems_buffer_* functions were greatly
>>> + * The code for rtems_buffer_* functions was greatly
>>> * inspired or coppied from:
>>> * - RTEMS fb_cirrus.c - Alexandru-Sever Horin
>>> (alex.seve...@gmail.com)
>>> */
>>> @@ -53,6 +59,22 @@
>>> #define FB_VESA_NAME "FB_VESA_RM"
>>>
>>> /**
>>> + * @brief Allows to enable initialization of this driver from an
>>> application
>>> + * by setting the value of this variable to non null value in
>>> + * user's module. The value of this variable will be then updated
>>> + * when linked with application's object.
>>> + *
>>> + * Further if the value points to string in format:
>>> + * "<resX>x<resY>[-<bpp>]"
>>> + * "auto" - try EDID to find mode that fits the display attached best
>>> + * "none" / "off" - do not initialize the driver
>>> + * the given parameters are used if applicable.
>>> + *
>>> + * Command line option string has priority over this string.
>>> + */
>>> +const char * const rtems_console_vbe_default;
>>> +
>>
>> If this variable is not exported elsewhere, it should be made 'static'.
>
>
> The point is to make this variable available for exporting somewhere
> else, specifically the application that wishes to use the driver, sets
> this variable to the string containing the resolution requested to be
> set. When the application is linked together with this module,
> rtems_console_vbe_default points to that string and the driver can
> initialize itself using the string from the application.
>
Then should this variable be declared in a header visible to both the
user application and this source code file as extern?
>>
>>> +/**
>>> * @brief Initializes VBE framebuffer during bootup.
>>> *
>>> * utilizes switches to real mode interrupts and therefore must be
>>> @@ -212,6 +234,14 @@ typedef struct {
>>> uint8_t bpp;
>>> } Mode_params;
>>>
>>> +typedef enum {
>>> + NO_SUITABLE_MODE = -1,
>>> + BAD_FORMAT = -2,
>>> + AUTO_SELECT = -3,
>>> + DONT_INIT = -4,
>>> + NO_MODE_REQ = -5,
>>> +} mode_err_ret_val;
>>> +
>>
>>
>> Can the valid and invalid modes be in the same enum, and that can be
>> used as the type too?
>
>
> Valid modes are obtained from graphics card and may be manufacturer or card
> dependant. Some of the modes specified by the VESA standard are defined as
> macros here:
> git.rtems.org/rtems/tree/c/src/lib/libbsp/i386/pc386/include/vbe3.h
>
> I added the enum (especially its values) to make the code better readable.
>
>
OK.
>>
>>> /**
>>> * @brief Find mode by resolution in the given list of modes
>>> *
>>> @@ -249,37 +279,48 @@ static uint16_t find_mode_by_resolution(Mode_params
>>> *mode_list,
>>> }
>>> i++;
>>> }
>>> - return -1;
>>> + return NO_SUITABLE_MODE;
>>> }
>>>
>>> /**
>>> - * @brief Find mode given within command line.
>>> + * @brief Find mode given in string format.
>>> *
>>> - * Parse command line option "--video=" if available.
>>> * expected format
>>> - * --video=<resX>x<resY>[-<bpp>]
>>> + * <resX>x<resY>[-<bpp>]
>>> * numbers <resX>, <resY> and <bpp> are decadic
>>> *
>>> * @param[in] mode_list list of modes to be searched
>>> * @param[in] list_length number of modes in the list
>>> + * @param[in] video_string string to be parsed
>>> * @retval video mode number to be set
>>> - * @retval -1 on parsing error or when no suitable mode found
>>> + * @retval -1 no suitable mode found
>>> + * @retval -2 bad format of the video_string
>>> + * @retval -3 automatic mode selection requested
>>> + * @retval -4 request to not initialize graphics
>>> + * @retval -5 no mode requested/empty video string
>>> */
>>> -static uint16_t find_mode_using_cmdline(Mode_params *mode_list,
>>> - uint8_t list_length)
>>> +static uint16_t find_mode_from_string(Mode_params *mode_list,
>>> + uint8_t list_length,
>>> + const char *video_string)
>>
>> Is it ok to cast a negative enum value into a uint16_t?
>
>
> I don't quite understand, what do you mean by ok.
> I use negative numbers to get values from the end of uint16_t range which
> should not overlap with possible valid mode numbers. Return value is used to
> inform the user why the driver was not initialized. The deciding switch
> cases (below) then have explicit casts into a uint16_t, so it is compared
> correctly.
>
"ok" in the sense that mixing signed and unsigned is sometimes a very bad idea.
In this case it seems safe, but you might also consider using
explicitly large uint16_t values in your enum or even just define some
macros with these large uint16_t values and then you can avoid the
casting. This isn't such a big deal to me since it is localized to
this one source code file, I'm just making some general comments.
>
>>
>>> {
>>> const char* opt;
>>> Mode_params cmdline_mode;
>>> char* endptr;
>>> - cmdline_mode.bpp = 0;
>>> - opt = bsp_cmdline_arg("--video=");
>>> + cmdline_mode.bpp = 16; /* default bpp */
>>> + opt = video_string;
>>> if (opt)
>>> {
>>> - opt += sizeof("--video=")-1;
>>> + if (strncmp(opt, "auto", 4) == 0)
>>> + return AUTO_SELECT;
>>> + if (strncmp(opt, "none", 4) == 0 ||
>>> + strncmp(opt, "off", 3) == 0)
>>> + {
>>> + return DONT_INIT;
>>> + }
>>> cmdline_mode.resX = strtol(opt, &endptr, 10);
>>> if (*endptr != 'x')
>>> {
>>> - return -1;
>>> + return BAD_FORMAT;
>>> }
>>> opt = endptr+1;
>>> cmdline_mode.resY = strtol(opt, &endptr, 10);
>>> @@ -294,21 +335,50 @@ static uint16_t find_mode_using_cmdline(Mode_params
>>> *mode_list,
>>> cmdline_mode.bpp = strtol(opt, &endptr, 10);
>>> if (*endptr != ' ')
>>> {
>>> - return -1;
>>> + return BAD_FORMAT;
>>> }
>>> }
>>> case ' ':
>>> case 0:
>>> break;
>>> default:
>>> - return -1;
>>> + return BAD_FORMAT;
>>> }
>>>
>>> - if (find_mode_by_resolution(mode_list, list_length,
>>> &cmdline_mode) !=
>>> - (uint16_t)-1)
>>> - return cmdline_mode.mode_number;
>>> + return find_mode_by_resolution(mode_list, list_length,
>>> &cmdline_mode);
>>> }
>>> - return -1;
>>> + return NO_MODE_REQ;
>>> +}
>>> +
>>> +
>>> +/**
>>> + * @brief Find mode given within command line.
>>> + *
>>> + * Parse command line option "--video=" if available.
>>> + * expected format
>>> + * --video=<resX>x<resY>[-<bpp>]
>>> + * numbers <resX>, <resY> and <bpp> are decadic
>>> + *
>>> + * @param[in] mode_list list of modes to be searched
>>> + * @param[in] list_length number of modes in the list
>>> + * @retval video mode number to be set
>>> + * @retval -1 no suitable mode found
>>> + * @retval -2 bad format of the video_string
>>> + * @retval -3 automatic mode selection requested
>>> + * @retval -4 request to not initialize graphics
>>> + * @retval -5 no mode requested/empty video string
>>> + */
>>> +static uint16_t find_mode_using_cmdline(Mode_params *mode_list,
>>> + uint8_t list_length)
>>> +{
>>> + const char* opt;
>>> + opt = bsp_cmdline_arg("--video=");
>>> + if (opt)
>>> + {
>>> + opt += sizeof("--video=")-1;
>>> + return find_mode_from_string(mode_list, list_length, opt);
>>> + }
>>> + return NO_MODE_REQ;
>>> }
>>>
>>> /**
>>> @@ -569,6 +639,7 @@ void vesa_realmode_bootup_init(void)
>>> uint16_t *modeNOPtr = (uint16_t*)
>>> i386_Real_to_physical(*(vmpSegOff+1), *vmpSegOff);
>>> uint16_t iterator = 0;
>>> +
>>> if (*(uint16_t*)vib->VideoModePtr == VBE_STUB_VideoModeList)
>>> {
>>> printk(FB_VESA_NAME " VBE Core not implemented!\n");
>>> @@ -618,6 +689,40 @@ void vesa_realmode_bootup_init(void)
>>> sorted_mode_params[nextFilteredMode].mode_number = 0;
>>>
>>> uint8_t number_of_modes = nextFilteredMode;
>>> +
>>> + /* first search for video argument in multiboot options */
>>> + vbe_used_mode = find_mode_using_cmdline(sorted_mode_params,
>>> + number_of_modes);
>>> +
>>> + if (vbe_used_mode == (uint16_t) NO_MODE_REQ) {
>>> + vbe_used_mode = find_mode_from_string(sorted_mode_params,
>>> + number_of_modes, rtems_console_vbe_default);
>>> + if (vbe_used_mode != (uint16_t) NO_MODE_REQ) {
>>> + printk(FB_VESA_NAME " using application option to select"
>>> + " video mode\n");
>>> + }
>>> + }
>>> + else
>>> + {
>>> + printk(FB_VESA_NAME " using command line option '--video='"
>>> + "to select video mode\n");
>>> + }
>>> +
>>> + switch (vbe_used_mode) {
>>> + case (uint16_t) NO_SUITABLE_MODE:
>>> + printk(FB_VESA_NAME " requested mode not found\n");
>>> + return;
>>> + case (uint16_t) BAD_FORMAT:
>>> + printk(FB_VESA_NAME " bad format of video requested\n");
>>> + return;
>>> + case (uint16_t) DONT_INIT:
>>> + printk(FB_VESA_NAME " selected not to initialize
>>> graphics\n");
>>> + return;
>>> + case (uint16_t) NO_MODE_REQ:
>>> + printk(FB_VESA_NAME " not initialized, no video
>>> selected\n");
>>> + return;
>>> + }
>>> +
>>> /* sort filtered modes */
>>> Mode_params modeXchgPlace;
>>> iterator = 0;
>>> @@ -657,12 +762,9 @@ void vesa_realmode_bootup_init(void)
>>> iterator++;
>>> }
>>>
>>> - /* first search for video argument in multiboot options */
>>> - vbe_used_mode = find_mode_using_cmdline(sorted_mode_params,
>>> - number_of_modes);
>>> - if (vbe_used_mode == (uint16_t)-1)
>>> + if (vbe_used_mode == (uint16_t)AUTO_SELECT)
>>> {
>>> - printk(FB_VESA_NAME " video on command line not provided"
>>> + printk(FB_VESA_NAME " auto video mode selected"
>>> "\n\ttrying EDID ...\n");
>>> /* second search monitor for good resolution */
>>> vbe_used_mode = find_mode_using_EDID(sorted_mode_params,
>>> --
>>> 1.9.1
>>>
>>> _______________________________________________
>>> devel mailing list
>>> devel@rtems.org
>>> http://lists.rtems.org/mailman/listinfo/devel
_______________________________________________
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel
