<div dir="ltr"><div>Hi Timur,</div><div><br></div><div>I agree with you about the coding style (the prefix), it was more of a general comment since the style is inconsistent with some other parts in the DC code.</div><div><br></div><div>If the code applies to any DCE version (even though it is unclear if any > DCE 10 is in the field with a DAC), then it's good for me. My only remaining concern would be about encountering one of those special GPU cards hardly testable. But this seems unlikely.</div><div><br></div><div>Alexandre</div></div><br><div class="gmail_quote gmail_quote_container"><div dir="ltr" class="gmail_attr">On Fri, Aug 1, 2025 at 5:29 PM Timur Kristóf <<a href="mailto:timur.kristof@gmail.com">timur.kristof@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br>2025. augusztus 1., péntek dátummal Alexandre Demers <<a href="mailto:alexandre.f.demers@gmail.com" target="_blank">alexandre.f.demers@gmail.com</a>> ezt írta:<br>>> Add stream encoders for DCE6-10 only, because there are definitely<br>>> graphics cards with analog connectors out there with these DCE<br>>> versions. I am not aware of newer ones.<br>><br>>> Considering that all stream encoder functions currently have to do<br>>> with digital streams, there is nothing for an analog stream<br>>> encoder to do, making them basically a no-op.<br>>> That being said, we still need some kind of stream encoder to<br>>> represent an analog stream, and it is beneficial to split them from<br>>> digital stream encoders in the code to make sure they don't<br>>> accidentally write any DIG* registers.<br>>><br>>> Signed-off-by: Timur Kristóf <timur.kristof at <a href="http://gmail.com" target="_blank">gmail.com</a>><br>>> ---<br>>> .../drm/amd/display/dc/dce/dce_stream_encoder.c | 14 ++++++++++++++<br>>> .../drm/amd/display/dc/dce/dce_stream_encoder.h | 5 +++++<br>>> .../display/dc/resource/dce100/dce100_resource.c | 6 ++++++<br>>> .../amd/display/dc/resource/dce60/dce60_resource.c | 8 ++++++++<br>>> .../amd/display/dc/resource/dce80/dce80_resource.c | 8 ++++++++<br>>> 5 files changed, 41 insertions(+)<br>>><br>>> diff --git a/drivers/gpu/drm/amd/display/dc/dce/dce_stream_encoder.c b/drivers/gpu/drm/amd/display/dc/dce/dce_stream_encoder.c<br>>> index 1130d7619b26..f8996ee2856b 100644<br>>> --- a/drivers/gpu/drm/amd/display/dc/dce/dce_stream_encoder.c<br>>> +++ b/drivers/gpu/drm/amd/display/dc/dce/dce_stream_encoder.c<br>>> @@ -1567,3 +1567,17 @@ void dce110_stream_encoder_construct(<br>>> enc110->se_shift = se_shift;<br>>> enc110->se_mask = se_mask;<br>>> }<br>>> +<br>>> +static const struct stream_encoder_funcs dce110_an_str_enc_funcs = {0};<br>>> +<br>>> +void dce110_analog_stream_encoder_construct(<br>>> + struct dce110_stream_encoder *enc110,<br>>> + struct dc_context *ctx,<br>>> + struct dc_bios *bp,<br>>> + enum engine_id eng_id)<br>>> +{<br>>> + enc110->base.funcs = &dce110_an_str_enc_funcs;<br>>> + enc110->base.ctx = ctx;<br>>> + enc110-><a href="http://base.id" target="_blank">base.id</a> = eng_id;<br>>> + enc110->base.bp = bp;<br>>> +}<br>><br>> Since we are adding analog stream encoder support only up to DCE10, wouldn't it be better if the prefix "dce100_" was used instead? I know there are a few functions in there that use "dce110_" as prefix and are replaced by functions specific to the DCE versions that behave differently (we even have dce60_ and dce80_ in the current patch), but this seems off otherwise.<br>><br>> IMO, if thie DCE code should be revisited, "dce_" should be the general prefix instead of "dce110_", with "dceXY_" being specific (as it is right now).<br>> Alexandre<br><br>Hi Alexandre,<br><br>Two reasons:<br><br>1. As best as I can tell, all DCE versions use dce110_stream_encoder regardless of its name. I agree that this is somewhat counter-intuitive but I wanted to follow the conventions in the rest of this file.<br><br>2. It is unclear which DCE versions actually have a DAC. On one hand there is some conversation on this ML that suggests that Hawaii and newer don't have a DAC, which is apparently wrong. Looking at the register headers, all DCE including the latest one have the DAC registers. Theoretically, if there were newer GPUs with DAC, this code would work fine on them too.<br><br>If you are not happy with the code style, I respect that but I would like if we tackled that separarely.<br><br>With these series, I want to just focus on fixing things and getting DC to work on SI and CIK with the least possible risk of breaking things.<br><br>Best regards,<br>Timur
</blockquote></div>