[PATCH 1/2] i386/pc386/VESA framebuffer driver: modified and extended initialization options

Gedare Bloom gedare at rtems.org
Tue May 19 17:19:11 UTC 2015


On Tue, May 19, 2015 at 1:09 PM, Jan Dolezal <dolezj21 at 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 <dolezj21 at 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.sever.h at 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 at rtems.org
>>> http://lists.rtems.org/mailman/listinfo/devel



More information about the devel mailing list