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

[Simon Glass]

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.

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

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