| Message ID | 1410986741-6801-18-git-send-email-sakari.ailus@iki.fi (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Sakari, Thank you for the patch. On Wednesday 17 September 2014 23:45:41 Sakari Ailus wrote: > Decrease the link frequency to the next lower if the user chooses a media > bus code (BPP) cannot be achieved using the selected link frequency. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > --- > drivers/media/i2c/smiapp/smiapp-core.c | 20 ++++++++++++++++++-- > 1 file changed, 18 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/i2c/smiapp/smiapp-core.c > b/drivers/media/i2c/smiapp/smiapp-core.c index 537ca92..ce2c34d 100644 > --- a/drivers/media/i2c/smiapp/smiapp-core.c > +++ b/drivers/media/i2c/smiapp/smiapp-core.c > @@ -286,11 +286,27 @@ static int smiapp_pll_update(struct smiapp_sensor > *sensor) > > pll->binning_horizontal = sensor->binning_horizontal; > pll->binning_vertical = sensor->binning_vertical; > - pll->link_freq = > - sensor->link_freq->qmenu_int[sensor->link_freq->val]; > pll->scale_m = sensor->scale_m; > pll->bits_per_pixel = sensor->csi_format->compressed; > > + if (!test_bit(sensor->link_freq->val, > + &sensor->valid_link_freqs[ > + sensor->csi_format->compressed > + - SMIAPP_COMPRESSED_BASE])) { > + /* > + * Setting the link frequency will perform PLL > + * re-calculation already, so skip that. > + */ > + return __v4l2_ctrl_s_ctrl( > + sensor->link_freq, > + __ffs(sensor->valid_link_freqs[ > + sensor->csi_format->compressed > + - SMIAPP_COMPRESSED_BASE])); I have an uneasy feeling about this, as smiapp_pll_update is called from the link freq s_ctrl handler. Have you double-checked the recursion bounds ? > + } > + > + pll->link_freq = > + sensor->link_freq->qmenu_int[sensor->link_freq->val]; > + > rval = smiapp_pll_try(sensor, pll); > if (rval < 0) > return rval;
Hi Laurent, Thank you for your comments. On Fri, Sep 26, 2014 at 01:44:03PM +0300, Laurent Pinchart wrote: > Hi Sakari, > > Thank you for the patch. > > On Wednesday 17 September 2014 23:45:41 Sakari Ailus wrote: > > Decrease the link frequency to the next lower if the user chooses a media > > bus code (BPP) cannot be achieved using the selected link frequency. > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > --- > > drivers/media/i2c/smiapp/smiapp-core.c | 20 ++++++++++++++++++-- > > 1 file changed, 18 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/media/i2c/smiapp/smiapp-core.c > > b/drivers/media/i2c/smiapp/smiapp-core.c index 537ca92..ce2c34d 100644 > > --- a/drivers/media/i2c/smiapp/smiapp-core.c > > +++ b/drivers/media/i2c/smiapp/smiapp-core.c > > @@ -286,11 +286,27 @@ static int smiapp_pll_update(struct smiapp_sensor > > *sensor) > > > > pll->binning_horizontal = sensor->binning_horizontal; > > pll->binning_vertical = sensor->binning_vertical; > > - pll->link_freq = > > - sensor->link_freq->qmenu_int[sensor->link_freq->val]; > > pll->scale_m = sensor->scale_m; > > pll->bits_per_pixel = sensor->csi_format->compressed; > > > > + if (!test_bit(sensor->link_freq->val, > > + &sensor->valid_link_freqs[ > > + sensor->csi_format->compressed > > + - SMIAPP_COMPRESSED_BASE])) { > > + /* > > + * Setting the link frequency will perform PLL > > + * re-calculation already, so skip that. > > + */ > > + return __v4l2_ctrl_s_ctrl( > > + sensor->link_freq, > > + __ffs(sensor->valid_link_freqs[ > > + sensor->csi_format->compressed > > + - SMIAPP_COMPRESSED_BASE])); > > I have an uneasy feeling about this, as smiapp_pll_update is called from the > link freq s_ctrl handler. Have you double-checked the recursion bounds ? We haven't actually done any PLL tree calculation yet here. The condition will evaluate true in a case when the user chooses a format which isn't available on a given link frequency, or chooses a link frequency which isn't available for a given format. The condition will be false the next time the function is called since we've just chosen a valid combination of the two. But now that you brought the topic up, I think the link frequency selection should just probably return -EBUSY if the selected link frquency cannot be used. Also __ffs() should be __fls() instead in order to still come up with the highest link freqency.
diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c index 537ca92..ce2c34d 100644 --- a/drivers/media/i2c/smiapp/smiapp-core.c +++ b/drivers/media/i2c/smiapp/smiapp-core.c @@ -286,11 +286,27 @@ static int smiapp_pll_update(struct smiapp_sensor *sensor) pll->binning_horizontal = sensor->binning_horizontal; pll->binning_vertical = sensor->binning_vertical; - pll->link_freq = - sensor->link_freq->qmenu_int[sensor->link_freq->val]; pll->scale_m = sensor->scale_m; pll->bits_per_pixel = sensor->csi_format->compressed; + if (!test_bit(sensor->link_freq->val, + &sensor->valid_link_freqs[ + sensor->csi_format->compressed + - SMIAPP_COMPRESSED_BASE])) { + /* + * Setting the link frequency will perform PLL + * re-calculation already, so skip that. + */ + return __v4l2_ctrl_s_ctrl( + sensor->link_freq, + __ffs(sensor->valid_link_freqs[ + sensor->csi_format->compressed + - SMIAPP_COMPRESSED_BASE])); + } + + pll->link_freq = + sensor->link_freq->qmenu_int[sensor->link_freq->val]; + rval = smiapp_pll_try(sensor, pll); if (rval < 0) return rval;
Decrease the link frequency to the next lower if the user chooses a media bus code (BPP) cannot be achieved using the selected link frequency. Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> --- drivers/media/i2c/smiapp/smiapp-core.c | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-)