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

Pavel Pisa pisa at cmp.felk.cvut.cz
Tue May 19 21:40:22 UTC 2015


Hello Gedare,

the first thanks for review.

On Tuesday 19 of May 2015 19:11:13 Jan Dolezal 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.

This variable has to be uninitialized/placed in COMMON section by default
in the RTEMS library to allow its declaration and initial value definition
in application if it requests/needs to initialize graphic.

But you are right that it should/needs to be declared in some header file
to suppress warning during compilation and to make variable visible/documented
for user - even that he/she does not need declaration because it has to be
defined by initialization. But again, warnings rule count even for user.
So it should go to some public header.

> >> +/**
> >>    * @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.

I have not spotted first that uint16_t is used as a function return value.
It is almost general rule that use of fixed size 16-bit return types
and in most cases even parameters is not optimal for 32-bit systems.
It requires instruction prefix on x86 in 32-bit mode for example and
complicates arithmetic which needs to limit range (but not by check/exception
in C but by modulo arithmetic).

Because we know for sure that the code is x86 specific and that RTEMS
newer target 16-bit x86 mode then my suggestion is to change return type
to int for all functions returning graphic mode. I.e. 

  static int find_mode_using_cmdline(Mode_params *mode_list,
                                        uint8_t list_length)

  static int find_mode_using_EDID( Mode_params *mode_list,
                                      uint8_t list_length)

etc.

Then values form 0 to 0xffff can directly map to BIOS defined values
and there is left whole negative range to report errors by values
from above enum.

This solution has advantage that there are no longer required ugly constructs
like

      if (find_mode_by_resolution(mode_list, list_length, &EDIDmode) !=
          (uint16_t)-1)

and straightforward and less error prone construct

      if (find_mode_by_resolution(mode_list, list_length, &EDIDmode) !=
          NO_SUITABLE_MODE)

could be used there.

To Jan Dolezal, please, look for all functions returning mode and comparing
the result and simplify the code.

> >>   /**
> >>    * @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.

This works for most/all C implementations but if I remember it is
defined as implementation specific behavior and does not conform
requirements for portable and code. But I think that above suggestion
should help to eliminate all these cases in the code.

Best wishes,

              Pavel Pisa


More information about the devel mailing list