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

2023-12-05 Thread 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  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  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 
 Signed-off-by: Devarsh Thakkar 
 ---
 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 
> 
>>
>> [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

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

2023-12-02 Thread Simon Glass 
Hi Devarsh,

On Sat, 25 Nov 2023 at 07:27, Devarsh Thakkar  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  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 
> >> Signed-off-by: Devarsh Thakkar 
> >> ---
> >> 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 


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

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

2023-11-25 Thread 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  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 
>> Signed-off-by: Devarsh Thakkar 
>> ---
>> 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

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

2023-11-12 Thread Simon Glass 
Hi Devarsh,

On Fri, 10 Nov 2023 at 08:29, Devarsh Thakkar  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 
> Signed-off-by: Devarsh Thakkar 
> ---
> 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

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

2023-11-10 Thread Devarsh Thakkar 
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 
Signed-off-by: Devarsh Thakkar 
---
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;
+   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