| Message ID | 20230329-rfc-msm-dsc-helper-v5-6-0108401d7886@quicinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Introduce MSM-specific DSC helpers | expand |
On 12/04/2023 22:09, Jessica Zhang wrote: > Add a check for valid dsc->slice_width value in dsi_timing_setup. > > Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com> > --- > drivers/gpu/drm/msm/dsi/dsi_host.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c > index 508577c596ff..6a6218a9655f 100644 > --- a/drivers/gpu/drm/msm/dsi/dsi_host.c > +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c > @@ -937,6 +937,12 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi) > return; > } > > + if (!dsc->slice_width || (mode->hdisplay < dsc->slice_width)) { This is an erroneous condition, correct. Can we move it to a better place, where we can return an error instead of ignoring it? I'd say that we should validate dsc->slice_width at the dsi_host_attach(). It well might be a good idea to add a helper that validates required dsc properties (e.g. version, bpp/bpc, slice_width, slice_height, slice_count). As for the mode->hdisplay, we have the following code in msm_dsi_host_check_dsc() (where pic_width = mode->hdisplay): if (pic_width % dsc->slice_width) {...} This way the only way how mode->hdisplay can be less than dsc->slice_width is if mode->hdisplay is 0 (which is forbidden if I remember correctly). So the second part of the check is useless. > + pr_err("DSI: invalid slice width %d (pic_width: %d)\n", > + dsc->slice_width, mode->hdisplay); > + return; > + } > + > dsc->pic_width = mode->hdisplay; > dsc->pic_height = mode->vdisplay; > DBG("Mode %dx%d\n", dsc->pic_width, dsc->pic_height); >
On 4/12/2023 12:24 PM, Dmitry Baryshkov wrote: > On 12/04/2023 22:09, Jessica Zhang wrote: >> Add a check for valid dsc->slice_width value in dsi_timing_setup. >> >> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com> >> --- >> drivers/gpu/drm/msm/dsi/dsi_host.c | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c >> b/drivers/gpu/drm/msm/dsi/dsi_host.c >> index 508577c596ff..6a6218a9655f 100644 >> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c >> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c >> @@ -937,6 +937,12 @@ static void dsi_timing_setup(struct msm_dsi_host >> *msm_host, bool is_bonded_dsi) >> return; >> } >> + if (!dsc->slice_width || (mode->hdisplay < dsc->slice_width)) { > > This is an erroneous condition, correct. Can we move it to a better > place, where we can return an error instead of ignoring it? > > I'd say that we should validate dsc->slice_width at the > dsi_host_attach(). It well might be a good idea to add a helper that > validates required dsc properties (e.g. version, bpp/bpc, slice_width, > slice_height, slice_count). > > As for the mode->hdisplay, we have the following code in > msm_dsi_host_check_dsc() (where pic_width = mode->hdisplay): > > if (pic_width % dsc->slice_width) {...} > > This way the only way how mode->hdisplay can be less than > dsc->slice_width is if mode->hdisplay is 0 (which is forbidden if I > remember correctly). So the second part of the check is useless. > Lets drop this from this series and come up with a better approach to validate dsc params. We will take it up once dsc over dsi and dp lands. >> + pr_err("DSI: invalid slice width %d (pic_width: %d)\n", >> + dsc->slice_width, mode->hdisplay); >> + return; >> + } >> + >> dsc->pic_width = mode->hdisplay; >> dsc->pic_height = mode->vdisplay; >> DBG("Mode %dx%d\n", dsc->pic_width, dsc->pic_height); >> >
On 13/04/2023 01:40, Abhinav Kumar wrote: > > > On 4/12/2023 12:24 PM, Dmitry Baryshkov wrote: >> On 12/04/2023 22:09, Jessica Zhang wrote: >>> Add a check for valid dsc->slice_width value in dsi_timing_setup. >>> >>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com> >>> --- >>> drivers/gpu/drm/msm/dsi/dsi_host.c | 6 ++++++ >>> 1 file changed, 6 insertions(+) >>> >>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c >>> b/drivers/gpu/drm/msm/dsi/dsi_host.c >>> index 508577c596ff..6a6218a9655f 100644 >>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c >>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c >>> @@ -937,6 +937,12 @@ static void dsi_timing_setup(struct msm_dsi_host >>> *msm_host, bool is_bonded_dsi) >>> return; >>> } >>> + if (!dsc->slice_width || (mode->hdisplay < dsc->slice_width)) { >> >> This is an erroneous condition, correct. Can we move it to a better >> place, where we can return an error instead of ignoring it? >> >> I'd say that we should validate dsc->slice_width at the >> dsi_host_attach(). It well might be a good idea to add a helper that >> validates required dsc properties (e.g. version, bpp/bpc, slice_width, >> slice_height, slice_count). >> >> As for the mode->hdisplay, we have the following code in >> msm_dsi_host_check_dsc() (where pic_width = mode->hdisplay): >> >> if (pic_width % dsc->slice_width) {...} >> >> This way the only way how mode->hdisplay can be less than >> dsc->slice_width is if mode->hdisplay is 0 (which is forbidden if I >> remember correctly). So the second part of the check is useless. >> > > Lets drop this from this series and come up with a better approach to > validate dsc params. We will take it up once dsc over dsi and dp lands. Sure, why not. > >>> + pr_err("DSI: invalid slice width %d (pic_width: %d)\n", >>> + dsc->slice_width, mode->hdisplay); >>> + return; >>> + } >>> + >>> dsc->pic_width = mode->hdisplay; >>> dsc->pic_height = mode->vdisplay; >>> DBG("Mode %dx%d\n", dsc->pic_width, dsc->pic_height); >>> >>
diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c index 508577c596ff..6a6218a9655f 100644 --- a/drivers/gpu/drm/msm/dsi/dsi_host.c +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c @@ -937,6 +937,12 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi) return; } + if (!dsc->slice_width || (mode->hdisplay < dsc->slice_width)) { + pr_err("DSI: invalid slice width %d (pic_width: %d)\n", + dsc->slice_width, mode->hdisplay); + return; + } + dsc->pic_width = mode->hdisplay; dsc->pic_height = mode->vdisplay; DBG("Mode %dx%d\n", dsc->pic_width, dsc->pic_height);
Add a check for valid dsc->slice_width value in dsi_timing_setup. Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com> --- drivers/gpu/drm/msm/dsi/dsi_host.c | 6 ++++++ 1 file changed, 6 insertions(+)