From: "Nautiyal, Ankit K" <ankit.k.nautiyal@intel.com>
To: "Kandpal, Suraj" <suraj.kandpal@intel.com>,
"intel-gfx@lists.freedesktop.org"
<intel-gfx@lists.freedesktop.org>
Subject: Re: [Intel-gfx] [PATCH v3 4/7] drm/i915/vdsc: Use MACRO to cleanup intel_dsc_get_pps_reg
Date: Tue, 18 Jul 2023 09:43:23 +0000 [thread overview]
Message-ID: <33dbc867-2351-45b4-a54f-12e316b50d53@intel.com> (raw)
In-Reply-To: <6476b61a-79a5-9f2b-0b59-48f950e20ce1@intel.com>
On 7/18/2023 2:54 PM, Nautiyal, Ankit K wrote:
> On 7/17/2023 3:39 PM, Suraj Kandpal wrote:
>> Use a Macro to clean up intel_dsc_get_pps_reg so that we don't have
>> to replicate so many if/else blocks.
>>
>> Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
>> ---
>> drivers/gpu/drm/i915/display/intel_vdsc.c | 124 ++++++----------------
>> 1 file changed, 32 insertions(+), 92 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_vdsc.c b/drivers/gpu/drm/i915/display/intel_vdsc.c
>> index 6d319f351a12..ed8fda431226 100644
>> --- a/drivers/gpu/drm/i915/display/intel_vdsc.c
>> +++ b/drivers/gpu/drm/i915/display/intel_vdsc.c
>> @@ -313,131 +313,71 @@ static void intel_dsc_get_pps_reg(struct intel_crtc_state *crtc_state, int pps,
>>
>> pipe_dsc = is_pipe_dsc(crtc, cpu_transcoder);
>>
>> +#define PRE_MTL_GET_DSC_REGISTER(pps, is_pipe_dsc, pipe) do { \
>> + if (is_pipe_dsc) { \
>> + *dsc_reg0 = ICL_DSC0_PICTURE_PARAMETER_SET_##pps(pipe); \
>> + *dsc_reg1 = ICL_DSC1_PICTURE_PARAMETER_SET_##pps(pipe); \
>> + } else { \
>> + *dsc_reg0 = DSCA_PICTURE_PARAMETER_SET_##pps; \
>> + *dsc_reg1 = DSCC_PICTURE_PARAMETER_SET_##pps; \
> I think it will be more efficient to add an enum intel_dsc_engine and
> have a function that returns the reg for given, pps, engine and pipe.
>
> something like:
>
> static i915_reg_t intel_dsc_get_pps_reg(const struct intel_crtc_state
> *crtc_state, int pps, enum vdsc_engine vdsc)
>
> Since offsets are regularly placed, we can have something like :
> ICL_DSC_PICTURE_PARAMETER_SET(engine, pps, pipe) and
>
> DSC_PICTURE_PARAMETER_SET(engine, pps) for ICL EDP/DSI case.
>
> Roughly it will be Offset of PPS Reg for Engine 0 + engine * 0x800 + pps
> * 4.
Sorry this is incorrect example. Need to get the difference, for engine
and pps to calculate the offset correctly.
Regards,
Ankit
>
> Though this will require quite some changes in reg macros in
> intel_vdsc_reg.h ,and also need to have a new enum for intel_dsc_engine,
>
> but IMHO it will be easier to get the required PPS reg this way.
>
> Regards,
>
> Ankit
>
>
>> + } \
>> +} while (0)
>> +
>> +#define MTL_GET_DSC_REGISTER(pps, pipe) do { \
>> + *dsc_reg0 = MTL_DSC0_PICTURE_PARAMETER_SET_##pps(pipe); \
>> + *dsc_reg1 = MTL_DSC1_PICTURE_PARAMETER_SET_##pps(pipe); \
>> +} while (0)
>> +
>> switch (pps) {
>> case 0:
>> - if (pipe_dsc) {
>> - *dsc_reg0 = ICL_DSC0_PICTURE_PARAMETER_SET_0(pipe);
>> - *dsc_reg1 = ICL_DSC1_PICTURE_PARAMETER_SET_0(pipe);
>> - } else {
>> - *dsc_reg0 = DSCA_PICTURE_PARAMETER_SET_0;
>> - *dsc_reg1 = DSCC_PICTURE_PARAMETER_SET_0;
>> - }
>> + PRE_MTL_GET_DSC_REGISTER(0, pipe_dsc, pipe);
>> break;
>> case 1:
>> - if (pipe_dsc) {
>> - *dsc_reg0 = ICL_DSC0_PICTURE_PARAMETER_SET_1(pipe);
>> - *dsc_reg1 = ICL_DSC1_PICTURE_PARAMETER_SET_1(pipe);
>> - } else {
>> - *dsc_reg0 = DSCA_PICTURE_PARAMETER_SET_1;
>> - *dsc_reg1 = DSCC_PICTURE_PARAMETER_SET_1;
>> - }
>> + PRE_MTL_GET_DSC_REGISTER(1, pipe_dsc, pipe);
>> break;
>> case 2:
>> - if (pipe_dsc) {
>> - *dsc_reg0 = ICL_DSC0_PICTURE_PARAMETER_SET_2(pipe);
>> - *dsc_reg1 = ICL_DSC1_PICTURE_PARAMETER_SET_2(pipe);
>> - } else {
>> - *dsc_reg0 = DSCA_PICTURE_PARAMETER_SET_2;
>> - *dsc_reg1 = DSCC_PICTURE_PARAMETER_SET_2;
>> - }
>> + PRE_MTL_GET_DSC_REGISTER(2, pipe_dsc, pipe);
>> break;
>> case 3:
>> - if (pipe_dsc) {
>> - *dsc_reg0 = ICL_DSC0_PICTURE_PARAMETER_SET_3(pipe);
>> - *dsc_reg1 = ICL_DSC1_PICTURE_PARAMETER_SET_3(pipe);
>> - } else {
>> - *dsc_reg0 = DSCA_PICTURE_PARAMETER_SET_3;
>> - *dsc_reg1 = DSCC_PICTURE_PARAMETER_SET_3;
>> - }
>> + PRE_MTL_GET_DSC_REGISTER(3, pipe_dsc, pipe);
>> break;
>> case 4:
>> - if (pipe_dsc) {
>> - *dsc_reg0 = ICL_DSC0_PICTURE_PARAMETER_SET_4(pipe);
>> - *dsc_reg1 = ICL_DSC1_PICTURE_PARAMETER_SET_4(pipe);
>> - } else {
>> - *dsc_reg0 = DSCA_PICTURE_PARAMETER_SET_4;
>> - *dsc_reg1 = DSCC_PICTURE_PARAMETER_SET_4;
>> - }
>> + PRE_MTL_GET_DSC_REGISTER(4, pipe_dsc, pipe);
>> break;
>> case 5:
>> - if (pipe_dsc) {
>> - *dsc_reg0 = ICL_DSC0_PICTURE_PARAMETER_SET_5(pipe);
>> - *dsc_reg1 = ICL_DSC1_PICTURE_PARAMETER_SET_5(pipe);
>> - } else {
>> - *dsc_reg0 = DSCA_PICTURE_PARAMETER_SET_5;
>> - *dsc_reg1 = DSCC_PICTURE_PARAMETER_SET_5;
>> - }
>> + PRE_MTL_GET_DSC_REGISTER(5, pipe_dsc, pipe);
>> break;
>> case 6:
>> - if (pipe_dsc) {
>> - *dsc_reg0 = ICL_DSC0_PICTURE_PARAMETER_SET_6(pipe);
>> - *dsc_reg1 = ICL_DSC1_PICTURE_PARAMETER_SET_6(pipe);
>> - } else {
>> - *dsc_reg0 = DSCA_PICTURE_PARAMETER_SET_6;
>> - *dsc_reg1 = DSCC_PICTURE_PARAMETER_SET_6;
>> - }
>> + PRE_MTL_GET_DSC_REGISTER(6, pipe_dsc, pipe);
>> break;
>> case 7:
>> - if (pipe_dsc) {
>> - *dsc_reg0 = ICL_DSC0_PICTURE_PARAMETER_SET_7(pipe);
>> - *dsc_reg1 = ICL_DSC1_PICTURE_PARAMETER_SET_7(pipe);
>> - } else {
>> - *dsc_reg0 = DSCA_PICTURE_PARAMETER_SET_7;
>> - *dsc_reg1 = DSCC_PICTURE_PARAMETER_SET_7;
>> - }
>> + PRE_MTL_GET_DSC_REGISTER(7, pipe_dsc, pipe);
>> break;
>> case 8:
>> - if (pipe_dsc) {
>> - *dsc_reg0 = ICL_DSC0_PICTURE_PARAMETER_SET_8(pipe);
>> - *dsc_reg1 = ICL_DSC1_PICTURE_PARAMETER_SET_8(pipe);
>> - } else {
>> - *dsc_reg0 = DSCA_PICTURE_PARAMETER_SET_8;
>> - *dsc_reg1 = DSCC_PICTURE_PARAMETER_SET_8;
>> - }
>> + PRE_MTL_GET_DSC_REGISTER(8, pipe_dsc, pipe);
>> break;
>> case 9:
>> - if (pipe_dsc) {
>> - *dsc_reg0 = ICL_DSC0_PICTURE_PARAMETER_SET_9(pipe);
>> - *dsc_reg1 = ICL_DSC1_PICTURE_PARAMETER_SET_9(pipe);
>> - } else {
>> - *dsc_reg0 = DSCA_PICTURE_PARAMETER_SET_9;
>> - *dsc_reg1 = DSCC_PICTURE_PARAMETER_SET_9;
>> - }
>> + PRE_MTL_GET_DSC_REGISTER(9, pipe_dsc, pipe);
>> break;
>> case 10:
>> - if (pipe_dsc) {
>> - *dsc_reg0 = ICL_DSC0_PICTURE_PARAMETER_SET_10(pipe);
>> - *dsc_reg1 = ICL_DSC1_PICTURE_PARAMETER_SET_10(pipe);
>> - } else {
>> - *dsc_reg0 = DSCA_PICTURE_PARAMETER_SET_10;
>> - *dsc_reg1 = DSCC_PICTURE_PARAMETER_SET_10;
>> - }
>> + PRE_MTL_GET_DSC_REGISTER(10, pipe_dsc, pipe);
>> break;
>> case 16:
>> - if (pipe_dsc) {
>> - *dsc_reg0 = ICL_DSC0_PICTURE_PARAMETER_SET_16(pipe);
>> - *dsc_reg1 = ICL_DSC1_PICTURE_PARAMETER_SET_16(pipe);
>> - } else {
>> - *dsc_reg0 = DSCA_PICTURE_PARAMETER_SET_16;
>> - *dsc_reg1 = DSCC_PICTURE_PARAMETER_SET_16;
>> - }
>> + PRE_MTL_GET_DSC_REGISTER(16, pipe_dsc, pipe);
>> break;
>> - /*
>> - * Since PPS_17 and PPS_18 were introduced from MTL dsc check
>> - * need not be done
>> - */
>> case 17:
>> - *dsc_reg0 = MTL_DSC0_PICTURE_PARAMETER_SET_17(pipe);
>> - *dsc_reg1 = MTL_DSC1_PICTURE_PARAMETER_SET_17(pipe);
>> + MTL_GET_DSC_REGISTER(17, pipe);
>> break;
>> case 18:
>> - *dsc_reg0 = MTL_DSC0_PICTURE_PARAMETER_SET_18(pipe);
>> - *dsc_reg1 = MTL_DSC1_PICTURE_PARAMETER_SET_18(pipe);
>> + MTL_GET_DSC_REGISTER(18, pipe);
>> break;
>> default:
>> MISSING_CASE(pps);
>> break;
>> }
>> +
>> +#undef PRE_MTL_GET_DSC_REGISTER
>> +#undef MTL_GET_DSC_REGISTER
>> }
>>
>> static void intel_dsc_pps_configure(const struct intel_crtc_state *crtc_state)
next prev parent reply other threads:[~2023-07-18 9:43 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-07-17 10:09 [Intel-gfx] [PATCH v3 0/7] Add DSC PPS readout Suraj Kandpal
2023-07-17 10:09 ` [Intel-gfx] [PATCH v3 1/7] drm/i915/vdsc: Refactor dsc register field macro Suraj Kandpal
2023-07-17 10:09 ` [Intel-gfx] [PATCH v3 2/7] drm/i915/vdsc: Add a check for dsc split cases Suraj Kandpal
2023-07-17 10:09 ` [Intel-gfx] [PATCH v3 3/7] drm/i915/vdsc: Add function to read any PPS register Suraj Kandpal
2023-07-17 10:09 ` [Intel-gfx] [PATCH v3 4/7] drm/i915/vdsc: Use MACRO to cleanup intel_dsc_get_pps_reg Suraj Kandpal
2023-07-18 9:24 ` Nautiyal, Ankit K
2023-07-18 9:43 ` Nautiyal, Ankit K [this message]
2023-07-17 10:09 ` [Intel-gfx] [PATCH v3 5/7] drm/i915/vdsc: Add function to write in PPS register Suraj Kandpal
2023-07-17 10:09 ` [Intel-gfx] [PATCH v3 6/7] drm/i915/vdsc: Fill the intel_dsc_get_pps_config function Suraj Kandpal
2023-07-17 10:09 ` [Intel-gfx] [PATCH v3 7/7] drm/i915/display: Compare the readout dsc pps params Suraj Kandpal
2023-07-17 14:48 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for Add DSC PPS readout (rev3) Patchwork
2023-07-17 14:48 ` [Intel-gfx] ✗ Fi.CI.SPARSE: " Patchwork
2023-07-17 15:07 ` [Intel-gfx] ✗ Fi.CI.BAT: failure " Patchwork
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=33dbc867-2351-45b4-a54f-12e316b50d53@intel.com \
--to=ankit.k.nautiyal@intel.com \
--cc=intel-gfx@lists.freedesktop.org \
--cc=suraj.kandpal@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox