[PATCH v2 5/6] video: Fill video handoff in video post probe

[Devarsh Thakkar]

Hi Simon,

Thanks for the review.

On 02/12/23 23:53, Simon Glass wrote:
> Hi Devarsh,
> 
> On Sat, 25 Nov 2023 at 07:27, Devarsh Thakkar <devarsht at ti.com> wrote:
>>
>> Hi Simon,
>>
>> Thanks for the review.
>>
>> On 13/11/23 01:31, Simon Glass wrote:
>>> Hi Devarsh,
>>>
>>> On Fri, 10 Nov 2023 at 08:29, Devarsh Thakkar <devarsht at ti.com> wrote:
>>>>
>>>> Fill video handoff fields in video_post_probe
>>>> as at this point we have full framebuffer-related
>>>> information.
>>>>
>>>> Also fill all the fields available in video hand-off
>>>> struct as those were missing earlier and U-boot
>>>
>>> U-Boot
>>>
>>>> framework expects them to be filled for some of the
>>>> functionalities.
>>>
>>> Can you wrap your commit messages to closer to 72 chars?
>>>
>>>>
>>>> Reported-by: Simon Glass <sjg at chromium.org>
>>>> Signed-off-by: Devarsh Thakkar <devarsht at ti.com>
>>>> ---
>>>> V2:
>>>> - No change
>>>>
>>>> V3:
>>>> - Fix commit message per review comment
>>>> - Add a note explaining assumption of single framebuffer
>>>> ---
>>>>  drivers/video/video-uclass.c | 29 +++++++++++++++++++----------
>>>>  1 file changed, 19 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/drivers/video/video-uclass.c b/drivers/video/video-uclass.c
>>>> index f619b5ae56..edc3376b46 100644
>>>> --- a/drivers/video/video-uclass.c
>>>> +++ b/drivers/video/video-uclass.c
>>>> @@ -154,16 +154,6 @@ int video_reserve(ulong *addrp)
>>>>         debug("Video frame buffers from %lx to %lx\n", gd->video_bottom,
>>>>               gd->video_top);
>>>>
>>>> -       if (spl_phase() == PHASE_SPL && CONFIG_IS_ENABLED(VIDEO_HANDOFF)) {
>>>> -               struct video_handoff *ho;
>>>> -
>>>> -               ho = bloblist_add(BLOBLISTT_U_BOOT_VIDEO, sizeof(*ho), 0);
>>>> -               if (!ho)
>>>> -                       return log_msg_ret("blf", -ENOENT);
>>>> -               ho->fb = *addrp;
>>>> -               ho->size = size;
>>>> -       }
>>>> -
>>>>         return 0;
>>>>  }
>>>>
>>>> @@ -559,6 +549,25 @@ static int video_post_probe(struct udevice *dev)
>>>>
>>>>         priv->fb_size = priv->line_length * priv->ysize;
>>>>
>>>> +       /*
>>>> +        * Set up video handoff fields for passing video blob to next stage
>>>> +        * NOTE:
>>>> +        * This assumes that reserved video memory only uses a single framebuffer
>>>> +        */
>>>> +       if (spl_phase() == PHASE_SPL && CONFIG_IS_ENABLED(BLOBLIST)) {
>>>> +               struct video_handoff *ho;
>>>> +
>>>> +               ho = bloblist_add(BLOBLISTT_U_BOOT_VIDEO, sizeof(*ho), 0);
>>>> +               if (!ho)
>>>> +                       return log_msg_ret("blf", -ENOENT);
>>>> +               ho->fb = gd->video_bottom;
>>>> +               ho->size = gd->video_top - gd->video_bottom;
>>>
>>> should be plat->base and plat->size
>>>
>>
>> plat->size contains the unaligned size actually. While reserving video memory,
>> the size of allocation is updated [0] as per default alignment (1 MiB) or
>> alignment requested by driver. So I thought it is better to pass actual
>> allocated size calculated using gd as the next stage receiving hand-off can
>> directly skip the region as per passed size. And since I used gd for
>> calculating size, I thought to stick to using gd for ho->fb too for consistency.
>>
>> Kindly let me know if any queries.
> 
> This sort of thing would have been useful to put in a comment in the
> code, or commit message.
> 

Thanks, will add it in comment and commit message.

> I still don't really see why the 'aligned' size isn't already in plat,
> after alloc_fb() is called.
> 

alloc_fb doesn't update plat->size as it is kept intact (unaligned)

Regards
Devarsh

> Anyway I will leave this to Anatolij
> 
> Reviewed-by: Simon Glass <sjg at chromium.org>
> 
>>
>> [0]:
>> https://source.denx.de/u-boot/u-boot/-/blob/u-boot-2023.07.y/drivers/video/video-uclass.c?ref_type=heads#L88
>>
>> Regards
>> Devarsh
>>
>>>> +               ho->xsize = priv->xsize;
>>>> +               ho->ysize = priv->ysize;
>>>> +               ho->line_length = priv->line_length;
>>>> +               ho->bpix = priv->bpix;
>>>> +       }
>>>> +
>>>>         if (IS_ENABLED(CONFIG_VIDEO_COPY) && plat->copy_base)
>>>>                 priv->copy_fb = map_sysmem(plat->copy_base, plat->size);
>>>>
>>>> --
>>>> 2.34.1
>>>>
>>>
>>> Regards,
>>> Simon