| Message ID | 20230521-drm-panels-sony-v1-0-541c341d6bee@somainline.org |
|---|---|
| Headers | show |
| Series | drm/panel: Drivers for four Sony CMD-mode (and DSC) panels | expand |
On Tue, 23 May 2023 at 01:32, Marijn Suijten <marijn.suijten@somainline.org> wrote: > > On 2023-05-22 04:19:45, Dmitry Baryshkov wrote: > > On 22/05/2023 00:23, Marijn Suijten wrote: > > > This SOFEF01-M Display-IC driver supports two modes with different > > > compatibles to differentiate between slightly different physical sizes > > > (panels) found on the Xperia 5 (6.1") and 10 II (6.0"). > > > > > > It is currently also used to hardcode significantly higher fake porches > > > for the Xperia 5, which are unused in transfers due to this being a > > > command-mode panel but do have an effect on the clock rates set by > > > dsi_host.c. Without higher clock rates this panel fails to achieve > > > 60fps and has significant tearing artifacts, while the same calculated > > > clock rate works perfectly fine on the Xperia 10 II. > > <snip> > > > > +/* Sony Xperia 5 (kumano bahamut) */ > > > +static const struct drm_display_mode samsung_sofef01_m_bahamut_mode = { > > > + /* > > > + * WARNING: These massive porches are wrong/useless for CMDmode > > > + * (and not defined in downstream DTS) but necessary to bump dsi > > > + * clocks higher, so that we can achieve proper 60fps without tearing. > > > + */ > > > + .clock = (1080 + 156 + 8 + 8) * (2520 + 2393 + 8 + 8) * 60 / 1000, > > > + .hdisplay = 1080, > > > + .hsync_start = 1080 + 156, > > > + .hsync_end = 1080 + 156 + 8, > > > + .htotal = 1080 + 156 + 8 + 8, > > > + .vdisplay = 2520, > > > + .vsync_start = 2520 + 2393, > > > + .vsync_end = 2520 + 2393 + 8, > > > + .vtotal = 2520 + 2393 + 8 + 8, > > > + .width_mm = 61, > > > + .height_mm = 142, > > > +}; > > > + > > > +/* Sony Xperia 10 II (seine pdx201) */ > > > +static const struct drm_display_mode samsung_sofef01_m_pdx201_mode = { > > > + .clock = (1080 + 8 + 8 + 8) * (2520 + 8 + 8 + 8) * 60 / 1000, > > > + .hdisplay = 1080, > > > + .hsync_start = 1080 + 8, > > > + .hsync_end = 1080 + 8 + 8, > > > + .htotal = 1080 + 8 + 8 + 8, > > > + .vdisplay = 2520, > > > + .vsync_start = 2520 + 8, > > > + .vsync_end = 2520 + 8 + 8, > > > + .vtotal = 2520 + 8 + 8 + 8, > > > + .width_mm = 60, > > > + .height_mm = 139, > > > +}; > > > + > > > +static const struct of_device_id samsung_sofef01_m_of_match[] = { > > > + { .compatible = "samsung,sofef01-m-bahamut", .data = &samsung_sofef01_m_bahamut_mode }, > > > + { .compatible = "samsung,sofef01-m-pdx201", .data = &samsung_sofef01_m_pdx201_mode }, > > > > Are there really two panels? Can we use one mode for both usecases? > > See the commit description where I explained exactly this: the panels > have different dimensions (6.1" vs 6.0", hence different DPI) and I also > abuse this to hack in higher clock rates via fake porches. > > I just ended up on a scary website that supposedly contains the panel > names: > > - Xperia 5 (bahamut, 6.1"): AMB609TC01 > - Xperia 10 II (pdx201, 6.0"): AMS597UT01 Great! From the patch description it was not obvious if those are two different panels or a single panel with slight difference in the glass cover. With these names in place (well, with two distinct names in place) it makes sense.
On 21/05/2023 22:23, Marijn Suijten wrote: > As per the config name this Display IC features a DSI command-mode > interface (or the command to switch to video mode is not > known/documented) and does not use any of the video-mode helper > utilities, hence should not select VIDEOMODE_HELPERS. In addition it > uses devm_gpiod_get() and related functions from GPIOLIB. > > Fixes: 5933baa36e26 ("drm/panel/samsung-sofef00: Add panel for OnePlus 6/T devices") > Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org> Reviewed-by: Caleb Connolly <caleb@connolly.tech> > --- > drivers/gpu/drm/panel/Kconfig | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig > index 2b9d6db7860ba..67ef898d133f2 100644 > --- a/drivers/gpu/drm/panel/Kconfig > +++ b/drivers/gpu/drm/panel/Kconfig > @@ -608,10 +608,10 @@ config DRM_PANEL_SAMSUNG_S6E8AA0 > > config DRM_PANEL_SAMSUNG_SOFEF00 > tristate "Samsung sofef00/s6e3fc2x01 OnePlus 6/6T DSI cmd mode panels" > + depends on GPIOLIB > depends on OF > depends on DRM_MIPI_DSI > depends on BACKLIGHT_CLASS_DEVICE > - select VIDEOMODE_HELPERS > help > Say Y or M here if you want to enable support for the Samsung AMOLED > command mode panels found in the OnePlus 6/6T smartphones. > > -- > 2.40.1 >
On 30/05/2023 00:11, Marijn Suijten wrote: > On 2023-05-22 04:16:20, Dmitry Baryshkov wrote: > <snip> >>> + if (ctx->dsi->dsc) { >> >> dsi->dsc is always set, thus this condition can be dropped. > > I want to leave room for possibly running the panel without DSC (at a > lower resolution/refresh rate, or at higher power consumption if there > is enough BW) by not assigning the pointer, if we get access to panel > documentation: probably one of the magic commands sent in this driver > controls it but we don't know which. > >>> + drm_dsc_pps_payload_pack(&pps, ctx->dsi->dsc); >>> + >>> + ret = mipi_dsi_picture_parameter_set(ctx->dsi, &pps); >>> + if (ret < 0) { >>> + dev_err(panel->dev, "failed to transmit PPS: %d\n", ret); >>> + goto fail; >>> + } >>> + ret = mipi_dsi_compression_mode(ctx->dsi, true); >>> + if (ret < 0) { >>> + dev_err(dev, "failed to enable compression mode: %d\n", ret); >>> + goto fail; >>> + } >>> + >>> + msleep(28); >>> + } >>> + >>> + ctx->prepared = true; >>> + return 0; >>> + >>> +fail: >>> + gpiod_set_value_cansleep(ctx->reset_gpio, 0); >>> + regulator_disable(ctx->vddio); >>> + return ret; >>> +} > <snip> >>> + /* This panel only supports DSC; unconditionally enable it */ > > On that note I should perhaps reword this. > >>> + dsi->dsc = dsc = devm_kzalloc(&dsi->dev, sizeof(*dsc), GFP_KERNEL); >> >> I think double assignments are frowned upon. > > Ack. > >> >>> + if (!dsc) >>> + return -ENOMEM; >>> + >>> + dsc->dsc_version_major = 1; >>> + dsc->dsc_version_minor = 1; >>> + >>> + dsc->slice_height = 32; >>> + dsc->slice_count = 2; >>> + // TODO: Get hdisplay from the mode >> >> Would you like to fix the TODO? > > I can't unless either migrating to drm_bridge (is that doable?) or > expand drm_panel. That's a larger task, but I don't think this driver > is the right place to track that desire. Should I drop the comment > entirely or reword it? I'd say, reword it, so that it becomes more obvious why this TODO can not be fixed at this moment. > >>> + WARN_ON(1440 % dsc->slice_count); >>> + dsc->slice_width = 1440 / dsc->slice_count; > > <snip> > > - Marijn
On 29/05/2023 23:58, Marijn Suijten wrote: > On 2023-05-23 01:56:46, Dmitry Baryshkov wrote: >> On Tue, 23 May 2023 at 01:32, Marijn Suijten >> <marijn.suijten@somainline.org> wrote: >>> >>> On 2023-05-22 04:19:45, Dmitry Baryshkov wrote: >>>> On 22/05/2023 00:23, Marijn Suijten wrote: >>>>> This SOFEF01-M Display-IC driver supports two modes with different >>>>> compatibles to differentiate between slightly different physical sizes >>>>> (panels) found on the Xperia 5 (6.1") and 10 II (6.0"). >>>>> >>>>> It is currently also used to hardcode significantly higher fake porches >>>>> for the Xperia 5, which are unused in transfers due to this being a >>>>> command-mode panel but do have an effect on the clock rates set by >>>>> dsi_host.c. Without higher clock rates this panel fails to achieve >>>>> 60fps and has significant tearing artifacts, while the same calculated >>>>> clock rate works perfectly fine on the Xperia 10 II. >>> >>> <snip> >>> >>>>> +/* Sony Xperia 5 (kumano bahamut) */ >>>>> +static const struct drm_display_mode samsung_sofef01_m_bahamut_mode = { >>>>> + /* >>>>> + * WARNING: These massive porches are wrong/useless for CMDmode >>>>> + * (and not defined in downstream DTS) but necessary to bump dsi >>>>> + * clocks higher, so that we can achieve proper 60fps without tearing. >>>>> + */ >>>>> + .clock = (1080 + 156 + 8 + 8) * (2520 + 2393 + 8 + 8) * 60 / 1000, >>>>> + .hdisplay = 1080, >>>>> + .hsync_start = 1080 + 156, >>>>> + .hsync_end = 1080 + 156 + 8, >>>>> + .htotal = 1080 + 156 + 8 + 8, >>>>> + .vdisplay = 2520, >>>>> + .vsync_start = 2520 + 2393, >>>>> + .vsync_end = 2520 + 2393 + 8, >>>>> + .vtotal = 2520 + 2393 + 8 + 8, >>>>> + .width_mm = 61, >>>>> + .height_mm = 142, >>>>> +}; >>>>> + >>>>> +/* Sony Xperia 10 II (seine pdx201) */ >>>>> +static const struct drm_display_mode samsung_sofef01_m_pdx201_mode = { >>>>> + .clock = (1080 + 8 + 8 + 8) * (2520 + 8 + 8 + 8) * 60 / 1000, >>>>> + .hdisplay = 1080, >>>>> + .hsync_start = 1080 + 8, >>>>> + .hsync_end = 1080 + 8 + 8, >>>>> + .htotal = 1080 + 8 + 8 + 8, >>>>> + .vdisplay = 2520, >>>>> + .vsync_start = 2520 + 8, >>>>> + .vsync_end = 2520 + 8 + 8, >>>>> + .vtotal = 2520 + 8 + 8 + 8, >>>>> + .width_mm = 60, >>>>> + .height_mm = 139, >>>>> +}; >>>>> + >>>>> +static const struct of_device_id samsung_sofef01_m_of_match[] = { >>>>> + { .compatible = "samsung,sofef01-m-bahamut", .data = &samsung_sofef01_m_bahamut_mode }, >>>>> + { .compatible = "samsung,sofef01-m-pdx201", .data = &samsung_sofef01_m_pdx201_mode }, >>>> >>>> Are there really two panels? Can we use one mode for both usecases? >>> >>> See the commit description where I explained exactly this: the panels >>> have different dimensions (6.1" vs 6.0", hence different DPI) and I also >>> abuse this to hack in higher clock rates via fake porches. >>> >>> I just ended up on a scary website that supposedly contains the panel >>> names: >>> >>> - Xperia 5 (bahamut, 6.1"): AMB609TC01 >>> - Xperia 10 II (pdx201, 6.0"): AMS597UT01 >> >> Great! From the patch description it was not obvious if those are two >> different panels or a single panel with slight difference in the glass >> cover. With these names in place (well, with two distinct names in >> place) it makes sense. > > For completeness: keep the current single file but embed these panel > names as suffix (eg. `samsung,sofef-01-m-am[bs]...`) to the compatible > (and document these more explicitly elsewhere)? Where do the sofef parts of the name come from? Glancing at other panels, I'd expect something simpler. Maybe: samsung,sofef01m-amb60... > > - Marijn > >> >> -- >> With best wishes >> Dmitry
On 2023-05-30 01:20:17, Dmitry Baryshkov wrote: > On 29/05/2023 23:58, Marijn Suijten wrote: > > On 2023-05-23 01:56:46, Dmitry Baryshkov wrote: > >> On Tue, 23 May 2023 at 01:32, Marijn Suijten > >> <marijn.suijten@somainline.org> wrote: > >>> > >>> On 2023-05-22 04:19:45, Dmitry Baryshkov wrote: > >>>> On 22/05/2023 00:23, Marijn Suijten wrote: > >>>>> This SOFEF01-M Display-IC driver supports two modes with different > >>>>> compatibles to differentiate between slightly different physical sizes > >>>>> (panels) found on the Xperia 5 (6.1") and 10 II (6.0"). > >>>>> > >>>>> It is currently also used to hardcode significantly higher fake porches > >>>>> for the Xperia 5, which are unused in transfers due to this being a > >>>>> command-mode panel but do have an effect on the clock rates set by > >>>>> dsi_host.c. Without higher clock rates this panel fails to achieve > >>>>> 60fps and has significant tearing artifacts, while the same calculated > >>>>> clock rate works perfectly fine on the Xperia 10 II. > >>> > >>> <snip> > >>> > >>>>> +/* Sony Xperia 5 (kumano bahamut) */ > >>>>> +static const struct drm_display_mode samsung_sofef01_m_bahamut_mode = { > >>>>> + /* > >>>>> + * WARNING: These massive porches are wrong/useless for CMDmode > >>>>> + * (and not defined in downstream DTS) but necessary to bump dsi > >>>>> + * clocks higher, so that we can achieve proper 60fps without tearing. > >>>>> + */ > >>>>> + .clock = (1080 + 156 + 8 + 8) * (2520 + 2393 + 8 + 8) * 60 / 1000, > >>>>> + .hdisplay = 1080, > >>>>> + .hsync_start = 1080 + 156, > >>>>> + .hsync_end = 1080 + 156 + 8, > >>>>> + .htotal = 1080 + 156 + 8 + 8, > >>>>> + .vdisplay = 2520, > >>>>> + .vsync_start = 2520 + 2393, > >>>>> + .vsync_end = 2520 + 2393 + 8, > >>>>> + .vtotal = 2520 + 2393 + 8 + 8, > >>>>> + .width_mm = 61, > >>>>> + .height_mm = 142, > >>>>> +}; > >>>>> + > >>>>> +/* Sony Xperia 10 II (seine pdx201) */ > >>>>> +static const struct drm_display_mode samsung_sofef01_m_pdx201_mode = { > >>>>> + .clock = (1080 + 8 + 8 + 8) * (2520 + 8 + 8 + 8) * 60 / 1000, > >>>>> + .hdisplay = 1080, > >>>>> + .hsync_start = 1080 + 8, > >>>>> + .hsync_end = 1080 + 8 + 8, > >>>>> + .htotal = 1080 + 8 + 8 + 8, > >>>>> + .vdisplay = 2520, > >>>>> + .vsync_start = 2520 + 8, > >>>>> + .vsync_end = 2520 + 8 + 8, > >>>>> + .vtotal = 2520 + 8 + 8 + 8, > >>>>> + .width_mm = 60, > >>>>> + .height_mm = 139, > >>>>> +}; > >>>>> + > >>>>> +static const struct of_device_id samsung_sofef01_m_of_match[] = { > >>>>> + { .compatible = "samsung,sofef01-m-bahamut", .data = &samsung_sofef01_m_bahamut_mode }, > >>>>> + { .compatible = "samsung,sofef01-m-pdx201", .data = &samsung_sofef01_m_pdx201_mode }, > >>>> > >>>> Are there really two panels? Can we use one mode for both usecases? > >>> > >>> See the commit description where I explained exactly this: the panels > >>> have different dimensions (6.1" vs 6.0", hence different DPI) and I also > >>> abuse this to hack in higher clock rates via fake porches. > >>> > >>> I just ended up on a scary website that supposedly contains the panel > >>> names: > >>> > >>> - Xperia 5 (bahamut, 6.1"): AMB609TC01 > >>> - Xperia 10 II (pdx201, 6.0"): AMS597UT01 > >> > >> Great! From the patch description it was not obvious if those are two > >> different panels or a single panel with slight difference in the glass > >> cover. With these names in place (well, with two distinct names in > >> place) it makes sense. > > > > For completeness: keep the current single file but embed these panel > > names as suffix (eg. `samsung,sofef-01-m-am[bs]...`) to the compatible > > (and document these more explicitly elsewhere)? > > Where do the sofef parts of the name come from? Glancing at other > panels, I'd expect something simpler. Maybe: That is the Driver-IC. Sorry, I meant sofef01-m without the first dash, matching the original compatibles. But can also drop the dash in 01-m if desired. > samsung,sofef01m-amb60... - Marijn
On 2023-05-30 01:18:40, Dmitry Baryshkov wrote: <snip> > >>>>> + ret = mipi_dsi_dcs_set_display_on(dsi); > >>>>> + if (ret < 0) { > >>>>> + dev_err(dev, "Failed to turn display on: %d\n", ret); > >>>>> + return ret; > >>>>> + } > >>>> > >>>> My usual question: should the mipi_dsi_dcs_exit_sleep_mode() / mipi_dsi_dcs_set_display_on() be moved from prepare() to enable() part? > >>> > >>> > >>> No, prepare is called before the video stream is started and when display is still in LPM mode and the mode hasn't been set. > >>> > >> > >> Yes, that's my point. Shouldn't we enable the panel _after_ starting the stream? > > > > I have never investigated what it takes to split these functions, but > > some of these panels do show some corruption at startup which may be > > circumvented by powering the panel on after starting the video stream? > > > > I'm just not sure where to make the split: downstream does describe a > > qcom,mdss-dsi-on-command and qcom,mdss-dsi-post-panel-on-command, where > > the latter only contains set_display_on() (not exit_sleep_mode()). > > It is documented like: > > > > same as "qcom,mdss-dsi-on-command" except commands are sent after > > displaying an image." > > > > So this seems like the right way to split them up, I'll test this out on > > all submitted panel drivers. > > Interesting enough, Neil suggested that sending all the commands during > pre_enable() is the correct sequence (especially for VIDEO mode panels), > since not all DSI hosts can send commands after switching to the VIDEO mode. Note that all these panels and Driver-ICs are command-mode, and/or programmed to run in command-mode, so there shouldn't be any notion of a VIDEO stream (any command-mode frame is just an "arbitrary command" as far as I understood). - Marijn
Hi Marijn, Dmitry, Caleb, Jessica, On 29/05/2023 23:11, Marijn Suijten wrote: > On 2023-05-22 04:16:20, Dmitry Baryshkov wrote: > <snip> >>> + if (ctx->dsi->dsc) { >> >> dsi->dsc is always set, thus this condition can be dropped. > > I want to leave room for possibly running the panel without DSC (at a > lower resolution/refresh rate, or at higher power consumption if there > is enough BW) by not assigning the pointer, if we get access to panel > documentation: probably one of the magic commands sent in this driver > controls it but we don't know which. I'd like to investigate if DSC should perhaps only be enabled if we run non certain platforms/socs ? I mean, we don't know if the controller supports DSC and those particular DSC parameters so we should probably start adding something like : static drm_dsc_config dsc_params_qcom = {} static const struct of_device_id panel_of_dsc_params[] = { { .compatible = "qcom,sm8150", , .data = &dsc_params_qcom }, { .compatible = "qcom,sm8250", , .data = &dsc_params_qcom }, { .compatible = "qcom,sm8350", , .data = &dsc_params_qcom }, { .compatible = "qcom,sm8450", , .data = &dsc_params_qcom }, }; ... static int sony_akatsuki_lgd_probe(struct mipi_dsi_device *dsi) ... const struct of_device_id *match; ... match = of_match_node(panel_of_dsc_params, of_root); if (match && match->data) { dsi->dsc = devm_kzalloc(&dsi->dev, sizeof(*dsc), GFP_KERNEL); memcpy(dsi->dsc, match->data, sizeof(*dsc)); } else { dev_warn(&dsi->dev, "DSI controller is not marked as supporting DSC\n"); } ... } and probably bail out if it's a DSC only panel. We could alternatively match on the DSI controller's dsi->host->dev instead of the SoC root compatible. Neil > >>> + drm_dsc_pps_payload_pack(&pps, ctx->dsi->dsc); >>> + >>> + ret = mipi_dsi_picture_parameter_set(ctx->dsi, &pps); >>> + if (ret < 0) { >>> + dev_err(panel->dev, "failed to transmit PPS: %d\n", ret); >>> + goto fail; >>> + } >>> + ret = mipi_dsi_compression_mode(ctx->dsi, true); >>> + if (ret < 0) { >>> + dev_err(dev, "failed to enable compression mode: %d\n", ret); >>> + goto fail; >>> + } >>> + >>> + msleep(28); >>> + } >>> + >>> + ctx->prepared = true; >>> + return 0; >>> + >>> +fail: >>> + gpiod_set_value_cansleep(ctx->reset_gpio, 0); >>> + regulator_disable(ctx->vddio); >>> + return ret; >>> +} > <snip> >>> + /* This panel only supports DSC; unconditionally enable it */ > > On that note I should perhaps reword this. > >>> + dsi->dsc = dsc = devm_kzalloc(&dsi->dev, sizeof(*dsc), GFP_KERNEL); >> >> I think double assignments are frowned upon. > > Ack. > >> >>> + if (!dsc) >>> + return -ENOMEM; >>> + >>> + dsc->dsc_version_major = 1; >>> + dsc->dsc_version_minor = 1; >>> + >>> + dsc->slice_height = 32; >>> + dsc->slice_count = 2; >>> + // TODO: Get hdisplay from the mode >> >> Would you like to fix the TODO? > > I can't unless either migrating to drm_bridge (is that doable?) or > expand drm_panel. That's a larger task, but I don't think this driver > is the right place to track that desire. Should I drop the comment > entirely or reword it? > >>> + WARN_ON(1440 % dsc->slice_count); >>> + dsc->slice_width = 1440 / dsc->slice_count; > > <snip> > > - Marijn
On 2023-05-30 01:39:10, Dmitry Baryshkov wrote: > On 30/05/2023 01:37, Marijn Suijten wrote: > > On 2023-05-30 01:18:40, Dmitry Baryshkov wrote: > > <snip> > >>>>>>> + ret = mipi_dsi_dcs_set_display_on(dsi); > >>>>>>> + if (ret < 0) { > >>>>>>> + dev_err(dev, "Failed to turn display on: %d\n", ret); > >>>>>>> + return ret; > >>>>>>> + } > >>>>>> > >>>>>> My usual question: should the mipi_dsi_dcs_exit_sleep_mode() / mipi_dsi_dcs_set_display_on() be moved from prepare() to enable() part? > >>>>> > >>>>> > >>>>> No, prepare is called before the video stream is started and when display is still in LPM mode and the mode hasn't been set. > >>>>> > >>>> > >>>> Yes, that's my point. Shouldn't we enable the panel _after_ starting the stream? > >>> > >>> I have never investigated what it takes to split these functions, but > >>> some of these panels do show some corruption at startup which may be > >>> circumvented by powering the panel on after starting the video stream? > >>> > >>> I'm just not sure where to make the split: downstream does describe a > >>> qcom,mdss-dsi-on-command and qcom,mdss-dsi-post-panel-on-command, where > >>> the latter only contains set_display_on() (not exit_sleep_mode()). > >>> It is documented like: > >>> > >>> same as "qcom,mdss-dsi-on-command" except commands are sent after > >>> displaying an image." > >>> > >>> So this seems like the right way to split them up, I'll test this out on > >>> all submitted panel drivers. > >> > >> Interesting enough, Neil suggested that sending all the commands during > >> pre_enable() is the correct sequence (especially for VIDEO mode panels), > >> since not all DSI hosts can send commands after switching to the VIDEO mode. > > > > Note that all these panels and Driver-ICs are command-mode, and/or > > programmed to run in command-mode, so there shouldn't be any notion of a > > VIDEO stream (any command-mode frame is just an "arbitrary command" as > > far as I understood). > > Yes, from the data stream point of view. I was talking about the DSI > host being able to send arbitrary commands or not after enabling the > video/cmd stream. Is this a known limitation of SM8250 then? Or is the brightness_set issue more likely a "problem" with the panel that the downstream kernel is "somehow" working around or aware of, and I just haven't found how/where it deals with that? (Alternatively we could be "doing it wrong" for other panels but it turns out to be working anyway) - Marijn
On Tue, 30 May 2023 at 10:24, Neil Armstrong <neil.armstrong@linaro.org> wrote: > > Hi Marijn, Dmitry, Caleb, Jessica, > > On 29/05/2023 23:11, Marijn Suijten wrote: > > On 2023-05-22 04:16:20, Dmitry Baryshkov wrote: > > <snip> > >>> + if (ctx->dsi->dsc) { > >> > >> dsi->dsc is always set, thus this condition can be dropped. > > > > I want to leave room for possibly running the panel without DSC (at a > > lower resolution/refresh rate, or at higher power consumption if there > > is enough BW) by not assigning the pointer, if we get access to panel > > documentation: probably one of the magic commands sent in this driver > > controls it but we don't know which. > > I'd like to investigate if DSC should perhaps only be enabled if we > run non certain platforms/socs ? > > I mean, we don't know if the controller supports DSC and those particular > DSC parameters so we should probably start adding something like : > > static drm_dsc_config dsc_params_qcom = {} > > static const struct of_device_id panel_of_dsc_params[] = { > { .compatible = "qcom,sm8150", , .data = &dsc_params_qcom }, > { .compatible = "qcom,sm8250", , .data = &dsc_params_qcom }, > { .compatible = "qcom,sm8350", , .data = &dsc_params_qcom }, > { .compatible = "qcom,sm8450", , .data = &dsc_params_qcom }, > }; I think this would damage the reusability of the drivers. The panel driver does not actually care if the SoC is SM8350, sunxi-something or RCar. Instead it cares about host capabilities. I think instead we should extend mipi_dsi_host: #define MIPI_DSI_HOST_MODE_VIDEO BIT(0) #define MIPI_DSI_HOST_MODE_CMD BIT(1) #define MIPI_DSI_HOST_VIDEO_SUPPORTS_COMMANDS BIT(2) // FIXME: do we need to provide additional caps here ? #define MIPI_DSI_DSC_1_1 BIT(0) #define MIPI_DSI_DSC_1_2 BIT(1) #define MIPI_DSI_DSC_NATIVE_422 BIT(2) #define MIPI_DSI_DSC_NATIVE_420 BIT(3) #define MIPI_DSI_DSC_FRAC_BPP BIT(4) // etc. struct mipi_dsi_host { // new fields only unsigned long mode_flags; unsigned long dsc_flags; }; Then the panel driver can adapt itself to the host capabilities and (possibly) select one of the internally supported DSC profiles. > > ... > static int sony_akatsuki_lgd_probe(struct mipi_dsi_device *dsi) > ... > const struct of_device_id *match; > > ... > match = of_match_node(panel_of_dsc_params, of_root); > if (match && match->data) { > dsi->dsc = devm_kzalloc(&dsi->dev, sizeof(*dsc), GFP_KERNEL); > memcpy(dsi->dsc, match->data, sizeof(*dsc)); > } else { > dev_warn(&dsi->dev, "DSI controller is not marked as supporting DSC\n"); > } > ... > } > > and probably bail out if it's a DSC only panel. > > We could alternatively match on the DSI controller's dsi->host->dev instead of the SoC root compatible. > > Neil
Il 30/05/23 13:44, Dmitry Baryshkov ha scritto: > On Tue, 30 May 2023 at 10:24, Neil Armstrong <neil.armstrong@linaro.org> wrote: >> >> Hi Marijn, Dmitry, Caleb, Jessica, >> >> On 29/05/2023 23:11, Marijn Suijten wrote: >>> On 2023-05-22 04:16:20, Dmitry Baryshkov wrote: >>> <snip> >>>>> + if (ctx->dsi->dsc) { >>>> >>>> dsi->dsc is always set, thus this condition can be dropped. >>> >>> I want to leave room for possibly running the panel without DSC (at a >>> lower resolution/refresh rate, or at higher power consumption if there >>> is enough BW) by not assigning the pointer, if we get access to panel >>> documentation: probably one of the magic commands sent in this driver >>> controls it but we don't know which. >> >> I'd like to investigate if DSC should perhaps only be enabled if we >> run non certain platforms/socs ? >> >> I mean, we don't know if the controller supports DSC and those particular >> DSC parameters so we should probably start adding something like : >> >> static drm_dsc_config dsc_params_qcom = {} >> >> static const struct of_device_id panel_of_dsc_params[] = { >> { .compatible = "qcom,sm8150", , .data = &dsc_params_qcom }, >> { .compatible = "qcom,sm8250", , .data = &dsc_params_qcom }, >> { .compatible = "qcom,sm8350", , .data = &dsc_params_qcom }, >> { .compatible = "qcom,sm8450", , .data = &dsc_params_qcom }, >> }; > > I think this would damage the reusability of the drivers. The panel > driver does not actually care if the SoC is SM8350, sunxi-something or > RCar. > Instead it cares about host capabilities. > > I think instead we should extend mipi_dsi_host: > > #define MIPI_DSI_HOST_MODE_VIDEO BIT(0) > #define MIPI_DSI_HOST_MODE_CMD BIT(1) > #define MIPI_DSI_HOST_VIDEO_SUPPORTS_COMMANDS BIT(2) > // FIXME: do we need to provide additional caps here ? > > #define MIPI_DSI_DSC_1_1 BIT(0) > #define MIPI_DSI_DSC_1_2 BIT(1) > #define MIPI_DSI_DSC_NATIVE_422 BIT(2) > #define MIPI_DSI_DSC_NATIVE_420 BIT(3) > #define MIPI_DSI_DSC_FRAC_BPP BIT(4) > // etc. > > struct mipi_dsi_host { > // new fields only > unsigned long mode_flags; > unsigned long dsc_flags; > }; > > Then the panel driver can adapt itself to the host capabilities and > (possibly) select one of the internally supported DSC profiles. > I completely agree about extending mipi_dsi_host, other SoCs could reuse that and support for DSC panels would become a lot cleaner. For example, on MediaTek DRM there's some support for DSC, more or less the same for SPRD DRM and some DSI bridge drivers... having a clean infrastructure would definitely help. I'm sad I cannot offer testing in that case because despite being sure that there are MTK smartphones around with DSI panels using DSC, I have none... and all of the Chromebooks are not using DSC anyway (but using DisplayPort compression, which is obviously an entirely different beast). >> >> ... >> static int sony_akatsuki_lgd_probe(struct mipi_dsi_device *dsi) >> ... >> const struct of_device_id *match; >> >> ... >> match = of_match_node(panel_of_dsc_params, of_root); >> if (match && match->data) { >> dsi->dsc = devm_kzalloc(&dsi->dev, sizeof(*dsc), GFP_KERNEL); >> memcpy(dsi->dsc, match->data, sizeof(*dsc)); >> } else { >> dev_warn(&dsi->dev, "DSI controller is not marked as supporting DSC\n"); >> } >> ... >> } >> >> and probably bail out if it's a DSC only panel. >> Usually DDICs support both DSC and non-DSC modes, depending on the initial programming (read: init commands)... but the usual issue is that many DDICs are not publicly documented for reasons, so yes, bailing out if DSC is not supported would be the only option, and would be fine at this point. Cheers, Angelo >> We could alternatively match on the DSI controller's dsi->host->dev instead of the SoC root compatible. >> >> Neil >
On 30/05/2023 15:15, AngeloGioacchino Del Regno wrote: > Il 30/05/23 13:44, Dmitry Baryshkov ha scritto: >> On Tue, 30 May 2023 at 10:24, Neil Armstrong >> <neil.armstrong@linaro.org> wrote: >>> >>> Hi Marijn, Dmitry, Caleb, Jessica, >>> >>> On 29/05/2023 23:11, Marijn Suijten wrote: >>>> On 2023-05-22 04:16:20, Dmitry Baryshkov wrote: >>>> <snip> >>>>>> +Â Â if (ctx->dsi->dsc) { >>>>> >>>>> dsi->dsc is always set, thus this condition can be dropped. >>>> >>>> I want to leave room for possibly running the panel without DSC (at a >>>> lower resolution/refresh rate, or at higher power consumption if there >>>> is enough BW) by not assigning the pointer, if we get access to panel >>>> documentation: probably one of the magic commands sent in this driver >>>> controls it but we don't know which. >>> >>> I'd like to investigate if DSC should perhaps only be enabled if we >>> run non certain platforms/socs ? >>> >>> I mean, we don't know if the controller supports DSC and those >>> particular >>> DSC parameters so we should probably start adding something like : >>> >>> static drm_dsc_config dsc_params_qcom = {} >>> >>> static const struct of_device_id panel_of_dsc_params[] = { >>> Â Â Â Â Â Â Â Â { .compatible = "qcom,sm8150", , .data = &dsc_params_qcom }, >>> Â Â Â Â Â Â Â Â { .compatible = "qcom,sm8250", , .data = &dsc_params_qcom }, >>> Â Â Â Â Â Â Â Â { .compatible = "qcom,sm8350", , .data = &dsc_params_qcom }, >>> Â Â Â Â Â Â Â Â { .compatible = "qcom,sm8450", , .data = &dsc_params_qcom }, >>> }; >> >> I think this would damage the reusability of the drivers. The panel >> driver does not actually care if the SoC is SM8350, sunxi-something or >> RCar. >> Instead it cares about host capabilities. >> >> I think instead we should extend mipi_dsi_host: >> >> #define MIPI_DSI_HOST_MODE_VIDEO BIT(0) >> #define MIPI_DSI_HOST_MODE_CMDÂ BIT(1) >> #define MIPI_DSI_HOST_VIDEO_SUPPORTS_COMMANDS BIT(2) >> // FIXME: do we need to provide additional caps here ? >> >> #define MIPI_DSI_DSC_1_1 BIT(0) >> #define MIPI_DSI_DSC_1_2 BIT(1) >> #define MIPI_DSI_DSC_NATIVE_422 BIT(2) >> #define MIPI_DSI_DSC_NATIVE_420 BIT(3) >> #define MIPI_DSI_DSC_FRAC_BPP BIT(4) >> // etc. >> >> struct mipi_dsi_host { >> Â // new fields only >> Â Â unsigned long mode_flags; >> Â Â unsigned long dsc_flags; >> }; >> >> Then the panel driver can adapt itself to the host capabilities and >> (possibly) select one of the internally supported DSC profiles. >> > > I completely agree about extending mipi_dsi_host, other SoCs could reuse > that and > support for DSC panels would become a lot cleaner. Sounds good. I will wait for one or two more days (to get the possible feedback on fields/flags/etc) and post an RFC patch to dri-devel. > > For example, on MediaTek DRM there's some support for DSC, more or less > the same > for SPRD DRM and some DSI bridge drivers... having a clean > infrastructure would > definitely help. > > I'm sad I cannot offer testing in that case because despite being sure > that there > are MTK smartphones around with DSI panels using DSC, I have none... and > all of the > Chromebooks are not using DSC anyway (but using DisplayPort compression, > which is > obviously an entirely different beast). > >>> >>> ... >>> static int sony_akatsuki_lgd_probe(struct mipi_dsi_device *dsi) >>> ... >>> Â Â Â Â Â Â Â Â const struct of_device_id *match; >>> >>> ... >>> Â Â Â Â Â Â Â Â match = of_match_node(panel_of_dsc_params, of_root); >>> Â Â Â Â Â Â Â Â if (match && match->data) { >>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â dsi->dsc = devm_kzalloc(&dsi->dev, sizeof(*dsc), >>> GFP_KERNEL); >>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â memcpy(dsi->dsc, match->data, sizeof(*dsc)); >>> Â Â Â Â Â Â Â Â } else { >>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â dev_warn(&dsi->dev, "DSI controller is not marked as >>> supporting DSC\n"); >>> Â Â Â Â Â Â Â Â } >>> ... >>> } >>> >>> and probably bail out if it's a DSC only panel. >>> > > Usually DDICs support both DSC and non-DSC modes, depending on the initial > programming (read: init commands)... but the usual issue is that many DDICs > are not publicly documented for reasons, so yes, bailing out if DSC is not > supported would be the only option, and would be fine at this point. > > Cheers, > Angelo > >>> We could alternatively match on the DSI controller's dsi->host->dev >>> instead of the SoC root compatible. >>> >>> Neil >> >
On 30/05/2023 14:36, Dmitry Baryshkov wrote: > On 30/05/2023 15:15, AngeloGioacchino Del Regno wrote: >> Il 30/05/23 13:44, Dmitry Baryshkov ha scritto: >>> On Tue, 30 May 2023 at 10:24, Neil Armstrong <neil.armstrong@linaro.org> wrote: >>>> >>>> Hi Marijn, Dmitry, Caleb, Jessica, >>>> >>>> On 29/05/2023 23:11, Marijn Suijten wrote: >>>>> On 2023-05-22 04:16:20, Dmitry Baryshkov wrote: >>>>> <snip> >>>>>>> +Â Â if (ctx->dsi->dsc) { >>>>>> >>>>>> dsi->dsc is always set, thus this condition can be dropped. >>>>> >>>>> I want to leave room for possibly running the panel without DSC (at a >>>>> lower resolution/refresh rate, or at higher power consumption if there >>>>> is enough BW) by not assigning the pointer, if we get access to panel >>>>> documentation: probably one of the magic commands sent in this driver >>>>> controls it but we don't know which. >>>> >>>> I'd like to investigate if DSC should perhaps only be enabled if we >>>> run non certain platforms/socs ? >>>> >>>> I mean, we don't know if the controller supports DSC and those particular >>>> DSC parameters so we should probably start adding something like : >>>> >>>> static drm_dsc_config dsc_params_qcom = {} >>>> >>>> static const struct of_device_id panel_of_dsc_params[] = { >>>> Â Â Â Â Â Â Â Â { .compatible = "qcom,sm8150", , .data = &dsc_params_qcom }, >>>> Â Â Â Â Â Â Â Â { .compatible = "qcom,sm8250", , .data = &dsc_params_qcom }, >>>> Â Â Â Â Â Â Â Â { .compatible = "qcom,sm8350", , .data = &dsc_params_qcom }, >>>> Â Â Â Â Â Â Â Â { .compatible = "qcom,sm8450", , .data = &dsc_params_qcom }, >>>> }; >>> >>> I think this would damage the reusability of the drivers. The panel >>> driver does not actually care if the SoC is SM8350, sunxi-something or >>> RCar. >>> Instead it cares about host capabilities. >>> >>> I think instead we should extend mipi_dsi_host: >>> >>> #define MIPI_DSI_HOST_MODE_VIDEO BIT(0) I assume all DSI controller supports Video mode, so it should be a negative here if for a reason it's not the case. There should also be a flag to tell if sending LP commands sending while in HS Video mode is supported. >>> #define MIPI_DSI_HOST_MODE_CMDÂ BIT(1) >>> #define MIPI_DSI_HOST_VIDEO_SUPPORTS_COMMANDS BIT(2) >>> // FIXME: do we need to provide additional caps here ? >>> >>> #define MIPI_DSI_DSC_1_1 BIT(0) >>> #define MIPI_DSI_DSC_1_2 BIT(1) >>> #define MIPI_DSI_DSC_NATIVE_422 BIT(2) >>> #define MIPI_DSI_DSC_NATIVE_420 BIT(3) >>> #define MIPI_DSI_DSC_FRAC_BPP BIT(4) >>> // etc. >>> >>> struct mipi_dsi_host { >>> Â // new fields only >>> Â Â unsigned long mode_flags; >>> Â Â unsigned long dsc_flags; >>> }; >>> >>> Then the panel driver can adapt itself to the host capabilities and >>> (possibly) select one of the internally supported DSC profiles. >>> >> >> I completely agree about extending mipi_dsi_host, other SoCs could reuse that and >> support for DSC panels would become a lot cleaner. > > Sounds good. I will wait for one or two more days (to get the possible feedback on fields/flags/etc) and post an RFC patch to dri-devel. Good, I was waiting until a DSC panel appears on the list (and I failed to be the first), it's now the case. For VTRD6130, the panel is capable of the 4 modes: - video mode - command mode - video mode & DSC - command mode & DSC So it would need such info to enable one of the mode in some order to determine. Thanks, Neil > >> >> For example, on MediaTek DRM there's some support for DSC, more or less the same >> for SPRD DRM and some DSI bridge drivers... having a clean infrastructure would >> definitely help. >> >> I'm sad I cannot offer testing in that case because despite being sure that there >> are MTK smartphones around with DSI panels using DSC, I have none... and all of the >> Chromebooks are not using DSC anyway (but using DisplayPort compression, which is >> obviously an entirely different beast). >> >>>> >>>> ... >>>> static int sony_akatsuki_lgd_probe(struct mipi_dsi_device *dsi) >>>> ... >>>> Â Â Â Â Â Â Â Â const struct of_device_id *match; >>>> >>>> ... >>>> Â Â Â Â Â Â Â Â match = of_match_node(panel_of_dsc_params, of_root); >>>> Â Â Â Â Â Â Â Â if (match && match->data) { >>>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â dsi->dsc = devm_kzalloc(&dsi->dev, sizeof(*dsc), GFP_KERNEL); >>>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â memcpy(dsi->dsc, match->data, sizeof(*dsc)); >>>> Â Â Â Â Â Â Â Â } else { >>>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â dev_warn(&dsi->dev, "DSI controller is not marked as supporting DSC\n"); >>>> Â Â Â Â Â Â Â Â } >>>> ... >>>> } >>>> >>>> and probably bail out if it's a DSC only panel. >>>> >> >> Usually DDICs support both DSC and non-DSC modes, depending on the initial >> programming (read: init commands)... but the usual issue is that many DDICs >> are not publicly documented for reasons, so yes, bailing out if DSC is not >> supported would be the only option, and would be fine at this point. >> >> Cheers, >> Angelo >> >>>> We could alternatively match on the DSI controller's dsi->host->dev instead of the SoC root compatible. >>>> >>>> Neil >>> >> >
On 5/29/2023 3:18 PM, Dmitry Baryshkov wrote: > On 30/05/2023 00:07, Marijn Suijten wrote: >> On 2023-05-22 15:58:56, Dmitry Baryshkov wrote: >>> On Mon, 22 May 2023 at 12:04, Neil Armstrong >>> <neil.armstrong@linaro.org> wrote: >>>> >>>> On 22/05/2023 03:16, Dmitry Baryshkov wrote: >>>>> On 22/05/2023 00:23, Marijn Suijten wrote: >>>>>> Sony provides an unlabeled LGD + Atmel maXTouch assembly in its >>>>>> Xperia >>>>>> XZ3 (tama akatsuki) phone, with custom DCS commands to match. >>>>>> >>>>>> This panel features Display Stream Compression 1.1. >>>>>> >>>>>> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org> >>>>>> --- >>>>>>   drivers/gpu/drm/panel/Kconfig                  | 11 + >>>>>>   drivers/gpu/drm/panel/Makefile                 |  1 + >>>>>>   drivers/gpu/drm/panel/panel-sony-akatsuki-lgd.c | 362 >>>>>> ++++++++++++++++++++++++ >>>>>>   3 files changed, 374 insertions(+) >>>>>> >>>>>> diff --git a/drivers/gpu/drm/panel/Kconfig >>>>>> b/drivers/gpu/drm/panel/Kconfig >>>>>> index 67ef898d133f2..18bd116e78a71 100644 >>>>>> --- a/drivers/gpu/drm/panel/Kconfig >>>>>> +++ b/drivers/gpu/drm/panel/Kconfig >>>>>> @@ -706,6 +706,17 @@ config DRM_PANEL_SONY_ACX565AKM >>>>>>         Say Y here if you want to enable support for the Sony >>>>>> ACX565AKM >>>>>>         800x600 3.5" panel (found on the Nokia N900). >>>>>> +config DRM_PANEL_SONY_AKATSUKI_LGD >>>>>> +   tristate "Sony Xperia XZ3 LGD panel" >>>>>> +   depends on GPIOLIB && OF >>>>>> +   depends on DRM_MIPI_DSI >>>>>> +   depends on BACKLIGHT_CLASS_DEVICE >>>>>> +   help >>>>>> +     Say Y here if you want to enable support for the Sony >>>>>> Xperia XZ3 >>>>>> +     1440x2880@60 6.0" OLED DSI cmd mode panel produced by LG >>>>>> Display. >>>>>> + >>>>>> +     This panel uses Display Stream Compression 1.1. >>>>>> + >>>>>>   config DRM_PANEL_SONY_TD4353_JDI >>>>>>       tristate "Sony TD4353 JDI panel" >>>>>>       depends on GPIOLIB && OF >>>>>> diff --git a/drivers/gpu/drm/panel/Makefile >>>>>> b/drivers/gpu/drm/panel/Makefile >>>>>> index ff169781e82d7..85133f73558f3 100644 >>>>>> --- a/drivers/gpu/drm/panel/Makefile >>>>>> +++ b/drivers/gpu/drm/panel/Makefile >>>>>> @@ -71,6 +71,7 @@ obj-$(CONFIG_DRM_PANEL_SITRONIX_ST7701) += >>>>>> panel-sitronix-st7701.o >>>>>>   obj-$(CONFIG_DRM_PANEL_SITRONIX_ST7703) += panel-sitronix-st7703.o >>>>>>   obj-$(CONFIG_DRM_PANEL_SITRONIX_ST7789V) += >>>>>> panel-sitronix-st7789v.o >>>>>>   obj-$(CONFIG_DRM_PANEL_SONY_ACX565AKM) += panel-sony-acx565akm.o >>>>>> +obj-$(CONFIG_DRM_PANEL_SONY_AKATSUKI_LGD) += >>>>>> panel-sony-akatsuki-lgd.o >>>>>>   obj-$(CONFIG_DRM_PANEL_SONY_TD4353_JDI) += panel-sony-td4353-jdi.o >>>>>>   obj-$(CONFIG_DRM_PANEL_SONY_TULIP_TRULY_NT35521) += >>>>>> panel-sony-tulip-truly-nt35521.o >>>>>>   obj-$(CONFIG_DRM_PANEL_TDO_TL070WSH30) += panel-tdo-tl070wsh30.o >>>>>> diff --git a/drivers/gpu/drm/panel/panel-sony-akatsuki-lgd.c >>>>>> b/drivers/gpu/drm/panel/panel-sony-akatsuki-lgd.c >>>>>> new file mode 100644 >>>>>> index 0000000000000..f55788f963dab >>>>>> --- /dev/null >>>>>> +++ b/drivers/gpu/drm/panel/panel-sony-akatsuki-lgd.c >>>>>> @@ -0,0 +1,362 @@ >>>>>> +// SPDX-License-Identifier: GPL-2.0-only >>>>>> +/* >>>>>> + * Copyright (c) 2023 Marijn Suijten <marijn.suijten@somainline.org> >>>>>> + * >>>>>> + * Based on Sony Downstream's "Atmel LGD ID5" Akatsuki panel dtsi. >>>>>> + */ >>>>>> + >>>>>> +#include <linux/backlight.h> >>>>>> +#include <linux/delay.h> >>>>>> +#include <linux/gpio/consumer.h> >>>>>> +#include <linux/module.h> >>>>>> +#include <linux/of.h> >>>>>> +#include <linux/of_device.h> >>>>>> +#include <linux/regulator/consumer.h> >>>>>> + >>>>>> +#include <video/mipi_display.h> >>>>>> + >>>>>> +#include <drm/drm_mipi_dsi.h> >>>>>> +#include <drm/drm_modes.h> >>>>>> +#include <drm/drm_panel.h> >>>>>> +#include <drm/drm_probe_helper.h> >>>>>> +#include <drm/display/drm_dsc.h> >>>>>> +#include <drm/display/drm_dsc_helper.h> >>>>>> + >>>>>> +struct sony_akatsuki_lgd { >>>>>> +   struct drm_panel panel; >>>>>> +   struct mipi_dsi_device *dsi; >>>>>> +   struct regulator *vddio; >>>>>> +   struct gpio_desc *reset_gpio; >>>>>> +   bool prepared; >>>>>> +}; >>>>>> + >>>>>> +static inline struct sony_akatsuki_lgd >>>>>> *to_sony_akatsuki_lgd(struct drm_panel *panel) >>>>>> +{ >>>>>> +   return container_of(panel, struct sony_akatsuki_lgd, panel); >>>>>> +} >>>>>> + >>>>>> +static int sony_akatsuki_lgd_on(struct sony_akatsuki_lgd *ctx) >>>>>> +{ >>>>>> +   struct mipi_dsi_device *dsi = ctx->dsi; >>>>>> +   struct device *dev = &dsi->dev; >>>>>> +   int ret; >>>>>> + >>>>>> +   dsi->mode_flags |= MIPI_DSI_MODE_LPM; >>>>>> + >>>>>> +   mipi_dsi_dcs_write_seq(dsi, 0x7f, 0x5a, 0x5a); >>>>>> +   mipi_dsi_dcs_write_seq(dsi, 0xf0, 0x5a, 0x5a); >>>>>> +   mipi_dsi_dcs_write_seq(dsi, 0xf1, 0x5a, 0x5a); >>>>>> +   mipi_dsi_dcs_write_seq(dsi, 0xf2, 0x5a, 0x5a); >>>>>> +   mipi_dsi_dcs_write_seq(dsi, 0x02, 0x01); >>>>>> +   mipi_dsi_dcs_write_seq(dsi, 0x59, 0x01); >>>>>> +   /* Enable backlight control */ >>>>>> +   mipi_dsi_dcs_write_seq(dsi, MIPI_DCS_WRITE_CONTROL_DISPLAY, >>>>>> BIT(5)); >>>>>> +   mipi_dsi_dcs_write_seq(dsi, 0x57, 0x20, 0x80, 0xde, 0x60, 0x00); >>>>>> + >>>>>> +   ret = mipi_dsi_dcs_set_column_address(dsi, 0, 1440 - 1); >>>>>> +   if (ret < 0) { >>>>>> +       dev_err(dev, "Failed to set column address: %d\n", ret); >>>>>> +       return ret; >>>>>> +   } >>>>>> + >>>>>> +   ret = mipi_dsi_dcs_set_page_address(dsi, 0, 2880 - 1); >>>>>> +   if (ret < 0) { >>>>>> +       dev_err(dev, "Failed to set page address: %d\n", ret); >>>>>> +       return ret; >>>>>> +   } >>>>>> + >>>>>> +   mipi_dsi_dcs_write_seq(dsi, MIPI_DCS_WRITE_POWER_SAVE, 0x00); >>>>>> + >>>>>> +   ret = mipi_dsi_dcs_set_tear_on(dsi, >>>>>> MIPI_DSI_DCS_TEAR_MODE_VBLANK); >>>>>> +   if (ret < 0) { >>>>>> +       dev_err(dev, "Failed to set tear on: %d\n", ret); >>>>>> +       return ret; >>>>>> +   } >>>>>> + >>>>>> +   mipi_dsi_dcs_write_seq(dsi, 0x7f, 0x5a, 0x5a); >>>>>> +   mipi_dsi_dcs_write_seq(dsi, 0xf0, 0x5a, 0x5a); >>>>>> +   mipi_dsi_dcs_write_seq(dsi, 0xf1, 0x5a, 0x5a); >>>>>> +   mipi_dsi_dcs_write_seq(dsi, 0xf2, 0x5a, 0x5a); >>>>>> +   mipi_dsi_dcs_write_seq(dsi, 0xb0, 0x03); >>>>>> +   mipi_dsi_dcs_write_seq(dsi, 0xf6, 0x04); >>>>>> +   mipi_dsi_dcs_write_seq(dsi, 0xb0, 0x05); >>>>>> +   mipi_dsi_dcs_write_seq(dsi, 0xf6, 0x01, 0x7f, 0x00); >>>>>> + >>>>>> +   ret = mipi_dsi_dcs_exit_sleep_mode(dsi); >>>>>> +   if (ret < 0) { >>>>>> +       dev_err(dev, "Failed to exit sleep mode: %d\n", ret); >>>>>> +       return ret; >>>>>> +   } >>>>>> +   msleep(120); >>>>>> + >>>>>> +   mipi_dsi_dcs_write_seq(dsi, 0xe3, 0xac, 0x19, 0x34, 0x14, 0x7d); >>>>>> + >>>>>> +   ret = mipi_dsi_dcs_set_display_on(dsi); >>>>>> +   if (ret < 0) { >>>>>> +       dev_err(dev, "Failed to turn display on: %d\n", ret); >>>>>> +       return ret; >>>>>> +   } >>>>> >>>>> My usual question: should the mipi_dsi_dcs_exit_sleep_mode() / >>>>> mipi_dsi_dcs_set_display_on() be moved from prepare() to enable() >>>>> part? >>>> >>>> >>>> No, prepare is called before the video stream is started and when >>>> display is still in LPM mode and the mode hasn't been set. >>>> >>> >>> Yes, that's my point. Shouldn't we enable the panel _after_ starting >>> the stream? >> >> I have never investigated what it takes to split these functions, but >> some of these panels do show some corruption at startup which may be >> circumvented by powering the panel on after starting the video stream? >> >> I'm just not sure where to make the split: downstream does describe a >> qcom,mdss-dsi-on-command and qcom,mdss-dsi-post-panel-on-command, where >> the latter only contains set_display_on() (not exit_sleep_mode()). >> It is documented like: >> >>     same as "qcom,mdss-dsi-on-command" except commands are sent after >>     displaying an image." >> >> So this seems like the right way to split them up, I'll test this out on >> all submitted panel drivers. > > Interesting enough, Neil suggested that sending all the commands during > pre_enable() is the correct sequence (especially for VIDEO mode panels), > since not all DSI hosts can send commands after switching to the VIDEO > mode. > I agree with Neil here. Yes, it does seem natural to think that sending the video stream before sending the on commands would avoid any potential corruption / garbage screen issues. But even from panel side should allow that. I have seen panel ON sequences where some explicitly ask for ON commands before the video stream. So, we cannot really generalize it and needs to be treated on a host-to-host and panel-to-panel basis.
On 2023-05-30 14:11:06, Dmitry Baryshkov wrote: > On Tue, 30 May 2023 at 11:27, Marijn Suijten > <marijn.suijten@somainline.org> wrote: > > > > On 2023-05-30 01:39:10, Dmitry Baryshkov wrote: > > > On 30/05/2023 01:37, Marijn Suijten wrote: > > > > On 2023-05-30 01:18:40, Dmitry Baryshkov wrote: > > > > <snip> > > > >>>>>>> + ret = mipi_dsi_dcs_set_display_on(dsi); > > > >>>>>>> + if (ret < 0) { > > > >>>>>>> + dev_err(dev, "Failed to turn display on: %d\n", ret); > > > >>>>>>> + return ret; > > > >>>>>>> + } > > > >>>>>> > > > >>>>>> My usual question: should the mipi_dsi_dcs_exit_sleep_mode() / mipi_dsi_dcs_set_display_on() be moved from prepare() to enable() part? > > > >>>>> > > > >>>>> > > > >>>>> No, prepare is called before the video stream is started and when display is still in LPM mode and the mode hasn't been set. > > > >>>>> > > > >>>> > > > >>>> Yes, that's my point. Shouldn't we enable the panel _after_ starting the stream? > > > >>> > > > >>> I have never investigated what it takes to split these functions, but > > > >>> some of these panels do show some corruption at startup which may be > > > >>> circumvented by powering the panel on after starting the video stream? > > > >>> > > > >>> I'm just not sure where to make the split: downstream does describe a > > > >>> qcom,mdss-dsi-on-command and qcom,mdss-dsi-post-panel-on-command, where > > > >>> the latter only contains set_display_on() (not exit_sleep_mode()). > > > >>> It is documented like: > > > >>> > > > >>> same as "qcom,mdss-dsi-on-command" except commands are sent after > > > >>> displaying an image." > > > >>> > > > >>> So this seems like the right way to split them up, I'll test this out on > > > >>> all submitted panel drivers. > > > >> > > > >> Interesting enough, Neil suggested that sending all the commands during > > > >> pre_enable() is the correct sequence (especially for VIDEO mode panels), > > > >> since not all DSI hosts can send commands after switching to the VIDEO mode. > > > > > > > > Note that all these panels and Driver-ICs are command-mode, and/or > > > > programmed to run in command-mode, so there shouldn't be any notion of a > > > > VIDEO stream (any command-mode frame is just an "arbitrary command" as > > > > far as I understood). > > > > > > Yes, from the data stream point of view. I was talking about the DSI > > > host being able to send arbitrary commands or not after enabling the > > > video/cmd stream. > > > > Is this a known limitation of SM8250 then? Or is the brightness_set > > issue more likely a "problem" with the panel that the downstream kernel > > is "somehow" working around or aware of, and I just haven't found > > how/where it deals with that? > > (Alternatively we could be "doing it wrong" for other panels but it > > turns out to be working anyway) > > Please excuse me for not being explicit enough. Qualcomm hardware > doesn't have this problem. Thus I was completely unaware of it before > talking to Neil. > So, our hardware works in most of the cases. Also excuse me for mocking the hardware here; it seems quite illogical for it to not work on this specific device which is more likely a failure in porting the panel DT to the driver than related to this specific SoC. There's probably one of the hundred-or-so DT params responsible for triggering a setting, delay, or other magic sequence that gets the brightness toggle working. - Marijn
On 30/05/2023 21:13, Marijn Suijten wrote: > On 2023-05-30 10:54:17, Abhinav Kumar wrote: >>> On 30/05/2023 00:07, Marijn Suijten wrote: >>>> On 2023-05-22 15:58:56, Dmitry Baryshkov wrote: >>>>> On Mon, 22 May 2023 at 12:04, Neil Armstrong >>>>> <neil.armstrong@linaro.org> wrote: >>>>>> >>>>>> On 22/05/2023 03:16, Dmitry Baryshkov wrote: >>>>>>> On 22/05/2023 00:23, Marijn Suijten wrote: >>>>>>>> Sony provides an unlabeled LGD + Atmel maXTouch assembly in its >>>>>>>> Xperia >>>>>>>> XZ3 (tama akatsuki) phone, with custom DCS commands to match. >>>>>>>> >>>>>>>> This panel features Display Stream Compression 1.1. >>>>>>>> >>>>>>>> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org> >>>>>>>> --- >>>>>>>>   drivers/gpu/drm/panel/Kconfig                  | 11 + >>>>>>>>   drivers/gpu/drm/panel/Makefile                 |  1 + >>>>>>>>   drivers/gpu/drm/panel/panel-sony-akatsuki-lgd.c | 362 >>>>>>>> ++++++++++++++++++++++++ >>>>>>>>   3 files changed, 374 insertions(+) >>>>>>>> >>>>>>>> diff --git a/drivers/gpu/drm/panel/Kconfig >>>>>>>> b/drivers/gpu/drm/panel/Kconfig >>>>>>>> index 67ef898d133f2..18bd116e78a71 100644 >>>>>>>> --- a/drivers/gpu/drm/panel/Kconfig >>>>>>>> +++ b/drivers/gpu/drm/panel/Kconfig >>>>>>>> @@ -706,6 +706,17 @@ config DRM_PANEL_SONY_ACX565AKM >>>>>>>>         Say Y here if you want to enable support for the Sony >>>>>>>> ACX565AKM >>>>>>>>         800x600 3.5" panel (found on the Nokia N900). >>>>>>>> +config DRM_PANEL_SONY_AKATSUKI_LGD >>>>>>>> +   tristate "Sony Xperia XZ3 LGD panel" >>>>>>>> +   depends on GPIOLIB && OF >>>>>>>> +   depends on DRM_MIPI_DSI >>>>>>>> +   depends on BACKLIGHT_CLASS_DEVICE >>>>>>>> +   help >>>>>>>> +     Say Y here if you want to enable support for the Sony >>>>>>>> Xperia XZ3 >>>>>>>> +     1440x2880@60 6.0" OLED DSI cmd mode panel produced by LG >>>>>>>> Display. >>>>>>>> + >>>>>>>> +     This panel uses Display Stream Compression 1.1. >>>>>>>> + >>>>>>>>   config DRM_PANEL_SONY_TD4353_JDI >>>>>>>>       tristate "Sony TD4353 JDI panel" >>>>>>>>       depends on GPIOLIB && OF >>>>>>>> diff --git a/drivers/gpu/drm/panel/Makefile >>>>>>>> b/drivers/gpu/drm/panel/Makefile >>>>>>>> index ff169781e82d7..85133f73558f3 100644 >>>>>>>> --- a/drivers/gpu/drm/panel/Makefile >>>>>>>> +++ b/drivers/gpu/drm/panel/Makefile >>>>>>>> @@ -71,6 +71,7 @@ obj-$(CONFIG_DRM_PANEL_SITRONIX_ST7701) += >>>>>>>> panel-sitronix-st7701.o >>>>>>>>   obj-$(CONFIG_DRM_PANEL_SITRONIX_ST7703) += panel-sitronix-st7703.o >>>>>>>>   obj-$(CONFIG_DRM_PANEL_SITRONIX_ST7789V) += >>>>>>>> panel-sitronix-st7789v.o >>>>>>>>   obj-$(CONFIG_DRM_PANEL_SONY_ACX565AKM) += panel-sony-acx565akm.o >>>>>>>> +obj-$(CONFIG_DRM_PANEL_SONY_AKATSUKI_LGD) += >>>>>>>> panel-sony-akatsuki-lgd.o >>>>>>>>   obj-$(CONFIG_DRM_PANEL_SONY_TD4353_JDI) += panel-sony-td4353-jdi.o >>>>>>>>   obj-$(CONFIG_DRM_PANEL_SONY_TULIP_TRULY_NT35521) += >>>>>>>> panel-sony-tulip-truly-nt35521.o >>>>>>>>   obj-$(CONFIG_DRM_PANEL_TDO_TL070WSH30) += panel-tdo-tl070wsh30.o >>>>>>>> diff --git a/drivers/gpu/drm/panel/panel-sony-akatsuki-lgd.c >>>>>>>> b/drivers/gpu/drm/panel/panel-sony-akatsuki-lgd.c >>>>>>>> new file mode 100644 >>>>>>>> index 0000000000000..f55788f963dab >>>>>>>> --- /dev/null >>>>>>>> +++ b/drivers/gpu/drm/panel/panel-sony-akatsuki-lgd.c >>>>>>>> @@ -0,0 +1,362 @@ >>>>>>>> +// SPDX-License-Identifier: GPL-2.0-only >>>>>>>> +/* >>>>>>>> + * Copyright (c) 2023 Marijn Suijten <marijn.suijten@somainline.org> >>>>>>>> + * >>>>>>>> + * Based on Sony Downstream's "Atmel LGD ID5" Akatsuki panel dtsi. >>>>>>>> + */ >>>>>>>> + >>>>>>>> +#include <linux/backlight.h> >>>>>>>> +#include <linux/delay.h> >>>>>>>> +#include <linux/gpio/consumer.h> >>>>>>>> +#include <linux/module.h> >>>>>>>> +#include <linux/of.h> >>>>>>>> +#include <linux/of_device.h> >>>>>>>> +#include <linux/regulator/consumer.h> >>>>>>>> + >>>>>>>> +#include <video/mipi_display.h> >>>>>>>> + >>>>>>>> +#include <drm/drm_mipi_dsi.h> >>>>>>>> +#include <drm/drm_modes.h> >>>>>>>> +#include <drm/drm_panel.h> >>>>>>>> +#include <drm/drm_probe_helper.h> >>>>>>>> +#include <drm/display/drm_dsc.h> >>>>>>>> +#include <drm/display/drm_dsc_helper.h> >>>>>>>> + >>>>>>>> +struct sony_akatsuki_lgd { >>>>>>>> +   struct drm_panel panel; >>>>>>>> +   struct mipi_dsi_device *dsi; >>>>>>>> +   struct regulator *vddio; >>>>>>>> +   struct gpio_desc *reset_gpio; >>>>>>>> +   bool prepared; >>>>>>>> +}; >>>>>>>> + >>>>>>>> +static inline struct sony_akatsuki_lgd >>>>>>>> *to_sony_akatsuki_lgd(struct drm_panel *panel) >>>>>>>> +{ >>>>>>>> +   return container_of(panel, struct sony_akatsuki_lgd, panel); >>>>>>>> +} >>>>>>>> + >>>>>>>> +static int sony_akatsuki_lgd_on(struct sony_akatsuki_lgd *ctx) >>>>>>>> +{ >>>>>>>> +   struct mipi_dsi_device *dsi = ctx->dsi; >>>>>>>> +   struct device *dev = &dsi->dev; >>>>>>>> +   int ret; >>>>>>>> + >>>>>>>> +   dsi->mode_flags |= MIPI_DSI_MODE_LPM; >>>>>>>> + >>>>>>>> +   mipi_dsi_dcs_write_seq(dsi, 0x7f, 0x5a, 0x5a); >>>>>>>> +   mipi_dsi_dcs_write_seq(dsi, 0xf0, 0x5a, 0x5a); >>>>>>>> +   mipi_dsi_dcs_write_seq(dsi, 0xf1, 0x5a, 0x5a); >>>>>>>> +   mipi_dsi_dcs_write_seq(dsi, 0xf2, 0x5a, 0x5a); >>>>>>>> +   mipi_dsi_dcs_write_seq(dsi, 0x02, 0x01); >>>>>>>> +   mipi_dsi_dcs_write_seq(dsi, 0x59, 0x01); >>>>>>>> +   /* Enable backlight control */ >>>>>>>> +   mipi_dsi_dcs_write_seq(dsi, MIPI_DCS_WRITE_CONTROL_DISPLAY, >>>>>>>> BIT(5)); >>>>>>>> +   mipi_dsi_dcs_write_seq(dsi, 0x57, 0x20, 0x80, 0xde, 0x60, 0x00); >>>>>>>> + >>>>>>>> +   ret = mipi_dsi_dcs_set_column_address(dsi, 0, 1440 - 1); >>>>>>>> +   if (ret < 0) { >>>>>>>> +       dev_err(dev, "Failed to set column address: %d\n", ret); >>>>>>>> +       return ret; >>>>>>>> +   } >>>>>>>> + >>>>>>>> +   ret = mipi_dsi_dcs_set_page_address(dsi, 0, 2880 - 1); >>>>>>>> +   if (ret < 0) { >>>>>>>> +       dev_err(dev, "Failed to set page address: %d\n", ret); >>>>>>>> +       return ret; >>>>>>>> +   } >>>>>>>> + >>>>>>>> +   mipi_dsi_dcs_write_seq(dsi, MIPI_DCS_WRITE_POWER_SAVE, 0x00); >>>>>>>> + >>>>>>>> +   ret = mipi_dsi_dcs_set_tear_on(dsi, >>>>>>>> MIPI_DSI_DCS_TEAR_MODE_VBLANK); >>>>>>>> +   if (ret < 0) { >>>>>>>> +       dev_err(dev, "Failed to set tear on: %d\n", ret); >>>>>>>> +       return ret; >>>>>>>> +   } >>>>>>>> + >>>>>>>> +   mipi_dsi_dcs_write_seq(dsi, 0x7f, 0x5a, 0x5a); >>>>>>>> +   mipi_dsi_dcs_write_seq(dsi, 0xf0, 0x5a, 0x5a); >>>>>>>> +   mipi_dsi_dcs_write_seq(dsi, 0xf1, 0x5a, 0x5a); >>>>>>>> +   mipi_dsi_dcs_write_seq(dsi, 0xf2, 0x5a, 0x5a); >>>>>>>> +   mipi_dsi_dcs_write_seq(dsi, 0xb0, 0x03); >>>>>>>> +   mipi_dsi_dcs_write_seq(dsi, 0xf6, 0x04); >>>>>>>> +   mipi_dsi_dcs_write_seq(dsi, 0xb0, 0x05); >>>>>>>> +   mipi_dsi_dcs_write_seq(dsi, 0xf6, 0x01, 0x7f, 0x00); >>>>>>>> + >>>>>>>> +   ret = mipi_dsi_dcs_exit_sleep_mode(dsi); >>>>>>>> +   if (ret < 0) { >>>>>>>> +       dev_err(dev, "Failed to exit sleep mode: %d\n", ret); >>>>>>>> +       return ret; >>>>>>>> +   } >>>>>>>> +   msleep(120); >>>>>>>> + >>>>>>>> +   mipi_dsi_dcs_write_seq(dsi, 0xe3, 0xac, 0x19, 0x34, 0x14, 0x7d); >>>>>>>> + >>>>>>>> +   ret = mipi_dsi_dcs_set_display_on(dsi); >>>>>>>> +   if (ret < 0) { >>>>>>>> +       dev_err(dev, "Failed to turn display on: %d\n", ret); >>>>>>>> +       return ret; >>>>>>>> +   } >>>>>>> >>>>>>> My usual question: should the mipi_dsi_dcs_exit_sleep_mode() / >>>>>>> mipi_dsi_dcs_set_display_on() be moved from prepare() to enable() >>>>>>> part? >>>>>> >>>>>> >>>>>> No, prepare is called before the video stream is started and when >>>>>> display is still in LPM mode and the mode hasn't been set. >>>>>> >>>>> >>>>> Yes, that's my point. Shouldn't we enable the panel _after_ starting >>>>> the stream? >>>> >>>> I have never investigated what it takes to split these functions, but >>>> some of these panels do show some corruption at startup which may be >>>> circumvented by powering the panel on after starting the video stream? >>>> >>>> I'm just not sure where to make the split: downstream does describe a >>>> qcom,mdss-dsi-on-command and qcom,mdss-dsi-post-panel-on-command, where >>>> the latter only contains set_display_on() (not exit_sleep_mode()). >>>> It is documented like: >>>> >>>>     same as "qcom,mdss-dsi-on-command" except commands are sent after >>>>     displaying an image." >>>> >>>> So this seems like the right way to split them up, I'll test this out on >>>> all submitted panel drivers. >>> >>> Interesting enough, Neil suggested that sending all the commands during >>> pre_enable() is the correct sequence (especially for VIDEO mode panels), >>> since not all DSI hosts can send commands after switching to the VIDEO >>> mode. >>> >> >> I agree with Neil here. >> >> Yes, it does seem natural to think that sending the video stream before >> sending the on commands would avoid any potential corruption / garbage >> screen issues. >> >> But even from panel side should allow that. I have seen panel ON >> sequences where some explicitly ask for ON commands before the video stream. >> >> So, we cannot really generalize it and needs to be treated on a >> host-to-host and panel-to-panel basis. > > All four panel drivers in this series have a `post-panel-on` separation > (containing _just_ the panel_on DCS) and should be implemented with a > separate .enable callback. > > More importantly: is the API to enable/disable, and separately > prepare/unprepare the panel exposed to userspace? And is there an app > (maybe as part of mesa/drm where modetest lives) that is able to trigger > these calls to test adequate behaviour? In the past I've seen panels > working, until my userspace suspends and turn the panel off, where it > never comes back up properly after. No. The prepare/enable (and disable/unrepare) translate to the bridge's pre_enable/enable and disable/post_disable callbacks, which are a part of modesetting. One can not call them separately.
Il 30/05/23 17:44, Neil Armstrong ha scritto: > On 30/05/2023 14:36, Dmitry Baryshkov wrote: >> On 30/05/2023 15:15, AngeloGioacchino Del Regno wrote: >>> Il 30/05/23 13:44, Dmitry Baryshkov ha scritto: >>>> On Tue, 30 May 2023 at 10:24, Neil Armstrong <neil.armstrong@linaro.org> wrote: >>>>> >>>>> Hi Marijn, Dmitry, Caleb, Jessica, >>>>> >>>>> On 29/05/2023 23:11, Marijn Suijten wrote: >>>>>> On 2023-05-22 04:16:20, Dmitry Baryshkov wrote: >>>>>> <snip> >>>>>>>> +Â Â if (ctx->dsi->dsc) { >>>>>>> >>>>>>> dsi->dsc is always set, thus this condition can be dropped. >>>>>> >>>>>> I want to leave room for possibly running the panel without DSC (at a >>>>>> lower resolution/refresh rate, or at higher power consumption if there >>>>>> is enough BW) by not assigning the pointer, if we get access to panel >>>>>> documentation: probably one of the magic commands sent in this driver >>>>>> controls it but we don't know which. >>>>> >>>>> I'd like to investigate if DSC should perhaps only be enabled if we >>>>> run non certain platforms/socs ? >>>>> >>>>> I mean, we don't know if the controller supports DSC and those particular >>>>> DSC parameters so we should probably start adding something like : >>>>> >>>>> static drm_dsc_config dsc_params_qcom = {} >>>>> >>>>> static const struct of_device_id panel_of_dsc_params[] = { >>>>> Â Â Â Â Â Â Â Â { .compatible = "qcom,sm8150", , .data = &dsc_params_qcom }, >>>>> Â Â Â Â Â Â Â Â { .compatible = "qcom,sm8250", , .data = &dsc_params_qcom }, >>>>> Â Â Â Â Â Â Â Â { .compatible = "qcom,sm8350", , .data = &dsc_params_qcom }, >>>>> Â Â Â Â Â Â Â Â { .compatible = "qcom,sm8450", , .data = &dsc_params_qcom }, >>>>> }; >>>> >>>> I think this would damage the reusability of the drivers. The panel >>>> driver does not actually care if the SoC is SM8350, sunxi-something or >>>> RCar. >>>> Instead it cares about host capabilities. >>>> >>>> I think instead we should extend mipi_dsi_host: >>>> >>>> #define MIPI_DSI_HOST_MODE_VIDEO BIT(0) > > I assume all DSI controller supports Video mode, so it should be a negative here > if for a reason it's not the case. Either all positive or all negative... and yes I agree that all DSI controllers support video mode nowadays, but: - Will that be true for future controllers? (likely yes, but you never know) - Is there any controller driver not implementing video mode? - Will there be one in the future? > > There should also be a flag to tell if sending LP commands sending while > in HS Video mode is supported. > +1. This is the case for both qcom and mtk. >>>> #define MIPI_DSI_HOST_MODE_CMDÂ BIT(1) >>>> #define MIPI_DSI_HOST_VIDEO_SUPPORTS_COMMANDS BIT(2) >>>> // FIXME: do we need to provide additional caps here ? >>>> >>>> #define MIPI_DSI_DSC_1_1 BIT(0) >>>> #define MIPI_DSI_DSC_1_2 BIT(1) >>>> #define MIPI_DSI_DSC_NATIVE_422 BIT(2) >>>> #define MIPI_DSI_DSC_NATIVE_420 BIT(3) >>>> #define MIPI_DSI_DSC_FRAC_BPP BIT(4) >>>> // etc. >>>> >>>> struct mipi_dsi_host { >>>> Â // new fields only >>>> Â Â unsigned long mode_flags; >>>> Â Â unsigned long dsc_flags; >>>> }; >>>> >>>> Then the panel driver can adapt itself to the host capabilities and >>>> (possibly) select one of the internally supported DSC profiles. >>>> >>> >>> I completely agree about extending mipi_dsi_host, other SoCs could reuse that and >>> support for DSC panels would become a lot cleaner. >> >> Sounds good. I will wait for one or two more days (to get the possible feedback >> on fields/flags/etc) and post an RFC patch to dri-devel. > > Good, I was waiting until a DSC panel appears on the list (and I failed to be the > first), it's now the case. > > For VTRD6130, the panel is capable of the 4 modes: > - video mode > - command mode > - video mode & DSC > - command mode & DSC > > So it would need such info to enable one of the mode in some order to determine. > Dynamically determining is not trivial, as that depends on multiple variables: - Availability of the modes (obviously) - Available lanes - Available bandwidth per lane - Available total bandwidth - Power consumption considerations (DSC IP may be using more or less power depending on the actual SoC//controller) - Thermal management: DSC may make no thermal sense as in, more heat output vs thermal envelope (laptop vs embedded vs handset) - Others Hence, the implementation should also provide a way of choosing a preferred mode on a per-controller basis (DSC or no compression). Just a few considerations that came to mind with a good sleep. Cheers! > Thanks, > Neil >> >>> >>> For example, on MediaTek DRM there's some support for DSC, more or less the same >>> for SPRD DRM and some DSI bridge drivers... having a clean infrastructure would >>> definitely help. >>> >>> I'm sad I cannot offer testing in that case because despite being sure that there >>> are MTK smartphones around with DSI panels using DSC, I have none... and all of the >>> Chromebooks are not using DSC anyway (but using DisplayPort compression, which is >>> obviously an entirely different beast). >>> >>>>> >>>>> ... >>>>> static int sony_akatsuki_lgd_probe(struct mipi_dsi_device *dsi) >>>>> ... >>>>> Â Â Â Â Â Â Â Â const struct of_device_id *match; >>>>> >>>>> ... >>>>> Â Â Â Â Â Â Â Â match = of_match_node(panel_of_dsc_params, of_root); >>>>> Â Â Â Â Â Â Â Â if (match && match->data) { >>>>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â dsi->dsc = devm_kzalloc(&dsi->dev, sizeof(*dsc), GFP_KERNEL); >>>>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â memcpy(dsi->dsc, match->data, sizeof(*dsc)); >>>>> Â Â Â Â Â Â Â Â } else { >>>>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â dev_warn(&dsi->dev, "DSI controller is not marked as >>>>> supporting DSC\n"); >>>>> Â Â Â Â Â Â Â Â } >>>>> ... >>>>> } >>>>> >>>>> and probably bail out if it's a DSC only panel. >>>>> >>> >>> Usually DDICs support both DSC and non-DSC modes, depending on the initial >>> programming (read: init commands)... but the usual issue is that many DDICs >>> are not publicly documented for reasons, so yes, bailing out if DSC is not >>> supported would be the only option, and would be fine at this point. >>> >>> Cheers, >>> Angelo >>> >>>>> We could alternatively match on the DSI controller's dsi->host->dev instead of >>>>> the SoC root compatible. >>>>> >>>>> Neil >>>> >>> >> >
Hi, On Tue, May 30, 2023 at 03:36:04PM +0300, Dmitry Baryshkov wrote: > On 30/05/2023 15:15, AngeloGioacchino Del Regno wrote: > > Il 30/05/23 13:44, Dmitry Baryshkov ha scritto: > > > On Tue, 30 May 2023 at 10:24, Neil Armstrong > > > <neil.armstrong@linaro.org> wrote: > > > > > > > > Hi Marijn, Dmitry, Caleb, Jessica, > > > > > > > > On 29/05/2023 23:11, Marijn Suijten wrote: > > > > > On 2023-05-22 04:16:20, Dmitry Baryshkov wrote: > > > > > <snip> > > > > > > > +Â Â if (ctx->dsi->dsc) { > > > > > > > > > > > > dsi->dsc is always set, thus this condition can be dropped. > > > > > > > > > > I want to leave room for possibly running the panel without DSC (at a > > > > > lower resolution/refresh rate, or at higher power consumption if there > > > > > is enough BW) by not assigning the pointer, if we get access to panel > > > > > documentation: probably one of the magic commands sent in this driver > > > > > controls it but we don't know which. > > > > > > > > I'd like to investigate if DSC should perhaps only be enabled if we > > > > run non certain platforms/socs ? > > > > > > > > I mean, we don't know if the controller supports DSC and those > > > > particular > > > > DSC parameters so we should probably start adding something like : > > > > > > > > static drm_dsc_config dsc_params_qcom = {} > > > > > > > > static const struct of_device_id panel_of_dsc_params[] = { > > > > Â Â Â Â Â Â Â Â { .compatible = "qcom,sm8150", , .data = &dsc_params_qcom }, > > > > Â Â Â Â Â Â Â Â { .compatible = "qcom,sm8250", , .data = &dsc_params_qcom }, > > > > Â Â Â Â Â Â Â Â { .compatible = "qcom,sm8350", , .data = &dsc_params_qcom }, > > > > Â Â Â Â Â Â Â Â { .compatible = "qcom,sm8450", , .data = &dsc_params_qcom }, > > > > }; > > > > > > I think this would damage the reusability of the drivers. The panel > > > driver does not actually care if the SoC is SM8350, sunxi-something or > > > RCar. > > > Instead it cares about host capabilities. > > > > > > I think instead we should extend mipi_dsi_host: > > > > > > #define MIPI_DSI_HOST_MODE_VIDEO BIT(0) > > > #define MIPI_DSI_HOST_MODE_CMDÂ BIT(1) > > > #define MIPI_DSI_HOST_VIDEO_SUPPORTS_COMMANDS BIT(2) > > > // FIXME: do we need to provide additional caps here ? > > > > > > #define MIPI_DSI_DSC_1_1 BIT(0) > > > #define MIPI_DSI_DSC_1_2 BIT(1) > > > #define MIPI_DSI_DSC_NATIVE_422 BIT(2) > > > #define MIPI_DSI_DSC_NATIVE_420 BIT(3) > > > #define MIPI_DSI_DSC_FRAC_BPP BIT(4) > > > // etc. > > > > > > struct mipi_dsi_host { > > > Â // new fields only > > > Â Â unsigned long mode_flags; > > > Â Â unsigned long dsc_flags; > > > }; > > > > > > Then the panel driver can adapt itself to the host capabilities and > > > (possibly) select one of the internally supported DSC profiles. > > > > > > > I completely agree about extending mipi_dsi_host, other SoCs could reuse > > that and > > support for DSC panels would become a lot cleaner. > > Sounds good. I will wait for one or two more days (to get the possible > feedback on fields/flags/etc) and post an RFC patch to dri-devel. I just came across that discussion, and couldn't find those patches, did you ever send them? Either way, I'm not really sure it's a good idea to multiply the capabilities flags of the DSI host, and we should just stick to the spec. If the spec says that we have to support DSC while video is output, then that's what the panels should expect. If a host isn't able to provide that, it's a bug and we should fix the controller driver instead of creating a workaround in the core for broken drivers. Another concern I have is that, those broken drivers are usually the undocumented ones that already have trouble supporting the most trivial setup. Creating more combinations both at the controller and panel level will just make it harder for those drivers. Maxime
On 05/07/2023 14:04, Maxime Ripard wrote: > Hi, > > On Tue, May 30, 2023 at 03:36:04PM +0300, Dmitry Baryshkov wrote: >> On 30/05/2023 15:15, AngeloGioacchino Del Regno wrote: >>> Il 30/05/23 13:44, Dmitry Baryshkov ha scritto: >>>> On Tue, 30 May 2023 at 10:24, Neil Armstrong >>>> <neil.armstrong@linaro.org> wrote: >>>>> >>>>> Hi Marijn, Dmitry, Caleb, Jessica, >>>>> >>>>> On 29/05/2023 23:11, Marijn Suijten wrote: >>>>>> On 2023-05-22 04:16:20, Dmitry Baryshkov wrote: >>>>>> <snip> >>>>>>>> +Â Â if (ctx->dsi->dsc) { >>>>>>> >>>>>>> dsi->dsc is always set, thus this condition can be dropped. >>>>>> >>>>>> I want to leave room for possibly running the panel without DSC (at a >>>>>> lower resolution/refresh rate, or at higher power consumption if there >>>>>> is enough BW) by not assigning the pointer, if we get access to panel >>>>>> documentation: probably one of the magic commands sent in this driver >>>>>> controls it but we don't know which. >>>>> >>>>> I'd like to investigate if DSC should perhaps only be enabled if we >>>>> run non certain platforms/socs ? >>>>> >>>>> I mean, we don't know if the controller supports DSC and those >>>>> particular >>>>> DSC parameters so we should probably start adding something like : >>>>> >>>>> static drm_dsc_config dsc_params_qcom = {} >>>>> >>>>> static const struct of_device_id panel_of_dsc_params[] = { >>>>> Â Â Â Â Â Â Â Â { .compatible = "qcom,sm8150", , .data = &dsc_params_qcom }, >>>>> Â Â Â Â Â Â Â Â { .compatible = "qcom,sm8250", , .data = &dsc_params_qcom }, >>>>> Â Â Â Â Â Â Â Â { .compatible = "qcom,sm8350", , .data = &dsc_params_qcom }, >>>>> Â Â Â Â Â Â Â Â { .compatible = "qcom,sm8450", , .data = &dsc_params_qcom }, >>>>> }; >>>> >>>> I think this would damage the reusability of the drivers. The panel >>>> driver does not actually care if the SoC is SM8350, sunxi-something or >>>> RCar. >>>> Instead it cares about host capabilities. >>>> >>>> I think instead we should extend mipi_dsi_host: >>>> >>>> #define MIPI_DSI_HOST_MODE_VIDEO BIT(0) >>>> #define MIPI_DSI_HOST_MODE_CMDÂ BIT(1) >>>> #define MIPI_DSI_HOST_VIDEO_SUPPORTS_COMMANDS BIT(2) >>>> // FIXME: do we need to provide additional caps here ? >>>> >>>> #define MIPI_DSI_DSC_1_1 BIT(0) >>>> #define MIPI_DSI_DSC_1_2 BIT(1) >>>> #define MIPI_DSI_DSC_NATIVE_422 BIT(2) >>>> #define MIPI_DSI_DSC_NATIVE_420 BIT(3) >>>> #define MIPI_DSI_DSC_FRAC_BPP BIT(4) >>>> // etc. >>>> >>>> struct mipi_dsi_host { >>>> Â // new fields only >>>> Â Â unsigned long mode_flags; >>>> Â Â unsigned long dsc_flags; >>>> }; >>>> >>>> Then the panel driver can adapt itself to the host capabilities and >>>> (possibly) select one of the internally supported DSC profiles. >>>> >>> >>> I completely agree about extending mipi_dsi_host, other SoCs could reuse >>> that and >>> support for DSC panels would become a lot cleaner. >> >> Sounds good. I will wait for one or two more days (to get the possible >> feedback on fields/flags/etc) and post an RFC patch to dri-devel. > > I just came across that discussion, and couldn't find those patches, did > you ever send them? > > Either way, I'm not really sure it's a good idea to multiply the > capabilities flags of the DSI host, and we should just stick to the > spec. If the spec says that we have to support DSC while video is > output, then that's what the panels should expect. Except some panels supports DSC & non-DSC, Video and Command mode, and all that is runtime configurable. How do you handle that ? > > If a host isn't able to provide that, it's a bug and we should fix the > controller driver instead of creating a workaround in the core for > broken drivers. > > Another concern I have is that, those broken drivers are usually the > undocumented ones that already have trouble supporting the most trivial > setup. Creating more combinations both at the controller and panel level > will just make it harder for those drivers. > > Maxime
On Wed, Jul 05, 2023 at 03:05:33PM +0200, Neil Armstrong wrote: > On 05/07/2023 14:04, Maxime Ripard wrote: > > Hi, > > > > On Tue, May 30, 2023 at 03:36:04PM +0300, Dmitry Baryshkov wrote: > > > On 30/05/2023 15:15, AngeloGioacchino Del Regno wrote: > > > > Il 30/05/23 13:44, Dmitry Baryshkov ha scritto: > > > > > On Tue, 30 May 2023 at 10:24, Neil Armstrong > > > > > <neil.armstrong@linaro.org> wrote: > > > > > > > > > > > > Hi Marijn, Dmitry, Caleb, Jessica, > > > > > > > > > > > > On 29/05/2023 23:11, Marijn Suijten wrote: > > > > > > > On 2023-05-22 04:16:20, Dmitry Baryshkov wrote: > > > > > > > <snip> > > > > > > > > > +Â Â if (ctx->dsi->dsc) { > > > > > > > > > > > > > > > > dsi->dsc is always set, thus this condition can be dropped. > > > > > > > > > > > > > > I want to leave room for possibly running the panel without DSC (at a > > > > > > > lower resolution/refresh rate, or at higher power consumption if there > > > > > > > is enough BW) by not assigning the pointer, if we get access to panel > > > > > > > documentation: probably one of the magic commands sent in this driver > > > > > > > controls it but we don't know which. > > > > > > > > > > > > I'd like to investigate if DSC should perhaps only be enabled if we > > > > > > run non certain platforms/socs ? > > > > > > > > > > > > I mean, we don't know if the controller supports DSC and those > > > > > > particular > > > > > > DSC parameters so we should probably start adding something like : > > > > > > > > > > > > static drm_dsc_config dsc_params_qcom = {} > > > > > > > > > > > > static const struct of_device_id panel_of_dsc_params[] = { > > > > > > Â Â Â Â Â Â Â Â { .compatible = "qcom,sm8150", , .data = &dsc_params_qcom }, > > > > > > Â Â Â Â Â Â Â Â { .compatible = "qcom,sm8250", , .data = &dsc_params_qcom }, > > > > > > Â Â Â Â Â Â Â Â { .compatible = "qcom,sm8350", , .data = &dsc_params_qcom }, > > > > > > Â Â Â Â Â Â Â Â { .compatible = "qcom,sm8450", , .data = &dsc_params_qcom }, > > > > > > }; > > > > > > > > > > I think this would damage the reusability of the drivers. The panel > > > > > driver does not actually care if the SoC is SM8350, sunxi-something or > > > > > RCar. > > > > > Instead it cares about host capabilities. > > > > > > > > > > I think instead we should extend mipi_dsi_host: > > > > > > > > > > #define MIPI_DSI_HOST_MODE_VIDEO BIT(0) > > > > > #define MIPI_DSI_HOST_MODE_CMDÂ BIT(1) > > > > > #define MIPI_DSI_HOST_VIDEO_SUPPORTS_COMMANDS BIT(2) > > > > > // FIXME: do we need to provide additional caps here ? > > > > > > > > > > #define MIPI_DSI_DSC_1_1 BIT(0) > > > > > #define MIPI_DSI_DSC_1_2 BIT(1) > > > > > #define MIPI_DSI_DSC_NATIVE_422 BIT(2) > > > > > #define MIPI_DSI_DSC_NATIVE_420 BIT(3) > > > > > #define MIPI_DSI_DSC_FRAC_BPP BIT(4) > > > > > // etc. > > > > > > > > > > struct mipi_dsi_host { > > > > > Â // new fields only > > > > > Â Â unsigned long mode_flags; > > > > > Â Â unsigned long dsc_flags; > > > > > }; > > > > > > > > > > Then the panel driver can adapt itself to the host capabilities and > > > > > (possibly) select one of the internally supported DSC profiles. > > > > > > > > > > > > > I completely agree about extending mipi_dsi_host, other SoCs could reuse > > > > that and > > > > support for DSC panels would become a lot cleaner. > > > > > > Sounds good. I will wait for one or two more days (to get the possible > > > feedback on fields/flags/etc) and post an RFC patch to dri-devel. > > > > I just came across that discussion, and couldn't find those patches, did > > you ever send them? > > > > Either way, I'm not really sure it's a good idea to multiply the > > capabilities flags of the DSI host, and we should just stick to the > > spec. If the spec says that we have to support DSC while video is > > output, then that's what the panels should expect. > > Except some panels supports DSC & non-DSC, Video and Command mode, and > all that is runtime configurable. How do you handle that ? In this case, most of the constraints are going to be on the encoder still so it should be the one driving it. The panel will only care about which mode has been selected, but it shouldn't be the one driving it, and thus we still don't really need to expose the host capabilities. This is very much like HDMI: the encoder knows what the monitor is capable of, will take a decision based on its capabilities and the monitor's and will then let the monitor know. But the monitor never knows what the encoder is truly capable of, nor will it enforce something. Maxime
On 05/07/2023 16:29, Maxime Ripard wrote: > On Wed, Jul 05, 2023 at 03:05:33PM +0200, Neil Armstrong wrote: >> On 05/07/2023 14:04, Maxime Ripard wrote: >>> Hi, >>> >>> On Tue, May 30, 2023 at 03:36:04PM +0300, Dmitry Baryshkov wrote: >>>> On 30/05/2023 15:15, AngeloGioacchino Del Regno wrote: >>>>> Il 30/05/23 13:44, Dmitry Baryshkov ha scritto: >>>>>> On Tue, 30 May 2023 at 10:24, Neil Armstrong >>>>>> <neil.armstrong@linaro.org> wrote: >>>>>>> >>>>>>> Hi Marijn, Dmitry, Caleb, Jessica, >>>>>>> >>>>>>> On 29/05/2023 23:11, Marijn Suijten wrote: >>>>>>>> On 2023-05-22 04:16:20, Dmitry Baryshkov wrote: >>>>>>>> <snip> >>>>>>>>>> +Â Â if (ctx->dsi->dsc) { >>>>>>>>> >>>>>>>>> dsi->dsc is always set, thus this condition can be dropped. >>>>>>>> >>>>>>>> I want to leave room for possibly running the panel without DSC (at a >>>>>>>> lower resolution/refresh rate, or at higher power consumption if there >>>>>>>> is enough BW) by not assigning the pointer, if we get access to panel >>>>>>>> documentation: probably one of the magic commands sent in this driver >>>>>>>> controls it but we don't know which. >>>>>>> >>>>>>> I'd like to investigate if DSC should perhaps only be enabled if we >>>>>>> run non certain platforms/socs ? >>>>>>> >>>>>>> I mean, we don't know if the controller supports DSC and those >>>>>>> particular >>>>>>> DSC parameters so we should probably start adding something like : >>>>>>> >>>>>>> static drm_dsc_config dsc_params_qcom = {} >>>>>>> >>>>>>> static const struct of_device_id panel_of_dsc_params[] = { >>>>>>> Â Â Â Â Â Â Â Â { .compatible = "qcom,sm8150", , .data = &dsc_params_qcom }, >>>>>>> Â Â Â Â Â Â Â Â { .compatible = "qcom,sm8250", , .data = &dsc_params_qcom }, >>>>>>> Â Â Â Â Â Â Â Â { .compatible = "qcom,sm8350", , .data = &dsc_params_qcom }, >>>>>>> Â Â Â Â Â Â Â Â { .compatible = "qcom,sm8450", , .data = &dsc_params_qcom }, >>>>>>> }; >>>>>> >>>>>> I think this would damage the reusability of the drivers. The panel >>>>>> driver does not actually care if the SoC is SM8350, sunxi-something or >>>>>> RCar. >>>>>> Instead it cares about host capabilities. >>>>>> >>>>>> I think instead we should extend mipi_dsi_host: >>>>>> >>>>>> #define MIPI_DSI_HOST_MODE_VIDEO BIT(0) >>>>>> #define MIPI_DSI_HOST_MODE_CMDÂ BIT(1) >>>>>> #define MIPI_DSI_HOST_VIDEO_SUPPORTS_COMMANDS BIT(2) >>>>>> // FIXME: do we need to provide additional caps here ? >>>>>> >>>>>> #define MIPI_DSI_DSC_1_1 BIT(0) >>>>>> #define MIPI_DSI_DSC_1_2 BIT(1) >>>>>> #define MIPI_DSI_DSC_NATIVE_422 BIT(2) >>>>>> #define MIPI_DSI_DSC_NATIVE_420 BIT(3) >>>>>> #define MIPI_DSI_DSC_FRAC_BPP BIT(4) >>>>>> // etc. >>>>>> >>>>>> struct mipi_dsi_host { >>>>>> Â // new fields only >>>>>> Â Â unsigned long mode_flags; >>>>>> Â Â unsigned long dsc_flags; >>>>>> }; >>>>>> >>>>>> Then the panel driver can adapt itself to the host capabilities and >>>>>> (possibly) select one of the internally supported DSC profiles. >>>>>> >>>>> >>>>> I completely agree about extending mipi_dsi_host, other SoCs could reuse >>>>> that and >>>>> support for DSC panels would become a lot cleaner. >>>> >>>> Sounds good. I will wait for one or two more days (to get the possible >>>> feedback on fields/flags/etc) and post an RFC patch to dri-devel. >>> >>> I just came across that discussion, and couldn't find those patches, did >>> you ever send them? No, I got sidetracked by other issues. >>> >>> Either way, I'm not really sure it's a good idea to multiply the >>> capabilities flags of the DSI host, and we should just stick to the >>> spec. If the spec says that we have to support DSC while video is >>> output, then that's what the panels should expect. >> >> Except some panels supports DSC & non-DSC, Video and Command mode, and >> all that is runtime configurable. How do you handle that ? > > In this case, most of the constraints are going to be on the encoder > still so it should be the one driving it. The panel will only care about > which mode has been selected, but it shouldn't be the one driving it, > and thus we still don't really need to expose the host capabilities. This is an interesting perspective. This means that we can and actually have to extend the drm_display_mode with the DSI data and compression information. For example, the panel that supports all four types for the 1080p should export several modes: 1920x1080-command 1920x1080-command-DSC 1920x1080-video 1920x1080-video-DSC where video/command and DSC are some kinds of flags and/or information in the drm_display_mode? Ideally DSC also has several sub-flags, which denote what kind of configuration is supported by the DSC sink (e.g. bpp, yuv, etc). Another option would be to get this handled via the bus format negotiation, but that sounds like worse idea to me. > This is very much like HDMI: the encoder knows what the monitor is > capable of, will take a decision based on its capabilities and the > monitor's and will then let the monitor know. But the monitor never > knows what the encoder is truly capable of, nor will it enforce > something. > > Maxime
On Wed, Jul 05, 2023 at 04:37:57PM +0300, Dmitry Baryshkov wrote: > > > > > > > > Either way, I'm not really sure it's a good idea to multiply the > > > > capabilities flags of the DSI host, and we should just stick to the > > > > spec. If the spec says that we have to support DSC while video is > > > > output, then that's what the panels should expect. > > > > > > Except some panels supports DSC & non-DSC, Video and Command mode, and > > > all that is runtime configurable. How do you handle that ? > > > > In this case, most of the constraints are going to be on the encoder > > still so it should be the one driving it. The panel will only care about > > which mode has been selected, but it shouldn't be the one driving it, > > and thus we still don't really need to expose the host capabilities. > > This is an interesting perspective. This means that we can and actually have > to extend the drm_display_mode with the DSI data and compression > information. I wouldn't extend drm_display_mode, but extending one of the state structures definitely. We already have some extra variables in drm_connector_state for HDMI, I don't think it would be a big deal to add a few for MIPI-DSI. We also floated the idea for a while to create bus-specific states, with helpers to match. Maybe it would be a good occasion to start doing it? > For example, the panel that supports all four types for the 1080p should > export several modes: > > 1920x1080-command > 1920x1080-command-DSC > 1920x1080-video > 1920x1080-video-DSC > > where video/command and DSC are some kinds of flags and/or information in > the drm_display_mode? Ideally DSC also has several sub-flags, which denote > what kind of configuration is supported by the DSC sink (e.g. bpp, yuv, > etc). So we have two things to do, right? We need to expose what the panel can take (ie, EDID for HDMI), and then we need to tell it what we picked (infoframes). We already express the former in mipi_dsi_device, so we could extend the flags stored there. And then, we need to tie what the DSI host chose to a given atomic state so the panel knows what was picked and how it should set everything up. > Another option would be to get this handled via the bus format negotiation, > but that sounds like worse idea to me. Yeah, I'm not really fond of the format negociation stuff either. Maxime
On Wed, 5 Jul 2023 at 17:24, Maxime Ripard <mripard@kernel.org> wrote: > > On Wed, Jul 05, 2023 at 04:37:57PM +0300, Dmitry Baryshkov wrote: > > > > > > > > > > Either way, I'm not really sure it's a good idea to multiply the > > > > > capabilities flags of the DSI host, and we should just stick to the > > > > > spec. If the spec says that we have to support DSC while video is > > > > > output, then that's what the panels should expect. > > > > > > > > Except some panels supports DSC & non-DSC, Video and Command mode, and > > > > all that is runtime configurable. How do you handle that ? > > > > > > In this case, most of the constraints are going to be on the encoder > > > still so it should be the one driving it. The panel will only care about > > > which mode has been selected, but it shouldn't be the one driving it, > > > and thus we still don't really need to expose the host capabilities. > > > > This is an interesting perspective. This means that we can and actually have > > to extend the drm_display_mode with the DSI data and compression > > information. > > I wouldn't extend drm_display_mode, but extending one of the state > structures definitely. > > We already have some extra variables in drm_connector_state for HDMI, > I don't think it would be a big deal to add a few for MIPI-DSI. > > We also floated the idea for a while to create bus-specific states, with > helpers to match. Maybe it would be a good occasion to start doing it? > > > For example, the panel that supports all four types for the 1080p should > > export several modes: > > > > 1920x1080-command > > 1920x1080-command-DSC > > 1920x1080-video > > 1920x1080-video-DSC > > > > where video/command and DSC are some kinds of flags and/or information in > > the drm_display_mode? Ideally DSC also has several sub-flags, which denote > > what kind of configuration is supported by the DSC sink (e.g. bpp, yuv, > > etc). > > So we have two things to do, right? We need to expose what the panel can > take (ie, EDID for HDMI), and then we need to tell it what we picked > (infoframes). > > We already express the former in mipi_dsi_device, so we could extend the > flags stored there. > > And then, we need to tie what the DSI host chose to a given atomic state > so the panel knows what was picked and how it should set everything up. This is definitely something we need. Marijn has been stuck with the panels that support different models ([1]). Would you prefer a separate API for this kind of information or abusing atomic_enable() is fine from your point of view? My vote would be for going with existing operations, with the slight fear of ending up with another DSI-specific hack (like pre_enable_prev_first). > > > Another option would be to get this handled via the bus format negotiation, > > but that sounds like worse idea to me. > > Yeah, I'm not really fond of the format negociation stuff either. [1] https://lore.kernel.org/linux-arm-msm/20230521-drm-panels-sony-v1-8-541c341d6bee@somainline.org/
On 05/07/2023 16:24, Maxime Ripard wrote: > On Wed, Jul 05, 2023 at 04:37:57PM +0300, Dmitry Baryshkov wrote: >>>>> >>>>> Either way, I'm not really sure it's a good idea to multiply the >>>>> capabilities flags of the DSI host, and we should just stick to the >>>>> spec. If the spec says that we have to support DSC while video is >>>>> output, then that's what the panels should expect. >>>> >>>> Except some panels supports DSC & non-DSC, Video and Command mode, and >>>> all that is runtime configurable. How do you handle that ? >>> >>> In this case, most of the constraints are going to be on the encoder >>> still so it should be the one driving it. The panel will only care about >>> which mode has been selected, but it shouldn't be the one driving it, >>> and thus we still don't really need to expose the host capabilities. >> >> This is an interesting perspective. This means that we can and actually have >> to extend the drm_display_mode with the DSI data and compression >> information. > > I wouldn't extend drm_display_mode, but extending one of the state > structures definitely. > > We already have some extra variables in drm_connector_state for HDMI, > I don't think it would be a big deal to add a few for MIPI-DSI. > > We also floated the idea for a while to create bus-specific states, with > helpers to match. Maybe it would be a good occasion to start doing it? > >> For example, the panel that supports all four types for the 1080p should >> export several modes: >> >> 1920x1080-command >> 1920x1080-command-DSC >> 1920x1080-video >> 1920x1080-video-DSC >> >> where video/command and DSC are some kinds of flags and/or information in >> the drm_display_mode? Ideally DSC also has several sub-flags, which denote >> what kind of configuration is supported by the DSC sink (e.g. bpp, yuv, >> etc). > > So we have two things to do, right? We need to expose what the panel can > take (ie, EDID for HDMI), and then we need to tell it what we picked > (infoframes). > > We already express the former in mipi_dsi_device, so we could extend the > flags stored there. > > And then, we need to tie what the DSI host chose to a given atomic state > so the panel knows what was picked and how it should set everything up. Yep this looks like a good plan Neil > >> Another option would be to get this handled via the bus format negotiation, >> but that sounds like worse idea to me. > > Yeah, I'm not really fond of the format negociation stuff either. > > Maxime
On Wed, Jul 05, 2023 at 06:20:13PM +0300, Dmitry Baryshkov wrote: > On Wed, 5 Jul 2023 at 17:24, Maxime Ripard <mripard@kernel.org> wrote: > > > > On Wed, Jul 05, 2023 at 04:37:57PM +0300, Dmitry Baryshkov wrote: > > > > > > > > > > > > Either way, I'm not really sure it's a good idea to multiply the > > > > > > capabilities flags of the DSI host, and we should just stick to the > > > > > > spec. If the spec says that we have to support DSC while video is > > > > > > output, then that's what the panels should expect. > > > > > > > > > > Except some panels supports DSC & non-DSC, Video and Command mode, and > > > > > all that is runtime configurable. How do you handle that ? > > > > > > > > In this case, most of the constraints are going to be on the encoder > > > > still so it should be the one driving it. The panel will only care about > > > > which mode has been selected, but it shouldn't be the one driving it, > > > > and thus we still don't really need to expose the host capabilities. > > > > > > This is an interesting perspective. This means that we can and actually have > > > to extend the drm_display_mode with the DSI data and compression > > > information. > > > > I wouldn't extend drm_display_mode, but extending one of the state > > structures definitely. > > > > We already have some extra variables in drm_connector_state for HDMI, > > I don't think it would be a big deal to add a few for MIPI-DSI. > > > > We also floated the idea for a while to create bus-specific states, with > > helpers to match. Maybe it would be a good occasion to start doing it? > > > > > For example, the panel that supports all four types for the 1080p should > > > export several modes: > > > > > > 1920x1080-command > > > 1920x1080-command-DSC > > > 1920x1080-video > > > 1920x1080-video-DSC > > > > > > where video/command and DSC are some kinds of flags and/or information in > > > the drm_display_mode? Ideally DSC also has several sub-flags, which denote > > > what kind of configuration is supported by the DSC sink (e.g. bpp, yuv, > > > etc). > > > > So we have two things to do, right? We need to expose what the panel can > > take (ie, EDID for HDMI), and then we need to tell it what we picked > > (infoframes). > > > > We already express the former in mipi_dsi_device, so we could extend the > > flags stored there. > > > > And then, we need to tie what the DSI host chose to a given atomic state > > so the panel knows what was picked and how it should set everything up. > > This is definitely something we need. Marijn has been stuck with the > panels that support different models ([1]). > > Would you prefer a separate API for this kind of information or > abusing atomic_enable() is fine from your point of view? > > My vote would be for going with existing operations, with the slight > fear of ending up with another DSI-specific hack (like > pre_enable_prev_first). I don't think we can get away without getting access to the atomic_state from the panel at least. Choosing one setup over another is likely going to depend on the mode, and that's only available in the state. We don't have to go the whole way though and create the sub-classes of drm_connector_state, but I think we should at least provide it to the panel. What do you think of creating a new set of atomic_* callbacks for panels, call that new set of functions from msm and start from there? Maxime
On 05/07/2023 19:53, Maxime Ripard wrote: > On Wed, Jul 05, 2023 at 06:20:13PM +0300, Dmitry Baryshkov wrote: >> On Wed, 5 Jul 2023 at 17:24, Maxime Ripard <mripard@kernel.org> wrote: >>> >>> On Wed, Jul 05, 2023 at 04:37:57PM +0300, Dmitry Baryshkov wrote: >>>>>>> >>>>>>> Either way, I'm not really sure it's a good idea to multiply the >>>>>>> capabilities flags of the DSI host, and we should just stick to the >>>>>>> spec. If the spec says that we have to support DSC while video is >>>>>>> output, then that's what the panels should expect. >>>>>> >>>>>> Except some panels supports DSC & non-DSC, Video and Command mode, and >>>>>> all that is runtime configurable. How do you handle that ? >>>>> >>>>> In this case, most of the constraints are going to be on the encoder >>>>> still so it should be the one driving it. The panel will only care about >>>>> which mode has been selected, but it shouldn't be the one driving it, >>>>> and thus we still don't really need to expose the host capabilities. >>>> >>>> This is an interesting perspective. This means that we can and actually have >>>> to extend the drm_display_mode with the DSI data and compression >>>> information. >>> >>> I wouldn't extend drm_display_mode, but extending one of the state >>> structures definitely. >>> >>> We already have some extra variables in drm_connector_state for HDMI, >>> I don't think it would be a big deal to add a few for MIPI-DSI. >>> >>> We also floated the idea for a while to create bus-specific states, with >>> helpers to match. Maybe it would be a good occasion to start doing it? >>> >>>> For example, the panel that supports all four types for the 1080p should >>>> export several modes: >>>> >>>> 1920x1080-command >>>> 1920x1080-command-DSC >>>> 1920x1080-video >>>> 1920x1080-video-DSC >>>> >>>> where video/command and DSC are some kinds of flags and/or information in >>>> the drm_display_mode? Ideally DSC also has several sub-flags, which denote >>>> what kind of configuration is supported by the DSC sink (e.g. bpp, yuv, >>>> etc). >>> >>> So we have two things to do, right? We need to expose what the panel can >>> take (ie, EDID for HDMI), and then we need to tell it what we picked >>> (infoframes). >>> >>> We already express the former in mipi_dsi_device, so we could extend the >>> flags stored there. >>> >>> And then, we need to tie what the DSI host chose to a given atomic state >>> so the panel knows what was picked and how it should set everything up. >> >> This is definitely something we need. Marijn has been stuck with the >> panels that support different models ([1]). >> >> Would you prefer a separate API for this kind of information or >> abusing atomic_enable() is fine from your point of view? >> >> My vote would be for going with existing operations, with the slight >> fear of ending up with another DSI-specific hack (like >> pre_enable_prev_first). > > I don't think we can get away without getting access to the atomic_state > from the panel at least. > > Choosing one setup over another is likely going to depend on the mode, > and that's only available in the state. > > We don't have to go the whole way though and create the sub-classes of > drm_connector_state, but I think we should at least provide it to the > panel. > > What do you think of creating a new set of atomic_* callbacks for > panels, call that new set of functions from msm and start from there? We are (somewhat) bound by the panel_bridge, but yeah, it seems possible.
On Wed, Jul 05, 2023 at 11:09:40PM +0300, Dmitry Baryshkov wrote: > On 05/07/2023 19:53, Maxime Ripard wrote: > > On Wed, Jul 05, 2023 at 06:20:13PM +0300, Dmitry Baryshkov wrote: > > > On Wed, 5 Jul 2023 at 17:24, Maxime Ripard <mripard@kernel.org> wrote: > > > > > > > > On Wed, Jul 05, 2023 at 04:37:57PM +0300, Dmitry Baryshkov wrote: > > > > > > > > > > > > > > > > Either way, I'm not really sure it's a good idea to multiply the > > > > > > > > capabilities flags of the DSI host, and we should just stick to the > > > > > > > > spec. If the spec says that we have to support DSC while video is > > > > > > > > output, then that's what the panels should expect. > > > > > > > > > > > > > > Except some panels supports DSC & non-DSC, Video and Command mode, and > > > > > > > all that is runtime configurable. How do you handle that ? > > > > > > > > > > > > In this case, most of the constraints are going to be on the encoder > > > > > > still so it should be the one driving it. The panel will only care about > > > > > > which mode has been selected, but it shouldn't be the one driving it, > > > > > > and thus we still don't really need to expose the host capabilities. > > > > > > > > > > This is an interesting perspective. This means that we can and actually have > > > > > to extend the drm_display_mode with the DSI data and compression > > > > > information. > > > > > > > > I wouldn't extend drm_display_mode, but extending one of the state > > > > structures definitely. > > > > > > > > We already have some extra variables in drm_connector_state for HDMI, > > > > I don't think it would be a big deal to add a few for MIPI-DSI. > > > > > > > > We also floated the idea for a while to create bus-specific states, with > > > > helpers to match. Maybe it would be a good occasion to start doing it? > > > > > > > > > For example, the panel that supports all four types for the 1080p should > > > > > export several modes: > > > > > > > > > > 1920x1080-command > > > > > 1920x1080-command-DSC > > > > > 1920x1080-video > > > > > 1920x1080-video-DSC > > > > > > > > > > where video/command and DSC are some kinds of flags and/or information in > > > > > the drm_display_mode? Ideally DSC also has several sub-flags, which denote > > > > > what kind of configuration is supported by the DSC sink (e.g. bpp, yuv, > > > > > etc). > > > > > > > > So we have two things to do, right? We need to expose what the panel can > > > > take (ie, EDID for HDMI), and then we need to tell it what we picked > > > > (infoframes). > > > > > > > > We already express the former in mipi_dsi_device, so we could extend the > > > > flags stored there. > > > > > > > > And then, we need to tie what the DSI host chose to a given atomic state > > > > so the panel knows what was picked and how it should set everything up. > > > > > > This is definitely something we need. Marijn has been stuck with the > > > panels that support different models ([1]). > > > > > > Would you prefer a separate API for this kind of information or > > > abusing atomic_enable() is fine from your point of view? > > > > > > My vote would be for going with existing operations, with the slight > > > fear of ending up with another DSI-specific hack (like > > > pre_enable_prev_first). > > > > I don't think we can get away without getting access to the atomic_state > > from the panel at least. > > > > Choosing one setup over another is likely going to depend on the mode, > > and that's only available in the state. > > > > We don't have to go the whole way though and create the sub-classes of > > drm_connector_state, but I think we should at least provide it to the > > panel. > > > > What do you think of creating a new set of atomic_* callbacks for > > panels, call that new set of functions from msm and start from there? > > We are (somewhat) bound by the panel_bridge, but yeah, it seems possible. Bridges have access to the atomic state already, so it's another place to plumb this through but I guess it would still be doable? Maxime
On 06/07/2023 09:24, Maxime Ripard wrote: > On Wed, Jul 05, 2023 at 11:09:40PM +0300, Dmitry Baryshkov wrote: >> On 05/07/2023 19:53, Maxime Ripard wrote: >>> On Wed, Jul 05, 2023 at 06:20:13PM +0300, Dmitry Baryshkov wrote: >>>> On Wed, 5 Jul 2023 at 17:24, Maxime Ripard <mripard@kernel.org> wrote: >>>>> >>>>> On Wed, Jul 05, 2023 at 04:37:57PM +0300, Dmitry Baryshkov wrote: >>>>>>>>> >>>>>>>>> Either way, I'm not really sure it's a good idea to multiply the >>>>>>>>> capabilities flags of the DSI host, and we should just stick to the >>>>>>>>> spec. If the spec says that we have to support DSC while video is >>>>>>>>> output, then that's what the panels should expect. >>>>>>>> >>>>>>>> Except some panels supports DSC & non-DSC, Video and Command mode, and >>>>>>>> all that is runtime configurable. How do you handle that ? >>>>>>> >>>>>>> In this case, most of the constraints are going to be on the encoder >>>>>>> still so it should be the one driving it. The panel will only care about >>>>>>> which mode has been selected, but it shouldn't be the one driving it, >>>>>>> and thus we still don't really need to expose the host capabilities. >>>>>> >>>>>> This is an interesting perspective. This means that we can and actually have >>>>>> to extend the drm_display_mode with the DSI data and compression >>>>>> information. >>>>> >>>>> I wouldn't extend drm_display_mode, but extending one of the state >>>>> structures definitely. >>>>> >>>>> We already have some extra variables in drm_connector_state for HDMI, >>>>> I don't think it would be a big deal to add a few for MIPI-DSI. >>>>> >>>>> We also floated the idea for a while to create bus-specific states, with >>>>> helpers to match. Maybe it would be a good occasion to start doing it? >>>>> >>>>>> For example, the panel that supports all four types for the 1080p should >>>>>> export several modes: >>>>>> >>>>>> 1920x1080-command >>>>>> 1920x1080-command-DSC >>>>>> 1920x1080-video >>>>>> 1920x1080-video-DSC >>>>>> >>>>>> where video/command and DSC are some kinds of flags and/or information in >>>>>> the drm_display_mode? Ideally DSC also has several sub-flags, which denote >>>>>> what kind of configuration is supported by the DSC sink (e.g. bpp, yuv, >>>>>> etc). >>>>> >>>>> So we have two things to do, right? We need to expose what the panel can >>>>> take (ie, EDID for HDMI), and then we need to tell it what we picked >>>>> (infoframes). >>>>> >>>>> We already express the former in mipi_dsi_device, so we could extend the >>>>> flags stored there. >>>>> >>>>> And then, we need to tie what the DSI host chose to a given atomic state >>>>> so the panel knows what was picked and how it should set everything up. >>>> >>>> This is definitely something we need. Marijn has been stuck with the >>>> panels that support different models ([1]). >>>> >>>> Would you prefer a separate API for this kind of information or >>>> abusing atomic_enable() is fine from your point of view? >>>> >>>> My vote would be for going with existing operations, with the slight >>>> fear of ending up with another DSI-specific hack (like >>>> pre_enable_prev_first). >>> >>> I don't think we can get away without getting access to the atomic_state >>> from the panel at least. >>> >>> Choosing one setup over another is likely going to depend on the mode, >>> and that's only available in the state. >>> >>> We don't have to go the whole way though and create the sub-classes of >>> drm_connector_state, but I think we should at least provide it to the >>> panel. >>> >>> What do you think of creating a new set of atomic_* callbacks for >>> panels, call that new set of functions from msm and start from there? >> >> We are (somewhat) bound by the panel_bridge, but yeah, it seems possible. > > Bridges have access to the atomic state already, so it's another place > to plumb this through but I guess it would still be doable? It's definitely doable, but I fear we won't be able to test most of the panel drivers, should we introduce a new atomic set of panel callbacks ? Or shall be simply move the "new" panel driver supporting atomic to bridge and only use panel_bridge for basic panels ? Neil > > Maxime
On Thu, Jul 06, 2023 at 09:33:15AM +0200, Neil Armstrong wrote: > On 06/07/2023 09:24, Maxime Ripard wrote: > > On Wed, Jul 05, 2023 at 11:09:40PM +0300, Dmitry Baryshkov wrote: > > > On 05/07/2023 19:53, Maxime Ripard wrote: > > > > On Wed, Jul 05, 2023 at 06:20:13PM +0300, Dmitry Baryshkov wrote: > > > > > On Wed, 5 Jul 2023 at 17:24, Maxime Ripard <mripard@kernel.org> wrote: > > > > > > > > > > > > On Wed, Jul 05, 2023 at 04:37:57PM +0300, Dmitry Baryshkov wrote: > > > > > > > > > > > > > > > > > > > > Either way, I'm not really sure it's a good idea to multiply the > > > > > > > > > > capabilities flags of the DSI host, and we should just stick to the > > > > > > > > > > spec. If the spec says that we have to support DSC while video is > > > > > > > > > > output, then that's what the panels should expect. > > > > > > > > > > > > > > > > > > Except some panels supports DSC & non-DSC, Video and Command mode, and > > > > > > > > > all that is runtime configurable. How do you handle that ? > > > > > > > > > > > > > > > > In this case, most of the constraints are going to be on the encoder > > > > > > > > still so it should be the one driving it. The panel will only care about > > > > > > > > which mode has been selected, but it shouldn't be the one driving it, > > > > > > > > and thus we still don't really need to expose the host capabilities. > > > > > > > > > > > > > > This is an interesting perspective. This means that we can and actually have > > > > > > > to extend the drm_display_mode with the DSI data and compression > > > > > > > information. > > > > > > > > > > > > I wouldn't extend drm_display_mode, but extending one of the state > > > > > > structures definitely. > > > > > > > > > > > > We already have some extra variables in drm_connector_state for HDMI, > > > > > > I don't think it would be a big deal to add a few for MIPI-DSI. > > > > > > > > > > > > We also floated the idea for a while to create bus-specific states, with > > > > > > helpers to match. Maybe it would be a good occasion to start doing it? > > > > > > > > > > > > > For example, the panel that supports all four types for the 1080p should > > > > > > > export several modes: > > > > > > > > > > > > > > 1920x1080-command > > > > > > > 1920x1080-command-DSC > > > > > > > 1920x1080-video > > > > > > > 1920x1080-video-DSC > > > > > > > > > > > > > > where video/command and DSC are some kinds of flags and/or information in > > > > > > > the drm_display_mode? Ideally DSC also has several sub-flags, which denote > > > > > > > what kind of configuration is supported by the DSC sink (e.g. bpp, yuv, > > > > > > > etc). > > > > > > > > > > > > So we have two things to do, right? We need to expose what the panel can > > > > > > take (ie, EDID for HDMI), and then we need to tell it what we picked > > > > > > (infoframes). > > > > > > > > > > > > We already express the former in mipi_dsi_device, so we could extend the > > > > > > flags stored there. > > > > > > > > > > > > And then, we need to tie what the DSI host chose to a given atomic state > > > > > > so the panel knows what was picked and how it should set everything up. > > > > > > > > > > This is definitely something we need. Marijn has been stuck with the > > > > > panels that support different models ([1]). > > > > > > > > > > Would you prefer a separate API for this kind of information or > > > > > abusing atomic_enable() is fine from your point of view? > > > > > > > > > > My vote would be for going with existing operations, with the slight > > > > > fear of ending up with another DSI-specific hack (like > > > > > pre_enable_prev_first). > > > > > > > > I don't think we can get away without getting access to the atomic_state > > > > from the panel at least. > > > > > > > > Choosing one setup over another is likely going to depend on the mode, > > > > and that's only available in the state. > > > > > > > > We don't have to go the whole way though and create the sub-classes of > > > > drm_connector_state, but I think we should at least provide it to the > > > > panel. > > > > > > > > What do you think of creating a new set of atomic_* callbacks for > > > > panels, call that new set of functions from msm and start from there? > > > > > > We are (somewhat) bound by the panel_bridge, but yeah, it seems possible. > > > > Bridges have access to the atomic state already, so it's another place > > to plumb this through but I guess it would still be doable? > > It's definitely doable, but I fear we won't be able to test most of the > panel drivers, should we introduce a new atomic set of panel callbacks ? That was my original intent yeah :) Creating an atomic_enable/disable/ etc. and then switch drm_panel_enable() to take the state (and fixing up all the callers), or create a drm_panel_enable_atomic() function. The latter is probably simpler, something like: int drm_panel_enable_atomic(struct drm_panel *panel, struct drm_atomic_state *state) { struct drm_panel_funcs *funcs = panel->funcs; if (funcs->atomic_enable) return funcs->atomic_enable(panel, state); return funcs->enable(panel); } And we should probably mention that it supersedes/deprecates drm_panel_enable. We've switched most of the other atomic hooks to take the full drm_atomic_state so I'd prefer to use it. However, for it to be somewhat useful we'd need to have access to the connector assigned to that panel. drm_panel doesn't store the drm_connector it's connected to at all, and of_drm_find_panel() doesn't take it as an argument so we can't fill it when we retrieve it either. So I guess we can go for: - Create a new set of atomic hooks - Create a new set of functions to call those hooks, that we would document as deprecating the former functions. Those functions would take a pointer to the drm_connector_state of the drm_connector it's connected to. - We add a TODO item to add a pointer to the connector in drm_panel - We add a TODO item that depend on the first one to switch the new functions and hooks to drm_atomic_state - We add a TODO item to convert callers of drm_panel_enable et al. to our new functions. It should work in all setups, paves a nice way forward and documents the trade-offs we had to take and eventually address. And without creating a dependency on 30+ patches series. Does it sound like a plan? > Or shall be simply move the "new" panel driver supporting atomic to bridge > and only use panel_bridge for basic panels ? I don't think we can expect panel_bridge to be used all the time any time soon, so I'd rather avoid to rely on it. Maxime
On 06/07/2023 09:59, Maxime Ripard wrote: > On Thu, Jul 06, 2023 at 09:33:15AM +0200, Neil Armstrong wrote: >> On 06/07/2023 09:24, Maxime Ripard wrote: >>> On Wed, Jul 05, 2023 at 11:09:40PM +0300, Dmitry Baryshkov wrote: >>>> On 05/07/2023 19:53, Maxime Ripard wrote: >>>>> On Wed, Jul 05, 2023 at 06:20:13PM +0300, Dmitry Baryshkov wrote: >>>>>> On Wed, 5 Jul 2023 at 17:24, Maxime Ripard <mripard@kernel.org> wrote: >>>>>>> >>>>>>> On Wed, Jul 05, 2023 at 04:37:57PM +0300, Dmitry Baryshkov wrote: >>>>>>>>>>> >>>>>>>>>>> Either way, I'm not really sure it's a good idea to multiply the >>>>>>>>>>> capabilities flags of the DSI host, and we should just stick to the >>>>>>>>>>> spec. If the spec says that we have to support DSC while video is >>>>>>>>>>> output, then that's what the panels should expect. >>>>>>>>>> >>>>>>>>>> Except some panels supports DSC & non-DSC, Video and Command mode, and >>>>>>>>>> all that is runtime configurable. How do you handle that ? >>>>>>>>> >>>>>>>>> In this case, most of the constraints are going to be on the encoder >>>>>>>>> still so it should be the one driving it. The panel will only care about >>>>>>>>> which mode has been selected, but it shouldn't be the one driving it, >>>>>>>>> and thus we still don't really need to expose the host capabilities. >>>>>>>> >>>>>>>> This is an interesting perspective. This means that we can and actually have >>>>>>>> to extend the drm_display_mode with the DSI data and compression >>>>>>>> information. >>>>>>> >>>>>>> I wouldn't extend drm_display_mode, but extending one of the state >>>>>>> structures definitely. >>>>>>> >>>>>>> We already have some extra variables in drm_connector_state for HDMI, >>>>>>> I don't think it would be a big deal to add a few for MIPI-DSI. >>>>>>> >>>>>>> We also floated the idea for a while to create bus-specific states, with >>>>>>> helpers to match. Maybe it would be a good occasion to start doing it? >>>>>>> >>>>>>>> For example, the panel that supports all four types for the 1080p should >>>>>>>> export several modes: >>>>>>>> >>>>>>>> 1920x1080-command >>>>>>>> 1920x1080-command-DSC >>>>>>>> 1920x1080-video >>>>>>>> 1920x1080-video-DSC >>>>>>>> >>>>>>>> where video/command and DSC are some kinds of flags and/or information in >>>>>>>> the drm_display_mode? Ideally DSC also has several sub-flags, which denote >>>>>>>> what kind of configuration is supported by the DSC sink (e.g. bpp, yuv, >>>>>>>> etc). >>>>>>> >>>>>>> So we have two things to do, right? We need to expose what the panel can >>>>>>> take (ie, EDID for HDMI), and then we need to tell it what we picked >>>>>>> (infoframes). >>>>>>> >>>>>>> We already express the former in mipi_dsi_device, so we could extend the >>>>>>> flags stored there. >>>>>>> >>>>>>> And then, we need to tie what the DSI host chose to a given atomic state >>>>>>> so the panel knows what was picked and how it should set everything up. >>>>>> >>>>>> This is definitely something we need. Marijn has been stuck with the >>>>>> panels that support different models ([1]). >>>>>> >>>>>> Would you prefer a separate API for this kind of information or >>>>>> abusing atomic_enable() is fine from your point of view? >>>>>> >>>>>> My vote would be for going with existing operations, with the slight >>>>>> fear of ending up with another DSI-specific hack (like >>>>>> pre_enable_prev_first). >>>>> >>>>> I don't think we can get away without getting access to the atomic_state >>>>> from the panel at least. >>>>> >>>>> Choosing one setup over another is likely going to depend on the mode, >>>>> and that's only available in the state. >>>>> >>>>> We don't have to go the whole way though and create the sub-classes of >>>>> drm_connector_state, but I think we should at least provide it to the >>>>> panel. >>>>> >>>>> What do you think of creating a new set of atomic_* callbacks for >>>>> panels, call that new set of functions from msm and start from there? >>>> >>>> We are (somewhat) bound by the panel_bridge, but yeah, it seems possible. >>> >>> Bridges have access to the atomic state already, so it's another place >>> to plumb this through but I guess it would still be doable? >> >> It's definitely doable, but I fear we won't be able to test most of the >> panel drivers, should we introduce a new atomic set of panel callbacks ? > > That was my original intent yeah :) > > Creating an atomic_enable/disable/ etc. and then switch > drm_panel_enable() to take the state (and fixing up all the callers), or > create a drm_panel_enable_atomic() function. > > The latter is probably simpler, something like: > > int drm_panel_enable_atomic(struct drm_panel *panel, > struct drm_atomic_state *state) > { > struct drm_panel_funcs *funcs = panel->funcs; > > if (funcs->atomic_enable) > return funcs->atomic_enable(panel, state); > > return funcs->enable(panel); > } > > And we should probably mention that it supersedes/deprecates > drm_panel_enable. > > We've switched most of the other atomic hooks to take the full > drm_atomic_state so I'd prefer to use it. However, for it to be somewhat > useful we'd need to have access to the connector assigned to that panel. > > drm_panel doesn't store the drm_connector it's connected to at all, and > of_drm_find_panel() doesn't take it as an argument so we can't fill it > when we retrieve it either. > > So I guess we can go for: > > - Create a new set of atomic hooks > > - Create a new set of functions to call those hooks, that we would > document as deprecating the former functions. Those functions would > take a pointer to the drm_connector_state of the drm_connector it's > connected to. > > - We add a TODO item to add a pointer to the connector in drm_panel > > - We add a TODO item that depend on the first one to switch the new > functions and hooks to drm_atomic_state > > - We add a TODO item to convert callers of drm_panel_enable et al. to > our new functions. > > It should work in all setups, paves a nice way forward and documents the > trade-offs we had to take and eventually address. And without creating a > dependency on 30+ patches series. > > Does it sound like a plan? Yep that looks a fine plan to start of > >> Or shall be simply move the "new" panel driver supporting atomic to bridge >> and only use panel_bridge for basic panels ? > > I don't think we can expect panel_bridge to be used all the time any > time soon, so I'd rather avoid to rely on it. Ack > > Maxime
Sending in a bunch of almost-finished (hence RFC) Sony panel drivers upon drm/msm request, to further discussions around DSC panels and DSI pclk calculations. This brings support for the following Sony Xperia devices: - Xperia XZ3 (DSC) - Xperia 1 (DSC) - Xperia 5 - Xperia 10 II (shared with Xperia 5) - Xperia 5 II (DSC, 120Hz) And, since the XZ3 already has all the DT in place to enable the panels on its smaller XZ2(c) siblings, an extra patch is included to add the new compatible string and properties to this device .dts. DTS for other boards/platforms will come later, after cleaning up preliminary patches (e.g. DPU catalog additions, SoC/board DTS for the MDSS subsystem, etc). - File- and compatible names: None of my downstream sources describe the product name of the panels used here; in few cases the Display-IC is known but for the Xperia XZ3 Xperia 1 we have to make-do with a vendor name only. Naming suggestions definitely welcome; i.e. I'm especially not fond of sony-griffin-samsung.c :) - Panels/drivers featuring multiple modes As brought up earlier in #freedreno drm_panel drivers can provide multiple modes, but the selected mode is never communicated back to the panel. This either has to be added to the driver, or the drivers in question have to be converted to drm_bridges (suggestion from #freedreno). That should allow us to select a mode at runtime, and downstream even defines "fast paths" to switch from one mode to the next (e.g. when only adjusting the refresh rate) without powering the panel off and on again, which we can hopefully support too. For now the choice between either mode has been hardcoded behind a static const bool. - DSC Not much to discuss here except that "it works" after piecing together various series on the lists. No dependencies to make this series apply/compile, though. - pclk The brunt of the discussion is around getting these command mode panels functioning at their desired 60Hz or 120Hz refresh rate without tearing/artifacts, and without hacks. Part of that discussion around DSC-specific timing adjustments is happening in [1], but the sofef01 (non-DSC) Driver-IC is also struggling on the Xperia 5 specifically, as outlined in that specific patch. That is currently "addressed" with a "porch hack" but should probably have some sort of overhead / transfer time taken into account in the MSM DSI driver. Let me know what the best place is to collate all the relevant info (links to downstream panel DTS, outcomes with different patches and tweaks, etc). A new fd.o drm/msm issue? [1]: https://gitlab.freedesktop.org/drm/msm/-/issues/24#note_1917707 Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org> --- Marijn Suijten (10): drm/panel: Clean up SOFEF00 config dependencies dt-bindings: display: panel: Describe Sony Xperia XZ3's LGD panel drm/panel: Add LGD panel driver for Sony Xperia XZ3 arm64: dts: qcom: sdm845-akatsuki: Configure OLED panel dt-bindings: display: panel: Describe Samsung SOFEF01-M Display-IC drm/panel/samsung-sofef01: Add panel driver for Sony Xperia 5 / 10 II dt-bindings: display: panel: Describe Samsung SOFEF03-M Display-IC drm/panel/samsung-sofef03: Add panel driver for Sony Xperia 5 II dt-bindings: display: panel: Describe Sony Xperia 1 display drm/panel/sony-griffin-samsung: Add panel driver for Sony Xperia 1 .../bindings/display/panel/samsung,sofef01-m.yaml | 109 ++++++ .../bindings/display/panel/samsung,sofef03-m.yaml | 73 ++++ .../bindings/display/panel/sony,akatsuki-lgd.yaml | 71 ++++ .../display/panel/sony,griffin-samsung.yaml | 73 ++++ .../dts/qcom/sdm845-sony-xperia-tama-akatsuki.dts | 9 + drivers/gpu/drm/panel/Kconfig | 52 ++- drivers/gpu/drm/panel/Makefile | 4 + drivers/gpu/drm/panel/panel-samsung-sofef01.c | 360 ++++++++++++++++++ drivers/gpu/drm/panel/panel-samsung-sofef03.c | 423 +++++++++++++++++++++ drivers/gpu/drm/panel/panel-sony-akatsuki-lgd.c | 362 ++++++++++++++++++ drivers/gpu/drm/panel/panel-sony-griffin-samsung.c | 410 ++++++++++++++++++++ 11 files changed, 1945 insertions(+), 1 deletion(-) --- base-commit: dbd91ef4e91c1ce3a24429f5fb3876b7a0306733 change-id: 20230521-drm-panels-sony-3c5ac3218427 Best regards,