| Message ID | 20220614191127.3420492-54-paul.elder@ideasonboard.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | media: rkisp1: Cleanups and add support for i.MX8MP | expand |
Hi Paul, Thank you for the patch. On Wed, Jun 15, 2022 at 04:11:25AM +0900, Paul Elder wrote: > On the ISP that is integrated in the i.MX8MP, the DMA base addresses are > encoded in 34-bit. Shift them to the left by 2 bits so that they can be I think you meant right, not left. > contained in 32 bits. The important part here is that this is how the address is encoded in the hardware. I suppose it's obvious, otherwise it woudln't work at all, but maybe it could be explained more explicitly ? On the ISP that is integrated in the i.MX8MP, DMA addresses have been extended to 34 bits, with the 32 MSBs stored in the DMA address registers and the 2 LSBs set to 0. Shift the buffer addresses right by 2 on that platform. > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > .../platform/rockchip/rkisp1/rkisp1-capture.c | 19 +++++++++++-------- > .../platform/rockchip/rkisp1/rkisp1-common.h | 1 + > .../platform/rockchip/rkisp1/rkisp1-dev.c | 3 ++- > 3 files changed, 14 insertions(+), 9 deletions(-) > > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c > index 35cec263c563..234b1f8488cb 100644 > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c > @@ -624,6 +624,9 @@ static void rkisp1_dummy_buf_destroy(struct rkisp1_capture *cap) > > static void rkisp1_set_next_buf(struct rkisp1_capture *cap) > { > + u8 shift = cap->rkisp1->info->features & RKISP1_FEATURE_DMA_34BIT ? > + 2 : 0; > + > cap->buf.curr = cap->buf.next; > cap->buf.next = NULL; > > @@ -636,7 +639,7 @@ static void rkisp1_set_next_buf(struct rkisp1_capture *cap) > buff_addr = cap->buf.next->buff_addr; > > rkisp1_write(cap->rkisp1, cap->config->mi.y_base_ad_init, > - buff_addr[RKISP1_PLANE_Y]); > + buff_addr[RKISP1_PLANE_Y] >> shift); > /* > * In order to support grey format we capture > * YUV422 planar format from the camera and > @@ -645,17 +648,17 @@ static void rkisp1_set_next_buf(struct rkisp1_capture *cap) > if (cap->pix.cfg->fourcc == V4L2_PIX_FMT_GREY) { > rkisp1_write(cap->rkisp1, > cap->config->mi.cb_base_ad_init, > - cap->buf.dummy.dma_addr); > + cap->buf.dummy.dma_addr >> shift); > rkisp1_write(cap->rkisp1, > cap->config->mi.cr_base_ad_init, > - cap->buf.dummy.dma_addr); > + cap->buf.dummy.dma_addr >> shift); > } else { > rkisp1_write(cap->rkisp1, > cap->config->mi.cb_base_ad_init, > - buff_addr[RKISP1_PLANE_CB]); > + buff_addr[RKISP1_PLANE_CB] >> shift); > rkisp1_write(cap->rkisp1, > cap->config->mi.cr_base_ad_init, > - buff_addr[RKISP1_PLANE_CR]); > + buff_addr[RKISP1_PLANE_CR] >> shift); > } > } else { > /* > @@ -663,11 +666,11 @@ static void rkisp1_set_next_buf(struct rkisp1_capture *cap) > * throw data if there is no available buffer. > */ > rkisp1_write(cap->rkisp1, cap->config->mi.y_base_ad_init, > - cap->buf.dummy.dma_addr); > + cap->buf.dummy.dma_addr >> shift); > rkisp1_write(cap->rkisp1, cap->config->mi.cb_base_ad_init, > - cap->buf.dummy.dma_addr); > + cap->buf.dummy.dma_addr >> shift); > rkisp1_write(cap->rkisp1, cap->config->mi.cr_base_ad_init, > - cap->buf.dummy.dma_addr); > + cap->buf.dummy.dma_addr >> shift); > } > > /* Set plane offsets */ > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h > index 96657e55a5b0..0b834579d08c 100644 > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h > @@ -113,6 +113,7 @@ enum rkisp1_feature { > RKISP1_FEATURE_DUAL_CROP = BIT(1), > RKISP1_FEATURE_RSZ_CROP = BIT(2), > RKISP1_FEATURE_MAIN_STRIDE = BIT(3), > + RKISP1_FEATURE_DMA_34BIT = BIT(4), > }; > > /* > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c > index d68a805e8b6b..4c77aa2bc50a 100644 > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c > @@ -506,7 +506,8 @@ static const struct rkisp1_info imx8mp_isp_info = { > .isr_size = ARRAY_SIZE(imx8mp_isp_isrs), > .isp_ver = IMX8MP_V10, > .features = RKISP1_FEATURE_RSZ_CROP > - | RKISP1_FEATURE_MAIN_STRIDE, > + | RKISP1_FEATURE_MAIN_STRIDE > + | RKISP1_FEATURE_DMA_34BIT, > }; > > static const struct of_device_id rkisp1_of_match[] = {
On 26.06.2022 14:38, Laurent Pinchart wrote: >Hi Paul, > >Thank you for the patch. > >On Wed, Jun 15, 2022 at 04:11:25AM +0900, Paul Elder wrote: >> On the ISP that is integrated in the i.MX8MP, the DMA base addresses are >> encoded in 34-bit. Shift them to the left by 2 bits so that they can be > >I think you meant right, not left. > >> contained in 32 bits. > >The important part here is that this is how the address is encoded in >the hardware. I suppose it's obvious, otherwise it woudln't work at all, >but maybe it could be explained more explicitly ? > >On the ISP that is integrated in the i.MX8MP, DMA addresses have been >extended to 34 bits, with the 32 MSBs stored in the DMA address >registers and the 2 LSBs set to 0. Shift the buffer addresses right by 2 >on that platform. If the 32 MSB are all stored in the in the dma address then why do we need to shift? > >> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > >Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >> --- >> .../platform/rockchip/rkisp1/rkisp1-capture.c | 19 +++++++++++-------- >> .../platform/rockchip/rkisp1/rkisp1-common.h | 1 + >> .../platform/rockchip/rkisp1/rkisp1-dev.c | 3 ++- >> 3 files changed, 14 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c >> index 35cec263c563..234b1f8488cb 100644 >> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c >> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c >> @@ -624,6 +624,9 @@ static void rkisp1_dummy_buf_destroy(struct rkisp1_capture *cap) >> >> static void rkisp1_set_next_buf(struct rkisp1_capture *cap) >> { >> + u8 shift = cap->rkisp1->info->features & RKISP1_FEATURE_DMA_34BIT ? >> + 2 : 0; >> + >> cap->buf.curr = cap->buf.next; >> cap->buf.next = NULL; >> >> @@ -636,7 +639,7 @@ static void rkisp1_set_next_buf(struct rkisp1_capture *cap) >> buff_addr = cap->buf.next->buff_addr; >> >> rkisp1_write(cap->rkisp1, cap->config->mi.y_base_ad_init, >> - buff_addr[RKISP1_PLANE_Y]); >> + buff_addr[RKISP1_PLANE_Y] >> shift); >> /* >> * In order to support grey format we capture >> * YUV422 planar format from the camera and >> @@ -645,17 +648,17 @@ static void rkisp1_set_next_buf(struct rkisp1_capture *cap) >> if (cap->pix.cfg->fourcc == V4L2_PIX_FMT_GREY) { >> rkisp1_write(cap->rkisp1, >> cap->config->mi.cb_base_ad_init, >> - cap->buf.dummy.dma_addr); >> + cap->buf.dummy.dma_addr >> shift); >> rkisp1_write(cap->rkisp1, >> cap->config->mi.cr_base_ad_init, >> - cap->buf.dummy.dma_addr); >> + cap->buf.dummy.dma_addr >> shift); >> } else { >> rkisp1_write(cap->rkisp1, >> cap->config->mi.cb_base_ad_init, >> - buff_addr[RKISP1_PLANE_CB]); >> + buff_addr[RKISP1_PLANE_CB] >> shift); >> rkisp1_write(cap->rkisp1, >> cap->config->mi.cr_base_ad_init, >> - buff_addr[RKISP1_PLANE_CR]); >> + buff_addr[RKISP1_PLANE_CR] >> shift); >> } >> } else { >> /* >> @@ -663,11 +666,11 @@ static void rkisp1_set_next_buf(struct rkisp1_capture *cap) >> * throw data if there is no available buffer. >> */ >> rkisp1_write(cap->rkisp1, cap->config->mi.y_base_ad_init, >> - cap->buf.dummy.dma_addr); >> + cap->buf.dummy.dma_addr >> shift); >> rkisp1_write(cap->rkisp1, cap->config->mi.cb_base_ad_init, >> - cap->buf.dummy.dma_addr); >> + cap->buf.dummy.dma_addr >> shift); >> rkisp1_write(cap->rkisp1, cap->config->mi.cr_base_ad_init, >> - cap->buf.dummy.dma_addr); >> + cap->buf.dummy.dma_addr >> shift); >> } >> >> /* Set plane offsets */ >> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h >> index 96657e55a5b0..0b834579d08c 100644 >> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h >> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h >> @@ -113,6 +113,7 @@ enum rkisp1_feature { >> RKISP1_FEATURE_DUAL_CROP = BIT(1), >> RKISP1_FEATURE_RSZ_CROP = BIT(2), >> RKISP1_FEATURE_MAIN_STRIDE = BIT(3), >> + RKISP1_FEATURE_DMA_34BIT = BIT(4), doc this field thanks, Dafna >> }; >> >> /* >> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c >> index d68a805e8b6b..4c77aa2bc50a 100644 >> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c >> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c >> @@ -506,7 +506,8 @@ static const struct rkisp1_info imx8mp_isp_info = { >> .isr_size = ARRAY_SIZE(imx8mp_isp_isrs), >> .isp_ver = IMX8MP_V10, >> .features = RKISP1_FEATURE_RSZ_CROP >> - | RKISP1_FEATURE_MAIN_STRIDE, >> + | RKISP1_FEATURE_MAIN_STRIDE >> + | RKISP1_FEATURE_DMA_34BIT, >> }; >> >> static const struct of_device_id rkisp1_of_match[] = { > >-- >Regards, > >Laurent Pinchart
Hi Dafna, On Fri, Jul 01, 2022 at 08:53:42AM +0300, Dafna Hirschfeld wrote: > On 26.06.2022 14:38, Laurent Pinchart wrote: > > On Wed, Jun 15, 2022 at 04:11:25AM +0900, Paul Elder wrote: > >> On the ISP that is integrated in the i.MX8MP, the DMA base addresses are > >> encoded in 34-bit. Shift them to the left by 2 bits so that they can be > > > > I think you meant right, not left. > > > >> contained in 32 bits. > > > > The important part here is that this is how the address is encoded in > > the hardware. I suppose it's obvious, otherwise it woudln't work at all, > > but maybe it could be explained more explicitly ? > > > > On the ISP that is integrated in the i.MX8MP, DMA addresses have been > > extended to 34 bits, with the 32 MSBs stored in the DMA address > > registers and the 2 LSBs set to 0. Shift the buffer addresses right by 2 > > on that platform. > > If the 32 MSB are all stored in the in the dma address then why do we > need to shift? On Rokchip SoCs the DMA address is 32-bit wide, and stored as-is in the registers. On i.MX8MP, the DMA address is 34-bit, and must be aligned to a multiple of 4 bytes, so the two LSBs are guaranteed to be 0. The DMA registers are still 32-bit wide, and store bits [33:2] of the DMA addresses, hence the shift. > >> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > >> --- > >> .../platform/rockchip/rkisp1/rkisp1-capture.c | 19 +++++++++++-------- > >> .../platform/rockchip/rkisp1/rkisp1-common.h | 1 + > >> .../platform/rockchip/rkisp1/rkisp1-dev.c | 3 ++- > >> 3 files changed, 14 insertions(+), 9 deletions(-) > >> > >> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c > >> index 35cec263c563..234b1f8488cb 100644 > >> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c > >> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c > >> @@ -624,6 +624,9 @@ static void rkisp1_dummy_buf_destroy(struct rkisp1_capture *cap) > >> > >> static void rkisp1_set_next_buf(struct rkisp1_capture *cap) > >> { > >> + u8 shift = cap->rkisp1->info->features & RKISP1_FEATURE_DMA_34BIT ? > >> + 2 : 0; > >> + > >> cap->buf.curr = cap->buf.next; > >> cap->buf.next = NULL; > >> > >> @@ -636,7 +639,7 @@ static void rkisp1_set_next_buf(struct rkisp1_capture *cap) > >> buff_addr = cap->buf.next->buff_addr; > >> > >> rkisp1_write(cap->rkisp1, cap->config->mi.y_base_ad_init, > >> - buff_addr[RKISP1_PLANE_Y]); > >> + buff_addr[RKISP1_PLANE_Y] >> shift); > >> /* > >> * In order to support grey format we capture > >> * YUV422 planar format from the camera and > >> @@ -645,17 +648,17 @@ static void rkisp1_set_next_buf(struct rkisp1_capture *cap) > >> if (cap->pix.cfg->fourcc == V4L2_PIX_FMT_GREY) { > >> rkisp1_write(cap->rkisp1, > >> cap->config->mi.cb_base_ad_init, > >> - cap->buf.dummy.dma_addr); > >> + cap->buf.dummy.dma_addr >> shift); > >> rkisp1_write(cap->rkisp1, > >> cap->config->mi.cr_base_ad_init, > >> - cap->buf.dummy.dma_addr); > >> + cap->buf.dummy.dma_addr >> shift); > >> } else { > >> rkisp1_write(cap->rkisp1, > >> cap->config->mi.cb_base_ad_init, > >> - buff_addr[RKISP1_PLANE_CB]); > >> + buff_addr[RKISP1_PLANE_CB] >> shift); > >> rkisp1_write(cap->rkisp1, > >> cap->config->mi.cr_base_ad_init, > >> - buff_addr[RKISP1_PLANE_CR]); > >> + buff_addr[RKISP1_PLANE_CR] >> shift); > >> } > >> } else { > >> /* > >> @@ -663,11 +666,11 @@ static void rkisp1_set_next_buf(struct rkisp1_capture *cap) > >> * throw data if there is no available buffer. > >> */ > >> rkisp1_write(cap->rkisp1, cap->config->mi.y_base_ad_init, > >> - cap->buf.dummy.dma_addr); > >> + cap->buf.dummy.dma_addr >> shift); > >> rkisp1_write(cap->rkisp1, cap->config->mi.cb_base_ad_init, > >> - cap->buf.dummy.dma_addr); > >> + cap->buf.dummy.dma_addr >> shift); > >> rkisp1_write(cap->rkisp1, cap->config->mi.cr_base_ad_init, > >> - cap->buf.dummy.dma_addr); > >> + cap->buf.dummy.dma_addr >> shift); > >> } > >> > >> /* Set plane offsets */ > >> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h > >> index 96657e55a5b0..0b834579d08c 100644 > >> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h > >> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h > >> @@ -113,6 +113,7 @@ enum rkisp1_feature { > >> RKISP1_FEATURE_DUAL_CROP = BIT(1), > >> RKISP1_FEATURE_RSZ_CROP = BIT(2), > >> RKISP1_FEATURE_MAIN_STRIDE = BIT(3), > >> + RKISP1_FEATURE_DMA_34BIT = BIT(4), > > doc this field Looks like only RKISP1_FEATURE_MIPI_CSI2 is documented. We'll fix that in v3. > >> }; > >> > >> /* > >> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c > >> index d68a805e8b6b..4c77aa2bc50a 100644 > >> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c > >> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c > >> @@ -506,7 +506,8 @@ static const struct rkisp1_info imx8mp_isp_info = { > >> .isr_size = ARRAY_SIZE(imx8mp_isp_isrs), > >> .isp_ver = IMX8MP_V10, > >> .features = RKISP1_FEATURE_RSZ_CROP > >> - | RKISP1_FEATURE_MAIN_STRIDE, > >> + | RKISP1_FEATURE_MAIN_STRIDE > >> + | RKISP1_FEATURE_DMA_34BIT, > >> }; > >> > >> static const struct of_device_id rkisp1_of_match[] = {
diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c index 35cec263c563..234b1f8488cb 100644 --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c @@ -624,6 +624,9 @@ static void rkisp1_dummy_buf_destroy(struct rkisp1_capture *cap) static void rkisp1_set_next_buf(struct rkisp1_capture *cap) { + u8 shift = cap->rkisp1->info->features & RKISP1_FEATURE_DMA_34BIT ? + 2 : 0; + cap->buf.curr = cap->buf.next; cap->buf.next = NULL; @@ -636,7 +639,7 @@ static void rkisp1_set_next_buf(struct rkisp1_capture *cap) buff_addr = cap->buf.next->buff_addr; rkisp1_write(cap->rkisp1, cap->config->mi.y_base_ad_init, - buff_addr[RKISP1_PLANE_Y]); + buff_addr[RKISP1_PLANE_Y] >> shift); /* * In order to support grey format we capture * YUV422 planar format from the camera and @@ -645,17 +648,17 @@ static void rkisp1_set_next_buf(struct rkisp1_capture *cap) if (cap->pix.cfg->fourcc == V4L2_PIX_FMT_GREY) { rkisp1_write(cap->rkisp1, cap->config->mi.cb_base_ad_init, - cap->buf.dummy.dma_addr); + cap->buf.dummy.dma_addr >> shift); rkisp1_write(cap->rkisp1, cap->config->mi.cr_base_ad_init, - cap->buf.dummy.dma_addr); + cap->buf.dummy.dma_addr >> shift); } else { rkisp1_write(cap->rkisp1, cap->config->mi.cb_base_ad_init, - buff_addr[RKISP1_PLANE_CB]); + buff_addr[RKISP1_PLANE_CB] >> shift); rkisp1_write(cap->rkisp1, cap->config->mi.cr_base_ad_init, - buff_addr[RKISP1_PLANE_CR]); + buff_addr[RKISP1_PLANE_CR] >> shift); } } else { /* @@ -663,11 +666,11 @@ static void rkisp1_set_next_buf(struct rkisp1_capture *cap) * throw data if there is no available buffer. */ rkisp1_write(cap->rkisp1, cap->config->mi.y_base_ad_init, - cap->buf.dummy.dma_addr); + cap->buf.dummy.dma_addr >> shift); rkisp1_write(cap->rkisp1, cap->config->mi.cb_base_ad_init, - cap->buf.dummy.dma_addr); + cap->buf.dummy.dma_addr >> shift); rkisp1_write(cap->rkisp1, cap->config->mi.cr_base_ad_init, - cap->buf.dummy.dma_addr); + cap->buf.dummy.dma_addr >> shift); } /* Set plane offsets */ diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h index 96657e55a5b0..0b834579d08c 100644 --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h @@ -113,6 +113,7 @@ enum rkisp1_feature { RKISP1_FEATURE_DUAL_CROP = BIT(1), RKISP1_FEATURE_RSZ_CROP = BIT(2), RKISP1_FEATURE_MAIN_STRIDE = BIT(3), + RKISP1_FEATURE_DMA_34BIT = BIT(4), }; /* diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c index d68a805e8b6b..4c77aa2bc50a 100644 --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c @@ -506,7 +506,8 @@ static const struct rkisp1_info imx8mp_isp_info = { .isr_size = ARRAY_SIZE(imx8mp_isp_isrs), .isp_ver = IMX8MP_V10, .features = RKISP1_FEATURE_RSZ_CROP - | RKISP1_FEATURE_MAIN_STRIDE, + | RKISP1_FEATURE_MAIN_STRIDE + | RKISP1_FEATURE_DMA_34BIT, }; static const struct of_device_id rkisp1_of_match[] = {
On the ISP that is integrated in the i.MX8MP, the DMA base addresses are encoded in 34-bit. Shift them to the left by 2 bits so that they can be contained in 32 bits. Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> --- .../platform/rockchip/rkisp1/rkisp1-capture.c | 19 +++++++++++-------- .../platform/rockchip/rkisp1/rkisp1-common.h | 1 + .../platform/rockchip/rkisp1/rkisp1-dev.c | 3 ++- 3 files changed, 14 insertions(+), 9 deletions(-)