Re: [PATCH v2 5/5] drivers: media: platform: Add NPCM Video Capture/Encode Engine driver

[Kun-Fa Lin]

Dear Paul,

These problems have been addressed in the new patch.
Could you please help to review the new patch v4? Thanks.

Regards,
Marvin

Kun-Fa Lin <milkfafa@xxxxxxxxx> 於 2022年5月17日 週二 上午10:59寫道：
>
> Dear Paul,
>
> Thanks for your review and comments.
>
> > Please mention the datasheet name and revision used to implement this?
> > How can your patch be tested?
> >
> > For a over 2000 line patch, I would expect a longer commit message with
> > a summary of the hardware features, and implementation.
>
> Okay, I'll add more information to the commit message, but it may not
> be appropriate to add the datasheet name since it is not public.
> And I tested with openbmc/obmc-ikvm (with patches to support Hextile
> encoding that our driver used) and used VNC Viewer to verify the video
> result.
>
> >
> > As the module author should you also be added to the file `MAINTAINERS`?
> > (Maybe even with a functional address <linux-npcm-video@xxxxxxxxxxx>?
> >
> > > Signed-off-by: Marvin Lin <kflin@xxxxxxxxxxx>
> >
> > Same comment as in 1/5 regarding the author email address.
>
> I'll add a new entry in MAINTAINERS.
>
> > > +++ b/drivers/media/platform/nuvoton/Kconfig
> > > @@ -0,0 +1,12 @@
> > > +# SPDX-License-Identifier: GPL-2.0-only
> > > +
> > > +comment "Nuvoton media platform drivers"
> > > +
> > > +config VIDEO_NUVOTON
> >
> > Is that driver going to support all Nuvoton devices or just NPCM? If
> > only NPCM, that should be part of the Kconfig config name?
> >
> > > +     tristate "Nuvoton NPCM Video Capture/Encode Engine driver"
> > > +     depends on V4L_PLATFORM_DRIVERS
> > > +     depends on VIDEO_DEV
> > > +     select VIDEOBUF2_DMA_CONTIG
> > > +     help
> > > +       Support for the Video Capture/Differentiation Engine (VCD) and
> > > +       Encoding Compression Engine (ECE) present on Nuvoton NPCM SoCs.
> >
> > Mention the module name?
> >
> >  > To compile this driver as a module, choose M here: the module will be
> > called XXX.
>
> The driver just supports NPCM. I'll change the config to
> VIDEO_NUVOTON_NPCM_VCD_ECE.
>
> > > +struct nuvoton_video_addr {
> > > +     unsigned int size;
> >
> > size_t?
>
> > > +struct rect_list_info {
> > > +     struct rect_list *list;
> > > +     struct rect_list *first;
> > > +     struct list_head *head;
> > > +     int index;
> > > +     int tile_perline;
> > > +     int tile_perrow;
> > > +     int offset_perline;
> > > +     int tile_size;
> > > +     int tile_cnt;
> >
> > Can all of these be unsigned?
>
> > > +     int frame_rate;
> > > +     int vb_index;
> >
> > Unsigned?
> >
>
> They will be addressed in the next patch.
>
> > > +     u32 bytesperline;
> > > +     u8 bytesperpixel;
> > > +     u32 rect_cnt;
> > > +     u8 num_buffers;
> > > +     struct list_head *list;
> > > +     u32 *rect;
> >
> > I would not limit the size?
>
> It's clearer to know that it stores u32 exactly.
>
> > > +static u32 nuvoton_video_ece_get_ed_size(struct nuvoton_video *video,
> > > +                                      u32 offset, void *addr)
> >
> > Use unsigned int as return value?
>
> Okay.
>
> > > +static void nuvoton_video_ece_enc_rect(struct nuvoton_video *video, u32 r_off_x,
> > > +                                    u32 r_off_y, u32 r_w, u32 r_h)
> > > +{
> > > +     struct regmap *ece = video->ece.regmap;
> > > +     u32 rect_offset = (r_off_y * video->bytesperline) + (r_off_x * 2);
> > > +     u32 temp;
> > > +     u32 w_tile;
> > > +     u32 h_tile;
> > > +     u32 w_size = ECE_TILE_W;
> > > +     u32 h_size = ECE_TILE_H;
> >
> > Any reason to fix the sizes?
>
> A "Hextile" is fixed to 16x16 pixels size, which is defined in Remote
> Framebuffer Protocol (RFC 6143, chapter 7.7.4 Hextile Encoding).
>
> > > +static void nuvoton_video_ece_ip_reset(struct nuvoton_video *video)
> > > +{
> > > +     reset_control_assert(video->ece.reset);
> > > +     msleep(100);
> > > +     reset_control_deassert(video->ece.reset);
> > > +     msleep(100);
> >
> > 100 ms is quite long. Please add a comment, where that is documented. Is
> > there a way to poll, if the device is done?
>
> I'll add a comment. It should be reduced to ~10 us (suggested in
> spec.) and there's no way to poll.
>
> > > +
> > > +static void nuvoton_video_free_diff_table(struct nuvoton_video *video)
> > > +{
> > > +     struct list_head *head, *pos, *nx;
> > > +     struct rect_list *tmp;
> > > +     int i;
> >
> > unsigned?
> >
>
> > > +static int nuvoton_video_find_rect(struct nuvoton_video *video,
> > > +                                struct rect_list_info *info, u32 offset)
> > > +{
> > > +     int i = info->index;
> > > +
> > > +     if (offset < info->tile_perline) {
> > > +             info->list = nuvoton_video_new_rect(video, offset, i);
> >
> > `i` is only used here, so use `info->index`?
> >
>
> > > +static int nuvoton_video_build_table(struct nuvoton_video *video,
> > > +                                  struct rect_list_info *info)
> > > +{
> > > +     int i = info->index;
> > > +     int j, ret, bit;
> >
> > Make `j` unsigned?
> >
> > > +     u32 value;
> > > +     struct regmap *vcd = video->vcd_regmap;
> > > +
> > > +     for (j = 0; j < info->offset_perline; j += 4) {
> > > +             regmap_read(vcd, VCD_DIFF_TBL + (j + i), &value);
> >
> > `i` is only used here, so use `info->index`?
> >
>
> > > +static void nuvoton_video_vcd_ip_reset(struct nuvoton_video *video)
> > > +{
> > > +     reset_control_assert(video->reset);
> > > +     msleep(100);
> > > +     reset_control_deassert(video->reset);
> > > +     msleep(100);
> >
> > 100 ms is quite long. Please add a comment, where that is documented. Is
> > there a way to poll, if the device is done?
> >
>
> > > +static int nuvoton_video_queue_setup(struct vb2_queue *q,
> > > +                                  unsigned int *num_buffers,
> > > +                                  unsigned int *num_planes,
> > > +                                  unsigned int sizes[],
> > > +                                  struct device *alloc_devs[])
> > > +{
> > > +     struct nuvoton_video *video = vb2_get_drv_priv(q);
> > > +     int i;
> >
> > unsigned?
> >
>
> > > +static void nuvoton_video_buf_queue(struct vb2_buffer *vb)
> > > +{
> > > +     int empty;
> > > +     struct nuvoton_video *video = vb2_get_drv_priv(vb->vb2_queue);
> > > +     struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> > > +     struct nuvoton_video_buffer *nvb = to_nuvoton_video_buffer(vbuf);
> > > +     unsigned long flags;
> > > +
> > > +     dev_dbg(video->dev, "%s\n", __func__);
> > > +
> > > +     spin_lock_irqsave(&video->lock, flags);
> > > +     empty = list_empty(&video->buffers);
> >
> > Where is empty read later?
> >
>
> > > +     regs = devm_platform_ioremap_resource_byname(pdev, VCD_MODULE_NAME);
> > > +     if (IS_ERR(regs)) {
> > > +             dev_err(&pdev->dev, "Failed to get VCD regmap resource!\n");
> >
> > Can you help the user more by saying what to fix like check devicetree
> > or so?
> >
>
> Okay. All of them will be addressed in the next patch.
>
> Regards,
> Marvin