Inki Dae wrote:
> Hi Tobias,
>
> Thanks for touching this code.
>
> But I think below changes chould be explained exactly. Anyway, below are my comments,
>
> 2017년 11월 23일 23:19에 Tobias Jakobi 이(가) 쓴 글:
>> If an interlaced video mode is selected, a IOMMU pagefault is
>> triggered by vp_video_buffer().
>>
>> Fix the most apparent bugs:
>> - pitch value for chroma plane
>> - divide by two of height and vpos
>>
>> This is more like a band-aid at this point. The VP plane is
>> still corrupted, but at least there are no more pagefaults.
>>
>> Signed-off-by: Tobias Jakobi <tjakobi@xxxxxxxxxxxxxxxxxxxxx>
>> ---
>> drivers/gpu/drm/exynos/exynos_mixer.c | 21 +++++++++++----------
>> 1 file changed, 11 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
>> index dc5d79465f9b..a18426379e57 100644
>> --- a/drivers/gpu/drm/exynos/exynos_mixer.c
>> +++ b/drivers/gpu/drm/exynos/exynos_mixer.c
>> @@ -485,7 +485,7 @@ static void vp_video_buffer(struct mixer_context *ctx,
>> chroma_addr[1] = chroma_addr[0] + 0x40;
>> } else {
>> luma_addr[1] = luma_addr[0] + fb->pitches[0];
>> - chroma_addr[1] = chroma_addr[0] + fb->pitches[0];
>> + chroma_addr[1] = chroma_addr[0] + fb->pitches[1];
>
> chroma_addr[1] is set to VP_BOT_C_PTR register and datasheet says about this register,
> "specifies base address for Chrominance of bottom field"
>
> Before that, we would need to look into NV12 pixel format and its video signal mode - interlaced or progressive.
You're mixing something up here. The buffer is (progressive) NV12, but the video
mode is interlaced.
> NV12 interfaced
> YYYYYYYYYY
> YYYYYYYYYY
> UVUVUVUVUV
> YYYYYYYYYY
> YYYYYYYYYY
> UVUVUVUVUV
>
> NV12 progressive
> YYYYYYYYYY
> YYYYYYYYYY
> YYYYYYYYYY
> YYYYYYYYYY
> UVUVUVUVUV
> UVUVUVUVUV
>
> As you can see above, the offset of the gem buffer should be decided according to video signal mode, and 'base address + the offset' should be set to chroma_addr[1] register properly.
>
> And also there would be one thing more we have to consider,
> User space can make two separated gem buffers - one is for luminance pixel data and other is for chrominance pixel data - and send them to KMS driver, or he can make one contiguous gem buffer which contains two kinds of pixel data and send it to KMS driver.
>
> I guess now vp side of mixer driver didn't manage these buffers properly so page fault happened. So I think a good way for it would be to handle two kinds of buffers properly - one continuous buffer or two separated buffers - through exynos_drm_gem_fb_dma_addr function, and calculate the offset properly according to video signal mode - interfaced or progressive.
>
> Regarding this, we had posted one patch and it had been merged to mainline. This patch added two new pixel formats, DRM_FORMAT_NV12M and DRM_FORMAT_YUV420M to judge how KMS driver should handle these gem buffers.
> However, this patch had been reverted[1] by Ville due to breaking the ABI. So the only way to identify buffer type would be to check how many buffers are set to exynos_drm_fb object.
That's not related. In fact the second part (div by 2) is what actually removes
the pagefaults.
- Tobias
> [1] https://lists.freedesktop.org/archives/dri-devel/2012-April/021812.html
>
> Thanks,
> Inki Dae
>
>> }
>> } else {
>> luma_addr[1] = 0;
>> @@ -507,25 +507,26 @@ static void vp_video_buffer(struct mixer_context *ctx,
>> vp_reg_write(ctx, VP_IMG_SIZE_Y, VP_IMG_HSIZE(fb->pitches[0]) |
>> VP_IMG_VSIZE(fb->height));
>> /* chroma plane for NV12/NV21 is half the height of the luma plane */
>> - vp_reg_write(ctx, VP_IMG_SIZE_C, VP_IMG_HSIZE(fb->pitches[0]) |
>> + vp_reg_write(ctx, VP_IMG_SIZE_C, VP_IMG_HSIZE(fb->pitches[1]) |
>> VP_IMG_VSIZE(fb->height / 2));
>>
>> vp_reg_write(ctx, VP_SRC_WIDTH, state->src.w);
>> - vp_reg_write(ctx, VP_SRC_HEIGHT, state->src.h);
>> vp_reg_write(ctx, VP_SRC_H_POSITION,
>> VP_SRC_H_POSITION_VAL(state->src.x));
>> - vp_reg_write(ctx, VP_SRC_V_POSITION, state->src.y);
>>
>> - vp_reg_write(ctx, VP_DST_WIDTH, state->crtc.w);
>> - vp_reg_write(ctx, VP_DST_H_POSITION, state->crtc.x);
>> if (test_bit(MXR_BIT_INTERLACE, &ctx->flags)) {
>> - vp_reg_write(ctx, VP_DST_HEIGHT, state->crtc.h / 2);
>> - vp_reg_write(ctx, VP_DST_V_POSITION, state->crtc.y / 2);
>> + vp_reg_write(ctx, VP_SRC_HEIGHT, state->src.h / 2);
>> + vp_reg_write(ctx, VP_SRC_V_POSITION, state->src.y / 2);
>> } else {
>> - vp_reg_write(ctx, VP_DST_HEIGHT, state->crtc.h);
>> - vp_reg_write(ctx, VP_DST_V_POSITION, state->crtc.y);
>> + vp_reg_write(ctx, VP_SRC_HEIGHT, state->src.h);
>> + vp_reg_write(ctx, VP_SRC_V_POSITION, state->src.y);
>> }
>>
>> + vp_reg_write(ctx, VP_DST_WIDTH, state->crtc.w);
>> + vp_reg_write(ctx, VP_DST_H_POSITION, state->crtc.x);
>> + vp_reg_write(ctx, VP_DST_HEIGHT, state->crtc.h);
>> + vp_reg_write(ctx, VP_DST_V_POSITION, state->crtc.y);
>> +
>> vp_reg_write(ctx, VP_H_RATIO, state->h_ratio);
>> vp_reg_write(ctx, VP_V_RATIO, state->v_ratio);
>>
>>
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
