Re: [Intel-gfx] [PATCH v5 6/7] drm/i915/gt: Create per-tile RPS sysfs interfaces

[Andi Shyti <andi.shyti@linux.intel.com>]

Hi Andrzej,

[...]

> > +static ssize_t act_freq_mhz_show(struct device *dev,
> > +				 struct device_attribute *attr, char *buff)
> > +{
> > +	s64 actual_freq = sysfs_gt_attribute_r_func(dev, attr,
> > +						    __act_freq_mhz_show);
> > +
> > +	return sysfs_emit(buff, "%u\n", (u32) actual_freq);
> 
> Again, variable can be just u32.

yes

[...]

> > +static int __boost_freq_mhz_store(struct intel_gt *gt, u32 val)
> > +{
> > +	struct intel_rps *rps = &gt->rps;
> > +	bool boost = false;
> > +
> > +	/* Validate against (static) hardware limits */
> > +	val = intel_freq_opcode(rps, val);
> > +	if (val < rps->min_freq || val > rps->max_freq)
> > +		return -EINVAL;
> > +
> > +	mutex_lock(&rps->lock);
> > +	if (val != rps->boost_freq) {
> > +		rps->boost_freq = val;
> > +		boost = atomic_read(&rps->num_waiters);
> > +	}
> > +	mutex_unlock(&rps->lock);
> > +	if (boost)
> > +		schedule_work(&rps->work);
> > +
> > +	return 0;
> 
> Why not intel_rps_set_boost_frequency?
> Why copy/paste from rps_set_boost_freq?

you are right... this must be some ugly leftover. Thanks!

[...]

> > +/* For now we have a static number of RP states */
> > +static ssize_t rps_rp_mhz_show(struct device *dev,
> > +			       struct device_attribute *attr,
> > +			       char *buff)
> > +{
> > +	struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev, attr->attr.name);
> > +	struct intel_rps *rps = &gt->rps;
> > +	u32 val;
> > +
> > +	if (attr == &dev_attr_gt_RP0_freq_mhz ||
> > +	    attr == &dev_attr_rps_RP0_freq_mhz) {
> > +		val = intel_rps_get_rp0_frequency(rps);
> > +	} else if (attr == &dev_attr_gt_RP1_freq_mhz ||
> > +		   attr == &dev_attr_rps_RP1_freq_mhz) {
> > +		   val = intel_rps_get_rp1_frequency(rps);
> > +	} else if (attr == &dev_attr_gt_RPn_freq_mhz ||
> > +		   attr == &dev_attr_rps_RPn_freq_mhz) {
> > +		   val = intel_rps_get_rpn_frequency(rps);
> > +	} else {
> > +		GEM_WARN_ON(1);
> > +		return -ENODEV;
> 
> I guess this is not necessary, otherwise similar path should be in other
> helpers.

yeah... it's a bit hacky, I must agree... will split it properly.

[...]

Thanks,
Andi