* [PATCH v1 0/2] media: camss: Fixups for large capture frames @ 2024-08-02 15:24 Jordan Crouse 2024-08-02 15:24 ` [PATCH v1 1/2] media: camss: Increase the maximum frame size Jordan Crouse 2024-08-02 15:24 ` [PATCH v1 2/2] media: camss: Avoid overwriting vfe clock rates for 8250 Jordan Crouse 0 siblings, 2 replies; 9+ messages in thread From: Jordan Crouse @ 2024-08-02 15:24 UTC (permalink / raw) To: linux-media Cc: Bryan O'Donoghue, Mauro Carvalho Chehab, Robert Foss, Todor Tomov, linux-arm-msm, linux-kernel A few small issues discovered while (thus far unsuccessfully) trying to bring up a 64MP sensor. The chosen frame limitation of 8192 seems to be somewhat arbitrary as there don't appear to be any hardware limits on maximum frame size. Double the maximum allowable frame size to accommodate bigger sensors. Next the larger data sizes end up needing bigger pixel clocks. This exposed a bug for 8250 devices where the VFE clocks are shared between two blocks, but the CSID block is being initialized second and overwriting the carefully selected clock rates from VFE. This was likely not a problem earlier because the lowest VFE clock rate that CSID was selecting was good enough for the family of sensors that were being used. Jordan Crouse (2): media: camss: Increase the maximum frame size media: camss: Avoid overwriting vfe clock rates for 8250 .../media/platform/qcom/camss/camss-csid.c | 8 +++---- .../media/platform/qcom/camss/camss-csiphy.c | 4 ++-- .../media/platform/qcom/camss/camss-ispif.c | 4 ++-- drivers/media/platform/qcom/camss/camss-vfe.c | 4 ++-- .../media/platform/qcom/camss/camss-video.c | 6 +++--- drivers/media/platform/qcom/camss/camss.c | 21 +++++++++++++------ 6 files changed, 28 insertions(+), 19 deletions(-) -- 2.40.1 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v1 1/2] media: camss: Increase the maximum frame size 2024-08-02 15:24 [PATCH v1 0/2] media: camss: Fixups for large capture frames Jordan Crouse @ 2024-08-02 15:24 ` Jordan Crouse 2024-08-06 23:09 ` Bryan O'Donoghue 2024-12-06 14:51 ` Luca Weiss 2024-08-02 15:24 ` [PATCH v1 2/2] media: camss: Avoid overwriting vfe clock rates for 8250 Jordan Crouse 1 sibling, 2 replies; 9+ messages in thread From: Jordan Crouse @ 2024-08-02 15:24 UTC (permalink / raw) To: linux-media Cc: Bryan O'Donoghue, Mauro Carvalho Chehab, Robert Foss, Todor Tomov, linux-arm-msm, linux-kernel Commit 35493d653a2d ("media: camss: add support for vidioc_enum_framesizes ioctl") added a maximum frame width and height but the values selected seemed to have been arbitrary. In reality the cam hardware doesn't seem to have a maximum size restriction so double up the maximum reported width and height to allow for larger frames. Also increase the maximum size checks at each point in the pipeline so the increased sizes are allowed all the way down to the sensor. Signed-off-by: Jordan Crouse <jorcrous@amazon.com> --- drivers/media/platform/qcom/camss/camss-csid.c | 8 ++++---- drivers/media/platform/qcom/camss/camss-csiphy.c | 4 ++-- drivers/media/platform/qcom/camss/camss-ispif.c | 4 ++-- drivers/media/platform/qcom/camss/camss-vfe.c | 4 ++-- drivers/media/platform/qcom/camss/camss-video.c | 6 +++--- 5 files changed, 13 insertions(+), 13 deletions(-) diff --git a/drivers/media/platform/qcom/camss/camss-csid.c b/drivers/media/platform/qcom/camss/camss-csid.c index 858db5d4ca75..886c42c82612 100644 --- a/drivers/media/platform/qcom/camss/camss-csid.c +++ b/drivers/media/platform/qcom/camss/camss-csid.c @@ -752,8 +752,8 @@ static void csid_try_format(struct csid_device *csid, if (i >= csid->res->formats->nformats) fmt->code = MEDIA_BUS_FMT_UYVY8_1X16; - fmt->width = clamp_t(u32, fmt->width, 1, 8191); - fmt->height = clamp_t(u32, fmt->height, 1, 8191); + fmt->width = clamp_t(u32, fmt->width, 1, 16383); + fmt->height = clamp_t(u32, fmt->height, 1, 16383); fmt->field = V4L2_FIELD_NONE; fmt->colorspace = V4L2_COLORSPACE_SRGB; @@ -781,8 +781,8 @@ static void csid_try_format(struct csid_device *csid, if (i >= csid->res->formats->nformats) fmt->code = MEDIA_BUS_FMT_UYVY8_1X16; - fmt->width = clamp_t(u32, fmt->width, 1, 8191); - fmt->height = clamp_t(u32, fmt->height, 1, 8191); + fmt->width = clamp_t(u32, fmt->width, 1, 16383); + fmt->height = clamp_t(u32, fmt->height, 1, 16383); fmt->field = V4L2_FIELD_NONE; } diff --git a/drivers/media/platform/qcom/camss/camss-csiphy.c b/drivers/media/platform/qcom/camss/camss-csiphy.c index 2f7361dfd461..43c35ad6ac84 100644 --- a/drivers/media/platform/qcom/camss/camss-csiphy.c +++ b/drivers/media/platform/qcom/camss/camss-csiphy.c @@ -368,8 +368,8 @@ static void csiphy_try_format(struct csiphy_device *csiphy, if (i >= csiphy->res->formats->nformats) fmt->code = MEDIA_BUS_FMT_UYVY8_1X16; - fmt->width = clamp_t(u32, fmt->width, 1, 8191); - fmt->height = clamp_t(u32, fmt->height, 1, 8191); + fmt->width = clamp_t(u32, fmt->width, 1, 16383); + fmt->height = clamp_t(u32, fmt->height, 1, 16383); fmt->field = V4L2_FIELD_NONE; fmt->colorspace = V4L2_COLORSPACE_SRGB; diff --git a/drivers/media/platform/qcom/camss/camss-ispif.c b/drivers/media/platform/qcom/camss/camss-ispif.c index a12dcc7ff438..01e2ded8da0b 100644 --- a/drivers/media/platform/qcom/camss/camss-ispif.c +++ b/drivers/media/platform/qcom/camss/camss-ispif.c @@ -912,8 +912,8 @@ static void ispif_try_format(struct ispif_line *line, if (i >= line->nformats) fmt->code = MEDIA_BUS_FMT_UYVY8_1X16; - fmt->width = clamp_t(u32, fmt->width, 1, 8191); - fmt->height = clamp_t(u32, fmt->height, 1, 8191); + fmt->width = clamp_t(u32, fmt->width, 1, 16383); + fmt->height = clamp_t(u32, fmt->height, 1, 16383); fmt->field = V4L2_FIELD_NONE; fmt->colorspace = V4L2_COLORSPACE_SRGB; diff --git a/drivers/media/platform/qcom/camss/camss-vfe.c b/drivers/media/platform/qcom/camss/camss-vfe.c index 83c5a36d071f..826c0fb31785 100644 --- a/drivers/media/platform/qcom/camss/camss-vfe.c +++ b/drivers/media/platform/qcom/camss/camss-vfe.c @@ -1049,8 +1049,8 @@ static void vfe_try_format(struct vfe_line *line, if (i >= line->nformats) fmt->code = MEDIA_BUS_FMT_UYVY8_1X16; - fmt->width = clamp_t(u32, fmt->width, 1, 8191); - fmt->height = clamp_t(u32, fmt->height, 1, 8191); + fmt->width = clamp_t(u32, fmt->width, 1, 16383); + fmt->height = clamp_t(u32, fmt->height, 1, 16383); fmt->field = V4L2_FIELD_NONE; fmt->colorspace = V4L2_COLORSPACE_SRGB; diff --git a/drivers/media/platform/qcom/camss/camss-video.c b/drivers/media/platform/qcom/camss/camss-video.c index cd72feca618c..5fee3733da8e 100644 --- a/drivers/media/platform/qcom/camss/camss-video.c +++ b/drivers/media/platform/qcom/camss/camss-video.c @@ -19,10 +19,10 @@ #include "camss.h" #define CAMSS_FRAME_MIN_WIDTH 1 -#define CAMSS_FRAME_MAX_WIDTH 8191 +#define CAMSS_FRAME_MAX_WIDTH 16833 #define CAMSS_FRAME_MIN_HEIGHT 1 -#define CAMSS_FRAME_MAX_HEIGHT_RDI 8191 -#define CAMSS_FRAME_MAX_HEIGHT_PIX 4096 +#define CAMSS_FRAME_MAX_HEIGHT_RDI 16833 +#define CAMSS_FRAME_MAX_HEIGHT_PIX 8192 /* ----------------------------------------------------------------------------- * Helper functions -- 2.40.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v1 1/2] media: camss: Increase the maximum frame size 2024-08-02 15:24 ` [PATCH v1 1/2] media: camss: Increase the maximum frame size Jordan Crouse @ 2024-08-06 23:09 ` Bryan O'Donoghue 2024-08-07 8:16 ` Bryan O'Donoghue 2024-12-06 14:51 ` Luca Weiss 1 sibling, 1 reply; 9+ messages in thread From: Bryan O'Donoghue @ 2024-08-06 23:09 UTC (permalink / raw) To: Jordan Crouse, linux-media Cc: Mauro Carvalho Chehab, Robert Foss, Todor Tomov, linux-arm-msm, linux-kernel On 02/08/2024 16:24, Jordan Crouse wrote: > Commit 35493d653a2d > ("media: camss: add support for vidioc_enum_framesizes ioctl") added a > maximum frame width and height but the values selected seemed to have > been arbitrary. In reality the cam hardware doesn't seem to have a maximum > size restriction so double up the maximum reported width and height to > allow for larger frames. > > Also increase the maximum size checks at each point in the pipeline so > the increased sizes are allowed all the way down to the sensor. So, I think this should be a Fixes: also. > > Signed-off-by: Jordan Crouse <jorcrous@amazon.com> > --- > > drivers/media/platform/qcom/camss/camss-csid.c | 8 ++++---- > drivers/media/platform/qcom/camss/camss-csiphy.c | 4 ++-- > drivers/media/platform/qcom/camss/camss-ispif.c | 4 ++-- > drivers/media/platform/qcom/camss/camss-vfe.c | 4 ++-- > drivers/media/platform/qcom/camss/camss-video.c | 6 +++--- > 5 files changed, 13 insertions(+), 13 deletions(-) > > diff --git a/drivers/media/platform/qcom/camss/camss-csid.c b/drivers/media/platform/qcom/camss/camss-csid.c > index 858db5d4ca75..886c42c82612 100644 > --- a/drivers/media/platform/qcom/camss/camss-csid.c > +++ b/drivers/media/platform/qcom/camss/camss-csid.c > @@ -752,8 +752,8 @@ static void csid_try_format(struct csid_device *csid, > if (i >= csid->res->formats->nformats) > fmt->code = MEDIA_BUS_FMT_UYVY8_1X16; > > - fmt->width = clamp_t(u32, fmt->width, 1, 8191); > - fmt->height = clamp_t(u32, fmt->height, 1, 8191); > + fmt->width = clamp_t(u32, fmt->width, 1, 16383); > + fmt->height = clamp_t(u32, fmt->height, 1, 16383); Feels like we should have a define instead of hard coded values repeated constantly. --- bod ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v1 1/2] media: camss: Increase the maximum frame size 2024-08-06 23:09 ` Bryan O'Donoghue @ 2024-08-07 8:16 ` Bryan O'Donoghue 0 siblings, 0 replies; 9+ messages in thread From: Bryan O'Donoghue @ 2024-08-07 8:16 UTC (permalink / raw) To: Jordan Crouse, linux-media Cc: Mauro Carvalho Chehab, Robert Foss, Todor Tomov, linux-arm-msm, linux-kernel On 07/08/2024 00:09, Bryan O'Donoghue wrote: >> >> Also increase the maximum size checks at each point in the pipeline so >> the increased sizes are allowed all the way down to the sensor. > > So, I think this should be a Fixes: also. TL;DR we don't need a Fixes: for your v2. Pardon me, not a Fixes: the sizeof(sensor) that can be supported is SoC specific - camera IP specific really - so 8192 is a blunt descriptor, raising to 16384 makes some kind of sense. A long term fix for this would be for each camss descriptor to declare the maximum sizeof(sensor) it supports. For sm8250, sc8280xp that's 64 megapixels - for older SoCs its going to be smaller. --- bod ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v1 1/2] media: camss: Increase the maximum frame size 2024-08-02 15:24 ` [PATCH v1 1/2] media: camss: Increase the maximum frame size Jordan Crouse 2024-08-06 23:09 ` Bryan O'Donoghue @ 2024-12-06 14:51 ` Luca Weiss 2025-04-07 15:59 ` Bryan O'Donoghue 1 sibling, 1 reply; 9+ messages in thread From: Luca Weiss @ 2024-12-06 14:51 UTC (permalink / raw) To: Jordan Crouse, linux-media Cc: Bryan O'Donoghue, Mauro Carvalho Chehab, Robert Foss, Todor Tomov, linux-arm-msm, linux-kernel On Fri Aug 2, 2024 at 5:24 PM CEST, Jordan Crouse wrote: > Commit 35493d653a2d > ("media: camss: add support for vidioc_enum_framesizes ioctl") added a > maximum frame width and height but the values selected seemed to have > been arbitrary. In reality the cam hardware doesn't seem to have a maximum > size restriction so double up the maximum reported width and height to > allow for larger frames. > > Also increase the maximum size checks at each point in the pipeline so > the increased sizes are allowed all the way down to the sensor. Hi Jordan, Looks like this hasn't landed yet, do you plan on resending this? Just wanted to try a 8192x6144 format but csid limiting the size to 8191 is a bit in the way. Regards Luca > > Signed-off-by: Jordan Crouse <jorcrous@amazon.com> > --- > > drivers/media/platform/qcom/camss/camss-csid.c | 8 ++++---- > drivers/media/platform/qcom/camss/camss-csiphy.c | 4 ++-- > drivers/media/platform/qcom/camss/camss-ispif.c | 4 ++-- > drivers/media/platform/qcom/camss/camss-vfe.c | 4 ++-- > drivers/media/platform/qcom/camss/camss-video.c | 6 +++--- > 5 files changed, 13 insertions(+), 13 deletions(-) > > diff --git a/drivers/media/platform/qcom/camss/camss-csid.c b/drivers/media/platform/qcom/camss/camss-csid.c > index 858db5d4ca75..886c42c82612 100644 > --- a/drivers/media/platform/qcom/camss/camss-csid.c > +++ b/drivers/media/platform/qcom/camss/camss-csid.c > @@ -752,8 +752,8 @@ static void csid_try_format(struct csid_device *csid, > if (i >= csid->res->formats->nformats) > fmt->code = MEDIA_BUS_FMT_UYVY8_1X16; > > - fmt->width = clamp_t(u32, fmt->width, 1, 8191); > - fmt->height = clamp_t(u32, fmt->height, 1, 8191); > + fmt->width = clamp_t(u32, fmt->width, 1, 16383); > + fmt->height = clamp_t(u32, fmt->height, 1, 16383); > > fmt->field = V4L2_FIELD_NONE; > fmt->colorspace = V4L2_COLORSPACE_SRGB; > @@ -781,8 +781,8 @@ static void csid_try_format(struct csid_device *csid, > if (i >= csid->res->formats->nformats) > fmt->code = MEDIA_BUS_FMT_UYVY8_1X16; > > - fmt->width = clamp_t(u32, fmt->width, 1, 8191); > - fmt->height = clamp_t(u32, fmt->height, 1, 8191); > + fmt->width = clamp_t(u32, fmt->width, 1, 16383); > + fmt->height = clamp_t(u32, fmt->height, 1, 16383); > > fmt->field = V4L2_FIELD_NONE; > } > diff --git a/drivers/media/platform/qcom/camss/camss-csiphy.c b/drivers/media/platform/qcom/camss/camss-csiphy.c > index 2f7361dfd461..43c35ad6ac84 100644 > --- a/drivers/media/platform/qcom/camss/camss-csiphy.c > +++ b/drivers/media/platform/qcom/camss/camss-csiphy.c > @@ -368,8 +368,8 @@ static void csiphy_try_format(struct csiphy_device *csiphy, > if (i >= csiphy->res->formats->nformats) > fmt->code = MEDIA_BUS_FMT_UYVY8_1X16; > > - fmt->width = clamp_t(u32, fmt->width, 1, 8191); > - fmt->height = clamp_t(u32, fmt->height, 1, 8191); > + fmt->width = clamp_t(u32, fmt->width, 1, 16383); > + fmt->height = clamp_t(u32, fmt->height, 1, 16383); > > fmt->field = V4L2_FIELD_NONE; > fmt->colorspace = V4L2_COLORSPACE_SRGB; > diff --git a/drivers/media/platform/qcom/camss/camss-ispif.c b/drivers/media/platform/qcom/camss/camss-ispif.c > index a12dcc7ff438..01e2ded8da0b 100644 > --- a/drivers/media/platform/qcom/camss/camss-ispif.c > +++ b/drivers/media/platform/qcom/camss/camss-ispif.c > @@ -912,8 +912,8 @@ static void ispif_try_format(struct ispif_line *line, > if (i >= line->nformats) > fmt->code = MEDIA_BUS_FMT_UYVY8_1X16; > > - fmt->width = clamp_t(u32, fmt->width, 1, 8191); > - fmt->height = clamp_t(u32, fmt->height, 1, 8191); > + fmt->width = clamp_t(u32, fmt->width, 1, 16383); > + fmt->height = clamp_t(u32, fmt->height, 1, 16383); > > fmt->field = V4L2_FIELD_NONE; > fmt->colorspace = V4L2_COLORSPACE_SRGB; > diff --git a/drivers/media/platform/qcom/camss/camss-vfe.c b/drivers/media/platform/qcom/camss/camss-vfe.c > index 83c5a36d071f..826c0fb31785 100644 > --- a/drivers/media/platform/qcom/camss/camss-vfe.c > +++ b/drivers/media/platform/qcom/camss/camss-vfe.c > @@ -1049,8 +1049,8 @@ static void vfe_try_format(struct vfe_line *line, > if (i >= line->nformats) > fmt->code = MEDIA_BUS_FMT_UYVY8_1X16; > > - fmt->width = clamp_t(u32, fmt->width, 1, 8191); > - fmt->height = clamp_t(u32, fmt->height, 1, 8191); > + fmt->width = clamp_t(u32, fmt->width, 1, 16383); > + fmt->height = clamp_t(u32, fmt->height, 1, 16383); > > fmt->field = V4L2_FIELD_NONE; > fmt->colorspace = V4L2_COLORSPACE_SRGB; > diff --git a/drivers/media/platform/qcom/camss/camss-video.c b/drivers/media/platform/qcom/camss/camss-video.c > index cd72feca618c..5fee3733da8e 100644 > --- a/drivers/media/platform/qcom/camss/camss-video.c > +++ b/drivers/media/platform/qcom/camss/camss-video.c > @@ -19,10 +19,10 @@ > #include "camss.h" > > #define CAMSS_FRAME_MIN_WIDTH 1 > -#define CAMSS_FRAME_MAX_WIDTH 8191 > +#define CAMSS_FRAME_MAX_WIDTH 16833 > #define CAMSS_FRAME_MIN_HEIGHT 1 > -#define CAMSS_FRAME_MAX_HEIGHT_RDI 8191 > -#define CAMSS_FRAME_MAX_HEIGHT_PIX 4096 > +#define CAMSS_FRAME_MAX_HEIGHT_RDI 16833 > +#define CAMSS_FRAME_MAX_HEIGHT_PIX 8192 > > /* ----------------------------------------------------------------------------- > * Helper functions ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v1 1/2] media: camss: Increase the maximum frame size 2024-12-06 14:51 ` Luca Weiss @ 2025-04-07 15:59 ` Bryan O'Donoghue 0 siblings, 0 replies; 9+ messages in thread From: Bryan O'Donoghue @ 2025-04-07 15:59 UTC (permalink / raw) To: Luca Weiss, Jordan Crouse, linux-media Cc: Mauro Carvalho Chehab, Robert Foss, Todor Tomov, linux-arm-msm, linux-kernel On 06/12/2024 14:51, Luca Weiss wrote: > On Fri Aug 2, 2024 at 5:24 PM CEST, Jordan Crouse wrote: >> Commit 35493d653a2d >> ("media: camss: add support for vidioc_enum_framesizes ioctl") added a >> maximum frame width and height but the values selected seemed to have >> been arbitrary. In reality the cam hardware doesn't seem to have a maximum >> size restriction so double up the maximum reported width and height to >> allow for larger frames. >> >> Also increase the maximum size checks at each point in the pipeline so >> the increased sizes are allowed all the way down to the sensor. > > Hi Jordan, > > Looks like this hasn't landed yet, do you plan on resending this? > > Just wanted to try a 8192x6144 format but csid limiting the size to 8191 > is a bit in the way. > > Regards > Luca > >> >> Signed-off-by: Jordan Crouse <jorcrous@amazon.com> >> --- >> >> drivers/media/platform/qcom/camss/camss-csid.c | 8 ++++---- >> drivers/media/platform/qcom/camss/camss-csiphy.c | 4 ++-- >> drivers/media/platform/qcom/camss/camss-ispif.c | 4 ++-- >> drivers/media/platform/qcom/camss/camss-vfe.c | 4 ++-- >> drivers/media/platform/qcom/camss/camss-video.c | 6 +++--- >> 5 files changed, 13 insertions(+), 13 deletions(-) >> >> diff --git a/drivers/media/platform/qcom/camss/camss-csid.c b/drivers/media/platform/qcom/camss/camss-csid.c >> index 858db5d4ca75..886c42c82612 100644 >> --- a/drivers/media/platform/qcom/camss/camss-csid.c >> +++ b/drivers/media/platform/qcom/camss/camss-csid.c >> @@ -752,8 +752,8 @@ static void csid_try_format(struct csid_device *csid, >> if (i >= csid->res->formats->nformats) >> fmt->code = MEDIA_BUS_FMT_UYVY8_1X16; >> >> - fmt->width = clamp_t(u32, fmt->width, 1, 8191); >> - fmt->height = clamp_t(u32, fmt->height, 1, 8191); >> + fmt->width = clamp_t(u32, fmt->width, 1, 16383); >> + fmt->height = clamp_t(u32, fmt->height, 1, 16383); >> >> fmt->field = V4L2_FIELD_NONE; >> fmt->colorspace = V4L2_COLORSPACE_SRGB; >> @@ -781,8 +781,8 @@ static void csid_try_format(struct csid_device *csid, >> if (i >= csid->res->formats->nformats) >> fmt->code = MEDIA_BUS_FMT_UYVY8_1X16; >> >> - fmt->width = clamp_t(u32, fmt->width, 1, 8191); >> - fmt->height = clamp_t(u32, fmt->height, 1, 8191); >> + fmt->width = clamp_t(u32, fmt->width, 1, 16383); >> + fmt->height = clamp_t(u32, fmt->height, 1, 16383); >> >> fmt->field = V4L2_FIELD_NONE; >> } >> diff --git a/drivers/media/platform/qcom/camss/camss-csiphy.c b/drivers/media/platform/qcom/camss/camss-csiphy.c >> index 2f7361dfd461..43c35ad6ac84 100644 >> --- a/drivers/media/platform/qcom/camss/camss-csiphy.c >> +++ b/drivers/media/platform/qcom/camss/camss-csiphy.c >> @@ -368,8 +368,8 @@ static void csiphy_try_format(struct csiphy_device *csiphy, >> if (i >= csiphy->res->formats->nformats) >> fmt->code = MEDIA_BUS_FMT_UYVY8_1X16; >> >> - fmt->width = clamp_t(u32, fmt->width, 1, 8191); >> - fmt->height = clamp_t(u32, fmt->height, 1, 8191); >> + fmt->width = clamp_t(u32, fmt->width, 1, 16383); >> + fmt->height = clamp_t(u32, fmt->height, 1, 16383); >> >> fmt->field = V4L2_FIELD_NONE; >> fmt->colorspace = V4L2_COLORSPACE_SRGB; >> diff --git a/drivers/media/platform/qcom/camss/camss-ispif.c b/drivers/media/platform/qcom/camss/camss-ispif.c >> index a12dcc7ff438..01e2ded8da0b 100644 >> --- a/drivers/media/platform/qcom/camss/camss-ispif.c >> +++ b/drivers/media/platform/qcom/camss/camss-ispif.c >> @@ -912,8 +912,8 @@ static void ispif_try_format(struct ispif_line *line, >> if (i >= line->nformats) >> fmt->code = MEDIA_BUS_FMT_UYVY8_1X16; >> >> - fmt->width = clamp_t(u32, fmt->width, 1, 8191); >> - fmt->height = clamp_t(u32, fmt->height, 1, 8191); >> + fmt->width = clamp_t(u32, fmt->width, 1, 16383); >> + fmt->height = clamp_t(u32, fmt->height, 1, 16383); >> >> fmt->field = V4L2_FIELD_NONE; >> fmt->colorspace = V4L2_COLORSPACE_SRGB; >> diff --git a/drivers/media/platform/qcom/camss/camss-vfe.c b/drivers/media/platform/qcom/camss/camss-vfe.c >> index 83c5a36d071f..826c0fb31785 100644 >> --- a/drivers/media/platform/qcom/camss/camss-vfe.c >> +++ b/drivers/media/platform/qcom/camss/camss-vfe.c >> @@ -1049,8 +1049,8 @@ static void vfe_try_format(struct vfe_line *line, >> if (i >= line->nformats) >> fmt->code = MEDIA_BUS_FMT_UYVY8_1X16; >> >> - fmt->width = clamp_t(u32, fmt->width, 1, 8191); >> - fmt->height = clamp_t(u32, fmt->height, 1, 8191); >> + fmt->width = clamp_t(u32, fmt->width, 1, 16383); >> + fmt->height = clamp_t(u32, fmt->height, 1, 16383); >> >> fmt->field = V4L2_FIELD_NONE; >> fmt->colorspace = V4L2_COLORSPACE_SRGB; >> diff --git a/drivers/media/platform/qcom/camss/camss-video.c b/drivers/media/platform/qcom/camss/camss-video.c >> index cd72feca618c..5fee3733da8e 100644 >> --- a/drivers/media/platform/qcom/camss/camss-video.c >> +++ b/drivers/media/platform/qcom/camss/camss-video.c >> @@ -19,10 +19,10 @@ >> #include "camss.h" >> >> #define CAMSS_FRAME_MIN_WIDTH 1 >> -#define CAMSS_FRAME_MAX_WIDTH 8191 >> +#define CAMSS_FRAME_MAX_WIDTH 16833 >> #define CAMSS_FRAME_MIN_HEIGHT 1 >> -#define CAMSS_FRAME_MAX_HEIGHT_RDI 8191 >> -#define CAMSS_FRAME_MAX_HEIGHT_PIX 4096 >> +#define CAMSS_FRAME_MAX_HEIGHT_RDI 16833 >> +#define CAMSS_FRAME_MAX_HEIGHT_PIX 8192 >> >> /* ----------------------------------------------------------------------------- >> * Helper functions > Ping Jordan ! Will buy you beer for a v2 of this series. --- bod ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v1 2/2] media: camss: Avoid overwriting vfe clock rates for 8250 2024-08-02 15:24 [PATCH v1 0/2] media: camss: Fixups for large capture frames Jordan Crouse 2024-08-02 15:24 ` [PATCH v1 1/2] media: camss: Increase the maximum frame size Jordan Crouse @ 2024-08-02 15:24 ` Jordan Crouse 2024-08-02 22:20 ` Bryan O'Donoghue 1 sibling, 1 reply; 9+ messages in thread From: Jordan Crouse @ 2024-08-02 15:24 UTC (permalink / raw) To: linux-media Cc: Bryan O'Donoghue, Mauro Carvalho Chehab, Robert Foss, Todor Tomov, linux-arm-msm, linux-kernel On sm8250 targets both the csid and vfe subsystems share a number of clocks. Commit b4436a18eedb ("media: camss: add support for SM8250 camss") reorganized the initialization sequence so that VFE gets initialized first but a side effect of that was that the CSID subsystem came in after and overwrites the set frequencies on the shared clocks. Empty the frequency tables for the shared clocks in the CSID resources so they won't overwrite the clock rates that the VFE has already set. Signed-off-by: Jordan Crouse <jorcrous@amazon.com> --- drivers/media/platform/qcom/camss/camss.c | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c index 51b1d3550421..d78644c3ebe9 100644 --- a/drivers/media/platform/qcom/camss/camss.c +++ b/drivers/media/platform/qcom/camss/camss.c @@ -915,6 +915,15 @@ static const struct camss_subdev_resources csiphy_res_8250[] = { } }; +/* + * Both CSID and VFE use some of the same vfe clocks and both + * should prepare/enable them but only the VFE subsystem should be in charge + * of setting the clock rates. + * + * Set the frequency tables for those clocks in the CSID resources to + * be empty so the csid subsystem doesn't overwrite the clock rates that the + * VFE already set. + */ static const struct camss_subdev_resources csid_res_8250[] = { /* CSID0 */ { @@ -922,8 +931,8 @@ static const struct camss_subdev_resources csid_res_8250[] = { .clock = { "vfe0_csid", "vfe0_cphy_rx", "vfe0", "vfe0_areg", "vfe0_ahb" }, .clock_rate = { { 400000000 }, { 400000000 }, - { 350000000, 475000000, 576000000, 720000000 }, - { 100000000, 200000000, 300000000, 400000000 }, + { 0 }, + { 0 }, { 0 } }, .reg = { "csid0" }, .interrupt = { "csid0" }, @@ -939,8 +948,8 @@ static const struct camss_subdev_resources csid_res_8250[] = { .clock = { "vfe1_csid", "vfe1_cphy_rx", "vfe1", "vfe1_areg", "vfe1_ahb" }, .clock_rate = { { 400000000 }, { 400000000 }, - { 350000000, 475000000, 576000000, 720000000 }, - { 100000000, 200000000, 300000000, 400000000 }, + { 0 }, + { 0 }, { 0 } }, .reg = { "csid1" }, .interrupt = { "csid1" }, @@ -956,7 +965,7 @@ static const struct camss_subdev_resources csid_res_8250[] = { .clock = { "vfe_lite_csid", "vfe_lite_cphy_rx", "vfe_lite", "vfe_lite_ahb" }, .clock_rate = { { 400000000 }, { 400000000 }, - { 400000000, 480000000 }, + { 0 }, { 0 } }, .reg = { "csid2" }, .interrupt = { "csid2" }, @@ -973,7 +982,7 @@ static const struct camss_subdev_resources csid_res_8250[] = { .clock = { "vfe_lite_csid", "vfe_lite_cphy_rx", "vfe_lite", "vfe_lite_ahb" }, .clock_rate = { { 400000000 }, { 400000000 }, - { 400000000, 480000000 }, + { 0 }, { 0 } }, .reg = { "csid3" }, .interrupt = { "csid3" }, -- 2.40.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v1 2/2] media: camss: Avoid overwriting vfe clock rates for 8250 2024-08-02 15:24 ` [PATCH v1 2/2] media: camss: Avoid overwriting vfe clock rates for 8250 Jordan Crouse @ 2024-08-02 22:20 ` Bryan O'Donoghue 2024-08-06 22:38 ` Jordan Crouse 0 siblings, 1 reply; 9+ messages in thread From: Bryan O'Donoghue @ 2024-08-02 22:20 UTC (permalink / raw) To: Jordan Crouse, linux-media Cc: Mauro Carvalho Chehab, Robert Foss, Todor Tomov, linux-arm-msm, linux-kernel On 02/08/2024 16:24, Jordan Crouse wrote: > On sm8250 targets both the csid and vfe subsystems share a number of > clocks. Commit b4436a18eedb ("media: camss: add support for SM8250 camss") > reorganized the initialization sequence so that VFE gets initialized first > but a side effect of that was that the CSID subsystem came in after and > overwrites the set frequencies on the shared clocks. > > Empty the frequency tables for the shared clocks in the CSID resources so > they won't overwrite the clock rates that the VFE has already set. > > Signed-off-by: Jordan Crouse <jorcrous@amazon.com> > --- > > drivers/media/platform/qcom/camss/camss.c | 21 +++++++++++++++------ > 1 file changed, 15 insertions(+), 6 deletions(-) > > diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c > index 51b1d3550421..d78644c3ebe9 100644 > --- a/drivers/media/platform/qcom/camss/camss.c > +++ b/drivers/media/platform/qcom/camss/camss.c > @@ -915,6 +915,15 @@ static const struct camss_subdev_resources csiphy_res_8250[] = { > } > }; > > +/* > + * Both CSID and VFE use some of the same vfe clocks and both > + * should prepare/enable them but only the VFE subsystem should be in charge > + * of setting the clock rates. > + * > + * Set the frequency tables for those clocks in the CSID resources to > + * be empty so the csid subsystem doesn't overwrite the clock rates that the > + * VFE already set. > + */ > static const struct camss_subdev_resources csid_res_8250[] = { > /* CSID0 */ > { > @@ -922,8 +931,8 @@ static const struct camss_subdev_resources csid_res_8250[] = { > .clock = { "vfe0_csid", "vfe0_cphy_rx", "vfe0", "vfe0_areg", "vfe0_ahb" }, > .clock_rate = { { 400000000 }, > { 400000000 }, > - { 350000000, 475000000, 576000000, 720000000 }, > - { 100000000, 200000000, 300000000, 400000000 }, > + { 0 }, > + { 0 }, > { 0 } }, > .reg = { "csid0" }, > .interrupt = { "csid0" }, > @@ -939,8 +948,8 @@ static const struct camss_subdev_resources csid_res_8250[] = { > .clock = { "vfe1_csid", "vfe1_cphy_rx", "vfe1", "vfe1_areg", "vfe1_ahb" }, > .clock_rate = { { 400000000 }, > { 400000000 }, > - { 350000000, 475000000, 576000000, 720000000 }, > - { 100000000, 200000000, 300000000, 400000000 }, > + { 0 }, > + { 0 }, > { 0 } }, > .reg = { "csid1" }, > .interrupt = { "csid1" }, > @@ -956,7 +965,7 @@ static const struct camss_subdev_resources csid_res_8250[] = { > .clock = { "vfe_lite_csid", "vfe_lite_cphy_rx", "vfe_lite", "vfe_lite_ahb" }, > .clock_rate = { { 400000000 }, > { 400000000 }, > - { 400000000, 480000000 }, > + { 0 }, > { 0 } }, > .reg = { "csid2" }, > .interrupt = { "csid2" }, > @@ -973,7 +982,7 @@ static const struct camss_subdev_resources csid_res_8250[] = { > .clock = { "vfe_lite_csid", "vfe_lite_cphy_rx", "vfe_lite", "vfe_lite_ahb" }, > .clock_rate = { { 400000000 }, > { 400000000 }, > - { 400000000, 480000000 }, > + { 0 }, > { 0 } }, > .reg = { "csid3" }, > .interrupt = { "csid3" }, Hi Jordan. Thanks for your patch. Just looking at the clocks you are zeroing here, I think _probably_ these zeroized clocks can be removed from the CSID set entirely. Could you investigate that ? Also please add Fixes: b4436a18eedb ("media: camss: add support for SM8250 camss") and cc stable@vger.kernel.org --- bod ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v1 2/2] media: camss: Avoid overwriting vfe clock rates for 8250 2024-08-02 22:20 ` Bryan O'Donoghue @ 2024-08-06 22:38 ` Jordan Crouse 0 siblings, 0 replies; 9+ messages in thread From: Jordan Crouse @ 2024-08-06 22:38 UTC (permalink / raw) To: Bryan O'Donoghue Cc: linux-media, Mauro Carvalho Chehab, Robert Foss, Todor Tomov, linux-arm-msm, linux-kernel On Fri, Aug 02, 2024 at 11:20:17PM +0100, Bryan O'Donoghue wrote: > > On 02/08/2024 16:24, Jordan Crouse wrote: > >On sm8250 targets both the csid and vfe subsystems share a number of > >clocks. Commit b4436a18eedb ("media: camss: add support for SM8250 camss") > >reorganized the initialization sequence so that VFE gets initialized first > >but a side effect of that was that the CSID subsystem came in after and > >overwrites the set frequencies on the shared clocks. > > > >Empty the frequency tables for the shared clocks in the CSID resources so > >they won't overwrite the clock rates that the VFE has already set. > > > >Signed-off-by: Jordan Crouse <jorcrous@amazon.com> > >--- > > > > drivers/media/platform/qcom/camss/camss.c | 21 +++++++++++++++------ > > 1 file changed, 15 insertions(+), 6 deletions(-) > > > >diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c > >index 51b1d3550421..d78644c3ebe9 100644 > >--- a/drivers/media/platform/qcom/camss/camss.c > >+++ b/drivers/media/platform/qcom/camss/camss.c > >@@ -915,6 +915,15 @@ static const struct camss_subdev_resources csiphy_res_8250[] = { > > } > > }; > > > >+/* > >+ * Both CSID and VFE use some of the same vfe clocks and both > >+ * should prepare/enable them but only the VFE subsystem should be in charge > >+ * of setting the clock rates. > >+ * > >+ * Set the frequency tables for those clocks in the CSID resources to > >+ * be empty so the csid subsystem doesn't overwrite the clock rates that the > >+ * VFE already set. > >+ */ > > static const struct camss_subdev_resources csid_res_8250[] = { > > /* CSID0 */ > > { > >@@ -922,8 +931,8 @@ static const struct camss_subdev_resources csid_res_8250[] = { > > .clock = { "vfe0_csid", "vfe0_cphy_rx", "vfe0", "vfe0_areg", "vfe0_ahb" }, > > .clock_rate = { { 400000000 }, > > { 400000000 }, > >- { 350000000, 475000000, 576000000, 720000000 }, > >- { 100000000, 200000000, 300000000, 400000000 }, > >+ { 0 }, > >+ { 0 }, > > { 0 } }, > > .reg = { "csid0" }, > > .interrupt = { "csid0" }, > >@@ -939,8 +948,8 @@ static const struct camss_subdev_resources csid_res_8250[] = { > > .clock = { "vfe1_csid", "vfe1_cphy_rx", "vfe1", "vfe1_areg", "vfe1_ahb" }, > > .clock_rate = { { 400000000 }, > > { 400000000 }, > >- { 350000000, 475000000, 576000000, 720000000 }, > >- { 100000000, 200000000, 300000000, 400000000 }, > >+ { 0 }, > >+ { 0 }, > > { 0 } }, > > .reg = { "csid1" }, > > .interrupt = { "csid1" }, > >@@ -956,7 +965,7 @@ static const struct camss_subdev_resources csid_res_8250[] = { > > .clock = { "vfe_lite_csid", "vfe_lite_cphy_rx", "vfe_lite", "vfe_lite_ahb" }, > > .clock_rate = { { 400000000 }, > > { 400000000 }, > >- { 400000000, 480000000 }, > >+ { 0 }, > > { 0 } }, > > .reg = { "csid2" }, > > .interrupt = { "csid2" }, > >@@ -973,7 +982,7 @@ static const struct camss_subdev_resources csid_res_8250[] = { > > .clock = { "vfe_lite_csid", "vfe_lite_cphy_rx", "vfe_lite", "vfe_lite_ahb" }, > > .clock_rate = { { 400000000 }, > > { 400000000 }, > >- { 400000000, 480000000 }, > >+ { 0 }, > > { 0 } }, > > .reg = { "csid3" }, > > .interrupt = { "csid3" }, > > Hi Jordan. > > Thanks for your patch. Just looking at the clocks you are zeroing here, > I think _probably_ these zeroized clocks can be removed from the CSID > set entirely. > > Could you investigate that ? I think that will work. It will cement the need for to VFE always run first but we can add documentation warnings about that. It looks like we can do this optimization for 845, sm8250 and sm2850xp. > > Also please add > > Fixes: b4436a18eedb ("media: camss: add support for SM8250 camss") and > cc stable@vger.kernel.org Will do. Jordan ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2025-04-07 15:59 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-08-02 15:24 [PATCH v1 0/2] media: camss: Fixups for large capture frames Jordan Crouse 2024-08-02 15:24 ` [PATCH v1 1/2] media: camss: Increase the maximum frame size Jordan Crouse 2024-08-06 23:09 ` Bryan O'Donoghue 2024-08-07 8:16 ` Bryan O'Donoghue 2024-12-06 14:51 ` Luca Weiss 2025-04-07 15:59 ` Bryan O'Donoghue 2024-08-02 15:24 ` [PATCH v1 2/2] media: camss: Avoid overwriting vfe clock rates for 8250 Jordan Crouse 2024-08-02 22:20 ` Bryan O'Donoghue 2024-08-06 22:38 ` Jordan Crouse
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).