Re: [PATCH v1 2/4] media: qcom: iris: Add rotation support for encoder

[Wangao Wang <wangao.wang@oss.qualcomm.com>]

On 10/16/2025 12:08 AM, Bryan O'Donoghue wrote:
>> iris_hfi_gen2_set_bitstream_resolution(struct iris_inst *inst, u32 pl
>>           payload_type = HFI_PAYLOAD_U32;
>>       } else {
>>           codec_align = inst->codec == V4L2_PIX_FMT_HEVC ? 32 : 16;
>> -        resolution = ALIGN(inst->enc_bitstream_width, codec_align) << 
>> 16 |
>> -            ALIGN(inst->enc_bitstream_height, codec_align);
>> +        if (is_rotation_90_or_270(inst))
>> +            resolution = ALIGN(inst->enc_bitstream_height, 
>> codec_align) << 16 |
>> +                ALIGN(inst->enc_bitstream_width, codec_align);
>> +        else
>> +            resolution = ALIGN(inst->enc_bitstream_width, 
>> codec_align) << 16 |
>> +                ALIGN(inst->enc_bitstream_height, codec_align);
> 
> That complex assignement can be expressed as a macro, static inline or 
> just a regular method instead of copy/paste the same code with one 
> variable changed.
> 
You're right, this code is reused in several places. I will address it 
in v2.>>   inline bool is_scaling_enabled(struct iris_inst *inst)
>>   {
>> -    return inst->crop.left != inst->compose.left ||
>> -        inst->crop.top != inst->compose.top ||
>> -        inst->crop.width != inst->compose.width ||
>> -        inst->crop.height != inst->compose.height;
>> +    return inst->fmt_dst->fmt.pix_mp.width != inst->fmt_src- 
>> >fmt.pix_mp.width ||
>> +        inst->fmt_dst->fmt.pix_mp.height != inst->fmt_src- 
>> >fmt.pix_mp.height;
> 
> These long chains of indirection make the code not very readable
> 
> Please take pointers to &fmt_dts->fmt and &fmt_src->fmt and reduce this 
> clause.
> 
Sure, will update in v2.

-- 
Best Regards,
Wangao