| Message ID | 20250203181436.87785-4-danila@jiaxyga.com |
|---|---|
| State | New |
| Headers | show |
| Series | sm7325-nothing-spacewar: Add and enable the panel | expand |
On Mon, Feb 03, 2025 at 09:14:26PM +0300, Danila Tikhonov wrote: > From: Eugene Lepshy <fekz115@gmail.com> > > DRM DSC helper has parameters for various bpc values other than 8: > (8/10/12/14/16). > > Remove this guard. > > Signed-off-by: Eugene Lepshy <fekz115@gmail.com> > Signed-off-by: Danila Tikhonov <danila@jiaxyga.com> > --- > drivers/gpu/drm/msm/dsi/dsi_host.c | 7 +------ > 1 file changed, 1 insertion(+), 6 deletions(-) > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
On 2025-02-03 21:14:26, Danila Tikhonov wrote: > From: Eugene Lepshy <fekz115@gmail.com> > > DRM DSC helper has parameters for various bpc values other than 8: Weird zero-width \u200b spaces here between "values" and "other", please delete those. > (8/10/12/14/16). > > Remove this guard. > > Signed-off-by: Eugene Lepshy <fekz115@gmail.com> > Signed-off-by: Danila Tikhonov <danila@jiaxyga.com> Should this patch elaborate that those "DRM DSC helper" don't have any additional guarding for the values you mention either, i.e. passing 9 or 11 or >16 don't seem to be checked anywhere else either? And your title might have space to spell out "Bits Per Component" entirely. > --- > drivers/gpu/drm/msm/dsi/dsi_host.c | 7 +------ > 1 file changed, 1 insertion(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c > index 007311c21fda..d182af7bbb81 100644 > --- a/drivers/gpu/drm/msm/dsi/dsi_host.c > +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c > @@ -1767,11 +1767,6 @@ static int dsi_populate_dsc_params(struct msm_dsi_host *msm_host, struct drm_dsc > return -EINVAL; > } > > - if (dsc->bits_per_component != 8) { > - DRM_DEV_ERROR(&msm_host->pdev->dev, "DSI does not support bits_per_component != 8 yet\n"); > - return -EOPNOTSUPP; > - } > - > dsc->simple_422 = 0; > dsc->convert_rgb = 1; > dsc->vbr_enable = 0; This seems supicous on the dpu1 side, in the original DSC 1.1 (not 1.2) block in dpu_hw_dsc_config(), which has: data |= (dsc->line_buf_depth << 3); data |= (dsc->simple_422 << 2); data |= (dsc->convert_rgb << 1); data |= dsc->bits_per_component; The original value of `8` would overlap with the lowest bit of line_buf_depth (4th bit in `data`). Now, the 2nd bit which will take the value from convert_rgb, which is already set to 1 above, will overlap with the 2nd bit in your new bpc value of 10. Can you double-check that this code in DPU1 is actually valid? I assume you have tested this panel at least and it is working (worthy mention in the cover letter?), this just seems like yet another mistake in the original bindings (though the register always had a matching value with downstream on 8 BPC panels for me). > @@ -1779,7 +1774,7 @@ static int dsi_populate_dsc_params(struct msm_dsi_host *msm_host, struct drm_dsc > drm_dsc_set_const_params(dsc); > drm_dsc_set_rc_buf_thresh(dsc); > > - /* handle only bpp = bpc = 8, pre-SCR panels */ > + /* handle only pre-SCR panels */ > ret = drm_dsc_setup_rc_params(dsc, DRM_DSC_1_1_PRE_SCR); Good catch - this comment sounds like it's documenting a limitation of this helper function, but the function does not have such limitations... rc_parameters_pre_scr has values for all these combinations. - Marijn > if (ret) { > DRM_DEV_ERROR(&msm_host->pdev->dev, "could not find DSC RC parameters\n"); > -- > 2.48.1 >
On Sat, Feb 08, 2025 at 11:09:56PM +0100, Marijn Suijten wrote: > On 2025-02-03 21:14:26, Danila Tikhonov wrote: > > From: Eugene Lepshy <fekz115@gmail.com> > > > > DRM DSC helper has parameters for various bpc values other than 8: > > Weird zero-width \u200b spaces here between "values" and "other", please delete > those. > > > (8/10/12/14/16). > > > > Remove this guard. > > > > Signed-off-by: Eugene Lepshy <fekz115@gmail.com> > > Signed-off-by: Danila Tikhonov <danila@jiaxyga.com> > > Should this patch elaborate that those "DRM DSC helper" don't have any > additional guarding for the values you mention either, i.e. passing 9 or 11 or > >16 don't seem to be checked anywhere else either? > > And your title might have space to spell out "Bits Per Component" entirely. > > > --- > > drivers/gpu/drm/msm/dsi/dsi_host.c | 7 +------ > > 1 file changed, 1 insertion(+), 6 deletions(-) > > > > diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c > > index 007311c21fda..d182af7bbb81 100644 > > --- a/drivers/gpu/drm/msm/dsi/dsi_host.c > > +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c > > @@ -1767,11 +1767,6 @@ static int dsi_populate_dsc_params(struct msm_dsi_host *msm_host, struct drm_dsc > > return -EINVAL; > > } > > > > - if (dsc->bits_per_component != 8) { > > - DRM_DEV_ERROR(&msm_host->pdev->dev, "DSI does not support bits_per_component != 8 yet\n"); > > - return -EOPNOTSUPP; > > - } > > - > > dsc->simple_422 = 0; > > dsc->convert_rgb = 1; > > dsc->vbr_enable = 0; > > This seems supicous on the dpu1 side, in the original DSC 1.1 (not 1.2) block in > dpu_hw_dsc_config(), which has: > > data |= (dsc->line_buf_depth << 3); > data |= (dsc->simple_422 << 2); > data |= (dsc->convert_rgb << 1); > data |= dsc->bits_per_component; > > The original value of `8` would overlap with the lowest bit of line_buf_depth > (4th bit in `data`). Now, the 2nd bit which will take the value from > convert_rgb, which is already set to 1 above, will overlap with the 2nd bit in > your new bpc value of 10. > > Can you double-check that this code in DPU1 is actually valid? I assume you > have tested this panel at least and it is working (worthy mention in the cover > letter?), this just seems like yet another mistake in the original bindings > (though the register always had a matching value with downstream on 8 BPC panels > for me). Indeed. msm-4.14 explicitly names that single-bit field as 'input_10_bits'. The block is supposed to support bpc of 8, 10 and 12. This bit should only be set for bpc=10. Marijn, thanks for catching it! We should start rewriting DPU register accessors to use generated accessors. At least it will clearly show if the field is a flag or a field which has some values. With the current code it is impossible to notice the difference. > > > @@ -1779,7 +1774,7 @@ static int dsi_populate_dsc_params(struct msm_dsi_host *msm_host, struct drm_dsc > > drm_dsc_set_const_params(dsc); > > drm_dsc_set_rc_buf_thresh(dsc); > > > > - /* handle only bpp = bpc = 8, pre-SCR panels */ > > + /* handle only pre-SCR panels */ > > ret = drm_dsc_setup_rc_params(dsc, DRM_DSC_1_1_PRE_SCR); > > Good catch - this comment sounds like it's documenting a limitation of > this helper function, but the function does not have such limitations... > rc_parameters_pre_scr has values for all these combinations. I think the =8 part is a leftover of the old, pre-helper code. > > - Marijn > > > if (ret) { > > DRM_DEV_ERROR(&msm_host->pdev->dev, "could not find DSC RC parameters\n"); > > -- > > 2.48.1 > >
On 8.02.2025 11:09 PM, Marijn Suijten wrote: > On 2025-02-03 21:14:26, Danila Tikhonov wrote: >> From: Eugene Lepshy <fekz115@gmail.com> >> >> DRM DSC helper has parameters for various bpc values other than 8: > > Weird zero-width \u200b spaces here between "values" and "other", please delete > those. > >> (8/10/12/14/16). >> >> Remove this guard. >> >> Signed-off-by: Eugene Lepshy <fekz115@gmail.com> >> Signed-off-by: Danila Tikhonov <danila@jiaxyga.com> > > Should this patch elaborate that those "DRM DSC helper" don't have any > additional guarding for the values you mention either, i.e. passing 9 or 11 or >> 16 don't seem to be checked anywhere else either? > > And your title might have space to spell out "Bits Per Component" entirely. > >> --- >> drivers/gpu/drm/msm/dsi/dsi_host.c | 7 +------ >> 1 file changed, 1 insertion(+), 6 deletions(-) >> >> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c >> index 007311c21fda..d182af7bbb81 100644 >> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c >> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c >> @@ -1767,11 +1767,6 @@ static int dsi_populate_dsc_params(struct msm_dsi_host *msm_host, struct drm_dsc >> return -EINVAL; >> } >> >> - if (dsc->bits_per_component != 8) { >> - DRM_DEV_ERROR(&msm_host->pdev->dev, "DSI does not support bits_per_component != 8 yet\n"); >> - return -EOPNOTSUPP; >> - } >> - >> dsc->simple_422 = 0; >> dsc->convert_rgb = 1; >> dsc->vbr_enable = 0; > > This seems supicous on the dpu1 side, in the original DSC 1.1 (not 1.2) block in > dpu_hw_dsc_config(), which has: > > data |= (dsc->line_buf_depth << 3); > data |= (dsc->simple_422 << 2); > data |= (dsc->convert_rgb << 1); > data |= dsc->bits_per_component; > > The original value of `8` would overlap with the lowest bit of line_buf_depth > (4th bit in `data`). Now, the 2nd bit which will take the value from > convert_rgb, which is already set to 1 above, will overlap with the 2nd bit in > your new bpc value of 10. > > Can you double-check that this code in DPU1 is actually valid? I assume you > have tested this panel at least and it is working (worthy mention in the cover > letter?), this just seems like yet another mistake in the original bindings > (though the register always had a matching value with downstream on 8 BPC panels > for me). It seems like the lowest bit should be set iff the input is 10bpc, the current situation where our '8' bleeds into the following (correctly named fields) is bad. Konrad
On 10.02.2025 6:10 PM, Konrad Dybcio wrote: > On 8.02.2025 11:09 PM, Marijn Suijten wrote: >> On 2025-02-03 21:14:26, Danila Tikhonov wrote: >>> From: Eugene Lepshy <fekz115@gmail.com> >>> >>> DRM DSC helper has parameters for various bpc values other than 8: >> >> Weird zero-width \u200b spaces here between "values" and "other", please delete >> those. >> >>> (8/10/12/14/16). >>> >>> Remove this guard. >>> >>> Signed-off-by: Eugene Lepshy <fekz115@gmail.com> >>> Signed-off-by: Danila Tikhonov <danila@jiaxyga.com> >> >> Should this patch elaborate that those "DRM DSC helper" don't have any >> additional guarding for the values you mention either, i.e. passing 9 or 11 or >>> 16 don't seem to be checked anywhere else either? >> >> And your title might have space to spell out "Bits Per Component" entirely. >> >>> --- >>> drivers/gpu/drm/msm/dsi/dsi_host.c | 7 +------ >>> 1 file changed, 1 insertion(+), 6 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c >>> index 007311c21fda..d182af7bbb81 100644 >>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c >>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c >>> @@ -1767,11 +1767,6 @@ static int dsi_populate_dsc_params(struct msm_dsi_host *msm_host, struct drm_dsc >>> return -EINVAL; >>> } >>> >>> - if (dsc->bits_per_component != 8) { >>> - DRM_DEV_ERROR(&msm_host->pdev->dev, "DSI does not support bits_per_component != 8 yet\n"); >>> - return -EOPNOTSUPP; >>> - } >>> - >>> dsc->simple_422 = 0; >>> dsc->convert_rgb = 1; >>> dsc->vbr_enable = 0; >> >> This seems supicous on the dpu1 side, in the original DSC 1.1 (not 1.2) block in >> dpu_hw_dsc_config(), which has: >> >> data |= (dsc->line_buf_depth << 3); >> data |= (dsc->simple_422 << 2); >> data |= (dsc->convert_rgb << 1); >> data |= dsc->bits_per_component; >> >> The original value of `8` would overlap with the lowest bit of line_buf_depth >> (4th bit in `data`). Now, the 2nd bit which will take the value from >> convert_rgb, which is already set to 1 above, will overlap with the 2nd bit in >> your new bpc value of 10. >> >> Can you double-check that this code in DPU1 is actually valid? I assume you >> have tested this panel at least and it is working (worthy mention in the cover >> letter?), this just seems like yet another mistake in the original bindings >> (though the register always had a matching value with downstream on 8 BPC panels >> for me). > > It seems like the lowest bit should be set iff the input is 10bpc, the > current situation where our '8' bleeds into the following (correctly named > fields) is bad. See also https://github.com/Wikidepia/kernel_xiaomi_santoni-4.9/blob/master/drivers/gpu/drm/msm/sde/sde_hw_dsc.c#L67-L80 Konrad
On Tue, Feb 11, 2025 at 09:06:19PM +0300, Danila Tikhonov wrote: > On 2/9/25 01:09, Marijn Suijten wrote: > > On 2025-02-03 21:14:26, Danila Tikhonov wrote: > > > From: Eugene Lepshy <fekz115@gmail.com> > > > > > > DRM DSC helper has parameters for various bpc values other than 8: > > Weird zero-width \u200b spaces here between "values" and "other", please delete > > those. > Thanks, I will fix it in the next version. > > > (8/10/12/14/16). > > > > > > Remove this guard. > > > > > > Signed-off-by: Eugene Lepshy <fekz115@gmail.com> > > > Signed-off-by: Danila Tikhonov <danila@jiaxyga.com> > > Should this patch elaborate that those "DRM DSC helper" don't have any > > additional guarding for the values you mention either, i.e. passing 9 or 11 or > > > 16 don't seem to be checked anywhere else either? > There are no other bpc checks, you are right. But to be honest I don't > really see any sense in this. Anyway, if you still want us to leave the > current guard and just extend it with new values (for example via > switch case) - let me know. Yes, please. Add a caselist and also a note that only 8, 10 and 12 are valid for DSC 1.1 block. Then whoever stomps upon other bpc value will have to extend the check, verifying DSC version. > > And your title might have space to spell out "Bits Per Component" entirely. > I'll fix that too. > > > --- > > > drivers/gpu/drm/msm/dsi/dsi_host.c | 7 +------ > > > 1 file changed, 1 insertion(+), 6 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c > > > index 007311c21fda..d182af7bbb81 100644 > > > --- a/drivers/gpu/drm/msm/dsi/dsi_host.c > > > +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c > > > @@ -1767,11 +1767,6 @@ static int dsi_populate_dsc_params(struct msm_dsi_host *msm_host, struct drm_dsc > > > return -EINVAL; > > > } > > > - if (dsc->bits_per_component != 8) { > > > - DRM_DEV_ERROR(&msm_host->pdev->dev, "DSI does not support bits_per_component != 8 yet\n"); > > > - return -EOPNOTSUPP; > > > - } > > > - > > > dsc->simple_422 = 0; > > > dsc->convert_rgb = 1; > > > dsc->vbr_enable = 0; > > This seems supicous on the dpu1 side, in the original DSC 1.1 (not 1.2) block in > > dpu_hw_dsc_config(), which has: > > > > data |= (dsc->line_buf_depth << 3); > > data |= (dsc->simple_422 << 2); > > data |= (dsc->convert_rgb << 1); > > data |= dsc->bits_per_component; > > > > The original value of `8` would overlap with the lowest bit of line_buf_depth > > (4th bit in `data`). Now, the 2nd bit which will take the value from > > convert_rgb, which is already set to 1 above, will overlap with the 2nd bit in > > your new bpc value of 10. > > > > Can you double-check that this code in DPU1 is actually valid? I assume you > > have tested this panel at least and it is working (worthy mention in the cover > > letter?), this just seems like yet another mistake in the original bindings > > (though the register always had a matching value with downstream on 8 BPC panels > > for me). > > Of course I have tested the panel and it works, I just thought it would > be obvious. We also have tested sm7150-xiaomi-courbet, sm8450-xiaomi-cupid > and sm8475-nothing-pong, which already have bpp = bpc = 10 panels and > with some hack it also work without any changes to the DRM. This is now being fixed by a separate patch. > > > > @@ -1779,7 +1774,7 @@ static int dsi_populate_dsc_params(struct msm_dsi_host *msm_host, struct drm_dsc > > > drm_dsc_set_const_params(dsc); > > > drm_dsc_set_rc_buf_thresh(dsc); > > > - /* handle only bpp = bpc = 8, pre-SCR panels */ > > > + /* handle only pre-SCR panels */ > > > ret = drm_dsc_setup_rc_params(dsc, DRM_DSC_1_1_PRE_SCR); > > Good catch - this comment sounds like it's documenting a limitation of > > this helper function, but the function does not have such limitations... > > rc_parameters_pre_scr has values for all these combinations. > Maybe we should remove this comment entirely? No, the pre-SCR comment is fine. > > Regards, > Danila > > - Marijn > > > > > if (ret) { > > > DRM_DEV_ERROR(&msm_host->pdev->dev, "could not find DSC RC parameters\n"); > > > -- > > > 2.48.1 > > >
diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c index 007311c21fda..d182af7bbb81 100644 --- a/drivers/gpu/drm/msm/dsi/dsi_host.c +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c @@ -1767,11 +1767,6 @@ static int dsi_populate_dsc_params(struct msm_dsi_host *msm_host, struct drm_dsc return -EINVAL; } - if (dsc->bits_per_component != 8) { - DRM_DEV_ERROR(&msm_host->pdev->dev, "DSI does not support bits_per_component != 8 yet\n"); - return -EOPNOTSUPP; - } - dsc->simple_422 = 0; dsc->convert_rgb = 1; dsc->vbr_enable = 0; @@ -1779,7 +1774,7 @@ static int dsi_populate_dsc_params(struct msm_dsi_host *msm_host, struct drm_dsc drm_dsc_set_const_params(dsc); drm_dsc_set_rc_buf_thresh(dsc); - /* handle only bpp = bpc = 8, pre-SCR panels */ + /* handle only pre-SCR panels */ ret = drm_dsc_setup_rc_params(dsc, DRM_DSC_1_1_PRE_SCR); if (ret) { DRM_DEV_ERROR(&msm_host->pdev->dev, "could not find DSC RC parameters\n");