On 02/22/2018 07:26 PM, Laurent Pinchart wrote:
> Hi Sergei,
>
> Thank you for the patch.
>
> On Thursday, 18 January 2018 16:05:51 EET Sergei Shtylyov wrote:
>> Commit d455b45f8393 ("v4l: vsp1: Add support for new VSP2-BS, VSP2-DL,
>> and VSP2-D instances") added support for the VSP2-D found in the R-Car
>> V3M (R8A77970) but the video output that VSP2-D sends to DU has a greenish
>> garbage-like line repeated every 8 screen rows. It turns out that R-Car
>> V3M has the LIF0 buffer attribute register that you need to set to a non-
>> default value in order to get rid of the output artifacts...
>>
>> Based on the original (and large) patch by Daisuke Matsushita
>> <daisuke.matsushita.ns@xxxxxxxxxxx>.
>>
>> Fixes: d455b45f8393 ("v4l: vsp1: Add support for new VSP2-BS, VSP2-DL and
>> VSP2-D instances") Signed-off-by: Sergei Shtylyov
>> <sergei.shtylyov@xxxxxxxxxxxxxxxxxx>
>>
>> ---
>> This patch is against the 'media_tree.git' repo's 'fixes' branch.
>>
>> Changes in version 3:
>> - reworded the comment in lif_configure();
>> - reworded the patch description.
>>
>> Changes in version 2:
>> - added a comment before the V3M SoC check;
>> - fixed indetation in that check;
>> - reformatted the patch description.
>>
>> drivers/media/platform/vsp1/vsp1_lif.c | 15 +++++++++++++++
>> drivers/media/platform/vsp1/vsp1_regs.h | 5 +++++
>> 2 files changed, 20 insertions(+)
>>
>> Index: media_tree/drivers/media/platform/vsp1/vsp1_lif.c
>> ===================================================================
>> --- media_tree.orig/drivers/media/platform/vsp1/vsp1_lif.c
>> +++ media_tree/drivers/media/platform/vsp1/vsp1_lif.c
>> @@ -155,6 +155,21 @@ static void lif_configure(struct vsp1_en
>> (obth << VI6_LIF_CTRL_OBTH_SHIFT) |
>> (format->code == 0 ? VI6_LIF_CTRL_CFMT : 0) |
>> VI6_LIF_CTRL_REQSEL | VI6_LIF_CTRL_LIF_EN);
>> +
>> + /*
>> + * On R-Car V3M the LIF0 buffer attribute register has to be set
>> + * to a non-default value to guarantee proper operation (otherwise
>> + * artifacts may appear on the output). The value required by
>> + * the manual is not explained but is likely a buffer size or
>> + * threshold...
>
> One period is enough :-)
Naw, I just like ellipsis! :-)
>> + */
>> + if ((entity->vsp1->version &
>> + (VI6_IP_VERSION_MODEL_MASK | VI6_IP_VERSION_SOC_MASK)) ==
>> + (VI6_IP_VERSION_MODEL_VSPD_V3 | VI6_IP_VERSION_SOC_V3M)) {
>> + vsp1_lif_write(lif, dl, VI6_LIF_LBA,
>> + VI6_LIF_LBA_LBA0 |
>> + (1536 << VI6_LIF_LBA_LBA1_SHIFT));
>> + }
>
> There's no need for braces
Well, multi-line statement was asking for them...
> or inner parentheses.
I thought so too. :-)
> To make this easier to read I
> propose defining a new macro VI6_IP_VERSION_MASK that combines both the model
> and SoC. Otherwise this looks good to me,
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
>
> I'll post a v4 with that change in reply to this e-mail, please let me know if
> you're fine with it.
Generally fine, yes...
[...]
MBR, Sergei
