Re: [REPOST PATCH v4 07/13] drm/msm/disp/dpu1: Add support for DSC in encoder

[Vinod Koul <vkoul@kernel.org>]

On 17-02-22, 23:32, Marijn Suijten wrote:
> On 2022-02-10 16:04:17, Vinod Koul wrote:
> > We need to configure the encoder for DSC configuration and calculate DSC
> > parameters for the given timing so this patch adds that support by
> > adding dpu_encoder_prep_dsc() which is invoked when DSC is enabled.
> > 
> > Signed-off-by: Vinod Koul <vkoul@kernel.org>
> > ---
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c   | 164 +++++++++++++++++-
> >  .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h  |   8 +
> >  2 files changed, 171 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > index 1e648db439f9..95a7bf362e81 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > @@ -21,6 +21,7 @@
> >  #include "dpu_hw_intf.h"
> >  #include "dpu_hw_ctl.h"
> >  #include "dpu_hw_dspp.h"
> > +#include "dpu_hw_dsc.h"
> >  #include "dpu_formats.h"
> >  #include "dpu_encoder_phys.h"
> >  #include "dpu_crtc.h"
> > @@ -136,6 +137,8 @@ enum dpu_enc_rc_states {
> >   * @cur_slave:		As above but for the slave encoder.
> >   * @hw_pp:		Handle to the pingpong blocks used for the display. No.
> >   *			pingpong blocks can be different than num_phys_encs.
> > + * @hw_dsc:		Handle to the DSC blocks used for the display.
> > + * @dsc_mask:		The bitmask of used DSC blocks.
> 
> No need for "^The" prefix here.

Removed

> 
> >   * @intfs_swapped:	Whether or not the phys_enc interfaces have been swapped
> >   *			for partial update right-only cases, such as pingpong
> >   *			split where virtual pingpong does not generate IRQs
> > @@ -169,6 +172,7 @@ enum dpu_enc_rc_states {
> >   * @topology:                   topology of the display
> >   * @idle_timeout:		idle timeout duration in milliseconds
> >   * @dp:				msm_dp pointer, for DP encoders
> > + * @dsc:			msm_display_dsc_config pointer, for DSC-enabled encoders
> >   */
> >  struct dpu_encoder_virt {
> >  	struct drm_encoder base;
> > @@ -182,6 +186,9 @@ struct dpu_encoder_virt {
> >  	struct dpu_encoder_phys *cur_master;
> >  	struct dpu_encoder_phys *cur_slave;
> >  	struct dpu_hw_pingpong *hw_pp[MAX_CHANNELS_PER_ENC];
> > +	struct dpu_hw_dsc *hw_dsc[MAX_CHANNELS_PER_ENC];
> > +
> > +	unsigned int dsc_mask;
> >  
> >  	bool intfs_swapped;
> >  
> > @@ -209,6 +216,9 @@ struct dpu_encoder_virt {
> >  	u32 idle_timeout;
> >  
> >  	struct msm_dp *dp;
> > +
> > +	/* DSC configuration */
> > +	struct msm_display_dsc_config *dsc;
> >  };
> >  
> >  #define to_dpu_encoder_virt(x) container_of(x, struct dpu_encoder_virt, base)
> > @@ -972,7 +982,8 @@ static void dpu_encoder_virt_mode_set(struct drm_encoder *drm_enc,
> >  	struct dpu_hw_blk *hw_ctl[MAX_CHANNELS_PER_ENC];
> >  	struct dpu_hw_blk *hw_lm[MAX_CHANNELS_PER_ENC];
> >  	struct dpu_hw_blk *hw_dspp[MAX_CHANNELS_PER_ENC] = { NULL };
> > -	int num_lm, num_ctl, num_pp;
> > +	struct dpu_hw_blk *hw_dsc[MAX_CHANNELS_PER_ENC];
> > +	int num_lm, num_ctl, num_pp, num_dsc;
> >  	int i, j;
> >  
> >  	if (!drm_enc) {
> > @@ -1027,6 +1038,23 @@ static void dpu_encoder_virt_mode_set(struct drm_encoder *drm_enc,
> >  		dpu_enc->hw_pp[i] = i < num_pp ? to_dpu_hw_pingpong(hw_pp[i])
> >  						: NULL;
> >  
> > +	dpu_enc->dsc_mask = 0;
> > +
> > +	if (dpu_enc->dsc) {
> > +		unsigned int dsc_mask = 0;
> 
> Perhaps move this definition outside the if, and unconditionally
> perform:
> 
> 	dpu_enc->dsc_mask = dsc_mask;
> 
> Below the if, instead of having to write it twice when ->dsc is truthy?

That is a good suggestion, updated now

> 
> > +
> > +		num_dsc = dpu_rm_get_assigned_resources(&dpu_kms->rm, global_state,
> > +							drm_enc->base.id, DPU_HW_BLK_DSC,
> > +							hw_dsc, ARRAY_SIZE(hw_dsc));
> > +		for (i = 0; i < MAX_CHANNELS_PER_ENC; i++)
> > +			dpu_enc->hw_dsc[i] = i < num_dsc ? to_dpu_hw_dsc(hw_dsc[i]) : NULL;
> > +
> > +		for (i = 0; i < num_dsc; i++)
> > +			dsc_mask |= BIT(dpu_enc->hw_dsc[i]->idx - DSC_0);
> 
> Perhaps you can merge these loops?  Assuming hw_dsc is already
> NULL-initialized, you only need to iterate to `i<num_dsc` once without
> extra conditional anyway?

Agreed, this does make it look neat

> 
> > +
> > +		dpu_enc->dsc_mask = dsc_mask;
> > +	}
> > +
> >  	cstate = to_dpu_crtc_state(drm_crtc->state);
> >  
> >  	for (i = 0; i < num_lm; i++) {
> > @@ -1739,6 +1767,127 @@ static void dpu_encoder_vsync_event_work_handler(struct kthread_work *work)
> >  			nsecs_to_jiffies(ktime_to_ns(wakeup_time)));
> >  }
> >  
> > +static void
> > +dpu_encoder_dsc_pclk_param_calc(struct msm_display_dsc_config *dsc, u32 width)
> > +{
> > +	int slice_count, slice_per_intf;
> > +	int bytes_in_slice, total_bytes_per_intf;
> > +
> > +	if (!dsc || !dsc->drm->slice_width || !dsc->drm->slice_count) {
> > +		DPU_ERROR("Invalid DSC/slices\n");
> 
> What's the forward slash for?

forward slash denotes an OR case in written, so above would imply
Invalid DSC or slices, I will make it 'or' to be clear

> 
> > +		return;
> > +	}
> > +
> > +	slice_count = dsc->drm->slice_count;
> > +	slice_per_intf = DIV_ROUND_UP(width, dsc->drm->slice_width);
> > +
> > +	/*
> > +	 * If slice_count is greater than slice_per_intf then default to 1.
> > +	 * This can happen during partial update.
> > +	 */
> > +	if (slice_count > slice_per_intf)
> > +		slice_count = 1;
> > +
> > +	bytes_in_slice = DIV_ROUND_UP(dsc->drm->slice_width *
> > +				      dsc->drm->bits_per_pixel, 8);
> > +	total_bytes_per_intf = bytes_in_slice * slice_per_intf;
> > +
> > +	dsc->eol_byte_num = total_bytes_per_intf % 3;
> > +	dsc->pclk_per_line =  DIV_ROUND_UP(total_bytes_per_intf, 3);
> > +	dsc->bytes_in_slice = bytes_in_slice;
> > +	dsc->bytes_per_pkt = bytes_in_slice * slice_count;
> > +	dsc->pkt_per_line = slice_per_intf / slice_count;
> > +}
> 
> I've seen the same calculations duplicated twice in dsi code.  Since the
> msm_display_dsc_config struct is available in a header, perhaps a single
> - easily reviewable and maintainable - calculation function should be
> available there too?

Let me try check if we can make it common..

-- 
~Vinod