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

[Devarsh Thakkar]

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.

[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