| Message ID | 20231110152944.647535-6-devarsht@ti.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Anatolij Gustschin |
| Headers | show |
| Series | Move framebuffer reservation for SPL to RAM end | expand |
Hi Devarsh, On Fri, 10 Nov 2023 at 08:29, Devarsh Thakkar <devarsht@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@chromium.org> > Signed-off-by: Devarsh Thakkar <devarsht@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 > + 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
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@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@chromium.org> >> Signed-off-by: Devarsh Thakkar <devarsht@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
Hi Devarsh, On Sat, 25 Nov 2023 at 07:27, Devarsh Thakkar <devarsht@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@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@chromium.org> > >> Signed-off-by: Devarsh Thakkar <devarsht@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@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
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@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@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@chromium.org> >>>> Signed-off-by: Devarsh Thakkar <devarsht@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@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
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; + 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);
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 framework expects them to be filled for some of the functionalities. Reported-by: Simon Glass <sjg@chromium.org> Signed-off-by: Devarsh Thakkar <devarsht@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(-)