| Message ID | 20230405-add-dsc-support-v4-5-15daf84f8dcb@quicinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add DSC v1.2 Support for DSI | expand |
On 2023-05-22 13:30:24, Jessica Zhang wrote: > Currently, slice_count is being used to calculate word count and > pkt_per_line. Instead, these values should be calculated using slice per > packet, which is not the same as slice_count. > > Slice count represents the number of soft slices per interface, and its > value will not always match that of slice per packet. For example, it is > possible to have cases where there are multiple soft slices per interface > but the panel specifies only one slice per packet. As discussed in many patches before, there is no definition of "soft slices" anyhwere. Unless we can have that, and reference it, this should more clearly explain what it means or leave out the word "soft" altogether. > Thus, use the default value of one slice per packet and remove slice_count > from the aforementioned calculations. > > Fixes: 08802f515c3c ("drm/msm/dsi: Add support for DSC configuration") > Fixes: bc6b6ff8135c ("drm/msm/dsi: Use DSC slice(s) packet size to compute word count") > Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com> > --- > drivers/gpu/drm/msm/dsi/dsi_host.c | 26 ++++++++++++++++---------- > 1 file changed, 16 insertions(+), 10 deletions(-) > > diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c > index d04f8bbd707d..2eed99afdba9 100644 > --- a/drivers/gpu/drm/msm/dsi/dsi_host.c > +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c > @@ -866,18 +866,17 @@ static void dsi_update_dsc_timing(struct msm_dsi_host *msm_host, bool is_cmd_mod > */ > slice_per_intf = msm_dsc_get_slices_per_intf(dsc, hdisplay); > > - /* > - * If slice_count is greater than slice_per_intf > - * then default to 1. This can happen during partial > - * update. > - */ > - if (dsc->slice_count > slice_per_intf) > - dsc->slice_count = 1; > - > total_bytes_per_intf = dsc->slice_chunk_size * slice_per_intf; > > eol_byte_num = total_bytes_per_intf % 3; > - pkt_per_line = slice_per_intf / dsc->slice_count; > + > + /* > + * Typically, pkt_per_line = slice_per_intf * slice_per_pkt. > + * > + * Since the current driver only supports slice_per_pkt = 1, > + * pkt_per_line will be equal to slice per intf for now. > + */ > + pkt_per_line = slice_per_intf; > > if (is_cmd_mode) /* packet data type */ > reg = DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE(MIPI_DSI_DCS_LONG_WRITE); > @@ -1001,7 +1000,14 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi) > if (!msm_host->dsc) > wc = hdisplay * dsi_get_bpp(msm_host->format) / 8 + 1; > else > - wc = msm_host->dsc->slice_chunk_size * msm_host->dsc->slice_count + 1; > + /* > + * When DSC is enabled, WC = slice_chunk_size * slice_per_packet + 1. > + * Currently, the driver only supports default value of slice_per_packet = 1 > + * > + * TODO: Expand mipi_dsi_device struct to hold slice_per_packet info > + * and adjust DSC math to account for slice_per_packet. > + */ Either rename this all to slice_per_pkt, or rename the above comment to slice_per_packet. After improving on that: Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org> We all learned the wrong thing initially, thanks for clearing up that slice_count != slice_per_pkt. - Marijn > + wc = msm_host->dsc->slice_chunk_size + 1; > > dsi_write(msm_host, REG_DSI_CMD_MDP_STREAM0_CTRL, > DSI_CMD_MDP_STREAM0_CTRL_WORD_COUNT(wc) | > > -- > 2.40.1 >
One more suggestion: DSC slice_count* On 2023-05-22 13:30:24, Jessica Zhan <Snip> - Marijn
On 5/22/2023 2:01 PM, Marijn Suijten wrote: > On 2023-05-22 13:30:24, Jessica Zhang wrote: >> Currently, slice_count is being used to calculate word count and >> pkt_per_line. Instead, these values should be calculated using slice per >> packet, which is not the same as slice_count. >> >> Slice count represents the number of soft slices per interface, and its >> value will not always match that of slice per packet. For example, it is >> possible to have cases where there are multiple soft slices per interface >> but the panel specifies only one slice per packet. > > As discussed in many patches before, there is no definition of "soft > slices" anyhwere. Unless we can have that, and reference it, this > should more clearly explain what it means or leave out the word "soft" > altogether. Hi Marijn, Acked. > >> Thus, use the default value of one slice per packet and remove slice_count >> from the aforementioned calculations. >> >> Fixes: 08802f515c3c ("drm/msm/dsi: Add support for DSC configuration") >> Fixes: bc6b6ff8135c ("drm/msm/dsi: Use DSC slice(s) packet size to compute word count") >> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com> >> --- >> drivers/gpu/drm/msm/dsi/dsi_host.c | 26 ++++++++++++++++---------- >> 1 file changed, 16 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c >> index d04f8bbd707d..2eed99afdba9 100644 >> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c >> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c >> @@ -866,18 +866,17 @@ static void dsi_update_dsc_timing(struct msm_dsi_host *msm_host, bool is_cmd_mod >> */ >> slice_per_intf = msm_dsc_get_slices_per_intf(dsc, hdisplay); >> >> - /* >> - * If slice_count is greater than slice_per_intf >> - * then default to 1. This can happen during partial >> - * update. >> - */ >> - if (dsc->slice_count > slice_per_intf) >> - dsc->slice_count = 1; >> - >> total_bytes_per_intf = dsc->slice_chunk_size * slice_per_intf; >> >> eol_byte_num = total_bytes_per_intf % 3; >> - pkt_per_line = slice_per_intf / dsc->slice_count; >> + >> + /* >> + * Typically, pkt_per_line = slice_per_intf * slice_per_pkt. >> + * >> + * Since the current driver only supports slice_per_pkt = 1, >> + * pkt_per_line will be equal to slice per intf for now. >> + */ >> + pkt_per_line = slice_per_intf; >> >> if (is_cmd_mode) /* packet data type */ >> reg = DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE(MIPI_DSI_DCS_LONG_WRITE); >> @@ -1001,7 +1000,14 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi) >> if (!msm_host->dsc) >> wc = hdisplay * dsi_get_bpp(msm_host->format) / 8 + 1; >> else >> - wc = msm_host->dsc->slice_chunk_size * msm_host->dsc->slice_count + 1; >> + /* >> + * When DSC is enabled, WC = slice_chunk_size * slice_per_packet + 1. >> + * Currently, the driver only supports default value of slice_per_packet = 1 >> + * >> + * TODO: Expand mipi_dsi_device struct to hold slice_per_packet info >> + * and adjust DSC math to account for slice_per_packet. >> + */ > > Either rename this all to slice_per_pkt, or rename the above comment to > slice_per_packet. Acked. Thanks, Jessica Zhang > > After improving on that: > > Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org> > > We all learned the wrong thing initially, thanks for clearing up that > slice_count != slice_per_pkt. > > - Marijn > >> + wc = msm_host->dsc->slice_chunk_size + 1; >> >> dsi_write(msm_host, REG_DSI_CMD_MDP_STREAM0_CTRL, >> DSI_CMD_MDP_STREAM0_CTRL_WORD_COUNT(wc) | >> >> -- >> 2.40.1 >>
diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c index d04f8bbd707d..2eed99afdba9 100644 --- a/drivers/gpu/drm/msm/dsi/dsi_host.c +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c @@ -866,18 +866,17 @@ static void dsi_update_dsc_timing(struct msm_dsi_host *msm_host, bool is_cmd_mod */ slice_per_intf = msm_dsc_get_slices_per_intf(dsc, hdisplay); - /* - * If slice_count is greater than slice_per_intf - * then default to 1. This can happen during partial - * update. - */ - if (dsc->slice_count > slice_per_intf) - dsc->slice_count = 1; - total_bytes_per_intf = dsc->slice_chunk_size * slice_per_intf; eol_byte_num = total_bytes_per_intf % 3; - pkt_per_line = slice_per_intf / dsc->slice_count; + + /* + * Typically, pkt_per_line = slice_per_intf * slice_per_pkt. + * + * Since the current driver only supports slice_per_pkt = 1, + * pkt_per_line will be equal to slice per intf for now. + */ + pkt_per_line = slice_per_intf; if (is_cmd_mode) /* packet data type */ reg = DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE(MIPI_DSI_DCS_LONG_WRITE); @@ -1001,7 +1000,14 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi) if (!msm_host->dsc) wc = hdisplay * dsi_get_bpp(msm_host->format) / 8 + 1; else - wc = msm_host->dsc->slice_chunk_size * msm_host->dsc->slice_count + 1; + /* + * When DSC is enabled, WC = slice_chunk_size * slice_per_packet + 1. + * Currently, the driver only supports default value of slice_per_packet = 1 + * + * TODO: Expand mipi_dsi_device struct to hold slice_per_packet info + * and adjust DSC math to account for slice_per_packet. + */ + wc = msm_host->dsc->slice_chunk_size + 1; dsi_write(msm_host, REG_DSI_CMD_MDP_STREAM0_CTRL, DSI_CMD_MDP_STREAM0_CTRL_WORD_COUNT(wc) |
Currently, slice_count is being used to calculate word count and pkt_per_line. Instead, these values should be calculated using slice per packet, which is not the same as slice_count. Slice count represents the number of soft slices per interface, and its value will not always match that of slice per packet. For example, it is possible to have cases where there are multiple soft slices per interface but the panel specifies only one slice per packet. Thus, use the default value of one slice per packet and remove slice_count from the aforementioned calculations. Fixes: 08802f515c3c ("drm/msm/dsi: Add support for DSC configuration") Fixes: bc6b6ff8135c ("drm/msm/dsi: Use DSC slice(s) packet size to compute word count") Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com> --- drivers/gpu/drm/msm/dsi/dsi_host.c | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-)