| Message ID | 20230405-add-dsc-support-v1-2-6bc6f03ae735@quicinc.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add DSC v1.2 Support for DSI | expand |
On 03/05/2023 04:19, Jessica Zhang wrote: > Currently, word count is calculated using slice_count. This is incorrect > as downstream uses slice per packet, which is different from > 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 word count calculation. > > 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 | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c > index 35c69dbe5f6f..b0d448ffb078 100644 > --- a/drivers/gpu/drm/msm/dsi/dsi_host.c > +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c > @@ -996,7 +996,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 drm_panel struct to hold slice_per_packet info > + * and adjust DSC math to account for slice_per_packet. slice_per_packet is not a part of the standard DSC, so I'm not sure how that can be implemented. And definitely we should not care about the drm_panel here. It should be either a part of drm_dsc_config, or mipi_dsi_device. > + */ > + 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) | >
On 5/3/2023 1:26 AM, Dmitry Baryshkov wrote: > On 03/05/2023 04:19, Jessica Zhang wrote: >> Currently, word count is calculated using slice_count. This is incorrect >> as downstream uses slice per packet, which is different from >> 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 word count calculation. >> >> 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 | 9 ++++++++- >> 1 file changed, 8 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c >> b/drivers/gpu/drm/msm/dsi/dsi_host.c >> index 35c69dbe5f6f..b0d448ffb078 100644 >> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c >> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c >> @@ -996,7 +996,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 drm_panel struct to hold slice_per_packet >> info >> + * and adjust DSC math to account for >> slice_per_packet. > > slice_per_packet is not a part of the standard DSC, so I'm not sure how > that can be implemented. And definitely we should not care about the > drm_panel here. It should be either a part of drm_dsc_config, or > mipi_dsi_device. Hi Dmitry, IIRC slice per packet is given by the panel specs with the default value being 1 if no value is specified, so it might be better to have it as part of mipi_dsi_device. Will update the TODO comment accordingly. Thanks, Jessica Zhang > >> + */ >> + 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) | >> > > -- > With best wishes > Dmitry >
On 5/3/2023 1:26 AM, Dmitry Baryshkov wrote: > On 03/05/2023 04:19, Jessica Zhang wrote: >> Currently, word count is calculated using slice_count. This is incorrect >> as downstream uses slice per packet, which is different from >> 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 word count calculation. >> >> 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 | 9 ++++++++- >> 1 file changed, 8 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c >> b/drivers/gpu/drm/msm/dsi/dsi_host.c >> index 35c69dbe5f6f..b0d448ffb078 100644 >> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c >> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c >> @@ -996,7 +996,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 drm_panel struct to hold slice_per_packet >> info >> + * and adjust DSC math to account for >> slice_per_packet. > > slice_per_packet is not a part of the standard DSC, so I'm not sure how > that can be implemented. And definitely we should not care about the > drm_panel here. It should be either a part of drm_dsc_config, or > mipi_dsi_device. > This is not correct. It is part of the DSI standard (not DSC standard). Please refer to Figure 40 "One Line Containing One Packet with Data from One or More Compressed Slices" and Figure 41 "One Line Containing More than One Compressed Pixel Stream Packet". This has details about this. So I still stand by my point that this should be in the drm_panel. >> + */ >> + 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) | >> >
On 08/05/2023 23:09, Abhinav Kumar wrote: > > > On 5/3/2023 1:26 AM, Dmitry Baryshkov wrote: >> On 03/05/2023 04:19, Jessica Zhang wrote: >>> Currently, word count is calculated using slice_count. This is incorrect >>> as downstream uses slice per packet, which is different from >>> 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 word count calculation. >>> >>> 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 | 9 ++++++++- >>> 1 file changed, 8 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c >>> b/drivers/gpu/drm/msm/dsi/dsi_host.c >>> index 35c69dbe5f6f..b0d448ffb078 100644 >>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c >>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c >>> @@ -996,7 +996,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 drm_panel struct to hold >>> slice_per_packet info >>> + * and adjust DSC math to account for >>> slice_per_packet. >> >> slice_per_packet is not a part of the standard DSC, so I'm not sure >> how that can be implemented. And definitely we should not care about >> the drm_panel here. It should be either a part of drm_dsc_config, or >> mipi_dsi_device. >> > > This is not correct. > > It is part of the DSI standard (not DSC standard). Please refer to > Figure 40 "One Line Containing One Packet with Data from One or More > Compressed Slices" and Figure 41 "One Line Containing More than One > Compressed Pixel Stream Packet". I have reviewed section 8.8.24 and Annex D of the DSI standard. It is not clear to me, if we can get away with always using slice_per_packet = 1. What is the DSI sink's difference between Fig. 40.(b) and Fig 41? Are there are known panels that require slice_per_packet != 1? If so, we will have to implement support for such configurations. > This has details about this. So I still stand by my point that this > should be in the drm_panel. Note, the driver doesn't use drm_panel directly. So slices_per_packet should go to mipi_dsi_device instead (which in turn can be filled from e.g. drm_panel or from any other source). > >>> + */ >>> + 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) | >>> >>
On 5/8/2023 4:27 PM, Dmitry Baryshkov wrote: > On 08/05/2023 23:09, Abhinav Kumar wrote: >> >> >> On 5/3/2023 1:26 AM, Dmitry Baryshkov wrote: >>> On 03/05/2023 04:19, Jessica Zhang wrote: >>>> Currently, word count is calculated using slice_count. This is >>>> incorrect >>>> as downstream uses slice per packet, which is different from >>>> 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 word count calculation. >>>> >>>> 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 | 9 ++++++++- >>>> 1 file changed, 8 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c >>>> b/drivers/gpu/drm/msm/dsi/dsi_host.c >>>> index 35c69dbe5f6f..b0d448ffb078 100644 >>>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c >>>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c >>>> @@ -996,7 +996,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 drm_panel struct to hold >>>> slice_per_packet info >>>> + * and adjust DSC math to account for >>>> slice_per_packet. >>> >>> slice_per_packet is not a part of the standard DSC, so I'm not sure >>> how that can be implemented. And definitely we should not care about >>> the drm_panel here. It should be either a part of drm_dsc_config, or >>> mipi_dsi_device. >>> >> >> This is not correct. >> >> It is part of the DSI standard (not DSC standard). Please refer to >> Figure 40 "One Line Containing One Packet with Data from One or More >> Compressed Slices" and Figure 41 "One Line Containing More than One >> Compressed Pixel Stream Packet". > > I have reviewed section 8.8.24 and Annex D of the DSI standard. > > It is not clear to me, if we can get away with always using > slice_per_packet = 1. What is the DSI sink's difference between Fig. > 40.(b) and Fig 41? > The difference is that in fig 40(b) there is only one packet of data (check closely, there is only one header). In fig 41, there are multiple headers so its showing multiple packets. > Are there are known panels that require slice_per_packet != 1? If so, we > will have to implement support for such configurations. > Unless explicitly requested by the panel, we can use 1. From the device tree files of the panels we support downstream, I do see qcom,mdss-dsc-slice-per-pkt set to 2 for some panels. I dont know whether those panels will not work with 1. I really don't think any of the DSC panels working with MSM were upstreamed. I think the one jessica will be posting (and merging) will be the first and that works with 1. If there are other panels in the works which require 2 slice_per_pkt, I would wait to first see them on the list and if they cannot work with 1 slice_per_pkt, add support for that. >> This has details about this. So I still stand by my point that this >> should be in the drm_panel. > > Note, the driver doesn't use drm_panel directly. So slices_per_packet > should go to mipi_dsi_device instead (which in turn can be filled from > e.g. drm_panel or from any other source). > >> >>>> + */ >>>> + 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) | >>>> >>> >
On 5/8/2023 5:47 PM, Dmitry Baryshkov wrote: > On 09/05/2023 03:45, Abhinav Kumar wrote: >> >> >> On 5/8/2023 4:27 PM, Dmitry Baryshkov wrote: >>> On 08/05/2023 23:09, Abhinav Kumar wrote: >>>> >>>> >>>> On 5/3/2023 1:26 AM, Dmitry Baryshkov wrote: >>>>> On 03/05/2023 04:19, Jessica Zhang wrote: >>>>>> Currently, word count is calculated using slice_count. This is >>>>>> incorrect >>>>>> as downstream uses slice per packet, which is different from >>>>>> 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 word count calculation. >>>>>> >>>>>> 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 | 9 ++++++++- >>>>>> 1 file changed, 8 insertions(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c >>>>>> b/drivers/gpu/drm/msm/dsi/dsi_host.c >>>>>> index 35c69dbe5f6f..b0d448ffb078 100644 >>>>>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c >>>>>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c >>>>>> @@ -996,7 +996,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 drm_panel struct to hold >>>>>> slice_per_packet info >>>>>> + * and adjust DSC math to account for >>>>>> slice_per_packet. >>>>> >>>>> slice_per_packet is not a part of the standard DSC, so I'm not sure >>>>> how that can be implemented. And definitely we should not care >>>>> about the drm_panel here. It should be either a part of >>>>> drm_dsc_config, or mipi_dsi_device. >>>>> >>>> >>>> This is not correct. >>>> >>>> It is part of the DSI standard (not DSC standard). Please refer to >>>> Figure 40 "One Line Containing One Packet with Data from One or More >>>> Compressed Slices" and Figure 41 "One Line Containing More than One >>>> Compressed Pixel Stream Packet". >>> >>> I have reviewed section 8.8.24 and Annex D of the DSI standard. >>> >>> It is not clear to me, if we can get away with always using >>> slice_per_packet = 1. What is the DSI sink's difference between Fig. >>> 40.(b) and Fig 41? >>> >> >> The difference is that in fig 40(b) there is only one packet of data >> (check closely, there is only one header). >> >> In fig 41, there are multiple headers so its showing multiple packets. > > Yes, this is the description of the pictures. I mean what is the > functional difference? > From the same section of the spec: "This is one method that can segregate slice-widths when the receiver has multiple instantiations of decoders and this packet structure is used to identify slice-width boundaries" From this it seems like when there are multiple decoders then having individual multiple packets helps to identify the boundaries correctly due to the header/checksum boundaries. How exactly this works on the panel side, needs more investigation but outside the boundary of this discussion. But I think overall, unless really requested by the panel one slice_per_pkt is more optimal as it reduces packet overhead. >> >>> Are there are known panels that require slice_per_packet != 1? If so, >>> we will have to implement support for such configurations. >>> >> >> Unless explicitly requested by the panel, we can use 1. From the >> device tree files of the panels we support downstream, I do see >> qcom,mdss-dsc-slice-per-pkt set to 2 for some panels. I dont know >> whether those panels will not work with 1. I really don't think any of >> the DSC panels working with MSM were upstreamed. >> >> I think the one jessica will be posting (and merging) will be the >> first and that works with 1. >> >> If there are other panels in the works which require 2 slice_per_pkt, >> I would wait to first see them on the list and if they cannot work >> with 1 slice_per_pkt, add support for that. > > If slice_per_pkt change is localized - touching only few lines in DSI or > in msm_helpers. Otherwise we should consider having that from the > beginning (but hardcoded to 1 for now). > Yes, from what I see downstream it touches only few lines in DSI and MSM helper. So should not be hard from MSM standpoint even in future, but the core changes would need agreement from the core maintainers. >> >>>> This has details about this. So I still stand by my point that this >>>> should be in the drm_panel. >>> >>> Note, the driver doesn't use drm_panel directly. So slices_per_packet >>> should go to mipi_dsi_device instead (which in turn can be filled >>> from e.g. drm_panel or from any other source). >>> >>>> >>>>>> + */ >>>>>> + 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) | >>>>>> >>>>> >>> >
On 9.05.2023 01:27, Dmitry Baryshkov wrote: > On 08/05/2023 23:09, Abhinav Kumar wrote: >> >> >> On 5/3/2023 1:26 AM, Dmitry Baryshkov wrote: >>> On 03/05/2023 04:19, Jessica Zhang wrote: >>>> Currently, word count is calculated using slice_count. This is incorrect >>>> as downstream uses slice per packet, which is different from >>>> 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 word count calculation. >>>> >>>> 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 | 9 ++++++++- >>>> 1 file changed, 8 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c >>>> index 35c69dbe5f6f..b0d448ffb078 100644 >>>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c >>>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c >>>> @@ -996,7 +996,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 drm_panel struct to hold slice_per_packet info >>>> + * and adjust DSC math to account for slice_per_packet. >>> >>> slice_per_packet is not a part of the standard DSC, so I'm not sure how that can be implemented. And definitely we should not care about the drm_panel here. It should be either a part of drm_dsc_config, or mipi_dsi_device. >>> >> >> This is not correct. >> >> It is part of the DSI standard (not DSC standard). Please refer to Figure 40 "One Line Containing One Packet with Data from One or More Compressed Slices" and Figure 41 "One Line Containing More than One Compressed Pixel Stream Packet". > > I have reviewed section 8.8.24 and Annex D of the DSI standard. > > It is not clear to me, if we can get away with always using slice_per_packet = 1. What is the DSI sink's difference between Fig. 40.(b) and Fig 41? > > Are there are known panels that require slice_per_packet != 1? If so, we will have to implement support for such configurations. At least two different ones on expensive Xperias (souxp00_a+amb650wh07 and sofef03_m) Konrad > >> This has details about this. So I still stand by my point that this should be in the drm_panel. > > Note, the driver doesn't use drm_panel directly. So slices_per_packet should go to mipi_dsi_device instead (which in turn can be filled from e.g. drm_panel or from any other source). > >> >>>> + */ >>>> + 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) | >>>> >>> >
On 09/05/2023 01:27, Dmitry Baryshkov wrote: > On 08/05/2023 23:09, Abhinav Kumar wrote: >> >> >> On 5/3/2023 1:26 AM, Dmitry Baryshkov wrote: >>> On 03/05/2023 04:19, Jessica Zhang wrote: >>>> Currently, word count is calculated using slice_count. This is incorrect >>>> as downstream uses slice per packet, which is different from >>>> 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 word count calculation. >>>> >>>> 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 | 9 ++++++++- >>>> 1 file changed, 8 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c >>>> index 35c69dbe5f6f..b0d448ffb078 100644 >>>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c >>>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c >>>> @@ -996,7 +996,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 drm_panel struct to hold slice_per_packet info >>>> + * and adjust DSC math to account for slice_per_packet. >>> >>> slice_per_packet is not a part of the standard DSC, so I'm not sure how that can be implemented. And definitely we should not care about the drm_panel here. It should be either a part of drm_dsc_config, or mipi_dsi_device. >>> >> >> This is not correct. >> >> It is part of the DSI standard (not DSC standard). Please refer to Figure 40 "One Line Containing One Packet with Data from One or More Compressed Slices" and Figure 41 "One Line Containing More than One Compressed Pixel Stream Packet". > > I have reviewed section 8.8.24 and Annex D of the DSI standard. > > It is not clear to me, if we can get away with always using slice_per_packet = 1. What is the DSI sink's difference between Fig. 40.(b) and Fig 41? > > Are there are known panels that require slice_per_packet != 1? If so, we will have to implement support for such configurations. > >> This has details about this. So I still stand by my point that this should be in the drm_panel. > > Note, the driver doesn't use drm_panel directly. So slices_per_packet should go to mipi_dsi_device instead (which in turn can be filled from e.g. drm_panel or from any other source). This is a big question, where should we set those parameters ? It's an even bigger questions for panels optionally supporting DSC in Video or Command mode (like the vtdr6130), how to select DSC or not ? DT is not an option. Those should tied to a panel+controller tuple. Neil > >> >>>> + */ >>>> + 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) | >>>> >>> >
On 09/05/2023 11:54, Konrad Dybcio wrote: > > > On 9.05.2023 10:23, Neil Armstrong wrote: >> On 09/05/2023 01:27, Dmitry Baryshkov wrote: >>> On 08/05/2023 23:09, Abhinav Kumar wrote: >>>> >>>> >>>> On 5/3/2023 1:26 AM, Dmitry Baryshkov wrote: >>>>> On 03/05/2023 04:19, Jessica Zhang wrote: >>>>>> Currently, word count is calculated using slice_count. This is incorrect >>>>>> as downstream uses slice per packet, which is different from >>>>>> 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 word count calculation. >>>>>> >>>>>> 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 | 9 ++++++++- >>>>>> 1 file changed, 8 insertions(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c >>>>>> index 35c69dbe5f6f..b0d448ffb078 100644 >>>>>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c >>>>>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c >>>>>> @@ -996,7 +996,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 drm_panel struct to hold slice_per_packet info >>>>>> + * and adjust DSC math to account for slice_per_packet. >>>>> >>>>> slice_per_packet is not a part of the standard DSC, so I'm not sure how that can be implemented. And definitely we should not care about the drm_panel here. It should be either a part of drm_dsc_config, or mipi_dsi_device. >>>>> >>>> >>>> This is not correct. >>>> >>>> It is part of the DSI standard (not DSC standard). Please refer to Figure 40 "One Line Containing One Packet with Data from One or More Compressed Slices" and Figure 41 "One Line Containing More than One Compressed Pixel Stream Packet". >>> >>> I have reviewed section 8.8.24 and Annex D of the DSI standard. >>> >>> It is not clear to me, if we can get away with always using slice_per_packet = 1. What is the DSI sink's difference between Fig. 40.(b) and Fig 41? >>> >>> Are there are known panels that require slice_per_packet != 1? If so, we will have to implement support for such configurations. >>> >>>> This has details about this. So I still stand by my point that this should be in the drm_panel. >>> >>> Note, the driver doesn't use drm_panel directly. So slices_per_packet should go to mipi_dsi_device instead (which in turn can be filled from e.g. drm_panel or from any other source). >> >> This is a big question, where should we set those parameters ? >> >> It's an even bigger questions for panels optionally supporting DSC in Video or Command mode (like the vtdr6130), >> how to select DSC or not ? DT is not an option. > Compressed vs uncompressed modes, maybe? Would be nice to make this > togglable from userspace.. But then it may not scale for panels with e.g. > 10 resolutions, all cmd/vid/dsc/nodsc Currently the panel/panel-bridge make decision on command vs video mode. We have no way to influence that decision. If you want to make that negotiable, I'd start with adding 'cmd_supported/video_supported/dsc_supported' flags to struct mipi_dsi_hosts. > > > Konrad >> >> Those should tied to a panel+controller tuple. >> >> Neil >> >>> >>>> >>>>>> + */ >>>>>> + 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) | >>>>>> >>>>> >>> >>
On 5/9/2023 11:02 AM, Abhinav Kumar wrote: > > > On 5/9/2023 4:42 AM, Dmitry Baryshkov wrote: >> On 09/05/2023 11:54, Konrad Dybcio wrote: >>> >>> >>> On 9.05.2023 10:23, Neil Armstrong wrote: >>>> On 09/05/2023 01:27, Dmitry Baryshkov wrote: >>>>> On 08/05/2023 23:09, Abhinav Kumar wrote: >>>>>> >>>>>> >>>>>> On 5/3/2023 1:26 AM, Dmitry Baryshkov wrote: >>>>>>> On 03/05/2023 04:19, Jessica Zhang wrote: >>>>>>>> Currently, word count is calculated using slice_count. This is >>>>>>>> incorrect >>>>>>>> as downstream uses slice per packet, which is different from >>>>>>>> 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 word count calculation. >>>>>>>> >>>>>>>> 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 | 9 ++++++++- >>>>>>>> 1 file changed, 8 insertions(+), 1 deletion(-) >>>>>>>> >>>>>>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c >>>>>>>> b/drivers/gpu/drm/msm/dsi/dsi_host.c >>>>>>>> index 35c69dbe5f6f..b0d448ffb078 100644 >>>>>>>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c >>>>>>>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c >>>>>>>> @@ -996,7 +996,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 drm_panel struct to hold >>>>>>>> slice_per_packet info >>>>>>>> + * and adjust DSC math to account for >>>>>>>> slice_per_packet. >>>>>>> >>>>>>> slice_per_packet is not a part of the standard DSC, so I'm not >>>>>>> sure how that can be implemented. And definitely we should not >>>>>>> care about the drm_panel here. It should be either a part of >>>>>>> drm_dsc_config, or mipi_dsi_device. >>>>>>> >>>>>> >>>>>> This is not correct. >>>>>> >>>>>> It is part of the DSI standard (not DSC standard). Please refer to >>>>>> Figure 40 "One Line Containing One Packet with Data from One or >>>>>> More Compressed Slices" and Figure 41 "One Line Containing More >>>>>> than One Compressed Pixel Stream Packet". >>>>> >>>>> I have reviewed section 8.8.24 and Annex D of the DSI standard. >>>>> >>>>> It is not clear to me, if we can get away with always using >>>>> slice_per_packet = 1. What is the DSI sink's difference between >>>>> Fig. 40.(b) and Fig 41? >>>>> >>>>> Are there are known panels that require slice_per_packet != 1? If >>>>> so, we will have to implement support for such configurations. >>>>> >>>>>> This has details about this. So I still stand by my point that >>>>>> this should be in the drm_panel. >>>>> >>>>> Note, the driver doesn't use drm_panel directly. So >>>>> slices_per_packet should go to mipi_dsi_device instead (which in >>>>> turn can be filled from e.g. drm_panel or from any other source). >>>> >>>> This is a big question, where should we set those parameters ? >>>> >>>> It's an even bigger questions for panels optionally supporting DSC >>>> in Video or Command mode (like the vtdr6130), >>>> how to select DSC or not ? DT is not an option. >>> Compressed vs uncompressed modes, maybe? Would be nice to make this >>> togglable from userspace.. But then it may not scale for panels with >>> e.g. >>> 10 resolutions, all cmd/vid/dsc/nodsc >> >> Currently the panel/panel-bridge make decision on command vs video >> mode. We have no way to influence that decision. If you want to make >> that negotiable, I'd start with adding >> 'cmd_supported/video_supported/dsc_supported' flags to struct >> mipi_dsi_hosts. >> > > Right. Isn't that issue there even today that if a panel supports DSC in > only one of the modes, we have no way to tell that as we have not > encountered such a panel in upstream yet. > > Also, fundamental question to folks who had panels requiring > slice_per_pkt as 2, > > if you had some panels which need a slice_per_pkt as 2, this support > could have been added even earlier by someone who had these panels even > in DSC 1.1. > > If these panels are not yet upstreamed, may i please know why this is > considered as a "breakage"? If they were working "somehow" due to > incorrect math / panel settings / DPU calculations, unfortunately we > have to work towards bringing up these panels properly and upstreaming > them rather than saying "oh, these panels were working somehow and now > we need to keep it working". I also want to add on top of Abhinav's point: Currently, the panel I'm testing this series on only uses slice_per_pkt=1, so I have no way to testing slice_per_pkt > 1. So in the case where we add support for slice_per_pkt > 1, I would prefer that be as a separate series as I would not be able to validate those changes on my current setup. Thanks, Jessica Zhang > >>> >>> >>> Konrad >>>> >>>> Those should tied to a panel+controller tuple. >>>> >>>> Neil >>>> >>>>> >>>>>> >>>>>>>> + */ >>>>>>>> + 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) | >>>>>>>> >>>>>>> >>>>> >>>> >>
diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c index 35c69dbe5f6f..b0d448ffb078 100644 --- a/drivers/gpu/drm/msm/dsi/dsi_host.c +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c @@ -996,7 +996,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 drm_panel 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, word count is calculated using slice_count. This is incorrect as downstream uses slice per packet, which is different from 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 word count calculation. 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 | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)