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

[Igor Grinberg <grinberg@compulab.co.il>]

On 12/13/16 22:29, Simon Glass wrote:
> Hi Igor,
> 
> On 12 December 2016 at 01:37, Igor Grinberg <grinberg@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@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@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.

Great idea!

> 
> 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.

Well, I thought about using printk only on a half of the above messages
and that gives about ~90 bytes, while it also can be optimized by using
the same parameterized string and thus add only ~50 bytes...

> 
>>
>> 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...

Yes, indeed that sounds like a nice project.
It would be great to be able to specify the debug verbosity on per build
basis (e.g. Kconfig).


-- 
Regards,
Igor.