Messages in this thread | | | Date | Tue, 9 May 2023 13:25:17 -0700 | Subject | Re: [Freedreno] [PATCH 2/4] drm/msm/dsi: Fix compressed word count calculation | From | Jessica Zhang <> |
| |
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) | >>>>>>>> >>>>>>> >>>>> >>>> >>
| |