[Freedreno] [PATCH RFC 5/5] drm/msm/dsi: Use MSM and DRM DSC helper methods
Dmitry Baryshkov
dmitry.baryshkov at linaro.org
Fri Mar 31 02:47:10 UTC 2023
On 31/03/2023 04:33, Abhinav Kumar wrote:
>
>
> On 3/30/2023 5:16 PM, Dmitry Baryshkov wrote:
>> On Fri, 31 Mar 2023 at 03:07, Jessica Zhang
>> <quic_jesszhan at quicinc.com> wrote:
>>>
>>>
>>>
>>> On 3/30/2023 4:14 PM, Dmitry Baryshkov wrote:
>>>> On 31/03/2023 01:49, Jessica Zhang wrote:
>>>>>
>>>>>
>>>>> On 3/29/2023 4:48 PM, Dmitry Baryshkov wrote:
>>>>>> On 30/03/2023 02:18, Jessica Zhang wrote:
>>>>>>> Use MSM and DRM DSC helper methods.
>>>>>>>
>>>>>>> Signed-off-by: Jessica Zhang <quic_jesszhan at quicinc.com>
>>>>>>> ---
>>>>>>> drivers/gpu/drm/msm/dsi/dsi_host.c | 18 ++++++++++++------
>>>>>>> 1 file changed, 12 insertions(+), 6 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>>>>> b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>>>>> index 74d38f90398a..7419fe58a941 100644
>>>>>>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>>>>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>>>>> @@ -31,6 +31,7 @@
>>>>>>> #include "msm_kms.h"
>>>>>>> #include "msm_gem.h"
>>>>>>> #include "phy/dsi_phy.h"
>>>>>>> +#include "disp/msm_dsc_helper.h"
>>>>>>> #define DSI_RESET_TOGGLE_DELAY_MS 20
>>>>>>> @@ -841,14 +842,14 @@ static void dsi_update_dsc_timing(struct
>>>>>>> msm_dsi_host *msm_host, bool is_cmd_mod
>>>>>>> {
>>>>>>> struct drm_dsc_config *dsc = msm_host->dsc;
>>>>>>> u32 reg, reg_ctrl, reg_ctrl2;
>>>>>>> - u32 slice_per_intf, total_bytes_per_intf;
>>>>>>> + u32 slice_per_intf;
>>>>>>> u32 pkt_per_line;
>>>>>>> u32 eol_byte_num;
>>>>>>> /* first calculate dsc parameters and then program
>>>>>>> * compress mode registers
>>>>>>> */
>>>>>>> - slice_per_intf = DIV_ROUND_UP(hdisplay, dsc->slice_width);
>>>>>>> + slice_per_intf = msm_dsc_get_slice_per_intf(dsc, hdisplay);
>>>>>>
>>>>>> This looks good
>>>>>>
>>>>>>> /*
>>>>>>> * If slice_count is greater than slice_per_intf
>>>>>>> @@ -858,10 +859,10 @@ static void dsi_update_dsc_timing(struct
>>>>>>> msm_dsi_host *msm_host, bool is_cmd_mod
>>>>>>> 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 = msm_dsc_get_eol_byte_num(msm_host->dsc,
>>>>>>> hdisplay,
>>>>>>> + dsi_get_bpp(msm_host->format));
>>>>>>> - eol_byte_num = total_bytes_per_intf % 3;
>>>>>>> - pkt_per_line = slice_per_intf / dsc->slice_count;
>>>>>>> + pkt_per_line = slice_per_intf / MSM_DSC_SLICE_PER_PKT;
>>>>>>
>>>>>> And for these values the result is definitely changed. Separate patch
>>>>>> & description please. Just in case, "values per downstream kernel" is
>>>>>> not a proper description for such changes.
>>>>>
>>>>> Hi Dmitry,
>>>>>
>>>>> Sure, I can put this into a separate patch.
>>>>>
>>>>> The reason this was changed from slice_count to SLICE_PER_PKT was
>>>>> because slice count and slice per packet aren't always equivalent.
>>>>> There can be cases where panel configures DSC to have multiple soft
>>>>> slices per interface, but the panel only specifies 1 slice per packet.
>>>>
>>>> Please put this nice description into the commit message. It is exactly
>>>> what I was looking for!
>>>>
>>>> BTW: Do you expect to change MSM_DSC_SLICE_PER_PKT later or it will
>>>> stay
>>>> at "1"? If so, it might be easier to drop it and instead add a comment.
>>>
>>> MSM_DSC_SLICE_PER_PKT is the default value for panels that don't specify
>>> a slice_per_pkt value. (Now that I think about it, might be better to
>>> call it MSM_DSC_DEFAULT_SLICE_PER_PKT instead...)
>>
>> Note, there is no slice_per_pkt in drm_dsc_config, so we must come up
>> with another way to pass this data from the panel or to deduce the
>> value in our driver.
>>
>>>
>>> I don't expect it to change in the future, but it's a little more
>>> readable than just dividing by 1 IMO. If you prefer dropping the macro
>>> and adding a comment, I'm also okay with that.
>>
>> There is no need to divide by 1, the value doesn't change. So I'd
>> probably prefer something like:
>>
>> /* Default to 1 slice per packet */
>> if (panel_slice_per_pkt)
>> pkt_per_line = slice_per_intf / panel_slice_per_pkt;
>> else
>> pkt_per_line = slice_per_intf;
>>
>> Or:
>>
>> /* Default to 1 slice per packet */
>> slice_per_pkt = 1;
>> if (panel_slice_per_pkt)
>> slice_per_pkt = panel_slice_per_pkt;
>> pkt_per_line = slice_per_intf / slice_per_pkt;
>>
>> BTW: could you possibly change 'intf' to 'line' to v2? It seems there
>> is a mixture of them through the code. If there is a difference
>> between intf and line which is not yet posted, it's fine to keep the
>> current code. WDYT?
>>
>
> No, I dont agree with the change from intf to line.
>
> In case of dual DSI, intf is not equal to line.
>
> 2 intfs = 1 line
>
> Hence that distinction is necessary.
Ack, this is what I was looking for!
so intf = line / num_intf?
Maybe I should explain the reason for my question:
msm_dsc_get_pclk_per_line() uses intf_width, calculates pclk_per_line
(not per intf). msm_dsc_get_dce_bytes_per_line() does the same thing
In this patch we take slice_per_intf, divide it with slice_per_pkt and
get pkt_per_line (rather than pkt_per_intf).
This is what prompted my question regarding intf vs line.
>
>>>
>>> Thanks,
>>>
>>> Jessica Zhang
>>>
>>>>
>>>> Regarding eol_byte_num, probably the best explanation would be that is
>>>> is a size of a padding rather than a size of a trailer bytes in a line
>>>> (and thus original calculation was incorrect).
>>>>
>>>>>
>>>>>>
>>>>>>> if (is_cmd_mode) /* packet data type */
>>>>>>> reg =
>>>>>>> DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE(MIPI_DSI_DCS_LONG_WRITE);
>>>>>>> @@ -911,6 +912,11 @@ static void dsi_timing_setup(struct
>>>>>>> msm_dsi_host *msm_host, bool is_bonded_dsi)
>>>>>>> DBG("");
>>>>>>> + if (msm_host->mode_flags & MIPI_DSI_MODE_VIDEO)
>>>>>>> + /* Default widebus_en to false for now. */
>>>>>>> + hdisplay = msm_dsc_get_pclk_per_line(msm_host->dsc,
>>>>>>> mode->hdisplay,
>>>>>>> + dsi_get_bpp(msm_host->format));
>>>>>>> +
>>>>>>
>>>>>> This is definitely something new and thus should probably go into a
>>>>>> separate patch and be described. Also I'm not sure how does that
>>>>>> interact with the hdisplay-related calculations below, under the
>>>>>> if(dsc) clause.
>>>>>
>>>>> After double-checking the math here, I think this part of the change
>>>>> is actually wrong. pclk_per_line is essentially doing hdisplay / 3,
>>>>> which is a repeat of what's being done in the `if (dsc)` block.
>>>>>
>>>>> Will replace `hdisplay /= 3` with the pclk_per_line calculation.
>>>>
>>>> Thanks!
>>>>
>>>>>
>>>>> Thanks,
>>>>>
>>>>> Jessica Zhang
>>>>>
>>>>>>
>>>>>>> /*
>>>>>>> * For bonded DSI mode, the current DRM mode has
>>>>>>> * the complete width of the panel. Since, the complete
>>>>>>> @@ -1759,7 +1765,7 @@ static int dsi_populate_dsc_params(struct
>>>>>>> msm_dsi_host *msm_host, struct drm_dsc
>>>>>>> return ret;
>>>>>>> }
>>>>>>> - dsc->initial_scale_value = 32;
>>>>>>> + dsc->initial_scale_value =
>>>>>>> drm_dsc_calculate_initial_scale_value(dsc);
>>>>>>
>>>>>> This is fine, we only support 8bpp where these values match.
>>>>>>
>>>>>>> dsc->line_buf_depth = dsc->bits_per_component + 1;
>>>>>>> return drm_dsc_compute_rc_parameters(dsc);
>>>>>>>
>>>>>>
>>>>>> --
>>>>>> With best wishes
>>>>>> Dmitry
>>>>>>
>>>>
>>>> --
>>>> With best wishes
>>>> Dmitry
>>>>
>>
>>
>>
--
With best wishes
Dmitry
More information about the dri-devel
mailing list