[U-Boot] [PATCH v4 2/2] splash: add support for loading splash from a FIT image

[Simon Glass]

Hi Igor,

On 12 December 2016 at 01:37, Igor Grinberg <grinberg at compulab.co.il> wrote:
> On 12/11/16 22:27, Simon Glass wrote:
>> Hi Igor,
>>
>> On 11 December 2016 at 10:37, Igor Grinberg <grinberg at compulab.co.il> wrote:
>>> Hi Tomas, Simon,
>>>
>>> Sorry, to break in that late...
>>> I have a quick question below.
>>>
>>> On 12/05/16 09:36, Tomas Melin wrote:
>>>> Enable support for loading a splash image from within a FIT image.
>>>> The image is assumed to be generated with mkimage -E flag to hold
>>>> the data external to the FIT.
>>>>
>>>> Signed-off-by: Tomas Melin <tomas.melin at vaisala.com>
>>>
>>> [...]
>>>
>>>> diff --git a/common/splash_source.c b/common/splash_source.c
>>>> index 70d724f..94b46d3 100644
>>>> --- a/common/splash_source.c
>>>> +++ b/common/splash_source.c
>>>
>>> [...]
>>>
>>>> +#ifdef CONFIG_FIT
>>>> +static int splash_load_fit(struct splash_location *location, u32 bmp_load_addr)
>>>> +{
>>>> +     int res;
>>>> +     int node_offset;
>>>> +     int splash_offset;
>>>> +     int splash_size;
>>>> +     struct image_header *img_header;
>>>> +     const u32 *fit_header;
>>>> +     u32 fit_size;
>>>> +     const size_t header_size = sizeof(struct image_header);
>>>> +
>>>> +     /* Read in image header */
>>>> +     res = splash_storage_read_raw(location, bmp_load_addr, header_size);
>>>> +     if (res < 0)
>>>> +             return res;
>>>> +
>>>> +     img_header = (struct image_header *)bmp_load_addr;
>>>> +     fit_size = fdt_totalsize(img_header);
>>>> +
>>>> +     /* Read in entire FIT */
>>>> +     fit_header = (const u32 *)(bmp_load_addr + header_size);
>>>> +     res = splash_storage_read_raw(location, (u32)fit_header, fit_size);
>>>> +     if (res < 0)
>>>> +             return res;
>>>> +
>>>> +     res = fit_check_format(fit_header);
>>>> +     if (!res) {
>>>> +             debug("Could not find valid FIT image\n");
>>>> +             return -EINVAL;
>>>> +     }
>>>> +
>>>> +     node_offset = fit_image_get_node(fit_header, location->name);
>>>> +     if (node_offset < 0) {
>>>> +             debug("Could not find splash image '%s' in FIT\n",
>>>> +                   location->name);
>>>> +             return -ENOENT;
>>>> +     }
>>>> +
>>>
>>> I think two above debug() are very legitimate - no need to shout if no FIT image
>>> or no splash in it...
>>>
>>>> +     res = fit_image_get_data_offset(fit_header, node_offset,
>>>> +                                     &splash_offset);
>>>> +     if (res < 0) {
>>>> +             debug("Could not find 'data-offset' property in FIT\n");
>>>> +             return res;
>>>> +     }
>>>> +
>>>> +     res = fit_image_get_data_size(fit_header, node_offset, &splash_size);
>>>> +     if (res < 0) {
>>>> +             debug("Could not find 'data-size' property in FIT\n");
>>>> +             return res;
>>>> +     }
>>>
>>> Now regarding these two, I'm not sure.
>>> Since we have found a valid FIT and also a node with a correct splash name,
>>> probably the intent is that we show the splash, right?
>>> But in the two above checks, we find inconsistencies that do not allow us to
>>> show the splash - meaning the FIT is not actually good (am I right here?).
>>> So may be we should report it to the 'user' and allow correcting the FIT?
>>> Otherwise, it is impossible to debug the image w/o a debug version of U-Boot...
>>> Do I make sense, or do I miss something?
>>
>> Yes that makes some sense, but the problem is that then you are
>> including error messages always which would never happen in a working
>> system (i.e. it just bloats the code).
>
> Unless, there a kind of corruption or a user mistake and then that same
> user can't even understand what happened because we do not help him with
> an error message.
> You cannot know that these error messages will never happen...
> This is a generic code which can be used by a wide variety of platforms -
> I don't think you can foresee all the possible use cases.
>
> We are talking about several tens of bytes vs. usability.
> If there is an error, it should be stated as such. It should not just
> exit silently...

I agree with that, there should definitely be an error printed. It
should say something like 'Failed to load splash screen (err=-4)' or
something like that. The error number should provide some clues and
people can dig in.

This patch would add around 200 bytes if debug() was changed to
printf(). Multiply that by several hundred if we did that sort of
thing throughout U-Boot.

>
> I think that debug() is a good thing to use, but we shouldn't be obsessed
> with it.

Fair enough, I don't want to get obsessed about it either! But general
code-size bloat is a real problem. so I think in general we should
make sure an error is printed when one occurs, and only cover some
common cases where the user can actually fix it (e.g SD card missing)
with more detailed error messages.

>
>>
>> So long as the error is reported (even if it is not a very specific
>> error), people can add DEBUG and track it down.
>
> That depends who 'people' are? Devs? Users?

Well in this case the user will never see the problem, unless someone
has screwed up the splash screen image. Mostly I'm talking about devs.

Better would be to have an expanded debug() system which lets you turn
debugging on globally when hunting for a problem. That would be a nice
project for someone...

Regards,
Simon