On 17-02-22, 16:11, Marijn Suijten wrote:
Hi Vinod,
Thanks for taking time to go through this review, please find some clarifications below.
On 2022-02-17 16:44:04, Vinod Koul wrote:
Hi Marijn,
On 11-12-21, 01:03, Marijn Suijten wrote:
+static int dsi_dsc_update_pic_dim(struct msm_display_dsc_config *dsc,
int pic_width, int pic_height)This function - adopted from downstream - does not seem to perform a whole lot, especially without the modulo checks against the slice size. Perhaps it can be inlined?
Most of the code here is :)
This was split from downstream code to check and update dimension. We can inline this, or should we leave that to compiler. I am not a very big fan of inlining...
It doesn't seem beneficial to code readability to have this function, which is only called just once and also has the same struct members read in a `DBG()` directly, abstracted away to a function. Not really concerned about generated code/performance FWIW.
Also note that the caller isn't checking the `-EINVAL` result...
I have made this void inline.
+{
- if (!dsc || !pic_width || !pic_height) {
pr_err("DSI: invalid input: pic_width: %d pic_height: %d\n", pic_width, pic_height);return -EINVAL;- }
- dsc->drm->pic_width = pic_width;
- dsc->drm->pic_height = pic_height;
- return 0;
+}
static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi) { struct drm_display_mode *mode = msm_host->mode; @@ -940,7 +954,68 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi) hdisplay /= 2; }
- if (msm_host->dsc) {
struct msm_display_dsc_config *dsc = msm_host->dsc;/* update dsc params with timing params */dsi_dsc_update_pic_dim(dsc, mode->hdisplay, mode->vdisplay);That is, the result code here should be checked (or function inlined).
This function return void, so no point in checking
/* we do the calculations for dsc parameters here so that* panel can use these parameters*/dsi_populate_dsc_params(dsc);/* Divide the display by 3 but keep back/font porch and* pulse width same*/A more general nit on the comments in this patch series: it is appreciated if comments explain the rationale rather than - or in addition to - merely paraphrasing the code that follows.
Yes it might be the case here, but in this case I wanted to explicitly point out hat we need to divide display by 3...
The main point here is justifying _why_ there's a division by 3 for the active portion of the signal, I presume that's the compression ratio (having not read into the DSC compression standard yet at all)?
I have updated this comment
if (msm_host->dsc) {struct msm_display_dsc_config *dsc = msm_host->dsc;u32 reg, reg_ctrl, reg_ctrl2;u32 slice_per_intf, bytes_in_slice, total_bytes_per_intf;reg_ctrl = dsi_read(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL);reg_ctrl2 = dsi_read(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL2);Shouldn't old values be masked out first, before writing new bits or values below? The video-mode variant doesn't read back old register values.
This follows downstream where the registers are read, modified and written back
Are you sure about this? The register values are never cleared, meaning that only bits get added through the `|=` below but never unset - unless downstream clears these registers elsewhere before ending up in (their equivalent of) dsi_timing_setup.
I have modified video mode to write and not read now. For command mode all bits are set to some value so no need to mask old values for that
Thanks. I forgot to mention: there seem to be a lot of similarities between the video and commandmode computations, can those possibly be factored out of the if-else to save on duplication and accidental mismatches like these?
Thanks, this was a good suggestion and am happy to report that I have incorporated this and indeed code looks better